| Message ID | 20211222090552.25972-2-jose.exposito89@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add missing format_mod_supported functions | expand |
On Wednesday, December 22nd, 2021 at 10:05, José Expósito <jose.exposito89@gmail.com> wrote: > Make "create_in_format_blob" behave as documented. LGTM, nice! Reviewed-by: Simon Ser <contact@emersion.fr> CC Ville just in case
On Wed, Dec 22, 2021 at 10:05:47AM +0100, José Expósito wrote: > The documentation for "drm_plane_funcs.format_mod_supported" reads: > > This *optional* hook is used for the DRM to determine if the given > format/modifier combination is valid for the plane. This allows the > DRM to generate the correct format bitmask (which formats apply to > which modifier), and to validate modifiers at atomic_check time. > > *If not present*, then any modifier in the plane's modifier > list is allowed with any of the plane's formats. > > However, where the function is not present, an invalid IN_FORMATS blob > property with modifiers but no formats is exposed to user-space. > > This breaks the latest Weston [1]. For testing purposes, I extracted the > affected code to a standalone program [2]. > > Make "create_in_format_blob" behave as documented. > > [1] https://gitlab.freedesktop.org/wayland/weston/-/blob/9.0/libweston/backend-drm/kms.c#L431 > [2] https://github.com/JoseExposito/drm-sandbox/blob/main/in_formats.c > > Signed-off-by: José Expósito <jose.exposito89@gmail.com> > --- > drivers/gpu/drm/drm_plane.c | 8 ++------ > 1 file changed, 2 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 82afb854141b..c1186b7215ee 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -202,17 +202,13 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane > > memcpy(formats_ptr(blob_data), plane->format_types, formats_size); > > - /* If we can't determine support, just bail */ > - if (!plane->funcs->format_mod_supported) > - goto done; > - > mod = modifiers_ptr(blob_data); > for (i = 0; i < plane->modifier_count; i++) { > for (j = 0; j < plane->format_count; j++) { > - if (plane->funcs->format_mod_supported(plane, > + if (!plane->funcs->format_mod_supported || > + plane->funcs->format_mod_supported(plane, > plane->format_types[j], > plane->modifiers[i])) { So instead of skipping the whole loop you just skip doing anything inside the loop? Can't see how that achieves anything at all. https://patchwork.freedesktop.org/series/83018/ is what I had in mind earlier but no one reviewed it and the discussion veered off track IIRC.
On Thursday, December 23rd, 2021 at 12:56, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > - /* If we can't determine support, just bail */ > > - if (!plane->funcs->format_mod_supported) > > - goto done; > > - > > mod = modifiers_ptr(blob_data); > > for (i = 0; i < plane->modifier_count; i++) { > > for (j = 0; j < plane->format_count; j++) { > > - if (plane->funcs->format_mod_supported(plane, > > + if (!plane->funcs->format_mod_supported || > > + plane->funcs->format_mod_supported(plane, > > plane->format_types[j], > > plane->modifiers[i])) { > > So instead of skipping the whole loop you just skip doing anything > inside the loop? Can't see how that achieves anything at all. No, the check is skipped when the function isn't populated by the driver.
On Thu, Dec 23, 2021 at 01:42:32PM +0000, Simon Ser wrote: > On Thursday, December 23rd, 2021 at 12:56, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > > > > - /* If we can't determine support, just bail */ > > > - if (!plane->funcs->format_mod_supported) > > > - goto done; > > > - > > > mod = modifiers_ptr(blob_data); > > > for (i = 0; i < plane->modifier_count; i++) { > > > for (j = 0; j < plane->format_count; j++) { > > > - if (plane->funcs->format_mod_supported(plane, > > > + if (!plane->funcs->format_mod_supported || > > > + plane->funcs->format_mod_supported(plane, > > > plane->format_types[j], > > > plane->modifiers[i])) { > > > > So instead of skipping the whole loop you just skip doing anything > > inside the loop? Can't see how that achieves anything at all. > > No, the check is skipped when the function isn't populated by the driver. Ah right. So we advertise all modifiers in that case. Looks like drm_plane_check_pixel_format() does support that model, so seems OK. Another related thing that might be worth checking is whether drivers generally do anything to validate the modifiers in the addfb2 ioctl. Looks like i915 and amdgpu are the only ones to use drm_any_plane_has_format() for that, so all the other drivers must either be checking it manually (or they're just potentially broken when handed unexpected modifiers by evil userspace).
Thanks for your reviews :) I'll wait a couple of days to see if somebody else wants to comment and I'll send v3 adding the reviewed by tags and fixing the compiler warning. On Thu, Dec 23, 2021 at 05:03:19PM +0200, Ville Syrjälä wrote: > Another related thing that might be worth checking is whether > drivers generally do anything to validate the modifiers in > the addfb2 ioctl. Looks like i915 and amdgpu are the only ones > to use drm_any_plane_has_format() for that, so all the other > drivers must either be checking it manually (or they're just > potentially broken when handed unexpected modifiers by evil > userspace). I'm pretty new to this subsystem, so please correct me if I'm wrong, but after looking into a couple of drivers I think you are right, this check is missing in some drivers. This possible bug reminds me of this ToDo task [1]: > 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. I had a look to the Raspberry Pi driver (mainly because I'm trying to understand it) and it looks like the check is missing. Other drivers, for example Mali, are checking the format modifier manually. I'll try to do some actual testing during Christmas and see how it goes. José Expósito [1] https://www.kernel.org/doc/html/latest/gpu/todo.html#drm-framebuffer-funcs-and-drm-mode-config-funcs-fb-create-cleanup
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 82afb854141b..c1186b7215ee 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -202,17 +202,13 @@ static int create_in_format_blob(struct drm_device *dev, struct drm_plane *plane memcpy(formats_ptr(blob_data), plane->format_types, formats_size); - /* If we can't determine support, just bail */ - if (!plane->funcs->format_mod_supported) - goto done; - mod = modifiers_ptr(blob_data); for (i = 0; i < plane->modifier_count; i++) { for (j = 0; j < plane->format_count; j++) { - if (plane->funcs->format_mod_supported(plane, + if (!plane->funcs->format_mod_supported || + plane->funcs->format_mod_supported(plane, plane->format_types[j], plane->modifiers[i])) { - mod->formats |= 1ULL << j; } }
The documentation for "drm_plane_funcs.format_mod_supported" reads: This *optional* hook is used for the DRM to determine if the given format/modifier combination is valid for the plane. This allows the DRM to generate the correct format bitmask (which formats apply to which modifier), and to validate modifiers at atomic_check time. *If not present*, then any modifier in the plane's modifier list is allowed with any of the plane's formats. However, where the function is not present, an invalid IN_FORMATS blob property with modifiers but no formats is exposed to user-space. This breaks the latest Weston [1]. For testing purposes, I extracted the affected code to a standalone program [2]. Make "create_in_format_blob" behave as documented. [1] https://gitlab.freedesktop.org/wayland/weston/-/blob/9.0/libweston/backend-drm/kms.c#L431 [2] https://github.com/JoseExposito/drm-sandbox/blob/main/in_formats.c Signed-off-by: José Expósito <jose.exposito89@gmail.com> --- drivers/gpu/drm/drm_plane.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)