[PATCH v2] drm/panel: Enable DSC and CMD mode for Visionox VTDR6130 panel
Paloma Arellano
quic_parellan at quicinc.com
Tue Aug 1 20:43:00 UTC 2023
On 8/1/2023 1:26 AM, neil.armstrong at 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 at 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 at quicinc.com>
>>>> Signed-off-by: Paloma Arellano <quic_parellan at 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
>
More information about the dri-devel
mailing list