diff mbox series

[9/9] Revert "drm/i915/xe2lpd: Treat cursor plane as regular plane for DDB allocation"

Message ID 20231213102519.13500-10-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915: Cursor vblank evasion | expand

Commit Message

Ville Syrjälä Dec. 13, 2023, 10:25 a.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This reverts commit cfeff354f70bb1d0deb0279506e3f7989bc16e28.

A core design consideration with legacy cursor updates is that the
cursor must not touch any other plane, even if we were to force it
to take the slow path. That is the real reason why the cursor uses
a fixed ddb allocation, not because bspec says so.

Treating cursors as any other plane during ddb allocation
violates that, which means we can now pull other planes into
fully unsynced legacy cursor mailbox commits. That is
definitely not something we've ever considered when designing
the rest of the code. The noarm+arm register write split in
particular makes that dangerous as previous updates can get
disarmed pretty much at any random time, and not necessarily
in an order that is actually safe (eg. against ddb overlaps).

So if we were to do this then:
- someone needs to expend the appropriate amount of brain
  cells thinking through all the tricky details
- we should do it for all skl+ platforms since all
  of those have double buffered wm/ddb registers. The current
  arbitrary mtl+ cutoff doesn't really make sense

For the moment just go back to the original behaviour where
the cursor's ddb alloation does not change outside of
modeset/fastset. As of now anything else isn't safe.

Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
Cc: Matt Roper <matthew.d.roper@intel.com>
Cc: Lucas De Marchi <lucas.demarchi@intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../gpu/drm/i915/display/intel_atomic_plane.c    |  6 +++---
 drivers/gpu/drm/i915/display/skl_watermark.c     | 16 +++++++---------
 2 files changed, 10 insertions(+), 12 deletions(-)

Comments

Lisovskiy, Stanislav Dec. 13, 2023, 11:28 a.m. UTC | #1 
On Wed, Dec 13, 2023 at 12:25:19PM +0200, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This reverts commit cfeff354f70bb1d0deb0279506e3f7989bc16e28.
> 
> A core design consideration with legacy cursor updates is that the
> cursor must not touch any other plane, even if we were to force it
> to take the slow path. That is the real reason why the cursor uses
> a fixed ddb allocation, not because bspec says so.
> 
> Treating cursors as any other plane during ddb allocation
> violates that, which means we can now pull other planes into
> fully unsynced legacy cursor mailbox commits. That is
> definitely not something we've ever considered when designing
> the rest of the code. The noarm+arm register write split in
> particular makes that dangerous as previous updates can get
> disarmed pretty much at any random time, and not necessarily
> in an order that is actually safe (eg. against ddb overlaps).
> 
> So if we were to do this then:
> - someone needs to expend the appropriate amount of brain
>   cells thinking through all the tricky details

So question is how can we avoid pulling other planes to the commit?..


Stan

> - we should do it for all skl+ platforms since all
>   of those have double buffered wm/ddb registers. The current
>   arbitrary mtl+ cutoff doesn't really make sense
> 
> For the moment just go back to the original behaviour where
> the cursor's ddb alloation does not change outside of
> modeset/fastset. As of now anything else isn't safe.
> 
> Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> Cc: Matt Roper <matthew.d.roper@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../gpu/drm/i915/display/intel_atomic_plane.c    |  6 +++---
>  drivers/gpu/drm/i915/display/skl_watermark.c     | 16 +++++++---------
>  2 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index 06c2455bdd78..76d77d5a0409 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -217,6 +217,9 @@ intel_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
>  	int width, height;
>  	unsigned int rel_data_rate;
>  
> +	if (plane->id == PLANE_CURSOR)
> +		return 0;
> +
>  	if (!plane_state->uapi.visible)
>  		return 0;
>  
> @@ -244,9 +247,6 @@ intel_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
>  
>  	rel_data_rate = width * height * fb->format->cpp[color_plane];
>  
> -	if (plane->id == PLANE_CURSOR)
> -		return rel_data_rate;
> -
>  	return intel_adjusted_rate(&plane_state->uapi.src,
>  				   &plane_state->uapi.dst,
>  				   rel_data_rate);
> diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> index 56588d6e24ae..051a02ac01a4 100644
> --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> @@ -1367,7 +1367,7 @@ skl_total_relative_data_rate(const struct intel_crtc_state *crtc_state)
>  	u64 data_rate = 0;
>  
>  	for_each_plane_id_on_crtc(crtc, plane_id) {
> -		if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20)
> +		if (plane_id == PLANE_CURSOR)
>  			continue;
>  
>  		data_rate += crtc_state->rel_data_rate[plane_id];
> @@ -1514,12 +1514,10 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
>  		return 0;
>  
>  	/* Allocate fixed number of blocks for cursor. */
> -	if (DISPLAY_VER(i915) < 20) {
> -		cursor_size = skl_cursor_allocation(crtc_state, num_active);
> -		iter.size -= cursor_size;
> -		skl_ddb_entry_init(&crtc_state->wm.skl.plane_ddb[PLANE_CURSOR],
> -				   alloc->end - cursor_size, alloc->end);
> -	}
> +	cursor_size = skl_cursor_allocation(crtc_state, num_active);
> +	iter.size -= cursor_size;
> +	skl_ddb_entry_init(&crtc_state->wm.skl.plane_ddb[PLANE_CURSOR],
> +			   alloc->end - cursor_size, alloc->end);
>  
>  	iter.data_rate = skl_total_relative_data_rate(crtc_state);
>  
> @@ -1533,7 +1531,7 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
>  			const struct skl_plane_wm *wm =
>  				&crtc_state->wm.skl.optimal.planes[plane_id];
>  
> -			if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20) {
> +			if (plane_id == PLANE_CURSOR) {
>  				const struct skl_ddb_entry *ddb =
>  					&crtc_state->wm.skl.plane_ddb[plane_id];
>  
> @@ -1581,7 +1579,7 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
>  		const struct skl_plane_wm *wm =
>  			&crtc_state->wm.skl.optimal.planes[plane_id];
>  
> -		if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20)
> +		if (plane_id == PLANE_CURSOR)
>  			continue;
>  
>  		if (DISPLAY_VER(i915) < 11 &&
> -- 
> 2.41.0
>
Ville Syrjälä Dec. 13, 2023, 3:15 p.m. UTC | #2 
On Wed, Dec 13, 2023 at 01:28:15PM +0200, Lisovskiy, Stanislav wrote:
> On Wed, Dec 13, 2023 at 12:25:19PM +0200, Ville Syrjala wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > This reverts commit cfeff354f70bb1d0deb0279506e3f7989bc16e28.
> > 
> > A core design consideration with legacy cursor updates is that the
> > cursor must not touch any other plane, even if we were to force it
> > to take the slow path. That is the real reason why the cursor uses
> > a fixed ddb allocation, not because bspec says so.
> > 
> > Treating cursors as any other plane during ddb allocation
> > violates that, which means we can now pull other planes into
> > fully unsynced legacy cursor mailbox commits. That is
> > definitely not something we've ever considered when designing
> > the rest of the code. The noarm+arm register write split in
> > particular makes that dangerous as previous updates can get
> > disarmed pretty much at any random time, and not necessarily
> > in an order that is actually safe (eg. against ddb overlaps).
> > 
> > So if we were to do this then:
> > - someone needs to expend the appropriate amount of brain
> >   cells thinking through all the tricky details
> 
> So question is how can we avoid pulling other planes to the commit?..

By preallocating the ddb, as we do already.

> 
> 
> Stan
> 
> > - we should do it for all skl+ platforms since all
> >   of those have double buffered wm/ddb registers. The current
> >   arbitrary mtl+ cutoff doesn't really make sense
> > 
> > For the moment just go back to the original behaviour where
> > the cursor's ddb alloation does not change outside of
> > modeset/fastset. As of now anything else isn't safe.
> > 
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > Cc: Matt Roper <matthew.d.roper@intel.com>
> > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_atomic_plane.c    |  6 +++---
> >  drivers/gpu/drm/i915/display/skl_watermark.c     | 16 +++++++---------
> >  2 files changed, 10 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index 06c2455bdd78..76d77d5a0409 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -217,6 +217,9 @@ intel_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
> >  	int width, height;
> >  	unsigned int rel_data_rate;
> >  
> > +	if (plane->id == PLANE_CURSOR)
> > +		return 0;
> > +
> >  	if (!plane_state->uapi.visible)
> >  		return 0;
> >  
> > @@ -244,9 +247,6 @@ intel_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
> >  
> >  	rel_data_rate = width * height * fb->format->cpp[color_plane];
> >  
> > -	if (plane->id == PLANE_CURSOR)
> > -		return rel_data_rate;
> > -
> >  	return intel_adjusted_rate(&plane_state->uapi.src,
> >  				   &plane_state->uapi.dst,
> >  				   rel_data_rate);
> > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > index 56588d6e24ae..051a02ac01a4 100644
> > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > @@ -1367,7 +1367,7 @@ skl_total_relative_data_rate(const struct intel_crtc_state *crtc_state)
> >  	u64 data_rate = 0;
> >  
> >  	for_each_plane_id_on_crtc(crtc, plane_id) {
> > -		if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20)
> > +		if (plane_id == PLANE_CURSOR)
> >  			continue;
> >  
> >  		data_rate += crtc_state->rel_data_rate[plane_id];
> > @@ -1514,12 +1514,10 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
> >  		return 0;
> >  
> >  	/* Allocate fixed number of blocks for cursor. */
> > -	if (DISPLAY_VER(i915) < 20) {
> > -		cursor_size = skl_cursor_allocation(crtc_state, num_active);
> > -		iter.size -= cursor_size;
> > -		skl_ddb_entry_init(&crtc_state->wm.skl.plane_ddb[PLANE_CURSOR],
> > -				   alloc->end - cursor_size, alloc->end);
> > -	}
> > +	cursor_size = skl_cursor_allocation(crtc_state, num_active);
> > +	iter.size -= cursor_size;
> > +	skl_ddb_entry_init(&crtc_state->wm.skl.plane_ddb[PLANE_CURSOR],
> > +			   alloc->end - cursor_size, alloc->end);
> >  
> >  	iter.data_rate = skl_total_relative_data_rate(crtc_state);
> >  
> > @@ -1533,7 +1531,7 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
> >  			const struct skl_plane_wm *wm =
> >  				&crtc_state->wm.skl.optimal.planes[plane_id];
> >  
> > -			if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20) {
> > +			if (plane_id == PLANE_CURSOR) {
> >  				const struct skl_ddb_entry *ddb =
> >  					&crtc_state->wm.skl.plane_ddb[plane_id];
> >  
> > @@ -1581,7 +1579,7 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
> >  		const struct skl_plane_wm *wm =
> >  			&crtc_state->wm.skl.optimal.planes[plane_id];
> >  
> > -		if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20)
> > +		if (plane_id == PLANE_CURSOR)
> >  			continue;
> >  
> >  		if (DISPLAY_VER(i915) < 11 &&
> > -- 
> > 2.41.0
> >
Ville Syrjälä Dec. 13, 2023, 3:29 p.m. UTC | #3 
On Wed, Dec 13, 2023 at 05:15:06PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 13, 2023 at 01:28:15PM +0200, Lisovskiy, Stanislav wrote:
> > On Wed, Dec 13, 2023 at 12:25:19PM +0200, Ville Syrjala wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > 
> > > This reverts commit cfeff354f70bb1d0deb0279506e3f7989bc16e28.
> > > 
> > > A core design consideration with legacy cursor updates is that the
> > > cursor must not touch any other plane, even if we were to force it
> > > to take the slow path. That is the real reason why the cursor uses
> > > a fixed ddb allocation, not because bspec says so.
> > > 
> > > Treating cursors as any other plane during ddb allocation
> > > violates that, which means we can now pull other planes into
> > > fully unsynced legacy cursor mailbox commits. That is
> > > definitely not something we've ever considered when designing
> > > the rest of the code. The noarm+arm register write split in
> > > particular makes that dangerous as previous updates can get
> > > disarmed pretty much at any random time, and not necessarily
> > > in an order that is actually safe (eg. against ddb overlaps).
> > > 
> > > So if we were to do this then:
> > > - someone needs to expend the appropriate amount of brain
> > >   cells thinking through all the tricky details
> > 
> > So question is how can we avoid pulling other planes to the commit?..
> 
> By preallocating the ddb, as we do already.

I guess one thing we could consider is allcating the cursor ddb
based on the cursors real size (as opposed to always allocating for
256x256 cursor), and not doing a mailbox update when changing size.
But as we learn in https://gitlab.freedesktop.org/drm/intel/-/issues/7687:
- current userspace always uses 256x256 until the PLANE_SIZE_HINTS
  stuff (or something similar) lands, so there is no point
- it would lead to worse power consumption with smaller cursors
  as there isn't enough extra ddb. The fact that we currently 
  allocate the minimum for 256x256 means smallers cursor sizes
  are more efficient. Some tests I did also indicated that the
  current code does not give optimal values even if we let it
  fully calculate the extra ddb like in the reverted commit here.
  We really need someone to take a proper look at how to tune
  the ddb allocation for optimal power consumption...

> 
> > 
> > 
> > Stan
> > 
> > > - we should do it for all skl+ platforms since all
> > >   of those have double buffered wm/ddb registers. The current
> > >   arbitrary mtl+ cutoff doesn't really make sense
> > > 
> > > For the moment just go back to the original behaviour where
> > > the cursor's ddb alloation does not change outside of
> > > modeset/fastset. As of now anything else isn't safe.
> > > 
> > > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > > Cc: Matt Roper <matthew.d.roper@intel.com>
> > > Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  .../gpu/drm/i915/display/intel_atomic_plane.c    |  6 +++---
> > >  drivers/gpu/drm/i915/display/skl_watermark.c     | 16 +++++++---------
> > >  2 files changed, 10 insertions(+), 12 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > index 06c2455bdd78..76d77d5a0409 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > @@ -217,6 +217,9 @@ intel_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
> > >  	int width, height;
> > >  	unsigned int rel_data_rate;
> > >  
> > > +	if (plane->id == PLANE_CURSOR)
> > > +		return 0;
> > > +
> > >  	if (!plane_state->uapi.visible)
> > >  		return 0;
> > >  
> > > @@ -244,9 +247,6 @@ intel_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
> > >  
> > >  	rel_data_rate = width * height * fb->format->cpp[color_plane];
> > >  
> > > -	if (plane->id == PLANE_CURSOR)
> > > -		return rel_data_rate;
> > > -
> > >  	return intel_adjusted_rate(&plane_state->uapi.src,
> > >  				   &plane_state->uapi.dst,
> > >  				   rel_data_rate);
> > > diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > index 56588d6e24ae..051a02ac01a4 100644
> > > --- a/drivers/gpu/drm/i915/display/skl_watermark.c
> > > +++ b/drivers/gpu/drm/i915/display/skl_watermark.c
> > > @@ -1367,7 +1367,7 @@ skl_total_relative_data_rate(const struct intel_crtc_state *crtc_state)
> > >  	u64 data_rate = 0;
> > >  
> > >  	for_each_plane_id_on_crtc(crtc, plane_id) {
> > > -		if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20)
> > > +		if (plane_id == PLANE_CURSOR)
> > >  			continue;
> > >  
> > >  		data_rate += crtc_state->rel_data_rate[plane_id];
> > > @@ -1514,12 +1514,10 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
> > >  		return 0;
> > >  
> > >  	/* Allocate fixed number of blocks for cursor. */
> > > -	if (DISPLAY_VER(i915) < 20) {
> > > -		cursor_size = skl_cursor_allocation(crtc_state, num_active);
> > > -		iter.size -= cursor_size;
> > > -		skl_ddb_entry_init(&crtc_state->wm.skl.plane_ddb[PLANE_CURSOR],
> > > -				   alloc->end - cursor_size, alloc->end);
> > > -	}
> > > +	cursor_size = skl_cursor_allocation(crtc_state, num_active);
> > > +	iter.size -= cursor_size;
> > > +	skl_ddb_entry_init(&crtc_state->wm.skl.plane_ddb[PLANE_CURSOR],
> > > +			   alloc->end - cursor_size, alloc->end);
> > >  
> > >  	iter.data_rate = skl_total_relative_data_rate(crtc_state);
> > >  
> > > @@ -1533,7 +1531,7 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
> > >  			const struct skl_plane_wm *wm =
> > >  				&crtc_state->wm.skl.optimal.planes[plane_id];
> > >  
> > > -			if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20) {
> > > +			if (plane_id == PLANE_CURSOR) {
> > >  				const struct skl_ddb_entry *ddb =
> > >  					&crtc_state->wm.skl.plane_ddb[plane_id];
> > >  
> > > @@ -1581,7 +1579,7 @@ skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
> > >  		const struct skl_plane_wm *wm =
> > >  			&crtc_state->wm.skl.optimal.planes[plane_id];
> > >  
> > > -		if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20)
> > > +		if (plane_id == PLANE_CURSOR)
> > >  			continue;
> > >  
> > >  		if (DISPLAY_VER(i915) < 11 &&
> > > -- 
> > > 2.41.0
> > > 
> 
> -- 
> Ville Syrjälä
> Intel
Ville Syrjälä Dec. 15, 2023, 11:15 a.m. UTC | #4 
On Wed, Dec 13, 2023 at 05:29:12PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 13, 2023 at 05:15:06PM +0200, Ville Syrjälä wrote:
> > On Wed, Dec 13, 2023 at 01:28:15PM +0200, Lisovskiy, Stanislav wrote:
> > > On Wed, Dec 13, 2023 at 12:25:19PM +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > 
> > > > This reverts commit cfeff354f70bb1d0deb0279506e3f7989bc16e28.
> > > > 
> > > > A core design consideration with legacy cursor updates is that the
> > > > cursor must not touch any other plane, even if we were to force it
> > > > to take the slow path. That is the real reason why the cursor uses
> > > > a fixed ddb allocation, not because bspec says so.
> > > > 
> > > > Treating cursors as any other plane during ddb allocation
> > > > violates that, which means we can now pull other planes into
> > > > fully unsynced legacy cursor mailbox commits. That is
> > > > definitely not something we've ever considered when designing
> > > > the rest of the code. The noarm+arm register write split in
> > > > particular makes that dangerous as previous updates can get
> > > > disarmed pretty much at any random time, and not necessarily
> > > > in an order that is actually safe (eg. against ddb overlaps).
> > > > 
> > > > So if we were to do this then:
> > > > - someone needs to expend the appropriate amount of brain
> > > >   cells thinking through all the tricky details
> > > 
> > > So question is how can we avoid pulling other planes to the commit?..
> > 
> > By preallocating the ddb, as we do already.
> 
> I guess one thing we could consider is allcating the cursor ddb
> based on the cursors real size (as opposed to always allocating for
> 256x256 cursor), and not doing a mailbox update when changing size.
> But as we learn in https://gitlab.freedesktop.org/drm/intel/-/issues/7687:
> - current userspace always uses 256x256 until the PLANE_SIZE_HINTS
>   stuff (or something similar) lands, so there is no point
> - it would lead to worse power consumption with smaller cursors
>   as there isn't enough extra ddb. The fact that we currently 
>   allocate the minimum for 256x256 means smallers cursor sizes
>   are more efficient. Some tests I did also indicated that the
>   current code does not give optimal values even if we let it
>   fully calculate the extra ddb like in the reverted commit here.
>   We really need someone to take a proper look at how to tune
>   the ddb allocation for optimal power consumption...

Oh, and another random idea I keep having occasionally would
be to by default assume that legacy cursor uapi wouldn't be
used, and then massage stuff sufficiently when the first use
appears to make it work well from that point onwards. That 
way we could try to be a bit more optimal with ddb/other
stuff for userspace that never uses the legacy cursor uapi.
But haven't really thought through the details on this one.
Lisovskiy, Stanislav Jan. 9, 2024, 12:17 p.m. UTC | #5 
On Fri, Dec 15, 2023 at 01:15:16PM +0200, Ville Syrjälä wrote:
> On Wed, Dec 13, 2023 at 05:29:12PM +0200, Ville Syrjälä wrote:
> > On Wed, Dec 13, 2023 at 05:15:06PM +0200, Ville Syrjälä wrote:
> > > On Wed, Dec 13, 2023 at 01:28:15PM +0200, Lisovskiy, Stanislav wrote:
> > > > On Wed, Dec 13, 2023 at 12:25:19PM +0200, Ville Syrjala wrote:
> > > > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > > > 
> > > > > This reverts commit cfeff354f70bb1d0deb0279506e3f7989bc16e28.
> > > > > 
> > > > > A core design consideration with legacy cursor updates is that the
> > > > > cursor must not touch any other plane, even if we were to force it
> > > > > to take the slow path. That is the real reason why the cursor uses
> > > > > a fixed ddb allocation, not because bspec says so.
> > > > > 
> > > > > Treating cursors as any other plane during ddb allocation
> > > > > violates that, which means we can now pull other planes into
> > > > > fully unsynced legacy cursor mailbox commits. That is
> > > > > definitely not something we've ever considered when designing
> > > > > the rest of the code. The noarm+arm register write split in
> > > > > particular makes that dangerous as previous updates can get
> > > > > disarmed pretty much at any random time, and not necessarily
> > > > > in an order that is actually safe (eg. against ddb overlaps).
> > > > > 
> > > > > So if we were to do this then:
> > > > > - someone needs to expend the appropriate amount of brain
> > > > >   cells thinking through all the tricky details
> > > > 
> > > > So question is how can we avoid pulling other planes to the commit?..
> > > 
> > > By preallocating the ddb, as we do already.
> > 
> > I guess one thing we could consider is allcating the cursor ddb
> > based on the cursors real size (as opposed to always allocating for
> > 256x256 cursor), and not doing a mailbox update when changing size.
> > But as we learn in https://gitlab.freedesktop.org/drm/intel/-/issues/7687:
> > - current userspace always uses 256x256 until the PLANE_SIZE_HINTS
> >   stuff (or something similar) lands, so there is no point
> > - it would lead to worse power consumption with smaller cursors
> >   as there isn't enough extra ddb. The fact that we currently 
> >   allocate the minimum for 256x256 means smallers cursor sizes
> >   are more efficient. Some tests I did also indicated that the
> >   current code does not give optimal values even if we let it
> >   fully calculate the extra ddb like in the reverted commit here.
> >   We really need someone to take a proper look at how to tune
> >   the ddb allocation for optimal power consumption...
> 
> Oh, and another random idea I keep having occasionally would
> be to by default assume that legacy cursor uapi wouldn't be
> used, and then massage stuff sufficiently when the first use
> appears to make it work well from that point onwards. That 
> way we could try to be a bit more optimal with ddb/other
> stuff for userspace that never uses the legacy cursor uapi.
> But haven't really thought through the details on this one.

Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>

> 
> -- 
> Ville Syrjälä
> Intel
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index 06c2455bdd78..76d77d5a0409 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -217,6 +217,9 @@  intel_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
 	int width, height;
 	unsigned int rel_data_rate;
 
+	if (plane->id == PLANE_CURSOR)
+		return 0;
+
 	if (!plane_state->uapi.visible)
 		return 0;
 
@@ -244,9 +247,6 @@  intel_plane_relative_data_rate(const struct intel_crtc_state *crtc_state,
 
 	rel_data_rate = width * height * fb->format->cpp[color_plane];
 
-	if (plane->id == PLANE_CURSOR)
-		return rel_data_rate;
-
 	return intel_adjusted_rate(&plane_state->uapi.src,
 				   &plane_state->uapi.dst,
 				   rel_data_rate);
diff --git a/drivers/gpu/drm/i915/display/skl_watermark.c b/drivers/gpu/drm/i915/display/skl_watermark.c
index 56588d6e24ae..051a02ac01a4 100644
--- a/drivers/gpu/drm/i915/display/skl_watermark.c
+++ b/drivers/gpu/drm/i915/display/skl_watermark.c
@@ -1367,7 +1367,7 @@  skl_total_relative_data_rate(const struct intel_crtc_state *crtc_state)
 	u64 data_rate = 0;
 
 	for_each_plane_id_on_crtc(crtc, plane_id) {
-		if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20)
+		if (plane_id == PLANE_CURSOR)
 			continue;
 
 		data_rate += crtc_state->rel_data_rate[plane_id];
@@ -1514,12 +1514,10 @@  skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
 		return 0;
 
 	/* Allocate fixed number of blocks for cursor. */
-	if (DISPLAY_VER(i915) < 20) {
-		cursor_size = skl_cursor_allocation(crtc_state, num_active);
-		iter.size -= cursor_size;
-		skl_ddb_entry_init(&crtc_state->wm.skl.plane_ddb[PLANE_CURSOR],
-				   alloc->end - cursor_size, alloc->end);
-	}
+	cursor_size = skl_cursor_allocation(crtc_state, num_active);
+	iter.size -= cursor_size;
+	skl_ddb_entry_init(&crtc_state->wm.skl.plane_ddb[PLANE_CURSOR],
+			   alloc->end - cursor_size, alloc->end);
 
 	iter.data_rate = skl_total_relative_data_rate(crtc_state);
 
@@ -1533,7 +1531,7 @@  skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
 			const struct skl_plane_wm *wm =
 				&crtc_state->wm.skl.optimal.planes[plane_id];
 
-			if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20) {
+			if (plane_id == PLANE_CURSOR) {
 				const struct skl_ddb_entry *ddb =
 					&crtc_state->wm.skl.plane_ddb[plane_id];
 
@@ -1581,7 +1579,7 @@  skl_crtc_allocate_plane_ddb(struct intel_atomic_state *state,
 		const struct skl_plane_wm *wm =
 			&crtc_state->wm.skl.optimal.planes[plane_id];
 
-		if (plane_id == PLANE_CURSOR && DISPLAY_VER(i915) < 20)
+		if (plane_id == PLANE_CURSOR)
 			continue;
 
 		if (DISPLAY_VER(i915) < 11 &&