Re: [PATCH 2/6] drm/i915/psr: Tie PSR2 support to Y coordinate requirement in intel_psr_init_dpcd()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 15, 2018 at 06:04:27PM -0700, Souza, Jose wrote:
> On Thu, 2018-03-15 at 17:35 -0700, Rodrigo Vivi wrote:
> > On Wed, Mar 14, 2018 at 03:36:13PM -0700, José Roberto de Souza
> > wrote:
> > > Move to only one place the sink requirements that the actual driver
> > > needs to enable PSR2.
> > > 
> > > Also intel_psr2_config_valid() is called every time the crtc config
> > > is computed, wasting some time every time it was checking for
> > > Y coordinate requirement.
> > > 
> > > This allow us to nuke y_cord_support and some of VSC setup code
> > > that
> > > was handling a scenario that would never happen(PSR2 without Y
> > > coordinate).
> > > 
> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@xxxxxxxxx>
> > > Cc: Rodrigo Vivi <rodrigo.vivi@xxxxxxxxx>
> > > Signed-off-by: José Roberto de Souza <jose.souza@xxxxxxxxx>
> > > ---
> > >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> > >  drivers/gpu/drm/i915/intel_psr.c | 55 ++++++++++++++++++++------
> > > --------------
> > >  2 files changed, 28 insertions(+), 28 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 8a584273f897..d4bc8d18f56c 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -603,7 +603,6 @@ struct i915_psr {
> > >  	unsigned busy_frontbuffer_bits;
> > >  	bool psr2_support;
> > >  	bool link_standby;
> > > -	bool y_cord_support;
> > >  	bool colorimetry_support;
> > >  	bool alpm;
> > >  	bool has_hw_tracking;
> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > > b/drivers/gpu/drm/i915/intel_psr.c
> > > index 9811f5f0bc75..62d97d5576d1 100644
> > > --- a/drivers/gpu/drm/i915/intel_psr.c
> > > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > > @@ -93,7 +93,7 @@ static void psr_aux_io_power_put(struct intel_dp
> > > *intel_dp)
> > >  	intel_display_power_put(dev_priv,
> > > psr_aux_domain(intel_dp));
> > >  }
> > >  
> > > -static bool intel_dp_get_y_cord_status(struct intel_dp *intel_dp)
> > > +static bool intel_dp_get_y_coord_required(struct intel_dp
> > > *intel_dp)
> > >  {
> > >  	uint8_t psr_caps = 0;
> > >  
> > > @@ -137,22 +137,38 @@ void intel_psr_init_dpcd(struct intel_dp
> > > *intel_dp)
> > >  
> > >  	if (INTEL_GEN(dev_priv) >= 9 &&
> > >  	    (intel_dp->psr_dpcd[0] & DP_PSR2_IS_SUPPORTED)) {
> > > -		uint8_t frame_sync_cap;
> > > +		uint8_t frame_sync_cap, y_coord_req;
> > >  
> > >  		dev_priv->psr.sink_support = true;
> > > +
> > > +		/* PSR2 needs frame sync to do selective updates
> > > */
> > >  		if (drm_dp_dpcd_readb(&intel_dp->aux,
> > >  				      DP_SINK_DEVICE_AUX_FRAME_SYN
> > > C_CAP,
> > >  				      &frame_sync_cap) != 1)
> > >  			frame_sync_cap = 0;
> > >  		frame_sync_cap = (frame_sync_cap &
> > > DP_AUX_FRAME_SYNC_CAP);
> > > -		/* PSR2 needs frame sync as well */
> > > -		dev_priv->psr.psr2_support = frame_sync_cap;
> > > -		DRM_DEBUG_KMS("PSR2 %s on sink",
> > > -			      dev_priv->psr.psr2_support ?
> > > "supported" : "not supported");
> > > +
> > > +		/*
> > > +		 * FIXME: Enable PSR2 only for y-coordinate PSR2
> > > panels
> > > +		 * After GTC implementation, remove this
> > > restriction.
> > 
> > I believe we should remove the FIXME and just comment out that non y-
> > coordinate
> > would need GTC that we currently don't implement.
> > 
> > later if we change our minds and decided to add that support we
> > revisit here
> > but without a FIXME poluting code for something we don't plan to
> > support for now.
> > 
> > > +		 *
> > > +		 * All panels that supports PSR version 3 that is
> > 
> > PSRv3? is there really something formal about that?
> 
> Yes, from eDP spec:
> 
> PSR Support and Version
> 
> 00h = Panel Self Refresh Capability is not supported (default).
> If PSR/PSR2 is supported, the SET_POWER_CAPABLE bit in the
> EDP_GENERAL_CAPABILITY_1 register (DPCD Address 00701h, bit 7) must
> be set to 1.
> 01h = Panel Self Refresh capability is supported, PSR version is 01h
> (PSR).
> 02h = Panel Self Refresh with Selective Update (SU) capability is
> supported,
> PSR version is 02h (PSR2). Y-coordinates for SU are not supported.
> Supported
> by eDP v1.4 (and higher).
> 03h = Panel Self Refresh with SU capability and Y-coordinate supported,
> PSR
> version is 03h (PSR2 + Y-coordinate). Supported by eDP v1.4a (and
> higher).

Oh cool! now I understand the comment.
Could we add the comment like in spec ie
s/PSR version 3 that is PSR2 + Y-coordinate/PSR version 03h (PSR2 + Y-coordinate)/
?
just to avoid giving impression of a PSR3 support and someone
freaking out thinking that we would need much more changes ;)

> 
> > 
> > > +		 * PSR2 + Y-coordinate support can handle Y
> > > coordinates in VSC
> > > +		 * but we are only sure that it is going to be
> > > used when
> > > +		 * required by the panel. This way panel is
> > > capable to do
> > > +		 * selective update even without a valid aux frame
> > > sync.
> > > +		 */
> > > +		y_coord_req =
> > > intel_dp_get_y_coord_required(intel_dp);
> > > +
> > > +		dev_priv->psr.psr2_support = frame_sync_cap &&
> > > y_coord_req;
> > > +		if (dev_priv->psr.psr2_support)
> > > +			DRM_DEBUG_KMS("PSR2 supported on sink\n");
> > > +		else
> > > +			DRM_DEBUG_KMS("PSR2 not supported on sink"
> > > +				      "(frame sync: %d Y-coord
> > > required: %d)\n",
> > > +				      frame_sync_cap,
> > > y_coord_req);
> > >  
> > >  		if (dev_priv->psr.psr2_support) {
> > > -			dev_priv->psr.y_cord_support =
> > > -				intel_dp_get_y_cord_status(intel_d
> > > p);
> > >  			dev_priv->psr.colorimetry_support =
> > >  				intel_dp_get_colorimetry_status(in
> > > tel_dp);
> > >  			dev_priv->psr.alpm =
> > > @@ -198,16 +214,12 @@ static void hsw_psr_setup_vsc(struct intel_dp
> > > *intel_dp,
> > >  		memset(&psr_vsc, 0, sizeof(psr_vsc));
> > >  		psr_vsc.sdp_header.HB0 = 0;
> > >  		psr_vsc.sdp_header.HB1 = 0x7;
> > > -		if (dev_priv->psr.colorimetry_support &&
> > > -		    dev_priv->psr.y_cord_support) {
> > > +		if (dev_priv->psr.colorimetry_support) {
> > >  			psr_vsc.sdp_header.HB2 = 0x5;
> > >  			psr_vsc.sdp_header.HB3 = 0x13;
> > > -		} else if (dev_priv->psr.y_cord_support) {
> > > +		} else {
> > >  			psr_vsc.sdp_header.HB2 = 0x4;
> > >  			psr_vsc.sdp_header.HB3 = 0xe;
> > > -		} else {
> > > -			psr_vsc.sdp_header.HB2 = 0x3;
> > > -			psr_vsc.sdp_header.HB3 = 0xc;
> > >  		}
> > >  	} else {
> > >  		/* Prepare VSC packet as per EDP 1.3 spec, Table
> > > 3.10 */
> > > @@ -478,15 +490,6 @@ static bool intel_psr2_config_valid(struct
> > > intel_dp *intel_dp,
> > >  		return false;
> > >  	}
> > >  
> > > -	/*
> > > -	 * FIXME:enable psr2 only for y-cordinate psr2 panels
> > > -	 * After gtc implementation , remove this restriction.
> > > -	 */
> > > -	if (!dev_priv->psr.y_cord_support) {
> > > -		DRM_DEBUG_KMS("PSR2 not enabled, panel does not
> > > support Y coordinate\n");
> > > -		return false;
> > > -	}
> > > -
> > >  	return true;
> > >  }
> > >  
> > > @@ -586,14 +589,12 @@ static void hsw_psr_enable_source(struct
> > > intel_dp *intel_dp,
> > >  	struct drm_device *dev = dig_port->base.base.dev;
> > >  	struct drm_i915_private *dev_priv = to_i915(dev);
> > >  	enum transcoder cpu_transcoder = crtc_state-
> > > >cpu_transcoder;
> > > -	u32 chicken;
> > >  
> > >  	psr_aux_io_power_get(intel_dp);
> > >  
> > >  	if (dev_priv->psr.psr2_support) {
> > > -		chicken = PSR2_VSC_ENABLE_PROG_HEADER;
> > > -		if (dev_priv->psr.y_cord_support)
> > > -			chicken |= PSR2_ADD_VERTICAL_LINE_COUNT;
> > > +		u32 chicken = PSR2_VSC_ENABLE_PROG_HEADER
> > > +			      | PSR2_ADD_VERTICAL_LINE_COUNT;
> > >  		I915_WRITE(CHICKEN_TRANS(cpu_transcoder),
> > > chicken);
> > >  
> > >  		I915_WRITE(EDP_PSR_DEBUG,
> > > -- 
> > > 2.16.2
> > > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux