[Intel-gfx] [PATCH 1/3] drm/i915: Verify address field of PCBR register.

[S, Deepak]

> Have you actually seen cases where the BIOS would be buggy enough to lock the power context addres at 0? If so, we should just scream and run away instead of blindly trying to write a new address to PCBR and pretending that things are fine after that.

We had faced some issue doing the initial version of BIOS. I just verified it and it looks fine and we can ignore this patch. 

I think the right fix would be to verify the pcbr lock bit and if it is set but still pcbr base address is zero then we should disable the rc6.

Thanks
Deepak

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala at linux.intel.com] 
Sent: Monday, December 9, 2013 3:05 PM
To: S, Deepak
Cc: intel-gfx at lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH 1/3] drm/i915: Verify address field of PCBR register.

On Sun, Dec 08, 2013 at 01:52:35PM +0530, deepak.s at intel.com wrote:
> From: Deepak S <deepak.s at intel.com>
> 
> On VLV the PCBR register has other bits besides the pcbr address 
> field. Verify only address field setup by BIOS to make sure we don't 
> misinterpret the PCBR setup.
> 
> Signed-off-by: Deepak S <deepak.s at intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c 
> b/drivers/gpu/drm/i915/intel_pm.c index e6d98fe..2e1340f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4036,7 +4036,12 @@ static void valleyview_setup_pctx(struct drm_device *dev)
>  	int pctx_size = 24*1024;
>  
>  	pcbr = I915_READ(VLV_PCBR);
> -	if (pcbr) {
> +
> +	/* PCBR Format: Bits 31:12 - Base address of Process Context

It's called power context, not process context.

> +	 * 		Bits 11:1 - Reserved

These are all 0

> +	 * 		Bit 0 - PCBR Lock

And if this is 1, then we can't change the address anyway, so I don't think this patch makes much sense.

Have you actually seen cases where the BIOS would be buggy enough to lock the power context addres at 0? If so, we should just scream and run away instead of blindly trying to write a new address to PCBR and pretending that things are fine after that.

> +	 * Check only address field if already setup by BIOS */
> +	if (pcbr >> 12) {
>  		/* BIOS set it up already, grab the pre-alloc'd space */
>  		int pcbr_offset;
>  
> --
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrjälä
Intel OTC