[PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>


As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
update is requested and there is an earlier update pending".


This patch is based on the rockchip patch below:
    http://article.gmane.org/gmane.comp.video.dri.devel/151678

Note: This patch was resent as the original submission wasn't addressed
      to the apparopriate recipients.

Robert Foss (1):
  drm/vc4: Return -EBUSY if there's already a pending flip event.

 drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
 drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.5.0

[PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>


As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
update is requested and there is an earlier update pending".


This patch is based on the rockchip patch below:
    http://article.gmane.org/gmane.comp.video.dri.devel/151678

Note: This patch was resent as the original submission wasn't addressed
      to the apparopriate recipients.

Robert Foss (1):
  drm/vc4: Return -EBUSY if there's already a pending flip event.

 drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
 drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

-- 
2.5.0

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

[PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
update is requested and there is an earlier update pending".

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
 drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 355ee4b..43193a3 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
 		.of_match_table = vc4_crtc_dt_match,
 	},
 };
+
+bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
+{
+	assert_spin_locked(&crtc->dev->event_lock);
+	return to_vc4_crtc(crtc)->event;
+}
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index fa2ad15..54c1fb5 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
 int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
 void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
 int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
+bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
 
 /* vc4_debugfs.c */
 int vc4_debugfs_init(struct drm_minor *minor);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 4718ae5..dc157a1e 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
 			     bool async)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
-	int ret;
-	int i;
 	uint64_t wait_seqno = 0;
 	struct vc4_commit *c;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	unsigned long flags;
+	int i, ret;
+
+	if (async) {
+		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+
+			spin_lock_irqsave(&dev->event_lock, flags);
+
+			if (crtc->state->event || 
+				vc4_crtc_has_pending_event(crtc)) {
+				spin_unlock_irqrestore(&dev->event_lock, flags);
+				return -EBUSY;
+			}
+			spin_unlock_irqrestore(&dev->event_lock, flags);
+		}
+	}
 
 	c = commit_init(state);
 	if (!c)
-- 
2.5.0

[PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
update is requested and there is an earlier update pending".

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
 drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 355ee4b..43193a3 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
 		.of_match_table = vc4_crtc_dt_match,
 	},
 };
+
+bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
+{
+	assert_spin_locked(&crtc->dev->event_lock);
+	return to_vc4_crtc(crtc)->event;
+}
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index fa2ad15..54c1fb5 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
 int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
 void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
 int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
+bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
 
 /* vc4_debugfs.c */
 int vc4_debugfs_init(struct drm_minor *minor);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 4718ae5..dc157a1e 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
 			     bool async)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
-	int ret;
-	int i;
 	uint64_t wait_seqno = 0;
 	struct vc4_commit *c;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	unsigned long flags;
+	int i, ret;
+
+	if (async) {
+		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+
+			spin_lock_irqsave(&dev->event_lock, flags);
+
+			if (crtc->state->event || 
+				vc4_crtc_has_pending_event(crtc)) {
+				spin_unlock_irqrestore(&dev->event_lock, flags);
+				return -EBUSY;
+			}
+			spin_unlock_irqrestore(&dev->event_lock, flags);
+		}
+	}
 
 	c = commit_init(state);
 	if (!c)
-- 
2.5.0

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

Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 3762 bytes --]

robert.foss@collabora.com writes:

> From: Robert Foss <robert.foss@collabora.com>
>
> As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
> update is requested and there is an earlier update pending".

Note: docs cited here are drm_crtc.h, and the whole quote is:

	 *  - -EBUSY, if an asynchronous updated is requested and there is
	 *    an earlier updated pending. Drivers are allowed to support a queue
	 *    of outstanding updates, but currently no driver supports that.
	 *    Note that drivers must wait for preceding updates to complete if a
	 *    synchronous update is requested, they are not allowed to fail the
	 *    commit in that case.

> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
>  drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>  drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
>  3 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 355ee4b..43193a3 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
>  		.of_match_table = vc4_crtc_dt_match,
>  	},
>  };
> +
> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
> +{
> +	assert_spin_locked(&crtc->dev->event_lock);
> +	return to_vc4_crtc(crtc)->event;
> +}
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index fa2ad15..54c1fb5 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
>  int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
>  void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
>  int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
>  
>  /* vc4_debugfs.c */
>  int vc4_debugfs_init(struct drm_minor *minor);
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 4718ae5..dc157a1e 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
>  			     bool async)
>  {
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
> -	int ret;
> -	int i;
>  	uint64_t wait_seqno = 0;
>  	struct vc4_commit *c;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	unsigned long flags;
> +	int i, ret;
> +
> +	if (async) {
> +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +
> +			spin_lock_irqsave(&dev->event_lock, flags);
> +
> +			if (crtc->state->event || 
> +				vc4_crtc_has_pending_event(crtc)) {
> +				spin_unlock_irqrestore(&dev->event_lock, flags);
> +				return -EBUSY;
> +			}
> +			spin_unlock_irqrestore(&dev->event_lock, flags);
> +		}
> +	}

By my reading, the point of returning -EBUSY here is just to avoid
blocking when the compositor asked for nonblocking.  You're checking
that each CRTC involved in the commit doesn't have a queued pageflip
event, except that then we immediately proceed to:

	/* Make sure that any outstanding modesets have finished. */
	ret = down_interruptible(&vc4->async_modeset);
	if (ret) {
		kfree(c);
		return ret;
	}

so this per-CRTC check doesn't prevent blocking if the set of CRTCs for
this commit was disjoint from the last one, right?  To implement the
minimal behavior, I think we'd want to just down_trylock instead in the
async case, I think.  And to implement the disjoint CRTC thing you were
trying for, I think we'd need to track a mask kind of like msm's
pending_crtcs.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 3762 bytes --]

robert.foss@collabora.com writes:

> From: Robert Foss <robert.foss@collabora.com>
>
> As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
> update is requested and there is an earlier update pending".

Note: docs cited here are drm_crtc.h, and the whole quote is:

	 *  - -EBUSY, if an asynchronous updated is requested and there is
	 *    an earlier updated pending. Drivers are allowed to support a queue
	 *    of outstanding updates, but currently no driver supports that.
	 *    Note that drivers must wait for preceding updates to complete if a
	 *    synchronous update is requested, they are not allowed to fail the
	 *    commit in that case.

> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
>  drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>  drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
>  3 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 355ee4b..43193a3 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
>  		.of_match_table = vc4_crtc_dt_match,
>  	},
>  };
> +
> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
> +{
> +	assert_spin_locked(&crtc->dev->event_lock);
> +	return to_vc4_crtc(crtc)->event;
> +}
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index fa2ad15..54c1fb5 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
>  int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
>  void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
>  int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
>  
>  /* vc4_debugfs.c */
>  int vc4_debugfs_init(struct drm_minor *minor);
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 4718ae5..dc157a1e 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
>  			     bool async)
>  {
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
> -	int ret;
> -	int i;
>  	uint64_t wait_seqno = 0;
>  	struct vc4_commit *c;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	unsigned long flags;
> +	int i, ret;
> +
> +	if (async) {
> +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +
> +			spin_lock_irqsave(&dev->event_lock, flags);
> +
> +			if (crtc->state->event || 
> +				vc4_crtc_has_pending_event(crtc)) {
> +				spin_unlock_irqrestore(&dev->event_lock, flags);
> +				return -EBUSY;
> +			}
> +			spin_unlock_irqrestore(&dev->event_lock, flags);
> +		}
> +	}

By my reading, the point of returning -EBUSY here is just to avoid
blocking when the compositor asked for nonblocking.  You're checking
that each CRTC involved in the commit doesn't have a queued pageflip
event, except that then we immediately proceed to:

	/* Make sure that any outstanding modesets have finished. */
	ret = down_interruptible(&vc4->async_modeset);
	if (ret) {
		kfree(c);
		return ret;
	}

so this per-CRTC check doesn't prevent blocking if the set of CRTCs for
this commit was disjoint from the last one, right?  To implement the
minimal behavior, I think we'd want to just down_trylock instead in the
async case, I think.  And to implement the disjoint CRTC thing you were
trying for, I think we'd need to track a mask kind of like msm's
pending_crtcs.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[Maarten Lankhorst]

Op 02-05-16 om 21:25 schreef robert.foss@collabora.com:
> From: Robert Foss <robert.foss@collabora.com>
>
> As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
> update is requested and there is an earlier update pending".
>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
>  drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>  drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
>  3 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 355ee4b..43193a3 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
>  		.of_match_table = vc4_crtc_dt_match,
>  	},
>  };
> +
> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
> +{
> +	assert_spin_locked(&crtc->dev->event_lock);
> +	return to_vc4_crtc(crtc)->event;
> +}
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index fa2ad15..54c1fb5 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
>  int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
>  void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
>  int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
>  
>  /* vc4_debugfs.c */
>  int vc4_debugfs_init(struct drm_minor *minor);
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 4718ae5..dc157a1e 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
>  			     bool async)
>  {
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
> -	int ret;
> -	int i;
>  	uint64_t wait_seqno = 0;
>  	struct vc4_commit *c;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	unsigned long flags;
> +	int i, ret;
> +
> +	if (async) {
> +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +
> +			spin_lock_irqsave(&dev->event_lock, flags);
> +
> +			if (crtc->state->event || 
^What's this check for? How can this even happen if you remove the event from the crtc state in atomic_flush?

~Maarten

Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[Maarten Lankhorst]

Op 02-05-16 om 21:25 schreef robert.foss@collabora.com:
> From: Robert Foss <robert.foss@collabora.com>
>
> As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
> update is requested and there is an earlier update pending".
>
> Signed-off-by: Robert Foss <robert.foss@collabora.com>
> ---
>  drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
>  drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>  drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
>  3 files changed, 25 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
> index 355ee4b..43193a3 100644
> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
> @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
>  		.of_match_table = vc4_crtc_dt_match,
>  	},
>  };
> +
> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
> +{
> +	assert_spin_locked(&crtc->dev->event_lock);
> +	return to_vc4_crtc(crtc)->event;
> +}
> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
> index fa2ad15..54c1fb5 100644
> --- a/drivers/gpu/drm/vc4/vc4_drv.h
> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
> @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
>  int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
>  void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
>  int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
>  
>  /* vc4_debugfs.c */
>  int vc4_debugfs_init(struct drm_minor *minor);
> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
> index 4718ae5..dc157a1e 100644
> --- a/drivers/gpu/drm/vc4/vc4_kms.c
> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
> @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
>  			     bool async)
>  {
>  	struct vc4_dev *vc4 = to_vc4_dev(dev);
> -	int ret;
> -	int i;
>  	uint64_t wait_seqno = 0;
>  	struct vc4_commit *c;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_crtc *crtc;
> +	unsigned long flags;
> +	int i, ret;
> +
> +	if (async) {
> +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
> +
> +			spin_lock_irqsave(&dev->event_lock, flags);
> +
> +			if (crtc->state->event || 
^What's this check for? How can this even happen if you remove the event from the crtc state in atomic_flush?

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

Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[Robert Foss]

On 05/02/2016 08:57 PM, Eric Anholt wrote:
> robert.foss@collabora.com writes:
>
>> From: Robert Foss <robert.foss@collabora.com>
>>
>> As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
>> update is requested and there is an earlier update pending".
>
> Note: docs cited here are drm_crtc.h, and the whole quote is:
>
> 	 *  - -EBUSY, if an asynchronous updated is requested and there is
> 	 *    an earlier updated pending. Drivers are allowed to support a queue
> 	 *    of outstanding updates, but currently no driver supports that.
> 	 *    Note that drivers must wait for preceding updates to complete if a
> 	 *    synchronous update is requested, they are not allowed to fail the
> 	 *    commit in that case.
>
>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>> ---
>>   drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
>>   drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>>   drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
>>   3 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
>> index 355ee4b..43193a3 100644
>> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
>> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
>> @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
>>   		.of_match_table = vc4_crtc_dt_match,
>>   	},
>>   };
>> +
>> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
>> +{
>> +	assert_spin_locked(&crtc->dev->event_lock);
>> +	return to_vc4_crtc(crtc)->event;
>> +}
>> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
>> index fa2ad15..54c1fb5 100644
>> --- a/drivers/gpu/drm/vc4/vc4_drv.h
>> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
>> @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
>>   int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
>>   void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
>>   int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
>> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
>>
>>   /* vc4_debugfs.c */
>>   int vc4_debugfs_init(struct drm_minor *minor);
>> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
>> index 4718ae5..dc157a1e 100644
>> --- a/drivers/gpu/drm/vc4/vc4_kms.c
>> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
>> @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
>>   			     bool async)
>>   {
>>   	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> -	int ret;
>> -	int i;
>>   	uint64_t wait_seqno = 0;
>>   	struct vc4_commit *c;
>> +	struct drm_crtc_state *crtc_state;
>> +	struct drm_crtc *crtc;
>> +	unsigned long flags;
>> +	int i, ret;
>> +
>> +	if (async) {
>> +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +
>> +			spin_lock_irqsave(&dev->event_lock, flags);
>> +
>> +			if (crtc->state->event ||
>> +				vc4_crtc_has_pending_event(crtc)) {
>> +				spin_unlock_irqrestore(&dev->event_lock, flags);
>> +				return -EBUSY;
>> +			}
>> +			spin_unlock_irqrestore(&dev->event_lock, flags);
>> +		}
>> +	}
>
> By my reading, the point of returning -EBUSY here is just to avoid
> blocking when the compositor asked for nonblocking.  You're checking
> that each CRTC involved in the commit doesn't have a queued pageflip
> event, except that then we immediately proceed to:
>
> 	/* Make sure that any outstanding modesets have finished. */
> 	ret = down_interruptible(&vc4->async_modeset);
> 	if (ret) {
> 		kfree(c);
> 		return ret;
> 	}
>
> so this per-CRTC check doesn't prevent blocking if the set of CRTCs for
> this commit was disjoint from the last one, right?  To implement the
> minimal behavior, I think we'd want to just down_trylock instead in the
> async case, I think.  And to implement the disjoint CRTC thing you were
> trying for, I think we'd need to track a mask kind of like msm's
> pending_crtcs.
>

