[RFC PATCH v3 0/3] drm/mipi-dsi: 16-bit Brightness Endianness Fix

[RFC PATCH v3 0/3] drm/mipi-dsi: 16-bit Brightness Endianness Fix

[Richard Acayan]

Changes since v2 (20230114010006.50471-1-mailingradian@gmail.com):
 - patch vtdr6130 to use _large (3/3)
 - remove Change-Id again (1/3)
 - change patch subject (1-2/3)
 - correct function name in patch description (2/3)
 - add Tested-by tags (1-2/3)

Changes since v1 (20230113041848.200704-1-mailingradian@gmail.com):
 - move 16-bit brightness handling to new functions and revert API
   change (1/2)
 - remove Change-Id in compliance with checkpatch.pl (1/2)
 - separate panel driver changes (2/2)

This series adds proper support for 16-bit MIPI DSI brightness and
cleans up existing panel drivers with 16-bit brightness.

Both setting and getting works on an external S6E3FA7
(max_brightness = 1023) panel driver with the first patch.

Even though this originally fixed both 8-bit and 16-bit brightness, the
8-bit fix is omitted for now because it affects ~13 panels that would
need to be tested:

    panel-asus-z00t-tm5p5-n35596.c
    panel-boe-bf060y8m-aj0.c
    panel-dsi-cm.c
    panel-ebbg-ft8719.c
    panel-jdi-fhd-r63452.c
    panel-jdi-lt070me05000.c
    panel-novatek-nt35510.c
    panel-novatek-nt35560.c
    panel-orisetech-otm8009a.c
    panel-raydium-rm67191.c
    panel-samsung-s6e63j0x03.c
    panel-sony-acx565akm.c
    panel-sony-tulip-truly-nt35521.c

Daniel Mentz (1):
  drm/mipi-dsi: Fix byte order of 16-bit DCS set/get brightness

Richard Acayan (2):
  drm/panel: sofef00: Use 16-bit brightness function
  drm/panel: vtdr6130: Use 16-bit brightness function

 drivers/gpu/drm/drm_mipi_dsi.c                | 52 +++++++++++++++++++
 drivers/gpu/drm/panel/panel-samsung-sofef00.c |  9 +---
 .../gpu/drm/panel/panel-visionox-vtdr6130.c   |  5 +-
 include/drm/drm_mipi_dsi.h                    |  4 ++
 4 files changed, 59 insertions(+), 11 deletions(-)

-- 
2.39.0

[RFC PATCH v3 1/3] drm/mipi-dsi: Fix byte order of 16-bit DCS set/get brightness

[Richard Acayan]

From: Daniel Mentz <danielmentz@google.com>

The MIPI DCS specification demands that brightness values are sent in
big endian byte order. It also states that one parameter (i.e. one byte)
shall be sent/received for 8 bit wide values, and two parameters shall
be used for values that are between 9 and 16 bits wide.

Add new functions to properly handle 16-bit brightness in big endian,
since the two 8- and 16-bit cases are distinct from each other.

Fixes: 1a9d759331b8 ("drm/dsi: Implement DCS set/get display brightness")
Signed-off-by: Daniel Mentz <danielmentz@google.com>
Link: https://android.googlesource.com/kernel/msm/+/754affd62d0ee268c686c53169b1dbb7deac8550
[richard: fix 16-bit brightness_get]
[richard: use separate functions instead of switch/case]
[richard: split into 16-bit component]
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
Tested-by: Caleb Connolly <caleb@connolly.tech>
---
 drivers/gpu/drm/drm_mipi_dsi.c | 52 ++++++++++++++++++++++++++++++++++
 include/drm/drm_mipi_dsi.h     |  4 +++
 2 files changed, 56 insertions(+)

diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
index 497ef4b6a90a..4bc15fbd009d 100644
--- a/drivers/gpu/drm/drm_mipi_dsi.c
+++ b/drivers/gpu/drm/drm_mipi_dsi.c
@@ -1224,6 +1224,58 @@ int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
 }
 EXPORT_SYMBOL(mipi_dsi_dcs_get_display_brightness);
 
+/**
+ * mipi_dsi_dcs_set_display_brightness_large() - sets the 16-bit brightness value
+ *    of the display
+ * @dsi: DSI peripheral device
+ * @brightness: brightness value
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_set_display_brightness_large(struct mipi_dsi_device *dsi,
+					     u16 brightness)
+{
+	u8 payload[2] = { brightness >> 8, brightness & 0xff };
+	ssize_t err;
+
+	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
+				 payload, sizeof(payload));
+	if (err < 0)
+		return err;
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_set_display_brightness_large);
+
+/**
+ * mipi_dsi_dcs_get_display_brightness_large() - gets the current 16-bit
+ *    brightness value of the display
+ * @dsi: DSI peripheral device
+ * @brightness: brightness value
+ *
+ * Return: 0 on success or a negative error code on failure.
+ */
+int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
+					     u16 *brightness)
+{
+	u8 brightness_be[2];
+	ssize_t err;
+
+	err = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_DISPLAY_BRIGHTNESS,
+				brightness_be, sizeof(brightness_be));
+	if (err <= 0) {
+		if (err == 0)
+			err = -ENODATA;
+
+		return err;
+	}
+
+	*brightness = (brightness_be[0] << 8) | brightness_be[1];
+
+	return 0;
+}
+EXPORT_SYMBOL(mipi_dsi_dcs_get_display_brightness_large);
+
 static int mipi_dsi_drv_probe(struct device *dev)
 {
 	struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver);
diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
index 4f503d99f668..16f30975b22b 100644
--- a/include/drm/drm_mipi_dsi.h
+++ b/include/drm/drm_mipi_dsi.h
@@ -296,6 +296,10 @@ int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi,
 					u16 brightness);
 int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
 					u16 *brightness);
