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.
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.
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.
BR,
Jani.
--
Jani Nikula, Intel Open Source Graphics Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
