| 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: <CAPVz0n2eDciXnLOo_cEh8Hjy=hY1NW0DNRs8K5Y7d0Ey5JGbNQ@mail.gmail.com>
Date: Thu, 19 Jun 2025 15:16:34 +0300
From: Svyatoslav Ryhel <clamor95@...il.com>
To: Maxime Ripard <mripard@...nel.org>
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
чт, 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.
> > > > + 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.
> Maxime
Powered by blists - more mailing lists