[PATCH] drm/gem: Check for valid formats

[PATCH] drm/gem: Check for valid formats

[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@suse.de>
Signed-off-by: Maíra Canal <mcanal@igalia.com>
---
 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])) {
+		drm_dbg(dev, "Unsupported pixel format %p4cc / modifier 0x%llx\n",
+			&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);
-- 
2.38.1

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3986 bytes --]

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@suse.de>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>   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.

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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[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@suse.de>
> > Signed-off-by: Maíra Canal <mcanal@igalia.com>
> > ---
> >   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.

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




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 5143 bytes --]

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@suse.de>
>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>>> ---
>>>    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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Daniel Vetter]

On Thu, 5 Jan 2023 at 16:43, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>
> 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@suse.de>
> >>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> >>> ---
> >>>    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?

All must have the same, addfb checks for that. See framebuffer_check()
-Daniel

>
> 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



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Rob Clark]

drive-by thought/concern, what are the odds that there is some wayland
compositor out there that creates an fb for every window surface, even
if it later decides to composite on the GPU because the display does
not support the format?  It seems like there is a non-zero chance of
breaking userspace..

BR,
-R

On Tue, Jan 3, 2023 at 4:55 AM Maíra Canal <mcanal@igalia.com> wrote:
>
> 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@suse.de>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>  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])) {
> +               drm_dbg(dev, "Unsupported pixel format %p4cc / modifier 0x%llx\n",
> +                       &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);
> --
> 2.38.1
>

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

[Simon Ser]

On Tuesday, January 3rd, 2023 at 23:46, Rob Clark <robdclark@gmail.com> wrote:

> drive-by thought/concern, what are the odds that there is some wayland
> compositor out there that creates an fb for every window surface, even
> if it later decides to composite on the GPU because the display does
> not support the format? It seems like there is a non-zero chance of
> breaking userspace..

User-space is prepared for ADDFB2 to fail. Other drivers already reject
IOCTLs with FB parameters not supported for scanout.

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

[Maíra Canal]

On 1/3/23 19:46, Rob Clark wrote:
> drive-by thought/concern, what are the odds that there is some wayland
> compositor out there that creates an fb for every window surface, even
> if it later decides to composite on the GPU because the display does
> not support the format?  It seems like there is a non-zero chance of
> breaking userspace..
>

As Simon pointed out, drivers like i915 and AMDGPU already reject IOCTLs
with invalid parameters, as you can see on [1] and [2], so this patch
would make the behavior more consistent between the drivers. That said,
I don't believe that this patch would break userspace, as userspace
already needs to handle with the existing drivers.

[1] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/i915/display/intel_fb.c#n1917
[2] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c#n1124

Best Regards,
- Maíra Canal
  
> BR,
> -R
> 

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

[Rob Clark]

On Tue, Jan 3, 2023 at 5:11 PM Maíra Canal <mcanal@igalia.com> wrote:
>
> On 1/3/23 19:46, Rob Clark wrote:
> > drive-by thought/concern, what are the odds that there is some wayland
> > compositor out there that creates an fb for every window surface, even
> > if it later decides to composite on the GPU because the display does
> > not support the format?  It seems like there is a non-zero chance of
> > breaking userspace..
> >
>
> As Simon pointed out, drivers like i915 and AMDGPU already reject IOCTLs
> with invalid parameters, as you can see on [1] and [2], so this patch
> would make the behavior more consistent between the drivers. That said,
> I don't believe that this patch would break userspace, as userspace
> already needs to handle with the existing drivers.

ok, maybe this won't be a problem then

BR,
-R

> [1] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/i915/display/intel_fb.c#n1917
> [2] https://cgit.freedesktop.org/drm/drm-misc/tree/drivers/gpu/drm/amd/amdgpu/amdgpu_display.c#n1124
>
> Best Regards,
> - Maíra Canal
>
> > BR,
> > -R
> >
>

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

[Daniel Vetter]

On Tue, Jan 03, 2023 at 09:53:23AM -0300, Maíra Canal wrote:
> 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@suse.de>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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 :-)
-Daniel

> ---
>  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])) {
> +		drm_dbg(dev, "Unsupported pixel format %p4cc / modifier 0x%llx\n",
> +			&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);
> -- 
> 2.38.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Maíra Canal]

On 1/5/23 12:26, Daniel Vetter wrote:
> On Tue, Jan 03, 2023 at 09:53:23AM -0300, Maíra Canal wrote:
>> 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@suse.de>
>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> 
> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 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.

Best Regards,
- Maíra Canal

> -Daniel
> 

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

[Daniel Vetter]

