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