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

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

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?

> -Daniel

Thank you very much,
Rodrigo.

> 
> > +
> >  	if (atomic->post_enable_primary)
> >  		intel_post_enable_primary(&crtc->base);
> >  
> > @@ -4705,6 +4708,9 @@ static void intel_pre_plane_update(struct 
> > intel_crtc *crtc)
> >  	if (crtc->atomic.disable_ips)
> >  		intel_ips_disable_if_alone(crtc);
> >  
> > +	if (crtc->atomic.disable_psr)
> > +		intel_psr_disable(crtc);
> > +
> >  	if (atomic->pre_disable_primary)
> >  		intel_pre_disable_primary(&crtc->base);
> >  
> > @@ -11560,9 +11566,18 @@ int intel_plane_atomic_calc_changes(struct 
> > drm_crtc_state *crtc_state,
> >  			intel_crtc->atomic.disable_ips = true;
> >  
> >  			intel_crtc->atomic.disable_fbc = true;
> > +
> > +			intel_crtc->atomic.disable_psr = true;
> >  		}
> >  		if (visible && intel_crtc->config->ips_ready)
> >  			intel_crtc->atomic.enable_ips = true;
> > +
> > +		if (visible && intel_crtc->config->psr_ready) {
> > +			if (i915.enable_psr)
> > +				intel_crtc->atomic.enable_psr = 
> > true;
> > +			else
> > +				intel_crtc->atomic.disable_psr = 
> > true;
> > +		}
> >  		/*
> >  		 * FBC does not work on some platforms for rotated
> >  		 * planes, so disable it when rotation is not 0 
> > and
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 92f59cc..f0ee497 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -2373,9 +2373,6 @@ static void intel_disable_dp(struct 
> > intel_encoder *encoder)
> >  	if (crtc->config->has_audio)
> >  		intel_audio_codec_disable(encoder);
> >  
> > -	if (HAS_PSR(dev) && !HAS_DDI(dev))
> > -		intel_psr_disable(crtc);
> > -
> >  	/* Make sure the panel is off before trying to change the 
> > mode. But also
> >  	 * ensure that we have vdd while we switch off the panel. 
> > */
> >  	intel_edp_panel_vdd_on(intel_dp);
> > @@ -2629,10 +2626,8 @@ static void g4x_enable_dp(struct 
> > intel_encoder *encoder)
> >  static void vlv_enable_dp(struct intel_encoder *encoder)
> >  {
> >  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder
> > ->base);
> > -	struct intel_crtc *crtc = to_intel_crtc(encoder
> > ->base.crtc);
> >  
> >  	intel_edp_backlight_on(intel_dp);
> > -	intel_psr_enable(crtc);
> >  }
> >  
> >  static void g4x_pre_enable_dp(struct intel_encoder *encoder)
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index cafe4c1..d599d54 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -531,6 +531,7 @@ struct intel_crtc_atomic_commit {
> >  	/* Sleepable operations to perform before commit */
> >  	bool disable_fbc;
> >  	bool disable_ips;
> > +	bool disable_psr;
> >  	bool disable_cxsr;
> >  	bool pre_disable_primary;
> >  	bool update_wm_pre, update_wm_post;
> > @@ -540,6 +541,7 @@ struct intel_crtc_atomic_commit {
> >  	bool wait_vblank;
> >  	bool update_fbc;
> >  	bool enable_ips;
> > +	bool enable_psr;
> >  	bool post_enable_primary;
> >  	unsigned update_sprite_watermarks;
> >  };
> > -- 
> > 2.4.3
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx