Message-ID: <20250619-wondrous-illegal-marten-a365a7@houat>
Date: Thu, 19 Jun 2025 15:13:34 +0200
From: Maxime Ripard <mripard@...nel.org>
To: Svyatoslav Ryhel <clamor95@...il.com>
Cc: Andrzej Hajda <andrzej.hajda@...el.com>, 
	Neil Armstrong <neil.armstrong@...aro.org>, Robert Foss <rfoss@...nel.org>, 
	Laurent Pinchart <Laurent.pinchart@...asonboard.com>, Jonas Karlman <jonas@...boo.se>, 
	Jernej Skrabec <jernej.skrabec@...il.com>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>, 
	Thomas Zimmermann <tzimmermann@...e.de>, David Airlie <airlied@...il.com>, 
	Simona Vetter <simona@...ll.ch>, Rob Herring <robh@...nel.org>, 
	Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, 
	dri-devel@...ts.freedesktop.org, devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 2/2] drm: bridge: Add support for Solomon SSD2825
 RGB/DSI bridge

On Thu, Jun 19, 2025 at 03:16:34PM +0300, Svyatoslav Ryhel wrote:
> чт, 19 черв. 2025 р. о 15:05 Maxime Ripard <mripard@...nel.org> пише:
> >
> > On Thu, Jun 19, 2025 at 01:17:12PM +0300, Svyatoslav Ryhel wrote:
> > > чт, 19 черв. 2025 р. о 12:41 Maxime Ripard <mripard@...nel.org> пише:
> > > >
> > > > On Mon, May 26, 2025 at 02:43:53PM +0300, Svyatoslav Ryhel wrote:
> > > > > +static ssize_t ssd2825_dsi_host_transfer(struct mipi_dsi_host *host,
> > > > > +                                      const struct mipi_dsi_msg *msg)
> > > > > +{
> > > > > +     struct ssd2825_priv *priv = dsi_host_to_ssd2825(host);
> > > > > +     struct mipi_dsi_device *dsi_dev = priv->output.dev;
> > > > > +     u8 buf = *(u8 *)msg->tx_buf;
> > > > > +     u16 config;
> > > > > +     int ret;
> > > > > +
> > > > > +     if (!priv->enabled) {
> > > > > +             dev_err(priv->dev, "Bridge is not enabled\n");
> > > > > +             return -ENODEV;
> > > > > +     }
> > > >
> > > > Transfers can and should happen even when the bridge is disabled. The
> > > > hardware might not permit that, but you'll need to elaborate in the
> > > > comment about why.
> > >
> > > This ensures that hw was configured properly in pre_enable and since
> > > pre_enable is void it will not return any errors if it fails.
> >
> > There's no relationship between the bridge pre_enable and enable hooks,
> > and the MIPI-DSI host transfer one. It's perfectly valid to call
> > transfer if the bridge is detached or disabled.
>
> That is twisted logic, but ok, fine, I don't care.

It's not twisted. It's perfectly valid for a panel to read its revision
at probe time, before it registers, for example.

It would happen before the panel is enabled, and before the bridge is
enabled.

> > > > > +     if (msg->rx_len) {
> > > > > +             dev_warn(priv->dev, "MIPI rx is not supported\n");
> > > > > +             return -EOPNOTSUPP;
> > > > > +     }
> > > > > +
> > > > > +     guard(mutex)(&priv->mlock);
> > > > > +
> > > > > +     ret = ssd2825_read_reg(priv, SSD2825_CONFIGURATION_REG, &config);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     switch (msg->type) {
> > > > > +     case MIPI_DSI_DCS_SHORT_WRITE:
> > > > > +     case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> > > > > +     case MIPI_DSI_DCS_LONG_WRITE:
> > > > > +             config |= SSD2825_CONF_REG_DCS;
> > > > > +             break;
> > > > > +     case MIPI_DSI_GENERIC_SHORT_WRITE_0_PARAM:
> > > > > +     case MIPI_DSI_GENERIC_SHORT_WRITE_1_PARAM:
> > > > > +     case MIPI_DSI_GENERIC_SHORT_WRITE_2_PARAM:
> > > > > +     case MIPI_DSI_GENERIC_LONG_WRITE:
> > > > > +             config &= ~SSD2825_CONF_REG_DCS;
> > > > > +             break;
> > > > > +     case MIPI_DSI_DCS_READ:
> > > > > +     case MIPI_DSI_GENERIC_READ_REQUEST_0_PARAM:
> > > > > +     case MIPI_DSI_GENERIC_READ_REQUEST_1_PARAM:
> > > > > +     case MIPI_DSI_GENERIC_READ_REQUEST_2_PARAM:
> > > > > +     default:
> > > > > +             return 0;
> > > > > +     }
> > > > > +
> > > > > +     ret = ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG, config);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     ret = ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0x0000);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     ret = ssd2825_write_dsi(priv, msg->tx_buf, msg->tx_len);
> > > > > +     if (ret)
> > > > > +             return ret;
> > > > > +
> > > > > +     if (buf == MIPI_DCS_SET_DISPLAY_ON) {
> > > > > +             /*
> > > > > +              * NOTE! This is here since it cannot be called in bridge enable because
> > > > > +              * bridge pre enable and bridge enable have no gap in between.
> > > > > +              *
> > > > > +              * Existing framework bridge-panel seq is:
> > > > > +              *      panel_prepare > bridge_pre_enable > bridge_enable > panel_enable
> > > > > +              *
> > > > > +              * Using prepare_prev_first was tested, but it switches seq like this:
> > > > > +              *      bridge_pre_enable > panel_prepare > bridge_enable > panel_enable
> > > > > +              *
> > > > > +              * This will not work since panel hw MUST be prepared before bridge is
> > > > > +              * configured. Correct seq should be:
> > > > > +              *      panel_prepare > bridge_pre_enable > panel_enable > bridge_enable
> > > >
> > > > Where is that requirement coming from?
> > >
> > > This is how my device's (LG P895) bridge-panel combo works. Panel hw
> > > must be enabled before bridge, then bridge hw, then panel can send
> > > init sequence and then bridge must complete configuration.
> >
> > Do you have a documentation for that DSI device?
> >
> 
> No
> 
> > DSI devices typically come with requirement of the power states of the
> > lanes, that's what you want to discuss here. How we can model that in
> > software is a discussion we need to have once we've identified what the
> > hardware needs exactly.
> >
> > > > panel prepare is documented as:
> > > >
> > > >   The .prepare() function is typically called before the display controller
> > > >   starts to transmit video data.
> > > >
> > > >
> > > > And video data transmission for bridges only happen at bridge_enable
> > > > time.
> > > >
> > > > So, from an API PoV, all the sequences above are correct.
> > >
> > > There is no way ATM for this bridge to complete configuration, there
> > > either should be a way to swap panel_enable and bridge_enable or there
> > > should be added an additional operation like bridge_post_enable or
> > > smth like that for cases like here when bridge has to complete
> > > configuration after panel init seq is sent.
> > >
> > > > > +              * Last two functions should be swapped related to existing framework.
> > > > > +              * I am not aware about method which allows that.
> > > > > +              *
> > > > > +              * Once there will be such method/flag, code below should be moved into
> > > > > +              * bridge_enable since it is basically a bridge configuration completing
> > > > > +              * after initial panel DSI sequence is completed.
> > > > > +              */
> > > >
> > > > If there's anything to fix, we should do it before introducing that
> > > > driver.
> > >
> > > I just want to have a bridge my device uses to be supported by
> > > mainline linux. I have no intention to touch any part of DRM framework
> > > and cause instabilities, maintainers rage and hate.
> >
> > And I just want all drivers to behave consistently.
> >
> 
> Ye, sure.
> 
> > > > > +static void ssd2825_bridge_atomic_pre_enable(struct drm_bridge *bridge,
> > > > > +                                          struct drm_atomic_state *state)
> > > > > +{
> > > > > +     struct ssd2825_priv *priv = bridge_to_ssd2825(bridge);
> > > > > +     struct mipi_dsi_device *dsi_dev = priv->output.dev;
> > > > > +     const struct drm_crtc_state *crtc_state;
> > > > > +     const struct drm_display_mode *mode;
> > > > > +     struct drm_connector *connector;
> > > > > +     struct drm_crtc *crtc;
> > > > > +     u32 input_bus_flags = bridge->timings->input_bus_flags;
> > > > > +     u16 flags = 0, config;
> > > > > +     u8 pixel_format;
> > > > > +     int ret;
> > > > > +
> > > > > +     if (priv->enabled)
> > > > > +             return;
> > > >
> > > > What is this guarding against?
> > >
> > > blocks repeating ssd2825_bridge_atomic_pre_enable calls
> >
> > Which happens in which situation?
> >
> > > > > +     /* Power Sequence */
> > > > > +     ret = clk_prepare_enable(priv->tx_clk);
> > > > > +     if (ret)
> > > > > +             dev_err(priv->dev, "error enabling tx_clk (%d)\n", ret);
> > > > > +
> > > > > +     ret = regulator_bulk_enable(ARRAY_SIZE(ssd2825_supplies), priv->supplies);
> > > > > +     if (ret)
> > > > > +             dev_err(priv->dev, "error enabling regulators (%d)\n", ret);
> > > > > +
> > > > > +     usleep_range(1000, 2000);
> > > > > +
> > > > > +     ssd2825_hw_reset(priv);
> > > > > +
> > > > > +     /* Perform SW reset */
> > > > > +     ssd2825_write_reg(priv, SSD2825_OPERATION_CTRL_REG, 0x0100);
> > > > > +
> > > > > +     /* Set pixel format */
> > > > > +     switch (dsi_dev->format) {
> > > > > +     case MIPI_DSI_FMT_RGB565:
> > > > > +             pixel_format = 0x00;
> > > > > +             break;
> > > > > +     case MIPI_DSI_FMT_RGB666_PACKED:
> > > > > +             pixel_format = 0x01;
> > > > > +             break;
> > > > > +     case MIPI_DSI_FMT_RGB666:
> > > > > +             pixel_format = 0x02;
> > > > > +             break;
> > > > > +     case MIPI_DSI_FMT_RGB888:
> > > > > +     default:
> > > > > +             pixel_format = 0x03;
> > > > > +             break;
> > > > > +     }
> > > > > +
> > > > > +     connector = drm_atomic_get_new_connector_for_encoder(state, bridge->encoder);
> > > > > +     crtc = drm_atomic_get_new_connector_state(state, connector)->crtc;
> > > > > +     crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> > > > > +     mode = &crtc_state->adjusted_mode;
> > > > > +
> > > > > +     /* Set panel timings */
> > > > > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_1,
> > > > > +                       ((mode->vtotal - mode->vsync_end) << 8) |
> > > > > +                       (mode->htotal - mode->hsync_end));
> > > > > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_2,
> > > > > +                       ((mode->vtotal - mode->vsync_start) << 8) |
> > > > > +                       (mode->htotal - mode->hsync_start));
> > > > > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_3,
> > > > > +                       ((mode->vsync_start - mode->vdisplay) << 8) |
> > > > > +                       (mode->hsync_start - mode->hdisplay));
> > > > > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_4, mode->hdisplay);
> > > > > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_5, mode->vdisplay);
> > > > > +
> > > > > +     if (mode->flags & DRM_MODE_FLAG_PHSYNC)
> > > > > +             flags |= SSD2825_HSYNC_HIGH;
> > > > > +
> > > > > +     if (mode->flags & DRM_MODE_FLAG_PVSYNC)
> > > > > +             flags |= SSD2825_VSYNC_HIGH;
> > > > > +
> > > > > +     if (dsi_dev->mode_flags & MIPI_DSI_MODE_VIDEO)
> > > > > +             flags |= SSD2825_NON_BURST_EV;
> > > > > +
> > > > > +     if (input_bus_flags & DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE)
> > > > > +             flags |= SSD2825_PCKL_HIGH;
> > > > > +
> > > > > +     ssd2825_write_reg(priv, SSD2825_RGB_INTERFACE_CTRL_REG_6, flags | pixel_format);
> > > > > +     ssd2825_write_reg(priv, SSD2825_LANE_CONFIGURATION_REG, dsi_dev->lanes - 1);
> > > > > +     ssd2825_write_reg(priv, SSD2825_TEST_REG, 0x0004);
> > > > > +
> > > > > +     /* Call PLL configuration */
> > > > > +     ssd2825_setup_pll(priv, mode);
> > > > > +
> > > > > +     usleep_range(10000, 11000);
> > > > > +
> > > > > +     config = SSD2825_CONF_REG_HS | SSD2825_CONF_REG_CKE | SSD2825_CONF_REG_DCS |
> > > > > +              SSD2825_CONF_REG_ECD | SSD2825_CONF_REG_EOT;
> > > > > +
> > > > > +     if (dsi_dev->mode_flags & MIPI_DSI_MODE_LPM)
> > > > > +             config &= ~SSD2825_CONF_REG_HS;
> > > > > +
> > > > > +     if (dsi_dev->mode_flags & MIPI_DSI_MODE_NO_EOT_PACKET)
> > > > > +             config &= ~SSD2825_CONF_REG_EOT;
> > > > > +
> > > > > +     /* Initial DSI configuration register set */
> > > > > +     ssd2825_write_reg(priv, SSD2825_CONFIGURATION_REG, config);
> > > > > +     ssd2825_write_reg(priv, SSD2825_VC_CTRL_REG, 0);
> > > > > +
> > > > > +     priv->enabled = true;
> > > > > +}
> > > > > +
> > > > > +static void ssd2825_bridge_atomic_enable(struct drm_bridge *bridge,
> > > > > +                                      struct drm_atomic_state *state)
> > > > > +{
> > > > > +     /* placeholder */
> > > > > +}
> > > >
> > > > That doesn't work with any bridge or panel that doesn't require any DCS
> > > > command to power up, unfortunately.
> > >
> > > Yes that is a flaw unfortunately, if you have suggestions of fixing
> > > this just tell me.
> >
> > Untangle pre_enable and enable from transfer, and in enable actually
> > enable the bridge, and it will work just fine.
> >
> 
> No it will not, I have tried and panel fails cause panel hw is init in
> pre-enable and init sequence is sent in enable, which is expected
> logic. Yet bridge cannot complete configuration because panel enable
> function is called AFTER bridge enable.

Then I guess we're at a dead-end now, aren't we?

Maxime

Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)

Powered by blists - more mailing lists