Re: [PATCH] drm/gem: Check for valid formats

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi

Am 05.01.23 um 16:24 schrieb Daniel Vetter:
On Tue, Jan 03, 2023 at 02:16:30PM +0100, Thomas Zimmermann wrote:
Hi,

thanks for the follow-up patch.

Am 03.01.23 um 13:53 schrieb Maíra Canal:
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().

Moreover, 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 therefor reject valid formats.

Suggested-by: Thomas Zimmermann <tzimmermann@xxxxxxx>
Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>
---
   Documentation/gpu/todo.rst                   | 7 ++-----
   drivers/gpu/drm/drm_gem_framebuffer_helper.c | 9 +++++++++
   2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst
index 1f8a5ebe188e..68bdafa0284f 100644
--- a/Documentation/gpu/todo.rst
+++ b/Documentation/gpu/todo.rst
@@ -276,11 +276,8 @@ Various hold-ups:
   - Need to switch to drm_fbdev_generic_setup(), otherwise a lot of the custom fb
     setup code can't be deleted.
-- Many drivers wrap drm_gem_fb_create() only to check for valid formats. For
-  atomic drivers we could check for valid formats by calling
-  drm_plane_check_pixel_format() against all planes, and pass if any plane
-  supports the format. For non-atomic that's not possible since like the format
-  list for the primary plane is fake and we'd therefor reject valid formats.
+- Need to switch to drm_gem_fb_create(), as now drm_gem_fb_create() checks for
+  valid formats for atomic drivers.
   - Many drivers subclass drm_framebuffer, we'd need a embedding compatible
     version of the varios drm_gem_fb_create functions. Maybe called
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index e93533b86037..b8a615a138cd 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -9,6 +9,7 @@
   #include <linux/module.h>
   #include <drm/drm_damage_helper.h>
+#include <drm/drm_drv.h>
   #include <drm/drm_fourcc.h>
   #include <drm/drm_framebuffer.h>
   #include <drm/drm_gem.h>
@@ -164,6 +165,14 @@ int drm_gem_fb_init_with_funcs(struct drm_device *dev,
   		return -EINVAL;
   	}
+	if (drm_drv_uses_atomic_modeset(dev) &&
+	    !drm_any_plane_has_format(dev, mode_cmd->pixel_format,
+				      mode_cmd->modifier[0])) {

Because this is a generic helper, it has to handle the odd cases as well.
Here we cannot assume modifier[0], because there's a modifier for each pixel
plane in multi-plane formats. (That's a different type of plane than the
struct plane we're passing in.) If one combination isn't supported, the
helper should fail.

This was a mistake in the addfb2 design, we later rectified that all
modifiers must match. This is because the modifier itsel can change the
number of planes (for aux compression planes and stuff like that).

The full drm format description is the (drm_fourcc, drm_format_modifier)
pair.

Does this mean that only modifier[0] will ever contain a valid value, OR that all modifiers[i] have to contain the same value?

Best regards
Thomas


This should be documented somewhere already, if not, good idea to update
addfb docs (or make them happen in the first place).
-Daniel


We get the number of pixel planes from the format info. So the correct
implementation is something like that

if (drm_drv_uses_atomic_modeset())) {
	for (i = 0; i < info->num_planes; ++i) {
         	if (!drm_any_plane_has_format(dev, pixel_format, \
				modifier[i]) {
			drm_dbg_kms(dev, "error msg");
			return -EINVAL;
		}
         }
}


+		drm_dbg(dev, "Unsupported pixel format %p4cc / modifier 0x%llx\n",

drm_dbg() is for drivers. Use drm_dbg_kms() please.

Best regards
Thomas


+			&mode_cmd->pixel_format, mode_cmd->modifier[0]);
+		return -EINVAL;
+	}
+
   	for (i = 0; i < info->num_planes; i++) {
   		unsigned int width = mode_cmd->width / (i ? info->hsub : 1);
   		unsigned int height = mode_cmd->height / (i ? info->vsub : 1);

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev





--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux