> -----Original Message-----
> From: Intel-gfx <intel-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx> On Behalf Of Ville
> Syrjala
> Sent: Monday, October 9, 2023 6:52 PM
> To: intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> Subject: [PATCH 2/4] drm/i915/dsb: Correct DSB command buffer
> cache coherency settings
>
> From: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxx>
> 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@xxxxxxxxxxxxxxx>
> ---
> 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
