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

[Imre Deak <imre.deak@intel.com>]

On ma, 2015-11-09 at 20:37 +0200, Ville Syrjälä wrote:
> On Mon, Nov 09, 2015 at 08:20:11PM +0200, Imre Deak wrote:
> > The device should be on when updating the GGTT PTEs, so add an
> > assert to
> > all relevant places.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 016739e..d4a84b8 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2358,6 +2358,8 @@ static void gen8_ggtt_insert_entries(struct
> > i915_address_space *vm,
> >  	struct sg_page_iter sg_iter;
> >  	dma_addr_t addr = 0; /* shut up gcc */
> >  
> > +	assert_device_not_suspended(vm->dev->dev_private);
> > +
> 
> We do have the GFX_FLSH_CNTL_GEN6 write in the insert_entries hooks,
> which should already do the check. But I see no harm in doing it
> explicitly as well. Serves as documentation if nothing more. The
> clear_range hooks don't do any register access, so there we should
> definitely have this.

Yes, I think it's better for symmetry with the other updaters. And it's
a WARN_ONCE, so shouldn't add noise.

> 
> >  	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
> >  		addr = sg_dma_address(sg_iter.sg) +
> >  			(sg_iter.sg_pgoffset << PAGE_SHIFT);
> > @@ -2404,6 +2406,8 @@ static void gen6_ggtt_insert_entries(struct
> > i915_address_space *vm,
> >  	struct sg_page_iter sg_iter;
> >  	dma_addr_t addr = 0;
> >  
> > +	assert_device_not_suspended(vm->dev->dev_private);
> 
> There's already a dev_priv around in all of these AFAICS.

Yep, just copy pasted these.. Will fix them.

> 
> > +
> >  	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
> >  		addr = sg_page_iter_dma_address(&sg_iter);
> >  		iowrite32(vm->pte_encode(addr, level, true,
> > flags), &gtt_entries[i]);
> > @@ -2442,6 +2446,8 @@ static void gen8_ggtt_clear_range(struct
> > i915_address_space *vm,
> >  	const int max_entries = gtt_total_entries(dev_priv->gtt) -
> > first_entry;
> >  	int i;
> >  
> > +	assert_device_not_suspended(vm->dev->dev_private);
> > +
> >  	if (WARN(num_entries > max_entries,
> >  		 "First entry = %d; Num entries = %d (max=%d)\n",
> >  		 first_entry, num_entries, max_entries))
> > @@ -2468,6 +2474,8 @@ static void gen6_ggtt_clear_range(struct
> > i915_address_space *vm,
> >  	const int max_entries = gtt_total_entries(dev_priv->gtt) -
> > first_entry;
> >  	int i;
> >  
> > +	assert_device_not_suspended(vm->dev->dev_private);
> > +
> >  	if (WARN(num_entries > max_entries,
> >  		 "First entry = %d; Num entries = %d (max=%d)\n",
> >  		 first_entry, num_entries, max_entries))
> > -- 
> > 2.5.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx