| Message ID | 1386490955-20608-1-git-send-email-deepak.s@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
> 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
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;