So what you're saying is that the set of CRTCs in state
might not contain all CRTCs and that this check might be incomplete for 
that reason?

I'm fairly new to these waters, so don't hesitate to tell me if I seem
to be misunderstanding something or am on a wild goose chase of some sort.

So you're suggesting something like this instead of
the per CRTC check:

- 	/* Make sure that any outstanding modesets have finished. */
- 	ret = down_interruptible(&vc4->async_modeset);
- 	if (ret) {
- 		kfree(c);
- 		return ret;
- 	}
+ 	/* Make sure that any outstanding modesets have finished. */
+ 	ret = down_trylock(&vc4->async_modeset);
+ 	if (ret) {
+ 		kfree(c);
+ 		return -EBUSY;
+ 	}

I've quickly tested the above patch and it does seem to work and fulfill 
all of my requirements.

Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[Robert Foss]

On 05/02/2016 08:57 PM, Eric Anholt wrote:
> robert.foss@collabora.com writes:
>
>> From: Robert Foss <robert.foss@collabora.com>
>>
>> As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
>> update is requested and there is an earlier update pending".
>
> Note: docs cited here are drm_crtc.h, and the whole quote is:
>
> 	 *  - -EBUSY, if an asynchronous updated is requested and there is
> 	 *    an earlier updated pending. Drivers are allowed to support a queue
> 	 *    of outstanding updates, but currently no driver supports that.
> 	 *    Note that drivers must wait for preceding updates to complete if a
> 	 *    synchronous update is requested, they are not allowed to fail the
> 	 *    commit in that case.
>
>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>> ---
>>   drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
>>   drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>>   drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
>>   3 files changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
>> index 355ee4b..43193a3 100644
>> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
>> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
>> @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
>>   		.of_match_table = vc4_crtc_dt_match,
>>   	},
>>   };
>> +
>> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
>> +{
>> +	assert_spin_locked(&crtc->dev->event_lock);
>> +	return to_vc4_crtc(crtc)->event;
>> +}
>> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
>> index fa2ad15..54c1fb5 100644
>> --- a/drivers/gpu/drm/vc4/vc4_drv.h
>> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
>> @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
>>   int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
>>   void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
>>   int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
>> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
>>
>>   /* vc4_debugfs.c */
>>   int vc4_debugfs_init(struct drm_minor *minor);
>> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
>> index 4718ae5..dc157a1e 100644
>> --- a/drivers/gpu/drm/vc4/vc4_kms.c
>> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
>> @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
>>   			     bool async)
>>   {
>>   	struct vc4_dev *vc4 = to_vc4_dev(dev);
>> -	int ret;
>> -	int i;
>>   	uint64_t wait_seqno = 0;
>>   	struct vc4_commit *c;
>> +	struct drm_crtc_state *crtc_state;
>> +	struct drm_crtc *crtc;
>> +	unsigned long flags;
>> +	int i, ret;
>> +
>> +	if (async) {
>> +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> +
>> +			spin_lock_irqsave(&dev->event_lock, flags);
>> +
>> +			if (crtc->state->event ||
>> +				vc4_crtc_has_pending_event(crtc)) {
>> +				spin_unlock_irqrestore(&dev->event_lock, flags);
>> +				return -EBUSY;
>> +			}
>> +			spin_unlock_irqrestore(&dev->event_lock, flags);
>> +		}
>> +	}
>
> By my reading, the point of returning -EBUSY here is just to avoid
> blocking when the compositor asked for nonblocking.  You're checking
> that each CRTC involved in the commit doesn't have a queued pageflip
> event, except that then we immediately proceed to:
>
> 	/* Make sure that any outstanding modesets have finished. */
> 	ret = down_interruptible(&vc4->async_modeset);
> 	if (ret) {
> 		kfree(c);
> 		return ret;
> 	}
>
> so this per-CRTC check doesn't prevent blocking if the set of CRTCs for
> this commit was disjoint from the last one, right?  To implement the
> minimal behavior, I think we'd want to just down_trylock instead in the
> async case, I think.  And to implement the disjoint CRTC thing you were
> trying for, I think we'd need to track a mask kind of like msm's
> pending_crtcs.
>

