Re: [PATCH v3 12/28] drm/i915: Convert mmio_flip::seqno to struct request

[John Harrison <John.C.Harrison@Intel.com>]

On 26/11/2014 09:23, Daniel Vetter wrote:
> On Mon, Nov 24, 2014 at 06:49:34PM +0000, John.C.Harrison@Intel.com wrote:
>> From: John Harrison <John.C.Harrison@Intel.com>
>>
>> Converted the mmio_flip 'seqno' value to be a request structure as part of the
>> on going seqno to request changes. This includes reference counting the request
>> being saved away to ensure it can not be retired and freed while the flip code
>> is still waiting on it.
>>
>> v2: Used the IRQ friendly request dereference call in the notify handler as that
>> code is called asynchronously without holding any useful mutex locks.
>>
>> For: VIZ-4377
>> Signed-off-by: John Harrison <John.C.Harrison@Intel.com>
>> Reviewed-by: Thomas Daniel <Thomas.Daniel@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++----------
>>   drivers/gpu/drm/i915/intel_drv.h     |    3 +--
>>   2 files changed, 11 insertions(+), 12 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index 097e8a1..cbf3cb7 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -9550,18 +9550,19 @@ static void intel_mmio_flip_work_func(struct work_struct *work)
>>   {
>>   	struct intel_crtc *intel_crtc =
>>   		container_of(work, struct intel_crtc, mmio_flip.work);
>> -	struct intel_engine_cs *ring;
>> -	uint32_t seqno;
>> -
>> -	seqno = intel_crtc->mmio_flip.seqno;
>> -	ring = intel_crtc->mmio_flip.ring;
>> +	struct intel_mmio_flip *mmio_flip;
>>   
>> -	if (seqno)
>> -		WARN_ON(__i915_wait_seqno(ring, seqno,
>> +	mmio_flip = &intel_crtc->mmio_flip;
>> +	if (mmio_flip->req)
>> +		WARN_ON(__i915_wait_seqno(i915_gem_request_get_ring(mmio_flip->req),
>> +					  i915_gem_request_get_seqno(mmio_flip->req),
>>   					  intel_crtc->reset_counter,
>>   					  false, NULL, NULL) != 0);
>>   
>>   	intel_do_mmio_flip(intel_crtc);
>> +	if (mmio_flip->req)
>> +		i915_gem_request_unreference_irq(mmio_flip->req);
> Can't we just grab dev->struct_mutex around here and reuse the normal
> request_unref? That would allow us to ditch all the unref_irq complexity.
> -Daniel
The unref_irq is needed for the flip_queued_req as that is still 
dereferenced from interrupt time. Possibly this one could now be 
downgraded to a mutex lock but before the recent rebase, the mmio_flip 
request was also being dereferenced at interrupt time so definitely 
needed the irq friendly version. Is there a worry that waiting for the 
mutex lock could stall the flip handler so long that it misses the next 
flip?

>> +	mmio_flip->req = NULL;
>>   }
>>   
>>   static int intel_queue_mmio_flip(struct drm_device *dev,
>> @@ -9573,9 +9574,8 @@ static int intel_queue_mmio_flip(struct drm_device *dev,
>>   {
>>   	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>   
>> -	intel_crtc->mmio_flip.seqno =
>> -			     i915_gem_request_get_seqno(obj->last_write_req);
>> -	intel_crtc->mmio_flip.ring = obj->ring;
>> +	i915_gem_request_assign(&intel_crtc->mmio_flip.req,
>> +				obj->last_write_req);
>>   
>>   	schedule_work(&intel_crtc->mmio_flip.work);
>>   
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 44b153c5..2755532 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -406,8 +406,7 @@ struct intel_pipe_wm {
>>   };
>>   
>>   struct intel_mmio_flip {
>> -	u32 seqno;
>> -	struct intel_engine_cs *ring;
>> +	struct drm_i915_gem_request *req;
>>   	struct work_struct work;
>>   };
>>   
>> -- 
>> 1.7.9.5
>>
>> _______________________________________________
>> 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