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 on 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 supporting 2, 4, 16, 256, and 65536
colors, with text console operation, fbtest, and modetest.
Overview:
- Patch 1 introduces a helper, to be used by later patches in the
series,
- Patch 2 introduces a flag to indicate color-indexed formats,
- Patches 3 and 4 correct calculations of bits per pixel for sub-byte
pixel formats,
- Patches 5 and 6 introduce the new C[124] formats,
- Patch 7 fixes an untested code path,
- Patch 8 documents the use of "red" for light-on-dark displays,
- Patches 9 and 10 add more fourcc codes for light-on-dark and
dark-on-light frame buffer formats, which may be useful for e.g. the
ssd130x and repaper drivers.
Changes compared to v2[1]:
- Add Reviewed-by,
- Document fill order,
- Fix FB_VISUAL_TRUECOLOR,
- Replace light-on-dark/dark-on-light by direct/inverse relationship
between channel value and brightness.
Changes compared to v1[2]:
- Reshuffle patches,
- New patch "[PATCH v2 02/10] drm/fourcc: Add
drm_format_info.is_color_indexed flag",
- Improve pixel descriptions,
- Require depth to match bpp in drm_mode_legacy_fb_format(),
- Set .is_color_indexed flag.
- Use drm_format_info_bpp() helper instead of deprecated .depth field
or format-dependent calculations,
- Use new .is_color_indexed field instead of checking against a list
of formats,
- Add Acked-by,
- Replace FIXME by TODO comment,
- New patch "[PATCH v2 08/10] [RFC] drm/fourcc: Document that
single-channel "red" can be any color",
- Add rationale for adding new formats,
- Add D[248] for completeness.
Notes:
- This is the first patch series in a series of 3:
- To make high-color modes work on big-endian, you also need [3],
- To make mode selection on the command line work for Atari video
modes, you need [4].
- There is also a related series of 3 patch series for modetest:
- Using modetest with low-color formats (C[124]) requires [5],
- Using modetest with high-color formats (RG16, XR24) requires
[6],
- Using modetest with video mode names containing dashes requires
[7].
- 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.
- While the Atari DRM driver is not fit for submission yet, you can
find it at [8], if you are adventurous.
Thanks for your comments!
[1] "[PATCH v2 00/10] drm: Add support for low-color frame buffer
formats"
https://lore.kernel.org/r/[email protected]/
[2] "[PATCH 0/8] drm: Add support for low-color frame buffer formats"
https://lore.kernel.org/r/[email protected]/
[3] "[PATCH 0/3] drm: Endianness fixes"
https://lore.kernel.org/r/[email protected]
[4] "[PATCH 0/5] drm/modes: Command line mode selection fixes and
improvements"
https://lore.kernel.org/r/[email protected]
[5] "[PATCH libdrm v2 00/10] Add support for low-color frame buffer
formats"
https://lore.kernel.org/r/[email protected]
[6] "[PATCH libdrm v2 00/10] Big-endian fixes"
https://lore.kernel.org/r/[email protected]
[7] "[PATCH libdrm] modetest: Add support for named modes containing
dashes"
[8] https://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k.git/log/?h=atari-drm-wip
Geert Uytterhoeven (10):
drm/fourcc: Add drm_format_info_bpp() helper
drm/fourcc: Add drm_format_info.is_color_indexed flag
drm/client: Use actual bpp when allocating frame buffers
drm/framebuffer: Use actual bpp for DRM_IOCTL_MODE_GETFB
drm/fourcc: Add DRM_FORMAT_C[124]
drm/fb-helper: Add support for DRM_FORMAT_C[124]
drm/gem-fb-helper: Use actual bpp for size calculations
drm/fourcc: Clarify the meaning of single-channel "red"
[RFC] drm/fourcc: Add DRM_FORMAT_R[124]
[RFC] drm/fourcc: Add DRM_FORMAT_D[1248]
drivers/gpu/drm/drm_client.c | 4 +-
drivers/gpu/drm/drm_fb_helper.c | 101 ++++++++++++++-----
drivers/gpu/drm/drm_fourcc.c | 55 +++++++++-
drivers/gpu/drm/drm_framebuffer.c | 2 +-
drivers/gpu/drm/drm_gem_framebuffer_helper.c | 12 +--
include/drm/drm_fourcc.h | 4 +
include/uapi/drm/drm_fourcc.h | 32 +++++-
7 files changed, 167 insertions(+), 43 deletions(-)
--
2.25.1
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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
Add a flag to struct drm_format_info to indicate if a format is
color-indexed, similar to the existing .is_yuv flag.
This way generic code and drivers can just check this flag, instead of
checking against a list of fourcc formats.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
v3:
- Add Reviewed-by,
v2:
- New.
---
drivers/gpu/drm/drm_fourcc.c | 2 +-
include/drm/drm_fourcc.h | 3 +++
2 files changed, 4 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index cf48ea0b2cb70ba8..6c76bd821d17e7c7 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -132,7 +132,7 @@ 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_C8, .depth = 8, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
+ { .format = DRM_FORMAT_C8, .depth = 8, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
{ .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 },
{ .format = DRM_FORMAT_R12, .depth = 12, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 3800a7ad7f0cda7a..532ae78ca747e6c4 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -138,6 +138,9 @@ struct drm_format_info {
/** @is_yuv: Is it a YUV format? */
bool is_yuv;
+
+ /** @is_color_indexed: Is it a color-indexed format? */
+ bool is_color_indexed;
};
/**
--
2.25.1
The AFBC helpers derive the number of bits per pixel from the deprecated
drm_format_info.cpp[] field, which does not take into account block
sizes.
Fix this by using the actual number of bits per pixel instead.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
Compile-tested only.
v3:
- Add Reviewed-by,
v2:
- Replace FIXME by TODO comment.
---
drivers/gpu/drm/drm_gem_framebuffer_helper.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_framebuffer_helper.c b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
index f4619803acd01e96..6b680ca8c0cfd8d7 100644
--- a/drivers/gpu/drm/drm_gem_framebuffer_helper.c
+++ b/drivers/gpu/drm/drm_gem_framebuffer_helper.c
@@ -492,6 +492,8 @@ void drm_gem_fb_end_cpu_access(struct drm_framebuffer *fb, enum dma_data_directi
}
EXPORT_SYMBOL(drm_gem_fb_end_cpu_access);
+// TODO Drop this function and replace by drm_format_info_bpp() once all
+// DRM_FORMAT_* provide proper block info in drivers/gpu/drm/drm_fourcc.c
static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
const struct drm_mode_fb_cmd2 *mode_cmd)
{
@@ -499,11 +501,6 @@ static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
info = drm_get_format_info(dev, mode_cmd);
- /* use whatever a driver has set */
- if (info->cpp[0])
- return info->cpp[0] * 8;
-
- /* guess otherwise */
switch (info->format) {
case DRM_FORMAT_YUV420_8BIT:
return 12;
@@ -512,11 +509,8 @@ static __u32 drm_gem_afbc_get_bpp(struct drm_device *dev,
case DRM_FORMAT_VUY101010:
return 30;
default:
- break;
+ return drm_format_info_bpp(info, 0);
}
-
- /* all attempts failed */
- return 0;
}
static int drm_gem_afbc_min_size(struct drm_device *dev,
--
2.25.1
Introduce fourcc codes for color-indexed frame buffer formats with two,
four, and sixteen colors, and provide a mapping from bits 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.
The fill order (the order in which multiple pixels are packed in a byte)
is the same order as used for indexed-color (2, 4, and 16 colors) images
in the PNG specification, Version 1.2.
This order is also the recommended and default order (FillOrder = 1) for
palette-color (16 colors) images in the TIFF 6.0 Specification, and is
also used for 16-color Linux frame buffer logos.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Pekka Paalanen <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
v3:
- Add Reviewed-by,
- Document fill order,
v2:
- Improve pixel description,
- Require depth to match bpp in drm_mode_legacy_fb_format(),
- Set .is_color_indexed flag.
---
drivers/gpu/drm/drm_fourcc.c | 21 +++++++++++++++++++++
include/uapi/drm/drm_fourcc.h | 3 +++
2 files changed, 24 insertions(+)
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 6c76bd821d17e7c7..29f4fe199c4ddcf0 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -43,6 +43,21 @@ uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
uint32_t fmt = DRM_FORMAT_INVALID;
switch (bpp) {
+ case 1:
+ if (depth == 1)
+ fmt = DRM_FORMAT_C1;
+ break;
+
+ case 2:
+ if (depth == 2)
+ fmt = DRM_FORMAT_C2;
+ break;
+
+ case 4:
+ if (depth == 4)
+ fmt = DRM_FORMAT_C4;
+ break;
+
case 8:
if (depth == 8)
fmt = DRM_FORMAT_C8;
@@ -132,6 +147,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, .is_color_indexed = true },
+ { .format = DRM_FORMAT_C2, .depth = 2, .num_planes = 1,
+ .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
+ { .format = DRM_FORMAT_C4, .depth = 4, .num_planes = 1,
+ .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
{ .format = DRM_FORMAT_C8, .depth = 8, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
{ .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 0980678d502dc784..e18de6f258302673 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', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */
+#define DRM_FORMAT_C2 fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */
+#define DRM_FORMAT_C4 fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
#define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
/* 8 bpp Red */
--
2.25.1
When allocating a frame buffer, the number of bits per pixel needed is
derived from the deprecated drm_format_info.cpp[] field. While this may
work 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 <[email protected]>
Acked-by: Thomas Zimmermann <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
v3:
- Add Reviewed-by,
v2:
- Add Acked-by.
---
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 af3b7395bf6932f7..2b230b4d69423752 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;
@@ -373,7 +373,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;
--
2.25.1
Traditionally, the first channel has been called the "red" channel, but
the fourcc values for single-channel "red" formats can also be used for
other single-channel formats with a direct relationship between channel
value and brightness, like grayscale.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
v3:
- Add Reviewed-by,
- Replace light-on-dark by direct relationship between channel value
and brightness,
v2:
- New.
---
include/uapi/drm/drm_fourcc.h | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index e18de6f258302673..dff5072fccaaf65c 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -104,16 +104,16 @@ extern "C" {
#define DRM_FORMAT_C4 fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
#define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
-/* 8 bpp Red */
+/* 8 bpp Red (direct relationship between channel value and brightness) */
#define DRM_FORMAT_R8 fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
-/* 10 bpp Red */
+/* 10 bpp Red (direct relationship between channel value and brightness) */
#define DRM_FORMAT_R10 fourcc_code('R', '1', '0', ' ') /* [15:0] x:R 6:10 little endian */
-/* 12 bpp Red */
+/* 12 bpp Red (direct relationship between channel value and brightness) */
#define DRM_FORMAT_R12 fourcc_code('R', '1', '2', ' ') /* [15:0] x:R 4:12 little endian */
-/* 16 bpp Red */
+/* 16 bpp Red (direct relationship between channel value and brightness) */
#define DRM_FORMAT_R16 fourcc_code('R', '1', '6', ' ') /* [15:0] R little endian */
/* 16 bpp RG */
--
2.25.1
Add a helper to retrieve the actual number of bits per pixel for a
plane, taking into account the number of characters and pixels per
block for tiled formats.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
v3:
- Add Reviewed-by,
v2:
- Move up.
---
drivers/gpu/drm/drm_fourcc.c | 19 +++++++++++++++++++
include/drm/drm_fourcc.h | 1 +
2 files changed, 20 insertions(+)
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 07741b678798b0f1..cf48ea0b2cb70ba8 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info,
}
EXPORT_SYMBOL(drm_format_info_block_height);
+/**
+ * drm_format_info_bpp - number of bits per pixel
+ * @info: pixel format info
+ * @plane: plane index
+ *
+ * Returns:
+ * The actual number of bits per pixel, depending on the plane index.
+ */
+unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane)
+{
+ if (!info || plane < 0 || plane >= info->num_planes)
+ return 0;
+
+ return info->char_per_block[plane] * 8 /
+ (drm_format_info_block_width(info, plane) *
+ drm_format_info_block_height(info, plane));
+}
+EXPORT_SYMBOL(drm_format_info_bpp);
+
/**
* drm_format_info_min_pitch - computes the minimum required pitch in bytes
* @info: pixel format info
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
index 22aa64d07c7905e2..3800a7ad7f0cda7a 100644
--- a/include/drm/drm_fourcc.h
+++ b/include/drm/drm_fourcc.h
@@ -313,6 +313,7 @@ unsigned int drm_format_info_block_width(const struct drm_format_info *info,
int plane);
unsigned int drm_format_info_block_height(const struct drm_format_info *info,
int plane);
+unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane);
uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
int plane, unsigned int buffer_width);
--
2.25.1
Add support for color-indexed frame buffer formats with two, four, and
sixteen colors to the DRM framebuffer helper functions:
1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
2. For color-indexed modes, the length of the color bitfields must be
set to the color depth, else the logo code may pick a logo with too
many colors. Drop the incorrect DAC width comment, which
originates from the i915 driver.
3. Accept C[124] modes when validating or filling in struct
fb_var_screeninfo, and use the correct number of bits per pixel.
4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
modes.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
If "[PATCH] drm/fb-helper: Remove helpers to change frame buffer
config"[1] is accepted, the changes to drm_fb_helper_check_var() can
just be removed.
v3:
- Fix FB_VISUAL_TRUECOLOR,
- Add Reviewed-by,
v2:
- Use drm_format_info_bpp() helper instead of deprecated .depth field
or format-dependent calculations,
- Use new .is_color_indexed field instead of checking against a list
of formats.
[1] Link: https://lore.kernel.org/r/[email protected]
---
drivers/gpu/drm/drm_fb_helper.c | 101 ++++++++++++++++++++++++--------
1 file changed, 75 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1705e8d345aba50a..5098efb374fe64ed 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -376,12 +376,31 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
struct iosys_map *dst)
{
struct drm_framebuffer *fb = fb_helper->fb;
- unsigned int cpp = fb->format->cpp[0];
- size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
- void *src = fb_helper->fbdev->screen_buffer + offset;
- size_t len = (clip->x2 - clip->x1) * cpp;
+ size_t offset = clip->y1 * fb->pitches[0];
+ size_t len = clip->x2 - clip->x1;
unsigned int y;
+ void *src;
+ switch (drm_format_info_bpp(fb->format, 0)) {
+ case 1:
+ offset += clip->x1 / 8;
+ len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
+ break;
+ case 2:
+ offset += clip->x1 / 4;
+ len = DIV_ROUND_UP(len + clip->x1 % 4, 4);
+ break;
+ case 4:
+ offset += clip->x1 / 2;
+ len = DIV_ROUND_UP(len + clip->x1 % 2, 2);
+ break;
+ default:
+ offset += clip->x1 * fb->format->cpp[0];
+ len *= fb->format->cpp[0];
+ break;
+ }
+
+ src = fb_helper->fbdev->screen_buffer + offset;
iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
for (y = clip->y1; y < clip->y2; y++) {
@@ -1273,19 +1292,23 @@ static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1,
}
static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
- u8 depth)
+ const struct drm_format_info *format)
{
- switch (depth) {
- case 8:
+ u8 depth = format->depth;
+
+ if (format->is_color_indexed) {
var->red.offset = 0;
var->green.offset = 0;
var->blue.offset = 0;
- var->red.length = 8; /* 8bit DAC */
- var->green.length = 8;
- var->blue.length = 8;
+ var->red.length = depth;
+ var->green.length = depth;
+ var->blue.length = depth;
var->transp.offset = 0;
var->transp.length = 0;
- break;
+ return;
+ }
+
+ switch (depth) {
case 15:
var->red.offset = 10;
var->green.offset = 5;
@@ -1340,7 +1363,9 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
{
struct drm_fb_helper *fb_helper = info->par;
struct drm_framebuffer *fb = fb_helper->fb;
+ const struct drm_format_info *format = fb->format;
struct drm_device *dev = fb_helper->dev;
+ unsigned int bpp;
if (in_dbg_master())
return -EINVAL;
@@ -1350,22 +1375,33 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
var->pixclock = 0;
}
- if ((drm_format_info_block_width(fb->format, 0) > 1) ||
- (drm_format_info_block_height(fb->format, 0) > 1))
- return -EINVAL;
+ switch (format->format) {
+ case DRM_FORMAT_C1:
+ case DRM_FORMAT_C2:
+ case DRM_FORMAT_C4:
+ /* supported format with sub-byte pixels */
+ break;
+
+ default:
+ if ((drm_format_info_block_width(format, 0) > 1) ||
+ (drm_format_info_block_height(format, 0) > 1))
+ return -EINVAL;
+ break;
+ }
/*
* Changes struct fb_var_screeninfo are currently not pushed back
* to KMS, hence fail if different settings are requested.
*/
- if (var->bits_per_pixel > fb->format->cpp[0] * 8 ||
+ bpp = drm_format_info_bpp(format, 0);
+ if (var->bits_per_pixel > bpp ||
var->xres > fb->width || var->yres > fb->height ||
var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in current fb "
"request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
var->xres, var->yres, var->bits_per_pixel,
var->xres_virtual, var->yres_virtual,
- fb->width, fb->height, fb->format->cpp[0] * 8);
+ fb->width, fb->height, bpp);
return -EINVAL;
}
@@ -1380,13 +1416,13 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
!var->blue.length && !var->transp.length &&
!var->red.msb_right && !var->green.msb_right &&
!var->blue.msb_right && !var->transp.msb_right) {
- drm_fb_helper_fill_pixel_fmt(var, fb->format->depth);
+ drm_fb_helper_fill_pixel_fmt(var, format);
}
/*
* Likewise, bits_per_pixel should be rounded up to a supported value.
*/
- var->bits_per_pixel = fb->format->cpp[0] * 8;
+ var->bits_per_pixel = bpp;
/*
* drm fbdev emulation doesn't support changing the pixel format at all,
@@ -1722,11 +1758,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
}
static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
- uint32_t depth)
+ bool is_color_indexed)
{
info->fix.type = FB_TYPE_PACKED_PIXELS;
- info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
- FB_VISUAL_TRUECOLOR;
+ info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
+ : FB_VISUAL_TRUECOLOR;
info->fix.mmio_start = 0;
info->fix.mmio_len = 0;
info->fix.type_aux = 0;
@@ -1743,19 +1779,31 @@ static void drm_fb_helper_fill_var(struct fb_info *info,
uint32_t fb_width, uint32_t fb_height)
{
struct drm_framebuffer *fb = fb_helper->fb;
+ const struct drm_format_info *format = fb->format;
+
+ switch (format->format) {
+ case DRM_FORMAT_C1:
+ case DRM_FORMAT_C2:
+ case DRM_FORMAT_C4:
+ /* supported format with sub-byte pixels */
+ break;
+
+ default:
+ WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
+ (drm_format_info_block_height(format, 0) > 1));
+ break;
+ }
- WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
- (drm_format_info_block_height(fb->format, 0) > 1));
info->pseudo_palette = fb_helper->pseudo_palette;
info->var.xres_virtual = fb->width;
info->var.yres_virtual = fb->height;
- info->var.bits_per_pixel = fb->format->cpp[0] * 8;
+ info->var.bits_per_pixel = drm_format_info_bpp(format, 0);
info->var.accel_flags = FB_ACCELF_TEXT;
info->var.xoffset = 0;
info->var.yoffset = 0;
info->var.activate = FB_ACTIVATE_NOW;
- drm_fb_helper_fill_pixel_fmt(&info->var, fb->format->depth);
+ drm_fb_helper_fill_pixel_fmt(&info->var, format);
info->var.xres = fb_width;
info->var.yres = fb_height;
@@ -1780,7 +1828,8 @@ void drm_fb_helper_fill_info(struct fb_info *info,
{
struct drm_framebuffer *fb = fb_helper->fb;
- drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->depth);
+ drm_fb_helper_fill_fix(info, fb->pitches[0],
+ fb->format->is_color_indexed);
drm_fb_helper_fill_var(info, fb_helper,
sizes->fb_width, sizes->fb_height);
--
2.25.1
When userspace queries the properties of a frame buffer, the number of
bits per pixel is derived from the deprecated drm_format_info.cpp[]
field, which does not take into account block sizes.
Fix this by using the actual number of bits per pixel instead.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
v3:
- Add Reviewed-by,
v2:
- No changes.
---
drivers/gpu/drm/drm_framebuffer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_framebuffer.c b/drivers/gpu/drm/drm_framebuffer.c
index 4562a8b865792456..9899bf1485b299ad 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -530,7 +530,7 @@ int drm_mode_getfb(struct drm_device *dev,
r->height = fb->height;
r->width = fb->width;
r->depth = fb->format->depth;
- r->bpp = fb->format->cpp[0] * 8;
+ r->bpp = drm_format_info_bpp(fb->format, 0);
r->pitch = fb->pitches[0];
/* GET_FB() is an unprivileged ioctl so we must not return a
--
2.25.1
As Rn covers single-channel formats with a direct relationship between
channel value and brightness, and Cn can be any colors, there are
currently no fourcc codes to describe single-channel formats with an
inverse relationship between channel value and brightness.
Introduce fourcc codes for a single-channel frame buffer format with
two, four, sixteen, or 256 brightness ("darkness") levels, where there
is an inverse relationship between channel value and brightness.
As the number of bits per pixel may be less than eight, some of these
formats rely on proper block handling for the calculation of bits per
pixel and pitch.
The fill order (the order in which multiple pixels are packed in a byte)
is the same order as used for grayscale (2, 4, and 16 levels) images in
the PNG specification, Version 1.2.
This order is also the recommended and default order (FillOrder = 1) for
bilevel and grayscale (16 levels) images in the TIFF 6.0 Specification,
and is also used for monochrome images in the PBM file format,
monochrome Linux frame buffer logos, and BDF and PSF (Linux kernel) font
files.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
RFC, as I have no immediate need for these formats.
v3:
- Add Reviewed-by,
- Document fill order,
- Replace light-on-dark/dark-on-light by direct/inverse relationship
between channel value and brightness,
v2:
- Add rationale for adding new formats,
- Improve pixel description,
- Add D[248] for completeness.
---
drivers/gpu/drm/drm_fourcc.c | 7 +++++++
include/uapi/drm/drm_fourcc.h | 12 ++++++++++++
2 files changed, 19 insertions(+)
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 05e65e9ab0c69c6a..e09331bb3bc73f21 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -154,6 +154,13 @@ const struct drm_format_info *__drm_format_info(u32 format)
{ .format = DRM_FORMAT_C4, .depth = 4, .num_planes = 1,
.char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
{ .format = DRM_FORMAT_C8, .depth = 8, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
+ { .format = DRM_FORMAT_D1, .depth = 1, .num_planes = 1,
+ .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+ { .format = DRM_FORMAT_D2, .depth = 2, .num_planes = 1,
+ .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+ { .format = DRM_FORMAT_D4, .depth = 4, .num_planes = 1,
+ .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+ { .format = DRM_FORMAT_D8, .depth = 8, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1 },
{ .format = DRM_FORMAT_R1, .depth = 1, .num_planes = 1,
.char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
{ .format = DRM_FORMAT_R2, .depth = 2, .num_planes = 1,
diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h
index 6db4f195d6a15129..7eff27eca7ce707f 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -104,6 +104,18 @@ extern "C" {
#define DRM_FORMAT_C4 fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
#define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
+/* 1 bpp Darkness (inverse relationship between channel value and brightness) */
+#define DRM_FORMAT_D1 fourcc_code('D', '1', ' ', ' ') /* [7:0] D0:D1:D2:D3:D4:D5:D6:D7 1:1:1:1:1:1:1:1 eight pixels/byte */
+
+/* 2 bpp Darkness (inverse relationship between channel value and brightness) */
+#define DRM_FORMAT_D2 fourcc_code('D', '2', ' ', ' ') /* [7:0] D0:D1:D2:D3 2:2:2:2 four pixels/byte */
+
+/* 4 bpp Darkness (inverse relationship between channel value and brightness) */
+#define DRM_FORMAT_D4 fourcc_code('D', '4', ' ', ' ') /* [7:0] D0:D1 4:4 two pixels/byte */
+
+/* 8 bpp Darkness (inverse relationship between channel value and brightness) */
+#define DRM_FORMAT_D8 fourcc_code('D', '8', ' ', ' ') /* [7:0] D */
+
/* 1 bpp Red (direct relationship between channel value and brightness) */
#define DRM_FORMAT_R1 fourcc_code('R', '1', ' ', ' ') /* [7:0] R0:R1:R2:R3:R4:R5:R6:R7 1:1:1:1:1:1:1:1 eight pixels/byte */
--
2.25.1
Introduce fourcc codes for single-channel frame buffer formats with two,
four, and sixteen brightness levels, where there is a direct
relationship between channel value and brightness.
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.
The fill order (the order in which multiple pixels are packed in a byte)
is the same order as used for grayscale (2, 4, and 16 levels) images in
the PNG specification, Version 1.2.
This order is also the recommended and default order (FillOrder = 1) for
bilevel and grayscale (16 levels) images in the TIFF 6.0 Specification,
and is also used for monochrome images in the PBM file format,
monochrome Linux frame buffer logos, and BDF and PSF (Linux kernel) font
files.
Signed-off-by: Geert Uytterhoeven <[email protected]>
Reviewed-by: Javier Martinez Canillas <[email protected]>
---
RFC, as I have no immediate need for these formats.
v3:
- Add Reviewed-by,
- Document fill order,
- Replace light-on-dark by direct relationship between channel value
and brightness,
v2:
- Improve pixel description.
---
drivers/gpu/drm/drm_fourcc.c | 6 ++++++
include/uapi/drm/drm_fourcc.h | 9 +++++++++
2 files changed, 15 insertions(+)
diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index 29f4fe199c4ddcf0..05e65e9ab0c69c6a 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -154,6 +154,12 @@ const struct drm_format_info *__drm_format_info(u32 format)
{ .format = DRM_FORMAT_C4, .depth = 4, .num_planes = 1,
.char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
{ .format = DRM_FORMAT_C8, .depth = 8, .num_planes = 1, .cpp = { 1, 0, 0 }, .hsub = 1, .vsub = 1, .is_color_indexed = true },
+ { .format = DRM_FORMAT_R1, .depth = 1, .num_planes = 1,
+ .char_per_block = { 1, }, .block_w = { 8, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+ { .format = DRM_FORMAT_R2, .depth = 2, .num_planes = 1,
+ .char_per_block = { 1, }, .block_w = { 4, }, .block_h = { 1, }, .hsub = 1, .vsub = 1 },
+ { .format = DRM_FORMAT_R4, .depth = 4, .num_planes = 1,
+ .char_per_block = { 1, }, .block_w = { 2, }, .block_h = { 1, }, .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 },
{ .format = DRM_FORMAT_R12, .depth = 12, .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 dff5072fccaaf65c..6db4f195d6a15129 100644
--- a/include/uapi/drm/drm_fourcc.h
+++ b/include/uapi/drm/drm_fourcc.h
@@ -104,6 +104,15 @@ extern "C" {
#define DRM_FORMAT_C4 fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */
#define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */
+/* 1 bpp Red (direct relationship between channel value and brightness) */
+#define DRM_FORMAT_R1 fourcc_code('R', '1', ' ', ' ') /* [7:0] R0:R1:R2:R3:R4:R5:R6:R7 1:1:1:1:1:1:1:1 eight pixels/byte */
+
+/* 2 bpp Red (direct relationship between channel value and brightness) */
+#define DRM_FORMAT_R2 fourcc_code('R', '2', ' ', ' ') /* [7:0] R0:R1:R2:R3 2:2:2:2 four pixels/byte */
+
+/* 4 bpp Red (direct relationship between channel value and brightness) */
+#define DRM_FORMAT_R4 fourcc_code('R', '4', ' ', ' ') /* [7:0] R0:R1 4:4 two pixels/byte */
+
/* 8 bpp Red (direct relationship between channel value and brightness) */
#define DRM_FORMAT_R8 fourcc_code('R', '8', ' ', ' ') /* [7:0] R */
--
2.25.1
Hi Geert,
On Fri, Jul 08, 2022 at 08:20:45PM +0200, 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 on small embedded displays.
IT is super to have this addressed - thanks!
>
> 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 supporting 2, 4, 16, 256, and 65536
> colors, with text console operation, fbtest, and modetest.
>
> Overview:
> - Patch 1 introduces a helper, to be used by later patches in the
> series,
> - Patch 2 introduces a flag to indicate color-indexed formats,
> - Patches 3 and 4 correct calculations of bits per pixel for sub-byte
> pixel formats,
> - Patches 5 and 6 introduce the new C[124] formats,
> - Patch 7 fixes an untested code path,
> - Patch 8 documents the use of "red" for light-on-dark displays,
> - Patches 9 and 10 add more fourcc codes for light-on-dark and
> dark-on-light frame buffer formats, which may be useful for e.g. the
> ssd130x and repaper drivers.
Applied all patches to drm-misc (drm-misc-next), including the last two
RFC patches as we then have the formats ready when a user pops up.
Sam
Hi
Am 09.07.22 um 15:38 schrieb Sam Ravnborg:
> Hi Geert,
>
> On Fri, Jul 08, 2022 at 08:20:45PM +0200, 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 on small embedded displays.
For the patchset
Acked-by: Thomas Zimemrmann <[email protected]>
>
> IT is super to have this addressed - thanks!
>
>>
>> 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 supporting 2, 4, 16, 256, and 65536
>> colors, with text console operation, fbtest, and modetest.
>>
>> Overview:
>> - Patch 1 introduces a helper, to be used by later patches in the
>> series,
>> - Patch 2 introduces a flag to indicate color-indexed formats,
>> - Patches 3 and 4 correct calculations of bits per pixel for sub-byte
>> pixel formats,
>> - Patches 5 and 6 introduce the new C[124] formats,
>> - Patch 7 fixes an untested code path,
>> - Patch 8 documents the use of "red" for light-on-dark displays,
>> - Patches 9 and 10 add more fourcc codes for light-on-dark and
>> dark-on-light frame buffer formats, which may be useful for e.g. the
>> ssd130x and repaper drivers.
>
> Applied all patches to drm-misc (drm-misc-next), including the last two
> RFC patches as we then have the formats ready when a user pops up.
I know it's v3 already, but give people at least a workday for reviewing
before merging patches of this size and impact. Friday-evening patches
are not supposed to be merged on Saturday afternoons.
Best regards
Thomas
>
> Sam
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
Hi Thomas,
On Mon, Jul 11, 2022 at 10:50:00AM +0200, Thomas Zimmermann wrote:
> Hi
>
> Am 09.07.22 um 15:38 schrieb Sam Ravnborg:
> > Hi Geert,
> >
> > On Fri, Jul 08, 2022 at 08:20:45PM +0200, 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 on small embedded displays.
>
> For the patchset
>
> Acked-by: Thomas Zimemrmann <[email protected]>
>
> >
> > IT is super to have this addressed - thanks!
> >
> > >
> > > 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 supporting 2, 4, 16, 256, and 65536
> > > colors, with text console operation, fbtest, and modetest.
> > >
> > > Overview:
> > > - Patch 1 introduces a helper, to be used by later patches in the
> > > series,
> > > - Patch 2 introduces a flag to indicate color-indexed formats,
> > > - Patches 3 and 4 correct calculations of bits per pixel for sub-byte
> > > pixel formats,
> > > - Patches 5 and 6 introduce the new C[124] formats,
> > > - Patch 7 fixes an untested code path,
> > > - Patch 8 documents the use of "red" for light-on-dark displays,
> > > - Patches 9 and 10 add more fourcc codes for light-on-dark and
> > > dark-on-light frame buffer formats, which may be useful for e.g. the
> > > ssd130x and repaper drivers.
> >
> > Applied all patches to drm-misc (drm-misc-next), including the last two
> > RFC patches as we then have the formats ready when a user pops up.
>
> I know it's v3 already, but give people at least a workday for reviewing
> before merging patches of this size and impact. Friday-evening patches are
> not supposed to be merged on Saturday afternoons.
Sorry for being too enthusiastic on this one.
Will wait a bit more in the future for these kind of patches.
Sam
Hi
Am 08.07.22 um 20:20 schrieb Geert Uytterhoeven:
> Add support for color-indexed frame buffer formats with two, four, and
> sixteen colors to the DRM framebuffer helper functions:
> 1. Add support for 1, 2, and 4 bits per pixel to the damage helper,
> 2. For color-indexed modes, the length of the color bitfields must be
> set to the color depth, else the logo code may pick a logo with too
> many colors. Drop the incorrect DAC width comment, which
> originates from the i915 driver.
> 3. Accept C[124] modes when validating or filling in struct
> fb_var_screeninfo, and use the correct number of bits per pixel.
> 4. Set the visual to FB_VISUAL_PSEUDOCOLOR for all color-indexed
> modes.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> Reviewed-by: Javier Martinez Canillas <[email protected]>
> ---
> If "[PATCH] drm/fb-helper: Remove helpers to change frame buffer
> config"[1] is accepted, the changes to drm_fb_helper_check_var() can
> just be removed.
>
> v3:
> - Fix FB_VISUAL_TRUECOLOR,
> - Add Reviewed-by,
>
> v2:
> - Use drm_format_info_bpp() helper instead of deprecated .depth field
> or format-dependent calculations,
> - Use new .is_color_indexed field instead of checking against a list
> of formats.
>
> [1] Link: https://lore.kernel.org/r/[email protected]
> ---
> drivers/gpu/drm/drm_fb_helper.c | 101 ++++++++++++++++++++++++--------
> 1 file changed, 75 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 1705e8d345aba50a..5098efb374fe64ed 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -376,12 +376,31 @@ static void drm_fb_helper_damage_blit_real(struct drm_fb_helper *fb_helper,
> struct iosys_map *dst)
> {
> struct drm_framebuffer *fb = fb_helper->fb;
> - unsigned int cpp = fb->format->cpp[0];
> - size_t offset = clip->y1 * fb->pitches[0] + clip->x1 * cpp;
> - void *src = fb_helper->fbdev->screen_buffer + offset;
> - size_t len = (clip->x2 - clip->x1) * cpp;
> + size_t offset = clip->y1 * fb->pitches[0];
> + size_t len = clip->x2 - clip->x1;
drm_rect_width() please.
> unsigned int y;
> + void *src;
>
> + switch (drm_format_info_bpp(fb->format, 0)) {
> + case 1:
> + offset += clip->x1 / 8;
> + len = DIV_ROUND_UP(len + clip->x1 % 8, 8);
> + break;
> + case 2:
> + offset += clip->x1 / 4;
> + len = DIV_ROUND_UP(len + clip->x1 % 4, 4);
> + break;
> + case 4:
> + offset += clip->x1 / 2;
> + len = DIV_ROUND_UP(len + clip->x1 % 2, 2);
> + break;
> + default:
> + offset += clip->x1 * fb->format->cpp[0];
> + len *= fb->format->cpp[0];
> + break;
> + }
> +
> + src = fb_helper->fbdev->screen_buffer + offset;
> iosys_map_incr(dst, offset); /* go to first pixel within clip rect */
>
> for (y = clip->y1; y < clip->y2; y++) {
> @@ -1273,19 +1292,23 @@ static bool drm_fb_pixel_format_equal(const struct fb_var_screeninfo *var_1,
> }
>
> static void drm_fb_helper_fill_pixel_fmt(struct fb_var_screeninfo *var,
> - u8 depth)
> + const struct drm_format_info *format)
> {
> - switch (depth) {
> - case 8:
> + u8 depth = format->depth;
> +
> + if (format->is_color_indexed) {
> var->red.offset = 0;
> var->green.offset = 0;
> var->blue.offset = 0;
> - var->red.length = 8; /* 8bit DAC */
> - var->green.length = 8;
> - var->blue.length = 8;
> + var->red.length = depth;
> + var->green.length = depth;
> + var->blue.length = depth;
> var->transp.offset = 0;
> var->transp.length = 0;
> - break;
> + return;
> + }
> +
> + switch (depth) {
> case 15:
> var->red.offset = 10;
> var->green.offset = 5;
> @@ -1340,7 +1363,9 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> {
> struct drm_fb_helper *fb_helper = info->par;
> struct drm_framebuffer *fb = fb_helper->fb;
> + const struct drm_format_info *format = fb->format;
> struct drm_device *dev = fb_helper->dev;
> + unsigned int bpp;
>
> if (in_dbg_master())
> return -EINVAL;
> @@ -1350,22 +1375,33 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> var->pixclock = 0;
> }
>
> - if ((drm_format_info_block_width(fb->format, 0) > 1) ||
> - (drm_format_info_block_height(fb->format, 0) > 1))
> - return -EINVAL;
> + switch (format->format) {
> + case DRM_FORMAT_C1:
> + case DRM_FORMAT_C2:
> + case DRM_FORMAT_C4:
> + /* supported format with sub-byte pixels */
> + break;
> +
> + default:
> + if ((drm_format_info_block_width(format, 0) > 1) ||
> + (drm_format_info_block_height(format, 0) > 1))
> + return -EINVAL;
> + break;
> + }
>
> /*
> * Changes struct fb_var_screeninfo are currently not pushed back
> * to KMS, hence fail if different settings are requested.
> */
> - if (var->bits_per_pixel > fb->format->cpp[0] * 8 ||
> + bpp = drm_format_info_bpp(format, 0);
> + if (var->bits_per_pixel > bpp ||
> var->xres > fb->width || var->yres > fb->height ||
> var->xres_virtual > fb->width || var->yres_virtual > fb->height) {
> drm_dbg_kms(dev, "fb requested width/height/bpp can't fit in current fb "
> "request %dx%d-%d (virtual %dx%d) > %dx%d-%d\n",
> var->xres, var->yres, var->bits_per_pixel,
> var->xres_virtual, var->yres_virtual,
> - fb->width, fb->height, fb->format->cpp[0] * 8);
> + fb->width, fb->height, bpp);
> return -EINVAL;
> }
>
> @@ -1380,13 +1416,13 @@ int drm_fb_helper_check_var(struct fb_var_screeninfo *var,
> !var->blue.length && !var->transp.length &&
> !var->red.msb_right && !var->green.msb_right &&
> !var->blue.msb_right && !var->transp.msb_right) {
> - drm_fb_helper_fill_pixel_fmt(var, fb->format->depth);
> + drm_fb_helper_fill_pixel_fmt(var, format);
> }
>
> /*
> * Likewise, bits_per_pixel should be rounded up to a supported value.
> */
> - var->bits_per_pixel = fb->format->cpp[0] * 8;
> + var->bits_per_pixel = bpp;
>
> /*
> * drm fbdev emulation doesn't support changing the pixel format at all,
> @@ -1722,11 +1758,11 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
> }
>
> static void drm_fb_helper_fill_fix(struct fb_info *info, uint32_t pitch,
> - uint32_t depth)
> + bool is_color_indexed)
> {
> info->fix.type = FB_TYPE_PACKED_PIXELS;
> - info->fix.visual = depth == 8 ? FB_VISUAL_PSEUDOCOLOR :
> - FB_VISUAL_TRUECOLOR;
> + info->fix.visual = is_color_indexed ? FB_VISUAL_PSEUDOCOLOR
> + : FB_VISUAL_TRUECOLOR;
> info->fix.mmio_start = 0;
> info->fix.mmio_len = 0;
> info->fix.type_aux = 0;
> @@ -1743,19 +1779,31 @@ static void drm_fb_helper_fill_var(struct fb_info *info,
> uint32_t fb_width, uint32_t fb_height)
> {
> struct drm_framebuffer *fb = fb_helper->fb;
> + const struct drm_format_info *format = fb->format;
> +
> + switch (format->format) {
> + case DRM_FORMAT_C1:
> + case DRM_FORMAT_C2:
> + case DRM_FORMAT_C4:
> + /* supported format with sub-byte pixels */
> + break;
> +
> + default:
> + WARN_ON((drm_format_info_block_width(format, 0) > 1) ||
> + (drm_format_info_block_height(format, 0) > 1));
> + break;
> + }
>
> - WARN_ON((drm_format_info_block_width(fb->format, 0) > 1) ||
> - (drm_format_info_block_height(fb->format, 0) > 1));
> info->pseudo_palette = fb_helper->pseudo_palette;
> info->var.xres_virtual = fb->width;
> info->var.yres_virtual = fb->height;
> - info->var.bits_per_pixel = fb->format->cpp[0] * 8;
> + info->var.bits_per_pixel = drm_format_info_bpp(format, 0);
> info->var.accel_flags = FB_ACCELF_TEXT;
> info->var.xoffset = 0;
> info->var.yoffset = 0;
> info->var.activate = FB_ACTIVATE_NOW;
>
> - drm_fb_helper_fill_pixel_fmt(&info->var, fb->format->depth);
> + drm_fb_helper_fill_pixel_fmt(&info->var, format);
>
> info->var.xres = fb_width;
> info->var.yres = fb_height;
> @@ -1780,7 +1828,8 @@ void drm_fb_helper_fill_info(struct fb_info *info,
> {
> struct drm_framebuffer *fb = fb_helper->fb;
>
> - drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->depth);
> + drm_fb_helper_fill_fix(info, fb->pitches[0],
> + fb->format->is_color_indexed);
> drm_fb_helper_fill_var(info, fb_helper,
> sizes->fb_width, sizes->fb_height);
>
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev
On Fri, Jul 08, 2022 at 08:20:46PM +0200, Geert Uytterhoeven wrote:
> Add a helper to retrieve the actual number of bits per pixel for a
> plane, taking into account the number of characters and pixels per
> block for tiled formats.
>
> Signed-off-by: Geert Uytterhoeven <[email protected]>
> Reviewed-by: Javier Martinez Canillas <[email protected]>
> ---
> v3:
> - Add Reviewed-by,
>
> v2:
> - Move up.
> ---
> drivers/gpu/drm/drm_fourcc.c | 19 +++++++++++++++++++
> include/drm/drm_fourcc.h | 1 +
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
> index 07741b678798b0f1..cf48ea0b2cb70ba8 100644
> --- a/drivers/gpu/drm/drm_fourcc.c
> +++ b/drivers/gpu/drm/drm_fourcc.c
> @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> }
> EXPORT_SYMBOL(drm_format_info_block_height);
>
> +/**
> + * drm_format_info_bpp - number of bits per pixel
> + * @info: pixel format info
> + * @plane: plane index
> + *
> + * Returns:
> + * The actual number of bits per pixel, depending on the plane index.
> + */
> +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane)
> +{
> + if (!info || plane < 0 || plane >= info->num_planes)
> + return 0;
> +
> + return info->char_per_block[plane] * 8 /
> + (drm_format_info_block_width(info, plane) *
> + drm_format_info_block_height(info, plane));
Do we really needs this for blocky formats where this is potentially
ill-defined? I think if there's no need then this should also return 0
when block_width/height != 1, it doesn't make much sense to compute bpp
when it's not really bits per _pixel_.
Minimally this needs to check whether the division actually makes sense or
whether there's a reminder, and if there's reminder, then fail. But that
feels like a bad hack and I think we should avoid it if it's not
absolutely necessary.
Otherwise lgtm.
-Daniel
> +}
> +EXPORT_SYMBOL(drm_format_info_bpp);
> +
> /**
> * drm_format_info_min_pitch - computes the minimum required pitch in bytes
> * @info: pixel format info
> diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h
> index 22aa64d07c7905e2..3800a7ad7f0cda7a 100644
> --- a/include/drm/drm_fourcc.h
> +++ b/include/drm/drm_fourcc.h
> @@ -313,6 +313,7 @@ unsigned int drm_format_info_block_width(const struct drm_format_info *info,
> int plane);
> unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> int plane);
> +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane);
> uint64_t drm_format_info_min_pitch(const struct drm_format_info *info,
> int plane, unsigned int buffer_width);
>
> --
> 2.25.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
On Mon, Jul 11, 2022 at 11:12:51AM +0200, Sam Ravnborg wrote:
> Hi Thomas,
>
> On Mon, Jul 11, 2022 at 10:50:00AM +0200, Thomas Zimmermann wrote:
> > Hi
> >
> > Am 09.07.22 um 15:38 schrieb Sam Ravnborg:
> > > Hi Geert,
> > >
> > > On Fri, Jul 08, 2022 at 08:20:45PM +0200, 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 on small embedded displays.
> >
> > For the patchset
> >
> > Acked-by: Thomas Zimemrmann <[email protected]>
> >
> > >
> > > IT is super to have this addressed - thanks!
> > >
> > > >
> > > > 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 supporting 2, 4, 16, 256, and 65536
> > > > colors, with text console operation, fbtest, and modetest.
> > > >
> > > > Overview:
> > > > - Patch 1 introduces a helper, to be used by later patches in the
> > > > series,
> > > > - Patch 2 introduces a flag to indicate color-indexed formats,
> > > > - Patches 3 and 4 correct calculations of bits per pixel for sub-byte
> > > > pixel formats,
> > > > - Patches 5 and 6 introduce the new C[124] formats,
> > > > - Patch 7 fixes an untested code path,
> > > > - Patch 8 documents the use of "red" for light-on-dark displays,
> > > > - Patches 9 and 10 add more fourcc codes for light-on-dark and
> > > > dark-on-light frame buffer formats, which may be useful for e.g. the
> > > > ssd130x and repaper drivers.
> > >
> > > Applied all patches to drm-misc (drm-misc-next), including the last two
> > > RFC patches as we then have the formats ready when a user pops up.
> >
> > I know it's v3 already, but give people at least a workday for reviewing
> > before merging patches of this size and impact. Friday-evening patches are
> > not supposed to be merged on Saturday afternoons.
>
> Sorry for being too enthusiastic on this one.
> Will wait a bit more in the future for these kind of patches.
Took me a bit longer to unburry and get to this, and lgtm except patch 1
where I have a semantic concern. Can you pls do the quick patch to adjust
that? Since this is all about the Cx/Rx/Dx formats I don't think it'll
matter really.
Thanks, Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Daniel,
On Wed, Aug 10, 2022 at 5:59 PM Daniel Vetter <[email protected]> wrote:
> On Fri, Jul 08, 2022 at 08:20:46PM +0200, Geert Uytterhoeven wrote:
> > Add a helper to retrieve the actual number of bits per pixel for a
> > plane, taking into account the number of characters and pixels per
> > block for tiled formats.
> >
> > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > Reviewed-by: Javier Martinez Canillas <[email protected]>
> > --- a/drivers/gpu/drm/drm_fourcc.c
> > +++ b/drivers/gpu/drm/drm_fourcc.c
> > @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > }
> > EXPORT_SYMBOL(drm_format_info_block_height);
> >
> > +/**
> > + * drm_format_info_bpp - number of bits per pixel
> > + * @info: pixel format info
> > + * @plane: plane index
> > + *
> > + * Returns:
> > + * The actual number of bits per pixel, depending on the plane index.
> > + */
> > +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane)
> > +{
> > + if (!info || plane < 0 || plane >= info->num_planes)
> > + return 0;
> > +
> > + return info->char_per_block[plane] * 8 /
> > + (drm_format_info_block_width(info, plane) *
> > + drm_format_info_block_height(info, plane));
>
> Do we really needs this for blocky formats where this is potentially
> ill-defined? I think if there's no need then this should also return 0
> when block_width/height != 1, it doesn't make much sense to compute bpp
> when it's not really bits per _pixel_.
Yes, we do need this. For low-color formats, the number of bits
per pixel is less than eight, and block_width is larger than one.
That is actually the point of this patch.
> Minimally this needs to check whether the division actually makes sense or
> whether there's a reminder, and if there's reminder, then fail. But that
> feels like a bad hack and I think we should avoid it if it's not
> absolutely necessary.
Looking at drivers/gpu/drm/drm_fourcc.c, the only supported format
where there can be a remainder is P030, which has 2 spare bits per
32-bit word, and thus is special anyway.
Still, 4 * 8 / 3 = 10, so you get the correct numbers of bits for
the first plane. For the second plane, you get 8 * 8 / 3 = 21,
but as .is_yuv = true, you have to divide that result by two again,
so you get 10 again.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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 Thu, Aug 11, 2022 at 09:59:39AM +0200, Geert Uytterhoeven wrote:
> Hi Daniel,
>
> On Wed, Aug 10, 2022 at 5:59 PM Daniel Vetter <[email protected]> wrote:
> > On Fri, Jul 08, 2022 at 08:20:46PM +0200, Geert Uytterhoeven wrote:
> > > Add a helper to retrieve the actual number of bits per pixel for a
> > > plane, taking into account the number of characters and pixels per
> > > block for tiled formats.
> > >
> > > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > > Reviewed-by: Javier Martinez Canillas <[email protected]>
>
> > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > > }
> > > EXPORT_SYMBOL(drm_format_info_block_height);
> > >
> > > +/**
> > > + * drm_format_info_bpp - number of bits per pixel
> > > + * @info: pixel format info
> > > + * @plane: plane index
> > > + *
> > > + * Returns:
> > > + * The actual number of bits per pixel, depending on the plane index.
> > > + */
> > > +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane)
> > > +{
> > > + if (!info || plane < 0 || plane >= info->num_planes)
> > > + return 0;
> > > +
> > > + return info->char_per_block[plane] * 8 /
> > > + (drm_format_info_block_width(info, plane) *
> > > + drm_format_info_block_height(info, plane));
> >
> > Do we really needs this for blocky formats where this is potentially
> > ill-defined? I think if there's no need then this should also return 0
> > when block_width/height != 1, it doesn't make much sense to compute bpp
> > when it's not really bits per _pixel_.
>
> Yes, we do need this. For low-color formats, the number of bits
> per pixel is less than eight, and block_width is larger than one.
> That is actually the point of this patch.
Hm right, I didn't realize that this is how we have to describe the
formats with less than 8 bpp.
I think we can include them easily with a check for char_per_block == 1
and then making sure that the division does not have a reminder (just in
case someone does something really funny, it could e.g. be a 332 layout or
something like that for 3 pixels).
> > Minimally this needs to check whether the division actually makes sense or
> > whether there's a reminder, and if there's reminder, then fail. But that
> > feels like a bad hack and I think we should avoid it if it's not
> > absolutely necessary.
>
> Looking at drivers/gpu/drm/drm_fourcc.c, the only supported format
> where there can be a remainder is P030, which has 2 spare bits per
> 32-bit word, and thus is special anyway.
> Still, 4 * 8 / 3 = 10, so you get the correct numbers of bits for
> the first plane. For the second plane, you get 8 * 8 / 3 = 21,
> but as .is_yuv = true, you have to divide that result by two again,
> so you get 10 again.
Yeah I don't think we should describe these with bpp or cpp or anything
like that. bpp < 8 makes sense since that's how this has been done since
decades, but trying to extend these to funny new formats is a bad idea.
This is also why cpp and depth refuse to compute these (or at least
should).
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
Hi Geert, Daniel.
On Thu, Aug 11, 2022 at 06:11:40PM +0200, Daniel Vetter wrote:
> On Thu, Aug 11, 2022 at 09:59:39AM +0200, Geert Uytterhoeven wrote:
> > Hi Daniel,
> >
> > On Wed, Aug 10, 2022 at 5:59 PM Daniel Vetter <[email protected]> wrote:
> > > On Fri, Jul 08, 2022 at 08:20:46PM +0200, Geert Uytterhoeven wrote:
> > > > Add a helper to retrieve the actual number of bits per pixel for a
> > > > plane, taking into account the number of characters and pixels per
> > > > block for tiled formats.
> > > >
> > > > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > > > Reviewed-by: Javier Martinez Canillas <[email protected]>
> >
> > > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > > @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > > > }
> > > > EXPORT_SYMBOL(drm_format_info_block_height);
> > > >
> > > > +/**
> > > > + * drm_format_info_bpp - number of bits per pixel
> > > > + * @info: pixel format info
> > > > + * @plane: plane index
> > > > + *
> > > > + * Returns:
> > > > + * The actual number of bits per pixel, depending on the plane index.
> > > > + */
> > > > +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane)
> > > > +{
> > > > + if (!info || plane < 0 || plane >= info->num_planes)
> > > > + return 0;
> > > > +
> > > > + return info->char_per_block[plane] * 8 /
> > > > + (drm_format_info_block_width(info, plane) *
> > > > + drm_format_info_block_height(info, plane));
> > >
> > > Do we really needs this for blocky formats where this is potentially
> > > ill-defined? I think if there's no need then this should also return 0
> > > when block_width/height != 1, it doesn't make much sense to compute bpp
> > > when it's not really bits per _pixel_.
> >
> > Yes, we do need this. For low-color formats, the number of bits
> > per pixel is less than eight, and block_width is larger than one.
> > That is actually the point of this patch.
>
> Hm right, I didn't realize that this is how we have to describe the
> formats with less than 8 bpp.
>
> I think we can include them easily with a check for char_per_block == 1
> and then making sure that the division does not have a reminder (just in
> case someone does something really funny, it could e.g. be a 332 layout or
> something like that for 3 pixels).
>
> > > Minimally this needs to check whether the division actually makes sense or
> > > whether there's a reminder, and if there's reminder, then fail. But that
> > > feels like a bad hack and I think we should avoid it if it's not
> > > absolutely necessary.
> >
> > Looking at drivers/gpu/drm/drm_fourcc.c, the only supported format
> > where there can be a remainder is P030, which has 2 spare bits per
> > 32-bit word, and thus is special anyway.
> > Still, 4 * 8 / 3 = 10, so you get the correct numbers of bits for
> > the first plane. For the second plane, you get 8 * 8 / 3 = 21,
> > but as .is_yuv = true, you have to divide that result by two again,
> > so you get 10 again.
>
> Yeah I don't think we should describe these with bpp or cpp or anything
> like that. bpp < 8 makes sense since that's how this has been done since
> decades, but trying to extend these to funny new formats is a bad idea.
> This is also why cpp and depth refuse to compute these (or at least
> should).
Daniel and I discussed this on irc. Let me try to recap here.
Using the bits per pixel info from drm_format_info is something we shall
try to avoid as this is often a sign of the wrong abstraction/design (my
words based on the irc talk).
So we shall limit the use of drm_format_info_bpp() to what we need now,
thus blocky formats should not be supported - to try to avoid seeing
this used more than necessary.
Daniel suggested a rename to drm_format_info_legacy_bpp() to make it
obvious that this is often/always the wrong solution. I did not jump on
doing the rename as I do not know stuff good enough to tell people what
to use when this is not the right solution. The rename is simple, it is
the follow-up that keep me away.
On top of this there is a few formats in drm_drourcc that has a depth
field set which should be dropped. .depth is only for the few legacy
formats where it is used today.
We would also like to convert the fbdev helpers to drm_format_info,
and doing so will likely teach us a bit more what we need and what we
can drop.
Geert - can you give drm_format_info_bpp() a spin so it is limited to
the formats used now (not the blocky ones).
Sam
Hi Sam,
On Thu, Aug 11, 2022 at 8:29 PM Sam Ravnborg <[email protected]> wrote:
> On Thu, Aug 11, 2022 at 06:11:40PM +0200, Daniel Vetter wrote:
> > On Thu, Aug 11, 2022 at 09:59:39AM +0200, Geert Uytterhoeven wrote:
> > > On Wed, Aug 10, 2022 at 5:59 PM Daniel Vetter <[email protected]> wrote:
> > > > On Fri, Jul 08, 2022 at 08:20:46PM +0200, Geert Uytterhoeven wrote:
> > > > > Add a helper to retrieve the actual number of bits per pixel for a
> > > > > plane, taking into account the number of characters and pixels per
> > > > > block for tiled formats.
> > > > >
> > > > > Signed-off-by: Geert Uytterhoeven <[email protected]>
> > > > > Reviewed-by: Javier Martinez Canillas <[email protected]>
> > >
> > > > > --- a/drivers/gpu/drm/drm_fourcc.c
> > > > > +++ b/drivers/gpu/drm/drm_fourcc.c
> > > > > @@ -370,6 +370,25 @@ unsigned int drm_format_info_block_height(const struct drm_format_info *info,
> > > > > }
> > > > > EXPORT_SYMBOL(drm_format_info_block_height);
> > > > >
> > > > > +/**
> > > > > + * drm_format_info_bpp - number of bits per pixel
> > > > > + * @info: pixel format info
> > > > > + * @plane: plane index
> > > > > + *
> > > > > + * Returns:
> > > > > + * The actual number of bits per pixel, depending on the plane index.
> > > > > + */
> > > > > +unsigned int drm_format_info_bpp(const struct drm_format_info *info, int plane)
> > > > > +{
> > > > > + if (!info || plane < 0 || plane >= info->num_planes)
> > > > > + return 0;
> > > > > +
> > > > > + return info->char_per_block[plane] * 8 /
> > > > > + (drm_format_info_block_width(info, plane) *
> > > > > + drm_format_info_block_height(info, plane));
> > > >
> > > > Do we really needs this for blocky formats where this is potentially
> > > > ill-defined? I think if there's no need then this should also return 0
> > > > when block_width/height != 1, it doesn't make much sense to compute bpp
> > > > when it's not really bits per _pixel_.
> > >
> > > Yes, we do need this. For low-color formats, the number of bits
> > > per pixel is less than eight, and block_width is larger than one.
> > > That is actually the point of this patch.
> >
> > Hm right, I didn't realize that this is how we have to describe the
> > formats with less than 8 bpp.
> >
> > I think we can include them easily with a check for char_per_block == 1
> > and then making sure that the division does not have a reminder (just in
> > case someone does something really funny, it could e.g. be a 332 layout or
> > something like that for 3 pixels).
> >
> > > > Minimally this needs to check whether the division actually makes sense or
> > > > whether there's a reminder, and if there's reminder, then fail. But that
> > > > feels like a bad hack and I think we should avoid it if it's not
> > > > absolutely necessary.
> > >
> > > Looking at drivers/gpu/drm/drm_fourcc.c, the only supported format
> > > where there can be a remainder is P030, which has 2 spare bits per
> > > 32-bit word, and thus is special anyway.
> > > Still, 4 * 8 / 3 = 10, so you get the correct numbers of bits for
> > > the first plane. For the second plane, you get 8 * 8 / 3 = 21,
> > > but as .is_yuv = true, you have to divide that result by two again,
> > > so you get 10 again.
> >
> > Yeah I don't think we should describe these with bpp or cpp or anything
> > like that. bpp < 8 makes sense since that's how this has been done since
> > decades, but trying to extend these to funny new formats is a bad idea.
> > This is also why cpp and depth refuse to compute these (or at least
> > should).
>
> Daniel and I discussed this on irc. Let me try to recap here.
> Using the bits per pixel info from drm_format_info is something we shall
> try to avoid as this is often a sign of the wrong abstraction/design (my
> words based on the irc talk).
> So we shall limit the use of drm_format_info_bpp() to what we need now,
> thus blocky formats should not be supported - to try to avoid seeing
> this used more than necessary.
>
> Daniel suggested a rename to drm_format_info_legacy_bpp() to make it
> obvious that this is often/always the wrong solution. I did not jump on
> doing the rename as I do not know stuff good enough to tell people what
> to use when this is not the right solution. The rename is simple, it is
> the follow-up that keep me away.
>
> On top of this there is a few formats in drm_drourcc that has a depth
> field set which should be dropped. .depth is only for the few legacy
> formats where it is used today.
>
> We would also like to convert the fbdev helpers to drm_format_info,
> and doing so will likely teach us a bit more what we need and what we
> can drop.
>
> Geert - can you give drm_format_info_bpp() a spin so it is limited to
> the formats used now (not the blocky ones).
You mean return 0 if char_per_block[] > 1?
I'm not sure it's actually safe to do so (and make this change this late
in the development cycle), as this is used in drm_client_buffer_create(),
drm_client_buffer_addfb(), and drm_mode_getfb(). Some of them do
rely on bpp to be non-zero.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
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.
> > >
> > > Yeah I don't think we should describe these with bpp or cpp or anything
> > > like that. bpp < 8 makes sense since that's how this has been done since
> > > decades, but trying to extend these to funny new formats is a bad idea.
> > > This is also why cpp and depth refuse to compute these (or at least
> > > should).
> >
> > Daniel and I discussed this on irc. Let me try to recap here.
> > Using the bits per pixel info from drm_format_info is something we shall
> > try to avoid as this is often a sign of the wrong abstraction/design (my
> > words based on the irc talk).
> > So we shall limit the use of drm_format_info_bpp() to what we need now,
> > thus blocky formats should not be supported - to try to avoid seeing
> > this used more than necessary.
> >
> > Daniel suggested a rename to drm_format_info_legacy_bpp() to make it
> > obvious that this is often/always the wrong solution. I did not jump on
> > doing the rename as I do not know stuff good enough to tell people what
> > to use when this is not the right solution. The rename is simple, it is
> > the follow-up that keep me away.
> >
> > On top of this there is a few formats in drm_drourcc that has a depth
> > field set which should be dropped. .depth is only for the few legacy
> > formats where it is used today.
> >
> > We would also like to convert the fbdev helpers to drm_format_info,
> > and doing so will likely teach us a bit more what we need and what we
> > can drop.
> >
> > Geert - can you give drm_format_info_bpp() a spin so it is limited to
> > the formats used now (not the blocky ones).
>
> You mean return 0 if char_per_block[] > 1?
if char_per_block[] > 1 AND block_w[] > 0 AND block_h[] > 0 should be
enough.
> I'm not sure it's actually safe to do so (and make this change this late
> in the development cycle), as this is used in drm_client_buffer_create(),
> drm_client_buffer_addfb(), and drm_mode_getfb().
drm_client_buffer_create() and drm_client_buffer_addfb() both get their
format from drm_mode_legacy_fb_format() which do not produce any blocky
formats - so they are good.
drm_mode_getfb() looks up a framebuffer originally created using one of
the above (I think), so here it should also be fine.
I do not see the need to push this to fixes, so it has a full cycle to
mature if it causes issues.
Sam