[PATCH 1/4] gpu: ipu-v3: pre: add double buffer status readback

[PATCH 1/4] gpu: ipu-v3: pre: add double buffer status readback

[Lucas Stach]

This allows the upper layers to check if a double buffer update has
been applied by the PRE or is still pending.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/ipu-v3/ipu-pre.c | 6 ++++++
 drivers/gpu/ipu-v3/ipu-prv.h | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/ipu-v3/ipu-pre.c b/drivers/gpu/ipu-v3/ipu-pre.c
index 2f8db9d62551..7389ad157fd6 100644
--- a/drivers/gpu/ipu-v3/ipu-pre.c
+++ b/drivers/gpu/ipu-v3/ipu-pre.c
@@ -259,6 +259,12 @@ void ipu_pre_update(struct ipu_pre *pre, unsigned int bufaddr)
 	writel(IPU_PRE_CTRL_SDW_UPDATE, pre->regs + IPU_PRE_CTRL_SET);
 }
 
+bool ipu_pre_update_done(struct ipu_pre *pre)
+{
+	return !(readl_relaxed(pre->regs + IPU_PRE_CTRL) &
+		 IPU_PRE_CTRL_SDW_UPDATE);
+}
+
 u32 ipu_pre_get_baddr(struct ipu_pre *pre)
 {
 	return (u32)pre->buffer_paddr;
diff --git a/drivers/gpu/ipu-v3/ipu-prv.h b/drivers/gpu/ipu-v3/ipu-prv.h
index d6beee99b6b8..215d342d8441 100644
--- a/drivers/gpu/ipu-v3/ipu-prv.h
+++ b/drivers/gpu/ipu-v3/ipu-prv.h
@@ -272,6 +272,7 @@ void ipu_pre_configure(struct ipu_pre *pre, unsigned int width,
 		       unsigned int height, unsigned int stride, u32 format,
 		       uint64_t modifier, unsigned int bufaddr);
 void ipu_pre_update(struct ipu_pre *pre, unsigned int bufaddr);
+bool ipu_pre_update_done(struct ipu_pre *pre);
 
 struct ipu_prg *ipu_prg_lookup_by_phandle(struct device *dev, const char *name,
 					  int ipu_id);
-- 
2.19.0

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

[PATCH 2/4] gpu: ipu-v3: prg: add function to get channel configure status

[Lucas Stach]

This allows channels using the PRG to check if a requested configuration
update has been applied or is still pending.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/ipu-v3/ipu-prg.c | 16 ++++++++++++++++
 include/video/imx-ipu-v3.h   |  1 +
 2 files changed, 17 insertions(+)

diff --git a/drivers/gpu/ipu-v3/ipu-prg.c b/drivers/gpu/ipu-v3/ipu-prg.c
index 38a3a9764e49..f78463cf1c15 100644
--- a/drivers/gpu/ipu-v3/ipu-prg.c
+++ b/drivers/gpu/ipu-v3/ipu-prg.c
@@ -347,6 +347,22 @@ int ipu_prg_channel_configure(struct ipuv3_channel *ipu_chan,
 }
 EXPORT_SYMBOL_GPL(ipu_prg_channel_configure);
 
+int ipu_prg_channel_configure_done(struct ipuv3_channel *ipu_chan)
+{
+	int prg_chan = ipu_prg_ipu_to_prg_chan(ipu_chan->num);
+	struct ipu_prg *prg = ipu_chan->ipu->prg_priv;
+	struct ipu_prg_channel *chan;
+
+	if (prg_chan < 0)
+		return -EINVAL;
+
+	chan = &prg->chan[prg_chan];
+	WARN_ON(!chan->enabled);
+
+	return ipu_pre_update_done(prg->pres[chan->used_pre]);
+}
+EXPORT_SYMBOL_GPL(ipu_prg_channel_configure_done);
+
 static int ipu_prg_probe(struct platform_device *pdev)
 {
 	struct device *dev = &pdev->dev;
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index abbad94e14a1..45802eab1084 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -345,6 +345,7 @@ int ipu_prg_channel_configure(struct ipuv3_channel *ipu_chan,
 			      unsigned int axi_id,  unsigned int width,
 			      unsigned int height, unsigned int stride,
 			      u32 format, uint64_t modifier, unsigned long *eba);
+int ipu_prg_channel_configure_done(struct ipuv3_channel *ipu_chan);
 
 /*
  * IPU CMOS Sensor Interface (csi) functions
-- 
2.19.0

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

[PATCH 3/4] drm/imx: ipuv3-plane: add function to query atomic update status

[Lucas Stach]

This function allows upper layer to check if a requested atomic update
to the plane has been applied or is still pending.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 20 ++++++++++++++++++++
 drivers/gpu/drm/imx/ipuv3-plane.h |  2 ++
 2 files changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 203f247d4854..c95a2fc51741 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -587,6 +587,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 		active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
 		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
 		ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
+		ipu_plane->next_buf = !active;
 		if (ipu_plane_separate_alpha(ipu_plane)) {
 			active = ipu_idmac_get_current_buffer(ipu_plane->alpha_ch);
 			ipu_cpmem_set_buffer(ipu_plane->alpha_ch, !active,
@@ -713,6 +714,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 	ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
 	ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 1, eba);
 	ipu_idmac_lock_enable(ipu_plane->ipu_ch, num_bursts);
+	ipu_plane->next_buf = -1;
 	ipu_plane_enable(ipu_plane);
 }
 
@@ -723,6 +725,24 @@ static const struct drm_plane_helper_funcs ipu_plane_helper_funcs = {
 	.atomic_update = ipu_plane_atomic_update,
 };
 
+bool ipu_plane_atomic_update_done(struct drm_plane *plane)
+{
+	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
+	struct drm_plane_state *state = plane->state;
+	struct ipu_plane_state *ipu_state = to_ipu_plane_state(state);
+
+	/* disabled crtcs must not block the update */
+	if (!state->crtc)
+		return true;
+
+	if (ipu_state->use_pre)
+		return ipu_prg_channel_configure_done(ipu_plane->ipu_ch);
+	else if (ipu_plane->next_buf >= 0)
+		return ipu_idmac_get_current_buffer(ipu_plane->ipu_ch) ==
+		       ipu_plane->next_buf;
+
+	return true;
+}
 int ipu_planes_assign_pre(struct drm_device *dev,
 			  struct drm_atomic_state *state)
 {
diff --git a/drivers/gpu/drm/imx/ipuv3-plane.h b/drivers/gpu/drm/imx/ipuv3-plane.h
index e563ea17a827..60eb6776a34e 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.h
+++ b/drivers/gpu/drm/imx/ipuv3-plane.h
@@ -27,6 +27,7 @@ struct ipu_plane {
 	int			dp_flow;
 
 	bool			disabling;
+	int			next_buf;
 };
 
 struct ipu_plane *ipu_plane_init(struct drm_device *dev, struct ipu_soc *ipu,
@@ -48,5 +49,6 @@ int ipu_plane_irq(struct ipu_plane *plane);
 
 void ipu_plane_disable(struct ipu_plane *ipu_plane, bool disable_dp_channel);
 void ipu_plane_disable_deferred(struct drm_plane *plane);
+bool ipu_plane_atomic_update_done(struct drm_plane *plane);
 
 #endif
-- 
2.19.0

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

[PATCH 4/4] drm/imx: only send commit done event when all state has been applied

[Lucas Stach]

Currently there is a small race window where we could manage to arm the
vblank event from atomic flush, but programming the hardware was too close
to the frame end, so the hardware will only apply the current state on the
next vblank. In this case we will send out the commit done event too early
causing userspace to reuse framebuffes that are still in use.

Instead of using the event arming mechnism, just remember the pending event
and send it from the vblank IRQ handler, once we are sure that all state
has been applied sucessfully.

Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
---
 drivers/gpu/drm/imx/ipuv3-crtc.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
index 7d4b710b837a..b0c95565a28d 100644
--- a/drivers/gpu/drm/imx/ipuv3-crtc.c
+++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
@@ -42,6 +42,7 @@ struct ipu_crtc {
 	struct ipu_dc		*dc;
 	struct ipu_di		*di;
 	int			irq;
+	struct drm_pending_vblank_event *event;
 };
 
 static inline struct ipu_crtc *to_ipu_crtc(struct drm_crtc *crtc)
@@ -181,8 +182,31 @@ static const struct drm_crtc_funcs ipu_crtc_funcs = {
 static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
 {
 	struct ipu_crtc *ipu_crtc = dev_id;
+	struct drm_crtc *crtc = &ipu_crtc->base;
+	unsigned long flags;
+	int i;
+
+	drm_crtc_handle_vblank(crtc);
+
+	if (ipu_crtc->event) {
+		for (i = 0; i < ARRAY_SIZE(ipu_crtc->plane); i++) {
+			struct ipu_plane *plane = ipu_crtc->plane[i];
+
+			if (!plane)
+				continue;
+
+			if (!ipu_plane_atomic_update_done(&plane->base))
+				break;
+		}
 
-	drm_crtc_handle_vblank(&ipu_crtc->base);
+		if (i == ARRAY_SIZE(ipu_crtc->plane)) {
+			spin_lock_irqsave(&crtc->dev->event_lock, flags);
+			drm_crtc_send_vblank_event(crtc, ipu_crtc->event);
+			ipu_crtc->event = NULL;
+			drm_crtc_vblank_put(crtc);
+			spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
+		}
+	}
 
 	return IRQ_HANDLED;
 }
@@ -229,13 +253,13 @@ static void ipu_crtc_atomic_begin(struct drm_crtc *crtc,
 static void ipu_crtc_atomic_flush(struct drm_crtc *crtc,
 				  struct drm_crtc_state *old_crtc_state)
 {
-	spin_lock_irq(&crtc->dev->event_lock);
+	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
+
 	if (crtc->state->event) {
 		WARN_ON(drm_crtc_vblank_get(crtc));
-		drm_crtc_arm_vblank_event(crtc, crtc->state->event);
+		ipu_crtc->event = crtc->state->event;
 		crtc->state->event = NULL;
 	}
-	spin_unlock_irq(&crtc->dev->event_lock);
 }
 
 static void ipu_crtc_mode_set_nofb(struct drm_crtc *crtc)
-- 
2.19.0

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

Re: [PATCH 1/4] gpu: ipu-v3: pre: add double buffer status readback

[Philipp Zabel]

Hi Lucas,

On Fri, 2018-09-14 at 18:59 +0200, Lucas Stach wrote:
> This allows the upper layers to check if a double buffer update has
> been applied by the PRE or is still pending.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/ipu-v3/ipu-pre.c | 6 ++++++
>  drivers/gpu/ipu-v3/ipu-prv.h | 1 +
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/ipu-v3/ipu-pre.c b/drivers/gpu/ipu-v3/ipu-pre.c
> index 2f8db9d62551..7389ad157fd6 100644
> --- a/drivers/gpu/ipu-v3/ipu-pre.c
> +++ b/drivers/gpu/ipu-v3/ipu-pre.c
> @@ -259,6 +259,12 @@ void ipu_pre_update(struct ipu_pre *pre, unsigned int bufaddr)
>  	writel(IPU_PRE_CTRL_SDW_UPDATE, pre->regs + IPU_PRE_CTRL_SET);
>  }
>  
> +bool ipu_pre_update_done(struct ipu_pre *pre)

Nitpick, can we invert the return value and call this
ipu_pre_update_pending? That way we don't return
update_done == true if no update has been issued at all.
The same goes for patches 2 and 3.

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 2/4] gpu: ipu-v3: prg: add function to get channel configure status

[Philipp Zabel]

On Fri, 2018-09-14 at 18:59 +0200, Lucas Stach wrote:
> This allows channels using the PRG to check if a requested configuration
> update has been applied or is still pending.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/ipu-v3/ipu-prg.c | 16 ++++++++++++++++
>  include/video/imx-ipu-v3.h   |  1 +
>  2 files changed, 17 insertions(+)
> 
> diff --git a/drivers/gpu/ipu-v3/ipu-prg.c b/drivers/gpu/ipu-v3/ipu-prg.c
> index 38a3a9764e49..f78463cf1c15 100644
> --- a/drivers/gpu/ipu-v3/ipu-prg.c
> +++ b/drivers/gpu/ipu-v3/ipu-prg.c
> @@ -347,6 +347,22 @@ int ipu_prg_channel_configure(struct ipuv3_channel *ipu_chan,
>  }
>  EXPORT_SYMBOL_GPL(ipu_prg_channel_configure);
>  
> +int ipu_prg_channel_configure_done(struct ipuv3_channel *ipu_chan)

Same as patch 1, I'd prefer ipu_prg_channel_configure_pending instead.
Also the return value is only ever used as a boolean, so just make this
return bool.

> +{
> +	int prg_chan = ipu_prg_ipu_to_prg_chan(ipu_chan->num);
> +	struct ipu_prg *prg = ipu_chan->ipu->prg_priv;
> +	struct ipu_prg_channel *chan;
> +
> +	if (prg_chan < 0)
> +		return -EINVAL;

Since nonexisting channels can't have updates pending, let's just return
false here.

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 3/4] drm/imx: ipuv3-plane: add function to query atomic update status

[Philipp Zabel]

On Fri, 2018-09-14 at 18:59 +0200, Lucas Stach wrote:
> This function allows upper layer to check if a requested atomic update
> to the plane has been applied or is still pending.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c | 20 ++++++++++++++++++++
>  drivers/gpu/drm/imx/ipuv3-plane.h |  2 ++
>  2 files changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 203f247d4854..c95a2fc51741 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -587,6 +587,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  		active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
>  		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
>  		ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
> +		ipu_plane->next_buf = !active;
>  		if (ipu_plane_separate_alpha(ipu_plane)) {
>  			active = ipu_idmac_get_current_buffer(ipu_plane->alpha_ch);
>  			ipu_cpmem_set_buffer(ipu_plane->alpha_ch, !active,
> @@ -713,6 +714,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  	ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
>  	ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 1, eba);
>  	ipu_idmac_lock_enable(ipu_plane->ipu_ch, num_bursts);
> +	ipu_plane->next_buf = -1;
>  	ipu_plane_enable(ipu_plane);
>  }
>  
> @@ -723,6 +725,24 @@ static const struct drm_plane_helper_funcs ipu_plane_helper_funcs = {
>  	.atomic_update = ipu_plane_atomic_update,
>  };
>  
> +bool ipu_plane_atomic_update_done(struct drm_plane *plane)

bool ipu_plane_atomic_update_pending

> +{
> +	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
> +	struct drm_plane_state *state = plane->state;
> +	struct ipu_plane_state *ipu_state = to_ipu_plane_state(state);
> +
> +	/* disabled crtcs must not block the update */
> +	if (!state->crtc)
> +		return true;
> +
> +	if (ipu_state->use_pre)
> +		return ipu_prg_channel_configure_done(ipu_plane->ipu_ch);

This hides the -EINVAL return value being interpreted as true, better
have this return bool.

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/imx: only send commit done event when all state has been applied

[Philipp Zabel]

On Fri, 2018-09-14 at 18:59 +0200, Lucas Stach wrote:
> Currently there is a small race window where we could manage to arm the
> vblank event from atomic flush, but programming the hardware was too close
> to the frame end, so the hardware will only apply the current state on the
> next vblank. In this case we will send out the commit done event too early
> causing userspace to reuse framebuffes that are still in use.
> 
> Instead of using the event arming mechnism, just remember the pending event
> and send it from the vblank IRQ handler, once we are sure that all state
> has been applied sucessfully.
> 
> Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/ipuv3-crtc.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> index 7d4b710b837a..b0c95565a28d 100644
> --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> @@ -42,6 +42,7 @@ struct ipu_crtc {
>  	struct ipu_dc		*dc;
>  	struct ipu_di		*di;
>  	int			irq;
> +	struct drm_pending_vblank_event *event;
>  };
>  
>  static inline struct ipu_crtc *to_ipu_crtc(struct drm_crtc *crtc)
> @@ -181,8 +182,31 @@ static const struct drm_crtc_funcs ipu_crtc_funcs = {
>  static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
>  {
>  	struct ipu_crtc *ipu_crtc = dev_id;
> +	struct drm_crtc *crtc = &ipu_crtc->base;
> +	unsigned long flags;
> +	int i;
> +
> +	drm_crtc_handle_vblank(crtc);
> +
> +	if (ipu_crtc->event) {
> +		for (i = 0; i < ARRAY_SIZE(ipu_crtc->plane); i++) {
> +			struct ipu_plane *plane = ipu_crtc->plane[i];
> +
> +			if (!plane)
> +				continue;
> +
> +			if (!ipu_plane_atomic_update_done(&plane->base))

			if (ipu_plane_atomic_update_pending(&plane->base))

> +				break;
> +		}
>  
> -	drm_crtc_handle_vblank(&ipu_crtc->base);
> +		if (i == ARRAY_SIZE(ipu_crtc->plane)) {
> +			spin_lock_irqsave(&crtc->dev->event_lock, flags);
> +			drm_crtc_send_vblank_event(crtc, ipu_crtc->event);
> +			ipu_crtc->event = NULL;

These two happen under the event spinlock, but where event is set in
ipu_crtc_atomic_flush, the locking is removed.

> +			drm_crtc_vblank_put(crtc);
> +			spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> +		}
> +	}
>  
>  	return IRQ_HANDLED;
>  }
> @@ -229,13 +253,13 @@ static void ipu_crtc_atomic_begin(struct drm_crtc *crtc,
>  static void ipu_crtc_atomic_flush(struct drm_crtc *crtc,
>  				  struct drm_crtc_state *old_crtc_state)
>  {
> -	spin_lock_irq(&crtc->dev->event_lock);
> +	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
> +
>  	if (crtc->state->event) {
>  		WARN_ON(drm_crtc_vblank_get(crtc));
> -		drm_crtc_arm_vblank_event(crtc, crtc->state->event);
> +		ipu_crtc->event = crtc->state->event;

We assume here that ipu_crtc->event is NULL and the irq handler is never
running at the same time, otherwise we would drop an event. This is non-
obvious to me, and I think it warrants a comment.

My understanding is the following:

- It is virtually impossible for atomic_flush to race against a delayed
  previous ipu_irq_handler because the previous commit's commit_tail
  would still be waiting for the vblank event to release it from
  drm_atomic_helper_wait_for_flip_done.

  However, if the last commit's tail finishes after the irq_handler
  calls drm_crtc_send_vblank_event(), and the new commit is issued, and
  its tail work scheduled, all before the next line in the irq_handler,
  ipu_crtc->event = NULL, then the new commit's tail could call
  drm_atomic_helper_commit_planes and therefore ipu_crtc_atomic_flush
  and ipu_crtc->event would be overwritten.

- It is unproblematic for a delayed atomic_flush to race against the
  next ipu_irq_handler because ipu_crtc->event will just not be set
  when the irq handler checks it, and the vblank event will be deferred
  to the next interrupt.

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/imx: only send commit done event when all state has been applied

[Philipp Zabel]

On Fri, 2018-10-05 at 17:11 +0200, Philipp Zabel wrote:
> On Fri, 2018-09-14 at 18:59 +0200, Lucas Stach wrote:
> > Currently there is a small race window where we could manage to arm the
> > vblank event from atomic flush, but programming the hardware was too close
> > to the frame end, so the hardware will only apply the current state on the
> > next vblank. In this case we will send out the commit done event too early
> > causing userspace to reuse framebuffes that are still in use.
> > 
> > Instead of using the event arming mechnism, just remember the pending event
> > and send it from the vblank IRQ handler, once we are sure that all state
> > has been applied sucessfully.
> > 
> > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > ---
> >  drivers/gpu/drm/imx/ipuv3-crtc.c | 32 ++++++++++++++++++++++++++++----
> >  1 file changed, 28 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > index 7d4b710b837a..b0c95565a28d 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > @@ -42,6 +42,7 @@ struct ipu_crtc {
> >  	struct ipu_dc		*dc;
> >  	struct ipu_di		*di;
> >  	int			irq;
> > +	struct drm_pending_vblank_event *event;
> >  };
> >  
> >  static inline struct ipu_crtc *to_ipu_crtc(struct drm_crtc *crtc)
> > @@ -181,8 +182,31 @@ static const struct drm_crtc_funcs ipu_crtc_funcs = {
> >  static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
> >  {
> >  	struct ipu_crtc *ipu_crtc = dev_id;
> > +	struct drm_crtc *crtc = &ipu_crtc->base;
> > +	unsigned long flags;
> > +	int i;
> > +
> > +	drm_crtc_handle_vblank(crtc);
> > +
> > +	if (ipu_crtc->event) {
> > +		for (i = 0; i < ARRAY_SIZE(ipu_crtc->plane); i++) {
> > +			struct ipu_plane *plane = ipu_crtc->plane[i];
> > +
> > +			if (!plane)
> > +				continue;
> > +
> > +			if (!ipu_plane_atomic_update_done(&plane->base))
> 
> 			if (ipu_plane_atomic_update_pending(&plane->base))
> 
> > +				break;
> > +		}
> >  
> > -	drm_crtc_handle_vblank(&ipu_crtc->base);
> > +		if (i == ARRAY_SIZE(ipu_crtc->plane)) {
> > +			spin_lock_irqsave(&crtc->dev->event_lock, flags);
> > +			drm_crtc_send_vblank_event(crtc, ipu_crtc->event);
> > +			ipu_crtc->event = NULL;
> 
> These two happen under the event spinlock, but where event is set in
> ipu_crtc_atomic_flush, the locking is removed.
> 
> > +			drm_crtc_vblank_put(crtc);
> > +			spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > +		}
> > +	}
> >  
> >  	return IRQ_HANDLED;
> >  }
> > @@ -229,13 +253,13 @@ static void ipu_crtc_atomic_begin(struct drm_crtc *crtc,
> >  static void ipu_crtc_atomic_flush(struct drm_crtc *crtc,
> >  				  struct drm_crtc_state *old_crtc_state)
> >  {
> > -	spin_lock_irq(&crtc->dev->event_lock);
> > +	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
> > +
> >  	if (crtc->state->event) {
> >  		WARN_ON(drm_crtc_vblank_get(crtc));
> > -		drm_crtc_arm_vblank_event(crtc, crtc->state->event);
> > +		ipu_crtc->event = crtc->state->event;
> 
> We assume here that ipu_crtc->event is NULL and the irq handler is never
> running at the same time, otherwise we would drop an event. This is non-
> obvious to me, and I think it warrants a comment.
> 
> My understanding is the following:
> 
> - It is virtually impossible for atomic_flush to race against a delayed
>   previous ipu_irq_handler because the previous commit's commit_tail
>   would still be waiting for the vblank event to release it from
>   drm_atomic_helper_wait_for_flip_done.
> 
>   However, if the last commit's tail finishes after the irq_handler
>   calls drm_crtc_send_vblank_event(), and the new commit is issued, and
>   its tail work scheduled, all before the next line in the irq_handler,
>   ipu_crtc->event = NULL, then the new commit's tail could call
>   drm_atomic_helper_commit_planes and therefore ipu_crtc_atomic_flush
>   and ipu_crtc->event would be overwritten.
> 
> - It is unproblematic for a delayed atomic_flush to race against the
>   next ipu_irq_handler because ipu_crtc->event will just not be set
>   when the irq handler checks it, and the vblank event will be deferred
>   to the next interrupt.

How do we proceed with this? Keep the spin lock?

regards
Philipp
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 4/4] drm/imx: only send commit done event when all state has been applied

[Daniel Vetter]

On Wed, Jan 23, 2019 at 12:35:02PM +0100, Philipp Zabel wrote:
> On Fri, 2018-10-05 at 17:11 +0200, Philipp Zabel wrote:
> > On Fri, 2018-09-14 at 18:59 +0200, Lucas Stach wrote:
> > > Currently there is a small race window where we could manage to arm the
> > > vblank event from atomic flush, but programming the hardware was too close
> > > to the frame end, so the hardware will only apply the current state on the
> > > next vblank. In this case we will send out the commit done event too early
> > > causing userspace to reuse framebuffes that are still in use.
> > > 
> > > Instead of using the event arming mechnism, just remember the pending event
> > > and send it from the vblank IRQ handler, once we are sure that all state
> > > has been applied sucessfully.
> > > 
> > > Signed-off-by: Lucas Stach <l.stach@pengutronix.de>
> > > ---
> > >  drivers/gpu/drm/imx/ipuv3-crtc.c | 32 ++++++++++++++++++++++++++++----
> > >  1 file changed, 28 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/imx/ipuv3-crtc.c b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > index 7d4b710b837a..b0c95565a28d 100644
> > > --- a/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > +++ b/drivers/gpu/drm/imx/ipuv3-crtc.c
> > > @@ -42,6 +42,7 @@ struct ipu_crtc {
> > >  	struct ipu_dc		*dc;
> > >  	struct ipu_di		*di;
> > >  	int			irq;
> > > +	struct drm_pending_vblank_event *event;
> > >  };
> > >  
> > >  static inline struct ipu_crtc *to_ipu_crtc(struct drm_crtc *crtc)
> > > @@ -181,8 +182,31 @@ static const struct drm_crtc_funcs ipu_crtc_funcs = {
> > >  static irqreturn_t ipu_irq_handler(int irq, void *dev_id)
> > >  {
> > >  	struct ipu_crtc *ipu_crtc = dev_id;
> > > +	struct drm_crtc *crtc = &ipu_crtc->base;
> > > +	unsigned long flags;
> > > +	int i;
> > > +
> > > +	drm_crtc_handle_vblank(crtc);
> > > +
> > > +	if (ipu_crtc->event) {
> > > +		for (i = 0; i < ARRAY_SIZE(ipu_crtc->plane); i++) {
> > > +			struct ipu_plane *plane = ipu_crtc->plane[i];
> > > +
> > > +			if (!plane)
> > > +				continue;
> > > +
> > > +			if (!ipu_plane_atomic_update_done(&plane->base))
> > 
> > 			if (ipu_plane_atomic_update_pending(&plane->base))
> > 
> > > +				break;
> > > +		}
> > >  
> > > -	drm_crtc_handle_vblank(&ipu_crtc->base);
> > > +		if (i == ARRAY_SIZE(ipu_crtc->plane)) {
> > > +			spin_lock_irqsave(&crtc->dev->event_lock, flags);
> > > +			drm_crtc_send_vblank_event(crtc, ipu_crtc->event);
> > > +			ipu_crtc->event = NULL;
> > 
> > These two happen under the event spinlock, but where event is set in
> > ipu_crtc_atomic_flush, the locking is removed.
> > 
> > > +			drm_crtc_vblank_put(crtc);
> > > +			spin_unlock_irqrestore(&crtc->dev->event_lock, flags);
> > > +		}
> > > +	}
> > >  
> > >  	return IRQ_HANDLED;
> > >  }
> > > @@ -229,13 +253,13 @@ static void ipu_crtc_atomic_begin(struct drm_crtc *crtc,
> > >  static void ipu_crtc_atomic_flush(struct drm_crtc *crtc,
> > >  				  struct drm_crtc_state *old_crtc_state)
> > >  {
> > > -	spin_lock_irq(&crtc->dev->event_lock);
> > > +	struct ipu_crtc *ipu_crtc = to_ipu_crtc(crtc);
> > > +
> > >  	if (crtc->state->event) {
> > >  		WARN_ON(drm_crtc_vblank_get(crtc));
> > > -		drm_crtc_arm_vblank_event(crtc, crtc->state->event);
> > > +		ipu_crtc->event = crtc->state->event;
> > 
> > We assume here that ipu_crtc->event is NULL and the irq handler is never
> > running at the same time, otherwise we would drop an event. This is non-
> > obvious to me, and I think it warrants a comment.
> > 
> > My understanding is the following:
> > 
> > - It is virtually impossible for atomic_flush to race against a delayed
> >   previous ipu_irq_handler because the previous commit's commit_tail
> >   would still be waiting for the vblank event to release it from
> >   drm_atomic_helper_wait_for_flip_done.
> > 
> >   However, if the last commit's tail finishes after the irq_handler
> >   calls drm_crtc_send_vblank_event(), and the new commit is issued, and
> >   its tail work scheduled, all before the next line in the irq_handler,
> >   ipu_crtc->event = NULL, then the new commit's tail could call
> >   drm_atomic_helper_commit_planes and therefore ipu_crtc_atomic_flush
> >   and ipu_crtc->event would be overwritten.
> > 
> > - It is unproblematic for a delayed atomic_flush to race against the
> >   next ipu_irq_handler because ipu_crtc->event will just not be set
> >   when the irq handler checks it, and the vblank event will be deferred
> >   to the next interrupt.
> 
> How do we proceed with this? Keep the spin lock?

Yeah, standard practice is to protect these things with a spinlock,
usually the drm->event_lock. Then the flip_done wait should make sure
overall ordering is correct, too.

Might be good to improve the kerneldocs that this is a recommended
pattern.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel