Re: [PATCH 3/4] drm/i915: Add .get_hw_state() method for planes

[Daniel Vetter <daniel@ffwll.ch>]

On Thu, Sep 10, 2015 at 07:36:39PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 10, 2015 at 06:30:20PM +0200, Daniel Vetter wrote:
> > On Thu, Sep 10, 2015 at 07:20:11PM +0300, Ville Syrjälä wrote:
> > > On Thu, Sep 10, 2015 at 07:13:46PM +0300, Ville Syrjälä wrote:
> > > > On Thu, Sep 10, 2015 at 06:07:34PM +0200, Daniel Vetter wrote:
> > > > > On Thu, Sep 10, 2015 at 06:59:09PM +0300, ville.syrjala@linux.intel.com wrote:
> > > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > > 
> > > > > > Add a .get_hw_state() method for planes, returning true or false
> > > > > > depending on whether the plane is enabled. Use it to populate the
> > > > > > plane state 'visible' during state readout.
> > > > > > 
> > > > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > > > Cc: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>
> > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > Do we really need this? The plane readout is fairly limited and we don't
> > > > > really care about recovering anything but the initial primary plane on
> > > > > driver load. Anything else can just be nuked if it misfits ...
> > > > 
> > > > At least avoids calling ->disable_plane for an already disabled plane.
> > > > We could even put a WARN there to catch broken code.
> > > 
> > > Oh and we could also do something like
> > > for_each_plane()
> > > 	assert(visible == get_hw_state);
> > > 
> > > somewhere to perhaps catch even more crap.
> > 
> > Yeah maybe, but I think that should be done as part of the patch series
> > with this patch here. As-is I don't think it adds a lot really ...
> 
> It would at least fix SKL/BXT primary readout, which I forgot to mention
> in the commit message naturally.

Yeah, please hide the bugfix less ;-) Adding ->get_hw_state shouldn't be
the subject either, it should be something like "Add plane hw state
readouf for skl" and the commit message then explains why you added a new
vfunc.

And we should have some idea what to do about the get_initial_plane_config
hook - having two very similar hooks is a bit confusing imo.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx