linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* Re: drm/i915: Avoid accessing the stolen address when it is unavailable
  2013-10-24 16:33 drm/i915: Avoid accessing the stolen address when it is unavailable Chuansheng Liu
@ 2013-10-24 12:17 ` Chris Wilson
  2013-10-24 20:56   ` [Intel-gfx] " Ben Widawsky
  0 siblings, 1 reply; 5+ messages in thread
From: Chris Wilson @ 2013-10-24 12:17 UTC (permalink / raw)
  To: Chuansheng Liu
  Cc: daniel.vetter, airlied, intel-gfx, dri-devel, linux-kernel, fei.li

On Fri, Oct 25, 2013 at 12:33:47AM +0800, Chuansheng Liu wrote:
> 
> In our platform, we hit the the stolen region initialization failure case,
> such as below log:
> [drm:i915_stolen_to_physical] *ERROR* conflict detected with stolen region: [0x7b000000]
> 
> And it causes the dev_priv->mm.stolen_base is NULL, in this case, we should
> avoid accessing it any more.
> 
> Here is possible call trace:
> intel_enable_gt_powersave -- >
> valleyview_enable_rps -- >
> valleyview_setup_pctx

The two create_stolen routines are no-ops in that case so all that
happens instead is that we read VLV_PCBR. However, really if
i915_gem_object_create_stolen_for_preallocated() fails we should abort
loading the driver as it means we have a hardware conflict and undefined
behaviour.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 5+ messages in thread

* drm/i915: Avoid accessing the stolen address when it is unavailable
@ 2013-10-24 16:33 Chuansheng Liu
  2013-10-24 12:17 ` Chris Wilson
  0 siblings, 1 reply; 5+ messages in thread
From: Chuansheng Liu @ 2013-10-24 16:33 UTC (permalink / raw)
  To: daniel.vetter, airlied
  Cc: intel-gfx, dri-devel, linux-kernel, chuansheng.liu, fei.li


In our platform, we hit the the stolen region initialization failure case,
such as below log:
[drm:i915_stolen_to_physical] *ERROR* conflict detected with stolen region: [0x7b000000]

And it causes the dev_priv->mm.stolen_base is NULL, in this case, we should
avoid accessing it any more.

Here is possible call trace:
intel_enable_gt_powersave -- >
valleyview_enable_rps -- >
valleyview_setup_pctx

Cc: Li Fei <fei.li@intel.com>
Signed-off-by: Liu, Chuansheng <chuansheng.liu@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 26c2ea3..1069b24 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3735,6 +3735,9 @@ static void valleyview_setup_pctx(struct drm_device *dev)
 	u32 pcbr;
 	int pctx_size = 24*1024;
 
+	if (!dev_priv->mm.stolen_base)
+		return;
+
 	pcbr = I915_READ(VLV_PCBR);
 	if (pcbr) {
 		/* BIOS set it up already, grab the pre-alloc'd space */
-- 
1.7.9.5




^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [Intel-gfx] drm/i915: Avoid accessing the stolen address when it is unavailable
  2013-10-24 12:17 ` Chris Wilson
@ 2013-10-24 20:56   ` Ben Widawsky
  2013-10-25  0:27     ` Liu, Chuansheng
  0 siblings, 1 reply; 5+ messages in thread
From: Ben Widawsky @ 2013-10-24 20:56 UTC (permalink / raw)
  To: Chris Wilson, Chuansheng Liu, daniel.vetter, airlied, intel-gfx,
	dri-devel, linux-kernel, fei.li

On Thu, Oct 24, 2013 at 01:17:06PM +0100, Chris Wilson wrote:
> On Fri, Oct 25, 2013 at 12:33:47AM +0800, Chuansheng Liu wrote:
> > 
> > In our platform, we hit the the stolen region initialization failure case,
> > such as below log:
> > [drm:i915_stolen_to_physical] *ERROR* conflict detected with stolen region: [0x7b000000]
> > 
> > And it causes the dev_priv->mm.stolen_base is NULL, in this case, we should
> > avoid accessing it any more.
> > 
> > Here is possible call trace:
> > intel_enable_gt_powersave -- >
> > valleyview_enable_rps -- >
> > valleyview_setup_pctx
> 
> The two create_stolen routines are no-ops in that case so all that
> happens instead is that we read VLV_PCBR. However, really if
> i915_gem_object_create_stolen_for_preallocated() fails we should abort
> loading the driver as it means we have a hardware conflict and undefined
> behaviour.
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

I agree. We should start treating these things as errors since no
RPS/RC6 is essentially not what anyone wants.

For another immediate solution it seems you can demote the DRM_ERROR to
DRM_DEBUG_DRIVER, and add a check in valleyview_enable_rps for the pctx
value.

