Hi Daniel,
On Tue, 27 Apr 2021 at 10:20, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> @@ -360,6 +373,9 @@ static int __drm_universal_plane_init(struct drm_device *dev,
> * drm_universal_plane_init() to let the DRM managed resource infrastructure
> * take care of cleanup and deallocation.
> *
> + * Drivers supporting modifiers must set @format_modifiers on all their planes,
> + * even those that only support DRM_FORMAT_MOD_LINEAR.
> + *
The comment says "must", yet we have an "if (format_modifiers)" in the codebase.
Shouldn't we add a WARN_ON() + return -EINVAL (or similar) so people
can see and fix their drivers?
As a follow-up one could even go a step further, by erroring out when
the driver hasn't provided valid modifier(s) and even removing
config::allow_fb_modifiers all together.
Although for stable - this series + WARN_ON (no return since it might
break buggy drivers) sounds good.
> @@ -909,6 +909,8 @@ struct drm_mode_config {
> * @allow_fb_modifiers:
> *
> * Whether the driver supports fb modifiers in the ADDFB2.1 ioctl call.
> + * Note that drivers should not set this directly, it is automatically
> + * set in drm_universal_plane_init().
> *
> * IMPORTANT:
> *
The new note and the existing IMPORTANT are in a weird mix.
Quoting the latter since it doesn't show in the diff.
If this is set the driver must fill out the full implicit modifier
information in their &drm_mode_config_funcs.fb_create hook for legacy
userspace which does not set modifiers. Otherwise the GETFB2 ioctl is
broken for modifier aware userspace.
In particular:
As the new note says "don't set it" and the existing note one says "if
it's set". Yet no drivers do "if (config->allow_fb_modifiers)".
Sadly, nothing comes to mind atm wrt alternative wording.
With the WARN_ON() added or s/must/should/ in the documentation, the series is:
Reviewed-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>
HTH
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
