Re: [CI 5/5] drm/i915: Expand subslice mask

["Summers, Stuart" <stuart.summers@intel.com>]

[-- Attachment #1.1: Type: text/plain, Size: 3657 bytes --]

On Wed, 2019-05-29 at 17:58 +0300, Jani Nikula wrote:
> On Fri, 24 May 2019, Stuart Summers <stuart.summers@intel.com> wrote:
> > Currently, the subslice_mask runtime parameter is stored as an
> > array of subslices per slice. Expand the subslice mask array to
> > better match what is presented to userspace through the
> > I915_QUERY_TOPOLOGY_INFO ioctl. The index into this array is
> > then calculated:
> >   slice * subslice stride + subslice index / 8
> > 
> > v2: fix spacing in set_sseu_info args
> >     use set_sseu_info to initialize sseu data when building
> >     device status in debugfs
> >     rename variables in intel_engine_types.h to avoid checkpatch
> >     warnings
> > v3: update headers in intel_sseu.h
> > v4: add const to some sseu_dev_info variables
> >     use sseu->eu_stride for EU stride calculations
> > v5: address review comments from Tvrtko and Daniele
> > v6: remove extra space in intel_sseu_get_subslices
> >     return the correct subslice enable in for_each_instdone
> >     add GEM_BUG_ON to ensure user doesn't pass invalid ss_mask size
> >     use printk formatted string for subslice mask
> > v7: remove string.h header and rebase
> > 
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Acked-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Reviewed-by: Daniele Ceraolo Spurio <
> > daniele.ceraolospurio@intel.com>
> > Signed-off-by: Stuart Summers <stuart.summers@intel.com>
> 
> As this patch uncovered a latent issue in 1e40d4aea57b
> ("drm/i915/cnl:
> Implement WaProgramMgsrForCorrectSliceSpecificMmioReads") and got
> reverted, I'll take the opportunity to comment. I acknowledge the
> revert
> is shooting the messenger a bit, and this will smell like maintainer
> bikeshedding.

I have no problem reworking regressions my series caused :) I just
wasn't aware there was an issue when merging.

> 
> Now, the first reaction looking at the commit was, it does not fare
> well
> on the "if a bisect landed on this commit, how happy would I be"
> scale.
> 
> While it's mostly refactoring, it could be chopped up to several
> logical
> and obvious steps. For example, add intel_sseu_set_info() first with
> no
> other changes. Add ss_stride and eu_stride to struct sseu_dev_info
> separately. Add intel_sseu_get_subslices() but don't expand yet, make
> it
> just sseu->subslice_mask[s] first. And so on, you get the idea, a
> series
> of small non-functional changes followed by patches with functional
> changes that stand out. Indeed patches 1-4 did this fine.

This is good general feedback, thanks. I'll split this up when
reposting.

> 
> It's easy on the reviewer, it's easy on whoever git blames years down
> the line. Trust me, we will.
> 
> And it would be the commit adding intel_sseu_get_subslices(), or the
> one
> adding the GEM_BUG_ON()s into it, that would blow up 1e40d4aea57b
> ("drm/i915/cnl: Implement
> WaProgramMgsrForCorrectSliceSpecificMmioReads").
> 
> One more note below.
> 
> > @@ -461,7 +461,9 @@ static int i915_getparam_ioctl(struct
> > drm_device *dev, void *data,
> >  			return -ENODEV;
> >  		break;
> >  	case I915_PARAM_SUBSLICE_MASK:
> > -		value = sseu->subslice_mask[0];
> > +		/* Only copy bits from the first slice */
> > +		memcpy(&value, sseu->subslice_mask,
> > +		       min(sseu->ss_stride, (u8)sizeof(value)));
> 
> Frankly I'd rather see this written in self-evident code without the
> comment.

Sure, I'll take a look.

Thanks for the comments!
Stuart

> 
> BR,
> Jani.
> 
> 

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 3270 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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