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

John Harrison
2014-11-26T12:12:15Z

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