Re: [PATCHv2 43/45] drm: omapdrm: don't wait in crtc_atomic_flush

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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];

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel