On Thu, Oct 12, 2023 at 09:40:23PM +0000, Shankar, Uma wrote: > > > > -----Original Message----- > > From: Intel-gfx <[email protected]> On Behalf Of Ville > > Syrjala > > Sent: Monday, October 9, 2023 6:52 PM > > To: [email protected] > > Subject: [Intel-gfx] [PATCH 2/4] drm/i915/dsb: Correct DSB command buffer > > cache coherency settings > > > > From: Ville Syrjälä <[email protected]> > > > > 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 <[email protected]>
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ä <[email protected]> > > --- > > 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
