Re: [PATCH 03/10] drm/i915: Use local variable for subslice_mask on HSW

From: Stuart Summers <stuart.summers@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 03/10] drm/i915: Use local variable for subslice_mask on HSW
Date: Fri, 02 Aug 2019 16:52:38 -0700	[thread overview]
Message-ID: <1564789958.4037.3.camel@intel.com> (raw)
In-Reply-To: <156478649348.12110.10159115356763914574@skylake-alporthouse-com>

On Fri, 2019-08-02 at 23:54 +0100, Chris Wilson wrote:
> Quoting Stuart Summers (2019-08-02 23:47:00)
> > On Fri, 2019-08-02 at 22:28 +0100, Chris Wilson wrote:
> > > Quoting Stuart Summers (2019-08-02 21:51:27)
> > > > Instead of assuming a single slice on HSW when defining
> > > > subslices for the platform, use a local variable to set
> > > > the maximum subslices per slice.
> > > > 
> > > > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_device_info.c | 11 ++++++-----
> > > >  1 file changed, 6 insertions(+), 5 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_device_info.c
> > > > b/drivers/gpu/drm/i915/intel_device_info.c
> > > > index 9a79d9d547c5..2b81cc731fa2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > > > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > > > @@ -541,6 +541,7 @@ static void haswell_sseu_info_init(struct
> > > > drm_i915_private *dev_priv)
> > > >  {
> > > >         struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)-
> > > > >sseu;
> > > >         u32 fuse1;
> > > > +       u8 subslice_mask;
> > > >         int s, ss;
> > > >  
> > > >         /*
> > > > @@ -553,16 +554,16 @@ static void haswell_sseu_info_init(struct
> > > > drm_i915_private *dev_priv)
> > > >                 /* fall through */
> > > >         case 1:
> > > >                 sseu->slice_mask = BIT(0);
> > > > -               sseu->subslice_mask[0] = BIT(0);
> > > > +               subslice_mask = BIT(0);
> > > >                 break;
> > > >         case 2:
> > > >                 sseu->slice_mask = BIT(0);
> > > > -               sseu->subslice_mask[0] = BIT(0) | BIT(1);
> > > > +               subslice_mask = BIT(0) | BIT(1);
> > > >                 break;
> > > >         case 3:
> > > >                 sseu->slice_mask = BIT(0) | BIT(1);
> > > > -               sseu->subslice_mask[0] = BIT(0) | BIT(1);
> > > > -               sseu->subslice_mask[1] = BIT(0) | BIT(1);
> > > > +               subslice_mask = BIT(0) | BIT(1);
> > > > +               subslice_mask = BIT(0) | BIT(1);
> > > 
> > > This is definitely not a single slice.
> > 
> > Thanks for the note Chris. Very true and my commit message is
> > misleading. Do you have any issue with the code changes I'm making
> > here? Or simply the commit message? I'll adjust the commit message
> > in
> > the next revision.
> 
> The duplication looks very wrong, just remove one of them and the
> reader
> isn't left wondering why??? At the moment, it makes me question
> whether
> there is loss of information with an incomplete subslice_mask[].

Bah, obvious mistake on my part here. Thanks for pointing this out.
I'll clean this up in the next revision.

Thanks,
Stuart

> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx