Re: [Intel-gfx] [PATCH] drm/i915/display: Use dma_fence interfaces instead of i915_sw_fence

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Wed, 2023-10-18 at 20:59 +0300, Ville Syrjälä wrote:
> On Wed, Oct 18, 2023 at 07:13:12PM +0200, Maarten Lankhorst wrote:
> > 
> > 
> > On 2023-10-18 17:38, Ville Syrjälä wrote:
> > > On Mon, Oct 16, 2023 at 11:08:03AM +0300, Jouni Högander wrote:
> > > > We are preparing for Xe driver. Xe driver doesn't have
> > > > i915_sw_fence
> > > > implementation. Lets drop i915_sw_fence usage from display code
> > > > and
> > > > use dma_fence interfaces directly.
> > > > 
> > > > For this purpose stack dma fences from related objects into old
> > > > and new
> > > > plane states using drm_gem_plane_helper_prepare_fb. Then wait
> > > > for these
> > > > stacked fences during atomic commit.
> > > > 
> > > > There is no be need for separate GPU reset handling in
> > > > intel_atomic_commit_fence_wait as the fences are signaled when
> > > > GPU hang is
> > > > detected and GPU is being reset.
> > > > 
> > > > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > > Cc: José Roberto de Souza <jose.souza@intel.com>
> > > > 
> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/display/intel_atomic.c   |  3 -
> > > >   .../gpu/drm/i915/display/intel_atomic_plane.c | 49 +++-------
> > > > --
> > > >   drivers/gpu/drm/i915/display/intel_display.c  | 78 ++++++----
> > > > ---------
> > > >   .../drm/i915/display/intel_display_types.h    |  2 -
> > > >   4 files changed, 37 insertions(+), 95 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > index 5d18145da279..ec0d5168b503 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c
> > > > @@ -331,9 +331,6 @@ void intel_atomic_state_free(struct
> > > > drm_atomic_state *_state)
> > > >   
> > > >         drm_atomic_state_default_release(&state->base);
> > > >         kfree(state->global_objs);
> > > > -
> > > > -       i915_sw_fence_fini(&state->commit_ready);
> > > > -
> > > >         kfree(state);
> > > >   }
> > > >   
> > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > index b1074350616c..d4f9168ec42c 100644
> > > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > > @@ -32,6 +32,7 @@
> > > >    */
> > > >   
> > > >   #include <drm/drm_atomic_helper.h>
> > > > +#include <drm/drm_gem_atomic_helper.h>
> > > >   #include <drm/drm_blend.h>
> > > >   #include <drm/drm_fourcc.h>
> > > >   
> > > > @@ -1035,7 +1036,7 @@ intel_prepare_plane_fb(struct drm_plane
> > > > *_plane,
> > > >         struct intel_atomic_state *state =
> > > >                 to_intel_atomic_state(new_plane_state-
> > > > >uapi.state);
> > > >         struct drm_i915_private *dev_priv = to_i915(plane-
> > > > >base.dev);
> > > > -       const struct intel_plane_state *old_plane_state =
> > > > +       struct intel_plane_state *old_plane_state =
> > > >                 intel_atomic_get_old_plane_state(state, plane);
> > > >         struct drm_i915_gem_object *obj =
> > > > intel_fb_obj(new_plane_state->hw.fb);
> > > >         struct drm_i915_gem_object *old_obj =
> > > > intel_fb_obj(old_plane_state->hw.fb);
> > > > @@ -1057,56 +1058,30 @@ intel_prepare_plane_fb(struct drm_plane
> > > > *_plane,
> > > >                  * This should only fail upon a hung GPU, in
> > > > which case we
> > > >                  * can safely continue.
> > > >                  */
> > > > -               if (new_crtc_state &&
> > > > intel_crtc_needs_modeset(new_crtc_state)) {
> > > > -                       ret =
> > > > i915_sw_fence_await_reservation(&state->commit_ready,
> > > > -                                                            
> > > > old_obj->base.resv,
> > > > -                                                            
> > > > false, 0,
> > > > -                                                            
> > > > GFP_KERNEL);
> > > > +               if (new_crtc_state &&
> > > > intel_crtc_needs_modeset(new_crtc_state) &&
> > > > +                   !dma_resv_test_signaled(old_obj->base.resv,
> > > > +                                          
> > > > dma_resv_usage_rw(false))) {
> > > > +                       ret =
> > > > drm_gem_plane_helper_prepare_fb(_plane, &old_plane_state-
> > > > >uapi);
> > > 
> > > This I think is broken. The old plane state and its fence can
> > > still be
> > > in use by the previous commit, so we cannot mutate it here. Thus
> > > we
> > > really need to get the implicit fence from the old fb chained
> > > into the
> > > new plane state's fence.
> > Is it even needed though? If new_plane_state always calls
> > prepare_fb.
> 
> It's explained in the comment.

Thank you Ville and Maarten for checking my patch. I have addressed
Villes comment about old plane state fence. Please check new version.

BR,

Jouni Högander

>