RE: [Intel-gfx] [PATCH] drm/i915: Read a shadowed mmio register for ggtt flush

[Sripada, Radhakrishna]

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-...@lists.freedesktop.org; dri-devel@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(&uncore->lock, flags);
> >>>>  intel_uncore_posting_read_fw(uncore,
> >>>> - 
> >>>> RING_HEAD(RENDER_RING_BASE));
> >>>> + 
> >>>> RING_TAIL(RENDER_RING_BASE));
> >>>>  spin_unlock_irqrestore(&uncore->lock, flags);
> >>>>  }
> >>>>}
> >>>> --
> >>>> 2.38.1

RE: [PATCH] drm/i915: Zap some empty lines

[Sripada, Radhakrishna]

> -Original Message-
> From: dri-devel  On Behalf Of Tvrtko
> Ursulin
> Sent: Wednesday, September 20, 2023 2:27 PM
> To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Srivatsa, Anusha ; Bhadane, Dnyaneshwar
> ; Sripada, Radhakrishna
> ; Ursulin, Tvrtko 
> Subject: [PATCH] drm/i915: Zap some empty lines
> 
> From: Tvrtko Ursulin 
> 
> Recent refactoring left an unsightly block of empty lines. Remove them.
> 
> Signed-off-by: Tvrtko Ursulin 
> Cc: Dnyaneshwar Bhadane 
> Cc: Anusha Srivatsa 
> Cc: Radhakrishna Sripada 

LGTM,
Reviewed-by: Radhakrishna Sripada 

> ---
>  drivers/gpu/drm/i915/i915_drv.h | 7 ---
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 87ffc477c3b1..511eba3bbdba 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -646,13 +646,6 @@ IS_SUBPLATFORM(const struct drm_i915_private *i915,
>  #define IS_TIGERLAKE_UY(i915) \
>   IS_SUBPLATFORM(i915, INTEL_TIGERLAKE, INTEL_SUBPLATFORM_UY)
> 
> -
> -
> -
> -
> -
> -
> -
>  #define IS_XEHPSDV_GRAPHICS_STEP(__i915, since, until) \
>   (IS_XEHPSDV(__i915) && IS_GRAPHICS_STEP(__i915, since, until))
> 
> --
> 2.39.2

RE: [Intel-gfx] [PATCH] drm/i915/mtl: Drop force_probe requirement

[Sripada, Radhakrishna]

Hi Rodrigo/Andi,

> -Original Message-
> From: Vivi, Rodrigo 
> Sent: Wednesday, September 6, 2023 7:38 PM
> To: Andi Shyti 
> Cc: Sripada, Radhakrishna ; intel-
> g...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915/mtl: Drop force_probe requirement
> 
> On Wed, Sep 06, 2023 at 11:25:35AM +0200, Andi Shyti wrote:
> > Hi Radhakrishna,
> >
> > On Tue, Sep 05, 2023 at 12:36:24PM -0700, Radhakrishna Sripada wrote:
> > > Meteorlake has been very usable for a while now, all of uapi changes
> > > related to fundamental platform usage have been finalized and all
> > > required firmware blobs are available. Recent CI results have also
> > > been healthy, so we're ready to drop the force_probe requirement and
> > > enable the platform by default.
> > >
> > > Cc: Rodrigo Vivi 
> > > Cc: Tvrtko Ursulin 
> > > Cc: Joonas Lahtinen 
> > > Cc: Jani Nikula 
> > > Signed-off-by: Radhakrishna Sripada 
> >
> > Please keep me in the loop as well... It's been a year I've been
> > working for this patch to work :)
Sure will do.

> >
> > > ---
> > >  drivers/gpu/drm/i915/i915_pci.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> b/drivers/gpu/drm/i915/i915_pci.c
> > > index df7c261410f7..fe748906c06f 100644
> > > --- a/drivers/gpu/drm/i915/i915_pci.c
> > > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > > @@ -836,7 +836,6 @@ static const struct intel_device_info mtl_info = {
> > >   .has_pxp = 1,
> > >   .memory_regions = REGION_SMEM | REGION_STOLEN_LMEM,
> > >   .platform_engine_mask = BIT(RCS0) | BIT(BCS0) | BIT(CCS0),
> > > - .require_force_probe = 1,
> >
> > What's the thinking behind this patch? Are you trying to
> > understand how CI behaves?
> 
> CI uses kernel config to force_probe. MTL is already being tested there.
> Also there's no 'CI' or 'HAX' tag on this patch.
> So I would assume this is the ask to remove the protection.
> But based on this question from Andi and knowing that he is working on
> the MTL w/a I'm assuming that this is not the right time yet to remove
> this protection.
> 
> Please ensure all the diligence is taken before sending this patch again.
> 
> Also ensure that the current CI failures are fixed (gt_pm and gt_engines)
> and that CI has a stable green picture.

Sure Rodrigo. I believe the changes in 
https://patchwork.freedesktop.org/series/123329/
are significant enough to not remove force_probe at this time.

Will wait for a later time till CI comes clean.

Thanks,
Radhakrishna
> 
> Thanks,
> Rodrigo.
> 
> >
> > Andi
> >
> > >   MTL_CACHELEVEL,
> > >  };
> > >
> > > --
> > > 2.34.1

RE: [PATCH] drm/i915: Avoid GGTT flushing on non-GGTT paths of i915_vma_pin_iomap

[Sripada, Radhakrishna]

Hi Tvrtko,

The changes makes sense and based on the description looks good.
I am bit skeptical about the exec buffer failure reported by ci hence,
withholding the r-b for now. If you believe the CI failure is unrelated
please feel free to add my r-b.

On a side note on platforms with non-coherent ggtt do we really
need to use the barriers twice under intel_gt_flush_ggtt_writes?

--Radhakrishna(RK) Sripada 

