[v3,06/13] drm: bridge: samsung-dsim: Add DSI init in bridge pre_enable()

Message ID 20220720155210.365977-7-jagan@amarulasolutions.com
State New
Headers show
Series
  • drm: bridge: Add Samsung MIPI DSIM bridge
Related show

Commit Message

Jagan Teki July 20, 2022, 3:52 p.m. UTC
Host transfer() in DSI master will invoke only when the DSI commands
are sent from DSI devices like DSI Panel or DSI bridges and this
host transfer wouldn't invoke for I2C-based-DSI bridge drivers.

Handling DSI host initialization in transfer calls misses the
controller setup for I2C configured DSI bridges.

This patch adds the DSI initialization from transfer to bridge
pre_enable as the bridge pre_enable API is invoked by core as
it is common across all classes of DSI device drivers.

v3:
* none

v2:
* check initialized state in samsung_dsim_init

v1:
* keep DSI init in host transfer

Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
---
 drivers/gpu/drm/bridge/samsung-dsim.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

Comments

Marek Szyprowski July 22, 2022, 3:35 p.m. UTC | #1
On 20.07.2022 17:52, Jagan Teki wrote:
> Host transfer() in DSI master will invoke only when the DSI commands
> are sent from DSI devices like DSI Panel or DSI bridges and this
> host transfer wouldn't invoke for I2C-based-DSI bridge drivers.
>
> Handling DSI host initialization in transfer calls misses the
> controller setup for I2C configured DSI bridges.
>
> This patch adds the DSI initialization from transfer to bridge
> pre_enable as the bridge pre_enable API is invoked by core as
> it is common across all classes of DSI device drivers.

This is still problematic in case of Exynos. Without a workaround like this

https://github.com/mszyprow/linux/commit/11bbfc61272da9610dd5c574bb8ef838dc150961

the display on the all real DSI panels on my Exynos based boards is broken.


>
> v3:
> * none
>
> v2:
> * check initialized state in samsung_dsim_init
>
> v1:
> * keep DSI init in host transfer
>
> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> ---
>   drivers/gpu/drm/bridge/samsung-dsim.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> index 9b74a3f98a17..b07909a52f2d 100644
> --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> @@ -1258,6 +1258,9 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
>   {
>   	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
>   
> +	if (dsi->state & DSIM_STATE_INITIALIZED)
> +		return 0;
> +
>   	samsung_dsim_reset(dsi);
>   	samsung_dsim_enable_irq(dsi);
>   
> @@ -1270,6 +1273,8 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
>   	samsung_dsim_set_phy_ctrl(dsi);
>   	samsung_dsim_init_link(dsi);
>   
> +	dsi->state |= DSIM_STATE_INITIALIZED;
> +
>   	return 0;
>   }
>   
> @@ -1289,6 +1294,10 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
>   	}
>   
>   	dsi->state |= DSIM_STATE_ENABLED;
> +
> +	ret = samsung_dsim_init(dsi);
> +	if (ret)
> +		return;
>   }
>   
>   static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
> @@ -1464,12 +1473,9 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
>   	if (!(dsi->state & DSIM_STATE_ENABLED))
>   		return -EINVAL;
>   
> -	if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
> -		ret = samsung_dsim_init(dsi);
> -		if (ret)
> -			return ret;
> -		dsi->state |= DSIM_STATE_INITIALIZED;
> -	}
> +	ret = samsung_dsim_init(dsi);
> +	if (ret)
> +		return ret;
>   
>   	ret = mipi_dsi_create_packet(&xfer.packet, msg);
>   	if (ret < 0)

Best regards
Dave Stevenson July 22, 2022, 4:05 p.m. UTC | #2
Hi Jagan and Marek.

On Fri, 22 Jul 2022 at 16:35, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
>
> On 20.07.2022 17:52, Jagan Teki wrote:
> > Host transfer() in DSI master will invoke only when the DSI commands
> > are sent from DSI devices like DSI Panel or DSI bridges and this
> > host transfer wouldn't invoke for I2C-based-DSI bridge drivers.
> >
> > Handling DSI host initialization in transfer calls misses the
> > controller setup for I2C configured DSI bridges.
> >
> > This patch adds the DSI initialization from transfer to bridge
> > pre_enable as the bridge pre_enable API is invoked by core as
> > it is common across all classes of DSI device drivers.
>
> This is still problematic in case of Exynos. Without a workaround like this
>
> https://github.com/mszyprow/linux/commit/11bbfc61272da9610dd5c574bb8ef838dc150961
>
> the display on the all real DSI panels on my Exynos based boards is broken.

