| Message ID | 20211001100610.2899-25-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/28] dma-buf: add dma_resv_for_each_fence_unlocked v7 | expand |
On 01/10/2021 11:06, Christian König wrote: > Makes the handling a bit more complex, but avoids the use of > dma_resv_get_excl_unlocked(). > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/drm_gem_atomic_helper.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c > index e570398abd78..21ed930042b8 100644 > --- a/drivers/gpu/drm/drm_gem_atomic_helper.c > +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c > @@ -143,6 +143,7 @@ > */ > int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) > { > + struct dma_resv_iter cursor; > struct drm_gem_object *obj; > struct dma_fence *fence; > > @@ -150,9 +151,17 @@ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_st > return 0; > > obj = drm_gem_fb_get_obj(state->fb, 0); > - fence = dma_resv_get_excl_unlocked(obj->resv); > - drm_atomic_set_fence_for_plane(state, fence); > + dma_resv_iter_begin(&cursor, obj->resv, false); > + dma_resv_for_each_fence_unlocked(&cursor, fence) { > + dma_fence_get(fence); > + dma_resv_iter_end(&cursor); > + /* TODO: We only use the first write fence here */ What is the TODO? NB instead? > + drm_atomic_set_fence_for_plane(state, fence); > + return 0; > + } > + dma_resv_iter_end(&cursor); > > + drm_atomic_set_fence_for_plane(state, NULL); dma_resv_iter_begin(&cursor, obj->resv, false); dma_resv_for_each_fence_unlocked(&cursor, fence) { dma_fence_get(fence); break; } dma_resv_iter_end(&cursor); drm_atomic_set_fence_for_plane(state, fence); Does this work? But overall I am not sure this is nicer. Is the goal to remove dma_resv_get_excl_unlocked as API it just does not happen in this series? Regards, Tvrtko > return 0; > } > EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); >
Am 05.10.21 um 09:53 schrieb Tvrtko Ursulin: > > On 01/10/2021 11:06, Christian König wrote: >> Makes the handling a bit more complex, but avoids the use of >> dma_resv_get_excl_unlocked(). >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/gpu/drm/drm_gem_atomic_helper.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c >> b/drivers/gpu/drm/drm_gem_atomic_helper.c >> index e570398abd78..21ed930042b8 100644 >> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c >> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c >> @@ -143,6 +143,7 @@ >> */ >> int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct >> drm_plane_state *state) >> { >> + struct dma_resv_iter cursor; >> struct drm_gem_object *obj; >> struct dma_fence *fence; >> @@ -150,9 +151,17 @@ int drm_gem_plane_helper_prepare_fb(struct >> drm_plane *plane, struct drm_plane_st >> return 0; >> obj = drm_gem_fb_get_obj(state->fb, 0); >> - fence = dma_resv_get_excl_unlocked(obj->resv); >> - drm_atomic_set_fence_for_plane(state, fence); >> + dma_resv_iter_begin(&cursor, obj->resv, false); >> + dma_resv_for_each_fence_unlocked(&cursor, fence) { >> + dma_fence_get(fence); >> + dma_resv_iter_end(&cursor); >> + /* TODO: We only use the first write fence here */ > > What is the TODO? NB instead? The drm atomic API can unfortunately handle only one fence and we can certainly have more than that. > >> + drm_atomic_set_fence_for_plane(state, fence); >> + return 0; >> + } >> + dma_resv_iter_end(&cursor); >> + drm_atomic_set_fence_for_plane(state, NULL); > > dma_resv_iter_begin(&cursor, obj->resv, false); > dma_resv_for_each_fence_unlocked(&cursor, fence) { > dma_fence_get(fence); > break; > } > dma_resv_iter_end(&cursor); > > drm_atomic_set_fence_for_plane(state, fence); > > Does this work? Yeah that should work as well. > > But overall I am not sure this is nicer. Is the goal to remove > dma_resv_get_excl_unlocked as API it just does not happen in this series? Yes, the only user left is the i915_gem_object_last_write_engine() function and that one you already removed in i915. Regards, Christian. > > Regards, > > Tvrtko > >> return 0; >> } >> EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); >>
On 05/10/2021 11:27, Christian König wrote: > Am 05.10.21 um 09:53 schrieb Tvrtko Ursulin: >> >> On 01/10/2021 11:06, Christian König wrote: >>> Makes the handling a bit more complex, but avoids the use of >>> dma_resv_get_excl_unlocked(). >>> >>> Signed-off-by: Christian König <christian.koenig@amd.com> >>> --- >>> drivers/gpu/drm/drm_gem_atomic_helper.c | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c >>> b/drivers/gpu/drm/drm_gem_atomic_helper.c >>> index e570398abd78..21ed930042b8 100644 >>> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c >>> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c >>> @@ -143,6 +143,7 @@ >>> */ >>> int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct >>> drm_plane_state *state) >>> { >>> + struct dma_resv_iter cursor; >>> struct drm_gem_object *obj; >>> struct dma_fence *fence; >>> @@ -150,9 +151,17 @@ int drm_gem_plane_helper_prepare_fb(struct >>> drm_plane *plane, struct drm_plane_st >>> return 0; >>> obj = drm_gem_fb_get_obj(state->fb, 0); >>> - fence = dma_resv_get_excl_unlocked(obj->resv); >>> - drm_atomic_set_fence_for_plane(state, fence); >>> + dma_resv_iter_begin(&cursor, obj->resv, false); >>> + dma_resv_for_each_fence_unlocked(&cursor, fence) { >>> + dma_fence_get(fence); >>> + dma_resv_iter_end(&cursor); >>> + /* TODO: We only use the first write fence here */ >> >> What is the TODO? NB instead? > > The drm atomic API can unfortunately handle only one fence and we can > certainly have more than that. > >> >>> + drm_atomic_set_fence_for_plane(state, fence); >>> + return 0; >>> + } >>> + dma_resv_iter_end(&cursor); >>> + drm_atomic_set_fence_for_plane(state, NULL); >> >> dma_resv_iter_begin(&cursor, obj->resv, false); >> dma_resv_for_each_fence_unlocked(&cursor, fence) { >> dma_fence_get(fence); >> break; >> } >> dma_resv_iter_end(&cursor); >> >> drm_atomic_set_fence_for_plane(state, fence); >> >> Does this work? > > Yeah that should work as well. > >> >> But overall I am not sure this is nicer. Is the goal to remove >> dma_resv_get_excl_unlocked as API it just does not happen in this series? > > Yes, the only user left is the i915_gem_object_last_write_engine() > function and that one you already removed in i915. To me the above feels clumsier than dma_resv_get_excl_unlocked and you can even view it as open coding that helper. So don't know, someone else can have a casting vote. I guess if support for more than one fence is coming soon(-ish) do drm atomic api then I could be convinced the iterator here makes sense today. Regards, Tvrtko > Regards, > Christian. > >> >> Regards, >> >> Tvrtko >> >>> return 0; >>> } >>> EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); >>> >
Am 05.10.21 um 12:47 schrieb Tvrtko Ursulin: > > On 05/10/2021 11:27, Christian König wrote: >> Am 05.10.21 um 09:53 schrieb Tvrtko Ursulin: >>> >>> On 01/10/2021 11:06, Christian König wrote: >>>> Makes the handling a bit more complex, but avoids the use of >>>> dma_resv_get_excl_unlocked(). >>>> >>>> Signed-off-by: Christian König <christian.koenig@amd.com> >>>> --- >>>> drivers/gpu/drm/drm_gem_atomic_helper.c | 13 +++++++++++-- >>>> 1 file changed, 11 insertions(+), 2 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c >>>> b/drivers/gpu/drm/drm_gem_atomic_helper.c >>>> index e570398abd78..21ed930042b8 100644 >>>> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c >>>> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c >>>> @@ -143,6 +143,7 @@ >>>> */ >>>> int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, >>>> struct drm_plane_state *state) >>>> { >>>> + struct dma_resv_iter cursor; >>>> struct drm_gem_object *obj; >>>> struct dma_fence *fence; >>>> @@ -150,9 +151,17 @@ int drm_gem_plane_helper_prepare_fb(struct >>>> drm_plane *plane, struct drm_plane_st >>>> return 0; >>>> obj = drm_gem_fb_get_obj(state->fb, 0); >>>> - fence = dma_resv_get_excl_unlocked(obj->resv); >>>> - drm_atomic_set_fence_for_plane(state, fence); >>>> + dma_resv_iter_begin(&cursor, obj->resv, false); >>>> + dma_resv_for_each_fence_unlocked(&cursor, fence) { >>>> + dma_fence_get(fence); >>>> + dma_resv_iter_end(&cursor); >>>> + /* TODO: We only use the first write fence here */ >>> >>> What is the TODO? NB instead? >> >> The drm atomic API can unfortunately handle only one fence and we can >> certainly have more than that. >> >>> >>>> + drm_atomic_set_fence_for_plane(state, fence); >>>> + return 0; >>>> + } >>>> + dma_resv_iter_end(&cursor); >>>> + drm_atomic_set_fence_for_plane(state, NULL); >>> >>> dma_resv_iter_begin(&cursor, obj->resv, false); >>> dma_resv_for_each_fence_unlocked(&cursor, fence) { >>> dma_fence_get(fence); >>> break; >>> } >>> dma_resv_iter_end(&cursor); >>> >>> drm_atomic_set_fence_for_plane(state, fence); >>> >>> Does this work? >> >> Yeah that should work as well. >> >>> >>> But overall I am not sure this is nicer. Is the goal to remove >>> dma_resv_get_excl_unlocked as API it just does not happen in this >>> series? >> >> Yes, the only user left is the i915_gem_object_last_write_engine() >> function and that one you already removed in i915. > > To me the above feels clumsier than dma_resv_get_excl_unlocked and you > can even view it as open coding that helper. So don't know, someone > else can have a casting vote. > > I guess if support for more than one fence is coming soon(-ish) do drm > atomic api then I could be convinced the iterator here makes sense today. Well Daniel noted that the drm atomic API needs some more work here because we don't handle different fences for different planes correctly either. We could as well postpone this change and fix the atomic functions first. Regards, Christian. > > Regards, > > Tvrtko > >> Regards, >> Christian. >> >>> >>> Regards, >>> >>> Tvrtko >>> >>>> return 0; >>>> } >>>> EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); >>>> >>
diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index e570398abd78..21ed930042b8 100644 --- a/drivers/gpu/drm/drm_gem_atomic_helper.c +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c @@ -143,6 +143,7 @@ */ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_state *state) { + struct dma_resv_iter cursor; struct drm_gem_object *obj; struct dma_fence *fence; @@ -150,9 +151,17 @@ int drm_gem_plane_helper_prepare_fb(struct drm_plane *plane, struct drm_plane_st return 0; obj = drm_gem_fb_get_obj(state->fb, 0); - fence = dma_resv_get_excl_unlocked(obj->resv); - drm_atomic_set_fence_for_plane(state, fence); + dma_resv_iter_begin(&cursor, obj->resv, false); + dma_resv_for_each_fence_unlocked(&cursor, fence) { + dma_fence_get(fence); + dma_resv_iter_end(&cursor); + /* TODO: We only use the first write fence here */ + drm_atomic_set_fence_for_plane(state, fence); + return 0; + } + dma_resv_iter_end(&cursor); + drm_atomic_set_fence_for_plane(state, NULL); return 0; } EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb);
Makes the handling a bit more complex, but avoids the use of dma_resv_get_excl_unlocked(). Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_gem_atomic_helper.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)