[Intel-gfx] [PATCH 8/8] drm/modifiers: Enforce consistency between the cap an IN_FORMATS

[Emil Velikov]

Hi Daniel,

On Tue, 27 Apr 2021 at 10:20, Daniel Vetter <daniel.vetter at ffwll.ch> 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 at collabora.com>

HTH
-Emil