[03/10] drm/i915: Use local variable for subslice_mask on HSW
diff mbox series

Message ID 20190802205134.303-4-stuart.summers@intel.com
State New
Headers show
Series
  • Refactor to expand subslice mask (rev 2)
Related show

Commit Message

Stuart Summers Aug. 2, 2019, 8:51 p.m. UTC
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(-)

Comments

Chris Wilson Aug. 2, 2019, 9:28 p.m. UTC | #1
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.
-Chris
Stuart Summers Aug. 2, 2019, 10:47 p.m. UTC | #2
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.

Thanks,
Stuart

> -Chris
Chris Wilson Aug. 2, 2019, 10:54 p.m. UTC | #3
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[].
-Chris
Stuart Summers Aug. 2, 2019, 11:52 p.m. UTC | #4
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

Patch
diff mbox series

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);
 		break;
 	}
 
@@ -584,7 +585,7 @@  static void haswell_sseu_info_init(struct drm_i915_private *dev_priv)
 	}
 
 	intel_sseu_set_info(sseu, hweight8(sseu->slice_mask),
-			    hweight8(sseu->subslice_mask[0]),
+			    hweight8(subslice_mask),
 			    sseu->eu_per_subslice);
 
 	for (s = 0; s < sseu->max_slices; s++) {