From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> To: Matt Roper <matthew.d.roper@intel.com> Cc: Jani Nikula <jani.nikula@intel.com>, Intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, Tvrtko Ursulin <tvrtko.ursulin@intel.com> Subject: Re: [PATCH 1/2] drm/i915: Don't use DRM_DEBUG_WARN_ON for unexpected l3bank/mslice config Date: Thu, 5 May 2022 12:02:45 +0100 [thread overview] Message-ID: <a746320b-8431-a171-4c73-3ed64eedc726@linux.intel.com> (raw) In-Reply-To: <YnLDMANc6xdnjOdy@mdroper-desk1.amr.corp.intel.com> On 04/05/2022 19:17, Matt Roper wrote: > On Wed, May 04, 2022 at 06:59:32PM +0100, Tvrtko Ursulin wrote: >> >> On 04/05/2022 17:48, Matt Roper wrote: >>> On Wed, May 04, 2022 at 01:07:14PM +0100, Tvrtko Ursulin wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> DRM_DEBUG_WARN_ON should only be used when we are certain CI is guaranteed >>>> to exercise a certain code path, so in case of values coming from MMIO >>>> reads we cannot be sure CI will have all the possible SKUs and parts. >>>> >>>> Use drm_warn instead and move logging to init phase while at it. >>> >>> Changing to drm_warn looks good, although moving the location changes >>> the intent a bit; I think originally the idea was to warn if we were >>> trying to do a steering lookup for a type that we never initialized >>> (e.g., an LNCF lookup for a !HAS_MSLICES platform where we never even >>> read the register in the first place). But I don't think we've ever >>> made a mistake that would cause us to trip the warning, so it probably >>> isn't terribly important to keep it there. >> >> Ah I see.. there we could put something like: >> >> case MSLICE: >> GEM_WARN_ON(!HAS_MSLICES(...)); >> > > Yeah, that would work for MSLICE and LNCF. Although L3BANK is a bit > stranger since we have multiple platforms that obtain the L3 bank mask > in completely different ways (Xe_HP reads it from XEHP_FUSE4, whereas > gen11/gen12 reads it from GEN10_MIRROR_FUSE3). We want to make sure > there that no matter which branch of init we take, we didn't forget to > initialize l3bank_mask somehow. The two init paths are not something present in drm-tip at this point, right? At least I couldn't find it. In which case it could be handled later by moving the drm_warn to tail of intel_gt_init_mmio, give or take. Anyway, I've sent v2 out with your r-b and GEM_WARN_ON for mslice/lncf. I won't merge it though until you definitely are okay with it so please have a look and confirm. Regards, Tvrtko > > Matt > >> ? >> >> Regards, >> >> Tvrtko >> >>> >>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >>> >>>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> Cc: Matt Roper <matthew.d.roper@intel.com> >>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/gt/intel_gt.c | 13 ++++++------- >>>> 1 file changed, 6 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c >>>> index 53307ca0eed0..c474e5c3ea5e 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c >>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c >>>> @@ -153,11 +153,14 @@ int intel_gt_init_mmio(struct intel_gt *gt) >>>> * An mslice is unavailable only if both the meml3 for the slice is >>>> * disabled *and* all of the DSS in the slice (quadrant) are disabled. >>>> */ >>>> - if (HAS_MSLICES(i915)) >>>> + if (HAS_MSLICES(i915)) { >>>> gt->info.mslice_mask = >>>> slicemask(gt, GEN_DSS_PER_MSLICE) | >>>> (intel_uncore_read(gt->uncore, GEN10_MIRROR_FUSE3) & >>>> GEN12_MEML3_EN_MASK); >>>> + if (!gt->info.mslice_mask) /* should be impossible! */ >>>> + drm_warn(&i915->drm, "mslice mask all zero!\n"); >>>> + } >>>> if (IS_DG2(i915)) { >>>> gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table; >>>> @@ -171,6 +174,8 @@ int intel_gt_init_mmio(struct intel_gt *gt) >>>> gt->info.l3bank_mask = >>>> ~intel_uncore_read(gt->uncore, GEN10_MIRROR_FUSE3) & >>>> GEN10_L3BANK_MASK; >>>> + if (!gt->info.l3bank_mask) /* should be impossible! */ >>>> + drm_warn(&i915->drm, "L3 bank mask is all zero!\n"); >>>> } else if (HAS_MSLICES(i915)) { >>>> MISSING_CASE(INTEL_INFO(i915)->platform); >>>> } >>>> @@ -882,20 +887,14 @@ static void intel_gt_get_valid_steering(struct intel_gt *gt, >>>> { >>>> switch (type) { >>>> case L3BANK: >>>> - GEM_DEBUG_WARN_ON(!gt->info.l3bank_mask); /* should be impossible! */ >>>> - >>>> *sliceid = 0; /* unused */ >>>> *subsliceid = __ffs(gt->info.l3bank_mask); >>>> break; >>>> case MSLICE: >>>> - GEM_DEBUG_WARN_ON(!gt->info.mslice_mask); /* should be impossible! */ >>>> - >>>> *sliceid = __ffs(gt->info.mslice_mask); >>>> *subsliceid = 0; /* unused */ >>>> break; >>>> case LNCF: >>>> - GEM_DEBUG_WARN_ON(!gt->info.mslice_mask); /* should be impossible! */ >>>> - >>>> /* >>>> * An LNCF is always present if its mslice is present, so we >>>> * can safely just steer to LNCF 0 in all cases. >>>> -- >>>> 2.32.0 >>>> >>> >
WARNING: multiple messages have this Message-ID (diff)
From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> To: Matt Roper <matthew.d.roper@intel.com> Cc: Jani Nikula <jani.nikula@intel.com>, Intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Don't use DRM_DEBUG_WARN_ON for unexpected l3bank/mslice config Date: Thu, 5 May 2022 12:02:45 +0100 [thread overview] Message-ID: <a746320b-8431-a171-4c73-3ed64eedc726@linux.intel.com> (raw) In-Reply-To: <YnLDMANc6xdnjOdy@mdroper-desk1.amr.corp.intel.com> On 04/05/2022 19:17, Matt Roper wrote: > On Wed, May 04, 2022 at 06:59:32PM +0100, Tvrtko Ursulin wrote: >> >> On 04/05/2022 17:48, Matt Roper wrote: >>> On Wed, May 04, 2022 at 01:07:14PM +0100, Tvrtko Ursulin wrote: >>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> >>>> DRM_DEBUG_WARN_ON should only be used when we are certain CI is guaranteed >>>> to exercise a certain code path, so in case of values coming from MMIO >>>> reads we cannot be sure CI will have all the possible SKUs and parts. >>>> >>>> Use drm_warn instead and move logging to init phase while at it. >>> >>> Changing to drm_warn looks good, although moving the location changes >>> the intent a bit; I think originally the idea was to warn if we were >>> trying to do a steering lookup for a type that we never initialized >>> (e.g., an LNCF lookup for a !HAS_MSLICES platform where we never even >>> read the register in the first place). But I don't think we've ever >>> made a mistake that would cause us to trip the warning, so it probably >>> isn't terribly important to keep it there. >> >> Ah I see.. there we could put something like: >> >> case MSLICE: >> GEM_WARN_ON(!HAS_MSLICES(...)); >> > > Yeah, that would work for MSLICE and LNCF. Although L3BANK is a bit > stranger since we have multiple platforms that obtain the L3 bank mask > in completely different ways (Xe_HP reads it from XEHP_FUSE4, whereas > gen11/gen12 reads it from GEN10_MIRROR_FUSE3). We want to make sure > there that no matter which branch of init we take, we didn't forget to > initialize l3bank_mask somehow. The two init paths are not something present in drm-tip at this point, right? At least I couldn't find it. In which case it could be handled later by moving the drm_warn to tail of intel_gt_init_mmio, give or take. Anyway, I've sent v2 out with your r-b and GEM_WARN_ON for mslice/lncf. I won't merge it though until you definitely are okay with it so please have a look and confirm. Regards, Tvrtko > > Matt > >> ? >> >> Regards, >> >> Tvrtko >> >>> >>> Reviewed-by: Matt Roper <matthew.d.roper@intel.com> >>> >>>> >>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> >>>> Cc: Matt Roper <matthew.d.roper@intel.com> >>>> Cc: Jani Nikula <jani.nikula@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/gt/intel_gt.c | 13 ++++++------- >>>> 1 file changed, 6 insertions(+), 7 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/gt/intel_gt.c b/drivers/gpu/drm/i915/gt/intel_gt.c >>>> index 53307ca0eed0..c474e5c3ea5e 100644 >>>> --- a/drivers/gpu/drm/i915/gt/intel_gt.c >>>> +++ b/drivers/gpu/drm/i915/gt/intel_gt.c >>>> @@ -153,11 +153,14 @@ int intel_gt_init_mmio(struct intel_gt *gt) >>>> * An mslice is unavailable only if both the meml3 for the slice is >>>> * disabled *and* all of the DSS in the slice (quadrant) are disabled. >>>> */ >>>> - if (HAS_MSLICES(i915)) >>>> + if (HAS_MSLICES(i915)) { >>>> gt->info.mslice_mask = >>>> slicemask(gt, GEN_DSS_PER_MSLICE) | >>>> (intel_uncore_read(gt->uncore, GEN10_MIRROR_FUSE3) & >>>> GEN12_MEML3_EN_MASK); >>>> + if (!gt->info.mslice_mask) /* should be impossible! */ >>>> + drm_warn(&i915->drm, "mslice mask all zero!\n"); >>>> + } >>>> if (IS_DG2(i915)) { >>>> gt->steering_table[MSLICE] = xehpsdv_mslice_steering_table; >>>> @@ -171,6 +174,8 @@ int intel_gt_init_mmio(struct intel_gt *gt) >>>> gt->info.l3bank_mask = >>>> ~intel_uncore_read(gt->uncore, GEN10_MIRROR_FUSE3) & >>>> GEN10_L3BANK_MASK; >>>> + if (!gt->info.l3bank_mask) /* should be impossible! */ >>>> + drm_warn(&i915->drm, "L3 bank mask is all zero!\n"); >>>> } else if (HAS_MSLICES(i915)) { >>>> MISSING_CASE(INTEL_INFO(i915)->platform); >>>> } >>>> @@ -882,20 +887,14 @@ static void intel_gt_get_valid_steering(struct intel_gt *gt, >>>> { >>>> switch (type) { >>>> case L3BANK: >>>> - GEM_DEBUG_WARN_ON(!gt->info.l3bank_mask); /* should be impossible! */ >>>> - >>>> *sliceid = 0; /* unused */ >>>> *subsliceid = __ffs(gt->info.l3bank_mask); >>>> break; >>>> case MSLICE: >>>> - GEM_DEBUG_WARN_ON(!gt->info.mslice_mask); /* should be impossible! */ >>>> - >>>> *sliceid = __ffs(gt->info.mslice_mask); >>>> *subsliceid = 0; /* unused */ >>>> break; >>>> case LNCF: >>>> - GEM_DEBUG_WARN_ON(!gt->info.mslice_mask); /* should be impossible! */ >>>> - >>>> /* >>>> * An LNCF is always present if its mslice is present, so we >>>> * can safely just steer to LNCF 0 in all cases. >>>> -- >>>> 2.32.0 >>>> >>> >
next prev parent reply other threads:[~2022-05-05 11:02 UTC|newest] Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-05-04 12:07 [PATCH 1/2] drm/i915: Don't use DRM_DEBUG_WARN_ON for unexpected l3bank/mslice config Tvrtko Ursulin 2022-05-04 12:07 ` [Intel-gfx] " Tvrtko Ursulin 2022-05-04 12:07 ` [PATCH 2/2] drm/i915: Don't use DRM_DEBUG_WARN_ON for ring unexpectedly not idle Tvrtko Ursulin 2022-05-04 12:07 ` [Intel-gfx] " Tvrtko Ursulin 2022-05-05 8:02 ` Mika Kuoppala 2022-05-05 8:02 ` [Intel-gfx] " Mika Kuoppala 2022-05-04 13:59 ` [Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Don't use DRM_DEBUG_WARN_ON for unexpected l3bank/mslice config Patchwork 2022-05-04 16:48 ` [PATCH 1/2] " Matt Roper 2022-05-04 16:48 ` [Intel-gfx] " Matt Roper 2022-05-04 17:59 ` Tvrtko Ursulin 2022-05-04 17:59 ` [Intel-gfx] " Tvrtko Ursulin 2022-05-04 18:17 ` Matt Roper 2022-05-04 18:17 ` [Intel-gfx] " Matt Roper 2022-05-05 11:02 ` Tvrtko Ursulin [this message] 2022-05-05 11:02 ` Tvrtko Ursulin 2022-05-05 16:05 ` Matt Roper 2022-05-05 16:05 ` [Intel-gfx] " Matt Roper 2022-05-04 20:43 ` [Intel-gfx] ✓ Fi.CI.IGT: success for series starting with [1/2] " Patchwork
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=a746320b-8431-a171-4c73-3ed64eedc726@linux.intel.com \ --to=tvrtko.ursulin@linux.intel.com \ --cc=Intel-gfx@lists.freedesktop.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=jani.nikula@intel.com \ --cc=matthew.d.roper@intel.com \ --cc=tvrtko.ursulin@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.