[4/9] drm/i915/icl: Do not change reserved registers related to PSR2

Submitted by José Roberto de Souza on Nov. 27, 2018, 12:37 a.m.

Details

Message ID 20181127003710.18618-4-jose.souza@intel.com
State New
Headers show
Series "Series without cover letter" ( rev: 1 ) in DRI devel

Not browsing as part of any series.

Commit Message

José Roberto de Souza Nov. 27, 2018, 12:37 a.m.
For ICL the bit 12 of CHICKEN_TRANS is reserved so we should not
touch it and as by default VSC_DATA_SEL_SOFTWARE_CONTROL is already
unset in gen10 + GLK we can just drop it and fix for both gens.

Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
---
 drivers/gpu/drm/i915/intel_psr.c | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)

Patch hide | download patch | download mbox

diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
index 607c3ec41679..7607a58a6ec0 100644
--- a/drivers/gpu/drm/i915/intel_psr.c
+++ b/drivers/gpu/drm/i915/intel_psr.c
@@ -635,17 +635,14 @@  static void intel_psr_enable_source(struct intel_dp *intel_dp,
 	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
 		hsw_psr_setup_aux(intel_dp);
 
-	if (dev_priv->psr.psr2_enabled) {
+	if (dev_priv->psr.psr2_enabled && (IS_GEN9(dev_priv) &&
+					   !IS_GEMINILAKE(dev_priv))) {
 		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
 							cpu_transcoder);
 		u32 chicken = I915_READ(reg);
 
-		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
-			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
-				   | PSR2_ADD_VERTICAL_LINE_COUNT);
-
-		else
-			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
+		chicken |= PSR2_VSC_ENABLE_PROG_HEADER |
+			   PSR2_ADD_VERTICAL_LINE_COUNT;
 		I915_WRITE(reg, chicken);
 	}
 

Comments

On Mon, Nov 26, 2018 at 04:37:05PM -0800, José Roberto de Souza wrote:
> For ICL the bit 12 of CHICKEN_TRANS is reserved so we should not
> touch it and as by default VSC_DATA_SEL_SOFTWARE_CONTROL is already
> unset in gen10 + GLK we can just drop it and fix for both gens.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

Ok, this patch seems right according to spec.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

But I wonder now if we need intel_psr_setup_vsc() at all for
platforms different than gen9.

Because description of this bit is:
This field enables the programmable header for the PSR2 VSC packet.

Without the programmable version I would assume display
engine is now responsible for setting header entirely?

Art?

> ---
>  drivers/gpu/drm/i915/intel_psr.c | 11 ++++-------
>  1 file changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
> index 607c3ec41679..7607a58a6ec0 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -635,17 +635,14 @@ static void intel_psr_enable_source(struct intel_dp *intel_dp,
>  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>  		hsw_psr_setup_aux(intel_dp);
>  
> -	if (dev_priv->psr.psr2_enabled) {
> +	if (dev_priv->psr.psr2_enabled && (IS_GEN9(dev_priv) &&
> +					   !IS_GEMINILAKE(dev_priv))) {
>  		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
>  							cpu_transcoder);
>  		u32 chicken = I915_READ(reg);
>  
> -		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
> -			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
> -				   | PSR2_ADD_VERTICAL_LINE_COUNT);
> -
> -		else
> -			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
> +		chicken |= PSR2_VSC_ENABLE_PROG_HEADER |
> +			   PSR2_ADD_VERTICAL_LINE_COUNT;
>  		I915_WRITE(reg, chicken);
>  	}
>  
> -- 
> 2.19.2
>
On Thu, 2018-11-29 at 14:15 -0800, Rodrigo Vivi wrote:
> On Mon, Nov 26, 2018 at 04:37:05PM -0800, José Roberto de Souza
> wrote:
> > For ICL the bit 12 of CHICKEN_TRANS is reserved so we should not
> > touch it and as by default VSC_DATA_SEL_SOFTWARE_CONTROL is already
> > unset in gen10 + GLK we can just drop it and fix for both gens.
> > 
> > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>
> 
> Ok, this patch seems right according to spec.
> 
> Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> 
> But I wonder now if we need intel_psr_setup_vsc() at all for
> platforms different than gen9.
> 
> Because description of this bit is:
> This field enables the programmable header for the PSR2 VSC packet.
> 
> Without the programmable version I would assume display
> engine is now responsible for setting header entirely?

As this was in a chicken register in gen 9 my guess is that it was
fixed on newer gens and as it is required for PSR2 we don't need to
manually enable it in driver but Art could confirm it.

