| Message ID | 1414501814-1465-3-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Oct 28, 2014 at 03:10:14PM +0200, Ander Conselvan de Oliveira wrote: > Currently we program just DPSCNTR and DSPSTRIDE directly from the ring > interrupt handler, which is fine since the hardware guarantees that > those are update atomically. When we have atomic page flips we'll want > to be able to update also the offset registers, and then we need to use > the vblank evade mechanism to guarantee atomicity. Since that mechanism > introduces a wait, we need to do the actual register write from a work > when it is triggered by the ring interrupt. > > v2: Explain the need for mmio_flip.work in the commit message (Paulo) > Initialize the mmio_flip work in intel_crtc_init() (Paulo) > Prevent new flips the previous flip work finishes (Paulo) > Don't acquire modeset locks for mmio flip work So that means you can replace the content of intel_do_mmio() by a call to dev_priv->display.update_primary_plane() on top of this patch? That'd do the right thing on SKL, which would make me happy.
2014-10-28 11:10 GMT-02:00 Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>: > Currently we program just DPSCNTR and DSPSTRIDE directly from the ring > interrupt handler, which is fine since the hardware guarantees that > those are update atomically. When we have atomic page flips we'll want > to be able to update also the offset registers, and then we need to use > the vblank evade mechanism to guarantee atomicity. Since that mechanism > introduces a wait, we need to do the actual register write from a work > when it is triggered by the ring interrupt. > > v2: Explain the need for mmio_flip.work in the commit message (Paulo) > Initialize the mmio_flip work in intel_crtc_init() (Paulo) > Prevent new flips the previous flip work finishes (Paulo) > Don't acquire modeset locks for mmio flip work > > Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++---- > drivers/gpu/drm/i915/intel_drv.h | 12 +++++++++++- > drivers/gpu/drm/i915/intel_sprite.c | 4 ++-- > 3 files changed, 39 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 8965f2d..86c2051 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -9314,11 +9314,15 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc) > struct intel_framebuffer *intel_fb = > to_intel_framebuffer(intel_crtc->base.primary->fb); > struct drm_i915_gem_object *obj = intel_fb->obj; > + bool atomic_update; > + u32 start_vbl_count; > u32 dspcntr; > u32 reg; > > intel_mark_page_flip_active(intel_crtc); > > + atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count); Don't we want to print something (error message?) in case this returns false? Besides this, my only worry is what is going to happen if we disable the CRTC or plane (or even suspend - S3 or runtime - or something like that) after we schedule the work, but before the work actually happens. Isn't it possible to end up accidentally reenabling something that was just disabled? We can't seem to queue a new flip before the current one finishes, but maybe we can shut down the CRTC while the work is scheduled? I don't know much about vblanks to properly answer that, so I'll let Daniel/Ville take a look at this. Everything else looks correct, so if someone guarantees the above question is not a problem: Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > + > reg = DSPCNTR(intel_crtc->plane); > dspcntr = I915_READ(reg); > > @@ -9332,6 +9336,21 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc) > I915_WRITE(DSPSURF(intel_crtc->plane), > intel_crtc->unpin_work->gtt_offset); > POSTING_READ(DSPSURF(intel_crtc->plane)); > + > + 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) > @@ -9374,15 +9393,15 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring) > struct intel_mmio_flip *mmio_flip; > > mmio_flip = &intel_crtc->mmio_flip; > - if (mmio_flip->seqno == 0) > + 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)) { > - intel_do_mmio_flip(intel_crtc); > - mmio_flip->seqno = 0; > + schedule_work(&intel_crtc->mmio_flip.work); > + mmio_flip->status = INTEL_MMIO_FLIP_WORK_SCHEDULED; > ring->irq_put(ring); > } > } > @@ -9400,7 +9419,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > int ret; > > - if (WARN_ON(intel_crtc->mmio_flip.seqno)) > + if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE)) > return -EBUSY; > > ret = intel_postpone_flip(obj); > @@ -9412,6 +9431,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, > } > > 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); > @@ -11883,6 +11903,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) > dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base; > dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base; > > + INIT_WORK(&intel_crtc->mmio_flip.work, intel_mmio_flip_work_func); > + > drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); > > WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index d53ac23..ff4af6b 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -399,9 +399,17 @@ 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 work_struct work; > }; > > struct intel_crtc { > @@ -1168,7 +1176,9 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data, > struct drm_file *file_priv); > int intel_sprite_get_colorkey(struct drm_device *dev, void *data, > struct drm_file *file_priv); > - > +bool intel_pipe_update_start(struct intel_crtc *crtc, > + uint32_t *start_vbl_count); > +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count); > > /* intel_tv.c */ > void intel_tv_init(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c > index c1d9547..9e2a5e2 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -62,7 +62,7 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs) > * > * Return: true if the call was successful > */ > -static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count) > +bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count) > { > struct drm_device *dev = crtc->base.dev; > const struct drm_display_mode *mode = &crtc->config.adjusted_mode; > @@ -135,7 +135,7 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl > * re-enables interrupts and verifies the update was actually completed > * before a vblank using the value of @start_vbl_count. > */ > -static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count) > +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count) > { > struct drm_device *dev = crtc->base.dev; > enum pipe pipe = crtc->pipe; > -- > 1.9.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Nov 3, 2014 at 7:37 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > Besides this, my only worry is what is going to happen if we disable > the CRTC or plane (or even suspend - S3 or runtime - or something like > that) after we schedule the work, but before the work actually > happens. Isn't it possible to end up accidentally reenabling something > that was just disabled? We can't seem to queue a new flip before the > current one finishes, but maybe we can shut down the CRTC while the > work is scheduled? I don't know much about vblanks to properly answer > that, so I'll let Daniel/Ville take a look at this. We do wait for pageflips to complete before disabling the crtc or updating the framebuffer. But I don't think we actually lack the corresponding code to do the same when disabling the pipe with dpms off. We have extensive sets of testcases to race pageflips against almost any combination of dpms calls, setCrtc changes, vblank waits or any other nonsense, so if that still works we should be good I think. Or maybe that's the reason why kms_flip has such a poor pass-rate? -Daniel
2014-11-03 16:53 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: > On Mon, Nov 3, 2014 at 7:37 PM, Paulo Zanoni <przanoni@gmail.com> wrote: >> Besides this, my only worry is what is going to happen if we disable >> the CRTC or plane (or even suspend - S3 or runtime - or something like >> that) after we schedule the work, but before the work actually >> happens. Isn't it possible to end up accidentally reenabling something >> that was just disabled? We can't seem to queue a new flip before the >> current one finishes, but maybe we can shut down the CRTC while the >> work is scheduled? I don't know much about vblanks to properly answer >> that, so I'll let Daniel/Ville take a look at this. > > We do wait for pageflips to complete before disabling the crtc or > updating the framebuffer. But, as far as I can see, those functions do not take into consideration the new workqueue added by this patch. > But I don't think we actually lack the > corresponding code to do the same when disabling the pipe with dpms > off. We have extensive sets of testcases to race pageflips against > almost any combination of dpms calls, setCrtc changes, vblank waits or > any other nonsense, so if that still works we should be good I think. > > Or maybe that's the reason why kms_flip has such a poor pass-rate? > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch
On Mon, Nov 3, 2014 at 8:13 PM, Paulo Zanoni <przanoni@gmail.com> wrote: > 2014-11-03 16:53 GMT-02:00 Daniel Vetter <daniel@ffwll.ch>: >> On Mon, Nov 3, 2014 at 7:37 PM, Paulo Zanoni <przanoni@gmail.com> wrote: >>> Besides this, my only worry is what is going to happen if we disable >>> the CRTC or plane (or even suspend - S3 or runtime - or something like >>> that) after we schedule the work, but before the work actually >>> happens. Isn't it possible to end up accidentally reenabling something >>> that was just disabled? We can't seem to queue a new flip before the >>> current one finishes, but maybe we can shut down the CRTC while the >>> work is scheduled? I don't know much about vblanks to properly answer >>> that, so I'll let Daniel/Ville take a look at this. >> >> We do wait for pageflips to complete before disabling the crtc or >> updating the framebuffer. > > But, as far as I can see, those functions do not take into > consideration the new workqueue added by this patch. Well we wait for the flip to happen. How that happens shouldn't really matter at all - the flip only completes on the next vblank, which might actaully be a lot later than when the work item has run. It's guaranteed to be after the mmio writes though. -Daniel
Hi Paulo, Thanks for reviewing. On 11/03/2014 08:37 PM, Paulo Zanoni wrote: > 2014-10-28 11:10 GMT-02:00 Ander Conselvan de Oliveira > <ander.conselvan.de.oliveira@intel.com>: >> Currently we program just DPSCNTR and DSPSTRIDE directly from the ring >> interrupt handler, which is fine since the hardware guarantees that >> those are update atomically. When we have atomic page flips we'll want >> to be able to update also the offset registers, and then we need to use >> the vblank evade mechanism to guarantee atomicity. Since that mechanism >> introduces a wait, we need to do the actual register write from a work >> when it is triggered by the ring interrupt. >> >> v2: Explain the need for mmio_flip.work in the commit message (Paulo) >> Initialize the mmio_flip work in intel_crtc_init() (Paulo) >> Prevent new flips the previous flip work finishes (Paulo) >> Don't acquire modeset locks for mmio flip work >> >> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++---- >> drivers/gpu/drm/i915/intel_drv.h | 12 +++++++++++- >> drivers/gpu/drm/i915/intel_sprite.c | 4 ++-- >> 3 files changed, 39 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 8965f2d..86c2051 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -9314,11 +9314,15 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc) >> struct intel_framebuffer *intel_fb = >> to_intel_framebuffer(intel_crtc->base.primary->fb); >> struct drm_i915_gem_object *obj = intel_fb->obj; >> + bool atomic_update; >> + u32 start_vbl_count; >> u32 dspcntr; >> u32 reg; >> >> intel_mark_page_flip_active(intel_crtc); >> >> + atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count); > > Don't we want to print something (error message?) in case this returns false? Good question. The function itself has a warn for one of the failure cases (when drm_vblank_get() fails) but it doesn't print anything if the calculation for min and max scanlines fails. It seems the latter case would fail if we have a bad mode (with vblank_start <= 1) or in some unrealistic scenarios involving a combination of low pixel clocks and high htotal. In any case, I guess we would be better to add a the warning to intel_pipe_update_start() itself, since there are other users that follow the same pattern. Ander > Besides this, my only worry is what is going to happen if we disable > the CRTC or plane (or even suspend - S3 or runtime - or something like > that) after we schedule the work, but before the work actually > happens. Isn't it possible to end up accidentally reenabling something > that was just disabled? We can't seem to queue a new flip before the > current one finishes, but maybe we can shut down the CRTC while the > work is scheduled? I don't know much about vblanks to properly answer > that, so I'll let Daniel/Ville take a look at this. > > Everything else looks correct, so if someone guarantees the above > question is not a problem: Reviewed-by: Paulo Zanoni > <paulo.r.zanoni@intel.com> > >> + >> reg = DSPCNTR(intel_crtc->plane); >> dspcntr = I915_READ(reg); >> >> @@ -9332,6 +9336,21 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc) >> I915_WRITE(DSPSURF(intel_crtc->plane), >> intel_crtc->unpin_work->gtt_offset); >> POSTING_READ(DSPSURF(intel_crtc->plane)); >> + >> + 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) >> @@ -9374,15 +9393,15 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring) >> struct intel_mmio_flip *mmio_flip; >> >> mmio_flip = &intel_crtc->mmio_flip; >> - if (mmio_flip->seqno == 0) >> + 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)) { >> - intel_do_mmio_flip(intel_crtc); >> - mmio_flip->seqno = 0; >> + schedule_work(&intel_crtc->mmio_flip.work); >> + mmio_flip->status = INTEL_MMIO_FLIP_WORK_SCHEDULED; >> ring->irq_put(ring); >> } >> } >> @@ -9400,7 +9419,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> int ret; >> >> - if (WARN_ON(intel_crtc->mmio_flip.seqno)) >> + if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE)) >> return -EBUSY; >> >> ret = intel_postpone_flip(obj); >> @@ -9412,6 +9431,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, >> } >> >> 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); >> @@ -11883,6 +11903,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) >> dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base; >> dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base; >> >> + INIT_WORK(&intel_crtc->mmio_flip.work, intel_mmio_flip_work_func); >> + >> drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); >> >> WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe); >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index d53ac23..ff4af6b 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -399,9 +399,17 @@ 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 work_struct work; >> }; >> >> struct intel_crtc { >> @@ -1168,7 +1176,9 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data, >> struct drm_file *file_priv); >> int intel_sprite_get_colorkey(struct drm_device *dev, void *data, >> struct drm_file *file_priv); >> - >> +bool intel_pipe_update_start(struct intel_crtc *crtc, >> + uint32_t *start_vbl_count); >> +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count); >> >> /* intel_tv.c */ >> void intel_tv_init(struct drm_device *dev); >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index c1d9547..9e2a5e2 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -62,7 +62,7 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs) >> * >> * Return: true if the call was successful >> */ >> -static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count) >> +bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count) >> { >> struct drm_device *dev = crtc->base.dev; >> const struct drm_display_mode *mode = &crtc->config.adjusted_mode; >> @@ -135,7 +135,7 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl >> * re-enables interrupts and verifies the update was actually completed >> * before a vblank using the value of @start_vbl_count. >> */ >> -static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count) >> +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count) >> { >> struct drm_device *dev = crtc->base.dev; >> enum pipe pipe = crtc->pipe; >> -- >> 1.9.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > --------------------------------------------------------------------- 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 Tue, Oct 28, 2014 at 03:10:14PM +0200, Ander Conselvan de Oliveira wrote: > @@ -9400,7 +9419,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > int ret; > > - if (WARN_ON(intel_crtc->mmio_flip.seqno)) > + if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE)) > return -EBUSY; Ok I've merged the patch, but I think this check is wrong. do_mmio_flip only sets this to idle _after_ the hw register writing is done. Which means the flip could complete and userspace could receive the event all before we set status to IDLE. Which means you can WARN and return -EBUSY here when the flip definitely completed. So either clear IDLE at the head of do_mmio_flip before doing anything, or drop this state again. After all the previous seqno check only looked for waiting or not. tbh I'm not even sure whether this buys us anything at all - we already check that the flip completed, which should ensure ordering. I think we could just rip out mmio_flip.status completely. Can you pls do a fixup patch? Thanks, Daniel
On Tue, Nov 04, 2014 at 12:04:03PM +0100, Daniel Vetter wrote: > On Tue, Oct 28, 2014 at 03:10:14PM +0200, Ander Conselvan de Oliveira wrote: > > @@ -9400,7 +9419,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > int ret; > > > > - if (WARN_ON(intel_crtc->mmio_flip.seqno)) > > + if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE)) > > return -EBUSY; > > Ok I've merged the patch, but I think this check is wrong. do_mmio_flip > only sets this to idle _after_ the hw register writing is done. Which > means the flip could complete and userspace could receive the event all > before we set status to IDLE. Which means you can WARN and return -EBUSY > here when the flip definitely completed. > > So either clear IDLE at the head of do_mmio_flip before doing anything, or > drop this state again. After all the previous seqno check only looked for > waiting or not. tbh I'm not even sure whether this buys us anything at all > - we already check that the flip completed, which should ensure ordering. > I think we could just rip out mmio_flip.status completely. > > Can you pls do a fixup patch? A better strategy would be to use the generic seqno mechanism for the flip from a worker which then wouldn't need this extra vblank state machinery on top of the vblank evade... -Chris
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 8965f2d..86c2051 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -9314,11 +9314,15 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc) struct intel_framebuffer *intel_fb = to_intel_framebuffer(intel_crtc->base.primary->fb); struct drm_i915_gem_object *obj = intel_fb->obj; + bool atomic_update; + u32 start_vbl_count; u32 dspcntr; u32 reg; intel_mark_page_flip_active(intel_crtc); + atomic_update = intel_pipe_update_start(intel_crtc, &start_vbl_count); + reg = DSPCNTR(intel_crtc->plane); dspcntr = I915_READ(reg); @@ -9332,6 +9336,21 @@ static void intel_do_mmio_flip(struct intel_crtc *intel_crtc) I915_WRITE(DSPSURF(intel_crtc->plane), intel_crtc->unpin_work->gtt_offset); POSTING_READ(DSPSURF(intel_crtc->plane)); + + 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) @@ -9374,15 +9393,15 @@ void intel_notify_mmio_flip(struct intel_engine_cs *ring) struct intel_mmio_flip *mmio_flip; mmio_flip = &intel_crtc->mmio_flip; - if (mmio_flip->seqno == 0) + 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)) { - intel_do_mmio_flip(intel_crtc); - mmio_flip->seqno = 0; + schedule_work(&intel_crtc->mmio_flip.work); + mmio_flip->status = INTEL_MMIO_FLIP_WORK_SCHEDULED; ring->irq_put(ring); } } @@ -9400,7 +9419,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, struct intel_crtc *intel_crtc = to_intel_crtc(crtc); int ret; - if (WARN_ON(intel_crtc->mmio_flip.seqno)) + if (WARN_ON(intel_crtc->mmio_flip.status != INTEL_MMIO_FLIP_IDLE)) return -EBUSY; ret = intel_postpone_flip(obj); @@ -9412,6 +9431,7 @@ static int intel_queue_mmio_flip(struct drm_device *dev, } 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); @@ -11883,6 +11903,8 @@ static void intel_crtc_init(struct drm_device *dev, int pipe) dev_priv->plane_to_crtc_mapping[intel_crtc->plane] = &intel_crtc->base; dev_priv->pipe_to_crtc_mapping[intel_crtc->pipe] = &intel_crtc->base; + INIT_WORK(&intel_crtc->mmio_flip.work, intel_mmio_flip_work_func); + drm_crtc_helper_add(&intel_crtc->base, &intel_helper_funcs); WARN_ON(drm_crtc_index(&intel_crtc->base) != intel_crtc->pipe); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index d53ac23..ff4af6b 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -399,9 +399,17 @@ 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 work_struct work; }; struct intel_crtc { @@ -1168,7 +1176,9 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data, struct drm_file *file_priv); int intel_sprite_get_colorkey(struct drm_device *dev, void *data, struct drm_file *file_priv); - +bool intel_pipe_update_start(struct intel_crtc *crtc, + uint32_t *start_vbl_count); +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count); /* intel_tv.c */ void intel_tv_init(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index c1d9547..9e2a5e2 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -62,7 +62,7 @@ static int usecs_to_scanlines(const struct drm_display_mode *mode, int usecs) * * Return: true if the call was successful */ -static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count) +bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl_count) { struct drm_device *dev = crtc->base.dev; const struct drm_display_mode *mode = &crtc->config.adjusted_mode; @@ -135,7 +135,7 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl * re-enables interrupts and verifies the update was actually completed * before a vblank using the value of @start_vbl_count. */ -static void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count) +void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count) { struct drm_device *dev = crtc->base.dev; enum pipe pipe = crtc->pipe;
Currently we program just DPSCNTR and DSPSTRIDE directly from the ring interrupt handler, which is fine since the hardware guarantees that those are update atomically. When we have atomic page flips we'll want to be able to update also the offset registers, and then we need to use the vblank evade mechanism to guarantee atomicity. Since that mechanism introduces a wait, we need to do the actual register write from a work when it is triggered by the ring interrupt. v2: Explain the need for mmio_flip.work in the commit message (Paulo) Initialize the mmio_flip work in intel_crtc_init() (Paulo) Prevent new flips the previous flip work finishes (Paulo) Don't acquire modeset locks for mmio flip work Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 30 ++++++++++++++++++++++++++---- drivers/gpu/drm/i915/intel_drv.h | 12 +++++++++++- drivers/gpu/drm/i915/intel_sprite.c | 4 ++-- 3 files changed, 39 insertions(+), 7 deletions(-)