| Message ID | 20180405154449.23038-7-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Daniel Vetter <daniel.vetter@ffwll.ch> writes: > Note that a pile of drivers don't seem to take implicit fencing into > account, or at least don't call drm_atoimc_set_fence_for_plane(). ^atomic > Cc'ing relevant people, or at least some. Some drivers also look like > they don't disable implicit fencing (e.g. amdgpu) because the explicit > fences and implicit fences are handled by entirely independent code > paths. > > I also wonder whether we shouldn't just make the recommended helpers > the default ones, since a lot of drivers don't bother to handle the > implicit fences at all it seems. The helpers won't blow up even for > non-GEM drivers or GEM drivers which don't fill out the gem bo > pointers in struct drm_framebuffer. > > Cc: Gerd Hoffmann <kraxel@redhat.com> > Cc: Alex Deucher <alexander.deucher@amd.com> > Cc: Harry Wentland <harry.wentland@amd.com> > Cc: Sinclair Yeh <syeh@vmware.com> > Cc: Thomas Hellstrom <thellstrom@vmware.com> > Cc: Gustavo Padovan <gustavo@padovan.org> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > Cc: Sean Paul <seanpaul@chromium.org> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 8 ++++++++ > include/drm/drm_modeset_helper_vtables.h | 5 ++++- > include/drm/drm_plane.h | 7 ++++++- > 3 files changed, 18 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 7d25c42f22db..ec77afbda0c3 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -1492,6 +1492,14 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane); > * Otherwise, if &drm_plane_state.fence is not set this function we just set it > * with the received implicit fence. In both cases this function consumes a > * reference for @fence. > + * > + * This way explicit fencing can be used to overrule implicit fencing, which is > + * important to make explicit fencing use-cases work: One example is using one > + * buffer for 2 screens with different refresh rates. Implicit fencing will > + * clamp rendering to the refresh rate of the slower screen, whereas explicit > + * fence allows 2 independent render and display loops on a single buffer. If a > + * driver allows obeys both implicit and explicit fences for plane updates, then > + * it will break all the benefits of explicit fencing. > */ Thanks for explaining why we should care about explicit fencing for display! I'd been trying and failing to generate a usecase. > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index d6da26d66a4b..1e2622e33208 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -80,7 +80,12 @@ struct drm_plane_state { > * @fence: > * > * Optional fence to wait for before scanning out @fb. Do not write this > - * directly, use drm_atomic_set_fence_for_plane() > + * directly, use drm_atomic_set_fence_for_plane(). The core atomic code > + * will set this when userspace is using explicit fencing. Optional suggestion: * Optional fence to wait for before scanning out @fb. The core * atomic code will set this when userspace is using explicit * fencing. Do not write this directly for a driver's implicit * fence, use drm_atomic_set_fence_for_plane() to ensure that * an explicit fence is preserved. > + * > + * Drivers should store any implicit fences in this from their Maybe s/fences/fence/ to make it more obvious that you can only attach one? > + * &drm_plane_helper.prepare_fb callback. See drm_gem_fb_prepare_fb() > + * and drm_gem_fb_simple_display_pipe_prepare_fb() for suitable helpers. > */ > struct dma_fence *fence; Regardless, Reviewed-by: Eric Anholt <eric@anholt.net>
On Fri, Apr 20, 2018 at 03:04:58PM -0700, Eric Anholt wrote: > Daniel Vetter <daniel.vetter@ffwll.ch> writes: > > > Note that a pile of drivers don't seem to take implicit fencing into > > account, or at least don't call drm_atoimc_set_fence_for_plane(). > > ^atomic > > > Cc'ing relevant people, or at least some. Some drivers also look like > > they don't disable implicit fencing (e.g. amdgpu) because the explicit > > fences and implicit fences are handled by entirely independent code > > paths. > > > > I also wonder whether we shouldn't just make the recommended helpers > > the default ones, since a lot of drivers don't bother to handle the > > implicit fences at all it seems. The helpers won't blow up even for > > non-GEM drivers or GEM drivers which don't fill out the gem bo > > pointers in struct drm_framebuffer. > > > > Cc: Gerd Hoffmann <kraxel@redhat.com> > > Cc: Alex Deucher <alexander.deucher@amd.com> > > Cc: Harry Wentland <harry.wentland@amd.com> > > Cc: Sinclair Yeh <syeh@vmware.com> > > Cc: Thomas Hellstrom <thellstrom@vmware.com> > > Cc: Gustavo Padovan <gustavo@padovan.org> > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> > > Cc: Sean Paul <seanpaul@chromium.org> > > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> > > --- > > drivers/gpu/drm/drm_atomic.c | 8 ++++++++ > > include/drm/drm_modeset_helper_vtables.h | 5 ++++- > > include/drm/drm_plane.h | 7 ++++++- > > 3 files changed, 18 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > > index 7d25c42f22db..ec77afbda0c3 100644 > > --- a/drivers/gpu/drm/drm_atomic.c > > +++ b/drivers/gpu/drm/drm_atomic.c > > @@ -1492,6 +1492,14 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane); > > * Otherwise, if &drm_plane_state.fence is not set this function we just set it > > * with the received implicit fence. In both cases this function consumes a > > * reference for @fence. > > + * > > + * This way explicit fencing can be used to overrule implicit fencing, which is > > + * important to make explicit fencing use-cases work: One example is using one > > + * buffer for 2 screens with different refresh rates. Implicit fencing will > > + * clamp rendering to the refresh rate of the slower screen, whereas explicit > > + * fence allows 2 independent render and display loops on a single buffer. If a > > + * driver allows obeys both implicit and explicit fences for plane updates, then > > + * it will break all the benefits of explicit fencing. > > */ > > Thanks for explaining why we should care about explicit fencing for > display! I'd been trying and failing to generate a usecase. > > > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > > index d6da26d66a4b..1e2622e33208 100644 > > --- a/include/drm/drm_plane.h > > +++ b/include/drm/drm_plane.h > > @@ -80,7 +80,12 @@ struct drm_plane_state { > > * @fence: > > * > > * Optional fence to wait for before scanning out @fb. Do not write this > > - * directly, use drm_atomic_set_fence_for_plane() > > + * directly, use drm_atomic_set_fence_for_plane(). The core atomic code > > + * will set this when userspace is using explicit fencing. > > Optional suggestion: > > * Optional fence to wait for before scanning out @fb. The core > * atomic code will set this when userspace is using explicit > * fencing. Do not write this directly for a driver's implicit > * fence, use drm_atomic_set_fence_for_plane() to ensure that > * an explicit fence is preserved. Yeah, that's better. > > > + * > > + * Drivers should store any implicit fences in this from their > > Maybe s/fences/fence/ to make it more obvious that you can only attach > one? Both suggestions implemented while applying, thanks very much for your review. -Daniel > > > + * &drm_plane_helper.prepare_fb callback. See drm_gem_fb_prepare_fb() > > + * and drm_gem_fb_simple_display_pipe_prepare_fb() for suitable helpers. > > */ > > struct dma_fence *fence; > > Regardless, > > Reviewed-by: Eric Anholt <eric@anholt.net>
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7d25c42f22db..ec77afbda0c3 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -1492,6 +1492,14 @@ EXPORT_SYMBOL(drm_atomic_set_fb_for_plane); * Otherwise, if &drm_plane_state.fence is not set this function we just set it * with the received implicit fence. In both cases this function consumes a * reference for @fence. + * + * This way explicit fencing can be used to overrule implicit fencing, which is + * important to make explicit fencing use-cases work: One example is using one + * buffer for 2 screens with different refresh rates. Implicit fencing will + * clamp rendering to the refresh rate of the slower screen, whereas explicit + * fence allows 2 independent render and display loops on a single buffer. If a + * driver allows obeys both implicit and explicit fences for plane updates, then + * it will break all the benefits of explicit fencing. */ void drm_atomic_set_fence_for_plane(struct drm_plane_state *plane_state, diff --git a/include/drm/drm_modeset_helper_vtables.h b/include/drm/drm_modeset_helper_vtables.h index 3e76ca805b0f..35e2a3a79fc5 100644 --- a/include/drm/drm_modeset_helper_vtables.h +++ b/include/drm/drm_modeset_helper_vtables.h @@ -1004,11 +1004,14 @@ struct drm_plane_helper_funcs { * This function must not block for outstanding rendering, since it is * called in the context of the atomic IOCTL even for async commits to * be able to return any errors to userspace. Instead the recommended - * way is to fill out the fence member of the passed-in + * way is to fill out the &drm_plane_state.fence of the passed-in * &drm_plane_state. If the driver doesn't support native fences then * equivalent functionality should be implemented through private * members in the plane structure. * + * Drivers which always have their buffers pinned should use + * drm_gem_fb_prepare_fb() for this hook. + * * The helpers will call @cleanup_fb with matching arguments for every * successful call to this hook. * diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index d6da26d66a4b..1e2622e33208 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -80,7 +80,12 @@ struct drm_plane_state { * @fence: * * Optional fence to wait for before scanning out @fb. Do not write this - * directly, use drm_atomic_set_fence_for_plane() + * directly, use drm_atomic_set_fence_for_plane(). The core atomic code + * will set this when userspace is using explicit fencing. + * + * Drivers should store any implicit fences in this from their + * &drm_plane_helper.prepare_fb callback. See drm_gem_fb_prepare_fb() + * and drm_gem_fb_simple_display_pipe_prepare_fb() for suitable helpers. */ struct dma_fence *fence;
Note that a pile of drivers don't seem to take implicit fencing into account, or at least don't call drm_atoimc_set_fence_for_plane(). Cc'ing relevant people, or at least some. Some drivers also look like they don't disable implicit fencing (e.g. amdgpu) because the explicit fences and implicit fences are handled by entirely independent code paths. I also wonder whether we shouldn't just make the recommended helpers the default ones, since a lot of drivers don't bother to handle the implicit fences at all it seems. The helpers won't blow up even for non-GEM drivers or GEM drivers which don't fill out the gem bo pointers in struct drm_framebuffer. Cc: Gerd Hoffmann <kraxel@redhat.com> Cc: Alex Deucher <alexander.deucher@amd.com> Cc: Harry Wentland <harry.wentland@amd.com> Cc: Sinclair Yeh <syeh@vmware.com> Cc: Thomas Hellstrom <thellstrom@vmware.com> Cc: Gustavo Padovan <gustavo@padovan.org> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com> Cc: Sean Paul <seanpaul@chromium.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- drivers/gpu/drm/drm_atomic.c | 8 ++++++++ include/drm/drm_modeset_helper_vtables.h | 5 ++++- include/drm/drm_plane.h | 7 ++++++- 3 files changed, 18 insertions(+), 2 deletions(-)