Re: [PATCH v3 3/4] drm: rcar-du: Fix race condition when disabling planes at CRTC stop

Hi Laurent,

On 29/07/17 22:08, Laurent Pinchart wrote:

When stopping the CRTC the driver must disable all planes and wait for the change to take effect at the next vblank. Merely calling drm_crtc_wait_one_vblank() is not enough, as the function doesn't include any mechanism to handle the race with vblank interrupts.

Replace the drm_crtc_wait_one_vblank() call with a manual mechanism that handles the vblank interrupt race.

This looks like a nasty hidden race (though I understand I unintentionally helped make it more prominent :D )

Fix looks good to me.

Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com

Reviewed-by: Kieran Bingham kieran.bingham+renesas@ideasonboard.com

---

drivers/gpu/drm/rcar-du/rcar_du_crtc.c | 58 ++++++++++++++++++++++++++++++---- drivers/gpu/drm/rcar-du/rcar_du_crtc.h | 8 +++++ 2 files changed, 60 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c index 17fd1cd5212c..6e5bd0b92dfa 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.c @@ -490,23 +490,51 @@ static void rcar_du_crtc_start(struct rcar_du_crtc *rcrtc) rcar_du_group_start_stop(rcrtc->group, true); }

+static void rcar_du_crtc_disable_planes(struct rcar_du_crtc *rcrtc) +{

• struct rcar_du_device *rcdu = rcrtc->group->dev;
• struct drm_crtc *crtc = &rcrtc->crtc;
• u32 status;
• /* Make sure vblank interrupts are enabled. */
• drm_crtc_vblank_get(crtc);
• /*

• * Disable planes and calculate how many vertical blanking interrupts we
  

• * have to wait for. If a vertical blanking interrupt has been triggered
  

• * but not processed yet, we don't know whether it occurred before or
  

• * after the planes got disabled. We thus have to wait for two vblank
  

• * interrupts in that case.
  

• */
  

• spin_lock_irq(&rcrtc->vblank_lock);
• rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
• status = rcar_du_crtc_read(rcrtc, DSSR);
• rcrtc->vblank_count = status & DSSR_VBK ? 2 : 1;
• spin_unlock_irq(&rcrtc->vblank_lock);
• if (!wait_event_timeout(rcrtc->vblank_wait, rcrtc->vblank_count == 0,

• 		msecs_to_jiffies(100)))
  

• dev_warn(rcdu->dev, "vertical blanking timeout\n");
  

• drm_crtc_vblank_put(crtc);

+}

static void rcar_du_crtc_stop(struct rcar_du_crtc *rcrtc) { struct drm_crtc *crtc = &rcrtc->crtc;

/* * Disable all planes and wait for the change to take effect. This is

• * required as the DSnPR registers are updated on vblank, and no vblank
  

• * will occur once the CRTC is stopped. Disabling planes when starting
  

• * the CRTC thus wouldn't be enough as it would start scanning out
  

• * immediately from old frame buffers until the next vblank.
  

• * required as the plane enable registers are updated on vblank, and no
  

• * vblank will occur once the CRTC is stopped. Disabling planes when
  

• * starting the CRTC thus wouldn't be enough as it would start scanning
  

• * out immediately from old frame buffers until the next vblank.
  

  • This increases the CRTC stop delay, especially when multiple CRTCs
  • are stopped in one operation as we now wait for one vblank per CRTC.
  • Whether this can be improved needs to be researched.
  */

• rcar_du_group_write(rcrtc->group, rcrtc->index % 2 ? DS2PR : DS1PR, 0);
• drm_crtc_wait_one_vblank(crtc);

• rcar_du_crtc_disable_planes(rcrtc);

  /*

  • Disable vertical blanking interrupt reporting. We first need to wait

@@ -695,10 +723,26 @@ static irqreturn_t rcar_du_crtc_irq(int irq, void *arg) irqreturn_t ret = IRQ_NONE; u32 status;

• spin_lock(&rcrtc->vblank_lock);

• status = rcar_du_crtc_read(rcrtc, DSSR); rcar_du_crtc_write(rcrtc, DSRCR, status & DSRCR_MASK);

  if (status & DSSR_VBK) {

• /*
  

•  * Wake up the vblank wait if the counter reaches 0. This must
  

•  * be protected by the vblank_lock to avoid races in
  

•  * rcar_du_crtc_disable_planes().
  

•  */
  

• if (rcrtc->vblank_count) {
  

• 	if (--rcrtc->vblank_count == 0)
  

• 		wake_up(&rcrtc->vblank_wait);
  

• }
  

• }

• spin_unlock(&rcrtc->vblank_lock);

• if (status & DSSR_VBK) { drm_crtc_handle_vblank(&rcrtc->crtc);

  if (rcdu->info->gen < 3)

@@ -756,6 +800,8 @@ int rcar_du_crtc_create(struct rcar_du_group *rgrp, unsigned int index) }

init_waitqueue_head(&rcrtc->flip_wait);

• init_waitqueue_head(&rcrtc->vblank_wait);

• spin_lock_init(&rcrtc->vblank_lock);

  rcrtc->group = rgrp; rcrtc->mmio_offset = mmio_offsets[index];

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h index 3cc376826592..065b91f5b1d9 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_crtc.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_crtc.h @@ -15,6 +15,7 @@ #define __RCAR_DU_CRTC_H__

#include <linux/mutex.h> +#include <linux/spinlock.h> #include <linux/wait.h>

#include <drm/drmP.h> @@ -33,6 +34,9 @@ struct rcar_du_vsp;

• @initialized: whether the CRTC has been initialized and clocks enabled
• @event: event to post when the pending page flip completes
• @flip_wait: wait queue used to signal page flip completion

• • @vblank_lock: protects vblank_wait and vblank_count
• • @vblank_wait: wait queue used to signal vertical blanking
• • @vblank_count: number of vertical blanking interrupts to wait for
  • @outputs: bitmask of the outputs (enum rcar_du_output) driven by this CRTC
  • @group: CRTC group this CRTC belongs to
  • @vsp: VSP feeding video to this CRTC

@@ -50,6 +54,10 @@ struct rcar_du_crtc { struct drm_pending_vblank_event *event; wait_queue_head_t flip_wait;

• spinlock_t vblank_lock;

• wait_queue_head_t vblank_wait;

• unsigned int vblank_count;

• unsigned int outputs;

  struct rcar_du_group *group;