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@xxxxxx> > --- > 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]; -- Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
