Re: [igt-dev] [PATCH i-g-t v15 14/15] Verify protected surfaces are dma buffer sharable

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Wed, 2021-10-06 at 16:00 +0000, Teres Alexis, Alan Previn wrote:
> My bad - still learning.. Here is my explanation which is not an
> excuse (i.e. I will create a separate JIRA):
> 
> IIRC correctly, I was looking for examples from other test code and
> used what I saw in gem_userptr_blits.c because all other PXP subtests
> needed the IOCTL return values (testing different failure values
> depending on inputs). I thought it would be better to start with that
> (instead of the ioctl_wrapper lib that didn't return the value).
> 
> However, in hindsight, what I should have done is modified the
> wrapper to export the variant I was looking for (also updating
> gem_userptr_blits too)... and even after realizing I didn't actually
> need the return value, it would at least still be using the wrapper.
> 
> I'll ensure I follow up on this as a separate patch if it got merged.

Yes, the patches got merged. Please provide a follow-up.

My bad for not noticing during review as well.

Thank you Michael.

> 
> ...alan
> 
> -----Original Message-----
> From: Ruhl, Michael J <michael.j.ruhl@intel.com>
> Sent: Wednesday, October 6, 2021 7:31 AM
> To: Teres Alexis, Alan Previn <alan.previn.teres.alexis@intel.com>;
> igt-dev@lists.freedesktop.org
> Cc: Teres Alexis, Alan Previn <alan.previn.teres.alexis@intel.com>;
> Vivi, Rodrigo <rodrigo.vivi@intel.com>
> Subject: RE: [igt-dev] [PATCH i-g-t v15 14/15] Verify protected
> surfaces are dma buffer sharable
> 
> > -----Original Message-----
> > From: igt-dev <igt-dev-bounces@lists.freedesktop.org> On Behalf Of
> > Alan
> > Previn
> > Sent: Tuesday, October 5, 2021 3:42 PM
> > To: igt-dev@lists.freedesktop.org
> > Cc: Teres Alexis, Alan Previn <alan.previn.teres.alexis@intel.com>;
> > Vivi, Rodrigo <rodrigo.vivi@intel.com>
> > Subject: [igt-dev] [PATCH i-g-t v15 14/15] Verify protected
> > surfaces
> > are dma buffer sharable
> > 
> > Verify we can export a protected surface from protected context A
> > into
> > protected context B (with different client driver handles) and
> > protected rendering is successful even after prior file handle is
> > closed (i.e. pxp specific refcounting works).
> > 
> > Signed-off-by: Alan Previn <alan.previn.teres.alexis@intel.com>
> > Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > ---
> > tests/i915/gem_pxp.c | 102
> > +++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 102 insertions(+)
> > 
> > diff --git a/tests/i915/gem_pxp.c b/tests/i915/gem_pxp.c index
> > 0ccba6aa..2848e487 100644
> > --- a/tests/i915/gem_pxp.c
> > +++ b/tests/i915/gem_pxp.c
> > @@ -621,6 +621,106 @@ static void
> > test_render_pxp_protsrc_to_protdest(int i915) 
> > buf_ops_destroy(bops);
> > }
> > 
> > +static int export_handle(int fd, uint32_t handle, int *outfd) {
> > struct
> > +drm_prime_handle args; int ret;
> > +
> > +args.handle = handle;
> > +args.flags = DRM_CLOEXEC;
> > +args.fd = -1;
> > +
> > +ret = drmIoctl(fd, DRM_IOCTL_PRIME_HANDLE_TO_FD, &args); if (ret)
> > ret
> > += errno; *outfd = args.fd;
> > +
> > +return ret;
> > +}
> 
> This is a repeat of similar function in lib/ioctl_wrappers
> (prime_handle_to_fd)
> 
> Is there a reason for the code duplication?
> 
> Especially confused because you are using prime_fd_to_handle for
> import...
> 
> Thanks,
> 
> M
> 
> 
> > +static void test_pxp_dmabuffshare_refcnt(void)
> > +{
> > +uint32_t ctx[2], sbo[2], dbo[2];
> > +struct intel_buf *sbuf[2], *dbuf[2];
> > +struct buf_ops *bops[2];
> > +struct intel_bb *ibb[2];
> > +int fd[2], dmabuf_fd = 0, ret, n, num_matches = 0; uint32_t
> > +encrypted[2][TSTSURF_SIZE/TSTSURF_BYTESPP];
> > +
> > +/* First, create the client driver handles and
> > + * protected dest buffer (is exported via dma-buff
> > + * from first handle and imported to the second).
> > + */
> > +for (n = 0; n < 2; ++n) {
> > +fd[n] = drm_open_driver(DRIVER_INTEL); igt_require(fd[n]); if (n
> > == 0)
> > +{ dbo[0] = alloc_and_fill_dest_buff(fd[0], true,
> > TSTSURF_SIZE,
> > +
> > TSTSURF_INITCOLOR1);
> > +} else {
> > +ret = export_handle(fd[0], dbo[0], &dmabuf_fd);
> > +igt_assert(ret == 0);
> > +dbo[1] = prime_fd_to_handle(fd[1], dmabuf_fd);
> > +igt_assert(dbo[1]);
> > +}
> > +}
> > +/* Repeat twice: Create a full set of assets to perform
> > + * a protected 3D session but using the same dest buffer
> > + * from above.
> > + */
> > +for (n = 0; n < 2; ++n) {
> > +ret = create_ctx_with_params(fd[n], true, true, true, false,
> > &ctx[n]);
> > +igt_assert_eq(ret, 0);
> > +igt_assert_eq(get_ctx_protected_param(fd[n], ctx[n]), 1);
> > +ibb[n] = intel_bb_create_with_context(fd[n], ctx[n], 4096);
> > +intel_bb_set_pxp(ibb[n], true, DISPLAY_APPTYPE,
> > +
> > I915_PROTECTED_CONTENT_DEFAULT_SESSION);
> > +
> > +bops[n] = buf_ops_create(fd[n]);
> > +if (n == 1)
> > +fill_bo_content(fd[1], dbo[1], TSTSURF_SIZE,
> > TSTSURF_INITCOLOR2);
> > +
> > +dbuf[n] = intel_buf_create_using_handle(bops[n], dbo[n],
> > TSTSURF_WIDTH,
> > +TSTSURF_HEIGHT,
> > TSTSURF_BYTESPP*8, 0,
> > +I915_TILING_NONE,
> > 0);
> > +intel_buf_set_pxp(dbuf[n], true);
> > +
> > +sbo[n] = alloc_and_fill_dest_buff(fd[n], false, TSTSURF_SIZE,
> > TSTSURF_FILLCOLOR1);
> > +sbuf[n] = intel_buf_create_using_handle(bops[n], sbo[n],
> > TSTSURF_WIDTH,
> > +TSTSURF_HEIGHT,
> > TSTSURF_BYTESPP*8, 0,
> > +I915_TILING_NONE,
> > 0);
> > +
> > +gen12_render_copyfunc(ibb[n], sbuf[n], 0, 0,
> > TSTSURF_WIDTH, TSTSURF_HEIGHT,
> > +      dbuf[n], 0, 0);
> > +gem_sync(fd[n], dbo[n]);
> > +
> > +assert_bo_content_check(fd[n], dbo[n],
> > COMPARE_COLOR_UNREADIBLE, TSTSURF_SIZE,
> > +TSTSURF_FILLCOLOR1, NULL, 0);
> > +assert_bo_content_check(fd[n], dbo[n], COPY_BUFFER,
> > TSTSURF_SIZE, 0, encrypted[n],
> > +TSTSURF_SIZE);
> > +
> > +/* free up all assets except the dest buffer to
> > + * verify dma buff refcounting is performed on
> > + * the protected dest buffer on the 2nd loop with
> > + * successful reuse in another protected render.
> > + */
> > +intel_bb_destroy(ibb[n]);
> > +intel_buf_destroy(sbuf[n]);
> > +intel_buf_destroy(dbuf[n]);
> > +gem_close(fd[n], sbo[n]);
> > +gem_close(fd[n], dbo[n]);
> > +gem_context_destroy(fd[n], ctx[n]);
> > +close(fd[n]);
> > +}
> > +
> > +/* Verify that encrypted output across loops were equal */
> > +for (n = 0; n < (TSTSURF_SIZE/4); ++n)
> > +if (encrypted[0][n] == encrypted[1][n])
> > +++num_matches;
> > +igt_assert(num_matches == (TSTSURF_SIZE/4));
> > +}
> > +
> > +
> > static void init_powermgt_resources(int i915, struct powermgt_data
> > *pm)
> > {
> > pm->debugfsdir = igt_debugfs_dir(i915);
> > @@ -989,6 +1089,8 @@ igt_main
> > test_render_pxp_src_to_protdest(i915);
> > igt_subtest("protected-encrypted-src-copy-not-readible")
> > test_render_pxp_protsrc_to_protdest(i915);
> > +igt_subtest("dmabuf-shared-protected-dst-is-context-
> > refcounted")
> > +test_pxp_dmabuffshare_refcnt();
> > }
> > igt_subtest_group {
> > igt_fixture {
> > --
> > 2.25.1
> 
>