So what you're saying is that the set of CRTCs in state
might not contain all CRTCs and that this check might be incomplete for 
that reason?

I'm fairly new to these waters, so don't hesitate to tell me if I seem
to be misunderstanding something or am on a wild goose chase of some sort.

So you're suggesting something like this instead of
the per CRTC check:

- 	/* Make sure that any outstanding modesets have finished. */
- 	ret = down_interruptible(&vc4->async_modeset);
- 	if (ret) {
- 		kfree(c);
- 		return ret;
- 	}
+ 	/* Make sure that any outstanding modesets have finished. */
+ 	ret = down_trylock(&vc4->async_modeset);
+ 	if (ret) {
+ 		kfree(c);
+ 		return -EBUSY;
+ 	}

I've quickly tested the above patch and it does seem to work and fulfill 
all of my requirements.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[Eric Anholt]

[-- Attachment #1: Type: text/plain, Size: 5062 bytes --]

Robert Foss <robert.foss@collabora.com> writes:

> On 05/02/2016 08:57 PM, Eric Anholt wrote:
>> robert.foss@collabora.com writes:
>>
>>> From: Robert Foss <robert.foss@collabora.com>
>>>
>>> As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
>>> update is requested and there is an earlier update pending".
>>
>> Note: docs cited here are drm_crtc.h, and the whole quote is:
>>
>> 	 *  - -EBUSY, if an asynchronous updated is requested and there is
>> 	 *    an earlier updated pending. Drivers are allowed to support a queue
>> 	 *    of outstanding updates, but currently no driver supports that.
>> 	 *    Note that drivers must wait for preceding updates to complete if a
>> 	 *    synchronous update is requested, they are not allowed to fail the
>> 	 *    commit in that case.
>>
>>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>>> ---
>>>   drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
>>>   drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>>>   drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
>>>   3 files changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
>>> index 355ee4b..43193a3 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
>>> @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
>>>   		.of_match_table = vc4_crtc_dt_match,
>>>   	},
>>>   };
>>> +
>>> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
>>> +{
>>> +	assert_spin_locked(&crtc->dev->event_lock);
>>> +	return to_vc4_crtc(crtc)->event;
>>> +}
>>> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
>>> index fa2ad15..54c1fb5 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_drv.h
>>> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
>>> @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
>>>   int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
>>>   void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
>>>   int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
>>> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
>>>
>>>   /* vc4_debugfs.c */
>>>   int vc4_debugfs_init(struct drm_minor *minor);
>>> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
>>> index 4718ae5..dc157a1e 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_kms.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
>>> @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
>>>   			     bool async)
>>>   {
>>>   	struct vc4_dev *vc4 = to_vc4_dev(dev);
>>> -	int ret;
>>> -	int i;
>>>   	uint64_t wait_seqno = 0;
>>>   	struct vc4_commit *c;
>>> +	struct drm_crtc_state *crtc_state;
>>> +	struct drm_crtc *crtc;
>>> +	unsigned long flags;
>>> +	int i, ret;
>>> +
>>> +	if (async) {
>>> +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>> +
>>> +			spin_lock_irqsave(&dev->event_lock, flags);
>>> +
>>> +			if (crtc->state->event ||
>>> +				vc4_crtc_has_pending_event(crtc)) {
>>> +				spin_unlock_irqrestore(&dev->event_lock, flags);
>>> +				return -EBUSY;
>>> +			}
>>> +			spin_unlock_irqrestore(&dev->event_lock, flags);
>>> +		}
>>> +	}
>>
>> By my reading, the point of returning -EBUSY here is just to avoid
>> blocking when the compositor asked for nonblocking.  You're checking
>> that each CRTC involved in the commit doesn't have a queued pageflip
>> event, except that then we immediately proceed to:
>>
>> 	/* Make sure that any outstanding modesets have finished. */
>> 	ret = down_interruptible(&vc4->async_modeset);
>> 	if (ret) {
>> 		kfree(c);
>> 		return ret;
>> 	}
>>
>> so this per-CRTC check doesn't prevent blocking if the set of CRTCs for
>> this commit was disjoint from the last one, right?  To implement the
>> minimal behavior, I think we'd want to just down_trylock instead in the
>> async case, I think.  And to implement the disjoint CRTC thing you were
>> trying for, I think we'd need to track a mask kind of like msm's
>> pending_crtcs.
>>
>
> So what you're saying is that the set of CRTCs in state
> might not contain all CRTCs and that this check might be incomplete for 
> that reason?
>
> I'm fairly new to these waters, so don't hesitate to tell me if I seem
> to be misunderstanding something or am on a wild goose chase of some sort.
>
> So you're suggesting something like this instead of
> the per CRTC check:
>
> - 	/* Make sure that any outstanding modesets have finished. */
> - 	ret = down_interruptible(&vc4->async_modeset);
> - 	if (ret) {
> - 		kfree(c);
> - 		return ret;
> - 	}
> + 	/* Make sure that any outstanding modesets have finished. */
> + 	ret = down_trylock(&vc4->async_modeset);
> + 	if (ret) {
> + 		kfree(c);
> + 		return -EBUSY;
> + 	}
>
> I've quickly tested the above patch and it does seem to work and fulfill 
> all of my requirements.

Note that you'd want to trylock only in the async case.  In the sync
case, you'd still want to block.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[Eric Anholt]

[-- Attachment #1.1: Type: text/plain, Size: 5062 bytes --]

Robert Foss <robert.foss@collabora.com> writes:

> On 05/02/2016 08:57 PM, Eric Anholt wrote:
>> robert.foss@collabora.com writes:
>>
>>> From: Robert Foss <robert.foss@collabora.com>
>>>
>>> As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
>>> update is requested and there is an earlier update pending".
>>
>> Note: docs cited here are drm_crtc.h, and the whole quote is:
>>
>> 	 *  - -EBUSY, if an asynchronous updated is requested and there is
>> 	 *    an earlier updated pending. Drivers are allowed to support a queue
>> 	 *    of outstanding updates, but currently no driver supports that.
>> 	 *    Note that drivers must wait for preceding updates to complete if a
>> 	 *    synchronous update is requested, they are not allowed to fail the
>> 	 *    commit in that case.
>>
>>> Signed-off-by: Robert Foss <robert.foss@collabora.com>
>>> ---
>>>   drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
>>>   drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
>>>   drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
>>>   3 files changed, 25 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
>>> index 355ee4b..43193a3 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_crtc.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c
>>> @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
>>>   		.of_match_table = vc4_crtc_dt_match,
>>>   	},
>>>   };
>>> +
>>> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
>>> +{
>>> +	assert_spin_locked(&crtc->dev->event_lock);
>>> +	return to_vc4_crtc(crtc)->event;
>>> +}
>>> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
>>> index fa2ad15..54c1fb5 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_drv.h
>>> +++ b/drivers/gpu/drm/vc4/vc4_drv.h
>>> @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
>>>   int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
>>>   void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
>>>   int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
>>> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
>>>
>>>   /* vc4_debugfs.c */
>>>   int vc4_debugfs_init(struct drm_minor *minor);
>>> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
>>> index 4718ae5..dc157a1e 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_kms.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_kms.c
>>> @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
>>>   			     bool async)
>>>   {
>>>   	struct vc4_dev *vc4 = to_vc4_dev(dev);
>>> -	int ret;
>>> -	int i;
>>>   	uint64_t wait_seqno = 0;
>>>   	struct vc4_commit *c;
>>> +	struct drm_crtc_state *crtc_state;
>>> +	struct drm_crtc *crtc;
>>> +	unsigned long flags;
>>> +	int i, ret;
>>> +
>>> +	if (async) {
>>> +		for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>> +
>>> +			spin_lock_irqsave(&dev->event_lock, flags);
>>> +
>>> +			if (crtc->state->event ||
>>> +				vc4_crtc_has_pending_event(crtc)) {
>>> +				spin_unlock_irqrestore(&dev->event_lock, flags);
>>> +				return -EBUSY;
>>> +			}
>>> +			spin_unlock_irqrestore(&dev->event_lock, flags);
>>> +		}
>>> +	}
>>
>> By my reading, the point of returning -EBUSY here is just to avoid
>> blocking when the compositor asked for nonblocking.  You're checking
>> that each CRTC involved in the commit doesn't have a queued pageflip
>> event, except that then we immediately proceed to:
>>
>> 	/* Make sure that any outstanding modesets have finished. */
>> 	ret = down_interruptible(&vc4->async_modeset);
>> 	if (ret) {
>> 		kfree(c);
>> 		return ret;
>> 	}
>>
>> so this per-CRTC check doesn't prevent blocking if the set of CRTCs for
>> this commit was disjoint from the last one, right?  To implement the
>> minimal behavior, I think we'd want to just down_trylock instead in the
>> async case, I think.  And to implement the disjoint CRTC thing you were
>> trying for, I think we'd need to track a mask kind of like msm's
>> pending_crtcs.
>>
>
> So what you're saying is that the set of CRTCs in state
> might not contain all CRTCs and that this check might be incomplete for 
> that reason?
>
> I'm fairly new to these waters, so don't hesitate to tell me if I seem
> to be misunderstanding something or am on a wild goose chase of some sort.
>
> So you're suggesting something like this instead of
> the per CRTC check:
>
> - 	/* Make sure that any outstanding modesets have finished. */
> - 	ret = down_interruptible(&vc4->async_modeset);
> - 	if (ret) {
> - 		kfree(c);
> - 		return ret;
> - 	}
> + 	/* Make sure that any outstanding modesets have finished. */
> + 	ret = down_trylock(&vc4->async_modeset);
> + 	if (ret) {
> + 		kfree(c);
> + 		return -EBUSY;
> + 	}
>
> I've quickly tested the above patch and it does seem to work and fulfill 
> all of my requirements.

