Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1405910 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v5 Tomeu Vizoso <tomeu.vizoso@collabora.com> - 2016-05-24 09:30 +0200
    Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending  flip event v5 Daniel Stone <daniel@fooishbar.org> - 2016-05-24 09:40 +0200
    Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v5 Heiko Stuebner <heiko@sntech.de> - 2016-05-24 10:30 +0200
      Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending  flip event v5 Daniel Vetter <daniel@ffwll.ch> - 2016-05-24 10:40 +0200
        Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending  flip event v5 Daniel Vetter <daniel@ffwll.ch> - 2016-05-24 10:40 +0200
          Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v5 Heiko Stuebner <heiko@sntech.de> - 2016-05-24 10:50 +0200
            Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending  flip event v5 Daniel Vetter <daniel@ffwll.ch> - 2016-05-24 11:10 +0200

#1405910 — [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v5

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.

v4: Hold dev->event_lock while checking the VOP's event field as
suggested by Daniel Stone.

v5: Only block if there's outstanding work if it's a blocking call.

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  | 25 ++++++++++++++++++++++---
 drivers/gpu/drm/rockchip/rockchip_drm_vop.c |  6 ++++++
 3 files changed, 29 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
index 56f43a364c7f..0b46617decd9 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_drv.h
@@ -76,6 +76,7 @@ void rockchip_drm_atomic_work(struct work_struct *work);
 int rockchip_register_crtc_funcs(struct drm_crtc *crtc,
 				 const struct rockchip_crtc_funcs *crtc_funcs);
 void rockchip_unregister_crtc_funcs(struct drm_crtc *crtc);
+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 755cfdba61cd..e9531353b8d2 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c
@@ -279,15 +279,34 @@ 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;
+	unsigned long flags;
+	int i, ret;
+
+	if (nonblock) {
+		for_each_crtc_in_state(state, crtc, crtc_state, i) {
+			spin_lock_irqsave(&dev->event_lock, flags);
+
+			if (crtc->state->event ||
+			    rockchip_drm_crtc_has_pending_event(crtc)) {
+				spin_unlock_irqrestore(&dev->event_lock, flags);
+				return -EBUSY;
+			}
+
+			spin_unlock_irqrestore(&dev->event_lock, flags);
+		}
+	}
 
 	ret = drm_atomic_helper_prepare_planes(dev, state);
 	if (ret)
 		return ret;
 
-	/* serialize outstanding nonblocking commits */
 	mutex_lock(&commit->lock);
-	flush_work(&commit->work);
+
+	/* serialize outstanding nonblocking commits */
+	if (!nonblock)
+		flush_work(&commit->work);
 
 	drm_atomic_helper_swap_state(dev, state);
 
diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
index 1c4d5b5a70a2..3f980f52c640 100644
--- a/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
+++ b/drivers/gpu/drm/rockchip/rockchip_drm_vop.c
@@ -836,6 +836,12 @@ static const struct drm_plane_funcs vop_plane_funcs = {
 	.atomic_destroy_state = vop_atomic_plane_destroy_state,
 };
 
