Re: [PATCH v4 0/5] drm/tiny: panel-mipi-dbi: Support 18 bits per color RGB666
On 6/4/24 15:20, Noralf Trønnes via B4 Relay wrote: > Hi, > > In this version I've fixed up a commit message that I had forgotten to > write before sending and improved a struct member name. > > See version 1 of the patchset for the full cover letter. > > Signed-off-by: Noralf Trønnes > --- Thanks a lot for reviewing the patches, applied to drm-misc-next. Noralf. > Changes in v4: > - Expand the commit message (Dmitry) > - s/emulation_format/pixel_format/ (Dmitry) > - Link to v3: > https://lore.kernel.org/r/20240603-panel-mipi-dbi-rgb666-v3-0-59ed53ca7...@tronnes.org > > Changes in v3: > - Added r-b's to patch 1 and 5 > - Link to v2: > https://lore.kernel.org/r/20240512-panel-mipi-dbi-rgb666-v2-0-49dd26632...@tronnes.org > > Changes in v2: > - binding: Use 'default: r5g6b5' (Rob) > - Link to v1: > https://lore.kernel.org/r/20240507-panel-mipi-dbi-rgb666-v1-0-6799234af...@tronnes.org > > --- > Noralf Trønnes (5): > dt-bindings: display: panel: mipi-dbi-spi: Add a pixel format property > drm/mipi-dbi: Remove mipi_dbi_machine_little_endian() > drm/mipi-dbi: Make bits per word configurable for pixel transfers > drm/mipi-dbi: Add support for DRM_FORMAT_RGB888 > drm/tiny: panel-mipi-dbi: Support the pixel format property > > .../bindings/display/panel/panel-mipi-dbi-spi.yaml | 30 + > drivers/gpu/drm/drm_mipi_dbi.c | 76 > +++--- > drivers/gpu/drm/tiny/panel-mipi-dbi.c | 55 +++- > include/drm/drm_mipi_dbi.h | 10 +++ > 4 files changed, 147 insertions(+), 24 deletions(-) > --- > base-commit: 0209df3b4731516fe77638bfc52ba2e9629c67cd > change-id: 20240405-panel-mipi-dbi-rgb666-4e033787d6c9 > > Best regards,
[PATCH v4 0/5] drm/tiny: panel-mipi-dbi: Support 18 bits per color RGB666
Hi, In this version I've fixed up a commit message that I had forgotten to write before sending and improved a struct member name. See version 1 of the patchset for the full cover letter. Signed-off-by: Noralf Trønnes --- Changes in v4: - Expand the commit message (Dmitry) - s/emulation_format/pixel_format/ (Dmitry) - Link to v3: https://lore.kernel.org/r/20240603-panel-mipi-dbi-rgb666-v3-0-59ed53ca7...@tronnes.org Changes in v3: - Added r-b's to patch 1 and 5 - Link to v2: https://lore.kernel.org/r/20240512-panel-mipi-dbi-rgb666-v2-0-49dd26632...@tronnes.org Changes in v2: - binding: Use 'default: r5g6b5' (Rob) - Link to v1: https://lore.kernel.org/r/20240507-panel-mipi-dbi-rgb666-v1-0-6799234af...@tronnes.org --- Noralf Trønnes (5): dt-bindings: display: panel: mipi-dbi-spi: Add a pixel format property drm/mipi-dbi: Remove mipi_dbi_machine_little_endian() drm/mipi-dbi: Make bits per word configurable for pixel transfers drm/mipi-dbi: Add support for DRM_FORMAT_RGB888 drm/tiny: panel-mipi-dbi: Support the pixel format property .../bindings/display/panel/panel-mipi-dbi-spi.yaml | 30 + drivers/gpu/drm/drm_mipi_dbi.c | 76 +++--- drivers/gpu/drm/tiny/panel-mipi-dbi.c | 55 +++- include/drm/drm_mipi_dbi.h | 10 +++ 4 files changed, 147 insertions(+), 24 deletions(-) --- base-commit: 0209df3b4731516fe77638bfc52ba2e9629c67cd change-id: 20240405-panel-mipi-dbi-rgb666-4e033787d6c9 Best regards, -- Noralf Trønnes
[PATCH v4 3/5] drm/mipi-dbi: Make bits per word configurable for pixel transfers
From: Noralf Trønnes
MIPI DCS write/set commands have 8 bit parameters except for the
write_memory commands where it depends on the pixel format.
drm_mipi_dbi does currently only support RGB565 which is 16-bit and it
has to make sure that the pixels enters the SPI bus in big endian format
since the MIPI DBI spec doesn't have support for little endian.
drm_mipi_dbi is optimized for DBI interface option 3 which means that the
16-bit bytes are swapped by the upper layer if the SPI bus does not
support 16 bits per word, signified by the swap_bytes member.
In order to support both 16-bit and 24-bit pixel transfers we need a way
to tell the DBI command layer the format of the buffer. Add a
write_memory_bpw member that the upper layer can use to tell how many
bits per word to use for the SPI transfer.
v4:
- Expand the commit message (Dmitry)
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 14 ++
include/drm/drm_mipi_dbi.h | 5 +
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index fa8aba6dc81c..77f8a828d6e0 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -1079,7 +1079,7 @@ static int mipi_dbi_typec1_command_read(struct mipi_dbi
*dbi, u8 *cmd,
static int mipi_dbi_typec1_command(struct mipi_dbi *dbi, u8 *cmd,
u8 *parameters, size_t num)
{
- unsigned int bpw = (*cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
+ unsigned int bpw = 8;
int ret;
if (mipi_dbi_command_is_read(dbi, *cmd))
@@ -1091,6 +1091,9 @@ static int mipi_dbi_typec1_command(struct mipi_dbi *dbi,
u8 *cmd,
if (ret || !num)
return ret;
+ if (*cmd == MIPI_DCS_WRITE_MEMORY_START)
+ bpw = dbi->write_memory_bpw;
+
return mipi_dbi_spi1_transfer(dbi, 1, parameters, num, bpw);
}
@@ -1184,8 +1187,8 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *dbi,
u8 *cmd,
if (ret || !num)
return ret;
- if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !dbi->swap_bytes)
- bpw = 16;
+ if (*cmd == MIPI_DCS_WRITE_MEMORY_START)
+ bpw = dbi->write_memory_bpw;
spi_bus_lock(spi->controller);
gpiod_set_value_cansleep(dbi->dc, 1);
@@ -1256,12 +1259,15 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct
mipi_dbi *dbi,
dbi->spi = spi;
dbi->read_commands = mipi_dbi_dcs_read_commands;
+ dbi->write_memory_bpw = 16;
if (dc) {
dbi->command = mipi_dbi_typec3_command;
dbi->dc = dc;
- if (!spi_is_bpw_supported(spi, 16))
+ if (!spi_is_bpw_supported(spi, 16)) {
+ dbi->write_memory_bpw = 8;
dbi->swap_bytes = true;
+ }
} else {
dbi->command = mipi_dbi_typec1_command;
dbi->tx_buf9_len = SZ_16K;
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index e8e0f8d39f3a..b36596efdcc3 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -56,6 +56,11 @@ struct mipi_dbi {
*/
struct spi_device *spi;
+ /**
+* @write_memory_bpw: Bits per word used on a
MIPI_DCS_WRITE_MEMORY_START transfer
+*/
+ unsigned int write_memory_bpw;
+
/**
* @dc: Optional D/C gpio.
*/
--
2.45.1
[PATCH v4 2/5] drm/mipi-dbi: Remove mipi_dbi_machine_little_endian()
From: Noralf Trønnes
mipi_dbi_machine_little_endian() should really have been called
mipi_dbi_framebuffer_little_endian() because that's the function it
performs. When I added support for these SPI displays I thought that the
framebuffers on big endian machines were also big endian, but I have
later learned that this is not the case. There's a bit in the fourcc code
that controls this: DRM_FORMAT_BIG_ENDIAN.
Just remove the function to avoid confusion. We can add big endian support
later should the need arise and we have hardware to test on.
Instead of just amending the docs, expand it to explain the endianness
handling.
Reviewed-by: Dmitry Baryshkov
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 35 +++
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index daac649aabdb..fa8aba6dc81c 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -824,15 +824,6 @@ u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi,
size_t len)
}
EXPORT_SYMBOL(mipi_dbi_spi_cmd_max_speed);
-static bool mipi_dbi_machine_little_endian(void)
-{
-#if defined(__LITTLE_ENDIAN)
- return true;
-#else
- return false;
-#endif
-}
-
/*
* MIPI DBI Type C Option 1
*
@@ -855,7 +846,7 @@ static int mipi_dbi_spi1e_transfer(struct mipi_dbi *dbi,
int dc,
const void *buf, size_t len,
unsigned int bpw)
{
- bool swap_bytes = (bpw == 16 && mipi_dbi_machine_little_endian());
+ bool swap_bytes = (bpw == 16);
size_t chunk, max_chunk = dbi->tx_buf9_len;
struct spi_device *spi = dbi->spi;
struct spi_transfer tr = {
@@ -1004,7 +995,7 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *dbi,
int dc,
size_t chunk = min(len, max_chunk);
unsigned int i;
- if (bpw == 16 && mipi_dbi_machine_little_endian()) {
+ if (bpw == 16) {
for (i = 0; i < (chunk * 2); i += 2) {
dst16[i] = *src16 >> 8;
dst16[i + 1] = *src16++ & 0xFF;
@@ -1218,11 +1209,23 @@ static int mipi_dbi_typec3_command(struct mipi_dbi
*dbi, u8 *cmd,
* If @dc is set, a Type C Option 3 interface is assumed, if not
* Type C Option 1.
*
- * If the SPI master driver doesn't support the necessary bits per word,
- * the following transformation is used:
+ * If the command is %MIPI_DCS_WRITE_MEMORY_START and the pixel format is
RGB565, endianness has
+ * to be taken into account. The MIPI DBI serial interface is big endian and
framebuffers are
+ * assumed stored in memory as little endian (%DRM_FORMAT_BIG_ENDIAN is not
supported).
*
- * - 9-bit: reorder buffer as 9x 8-bit words, padded with no-op command.
- * - 16-bit: if big endian send as 8-bit, if little endian swap bytes
+ * This is how endianness is handled:
+ *
+ * Option 1 (D/C as a bit): The buffer is sent on the wire byte by byte so the
16-bit buffer is
+ * byteswapped before transfer.
+ *
+ * Option 3 (D/C as a gpio): If the SPI controller supports 16 bits per word
the buffer can be
+ * sent as-is. If not the caller is responsible for
swapping the bytes
+ * before calling mipi_dbi_command_buf() and the
buffer is sent 8 bpw.
+ *
+ * This handling is optimised for %DRM_FORMAT_RGB565 framebuffers.
+ *
+ * If the interface is Option 1 and the SPI controller doesn't support 9 bits
per word,
+ * the buffer is sent as 9x 8-bit words, padded with MIPI DCS no-op commands
if necessary.
*
* Returns:
* Zero on success, negative error code on failure.
@@ -1257,7 +1260,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct
mipi_dbi *dbi,
if (dc) {
dbi->command = mipi_dbi_typec3_command;
dbi->dc = dc;
- if (mipi_dbi_machine_little_endian() &&
!spi_is_bpw_supported(spi, 16))
+ if (!spi_is_bpw_supported(spi, 16))
dbi->swap_bytes = true;
} else {
dbi->command = mipi_dbi_typec1_command;
--
2.45.1
[PATCH v4 4/5] drm/mipi-dbi: Add support for DRM_FORMAT_RGB888
From: Noralf Trønnes
DRM_FORMAT_RGB888 is 24 bits per pixel and it would be natural to send it
on the SPI bus using a 24 bits per word transfer. The problem with this
is that not all SPI controllers support 24 bpw.
Since DRM_FORMAT_RGB888 is stored in memory as little endian and the SPI
bus is big endian we use 8 bpw to always get the same pixel format on the
bus: b8g8r8.
The MIPI DCS specification lists the standard commands that can be sent
over the MIPI DBI interface. The set_address_mode (36h) command has one
bit in the parameter that controls RGB/BGR order. This means that the
controller can be configured to receive the pixel as BGR.
RGB888 is rarely supported on these controllers but RGB666 is very common.
All datasheets I have seen do at least support the pixel format option
where each color is sent as one byte and the 6 MSB's are used.
All this put together means that we can send each pixel as b8g8r8 and an
RGB666 capable controller sees this as b6x2g6x2r6x2.
v4:
- s/emulation_format/pixel_format/ (Dmitry)
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 29 +
include/drm/drm_mipi_dbi.h | 5 +
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 77f8a828d6e0..1661190c29a2 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -206,6 +206,7 @@ int mipi_dbi_buf_copy(void *dst, struct iosys_map *src,
struct drm_framebuffer *
struct drm_rect *clip, bool swap,
struct drm_format_conv_state *fmtcnv_state)
{
+ struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
struct iosys_map dst_map = IOSYS_MAP_INIT_VADDR(dst);
int ret;
@@ -222,8 +223,18 @@ int mipi_dbi_buf_copy(void *dst, struct iosys_map *src,
struct drm_framebuffer *
else
drm_fb_memcpy(_map, NULL, src, fb, clip);
break;
+ case DRM_FORMAT_RGB888:
+ drm_fb_memcpy(_map, NULL, src, fb, clip);
+ break;
case DRM_FORMAT_XRGB:
- drm_fb_xrgb_to_rgb565(_map, NULL, src, fb, clip,
fmtcnv_state, swap);
+ switch (dbidev->pixel_format) {
+ case DRM_FORMAT_RGB565:
+ drm_fb_xrgb_to_rgb565(_map, NULL, src, fb,
clip, fmtcnv_state, swap);
+ break;
+ case DRM_FORMAT_RGB888:
+ drm_fb_xrgb_to_rgb888(_map, NULL, src, fb,
clip, fmtcnv_state);
+ break;
+ }
break;
default:
drm_err_once(fb->dev, "Format is not supported: %p4cc\n",
@@ -260,9 +271,11 @@ static void mipi_dbi_fb_dirty(struct iosys_map *src,
struct drm_framebuffer *fb,
struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
unsigned int height = rect->y2 - rect->y1;
unsigned int width = rect->x2 - rect->x1;
+ const struct drm_format_info *dst_format;
struct mipi_dbi *dbi = >dbi;
bool swap = dbi->swap_bytes;
int ret = 0;
+ size_t len;
bool full;
void *tr;
@@ -283,8 +296,13 @@ static void mipi_dbi_fb_dirty(struct iosys_map *src,
struct drm_framebuffer *fb,
mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
rect->y2 - 1);
- ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr,
- width * height * 2);
+ if (fb->format->format == DRM_FORMAT_XRGB)
+ dst_format = drm_format_info(dbidev->pixel_format);
+ else
+ dst_format = fb->format;
+ len = drm_format_info_min_pitch(dst_format, 0, width) * height;
+
+ ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr, len);
err_msg:
if (ret)
drm_err_once(fb->dev, "Failed to update display %d\n", ret);
@@ -572,7 +590,7 @@ static const uint32_t mipi_dbi_formats[] = {
* has one fixed _display_mode which is rotated according to @rotation.
* This mode is used to set the mode config min/max width/height properties.
*
- * Use mipi_dbi_dev_init() if you don't need custom formats.
+ * Use mipi_dbi_dev_init() if you want native RGB565 and emulated XRGB
format.
*
* Note:
* Some of the helper functions expects RGB565 to be the default format and the
@@ -631,6 +649,9 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev
*dbidev,
drm->mode_config.min_height = dbidev->mode.vdisplay;
drm->mode_config.max_height = dbidev->mode.vdisplay;
dbidev->rotation = rotation;
+ dbidev->pixel_format = formats[0];
+ if (formats[0] == DRM_FORMAT_RGB888)
+
[PATCH v4 1/5] dt-bindings: display: panel: mipi-dbi-spi: Add a pixel format property
From: Noralf Trønnes The MIPI DBI 2.0 specification (2005) lists only two pixel formats for the Type C Interface (SPI) and that is 3-bits/pixel RGB111 with 2 options for bit layout. For Type A and B (parallel) the following formats are listed: RGB332, RGB444, RGB565, RGB666 and RGB888 (some have 2 options for the bit layout). Many MIPI DBI compatible controllers support all interface types on the same chip and often the manufacturers have chosen to provide support for the Type A/B interface pixel formats also on the Type C interface. Some chips provide many pixel formats with optional bit layouts over SPI, but the most common by far are RGB565 and RGB666. So even if the specification doesn't list these formats for the Type C interface, the industry has chosen to include them. The MIPI DCS specification lists the standard commands that can be sent over the MIPI DBI interface. The set_address_mode (36h) command has one bit in the parameter that controls RGB/BGR order: This bit controls the RGB data latching order transferred from the peripheral’s frame memory to the display device. This means that each supported RGB format also has a BGR variant. Based on this rationale document the following pixel formats describing the bit layout going over the wire: - RGB111 (option 1): x2r1g1b1r1g1b1 (2 pixels per byte) - BGR111 (option 1): x2b1g1r1b1g1r1 (2 pixels per byte) - RGB111 (option 2): x1r1g1b1x1r1g1b1 (2 pixels per byte) - BGR111 (option 2): x1b1g1r1x1b1g1r1 (2 pixels per byte) - RGB565: r5g6b5 (2 bytes) - BGR565: b5g6r5 (2 bytes) - RGB666: r6x2g6x2b6x2 (3 bytes) - BGR666: b6x2g6x2r6x2 (3 bytes) (x: don't care) v2: - Use 'default: r5g6b5' (Rob) Reviewed-by: Rob Herring (Arm) Signed-off-by: Noralf Trønnes --- .../bindings/display/panel/panel-mipi-dbi-spi.yaml | 30 ++ 1 file changed, 30 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml index e808215cb39e..8994549b4bff 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml @@ -50,6 +50,12 @@ description: | |Command or data | || + The standard defines one pixel format for type C: RGB111. The industry + however has decided to provide the type A/B interface pixel formats also on + the Type C interface and most common among these are RGB565 and RGB666. + The MIPI DCS command set_address_mode (36h) has one bit that controls RGB/BGR + order. This gives each supported RGB format a BGR variant. + The panel resolution is specified using the panel-timing node properties hactive (width) and vactive (height). The other mandatory panel-timing properties should be set to zero except clock-frequency which can be @@ -90,6 +96,28 @@ properties: spi-3wire: true + format: +description: > + Pixel format in bit order as going on the wire: +* `x2r1g1b1r1g1b1` - RGB111, 2 pixels per byte +* `x2b1g1r1b1g1r1` - BGR111, 2 pixels per byte +* `x1r1g1b1x1r1g1b1` - RGB111, 2 pixels per byte +* `x1b1g1r1x1b1g1r1` - BGR111, 2 pixels per byte +* `r5g6b5` - RGB565, 2 bytes +* `b5g6r5` - BGR565, 2 bytes +* `r6x2g6x2b6x2` - RGB666, 3 bytes +* `b6x2g6x2r6x2` - BGR666, 3 bytes +enum: + - x2r1g1b1r1g1b1 + - x2b1g1r1b1g1r1 + - x1r1g1b1x1r1g1b1 + - x1b1g1r1x1b1g1r1 + - r5g6b5 + - b5g6r5 + - r6x2g6x2b6x2 + - b6x2g6x2r6x2 +default: r5g6b5 + required: - compatible - reg @@ -116,6 +144,8 @@ examples: reset-gpios = < 25 GPIO_ACTIVE_HIGH>; write-only; +format = "r5g6b5"; + backlight = <>; width-mm = <35>; -- 2.45.1
[PATCH v4 5/5] drm/tiny: panel-mipi-dbi: Support the pixel format property
From: Noralf Trønnes
Add support for these pixel format property values:
- r5g6b5, RGB565
- b6x2g6x2r6x2, BGR666
BGR666 is presented to userspace as RGB888. The 2 LSB in each color
are discarded by the controller. The pixel is sent on the wire using
8 bits per word (little endian) so the controller sees it as BGR.
RGB565 is the default if the property is not present.
Reviewed-by: Neil Armstrong
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/tiny/panel-mipi-dbi.c | 55 ++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
index f80a141fcf36..f3aa2abce314 100644
--- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
+++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
@@ -26,6 +26,49 @@
#include
+struct panel_mipi_dbi_format {
+ const char *name;
+ u32 fourcc;
+ unsigned int bpp;
+};
+
+static const struct panel_mipi_dbi_format panel_mipi_dbi_formats[] = {
+ { "r5g6b5", DRM_FORMAT_RGB565, 16 },
+ { "b6x2g6x2r6x2", DRM_FORMAT_RGB888, 24 },
+};
+
+static int panel_mipi_dbi_get_format(struct device *dev, u32 *formats,
unsigned int *bpp)
+{
+ const char *format_name;
+ unsigned int i;
+ int ret;
+
+ formats[1] = DRM_FORMAT_XRGB;
+
+ ret = device_property_read_string(dev, "format", _name);
+ if (ret) {
+ /* Old Device Trees don't have this property */
+ formats[0] = DRM_FORMAT_RGB565;
+ *bpp = 16;
+ return 0;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(panel_mipi_dbi_formats); i++) {
+ const struct panel_mipi_dbi_format *format =
_mipi_dbi_formats[i];
+
+ if (strcmp(format_name, format->name))
+ continue;
+
+ formats[0] = format->fourcc;
+ *bpp = format->bpp;
+ return 0;
+ }
+
+ dev_err(dev, "Pixel format is not supported: '%s'\n", format_name);
+
+ return -EINVAL;
+}
+
static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D',
'B', 'I',
0, 0, 0, 0, 0, 0, 0 };
@@ -276,6 +319,9 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
struct drm_device *drm;
struct mipi_dbi *dbi;
struct gpio_desc *dc;
+ unsigned int bpp;
+ size_t buf_size;
+ u32 formats[2];
int ret;
dbidev = devm_drm_dev_alloc(dev, _mipi_dbi_driver, struct
mipi_dbi_dev, drm);
@@ -323,7 +369,14 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
if (IS_ERR(dbidev->driver_private))
return PTR_ERR(dbidev->driver_private);
- ret = mipi_dbi_dev_init(dbidev, _mipi_dbi_pipe_funcs, , 0);
+ ret = panel_mipi_dbi_get_format(dev, formats, );
+ if (ret)
+ return ret;
+
+ buf_size = DIV_ROUND_UP(mode.hdisplay * mode.vdisplay * bpp, 8);
+ ret = mipi_dbi_dev_init_with_formats(dbidev, _mipi_dbi_pipe_funcs,
+formats, ARRAY_SIZE(formats),
+, 0, buf_size);
if (ret)
return ret;
--
2.45.1
[PATCH v3 4/5] drm/mipi-dbi: Add support for DRM_FORMAT_RGB888
From: Noralf Trønnes
DRM_FORMAT_RGB888 is 24 bits per pixel and it would be natural to send it
on the SPI bus using a 24 bits per word transfer. The problem with this
is that not all SPI controllers support 24 bpw.
Since DRM_FORMAT_RGB888 is stored in memory as little endian and the SPI
bus is big endian we use 8 bpw to always get the same pixel format on the
bus: b8g8r8.
The MIPI DCS specification lists the standard commands that can be sent
over the MIPI DBI interface. The set_address_mode (36h) command has one
bit in the parameter that controls RGB/BGR order. This means that the
controller can be configured to receive the pixel as BGR.
RGB888 is rarely supported on these controllers but RGB666 is very common.
All datasheets I have seen do at least support the pixel format option
where each color is sent as one byte and the 6 MSB's are used.
All this put together means that we can send each pixel as b8g8r8 and an
RGB666 capable controller sees this as b6x2g6x2r6x2.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 29 +
include/drm/drm_mipi_dbi.h | 5 +
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 77f8a828d6e0..eb330676857c 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -206,6 +206,7 @@ int mipi_dbi_buf_copy(void *dst, struct iosys_map *src,
struct drm_framebuffer *
struct drm_rect *clip, bool swap,
struct drm_format_conv_state *fmtcnv_state)
{
+ struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
struct iosys_map dst_map = IOSYS_MAP_INIT_VADDR(dst);
int ret;
@@ -222,8 +223,18 @@ int mipi_dbi_buf_copy(void *dst, struct iosys_map *src,
struct drm_framebuffer *
else
drm_fb_memcpy(_map, NULL, src, fb, clip);
break;
+ case DRM_FORMAT_RGB888:
+ drm_fb_memcpy(_map, NULL, src, fb, clip);
+ break;
case DRM_FORMAT_XRGB:
- drm_fb_xrgb_to_rgb565(_map, NULL, src, fb, clip,
fmtcnv_state, swap);
+ switch (dbidev->emulation_format) {
+ case DRM_FORMAT_RGB565:
+ drm_fb_xrgb_to_rgb565(_map, NULL, src, fb,
clip, fmtcnv_state, swap);
+ break;
+ case DRM_FORMAT_RGB888:
+ drm_fb_xrgb_to_rgb888(_map, NULL, src, fb,
clip, fmtcnv_state);
+ break;
+ }
break;
default:
drm_err_once(fb->dev, "Format is not supported: %p4cc\n",
@@ -260,9 +271,11 @@ static void mipi_dbi_fb_dirty(struct iosys_map *src,
struct drm_framebuffer *fb,
struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
unsigned int height = rect->y2 - rect->y1;
unsigned int width = rect->x2 - rect->x1;
+ const struct drm_format_info *dst_format;
struct mipi_dbi *dbi = >dbi;
bool swap = dbi->swap_bytes;
int ret = 0;
+ size_t len;
bool full;
void *tr;
@@ -283,8 +296,13 @@ static void mipi_dbi_fb_dirty(struct iosys_map *src,
struct drm_framebuffer *fb,
mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
rect->y2 - 1);
- ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr,
- width * height * 2);
+ if (fb->format->format == DRM_FORMAT_XRGB)
+ dst_format = drm_format_info(dbidev->emulation_format);
+ else
+ dst_format = fb->format;
+ len = drm_format_info_min_pitch(dst_format, 0, width) * height;
+
+ ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr, len);
err_msg:
if (ret)
drm_err_once(fb->dev, "Failed to update display %d\n", ret);
@@ -572,7 +590,7 @@ static const uint32_t mipi_dbi_formats[] = {
* has one fixed _display_mode which is rotated according to @rotation.
* This mode is used to set the mode config min/max width/height properties.
*
- * Use mipi_dbi_dev_init() if you don't need custom formats.
+ * Use mipi_dbi_dev_init() if you want native RGB565 and emulated XRGB
format.
*
* Note:
* Some of the helper functions expects RGB565 to be the default format and the
@@ -631,6 +649,9 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev
*dbidev,
drm->mode_config.min_height = dbidev->mode.vdisplay;
drm->mode_config.max_height = dbidev->mode.vdisplay;
dbidev->rotation = rotation;
+ dbidev->emulation_format = formats[0];
+ if (formats[0] == DRM_FORMAT_RGB888)
+ dbidev->dbi.writ
[PATCH v3 3/5] drm/mipi-dbi: Make bits per word configurable for pixel transfers
From: Noralf Trønnes
This prepares for supporting other pixel formats than RGB565.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 14 ++
include/drm/drm_mipi_dbi.h | 5 +
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index fa8aba6dc81c..77f8a828d6e0 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -1079,7 +1079,7 @@ static int mipi_dbi_typec1_command_read(struct mipi_dbi
*dbi, u8 *cmd,
static int mipi_dbi_typec1_command(struct mipi_dbi *dbi, u8 *cmd,
u8 *parameters, size_t num)
{
- unsigned int bpw = (*cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
+ unsigned int bpw = 8;
int ret;
if (mipi_dbi_command_is_read(dbi, *cmd))
@@ -1091,6 +1091,9 @@ static int mipi_dbi_typec1_command(struct mipi_dbi *dbi,
u8 *cmd,
if (ret || !num)
return ret;
+ if (*cmd == MIPI_DCS_WRITE_MEMORY_START)
+ bpw = dbi->write_memory_bpw;
+
return mipi_dbi_spi1_transfer(dbi, 1, parameters, num, bpw);
}
@@ -1184,8 +1187,8 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *dbi,
u8 *cmd,
if (ret || !num)
return ret;
- if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !dbi->swap_bytes)
- bpw = 16;
+ if (*cmd == MIPI_DCS_WRITE_MEMORY_START)
+ bpw = dbi->write_memory_bpw;
spi_bus_lock(spi->controller);
gpiod_set_value_cansleep(dbi->dc, 1);
@@ -1256,12 +1259,15 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct
mipi_dbi *dbi,
dbi->spi = spi;
dbi->read_commands = mipi_dbi_dcs_read_commands;
+ dbi->write_memory_bpw = 16;
if (dc) {
dbi->command = mipi_dbi_typec3_command;
dbi->dc = dc;
- if (!spi_is_bpw_supported(spi, 16))
+ if (!spi_is_bpw_supported(spi, 16)) {
+ dbi->write_memory_bpw = 8;
dbi->swap_bytes = true;
+ }
} else {
dbi->command = mipi_dbi_typec1_command;
dbi->tx_buf9_len = SZ_16K;
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index e8e0f8d39f3a..b36596efdcc3 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -56,6 +56,11 @@ struct mipi_dbi {
*/
struct spi_device *spi;
+ /**
+* @write_memory_bpw: Bits per word used on a
MIPI_DCS_WRITE_MEMORY_START transfer
+*/
+ unsigned int write_memory_bpw;
+
/**
* @dc: Optional D/C gpio.
*/
--
2.45.1
[PATCH v3 1/5] dt-bindings: display: panel: mipi-dbi-spi: Add a pixel format property
From: Noralf Trønnes The MIPI DBI 2.0 specification (2005) lists only two pixel formats for the Type C Interface (SPI) and that is 3-bits/pixel RGB111 with 2 options for bit layout. For Type A and B (parallel) the following formats are listed: RGB332, RGB444, RGB565, RGB666 and RGB888 (some have 2 options for the bit layout). Many MIPI DBI compatible controllers support all interface types on the same chip and often the manufacturers have chosen to provide support for the Type A/B interface pixel formats also on the Type C interface. Some chips provide many pixel formats with optional bit layouts over SPI, but the most common by far are RGB565 and RGB666. So even if the specification doesn't list these formats for the Type C interface, the industry has chosen to include them. The MIPI DCS specification lists the standard commands that can be sent over the MIPI DBI interface. The set_address_mode (36h) command has one bit in the parameter that controls RGB/BGR order: This bit controls the RGB data latching order transferred from the peripheral’s frame memory to the display device. This means that each supported RGB format also has a BGR variant. Based on this rationale document the following pixel formats describing the bit layout going over the wire: - RGB111 (option 1): x2r1g1b1r1g1b1 (2 pixels per byte) - BGR111 (option 1): x2b1g1r1b1g1r1 (2 pixels per byte) - RGB111 (option 2): x1r1g1b1x1r1g1b1 (2 pixels per byte) - BGR111 (option 2): x1b1g1r1x1b1g1r1 (2 pixels per byte) - RGB565: r5g6b5 (2 bytes) - BGR565: b5g6r5 (2 bytes) - RGB666: r6x2g6x2b6x2 (3 bytes) - BGR666: b6x2g6x2r6x2 (3 bytes) (x: don't care) v2: - Use 'default: r5g6b5' (Rob) Reviewed-by: Rob Herring (Arm) Signed-off-by: Noralf Trønnes --- .../bindings/display/panel/panel-mipi-dbi-spi.yaml | 30 ++ 1 file changed, 30 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml index e808215cb39e..8994549b4bff 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml @@ -50,6 +50,12 @@ description: | |Command or data | || + The standard defines one pixel format for type C: RGB111. The industry + however has decided to provide the type A/B interface pixel formats also on + the Type C interface and most common among these are RGB565 and RGB666. + The MIPI DCS command set_address_mode (36h) has one bit that controls RGB/BGR + order. This gives each supported RGB format a BGR variant. + The panel resolution is specified using the panel-timing node properties hactive (width) and vactive (height). The other mandatory panel-timing properties should be set to zero except clock-frequency which can be @@ -90,6 +96,28 @@ properties: spi-3wire: true + format: +description: > + Pixel format in bit order as going on the wire: +* `x2r1g1b1r1g1b1` - RGB111, 2 pixels per byte +* `x2b1g1r1b1g1r1` - BGR111, 2 pixels per byte +* `x1r1g1b1x1r1g1b1` - RGB111, 2 pixels per byte +* `x1b1g1r1x1b1g1r1` - BGR111, 2 pixels per byte +* `r5g6b5` - RGB565, 2 bytes +* `b5g6r5` - BGR565, 2 bytes +* `r6x2g6x2b6x2` - RGB666, 3 bytes +* `b6x2g6x2r6x2` - BGR666, 3 bytes +enum: + - x2r1g1b1r1g1b1 + - x2b1g1r1b1g1r1 + - x1r1g1b1x1r1g1b1 + - x1b1g1r1x1b1g1r1 + - r5g6b5 + - b5g6r5 + - r6x2g6x2b6x2 + - b6x2g6x2r6x2 +default: r5g6b5 + required: - compatible - reg @@ -116,6 +144,8 @@ examples: reset-gpios = < 25 GPIO_ACTIVE_HIGH>; write-only; +format = "r5g6b5"; + backlight = <>; width-mm = <35>; -- 2.45.1
[PATCH v3 2/5] drm/mipi-dbi: Remove mipi_dbi_machine_little_endian()
From: Noralf Trønnes
mipi_dbi_machine_little_endian() should really have been called
mipi_dbi_framebuffer_little_endian() because that's the function it
performs. When I added support for these SPI displays I thought that the
framebuffers on big endian machines were also big endian, but I have
later learned that this is not the case. There's a bit in the fourcc code
that controls this: DRM_FORMAT_BIG_ENDIAN.
Just remove the function to avoid confusion. We can add big endian support
later should the need arise and we have hardware to test on.
Instead of just amending the docs, expand it to explain the endianness
handling.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 35 +++
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index daac649aabdb..fa8aba6dc81c 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -824,15 +824,6 @@ u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi,
size_t len)
}
EXPORT_SYMBOL(mipi_dbi_spi_cmd_max_speed);
-static bool mipi_dbi_machine_little_endian(void)
-{
-#if defined(__LITTLE_ENDIAN)
- return true;
-#else
- return false;
-#endif
-}
-
/*
* MIPI DBI Type C Option 1
*
@@ -855,7 +846,7 @@ static int mipi_dbi_spi1e_transfer(struct mipi_dbi *dbi,
int dc,
const void *buf, size_t len,
unsigned int bpw)
{
- bool swap_bytes = (bpw == 16 && mipi_dbi_machine_little_endian());
+ bool swap_bytes = (bpw == 16);
size_t chunk, max_chunk = dbi->tx_buf9_len;
struct spi_device *spi = dbi->spi;
struct spi_transfer tr = {
@@ -1004,7 +995,7 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *dbi,
int dc,
size_t chunk = min(len, max_chunk);
unsigned int i;
- if (bpw == 16 && mipi_dbi_machine_little_endian()) {
+ if (bpw == 16) {
for (i = 0; i < (chunk * 2); i += 2) {
dst16[i] = *src16 >> 8;
dst16[i + 1] = *src16++ & 0xFF;
@@ -1218,11 +1209,23 @@ static int mipi_dbi_typec3_command(struct mipi_dbi
*dbi, u8 *cmd,
* If @dc is set, a Type C Option 3 interface is assumed, if not
* Type C Option 1.
*
- * If the SPI master driver doesn't support the necessary bits per word,
- * the following transformation is used:
+ * If the command is %MIPI_DCS_WRITE_MEMORY_START and the pixel format is
RGB565, endianness has
+ * to be taken into account. The MIPI DBI serial interface is big endian and
framebuffers are
+ * assumed stored in memory as little endian (%DRM_FORMAT_BIG_ENDIAN is not
supported).
*
- * - 9-bit: reorder buffer as 9x 8-bit words, padded with no-op command.
- * - 16-bit: if big endian send as 8-bit, if little endian swap bytes
+ * This is how endianness is handled:
+ *
+ * Option 1 (D/C as a bit): The buffer is sent on the wire byte by byte so the
16-bit buffer is
+ * byteswapped before transfer.
+ *
+ * Option 3 (D/C as a gpio): If the SPI controller supports 16 bits per word
the buffer can be
+ * sent as-is. If not the caller is responsible for
swapping the bytes
+ * before calling mipi_dbi_command_buf() and the
buffer is sent 8 bpw.
+ *
+ * This handling is optimised for %DRM_FORMAT_RGB565 framebuffers.
+ *
+ * If the interface is Option 1 and the SPI controller doesn't support 9 bits
per word,
+ * the buffer is sent as 9x 8-bit words, padded with MIPI DCS no-op commands
if necessary.
*
* Returns:
* Zero on success, negative error code on failure.
@@ -1257,7 +1260,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct
mipi_dbi *dbi,
if (dc) {
dbi->command = mipi_dbi_typec3_command;
dbi->dc = dc;
- if (mipi_dbi_machine_little_endian() &&
!spi_is_bpw_supported(spi, 16))
+ if (!spi_is_bpw_supported(spi, 16))
dbi->swap_bytes = true;
} else {
dbi->command = mipi_dbi_typec1_command;
--
2.45.1
[PATCH v3 0/5] drm/tiny: panel-mipi-dbi: Support 18 bits per color RGB666
Hi, The binding and driver patches have been reviewed and I appreciate if someone can take a look at the drm_mipi_dbi patches. The first patch removes some erroneous big endian code and the 2 other makes it possible to configure bits per word for the SPI pixel transfer. It's currently hardcoded for RGB565. See version 1 of the patchset for the full cover letter. Signed-off-by: Noralf Trønnes --- Changes in v3: - Added r-b's to patch 1 and 5 - Link to v2: https://lore.kernel.org/r/20240512-panel-mipi-dbi-rgb666-v2-0-49dd26632...@tronnes.org Changes in v2: - binding: Use 'default: r5g6b5' (Rob) - Link to v1: https://lore.kernel.org/r/20240507-panel-mipi-dbi-rgb666-v1-0-6799234af...@tronnes.org --- Noralf Trønnes (5): dt-bindings: display: panel: mipi-dbi-spi: Add a pixel format property drm/mipi-dbi: Remove mipi_dbi_machine_little_endian() drm/mipi-dbi: Make bits per word configurable for pixel transfers drm/mipi-dbi: Add support for DRM_FORMAT_RGB888 drm/tiny: panel-mipi-dbi: Support the pixel format property .../bindings/display/panel/panel-mipi-dbi-spi.yaml | 30 + drivers/gpu/drm/drm_mipi_dbi.c | 76 +++--- drivers/gpu/drm/tiny/panel-mipi-dbi.c | 55 +++- include/drm/drm_mipi_dbi.h | 10 +++ 4 files changed, 147 insertions(+), 24 deletions(-) --- base-commit: 0209df3b4731516fe77638bfc52ba2e9629c67cd change-id: 20240405-panel-mipi-dbi-rgb666-4e033787d6c9 Best regards, -- Noralf Trønnes
[PATCH v3 5/5] drm/tiny: panel-mipi-dbi: Support the pixel format property
From: Noralf Trønnes
Add support for these pixel format property values:
- r5g6b5, RGB565
- b6x2g6x2r6x2, BGR666
BGR666 is presented to userspace as RGB888. The 2 LSB in each color
are discarded by the controller. The pixel is sent on the wire using
8 bits per word (little endian) so the controller sees it as BGR.
RGB565 is the default if the property is not present.
Reviewed-by: Neil Armstrong
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/tiny/panel-mipi-dbi.c | 55 ++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
index f80a141fcf36..f3aa2abce314 100644
--- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
+++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
@@ -26,6 +26,49 @@
#include
+struct panel_mipi_dbi_format {
+ const char *name;
+ u32 fourcc;
+ unsigned int bpp;
+};
+
+static const struct panel_mipi_dbi_format panel_mipi_dbi_formats[] = {
+ { "r5g6b5", DRM_FORMAT_RGB565, 16 },
+ { "b6x2g6x2r6x2", DRM_FORMAT_RGB888, 24 },
+};
+
+static int panel_mipi_dbi_get_format(struct device *dev, u32 *formats,
unsigned int *bpp)
+{
+ const char *format_name;
+ unsigned int i;
+ int ret;
+
+ formats[1] = DRM_FORMAT_XRGB;
+
+ ret = device_property_read_string(dev, "format", _name);
+ if (ret) {
+ /* Old Device Trees don't have this property */
+ formats[0] = DRM_FORMAT_RGB565;
+ *bpp = 16;
+ return 0;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(panel_mipi_dbi_formats); i++) {
+ const struct panel_mipi_dbi_format *format =
_mipi_dbi_formats[i];
+
+ if (strcmp(format_name, format->name))
+ continue;
+
+ formats[0] = format->fourcc;
+ *bpp = format->bpp;
+ return 0;
+ }
+
+ dev_err(dev, "Pixel format is not supported: '%s'\n", format_name);
+
+ return -EINVAL;
+}
+
static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D',
'B', 'I',
0, 0, 0, 0, 0, 0, 0 };
@@ -276,6 +319,9 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
struct drm_device *drm;
struct mipi_dbi *dbi;
struct gpio_desc *dc;
+ unsigned int bpp;
+ size_t buf_size;
+ u32 formats[2];
int ret;
dbidev = devm_drm_dev_alloc(dev, _mipi_dbi_driver, struct
mipi_dbi_dev, drm);
@@ -323,7 +369,14 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
if (IS_ERR(dbidev->driver_private))
return PTR_ERR(dbidev->driver_private);
- ret = mipi_dbi_dev_init(dbidev, _mipi_dbi_pipe_funcs, , 0);
+ ret = panel_mipi_dbi_get_format(dev, formats, );
+ if (ret)
+ return ret;
+
+ buf_size = DIV_ROUND_UP(mode.hdisplay * mode.vdisplay * bpp, 8);
+ ret = mipi_dbi_dev_init_with_formats(dbidev, _mipi_dbi_pipe_funcs,
+formats, ARRAY_SIZE(formats),
+, 0, buf_size);
if (ret)
return ret;
--
2.45.1
[PATCH v2 3/5] drm/mipi-dbi: Make bits per word configurable for pixel transfers
From: Noralf Trønnes
This prepares for supporting other pixel formats than RGB565.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 14 ++
include/drm/drm_mipi_dbi.h | 5 +
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index fa8aba6dc81c..77f8a828d6e0 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -1079,7 +1079,7 @@ static int mipi_dbi_typec1_command_read(struct mipi_dbi
*dbi, u8 *cmd,
static int mipi_dbi_typec1_command(struct mipi_dbi *dbi, u8 *cmd,
u8 *parameters, size_t num)
{
- unsigned int bpw = (*cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
+ unsigned int bpw = 8;
int ret;
if (mipi_dbi_command_is_read(dbi, *cmd))
@@ -1091,6 +1091,9 @@ static int mipi_dbi_typec1_command(struct mipi_dbi *dbi,
u8 *cmd,
if (ret || !num)
return ret;
+ if (*cmd == MIPI_DCS_WRITE_MEMORY_START)
+ bpw = dbi->write_memory_bpw;
+
return mipi_dbi_spi1_transfer(dbi, 1, parameters, num, bpw);
}
@@ -1184,8 +1187,8 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *dbi,
u8 *cmd,
if (ret || !num)
return ret;
- if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !dbi->swap_bytes)
- bpw = 16;
+ if (*cmd == MIPI_DCS_WRITE_MEMORY_START)
+ bpw = dbi->write_memory_bpw;
spi_bus_lock(spi->controller);
gpiod_set_value_cansleep(dbi->dc, 1);
@@ -1256,12 +1259,15 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct
mipi_dbi *dbi,
dbi->spi = spi;
dbi->read_commands = mipi_dbi_dcs_read_commands;
+ dbi->write_memory_bpw = 16;
if (dc) {
dbi->command = mipi_dbi_typec3_command;
dbi->dc = dc;
- if (!spi_is_bpw_supported(spi, 16))
+ if (!spi_is_bpw_supported(spi, 16)) {
+ dbi->write_memory_bpw = 8;
dbi->swap_bytes = true;
+ }
} else {
dbi->command = mipi_dbi_typec1_command;
dbi->tx_buf9_len = SZ_16K;
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index e8e0f8d39f3a..b36596efdcc3 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -56,6 +56,11 @@ struct mipi_dbi {
*/
struct spi_device *spi;
+ /**
+* @write_memory_bpw: Bits per word used on a
MIPI_DCS_WRITE_MEMORY_START transfer
+*/
+ unsigned int write_memory_bpw;
+
/**
* @dc: Optional D/C gpio.
*/
--
2.45.0
[PATCH v2 5/5] drm/tiny: panel-mipi-dbi: Support the pixel format property
From: Noralf Trønnes
Add support for these pixel format property values:
- r5g6b5, RGB565
- b6x2g6x2r6x2, BGR666
BGR666 is presented to userspace as RGB888. The 2 LSB in each color
are discarded by the controller. The pixel is sent on the wire using
8 bits per word (little endian) so the controller sees it as BGR.
RGB565 is the default if the property is not present.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/tiny/panel-mipi-dbi.c | 55 ++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
index f80a141fcf36..f3aa2abce314 100644
--- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
+++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
@@ -26,6 +26,49 @@
#include
+struct panel_mipi_dbi_format {
+ const char *name;
+ u32 fourcc;
+ unsigned int bpp;
+};
+
+static const struct panel_mipi_dbi_format panel_mipi_dbi_formats[] = {
+ { "r5g6b5", DRM_FORMAT_RGB565, 16 },
+ { "b6x2g6x2r6x2", DRM_FORMAT_RGB888, 24 },
+};
+
+static int panel_mipi_dbi_get_format(struct device *dev, u32 *formats,
unsigned int *bpp)
+{
+ const char *format_name;
+ unsigned int i;
+ int ret;
+
+ formats[1] = DRM_FORMAT_XRGB;
+
+ ret = device_property_read_string(dev, "format", _name);
+ if (ret) {
+ /* Old Device Trees don't have this property */
+ formats[0] = DRM_FORMAT_RGB565;
+ *bpp = 16;
+ return 0;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(panel_mipi_dbi_formats); i++) {
+ const struct panel_mipi_dbi_format *format =
_mipi_dbi_formats[i];
+
+ if (strcmp(format_name, format->name))
+ continue;
+
+ formats[0] = format->fourcc;
+ *bpp = format->bpp;
+ return 0;
+ }
+
+ dev_err(dev, "Pixel format is not supported: '%s'\n", format_name);
+
+ return -EINVAL;
+}
+
static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D',
'B', 'I',
0, 0, 0, 0, 0, 0, 0 };
@@ -276,6 +319,9 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
struct drm_device *drm;
struct mipi_dbi *dbi;
struct gpio_desc *dc;
+ unsigned int bpp;
+ size_t buf_size;
+ u32 formats[2];
int ret;
dbidev = devm_drm_dev_alloc(dev, _mipi_dbi_driver, struct
mipi_dbi_dev, drm);
@@ -323,7 +369,14 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
if (IS_ERR(dbidev->driver_private))
return PTR_ERR(dbidev->driver_private);
- ret = mipi_dbi_dev_init(dbidev, _mipi_dbi_pipe_funcs, , 0);
+ ret = panel_mipi_dbi_get_format(dev, formats, );
+ if (ret)
+ return ret;
+
+ buf_size = DIV_ROUND_UP(mode.hdisplay * mode.vdisplay * bpp, 8);
+ ret = mipi_dbi_dev_init_with_formats(dbidev, _mipi_dbi_pipe_funcs,
+formats, ARRAY_SIZE(formats),
+, 0, buf_size);
if (ret)
return ret;
--
2.45.0
[PATCH v2 4/5] drm/mipi-dbi: Add support for DRM_FORMAT_RGB888
From: Noralf Trønnes
DRM_FORMAT_RGB888 is 24 bits per pixel and it would be natural to send it
on the SPI bus using a 24 bits per word transfer. The problem with this
is that not all SPI controllers support 24 bpw.
Since DRM_FORMAT_RGB888 is stored in memory as little endian and the SPI
bus is big endian we use 8 bpw to always get the same pixel format on the
bus: b8g8r8.
The MIPI DCS specification lists the standard commands that can be sent
over the MIPI DBI interface. The set_address_mode (36h) command has one
bit in the parameter that controls RGB/BGR order. This means that the
controller can be configured to receive the pixel as BGR.
RGB888 is rarely supported on these controllers but RGB666 is very common.
All datasheets I have seen do at least support the pixel format option
where each color is sent as one byte and the 6 MSB's are used.
All this put together means that we can send each pixel as b8g8r8 and an
RGB666 capable controller sees this as b6x2g6x2r6x2.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 29 +
include/drm/drm_mipi_dbi.h | 5 +
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 77f8a828d6e0..eb330676857c 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -206,6 +206,7 @@ int mipi_dbi_buf_copy(void *dst, struct iosys_map *src,
struct drm_framebuffer *
struct drm_rect *clip, bool swap,
struct drm_format_conv_state *fmtcnv_state)
{
+ struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
struct iosys_map dst_map = IOSYS_MAP_INIT_VADDR(dst);
int ret;
@@ -222,8 +223,18 @@ int mipi_dbi_buf_copy(void *dst, struct iosys_map *src,
struct drm_framebuffer *
else
drm_fb_memcpy(_map, NULL, src, fb, clip);
break;
+ case DRM_FORMAT_RGB888:
+ drm_fb_memcpy(_map, NULL, src, fb, clip);
+ break;
case DRM_FORMAT_XRGB:
- drm_fb_xrgb_to_rgb565(_map, NULL, src, fb, clip,
fmtcnv_state, swap);
+ switch (dbidev->emulation_format) {
+ case DRM_FORMAT_RGB565:
+ drm_fb_xrgb_to_rgb565(_map, NULL, src, fb,
clip, fmtcnv_state, swap);
+ break;
+ case DRM_FORMAT_RGB888:
+ drm_fb_xrgb_to_rgb888(_map, NULL, src, fb,
clip, fmtcnv_state);
+ break;
+ }
break;
default:
drm_err_once(fb->dev, "Format is not supported: %p4cc\n",
@@ -260,9 +271,11 @@ static void mipi_dbi_fb_dirty(struct iosys_map *src,
struct drm_framebuffer *fb,
struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
unsigned int height = rect->y2 - rect->y1;
unsigned int width = rect->x2 - rect->x1;
+ const struct drm_format_info *dst_format;
struct mipi_dbi *dbi = >dbi;
bool swap = dbi->swap_bytes;
int ret = 0;
+ size_t len;
bool full;
void *tr;
@@ -283,8 +296,13 @@ static void mipi_dbi_fb_dirty(struct iosys_map *src,
struct drm_framebuffer *fb,
mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
rect->y2 - 1);
- ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr,
- width * height * 2);
+ if (fb->format->format == DRM_FORMAT_XRGB)
+ dst_format = drm_format_info(dbidev->emulation_format);
+ else
+ dst_format = fb->format;
+ len = drm_format_info_min_pitch(dst_format, 0, width) * height;
+
+ ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr, len);
err_msg:
if (ret)
drm_err_once(fb->dev, "Failed to update display %d\n", ret);
@@ -572,7 +590,7 @@ static const uint32_t mipi_dbi_formats[] = {
* has one fixed _display_mode which is rotated according to @rotation.
* This mode is used to set the mode config min/max width/height properties.
*
- * Use mipi_dbi_dev_init() if you don't need custom formats.
+ * Use mipi_dbi_dev_init() if you want native RGB565 and emulated XRGB
format.
*
* Note:
* Some of the helper functions expects RGB565 to be the default format and the
@@ -631,6 +649,9 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev
*dbidev,
drm->mode_config.min_height = dbidev->mode.vdisplay;
drm->mode_config.max_height = dbidev->mode.vdisplay;
dbidev->rotation = rotation;
+ dbidev->emulation_format = formats[0];
+ if (formats[0] == DRM_FORMAT_RGB888)
+ dbidev->dbi.writ
[PATCH v2 1/5] dt-bindings: display: panel: mipi-dbi-spi: Add a pixel format property
From: Noralf Trønnes The MIPI DBI 2.0 specification (2005) lists only two pixel formats for the Type C Interface (SPI) and that is 3-bits/pixel RGB111 with 2 options for bit layout. For Type A and B (parallel) the following formats are listed: RGB332, RGB444, RGB565, RGB666 and RGB888 (some have 2 options for the bit layout). Many MIPI DBI compatible controllers support all interface types on the same chip and often the manufacturers have chosen to provide support for the Type A/B interface pixel formats also on the Type C interface. Some chips provide many pixel formats with optional bit layouts over SPI, but the most common by far are RGB565 and RGB666. So even if the specification doesn't list these formats for the Type C interface, the industry has chosen to include them. The MIPI DCS specification lists the standard commands that can be sent over the MIPI DBI interface. The set_address_mode (36h) command has one bit in the parameter that controls RGB/BGR order: This bit controls the RGB data latching order transferred from the peripheral’s frame memory to the display device. This means that each supported RGB format also has a BGR variant. Based on this rationale document the following pixel formats describing the bit layout going over the wire: - RGB111 (option 1): x2r1g1b1r1g1b1 (2 pixels per byte) - BGR111 (option 1): x2b1g1r1b1g1r1 (2 pixels per byte) - RGB111 (option 2): x1r1g1b1x1r1g1b1 (2 pixels per byte) - BGR111 (option 2): x1b1g1r1x1b1g1r1 (2 pixels per byte) - RGB565: r5g6b5 (2 bytes) - BGR565: b5g6r5 (2 bytes) - RGB666: r6x2g6x2b6x2 (3 bytes) - BGR666: b6x2g6x2r6x2 (3 bytes) (x: don't care) v2: - Use 'default: r5g6b5' (Rob) Signed-off-by: Noralf Trønnes --- .../bindings/display/panel/panel-mipi-dbi-spi.yaml | 30 ++ 1 file changed, 30 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml index e808215cb39e..8994549b4bff 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml @@ -50,6 +50,12 @@ description: | |Command or data | || + The standard defines one pixel format for type C: RGB111. The industry + however has decided to provide the type A/B interface pixel formats also on + the Type C interface and most common among these are RGB565 and RGB666. + The MIPI DCS command set_address_mode (36h) has one bit that controls RGB/BGR + order. This gives each supported RGB format a BGR variant. + The panel resolution is specified using the panel-timing node properties hactive (width) and vactive (height). The other mandatory panel-timing properties should be set to zero except clock-frequency which can be @@ -90,6 +96,28 @@ properties: spi-3wire: true + format: +description: > + Pixel format in bit order as going on the wire: +* `x2r1g1b1r1g1b1` - RGB111, 2 pixels per byte +* `x2b1g1r1b1g1r1` - BGR111, 2 pixels per byte +* `x1r1g1b1x1r1g1b1` - RGB111, 2 pixels per byte +* `x1b1g1r1x1b1g1r1` - BGR111, 2 pixels per byte +* `r5g6b5` - RGB565, 2 bytes +* `b5g6r5` - BGR565, 2 bytes +* `r6x2g6x2b6x2` - RGB666, 3 bytes +* `b6x2g6x2r6x2` - BGR666, 3 bytes +enum: + - x2r1g1b1r1g1b1 + - x2b1g1r1b1g1r1 + - x1r1g1b1x1r1g1b1 + - x1b1g1r1x1b1g1r1 + - r5g6b5 + - b5g6r5 + - r6x2g6x2b6x2 + - b6x2g6x2r6x2 +default: r5g6b5 + required: - compatible - reg @@ -116,6 +144,8 @@ examples: reset-gpios = < 25 GPIO_ACTIVE_HIGH>; write-only; +format = "r5g6b5"; + backlight = <>; width-mm = <35>; -- 2.45.0
[PATCH v2 2/5] drm/mipi-dbi: Remove mipi_dbi_machine_little_endian()
From: Noralf Trønnes
mipi_dbi_machine_little_endian() should really have been called
mipi_dbi_framebuffer_little_endian() because that's the function it
performs. When I added support for these SPI displays I thought that the
framebuffers on big endian machines were also big endian, but I have
later learned that this is not the case. There's a bit in the fourcc code
that controls this: DRM_FORMAT_BIG_ENDIAN.
Just remove the function to avoid confusion. We can add big endian support
later should the need arise and we have hardware to test on.
Instead of just amending the docs, expand it to explain the endianness
handling.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 35 +++
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index daac649aabdb..fa8aba6dc81c 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -824,15 +824,6 @@ u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi,
size_t len)
}
EXPORT_SYMBOL(mipi_dbi_spi_cmd_max_speed);
-static bool mipi_dbi_machine_little_endian(void)
-{
-#if defined(__LITTLE_ENDIAN)
- return true;
-#else
- return false;
-#endif
-}
-
/*
* MIPI DBI Type C Option 1
*
@@ -855,7 +846,7 @@ static int mipi_dbi_spi1e_transfer(struct mipi_dbi *dbi,
int dc,
const void *buf, size_t len,
unsigned int bpw)
{
- bool swap_bytes = (bpw == 16 && mipi_dbi_machine_little_endian());
+ bool swap_bytes = (bpw == 16);
size_t chunk, max_chunk = dbi->tx_buf9_len;
struct spi_device *spi = dbi->spi;
struct spi_transfer tr = {
@@ -1004,7 +995,7 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *dbi,
int dc,
size_t chunk = min(len, max_chunk);
unsigned int i;
- if (bpw == 16 && mipi_dbi_machine_little_endian()) {
+ if (bpw == 16) {
for (i = 0; i < (chunk * 2); i += 2) {
dst16[i] = *src16 >> 8;
dst16[i + 1] = *src16++ & 0xFF;
@@ -1218,11 +1209,23 @@ static int mipi_dbi_typec3_command(struct mipi_dbi
*dbi, u8 *cmd,
* If @dc is set, a Type C Option 3 interface is assumed, if not
* Type C Option 1.
*
- * If the SPI master driver doesn't support the necessary bits per word,
- * the following transformation is used:
+ * If the command is %MIPI_DCS_WRITE_MEMORY_START and the pixel format is
RGB565, endianness has
+ * to be taken into account. The MIPI DBI serial interface is big endian and
framebuffers are
+ * assumed stored in memory as little endian (%DRM_FORMAT_BIG_ENDIAN is not
supported).
*
- * - 9-bit: reorder buffer as 9x 8-bit words, padded with no-op command.
- * - 16-bit: if big endian send as 8-bit, if little endian swap bytes
+ * This is how endianness is handled:
+ *
+ * Option 1 (D/C as a bit): The buffer is sent on the wire byte by byte so the
16-bit buffer is
+ * byteswapped before transfer.
+ *
+ * Option 3 (D/C as a gpio): If the SPI controller supports 16 bits per word
the buffer can be
+ * sent as-is. If not the caller is responsible for
swapping the bytes
+ * before calling mipi_dbi_command_buf() and the
buffer is sent 8 bpw.
+ *
+ * This handling is optimised for %DRM_FORMAT_RGB565 framebuffers.
+ *
+ * If the interface is Option 1 and the SPI controller doesn't support 9 bits
per word,
+ * the buffer is sent as 9x 8-bit words, padded with MIPI DCS no-op commands
if necessary.
*
* Returns:
* Zero on success, negative error code on failure.
@@ -1257,7 +1260,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct
mipi_dbi *dbi,
if (dc) {
dbi->command = mipi_dbi_typec3_command;
dbi->dc = dc;
- if (mipi_dbi_machine_little_endian() &&
!spi_is_bpw_supported(spi, 16))
+ if (!spi_is_bpw_supported(spi, 16))
dbi->swap_bytes = true;
} else {
dbi->command = mipi_dbi_typec1_command;
--
2.45.0
[PATCH 4/5] drm/mipi-dbi: Add support for DRM_FORMAT_RGB888
From: Noralf Trønnes
DRM_FORMAT_RGB888 is 24 bits per pixel and it would be natural to send it
on the SPI bus using a 24 bits per word transfer. The problem with this
is that not all SPI controllers support 24 bpw.
Since DRM_FORMAT_RGB888 is stored in memory as little endian and the SPI
bus is big endian we use 8 bpw to always get the same pixel format on the
bus: b8g8r8.
The MIPI DCS specification lists the standard commands that can be sent
over the MIPI DBI interface. The set_address_mode (36h) command has one
bit in the parameter that controls RGB/BGR order. This means that the
controller can be configured to receive the pixel as BGR.
RGB888 is rarely supported on these controllers but RGB666 is very common.
All datasheets I have seen do at least support the pixel format option
where each color is sent as one byte and the 6 MSB's are used.
All this put together means that we can send each pixel as b8g8r8 and an
RGB666 capable controller sees this as b6x2g6x2r6x2.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 29 +
include/drm/drm_mipi_dbi.h | 5 +
2 files changed, 30 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index 77f8a828d6e0..eb330676857c 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -206,6 +206,7 @@ int mipi_dbi_buf_copy(void *dst, struct iosys_map *src,
struct drm_framebuffer *
struct drm_rect *clip, bool swap,
struct drm_format_conv_state *fmtcnv_state)
{
+ struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
struct drm_gem_object *gem = drm_gem_fb_get_obj(fb, 0);
struct iosys_map dst_map = IOSYS_MAP_INIT_VADDR(dst);
int ret;
@@ -222,8 +223,18 @@ int mipi_dbi_buf_copy(void *dst, struct iosys_map *src,
struct drm_framebuffer *
else
drm_fb_memcpy(_map, NULL, src, fb, clip);
break;
+ case DRM_FORMAT_RGB888:
+ drm_fb_memcpy(_map, NULL, src, fb, clip);
+ break;
case DRM_FORMAT_XRGB:
- drm_fb_xrgb_to_rgb565(_map, NULL, src, fb, clip,
fmtcnv_state, swap);
+ switch (dbidev->emulation_format) {
+ case DRM_FORMAT_RGB565:
+ drm_fb_xrgb_to_rgb565(_map, NULL, src, fb,
clip, fmtcnv_state, swap);
+ break;
+ case DRM_FORMAT_RGB888:
+ drm_fb_xrgb_to_rgb888(_map, NULL, src, fb,
clip, fmtcnv_state);
+ break;
+ }
break;
default:
drm_err_once(fb->dev, "Format is not supported: %p4cc\n",
@@ -260,9 +271,11 @@ static void mipi_dbi_fb_dirty(struct iosys_map *src,
struct drm_framebuffer *fb,
struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
unsigned int height = rect->y2 - rect->y1;
unsigned int width = rect->x2 - rect->x1;
+ const struct drm_format_info *dst_format;
struct mipi_dbi *dbi = >dbi;
bool swap = dbi->swap_bytes;
int ret = 0;
+ size_t len;
bool full;
void *tr;
@@ -283,8 +296,13 @@ static void mipi_dbi_fb_dirty(struct iosys_map *src,
struct drm_framebuffer *fb,
mipi_dbi_set_window_address(dbidev, rect->x1, rect->x2 - 1, rect->y1,
rect->y2 - 1);
- ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr,
- width * height * 2);
+ if (fb->format->format == DRM_FORMAT_XRGB)
+ dst_format = drm_format_info(dbidev->emulation_format);
+ else
+ dst_format = fb->format;
+ len = drm_format_info_min_pitch(dst_format, 0, width) * height;
+
+ ret = mipi_dbi_command_buf(dbi, MIPI_DCS_WRITE_MEMORY_START, tr, len);
err_msg:
if (ret)
drm_err_once(fb->dev, "Failed to update display %d\n", ret);
@@ -572,7 +590,7 @@ static const uint32_t mipi_dbi_formats[] = {
* has one fixed _display_mode which is rotated according to @rotation.
* This mode is used to set the mode config min/max width/height properties.
*
- * Use mipi_dbi_dev_init() if you don't need custom formats.
+ * Use mipi_dbi_dev_init() if you want native RGB565 and emulated XRGB
format.
*
* Note:
* Some of the helper functions expects RGB565 to be the default format and the
@@ -631,6 +649,9 @@ int mipi_dbi_dev_init_with_formats(struct mipi_dbi_dev
*dbidev,
drm->mode_config.min_height = dbidev->mode.vdisplay;
drm->mode_config.max_height = dbidev->mode.vdisplay;
dbidev->rotation = rotation;
+ dbidev->emulation_format = formats[0];
+ if (formats[0] == DRM_FORMAT_RGB888)
+ dbidev->dbi.writ
[PATCH 3/5] drm/mipi-dbi: Make bits per word configurable for pixel transfers
From: Noralf Trønnes
This prepares for supporting other pixel formats than RGB565.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 14 ++
include/drm/drm_mipi_dbi.h | 5 +
2 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index fa8aba6dc81c..77f8a828d6e0 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -1079,7 +1079,7 @@ static int mipi_dbi_typec1_command_read(struct mipi_dbi
*dbi, u8 *cmd,
static int mipi_dbi_typec1_command(struct mipi_dbi *dbi, u8 *cmd,
u8 *parameters, size_t num)
{
- unsigned int bpw = (*cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
+ unsigned int bpw = 8;
int ret;
if (mipi_dbi_command_is_read(dbi, *cmd))
@@ -1091,6 +1091,9 @@ static int mipi_dbi_typec1_command(struct mipi_dbi *dbi,
u8 *cmd,
if (ret || !num)
return ret;
+ if (*cmd == MIPI_DCS_WRITE_MEMORY_START)
+ bpw = dbi->write_memory_bpw;
+
return mipi_dbi_spi1_transfer(dbi, 1, parameters, num, bpw);
}
@@ -1184,8 +1187,8 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *dbi,
u8 *cmd,
if (ret || !num)
return ret;
- if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !dbi->swap_bytes)
- bpw = 16;
+ if (*cmd == MIPI_DCS_WRITE_MEMORY_START)
+ bpw = dbi->write_memory_bpw;
spi_bus_lock(spi->controller);
gpiod_set_value_cansleep(dbi->dc, 1);
@@ -1256,12 +1259,15 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct
mipi_dbi *dbi,
dbi->spi = spi;
dbi->read_commands = mipi_dbi_dcs_read_commands;
+ dbi->write_memory_bpw = 16;
if (dc) {
dbi->command = mipi_dbi_typec3_command;
dbi->dc = dc;
- if (!spi_is_bpw_supported(spi, 16))
+ if (!spi_is_bpw_supported(spi, 16)) {
+ dbi->write_memory_bpw = 8;
dbi->swap_bytes = true;
+ }
} else {
dbi->command = mipi_dbi_typec1_command;
dbi->tx_buf9_len = SZ_16K;
diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
index e8e0f8d39f3a..b36596efdcc3 100644
--- a/include/drm/drm_mipi_dbi.h
+++ b/include/drm/drm_mipi_dbi.h
@@ -56,6 +56,11 @@ struct mipi_dbi {
*/
struct spi_device *spi;
+ /**
+* @write_memory_bpw: Bits per word used on a
MIPI_DCS_WRITE_MEMORY_START transfer
+*/
+ unsigned int write_memory_bpw;
+
/**
* @dc: Optional D/C gpio.
*/
--
2.45.0
[PATCH 5/5] drm/tiny: panel-mipi-dbi: Support the pixel format property
From: Noralf Trønnes
Add support for these pixel format property values:
- r5g6b5, RGB565
- b6x2g6x2r6x2, BGR666
BGR666 is presented to userspace as RGB888. The 2 LSB in each color
are discarded by the controller. The pixel is sent on the wire using
8 bits per word (little endian) so the controller sees it as BGR.
RGB565 is the default if the property is not present.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/tiny/panel-mipi-dbi.c | 55 ++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
index f80a141fcf36..f3aa2abce314 100644
--- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c
+++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c
@@ -26,6 +26,49 @@
#include
+struct panel_mipi_dbi_format {
+ const char *name;
+ u32 fourcc;
+ unsigned int bpp;
+};
+
+static const struct panel_mipi_dbi_format panel_mipi_dbi_formats[] = {
+ { "r5g6b5", DRM_FORMAT_RGB565, 16 },
+ { "b6x2g6x2r6x2", DRM_FORMAT_RGB888, 24 },
+};
+
+static int panel_mipi_dbi_get_format(struct device *dev, u32 *formats,
unsigned int *bpp)
+{
+ const char *format_name;
+ unsigned int i;
+ int ret;
+
+ formats[1] = DRM_FORMAT_XRGB;
+
+ ret = device_property_read_string(dev, "format", _name);
+ if (ret) {
+ /* Old Device Trees don't have this property */
+ formats[0] = DRM_FORMAT_RGB565;
+ *bpp = 16;
+ return 0;
+ }
+
+ for (i = 0; i < ARRAY_SIZE(panel_mipi_dbi_formats); i++) {
+ const struct panel_mipi_dbi_format *format =
_mipi_dbi_formats[i];
+
+ if (strcmp(format_name, format->name))
+ continue;
+
+ formats[0] = format->fourcc;
+ *bpp = format->bpp;
+ return 0;
+ }
+
+ dev_err(dev, "Pixel format is not supported: '%s'\n", format_name);
+
+ return -EINVAL;
+}
+
static const u8 panel_mipi_dbi_magic[15] = { 'M', 'I', 'P', 'I', ' ', 'D',
'B', 'I',
0, 0, 0, 0, 0, 0, 0 };
@@ -276,6 +319,9 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
struct drm_device *drm;
struct mipi_dbi *dbi;
struct gpio_desc *dc;
+ unsigned int bpp;
+ size_t buf_size;
+ u32 formats[2];
int ret;
dbidev = devm_drm_dev_alloc(dev, _mipi_dbi_driver, struct
mipi_dbi_dev, drm);
@@ -323,7 +369,14 @@ static int panel_mipi_dbi_spi_probe(struct spi_device *spi)
if (IS_ERR(dbidev->driver_private))
return PTR_ERR(dbidev->driver_private);
- ret = mipi_dbi_dev_init(dbidev, _mipi_dbi_pipe_funcs, , 0);
+ ret = panel_mipi_dbi_get_format(dev, formats, );
+ if (ret)
+ return ret;
+
+ buf_size = DIV_ROUND_UP(mode.hdisplay * mode.vdisplay * bpp, 8);
+ ret = mipi_dbi_dev_init_with_formats(dbidev, _mipi_dbi_pipe_funcs,
+formats, ARRAY_SIZE(formats),
+, 0, buf_size);
if (ret)
return ret;
--
2.45.0
[PATCH 2/5] drm/mipi-dbi: Remove mipi_dbi_machine_little_endian()
From: Noralf Trønnes
mipi_dbi_machine_little_endian() should really have been called
mipi_dbi_framebuffer_little_endian() because that's the function it
performs. When I added support for these SPI displays I thought that the
framebuffers on big endian machines were also big endian, but I have
later learned that this is not the case. There's a bit in the fourcc code
that controls this: DRM_FORMAT_BIG_ENDIAN.
Just remove the function to avoid confusion. We can add big endian support
later should the need arise and we have hardware to test on.
Instead of just amending the docs, expand it to explain the endianness
handling.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_mipi_dbi.c | 35 +++
1 file changed, 19 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
index daac649aabdb..fa8aba6dc81c 100644
--- a/drivers/gpu/drm/drm_mipi_dbi.c
+++ b/drivers/gpu/drm/drm_mipi_dbi.c
@@ -824,15 +824,6 @@ u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi,
size_t len)
}
EXPORT_SYMBOL(mipi_dbi_spi_cmd_max_speed);
-static bool mipi_dbi_machine_little_endian(void)
-{
-#if defined(__LITTLE_ENDIAN)
- return true;
-#else
- return false;
-#endif
-}
-
/*
* MIPI DBI Type C Option 1
*
@@ -855,7 +846,7 @@ static int mipi_dbi_spi1e_transfer(struct mipi_dbi *dbi,
int dc,
const void *buf, size_t len,
unsigned int bpw)
{
- bool swap_bytes = (bpw == 16 && mipi_dbi_machine_little_endian());
+ bool swap_bytes = (bpw == 16);
size_t chunk, max_chunk = dbi->tx_buf9_len;
struct spi_device *spi = dbi->spi;
struct spi_transfer tr = {
@@ -1004,7 +995,7 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *dbi,
int dc,
size_t chunk = min(len, max_chunk);
unsigned int i;
- if (bpw == 16 && mipi_dbi_machine_little_endian()) {
+ if (bpw == 16) {
for (i = 0; i < (chunk * 2); i += 2) {
dst16[i] = *src16 >> 8;
dst16[i + 1] = *src16++ & 0xFF;
@@ -1218,11 +1209,23 @@ static int mipi_dbi_typec3_command(struct mipi_dbi
*dbi, u8 *cmd,
* If @dc is set, a Type C Option 3 interface is assumed, if not
* Type C Option 1.
*
- * If the SPI master driver doesn't support the necessary bits per word,
- * the following transformation is used:
+ * If the command is %MIPI_DCS_WRITE_MEMORY_START and the pixel format is
RGB565, endianness has
+ * to be taken into account. The MIPI DBI serial interface is big endian and
framebuffers are
+ * assumed stored in memory as little endian (%DRM_FORMAT_BIG_ENDIAN is not
supported).
*
- * - 9-bit: reorder buffer as 9x 8-bit words, padded with no-op command.
- * - 16-bit: if big endian send as 8-bit, if little endian swap bytes
+ * This is how endianness is handled:
+ *
+ * Option 1 (D/C as a bit): The buffer is sent on the wire byte by byte so the
16-bit buffer is
+ * byteswapped before transfer.
+ *
+ * Option 3 (D/C as a gpio): If the SPI controller supports 16 bits per word
the buffer can be
+ * sent as-is. If not the caller is responsible for
swapping the bytes
+ * before calling mipi_dbi_command_buf() and the
buffer is sent 8 bpw.
+ *
+ * This handling is optimised for %DRM_FORMAT_RGB565 framebuffers.
+ *
+ * If the interface is Option 1 and the SPI controller doesn't support 9 bits
per word,
+ * the buffer is sent as 9x 8-bit words, padded with MIPI DCS no-op commands
if necessary.
*
* Returns:
* Zero on success, negative error code on failure.
@@ -1257,7 +1260,7 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct
mipi_dbi *dbi,
if (dc) {
dbi->command = mipi_dbi_typec3_command;
dbi->dc = dc;
- if (mipi_dbi_machine_little_endian() &&
!spi_is_bpw_supported(spi, 16))
+ if (!spi_is_bpw_supported(spi, 16))
dbi->swap_bytes = true;
} else {
dbi->command = mipi_dbi_typec1_command;
--
2.45.0
[PATCH 1/5] dt-bindings: display: panel: mipi-dbi-spi: Add a pixel format property
From: Noralf Trønnes The MIPI DBI 2.0 specification (2005) lists only two pixel formats for the Type C Interface (SPI) and that is 3-bits/pixel RGB111 with 2 options for bit layout. For Type A and B (parallel) the following formats are listed: RGB332, RGB444, RGB565, RGB666 and RGB888 (some have 2 options for the bit layout). Many MIPI DBI compatible controllers support all interface types on the same chip and often the manufacturers have chosen to provide support for the Type A/B interface pixel formats also on the Type C interface. Some chips provide many pixel formats with optional bit layouts over SPI, but the most common by far are RGB565 and RGB666. So even if the specification doesn't list these formats for the Type C interface, the industry has chosen to include them. The MIPI DCS specification lists the standard commands that can be sent over the MIPI DBI interface. The set_address_mode (36h) command has one bit in the parameter that controls RGB/BGR order: This bit controls the RGB data latching order transferred from the peripheral’s frame memory to the display device. This means that each supported RGB format also has a BGR variant. Based on this rationale document the following pixel formats describing the bit layout going over the wire: - RGB111 (option 1): x2r1g1b1r1g1b1 (2 pixels per byte) - BGR111 (option 1): x2b1g1r1b1g1r1 (2 pixels per byte) - RGB111 (option 2): x1r1g1b1x1r1g1b1 (2 pixels per byte) - BGR111 (option 2): x1b1g1r1x1b1g1r1 (2 pixels per byte) - RGB565: r5g6b5 (2 bytes) - BGR565: b5g6r5 (2 bytes) - RGB666: r6x2g6x2b6x2 (3 bytes) - BGR666: b6x2g6x2r6x2 (3 bytes) (x: don't care) Signed-off-by: Noralf Trønnes --- .../bindings/display/panel/panel-mipi-dbi-spi.yaml | 31 ++ 1 file changed, 31 insertions(+) diff --git a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml index e808215cb39e..dac8f9af100e 100644 --- a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml @@ -50,6 +50,12 @@ description: | |Command or data | || + The standard defines one pixel format for type C: RGB111. The industry + however has decided to provide the type A/B interface pixel formats also on + the Type C interface and most common among these are RGB565 and RGB666. + The MIPI DCS command set_address_mode (36h) has one bit that controls RGB/BGR + order. This gives each supported RGB format a BGR variant. + The panel resolution is specified using the panel-timing node properties hactive (width) and vactive (height). The other mandatory panel-timing properties should be set to zero except clock-frequency which can be @@ -90,6 +96,29 @@ properties: spi-3wire: true + format: +description: > + Pixel format in bit order as going on the wire: +* `x2r1g1b1r1g1b1` - RGB111, 2 pixels per byte +* `x2b1g1r1b1g1r1` - BGR111, 2 pixels per byte +* `x1r1g1b1x1r1g1b1` - RGB111, 2 pixels per byte +* `x1b1g1r1x1b1g1r1` - BGR111, 2 pixels per byte +* `r5g6b5` - RGB565, 2 bytes +* `b5g6r5` - BGR565, 2 bytes +* `r6x2g6x2b6x2` - RGB666, 3 bytes +* `b6x2g6x2r6x2` - BGR666, 3 bytes + This property is optional for backwards compatibility and `r5g6b5` is + assumed in its absence. +enum: + - x2r1g1b1r1g1b1 + - x2b1g1r1b1g1r1 + - x1r1g1b1x1r1g1b1 + - x1b1g1r1x1b1g1r1 + - r5g6b5 + - b5g6r5 + - r6x2g6x2b6x2 + - b6x2g6x2r6x2 + required: - compatible - reg @@ -116,6 +145,8 @@ examples: reset-gpios = < 25 GPIO_ACTIVE_HIGH>; write-only; +format = "r5g6b5"; + backlight = <>; width-mm = <35>; -- 2.45.0
Re: [PATCH 1/4] drm/tiny: ili9225: drop driver owner assignment
On 4/24/24 08:45, Krzysztof Kozlowski wrote: > On 27/03/2024 18:48, Krzysztof Kozlowski wrote: >> Core in spi_register_driver() already sets the .owner, so driver >> does not need to. >> >> Signed-off-by: Krzysztof Kozlowski >> --- >> drivers/gpu/drm/tiny/ili9225.c | 1 - >> 1 file changed, 1 deletion(-) > > It has been almost a month. Any comments on this patchset? Can this be > picked up? > Thanks for cleaning up, patches applied to drm-misc-next. Noralf.
Re: [PATCH 2/2] drm/print: drop include seq_file.h
On 4/26/24 09:28, Jani Nikula wrote: > On Thu, 25 Apr 2024, Noralf Trønnes wrote: >> On 4/22/24 14:10, Jani Nikula wrote: >>> Never include where a forward declaration will suffice. >>> >>> Reviewed-by: Andrzej Hajda >>> Acked-by: Maxime Ripard >>> Link: >>> https://patchwork.freedesktop.org/patch/msgid/20240410141434.157908-2-jani.nik...@intel.com >>> Signed-off-by: Jani Nikula >>> --- >>> include/drm/drm_print.h | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h >>> index 561c3b96b6fd..089950ad8681 100644 >>> --- a/include/drm/drm_print.h >>> +++ b/include/drm/drm_print.h >>> @@ -28,7 +28,6 @@ >>> >>> #include >>> #include >>> -#include >>> #include >>> #include >>> >>> @@ -36,6 +35,7 @@ >>> >>> struct debugfs_regset32; >>> struct drm_device; >>> +struct seq_file; >>> >>> /* Do *not* use outside of drm_print.[ch]! */ >>> extern unsigned long __drm_debug; >> >> Looks like this broke komeda and omapdrm on arm: >> >> /home/notro/develop/dim-linux/src/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c: >> In function ‘komeda_pipeline_dump_register’: >> /home/notro/develop/dim-linux/src/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c:366:9: >> error: implicit declaration of function ‘seq_printf’; did you mean >> ‘drm_printf’? [-Werror=implicit-function-declaration] >> 366 | seq_printf(sf, "\n Pipeline-%d ==\n", >> pipe->id); >> | ^~ >> | drm_printf >> >> /home/notro/develop/dim-linux/src/drivers/gpu/drm/omapdrm/omap_fb.c: In >> function ‘omap_framebuffer_describe’: >> /home/notro/develop/dim-linux/src/drivers/gpu/drm/omapdrm/omap_fb.c:325:9: >> error: implicit declaration of function ‘seq_printf’; did you mean >> ‘drm_printf’? [-Werror=implicit-function-declaration] >> 325 | seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width, fb->height, >> | ^~ >> | drm_printf >> >> Noralf. > > Sad trombone. I built on arm and arm64, with omap and komeda enabled, > but apparently still missed some options. Sorry. :( > > Dave fixed these when pulling drm-misc-next, so a backmerge from > drm-next to drm-misc-next should handle it. > > Sorry again, > Jani. > No problem, I was just suprised that the build bots hadn't caught this, but just minutes after I sent this it was detected. I kind of assumed that in this day and age there was a bot that checked drm-tip after each push/sync, but not there yet I guess. I used the defconfigs from drm-tip:rerere-cache which caught this. Noralf.
Re: [PATCH 2/2] drm/print: drop include seq_file.h
On 4/22/24 14:10, Jani Nikula wrote: > Never include where a forward declaration will suffice. > > Reviewed-by: Andrzej Hajda > Acked-by: Maxime Ripard > Link: > https://patchwork.freedesktop.org/patch/msgid/20240410141434.157908-2-jani.nik...@intel.com > Signed-off-by: Jani Nikula > --- > include/drm/drm_print.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/drm/drm_print.h b/include/drm/drm_print.h > index 561c3b96b6fd..089950ad8681 100644 > --- a/include/drm/drm_print.h > +++ b/include/drm/drm_print.h > @@ -28,7 +28,6 @@ > > #include > #include > -#include > #include > #include > > @@ -36,6 +35,7 @@ > > struct debugfs_regset32; > struct drm_device; > +struct seq_file; > > /* Do *not* use outside of drm_print.[ch]! */ > extern unsigned long __drm_debug; Looks like this broke komeda and omapdrm on arm: /home/notro/develop/dim-linux/src/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c: In function ‘komeda_pipeline_dump_register’: /home/notro/develop/dim-linux/src/drivers/gpu/drm/arm/display/komeda/komeda_pipeline.c:366:9: error: implicit declaration of function ‘seq_printf’; did you mean ‘drm_printf’? [-Werror=implicit-function-declaration] 366 | seq_printf(sf, "\n Pipeline-%d ==\n", pipe->id); | ^~ | drm_printf /home/notro/develop/dim-linux/src/drivers/gpu/drm/omapdrm/omap_fb.c: In function ‘omap_framebuffer_describe’: /home/notro/develop/dim-linux/src/drivers/gpu/drm/omapdrm/omap_fb.c:325:9: error: implicit declaration of function ‘seq_printf’; did you mean ‘drm_printf’? [-Werror=implicit-function-declaration] 325 | seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width, fb->height, | ^~ | drm_printf Noralf.
Re: [PATCH 09/15] drm/gud: switch to struct drm_edid
On 4/16/24 15:22, Jani Nikula wrote: > Prefer struct drm_edid based functions over struct edid. > > Signed-off-by: Jani Nikula > > --- > Thanks, Reviewed-by: Noralf Trønnes
Re: [PATCH 37/43] drm/tiny/mi0283qt: Use fbdev-dma
On 3/12/24 16:45, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by mi0283qt. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: "Noralf Trønnes" > --- > drivers/gpu/drm/tiny/mi0283qt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Acked-by: Noralf Trønnes
Re: [PATCH 10/43] drm/gud: Use fbdev-shmem
On 3/12/24 16:45, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-shmem. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: "Noralf Trønnes" > --- Acked-by: Noralf Trønnes
Re: [PATCH 38/43] drm/tiny/panel-mipi-dbi: Use fbdev-dma
On 3/12/24 16:45, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by panel-mipi-dbi. Avoids the > overhead of fbdev-generic's additional shadow buffering. No functional > changes. > > Signed-off-by: Thomas Zimmermann > Cc: "Noralf Trønnes" > --- > drivers/gpu/drm/tiny/panel-mipi-dbi.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Acked-by: Noralf Trønnes
Re: [PATCH 39/43] drm/tiny/repaper: Use fbdev-dma
On 3/12/24 16:45, Thomas Zimmermann wrote: > Implement fbdev emulation with fbdev-dma. Fbdev-dma now supports > damage handling, which is required by repaper. Avoids the overhead of > fbdev-generic's additional shadow buffering. No functional changes. > > Signed-off-by: Thomas Zimmermann > Cc: "Noralf Trønnes" > --- > drivers/gpu/drm/tiny/repaper.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > Acked-by: Noralf Trønnes
Re: State of affairs with Ilitek 9341 support
On 12/8/23 17:00, Andy Shevchenko wrote: > Included authors and latest (non-white-space) contributors to the drivers > in question along with relevant mailing list and respective (active in the > area) maintainers. > > I already had risen the question in times when 4th (sic!) driver for the same > hardware was about to be pulled into upstream that we have to somehow reduce > the code base and unify device properties. > > So, the main question here "What is the plan and where are we now?" > > I admit that fbtft case is special as it supports, in particular, platform > device (parallel interface) and also well established in the embedded world. > What about the rest? > > N.B. Besides the fact that panel drivers are too OF-centric, which is bad > practice for the new kernel code in general and has to be stopped. I.o.w. > seeing of_property_*() or alike in the driver after ca. 2020 should be > immediate NAK unless it's very well justified why it may not be used on > non-OF systems. > Last year drivers/gpu/drm/tiny/panel-mipi-dbi.c was added to support all MIPI DBI compatible (ili9341) SPI displays. It loads the initialisation commands from a firmware file. For more info see https://github.com/notro/panel-mipi-dbi/wiki. When I started on fbtft in 2013 I didn't know about MIPI DBI so I made some common bus access functions and one driver per controller and that driver had an initialisation sequence to match the panel I had. Then I discovered that displays using the same controller could have different init sequences so I added a Device Tree property that could override the driver init. In 2015 fbtft was added to drivers/staging, but later that year fbdev was closed for new drivers so it was a dead end. I started to work on porting fbtft to DRM and almost 2 years later support for the MI0283QT panel (ILI9341) was added. I had now learned about MIPI DBI so a library to handle that was added. I had asked on the Device Tree ML about the property and I was told that I couldn't have that which meant that I couldn't get away with having just one driver for the MIPI DBI compatible display panels as I was first hoping for. I was aware that there was a challenge going from fbtft to DRM because in fbtft there is support for all panel setups using the property, but in DRM every panel needed support in a driver. So I started to look at adding Device Tree properties to describe the setup for one controller. This would make it easy to describe a new panel in Device Tree for a supported controller. Maxime Ripard came up with the idea to have the controller initialisation commands in a firmware file which meant that we could get away with having just one driver for all MIPI DBI SPI panels (which is the vast majority of these SPI pixel upload panels). This meant that SPI support could be removed from all the MIPI DBI compatible controllers in fbtft since there's now a solution for them in DRM. The drivers themselves must stay since they also have parallel bus support which is lacking in DRM. My plan was to wait for panel-mipi-dbi to hit an LTS and then either prepare patches to remove MIPI DBI SPI support from fbtft or at least send an email to staging about the new driver. Unfortunately my health problems got worse and many plans went out the window. ILI9341 DRM drivers - drivers/gpu/drm/tiny/mi0283qt.c This was the first driver added for the MI0283QT panel series - drivers/gpu/drm/tiny/ili9341.c Later ili9341 based panels was decided to be added to a controller specific driver. - drivers/gpu/drm/tiny/panel-mipi-dbi.c Generic MIPI DBI SPI driver that loads init commands from a firmware file. It uses of_property_read_string_index() and of_get_drm_panel_display_mode(). I don't know if it's possible to make device_property_*() versions of those. - drivers/gpu/drm/panel/panel-ilitek-ili9341.c This driver supports the MIPI DPI (RGB) interface on the controller. Controller init is done over MIPI DBI SPI. The driver does also for some reason support the same panel as the ili9341.c driver does. So 2 drivers for the same panel... Sidenote: It is possible to make a generic panel-mipi-dpi.c driver for panels that use DPI for pixels and DBI for init loaded from a firmware file. Noralf.
Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler
On 10/10/23 11:25, Maxime Ripard wrote: > > > On Tue, Oct 10, 2023 at 10:55:09AM +0200, Thomas Zimmermann wrote: So if I understand correctly, drm_panic would pre-allocate a plane/commit, and use that when a panic occurs ? >>> >>> And have it checked already, yes. We would only wait for a panic to >>> happen to pull the trigger on the commit. >>> I have two concern about this approach: - How much memory would be allocated for this ? a whole framebuffer can be big for just this use case. >> >> As I outlined in my email at [1], there are a number of different scenarios. >> The question of atomic state and commits is entirely separate from the DRM >> panic handler. We should not throw them together. Whatever is necessary is >> get a scanout buffer, should happen on the driver side of >> get_scanout_buffer, not on the drm_panic side. >> >> [1] >> https://lore.kernel.org/dri-devel/39bd4c35-8a61-42ee-8675-ccea4f5d4...@suse.de/T/#m22f116e9438e00a5f0a9dc43987d4153424f8be1 >> >>> >>> I'dd expect a whole framebuffer for the current >>> configuration/resolution. It would be typically 4MB for a full-HD system >>> which isn't a lot really and I guess we can always add an option to >>> disable the mechanism if needed. >>> - I find it risky to completely reconfigure the hardware in a panic handler. >>> >>> I would expect to only change the format and base address of the >>> framebuffer. I guess it can fail, but it doesn't seem that different to >>> the async plane update we already have and works well. >> >> The one thing I don't understand is: Why should we use atomic commits in the >> first place? It doesn't make sense for firmware-based drivers. > > Because this is generic infrastructure that is valuable for any drivers > and not only firmware-based drivers? > >> In some drivers, even the simple ast, we hold locks during the regular >> commit. Trying to run the panic commit concurrently will likely give a >> deadlock. > > You're in the middle of a panic. Don't take any locks and you won't deadlock. > >> In the end it's a per-driver decision, but in most cases, the driver can >> easily switch to a default mode with some ad-hoc code. > > When was the last time a per-driver decision has been a good thing? I'm > sorry, but the get_scanout_buffer approach buffer won't work for any > driver out there. > > I'm fine with discussing alternatives if you don't like the ones I > suggested, but they must allow the panic handler infrastructure to work > with any driver we have, not just 4. > Why can't we use the model[1] suggested by Daniel using a draw_pixel callback giving drivers full control on how they can put a pixel on the display? This will even work for the AMD debug interface. In the linear CPU accessible buffer case, we can provide a helper for that, maybe we can do helpers for other common cases as well. Adding to that we would need a panic_setup/enter and panic_teardown/exit callback. Noralf. [1] https://lore.kernel.org/dri-devel/20160810091529.GQ6232@phenom.ffwll.local/
Re: [PATCH v4 4/4] drm/mgag200: Add drm_panic support
On 10/3/23 16:22, Jocelyn Falempe wrote:
> Add support for the drm_panic module, which displays a message to
> the screen when a kernel panic occurs.
>
> Signed-off-by: Jocelyn Falempe
> ---
> drivers/gpu/drm/mgag200/mgag200_drv.c | 24
> 1 file changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.c
> b/drivers/gpu/drm/mgag200/mgag200_drv.c
> index 976f0ab2006b..229d9c116b42 100644
> --- a/drivers/gpu/drm/mgag200/mgag200_drv.c
> +++ b/drivers/gpu/drm/mgag200/mgag200_drv.c
> @@ -12,10 +12,12 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> #include
> +#include
> #include
>
> #include "mgag200_drv.h"
> @@ -83,6 +85,27 @@ resource_size_t mgag200_probe_vram(void __iomem *mem,
> resource_size_t size)
> return offset - 65536;
> }
>
> +static int mgag200_get_scanout_buffer(struct drm_device *dev,
> + struct drm_scanout_buffer *sb)
> +{
> + struct drm_plane *plane;
> + struct mga_device *mdev = to_mga_device(dev);
> + struct iosys_map map = IOSYS_MAP_INIT_VADDR_IOMEM(mdev->vram);
> +
> + /* mgag200 has only one plane */
> + drm_for_each_plane(plane, dev) {
In my first 2016 attempt on a panic handler I was told to trylock crtc
and plane and skip if it wasn't possible:
- We need locking. One of the biggest problems with the old oops handling
was that it was very good at trampling over driver state, causing more
(unrelated) oopses in kms code and making sure the original oops was no
longer visible. I think the shared code must take care of all the
locking needs to avoid fragile code in drivers. ww_mutex_trylock on the
drm_crtc and drm_plane should be enough (we need both for drivers where
planes can be reassigned between drivers).
See [1] for a list of other things to consider.
Noralf.
[1]
https://lore.kernel.org/dri-devel/20160810091529.GQ6232@phenom.ffwll.local/
> + if (!plane->state || !plane->state->fb)
> + return -ENODEV;
> + sb->format = plane->state->fb->format;
> + sb->width = plane->state->fb->width;
> + sb->height = plane->state->fb->height;
> + sb->pitch = plane->state->fb->pitches[0];
> + sb->map = map;
> + return 0;
> + }
> + return -ENODEV;
> +}
> +
> /*
> * DRM driver
> */
> @@ -98,6 +121,7 @@ static const struct drm_driver mgag200_driver = {
> .major = DRIVER_MAJOR,
> .minor = DRIVER_MINOR,
> .patchlevel = DRIVER_PATCHLEVEL,
> + .get_scanout_buffer = mgag200_get_scanout_buffer,
> DRM_GEM_SHMEM_DRIVER_OPS,
> };
>
Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler
On 10/3/23 16:22, Jocelyn Falempe wrote:
> This module displays a user friendly message when a kernel panic
> occurs. It currently doesn't contain any debug information,
> but that can be added later.
>
> v2
> * Use get_scanout_buffer() instead of the drm client API.
> (Thomas Zimmermann)
> * Add the panic reason to the panic message (Nerdopolis)
> * Add an exclamation mark (Nerdopolis)
>
> v3
> * Rework the drawing functions, to write the pixels line by line and
> to use the drm conversion helper to support other formats.
> (Thomas Zimmermann)
>
> v4
> * Use drm_fb_r1_to_32bit for fonts (Thomas Zimmermann)
> * Remove the default y to DRM_PANIC config option (Thomas Zimmermann)
> * Add foreground/background color config option
> * Fix the bottom lines not painted if the framebuffer height
>is not a multiple of the font height.
> * Automatically register the device to drm_panic, if the function
>get_scanout_buffer exists. (Thomas Zimmermann)
>
> Signed-off-by: Jocelyn Falempe
> ---
> drivers/gpu/drm/Kconfig | 22 ++
> drivers/gpu/drm/Makefile| 1 +
> drivers/gpu/drm/drm_drv.c | 8 +
> drivers/gpu/drm/drm_panic.c | 413
> include/drm/drm_drv.h | 14 ++
> include/drm/drm_panic.h | 41
> 6 files changed, 499 insertions(+)
> create mode 100644 drivers/gpu/drm/drm_panic.c
> create mode 100644 include/drm/drm_panic.h
>
> diff --git a/drivers/gpu/drm/drm_panic.c b/drivers/gpu/drm/drm_panic.c
> new file mode 100644
> +static void draw_panic_device(struct drm_device *dev, const char *msg)
> +{
> + struct drm_scanout_buffer sb;
> +
> + if (dev->driver->get_scanout_buffer(dev, ))
> + return;
> + if (!drm_panic_line_buf)
> + return;
> +
Unless something has changed since 2019 we need to make sure fbcon
hasn't already printed the panic to avoid jumbled output. See [1] for
more info.
Noralf.
[1]
https://lore.kernel.org/dri-devel/20190312095320.GX2665@phenom.ffwll.local/
> + /* to avoid buffer overflow on drm_panic_line_buf */
> + if (sb.width > DRM_PANIC_MAX_WIDTH)
> + sb.width = DRM_PANIC_MAX_WIDTH;
> +
> + draw_panic_static(, msg);
> +}
Re: [PATCH v4 0/7] drm: Reuse temporary memory for format conversion
On 10/5/23 11:04, Thomas Zimmermann wrote: > DRM's format-conversion helpers require temporary memory. Pass the > buffer from the caller and keep it allocated over several calls. Allow > the caller to preallocate the buffer memory. > > The motivation for this patchset is the recent work on a DRM panic > handler. [1] The panic handler requires format conversion to display an > error to the screen. But allocating memory during kernel panics is > fragile. The changes in this patchset enable the DRM panic handler to > preallocate buffer storage before the panic occurs. > I've been thinking about this and afaiu this requires the display hw to switch to the new panic buffer for scanout, right? I don't think that is possible for any complex hw to do in a panic situation. Or are you thinking that the driver should somehow "memcpy" this buffer to the actual scanout buffer? Noralf. > As an additonal benefit, drivers can now keep the temporary storage > across multiple updates. Avoiding memory allocation slightly reduces > the CPU overhead of the format helpers. > > Patch 1 adds struct drm_format_conv_state, a simple interface to pass > around the buffer storage. Patch 2 adds an instance of the struct to > the shadow-plane state. Patch 3 moves the buffer's memory management > from the format helpers into their callers within the DRM drivers. Most > of the afected drivers use the state instance stored in their shadow- > plane state. The shadow-plane code releases the buffer memory automatically. > > Patches 4 to 7 update three drivers to preallocate the format-conversion > buffer in their plane's atomic_check function. The driver thus detects OOM > errors before the display update begins. > > Tested with simpledrm. > > v4: > * rename struct to drm_format_conv_state (Javier) > * replace ARRAY_SIZE() with sizeof() (Jani) > * store buffer in shadow-plane state (Javier, Maxime) > * prealloc in atomic_check in several drivers > v3: > * no changes > v2: > * reserve storage during probing in the drivers > > [1] https://patchwork.freedesktop.org/series/122244/ > > Thomas Zimmermann (7): > drm/format-helper: Cache buffers with struct drm_format_conv_state > drm/atomic-helper: Add format-conversion state to shadow-plane state > drm/format-helper: Pass format-conversion state to helpers > drm/ofdrm: Preallocate format-conversion buffer in atomic_check > drm/simpledrm: Preallocate format-conversion buffer in atomic_check > drm/ssd130x: Fix atomic_check for disabled planes > drm/ssd130x: Preallocate format-conversion buffer in atomic_check > > drivers/gpu/drm/drm_format_helper.c | 212 +- > drivers/gpu/drm/drm_gem_atomic_helper.c | 9 + > drivers/gpu/drm/drm_mipi_dbi.c| 19 +- > drivers/gpu/drm/gud/gud_pipe.c| 30 ++- > drivers/gpu/drm/solomon/ssd130x.c | 36 ++- > .../gpu/drm/tests/drm_format_helper_test.c| 72 +++--- > drivers/gpu/drm/tiny/cirrus.c | 3 +- > drivers/gpu/drm/tiny/ili9225.c| 10 +- > drivers/gpu/drm/tiny/ofdrm.c | 16 +- > drivers/gpu/drm/tiny/repaper.c| 8 +- > drivers/gpu/drm/tiny/simpledrm.c | 43 +++- > drivers/gpu/drm/tiny/st7586.c | 19 +- > include/drm/drm_format_helper.h | 81 +-- > include/drm/drm_gem_atomic_helper.h | 10 + > include/drm/drm_mipi_dbi.h| 4 +- > 15 files changed, 428 insertions(+), 144 deletions(-) > > > base-commit: 57d3b83a83c5527325efb5bcaf594da09fe4a41b
Re: [PATCH v4 2/4] drm/panic: Add a drm panic handler
On 10/6/23 16:35, Maxime Ripard wrote:
> Hi Jocelyn,
>
> On Thu, Oct 05, 2023 at 11:16:15AM +0200, Jocelyn Falempe wrote:
>> On 05/10/2023 10:18, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Tue, Oct 03, 2023 at 04:22:45PM +0200, Jocelyn Falempe wrote:
diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
index 89e2706cac56..e538c87116d3 100644
--- a/include/drm/drm_drv.h
+++ b/include/drm/drm_drv.h
@@ -43,6 +43,7 @@ struct dma_buf_attachment;
struct drm_display_mode;
struct drm_mode_create_dumb;
struct drm_printer;
+struct drm_scanout_buffer;
struct sg_table;
/**
@@ -408,6 +409,19 @@ struct drm_driver {
*/
void (*show_fdinfo)(struct drm_printer *p, struct drm_file *f);
+ /**
+ * @get_scanout_buffer:
+ *
+ * Get the current scanout buffer, to display a panic message with
drm_panic.
+ * It is called from a panic callback, and must follow its restrictions.
+ *
+ * Returns:
+ *
+ * Zero on success, negative errno on failure.
+ */
+ int (*get_scanout_buffer)(struct drm_device *dev,
+struct drm_scanout_buffer *sb);
+
>>>
>>> What is the format of that buffer? What is supposed to happen if the
>>> planes / CRTC are setup in a way that is incompatible with the buffer
>>> format?
>>
>> Currently, it only supports linear format, either in system memory, or
>> iomem.
>> But really what is needed is the screen size, and a way to write pixels to
>> it.
>> For more complex GPU, I don't know if it's easier to reprogram the GPU to
>> linear format, or to add a simple "tiled" support to drm_panic.
>> What would you propose as a panic interface to handle those complex format ?
>
> It's not just about tiling, but also about YUV formats. If the display
> engine is currently playing a video at the moment, it's probably going
> to output some variation of multi-planar YUV and you won't have an RGB
> buffer available.
>
I had support for some YUV formats in my 2019 attempt on a panic
handler[1] and I made a recording of a test run as well[2] (see 4:30 for
YUV). There was a discussion about challenges and i915 can disable
tiling by flipping a bit in a register[3] and AMD has a debug
interface[4] they can use to write pixels.
Noralf.
[1]
https://lore.kernel.org/dri-devel/20190311174218.51899-1-nor...@tronnes.org/
[2] https://youtu.be/lZ80vL4dgpE
[3]
https://lore.kernel.org/dri-devel/20190314095004.GP2665@phenom.ffwll.local/
[4]
https://lore.kernel.org/dri-devel/d233c376-ed07-2127-6084-8292d313d...@amd.com/
> Same story if you're using a dma-buf buffer. You might not even be able
> to access that buffer at all from the CPU or the kernel.
>
> I really think we should have some emergency state ready to commit on
> the side, and possibly a panic_commit function to prevent things like
> sleeping or waiting that regular atomic_commit can use.
>
> That way, you know have all the resources available to you any time.
>
>> Sometime it's also just not possible to write pixels to the screen, like if
>> the panic occurs in the middle of suspend/resume, or during a mode-setting,
>> and the hardware state is broken. In this case it's ok to return an error,
>> and nothing will get displayed.
>
> And yeah, you won't be able to do it every time, but if it's never for
> some workload it's going to be a concern.
>
> Anyway, we should at the very least document what we expect here.
>
> Maxime
Re: [PATCH v4 3/7] drm/format-helper: Pass format-conversion state to helpers
On 10/5/23 11:04, Thomas Zimmermann wrote:
> Pass an instance of struct drm_format_conv_state to DRM's format
> conversion helpers. Update all callers.
>
> Most drivers can use the format-conversion state from their shadow-
> plane state. The shadow plane's destroy function releases the
> allocated buffer. Drivers will later be able to allocate a buffer
> of appropriate size in their plane's atomic_check code.
>
> The gud driver uses a separate thread for committing updates. For
> now, the update worker contains its own format-conversion state.
>
> Images in the format-helper tests are small. The tests preallocate
> a static page for the temporary buffer. Unloading the module releases
> the memory.
>
> v3:
> * store buffer in shadow-plane state (Javier, Maxime)
> * replace ARRAY_SIZE() with sizeof() (Jani)
>
> Signed-off-by: Thomas Zimmermann
> Cc: Noralf Trønnes
> Cc: Javier Martinez Canillas
> Cc: Gerd Hoffmann
> Cc: David Lechner
> ---
> diff --git a/drivers/gpu/drm/drm_format_helper.c
> b/drivers/gpu/drm/drm_format_helper.c
> index 37c499ae4fe4f..b9b3dadf7b5f8 100644
> --- a/drivers/gpu/drm/drm_format_helper.c
> +++ b/drivers/gpu/drm/drm_format_helper.c
> @@ -328,6 +324,7 @@ static void drm_fb_swab32_line(void *dbuf, const void
> *sbuf, unsigned int pixels
> * @fb: DRM framebuffer
> * @clip: Clip rectangle area to copy
> * @cached: Source buffer is mapped cached (eg. not write-combined)
> + * @xfrm: Transform and conversion state
Here and throughout the patch: xfrm does not match the argument name.
> *
> * This function copies parts of a framebuffer to display memory and swaps
> per-pixel
> * bytes during the process. Destination and framebuffer formats must match.
> The
> @@ -342,7 +339,8 @@ static void drm_fb_swab32_line(void *dbuf, const void
> *sbuf, unsigned int pixels
> */
> void drm_fb_swab(struct iosys_map *dst, const unsigned int *dst_pitch,
>const struct iosys_map *src, const struct drm_framebuffer *fb,
> - const struct drm_rect *clip, bool cached)
> + const struct drm_rect *clip, bool cached,
> + struct drm_format_conv_state *state)
> {
> const struct drm_format_info *format = fb->format;
> u8 cpp = DIV_ROUND_UP(drm_format_info_bpp(format, 0), 8);
> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> index 73dd4f4289c20..826fb20dbbf0d 100644
> --- a/drivers/gpu/drm/tiny/repaper.c
> +++ b/drivers/gpu/drm/tiny/repaper.c
> @@ -830,13 +831,14 @@ static void repaper_pipe_update(struct
> drm_simple_display_pipe *pipe,
> struct drm_plane_state *old_state)
> {
> struct drm_plane_state *state = pipe->plane.state;
> + struct drm_shadow_plane_state *shadow_plane_state =
> to_drm_shadow_plane_state(state);
> struct drm_rect rect;
>
> if (!pipe->crtc.state->active)
> return;
>
> if (drm_atomic_helper_damage_merged(old_state, state, ))
> - repaper_fb_dirty(state->fb);
> + repaper_fb_dirty(state->fb, _plane_state->fmtcnv_state);
This won't work since repaper doesn't use the shadow plane helper.
Noralf.
> }
>
> static const struct drm_simple_display_pipe_funcs repaper_pipe_funcs = {
Re: [PATCH v4 2/7] drm/atomic-helper: Add format-conversion state to shadow-plane state
On 10/5/23 11:04, Thomas Zimmermann wrote: > Store an instance of struct drm_format_conv_state in the shadow-plane > state struct drm_shadow_plane_state. Many drivers with shadow planes > use DRM's format helpers to copy or convert the framebuffer data to > backing storage in the scanout buffer. The shadow plane provides the > necessary state and manages the conversion's intermediate buffer memory. > > Signed-off-by: Thomas Zimmermann > --- Acked-by: Noralf Trønnes
Re: [PATCH v4 1/7] drm/format-helper: Cache buffers with struct drm_format_conv_state
On 10/5/23 11:04, Thomas Zimmermann wrote: > Hold temporary memory for format conversion in an instance of struct > drm_format_conv_state. Update internal helpers of DRM's format-conversion > code accordingly. Drivers will later be able to maintain this cache by > themselves. > > Besides caching, struct drm_format_conv_state will be useful to hold > additional information for format conversion, such as palette data or > foreground/background colors. This will enable conversion from indexed > color formats to component-based formats. > > v3: > * rename struct drm_xfrm_buf to struct drm_format_conv_state > (Javier) > * remove managed cleanup > * add drm_format_conv_state_copy() for shadow-plane support > > Signed-off-by: Thomas Zimmermann > --- > drivers/gpu/drm/drm_format_helper.c | 115 +--- > include/drm/drm_format_helper.h | 51 > 2 files changed, 155 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/drm_format_helper.c > b/drivers/gpu/drm/drm_format_helper.c > index f93a4efcee909..37c499ae4fe4f 100644 > --- a/drivers/gpu/drm/drm_format_helper.c > +++ b/drivers/gpu/drm/drm_format_helper.c > @@ -20,6 +20,94 @@ > #include > #include > > +/** > + * drm_format_conv_state_init - Initialize format-conversion state > + * @state: The state to initialize > + * > + * Clears all fields in struct drm_format_conv_state and installs a DRM > + * release action for the buffer. The buffer will be empty with no > + * preallocated resources. You forgot to remove the release action note in this version. With that fixed: Acked-by: Noralf Trønnes
Re: [RFC][PATCH v2 0/2] drm/panic: Add a drm panic handler
Hi, On 9/15/23 10:28, Jocelyn Falempe wrote: > This introduces a new drm panic handler, which displays a message when a > panic occurs. > So when fbcon is disabled, you can still see a kernel panic. > > This is one of the missing feature, when disabling VT/fbcon in the kernel: > https://www.reddit.com/r/linux/comments/10eccv9/config_vtn_in_2023/ > Fbcon can be replaced by a userspace kms console, but the panic screen must > be done in the kernel. > > This is a proof of concept, and works only with simpledrm, using a new > get_scanout_buffer() api > There's a panic handling entry in Documentation/gpu/todo.rst pointing to some work done in this area. Noralf. > To test it, make sure you're using the simpledrm driver, and trigger a panic: > echo c > /proc/sysrq-trigger > > v2 > * Use get_scanout_buffer() instead of the drm client API. (Thomas Zimmermann) > * Add the panic reason to the panic message (Nerdopolis) > * Add an exclamation mark (Nerdopolis) > > I didn't reuse the fbdev functions yet, that would need some fbdev > refactoring, because they rely on struct fb_info, and struct vc_data (for > font/console). But I still plan to at least try it for v3. > > A few more though: > 1) what about gpu with multiple monitor connected ? > maybe get_scanout_buffer() could return a list of scanout buffers ? > 2) I think for some GPU drivers, there might need a flush_scanout_buffer() > function, that should be called after the scanout buffer has been filled ? > > Best regards, > > Jocelyn Falempe (2): > drm/panic: Add a drm panic handler > drm/simpledrm: Add drm_panic support > > drivers/gpu/drm/Kconfig | 11 ++ > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/drm_drv.c| 3 + > drivers/gpu/drm/drm_panic.c | 270 +++ > drivers/gpu/drm/tiny/simpledrm.c | 17 ++ > include/drm/drm_drv.h| 14 ++ > include/drm/drm_panic.h | 41 + > 7 files changed, 357 insertions(+) > create mode 100644 drivers/gpu/drm/drm_panic.c > create mode 100644 include/drm/drm_panic.h > > > base-commit: 2dde18cd1d8fac735875f2e4987f11817cc0bc2c
Re: [PATCH] drm/repaper: fix -Wvoid-pointer-to-enum-cast warning
On 8/16/23 21:55, Justin Stitt wrote:
> When building with clang 18 I see the following warning:
> | drivers/gpu/drm/tiny/repaper.c:952:11: warning: cast to smaller
> integer
> | type 'enum repaper_model' from 'const void *'
> [-Wvoid-pointer-to-enum-cast]
> | 952 | model = (enum repaper_model)match;
> |
>
> This is due to the fact that `match` is a void* while `enum repaper_model`
> has the size of an int.
>
> Add uintptr_t cast to silence clang warning while also keeping enum cast
> for readability and consistency with other `model` assignment just a
> few lines below:
> | model = (enum repaper_model)spi_id->driver_data;
>
> Link: https://github.com/ClangBuiltLinux/linux/issues/1910
> Reported-by: Nathan Chancellor
> Signed-off-by: Justin Stitt
> ---
Thanks, applied to drm-misc-next.
Noralf.
> drivers/gpu/drm/tiny/repaper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/tiny/repaper.c b/drivers/gpu/drm/tiny/repaper.c
> index c2677d081a7b..165f2099e7d8 100644
> --- a/drivers/gpu/drm/tiny/repaper.c
> +++ b/drivers/gpu/drm/tiny/repaper.c
> @@ -949,7 +949,7 @@ static int repaper_probe(struct spi_device *spi)
>
> match = device_get_match_data(dev);
> if (match) {
> - model = (enum repaper_model)match;
> + model = (enum repaper_model)(uintptr_t)match;
> } else {
> spi_id = spi_get_device_id(spi);
> model = (enum repaper_model)spi_id->driver_data;
>
> ---
> base-commit: 2ccdd1b13c591d306f0401d98dedc4bdcd02b421
> change-id: 20230816-void-drivers-gpu-drm-tiny-repaper-a08321cd99d7
>
> Best regards,
> --
> Justin Stitt
>
Re: [PATCH v3 0/2] drm/mipi-dbi: Allow using same the D/C GPIO for multiple displays
On 7/24/23 08:56, Otto Pflüger wrote: > When multiple displays are connected to the same SPI bus, the data/command > switch is sometimes considered part of the bus and is shared among the > displays. > > This series adds the GPIO_FLAGS_BIT_NONEXCLUSIVE flag for this GPIO and > SPI bus locking to the panel-mipi-dbi/drm_mipi_dbi drivers to support > this hardware setup. > > --- Thanks, applied to drm-misc-next. Noralf. > Changes in v3: > - add comment and remove unnecessary line break as suggested by Noralf > Changes in v2: > - fix uses of mipi_dbi_spi_transfer outside drm_mipi_dbi.c (errors > reported by kernel test robot) > - remove the is_locked argument introduced in v1 which was always set to > true > > Otto Pflüger (2): > drm/mipi-dbi: Lock SPI bus before setting D/C GPIO > drm/tiny: panel-mipi-dbi: Allow sharing the D/C GPIO > > drivers/gpu/drm/drm_mipi_dbi.c| 17 + > drivers/gpu/drm/tiny/ili9225.c| 7 ++- > drivers/gpu/drm/tiny/ili9486.c| 4 > drivers/gpu/drm/tiny/panel-mipi-dbi.c | 3 ++- > 4 files changed, 25 insertions(+), 6 deletions(-) > > > base-commit: ae867bc97b713121b2a7f5fcac68378a0774739b
Re: [PATCH v2 1/2] drm/mipi-dbi: Lock SPI bus before setting D/C GPIO
On 7/20/23 12:26, Otto Pflüger wrote: > Multiple displays may be connected to the same bus and share a D/C GPIO, > so the display driver needs exclusive access to the bus to ensure that > it can control the D/C GPIO safely. > > Signed-off-by: Otto Pflüger > --- > drivers/gpu/drm/drm_mipi_dbi.c | 17 + > drivers/gpu/drm/tiny/ili9225.c | 7 ++- > drivers/gpu/drm/tiny/ili9486.c | 4 You forgot to CC the driver maintainers, please add them in the next version of the patchset. The get_maintainer script should be able to pick them out for you. Noralf.
Re: [PATCH v2 2/2] drm/tiny: panel-mipi-dbi: Allow sharing the D/C GPIO
On 7/20/23 12:26, Otto Pflüger wrote: > Displays that are connected to the same SPI bus may share the D/C GPIO. > Use GPIOD_FLAGS_BIT_NONEXCLUSIVE to allow access to the same GPIO for > multiple panel-mipi-dbi instances. Exclusive access to the GPIO during > transfers is ensured by the locking in drm_mipi_dbi.c. > > Signed-off-by: Otto Pflüger > --- > drivers/gpu/drm/tiny/panel-mipi-dbi.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/tiny/panel-mipi-dbi.c > b/drivers/gpu/drm/tiny/panel-mipi-dbi.c > index eb9f13f18a02..e616e3890043 100644 > --- a/drivers/gpu/drm/tiny/panel-mipi-dbi.c > +++ b/drivers/gpu/drm/tiny/panel-mipi-dbi.c > @@ -307,7 +307,8 @@ static int panel_mipi_dbi_spi_probe(struct spi_device > *spi) > if (IS_ERR(dbi->reset)) > return dev_err_probe(dev, PTR_ERR(dbi->reset), "Failed to get > GPIO 'reset'\n"); > > - dc = devm_gpiod_get_optional(dev, "dc", GPIOD_OUT_LOW); Please add a comment here that sharing only works when on the same SPI bus. > + dc = devm_gpiod_get_optional(dev, "dc", > + GPIOD_OUT_LOW | > GPIOD_FLAGS_BIT_NONEXCLUSIVE); I'd prefer this to be on one line, the 80 column limit has been upped to a 100 now. Noralf. > if (IS_ERR(dc)) > return dev_err_probe(dev, PTR_ERR(dc), "Failed to get GPIO > 'dc'\n"); >
Re: [PATCH v2 1/2] drm/mipi-dbi: Lock SPI bus before setting D/C GPIO
On 7/20/23 12:26, Otto Pflüger wrote: > Multiple displays may be connected to the same bus and share a D/C GPIO, > so the display driver needs exclusive access to the bus to ensure that > it can control the D/C GPIO safely. > > Signed-off-by: Otto Pflüger > --- Reviewed-by: Noralf Trønnes
Re: [PATCH v2 03/24] drm/gud: use vmalloc_array and vcalloc
On 6/27/23 16:43, Julia Lawall wrote:
> Use vmalloc_array and vcalloc to protect against
> multiplication overflows.
>
> The changes were done using the following Coccinelle
> semantic patch:
>
> //
> @initialize:ocaml@
> @@
>
> let rename alloc =
> match alloc with
> "vmalloc" -> "vmalloc_array"
> | "vzalloc" -> "vcalloc"
> | _ -> failwith "unknown"
>
> @@
> size_t e1,e2;
> constant C1, C2;
> expression E1, E2, COUNT, x1, x2, x3;
> typedef u8;
> typedef __u8;
> type t = {u8,__u8,char,unsigned char};
> identifier alloc = {vmalloc,vzalloc};
> fresh identifier realloc = script:ocaml(alloc) { rename alloc };
> @@
>
> (
> alloc(x1*x2*x3)
> |
> alloc(C1 * C2)
> |
> alloc((sizeof(t)) * (COUNT), ...)
> |
> - alloc((e1) * (e2))
> + realloc(e1, e2)
> |
> - alloc((e1) * (COUNT))
> + realloc(COUNT, e1)
> |
> - alloc((E1) * (E2))
> + realloc(E1, E2)
> )
> //
>
> Signed-off-by: Julia Lawall
>
> ---
Thanks, applied to drm-misc-next.
Noralf.
> v2: Use vmalloc_array and vcalloc instead of array_size.
> This also leaves a multiplication of a constant by a sizeof
> as is. Two patches are thus dropped from the series.
>
> drivers/gpu/drm/gud/gud_pipe.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff -u -p a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -390,7 +390,7 @@ static int gud_fb_queue_damage(struct gu
> mutex_lock(>damage_lock);
>
> if (!gdrm->shadow_buf) {
> - gdrm->shadow_buf = vzalloc(fb->pitches[0] * fb->height);
> + gdrm->shadow_buf = vcalloc(fb->pitches[0], fb->height);
> if (!gdrm->shadow_buf) {
> mutex_unlock(>damage_lock);
> return -ENOMEM;
>
Re: [PATCH v2 3/8] dt-bindings: display: panel: mipi-dbi-spi: add spi-3wire property
On 6/14/23 14:32, Leonard Göhrs wrote: > Some MIPI DBI panels support a three wire mode (clock, chip select, > bidirectional data) that can be used to ask the panel if it is already set > up by e.g. the bootloader and can thus skip the initialization. > This enables a flicker-free boot. > > Signed-off-by: Leonard Göhrs > Acked-by: Conor Dooley > Reviewed-by: Rob Herring > --- Reviewed-by: Noralf Trønnes > .../devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > index c07da1a9e6288..2f0238b770eba 100644 > --- a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > @@ -87,6 +87,8 @@ properties: >Logic level supply for interface signals (Vddi). >No need to set if this is the same as power-supply. > > + spi-3wire: true > + > required: >- compatible >- reg
Re: [PATCH v2 2/8] dt-bindings: display: panel: mipi-dbi-spi: add shineworld lh133k compatible
On 6/14/23 14:32, Leonard Göhrs wrote: > The Shineworld LH133K is a 1.3" 240x240px RGB LCD with a MIPI DBI > compatible SPI interface. > The initialization procedure is quite basic with the exception of > requiring inverted colors. > A basic mipi-dbi-cmd[1] script to get the display running thus looks > like this: > > $ cat shineworld,lh133k.txt > command 0x11 # exit sleep mode > delay 120 > > # The display seems to require display color inversion, so enable it. > command 0x21 # INVON > > # Enable normal display mode (in contrast to partial display mode). > command 0x13 # NORON > command 0x29 # MIPI_DCS_SET_DISPLAY_ON > > $ mipi-dbi-cmd shineworld,lh133k.bin shineworld,lh133k.txt > > [1]: https://github.com/notro/panel-mipi-dbi > > Signed-off-by: Leonard Göhrs > Acked-by: Conor Dooley > --- Normally I would take this trough drm-misc-next but -rc6 is the cutoff so if I do that it won't make it to 6.5. If the other patches make it to 6.5 the dtb checks will fail. I'm okay with the patches going through another tree if that's preferred. Let me know if I should apply the mipi-dbi-spi patches. Reviewed-by: Noralf Trønnes > .../devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml| 1 + > 1 file changed, 1 insertion(+) > > diff --git > a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > index 9b701df5e9d28..c07da1a9e6288 100644 > --- a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > @@ -67,6 +67,7 @@ properties: > items: >- enum: >- sainsmart18 > + - shineworld,lh133k >- const: panel-mipi-dbi-spi > >write-only:
Re: [PATCH v1 2/8] dt-bindings: display: panel: mipi-dbi-spi: add spi-3wire property
On 6/7/23 13:55, Leonard Göhrs wrote: > Some MIPI DBI panels support a three wire mode (clock, chip select, > bidirectional data) that can be used to ask the panel if it is already set > up by e.g. the bootloader and can thus skip the initialization. > This enables a flicker-free boot. > > Signed-off-by: Leonard Göhrs > --- > .../devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git > a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > index c07da1a9e6288..2f0238b770eba 100644 > --- a/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > +++ b/Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml > @@ -87,6 +87,8 @@ properties: >Logic level supply for interface signals (Vddi). >No need to set if this is the same as power-supply. > > + spi-3wire: true > + I don't think this should be added here. spi-cpha and spi-cpol are also supported but they are not mentioned. Instead those are documented in bindings/spi/spi-controller.yaml. Why they're not documented in bindings/spi/spi-peripheral-props.yaml instead which this binding has a ref to, I have no idea. Noralf. > required: >- compatible >- reg
Re: gud: set PATH connector property
On 3/2/23 13:01, Simon Ser wrote:
> On Tuesday, February 28th, 2023 at 16:16, Peter Stuge wrote:
>
>> Simon Ser wrote:
>>
> Would it be possible to set the PATH connector property based on the
> USB port used by gud?
Sadly not really easily.
The physical topology underneath each host controller is stable but
bus numbers (usb1, usb2 etc.) are not.
>>>
>>> Oh, that's news to me. So if I unplug and replug a USB device, the bus
>>> number and bus device number might change?
>>
>> The bus number is stable as long as the bus (host controller) exists.
>>
>>> Or does this happen after a power-cycle? Or is this hardware-specific?
>>
>> Consider a host controller on a plug-in card, like ExpressCard (usb1)
>> and perhaps Thunderbolt (usb2) for a more modern example.
>>
>> The bus on each new host controller gets the next available bus number.
>>
>> Plug ExpressCard before Thunderbolt to get the order above. Unplug
>> both (usb1+usb2 disappear) then plug Thunderbolt back in before
>> ExpressCard; now Thunderbolt is usb1 and ExpressCard usb2.
>
> Hm, right. With a first-come-first-served scheme, there is no way to
> have stable identifiers.
>
> I'm having a look at prior art: udev has similar needs for network
> interface names. For USB they use [2] a scheme with
> port/config/interface. I have no idea what meaning these have, but
> would they be useful for building a PATH KMS property?
>
> [1]:
> https://www.freedesktop.org/software/systemd/man/systemd.net-naming-scheme.html
> [2]:
> https://github.com/systemd/systemd/blob/7a67afe33192ce4a55e6825b80554fb4ebbb4b03/src/udev/udev-builtin-net_id.c#L758
>
I'm no expert but that looks like a good idea, it has probably been well
scrutinized.
Maybe we can do something like this, not tested:
/* PATH=usb:[[P]s[f]]uo */
int drm_connector_set_path_property_usb(struct drm_connector *connector,
struct usb_interface *intf)
{
u8 config = intf->cur_altsetting->desc.bAlternateSetting;
u8 ifnum = intf->cur_altsetting->desc.bInterfaceNumber;
struct usb_device *usb = interface_to_usbdev(intf);
struct device *dev = >dev;
char path[255], temp[64];
strlcpy(path, "usb:", sizeof(path));
while (dev = dev->parent) {
struct pci_dev *pci;
if (dev->bus != _bus_type)
continue;
pci = to_pci_dev(dev);
if (pci_domain_nr(pci->bus)) {
snprintf(temp, sizeof(temp), "P%u",
pci_domain_nr(pci->bus));
strlcat(path, temp, sizeof(path));
}
snprintf(temp, sizeof(temp), "s%u", PCI_SLOT(pci->devfn));
strlcat(path, temp, sizeof(path));
if (pci->multifunction) {
snprintf(temp, sizeof(temp), "f%u",
PCI_FUNC(pci->devfn));
strlcat(path, temp, sizeof(path));
}
break;
}
snprintf(temp, sizeof(temp), "u%s", usb->devpath);
strlcat(path, temp, sizeof(path));
if (config) {
snprintf(temp, sizeof(temp), "c%u", config);
strlcat(path, temp, sizeof(path));
}
if (ifnum) {
snprintf(temp, sizeof(temp), "i%u", ifnum);
strlcat(path, temp, sizeof(path));
}
snprintf(temp, sizeof(temp), "o%s", connector->index);
strlcat(path, temp, sizeof(path));
return drm_connector_set_path_property(connector, path);
}
Noralf.
Re: gud: set PATH connector property
Hi Simon, On 2/28/23 12:36, Simon Ser wrote: > Hi Noralf, > > Would it be possible to set the PATH connector property based on the > USB port used by gud? > > This would give user-space a persistent identifier for the connector: > if the user plugs in a USB display on a given port, the PATH would be > the same even if the machine rebooted or the displays were plugged in > in a different order. > > DP-MST already sets this. User-space can use this property to store > output configuration metadata. > drm_mode_connector_set_path_property() docs: This creates a property to expose to userspace to specify a connector path. This is mainly used for DisplayPort MST where connectors have a topology and we want to allow userspace to give them more meaningful names. Here it says that it's about giving meaningful names to connectors, I think it would be useful to know that a HDMI-A connector is on a USB adapter for instance and not on the laptop/motherboard. You mention output configuration however, why does userspace use the path to a connector when storing the multi display config and not some unique property on the displays themselves like the serial number? Noralf.
Re: Driver for CFAF240320X0-020T display
On 1/5/23 13:57, Fabio Estevam wrote: > Hi Noralf, > > On Fri, Dec 16, 2022 at 9:30 AM Noralf Trønnes wrote: > >> There is a DRM driver that can be used with all of these controllers: >> drivers/gpu/drm/tiny/panel-mipi-dbi.c. It uses a firmware file for the >> init commands. >> >> Binding: >> Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml >> Wiki: https://github.com/notro/panel-mipi-dbi/wiki > > Thanks for your suggestion. > > I was able to get the CFAF240320X0-020T display to work with the > panel-mipi-dbi.c > driver. > > You did a great job on this driver and wiki! > Thanks, glad you could make it work. Noralf.
Re: [PATCH 2/9] drm/gud: use new debugfs device-centered functions
On 12/26/22 16:50, Maíra Canal wrote: > Replace the use of drm_debugfs_create_files() with the new > drm_debugfs_add_file() function, which center the debugfs files > management on the drm_device instead of drm_minor. Moreover, remove the > debugfs_init hook and add the debugfs files directly on gud_probe(), > before drm_dev_register(). > > Signed-off-by: Maíra Canal > --- Acked-by: Noralf Trønnes
Re: Driver for CFAF240320X0-020T display
Den 16.12.2022 12.54, skrev Fabio Estevam: > Hi, > > Does anyone know if the Crystalfontz CFAF240320X0-020T display is > supported in Linux? > > https://www.crystalfontz.com/product/cfaf240320x0020t-2inch-240x320-color-tft > > It uses a Sitronix ST7789V controller. > > For the ST7789V, there is a drm driver: > drivers/gpu/drm/panel/panel-sitronix-st7789v.c > > and also an fbtft one: > drivers/staging/fbtft/fb_st7789v.c > I see they both have the same compatible string, that's unfortunate > Documentation/devicetree/bindings/display/panel/sitronix,st7789v.yaml > shows a remote-endpoint property, so I assume this only works when st7789v > is connected via RGB parallel, correct? > Yes, pixels are sent over MIPI DPI, the init/setup commands are sent over MIPI DBI (SPI). > On my board, the CFAF240320X0-020T is connected via SPI only, so I > guess I should try the fbtdt driver? > Many of these controllers support the MIPI DCS command set and accepts these commands over MIPI DBI (including pixels). Mostly the DBI mode is SPI. fbtft have support for many MIPI DBI compatible controllers and the only thing that differs between them is the initialization commands. In fbtft this can be overridden using the DT property init=. There is a DRM driver that can be used with all of these controllers: drivers/gpu/drm/tiny/panel-mipi-dbi.c. It uses a firmware file for the init commands. Binding: Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yaml Wiki: https://github.com/notro/panel-mipi-dbi/wiki > The arch/riscv/boot/dts/canaan/sipeed_maix_* boards use compatible = > "sitronix,st7789v" > > Do these boards have st7789v functional? Are they using the fbtft or drm > driver? > Looks like they use the fbtft driver since remote-endpoint is not set. Noralf. > Appreciate any suggestions. > > Thanks, > > Fabio Estevam
Re: [PATCH v2 0/3] drm/tiny: panel-mipi-dbi: Support separate I/O voltage supply
Den 01.12.2022 17.02, skrev Otto Pflüger: > As stated in > Documentation/devicetree/bindings/display/panel/panel-mipi-dbi-spi.yml, > the MIPI DBI specification defines two power supplies, one for powering > the panel and one for the I/O voltage. The panel-mipi-dbi driver > currently only supports specifying a single "power-supply" in the > device tree. > > Add support for a second power supply defined in a new "io-supply" > device tree property to make the driver properly configure the voltage > regulators on platforms where separate supplies are used. > > Changes in v2: > - Don't list power-supply in the properties section of >panel-mipi-dbi-spi.yaml because it is already in panel-common.yaml > > Otto Pflüger (3): > drm/mipi-dbi: Support separate I/O regulator > drm/tiny: panel-mipi-dbi: Read I/O supply from DT > dt-bindings: display: panel: mipi-dbi-spi: Add io-supply > Series applied to drm-misc-next. Noralf.
Re: [PATCH] drm/gud: Fix missing include
Den 08.12.2022 09.15, skrev Thomas Zimmermann:
> Hi
>
> Am 07.12.22 um 20:51 schrieb Noralf Trønnes via B4 Submission Endpoint:
>> From: Noralf Trønnes
>>
>> Add missing vmalloc.h include.
>>
>> Fixes: c17d048609bf ("drm/gud: Use the shadow plane helper")
>> Reported-by: kernel test robot
>> Signed-off-by: Noralf Trønnes
>
> Reviewed-by: Thomas Zimmermann
>
Thanks.
> The missing-vmalloc() error is a real classic. :( Some architectures
> declare the function and some don't.
>
Yeah, I think I've seen this before. Good we have the bots to cover the
corner cases.
Noralf.
> Best regards
> Thomas
>
>> ---
>> drivers/gpu/drm/gud/gud_pipe.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/gpu/drm/gud/gud_pipe.c
>> b/drivers/gpu/drm/gud/gud_pipe.c
>> index 62c43d3632d4..dc16a92625d4 100644
>> --- a/drivers/gpu/drm/gud/gud_pipe.c
>> +++ b/drivers/gpu/drm/gud/gud_pipe.c
>> @@ -5,6 +5,7 @@
>> #include
>> #include
>> +#include
>> #include
>> #include
>>
>> ---
>> base-commit: 5ad8e63ebba3d5a0730b43180b200e41eeb9409c
>> change-id: 20221207-gud-missing-include-9ccf56382f8a
>>
>> Best regards,
>
[PATCH] drm/gud: Fix missing include
From: Noralf Trønnes
Add missing vmalloc.h include.
Fixes: c17d048609bf ("drm/gud: Use the shadow plane helper")
Reported-by: kernel test robot
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/gud/gud_pipe.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 62c43d3632d4..dc16a92625d4 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -5,6 +5,7 @@
#include
#include
+#include
#include
#include
---
base-commit: 5ad8e63ebba3d5a0730b43180b200e41eeb9409c
change-id: 20221207-gud-missing-include-9ccf56382f8a
Best regards,
--
Noralf Trønnes
Re: [PATCH v2 0/6] drm/gud: Use the shadow plane helper
Den 30.11.2022 20.26, skrev Noralf Trønnes via B4 Submission Endpoint: > Hi, > > I have started to look at igt for testing and want to use CRC tests. To > implement support for this I need to move away from the simple kms > helper. > > When looking around for examples I came across Thomas' nice shadow > helper and thought, yes this is perfect for drm/gud. So I'll switch to > that before I move away from the simple kms helper. > > The async framebuffer flushing code path now uses a shadow buffer and > doesn't touch the framebuffer when it shouldn't. I have also taken the > opportunity to inline the synchronous flush code path and make this the > default flushing stategy. > > Noralf. > > Cc: Maxime Ripard > Cc: Thomas Zimmermann > Cc: dri-devel@lists.freedesktop.org > Signed-off-by: Noralf Trønnes > > --- > Changes in v2: > - Drop patch (Thomas): > drm/gem: shadow_fb_access: Prepare imported buffers for CPU access > - Use src as variable name for iosys_map (Thomas) > - Prepare imported buffer for CPU access in the driver (Thomas) > - New patch: make sync flushing the default (Thomas) > - Link to v1: > https://lore.kernel.org/r/20221122-gud-shadow-plane-v1-0-9de3afa33...@tronnes.org > > --- > Noralf Trønnes (6): > drm/gud: Fix UBSAN warning > drm/gud: Don't retry a failed framebuffer flush > drm/gud: Split up gud_flush_work() > drm/gud: Prepare buffer for CPU access in gud_flush_work() > drm/gud: Use the shadow plane helper > drm/gud: Enable synchronous flushing by default > Applied to drm-misc-next, thanks for reviewing! Noralf.
Re: [PATCH v2 3/3] dt-bindings: display: panel: mipi-dbi-spi: Add io-supply
Den 01.12.2022 17.02, skrev Otto Pflüger: > Add documentation for the new io-supply property, which specifies the > regulator for the I/O voltage supply on platforms where the panel > panel power and I/O supplies are separate. > > Signed-off-by: Otto Pflüger > --- Reviewed-by: Noralf Trønnes
Re: [PATCH v2 2/3] drm/tiny: panel-mipi-dbi: Read I/O supply from DT
Den 01.12.2022 17.02, skrev Otto Pflüger: > To support platforms with a separate I/O voltage supply, set the new > io_regulator property along with the regulator property of the DBI > device. Read the I/O supply from a new "io-supply" device tree > property. > > Signed-off-by: Otto Pflüger > --- Reviewed-by: Noralf Trønnes
Re: [PATCH v2 1/3] drm/mipi-dbi: Support separate I/O regulator
Den 01.12.2022 17.02, skrev Otto Pflüger: > The MIPI DBI specification defines separate vdd (panel power) and > vddi (I/O voltage) supplies. Displays that require different voltages > for the different supplies do exist, so the supplies cannot be > combined into one as they are now. Add a new io_regulator property to > the mipi_dbi_dev struct which can be set by the panel driver along > with the regulator property. > > Signed-off-by: Otto Pflüger > --- Thanks for fixing this, I'll apply the patches in a few days. Reviewed-by: Noralf Trønnes
Re: [PATCH v2 6/8] drm/mipi-dbi: Support shadow-plane state
Den 02.12.2022 13.56, skrev Thomas Zimmermann:
> Implement MIPI DBI planes with struct drm_shadow_plane_state, so that the
> respective drivers can use the vmap'ed GEM-buffer memory. Implement state
> helpers, the {begin,end}_fb_access helpers and wire up everything.
>
> With this commit, MIPI DBI drivers can access the GEM object's memory
> that is provided by shadow-plane state. The actual changes to drivers
> are implemented separately.
>
> v2:
> * use shadow-plane state directly (Noralf)
>
> Signed-off-by: Thomas Zimmermann
> Tested-by: Javier Martinez Canillas
> Tested-by: Noralf Trønnes # drm/tiny/mi0283qt
> ---
Reviewed-by: Noralf Trønnes
Re: [PATCH v2 6/8] drm/mipi-dbi: Support shadow-plane state
Den 02.12.2022 13.56, skrev Thomas Zimmermann:
> Implement MIPI DBI planes with struct drm_shadow_plane_state, so that the
> respective drivers can use the vmap'ed GEM-buffer memory. Implement state
> helpers, the {begin,end}_fb_access helpers and wire up everything.
>
> With this commit, MIPI DBI drivers can access the GEM object's memory
> that is provided by shadow-plane state. The actual changes to drivers
> are implemented separately.
>
> v2:
> * use shadow-plane state directly (Noralf)
>
> Signed-off-by: Thomas Zimmermann
> Tested-by: Javier Martinez Canillas
> Tested-by: Noralf Trønnes # drm/tiny/mi0283qt
> ---
> drivers/gpu/drm/drm_mipi_dbi.c | 85 ++
> drivers/gpu/drm/tiny/ili9225.c | 5 ++
> drivers/gpu/drm/tiny/st7586.c | 5 ++
> include/drm/drm_mipi_dbi.h | 16 ++-
> 4 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> index f58123327ed6..b808de61c5bc 100644
> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> @@ -21,6 +21,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -433,6 +434,90 @@ void mipi_dbi_pipe_disable(struct
> drm_simple_display_pipe *pipe)
> }
> EXPORT_SYMBOL(mipi_dbi_pipe_disable);
>
> +/**
> + * mipi_dbi_pipe_begin_fb_access - MIPI DBI pipe begin-access helper
> + * @pipe: Display pipe
> + * @plane_state: Plane state
> + *
> + * This function implements struct _simple_display_funcs.begin_fb_access.
> + *
> + * See drm_gem_begin_shadow_fb_access() for details and
> mipi_dbi_pipe_cleanup_fb()
> + * for cleanup.
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
> + */
> +int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
> + struct drm_plane_state *plane_state)
> +{
> + return drm_gem_begin_shadow_fb_access(>plane, plane_state);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_begin_fb_access);
> +
> +/**
> + * mipi_dbi_pipe_end_fb_access - MIPI DBI pipe end-access helper
> + * @pipe: Display pipe
> + * @plane_state: Plane state
> + *
> + * This function implements struct _simple_display_funcs.end_fb_access.
> + *
> + * See mipi_dbi_pipe_begin_fb_access().
> + */
> +void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
> + struct drm_plane_state *plane_state)
> +{
> + drm_gem_end_shadow_fb_access(>plane, plane_state);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_end_fb_access);
> +
> +/**
> + * mipi_dbi_pipe_reset_plane - MIPI DBI plane-reset helper
> + * @pipe: Display pipe
> + *
> + * This function implements struct _simple_display_funcs.reset_plane
> + * for MIPI DBI planes.
> + */
> +void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe)
> +{
> + drm_gem_reset_shadow_plane(>plane);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_reset_plane);
> +
> +/**
> + * mipi_dbi_pipe_duplicate_plane_state - duplicates MIPI DBI plane state
> + * @pipe: Display pipe
> + *
> + * This function implements struct
> _simple_display_funcs.duplicate_plane_state
> + * for MIPI DBI planes.
> + *
> + * See drm_gem_duplicate_shadow_plane_state() for additional details.
> + *
> + * Returns:
> + * A pointer to a new plane state on success, or NULL otherwise.
> + */
> +struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct
> drm_simple_display_pipe *pipe)
> +{
> + return drm_gem_duplicate_shadow_plane_state(>plane);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_duplicate_plane_state);
> +
> +/**
> + * mipi_dbi_pipe_destroy_plane_state - cleans up MIPI DBI plane state
> + * @pipe: Display pipe
> + * @plane_state: Plane state
> + *
> + * This function implements struct
> drm_simple_display_funcs.destroy_plane_state
> + * for MIPI DBI planes.
> + *
> + * See drm_gem_destroy_shadow_plane_state() for additional details.
> + */
> +void mipi_dbi_pipe_destroy_plane_state(struct drm_simple_display_pipe *pipe,
> +struct drm_plane_state *plane_state)
> +{
> + drm_gem_destroy_shadow_plane_state(>plane, plane_state);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_destroy_plane_state);
> +
> static int mipi_dbi_connector_get_modes(struct drm_connector *connector)
> {
> struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(connector->dev);
> diff --git a/drivers/gpu/drm/tiny/ili9225.c b/drivers/gpu/drm/tiny/ili9225.c
> index ae94c74d0163..a69aec8402bc 100644
> --- a/drivers/gpu/drm/tiny/ili9225.c
> +++ b/drivers/gpu/drm/tin
Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers
Den 02.12.2022 12.50, skrev Thomas Zimmermann: > >>> >>> You use drm_gem_fb_vmap() in the other places but here you access the >>> object directly (and in the next hunk), but again not so important since >>> it goes away in a later patch. >> >> I'll update this patch to use drm_gem_fb_vmap() consistently. > > And after looking at the impact and churn, I rather go with the existing > code that initializes from the GEM DMA object. > > Noralf, is there a reason why most of MIPI DBI uses DMA helpers? In > terms of flexibility and resource consumption, wouldn't SHMEM helpers be > a better fit? > The SHMEM helper didn't exist at the time. The SPI subsystem doesn't have an interface for scatter/gather transfers and DMA is needed in order to run at full speed. SPI does convert an is_vmalloc_addr() buffer to an sg list of pages in spi_map_buf() so it solves the missing interface that way. I have never tried to pass a shmem buffer to spi_sync() so I don't know if it works, but I guess that it will work. Bare in mind that theses buffers are at most 400k in size so I'm not sure there's much to gain in term of resources at least. Noralf.
Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers
Den 02.12.2022 12.27, skrev Thomas Zimmermann:
> Hi
>
> Am 25.11.22 um 18:39 schrieb Noralf Trønnes:
>>
>>
>> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>>> Move the vmap/vunmap blocks from the inner fb_dirty helpers into the
>>> MIPI DBI update helpers. The function calls can result in waiting and/or
>>> processing overhead. Reduce the penalties by executing the functions
>>> once
>>> in the outer-most function of the pipe update.
>>>
>>> This change also prepares for MIPI DBI for shadow-plane helpers. With
>>> shadow-plane helpers, transfer source buffers are mapped into kernel
>>> address space automatically.
>>>
>>> Signed-off-by: Thomas Zimmermann
>>> ---
>>> @@ -303,9 +291,6 @@ static void mipi_dbi_fb_dirty(struct
>>> drm_framebuffer *fb, struct drm_rect *rect)
>>> if (ret)
>>> drm_err_once(fb->dev, "Failed to update display %d\n", ret);
>>> - drm_gem_fb_vunmap(fb, map);
>>> -
>>> -err_drm_dev_exit:
>>> drm_dev_exit(idx);
>>> }
>>> @@ -338,14 +323,27 @@ EXPORT_SYMBOL(mipi_dbi_pipe_mode_valid);
>>> void mipi_dbi_pipe_update(struct drm_simple_display_pipe *pipe,
>>> struct drm_plane_state *old_state)
>>> {
>>> + struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>>> + struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>>
>> These should have been zeroed to avoid UBSAN complaint, but you'll
>> remove these later so not so important.
>
> Will be fixed.
>
> But do you know how these warnings happen? I went through the helpers
> before and changed them to only access the format-info's relevant
> planes. No unintialized memory access should be reported.
>
It happens with imported buffers, iosys_map_clear() looks at the
uninitialized boolean variable ->is_iomem:
drm_gem_fb_vmap -> ... -> dma_buf_vmap -> iosys_map_clear
static inline void iosys_map_clear(struct iosys_map *map)
{
if (map->is_iomem) {
map->vaddr_iomem = NULL;
map->is_iomem = false;
} else {
map->vaddr = NULL;
}
}
Noralf.
Re: [PATCH v2 0/6] drm/gud: Use the shadow plane helper
Den 01.12.2022 15.16, skrev Konstantin Ryabitsev: > On Thu, Dec 01, 2022 at 02:34:41PM +0100, Javier Martinez Canillas wrote: Konstantin, Can you add a rule in b4 to exclude sta...@vger.kernel.org (sta...@kernel.org as well?) from getting the whole patchset? >>> >>> sta...@kernel.org is a pipe to /dev/null so that's not needed to be >>> messed with. >>> >>> As for this needing special casing in b4, it's rare that you send out a >>> patch series and only want 1 or 2 of them in stable, right? >>> >> >> Not really, it's very common for a patch-series to contain fixes (that could >> go to stable if applicable) and change that are not suitable for stable. The >> problem as Noralf mentioned is that the b4 tool doesn't seem to allow Cc'ing >> individual patches to different recipients, and everyone get the whole set. >> >> So either b4 needs to have this support, exclude sta...@vger.kernel.org when >> sending a set or sta...@vger.kernel.org ignore patches without a Fixes: tag. > > I think what I can do is a special logic for Cc: trailers: > > - Any Cc: trailers we find in the cover letter receive the entire series > - Any Cc: trailers in individual patches only receive these individual patches > That should cover my use cases. I can now do 'b4 prep --auto-to-cc' and then trim down the cc list in the cover letter if necessary. > Thank you for being patient -- we'll get this right, I promise. > Thanks for getting it right. b4 can replace parts of my own tooling and do it smoother so I think I'll continue to use it. Noralf.
Re: [PATCH v2 0/6] drm/gud: Use the shadow plane helper
Den 01.12.2022 13.12, skrev Greg KH: > On Thu, Dec 01, 2022 at 11:00:44AM +0100, Noralf Trønnes wrote: >> >> >> Den 01.12.2022 06.55, skrev Greg KH: >>> On Wed, Nov 30, 2022 at 08:26:48PM +0100, Noralf Trønnes via B4 Submission >>> Endpoint wrote: >>>> Hi, >>>> >>>> I have started to look at igt for testing and want to use CRC tests. To >>>> implement support for this I need to move away from the simple kms >>>> helper. >>>> >>>> When looking around for examples I came across Thomas' nice shadow >>>> helper and thought, yes this is perfect for drm/gud. So I'll switch to >>>> that before I move away from the simple kms helper. >>>> >>>> The async framebuffer flushing code path now uses a shadow buffer and >>>> doesn't touch the framebuffer when it shouldn't. I have also taken the >>>> opportunity to inline the synchronous flush code path and make this the >>>> default flushing stategy. >>>> >>>> Noralf. >>>> >>>> Cc: Maxime Ripard >>>> Cc: Thomas Zimmermann >>>> Cc: dri-devel@lists.freedesktop.org >>>> Signed-off-by: Noralf Trønnes >>>> >>>> --- >>>> Changes in v2: >>>> - Drop patch (Thomas): >>>> drm/gem: shadow_fb_access: Prepare imported buffers for CPU access >>>> - Use src as variable name for iosys_map (Thomas) >>>> - Prepare imported buffer for CPU access in the driver (Thomas) >>>> - New patch: make sync flushing the default (Thomas) >>>> - Link to v1: >>>> https://lore.kernel.org/r/20221122-gud-shadow-plane-v1-0-9de3afa33...@tronnes.org >>> >>> >>> >>> This is not the correct way to submit patches for inclusion in the >>> stable kernel tree. Please read: >>> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html >>> for how to do this properly. >>> >>> >> >> Care to elaborate? >> Is it because stable got the whole patchset and not just the one fix >> patch that cc'ed stable? > > That is what triggered this, yes. > >> This patchset was sent using the b4 tool and I can't control this >> aspect. Everyone mentioned in the patches gets the whole set. > > Fair enough, but watch out, bots will report this as being a problem as > they can't always read through all patches in a series to notice this... > Konstantin, Can you add a rule in b4 to exclude sta...@vger.kernel.org (sta...@kernel.org as well?) from getting the whole patchset? Noralf.
Re: [PATCH v2 0/6] drm/gud: Use the shadow plane helper
Den 01.12.2022 06.55, skrev Greg KH: > On Wed, Nov 30, 2022 at 08:26:48PM +0100, Noralf Trønnes via B4 Submission > Endpoint wrote: >> Hi, >> >> I have started to look at igt for testing and want to use CRC tests. To >> implement support for this I need to move away from the simple kms >> helper. >> >> When looking around for examples I came across Thomas' nice shadow >> helper and thought, yes this is perfect for drm/gud. So I'll switch to >> that before I move away from the simple kms helper. >> >> The async framebuffer flushing code path now uses a shadow buffer and >> doesn't touch the framebuffer when it shouldn't. I have also taken the >> opportunity to inline the synchronous flush code path and make this the >> default flushing stategy. >> >> Noralf. >> >> Cc: Maxime Ripard >> Cc: Thomas Zimmermann >> Cc: dri-devel@lists.freedesktop.org >> Signed-off-by: Noralf Trønnes >> >> --- >> Changes in v2: >> - Drop patch (Thomas): >> drm/gem: shadow_fb_access: Prepare imported buffers for CPU access >> - Use src as variable name for iosys_map (Thomas) >> - Prepare imported buffer for CPU access in the driver (Thomas) >> - New patch: make sync flushing the default (Thomas) >> - Link to v1: >> https://lore.kernel.org/r/20221122-gud-shadow-plane-v1-0-9de3afa33...@tronnes.org > > > > This is not the correct way to submit patches for inclusion in the > stable kernel tree. Please read: > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html > for how to do this properly. > > Care to elaborate? Is it because stable got the whole patchset and not just the one fix patch that cc'ed stable? This patchset was sent using the b4 tool and I can't control this aspect. Everyone mentioned in the patches gets the whole set. Noralf.
[PATCH v2 6/6] drm/gud: Enable synchronous flushing by default
From: Noralf Trønnes gud has a module parameter that controls whether framebuffer flushing happens synchronously during the commit or asynchronously in a worker. GNOME before version 3.38 handled all displays in the same rendering loop. This lead to gud slowing down the refresh rate for a faster monitor. This has now been fixed so lets change the default. The plan is to remove async flushing in the future. The code is now structured in a way that makes it easy to do this. Link: https://blogs.gnome.org/shell-dev/2020/07/02/splitting-up-the-frame-clock/ Suggested-by: Thomas Zimmermann Signed-off-by: Noralf Trønnes --- drivers/gpu/drm/gud/gud_pipe.c | 12 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c index 92189474a7ed..62c43d3632d4 100644 --- a/drivers/gpu/drm/gud/gud_pipe.c +++ b/drivers/gpu/drm/gud/gud_pipe.c @@ -25,17 +25,13 @@ #include "gud_internal.h" /* - * Some userspace rendering loops runs all displays in the same loop. + * Some userspace rendering loops run all displays in the same loop. * This means that a fast display will have to wait for a slow one. - * For this reason gud does flushing asynchronous by default. - * The down side is that in e.g. a single display setup userspace thinks - * the display is insanely fast since the driver reports back immediately - * that the flush/pageflip is done. This wastes CPU and power. - * Such users might want to set this module parameter to false. + * Such users might want to enable this module parameter. */ -static bool gud_async_flush = true; +static bool gud_async_flush; module_param_named(async_flush, gud_async_flush, bool, 0644); -MODULE_PARM_DESC(async_flush, "Enable asynchronous flushing [default=true]"); +MODULE_PARM_DESC(async_flush, "Enable asynchronous flushing [default=0]"); /* * FIXME: The driver is probably broken on Big Endian machines. -- 2.34.1
[PATCH v2 3/6] drm/gud: Split up gud_flush_work()
From: Noralf Trønnes
In preparation for inlining synchronous flushing split out the part of
gud_flush_work() that can be shared by the sync and async code paths.
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/gud/gud_pipe.c | 72 +++---
1 file changed, 39 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index ff1358815af5..d2af9947494f 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -333,15 +333,49 @@ void gud_clear_damage(struct gud_device *gdrm)
gdrm->damage.y2 = 0;
}
+static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer
*fb,
+struct drm_rect *damage)
+{
+ const struct drm_format_info *format;
+ unsigned int i, lines;
+ size_t pitch;
+ int ret;
+
+ format = fb->format;
+ if (format->format == DRM_FORMAT_XRGB &&
gdrm->xrgb_emulation_format)
+ format = gdrm->xrgb_emulation_format;
+
+ /* Split update if it's too big */
+ pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(damage));
+ lines = drm_rect_height(damage);
+
+ if (gdrm->bulk_len < lines * pitch)
+ lines = gdrm->bulk_len / pitch;
+
+ for (i = 0; i < DIV_ROUND_UP(drm_rect_height(damage), lines); i++) {
+ struct drm_rect rect = *damage;
+
+ rect.y1 += i * lines;
+ rect.y2 = min_t(u32, rect.y1 + lines, damage->y2);
+
+ ret = gud_flush_rect(gdrm, fb, format, );
+ if (ret) {
+ if (ret != -ENODEV && ret != -ECONNRESET &&
+ ret != -ESHUTDOWN && ret != -EPROTO)
+ dev_err_ratelimited(fb->dev->dev,
+ "Failed to flush
framebuffer: error=%d\n", ret);
+ gdrm->prev_flush_failed = true;
+ break;
+ }
+ }
+}
+
void gud_flush_work(struct work_struct *work)
{
struct gud_device *gdrm = container_of(work, struct gud_device, work);
- const struct drm_format_info *format;
struct drm_framebuffer *fb;
struct drm_rect damage;
- unsigned int i, lines;
- int idx, ret = 0;
- size_t pitch;
+ int idx;
if (!drm_dev_enter(>drm, ))
return;
@@ -356,35 +390,7 @@ void gud_flush_work(struct work_struct *work)
if (!fb)
goto out;
- format = fb->format;
- if (format->format == DRM_FORMAT_XRGB &&
gdrm->xrgb_emulation_format)
- format = gdrm->xrgb_emulation_format;
-
- /* Split update if it's too big */
- pitch = drm_format_info_min_pitch(format, 0, drm_rect_width());
- lines = drm_rect_height();
-
- if (gdrm->bulk_len < lines * pitch)
- lines = gdrm->bulk_len / pitch;
-
- for (i = 0; i < DIV_ROUND_UP(drm_rect_height(), lines); i++) {
- struct drm_rect rect = damage;
-
- rect.y1 += i * lines;
- rect.y2 = min_t(u32, rect.y1 + lines, damage.y2);
-
- ret = gud_flush_rect(gdrm, fb, format, );
- if (ret) {
- if (ret != -ENODEV && ret != -ECONNRESET &&
- ret != -ESHUTDOWN && ret != -EPROTO)
- dev_err_ratelimited(fb->dev->dev,
- "Failed to flush
framebuffer: error=%d\n", ret);
- gdrm->prev_flush_failed = true;
- break;
- }
-
- gdrm->prev_flush_failed = false;
- }
+ gud_flush_damage(gdrm, fb, );
drm_framebuffer_put(fb);
out:
--
2.34.1
[PATCH v2 4/6] drm/gud: Prepare buffer for CPU access in gud_flush_work()
From: Noralf Trønnes
In preparation for moving to the shadow plane helper prepare the
framebuffer for CPU access as early as possible.
v2:
- Use src as variable name for iosys_map (Thomas)
Reviewed-by: Javier Martinez Canillas
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/gud/gud_pipe.c | 67 +-
1 file changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index d2af9947494f..98fe8efda4a9 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -152,32 +153,21 @@ static size_t gud_xrgb_to_color(u8 *dst, const struct
drm_format_info *forma
}
static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
+ const struct iosys_map *src, bool cached_reads,
const struct drm_format_info *format, struct drm_rect
*rect,
struct gud_set_buffer_req *req)
{
- struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
u8 compression = gdrm->compression;
- struct iosys_map map[DRM_FORMAT_MAX_PLANES] = { };
- struct iosys_map map_data[DRM_FORMAT_MAX_PLANES] = { };
struct iosys_map dst;
void *vaddr, *buf;
size_t pitch, len;
- int ret = 0;
pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(rect));
len = pitch * drm_rect_height(rect);
if (len > gdrm->bulk_len)
return -E2BIG;
- ret = drm_gem_fb_vmap(fb, map, map_data);
- if (ret)
- return ret;
-
- vaddr = map_data[0].vaddr;
-
- ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
- if (ret)
- goto vunmap;
+ vaddr = src[0].vaddr;
retry:
if (compression)
buf = gdrm->compress_buf;
@@ -192,29 +182,27 @@ static int gud_prep_flush(struct gud_device *gdrm, struct
drm_framebuffer *fb,
if (format != fb->format) {
if (format->format == GUD_DRM_FORMAT_R1) {
len = gud_xrgb_to_r124(buf, format, vaddr, fb,
rect);
- if (!len) {
- ret = -ENOMEM;
- goto end_cpu_access;
- }
+ if (!len)
+ return -ENOMEM;
} else if (format->format == DRM_FORMAT_R8) {
- drm_fb_xrgb_to_gray8(, NULL, map_data, fb,
rect);
+ drm_fb_xrgb_to_gray8(, NULL, src, fb, rect);
} else if (format->format == DRM_FORMAT_RGB332) {
- drm_fb_xrgb_to_rgb332(, NULL, map_data, fb,
rect);
+ drm_fb_xrgb_to_rgb332(, NULL, src, fb, rect);
} else if (format->format == DRM_FORMAT_RGB565) {
- drm_fb_xrgb_to_rgb565(, NULL, map_data, fb,
rect,
+ drm_fb_xrgb_to_rgb565(, NULL, src, fb, rect,
gud_is_big_endian());
} else if (format->format == DRM_FORMAT_RGB888) {
- drm_fb_xrgb_to_rgb888(, NULL, map_data, fb,
rect);
+ drm_fb_xrgb_to_rgb888(, NULL, src, fb, rect);
} else {
len = gud_xrgb_to_color(buf, format, vaddr, fb,
rect);
}
} else if (gud_is_big_endian() && format->cpp[0] > 1) {
- drm_fb_swab(, NULL, map_data, fb, rect, !import_attach);
- } else if (compression && !import_attach && pitch == fb->pitches[0]) {
+ drm_fb_swab(, NULL, src, fb, rect, cached_reads);
+ } else if (compression && cached_reads && pitch == fb->pitches[0]) {
/* can compress directly from the framebuffer */
buf = vaddr + rect->y1 * pitch;
} else {
- drm_fb_memcpy(, NULL, map_data, fb, rect);
+ drm_fb_memcpy(, NULL, src, fb, rect);
}
memset(req, 0, sizeof(*req));
@@ -237,12 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct
drm_framebuffer *fb,
req->compressed_length = cpu_to_le32(complen);
}
-end_cpu_access:
- drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-vunmap:
- drm_gem_fb_vunmap(fb, map);
-
- return ret;
+ return 0;
}
struct gud_usb_bulk_context {
@@ -285,6 +268,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t len)
}
static int gud_flush_rect(struct gud_device *gdrm, struct drm_framebuffer *fb,
+ const struct iosys_map *src, bool cached_reads,
const struct drm_format_in
[PATCH v2 1/6] drm/gud: Fix UBSAN warning
From: Noralf Trønnes
UBSAN complains about invalid value for bool:
[ 101.165172] [drm] Initialized gud 1.0.0 20200422 for 2-3.2:1.0 on minor 1
[ 101.213360] gud 2-3.2:1.0: [drm] fb1: guddrmfb frame buffer device
[ 101.213426] usbcore: registered new interface driver gud
[ 101.989431]
[ 101.989441] UBSAN: invalid-load in linux/include/linux/iosys-map.h:253:9
[ 101.989447] load of value 121 is not a valid value for type '_Bool'
[ 101.989451] CPU: 1 PID: 455 Comm: kworker/1:6 Not tainted
5.18.0-rc5-gud-5.18-rc5 #3
[ 101.989456] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS
L71 Ver. 01.44 04/12/2018
[ 101.989459] Workqueue: events_long gud_flush_work [gud]
[ 101.989471] Call Trace:
[ 101.989474]
[ 101.989479] dump_stack_lvl+0x49/0x5f
[ 101.989488] dump_stack+0x10/0x12
[ 101.989493] ubsan_epilogue+0x9/0x3b
[ 101.989498] __ubsan_handle_load_invalid_value.cold+0x44/0x49
[ 101.989504] dma_buf_vmap.cold+0x38/0x3d
[ 101.989511] ? find_busiest_group+0x48/0x300
[ 101.989520] drm_gem_shmem_vmap+0x76/0x1b0 [drm_shmem_helper]
[ 101.989528] drm_gem_shmem_object_vmap+0x9/0xb [drm_shmem_helper]
[ 101.989535] drm_gem_vmap+0x26/0x60 [drm]
[ 101.989594] drm_gem_fb_vmap+0x47/0x150 [drm_kms_helper]
[ 101.989630] gud_prep_flush+0xc1/0x710 [gud]
[ 101.989639] ? _raw_spin_lock+0x17/0x40
[ 101.989648] gud_flush_work+0x1e0/0x430 [gud]
[ 101.989653] ? __switch_to+0x11d/0x470
[ 101.989664] process_one_work+0x21f/0x3f0
[ 101.989673] worker_thread+0x200/0x3e0
[ 101.989679] ? rescuer_thread+0x390/0x390
[ 101.989684] kthread+0xfd/0x130
[ 101.989690] ? kthread_complete_and_exit+0x20/0x20
[ 101.989696] ret_from_fork+0x22/0x30
[ 101.989706]
[ 101.989708]
The source of this warning is in iosys_map_clear() called from
dma_buf_vmap(). It conditionally sets values based on map->is_iomem. The
iosys_map variables are allocated uninitialized on the stack leading to
->is_iomem having all kinds of values and not only 0/1.
Fix this by zeroing the iosys_map variables.
Fixes: 40e1a70b4aed ("drm: Add GUD USB Display driver")
Cc: # v5.18+
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/gud/gud_pipe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 7c6dc2bcd14a..61f4abaf1811 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -157,8 +157,8 @@ static int gud_prep_flush(struct gud_device *gdrm, struct
drm_framebuffer *fb,
{
struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
u8 compression = gdrm->compression;
- struct iosys_map map[DRM_FORMAT_MAX_PLANES];
- struct iosys_map map_data[DRM_FORMAT_MAX_PLANES];
+ struct iosys_map map[DRM_FORMAT_MAX_PLANES] = { };
+ struct iosys_map map_data[DRM_FORMAT_MAX_PLANES] = { };
struct iosys_map dst;
void *vaddr, *buf;
size_t pitch, len;
--
2.34.1
[PATCH v2 5/6] drm/gud: Use the shadow plane helper
From: Noralf Trønnes
Use the shadow plane helper to take care of mapping the framebuffer for
CPU access. The synchronous flushing is now done inline without the use of
a worker. The async path now uses a shadow buffer to hold framebuffer
changes and it doesn't read the framebuffer behind userspace's back
anymore.
v2:
- Use src as variable name for iosys_map (Thomas)
- Prepare imported buffer for CPU access in the driver (Thomas)
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/gud/gud_drv.c | 1 +
drivers/gpu/drm/gud/gud_internal.h | 1 +
drivers/gpu/drm/gud/gud_pipe.c | 81 ++
3 files changed, 57 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
index d57dab104358..5aac7cda0505 100644
--- a/drivers/gpu/drm/gud/gud_drv.c
+++ b/drivers/gpu/drm/gud/gud_drv.c
@@ -365,6 +365,7 @@ static void gud_debugfs_init(struct drm_minor *minor)
static const struct drm_simple_display_pipe_funcs gud_pipe_funcs = {
.check = gud_pipe_check,
.update = gud_pipe_update,
+ DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS
};
static const struct drm_mode_config_funcs gud_mode_config_funcs = {
diff --git a/drivers/gpu/drm/gud/gud_internal.h
b/drivers/gpu/drm/gud/gud_internal.h
index e351a1f1420d..0d148a6f27aa 100644
--- a/drivers/gpu/drm/gud/gud_internal.h
+++ b/drivers/gpu/drm/gud/gud_internal.h
@@ -43,6 +43,7 @@ struct gud_device {
struct drm_framebuffer *fb;
struct drm_rect damage;
bool prev_flush_failed;
+ void *shadow_buf;
};
static inline struct gud_device *to_gud_device(struct drm_device *drm)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 98fe8efda4a9..92189474a7ed 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -358,10 +358,10 @@ static void gud_flush_damage(struct gud_device *gdrm,
struct drm_framebuffer *fb
void gud_flush_work(struct work_struct *work)
{
struct gud_device *gdrm = container_of(work, struct gud_device, work);
- struct iosys_map gem_map = { }, fb_map = { };
+ struct iosys_map shadow_map;
struct drm_framebuffer *fb;
struct drm_rect damage;
- int idx, ret;
+ int idx;
if (!drm_dev_enter(>drm, ))
return;
@@ -369,6 +369,7 @@ void gud_flush_work(struct work_struct *work)
mutex_lock(>damage_lock);
fb = gdrm->fb;
gdrm->fb = NULL;
+ iosys_map_set_vaddr(_map, gdrm->shadow_buf);
damage = gdrm->damage;
gud_clear_damage(gdrm);
mutex_unlock(>damage_lock);
@@ -376,33 +377,33 @@ void gud_flush_work(struct work_struct *work)
if (!fb)
goto out;
- ret = drm_gem_fb_vmap(fb, _map, _map);
- if (ret)
- goto fb_put;
+ gud_flush_damage(gdrm, fb, _map, true, );
- ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
- if (ret)
- goto vunmap;
-
- /* Imported buffers are assumed to be WriteCombined with uncached reads
*/
- gud_flush_damage(gdrm, fb, _map, !fb->obj[0]->import_attach,
);
-
- drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-vunmap:
- drm_gem_fb_vunmap(fb, _map);
-fb_put:
drm_framebuffer_put(fb);
out:
drm_dev_exit(idx);
}
-static void gud_fb_queue_damage(struct gud_device *gdrm, struct
drm_framebuffer *fb,
- struct drm_rect *damage)
+static int gud_fb_queue_damage(struct gud_device *gdrm, struct drm_framebuffer
*fb,
+ const struct iosys_map *src, struct drm_rect
*damage)
{
struct drm_framebuffer *old_fb = NULL;
+ struct iosys_map shadow_map;
mutex_lock(>damage_lock);
+ if (!gdrm->shadow_buf) {
+ gdrm->shadow_buf = vzalloc(fb->pitches[0] * fb->height);
+ if (!gdrm->shadow_buf) {
+ mutex_unlock(>damage_lock);
+ return -ENOMEM;
+ }
+ }
+
+ iosys_map_set_vaddr(_map, gdrm->shadow_buf);
+ iosys_map_incr(_map, drm_fb_clip_offset(fb->pitches[0],
fb->format, damage));
+ drm_fb_memcpy(_map, fb->pitches, src, fb, damage);
+
if (fb != gdrm->fb) {
old_fb = gdrm->fb;
drm_framebuffer_get(fb);
@@ -420,6 +421,26 @@ static void gud_fb_queue_damage(struct gud_device *gdrm,
struct drm_framebuffer
if (old_fb)
drm_framebuffer_put(old_fb);
+
+ return 0;
+}
+
+static void gud_fb_handle_damage(struct gud_device *gdrm, struct
drm_framebuffer *fb,
+const struct iosys_map *src, struct drm_rect
*damage)
+{
+ int ret;
+
+ if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE)
+ drm_rect_init(damage, 0, 0, fb->width, fb->height);
+
+
[PATCH v2 2/6] drm/gud: Don't retry a failed framebuffer flush
From: Noralf Trønnes
If a framebuffer flush fails the driver will do one retry by requeing the
worker. Currently the worker is used even for synchronous flushing, but a
later patch will inline it, so this needs to change. Thinking about how to
solve this I came to the conclusion that this retry mechanism was a fix
for a problem that was only in the mind of the developer (me) and not
something that solved a real problem.
So let's remove this for now and revisit later should it become necessary.
gud_add_damage() has now only one caller so it can be inlined.
Reviewed-by: Javier Martinez Canillas
Reviewed-by: Thomas Zimmermann
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/gud/gud_pipe.c | 48 +++---
1 file changed, 8 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 61f4abaf1811..ff1358815af5 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -333,37 +333,6 @@ void gud_clear_damage(struct gud_device *gdrm)
gdrm->damage.y2 = 0;
}
-static void gud_add_damage(struct gud_device *gdrm, struct drm_rect *damage)
-{
- gdrm->damage.x1 = min(gdrm->damage.x1, damage->x1);
- gdrm->damage.y1 = min(gdrm->damage.y1, damage->y1);
- gdrm->damage.x2 = max(gdrm->damage.x2, damage->x2);
- gdrm->damage.y2 = max(gdrm->damage.y2, damage->y2);
-}
-
-static void gud_retry_failed_flush(struct gud_device *gdrm, struct
drm_framebuffer *fb,
- struct drm_rect *damage)
-{
- /*
-* pipe_update waits for the worker when the display mode is going to
change.
-* This ensures that the width and height is still the same making it
safe to
-* add back the damage.
-*/
-
- mutex_lock(>damage_lock);
- if (!gdrm->fb) {
- drm_framebuffer_get(fb);
- gdrm->fb = fb;
- }
- gud_add_damage(gdrm, damage);
- mutex_unlock(>damage_lock);
-
- /* Retry only once to avoid a possible storm in case of continues
errors. */
- if (!gdrm->prev_flush_failed)
- queue_work(system_long_wq, >work);
- gdrm->prev_flush_failed = true;
-}
-
void gud_flush_work(struct work_struct *work)
{
struct gud_device *gdrm = container_of(work, struct gud_device, work);
@@ -407,14 +376,10 @@ void gud_flush_work(struct work_struct *work)
ret = gud_flush_rect(gdrm, fb, format, );
if (ret) {
if (ret != -ENODEV && ret != -ECONNRESET &&
- ret != -ESHUTDOWN && ret != -EPROTO) {
- bool prev_flush_failed =
gdrm->prev_flush_failed;
-
- gud_retry_failed_flush(gdrm, fb, );
- if (!prev_flush_failed)
- dev_err_ratelimited(fb->dev->dev,
- "Failed to flush
framebuffer: error=%d\n", ret);
- }
+ ret != -ESHUTDOWN && ret != -EPROTO)
+ dev_err_ratelimited(fb->dev->dev,
+ "Failed to flush
framebuffer: error=%d\n", ret);
+ gdrm->prev_flush_failed = true;
break;
}
@@ -439,7 +404,10 @@ static void gud_fb_queue_damage(struct gud_device *gdrm,
struct drm_framebuffer
gdrm->fb = fb;
}
- gud_add_damage(gdrm, damage);
+ gdrm->damage.x1 = min(gdrm->damage.x1, damage->x1);
+ gdrm->damage.y1 = min(gdrm->damage.y1, damage->y1);
+ gdrm->damage.x2 = max(gdrm->damage.x2, damage->x2);
+ gdrm->damage.y2 = max(gdrm->damage.y2, damage->y2);
mutex_unlock(>damage_lock);
--
2.34.1
[PATCH v2 0/6] drm/gud: Use the shadow plane helper
Hi, I have started to look at igt for testing and want to use CRC tests. To implement support for this I need to move away from the simple kms helper. When looking around for examples I came across Thomas' nice shadow helper and thought, yes this is perfect for drm/gud. So I'll switch to that before I move away from the simple kms helper. The async framebuffer flushing code path now uses a shadow buffer and doesn't touch the framebuffer when it shouldn't. I have also taken the opportunity to inline the synchronous flush code path and make this the default flushing stategy. Noralf. Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: dri-devel@lists.freedesktop.org Signed-off-by: Noralf Trønnes --- Changes in v2: - Drop patch (Thomas): drm/gem: shadow_fb_access: Prepare imported buffers for CPU access - Use src as variable name for iosys_map (Thomas) - Prepare imported buffer for CPU access in the driver (Thomas) - New patch: make sync flushing the default (Thomas) - Link to v1: https://lore.kernel.org/r/20221122-gud-shadow-plane-v1-0-9de3afa33...@tronnes.org --- Noralf Trønnes (6): drm/gud: Fix UBSAN warning drm/gud: Don't retry a failed framebuffer flush drm/gud: Split up gud_flush_work() drm/gud: Prepare buffer for CPU access in gud_flush_work() drm/gud: Use the shadow plane helper drm/gud: Enable synchronous flushing by default drivers/gpu/drm/gud/gud_drv.c | 1 + drivers/gpu/drm/gud/gud_internal.h | 1 + drivers/gpu/drm/gud/gud_pipe.c | 222 ++--- 3 files changed, 112 insertions(+), 112 deletions(-) --- base-commit: 7257702951305b1f0259c3468c39fc59d1ad4d8b change-id: 20221122-gud-shadow-plane-ae37a95d4d8d Best regards, -- Noralf Trønnes
Re: [PATCH 7/8] drm/mipi-dbi: Use shadow-plane mappings
Den 21.11.2022 11.45, skrev Thomas Zimmermann: > Use the buffer mappings provided by shadow-plane helpers. As the > mappings are established while the commit can still fail, errors > are now reported correctly to callers. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Noralf Trønnes
Re: [PATCH 6/8] drm/mipi-dbi: Support shadow-plane state
Den 28.11.2022 13.10, skrev Thomas Zimmermann:
> Hi
>
> Am 25.11.22 um 18:48 schrieb Noralf Trønnes:
>>
>>
>> Den 21.11.2022 11.45, skrev Thomas Zimmermann:
>>> Introduce struct drm_mipi_dbi_plane_state that contains state related to
>>> MIPI DBI. It currently only inherits from struct drm_shadow_plane_state,
>>> so that MIPI DBI drivers can use the vmap'ed GEM-buffer memory.
>>> Implement
>>> state helpers, the {begin,end}_fb_access helpers and wire up everything.
>>>
>>> With this commit, MIPI DBI drivers can access the GEM object's memory
>>> that is provided by shadow-plane state. The actual changes to drivers
>>> are implemented separately. The new struct drm_mipi_dbi_plane was added
>>> to avoid exposing struct drm_shadow_plane_state directly. The latter is
>>> a detail of the actual implementation and having it in the MIPI driver
>>> interface seems unintuitive.
>>
>> I don't understand this reasoning. The update functions still uses
>> drm_shadow_plane_state in order to access ->data[0]. If you want to
>> avoid exposing it, can't you add an accessor function for ->data[0]
>> instead? That would actually be useful to me at least since when I first
>> read the shadow plane code I didn't understand what data really was
>> referring to. fb_map would have been more clear to me.
>
> There's nothing wrong with accessing shadow-plane state directly. I
> simply found it non-intuitive to leave MIPI without it's own plane-state
> structure. From the perspective of a MIPI-based driver, up-casting to a
> shadow-plane state feels arbitrary. Upcasting to a MIPI plane state
> appears logical.
>
> Anyway, if using the shadow-plane state without the mipi plane state is
> preferred, I'll change the code accordingly.
>
I prefer to drop this. When I see the subclassed plane state without any
additional state members I'm left wondering why this is done and I start
looking for a TODO.
Noralf.
> Best regards
> Thomas
>
>>
>> Noralf.
>>
>>>
>>> Signed-off-by: Thomas Zimmermann
>>> ---
>>> drivers/gpu/drm/drm_mipi_dbi.c | 113 +
>>> drivers/gpu/drm/tiny/ili9225.c | 5 ++
>>> drivers/gpu/drm/tiny/st7586.c | 5 ++
>>> include/drm/drm_mipi_dbi.h | 30 -
>>> 4 files changed, 152 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c
>>> b/drivers/gpu/drm/drm_mipi_dbi.c
>>> index 40e59a3a6481e..3030344d25b48 100644
>>> --- a/drivers/gpu/drm/drm_mipi_dbi.c
>>> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
>>> @@ -436,6 +436,119 @@ void mipi_dbi_pipe_disable(struct
>>> drm_simple_display_pipe *pipe)
>>> }
>>> EXPORT_SYMBOL(mipi_dbi_pipe_disable);
>>> +/**
>>> + * mipi_dbi_pipe_begin_fb_access - MIPI DBI pipe begin-access helper
>>> + * @pipe: Display pipe
>>> + * @plane_state: Plane state
>>> + *
>>> + * This function implements struct
>>> _simple_display_funcs.begin_fb_access.
>>> + *
>>> + * See drm_gem_begin_shadow_fb_access() for details and
>>> mipi_dbi_pipe_cleanup_fb()
>>> + * for cleanup.
>>> + *
>>> + * Returns:
>>> + * 0 on success, or a negative errno code otherwise.
>>> + */
>>> +int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
>>> + struct drm_plane_state *plane_state)
>>> +{
>>> + return drm_gem_begin_shadow_fb_access(>plane, plane_state);
>>> +}
>>> +EXPORT_SYMBOL(mipi_dbi_pipe_begin_fb_access);
>>> +
>>> +/**
>>> + * mipi_dbi_pipe_end_fb_access - MIPI DBI pipe end-access helper
>>> + * @pipe: Display pipe
>>> + * @plane_state: Plane state
>>> + *
>>> + * This function implements struct
>>> _simple_display_funcs.end_fb_access.
>>> + *
>>> + * See mipi_dbi_pipe_begin_fb_access().
>>> + */
>>> +void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
>>> + struct drm_plane_state *plane_state)
>>> +{
>>> + drm_gem_end_shadow_fb_access(>plane, plane_state);
>>> +}
>>> +EXPORT_SYMBOL(mipi_dbi_pipe_end_fb_access);
>>> +
>>> +/**
>>> + * mipi_dbi_pipe_reset_plane - MIPI DBI plane-reset helper
>>> + * @pipe: Display pipe
>>> + *
>>> + * This function implements struct
>>> _simple_display_funcs.reset_plane
>>> + * for MIPI DBI planes.
>>> +
Re: [PATCH 0/8] drm/mipi-dbi: Convert to shadow-plane helpers
Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> Convert the MIPI-DBI-based drivers to shadow-plane helpers. The
> drivers vmap/vunmap GEM buffer memory during the atomic commit.
> Shadow-plane helpers automate this process.
>
> Patches 1 to 4 prepare the MIPI code for the change and simplify
> the restof the patchset.
>
> Patches 5 to 7 rework the vmap code in the MIPI-DBI drivers and add
> shadow-plane helpers. Most of the affected drivers call MIPI-DBI
> helpers and get the update automatically. Only ili9225 and st7586
> require changes to their source code.
>
> Patch 8 simplifies drm_dev_enter() and _exit(). It's not strictly
> needed, but streamlines the driver code and make sense overall.
>
> Testing is welcome, as I don't have any hardware to test these
> changes myself.
>
Tested-by: Noralf Trønnes # drm/tiny/mi0283qt
> Thomas Zimmermann (8):
> drm/simple-kms: Remove drm_gem_simple_display_pipe_prepare_fb()
> drm/ili9225: Call MIPI DBI mode_valid helper
> drm/st7586: Call MIPI DBI mode_valid helper
> drm/mipi-dbi: Initialize default driver functions with macro
> drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update
> helpers
> drm/mipi-dbi: Support shadow-plane state
> drm/mipi-dbi: Use shadow-plane mappings
> drm/mipi-dbi: Move drm_dev_{enter,exit}() out from fb_dirty functions
>
> drivers/gpu/drm/drm_gem_atomic_helper.c | 31 +---
> drivers/gpu/drm/drm_mipi_dbi.c | 175 +++
> drivers/gpu/drm/drm_simple_kms_helper.c | 2 +-
> drivers/gpu/drm/panel/panel-ilitek-ili9341.c | 6 +-
> drivers/gpu/drm/tiny/hx8357d.c | 5 +-
> drivers/gpu/drm/tiny/ili9163.c | 6 +-
> drivers/gpu/drm/tiny/ili9225.c | 42 +++--
> drivers/gpu/drm/tiny/ili9341.c | 5 +-
> drivers/gpu/drm/tiny/ili9486.c | 5 +-
> drivers/gpu/drm/tiny/mi0283qt.c | 5 +-
> drivers/gpu/drm/tiny/panel-mipi-dbi.c| 5 +-
> drivers/gpu/drm/tiny/st7586.c| 45 +++--
> drivers/gpu/drm/tiny/st7735r.c | 5 +-
> include/drm/drm_gem_atomic_helper.h | 2 -
> include/drm/drm_mipi_dbi.h | 50 +-
> include/drm/drm_plane.h | 4 +-
> include/drm/drm_simple_kms_helper.h | 4 +-
> 17 files changed, 265 insertions(+), 132 deletions(-)
>
>
> base-commit: b7598e2b3a3116bb5ddbf756db30a0e5dc0877ea
> prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
> prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
> prerequisite-patch-id: 3f204510fcbf9530d6540bd8e6128cce598988b6
Re: [PATCH 8/8] drm/mipi-dbi: Move drm_dev_{enter,exit}() out from fb_dirty functions
Den 21.11.2022 11.45, skrev Thomas Zimmermann: > Call drm_dev_enter() and drm_dev_exit() in the outer-most callbacks > of the modesetting pipeline. If drm_dev_enter() fails, the driver can > thus avoid unnecessary work. > > Signed-off-by: Thomas Zimmermann > --- Makes sense in order to make the code more readable, the other *_fb_dirty call sites (*_enable) are already protected. Reviewed-by: Noralf Trønnes
Re: [PATCH 6/8] drm/mipi-dbi: Support shadow-plane state
Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> Introduce struct drm_mipi_dbi_plane_state that contains state related to
> MIPI DBI. It currently only inherits from struct drm_shadow_plane_state,
> so that MIPI DBI drivers can use the vmap'ed GEM-buffer memory. Implement
> state helpers, the {begin,end}_fb_access helpers and wire up everything.
>
> With this commit, MIPI DBI drivers can access the GEM object's memory
> that is provided by shadow-plane state. The actual changes to drivers
> are implemented separately. The new struct drm_mipi_dbi_plane was added
> to avoid exposing struct drm_shadow_plane_state directly. The latter is
> a detail of the actual implementation and having it in the MIPI driver
> interface seems unintuitive.
I don't understand this reasoning. The update functions still uses
drm_shadow_plane_state in order to access ->data[0]. If you want to
avoid exposing it, can't you add an accessor function for ->data[0]
instead? That would actually be useful to me at least since when I first
read the shadow plane code I didn't understand what data really was
referring to. fb_map would have been more clear to me.
Noralf.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/drm_mipi_dbi.c | 113 +
> drivers/gpu/drm/tiny/ili9225.c | 5 ++
> drivers/gpu/drm/tiny/st7586.c | 5 ++
> include/drm/drm_mipi_dbi.h | 30 -
> 4 files changed, 152 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dbi.c b/drivers/gpu/drm/drm_mipi_dbi.c
> index 40e59a3a6481e..3030344d25b48 100644
> --- a/drivers/gpu/drm/drm_mipi_dbi.c
> +++ b/drivers/gpu/drm/drm_mipi_dbi.c
> @@ -436,6 +436,119 @@ void mipi_dbi_pipe_disable(struct
> drm_simple_display_pipe *pipe)
> }
> EXPORT_SYMBOL(mipi_dbi_pipe_disable);
>
> +/**
> + * mipi_dbi_pipe_begin_fb_access - MIPI DBI pipe begin-access helper
> + * @pipe: Display pipe
> + * @plane_state: Plane state
> + *
> + * This function implements struct _simple_display_funcs.begin_fb_access.
> + *
> + * See drm_gem_begin_shadow_fb_access() for details and
> mipi_dbi_pipe_cleanup_fb()
> + * for cleanup.
> + *
> + * Returns:
> + * 0 on success, or a negative errno code otherwise.
> + */
> +int mipi_dbi_pipe_begin_fb_access(struct drm_simple_display_pipe *pipe,
> + struct drm_plane_state *plane_state)
> +{
> + return drm_gem_begin_shadow_fb_access(>plane, plane_state);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_begin_fb_access);
> +
> +/**
> + * mipi_dbi_pipe_end_fb_access - MIPI DBI pipe end-access helper
> + * @pipe: Display pipe
> + * @plane_state: Plane state
> + *
> + * This function implements struct _simple_display_funcs.end_fb_access.
> + *
> + * See mipi_dbi_pipe_begin_fb_access().
> + */
> +void mipi_dbi_pipe_end_fb_access(struct drm_simple_display_pipe *pipe,
> + struct drm_plane_state *plane_state)
> +{
> + drm_gem_end_shadow_fb_access(>plane, plane_state);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_end_fb_access);
> +
> +/**
> + * mipi_dbi_pipe_reset_plane - MIPI DBI plane-reset helper
> + * @pipe: Display pipe
> + *
> + * This function implements struct _simple_display_funcs.reset_plane
> + * for MIPI DBI planes.
> + */
> +void mipi_dbi_pipe_reset_plane(struct drm_simple_display_pipe *pipe)
> +{
> + struct drm_plane *plane = >plane;
> + struct mipi_dbi_plane_state *mipi_dbi_plane_state;
> +
> + if (plane->state) {
> + mipi_dbi_pipe_destroy_plane_state(pipe, plane->state);
> + plane->state = NULL; /* must be set to NULL here */
> + }
> +
> + mipi_dbi_plane_state = kzalloc(sizeof(*mipi_dbi_plane_state),
> GFP_KERNEL);
> + if (!mipi_dbi_plane_state)
> + return;
> + __drm_gem_reset_shadow_plane(plane,
> _dbi_plane_state->shadow_plane_state);
> +}
> +EXPORT_SYMBOL(mipi_dbi_pipe_reset_plane);
> +
> +/**
> + * mipi_dbi_pipe_duplicate_plane_state - duplicates MIPI DBI plane state
> + * @pipe: Display pipe
> + *
> + * This function implements struct
> _simple_display_funcs.duplicate_plane_state
> + * for MIPI DBI planes.
> + *
> + * See drm_gem_duplicate_shadow_plane_state() for additional details.
> + *
> + * Returns:
> + * A pointer to a new plane state on success, or NULL otherwise.
> + */
> +struct drm_plane_state *mipi_dbi_pipe_duplicate_plane_state(struct
> drm_simple_display_pipe *pipe)
> +{
> + struct drm_plane *plane = >plane;
> + struct drm_plane_state *plane_state = plane->state;
> + struct mipi_dbi_plane_state *new_mipi_dbi_plane_state;
> + struct drm_shadow_plane_state *new_shadow_plane_state;
> +
> + if (!plane_state)
> + return NULL;
> +
> + new_mipi_dbi_plane_state = kzalloc(sizeof(*new_mipi_dbi_plane_state),
> GFP_KERNEL);
> + if (!new_mipi_dbi_plane_state)
> + return NULL;
> + new_shadow_plane_state = _mipi_dbi_plane_state->shadow_plane_state;
> +
> + __drm_gem_duplicate_shadow_plane_state(plane,
Re: [PATCH 5/8] drm/mipi-dbi: Prepare framebuffer copy operation in pipe-update helpers
if (!pipe->crtc.state->active)
> return;
>
> + ret = drm_gem_fb_vmap(fb, map, data);
> + if (ret)
> + return;
> +
> if (drm_atomic_helper_damage_merged(old_state, state, ))
> - ili9225_fb_dirty(state->fb, );
> + ili9225_fb_dirty([0], fb, );
> +
> + drm_gem_fb_vunmap(fb, map);
> }
>
> static void ili9225_pipe_enable(struct drm_simple_display_pipe *pipe,
> @@ -186,6 +197,8 @@ static void ili9225_pipe_enable(struct
> drm_simple_display_pipe *pipe,
> .y1 = 0,
> .y2 = fb->height,
> };
> + struct iosys_map map[DRM_FORMAT_MAX_PLANES];
> + struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> int ret, idx;
> u8 am_id;
>
> @@ -276,7 +289,13 @@ static void ili9225_pipe_enable(struct
> drm_simple_display_pipe *pipe,
>
> ili9225_command(dbi, ILI9225_DISPLAY_CONTROL_1, 0x1017);
>
> - ili9225_fb_dirty(fb, );
> + ret = drm_gem_fb_vmap(fb, map, data);
> + if (ret)
> + goto out_exit;
> +
> + ili9225_fb_dirty([0], fb, );
> +
> + drm_gem_fb_vunmap(fb, map);
> out_exit:
> drm_dev_exit(idx);
> }
> diff --git a/drivers/gpu/drm/tiny/st7586.c b/drivers/gpu/drm/tiny/st7586.c
> index 6bdd23e2a47c7..e773b1f2fd5f3 100644
> --- a/drivers/gpu/drm/tiny/st7586.c
> +++ b/drivers/gpu/drm/tiny/st7586.c
> @@ -92,25 +92,24 @@ static void st7586_xrgb_to_gray332(u8 *dst, void
> *vaddr,
> kfree(buf);
> }
>
> -static int st7586_buf_copy(void *dst, struct drm_framebuffer *fb,
> +static int st7586_buf_copy(void *dst, struct iosys_map *src, struct
> drm_framebuffer *fb,
> struct drm_rect *clip)
> {
> - struct drm_gem_dma_object *dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
> - void *src = dma_obj->vaddr;
> - int ret = 0;
> + int ret;
>
> ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
> if (ret)
> return ret;
>
> - st7586_xrgb_to_gray332(dst, src, fb, clip);
> + st7586_xrgb_to_gray332(dst, src->vaddr, fb, clip);
>
> drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>
> return 0;
> }
>
> -static void st7586_fb_dirty(struct drm_framebuffer *fb, struct drm_rect
> *rect)
> +static void st7586_fb_dirty(struct iosys_map *src, struct drm_framebuffer
> *fb,
> + struct drm_rect *rect)
> {
> struct mipi_dbi_dev *dbidev = drm_to_mipi_dbi_dev(fb->dev);
> struct mipi_dbi *dbi = >dbi;
> @@ -125,7 +124,7 @@ static void st7586_fb_dirty(struct drm_framebuffer *fb,
> struct drm_rect *rect)
>
> DRM_DEBUG_KMS("Flushing [FB:%d] " DRM_RECT_FMT "\n", fb->base.id,
> DRM_RECT_ARG(rect));
>
> - ret = st7586_buf_copy(dbidev->tx_buf, fb, rect);
> + ret = st7586_buf_copy(dbidev->tx_buf, src, fb, rect);
> if (ret)
> goto err_msg;
>
> @@ -154,13 +153,19 @@ static void st7586_pipe_update(struct
> drm_simple_display_pipe *pipe,
> struct drm_plane_state *old_state)
> {
> struct drm_plane_state *state = pipe->plane.state;
> + struct drm_framebuffer *fb = state->fb;
> + struct drm_gem_dma_object *dma_obj;
> + struct iosys_map src;
> struct drm_rect rect;
>
> if (!pipe->crtc.state->active)
> return;
>
> + dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
> + iosys_map_set_vaddr(, dma_obj->vaddr);
> +
You use drm_gem_fb_vmap() in the other places but here you access the
object directly (and in the next hunk), but again not so important since
it goes away in a later patch.
With the comments considered:
Reviewed-by: Noralf Trønnes
> if (drm_atomic_helper_damage_merged(old_state, state, ))
> - st7586_fb_dirty(state->fb, );
> + st7586_fb_dirty(, fb, );
> }
>
> static void st7586_pipe_enable(struct drm_simple_display_pipe *pipe,
> @@ -176,6 +181,8 @@ static void st7586_pipe_enable(struct
> drm_simple_display_pipe *pipe,
> .y1 = 0,
> .y2 = fb->height,
> };
> + struct drm_gem_dma_object *dma_obj;
> + struct iosys_map src;
> int idx, ret;
> u8 addr_mode;
>
> @@ -235,7 +242,10 @@ static void st7586_pipe_enable(struct
> drm_simple_display_pipe *pipe,
>
> msleep(100);
>
> - st7586_fb_dirty(fb, );
> + dma_obj = drm_fb_dma_get_gem_obj(fb, 0);
> + iosys_map_set_vaddr(, dma_obj->vaddr);
> +
> + st7586_fb_dirty(, fb, );
>
> mipi_dbi_command(dbi, MIPI_DCS_SET_DISPLAY_ON);
> out_exit:
> diff --git a/include/drm/drm_mipi_dbi.h b/include/drm/drm_mipi_dbi.h
> index 8c4ea7956d61d..36ac8495566b0 100644
> --- a/include/drm/drm_mipi_dbi.h
> +++ b/include/drm/drm_mipi_dbi.h
> @@ -13,9 +13,10 @@
> #include
>
> struct drm_rect;
> -struct spi_device;
> struct gpio_desc;
> +struct iosys_map;
> struct regulator;
> +struct spi_device;
>
> /**
> * struct mipi_dbi - MIPI DBI interface
> @@ -176,8 +177,9 @@ int mipi_dbi_command_read(struct mipi_dbi *dbi, u8 cmd,
> u8 *val);
> int mipi_dbi_command_buf(struct mipi_dbi *dbi, u8 cmd, u8 *data, size_t len);
> int mipi_dbi_command_stackbuf(struct mipi_dbi *dbi, u8 cmd, const u8 *data,
> size_t len);
> -int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
> +int mipi_dbi_buf_copy(void *dst, struct iosys_map *src, struct
> drm_framebuffer *fb,
> struct drm_rect *clip, bool swap);
> +
> /**
> * mipi_dbi_command - MIPI DCS command with optional parameter(s)
> * @dbi: MIPI DBI structure
Re: [PATCH 4/8] drm/mipi-dbi: Initialize default driver functions with macro
Den 21.11.2022 11.45, skrev Thomas Zimmermann:
> Introduce DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS to initialize MIPI-DBI
> helpers to default values and convert drivers. The prepare_fb function
> set by some drivers is called implicitly by simple-kms helpers, so leave
> it out.
>
> Signed-off-by: Thomas Zimmermann
> ---
> static const struct drm_simple_display_pipe_funcs hx8357d_pipe_funcs = {
> - .mode_valid = mipi_dbi_pipe_mode_valid,
> - .enable = yx240qv29_enable,
> - .disable = mipi_dbi_pipe_disable,
> - .update = mipi_dbi_pipe_update,
> + DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(yx240qv29_enable),
> };
> static const struct drm_simple_display_pipe_funcs ili9163_pipe_funcs = {
> - .mode_valid = mipi_dbi_pipe_mode_valid,
> - .enable = yx240qv29_enable,
> - .disable = mipi_dbi_pipe_disable,
> - .update = mipi_dbi_pipe_update,
> + DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(yx240qv29_enable),
> };
> static const struct drm_simple_display_pipe_funcs ili9341_pipe_funcs = {
> - .mode_valid = mipi_dbi_pipe_mode_valid,
> - .enable = yx240qv29_enable,
> - .disable = mipi_dbi_pipe_disable,
> - .update = mipi_dbi_pipe_update,
> + DRM_MIPI_DBI_SIMPLE_DISPLAY_PIPE_FUNCS(yx240qv29_enable),
> };
3 drivers have the same enable function name, ili9163 and hx8357d has
clearly copied from ili9341 which actually supports the yx240qv29 panel.
At least hx8357d managed to update the display mode variable name,
ili9163 didn't. It's not unlikely that I reviewed these drivers...
But that has nothing to do with this patch:
Reviewed-by: Noralf Trønnes
Re: [PATCH 3/8] drm/st7586: Call MIPI DBI mode_valid helper
Den 21.11.2022 11.45, skrev Thomas Zimmermann: > MIPI DBI drivers validate each mode against their native resolution. > Add this test to st7586. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Noralf Trønnes
Re: [PATCH 2/8] drm/ili9225: Call MIPI DBI mode_valid helper
Den 21.11.2022 11.45, skrev Thomas Zimmermann: > MIPI DBI drivers validate each mode against their native resolution. > Add this test to ili9225. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Noralf Trønnes
Re: [PATCH 1/8] drm/simple-kms: Remove drm_gem_simple_display_pipe_prepare_fb()
Den 21.11.2022 11.45, skrev Thomas Zimmermann: > The helper drm_gem_simple_display_pipe_prepare_fb() is simple-KMS' > default implementation for prepare_fb. Remove the call from drivers > that set it explicitly. Then inline the helper into the only caller > within simple-kms helpers and remove . No functional changes. > > Simple-KMS drivers that implement the prepare_fb callback should call > drm_gem_plane_helper_prepare_fb() directly. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Noralf Trønnes
Re: [PATCH v10 06/19] drm/modes: Add a function to generate analog display modes
Den 17.11.2022 10.28, skrev Maxime Ripard: > Multiple drivers (meson, vc4, sun4i) define analog TV 525-lines and > 625-lines modes in their drivers. > > Since those modes are fairly standard, and that we'll need to use them > in more places in the future, it makes sense to move their definition > into the core framework. > > However, analog display usually have fairly loose timings requirements, > the only discrete parameters being the total number of lines and pixel > clock frequency. Thus, we created a function that will create a display > mode from the standard, the pixel frequency and the active area. > > Tested-by: Mateusz Kwiatkowski > Signed-off-by: Maxime Ripard > > --- I'm no domain expert so apart from the timing details which I can't comment on, it looks fine. I personally advocated for a much simpler solution for these NTSC and PAL modes, but AIUI this is part of a grander plan to support devices with other timings. Acked-by: Noralf Trønnes
Re: [PATCH v10 05/19] drm/connector: Add TV standard property
Den 17.11.2022 10.28, skrev Maxime Ripard:
> The TV mode property has been around for a while now to select and get the
> current TV mode output on an analog TV connector.
>
> Despite that property name being generic, its content isn't and has been
> driver-specific which makes it hard to build any generic behaviour on top
> of it, both in kernel and user-space.
>
> Let's create a new enum tv norm property, that can contain any of the
> analog TV standards currently supported by kernel drivers. Each driver can
> then pass in a bitmask of the modes it supports, and the property
> creation function will filter out the modes not supported.
>
> We'll then be able to phase out the older tv mode property.
>
> Tested-by: Mateusz Kwiatkowski
> Signed-off-by: Maxime Ripard
>
> ---
> Changes in v10:
> - Fix checkpatch warning
>
> Changes in v5:
> - Create an analog TV properties documentation section, and document TV
> Mode there instead of the csv file
>
> Changes in v4:
> - Add property documentation to kms-properties.csv
> - Fix documentation
> ---
> Documentation/gpu/drm-kms.rst | 6 ++
> drivers/gpu/drm/drm_atomic_uapi.c | 4 ++
> drivers/gpu/drm/drm_connector.c | 122
> +-
> include/drm/drm_connector.h | 64
> include/drm/drm_mode_config.h | 8 +++
> 5 files changed, 203 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst
> index b4377a545425..321f2f582c64 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -520,6 +520,12 @@ HDMI Specific Connector Properties
> .. kernel-doc:: drivers/gpu/drm/drm_connector.c
> :doc: HDMI connector properties
>
> +Analog TV Specific Connector Properties
> +--
> +
> +.. kernel-doc:: drivers/gpu/drm/drm_connector.c
> + :doc: Analog TV Connector Properties
> +
> Standard CRTC Properties
>
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index 7f2b9a07fbdf..d867e7f9f2cd 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -700,6 +700,8 @@ static int drm_atomic_connector_set_property(struct
> drm_connector *connector,
> state->tv.margins.bottom = val;
> } else if (property == config->legacy_tv_mode_property) {
> state->tv.legacy_mode = val;
> + } else if (property == config->tv_mode_property) {
> + state->tv.mode = val;
> } else if (property == config->tv_brightness_property) {
> state->tv.brightness = val;
> } else if (property == config->tv_contrast_property) {
> @@ -810,6 +812,8 @@ drm_atomic_connector_get_property(struct drm_connector
> *connector,
> *val = state->tv.margins.bottom;
> } else if (property == config->legacy_tv_mode_property) {
> *val = state->tv.legacy_mode;
> + } else if (property == config->tv_mode_property) {
> + *val = state->tv.mode;
> } else if (property == config->tv_brightness_property) {
> *val = state->tv.brightness;
> } else if (property == config->tv_contrast_property) {
> diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
> index 06e737ed15f5..07d449736956 100644
> --- a/drivers/gpu/drm/drm_connector.c
> +++ b/drivers/gpu/drm/drm_connector.c
> @@ -984,6 +984,17 @@ static const struct drm_prop_enum_list
> drm_dvi_i_subconnector_enum_list[] = {
> DRM_ENUM_NAME_FN(drm_get_dvi_i_subconnector_name,
>drm_dvi_i_subconnector_enum_list)
>
> +static const struct drm_prop_enum_list drm_tv_mode_enum_list[] = {
> + { DRM_MODE_TV_MODE_NTSC, "NTSC" },
> + { DRM_MODE_TV_MODE_NTSC_443, "NTSC-443" },
> + { DRM_MODE_TV_MODE_NTSC_J, "NTSC-J" },
> + { DRM_MODE_TV_MODE_PAL, "PAL" },
> + { DRM_MODE_TV_MODE_PAL_M, "PAL-M" },
> + { DRM_MODE_TV_MODE_PAL_N, "PAL-N" },
> + { DRM_MODE_TV_MODE_SECAM, "SECAM" },
> +};
> +DRM_ENUM_NAME_FN(drm_get_tv_mode_name, drm_tv_mode_enum_list)
> +
This patch looks good but since I'm no TV standards expert I can't say
if the content of this list is a good choice for reflecting the world of
TV standards.
Acked-by: Noralf Trønnes
> static const struct drm_prop_enum_list drm_tv_select_enum_list[] = {
> { DRM_MODE_SUBCONNECTOR_Automatic, "Automatic" }, /* DVI-I and TV-out */
> { DRM_MODE_SUBCONNECTOR_Composite, "Composite" }
Re: [PATCH 1/6] drm/gem: shadow_fb_access: Prepare imported buffers for CPU access
Den 23.11.2022 09.22, skrev Javier Martinez Canillas:
> Hello Noralf,
>
> On 11/22/22 21:58, Noralf Trønnes via B4 Submission Endpoint wrote:
>> From: Noralf Trønnes
>>
>> Complete the shadow fb access functions by also preparing imported buffers
>> for CPU access. Update the affected drivers that currently use
>> drm_gem_fb_begin_cpu_access().
>>
>> Through this change the following SHMEM drivers will now also make sure
>> their imported buffers are prepared for CPU access: cirrus, hyperv,
>> mgag200, vkms
>>
>
> [...]
>
>> @@ -378,11 +379,20 @@ int drm_gem_begin_shadow_fb_access(struct drm_plane
>> *plane, struct drm_plane_sta
>> {
>> struct drm_shadow_plane_state *shadow_plane_state =
>> to_drm_shadow_plane_state(plane_state);
>> struct drm_framebuffer *fb = plane_state->fb;
>> +int ret;
>>
>> if (!fb)
>> return 0;
>>
>> -return drm_gem_fb_vmap(fb, shadow_plane_state->map,
>> shadow_plane_state->data);
>> +ret = drm_gem_fb_vmap(fb, shadow_plane_state->map,
>> shadow_plane_state->data);
>> +if (ret)
>> +return ret;
>> +
>> +ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> +if (ret)
>> +drm_gem_fb_vunmap(fb, shadow_plane_state->map);
>> +
>> +return ret;
>
> Makes sense to me to have the CPU access prepare here too.
>
>> diff --git a/drivers/gpu/drm/solomon/ssd130x.c
>> b/drivers/gpu/drm/solomon/ssd130x.c
>> index 53464afc2b9a..58a2f0113f24 100644
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -544,7 +544,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer
>> *fb, const struct iosys_m
>> struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
>> struct iosys_map dst;
>> unsigned int dst_pitch;
>> -int ret = 0;
>> u8 *buf = NULL;
>>
>> /* Align y to display page boundaries */
>> @@ -556,21 +555,14 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer
>> *fb, const struct iosys_m
>> if (!buf)
>> return -ENOMEM;
>>
>> -ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> -if (ret)
>> -goto out_free;
>> -
>> iosys_map_set_vaddr(, buf);
>> drm_fb_xrgb_to_mono(, _pitch, vmap, fb, rect);
>>
>> -drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> -
>
> The only thing I'm not sure about is that drivers used to call the begin/end
> CPU access only during the window while the BO data was accessed but now the
> windows will be slightly bigger if that happens in .{begin,end}_fb_access.
>
I didn't think that could be an issue since userspace isn't touching the
buffer while the commit is ongoing anyways, but it's a complicated
machinery. We'll see what Daniel has to say.
Noralf.
> If that's not an issue then, I agree with your patch since it simplifies the
> logic in the drivers and gets rid of duplicated code.
>
> Reviewed-by: Javier Martinez Canillas
>
Re: [PATCH 6/6] drm/gud: Use the shadow plane helper
Den 23.11.2022 11.38, skrev Thomas Zimmermann:
> Hi
>
> Am 22.11.22 um 21:58 schrieb Noralf Trønnes via B4 Submission Endpoint:
>> From: Noralf Trønnes
>>
>> Use the shadow plane helper to take care of preparing the framebuffer for
>> CPU access. The synchronous flushing is now done inline without the
>> use of
>> a worker. The async path now uses a shadow buffer to hold framebuffer
>> changes and it doesn't read the framebuffer behind userspace's back
>> anymore.
>>
>> Signed-off-by: Noralf Trønnes
>> ---
>> drivers/gpu/drm/gud/gud_drv.c | 1 +
>> drivers/gpu/drm/gud/gud_internal.h | 1 +
>> drivers/gpu/drm/gud/gud_pipe.c | 69
>> --
>> 3 files changed, 46 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/gud/gud_drv.c
>> b/drivers/gpu/drm/gud/gud_drv.c
>> index d57dab104358..5aac7cda0505 100644
>> --- a/drivers/gpu/drm/gud/gud_drv.c
>> +++ b/drivers/gpu/drm/gud/gud_drv.c
>> @@ -365,6 +365,7 @@ static void gud_debugfs_init(struct drm_minor *minor)
>> static const struct drm_simple_display_pipe_funcs gud_pipe_funcs = {
>> .check = gud_pipe_check,
>> .update = gud_pipe_update,
>> + DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS
>> };
>> static const struct drm_mode_config_funcs gud_mode_config_funcs = {
>> diff --git a/drivers/gpu/drm/gud/gud_internal.h
>> b/drivers/gpu/drm/gud/gud_internal.h
>> index e351a1f1420d..0d148a6f27aa 100644
>> --- a/drivers/gpu/drm/gud/gud_internal.h
>> +++ b/drivers/gpu/drm/gud/gud_internal.h
>> @@ -43,6 +43,7 @@ struct gud_device {
>> struct drm_framebuffer *fb;
>> struct drm_rect damage;
>> bool prev_flush_failed;
>> + void *shadow_buf;
>> };
>> static inline struct gud_device *to_gud_device(struct drm_device
>> *drm)
>> diff --git a/drivers/gpu/drm/gud/gud_pipe.c
>> b/drivers/gpu/drm/gud/gud_pipe.c
>> index dfada6eedc58..7686325f7ee7 100644
>> --- a/drivers/gpu/drm/gud/gud_pipe.c
>> +++ b/drivers/gpu/drm/gud/gud_pipe.c
>> @@ -358,10 +358,10 @@ static void gud_flush_damage(struct gud_device
>> *gdrm, struct drm_framebuffer *fb
>> void gud_flush_work(struct work_struct *work)
>> {
>> struct gud_device *gdrm = container_of(work, struct gud_device,
>> work);
>> - struct iosys_map gem_map = { }, fb_map = { };
>> + struct iosys_map shadow_map;
>> struct drm_framebuffer *fb;
>> struct drm_rect damage;
>> - int idx, ret;
>> + int idx;
>> if (!drm_dev_enter(>drm, ))
>> return;
>> @@ -369,6 +369,7 @@ void gud_flush_work(struct work_struct *work)
>> mutex_lock(>damage_lock);
>> fb = gdrm->fb;
>> gdrm->fb = NULL;
>> + iosys_map_set_vaddr(_map, gdrm->shadow_buf);
>> damage = gdrm->damage;
>> gud_clear_damage(gdrm);
>> mutex_unlock(>damage_lock);
>> @@ -376,33 +377,33 @@ void gud_flush_work(struct work_struct *work)
>> if (!fb)
>> goto out;
>> - ret = drm_gem_fb_vmap(fb, _map, _map);
>> - if (ret)
>> - goto fb_put;
>> + gud_flush_damage(gdrm, fb, _map, true, );
>> - ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> - if (ret)
>> - goto vunmap;
>> -
>> - /* Imported buffers are assumed to be WriteCombined with uncached
>> reads */
>> - gud_flush_damage(gdrm, fb, _map, !fb->obj[0]->import_attach,
>> );
>> -
>> - drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> -vunmap:
>> - drm_gem_fb_vunmap(fb, _map);
>> -fb_put:
>> drm_framebuffer_put(fb);
>> out:
>> drm_dev_exit(idx);
>> }
>> -static void gud_fb_queue_damage(struct gud_device *gdrm, struct
>> drm_framebuffer *fb,
>> - struct drm_rect *damage)
>> +static int gud_fb_queue_damage(struct gud_device *gdrm, struct
>> drm_framebuffer *fb,
>> + const struct iosys_map *map, struct drm_rect *damage)
>> {
>> struct drm_framebuffer *old_fb = NULL;
>> + struct iosys_map shadow_map;
>> mutex_lock(>damage_lock);
>> + if (!gdrm->shadow_buf) {
>> + gdrm->shadow_buf = vzalloc(fb->pitches[0] * fb->height);
>> + if (!gdrm->shadow_buf) {
>> + mutex_unlock(>damage_lock);
>> + return -ENOMEM;
>> + }
>> + }
>> +
>> + iosys_map_s
Re: [PATCH 1/6] drm/gem: shadow_fb_access: Prepare imported buffers for CPU access
Den 23.11.2022 09.39, skrev Thomas Zimmermann:
> (cc: danvet)
>
> Hi
>
> Am 22.11.22 um 21:58 schrieb Noralf Trønnes via B4 Submission Endpoint:
>> From: Noralf Trønnes
>>
>> Complete the shadow fb access functions by also preparing imported
>> buffers
>> for CPU access. Update the affected drivers that currently use
>> drm_gem_fb_begin_cpu_access().
>>
>> Through this change the following SHMEM drivers will now also make sure
>> their imported buffers are prepared for CPU access: cirrus, hyperv,
>> mgag200, vkms
>
> I had a similar patch recently, but Daniel shot it down. AFAIR
> begin_cpu_access *somehow* interferes with *something* and that can
> leads to *problems.* Sorry that's the best I remember. Daniel should
> know. :D
>
I loved this explanation, it gave me a good laugh :)
Yeah, I wondered why you hadn't done it, but assumed you just hadn't
gotten atround to it yet and if there were other reasons I would be told :)
Noralf.
> Best regards
> Thomas
>
>>
>> Cc: Thomas Zimmermann
>> Cc: Javier Martinez Canillas
>> Cc: Hans de Goede
>> Cc: Dave Airlie
>> Signed-off-by: Noralf Trønnes
>> ---
>> drivers/gpu/drm/drm_gem_atomic_helper.c | 13 -
>> drivers/gpu/drm/solomon/ssd130x.c | 10 +-
>> drivers/gpu/drm/tiny/gm12u320.c | 10 +-
>> drivers/gpu/drm/tiny/ofdrm.c | 10 ++
>> drivers/gpu/drm/tiny/simpledrm.c | 10 ++
>> drivers/gpu/drm/udl/udl_modeset.c | 11 ++-
>> 6 files changed, 20 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c
>> b/drivers/gpu/drm/drm_gem_atomic_helper.c
>> index e42800718f51..0eef4bb30d25 100644
>> --- a/drivers/gpu/drm/drm_gem_atomic_helper.c
>> +++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
>> @@ -368,6 +368,7 @@ EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
>> * maps all buffer objects of the plane's framebuffer into kernel
>> address
>> * space and stores them in struct _shadow_plane_state.map. The
>> first data
>> * bytes are available in struct _shadow_plane_state.data.
>> + * It also prepares imported buffers for CPU access.
>> *
>> * See drm_gem_end_shadow_fb_access() for cleanup.
>> *
>> @@ -378,11 +379,20 @@ int drm_gem_begin_shadow_fb_access(struct
>> drm_plane *plane, struct drm_plane_sta
>> {
>> struct drm_shadow_plane_state *shadow_plane_state =
>> to_drm_shadow_plane_state(plane_state);
>> struct drm_framebuffer *fb = plane_state->fb;
>> + int ret;
>> if (!fb)
>> return 0;
>> - return drm_gem_fb_vmap(fb, shadow_plane_state->map,
>> shadow_plane_state->data);
>> + ret = drm_gem_fb_vmap(fb, shadow_plane_state->map,
>> shadow_plane_state->data);
>> + if (ret)
>> + return ret;
>> +
>> + ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> + if (ret)
>> + drm_gem_fb_vunmap(fb, shadow_plane_state->map);
>> +
>> + return ret;
>> }
>> EXPORT_SYMBOL(drm_gem_begin_shadow_fb_access);
>> @@ -404,6 +414,7 @@ void drm_gem_end_shadow_fb_access(struct
>> drm_plane *plane, struct drm_plane_stat
>> if (!fb)
>> return;
>> + drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> drm_gem_fb_vunmap(fb, shadow_plane_state->map);
>> }
>> EXPORT_SYMBOL(drm_gem_end_shadow_fb_access);
>> diff --git a/drivers/gpu/drm/solomon/ssd130x.c
>> b/drivers/gpu/drm/solomon/ssd130x.c
>> index 53464afc2b9a..58a2f0113f24 100644
>> --- a/drivers/gpu/drm/solomon/ssd130x.c
>> +++ b/drivers/gpu/drm/solomon/ssd130x.c
>> @@ -544,7 +544,6 @@ static int ssd130x_fb_blit_rect(struct
>> drm_framebuffer *fb, const struct iosys_m
>> struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
>> struct iosys_map dst;
>> unsigned int dst_pitch;
>> - int ret = 0;
>> u8 *buf = NULL;
>> /* Align y to display page boundaries */
>> @@ -556,21 +555,14 @@ static int ssd130x_fb_blit_rect(struct
>> drm_framebuffer *fb, const struct iosys_m
>> if (!buf)
>> return -ENOMEM;
>> - ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
>> - if (ret)
>> - goto out_free;
>> -
>> iosys_map_set_vaddr(, buf);
>> drm_fb_xrgb_to_mono(, _pitch, vmap, fb, rect);
>> - drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
>> -
>> ssd130x_update_rect(ss
Re: git send-email friendly smtp provider anyone?
Den 22.11.2022 20.22, skrev Noralf Trønnes:
>
>
> Den 22.11.2022 19.50, skrev Konstantin Ryabitsev:
>> On Tue, Nov 22, 2022 at 06:42:19PM +0100, Noralf Trønnes wrote:
>>> The first thing that strikes me is that everyone mentioned in one of the
>>> patches get the entire patchset, even sta...@vger.kernel.org (cc'ed in a
>>> fixes patch). The first patch touches a core file and as a result a few
>>> drivers, so I've cc'ed the driver maintainers in that patch, but now
>>> they get the entire patchset where 5 of 6 patches is about a driver that
>>> I maintain. So from their point of view, they see a patchset about a
>>> driver they don't care about and a patch touching a core file, but from
>>> the subject it's not apparent that it touches their driver. I'm afraid
>>> that this might result in none of them looking at that patch. In this
>>> particular case it's not that important, but in another case it might be.
>>
>> I did some (unscientific) polling among kernel maintainers and, by a vast
>> margin, they always prefer to receive the entire series instead of
>> cherry-picked patches -- having the entire series helps provide important
>> context for the change they are looking at.
>>
>> So, this is deliberate and, for now at least, not configurable. Unless you're
>> sending 100+ patch series, I doubt anyone will have any problem with
>> receiving
>> the whole series instead of individual patches.
>>
>>> As for the setting up the web endpoint, should I just follow the b4 docs
>>> on that?
>>>
>>> I use b4 version 0.10.1, is that recent enough?
>>
>> Yes. There will be a 0.10.2 in the near future, but the incoming fixes
>> shouldn't make much difference for the b4 send code.
>>
>
> This is what I got:
>
> $ b4 send --web-auth-verify
> Signing challenge
> Submitting verification to https://lkml.kernel.org/_b4_submit
> Traceback (most recent call last):
> File "/home/pi/.local/bin/b4", line 8, in
> sys.exit(cmd())
> File "/home/pi/.local/lib/python3.10/site-packages/b4/command.py",
> line 341, in cmd
> cmdargs.func(cmdargs)
> File "/home/pi/.local/lib/python3.10/site-packages/b4/command.py",
> line 86, in cmd_send
> b4.ez.cmd_send(cmdargs)
> File "/home/pi/.local/lib/python3.10/site-packages/b4/ez.py", line
> 1102, in cmd_send
> auth_verify(cmdargs)
> File "/home/pi/.local/lib/python3.10/site-packages/b4/ez.py", line
> 188, in auth_verify
> res = ses.post(endpoint, json=req)
> File "/usr/lib/python3/dist-packages/requests/sessions.py", line 590,
> in post
> return self.request('POST', url, data=data, json=json, **kwargs)
> File "/usr/lib/python3/dist-packages/requests/sessions.py", line 528,
> in request
> prep = self.prepare_request(req)
> File "/usr/lib/python3/dist-packages/requests/sessions.py", line 456,
> in prepare_request
> p.prepare(
> File "/usr/lib/python3/dist-packages/requests/models.py", line 319, in
> prepare
> self.prepare_body(data, files, json)
> File "/usr/lib/python3/dist-packages/requests/models.py", line 469, in
> prepare_body
> body = complexjson.dumps(json)
> File "/usr/lib/python3.10/json/__init__.py", line 231, in dumps
> return _default_encoder.encode(obj)
> File "/usr/lib/python3.10/json/encoder.py", line 199, in encode
> chunks = self.iterencode(o, _one_shot=True)
> File "/usr/lib/python3.10/json/encoder.py", line 257, in iterencode
> return _iterencode(o, 0)
> File "/usr/lib/python3.10/json/encoder.py", line 179, in default
> raise TypeError(f'Object of type {o.__class__.__name__} '
> TypeError: Object of type bytes is not JSON serializable
>
Konstantin found a workaround, so I was able to push the patches.
Here's the result if anyone is interested in seeing the result of using
b4 and the web endpoint:
https://lore.kernel.org/dri-devel/20221122-gud-shadow-plane-v1-0-9de3afa33...@tronnes.org/
Patchwork gave me a new submitter ID:
https://patchwork.freedesktop.org/series/111222/
Noralf.
[PATCH 6/6] drm/gud: Use the shadow plane helper
From: Noralf Trønnes
Use the shadow plane helper to take care of preparing the framebuffer for
CPU access. The synchronous flushing is now done inline without the use of
a worker. The async path now uses a shadow buffer to hold framebuffer
changes and it doesn't read the framebuffer behind userspace's back
anymore.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/gud/gud_drv.c | 1 +
drivers/gpu/drm/gud/gud_internal.h | 1 +
drivers/gpu/drm/gud/gud_pipe.c | 69 --
3 files changed, 46 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_drv.c b/drivers/gpu/drm/gud/gud_drv.c
index d57dab104358..5aac7cda0505 100644
--- a/drivers/gpu/drm/gud/gud_drv.c
+++ b/drivers/gpu/drm/gud/gud_drv.c
@@ -365,6 +365,7 @@ static void gud_debugfs_init(struct drm_minor *minor)
static const struct drm_simple_display_pipe_funcs gud_pipe_funcs = {
.check = gud_pipe_check,
.update = gud_pipe_update,
+ DRM_GEM_SIMPLE_DISPLAY_PIPE_SHADOW_PLANE_FUNCS
};
static const struct drm_mode_config_funcs gud_mode_config_funcs = {
diff --git a/drivers/gpu/drm/gud/gud_internal.h
b/drivers/gpu/drm/gud/gud_internal.h
index e351a1f1420d..0d148a6f27aa 100644
--- a/drivers/gpu/drm/gud/gud_internal.h
+++ b/drivers/gpu/drm/gud/gud_internal.h
@@ -43,6 +43,7 @@ struct gud_device {
struct drm_framebuffer *fb;
struct drm_rect damage;
bool prev_flush_failed;
+ void *shadow_buf;
};
static inline struct gud_device *to_gud_device(struct drm_device *drm)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index dfada6eedc58..7686325f7ee7 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -358,10 +358,10 @@ static void gud_flush_damage(struct gud_device *gdrm,
struct drm_framebuffer *fb
void gud_flush_work(struct work_struct *work)
{
struct gud_device *gdrm = container_of(work, struct gud_device, work);
- struct iosys_map gem_map = { }, fb_map = { };
+ struct iosys_map shadow_map;
struct drm_framebuffer *fb;
struct drm_rect damage;
- int idx, ret;
+ int idx;
if (!drm_dev_enter(>drm, ))
return;
@@ -369,6 +369,7 @@ void gud_flush_work(struct work_struct *work)
mutex_lock(>damage_lock);
fb = gdrm->fb;
gdrm->fb = NULL;
+ iosys_map_set_vaddr(_map, gdrm->shadow_buf);
damage = gdrm->damage;
gud_clear_damage(gdrm);
mutex_unlock(>damage_lock);
@@ -376,33 +377,33 @@ void gud_flush_work(struct work_struct *work)
if (!fb)
goto out;
- ret = drm_gem_fb_vmap(fb, _map, _map);
- if (ret)
- goto fb_put;
+ gud_flush_damage(gdrm, fb, _map, true, );
- ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
- if (ret)
- goto vunmap;
-
- /* Imported buffers are assumed to be WriteCombined with uncached reads
*/
- gud_flush_damage(gdrm, fb, _map, !fb->obj[0]->import_attach,
);
-
- drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-vunmap:
- drm_gem_fb_vunmap(fb, _map);
-fb_put:
drm_framebuffer_put(fb);
out:
drm_dev_exit(idx);
}
-static void gud_fb_queue_damage(struct gud_device *gdrm, struct
drm_framebuffer *fb,
- struct drm_rect *damage)
+static int gud_fb_queue_damage(struct gud_device *gdrm, struct drm_framebuffer
*fb,
+ const struct iosys_map *map, struct drm_rect
*damage)
{
struct drm_framebuffer *old_fb = NULL;
+ struct iosys_map shadow_map;
mutex_lock(>damage_lock);
+ if (!gdrm->shadow_buf) {
+ gdrm->shadow_buf = vzalloc(fb->pitches[0] * fb->height);
+ if (!gdrm->shadow_buf) {
+ mutex_unlock(>damage_lock);
+ return -ENOMEM;
+ }
+ }
+
+ iosys_map_set_vaddr(_map, gdrm->shadow_buf);
+ iosys_map_incr(_map, drm_fb_clip_offset(fb->pitches[0],
fb->format, damage));
+ drm_fb_memcpy(_map, fb->pitches, map, fb, damage);
+
if (fb != gdrm->fb) {
old_fb = gdrm->fb;
drm_framebuffer_get(fb);
@@ -420,6 +421,26 @@ static void gud_fb_queue_damage(struct gud_device *gdrm,
struct drm_framebuffer
if (old_fb)
drm_framebuffer_put(old_fb);
+
+ return 0;
+}
+
+static void gud_fb_handle_damage(struct gud_device *gdrm, struct
drm_framebuffer *fb,
+const struct iosys_map *map, struct drm_rect
*damage)
+{
+ int ret;
+
+ if (gdrm->flags & GUD_DISPLAY_FLAG_FULL_UPDATE)
+ drm_rect_init(damage, 0, 0, fb->width, fb->height);
+
+ if (gud_async_flush) {
+ ret = gud_fb_queue_damage(gdrm, fb, map, damage);
+ if (ret != -
[PATCH 4/6] drm/gud: Split up gud_flush_work()
From: Noralf Trønnes
In preparation for inlining synchronous flushing split out the part of
gud_flush_work() that can be shared by the sync and async code paths.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/gud/gud_pipe.c | 72 +++---
1 file changed, 39 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index ff1358815af5..d2af9947494f 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -333,15 +333,49 @@ void gud_clear_damage(struct gud_device *gdrm)
gdrm->damage.y2 = 0;
}
+static void gud_flush_damage(struct gud_device *gdrm, struct drm_framebuffer
*fb,
+struct drm_rect *damage)
+{
+ const struct drm_format_info *format;
+ unsigned int i, lines;
+ size_t pitch;
+ int ret;
+
+ format = fb->format;
+ if (format->format == DRM_FORMAT_XRGB &&
gdrm->xrgb_emulation_format)
+ format = gdrm->xrgb_emulation_format;
+
+ /* Split update if it's too big */
+ pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(damage));
+ lines = drm_rect_height(damage);
+
+ if (gdrm->bulk_len < lines * pitch)
+ lines = gdrm->bulk_len / pitch;
+
+ for (i = 0; i < DIV_ROUND_UP(drm_rect_height(damage), lines); i++) {
+ struct drm_rect rect = *damage;
+
+ rect.y1 += i * lines;
+ rect.y2 = min_t(u32, rect.y1 + lines, damage->y2);
+
+ ret = gud_flush_rect(gdrm, fb, format, );
+ if (ret) {
+ if (ret != -ENODEV && ret != -ECONNRESET &&
+ ret != -ESHUTDOWN && ret != -EPROTO)
+ dev_err_ratelimited(fb->dev->dev,
+ "Failed to flush
framebuffer: error=%d\n", ret);
+ gdrm->prev_flush_failed = true;
+ break;
+ }
+ }
+}
+
void gud_flush_work(struct work_struct *work)
{
struct gud_device *gdrm = container_of(work, struct gud_device, work);
- const struct drm_format_info *format;
struct drm_framebuffer *fb;
struct drm_rect damage;
- unsigned int i, lines;
- int idx, ret = 0;
- size_t pitch;
+ int idx;
if (!drm_dev_enter(>drm, ))
return;
@@ -356,35 +390,7 @@ void gud_flush_work(struct work_struct *work)
if (!fb)
goto out;
- format = fb->format;
- if (format->format == DRM_FORMAT_XRGB &&
gdrm->xrgb_emulation_format)
- format = gdrm->xrgb_emulation_format;
-
- /* Split update if it's too big */
- pitch = drm_format_info_min_pitch(format, 0, drm_rect_width());
- lines = drm_rect_height();
-
- if (gdrm->bulk_len < lines * pitch)
- lines = gdrm->bulk_len / pitch;
-
- for (i = 0; i < DIV_ROUND_UP(drm_rect_height(), lines); i++) {
- struct drm_rect rect = damage;
-
- rect.y1 += i * lines;
- rect.y2 = min_t(u32, rect.y1 + lines, damage.y2);
-
- ret = gud_flush_rect(gdrm, fb, format, );
- if (ret) {
- if (ret != -ENODEV && ret != -ECONNRESET &&
- ret != -ESHUTDOWN && ret != -EPROTO)
- dev_err_ratelimited(fb->dev->dev,
- "Failed to flush
framebuffer: error=%d\n", ret);
- gdrm->prev_flush_failed = true;
- break;
- }
-
- gdrm->prev_flush_failed = false;
- }
+ gud_flush_damage(gdrm, fb, );
drm_framebuffer_put(fb);
out:
--
b4 0.10.1
[PATCH 5/6] drm/gud: Prepare buffer for CPU access in gud_flush_work()
From: Noralf Trønnes
In preparation for moving to the shadow plane helper prepare the
framebuffer for CPU access as early as possible.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/gud/gud_pipe.c | 67 +-
1 file changed, 33 insertions(+), 34 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index d2af9947494f..dfada6eedc58 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -15,6 +15,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -152,32 +153,21 @@ static size_t gud_xrgb_to_color(u8 *dst, const struct
drm_format_info *forma
}
static int gud_prep_flush(struct gud_device *gdrm, struct drm_framebuffer *fb,
+ const struct iosys_map *map, bool cached_reads,
const struct drm_format_info *format, struct drm_rect
*rect,
struct gud_set_buffer_req *req)
{
- struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
u8 compression = gdrm->compression;
- struct iosys_map map[DRM_FORMAT_MAX_PLANES] = { };
- struct iosys_map map_data[DRM_FORMAT_MAX_PLANES] = { };
struct iosys_map dst;
void *vaddr, *buf;
size_t pitch, len;
- int ret = 0;
pitch = drm_format_info_min_pitch(format, 0, drm_rect_width(rect));
len = pitch * drm_rect_height(rect);
if (len > gdrm->bulk_len)
return -E2BIG;
- ret = drm_gem_fb_vmap(fb, map, map_data);
- if (ret)
- return ret;
-
- vaddr = map_data[0].vaddr;
-
- ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
- if (ret)
- goto vunmap;
+ vaddr = map[0].vaddr;
retry:
if (compression)
buf = gdrm->compress_buf;
@@ -192,29 +182,27 @@ static int gud_prep_flush(struct gud_device *gdrm, struct
drm_framebuffer *fb,
if (format != fb->format) {
if (format->format == GUD_DRM_FORMAT_R1) {
len = gud_xrgb_to_r124(buf, format, vaddr, fb,
rect);
- if (!len) {
- ret = -ENOMEM;
- goto end_cpu_access;
- }
+ if (!len)
+ return -ENOMEM;
} else if (format->format == DRM_FORMAT_R8) {
- drm_fb_xrgb_to_gray8(, NULL, map_data, fb,
rect);
+ drm_fb_xrgb_to_gray8(, NULL, map, fb, rect);
} else if (format->format == DRM_FORMAT_RGB332) {
- drm_fb_xrgb_to_rgb332(, NULL, map_data, fb,
rect);
+ drm_fb_xrgb_to_rgb332(, NULL, map, fb, rect);
} else if (format->format == DRM_FORMAT_RGB565) {
- drm_fb_xrgb_to_rgb565(, NULL, map_data, fb,
rect,
+ drm_fb_xrgb_to_rgb565(, NULL, map, fb, rect,
gud_is_big_endian());
} else if (format->format == DRM_FORMAT_RGB888) {
- drm_fb_xrgb_to_rgb888(, NULL, map_data, fb,
rect);
+ drm_fb_xrgb_to_rgb888(, NULL, map, fb, rect);
} else {
len = gud_xrgb_to_color(buf, format, vaddr, fb,
rect);
}
} else if (gud_is_big_endian() && format->cpp[0] > 1) {
- drm_fb_swab(, NULL, map_data, fb, rect, !import_attach);
- } else if (compression && !import_attach && pitch == fb->pitches[0]) {
+ drm_fb_swab(, NULL, map, fb, rect, cached_reads);
+ } else if (compression && cached_reads && pitch == fb->pitches[0]) {
/* can compress directly from the framebuffer */
buf = vaddr + rect->y1 * pitch;
} else {
- drm_fb_memcpy(, NULL, map_data, fb, rect);
+ drm_fb_memcpy(, NULL, map, fb, rect);
}
memset(req, 0, sizeof(*req));
@@ -237,12 +225,7 @@ static int gud_prep_flush(struct gud_device *gdrm, struct
drm_framebuffer *fb,
req->compressed_length = cpu_to_le32(complen);
}
-end_cpu_access:
- drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-vunmap:
- drm_gem_fb_vunmap(fb, map);
-
- return ret;
+ return 0;
}
struct gud_usb_bulk_context {
@@ -285,6 +268,7 @@ static int gud_usb_bulk(struct gud_device *gdrm, size_t len)
}
static int gud_flush_rect(struct gud_device *gdrm, struct drm_framebuffer *fb,
+ const struct iosys_map *map, bool cached_reads,
const struct drm_format_info *format, struct drm_rect
*rect)
{
struct gud_set_buffer_req req;
@@ -293,7 +277,7 @@ sta
[PATCH 0/6] drm/gud: Use the shadow plane helper
From: Noralf Trønnes Hi, I have started to look at igt for testing and want to use CRC tests. To implement support for this I need to move away from the simple kms helper. When looking around for examples I came across Thomas' nice shadow helper and thought, yes this is perfect for drm/gud. So I'll switch to that before I move away from the simple kms helper. The async framebuffer flushing code path now uses a shadow buffer and doesn't touch the framebuffer when it shouldn't. I have also taken the opportunity to inline the synchronous flush code path since this will be the future default when userspace predominently don't run all displays in the same rendering loop. A shared rendering loop slows down all displays to run at the speed of the slowest one. Noralf. Cc: Maxime Ripard Cc: Thomas Zimmermann Cc: dri-devel@lists.freedesktop.org Signed-off-by: Noralf Trønnes --- Noralf Trønnes (6): drm/gem: shadow_fb_access: Prepare imported buffers for CPU access drm/gud: Fix UBSAN warning drm/gud: Don't retry a failed framebuffer flush drm/gud: Split up gud_flush_work() drm/gud: Prepare buffer for CPU access in gud_flush_work() drm/gud: Use the shadow plane helper drivers/gpu/drm/drm_gem_atomic_helper.c | 13 ++- drivers/gpu/drm/gud/gud_drv.c | 1 + drivers/gpu/drm/gud/gud_internal.h | 1 + drivers/gpu/drm/gud/gud_pipe.c | 198 +++- drivers/gpu/drm/solomon/ssd130x.c | 10 +- drivers/gpu/drm/tiny/gm12u320.c | 10 +- drivers/gpu/drm/tiny/ofdrm.c| 10 +- drivers/gpu/drm/tiny/simpledrm.c| 10 +- drivers/gpu/drm/udl/udl_modeset.c | 11 +- 9 files changed, 117 insertions(+), 147 deletions(-) --- base-commit: 7257702951305b1f0259c3468c39fc59d1ad4d8b change-id: 20221122-gud-shadow-plane-ae37a95d4d8d Best regards, -- Noralf Trønnes
[PATCH 2/6] drm/gud: Fix UBSAN warning
From: Noralf Trønnes
UBSAN complains about invalid value for bool:
[ 101.165172] [drm] Initialized gud 1.0.0 20200422 for 2-3.2:1.0 on minor 1
[ 101.213360] gud 2-3.2:1.0: [drm] fb1: guddrmfb frame buffer device
[ 101.213426] usbcore: registered new interface driver gud
[ 101.989431]
[ 101.989441] UBSAN: invalid-load in
/home/pi/linux/include/linux/iosys-map.h:253:9
[ 101.989447] load of value 121 is not a valid value for type '_Bool'
[ 101.989451] CPU: 1 PID: 455 Comm: kworker/1:6 Not tainted
5.18.0-rc5-gud-5.18-rc5 #3
[ 101.989456] Hardware name: Hewlett-Packard HP EliteBook 820 G1/1991, BIOS
L71 Ver. 01.44 04/12/2018
[ 101.989459] Workqueue: events_long gud_flush_work [gud]
[ 101.989471] Call Trace:
[ 101.989474]
[ 101.989479] dump_stack_lvl+0x49/0x5f
[ 101.989488] dump_stack+0x10/0x12
[ 101.989493] ubsan_epilogue+0x9/0x3b
[ 101.989498] __ubsan_handle_load_invalid_value.cold+0x44/0x49
[ 101.989504] dma_buf_vmap.cold+0x38/0x3d
[ 101.989511] ? find_busiest_group+0x48/0x300
[ 101.989520] drm_gem_shmem_vmap+0x76/0x1b0 [drm_shmem_helper]
[ 101.989528] drm_gem_shmem_object_vmap+0x9/0xb [drm_shmem_helper]
[ 101.989535] drm_gem_vmap+0x26/0x60 [drm]
[ 101.989594] drm_gem_fb_vmap+0x47/0x150 [drm_kms_helper]
[ 101.989630] gud_prep_flush+0xc1/0x710 [gud]
[ 101.989639] ? _raw_spin_lock+0x17/0x40
[ 101.989648] gud_flush_work+0x1e0/0x430 [gud]
[ 101.989653] ? __switch_to+0x11d/0x470
[ 101.989664] process_one_work+0x21f/0x3f0
[ 101.989673] worker_thread+0x200/0x3e0
[ 101.989679] ? rescuer_thread+0x390/0x390
[ 101.989684] kthread+0xfd/0x130
[ 101.989690] ? kthread_complete_and_exit+0x20/0x20
[ 101.989696] ret_from_fork+0x22/0x30
[ 101.989706]
[ 101.989708]
The source of this warning is in iosys_map_clear() called from
dma_buf_vmap(). It conditionally sets values based on map->is_iomem. The
iosys_map variables are allocated uninitialized on the stack leading to
->is_iomem having all kinds of values and not only 0/1.
Fix this by zeroing the iosys_map variables.
Fixes: 40e1a70b4aed ("drm: Add GUD USB Display driver")
Cc: # v5.18+
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/gud/gud_pipe.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 7c6dc2bcd14a..61f4abaf1811 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -157,8 +157,8 @@ static int gud_prep_flush(struct gud_device *gdrm, struct
drm_framebuffer *fb,
{
struct dma_buf_attachment *import_attach = fb->obj[0]->import_attach;
u8 compression = gdrm->compression;
- struct iosys_map map[DRM_FORMAT_MAX_PLANES];
- struct iosys_map map_data[DRM_FORMAT_MAX_PLANES];
+ struct iosys_map map[DRM_FORMAT_MAX_PLANES] = { };
+ struct iosys_map map_data[DRM_FORMAT_MAX_PLANES] = { };
struct iosys_map dst;
void *vaddr, *buf;
size_t pitch, len;
--
b4 0.10.1
[PATCH 3/6] drm/gud: Don't retry a failed framebuffer flush
From: Noralf Trønnes
If a framebuffer flush fails the driver will do one retry by requeing the
worker. Currently the worker is used even for synchronous flushing, but a
later patch will inline it, so this needs to change. Thinking about how to
solve this I came to the conclusion that this retry mechanism was a fix
for a problem that was only in the mind of the developer (me) and not
something that solved a real problem.
So let's remove this for now and revisit later should it become necessary.
gud_add_damage() has now only one caller so it can be inlined.
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/gud/gud_pipe.c | 48 +++---
1 file changed, 8 insertions(+), 40 deletions(-)
diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 61f4abaf1811..ff1358815af5 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -333,37 +333,6 @@ void gud_clear_damage(struct gud_device *gdrm)
gdrm->damage.y2 = 0;
}
-static void gud_add_damage(struct gud_device *gdrm, struct drm_rect *damage)
-{
- gdrm->damage.x1 = min(gdrm->damage.x1, damage->x1);
- gdrm->damage.y1 = min(gdrm->damage.y1, damage->y1);
- gdrm->damage.x2 = max(gdrm->damage.x2, damage->x2);
- gdrm->damage.y2 = max(gdrm->damage.y2, damage->y2);
-}
-
-static void gud_retry_failed_flush(struct gud_device *gdrm, struct
drm_framebuffer *fb,
- struct drm_rect *damage)
-{
- /*
-* pipe_update waits for the worker when the display mode is going to
change.
-* This ensures that the width and height is still the same making it
safe to
-* add back the damage.
-*/
-
- mutex_lock(>damage_lock);
- if (!gdrm->fb) {
- drm_framebuffer_get(fb);
- gdrm->fb = fb;
- }
- gud_add_damage(gdrm, damage);
- mutex_unlock(>damage_lock);
-
- /* Retry only once to avoid a possible storm in case of continues
errors. */
- if (!gdrm->prev_flush_failed)
- queue_work(system_long_wq, >work);
- gdrm->prev_flush_failed = true;
-}
-
void gud_flush_work(struct work_struct *work)
{
struct gud_device *gdrm = container_of(work, struct gud_device, work);
@@ -407,14 +376,10 @@ void gud_flush_work(struct work_struct *work)
ret = gud_flush_rect(gdrm, fb, format, );
if (ret) {
if (ret != -ENODEV && ret != -ECONNRESET &&
- ret != -ESHUTDOWN && ret != -EPROTO) {
- bool prev_flush_failed =
gdrm->prev_flush_failed;
-
- gud_retry_failed_flush(gdrm, fb, );
- if (!prev_flush_failed)
- dev_err_ratelimited(fb->dev->dev,
- "Failed to flush
framebuffer: error=%d\n", ret);
- }
+ ret != -ESHUTDOWN && ret != -EPROTO)
+ dev_err_ratelimited(fb->dev->dev,
+ "Failed to flush
framebuffer: error=%d\n", ret);
+ gdrm->prev_flush_failed = true;
break;
}
@@ -439,7 +404,10 @@ static void gud_fb_queue_damage(struct gud_device *gdrm,
struct drm_framebuffer
gdrm->fb = fb;
}
- gud_add_damage(gdrm, damage);
+ gdrm->damage.x1 = min(gdrm->damage.x1, damage->x1);
+ gdrm->damage.y1 = min(gdrm->damage.y1, damage->y1);
+ gdrm->damage.x2 = max(gdrm->damage.x2, damage->x2);
+ gdrm->damage.y2 = max(gdrm->damage.y2, damage->y2);
mutex_unlock(>damage_lock);
--
b4 0.10.1
[PATCH 1/6] drm/gem: shadow_fb_access: Prepare imported buffers for CPU access
From: Noralf Trønnes
Complete the shadow fb access functions by also preparing imported buffers
for CPU access. Update the affected drivers that currently use
drm_gem_fb_begin_cpu_access().
Through this change the following SHMEM drivers will now also make sure
their imported buffers are prepared for CPU access: cirrus, hyperv,
mgag200, vkms
Cc: Thomas Zimmermann
Cc: Javier Martinez Canillas
Cc: Hans de Goede
Cc: Dave Airlie
Signed-off-by: Noralf Trønnes
---
drivers/gpu/drm/drm_gem_atomic_helper.c | 13 -
drivers/gpu/drm/solomon/ssd130x.c | 10 +-
drivers/gpu/drm/tiny/gm12u320.c | 10 +-
drivers/gpu/drm/tiny/ofdrm.c| 10 ++
drivers/gpu/drm/tiny/simpledrm.c| 10 ++
drivers/gpu/drm/udl/udl_modeset.c | 11 ++-
6 files changed, 20 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem_atomic_helper.c
b/drivers/gpu/drm/drm_gem_atomic_helper.c
index e42800718f51..0eef4bb30d25 100644
--- a/drivers/gpu/drm/drm_gem_atomic_helper.c
+++ b/drivers/gpu/drm/drm_gem_atomic_helper.c
@@ -368,6 +368,7 @@ EXPORT_SYMBOL(drm_gem_reset_shadow_plane);
* maps all buffer objects of the plane's framebuffer into kernel address
* space and stores them in struct _shadow_plane_state.map. The first data
* bytes are available in struct _shadow_plane_state.data.
+ * It also prepares imported buffers for CPU access.
*
* See drm_gem_end_shadow_fb_access() for cleanup.
*
@@ -378,11 +379,20 @@ int drm_gem_begin_shadow_fb_access(struct drm_plane
*plane, struct drm_plane_sta
{
struct drm_shadow_plane_state *shadow_plane_state =
to_drm_shadow_plane_state(plane_state);
struct drm_framebuffer *fb = plane_state->fb;
+ int ret;
if (!fb)
return 0;
- return drm_gem_fb_vmap(fb, shadow_plane_state->map,
shadow_plane_state->data);
+ ret = drm_gem_fb_vmap(fb, shadow_plane_state->map,
shadow_plane_state->data);
+ if (ret)
+ return ret;
+
+ ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
+ if (ret)
+ drm_gem_fb_vunmap(fb, shadow_plane_state->map);
+
+ return ret;
}
EXPORT_SYMBOL(drm_gem_begin_shadow_fb_access);
@@ -404,6 +414,7 @@ void drm_gem_end_shadow_fb_access(struct drm_plane *plane,
struct drm_plane_stat
if (!fb)
return;
+ drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
drm_gem_fb_vunmap(fb, shadow_plane_state->map);
}
EXPORT_SYMBOL(drm_gem_end_shadow_fb_access);
diff --git a/drivers/gpu/drm/solomon/ssd130x.c
b/drivers/gpu/drm/solomon/ssd130x.c
index 53464afc2b9a..58a2f0113f24 100644
--- a/drivers/gpu/drm/solomon/ssd130x.c
+++ b/drivers/gpu/drm/solomon/ssd130x.c
@@ -544,7 +544,6 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer *fb,
const struct iosys_m
struct ssd130x_device *ssd130x = drm_to_ssd130x(fb->dev);
struct iosys_map dst;
unsigned int dst_pitch;
- int ret = 0;
u8 *buf = NULL;
/* Align y to display page boundaries */
@@ -556,21 +555,14 @@ static int ssd130x_fb_blit_rect(struct drm_framebuffer
*fb, const struct iosys_m
if (!buf)
return -ENOMEM;
- ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
- if (ret)
- goto out_free;
-
iosys_map_set_vaddr(, buf);
drm_fb_xrgb_to_mono(, _pitch, vmap, fb, rect);
- drm_gem_fb_end_cpu_access(fb, DMA_FROM_DEVICE);
-
ssd130x_update_rect(ssd130x, buf, rect);
-out_free:
kfree(buf);
- return ret;
+ return 0;
}
static void ssd130x_primary_plane_helper_atomic_update(struct drm_plane *plane,
diff --git a/drivers/gpu/drm/tiny/gm12u320.c b/drivers/gpu/drm/tiny/gm12u320.c
index 130fd07a967d..59aad4b468cc 100644
--- a/drivers/gpu/drm/tiny/gm12u320.c
+++ b/drivers/gpu/drm/tiny/gm12u320.c
@@ -252,7 +252,7 @@ static void gm12u320_32bpp_to_24bpp_packed(u8 *dst, u8
*src, int len)
static void gm12u320_copy_fb_to_blocks(struct gm12u320_device *gm12u320)
{
- int block, dst_offset, len, remain, ret, x1, x2, y1, y2;
+ int block, dst_offset, len, remain, x1, x2, y1, y2;
struct drm_framebuffer *fb;
void *vaddr;
u8 *src;
@@ -269,12 +269,6 @@ static void gm12u320_copy_fb_to_blocks(struct
gm12u320_device *gm12u320)
y2 = gm12u320->fb_update.rect.y2;
vaddr = gm12u320->fb_update.src_map.vaddr; /* TODO: Use mapping
abstraction properly */
- ret = drm_gem_fb_begin_cpu_access(fb, DMA_FROM_DEVICE);
- if (ret) {
- GM12U320_ERR("drm_gem_fb_begin_cpu_access err: %d\n", ret);
- goto put_fb;
- }
-
src = vaddr + y1 * fb->pitches[0] + x1 * 4;
x1 += (GM12U320_REAL_WIDTH - GM12U320_USER_WIDTH) / 2;
@@ -309,8 +303,6 @@ static void gm12u320_copy_fb_to_blocks(struct
gm12u320_device *gm1
Re: git send-email friendly smtp provider anyone?
Den 22.11.2022 19.50, skrev Konstantin Ryabitsev:
> On Tue, Nov 22, 2022 at 06:42:19PM +0100, Noralf Trønnes wrote:
>> The first thing that strikes me is that everyone mentioned in one of the
>> patches get the entire patchset, even sta...@vger.kernel.org (cc'ed in a
>> fixes patch). The first patch touches a core file and as a result a few
>> drivers, so I've cc'ed the driver maintainers in that patch, but now
>> they get the entire patchset where 5 of 6 patches is about a driver that
>> I maintain. So from their point of view, they see a patchset about a
>> driver they don't care about and a patch touching a core file, but from
>> the subject it's not apparent that it touches their driver. I'm afraid
>> that this might result in none of them looking at that patch. In this
>> particular case it's not that important, but in another case it might be.
>
> I did some (unscientific) polling among kernel maintainers and, by a vast
> margin, they always prefer to receive the entire series instead of
> cherry-picked patches -- having the entire series helps provide important
> context for the change they are looking at.
>
> So, this is deliberate and, for now at least, not configurable. Unless you're
> sending 100+ patch series, I doubt anyone will have any problem with receiving
> the whole series instead of individual patches.
>
>> As for the setting up the web endpoint, should I just follow the b4 docs
>> on that?
>>
>> I use b4 version 0.10.1, is that recent enough?
>
> Yes. There will be a 0.10.2 in the near future, but the incoming fixes
> shouldn't make much difference for the b4 send code.
>
This is what I got:
$ b4 send --web-auth-verify
Signing challenge
Submitting verification to https://lkml.kernel.org/_b4_submit
Traceback (most recent call last):
File "/home/pi/.local/bin/b4", line 8, in
sys.exit(cmd())
File "/home/pi/.local/lib/python3.10/site-packages/b4/command.py",
line 341, in cmd
cmdargs.func(cmdargs)
File "/home/pi/.local/lib/python3.10/site-packages/b4/command.py",
line 86, in cmd_send
b4.ez.cmd_send(cmdargs)
File "/home/pi/.local/lib/python3.10/site-packages/b4/ez.py", line
1102, in cmd_send
auth_verify(cmdargs)
File "/home/pi/.local/lib/python3.10/site-packages/b4/ez.py", line
188, in auth_verify
res = ses.post(endpoint, json=req)
File "/usr/lib/python3/dist-packages/requests/sessions.py", line 590,
in post
return self.request('POST', url, data=data, json=json, **kwargs)
File "/usr/lib/python3/dist-packages/requests/sessions.py", line 528,
in request
prep = self.prepare_request(req)
File "/usr/lib/python3/dist-packages/requests/sessions.py", line 456,
in prepare_request
p.prepare(
File "/usr/lib/python3/dist-packages/requests/models.py", line 319, in
prepare
self.prepare_body(data, files, json)
File "/usr/lib/python3/dist-packages/requests/models.py", line 469, in
prepare_body
body = complexjson.dumps(json)
File "/usr/lib/python3.10/json/__init__.py", line 231, in dumps
return _default_encoder.encode(obj)
File "/usr/lib/python3.10/json/encoder.py", line 199, in encode
chunks = self.iterencode(o, _one_shot=True)
File "/usr/lib/python3.10/json/encoder.py", line 257, in iterencode
return _iterencode(o, 0)
File "/usr/lib/python3.10/json/encoder.py", line 179, in default
raise TypeError(f'Object of type {o.__class__.__name__} '
TypeError: Object of type bytes is not JSON serializable
$ python3 --version
Python 3.10.6
Turning on debug output didn't add much:
$ b4 -d send --web-auth-verify 7ad470b4-f531-4632-8093-738d4d3e5d88
Running git --no-pager rev-parse --show-toplevel
Running git --no-pager config -z --get-regexp b4\..*
Running git --no-pager config -z --get-regexp gpg\..*
Running git --no-pager config -z --get-regexp user\..*
Signing challenge
Submitting verification to https://lkml.kernel.org/_b4_submit
Traceback (most recent call last):
File "/home/pi/.local/bin/b4", line 8, in
sys.exit(cmd())
File "/home/pi/.local/lib/python3.10/site-packages/b4/command.py",
line 341, in cmd
cmdargs.func(cmdargs)
File "/home/pi/.local/lib/python3.10/site-packages/b4/command.py",
line 86, in cmd_send
b4.ez.cmd_send(cmdargs)
File "/home/pi/.local/lib/python3.10/site-packages/b4/ez.py", line
1102, in cmd_send
auth_verify(cmdargs)
File "/home/pi/.local/lib/python3.10/site-packages/b4/ez.py", line
188, in auth_verify
res = ses.post(endpoint, json=req)
File "/usr/lib/python3/dist-packages/requests/sessions.py", line 590,
in post
return self.request('POST', url, data=data, json=json, **kwargs)
File "/usr/lib/python3/dist-packages/requests/sess
