[PATCH] drm/gem: Check for valid formats

[Maíra Canal]

On 1/5/23 15:54, Simon Ser wrote:
> On Thursday, January 5th, 2023 at 19:48, Maíra Canal <mcanal at igalia.com> wrote:
> 
>> On 1/5/23 15:36, Simon Ser wrote:
>>
>>> On Thursday, January 5th, 2023 at 19:30, Maíra Canal mcanal at igalia.com wrote:
>>>
>>>>>>> I think to really make sure we have consensus it'd be good to extend this
>>>>>>> to a series which removes all the callers to drm_any_plane_has_format()
>>>>>>> from the various drivers, and then unexports that helper. That way your
>>>>>>> series here will have more eyes on it :-)
>>>>>>
>>>>>> I took a look at the callers to drm_any_plane_has_format() and there are only
>>>>>> 3 callers (amdgpu, i915 and vmwgfx). They all use drm_any_plane_has_format()
>>>>>> before calling drm_framebuffer_init(). So, I'm not sure I could remove
>>>>>> drm_any_plane_has_format() from those drivers. Maybe adding this same check
>>>>>> to drm_gem_fb_init() and refactor the drivers to make them use drm_gem_fb_init(),
>>>>>> but I guess this would be part of a different series.
>>>>>
>>>>> Well vmwgfx still not yet using gem afaik, so that doesn't work.
>>>>>
>>>>> But why can't we move the modifier check int drm_framebuffer_init()?
>>>>> That's kinda where it probably should be anyway, there's nothing gem
>>>>> bo specific in the code you're adding.
>>>>
>>>> I'm not sure if this would correct the problem that I was trying to fix initially.
>>>> I was trying to make sure that the drivers pass on the
>>>> igt at kms_addfb_basic@addfb25-bad-modifier IGT test by making sure that the addfb
>>>> ioctl returns the error.
>>>>
>>>> By moving the modifier check to the drm_framebuffer_init(), the test would still
>>>> fail.
>>>
>>> Hm, why is that? The ADDFB2 IOCTL impl calls drm_framebuffer_init().
>>
>>
>>  From what I can see, ADDFB2 IOCTL calls drm_internal_framebuffer_create() [1],
>> then drm_internal_framebuffer_create() calls the fb_create hook [2]. I'm not sure
>> here ADDFB2 implicitly calls drm_framebuffer_init(), but I'm probably missing
>> something.
> 
> Right, but then all drivers will call drm_framebuffer_init() somewhere
> in their fb_create hook. For instance amdgpu calls it in
> amdgpu_display_gem_fb_verify_and_init().

I see. Thanks for explaining me the relation between the functions. I will send a v3
of this patch, introducing the check on drm_framebuffer_init().

Best Regards,
- Maíra Canal