[PATCH] drm/i915: reset GPU after clock gating init

[PATCH] drm/i915: reset GPU after clock gating init

[Jesse Barnes]

This is needed for SNB at least after disabling CSunit clock gating, and
shouldn't hurt on other platforms either.

This fixes an issue on James's machine where RC6 wouldn't always get
enabled.

Tested-by: James Kukunas <james.t.kukunas@intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_display.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6d8a5ed..2fccd8f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8718,6 +8718,7 @@ void intel_modeset_init_hw(struct drm_device *dev)
 	intel_prepare_ddi(dev);
 
 	intel_init_clock_gating(dev);
+	intel_gpu_reset(dev);
 
 	mutex_lock(&dev->struct_mutex);
 	intel_enable_gt_powersave(dev);
-- 
1.7.9.5

Re: [PATCH] drm/i915: reset GPU after clock gating init

[Daniel Vetter]

On Thu, Nov 15, 2012 at 7:24 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> This is needed for SNB at least after disabling CSunit clock gating, and
> shouldn't hurt on other platforms either.
>
> This fixes an issue on James's machine where RC6 wouldn't always get
> enabled.
>
> Tested-by: James Kukunas <james.t.kukunas@intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 6d8a5ed..2fccd8f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8718,6 +8718,7 @@ void intel_modeset_init_hw(struct drm_device *dev)
>         intel_prepare_ddi(dev);
>
>         intel_init_clock_gating(dev);
> +       intel_gpu_reset(dev);

Unconditionally resetting machines tends to upset users, especially
since it isn't implemented or doesn't work on gen2/3, leaving a wedged
machine behind. And it spews ERRORs into dmesg. Also, a tiny comment
explaining what's going on would be preferable.

I think the better approach is to either call the low-level reset
function from the relevant clock-gating callbacks (maybe shovel the
reset functions into dev_priv->gt.reset while at it). Or alternatively
figure out what's wrong with our init sequence and reorder things
(maybe that specific w/a needs to happen before we enable rings, it
would not be the first one).
-Daniel

>
>         mutex_lock(&dev->struct_mutex);
>         intel_enable_gt_powersave(dev);
> --
> 1.7.9.5
>
> _______________________________________________
> 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] drm/i915: reset GPU after clock gating init

[Jesse Barnes]

On Thu, 15 Nov 2012 21:05:04 +0100
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Thu, Nov 15, 2012 at 7:24 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > This is needed for SNB at least after disabling CSunit clock gating, and
> > shouldn't hurt on other platforms either.
> >
> > This fixes an issue on James's machine where RC6 wouldn't always get
> > enabled.
> >
> > Tested-by: James Kukunas <james.t.kukunas@intel.com>
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |    1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 6d8a5ed..2fccd8f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -8718,6 +8718,7 @@ void intel_modeset_init_hw(struct drm_device *dev)
> >         intel_prepare_ddi(dev);
> >
> >         intel_init_clock_gating(dev);
> > +       intel_gpu_reset(dev);
> 
> Unconditionally resetting machines tends to upset users, especially
> since it isn't implemented or doesn't work on gen2/3, leaving a wedged
> machine behind. And it spews ERRORs into dmesg. Also, a tiny comment
> explaining what's going on would be preferable.
> 
> I think the better approach is to either call the low-level reset
> function from the relevant clock-gating callbacks (maybe shovel the
> reset functions into dev_priv->gt.reset while at it). Or alternatively
> figure out what's wrong with our init sequence and reorder things
> (maybe that specific w/a needs to happen before we enable rings, it
> would not be the first one).

No this workaround is specifically: disable CSunit, reset render.

Yeah it can be stuffed in the gen6 clock gating init, or even pushed
off into the GT rps init work handler, since it may take awhile.  I'll
clean it up.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: reset GPU after clock gating init

[Jesse Barnes]

On Thu, 15 Nov 2012 12:10:28 -0800
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Thu, 15 Nov 2012 21:05:04 +0100
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Thu, Nov 15, 2012 at 7:24 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > > This is needed for SNB at least after disabling CSunit clock gating, and
> > > shouldn't hurt on other platforms either.
> > >
> > > This fixes an issue on James's machine where RC6 wouldn't always get
> > > enabled.
> > >
> > > Tested-by: James Kukunas <james.t.kukunas@intel.com>
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |    1 +
> > >  1 file changed, 1 insertion(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 6d8a5ed..2fccd8f 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -8718,6 +8718,7 @@ void intel_modeset_init_hw(struct drm_device *dev)
> > >         intel_prepare_ddi(dev);
> > >
> > >         intel_init_clock_gating(dev);
> > > +       intel_gpu_reset(dev);
> > 
> > Unconditionally resetting machines tends to upset users, especially
> > since it isn't implemented or doesn't work on gen2/3, leaving a wedged
> > machine behind. And it spews ERRORs into dmesg. Also, a tiny comment
> > explaining what's going on would be preferable.
> > 
> > I think the better approach is to either call the low-level reset
> > function from the relevant clock-gating callbacks (maybe shovel the
> > reset functions into dev_priv->gt.reset while at it). Or alternatively
> > figure out what's wrong with our init sequence and reorder things
> > (maybe that specific w/a needs to happen before we enable rings, it
> > would not be the first one).
> 
> No this workaround is specifically: disable CSunit, reset render.
> 
> Yeah it can be stuffed in the gen6 clock gating init, or even pushed
> off into the GT rps init work handler, since it may take awhile.  I'll
> clean it up.
> 

Apparently this causes performance regressions on the Dell machines it
"fixes".  Can anyone else confirm that?  Maybe we're not restoring some
state after reset which causes trouble...

-- 
Jesse Barnes, Intel Open Source Technology Center