+int mipi_dsi_dcs_set_display_brightness_large(struct mipi_dsi_device *dsi,
+					     u16 brightness);
+int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
+					     u16 *brightness);
 
 /**
  * mipi_dsi_generic_write_seq - transmit data using a generic write packet
-- 
2.39.0

Re: [RFC PATCH v3 1/3] drm/mipi-dsi: Fix byte order of 16-bit DCS set/get brightness

[Neil Armstrong]

On 16/01/2023 23:49, Richard Acayan wrote:
> From: Daniel Mentz <danielmentz@google.com>
> 
> The MIPI DCS specification demands that brightness values are sent in
> big endian byte order. It also states that one parameter (i.e. one byte)
> shall be sent/received for 8 bit wide values, and two parameters shall
> be used for values that are between 9 and 16 bits wide.
> 
> Add new functions to properly handle 16-bit brightness in big endian,
> since the two 8- and 16-bit cases are distinct from each other.
> 
> Fixes: 1a9d759331b8 ("drm/dsi: Implement DCS set/get display brightness")
> Signed-off-by: Daniel Mentz <danielmentz@google.com>
> Link: https://android.googlesource.com/kernel/msm/+/754affd62d0ee268c686c53169b1dbb7deac8550
> [richard: fix 16-bit brightness_get]
> [richard: use separate functions instead of switch/case]
> [richard: split into 16-bit component]
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> Tested-by: Caleb Connolly <caleb@connolly.tech>
> ---
>   drivers/gpu/drm/drm_mipi_dsi.c | 52 ++++++++++++++++++++++++++++++++++
>   include/drm/drm_mipi_dsi.h     |  4 +++
>   2 files changed, 56 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 497ef4b6a90a..4bc15fbd009d 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -1224,6 +1224,58 @@ int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
>   }
>   EXPORT_SYMBOL(mipi_dsi_dcs_get_display_brightness);
>   
> +/**
> + * mipi_dsi_dcs_set_display_brightness_large() - sets the 16-bit brightness value
> + *    of the display
> + * @dsi: DSI peripheral device
> + * @brightness: brightness value
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int mipi_dsi_dcs_set_display_brightness_large(struct mipi_dsi_device *dsi,
> +					     u16 brightness)
> +{
> +	u8 payload[2] = { brightness >> 8, brightness & 0xff };
> +	ssize_t err;
> +
> +	err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
> +				 payload, sizeof(payload));
> +	if (err < 0)
> +		return err;
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mipi_dsi_dcs_set_display_brightness_large);
> +
> +/**
> + * mipi_dsi_dcs_get_display_brightness_large() - gets the current 16-bit
> + *    brightness value of the display
> + * @dsi: DSI peripheral device
> + * @brightness: brightness value
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
> +					     u16 *brightness)
> +{
> +	u8 brightness_be[2];
> +	ssize_t err;
> +
> +	err = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_DISPLAY_BRIGHTNESS,
> +				brightness_be, sizeof(brightness_be));
> +	if (err <= 0) {
> +		if (err == 0)
> +			err = -ENODATA;
> +
> +		return err;
> +	}
> +
> +	*brightness = (brightness_be[0] << 8) | brightness_be[1];
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL(mipi_dsi_dcs_get_display_brightness_large);
> +
>   static int mipi_dsi_drv_probe(struct device *dev)
>   {
>   	struct mipi_dsi_driver *drv = to_mipi_dsi_driver(dev->driver);
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 4f503d99f668..16f30975b22b 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -296,6 +296,10 @@ int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi,
>   					u16 brightness);
>   int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
>   					u16 *brightness);
> +int mipi_dsi_dcs_set_display_brightness_large(struct mipi_dsi_device *dsi,
> +					     u16 brightness);
> +int mipi_dsi_dcs_get_display_brightness_large(struct mipi_dsi_device *dsi,
> +					     u16 *brightness);
>   
>   /**
>    * mipi_dsi_generic_write_seq - transmit data using a generic write packet

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

[RFC PATCH v3 2/3] drm/panel: sofef00: Use 16-bit brightness function

[Richard Acayan]

These panels communicate brightness in big endian. This is not a quirk
of the panels themselves, but rather, a part of the MIPI standard. Use
the new mipi_dsi_dcs_set_display_brightness_large() function that
properly handles 16-bit brightness instead of doing special processing
of the brightness values.

Signed-off-by: Richard Acayan <mailingradian@gmail.com>
Tested-by: Caleb Connolly <caleb@connolly.tech>
---
 drivers/gpu/drm/panel/panel-samsung-sofef00.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-samsung-sofef00.c b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
index 9db49a028930..1ebb79e3103c 100644
--- a/drivers/gpu/drm/panel/panel-samsung-sofef00.c
+++ b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
@@ -10,7 +10,6 @@
 #include <linux/of.h>
 #include <linux/of_device.h>
 #include <linux/regulator/consumer.h>
-#include <linux/swab.h>
 #include <linux/backlight.h>
 
 #include <video/mipi_display.h>
@@ -213,13 +212,9 @@ static int sofef00_panel_bl_update_status(struct backlight_device *bl)
 {
 	struct mipi_dsi_device *dsi = bl_get_data(bl);
 	int err;
-	u16 brightness;
+	u16 brightness = (u16)backlight_get_brightness(bl);
 
-	brightness = (u16)backlight_get_brightness(bl);
-	// This panel needs the high and low bytes swapped for the brightness value
-	brightness = __swab16(brightness);
-
-	err = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
+	err = mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
 	if (err < 0)
 		return err;
 
-- 
2.39.0

Re: [RFC PATCH v3 2/3] drm/panel: sofef00: Use 16-bit brightness function

[Neil Armstrong]

On 16/01/2023 23:49, Richard Acayan wrote:
> These panels communicate brightness in big endian. This is not a quirk
> of the panels themselves, but rather, a part of the MIPI standard. Use
> the new mipi_dsi_dcs_set_display_brightness_large() function that
> properly handles 16-bit brightness instead of doing special processing
> of the brightness values.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> Tested-by: Caleb Connolly <caleb@connolly.tech>
> ---
>   drivers/gpu/drm/panel/panel-samsung-sofef00.c | 9 ++-------
>   1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-samsung-sofef00.c b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
> index 9db49a028930..1ebb79e3103c 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-sofef00.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
> @@ -10,7 +10,6 @@
>   #include <linux/of.h>
>   #include <linux/of_device.h>
>   #include <linux/regulator/consumer.h>
> -#include <linux/swab.h>
>   #include <linux/backlight.h>
>   
>   #include <video/mipi_display.h>
> @@ -213,13 +212,9 @@ static int sofef00_panel_bl_update_status(struct backlight_device *bl)
>   {
>   	struct mipi_dsi_device *dsi = bl_get_data(bl);
>   	int err;
> -	u16 brightness;
> +	u16 brightness = (u16)backlight_get_brightness(bl);
>   
> -	brightness = (u16)backlight_get_brightness(bl);
> -	// This panel needs the high and low bytes swapped for the brightness value
> -	brightness = __swab16(brightness);
> -
> -	err = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
> +	err = mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
>   	if (err < 0)
>   		return err;
>   

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

[RFC PATCH v3 3/3] drm/panel: vtdr6130: Use 16-bit brightness function

[Richard Acayan]

This panel communicates brightness in big endian. This is not a quirk of
the panels themselves, but rather, a part of the MIPI standard. Use the
new mipi_dsi_dcs_set_display_brightness_large() function that properly
handles 16-bit brightness instead of bypassing the brightness functions
entirely.

Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
 drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
index f9a6abc1e121..1092075b31a5 100644
--- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
+++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
@@ -243,12 +243,9 @@ static int visionox_vtdr6130_bl_update_status(struct backlight_device *bl)
 {
 	struct mipi_dsi_device *dsi = bl_get_data(bl);
 	u16 brightness = backlight_get_brightness(bl);
-	/* Panel needs big-endian order of brightness value */
-	u8 payload[2] = { brightness >> 8, brightness & 0xff };
 	int ret;
 
-	ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
-				 payload, sizeof(payload));
+	mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
 	if (ret < 0)
 		return ret;
 
-- 
2.39.0

Re: [RFC PATCH v3 3/3] drm/panel: vtdr6130: Use 16-bit brightness function

[Neil Armstrong]

On 16/01/2023 23:49, Richard Acayan wrote:
> This panel communicates brightness in big endian. This is not a quirk of
> the panels themselves, but rather, a part of the MIPI standard. Use the
> new mipi_dsi_dcs_set_display_brightness_large() function that properly
> handles 16-bit brightness instead of bypassing the brightness functions
> entirely.
> 
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
>   drivers/gpu/drm/panel/panel-visionox-vtdr6130.c | 5 +----
>   1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> index f9a6abc1e121..1092075b31a5 100644
> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c
> @@ -243,12 +243,9 @@ static int visionox_vtdr6130_bl_update_status(struct backlight_device *bl)
>   {
>   	struct mipi_dsi_device *dsi = bl_get_data(bl);
>   	u16 brightness = backlight_get_brightness(bl);
> -	/* Panel needs big-endian order of brightness value */
> -	u8 payload[2] = { brightness >> 8, brightness & 0xff };
>   	int ret;
>   
> -	ret = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
> -				 payload, sizeof(payload));
> +	mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
>   	if (ret < 0)
>   		return ret;
>   

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>
Tested-by: Neil Armstrong <neil.armstrong@linaro.org> # on SM8550-MTP

Thanks !

Re: [RFC PATCH v3 0/3] drm/mipi-dsi: 16-bit Brightness Endianness Fix

[Sam Ravnborg]

Hi Richard.
On Mon, Jan 16, 2023 at 05:49:06PM -0500, Richard Acayan wrote:
> Changes since v2 (20230114010006.50471-1-mailingradian@gmail.com):
>  - patch vtdr6130 to use _large (3/3)
>  - remove Change-Id again (1/3)
>  - change patch subject (1-2/3)
>  - correct function name in patch description (2/3)
>  - add Tested-by tags (1-2/3)
> 
> Changes since v1 (20230113041848.200704-1-mailingradian@gmail.com):
>  - move 16-bit brightness handling to new functions and revert API
>    change (1/2)
>  - remove Change-Id in compliance with checkpatch.pl (1/2)
>  - separate panel driver changes (2/2)
> 
> This series adds proper support for 16-bit MIPI DSI brightness and
> cleans up existing panel drivers with 16-bit brightness.

The series is:
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Neil - I hope you can land this in drm-misc.

	Sam

Re: [RFC PATCH v3 0/3] drm/mipi-dsi: 16-bit Brightness Endianness Fix

[Neil Armstrong]

On 19/01/2023 08:14, Sam Ravnborg wrote:
> Hi Richard.
> On Mon, Jan 16, 2023 at 05:49:06PM -0500, Richard Acayan wrote:
>> Changes since v2 (20230114010006.50471-1-mailingradian@gmail.com):
>>   - patch vtdr6130 to use _large (3/3)
>>   - remove Change-Id again (1/3)
>>   - change patch subject (1-2/3)
>>   - correct function name in patch description (2/3)
>>   - add Tested-by tags (1-2/3)
>>
>> Changes since v1 (20230113041848.200704-1-mailingradian@gmail.com):
>>   - move 16-bit brightness handling to new functions and revert API
>>     change (1/2)
>>   - remove Change-Id in compliance with checkpatch.pl (1/2)
>>   - separate panel driver changes (2/2)
>>
>> This series adds proper support for 16-bit MIPI DSI brightness and
>> cleans up existing panel drivers with 16-bit brightness.
> 
> The series is:
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> Neil - I hope you can land this in drm-misc.

Sure, done !

Thanks,
Neil

> 
> 	Sam

Re: [RFC PATCH v3 0/3] drm/mipi-dsi: 16-bit Brightness Endianness Fix

[Neil Armstrong]

Hi,

On Mon, 16 Jan 2023 17:49:06 -0500, Richard Acayan wrote:
> Changes since v2 (20230114010006.50471-1-mailingradian@gmail.com):
>  - patch vtdr6130 to use _large (3/3)
>  - remove Change-Id again (1/3)
>  - change patch subject (1-2/3)
>  - correct function name in patch description (2/3)
>  - add Tested-by tags (1-2/3)
> 
> [...]

Thanks, Applied to https://anongit.freedesktop.org/git/drm/drm-misc.git (drm-misc-next)

[1/3] drm/mipi-dsi: Fix byte order of 16-bit DCS set/get brightness
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=c9d27c6be518b4ef2966d9564654ef99292ea1b3
[2/3] drm/panel: sofef00: Use 16-bit brightness function
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=fd40749a4f62a03d0aebe6eb446ea84a9901795a
[3/3] drm/panel: vtdr6130: Use 16-bit brightness function
      https://cgit.freedesktop.org/drm/drm-misc/commit/?id=9402cde9347eca050e14ea9e47270e84a6899162

-- 
Neil