Note that you'd want to trylock only in the async case.  In the sync
case, you'd still want to block.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 818 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

[PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
update is requested and there is an earlier update pending".


This patch is based on the rockchip patch below:
    http://article.gmane.org/gmane.comp.video.dri.devel/151678

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
 drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 355ee4b..43193a3 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
 		.of_match_table = vc4_crtc_dt_match,
 	},
 };
+
+bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
+{
+	assert_spin_locked(&crtc->dev->event_lock);
+	return to_vc4_crtc(crtc)->event;
+}
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index fa2ad15..54c1fb5 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
 int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
 void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
 int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
+bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
 
 /* vc4_debugfs.c */
 int vc4_debugfs_init(struct drm_minor *minor);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 4718ae5..dc157a1e 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
 			     bool async)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
-	int ret;
-	int i;
 	uint64_t wait_seqno = 0;
 	struct vc4_commit *c;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	unsigned long flags;
+	int i, ret;
+
+	if (async) {
+		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+
+			spin_lock_irqsave(&dev->event_lock, flags);
+
+			if (crtc->state->event || 
+				vc4_crtc_has_pending_event(crtc)) {
+				spin_unlock_irqrestore(&dev->event_lock, flags);
+				return -EBUSY;
+			}
+			spin_unlock_irqrestore(&dev->event_lock, flags);
+		}
+	}
 
 	c = commit_init(state);
 	if (!c)
