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@xxxxxxxxxxxxxxx> writes:
> >
> > > From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > >
> > > 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@xxxxxxxxxx>
> > > Testcase: igt/kms_addfb_basic/expected-formats
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > > ---
> > > 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@xxxxxxxxxxxxxxx>
AuthorDate: Fri Dec 22 21:22:30 2017 +0200
Commit: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
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.
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