On Thu, 5 Jan 2023 at 18:48, Maíra Canal <mcanal@igalia.com> wrote:
>
> On 1/5/23 12:26, Daniel Vetter wrote:
> > On Tue, Jan 03, 2023 at 09:53:23AM -0300, Maíra Canal wrote:
> >> 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@suse.de>
> >> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> >
> > Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> >
> > 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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Maíra Canal]

On 1/5/23 15:22, Daniel Vetter wrote:
> On Thu, 5 Jan 2023 at 18:48, Maíra Canal <mcanal@igalia.com> wrote:
>>
>> On 1/5/23 12:26, Daniel Vetter wrote:
>>> On Tue, Jan 03, 2023 at 09:53:23AM -0300, Maíra Canal wrote:
>>>> 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@suse.de>
>>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>>>
>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> 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.

Best Regards,
- Maíra Canal

> -Daniel

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

[Simon Ser]

On Thursday, January 5th, 2023 at 19:30, Maíra Canal <mcanal@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@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().

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

[Maíra Canal]

On 1/5/23 15:36, Simon Ser wrote:
> On Thursday, January 5th, 2023 at 19:30, Maíra Canal <mcanal@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@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

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

[Simon Ser]

On Thursday, January 5th, 2023 at 19:48, Maíra Canal <mcanal@igalia.com> wrote:

> On 1/5/23 15:36, Simon Ser wrote:
> 
> > On Thursday, January 5th, 2023 at 19:30, Maíra Canal mcanal@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@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().

Re: [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@igalia.com> wrote:
> 
>> On 1/5/23 15:36, Simon Ser wrote:
>>
>>> On Thursday, January 5th, 2023 at 19:30, Maíra Canal mcanal@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@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

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

[Simon Ser]

To be honest, your suggestion to put the check inside framebuffer_check()
sounds like a better idea: we wouldn't even hit any driver-specific
code-path when the check fails. Daniel, do you think there'd be an issue
with this approach?

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

[Daniel Vetter]

On Thu, Jan 05, 2023 at 08:04:13PM +0000, Simon Ser wrote:
> To be honest, your suggestion to put the check inside framebuffer_check()
> sounds like a better idea: we wouldn't even hit any driver-specific
> code-path when the check fails. Daniel, do you think there'd be an issue
> with this approach?

framebuffer_check probably even better, since it'll stop nonsense before a
driver potentially blows up. Which would be a CVE. So even better at
catching potential issues.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2335 bytes --]

Hi

Am 05.01.23 um 19:22 schrieb Daniel Vetter:
> On Thu, 5 Jan 2023 at 18:48, Maíra Canal <mcanal@igalia.com> wrote:
>>
>> On 1/5/23 12:26, Daniel Vetter wrote:
>>> On Tue, Jan 03, 2023 at 09:53:23AM -0300, Maíra Canal wrote:
>>>> 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@suse.de>
>>>> Signed-off-by: Maíra Canal <mcanal@igalia.com>
>>>
>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>
>>> 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.

There was a patchset that converted vmwgfx to GEM IIRC. It even uses 
generic fbdev emulation now, for which GEM is a hard requirement.

Best regards
Thomas

> 
> 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.
> -Daniel

-- 
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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 3381 bytes --]

Hi

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@suse.de>
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>   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])) {
> +		drm_dbg(dev, "Unsupported pixel format %p4cc / modifier 0x%llx\n",
> +			&mode_cmd->pixel_format, mode_cmd->modifier[0]);

Given Daniel's comment on modifier[0], please change this comment to 
drm_dbg_kms() and you can

Reviewed-by: Thomas Zimmermann <tzimmermann@suse.de>

to this patch.

Best regards
Thomas

> +		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

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[PATCH] drm/gem: Check for valid formats

[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.

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.

Signed-off-by: Maíra Canal <mcanal@igalia.com>
---

This patch is a recapture of a series I 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]. 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, I propose going 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/

Best Regards,
- Maíra Canal

---
 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])) {
+		drm_dbg(dev, "Unsupported pixel format %p4cc / modifier 0x%llx\n",
+			&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);
-- 
2.39.2

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

[Ville Syrjälä]

On Wed, Apr 12, 2023 at 11:29:23AM -0300, Maíra Canal wrote:
> 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.
> 
> 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.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>

Seems sane enough, assuming all the drivers using this are happy
with the baked in !modifiers -> linear assumption. That might be
a good thing to highlight in the docs.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

BTW long ago I tried to make igt check this stugg a bit
more vehemently:
https://patchwork.freedesktop.org/patch/239814/?series=46876&rev=1
Someone might want to pick that up again and see if it
cab be makde to work with more drivers than just i915.

> ---
> 
> This patch is a recapture of a series I 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]. 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, I propose going 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/
> 
> Best Regards,
> - Maíra Canal
> 
> ---
>  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])) {
> +		drm_dbg(dev, "Unsupported pixel format %p4cc / modifier 0x%llx\n",
> +			&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);
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel