[Intel-gfx] [PATCH 3/3] drm/i915: Use a different vblank worker for atomic unpin

[Ville Syrjälä]

On Fri, Nov 17, 2023 at 09:14:31AM +0200, Ville Syrjälä wrote:
> On Thu, Nov 09, 2023 at 04:34:50PM +0100, Maarten Lankhorst wrote:
> > For the atomic codepath we unpin_work in old_plane_state to unpin the
> > old fb. As this happened after swapping state, this is allowed.
> > 
> > Use the unpin_work only as a barrier, and keep doing the actual
> > unpinning in the atomic path.
> > 
> > Signed-off-by: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_atomic_plane.c | 18 ++++++++++++
> >  .../gpu/drm/i915/display/intel_atomic_plane.h |  2 ++
> >  drivers/gpu/drm/i915/display/intel_crtc.c     | 28 +++++++++++++++++++
> >  3 files changed, 48 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index 06c2455bdd788..6ddb4f4ec79ac 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -1164,6 +1164,9 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
> >  	intel_display_rps_mark_interactive(dev_priv, state, false);
> >  
> >  	/* Should only be called after a successful intel_prepare_plane_fb()! */
> > +	if (old_plane_state->unpin_work.vblank)
> > +		drm_vblank_work_flush(&old_plane_state->unpin_work);
> 
> This feels rather convoluted. Can't we just schedule the cleanup work
> to the vblank worker instead of the normal wq?

I suppose the other option would be to just do a vblank wait right here.

> 
> > +
> >  	intel_plane_unpin_fb(old_plane_state);
> >  }
> >  
> > @@ -1176,3 +1179,18 @@ void intel_plane_helper_add(struct intel_plane *plane)
> >  {
> >  	drm_plane_helper_add(&plane->base, &intel_plane_helper_funcs);
> >  }
> > +
> > +/* Completion is enough */
> > +static void intel_plane_cursor_vblank_work(struct kthread_work *base)
> > +{ }
> > +
> > +void intel_plane_init_cursor_vblank_work(struct intel_plane_state *old_plane_state,
> > +					 struct intel_plane_state *new_plane_state)
> > +{
> > +	if (!old_plane_state->ggtt_vma ||
> > +	    old_plane_state->ggtt_vma == new_plane_state->ggtt_vma)
> > +		return;
> > +
> > +	drm_vblank_work_init(&old_plane_state->unpin_work, old_plane_state->uapi.crtc,
> > +			     intel_plane_cursor_vblank_work);
> > +}
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.h b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> > index 191dad0efc8e6..5a897cf6fa021 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.h
> > @@ -66,5 +66,7 @@ int intel_plane_check_src_coordinates(struct intel_plane_state *plane_state);
> >  void intel_plane_set_invisible(struct intel_crtc_state *crtc_state,
> >  			       struct intel_plane_state *plane_state);
> >  void intel_plane_helper_add(struct intel_plane *plane);
> > +void intel_plane_init_cursor_vblank_work(struct intel_plane_state *old_plane_state,
> > +					 struct intel_plane_state *new_plane_state);
> >  
> >  #endif /* __INTEL_ATOMIC_PLANE_H__ */
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> > index 1fd068e6e26ca..755c40fd0ac13 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > @@ -559,6 +559,19 @@ void intel_pipe_update_start(struct intel_atomic_state *state,
> >  	if (intel_crtc_needs_vblank_work(new_crtc_state))
> >  		intel_crtc_vblank_work_init(new_crtc_state);
> >  
> > +	if (state->base.legacy_cursor_update) {
> > +		struct intel_plane *plane;
> > +		struct intel_plane_state *old_plane_state, *new_plane_state;
> > +		int i;
> > +
> > +		for_each_oldnew_intel_plane_in_state(state, plane, old_plane_state,
> > +						     new_plane_state, i) {
> > +			if (old_plane_state->uapi.crtc == &crtc->base)
> > +				intel_plane_init_cursor_vblank_work(old_plane_state,
> > +								    new_plane_state);
> > +		}
> > +	}
> > +
> >  	intel_crtc_vblank_evade_scanlines(state, crtc, &min, &max, &vblank_start);
> >  	if (min <= 0 || max <= 0)
> >  		goto irq_disable;
> > @@ -721,6 +734,21 @@ void intel_pipe_update_end(struct intel_atomic_state *state,
> >  		new_crtc_state->uapi.event = NULL;
> >  	}
> >  
> > +	if (state->base.legacy_cursor_update) {
> > +		struct intel_plane *plane;
> > +		struct intel_plane_state *old_plane_state;
> > +		int i;
> > +
> > +		for_each_old_intel_plane_in_state(state, plane, old_plane_state, i) {
> > +			if (old_plane_state->uapi.crtc == &crtc->base &&
> > +			    old_plane_state->unpin_work.vblank) {
> > +				drm_vblank_work_schedule(&old_plane_state->unpin_work,
> > +							 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
> > +							 false);
> > +			}
> > +		}
> > +	}
> > +
> >  	/*
> >  	 * Send VRR Push to terminate Vblank. If we are already in vblank
> >  	 * this has to be done _after_ sampling the frame counter, as
> > -- 
> > 2.39.2
> 
> -- 
> Ville Syrjälä
> Intel

-- 
Ville Syrjälä
Intel