| Message ID | 20190820230544.170010-8-stuart.summers@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor to expand subslice mask (rev 2) | expand |
Quoting Stuart Summers (2019-08-21 00:05:40) > Add a subslice stride calculation when setting subslices. This > aligns more closely with the userspace expectation of the subslice > mask structure. > > v2: Use local variable for subslice_mask on HSW and > clean up a few other subslice_mask local variable > changes > v3: Add GEM_BUG_ON for ss_stride to prevent array overflow (Chris) > Split main set function and refactors in intel_device_info.c > into separate patches (Chris) > > Signed-off-by: Stuart Summers <stuart.summers@intel.com> > --- > drivers/gpu/drm/i915/gt/intel_sseu.c | 12 ++++++++++-- > drivers/gpu/drm/i915/gt/intel_sseu.h | 2 +- > 2 files changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_sseu.c b/drivers/gpu/drm/i915/gt/intel_sseu.c > index 79a9b5f186f9..5d537ec97fcc 100644 > --- a/drivers/gpu/drm/i915/gt/intel_sseu.c > +++ b/drivers/gpu/drm/i915/gt/intel_sseu.c > @@ -31,9 +31,17 @@ intel_sseu_subslice_total(const struct sseu_dev_info *sseu) > } > > void intel_sseu_set_subslices(struct sseu_dev_info *sseu, int slice, > - u8 ss_mask) > + u32 ss_mask) > { > - sseu->subslice_mask[i] = ss_mask & 0xff; > + int i, offset; > + > + GEM_BUG_ON(sseu->ss_stride > 32); > + > + offset = slice * sseu->ss_stride; > + > + for (i = 0; i < sseu->ss_stride; i++) You are saying that i can be a maximum of 32, and u32 >> (8 * 32) is legal? > + sseu->subslice_mask[offset + i] = > + (ss_mask >> (BITS_PER_BYTE * i)) & 0xff; The 0xff is, and was, superfluous. Shrug. -Chris
On Wed, 2019-08-21 at 23:36 +0100, Chris Wilson wrote: > Quoting Stuart Summers (2019-08-21 00:05:40) > > Add a subslice stride calculation when setting subslices. This > > aligns more closely with the userspace expectation of the subslice > > mask structure. > > > > v2: Use local variable for subslice_mask on HSW and > > clean up a few other subslice_mask local variable > > changes > > v3: Add GEM_BUG_ON for ss_stride to prevent array overflow (Chris) > > Split main set function and refactors in intel_device_info.c > > into separate patches (Chris) > > > > Signed-off-by: Stuart Summers <stuart.summers@intel.com> > > --- > > drivers/gpu/drm/i915/gt/intel_sseu.c | 12 ++++++++++-- > > drivers/gpu/drm/i915/gt/intel_sseu.h | 2 +- > > 2 files changed, 11 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/gt/intel_sseu.c > > b/drivers/gpu/drm/i915/gt/intel_sseu.c > > index 79a9b5f186f9..5d537ec97fcc 100644 > > --- a/drivers/gpu/drm/i915/gt/intel_sseu.c > > +++ b/drivers/gpu/drm/i915/gt/intel_sseu.c > > @@ -31,9 +31,17 @@ intel_sseu_subslice_total(const struct > > sseu_dev_info *sseu) > > } > > > > void intel_sseu_set_subslices(struct sseu_dev_info *sseu, int > > slice, > > - u8 ss_mask) > > + u32 ss_mask) > > { > > - sseu->subslice_mask[i] = ss_mask & 0xff; > > + int i, offset; > > + > > + GEM_BUG_ON(sseu->ss_stride > 32); > > + > > + offset = slice * sseu->ss_stride; > > + > > + for (i = 0; i < sseu->ss_stride; i++) > > You are saying that i can be a maximum of 32, and > u32 >> (8 * 32) is legal? Yeah the stride is calculated as a div-by-8, so really this should be max of 4, not 32. I'll fix this and repost. > > > + sseu->subslice_mask[offset + i] = > > + (ss_mask >> (BITS_PER_BYTE * i)) & 0xff; > > The 0xff is, and was, superfluous. Shrug. I agree in the previous patch this wasn't really interesting. I can remove that. But here, IMO, having the explicit AND makes this clearer. Thanks, Stuart > -Chris
diff --git a/drivers/gpu/drm/i915/gt/intel_sseu.c b/drivers/gpu/drm/i915/gt/intel_sseu.c index 79a9b5f186f9..5d537ec97fcc 100644 --- a/drivers/gpu/drm/i915/gt/intel_sseu.c +++ b/drivers/gpu/drm/i915/gt/intel_sseu.c @@ -31,9 +31,17 @@ intel_sseu_subslice_total(const struct sseu_dev_info *sseu) } void intel_sseu_set_subslices(struct sseu_dev_info *sseu, int slice, - u8 ss_mask) + u32 ss_mask) { - sseu->subslice_mask[i] = ss_mask & 0xff; + int i, offset; + + GEM_BUG_ON(sseu->ss_stride > 32); + + offset = slice * sseu->ss_stride; + + for (i = 0; i < sseu->ss_stride; i++) + sseu->subslice_mask[offset + i] = + (ss_mask >> (BITS_PER_BYTE * i)) & 0xff; } unsigned int diff --git a/drivers/gpu/drm/i915/gt/intel_sseu.h b/drivers/gpu/drm/i915/gt/intel_sseu.h index 3099e04611ce..2261d4e7d98b 100644 --- a/drivers/gpu/drm/i915/gt/intel_sseu.h +++ b/drivers/gpu/drm/i915/gt/intel_sseu.h @@ -76,7 +76,7 @@ unsigned int intel_sseu_subslices_per_slice(const struct sseu_dev_info *sseu, u8 slice); void intel_sseu_set_subslices(struct sseu_dev_info *sseu, int slice, - u8 ss_mask); + u32 ss_mask); u32 intel_sseu_make_rpcs(struct drm_i915_private *i915, const struct intel_sseu *req_sseu);
Add a subslice stride calculation when setting subslices. This aligns more closely with the userspace expectation of the subslice mask structure. v2: Use local variable for subslice_mask on HSW and clean up a few other subslice_mask local variable changes v3: Add GEM_BUG_ON for ss_stride to prevent array overflow (Chris) Split main set function and refactors in intel_device_info.c into separate patches (Chris) Signed-off-by: Stuart Summers <stuart.summers@intel.com> --- drivers/gpu/drm/i915/gt/intel_sseu.c | 12 ++++++++++-- drivers/gpu/drm/i915/gt/intel_sseu.h | 2 +- 2 files changed, 11 insertions(+), 3 deletions(-)