Re: [igt-dev] [PATCH v2 2/4] lib/igt_fb: Add support for P01x formats, v4.

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

On 7.2.2019 14.10, Maarten Lankhorst wrote:
> Op 07-02-2019 om 12:52 schreef Juha-Pekka Heikkila:
>> On 7.2.2019 11.21, Maarten Lankhorst wrote:
>>> The P01x formats are planar 16 bits per component, with the unused lower bits set to 0.
>>> This means they can all be converted the same way. Only the range is slightly different,
>>> and this is handled in the color_encoding implementation.
>>>
>>> This requires cairo 1.17.2 and pixman 0.36. This works but doesn't give extra precision.
>>> For more than 8 bits precision a few more patches are required to pixman, pending review:
>>> https://lists.freedesktop.org/archives/pixman/2019-January/004815.html
>>> https://lists.freedesktop.org/archives/pixman/2019-January/004809.html
>>>
>>> Once those are merged, we will require the next pixman release for better precision.
>>>
>>> Changes since v1:
>>> - Add fallback color definitions when compiling on cairo version < 1.17.2.
>>> - Skip when FB creation fails on HDR formats, instead of failing.
>>> Changes since v2:
>>> - Complain slightly harder when pixman/cairo are out of date.
>>> - Create a fb with alpha when converting to pixman formats with alpha.
>>> - Oops, s/pixman_format_code_t/cairo_format_t/
>>> Changes since v3:
>>> - Rebase on top of upstream YUV changes.
>>>
>>> Signed-off-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> ---
>>>    configure.ac                  |  11 +-
>>>    include/drm-uapi/drm_fourcc.h |  10 ++
>>>    lib/igt_color_encoding.c      |   4 +
>>>    lib/igt_fb.c                  | 293 +++++++++++++++++++++++++++++++++-
>>>    lib/igt_fb.h                  |   6 +
>>>    meson.build                   |  10 ++
>>>    6 files changed, 329 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/configure.ac b/configure.ac
>>> index b46f024f875a..a333cf8d6dbb 100644
>>> --- a/configure.ac
>>> +++ b/configure.ac
>>> @@ -136,10 +136,17 @@ fi
>>>    PKG_CHECK_MODULES(XRANDR, xrandr >= 1.3, AC_DEFINE(HAVE_XRANDR, 1, [Have libXrandr]), [have_xrandr=no])
>>>      # for testdisplay
>>> -PKG_CHECK_MODULES(CAIRO, [cairo >= 1.12.0])
>>> +PKG_CHECK_MODULES(CAIRO, [cairo >= 1.17.2], [],
>>> +    [AC_MSG_WARN([Cairo too old, HDR formats not available. Upgrade to cairo 1.17.2])
>>> +    PKG_CHECK_MODULES(CAIRO, [cairo >= 1.12.0])]
>>> +)
>>>    PKG_CHECK_MODULES(LIBUDEV, [libudev])
>>>    PKG_CHECK_MODULES(GLIB, [glib-2.0])
>>> -PKG_CHECK_MODULES(PIXMAN, [pixman-1])
>>> +PKG_CHECK_MODULES(PIXMAN, [pixman-1 >= 0.36.0], [], [
>>> +    AC_MSG_WARN([Pixman too old, HDR formats not available. Upgrade to pixman 0.36.0])
>>> +    PKG_CHECK_MODULES(PIXMAN, [pixman-1])
>>> +])
>>> +
>>>    PKG_CHECK_MODULES(GSL, [gsl], [gsl=yes], [gsl=no])
>>>    AM_CONDITIONAL(HAVE_GSL, [test "x$gsl" = xyes])
>>>    diff --git a/include/drm-uapi/drm_fourcc.h b/include/drm-uapi/drm_fourcc.h
>>> index 41106c835747..fe6b66ef1756 100644
>>> --- a/include/drm-uapi/drm_fourcc.h
>>> +++ b/include/drm-uapi/drm_fourcc.h
>>> @@ -195,6 +195,16 @@ extern "C" {
>>>    #define DRM_FORMAT_NV24        fourcc_code('N', 'V', '2', '4') /* non-subsampled Cr:Cb plane */
>>>    #define DRM_FORMAT_NV42        fourcc_code('N', 'V', '4', '2') /* non-subsampled Cb:Cr plane */
>>>    +/*
>>> + * 2 plane YCbCr
>>> + * index 0 = Y plane, [15:0] Y little endian where Pxxx indicate
>>> + * component xxx msb Y [xxx:16-xxx]
>>> + * index 1 = Cr:Cb plane, [31:0] Cr:Cb little endian [xxx:16-xxx:xxx:16-xxx]
>>> + */
>>> +#define DRM_FORMAT_P010        fourcc_code('P', '0', '1', '0') /* 2x2 subsampled Cr:Cb plane, 10 bit per channel */
>>> +#define DRM_FORMAT_P012        fourcc_code('P', '0', '1', '2') /* 2x2 subsampled Cr:Cb plane, 12 bit per channel */
>>> +#define DRM_FORMAT_P016        fourcc_code('P', '0', '1', '6') /* 2x2 subsampled Cr:Cb plane, 16 bit per channel */
>>> +
>>>    /*
>>>     * 3 plane YCbCr
>>>     * index 0: Y plane, [7:0] Y
>>> diff --git a/lib/igt_color_encoding.c b/lib/igt_color_encoding.c
>>> index 4cbe18e217e3..b7a12a1e07f7 100644
>>> --- a/lib/igt_color_encoding.c
>>> +++ b/lib/igt_color_encoding.c
>>> @@ -135,11 +135,15 @@ static const struct color_encoding_format {
>>>        float ofs_y, max_y, ofs_cbcr, mid_cbcr, max_cbcr;
>>>    } formats[] = {
>>>        { DRM_FORMAT_XRGB8888, 255.f, },
>>> +    { IGT_FORMAT_FLOAT, 1.f, },
>>>        { DRM_FORMAT_NV12, 255.f, 16.f, 235.f, 16.f, 128.f, 240.f },
>>>        { DRM_FORMAT_YUYV, 255.f, 16.f, 235.f, 16.f, 128.f, 240.f },
>>>        { DRM_FORMAT_YVYU, 255.f, 16.f, 235.f, 16.f, 128.f, 240.f },
>>>        { DRM_FORMAT_UYVY, 255.f, 16.f, 235.f, 16.f, 128.f, 240.f },
>>>        { DRM_FORMAT_VYUY, 255.f, 16.f, 235.f, 16.f, 128.f, 240.f },
>>> +    { DRM_FORMAT_P010, 65472.f, 4096.f, 60160.f, 4096.f, 32768.f, 61440.f },
>>> +    { DRM_FORMAT_P012, 65520.f, 4096.f, 60160.f, 4096.f, 32768.f, 61440.f },
>>> +    { DRM_FORMAT_P016, 65535.f, 4096.f, 60160.f, 4096.f, 32768.f, 61440.f },
>>>    };
>>>      static const struct color_encoding_format *lookup_fourcc(uint32_t fourcc)
>>> diff --git a/lib/igt_fb.c b/lib/igt_fb.c
>>> index db3ae06748d1..12763cc85c86 100644
>>> --- a/lib/igt_fb.c
>>> +++ b/lib/igt_fb.c
>>> @@ -62,6 +62,20 @@
>>>      #define PIXMAN_invalid    0
>>>    +#if CAIRO_VERSION < CAIRO_VERSION_ENCODE(1, 17, 2)
>>> +/*
>>> + * We need cairo 1.17.2 to use HDR formats, but the only thing added is a value
>>> + * to cairo_format_t.
>>> + *
>>> + * To prevent going outside the enum, make cairo_format_t an int and define
>>> + * ourselves.
>>> + */
>>> +
>>> +#define CAIRO_FORMAT_RGB96F (6)
>>> +#define CAIRO_FORMAT_RGBA128F (7)
>>> +#define cairo_format_t int
>>> +#endif
>>> +
>>>    /* drm fourcc/cairo format maps */
>>>    static const struct format_desc_struct {
>>>        const char *name;
>>> @@ -170,6 +184,25 @@ static const struct format_desc_struct {
>>>          .num_planes = 1, .plane_bpp = { 16, },
>>>          .hsub = 2, .vsub = 1,
>>>        },
>>> +    { .name = "P010", .depth = -1, .drm_id = DRM_FORMAT_P010,
>>> +      .cairo_id = CAIRO_FORMAT_RGB96F,
>>> +      .num_planes = 2, .plane_bpp = { 16, 32 },
>>> +      .vsub = 2, .hsub = 2,
>>> +    },
>>> +    { .name = "P012", .depth = -1, .drm_id = DRM_FORMAT_P012,
>>> +      .cairo_id = CAIRO_FORMAT_RGB96F,
>>> +      .num_planes = 2, .plane_bpp = { 16, 32 },
>>> +      .vsub = 2, .hsub = 2,
>>> +    },
>>> +    { .name = "P016", .depth = -1, .drm_id = DRM_FORMAT_P016,
>>> +      .cairo_id = CAIRO_FORMAT_RGB96F,
>>> +      .num_planes = 2, .plane_bpp = { 16, 32 },
>>> +      .vsub = 2, .hsub = 2,
>>> +    },
>>> +    { .name = "IGT-FLOAT", .depth = -1, .drm_id = IGT_FORMAT_FLOAT,
>>> +      .cairo_id = CAIRO_FORMAT_INVALID,
>>> +      .num_planes = 1, .plane_bpp = { 128 },
>>> +    },
>>>    };
>>>    #define for_each_format(f)    \
>>>        for (f = format_desc; f - format_desc < ARRAY_SIZE(format_desc); f++)
>>> @@ -520,6 +553,14 @@ static void clear_yuv_buffer(struct igt_fb *fb)
>>>                full_range ? 0x00800080 : 0x10801080,
>>>                fb->strides[0] * fb->plane_height[0] / sizeof(wchar_t));
>>>            break;
>>> +    case DRM_FORMAT_P010:
>>> +    case DRM_FORMAT_P012:
>>> +    case DRM_FORMAT_P016:
>>> +        wmemset(ptr, full_range ? 0 : 0x10001000,
>>> +            fb->offsets[1] / sizeof(wchar_t));
>>> +        wmemset(ptr + fb->offsets[1], 0x80008000,
>>> +            fb->strides[1] * fb->plane_height[1] / sizeof(wchar_t));
>>> +        break;
>>>        }
>>>          igt_fb_unmap_buffer(fb, ptr);
>>> @@ -1496,6 +1537,7 @@ struct fb_convert_blit_upload {
>>>    };
>>>      static void *igt_fb_create_cairo_shadow_buffer(int fd,
>>> +                           unsigned drm_format,
>>>                               unsigned int width,
>>>                               unsigned int height,
>>>                               struct igt_fb *shadow)
>>> @@ -1505,7 +1547,7 @@ static void *igt_fb_create_cairo_shadow_buffer(int fd,
>>>        igt_assert(shadow);
>>>          fb_init(shadow, fd, width, height,
>>> -        DRM_FORMAT_XRGB8888, LOCAL_DRM_FORMAT_MOD_NONE,
>>> +        drm_format, LOCAL_DRM_FORMAT_MOD_NONE,
>>>            IGT_COLOR_YCBCR_BT709, IGT_COLOR_YCBCR_LIMITED_RANGE);
>>>          shadow->strides[0] = ALIGN(width * shadow->plane_bpp[0], 16);
>>> @@ -1599,6 +1641,9 @@ static void get_yuv_parameters(struct igt_fb *fb, struct yuv_parameters *params)
>>>          switch (fb->drm_format) {
>>>        case DRM_FORMAT_NV12:
>>> +    case DRM_FORMAT_P010:
>>> +    case DRM_FORMAT_P012:
>>> +    case DRM_FORMAT_P016:
>>>            params->y_inc = 1;
>>>            params->uv_inc = 2;
>>
>> Hi Maarten,
>>
>> I was wondering if above y_inc and uv_inc for Pxxx should be 2x of those for NV12?
>>
>> I noticed they're used below like "y_tmp += params.y_inc" where y_tmp is uint8_t*.
>>
>> Other than this all look good to me in this patch. I don't have HW to test with so I cannot go see how it work.
> 
> Well we increase uint16_t pointers, so it worked out. Tested it myself for P010, didn't test the Y21x Y41x formats but should be correct. :)

Ah, now I got it. I was looking just at first instance where the call 
was made to get those parameters.

This patch is
Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>

/Juha-Pekka


_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev