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
WARNING: multiple messages have this Message-ID (diff)
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: [Intel-gfx] [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 _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2021-04-27 11:32 UTC|newest] Thread overview: 50+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-04-27 9:20 [PATCH 1/8] drm/arm: Don't set allow_fb_modifiers explicitly Daniel Vetter 2021-04-27 9:20 ` [Intel-gfx] " Daniel Vetter 2021-04-27 9:20 ` [PATCH 2/8] drm/arm/malidp: Always list modifiers Daniel Vetter 2021-04-27 9:20 ` [Intel-gfx] " Daniel Vetter 2021-04-27 9:20 ` Daniel Vetter 2021-04-27 15:41 ` Liviu Dudau 2021-04-27 15:41 ` [Intel-gfx] " Liviu Dudau 2021-04-27 15:41 ` Liviu Dudau 2021-04-27 9:20 ` [PATCH 3/8] drm/i915: Don't set allow_fb_modifiers explicitly Daniel Vetter 2021-04-27 9:20 ` [Intel-gfx] " Daniel Vetter 2021-04-27 9:20 ` [PATCH 4/8] drm/msm/dpu1: " Daniel Vetter 2021-04-27 9:20 ` [Intel-gfx] " Daniel Vetter 2021-04-27 9:20 ` [PATCH 5/8] drm/msm/mdp4: Fix modifier support enabling Daniel Vetter 2021-04-27 9:20 ` [Intel-gfx] " Daniel Vetter 2021-04-27 9:20 ` Daniel Vetter 2021-04-27 9:20 ` [PATCH 6/8] drm/nouveau: Don't set allow_fb_modifiers explicitly Daniel Vetter 2021-04-27 9:20 ` [Intel-gfx] " Daniel Vetter 2021-04-27 9:20 ` Daniel Vetter 2021-04-27 9:20 ` [Nouveau] " Daniel Vetter 2021-04-27 9:20 ` [PATCH 7/8] drm/stm: " Daniel Vetter 2021-04-27 9:20 ` [Intel-gfx] " Daniel Vetter 2021-04-27 9:20 ` Daniel Vetter 2021-04-29 19:35 ` Philippe CORNU - foss 2021-04-29 19:35 ` [Intel-gfx] " Philippe CORNU - foss 2021-04-29 19:35 ` Philippe CORNU - foss 2021-04-27 9:20 ` [PATCH 8/8] drm/modifiers: Enforce consistency between the cap an IN_FORMATS Daniel Vetter 2021-04-27 9:20 ` [Intel-gfx] " Daniel Vetter 2021-04-27 9:30 ` Simon Ser 2021-04-27 9:30 ` [Intel-gfx] " Simon Ser 2021-04-27 11:32 ` Emil Velikov [this message] 2021-04-27 11:32 ` Emil Velikov 2021-04-27 12:22 ` Daniel Vetter 2021-04-27 12:22 ` [Intel-gfx] " Daniel Vetter 2021-05-04 14:10 ` Emil Velikov 2021-05-04 14:10 ` [Intel-gfx] " Emil Velikov 2021-05-04 14:58 ` Simon Ser 2021-05-04 14:58 ` [Intel-gfx] " Simon Ser 2021-05-04 15:48 ` Emil Velikov 2021-05-04 15:48 ` [Intel-gfx] " Emil Velikov 2021-05-04 13:38 ` Pekka Paalanen 2021-05-04 13:38 ` [Intel-gfx] " Pekka Paalanen 2021-05-05 19:24 ` Daniel Vetter 2021-05-05 19:24 ` [Intel-gfx] " Daniel Vetter 2021-05-06 9:55 ` Daniel Vetter 2021-05-06 9:55 ` [Intel-gfx] " Daniel Vetter 2021-04-27 10:58 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for series starting with [1/8] drm/arm: Don't set allow_fb_modifiers explicitly Patchwork 2021-04-27 11:28 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork 2021-04-27 15:25 ` [PATCH 1/8] " Liviu Dudau 2021-04-27 15:25 ` [Intel-gfx] " Liviu Dudau 2021-04-27 15:46 ` [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/8] " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=CACvgo51rQJmHc1K-MSq-WLZkwVt34MY73csgEyxorrYsKPwQiA@mail.gmail.com \ --to=emil.l.velikov@gmail.com \ --cc=airlied@linux.ie \ --cc=daniel.vetter@ffwll.ch \ --cc=daniel.vetter@intel.com \ --cc=dri-devel@lists.freedesktop.org \ --cc=intel-gfx@lists.freedesktop.org \ --cc=maxime@cerno.tech \ --cc=pekka.paalanen@collabora.com \ --cc=tzimmermann@suse.de \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.