On 1/5/23 15:36, Simon Ser wrote:
On Thursday, January 5th, 2023 at 19:30, Maíra Canal <mcanal@xxxxxxxxxx> 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@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.
Also, by looking at this file, I realize that maybe it would be better to add the
check on framebuffer_check(), but I don't know how does it sound to Daniel.
[1] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/drm_framebuffer.c#n346
[2] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/drm_framebuffer.c#n321
Best Regards,
- Maíra Canal