-- 
2.5.0

[PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event.

[robert.foss]

From: Robert Foss <robert.foss@collabora.com>

As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
update is requested and there is an earlier update pending".

This patch is based on the rockchip patch below:
    http://article.gmane.org/gmane.comp.video.dri.devel/151678

Signed-off-by: Robert Foss <robert.foss@collabora.com>
---
 drivers/gpu/drm/vc4/vc4_crtc.c |  6 ++++++
 drivers/gpu/drm/vc4/vc4_drv.h  |  1 +
 drivers/gpu/drm/vc4/vc4_kms.c  | 20 ++++++++++++++++++--
 3 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c
index 355ee4b..43193a3 100644
--- a/drivers/gpu/drm/vc4/vc4_crtc.c
+++ b/drivers/gpu/drm/vc4/vc4_crtc.c
@@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = {
 		.of_match_table = vc4_crtc_dt_match,
 	},
 };
+
+bool vc4_crtc_has_pending_event(struct drm_crtc *crtc)
+{
+	assert_spin_locked(&crtc->dev->event_lock);
+	return to_vc4_crtc(crtc)->event;
+}
diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h
index fa2ad15..54c1fb5 100644
--- a/drivers/gpu/drm/vc4/vc4_drv.h
+++ b/drivers/gpu/drm/vc4/vc4_drv.h
@@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver;
 int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id);
 void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id);
 int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg);
+bool vc4_crtc_has_pending_event(struct drm_crtc *crtc);
 
 /* vc4_debugfs.c */
 int vc4_debugfs_init(struct drm_minor *minor);
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c
index 4718ae5..dc157a1e 100644
--- a/drivers/gpu/drm/vc4/vc4_kms.c
+++ b/drivers/gpu/drm/vc4/vc4_kms.c
@@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev,
 			     bool async)
 {
 	struct vc4_dev *vc4 = to_vc4_dev(dev);
-	int ret;
-	int i;
 	uint64_t wait_seqno = 0;
 	struct vc4_commit *c;
+	struct drm_crtc_state *crtc_state;
+	struct drm_crtc *crtc;
+	unsigned long flags;
+	int i, ret;
+
+	if (async) {
+		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+
+			spin_lock_irqsave(&dev->event_lock, flags);
+
+			if (crtc->state->event || 
+				vc4_crtc_has_pending_event(crtc)) {
+				spin_unlock_irqrestore(&dev->event_lock, flags);
+				return -EBUSY;
+			}
+			spin_unlock_irqrestore(&dev->event_lock, flags);
+		}
+	}
 
 	c = commit_init(state);
 	if (!c)
-- 
2.5.0

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