[RFC] drm/dsi-mipi: Fix byte order of DCS set/get brightness
Commit Message
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.
Fixes: 1a9d759331b8 ("drm/dsi: Implement DCS set/get display brightness")
Signed-off-by: Daniel Mentz <danielmentz@google.com>
Change-Id: I24306e21ec6a5ff48ea121d977419a81d5b44152
Link: https://android.googlesource.com/kernel/msm/+/754affd62d0ee268c686c53169b1dbb7deac8550
[richard: fix 16-bit brightness_get; change brightness call by all panels on the -next tree]
Signed-off-by: Richard Acayan <mailingradian@gmail.com>
---
drivers/gpu/drm/drm_mipi_dsi.c | 44 ++++++++++++++++---
.../drm/panel/panel-asus-z00t-tm5p5-n35596.c | 4 +-
.../gpu/drm/panel/panel-boe-bf060y8m-aj0.c | 4 +-
drivers/gpu/drm/panel/panel-ebbg-ft8719.c | 2 +-
drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c | 2 +-
.../gpu/drm/panel/panel-jdi-lt070me05000.c | 4 +-
drivers/gpu/drm/panel/panel-raydium-rm67191.c | 4 +-
.../gpu/drm/panel/panel-samsung-s6e63j0x03.c | 2 +-
drivers/gpu/drm/panel/panel-samsung-sofef00.c | 4 +-
.../panel/panel-sony-tulip-truly-nt35521.c | 4 +-
include/drm/drm_mipi_dsi.h | 4 +-
11 files changed, 55 insertions(+), 23 deletions(-)
Comments
On Thu, Jan 12, 2023 at 11:18:48PM -0500, 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.
>
> Fixes: 1a9d759331b8 ("drm/dsi: Implement DCS set/get display brightness")
> Signed-off-by: Daniel Mentz <danielmentz@google.com>
> Change-Id: I24306e21ec6a5ff48ea121d977419a81d5b44152
> Link: https://android.googlesource.com/kernel/msm/+/754affd62d0ee268c686c53169b1dbb7deac8550
> [richard: fix 16-bit brightness_get; change brightness call by all panels on the -next tree]
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
> drivers/gpu/drm/drm_mipi_dsi.c | 44 ++++++++++++++++---
> .../drm/panel/panel-asus-z00t-tm5p5-n35596.c | 4 +-
> .../gpu/drm/panel/panel-boe-bf060y8m-aj0.c | 4 +-
> drivers/gpu/drm/panel/panel-ebbg-ft8719.c | 2 +-
> drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c | 2 +-
> .../gpu/drm/panel/panel-jdi-lt070me05000.c | 4 +-
> drivers/gpu/drm/panel/panel-raydium-rm67191.c | 4 +-
> .../gpu/drm/panel/panel-samsung-s6e63j0x03.c | 2 +-
> drivers/gpu/drm/panel/panel-samsung-sofef00.c | 4 +-
> .../panel/panel-sony-tulip-truly-nt35521.c | 4 +-
> include/drm/drm_mipi_dsi.h | 4 +-
> 11 files changed, 55 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 497ef4b6a90a..1e6dbf986d97 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -1180,17 +1180,33 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_scanline);
> * display
> * @dsi: DSI peripheral device
> * @brightness: brightness value
> + * @num_params: Number of parameters (bytes) to encode brightness value in. The
> + * MIPI specification states that one parameter shall be sent for
> + * devices that support 8-bit brightness levels. For devices that
> + * support brightness levels wider than 8-bit, two parameters
> + * shall be sent.
Only one brightness parameter is always sent, no matter how large it is. This
isn't the number of parameters, but the number of bytes ("num_bytes").
If there are always one or two bytes, would it be better to have this as a
"large" boolean?
> *
> * Return: 0 on success or a negative error code on failure.
> */
> int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi,
> - u16 brightness)
> + u16 brightness, size_t num_params)
> {
> - u8 payload[2] = { brightness & 0xff, brightness >> 8 };
> + u8 payload[2];
> ssize_t err;
>
> + switch (num_params) {
> + case 1:
> + payload[0] = brightness & 0xff;
> + break;
> + case 2:
> + payload[0] = brightness >> 8;
> + payload[1] = brightness & 0xff;
> + break;
> + default:
> + return -EINVAL;
> + }
> err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
> - payload, sizeof(payload));
> + payload, num_params);
> if (err < 0)
> return err;
>
> @@ -1203,16 +1219,25 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_display_brightness);
> * of the display
> * @dsi: DSI peripheral device
> * @brightness: brightness value
> + * @num_params: Number of parameters (i.e. bytes) the brightness value is
> + * encoded in. The MIPI specification states that one parameter
> + * shall be returned from devices that support 8-bit brightness
> + * levels. Devices that support brightness levels wider than
> + * 8-bit return two parameters (i.e. bytes).
> *
> * Return: 0 on success or a negative error code on failure.
> */
> int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
> - u16 *brightness)
> + u16 *brightness, size_t num_params)
> {
> + u8 payload[2];
> ssize_t err;
>
> + if (!(num_params == 1 || num_params == 2))
> + return -EINVAL;
> +
> err = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_DISPLAY_BRIGHTNESS,
> - brightness, sizeof(*brightness));
> + payload, num_params);
> if (err <= 0) {
> if (err == 0)
> err = -ENODATA;
> @@ -1220,6 +1245,15 @@ int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
> return err;
> }
>
> + switch (num_params) {
> + case 1:
> + *brightness = payload[0];
> + break;
> + case 2:
> + *brightness = (payload[0] << 8) | payload[1];
> + break;
> + }
> +
> return 0;
> }
> EXPORT_SYMBOL(mipi_dsi_dcs_get_display_brightness);
> diff --git a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
> index b3235781e6ba..9f849c68d57c 100644
> --- a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
> +++ b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
> @@ -220,7 +220,7 @@ static int tm5p5_nt35596_bl_update_status(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
> if (ret < 0)
> return ret;
>
> @@ -237,7 +237,7 @@ static int tm5p5_nt35596_bl_get_brightness(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c b/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
> index ad58840eda41..5a0366366055 100644
> --- a/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
> +++ b/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
> @@ -253,7 +253,7 @@ static int boe_bf060y8m_aj0_bl_update_status(struct backlight_device *bl)
> u16 brightness = backlight_get_brightness(bl);
> int ret;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
> if (ret < 0)
> return ret;
>
> @@ -266,7 +266,7 @@ static int boe_bf060y8m_aj0_bl_get_brightness(struct backlight_device *bl)
> u16 brightness;
> int ret;
>
> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/panel/panel-ebbg-ft8719.c b/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
> index e85d63a176d0..0666e0390c7c 100644
> --- a/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
> +++ b/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
> @@ -62,7 +62,7 @@ static int ebbg_ft8719_on(struct ebbg_ft8719 *ctx)
>
> dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
> if (ret < 0) {
> dev_err(dev, "Failed to set display brightness: %d\n", ret);
> return ret;
> diff --git a/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c b/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
> index d8765b2294fb..47ebf7b7da4a 100644
> --- a/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
> +++ b/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
> @@ -102,7 +102,7 @@ static int jdi_fhd_r63452_on(struct jdi_fhd_r63452 *ctx)
> return ret;
> }
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
> if (ret < 0) {
> dev_err(dev, "Failed to set display brightness: %d\n", ret);
> return ret;
> diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> index 8f4f137a2af6..7d2f147a2504 100644
> --- a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> +++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> @@ -332,7 +332,7 @@ static int dsi_dcs_bl_get_brightness(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
> if (ret < 0)
> return ret;
>
> @@ -348,7 +348,7 @@ static int dsi_dcs_bl_update_status(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm67191.c b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> index dbb1ed4efbed..00f6bd163af2 100644
> --- a/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> +++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> @@ -473,7 +473,7 @@ static int rad_bl_get_brightness(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
> if (ret < 0)
> return ret;
>
> @@ -493,7 +493,7 @@ static int rad_bl_update_status(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c b/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> index e06fd35de814..f5e235db7ad2 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> @@ -365,7 +365,7 @@ static int s6e63j0x03_enable(struct drm_panel *panel)
> return ret;
>
> /* set default white brightness */
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/panel/panel-samsung-sofef00.c b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
> index 1a0d24595faa..b481195c9b8f 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-sofef00.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
> @@ -224,10 +224,8 @@ static int sofef00_panel_bl_update_status(struct backlight_device *bl)
> u16 brightness;
>
> 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(dsi, brightness, 2);
> if (err < 0)
> return err;
>
> diff --git a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> index fa9be3c299c0..dbf179fdda88 100644
> --- a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> +++ b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> @@ -420,7 +420,7 @@ static int truly_nt35521_bl_update_status(struct backlight_device *bl)
> u16 brightness = backlight_get_brightness(bl);
> int ret;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
> if (ret < 0)
> return ret;
>
> @@ -433,7 +433,7 @@ static int truly_nt35521_bl_get_brightness(struct backlight_device *bl)
> u16 brightness;
> int ret;
>
> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 4f503d99f668..da02f09f251b 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -293,9 +293,9 @@ int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
> int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format);
> int mipi_dsi_dcs_set_tear_scanline(struct mipi_dsi_device *dsi, u16 scanline);
> int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi,
> - u16 brightness);
> + u16 brightness, size_t num_params);
> int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
> - u16 *brightness);
> + u16 *brightness, size_t num_params);
>
> /**
> * mipi_dsi_generic_write_seq - transmit data using a generic write packet
> --
> 2.39.0
>
Hi Richard/Daniel.
On Thu, Jan 12, 2023 at 11:18:48PM -0500, 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.
It is only a few week ago someone write they needed a byteswap to use
the kernel provided mipi_dsi_dcs_set_display_brightness(). I did not
realize then that this is because it is buggy.
Some comments on the patch:
- It would be nice to split it up so first patch fixes that the
parameters are in big-endian. This would go for both set and get.
- Second patch could then introduce the possibility to a driver to
use a variant that matches the display.
I would suggest to go for two functions:
mipi_dsi_dcs_set_display_brightness()
mipi_dsi_dcs_set_display_brightness_two()
So it is obvious when you go for the two byte version without adding a
bool parameter.
If the implementation is done using a helper or two independent
functions are up to you.
The patch split would be nice, but at least we should have two functions
and not a bool parameter.
Both patches should have the same Fixes tag.
Sam
>
> Fixes: 1a9d759331b8 ("drm/dsi: Implement DCS set/get display brightness")
> Signed-off-by: Daniel Mentz <danielmentz@google.com>
> Change-Id: I24306e21ec6a5ff48ea121d977419a81d5b44152
> Link: https://android.googlesource.com/kernel/msm/+/754affd62d0ee268c686c53169b1dbb7deac8550
> [richard: fix 16-bit brightness_get; change brightness call by all panels on the -next tree]
> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
> ---
> drivers/gpu/drm/drm_mipi_dsi.c | 44 ++++++++++++++++---
> .../drm/panel/panel-asus-z00t-tm5p5-n35596.c | 4 +-
> .../gpu/drm/panel/panel-boe-bf060y8m-aj0.c | 4 +-
> drivers/gpu/drm/panel/panel-ebbg-ft8719.c | 2 +-
> drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c | 2 +-
> .../gpu/drm/panel/panel-jdi-lt070me05000.c | 4 +-
> drivers/gpu/drm/panel/panel-raydium-rm67191.c | 4 +-
> .../gpu/drm/panel/panel-samsung-s6e63j0x03.c | 2 +-
> drivers/gpu/drm/panel/panel-samsung-sofef00.c | 4 +-
> .../panel/panel-sony-tulip-truly-nt35521.c | 4 +-
> include/drm/drm_mipi_dsi.h | 4 +-
> 11 files changed, 55 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index 497ef4b6a90a..1e6dbf986d97 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -1180,17 +1180,33 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_scanline);
> * display
> * @dsi: DSI peripheral device
> * @brightness: brightness value
> + * @num_params: Number of parameters (bytes) to encode brightness value in. The
> + * MIPI specification states that one parameter shall be sent for
> + * devices that support 8-bit brightness levels. For devices that
> + * support brightness levels wider than 8-bit, two parameters
> + * shall be sent.
> *
> * Return: 0 on success or a negative error code on failure.
> */
> int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi,
> - u16 brightness)
> + u16 brightness, size_t num_params)
> {
> - u8 payload[2] = { brightness & 0xff, brightness >> 8 };
> + u8 payload[2];
> ssize_t err;
>
> + switch (num_params) {
> + case 1:
> + payload[0] = brightness & 0xff;
> + break;
> + case 2:
> + payload[0] = brightness >> 8;
> + payload[1] = brightness & 0xff;
> + break;
> + default:
> + return -EINVAL;
> + }
> err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
> - payload, sizeof(payload));
> + payload, num_params);
> if (err < 0)
> return err;
>
> @@ -1203,16 +1219,25 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_display_brightness);
> * of the display
> * @dsi: DSI peripheral device
> * @brightness: brightness value
> + * @num_params: Number of parameters (i.e. bytes) the brightness value is
> + * encoded in. The MIPI specification states that one parameter
> + * shall be returned from devices that support 8-bit brightness
> + * levels. Devices that support brightness levels wider than
> + * 8-bit return two parameters (i.e. bytes).
> *
> * Return: 0 on success or a negative error code on failure.
> */
> int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
> - u16 *brightness)
> + u16 *brightness, size_t num_params)
> {
> + u8 payload[2];
> ssize_t err;
>
> + if (!(num_params == 1 || num_params == 2))
> + return -EINVAL;
> +
> err = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_DISPLAY_BRIGHTNESS,
> - brightness, sizeof(*brightness));
> + payload, num_params);
> if (err <= 0) {
> if (err == 0)
> err = -ENODATA;
> @@ -1220,6 +1245,15 @@ int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
> return err;
> }
>
> + switch (num_params) {
> + case 1:
> + *brightness = payload[0];
> + break;
> + case 2:
> + *brightness = (payload[0] << 8) | payload[1];
> + break;
> + }
> +
> return 0;
> }
> EXPORT_SYMBOL(mipi_dsi_dcs_get_display_brightness);
> diff --git a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
> index b3235781e6ba..9f849c68d57c 100644
> --- a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
> +++ b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
> @@ -220,7 +220,7 @@ static int tm5p5_nt35596_bl_update_status(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
> if (ret < 0)
> return ret;
>
> @@ -237,7 +237,7 @@ static int tm5p5_nt35596_bl_get_brightness(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c b/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
> index ad58840eda41..5a0366366055 100644
> --- a/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
> +++ b/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
> @@ -253,7 +253,7 @@ static int boe_bf060y8m_aj0_bl_update_status(struct backlight_device *bl)
> u16 brightness = backlight_get_brightness(bl);
> int ret;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
> if (ret < 0)
> return ret;
>
> @@ -266,7 +266,7 @@ static int boe_bf060y8m_aj0_bl_get_brightness(struct backlight_device *bl)
> u16 brightness;
> int ret;
>
> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/panel/panel-ebbg-ft8719.c b/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
> index e85d63a176d0..0666e0390c7c 100644
> --- a/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
> +++ b/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
> @@ -62,7 +62,7 @@ static int ebbg_ft8719_on(struct ebbg_ft8719 *ctx)
>
> dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
> if (ret < 0) {
> dev_err(dev, "Failed to set display brightness: %d\n", ret);
> return ret;
> diff --git a/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c b/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
> index d8765b2294fb..47ebf7b7da4a 100644
> --- a/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
> +++ b/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
> @@ -102,7 +102,7 @@ static int jdi_fhd_r63452_on(struct jdi_fhd_r63452 *ctx)
> return ret;
> }
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
> if (ret < 0) {
> dev_err(dev, "Failed to set display brightness: %d\n", ret);
> return ret;
> diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> index 8f4f137a2af6..7d2f147a2504 100644
> --- a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> +++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
> @@ -332,7 +332,7 @@ static int dsi_dcs_bl_get_brightness(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
> if (ret < 0)
> return ret;
>
> @@ -348,7 +348,7 @@ static int dsi_dcs_bl_update_status(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm67191.c b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> index dbb1ed4efbed..00f6bd163af2 100644
> --- a/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> +++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
> @@ -473,7 +473,7 @@ static int rad_bl_get_brightness(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
> if (ret < 0)
> return ret;
>
> @@ -493,7 +493,7 @@ static int rad_bl_update_status(struct backlight_device *bl)
>
> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c b/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> index e06fd35de814..f5e235db7ad2 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
> @@ -365,7 +365,7 @@ static int s6e63j0x03_enable(struct drm_panel *panel)
> return ret;
>
> /* set default white brightness */
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/drivers/gpu/drm/panel/panel-samsung-sofef00.c b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
> index 1a0d24595faa..b481195c9b8f 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-sofef00.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
> @@ -224,10 +224,8 @@ static int sofef00_panel_bl_update_status(struct backlight_device *bl)
> u16 brightness;
>
> 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(dsi, brightness, 2);
> if (err < 0)
> return err;
>
> diff --git a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> index fa9be3c299c0..dbf179fdda88 100644
> --- a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> +++ b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
> @@ -420,7 +420,7 @@ static int truly_nt35521_bl_update_status(struct backlight_device *bl)
> u16 brightness = backlight_get_brightness(bl);
> int ret;
>
> - ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
> + ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
> if (ret < 0)
> return ret;
>
> @@ -433,7 +433,7 @@ static int truly_nt35521_bl_get_brightness(struct backlight_device *bl)
> u16 brightness;
> int ret;
>
> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
> if (ret < 0)
> return ret;
>
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 4f503d99f668..da02f09f251b 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -293,9 +293,9 @@ int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
> int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format);
> int mipi_dsi_dcs_set_tear_scanline(struct mipi_dsi_device *dsi, u16 scanline);
> int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi,
> - u16 brightness);
> + u16 brightness, size_t num_params);
> int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
> - u16 *brightness);
> + u16 *brightness, size_t num_params);
>
> /**
> * mipi_dsi_generic_write_seq - transmit data using a generic write packet
> --
> 2.39.0
On Fri, Jan 13, 2023 at 05:34:55PM +0100, Sam Ravnborg wrote:
> Hi Richard/Daniel.
Not sure if you saw the original commit that I linked.
This patch was written in 2017 for the Pixel 3 and Pixel 3 XL
smartphones. I didn't contact the original author before sending this,
since the kernel that this patch originated from is EOL.
This greeting is still fine because it would be great if the original
author took responsibility for getting this patch applied.
>
> On Thu, Jan 12, 2023 at 11:18:48PM -0500, 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.
> It is only a few week ago someone write they needed a byteswap to use
> the kernel provided mipi_dsi_dcs_set_display_brightness(). I did not
> realize then that this is because it is buggy.
I didn't even consider sending this patch when I initially saw it. I
would have performed the byte swap in a new S6E3FA7 panel driver just
like in SOFEF00.
>
> Some comments on the patch:
> - It would be nice to split it up so first patch fixes that the
> parameters are in big-endian. This would go for both set and get.
The brightness is only big endian if it's 16-bit.
Most MIPI panel drivers in the kernel tree only send an 8-bit
brightness, or none at all. For these panels, the brightness can be sent
as a 16-bit integer in little endian, as it has the same effect as an
8-bit integer, with a zero byte appended. The original MIPI brightness
functions work properly with no changes for 8-bit brightness panels
only.
If we want separate 8-bit (original) and 16-bit functions, the 8-bit
functions shouldn't pass the brightness in big endian. That doesn't mean
we shouldn't change them at all.
>
> - Second patch could then introduce the possibility to a driver to
> use a variant that matches the display.
> I would suggest to go for two functions:
> mipi_dsi_dcs_set_display_brightness()
> mipi_dsi_dcs_set_display_brightness_two()
I'm thinking of a "_large" or "_wide" suffix instead. This would make a
32-bit suffix tricky though, if it is ever needed.
> So it is obvious when you go for the two byte version without adding a
> bool parameter.
> If the implementation is done using a helper or two independent
> functions are up to you.
These get_display_brightness and set_display_brightness functions are
already helpers. I'll have them duplicated.
>
> The patch split would be nice, but at least we should have two functions
> and not a bool parameter.
Yes, that was a bad idea on my part. With separate functions for 16-bit
brightness, there is no API change, less panel driver touching, and
panel driver changes can happen in separate patches. Thank you for the
suggestion.
> Both patches should have the same Fixes tag.
>
> Sam
>
>
>>
>> Fixes: 1a9d759331b8 ("drm/dsi: Implement DCS set/get display brightness")
>> Signed-off-by: Daniel Mentz <danielmentz@google.com>
>> Change-Id: I24306e21ec6a5ff48ea121d977419a81d5b44152
>> Link: https://android.googlesource.com/kernel/msm/+/754affd62d0ee268c686c53169b1dbb7deac8550
>> [richard: fix 16-bit brightness_get; change brightness call by all panels on the -next tree]
>> Signed-off-by: Richard Acayan <mailingradian@gmail.com>
>> ---
>> drivers/gpu/drm/drm_mipi_dsi.c | 44 ++++++++++++++++---
>> .../drm/panel/panel-asus-z00t-tm5p5-n35596.c | 4 +-
>> .../gpu/drm/panel/panel-boe-bf060y8m-aj0.c | 4 +-
>> drivers/gpu/drm/panel/panel-ebbg-ft8719.c | 2 +-
>> drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c | 2 +-
>> .../gpu/drm/panel/panel-jdi-lt070me05000.c | 4 +-
>> drivers/gpu/drm/panel/panel-raydium-rm67191.c | 4 +-
>> .../gpu/drm/panel/panel-samsung-s6e63j0x03.c | 2 +-
>> drivers/gpu/drm/panel/panel-samsung-sofef00.c | 4 +-
>> .../panel/panel-sony-tulip-truly-nt35521.c | 4 +-
>> include/drm/drm_mipi_dsi.h | 4 +-
>> 11 files changed, 55 insertions(+), 23 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
>> index 497ef4b6a90a..1e6dbf986d97 100644
>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
>> @@ -1180,17 +1180,33 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_scanline);
>> * display
>> * @dsi: DSI peripheral device
>> * @brightness: brightness value
>> + * @num_params: Number of parameters (bytes) to encode brightness value in. The
>> + * MIPI specification states that one parameter shall be sent for
>> + * devices that support 8-bit brightness levels. For devices that
>> + * support brightness levels wider than 8-bit, two parameters
>> + * shall be sent.
>> *
>> * Return: 0 on success or a negative error code on failure.
>> */
>> int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi,
>> - u16 brightness)
>> + u16 brightness, size_t num_params)
>> {
>> - u8 payload[2] = { brightness & 0xff, brightness >> 8 };
>> + u8 payload[2];
>> ssize_t err;
>>
>> + switch (num_params) {
>> + case 1:
>> + payload[0] = brightness & 0xff;
>> + break;
>> + case 2:
>> + payload[0] = brightness >> 8;
>> + payload[1] = brightness & 0xff;
>> + break;
>> + default:
>> + return -EINVAL;
>> + }
>> err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
>> - payload, sizeof(payload));
>> + payload, num_params);
>> if (err < 0)
>> return err;
>>
>> @@ -1203,16 +1219,25 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_display_brightness);
>> * of the display
>> * @dsi: DSI peripheral device
>> * @brightness: brightness value
>> + * @num_params: Number of parameters (i.e. bytes) the brightness value is
>> + * encoded in. The MIPI specification states that one parameter
>> + * shall be returned from devices that support 8-bit brightness
>> + * levels. Devices that support brightness levels wider than
>> + * 8-bit return two parameters (i.e. bytes).
>> *
>> * Return: 0 on success or a negative error code on failure.
>> */
>> int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
>> - u16 *brightness)
>> + u16 *brightness, size_t num_params)
>> {
>> + u8 payload[2];
>> ssize_t err;
>>
>> + if (!(num_params == 1 || num_params == 2))
>> + return -EINVAL;
>> +
>> err = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_DISPLAY_BRIGHTNESS,
>> - brightness, sizeof(*brightness));
>> + payload, num_params);
>> if (err <= 0) {
>> if (err == 0)
>> err = -ENODATA;
>> @@ -1220,6 +1245,15 @@ int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
>> return err;
>> }
>>
>> + switch (num_params) {
>> + case 1:
>> + *brightness = payload[0];
>> + break;
>> + case 2:
>> + *brightness = (payload[0] << 8) | payload[1];
>> + break;
>> + }
>> +
>> return 0;
>> }
>> EXPORT_SYMBOL(mipi_dsi_dcs_get_display_brightness);
>> diff --git a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
>> index b3235781e6ba..9f849c68d57c 100644
>> --- a/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
>> +++ b/drivers/gpu/drm/panel/panel-asus-z00t-tm5p5-n35596.c
>> @@ -220,7 +220,7 @@ static int tm5p5_nt35596_bl_update_status(struct backlight_device *bl)
>>
>> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>>
>> - ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
>> + ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
>> if (ret < 0)
>> return ret;
>>
>> @@ -237,7 +237,7 @@ static int tm5p5_nt35596_bl_get_brightness(struct backlight_device *bl)
>>
>> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>>
>> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
>> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
>> if (ret < 0)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c b/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
>> index ad58840eda41..5a0366366055 100644
>> --- a/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
>> +++ b/drivers/gpu/drm/panel/panel-boe-bf060y8m-aj0.c
>> @@ -253,7 +253,7 @@ static int boe_bf060y8m_aj0_bl_update_status(struct backlight_device *bl)
>> u16 brightness = backlight_get_brightness(bl);
>> int ret;
>>
>> - ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
>> + ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
>> if (ret < 0)
>> return ret;
>>
>> @@ -266,7 +266,7 @@ static int boe_bf060y8m_aj0_bl_get_brightness(struct backlight_device *bl)
>> u16 brightness;
>> int ret;
>>
>> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
>> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
>> if (ret < 0)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/panel/panel-ebbg-ft8719.c b/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
>> index e85d63a176d0..0666e0390c7c 100644
>> --- a/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
>> +++ b/drivers/gpu/drm/panel/panel-ebbg-ft8719.c
>> @@ -62,7 +62,7 @@ static int ebbg_ft8719_on(struct ebbg_ft8719 *ctx)
>>
>> dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>>
>> - ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
>> + ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
>> if (ret < 0) {
>> dev_err(dev, "Failed to set display brightness: %d\n", ret);
>> return ret;
>> diff --git a/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c b/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
>> index d8765b2294fb..47ebf7b7da4a 100644
>> --- a/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
>> +++ b/drivers/gpu/drm/panel/panel-jdi-fhd-r63452.c
>> @@ -102,7 +102,7 @@ static int jdi_fhd_r63452_on(struct jdi_fhd_r63452 *ctx)
>> return ret;
>> }
>>
>> - ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
>> + ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
>> if (ret < 0) {
>> dev_err(dev, "Failed to set display brightness: %d\n", ret);
>> return ret;
>> diff --git a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
>> index 8f4f137a2af6..7d2f147a2504 100644
>> --- a/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
>> +++ b/drivers/gpu/drm/panel/panel-jdi-lt070me05000.c
>> @@ -332,7 +332,7 @@ static int dsi_dcs_bl_get_brightness(struct backlight_device *bl)
>>
>> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>>
>> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
>> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
>> if (ret < 0)
>> return ret;
>>
>> @@ -348,7 +348,7 @@ static int dsi_dcs_bl_update_status(struct backlight_device *bl)
>>
>> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>>
>> - ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
>> + ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness, 1);
>> if (ret < 0)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/panel/panel-raydium-rm67191.c b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
>> index dbb1ed4efbed..00f6bd163af2 100644
>> --- a/drivers/gpu/drm/panel/panel-raydium-rm67191.c
>> +++ b/drivers/gpu/drm/panel/panel-raydium-rm67191.c
>> @@ -473,7 +473,7 @@ static int rad_bl_get_brightness(struct backlight_device *bl)
>>
>> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>>
>> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
>> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
>> if (ret < 0)
>> return ret;
>>
>> @@ -493,7 +493,7 @@ static int rad_bl_update_status(struct backlight_device *bl)
>>
>> dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>>
>> - ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
>> + ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness, 1);
>> if (ret < 0)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c b/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
>> index e06fd35de814..f5e235db7ad2 100644
>> --- a/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
>> +++ b/drivers/gpu/drm/panel/panel-samsung-s6e63j0x03.c
>> @@ -365,7 +365,7 @@ static int s6e63j0x03_enable(struct drm_panel *panel)
>> return ret;
>>
>> /* set default white brightness */
>> - ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
>> + ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
>> if (ret < 0)
>> return ret;
>>
>> diff --git a/drivers/gpu/drm/panel/panel-samsung-sofef00.c b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
>> index 1a0d24595faa..b481195c9b8f 100644
>> --- a/drivers/gpu/drm/panel/panel-samsung-sofef00.c
>> +++ b/drivers/gpu/drm/panel/panel-samsung-sofef00.c
>> @@ -224,10 +224,8 @@ static int sofef00_panel_bl_update_status(struct backlight_device *bl)
>> u16 brightness;
>>
>> 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(dsi, brightness, 2);
>> if (err < 0)
>> return err;
>>
>> diff --git a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
>> index fa9be3c299c0..dbf179fdda88 100644
>> --- a/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
>> +++ b/drivers/gpu/drm/panel/panel-sony-tulip-truly-nt35521.c
>> @@ -420,7 +420,7 @@ static int truly_nt35521_bl_update_status(struct backlight_device *bl)
>> u16 brightness = backlight_get_brightness(bl);
>> int ret;
>>
>> - ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
>> + ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
>> if (ret < 0)
>> return ret;
>>
>> @@ -433,7 +433,7 @@ static int truly_nt35521_bl_get_brightness(struct backlight_device *bl)
>> u16 brightness;
>> int ret;
>>
>> - ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
>> + ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
>> if (ret < 0)
>> return ret;
>>
>> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
>> index 4f503d99f668..da02f09f251b 100644
>> --- a/include/drm/drm_mipi_dsi.h
>> +++ b/include/drm/drm_mipi_dsi.h
>> @@ -293,9 +293,9 @@ int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
>> int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format);
>> int mipi_dsi_dcs_set_tear_scanline(struct mipi_dsi_device *dsi, u16 scanline);
>> int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi,
>> - u16 brightness);
>> + u16 brightness, size_t num_params);
>> int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
>> - u16 *brightness);
>> + u16 *brightness, size_t num_params);
>>
>> /**
>> * mipi_dsi_generic_write_seq - transmit data using a generic write packet
>> --
>> 2.39.0
Hi Richard,
On Fri, Jan 13, 2023 at 04:24:23PM -0500, Richard Acayan wrote:
> On Fri, Jan 13, 2023 at 05:34:55PM +0100, Sam Ravnborg wrote:
> > Hi Richard/Daniel.
> Not sure if you saw the original commit that I linked.
>
> This patch was written in 2017 for the Pixel 3 and Pixel 3 XL
> smartphones. I didn't contact the original author before sending this,
> since the kernel that this patch originated from is EOL.
>
> This greeting is still fine because it would be great if the original
> author took responsibility for getting this patch applied.
>
> >
> > On Thu, Jan 12, 2023 at 11:18:48PM -0500, 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.
> > It is only a few week ago someone write they needed a byteswap to use
> > the kernel provided mipi_dsi_dcs_set_display_brightness(). I did not
> > realize then that this is because it is buggy.
> I didn't even consider sending this patch when I initially saw it. I
> would have performed the byte swap in a new S6E3FA7 panel driver just
> like in SOFEF00.
>
> >
> > Some comments on the patch:
> > - It would be nice to split it up so first patch fixes that the
> > parameters are in big-endian. This would go for both set and get.
> The brightness is only big endian if it's 16-bit.
>
> Most MIPI panel drivers in the kernel tree only send an 8-bit
> brightness, or none at all. For these panels, the brightness can be sent
> as a 16-bit integer in little endian, as it has the same effect as an
> 8-bit integer, with a zero byte appended. The original MIPI brightness
> functions work properly with no changes for 8-bit brightness panels
> only.
>
> If we want separate 8-bit (original) and 16-bit functions, the 8-bit
> functions shouldn't pass the brightness in big endian. That doesn't mean
> we shouldn't change them at all.
Agree, the brightness is passed exactly as returned by
backlight_get_brightness(), and the helper take care of the correct byte
ordering.
>
> >
> > - Second patch could then introduce the possibility to a driver to
> > use a variant that matches the display.
> > I would suggest to go for two functions:
> > mipi_dsi_dcs_set_display_brightness()
> > mipi_dsi_dcs_set_display_brightness_two()
> I'm thinking of a "_large" or "_wide" suffix instead. This would make a
> 32-bit suffix tricky though, if it is ever needed.
_large is way better than _two, and would be my choice.
>
> > So it is obvious when you go for the two byte version without adding a
> > bool parameter.
> > If the implementation is done using a helper or two independent
> > functions are up to you.
> These get_display_brightness and set_display_brightness functions are
> already helpers. I'll have them duplicated.
>
> >
> > The patch split would be nice, but at least we should have two functions
> > and not a bool parameter.
> Yes, that was a bad idea on my part. With separate functions for 16-bit
> brightness, there is no API change, less panel driver touching, and
> panel driver changes can happen in separate patches. Thank you for the
> suggestion.
Great, looks forward for the patches.
Sam
@@ -1180,17 +1180,33 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_tear_scanline);
* display
* @dsi: DSI peripheral device
* @brightness: brightness value
+ * @num_params: Number of parameters (bytes) to encode brightness value in. The
+ * MIPI specification states that one parameter shall be sent for
+ * devices that support 8-bit brightness levels. For devices that
+ * support brightness levels wider than 8-bit, two parameters
+ * shall be sent.
*
* Return: 0 on success or a negative error code on failure.
*/
int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi,
- u16 brightness)
+ u16 brightness, size_t num_params)
{
- u8 payload[2] = { brightness & 0xff, brightness >> 8 };
+ u8 payload[2];
ssize_t err;
+ switch (num_params) {
+ case 1:
+ payload[0] = brightness & 0xff;
+ break;
+ case 2:
+ payload[0] = brightness >> 8;
+ payload[1] = brightness & 0xff;
+ break;
+ default:
+ return -EINVAL;
+ }
err = mipi_dsi_dcs_write(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS,
- payload, sizeof(payload));
+ payload, num_params);
if (err < 0)
return err;
@@ -1203,16 +1219,25 @@ EXPORT_SYMBOL(mipi_dsi_dcs_set_display_brightness);
* of the display
* @dsi: DSI peripheral device
* @brightness: brightness value
+ * @num_params: Number of parameters (i.e. bytes) the brightness value is
+ * encoded in. The MIPI specification states that one parameter
+ * shall be returned from devices that support 8-bit brightness
+ * levels. Devices that support brightness levels wider than
+ * 8-bit return two parameters (i.e. bytes).
*
* Return: 0 on success or a negative error code on failure.
*/
int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
- u16 *brightness)
+ u16 *brightness, size_t num_params)
{
+ u8 payload[2];
ssize_t err;
+ if (!(num_params == 1 || num_params == 2))
+ return -EINVAL;
+
err = mipi_dsi_dcs_read(dsi, MIPI_DCS_GET_DISPLAY_BRIGHTNESS,
- brightness, sizeof(*brightness));
+ payload, num_params);
if (err <= 0) {
if (err == 0)
err = -ENODATA;
@@ -1220,6 +1245,15 @@ int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
return err;
}
+ switch (num_params) {
+ case 1:
+ *brightness = payload[0];
+ break;
+ case 2:
+ *brightness = (payload[0] << 8) | payload[1];
+ break;
+ }
+
return 0;
}
EXPORT_SYMBOL(mipi_dsi_dcs_get_display_brightness);
@@ -220,7 +220,7 @@ static int tm5p5_nt35596_bl_update_status(struct backlight_device *bl)
dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
- ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
+ ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
if (ret < 0)
return ret;
@@ -237,7 +237,7 @@ static int tm5p5_nt35596_bl_get_brightness(struct backlight_device *bl)
dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
- ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
+ ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
if (ret < 0)
return ret;
@@ -253,7 +253,7 @@ static int boe_bf060y8m_aj0_bl_update_status(struct backlight_device *bl)
u16 brightness = backlight_get_brightness(bl);
int ret;
- ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
+ ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
if (ret < 0)
return ret;
@@ -266,7 +266,7 @@ static int boe_bf060y8m_aj0_bl_get_brightness(struct backlight_device *bl)
u16 brightness;
int ret;
- ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
+ ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
if (ret < 0)
return ret;
@@ -62,7 +62,7 @@ static int ebbg_ft8719_on(struct ebbg_ft8719 *ctx)
dsi->mode_flags |= MIPI_DSI_MODE_LPM;
- ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
+ ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
if (ret < 0) {
dev_err(dev, "Failed to set display brightness: %d\n", ret);
return ret;
@@ -102,7 +102,7 @@ static int jdi_fhd_r63452_on(struct jdi_fhd_r63452 *ctx)
return ret;
}
- ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
+ ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
if (ret < 0) {
dev_err(dev, "Failed to set display brightness: %d\n", ret);
return ret;
@@ -332,7 +332,7 @@ static int dsi_dcs_bl_get_brightness(struct backlight_device *bl)
dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
- ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
+ ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
if (ret < 0)
return ret;
@@ -348,7 +348,7 @@ static int dsi_dcs_bl_update_status(struct backlight_device *bl)
dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
- ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
+ ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness, 1);
if (ret < 0)
return ret;
@@ -473,7 +473,7 @@ static int rad_bl_get_brightness(struct backlight_device *bl)
dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
- ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
+ ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
if (ret < 0)
return ret;
@@ -493,7 +493,7 @@ static int rad_bl_update_status(struct backlight_device *bl)
dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
- ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness);
+ ret = mipi_dsi_dcs_set_display_brightness(dsi, bl->props.brightness, 1);
if (ret < 0)
return ret;
@@ -365,7 +365,7 @@ static int s6e63j0x03_enable(struct drm_panel *panel)
return ret;
/* set default white brightness */
- ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff);
+ ret = mipi_dsi_dcs_set_display_brightness(dsi, 0x00ff, 1);
if (ret < 0)
return ret;
@@ -224,10 +224,8 @@ static int sofef00_panel_bl_update_status(struct backlight_device *bl)
u16 brightness;
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(dsi, brightness, 2);
if (err < 0)
return err;
@@ -420,7 +420,7 @@ static int truly_nt35521_bl_update_status(struct backlight_device *bl)
u16 brightness = backlight_get_brightness(bl);
int ret;
- ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness);
+ ret = mipi_dsi_dcs_set_display_brightness(dsi, brightness, 1);
if (ret < 0)
return ret;
@@ -433,7 +433,7 @@ static int truly_nt35521_bl_get_brightness(struct backlight_device *bl)
u16 brightness;
int ret;
- ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness);
+ ret = mipi_dsi_dcs_get_display_brightness(dsi, &brightness, 1);
if (ret < 0)
return ret;
@@ -293,9 +293,9 @@ int mipi_dsi_dcs_set_tear_on(struct mipi_dsi_device *dsi,
int mipi_dsi_dcs_set_pixel_format(struct mipi_dsi_device *dsi, u8 format);
int mipi_dsi_dcs_set_tear_scanline(struct mipi_dsi_device *dsi, u16 scanline);
int mipi_dsi_dcs_set_display_brightness(struct mipi_dsi_device *dsi,
- u16 brightness);
+ u16 brightness, size_t num_params);
int mipi_dsi_dcs_get_display_brightness(struct mipi_dsi_device *dsi,
- u16 *brightness);
+ u16 *brightness, size_t num_params);
/**
* mipi_dsi_generic_write_seq - transmit data using a generic write packet