| Message ID | 1415185380-6609-1-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Nov 05, 2014 at 01:03:00PM +0200, Ander Conselvan de Oliveira wrote: > This simplifies the code quite a bit compared to iterating over all > rings during the ring interrupt. > > Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip > struct is only accessed in two places. The first is when the flip is > queued and the other when the mmio writes are done. Since a flip cannot > be queued while there is a pending flip, the two paths shouldn't ever > run in parallel. We might need to revisit that if support for replacing > flips is implemented though. > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > > I'm not sure if locking dev->struct_mutex in the work function might > have any ill effects, so I'd appreciate if someone could enlighten me. Good news is you can wait on the seqno without holding any locks. You need to use the lower level entry point __wait_seqno() though. Overall it looked good. -Chris
On 11/05/2014 01:23 PM, Chris Wilson wrote: > On Wed, Nov 05, 2014 at 01:03:00PM +0200, Ander Conselvan de Oliveira wrote: >> This simplifies the code quite a bit compared to iterating over all >> rings during the ring interrupt. >> >> Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip >> struct is only accessed in two places. The first is when the flip is >> queued and the other when the mmio writes are done. Since a flip cannot >> be queued while there is a pending flip, the two paths shouldn't ever >> run in parallel. We might need to revisit that if support for replacing >> flips is implemented though. >> >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> >> --- >> >> I'm not sure if locking dev->struct_mutex in the work function might >> have any ill effects, so I'd appreciate if someone could enlighten me. > > Good news is you can wait on the seqno without holding any locks. You > need to use the lower level entry point __wait_seqno() though. I considered the fact that __wait_seqno() is static as a warning that I shouldn't use it directly. Should I just change that so I can use it intel_display.c? The other reason I avoided it is because I didn't really understand how the barrier for reading the reset_counter should work. Is it sufficient that I do atomic_read(&dev_priv->gpu_error.reset_counter)) before calling __wait_seqno() or is there something more to it? Thanks, ander --------------------------------------------------------------------- Intel Finland Oy Registered Address: PL 281, 00181 Helsinki Business Identity Code: 0357606 - 4 Domiciled in Helsinki This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). Any review or distribution by others is strictly prohibited. If you are not the intended recipient, please contact the sender and delete all copies.
On Wed, Nov 05, 2014 at 02:23:07PM +0200, Ander Conselvan de Oliveira wrote: > On 11/05/2014 01:23 PM, Chris Wilson wrote: > >On Wed, Nov 05, 2014 at 01:03:00PM +0200, Ander Conselvan de Oliveira wrote: > >>This simplifies the code quite a bit compared to iterating over all > >>rings during the ring interrupt. > >> > >>Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip > >>struct is only accessed in two places. The first is when the flip is > >>queued and the other when the mmio writes are done. Since a flip cannot > >>be queued while there is a pending flip, the two paths shouldn't ever > >>run in parallel. We might need to revisit that if support for replacing > >>flips is implemented though. > >> > >>Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > >>--- > >> > >>I'm not sure if locking dev->struct_mutex in the work function might > >>have any ill effects, so I'd appreciate if someone could enlighten me. > > > >Good news is you can wait on the seqno without holding any locks. You > >need to use the lower level entry point __wait_seqno() though. > > I considered the fact that __wait_seqno() is static as a warning > that I shouldn't use it directly. Should I just change that so I can > use it intel_display.c? > > The other reason I avoided it is because I didn't really understand > how the barrier for reading the reset_counter should work. Is it > sufficient that I do > > atomic_read(&dev_priv->gpu_error.reset_counter)) > > before calling __wait_seqno() or is there something more to it? We should already have a reset counter associated with the flip (at least I do...) But yes, the least you need to do is pass down the current atomic_read(&reset_counter). That it is __wait_seqno() is indeed meant to make you think twice before using it, but really the only problem with is the name and cumbersome arguments. I have no qualms about renaming it as __i915_wait_seqno() and using it here - it is what I settled on in the requests conversion. -Chris
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 318a6a0..5fff287 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -979,9 +979,6 @@ static void notify_ring(struct drm_device *dev, trace_i915_gem_request_complete(ring); - if (drm_core_check_feature(dev, DRIVER_MODESET)) - intel_notify_mmio_flip(ring); - wake_up_all(&ring->irq_queue); i915_queue_hangcheck(dev); } diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index f3ff8e8..24cfe19 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9424,73 +9424,25 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc) if (atomic_update) intel_pipe_update_end(intel_crtc, start_vbl_count); - - spin_lock_irq(&dev_priv->mmio_flip_lock); - intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_IDLE; - spin_unlock_irq(&dev_priv->mmio_flip_lock); } 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); - - intel_do_mmio_flip(intel_crtc); -} - -static int intel_postpone_flip(struct drm_i915_gem_object *obj) -{ struct intel_engine_cs *ring; - int ret; - - lockdep_assert_held(&obj->base.dev->struct_mutex); - - if (!obj->last_write_seqno) - return 0; - - ring = obj->ring; + uint32_t seqno; - if (i915_seqno_passed(ring->get_seqno(ring, true), - obj->last_write_seqno)) - return 0; - - ret = i915_gem_check_olr(ring, obj->last_write_seqno); - if (ret) - return ret; - - if (WARN_ON(!ring->irq_get(ring))) - return 0; + seqno = intel_crtc->mmio_flip.seqno; + ring = intel_crtc->mmio_flip.ring; - return 1; -} - -void intel_notify_mmio_flip(struct intel_engine_cs *ring) -{ - struct drm_i915_private *dev_priv = to_i915(ring->dev); - struct intel_crtc *intel_crtc; - unsigned long irq_flags; - u32 seqno; - - seqno = ring->get_seqno(ring, false); - - spin_lock_irqsave(&dev_priv->mmio_flip_lock, irq_flags); - for_each_intel_crtc(ring->dev, intel_crtc) { - struct intel_mmio_flip *mmio_flip; - - mmio_flip = &intel_crtc->mmio_flip; - if (mmio_flip->status != INTEL_MMIO_FLIP_WAIT_RING) - continue; - - if (ring->id != mmio_flip->ring_id) - continue; - - if (i915_seqno_passed(seqno, mmio_flip->seqno)) { - schedule_work(&intel_crtc->mmio_flip.work); - mmio_flip->status = INTEL_MMIO_FLIP_WORK_SCHEDULED; - ring->irq_put(ring); - } + if (seqno) { + mutex_lock(&ring->dev->struct_mutex); + WARN_ON(i915_wait_seqno(ring, seqno) != 0); + mutex_unlock(&ring->dev->struct_mutex); } - spin_unlock_irqrestore(&dev_priv->mmio_flip_lock, irq_flags); + + intel_do_mmio_flip(intel_crtc); } static int intel_queue_mmio_flip(struct drm_device *dev, @@ -9500,32 +9452,13 @@ static int intel_queue_mmio_flip(struct drm_device *dev, struct intel_engine_cs *ring, uint32_t flags) { - struct drm_i915_private *dev_priv = dev->dev_private; struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - int ret; - - if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE)) - return -EBUSY; - - ret = intel_postpone_flip(obj); - if (ret < 0) - return ret; - if (ret == 0) { - intel_do_mmio_flip(intel_crtc); - return 0; - } - spin_lock_irq(&dev_priv->mmio_flip_lock); - intel_crtc->mmio_flip.status = INTEL_MMIO_FLIP_WAIT_RING; intel_crtc->mmio_flip.seqno = obj->last_write_seqno; - intel_crtc->mmio_flip.ring_id = obj->ring->id; - spin_unlock_irq(&dev_priv->mmio_flip_lock); + intel_crtc->mmio_flip.ring = obj->ring; + + schedule_work(&intel_crtc->mmio_flip.work); - /* - * Double check to catch cases where irq fired before - * mmio flip data was ready - */ - intel_notify_mmio_flip(obj->ring); return 0; } diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index c8ddde6..492d346 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -399,16 +399,9 @@ struct intel_pipe_wm { bool sprites_scaled; }; -enum intel_mmio_flip_status { - INTEL_MMIO_FLIP_IDLE = 0, - INTEL_MMIO_FLIP_WAIT_RING, - INTEL_MMIO_FLIP_WORK_SCHEDULED, -}; - struct intel_mmio_flip { u32 seqno; - u32 ring_id; - enum intel_mmio_flip_status status; + struct intel_engine_cs *ring; struct work_struct work; };
This simplifies the code quite a bit compared to iterating over all rings during the ring interrupt. Also, it allows us to drop the mmio_flip spinlock, since the mmio_flip struct is only accessed in two places. The first is when the flip is queued and the other when the mmio writes are done. Since a flip cannot be queued while there is a pending flip, the two paths shouldn't ever run in parallel. We might need to revisit that if support for replacing flips is implemented though. Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- I'm not sure if locking dev->struct_mutex in the work function might have any ill effects, so I'd appreciate if someone could enlighten me. Thanks, Ander --- drivers/gpu/drm/i915/i915_irq.c | 3 -- drivers/gpu/drm/i915/intel_display.c | 91 +++++------------------------------- drivers/gpu/drm/i915/intel_drv.h | 9 +--- 3 files changed, 13 insertions(+), 90 deletions(-)