| Message ID | 1433408582-9828-44-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:03:00 Tomi Valkeinen wrote: > omap_crtc_atomic_flush() sets the GO bit and waits for it to get unset, > which tells us the HW has taken the new configuration into use. > > This is unnecessary as omap_atomic_complete() waits for all the GO bits > to get unset. In fact, waiting is wrong, as with multiple CRTCs we would > not get the the changes to all the CRTCs as soon as possible, but only > one after another. > > This patch removes the wait. > > Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> > --- > drivers/gpu/drm/omapdrm/omap_crtc.c | 12 ------------ > 1 file changed, 12 deletions(-) > > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 8f905d2c8074..2ec34dc0c66c > 100644 > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c > @@ -17,8 +17,6 @@ > * this program. If not, see <http://www.gnu.org/licenses/>. > */ > > -#include <linux/completion.h> > - > #include <drm/drm_atomic.h> > #include <drm/drm_atomic_helper.h> > #include <drm/drm_crtc.h> > @@ -52,8 +50,6 @@ struct omap_crtc { > /* pending event */ > struct drm_pending_vblank_event *event; > > - struct completion completion; > - > bool ignore_digit_sync_lost; > }; > > @@ -317,8 +313,6 @@ static void omap_crtc_vblank_irq(struct omap_drm_irq > *irq, uint32_t irqstatus) > > /* wakeup userspace */ > omap_crtc_complete_page_flip(&omap_crtc->base); > - > - complete(&omap_crtc->completion); > } > > static int omap_crtc_flush(struct drm_crtc *crtc) > @@ -332,10 +326,6 @@ static int omap_crtc_flush(struct drm_crtc *crtc) > if (dispc_mgr_is_enabled(omap_crtc->channel)) { > dispc_mgr_go(omap_crtc->channel); > omap_irq_register(crtc->dev, &omap_crtc->vblank_irq); > - > - WARN_ON(!wait_for_completion_timeout(&omap_crtc->completion, > - msecs_to_jiffies(100))); > - reinit_completion(&omap_crtc->completion); I wonder whether this could introduce a race condition between the CRTC vblank interrupt handler register here, and the wait for gos in atomic_commit. The latter might run before our CRTC vblank interrupt handler, potentially starting processing of the next commit with vblank_irq still registered. Bonus points if you can remove vblank_irq completely while fixing that :-) I'd rather see omap_crtc_vblank_irq() being called directly and unconditionally from omap_irq_handler(), and the drm_crtc_handle_vblank() call being moved to omap_crtc_vblank_irq(). > } > > return 0; > @@ -517,8 +507,6 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, > > crtc = &omap_crtc->base; > > - init_completion(&omap_crtc->completion); > - > omap_crtc->channel = channel; > omap_crtc->name = channel_names[channel];
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c index 8f905d2c8074..2ec34dc0c66c 100644 --- a/drivers/gpu/drm/omapdrm/omap_crtc.c +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c @@ -17,8 +17,6 @@ * this program. If not, see <http://www.gnu.org/licenses/>. */ -#include <linux/completion.h> - #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_crtc.h> @@ -52,8 +50,6 @@ struct omap_crtc { /* pending event */ struct drm_pending_vblank_event *event; - struct completion completion; - bool ignore_digit_sync_lost; }; @@ -317,8 +313,6 @@ static void omap_crtc_vblank_irq(struct omap_drm_irq *irq, uint32_t irqstatus) /* wakeup userspace */ omap_crtc_complete_page_flip(&omap_crtc->base); - - complete(&omap_crtc->completion); } static int omap_crtc_flush(struct drm_crtc *crtc) @@ -332,10 +326,6 @@ static int omap_crtc_flush(struct drm_crtc *crtc) if (dispc_mgr_is_enabled(omap_crtc->channel)) { dispc_mgr_go(omap_crtc->channel); omap_irq_register(crtc->dev, &omap_crtc->vblank_irq); - - WARN_ON(!wait_for_completion_timeout(&omap_crtc->completion, - msecs_to_jiffies(100))); - reinit_completion(&omap_crtc->completion); } return 0; @@ -517,8 +507,6 @@ struct drm_crtc *omap_crtc_init(struct drm_device *dev, crtc = &omap_crtc->base; - init_completion(&omap_crtc->completion); - omap_crtc->channel = channel; omap_crtc->name = channel_names[channel];
omap_crtc_atomic_flush() sets the GO bit and waits for it to get unset, which tells us the HW has taken the new configuration into use. This is unnecessary as omap_atomic_complete() waits for all the GO bits to get unset. In fact, waiting is wrong, as with multiple CRTCs we would not get the the changes to all the CRTCs as soon as possible, but only one after another. This patch removes the wait. Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com> --- drivers/gpu/drm/omapdrm/omap_crtc.c | 12 ------------ 1 file changed, 12 deletions(-)