Re: [igt-dev] [PATCH i-g-t 2/2] lib/igt_fb: Add support for testing of 16 bpc fixed point formats.

[Mario Kleiner <mario.kleiner.de@gmail.com>]

On Mon, May 3, 2021 at 8:48 PM Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
>
> On Mon, May 03, 2021 at 08:25:55PM +0200, Mario Kleiner wrote:
> > This is used to support testing the 16 bpc formats, e.g., via:
> >
> > kms_plane --run-subtest pixel-format-pipe-A-planes
> >
> > So far this was successfully tested on AMD RavenRidge with DCN-1
> > display hw.
> >
> > The new conversion routines are slightly adapted copies of the
> > convert_float_to_fp16() and convert_fp16_to_float() functions,
> > with the conversion math modified for float <-> uint16 instead.
> >
> > Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Cc: Alex Deucher <alexander.deucher@amd.com>
> > ---
> >  lib/igt_fb.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 123 insertions(+)
> >
> > diff --git a/lib/igt_fb.c b/lib/igt_fb.c
> > index 954e1181..148e6c0f 100644
> > --- a/lib/igt_fb.c
> > +++ b/lib/igt_fb.c
> > @@ -220,6 +220,22 @@ static const struct format_desc_struct {
> >         .cairo_id = CAIRO_FORMAT_RGBA128F, .convert = true,
> >         .num_planes = 1, .plane_bpp = { 64, },
> >       },
> > +     { .name = "XRGB16161616", .depth = -1, .drm_id = DRM_FORMAT_XRGB16161616,
> > +       .cairo_id = CAIRO_FORMAT_RGBA128F, .convert = true,
> > +       .num_planes = 1, .plane_bpp = { 64, },
> > +     },
> > +     { .name = "ARGB16161616", .depth = -1, .drm_id = DRM_FORMAT_ARGB16161616,
> > +       .cairo_id = CAIRO_FORMAT_RGBA128F, .convert = true,
> > +       .num_planes = 1, .plane_bpp = { 64, },
> > +     },
> > +     { .name = "XBGR16161616", .depth = -1, .drm_id = DRM_FORMAT_XBGR16161616,
> > +       .cairo_id = CAIRO_FORMAT_RGBA128F, .convert = true,
> > +       .num_planes = 1, .plane_bpp = { 64, },
> > +     },
> > +     { .name = "ABGR16161616", .depth = -1, .drm_id = DRM_FORMAT_ABGR16161616,
> > +       .cairo_id = CAIRO_FORMAT_RGBA128F, .convert = true,
> > +       .num_planes = 1, .plane_bpp = { 64, },
> > +     },
> >       { .name = "NV12", .depth = -1, .drm_id = DRM_FORMAT_NV12,
> >         .cairo_id = CAIRO_FORMAT_RGB24, .convert = true,
> >         .num_planes = 2, .plane_bpp = { 8, 16, },
> > @@ -3365,9 +3381,13 @@ static const unsigned char *rgbx_swizzle(uint32_t format)
> >       default:
> >       case DRM_FORMAT_XRGB16161616F:
> >       case DRM_FORMAT_ARGB16161616F:
> > +     case DRM_FORMAT_XRGB16161616:
> > +     case DRM_FORMAT_ARGB16161616:
> >               return swizzle_bgrx;
> >       case DRM_FORMAT_XBGR16161616F:
> >       case DRM_FORMAT_ABGR16161616F:
> > +     case DRM_FORMAT_XBGR16161616:
> > +     case DRM_FORMAT_ABGR16161616:
> >               return swizzle_rgbx;
> >       }
> >  }
> > @@ -3451,6 +3471,97 @@ static void convert_float_to_fp16(struct fb_convert *cvt)
> >       }
> >  }
> >
> > +static void float_to_uint16(const float *f, uint16_t *h, unsigned int num)
> > +{
> > +     for (int i = 0; i < num; i++)
> > +             h[i] = f[i] * 65535.0f + 0.5f;
> > +}
> > +
> > +static void uint16_to_float(const uint16_t *h, float *f, unsigned int num)
> > +{
> > +     for (int i = 0; i < num; i++)
> > +             f[i] = ((float) h[i]) / 65535.0f;
>
> nit: the cast shouldn't be necessary.
>
> Looks good otherwise.
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
Thanks for the quick review. Compiler and test results agree with you :).

However, all other uintsomething_to_float() conversion routines, e.g.,
for the yuv formats and for fp16_to_float() all use that apparently
redundant cast, which is why I left it for consistency with the rest
of the code. And in case those wiser people who wrote those bits may
know something that i don't see?

Should I change the patch to drop the cast, and resend?
-mario

> > +}
> > +
> > +static void convert_uint16_to_float(struct fb_convert *cvt)
> > +{
> > +     int i, j;
> > +     uint16_t *up16;
> > +     float *ptr = cvt->dst.ptr;
> > +     unsigned int float_stride = cvt->dst.fb->strides[0] / sizeof(*ptr);
> > +     unsigned int up16_stride = cvt->src.fb->strides[0] / sizeof(*up16);
> > +     const unsigned char *swz = rgbx_swizzle(cvt->src.fb->drm_format);
> > +     bool needs_reswizzle = swz != swizzle_rgbx;
> > +
> > +     uint16_t *buf = convert_src_get(cvt);
> > +     up16 = buf + cvt->src.fb->offsets[0] / sizeof(*buf);
> > +
> > +     for (i = 0; i < cvt->dst.fb->height; i++) {
> > +             if (needs_reswizzle) {
> > +                     const uint16_t *u16_tmp = up16;
> > +                     float *rgb_tmp = ptr;
> > +
> > +                     for (j = 0; j < cvt->dst.fb->width; j++) {
> > +                             struct igt_vec4 rgb;
> > +
> > +                             uint16_to_float(u16_tmp, rgb.d, 4);
> > +
> > +                             rgb_tmp[0] = rgb.d[swz[0]];
> > +                             rgb_tmp[1] = rgb.d[swz[1]];
> > +                             rgb_tmp[2] = rgb.d[swz[2]];
> > +                             rgb_tmp[3] = rgb.d[swz[3]];
> > +
> > +                             rgb_tmp += 4;
> > +                             u16_tmp += 4;
> > +                     }
> > +             } else {
> > +                     uint16_to_float(up16, ptr, cvt->dst.fb->width * 4);
> > +             }
> > +
> > +             ptr += float_stride;
> > +             up16 += up16_stride;
> > +     }
> > +
> > +     convert_src_put(cvt, buf);
> > +}
> > +
> > +static void convert_float_to_uint16(struct fb_convert *cvt)
> > +{
> > +     int i, j;
> > +     uint16_t *up16 = cvt->dst.ptr + cvt->dst.fb->offsets[0];
> > +     const float *ptr = cvt->src.ptr;
> > +     unsigned float_stride = cvt->src.fb->strides[0] / sizeof(*ptr);
> > +     unsigned up16_stride = cvt->dst.fb->strides[0] / sizeof(*up16);
> > +     const unsigned char *swz = rgbx_swizzle(cvt->dst.fb->drm_format);
> > +     bool needs_reswizzle = swz != swizzle_rgbx;
> > +
> > +     for (i = 0; i < cvt->dst.fb->height; i++) {
> > +             if (needs_reswizzle) {
> > +                     const float *rgb_tmp = ptr;
> > +                     uint16_t *u16_tmp = up16;
> > +
> > +                     for (j = 0; j < cvt->dst.fb->width; j++) {
> > +                             struct igt_vec4 rgb;
> > +
> > +                             rgb.d[0] = rgb_tmp[swz[0]];
> > +                             rgb.d[1] = rgb_tmp[swz[1]];
> > +                             rgb.d[2] = rgb_tmp[swz[2]];
> > +                             rgb.d[3] = rgb_tmp[swz[3]];
> > +
> > +                             float_to_uint16(rgb.d, u16_tmp, 4);
> > +
> > +                             rgb_tmp += 4;
> > +                             u16_tmp += 4;
> > +                     }
> > +             } else {
> > +                     float_to_uint16(ptr, up16, cvt->dst.fb->width * 4);
> > +             }
> > +
> > +             ptr += float_stride;
> > +             up16 += up16_stride;
> > +     }
> > +}
> > +
> >  static void convert_pixman(struct fb_convert *cvt)
> >  {
> >       pixman_format_code_t src_pixman = drm_format_to_pixman(cvt->src.fb->drm_format);
> > @@ -3560,6 +3671,12 @@ static void fb_convert(struct fb_convert *cvt)
> >               case DRM_FORMAT_ABGR16161616F:
> >                       convert_fp16_to_float(cvt);
> >                       return;
> > +             case DRM_FORMAT_XRGB16161616:
> > +             case DRM_FORMAT_XBGR16161616:
> > +             case DRM_FORMAT_ARGB16161616:
> > +             case DRM_FORMAT_ABGR16161616:
> > +                     convert_uint16_to_float(cvt);
> > +                     return;
> >               }
> >       } else if (cvt->src.fb->drm_format == IGT_FORMAT_FLOAT) {
> >               switch (cvt->dst.fb->drm_format) {
> > @@ -3589,6 +3706,12 @@ static void fb_convert(struct fb_convert *cvt)
> >               case DRM_FORMAT_ABGR16161616F:
> >                       convert_float_to_fp16(cvt);
> >                       return;
> > +             case DRM_FORMAT_XRGB16161616:
> > +             case DRM_FORMAT_XBGR16161616:
> > +             case DRM_FORMAT_ARGB16161616:
> > +             case DRM_FORMAT_ABGR16161616:
> > +                     convert_float_to_uint16(cvt);
> > +                     return;
> >               }
> >       }
> >
> > --
> > 2.25.1
>
> --
> Ville Syrjälä
> Intel
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev