Re: [Intel-gfx] [PATCH 1/3] drm/i915: Set all unused color plane offsets to ~0xfff again

[Imre Deak <imre.deak@intel.com>]

On Thu, Oct 08, 2020 at 01:16:06PM +0300, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> When the number of potential color planes grew to 4 we stopped
> setting all unused color plane offsets to ~0xfff. The code
> still tries to do this, but actually does nothing since the
> loop limits are bogus.
> 
> skl_check_main_surface() actually depends on this ~0xfff
> behaviour as it will make sure to move the main surface
> offset below the aux surface offset because the hardware
> AUX_DIST must be a non-negative value [1], and for simplicity
> it doesn't bother checking if the AUX plane is actually
> needed or not. So currently it may end up shuffling the
> main surface around based on some stale leftover AUX offset.
> 
> The skl+ plane code also just blindly calculates the AUX_DIST
> whether or not the AUX plane is actually needed by the hw or
> not, and that too will now potentially use some stale AUX
> surface offset in the calculation. Would seem nicer to
> guarantee a consistent non-negative AUX_DIST always.
> 
> So bring back the original ~0xfff offset behaviour for
> unused color planes. Though it doesn't seem super likely
> that this inconsistency would cause any real issues.
> 
> Cc: Dhinakaran Pandiyan <dhinakaran.pandiyan@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Radhakrishna Sripada <radhakrishna.sripada@intel.com>
> Fixes: 2dfbf9d2873a ("drm/i915/tgl: Gen-12 display can decompress surfaces compressed by the media engine")
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Arg. Yes skl_check_main_surface() adjusts now the address needlessly.
The fix looks ok to me:

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/display/intel_display.c | 17 +++++------------
>  1 file changed, 5 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index 907e1d155443..44fd7059838f 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -4104,8 +4104,7 @@ static int skl_check_ccs_aux_surface(struct intel_plane_state *plane_state)
>  int skl_check_plane_surface(struct intel_plane_state *plane_state)
>  {
>  	const struct drm_framebuffer *fb = plane_state->hw.fb;
> -	int ret;
> -	bool needs_aux = false;
> +	int ret, i;
>  
>  	ret = intel_plane_compute_gtt(plane_state);
>  	if (ret)
> @@ -4119,7 +4118,6 @@ int skl_check_plane_surface(struct intel_plane_state *plane_state)
>  	 * it.
>  	 */
>  	if (is_ccs_modifier(fb->modifier)) {
> -		needs_aux = true;
>  		ret = skl_check_ccs_aux_surface(plane_state);
>  		if (ret)
>  			return ret;
> @@ -4127,20 +4125,15 @@ int skl_check_plane_surface(struct intel_plane_state *plane_state)
>  
>  	if (intel_format_info_is_yuv_semiplanar(fb->format,
>  						fb->modifier)) {
> -		needs_aux = true;
>  		ret = skl_check_nv12_aux_surface(plane_state);
>  		if (ret)
>  			return ret;
>  	}
>  
> -	if (!needs_aux) {
> -		int i;
> -
> -		for (i = 1; i < fb->format->num_planes; i++) {
> -			plane_state->color_plane[i].offset = ~0xfff;
> -			plane_state->color_plane[i].x = 0;
> -			plane_state->color_plane[i].y = 0;
> -		}
> +	for (i = fb->format->num_planes; i < ARRAY_SIZE(plane_state->color_plane); i++) {
> +		plane_state->color_plane[i].offset = ~0xfff;
> +		plane_state->color_plane[i].x = 0;
> +		plane_state->color_plane[i].y = 0;
>  	}
>  
>  	ret = skl_check_main_surface(plane_state);
> -- 
> 2.26.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx