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

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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@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?

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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev