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

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

[deepak.s]

From: Deepak S <deepak.s@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@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
+	 * 		Bits 11:1 - Reserved
+	 * 		Bit 0 - PCBR Lock
+	 * 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

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

[Daniel Vetter]

On Sun, Dec 08, 2013 at 01:52:35PM +0530, deepak.s@intel.com wrote:
> From: Deepak S <deepak.s@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@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
> +	 * 		Bits 11:1 - Reserved
> +	 * 		Bit 0 - PCBR Lock
> +	 * Check only address field if already setup by BIOS */
> +	if (pcbr >> 12) {

I'd just add a VLV_PCBR_ADDR_MASK #define and & it. That's a bit more the
idiom we usually use, and also allows us to ditcht the comment. You could
even add a VLV_PCBR_LOCK #define if you want.
-Daniel

>  		/* BIOS set it up already, grab the pre-alloc'd space */
>  		int pcbr_offset;
>  
> -- 
> 1.8.4.2
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Ville Syrjälä]

On Sun, Dec 08, 2013 at 01:52:35PM +0530, deepak.s@intel.com wrote:
> From: Deepak S <deepak.s@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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [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@linux.intel.com] 
Sent: Monday, December 9, 2013 3:05 PM
To: S, Deepak
Cc: intel-gfx@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@intel.com wrote:
> From: Deepak S <deepak.s@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@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@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
Ville Syrjälä
Intel OTC