[igt-dev] [PATCH i-g-t v2] lib: Remove deprecated interfaces from huc_copy for Gen12+ platforms.
Kamil Konieczny
kamil.konieczny at linux.intel.com
Mon Nov 21 13:50:08 UTC 2022
Hi Tony,
On 2022-11-18 at 10:25:40 -0800, Ye, Tony wrote:
>
> On 11/17/2022 7:52 AM, Kamil Konieczny wrote:
> > Hi,
> >
> > please change subject of this commit, there are no changes
> > in old huc_copy in this patch. Also remove dot from end of
> > the Subject: line, s/platforms./platforms/
> >
> > On 2022-11-16 at 16:07:06 +0530, Vikas Srivastava wrote:
> > > From: "Ye, Tony" <tony.ye at intel.com>
> > >
> > > Stop using relocs and the legacy execbuf flags.
> > You should put here a little more explanation, as I see this it
> > is rather
> >
> > Add new huc_copy for gen12+ with no-relocs and no legacy flags
> >
> > and there are no changes in old code (except for adding that
> > AT_LEAST_GEN(12)).
>
> The patch is to switch to softpin from relocations in the huc_copy test on
> Gen12+. The old platforms stay with relocations. See the change in the
> igt_get_huc_copyfunc function.
Thank you for explanation, it was not clear from commit message,
so maybe Vikas will add more explanations here.
>
>
> >
> > > Signed-off-by: "Ye, Tony" <tony.ye at intel.com>
> > > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> > > Acked-by: Priyanka Dandamudi <priyanka.dandamudi at intel.com>
> > >
> > > ---
> > > lib/huc_copy.c | 77 +++++++++++++++++++++++++++++++++++++++++
> > > lib/huc_copy.h | 4 +++
> > > lib/intel_batchbuffer.c | 8 +++--
> > > 3 files changed, 87 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/huc_copy.c b/lib/huc_copy.c
> > > index 6ec68864b..3b011a305 100644
> > > --- a/lib/huc_copy.c
> > > +++ b/lib/huc_copy.c
> > > @@ -26,6 +26,8 @@
> > > #include "drmtest.h"
> > > #include "huc_copy.h"
> > > #include "intel_allocator.h"
> > > +#include "intel_ctx.h"
> > > +#include "i915/gem_engine_topology.h"
> > > static void
> > > gen9_emit_huc_virtual_addr_state(struct drm_i915_gem_exec_object2 *src,
> > > @@ -123,3 +125,78 @@ gen9_huc_copyfunc(int fd, uint64_t ahnd,
> > > gem_execbuf(fd, &execbuf);
> > > }
> > > +
> > > +static void
> > > +gen12_emit_huc_virtual_addr_state(struct drm_i915_gem_exec_object2 *src,
> > > + struct drm_i915_gem_exec_object2 *dst,
> > > + uint32_t *buf,
> > > + int *i)
> > > +{
> > > + buf[(*i)++] = HUC_VIRTUAL_ADDR_STATE;
> > > +
> > > + for (int j = 0; j < HUC_VIRTUAL_ADDR_REGION_NUM; j++) {
> > > + if (j == HUC_VIRTUAL_ADDR_REGION_SRC) {
> > > + buf[(*i)++] = src->offset;
> > Addr is 48-bit and here we copy only lower 32-bit,
> > this may work with relocs but not with pinned addresses.
>
>
> The test only uses 3 addresses: 2M, 4M, 6M. So 48b flag is not set and we
> only copy the lower 32 bits.
I see, so here we do some simplification.
imho here we should add some asserts to be sure it is 32-bit
or even more, addr + size <= 32-bit.
>
>
> >
> > > + } else if (j == HUC_VIRTUAL_ADDR_REGION_DST) {
> > > + buf[(*i)++] = dst->offset;
> > Same here.
> >
> > > + } else {
> > > + buf[(*i)++] = 0;
> > > + }
> > > + buf[(*i)++] = 0;
> > > + buf[(*i)++] = 0;
> > > + }
> > > +}
> > > +
> > > +void
> > > +gen12_huc_copyfunc(int fd, uint64_t ahnd,
> > > + struct drm_i915_gem_exec_object2 *obj, uint64_t *objsize)
> > > +{
> > > + struct drm_i915_gem_execbuffer2 execbuf;
> > > + int i = 0;
> > > + uint32_t buf[63];
> > > + const intel_ctx_t *ctx;
> > > +
> > > + /* load huc kernel */
> > > + buf[i++] = HUC_IMEM_STATE;
> > > + buf[i++] = 0;
> > > + buf[i++] = 0;
> > > + buf[i++] = 0;
> > > + buf[i++] = 0x3;
> > > +
> > > + buf[i++] = MFX_WAIT;
> > > + buf[i++] = MFX_WAIT;
> > > +
> > > + buf[i++] = HUC_PIPE_MODE_SELECT;
> > > + buf[i++] = 0;
> > > + buf[i++] = 0;
> > > +
> > > + buf[i++] = MFX_WAIT;
> > > +
> > > + obj[0].offset = 0x200000;
> > ----------------------- ^
> > Use ahnd as in gen9_huc_copy (without reloc code).
>
>
> The code here is simply assigning a graphics virtual address to the source
> buffer object.
>
>
Why not just copy code from gen9 into gen12 with some
modifications ?
> > > + obj[0].flags |= EXEC_OBJECT_PINNED;
> > Add also:
> > | EXEC_OBJECT_SUPPORTS_48B_ADDRESS
>
> As explained above, we don't need 48b address.
>
>
> >
> > > + obj[1].offset = 0x400000;
> > Same here.
> >
> > > + obj[1].flags |= EXEC_OBJECT_PINNED;
> > > + obj[1].flags |= EXEC_OBJECT_WRITE;
> > Add also
> > | EXEC_OBJECT_SUPPORTS_48B_ADDRESS
> >
> > and maybe make it one bit-or like:
> > obj[1].flags |= EXEC_OBJECT_PINNED | EXEC_OBJECT_WRITE |
> > EXEC_OBJECT_SUPPORTS_48B_ADDRESS;
> >
> > > + obj[2].offset = 0x600000;
> > Same here.
> >
> > > + obj[2].flags |= EXEC_OBJECT_PINNED;
> > Same here (48-bit).
> >
> > Regards,
> > Kamil
> >
> > > + gen12_emit_huc_virtual_addr_state(&obj[0], &obj[1], buf, &i);
> > > +
> > > + buf[i++] = HUC_START;
> > > + buf[i++] = 1;
> > > +
> > > + buf[i++] = MI_BATCH_BUFFER_END;
> > > +
> > > + gem_write(fd, obj[2].handle, 0, buf, sizeof(buf));
> > > +
> > > + memset(&execbuf, 0, sizeof(execbuf));
> > > + execbuf.buffers_ptr = to_user_pointer(obj);
> > > + execbuf.buffer_count = 3;
> > > + execbuf.flags = I915_EXEC_DEFAULT;
> > > +
> > > + ctx = intel_ctx_create_for_engine(fd, I915_ENGINE_CLASS_VIDEO, 0);
> > > + execbuf.rsvd1 = ctx->id;
> > > +
> > > + gem_execbuf(fd, &execbuf);
> > > +
> > > + intel_ctx_destroy(fd, ctx);
> > > +}
> > > diff --git a/lib/huc_copy.h b/lib/huc_copy.h
> > > index 69d140933..e87e62c9c 100644
> > > --- a/lib/huc_copy.h
> > > +++ b/lib/huc_copy.h
> > > @@ -46,4 +46,8 @@ void
> > > gen9_huc_copyfunc(int fd, uint64_t ahnd,
> > > struct drm_i915_gem_exec_object2 *obj, uint64_t *objsize);
> > > +void
> > > +gen12_huc_copyfunc(int fd, uint64_t ahnd,
> > > + struct drm_i915_gem_exec_object2 *obj, uint64_t *objsize);
> > > +
I spotted one more problem, the objsize[0] (and indexes 1, 2)
are not used here, if we want to use 2M addresses with size 2M
we should also assert objsize[i] <= 2M for i=0,1
For now tests uses only 4K but this is library function so we
should play safe.
Regards,
Kamil
> > > #endif /* HUC_COPY_H */
> > > diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
> > > index 19a1fbe4d..9a855a7f0 100644
> > > --- a/lib/intel_batchbuffer.c
> > > +++ b/lib/intel_batchbuffer.c
> > > @@ -3066,14 +3066,18 @@ void intel_bb_copy_intel_buf(struct intel_bb *ibb,
> > > * Returns:
> > > *
> > > * The platform-specific huc copy function pointer for the device specified
> > > - * with @devid. Will return NULL when no media spin function is implemented.
> > > + * with @devid. Will return NULL when no huc copy function is implemented.
> > > */
> > > igt_huc_copyfunc_t igt_get_huc_copyfunc(int devid)
> > > {
> > > igt_huc_copyfunc_t copy = NULL;
> > > - if (IS_GEN12(devid) || IS_GEN11(devid) || IS_GEN9(devid))
> > > + if (AT_LEAST_GEN(devid, 12))
> > > + copy = gen12_huc_copyfunc;
>
> As mentioned above, we switch to softpin on Gen12+ and stay with old func on
> earlier platforms.
>
> Thanks,
> Tony
>
>
> > > + else if (IS_GEN11(devid) || IS_GEN9(devid))
> > > copy = gen9_huc_copyfunc;
> > > + else
> > > + copy = NULL;
> > > return copy;
> > > }
> > > --
> > > 2.25.1
> > >
More information about the igt-dev
mailing list