I'd queried on the other thread trying to address DSI operation [1] as
to whether the test for STOP_STATE (presumably LP-11) at [2] was
actually valid, but had no response.
There is no need to check for bus contention at that point, but should
it happen the driver doesn't write the registers in lines 862-868
having returned -EFAULT at line 853. The controller is therefore only
partially initialised.

I may be misinterpreting what that code is waiting for though, or the
hardware may require some further state before it can be initialised.

  Dave

[1] https://www.mail-archive.com/dri-devel@lists.freedesktop.org/msg397703.html
[2] https://github.com/mszyprow/linux/blob/11bbfc61272da9610dd5c574bb8ef838dc150961/drivers/gpu/drm/bridge/samsung-dsim.c#L850

> >
> > v3:
> > * none
> >
> > v2:
> > * check initialized state in samsung_dsim_init
> >
> > v1:
> > * keep DSI init in host transfer
> >
> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com>
> > ---
> >   drivers/gpu/drm/bridge/samsung-dsim.c | 18 ++++++++++++------
> >   1 file changed, 12 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
> > index 9b74a3f98a17..b07909a52f2d 100644
> > --- a/drivers/gpu/drm/bridge/samsung-dsim.c
> > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c
> > @@ -1258,6 +1258,9 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
> >   {
> >       const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
> >
> > +     if (dsi->state & DSIM_STATE_INITIALIZED)
> > +             return 0;
> > +
> >       samsung_dsim_reset(dsi);
> >       samsung_dsim_enable_irq(dsi);
> >
> > @@ -1270,6 +1273,8 @@ static int samsung_dsim_init(struct samsung_dsim *dsi)
> >       samsung_dsim_set_phy_ctrl(dsi);
> >       samsung_dsim_init_link(dsi);
> >
> > +     dsi->state |= DSIM_STATE_INITIALIZED;
> > +
> >       return 0;
> >   }
> >
> > @@ -1289,6 +1294,10 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
> >       }
> >
> >       dsi->state |= DSIM_STATE_ENABLED;
> > +
> > +     ret = samsung_dsim_init(dsi);
> > +     if (ret)
> > +             return;
> >   }
> >
> >   static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
> > @@ -1464,12 +1473,9 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
> >       if (!(dsi->state & DSIM_STATE_ENABLED))
> >               return -EINVAL;
> >
> > -     if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
> > -             ret = samsung_dsim_init(dsi);
> > -             if (ret)
> > -                     return ret;
> > -             dsi->state |= DSIM_STATE_INITIALIZED;
> > -     }
> > +     ret = samsung_dsim_init(dsi);
> > +     if (ret)
> > +             return ret;
> >
> >       ret = mipi_dsi_create_packet(&xfer.packet, msg);
> >       if (ret < 0)
>
> Best regards
> --
> Marek Szyprowski, PhD
> Samsung R&D Institute Poland
>
Jagan Teki Aug. 29, 2022, 6:30 p.m. UTC | #3
Hi Dave,

On Fri, Jul 22, 2022 at 9:35 PM Dave Stevenson
<dave.stevenson@raspberrypi.com> wrote:
>
> Hi Jagan and Marek.
>
> On Fri, 22 Jul 2022 at 16:35, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> >
> > On 20.07.2022 17:52, Jagan Teki wrote:
> > > Host transfer() in DSI master will invoke only when the DSI commands
> > > are sent from DSI devices like DSI Panel or DSI bridges and this
> > > host transfer wouldn't invoke for I2C-based-DSI bridge drivers.
> > >
> > > Handling DSI host initialization in transfer calls misses the
> > > controller setup for I2C configured DSI bridges.
> > >
> > > This patch adds the DSI initialization from transfer to bridge
> > > pre_enable as the bridge pre_enable API is invoked by core as
> > > it is common across all classes of DSI device drivers.
> >
> > This is still problematic in case of Exynos. Without a workaround like this
> >
> > https://github.com/mszyprow/linux/commit/11bbfc61272da9610dd5c574bb8ef838dc150961
> >
> > the display on the all real DSI panels on my Exynos based boards is broken.
>
> I'd queried on the other thread trying to address DSI operation [1] as
> to whether the test for STOP_STATE (presumably LP-11) at [2] was
> actually valid, but had no response.
> There is no need to check for bus contention at that point, but should
> it happen the driver doesn't write the registers in lines 862-868
> having returned -EFAULT at line 853. The controller is therefore only
> partially initialised.

Can you link me if you have any updated series on this? or the
existing one is the latest one itself?

