| Message ID | 20210917123513.1106-22-christian.koenig@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/26] dma-buf: add dma_resv_for_each_fence_unlocked v2 | expand |
On Fri, Sep 17, 2021 at 02:35:08PM +0200, Christian König wrote: > Makes the handling a bit more complex, but avoids the use of > dma_resv_get_excl_unlocked(). > > v2: add missing rcu_read_lock()/unlock() > > Signed-off-by: Christian König <christian.koenig@amd.com> > --- > drivers/gpu/drm/drm_gem_atomic_helper.c | 14 ++++++++++++-- > 1 file changed, 12 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..d8f9c6432544 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,18 @@ 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); > + rcu_read_lock(); > + dma_resv_iter_begin(&cursor, obj->resv, false); > + dma_resv_for_each_fence_unlocked(&cursor, fence) { > + rcu_read_unlock(); > + /* TODO: We only use the first write fence here */ > + drm_atomic_set_fence_for_plane(state, fence); Yeah I wonder whether we should/need to collate them all together. But I guesss whomever hits that first with their funny multi-plane yuv or whatever gets to do that. Or I'm not clear on what exactly your TODO here means? > + return 0; > + } > + dma_resv_iter_end(&cursor); > + rcu_read_unlock(); Imo we should do full dma_resv_lock here. atomic helpers are designed to allow this, and it simplifies things. Also it really doesn't matter for atomic, we should be able to do 60fps*a few planes easily :-) -Daniel > > + drm_atomic_set_fence_for_plane(state, NULL); > return 0; > } > EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); > -- > 2.25.1 >
Am 17.09.21 um 16:55 schrieb Daniel Vetter: > On Fri, Sep 17, 2021 at 02:35:08PM +0200, Christian König wrote: >> Makes the handling a bit more complex, but avoids the use of >> dma_resv_get_excl_unlocked(). >> >> v2: add missing rcu_read_lock()/unlock() >> >> Signed-off-by: Christian König <christian.koenig@amd.com> >> --- >> drivers/gpu/drm/drm_gem_atomic_helper.c | 14 ++++++++++++-- >> 1 file changed, 12 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..d8f9c6432544 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,18 @@ 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); >> + rcu_read_lock(); >> + dma_resv_iter_begin(&cursor, obj->resv, false); >> + dma_resv_for_each_fence_unlocked(&cursor, fence) { >> + rcu_read_unlock(); >> + /* TODO: We only use the first write fence here */ >> + drm_atomic_set_fence_for_plane(state, fence); > Yeah I wonder whether we should/need to collate them all together. But I > guesss whomever hits that first with their funny multi-plane yuv or > whatever gets to do that. Or I'm not clear on what exactly your TODO here > means? Yeah, exactly that. Basically we have use cases where where we have more than one fence to wait for. The TODO is here because adding that to the atomic helper is just not my construction site at the moment. Regards, Christian. > >> + return 0; >> + } >> + dma_resv_iter_end(&cursor); >> + rcu_read_unlock(); > Imo we should do full dma_resv_lock here. atomic helpers are designed to > allow this, and it simplifies things. Also it really doesn't matter for > atomic, we should be able to do 60fps*a few planes easily :-) > -Daniel > >> >> + drm_atomic_set_fence_for_plane(state, NULL); >> return 0; >> } >> EXPORT_SYMBOL_GPL(drm_gem_plane_helper_prepare_fb); >> -- >> 2.25.1 >>
diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c b/drivers/gpu/drm/drm_gem_atomic_helper.c index e570398abd78..d8f9c6432544 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,18 @@ 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); + rcu_read_lock(); + dma_resv_iter_begin(&cursor, obj->resv, false); + dma_resv_for_each_fence_unlocked(&cursor, fence) { + rcu_read_unlock(); + /* TODO: We only use the first write fence here */ + drm_atomic_set_fence_for_plane(state, fence); + return 0; + } + dma_resv_iter_end(&cursor); + rcu_read_unlock(); + 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(). v2: add missing rcu_read_lock()/unlock() Signed-off-by: Christian König <christian.koenig@amd.com> --- drivers/gpu/drm/drm_gem_atomic_helper.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)