diff mbox series

drm/i915: Use vblank worker to unpin old legacy cursor fb safely

Message ID 20230831162643.20354-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Use vblank worker to unpin old legacy cursor fb safely | expand

Commit Message

Ville Syrjala Aug. 31, 2023, 4:26 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

The cursor hardware only does sync updates, and thus the hardware
will be scanning out from the old fb until the next start of vblank.
So in order to make the legacy cursor fastpath actually safe we
should not unpin the old fb until we're sure the hardware has
ceased accessing it. The simplest approach is to just use a vblank
work here to do the delayed unpin.

Not 100% sure it's a good idea to put this onto the same high
priority vblank worker as eg. our timing critical gamma updates.
But let's keep it simple for now, and it we later discover that
this is causing problems we can think about adding a lower
priority worker for such things.

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/display/intel_cursor.c   | 25 +++++++++++++++++--
 .../drm/i915/display/intel_display_types.h    |  3 +++
 2 files changed, 26 insertions(+), 2 deletions(-)

Comments

Maarten Lankhorst Sept. 1, 2023, 10:16 a.m. UTC | #1
Hey,


Den 2023-08-31 kl. 18:26, skrev Ville Syrjala:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> The cursor hardware only does sync updates, and thus the hardware
> will be scanning out from the old fb until the next start of vblank.
> So in order to make the legacy cursor fastpath actually safe we
> should not unpin the old fb until we're sure the hardware has
> ceased accessing it. The simplest approach is to just use a vblank
> work here to do the delayed unpin.
>
> Not 100% sure it's a good idea to put this onto the same high
> priority vblank worker as eg. our timing critical gamma updates.
> But let's keep it simple for now, and it we later discover that
> this is causing problems we can think about adding a lower
> priority worker for such things.
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/display/intel_cursor.c   | 25 +++++++++++++++++--
>   .../drm/i915/display/intel_display_types.h    |  3 +++
>   2 files changed, 26 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> index b342fad180ca..2bd1a79c6955 100644
> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> @@ -603,6 +603,16 @@ static bool intel_cursor_format_mod_supported(struct drm_plane *_plane,
>   	return format == DRM_FORMAT_ARGB8888;
>   }
>   
> +static void intel_cursor_unpin_work(struct kthread_work *base)
> +{
> +	struct drm_vblank_work *work = to_drm_vblank_work(base);
> +	struct intel_plane_state *plane_state =
> +		container_of(work, typeof(*plane_state), unpin_work);
> +
> +	intel_plane_unpin_fb(plane_state);
> +	intel_plane_destroy_state(plane_state->uapi.plane, &plane_state->uapi);
> +}
> +
>   static int
>   intel_legacy_cursor_update(struct drm_plane *_plane,
>   			   struct drm_crtc *_crtc,
> @@ -730,14 +740,25 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>   
>   	local_irq_enable();
>   
> -	intel_plane_unpin_fb(old_plane_state);
> +	if (old_plane_state->hw.fb != new_plane_state->hw.fb) {
> +		drm_vblank_work_init(&old_plane_state->unpin_work, &crtc->base,
> +				     intel_cursor_unpin_work);
> +
> +		drm_vblank_work_schedule(&old_plane_state->unpin_work,
> +					 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
> +					 false);
> +
> +		old_plane_state = NULL;
> +	} else {
> +		intel_plane_unpin_fb(old_plane_state);
> +	}

Maybe check if crtc is active and no modeset is happening? Similar to 
how the vblank worker is used in other cases. That should hopefully fix 
the cursor leak test.

Cheers,

~Maarten
Ville Syrjala Sept. 1, 2023, 10:56 a.m. UTC | #2
On Fri, Sep 01, 2023 at 12:16:21PM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> 
> Den 2023-08-31 kl. 18:26, skrev Ville Syrjala:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > The cursor hardware only does sync updates, and thus the hardware
> > will be scanning out from the old fb until the next start of vblank.
> > So in order to make the legacy cursor fastpath actually safe we
> > should not unpin the old fb until we're sure the hardware has
> > ceased accessing it. The simplest approach is to just use a vblank
> > work here to do the delayed unpin.
> >
> > Not 100% sure it's a good idea to put this onto the same high
> > priority vblank worker as eg. our timing critical gamma updates.
> > But let's keep it simple for now, and it we later discover that
> > this is causing problems we can think about adding a lower
> > priority worker for such things.
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/display/intel_cursor.c   | 25 +++++++++++++++++--
> >   .../drm/i915/display/intel_display_types.h    |  3 +++
> >   2 files changed, 26 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
> > index b342fad180ca..2bd1a79c6955 100644
> > --- a/drivers/gpu/drm/i915/display/intel_cursor.c
> > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
> > @@ -603,6 +603,16 @@ static bool intel_cursor_format_mod_supported(struct drm_plane *_plane,
> >   	return format == DRM_FORMAT_ARGB8888;
> >   }
> >   
> > +static void intel_cursor_unpin_work(struct kthread_work *base)
> > +{
> > +	struct drm_vblank_work *work = to_drm_vblank_work(base);
> > +	struct intel_plane_state *plane_state =
> > +		container_of(work, typeof(*plane_state), unpin_work);
> > +
> > +	intel_plane_unpin_fb(plane_state);
> > +	intel_plane_destroy_state(plane_state->uapi.plane, &plane_state->uapi);
> > +}
> > +
> >   static int
> >   intel_legacy_cursor_update(struct drm_plane *_plane,
> >   			   struct drm_crtc *_crtc,
> > @@ -730,14 +740,25 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
> >   
> >   	local_irq_enable();
> >   
> > -	intel_plane_unpin_fb(old_plane_state);
> > +	if (old_plane_state->hw.fb != new_plane_state->hw.fb) {
> > +		drm_vblank_work_init(&old_plane_state->unpin_work, &crtc->base,
> > +				     intel_cursor_unpin_work);
> > +
> > +		drm_vblank_work_schedule(&old_plane_state->unpin_work,
> > +					 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
> > +					 false);
> > +
> > +		old_plane_state = NULL;
> > +	} else {
> > +		intel_plane_unpin_fb(old_plane_state);
> > +	}
> 
> Maybe check if crtc is active and no modeset is happening?

We wouldn't be on this codepath if that wasn't the case.

> Similar to 
> how the vblank worker is used in other cases. That should hopefully fix 
> the cursor leak test.

The leak is likely due to the vblank worker being a bit crazy
and sometimes silently cancelling pending works. I fired a new
series at trybot that tries to avoid that.
Maarten Lankhorst Sept. 12, 2023, 7:44 p.m. UTC | #3
Hey,

On 2023-09-01 12:56, Ville Syrjälä wrote:
> On Fri, Sep 01, 2023 at 12:16:21PM +0200, Maarten Lankhorst wrote:
>> Hey,
>>
>>
>> Den 2023-08-31 kl. 18:26, skrev Ville Syrjala:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> The cursor hardware only does sync updates, and thus the hardware
>>> will be scanning out from the old fb until the next start of vblank.
>>> So in order to make the legacy cursor fastpath actually safe we
>>> should not unpin the old fb until we're sure the hardware has
>>> ceased accessing it. The simplest approach is to just use a vblank
>>> work here to do the delayed unpin.
>>>
>>> Not 100% sure it's a good idea to put this onto the same high
>>> priority vblank worker as eg. our timing critical gamma updates.
>>> But let's keep it simple for now, and it we later discover that
>>> this is causing problems we can think about adding a lower
>>> priority worker for such things.
>>>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/display/intel_cursor.c   | 25 +++++++++++++++++--
>>>    .../drm/i915/display/intel_display_types.h    |  3 +++
>>>    2 files changed, 26 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
>>> index b342fad180ca..2bd1a79c6955 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_cursor.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_cursor.c
>>> @@ -603,6 +603,16 @@ static bool intel_cursor_format_mod_supported(struct drm_plane *_plane,
>>>    	return format == DRM_FORMAT_ARGB8888;
>>>    }
>>>    
>>> +static void intel_cursor_unpin_work(struct kthread_work *base)
>>> +{
>>> +	struct drm_vblank_work *work = to_drm_vblank_work(base);
>>> +	struct intel_plane_state *plane_state =
>>> +		container_of(work, typeof(*plane_state), unpin_work);
>>> +
>>> +	intel_plane_unpin_fb(plane_state);
>>> +	intel_plane_destroy_state(plane_state->uapi.plane, &plane_state->uapi);
>>> +}
>>> +
>>>    static int
>>>    intel_legacy_cursor_update(struct drm_plane *_plane,
>>>    			   struct drm_crtc *_crtc,
>>> @@ -730,14 +740,25 @@ intel_legacy_cursor_update(struct drm_plane *_plane,
>>>    
>>>    	local_irq_enable();
>>>    
>>> -	intel_plane_unpin_fb(old_plane_state);
>>> +	if (old_plane_state->hw.fb != new_plane_state->hw.fb) {
>>> +		drm_vblank_work_init(&old_plane_state->unpin_work, &crtc->base,
>>> +				     intel_cursor_unpin_work);
>>> +
>>> +		drm_vblank_work_schedule(&old_plane_state->unpin_work,
>>> +					 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
>>> +					 false);
>>> +
>>> +		old_plane_state = NULL;
>>> +	} else {
>>> +		intel_plane_unpin_fb(old_plane_state);
>>> +	}
>>
>> Maybe check if crtc is active and no modeset is happening?
> 
> We wouldn't be on this codepath if that wasn't the case.
> 
>> Similar to
>> how the vblank worker is used in other cases. That should hopefully fix
>> the cursor leak test.
> 
> The leak is likely due to the vblank worker being a bit crazy
> and sometimes silently cancelling pending works. I fired a new
> series at trybot that tries to avoid that.
> 

I saw the trybot series. Have you tried fixing it to fire off the vblank 
  work immediately before disabling?

~Maarten
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c b/drivers/gpu/drm/i915/display/intel_cursor.c
index b342fad180ca..2bd1a79c6955 100644
--- a/drivers/gpu/drm/i915/display/intel_cursor.c
+++ b/drivers/gpu/drm/i915/display/intel_cursor.c
@@ -603,6 +603,16 @@  static bool intel_cursor_format_mod_supported(struct drm_plane *_plane,
 	return format == DRM_FORMAT_ARGB8888;
 }
 
+static void intel_cursor_unpin_work(struct kthread_work *base)
+{
+	struct drm_vblank_work *work = to_drm_vblank_work(base);
+	struct intel_plane_state *plane_state =
+		container_of(work, typeof(*plane_state), unpin_work);
+
+	intel_plane_unpin_fb(plane_state);
+	intel_plane_destroy_state(plane_state->uapi.plane, &plane_state->uapi);
+}
+
 static int
 intel_legacy_cursor_update(struct drm_plane *_plane,
 			   struct drm_crtc *_crtc,
@@ -730,14 +740,25 @@  intel_legacy_cursor_update(struct drm_plane *_plane,
 
 	local_irq_enable();
 
-	intel_plane_unpin_fb(old_plane_state);
+	if (old_plane_state->hw.fb != new_plane_state->hw.fb) {
+		drm_vblank_work_init(&old_plane_state->unpin_work, &crtc->base,
+				     intel_cursor_unpin_work);
+
+		drm_vblank_work_schedule(&old_plane_state->unpin_work,
+					 drm_crtc_accurate_vblank_count(&crtc->base) + 1,
+					 false);
+
+		old_plane_state = NULL;
+	} else {
+		intel_plane_unpin_fb(old_plane_state);
+	}
 
 out_free:
 	if (new_crtc_state)
 		intel_crtc_destroy_state(&crtc->base, &new_crtc_state->uapi);
 	if (ret)
 		intel_plane_destroy_state(&plane->base, &new_plane_state->uapi);
-	else
+	else if (old_plane_state)
 		intel_plane_destroy_state(&plane->base, &old_plane_state->uapi);
 	return ret;
 
diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
index c62f4ec315e8..07394a33e747 100644
--- a/drivers/gpu/drm/i915/display/intel_display_types.h
+++ b/drivers/gpu/drm/i915/display/intel_display_types.h
@@ -702,6 +702,9 @@  struct intel_plane_state {
 
 	struct intel_fb_view view;
 
+	/* for legacy cursor fb unpin */
+	struct drm_vblank_work unpin_work;
+
 	/* Plane pxp decryption state */
 	bool decrypt;