[Intel-gfx] [PATCH v3] drm/i915: Simplify and fix object to display tracking

[Chris Wilson]

On Tue, Mar 31, 2015 at 04:10:05PM +0300, Joonas Lahtinen wrote:
> On ti, 2015-03-31 at 13:55 +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > 
> > Purpose of this tracking is to know when to flush the cache between
> > the CPU and the non-coherent display engine. Prior to:
> > 
> >    commit 121920faf2ccce9aa66a7e2588415c9647b66104
> >    Author: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >    Date:   Mon Mar 23 11:10:37 2015 +0000
> > 
> >        drm/i915/skl: Query display address through a wrapper
> > 
> > This worked by a mix of direct flag manipulation and checking for
> > existence of a pinned GGTT VMA.
> > 
> > With the introduction of rotated display mappings this approach is
> > no longer correct.
> > 
> > New simpler approach is to just keep this count over calls which pin
> > and unpin objects to and from display, at the slight cost of extra
> > space in every bo.
> > 
> > (Inspired and extracted code from a larger rework by Chris Wilson.)
> > 
> > v2: Remove the limit since it is not well defined. (Chris Wilson, Ville Syrjälä)
> > v3: Commit message corrections. (Chris Wilson)
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> > Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk>
> > Cc: Chris Wilson <chris at chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h |  3 ++-
> >  drivers/gpu/drm/i915/i915_gem.c | 30 ++++++------------------------
> >  2 files changed, 8 insertions(+), 25 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 4ef320c..37abd58 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1969,7 +1969,6 @@ struct drm_i915_gem_object {
> >  	 */
> >  	unsigned int fault_mappable:1;
> >  	unsigned int pin_mappable:1;
> > -	unsigned int pin_display:1;
> >  
> >  	/*
> >  	 * Is the object to be mapped as read-only to the GPU
> > @@ -1983,6 +1982,8 @@ struct drm_i915_gem_object {
> >  
> >  	unsigned int frontbuffer_bits:INTEL_FRONTBUFFER_BITS;
> >  
> > +	unsigned int pin_display;
> > +
> 
> Here. Comment below.
> 
> >  	struct sg_table *pages;
> >  	int pages_pin_count;
> >  
> 
> How about naming it like the variable just below it (pages_pin_count).
> So let it be "display_pin_count", and make it just "int" too, I don't
> think the 1 bit reduction in pin count matters really, but having sign
> makes it useful for detecting negative pin count errors? At least I'd
> make it signed integer and make some of the spots WARN_ON(xxx--- < 0).

We already have the counter underflow detection. We could just as well
make pages_pin_count unsigned. I blame my own laziness.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre