Hi Daniel, Thanks for the extra clarification. On Tue, 27 Apr 2021 at 13:22, Daniel Vetter <daniel@xxxxxxxx> wrote: > > On Tue, Apr 27, 2021 at 12:32:19PM +0100, Emil Velikov wrote: > > 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? > > This is a must only for drivers supporting modifiers, not all drivers. > Hence the check in the if. I did add WARN_ON for the combos that get stuff > wrong though (like only supply one side of the modifier info, not both). > Hmm you're spot on - the arm/malidp patch threw me off for a minute. > > 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. > > Well that currently only exists to avoid walking the plane list (which we > need to do for validation that all planes are the same). It's quite tricky > code for tiny benefit, so I don't think it's worth it trying to remove > allow_fb_modifiers completely. > Pardon if I'm saying something painfully silly - it's been a while since I've looked closely at KMS. >From some grepping around, removing ::allow_fb_modifiers would be OK although it's a secondary goal. It feels like the bigger win will be simpler modifier handling in DRM. In particular, one could always "inject" the linear modifier within drm_universal_plane_init() and always expose DRM_CAP_ADDFB2_MODIFIERS. Some drivers mxsfb, mgag200, stm and likely others already advertise the CAP, even though they seemingly lack any modifiers. The linear/invalid cargo-cult to drm_universal_plane_init() seems strong and this series adds even more. Another plus of always exposing the CAP, is that one could mandate (or nuke) optional .format_mod_supported that you/Ville discussed earlier[1]. Currently things are weird, since it's required to create IN_FORMAT blob, yet drivers lack it while simultaneously exposing the CAP to userspace. One such example is exynos... Although recently it recently dropped `allow_fb_modifiers = true` and there are no modifiers passed to drm_universal_plane_init(), so the CAP is no longer supported. Inki you might want to check, if that broke your userspace. Tl:Dr: There _might_ be value in simplifying the modifier handling _after_ these fixes land. [1] https://lore.kernel.org/dri-devel/CAKMK7uGNP5us8KFffnPwq7g4b0-B2q-m7deqz_rPHtCrh_qUTw@xxxxxxxxxxxxxx/ > > 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. > > Yeah it's a bit disappointing. > > > With the WARN_ON() added or s/must/should/ in the documentation, the series is: > > With my clarification, can you please recheck whether as-is it's not > correct? > Indeed - with the series as-is my RB stands. Thanks -Emil _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
