drm/gem: Check for valid formats
authorMaíra Canal <mcanal@igalia.com>
Wed, 12 Apr 2023 14:29:23 +0000 (11:29 -0300)
committerMaíra Canal <mairacanal@riseup.net>
Wed, 19 Apr 2023 23:19:27 +0000 (20:19 -0300)
commitc91acda3a380bcaf41b67c8fbab668ef8ddf91c3
treef34d52d2bcdefb116649e3acfb242f33ffb98ba0
parent96c7c2f4d5bd94b15fe63448c087f01607b56f4a
drm/gem: Check for valid formats

Currently, drm_gem_fb_create() doesn't check if the pixel format is
supported, which can lead to the acceptance of invalid pixel formats
e.g. the acceptance of invalid modifiers. Therefore, add a check for
valid formats on drm_gem_fb_create().

Note that this check is only valid for atomic drivers, because, for
non-atomic drivers, checking drm_any_plane_has_format() is not
possible since the format list for the primary plane is fake, and we'd
therefore reject valid formats.

Adding this check to drm_gem_fb_create() will guarantee that the
igt@kms_addfb_basic@addfb25-bad-modifier IGT test passes for drivers
using this callback.

This commit is a recapture of a series sent a while ago. Initially,
I sent a patch [1] similar to this one in which I introduced the
format check to drm_gem_fb_create().

Based on the feedback on the patch, I placed the check inside
framebuffer_check() [2] so that it wouldn't be needed to hit any
driver-specific code path when the check fails. Therefore, we could
remove the check from the specific drivers (i915, amdgpu, and vmwgfx).

But, with some new feedback, it was shown that introducing this check
inside framebuffer_check() is problematic for the i915 driver [3].
For the i915 driver, in the legacy case, in which we don't get the
modifier from the userspace, i915's fb_create hook computes the right
modifier, which isn't necessarily linear.  Therefore, if we check the
modifier before that point, we might get wrong answers.

So, I kept the check inside the i915 driver and removed the check from
amdgpu and vmwgfx [4]. But, this yet hasn't solved the i915 problem [5].

As we cannot add the check inside framebuffer_check() without
affecting the i915 behavior, this commit went back to the original
patch. This way we can guarantee a more uniform behavior from the
drivers that use the drm_gem_fb_create() callback.

[1] https://lore.kernel.org/dri-devel/20230103125322.855089-1-mcanal@igalia.com/T/
[2] https://lore.kernel.org/dri-devel/20230109105807.18172-1-mcanal@igalia.com/T/
[3] https://lore.kernel.org/dri-devel/Y8AAdW2y7zN7DCUZ@intel.com/
[4] https://lore.kernel.org/dri-devel/20230113112743.188486-1-mcanal@igalia.com/T/
[5] https://lore.kernel.org/dri-devel/Y8FXWvEhO7GCRKVJ@intel.com/

Signed-off-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Maíra Canal <mairacanal@riseup.net>
Link: https://patchwork.freedesktop.org/patch/msgid/20230412142923.136707-1-mcanal@igalia.com
Documentation/gpu/todo.rst
drivers/gpu/drm/drm_gem_framebuffer_helper.c