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

[Mario Kleiner]

On Mon, May 3, 2021 at 9:43 PM Ville Syrjälä
<ville.syrjala at linux.intel.com> wrote:
>
> On Mon, May 03, 2021 at 09:32:19PM +0200, Mario Kleiner wrote:
> > On Mon, May 3, 2021 at 8:48 PM Ville Syrjälä
> > <ville.syrjala at 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 at gmail.com>
> > > > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > Cc: Alex Deucher <alexander.deucher at 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 at 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?
>
> Maybe not be worth the hassl esp. if there are redundant casts all over
> anyway. We could do a global pass later to clear them all up, or just
> squint and pretend they're not there :P
>
> --
> Ville Syrjälä
> Intel

Ok, then i'll leave it as is.
-mario