| Message ID | 1496392332-8722-4-git-send-email-boris.brezillon@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Boris Brezillon <boris.brezillon@free-electrons.com> writes: > The VC4 KMS driver is implementing its own ->atomic_commit() but there > are a few generic helpers we can use instead of open-coding the logic. > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > --- > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- > 1 file changed, 12 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > index ad7925a9e0ea..f229abc0991b 100644 > --- a/drivers/gpu/drm/vc4/vc4_kms.c > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > struct drm_device *dev = state->dev; > struct vc4_dev *vc4 = to_vc4_dev(dev); > > + drm_atomic_helper_wait_for_fences(dev, state, false); > + > + drm_atomic_helper_wait_for_dependencies(state); With this wait_for_fences() addition and the reservation stuff that landed, I think we can rip out the "seqno cb" in vc4, and just use drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you see anything missing, with that? > + > drm_atomic_helper_commit_modeset_disables(dev, state); > > drm_atomic_helper_commit_planes(dev, state, 0); > @@ -57,10 +61,14 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > */ > state->legacy_cursor_update = false; > > + drm_atomic_helper_commit_hw_done(state); > + > drm_atomic_helper_wait_for_vblanks(dev, state); > > drm_atomic_helper_cleanup_planes(dev, state); > > + drm_atomic_helper_commit_cleanup_done(state); > + > drm_atomic_state_put(state); > > up(&vc4->async_modeset); > @@ -117,32 +125,10 @@ static int vc4_atomic_commit(struct drm_device *dev, > if (!c) > return -ENOMEM; > > - /* Make sure that any outstanding modesets have finished. */ > - if (nonblock) { > - struct drm_crtc *crtc; > - struct drm_crtc_state *crtc_state; > - unsigned long flags; > - bool busy = false; > - > - /* > - * If there's an undispatched event to send then we're > - * obviously still busy. If there isn't, then we can > - * unconditionally wait for the semaphore because it > - * shouldn't be contended (for long). > - * > - * This is to prevent a race where queuing a new flip > - * from userspace immediately on receipt of an event > - * beats our clean-up and returns EBUSY. > - */ > - spin_lock_irqsave(&dev->event_lock, flags); > - for_each_crtc_in_state(state, crtc, crtc_state, i) > - busy |= vc4_event_pending(crtc); > - spin_unlock_irqrestore(&dev->event_lock, flags); > - if (busy) { > - kfree(c); > - return -EBUSY; > - } > - } > + ret = drm_atomic_helper_setup_commit(state, nonblock); > + if (ret) > + return ret; > + Looks like vc4_event_pending() should be garbage-collected with this commit. > ret = down_interruptible(&vc4->async_modeset); > if (ret) { > kfree(c); > -- > 2.7.4
On Tue, 06 Jun 2017 13:27:09 -0700 Eric Anholt <eric@anholt.net> wrote: > Boris Brezillon <boris.brezillon@free-electrons.com> writes: > > > The VC4 KMS driver is implementing its own ->atomic_commit() but there > > are a few generic helpers we can use instead of open-coding the logic. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- > > 1 file changed, 12 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > > index ad7925a9e0ea..f229abc0991b 100644 > > --- a/drivers/gpu/drm/vc4/vc4_kms.c > > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > > struct drm_device *dev = state->dev; > > struct vc4_dev *vc4 = to_vc4_dev(dev); > > > > + drm_atomic_helper_wait_for_fences(dev, state, false); > > + > > + drm_atomic_helper_wait_for_dependencies(state); > > With this wait_for_fences() addition and the reservation stuff that > landed, I think we can rip out the "seqno cb" in vc4, and just use > drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you > see anything missing, with that? I can't tell. I haven't dig enough to understand what this seqno cb was used for :-), but Daniel was suggesting the same thing. I'll try to better understand what seqno cb does and if it's all safe to get rid of it and use the standard helpers. > > > + > > drm_atomic_helper_commit_modeset_disables(dev, state); > > > > drm_atomic_helper_commit_planes(dev, state, 0); > > @@ -57,10 +61,14 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > > */ > > state->legacy_cursor_update = false; > > > > + drm_atomic_helper_commit_hw_done(state); > > + > > drm_atomic_helper_wait_for_vblanks(dev, state); > > > > drm_atomic_helper_cleanup_planes(dev, state); > > > > + drm_atomic_helper_commit_cleanup_done(state); > > + > > drm_atomic_state_put(state); > > > > up(&vc4->async_modeset); > > @@ -117,32 +125,10 @@ static int vc4_atomic_commit(struct drm_device *dev, > > if (!c) > > return -ENOMEM; > > > > - /* Make sure that any outstanding modesets have finished. */ > > - if (nonblock) { > > - struct drm_crtc *crtc; > > - struct drm_crtc_state *crtc_state; > > - unsigned long flags; > > - bool busy = false; > > - > > - /* > > - * If there's an undispatched event to send then we're > > - * obviously still busy. If there isn't, then we can > > - * unconditionally wait for the semaphore because it > > - * shouldn't be contended (for long). > > - * > > - * This is to prevent a race where queuing a new flip > > - * from userspace immediately on receipt of an event > > - * beats our clean-up and returns EBUSY. > > - */ > > - spin_lock_irqsave(&dev->event_lock, flags); > > - for_each_crtc_in_state(state, crtc, crtc_state, i) > > - busy |= vc4_event_pending(crtc); > > - spin_unlock_irqrestore(&dev->event_lock, flags); > > - if (busy) { > > - kfree(c); > > - return -EBUSY; > > - } > > - } > > + ret = drm_atomic_helper_setup_commit(state, nonblock); > > + if (ret) > > + return ret; > > + > > Looks like vc4_event_pending() should be garbage-collected with this > commit. Indeed. Thanks for the review.
Boris Brezillon <boris.brezillon@free-electrons.com> writes: > On Tue, 06 Jun 2017 13:27:09 -0700 > Eric Anholt <eric@anholt.net> wrote: > >> Boris Brezillon <boris.brezillon@free-electrons.com> writes: >> >> > The VC4 KMS driver is implementing its own ->atomic_commit() but there >> > are a few generic helpers we can use instead of open-coding the logic. >> > >> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> >> > --- >> > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- >> > 1 file changed, 12 insertions(+), 26 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c >> > index ad7925a9e0ea..f229abc0991b 100644 >> > --- a/drivers/gpu/drm/vc4/vc4_kms.c >> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c >> > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) >> > struct drm_device *dev = state->dev; >> > struct vc4_dev *vc4 = to_vc4_dev(dev); >> > >> > + drm_atomic_helper_wait_for_fences(dev, state, false); >> > + >> > + drm_atomic_helper_wait_for_dependencies(state); >> >> With this wait_for_fences() addition and the reservation stuff that >> landed, I think we can rip out the "seqno cb" in vc4, and just use >> drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you >> see anything missing, with that? > > I can't tell. I haven't dig enough to understand what this seqno cb was > used for :-), but Daniel was suggesting the same thing. I'll try to > better understand what seqno cb does and if it's all safe to get rid of > it and use the standard helpers. The seqno cb was the thing for stalling the modeset until V3D was done rendering to the planes. The wait_for_fences() does the same thing using generic dmabuf reservations, so the seqno cb isn't needed any more.
+Gustavo On Tue, 06 Jun 2017 13:27:09 -0700 Eric Anholt <eric@anholt.net> wrote: > Boris Brezillon <boris.brezillon@free-electrons.com> writes: > > > The VC4 KMS driver is implementing its own ->atomic_commit() but there > > are a few generic helpers we can use instead of open-coding the logic. > > > > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > > --- > > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- > > 1 file changed, 12 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > > index ad7925a9e0ea..f229abc0991b 100644 > > --- a/drivers/gpu/drm/vc4/vc4_kms.c > > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > > struct drm_device *dev = state->dev; > > struct vc4_dev *vc4 = to_vc4_dev(dev); > > > > + drm_atomic_helper_wait_for_fences(dev, state, false); > > + > > + drm_atomic_helper_wait_for_dependencies(state); > > With this wait_for_fences() addition and the reservation stuff that > landed, I think we can rip out the "seqno cb" in vc4, and just use > drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you > see anything missing, with that? We can probably get rid of "seqno cb", but I'm not sure we have everything in place to use drm_atomic_helper_commit_tail() and drm_atomic_helper_commit() yet. What about async page flips? The async_modeset semaphore is taken in vc4_atomic_commit() to prevent races with async page flips. Can we safely get rid of this? Would Gustavo's work on async plane update help us get rid of this semaphore? Also note that patch 4 replaces the call to drm_atomic_helper_wait_for_vblanks() by one to drm_atomic_helper_wait_for_flip_done(), and we haven't decided yet if this is something we want to make generic.
Eric Anholt <eric@anholt.net> writes: > [ Unknown signature status ] > Boris Brezillon <boris.brezillon@free-electrons.com> writes: > >> On Tue, 06 Jun 2017 13:27:09 -0700 >> Eric Anholt <eric@anholt.net> wrote: >> >>> Boris Brezillon <boris.brezillon@free-electrons.com> writes: >>> >>> > The VC4 KMS driver is implementing its own ->atomic_commit() but there >>> > are a few generic helpers we can use instead of open-coding the logic. >>> > >>> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> >>> > --- >>> > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- >>> > 1 file changed, 12 insertions(+), 26 deletions(-) >>> > >>> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c >>> > index ad7925a9e0ea..f229abc0991b 100644 >>> > --- a/drivers/gpu/drm/vc4/vc4_kms.c >>> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c >>> > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) >>> > struct drm_device *dev = state->dev; >>> > struct vc4_dev *vc4 = to_vc4_dev(dev); >>> > >>> > + drm_atomic_helper_wait_for_fences(dev, state, false); >>> > + >>> > + drm_atomic_helper_wait_for_dependencies(state); >>> >>> With this wait_for_fences() addition and the reservation stuff that >>> landed, I think we can rip out the "seqno cb" in vc4, and just use >>> drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you >>> see anything missing, with that? >> >> I can't tell. I haven't dig enough to understand what this seqno cb was >> used for :-), but Daniel was suggesting the same thing. I'll try to >> better understand what seqno cb does and if it's all safe to get rid of >> it and use the standard helpers. > > The seqno cb was the thing for stalling the modeset until V3D was done > rendering to the planes. The wait_for_fences() does the same thing > using generic dmabuf reservations, so the seqno cb isn't needed any > more. Oh, we've got another user of the seqno cb (async flips), anyway, so we can't delete it quite yet. I've pushed this patch since it's a clear improvement (less code, waits for fences from other devices if any).
On Thu, 15 Jun 2017 16:33:11 -0700 Eric Anholt <eric@anholt.net> wrote: > Eric Anholt <eric@anholt.net> writes: > > > [ Unknown signature status ] > > Boris Brezillon <boris.brezillon@free-electrons.com> writes: > > > >> On Tue, 06 Jun 2017 13:27:09 -0700 > >> Eric Anholt <eric@anholt.net> wrote: > >> > >>> Boris Brezillon <boris.brezillon@free-electrons.com> writes: > >>> > >>> > The VC4 KMS driver is implementing its own ->atomic_commit() but there > >>> > are a few generic helpers we can use instead of open-coding the logic. > >>> > > >>> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> > >>> > --- > >>> > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- > >>> > 1 file changed, 12 insertions(+), 26 deletions(-) > >>> > > >>> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c > >>> > index ad7925a9e0ea..f229abc0991b 100644 > >>> > --- a/drivers/gpu/drm/vc4/vc4_kms.c > >>> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c > >>> > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) > >>> > struct drm_device *dev = state->dev; > >>> > struct vc4_dev *vc4 = to_vc4_dev(dev); > >>> > > >>> > + drm_atomic_helper_wait_for_fences(dev, state, false); > >>> > + > >>> > + drm_atomic_helper_wait_for_dependencies(state); > >>> > >>> With this wait_for_fences() addition and the reservation stuff that > >>> landed, I think we can rip out the "seqno cb" in vc4, and just use > >>> drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you > >>> see anything missing, with that? > >> > >> I can't tell. I haven't dig enough to understand what this seqno cb was > >> used for :-), but Daniel was suggesting the same thing. I'll try to > >> better understand what seqno cb does and if it's all safe to get rid of > >> it and use the standard helpers. > > > > The seqno cb was the thing for stalling the modeset until V3D was done > > rendering to the planes. The wait_for_fences() does the same thing > > using generic dmabuf reservations, so the seqno cb isn't needed any > > more. > > Oh, we've got another user of the seqno cb (async flips), anyway, so we > can't delete it quite yet. I've pushed this patch since it's a clear > improvement (less code, waits for fences from other devices if any). Ok. Did you fix the leak before applying?
Boris Brezillon <boris.brezillon@free-electrons.com> writes: > On Thu, 15 Jun 2017 16:33:11 -0700 > Eric Anholt <eric@anholt.net> wrote: > >> Eric Anholt <eric@anholt.net> writes: >> >> > [ Unknown signature status ] >> > Boris Brezillon <boris.brezillon@free-electrons.com> writes: >> > >> >> On Tue, 06 Jun 2017 13:27:09 -0700 >> >> Eric Anholt <eric@anholt.net> wrote: >> >> >> >>> Boris Brezillon <boris.brezillon@free-electrons.com> writes: >> >>> >> >>> > The VC4 KMS driver is implementing its own ->atomic_commit() but there >> >>> > are a few generic helpers we can use instead of open-coding the logic. >> >>> > >> >>> > Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> >> >>> > --- >> >>> > drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- >> >>> > 1 file changed, 12 insertions(+), 26 deletions(-) >> >>> > >> >>> > diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c >> >>> > index ad7925a9e0ea..f229abc0991b 100644 >> >>> > --- a/drivers/gpu/drm/vc4/vc4_kms.c >> >>> > +++ b/drivers/gpu/drm/vc4/vc4_kms.c >> >>> > @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) >> >>> > struct drm_device *dev = state->dev; >> >>> > struct vc4_dev *vc4 = to_vc4_dev(dev); >> >>> > >> >>> > + drm_atomic_helper_wait_for_fences(dev, state, false); >> >>> > + >> >>> > + drm_atomic_helper_wait_for_dependencies(state); >> >>> >> >>> With this wait_for_fences() addition and the reservation stuff that >> >>> landed, I think we can rip out the "seqno cb" in vc4, and just use >> >>> drm_atomic_helper_commit() and drm_atomic_hepler_commit_tail(). Do you >> >>> see anything missing, with that? >> >> >> >> I can't tell. I haven't dig enough to understand what this seqno cb was >> >> used for :-), but Daniel was suggesting the same thing. I'll try to >> >> better understand what seqno cb does and if it's all safe to get rid of >> >> it and use the standard helpers. >> > >> > The seqno cb was the thing for stalling the modeset until V3D was done >> > rendering to the planes. The wait_for_fences() does the same thing >> > using generic dmabuf reservations, so the seqno cb isn't needed any >> > more. >> >> Oh, we've got another user of the seqno cb (async flips), anyway, so we >> can't delete it quite yet. I've pushed this patch since it's a clear >> improvement (less code, waits for fences from other devices if any). > > Ok. Did you fix the leak before applying? I don't think I see discussion of a leak. What are you referring to?
diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c index ad7925a9e0ea..f229abc0991b 100644 --- a/drivers/gpu/drm/vc4/vc4_kms.c +++ b/drivers/gpu/drm/vc4/vc4_kms.c @@ -42,6 +42,10 @@ vc4_atomic_complete_commit(struct vc4_commit *c) struct drm_device *dev = state->dev; struct vc4_dev *vc4 = to_vc4_dev(dev); + drm_atomic_helper_wait_for_fences(dev, state, false); + + drm_atomic_helper_wait_for_dependencies(state); + drm_atomic_helper_commit_modeset_disables(dev, state); drm_atomic_helper_commit_planes(dev, state, 0); @@ -57,10 +61,14 @@ vc4_atomic_complete_commit(struct vc4_commit *c) */ state->legacy_cursor_update = false; + drm_atomic_helper_commit_hw_done(state); + drm_atomic_helper_wait_for_vblanks(dev, state); drm_atomic_helper_cleanup_planes(dev, state); + drm_atomic_helper_commit_cleanup_done(state); + drm_atomic_state_put(state); up(&vc4->async_modeset); @@ -117,32 +125,10 @@ static int vc4_atomic_commit(struct drm_device *dev, if (!c) return -ENOMEM; - /* Make sure that any outstanding modesets have finished. */ - if (nonblock) { - struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; - unsigned long flags; - bool busy = false; - - /* - * If there's an undispatched event to send then we're - * obviously still busy. If there isn't, then we can - * unconditionally wait for the semaphore because it - * shouldn't be contended (for long). - * - * This is to prevent a race where queuing a new flip - * from userspace immediately on receipt of an event - * beats our clean-up and returns EBUSY. - */ - spin_lock_irqsave(&dev->event_lock, flags); - for_each_crtc_in_state(state, crtc, crtc_state, i) - busy |= vc4_event_pending(crtc); - spin_unlock_irqrestore(&dev->event_lock, flags); - if (busy) { - kfree(c); - return -EBUSY; - } - } + ret = drm_atomic_helper_setup_commit(state, nonblock); + if (ret) + return ret; + ret = down_interruptible(&vc4->async_modeset); if (ret) { kfree(c);
The VC4 KMS driver is implementing its own ->atomic_commit() but there are a few generic helpers we can use instead of open-coding the logic. Signed-off-by: Boris Brezillon <boris.brezillon@free-electrons.com> --- drivers/gpu/drm/vc4/vc4_kms.c | 38 ++++++++++++-------------------------- 1 file changed, 12 insertions(+), 26 deletions(-)