All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Andi Kleen <ak@linux.intel.com>
Cc: airlied@linux.ie, intel-gfx@lists.freedesktop.org,
	Andi Kleen <andi@firstfloor.org>,
	dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] i915: Increase *_latency array size
Date: Wed, 5 May 2021 17:25:27 +0300	[thread overview]
Message-ID: <YJKq15HgIWQqFeho@intel.com> (raw)
In-Reply-To: <20210505141830.GP4032392@tassilo.jf.intel.com>

On Wed, May 05, 2021 at 07:18:30AM -0700, Andi Kleen wrote:
> > > Increase all the latency fields to 8 members, which is enough for SKL.
> > >
> > > I don't know if they are correctly initialized upto 8, but dev_priv
> > > should start out as zero, so presumably they will be zero.
> > 
> > Thanks, the warning should be fixed by commit
> > 
> > c6deb5e97ded ("drm/i915/pm: Make the wm parameter of print_wm_latency a pointer")
> > 
> > in drm-intel-next.
> 
> That's just hiding the problem.
> 
> > 
> > There doesn't actually seem to be a bug here,
> 
> Can you explain that please?
> 
> This is the loop in question
> 
>  max_level = ilk_wm_max_level(dev_priv);
> 
>         for (level = 0; level <= max_level; level++) {
>                 unsigned int latency = wm[level];
> 
>                 if (latency == 0) {
>                         drm_dbg_kms(&dev_priv->drm,
>                                     "%s WM%d latency not provided\n",
>                                     name, level);
>                         continue;
>                 }
> 
> 		...
> 	}
> 
> (no other loop termination condition)
> 
> and ilk_wm_max_level is
> 
> int ilk_wm_max_level(const struct drm_i915_private *dev_priv)
> {
>         /* how many WM levels are we expecting */
>         if (INTEL_GEN(dev_priv) >= 9)
>                 return 7;
>         else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>                 return 4;
>         else if (INTEL_GEN(dev_priv) >= 6)
>                 return 3;
>         else
>                 return 2;
> }
> 
> There is no loop termination in the loop above, it will always read
> every member through the max level reported. And on GEN>=9 it will be 7, while
> the input array for several of the cases has only 5 members.
> 
> So it will read beyond the array and gcc is correct in complaining.
> 
> What do I miss when you say there is no bug?

We always use dev_priv->wm.skl_latency[] for gen9+. See
{pri,spr,cur}_wm_latency_show(), skl_setup_wm_latency(), etc.

-- 
Ville Syrjälä
Intel
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

WARNING: multiple messages have this Message-ID (diff)
From: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
To: Andi Kleen <ak@linux.intel.com>
Cc: airlied@linux.ie, intel-gfx@lists.freedesktop.org,
	Andi Kleen <andi@firstfloor.org>,
	dri-devel@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] i915: Increase *_latency array size
Date: Wed, 5 May 2021 17:25:27 +0300	[thread overview]
Message-ID: <YJKq15HgIWQqFeho@intel.com> (raw)
In-Reply-To: <20210505141830.GP4032392@tassilo.jf.intel.com>

On Wed, May 05, 2021 at 07:18:30AM -0700, Andi Kleen wrote:
> > > Increase all the latency fields to 8 members, which is enough for SKL.
> > >
> > > I don't know if they are correctly initialized upto 8, but dev_priv
> > > should start out as zero, so presumably they will be zero.
> > 
> > Thanks, the warning should be fixed by commit
> > 
> > c6deb5e97ded ("drm/i915/pm: Make the wm parameter of print_wm_latency a pointer")
> > 
> > in drm-intel-next.
> 
> That's just hiding the problem.
> 
> > 
> > There doesn't actually seem to be a bug here,
> 
> Can you explain that please?
> 
> This is the loop in question
> 
>  max_level = ilk_wm_max_level(dev_priv);
> 
>         for (level = 0; level <= max_level; level++) {
>                 unsigned int latency = wm[level];
> 
>                 if (latency == 0) {
>                         drm_dbg_kms(&dev_priv->drm,
>                                     "%s WM%d latency not provided\n",
>                                     name, level);
>                         continue;
>                 }
> 
> 		...
> 	}
> 
> (no other loop termination condition)
> 
> and ilk_wm_max_level is
> 
> int ilk_wm_max_level(const struct drm_i915_private *dev_priv)
> {
>         /* how many WM levels are we expecting */
>         if (INTEL_GEN(dev_priv) >= 9)
>                 return 7;
>         else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
>                 return 4;
>         else if (INTEL_GEN(dev_priv) >= 6)
>                 return 3;
>         else
>                 return 2;
> }
> 
> There is no loop termination in the loop above, it will always read
> every member through the max level reported. And on GEN>=9 it will be 7, while
> the input array for several of the cases has only 5 members.
> 
> So it will read beyond the array and gcc is correct in complaining.
> 
> What do I miss when you say there is no bug?

We always use dev_priv->wm.skl_latency[] for gen9+. See
{pri,spr,cur}_wm_latency_show(), skl_setup_wm_latency(), etc.

-- 
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2021-05-05 14:25 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-05  3:37 [Intel-gfx] [PATCH] i915: Increase *_latency array size Andi Kleen
2021-05-05  3:37 ` Andi Kleen
2021-05-05  4:23 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2021-05-05  5:43 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2021-05-05  6:32 ` [Intel-gfx] [PATCH] " Jani Nikula
2021-05-05  6:32   ` Jani Nikula
2021-05-05 14:18   ` [Intel-gfx] " Andi Kleen
2021-05-05 14:18     ` Andi Kleen
2021-05-05 14:25     ` Ville Syrjälä [this message]
2021-05-05 14:25       ` [Intel-gfx] " Ville Syrjälä
2021-05-06 10:23       ` Jani Nikula
2021-05-06 10:23         ` Jani Nikula

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=YJKq15HgIWQqFeho@intel.com \
    --to=ville.syrjala@linux.intel.com \
    --cc=airlied@linux.ie \
    --cc=ak@linux.intel.com \
    --cc=andi@firstfloor.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=intel-gfx@lists.freedesktop.org \
    /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: link
Be 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.