> 
> Art?
> 
> > ---
> >  drivers/gpu/drm/i915/intel_psr.c | 11 ++++-------
> >  1 file changed, 4 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_psr.c
> > b/drivers/gpu/drm/i915/intel_psr.c
> > index 607c3ec41679..7607a58a6ec0 100644
> > --- a/drivers/gpu/drm/i915/intel_psr.c
> > +++ b/drivers/gpu/drm/i915/intel_psr.c
> > @@ -635,17 +635,14 @@ static void intel_psr_enable_source(struct
> > intel_dp *intel_dp,
> >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
> >  		hsw_psr_setup_aux(intel_dp);
> >  
> > -	if (dev_priv->psr.psr2_enabled) {
> > +	if (dev_priv->psr.psr2_enabled && (IS_GEN9(dev_priv) &&
> > +					   !IS_GEMINILAKE(dev_priv))) {
> >  		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,
> >  							cpu_transcoder)
> > ;
> >  		u32 chicken = I915_READ(reg);
> >  
> > -		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))
> > -			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER
> > -				   | PSR2_ADD_VERTICAL_LINE_COUNT);
> > -
> > -		else
> > -			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;
> > +		chicken |= PSR2_VSC_ENABLE_PROG_HEADER |
> > +			   PSR2_ADD_VERTICAL_LINE_COUNT;
> >  		I915_WRITE(reg, chicken);
> >  	}
> >  
> > -- 
> > 2.19.2
> >
I'll check on it.

> -----Original Message-----

> From: Souza, Jose

> Sent: Thursday, 29 November, 2018 3:47 PM

> To: Vivi, Rodrigo <rodrigo.vivi@intel.com>

> Cc: dri-devel@lists.freedesktop.org; intel-gfx@lists.freedesktop.org;

> Runyan, Arthur J <arthur.j.runyan@intel.com>; Pandiyan, Dhinakaran

> <dhinakaran.pandiyan@intel.com>

> Subject: Re: [PATCH 4/9] drm/i915/icl: Do not change reserved registers

> related to PSR2

> 

> On Thu, 2018-11-29 at 14:15 -0800, Rodrigo Vivi wrote:

> > On Mon, Nov 26, 2018 at 04:37:05PM -0800, José Roberto de Souza

> > wrote:

> > > For ICL the bit 12 of CHICKEN_TRANS is reserved so we should not

> > > touch it and as by default VSC_DATA_SEL_SOFTWARE_CONTROL is

> already

> > > unset in gen10 + GLK we can just drop it and fix for both gens.

> > >

> > > Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>

> > > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>

> > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com>

> >

> > Ok, this patch seems right according to spec.

> >

> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> >

> > But I wonder now if we need intel_psr_setup_vsc() at all for

> > platforms different than gen9.

> >

> > Because description of this bit is:

> > This field enables the programmable header for the PSR2 VSC packet.

> >

> > Without the programmable version I would assume display

> > engine is now responsible for setting header entirely?

> 

> As this was in a chicken register in gen 9 my guess is that it was

> fixed on newer gens and as it is required for PSR2 we don't need to

> manually enable it in driver but Art could confirm it.

> 

> >

> > Art?

> >

> > > ---

> > >  drivers/gpu/drm/i915/intel_psr.c | 11 ++++-------

> > >  1 file changed, 4 insertions(+), 7 deletions(-)

> > >

> > > diff --git a/drivers/gpu/drm/i915/intel_psr.c

> > > b/drivers/gpu/drm/i915/intel_psr.c

> > > index 607c3ec41679..7607a58a6ec0 100644

> > > --- a/drivers/gpu/drm/i915/intel_psr.c

> > > +++ b/drivers/gpu/drm/i915/intel_psr.c

> > > @@ -635,17 +635,14 @@ static void intel_psr_enable_source(struct

> > > intel_dp *intel_dp,

> > >  	if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))

> > >  		hsw_psr_setup_aux(intel_dp);

> > >

> > > -	if (dev_priv->psr.psr2_enabled) {

> > > +	if (dev_priv->psr.psr2_enabled && (IS_GEN9(dev_priv) &&

> > > +					   !IS_GEMINILAKE(dev_priv))) {

> > >  		i915_reg_t reg = gen9_chicken_trans_reg(dev_priv,

> > >  							cpu_transcoder)

> > > ;

> > >  		u32 chicken = I915_READ(reg);

> > >

> > > -		if (IS_GEN9(dev_priv) && !IS_GEMINILAKE(dev_priv))

> > > -			chicken |= (PSR2_VSC_ENABLE_PROG_HEADER

> > > -				   | PSR2_ADD_VERTICAL_LINE_COUNT);

> > > -

> > > -		else

> > > -			chicken &= ~VSC_DATA_SEL_SOFTWARE_CONTROL;

> > > +		chicken |= PSR2_VSC_ENABLE_PROG_HEADER |

> > > +			   PSR2_ADD_VERTICAL_LINE_COUNT;

> > >  		I915_WRITE(reg, chicken);

> > >  	}

> > >

> > > --

> > > 2.19.2

> > >