| Message ID | 20221209152343.180139-11-jagan@amarulasolutions.com |
|---|---|
| State | New |
| Headers | show |
| Series | drm: bridge: Add Samsung MIPI DSIM bridge | expand |
Hi Jagan, On 09.12.2022 16:23, Jagan Teki wrote: > The existing drm panels and bridges in Exynos required host > initialization during the first DSI command transfer even though > the initialization was done before. > > This host reinitialization is handled via DSIM_STATE_REINITIALIZED > flag and triggers from host transfer. > > Do this exclusively for Exynos. > > Initial logic is derived from Marek Szyprowski changes. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > Changes from v9: > - derived from v8 > - added comments > > drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- > include/drm/bridge/samsung-dsim.h | 5 +++-- > 2 files changed, 17 insertions(+), 3 deletions(-) The following chunk is missing compared to v8: diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index 6e9ad955ebd3..6a9403cb92ae 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim *dsi, unsigned int flag) return 0; samsung_dsim_reset(dsi); - samsung_dsim_enable_irq(dsi); + + if (!(dsi->state & DSIM_STATE_INITIALIZED)) + samsung_dsim_enable_irq(dsi); if (driver_data->reg_values[RESET_TYPE] == DSIM_FUNCRST) samsung_dsim_enable_lane(dsi, BIT(dsi->lanes) - 1); > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c > index 2e15d753fdd0..ec3ab679afd9 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -1254,6 +1254,19 @@ static int samsung_dsim_init(struct samsung_dsim *dsi, unsigned int flag) > { > const struct samsung_dsim_driver_data *driver_data = dsi->driver_data; > > + /* > + * FIXME: > + * The existing drm panels and bridges in Exynos required host > + * initialization during the first DSI command transfer even though > + * the initialization was done before. > + * > + * This host reinitialization is handled via DSIM_STATE_REINITIALIZED > + * flag and triggers from host transfer. Do this exclusively for Exynos. > + */ > + if ((dsi->plat_data->hw_type == SAMSUNG_DSIM_TYPE_IMX8MM) && > + dsi->state & DSIM_STATE_REINITIALIZED) > + return 0; > + > if (dsi->state & flag) > return 0; > > @@ -1467,7 +1480,7 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host, > if (!(dsi->state & DSIM_STATE_ENABLED)) > return -EINVAL; > > - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > + ret = samsung_dsim_init(dsi, DSIM_STATE_REINITIALIZED); > if (ret) > return ret; > > diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h > index b8132bf8e36f..0c5a905f3de7 100644 > --- a/include/drm/bridge/samsung-dsim.h > +++ b/include/drm/bridge/samsung-dsim.h > @@ -17,8 +17,9 @@ struct samsung_dsim; > > #define DSIM_STATE_ENABLED BIT(0) > #define DSIM_STATE_INITIALIZED BIT(1) > -#define DSIM_STATE_CMD_LPM BIT(2) > -#define DSIM_STATE_VIDOUT_AVAILABLE BIT(3) > +#define DSIM_STATE_REINITIALIZED BIT(2) > +#define DSIM_STATE_CMD_LPM BIT(3) > +#define DSIM_STATE_VIDOUT_AVAILABLE BIT(4) > > enum samsung_dsim_type { > SAMSUNG_DSIM_TYPE_EXYNOS3250, Best regards
On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hi Jagan, > > On 09.12.2022 16:23, Jagan Teki wrote: > > The existing drm panels and bridges in Exynos required host > > initialization during the first DSI command transfer even though > > the initialization was done before. > > > > This host reinitialization is handled via DSIM_STATE_REINITIALIZED > > flag and triggers from host transfer. > > > > Do this exclusively for Exynos. > > > > Initial logic is derived from Marek Szyprowski changes. > > > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > > --- > > Changes from v9: > > - derived from v8 > > - added comments > > > > drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- > > include/drm/bridge/samsung-dsim.h | 5 +++-- > > 2 files changed, 17 insertions(+), 3 deletions(-) > > The following chunk is missing compared to v8: > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > b/drivers/gpu/drm/bridge/samsung-dsim.c > index 6e9ad955ebd3..6a9403cb92ae 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim > *dsi, unsigned int flag) > return 0; > > samsung_dsim_reset(dsi); > - samsung_dsim_enable_irq(dsi); > + > + if (!(dsi->state & DSIM_STATE_INITIALIZED)) > + samsung_dsim_enable_irq(dsi); Is this really required? does it make sure that the IRQ does not enable twice? Jagan.
On 12.12.2022 09:32, Jagan Teki wrote: > On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> Hi Jagan, >> >> On 09.12.2022 16:23, Jagan Teki wrote: >>> The existing drm panels and bridges in Exynos required host >>> initialization during the first DSI command transfer even though >>> the initialization was done before. >>> >>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED >>> flag and triggers from host transfer. >>> >>> Do this exclusively for Exynos. >>> >>> Initial logic is derived from Marek Szyprowski changes. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>> --- >>> Changes from v9: >>> - derived from v8 >>> - added comments >>> >>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- >>> include/drm/bridge/samsung-dsim.h | 5 +++-- >>> 2 files changed, 17 insertions(+), 3 deletions(-) >> The following chunk is missing compared to v8: >> >> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >> b/drivers/gpu/drm/bridge/samsung-dsim.c >> index 6e9ad955ebd3..6a9403cb92ae 100644 >> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim >> *dsi, unsigned int flag) >> return 0; >> >> samsung_dsim_reset(dsi); >> - samsung_dsim_enable_irq(dsi); >> + >> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) >> + samsung_dsim_enable_irq(dsi); > Is this really required? does it make sure that the IRQ does not enable twice? That's what that check does. Without the 'if (!(dsi->state & DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first from pre_enable, then from the first transfer), what leads to a warning from irq core. Best regards
On 12.12.2022 09:43, Marek Szyprowski wrote: > On 12.12.2022 09:32, Jagan Teki wrote: >> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski >> <m.szyprowski@samsung.com> wrote: >>> Hi Jagan, >>> >>> On 09.12.2022 16:23, Jagan Teki wrote: >>>> The existing drm panels and bridges in Exynos required host >>>> initialization during the first DSI command transfer even though >>>> the initialization was done before. >>>> >>>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED >>>> flag and triggers from host transfer. >>>> >>>> Do this exclusively for Exynos. >>>> >>>> Initial logic is derived from Marek Szyprowski changes. >>>> >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>>> --- >>>> Changes from v9: >>>> - derived from v8 >>>> - added comments >>>> >>>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- >>>> include/drm/bridge/samsung-dsim.h | 5 +++-- >>>> 2 files changed, 17 insertions(+), 3 deletions(-) >>> The following chunk is missing compared to v8: >>> >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>> index 6e9ad955ebd3..6a9403cb92ae 100644 >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim >>> *dsi, unsigned int flag) >>> return 0; >>> >>> samsung_dsim_reset(dsi); >>> - samsung_dsim_enable_irq(dsi); >>> + >>> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) >>> + samsung_dsim_enable_irq(dsi); >> Is this really required? does it make sure that the IRQ does not >> enable twice? > > That's what that check does. Without the 'if (!(dsi->state & > DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first > from pre_enable, then from the first transfer), what leads to a > warning from irq core. I've just noticed that we also would need to clear the DSIM_STATE_REINITIALIZED flag in dsim_suspend. However I've found that a bit simpler patch would keep the current code flow for Exynos instead of this reinitialization hack. This can be applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host init in pre_enable" patch: diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index 0b2e52585485..acc95c61ae45 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -1291,9 +1291,11 @@ static void samsung_dsim_atomic_pre_enable(struct drm_bridge *bridge, dsi->state |= DSIM_STATE_ENABLED; - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); - if (ret) - return; + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); + if (ret) + return; + } } static void samsung_dsim_atomic_enable(struct drm_bridge *bridge, diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h index b8132bf8e36f..b4e26de88b9e 100644 --- a/include/drm/bridge/samsung-dsim.h +++ b/include/drm/bridge/samsung-dsim.h @@ -30,6 +30,9 @@ enum samsung_dsim_type { SAMSUNG_DSIM_TYPE_COUNT, }; +#define samsung_dsim_hw_is_exynos(hw) ((hw) >= SAMSUNG_DSIM_TYPE_EXYNOS3250 && \ + (hw) <= SAMSUNG_DSIM_TYPE_EXYNOS5433) + struct samsung_dsim_transfer { struct list_head list; struct completion completed; Best regards
On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 12.12.2022 09:43, Marek Szyprowski wrote: > > On 12.12.2022 09:32, Jagan Teki wrote: > >> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski > >> <m.szyprowski@samsung.com> wrote: > >>> Hi Jagan, > >>> > >>> On 09.12.2022 16:23, Jagan Teki wrote: > >>>> The existing drm panels and bridges in Exynos required host > >>>> initialization during the first DSI command transfer even though > >>>> the initialization was done before. > >>>> > >>>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED > >>>> flag and triggers from host transfer. > >>>> > >>>> Do this exclusively for Exynos. > >>>> > >>>> Initial logic is derived from Marek Szyprowski changes. > >>>> > >>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > >>>> --- > >>>> Changes from v9: > >>>> - derived from v8 > >>>> - added comments > >>>> > >>>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- > >>>> include/drm/bridge/samsung-dsim.h | 5 +++-- > >>>> 2 files changed, 17 insertions(+), 3 deletions(-) > >>> The following chunk is missing compared to v8: > >>> > >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >>> b/drivers/gpu/drm/bridge/samsung-dsim.c > >>> index 6e9ad955ebd3..6a9403cb92ae 100644 > >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim > >>> *dsi, unsigned int flag) > >>> return 0; > >>> > >>> samsung_dsim_reset(dsi); > >>> - samsung_dsim_enable_irq(dsi); > >>> + > >>> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) > >>> + samsung_dsim_enable_irq(dsi); > >> Is this really required? does it make sure that the IRQ does not > >> enable twice? > > > > That's what that check does. Without the 'if (!(dsi->state & > > DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first > > from pre_enable, then from the first transfer), what leads to a > > warning from irq core. > > I've just noticed that we also would need to clear the > DSIM_STATE_REINITIALIZED flag in dsim_suspend. > > However I've found that a bit simpler patch would keep the current code > flow for Exynos instead of this reinitialization hack. This can be > applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host > init in pre_enable" patch: > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > b/drivers/gpu/drm/bridge/samsung-dsim.c > index 0b2e52585485..acc95c61ae45 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -1291,9 +1291,11 @@ static void samsung_dsim_atomic_pre_enable(struct > drm_bridge *bridge, > > dsi->state |= DSIM_STATE_ENABLED; > > - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > - if (ret) > - return; > + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { > + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > + if (ret) > + return; > + } Sorry, I don't understand this. Does it mean Exynos doesn't need to init host in pre_enable? If I remember correctly even though the host is initialized it has to reinitialize during the first transfer - This is what the Exynos requirement is. Please correct or explain here. Jagan.
Hi Marek, On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 12.12.2022 16:33, Jagan Teki wrote: > > On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: > > On 12.12.2022 09:43, Marek Szyprowski wrote: > > On 12.12.2022 09:32, Jagan Teki wrote: > > On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: > > Hi Jagan, > > On 09.12.2022 16:23, Jagan Teki wrote: > > The existing drm panels and bridges in Exynos required host > initialization during the first DSI command transfer even though > the initialization was done before. > > This host reinitialization is handled via DSIM_STATE_REINITIALIZED > flag and triggers from host transfer. > > Do this exclusively for Exynos. > > Initial logic is derived from Marek Szyprowski changes. > > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > --- > Changes from v9: > - derived from v8 > - added comments > > drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- > include/drm/bridge/samsung-dsim.h | 5 +++-- > 2 files changed, 17 insertions(+), 3 deletions(-) > > The following chunk is missing compared to v8: > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > b/drivers/gpu/drm/bridge/samsung-dsim.c > index 6e9ad955ebd3..6a9403cb92ae 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim > *dsi, unsigned int flag) > return 0; > > samsung_dsim_reset(dsi); > - samsung_dsim_enable_irq(dsi); > + > + if (!(dsi->state & DSIM_STATE_INITIALIZED)) > + samsung_dsim_enable_irq(dsi); > > Is this really required? does it make sure that the IRQ does not > enable twice? > > That's what that check does. Without the 'if (!(dsi->state & > DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first > from pre_enable, then from the first transfer), what leads to a > warning from irq core. > > I've just noticed that we also would need to clear the > DSIM_STATE_REINITIALIZED flag in dsim_suspend. > > However I've found that a bit simpler patch would keep the current code > flow for Exynos instead of this reinitialization hack. This can be > applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host > init in pre_enable" patch: > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > b/drivers/gpu/drm/bridge/samsung-dsim.c > index 0b2e52585485..acc95c61ae45 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -1291,9 +1291,11 @@ static void samsung_dsim_atomic_pre_enable(struct > drm_bridge *bridge, > > dsi->state |= DSIM_STATE_ENABLED; > > - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > - if (ret) > - return; > + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { > + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > + if (ret) > + return; > + } > > Sorry, I don't understand this. Does it mean Exynos doesn't need to > init host in pre_enable? If I remember correctly even though the host > is initialized it has to reinitialize during the first transfer - This > is what the Exynos requirement is. Please correct or explain here. > > This is a matter of enabling power regulator(s) in the right order and doing the host initialization in the right moment. It was never a matter of re-initialization. See the current code for the reference (it uses the same approach as my above change). I've already explained that here: > > https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ > > If you would like to see the exact proper moment of the dsi host initialization on the Exynos see the code here: > > https://github.com/mszyprow/linux/tree/v5.18-next-20220511-dsi-rework and patches adding mipi_dsi_host_init() to panel/bridge drivers. As I said before, the downstream bridge needs an explicit call to host init via mipi_dsi_host_init - this is indeed not a usual use-case scenario. Let's handle this with a REINIT fix and see if we can update this later to handle both scenarios. Would you please test this repo, I have included all. https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 Thanks, Jagan.
Hi Jagan,
On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote:
> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10
Please preserve the authorship of the patches.
This one is from Marek Vasut:
https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680
but in your tree, it appears as if you were the original author.
Please double-check globally.
Hi Fabio, On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote: > > Hi Jagan, > > On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > > > https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > > Please preserve the authorship of the patches. > > This one is from Marek Vasut: > https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680 The original patch was changed with respect to this one and that is the reason I have to keep his signed-off-by. Jagan.
Hi, On 13.12.2022 11:40, Jagan Teki wrote: > On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> On 12.12.2022 16:33, Jagan Teki wrote: >> >> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski >> <m.szyprowski@samsung.com> wrote: >> >> On 12.12.2022 09:43, Marek Szyprowski wrote: >> >> On 12.12.2022 09:32, Jagan Teki wrote: >> >> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski >> <m.szyprowski@samsung.com> wrote: >> >> Hi Jagan, >> >> On 09.12.2022 16:23, Jagan Teki wrote: >> >> The existing drm panels and bridges in Exynos required host >> initialization during the first DSI command transfer even though >> the initialization was done before. >> >> This host reinitialization is handled via DSIM_STATE_REINITIALIZED >> flag and triggers from host transfer. >> >> Do this exclusively for Exynos. >> >> Initial logic is derived from Marek Szyprowski changes. >> >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >> --- >> Changes from v9: >> - derived from v8 >> - added comments >> >> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- >> include/drm/bridge/samsung-dsim.h | 5 +++-- >> 2 files changed, 17 insertions(+), 3 deletions(-) >> >> The following chunk is missing compared to v8: >> >> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >> b/drivers/gpu/drm/bridge/samsung-dsim.c >> index 6e9ad955ebd3..6a9403cb92ae 100644 >> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim >> *dsi, unsigned int flag) >> return 0; >> >> samsung_dsim_reset(dsi); >> - samsung_dsim_enable_irq(dsi); >> + >> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) >> + samsung_dsim_enable_irq(dsi); >> >> Is this really required? does it make sure that the IRQ does not >> enable twice? >> >> That's what that check does. Without the 'if (!(dsi->state & >> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first >> from pre_enable, then from the first transfer), what leads to a >> warning from irq core. >> >> I've just noticed that we also would need to clear the >> DSIM_STATE_REINITIALIZED flag in dsim_suspend. >> >> However I've found that a bit simpler patch would keep the current code >> flow for Exynos instead of this reinitialization hack. This can be >> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host >> init in pre_enable" patch: >> >> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >> b/drivers/gpu/drm/bridge/samsung-dsim.c >> index 0b2e52585485..acc95c61ae45 100644 >> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >> @@ -1291,9 +1291,11 @@ static void samsung_dsim_atomic_pre_enable(struct >> drm_bridge *bridge, >> >> dsi->state |= DSIM_STATE_ENABLED; >> >> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >> - if (ret) >> - return; >> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { >> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >> + if (ret) >> + return; >> + } >> >> Sorry, I don't understand this. Does it mean Exynos doesn't need to >> init host in pre_enable? If I remember correctly even though the host >> is initialized it has to reinitialize during the first transfer - This >> is what the Exynos requirement is. Please correct or explain here. >> >> This is a matter of enabling power regulator(s) in the right order and doing the host initialization in the right moment. It was never a matter of re-initialization. See the current code for the reference (it uses the same approach as my above change). I've already explained that here: >> >> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ >> >> If you would like to see the exact proper moment of the dsi host initialization on the Exynos see the code here: >> >> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework and patches adding mipi_dsi_host_init() to panel/bridge drivers. > As I said before, the downstream bridge needs an explicit call to host > init via mipi_dsi_host_init - this is indeed not a usual use-case > scenario. Let's handle this with a REINIT fix and see if we can update > this later to handle both scenarios. > > Would you please test this repo, I have included all. > > https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 This doesn't work on TM2e board. Give me some time to find why... Best regards
On 12/13/22 11:53, Jagan Teki wrote: > Hi Fabio, Hi, > On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote: >> >> Hi Jagan, >> >> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >> >>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 >> >> Please preserve the authorship of the patches. >> >> This one is from Marek Vasut: >> https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680 > > The original patch was changed with respect to this one and that is > the reason I have to keep his signed-off-by. You did change the authorship of the patch, not just a SoB line. It seems that the only change is dropped comment, which was squashed into earlier patch in this series, see the original submission: https://patchwork.freedesktop.org/patch/507166/ btw. it seems hunk 3 has disappeared, the samsung_dsim_attach() hw_type check.
On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut <marex@denx.de> wrote: > > On 12/13/22 11:53, Jagan Teki wrote: > > Hi Fabio, > > Hi, > > > On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote: > >> > >> Hi Jagan, > >> > >> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >> > >>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > >> > >> Please preserve the authorship of the patches. > >> > >> This one is from Marek Vasut: > >> https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680 > > > > The original patch was changed with respect to this one and that is > > the reason I have to keep his signed-off-by. > > You did change the authorship of the patch, not just a SoB line. > It seems that the only change is dropped comment, which was squashed > into earlier patch in this series, see the original submission: OKay. I will update it on V10 or if you want to send it from your side then I will exclude it from the series. let me know. > > https://patchwork.freedesktop.org/patch/507166/ > > btw. it seems hunk 3 has disappeared, the samsung_dsim_attach() hw_type > check. Do you mean previous = NULL; addition? Jagan.
On 12/13/22 14:18, Jagan Teki wrote: > On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut <marex@denx.de> wrote: >> >> On 12/13/22 11:53, Jagan Teki wrote: >>> Hi Fabio, >> >> Hi, >> >>> On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote: >>>> >>>> Hi Jagan, >>>> >>>> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>> >>>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 >>>> >>>> Please preserve the authorship of the patches. >>>> >>>> This one is from Marek Vasut: >>>> https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680 >>> >>> The original patch was changed with respect to this one and that is >>> the reason I have to keep his signed-off-by. >> >> You did change the authorship of the patch, not just a SoB line. >> It seems that the only change is dropped comment, which was squashed >> into earlier patch in this series, see the original submission: > > OKay. I will update it on V10 or if you want to send it from your side > then I will exclude it from the series. let me know. Just keep the authorship intact, unless there is significant change to the patch. >> https://patchwork.freedesktop.org/patch/507166/ >> >> btw. it seems hunk 3 has disappeared, the samsung_dsim_attach() hw_type >> check. > > Do you mean previous = NULL; addition? Yes, this hunk has been dropped.
On Tue, Dec 13, 2022 at 6:51 PM Marek Vasut <marex@denx.de> wrote: > > On 12/13/22 14:18, Jagan Teki wrote: > > On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut <marex@denx.de> wrote: > >> > >> On 12/13/22 11:53, Jagan Teki wrote: > >>> Hi Fabio, > >> > >> Hi, > >> > >>> On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote: > >>>> > >>>> Hi Jagan, > >>>> > >>>> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote: > >>>> > >>>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > >>>> > >>>> Please preserve the authorship of the patches. > >>>> > >>>> This one is from Marek Vasut: > >>>> https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680 > >>> > >>> The original patch was changed with respect to this one and that is > >>> the reason I have to keep his signed-off-by. > >> > >> You did change the authorship of the patch, not just a SoB line. > >> It seems that the only change is dropped comment, which was squashed > >> into earlier patch in this series, see the original submission: > > > > OKay. I will update it on V10 or if you want to send it from your side > > then I will exclude it from the series. let me know. > > Just keep the authorship intact, unless there is significant change to > the patch. Please confirm it. https://gitlab.com/openedev/kernel/-/commit/8ce066d7fdf45e17cb1979376e70e6be353e001b > > >> https://patchwork.freedesktop.org/patch/507166/ > >> > >> btw. it seems hunk 3 has disappeared, the samsung_dsim_attach() hw_type > >> check. > > > > Do you mean previous = NULL; addition? > > Yes, this hunk has been dropped. Yes this FIXME has dropped due to Dave's changes. Jagan.
On 12/13/22 14:26, Jagan Teki wrote: > On Tue, Dec 13, 2022 at 6:51 PM Marek Vasut <marex@denx.de> wrote: >> >> On 12/13/22 14:18, Jagan Teki wrote: >>> On Tue, Dec 13, 2022 at 6:44 PM Marek Vasut <marex@denx.de> wrote: >>>> >>>> On 12/13/22 11:53, Jagan Teki wrote: >>>>> Hi Fabio, >>>> >>>> Hi, >>>> >>>>> On Tue, Dec 13, 2022 at 4:17 PM Fabio Estevam <festevam@gmail.com> wrote: >>>>>> >>>>>> Hi Jagan, >>>>>> >>>>>> On Tue, Dec 13, 2022 at 7:40 AM Jagan Teki <jagan@amarulasolutions.com> wrote: >>>>>> >>>>>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 >>>>>> >>>>>> Please preserve the authorship of the patches. >>>>>> >>>>>> This one is from Marek Vasut: >>>>>> https://gitlab.com/openedev/kernel/-/commit/e244fa552402caebcf48cd6710fd387429f7f680 >>>>> >>>>> The original patch was changed with respect to this one and that is >>>>> the reason I have to keep his signed-off-by. >>>> >>>> You did change the authorship of the patch, not just a SoB line. >>>> It seems that the only change is dropped comment, which was squashed >>>> into earlier patch in this series, see the original submission: >>> >>> OKay. I will update it on V10 or if you want to send it from your side >>> then I will exclude it from the series. let me know. >> >> Just keep the authorship intact, unless there is significant change to >> the patch. > > Please confirm it. > https://gitlab.com/openedev/kernel/-/commit/8ce066d7fdf45e17cb1979376e70e6be353e001b Seems OK. thanks >>>> https://patchwork.freedesktop.org/patch/507166/ >>>> >>>> btw. it seems hunk 3 has disappeared, the samsung_dsim_attach() hw_type >>>> check. >>> >>> Do you mean previous = NULL; addition? >> >> Yes, this hunk has been dropped. > > Yes this FIXME has dropped due to Dave's changes. OK
On Tue, Dec 13, 2022 at 5:50 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > Hi, > > On 13.12.2022 11:40, Jagan Teki wrote: > > On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski > > <m.szyprowski@samsung.com> wrote: > >> On 12.12.2022 16:33, Jagan Teki wrote: > >> > >> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski > >> <m.szyprowski@samsung.com> wrote: > >> > >> On 12.12.2022 09:43, Marek Szyprowski wrote: > >> > >> On 12.12.2022 09:32, Jagan Teki wrote: > >> > >> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski > >> <m.szyprowski@samsung.com> wrote: > >> > >> Hi Jagan, > >> > >> On 09.12.2022 16:23, Jagan Teki wrote: > >> > >> The existing drm panels and bridges in Exynos required host > >> initialization during the first DSI command transfer even though > >> the initialization was done before. > >> > >> This host reinitialization is handled via DSIM_STATE_REINITIALIZED > >> flag and triggers from host transfer. > >> > >> Do this exclusively for Exynos. > >> > >> Initial logic is derived from Marek Szyprowski changes. > >> > >> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > >> --- > >> Changes from v9: > >> - derived from v8 > >> - added comments > >> > >> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- > >> include/drm/bridge/samsung-dsim.h | 5 +++-- > >> 2 files changed, 17 insertions(+), 3 deletions(-) > >> > >> The following chunk is missing compared to v8: > >> > >> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >> b/drivers/gpu/drm/bridge/samsung-dsim.c > >> index 6e9ad955ebd3..6a9403cb92ae 100644 > >> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim > >> *dsi, unsigned int flag) > >> return 0; > >> > >> samsung_dsim_reset(dsi); > >> - samsung_dsim_enable_irq(dsi); > >> + > >> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) > >> + samsung_dsim_enable_irq(dsi); > >> > >> Is this really required? does it make sure that the IRQ does not > >> enable twice? > >> > >> That's what that check does. Without the 'if (!(dsi->state & > >> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first > >> from pre_enable, then from the first transfer), what leads to a > >> warning from irq core. > >> > >> I've just noticed that we also would need to clear the > >> DSIM_STATE_REINITIALIZED flag in dsim_suspend. > >> > >> However I've found that a bit simpler patch would keep the current code > >> flow for Exynos instead of this reinitialization hack. This can be > >> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host > >> init in pre_enable" patch: > >> > >> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >> b/drivers/gpu/drm/bridge/samsung-dsim.c > >> index 0b2e52585485..acc95c61ae45 100644 > >> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >> @@ -1291,9 +1291,11 @@ static void samsung_dsim_atomic_pre_enable(struct > >> drm_bridge *bridge, > >> > >> dsi->state |= DSIM_STATE_ENABLED; > >> > >> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > >> - if (ret) > >> - return; > >> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { > >> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > >> + if (ret) > >> + return; > >> + } > >> > >> Sorry, I don't understand this. Does it mean Exynos doesn't need to > >> init host in pre_enable? If I remember correctly even though the host > >> is initialized it has to reinitialize during the first transfer - This > >> is what the Exynos requirement is. Please correct or explain here. > >> > >> This is a matter of enabling power regulator(s) in the right order and doing the host initialization in the right moment. It was never a matter of re-initialization. See the current code for the reference (it uses the same approach as my above change). I've already explained that here: > >> > >> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ > >> > >> If you would like to see the exact proper moment of the dsi host initialization on the Exynos see the code here: > >> > >> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework and patches adding mipi_dsi_host_init() to panel/bridge drivers. > > As I said before, the downstream bridge needs an explicit call to host > > init via mipi_dsi_host_init - this is indeed not a usual use-case > > scenario. Let's handle this with a REINIT fix and see if we can update > > this later to handle both scenarios. > > > > Would you please test this repo, I have included all. > > > > https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > > This doesn't work on TM2e board. Give me some time to find why... May be some mode_flags changes in the panel driver. Jagan.
On 13.12.2022 13:20, Marek Szyprowski wrote: > On 13.12.2022 11:40, Jagan Teki wrote: >> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski >> <m.szyprowski@samsung.com> wrote: >>> On 12.12.2022 16:33, Jagan Teki wrote: >>> >>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski >>> <m.szyprowski@samsung.com> wrote: >>> >>> On 12.12.2022 09:43, Marek Szyprowski wrote: >>> >>> On 12.12.2022 09:32, Jagan Teki wrote: >>> >>> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski >>> <m.szyprowski@samsung.com> wrote: >>> >>> Hi Jagan, >>> >>> On 09.12.2022 16:23, Jagan Teki wrote: >>> >>> The existing drm panels and bridges in Exynos required host >>> initialization during the first DSI command transfer even though >>> the initialization was done before. >>> >>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED >>> flag and triggers from host transfer. >>> >>> Do this exclusively for Exynos. >>> >>> Initial logic is derived from Marek Szyprowski changes. >>> >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>> --- >>> Changes from v9: >>> - derived from v8 >>> - added comments >>> >>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- >>> include/drm/bridge/samsung-dsim.h | 5 +++-- >>> 2 files changed, 17 insertions(+), 3 deletions(-) >>> >>> The following chunk is missing compared to v8: >>> >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>> index 6e9ad955ebd3..6a9403cb92ae 100644 >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim >>> *dsi, unsigned int flag) >>> return 0; >>> >>> samsung_dsim_reset(dsi); >>> - samsung_dsim_enable_irq(dsi); >>> + >>> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) >>> + samsung_dsim_enable_irq(dsi); >>> >>> Is this really required? does it make sure that the IRQ does not >>> enable twice? >>> >>> That's what that check does. Without the 'if (!(dsi->state & >>> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first >>> from pre_enable, then from the first transfer), what leads to a >>> warning from irq core. >>> >>> I've just noticed that we also would need to clear the >>> DSIM_STATE_REINITIALIZED flag in dsim_suspend. >>> >>> However I've found that a bit simpler patch would keep the current code >>> flow for Exynos instead of this reinitialization hack. This can be >>> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host >>> init in pre_enable" patch: >>> >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>> index 0b2e52585485..acc95c61ae45 100644 >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>> @@ -1291,9 +1291,11 @@ static void >>> samsung_dsim_atomic_pre_enable(struct >>> drm_bridge *bridge, >>> >>> dsi->state |= DSIM_STATE_ENABLED; >>> >>> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >>> - if (ret) >>> - return; >>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { >>> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >>> + if (ret) >>> + return; >>> + } >>> >>> Sorry, I don't understand this. Does it mean Exynos doesn't need to >>> init host in pre_enable? If I remember correctly even though the host >>> is initialized it has to reinitialize during the first transfer - This >>> is what the Exynos requirement is. Please correct or explain here. >>> >>> This is a matter of enabling power regulator(s) in the right order >>> and doing the host initialization in the right moment. It was never >>> a matter of re-initialization. See the current code for the >>> reference (it uses the same approach as my above change). I've >>> already explained that here: >>> >>> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ >>> >>> >>> If you would like to see the exact proper moment of the dsi host >>> initialization on the Exynos see the code here: >>> >>> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework >>> and patches adding mipi_dsi_host_init() to panel/bridge drivers. >> As I said before, the downstream bridge needs an explicit call to host >> init via mipi_dsi_host_init - this is indeed not a usual use-case >> scenario. Let's handle this with a REINIT fix and see if we can update >> this later to handle both scenarios. >> >> Would you please test this repo, I have included all. >> >> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > > This doesn't work on TM2e board. Give me some time to find why... > The following change is missing in "drm: bridge: Generalize Exynos-DSI driver into a Samsung DSIM bridge" patch: diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index 1dbff2bee93f..81828b5ee0ac 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -1745,6 +1745,7 @@ int samsung_dsim_probe(struct platform_device *pdev) dsi->bridge.funcs = &samsung_dsim_bridge_funcs; dsi->bridge.of_node = dev->of_node; dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; + dsi->bridge.pre_enable_prev_first = true; /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts HS/VS/DE */ if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) After adding the above, all my test platform works fine. BTW, I still think that it is worth replacing the "drm: exynos: dsi: Add host initialization in pre_enable" patch with the following simple change and propagate it to bridge/samsung-dsim.c: diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c b/drivers/gpu/drm/exynos/exynos_drm_dsi.c index fdaf514b39f2..071b74d60dcb 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c @@ -254,6 +254,9 @@ struct exynos_dsi_transfer { #define DSIM_STATE_CMD_LPM BIT(2) #define DSIM_STATE_VIDOUT_AVAILABLE BIT(3) +#define exynos_dsi_hw_is_exynos(hw) \ + ((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433) + enum exynos_dsi_type { DSIM_TYPE_EXYNOS3250, DSIM_TYPE_EXYNOS4210, @@ -1344,6 +1347,9 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) { const struct exynos_dsi_driver_data *driver_data = dsi->driver_data; + if (dsi->state & DSIM_STATE_INITIALIZED) + return 0; + exynos_dsi_reset(dsi); exynos_dsi_enable_irq(dsi); @@ -1356,6 +1362,8 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) exynos_dsi_set_phy_ctrl(dsi); exynos_dsi_init_link(dsi); + dsi->state |= DSIM_STATE_INITIALIZED; + return 0; } @@ -1411,6 +1419,12 @@ static void exynos_dsi_atomic_pre_enable(struct drm_bridge *bridge, } dsi->state |= DSIM_STATE_ENABLED; + + if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) { + ret = exynos_dsi_init(dsi); + if (ret) + return; + } } static void exynos_dsi_atomic_enable(struct drm_bridge *bridge, @@ -1557,12 +1571,9 @@ static ssize_t exynos_dsi_host_transfer(struct mipi_dsi_host *host, if (!(dsi->state & DSIM_STATE_ENABLED)) return -EINVAL; - if (!(dsi->state & DSIM_STATE_INITIALIZED)) { - ret = exynos_dsi_init(dsi); - if (ret) - return ret; - dsi->state |= DSIM_STATE_INITIALIZED; - } + ret = exynos_dsi_init(dsi); + if (ret) + return ret; ret = mipi_dsi_create_packet(&xfer.packet, msg); if (ret < 0) Best regards
On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 13.12.2022 13:20, Marek Szyprowski wrote: > > On 13.12.2022 11:40, Jagan Teki wrote: > >> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski > >> <m.szyprowski@samsung.com> wrote: > >>> On 12.12.2022 16:33, Jagan Teki wrote: > >>> > >>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski > >>> <m.szyprowski@samsung.com> wrote: > >>> > >>> On 12.12.2022 09:43, Marek Szyprowski wrote: > >>> > >>> On 12.12.2022 09:32, Jagan Teki wrote: > >>> > >>> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski > >>> <m.szyprowski@samsung.com> wrote: > >>> > >>> Hi Jagan, > >>> > >>> On 09.12.2022 16:23, Jagan Teki wrote: > >>> > >>> The existing drm panels and bridges in Exynos required host > >>> initialization during the first DSI command transfer even though > >>> the initialization was done before. > >>> > >>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED > >>> flag and triggers from host transfer. > >>> > >>> Do this exclusively for Exynos. > >>> > >>> Initial logic is derived from Marek Szyprowski changes. > >>> > >>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > >>> --- > >>> Changes from v9: > >>> - derived from v8 > >>> - added comments > >>> > >>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- > >>> include/drm/bridge/samsung-dsim.h | 5 +++-- > >>> 2 files changed, 17 insertions(+), 3 deletions(-) > >>> > >>> The following chunk is missing compared to v8: > >>> > >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >>> b/drivers/gpu/drm/bridge/samsung-dsim.c > >>> index 6e9ad955ebd3..6a9403cb92ae 100644 > >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim > >>> *dsi, unsigned int flag) > >>> return 0; > >>> > >>> samsung_dsim_reset(dsi); > >>> - samsung_dsim_enable_irq(dsi); > >>> + > >>> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) > >>> + samsung_dsim_enable_irq(dsi); > >>> > >>> Is this really required? does it make sure that the IRQ does not > >>> enable twice? > >>> > >>> That's what that check does. Without the 'if (!(dsi->state & > >>> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first > >>> from pre_enable, then from the first transfer), what leads to a > >>> warning from irq core. > >>> > >>> I've just noticed that we also would need to clear the > >>> DSIM_STATE_REINITIALIZED flag in dsim_suspend. > >>> > >>> However I've found that a bit simpler patch would keep the current code > >>> flow for Exynos instead of this reinitialization hack. This can be > >>> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host > >>> init in pre_enable" patch: > >>> > >>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >>> b/drivers/gpu/drm/bridge/samsung-dsim.c > >>> index 0b2e52585485..acc95c61ae45 100644 > >>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >>> @@ -1291,9 +1291,11 @@ static void > >>> samsung_dsim_atomic_pre_enable(struct > >>> drm_bridge *bridge, > >>> > >>> dsi->state |= DSIM_STATE_ENABLED; > >>> > >>> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > >>> - if (ret) > >>> - return; > >>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { > >>> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > >>> + if (ret) > >>> + return; > >>> + } > >>> > >>> Sorry, I don't understand this. Does it mean Exynos doesn't need to > >>> init host in pre_enable? If I remember correctly even though the host > >>> is initialized it has to reinitialize during the first transfer - This > >>> is what the Exynos requirement is. Please correct or explain here. > >>> > >>> This is a matter of enabling power regulator(s) in the right order > >>> and doing the host initialization in the right moment. It was never > >>> a matter of re-initialization. See the current code for the > >>> reference (it uses the same approach as my above change). I've > >>> already explained that here: > >>> > >>> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ > >>> > >>> > >>> If you would like to see the exact proper moment of the dsi host > >>> initialization on the Exynos see the code here: > >>> > >>> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework > >>> and patches adding mipi_dsi_host_init() to panel/bridge drivers. > >> As I said before, the downstream bridge needs an explicit call to host > >> init via mipi_dsi_host_init - this is indeed not a usual use-case > >> scenario. Let's handle this with a REINIT fix and see if we can update > >> this later to handle both scenarios. > >> > >> Would you please test this repo, I have included all. > >> > >> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > > > > This doesn't work on TM2e board. Give me some time to find why... > > > The following change is missing in "drm: bridge: Generalize Exynos-DSI > driver into a Samsung DSIM bridge" patch: > > diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > b/drivers/gpu/drm/bridge/samsung-dsim.c > index 1dbff2bee93f..81828b5ee0ac 100644 > --- a/drivers/gpu/drm/bridge/samsung-dsim.c > +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > @@ -1745,6 +1745,7 @@ int samsung_dsim_probe(struct platform_device *pdev) > dsi->bridge.funcs = &samsung_dsim_bridge_funcs; > dsi->bridge.of_node = dev->of_node; > dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; > + dsi->bridge.pre_enable_prev_first = true; Can you check this and confirm, I keep this in exynos side. https://gitlab.com/openedev/kernel/-/commit/ccb02df7a313fdf91d8e116b0ec3d6c945fbb6fd#c93f0ce4d81b854fbde970e341fb307f1be78c16_1865_189 > > /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts > HS/VS/DE */ > if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) > > > After adding the above, all my test platform works fine. > > BTW, I still think that it is worth replacing the "drm: exynos: dsi: Add > host initialization in pre_enable" patch with the following simple > change and propagate it to bridge/samsung-dsim.c: > > diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > index fdaf514b39f2..071b74d60dcb 100644 > --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > @@ -254,6 +254,9 @@ struct exynos_dsi_transfer { > #define DSIM_STATE_CMD_LPM BIT(2) > #define DSIM_STATE_VIDOUT_AVAILABLE BIT(3) > > +#define exynos_dsi_hw_is_exynos(hw) \ > + ((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433) > + > enum exynos_dsi_type { > DSIM_TYPE_EXYNOS3250, > DSIM_TYPE_EXYNOS4210, > @@ -1344,6 +1347,9 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) > { > const struct exynos_dsi_driver_data *driver_data = > dsi->driver_data; > > + if (dsi->state & DSIM_STATE_INITIALIZED) > + return 0; > + > exynos_dsi_reset(dsi); > exynos_dsi_enable_irq(dsi); > > @@ -1356,6 +1362,8 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) > exynos_dsi_set_phy_ctrl(dsi); > exynos_dsi_init_link(dsi); > > + dsi->state |= DSIM_STATE_INITIALIZED; > + > return 0; > } > > @@ -1411,6 +1419,12 @@ static void exynos_dsi_atomic_pre_enable(struct > drm_bridge *bridge, > } > > dsi->state |= DSIM_STATE_ENABLED; > + > + if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) { > + ret = exynos_dsi_init(dsi); > + if (ret) > + return; > + } > } > > static void exynos_dsi_atomic_enable(struct drm_bridge *bridge, > @@ -1557,12 +1571,9 @@ static ssize_t exynos_dsi_host_transfer(struct > mipi_dsi_host *host, > if (!(dsi->state & DSIM_STATE_ENABLED)) > return -EINVAL; > > - if (!(dsi->state & DSIM_STATE_INITIALIZED)) { > - ret = exynos_dsi_init(dsi); > - if (ret) > - return ret; > - dsi->state |= DSIM_STATE_INITIALIZED; > - } > + ret = exynos_dsi_init(dsi); > + if (ret) > + return ret; Below patch handling similar behavior by checking exynos hw_type at exynos_dsi_init, isn't it? Please check and let me know if I missing anything. https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 Jagan.
On 13.12.2022 15:18, Jagan Teki wrote: > On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> On 13.12.2022 13:20, Marek Szyprowski wrote: >>> On 13.12.2022 11:40, Jagan Teki wrote: >>>> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski >>>> <m.szyprowski@samsung.com> wrote: >>>>> On 12.12.2022 16:33, Jagan Teki wrote: >>>>> >>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski >>>>> <m.szyprowski@samsung.com> wrote: >>>>> >>>>> On 12.12.2022 09:43, Marek Szyprowski wrote: >>>>> >>>>> On 12.12.2022 09:32, Jagan Teki wrote: >>>>> >>>>> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski >>>>> <m.szyprowski@samsung.com> wrote: >>>>> >>>>> Hi Jagan, >>>>> >>>>> On 09.12.2022 16:23, Jagan Teki wrote: >>>>> >>>>> The existing drm panels and bridges in Exynos required host >>>>> initialization during the first DSI command transfer even though >>>>> the initialization was done before. >>>>> >>>>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED >>>>> flag and triggers from host transfer. >>>>> >>>>> Do this exclusively for Exynos. >>>>> >>>>> Initial logic is derived from Marek Szyprowski changes. >>>>> >>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>>>> --- >>>>> Changes from v9: >>>>> - derived from v8 >>>>> - added comments >>>>> >>>>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- >>>>> include/drm/bridge/samsung-dsim.h | 5 +++-- >>>>> 2 files changed, 17 insertions(+), 3 deletions(-) >>>>> >>>>> The following chunk is missing compared to v8: >>>>> >>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>> index 6e9ad955ebd3..6a9403cb92ae 100644 >>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim >>>>> *dsi, unsigned int flag) >>>>> return 0; >>>>> >>>>> samsung_dsim_reset(dsi); >>>>> - samsung_dsim_enable_irq(dsi); >>>>> + >>>>> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) >>>>> + samsung_dsim_enable_irq(dsi); >>>>> >>>>> Is this really required? does it make sure that the IRQ does not >>>>> enable twice? >>>>> >>>>> That's what that check does. Without the 'if (!(dsi->state & >>>>> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first >>>>> from pre_enable, then from the first transfer), what leads to a >>>>> warning from irq core. >>>>> >>>>> I've just noticed that we also would need to clear the >>>>> DSIM_STATE_REINITIALIZED flag in dsim_suspend. >>>>> >>>>> However I've found that a bit simpler patch would keep the current code >>>>> flow for Exynos instead of this reinitialization hack. This can be >>>>> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host >>>>> init in pre_enable" patch: >>>>> >>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>> index 0b2e52585485..acc95c61ae45 100644 >>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>> @@ -1291,9 +1291,11 @@ static void >>>>> samsung_dsim_atomic_pre_enable(struct >>>>> drm_bridge *bridge, >>>>> >>>>> dsi->state |= DSIM_STATE_ENABLED; >>>>> >>>>> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >>>>> - if (ret) >>>>> - return; >>>>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { >>>>> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >>>>> + if (ret) >>>>> + return; >>>>> + } >>>>> >>>>> Sorry, I don't understand this. Does it mean Exynos doesn't need to >>>>> init host in pre_enable? If I remember correctly even though the host >>>>> is initialized it has to reinitialize during the first transfer - This >>>>> is what the Exynos requirement is. Please correct or explain here. >>>>> >>>>> This is a matter of enabling power regulator(s) in the right order >>>>> and doing the host initialization in the right moment. It was never >>>>> a matter of re-initialization. See the current code for the >>>>> reference (it uses the same approach as my above change). I've >>>>> already explained that here: >>>>> >>>>> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ >>>>> >>>>> >>>>> If you would like to see the exact proper moment of the dsi host >>>>> initialization on the Exynos see the code here: >>>>> >>>>> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework >>>>> and patches adding mipi_dsi_host_init() to panel/bridge drivers. >>>> As I said before, the downstream bridge needs an explicit call to host >>>> init via mipi_dsi_host_init - this is indeed not a usual use-case >>>> scenario. Let's handle this with a REINIT fix and see if we can update >>>> this later to handle both scenarios. >>>> >>>> Would you please test this repo, I have included all. >>>> >>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 >>> This doesn't work on TM2e board. Give me some time to find why... >>> >> The following change is missing in "drm: bridge: Generalize Exynos-DSI >> driver into a Samsung DSIM bridge" patch: >> >> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >> b/drivers/gpu/drm/bridge/samsung-dsim.c >> index 1dbff2bee93f..81828b5ee0ac 100644 >> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >> @@ -1745,6 +1745,7 @@ int samsung_dsim_probe(struct platform_device *pdev) >> dsi->bridge.funcs = &samsung_dsim_bridge_funcs; >> dsi->bridge.of_node = dev->of_node; >> dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; >> + dsi->bridge.pre_enable_prev_first = true; > Can you check this and confirm, I keep this in exynos side. > https://gitlab.com/openedev/kernel/-/commit/ccb02df7a313fdf91d8e116b0ec3d6c945fbb6fd#c93f0ce4d81b854fbde970e341fb307f1be78c16_1865_189 This one is fine! >> /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts >> HS/VS/DE */ >> if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) >> >> >> After adding the above, all my test platform works fine. >> >> BTW, I still think that it is worth replacing the "drm: exynos: dsi: Add >> host initialization in pre_enable" patch with the following simple >> change and propagate it to bridge/samsung-dsim.c: >> >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> index fdaf514b39f2..071b74d60dcb 100644 >> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >> @@ -254,6 +254,9 @@ struct exynos_dsi_transfer { >> #define DSIM_STATE_CMD_LPM BIT(2) >> #define DSIM_STATE_VIDOUT_AVAILABLE BIT(3) >> >> +#define exynos_dsi_hw_is_exynos(hw) \ >> + ((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433) >> + >> enum exynos_dsi_type { >> DSIM_TYPE_EXYNOS3250, >> DSIM_TYPE_EXYNOS4210, >> @@ -1344,6 +1347,9 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) >> { >> const struct exynos_dsi_driver_data *driver_data = >> dsi->driver_data; >> >> + if (dsi->state & DSIM_STATE_INITIALIZED) >> + return 0; >> + >> exynos_dsi_reset(dsi); >> exynos_dsi_enable_irq(dsi); >> >> @@ -1356,6 +1362,8 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) >> exynos_dsi_set_phy_ctrl(dsi); >> exynos_dsi_init_link(dsi); >> >> + dsi->state |= DSIM_STATE_INITIALIZED; >> + >> return 0; >> } >> >> @@ -1411,6 +1419,12 @@ static void exynos_dsi_atomic_pre_enable(struct >> drm_bridge *bridge, >> } >> >> dsi->state |= DSIM_STATE_ENABLED; >> + >> + if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) { >> + ret = exynos_dsi_init(dsi); >> + if (ret) >> + return; >> + } >> } >> >> static void exynos_dsi_atomic_enable(struct drm_bridge *bridge, >> @@ -1557,12 +1571,9 @@ static ssize_t exynos_dsi_host_transfer(struct >> mipi_dsi_host *host, >> if (!(dsi->state & DSIM_STATE_ENABLED)) >> return -EINVAL; >> >> - if (!(dsi->state & DSIM_STATE_INITIALIZED)) { >> - ret = exynos_dsi_init(dsi); >> - if (ret) >> - return ret; >> - dsi->state |= DSIM_STATE_INITIALIZED; >> - } >> + ret = exynos_dsi_init(dsi); >> + if (ret) >> + return ret; > Below patch handling similar behavior by checking exynos hw_type at > exynos_dsi_init, isn't it? Please check and let me know if I missing > anything. > > https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 You don't miss anything. Your version also works, but I just proposed a bit simpler code. Best regards
On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 13.12.2022 15:18, Jagan Teki wrote: > > On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski > > <m.szyprowski@samsung.com> wrote: > >> On 13.12.2022 13:20, Marek Szyprowski wrote: > >>> On 13.12.2022 11:40, Jagan Teki wrote: > >>>> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski > >>>> <m.szyprowski@samsung.com> wrote: > >>>>> On 12.12.2022 16:33, Jagan Teki wrote: > >>>>> > >>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski > >>>>> <m.szyprowski@samsung.com> wrote: > >>>>> > >>>>> On 12.12.2022 09:43, Marek Szyprowski wrote: > >>>>> > >>>>> On 12.12.2022 09:32, Jagan Teki wrote: > >>>>> > >>>>> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski > >>>>> <m.szyprowski@samsung.com> wrote: > >>>>> > >>>>> Hi Jagan, > >>>>> > >>>>> On 09.12.2022 16:23, Jagan Teki wrote: > >>>>> > >>>>> The existing drm panels and bridges in Exynos required host > >>>>> initialization during the first DSI command transfer even though > >>>>> the initialization was done before. > >>>>> > >>>>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED > >>>>> flag and triggers from host transfer. > >>>>> > >>>>> Do this exclusively for Exynos. > >>>>> > >>>>> Initial logic is derived from Marek Szyprowski changes. > >>>>> > >>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > >>>>> --- > >>>>> Changes from v9: > >>>>> - derived from v8 > >>>>> - added comments > >>>>> > >>>>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- > >>>>> include/drm/bridge/samsung-dsim.h | 5 +++-- > >>>>> 2 files changed, 17 insertions(+), 3 deletions(-) > >>>>> > >>>>> The following chunk is missing compared to v8: > >>>>> > >>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>> index 6e9ad955ebd3..6a9403cb92ae 100644 > >>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim > >>>>> *dsi, unsigned int flag) > >>>>> return 0; > >>>>> > >>>>> samsung_dsim_reset(dsi); > >>>>> - samsung_dsim_enable_irq(dsi); > >>>>> + > >>>>> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) > >>>>> + samsung_dsim_enable_irq(dsi); > >>>>> > >>>>> Is this really required? does it make sure that the IRQ does not > >>>>> enable twice? > >>>>> > >>>>> That's what that check does. Without the 'if (!(dsi->state & > >>>>> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first > >>>>> from pre_enable, then from the first transfer), what leads to a > >>>>> warning from irq core. > >>>>> > >>>>> I've just noticed that we also would need to clear the > >>>>> DSIM_STATE_REINITIALIZED flag in dsim_suspend. > >>>>> > >>>>> However I've found that a bit simpler patch would keep the current code > >>>>> flow for Exynos instead of this reinitialization hack. This can be > >>>>> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host > >>>>> init in pre_enable" patch: > >>>>> > >>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>> index 0b2e52585485..acc95c61ae45 100644 > >>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>> @@ -1291,9 +1291,11 @@ static void > >>>>> samsung_dsim_atomic_pre_enable(struct > >>>>> drm_bridge *bridge, > >>>>> > >>>>> dsi->state |= DSIM_STATE_ENABLED; > >>>>> > >>>>> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > >>>>> - if (ret) > >>>>> - return; > >>>>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { > >>>>> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > >>>>> + if (ret) > >>>>> + return; > >>>>> + } > >>>>> > >>>>> Sorry, I don't understand this. Does it mean Exynos doesn't need to > >>>>> init host in pre_enable? If I remember correctly even though the host > >>>>> is initialized it has to reinitialize during the first transfer - This > >>>>> is what the Exynos requirement is. Please correct or explain here. > >>>>> > >>>>> This is a matter of enabling power regulator(s) in the right order > >>>>> and doing the host initialization in the right moment. It was never > >>>>> a matter of re-initialization. See the current code for the > >>>>> reference (it uses the same approach as my above change). I've > >>>>> already explained that here: > >>>>> > >>>>> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ > >>>>> > >>>>> > >>>>> If you would like to see the exact proper moment of the dsi host > >>>>> initialization on the Exynos see the code here: > >>>>> > >>>>> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework > >>>>> and patches adding mipi_dsi_host_init() to panel/bridge drivers. > >>>> As I said before, the downstream bridge needs an explicit call to host > >>>> init via mipi_dsi_host_init - this is indeed not a usual use-case > >>>> scenario. Let's handle this with a REINIT fix and see if we can update > >>>> this later to handle both scenarios. > >>>> > >>>> Would you please test this repo, I have included all. > >>>> > >>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > >>> This doesn't work on TM2e board. Give me some time to find why... > >>> > >> The following change is missing in "drm: bridge: Generalize Exynos-DSI > >> driver into a Samsung DSIM bridge" patch: > >> > >> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >> b/drivers/gpu/drm/bridge/samsung-dsim.c > >> index 1dbff2bee93f..81828b5ee0ac 100644 > >> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >> @@ -1745,6 +1745,7 @@ int samsung_dsim_probe(struct platform_device *pdev) > >> dsi->bridge.funcs = &samsung_dsim_bridge_funcs; > >> dsi->bridge.of_node = dev->of_node; > >> dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; > >> + dsi->bridge.pre_enable_prev_first = true; > > Can you check this and confirm, I keep this in exynos side. > > https://gitlab.com/openedev/kernel/-/commit/ccb02df7a313fdf91d8e116b0ec3d6c945fbb6fd#c93f0ce4d81b854fbde970e341fb307f1be78c16_1865_189 > > This one is fine! > > >> /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts > >> HS/VS/DE */ > >> if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) > >> > >> > >> After adding the above, all my test platform works fine. > >> > >> BTW, I still think that it is worth replacing the "drm: exynos: dsi: Add > >> host initialization in pre_enable" patch with the following simple > >> change and propagate it to bridge/samsung-dsim.c: > >> > >> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >> index fdaf514b39f2..071b74d60dcb 100644 > >> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >> @@ -254,6 +254,9 @@ struct exynos_dsi_transfer { > >> #define DSIM_STATE_CMD_LPM BIT(2) > >> #define DSIM_STATE_VIDOUT_AVAILABLE BIT(3) > >> > >> +#define exynos_dsi_hw_is_exynos(hw) \ > >> + ((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433) > >> + > >> enum exynos_dsi_type { > >> DSIM_TYPE_EXYNOS3250, > >> DSIM_TYPE_EXYNOS4210, > >> @@ -1344,6 +1347,9 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) > >> { > >> const struct exynos_dsi_driver_data *driver_data = > >> dsi->driver_data; > >> > >> + if (dsi->state & DSIM_STATE_INITIALIZED) > >> + return 0; > >> + > >> exynos_dsi_reset(dsi); > >> exynos_dsi_enable_irq(dsi); > >> > >> @@ -1356,6 +1362,8 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) > >> exynos_dsi_set_phy_ctrl(dsi); > >> exynos_dsi_init_link(dsi); > >> > >> + dsi->state |= DSIM_STATE_INITIALIZED; > >> + > >> return 0; > >> } > >> > >> @@ -1411,6 +1419,12 @@ static void exynos_dsi_atomic_pre_enable(struct > >> drm_bridge *bridge, > >> } > >> > >> dsi->state |= DSIM_STATE_ENABLED; > >> + > >> + if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) { > >> + ret = exynos_dsi_init(dsi); > >> + if (ret) > >> + return; > >> + } > >> } > >> > >> static void exynos_dsi_atomic_enable(struct drm_bridge *bridge, > >> @@ -1557,12 +1571,9 @@ static ssize_t exynos_dsi_host_transfer(struct > >> mipi_dsi_host *host, > >> if (!(dsi->state & DSIM_STATE_ENABLED)) > >> return -EINVAL; > >> > >> - if (!(dsi->state & DSIM_STATE_INITIALIZED)) { > >> - ret = exynos_dsi_init(dsi); > >> - if (ret) > >> - return ret; > >> - dsi->state |= DSIM_STATE_INITIALIZED; > >> - } > >> + ret = exynos_dsi_init(dsi); > >> + if (ret) > >> + return ret; > > Below patch handling similar behavior by checking exynos hw_type at > > exynos_dsi_init, isn't it? Please check and let me know if I missing > > anything. > > > > https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 > > You don't miss anything. Your version also works, but I just proposed a > bit simpler code. Do your changes don't need a DSIM_STATE_REINITIALIZED flag? would you please share the change on top of this commit? https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 Jagan.
On 13.12.2022 16:15, Jagan Teki wrote: > On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> On 13.12.2022 15:18, Jagan Teki wrote: >>> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski >>> <m.szyprowski@samsung.com> wrote: >>>> On 13.12.2022 13:20, Marek Szyprowski wrote: >>>>> On 13.12.2022 11:40, Jagan Teki wrote: >>>>>> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski >>>>>> <m.szyprowski@samsung.com> wrote: >>>>>>> On 12.12.2022 16:33, Jagan Teki wrote: >>>>>>> >>>>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski >>>>>>> <m.szyprowski@samsung.com> wrote: >>>>>>> >>>>>>> On 12.12.2022 09:43, Marek Szyprowski wrote: >>>>>>> >>>>>>> On 12.12.2022 09:32, Jagan Teki wrote: >>>>>>> >>>>>>> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski >>>>>>> <m.szyprowski@samsung.com> wrote: >>>>>>> >>>>>>> Hi Jagan, >>>>>>> >>>>>>> On 09.12.2022 16:23, Jagan Teki wrote: >>>>>>> >>>>>>> The existing drm panels and bridges in Exynos required host >>>>>>> initialization during the first DSI command transfer even though >>>>>>> the initialization was done before. >>>>>>> >>>>>>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED >>>>>>> flag and triggers from host transfer. >>>>>>> >>>>>>> Do this exclusively for Exynos. >>>>>>> >>>>>>> Initial logic is derived from Marek Szyprowski changes. >>>>>>> >>>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>>>>>> --- >>>>>>> Changes from v9: >>>>>>> - derived from v8 >>>>>>> - added comments >>>>>>> >>>>>>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- >>>>>>> include/drm/bridge/samsung-dsim.h | 5 +++-- >>>>>>> 2 files changed, 17 insertions(+), 3 deletions(-) >>>>>>> >>>>>>> The following chunk is missing compared to v8: >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>> index 6e9ad955ebd3..6a9403cb92ae 100644 >>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim >>>>>>> *dsi, unsigned int flag) >>>>>>> return 0; >>>>>>> >>>>>>> samsung_dsim_reset(dsi); >>>>>>> - samsung_dsim_enable_irq(dsi); >>>>>>> + >>>>>>> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) >>>>>>> + samsung_dsim_enable_irq(dsi); >>>>>>> >>>>>>> Is this really required? does it make sure that the IRQ does not >>>>>>> enable twice? >>>>>>> >>>>>>> That's what that check does. Without the 'if (!(dsi->state & >>>>>>> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first >>>>>>> from pre_enable, then from the first transfer), what leads to a >>>>>>> warning from irq core. >>>>>>> >>>>>>> I've just noticed that we also would need to clear the >>>>>>> DSIM_STATE_REINITIALIZED flag in dsim_suspend. >>>>>>> >>>>>>> However I've found that a bit simpler patch would keep the current code >>>>>>> flow for Exynos instead of this reinitialization hack. This can be >>>>>>> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host >>>>>>> init in pre_enable" patch: >>>>>>> >>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>> index 0b2e52585485..acc95c61ae45 100644 >>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>> @@ -1291,9 +1291,11 @@ static void >>>>>>> samsung_dsim_atomic_pre_enable(struct >>>>>>> drm_bridge *bridge, >>>>>>> >>>>>>> dsi->state |= DSIM_STATE_ENABLED; >>>>>>> >>>>>>> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >>>>>>> - if (ret) >>>>>>> - return; >>>>>>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { >>>>>>> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >>>>>>> + if (ret) >>>>>>> + return; >>>>>>> + } >>>>>>> >>>>>>> Sorry, I don't understand this. Does it mean Exynos doesn't need to >>>>>>> init host in pre_enable? If I remember correctly even though the host >>>>>>> is initialized it has to reinitialize during the first transfer - This >>>>>>> is what the Exynos requirement is. Please correct or explain here. >>>>>>> >>>>>>> This is a matter of enabling power regulator(s) in the right order >>>>>>> and doing the host initialization in the right moment. It was never >>>>>>> a matter of re-initialization. See the current code for the >>>>>>> reference (it uses the same approach as my above change). I've >>>>>>> already explained that here: >>>>>>> >>>>>>> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ >>>>>>> >>>>>>> >>>>>>> If you would like to see the exact proper moment of the dsi host >>>>>>> initialization on the Exynos see the code here: >>>>>>> >>>>>>> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework >>>>>>> and patches adding mipi_dsi_host_init() to panel/bridge drivers. >>>>>> As I said before, the downstream bridge needs an explicit call to host >>>>>> init via mipi_dsi_host_init - this is indeed not a usual use-case >>>>>> scenario. Let's handle this with a REINIT fix and see if we can update >>>>>> this later to handle both scenarios. >>>>>> >>>>>> Would you please test this repo, I have included all. >>>>>> >>>>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 >>>>> This doesn't work on TM2e board. Give me some time to find why... >>>>> >>>> The following change is missing in "drm: bridge: Generalize Exynos-DSI >>>> driver into a Samsung DSIM bridge" patch: >>>> >>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>>> index 1dbff2bee93f..81828b5ee0ac 100644 >>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>>> @@ -1745,6 +1745,7 @@ int samsung_dsim_probe(struct platform_device *pdev) >>>> dsi->bridge.funcs = &samsung_dsim_bridge_funcs; >>>> dsi->bridge.of_node = dev->of_node; >>>> dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; >>>> + dsi->bridge.pre_enable_prev_first = true; >>> Can you check this and confirm, I keep this in exynos side. >>> https://gitlab.com/openedev/kernel/-/commit/ccb02df7a313fdf91d8e116b0ec3d6c945fbb6fd#c93f0ce4d81b854fbde970e341fb307f1be78c16_1865_189 >> This one is fine! >> >>>> /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts >>>> HS/VS/DE */ >>>> if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) >>>> >>>> >>>> After adding the above, all my test platform works fine. >>>> >>>> BTW, I still think that it is worth replacing the "drm: exynos: dsi: Add >>>> host initialization in pre_enable" patch with the following simple >>>> change and propagate it to bridge/samsung-dsim.c: >>>> >>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>> index fdaf514b39f2..071b74d60dcb 100644 >>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>> @@ -254,6 +254,9 @@ struct exynos_dsi_transfer { >>>> #define DSIM_STATE_CMD_LPM BIT(2) >>>> #define DSIM_STATE_VIDOUT_AVAILABLE BIT(3) >>>> >>>> +#define exynos_dsi_hw_is_exynos(hw) \ >>>> + ((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433) >>>> + >>>> enum exynos_dsi_type { >>>> DSIM_TYPE_EXYNOS3250, >>>> DSIM_TYPE_EXYNOS4210, >>>> @@ -1344,6 +1347,9 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) >>>> { >>>> const struct exynos_dsi_driver_data *driver_data = >>>> dsi->driver_data; >>>> >>>> + if (dsi->state & DSIM_STATE_INITIALIZED) >>>> + return 0; >>>> + >>>> exynos_dsi_reset(dsi); >>>> exynos_dsi_enable_irq(dsi); >>>> >>>> @@ -1356,6 +1362,8 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) >>>> exynos_dsi_set_phy_ctrl(dsi); >>>> exynos_dsi_init_link(dsi); >>>> >>>> + dsi->state |= DSIM_STATE_INITIALIZED; >>>> + >>>> return 0; >>>> } >>>> >>>> @@ -1411,6 +1419,12 @@ static void exynos_dsi_atomic_pre_enable(struct >>>> drm_bridge *bridge, >>>> } >>>> >>>> dsi->state |= DSIM_STATE_ENABLED; >>>> + >>>> + if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) { >>>> + ret = exynos_dsi_init(dsi); >>>> + if (ret) >>>> + return; >>>> + } >>>> } >>>> >>>> static void exynos_dsi_atomic_enable(struct drm_bridge *bridge, >>>> @@ -1557,12 +1571,9 @@ static ssize_t exynos_dsi_host_transfer(struct >>>> mipi_dsi_host *host, >>>> if (!(dsi->state & DSIM_STATE_ENABLED)) >>>> return -EINVAL; >>>> >>>> - if (!(dsi->state & DSIM_STATE_INITIALIZED)) { >>>> - ret = exynos_dsi_init(dsi); >>>> - if (ret) >>>> - return ret; >>>> - dsi->state |= DSIM_STATE_INITIALIZED; >>>> - } >>>> + ret = exynos_dsi_init(dsi); >>>> + if (ret) >>>> + return ret; >>> Below patch handling similar behavior by checking exynos hw_type at >>> exynos_dsi_init, isn't it? Please check and let me know if I missing >>> anything. >>> >>> https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 >> You don't miss anything. Your version also works, but I just proposed a >> bit simpler code. > Do your changes don't need a DSIM_STATE_REINITIALIZED flag? would you > please share the change on top of this commit? > https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 It doesn't need the DSIM_STATE_REINITIALIZED flag because the initialization is done only once - in pre_enable for non-Exynos case and on the first transfer for the Exynos case. In both cases the same flag (DSIM_STATE_INITIALIZED) is used. See the attached patch. Best regards
On Tue, Dec 13, 2022 at 9:11 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 13.12.2022 16:15, Jagan Teki wrote: > > On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski > > <m.szyprowski@samsung.com> wrote: > >> On 13.12.2022 15:18, Jagan Teki wrote: > >>> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski > >>> <m.szyprowski@samsung.com> wrote: > >>>> On 13.12.2022 13:20, Marek Szyprowski wrote: > >>>>> On 13.12.2022 11:40, Jagan Teki wrote: > >>>>>> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski > >>>>>> <m.szyprowski@samsung.com> wrote: > >>>>>>> On 12.12.2022 16:33, Jagan Teki wrote: > >>>>>>> > >>>>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski > >>>>>>> <m.szyprowski@samsung.com> wrote: > >>>>>>> > >>>>>>> On 12.12.2022 09:43, Marek Szyprowski wrote: > >>>>>>> > >>>>>>> On 12.12.2022 09:32, Jagan Teki wrote: > >>>>>>> > >>>>>>> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski > >>>>>>> <m.szyprowski@samsung.com> wrote: > >>>>>>> > >>>>>>> Hi Jagan, > >>>>>>> > >>>>>>> On 09.12.2022 16:23, Jagan Teki wrote: > >>>>>>> > >>>>>>> The existing drm panels and bridges in Exynos required host > >>>>>>> initialization during the first DSI command transfer even though > >>>>>>> the initialization was done before. > >>>>>>> > >>>>>>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED > >>>>>>> flag and triggers from host transfer. > >>>>>>> > >>>>>>> Do this exclusively for Exynos. > >>>>>>> > >>>>>>> Initial logic is derived from Marek Szyprowski changes. > >>>>>>> > >>>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > >>>>>>> --- > >>>>>>> Changes from v9: > >>>>>>> - derived from v8 > >>>>>>> - added comments > >>>>>>> > >>>>>>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- > >>>>>>> include/drm/bridge/samsung-dsim.h | 5 +++-- > >>>>>>> 2 files changed, 17 insertions(+), 3 deletions(-) > >>>>>>> > >>>>>>> The following chunk is missing compared to v8: > >>>>>>> > >>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>> index 6e9ad955ebd3..6a9403cb92ae 100644 > >>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim > >>>>>>> *dsi, unsigned int flag) > >>>>>>> return 0; > >>>>>>> > >>>>>>> samsung_dsim_reset(dsi); > >>>>>>> - samsung_dsim_enable_irq(dsi); > >>>>>>> + > >>>>>>> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) > >>>>>>> + samsung_dsim_enable_irq(dsi); > >>>>>>> > >>>>>>> Is this really required? does it make sure that the IRQ does not > >>>>>>> enable twice? > >>>>>>> > >>>>>>> That's what that check does. Without the 'if (!(dsi->state & > >>>>>>> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first > >>>>>>> from pre_enable, then from the first transfer), what leads to a > >>>>>>> warning from irq core. > >>>>>>> > >>>>>>> I've just noticed that we also would need to clear the > >>>>>>> DSIM_STATE_REINITIALIZED flag in dsim_suspend. > >>>>>>> > >>>>>>> However I've found that a bit simpler patch would keep the current code > >>>>>>> flow for Exynos instead of this reinitialization hack. This can be > >>>>>>> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host > >>>>>>> init in pre_enable" patch: > >>>>>>> > >>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>> index 0b2e52585485..acc95c61ae45 100644 > >>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>> @@ -1291,9 +1291,11 @@ static void > >>>>>>> samsung_dsim_atomic_pre_enable(struct > >>>>>>> drm_bridge *bridge, > >>>>>>> > >>>>>>> dsi->state |= DSIM_STATE_ENABLED; > >>>>>>> > >>>>>>> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > >>>>>>> - if (ret) > >>>>>>> - return; > >>>>>>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { > >>>>>>> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > >>>>>>> + if (ret) > >>>>>>> + return; > >>>>>>> + } > >>>>>>> > >>>>>>> Sorry, I don't understand this. Does it mean Exynos doesn't need to > >>>>>>> init host in pre_enable? If I remember correctly even though the host > >>>>>>> is initialized it has to reinitialize during the first transfer - This > >>>>>>> is what the Exynos requirement is. Please correct or explain here. > >>>>>>> > >>>>>>> This is a matter of enabling power regulator(s) in the right order > >>>>>>> and doing the host initialization in the right moment. It was never > >>>>>>> a matter of re-initialization. See the current code for the > >>>>>>> reference (it uses the same approach as my above change). I've > >>>>>>> already explained that here: > >>>>>>> > >>>>>>> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ > >>>>>>> > >>>>>>> > >>>>>>> If you would like to see the exact proper moment of the dsi host > >>>>>>> initialization on the Exynos see the code here: > >>>>>>> > >>>>>>> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework > >>>>>>> and patches adding mipi_dsi_host_init() to panel/bridge drivers. > >>>>>> As I said before, the downstream bridge needs an explicit call to host > >>>>>> init via mipi_dsi_host_init - this is indeed not a usual use-case > >>>>>> scenario. Let's handle this with a REINIT fix and see if we can update > >>>>>> this later to handle both scenarios. > >>>>>> > >>>>>> Would you please test this repo, I have included all. > >>>>>> > >>>>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > >>>>> This doesn't work on TM2e board. Give me some time to find why... > >>>>> > >>>> The following change is missing in "drm: bridge: Generalize Exynos-DSI > >>>> driver into a Samsung DSIM bridge" patch: > >>>> > >>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>> b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>> index 1dbff2bee93f..81828b5ee0ac 100644 > >>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>> @@ -1745,6 +1745,7 @@ int samsung_dsim_probe(struct platform_device *pdev) > >>>> dsi->bridge.funcs = &samsung_dsim_bridge_funcs; > >>>> dsi->bridge.of_node = dev->of_node; > >>>> dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; > >>>> + dsi->bridge.pre_enable_prev_first = true; > >>> Can you check this and confirm, I keep this in exynos side. > >>> https://gitlab.com/openedev/kernel/-/commit/ccb02df7a313fdf91d8e116b0ec3d6c945fbb6fd#c93f0ce4d81b854fbde970e341fb307f1be78c16_1865_189 > >> This one is fine! > >> > >>>> /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts > >>>> HS/VS/DE */ > >>>> if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) > >>>> > >>>> > >>>> After adding the above, all my test platform works fine. > >>>> > >>>> BTW, I still think that it is worth replacing the "drm: exynos: dsi: Add > >>>> host initialization in pre_enable" patch with the following simple > >>>> change and propagate it to bridge/samsung-dsim.c: > >>>> > >>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >>>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >>>> index fdaf514b39f2..071b74d60dcb 100644 > >>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >>>> @@ -254,6 +254,9 @@ struct exynos_dsi_transfer { > >>>> #define DSIM_STATE_CMD_LPM BIT(2) > >>>> #define DSIM_STATE_VIDOUT_AVAILABLE BIT(3) > >>>> > >>>> +#define exynos_dsi_hw_is_exynos(hw) \ > >>>> + ((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433) > >>>> + > >>>> enum exynos_dsi_type { > >>>> DSIM_TYPE_EXYNOS3250, > >>>> DSIM_TYPE_EXYNOS4210, > >>>> @@ -1344,6 +1347,9 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) > >>>> { > >>>> const struct exynos_dsi_driver_data *driver_data = > >>>> dsi->driver_data; > >>>> > >>>> + if (dsi->state & DSIM_STATE_INITIALIZED) > >>>> + return 0; > >>>> + > >>>> exynos_dsi_reset(dsi); > >>>> exynos_dsi_enable_irq(dsi); > >>>> > >>>> @@ -1356,6 +1362,8 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) > >>>> exynos_dsi_set_phy_ctrl(dsi); > >>>> exynos_dsi_init_link(dsi); > >>>> > >>>> + dsi->state |= DSIM_STATE_INITIALIZED; > >>>> + > >>>> return 0; > >>>> } > >>>> > >>>> @@ -1411,6 +1419,12 @@ static void exynos_dsi_atomic_pre_enable(struct > >>>> drm_bridge *bridge, > >>>> } > >>>> > >>>> dsi->state |= DSIM_STATE_ENABLED; > >>>> + > >>>> + if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) { > >>>> + ret = exynos_dsi_init(dsi); > >>>> + if (ret) > >>>> + return; > >>>> + } > >>>> } > >>>> > >>>> static void exynos_dsi_atomic_enable(struct drm_bridge *bridge, > >>>> @@ -1557,12 +1571,9 @@ static ssize_t exynos_dsi_host_transfer(struct > >>>> mipi_dsi_host *host, > >>>> if (!(dsi->state & DSIM_STATE_ENABLED)) > >>>> return -EINVAL; > >>>> > >>>> - if (!(dsi->state & DSIM_STATE_INITIALIZED)) { > >>>> - ret = exynos_dsi_init(dsi); > >>>> - if (ret) > >>>> - return ret; > >>>> - dsi->state |= DSIM_STATE_INITIALIZED; > >>>> - } > >>>> + ret = exynos_dsi_init(dsi); > >>>> + if (ret) > >>>> + return ret; > >>> Below patch handling similar behavior by checking exynos hw_type at > >>> exynos_dsi_init, isn't it? Please check and let me know if I missing > >>> anything. > >>> > >>> https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 > >> You don't miss anything. Your version also works, but I just proposed a > >> bit simpler code. > > Do your changes don't need a DSIM_STATE_REINITIALIZED flag? would you > > please share the change on top of this commit? > > https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 > > It doesn't need the DSIM_STATE_REINITIALIZED flag because the > initialization is done only once - in pre_enable for non-Exynos case and > on the first transfer for the Exynos case. In both cases the same flag > (DSIM_STATE_INITIALIZED) is used. > > See the attached patch. Thanks, I have included the changes and added your authorship as well. Please test this final version and let me know if you have any comments. https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 Jagan.
On 14.12.2022 06:33, Jagan Teki wrote: > On Tue, Dec 13, 2022 at 9:11 PM Marek Szyprowski > <m.szyprowski@samsung.com> wrote: >> On 13.12.2022 16:15, Jagan Teki wrote: >>> On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski >>> <m.szyprowski@samsung.com> wrote: >>>> On 13.12.2022 15:18, Jagan Teki wrote: >>>>> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski >>>>> <m.szyprowski@samsung.com> wrote: >>>>>> On 13.12.2022 13:20, Marek Szyprowski wrote: >>>>>>> On 13.12.2022 11:40, Jagan Teki wrote: >>>>>>>> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski >>>>>>>> <m.szyprowski@samsung.com> wrote: >>>>>>>>> On 12.12.2022 16:33, Jagan Teki wrote: >>>>>>>>> >>>>>>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski >>>>>>>>> <m.szyprowski@samsung.com> wrote: >>>>>>>>> >>>>>>>>> On 12.12.2022 09:43, Marek Szyprowski wrote: >>>>>>>>> >>>>>>>>> On 12.12.2022 09:32, Jagan Teki wrote: >>>>>>>>> >>>>>>>>> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski >>>>>>>>> <m.szyprowski@samsung.com> wrote: >>>>>>>>> >>>>>>>>> Hi Jagan, >>>>>>>>> >>>>>>>>> On 09.12.2022 16:23, Jagan Teki wrote: >>>>>>>>> >>>>>>>>> The existing drm panels and bridges in Exynos required host >>>>>>>>> initialization during the first DSI command transfer even though >>>>>>>>> the initialization was done before. >>>>>>>>> >>>>>>>>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED >>>>>>>>> flag and triggers from host transfer. >>>>>>>>> >>>>>>>>> Do this exclusively for Exynos. >>>>>>>>> >>>>>>>>> Initial logic is derived from Marek Szyprowski changes. >>>>>>>>> >>>>>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> >>>>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> >>>>>>>>> --- >>>>>>>>> Changes from v9: >>>>>>>>> - derived from v8 >>>>>>>>> - added comments >>>>>>>>> >>>>>>>>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- >>>>>>>>> include/drm/bridge/samsung-dsim.h | 5 +++-- >>>>>>>>> 2 files changed, 17 insertions(+), 3 deletions(-) >>>>>>>>> >>>>>>>>> The following chunk is missing compared to v8: >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>> index 6e9ad955ebd3..6a9403cb92ae 100644 >>>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim >>>>>>>>> *dsi, unsigned int flag) >>>>>>>>> return 0; >>>>>>>>> >>>>>>>>> samsung_dsim_reset(dsi); >>>>>>>>> - samsung_dsim_enable_irq(dsi); >>>>>>>>> + >>>>>>>>> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) >>>>>>>>> + samsung_dsim_enable_irq(dsi); >>>>>>>>> >>>>>>>>> Is this really required? does it make sure that the IRQ does not >>>>>>>>> enable twice? >>>>>>>>> >>>>>>>>> That's what that check does. Without the 'if (!(dsi->state & >>>>>>>>> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first >>>>>>>>> from pre_enable, then from the first transfer), what leads to a >>>>>>>>> warning from irq core. >>>>>>>>> >>>>>>>>> I've just noticed that we also would need to clear the >>>>>>>>> DSIM_STATE_REINITIALIZED flag in dsim_suspend. >>>>>>>>> >>>>>>>>> However I've found that a bit simpler patch would keep the current code >>>>>>>>> flow for Exynos instead of this reinitialization hack. This can be >>>>>>>>> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host >>>>>>>>> init in pre_enable" patch: >>>>>>>>> >>>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>> index 0b2e52585485..acc95c61ae45 100644 >>>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>>>>> @@ -1291,9 +1291,11 @@ static void >>>>>>>>> samsung_dsim_atomic_pre_enable(struct >>>>>>>>> drm_bridge *bridge, >>>>>>>>> >>>>>>>>> dsi->state |= DSIM_STATE_ENABLED; >>>>>>>>> >>>>>>>>> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >>>>>>>>> - if (ret) >>>>>>>>> - return; >>>>>>>>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { >>>>>>>>> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); >>>>>>>>> + if (ret) >>>>>>>>> + return; >>>>>>>>> + } >>>>>>>>> >>>>>>>>> Sorry, I don't understand this. Does it mean Exynos doesn't need to >>>>>>>>> init host in pre_enable? If I remember correctly even though the host >>>>>>>>> is initialized it has to reinitialize during the first transfer - This >>>>>>>>> is what the Exynos requirement is. Please correct or explain here. >>>>>>>>> >>>>>>>>> This is a matter of enabling power regulator(s) in the right order >>>>>>>>> and doing the host initialization in the right moment. It was never >>>>>>>>> a matter of re-initialization. See the current code for the >>>>>>>>> reference (it uses the same approach as my above change). I've >>>>>>>>> already explained that here: >>>>>>>>> >>>>>>>>> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ >>>>>>>>> >>>>>>>>> >>>>>>>>> If you would like to see the exact proper moment of the dsi host >>>>>>>>> initialization on the Exynos see the code here: >>>>>>>>> >>>>>>>>> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework >>>>>>>>> and patches adding mipi_dsi_host_init() to panel/bridge drivers. >>>>>>>> As I said before, the downstream bridge needs an explicit call to host >>>>>>>> init via mipi_dsi_host_init - this is indeed not a usual use-case >>>>>>>> scenario. Let's handle this with a REINIT fix and see if we can update >>>>>>>> this later to handle both scenarios. >>>>>>>> >>>>>>>> Would you please test this repo, I have included all. >>>>>>>> >>>>>>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 >>>>>>> This doesn't work on TM2e board. Give me some time to find why... >>>>>>> >>>>>> The following change is missing in "drm: bridge: Generalize Exynos-DSI >>>>>> driver into a Samsung DSIM bridge" patch: >>>>>> >>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>> index 1dbff2bee93f..81828b5ee0ac 100644 >>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c >>>>>> @@ -1745,6 +1745,7 @@ int samsung_dsim_probe(struct platform_device *pdev) >>>>>> dsi->bridge.funcs = &samsung_dsim_bridge_funcs; >>>>>> dsi->bridge.of_node = dev->of_node; >>>>>> dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; >>>>>> + dsi->bridge.pre_enable_prev_first = true; >>>>> Can you check this and confirm, I keep this in exynos side. >>>>> https://gitlab.com/openedev/kernel/-/commit/ccb02df7a313fdf91d8e116b0ec3d6c945fbb6fd#c93f0ce4d81b854fbde970e341fb307f1be78c16_1865_189 >>>> This one is fine! >>>> >>>>>> /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts >>>>>> HS/VS/DE */ >>>>>> if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) >>>>>> >>>>>> >>>>>> After adding the above, all my test platform works fine. >>>>>> >>>>>> BTW, I still think that it is worth replacing the "drm: exynos: dsi: Add >>>>>> host initialization in pre_enable" patch with the following simple >>>>>> change and propagate it to bridge/samsung-dsim.c: >>>>>> >>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>> index fdaf514b39f2..071b74d60dcb 100644 >>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c >>>>>> @@ -254,6 +254,9 @@ struct exynos_dsi_transfer { >>>>>> #define DSIM_STATE_CMD_LPM BIT(2) >>>>>> #define DSIM_STATE_VIDOUT_AVAILABLE BIT(3) >>>>>> >>>>>> +#define exynos_dsi_hw_is_exynos(hw) \ >>>>>> + ((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433) >>>>>> + >>>>>> enum exynos_dsi_type { >>>>>> DSIM_TYPE_EXYNOS3250, >>>>>> DSIM_TYPE_EXYNOS4210, >>>>>> @@ -1344,6 +1347,9 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) >>>>>> { >>>>>> const struct exynos_dsi_driver_data *driver_data = >>>>>> dsi->driver_data; >>>>>> >>>>>> + if (dsi->state & DSIM_STATE_INITIALIZED) >>>>>> + return 0; >>>>>> + >>>>>> exynos_dsi_reset(dsi); >>>>>> exynos_dsi_enable_irq(dsi); >>>>>> >>>>>> @@ -1356,6 +1362,8 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) >>>>>> exynos_dsi_set_phy_ctrl(dsi); >>>>>> exynos_dsi_init_link(dsi); >>>>>> >>>>>> + dsi->state |= DSIM_STATE_INITIALIZED; >>>>>> + >>>>>> return 0; >>>>>> } >>>>>> >>>>>> @@ -1411,6 +1419,12 @@ static void exynos_dsi_atomic_pre_enable(struct >>>>>> drm_bridge *bridge, >>>>>> } >>>>>> >>>>>> dsi->state |= DSIM_STATE_ENABLED; >>>>>> + >>>>>> + if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) { >>>>>> + ret = exynos_dsi_init(dsi); >>>>>> + if (ret) >>>>>> + return; >>>>>> + } >>>>>> } >>>>>> >>>>>> static void exynos_dsi_atomic_enable(struct drm_bridge *bridge, >>>>>> @@ -1557,12 +1571,9 @@ static ssize_t exynos_dsi_host_transfer(struct >>>>>> mipi_dsi_host *host, >>>>>> if (!(dsi->state & DSIM_STATE_ENABLED)) >>>>>> return -EINVAL; >>>>>> >>>>>> - if (!(dsi->state & DSIM_STATE_INITIALIZED)) { >>>>>> - ret = exynos_dsi_init(dsi); >>>>>> - if (ret) >>>>>> - return ret; >>>>>> - dsi->state |= DSIM_STATE_INITIALIZED; >>>>>> - } >>>>>> + ret = exynos_dsi_init(dsi); >>>>>> + if (ret) >>>>>> + return ret; >>>>> Below patch handling similar behavior by checking exynos hw_type at >>>>> exynos_dsi_init, isn't it? Please check and let me know if I missing >>>>> anything. >>>>> >>>>> https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 >>>> You don't miss anything. Your version also works, but I just proposed a >>>> bit simpler code. >>> Do your changes don't need a DSIM_STATE_REINITIALIZED flag? would you >>> please share the change on top of this commit? >>> https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 >> It doesn't need the DSIM_STATE_REINITIALIZED flag because the >> initialization is done only once - in pre_enable for non-Exynos case and >> on the first transfer for the Exynos case. In both cases the same flag >> (DSIM_STATE_INITIALIZED) is used. >> >> See the attached patch. > Thanks, I have included the changes and added your authorship as well. > > Please test this final version and let me know if you have any comments. > https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 Fine for me. Best regards
On Wed, Dec 14, 2022 at 1:34 PM Marek Szyprowski <m.szyprowski@samsung.com> wrote: > > On 14.12.2022 06:33, Jagan Teki wrote: > > On Tue, Dec 13, 2022 at 9:11 PM Marek Szyprowski > > <m.szyprowski@samsung.com> wrote: > >> On 13.12.2022 16:15, Jagan Teki wrote: > >>> On Tue, Dec 13, 2022 at 8:24 PM Marek Szyprowski > >>> <m.szyprowski@samsung.com> wrote: > >>>> On 13.12.2022 15:18, Jagan Teki wrote: > >>>>> On Tue, Dec 13, 2022 at 7:31 PM Marek Szyprowski > >>>>> <m.szyprowski@samsung.com> wrote: > >>>>>> On 13.12.2022 13:20, Marek Szyprowski wrote: > >>>>>>> On 13.12.2022 11:40, Jagan Teki wrote: > >>>>>>>> On Tue, Dec 13, 2022 at 2:28 PM Marek Szyprowski > >>>>>>>> <m.szyprowski@samsung.com> wrote: > >>>>>>>>> On 12.12.2022 16:33, Jagan Teki wrote: > >>>>>>>>> > >>>>>>>>> On Mon, Dec 12, 2022 at 8:52 PM Marek Szyprowski > >>>>>>>>> <m.szyprowski@samsung.com> wrote: > >>>>>>>>> > >>>>>>>>> On 12.12.2022 09:43, Marek Szyprowski wrote: > >>>>>>>>> > >>>>>>>>> On 12.12.2022 09:32, Jagan Teki wrote: > >>>>>>>>> > >>>>>>>>> On Mon, Dec 12, 2022 at 1:56 PM Marek Szyprowski > >>>>>>>>> <m.szyprowski@samsung.com> wrote: > >>>>>>>>> > >>>>>>>>> Hi Jagan, > >>>>>>>>> > >>>>>>>>> On 09.12.2022 16:23, Jagan Teki wrote: > >>>>>>>>> > >>>>>>>>> The existing drm panels and bridges in Exynos required host > >>>>>>>>> initialization during the first DSI command transfer even though > >>>>>>>>> the initialization was done before. > >>>>>>>>> > >>>>>>>>> This host reinitialization is handled via DSIM_STATE_REINITIALIZED > >>>>>>>>> flag and triggers from host transfer. > >>>>>>>>> > >>>>>>>>> Do this exclusively for Exynos. > >>>>>>>>> > >>>>>>>>> Initial logic is derived from Marek Szyprowski changes. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com> > >>>>>>>>> Signed-off-by: Jagan Teki <jagan@amarulasolutions.com> > >>>>>>>>> --- > >>>>>>>>> Changes from v9: > >>>>>>>>> - derived from v8 > >>>>>>>>> - added comments > >>>>>>>>> > >>>>>>>>> drivers/gpu/drm/bridge/samsung-dsim.c | 15 ++++++++++++++- > >>>>>>>>> include/drm/bridge/samsung-dsim.h | 5 +++-- > >>>>>>>>> 2 files changed, 17 insertions(+), 3 deletions(-) > >>>>>>>>> > >>>>>>>>> The following chunk is missing compared to v8: > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>>>> index 6e9ad955ebd3..6a9403cb92ae 100644 > >>>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>>>> @@ -1315,7 +1315,9 @@ static int samsung_dsim_init(struct samsung_dsim > >>>>>>>>> *dsi, unsigned int flag) > >>>>>>>>> return 0; > >>>>>>>>> > >>>>>>>>> samsung_dsim_reset(dsi); > >>>>>>>>> - samsung_dsim_enable_irq(dsi); > >>>>>>>>> + > >>>>>>>>> + if (!(dsi->state & DSIM_STATE_INITIALIZED)) > >>>>>>>>> + samsung_dsim_enable_irq(dsi); > >>>>>>>>> > >>>>>>>>> Is this really required? does it make sure that the IRQ does not > >>>>>>>>> enable twice? > >>>>>>>>> > >>>>>>>>> That's what that check does. Without the 'if (!(dsi->state & > >>>>>>>>> DSIM_STATE_INITIALIZED))' check, the irqs will be enabled twice (first > >>>>>>>>> from pre_enable, then from the first transfer), what leads to a > >>>>>>>>> warning from irq core. > >>>>>>>>> > >>>>>>>>> I've just noticed that we also would need to clear the > >>>>>>>>> DSIM_STATE_REINITIALIZED flag in dsim_suspend. > >>>>>>>>> > >>>>>>>>> However I've found that a bit simpler patch would keep the current code > >>>>>>>>> flow for Exynos instead of this reinitialization hack. This can be > >>>>>>>>> applied on the "[PATCH v9 09/18] drm: bridge: samsung-dsim: Add host > >>>>>>>>> init in pre_enable" patch: > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>>>> index 0b2e52585485..acc95c61ae45 100644 > >>>>>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>>>>> @@ -1291,9 +1291,11 @@ static void > >>>>>>>>> samsung_dsim_atomic_pre_enable(struct > >>>>>>>>> drm_bridge *bridge, > >>>>>>>>> > >>>>>>>>> dsi->state |= DSIM_STATE_ENABLED; > >>>>>>>>> > >>>>>>>>> - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > >>>>>>>>> - if (ret) > >>>>>>>>> - return; > >>>>>>>>> + if (!samsung_dsim_hw_is_exynos(dsi->plat_data->hw_type)) { > >>>>>>>>> + ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); > >>>>>>>>> + if (ret) > >>>>>>>>> + return; > >>>>>>>>> + } > >>>>>>>>> > >>>>>>>>> Sorry, I don't understand this. Does it mean Exynos doesn't need to > >>>>>>>>> init host in pre_enable? If I remember correctly even though the host > >>>>>>>>> is initialized it has to reinitialize during the first transfer - This > >>>>>>>>> is what the Exynos requirement is. Please correct or explain here. > >>>>>>>>> > >>>>>>>>> This is a matter of enabling power regulator(s) in the right order > >>>>>>>>> and doing the host initialization in the right moment. It was never > >>>>>>>>> a matter of re-initialization. See the current code for the > >>>>>>>>> reference (it uses the same approach as my above change). I've > >>>>>>>>> already explained that here: > >>>>>>>>> > >>>>>>>>> https://lore.kernel.org/all/e96197f9-948a-997e-5453-9f9d179b5f5a@samsung.com/ > >>>>>>>>> > >>>>>>>>> > >>>>>>>>> If you would like to see the exact proper moment of the dsi host > >>>>>>>>> initialization on the Exynos see the code here: > >>>>>>>>> > >>>>>>>>> https://protect2.fireeye.com/v1/url?k=5dc33900-0258001f-5dc2b24f-000babdfecba-f7c1a2a1905c83ca&q=1&e=f086bfdb-9055-48bd-b9c2-5dffb6c0d558&u=https%3A%2F%2Fgithub.com%2Fmszyprow%2Flinux%2Ftree%2Fv5.18-next-20220511-dsi-rework > >>>>>>>>> and patches adding mipi_dsi_host_init() to panel/bridge drivers. > >>>>>>>> As I said before, the downstream bridge needs an explicit call to host > >>>>>>>> init via mipi_dsi_host_init - this is indeed not a usual use-case > >>>>>>>> scenario. Let's handle this with a REINIT fix and see if we can update > >>>>>>>> this later to handle both scenarios. > >>>>>>>> > >>>>>>>> Would you please test this repo, I have included all. > >>>>>>>> > >>>>>>>> https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > >>>>>>> This doesn't work on TM2e board. Give me some time to find why... > >>>>>>> > >>>>>> The following change is missing in "drm: bridge: Generalize Exynos-DSI > >>>>>> driver into a Samsung DSIM bridge" patch: > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>> b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>> index 1dbff2bee93f..81828b5ee0ac 100644 > >>>>>> --- a/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>> +++ b/drivers/gpu/drm/bridge/samsung-dsim.c > >>>>>> @@ -1745,6 +1745,7 @@ int samsung_dsim_probe(struct platform_device *pdev) > >>>>>> dsi->bridge.funcs = &samsung_dsim_bridge_funcs; > >>>>>> dsi->bridge.of_node = dev->of_node; > >>>>>> dsi->bridge.type = DRM_MODE_CONNECTOR_DSI; > >>>>>> + dsi->bridge.pre_enable_prev_first = true; > >>>>> Can you check this and confirm, I keep this in exynos side. > >>>>> https://gitlab.com/openedev/kernel/-/commit/ccb02df7a313fdf91d8e116b0ec3d6c945fbb6fd#c93f0ce4d81b854fbde970e341fb307f1be78c16_1865_189 > >>>> This one is fine! > >>>> > >>>>>> /* DE_LOW: i.MX8M Mini/Nano LCDIF-DSIM glue logic inverts > >>>>>> HS/VS/DE */ > >>>>>> if (dsi->plat_data->hw_type == DSIM_TYPE_IMX8MM) > >>>>>> > >>>>>> > >>>>>> After adding the above, all my test platform works fine. > >>>>>> > >>>>>> BTW, I still think that it is worth replacing the "drm: exynos: dsi: Add > >>>>>> host initialization in pre_enable" patch with the following simple > >>>>>> change and propagate it to bridge/samsung-dsim.c: > >>>>>> > >>>>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >>>>>> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >>>>>> index fdaf514b39f2..071b74d60dcb 100644 > >>>>>> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >>>>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c > >>>>>> @@ -254,6 +254,9 @@ struct exynos_dsi_transfer { > >>>>>> #define DSIM_STATE_CMD_LPM BIT(2) > >>>>>> #define DSIM_STATE_VIDOUT_AVAILABLE BIT(3) > >>>>>> > >>>>>> +#define exynos_dsi_hw_is_exynos(hw) \ > >>>>>> + ((hw) >= DSIM_TYPE_EXYNOS3250 && (hw) <= DSIM_TYPE_EXYNOS5433) > >>>>>> + > >>>>>> enum exynos_dsi_type { > >>>>>> DSIM_TYPE_EXYNOS3250, > >>>>>> DSIM_TYPE_EXYNOS4210, > >>>>>> @@ -1344,6 +1347,9 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) > >>>>>> { > >>>>>> const struct exynos_dsi_driver_data *driver_data = > >>>>>> dsi->driver_data; > >>>>>> > >>>>>> + if (dsi->state & DSIM_STATE_INITIALIZED) > >>>>>> + return 0; > >>>>>> + > >>>>>> exynos_dsi_reset(dsi); > >>>>>> exynos_dsi_enable_irq(dsi); > >>>>>> > >>>>>> @@ -1356,6 +1362,8 @@ static int exynos_dsi_init(struct exynos_dsi *dsi) > >>>>>> exynos_dsi_set_phy_ctrl(dsi); > >>>>>> exynos_dsi_init_link(dsi); > >>>>>> > >>>>>> + dsi->state |= DSIM_STATE_INITIALIZED; > >>>>>> + > >>>>>> return 0; > >>>>>> } > >>>>>> > >>>>>> @@ -1411,6 +1419,12 @@ static void exynos_dsi_atomic_pre_enable(struct > >>>>>> drm_bridge *bridge, > >>>>>> } > >>>>>> > >>>>>> dsi->state |= DSIM_STATE_ENABLED; > >>>>>> + > >>>>>> + if (!exynos_dsi_hw_is_exynos(dsi->plat_data->hw_type)) { > >>>>>> + ret = exynos_dsi_init(dsi); > >>>>>> + if (ret) > >>>>>> + return; > >>>>>> + } > >>>>>> } > >>>>>> > >>>>>> static void exynos_dsi_atomic_enable(struct drm_bridge *bridge, > >>>>>> @@ -1557,12 +1571,9 @@ static ssize_t exynos_dsi_host_transfer(struct > >>>>>> mipi_dsi_host *host, > >>>>>> if (!(dsi->state & DSIM_STATE_ENABLED)) > >>>>>> return -EINVAL; > >>>>>> > >>>>>> - if (!(dsi->state & DSIM_STATE_INITIALIZED)) { > >>>>>> - ret = exynos_dsi_init(dsi); > >>>>>> - if (ret) > >>>>>> - return ret; > >>>>>> - dsi->state |= DSIM_STATE_INITIALIZED; > >>>>>> - } > >>>>>> + ret = exynos_dsi_init(dsi); > >>>>>> + if (ret) > >>>>>> + return ret; > >>>>> Below patch handling similar behavior by checking exynos hw_type at > >>>>> exynos_dsi_init, isn't it? Please check and let me know if I missing > >>>>> anything. > >>>>> > >>>>> https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 > >>>> You don't miss anything. Your version also works, but I just proposed a > >>>> bit simpler code. > >>> Do your changes don't need a DSIM_STATE_REINITIALIZED flag? would you > >>> please share the change on top of this commit? > >>> https://gitlab.com/openedev/kernel/-/commit/d19d491eef92b92e12a26265697274ce666eddb5 > >> It doesn't need the DSIM_STATE_REINITIALIZED flag because the > >> initialization is done only once - in pre_enable for non-Exynos case and > >> on the first transfer for the Exynos case. In both cases the same flag > >> (DSIM_STATE_INITIALIZED) is used. > >> > >> See the attached patch. > > Thanks, I have included the changes and added your authorship as well. > > > > Please test this final version and let me know if you have any comments. > > https://gitlab.com/openedev/kernel/-/commits/imx8mm-dsi-v10 > > > Fine for me. Thanks, I will send V10.
diff --git a/drivers/gpu/drm/bridge/samsung-dsim.c b/drivers/gpu/drm/bridge/samsung-dsim.c index 2e15d753fdd0..ec3ab679afd9 100644 --- a/drivers/gpu/drm/bridge/samsung-dsim.c +++ b/drivers/gpu/drm/bridge/samsung-dsim.c @@ -1254,6 +1254,19 @@ static int samsung_dsim_init(struct samsung_dsim *dsi, unsigned int flag) { const struct samsung_dsim_driver_data *driver_data = dsi->driver_data; + /* + * FIXME: + * The existing drm panels and bridges in Exynos required host + * initialization during the first DSI command transfer even though + * the initialization was done before. + * + * This host reinitialization is handled via DSIM_STATE_REINITIALIZED + * flag and triggers from host transfer. Do this exclusively for Exynos. + */ + if ((dsi->plat_data->hw_type == SAMSUNG_DSIM_TYPE_IMX8MM) && + dsi->state & DSIM_STATE_REINITIALIZED) + return 0; + if (dsi->state & flag) return 0; @@ -1467,7 +1480,7 @@ static ssize_t samsung_dsim_host_transfer(struct mipi_dsi_host *host, if (!(dsi->state & DSIM_STATE_ENABLED)) return -EINVAL; - ret = samsung_dsim_init(dsi, DSIM_STATE_INITIALIZED); + ret = samsung_dsim_init(dsi, DSIM_STATE_REINITIALIZED); if (ret) return ret; diff --git a/include/drm/bridge/samsung-dsim.h b/include/drm/bridge/samsung-dsim.h index b8132bf8e36f..0c5a905f3de7 100644 --- a/include/drm/bridge/samsung-dsim.h +++ b/include/drm/bridge/samsung-dsim.h @@ -17,8 +17,9 @@ struct samsung_dsim; #define DSIM_STATE_ENABLED BIT(0) #define DSIM_STATE_INITIALIZED BIT(1) -#define DSIM_STATE_CMD_LPM BIT(2) -#define DSIM_STATE_VIDOUT_AVAILABLE BIT(3) +#define DSIM_STATE_REINITIALIZED BIT(2) +#define DSIM_STATE_CMD_LPM BIT(3) +#define DSIM_STATE_VIDOUT_AVAILABLE BIT(4) enum samsung_dsim_type { SAMSUNG_DSIM_TYPE_EXYNOS3250,