Re: [PATCH 12/31] drm/i915: Fix PSR initialization.

[Daniel Vetter <daniel@ffwll.ch>]

On Wed, Nov 18, 2015 at 06:39:45PM +0000, Vivi, Rodrigo wrote:
> On Wed, 2015-11-18 at 11:12 +0100, Daniel Vetter wrote:
> > On Thu, Nov 05, 2015 at 10:50:04AM -0800, Rodrigo Vivi wrote:
> > > PSR is still disabled by default, but even passing 
> > > i915.enable_psr=1
> > > at this point we weren't able to get PSR working because with
> > > fastboot by default in place we weren't executing the path that 
> > > enables
> > > encoder and consequently PSR.
> > > 
> > > Now with psr_ready in place and PSR using crtc signature we can 
> > > move
> > > its enable/disable sequences from the encoder enable to the post
> > > atomic modeset functions.
> > > 
> > > i915.enable_psr parameter is still used to enable/disable psr 
> > > feature
> > > on the next primary plane update. So current test cases that relies
> > > on this flow still works.
> > > 
> > > Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c     |  2 --
> > >  drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++++++
> > >  drivers/gpu/drm/i915/intel_dp.c      |  5 -----
> > >  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
> > >  4 files changed, 17 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index b8f8dee..36db970 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -2404,7 +2404,6 @@ static void intel_enable_ddi(struct 
> > > intel_encoder *intel_encoder)
> > >  			intel_dp_stop_link_train(intel_dp);
> > >  
> > >  		intel_edp_backlight_on(intel_dp);
> > > -		intel_psr_enable(intel_crtc);
> > >  		intel_edp_drrs_enable(intel_dp);
> > >  	}
> > >  
> > > @@ -2432,7 +2431,6 @@ static void intel_disable_ddi(struct 
> > > intel_encoder *intel_encoder)
> > >  		struct intel_dp *intel_dp = 
> > > enc_to_intel_dp(encoder);
> > >  
> > >  		intel_edp_drrs_disable(intel_dp);
> > > -		intel_psr_disable(intel_crtc);
> > >  		intel_edp_backlight_off(intel_dp);
> > >  	}
> > >  }
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > index 869929d..f67e2ee 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4687,6 +4687,9 @@ static void intel_post_plane_update(struct 
> > > intel_crtc *crtc)
> > >  	if (atomic->enable_ips)
> > >  		intel_ips_enable(crtc);
> > >  
> > > +	if (atomic->enable_psr)
> > > +		intel_psr_enable(crtc);
> > 
> > What we need here is a post-modeset fixup hook for 
> > encoders/connectors.
> > That would avoid the layering violation of going through a crtc and 
> > then
> > doing the crtc->encoder lookup you do in the previous patch. And we 
> > have
> > other uses for this, e.g. mipi self refresh, fixing up infoframes and 
> > all
> > those things.
> 
> I see your point but I'm not sure if I totally got your idea.
> 
> So would it be a generic hook when we detect encoder/connector being
> enabled/disabled?
> 
> and how to determine that? could you please elaborate your idea a bit
> more?


diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c3bc6ab77508..427daac480f5 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13273,6 +13273,11 @@ static int intel_atomic_commit(struct drm_device *dev,
 			modeset_put_power_domains(dev_priv, put_domains);
 
 		intel_post_plane_update(intel_crtc);
+
+		if (needs_modeset || update_pipe) {
+			for_each_encoder_on_crtc(dev, crtc, encoder)
+				encoder->enable_fixup(encoder);
+		}
 	}
 
 	/* FIXME: add subpixel order */


With this enable_fixup (maybe we should call it post_plane_enable) will
will get called both when we do a full modeset, but also when we just do a
fast modeset/pipe update. Then we can move everything that we're currently
doing in ->enable which also must be done for fastboot into
->post_plane_enable, like infoframes, DRRS, psr or whatever.

The only tricky part is that the core won't do the book-keeping for us
anymore, i.e. we need to check ourselves in this new hook whether we need
to do the setup or not, since if userspace only changes the scaling a lot
we might get a lot of ->post_plane_enable calls without any ->disable
calls in between. So that's also where you have to put your checks:

intel_dp_post_plane_enable()
{
	if (!psr_enabled)
		enable_psr();

	if (!DRRS_on)
		enable_drrs();

	/* maybe in the future */
	if (!audio_enabled)
		enable_audio();
}


intel_hdmi_post_plane_enable()
{
	/* ifxup infoframes if they're crap from the bios */
}

Cheers, 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