Re: [PATCH 8/8] drm/modifiers: Enforce consistency between the cap an IN_FORMATS

From: Emil Velikov <emil.l.velikov@gmail.com>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Pekka Paalanen <pekka.paalanen@collabora.com>,
	David Airlie <airlied@linux.ie>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Maxime Ripard <maxime@cerno.tech>,
	Thomas Zimmermann <tzimmermann@suse.de>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 8/8] drm/modifiers: Enforce consistency between the cap an IN_FORMATS
Date: Tue, 27 Apr 2021 12:32:19 +0100	[thread overview]
Message-ID: <CACvgo51rQJmHc1K-MSq-WLZkwVt34MY73csgEyxorrYsKPwQiA@mail.gmail.com> (raw)
In-Reply-To: <20210427092018.832258-8-daniel.vetter@ffwll.ch>

Hi Daniel,

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

HTH
-Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel