Re: [PATCH v4 1/2] drm: Add drm_any_plane_has_format()

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Oct 30, 2018 at 10:35:07AM +0100, Daniel Vetter wrote:
> On Mon, Oct 29, 2018 at 04:00:04PM -0700, Eric Anholt wrote:
> > Ville Syrjala <ville.syrjala@linux.intel.com> writes:
> > 
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Add a function to check whether there is at least one plane that
> > > supports a specific format and modifier combination. Drivers can
> > > use this to reject unsupported formats/modifiers in .fb_create().
> > >
> > > v2: 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
> > >
> > > Cc: Eric Anholt <eric@anholt.net>
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > 
> > I don't particularly see the point in having FB creation duplicate the
> > validation that atomic check will eventually do, and it means that FB
> > creation cost scales with plane count, but if i915's going to do this,
> > it seems reasonable for them.
> 
> atomic_check checks for a given plane only, I do think it makes sense to
> make sure you can't create framebuffers that are impossible to use on a
> given driver at addfb time.
> 
> In case the overhead is ever critical, we could compile a static map of
> this at driver load time, and then check that.
> 
> Aside: Shouldn't we make this the default for atomic drivers? With
> atomic drivers we can assume that all planes have valid format lists
> (because atomic_check checks them already). Only with non-atomic drivers,
> how might have a faked primary plane is this not a valid assumption ...

The problem of making this automagic for everyone was the
legacy tiling->modifier thing.

https://patchwork.freedesktop.org/patch/210193/ +
https://patchwork.freedesktop.org/patch/208070/
is the best I could really come up with when I tried to make this
entirely automagic. I haven't bothered to revisit those because I
wasn't entirely happy with needing the extra vfunc, and people
didn't seem too excited about this idea.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel