On Fri, Oct 08, 2021 at 12:49:57PM +0300, Ville Syrjälä wrote:
> On Thu, Sep 30, 2021 at 05:58:16PM -0700, Matt Roper wrote:
> > The I915_TILING_* definitions in the uapi header are intended solely for
> > tiling modes that are visible to the old de-tiling fence ioctls. Since
> > modern hardware does not support de-tiling fences, we should not add new
> > definitions for new tiling types going forward. However we do want the
> > client blit selftest to eventually cover other new tiling modes (such as
> > Tile4), so switch it to using its own enum of tiling modes.
> >
> > Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> > Cc: Stanislav Lisovskiy <stanislav.lisovskiy@xxxxxxxxx>
> > Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
> > ---
> > .../i915/gem/selftests/i915_gem_client_blt.c | 29 ++++++++++++-------
> > include/uapi/drm/i915_drm.h | 6 ++++
> > 2 files changed, 24 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c b/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
> > index ecbcbb86ae1e..8402ed925a69 100644
> > --- a/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
> > +++ b/drivers/gpu/drm/i915/gem/selftests/i915_gem_client_blt.c
> > @@ -17,13 +17,20 @@
> > #include "huge_gem_object.h"
> > #include "mock_context.h"
> >
> > +enum client_tiling {
> > + CLIENT_TILING_LINEAR,
> > + CLIENT_TILING_X,
> > + CLIENT_TILING_Y,
> > + CLIENT_NUM_TILING_TYPES
> > +};
> > +
> > #define WIDTH 512
> > #define HEIGHT 32
> >
> > struct blit_buffer {
> > struct i915_vma *vma;
> > u32 start_val;
> > - u32 tiling;
> > + enum client_tiling tiling;
> > };
> >
> > struct tiled_blits {
> > @@ -53,9 +60,9 @@ static int prepare_blit(const struct tiled_blits *t,
> > *cs++ = MI_LOAD_REGISTER_IMM(1);
> > *cs++ = i915_mmio_reg_offset(BCS_SWCTRL);
> > cmd = (BCS_SRC_Y | BCS_DST_Y) << 16;
> > - if (src->tiling == I915_TILING_Y)
> > + if (src->tiling == CLIENT_TILING_Y)
> > cmd |= BCS_SRC_Y;
> > - if (dst->tiling == I915_TILING_Y)
> > + if (dst->tiling == CLIENT_TILING_Y)
> > cmd |= BCS_DST_Y;
> > *cs++ = cmd;
> >
> > @@ -172,7 +179,7 @@ static int tiled_blits_create_buffers(struct tiled_blits *t,
> >
> > t->buffers[i].vma = vma;
> > t->buffers[i].tiling =
> > - i915_prandom_u32_max_state(I915_TILING_Y + 1, prng);
> > + i915_prandom_u32_max_state(CLIENT_TILING_Y + 1, prng);
> > }
> >
> > return 0;
> > @@ -197,17 +204,17 @@ static u64 swizzle_bit(unsigned int bit, u64 offset)
> > static u64 tiled_offset(const struct intel_gt *gt,
> > u64 v,
> > unsigned int stride,
> > - unsigned int tiling)
> > + enum client_tiling tiling)
> > {
> > unsigned int swizzle;
> > u64 x, y;
> >
> > - if (tiling == I915_TILING_NONE)
> > + if (tiling == CLIENT_TILING_LINEAR)
> > return v;
> >
> > y = div64_u64_rem(v, stride, &x);
> >
> > - if (tiling == I915_TILING_X) {
> > + if (tiling == CLIENT_TILING_X) {
> > v = div64_u64_rem(y, 8, &y) * stride * 8;
> > v += y * 512;
> > v += div64_u64_rem(x, 512, &x) << 12;
> > @@ -244,12 +251,12 @@ static u64 tiled_offset(const struct intel_gt *gt,
> > return v;
> > }
> >
> > -static const char *repr_tiling(int tiling)
> > +static const char *repr_tiling(enum client_tiling tiling)
> > {
> > switch (tiling) {
> > - case I915_TILING_NONE: return "linear";
> > - case I915_TILING_X: return "X";
> > - case I915_TILING_Y: return "Y";
> > + case CLIENT_TILING_LINEAR: return "linear";
> > + case CLIENT_TILING_X: return "X";
> > + case CLIENT_TILING_Y: return "Y";
> > default: return "unknown";
> > }
> > }
> > diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> > index bde5860b3686..00311a63068e 100644
> > --- a/include/uapi/drm/i915_drm.h
> > +++ b/include/uapi/drm/i915_drm.h
> > @@ -1522,6 +1522,12 @@ struct drm_i915_gem_caching {
> > #define I915_TILING_NONE 0
> > #define I915_TILING_X 1
> > #define I915_TILING_Y 2
> > +/*
> > + * Do not add new tiling types here. The I915_TILING_* values are for
> > + * de-tiling fence registers that no longer exist on modern platforms. Although
> > + * the hardware may support new types of tiling in general (e.g., Tile4), we
> > + * do not need to add them to the uapi that is specific to now-defunct ioctls.
> > + */
> > #define I915_TILING_LAST I915_TILING_Y
>
> I think we should split this one into a separate patch to give it
> some visibility. The people who care about gem uapi seem to be in
> some kind of early winter hibernation and no one read this.
>
> Apart from that
> Reviewed-by: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
Thanks. I dropped the comment here and pushed the rest of the patch.
I'll re-send the uapi header comment separately to increase visibility.
Matt
>
> >
> > #define I915_BIT_6_SWIZZLE_NONE 0
> > --
> > 2.33.0
>
> --
> Ville Syrjälä
> Intel
--
Matt Roper
Graphics Software Engineer
VTT-OSGC Platform Enablement
Intel Corporation
(916) 356-2795
