| Message ID | a89f148bf61bc20a7bb9c0e8ba030b0b770f9fe2.1657302103.git.geert@linux-m68k.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Big-endian fixes | expand |
On Fri, Jul 08, 2022 at 08:21:43PM +0200, Geert Uytterhoeven wrote: > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > Any better suggestion than appending "be"? > > v2: > - New. > --- > tests/util/format.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/tests/util/format.c b/tests/util/format.c > index a5464de6fc1ac70f..42a652c9a402a654 100644 > --- a/tests/util/format.c > +++ b/tests/util/format.c > @@ -76,6 +76,9 @@ static const struct util_format_info format_info[] = { > { DRM_FORMAT_BGRX5551, "BX15", MAKE_RGB_INFO(5, 1, 5, 6, 5, 11, 0, 0) }, > { DRM_FORMAT_RGB565, "RG16", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, > { DRM_FORMAT_BGR565, "BG16", MAKE_RGB_INFO(5, 0, 6, 5, 5, 11, 0, 0) }, > + /* Big-endian RGB16 */ > + { DRM_FORMAT_XRGB1555 | DRM_FORMAT_BIG_ENDIAN, "XR15be", MAKE_RGB_INFO(5, 10, 5, 5, 5, 0, 0, 0) }, > + { DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN, "RG16be", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, How about just stripping the BE bit in util_format_info_find() so we don't have to duplicate the entries in the table? I guess util_format_fourcc() would end up being more a bit complicated since you'd have to massage the string. But I'm not sure why we even store the fourcc as a string in the table anyway. Could just add some kind of string_to_fourcc() thingy instead AFAICS. > /* RGB24 */ > { DRM_FORMAT_BGR888, "BG24", MAKE_RGB_INFO(8, 0, 8, 8, 8, 16, 0, 0) }, > { DRM_FORMAT_RGB888, "RG24", MAKE_RGB_INFO(8, 16, 8, 8, 8, 0, 0, 0) }, > -- > 2.25.1
Hi Ville, On Mon, Jul 11, 2022 at 2:17 PM Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Fri, Jul 08, 2022 at 08:21:43PM +0200, Geert Uytterhoeven wrote: > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > --- > > Any better suggestion than appending "be"? > > > > v2: > > - New. > > --- a/tests/util/format.c > > +++ b/tests/util/format.c > > @@ -76,6 +76,9 @@ static const struct util_format_info format_info[] = { > > { DRM_FORMAT_BGRX5551, "BX15", MAKE_RGB_INFO(5, 1, 5, 6, 5, 11, 0, 0) }, > > { DRM_FORMAT_RGB565, "RG16", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, > > { DRM_FORMAT_BGR565, "BG16", MAKE_RGB_INFO(5, 0, 6, 5, 5, 11, 0, 0) }, > > + /* Big-endian RGB16 */ > > + { DRM_FORMAT_XRGB1555 | DRM_FORMAT_BIG_ENDIAN, "XR15be", MAKE_RGB_INFO(5, 10, 5, 5, 5, 0, 0, 0) }, > > + { DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN, "RG16be", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, > > How about just stripping the BE bit in util_format_info_find() > so we don't have to duplicate the entries in the table? There is no need to support big-endian variants of all formats. E.g. big-endian [AX]RGB8888 just map to little-endian BGR[AX]8888. XRGB1555 and RGB565 are probably the only RGB formats we care about. Or perhaps some of the *30 formats? > I guess util_format_fourcc() would end up being more a bit > complicated since you'd have to massage the string. True. > But I'm not sure why we even store the fourcc as a string in > the table anyway. Could just add some kind of string_to_fourcc() > thingy instead AFAICS. I guess that can be done. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On 2022-07-11 14:34, Geert Uytterhoeven wrote: > Hi Ville, > > On Mon, Jul 11, 2022 at 2:17 PM Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: >> On Fri, Jul 08, 2022 at 08:21:43PM +0200, Geert Uytterhoeven wrote: >>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> >>> --- >>> Any better suggestion than appending "be"? >>> >>> v2: >>> - New. > >>> --- a/tests/util/format.c >>> +++ b/tests/util/format.c >>> @@ -76,6 +76,9 @@ static const struct util_format_info format_info[] = { >>> { DRM_FORMAT_BGRX5551, "BX15", MAKE_RGB_INFO(5, 1, 5, 6, 5, 11, 0, 0) }, >>> { DRM_FORMAT_RGB565, "RG16", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, >>> { DRM_FORMAT_BGR565, "BG16", MAKE_RGB_INFO(5, 0, 6, 5, 5, 11, 0, 0) }, >>> + /* Big-endian RGB16 */ >>> + { DRM_FORMAT_XRGB1555 | DRM_FORMAT_BIG_ENDIAN, "XR15be", MAKE_RGB_INFO(5, 10, 5, 5, 5, 0, 0, 0) }, >>> + { DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN, "RG16be", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, >> >> How about just stripping the BE bit in util_format_info_find() >> so we don't have to duplicate the entries in the table? > > There is no need to support big-endian variants of all formats. > E.g. big-endian [AX]RGB8888 just map to little-endian BGR[AX]8888. > > XRGB1555 and RGB565 are probably the only RGB formats we care about. > Or perhaps some of the *30 formats? I'd say all of those, and any other formats with components straddling byte boundaries (including formats with multi-byte components). P.S. libdrm changes are now reviewed and merged as GitLab merge requests: https://gitlab.freedesktop.org/mesa/drm/-/merge_requests I guess CONTRIBUTING.rst could make this clearer.
Hi Ville, On Mon, Jul 11, 2022 at 2:34 PM Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Mon, Jul 11, 2022 at 2:17 PM Ville Syrjälä > <ville.syrjala@linux.intel.com> wrote: > > On Fri, Jul 08, 2022 at 08:21:43PM +0200, Geert Uytterhoeven wrote: > > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > > > --- > > > Any better suggestion than appending "be"? > > > > > > v2: > > > - New. > > > > --- a/tests/util/format.c > > > +++ b/tests/util/format.c > > > @@ -76,6 +76,9 @@ static const struct util_format_info format_info[] = { > > > { DRM_FORMAT_BGRX5551, "BX15", MAKE_RGB_INFO(5, 1, 5, 6, 5, 11, 0, 0) }, > > > { DRM_FORMAT_RGB565, "RG16", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, > > > { DRM_FORMAT_BGR565, "BG16", MAKE_RGB_INFO(5, 0, 6, 5, 5, 11, 0, 0) }, > > > + /* Big-endian RGB16 */ > > > + { DRM_FORMAT_XRGB1555 | DRM_FORMAT_BIG_ENDIAN, "XR15be", MAKE_RGB_INFO(5, 10, 5, 5, 5, 0, 0, 0) }, > > > + { DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN, "RG16be", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, > > > > But I'm not sure why we even store the fourcc as a string in > > the table anyway. Could just add some kind of string_to_fourcc() > > thingy instead AFAICS. > > I guess that can be done. Nowadays we have drmGetFormatName(), which returns an allocated string with the name for a format code. Using that helper in string_to_fourcc() would mean looping over the table, and for each entry, calling drmGetFormatName(), comparing the name, and freeing the name again. Would that be acceptable? Thanks! Gr{oetje,eeting}s, Geert
diff --git a/tests/util/format.c b/tests/util/format.c index a5464de6fc1ac70f..42a652c9a402a654 100644 --- a/tests/util/format.c +++ b/tests/util/format.c @@ -76,6 +76,9 @@ static const struct util_format_info format_info[] = { { DRM_FORMAT_BGRX5551, "BX15", MAKE_RGB_INFO(5, 1, 5, 6, 5, 11, 0, 0) }, { DRM_FORMAT_RGB565, "RG16", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, { DRM_FORMAT_BGR565, "BG16", MAKE_RGB_INFO(5, 0, 6, 5, 5, 11, 0, 0) }, + /* Big-endian RGB16 */ + { DRM_FORMAT_XRGB1555 | DRM_FORMAT_BIG_ENDIAN, "XR15be", MAKE_RGB_INFO(5, 10, 5, 5, 5, 0, 0, 0) }, + { DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN, "RG16be", MAKE_RGB_INFO(5, 11, 6, 5, 5, 0, 0, 0) }, /* RGB24 */ { DRM_FORMAT_BGR888, "BG24", MAKE_RGB_INFO(8, 0, 8, 8, 8, 16, 0, 0) }, { DRM_FORMAT_RGB888, "RG24", MAKE_RGB_INFO(8, 16, 8, 8, 8, 0, 0, 0) },
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> --- Any better suggestion than appending "be"? v2: - New. --- tests/util/format.c | 3 +++ 1 file changed, 3 insertions(+)