Nouveau - Sep 2022 - [PATCH v2 10/41] drm/modes: Add a function to generate analog display modes

Hi,

On Wed, Aug 31, 2022 at 10:14:28AM +0200, Geert Uytterhoeven
wrote:
> > > +enum drm_mode_analog {
> > > +    DRM_MODE_ANALOG_NTSC,
> > > +    DRM_MODE_ANALOG_PAL,
> > > +};
> >
> > Using "NTSC" and "PAL" to describe the 50Hz and
60Hz analog TV modes is common,
> > but strictly speaking a misnomer. Those are color encoding systems,
and your
> > patchset fully supports lesser used, but standard encodings for those
(e.g.
> > PAL-M for 60Hz and SECAM for 50Hz). I'd propose switching to some
more neutral
> > naming scheme. Some ideas:
> >
> > - DRM_MODE_ANALOG_60_HZ / DRM_MODE_ANALOG_50_HZ (after standard
refresh rate)
> > - DRM_MODE_ANALOG_525_LINES / DRM_MODE_ANALOG_625_LINES (after
standard line
> >   count)
> 
> IMHO these are bad names, as e.g. VGA640x480 at 60 is also analog, using
> 60 Hz and 525 lines.  Add "TV" to the name?
> 
> > - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after corresponding
ITU System
> >   Letter Designations)
> 
> Or DRM_MODE_ITU_*?
> But given the long list of letters, this looks fragile to me.
Does it matter at all? It's an internal API that isn't exposed at all.
I'd rather have a common name that everyone can understand in this case
rather than a *perfect* name where most will scratch their head
wondering what it's about.

Maxime
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 228 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/nouveau/attachments/20220905/efa5286f/attachment-0001.sig>

Hi Maxime,

W dniu 5.09.2022 o 15:32, Maxime Ripard pisze:
> Hi,
>
> On Wed, Aug 31, 2022 at 10:14:28AM +0200, Geert Uytterhoeven wrote:
>>>> +enum drm_mode_analog {
>>>> +??? DRM_MODE_ANALOG_NTSC,
>>>> +??? DRM_MODE_ANALOG_PAL,
>>>> +};
>>>
>>> Using "NTSC" and "PAL" to describe the 50Hz and
60Hz analog TV modes is common,
>>> but strictly speaking a misnomer. Those are color encoding systems,
and your
>>> patchset fully supports lesser used, but standard encodings for
those (e.g.
>>> PAL-M for 60Hz and SECAM for 50Hz). I'd propose switching to
some more neutral
>>> naming scheme. Some ideas:
>>>
>>> - DRM_MODE_ANALOG_60_HZ / DRM_MODE_ANALOG_50_HZ (after standard
refresh rate)
>>> - DRM_MODE_ANALOG_525_LINES / DRM_MODE_ANALOG_625_LINES (after
standard line
>>>?? count)
>>
>> IMHO these are bad names, as e.g. VGA640x480 at 60 is also analog,
using
>> 60 Hz and 525 lines.? Add "TV" to the name?
>>
>>> - DRM_MODE_ANALOG_JM / DRM_MODE_ANALOG_BDGHIKLN (after
corresponding ITU System
>>>?? Letter Designations)
>>
>> Or DRM_MODE_ITU_*?
>> But given the long list of letters, this looks fragile to me.
>
> Does it matter at all? It's an internal API that isn't exposed at
all.
> I'd rather have a common name that everyone can understand in this case
> rather than a *perfect* name where most will scratch their head
> wondering what it's about.
You may have a point. But in that case, maybe it'd make sense to at least
add
a short comment explaining what do you mean by "NTSC" and
"PAL" in this context?

Best regards,
Mateusz Kwiatkowski