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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Oct 12, 2023 at 09:40:23PM +0000, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: Monday, October 9, 2023 6:52 PM
> > To: intel-gfx@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@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@intel.com>

Thanks. This series is now merged. Fingers crossed DSB will behave
nicely...

> 
> > 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@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
> 

-- 
Ville Syrjälä
Intel