Message ID | 20180315184817.GC5453@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Ville Syrjälä <ville.syrjala@linux.intel.com> writes: > On Thu, Mar 15, 2018 at 08:03:44PM +0200, Ville Syrjälä wrote: >> On Thu, Mar 15, 2018 at 07:48:02PM +0200, Ville Syrjälä wrote: >> > On Thu, Mar 15, 2018 at 10:42:17AM -0700, Eric Anholt wrote: >> > > Ville Syrjala <ville.syrjala@linux.intel.com> writes: >> > > >> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > > >> > > > To make life easier for drivers, let's have the core check that the >> > > > requested pixel format is supported by at least one plane when creating >> > > > a new framebuffer. >> > > > >> > > > This eases the burden on drivers by making sure they'll never get >> > > > requests to create fbs with unsupported pixel formats. Thanks to the >> > > > new .fb_modifier() hook this check can now be done whether the request >> > > > specifies the modifier directly or driver has to deduce it from the >> > > > gem bo tiling (or via any other method really). >> > > > >> > > > v0: Accept anyformat if the driver doesn't do planes (Eric) >> > > > s/planes_have_format/any_plane_has_format/ (Eric) >> > > > Check the modifier as well since we already have a function >> > > > that does both >> > > > v3: Don't do the check in the core since we may not know the >> > > > modifier yet, instead export the function and let drivers >> > > > call it themselves >> > > > v4: Unexport the functiona and put the format_default check back >> > > > since this will again be called by the core, ie. undo v3 ;) >> > > > >> > > > Cc: Eric Anholt <eric@anholt.net> >> > > > Testcase: igt/kms_addfb_basic/expected-formats >> > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> >> > > > --- >> > > > drivers/gpu/drm/drm_framebuffer.c | 30 ++++++++++++++++++++++++++++++ >> > > > 1 file changed, 30 insertions(+) >> > > > >> > > > diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c >> > > > index 21d3d51eb261..e618a6b728d4 100644 >> > > > --- a/drivers/gpu/drm/drm_framebuffer.c >> > > > +++ b/drivers/gpu/drm/drm_framebuffer.c >> > > > @@ -152,6 +152,26 @@ static int fb_plane_height(int height, >> > > > return DIV_ROUND_UP(height, format->vsub); >> > > > } >> > > > >> > > > +static bool any_plane_has_format(struct drm_device *dev, >> > > > + u32 format, u64 modifier) >> > > > +{ >> > > > + struct drm_plane *plane; >> > > > + >> > > > + drm_for_each_plane(plane, dev) { >> > > > + /* >> > > > + * In case the driver doesn't really do >> > > > + * planes we have to accept any format here. >> > > > + */ >> > > > + if (plane->format_default) >> > > > + return true; >> > > > + >> > > > + if (drm_plane_check_pixel_format(plane, format, modifier) == 0) >> > > > + return true; >> > > > + } >> > > > + >> > > > + return false; >> > > > +} >> > > >> > > This drm_plane_check_pixel_format() will always fail for VC4's SAND >> > > modifiers or VC5's UIF modifiers, where we're using the middle 48 bits >> > > as a bit of metadata (like how we have horizontal stride passed outside >> > > of the modifier) and you can't list all of the possible values in an >> > > array on the plane. >> > >> > Hmm. drm_atomic_plane_check() etc. call this thing as well. How does >> > that manage to work currently? >> >> Maybe it doesn't. I added the modifier checks in >> >> commit 23163a7d4b032489d375099d56571371c0456980 >> Author: Ville Syrjälä <ville.syrjala@linux.intel.com> >> AuthorDate: Fri Dec 22 21:22:30 2017 +0200 >> Commit: Ville Syrjälä <ville.syrjala@linux.intel.com> >> CommitDate: Mon Feb 26 16:29:47 2018 +0200 >> >> drm: Check that the plane supports the request format+modifier combo >> >> Maybe that broke vc4? >> >> Hmm. So either we need to stop checking against the modifiers array and >> rely purely or .format_mod_supported(), or we need to somehow get the >> driver to reduce the modifier to its base form. I guess we could make >> .fb_modifier() do that and call it also for addfb with modifiers. And >> I'd need to undo some of the modifier[0] vs. deduced modifier changes >> I did to framebuffer_check(), and we'd need to preserve the original >> modifier in the request for .fb_create(). Oh, but that wouldn't allow >> returning a non-base modifier from .fb_modifuer() for the !modifiers >> case. This is turning slightly more tricky than I had hoped... >> >> I guess relying on .format_mod_supported() might be what we need to >> do. Unfortunately it does mean that the .format_mod_supported() >> implementations must be prepared for modifiers that were not >> registered with the plane. Which does feel quite a bit more >> fragile. > > And that last apporach would be annoying for i915. So I think the other > apporach is better. > > Fortunately it seems your SAND modifiers aren't upstream yet so I didn't > break them :) I had a quick look at your github and it looks like the > first apporach would work. > > Something like this is what I had in mind. Loosk like you could plug in > fourcc_mod_broadcom_mod() almost directly into .base_modifier(). I think just trusting format_mod_supported's result makes more sense. See the patch series I just sent.
diff --git a/drivers/gpu/drm/drm_plane.c b/drivers/gpu/drm/drm_plane.c index a5d1fc7e8a37..250f66d51c2c 100644 --- a/drivers/gpu/drm/drm_plane.c +++ b/drivers/gpu/drm/drm_plane.c @@ -552,6 +552,8 @@ int drm_mode_getplane(struct drm_device *dev, void *data, int drm_plane_check_pixel_format(struct drm_plane *plane, u32 format, u64 modifier) { + struct drm_device *dev = plane->dev; + u64 base_modifier; unsigned int i; for (i = 0; i < plane->format_count; i++) { @@ -564,8 +566,13 @@ int drm_plane_check_pixel_format(struct drm_plane *plane, if (!plane->modifier_count) return 0; + if (dev->mode_config.funcs->base_modifier) + base_modifier = dev->mode_config.funcs->base_modifier(dev, modifier); + else + base_modifier = modifier; + for (i = 0; i < plane->modifier_count; i++) { - if (modifier == plane->modifiers[i]) + if (base_modifier == plane->modifiers[i]) break; } if (i == plane->modifier_count) diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index c74aed292b58..2ff2438263ef 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -45,6 +45,19 @@ struct drm_display_mode; * involve drivers. */ struct drm_mode_config_funcs { + /** + * @base_modifier: + * + * Reduce the passed in modifier to its base form. This optional + * hook needs to be provided by any driver that embeds extra + * metadata within the format modifier. + * + * RETURNS: + * The base form of the modifier with any extra metadata + * cleared out. + */ + u64 (*base_modifier)(struct drm_device *dev, u64 modifier); + /** * @fb_modifier: * diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index f7bf4a48b1c3..5e26d2a5f6ab 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -489,8 +489,6 @@ enum drm_plane_type { * @format_types: array of formats supported by this plane * @format_count: number of formats supported * @format_default: driver hasn't supplied supported formats for the plane - * @modifiers: array of modifiers supported by this plane - * @modifier_count: number of modifiers supported * @old_fb: Temporary tracking of the old fb while a modeset is ongoing. Used by * drm_mode_set_config_internal() to implement correct refcounting. * @funcs: helper functions @@ -524,7 +522,15 @@ struct drm_plane { unsigned int format_count; bool format_default; + /** + * @modifiers: Array of modifiers supported by this plane. If + * the driver embeds any extra metadata within modifiers these + * modifiers must be in their base form. + */ uint64_t *modifiers; + /** + * @modifier_count: number of modifiers supported + */ unsigned int modifier_count; /**