Re: [Intel-gfx] [PATCH 06/10] drm/i915/psr: set CHICKEN_TRANS for psr2

From: Chris Wilson <chris@chris-wilson.co.uk>
To: vathsala nagaraju <vathsala.nagaraju@intel.com>
Cc: intel-gfx@lists.freedesktop.org,
	Patil Deepti <deepti.patil@intel.com>,
	dri-devel@lists.freedesktop.org,
	Rodrigo Vivi <rodrigo.vivi@intel.com>
Subject: Re: [Intel-gfx] [PATCH 06/10] drm/i915/psr: set CHICKEN_TRANS for psr2
Date: Sat, 7 Jan 2017 19:44:29 +0000	[thread overview]
Message-ID: <20170107194429.GP19067@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <1483812724-6503-1-git-send-email-vathsala.nagaraju@intel.com>

On Sat, Jan 07, 2017 at 11:42:04PM +0530, vathsala nagaraju wrote:
> As per bpsec, CHICKEN_TRANS_EDP bit 12 ,15
> must be programmed.
> Enable bit 12 for programmable header packet.
> Enable bit 15 for Y cordinate support.
> 
> v2: (Rodrigo)
> - move CHICKEN_TRANS_EDP bit set logic right after setup_vsc
> 
> v3:(Rodrigo)
> - initialize chicken_trans to CHICKEN_TRANS_BIT12 instead of 0
> 
> v4:(Rodrigo)
> - initialize chicken_trans=0,add chicken_trans=CHICKEN_TRANS_BIT12

Weird. Just scope the variable properly, use the correct type.

> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: Jim Bride <jim.bride@linux.intel.com>
> Signed-off-by: vathsala nagaraju <vathsala.nagaraju@intel.com>
> Signed-off-by: Patil Deepti <deepti.patil@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h  | 7 +++++++
>  drivers/gpu/drm/i915/intel_psr.c | 7 +++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7830e6e..5ca506a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6449,6 +6449,13 @@ enum {
>  #define  BDW_DPRS_MASK_VBLANK_SRD	(1 << 0)
>  #define CHICKEN_PIPESL_1(pipe) _MMIO_PIPE(pipe, _CHICKEN_PIPESL_1_A, _CHICKEN_PIPESL_1_B)
>  
> +#define CHICKEN_TRANS_A         0x420c0
> +#define CHICKEN_TRANS_B         0x420c4
> +#define CHICKEN_TRANS(trans) _MMIO_TRANS(trans, CHICKEN_TRANS_A, CHICKEN_TRANS_B)
> +#define TRANS_EDP              3
> +#define CHICKEN_TRANS_BIT12    (1<<12)
> +#define CHICKEN_TRANS_BIT15    (1<<15)

Useless naming. Either given them proper names or don't.

>  	if (!HAS_PSR(dev_priv)) {
		u32 chicken;

>  		DRM_DEBUG_KMS("PSR not supported on this platform\n");
> @@ -505,6 +506,12 @@ void intel_psr_enable(struct intel_dp *intel_dp)
>  				dev_priv->psr.psr2_support = false;
>  			else
>  				skl_psr_setup_su_vsc(intel_dp);
> +			/* Set CHICKEN_TRANS_BIT12 for programable header */
> +			chicken_trans = CHICKEN_TRANS_BIT12;

We can see you are setting CHICKEN_TRANS_BIT12, so don't bother
repeating that. What programmable header? Why is this in a chicken bit,
what is the bspec reference, all of those would be useful to answer.

> +			/* Set CHICKEN_TRANS_BIT15 if Y coordinate is supported */
> +			if (dev_priv->psr.y_cord_support)
> +				chicken_trans |= CHICKEN_TRANS_BIT15;

Again. Tell us why, we can read the code. Are the names meaningful? More
meaningful than chicken |= BIT(15); ?

> +			I915_WRITE(CHICKEN_TRANS(TRANS_EDP), chicken_trans);
>  		} else {
>  			/* set up vsc header for psr1 */
>  			hsw_psr_setup_vsc(intel_dp);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel