diff mbox series

[RFC,03/10] drm/panel: Add LGD panel driver for Sony Xperia XZ3

Message ID 20230521-drm-panels-sony-v1-3-541c341d6bee@somainline.org
State New
Headers
Series drm/panel: Drivers for four Sony CMD-mode (and DSC) panels |

Commit Message

Marijn Suijten May 21, 2023, 9:23 p.m. UTC 
  Sony provides an unlabeled LGD + Atmel maXTouch assembly in its Xperia
XZ3 (tama akatsuki) phone, with custom DCS commands to match.

This panel features Display Stream Compression 1.1.

Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
---
 drivers/gpu/drm/panel/Kconfig                   |  11 +
 drivers/gpu/drm/panel/Makefile                  |   1 +
 drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362 ++++++++++++++++++++++++
 3 files changed, 374 insertions(+)

Comments

Dmitry Baryshkov May 22, 2023, 1:16 a.m. UTC | #1 
On 22/05/2023 00:23, Marijn Suijten wrote:
> Sony provides an unlabeled LGD + Atmel maXTouch assembly in its Xperia
> XZ3 (tama akatsuki) phone, with custom DCS commands to match.
> 
> This panel features Display Stream Compression 1.1.
> 
> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> ---
>   drivers/gpu/drm/panel/Kconfig                   |  11 +
>   drivers/gpu/drm/panel/Makefile                  |   1 +
>   drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362 ++++++++++++++++++++++++
>   3 files changed, 374 insertions(+)
> 
> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> index 67ef898d133f2..18bd116e78a71 100644
> --- a/drivers/gpu/drm/panel/Kconfig
> +++ b/drivers/gpu/drm/panel/Kconfig
> @@ -706,6 +706,17 @@ config DRM_PANEL_SONY_ACX565AKM
>   	  Say Y here if you want to enable support for the Sony ACX565AKM
>   	  800x600 3.5" panel (found on the Nokia N900).
>   
> +config DRM_PANEL_SONY_AKATSUKI_LGD
> +	tristate "Sony Xperia XZ3 LGD panel"
> +	depends on GPIOLIB && OF
> +	depends on DRM_MIPI_DSI
> +	depends on BACKLIGHT_CLASS_DEVICE
> +	help
> +	  Say Y here if you want to enable support for the Sony Xperia XZ3
> +	  1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG Display.
> +
> +	  This panel uses Display Stream Compression 1.1.
> +
>   config DRM_PANEL_SONY_TD4353_JDI
>   	tristate "Sony TD4353 JDI panel"
>   	depends on GPIOLIB && OF
> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> index ff169781e82d7..85133f73558f3 100644
> --- a/drivers/gpu/drm/panel/Makefile
> +++ b/drivers/gpu/drm/panel/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += panel-sitronix-st7701.o
>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o
>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
>   obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
> +obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) += panel-sony-akatsuki-lgd.o
>   obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o
>   obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) += panel-sony-tulip-truly-nt35521.o
>   obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o
> diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
> new file mode 100644
> index 0000000000000..f55788f963dab
> --- /dev/null
> +++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
> @@ -0,0 +1,362 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org>
> + *
> + * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi.
> + */
> +
> +#include <linux/backlight.h>
> +#include <linux/delay.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/regulator/consumer.h>
> +
> +#include <video/mipi_display.h>
> +
> +#include <drm/drm_mipi_dsi.h>
> +#include <drm/drm_modes.h>
> +#include <drm/drm_panel.h>
> +#include <drm/drm_probe_helper.h>
> +#include <drm/display/drm_dsc.h>
> +#include <drm/display/drm_dsc_helper.h>
> +
> +struct sony_akatsuki_lgd {
> +	struct drm_panel panel;
> +	struct mipi_dsi_device *dsi;
> +	struct regulator *vddio;
> +	struct gpio_desc *reset_gpio;
> +	bool prepared;
> +};
> +
> +static inline struct sony_akatsuki_lgd *to_sony_akatsuki_lgd(struct drm_panel *panel)
> +{
> +	return container_of(panel, struct sony_akatsuki_lgd, panel);
> +}
> +
> +static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx)
> +{
> +	struct mipi_dsi_device *dsi = ctx->dsi;
> +	struct device *dev = &dsi->dev;
> +	int ret;
> +
> +	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> +
> +	mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
> +	mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
> +	mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
> +	mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
> +	mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01);
> +	mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01);
> +	/* Enable backlight control */
> +	mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, BIT(5));
> +	mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00);
> +
> +	ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set column address: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set page address: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
> +
> +	ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set tear on: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
> +	mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
> +	mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
> +	mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
> +	mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03);
> +	mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04);
> +	mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05);
> +	mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00);
> +
> +	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> +		return ret;
> +	}
> +	msleep(120);
> +
> +	mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d);
> +
> +	ret = mipi_dsi_dcs_set_display_on(dsi);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to turn display on: %d\n", ret);
> +		return ret;
> +	}

My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / 
mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?

> +
> +	return 0;
> +}
> +
> +static int sony_akatsuki_lgd_off(struct sony_akatsuki_lgd *ctx)
> +{
> +	struct mipi_dsi_device *dsi = ctx->dsi;
> +	struct device *dev = &dsi->dev;
> +	int ret;
> +
> +	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
> +
> +	ret = mipi_dsi_dcs_set_display_off(dsi);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to turn display off: %d\n", ret);
> +		return ret;
> +	}
> +	msleep(20);
> +
> +	ret = mipi_dsi_dcs_set_tear_off(dsi);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to set tear off: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enter sleep mode: %d\n", ret);
> +		return ret;
> +	}
> +	msleep(100);
> +
> +	return 0;
> +}
> +
> +static int sony_akatsuki_lgd_prepare(struct drm_panel *panel)
> +{
> +	struct sony_akatsuki_lgd *ctx = to_sony_akatsuki_lgd(panel);
> +	struct drm_dsc_picture_parameter_set pps;
> +	struct device *dev = &ctx->dsi->dev;
> +	int ret;
> +
> +	if (ctx->prepared)
> +		return 0;
> +
> +	ret = regulator_enable(ctx->vddio);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to enable vddio regulator: %d\n", ret);
> +		return ret;
> +	}
> +
> +	msleep(100);
> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
> +	usleep_range(5000, 5100);
> +
> +	ret = sony_akatsuki_lgd_on(ctx);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to power on panel: %d\n", ret);
> +		goto fail;
> +	}
> +
> +	if (ctx->dsi->dsc) {

dsi->dsc is always set, thus this condition can be dropped.

> +		drm_dsc_pps_payload_pack(&pps, ctx->dsi->dsc);
> +
> +		ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
> +		if (ret < 0) {
> +			dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
> +			goto fail;
> +		}
> +		ret = mipi_dsi_compression_mode(ctx->dsi, true);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to enable compression mode: %d\n", ret);
> +			goto fail;
> +		}
> +
> +		msleep(28);
> +	}
> +
> +	ctx->prepared = true;
> +	return 0;
> +
> +fail:
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +	regulator_disable(ctx->vddio);
> +	return ret;
> +}
> +
> +static int sony_akatsuki_lgd_unprepare(struct drm_panel *panel)
> +{
> +	struct sony_akatsuki_lgd *ctx = to_sony_akatsuki_lgd(panel);
> +	struct device *dev = &ctx->dsi->dev;
> +	int ret;
> +
> +	if (!ctx->prepared)
> +		return 0;
> +
> +	ret = sony_akatsuki_lgd_off(ctx);
> +	if (ret < 0)
> +		dev_err(dev, "Failed to power off panel: %d\n", ret);
> +
> +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> +	regulator_disable(ctx->vddio);
> +
> +	usleep_range(5000, 5100);
> +
> +	ctx->prepared = false;
> +	return 0;
> +}
> +
> +static const struct drm_display_mode sony_akatsuki_lgd_mode = {
> +	.clock = (1440 + 312 + 8 + 8) * (2880 + 48 + 8 + 8) * 60 / 1000,
> +	.hdisplay = 1440,
> +	.hsync_start = 1440 + 312,
> +	.hsync_end = 1440 + 312 + 8,
> +	.htotal = 1440 + 312 + 8 + 8,
> +	.vdisplay = 2880,
> +	.vsync_start = 2880 + 48,
> +	.vsync_end = 2880 + 48 + 8,
> +	.vtotal = 2880 + 48 + 8 + 8,
> +	.width_mm = 68,
> +	.height_mm = 136,
> +};
> +
> +static int sony_akatsuki_lgd_get_modes(struct drm_panel *panel,
> +				   struct drm_connector *connector)
> +{
> +	return drm_connector_helper_get_modes_fixed(connector, &sony_akatsuki_lgd_mode);
> +}
> +
> +static const struct drm_panel_funcs sony_akatsuki_lgd_panel_funcs = {
> +	.prepare = sony_akatsuki_lgd_prepare,
> +	.unprepare = sony_akatsuki_lgd_unprepare,
> +	.get_modes = sony_akatsuki_lgd_get_modes,
> +};
> +
> +static int sony_akatsuki_lgd_bl_update_status(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi = bl_get_data(bl);
> +	u16 brightness = backlight_get_brightness(bl);
> +
> +	return mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
> +}
> +
> +static int sony_akatsuki_lgd_bl_get_brightness(struct backlight_device *bl)
> +{
> +	struct mipi_dsi_device *dsi = bl_get_data(bl);
> +	u16 brightness;
> +	int ret;
> +
> +	ret = mipi_dsi_dcs_get_display_brightness_large(dsi, &brightness);
> +	if (ret < 0)
> +		return ret;
> +
> +	return brightness & 0x3ff;
> +}
> +
> +static const struct backlight_ops sony_akatsuki_lgd_bl_ops = {
> +	.update_status = sony_akatsuki_lgd_bl_update_status,
> +	.get_brightness = sony_akatsuki_lgd_bl_get_brightness,
> +};
> +
> +static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
> +{
> +	const struct backlight_properties props = {
> +		.type = BACKLIGHT_RAW,
> +		.brightness = 100,
> +		.max_brightness = 1023,
> +	};
> +	struct device *dev = &dsi->dev;
> +	struct sony_akatsuki_lgd *ctx;
> +	struct drm_dsc_config *dsc;
> +	int ret;
> +
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	ctx->vddio = devm_regulator_get(dev, "vddio");
> +	if (IS_ERR(ctx->vddio))
> +		return dev_err_probe(dev, PTR_ERR(ctx->vddio),
> +				     "Failed to get vddio\n");
> +
> +	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
> +	if (IS_ERR(ctx->reset_gpio))
> +		return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
> +				     "Failed to get reset-gpios\n");
> +
> +	ctx->dsi = dsi;
> +	mipi_dsi_set_drvdata(dsi, ctx);
> +
> +	dsi->lanes = 4;
> +	dsi->format = MIPI_DSI_FMT_RGB888;
> +	dsi->mode_flags = MIPI_DSI_CLOCK_NON_CONTINUOUS;
> +
> +	drm_panel_init(&ctx->panel, dev, &sony_akatsuki_lgd_panel_funcs,
> +		       DRM_MODE_CONNECTOR_DSI);
> +
> +	ctx->panel.backlight = devm_backlight_device_register(dev, dev_name(dev), dev, dsi,
> +					      &sony_akatsuki_lgd_bl_ops, &props);
> +	if (IS_ERR(ctx->panel.backlight))
> +		return dev_err_probe(dev, PTR_ERR(ctx->panel.backlight),
> +				     "Failed to create backlight\n");
> +
> +	drm_panel_add(&ctx->panel);
> +
> +	/* This panel only supports DSC; unconditionally enable it */
> +	dsi->dsc = dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);

I think double assignments are frowned upon.

> +	if (!dsc)
> +		return -ENOMEM;
> +
> +	dsc->dsc_version_major = 1;
> +	dsc->dsc_version_minor = 1;
> +
> +	dsc->slice_height = 32;
> +	dsc->slice_count = 2;
> +	// TODO: Get hdisplay from the mode

Would you like to fix the TODO?

> +	WARN_ON(1440 % dsc->slice_count);
> +	dsc->slice_width = 1440 / dsc->slice_count;
> +	dsc->bits_per_component = 8;
> +	dsc->bits_per_pixel = 8 << 4; /* 4 fractional bits */
> +	dsc->block_pred_enable = true;
> +
> +	ret = mipi_dsi_attach(dsi);
> +	if (ret < 0) {
> +		dev_err(dev, "Failed to attach to DSI host: %d\n", ret);
> +		drm_panel_remove(&ctx->panel);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static void sony_akatsuki_lgd_remove(struct mipi_dsi_device *dsi)
> +{
> +	struct sony_akatsuki_lgd *ctx = mipi_dsi_get_drvdata(dsi);
> +	int ret;
> +
> +	ret = mipi_dsi_detach(dsi);
> +	if (ret < 0)
> +		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
> +
> +	drm_panel_remove(&ctx->panel);
> +}
> +
> +static const struct of_device_id sony_akatsuki_lgd_of_match[] = {
> +	{ .compatible = "sony,akatsuki-lgd" },
> +	{ /* sentinel */ }
> +};
> +MODULE_DEVICE_TABLE(of, sony_akatsuki_lgd_of_match);
> +
> +static struct mipi_dsi_driver sony_akatsuki_lgd_driver = {
> +	.probe = sony_akatsuki_lgd_probe,
> +	.remove = sony_akatsuki_lgd_remove,
> +	.driver = {
> +		.name = "panel-sony-akatsuki-lgd",
> +		.of_match_table = sony_akatsuki_lgd_of_match,
> +	},
> +};
> +module_mipi_dsi_driver(sony_akatsuki_lgd_driver);
> +
> +MODULE_AUTHOR("Marijn Suijten <marijn.suijten@somainline.org>");
> +MODULE_DESCRIPTION("DRM panel driver for an unnamed LGD OLED panel found in the Sony Xperia XZ3");
> +MODULE_LICENSE("GPL");
>
Neil Armstrong May 22, 2023, 9:04 a.m. UTC | #2 
On 22/05/2023 03:16, Dmitry Baryshkov wrote:
> On 22/05/2023 00:23, Marijn Suijten wrote:
>> Sony provides an unlabeled LGD + Atmel maXTouch assembly in its Xperia
>> XZ3 (tama akatsuki) phone, with custom DCS commands to match.
>>
>> This panel features Display Stream Compression 1.1.
>>
>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>> ---
>>   drivers/gpu/drm/panel/Kconfig                   |  11 +
>>   drivers/gpu/drm/panel/Makefile                  |   1 +
>>   drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362 ++++++++++++++++++++++++
>>   3 files changed, 374 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>> index 67ef898d133f2..18bd116e78a71 100644
>> --- a/drivers/gpu/drm/panel/Kconfig
>> +++ b/drivers/gpu/drm/panel/Kconfig
>> @@ -706,6 +706,17 @@ config DRM_PANEL_SONY_ACX565AKM
>>         Say Y here if you want to enable support for the Sony ACX565AKM
>>         800x600 3.5" panel (found on the Nokia N900).
>> +config DRM_PANEL_SONY_AKATSUKI_LGD
>> +    tristate "Sony Xperia XZ3 LGD panel"
>> +    depends on GPIOLIB && OF
>> +    depends on DRM_MIPI_DSI
>> +    depends on BACKLIGHT_CLASS_DEVICE
>> +    help
>> +      Say Y here if you want to enable support for the Sony Xperia XZ3
>> +      1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG Display.
>> +
>> +      This panel uses Display Stream Compression 1.1.
>> +
>>   config DRM_PANEL_SONY_TD4353_JDI
>>       tristate "Sony TD4353 JDI panel"
>>       depends on GPIOLIB && OF
>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>> index ff169781e82d7..85133f73558f3 100644
>> --- a/drivers/gpu/drm/panel/Makefile
>> +++ b/drivers/gpu/drm/panel/Makefile
>> @@ -71,6 +71,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += panel-sitronix-st7701.o
>>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o
>>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
>>   obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
>> +obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) += panel-sony-akatsuki-lgd.o
>>   obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o
>>   obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) += panel-sony-tulip-truly-nt35521.o
>>   obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o
>> diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>> new file mode 100644
>> index 0000000000000..f55788f963dab
>> --- /dev/null
>> +++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>> @@ -0,0 +1,362 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org>
>> + *
>> + * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi.
>> + */
>> +
>> +#include <linux/backlight.h>
>> +#include <linux/delay.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/regulator/consumer.h>
>> +
>> +#include <video/mipi_display.h>
>> +
>> +#include <drm/drm_mipi_dsi.h>
>> +#include <drm/drm_modes.h>
>> +#include <drm/drm_panel.h>
>> +#include <drm/drm_probe_helper.h>
>> +#include <drm/display/drm_dsc.h>
>> +#include <drm/display/drm_dsc_helper.h>
>> +
>> +struct sony_akatsuki_lgd {
>> +    struct drm_panel panel;
>> +    struct mipi_dsi_device *dsi;
>> +    struct regulator *vddio;
>> +    struct gpio_desc *reset_gpio;
>> +    bool prepared;
>> +};
>> +
>> +static inline struct sony_akatsuki_lgd *to_sony_akatsuki_lgd(struct drm_panel *panel)
>> +{
>> +    return container_of(panel, struct sony_akatsuki_lgd, panel);
>> +}
>> +
>> +static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx)
>> +{
>> +    struct mipi_dsi_device *dsi = ctx->dsi;
>> +    struct device *dev = &dsi->dev;
>> +    int ret;
>> +
>> +    dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>> +
>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
>> +    mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01);
>> +    mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01);
>> +    /* Enable backlight control */
>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, BIT(5));
>> +    mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00);
>> +
>> +    ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to set column address: %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to set page address: %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
>> +
>> +    ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to set tear on: %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03);
>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04);
>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05);
>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00);
>> +
>> +    ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
>> +        return ret;
>> +    }
>> +    msleep(120);
>> +
>> +    mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d);
>> +
>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
>> +        return ret;
>> +    }
> 
> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?


No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.

Neil

> 
>> +
>> +    return 0;
>> +}
>> +
>> +static int sony_akatsuki_lgd_off(struct sony_akatsuki_lgd *ctx)
>> +{
>> +    struct mipi_dsi_device *dsi = ctx->dsi;
>> +    struct device *dev = &dsi->dev;
>> +    int ret;
>> +
>> +    dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
>> +
>> +    ret = mipi_dsi_dcs_set_display_off(dsi);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to turn display off: %d\n", ret);
>> +        return ret;
>> +    }
>> +    msleep(20);
>> +
>> +    ret = mipi_dsi_dcs_set_tear_off(dsi);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to set tear off: %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to enter sleep mode: %d\n", ret);
>> +        return ret;
>> +    }
>> +    msleep(100);
>> +
>> +    return 0;
>> +}
>> +
>> +static int sony_akatsuki_lgd_prepare(struct drm_panel *panel)
>> +{
>> +    struct sony_akatsuki_lgd *ctx = to_sony_akatsuki_lgd(panel);
>> +    struct drm_dsc_picture_parameter_set pps;
>> +    struct device *dev = &ctx->dsi->dev;
>> +    int ret;
>> +
>> +    if (ctx->prepared)
>> +        return 0;
>> +
>> +    ret = regulator_enable(ctx->vddio);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to enable vddio regulator: %d\n", ret);
>> +        return ret;
>> +    }
>> +
>> +    msleep(100);
>> +
>> +    gpiod_set_value_cansleep(ctx->reset_gpio, 1);
>> +    usleep_range(5000, 5100);
>> +
>> +    ret = sony_akatsuki_lgd_on(ctx);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to power on panel: %d\n", ret);
>> +        goto fail;
>> +    }
>> +
>> +    if (ctx->dsi->dsc) {
> 
> dsi->dsc is always set, thus this condition can be dropped.
> 
>> +        drm_dsc_pps_payload_pack(&pps, ctx->dsi->dsc);
>> +
>> +        ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
>> +        if (ret < 0) {
>> +            dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
>> +            goto fail;
>> +        }
>> +        ret = mipi_dsi_compression_mode(ctx->dsi, true);
>> +        if (ret < 0) {
>> +            dev_err(dev, "failed to enable compression mode: %d\n", ret);
>> +            goto fail;
>> +        }
>> +
>> +        msleep(28);
>> +    }
>> +
>> +    ctx->prepared = true;
>> +    return 0;
>> +
>> +fail:
>> +    gpiod_set_value_cansleep(ctx->reset_gpio, 0);
>> +    regulator_disable(ctx->vddio);
>> +    return ret;
>> +}
>> +
>> +static int sony_akatsuki_lgd_unprepare(struct drm_panel *panel)
>> +{
>> +    struct sony_akatsuki_lgd *ctx = to_sony_akatsuki_lgd(panel);
>> +    struct device *dev = &ctx->dsi->dev;
>> +    int ret;
>> +
>> +    if (!ctx->prepared)
>> +        return 0;
>> +
>> +    ret = sony_akatsuki_lgd_off(ctx);
>> +    if (ret < 0)
>> +        dev_err(dev, "Failed to power off panel: %d\n", ret);
>> +
>> +    gpiod_set_value_cansleep(ctx->reset_gpio, 0);
>> +    regulator_disable(ctx->vddio);
>> +
>> +    usleep_range(5000, 5100);
>> +
>> +    ctx->prepared = false;
>> +    return 0;
>> +}
>> +
>> +static const struct drm_display_mode sony_akatsuki_lgd_mode = {
>> +    .clock = (1440 + 312 + 8 + 8) * (2880 + 48 + 8 + 8) * 60 / 1000,
>> +    .hdisplay = 1440,
>> +    .hsync_start = 1440 + 312,
>> +    .hsync_end = 1440 + 312 + 8,
>> +    .htotal = 1440 + 312 + 8 + 8,
>> +    .vdisplay = 2880,
>> +    .vsync_start = 2880 + 48,
>> +    .vsync_end = 2880 + 48 + 8,
>> +    .vtotal = 2880 + 48 + 8 + 8,
>> +    .width_mm = 68,
>> +    .height_mm = 136,
>> +};
>> +
>> +static int sony_akatsuki_lgd_get_modes(struct drm_panel *panel,
>> +                   struct drm_connector *connector)
>> +{
>> +    return drm_connector_helper_get_modes_fixed(connector, &sony_akatsuki_lgd_mode);
>> +}
>> +
>> +static const struct drm_panel_funcs sony_akatsuki_lgd_panel_funcs = {
>> +    .prepare = sony_akatsuki_lgd_prepare,
>> +    .unprepare = sony_akatsuki_lgd_unprepare,
>> +    .get_modes = sony_akatsuki_lgd_get_modes,
>> +};
>> +
>> +static int sony_akatsuki_lgd_bl_update_status(struct backlight_device *bl)
>> +{
>> +    struct mipi_dsi_device *dsi = bl_get_data(bl);
>> +    u16 brightness = backlight_get_brightness(bl);
>> +
>> +    return mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
>> +}
>> +
>> +static int sony_akatsuki_lgd_bl_get_brightness(struct backlight_device *bl)
>> +{
>> +    struct mipi_dsi_device *dsi = bl_get_data(bl);
>> +    u16 brightness;
>> +    int ret;
>> +
>> +    ret = mipi_dsi_dcs_get_display_brightness_large(dsi, &brightness);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    return brightness & 0x3ff;
>> +}
>> +
>> +static const struct backlight_ops sony_akatsuki_lgd_bl_ops = {
>> +    .update_status = sony_akatsuki_lgd_bl_update_status,
>> +    .get_brightness = sony_akatsuki_lgd_bl_get_brightness,
>> +};
>> +
>> +static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
>> +{
>> +    const struct backlight_properties props = {
>> +        .type = BACKLIGHT_RAW,
>> +        .brightness = 100,
>> +        .max_brightness = 1023,
>> +    };
>> +    struct device *dev = &dsi->dev;
>> +    struct sony_akatsuki_lgd *ctx;
>> +    struct drm_dsc_config *dsc;
>> +    int ret;
>> +
>> +    ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
>> +    if (!ctx)
>> +        return -ENOMEM;
>> +
>> +    ctx->vddio = devm_regulator_get(dev, "vddio");
>> +    if (IS_ERR(ctx->vddio))
>> +        return dev_err_probe(dev, PTR_ERR(ctx->vddio),
>> +                     "Failed to get vddio\n");
>> +
>> +    ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
>> +    if (IS_ERR(ctx->reset_gpio))
>> +        return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
>> +                     "Failed to get reset-gpios\n");
>> +
>> +    ctx->dsi = dsi;
>> +    mipi_dsi_set_drvdata(dsi, ctx);
>> +
>> +    dsi->lanes = 4;
>> +    dsi->format = MIPI_DSI_FMT_RGB888;
>> +    dsi->mode_flags = MIPI_DSI_CLOCK_NON_CONTINUOUS;
>> +
>> +    drm_panel_init(&ctx->panel, dev, &sony_akatsuki_lgd_panel_funcs,
>> +               DRM_MODE_CONNECTOR_DSI);
>> +
>> +    ctx->panel.backlight = devm_backlight_device_register(dev, dev_name(dev), dev, dsi,
>> +                          &sony_akatsuki_lgd_bl_ops, &props);
>> +    if (IS_ERR(ctx->panel.backlight))
>> +        return dev_err_probe(dev, PTR_ERR(ctx->panel.backlight),
>> +                     "Failed to create backlight\n");
>> +
>> +    drm_panel_add(&ctx->panel);
>> +
>> +    /* This panel only supports DSC; unconditionally enable it */
>> +    dsi->dsc = dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
> 
> I think double assignments are frowned upon.
> 
>> +    if (!dsc)
>> +        return -ENOMEM;
>> +
>> +    dsc->dsc_version_major = 1;
>> +    dsc->dsc_version_minor = 1;
>> +
>> +    dsc->slice_height = 32;
>> +    dsc->slice_count = 2;
>> +    // TODO: Get hdisplay from the mode
> 
> Would you like to fix the TODO?
> 
>> +    WARN_ON(1440 % dsc->slice_count);
>> +    dsc->slice_width = 1440 / dsc->slice_count;
>> +    dsc->bits_per_component = 8;
>> +    dsc->bits_per_pixel = 8 << 4; /* 4 fractional bits */
>> +    dsc->block_pred_enable = true;
>> +
>> +    ret = mipi_dsi_attach(dsi);
>> +    if (ret < 0) {
>> +        dev_err(dev, "Failed to attach to DSI host: %d\n", ret);
>> +        drm_panel_remove(&ctx->panel);
>> +        return ret;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void sony_akatsuki_lgd_remove(struct mipi_dsi_device *dsi)
>> +{
>> +    struct sony_akatsuki_lgd *ctx = mipi_dsi_get_drvdata(dsi);
>> +    int ret;
>> +
>> +    ret = mipi_dsi_detach(dsi);
>> +    if (ret < 0)
>> +        dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
>> +
>> +    drm_panel_remove(&ctx->panel);
>> +}
>> +
>> +static const struct of_device_id sony_akatsuki_lgd_of_match[] = {
>> +    { .compatible = "sony,akatsuki-lgd" },
>> +    { /* sentinel */ }
>> +};
>> +MODULE_DEVICE_TABLE(of, sony_akatsuki_lgd_of_match);
>> +
>> +static struct mipi_dsi_driver sony_akatsuki_lgd_driver = {
>> +    .probe = sony_akatsuki_lgd_probe,
>> +    .remove = sony_akatsuki_lgd_remove,
>> +    .driver = {
>> +        .name = "panel-sony-akatsuki-lgd",
>> +        .of_match_table = sony_akatsuki_lgd_of_match,
>> +    },
>> +};
>> +module_mipi_dsi_driver(sony_akatsuki_lgd_driver);
>> +
>> +MODULE_AUTHOR("Marijn Suijten <marijn.suijten@somainline.org>");
>> +MODULE_DESCRIPTION("DRM panel driver for an unnamed LGD OLED panel found in the Sony Xperia XZ3");
>> +MODULE_LICENSE("GPL");
>>
>
Dmitry Baryshkov May 22, 2023, 12:58 p.m. UTC | #3 
On Mon, 22 May 2023 at 12:04, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> On 22/05/2023 03:16, Dmitry Baryshkov wrote:
> > On 22/05/2023 00:23, Marijn Suijten wrote:
> >> Sony provides an unlabeled LGD + Atmel maXTouch assembly in its Xperia
> >> XZ3 (tama akatsuki) phone, with custom DCS commands to match.
> >>
> >> This panel features Display Stream Compression 1.1.
> >>
> >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> >> ---
> >>   drivers/gpu/drm/panel/Kconfig                   |  11 +
> >>   drivers/gpu/drm/panel/Makefile                  |   1 +
> >>   drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362 ++++++++++++++++++++++++
> >>   3 files changed, 374 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> >> index 67ef898d133f2..18bd116e78a71 100644
> >> --- a/drivers/gpu/drm/panel/Kconfig
> >> +++ b/drivers/gpu/drm/panel/Kconfig
> >> @@ -706,6 +706,17 @@ config DRM_PANEL_SONY_ACX565AKM
> >>         Say Y here if you want to enable support for the Sony ACX565AKM
> >>         800x600 3.5" panel (found on the Nokia N900).
> >> +config DRM_PANEL_SONY_AKATSUKI_LGD
> >> +    tristate "Sony Xperia XZ3 LGD panel"
> >> +    depends on GPIOLIB && OF
> >> +    depends on DRM_MIPI_DSI
> >> +    depends on BACKLIGHT_CLASS_DEVICE
> >> +    help
> >> +      Say Y here if you want to enable support for the Sony Xperia XZ3
> >> +      1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG Display.
> >> +
> >> +      This panel uses Display Stream Compression 1.1.
> >> +
> >>   config DRM_PANEL_SONY_TD4353_JDI
> >>       tristate "Sony TD4353 JDI panel"
> >>       depends on GPIOLIB && OF
> >> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> >> index ff169781e82d7..85133f73558f3 100644
> >> --- a/drivers/gpu/drm/panel/Makefile
> >> +++ b/drivers/gpu/drm/panel/Makefile
> >> @@ -71,6 +71,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += panel-sitronix-st7701.o
> >>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o
> >>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
> >>   obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
> >> +obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) += panel-sony-akatsuki-lgd.o
> >>   obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o
> >>   obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) += panel-sony-tulip-truly-nt35521.o
> >>   obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o
> >> diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
> >> new file mode 100644
> >> index 0000000000000..f55788f963dab
> >> --- /dev/null
> >> +++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
> >> @@ -0,0 +1,362 @@
> >> +// SPDX-License-Identifier: GPL-2.0-only
> >> +/*
> >> + * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org>
> >> + *
> >> + * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi.
> >> + */
> >> +
> >> +#include <linux/backlight.h>
> >> +#include <linux/delay.h>
> >> +#include <linux/gpio/consumer.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_device.h>
> >> +#include <linux/regulator/consumer.h>
> >> +
> >> +#include <video/mipi_display.h>
> >> +
> >> +#include <drm/drm_mipi_dsi.h>
> >> +#include <drm/drm_modes.h>
> >> +#include <drm/drm_panel.h>
> >> +#include <drm/drm_probe_helper.h>
> >> +#include <drm/display/drm_dsc.h>
> >> +#include <drm/display/drm_dsc_helper.h>
> >> +
> >> +struct sony_akatsuki_lgd {
> >> +    struct drm_panel panel;
> >> +    struct mipi_dsi_device *dsi;
> >> +    struct regulator *vddio;
> >> +    struct gpio_desc *reset_gpio;
> >> +    bool prepared;
> >> +};
> >> +
> >> +static inline struct sony_akatsuki_lgd *to_sony_akatsuki_lgd(struct drm_panel *panel)
> >> +{
> >> +    return container_of(panel, struct sony_akatsuki_lgd, panel);
> >> +}
> >> +
> >> +static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx)
> >> +{
> >> +    struct mipi_dsi_device *dsi = ctx->dsi;
> >> +    struct device *dev = &dsi->dev;
> >> +    int ret;
> >> +
> >> +    dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> >> +
> >> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01);
> >> +    /* Enable backlight control */
> >> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, BIT(5));
> >> +    mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00);
> >> +
> >> +    ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1);
> >> +    if (ret < 0) {
> >> +        dev_err(dev, "Failed to set column address: %d\n", ret);
> >> +        return ret;
> >> +    }
> >> +
> >> +    ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1);
> >> +    if (ret < 0) {
> >> +        dev_err(dev, "Failed to set page address: %d\n", ret);
> >> +        return ret;
> >> +    }
> >> +
> >> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
> >> +
> >> +    ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> >> +    if (ret < 0) {
> >> +        dev_err(dev, "Failed to set tear on: %d\n", ret);
> >> +        return ret;
> >> +    }
> >> +
> >> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05);
> >> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00);
> >> +
> >> +    ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> >> +    if (ret < 0) {
> >> +        dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> >> +        return ret;
> >> +    }
> >> +    msleep(120);
> >> +
> >> +    mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d);
> >> +
> >> +    ret = mipi_dsi_dcs_set_display_on(dsi);
> >> +    if (ret < 0) {
> >> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
> >> +        return ret;
> >> +    }
> >
> > My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?
>
>
> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.
>

Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream?
Marijn Suijten May 29, 2023, 9:07 p.m. UTC | #4 
On 2023-05-22 15:58:56, Dmitry Baryshkov wrote:
> On Mon, 22 May 2023 at 12:04, Neil Armstrong <neil.armstrong@linaro.org> wrote:
> >
> > On 22/05/2023 03:16, Dmitry Baryshkov wrote:
> > > On 22/05/2023 00:23, Marijn Suijten wrote:
> > >> Sony provides an unlabeled LGD + Atmel maXTouch assembly in its Xperia
> > >> XZ3 (tama akatsuki) phone, with custom DCS commands to match.
> > >>
> > >> This panel features Display Stream Compression 1.1.
> > >>
> > >> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> > >> ---
> > >>   drivers/gpu/drm/panel/Kconfig                   |  11 +
> > >>   drivers/gpu/drm/panel/Makefile                  |   1 +
> > >>   drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362 ++++++++++++++++++++++++
> > >>   3 files changed, 374 insertions(+)
> > >>
> > >> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
> > >> index 67ef898d133f2..18bd116e78a71 100644
> > >> --- a/drivers/gpu/drm/panel/Kconfig
> > >> +++ b/drivers/gpu/drm/panel/Kconfig
> > >> @@ -706,6 +706,17 @@ config DRM_PANEL_SONY_ACX565AKM
> > >>         Say Y here if you want to enable support for the Sony ACX565AKM
> > >>         800x600 3.5" panel (found on the Nokia N900).
> > >> +config DRM_PANEL_SONY_AKATSUKI_LGD
> > >> +    tristate "Sony Xperia XZ3 LGD panel"
> > >> +    depends on GPIOLIB && OF
> > >> +    depends on DRM_MIPI_DSI
> > >> +    depends on BACKLIGHT_CLASS_DEVICE
> > >> +    help
> > >> +      Say Y here if you want to enable support for the Sony Xperia XZ3
> > >> +      1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG Display.
> > >> +
> > >> +      This panel uses Display Stream Compression 1.1.
> > >> +
> > >>   config DRM_PANEL_SONY_TD4353_JDI
> > >>       tristate "Sony TD4353 JDI panel"
> > >>       depends on GPIOLIB && OF
> > >> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
> > >> index ff169781e82d7..85133f73558f3 100644
> > >> --- a/drivers/gpu/drm/panel/Makefile
> > >> +++ b/drivers/gpu/drm/panel/Makefile
> > >> @@ -71,6 +71,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += panel-sitronix-st7701.o
> > >>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o
> > >>   obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
> > >>   obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
> > >> +obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) += panel-sony-akatsuki-lgd.o
> > >>   obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o
> > >>   obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) += panel-sony-tulip-truly-nt35521.o
> > >>   obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o
> > >> diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
> > >> new file mode 100644
> > >> index 0000000000000..f55788f963dab
> > >> --- /dev/null
> > >> +++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
> > >> @@ -0,0 +1,362 @@
> > >> +// SPDX-License-Identifier: GPL-2.0-only
> > >> +/*
> > >> + * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org>
> > >> + *
> > >> + * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi.
> > >> + */
> > >> +
> > >> +#include <linux/backlight.h>
> > >> +#include <linux/delay.h>
> > >> +#include <linux/gpio/consumer.h>
> > >> +#include <linux/module.h>
> > >> +#include <linux/of.h>
> > >> +#include <linux/of_device.h>
> > >> +#include <linux/regulator/consumer.h>
> > >> +
> > >> +#include <video/mipi_display.h>
> > >> +
> > >> +#include <drm/drm_mipi_dsi.h>
> > >> +#include <drm/drm_modes.h>
> > >> +#include <drm/drm_panel.h>
> > >> +#include <drm/drm_probe_helper.h>
> > >> +#include <drm/display/drm_dsc.h>
> > >> +#include <drm/display/drm_dsc_helper.h>
> > >> +
> > >> +struct sony_akatsuki_lgd {
> > >> +    struct drm_panel panel;
> > >> +    struct mipi_dsi_device *dsi;
> > >> +    struct regulator *vddio;
> > >> +    struct gpio_desc *reset_gpio;
> > >> +    bool prepared;
> > >> +};
> > >> +
> > >> +static inline struct sony_akatsuki_lgd *to_sony_akatsuki_lgd(struct drm_panel *panel)
> > >> +{
> > >> +    return container_of(panel, struct sony_akatsuki_lgd, panel);
> > >> +}
> > >> +
> > >> +static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx)
> > >> +{
> > >> +    struct mipi_dsi_device *dsi = ctx->dsi;
> > >> +    struct device *dev = &dsi->dev;
> > >> +    int ret;
> > >> +
> > >> +    dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> > >> +
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01);
> > >> +    /* Enable backlight control */
> > >> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, BIT(5));
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00);
> > >> +
> > >> +    ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1);
> > >> +    if (ret < 0) {
> > >> +        dev_err(dev, "Failed to set column address: %d\n", ret);
> > >> +        return ret;
> > >> +    }
> > >> +
> > >> +    ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1);
> > >> +    if (ret < 0) {
> > >> +        dev_err(dev, "Failed to set page address: %d\n", ret);
> > >> +        return ret;
> > >> +    }
> > >> +
> > >> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
> > >> +
> > >> +    ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> > >> +    if (ret < 0) {
> > >> +        dev_err(dev, "Failed to set tear on: %d\n", ret);
> > >> +        return ret;
> > >> +    }
> > >> +
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05);
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00);
> > >> +
> > >> +    ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> > >> +    if (ret < 0) {
> > >> +        dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> > >> +        return ret;
> > >> +    }
> > >> +    msleep(120);
> > >> +
> > >> +    mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d);
> > >> +
> > >> +    ret = mipi_dsi_dcs_set_display_on(dsi);
> > >> +    if (ret < 0) {
> > >> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
> > >> +        return ret;
> > >> +    }
> > >
> > > My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?
> >
> >
> > No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.
> >
> 
> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream?

I have never investigated what it takes to split these functions, but
some of these panels do show some corruption at startup which may be
circumvented by powering the panel on after starting the video stream?

I'm just not sure where to make the split: downstream does describe a
qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
the latter only contains set_display_on() (not exit_sleep_mode()).
It is documented like:

    same as "qcom,mdss-dsi-on-command" except commands are sent after
    displaying an image."

So this seems like the right way to split them up, I'll test this out on
all submitted panel drivers.

- Marijn

> 
> -- 
> With best wishes
> Dmitry
Marijn Suijten May 29, 2023, 9:11 p.m. UTC | #5 
On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
<snip>
> > +	if (ctx->dsi->dsc) {
> 
> dsi->dsc is always set, thus this condition can be dropped.

I want to leave room for possibly running the panel without DSC (at a
lower resolution/refresh rate, or at higher power consumption if there
is enough BW) by not assigning the pointer, if we get access to panel
documentation: probably one of the magic commands sent in this driver
controls it but we don't know which.

> > +		drm_dsc_pps_payload_pack(&pps, ctx->dsi->dsc);
> > +
> > +		ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
> > +		if (ret < 0) {
> > +			dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
> > +			goto fail;
> > +		}
> > +		ret = mipi_dsi_compression_mode(ctx->dsi, true);
> > +		if (ret < 0) {
> > +			dev_err(dev, "failed to enable compression mode: %d\n", ret);
> > +			goto fail;
> > +		}
> > +
> > +		msleep(28);
> > +	}
> > +
> > +	ctx->prepared = true;
> > +	return 0;
> > +
> > +fail:
> > +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
> > +	regulator_disable(ctx->vddio);
> > +	return ret;
> > +}
<snip>
> > +	/* This panel only supports DSC; unconditionally enable it */

On that note I should perhaps reword this.

> > +	dsi->dsc = dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
> 
> I think double assignments are frowned upon.

Ack.

> 
> > +	if (!dsc)
> > +		return -ENOMEM;
> > +
> > +	dsc->dsc_version_major = 1;
> > +	dsc->dsc_version_minor = 1;
> > +
> > +	dsc->slice_height = 32;
> > +	dsc->slice_count = 2;
> > +	// TODO: Get hdisplay from the mode
> 
> Would you like to fix the TODO?

I can't unless either migrating to drm_bridge (is that doable?) or
expand drm_panel.  That's a larger task, but I don't think this driver
is the right place to track that desire.  Should I drop the comment
entirely or reword it?

> > +	WARN_ON(1440 % dsc->slice_count);
> > +	dsc->slice_width = 1440 / dsc->slice_count;

<snip>

- Marijn
Dmitry Baryshkov May 29, 2023, 10:17 p.m. UTC | #6 
On 30/05/2023 00:11, Marijn Suijten wrote:
> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
> <snip>
>>> +	if (ctx->dsi->dsc) {
>>
>> dsi->dsc is always set, thus this condition can be dropped.
> 
> I want to leave room for possibly running the panel without DSC (at a
> lower resolution/refresh rate, or at higher power consumption if there
> is enough BW) by not assigning the pointer, if we get access to panel
> documentation: probably one of the magic commands sent in this driver
> controls it but we don't know which.
> 
>>> +		drm_dsc_pps_payload_pack(&pps, ctx->dsi->dsc);
>>> +
>>> +		ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
>>> +		if (ret < 0) {
>>> +			dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
>>> +			goto fail;
>>> +		}
>>> +		ret = mipi_dsi_compression_mode(ctx->dsi, true);
>>> +		if (ret < 0) {
>>> +			dev_err(dev, "failed to enable compression mode: %d\n", ret);
>>> +			goto fail;
>>> +		}
>>> +
>>> +		msleep(28);
>>> +	}
>>> +
>>> +	ctx->prepared = true;
>>> +	return 0;
>>> +
>>> +fail:
>>> +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
>>> +	regulator_disable(ctx->vddio);
>>> +	return ret;
>>> +}
> <snip>
>>> +	/* This panel only supports DSC; unconditionally enable it */
> 
> On that note I should perhaps reword this.
> 
>>> +	dsi->dsc = dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
>>
>> I think double assignments are frowned upon.
> 
> Ack.
> 
>>
>>> +	if (!dsc)
>>> +		return -ENOMEM;
>>> +
>>> +	dsc->dsc_version_major = 1;
>>> +	dsc->dsc_version_minor = 1;
>>> +
>>> +	dsc->slice_height = 32;
>>> +	dsc->slice_count = 2;
>>> +	// TODO: Get hdisplay from the mode
>>
>> Would you like to fix the TODO?
> 
> I can't unless either migrating to drm_bridge (is that doable?) or
> expand drm_panel.  That's a larger task, but I don't think this driver
> is the right place to track that desire.  Should I drop the comment
> entirely or reword it?

I'd say, reword it, so that it becomes more obvious why this TODO can 
not be fixed at this moment.

