| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <fe1d5f6b-006e-494f-8d07-e6c751d4c694@tecnico.ulisboa.pt>
Date: Fri, 18 Jul 2025 11:43:20 +0100
From: Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>
To: Brigham Campbell <me@...ghamcampbell.com>, dianders@...omium.org,
tejasvipin76@...il.com, skhan@...uxfoundation.org,
linux-kernel-mentees@...ts.linux.dev, dri-devel@...ts.freedesktop.org,
linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org,
Neil Armstrong <neil.armstrong@...aro.org>,
Jessica Zhang <jessica.zhang@....qualcomm.com>,
Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>
Subject: Re: [PATCH v4 2/4] drm/panel: jdi-lpm102a188a: Fix bug and clean up
driver
On 7/17/25 5:40 PM, Brigham Campbell wrote:
> Fix bug in unprepare() which causes the function's return value to be
> that of the last mipi "enter sleep mode" command.
>
> Update driver to use the "multi" variant of MIPI functions in order to
> facilitate improved error handling and remove the panel's dependency on
> deprecated MIPI functions.
>
> Use the new mipi_dsi_dual macro to reduce code duplication.
>
> Signed-off-by: Brigham Campbell <me@...ghamcampbell.com>
Reviewed-by: Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>
Tested-by: Diogo Ivo <diogo.ivo@...nico.ulisboa.pt>
Thanks for the patch! Just the smallest of nits in the review but it's
fine by me if this goes in as is.
> ---
> drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c | 197 ++++++------------
> 1 file changed, 60 insertions(+), 137 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> index 5b5082efb282..9df67facdc47 100644
> --- a/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> +++ b/drivers/gpu/drm/panel/panel-jdi-lpm102a188a.c
> @@ -81,25 +81,25 @@ static int jdi_panel_disable(struct drm_panel *panel)
> static int jdi_panel_unprepare(struct drm_panel *panel)
> {
> struct jdi_panel *jdi = to_panel_jdi(panel);
> - int ret;
>
> - ret = mipi_dsi_dcs_set_display_off(jdi->link1);
> - if (ret < 0)
> - dev_err(panel->dev, "failed to set display off: %d\n", ret);
> + /*
> + * One context per panel since we'll continue trying to shut down the
> + * other panel even if one isn't responding.
> + */
> + struct mipi_dsi_multi_context dsi_ctx1 = { .dsi = jdi->link1 };
> + struct mipi_dsi_multi_context dsi_ctx2 = { .dsi = jdi->link2 };
>
> - ret = mipi_dsi_dcs_set_display_off(jdi->link2);
> - if (ret < 0)
> - dev_err(panel->dev, "failed to set display off: %d\n", ret);
> + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx1);
> + mipi_dsi_dcs_set_display_off_multi(&dsi_ctx2);
>
> /* Specified by JDI @ 50ms, subject to change */
> msleep(50);
>
> - ret = mipi_dsi_dcs_enter_sleep_mode(jdi->link1);
> - if (ret < 0)
> - dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
> - ret = mipi_dsi_dcs_enter_sleep_mode(jdi->link2);
> - if (ret < 0)
> - dev_err(panel->dev, "failed to enter sleep mode: %d\n", ret);
> + /* Doesn't hurt to try sleep mode even if display off fails */
> + dsi_ctx1.accum_err = 0;
> + dsi_ctx2.accum_err = 0;
> + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx1);
> + mipi_dsi_dcs_enter_sleep_mode_multi(&dsi_ctx2);
>
> /* Specified by JDI @ 150ms, subject to change */
> msleep(150);
> @@ -123,72 +123,47 @@ static int jdi_panel_unprepare(struct drm_panel *panel)
> /* Specified by JDI @ 20ms, subject to change */
> msleep(20);
>
> - return ret;
> -}
> -
> -static int jdi_setup_symmetrical_split(struct mipi_dsi_device *left,
> - struct mipi_dsi_device *right,
> - const struct drm_display_mode *mode)
> -{
> - int err;
> -
> - err = mipi_dsi_dcs_set_column_address(left, 0, mode->hdisplay / 2 - 1);
> - if (err < 0) {
> - dev_err(&left->dev, "failed to set column address: %d\n", err);
> - return err;
> - }
> -
> - err = mipi_dsi_dcs_set_column_address(right, 0, mode->hdisplay / 2 - 1);
> - if (err < 0) {
> - dev_err(&right->dev, "failed to set column address: %d\n", err);
> - return err;
> - }
> -
> - err = mipi_dsi_dcs_set_page_address(left, 0, mode->vdisplay - 1);
> - if (err < 0) {
> - dev_err(&left->dev, "failed to set page address: %d\n", err);
> - return err;
> - }
> -
> - err = mipi_dsi_dcs_set_page_address(right, 0, mode->vdisplay - 1);
> - if (err < 0) {
> - dev_err(&right->dev, "failed to set page address: %d\n", err);
> - return err;
> - }
> -
> return 0;
> }
>
> -static int jdi_write_dcdc_registers(struct jdi_panel *jdi)
> +static void jdi_setup_symmetrical_split(struct mipi_dsi_multi_context *dsi_ctx,
> + struct mipi_dsi_device *left,
> + struct mipi_dsi_device *right,
> + const struct drm_display_mode *mode)
> +{
> + mipi_dsi_dual(mipi_dsi_dcs_set_column_address_multi,
> + left, right, dsi_ctx,
> + 0, mode->hdisplay / 2 - 1);
> + mipi_dsi_dual(mipi_dsi_dcs_set_page_address_multi,
> + left, right, dsi_ctx,
> + 0, mode->vdisplay - 1);
> +}
> +
> +static void jdi_write_dcdc_registers(struct mipi_dsi_multi_context *dsi_ctx,
> + struct jdi_panel *jdi)
> {
> /* Clear the manufacturer command access protection */
> - mipi_dsi_generic_write_seq(jdi->link1, MCS_CMD_ACS_PROT,
> - MCS_CMD_ACS_PROT_OFF);
> - mipi_dsi_generic_write_seq(jdi->link2, MCS_CMD_ACS_PROT,
> - MCS_CMD_ACS_PROT_OFF);
> + mipi_dsi_dual(mipi_dsi_generic_write_seq_multi,
> + jdi->link1, jdi->link2, dsi_ctx,
> + MCS_CMD_ACS_PROT, MCS_CMD_ACS_PROT_OFF);
> /*
> - * Change the VGH/VGL divide rations to move the noise generated by the
> + * Change the VGH/VGL divide ratios to move the noise generated by the
> * TCONN. This should hopefully avoid interaction with the backlight
> * controller.
> */
> - mipi_dsi_generic_write_seq(jdi->link1, MCS_PWR_CTRL_FUNC,
> - MCS_PWR_CTRL_PARAM1_VGH_330_DIV |
> - MCS_PWR_CTRL_PARAM1_DEFAULT,
> - MCS_PWR_CTRL_PARAM2_VGL_410_DIV |
> - MCS_PWR_CTRL_PARAM2_DEFAULT);
> -
> - mipi_dsi_generic_write_seq(jdi->link2, MCS_PWR_CTRL_FUNC,
> - MCS_PWR_CTRL_PARAM1_VGH_330_DIV |
> - MCS_PWR_CTRL_PARAM1_DEFAULT,
> - MCS_PWR_CTRL_PARAM2_VGL_410_DIV |
> - MCS_PWR_CTRL_PARAM2_DEFAULT);
> -
> - return 0;
> + mipi_dsi_dual(mipi_dsi_generic_write_seq_multi,
> + jdi->link1, jdi->link2, dsi_ctx,
> + MCS_PWR_CTRL_FUNC,
> + MCS_PWR_CTRL_PARAM1_VGH_330_DIV |
> + MCS_PWR_CTRL_PARAM1_DEFAULT,
> + MCS_PWR_CTRL_PARAM2_VGL_410_DIV |
> + MCS_PWR_CTRL_PARAM2_DEFAULT);
> }
>
> static int jdi_panel_prepare(struct drm_panel *panel)
> {
> struct jdi_panel *jdi = to_panel_jdi(panel);
> + struct mipi_dsi_multi_context dsi_ctx = { .accum_err = 0 };
Technically this should be in reverse christmas tree order but it is
just a tiny nitpick.
> int err;
>
> /* Disable backlight to avoid showing random pixels
> @@ -231,88 +206,36 @@ static int jdi_panel_prepare(struct drm_panel *panel)
> * put in place to communicate the configuration back to the DSI host
> * controller.
> */
> - err = jdi_setup_symmetrical_split(jdi->link1, jdi->link2,
> - jdi->mode);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to set up symmetrical split: %d\n",
> - err);
> - goto poweroff;
> - }
> + jdi_setup_symmetrical_split(&dsi_ctx, jdi->link1, jdi->link2,
> + jdi->mode);
>
> - err = mipi_dsi_dcs_set_tear_scanline(jdi->link1,
> - jdi->mode->vdisplay - 16);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to set tear scanline: %d\n", err);
> - goto poweroff;
> - }
> + mipi_dsi_dual(mipi_dsi_dcs_set_tear_scanline_multi,
> + jdi->link1, jdi->link2, &dsi_ctx,
> + jdi->mode->vdisplay - 16);
>
> - err = mipi_dsi_dcs_set_tear_scanline(jdi->link2,
> - jdi->mode->vdisplay - 16);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to set tear scanline: %d\n", err);
> - goto poweroff;
> - }
> + mipi_dsi_dual(mipi_dsi_dcs_set_tear_on_multi,
> + jdi->link1, jdi->link2, &dsi_ctx,
> + MIPI_DSI_DCS_TEAR_MODE_VBLANK);
>
> - err = mipi_dsi_dcs_set_tear_on(jdi->link1,
> - MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to set tear on: %d\n", err);
> - goto poweroff;
> - }
> + mipi_dsi_dual(mipi_dsi_dcs_set_pixel_format_multi,
> + jdi->link1, jdi->link2, &dsi_ctx,
> + MIPI_DCS_PIXEL_FMT_24BIT);
>
> - err = mipi_dsi_dcs_set_tear_on(jdi->link2,
> - MIPI_DSI_DCS_TEAR_MODE_VBLANK);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to set tear on: %d\n", err);
> - goto poweroff;
> - }
> + mipi_dsi_dual(mipi_dsi_dcs_exit_sleep_mode_multi,
> + jdi->link1, jdi->link2, &dsi_ctx);
>
> - err = mipi_dsi_dcs_set_pixel_format(jdi->link1, MIPI_DCS_PIXEL_FMT_24BIT);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to set pixel format: %d\n", err);
> - goto poweroff;
> - }
> -
> - err = mipi_dsi_dcs_set_pixel_format(jdi->link2, MIPI_DCS_PIXEL_FMT_24BIT);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to set pixel format: %d\n", err);
> - goto poweroff;
> - }
> -
> - err = mipi_dsi_dcs_exit_sleep_mode(jdi->link1);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
> - goto poweroff;
> - }
> -
> - err = mipi_dsi_dcs_exit_sleep_mode(jdi->link2);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to exit sleep mode: %d\n", err);
> - goto poweroff;
> - }
> -
> - err = jdi_write_dcdc_registers(jdi);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to write dcdc registers: %d\n", err);
> - goto poweroff;
> - }
> + jdi_write_dcdc_registers(&dsi_ctx, jdi);
> /*
> - * We need to wait 150ms between mipi_dsi_dcs_exit_sleep_mode() and
> - * mipi_dsi_dcs_set_display_on().
> + * We need to wait 150ms between mipi_dsi_dcs_exit_sleep_mode_multi()
> + * and mipi_dsi_dcs_set_display_on_multi().
> */
> - msleep(150);
> + mipi_dsi_msleep(&dsi_ctx, 150);
>
> - err = mipi_dsi_dcs_set_display_on(jdi->link1);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to set display on: %d\n", err);
> - goto poweroff;
> - }
> + mipi_dsi_dual(mipi_dsi_dcs_set_display_on_multi,
> + jdi->link1, jdi->link2, &dsi_ctx);
>
> - err = mipi_dsi_dcs_set_display_on(jdi->link2);
> - if (err < 0) {
> - dev_err(panel->dev, "failed to set display on: %d\n", err);
> + if (dsi_ctx.accum_err < 0)
> goto poweroff;
> - }
>
> jdi->link1->mode_flags &= ~MIPI_DSI_MODE_LPM;
> jdi->link2->mode_flags &= ~MIPI_DSI_MODE_LPM;
Powered by blists - more mailing lists