[igt-dev] [PATCH i-g-t] tools/amd_hdmi_compliance_yuv420: Test 4K video modes with YUV encoding
Petri Latvala
petri.latvala at intel.com
Fri Sep 13 09:57:45 UTC 2019
On Wed, Sep 11, 2019 at 02:29:51PM +0000, Kazlauskas, Nicholas wrote:
> On 2019-09-11 10:11 a.m., Wang, Chao-kai (Stylon) wrote:
> > From: Stylon Wang <stylon.wang at amd.com>
> >
> > HDMI 2.0 compliance tests needs 4K modes with YUV encoding.
> > This test selects 4K modes based on specified VIC.
>
> It helps to note what changed in a patch series in both the patch title
> and in the commit message here.
>
> You can add a v2 to the patch header by appending a -v2 to your command, eg:
>
> "git send-email -v2 ..."
>
> And in the commit description you should do something like:
>
> v2:
> - example change 1
> - example change 2
>
> Other than that, I do have some general comments and nitpicks:
>
> I think this tool might be better off named as just amd_hdmi_compliance
> so we can expand on this later.
Is there anything specific that makes this tool AMD-specific? It even
uses DRIVER_ANY.
I guess the same question can be asked about intel_dp_compliance.c...
--
Petri Latvala
>
> Nicholas Kazlauskas
>
> >
> > Signed-off-by: Stylon Wang <stylon.wang at amd.com>
> > ---
> > tools/Makefile.am | 3 +
> > tools/Makefile.sources | 4 +
> > tools/amd_hdmi_compliance_yuv420.c | 193 +++++++++++++++++++++++++++++
> > tools/meson.build | 5 +
> > 4 files changed, 205 insertions(+)
> > create mode 100644 tools/amd_hdmi_compliance_yuv420.c
> >
> > diff --git a/tools/Makefile.am b/tools/Makefile.am
> > index 4f54720f..1a23e078 100644
> > --- a/tools/Makefile.am
> > +++ b/tools/Makefile.am
> > @@ -11,6 +11,9 @@ bin_PROGRAMS += intel_dp_compliance
> > intel_dp_compliance_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
> > intel_dp_compliance_LDADD = $(top_builddir)/lib/libintel_tools.la
> >
> > +bin_PROGRAMS += amd_hdmi_compliance_yuv420
> > +amd_hdmi_compliance_yuv420_CFLAGS = $(AM_CFLAGS) $(GLIB_CFLAGS)
> > +
> > SUBDIRS = null_state_gen registers
> >
> > AM_CPPFLAGS = -I$(top_srcdir) -I$(top_srcdir)/include/drm-uapi -I$(top_srcdir)/lib \
> > diff --git a/tools/Makefile.sources b/tools/Makefile.sources
> > index 50706f41..8196cb59 100644
> > --- a/tools/Makefile.sources
> > +++ b/tools/Makefile.sources
> > @@ -66,3 +66,7 @@ intel_dp_compliance_SOURCES = \
> > intel_dp_compliance_hotplug.c \
> > $(NULL)
> >
> > +amd_hdmi_compliance_yuv420_SOURCES = \
> > + amd_hdmi_compliance_yuv420.c \
> > + $(NULL)
> > +
> > diff --git a/tools/amd_hdmi_compliance_yuv420.c b/tools/amd_hdmi_compliance_yuv420.c
> > new file mode 100644
> > index 00000000..5f5eae2c
> > --- /dev/null
> > +++ b/tools/amd_hdmi_compliance_yuv420.c
> > @@ -0,0 +1,193 @@
> > +/*
> > + * Copyright 2019 Advanced Micro Devices, Inc.
> > + *
> > + * Permission is hereby granted, free of charge, to any person obtaining a
> > + * copy of this software and associated documentation files (the "Software"),
> > + * to deal in the Software without restriction, including without limitation
> > + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> > + * and/or sell copies of the Software, and to permit persons to whom the
> > + * Software is furnished to do so, subject to the following conditions:
> > + *
> > + * The above copyright notice and this permission notice shall be included in
> > + * all copies or substantial portions of the Software.
> > + *
> > + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> > + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> > + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
> > + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR
> > + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE,
> > + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
> > + * OTHER DEALINGS IN THE SOFTWARE.
> > + */
> > +
> > +#include "igt.h"
> > +
> > +/* Common test data */
> > +typedef struct data {
> > + struct igt_fb pattern_fb_info;
> > + int fd;
> > + igt_display_t display;
> > + igt_plane_t *primary;
> > + igt_output_t *output;
> > + igt_pipe_t *pipe;
> > + enum pipe pipe_id;
> > + bool use_virtual_connector;
> > +} data_t;
> > +
> > +// Video modes indexed by VIC
>
> IGT uses kernel style formatting so this comment should be:
>
> /* Video modes indexed by VIC */
>
> > +static drmModeModeInfo test_modes[] = {
> > + [1] = { 25175,
> > + 640, 656, 752, 800, 0,
> > + 480, 489, 492, 525, 0,
> > + 60, 0xa, 0x40,
> > + "640x480", /* VIC 1 */
> > + },
> > + [96] = { 594000,
> > + 3840, 4896, 4984, 5280, 0,
> > + 2160, 2168, 2178, 2250, 0,
> > + 50, 0x5|DRM_MODE_FLAG_PIC_AR_16_9, 0x40,
> > + "3840x2160", /* VIC 96 */
> > + },
> > + [97] = { 594000,
> > + 3840, 4016, 4104, 4400, 0,
> > + 2160, 2168, 2178, 2250, 0,
> > + 60, 0x5|DRM_MODE_FLAG_PIC_AR_16_9, 0x40,
> > + "3840x2160", /* VIC 97 */
> > + },
> > + [101] = { 594000,
> > + 4096, 5064, 5152, 5280, 0,
> > + 2160, 2168, 2178, 2250, 0,
> > + 50, 0x5|DRM_MODE_FLAG_PIC_AR_256_135, 0x40,
> > + "4096x2160", /* VIC 101 */
> > + },
> > + [102] = { 594000,
> > + 4096, 4184, 4272, 4400, 0,
> > + 2160, 2168, 2178, 2250, 0,
> > + 60, 0x5|DRM_MODE_FLAG_PIC_AR_256_135, 0x40,
> > + "4096x2160", /* VIC 102 */
> > + },
> > + [106] = { 594000,
> > + 3840, 4896, 4984, 5280, 0,
> > + 2160, 2168, 2178, 2250, 0,
> > + 50, 0x5|DRM_MODE_FLAG_PIC_AR_64_27, 0x40,
> > + "3840x2160", /* VIC 106 */
> > + },
> > + [107] = { 594000,
> > + 3840, 4016, 4104, 4400, 0,
> > + 2160, 2168, 2178, 2250, 0,
> > + 60, 0x5|DRM_MODE_FLAG_PIC_AR_64_27, 0x40,
> > + "3840x2160", /* VIC 107 */
> > + },
> > +};
> > +
> > +/* Common test setup. */
> > +static void test_init(data_t *data)
> > +{
> > + igt_display_t *display = &data->display;
> > +
> > + data->pipe_id = PIPE_A;
> > + data->pipe = &data->display.pipes[data->pipe_id];
> > +
> > + igt_display_reset(display);
> > +
> > + /* find a connected HDMI output */
> > + data->output = NULL;
> > + for (int i=0; i < data->display.n_outputs; ++i) {
> > + drmModeConnector *connector = data->display.outputs[i].config.connector;
> > + if (connector->connection == DRM_MODE_CONNECTED &&
> > + (connector->connector_type == DRM_MODE_CONNECTOR_HDMIA ||
> > + (data->use_virtual_connector &&
> > + connector->connector_type == DRM_MODE_CONNECTOR_VIRTUAL))) {
> > + data->output = &data->display.outputs[i];
> > + }
> > + }
> > +
> > + igt_require(data->output);
> > +
> > + data->primary =
> > + igt_pipe_get_plane_type(data->pipe, DRM_PLANE_TYPE_PRIMARY);
> > +
> > + igt_output_set_pipe(data->output, data->pipe_id);
> > +
> > +}
> > +
> > +/* Common test cleanup. */
> > +static void test_fini(data_t *data)
> > +{
> > + igt_display_reset(&data->display);
> > +}
> > +
> > +static void test_vic_mode(data_t *data, int vic)
> > +{
> > + igt_display_t *display = &data->display;
> > + drmModeModeInfo *mode;
> > + igt_fb_t afb;
> > +
> > + test_init(data);
> > +
> > + mode = &test_modes[vic];
> > +
> > + igt_output_override_mode(data->output, mode);
> > +
> > + igt_create_pattern_fb(data->fd, mode->hdisplay, mode->vdisplay, DRM_FORMAT_XRGB8888, 0, &afb);
> > +
> > + igt_plane_set_fb(data->primary, &afb);
> > +
> > + igt_display_commit_atomic(display, DRM_MODE_ATOMIC_ALLOW_MODESET, NULL);
> > +
> > + fprintf(stderr, "Press [Enter] to finish\n");
>
> I think igt_info(...) is preferred here over fprintf directly.
>
> > + getchar();
> I understand that you aren't using igt_debug_wait_for_keypress here, but
> I think an improvement that could be made is to extract this out into a
> sub function that waits explicitly for the newline:
>
> static void wait_for_keypress(void)
> {
> while (getchar() != '\n')
> ;
> }
>
> > +
> > + test_fini(data);
> > +}
> > +
> > +const char *optstr = "hvt:";
> > +static void usage(const char *name)
> > +{
> > + fprintf(stderr, "Usage: %s [-ht]\n", name);
> > + fprintf(stderr, "-h: show help\n");
> > + fprintf(stderr, "-t vic: select video mode based on VIC\n");
> > + fprintf(stderr, "-v: test on 'Virtual' connector as well, for debugging.\n");
>
> igt_info(...) if possible here
>
> > +}
> > +
> > +int main(int argc, char **argv)
> > +{
> > + data_t data;
> > + int c;
> > + int vic = 1; /* default to VIC 1 (640x480) */
> > +
> > + memset(&data, 0, sizeof(data));
> > +
> > + while((c = getopt(argc, argv, optstr)) != -1) {
> > + switch(c) {
> > + case 't':
> > + vic = atoi(optarg);
> > + break;
> > + case 'v':
> > + data.use_virtual_connector = true;
> > + break;
> > + default:
> > + case 'h':
> > + usage(argv[0]);
> > + exit(1);
> > + }
> > + }
> > +
> > + if (vic < 1 ||
> > + vic > ARRAY_SIZE(test_modes) ||
> > + !test_modes[vic].name[0]) {
> > + fprintf(stderr, "VIC %d is not supported\n", vic);
>
> igt_info(...) if possible here
>
> > + exit(1);
> > + }
> > +
> > + data.fd = drm_open_driver_master(DRIVER_ANY);
> > + kmstest_set_vt_graphics_mode();
> > +
> > + igt_display_require(&data.display, data.fd);
> > + igt_require(data.display.is_atomic);
> > + igt_display_require_output(&data.display);
> > +
> > + test_vic_mode(&data, vic);
> > +
> > + igt_display_fini(&data.display);
> > +}
> > diff --git a/tools/meson.build b/tools/meson.build
> > index 6e72b263..fa366ab2 100644
> > --- a/tools/meson.build
> > +++ b/tools/meson.build
> > @@ -100,6 +100,11 @@ executable('intel_gpu_top', 'intel_gpu_top.c',
> > install_rpath : bindir_rpathdir,
> > dependencies : lib_igt_perf)
> >
> > +executable('amd_hdmi_compliance_yuv420', 'amd_hdmi_compliance_yuv420.c',
> > + dependencies : [tool_deps],
> > + install_rpath : bindir_rpathdir,
> > + install : true)
> > +
> > conf_data = configuration_data()
> > conf_data.set('prefix', prefix)
> > conf_data.set('exec_prefix', '${prefix}')
> >
>
> _______________________________________________
> igt-dev mailing list
> igt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
More information about the igt-dev
mailing list