[Intel-gfx] [PATCH v2] drm/i915: Initialize primary plane src/dst coords when reading hw state

[Daniel Vetter]

On Tue, Jan 20, 2015 at 11:46:57AM +0200, Ville Syrjälä wrote:
> On Tue, Jan 20, 2015 at 11:33:22AM +0200, Ander Conselvan de Oliveira wrote:
> > On 01/20/2015 11:22 AM, Daniel Vetter wrote:
> > > On Mon, Jan 19, 2015 at 03:51:47PM +0200, Ander Conselvan de Oliveira wrote:
> > >> @@ -13346,6 +13364,7 @@ static void intel_modeset_readout_hw_state(struct drm_device *dev)
> > >>
> > >>   		crtc->base.enabled = crtc->active;
> > >>   		crtc->primary_enabled = primary_get_hw_state(crtc);
> > >> +		primary_update_size(crtc);
> > >
> > > I think Ville raised a really good point about the fragility of this and
> > > restoring plane state correctly. I think conceptually it makes more sense
> > > to restore the primary plane state together with the fb in the loop at the
> > > end of intel_modeset_init. Would that still work, or is that too late for
> > > when we change pipe state when sanititizing crtcs?
> > 
> > That should work. Actually, Chris sent a patch that did that some time 
> > ago, and Ville commented that "[he was] thinking that for now these 
> > would be better placed in intel_modeset_readout_hw_state() where we read 
> > out the primary plane enabled state as well". [1]
> > 
> > So just to make it completely clear, does the problem of restoring plane 
> > state correctly supersedes Ville's previous comment?
> 
> Well, I still think intel_modeset_readout_hw_state() is the right place
> for this. We would just need to keep the user state and current state
> clearly separated (intel_modeset_readout_hw_state() should not touch the
> user state). But if that's too hard to do without massive changes, maybe
> we can put it somewhere else in the meantime.

Well most of the plane state readout for the initial config (which this
seems to be a problem with) isn't in that function but in modeset_init.
And we currently have no means to check the requested vs the actual state
for the plane configuration.

Thinking about that we probably don't need that really: The reason for the
modeset config checks is to catch bugs with requested vs. actual state,
since we can't test anything really automatically. But with plane config
changes we can do full functional tests using display CRCs. So from that
pov I don't see a big reason to add all that complexity.

Long term we should try to consolidate the plane state readout a bit
(outside of readout_hw_state perhaps), but for now just adding the code in
the modeset_init loop seems best to me.

>> > [1] 
> > http://permalink.gmane.org/gmane.comp.freedesktop.xorg.drivers.intel/46466

In short I disagree with your comment referenced above.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch