[igt-dev] [PATCH i-g-t 1/2] tests/kms_plane: optimize pixel format tests
Juha-Pekka Heikkila
juhapekka.heikkila at gmail.com
Fri Nov 27 19:53:34 UTC 2020
On 27.11.2020 17.26, Ville Syrjälä wrote:
> On Wed, Nov 25, 2020 at 09:50:50PM +0200, Juha-Pekka Heikkila wrote:
>> On packed formats there's no need to show separate colors on separate fbs.
>> Here augmented path with packed color handling which check colors just on
>> one fb.
>>
>> This cannot be directly applied to planar yuv formats because of scaler
>> use with those formats.
>
> Not entirely sure we can use this for any subsampled format. I guess
> the fact that you used horizontal lines does make it work for 4:2:2
> formats now, but aren't there potentially (on other hw) packed
> YCbCr formats with vertical subsampling?
I used entire fb wide lines because of subsampled formats. There
probably could exist vertically subsampled formats but I'd assume
supporting such need other changes to igt core parts too? You have some
specific case in mind? Maybe such formats will need a bit in format
definition indicating subsampling direction which would be used here.
>
>>
>> On my ICL this drop test execution time from 44.91s to 21.98s
>>
>> Signed-off-by: Juha-Pekka Heikkila <juhapekka.heikkila at gmail.com>
>> ---
>> tests/kms_plane.c | 88 ++++++++++++++++++++++++++++++++++++++---------
>> 1 file changed, 71 insertions(+), 17 deletions(-)
>>
>> diff --git a/tests/kms_plane.c b/tests/kms_plane.c
>> index 4ce859fba..749a75eee 100644
>> --- a/tests/kms_plane.c
>> +++ b/tests/kms_plane.c
>> @@ -466,7 +466,8 @@ static void prepare_format_color(data_t *data, enum pipe pipe,
>> int width, int height,
>> enum igt_color_encoding color_encoding,
>> enum igt_color_range color_range,
>> - const color_t *c, struct igt_fb *fb)
>> + const color_t *c, struct igt_fb *fb,
>> + bool singlefbtest)
>> {
>> cairo_t *cr;
>>
>> @@ -480,6 +481,15 @@ static void prepare_format_color(data_t *data, enum pipe pipe,
>> igt_paint_color(cr, 0, 0, width, height,
>> c->red, c->green, c->blue);
>>
>> + if (singlefbtest) {
>> + for (int n = 0; n < data->num_colors; n++) {
>> + igt_paint_color(cr, 0, n * 4, fb->width, 2,
>> + data->colors[n].red,
>> + data->colors[n].green,
>> + data->colors[n].blue);
>> + }
>> + }
>> +
>> igt_put_cairo_ctx(cr);
>> } else {
>> igt_create_fb_with_bo_size(data->drm_fd,
>> @@ -505,6 +515,16 @@ static void prepare_format_color(data_t *data, enum pipe pipe,
>> width, height,
>> c->red, c->green, c->blue);
>>
>> + if (singlefbtest) {
>> + int inc = format != DRM_FORMAT_XRGB8888 ? data->crop : 0;
>> + for (int n = 0; n < data->num_colors; n++) {
>> + igt_paint_color(cr, 0, inc + n * 4, fb->width, 2,
>> + data->colors[n].red,
>> + data->colors[n].green,
>> + data->colors[n].blue);
>
> Seems like a good candidate for a new helper function
>
> And I think I would just make it use extended_colors unconditionally,
> and probably render the lines a bit thicker (eg. just
> fb->height/num_ext_colors) to make it possible to actually see the
> individual lines on high res displays.
I did like even numbers for thickness, with filling up entire fb I had
concern on slowing down those older platforms where primary plane has to
be mode size. I could make drawing height unconditionally 64 pixel tall
without too much worry.
Black lines between colors is legacy of ideas to get also planar formats
working. Planar formats could be used this way if involved another plane
with alpha as black mask but I though it probably is not such a great idea.
>
>> + }
>> + }
>> +
>> igt_put_cairo_ctx(cr);
>> }
>>
>> @@ -548,13 +568,35 @@ static void capture_crc(data_t *data, unsigned int vblank, igt_crc_t *crc)
>> crc->frame, vblank);
>> }
>>
>> -static void capture_format_crcs(data_t *data, enum pipe pipe,
>> - igt_plane_t *plane,
>> - uint32_t format, uint64_t modifier,
>> - int width, int height,
>> - enum igt_color_encoding encoding,
>> - enum igt_color_range range,
>> - igt_crc_t crc[], struct igt_fb *fb)
>> +static void capture_format_crcs_packed(data_t *data, enum pipe pipe,
>> + igt_plane_t *plane,
>> + uint32_t format, uint64_t modifier,
>> + int width, int height,
>> + enum igt_color_encoding encoding,
>> + enum igt_color_range range,
>> + igt_crc_t crc[], struct igt_fb *fb)
>> +{
>> + struct igt_fb old_fb = *fb;
>> + const color_t black = { 0.0f, 0.0f, 0.0f };
>> +
>> + prepare_format_color(data, pipe, plane, format, modifier,
>> + width, height, encoding, range, &black, fb, true);
>> +
>> + igt_display_commit_atomic(&data->display, DRM_MODE_ATOMIC_ALLOW_MODESET,
>> + NULL);
>> +
>> + igt_remove_fb(data->drm_fd, &old_fb);
>> +
>> + igt_pipe_crc_get_current(data->drm_fd, data->pipe_crc, &crc[0]);
>> +}
>> +
>> +static void capture_format_crcs_planar(data_t *data, enum pipe pipe,
>> + igt_plane_t *plane,
>> + uint32_t format, uint64_t modifier,
>> + int width, int height,
>> + enum igt_color_encoding encoding,
>> + enum igt_color_range range,
>> + igt_crc_t crc[], struct igt_fb *fb)
>> {
>> unsigned int vblank[ARRAY_SIZE(colors_extended)];
>> struct drm_event_vblank ev;
>> @@ -567,7 +609,7 @@ restart_round:
>> int ret;
>>
>> prepare_format_color(data, pipe, plane, format, modifier,
>> - width, height, encoding, range, c, fb);
>> + width, height, encoding, range, c, fb, false);
>>
>> if (data->display.is_atomic && i >= 1) {
>> igt_assert(read(data->drm_fd, &ev, sizeof(ev)) == sizeof(ev));
>> @@ -684,12 +726,20 @@ static bool test_format_plane_colors(data_t *data, enum pipe pipe,
>> int crc_mismatch_count = 0;
>> bool result = true;
>> int i;
>> + bool planar = igt_format_is_yuv_semiplanar(format);
>>
>> - capture_format_crcs(data, pipe, plane, format, modifier,
>> - width, height, encoding, range, crc, fb);
>> + if (planar)
>> + capture_format_crcs_planar(data, pipe, plane, format, modifier,
>> + width, height, encoding, range, crc,
>> + fb);
>> + else
>> + capture_format_crcs_packed(data, pipe, plane, format, modifier,
>> + width, height, encoding, range, crc,
>> + fb);
>>
>> - for (i = 0; i < data->num_colors; i++) {
>> - if (!igt_check_crc_equal(&crc[i], &ref_crc[i])) {
>> + for (i = 0; i < (planar ? data->num_colors : 1); i++) {
>
> Can't we just make num_colors correct? Ie. make it 1 for packed formats.
>
>> +
>> + if (!igt_check_crc_equal(&crc[i], &ref_crc[i + (planar ? 1 : 0)])) {
>
> This ref_crc indexing is rather ugly. I guess I would add a small helper
> for it to at least hide it better.
I'll rework these bits a bit and put another version. Thx for the comments.
>
>> crc_mismatch_count++;
>> crc_mismatch_mask |= (1 << i);
>> result = false;
>> @@ -784,7 +834,7 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
>> uint32_t format, ref_format;
>> uint64_t modifier, ref_modifier;
>> uint64_t width, height;
>> - igt_crc_t ref_crc[ARRAY_SIZE(colors_extended)];
>> + igt_crc_t ref_crc[ARRAY_SIZE(colors_extended) + 1];
>> bool result = true;
>>
>> /*
>> @@ -843,9 +893,13 @@ static bool test_format_plane(data_t *data, enum pipe pipe,
>> igt_remove_fb(data->drm_fd, &test_fb);
>> }
>>
>> - capture_format_crcs(data, pipe, plane, format, modifier,
>> - width, height, IGT_COLOR_YCBCR_BT709,
>> - IGT_COLOR_YCBCR_LIMITED_RANGE, ref_crc, &fb);
>> + capture_format_crcs_packed(data, pipe, plane, format, modifier,
>> + width, height, IGT_COLOR_YCBCR_BT709,
>> + IGT_COLOR_YCBCR_LIMITED_RANGE, ref_crc, &fb);
>> +
>> + capture_format_crcs_planar(data, pipe, plane, format, modifier,
>> + width, height, IGT_COLOR_YCBCR_BT709,
>> + IGT_COLOR_YCBCR_LIMITED_RANGE, &ref_crc[1], &fb);
>>
>> /*
>> * Make sure we have some difference between the colors. This
>> --
>> 2.28.0
>>
>> _______________________________________________
>> 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