чт, 19 черв. 2025 р. о 16:13 Maxime Ripard <[email protected]> пише: > > On Thu, Jun 19, 2025 at 03:16:34PM +0300, Svyatoslav Ryhel wrote: > > чт, 19 черв. 2025 р. о 15:05 Maxime Ripard <[email protected]> пише: > > > > > > On Thu, Jun 19, 2025 at 01:17:12PM +0300, Svyatoslav Ryhel wrote: > > > > чт, 19 черв. 2025 р. о 12:41 Maxime Ripard <[email protected]> пише: > > > > > > > > > > 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? >
I tried to address issues that you have pointed to here in v7 version of this patch set. https://lore.kernel.org/lkml/[email protected]/ > Maxime
