| Message ID | 20200707201229.472834-8-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | dma-fence annotations, round 3 | expand |
On Tue, Jul 07, 2020 at 10:12:11PM +0200, Daniel Vetter wrote: > Like the helpers, nothing special. Well except not, because we the > critical section extends until after hw_done(), since that's the last > thing which could hold up a subsequent atomic commit. That means the > wait_for_flip_done is included, but that's not a problem, we're > allowed to call dma_fence_wait() from signalling critical sections. > Even on our own fence (which this does), it's just a bit confusing. > But in a way those last 2 function calls are already part of the fence > signalling critical section for the next atomic commit. > > Reading this I'm wondering why komeda waits for flip_done() before > calling hw_done(), which is a bit backwards (but hey hw can be > special). Might be good to throw a comment in there that explains why, > because the original commit that added this just doesn't. Hi Daniel: It's a typo, thank you for pointing this out, and I'll give a fix after this series have been merged. for this patch Reviewed-by: James Qian Wang <james.qian.wang@arm.com> > Cc: "James (Qian) Wang" <james.qian.wang@arm.com> > Cc: Liviu Dudau <liviu.dudau@arm.com> > Cc: Mihail Atanassov <mihail.atanassov@arm.com> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c > index 1f6682032ca4..cc5b5915bc5e 100644 > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c > @@ -73,6 +73,7 @@ static struct drm_driver komeda_kms_driver = { > static void komeda_kms_commit_tail(struct drm_atomic_state *old_state) > { > struct drm_device *dev = old_state->dev; > + bool fence_cookie = dma_fence_begin_signalling(); > > drm_atomic_helper_commit_modeset_disables(dev, old_state); > > @@ -85,6 +86,8 @@ static void komeda_kms_commit_tail(struct drm_atomic_state *old_state) > > drm_atomic_helper_commit_hw_done(old_state); > > + dma_fence_end_signalling(fence_cookie); > + > drm_atomic_helper_cleanup_planes(dev, old_state); > } > > -- > 2.27.0
On Wed, Jul 08, 2020 at 01:17:39PM +0800, james qian wang (Arm Technology China) wrote: > On Tue, Jul 07, 2020 at 10:12:11PM +0200, Daniel Vetter wrote: > > Like the helpers, nothing special. Well except not, because we the > > critical section extends until after hw_done(), since that's the last > > thing which could hold up a subsequent atomic commit. That means the > > wait_for_flip_done is included, but that's not a problem, we're > > allowed to call dma_fence_wait() from signalling critical sections. > > Even on our own fence (which this does), it's just a bit confusing. > > But in a way those last 2 function calls are already part of the fence > > signalling critical section for the next atomic commit. > > > > Reading this I'm wondering why komeda waits for flip_done() before > > calling hw_done(), which is a bit backwards (but hey hw can be > > special). Might be good to throw a comment in there that explains why, > > because the original commit that added this just doesn't. > > Hi Daniel: > > It's a typo, thank you for pointing this out, and I'll give a fix after > this series have been merged. > > for this patch > > Reviewed-by: James Qian Wang <james.qian.wang@arm.com> Hi James, Thanks for revieweing. Note that the "wrong" order doesn't have to be a real problem, there's other drivers which need this one too. But they explain why in a comment. So if you change that, make sure you test it all well to avoid surprises. Testing (with lockdep enabled) would be really good here, can you try to do that too? Also, next patch is for drm/malidp, can you pls review that patch too? Thanks, Daniel > > > Cc: "James (Qian) Wang" <james.qian.wang@arm.com> > > Cc: Liviu Dudau <liviu.dudau@arm.com> > > Cc: Mihail Atanassov <mihail.atanassov@arm.com> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c > > index 1f6682032ca4..cc5b5915bc5e 100644 > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c > > @@ -73,6 +73,7 @@ static struct drm_driver komeda_kms_driver = { > > static void komeda_kms_commit_tail(struct drm_atomic_state *old_state) > > { > > struct drm_device *dev = old_state->dev; > > + bool fence_cookie = dma_fence_begin_signalling(); > > > > drm_atomic_helper_commit_modeset_disables(dev, old_state); > > > > @@ -85,6 +86,8 @@ static void komeda_kms_commit_tail(struct drm_atomic_state *old_state) > > > > drm_atomic_helper_commit_hw_done(old_state); > > > > + dma_fence_end_signalling(fence_cookie); > > + > > drm_atomic_helper_cleanup_planes(dev, old_state); > > } > > > > -- > > 2.27.0
diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c index 1f6682032ca4..cc5b5915bc5e 100644 --- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c +++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c @@ -73,6 +73,7 @@ static struct drm_driver komeda_kms_driver = { static void komeda_kms_commit_tail(struct drm_atomic_state *old_state) { struct drm_device *dev = old_state->dev; + bool fence_cookie = dma_fence_begin_signalling(); drm_atomic_helper_commit_modeset_disables(dev, old_state); @@ -85,6 +86,8 @@ static void komeda_kms_commit_tail(struct drm_atomic_state *old_state) drm_atomic_helper_commit_hw_done(old_state); + dma_fence_end_signalling(fence_cookie); + drm_atomic_helper_cleanup_planes(dev, old_state); }
Like the helpers, nothing special. Well except not, because we the critical section extends until after hw_done(), since that's the last thing which could hold up a subsequent atomic commit. That means the wait_for_flip_done is included, but that's not a problem, we're allowed to call dma_fence_wait() from signalling critical sections. Even on our own fence (which this does), it's just a bit confusing. But in a way those last 2 function calls are already part of the fence signalling critical section for the next atomic commit. Reading this I'm wondering why komeda waits for flip_done() before calling hw_done(), which is a bit backwards (but hey hw can be special). Might be good to throw a comment in there that explains why, because the original commit that added this just doesn't. Cc: "James (Qian) Wang" <james.qian.wang@arm.com> Cc: Liviu Dudau <liviu.dudau@arm.com> Cc: Mihail Atanassov <mihail.atanassov@arm.com> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 3 +++ 1 file changed, 3 insertions(+)