| 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: <20250619-slim-bright-warthog-77f8ed@houat>
Date: Thu, 19 Jun 2025 14:04:57 +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 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.
> > > + 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?
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.
> > > +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.
Maxime
Download attachment "signature.asc" of type "application/pgp-signature" (274 bytes)
Powered by blists - more mailing lists