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

[Daniel Vetter]

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