Re: [PATCH 1/6] drm/i915: Reorder gen3/4 swizzle detection logic

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 18/03/2019 16:56, Ville Syrjala wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> g33/i964g/g45 are the exceptional cases when it comes to
> the swizzle detectiong. Let's reorder the code to handle
> them first and let everything else be handled by the
> else branch. This allows us to unset .is_mobile for the
> desktop PNV variant (which supposedly must follow the
> "mobile" path here).
> 
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem_fence_reg.c | 65 +++++++++++------------
>   1 file changed, 32 insertions(+), 33 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_fence_reg.c b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> index 65624b8e4d15..9418ad499b7e 100644
> --- a/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> +++ b/drivers/gpu/drm/i915/i915_gem_fence_reg.c
> @@ -585,8 +585,38 @@ i915_gem_detect_bit_6_swizzle(struct drm_i915_private *dev_priv)
>   		 */
>   		swizzle_x = I915_BIT_6_SWIZZLE_NONE;
>   		swizzle_y = I915_BIT_6_SWIZZLE_NONE;
> -	} else if (IS_MOBILE(dev_priv) ||
> -		   IS_I915G(dev_priv) || IS_I945G(dev_priv)) {
> +	} else if (IS_G45(dev_priv) || IS_I965G(dev_priv) || IS_G33(dev_priv)) {
> +		/* The 965, G33, and newer, have a very flexible memory
> +		 * configuration.  It will enable dual-channel mode
> +		 * (interleaving) on as much memory as it can, and the GPU
> +		 * will additionally sometimes enable different bit 6
> +		 * swizzling for tiled objects from the CPU.
> +		 *
> +		 * Here's what I found on the G965:
> +		 *    slot fill         memory size  swizzling
> +		 * 0A   0B   1A   1B    1-ch   2-ch
> +		 * 512  0    0    0     512    0     O
> +		 * 512  0    512  0     16     1008  X
> +		 * 512  0    0    512   16     1008  X
> +		 * 0    512  0    512   16     1008  X
> +		 * 1024 1024 1024 0     2048   1024  O
> +		 *
> +		 * We could probably detect this based on either the DRB
> +		 * matching, which was the case for the swizzling required in
> +		 * the table above, or from the 1-ch value being less than
> +		 * the minimum size of a rank.
> +		 *
> +		 * Reports indicate that the swizzling actually
> +		 * varies depending upon page placement inside the
> +		 * channels, i.e. we see swizzled pages where the
> +		 * banks of memory are paired and unswizzled on the
> +		 * uneven portion, so leave that as unknown.
> +		 */
> +		if (I915_READ16(C0DRB3) == I915_READ16(C1DRB3)) {
> +			swizzle_x = I915_BIT_6_SWIZZLE_9_10;
> +			swizzle_y = I915_BIT_6_SWIZZLE_9;
> +		}
> +	} else {
>   		u32 dcc;
>   
>   		/* On 9xx chipsets, channel interleave by the CPU is
> @@ -636,37 +666,6 @@ i915_gem_detect_bit_6_swizzle(struct drm_i915_private *dev_priv)
>   			swizzle_x = I915_BIT_6_SWIZZLE_UNKNOWN;
>   			swizzle_y = I915_BIT_6_SWIZZLE_UNKNOWN;
>   		}
> -	} else {
> -		/* The 965, G33, and newer, have a very flexible memory
> -		 * configuration.  It will enable dual-channel mode
> -		 * (interleaving) on as much memory as it can, and the GPU
> -		 * will additionally sometimes enable different bit 6
> -		 * swizzling for tiled objects from the CPU.
> -		 *
> -		 * Here's what I found on the G965:
> -		 *    slot fill         memory size  swizzling
> -		 * 0A   0B   1A   1B    1-ch   2-ch
> -		 * 512  0    0    0     512    0     O
> -		 * 512  0    512  0     16     1008  X
> -		 * 512  0    0    512   16     1008  X
> -		 * 0    512  0    512   16     1008  X
> -		 * 1024 1024 1024 0     2048   1024  O
> -		 *
> -		 * We could probably detect this based on either the DRB
> -		 * matching, which was the case for the swizzling required in
> -		 * the table above, or from the 1-ch value being less than
> -		 * the minimum size of a rank.
> -		 *
> -		 * Reports indicate that the swizzling actually
> -		 * varies depending upon page placement inside the
> -		 * channels, i.e. we see swizzled pages where the
> -		 * banks of memory are paired and unswizzled on the
> -		 * uneven portion, so leave that as unknown.
> -		 */
> -		if (I915_READ16(C0DRB3) == I915_READ16(C1DRB3)) {
> -			swizzle_x = I915_BIT_6_SWIZZLE_9_10;
> -			swizzle_y = I915_BIT_6_SWIZZLE_9;
> -		}
>   	}
>   
>   	if (swizzle_x == I915_BIT_6_SWIZZLE_UNKNOWN ||
> 

Old/new/future behaviour checks out as far as I can see.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx