[Intel-gfx] [PATCH 2/4] drm/i915/dsb: Correct DSB command buffer cache coherency settings

[Shankar, Uma]

> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of Ville
> Syrjala
> Sent: Monday, October 9, 2023 6:52 PM
> To: intel-gfx at lists.freedesktop.org
> Subject: [Intel-gfx] [PATCH 2/4] drm/i915/dsb: Correct DSB command buffer
> cache coherency settings
> 
> From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> 
> The display engine does not snoop the caches so shoukd to mark the DSB

Nit: Typo here

I am not sure on the cache behaviour so someone from core can also ack.
To me , looks logically correct.
Reviewed-by: Uma Shankar <uma.shankar at intel.com>

> command buffer as I915_CACHE_NONE.
> i915_gem_object_create_internal() always gives us I915_CACHE_LLC on LLC
> platforms. And to make things 100% correct we should also clflush at the end, if
> necessary.
> 
> Note that currently this is a non-issue as we always write the command buffer
> through a WC mapping, so a cache flush is not actually needed. But we might
> actually want to consider a WB mapping since we also end up reading from the
> command buffer (in the indexed reg write handling). Either that or we should do
> something else to avoid those reads (might actually be even more sensible on
> DGFX since we end up reading over PCIe). But we should measure the overhead
> first...
> 
> Anyways, no real harm in adding the belts and suspenders here so that the code
> will work correctly regardless of how we map the buffer. If we do get a WC
> mapping (as we request)
> i915_gem_object_flush_map() will be a nop. Well, apart form a wmb() which may
> just flush the WC buffer a bit earlier than would otherwise happen (at the latest
> the mmio accesses would trigger the WC flush).
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dsb.c | 15 +++++++++++----
>  1 file changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> b/drivers/gpu/drm/i915/display/intel_dsb.c
> index 7410ba3126f9..78b6fe24dcd8 100644
> --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> @@ -316,6 +316,8 @@ void intel_dsb_finish(struct intel_dsb *dsb)
>  				   DSB_FORCE_DEWAKE, 0);
> 
>  	intel_dsb_align_tail(dsb);
> +
> +	i915_gem_object_flush_map(dsb->vma->obj);
>  }
> 
>  static int intel_dsb_dewake_scanline(const struct intel_crtc_state *crtc_state)
> @@ -462,13 +464,18 @@ struct intel_dsb *intel_dsb_prepare(const struct
> intel_crtc_state *crtc_state,
>  	/* ~1 qword per instruction, full cachelines */
>  	size = ALIGN(max_cmds * 8, CACHELINE_BYTES);
> 
> -	if (HAS_LMEM(i915))
> +	if (HAS_LMEM(i915)) {
>  		obj = i915_gem_object_create_lmem(i915, PAGE_ALIGN(size),
> 
> I915_BO_ALLOC_CONTIGUOUS);
> -	else
> +		if (IS_ERR(obj))
> +			goto out_put_rpm;
> +	} else {
>  		obj = i915_gem_object_create_internal(i915, PAGE_ALIGN(size));
> -	if (IS_ERR(obj))
> -		goto out_put_rpm;
> +		if (IS_ERR(obj))
> +			goto out_put_rpm;
> +
> +		i915_gem_object_set_cache_coherency(obj,
> I915_CACHE_NONE);
> +	}
> 
>  	vma = i915_gem_object_ggtt_pin(obj, NULL, 0, 0, 0);
>  	if (IS_ERR(vma)) {
> --
> 2.41.0