| Message ID | 20211221101319.7980-2-jose.exposito89@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add missing format_mod_supported functions | expand |
Reviewed-by: Simon Ser <contact@emersion.fr>
Please ping me in a week or so if nobody objected and this isn't merged.
On Tue, 21 Dec 2021 at 13:13, José Expósito <jose.exposito89@gmail.com> wrote: > > Adding format modifiers without implementing the function > "drm_plane_funcs.format_mod_supported" exposes an invalid IN_FORMATS > blob with modifiers but no formats to user-space. I think the fix should be applied to the generic code. The docs at drm_plane.h clearly state that this callback is optional: * 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 valdiate 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. So, one should fix the core code in create_in_format_blob() to include all combinations. > > This breaks the latest Weston [1]. For testing purposes, I extracted the > affected code to a standalone program [2]. > > Make clear in the IN_FORMATS documentation that implementing > "format_mod_supported" is mandatory. format_mod_supported() being optional or mandatory should be documented in the format_mod_supported() docs, not in the IN_FORMAT docs. > > [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 | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c > index 82afb854141b..347571f8909a 100644 > --- a/drivers/gpu/drm/drm_plane.c > +++ b/drivers/gpu/drm/drm_plane.c > @@ -126,8 +126,11 @@ > * IN_FORMATS: > * Blob property which contains the set of buffer format and modifier > * pairs supported by this plane. The blob is a struct > - * drm_format_modifier_blob. Without this property the plane doesn't > - * support buffers with modifiers. Userspace cannot change this property. > + * drm_format_modifier_blob and the drm_plane_funcs.format_mod_supported > + * function must be implemented by the driver to determine if the given > + * format/modifier combination is valid for the plane. Without this property > + * the plane doesn't support buffers with modifiers. Userspace cannot change > + * this property. > * > * Note that userspace can check the &DRM_CAP_ADDFB2_MODIFIERS driver > * capability for general modifier support. If this flag is set then every > -- > 2.25.1 >
On Tuesday, December 21st, 2021 at 11:48, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> I think the fix should be applied to the generic code.
Related:
https://lore.kernel.org/dri-devel/t1g_xNE6hgj1nTQfx2UWob1wbsCAxElBvWRwNSY_EzmlEe_9WWpq8-vQKyJPK6wZY8q8BqHl-KoGwS5V91VgN8lGIl3PJt7s2fkdsRd3y70=@emersion.fr/T/#u
On Tue, 21 Dec 2021 at 13:50, Simon Ser <contact@emersion.fr> wrote: > > On Tuesday, December 21st, 2021 at 11:48, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > > > I think the fix should be applied to the generic code. > > Related: > https://lore.kernel.org/dri-devel/t1g_xNE6hgj1nTQfx2UWob1wbsCAxElBvWRwNSY_EzmlEe_9WWpq8-vQKyJPK6wZY8q8BqHl-KoGwS5V91VgN8lGIl3PJt7s2fkdsRd3y70=@emersion.fr/T/#u I'd still suggest to fix create_in_format_blob()
On Tuesday, December 21st, 2021 at 13:33, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:
> I'd still suggest to fix create_in_format_blob()
Yeah, I agree. I thought there were a good reason why create_in_format_blob()
behaves this way but can't find anything in the Git history, so fixing it to
behave as the documentation says would be best.
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index 82afb854141b..347571f8909a 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -126,8 +126,11 @@ * IN_FORMATS: * Blob property which contains the set of buffer format and modifier * pairs supported by this plane. The blob is a struct - * drm_format_modifier_blob. Without this property the plane doesn't - * support buffers with modifiers. Userspace cannot change this property. + * drm_format_modifier_blob and the drm_plane_funcs.format_mod_supported + * function must be implemented by the driver to determine if the given + * format/modifier combination is valid for the plane. Without this property + * the plane doesn't support buffers with modifiers. Userspace cannot change + * this property. * * Note that userspace can check the &DRM_CAP_ADDFB2_MODIFIERS driver * capability for general modifier support. If this flag is set then every
Adding format modifiers without implementing the function "drm_plane_funcs.format_mod_supported" exposes an invalid IN_FORMATS blob with modifiers but no formats to user-space. This breaks the latest Weston [1]. For testing purposes, I extracted the affected code to a standalone program [2]. Make clear in the IN_FORMATS documentation that implementing "format_mod_supported" is mandatory. [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 | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)