On Wed, 2019-05-29 at 17:58 +0300, Jani Nikula wrote:
> On Fri, 24 May 2019, Stuart Summers <stuart.summers@xxxxxxxxx> 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@xxxxxxxxx>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> > Acked-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> > Reviewed-by: Daniele Ceraolo Spurio <
> > daniele.ceraolospurio@xxxxxxxxx>
> > Signed-off-by: Stuart Summers <stuart.summers@xxxxxxxxx>
>
> 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:
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