> -Original Message-
> From: Tvrtko Ursulin 
> Sent: Monday, July 24, 2023 5:57 AM
> To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Ursulin, Tvrtko ; Sripada, Radhakrishna
> ; sta...@vger.kernel.org
> Subject: [PATCH] drm/i915: Avoid GGTT flushing on non-GGTT paths of
> i915_vma_pin_iomap
> 
> From: Tvrtko Ursulin 
> 
> Commit 4bc91dbde0da ("drm/i915/lmem: Bypass aperture when lmem is
> available")
> added a code path which does not map via GGTT, but was still setting the
> ggtt write bit, and so triggering the GGTT flushing.
> 
> Fix it by not setting that bit unless the GGTT mapping path was used, and
> replace the flush with wmb() in i915_vma_flush_writes().
> 
> This also works for the i915_gem_object_pin_map path added in
> d976521a995a ("drm/i915: extend i915_vma_pin_iomap()").
> 
> It is hard to say if the fix has any observable effect, given that the
> write-combine buffer gets flushed from intel_gt_flush_ggtt_writes too, but
> apart from code clarity, skipping the needless GGTT flushing could be
> beneficial on platforms with non-coherent GGTT. (See the code flow in
> intel_gt_flush_ggtt_writes().)
> 
> Signed-off-by: Tvrtko Ursulin 
> Fixes: 4bc91dbde0da ("drm/i915/lmem: Bypass aperture when lmem is
> available")
> References: d976521a995a ("drm/i915: extend i915_vma_pin_iomap()")
> Cc: Radhakrishna Sripada 
> Cc:  # v5.14+
> ---
>  drivers/gpu/drm/i915/i915_vma.c | 6 +-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_vma.c
> b/drivers/gpu/drm/i915/i915_vma.c
> index ffb425ba591c..f2b626cd2755 100644
> --- a/drivers/gpu/drm/i915/i915_vma.c
> +++ b/drivers/gpu/drm/i915/i915_vma.c
> @@ -602,7 +602,9 @@ void __iomem *i915_vma_pin_iomap(struct i915_vma
> *vma)
>   if (err)
>   goto err_unpin;
> 
> - i915_vma_set_ggtt_write(vma);
> + if (!i915_gem_object_is_lmem(vma->obj) &&
> + i915_vma_is_map_and_fenceable(vma))
> + i915_vma_set_ggtt_write(vma);
> 
>   /* NB Access through the GTT requires the device to be awake. */
>   return page_mask_bits(ptr);
> @@ -617,6 +619,8 @@ void i915_vma_flush_writes(struct i915_vma *vma)
>  {
>   if (i915_vma_unset_ggtt_write(vma))
>   intel_gt_flush_ggtt_writes(vma->vm->gt);
> + else
> + wmb(); /* Just flush the write-combine buffer. */
>  }
> 
>  void i915_vma_unpin_iomap(struct i915_vma *vma)
> --
> 2.39.2

RE: [PATCH] drm/i915: Use the i915_vma_flush_writes helper

[Sripada, Radhakrishna]

> -Original Message-
> From: dri-devel  On Behalf Of Tvrtko
> Ursulin
> Sent: Friday, July 21, 2023 6:08 AM
> To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Ursulin, Tvrtko 
> Subject: [PATCH] drm/i915: Use the i915_vma_flush_writes helper
> 
> From: Tvrtko Ursulin 
> 
> We can use the existing helper in flush_write_domain() and save some lines
> of code.
> 
LGTM,
Radhakrishna Sripada 

--Radhakrishna(RK) Sripada
> Signed-off-by: Tvrtko Ursulin 
> ---
>  drivers/gpu/drm/i915/gem/i915_gem_domain.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> index dfaaa8b66ac3..ffddec1d2a76 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_domain.c
> @@ -68,10 +68,8 @@ flush_write_domain(struct drm_i915_gem_object *obj,
> unsigned int flush_domains)
>   switch (obj->write_domain) {
>   case I915_GEM_DOMAIN_GTT:
>   spin_lock(&obj->vma.lock);
> - for_each_ggtt_vma(vma, obj) {
> - if (i915_vma_unset_ggtt_write(vma))
> - intel_gt_flush_ggtt_writes(vma->vm->gt);
> - }
> + for_each_ggtt_vma(vma, obj)
> + i915_vma_flush_writes(vma);
>   spin_unlock(&obj->vma.lock);
> 
>   i915_gem_object_flush_frontbuffer(obj, ORIGIN_CPU);
> --
> 2.39.2

RE: [PATCH v11 0/8] drm/i915/pxp: Add MTL PXP Support

[Sripada, Radhakrishna]

Thank you for the patches and the reviews. Pushed.

- Radhakrishna(RK) Sripada

> -Original Message-
> From: dri-devel  On Behalf Of Alan
> Previn
> Sent: Thursday, May 11, 2023 4:18 PM
> To: intel-...@lists.freedesktop.org
> Cc: Teres Alexis, Alan Previn ; Ursulin,
> Tvrtko ; Juston Li ; dri-
> de...@lists.freedesktop.org; Ceraolo Spurio, Daniele
> ; Landwerlin, Lionel G
> ; Vivi, Rodrigo ;
> Justen, Jordan L 
> Subject: [PATCH v11 0/8] drm/i915/pxp: Add MTL PXP Support
> 
> This series enables PXP on MTL. On ADL/TGL platforms, we rely on
> the mei driver via the i915-mei PXP component interface to establish
> a connection to the security firmware via the HECI device interface.
> That interface is used to create and teardown the PXP ARB session.
> PXP ARB session is created when protected contexts are created.
> 
> In this series, the front end behaviors and interfaces (uapi) remain
> the same. We add backend support for MTL but with MTL we directly use
> the GSC-CS engine on the MTL GPU device to send messages to the PXP
> (a.k.a. GSC a.k.a graphics-security) firmware. With MTL, the format
> of the message is slightly different with a 2-layer packetization
> that is explained in detail in Patch #3. Also, the second layer
> which is the actual PXP firmware packet is now rev'd to version 4.3
> for MTL that is defined in Patch #5.
> 
> Take note that Patch #4 adds the buffer allocation and gsccs-send-
> message without actually being called by the arb-creation code
> which gets added in Patch #5. Additionally, a seperate series being
> reviewed is introducing a change for session teardown (in pxp front-
> end layer that will need backend support by both legacy and gsccs).
> If we squash all of these together (buffer-alloc, send-message,
> arb-creation and, in future, session-termination), the single patch
> will be rather large. That said, we are keeping Patch #4 and #5
> separate for now, but at merge time, we can squash them together
> if maintainer requires it.
> 
> Changes from prior revs:
>v1 : - fixed when building with CONFIG_PXP disabled.
> - more alignment with gsc_mtl_header structure from the HDCP
>v2 : - (all following changes as per reviews from Daniele)
> - squashed Patch #1 from v1 into the next one.
> - replaced unnecessary "uses_gsccs" boolean in the pxp
>   with "HAS_ENGINE(pxp->ctrl_gt, GSC0)".
> - moved the stashing of gsccs resources from a dynamically
>   allocated opaque handle to an explicit sub-struct in
>   'struct intel_pxp'.
> - moved the buffer object allocations from Patch #1 of this
>   series to Patch #5 (but keep the context allocation in
>   Patch #1).
> - used the kernel default ppgtt for the gsccs context.
> - optimized the buffer allocation and deallocation code
>   and drop the need to stash the drm_i915_gem_object.
> - use a macro with the right mmio reg base (depending
>   on root-tile vs media-tile) along with common relative
>   offset to access all KCR registers thus minimizing
>   changes to the KCR register access codes.
> - fixed bugs in the heci packet request submission code
>   in Patch #3 (of this series)
> - add comments in the mtl-gsc-heci-header regarding the
>   host-session-handle.
> - re-use tee-mutex instead of introducing a gsccs specific
>   cmd mutex.
> - minor cosmetic improvements in Patch #5.
>   - before creating arb session, ensure intel_pxp_start
>   first ensures the GSC FW is up and running.
> - use 2 second timeout for the pending-bit scenario when
>   sending command to GSC-FW as per specs.
> - simplify intel_pxp_get_irq_gt with addition comments
> - redo Patch #7 to minimize the changes without introducing
>   a common  abstraction helper for suspend/resume/init/fini
>   codes that have to change the kcr power state.
>v3 : - rebase onto latest drm-tip with the updated firmware
>   session invalidation flow
> - on Patch#1: move 'mutex_init(&pxp->tee_mutex)' to a common
>   init place in intel_pxp_init since its needed everywhere
>   (Daniele)
> - on Patch#1: remove unneccasary "ce->vm = i915_vm_get(vm);"
>   (Daniele)
> - on Patch#2: move the introduction of host_session_handle to
>   Patch#4 where it starts getting used.
> - on Patch#4: move host-session-handle initialization to the
>   allocate-resources during gsccs-init (away from Patch#5)
>   and add the required call to PXP-firmware to cleanup the
>   host-session-handle in destroy-resources during gsccs-fini
> - on Patch#5: add dispatching of arb session termination
>   firmware cmd during session teardown (as per latest
>   upstream flows)
>v4 : - Added proper initialization and cleanup of ho

RE: [PATCH v3] drm/i915/mtl: Disable stolen memory backed FB for A0

[Sripada, Radhakrishna]

> -Original Message-
> From: Das, Nirmoy 
> Sent: Tuesday, April 4, 2023 11:14 AM
> To: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; Das, Nirmoy ;
> Auld, Matthew ; Andi Shyti
> ; Ceraolo Spurio, Daniele
> ; De Marchi, Lucas
> ; Sripada, Radhakrishna
> 
> Subject: [PATCH v3] drm/i915/mtl: Disable stolen memory backed FB for A0
> 
> Stolen memory is not usable for MTL A0 stepping beyond
> certain access size and we have no control over userspace
> access size of /dev/fb which can be backed by stolen memory.
> So disable stolen memory backed fb by setting i915->dsm.usable_size
> to zero.
> 
> v2: remove hsdes reference and fix commit message(Andi)
> v3: use revid as we want to target SOC stepping(Radhakrishna)
> 
> Cc: Matthew Auld 
> Cc: Andi Shyti 
> Cc: Daniele Ceraolo Spurio 
> Cc: Lucas De Marchi 
> Cc: Radhakrishna Sripada 
> Signed-off-by: Nirmoy Das 
> Reviewed-by: Andi Shyti 


LGTM,
Reviewed-by: Radhakrishna Sripada 

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_stolen.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> index 8ac376c24aa2..ee492d823f1b 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_stolen.c
> @@ -535,6 +535,14 @@ static int i915_gem_init_stolen(struct
> intel_memory_region *mem)
>   /* Basic memrange allocator for stolen space. */
>   drm_mm_init(&i915->mm.stolen, 0, i915->dsm.usable_size);
> 
> + /*
> +  * Access to stolen lmem beyond certain size for MTL A0 stepping
> +  * would crash the machine. Disable stolen lmem for userspace access
> +  * by setting usable_size to zero.
> +  */
> + if (IS_METEORLAKE(i915) && INTEL_REVID(i915) == 0x0)
> + i915->dsm.usable_size = 0;
> +
>   return 0;
>  }
> 
> --
> 2.39.0

RE: [PATCH v2 1/2] drm/i915/gt: Create per-tile debugfs files

[Sripada, Radhakrishna]

> -Original Message-
> From: Andi Shyti 
> Sent: Friday, March 3, 2023 7:24 PM
> To: Sripada, Radhakrishna 
> Cc: Andi Shyti ; intel-...@lists.freedesktop.org; 
> dri-
> de...@lists.freedesktop.org; Tvrtko Ursulin ;
> Andi Shyti ; Patelczyk, Maciej
> ; Wajdeczko, Michal
> 
> Subject: Re: [PATCH v2 1/2] drm/i915/gt: Create per-tile debugfs files
> 
> On Wed, Mar 01, 2023 at 09:35:33PM +, Sripada, Radhakrishna wrote:
> > I am not sure if Tiles is appropriate usage here. Since MTL does not have 
> > the
> concept of tiles.
> > Shouldn't we be using gt instead of tile in our usage?
> >
> > With s/tile/gt/g,
> > Reviewed-by: Radhakrishna Sripada 
> 
> Just one question... you reviewed twice Patch number 1. Did you
> mean to review patch 1 and patch 2?

This was for Patch1 itself. I did not include s/tile/gt/g during the first time 
I gave r-b
hence added that with new r-b.

-RK
> 
> Thanks,
> Andi
> 
> >
> > > -Original Message-
> > > From: dri-devel  On Behalf Of
> Andi
> > > Shyti
> > > Sent: Wednesday, March 1, 2023 3:03 AM
> > > To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> > > Cc: Tvrtko Ursulin ; Andi Shyti
> > > ; Patelczyk, Maciej ; Andi
> > > Shyti ; Wajdeczko, Michal
> > > 
> > > Subject: [PATCH v2 1/2] drm/i915/gt: Create per-tile debugfs files
> > >
> > > To support multi-GT configurations, we need to generate
> > > independent debug files for each GT.
> > >
> > > To achieve this create a separate directory for each GT under the
> > > debugfs directory. For instance, in a system with two tiles, the
> > > debugfs structure would look like this:
> > >
> > > /sys/kernel/debug/dri
> > >   └── 0
> > >   ├── gt0
> > >   │   ├── drpc
> > >   │   ├── engines
> > >   │   ├── forcewake
> > >   │   ├── frequency
> > >   │   └── rps_boost
> > >   └── gt1
> > >   :   ├── drpc
> > >   :   ├── engines
> > >   :   ├── forcewake
> > >       ├── frequency
> > >       └── rps_boost
> > >
> > > Signed-off-by: Andi Shyti 
> > > Cc: Tvrtko Ursulin 
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_gt_debugfs.c| 4 +++-
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc.h| 2 ++
> > >  drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 5 -
> > >  drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c | 2 ++
> > >  4 files changed, 11 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> > > b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> > > index 5fc2df01aa0df..4dc23b8d3aa2d 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> > > +++ b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> > > @@ -83,11 +83,13 @@ static void gt_debugfs_register(struct intel_gt *gt,
> > > struct dentry *root)
> > >  void intel_gt_debugfs_register(struct intel_gt *gt)
> > >  {
> > >   struct dentry *root;
> > > + char gtname[4];
> > >
> > >   if (!gt->i915->drm.primary->debugfs_root)
> > >   return;
> > >
> > > - root = debugfs_create_dir("gt", gt->i915->drm.primary->debugfs_root);
> > > + snprintf(gtname, sizeof(gtname), "gt%u", gt->info.id);
> > > + root = debugfs_create_dir(gtname, gt->i915->drm.primary-
> > > >debugfs_root);
> > >   if (IS_ERR(root))
> > >   return;
> > >
> > > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > index bb4dfe707a7d0..e46aac1a41e6d 100644
> > > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > > @@ -42,6 +42,8 @@ struct intel_guc {
> > >   /** @capture: the error-state-capture module's data and objects */
> > >   struct intel_guc_state_capture *capture;
> > >
> > > + struct dentry *dbgfs_node;
> > > +
> > >   /** @sched_engine: Global engine used to submit requests to GuC */
> > >   struct i915_sched_engine *sched_engine;
> > >   /**
> > > diff --git a/drivers/gpu/drm/i915

RE: [PATCH v2 1/2] drm/i915/gt: Create per-tile debugfs files

[Sripada, Radhakrishna]

I am not sure if Tiles is appropriate usage here. Since MTL does not have the 
concept of tiles.
Shouldn't we be using gt instead of tile in our usage?

With s/tile/gt/g,
Reviewed-by: Radhakrishna Sripada  

> -Original Message-
> From: dri-devel  On Behalf Of Andi
> Shyti
> Sent: Wednesday, March 1, 2023 3:03 AM
> To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Tvrtko Ursulin ; Andi Shyti
> ; Patelczyk, Maciej ; Andi
> Shyti ; Wajdeczko, Michal
> 
> Subject: [PATCH v2 1/2] drm/i915/gt: Create per-tile debugfs files
> 
> To support multi-GT configurations, we need to generate
> independent debug files for each GT.
> 
> To achieve this create a separate directory for each GT under the
> debugfs directory. For instance, in a system with two tiles, the
> debugfs structure would look like this:
> 
> /sys/kernel/debug/dri
>   └── 0
>   ├── gt0
>   │   ├── drpc
>   │   ├── engines
>   │   ├── forcewake
>   │   ├── frequency
>   │   └── rps_boost
>   └── gt1
>   :   ├── drpc
>   :   ├── engines
>   :   ├── forcewake
>       ├── frequency
>       └── rps_boost
> 
> Signed-off-by: Andi Shyti 
> Cc: Tvrtko Ursulin 
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_debugfs.c| 4 +++-
>  drivers/gpu/drm/i915/gt/uc/intel_guc.h| 2 ++
>  drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 5 -
>  drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c | 2 ++
>  4 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> index 5fc2df01aa0df..4dc23b8d3aa2d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> @@ -83,11 +83,13 @@ static void gt_debugfs_register(struct intel_gt *gt,
> struct dentry *root)
>  void intel_gt_debugfs_register(struct intel_gt *gt)
>  {
>   struct dentry *root;
> + char gtname[4];
> 
>   if (!gt->i915->drm.primary->debugfs_root)
>   return;
> 
> - root = debugfs_create_dir("gt", gt->i915->drm.primary->debugfs_root);
> + snprintf(gtname, sizeof(gtname), "gt%u", gt->info.id);
> + root = debugfs_create_dir(gtname, gt->i915->drm.primary-
> >debugfs_root);
>   if (IS_ERR(root))
>   return;
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index bb4dfe707a7d0..e46aac1a41e6d 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -42,6 +42,8 @@ struct intel_guc {
>   /** @capture: the error-state-capture module's data and objects */
>   struct intel_guc_state_capture *capture;
> 
> + struct dentry *dbgfs_node;
> +
>   /** @sched_engine: Global engine used to submit requests to GuC */
>   struct i915_sched_engine *sched_engine;
>   /**
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> index 195db8c9d4200..55bc8b55fbc05 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -542,8 +542,11 @@ static int guc_log_relay_create(struct intel_guc_log
> *log)
>*/
>   n_subbufs = 8;
> 
> + if (!guc->dbgfs_node)
> + return -ENOENT;
> +
>   guc_log_relay_chan = relay_open("guc_log",
> - i915->drm.primary->debugfs_root,
> + guc->dbgfs_node,
>   subbuf_size, n_subbufs,
>   &relay_callbacks, i915);
>   if (!guc_log_relay_chan) {
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c
> b/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c
> index 284d6fbc2d08c..2f93cc4e408a8 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c
> @@ -54,6 +54,8 @@ void intel_uc_debugfs_register(struct intel_uc *uc, struct
> dentry *gt_root)
>   if (IS_ERR(root))
>   return;
> 
> + uc->guc.dbgfs_node = root;
> +
>   intel_gt_debugfs_register_files(root, files, ARRAY_SIZE(files), uc);
> 
>   intel_guc_debugfs_register(&uc->guc, root);
> --
> 2.39.1

RE: [PATCH v2 1/2] drm/i915/gt: Create per-tile debugfs files

[Sripada, Radhakrishna]

> -Original Message-
> From: dri-devel  On Behalf Of Andi
> Shyti
> Sent: Wednesday, March 1, 2023 3:03 AM
> To: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Cc: Tvrtko Ursulin ; Andi Shyti
> ; Patelczyk, Maciej ; Andi
> Shyti ; Wajdeczko, Michal
> 
> Subject: [PATCH v2 1/2] drm/i915/gt: Create per-tile debugfs files
> 
> To support multi-GT configurations, we need to generate
> independent debug files for each GT.
> 
> To achieve this create a separate directory for each GT under the
> debugfs directory. For instance, in a system with two tiles, the
> debugfs structure would look like this:
> 
> /sys/kernel/debug/dri
>   └── 0
>   ├── gt0
>   │   ├── drpc
>   │   ├── engines
>   │   ├── forcewake
>   │   ├── frequency
>   │   └── rps_boost
>   └── gt1
>   :   ├── drpc
>   :   ├── engines
>   :   ├── forcewake
>       ├── frequency
>       └── rps_boost
> 
> Signed-off-by: Andi Shyti 
> Cc: Tvrtko Ursulin 

LGTM,
Reviewed-by: Radhakrishna Sripada 
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_debugfs.c| 4 +++-
>  drivers/gpu/drm/i915/gt/uc/intel_guc.h| 2 ++
>  drivers/gpu/drm/i915/gt/uc/intel_guc_log.c| 5 -
>  drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c | 2 ++
>  4 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> index 5fc2df01aa0df..4dc23b8d3aa2d 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_debugfs.c
> @@ -83,11 +83,13 @@ static void gt_debugfs_register(struct intel_gt *gt,
> struct dentry *root)
>  void intel_gt_debugfs_register(struct intel_gt *gt)
>  {
>   struct dentry *root;
> + char gtname[4];
> 
>   if (!gt->i915->drm.primary->debugfs_root)
>   return;
> 
> - root = debugfs_create_dir("gt", gt->i915->drm.primary->debugfs_root);
> + snprintf(gtname, sizeof(gtname), "gt%u", gt->info.id);
> + root = debugfs_create_dir(gtname, gt->i915->drm.primary-
> >debugfs_root);
>   if (IS_ERR(root))
>   return;
> 
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> index bb4dfe707a7d0..e46aac1a41e6d 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> @@ -42,6 +42,8 @@ struct intel_guc {
>   /** @capture: the error-state-capture module's data and objects */
>   struct intel_guc_state_capture *capture;
> 
> + struct dentry *dbgfs_node;
> +
>   /** @sched_engine: Global engine used to submit requests to GuC */
>   struct i915_sched_engine *sched_engine;
>   /**
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> index 195db8c9d4200..55bc8b55fbc05 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_log.c
> @@ -542,8 +542,11 @@ static int guc_log_relay_create(struct intel_guc_log
> *log)
>*/
>   n_subbufs = 8;
> 
> + if (!guc->dbgfs_node)
> + return -ENOENT;
> +
>   guc_log_relay_chan = relay_open("guc_log",
> - i915->drm.primary->debugfs_root,
> + guc->dbgfs_node,
>   subbuf_size, n_subbufs,
>   &relay_callbacks, i915);
>   if (!guc_log_relay_chan) {
> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c
> b/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c
> index 284d6fbc2d08c..2f93cc4e408a8 100644
> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c
> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_debugfs.c
> @@ -54,6 +54,8 @@ void intel_uc_debugfs_register(struct intel_uc *uc, struct
> dentry *gt_root)
>   if (IS_ERR(root))
>   return;
> 
> + uc->guc.dbgfs_node = root;
> +
>   intel_gt_debugfs_register_files(root, files, ARRAY_SIZE(files), uc);
> 
>   intel_guc_debugfs_register(&uc->guc, root);
> --
> 2.39.1

RE: [PATCH] drm/i915/xelpmp: Consider GSI offset when doing MCR lookups

[Sripada, Radhakrishna]

> -Original Message-
> From: dri-devel  On Behalf Of Matt
> Roper
> Sent: Monday, February 13, 2023 4:19 PM
> To: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Subject: [PATCH] drm/i915/xelpmp: Consider GSI offset when doing MCR
> lookups
> 
> MCR range tables use the final MMIO offset of a register (including the
> 0x38 GSI offset when applicable).  Since the i915_mcr_reg_t passed
> as a parameter during steering lookup does not include the GSI offset,
> we need to add it back in for GSI registers before searching the tables.
> 
> Fixes: a7ec65fc7e83 ("drm/i915/xelpmp: Add multicast steering for media GT")

LGTM,
Reviewed-by: Radhakrishna Sripada 

> Signed-off-by: Matt Roper 
> ---
>  drivers/gpu/drm/i915/gt/intel_gt_mcr.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> index a4a8b8bc5737..03632df27de3 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt_mcr.c
> @@ -561,12 +561,15 @@ static bool reg_needs_read_steering(struct intel_gt
> *gt,
>   i915_mcr_reg_t reg,
>   enum intel_steering_type type)
>  {
> - const u32 offset = i915_mmio_reg_offset(reg);
> + u32 offset = i915_mmio_reg_offset(reg);
>   const struct intel_mmio_range *entry;
> 
>   if (likely(!gt->steering_table[type]))
>   return false;
> 
> + if (IS_GSI_REG(offset))
> + offset += gt->uncore->gsi_offset;
> +
>   for (entry = gt->steering_table[type]; entry->end; entry++) {
>   if (offset >= entry->start && offset <= entry->end)
>   return true;
> --
> 2.39.1

RE: [Intel-gfx] [PATCH] drm/i915: Use graphics ver, rel info for media on old platforms

[Sripada, Radhakrishna]

Thank you for the feedback. Incorporated the review and posted new patches here 
[1].

Thanks,
Radhakrishna(RK) Sripada

[1] https://patchwork.freedesktop.org/series/109588/

> -Original Message-
> From: Ville Syrjälä 
> Sent: Tuesday, October 11, 2022 3:33 AM
> To: Jani Nikula 
> Cc: Sripada, Radhakrishna ; intel-
> g...@lists.freedesktop.org; De Marchi, Lucas ; dri-
> de...@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Use graphics ver, rel info for 
> media on
> old platforms
> 
> On Tue, Oct 11, 2022 at 01:10:26PM +0300, Jani Nikula wrote:
> > On Tue, 11 Oct 2022, "Sripada, Radhakrishna"
>  wrote:
> > > Hi Jani,
> > >
> > >> -Original Message-
> > >> From: Jani Nikula 
> > >> Sent: Tuesday, October 11, 2022 12:28 AM
> > >> To: Sripada, Radhakrishna ; intel-
> > >> g...@lists.freedesktop.org
> > >> Cc: dri-devel@lists.freedesktop.org; Sripada, Radhakrishna
> > >> ; De Marchi, Lucas
> > >> ; Roper, Matthew D
> > >> 
> > >> Subject: Re: [PATCH] drm/i915: Use graphics ver, rel info for media on 
> > >> old
> > >> platforms
> > >>
> > >> On Mon, 10 Oct 2022, Radhakrishna Sripada
> 
> > >> wrote:
> > >> > Platforms prior to MTL do not have a separate media and graphics
> version.
> > >> > On platforms where GMD id is not supported, reuse the graphics ip
> version,
> > >> > release info for media.
> > >> >
> > >> > The rest of the IP graphics, display versions would be copied during 
> > >> > driver
> > >> > creation.
> > >> >
> > >> > While at it warn if GMD is not used for platforms greater than gen12.
> > >> >
> > >> > Fixes: c2c7075225ef ("drm/i915: Read graphics/media/display arch
> version
> > >> from hw")
> > >> > Cc: Jani Nikula 
> > >> > Cc: Lucas De Marchi 
> > >> > Cc: Matt Roper 
> > >> > Signed-off-by: Radhakrishna Sripada 
> > >> > ---
> > >> >  drivers/gpu/drm/i915/intel_device_info.c | 12 +++-
> > >> >  1 file changed, 11 insertions(+), 1 deletion(-)
> > >> >
> > >> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> > >> b/drivers/gpu/drm/i915/intel_device_info.c
> > >> > index 090097bb3c0a..ba178b61bceb 100644
> > >> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > >> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > >> > @@ -329,8 +329,18 @@ static void intel_ipver_early_init(struct
> > >> drm_i915_private *i915)
> > >> >  {
> > >> >struct intel_runtime_info *runtime = RUNTIME_INFO(i915);
> > >> >
> > >> > -  if (!HAS_GMD_ID(i915))
> > >> > +  if (!HAS_GMD_ID(i915)) {
> > >> > +  drm_WARN_ON(&i915->drm, RUNTIME_INFO(i915)-
> > >> >graphics.ip.ver > 12);
> > >> > +  /*
> > >> > +   * On older platforms, graphics and media share the 
> > >> > same ip
> > >> > +   * version and release.
> > >> > +   */
> > >> > +  RUNTIME_INFO(i915)->media.ip.ver =
> > >> > +  RUNTIME_INFO(i915)->graphics.ip.ver;
> > >> > +  RUNTIME_INFO(i915)->media.ip.rel =
> > >> > +  RUNTIME_INFO(i915)->graphics.ip.rel;
> > >>
> > >> You could assign the whole struct ip_version (*) at once, or is there a
> > >> reason you're intentionally not assigning step?
> > > Step info would anyways be determined later in the function 
> > > intel_step_init.
> > > We already have macros in place to handle common gt and media steps
> there.
> > >
> > > Do you suggest we memcpy(&RUNTIME_INFO(i915)->media.ip,
> &RUNTIME_INFO->graphics.ip, sizeof(struct ip_version)) here?
> >
> > Simple assign should do it for such a small struct.
> 
> IMO for any struct. Only use memcpy() when copying arrays and such.
> 
> --
> Ville Syrjälä
> Intel

RE: [PATCH] drm/i915: Use graphics ver, rel info for media on old platforms

[Sripada, Radhakrishna]

Hi Jani,

> -Original Message-
> From: Jani Nikula 
> Sent: Tuesday, October 11, 2022 12:28 AM
> To: Sripada, Radhakrishna ; intel-
> g...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; Sripada, Radhakrishna
> ; De Marchi, Lucas
> ; Roper, Matthew D
> 
> Subject: Re: [PATCH] drm/i915: Use graphics ver, rel info for media on old
> platforms
> 
> On Mon, 10 Oct 2022, Radhakrishna Sripada 
> wrote:
> > Platforms prior to MTL do not have a separate media and graphics version.
> > On platforms where GMD id is not supported, reuse the graphics ip version,
> > release info for media.
> >
> > The rest of the IP graphics, display versions would be copied during driver
> > creation.
> >
> > While at it warn if GMD is not used for platforms greater than gen12.
> >
> > Fixes: c2c7075225ef ("drm/i915: Read graphics/media/display arch version
> from hw")
> > Cc: Jani Nikula 
> > Cc: Lucas De Marchi 
> > Cc: Matt Roper 
> > Signed-off-by: Radhakrishna Sripada 
> > ---
> >  drivers/gpu/drm/i915/intel_device_info.c | 12 +++-
> >  1 file changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> b/drivers/gpu/drm/i915/intel_device_info.c
> > index 090097bb3c0a..ba178b61bceb 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -329,8 +329,18 @@ static void intel_ipver_early_init(struct
> drm_i915_private *i915)
> >  {
> > struct intel_runtime_info *runtime = RUNTIME_INFO(i915);
> >
> > -   if (!HAS_GMD_ID(i915))
> > +   if (!HAS_GMD_ID(i915)) {
> > +   drm_WARN_ON(&i915->drm, RUNTIME_INFO(i915)-
> >graphics.ip.ver > 12);
> > +   /*
> > +* On older platforms, graphics and media share the same ip
> > +* version and release.
> > +*/
> > +   RUNTIME_INFO(i915)->media.ip.ver =
> > +   RUNTIME_INFO(i915)->graphics.ip.ver;
> > +   RUNTIME_INFO(i915)->media.ip.rel =
> > +   RUNTIME_INFO(i915)->graphics.ip.rel;
> 
> You could assign the whole struct ip_version (*) at once, or is there a
> reason you're intentionally not assigning step?
Step info would anyways be determined later in the function intel_step_init.
We already have macros in place to handle common gt and media steps there.

Do you suggest we memcpy(&RUNTIME_INFO(i915)->media.ip, 
&RUNTIME_INFO->graphics.ip, sizeof(struct ip_version)) here?

> 
> BR,
> Jani.
> 
> (*) Why does that name not have intel_ prefix?
Good question. Since introduced in " a5b7ef27da60 drm/i915: Add struct to hold 
IP version"
we have been using as is. The author might have felt that the structure is not 
big enough/used in as many places
to have an intel_ prefix. Do you see a symbol collision here that we need to 
add intel_ prefix? If so should we do it
in a separate patch?

Thanks,
Radhakrishna(RK) Sripada
> 
> > return;
> > +   }
> >
> > ip_ver_read(i915, i915_mmio_reg_offset(GMD_ID_GRAPHICS),
> > &runtime->graphics.ip);
> 
> --
> Jani Nikula, Intel Open Source Graphics Center

RE: [PATCH v4.1] drm/i915/mtl: Define engine context layouts

[Sripada, Radhakrishna]

> -Original Message-
> From: De Marchi, Lucas 
> Sent: Thursday, September 29, 2022 5:11 PM
> To: Sripada, Radhakrishna 
> Cc: intel-...@lists.freedesktop.org; dri-devel@lists.freedesktop.org
> Subject: Re: [PATCH v4.1] drm/i915/mtl: Define engine context layouts
> 
> On Wed, Sep 28, 2022 at 08:55:11AM -0700, Radhakrishna Sripada wrote:
> >From: Matt Roper 
> >
> >The part of the media and blitter engine contexts that we care about for
> >setting up an initial state on MTL are nearly similar to DG2 (and PVC).
> >The difference being PRT_BB_STATE being replaced with NOP.
> >
> >For render/compute engines, the part of the context images are nearly
> >the same, although the layout had a very slight change --- one POSH
> >register was removed and the placement of some LRI/noops adjusted
> >slightly to compensate.
> >
> >v2:
> > - Dg2, mtl xcs offsets slightly vary. Use a separate offsets array(Bala)
> > - Add missing nop in xcs offsets(Bala)
> >v3:
> > - Fix the spacing for nop in xcs offset(MattR)
> >v4:
> > - Fix rcs register offset(MattR)
> >v4.1:
> > - Fix commit message(Lucas)
> >
> >Bspec: 46261, 46260, 45585
> >Cc: Balasubramani Vivekanandan 
> >Cc: Licas De Marchi 
> >Signed-off-by: Matt Roper 
> >Signed-off-by: Radhakrishna Sripada 
> 
> 
> Reviewed-by: Lucas De Marchi 
Pushed the patch, Thanks for the review.

-Radhakrishna Sripada
> 
> Lucas De Marchi

RE: [PATCH v3 07/14] drm/i915: Use a DRM-managed action to release the PCI bridge device

[Sripada, Radhakrishna]

> -Original Message-
> From: dri-devel  On Behalf Of Matt
> Roper
> Sent: Tuesday, September 6, 2022 4:49 PM
> To: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Subject: [PATCH v3 07/14] drm/i915: Use a DRM-managed action to release the
> PCI bridge device
> 
> As we start supporting multiple uncore structures in future patches, the
> MMIO cleanup (which make also get called mid-init if there's a failure)
> will become more complicated.  Moving to DRM-managed actions will help
> keep things simple.
> 
LGTM,
Reviewed-by: Radhakrishna Sripada 

> Signed-off-by: Matt Roper 
> ---
>  drivers/gpu/drm/i915/i915_driver.c | 12 +---
>  1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index 18acba1bc3b0..1f46dd1ffaf7 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -105,6 +105,12 @@ static const char irst_name[] = "INT3392";
> 
>  static const struct drm_driver i915_drm_driver;
> 
> +static void i915_release_bridge_dev(struct drm_device *dev,
> + void *bridge)
> +{
> + pci_dev_put(bridge);
> +}
> +
>  static int i915_get_bridge_dev(struct drm_i915_private *dev_priv)
>  {
>   int domain = pci_domain_nr(to_pci_dev(dev_priv->drm.dev)->bus);
> @@ -115,7 +121,9 @@ static int i915_get_bridge_dev(struct drm_i915_private
> *dev_priv)
>   drm_err(&dev_priv->drm, "bridge device not found\n");
>   return -EIO;
>   }
> - return 0;
> +
> + return drmm_add_action_or_reset(&dev_priv->drm,
> i915_release_bridge_dev,
> + dev_priv->bridge_dev);
>  }
> 
>  /* Allocate space for the MCH regs if needed, return nonzero on error */
> @@ -452,7 +460,6 @@ static int i915_driver_mmio_probe(struct
> drm_i915_private *dev_priv)
>  err_uncore:
>   intel_teardown_mchbar(dev_priv);
>   intel_uncore_fini_mmio(&dev_priv->uncore);
> - pci_dev_put(dev_priv->bridge_dev);
> 
>   return ret;
>  }
> @@ -465,7 +472,6 @@ static void i915_driver_mmio_release(struct
> drm_i915_private *dev_priv)
>  {
>   intel_teardown_mchbar(dev_priv);
>   intel_uncore_fini_mmio(&dev_priv->uncore);
> - pci_dev_put(dev_priv->bridge_dev);
>  }
> 
>  /**
> --
> 2.37.2

RE: [Intel-gfx] [PATCH v4 02/11] drm/i915: Read graphics/media/display arch version from hw

[Sripada, Radhakrishna]

Hi Lucas/Matt,

> -Original Message-
> From: De Marchi, Lucas 
> Sent: Wednesday, September 7, 2022 3:21 PM
> To: Roper, Matthew D 
> Cc: Sripada, Radhakrishna ; intel-
> g...@lists.freedesktop.org; dri-devel@lists.freedesktop.org; Vivi, Rodrigo
> 
> Subject: Re: [Intel-gfx] [PATCH v4 02/11] drm/i915: Read 
> graphics/media/display
> arch version from hw
> 
> On Wed, Sep 07, 2022 at 03:13:31PM -0700, Matt Roper wrote:
> >On Wed, Sep 07, 2022 at 01:49:25PM -0700, Lucas De Marchi wrote:
> >> On Thu, Sep 01, 2022 at 11:03:33PM -0700, Radhakrishna Sripada wrote:
> >> > From: Matt Roper 
> >> >
> >> > Going forward, the hardware teams no longer consider new platforms to
> >> > have a "generation" in the way we've defined it for past platforms.
> >> > Instead, each IP block (graphics, media, display) will have their own
> >> > architecture major.minor versions and stepping ID's which should be read
> >> > directly from a register in the MMIO space.  New hardware programming
> >> > styles, features, and workarounds should be conditional solely on the
> >> > architecture version, and should no longer be derived from the PCI
> >> > device ID, revision ID, or platform-specific feature flags.
> >> >
> >> > Bspec: 63361, 64111
> >> >
> >> > v2:
> >> >  - Move the IP version readout to intel_device_info.c
> >> >  - Convert the macro into a function
> >> >
> >> > v3:
> >> >  - Move subplatform init to runtime early init
> >> >  - Cache runtime ver, release info to compare with hardware values.
> >> >
> >> > Signed-off-by: Matt Roper 
> >> > Signed-off-by: Rodrigo Vivi 
> >> > Signed-off-by: Radhakrishna Sripada 
> >> > ---
> >> > drivers/gpu/drm/i915/gt/intel_gt_regs.h  |  2 +
> >> > drivers/gpu/drm/i915/i915_driver.c   |  3 +-
> >> > drivers/gpu/drm/i915/i915_drv.h  |  2 +
> >> > drivers/gpu/drm/i915/i915_pci.c  |  1 +
> >> > drivers/gpu/drm/i915/i915_reg.h  |  7 +++
> >> > drivers/gpu/drm/i915/intel_device_info.c | 74
> +++-
> >> > drivers/gpu/drm/i915/intel_device_info.h | 12 +++-
> >> > 7 files changed, 98 insertions(+), 3 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> >> > index d414785003cc..579da62158c4 100644
> >> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> >> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> >> > @@ -39,6 +39,8 @@
> >> > #define FORCEWAKE_ACK_RENDER_GEN9_MMIO(0xd84)
> >> > #define FORCEWAKE_ACK_MEDIA_GEN9 _MMIO(0xd88)
> >> >
> >> > +#define GMD_ID_GRAPHICS
>   _MMIO(0xd8c)
> >> > +
> >> > #define MCFG_MCR_SELECTOR_MMIO(0xfd0)
> >> > #define SF_MCR_SELECTOR  _MMIO(0xfd8)
> >> > #define GEN8_MCR_SELECTOR_MMIO(0xfdc)
> >> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> >> > index 56a2bcddb2af..a1ab49521d19 100644
> >> > --- a/drivers/gpu/drm/i915/i915_driver.c
> >> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> >> > @@ -323,7 +323,8 @@ static int i915_driver_early_probe(struct
> drm_i915_private *dev_priv)
> >> >  if (i915_inject_probe_failure(dev_priv))
> >> >  return -ENODEV;
> >> >
> >> > -intel_device_info_subplatform_init(dev_priv);
> >> > +intel_device_info_runtime_init_early(dev_priv);
> >> > +
> >> >  intel_step_init(dev_priv);
> >> >
> >> >  intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug);
> >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> >> > index f85a470397a5..405b59b8c05c 100644
> >> > --- a/drivers/gpu/drm/i915/i915_drv.h
> >> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> > @@ -936,6 +936,8 @@ IS_SUBPLATFORM(const struct drm_i915_private
> *i915,
> >> >
> >> > #define HAS_GMCH(dev_priv) (INTEL_INFO(dev_priv)->display.has_gmch)
> >> >
> >> > +#define HAS_GMD_ID(i915)INTEL_INFO(i915)->has_gmd_id
> >> > +
> >> > #define HAS_LSPCON(dev_priv) (IS_DISPLAY_VER(dev_priv, 

RE: [Intel-gfx] [PATCH v3 02/11] drm/i915: Read graphics/media/display arch version from hw

[Sripada, Radhakrishna]

Hi Jani,

> -Original Message-
> From: Jani Nikula 
> Sent: Thursday, September 1, 2022 12:58 AM
> To: Sripada, Radhakrishna ; intel-
> g...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v3 02/11] drm/i915: Read 
> graphics/media/display
> arch version from hw
> 
> On Wed, 31 Aug 2022, Radhakrishna Sripada 
> wrote:
> > From: Matt Roper 
> >
> > Going forward, the hardware teams no longer consider new platforms to
> > have a "generation" in the way we've defined it for past platforms.
> > Instead, each IP block (graphics, media, display) will have their own
> > architecture major.minor versions and stepping ID's which should be read
> > directly from a register in the MMIO space.  New hardware programming
> > styles, features, and workarounds should be conditional solely on the
> > architecture version, and should no longer be derived from the PCI
> > device ID, revision ID, or platform-specific feature flags.
> >
> > Bspec: 63361, 64111
> >
> > v2:
> >   - Move the IP version readout to intel_device_info.c
> >   - Convert the macro into a function
> >
> > Signed-off-by: Matt Roper 
> > Signed-off-by: Rodrigo Vivi 
> > Signed-off-by: Radhakrishna Sripada 
> > ---
> >  drivers/gpu/drm/i915/gt/intel_gt_regs.h  |  2 +
> >  drivers/gpu/drm/i915/i915_driver.c   |  2 +
> >  drivers/gpu/drm/i915/i915_drv.h  |  2 +
> >  drivers/gpu/drm/i915/i915_pci.c  |  1 +
> >  drivers/gpu/drm/i915/i915_reg.h  |  6 ++
> >  drivers/gpu/drm/i915/intel_device_info.c | 73 
> >  drivers/gpu/drm/i915/intel_device_info.h |  3 +
> >  7 files changed, 89 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > index d414785003cc..579da62158c4 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h
> > @@ -39,6 +39,8 @@
> >  #define FORCEWAKE_ACK_RENDER_GEN9  _MMIO(0xd84)
> >  #define FORCEWAKE_ACK_MEDIA_GEN9   _MMIO(0xd88)
> >
> > +#define GMD_ID_GRAPHICS_MMIO(0xd8c)
> > +
> >  #define MCFG_MCR_SELECTOR  _MMIO(0xfd0)
> >  #define SF_MCR_SELECTOR_MMIO(0xfd8)
> >  #define GEN8_MCR_SELECTOR  _MMIO(0xfdc)
> > diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> > index 3aedc33ded57..5826c70d6fa5 100644
> > --- a/drivers/gpu/drm/i915/i915_driver.c
> > +++ b/drivers/gpu/drm/i915/i915_driver.c
> > @@ -323,6 +323,8 @@ static int i915_driver_early_probe(struct
> drm_i915_private *dev_priv)
> > if (i915_inject_probe_failure(dev_priv))
> > return -ENODEV;
> >
> > +   intel_device_info_runtime_init_early(dev_priv);
> > +
> > intel_device_info_subplatform_init(dev_priv);
> 
> Hmm, so why not move the subplatform init call to runtime init early?
Will move in next rev.

> 
> > intel_step_init(dev_priv);
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> > index bf60593a4ce5..935ff3486fef 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -936,6 +936,8 @@ IS_SUBPLATFORM(const struct drm_i915_private
> *i915,
> >
> >  #define HAS_GMCH(dev_priv) (INTEL_INFO(dev_priv)->display.has_gmch)
> >
> > +#define HAS_GMD_ID(i915)   INTEL_INFO(i915)->has_gmd_id
> > +
> >  #define HAS_LSPCON(dev_priv) (IS_DISPLAY_VER(dev_priv, 9, 10))
> >
> >  #define HAS_L3_CCS_READ(i915) (INTEL_INFO(i915)->has_l3_ccs_read)
> > diff --git a/drivers/gpu/drm/i915/i915_pci.c
> b/drivers/gpu/drm/i915/i915_pci.c
> > index 72577e327c71..9772c315185d 100644
> > --- a/drivers/gpu/drm/i915/i915_pci.c
> > +++ b/drivers/gpu/drm/i915/i915_pci.c
> > @@ -1129,6 +1129,7 @@ static const struct intel_device_info mtl_info = {
> > PLATFORM(INTEL_METEORLAKE),
> > .display.has_modular_fia = 1,
> > .has_flat_ccs = 0,
> > +   .has_gmd_id = 1,
> > .has_snoop = 1,
> > .__runtime.memory_regions = REGION_SMEM |
> REGION_STOLEN_LMEM,
> > .__runtime.platform_engine_mask = BIT(RCS0) | BIT(BCS0) | BIT(CCS0),
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> > index 5e6239864c35..f52ed6d00030 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> >

RE: [PATCH 5/8] drm/i915: Rename and expose common GT early init routine

[Sripada, Radhakrishna]

> -Original Message-
> From: Roper, Matthew D 
> Sent: Monday, August 29, 2022 10:03 AM
> To: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; Sripada, Radhakrishna
> ; Roper, Matthew D
> 
> Subject: [PATCH 5/8] drm/i915: Rename and expose common GT early init
> routine
> 
> The common early GT init is needed for initialization of all GT types
> (root/primary, remote tile, standalone media).  Since standalone media
> (coming in the next patch) will be implemented in a separate file,
> rename and expose the function for use.
> 
Reviewed-by: Radhakrishna Sripada 

- RK Sripada
> Signed-off-by: Matt Roper 
> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c | 6 +++---
>  drivers/gpu/drm/i915/gt/intel_gt.h | 1 +
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index 7c0525e96155..d21ec11346a5 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -35,7 +35,7 @@
>  #include "intel_uncore.h"
>  #include "shmem_utils.h"
> 
> -static void __intel_gt_init_early(struct intel_gt *gt)
> +void intel_gt_common_init_early(struct intel_gt *gt)
>  {
>   spin_lock_init(>->irq_lock);
> 
> @@ -65,7 +65,7 @@ void intel_root_gt_init_early(struct drm_i915_private
> *i915)
>   gt->i915 = i915;
>   gt->uncore = &i915->uncore;
> 
> - __intel_gt_init_early(gt);
> + intel_gt_common_init_early(gt);
>  }
> 
>  static int intel_gt_probe_lmem(struct intel_gt *gt)
> @@ -789,7 +789,7 @@ static int intel_gt_tile_setup(struct intel_gt *gt,
> phys_addr_t phys_addr)
> 
>   gt->uncore = uncore;
> 
> - __intel_gt_init_early(gt);
> + intel_gt_common_init_early(gt);
>   }
> 
>   intel_uncore_init_early(gt->uncore, gt);
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.h
> b/drivers/gpu/drm/i915/gt/intel_gt.h
> index 4d8779529cc2..c9a359f35d0f 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.h
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.h
> @@ -44,6 +44,7 @@ static inline struct intel_gt *gsc_to_gt(struct intel_gsc
> *gsc)
>   return container_of(gsc, struct intel_gt, gsc);
>  }
> 
> +void intel_gt_common_init_early(struct intel_gt *gt);
>  void intel_root_gt_init_early(struct drm_i915_private *i915);
>  int intel_gt_assign_ggtt(struct intel_gt *gt);
>  int intel_gt_init_mmio(struct intel_gt *gt);
> --
> 2.37.2

RE: [PATCH 4/8] drm/i915: Prepare more multi-GT initialization

[Sripada, Radhakrishna]

> -Original Message-
> From: Roper, Matthew D 
> Sent: Monday, August 29, 2022 10:03 AM
> To: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; Sripada, Radhakrishna
> ; Roper, Matthew D
> ; Iddamsetty, Aravind
> 
> Subject: [PATCH 4/8] drm/i915: Prepare more multi-GT initialization
> 
> We're going to introduce an additional intel_gt for MTL's media unit
> soon.  Let's provide a bit more multi-GT initialization framework in
> preparation for that.  The initialization will pull the list of GTs for
> a platform from the device info structure.  Although necessary for the
> immediate MTL media enabling, this same framework will also be used
> farther down the road when we enable remote tiles on xehpsdv and pvc.
> 
> Cc: Aravind Iddamsetty 
LGTM.
Reviewed-by: Radhakrishna Sripada 

- RK Sripada

> Signed-off-by: Matt Roper 
> ---
>  drivers/gpu/drm/i915/gt/intel_engine_cs.c |  2 +-
>  drivers/gpu/drm/i915/gt/intel_gt.c| 48 +--
>  drivers/gpu/drm/i915/gt/intel_gt.h|  1 -
>  drivers/gpu/drm/i915/gt/intel_gt_types.h  |  3 ++
>  drivers/gpu/drm/i915/i915_drv.h   |  2 +
>  drivers/gpu/drm/i915/intel_device_info.h  | 16 +++
>  .../gpu/drm/i915/selftests/mock_gem_device.c  |  1 +
>  7 files changed, 67 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> index 275ad72940c1..41acc285e8bf 100644
> --- a/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/gt/intel_engine_cs.c
> @@ -736,7 +736,7 @@ static intel_engine_mask_t init_engine_mask(struct
> intel_gt *gt)
>   u16 vdbox_mask;
>   u16 vebox_mask;
> 
> - info->engine_mask = RUNTIME_INFO(i915)->platform_engine_mask;
> + GEM_BUG_ON(!info->engine_mask);
> 
>   if (GRAPHICS_VER(i915) < 11)
>   return info->engine_mask;
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index cf7aab7adb30..7c0525e96155 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -807,17 +807,16 @@ static void
>  intel_gt_tile_cleanup(struct intel_gt *gt)
>  {
>   intel_uncore_cleanup_mmio(gt->uncore);
> -
> - if (!gt_is_root(gt))
> - kfree(gt);
>  }
> 
>  int intel_gt_probe_all(struct drm_i915_private *i915)
>  {
>   struct pci_dev *pdev = to_pci_dev(i915->drm.dev);
>   struct intel_gt *gt = &i915->gt0;
> + const struct intel_gt_definition *gtdef;
>   phys_addr_t phys_addr;
>   unsigned int mmio_bar;
> + unsigned int i;
>   int ret;
> 
>   mmio_bar = GRAPHICS_VER(i915) == 2 ? GEN2_GTTMMADR_BAR :
> GTTMMADR_BAR;
> @@ -828,14 +827,55 @@ int intel_gt_probe_all(struct drm_i915_private *i915)
>* and it has been already initialized early during probe
>* in i915_driver_probe()
>*/
> + gt->i915 = i915;
> + gt->name = "Primary GT";
> + gt->info.engine_mask = RUNTIME_INFO(i915)->platform_engine_mask;
> +
> + drm_dbg(&i915->drm, "Setting up %s\n", gt->name);
>   ret = intel_gt_tile_setup(gt, phys_addr);
>   if (ret)
>   return ret;
> 
>   i915->gt[0] = gt;
> 
> - /* TODO: add more tiles */
> + for (i = 1, gtdef = &INTEL_INFO(i915)->extra_gt_list[i - 1];
> +  gtdef->setup != NULL;
> +  i++, gtdef = &INTEL_INFO(i915)->extra_gt_list[i - 1]) {
> + gt = drmm_kzalloc(&i915->drm, sizeof(*gt), GFP_KERNEL);
> + if (!gt) {
> + ret = -ENOMEM;
> + goto err;
> + }
> +
> + gt->i915 = i915;
> + gt->name = gtdef->name;
> + gt->type = gtdef->type;
> + gt->info.engine_mask = gtdef->engine_mask;
> + gt->info.id = i;
> +
> + drm_dbg(&i915->drm, "Setting up %s\n", gt->name);
> + if (GEM_WARN_ON(range_overflows_t(resource_size_t,
> +   gtdef->mapping_base,
> +   SZ_16M,
> +   pci_resource_len(pdev,
> mmio_bar {
> + ret = -ENODEV;
> + goto err;
> + }
> +
> + ret = gtdef->setup(gt, phys_addr + gtdef->mapping_base);
> + if (ret)
> + goto err;
> +
> + i91

RE: [PATCH 3/8] drm/i915: Use managed allocations for extra uncore objects

[Sripada, Radhakrishna]

> -Original Message-
> From: Roper, Matthew D 
> Sent: Monday, August 29, 2022 10:03 AM
> To: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; Sripada, Radhakrishna
> ; Roper, Matthew D
> 
> Subject: [PATCH 3/8] drm/i915: Use managed allocations for extra uncore
> objects
> 
> We're slowly transitioning the init-time kzalloc's of the driver over to
> DRM-managed allocations; let's make sure the uncore objects allocated
> for non-root GTs are thus allocated.
> 
Reviewed-by: Radhakrishna Sripada 

- RK Sripada
> Signed-off-by: Matt Roper 
> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index a82b5e2e0d83..cf7aab7adb30 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -783,7 +783,7 @@ static int intel_gt_tile_setup(struct intel_gt *gt,
> phys_addr_t phys_addr)
>   if (!gt_is_root(gt)) {
>   struct intel_uncore *uncore;
> 
> - uncore = kzalloc(sizeof(*uncore), GFP_KERNEL);
> + uncore = drmm_kzalloc(>->i915->drm, sizeof(*uncore),
> GFP_KERNEL);
>   if (!uncore)
>   return -ENOMEM;
> 
> @@ -808,10 +808,8 @@ intel_gt_tile_cleanup(struct intel_gt *gt)
>  {
>   intel_uncore_cleanup_mmio(gt->uncore);
> 
> - if (!gt_is_root(gt)) {
> - kfree(gt->uncore);
> + if (!gt_is_root(gt))
>   kfree(gt);
> - }
>  }
> 
>  int intel_gt_probe_all(struct drm_i915_private *i915)
> --
> 2.37.2

RE: [PATCH 2/8] drm/i915: Only hook up uncore->debug for primary uncore

[Sripada, Radhakrishna]

Hi Matt,

> -Original Message-
> From: Roper, Matthew D 
> Sent: Monday, August 29, 2022 10:03 AM
> To: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; Sripada, Radhakrishna
> ; Roper, Matthew D
> 
> Subject: [PATCH 2/8] drm/i915: Only hook up uncore->debug for primary uncore
> 
> The original intent of intel_uncore_mmio_debug as described in commit
> 0a9b26306d6a ("drm/i915: split out uncore_mmio_debug") was to be a
> singleton structure that could be shared between multiple GTs' uncore
> objects in a multi-tile system.  Somehow we went off track and
> started allocating separate instances of this structure for each GT,
> which defeats that original goal.
> 
> But in reality, there isn't even a need to share the mmio_debug between
> multiple GTs; on all modern platforms (i.e., everything after gen7)
> unclaimed register accesses are something that can only be detected for
> display registers.  There's no point in grabbing the debug spinlock and
> checking for unclaimed accesses on an uncore used by an xehpsdv or pvc
> remote tile GT, or the uncore used by a mtl standalone media GT since
> all of the display accesses go through the primary intel_uncore.
> 
> The simplest solution is to simply leave uncore->debug NULL on all
> intel_uncore instances except for the primary one.  This will allow us
> to avoid the pointless debug spinlock acquisition we've been doing on
> MMIO accesses coming in through these intel_uncores.
> 
Reviewed-by: Radhakrishna Sripada 

> Signed-off-by: Matt Roper 
> ---
>  drivers/gpu/drm/i915/gt/intel_gt.c  |  9 -
>  drivers/gpu/drm/i915/i915_driver.c  |  2 +-
>  drivers/gpu/drm/i915/intel_uncore.c | 23 ++-
>  drivers/gpu/drm/i915/intel_uncore.h |  3 +--
>  4 files changed, 20 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c
> b/drivers/gpu/drm/i915/gt/intel_gt.c
> index e4bac2431e41..a82b5e2e0d83 100644
> --- a/drivers/gpu/drm/i915/gt/intel_gt.c
> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c
> @@ -781,21 +781,13 @@ static int intel_gt_tile_setup(struct intel_gt *gt,
> phys_addr_t phys_addr)
>   int ret;
> 
>   if (!gt_is_root(gt)) {
> - struct intel_uncore_mmio_debug *mmio_debug;
>   struct intel_uncore *uncore;
> 
>   uncore = kzalloc(sizeof(*uncore), GFP_KERNEL);
>   if (!uncore)
>   return -ENOMEM;
> 
> - mmio_debug = kzalloc(sizeof(*mmio_debug), GFP_KERNEL);
> - if (!mmio_debug) {
> - kfree(uncore);
> - return -ENOMEM;
> - }
> -
>   gt->uncore = uncore;
> - gt->uncore->debug = mmio_debug;
> 
>   __intel_gt_init_early(gt);
>   }
> @@ -817,7 +809,6 @@ intel_gt_tile_cleanup(struct intel_gt *gt)
>   intel_uncore_cleanup_mmio(gt->uncore);
> 
>   if (!gt_is_root(gt)) {
> - kfree(gt->uncore->debug);
>   kfree(gt->uncore);
>   kfree(gt);
>   }
> diff --git a/drivers/gpu/drm/i915/i915_driver.c
> b/drivers/gpu/drm/i915/i915_driver.c
> index 053a7dab5506..de9020771836 100644
> --- a/drivers/gpu/drm/i915/i915_driver.c
> +++ b/drivers/gpu/drm/i915/i915_driver.c
> @@ -326,7 +326,7 @@ static int i915_driver_early_probe(struct
> drm_i915_private *dev_priv)
>   intel_device_info_subplatform_init(dev_priv);
>   intel_step_init(dev_priv);
> 
> - intel_uncore_mmio_debug_init_early(&dev_priv->mmio_debug);
> + intel_uncore_mmio_debug_init_early(dev_priv);
> 
>   spin_lock_init(&dev_priv->irq_lock);
>   spin_lock_init(&dev_priv->gpu_error.lock);
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> b/drivers/gpu/drm/i915/intel_uncore.c
> index e717ea55484a..6841f76533f9 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -44,14 +44,19 @@ fw_domains_get(struct intel_uncore *uncore, enum
> forcewake_domains fw_domains)
>  }
> 
>  void
> -intel_uncore_mmio_debug_init_early(struct intel_uncore_mmio_debug
> *mmio_debug)
> +intel_uncore_mmio_debug_init_early(struct drm_i915_private *i915)
>  {
> - spin_lock_init(&mmio_debug->lock);
> - mmio_debug->unclaimed_mmio_check = 1;
> + spin_lock_init(&i915->mmio_debug.lock);
> + i915->mmio_debug.unclaimed_mmio_check = 1;
> +
> + i915->uncore.debug = &i915->mmio_debug;
>  }
> 
>  static void mmio_debug_suspend(struct intel_uncore *uncore)
>  {
> + if (!uncore->debug)
> + return;
> +
&

RE: [PATCH 1/8] drm/i915: Move locking and unclaimed check into mmio_debug_{suspend,resume}

[Sripada, Radhakrishna]

> -Original Message-
> From: Roper, Matthew D 
> Sent: Monday, August 29, 2022 10:03 AM
> To: intel-...@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org; Sripada, Radhakrishna
> ; Roper, Matthew D
> 
> Subject: [PATCH 1/8] drm/i915: Move locking and unclaimed check into
> mmio_debug_{suspend,resume}
> 
> Moving the locking for MMIO debug (and the final check for unclaimed
> accesses when resuming debug after a userspace-initiated forcewake) will
> make it simpler to completely skip MMIO debug handling on uncores that
> don't support it in future patches.
> 
LGTM,
Reviewed-by: Radhakrishna Sripada 

> Signed-off-by: Matt Roper 
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 41 +++--
>  1 file changed, 21 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> b/drivers/gpu/drm/i915/intel_uncore.c
> index 9b81b2543ce2..e717ea55484a 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -50,23 +50,33 @@ intel_uncore_mmio_debug_init_early(struct
> intel_uncore_mmio_debug *mmio_debug)
>   mmio_debug->unclaimed_mmio_check = 1;
>  }
> 
> -static void mmio_debug_suspend(struct intel_uncore_mmio_debug
> *mmio_debug)
> +static void mmio_debug_suspend(struct intel_uncore *uncore)
>  {
> - lockdep_assert_held(&mmio_debug->lock);
> + spin_lock(&uncore->debug->lock);
> 
>   /* Save and disable mmio debugging for the user bypass */
> - if (!mmio_debug->suspend_count++) {
> - mmio_debug->saved_mmio_check = mmio_debug-
> >unclaimed_mmio_check;
> - mmio_debug->unclaimed_mmio_check = 0;
> + if (!uncore->debug->suspend_count++) {
> + uncore->debug->saved_mmio_check = uncore->debug-
> >unclaimed_mmio_check;
> + uncore->debug->unclaimed_mmio_check = 0;
>   }
> +
> + spin_unlock(&uncore->debug->lock);
>  }
> 
> -static void mmio_debug_resume(struct intel_uncore_mmio_debug
> *mmio_debug)
> +static bool check_for_unclaimed_mmio(struct intel_uncore *uncore);
> +
> +static void mmio_debug_resume(struct intel_uncore *uncore)
>  {
> - lockdep_assert_held(&mmio_debug->lock);
> + spin_lock(&uncore->debug->lock);
> +
> + if (!--uncore->debug->suspend_count)
> + uncore->debug->unclaimed_mmio_check = uncore->debug-
> >saved_mmio_check;
> 
> - if (!--mmio_debug->suspend_count)
> - mmio_debug->unclaimed_mmio_check = mmio_debug-
> >saved_mmio_check;
> + if (check_for_unclaimed_mmio(uncore))
> + drm_info(&uncore->i915->drm,
> +  "Invalid mmio detected during user access\n");
> +
> + spin_unlock(&uncore->debug->lock);
>  }
> 
>  static const char * const forcewake_domain_names[] = {
> @@ -677,9 +687,7 @@ void intel_uncore_forcewake_user_get(struct
> intel_uncore *uncore)
>   spin_lock_irq(&uncore->lock);
>   if (!uncore->user_forcewake_count++) {
>   intel_uncore_forcewake_get__locked(uncore,
> FORCEWAKE_ALL);
> - spin_lock(&uncore->debug->lock);
> - mmio_debug_suspend(uncore->debug);
> - spin_unlock(&uncore->debug->lock);
> + mmio_debug_suspend(uncore);
>   }
>   spin_unlock_irq(&uncore->lock);
>  }
> @@ -695,14 +703,7 @@ void intel_uncore_forcewake_user_put(struct
> intel_uncore *uncore)
>  {
>   spin_lock_irq(&uncore->lock);
>   if (!--uncore->user_forcewake_count) {
> - spin_lock(&uncore->debug->lock);
> - mmio_debug_resume(uncore->debug);
> -
> - if (check_for_unclaimed_mmio(uncore))
> - drm_info(&uncore->i915->drm,
> -  "Invalid mmio detected during user access\n");
> - spin_unlock(&uncore->debug->lock);
> -
> + mmio_debug_resume(uncore);
>   intel_uncore_forcewake_put__locked(uncore,
> FORCEWAKE_ALL);
>   }
>   spin_unlock_irq(&uncore->lock);
> --
> 2.37.2