Re: [PATCH i-g-t v2 9/9] tests/intel/xe_pxp: Test encrypted FBs

["Teres Alexis, Alan Previn" <alan.previn.teres.alexis@intel.com>]

LGTM - similiar flow to the i915's version (which is expected since i915 and Xe
both are KMS based implementations on the display side of things).

Appreciate the render-copy taking src from the reference FB (as opposed to a temp
buffer that just happens to be initialized to the same color unlike the i915 way).

I have one feedback on abstracting some repeating sequence into a helper,
but given its minimal value, I consider that a nit. Thus:

Reviewed-by: Alan Previn <alan.previn.teres.alexis@intel.com>

On Wed, 2025-01-15 at 16:19 -0800, Daniele Ceraolo Spurio wrote:
> PXP allows a user to send an encrypted BO to the display HW without
> having to decode it. The driver needs however to tell the HW that the
> BO is encrypted, otherwise it won't be displayed correctly. Furthermore,
> if PXP is terminated before the FB is displayed, we expect to see a black
> screen instead of what's in the BO.
> 
> 
alan:snip

> +static void test_display_pxp_fb(int fd, igt_display_t *display)
> +{
> +       igt_output_t *output;
> +       drmModeModeInfo *mode;
> +       igt_fb_t ref_fb, pxp_fb;
> +       igt_plane_t *plane;
> +       igt_pipe_t *pipe;
> +       int width = 0, height = 0, i = 0;
> +       uint32_t q;
> +       uint32_t vm;
> +
> +       vm = xe_vm_create(fd, 0, 0);
> +       q = create_pxp_rcs_queue(fd, vm); /* start the PXP session */
> +
> +       for_each_connected_output(display, output) {
> +               mode = igt_output_get_mode(output);
> +
> +               width = max_t(int, width, mode->hdisplay);
> +               height = max_t(int, height, mode->vdisplay);
> +       }
> +
> +       igt_create_color_fb(fd, width, height, DRM_FORMAT_XRGB8888, DRM_FORMAT_MOD_LINEAR,
> +                           0, 1, 0, &ref_fb);
> +
> +       /* Do a modeset on all outputs */
> +       for_each_connected_output(display, output) {
> +               mode = igt_output_get_mode(output);
> +               pipe = &display->pipes[i];
> +               plane = igt_pipe_get_plane_type(pipe, DRM_PLANE_TYPE_PRIMARY);
> +               igt_require(igt_pipe_connector_valid(i, output));
> +               igt_output_set_pipe(output, i);
> +
> +               igt_plane_set_fb(plane, &ref_fb);
> +               igt_fb_set_size(&ref_fb, plane, mode->hdisplay, mode->vdisplay);
> +               igt_plane_set_size(plane, mode->hdisplay, mode->vdisplay);
> +
> +               igt_display_commit2(display, COMMIT_ATOMIC);
alan: nit: I see an opportunity for optimization by adding a local helper for the following sequence of KMS calls that
seems to be replicated 3 times for this test:
		igt_plane_set_fb(plane, fb);
		igt_fb_set_size(fb, plane, height, width);
		igt_plane_set_size(plane, height, width);
		igt_display_commit2(display, COMMIT_ATOMIC);

> +               i++;
> +       }
> +
> +       /* Create an encrypted FB with the same contents as ref_fb */
> +       setup_protected_fb_from_ref(fd, &ref_fb, &pxp_fb, q, vm);
> +
> +       /* Flip both FBs and make sure the CRCs match */
> +       compare_crcs(fd, display, &ref_fb, &pxp_fb);
> +
> +       igt_remove_fb(fd, &ref_fb);
> +       igt_remove_fb(fd, &pxp_fb);
> +       xe_exec_queue_destroy(fd, q);
> +       xe_vm_destroy(fd, vm);
> +}
> +
> +/**
> + * SUBTEST: display-black-pxp-fb
> + * Description: Test that an invalid encrypted fb is correctly converted to a
> + *              black screen by comparing its CRCs with the ones generated by a
> + *              non-encrypted FB filled with black
> + */
> +
> +static void test_display_black_pxp_fb(int fd, igt_display_t *display)

alan:snip