| Message ID | 1433408582-9828-43-git-send-email-tomi.valkeinen@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Tomi, Thank you for the patch. On Thursday 04 June 2015 12:02:59 Tomi Valkeinen wrote: > omap_atomic_complete() uses drm_atomic_helper_wait_for_vblanks() to wait > for all operations to finish. That works, but can easily cause waits for > vblanks when no wait is actually necessary. It actually doesn't work properly, as a full mode set without any change to framebuffers will need to reenable planes after enabling the CRTC and thus sets the go bit, but the drm_atomic_helper_wait_for_vblanks() function called during atomic commit waits for vblank only on CRTCs that have seen a page flip. This leads to missing waits, resulting in a go busy warning if the next .atomic_flush() call occurs too soon. Your patch fixes that issue, so I'm fine with the code, but I think the commit message should be fixed. Then, Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > This patch adds omap_atomic_wait_for_gos() and uses it instead. > omap_atomic_wait_for_gos() waits for the GO bit to get unset for all > relevant crtcs. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/omapdrm/omap_crtc.c | 7 +++++++ > drivers/gpu/drm/omapdrm/omap_drv.c | 31 ++++++++++++++++++++++++++++++- > drivers/gpu/drm/omapdrm/omap_drv.h | 1 + > 3 files changed, 38 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 2c0d91a67418..8f905d2c8074 > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -80,6 +80,13 @@ enum omap_channel omap_crtc_channel(struct drm_crtc > *crtc) return omap_crtc->channel; > } > > +bool omap_crtc_needs_wait(struct drm_crtc *crtc) > +{ > + struct omap_crtc *omap_crtc = to_omap_crtc(crtc); > + > + return dispc_mgr_go_busy(omap_crtc->channel); > +} > + > /* > --------------------------------------------------------------------------- > -- * DSS Manager Functions > */ > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c > b/drivers/gpu/drm/omapdrm/omap_drv.c index 50f555530e55..c03405593f9f > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.c > +++ b/drivers/gpu/drm/omapdrm/omap_drv.c > @@ -66,6 +66,35 @@ struct omap_atomic_state_commit { > u32 crtcs; > }; > > +static void omap_atomic_wait_for_gos(struct drm_device *dev, > + struct drm_atomic_state *old_state) > +{ > + struct drm_crtc *crtc; > + struct drm_crtc_state *old_crtc_state; > + int i, ret; > + > + for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { > + if (!crtc->state->enable) > + continue; > + > + if (!omap_crtc_needs_wait(crtc)) > + continue; > + > + ret = drm_crtc_vblank_get(crtc); > + if (ret != 0) > + continue; > + > + ret = wait_event_timeout(dev->vblank[i].queue, > + !omap_crtc_needs_wait(crtc), > + msecs_to_jiffies(50)); > + if (!ret) > + dev_warn(dev->dev, > + "atomic flush timeout (pipe %u)!\n", i); > + > + drm_crtc_vblank_put(crtc); > + } > +} > + > static void omap_atomic_complete(struct omap_atomic_state_commit *commit) > { > struct drm_device *dev = commit->dev; > @@ -79,7 +108,7 @@ static void omap_atomic_complete(struct > omap_atomic_state_commit *commit) drm_atomic_helper_commit_planes(dev, > old_state); > drm_atomic_helper_commit_modeset_enables(dev, old_state); > > - drm_atomic_helper_wait_for_vblanks(dev, old_state); > + omap_atomic_wait_for_gos(dev, old_state); > > drm_atomic_helper_cleanup_planes(dev, old_state); > > diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h > b/drivers/gpu/drm/omapdrm/omap_drv.h index 0b7a055bf007..27bbf7c4d76e > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_drv.h > +++ b/drivers/gpu/drm/omapdrm/omap_drv.h > @@ -147,6 +147,7 @@ void omap_crtc_pre_init(void); > void omap_crtc_pre_uninit(void); > struct drm_crtc *omap_crtc_init(struct drm_device *dev, > struct drm_plane *plane, enum omap_channel channel, int id); > +bool omap_crtc_needs_wait(struct drm_crtc *crtc); > > struct drm_plane *omap_plane_init(struct drm_device *dev, > int id, enum drm_plane_type type);
On 06/06/15 07:10, Laurent Pinchart wrote: > Hi Tomi, > > Thank you for the patch. > > On Thursday 04 June 2015 12:02:59 Tomi Valkeinen wrote: >> omap_atomic_complete() uses drm_atomic_helper_wait_for_vblanks() to wait >> for all operations to finish. That works, but can easily cause waits for >> vblanks when no wait is actually necessary. > > It actually doesn't work properly, as a full mode set without any change to > framebuffers will need to reenable planes after enabling the CRTC and thus > sets the go bit, but the drm_atomic_helper_wait_for_vblanks() function called That's not how it goes. In complete(), we first disable the crtcs, then set the planes, then enable the crtcs. We don't first enable the crtcs, and then enable the planes. So everything is already configured at the time we enable the crtc, and no GO bit is needed, and thus no wait is needed. > during atomic commit waits for vblank only on CRTCs that have seen a page > flip. This leads to missing waits, resulting in a go busy warning if the next > .atomic_flush() call occurs too soon. I think those issues came from your patch that moved the plane config to be done after crtc enable. I dropped that patch. Tomi
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 2c0d91a67418..8f905d2c8074 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -80,6 +80,13 @@ enum omap_channel omap_crtc_channel(struct drm_crtc *crtc) return omap_crtc->channel; } +bool omap_crtc_needs_wait(struct drm_crtc *crtc) +{ + struct omap_crtc *omap_crtc = to_omap_crtc(crtc); + + return dispc_mgr_go_busy(omap_crtc->channel); +} + /* ----------------------------------------------------------------------------- * DSS Manager Functions */ diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c index 50f555530e55..c03405593f9f 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.c +++ b/drivers/gpu/drm/omapdrm/omap_drv.c @@ -66,6 +66,35 @@ struct omap_atomic_state_commit { u32 crtcs; }; +static void omap_atomic_wait_for_gos(struct drm_device *dev, + struct drm_atomic_state *old_state) +{ + struct drm_crtc *crtc; + struct drm_crtc_state *old_crtc_state; + int i, ret; + + for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) { + if (!crtc->state->enable) + continue; + + if (!omap_crtc_needs_wait(crtc)) + continue; + + ret = drm_crtc_vblank_get(crtc); + if (ret != 0) + continue; + + ret = wait_event_timeout(dev->vblank[i].queue, + !omap_crtc_needs_wait(crtc), + msecs_to_jiffies(50)); + if (!ret) + dev_warn(dev->dev, + "atomic flush timeout (pipe %u)!\n", i); + + drm_crtc_vblank_put(crtc); + } +} + static void omap_atomic_complete(struct omap_atomic_state_commit *commit) { struct drm_device *dev = commit->dev; @@ -79,7 +108,7 @@ static void omap_atomic_complete(struct omap_atomic_state_commit *commit) drm_atomic_helper_commit_planes(dev, old_state); drm_atomic_helper_commit_modeset_enables(dev, old_state); - drm_atomic_helper_wait_for_vblanks(dev, old_state); + omap_atomic_wait_for_gos(dev, old_state); drm_atomic_helper_cleanup_planes(dev, old_state); diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h b/drivers/gpu/drm/omapdrm/omap_drv.h index 0b7a055bf007..27bbf7c4d76e 100644 --- a/drivers/gpu/drm/omapdrm/omap_drv.h +++ b/drivers/gpu/drm/omapdrm/omap_drv.h @@ -147,6 +147,7 @@ void omap_crtc_pre_init(void); void omap_crtc_pre_uninit(void); struct drm_crtc *omap_crtc_init(struct drm_device *dev, struct drm_plane *plane, enum omap_channel channel, int id); +bool omap_crtc_needs_wait(struct drm_crtc *crtc); struct drm_plane *omap_plane_init(struct drm_device *dev, int id, enum drm_plane_type type);
omap_atomic_complete() uses drm_atomic_helper_wait_for_vblanks() to wait for all operations to finish. That works, but can easily cause waits for vblanks when no wait is actually necessary. This patch adds omap_atomic_wait_for_gos() and uses it instead. omap_atomic_wait_for_gos() waits for the GO bit to get unset for all relevant crtcs. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/omap_crtc.c | 7 +++++++ drivers/gpu/drm/omapdrm/omap_drv.c | 31 ++++++++++++++++++++++++++++++- drivers/gpu/drm/omapdrm/omap_drv.h | 1 + 3 files changed, 38 insertions(+), 1 deletion(-)