drm/vc4: Return -EBUSY if there's already a pending flip event.
Subsystems (3)
PATCH 1/1 drm/vc4: Return -EBUSY if there's already a pending flip event.
From: Robert Foss <[email protected]>
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(-)
Signature · 6 lines
2.5.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH 1/1 drm/vc4: Return -EBUSY if there's already a pending flip event.
From: Robert Foss <[email protected]>
As per the docs, atomic_commit should return -EBUSY "if an asycnhronous update is requested and there is an earlier update pending".
--- 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(-)
drivers/gpu/drm/vc4/vc4_crtc.c +6 -0
@@ -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; +}
drivers/gpu/drm/vc4/vc4_drv.h +1 -0
@@ -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);
drivers/gpu/drm/vc4/vc4_kms.c +18 -2
@@ -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)
Signature · 6 lines
2.5.0 _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 4 lines · [email protected] writes:
From: Robert Foss <[email protected]>
As per the docs,
atomic_commitshould 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.
Quoted 53 lines
Signed-off-by Robert Foss <[email protected]>--- 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(-)drivers/gpu/drm/vc4/vc4_crtc.c +6
-0@@ -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; +}drivers/gpu/drm/vc4/vc4_drv.h +1
-0@@ -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);drivers/gpu/drm/vc4/vc4_kms.c +8
-2@@ -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) { +
drivers/gpu/drm/vc4/vc4_kms.c static int vc4_atomic_commit(struct drm_device *dev, · line 129
Quoted 10 lines
drivers/gpu/drm/vc4/vc4_kms.c +10
-0+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.
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 92 lines · Eric Anholt
Quoted 4 lines · [email protected] writes:
From: Robert Foss <[email protected]>
As per the docs,
atomic_commitshould 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.Quoted 53 lines
Signed-off-by Robert Foss <[email protected]>--- 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(-)drivers/gpu/drm/vc4/vc4_crtc.c +6
-0@@ -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; +}drivers/gpu/drm/vc4/vc4_drv.h +1
-0@@ -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);drivers/gpu/drm/vc4/vc4_kms.c +8
-2@@ -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) { +drivers/gpu/drm/vc4/vc4_kms.c static int vc4_atomic_commit(struct drm_device *dev, · line 129
Quoted 10 lines
drivers/gpu/drm/vc4/vc4_kms.c +10
-0+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
-EBUSYhere 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; }
Quoted 7 lines
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_trylockinstead 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'spending_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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Quoted 124 lines · Robert Foss
Quoted 92 lines · Eric Anholt
Quoted 4 lines · [email protected] writes:
From: Robert Foss <[email protected]>
As per the docs,
atomic_commitshould 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.Quoted 53 lines
Signed-off-by Robert Foss <[email protected]>--- 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(-)drivers/gpu/drm/vc4/vc4_crtc.c +6
-0@@ -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; +}drivers/gpu/drm/vc4/vc4_drv.h +1
-0@@ -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);drivers/gpu/drm/vc4/vc4_kms.c +8
-2@@ -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) { +drivers/gpu/drm/vc4/vc4_kms.c static int vc4_atomic_commit(struct drm_device *dev, · line 129
Quoted 10 lines
drivers/gpu/drm/vc4/vc4_kms.c +10
-0+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
-EBUSYhere 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; }Quoted 7 lines
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_trylockinstead 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'spending_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:
(quoted diff) +6
-6- /* 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; + }
Quoted 2 lines
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.
_______________________________________________
dri-devel mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Maarten Lankhorst
maintainerOp 02-05-16 om 21:25 schreef [email protected]:
Quoted 51 lines
From: Robert Foss <[email protected]>
As per the docs,
atomic_commitshould return-EBUSY"if an asycnhronous update is requested and there is an earlier update pending".Signed-off-by Robert Foss <[email protected]>--- 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(-)drivers/gpu/drm/vc4/vc4_crtc.c +6
-0@@ -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; +}drivers/gpu/drm/vc4/vc4_drv.h +1
-0@@ -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);drivers/gpu/drm/vc4/vc4_kms.c +1
-2@@ -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;
drivers/gpu/drm/vc4/vc4_kms.c static int vc4_atomic_commit(struct drm_device *dev, · line 122
Quoted 10 lines
drivers/gpu/drm/vc4/vc4_kms.c +10
-0+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
[email protected]
https://lists.freedesktop.org/mailman/listinfo/dri-devel