> 
>>> +	WARN_ON(1440 % dsc->slice_count);
>>> +	dsc->slice_width = 1440 / dsc->slice_count;
> 
> <snip>
> 
> - Marijn
Dmitry Baryshkov May 29, 2023, 10:18 p.m. UTC | #7 
On 30/05/2023 00:07, Marijn Suijten wrote:
> On 2023-05-22 15:58:56, Dmitry Baryshkov wrote:
>> On Mon, 22 May 2023 at 12:04, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>
>>> On 22/05/2023 03:16, Dmitry Baryshkov wrote:
>>>> On 22/05/2023 00:23, Marijn Suijten wrote:
>>>>> Sony provides an unlabeled LGD + Atmel maXTouch assembly in its Xperia
>>>>> XZ3 (tama akatsuki) phone, with custom DCS commands to match.
>>>>>
>>>>> This panel features Display Stream Compression 1.1.
>>>>>
>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>> ---
>>>>>    drivers/gpu/drm/panel/Kconfig                   |  11 +
>>>>>    drivers/gpu/drm/panel/Makefile                  |   1 +
>>>>>    drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362 ++++++++++++++++++++++++
>>>>>    3 files changed, 374 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
>>>>> index 67ef898d133f2..18bd116e78a71 100644
>>>>> --- a/drivers/gpu/drm/panel/Kconfig
>>>>> +++ b/drivers/gpu/drm/panel/Kconfig
>>>>> @@ -706,6 +706,17 @@ config DRM_PANEL_SONY_ACX565AKM
>>>>>          Say Y here if you want to enable support for the Sony ACX565AKM
>>>>>          800x600 3.5" panel (found on the Nokia N900).
>>>>> +config DRM_PANEL_SONY_AKATSUKI_LGD
>>>>> +    tristate "Sony Xperia XZ3 LGD panel"
>>>>> +    depends on GPIOLIB && OF
>>>>> +    depends on DRM_MIPI_DSI
>>>>> +    depends on BACKLIGHT_CLASS_DEVICE
>>>>> +    help
>>>>> +      Say Y here if you want to enable support for the Sony Xperia XZ3
>>>>> +      1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG Display.
>>>>> +
>>>>> +      This panel uses Display Stream Compression 1.1.
>>>>> +
>>>>>    config DRM_PANEL_SONY_TD4353_JDI
>>>>>        tristate "Sony TD4353 JDI panel"
>>>>>        depends on GPIOLIB && OF
>>>>> diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
>>>>> index ff169781e82d7..85133f73558f3 100644
>>>>> --- a/drivers/gpu/drm/panel/Makefile
>>>>> +++ b/drivers/gpu/drm/panel/Makefile
>>>>> @@ -71,6 +71,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += panel-sitronix-st7701.o
>>>>>    obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o
>>>>>    obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
>>>>>    obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
>>>>> +obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) += panel-sony-akatsuki-lgd.o
>>>>>    obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o
>>>>>    obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) += panel-sony-tulip-truly-nt35521.o
>>>>>    obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o
>>>>> diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>>>>> new file mode 100644
>>>>> index 0000000000000..f55788f963dab
>>>>> --- /dev/null
>>>>> +++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>>>>> @@ -0,0 +1,362 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>> +/*
>>>>> + * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org>
>>>>> + *
>>>>> + * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi.
>>>>> + */
>>>>> +
>>>>> +#include <linux/backlight.h>
>>>>> +#include <linux/delay.h>
>>>>> +#include <linux/gpio/consumer.h>
>>>>> +#include <linux/module.h>
>>>>> +#include <linux/of.h>
>>>>> +#include <linux/of_device.h>
>>>>> +#include <linux/regulator/consumer.h>
>>>>> +
>>>>> +#include <video/mipi_display.h>
>>>>> +
>>>>> +#include <drm/drm_mipi_dsi.h>
>>>>> +#include <drm/drm_modes.h>
>>>>> +#include <drm/drm_panel.h>
>>>>> +#include <drm/drm_probe_helper.h>
>>>>> +#include <drm/display/drm_dsc.h>
>>>>> +#include <drm/display/drm_dsc_helper.h>
>>>>> +
>>>>> +struct sony_akatsuki_lgd {
>>>>> +    struct drm_panel panel;
>>>>> +    struct mipi_dsi_device *dsi;
>>>>> +    struct regulator *vddio;
>>>>> +    struct gpio_desc *reset_gpio;
>>>>> +    bool prepared;
>>>>> +};
>>>>> +
>>>>> +static inline struct sony_akatsuki_lgd *to_sony_akatsuki_lgd(struct drm_panel *panel)
>>>>> +{
>>>>> +    return container_of(panel, struct sony_akatsuki_lgd, panel);
>>>>> +}
>>>>> +
>>>>> +static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx)
>>>>> +{
>>>>> +    struct mipi_dsi_device *dsi = ctx->dsi;
>>>>> +    struct device *dev = &dsi->dev;
>>>>> +    int ret;
>>>>> +
>>>>> +    dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>>>>> +
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01);
>>>>> +    /* Enable backlight control */
>>>>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, BIT(5));
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00);
>>>>> +
>>>>> +    ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(dev, "Failed to set column address: %d\n", ret);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(dev, "Failed to set page address: %d\n", ret);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
>>>>> +
>>>>> +    ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(dev, "Failed to set tear on: %d\n", ret);
>>>>> +        return ret;
>>>>> +    }
>>>>> +
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05);
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00);
>>>>> +
>>>>> +    ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
>>>>> +        return ret;
>>>>> +    }
>>>>> +    msleep(120);
>>>>> +
>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d);
>>>>> +
>>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
>>>>> +    if (ret < 0) {
>>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
>>>>> +        return ret;
>>>>> +    }
>>>>
>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?
>>>
>>>
>>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.
>>>
>>
>> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream?
> 
> I have never investigated what it takes to split these functions, but
> some of these panels do show some corruption at startup which may be
> circumvented by powering the panel on after starting the video stream?
> 
> I'm just not sure where to make the split: downstream does describe a
> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
> the latter only contains set_display_on() (not exit_sleep_mode()).
> It is documented like:
> 
>      same as "qcom,mdss-dsi-on-command" except commands are sent after
>      displaying an image."
> 
> So this seems like the right way to split them up, I'll test this out on
> all submitted panel drivers.

Interesting enough, Neil suggested that sending all the commands during 
pre_enable() is the correct sequence (especially for VIDEO mode panels), 
since not all DSI hosts can send commands after switching to the VIDEO mode.
Dmitry Baryshkov May 29, 2023, 10:36 p.m. UTC | #8 
On 30/05/2023 00:11, Marijn Suijten wrote:
> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
> <snip>
>>> +	if (ctx->dsi->dsc) {
>>
>> dsi->dsc is always set, thus this condition can be dropped.
> 
> I want to leave room for possibly running the panel without DSC (at a
> lower resolution/refresh rate, or at higher power consumption if there
> is enough BW) by not assigning the pointer, if we get access to panel
> documentation: probably one of the magic commands sent in this driver
> controls it but we don't know which.

This sounds like 'a possible room for expansion' which might never be 
actually used. I think, if we ever get such information or when the 
panel's DSC config gets variadic following the mode, we can reintroduce 
this check.

> 
>>> +		drm_dsc_pps_payload_pack(&pps, ctx->dsi->dsc);
>>> +
>>> +		ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
>>> +		if (ret < 0) {
>>> +			dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
>>> +			goto fail;
>>> +		}
>>> +		ret = mipi_dsi_compression_mode(ctx->dsi, true);
>>> +		if (ret < 0) {
>>> +			dev_err(dev, "failed to enable compression mode: %d\n", ret);
>>> +			goto fail;
>>> +		}
>>> +
>>> +		msleep(28);
>>> +	}
>>> +
>>> +	ctx->prepared = true;
>>> +	return 0;
>>> +
>>> +fail:
>>> +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
>>> +	regulator_disable(ctx->vddio);
>>> +	return ret;
>>> +}
> <snip>
>>> +	/* This panel only supports DSC; unconditionally enable it */
Marijn Suijten May 29, 2023, 10:37 p.m. UTC | #9 
On 2023-05-30 01:18:40, Dmitry Baryshkov wrote:
<snip>
> >>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
> >>>>> +    if (ret < 0) {
> >>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
> >>>>> +        return ret;
> >>>>> +    }
> >>>>
> >>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?
> >>>
> >>>
> >>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.
> >>>
> >>
> >> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream?
> > 
> > I have never investigated what it takes to split these functions, but
> > some of these panels do show some corruption at startup which may be
> > circumvented by powering the panel on after starting the video stream?
> > 
> > I'm just not sure where to make the split: downstream does describe a
> > qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
> > the latter only contains set_display_on() (not exit_sleep_mode()).
> > It is documented like:
> > 
> >      same as "qcom,mdss-dsi-on-command" except commands are sent after
> >      displaying an image."
> > 
> > So this seems like the right way to split them up, I'll test this out on
> > all submitted panel drivers.
> 
> Interesting enough, Neil suggested that sending all the commands during 
> pre_enable() is the correct sequence (especially for VIDEO mode panels), 
> since not all DSI hosts can send commands after switching to the VIDEO mode.

Note that all these panels and Driver-ICs are command-mode, and/or
programmed to run in command-mode, so there shouldn't be any notion of a
VIDEO stream (any command-mode frame is just an "arbitrary command" as
far as I understood).

- Marijn
Dmitry Baryshkov May 29, 2023, 10:39 p.m. UTC | #10 
On 30/05/2023 01:37, Marijn Suijten wrote:
> On 2023-05-30 01:18:40, Dmitry Baryshkov wrote:
> <snip>
>>>>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
>>>>>>> +    if (ret < 0) {
>>>>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
>>>>>>> +        return ret;
>>>>>>> +    }
>>>>>>
>>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?
>>>>>
>>>>>
>>>>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.
>>>>>
>>>>
>>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream?
>>>
>>> I have never investigated what it takes to split these functions, but
>>> some of these panels do show some corruption at startup which may be
>>> circumvented by powering the panel on after starting the video stream?
>>>
>>> I'm just not sure where to make the split: downstream does describe a
>>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
>>> the latter only contains set_display_on() (not exit_sleep_mode()).
>>> It is documented like:
>>>
>>>       same as "qcom,mdss-dsi-on-command" except commands are sent after
>>>       displaying an image."
>>>
>>> So this seems like the right way to split them up, I'll test this out on
>>> all submitted panel drivers.
>>
>> Interesting enough, Neil suggested that sending all the commands during
>> pre_enable() is the correct sequence (especially for VIDEO mode panels),
>> since not all DSI hosts can send commands after switching to the VIDEO mode.
> 
> Note that all these panels and Driver-ICs are command-mode, and/or
> programmed to run in command-mode, so there shouldn't be any notion of a
> VIDEO stream (any command-mode frame is just an "arbitrary command" as
> far as I understood).

Yes, from the data stream point of view. I was talking about the DSI 
host being able to send arbitrary commands or not after enabling the 
video/cmd stream.

> 
> - Marijn
Neil Armstrong May 30, 2023, 7:24 a.m. UTC | #11 
Hi Marijn, Dmitry, Caleb, Jessica,

On 29/05/2023 23:11, Marijn Suijten wrote:
> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
> <snip>
>>> +	if (ctx->dsi->dsc) {
>>
>> dsi->dsc is always set, thus this condition can be dropped.
> 
> I want to leave room for possibly running the panel without DSC (at a
> lower resolution/refresh rate, or at higher power consumption if there
> is enough BW) by not assigning the pointer, if we get access to panel
> documentation: probably one of the magic commands sent in this driver
> controls it but we don't know which.

I'd like to investigate if DSC should perhaps only be enabled if we
run non certain platforms/socs ?

I mean, we don't know if the controller supports DSC and those particular
DSC parameters so we should probably start adding something like :

static drm_dsc_config dsc_params_qcom = {}

static const struct of_device_id panel_of_dsc_params[] = {
	{ .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
	{ .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
	{ .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
	{ .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
};

...
static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
...
	const struct of_device_id *match;

...
	match = of_match_node(panel_of_dsc_params, of_root);
	if (match && match->data) {
		dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
		memcpy(dsi->dsc, match->data, sizeof(*dsc));
	} else {
		dev_warn(&dsi->dev, "DSI controller is not marked as supporting DSC\n");
	}
...
}

and probably bail out if it's a DSC only panel.

We could alternatively match on the DSI controller's dsi->host->dev instead of the SoC root compatible.

Neil

> 
>>> +		drm_dsc_pps_payload_pack(&pps, ctx->dsi->dsc);
>>> +
>>> +		ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
>>> +		if (ret < 0) {
>>> +			dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
>>> +			goto fail;
>>> +		}
>>> +		ret = mipi_dsi_compression_mode(ctx->dsi, true);
>>> +		if (ret < 0) {
>>> +			dev_err(dev, "failed to enable compression mode: %d\n", ret);
>>> +			goto fail;
>>> +		}
>>> +
>>> +		msleep(28);
>>> +	}
>>> +
>>> +	ctx->prepared = true;
>>> +	return 0;
>>> +
>>> +fail:
>>> +	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
>>> +	regulator_disable(ctx->vddio);
>>> +	return ret;
>>> +}
> <snip>
>>> +	/* This panel only supports DSC; unconditionally enable it */
> 
> On that note I should perhaps reword this.
> 
>>> +	dsi->dsc = dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
>>
>> I think double assignments are frowned upon.
> 
> Ack.
> 
>>
>>> +	if (!dsc)
>>> +		return -ENOMEM;
>>> +
>>> +	dsc->dsc_version_major = 1;
>>> +	dsc->dsc_version_minor = 1;
>>> +
>>> +	dsc->slice_height = 32;
>>> +	dsc->slice_count = 2;
>>> +	// TODO: Get hdisplay from the mode
>>
>> Would you like to fix the TODO?
> 
> I can't unless either migrating to drm_bridge (is that doable?) or
> expand drm_panel.  That's a larger task, but I don't think this driver
> is the right place to track that desire.  Should I drop the comment
> entirely or reword it?
> 
>>> +	WARN_ON(1440 % dsc->slice_count);
>>> +	dsc->slice_width = 1440 / dsc->slice_count;
> 
> <snip>
> 
> - Marijn
Marijn Suijten May 30, 2023, 8:27 a.m. UTC | #12 
On 2023-05-30 01:39:10, Dmitry Baryshkov wrote:
> On 30/05/2023 01:37, Marijn Suijten wrote:
> > On 2023-05-30 01:18:40, Dmitry Baryshkov wrote:
> > <snip>
> >>>>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
> >>>>>>> +    if (ret < 0) {
> >>>>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
> >>>>>>> +        return ret;
> >>>>>>> +    }
> >>>>>>
> >>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?
> >>>>>
> >>>>>
> >>>>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.
> >>>>>
> >>>>
> >>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream?
> >>>
> >>> I have never investigated what it takes to split these functions, but
> >>> some of these panels do show some corruption at startup which may be
> >>> circumvented by powering the panel on after starting the video stream?
> >>>
> >>> I'm just not sure where to make the split: downstream does describe a
> >>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
> >>> the latter only contains set_display_on() (not exit_sleep_mode()).
> >>> It is documented like:
> >>>
> >>>       same as "qcom,mdss-dsi-on-command" except commands are sent after
> >>>       displaying an image."
> >>>
> >>> So this seems like the right way to split them up, I'll test this out on
> >>> all submitted panel drivers.
> >>
> >> Interesting enough, Neil suggested that sending all the commands during
> >> pre_enable() is the correct sequence (especially for VIDEO mode panels),
> >> since not all DSI hosts can send commands after switching to the VIDEO mode.
> > 
> > Note that all these panels and Driver-ICs are command-mode, and/or
> > programmed to run in command-mode, so there shouldn't be any notion of a
> > VIDEO stream (any command-mode frame is just an "arbitrary command" as
> > far as I understood).
> 
> Yes, from the data stream point of view. I was talking about the DSI 
> host being able to send arbitrary commands or not after enabling the 
> video/cmd stream.

Is this a known limitation of SM8250 then?  Or is the brightness_set
issue more likely a "problem" with the panel that the downstream kernel
is "somehow" working around or aware of, and I just haven't found
how/where it deals with that?
(Alternatively we could be "doing it wrong" for other panels but it
 turns out to be working anyway)

- Marijn
Marijn Suijten May 30, 2023, 8:41 a.m. UTC | #13 
On 2023-05-30 09:24:24, Neil Armstrong wrote:
> Hi Marijn, Dmitry, Caleb, Jessica,
> 
> On 29/05/2023 23:11, Marijn Suijten wrote:
> > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
> > <snip>
> >>> +	if (ctx->dsi->dsc) {
> >>
> >> dsi->dsc is always set, thus this condition can be dropped.
> > 
> > I want to leave room for possibly running the panel without DSC (at a
> > lower resolution/refresh rate, or at higher power consumption if there
> > is enough BW) by not assigning the pointer, if we get access to panel
> > documentation: probably one of the magic commands sent in this driver
> > controls it but we don't know which.
> 
> I'd like to investigate if DSC should perhaps only be enabled if we
> run non certain platforms/socs ?
> 
> I mean, we don't know if the controller supports DSC and those particular
> DSC parameters so we should probably start adding something like :
> 
> static drm_dsc_config dsc_params_qcom = {}
> 
> static const struct of_device_id panel_of_dsc_params[] = {
> 	{ .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
> 	{ .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
> 	{ .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
> 	{ .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
> };

I'd absolutely hate hardcoding a list of compatible SoC names in a panel
driver.  For one these lists will fall out of date really soon even if
we store this list in a generic place: even the current DPU catalog and
new entries floating on the lists weren't faithfully representing DSC
capabilities (but that's all being / been fixed now).

What's more, most of these panel drivers are "hardcoded" for a specific
(smartphone) device (and SoC...) since we don't (usually) have the
DrIC/panel name nor documentation to make the commands generic enough.
I don't think we should be specific on that end, while being generic on
the DSC side.

That does mean I'll remove the if (dsc) here, as Dmitry noted most of
this driver expects/requires it is enabled.

> ...
> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
> ...
> 	const struct of_device_id *match;
> 
> ...
> 	match = of_match_node(panel_of_dsc_params, of_root);
> 	if (match && match->data) {
> 		dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
> 		memcpy(dsi->dsc, match->data, sizeof(*dsc));
> 	} else {
> 		dev_warn(&dsi->dev, "DSI controller is not marked as supporting DSC\n");
> 	}
> ...
> }
> 
> and probably bail out if it's a DSC only panel.
> 
> We could alternatively match on the DSI controller's dsi->host->dev instead of the SoC root compatible.

I'd much rather have the DSI host/controller state whether it is capable
of DSC (likely allowing us to expose different modes for panels that
support toggling DSC), but for starters also validate (in DPU?) that the
pointer is NULL when the hardware does not support it (but maybe that
already happens implicitly somewhere in e.g.
dpu_encoder_virt_atomic_mode_set when finding the DSC blocks).

- Marijn
Konrad Dybcio May 30, 2023, 9:29 a.m. UTC | #14 
On 30.05.2023 10:41, Marijn Suijten wrote:
> On 2023-05-30 09:24:24, Neil Armstrong wrote:
>> Hi Marijn, Dmitry, Caleb, Jessica,
>>
>> On 29/05/2023 23:11, Marijn Suijten wrote:
>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
>>> <snip>
>>>>> +	if (ctx->dsi->dsc) {
>>>>
>>>> dsi->dsc is always set, thus this condition can be dropped.
>>>
>>> I want to leave room for possibly running the panel without DSC (at a
>>> lower resolution/refresh rate, or at higher power consumption if there
>>> is enough BW) by not assigning the pointer, if we get access to panel
>>> documentation: probably one of the magic commands sent in this driver
>>> controls it but we don't know which.
>>
>> I'd like to investigate if DSC should perhaps only be enabled if we
>> run non certain platforms/socs ?
>>
>> I mean, we don't know if the controller supports DSC and those particular
>> DSC parameters so we should probably start adding something like :
>>
>> static drm_dsc_config dsc_params_qcom = {}
>>
>> static const struct of_device_id panel_of_dsc_params[] = {
>> 	{ .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
>> 	{ .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
>> 	{ .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
>> 	{ .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
>> };
> 
> I'd absolutely hate hardcoding a list of compatible SoC names in a panel
> driver.  For one these lists will fall out of date really soon even if
> we store this list in a generic place: even the current DPU catalog and
> new entries floating on the lists weren't faithfully representing DSC
> capabilities (but that's all being / been fixed now).
Yes, a driver should behave predictably, regardless of the platform.

> 
> What's more, most of these panel drivers are "hardcoded" for a specific
> (smartphone) device (and SoC...) since we don't (usually) have the
> DrIC/panel name nor documentation to make the commands generic enough.
> I don't think we should be specific on that end, while being generic on
> the DSC side.
> 
> That does mean I'll remove the if (dsc) here, as Dmitry noted most of
> this driver expects/requires it is enabled.
I'd say we could assume it's mandatory as of today.

Konrad
> 
>> ...
>> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
>> ...
>> 	const struct of_device_id *match;
>>
>> ...
>> 	match = of_match_node(panel_of_dsc_params, of_root);
>> 	if (match && match->data) {
>> 		dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
>> 		memcpy(dsi->dsc, match->data, sizeof(*dsc));
>> 	} else {
>> 		dev_warn(&dsi->dev, "DSI controller is not marked as supporting DSC\n");
>> 	}
>> ...
>> }
>>
>> and probably bail out if it's a DSC only panel.
>>
>> We could alternatively match on the DSI controller's dsi->host->dev instead of the SoC root compatible.
> 
> I'd much rather have the DSI host/controller state whether it is capable
> of DSC (likely allowing us to expose different modes for panels that
> support toggling DSC), but for starters also validate (in DPU?) that the
> pointer is NULL when the hardware does not support it (but maybe that
> already happens implicitly somewhere in e.g.
> dpu_encoder_virt_atomic_mode_set when finding the DSC blocks).
> 
> - Marijn
Dmitry Baryshkov May 30, 2023, 11:11 a.m. UTC | #15 
On Tue, 30 May 2023 at 11:27, Marijn Suijten
<marijn.suijten@somainline.org> wrote:
>
> On 2023-05-30 01:39:10, Dmitry Baryshkov wrote:
> > On 30/05/2023 01:37, Marijn Suijten wrote:
> > > On 2023-05-30 01:18:40, Dmitry Baryshkov wrote:
> > > <snip>
> > >>>>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
> > >>>>>>> +    if (ret < 0) {
> > >>>>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
> > >>>>>>> +        return ret;
> > >>>>>>> +    }
> > >>>>>>
> > >>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?
> > >>>>>
> > >>>>>
> > >>>>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.
> > >>>>>
> > >>>>
> > >>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream?
> > >>>
> > >>> I have never investigated what it takes to split these functions, but
> > >>> some of these panels do show some corruption at startup which may be
> > >>> circumvented by powering the panel on after starting the video stream?
> > >>>
> > >>> I'm just not sure where to make the split: downstream does describe a
> > >>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
> > >>> the latter only contains set_display_on() (not exit_sleep_mode()).
> > >>> It is documented like:
> > >>>
> > >>>       same as "qcom,mdss-dsi-on-command" except commands are sent after
> > >>>       displaying an image."
> > >>>
> > >>> So this seems like the right way to split them up, I'll test this out on
> > >>> all submitted panel drivers.
> > >>
> > >> Interesting enough, Neil suggested that sending all the commands during
> > >> pre_enable() is the correct sequence (especially for VIDEO mode panels),
> > >> since not all DSI hosts can send commands after switching to the VIDEO mode.
> > >
> > > Note that all these panels and Driver-ICs are command-mode, and/or
> > > programmed to run in command-mode, so there shouldn't be any notion of a
> > > VIDEO stream (any command-mode frame is just an "arbitrary command" as
> > > far as I understood).
> >
> > Yes, from the data stream point of view. I was talking about the DSI
> > host being able to send arbitrary commands or not after enabling the
> > video/cmd stream.
>
> Is this a known limitation of SM8250 then?  Or is the brightness_set
> issue more likely a "problem" with the panel that the downstream kernel
> is "somehow" working around or aware of, and I just haven't found
> how/where it deals with that?
> (Alternatively we could be "doing it wrong" for other panels but it
>  turns out to be working anyway)

Please excuse me for not being explicit enough. Qualcomm hardware
doesn't have this problem. Thus I was completely unaware of it before
talking to Neil.
So, our hardware works in most of the cases.
Dmitry Baryshkov May 30, 2023, 11:44 a.m. UTC | #16 
On Tue, 30 May 2023 at 10:24, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>
> Hi Marijn, Dmitry, Caleb, Jessica,
>
> On 29/05/2023 23:11, Marijn Suijten wrote:
> > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
> > <snip>
> >>> +   if (ctx->dsi->dsc) {
> >>
> >> dsi->dsc is always set, thus this condition can be dropped.
> >
> > I want to leave room for possibly running the panel without DSC (at a
> > lower resolution/refresh rate, or at higher power consumption if there
> > is enough BW) by not assigning the pointer, if we get access to panel
> > documentation: probably one of the magic commands sent in this driver
> > controls it but we don't know which.
>
> I'd like to investigate if DSC should perhaps only be enabled if we
> run non certain platforms/socs ?
>
> I mean, we don't know if the controller supports DSC and those particular
> DSC parameters so we should probably start adding something like :
>
> static drm_dsc_config dsc_params_qcom = {}
>
> static const struct of_device_id panel_of_dsc_params[] = {
>         { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
>         { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
>         { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
>         { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
> };

I think this would damage the reusability of the drivers. The panel
driver does not actually care if the SoC is SM8350, sunxi-something or
RCar.
Instead it cares about host capabilities.

I think instead we should extend mipi_dsi_host:

#define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
#define MIPI_DSI_HOST_MODE_CMD  BIT(1)
#define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
// FIXME: do we need to provide additional caps here ?

#define MIPI_DSI_DSC_1_1 BIT(0)
#define MIPI_DSI_DSC_1_2 BIT(1)
#define MIPI_DSI_DSC_NATIVE_422 BIT(2)
#define MIPI_DSI_DSC_NATIVE_420 BIT(3)
#define MIPI_DSI_DSC_FRAC_BPP BIT(4)
// etc.

struct mipi_dsi_host {
 // new fields only
  unsigned long mode_flags;
  unsigned long dsc_flags;
};

Then the panel driver can adapt itself to the host capabilities and
(possibly) select one of the internally supported DSC profiles.

>
> ...
> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
> ...
>         const struct of_device_id *match;
>
> ...
>         match = of_match_node(panel_of_dsc_params, of_root);
>         if (match && match->data) {
>                 dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
>                 memcpy(dsi->dsc, match->data, sizeof(*dsc));
>         } else {
>                 dev_warn(&dsi->dev, "DSI controller is not marked as supporting DSC\n");
>         }
> ...
> }
>
> and probably bail out if it's a DSC only panel.
>
> We could alternatively match on the DSI controller's dsi->host->dev instead of the SoC root compatible.
>
> Neil
AngeloGioacchino Del Regno May 30, 2023, 12:15 p.m. UTC | #17 
Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
> On Tue, 30 May 2023 at 10:24, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>
>> Hi Marijn, Dmitry, Caleb, Jessica,
>>
>> On 29/05/2023 23:11, Marijn Suijten wrote:
>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
>>> <snip>
>>>>> +   if (ctx->dsi->dsc) {
>>>>
>>>> dsi->dsc is always set, thus this condition can be dropped.
>>>
>>> I want to leave room for possibly running the panel without DSC (at a
>>> lower resolution/refresh rate, or at higher power consumption if there
>>> is enough BW) by not assigning the pointer, if we get access to panel
>>> documentation: probably one of the magic commands sent in this driver
>>> controls it but we don't know which.
>>
>> I'd like to investigate if DSC should perhaps only be enabled if we
>> run non certain platforms/socs ?
>>
>> I mean, we don't know if the controller supports DSC and those particular
>> DSC parameters so we should probably start adding something like :
>>
>> static drm_dsc_config dsc_params_qcom = {}
>>
>> static const struct of_device_id panel_of_dsc_params[] = {
>>          { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
>>          { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
>>          { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
>>          { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
>> };
> 
> I think this would damage the reusability of the drivers. The panel
> driver does not actually care if the SoC is SM8350, sunxi-something or
> RCar.
> Instead it cares about host capabilities.
> 
> I think instead we should extend mipi_dsi_host:
> 
> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
> #define MIPI_DSI_HOST_MODE_CMD  BIT(1)
> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
> // FIXME: do we need to provide additional caps here ?
> 
> #define MIPI_DSI_DSC_1_1 BIT(0)
> #define MIPI_DSI_DSC_1_2 BIT(1)
> #define MIPI_DSI_DSC_NATIVE_422 BIT(2)
> #define MIPI_DSI_DSC_NATIVE_420 BIT(3)
> #define MIPI_DSI_DSC_FRAC_BPP BIT(4)
> // etc.
> 
> struct mipi_dsi_host {
>   // new fields only
>    unsigned long mode_flags;
>    unsigned long dsc_flags;
> };
> 
> Then the panel driver can adapt itself to the host capabilities and
> (possibly) select one of the internally supported DSC profiles.
> 

I completely agree about extending mipi_dsi_host, other SoCs could reuse that and
support for DSC panels would become a lot cleaner.

For example, on MediaTek DRM there's some support for DSC, more or less the same
for SPRD DRM and some DSI bridge drivers... having a clean infrastructure would
definitely help.

I'm sad I cannot offer testing in that case because despite being sure that there
are MTK smartphones around with DSI panels using DSC, I have none... and all of the
Chromebooks are not using DSC anyway (but using DisplayPort compression, which is
obviously an entirely different beast).

>>
>> ...
>> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
>> ...
>>          const struct of_device_id *match;
>>
>> ...
>>          match = of_match_node(panel_of_dsc_params, of_root);
>>          if (match && match->data) {
>>                  dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
>>                  memcpy(dsi->dsc, match->data, sizeof(*dsc));
>>          } else {
>>                  dev_warn(&dsi->dev, "DSI controller is not marked as supporting DSC\n");
>>          }
>> ...
>> }
>>
>> and probably bail out if it's a DSC only panel.
>>

Usually DDICs support both DSC and non-DSC modes, depending on the initial
programming (read: init commands)... but the usual issue is that many DDICs
are not publicly documented for reasons, so yes, bailing out if DSC is not
supported would be the only option, and would be fine at this point.

Cheers,
Angelo

>> We could alternatively match on the DSI controller's dsi->host->dev instead of the SoC root compatible.
>>
>> Neil
>
Dmitry Baryshkov May 30, 2023, 12:36 p.m. UTC | #18 
On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote:
> Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
>> On Tue, 30 May 2023 at 10:24, Neil Armstrong 
>> <neil.armstrong@linaro.org> wrote:
>>>
>>> Hi Marijn, Dmitry, Caleb, Jessica,
>>>
>>> On 29/05/2023 23:11, Marijn Suijten wrote:
>>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
>>>> <snip>
>>>>>> +   if (ctx->dsi->dsc) {
>>>>>
>>>>> dsi->dsc is always set, thus this condition can be dropped.
>>>>
>>>> I want to leave room for possibly running the panel without DSC (at a
>>>> lower resolution/refresh rate, or at higher power consumption if there
>>>> is enough BW) by not assigning the pointer, if we get access to panel
>>>> documentation: probably one of the magic commands sent in this driver
>>>> controls it but we don't know which.
>>>
>>> I'd like to investigate if DSC should perhaps only be enabled if we
>>> run non certain platforms/socs ?
>>>
>>> I mean, we don't know if the controller supports DSC and those 
>>> particular
>>> DSC parameters so we should probably start adding something like :
>>>
>>> static drm_dsc_config dsc_params_qcom = {}
>>>
>>> static const struct of_device_id panel_of_dsc_params[] = {
>>>          { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
>>>          { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
>>>          { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
>>>          { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
>>> };
>>
>> I think this would damage the reusability of the drivers. The panel
>> driver does not actually care if the SoC is SM8350, sunxi-something or
>> RCar.
>> Instead it cares about host capabilities.
>>
>> I think instead we should extend mipi_dsi_host:
>>
>> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
>> #define MIPI_DSI_HOST_MODE_CMD  BIT(1)
>> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
>> // FIXME: do we need to provide additional caps here ?
>>
>> #define MIPI_DSI_DSC_1_1 BIT(0)
>> #define MIPI_DSI_DSC_1_2 BIT(1)
>> #define MIPI_DSI_DSC_NATIVE_422 BIT(2)
>> #define MIPI_DSI_DSC_NATIVE_420 BIT(3)
>> #define MIPI_DSI_DSC_FRAC_BPP BIT(4)
>> // etc.
>>
>> struct mipi_dsi_host {
>>   // new fields only
>>    unsigned long mode_flags;
>>    unsigned long dsc_flags;
>> };
>>
>> Then the panel driver can adapt itself to the host capabilities and
>> (possibly) select one of the internally supported DSC profiles.
>>
> 
> I completely agree about extending mipi_dsi_host, other SoCs could reuse 
> that and
> support for DSC panels would become a lot cleaner.

Sounds good. I will wait for one or two more days (to get the possible 
feedback on fields/flags/etc) and post an RFC patch to dri-devel.

> 
> For example, on MediaTek DRM there's some support for DSC, more or less 
> the same
> for SPRD DRM and some DSI bridge drivers... having a clean 
> infrastructure would
> definitely help.
> 
> I'm sad I cannot offer testing in that case because despite being sure 
> that there
> are MTK smartphones around with DSI panels using DSC, I have none... and 
> all of the
> Chromebooks are not using DSC anyway (but using DisplayPort compression, 
> which is
> obviously an entirely different beast).
> 
>>>
>>> ...
>>> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
>>> ...
>>>          const struct of_device_id *match;
>>>
>>> ...
>>>          match = of_match_node(panel_of_dsc_params, of_root);
>>>          if (match && match->data) {
>>>                  dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), 
>>> GFP_KERNEL);
>>>                  memcpy(dsi->dsc, match->data, sizeof(*dsc));
>>>          } else {
>>>                  dev_warn(&dsi->dev, "DSI controller is not marked as 
>>> supporting DSC\n");
>>>          }
>>> ...
>>> }
>>>
>>> and probably bail out if it's a DSC only panel.
>>>
> 
> Usually DDICs support both DSC and non-DSC modes, depending on the initial
> programming (read: init commands)... but the usual issue is that many DDICs
> are not publicly documented for reasons, so yes, bailing out if DSC is not
> supported would be the only option, and would be fine at this point.
> 
> Cheers,
> Angelo
> 
>>> We could alternatively match on the DSI controller's dsi->host->dev 
>>> instead of the SoC root compatible.
>>>
>>> Neil
>>
>
Neil Armstrong May 30, 2023, 3:44 p.m. UTC | #19 
On 30/05/2023 14:36, Dmitry Baryshkov wrote:
> On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote:
>> Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
>>> On Tue, 30 May 2023 at 10:24, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>>
>>>> Hi Marijn, Dmitry, Caleb, Jessica,
>>>>
>>>> On 29/05/2023 23:11, Marijn Suijten wrote:
>>>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
>>>>> <snip>
>>>>>>> +   if (ctx->dsi->dsc) {
>>>>>>
>>>>>> dsi->dsc is always set, thus this condition can be dropped.
>>>>>
>>>>> I want to leave room for possibly running the panel without DSC (at a
>>>>> lower resolution/refresh rate, or at higher power consumption if there
>>>>> is enough BW) by not assigning the pointer, if we get access to panel
>>>>> documentation: probably one of the magic commands sent in this driver
>>>>> controls it but we don't know which.
>>>>
>>>> I'd like to investigate if DSC should perhaps only be enabled if we
>>>> run non certain platforms/socs ?
>>>>
>>>> I mean, we don't know if the controller supports DSC and those particular
>>>> DSC parameters so we should probably start adding something like :
>>>>
>>>> static drm_dsc_config dsc_params_qcom = {}
>>>>
>>>> static const struct of_device_id panel_of_dsc_params[] = {
>>>>          { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
>>>>          { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
>>>>          { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
>>>>          { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
>>>> };
>>>
>>> I think this would damage the reusability of the drivers. The panel
>>> driver does not actually care if the SoC is SM8350, sunxi-something or
>>> RCar.
>>> Instead it cares about host capabilities.
>>>
>>> I think instead we should extend mipi_dsi_host:
>>>
>>> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0)

I assume all DSI controller supports Video mode, so it should be a negative here
if for a reason it's not the case.

There should also be a flag to tell if sending LP commands sending while
in HS Video mode is supported.

>>> #define MIPI_DSI_HOST_MODE_CMD  BIT(1)
>>> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
>>> // FIXME: do we need to provide additional caps here ?
>>>
>>> #define MIPI_DSI_DSC_1_1 BIT(0)
>>> #define MIPI_DSI_DSC_1_2 BIT(1)
>>> #define MIPI_DSI_DSC_NATIVE_422 BIT(2)
>>> #define MIPI_DSI_DSC_NATIVE_420 BIT(3)
>>> #define MIPI_DSI_DSC_FRAC_BPP BIT(4)
>>> // etc.
>>>
>>> struct mipi_dsi_host {
>>>   // new fields only
>>>    unsigned long mode_flags;
>>>    unsigned long dsc_flags;
>>> };
>>>
>>> Then the panel driver can adapt itself to the host capabilities and
>>> (possibly) select one of the internally supported DSC profiles.
>>>
>>
>> I completely agree about extending mipi_dsi_host, other SoCs could reuse that and
>> support for DSC panels would become a lot cleaner.
> 
> Sounds good. I will wait for one or two more days (to get the possible feedback on fields/flags/etc) and post an RFC patch to dri-devel.

Good, I was waiting until a DSC panel appears on the list (and I failed to be the first), it's now the case.

For VTRD6130, the panel is capable of the 4 modes:
- video mode
- command mode
- video mode & DSC
- command mode & DSC

So it would need such info to enable one of the mode in some order to determine.

Thanks,
Neil
> 
>>
>> For example, on MediaTek DRM there's some support for DSC, more or less the same
>> for SPRD DRM and some DSI bridge drivers... having a clean infrastructure would
>> definitely help.
>>
>> I'm sad I cannot offer testing in that case because despite being sure that there
>> are MTK smartphones around with DSI panels using DSC, I have none... and all of the
>> Chromebooks are not using DSC anyway (but using DisplayPort compression, which is
>> obviously an entirely different beast).
>>
>>>>
>>>> ...
>>>> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
>>>> ...
>>>>          const struct of_device_id *match;
>>>>
>>>> ...
>>>>          match = of_match_node(panel_of_dsc_params, of_root);
>>>>          if (match && match->data) {
>>>>                  dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
>>>>                  memcpy(dsi->dsc, match->data, sizeof(*dsc));
>>>>          } else {
>>>>                  dev_warn(&dsi->dev, "DSI controller is not marked as supporting DSC\n");
>>>>          }
>>>> ...
>>>> }
>>>>
>>>> and probably bail out if it's a DSC only panel.
>>>>
>>
>> Usually DDICs support both DSC and non-DSC modes, depending on the initial
>> programming (read: init commands)... but the usual issue is that many DDICs
>> are not publicly documented for reasons, so yes, bailing out if DSC is not
>> supported would be the only option, and would be fine at this point.
>>
>> Cheers,
>> Angelo
>>
>>>> We could alternatively match on the DSI controller's dsi->host->dev instead of the SoC root compatible.
>>>>
>>>> Neil
>>>
>>
>
Abhinav Kumar May 30, 2023, 5:54 p.m. UTC | #20 
On 5/29/2023 3:18 PM, Dmitry Baryshkov wrote:
> On 30/05/2023 00:07, Marijn Suijten wrote:
>> On 2023-05-22 15:58:56, Dmitry Baryshkov wrote:
>>> On Mon, 22 May 2023 at 12:04, Neil Armstrong 
>>> <neil.armstrong@linaro.org> wrote:
>>>>
>>>> On 22/05/2023 03:16, Dmitry Baryshkov wrote:
>>>>> On 22/05/2023 00:23, Marijn Suijten wrote:
>>>>>> Sony provides an unlabeled LGD + Atmel maXTouch assembly in its 
>>>>>> Xperia
>>>>>> XZ3 (tama akatsuki) phone, with custom DCS commands to match.
>>>>>>
>>>>>> This panel features Display Stream Compression 1.1.
>>>>>>
>>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>>> ---
>>>>>>    drivers/gpu/drm/panel/Kconfig                   |  11 +
>>>>>>    drivers/gpu/drm/panel/Makefile                  |   1 +
>>>>>>    drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362 
>>>>>> ++++++++++++++++++++++++
>>>>>>    3 files changed, 374 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/panel/Kconfig 
>>>>>> b/drivers/gpu/drm/panel/Kconfig
>>>>>> index 67ef898d133f2..18bd116e78a71 100644
>>>>>> --- a/drivers/gpu/drm/panel/Kconfig
>>>>>> +++ b/drivers/gpu/drm/panel/Kconfig
>>>>>> @@ -706,6 +706,17 @@ config DRM_PANEL_SONY_ACX565AKM
>>>>>>          Say Y here if you want to enable support for the Sony 
>>>>>> ACX565AKM
>>>>>>          800x600 3.5" panel (found on the Nokia N900).
>>>>>> +config DRM_PANEL_SONY_AKATSUKI_LGD
>>>>>> +    tristate "Sony Xperia XZ3 LGD panel"
>>>>>> +    depends on GPIOLIB && OF
>>>>>> +    depends on DRM_MIPI_DSI
>>>>>> +    depends on BACKLIGHT_CLASS_DEVICE
>>>>>> +    help
>>>>>> +      Say Y here if you want to enable support for the Sony 
>>>>>> Xperia XZ3
>>>>>> +      1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG 
>>>>>> Display.
>>>>>> +
>>>>>> +      This panel uses Display Stream Compression 1.1.
>>>>>> +
>>>>>>    config DRM_PANEL_SONY_TD4353_JDI
>>>>>>        tristate "Sony TD4353 JDI panel"
>>>>>>        depends on GPIOLIB && OF
>>>>>> diff --git a/drivers/gpu/drm/panel/Makefile 
>>>>>> b/drivers/gpu/drm/panel/Makefile
>>>>>> index ff169781e82d7..85133f73558f3 100644
>>>>>> --- a/drivers/gpu/drm/panel/Makefile
>>>>>> +++ b/drivers/gpu/drm/panel/Makefile
>>>>>> @@ -71,6 +71,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += 
>>>>>> panel-sitronix-st7701.o
>>>>>>    obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o
>>>>>>    obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += 
>>>>>> panel-sitronix-st7789v.o
>>>>>>    obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
>>>>>> +obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) += 
>>>>>> panel-sony-akatsuki-lgd.o
>>>>>>    obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o
>>>>>>    obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) += 
>>>>>> panel-sony-tulip-truly-nt35521.o
>>>>>>    obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o
>>>>>> diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c 
>>>>>> b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>>>>>> new file mode 100644
>>>>>> index 0000000000000..f55788f963dab
>>>>>> --- /dev/null
>>>>>> +++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>>>>>> @@ -0,0 +1,362 @@
>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>> +/*
>>>>>> + * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org>
>>>>>> + *
>>>>>> + * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi.
>>>>>> + */
>>>>>> +
>>>>>> +#include <linux/backlight.h>
>>>>>> +#include <linux/delay.h>
>>>>>> +#include <linux/gpio/consumer.h>
>>>>>> +#include <linux/module.h>
>>>>>> +#include <linux/of.h>
>>>>>> +#include <linux/of_device.h>
>>>>>> +#include <linux/regulator/consumer.h>
>>>>>> +
>>>>>> +#include <video/mipi_display.h>
>>>>>> +
>>>>>> +#include <drm/drm_mipi_dsi.h>
>>>>>> +#include <drm/drm_modes.h>
>>>>>> +#include <drm/drm_panel.h>
>>>>>> +#include <drm/drm_probe_helper.h>
>>>>>> +#include <drm/display/drm_dsc.h>
>>>>>> +#include <drm/display/drm_dsc_helper.h>
>>>>>> +
>>>>>> +struct sony_akatsuki_lgd {
>>>>>> +    struct drm_panel panel;
>>>>>> +    struct mipi_dsi_device *dsi;
>>>>>> +    struct regulator *vddio;
>>>>>> +    struct gpio_desc *reset_gpio;
>>>>>> +    bool prepared;
>>>>>> +};
>>>>>> +
>>>>>> +static inline struct sony_akatsuki_lgd 
>>>>>> *to_sony_akatsuki_lgd(struct drm_panel *panel)
>>>>>> +{
>>>>>> +    return container_of(panel, struct sony_akatsuki_lgd, panel);
>>>>>> +}
>>>>>> +
>>>>>> +static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx)
>>>>>> +{
>>>>>> +    struct mipi_dsi_device *dsi = ctx->dsi;
>>>>>> +    struct device *dev = &dsi->dev;
>>>>>> +    int ret;
>>>>>> +
>>>>>> +    dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>>>>>> +
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01);
>>>>>> +    /* Enable backlight control */
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 
>>>>>> BIT(5));
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00);
>>>>>> +
>>>>>> +    ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1);
>>>>>> +    if (ret < 0) {
>>>>>> +        dev_err(dev, "Failed to set column address: %d\n", ret);
>>>>>> +        return ret;
>>>>>> +    }
>>>>>> +
>>>>>> +    ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1);
>>>>>> +    if (ret < 0) {
>>>>>> +        dev_err(dev, "Failed to set page address: %d\n", ret);
>>>>>> +        return ret;
>>>>>> +    }
>>>>>> +
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
>>>>>> +
>>>>>> +    ret = mipi_dsi_dcs_set_tear_on(dsi, 
>>>>>> MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>>>>>> +    if (ret < 0) {
>>>>>> +        dev_err(dev, "Failed to set tear on: %d\n", ret);
>>>>>> +        return ret;
>>>>>> +    }
>>>>>> +
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05);
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00);
>>>>>> +
>>>>>> +    ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>>>>>> +    if (ret < 0) {
>>>>>> +        dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
>>>>>> +        return ret;
>>>>>> +    }
>>>>>> +    msleep(120);
>>>>>> +
>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d);
>>>>>> +
>>>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
>>>>>> +    if (ret < 0) {
>>>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
>>>>>> +        return ret;
>>>>>> +    }
>>>>>
>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / 
>>>>> mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() 
>>>>> part?
>>>>
>>>>
>>>> No, prepare is called before the video stream is started and when 
>>>> display is still in LPM mode and the mode hasn't been set.
>>>>
>>>
>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting 
>>> the stream?
>>
>> I have never investigated what it takes to split these functions, but
>> some of these panels do show some corruption at startup which may be
>> circumvented by powering the panel on after starting the video stream?
>>
>> I'm just not sure where to make the split: downstream does describe a
>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
>> the latter only contains set_display_on() (not exit_sleep_mode()).
>> It is documented like:
>>
>>      same as "qcom,mdss-dsi-on-command" except commands are sent after
>>      displaying an image."
>>
>> So this seems like the right way to split them up, I'll test this out on
>> all submitted panel drivers.
> 
> Interesting enough, Neil suggested that sending all the commands during 
> pre_enable() is the correct sequence (especially for VIDEO mode panels), 
> since not all DSI hosts can send commands after switching to the VIDEO 
> mode.
> 

I agree with Neil here.

Yes, it does seem natural to think that sending the video stream before 
sending the on commands would avoid any potential corruption / garbage 
screen issues.

But even from panel side should allow that. I have seen panel ON 
sequences where some explicitly ask for ON commands before the video stream.

So, we cannot really generalize it and needs to be treated on a 
host-to-host and panel-to-panel basis.
Marijn Suijten May 30, 2023, 6:13 p.m. UTC | #21 
On 2023-05-30 10:54:17, Abhinav Kumar wrote:
> > On 30/05/2023 00:07, Marijn Suijten wrote:
> >> On 2023-05-22 15:58:56, Dmitry Baryshkov wrote:
> >>> On Mon, 22 May 2023 at 12:04, Neil Armstrong 
> >>> <neil.armstrong@linaro.org> wrote:
> >>>>
> >>>> On 22/05/2023 03:16, Dmitry Baryshkov wrote:
> >>>>> On 22/05/2023 00:23, Marijn Suijten wrote:
> >>>>>> Sony provides an unlabeled LGD + Atmel maXTouch assembly in its 
> >>>>>> Xperia
> >>>>>> XZ3 (tama akatsuki) phone, with custom DCS commands to match.
> >>>>>>
> >>>>>> This panel features Display Stream Compression 1.1.
> >>>>>>
> >>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
> >>>>>> ---
> >>>>>>    drivers/gpu/drm/panel/Kconfig                   |  11 +
> >>>>>>    drivers/gpu/drm/panel/Makefile                  |   1 +
> >>>>>>    drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362 
> >>>>>> ++++++++++++++++++++++++
> >>>>>>    3 files changed, 374 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/gpu/drm/panel/Kconfig 
> >>>>>> b/drivers/gpu/drm/panel/Kconfig
> >>>>>> index 67ef898d133f2..18bd116e78a71 100644
> >>>>>> --- a/drivers/gpu/drm/panel/Kconfig
> >>>>>> +++ b/drivers/gpu/drm/panel/Kconfig
> >>>>>> @@ -706,6 +706,17 @@ config DRM_PANEL_SONY_ACX565AKM
> >>>>>>          Say Y here if you want to enable support for the Sony 
> >>>>>> ACX565AKM
> >>>>>>          800x600 3.5" panel (found on the Nokia N900).
> >>>>>> +config DRM_PANEL_SONY_AKATSUKI_LGD
> >>>>>> +    tristate "Sony Xperia XZ3 LGD panel"
> >>>>>> +    depends on GPIOLIB && OF
> >>>>>> +    depends on DRM_MIPI_DSI
> >>>>>> +    depends on BACKLIGHT_CLASS_DEVICE
> >>>>>> +    help
> >>>>>> +      Say Y here if you want to enable support for the Sony 
> >>>>>> Xperia XZ3
> >>>>>> +      1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG 
> >>>>>> Display.
> >>>>>> +
> >>>>>> +      This panel uses Display Stream Compression 1.1.
> >>>>>> +
> >>>>>>    config DRM_PANEL_SONY_TD4353_JDI
> >>>>>>        tristate "Sony TD4353 JDI panel"
> >>>>>>        depends on GPIOLIB && OF
> >>>>>> diff --git a/drivers/gpu/drm/panel/Makefile 
> >>>>>> b/drivers/gpu/drm/panel/Makefile
> >>>>>> index ff169781e82d7..85133f73558f3 100644
> >>>>>> --- a/drivers/gpu/drm/panel/Makefile
> >>>>>> +++ b/drivers/gpu/drm/panel/Makefile
> >>>>>> @@ -71,6 +71,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += 
> >>>>>> panel-sitronix-st7701.o
> >>>>>>    obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o
> >>>>>>    obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += 
> >>>>>> panel-sitronix-st7789v.o
> >>>>>>    obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
> >>>>>> +obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) += 
> >>>>>> panel-sony-akatsuki-lgd.o
> >>>>>>    obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o
> >>>>>>    obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) += 
> >>>>>> panel-sony-tulip-truly-nt35521.o
> >>>>>>    obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o
> >>>>>> diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c 
> >>>>>> b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
> >>>>>> new file mode 100644
> >>>>>> index 0000000000000..f55788f963dab
> >>>>>> --- /dev/null
> >>>>>> +++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
> >>>>>> @@ -0,0 +1,362 @@
> >>>>>> +// SPDX-License-Identifier: GPL-2.0-only
> >>>>>> +/*
> >>>>>> + * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org>
> >>>>>> + *
> >>>>>> + * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi.
> >>>>>> + */
> >>>>>> +
> >>>>>> +#include <linux/backlight.h>
> >>>>>> +#include <linux/delay.h>
> >>>>>> +#include <linux/gpio/consumer.h>
> >>>>>> +#include <linux/module.h>
> >>>>>> +#include <linux/of.h>
> >>>>>> +#include <linux/of_device.h>
> >>>>>> +#include <linux/regulator/consumer.h>
> >>>>>> +
> >>>>>> +#include <video/mipi_display.h>
> >>>>>> +
> >>>>>> +#include <drm/drm_mipi_dsi.h>
> >>>>>> +#include <drm/drm_modes.h>
> >>>>>> +#include <drm/drm_panel.h>
> >>>>>> +#include <drm/drm_probe_helper.h>
> >>>>>> +#include <drm/display/drm_dsc.h>
> >>>>>> +#include <drm/display/drm_dsc_helper.h>
> >>>>>> +
> >>>>>> +struct sony_akatsuki_lgd {
> >>>>>> +    struct drm_panel panel;
> >>>>>> +    struct mipi_dsi_device *dsi;
> >>>>>> +    struct regulator *vddio;
> >>>>>> +    struct gpio_desc *reset_gpio;
> >>>>>> +    bool prepared;
> >>>>>> +};
> >>>>>> +
> >>>>>> +static inline struct sony_akatsuki_lgd 
> >>>>>> *to_sony_akatsuki_lgd(struct drm_panel *panel)
> >>>>>> +{
> >>>>>> +    return container_of(panel, struct sony_akatsuki_lgd, panel);
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx)
> >>>>>> +{
> >>>>>> +    struct mipi_dsi_device *dsi = ctx->dsi;
> >>>>>> +    struct device *dev = &dsi->dev;
> >>>>>> +    int ret;
> >>>>>> +
> >>>>>> +    dsi->mode_flags |= MIPI_DSI_MODE_LPM;
> >>>>>> +
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01);
> >>>>>> +    /* Enable backlight control */
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 
> >>>>>> BIT(5));
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00);
> >>>>>> +
> >>>>>> +    ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1);
> >>>>>> +    if (ret < 0) {
> >>>>>> +        dev_err(dev, "Failed to set column address: %d\n", ret);
> >>>>>> +        return ret;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1);
> >>>>>> +    if (ret < 0) {
> >>>>>> +        dev_err(dev, "Failed to set page address: %d\n", ret);
> >>>>>> +        return ret;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
> >>>>>> +
> >>>>>> +    ret = mipi_dsi_dcs_set_tear_on(dsi, 
> >>>>>> MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> >>>>>> +    if (ret < 0) {
> >>>>>> +        dev_err(dev, "Failed to set tear on: %d\n", ret);
> >>>>>> +        return ret;
> >>>>>> +    }
> >>>>>> +
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05);
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00);
> >>>>>> +
> >>>>>> +    ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
> >>>>>> +    if (ret < 0) {
> >>>>>> +        dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
> >>>>>> +        return ret;
> >>>>>> +    }
> >>>>>> +    msleep(120);
> >>>>>> +
> >>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d);
> >>>>>> +
> >>>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
> >>>>>> +    if (ret < 0) {
> >>>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
> >>>>>> +        return ret;
> >>>>>> +    }
> >>>>>
> >>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / 
> >>>>> mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() 
> >>>>> part?
> >>>>
> >>>>
> >>>> No, prepare is called before the video stream is started and when 
> >>>> display is still in LPM mode and the mode hasn't been set.
> >>>>
> >>>
> >>> Yes, that's my point. Shouldn't we enable the panel _after_ starting 
> >>> the stream?
> >>
> >> I have never investigated what it takes to split these functions, but
> >> some of these panels do show some corruption at startup which may be
> >> circumvented by powering the panel on after starting the video stream?
> >>
> >> I'm just not sure where to make the split: downstream does describe a
> >> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
> >> the latter only contains set_display_on() (not exit_sleep_mode()).
> >> It is documented like:
> >>
> >>      same as "qcom,mdss-dsi-on-command" except commands are sent after
> >>      displaying an image."
> >>
> >> So this seems like the right way to split them up, I'll test this out on
> >> all submitted panel drivers.
> > 
> > Interesting enough, Neil suggested that sending all the commands during 
> > pre_enable() is the correct sequence (especially for VIDEO mode panels), 
> > since not all DSI hosts can send commands after switching to the VIDEO 
> > mode.
> > 
> 
> I agree with Neil here.
> 
> Yes, it does seem natural to think that sending the video stream before 
> sending the on commands would avoid any potential corruption / garbage 
> screen issues.
> 
> But even from panel side should allow that. I have seen panel ON 
> sequences where some explicitly ask for ON commands before the video stream.
> 
> So, we cannot really generalize it and needs to be treated on a 
> host-to-host and panel-to-panel basis.

All four panel drivers in this series have a `post-panel-on` separation
(containing _just_ the panel_on DCS) and should be implemented with a
separate .enable callback.

More importantly: is the API to enable/disable, and separately
prepare/unprepare the panel exposed to userspace?  And is there an app
(maybe as part of mesa/drm where modetest lives) that is able to trigger
these calls to test adequate behaviour?  In the past I've seen panels
working, until my userspace suspends and turn the panel off, where it
never comes back up properly after.

- Marijn
Marijn Suijten May 30, 2023, 6:19 p.m. UTC | #22 
On 2023-05-30 14:11:06, Dmitry Baryshkov wrote:
> On Tue, 30 May 2023 at 11:27, Marijn Suijten
> <marijn.suijten@somainline.org> wrote:
> >
> > On 2023-05-30 01:39:10, Dmitry Baryshkov wrote:
> > > On 30/05/2023 01:37, Marijn Suijten wrote:
> > > > On 2023-05-30 01:18:40, Dmitry Baryshkov wrote:
> > > > <snip>
> > > >>>>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
> > > >>>>>>> +    if (ret < 0) {
> > > >>>>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
> > > >>>>>>> +        return ret;
> > > >>>>>>> +    }
> > > >>>>>>
> > > >>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() / mipi_dsi_dcs_set_display_on() be moved from prepare() to enable() part?
> > > >>>>>
> > > >>>>>
> > > >>>>> No, prepare is called before the video stream is started and when display is still in LPM mode and the mode hasn't been set.
> > > >>>>>
> > > >>>>
> > > >>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting the stream?
> > > >>>
> > > >>> I have never investigated what it takes to split these functions, but
> > > >>> some of these panels do show some corruption at startup which may be
> > > >>> circumvented by powering the panel on after starting the video stream?
> > > >>>
> > > >>> I'm just not sure where to make the split: downstream does describe a
> > > >>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
> > > >>> the latter only contains set_display_on() (not exit_sleep_mode()).
> > > >>> It is documented like:
> > > >>>
> > > >>>       same as "qcom,mdss-dsi-on-command" except commands are sent after
> > > >>>       displaying an image."
> > > >>>
> > > >>> So this seems like the right way to split them up, I'll test this out on
> > > >>> all submitted panel drivers.
> > > >>
> > > >> Interesting enough, Neil suggested that sending all the commands during
> > > >> pre_enable() is the correct sequence (especially for VIDEO mode panels),
> > > >> since not all DSI hosts can send commands after switching to the VIDEO mode.
> > > >
> > > > Note that all these panels and Driver-ICs are command-mode, and/or
> > > > programmed to run in command-mode, so there shouldn't be any notion of a
> > > > VIDEO stream (any command-mode frame is just an "arbitrary command" as
> > > > far as I understood).
> > >
> > > Yes, from the data stream point of view. I was talking about the DSI
> > > host being able to send arbitrary commands or not after enabling the
> > > video/cmd stream.
> >
> > Is this a known limitation of SM8250 then?  Or is the brightness_set
> > issue more likely a "problem" with the panel that the downstream kernel
> > is "somehow" working around or aware of, and I just haven't found
> > how/where it deals with that?
> > (Alternatively we could be "doing it wrong" for other panels but it
> >  turns out to be working anyway)
> 
> Please excuse me for not being explicit enough. Qualcomm hardware
> doesn't have this problem. Thus I was completely unaware of it before
> talking to Neil.
> So, our hardware works in most of the cases.

Also excuse me for mocking the hardware here; it seems quite illogical
for it to not work on this specific device which is more likely a
failure in porting the panel DT to the driver than related to this
specific SoC. There's probably one of the hundred-or-so DT params
responsible for triggering a setting, delay, or other magic sequence
that gets the brightness toggle working.

- Marijn
Dmitry Baryshkov May 30, 2023, 11:16 p.m. UTC | #23 
On 30/05/2023 21:13, Marijn Suijten wrote:
> On 2023-05-30 10:54:17, Abhinav Kumar wrote:
>>> On 30/05/2023 00:07, Marijn Suijten wrote:
>>>> On 2023-05-22 15:58:56, Dmitry Baryshkov wrote:
>>>>> On Mon, 22 May 2023 at 12:04, Neil Armstrong
>>>>> <neil.armstrong@linaro.org> wrote:
>>>>>>
>>>>>> On 22/05/2023 03:16, Dmitry Baryshkov wrote:
>>>>>>> On 22/05/2023 00:23, Marijn Suijten wrote:
>>>>>>>> Sony provides an unlabeled LGD + Atmel maXTouch assembly in its
>>>>>>>> Xperia
>>>>>>>> XZ3 (tama akatsuki) phone, with custom DCS commands to match.
>>>>>>>>
>>>>>>>> This panel features Display Stream Compression 1.1.
>>>>>>>>
>>>>>>>> Signed-off-by: Marijn Suijten <marijn.suijten@somainline.org>
>>>>>>>> ---
>>>>>>>>     drivers/gpu/drm/panel/Kconfig                   |  11 +
>>>>>>>>     drivers/gpu/drm/panel/Makefile                  |   1 +
>>>>>>>>     drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c | 362
>>>>>>>> ++++++++++++++++++++++++
>>>>>>>>     3 files changed, 374 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/panel/Kconfig
>>>>>>>> b/drivers/gpu/drm/panel/Kconfig
>>>>>>>> index 67ef898d133f2..18bd116e78a71 100644
>>>>>>>> --- a/drivers/gpu/drm/panel/Kconfig
>>>>>>>> +++ b/drivers/gpu/drm/panel/Kconfig
>>>>>>>> @@ -706,6 +706,17 @@ config DRM_PANEL_SONY_ACX565AKM
>>>>>>>>           Say Y here if you want to enable support for the Sony
>>>>>>>> ACX565AKM
>>>>>>>>           800x600 3.5" panel (found on the Nokia N900).
>>>>>>>> +config DRM_PANEL_SONY_AKATSUKI_LGD
>>>>>>>> +    tristate "Sony Xperia XZ3 LGD panel"
>>>>>>>> +    depends on GPIOLIB && OF
>>>>>>>> +    depends on DRM_MIPI_DSI
>>>>>>>> +    depends on BACKLIGHT_CLASS_DEVICE
>>>>>>>> +    help
>>>>>>>> +      Say Y here if you want to enable support for the Sony
>>>>>>>> Xperia XZ3
>>>>>>>> +      1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG
>>>>>>>> Display.
>>>>>>>> +
>>>>>>>> +      This panel uses Display Stream Compression 1.1.
>>>>>>>> +
>>>>>>>>     config DRM_PANEL_SONY_TD4353_JDI
>>>>>>>>         tristate "Sony TD4353 JDI panel"
>>>>>>>>         depends on GPIOLIB && OF
>>>>>>>> diff --git a/drivers/gpu/drm/panel/Makefile
>>>>>>>> b/drivers/gpu/drm/panel/Makefile
>>>>>>>> index ff169781e82d7..85133f73558f3 100644
>>>>>>>> --- a/drivers/gpu/drm/panel/Makefile
>>>>>>>> +++ b/drivers/gpu/drm/panel/Makefile
>>>>>>>> @@ -71,6 +71,7 @@ obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) +=
>>>>>>>> panel-sitronix-st7701.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) +=
>>>>>>>> panel-sitronix-st7789v.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
>>>>>>>> +obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) +=
>>>>>>>> panel-sony-akatsuki-lgd.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) +=
>>>>>>>> panel-sony-tulip-truly-nt35521.o
>>>>>>>>     obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o
>>>>>>>> diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>>>>>>>> b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000000000..f55788f963dab
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
>>>>>>>> @@ -0,0 +1,362 @@
>>>>>>>> +// SPDX-License-Identifier: GPL-2.0-only
>>>>>>>> +/*
>>>>>>>> + * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org>
>>>>>>>> + *
>>>>>>>> + * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi.
>>>>>>>> + */
>>>>>>>> +
>>>>>>>> +#include <linux/backlight.h>
>>>>>>>> +#include <linux/delay.h>
>>>>>>>> +#include <linux/gpio/consumer.h>
>>>>>>>> +#include <linux/module.h>
>>>>>>>> +#include <linux/of.h>
>>>>>>>> +#include <linux/of_device.h>
>>>>>>>> +#include <linux/regulator/consumer.h>
>>>>>>>> +
>>>>>>>> +#include <video/mipi_display.h>
>>>>>>>> +
>>>>>>>> +#include <drm/drm_mipi_dsi.h>
>>>>>>>> +#include <drm/drm_modes.h>
>>>>>>>> +#include <drm/drm_panel.h>
>>>>>>>> +#include <drm/drm_probe_helper.h>
>>>>>>>> +#include <drm/display/drm_dsc.h>
>>>>>>>> +#include <drm/display/drm_dsc_helper.h>
>>>>>>>> +
>>>>>>>> +struct sony_akatsuki_lgd {
>>>>>>>> +    struct drm_panel panel;
>>>>>>>> +    struct mipi_dsi_device *dsi;
>>>>>>>> +    struct regulator *vddio;
>>>>>>>> +    struct gpio_desc *reset_gpio;
>>>>>>>> +    bool prepared;
>>>>>>>> +};
>>>>>>>> +
>>>>>>>> +static inline struct sony_akatsuki_lgd
>>>>>>>> *to_sony_akatsuki_lgd(struct drm_panel *panel)
>>>>>>>> +{
>>>>>>>> +    return container_of(panel, struct sony_akatsuki_lgd, panel);
>>>>>>>> +}
>>>>>>>> +
>>>>>>>> +static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx)
>>>>>>>> +{
>>>>>>>> +    struct mipi_dsi_device *dsi = ctx->dsi;
>>>>>>>> +    struct device *dev = &dsi->dev;
>>>>>>>> +    int ret;
>>>>>>>> +
>>>>>>>> +    dsi->mode_flags |= MIPI_DSI_MODE_LPM;
>>>>>>>> +
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01);
>>>>>>>> +    /* Enable backlight control */
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY,
>>>>>>>> BIT(5));
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00);
>>>>>>>> +
>>>>>>>> +    ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1);
>>>>>>>> +    if (ret < 0) {
>>>>>>>> +        dev_err(dev, "Failed to set column address: %d\n", ret);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1);
>>>>>>>> +    if (ret < 0) {
>>>>>>>> +        dev_err(dev, "Failed to set page address: %d\n", ret);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
>>>>>>>> +
>>>>>>>> +    ret = mipi_dsi_dcs_set_tear_on(dsi,
>>>>>>>> MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>>>>>>>> +    if (ret < 0) {
>>>>>>>> +        dev_err(dev, "Failed to set tear on: %d\n", ret);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>> +
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05);
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00);
>>>>>>>> +
>>>>>>>> +    ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
>>>>>>>> +    if (ret < 0) {
>>>>>>>> +        dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>> +    msleep(120);
>>>>>>>> +
>>>>>>>> +    mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d);
>>>>>>>> +
>>>>>>>> +    ret = mipi_dsi_dcs_set_display_on(dsi);
>>>>>>>> +    if (ret < 0) {
>>>>>>>> +        dev_err(dev, "Failed to turn display on: %d\n", ret);
>>>>>>>> +        return ret;
>>>>>>>> +    }
>>>>>>>
>>>>>>> My usual question: should the mipi_dsi_dcs_exit_sleep_mode() /
>>>>>>> mipi_dsi_dcs_set_display_on() be moved from prepare() to enable()
>>>>>>> part?
>>>>>>
>>>>>>
>>>>>> No, prepare is called before the video stream is started and when
>>>>>> display is still in LPM mode and the mode hasn't been set.
>>>>>>
>>>>>
>>>>> Yes, that's my point. Shouldn't we enable the panel _after_ starting
>>>>> the stream?
>>>>
>>>> I have never investigated what it takes to split these functions, but
>>>> some of these panels do show some corruption at startup which may be
>>>> circumvented by powering the panel on after starting the video stream?
>>>>
>>>> I'm just not sure where to make the split: downstream does describe a
>>>> qcom,mdss-dsi-on-command and qcom,mdss-dsi-post-panel-on-command, where
>>>> the latter only contains set_display_on() (not exit_sleep_mode()).
>>>> It is documented like:
>>>>
>>>>       same as "qcom,mdss-dsi-on-command" except commands are sent after
>>>>       displaying an image."
>>>>
>>>> So this seems like the right way to split them up, I'll test this out on
>>>> all submitted panel drivers.
>>>
>>> Interesting enough, Neil suggested that sending all the commands during
>>> pre_enable() is the correct sequence (especially for VIDEO mode panels),
>>> since not all DSI hosts can send commands after switching to the VIDEO
>>> mode.
>>>
>>
>> I agree with Neil here.
>>
>> Yes, it does seem natural to think that sending the video stream before
>> sending the on commands would avoid any potential corruption / garbage
>> screen issues.
>>
>> But even from panel side should allow that. I have seen panel ON
>> sequences where some explicitly ask for ON commands before the video stream.
>>
>> So, we cannot really generalize it and needs to be treated on a
>> host-to-host and panel-to-panel basis.
> 
> All four panel drivers in this series have a `post-panel-on` separation
> (containing _just_ the panel_on DCS) and should be implemented with a
> separate .enable callback.
> 
> More importantly: is the API to enable/disable, and separately
> prepare/unprepare the panel exposed to userspace?  And is there an app
> (maybe as part of mesa/drm where modetest lives) that is able to trigger
> these calls to test adequate behaviour?  In the past I've seen panels
> working, until my userspace suspends and turn the panel off, where it
> never comes back up properly after.

No. The prepare/enable (and disable/unrepare) translate to the bridge's 
pre_enable/enable and disable/post_disable callbacks, which are a part 
of modesetting. One can not call them separately.
AngeloGioacchino Del Regno May 31, 2023, 8:02 a.m. UTC | #24 
Il 30/05/23 17:44, Neil Armstrong ha scritto:
> On 30/05/2023 14:36, Dmitry Baryshkov wrote:
>> On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote:
>>> Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
>>>> On Tue, 30 May 2023 at 10:24, Neil Armstrong <neil.armstrong@linaro.org> wrote:
>>>>>
>>>>> Hi Marijn, Dmitry, Caleb, Jessica,
>>>>>
>>>>> On 29/05/2023 23:11, Marijn Suijten wrote:
>>>>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
>>>>>> <snip>
>>>>>>>> +   if (ctx->dsi->dsc) {
>>>>>>>
>>>>>>> dsi->dsc is always set, thus this condition can be dropped.
>>>>>>
>>>>>> I want to leave room for possibly running the panel without DSC (at a
>>>>>> lower resolution/refresh rate, or at higher power consumption if there
>>>>>> is enough BW) by not assigning the pointer, if we get access to panel
>>>>>> documentation: probably one of the magic commands sent in this driver
>>>>>> controls it but we don't know which.
>>>>>
>>>>> I'd like to investigate if DSC should perhaps only be enabled if we
>>>>> run non certain platforms/socs ?
>>>>>
>>>>> I mean, we don't know if the controller supports DSC and those particular
>>>>> DSC parameters so we should probably start adding something like :
>>>>>
>>>>> static drm_dsc_config dsc_params_qcom = {}
>>>>>
>>>>> static const struct of_device_id panel_of_dsc_params[] = {
>>>>>          { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
>>>>>          { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
>>>>>          { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
>>>>>          { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
>>>>> };
>>>>
>>>> I think this would damage the reusability of the drivers. The panel
>>>> driver does not actually care if the SoC is SM8350, sunxi-something or
>>>> RCar.
>>>> Instead it cares about host capabilities.
>>>>
>>>> I think instead we should extend mipi_dsi_host:
>>>>
>>>> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
> 
> I assume all DSI controller supports Video mode, so it should be a negative here
> if for a reason it's not the case.

Either all positive or all negative... and yes I agree that all DSI controllers
support video mode nowadays, but:
  - Will that be true for future controllers? (likely yes, but you never know)
  - Is there any controller driver not implementing video mode?
    - Will there be one in the future?

> 
> There should also be a flag to tell if sending LP commands sending while
> in HS Video mode is supported.
> 

+1. This is the case for both qcom and mtk.

>>>> #define MIPI_DSI_HOST_MODE_CMD  BIT(1)
>>>> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
>>>> // FIXME: do we need to provide additional caps here ?
>>>>
>>>> #define MIPI_DSI_DSC_1_1 BIT(0)
>>>> #define MIPI_DSI_DSC_1_2 BIT(1)
>>>> #define MIPI_DSI_DSC_NATIVE_422 BIT(2)
>>>> #define MIPI_DSI_DSC_NATIVE_420 BIT(3)
>>>> #define MIPI_DSI_DSC_FRAC_BPP BIT(4)
>>>> // etc.
>>>>
>>>> struct mipi_dsi_host {
>>>>   // new fields only
>>>>    unsigned long mode_flags;
>>>>    unsigned long dsc_flags;
>>>> };
>>>>
>>>> Then the panel driver can adapt itself to the host capabilities and
>>>> (possibly) select one of the internally supported DSC profiles.
>>>>
>>>
>>> I completely agree about extending mipi_dsi_host, other SoCs could reuse that and
>>> support for DSC panels would become a lot cleaner.
>>
>> Sounds good. I will wait for one or two more days (to get the possible feedback 
>> on fields/flags/etc) and post an RFC patch to dri-devel.
> 
> Good, I was waiting until a DSC panel appears on the list (and I failed to be the 
> first), it's now the case.
> 
> For VTRD6130, the panel is capable of the 4 modes:
> - video mode
> - command mode
> - video mode & DSC
> - command mode & DSC
> 
> So it would need such info to enable one of the mode in some order to determine.
> 

Dynamically determining is not trivial, as that depends on multiple variables:
  - Availability of the modes (obviously)
  - Available lanes
    - Available bandwidth per lane
      - Available total bandwidth
  - Power consumption considerations (DSC IP may be using more or less power
    depending on the actual SoC//controller)
    - Thermal management: DSC may make no thermal sense as in, more heat output
      vs thermal envelope (laptop vs embedded vs handset)
  - Others

Hence, the implementation should also provide a way of choosing a preferred mode
on a per-controller basis (DSC or no compression).

Just a few considerations that came to mind with a good sleep.

Cheers!

> Thanks,
> Neil
>>
>>>
>>> For example, on MediaTek DRM there's some support for DSC, more or less the same
>>> for SPRD DRM and some DSI bridge drivers... having a clean infrastructure would
>>> definitely help.
>>>
>>> I'm sad I cannot offer testing in that case because despite being sure that there
>>> are MTK smartphones around with DSI panels using DSC, I have none... and all of the
>>> Chromebooks are not using DSC anyway (but using DisplayPort compression, which is
>>> obviously an entirely different beast).
>>>
>>>>>
>>>>> ...
>>>>> static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
>>>>> ...
>>>>>          const struct of_device_id *match;
>>>>>
>>>>> ...
>>>>>          match = of_match_node(panel_of_dsc_params, of_root);
>>>>>          if (match && match->data) {
>>>>>                  dsi->dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
>>>>>                  memcpy(dsi->dsc, match->data, sizeof(*dsc));
>>>>>          } else {
>>>>>                  dev_warn(&dsi->dev, "DSI controller is not marked as 
>>>>> supporting DSC\n");
>>>>>          }
>>>>> ...
>>>>> }
>>>>>
>>>>> and probably bail out if it's a DSC only panel.
>>>>>
>>>
>>> Usually DDICs support both DSC and non-DSC modes, depending on the initial
>>> programming (read: init commands)... but the usual issue is that many DDICs
>>> are not publicly documented for reasons, so yes, bailing out if DSC is not
>>> supported would be the only option, and would be fine at this point.
>>>
>>> Cheers,
>>> Angelo
>>>
>>>>> We could alternatively match on the DSI controller's dsi->host->dev instead of 
>>>>> the SoC root compatible.
>>>>>
>>>>> Neil
>>>>
>>>
>>
>
Maxime Ripard July 5, 2023, 12:04 p.m. UTC | #25 
Hi,

On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote:
> On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote:
> > Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
> > > On Tue, 30 May 2023 at 10:24, Neil Armstrong
> > > <neil.armstrong@linaro.org> wrote:
> > > > 
> > > > Hi Marijn, Dmitry, Caleb, Jessica,
> > > > 
> > > > On 29/05/2023 23:11, Marijn Suijten wrote:
> > > > > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
> > > > > <snip>
> > > > > > > +   if (ctx->dsi->dsc) {
> > > > > > 
> > > > > > dsi->dsc is always set, thus this condition can be dropped.
> > > > > 
> > > > > I want to leave room for possibly running the panel without DSC (at a
> > > > > lower resolution/refresh rate, or at higher power consumption if there
> > > > > is enough BW) by not assigning the pointer, if we get access to panel
> > > > > documentation: probably one of the magic commands sent in this driver
> > > > > controls it but we don't know which.
> > > > 
> > > > I'd like to investigate if DSC should perhaps only be enabled if we
> > > > run non certain platforms/socs ?
> > > > 
> > > > I mean, we don't know if the controller supports DSC and those
> > > > particular
> > > > DSC parameters so we should probably start adding something like :
> > > > 
> > > > static drm_dsc_config dsc_params_qcom = {}
> > > > 
> > > > static const struct of_device_id panel_of_dsc_params[] = {
> > > >          { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
> > > >          { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
> > > >          { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
> > > >          { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
> > > > };
> > > 
> > > I think this would damage the reusability of the drivers. The panel
> > > driver does not actually care if the SoC is SM8350, sunxi-something or
> > > RCar.
> > > Instead it cares about host capabilities.
> > > 
> > > I think instead we should extend mipi_dsi_host:
> > > 
> > > #define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
> > > #define MIPI_DSI_HOST_MODE_CMD  BIT(1)
> > > #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
> > > // FIXME: do we need to provide additional caps here ?
> > > 
> > > #define MIPI_DSI_DSC_1_1 BIT(0)
> > > #define MIPI_DSI_DSC_1_2 BIT(1)
> > > #define MIPI_DSI_DSC_NATIVE_422 BIT(2)
> > > #define MIPI_DSI_DSC_NATIVE_420 BIT(3)
> > > #define MIPI_DSI_DSC_FRAC_BPP BIT(4)
> > > // etc.
> > > 
> > > struct mipi_dsi_host {
> > >   // new fields only
> > >    unsigned long mode_flags;
> > >    unsigned long dsc_flags;
> > > };
> > > 
> > > Then the panel driver can adapt itself to the host capabilities and
> > > (possibly) select one of the internally supported DSC profiles.
> > > 
> > 
> > I completely agree about extending mipi_dsi_host, other SoCs could reuse
> > that and
> > support for DSC panels would become a lot cleaner.
> 
> Sounds good. I will wait for one or two more days (to get the possible
> feedback on fields/flags/etc) and post an RFC patch to dri-devel.

I just came across that discussion, and couldn't find those patches, did
you ever send them?

Either way, I'm not really sure it's a good idea to multiply the
capabilities flags of the DSI host, and we should just stick to the
spec. If the spec says that we have to support DSC while video is
output, then that's what the panels should expect.

If a host isn't able to provide that, it's a bug and we should fix the
controller driver instead of creating a workaround in the core for
broken drivers.

Another concern I have is that, those broken drivers are usually the
undocumented ones that already have trouble supporting the most trivial
setup. Creating more combinations both at the controller and panel level
will just make it harder for those drivers.

Maxime
Neil Armstrong July 5, 2023, 1:05 p.m. UTC | #26 
On 05/07/2023 14:04, Maxime Ripard wrote:
> Hi,
> 
> On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote:
>> On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote:
>>> Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
>>>> On Tue, 30 May 2023 at 10:24, Neil Armstrong
>>>> <neil.armstrong@linaro.org> wrote:
>>>>>
>>>>> Hi Marijn, Dmitry, Caleb, Jessica,
>>>>>
>>>>> On 29/05/2023 23:11, Marijn Suijten wrote:
>>>>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
>>>>>> <snip>
>>>>>>>> +   if (ctx->dsi->dsc) {
>>>>>>>
>>>>>>> dsi->dsc is always set, thus this condition can be dropped.
>>>>>>
>>>>>> I want to leave room for possibly running the panel without DSC (at a
>>>>>> lower resolution/refresh rate, or at higher power consumption if there
>>>>>> is enough BW) by not assigning the pointer, if we get access to panel
>>>>>> documentation: probably one of the magic commands sent in this driver
>>>>>> controls it but we don't know which.
>>>>>
>>>>> I'd like to investigate if DSC should perhaps only be enabled if we
>>>>> run non certain platforms/socs ?
>>>>>
>>>>> I mean, we don't know if the controller supports DSC and those
>>>>> particular
>>>>> DSC parameters so we should probably start adding something like :
>>>>>
>>>>> static drm_dsc_config dsc_params_qcom = {}
>>>>>
>>>>> static const struct of_device_id panel_of_dsc_params[] = {
>>>>>           { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
>>>>>           { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
>>>>>           { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
>>>>>           { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
>>>>> };
>>>>
>>>> I think this would damage the reusability of the drivers. The panel
>>>> driver does not actually care if the SoC is SM8350, sunxi-something or
>>>> RCar.
>>>> Instead it cares about host capabilities.
>>>>
>>>> I think instead we should extend mipi_dsi_host:
>>>>
>>>> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
>>>> #define MIPI_DSI_HOST_MODE_CMD  BIT(1)
>>>> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
>>>> // FIXME: do we need to provide additional caps here ?
>>>>
>>>> #define MIPI_DSI_DSC_1_1 BIT(0)
>>>> #define MIPI_DSI_DSC_1_2 BIT(1)
>>>> #define MIPI_DSI_DSC_NATIVE_422 BIT(2)
>>>> #define MIPI_DSI_DSC_NATIVE_420 BIT(3)
>>>> #define MIPI_DSI_DSC_FRAC_BPP BIT(4)
>>>> // etc.
>>>>
>>>> struct mipi_dsi_host {
>>>>    // new fields only
>>>>     unsigned long mode_flags;
>>>>     unsigned long dsc_flags;
>>>> };
>>>>
>>>> Then the panel driver can adapt itself to the host capabilities and
>>>> (possibly) select one of the internally supported DSC profiles.
>>>>
>>>
>>> I completely agree about extending mipi_dsi_host, other SoCs could reuse
>>> that and
>>> support for DSC panels would become a lot cleaner.
>>
>> Sounds good. I will wait for one or two more days (to get the possible
>> feedback on fields/flags/etc) and post an RFC patch to dri-devel.
> 
> I just came across that discussion, and couldn't find those patches, did
> you ever send them?
> 
> Either way, I'm not really sure it's a good idea to multiply the
> capabilities flags of the DSI host, and we should just stick to the
> spec. If the spec says that we have to support DSC while video is
> output, then that's what the panels should expect.

Except some panels supports DSC & non-DSC, Video and Command mode, and
all that is runtime configurable. How do you handle that ?

> 
> If a host isn't able to provide that, it's a bug and we should fix the
> controller driver instead of creating a workaround in the core for
> broken drivers.
> 
> Another concern I have is that, those broken drivers are usually the
> undocumented ones that already have trouble supporting the most trivial
> setup. Creating more combinations both at the controller and panel level
> will just make it harder for those drivers.
> 
> Maxime
Maxime Ripard July 5, 2023, 1:29 p.m. UTC | #27 
On Wed, Jul 05, 2023 at 03:05:33PM +0200, Neil Armstrong wrote:
> On 05/07/2023 14:04, Maxime Ripard wrote:
> > Hi,
> > 
> > On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote:
> > > On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote:
> > > > Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
> > > > > On Tue, 30 May 2023 at 10:24, Neil Armstrong
> > > > > <neil.armstrong@linaro.org> wrote:
> > > > > > 
> > > > > > Hi Marijn, Dmitry, Caleb, Jessica,
> > > > > > 
> > > > > > On 29/05/2023 23:11, Marijn Suijten wrote:
> > > > > > > On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
> > > > > > > <snip>
> > > > > > > > > +   if (ctx->dsi->dsc) {
> > > > > > > > 
> > > > > > > > dsi->dsc is always set, thus this condition can be dropped.
> > > > > > > 
> > > > > > > I want to leave room for possibly running the panel without DSC (at a
> > > > > > > lower resolution/refresh rate, or at higher power consumption if there
> > > > > > > is enough BW) by not assigning the pointer, if we get access to panel
> > > > > > > documentation: probably one of the magic commands sent in this driver
> > > > > > > controls it but we don't know which.
> > > > > > 
> > > > > > I'd like to investigate if DSC should perhaps only be enabled if we
> > > > > > run non certain platforms/socs ?
> > > > > > 
> > > > > > I mean, we don't know if the controller supports DSC and those
> > > > > > particular
> > > > > > DSC parameters so we should probably start adding something like :
> > > > > > 
> > > > > > static drm_dsc_config dsc_params_qcom = {}
> > > > > > 
> > > > > > static const struct of_device_id panel_of_dsc_params[] = {
> > > > > >           { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
> > > > > >           { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
> > > > > >           { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
> > > > > >           { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
> > > > > > };
> > > > > 
> > > > > I think this would damage the reusability of the drivers. The panel
> > > > > driver does not actually care if the SoC is SM8350, sunxi-something or
> > > > > RCar.
> > > > > Instead it cares about host capabilities.
> > > > > 
> > > > > I think instead we should extend mipi_dsi_host:
> > > > > 
> > > > > #define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
> > > > > #define MIPI_DSI_HOST_MODE_CMD  BIT(1)
> > > > > #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
> > > > > // FIXME: do we need to provide additional caps here ?
> > > > > 
> > > > > #define MIPI_DSI_DSC_1_1 BIT(0)
> > > > > #define MIPI_DSI_DSC_1_2 BIT(1)
> > > > > #define MIPI_DSI_DSC_NATIVE_422 BIT(2)
> > > > > #define MIPI_DSI_DSC_NATIVE_420 BIT(3)
> > > > > #define MIPI_DSI_DSC_FRAC_BPP BIT(4)
> > > > > // etc.
> > > > > 
> > > > > struct mipi_dsi_host {
> > > > >    // new fields only
> > > > >     unsigned long mode_flags;
> > > > >     unsigned long dsc_flags;
> > > > > };
> > > > > 
> > > > > Then the panel driver can adapt itself to the host capabilities and
> > > > > (possibly) select one of the internally supported DSC profiles.
> > > > > 
> > > > 
> > > > I completely agree about extending mipi_dsi_host, other SoCs could reuse
> > > > that and
> > > > support for DSC panels would become a lot cleaner.
> > > 
> > > Sounds good. I will wait for one or two more days (to get the possible
> > > feedback on fields/flags/etc) and post an RFC patch to dri-devel.
> > 
> > I just came across that discussion, and couldn't find those patches, did
> > you ever send them?
> > 
> > Either way, I'm not really sure it's a good idea to multiply the
> > capabilities flags of the DSI host, and we should just stick to the
> > spec. If the spec says that we have to support DSC while video is
> > output, then that's what the panels should expect.
> 
> Except some panels supports DSC & non-DSC, Video and Command mode, and
> all that is runtime configurable. How do you handle that ?

In this case, most of the constraints are going to be on the encoder
still so it should be the one driving it. The panel will only care about
which mode has been selected, but it shouldn't be the one driving it,
and thus we still don't really need to expose the host capabilities.

This is very much like HDMI: the encoder knows what the monitor is
capable of, will take a decision based on its capabilities and the
monitor's and will then let the monitor know. But the monitor never
knows what the encoder is truly capable of, nor will it enforce
something.

Maxime
Dmitry Baryshkov July 5, 2023, 1:37 p.m. UTC | #28 
On 05/07/2023 16:29, Maxime Ripard wrote:
> On Wed, Jul 05, 2023 at 03:05:33PM +0200, Neil Armstrong wrote:
>> On 05/07/2023 14:04, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Tue, May 30, 2023 at 03:36:04PM +0300, Dmitry Baryshkov wrote:
>>>> On 30/05/2023 15:15, AngeloGioacchino Del Regno wrote:
>>>>> Il 30/05/23 13:44, Dmitry Baryshkov ha scritto:
>>>>>> On Tue, 30 May 2023 at 10:24, Neil Armstrong
>>>>>> <neil.armstrong@linaro.org> wrote:
>>>>>>>
>>>>>>> Hi Marijn, Dmitry, Caleb, Jessica,
>>>>>>>
>>>>>>> On 29/05/2023 23:11, Marijn Suijten wrote:
>>>>>>>> On 2023-05-22 04:16:20, Dmitry Baryshkov wrote:
>>>>>>>> <snip>
>>>>>>>>>> +   if (ctx->dsi->dsc) {
>>>>>>>>>
>>>>>>>>> dsi->dsc is always set, thus this condition can be dropped.
>>>>>>>>
>>>>>>>> I want to leave room for possibly running the panel without DSC (at a
>>>>>>>> lower resolution/refresh rate, or at higher power consumption if there
>>>>>>>> is enough BW) by not assigning the pointer, if we get access to panel
>>>>>>>> documentation: probably one of the magic commands sent in this driver
>>>>>>>> controls it but we don't know which.
>>>>>>>
>>>>>>> I'd like to investigate if DSC should perhaps only be enabled if we
>>>>>>> run non certain platforms/socs ?
>>>>>>>
>>>>>>> I mean, we don't know if the controller supports DSC and those
>>>>>>> particular
>>>>>>> DSC parameters so we should probably start adding something like :
>>>>>>>
>>>>>>> static drm_dsc_config dsc_params_qcom = {}
>>>>>>>
>>>>>>> static const struct of_device_id panel_of_dsc_params[] = {
>>>>>>>            { .compatible = "qcom,sm8150", , .data = &dsc_params_qcom },
>>>>>>>            { .compatible = "qcom,sm8250", , .data = &dsc_params_qcom },
>>>>>>>            { .compatible = "qcom,sm8350", , .data = &dsc_params_qcom },
>>>>>>>            { .compatible = "qcom,sm8450", , .data = &dsc_params_qcom },
>>>>>>> };
>>>>>>
>>>>>> I think this would damage the reusability of the drivers. The panel
>>>>>> driver does not actually care if the SoC is SM8350, sunxi-something or
>>>>>> RCar.
>>>>>> Instead it cares about host capabilities.
>>>>>>
>>>>>> I think instead we should extend mipi_dsi_host:
>>>>>>
>>>>>> #define MIPI_DSI_HOST_MODE_VIDEO BIT(0)
>>>>>> #define MIPI_DSI_HOST_MODE_CMD  BIT(1)
>>>>>> #define MIPI_DSI_HOST_VIDEO_SUPPORTS_COMMANDS BIT(2)
>>>>>> // FIXME: do we need to provide additional caps here ?
>>>>>>
>>>>>> #define MIPI_DSI_DSC_1_1 BIT(0)
>>>>>> #define MIPI_DSI_DSC_1_2 BIT(1)
>>>>>> #define MIPI_DSI_DSC_NATIVE_422 BIT(2)
>>>>>> #define MIPI_DSI_DSC_NATIVE_420 BIT(3)
>>>>>> #define MIPI_DSI_DSC_FRAC_BPP BIT(4)
>>>>>> // etc.
>>>>>>
>>>>>> struct mipi_dsi_host {
>>>>>>     // new fields only
>>>>>>      unsigned long mode_flags;
>>>>>>      unsigned long dsc_flags;
>>>>>> };
>>>>>>
>>>>>> Then the panel driver can adapt itself to the host capabilities and
>>>>>> (possibly) select one of the internally supported DSC profiles.
>>>>>>
>>>>>
>>>>> I completely agree about extending mipi_dsi_host, other SoCs could reuse
>>>>> that and
>>>>> support for DSC panels would become a lot cleaner.
>>>>
>>>> Sounds good. I will wait for one or two more days (to get the possible
>>>> feedback on fields/flags/etc) and post an RFC patch to dri-devel.
>>>
>>> I just came across that discussion, and couldn't find those patches, did
>>> you ever send them?

No, I got sidetracked by other issues.

>>>
>>> Either way, I'm not really sure it's a good idea to multiply the
>>> capabilities flags of the DSI host, and we should just stick to the
>>> spec. If the spec says that we have to support DSC while video is
>>> output, then that's what the panels should expect.
>>
>> Except some panels supports DSC & non-DSC, Video and Command mode, and
>> all that is runtime configurable. How do you handle that ?
> 
> In this case, most of the constraints are going to be on the encoder
> still so it should be the one driving it. The panel will only care about
> which mode has been selected, but it shouldn't be the one driving it,
> and thus we still don't really need to expose the host capabilities.

This is an interesting perspective. This means that we can and actually 
have to extend the drm_display_mode with the DSI data and compression 
information.

For example, the panel that supports all four types for the 1080p should 
export several modes:

1920x1080-command
1920x1080-command-DSC
1920x1080-video
1920x1080-video-DSC

where video/command and DSC are some kinds of flags and/or information 
in the drm_display_mode? Ideally DSC also has several sub-flags, which 
denote what kind of configuration is supported by the DSC sink (e.g. 
bpp, yuv, etc).

Another option would be to get this handled via the bus format 
negotiation, but that sounds like worse idea to me.

> This is very much like HDMI: the encoder knows what the monitor is
> capable of, will take a decision based on its capabilities and the
> monitor's and will then let the monitor know. But the monitor never
> knows what the encoder is truly capable of, nor will it enforce
> something.
> 
> Maxime
Maxime Ripard July 5, 2023, 2:24 p.m. UTC | #29 
On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote:
> > > >
> > > > Either way, I'm not really sure it's a good idea to multiply the
> > > > capabilities flags of the DSI host, and we should just stick to the
> > > > spec. If the spec says that we have to support DSC while video is
> > > > output, then that's what the panels should expect.
> > > 
> > > Except some panels supports DSC & non-DSC, Video and Command mode, and
> > > all that is runtime configurable. How do you handle that ?
> > 
> > In this case, most of the constraints are going to be on the encoder
> > still so it should be the one driving it. The panel will only care about
> > which mode has been selected, but it shouldn't be the one driving it,
> > and thus we still don't really need to expose the host capabilities.
> 
> This is an interesting perspective. This means that we can and actually have
> to extend the drm_display_mode with the DSI data and compression
> information.

I wouldn't extend drm_display_mode, but extending one of the state
structures definitely.

We already have some extra variables in drm_connector_state for HDMI,
I don't think it would be a big deal to add a few for MIPI-DSI.

We also floated the idea for a while to create bus-specific states, with
helpers to match. Maybe it would be a good occasion to start doing it?

> For example, the panel that supports all four types for the 1080p should
> export several modes:
> 
> 1920x1080-command
> 1920x1080-command-DSC
> 1920x1080-video
> 1920x1080-video-DSC
>
> where video/command and DSC are some kinds of flags and/or information in
> the drm_display_mode? Ideally DSC also has several sub-flags, which denote
> what kind of configuration is supported by the DSC sink (e.g. bpp, yuv,
> etc).

So we have two things to do, right? We need to expose what the panel can
take (ie, EDID for HDMI), and then we need to tell it what we picked
(infoframes).

We already express the former in mipi_dsi_device, so we could extend the
flags stored there.

And then, we need to tie what the DSI host chose to a given atomic state
so the panel knows what was picked and how it should set everything up.

> Another option would be to get this handled via the bus format negotiation,
> but that sounds like worse idea to me.

Yeah, I'm not really fond of the format negociation stuff either.

Maxime
Dmitry Baryshkov July 5, 2023, 3:20 p.m. UTC | #30 
On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote:
>
> On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote:
> > > > >
> > > > > Either way, I'm not really sure it's a good idea to multiply the
> > > > > capabilities flags of the DSI host, and we should just stick to the
> > > > > spec. If the spec says that we have to support DSC while video is
> > > > > output, then that's what the panels should expect.
> > > >
> > > > Except some panels supports DSC & non-DSC, Video and Command mode, and
> > > > all that is runtime configurable. How do you handle that ?
> > >
> > > In this case, most of the constraints are going to be on the encoder
> > > still so it should be the one driving it. The panel will only care about
> > > which mode has been selected, but it shouldn't be the one driving it,
> > > and thus we still don't really need to expose the host capabilities.
> >
> > This is an interesting perspective. This means that we can and actually have
> > to extend the drm_display_mode with the DSI data and compression
> > information.
>
> I wouldn't extend drm_display_mode, but extending one of the state
> structures definitely.
>
> We already have some extra variables in drm_connector_state for HDMI,
> I don't think it would be a big deal to add a few for MIPI-DSI.
>
> We also floated the idea for a while to create bus-specific states, with
> helpers to match. Maybe it would be a good occasion to start doing it?
>
> > For example, the panel that supports all four types for the 1080p should
> > export several modes:
> >
> > 1920x1080-command
> > 1920x1080-command-DSC
> > 1920x1080-video
> > 1920x1080-video-DSC
> >
> > where video/command and DSC are some kinds of flags and/or information in
> > the drm_display_mode? Ideally DSC also has several sub-flags, which denote
> > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv,
> > etc).
>
> So we have two things to do, right? We need to expose what the panel can
> take (ie, EDID for HDMI), and then we need to tell it what we picked
> (infoframes).
>
> We already express the former in mipi_dsi_device, so we could extend the
> flags stored there.
>
> And then, we need to tie what the DSI host chose to a given atomic state
> so the panel knows what was picked and how it should set everything up.

This is definitely something we need. Marijn has been stuck with the
panels that support different models ([1]).

Would you prefer a separate API for this kind of information or
abusing atomic_enable() is fine from your point of view?

My vote would be for going with existing operations, with the slight
fear of ending up with another DSI-specific hack (like
pre_enable_prev_first).

>
> > Another option would be to get this handled via the bus format negotiation,
> > but that sounds like worse idea to me.
>
> Yeah, I'm not really fond of the format negociation stuff either.


[1] https://lore.kernel.org/linux-arm-msm/20230521-drm-panels-sony-v1-8-541c341d6bee@somainline.org/
Neil Armstrong July 5, 2023, 3:58 p.m. UTC | #31 
On 05/07/2023 16:24, Maxime Ripard wrote:
> On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote:
>>>>>
>>>>> Either way, I'm not really sure it's a good idea to multiply the
>>>>> capabilities flags of the DSI host, and we should just stick to the
>>>>> spec. If the spec says that we have to support DSC while video is
>>>>> output, then that's what the panels should expect.
>>>>
>>>> Except some panels supports DSC & non-DSC, Video and Command mode, and
>>>> all that is runtime configurable. How do you handle that ?
>>>
>>> In this case, most of the constraints are going to be on the encoder
>>> still so it should be the one driving it. The panel will only care about
>>> which mode has been selected, but it shouldn't be the one driving it,
>>> and thus we still don't really need to expose the host capabilities.
>>
>> This is an interesting perspective. This means that we can and actually have
>> to extend the drm_display_mode with the DSI data and compression
>> information.
> 
> I wouldn't extend drm_display_mode, but extending one of the state
> structures definitely.
> 
> We already have some extra variables in drm_connector_state for HDMI,
> I don't think it would be a big deal to add a few for MIPI-DSI.
> 
> We also floated the idea for a while to create bus-specific states, with
> helpers to match. Maybe it would be a good occasion to start doing it?
> 
>> For example, the panel that supports all four types for the 1080p should
>> export several modes:
>>
>> 1920x1080-command
>> 1920x1080-command-DSC
>> 1920x1080-video
>> 1920x1080-video-DSC
>>
>> where video/command and DSC are some kinds of flags and/or information in
>> the drm_display_mode? Ideally DSC also has several sub-flags, which denote
>> what kind of configuration is supported by the DSC sink (e.g. bpp, yuv,
>> etc).
> 
> So we have two things to do, right? We need to expose what the panel can
> take (ie, EDID for HDMI), and then we need to tell it what we picked
> (infoframes).
> 
> We already express the former in mipi_dsi_device, so we could extend the
> flags stored there.
> 
> And then, we need to tie what the DSI host chose to a given atomic state
> so the panel knows what was picked and how it should set everything up.

Yep this looks like a good plan

Neil

> 
>> Another option would be to get this handled via the bus format negotiation,
>> but that sounds like worse idea to me.
> 
> Yeah, I'm not really fond of the format negociation stuff either.
> 
> Maxime
Maxime Ripard July 5, 2023, 4:53 p.m. UTC | #32 
On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote:
> On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote:
> >
> > On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote:
> > > > > >
> > > > > > Either way, I'm not really sure it's a good idea to multiply the
> > > > > > capabilities flags of the DSI host, and we should just stick to the
> > > > > > spec. If the spec says that we have to support DSC while video is
> > > > > > output, then that's what the panels should expect.
> > > > >
> > > > > Except some panels supports DSC & non-DSC, Video and Command mode, and
> > > > > all that is runtime configurable. How do you handle that ?
> > > >
> > > > In this case, most of the constraints are going to be on the encoder
> > > > still so it should be the one driving it. The panel will only care about
> > > > which mode has been selected, but it shouldn't be the one driving it,
> > > > and thus we still don't really need to expose the host capabilities.
> > >
> > > This is an interesting perspective. This means that we can and actually have
> > > to extend the drm_display_mode with the DSI data and compression
> > > information.
> >
> > I wouldn't extend drm_display_mode, but extending one of the state
> > structures definitely.
> >
> > We already have some extra variables in drm_connector_state for HDMI,
> > I don't think it would be a big deal to add a few for MIPI-DSI.
> >
> > We also floated the idea for a while to create bus-specific states, with
> > helpers to match. Maybe it would be a good occasion to start doing it?
> >
> > > For example, the panel that supports all four types for the 1080p should
> > > export several modes:
> > >
> > > 1920x1080-command
> > > 1920x1080-command-DSC
> > > 1920x1080-video
> > > 1920x1080-video-DSC
> > >
> > > where video/command and DSC are some kinds of flags and/or information in
> > > the drm_display_mode? Ideally DSC also has several sub-flags, which denote
> > > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv,
> > > etc).
> >
> > So we have two things to do, right? We need to expose what the panel can
> > take (ie, EDID for HDMI), and then we need to tell it what we picked
> > (infoframes).
> >
> > We already express the former in mipi_dsi_device, so we could extend the
> > flags stored there.
> >
> > And then, we need to tie what the DSI host chose to a given atomic state
> > so the panel knows what was picked and how it should set everything up.
> 
> This is definitely something we need. Marijn has been stuck with the
> panels that support different models ([1]).
> 
> Would you prefer a separate API for this kind of information or
> abusing atomic_enable() is fine from your point of view?
> 
> My vote would be for going with existing operations, with the slight
> fear of ending up with another DSI-specific hack (like
> pre_enable_prev_first).

I don't think we can get away without getting access to the atomic_state
from the panel at least.

Choosing one setup over another is likely going to depend on the mode,
and that's only available in the state.

We don't have to go the whole way though and create the sub-classes of
drm_connector_state, but I think we should at least provide it to the
panel.

What do you think of creating a new set of atomic_* callbacks for
panels, call that new set of functions from msm and start from there?

Maxime
Dmitry Baryshkov July 5, 2023, 8:09 p.m. UTC | #33 
On 05/07/2023 19:53, Maxime Ripard wrote:
> On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote:
>> On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote:
>>>
>>> On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote:
>>>>>>>
>>>>>>> Either way, I'm not really sure it's a good idea to multiply the
>>>>>>> capabilities flags of the DSI host, and we should just stick to the
>>>>>>> spec. If the spec says that we have to support DSC while video is
>>>>>>> output, then that's what the panels should expect.
>>>>>>
>>>>>> Except some panels supports DSC & non-DSC, Video and Command mode, and
>>>>>> all that is runtime configurable. How do you handle that ?
>>>>>
>>>>> In this case, most of the constraints are going to be on the encoder
>>>>> still so it should be the one driving it. The panel will only care about
>>>>> which mode has been selected, but it shouldn't be the one driving it,
>>>>> and thus we still don't really need to expose the host capabilities.
>>>>
>>>> This is an interesting perspective. This means that we can and actually have
>>>> to extend the drm_display_mode with the DSI data and compression
>>>> information.
>>>
>>> I wouldn't extend drm_display_mode, but extending one of the state
>>> structures definitely.
>>>
>>> We already have some extra variables in drm_connector_state for HDMI,
>>> I don't think it would be a big deal to add a few for MIPI-DSI.
>>>
>>> We also floated the idea for a while to create bus-specific states, with
>>> helpers to match. Maybe it would be a good occasion to start doing it?
>>>
>>>> For example, the panel that supports all four types for the 1080p should
>>>> export several modes:
>>>>
>>>> 1920x1080-command
>>>> 1920x1080-command-DSC
>>>> 1920x1080-video
>>>> 1920x1080-video-DSC
>>>>
>>>> where video/command and DSC are some kinds of flags and/or information in
>>>> the drm_display_mode? Ideally DSC also has several sub-flags, which denote
>>>> what kind of configuration is supported by the DSC sink (e.g. bpp, yuv,
>>>> etc).
>>>
>>> So we have two things to do, right? We need to expose what the panel can
>>> take (ie, EDID for HDMI), and then we need to tell it what we picked
>>> (infoframes).
>>>
>>> We already express the former in mipi_dsi_device, so we could extend the
>>> flags stored there.
>>>
>>> And then, we need to tie what the DSI host chose to a given atomic state
>>> so the panel knows what was picked and how it should set everything up.
>>
>> This is definitely something we need. Marijn has been stuck with the
>> panels that support different models ([1]).
>>
>> Would you prefer a separate API for this kind of information or
>> abusing atomic_enable() is fine from your point of view?
>>
>> My vote would be for going with existing operations, with the slight
>> fear of ending up with another DSI-specific hack (like
>> pre_enable_prev_first).
> 
> I don't think we can get away without getting access to the atomic_state
> from the panel at least.
> 
> Choosing one setup over another is likely going to depend on the mode,
> and that's only available in the state.
> 
> We don't have to go the whole way though and create the sub-classes of
> drm_connector_state, but I think we should at least provide it to the
> panel.
> 
> What do you think of creating a new set of atomic_* callbacks for
> panels, call that new set of functions from msm and start from there?

We are (somewhat) bound by the panel_bridge, but yeah, it seems possible.
Maxime Ripard July 6, 2023, 7:24 a.m. UTC | #34 
On Wed, Jul 05, 2023 at 11:09:40PM +0300, Dmitry Baryshkov wrote:
> On 05/07/2023 19:53, Maxime Ripard wrote:
> > On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote:
> > > On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote:
> > > > 
> > > > On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > 
> > > > > > > > Either way, I'm not really sure it's a good idea to multiply the
> > > > > > > > capabilities flags of the DSI host, and we should just stick to the
> > > > > > > > spec. If the spec says that we have to support DSC while video is
> > > > > > > > output, then that's what the panels should expect.
> > > > > > > 
> > > > > > > Except some panels supports DSC & non-DSC, Video and Command mode, and
> > > > > > > all that is runtime configurable. How do you handle that ?
> > > > > > 
> > > > > > In this case, most of the constraints are going to be on the encoder
> > > > > > still so it should be the one driving it. The panel will only care about
> > > > > > which mode has been selected, but it shouldn't be the one driving it,
> > > > > > and thus we still don't really need to expose the host capabilities.
> > > > > 
> > > > > This is an interesting perspective. This means that we can and actually have
> > > > > to extend the drm_display_mode with the DSI data and compression
> > > > > information.
> > > > 
> > > > I wouldn't extend drm_display_mode, but extending one of the state
> > > > structures definitely.
> > > > 
> > > > We already have some extra variables in drm_connector_state for HDMI,
> > > > I don't think it would be a big deal to add a few for MIPI-DSI.
> > > > 
> > > > We also floated the idea for a while to create bus-specific states, with
> > > > helpers to match. Maybe it would be a good occasion to start doing it?
> > > > 
> > > > > For example, the panel that supports all four types for the 1080p should
> > > > > export several modes:
> > > > > 
> > > > > 1920x1080-command
> > > > > 1920x1080-command-DSC
> > > > > 1920x1080-video
> > > > > 1920x1080-video-DSC
> > > > > 
> > > > > where video/command and DSC are some kinds of flags and/or information in
> > > > > the drm_display_mode? Ideally DSC also has several sub-flags, which denote
> > > > > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv,
> > > > > etc).
> > > > 
> > > > So we have two things to do, right? We need to expose what the panel can
> > > > take (ie, EDID for HDMI), and then we need to tell it what we picked
> > > > (infoframes).
> > > > 
> > > > We already express the former in mipi_dsi_device, so we could extend the
> > > > flags stored there.
> > > > 
> > > > And then, we need to tie what the DSI host chose to a given atomic state
> > > > so the panel knows what was picked and how it should set everything up.
> > > 
> > > This is definitely something we need. Marijn has been stuck with the
> > > panels that support different models ([1]).
> > > 
> > > Would you prefer a separate API for this kind of information or
> > > abusing atomic_enable() is fine from your point of view?
> > > 
> > > My vote would be for going with existing operations, with the slight
> > > fear of ending up with another DSI-specific hack (like
> > > pre_enable_prev_first).
> > 
> > I don't think we can get away without getting access to the atomic_state
> > from the panel at least.
> > 
> > Choosing one setup over another is likely going to depend on the mode,
> > and that's only available in the state.
> > 
> > We don't have to go the whole way though and create the sub-classes of
> > drm_connector_state, but I think we should at least provide it to the
> > panel.
> > 
> > What do you think of creating a new set of atomic_* callbacks for
> > panels, call that new set of functions from msm and start from there?
> 
> We are (somewhat) bound by the panel_bridge, but yeah, it seems possible.

Bridges have access to the atomic state already, so it's another place
to plumb this through but I guess it would still be doable?

Maxime
Neil Armstrong July 6, 2023, 7:33 a.m. UTC | #35 
On 06/07/2023 09:24, Maxime Ripard wrote:
> On Wed, Jul 05, 2023 at 11:09:40PM +0300, Dmitry Baryshkov wrote:
>> On 05/07/2023 19:53, Maxime Ripard wrote:
>>> On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote:
>>>> On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote:
>>>>>
>>>>> On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote:
>>>>>>>>>
>>>>>>>>> Either way, I'm not really sure it's a good idea to multiply the
>>>>>>>>> capabilities flags of the DSI host, and we should just stick to the
>>>>>>>>> spec. If the spec says that we have to support DSC while video is
>>>>>>>>> output, then that's what the panels should expect.
>>>>>>>>
>>>>>>>> Except some panels supports DSC & non-DSC, Video and Command mode, and
>>>>>>>> all that is runtime configurable. How do you handle that ?
>>>>>>>
>>>>>>> In this case, most of the constraints are going to be on the encoder
>>>>>>> still so it should be the one driving it. The panel will only care about
>>>>>>> which mode has been selected, but it shouldn't be the one driving it,
>>>>>>> and thus we still don't really need to expose the host capabilities.
>>>>>>
>>>>>> This is an interesting perspective. This means that we can and actually have
>>>>>> to extend the drm_display_mode with the DSI data and compression
>>>>>> information.
>>>>>
>>>>> I wouldn't extend drm_display_mode, but extending one of the state
>>>>> structures definitely.
>>>>>
>>>>> We already have some extra variables in drm_connector_state for HDMI,
>>>>> I don't think it would be a big deal to add a few for MIPI-DSI.
>>>>>
>>>>> We also floated the idea for a while to create bus-specific states, with
>>>>> helpers to match. Maybe it would be a good occasion to start doing it?
>>>>>
>>>>>> For example, the panel that supports all four types for the 1080p should
>>>>>> export several modes:
>>>>>>
>>>>>> 1920x1080-command
>>>>>> 1920x1080-command-DSC
>>>>>> 1920x1080-video
>>>>>> 1920x1080-video-DSC
>>>>>>
>>>>>> where video/command and DSC are some kinds of flags and/or information in
>>>>>> the drm_display_mode? Ideally DSC also has several sub-flags, which denote
>>>>>> what kind of configuration is supported by the DSC sink (e.g. bpp, yuv,
>>>>>> etc).
>>>>>
>>>>> So we have two things to do, right? We need to expose what the panel can
>>>>> take (ie, EDID for HDMI), and then we need to tell it what we picked
>>>>> (infoframes).
>>>>>
>>>>> We already express the former in mipi_dsi_device, so we could extend the
>>>>> flags stored there.
>>>>>
>>>>> And then, we need to tie what the DSI host chose to a given atomic state
>>>>> so the panel knows what was picked and how it should set everything up.
>>>>
>>>> This is definitely something we need. Marijn has been stuck with the
>>>> panels that support different models ([1]).
>>>>
>>>> Would you prefer a separate API for this kind of information or
>>>> abusing atomic_enable() is fine from your point of view?
>>>>
>>>> My vote would be for going with existing operations, with the slight
>>>> fear of ending up with another DSI-specific hack (like
>>>> pre_enable_prev_first).
>>>
>>> I don't think we can get away without getting access to the atomic_state
>>> from the panel at least.
>>>
>>> Choosing one setup over another is likely going to depend on the mode,
>>> and that's only available in the state.
>>>
>>> We don't have to go the whole way though and create the sub-classes of
>>> drm_connector_state, but I think we should at least provide it to the
>>> panel.
>>>
>>> What do you think of creating a new set of atomic_* callbacks for
>>> panels, call that new set of functions from msm and start from there?
>>
>> We are (somewhat) bound by the panel_bridge, but yeah, it seems possible.
> 
> Bridges have access to the atomic state already, so it's another place
> to plumb this through but I guess it would still be doable?

It's definitely doable, but I fear we won't be able to test most of the
panel drivers, should we introduce a new atomic set of panel callbacks ?

Or shall be simply move the "new" panel driver supporting atomic to bridge
and only use panel_bridge for basic panels ?

Neil

> 
> Maxime
Maxime Ripard July 6, 2023, 7:59 a.m. UTC | #36 
On Thu, Jul 06, 2023 at 09:33:15AM +0200, Neil Armstrong wrote:
> On 06/07/2023 09:24, Maxime Ripard wrote:
> > On Wed, Jul 05, 2023 at 11:09:40PM +0300, Dmitry Baryshkov wrote:
> > > On 05/07/2023 19:53, Maxime Ripard wrote:
> > > > On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote:
> > > > > On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote:
> > > > > > 
> > > > > > On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote:
> > > > > > > > > > 
> > > > > > > > > > Either way, I'm not really sure it's a good idea to multiply the
> > > > > > > > > > capabilities flags of the DSI host, and we should just stick to the
> > > > > > > > > > spec. If the spec says that we have to support DSC while video is
> > > > > > > > > > output, then that's what the panels should expect.
> > > > > > > > > 
> > > > > > > > > Except some panels supports DSC & non-DSC, Video and Command mode, and
> > > > > > > > > all that is runtime configurable. How do you handle that ?
> > > > > > > > 
> > > > > > > > In this case, most of the constraints are going to be on the encoder
> > > > > > > > still so it should be the one driving it. The panel will only care about
> > > > > > > > which mode has been selected, but it shouldn't be the one driving it,
> > > > > > > > and thus we still don't really need to expose the host capabilities.
> > > > > > > 
> > > > > > > This is an interesting perspective. This means that we can and actually have
> > > > > > > to extend the drm_display_mode with the DSI data and compression
> > > > > > > information.
> > > > > > 
> > > > > > I wouldn't extend drm_display_mode, but extending one of the state
> > > > > > structures definitely.
> > > > > > 
> > > > > > We already have some extra variables in drm_connector_state for HDMI,
> > > > > > I don't think it would be a big deal to add a few for MIPI-DSI.
> > > > > > 
> > > > > > We also floated the idea for a while to create bus-specific states, with
> > > > > > helpers to match. Maybe it would be a good occasion to start doing it?
> > > > > > 
> > > > > > > For example, the panel that supports all four types for the 1080p should
> > > > > > > export several modes:
> > > > > > > 
> > > > > > > 1920x1080-command
> > > > > > > 1920x1080-command-DSC
> > > > > > > 1920x1080-video
> > > > > > > 1920x1080-video-DSC
> > > > > > > 
> > > > > > > where video/command and DSC are some kinds of flags and/or information in
> > > > > > > the drm_display_mode? Ideally DSC also has several sub-flags, which denote
> > > > > > > what kind of configuration is supported by the DSC sink (e.g. bpp, yuv,
> > > > > > > etc).
> > > > > > 
> > > > > > So we have two things to do, right? We need to expose what the panel can
> > > > > > take (ie, EDID for HDMI), and then we need to tell it what we picked
> > > > > > (infoframes).
> > > > > > 
> > > > > > We already express the former in mipi_dsi_device, so we could extend the
> > > > > > flags stored there.
> > > > > > 
> > > > > > And then, we need to tie what the DSI host chose to a given atomic state
> > > > > > so the panel knows what was picked and how it should set everything up.
> > > > > 
> > > > > This is definitely something we need. Marijn has been stuck with the
> > > > > panels that support different models ([1]).
> > > > > 
> > > > > Would you prefer a separate API for this kind of information or
> > > > > abusing atomic_enable() is fine from your point of view?
> > > > > 
> > > > > My vote would be for going with existing operations, with the slight
> > > > > fear of ending up with another DSI-specific hack (like
> > > > > pre_enable_prev_first).
> > > > 
> > > > I don't think we can get away without getting access to the atomic_state
> > > > from the panel at least.
> > > > 
> > > > Choosing one setup over another is likely going to depend on the mode,
> > > > and that's only available in the state.
> > > > 
> > > > We don't have to go the whole way though and create the sub-classes of
> > > > drm_connector_state, but I think we should at least provide it to the
> > > > panel.
> > > > 
> > > > What do you think of creating a new set of atomic_* callbacks for
> > > > panels, call that new set of functions from msm and start from there?
> > > 
> > > We are (somewhat) bound by the panel_bridge, but yeah, it seems possible.
> > 
> > Bridges have access to the atomic state already, so it's another place
> > to plumb this through but I guess it would still be doable?
> 
> It's definitely doable, but I fear we won't be able to test most of the
> panel drivers, should we introduce a new atomic set of panel callbacks ?

That was my original intent yeah :)

Creating an atomic_enable/disable/ etc. and then switch
drm_panel_enable() to take the state (and fixing up all the callers), or
create a drm_panel_enable_atomic() function.

The latter is probably simpler, something like:

int drm_panel_enable_atomic(struct drm_panel *panel,
    			    struct drm_atomic_state *state)
{
	struct drm_panel_funcs *funcs = panel->funcs;

	if (funcs->atomic_enable)
		return funcs->atomic_enable(panel, state);

	return funcs->enable(panel);
}

And we should probably mention that it supersedes/deprecates
drm_panel_enable.

We've switched most of the other atomic hooks to take the full
drm_atomic_state so I'd prefer to use it. However, for it to be somewhat
useful we'd need to have access to the connector assigned to that panel.

drm_panel doesn't store the drm_connector it's connected to at all, and
of_drm_find_panel() doesn't take it as an argument so we can't fill it
when we retrieve it either.

So I guess we can go for:

 - Create a new set of atomic hooks

 - Create a new set of functions to call those hooks, that we would
   document as deprecating the former functions. Those functions would
   take a pointer to the drm_connector_state of the drm_connector it's
   connected to.

 - We add a TODO item to add a pointer to the connector in drm_panel

 - We add a TODO item that depend on the first one to switch the new
   functions and hooks to drm_atomic_state

 - We add a TODO item to convert callers of drm_panel_enable et al. to
   our new functions.

It should work in all setups, paves a nice way forward and documents the
trade-offs we had to take and eventually address. And without creating a
dependency on 30+ patches series.

Does it sound like a plan?

> Or shall be simply move the "new" panel driver supporting atomic to bridge
> and only use panel_bridge for basic panels ?

I don't think we can expect panel_bridge to be used all the time any
time soon, so I'd rather avoid to rely on it.

Maxime
Neil Armstrong July 6, 2023, 8:03 a.m. UTC | #37 
On 06/07/2023 09:59, Maxime Ripard wrote:
> On Thu, Jul 06, 2023 at 09:33:15AM +0200, Neil Armstrong wrote:
>> On 06/07/2023 09:24, Maxime Ripard wrote:
>>> On Wed, Jul 05, 2023 at 11:09:40PM +0300, Dmitry Baryshkov wrote:
>>>> On 05/07/2023 19:53, Maxime Ripard wrote:
>>>>> On Wed, Jul 05, 2023 at 06:20:13PM +0300, Dmitry Baryshkov wrote:
>>>>>> On Wed, 5 Jul 2023 at 17:24, Maxime Ripard <mripard@kernel.org> wrote:
>>>>>>>
>>>>>>> On Wed, Jul 05, 2023 at 04:37:57PM +0300, Dmitry Baryshkov wrote:
>>>>>>>>>>>
>>>>>>>>>>> Either way, I'm not really sure it's a good idea to multiply the
>>>>>>>>>>> capabilities flags of the DSI host, and we should just stick to the
>>>>>>>>>>> spec. If the spec says that we have to support DSC while video is
>>>>>>>>>>> output, then that's what the panels should expect.
>>>>>>>>>>
>>>>>>>>>> Except some panels supports DSC & non-DSC, Video and Command mode, and
>>>>>>>>>> all that is runtime configurable. How do you handle that ?
>>>>>>>>>
>>>>>>>>> In this case, most of the constraints are going to be on the encoder
>>>>>>>>> still so it should be the one driving it. The panel will only care about
>>>>>>>>> which mode has been selected, but it shouldn't be the one driving it,
>>>>>>>>> and thus we still don't really need to expose the host capabilities.
>>>>>>>>
>>>>>>>> This is an interesting perspective. This means that we can and actually have
>>>>>>>> to extend the drm_display_mode with the DSI data and compression
>>>>>>>> information.
>>>>>>>
>>>>>>> I wouldn't extend drm_display_mode, but extending one of the state
>>>>>>> structures definitely.
>>>>>>>
>>>>>>> We already have some extra variables in drm_connector_state for HDMI,
>>>>>>> I don't think it would be a big deal to add a few for MIPI-DSI.
>>>>>>>
>>>>>>> We also floated the idea for a while to create bus-specific states, with
>>>>>>> helpers to match. Maybe it would be a good occasion to start doing it?
>>>>>>>
>>>>>>>> For example, the panel that supports all four types for the 1080p should
>>>>>>>> export several modes:
>>>>>>>>
>>>>>>>> 1920x1080-command
>>>>>>>> 1920x1080-command-DSC
>>>>>>>> 1920x1080-video
>>>>>>>> 1920x1080-video-DSC
>>>>>>>>
>>>>>>>> where video/command and DSC are some kinds of flags and/or information in
>>>>>>>> the drm_display_mode? Ideally DSC also has several sub-flags, which denote
>>>>>>>> what kind of configuration is supported by the DSC sink (e.g. bpp, yuv,
>>>>>>>> etc).
>>>>>>>
>>>>>>> So we have two things to do, right? We need to expose what the panel can
>>>>>>> take (ie, EDID for HDMI), and then we need to tell it what we picked
>>>>>>> (infoframes).
>>>>>>>
>>>>>>> We already express the former in mipi_dsi_device, so we could extend the
>>>>>>> flags stored there.
>>>>>>>
>>>>>>> And then, we need to tie what the DSI host chose to a given atomic state
>>>>>>> so the panel knows what was picked and how it should set everything up.
>>>>>>
>>>>>> This is definitely something we need. Marijn has been stuck with the
>>>>>> panels that support different models ([1]).
>>>>>>
>>>>>> Would you prefer a separate API for this kind of information or
>>>>>> abusing atomic_enable() is fine from your point of view?
>>>>>>
>>>>>> My vote would be for going with existing operations, with the slight
>>>>>> fear of ending up with another DSI-specific hack (like
>>>>>> pre_enable_prev_first).
>>>>>
>>>>> I don't think we can get away without getting access to the atomic_state
>>>>> from the panel at least.
>>>>>
>>>>> Choosing one setup over another is likely going to depend on the mode,
>>>>> and that's only available in the state.
>>>>>
>>>>> We don't have to go the whole way though and create the sub-classes of
>>>>> drm_connector_state, but I think we should at least provide it to the
>>>>> panel.
>>>>>
>>>>> What do you think of creating a new set of atomic_* callbacks for
>>>>> panels, call that new set of functions from msm and start from there?
>>>>
>>>> We are (somewhat) bound by the panel_bridge, but yeah, it seems possible.
>>>
>>> Bridges have access to the atomic state already, so it's another place
>>> to plumb this through but I guess it would still be doable?
>>
>> It's definitely doable, but I fear we won't be able to test most of the
>> panel drivers, should we introduce a new atomic set of panel callbacks ?
> 
> That was my original intent yeah :)
> 
> Creating an atomic_enable/disable/ etc. and then switch
> drm_panel_enable() to take the state (and fixing up all the callers), or
> create a drm_panel_enable_atomic() function.
> 
> The latter is probably simpler, something like:
> 
> int drm_panel_enable_atomic(struct drm_panel *panel,
>      			    struct drm_atomic_state *state)
> {
> 	struct drm_panel_funcs *funcs = panel->funcs;
> 
> 	if (funcs->atomic_enable)
> 		return funcs->atomic_enable(panel, state);
> 
> 	return funcs->enable(panel);
> }
> 
> And we should probably mention that it supersedes/deprecates
> drm_panel_enable.
> 
> We've switched most of the other atomic hooks to take the full
> drm_atomic_state so I'd prefer to use it. However, for it to be somewhat
> useful we'd need to have access to the connector assigned to that panel.
> 
> drm_panel doesn't store the drm_connector it's connected to at all, and
> of_drm_find_panel() doesn't take it as an argument so we can't fill it
> when we retrieve it either.
> 
> So I guess we can go for:
> 
>   - Create a new set of atomic hooks
> 
>   - Create a new set of functions to call those hooks, that we would
>     document as deprecating the former functions. Those functions would
>     take a pointer to the drm_connector_state of the drm_connector it's
>     connected to.
> 
>   - We add a TODO item to add a pointer to the connector in drm_panel
> 
>   - We add a TODO item that depend on the first one to switch the new
>     functions and hooks to drm_atomic_state
> 
>   - We add a TODO item to convert callers of drm_panel_enable et al. to
>     our new functions.
> 
> It should work in all setups, paves a nice way forward and documents the
> trade-offs we had to take and eventually address. And without creating a
> dependency on 30+ patches series.
> 
> Does it sound like a plan?

Yep that looks a fine plan to start of

> 
>> Or shall be simply move the "new" panel driver supporting atomic to bridge
>> and only use panel_bridge for basic panels ?
> 
> I don't think we can expect panel_bridge to be used all the time any
> time soon, so I'd rather avoid to rely on it.

Ack

> 
> Maxime
diff mbox series

Patch

diff --git a/drivers/gpu/drm/panel/Kconfig b/drivers/gpu/drm/panel/Kconfig
index 67ef898d133f2..18bd116e78a71 100644
--- a/drivers/gpu/drm/panel/Kconfig
+++ b/drivers/gpu/drm/panel/Kconfig
@@ -706,6 +706,17 @@  config DRM_PANEL_SONY_ACX565AKM
 	  Say Y here if you want to enable support for the Sony ACX565AKM
 	  800x600 3.5" panel (found on the Nokia N900).
 
+config DRM_PANEL_SONY_AKATSUKI_LGD
+	tristate "Sony Xperia XZ3 LGD panel"
+	depends on GPIOLIB && OF
+	depends on DRM_MIPI_DSI
+	depends on BACKLIGHT_CLASS_DEVICE
+	help
+	  Say Y here if you want to enable support for the Sony Xperia XZ3
+	  1440x2880@60 6.0" OLED DSI cmd mode panel produced by LG Display.
+
+	  This panel uses Display Stream Compression 1.1.
+
 config DRM_PANEL_SONY_TD4353_JDI
 	tristate "Sony TD4353 JDI panel"
 	depends on GPIOLIB && OF
diff --git a/drivers/gpu/drm/panel/Makefile b/drivers/gpu/drm/panel/Makefile
index ff169781e82d7..85133f73558f3 100644
--- a/drivers/gpu/drm/panel/Makefile
+++ b/drivers/gpu/drm/panel/Makefile
@@ -71,6 +71,7 @@  obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7701) += panel-sitronix-st7701.o
 obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7703) += panel-sitronix-st7703.o
 obj-$(CONFIG_DRM_PANEL_SITRONIX_ST7789V) += panel-sitronix-st7789v.o
 obj-$(CONFIG_DRM_PANEL_SONY_ACX565AKM) += panel-sony-acx565akm.o
+obj-$(CONFIG_DRM_PANEL_SONY_AKATSUKI_LGD) += panel-sony-akatsuki-lgd.o
 obj-$(CONFIG_DRM_PANEL_SONY_TD4353_JDI) += panel-sony-td4353-jdi.o
 obj-$(CONFIG_DRM_PANEL_SONY_TULIP_TRULY_NT35521) += panel-sony-tulip-truly-nt35521.o
 obj-$(CONFIG_DRM_PANEL_TDO_TL070WSH30) += panel-tdo-tl070wsh30.o
diff --git a/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
new file mode 100644
index 0000000000000..f55788f963dab
--- /dev/null
+++ b/drivers/gpu/drm/panel/panel-sony-akatsuki-lgd.c
@@ -0,0 +1,362 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (c) 2023 Marijn Suijten <marijn.suijten@somainline.org>
+ *
+ * Based on Sony Downstream's "Atmel LGD ID5" Akatsuki panel dtsi.
+ */
+
+#include <linux/backlight.h>
+#include <linux/delay.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/regulator/consumer.h>
+
+#include <video/mipi_display.h>
+
+#include <drm/drm_mipi_dsi.h>
+#include <drm/drm_modes.h>
+#include <drm/drm_panel.h>
+#include <drm/drm_probe_helper.h>
+#include <drm/display/drm_dsc.h>
+#include <drm/display/drm_dsc_helper.h>
+
+struct sony_akatsuki_lgd {
+	struct drm_panel panel;
+	struct mipi_dsi_device *dsi;
+	struct regulator *vddio;
+	struct gpio_desc *reset_gpio;
+	bool prepared;
+};
+
+static inline struct sony_akatsuki_lgd *to_sony_akatsuki_lgd(struct drm_panel *panel)
+{
+	return container_of(panel, struct sony_akatsuki_lgd, panel);
+}
+
+static int sony_akatsuki_lgd_on(struct sony_akatsuki_lgd *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	int ret;
+
+	dsi->mode_flags |= MIPI_DSI_MODE_LPM;
+
+	mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_seq(dsi, 0x02, 0x01);
+	mipi_dsi_dcs_write_seq(dsi, 0x59, 0x01);
+	/* Enable backlight control */
+	mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, BIT(5));
+	mipi_dsi_dcs_write_seq(dsi, 0x57, 0x20, 0x80, 0xde, 0x60, 0x00);
+
+	ret = mipi_dsi_dcs_set_column_address(dsi, 0, 1440 - 1);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set column address: %d\n", ret);
+		return ret;
+	}
+
+	ret = mipi_dsi_dcs_set_page_address(dsi, 0, 2880 - 1);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set page address: %d\n", ret);
+		return ret;
+	}
+
+	mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_POWER_SAVE, 0x00);
+
+	ret = mipi_dsi_dcs_set_tear_on(dsi, MIPI_DSI_DCS_TEAR_MODE_VBLANK);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set tear on: %d\n", ret);
+		return ret;
+	}
+
+	mipi_dsi_dcs_write_seq(dsi, 0x7f, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_seq(dsi, 0xf0, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_seq(dsi, 0xf1, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_seq(dsi, 0xf2, 0x5a, 0x5a);
+	mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x03);
+	mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x04);
+	mipi_dsi_dcs_write_seq(dsi, 0xb0, 0x05);
+	mipi_dsi_dcs_write_seq(dsi, 0xf6, 0x01, 0x7f, 0x00);
+
+	ret = mipi_dsi_dcs_exit_sleep_mode(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to exit sleep mode: %d\n", ret);
+		return ret;
+	}
+	msleep(120);
+
+	mipi_dsi_dcs_write_seq(dsi, 0xe3, 0xac, 0x19, 0x34, 0x14, 0x7d);
+
+	ret = mipi_dsi_dcs_set_display_on(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to turn display on: %d\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+
+static int sony_akatsuki_lgd_off(struct sony_akatsuki_lgd *ctx)
+{
+	struct mipi_dsi_device *dsi = ctx->dsi;
+	struct device *dev = &dsi->dev;
+	int ret;
+
+	dsi->mode_flags &= ~MIPI_DSI_MODE_LPM;
+
+	ret = mipi_dsi_dcs_set_display_off(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to turn display off: %d\n", ret);
+		return ret;
+	}
+	msleep(20);
+
+	ret = mipi_dsi_dcs_set_tear_off(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to set tear off: %d\n", ret);
+		return ret;
+	}
+
+	ret = mipi_dsi_dcs_enter_sleep_mode(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enter sleep mode: %d\n", ret);
+		return ret;
+	}
+	msleep(100);
+
+	return 0;
+}
+
+static int sony_akatsuki_lgd_prepare(struct drm_panel *panel)
+{
+	struct sony_akatsuki_lgd *ctx = to_sony_akatsuki_lgd(panel);
+	struct drm_dsc_picture_parameter_set pps;
+	struct device *dev = &ctx->dsi->dev;
+	int ret;
+
+	if (ctx->prepared)
+		return 0;
+
+	ret = regulator_enable(ctx->vddio);
+	if (ret < 0) {
+		dev_err(dev, "Failed to enable vddio regulator: %d\n", ret);
+		return ret;
+	}
+
+	msleep(100);
+
+	gpiod_set_value_cansleep(ctx->reset_gpio, 1);
+	usleep_range(5000, 5100);
+
+	ret = sony_akatsuki_lgd_on(ctx);
+	if (ret < 0) {
+		dev_err(dev, "Failed to power on panel: %d\n", ret);
+		goto fail;
+	}
+
+	if (ctx->dsi->dsc) {
+		drm_dsc_pps_payload_pack(&pps, ctx->dsi->dsc);
+
+		ret = mipi_dsi_picture_parameter_set(ctx->dsi, &pps);
+		if (ret < 0) {
+			dev_err(panel->dev, "failed to transmit PPS: %d\n", ret);
+			goto fail;
+		}
+		ret = mipi_dsi_compression_mode(ctx->dsi, true);
+		if (ret < 0) {
+			dev_err(dev, "failed to enable compression mode: %d\n", ret);
+			goto fail;
+		}
+
+		msleep(28);
+	}
+
+	ctx->prepared = true;
+	return 0;
+
+fail:
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+	regulator_disable(ctx->vddio);
+	return ret;
+}
+
+static int sony_akatsuki_lgd_unprepare(struct drm_panel *panel)
+{
+	struct sony_akatsuki_lgd *ctx = to_sony_akatsuki_lgd(panel);
+	struct device *dev = &ctx->dsi->dev;
+	int ret;
+
+	if (!ctx->prepared)
+		return 0;
+
+	ret = sony_akatsuki_lgd_off(ctx);
+	if (ret < 0)
+		dev_err(dev, "Failed to power off panel: %d\n", ret);
+
+	gpiod_set_value_cansleep(ctx->reset_gpio, 0);
+	regulator_disable(ctx->vddio);
+
+	usleep_range(5000, 5100);
+
+	ctx->prepared = false;
+	return 0;
+}
+
+static const struct drm_display_mode sony_akatsuki_lgd_mode = {
+	.clock = (1440 + 312 + 8 + 8) * (2880 + 48 + 8 + 8) * 60 / 1000,
+	.hdisplay = 1440,
+	.hsync_start = 1440 + 312,
+	.hsync_end = 1440 + 312 + 8,
+	.htotal = 1440 + 312 + 8 + 8,
+	.vdisplay = 2880,
+	.vsync_start = 2880 + 48,
+	.vsync_end = 2880 + 48 + 8,
+	.vtotal = 2880 + 48 + 8 + 8,
+	.width_mm = 68,
+	.height_mm = 136,
+};
+
+static int sony_akatsuki_lgd_get_modes(struct drm_panel *panel,
+				   struct drm_connector *connector)
+{
+	return drm_connector_helper_get_modes_fixed(connector, &sony_akatsuki_lgd_mode);
+}
+
+static const struct drm_panel_funcs sony_akatsuki_lgd_panel_funcs = {
+	.prepare = sony_akatsuki_lgd_prepare,
+	.unprepare = sony_akatsuki_lgd_unprepare,
+	.get_modes = sony_akatsuki_lgd_get_modes,
+};
+
+static int sony_akatsuki_lgd_bl_update_status(struct backlight_device *bl)
+{
+	struct mipi_dsi_device *dsi = bl_get_data(bl);
+	u16 brightness = backlight_get_brightness(bl);
+
+	return mipi_dsi_dcs_set_display_brightness_large(dsi, brightness);
+}
+
+static int sony_akatsuki_lgd_bl_get_brightness(struct backlight_device *bl)
+{
+	struct mipi_dsi_device *dsi = bl_get_data(bl);
+	u16 brightness;
+	int ret;
+
+	ret = mipi_dsi_dcs_get_display_brightness_large(dsi, &brightness);
+	if (ret < 0)
+		return ret;
+
+	return brightness & 0x3ff;
+}
+
+static const struct backlight_ops sony_akatsuki_lgd_bl_ops = {
+	.update_status = sony_akatsuki_lgd_bl_update_status,
+	.get_brightness = sony_akatsuki_lgd_bl_get_brightness,
+};
+
+static int sony_akatsuki_lgd_probe(struct mipi_dsi_device *dsi)
+{
+	const struct backlight_properties props = {
+		.type = BACKLIGHT_RAW,
+		.brightness = 100,
+		.max_brightness = 1023,
+	};
+	struct device *dev = &dsi->dev;
+	struct sony_akatsuki_lgd *ctx;
+	struct drm_dsc_config *dsc;
+	int ret;
+
+	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx->vddio = devm_regulator_get(dev, "vddio");
+	if (IS_ERR(ctx->vddio))
+		return dev_err_probe(dev, PTR_ERR(ctx->vddio),
+				     "Failed to get vddio\n");
+
+	ctx->reset_gpio = devm_gpiod_get(dev, "reset", GPIOD_ASIS);
+	if (IS_ERR(ctx->reset_gpio))
+		return dev_err_probe(dev, PTR_ERR(ctx->reset_gpio),
+				     "Failed to get reset-gpios\n");
+
+	ctx->dsi = dsi;
+	mipi_dsi_set_drvdata(dsi, ctx);
+
+	dsi->lanes = 4;
+	dsi->format = MIPI_DSI_FMT_RGB888;
+	dsi->mode_flags = MIPI_DSI_CLOCK_NON_CONTINUOUS;
+
+	drm_panel_init(&ctx->panel, dev, &sony_akatsuki_lgd_panel_funcs,
+		       DRM_MODE_CONNECTOR_DSI);
+
+	ctx->panel.backlight = devm_backlight_device_register(dev, dev_name(dev), dev, dsi,
+					      &sony_akatsuki_lgd_bl_ops, &props);
+	if (IS_ERR(ctx->panel.backlight))
+		return dev_err_probe(dev, PTR_ERR(ctx->panel.backlight),
+				     "Failed to create backlight\n");
+
+	drm_panel_add(&ctx->panel);
+
+	/* This panel only supports DSC; unconditionally enable it */
+	dsi->dsc = dsc = devm_kzalloc(&dsi->dev, sizeof(*dsc), GFP_KERNEL);
+	if (!dsc)
+		return -ENOMEM;
+
+	dsc->dsc_version_major = 1;
+	dsc->dsc_version_minor = 1;
+
+	dsc->slice_height = 32;
+	dsc->slice_count = 2;
+	// TODO: Get hdisplay from the mode
+	WARN_ON(1440 % dsc->slice_count);
+	dsc->slice_width = 1440 / dsc->slice_count;
+	dsc->bits_per_component = 8;
+	dsc->bits_per_pixel = 8 << 4; /* 4 fractional bits */
+	dsc->block_pred_enable = true;
+
+	ret = mipi_dsi_attach(dsi);
+	if (ret < 0) {
+		dev_err(dev, "Failed to attach to DSI host: %d\n", ret);
+		drm_panel_remove(&ctx->panel);
+		return ret;
+	}
+
+	return 0;
+}
+
+static void sony_akatsuki_lgd_remove(struct mipi_dsi_device *dsi)
+{
+	struct sony_akatsuki_lgd *ctx = mipi_dsi_get_drvdata(dsi);
+	int ret;
+
+	ret = mipi_dsi_detach(dsi);
+	if (ret < 0)
+		dev_err(&dsi->dev, "Failed to detach from DSI host: %d\n", ret);
+
+	drm_panel_remove(&ctx->panel);
+}
+
+static const struct of_device_id sony_akatsuki_lgd_of_match[] = {
+	{ .compatible = "sony,akatsuki-lgd" },
+	{ /* sentinel */ }
+};
+MODULE_DEVICE_TABLE(of, sony_akatsuki_lgd_of_match);
+
+static struct mipi_dsi_driver sony_akatsuki_lgd_driver = {
+	.probe = sony_akatsuki_lgd_probe,
+	.remove = sony_akatsuki_lgd_remove,
+	.driver = {
+		.name = "panel-sony-akatsuki-lgd",
+		.of_match_table = sony_akatsuki_lgd_of_match,
+	},
+};
+module_mipi_dsi_driver(sony_akatsuki_lgd_driver);
+
+MODULE_AUTHOR("Marijn Suijten <marijn.suijten@somainline.org>");
+MODULE_DESCRIPTION("DRM panel driver for an unnamed LGD OLED panel found in the Sony Xperia XZ3");
+MODULE_LICENSE("GPL");