Thanks,
Jagan.
Dave Stevenson Aug. 30, 2022, 2:24 p.m. UTC | #4
Hi Jagan

On Mon, 29 Aug 2022 at 19:31, Jagan Teki <jagan@amarulasolutions.com> wrote:
>
> Hi Dave,
>
> On Fri, Jul 22, 2022 at 9:35 PM Dave Stevenson
> <dave.stevenson@raspberrypi.com> wrote:
> >
> > Hi Jagan and Marek.
> >
> > On Fri, 22 Jul 2022 at 16:35, Marek Szyprowski <m.szyprowski@samsung.com> wrote:
> > >
> > > On 20.07.2022 17:52, Jagan Teki wrote:
> > > > Host transfer() in DSI master will invoke only when the DSI commands
> > > > are sent from DSI devices like DSI Panel or DSI bridges and this
> > > > host transfer wouldn't invoke for I2C-based-DSI bridge drivers.
> > > >
> > > > Handling DSI host initialization in transfer calls misses the
> > > > controller setup for I2C configured DSI bridges.
> > > >
> > > > This patch adds the DSI initialization from transfer to bridge
> > > > pre_enable as the bridge pre_enable API is invoked by core as
> > > > it is common across all classes of DSI device drivers.
> > >
> > > This is still problematic in case of Exynos. Without a workaround like this
> > >
> > > https://github.com/mszyprow/linux/commit/11bbfc61272da9610dd5c574bb8ef838dc150961
> > >
> > > the display on the all real DSI panels on my Exynos based boards is broken.
> >
> > I'd queried on the other thread trying to address DSI operation [1] as
> > to whether the test for STOP_STATE (presumably LP-11) at [2] was
> > actually valid, but had no response.
> > There is no need to check for bus contention at that point, but should
> > it happen the driver doesn't write the registers in lines 862-868
> > having returned -EFAULT at line 853. The controller is therefore only
> > partially initialised.
>
> Can you link me if you have any updated series on this? or the
> existing one is the latest one itself?

I've not updated my patch set as I didn't think there had been any
significant review comments to action -
https://patchwork.freedesktop.org/series/100252/ is still the latest.

Sam had suggested changing upstream to prev/next, but seeing as no one
else has expressed a view on that I didn't see much point in
respinning. If others agree with Sam, then I'll do it.

Checking I do note that the suggested change to drop
drm_bridge_chain_* API has been done by Sam. I don't see that having
been merged, but once it is patch 1 needs to be dropped / reworked.

I don't have any Exynos hardware, so can't really help out on the DSI
issues there other than making suggestions by inspection of the code.

  Dave

> Thanks,
> Jagan.

Patch

diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c
index 9b74a3f98a17..b07909a52f2d 100644
--- a/drivers/gpu/drm/bridge/samsung-dsim.c
+++ b/drivers/gpu/drm/bridge/samsung-dsim.c
@@ -1258,6 +1258,9 @@  static int samsung_dsim_init(struct samsung_dsim *dsi)
 {
 	const struct samsung_dsim_driver_data *driver_data = dsi->driver_data;
 
+	if (dsi->state & DSIM_STATE_INITIALIZED)
+		return 0;
+
 	samsung_dsim_reset(dsi);
 	samsung_dsim_enable_irq(dsi);
 
@@ -1270,6 +1273,8 @@  static int samsung_dsim_init(struct samsung_dsim *dsi)
 	samsung_dsim_set_phy_ctrl(dsi);
 	samsung_dsim_init_link(dsi);
 
+	dsi->state |= DSIM_STATE_INITIALIZED;
+
 	return 0;
 }
 
@@ -1289,6 +1294,10 @@  static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge,
 	}
 
 	dsi->state |= DSIM_STATE_ENABLED;
+
+	ret = samsung_dsim_init(dsi);
+	if (ret)
+		return;
 }
 
 static void samsung_dsim_atomic_enable(struct drm_bridge *bridge,
@@ -1464,12 +1473,9 @@  static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host,
 	if (!(dsi->state & DSIM_STATE_ENABLED))
 		return -EINVAL;
 
-	if (!(dsi->state & DSIM_STATE_INITIALIZED)) {
-		ret = samsung_dsim_init(dsi);
-		if (ret)
-			return ret;
-		dsi->state |= DSIM_STATE_INITIALIZED;
-	}
+	ret = samsung_dsim_init(dsi);
+	if (ret)
+		return ret;
 
 	ret = mipi_dsi_create_packet(&xfer.packet, msg);
 	if (ret < 0)