Nouveau - Sep 2022 - [PATCH v2 32/41] drm/vc4: vec: Convert to the new TV mode property

Hi Noralf,

On Tue, Aug 30, 2022 at 09:01:08PM +0200, Noralf Tr?nnes
wrote:
> > +static const struct drm_prop_enum_list tv_mode_names[] = {
> 
> Maybe call it legacy_tv_mode_enums?
> 
> > 
> > +	{ VC4_VEC_TV_MODE_NTSC, "NTSC", },
> > 
> > +	{ VC4_VEC_TV_MODE_NTSC_J, "NTSC-J", },
> > 
> > +	{ VC4_VEC_TV_MODE_PAL, "PAL", },
> > 
> > +	{ VC4_VEC_TV_MODE_PAL_M, "PAL-M", },
> 
> If you use DRM_MODE_TV_MODE_* here you don't need to translate the
value
> using the switch statement in get/set property, you can use the value
> directly to get/set tv.mode.
I'm sorry, I'm not quite sure what you mean by that. If we expose the
DRM_MODE_TV_MODE_* properties there, won't that change the values the
userspace will need to use to set that property?

Maxime

Hi Maxime,

W dniu 08.09.2022 o 13:23, Maxime Ripard pisze:
> Hi Noralf,
>
> On Tue, Aug 30, 2022 at 09:01:08PM +0200, Noralf Tr?nnes wrote:
>>> +static const struct drm_prop_enum_list tv_mode_names[] = {
>>
>> Maybe call it legacy_tv_mode_enums?
>>
>>>
>>> +?? ?{ VC4_VEC_TV_MODE_NTSC, "NTSC", },
>>>
>>> +?? ?{ VC4_VEC_TV_MODE_NTSC_J, "NTSC-J", },
>>>
>>> +?? ?{ VC4_VEC_TV_MODE_PAL, "PAL", },
>>>
>>> +?? ?{ VC4_VEC_TV_MODE_PAL_M, "PAL-M", },
>>
>> If you use DRM_MODE_TV_MODE_* here you don't need to translate the
value
>> using the switch statement in get/set property, you can use the value
>> directly to get/set tv.mode.
>
> I'm sorry, I'm not quite sure what you mean by that. If we expose
the
> DRM_MODE_TV_MODE_* properties there, won't that change the values the
> userspace will need to use to set that property?
I'd just like to point out that if numerical values of these enums are your
concern, then you're (or perhaps I am ;) already breaking this by adding new
modes in patch 33/41 in this series.

And the values (and names!) added by that patch (33/41) don't match those
currently used by the downstream version
(https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/gpu/drm/vc4/vc4_vec.c).
If any userspace software is manipulating this property, it's most likely
targeting the downstream code. But since you're not aiming for consistency
with
that, I was under the impression that compatibility isn't a concern.

Best regards,
Mateusz Kwiatkowski

Den 08.09.2022 13.23, skrev Maxime Ripard:
> Hi Noralf,
> 
> On Tue, Aug 30, 2022 at 09:01:08PM +0200, Noralf Tr?nnes wrote:
>>> +static const struct drm_prop_enum_list tv_mode_names[] = {
>>
>> Maybe call it legacy_tv_mode_enums?
>>
>>>
>>> +	{ VC4_VEC_TV_MODE_NTSC, "NTSC", },
>>>
>>> +	{ VC4_VEC_TV_MODE_NTSC_J, "NTSC-J", },
>>>
>>> +	{ VC4_VEC_TV_MODE_PAL, "PAL", },
>>>
>>> +	{ VC4_VEC_TV_MODE_PAL_M, "PAL-M", },
>>
>> If you use DRM_MODE_TV_MODE_* here you don't need to translate the
value
>> using the switch statement in get/set property, you can use the value
>> directly to get/set tv.mode.
> 
> I'm sorry, I'm not quite sure what you mean by that. If we expose
the
> DRM_MODE_TV_MODE_* properties there, won't that change the values the
> userspace will need to use to set that property?
> 
You're right ofc, I forgot that the enum values are also UABI.

Noralf.