| Message ID | 20230728012623.22991-1-quic_parellan@quicinc.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] drm/panel: Enable DSC and CMD mode for Visionox VTDR6130 panel | expand |
On Fri, 28 Jul 2023 at 04:26, Paloma Arellano <quic_parellan@quicinc.com> wrote: > > Enable display compression (DSC v1.2) and CMD mode for 1080x2400 Visionox > VTDR6130 AMOLED DSI panel. In addition, this patch will set the default > to command mode with DSC enabled. > > Note: This patch has only been validated DSC over command mode as DSC over > video mode has never been validated for the MSM driver before. > > Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] > > Changes since v1: > - Changed from email address > > [1] https://patchwork.freedesktop.org/series/121337/ > > Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> > Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > --- > .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 ++++++++++++++++++- > 1 file changed, 73 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > index e1363e128e7e..5658d39a3a6b 100644 > --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > @@ -9,6 +9,7 @@ > #include <linux/of.h> > > #include <drm/display/drm_dsc.h> > +#include <drm/display/drm_dsc_helper.h> > #include <drm/drm_mipi_dsi.h> > #include <drm/drm_modes.h> > #include <drm/drm_panel.h> > @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { > struct mipi_dsi_device *dsi; > struct gpio_desc *reset_gpio; > struct regulator_bulk_data supplies[3]; > - bool prepared; > + bool prepared, enabled; > + bool video_mode; > }; > > static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct drm_panel *panel) > @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct visionox_vtdr6130 *ctx) > if (ret) > return ret; > > + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); > mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20); > mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00); > mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); > mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); > mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); > - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); > + > + if (ctx->video_mode) > + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); > + else > + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); > + > mipi_dsi_dcs_write_seq(dsi, 0x70, > 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, 0x09, 0x60, 0x04, > 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, 0x1c, 0x02, 0x00, > @@ -214,6 +222,42 @@ static const struct drm_display_mode visionox_vtdr6130_mode = { > .height_mm = 157, > }; > > +static int visionox_vtdr6130_enable(struct drm_panel *panel) > +{ > + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); > + struct mipi_dsi_device *dsi = ctx->dsi; > + struct drm_dsc_picture_parameter_set pps; > + int ret; > + > + if (ctx->enabled) > + return 0; > + > + if (!dsi->dsc) { > + dev_err(&dsi->dev, "DSC not attached to DSI\n"); > + return -ENODEV; > + } The error message is misleading. Also, if you don't want to enable DSC for the video mode, this will break. > + > + drm_dsc_pps_payload_pack(&pps, dsi->dsc); > + ret = mipi_dsi_picture_parameter_set(dsi, &pps); > + if (ret) { > + dev_err(&dsi->dev, "Failed to set PPS\n"); > + return ret; > + } > + > + ctx->enabled = true; Do we need this refcount just for PPS upload? What will happen if PPS is uploaded several times? > + > + return 0; > +} > + > +static int visionox_vtdr6130_disable(struct drm_panel *panel) > +{ > + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); > + > + ctx->enabled = false; > + > + return 0; > +} > + > static int visionox_vtdr6130_get_modes(struct drm_panel *panel, > struct drm_connector *connector) > { > @@ -237,6 +281,8 @@ static const struct drm_panel_funcs visionox_vtdr6130_panel_funcs = { > .prepare = visionox_vtdr6130_prepare, > .unprepare = visionox_vtdr6130_unprepare, > .get_modes = visionox_vtdr6130_get_modes, > + .enable = visionox_vtdr6130_enable, > + .disable = visionox_vtdr6130_disable, > }; > > static int visionox_vtdr6130_bl_update_status(struct backlight_device *bl) > @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) > { > struct device *dev = &dsi->dev; > struct visionox_vtdr6130 *ctx; > + struct drm_dsc_config *dsc; > int ret; > > ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); > if (!ctx) > return -ENOMEM; > + > + ctx->video_mode = of_property_read_bool(dev->of_node, "enforce-video-mode"); Please also add a DT bindings patch. > + > + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); > + if (!dsc) > + return -ENOMEM; You can add struct drm_dsc_config to struct visionox_vtdr6130 instead of allocating it. > + > + /* Set DSC params */ > + dsc->dsc_version_major = 0x1; > + dsc->dsc_version_minor = 0x2; > + > + dsc->slice_height = 40; > + dsc->slice_width = 540; > + dsc->slice_count = 2; > + dsc->bits_per_component = 8; > + dsc->bits_per_pixel = 8 << 4; > + dsc->block_pred_enable = true; > + > + dsi->dsc = dsc; Only in command mode? > > ctx->supplies[0].supply = "vddio"; > ctx->supplies[1].supply = "vci"; > @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) > > dsi->lanes = 4; > dsi->format = MIPI_DSI_FMT_RGB888; > - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | > - MIPI_DSI_CLOCK_NON_CONTINUOUS; > + > + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS; Keep the line split please. > + if (ctx->video_mode) > + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; > + > ctx->panel.prepare_prev_first = true; > > drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, > -- > 2.41.0 >
On 7/28/2023 2:37 AM, Dmitry Baryshkov wrote: > On Fri, 28 Jul 2023 at 04:26, Paloma Arellano <quic_parellan@quicinc.com> wrote: >> >> Enable display compression (DSC v1.2) and CMD mode for 1080x2400 Visionox >> VTDR6130 AMOLED DSI panel. In addition, this patch will set the default >> to command mode with DSC enabled. >> >> Note: This patch has only been validated DSC over command mode as DSC over >> video mode has never been validated for the MSM driver before. >> >> Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] >> >> Changes since v1: >> - Changed from email address >> >> [1] https://patchwork.freedesktop.org/series/121337/ >> >> Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >> --- >> .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 ++++++++++++++++++- >> 1 file changed, 73 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >> index e1363e128e7e..5658d39a3a6b 100644 >> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >> @@ -9,6 +9,7 @@ >> #include <linux/of.h> >> >> #include <drm/display/drm_dsc.h> >> +#include <drm/display/drm_dsc_helper.h> >> #include <drm/drm_mipi_dsi.h> >> #include <drm/drm_modes.h> >> #include <drm/drm_panel.h> >> @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { >> struct mipi_dsi_device *dsi; >> struct gpio_desc *reset_gpio; >> struct regulator_bulk_data supplies[3]; >> - bool prepared; >> + bool prepared, enabled; >> + bool video_mode; >> }; >> >> static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct drm_panel *panel) >> @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct visionox_vtdr6130 *ctx) >> if (ret) >> return ret; >> >> + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); >> mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20); >> mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00); >> mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); >> mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); >> mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); >> - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >> + >> + if (ctx->video_mode) >> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >> + else >> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); >> + >> mipi_dsi_dcs_write_seq(dsi, 0x70, >> 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, 0x09, 0x60, 0x04, >> 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, 0x1c, 0x02, 0x00, >> @@ -214,6 +222,42 @@ static const struct drm_display_mode visionox_vtdr6130_mode = { >> .height_mm = 157, >> }; >> >> +static int visionox_vtdr6130_enable(struct drm_panel *panel) >> +{ >> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >> + struct mipi_dsi_device *dsi = ctx->dsi; >> + struct drm_dsc_picture_parameter_set pps; >> + int ret; >> + >> + if (ctx->enabled) >> + return 0; >> + >> + if (!dsi->dsc) { >> + dev_err(&dsi->dev, "DSC not attached to DSI\n"); >> + return -ENODEV; >> + } > > The error message is misleading. Also, if you don't want to enable DSC > for the video mode, this will break. > >> + >> + drm_dsc_pps_payload_pack(&pps, dsi->dsc); >> + ret = mipi_dsi_picture_parameter_set(dsi, &pps); >> + if (ret) { >> + dev_err(&dsi->dev, "Failed to set PPS\n"); >> + return ret; >> + } >> + >> + ctx->enabled = true; > > Do we need this refcount just for PPS upload? What will happen if PPS > is uploaded several times? > >> + >> + return 0; >> +} >> + >> +static int visionox_vtdr6130_disable(struct drm_panel *panel) >> +{ >> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >> + >> + ctx->enabled = false; >> + >> + return 0; >> +} >> + >> static int visionox_vtdr6130_get_modes(struct drm_panel *panel, >> struct drm_connector *connector) >> { >> @@ -237,6 +281,8 @@ static const struct drm_panel_funcs visionox_vtdr6130_panel_funcs = { >> .prepare = visionox_vtdr6130_prepare, >> .unprepare = visionox_vtdr6130_unprepare, >> .get_modes = visionox_vtdr6130_get_modes, >> + .enable = visionox_vtdr6130_enable, >> + .disable = visionox_vtdr6130_disable, >> }; >> >> static int visionox_vtdr6130_bl_update_status(struct backlight_device *bl) >> @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) >> { >> struct device *dev = &dsi->dev; >> struct visionox_vtdr6130 *ctx; >> + struct drm_dsc_config *dsc; >> int ret; >> >> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >> if (!ctx) >> return -ENOMEM; >> + >> + ctx->video_mode = of_property_read_bool(dev->of_node, "enforce-video-mode"); > > Please also add a DT bindings patch. > >> + >> + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); >> + if (!dsc) >> + return -ENOMEM; > > You can add struct drm_dsc_config to struct visionox_vtdr6130 instead > of allocating it. > >> + >> + /* Set DSC params */ >> + dsc->dsc_version_major = 0x1; >> + dsc->dsc_version_minor = 0x2; >> + >> + dsc->slice_height = 40; >> + dsc->slice_width = 540; >> + dsc->slice_count = 2; >> + dsc->bits_per_component = 8; >> + dsc->bits_per_pixel = 8 << 4; >> + dsc->block_pred_enable = true; >> + >> + dsi->dsc = dsc; > > Only in command mode? Hi Dmitry, The intention of the patch wasn't to enable DSC for only command mode. We didn't want to limit DSC to only command mode because, while the MSM DPU driver isn't able to validate DSC on video mode, other vendors might have already validated DSC on video mode and would benefit from this patch. FWIW, inital driver commit [1] notes that the panel is meant to work with compressed streams in general and DSC support was tob be added later on. Thanks, Jessica Zhang [1] https://patchwork.freedesktop.org/patch/517483/?series=112369&rev=2 > >> >> ctx->supplies[0].supply = "vddio"; >> ctx->supplies[1].supply = "vci"; >> @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) >> >> dsi->lanes = 4; >> dsi->format = MIPI_DSI_FMT_RGB888; >> - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | >> - MIPI_DSI_CLOCK_NON_CONTINUOUS; >> + >> + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS; > > Keep the line split please. > >> + if (ctx->video_mode) >> + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; >> + >> ctx->panel.prepare_prev_first = true; >> >> drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, >> -- >> 2.41.0 >> > > > -- > With best wishes > Dmitry
On 29/07/2023 00:44, Jessica Zhang wrote: > > > On 7/28/2023 2:37 AM, Dmitry Baryshkov wrote: >> On Fri, 28 Jul 2023 at 04:26, Paloma Arellano >> <quic_parellan@quicinc.com> wrote: >>> >>> Enable display compression (DSC v1.2) and CMD mode for 1080x2400 >>> Visionox >>> VTDR6130 AMOLED DSI panel. In addition, this patch will set the default >>> to command mode with DSC enabled. >>> >>> Note: This patch has only been validated DSC over command mode as DSC >>> over >>> video mode has never been validated for the MSM driver before. >>> >>> Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] >>> >>> Changes since v1: >>> - Changed from email address >>> >>> [1] https://patchwork.freedesktop.org/series/121337/ >>> >>> Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>> --- >>> .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 ++++++++++++++++++- >>> 1 file changed, 73 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> index e1363e128e7e..5658d39a3a6b 100644 >>> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> @@ -9,6 +9,7 @@ >>> #include <linux/of.h> >>> >>> #include <drm/display/drm_dsc.h> >>> +#include <drm/display/drm_dsc_helper.h> >>> #include <drm/drm_mipi_dsi.h> >>> #include <drm/drm_modes.h> >>> #include <drm/drm_panel.h> >>> @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { >>> struct mipi_dsi_device *dsi; >>> struct gpio_desc *reset_gpio; >>> struct regulator_bulk_data supplies[3]; >>> - bool prepared; >>> + bool prepared, enabled; >>> + bool video_mode; >>> }; >>> >>> static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct >>> drm_panel *panel) >>> @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct >>> visionox_vtdr6130 *ctx) >>> if (ret) >>> return ret; >>> >>> + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); >>> mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, >>> 0x20); >>> mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, >>> 0x00, 0x00); >>> mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); >>> mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); >>> mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); >>> - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>> + >>> + if (ctx->video_mode) >>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>> + else >>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); >>> + >>> mipi_dsi_dcs_write_seq(dsi, 0x70, >>> 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, >>> 0x09, 0x60, 0x04, >>> 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, >>> 0x1c, 0x02, 0x00, >>> @@ -214,6 +222,42 @@ static const struct drm_display_mode >>> visionox_vtdr6130_mode = { >>> .height_mm = 157, >>> }; >>> >>> +static int visionox_vtdr6130_enable(struct drm_panel *panel) >>> +{ >>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>> + struct mipi_dsi_device *dsi = ctx->dsi; >>> + struct drm_dsc_picture_parameter_set pps; >>> + int ret; >>> + >>> + if (ctx->enabled) >>> + return 0; >>> + >>> + if (!dsi->dsc) { >>> + dev_err(&dsi->dev, "DSC not attached to DSI\n"); >>> + return -ENODEV; >>> + } >> >> The error message is misleading. Also, if you don't want to enable DSC >> for the video mode, this will break. >> >>> + >>> + drm_dsc_pps_payload_pack(&pps, dsi->dsc); >>> + ret = mipi_dsi_picture_parameter_set(dsi, &pps); >>> + if (ret) { >>> + dev_err(&dsi->dev, "Failed to set PPS\n"); >>> + return ret; >>> + } >>> + >>> + ctx->enabled = true; >> >> Do we need this refcount just for PPS upload? What will happen if PPS >> is uploaded several times? >> >>> + >>> + return 0; >>> +} >>> + >>> +static int visionox_vtdr6130_disable(struct drm_panel *panel) >>> +{ >>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>> + >>> + ctx->enabled = false; >>> + >>> + return 0; >>> +} >>> + >>> static int visionox_vtdr6130_get_modes(struct drm_panel *panel, >>> struct drm_connector *connector) >>> { >>> @@ -237,6 +281,8 @@ static const struct drm_panel_funcs >>> visionox_vtdr6130_panel_funcs = { >>> .prepare = visionox_vtdr6130_prepare, >>> .unprepare = visionox_vtdr6130_unprepare, >>> .get_modes = visionox_vtdr6130_get_modes, >>> + .enable = visionox_vtdr6130_enable, >>> + .disable = visionox_vtdr6130_disable, >>> }; >>> >>> static int visionox_vtdr6130_bl_update_status(struct >>> backlight_device *bl) >>> @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct >>> mipi_dsi_device *dsi) >>> { >>> struct device *dev = &dsi->dev; >>> struct visionox_vtdr6130 *ctx; >>> + struct drm_dsc_config *dsc; >>> int ret; >>> >>> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >>> if (!ctx) >>> return -ENOMEM; >>> + >>> + ctx->video_mode = of_property_read_bool(dev->of_node, >>> "enforce-video-mode"); >> >> Please also add a DT bindings patch. >> >>> + >>> + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); >>> + if (!dsc) >>> + return -ENOMEM; >> >> You can add struct drm_dsc_config to struct visionox_vtdr6130 instead >> of allocating it. >> >>> + >>> + /* Set DSC params */ >>> + dsc->dsc_version_major = 0x1; >>> + dsc->dsc_version_minor = 0x2; >>> + >>> + dsc->slice_height = 40; >>> + dsc->slice_width = 540; >>> + dsc->slice_count = 2; >>> + dsc->bits_per_component = 8; >>> + dsc->bits_per_pixel = 8 << 4; >>> + dsc->block_pred_enable = true; >>> + >>> + dsi->dsc = dsc; >> >> Only in command mode? > > Hi Dmitry, > > The intention of the patch wasn't to enable DSC for only command mode. > > We didn't want to limit DSC to only command mode because, while the MSM > DPU driver isn't able to validate DSC on video mode, other vendors might > have already validated DSC on video mode and would benefit from this patch. > > FWIW, inital driver commit [1] notes that the panel is meant to work > with compressed streams in general and DSC support was tob be added > later on. Ack. > > Thanks, > > Jessica Zhang > > [1] https://patchwork.freedesktop.org/patch/517483/?series=112369&rev=2 > >> >>> >>> ctx->supplies[0].supply = "vddio"; >>> ctx->supplies[1].supply = "vci"; >>> @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct >>> mipi_dsi_device *dsi) >>> >>> dsi->lanes = 4; >>> dsi->format = MIPI_DSI_FMT_RGB888; >>> - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >>> MIPI_DSI_MODE_NO_EOT_PACKET | >>> - MIPI_DSI_CLOCK_NON_CONTINUOUS; >>> + >>> + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | >>> MIPI_DSI_CLOCK_NON_CONTINUOUS; >> >> Keep the line split please. >> >>> + if (ctx->video_mode) >>> + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; >>> + >>> ctx->panel.prepare_prev_first = true; >>> >>> drm_panel_init(&ctx->panel, dev, >>> &visionox_vtdr6130_panel_funcs, >>> -- >>> 2.41.0 >>> >> >> >> -- >> With best wishes >> Dmitry
Hi, On 28/07/2023 03:26, Paloma Arellano wrote: > Enable display compression (DSC v1.2) and CMD mode for 1080x2400 Visionox > VTDR6130 AMOLED DSI panel. In addition, this patch will set the default > to command mode with DSC enabled. > > Note: This patch has only been validated DSC over command mode as DSC over > video mode has never been validated for the MSM driver before. > > Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] I'll run a test on my SM8550 platform, Thanks, Neil > > Changes since v1: > - Changed from email address > > [1] https://patchwork.freedesktop.org/series/121337/ > > Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> > Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > --- > .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 ++++++++++++++++++- > 1 file changed, 73 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > index e1363e128e7e..5658d39a3a6b 100644 > --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > @@ -9,6 +9,7 @@ > #include <linux/of.h> > > #include <drm/display/drm_dsc.h> > +#include <drm/display/drm_dsc_helper.h> > #include <drm/drm_mipi_dsi.h> > #include <drm/drm_modes.h> > #include <drm/drm_panel.h> > @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { > struct mipi_dsi_device *dsi; > struct gpio_desc *reset_gpio; > struct regulator_bulk_data supplies[3]; > - bool prepared; > + bool prepared, enabled; > + bool video_mode; > }; > > static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct drm_panel *panel) > @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct visionox_vtdr6130 *ctx) > if (ret) > return ret; > > + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); > mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20); > mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00); > mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); > mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); > mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); > - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); > + > + if (ctx->video_mode) > + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); > + else > + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); > + > mipi_dsi_dcs_write_seq(dsi, 0x70, > 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, 0x09, 0x60, 0x04, > 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, 0x1c, 0x02, 0x00, > @@ -214,6 +222,42 @@ static const struct drm_display_mode visionox_vtdr6130_mode = { > .height_mm = 157, > }; > > +static int visionox_vtdr6130_enable(struct drm_panel *panel) > +{ > + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); > + struct mipi_dsi_device *dsi = ctx->dsi; > + struct drm_dsc_picture_parameter_set pps; > + int ret; > + > + if (ctx->enabled) > + return 0; > + > + if (!dsi->dsc) { > + dev_err(&dsi->dev, "DSC not attached to DSI\n"); > + return -ENODEV; > + } > + > + drm_dsc_pps_payload_pack(&pps, dsi->dsc); > + ret = mipi_dsi_picture_parameter_set(dsi, &pps); > + if (ret) { > + dev_err(&dsi->dev, "Failed to set PPS\n"); > + return ret; > + } > + > + ctx->enabled = true; > + > + return 0; > +} > + > +static int visionox_vtdr6130_disable(struct drm_panel *panel) > +{ > + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); > + > + ctx->enabled = false; > + > + return 0; > +} > + > static int visionox_vtdr6130_get_modes(struct drm_panel *panel, > struct drm_connector *connector) > { > @@ -237,6 +281,8 @@ static const struct drm_panel_funcs visionox_vtdr6130_panel_funcs = { > .prepare = visionox_vtdr6130_prepare, > .unprepare = visionox_vtdr6130_unprepare, > .get_modes = visionox_vtdr6130_get_modes, > + .enable = visionox_vtdr6130_enable, > + .disable = visionox_vtdr6130_disable, > }; > > static int visionox_vtdr6130_bl_update_status(struct backlight_device *bl) > @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) > { > struct device *dev = &dsi->dev; > struct visionox_vtdr6130 *ctx; > + struct drm_dsc_config *dsc; > int ret; > > ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); > if (!ctx) > return -ENOMEM; > + > + ctx->video_mode = of_property_read_bool(dev->of_node, "enforce-video-mode"); > + > + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); > + if (!dsc) > + return -ENOMEM; > + > + /* Set DSC params */ > + dsc->dsc_version_major = 0x1; > + dsc->dsc_version_minor = 0x2; > + > + dsc->slice_height = 40; > + dsc->slice_width = 540; > + dsc->slice_count = 2; > + dsc->bits_per_component = 8; > + dsc->bits_per_pixel = 8 << 4; > + dsc->block_pred_enable = true; > + > + dsi->dsc = dsc; > > ctx->supplies[0].supply = "vddio"; > ctx->supplies[1].supply = "vci"; > @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) > > dsi->lanes = 4; > dsi->format = MIPI_DSI_FMT_RGB888; > - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | > - MIPI_DSI_CLOCK_NON_CONTINUOUS; > + > + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS; > + if (ctx->video_mode) > + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; > + > ctx->panel.prepare_prev_first = true; > > drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs,
CC-ing rest of correspondents On 7/28/2023 5:43 PM, Paloma Arellano wrote: > > On 7/28/2023 2:37 AM, Dmitry Baryshkov wrote: >> On Fri, 28 Jul 2023 at 04:26, Paloma Arellano >> <quic_parellan@quicinc.com> wrote: >>> Enable display compression (DSC v1.2) and CMD mode for 1080x2400 >>> Visionox >>> VTDR6130 AMOLED DSI panel. In addition, this patch will set the default >>> to command mode with DSC enabled. >>> >>> Note: This patch has only been validated DSC over command mode as >>> DSC over >>> video mode has never been validated for the MSM driver before. >>> >>> Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] >>> >>> Changes since v1: >>> - Changed from email address >>> >>> [1] https://patchwork.freedesktop.org/series/121337/ >>> >>> Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>> --- >>> .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 >>> ++++++++++++++++++- >>> 1 file changed, 73 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> index e1363e128e7e..5658d39a3a6b 100644 >>> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> @@ -9,6 +9,7 @@ >>> #include <linux/of.h> >>> >>> #include <drm/display/drm_dsc.h> >>> +#include <drm/display/drm_dsc_helper.h> >>> #include <drm/drm_mipi_dsi.h> >>> #include <drm/drm_modes.h> >>> #include <drm/drm_panel.h> >>> @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { >>> struct mipi_dsi_device *dsi; >>> struct gpio_desc *reset_gpio; >>> struct regulator_bulk_data supplies[3]; >>> - bool prepared; >>> + bool prepared, enabled; >>> + bool video_mode; >>> }; >>> >>> static inline struct visionox_vtdr6130 >>> *to_visionox_vtdr6130(struct drm_panel *panel) >>> @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct >>> visionox_vtdr6130 *ctx) >>> if (ret) >>> return ret; >>> >>> + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); >>> mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, >>> 0x20); >>> mipi_dsi_dcs_write_seq(dsi, >>> MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00); >>> mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); >>> mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); >>> mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); >>> - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>> + >>> + if (ctx->video_mode) >>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>> + else >>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); >>> + >>> mipi_dsi_dcs_write_seq(dsi, 0x70, >>> 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, >>> 0x09, 0x60, 0x04, >>> 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, >>> 0x1c, 0x02, 0x00, >>> @@ -214,6 +222,42 @@ static const struct drm_display_mode >>> visionox_vtdr6130_mode = { >>> .height_mm = 157, >>> }; >>> >>> +static int visionox_vtdr6130_enable(struct drm_panel *panel) >>> +{ >>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>> + struct mipi_dsi_device *dsi = ctx->dsi; >>> + struct drm_dsc_picture_parameter_set pps; >>> + int ret; >>> + >>> + if (ctx->enabled) >>> + return 0; >>> + >>> + if (!dsi->dsc) { >>> + dev_err(&dsi->dev, "DSC not attached to DSI\n"); >>> + return -ENODEV; >>> + } >> The error message is misleading. Also, if you don't want to enable DSC >> for the video mode, this will break. > Changed the phrasing to "DSC not enabled on DSI" >> >>> + >>> + drm_dsc_pps_payload_pack(&pps, dsi->dsc); >>> + ret = mipi_dsi_picture_parameter_set(dsi, &pps); >>> + if (ret) { >>> + dev_err(&dsi->dev, "Failed to set PPS\n"); >>> + return ret; >>> + } >>> + >>> + ctx->enabled = true; >> Do we need this refcount just for PPS upload? What will happen if PPS >> is uploaded several times? > > Removing the refcount does not crash the device. Wouldn't it be better > to send the configuration once, instead of re-sending the pps every > time the panel is enabled? > >> >>> + >>> + return 0; >>> +} >>> + >>> +static int visionox_vtdr6130_disable(struct drm_panel *panel) >>> +{ >>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>> + >>> + ctx->enabled = false; >>> + >>> + return 0; >>> +} >>> + >>> static int visionox_vtdr6130_get_modes(struct drm_panel *panel, >>> struct drm_connector >>> *connector) >>> { >>> @@ -237,6 +281,8 @@ static const struct drm_panel_funcs >>> visionox_vtdr6130_panel_funcs = { >>> .prepare = visionox_vtdr6130_prepare, >>> .unprepare = visionox_vtdr6130_unprepare, >>> .get_modes = visionox_vtdr6130_get_modes, >>> + .enable = visionox_vtdr6130_enable, >>> + .disable = visionox_vtdr6130_disable, >>> }; >>> >>> static int visionox_vtdr6130_bl_update_status(struct >>> backlight_device *bl) >>> @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct >>> mipi_dsi_device *dsi) >>> { >>> struct device *dev = &dsi->dev; >>> struct visionox_vtdr6130 *ctx; >>> + struct drm_dsc_config *dsc; >>> int ret; >>> >>> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >>> if (!ctx) >>> return -ENOMEM; >>> + >>> + ctx->video_mode = of_property_read_bool(dev->of_node, >>> "enforce-video-mode"); >> Please also add a DT bindings patch. > Ack >> >>> + >>> + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); >>> + if (!dsc) >>> + return -ENOMEM; >> You can add struct drm_dsc_config to struct visionox_vtdr6130 instead >> of allocating it. > Is there a benefit of adding the drm_dsc_config to the panel struct > versus just allocating it separately? >> >>> + >>> + /* Set DSC params */ >>> + dsc->dsc_version_major = 0x1; >>> + dsc->dsc_version_minor = 0x2; >>> + >>> + dsc->slice_height = 40; >>> + dsc->slice_width = 540; >>> + dsc->slice_count = 2; >>> + dsc->bits_per_component = 8; >>> + dsc->bits_per_pixel = 8 << 4; >>> + dsc->block_pred_enable = true; >>> + >>> + dsi->dsc = dsc; >> Only in command mode? > This has been resolved in a separate thread. >> >>> ctx->supplies[0].supply = "vddio"; >>> ctx->supplies[1].supply = "vci"; >>> @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct >>> mipi_dsi_device *dsi) >>> >>> dsi->lanes = 4; >>> dsi->format = MIPI_DSI_FMT_RGB888; >>> - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >>> MIPI_DSI_MODE_NO_EOT_PACKET | >>> - MIPI_DSI_CLOCK_NON_CONTINUOUS; >>> + >>> + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | >>> MIPI_DSI_CLOCK_NON_CONTINUOUS; >> Keep the line split please. > Ack >> >>> + if (ctx->video_mode) >>> + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; >>> + >>> ctx->panel.prepare_prev_first = true; >>> >>> drm_panel_init(&ctx->panel, dev, >>> &visionox_vtdr6130_panel_funcs, >>> -- >>> 2.41.0 >>> >>
On 28/07/2023 23:44, Jessica Zhang wrote: > > > On 7/28/2023 2:37 AM, Dmitry Baryshkov wrote: >> On Fri, 28 Jul 2023 at 04:26, Paloma Arellano <quic_parellan@quicinc.com> wrote: >>> >>> Enable display compression (DSC v1.2) and CMD mode for 1080x2400 Visionox >>> VTDR6130 AMOLED DSI panel. In addition, this patch will set the default >>> to command mode with DSC enabled. >>> >>> Note: This patch has only been validated DSC over command mode as DSC over >>> video mode has never been validated for the MSM driver before. >>> >>> Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] >>> >>> Changes since v1: >>> - Changed from email address >>> >>> [1] https://patchwork.freedesktop.org/series/121337/ >>> >>> Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>> --- >>> .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 ++++++++++++++++++- >>> 1 file changed, 73 insertions(+), 4 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> index e1363e128e7e..5658d39a3a6b 100644 >>> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>> @@ -9,6 +9,7 @@ >>> #include <linux/of.h> >>> >>> #include <drm/display/drm_dsc.h> >>> +#include <drm/display/drm_dsc_helper.h> >>> #include <drm/drm_mipi_dsi.h> >>> #include <drm/drm_modes.h> >>> #include <drm/drm_panel.h> >>> @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { >>> struct mipi_dsi_device *dsi; >>> struct gpio_desc *reset_gpio; >>> struct regulator_bulk_data supplies[3]; >>> - bool prepared; >>> + bool prepared, enabled; >>> + bool video_mode; >>> }; >>> >>> static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct drm_panel *panel) >>> @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct visionox_vtdr6130 *ctx) >>> if (ret) >>> return ret; >>> >>> + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); >>> mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20); >>> mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00); >>> mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); >>> mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); >>> mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); >>> - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>> + >>> + if (ctx->video_mode) >>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>> + else >>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); >>> + >>> mipi_dsi_dcs_write_seq(dsi, 0x70, >>> 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, 0x09, 0x60, 0x04, >>> 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, 0x1c, 0x02, 0x00, >>> @@ -214,6 +222,42 @@ static const struct drm_display_mode visionox_vtdr6130_mode = { >>> .height_mm = 157, >>> }; >>> >>> +static int visionox_vtdr6130_enable(struct drm_panel *panel) >>> +{ >>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>> + struct mipi_dsi_device *dsi = ctx->dsi; >>> + struct drm_dsc_picture_parameter_set pps; >>> + int ret; >>> + >>> + if (ctx->enabled) >>> + return 0; >>> + >>> + if (!dsi->dsc) { >>> + dev_err(&dsi->dev, "DSC not attached to DSI\n"); >>> + return -ENODEV; >>> + } >> >> The error message is misleading. Also, if you don't want to enable DSC >> for the video mode, this will break. >> >>> + >>> + drm_dsc_pps_payload_pack(&pps, dsi->dsc); >>> + ret = mipi_dsi_picture_parameter_set(dsi, &pps); >>> + if (ret) { >>> + dev_err(&dsi->dev, "Failed to set PPS\n"); >>> + return ret; >>> + } >>> + >>> + ctx->enabled = true; >> >> Do we need this refcount just for PPS upload? What will happen if PPS >> is uploaded several times? >> >>> + >>> + return 0; >>> +} >>> + >>> +static int visionox_vtdr6130_disable(struct drm_panel *panel) >>> +{ >>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>> + >>> + ctx->enabled = false; >>> + >>> + return 0; >>> +} >>> + >>> static int visionox_vtdr6130_get_modes(struct drm_panel *panel, >>> struct drm_connector *connector) >>> { >>> @@ -237,6 +281,8 @@ static const struct drm_panel_funcs visionox_vtdr6130_panel_funcs = { >>> .prepare = visionox_vtdr6130_prepare, >>> .unprepare = visionox_vtdr6130_unprepare, >>> .get_modes = visionox_vtdr6130_get_modes, >>> + .enable = visionox_vtdr6130_enable, >>> + .disable = visionox_vtdr6130_disable, >>> }; >>> >>> static int visionox_vtdr6130_bl_update_status(struct backlight_device *bl) >>> @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) >>> { >>> struct device *dev = &dsi->dev; >>> struct visionox_vtdr6130 *ctx; >>> + struct drm_dsc_config *dsc; >>> int ret; >>> >>> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >>> if (!ctx) >>> return -ENOMEM; >>> + >>> + ctx->video_mode = of_property_read_bool(dev->of_node, "enforce-video-mode"); >> >> Please also add a DT bindings patch. >> >>> + >>> + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); >>> + if (!dsc) >>> + return -ENOMEM; >> >> You can add struct drm_dsc_config to struct visionox_vtdr6130 instead >> of allocating it. >> >>> + >>> + /* Set DSC params */ >>> + dsc->dsc_version_major = 0x1; >>> + dsc->dsc_version_minor = 0x2; >>> + >>> + dsc->slice_height = 40; >>> + dsc->slice_width = 540; >>> + dsc->slice_count = 2; >>> + dsc->bits_per_component = 8; >>> + dsc->bits_per_pixel = 8 << 4; >>> + dsc->block_pred_enable = true; >>> + >>> + dsi->dsc = dsc; >> >> Only in command mode? > > Hi Dmitry, > > The intention of the patch wasn't to enable DSC for only command mode. > > We didn't want to limit DSC to only command mode because, while the MSM DPU driver isn't able to validate DSC on video mode, other vendors might have already validated DSC on video mode and would benefit from this patch. > > FWIW, inital driver commit [1] notes that the panel is meant to work with compressed streams in general and DSC support was tob be added later on. The panel supports Video, Video+DSC, CMD, CMD+DSC, so it would be great to be able to select any of the supported modes, including the non-compressed ones. So enforce-video-mode is great, but an enforce-uncompressed-mode would be necessary aswell. Neil > > Thanks, > > Jessica Zhang > > [1] https://patchwork.freedesktop.org/patch/517483/?series=112369&rev=2 > >> >>> >>> ctx->supplies[0].supply = "vddio"; >>> ctx->supplies[1].supply = "vci"; >>> @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) >>> >>> dsi->lanes = 4; >>> dsi->format = MIPI_DSI_FMT_RGB888; >>> - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | >>> - MIPI_DSI_CLOCK_NON_CONTINUOUS; >>> + >>> + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS; >> >> Keep the line split please. >> >>> + if (ctx->video_mode) >>> + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; >>> + >>> ctx->panel.prepare_prev_first = true; >>> >>> drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, >>> -- >>> 2.41.0 >>> >> >> >> -- >> With best wishes >> Dmitry
On 8/1/2023 1:26 AM, neil.armstrong@linaro.org wrote: > On 28/07/2023 23:44, Jessica Zhang wrote: >> >> >> On 7/28/2023 2:37 AM, Dmitry Baryshkov wrote: >>> On Fri, 28 Jul 2023 at 04:26, Paloma Arellano >>> <quic_parellan@quicinc.com> wrote: >>>> >>>> Enable display compression (DSC v1.2) and CMD mode for 1080x2400 >>>> Visionox >>>> VTDR6130 AMOLED DSI panel. In addition, this patch will set the >>>> default >>>> to command mode with DSC enabled. >>>> >>>> Note: This patch has only been validated DSC over command mode as >>>> DSC over >>>> video mode has never been validated for the MSM driver before. >>>> >>>> Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] >>>> >>>> Changes since v1: >>>> - Changed from email address >>>> >>>> [1] https://patchwork.freedesktop.org/series/121337/ >>>> >>>> Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>> --- >>>> .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 >>>> ++++++++++++++++++- >>>> 1 file changed, 73 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>> b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>> index e1363e128e7e..5658d39a3a6b 100644 >>>> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>> @@ -9,6 +9,7 @@ >>>> #include <linux/of.h> >>>> >>>> #include <drm/display/drm_dsc.h> >>>> +#include <drm/display/drm_dsc_helper.h> >>>> #include <drm/drm_mipi_dsi.h> >>>> #include <drm/drm_modes.h> >>>> #include <drm/drm_panel.h> >>>> @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { >>>> struct mipi_dsi_device *dsi; >>>> struct gpio_desc *reset_gpio; >>>> struct regulator_bulk_data supplies[3]; >>>> - bool prepared; >>>> + bool prepared, enabled; >>>> + bool video_mode; >>>> }; >>>> >>>> static inline struct visionox_vtdr6130 >>>> *to_visionox_vtdr6130(struct drm_panel *panel) >>>> @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct >>>> visionox_vtdr6130 *ctx) >>>> if (ret) >>>> return ret; >>>> >>>> + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); >>>> mipi_dsi_dcs_write_seq(dsi, >>>> MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20); >>>> mipi_dsi_dcs_write_seq(dsi, >>>> MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00); >>>> mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); >>>> mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); >>>> mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); >>>> - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>>> + >>>> + if (ctx->video_mode) >>>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>>> + else >>>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); >>>> + >>>> mipi_dsi_dcs_write_seq(dsi, 0x70, >>>> 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, >>>> 0x09, 0x60, 0x04, >>>> 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, >>>> 0x1c, 0x02, 0x00, >>>> @@ -214,6 +222,42 @@ static const struct drm_display_mode >>>> visionox_vtdr6130_mode = { >>>> .height_mm = 157, >>>> }; >>>> >>>> +static int visionox_vtdr6130_enable(struct drm_panel *panel) >>>> +{ >>>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>>> + struct mipi_dsi_device *dsi = ctx->dsi; >>>> + struct drm_dsc_picture_parameter_set pps; >>>> + int ret; >>>> + >>>> + if (ctx->enabled) >>>> + return 0; >>>> + >>>> + if (!dsi->dsc) { >>>> + dev_err(&dsi->dev, "DSC not attached to DSI\n"); >>>> + return -ENODEV; >>>> + } >>> >>> The error message is misleading. Also, if you don't want to enable DSC >>> for the video mode, this will break. >>> >>>> + >>>> + drm_dsc_pps_payload_pack(&pps, dsi->dsc); >>>> + ret = mipi_dsi_picture_parameter_set(dsi, &pps); >>>> + if (ret) { >>>> + dev_err(&dsi->dev, "Failed to set PPS\n"); >>>> + return ret; >>>> + } >>>> + >>>> + ctx->enabled = true; >>> >>> Do we need this refcount just for PPS upload? What will happen if PPS >>> is uploaded several times? >>> >>>> + >>>> + return 0; >>>> +} >>>> + >>>> +static int visionox_vtdr6130_disable(struct drm_panel *panel) >>>> +{ >>>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>>> + >>>> + ctx->enabled = false; >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int visionox_vtdr6130_get_modes(struct drm_panel *panel, >>>> struct drm_connector >>>> *connector) >>>> { >>>> @@ -237,6 +281,8 @@ static const struct drm_panel_funcs >>>> visionox_vtdr6130_panel_funcs = { >>>> .prepare = visionox_vtdr6130_prepare, >>>> .unprepare = visionox_vtdr6130_unprepare, >>>> .get_modes = visionox_vtdr6130_get_modes, >>>> + .enable = visionox_vtdr6130_enable, >>>> + .disable = visionox_vtdr6130_disable, >>>> }; >>>> >>>> static int visionox_vtdr6130_bl_update_status(struct >>>> backlight_device *bl) >>>> @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct >>>> mipi_dsi_device *dsi) >>>> { >>>> struct device *dev = &dsi->dev; >>>> struct visionox_vtdr6130 *ctx; >>>> + struct drm_dsc_config *dsc; >>>> int ret; >>>> >>>> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >>>> if (!ctx) >>>> return -ENOMEM; >>>> + >>>> + ctx->video_mode = of_property_read_bool(dev->of_node, >>>> "enforce-video-mode"); >>> >>> Please also add a DT bindings patch. >>> >>>> + >>>> + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); >>>> + if (!dsc) >>>> + return -ENOMEM; >>> >>> You can add struct drm_dsc_config to struct visionox_vtdr6130 instead >>> of allocating it. >>> >>>> + >>>> + /* Set DSC params */ >>>> + dsc->dsc_version_major = 0x1; >>>> + dsc->dsc_version_minor = 0x2; >>>> + >>>> + dsc->slice_height = 40; >>>> + dsc->slice_width = 540; >>>> + dsc->slice_count = 2; >>>> + dsc->bits_per_component = 8; >>>> + dsc->bits_per_pixel = 8 << 4; >>>> + dsc->block_pred_enable = true; >>>> + >>>> + dsi->dsc = dsc; >>> >>> Only in command mode? >> >> Hi Dmitry, >> >> The intention of the patch wasn't to enable DSC for only command mode. >> >> We didn't want to limit DSC to only command mode because, while the >> MSM DPU driver isn't able to validate DSC on video mode, other >> vendors might have already validated DSC on video mode and would >> benefit from this patch. >> >> FWIW, inital driver commit [1] notes that the panel is meant to work >> with compressed streams in general and DSC support was tob be added >> later on. > > The panel supports Video, Video+DSC, CMD, CMD+DSC, so it would be > great to be able to > select any of the supported modes, including the non-compressed ones. > > So enforce-video-mode is great, but an enforce-uncompressed-mode would > be necessary > aswell. > > Neil Hi Neil, Are you suggesting to add a new binding to handle the 'enforce-uncompressed-mode'? -Paloma > >> >> Thanks, >> >> Jessica Zhang >> >> [1] https://patchwork.freedesktop.org/patch/517483/?series=112369&rev=2 >> >>> >>>> >>>> ctx->supplies[0].supply = "vddio"; >>>> ctx->supplies[1].supply = "vci"; >>>> @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct >>>> mipi_dsi_device *dsi) >>>> >>>> dsi->lanes = 4; >>>> dsi->format = MIPI_DSI_FMT_RGB888; >>>> - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >>>> MIPI_DSI_MODE_NO_EOT_PACKET | >>>> - MIPI_DSI_CLOCK_NON_CONTINUOUS; >>>> + >>>> + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | >>>> MIPI_DSI_CLOCK_NON_CONTINUOUS; >>> >>> Keep the line split please. >>> >>>> + if (ctx->video_mode) >>>> + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; >>>> + >>>> ctx->panel.prepare_prev_first = true; >>>> >>>> drm_panel_init(&ctx->panel, dev, >>>> &visionox_vtdr6130_panel_funcs, >>>> -- >>>> 2.41.0 >>>> >>> >>> >>> -- >>> With best wishes >>> Dmitry >
On 01/08/2023 23:43, Paloma Arellano wrote: > > On 8/1/2023 1:26 AM, neil.armstrong@linaro.org wrote: >> On 28/07/2023 23:44, Jessica Zhang wrote: >>> >>> >>> On 7/28/2023 2:37 AM, Dmitry Baryshkov wrote: >>>> On Fri, 28 Jul 2023 at 04:26, Paloma Arellano >>>> <quic_parellan@quicinc.com> wrote: >>>>> >>>>> Enable display compression (DSC v1.2) and CMD mode for 1080x2400 >>>>> Visionox >>>>> VTDR6130 AMOLED DSI panel. In addition, this patch will set the >>>>> default >>>>> to command mode with DSC enabled. >>>>> >>>>> Note: This patch has only been validated DSC over command mode as >>>>> DSC over >>>>> video mode has never been validated for the MSM driver before. >>>>> >>>>> Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] >>>>> >>>>> Changes since v1: >>>>> - Changed from email address >>>>> >>>>> [1] https://patchwork.freedesktop.org/series/121337/ >>>>> >>>>> Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>>> --- >>>>> .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 >>>>> ++++++++++++++++++- >>>>> 1 file changed, 73 insertions(+), 4 deletions(-) >>>>> >>>>> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>> b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>> index e1363e128e7e..5658d39a3a6b 100644 >>>>> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>> @@ -9,6 +9,7 @@ >>>>> #include <linux/of.h> >>>>> >>>>> #include <drm/display/drm_dsc.h> >>>>> +#include <drm/display/drm_dsc_helper.h> >>>>> #include <drm/drm_mipi_dsi.h> >>>>> #include <drm/drm_modes.h> >>>>> #include <drm/drm_panel.h> >>>>> @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { >>>>> struct mipi_dsi_device *dsi; >>>>> struct gpio_desc *reset_gpio; >>>>> struct regulator_bulk_data supplies[3]; >>>>> - bool prepared; >>>>> + bool prepared, enabled; >>>>> + bool video_mode; >>>>> }; >>>>> >>>>> static inline struct visionox_vtdr6130 >>>>> *to_visionox_vtdr6130(struct drm_panel *panel) >>>>> @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct >>>>> visionox_vtdr6130 *ctx) >>>>> if (ret) >>>>> return ret; >>>>> >>>>> + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); >>>>> mipi_dsi_dcs_write_seq(dsi, >>>>> MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20); >>>>> mipi_dsi_dcs_write_seq(dsi, >>>>> MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00); >>>>> mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); >>>>> mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); >>>>> mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); >>>>> - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>>>> + >>>>> + if (ctx->video_mode) >>>>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>>>> + else >>>>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); >>>>> + >>>>> mipi_dsi_dcs_write_seq(dsi, 0x70, >>>>> 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, >>>>> 0x09, 0x60, 0x04, >>>>> 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, >>>>> 0x1c, 0x02, 0x00, >>>>> @@ -214,6 +222,42 @@ static const struct drm_display_mode >>>>> visionox_vtdr6130_mode = { >>>>> .height_mm = 157, >>>>> }; >>>>> >>>>> +static int visionox_vtdr6130_enable(struct drm_panel *panel) >>>>> +{ >>>>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>>>> + struct mipi_dsi_device *dsi = ctx->dsi; >>>>> + struct drm_dsc_picture_parameter_set pps; >>>>> + int ret; >>>>> + >>>>> + if (ctx->enabled) >>>>> + return 0; >>>>> + >>>>> + if (!dsi->dsc) { >>>>> + dev_err(&dsi->dev, "DSC not attached to DSI\n"); >>>>> + return -ENODEV; >>>>> + } >>>> >>>> The error message is misleading. Also, if you don't want to enable DSC >>>> for the video mode, this will break. >>>> >>>>> + >>>>> + drm_dsc_pps_payload_pack(&pps, dsi->dsc); >>>>> + ret = mipi_dsi_picture_parameter_set(dsi, &pps); >>>>> + if (ret) { >>>>> + dev_err(&dsi->dev, "Failed to set PPS\n"); >>>>> + return ret; >>>>> + } >>>>> + >>>>> + ctx->enabled = true; >>>> >>>> Do we need this refcount just for PPS upload? What will happen if PPS >>>> is uploaded several times? >>>> >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> +static int visionox_vtdr6130_disable(struct drm_panel *panel) >>>>> +{ >>>>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>>>> + >>>>> + ctx->enabled = false; >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static int visionox_vtdr6130_get_modes(struct drm_panel *panel, >>>>> struct drm_connector >>>>> *connector) >>>>> { >>>>> @@ -237,6 +281,8 @@ static const struct drm_panel_funcs >>>>> visionox_vtdr6130_panel_funcs = { >>>>> .prepare = visionox_vtdr6130_prepare, >>>>> .unprepare = visionox_vtdr6130_unprepare, >>>>> .get_modes = visionox_vtdr6130_get_modes, >>>>> + .enable = visionox_vtdr6130_enable, >>>>> + .disable = visionox_vtdr6130_disable, >>>>> }; >>>>> >>>>> static int visionox_vtdr6130_bl_update_status(struct >>>>> backlight_device *bl) >>>>> @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct >>>>> mipi_dsi_device *dsi) >>>>> { >>>>> struct device *dev = &dsi->dev; >>>>> struct visionox_vtdr6130 *ctx; >>>>> + struct drm_dsc_config *dsc; >>>>> int ret; >>>>> >>>>> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >>>>> if (!ctx) >>>>> return -ENOMEM; >>>>> + >>>>> + ctx->video_mode = of_property_read_bool(dev->of_node, >>>>> "enforce-video-mode"); >>>> >>>> Please also add a DT bindings patch. >>>> >>>>> + >>>>> + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); >>>>> + if (!dsc) >>>>> + return -ENOMEM; >>>> >>>> You can add struct drm_dsc_config to struct visionox_vtdr6130 instead >>>> of allocating it. >>>> >>>>> + >>>>> + /* Set DSC params */ >>>>> + dsc->dsc_version_major = 0x1; >>>>> + dsc->dsc_version_minor = 0x2; >>>>> + >>>>> + dsc->slice_height = 40; >>>>> + dsc->slice_width = 540; >>>>> + dsc->slice_count = 2; >>>>> + dsc->bits_per_component = 8; >>>>> + dsc->bits_per_pixel = 8 << 4; >>>>> + dsc->block_pred_enable = true; >>>>> + >>>>> + dsi->dsc = dsc; >>>> >>>> Only in command mode? >>> >>> Hi Dmitry, >>> >>> The intention of the patch wasn't to enable DSC for only command mode. >>> >>> We didn't want to limit DSC to only command mode because, while the >>> MSM DPU driver isn't able to validate DSC on video mode, other >>> vendors might have already validated DSC on video mode and would >>> benefit from this patch. >>> >>> FWIW, inital driver commit [1] notes that the panel is meant to work >>> with compressed streams in general and DSC support was tob be added >>> later on. >> >> The panel supports Video, Video+DSC, CMD, CMD+DSC, so it would be >> great to be able to >> select any of the supported modes, including the non-compressed ones. >> >> So enforce-video-mode is great, but an enforce-uncompressed-mode would >> be necessary >> aswell. >> >> Neil > > Hi Neil, > > Are you suggesting to add a new binding to handle the > 'enforce-uncompressed-mode'? In my opinion: please add new property next to the existing one. > > -Paloma > >> >>> >>> Thanks, >>> >>> Jessica Zhang >>> >>> [1] https://patchwork.freedesktop.org/patch/517483/?series=112369&rev=2 >>> >>>> >>>>> >>>>> ctx->supplies[0].supply = "vddio"; >>>>> ctx->supplies[1].supply = "vci"; >>>>> @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct >>>>> mipi_dsi_device *dsi) >>>>> >>>>> dsi->lanes = 4; >>>>> dsi->format = MIPI_DSI_FMT_RGB888; >>>>> - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >>>>> MIPI_DSI_MODE_NO_EOT_PACKET | >>>>> - MIPI_DSI_CLOCK_NON_CONTINUOUS; >>>>> + >>>>> + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | >>>>> MIPI_DSI_CLOCK_NON_CONTINUOUS; >>>> >>>> Keep the line split please. >>>> >>>>> + if (ctx->video_mode) >>>>> + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; >>>>> + >>>>> ctx->panel.prepare_prev_first = true; >>>>> >>>>> drm_panel_init(&ctx->panel, dev, >>>>> &visionox_vtdr6130_panel_funcs, >>>>> -- >>>>> 2.41.0 >>>>> >>>> >>>> >>>> -- >>>> With best wishes >>>> Dmitry >>
On 01/08/2023 22:46, Dmitry Baryshkov wrote: > On 01/08/2023 23:43, Paloma Arellano wrote: >> >> On 8/1/2023 1:26 AM, neil.armstrong@linaro.org wrote: >>> On 28/07/2023 23:44, Jessica Zhang wrote: >>>> >>>> >>>> On 7/28/2023 2:37 AM, Dmitry Baryshkov wrote: >>>>> On Fri, 28 Jul 2023 at 04:26, Paloma Arellano <quic_parellan@quicinc.com> wrote: >>>>>> >>>>>> Enable display compression (DSC v1.2) and CMD mode for 1080x2400 Visionox >>>>>> VTDR6130 AMOLED DSI panel. In addition, this patch will set the default >>>>>> to command mode with DSC enabled. >>>>>> >>>>>> Note: This patch has only been validated DSC over command mode as DSC over >>>>>> video mode has never been validated for the MSM driver before. >>>>>> >>>>>> Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] >>>>>> >>>>>> Changes since v1: >>>>>> - Changed from email address >>>>>> >>>>>> [1] https://patchwork.freedesktop.org/series/121337/ >>>>>> >>>>>> Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> >>>>>> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >>>>>> --- >>>>>> .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 ++++++++++++++++++- >>>>>> 1 file changed, 73 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>>> index e1363e128e7e..5658d39a3a6b 100644 >>>>>> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>>> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >>>>>> @@ -9,6 +9,7 @@ >>>>>> #include <linux/of.h> >>>>>> >>>>>> #include <drm/display/drm_dsc.h> >>>>>> +#include <drm/display/drm_dsc_helper.h> >>>>>> #include <drm/drm_mipi_dsi.h> >>>>>> #include <drm/drm_modes.h> >>>>>> #include <drm/drm_panel.h> >>>>>> @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { >>>>>> struct mipi_dsi_device *dsi; >>>>>> struct gpio_desc *reset_gpio; >>>>>> struct regulator_bulk_data supplies[3]; >>>>>> - bool prepared; >>>>>> + bool prepared, enabled; >>>>>> + bool video_mode; >>>>>> }; >>>>>> >>>>>> static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct drm_panel *panel) >>>>>> @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct visionox_vtdr6130 *ctx) >>>>>> if (ret) >>>>>> return ret; >>>>>> >>>>>> + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); >>>>>> mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20); >>>>>> mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00); >>>>>> mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); >>>>>> mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); >>>>>> mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); >>>>>> - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>>>>> + >>>>>> + if (ctx->video_mode) >>>>>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >>>>>> + else >>>>>> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); >>>>>> + >>>>>> mipi_dsi_dcs_write_seq(dsi, 0x70, >>>>>> 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, 0x09, 0x60, 0x04, >>>>>> 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, 0x1c, 0x02, 0x00, >>>>>> @@ -214,6 +222,42 @@ static const struct drm_display_mode visionox_vtdr6130_mode = { >>>>>> .height_mm = 157, >>>>>> }; >>>>>> >>>>>> +static int visionox_vtdr6130_enable(struct drm_panel *panel) >>>>>> +{ >>>>>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>>>>> + struct mipi_dsi_device *dsi = ctx->dsi; >>>>>> + struct drm_dsc_picture_parameter_set pps; >>>>>> + int ret; >>>>>> + >>>>>> + if (ctx->enabled) >>>>>> + return 0; >>>>>> + >>>>>> + if (!dsi->dsc) { >>>>>> + dev_err(&dsi->dev, "DSC not attached to DSI\n"); >>>>>> + return -ENODEV; >>>>>> + } >>>>> >>>>> The error message is misleading. Also, if you don't want to enable DSC >>>>> for the video mode, this will break. >>>>> >>>>>> + >>>>>> + drm_dsc_pps_payload_pack(&pps, dsi->dsc); >>>>>> + ret = mipi_dsi_picture_parameter_set(dsi, &pps); >>>>>> + if (ret) { >>>>>> + dev_err(&dsi->dev, "Failed to set PPS\n"); >>>>>> + return ret; >>>>>> + } >>>>>> + >>>>>> + ctx->enabled = true; >>>>> >>>>> Do we need this refcount just for PPS upload? What will happen if PPS >>>>> is uploaded several times? >>>>> >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> +static int visionox_vtdr6130_disable(struct drm_panel *panel) >>>>>> +{ >>>>>> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >>>>>> + >>>>>> + ctx->enabled = false; >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> static int visionox_vtdr6130_get_modes(struct drm_panel *panel, >>>>>> struct drm_connector *connector) >>>>>> { >>>>>> @@ -237,6 +281,8 @@ static const struct drm_panel_funcs visionox_vtdr6130_panel_funcs = { >>>>>> .prepare = visionox_vtdr6130_prepare, >>>>>> .unprepare = visionox_vtdr6130_unprepare, >>>>>> .get_modes = visionox_vtdr6130_get_modes, >>>>>> + .enable = visionox_vtdr6130_enable, >>>>>> + .disable = visionox_vtdr6130_disable, >>>>>> }; >>>>>> >>>>>> static int visionox_vtdr6130_bl_update_status(struct backlight_device *bl) >>>>>> @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) >>>>>> { >>>>>> struct device *dev = &dsi->dev; >>>>>> struct visionox_vtdr6130 *ctx; >>>>>> + struct drm_dsc_config *dsc; >>>>>> int ret; >>>>>> >>>>>> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >>>>>> if (!ctx) >>>>>> return -ENOMEM; >>>>>> + >>>>>> + ctx->video_mode = of_property_read_bool(dev->of_node, "enforce-video-mode"); >>>>> >>>>> Please also add a DT bindings patch. >>>>> >>>>>> + >>>>>> + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); >>>>>> + if (!dsc) >>>>>> + return -ENOMEM; >>>>> >>>>> You can add struct drm_dsc_config to struct visionox_vtdr6130 instead >>>>> of allocating it. >>>>> >>>>>> + >>>>>> + /* Set DSC params */ >>>>>> + dsc->dsc_version_major = 0x1; >>>>>> + dsc->dsc_version_minor = 0x2; >>>>>> + >>>>>> + dsc->slice_height = 40; >>>>>> + dsc->slice_width = 540; >>>>>> + dsc->slice_count = 2; >>>>>> + dsc->bits_per_component = 8; >>>>>> + dsc->bits_per_pixel = 8 << 4; >>>>>> + dsc->block_pred_enable = true; >>>>>> + >>>>>> + dsi->dsc = dsc; >>>>> >>>>> Only in command mode? >>>> >>>> Hi Dmitry, >>>> >>>> The intention of the patch wasn't to enable DSC for only command mode. >>>> >>>> We didn't want to limit DSC to only command mode because, while the MSM DPU driver isn't able to validate DSC on video mode, other vendors might have already validated DSC on video mode and would benefit from this patch. >>>> >>>> FWIW, inital driver commit [1] notes that the panel is meant to work with compressed streams in general and DSC support was tob be added later on. >>> >>> The panel supports Video, Video+DSC, CMD, CMD+DSC, so it would be great to be able to >>> select any of the supported modes, including the non-compressed ones. >>> >>> So enforce-video-mode is great, but an enforce-uncompressed-mode would be necessary >>> aswell. >>> >>> Neil >> >> Hi Neil, >> >> Are you suggesting to add a new binding to handle the 'enforce-uncompressed-mode'? > > In my opinion: please add new property next to the existing one. Yes please Thanks, Neil > >> >> -Paloma >> >>> >>>> >>>> Thanks, >>>> >>>> Jessica Zhang >>>> >>>> [1] https://patchwork.freedesktop.org/patch/517483/?series=112369&rev=2 >>>> >>>>> >>>>>> >>>>>> ctx->supplies[0].supply = "vddio"; >>>>>> ctx->supplies[1].supply = "vci"; >>>>>> @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) >>>>>> >>>>>> dsi->lanes = 4; >>>>>> dsi->format = MIPI_DSI_FMT_RGB888; >>>>>> - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | >>>>>> - MIPI_DSI_CLOCK_NON_CONTINUOUS; >>>>>> + >>>>>> + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS; >>>>> >>>>> Keep the line split please. >>>>> >>>>>> + if (ctx->video_mode) >>>>>> + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; >>>>>> + >>>>>> ctx->panel.prepare_prev_first = true; >>>>>> >>>>>> drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, >>>>>> -- >>>>>> 2.41.0 >>>>>> >>>>> >>>>> >>>>> -- >>>>> With best wishes >>>>> Dmitry >>> >
Hi Paloma, On 28/07/2023 03:26, Paloma Arellano wrote: > Enable display compression (DSC v1.2) and CMD mode for 1080x2400 Visionox > VTDR6130 AMOLED DSI panel. In addition, this patch will set the default > to command mode with DSC enabled. > > Note: This patch has only been validated DSC over command mode as DSC over > video mode has never been validated for the MSM driver before. I was able to test this on the QRD8550 on top of v6.6-rc5, display works fine, but when runnning: # modetest -r -v <snip> setting mode 1080x2400-144.00Hz on connectors 32, crtc 95 failed to set gamma: Function not implemented freq: 74.22Hz freq: 73.09Hz freq: 72.48Hz We get a correct 144Hz vsync test in video mode. Do you know why this happens ? Neil > > Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] > > Changes since v1: > - Changed from email address > > [1] https://patchwork.freedesktop.org/series/121337/ > > Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> > Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> > --- > .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 ++++++++++++++++++- > 1 file changed, 73 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > index e1363e128e7e..5658d39a3a6b 100644 > --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c > @@ -9,6 +9,7 @@ > #include <linux/of.h> > > #include <drm/display/drm_dsc.h> > +#include <drm/display/drm_dsc_helper.h> > #include <drm/drm_mipi_dsi.h> > #include <drm/drm_modes.h> > #include <drm/drm_panel.h> > @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { > struct mipi_dsi_device *dsi; > struct gpio_desc *reset_gpio; > struct regulator_bulk_data supplies[3]; > - bool prepared; > + bool prepared, enabled; > + bool video_mode; > }; > > static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct drm_panel *panel) > @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct visionox_vtdr6130 *ctx) > if (ret) > return ret; > > + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); > mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20); > mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00); > mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); > mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); > mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); > - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); > + > + if (ctx->video_mode) > + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); > + else > + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); > + > mipi_dsi_dcs_write_seq(dsi, 0x70, > 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, 0x09, 0x60, 0x04, > 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, 0x1c, 0x02, 0x00, > @@ -214,6 +222,42 @@ static const struct drm_display_mode visionox_vtdr6130_mode = { > .height_mm = 157, > }; > > +static int visionox_vtdr6130_enable(struct drm_panel *panel) > +{ > + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); > + struct mipi_dsi_device *dsi = ctx->dsi; > + struct drm_dsc_picture_parameter_set pps; > + int ret; > + > + if (ctx->enabled) > + return 0; > + > + if (!dsi->dsc) { > + dev_err(&dsi->dev, "DSC not attached to DSI\n"); > + return -ENODEV; > + } > + > + drm_dsc_pps_payload_pack(&pps, dsi->dsc); > + ret = mipi_dsi_picture_parameter_set(dsi, &pps); > + if (ret) { > + dev_err(&dsi->dev, "Failed to set PPS\n"); > + return ret; > + } > + > + ctx->enabled = true; > + > + return 0; > +} > + > +static int visionox_vtdr6130_disable(struct drm_panel *panel) > +{ > + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); > + > + ctx->enabled = false; > + > + return 0; > +} > + > static int visionox_vtdr6130_get_modes(struct drm_panel *panel, > struct drm_connector *connector) > { > @@ -237,6 +281,8 @@ static const struct drm_panel_funcs visionox_vtdr6130_panel_funcs = { > .prepare = visionox_vtdr6130_prepare, > .unprepare = visionox_vtdr6130_unprepare, > .get_modes = visionox_vtdr6130_get_modes, > + .enable = visionox_vtdr6130_enable, > + .disable = visionox_vtdr6130_disable, > }; > > static int visionox_vtdr6130_bl_update_status(struct backlight_device *bl) > @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) > { > struct device *dev = &dsi->dev; > struct visionox_vtdr6130 *ctx; > + struct drm_dsc_config *dsc; > int ret; > > ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); > if (!ctx) > return -ENOMEM; > + > + ctx->video_mode = of_property_read_bool(dev->of_node, "enforce-video-mode"); > + > + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); > + if (!dsc) > + return -ENOMEM; > + > + /* Set DSC params */ > + dsc->dsc_version_major = 0x1; > + dsc->dsc_version_minor = 0x2; > + > + dsc->slice_height = 40; > + dsc->slice_width = 540; > + dsc->slice_count = 2; > + dsc->bits_per_component = 8; > + dsc->bits_per_pixel = 8 << 4; > + dsc->block_pred_enable = true; > + > + dsi->dsc = dsc; > > ctx->supplies[0].supply = "vddio"; > ctx->supplies[1].supply = "vci"; > @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) > > dsi->lanes = 4; > dsi->format = MIPI_DSI_FMT_RGB888; > - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | > - MIPI_DSI_CLOCK_NON_CONTINUOUS; > + > + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS; > + if (ctx->video_mode) > + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; > + > ctx->panel.prepare_prev_first = true; > > drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs,
On 10/9/2023 3:32 AM, neil.armstrong@linaro.org wrote: > Hi Paloma, > > On 28/07/2023 03:26, Paloma Arellano wrote: >> Enable display compression (DSC v1.2) and CMD mode for 1080x2400 Visionox >> VTDR6130 AMOLED DSI panel. In addition, this patch will set the default >> to command mode with DSC enabled. >> >> Note: This patch has only been validated DSC over command mode as DSC >> over >> video mode has never been validated for the MSM driver before. > > I was able to test this on the QRD8550 on top of v6.6-rc5, display works > fine, > but when runnning: > # modetest -r -v > <snip> > setting mode 1080x2400-144.00Hz on connectors 32, crtc 95 > failed to set gamma: Function not implemented > freq: 74.22Hz > freq: 73.09Hz > freq: 72.48Hz > > We get a correct 144Hz vsync test in video mode. > > Do you know why this happens ? Hi Neil, Thanks for reporting this. FWIW, I saw a slight drop in FPS (from 120 to 90Hz) when enabling DSC for the r66451 command mode panel though the drop reported here seems more drastic. I'll try recreating this on SM8550 MTP and investigate it. Thanks, Jessica Zhang > > Neil > >> >> Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] >> >> Changes since v1: >> - Changed from email address >> >> [1] https://patchwork.freedesktop.org/series/121337/ >> >> Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> >> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> >> --- >> .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 ++++++++++++++++++- >> 1 file changed, 73 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >> b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >> index e1363e128e7e..5658d39a3a6b 100644 >> --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >> +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c >> @@ -9,6 +9,7 @@ >> #include <linux/of.h> >> #include <drm/display/drm_dsc.h> >> +#include <drm/display/drm_dsc_helper.h> >> #include <drm/drm_mipi_dsi.h> >> #include <drm/drm_modes.h> >> #include <drm/drm_panel.h> >> @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { >> struct mipi_dsi_device *dsi; >> struct gpio_desc *reset_gpio; >> struct regulator_bulk_data supplies[3]; >> - bool prepared; >> + bool prepared, enabled; >> + bool video_mode; >> }; >> static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct >> drm_panel *panel) >> @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct >> visionox_vtdr6130 *ctx) >> if (ret) >> return ret; >> + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); >> mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20); >> mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, >> 0x00, 0x00); >> mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); >> mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); >> mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); >> - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >> + >> + if (ctx->video_mode) >> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); >> + else >> + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); >> + >> mipi_dsi_dcs_write_seq(dsi, 0x70, >> 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, 0x09, 0x60, >> 0x04, >> 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, 0x1c, 0x02, >> 0x00, >> @@ -214,6 +222,42 @@ static const struct drm_display_mode >> visionox_vtdr6130_mode = { >> .height_mm = 157, >> }; >> +static int visionox_vtdr6130_enable(struct drm_panel *panel) >> +{ >> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >> + struct mipi_dsi_device *dsi = ctx->dsi; >> + struct drm_dsc_picture_parameter_set pps; >> + int ret; >> + >> + if (ctx->enabled) >> + return 0; >> + >> + if (!dsi->dsc) { >> + dev_err(&dsi->dev, "DSC not attached to DSI\n"); >> + return -ENODEV; >> + } >> + >> + drm_dsc_pps_payload_pack(&pps, dsi->dsc); >> + ret = mipi_dsi_picture_parameter_set(dsi, &pps); >> + if (ret) { >> + dev_err(&dsi->dev, "Failed to set PPS\n"); >> + return ret; >> + } >> + >> + ctx->enabled = true; >> + >> + return 0; >> +} >> + >> +static int visionox_vtdr6130_disable(struct drm_panel *panel) >> +{ >> + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); >> + >> + ctx->enabled = false; >> + >> + return 0; >> +} >> + >> static int visionox_vtdr6130_get_modes(struct drm_panel *panel, >> struct drm_connector *connector) >> { >> @@ -237,6 +281,8 @@ static const struct drm_panel_funcs >> visionox_vtdr6130_panel_funcs = { >> .prepare = visionox_vtdr6130_prepare, >> .unprepare = visionox_vtdr6130_unprepare, >> .get_modes = visionox_vtdr6130_get_modes, >> + .enable = visionox_vtdr6130_enable, >> + .disable = visionox_vtdr6130_disable, >> }; >> static int visionox_vtdr6130_bl_update_status(struct >> backlight_device *bl) >> @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct >> mipi_dsi_device *dsi) >> { >> struct device *dev = &dsi->dev; >> struct visionox_vtdr6130 *ctx; >> + struct drm_dsc_config *dsc; >> int ret; >> ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); >> if (!ctx) >> return -ENOMEM; >> + >> + ctx->video_mode = of_property_read_bool(dev->of_node, >> "enforce-video-mode"); >> + >> + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); >> + if (!dsc) >> + return -ENOMEM; >> + >> + /* Set DSC params */ >> + dsc->dsc_version_major = 0x1; >> + dsc->dsc_version_minor = 0x2; >> + >> + dsc->slice_height = 40; >> + dsc->slice_width = 540; >> + dsc->slice_count = 2; >> + dsc->bits_per_component = 8; >> + dsc->bits_per_pixel = 8 << 4; >> + dsc->block_pred_enable = true; >> + >> + dsi->dsc = dsc; >> ctx->supplies[0].supply = "vddio"; >> ctx->supplies[1].supply = "vci"; >> @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct >> mipi_dsi_device *dsi) >> dsi->lanes = 4; >> dsi->format = MIPI_DSI_FMT_RGB888; >> - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | >> MIPI_DSI_MODE_NO_EOT_PACKET | >> - MIPI_DSI_CLOCK_NON_CONTINUOUS; >> + >> + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | >> MIPI_DSI_CLOCK_NON_CONTINUOUS; >> + if (ctx->video_mode) >> + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; >> + >> ctx->panel.prepare_prev_first = true; >> drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs, >
diff --git a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c index e1363e128e7e..5658d39a3a6b 100644 --- a/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c +++ b/drivers/gpu/drm/panel/panel-visionox-vtdr6130.c @@ -9,6 +9,7 @@ #include <linux/of.h> #include <drm/display/drm_dsc.h> +#include <drm/display/drm_dsc_helper.h> #include <drm/drm_mipi_dsi.h> #include <drm/drm_modes.h> #include <drm/drm_panel.h> @@ -20,7 +21,8 @@ struct visionox_vtdr6130 { struct mipi_dsi_device *dsi; struct gpio_desc *reset_gpio; struct regulator_bulk_data supplies[3]; - bool prepared; + bool prepared, enabled; + bool video_mode; }; static inline struct visionox_vtdr6130 *to_visionox_vtdr6130(struct drm_panel *panel) @@ -50,12 +52,18 @@ static int visionox_vtdr6130_on(struct visionox_vtdr6130 *ctx) if (ret) return ret; + mipi_dsi_dcs_write_seq(dsi, 0x03, 0x01); mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_WRITE_CONTROL_DISPLAY, 0x20); mipi_dsi_dcs_write_seq(dsi, MIPI_DCS_SET_DISPLAY_BRIGHTNESS, 0x00, 0x00); mipi_dsi_dcs_write_seq(dsi, 0x59, 0x09); mipi_dsi_dcs_write_seq(dsi, 0x6c, 0x01); mipi_dsi_dcs_write_seq(dsi, 0x6d, 0x00); - mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); + + if (ctx->video_mode) + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x01); + else + mipi_dsi_dcs_write_seq(dsi, 0x6f, 0x02); + mipi_dsi_dcs_write_seq(dsi, 0x70, 0x12, 0x00, 0x00, 0xab, 0x30, 0x80, 0x09, 0x60, 0x04, 0x38, 0x00, 0x28, 0x02, 0x1c, 0x02, 0x1c, 0x02, 0x00, @@ -214,6 +222,42 @@ static const struct drm_display_mode visionox_vtdr6130_mode = { .height_mm = 157, }; +static int visionox_vtdr6130_enable(struct drm_panel *panel) +{ + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); + struct mipi_dsi_device *dsi = ctx->dsi; + struct drm_dsc_picture_parameter_set pps; + int ret; + + if (ctx->enabled) + return 0; + + if (!dsi->dsc) { + dev_err(&dsi->dev, "DSC not attached to DSI\n"); + return -ENODEV; + } + + drm_dsc_pps_payload_pack(&pps, dsi->dsc); + ret = mipi_dsi_picture_parameter_set(dsi, &pps); + if (ret) { + dev_err(&dsi->dev, "Failed to set PPS\n"); + return ret; + } + + ctx->enabled = true; + + return 0; +} + +static int visionox_vtdr6130_disable(struct drm_panel *panel) +{ + struct visionox_vtdr6130 *ctx = to_visionox_vtdr6130(panel); + + ctx->enabled = false; + + return 0; +} + static int visionox_vtdr6130_get_modes(struct drm_panel *panel, struct drm_connector *connector) { @@ -237,6 +281,8 @@ static const struct drm_panel_funcs visionox_vtdr6130_panel_funcs = { .prepare = visionox_vtdr6130_prepare, .unprepare = visionox_vtdr6130_unprepare, .get_modes = visionox_vtdr6130_get_modes, + .enable = visionox_vtdr6130_enable, + .disable = visionox_vtdr6130_disable, }; static int visionox_vtdr6130_bl_update_status(struct backlight_device *bl) @@ -269,11 +315,31 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) { struct device *dev = &dsi->dev; struct visionox_vtdr6130 *ctx; + struct drm_dsc_config *dsc; int ret; ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL); if (!ctx) return -ENOMEM; + + ctx->video_mode = of_property_read_bool(dev->of_node, "enforce-video-mode"); + + dsc = devm_kzalloc(dev, sizeof(*dsc), GFP_KERNEL); + if (!dsc) + return -ENOMEM; + + /* Set DSC params */ + dsc->dsc_version_major = 0x1; + dsc->dsc_version_minor = 0x2; + + dsc->slice_height = 40; + dsc->slice_width = 540; + dsc->slice_count = 2; + dsc->bits_per_component = 8; + dsc->bits_per_pixel = 8 << 4; + dsc->block_pred_enable = true; + + dsi->dsc = dsc; ctx->supplies[0].supply = "vddio"; ctx->supplies[1].supply = "vci"; @@ -294,8 +360,11 @@ static int visionox_vtdr6130_probe(struct mipi_dsi_device *dsi) dsi->lanes = 4; dsi->format = MIPI_DSI_FMT_RGB888; - dsi->mode_flags = MIPI_DSI_MODE_VIDEO | MIPI_DSI_MODE_NO_EOT_PACKET | - MIPI_DSI_CLOCK_NON_CONTINUOUS; + + dsi->mode_flags = MIPI_DSI_MODE_NO_EOT_PACKET | MIPI_DSI_CLOCK_NON_CONTINUOUS; + if (ctx->video_mode) + dsi->mode_flags |= MIPI_DSI_MODE_VIDEO; + ctx->panel.prepare_prev_first = true; drm_panel_init(&ctx->panel, dev, &visionox_vtdr6130_panel_funcs,
Enable display compression (DSC v1.2) and CMD mode for 1080x2400 Visionox VTDR6130 AMOLED DSI panel. In addition, this patch will set the default to command mode with DSC enabled. Note: This patch has only been validated DSC over command mode as DSC over video mode has never been validated for the MSM driver before. Depends on: "Add prepare_prev_first flag to Visionox VTDR6130" [1] Changes since v1: - Changed from email address [1] https://patchwork.freedesktop.org/series/121337/ Suggested-by: Jessica Zhang <quic_jesszhan@quicinc.com> Signed-off-by: Paloma Arellano <quic_parellan@quicinc.com> --- .../gpu/drm/panel/panel-visionox-vtdr6130.c | 77 ++++++++++++++++++- 1 file changed, 73 insertions(+), 4 deletions(-)