On Thu, Sep 29, 2022 at 06:05:00PM +0100, Diogo Ivo wrote:
> In cases where the DSI module is left on by the bootloader
> some panels may fail to initialize if the enable register is not cleared
> before the panel's initialization sequence. Clear it and add an optional
> device tree property to inform the driver if this is the case.
>
> Signed-off-by: Diogo Ivo <diogo.ivo@xxxxxxxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/tegra/dsi.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/dsi.c b/drivers/gpu/drm/tegra/dsi.c
> index de1333dc0d86..f66514379913 100644
> --- a/drivers/gpu/drm/tegra/dsi.c
> +++ b/drivers/gpu/drm/tegra/dsi.c
> @@ -78,6 +78,8 @@ struct tegra_dsi {
> unsigned int video_fifo_depth;
> unsigned int host_fifo_depth;
>
> + bool enabled;
There should be no need to track this. DRM/KMS internally already knows,
so we should make use of the built-in mechanisms as much as possible.
> +
> /* for ganged-mode support */
> struct tegra_dsi *master;
> struct tegra_dsi *slave;
> @@ -460,6 +462,8 @@ static void tegra_dsi_enable(struct tegra_dsi *dsi)
> value |= DSI_POWER_CONTROL_ENABLE;
> tegra_dsi_writel(dsi, value, DSI_POWER_CONTROL);
>
> + dsi->enabled = true;
> +
> if (dsi->slave)
> tegra_dsi_enable(dsi->slave);
> }
> @@ -737,6 +741,8 @@ static void tegra_dsi_disable(struct tegra_dsi *dsi)
> value &= ~DSI_POWER_CONTROL_ENABLE;
> tegra_dsi_writel(dsi, value, DSI_POWER_CONTROL);
>
> + dsi->enabled = false;
> +
> if (dsi->slave)
> tegra_dsi_disable(dsi->slave);
>
> @@ -912,6 +918,27 @@ static void tegra_dsi_encoder_enable(struct drm_encoder *encoder)
> u32 value;
> int err;
>
> + /* If the bootloader enabled DSI it needs to be disabled
> + * in order for the panel initialization commands to be
> + * properly sent.
> + */
> + if (dsi->enabled) {
> + if (dc) {
> + value = tegra_dc_readl(dc, DC_DISP_DISP_WIN_OPTIONS);
> + value &= ~DSI_ENABLE;
> + tegra_dc_writel(dc, value, DC_DISP_DISP_WIN_OPTIONS);
> +
> + tegra_dc_commit(dc);
> + }
> +
> + err = tegra_dsi_wait_idle(dsi, 100);
> + if (err < 0)
> + dev_dbg(dsi->dev, "failed to idle DSI: %d\n", err);
> +
> + /* disable DSI controller */
> + tegra_dsi_disable(dsi);
> + }
This is suboptimal because it is largely a duplication of what we
already have in tegra_dsi_disable(). Also, see below.
> +
> err = tegra_dsi_prepare(dsi);
> if (err < 0) {
> dev_err(dsi->dev, "failed to prepare: %d\n", err);
> @@ -1573,6 +1600,8 @@ static int tegra_dsi_probe(struct platform_device *pdev)
>
> dsi->output.connector.polled = DRM_CONNECTOR_POLL_HPD;
>
> + /* Check if the DSI module was left on by bootloader. */
> + dsi->enabled = of_property_read_bool(pdev->dev.of_node, "nvidia,boot-on");
The isn't a documented property. But before you go and add this, are
there no alternative ways to detect that the DSI controller is active?
Could we not read one of the registers to find out?
DRM/KMS has built-in mechanisms to read back hardware state on boot, so
I wonder if we can hook that up. It'd make the most sense if all sub-
drivers did this, because then we could eventually inherit the
bootloader configuration and transition to the kernel display driver
seamlessly, but doing this in DSI first may help prepare for that more
extended use-case.
A slightly simpler alternative would be to add the reset code to the
encoder's or connector's ->reset() implementation. This is called at the
right time (i.e. when the mode configuration is first reset), so you can
run the workaround from tegra_dsi_encoder_enable() there. That's better
than having this guarded by the dsi->enabled flag so that it is run only
once.
Thierry
Attachment:
signature.asc
Description: PGP signature
