| Message ID | 20230103125322.855089-1-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/gem: Check for valid formats | expand |
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);
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 >
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.
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 >
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 > > >
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
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 >
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 > > > >
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
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);
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 >
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
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
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().
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
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().
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
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?
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
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
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);
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(-)