Re: [PATCH v3 20/21] drm/i915/display: perform transient flush

[Matthew Auld <matthew.auld@intel.com>]

On 15/04/2024 19:14, Matt Roper wrote:
> On Mon, Apr 15, 2024 at 10:07:32AM -0700, Matt Roper wrote:
>> On Mon, Apr 15, 2024 at 01:44:22PM +0530, Balasubramani Vivekanandan wrote:
>>> From: Matthew Auld <matthew.auld@intel.com>
>>>
>>> Perform manual transient cache flush prior to flip and at the end of
>>> frontbuffer_flush. This is needed to ensure display engine doesn't see
>>> garbage if the surface is L3:XD dirty.
>>>
>>> Testcase: igt@xe-pat@display-vs-wb-transient
>>
>> Has the IGT patch for this been sent yet?  If not, we should probably
>> make sure that happens soon, and then use the CI Test-with: thing if
>> there winds up being another revision of this series so that this will
>> be included in the CI results.
> 
> Oh, it looks like this test already landed back in early March; I just
> didn't look back far enough in the git history originally.  You can
> ignore this comment.

Yeah, my thinking was to upstream the IGT bits early for LNL, since it 
is still applicable there. Only difference is we are verifying we don't 
need the TDF on that platform, whereas on BMG the test will only pass 
with this patch.

> 
> 
> Matt
> 
>>
>> Anyway, the changes here look good to me,
>>
>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com>
>>
>>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>>> Signed-off-by: Balasubramani Vivekanandan <balasubramani.vivekanandan@intel.com>
>>> Acked-by: Nirmoy Das <nirmoy.das@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_display.c  |  3 +++
>>>   .../gpu/drm/i915/display/intel_frontbuffer.c  |  2 ++
>>>   drivers/gpu/drm/i915/display/intel_tdf.h      | 25 +++++++++++++++++++
>>>   drivers/gpu/drm/xe/Makefile                   |  3 ++-
>>>   drivers/gpu/drm/xe/display/xe_tdf.c           | 13 ++++++++++
>>>   5 files changed, 45 insertions(+), 1 deletion(-)
>>>   create mode 100644 drivers/gpu/drm/i915/display/intel_tdf.h
>>>   create mode 100644 drivers/gpu/drm/xe/display/xe_tdf.c
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>> index 67697d9a559c..4fc46edcb4ad 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -110,6 +110,7 @@
>>>   #include "intel_sdvo.h"
>>>   #include "intel_snps_phy.h"
>>>   #include "intel_tc.h"
>>> +#include "intel_tdf.h"
>>>   #include "intel_tv.h"
>>>   #include "intel_vblank.h"
>>>   #include "intel_vdsc.h"
>>> @@ -7242,6 +7243,8 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state)
>>>   
>>>   	intel_atomic_commit_fence_wait(state);
>>>   
>>> +	intel_td_flush(dev_priv);
>>> +
>>>   	drm_atomic_helper_wait_for_dependencies(&state->base);
>>>   	drm_dp_mst_atomic_wait_for_dependencies(&state->base);
>>>   	intel_atomic_global_state_wait_for_dependencies(state);
>>> diff --git a/drivers/gpu/drm/i915/display/intel_frontbuffer.c b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>>> index 2ea37c0414a9..4923c340a0b6 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_frontbuffer.c
>>> @@ -65,6 +65,7 @@
>>>   #include "intel_fbc.h"
>>>   #include "intel_frontbuffer.h"
>>>   #include "intel_psr.h"
>>> +#include "intel_tdf.h"
>>>   
>>>   /**
>>>    * frontbuffer_flush - flush frontbuffer
>>> @@ -93,6 +94,7 @@ static void frontbuffer_flush(struct drm_i915_private *i915,
>>>   	trace_intel_frontbuffer_flush(i915, frontbuffer_bits, origin);
>>>   
>>>   	might_sleep();
>>> +	intel_td_flush(i915);
>>>   	intel_drrs_flush(i915, frontbuffer_bits);
>>>   	intel_psr_flush(i915, frontbuffer_bits, origin);
>>>   	intel_fbc_flush(i915, frontbuffer_bits, origin);
>>> diff --git a/drivers/gpu/drm/i915/display/intel_tdf.h b/drivers/gpu/drm/i915/display/intel_tdf.h
>>> new file mode 100644
>>> index 000000000000..353cde21f6c2
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/i915/display/intel_tdf.h
>>> @@ -0,0 +1,25 @@
>>> +/* SPDX-License-Identifier: MIT */
>>> +/*
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +
>>> +#ifndef __INTEL_TDF_H__
>>> +#define __INTEL_TDF_H__
>>> +
>>> +/*
>>> + * TDF (Transient-Data-Flush) is needed for Xe2+ where special L3:XD caching can
>>> + * be enabled through various PAT index modes. Idea is to use this caching mode
>>> + * when for example rendering onto the display surface, with the promise that
>>> + * KMD will ensure transient cache entries are always flushed by the time we do
>>> + * the display flip, since display engine is never coherent with CPU/GPU caches.
>>> + */
>>> +
>>> +struct drm_i915_private;
>>> +
>>> +#ifdef I915
>>> +static inline void intel_td_flush(struct drm_i915_private *i915) {}
>>> +#else
>>> +void intel_td_flush(struct drm_i915_private *i915);
>>> +#endif
>>> +
>>> +#endif
>>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>>> index 6015c9e41f24..97a8674cdd76 100644
>>> --- a/drivers/gpu/drm/xe/Makefile
>>> +++ b/drivers/gpu/drm/xe/Makefile
>>> @@ -198,7 +198,8 @@ xe-$(CONFIG_DRM_XE_DISPLAY) += \
>>>   	display/xe_dsb_buffer.o \
>>>   	display/xe_fb_pin.o \
>>>   	display/xe_hdcp_gsc.o \
>>> -	display/xe_plane_initial.o
>>> +	display/xe_plane_initial.o \
>>> +	display/xe_tdf.o
>>>   
>>>   # SOC code shared with i915
>>>   xe-$(CONFIG_DRM_XE_DISPLAY) += \
>>> diff --git a/drivers/gpu/drm/xe/display/xe_tdf.c b/drivers/gpu/drm/xe/display/xe_tdf.c
>>> new file mode 100644
>>> index 000000000000..2c0d4e144e09
>>> --- /dev/null
>>> +++ b/drivers/gpu/drm/xe/display/xe_tdf.c
>>> @@ -0,0 +1,13 @@
>>> +// SPDX-License-Identifier: MIT
>>> +/*
>>> + * Copyright © 2024 Intel Corporation
>>> + */
>>> +
>>> +#include "xe_device.h"
>>> +#include "intel_display_types.h"
>>> +#include "intel_tdf.h"
>>> +
>>> +void intel_td_flush(struct drm_i915_private *i915)
>>> +{
>>> +	xe_device_td_flush(i915);
>>> +}
>>> -- 
>>> 2.25.1
>>>
>>
>> -- 
>> Matt Roper
>> Graphics Software Engineer
>> Linux GPU Platform Enablement
>> Intel Corporation
>