Re: [PATCH v3] drm/i915: Simplify and fix object to display tracking

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3] drm/i915: Simplify and fix object to display tracking
Date: Tue, 31 Mar 2015 14:15:58 +0100	[thread overview]
Message-ID: <551A9E0E.5060007@linux.intel.com> (raw)
In-Reply-To: <1427807405.21482.6.camel@jlahtine-mobl1>

Hi,

On 03/31/2015 02:10 PM, Joonas Lahtinen wrote:
> On ti, 2015-03-31 at 13:55 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@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@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@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Chris Wilson <chris@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).

It can't go negative due check in i915_gem_object_unpin_from_display_plane.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx