[PATCH v2] drm/panel: Enable DSC and CMD mode for Visionox VTDR6130 panel
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Tue Aug 1 20:46:25 UTC 2023
On 01/08/2023 23:43, Paloma Arellano wrote:
>
> 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'?
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
>>
--
With best wishes
Dmitry
More information about the dri-devel
mailing list