| Message ID | 20220215165226.2738568-1-geert@linux-m68k.org |
|---|---|
| Headers | show |
| Series | drm: Add support for low-color frame buffer formats | expand |
On Tue, 15 Feb 2022 17:52:19 +0100 Geert Uytterhoeven <geert@linux-m68k.org> wrote: > Introduce fourcc codes for color-indexed frame buffer formats with two, > four, and sixteen color, and provide a suitable mapping from bit per > pixel and depth to fourcc codes. > > As the number of bits per pixel is less than eight, these rely on proper > block handling for the calculation of bits per pixel and pitch. > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> > --- > Do we want to keep the rounding down if depth < bpp, or insist on depth > == bpp? I don't think the rounding down will still be needed after > "[PATCH 4/8] drm/client: Use actual bpp when allocating frame buffers". > --- > drivers/gpu/drm/drm_fourcc.c | 18 ++++++++++++++++++ > include/uapi/drm/drm_fourcc.h | 3 +++ > 2 files changed, 21 insertions(+) > > diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c > index 07741b678798b0f1..60ce63d728b8e308 100644 > --- a/drivers/gpu/drm/drm_fourcc.c > +++ b/drivers/gpu/drm/drm_fourcc.c > @@ -46,6 +46,18 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth) > case 8: > if (depth == 8) > fmt = DRM_FORMAT_C8; > + fallthrough; > + case 4: > + if (depth == 4) > + fmt = DRM_FORMAT_C4; > + fallthrough; > + case 2: > + if (depth == 2) > + fmt = DRM_FORMAT_C2; > + fallthrough; > + case 1: > + if (depth == 1) > + fmt = DRM_FORMAT_C1; > break; > > case 16: > @@ -132,6 +144,12 @@ EXPORT_SYMBOL(drm_driver_legacy_fb_format); > const struct drm_format_info *__drm_format_info(u32 format) > { > static const struct drm_format_info formats[] = { > + { .format = DRM_FORMAT_C1, .depth = 1, .num_planes = 1, > + .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 }, > + { .format = DRM_FORMAT_C2, .depth = 2, .num_planes = 1, > + .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 }, > + { .format = DRM_FORMAT_C4, .depth = 4, .num_planes = 1, > + .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 }, > { .format = DRM_FORMAT_C8, .depth = 8, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 }, > { .format = DRM_FORMAT_R8, .depth = 8, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 }, > { .format = DRM_FORMAT_R10, .depth = 10, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 }, > diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h > index fc0c1454d2757d5d..3f09174670b3cce6 100644 > --- a/include/uapi/drm/drm_fourcc.h > +++ b/include/uapi/drm/drm_fourcc.h > @@ -99,6 +99,9 @@ extern "C" { > #define DRM_FORMAT_INVALID 0 > > /* color index */ > +#define DRM_FORMAT_C1 fourcc_code('C', '1', ' ', ' ') /* [0] C */ > +#define DRM_FORMAT_C2 fourcc_code('C', '2', ' ', ' ') /* [1:0] C */ > +#define DRM_FORMAT_C4 fourcc_code('C', '4', ' ', ' ') /* [3:0] C */ Hi Geert, generally this looks fine to me though I'm not familiar with the code. The thing I'm missing here is a more precise description of the new pixel formats. > #define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */ This description of C8 is a little vague maybe, but presumably one pixel being one byte, the address of pixel x is just &bytes[x]. C4, C2 and C1 should also specify the pixel order within the byte. There is some precedent of that in with some YUV formats in this file. Maybe something like: C2 /* [7:0] c0:c1:c2:c3 2:2:2:2 four pixels per byte */ or the other way around, which ever your ordering is? Thanks, pq
Am 15.02.22 um 17:52 schrieb Geert Uytterhoeven: > When allocating a frame buffer, the number of bits per pixel needed is > derived from the deprecated drm_format_info.cpp[] field. While this > works for formats using less than 8 bits per pixel, it does lead to a > large overallocation. > > Reduce memory consumption by using the actual number of bits per pixel > instead. > > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org> Acked-by: Thomas Zimmermann <tzimmermann@suse.de> > --- > drivers/gpu/drm/drm_client.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/drm_client.c b/drivers/gpu/drm/drm_client.c > index ce45e380f4a2028f..c6a279e3de95591a 100644 > --- a/drivers/gpu/drm/drm_client.c > +++ b/drivers/gpu/drm/drm_client.c > @@ -264,7 +264,7 @@ drm_client_buffer_create(struct drm_client_dev *client, u32 width, u32 height, u > > dumb_args.width = width; > dumb_args.height = height; > - dumb_args.bpp = info->cpp[0] * 8; > + dumb_args.bpp = drm_format_info_bpp(info, 0); > ret = drm_mode_create_dumb(dev, &dumb_args, client->file); > if (ret) > goto err_delete; > @@ -372,7 +372,7 @@ static int drm_client_buffer_addfb(struct drm_client_buffer *buffer, > int ret; > > info = drm_format_info(format); > - fb_req.bpp = info->cpp[0] * 8; > + fb_req.bpp = drm_format_info_bpp(info, 0); > fb_req.depth = info->depth; > fb_req.width = width; > fb_req.height = height;
On Thursday, February 17th, 2022 at 17:12, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > What is C0? > > A non-existing color-indexed mode with zero colors ;-) > Introduced purely to make a check like in the comment below work. > What we really want to check here is if the mode is color-indexed > or not... Maybe it would be worth introducing a drm_format_info_is_color_indexed function? Would be self-describing when used, and would avoid to miss some places to update when adding new color-indexed formats.
Hi Simon, On Thu, Feb 17, 2022 at 5:18 PM Simon Ser <contact@emersion.fr> wrote: > On Thursday, February 17th, 2022 at 17:12, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > > > What is C0? > > > > A non-existing color-indexed mode with zero colors ;-) > > Introduced purely to make a check like in the comment below work. > > What we really want to check here is if the mode is color-indexed > > or not... > > Maybe it would be worth introducing a drm_format_info_is_color_indexed > function? Would be self-describing when used, and would avoid to miss > some places to update when adding new color-indexed formats. Yep, and a .is_color_indexed flag, cfr. the existing .is_yuv flag. 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
Hi Geert, > > > + switch (fb->format->depth) { > > > > The depth field is deprecated. It's probably better to use > > fb->format->format and test against 4CC codes. > > The reason I checked for depth instead of a 4CC code is that the only > thing that matters here is the number of bits per pixel. Hence this > function won't need any changes to support R1, R2, R4, and D1 later. > When we get here, we already know that we are using a format that > is supported by the fbdev helper code, and thus passed the 4CC > checks elsewhere. > > Alternatively, we could introduce drm_format_info_bpp() earlier in > the series, and use that? The drm_format_info_bpp() is very descriptive, so yes it would be good to use it here also. Sam
Hi Geert, On Tue, Feb 15, 2022 at 05:52:18PM +0100, Geert Uytterhoeven wrote: > Hi all, > > A long outstanding issue with the DRM subsystem has been the lack of > support for low-color displays, as used typically on older desktop > systems and small embedded displays. This is one of the pieces missing for a long time - great to see something done here. Thanks Geert! Sam
Am 17.02.22 um 21:37 schrieb Sam Ravnborg: > Hi Geert, > > On Tue, Feb 15, 2022 at 05:52:18PM +0100, Geert Uytterhoeven wrote: >> Hi all, >> >> A long outstanding issue with the DRM subsystem has been the lack of >> support for low-color displays, as used typically on older desktop >> systems and small embedded displays. > > This is one of the pieces missing for a long time - great to see > something done here. Thanks Geert! Absolutely! I'm looking forward to see these patches being merged. Best regards Thomas > > Sam
Hi all, A long outstanding issue with the DRM subsystem has been the lack of support for low-color displays, as used typically on older desktop systems and small embedded displays. This patch series adds support for color-indexed frame buffer formats with 2, 4, and 16 colors. It has been tested on ARAnyM using a work-in-progress Atari DRM driver, with text console operation and fbtest. Overview: - Patches 1 and 2 give a working system, albeit with a too large pitch (line length), - Patches 3 and 4 reduce memory consumption by correcting the pitch in case bpp < 8, - Patches 5 and 6 are untested, but may become useful with DRM userspace, - Patches 7 and 8 add more fourcc codes for grayscale and monochrome frame buffer formats, which may be useful for e.g. the ssd130x and repaper drivers. Notes: - I haven't looked yet into making modetest draw a correct image. - As this was used on emulated hardware only, and I do not have Atari hardware, I do not have performance figures to compare with fbdev. I hope to do proper measuring with an Amiga DRM driver, eventually. Thanks for your comments! Geert Uytterhoeven (8): drm/fourcc: Add DRM_FORMAT_C[124] drm/fb-helper: Add support for DRM_FORMAT_C[124] drm/fourcc: Add drm_format_info_bpp() helper drm/client: Use actual bpp when allocating frame buffers drm/framebuffer: Use actual bpp for DRM_IOCTL_MODE_GETFB drm/gem-fb-helper: Use actual bpp for size calculations drm/fourcc: Add DRM_FORMAT_R[124] drm/fourcc: Add DRM_FORMAT_D1 drivers/gpu/drm/drm_client.c | 4 +- drivers/gpu/drm/drm_fb_helper.c | 120 ++++++++++++++----- drivers/gpu/drm/drm_fourcc.c | 45 +++++++ drivers/gpu/drm/drm_framebuffer.c | 2 +- drivers/gpu/drm/drm_gem_framebuffer_helper.c | 12 +- include/drm/drm_fourcc.h | 1 + include/uapi/drm/drm_fourcc.h | 15 +++ 7 files changed, 160 insertions(+), 39 deletions(-)