On Thu, Oct 12, 2023 at 09:40:23PM +0000, Shankar, Uma wrote:
>
>
> > -----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>
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@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
>
--
Ville Syrjälä
Intel
