* [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v3
@ 2016-04-04 13:55 Tomeu Vizoso
2016-04-04 15:44 ` Daniel Stone
0 siblings, 1 reply; 4+ messages in thread
From: Tomeu Vizoso @ 2016-04-04 13:55 UTC (permalink / raw)
To: linux-kernel
Cc: Tomeu Vizoso, Mark Yao, David Airlie, Heiko Stuebner, dri-devel,
linux-arm-kernel, linux-rockchip
As per the docs, atomic_commit should return -EBUSY "if an asycnhronous
updated is requested and there is an earlier updated pending".
v2: Use the status of the workqueue instead of vop->event, and don't add
a superfluous wait on the workqueue.
v3: Drop work_busy, as there's a sizeable delay when the worker
finishes, which introduces a race in which the client has already
received the last flip event but the next page flip ioctl will still
return -EBUSY because work_busy returns outdated information.
Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
---
drivers/gpu/drm/rockchip/rockchip_drm_drv.h | 1 +
drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 13 ++++++++++++-
drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 5 +++++
3 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 3529f692edb8..37952eefd33d 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -69,6 +69,7 @@ int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc);
int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc, int connector_type,
int out_mode);
+bool rockchip_drm_crtc_has_pending_event(struct drm_crtc *crtc);
int rockchip_drm_dma_attach_device(struct drm_device *drm_dev,
struct device *dev);
void rockchip_drm_dma_detach_device(struct drm_device *drm_dev,
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
index 3b8f652698f8..8305bbd2a4d7 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -280,7 +280,18 @@ int rockchip_drm_atomic_commit(struct drm_device *dev,
{
struct rockchip_drm_private *private = dev->dev_private;
struct rockchip_atomic_commit *commit = &private->commit;
- int ret;
+ struct drm_crtc_state *crtc_state;
+ struct drm_crtc *crtc;
+ int i, ret;
+
+ if (async) {
+ for_each_crtc_in_state(state, crtc, crtc_state, i) {
+ if (crtc->state->event ||
+ rockchip_drm_crtc_has_pending_event(crtc)) {
+ return -EBUSY;
+ }
+ }
+ }
ret = drm_atomic_helper_prepare_planes(dev, state);
if (ret)
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index e2118e62345b..5bcdd8dc7499 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -848,6 +848,11 @@ int rockchip_drm_crtc_mode_config(struct drm_crtc *crtc,
}
EXPORT_SYMBOL_GPL(rockchip_drm_crtc_mode_config);
+bool rockchip_drm_crtc_has_pending_event(struct drm_crtc *crtc)
+{
+ return to_vop(crtc)->event;
+}
+
static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
{
struct vop *vop = to_vop(crtc);
--
2.5.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v3
2016-04-04 13:55 [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v3 Tomeu Vizoso
@ 2016-04-04 15:44 ` Daniel Stone
2016-04-05 14:07 ` Tomeu Vizoso
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Stone @ 2016-04-04 15:44 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: Linux Kernel Mailing List, dri-devel, linux-rockchip, linux-arm-kernel
Hi Tomeu,
On 4 April 2016 at 14:55, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> index 3b8f652698f8..8305bbd2a4d7 100644
> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
> @@ -280,7 +280,18 @@ int rockchip_drm_atomic_commit(struct drm_device *dev,
> {
> struct rockchip_drm_private *private = dev->dev_private;
> struct rockchip_atomic_commit *commit = &private->commit;
> - int ret;
> + struct drm_crtc_state *crtc_state;
> + struct drm_crtc *crtc;
> + int i, ret;
> +
> + if (async) {
> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
> + if (crtc->state->event ||
> + rockchip_drm_crtc_has_pending_event(crtc)) {
> + return -EBUSY;
> + }
> + }
> + }
Hmmm ...
Doesn't this trigger before the VOP atomic_begin() helper, meaning
that anything with an event set will trigger the check? Seems like it
should be && rather than ||.
Also, I know rockchip_drm_vop.c isn't holding dev->event_lock when it
checks vop->event, but it really should be ... and so should this
check. Otherwise you can race with the IRQ handler, which isn't
required to hold the CRTC lock.
Cheers,
Daniel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v3
2016-04-04 15:44 ` Daniel Stone
@ 2016-04-05 14:07 ` Tomeu Vizoso
2016-05-24 7:19 ` Daniel Stone
0 siblings, 1 reply; 4+ messages in thread
From: Tomeu Vizoso @ 2016-04-05 14:07 UTC (permalink / raw)
To: Daniel Stone
Cc: linux-arm-kernel, linux-rockchip, Linux Kernel Mailing List, dri-devel
On 4 April 2016 at 17:44, Daniel Stone <daniel@fooishbar.org> wrote:
> Hi Tomeu,
>
> On 4 April 2016 at 14:55, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>> diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> index 3b8f652698f8..8305bbd2a4d7 100644
>> --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
>> @@ -280,7 +280,18 @@ int rockchip_drm_atomic_commit(struct drm_device *dev,
>> {
>> struct rockchip_drm_private *private = dev->dev_private;
>> struct rockchip_atomic_commit *commit = &private->commit;
>> - int ret;
>> + struct drm_crtc_state *crtc_state;
>> + struct drm_crtc *crtc;
>> + int i, ret;
>> +
>> + if (async) {
>> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
>> + if (crtc->state->event ||
>> + rockchip_drm_crtc_has_pending_event(crtc)) {
>> + return -EBUSY;
>> + }
>> + }
>> + }
>
> Hmmm ...
>
> Doesn't this trigger before the VOP atomic_begin() helper, meaning
> that anything with an event set will trigger the check? Seems like it
> should be && rather than ||.
So, these are the two cases that this code aims to handle:
1. A previous request with an event set hasn't progressed to
atomic_begin yet, so the event field in drm_crtc_state (at this point,
the old state) is still populated but vop->event still isn't.
2. A previous request has already progressed to atomic_begin but we
haven't gotten yet the interrupt that signals its completion, so
vop->event is populated but crtc->state->event isn't anymore.
My understanding is that in both cases there's a pending flip event
and we have to return -EBUSY if it's as async request.
> Also, I know rockchip_drm_vop.c isn't holding dev->event_lock when it
> checks vop->event, but it really should be ... and so should this
> check. Otherwise you can race with the IRQ handler, which isn't
> required to hold the CRTC lock.
Cool, thanks.
Tomeu
> Cheers,
> Daniel
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v3
2016-04-05 14:07 ` Tomeu Vizoso
@ 2016-05-24 7:19 ` Daniel Stone
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Stone @ 2016-05-24 7:19 UTC (permalink / raw)
To: Tomeu Vizoso
Cc: linux-arm-kernel, linux-rockchip, Linux Kernel Mailing List, dri-devel
Hi Tomeu,
On 5 April 2016 at 16:07, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> On 4 April 2016 at 17:44, Daniel Stone <daniel@fooishbar.org> wrote:
>> On 4 April 2016 at 14:55, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
>>> + if (async) {
>>> + for_each_crtc_in_state(state, crtc, crtc_state, i) {
>>> + if (crtc->state->event ||
>>> + rockchip_drm_crtc_has_pending_event(crtc)) {
>>> + return -EBUSY;
>>> + }
>>> + }
>>> + }
>>
>> Hmmm ...
>>
>> Doesn't this trigger before the VOP atomic_begin() helper, meaning
>> that anything with an event set will trigger the check? Seems like it
>> should be && rather than ||.
>
> So, these are the two cases that this code aims to handle:
>
> 1. A previous request with an event set hasn't progressed to
> atomic_begin yet, so the event field in drm_crtc_state (at this point,
> the old state) is still populated but vop->event still isn't.
Ah right, this was what I was missing: the async (non-blocking)
implementation. Sounds good to me then.
Cheers,
Daniel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-05-24 7:19 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-04-04 13:55 [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v3 Tomeu Vizoso
2016-04-04 15:44 ` Daniel Stone
2016-04-05 14:07 ` Tomeu Vizoso
2016-05-24 7:19 ` Daniel Stone
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).