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

[Jani Nikula <jani.nikula@linux.intel.com>]

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.

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
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx