Re: [PATCH v2 4/4] drm/i915: add assert_device_not_suspended to GGTT PTE updaters

Imre Deak
2015-11-09T21:29:58Z

On Mon, 2015-11-09 at 23:24 +0200, Imre Deak wrote:
> On Mon, 2015-11-09 at 21:11 +0000, Chris Wilson wrote:
> > On Mon, Nov 09, 2015 at 09:14:19PM +0200, Imre Deak wrote:
> > > The device should be on when updating the GGTT PTEs, so add an assert to
> > > all relevant places.
> > >
> > > v2:
> > > - use the existing dev_priv directly everywhere (Ville)
> > >
> > > Signed-off-by: Imre Deak <imre.deak@intel.com>
> >
> > For completeness, add one to i915_ggtt_insert_entries(). Yes, I know
> > that we never will support rpm on any of those devices, but we may as
> > well be consistent in documenting that these functions will write
> > through the magic PCI region.
>
> Ok, forgot about that one.
>
> > An issue I see here is dev_priv->pm.suspended doesn't actually say that
> > rpm can't suspend during the GGTT PTEs writes. Would we not want
> > something more like assert_rpm_wakelock() and !dev_priv->pm.can_suspend?
>
> Well, after this patchset we assert that the RPM refcount!=0 too, which
> should guarantee that provided that pm.suspended=false as well. It's
> possible to have a non-zero refcount with a suspended state (in case of
> rpm_get_no_resume).

But you are right that assert_device_not_suspended doesn't reflect that
completely, so I could rename it.

>
> > -Chris
> >