+bool rockchip_drm_crtc_has_pending_event(struct drm_crtc *crtc)
+{
+	assert_spin_locked(&crtc->dev->event_lock);
+	return to_vop(crtc)->event;
+}
+
 static int vop_crtc_enable_vblank(struct drm_crtc *crtc)
 {
 	struct vop *vop = to_vop(crtc);
-- 
2.5.5

[toc] | [next] | [standalone]

#1405913 — Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v5

On 24 May 2016 at 09:27, Tomeu Vizoso <tomeu.vizoso@collabora.com> wrote:
> 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.
>
> v4: Hold dev->event_lock while checking the VOP's event field as
> suggested by Daniel Stone.
>
> v5: Only block if there's outstanding work if it's a blocking call.
>
> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>

Reviewed-by: Daniel Stone <daniels@collabora.com>

Cheers,
Daniel

[toc] | [prev] | [next] | [standalone]

#1405934

Hi Tomeu,

Patch subject: please put the version into the brackets, so [PATCH v5] as it 
shouldn't be part of the commit log.

Am Dienstag, 24. Mai 2016, 09:27:37 schrieb Tomeu Vizoso:
> 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.
> 
> v4: Hold dev->event_lock while checking the VOP's event field as
> suggested by Daniel Stone.
> 
> v5: Only block if there's outstanding work if it's a blocking call.

similarly, please put the changelog below the "---" and above the diffstat.


> Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> ---

aka here.

>  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |  1 +
>  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  | 25
> ++++++++++++++++++++++--- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 
> 6 ++++++
>  3 files changed, 29 insertions(+), 3 deletions(-)

Heiko

[toc] | [prev] | [next] | [standalone]

#1405940 — Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v5

On Tue, May 24, 2016 at 10:28:42AM +0200, Heiko Stuebner wrote:
> Hi Tomeu,
> 
> Patch subject: please put the version into the brackets, so [PATCH v5] as it 
> shouldn't be part of the commit log.
> 
> Am Dienstag, 24. Mai 2016, 09:27:37 schrieb Tomeu Vizoso:
> > 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.
> > 
> > v4: Hold dev->event_lock while checking the VOP's event field as
> > suggested by Daniel Stone.
> > 
> > v5: Only block if there's outstanding work if it's a blocking call.
> 
> similarly, please put the changelog below the "---" and above the diffstat.

drm culture is to keep it above, since it's kinda useful sometimes when
later on trying to reconstruct wtf was discussed and why a patch was
merged.
-Daniel

> 
> 
> > Signed-off-by: Tomeu Vizoso <tomeu.vizoso@collabora.com>
> > ---
> 
> aka here.
> 
> >  drivers/gpu/drm/rockchip/rockchip_drm_drv.h |  1 +
> >  drivers/gpu/drm/rockchip/rockchip_drm_fb.c  | 25
> > ++++++++++++++++++++++--- drivers/gpu/drm/rockchip/rockchip_drm_vop.c | 
> > 6 ++++++
> >  3 files changed, 29 insertions(+), 3 deletions(-)
> 
> Heiko
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1405943 — Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v5

On Tue, May 24, 2016 at 10:30:50AM +0200, Daniel Vetter wrote:
> On Tue, May 24, 2016 at 10:28:42AM +0200, Heiko Stuebner wrote:
> > Hi Tomeu,
> > 
> > Patch subject: please put the version into the brackets, so [PATCH v5] as it 
> > shouldn't be part of the commit log.
> > 
> > Am Dienstag, 24. Mai 2016, 09:27:37 schrieb Tomeu Vizoso:
> > > 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.
> > > 
> > > v4: Hold dev->event_lock while checking the VOP's event field as
> > > suggested by Daniel Stone.
> > > 
> > > v5: Only block if there's outstanding work if it's a blocking call.
> > 
> > similarly, please put the changelog below the "---" and above the diffstat.
> 
> drm culture is to keep it above, since it's kinda useful sometimes when
> later on trying to reconstruct wtf was discussed and why a patch was
> merged.

Maybe needs a bit more context: The only stuff you raised in your review
is tiny style nits of pretty much utter irrelevance. No substantial and
material feedback anywehere, and in my opinion in such a case either fix
up the nits when applying (when you feel really strongly about perfect
patches), or just merge as-is.

But sending out content-less bikesheds like these just adds noise and
helps no-one. I think at least some spelling stuff is the minimal bar (but
then just include your r-b tag), but personally I don't even care about
that so much, as long as it's still legible.

Thanks, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1405946

Am Dienstag, 24. Mai 2016, 10:37:49 schrieb Daniel Vetter:
> On Tue, May 24, 2016 at 10:30:50AM +0200, Daniel Vetter wrote:
> > On Tue, May 24, 2016 at 10:28:42AM +0200, Heiko Stuebner wrote:
> > > Hi Tomeu,
> > > 
> > > Patch subject: please put the version into the brackets, so [PATCH v5]
> > > as it shouldn't be part of the commit log.
> > > 
> > > Am Dienstag, 24. Mai 2016, 09:27:37 schrieb Tomeu Vizoso:
> > > > 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.
> > > > 
> > > > v4: Hold dev->event_lock while checking the VOP's event field as
> > > > suggested by Daniel Stone.
> > > > 
> > > > v5: Only block if there's outstanding work if it's a blocking call.
> > > 
> > > similarly, please put the changelog below the "---" and above the
> > > diffstat.> 
> > drm culture is to keep it above, since it's kinda useful sometimes when
> > later on trying to reconstruct wtf was discussed and why a patch was
> > merged.
> 
> Maybe needs a bit more context: The only stuff you raised in your review
> is tiny style nits of pretty much utter irrelevance. No substantial and
> material feedback anywehere, and in my opinion in such a case either fix
> up the nits when applying (when you feel really strongly about perfect
> patches), or just merge as-is.
> 
> But sending out content-less bikesheds like these just adds noise and
> helps no-one. I think at least some spelling stuff is the minimal bar (but
> then just include your r-b tag), but personally I don't even care about
> that so much, as long as it's still legible.

ok, will keep that (both mails) in mind for future stuff.

Heiko

[toc] | [prev] | [next] | [standalone]

#1405960 — Re: [PATCH] drm/rockchip: Return -EBUSY if there's already a pending flip event v5

On Tue, May 24, 2016 at 10:41:30AM +0200, Heiko Stuebner wrote:
> Am Dienstag, 24. Mai 2016, 10:37:49 schrieb Daniel Vetter:
> > On Tue, May 24, 2016 at 10:30:50AM +0200, Daniel Vetter wrote:
> > > On Tue, May 24, 2016 at 10:28:42AM +0200, Heiko Stuebner wrote:
> > > > Hi Tomeu,
> > > > 
> > > > Patch subject: please put the version into the brackets, so [PATCH v5]
> > > > as it shouldn't be part of the commit log.
> > > > 
> > > > Am Dienstag, 24. Mai 2016, 09:27:37 schrieb Tomeu Vizoso:
> > > > > 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.
> > > > > 
> > > > > v4: Hold dev->event_lock while checking the VOP's event field as
> > > > > suggested by Daniel Stone.
> > > > > 
> > > > > v5: Only block if there's outstanding work if it's a blocking call.
> > > > 
> > > > similarly, please put the changelog below the "---" and above the
> > > > diffstat.> 
> > > drm culture is to keep it above, since it's kinda useful sometimes when
> > > later on trying to reconstruct wtf was discussed and why a patch was
> > > merged.
> > 
> > Maybe needs a bit more context: The only stuff you raised in your review
> > is tiny style nits of pretty much utter irrelevance. No substantial and
> > material feedback anywehere, and in my opinion in such a case either fix
> > up the nits when applying (when you feel really strongly about perfect
> > patches), or just merge as-is.
> > 
> > But sending out content-less bikesheds like these just adds noise and
> > helps no-one. I think at least some spelling stuff is the minimal bar (but
> > then just include your r-b tag), but personally I don't even care about
> > that so much, as long as it's still legible.
> 
> ok, will keep that (both mails) in mind for future stuff.

And to clarify, review of patches is very much appreciated, but to be
effective it should be top down. First assess whether it's a good idea,
then whether the implementation makes sense, then go down into style
naming and details like that. And it's important to tell the submitter
where they are in that process, too. More in-depth writeup of a good
review approach:

http://sarah.thesharps.us/2014/09/01/the-gentle-art-of-patch-review/

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web