Re: [Intel-gfx] [PATCH] drm/i915: Read a shadowed mmio register for ggtt flush
Hi Vinay, > -Original Message- > From: dri-devel On Behalf Of > Belgaumkar, Vinay > Sent: Thursday, November 9, 2023 5:02 PM > To: Ville Syrjälä > Cc: intel-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Read a shadowed mmio register for > ggtt flush > > > On 11/9/2023 12:35 PM, Ville Syrjälä wrote: > > On Thu, Nov 09, 2023 at 12:01:26PM -0800, Belgaumkar, Vinay wrote: > >> On 11/9/2023 11:30 AM, Ville Syrjälä wrote: > >>> On Thu, Nov 09, 2023 at 11:21:48AM -0800, Vinay Belgaumkar wrote: > >>>> We read RENDER_HEAD as a part of the flush. If GT is in > >>>> deeper sleep states, this could lead to read errors since we are > >>>> not using a forcewake. Safer to read a shadowed register instead. > >>> IIRC shadowing is only thing for writes, not reads. > >> Sure, but reading from a shadowed register does return the cached value > > Does it? I suppose that would make some sense, but I don't recall that > > ever being stated anywhere. At least before the shadow registers > > existed reads would just give you zeroes when not awake. > > > >> (even though we don't care about the vakue here). When GT is in deeper > >> sleep states, it is better to read a shadowed (cached) value instead of > >> trying to attempt an mmio register read without a force wake anyways. > > So you're saying reads from non-shadowed registers fails somehow > > when not awake? How exactly do they fail? And when reading from > > a shadowed register that failure never happens? > > We could hit problems like the one being addressed here - > https://patchwork.freedesktop.org/series/125356/. Reading from a > shadowed register will avoid any needless references(without a wake) to > the MMIO space. Shouldn't hurt to make this change for all gens IMO. The proposed usage looks accurate for the issue described. Reviewed-by: Radhakrishna Sripada --Radhakrishna(RK) Sripada > > Thanks, > > Vinay. > > > > >> Thanks, > >> > >> Vinay. > >> > >>>> Cc: John Harrison > >>>> Cc: Daniele Ceraolo Spurio > >>>> Signed-off-by: Vinay Belgaumkar > >>>> --- > >>>>drivers/gpu/drm/i915/gt/intel_gt.c | 2 +- > >>>>1 file changed, 1 insertion(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c > b/drivers/gpu/drm/i915/gt/intel_gt.c > >>>> index ed32bf5b1546..ea814ea5f700 100644 > >>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c > >>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c > >>>> @@ -451,7 +451,7 @@ void intel_gt_flush_ggtt_writes(struct intel_gt *gt) > >>>> > >>>> spin_lock_irqsave(>lock, flags); > >>>> intel_uncore_posting_read_fw(uncore, > >>>> - > >>>> RING_HEAD(RENDER_RING_BASE)); > >>>> + > >>>> RING_TAIL(RENDER_RING_BASE)); > >>>> spin_unlock_irqrestore(>lock, flags); > >>>> } > >>>>} > >>>> -- > >>>> 2.38.1
Re: [Intel-gfx] [PATCH] drm/i915: Read a shadowed mmio register for ggtt flush
On 11/9/2023 12:35 PM, Ville Syrjälä wrote: On Thu, Nov 09, 2023 at 12:01:26PM -0800, Belgaumkar, Vinay wrote: On 11/9/2023 11:30 AM, Ville Syrjälä wrote: On Thu, Nov 09, 2023 at 11:21:48AM -0800, Vinay Belgaumkar wrote: We read RENDER_HEAD as a part of the flush. If GT is in deeper sleep states, this could lead to read errors since we are not using a forcewake. Safer to read a shadowed register instead. IIRC shadowing is only thing for writes, not reads. Sure, but reading from a shadowed register does return the cached value Does it? I suppose that would make some sense, but I don't recall that ever being stated anywhere. At least before the shadow registers existed reads would just give you zeroes when not awake. (even though we don't care about the vakue here). When GT is in deeper sleep states, it is better to read a shadowed (cached) value instead of trying to attempt an mmio register read without a force wake anyways. So you're saying reads from non-shadowed registers fails somehow when not awake? How exactly do they fail? And when reading from a shadowed register that failure never happens? We could hit problems like the one being addressed here - https://patchwork.freedesktop.org/series/125356/. Reading from a shadowed register will avoid any needless references(without a wake) to the MMIO space. Shouldn't hurt to make this change for all gens IMO. Thanks, Vinay. Thanks, Vinay. Cc: John Harrison Cc: Daniele Ceraolo Spurio Signed-off-by: Vinay Belgaumkar --- drivers/gpu/drm/i915/gt/intel_gt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index ed32bf5b1546..ea814ea5f700 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -451,7 +451,7 @@ void intel_gt_flush_ggtt_writes(struct intel_gt *gt) spin_lock_irqsave(>lock, flags); intel_uncore_posting_read_fw(uncore, -RING_HEAD(RENDER_RING_BASE)); +RING_TAIL(RENDER_RING_BASE)); spin_unlock_irqrestore(>lock, flags); } } -- 2.38.1
Re: [Intel-gfx] [PATCH] drm/i915: Read a shadowed mmio register for ggtt flush
On Thu, Nov 09, 2023 at 12:01:26PM -0800, Belgaumkar, Vinay wrote: > > On 11/9/2023 11:30 AM, Ville Syrjälä wrote: > > On Thu, Nov 09, 2023 at 11:21:48AM -0800, Vinay Belgaumkar wrote: > >> We read RENDER_HEAD as a part of the flush. If GT is in > >> deeper sleep states, this could lead to read errors since we are > >> not using a forcewake. Safer to read a shadowed register instead. > > IIRC shadowing is only thing for writes, not reads. > > Sure, but reading from a shadowed register does return the cached value Does it? I suppose that would make some sense, but I don't recall that ever being stated anywhere. At least before the shadow registers existed reads would just give you zeroes when not awake. > (even though we don't care about the vakue here). When GT is in deeper > sleep states, it is better to read a shadowed (cached) value instead of > trying to attempt an mmio register read without a force wake anyways. So you're saying reads from non-shadowed registers fails somehow when not awake? How exactly do they fail? And when reading from a shadowed register that failure never happens? > > Thanks, > > Vinay. > > > > >> Cc: John Harrison > >> Cc: Daniele Ceraolo Spurio > >> Signed-off-by: Vinay Belgaumkar > >> --- > >> drivers/gpu/drm/i915/gt/intel_gt.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c > >> b/drivers/gpu/drm/i915/gt/intel_gt.c > >> index ed32bf5b1546..ea814ea5f700 100644 > >> --- a/drivers/gpu/drm/i915/gt/intel_gt.c > >> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c > >> @@ -451,7 +451,7 @@ void intel_gt_flush_ggtt_writes(struct intel_gt *gt) > >> > >>spin_lock_irqsave(>lock, flags); > >>intel_uncore_posting_read_fw(uncore, > >> - RING_HEAD(RENDER_RING_BASE)); > >> + RING_TAIL(RENDER_RING_BASE)); > >>spin_unlock_irqrestore(>lock, flags); > >>} > >> } > >> -- > >> 2.38.1 -- Ville Syrjälä Intel
Re: [Intel-gfx] [PATCH] drm/i915: Read a shadowed mmio register for ggtt flush
On 11/9/2023 11:30 AM, Ville Syrjälä wrote: On Thu, Nov 09, 2023 at 11:21:48AM -0800, Vinay Belgaumkar wrote: We read RENDER_HEAD as a part of the flush. If GT is in deeper sleep states, this could lead to read errors since we are not using a forcewake. Safer to read a shadowed register instead. IIRC shadowing is only thing for writes, not reads. Sure, but reading from a shadowed register does return the cached value (even though we don't care about the vakue here). When GT is in deeper sleep states, it is better to read a shadowed (cached) value instead of trying to attempt an mmio register read without a force wake anyways. Thanks, Vinay. Cc: John Harrison Cc: Daniele Ceraolo Spurio Signed-off-by: Vinay Belgaumkar --- drivers/gpu/drm/i915/gt/intel_gt.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c index ed32bf5b1546..ea814ea5f700 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt.c +++ b/drivers/gpu/drm/i915/gt/intel_gt.c @@ -451,7 +451,7 @@ void intel_gt_flush_ggtt_writes(struct intel_gt *gt) spin_lock_irqsave(>lock, flags); intel_uncore_posting_read_fw(uncore, -RING_HEAD(RENDER_RING_BASE)); +RING_TAIL(RENDER_RING_BASE)); spin_unlock_irqrestore(>lock, flags); } } -- 2.38.1
Re: [Intel-gfx] [PATCH] drm/i915: Read a shadowed mmio register for ggtt flush
On Thu, Nov 09, 2023 at 11:21:48AM -0800, Vinay Belgaumkar wrote: > We read RENDER_HEAD as a part of the flush. If GT is in > deeper sleep states, this could lead to read errors since we are > not using a forcewake. Safer to read a shadowed register instead. IIRC shadowing is only thing for writes, not reads. > > Cc: John Harrison > Cc: Daniele Ceraolo Spurio > Signed-off-by: Vinay Belgaumkar > --- > drivers/gpu/drm/i915/gt/intel_gt.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c > b/drivers/gpu/drm/i915/gt/intel_gt.c > index ed32bf5b1546..ea814ea5f700 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt.c > @@ -451,7 +451,7 @@ void intel_gt_flush_ggtt_writes(struct intel_gt *gt) > > spin_lock_irqsave(>lock, flags); > intel_uncore_posting_read_fw(uncore, > - RING_HEAD(RENDER_RING_BASE)); > + RING_TAIL(RENDER_RING_BASE)); > spin_unlock_irqrestore(>lock, flags); > } > } > -- > 2.38.1 -- Ville Syrjälä Intel