-- 
Ben Widawsky, Intel Open Source Technology Center

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [Intel-gfx] drm/i915: Avoid accessing the stolen address when it is unavailable
  2013-10-24 20:56   ` [Intel-gfx] " Ben Widawsky
@ 2013-10-25  0:27     ` Liu, Chuansheng
  2013-10-25  8:07       ` Chris Wilson
  0 siblings, 1 reply; 5+ messages in thread
From: Liu, Chuansheng @ 2013-10-25  0:27 UTC (permalink / raw)
  To: Ben Widawsky, Chris Wilson, daniel.vetter, airlied, intel-gfx,
	dri-devel, linux-kernel, Li, Fei

Hello Chris and Ben,

> -----Original Message-----
> From: Ben Widawsky [mailto:ben@bwidawsk.net]
> Sent: Friday, October 25, 2013 4:57 AM
> To: Chris Wilson; Liu, Chuansheng; daniel.vetter@ffwll.ch; airlied@linux.ie;
> intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> linux-kernel@vger.kernel.org; Li, Fei
> Subject: Re: [Intel-gfx] drm/i915: Avoid accessing the stolen address when it is
> unavailable
> 
> On Thu, Oct 24, 2013 at 01:17:06PM +0100, Chris Wilson wrote:
> > On Fri, Oct 25, 2013 at 12:33:47AM +0800, Chuansheng Liu wrote:
> > >
> > > In our platform, we hit the the stolen region initialization failure case,
> > > such as below log:
> > > [drm:i915_stolen_to_physical] *ERROR* conflict detected with stolen
> region: [0x7b000000]
> > >
> > > And it causes the dev_priv->mm.stolen_base is NULL, in this case, we
> should
> > > avoid accessing it any more.
> > >
> > > Here is possible call trace:
> > > intel_enable_gt_powersave -- >
> > > valleyview_enable_rps -- >
> > > valleyview_setup_pctx
> >
> > The two create_stolen routines are no-ops in that case so all that
> > happens instead is that we read VLV_PCBR. However, really if
> > i915_gem_object_create_stolen_for_preallocated() fails we should abort
> > loading the driver as it means we have a hardware conflict and undefined
> > behaviour.
In case of dev_priv->mm.stolen_base == NULL, and the valleyview_setup_pctx() is called
at the first time, it will call i915_gem_object_create_stolen_for_preallocated(), which should
should return NULL always due to (!drm_mm_initialized(&dev_priv->mm.stolen)).

After that, every time specially when doing pm operation, the above scenario will
be called again and again.

Here this patch is to save some time for PM operation, we do not need to read
VLV_PCBR and pcbr_offset calculation in case of stolen_base == NULL.

Is it making sense? Thanks.

> 
> I agree. We should start treating these things as errors since no
> RPS/RC6 is essentially not what anyone wants.
> 
> DRM_DEBUG_DRIVER, and add a check in valleyview_enable_rps for the pctx
> value.
The pctx is already checked in valleyview_disable_rps().
Do we need more checking in case of pctx == NULL?


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [Intel-gfx] drm/i915: Avoid accessing the stolen address when it is unavailable
  2013-10-25  0:27     ` Liu, Chuansheng
@ 2013-10-25  8:07       ` Chris Wilson
  0 siblings, 0 replies; 5+ messages in thread
From: Chris Wilson @ 2013-10-25  8:07 UTC (permalink / raw)
  To: Liu, Chuansheng
  Cc: Ben Widawsky, daniel.vetter, airlied, intel-gfx, dri-devel,
	linux-kernel, Li, Fei

On Fri, Oct 25, 2013 at 12:27:42AM +0000, Liu, Chuansheng wrote:
> Hello Chris and Ben,
> 
> > -----Original Message-----
> > From: Ben Widawsky [mailto:ben@bwidawsk.net]
> > Sent: Friday, October 25, 2013 4:57 AM
> > To: Chris Wilson; Liu, Chuansheng; daniel.vetter@ffwll.ch; airlied@linux.ie;
> > intel-gfx@lists.freedesktop.org; dri-devel@lists.freedesktop.org;
> > linux-kernel@vger.kernel.org; Li, Fei
> > Subject: Re: [Intel-gfx] drm/i915: Avoid accessing the stolen address when it is
> > unavailable
> > 
> > On Thu, Oct 24, 2013 at 01:17:06PM +0100, Chris Wilson wrote:
> > > On Fri, Oct 25, 2013 at 12:33:47AM +0800, Chuansheng Liu wrote:
> > > >
> > > > In our platform, we hit the the stolen region initialization failure case,
> > > > such as below log:
> > > > [drm:i915_stolen_to_physical] *ERROR* conflict detected with stolen
> > region: [0x7b000000]
> > > >
> > > > And it causes the dev_priv->mm.stolen_base is NULL, in this case, we
> > should
> > > > avoid accessing it any more.
> > > >
> > > > Here is possible call trace:
> > > > intel_enable_gt_powersave -- >
> > > > valleyview_enable_rps -- >
> > > > valleyview_setup_pctx
> > >
> > > The two create_stolen routines are no-ops in that case so all that
> > > happens instead is that we read VLV_PCBR. However, really if
> > > i915_gem_object_create_stolen_for_preallocated() fails we should abort
> > > loading the driver as it means we have a hardware conflict and undefined
> > > behaviour.
> In case of dev_priv->mm.stolen_base == NULL, and the valleyview_setup_pctx() is called
> at the first time, it will call i915_gem_object_create_stolen_for_preallocated(), which should
> should return NULL always due to (!drm_mm_initialized(&dev_priv->mm.stolen)).
> 
> After that, every time specially when doing pm operation, the above scenario will
> be called again and again.
> 
> Here this patch is to save some time for PM operation, we do not need to read
> VLV_PCBR and pcbr_offset calculation in case of stolen_base == NULL.
> 
> Is it making sense? Thanks.

I see. No, it is a pointless optimisation that leaks knowledge about
internals of another subsystem to paper over a kernel bug.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2013-10-25  8:07 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-10-24 16:33 drm/i915: Avoid accessing the stolen address when it is unavailable Chuansheng Liu
2013-10-24 12:17 ` Chris Wilson
2013-10-24 20:56   ` [Intel-gfx] " Ben Widawsky
2013-10-25  0:27     ` Liu, Chuansheng
2013-10-25  8:07       ` Chris Wilson

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).