[PATCH 0/7] drm/bridge: cdns-dsi: Fix the color-shift issue

[PATCH 0/7] drm/bridge: cdns-dsi: Fix the color-shift issue

[Aradhya Bhatia]

Hello all,

This series provides some crucial fixes and improvements for the Cadence's DSI
TX (cdns-dsi) controller found commonly in Texas Instruments' J7 family of SoCs
as well as in AM62P.

The cdns-dsi bridge consumes the crtc_* timing parameters for programming the
timing parameters. A patch has been added in tidss to make sure the crtc_*
timings get populated.

It further adds support for "early_enable" and "late_disable" DRM bridge hooks.
These hooks are same as the existing "(pre_)enable" and "(post_)disable" hooks,
except that the early_enable hook gets called before the CRTC is even enabled in
the display pipeline and the late_disable hook gets called after the CRTC is
disabled.
The cdns-dsi controller requires to be enabled before the previous entity
enables its stream[0]. It's a strict requirement which, if not followed, causes
the colors to "shift" on the display. Since the previous entity is TIDSS in this
case, which gets enabled via the tidss_crtc hooks, early_enable/late_disable API
in the cdns-dsi bridge is the way to solve the issue.
The early_enable/late_disable APIs also help with the OLDI TXes available on the
AM62/AM62P SoCs, which will be a part of separate series.

This spec also requires the Clock and Data Lanes be ready before the DSI TX
enables its stream[0]. A patch has been added to make the code wait for that to
happen. Going ahead with further DSI (and DSS configuration), while the lanes
are not ready, has been found as another reason for shift in colors.

All these patches have been tested on TI's vendor tree kernel with more devices,
but for the mainline, these patches have been tested with J721E based
BeagleboneAI64 along with a RaspberryPi 7" DSI panel. The extra patches can be
found in the "next_dsi_finals-v1-test_rpi" branch of my github fork[1] for
anyone who would like to test them.

Thanks,
Aradhya


[0]: Section 12.6.5.7.3: Start-up Procesure [For DSI TX controller]
     in TDA4VM Technical Reference Manual https://www.ti.com/lit/zip/spruil1

[1]: https://github.com/aradhya07/linux-ab/tree/next_dsi_finals-v1-test_rpi


Aradhya Bhatia (7):
  drm/tidss: Add CRTC mode_fixup
  drm/bridge: cdns-dsi: Fix minor bugs
  drm/bridge: cdns-dsi: Wait for Clk and Data Lanes to be ready
  drm/bridge: cdns-dsi: Reset the DCS write FIFO
  drm/bridge: cdns-dsi: Support atomic bridge APIs
  drm/bridge: Introduce early_enable and late disable
  drm/bridge: cdns-dsi: Implement early_enable and late_disable

 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 91 ++++++++++++++-----
 drivers/gpu/drm/drm_atomic_helper.c           | 67 ++++++++++++++
 drivers/gpu/drm/drm_bridge.c                  | 84 +++++++++++++++++
 drivers/gpu/drm/tidss/tidss_crtc.c            | 11 +++
 include/drm/drm_bridge.h                      | 73 +++++++++++++++
 5 files changed, 303 insertions(+), 23 deletions(-)


base-commit: 75fa778d74b786a1608d55d655d42b480a6fa8bd
-- 
2.34.1

[PATCH 1/7] drm/tidss: Add CRTC mode_fixup

[Aradhya Bhatia]

Add support for mode_fixup for the tidss CRTC.

Some bridges like the cdns-dsi consume the crtc_* timing parameters for
programming the blanking values. Allow for the normal timing parameters
to get copied to crtc_* timing params.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/tidss/tidss_crtc.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
index 94f8e3178df5..797ef53d9ad2 100644
--- a/drivers/gpu/drm/tidss/tidss_crtc.c
+++ b/drivers/gpu/drm/tidss/tidss_crtc.c
@@ -309,12 +309,23 @@ enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
 	return dispc_vp_mode_valid(tidss->dispc, tcrtc->hw_videoport, mode);
 }
 
+static
+bool tidss_crtc_mode_fixup(struct drm_crtc *crtc,
+			   const struct drm_display_mode *mode,
+			   struct drm_display_mode *adjusted_mode)
+{
+	drm_mode_set_crtcinfo(adjusted_mode, 0);
+
+	return true;
+}
+
 static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
 	.atomic_check = tidss_crtc_atomic_check,
 	.atomic_flush = tidss_crtc_atomic_flush,
 	.atomic_enable = tidss_crtc_atomic_enable,
 	.atomic_disable = tidss_crtc_atomic_disable,
 
+	.mode_fixup = tidss_crtc_mode_fixup,
 	.mode_valid = tidss_crtc_mode_valid,
 };
 
-- 
2.34.1

Re: [PATCH 1/7] drm/tidss: Add CRTC mode_fixup

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1754 bytes --]

Hi,

On Sat, May 11, 2024 at 09:00:45PM +0530, Aradhya Bhatia wrote:
> Add support for mode_fixup for the tidss CRTC.
> 
> Some bridges like the cdns-dsi consume the crtc_* timing parameters for
> programming the blanking values. Allow for the normal timing parameters
> to get copied to crtc_* timing params.
> 
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  drivers/gpu/drm/tidss/tidss_crtc.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> index 94f8e3178df5..797ef53d9ad2 100644
> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> @@ -309,12 +309,23 @@ enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
>  	return dispc_vp_mode_valid(tidss->dispc, tcrtc->hw_videoport, mode);
>  }
>  
> +static
> +bool tidss_crtc_mode_fixup(struct drm_crtc *crtc,
> +			   const struct drm_display_mode *mode,
> +			   struct drm_display_mode *adjusted_mode)
> +{
> +	drm_mode_set_crtcinfo(adjusted_mode, 0);
> +
> +	return true;
> +}
> +
>  static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
>  	.atomic_check = tidss_crtc_atomic_check,
>  	.atomic_flush = tidss_crtc_atomic_flush,
>  	.atomic_enable = tidss_crtc_atomic_enable,
>  	.atomic_disable = tidss_crtc_atomic_disable,
>  
> +	.mode_fixup = tidss_crtc_mode_fixup,
>  	.mode_valid = tidss_crtc_mode_valid,
>  };

mode_fixup is deprecated for atomic drivers, so the solution must be
different there.

It's also not clear to me how it could change anything there:
drm_mode_set_crtcinfo with no flags will make crtc_* field exactly
identical to their !crtc counterparts.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 1/7] drm/tidss: Add CRTC mode_fixup

[Aradhya Bhatia]

Hi Maxime,

Thank you for reviewing the patches.

On 16/05/24 13:40, Maxime Ripard wrote:
> Hi,
> 
> On Sat, May 11, 2024 at 09:00:45PM +0530, Aradhya Bhatia wrote:
>> Add support for mode_fixup for the tidss CRTC.
>>
>> Some bridges like the cdns-dsi consume the crtc_* timing parameters for
>> programming the blanking values. Allow for the normal timing parameters
>> to get copied to crtc_* timing params.
>>
>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>> ---
>>  drivers/gpu/drm/tidss/tidss_crtc.c | 11 +++++++++++
>>  1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
>> index 94f8e3178df5..797ef53d9ad2 100644
>> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
>> @@ -309,12 +309,23 @@ enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
>>  	return dispc_vp_mode_valid(tidss->dispc, tcrtc->hw_videoport, mode);
>>  }
>>  
>> +static
>> +bool tidss_crtc_mode_fixup(struct drm_crtc *crtc,
>> +			   const struct drm_display_mode *mode,
>> +			   struct drm_display_mode *adjusted_mode)
>> +{
>> +	drm_mode_set_crtcinfo(adjusted_mode, 0);
>> +
>> +	return true;
>> +}
>> +
>>  static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
>>  	.atomic_check = tidss_crtc_atomic_check,
>>  	.atomic_flush = tidss_crtc_atomic_flush,
>>  	.atomic_enable = tidss_crtc_atomic_enable,
>>  	.atomic_disable = tidss_crtc_atomic_disable,
>>  
>> +	.mode_fixup = tidss_crtc_mode_fixup,
>>  	.mode_valid = tidss_crtc_mode_valid,
>>  };
> 
> mode_fixup is deprecated for atomic drivers, so the solution must be
> different there.
> 
> It's also not clear to me how it could change anything there:
> drm_mode_set_crtcinfo with no flags will make crtc_* field exactly
> identical to their !crtc counterparts.
>

I checked the flag options. There isn't any flag required. The only
reason to add this call is because cdns-dsi strictly requires the crtc_*
fields to be populated during the bridge enable.

Secondly, if mode_fixup is deprecated, I think the crtc_atomic_check
would be the next best place to add this call.


Regards
Aradhya

Re: [PATCH 1/7] drm/tidss: Add CRTC mode_fixup

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2734 bytes --]

On Thu, May 16, 2024 at 04:33:40PM GMT, Aradhya Bhatia wrote:
> Hi Maxime,
> 
> Thank you for reviewing the patches.
> 
> On 16/05/24 13:40, Maxime Ripard wrote:
> > Hi,
> > 
> > On Sat, May 11, 2024 at 09:00:45PM +0530, Aradhya Bhatia wrote:
> >> Add support for mode_fixup for the tidss CRTC.
> >>
> >> Some bridges like the cdns-dsi consume the crtc_* timing parameters for
> >> programming the blanking values. Allow for the normal timing parameters
> >> to get copied to crtc_* timing params.
> >>
> >> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> >> ---
> >>  drivers/gpu/drm/tidss/tidss_crtc.c | 11 +++++++++++
> >>  1 file changed, 11 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
> >> index 94f8e3178df5..797ef53d9ad2 100644
> >> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
> >> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
> >> @@ -309,12 +309,23 @@ enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
> >>  	return dispc_vp_mode_valid(tidss->dispc, tcrtc->hw_videoport, mode);
> >>  }
> >>  
> >> +static
> >> +bool tidss_crtc_mode_fixup(struct drm_crtc *crtc,
> >> +			   const struct drm_display_mode *mode,
> >> +			   struct drm_display_mode *adjusted_mode)
> >> +{
> >> +	drm_mode_set_crtcinfo(adjusted_mode, 0);
> >> +
> >> +	return true;
> >> +}
> >> +
> >>  static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
> >>  	.atomic_check = tidss_crtc_atomic_check,
> >>  	.atomic_flush = tidss_crtc_atomic_flush,
> >>  	.atomic_enable = tidss_crtc_atomic_enable,
> >>  	.atomic_disable = tidss_crtc_atomic_disable,
> >>  
> >> +	.mode_fixup = tidss_crtc_mode_fixup,
> >>  	.mode_valid = tidss_crtc_mode_valid,
> >>  };
> > 
> > mode_fixup is deprecated for atomic drivers, so the solution must be
> > different there.
> > 
> > It's also not clear to me how it could change anything there:
> > drm_mode_set_crtcinfo with no flags will make crtc_* field exactly
> > identical to their !crtc counterparts.
> >
> 
> I checked the flag options. There isn't any flag required. The only
> reason to add this call is because cdns-dsi strictly requires the crtc_*
> fields to be populated during the bridge enable.
> 
> Secondly, if mode_fixup is deprecated, I think the crtc_atomic_check
> would be the next best place to add this call.

That would be better, yes, but we shouldn't even have to do that in the
first place. AFAIK all the path that create a drm_display_mode will call
drm_mode_set_crtcinfo on it to fill those fields. So if they are missing
somewhere, that's what the actual bug is, not something we should work
around of at the driver level.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 1/7] drm/tidss: Add CRTC mode_fixup

[Aradhya Bhatia]

Hi Maxime,

On 21/05/24 18:48, Maxime Ripard wrote:
> On Thu, May 16, 2024 at 04:33:40PM GMT, Aradhya Bhatia wrote:
>> Hi Maxime,
>>
>> Thank you for reviewing the patches.
>>
>> On 16/05/24 13:40, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Sat, May 11, 2024 at 09:00:45PM +0530, Aradhya Bhatia wrote:
>>>> Add support for mode_fixup for the tidss CRTC.
>>>>
>>>> Some bridges like the cdns-dsi consume the crtc_* timing parameters for
>>>> programming the blanking values. Allow for the normal timing parameters
>>>> to get copied to crtc_* timing params.
>>>>
>>>> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
>>>> ---
>>>>  drivers/gpu/drm/tidss/tidss_crtc.c | 11 +++++++++++
>>>>  1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c b/drivers/gpu/drm/tidss/tidss_crtc.c
>>>> index 94f8e3178df5..797ef53d9ad2 100644
>>>> --- a/drivers/gpu/drm/tidss/tidss_crtc.c
>>>> +++ b/drivers/gpu/drm/tidss/tidss_crtc.c
>>>> @@ -309,12 +309,23 @@ enum drm_mode_status tidss_crtc_mode_valid(struct drm_crtc *crtc,
>>>>  	return dispc_vp_mode_valid(tidss->dispc, tcrtc->hw_videoport, mode);
>>>>  }
>>>>  
>>>> +static
>>>> +bool tidss_crtc_mode_fixup(struct drm_crtc *crtc,
>>>> +			   const struct drm_display_mode *mode,
>>>> +			   struct drm_display_mode *adjusted_mode)
>>>> +{
>>>> +	drm_mode_set_crtcinfo(adjusted_mode, 0);
>>>> +
>>>> +	return true;
>>>> +}
>>>> +
>>>>  static const struct drm_crtc_helper_funcs tidss_crtc_helper_funcs = {
>>>>  	.atomic_check = tidss_crtc_atomic_check,
>>>>  	.atomic_flush = tidss_crtc_atomic_flush,
>>>>  	.atomic_enable = tidss_crtc_atomic_enable,
>>>>  	.atomic_disable = tidss_crtc_atomic_disable,
>>>>  
>>>> +	.mode_fixup = tidss_crtc_mode_fixup,
>>>>  	.mode_valid = tidss_crtc_mode_valid,
>>>>  };
>>>
>>> mode_fixup is deprecated for atomic drivers, so the solution must be
>>> different there.
>>>
>>> It's also not clear to me how it could change anything there:
>>> drm_mode_set_crtcinfo with no flags will make crtc_* field exactly
>>> identical to their !crtc counterparts.
>>>
>>
>> I checked the flag options. There isn't any flag required. The only
>> reason to add this call is because cdns-dsi strictly requires the crtc_*
>> fields to be populated during the bridge enable.
>>
>> Secondly, if mode_fixup is deprecated, I think the crtc_atomic_check
>> would be the next best place to add this call.
> 
> That would be better, yes, but we shouldn't even have to do that in the
> first place. AFAIK all the path that create a drm_display_mode will call
> drm_mode_set_crtcinfo on it to fill those fields. So if they are missing
> somewhere, that's what the actual bug is, not something we should work
> around of at the driver level.
> 

You are right. This patch isn't required at all. The fix around the
mode->crtc_clock usage in one place in the cdns-dsi driver makes this
patch unnecessary. The DRM core does duplicate the timing parameters at
a later stage for the cdns-dsi bridge_enable to consume.

Dropped this patch in v2. Thanks!

Regards
Aradhya

[PATCH 2/7] drm/bridge: cdns-dsi: Fix minor bugs

[Aradhya Bhatia]

Update the Phy initialized state to "not initialized" when the driver
(and the hardware by extension) gets suspended. This will allow the Phy
to get initialized again after resume.

Fix the OF node that gets passed to find the next available bridge in
the display pipeline.

Fix the order of DSI Link and DSI Phy inits. The link init needs to
happen before the Phy is initialized, so the Phy can lock on the
incoming PLL reference clock. If this doesn't happen, the Phy cannot
lock (until DSI Link is init later on). This causes a warning dump
during the kernel boot.

Allow the D-Phy config checks to use mode->clock instead of
mode->crtc_clock during mode_valid checks, like everywhere else in the
driver.

Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 7457d38622b0..557b037bbc67 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -574,7 +574,7 @@ static int cdns_dsi_check_conf(struct cdns_dsi *dsi,
 	if (ret)
 		return ret;
 
-	phy_mipi_dphy_get_default_config(mode->crtc_clock * 1000,
+	phy_mipi_dphy_get_default_config((mode_valid_check ? mode->clock : mode->crtc_clock) * 1000,
 					 mipi_dsi_pixel_format_to_bpp(output->dev->format),
 					 nlanes, phy_cfg);
 
@@ -775,8 +775,8 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 
 	WARN_ON_ONCE(cdns_dsi_check_conf(dsi, mode, &dsi_cfg, false));
 
-	cdns_dsi_hs_init(dsi);
 	cdns_dsi_init_link(dsi);
+	cdns_dsi_hs_init(dsi);
 
 	writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
 	       dsi->regs + VID_HSIZE1);
@@ -952,7 +952,7 @@ static int cdns_dsi_attach(struct mipi_dsi_host *host,
 		bridge = drm_panel_bridge_add_typed(panel,
 						    DRM_MODE_CONNECTOR_DSI);
 	} else {
-		bridge = of_drm_find_bridge(dev->dev.of_node);
+		bridge = of_drm_find_bridge(np);
 		if (!bridge)
 			bridge = ERR_PTR(-EINVAL);
 	}
@@ -1153,6 +1153,7 @@ static int __maybe_unused cdns_dsi_suspend(struct device *dev)
 	clk_disable_unprepare(dsi->dsi_p_clk);
 	reset_control_assert(dsi->dsi_p_rst);
 	dsi->link_initialized = false;
+	dsi->phy_initialized = false;
 	return 0;
 }
 
-- 
2.34.1

Re: [PATCH 2/7] drm/bridge: cdns-dsi: Fix minor bugs

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 875 bytes --]

On Sat, May 11, 2024 at 09:00:46PM +0530, Aradhya Bhatia wrote:
> Update the Phy initialized state to "not initialized" when the driver
> (and the hardware by extension) gets suspended. This will allow the Phy
> to get initialized again after resume.
> 
> Fix the OF node that gets passed to find the next available bridge in
> the display pipeline.
> 
> Fix the order of DSI Link and DSI Phy inits. The link init needs to
> happen before the Phy is initialized, so the Phy can lock on the
> incoming PLL reference clock. If this doesn't happen, the Phy cannot
> lock (until DSI Link is init later on). This causes a warning dump
> during the kernel boot.
> 
> Allow the D-Phy config checks to use mode->clock instead of
> mode->crtc_clock during mode_valid checks, like everywhere else in the
> driver.

All these should be in separate patches.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 2/7] drm/bridge: cdns-dsi: Fix minor bugs

[Aradhya Bhatia]

On 16/05/24 13:41, Maxime Ripard wrote:
> On Sat, May 11, 2024 at 09:00:46PM +0530, Aradhya Bhatia wrote:
>> Update the Phy initialized state to "not initialized" when the driver
>> (and the hardware by extension) gets suspended. This will allow the Phy
>> to get initialized again after resume.
>>
>> Fix the OF node that gets passed to find the next available bridge in
>> the display pipeline.
>>
>> Fix the order of DSI Link and DSI Phy inits. The link init needs to
>> happen before the Phy is initialized, so the Phy can lock on the
>> incoming PLL reference clock. If this doesn't happen, the Phy cannot
>> lock (until DSI Link is init later on). This causes a warning dump
>> during the kernel boot.
>>
>> Allow the D-Phy config checks to use mode->clock instead of
>> mode->crtc_clock during mode_valid checks, like everywhere else in the
>> driver.
> 
> All these should be in separate patches.
> 

Alright! I will split them into different patches in v2.

Regards
Aradhya

[PATCH 3/7] drm/bridge: cdns-dsi: Wait for Clk and Data Lanes to be ready

[Aradhya Bhatia]

Once the DSI Link and DSI Phy are initialized, the code needs to wait
for Clk and Data Lanes to be ready, before continuing configuration.
This is in accordance with the DSI Start-up procedure, found in the
Technical Reference Manual of Texas Instrument's J721E SoC[0] which
houses this DSI TX controller.

If the previous bridge (or crtc/encoder) are configured pre-maturely,
the input signal FIFO gets corrupt. This introduces a color-shift on the
display.

Allow the driver to wait for the clk and data lanes to get ready during
DSI enable.

Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 557b037bbc67..05d2f4cc50da 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -761,7 +761,7 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 	struct phy_configure_opts_mipi_dphy *phy_cfg = &output->phy_opts.mipi_dphy;
 	unsigned long tx_byte_period;
 	struct cdns_dsi_cfg dsi_cfg;
-	u32 tmp, reg_wakeup, div;
+	u32 tmp, reg_wakeup, div, status;
 	int nlanes;
 
 	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
@@ -778,6 +778,17 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 	cdns_dsi_init_link(dsi);
 	cdns_dsi_hs_init(dsi);
 
+	/*
+	 * Now that the DSI Link and DSI Phy are initialized,
+	 * wait for the CLK and Data Lanes to be ready.
+	 */
+	tmp = CLK_LANE_RDY;
+	for (int i = 0; i < nlanes; i++)
+		tmp |= DATA_LANE_RDY(i);
+
+	WARN_ON_ONCE(readl_poll_timeout(dsi->regs + MCTL_MAIN_STS, status,
+					status & tmp, 100, 0));
+
 	writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
 	       dsi->regs + VID_HSIZE1);
 	writel(HFP_LEN(dsi_cfg.hfp) | HACT_LEN(dsi_cfg.hact),
-- 
2.34.1

Re: [EXTERNAL] [PATCH 3/7] drm/bridge: cdns-dsi: Wait for Clk and Data Lanes to be ready

[Dominik Haller]

Hi Aradhya,

this fixes the color-shift bug on our setup:
j721s2 -> ti,sn65dsi83 -> edt,etml1010g3dra



Tested-by: Dominik Haller <d.haller@phytec.de>


On Sat, 2024-05-11 at 21:00 +0530, Aradhya Bhatia wrote:
> Once the DSI Link and DSI Phy are initialized, the code needs to wait
> for Clk and Data Lanes to be ready, before continuing configuration.
> This is in accordance with the DSI Start-up procedure, found in the
> Technical Reference Manual of Texas Instrument's J721E SoC[0] which
> houses this DSI TX controller.
> 
> If the previous bridge (or crtc/encoder) are configured pre-maturely,
> the input signal FIFO gets corrupt. This introduces a color-shift on
> the
> display.
> 
> Allow the driver to wait for the clk and data lanes to get ready
> during
> DSI enable.
> 
> Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
> Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
> ---
>  drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> index 557b037bbc67..05d2f4cc50da 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -761,7 +761,7 @@ static void cdns_dsi_bridge_enable(struct
> drm_bridge *bridge)
>  	struct phy_configure_opts_mipi_dphy *phy_cfg = &output-
> >phy_opts.mipi_dphy;
>  	unsigned long tx_byte_period;
>  	struct cdns_dsi_cfg dsi_cfg;
> -	u32 tmp, reg_wakeup, div;
> +	u32 tmp, reg_wakeup, div, status;
>  	int nlanes;
>  
>  	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
> @@ -778,6 +778,17 @@ static void cdns_dsi_bridge_enable(struct
> drm_bridge *bridge)
>  	cdns_dsi_init_link(dsi);
>  	cdns_dsi_hs_init(dsi);
>  
> +	/*
> +	 * Now that the DSI Link and DSI Phy are initialized,
> +	 * wait for the CLK and Data Lanes to be ready.
> +	 */
> +	tmp = CLK_LANE_RDY;
> +	for (int i = 0; i < nlanes; i++)
> +		tmp |= DATA_LANE_RDY(i);
> +
> +	WARN_ON_ONCE(readl_poll_timeout(dsi->regs + MCTL_MAIN_STS,
> status,
> +					status & tmp, 100, 0));
> +
>  	writel(HBP_LEN(dsi_cfg.hbp) | HSA_LEN(dsi_cfg.hsa),
>  	       dsi->regs + VID_HSIZE1);
>  	writel(HFP_LEN(dsi_cfg.hfp) | HACT_LEN(dsi_cfg.hact),

[PATCH 4/7] drm/bridge: cdns-dsi: Reset the DCS write FIFO

[Aradhya Bhatia]

Allow the DCS Write FIFO in the cdns-dsi controller to reset before any
DCS packet is transmitted to the DSI sink device.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 05d2f4cc50da..87fdd07ca0bc 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -1037,6 +1037,9 @@ static ssize_t cdns_dsi_transfer(struct mipi_dsi_host *host,
 
 	cdns_dsi_init_link(dsi);
 
+	/* Reset the DCS Write FIFO */
+	writel(0x00, dsi->regs + DIRECT_CMD_FIFO_RST);
+
 	ret = mipi_dsi_create_packet(&packet, msg);
 	if (ret)
 		goto out;
-- 
2.34.1

[PATCH 5/7] drm/bridge: cdns-dsi: Support atomic bridge APIs

[Aradhya Bhatia]

Change the existing (and deparacated) bridge hooks, to the bridge
atomic APIs.

Add drm helpers for duplicate_state, destroy_state, and bridge_reset
bridge hooks.

Further add support for the input format negotiation hook.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 70 ++++++++++++++++---
 1 file changed, 62 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 87fdd07ca0bc..c7519d18e94f 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -13,6 +13,7 @@
 #include <linux/clk.h>
 #include <linux/interrupt.h>
 #include <linux/iopoll.h>
+#include <linux/media-bus-format.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/of_graph.h>
@@ -655,7 +656,8 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
+					   struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -675,7 +677,8 @@ static void cdns_dsi_bridge_disable(struct drm_bridge *bridge)
 	pm_runtime_put(dsi->base.dev);
 }
 
-static void cdns_dsi_bridge_post_disable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_post_disable(struct drm_bridge *bridge,
+					 struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -752,7 +755,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
 	dsi->link_initialized = true;
 }
 
-static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
+					  struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -903,7 +907,8 @@ static void cdns_dsi_bridge_enable(struct drm_bridge *bridge)
 	writel(tmp, dsi->regs + MCTL_MAIN_EN);
 }
 
-static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
+static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge,
+				       struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -915,13 +920,62 @@ static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge)
 	cdns_dsi_hs_init(dsi);
 }
 
+static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
+					       struct drm_bridge_state *bridge_state,
+					       struct drm_crtc_state *crtc_state,
+					       struct drm_connector_state *conn_state,
+					       u32 output_fmt,
+					       unsigned int *num_input_fmts)
+{
+	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
+	struct cdns_dsi *dsi = input_to_dsi(input);
+	struct cdns_dsi_output *output = &dsi->output;
+	u32 *input_fmts;
+
+	*num_input_fmts = 0;
+
+	input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+	if (!input_fmts)
+		return NULL;
+
+	switch (output->dev->format) {
+	case MIPI_DSI_FMT_RGB888:
+		input_fmts[0] = MEDIA_BUS_FMT_RGB888_1X24;
+		break;
+
+	case MIPI_DSI_FMT_RGB666:
+		input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X24_CPADHI;
+		break;
+
+	case MIPI_DSI_FMT_RGB666_PACKED:
+		input_fmts[0] = MEDIA_BUS_FMT_RGB666_1X18;
+		break;
+
+	case MIPI_DSI_FMT_RGB565:
+		input_fmts[0] = MEDIA_BUS_FMT_RGB565_1X16;
+		break;
+
+	default:
+		/* Unsupported DSI Format */
+		return NULL;
+	}
+
+	*num_input_fmts = 1;
+
+	return input_fmts;
+}
+
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,
-	.disable = cdns_dsi_bridge_disable,
-	.pre_enable = cdns_dsi_bridge_pre_enable,
-	.enable = cdns_dsi_bridge_enable,
-	.post_disable = cdns_dsi_bridge_post_disable,
+	.atomic_disable = cdns_dsi_bridge_atomic_disable,
+	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
+	.atomic_enable = cdns_dsi_bridge_atomic_enable,
+	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
+	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+	.atomic_reset = drm_atomic_helper_bridge_reset,
+	.atomic_get_input_bus_fmts = cdns_dsi_bridge_get_input_bus_fmts,
 };
 
 static int cdns_dsi_attach(struct mipi_dsi_host *host,
-- 
2.34.1

Re: [PATCH 5/7] drm/bridge: cdns-dsi: Support atomic bridge APIs

[Markus Elfring]

> Change the existing (and deparacated) bridge hooks, …
                           deprecated?

Regards,
Markus

[PATCH 6/7] drm/bridge: Introduce early_enable and late disable

[Aradhya Bhatia]

With the existing pre_enable and enable function hooks, the order of
enable of display elements looks as follows,

crtc_enable -> bridge[n]_pre_enable -> ... -> bridge[1]_pre_enable ->
encoder_enable -> bridge[1]_enable -> ... -> bridge[n]_enable

and vice versa for the disable.

Some bridges need to be up and running before and after their source
gets enabled and has run. In some case, that source is a display unit,
controlled as part of &drm_crtc.

For those bridges, add support for early_enable and late_disable
function hooks.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 67 +++++++++++++++++++++++
 drivers/gpu/drm/drm_bridge.c        | 84 +++++++++++++++++++++++++++++
 include/drm/drm_bridge.h            | 73 +++++++++++++++++++++++++
 3 files changed, 224 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fb97b51b38f1..396bb83e4296 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1234,6 +1234,49 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
 		if (ret == 0)
 			drm_crtc_vblank_put(crtc);
 	}
+
+	for_each_oldnew_connector_in_state(old_state, connector, old_conn_state,
+					   new_conn_state, i) {
+		struct drm_encoder *encoder;
+		struct drm_bridge *bridge;
+
+		/*
+		 * Shut down everything that's in the changeset and currently
+		 * still on. So need to check the old, saved state.
+		 */
+		if (!old_conn_state->crtc)
+			continue;
+
+		old_crtc_state = drm_atomic_get_old_crtc_state(old_state, old_conn_state->crtc);
+
+		if (new_conn_state->crtc)
+			new_crtc_state = drm_atomic_get_new_crtc_state(old_state,
+								       new_conn_state->crtc);
+		else
+			new_crtc_state = NULL;
+
+		if (!crtc_needs_disable(old_crtc_state, new_crtc_state) ||
+		    !drm_atomic_crtc_needs_modeset(old_conn_state->crtc->state))
+			continue;
+
+		encoder = old_conn_state->best_encoder;
+
+		/* We shouldn't get this far if we didn't previously have
+		 * an encoder.. but WARN_ON() rather than explode.
+		 */
+		if (WARN_ON(!encoder))
+			continue;
+
+		drm_dbg_atomic(dev, "disabling [ENCODER:%d:%s]\n",
+			       encoder->base.id, encoder->name);
+
+		/*
+		 * Each encoder has at most one connector (since we always steal
+		 * it away), so we won't call disable hooks twice.
+		 */
+		bridge = drm_bridge_chain_get_first_bridge(encoder);
+		drm_atomic_bridge_chain_late_disable(bridge, old_state);
+	}
 }
 
 /**
@@ -1469,6 +1512,30 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
 	struct drm_connector_state *new_conn_state;
 	int i;
 
+	for_each_new_connector_in_state(old_state, connector, new_conn_state, i) {
+		struct drm_encoder *encoder;
+		struct drm_bridge *bridge;
+
+		if (!new_conn_state->best_encoder)
+			continue;
+
+		if (!new_conn_state->crtc->state->active ||
+		    !drm_atomic_crtc_needs_modeset(new_conn_state->crtc->state))
+			continue;
+
+		encoder = new_conn_state->best_encoder;
+
+		drm_dbg_atomic(dev, "enabling [ENCODER:%d:%s]\n",
+			       encoder->base.id, encoder->name);
+
+		/*
+		 * Each encoder has at most one connector (since we always steal
+		 * it away), so we won't call enable hooks twice.
+		 */
+		bridge = drm_bridge_chain_get_first_bridge(encoder);
+		drm_atomic_bridge_chain_early_enable(bridge, old_state);
+	}
+
 	for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
 		const struct drm_crtc_helper_funcs *funcs;
 
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 28abe9aa99ca..b9070f6b3554 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -623,6 +623,50 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_disable);
 
+/**
+ * drm_atomic_bridge_chain_late_disable - disables all bridges in the encoder
+ *					  chain after crtc is disabled.
+ * @bridge: bridge control structure
+ * @old_state: old atomic state
+ *
+ * Calls &drm_bridge_funcs.atomic_late_disable (falls back on
+ * &drm_bridge_funcs.late_disable) op for all the bridges in the
+ * encoder chain, starting from the last bridge to the first. These are called
+ * after calling &drm_crtc_helper_funcs.atomic_disable.
+ *
+ * Note: the bridge passed should be the one closest to the encoder
+ */
+void drm_atomic_bridge_chain_late_disable(struct drm_bridge *bridge,
+					  struct drm_atomic_state *old_state)
+{
+	struct drm_encoder *encoder;
+	struct drm_bridge *iter;
+
+	if (!bridge)
+		return;
+
+	encoder = bridge->encoder;
+	list_for_each_entry_reverse(iter, &encoder->bridge_chain, chain_node) {
+		if (iter->funcs->atomic_late_disable) {
+			struct drm_bridge_state *old_bridge_state;
+
+			old_bridge_state =
+				drm_atomic_get_old_bridge_state(old_state,
+								iter);
+			if (WARN_ON(!old_bridge_state))
+				return;
+
+			iter->funcs->atomic_late_disable(iter, old_bridge_state);
+		} else if (iter->funcs->late_disable) {
+			iter->funcs->late_disable(iter);
+		}
+
+		if (iter == bridge)
+			break;
+	}
+}
+EXPORT_SYMBOL(drm_atomic_bridge_chain_late_disable);
+
 static void drm_atomic_bridge_call_post_disable(struct drm_bridge *bridge,
 						struct drm_atomic_state *old_state)
 {
@@ -728,6 +772,46 @@ void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
 }
 EXPORT_SYMBOL(drm_atomic_bridge_chain_post_disable);
 
+/**
+ * drm_atomic_bridge_chain_early_enable - enables all bridges in the encoder
+ *					  chain before it's crtc is enabled
+ * @bridge: bridge control structure
+ * @old_state: old atomic state
+ *
+ * Calls &drm_bridge_funcs.atomic_early_enable (falls back on
+ * &drm_bridge_funcs.early_enable) op for all the bridges in the
+ * encoder chain, starting from the first bridge to the last. These are called
+ * before even the &drm_crtc_helper_funcs.atomic_enable is called.
+ *
+ * Note: the bridge passed should be the one closest to the encoder.
+ */
+void drm_atomic_bridge_chain_early_enable(struct drm_bridge *bridge,
+					  struct drm_atomic_state *old_state)
+{
+	struct drm_encoder *encoder;
+
+	if (!bridge)
+		return;
+
+	encoder = bridge->encoder;
+	list_for_each_entry_from(bridge, &encoder->bridge_chain, chain_node) {
+		if (bridge->funcs->atomic_early_enable) {
+			struct drm_bridge_state *old_bridge_state;
+
+			old_bridge_state =
+				drm_atomic_get_old_bridge_state(old_state,
+								bridge);
+			if (WARN_ON(!old_bridge_state))
+				return;
+
+			bridge->funcs->atomic_early_enable(bridge, old_bridge_state);
+		} else if (bridge->funcs->early_enable) {
+			bridge->funcs->early_enable(bridge);
+		}
+	}
+}
+EXPORT_SYMBOL(drm_atomic_bridge_chain_early_enable);
+
 static void drm_atomic_bridge_call_pre_enable(struct drm_bridge *bridge,
 					      struct drm_atomic_state *old_state)
 {
diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
index 4baca0d9107b..40f93230abb2 100644
--- a/include/drm/drm_bridge.h
+++ b/include/drm/drm_bridge.h
@@ -206,6 +206,20 @@ struct drm_bridge_funcs {
 	 */
 	void (*post_disable)(struct drm_bridge *bridge);
 
+	/**
+	 * @late_disable:
+	 *
+	 * This callback should disable the bridge. It is called right after the
+	 * preceding element in the display pipe is disabled. If the preceding
+	 * element is a bridge this means it's called after that bridge's
+	 * @atomic_post_disable. If the preceding element is a &drm_crtc it's
+	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
+	 * hook.
+	 *
+	 * The @late_disable callback is optional.
+	 */
+	void (*late_disable)(struct drm_bridge *bridge);
+
 	/**
 	 * @mode_set:
 	 *
@@ -235,6 +249,26 @@ struct drm_bridge_funcs {
 	void (*mode_set)(struct drm_bridge *bridge,
 			 const struct drm_display_mode *mode,
 			 const struct drm_display_mode *adjusted_mode);
+
+	/**
+	 * @early_enable:
+	 *
+	 * This callback should enable the bridge. It is called right before
+	 * the preceding element in the display pipe is enabled. If the
+	 * preceding element is a bridge this means it's called before that
+	 * bridge's @early_enable. If the preceding element is a &drm_crtc it's
+	 * called right before the crtc's &drm_crtc_helper_funcs.atomic_enable
+	 * hook.
+	 *
+	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
+	 * will not yet be running when this callback is called. The bridge can
+	 * enable the display link feeding the next bridge in the chain (if
+	 * there is one) when this callback is called.
+	 *
+	 * The @early_enable callback is optional.
+	 */
+	void (*early_enable)(struct drm_bridge *bridge);
+
 	/**
 	 * @pre_enable:
 	 *
@@ -285,6 +319,26 @@ struct drm_bridge_funcs {
 	 */
 	void (*enable)(struct drm_bridge *bridge);
 
+	/**
+	 * @atomic_early_enable:
+	 *
+	 * This callback should enable the bridge. It is called right before
+	 * the preceding element in the display pipe is enabled. If the
+	 * preceding element is a bridge this means it's called before that
+	 * bridge's @atomic_early_enable. If the preceding element is a
+	 * &drm_crtc it's called right before the crtc's
+	 * &drm_crtc_helper_funcs.atomic_enable hook.
+	 *
+	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
+	 * will not yet be running when this callback is called. The bridge can
+	 * enable the display link feeding the next bridge in the chain (if
+	 * there is one) when this callback is called.
+	 *
+	 * The @early_enable callback is optional.
+	 */
+	void (*atomic_early_enable)(struct drm_bridge *bridge,
+				    struct drm_bridge_state *old_bridge_state);
+
 	/**
 	 * @atomic_pre_enable:
 	 *
@@ -361,6 +415,21 @@ struct drm_bridge_funcs {
 	void (*atomic_post_disable)(struct drm_bridge *bridge,
 				    struct drm_bridge_state *old_bridge_state);
 
+	/**
+	 * @atomic_late_disable:
+	 *
+	 * This callback should disable the bridge. It is called right after the
+	 * preceding element in the display pipe is disabled. If the preceding
+	 * element is a bridge this means it's called after that bridge's
+	 * @atomic_late_disable. If the preceding element is a &drm_crtc it's
+	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
+	 * hook.
+	 *
+	 * The @atomic_late_disable callback is optional.
+	 */
+	void (*atomic_late_disable)(struct drm_bridge *bridge,
+				    struct drm_bridge_state *old_bridge_state);
+
 	/**
 	 * @atomic_duplicate_state:
 	 *
@@ -873,6 +942,10 @@ void drm_atomic_bridge_chain_disable(struct drm_bridge *bridge,
 				     struct drm_atomic_state *state);
 void drm_atomic_bridge_chain_post_disable(struct drm_bridge *bridge,
 					  struct drm_atomic_state *state);
+void drm_atomic_bridge_chain_late_disable(struct drm_bridge *bridge,
+					  struct drm_atomic_state *state);
+void drm_atomic_bridge_chain_early_enable(struct drm_bridge *bridge,
+					  struct drm_atomic_state *state);
 void drm_atomic_bridge_chain_pre_enable(struct drm_bridge *bridge,
 					struct drm_atomic_state *state);
 void drm_atomic_bridge_chain_enable(struct drm_bridge *bridge,
-- 
2.34.1

Re: [PATCH 6/7] drm/bridge: Introduce early_enable and late disable

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4716 bytes --]

On Sat, May 11, 2024 at 09:00:50PM +0530, Aradhya Bhatia wrote:
> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
> index 4baca0d9107b..40f93230abb2 100644
> --- a/include/drm/drm_bridge.h
> +++ b/include/drm/drm_bridge.h
> @@ -206,6 +206,20 @@ struct drm_bridge_funcs {
>  	 */
>  	void (*post_disable)(struct drm_bridge *bridge);
>  
> +	/**
> +	 * @late_disable:
> +	 *
> +	 * This callback should disable the bridge. It is called right after the
> +	 * preceding element in the display pipe is disabled. If the preceding
> +	 * element is a bridge this means it's called after that bridge's
> +	 * @atomic_post_disable. If the preceding element is a &drm_crtc it's
> +	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
> +	 * hook.
> +	 *
> +	 * The @late_disable callback is optional.
> +	 */
> +	void (*late_disable)(struct drm_bridge *bridge);
> +
>  	/**
>  	 * @mode_set:
>  	 *
> @@ -235,6 +249,26 @@ struct drm_bridge_funcs {
>  	void (*mode_set)(struct drm_bridge *bridge,
>  			 const struct drm_display_mode *mode,
>  			 const struct drm_display_mode *adjusted_mode);
> +
> +	/**
> +	 * @early_enable:
> +	 *
> +	 * This callback should enable the bridge. It is called right before
> +	 * the preceding element in the display pipe is enabled. If the
> +	 * preceding element is a bridge this means it's called before that
> +	 * bridge's @early_enable. If the preceding element is a &drm_crtc it's
> +	 * called right before the crtc's &drm_crtc_helper_funcs.atomic_enable
> +	 * hook.
> +	 *
> +	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
> +	 * will not yet be running when this callback is called. The bridge can
> +	 * enable the display link feeding the next bridge in the chain (if
> +	 * there is one) when this callback is called.
> +	 *
> +	 * The @early_enable callback is optional.
> +	 */
> +	void (*early_enable)(struct drm_bridge *bridge);
> +

You don't need the legacy option here, just go straight for the atomic one.

>  	/**
>  	 * @pre_enable:
>  	 *
> @@ -285,6 +319,26 @@ struct drm_bridge_funcs {
>  	 */
>  	void (*enable)(struct drm_bridge *bridge);
>  
> +	/**
> +	 * @atomic_early_enable:
> +	 *
> +	 * This callback should enable the bridge. It is called right before
> +	 * the preceding element in the display pipe is enabled. If the
> +	 * preceding element is a bridge this means it's called before that
> +	 * bridge's @atomic_early_enable. If the preceding element is a
> +	 * &drm_crtc it's called right before the crtc's
> +	 * &drm_crtc_helper_funcs.atomic_enable hook.
> +	 *
> +	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
> +	 * will not yet be running when this callback is called. The bridge can
> +	 * enable the display link feeding the next bridge in the chain (if
> +	 * there is one) when this callback is called.
> +	 *
> +	 * The @early_enable callback is optional.
> +	 */
> +	void (*atomic_early_enable)(struct drm_bridge *bridge,
> +				    struct drm_bridge_state *old_bridge_state);
> +
>  	/**
>  	 * @atomic_pre_enable:
>  	 *
> @@ -361,6 +415,21 @@ struct drm_bridge_funcs {
>  	void (*atomic_post_disable)(struct drm_bridge *bridge,
>  				    struct drm_bridge_state *old_bridge_state);
>  
> +	/**
> +	 * @atomic_late_disable:
> +	 *
> +	 * This callback should disable the bridge. It is called right after the
> +	 * preceding element in the display pipe is disabled. If the preceding
> +	 * element is a bridge this means it's called after that bridge's
> +	 * @atomic_late_disable. If the preceding element is a &drm_crtc it's
> +	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
> +	 * hook.
> +	 *
> +	 * The @atomic_late_disable callback is optional.
> +	 */
> +	void (*atomic_late_disable)(struct drm_bridge *bridge,
> +				    struct drm_bridge_state *old_bridge_state);
> +

But more importantly, I don't quite get the use case you're trying to
solve here.

If I got the rest of your series, the Cadence DSI bridge needs to be
powered up before its source is started. You can't use atomic_enable or
atomic_pre_enable because it would start the source before the DSI
bridge. Is that correct?

If it is, then how is it different from what
drm_atomic_bridge_chain_pre_enable is doing? The assumption there is
that it starts enabling bridges last to first, to it should be enabled
before anything starts.

The whole bridge enabling order code starts to be a bit of a mess, so it
would be great if you could list all the order variations we have
currently, and why none work for cdns-dsi.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 6/7] drm/bridge: Introduce early_enable and late disable

[Aradhya Bhatia]

Hi Maxime,

Thank you for reviewing the patches!

On 16/05/24 13:52, Maxime Ripard wrote:
> On Sat, May 11, 2024 at 09:00:50PM +0530, Aradhya Bhatia wrote:
>> diff --git a/include/drm/drm_bridge.h b/include/drm/drm_bridge.h
>> index 4baca0d9107b..40f93230abb2 100644
>> --- a/include/drm/drm_bridge.h
>> +++ b/include/drm/drm_bridge.h
>> @@ -206,6 +206,20 @@ struct drm_bridge_funcs {
>>  	 */
>>  	void (*post_disable)(struct drm_bridge *bridge);
>>  
>> +	/**
>> +	 * @late_disable:
>> +	 *
>> +	 * This callback should disable the bridge. It is called right after the
>> +	 * preceding element in the display pipe is disabled. If the preceding
>> +	 * element is a bridge this means it's called after that bridge's
>> +	 * @atomic_post_disable. If the preceding element is a &drm_crtc it's
>> +	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
>> +	 * hook.
>> +	 *
>> +	 * The @late_disable callback is optional.
>> +	 */
>> +	void (*late_disable)(struct drm_bridge *bridge);
>> +
>>  	/**
>>  	 * @mode_set:
>>  	 *
>> @@ -235,6 +249,26 @@ struct drm_bridge_funcs {
>>  	void (*mode_set)(struct drm_bridge *bridge,
>>  			 const struct drm_display_mode *mode,
>>  			 const struct drm_display_mode *adjusted_mode);
>> +
>> +	/**
>> +	 * @early_enable:
>> +	 *
>> +	 * This callback should enable the bridge. It is called right before
>> +	 * the preceding element in the display pipe is enabled. If the
>> +	 * preceding element is a bridge this means it's called before that
>> +	 * bridge's @early_enable. If the preceding element is a &drm_crtc it's
>> +	 * called right before the crtc's &drm_crtc_helper_funcs.atomic_enable
>> +	 * hook.
>> +	 *
>> +	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
>> +	 * will not yet be running when this callback is called. The bridge can
>> +	 * enable the display link feeding the next bridge in the chain (if
>> +	 * there is one) when this callback is called.
>> +	 *
>> +	 * The @early_enable callback is optional.
>> +	 */
>> +	void (*early_enable)(struct drm_bridge *bridge);
>> +
> 
> You don't need the legacy option here, just go straight for the atomic one.

Got it! I can remove these in v2.

> 
>>  	/**
>>  	 * @pre_enable:
>>  	 *
>> @@ -285,6 +319,26 @@ struct drm_bridge_funcs {
>>  	 */
>>  	void (*enable)(struct drm_bridge *bridge);
>>  
>> +	/**
>> +	 * @atomic_early_enable:
>> +	 *
>> +	 * This callback should enable the bridge. It is called right before
>> +	 * the preceding element in the display pipe is enabled. If the
>> +	 * preceding element is a bridge this means it's called before that
>> +	 * bridge's @atomic_early_enable. If the preceding element is a
>> +	 * &drm_crtc it's called right before the crtc's
>> +	 * &drm_crtc_helper_funcs.atomic_enable hook.
>> +	 *
>> +	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
>> +	 * will not yet be running when this callback is called. The bridge can
>> +	 * enable the display link feeding the next bridge in the chain (if
>> +	 * there is one) when this callback is called.
>> +	 *
>> +	 * The @early_enable callback is optional.
>> +	 */
>> +	void (*atomic_early_enable)(struct drm_bridge *bridge,
>> +				    struct drm_bridge_state *old_bridge_state);
>> +
>>  	/**
>>  	 * @atomic_pre_enable:
>>  	 *
>> @@ -361,6 +415,21 @@ struct drm_bridge_funcs {
>>  	void (*atomic_post_disable)(struct drm_bridge *bridge,
>>  				    struct drm_bridge_state *old_bridge_state);
>>  
>> +	/**
>> +	 * @atomic_late_disable:
>> +	 *
>> +	 * This callback should disable the bridge. It is called right after the
>> +	 * preceding element in the display pipe is disabled. If the preceding
>> +	 * element is a bridge this means it's called after that bridge's
>> +	 * @atomic_late_disable. If the preceding element is a &drm_crtc it's
>> +	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
>> +	 * hook.
>> +	 *
>> +	 * The @atomic_late_disable callback is optional.
>> +	 */
>> +	void (*atomic_late_disable)(struct drm_bridge *bridge,
>> +				    struct drm_bridge_state *old_bridge_state);
>> +
> 
> But more importantly, I don't quite get the use case you're trying to
> solve here.
> 
> If I got the rest of your series, the Cadence DSI bridge needs to be
> powered up before its source is started. You can't use atomic_enable or
> atomic_pre_enable because it would start the source before the DSI
> bridge. Is that correct?
> 

That's right. I cannot use bridge_atomic_pre_enable /
bridge_atomic_enable here. But that's because my source is CRTC, which
gets enabled via crtc_atomic_enable.


> If it is, then how is it different from what
> drm_atomic_bridge_chain_pre_enable is doing? The assumption there is
> that it starts enabling bridges last to first, to it should be enabled
> before anything starts.
> 
> The whole bridge enabling order code starts to be a bit of a mess, so it
> would be great if you could list all the order variations we have
> currently, and why none work for cdns-dsi.
> 

Of course! I can elaborate on the order.

Without my patches (and given there isn't any bridge setting the
"pre_enable_prev_first" flag) the order of enable for any single display
chain, looks like this -

	crtc_enable
	
	bridge[n]_pre_enable
	---
	bridge[1]_pre_enable

	encoder_enable

	bridge[1]_enable
	---
	bridge[n]_enable

The tidss enables at the crtc_enable level, and hence is the first
entity with stream on. cdns-dsi doesn't stand a chance with
bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no
bridge call happening before crtc currently.

That's where the early_enable APIs come into picture. They are being
called before the crtc is even enabled, helping cdns-dsi to be enabled
before tidss. The order then looks like -


	bridge[1]_early_enable
	---
	bridge[n]_early_enable

	crtc_enable
	---
	(the order is same from here on)


Regards
Aradhya

Re: [PATCH 6/7] drm/bridge: Introduce early_enable and late disable

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 4792 bytes --]

Hi,

On Thu, May 16, 2024 at 03:10:15PM GMT, Aradhya Bhatia wrote:
> >>  	/**
> >>  	 * @pre_enable:
> >>  	 *
> >> @@ -285,6 +319,26 @@ struct drm_bridge_funcs {
> >>  	 */
> >>  	void (*enable)(struct drm_bridge *bridge);
> >>  
> >> +	/**
> >> +	 * @atomic_early_enable:
> >> +	 *
> >> +	 * This callback should enable the bridge. It is called right before
> >> +	 * the preceding element in the display pipe is enabled. If the
> >> +	 * preceding element is a bridge this means it's called before that
> >> +	 * bridge's @atomic_early_enable. If the preceding element is a
> >> +	 * &drm_crtc it's called right before the crtc's
> >> +	 * &drm_crtc_helper_funcs.atomic_enable hook.
> >> +	 *
> >> +	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
> >> +	 * will not yet be running when this callback is called. The bridge can
> >> +	 * enable the display link feeding the next bridge in the chain (if
> >> +	 * there is one) when this callback is called.
> >> +	 *
> >> +	 * The @early_enable callback is optional.
> >> +	 */
> >> +	void (*atomic_early_enable)(struct drm_bridge *bridge,
> >> +				    struct drm_bridge_state *old_bridge_state);
> >> +
> >>  	/**
> >>  	 * @atomic_pre_enable:
> >>  	 *
> >> @@ -361,6 +415,21 @@ struct drm_bridge_funcs {
> >>  	void (*atomic_post_disable)(struct drm_bridge *bridge,
> >>  				    struct drm_bridge_state *old_bridge_state);
> >>  
> >> +	/**
> >> +	 * @atomic_late_disable:
> >> +	 *
> >> +	 * This callback should disable the bridge. It is called right after the
> >> +	 * preceding element in the display pipe is disabled. If the preceding
> >> +	 * element is a bridge this means it's called after that bridge's
> >> +	 * @atomic_late_disable. If the preceding element is a &drm_crtc it's
> >> +	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
> >> +	 * hook.
> >> +	 *
> >> +	 * The @atomic_late_disable callback is optional.
> >> +	 */
> >> +	void (*atomic_late_disable)(struct drm_bridge *bridge,
> >> +				    struct drm_bridge_state *old_bridge_state);
> >> +
> > 
> > But more importantly, I don't quite get the use case you're trying to
> > solve here.
> > 
> > If I got the rest of your series, the Cadence DSI bridge needs to be
> > powered up before its source is started. You can't use atomic_enable or
> > atomic_pre_enable because it would start the source before the DSI
> > bridge. Is that correct?
> > 
> 
> That's right. I cannot use bridge_atomic_pre_enable /
> bridge_atomic_enable here. But that's because my source is CRTC, which
> gets enabled via crtc_atomic_enable.
> 
> 
> > If it is, then how is it different from what
> > drm_atomic_bridge_chain_pre_enable is doing? The assumption there is
> > that it starts enabling bridges last to first, to it should be enabled
> > before anything starts.
> > 
> > The whole bridge enabling order code starts to be a bit of a mess, so it
> > would be great if you could list all the order variations we have
> > currently, and why none work for cdns-dsi.
> > 
> 
> Of course! I can elaborate on the order.
> 
> Without my patches (and given there isn't any bridge setting the
> "pre_enable_prev_first" flag) the order of enable for any single display
> chain, looks like this -
> 
> 	crtc_enable
> 	
> 	bridge[n]_pre_enable
> 	---
> 	bridge[1]_pre_enable
> 
> 	encoder_enable
> 
> 	bridge[1]_enable
> 	---
> 	bridge[n]_enable
> 
> The tidss enables at the crtc_enable level, and hence is the first
> entity with stream on. cdns-dsi doesn't stand a chance with
> bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no
> bridge call happening before crtc currently.

Thanks for filling the blanks :)

I assume that since cdns-dsi is a bridge, and it only has a simple
encoder implementation, for it to receive some video signal we need to
enable the CRTC before the bridge.

If so, I think that's the original intent between the bridge pre_enable.
The original documentation had:

  pre_enable: this contains things needed to be done for the bridge
  before this contains things needed to be done for the bridge before
  this contains things needed to be done for the bridge before.

and the current one has:

  The display pipe (i.e. clocks and timing signals) feeding this bridge
  will not yet be running when this callback is called. The bridge must
  not enable the display link feeding the next bridge in the chain (if
  there is one) when this callback is called.

I would say the CRTC is such a source, even more so now that the encoder
is usually transparent, so I think we should instead move the crtc
enable call after the bridge pre_enable.

Would that work?

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 6/7] drm/bridge: Introduce early_enable and late disable

[Aradhya Bhatia]

Hi Maxime,

On 21/05/24 18:45, Maxime Ripard wrote:
> Hi,
> 
> On Thu, May 16, 2024 at 03:10:15PM GMT, Aradhya Bhatia wrote:
>>>>  	/**
>>>>  	 * @pre_enable:
>>>>  	 *
>>>> @@ -285,6 +319,26 @@ struct drm_bridge_funcs {
>>>>  	 */
>>>>  	void (*enable)(struct drm_bridge *bridge);
>>>>  
>>>> +	/**
>>>> +	 * @atomic_early_enable:
>>>> +	 *
>>>> +	 * This callback should enable the bridge. It is called right before
>>>> +	 * the preceding element in the display pipe is enabled. If the
>>>> +	 * preceding element is a bridge this means it's called before that
>>>> +	 * bridge's @atomic_early_enable. If the preceding element is a
>>>> +	 * &drm_crtc it's called right before the crtc's
>>>> +	 * &drm_crtc_helper_funcs.atomic_enable hook.
>>>> +	 *
>>>> +	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
>>>> +	 * will not yet be running when this callback is called. The bridge can
>>>> +	 * enable the display link feeding the next bridge in the chain (if
>>>> +	 * there is one) when this callback is called.
>>>> +	 *
>>>> +	 * The @early_enable callback is optional.
>>>> +	 */
>>>> +	void (*atomic_early_enable)(struct drm_bridge *bridge,
>>>> +				    struct drm_bridge_state *old_bridge_state);
>>>> +
>>>>  	/**
>>>>  	 * @atomic_pre_enable:
>>>>  	 *
>>>> @@ -361,6 +415,21 @@ struct drm_bridge_funcs {
>>>>  	void (*atomic_post_disable)(struct drm_bridge *bridge,
>>>>  				    struct drm_bridge_state *old_bridge_state);
>>>>  
>>>> +	/**
>>>> +	 * @atomic_late_disable:
>>>> +	 *
>>>> +	 * This callback should disable the bridge. It is called right after the
>>>> +	 * preceding element in the display pipe is disabled. If the preceding
>>>> +	 * element is a bridge this means it's called after that bridge's
>>>> +	 * @atomic_late_disable. If the preceding element is a &drm_crtc it's
>>>> +	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
>>>> +	 * hook.
>>>> +	 *
>>>> +	 * The @atomic_late_disable callback is optional.
>>>> +	 */
>>>> +	void (*atomic_late_disable)(struct drm_bridge *bridge,
>>>> +				    struct drm_bridge_state *old_bridge_state);
>>>> +
>>>
>>> But more importantly, I don't quite get the use case you're trying to
>>> solve here.
>>>
>>> If I got the rest of your series, the Cadence DSI bridge needs to be
>>> powered up before its source is started. You can't use atomic_enable or
>>> atomic_pre_enable because it would start the source before the DSI
>>> bridge. Is that correct?
>>>
>>
>> That's right. I cannot use bridge_atomic_pre_enable /
>> bridge_atomic_enable here. But that's because my source is CRTC, which
>> gets enabled via crtc_atomic_enable.
>>
>>
>>> If it is, then how is it different from what
>>> drm_atomic_bridge_chain_pre_enable is doing? The assumption there is
>>> that it starts enabling bridges last to first, to it should be enabled
>>> before anything starts.
>>>
>>> The whole bridge enabling order code starts to be a bit of a mess, so it
>>> would be great if you could list all the order variations we have
>>> currently, and why none work for cdns-dsi.
>>>
>>
>> Of course! I can elaborate on the order.
>>
>> Without my patches (and given there isn't any bridge setting the
>> "pre_enable_prev_first" flag) the order of enable for any single display
>> chain, looks like this -
>>
>> 	crtc_enable
>> 	
>> 	bridge[n]_pre_enable
>> 	---
>> 	bridge[1]_pre_enable
>>
>> 	encoder_enable
>>
>> 	bridge[1]_enable
>> 	---
>> 	bridge[n]_enable
>>
>> The tidss enables at the crtc_enable level, and hence is the first
>> entity with stream on. cdns-dsi doesn't stand a chance with
>> bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no
>> bridge call happening before crtc currently.
> 
> Thanks for filling the blanks :)
> 
> I assume that since cdns-dsi is a bridge, and it only has a simple
> encoder implementation, for it to receive some video signal we need to
> enable the CRTC before the bridge.
> 
> If so, I think that's the original intent between the bridge pre_enable.
> The original documentation had:
> 
>   pre_enable: this contains things needed to be done for the bridge
>   before this contains things needed to be done for the bridge before
>   this contains things needed to be done for the bridge before.
> 
> and the current one has:
> 
>   The display pipe (i.e. clocks and timing signals) feeding this bridge
>   will not yet be running when this callback is called. The bridge must
>   not enable the display link feeding the next bridge in the chain (if
>   there is one) when this callback is called.
> 
> I would say the CRTC is such a source, even more so now that the encoder
> is usually transparent, so I think we should instead move the crtc
> enable call after the bridge pre_enable.

Hmm, if I understand you right, the newer sequence of calls will look
like this,

	bridge[n]_pre_enable
	---
	bridge[1]_pre_enable

	crtc_enable
	encoder_enable

	bridge[1]_enable
	---
	bridge[n]_enable

I do agree with this. This makes sense. CRTC is indeed such a source,
and should ideally be enabled after the bridges are pre_enabled.

> 
> Would that work?
> 

So, this could potentially work, yes. The cdns-dsi would get pre_enabled
after all bridges after cdns-dsi are pre_enabled. But over a quick test
with BBAI64 + RPi Panel, I don't see any issue.

However, the one concern that I have right now, is about breaking any
existing (albeit faulty) implementation which relies on CRTC being
enabled before the bridges are pre_enabled. =)


Regards
Aradhya

Re: [PATCH 6/7] drm/bridge: Introduce early_enable and late disable

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 6384 bytes --]

On Fri, May 24, 2024 at 04:38:13PM GMT, Aradhya Bhatia wrote:
> Hi Maxime,
> 
> On 21/05/24 18:45, Maxime Ripard wrote:
> > Hi,
> > 
> > On Thu, May 16, 2024 at 03:10:15PM GMT, Aradhya Bhatia wrote:
> >>>>  	/**
> >>>>  	 * @pre_enable:
> >>>>  	 *
> >>>> @@ -285,6 +319,26 @@ struct drm_bridge_funcs {
> >>>>  	 */
> >>>>  	void (*enable)(struct drm_bridge *bridge);
> >>>>  
> >>>> +	/**
> >>>> +	 * @atomic_early_enable:
> >>>> +	 *
> >>>> +	 * This callback should enable the bridge. It is called right before
> >>>> +	 * the preceding element in the display pipe is enabled. If the
> >>>> +	 * preceding element is a bridge this means it's called before that
> >>>> +	 * bridge's @atomic_early_enable. If the preceding element is a
> >>>> +	 * &drm_crtc it's called right before the crtc's
> >>>> +	 * &drm_crtc_helper_funcs.atomic_enable hook.
> >>>> +	 *
> >>>> +	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
> >>>> +	 * will not yet be running when this callback is called. The bridge can
> >>>> +	 * enable the display link feeding the next bridge in the chain (if
> >>>> +	 * there is one) when this callback is called.
> >>>> +	 *
> >>>> +	 * The @early_enable callback is optional.
> >>>> +	 */
> >>>> +	void (*atomic_early_enable)(struct drm_bridge *bridge,
> >>>> +				    struct drm_bridge_state *old_bridge_state);
> >>>> +
> >>>>  	/**
> >>>>  	 * @atomic_pre_enable:
> >>>>  	 *
> >>>> @@ -361,6 +415,21 @@ struct drm_bridge_funcs {
> >>>>  	void (*atomic_post_disable)(struct drm_bridge *bridge,
> >>>>  				    struct drm_bridge_state *old_bridge_state);
> >>>>  
> >>>> +	/**
> >>>> +	 * @atomic_late_disable:
> >>>> +	 *
> >>>> +	 * This callback should disable the bridge. It is called right after the
> >>>> +	 * preceding element in the display pipe is disabled. If the preceding
> >>>> +	 * element is a bridge this means it's called after that bridge's
> >>>> +	 * @atomic_late_disable. If the preceding element is a &drm_crtc it's
> >>>> +	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
> >>>> +	 * hook.
> >>>> +	 *
> >>>> +	 * The @atomic_late_disable callback is optional.
> >>>> +	 */
> >>>> +	void (*atomic_late_disable)(struct drm_bridge *bridge,
> >>>> +				    struct drm_bridge_state *old_bridge_state);
> >>>> +
> >>>
> >>> But more importantly, I don't quite get the use case you're trying to
> >>> solve here.
> >>>
> >>> If I got the rest of your series, the Cadence DSI bridge needs to be
> >>> powered up before its source is started. You can't use atomic_enable or
> >>> atomic_pre_enable because it would start the source before the DSI
> >>> bridge. Is that correct?
> >>>
> >>
> >> That's right. I cannot use bridge_atomic_pre_enable /
> >> bridge_atomic_enable here. But that's because my source is CRTC, which
> >> gets enabled via crtc_atomic_enable.
> >>
> >>
> >>> If it is, then how is it different from what
> >>> drm_atomic_bridge_chain_pre_enable is doing? The assumption there is
> >>> that it starts enabling bridges last to first, to it should be enabled
> >>> before anything starts.
> >>>
> >>> The whole bridge enabling order code starts to be a bit of a mess, so it
> >>> would be great if you could list all the order variations we have
> >>> currently, and why none work for cdns-dsi.
> >>>
> >>
> >> Of course! I can elaborate on the order.
> >>
> >> Without my patches (and given there isn't any bridge setting the
> >> "pre_enable_prev_first" flag) the order of enable for any single display
> >> chain, looks like this -
> >>
> >> 	crtc_enable
> >> 	
> >> 	bridge[n]_pre_enable
> >> 	---
> >> 	bridge[1]_pre_enable
> >>
> >> 	encoder_enable
> >>
> >> 	bridge[1]_enable
> >> 	---
> >> 	bridge[n]_enable
> >>
> >> The tidss enables at the crtc_enable level, and hence is the first
> >> entity with stream on. cdns-dsi doesn't stand a chance with
> >> bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no
> >> bridge call happening before crtc currently.
> > 
> > Thanks for filling the blanks :)
> > 
> > I assume that since cdns-dsi is a bridge, and it only has a simple
> > encoder implementation, for it to receive some video signal we need to
> > enable the CRTC before the bridge.
> > 
> > If so, I think that's the original intent between the bridge pre_enable.
> > The original documentation had:
> > 
> >   pre_enable: this contains things needed to be done for the bridge
> >   before this contains things needed to be done for the bridge before
> >   this contains things needed to be done for the bridge before.
> > 
> > and the current one has:
> > 
> >   The display pipe (i.e. clocks and timing signals) feeding this bridge
> >   will not yet be running when this callback is called. The bridge must
> >   not enable the display link feeding the next bridge in the chain (if
> >   there is one) when this callback is called.
> > 
> > I would say the CRTC is such a source, even more so now that the encoder
> > is usually transparent, so I think we should instead move the crtc
> > enable call after the bridge pre_enable.
> 
> Hmm, if I understand you right, the newer sequence of calls will look
> like this,
> 
> 	bridge[n]_pre_enable
> 	---
> 	bridge[1]_pre_enable
> 
> 	crtc_enable
> 	encoder_enable
> 
> 	bridge[1]_enable
> 	---
> 	bridge[n]_enable

Yes :)

> I do agree with this. This makes sense. CRTC is indeed such a source,
> and should ideally be enabled after the bridges are pre_enabled.
> 
> > 
> > Would that work?
> > 
> 
> So, this could potentially work, yes. The cdns-dsi would get pre_enabled
> after all bridges after cdns-dsi are pre_enabled. But over a quick test
> with BBAI64 + RPi Panel, I don't see any issue.
> 
> However, the one concern that I have right now, is about breaking any
> existing (albeit faulty) implementation which relies on CRTC being
> enabled before the bridges are pre_enabled. =)

I don't think it'll be a big deal. If there was a proper encoder driver,
it was probably gating the signal until it's enabled. If there isn't,
then yeah it might disrupt things, but it mostly means that the driver
wasn't properly split between pre_enable and enable.

So I think it's worth trying, and we'll see the outcome.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH 6/7] drm/bridge: Introduce early_enable and late disable

[Aradhya Bhatia]

Hi Maxime,

On 28/05/24 17:13, Maxime Ripard wrote:
> On Fri, May 24, 2024 at 04:38:13PM GMT, Aradhya Bhatia wrote:
>> Hi Maxime,
>>
>> On 21/05/24 18:45, Maxime Ripard wrote:
>>> Hi,
>>>
>>> On Thu, May 16, 2024 at 03:10:15PM GMT, Aradhya Bhatia wrote:
>>>>>>  	/**
>>>>>>  	 * @pre_enable:
>>>>>>  	 *
>>>>>> @@ -285,6 +319,26 @@ struct drm_bridge_funcs {
>>>>>>  	 */
>>>>>>  	void (*enable)(struct drm_bridge *bridge);
>>>>>>  
>>>>>> +	/**
>>>>>> +	 * @atomic_early_enable:
>>>>>> +	 *
>>>>>> +	 * This callback should enable the bridge. It is called right before
>>>>>> +	 * the preceding element in the display pipe is enabled. If the
>>>>>> +	 * preceding element is a bridge this means it's called before that
>>>>>> +	 * bridge's @atomic_early_enable. If the preceding element is a
>>>>>> +	 * &drm_crtc it's called right before the crtc's
>>>>>> +	 * &drm_crtc_helper_funcs.atomic_enable hook.
>>>>>> +	 *
>>>>>> +	 * The display pipe (i.e. clocks and timing signals) feeding this bridge
>>>>>> +	 * will not yet be running when this callback is called. The bridge can
>>>>>> +	 * enable the display link feeding the next bridge in the chain (if
>>>>>> +	 * there is one) when this callback is called.
>>>>>> +	 *
>>>>>> +	 * The @early_enable callback is optional.
>>>>>> +	 */
>>>>>> +	void (*atomic_early_enable)(struct drm_bridge *bridge,
>>>>>> +				    struct drm_bridge_state *old_bridge_state);
>>>>>> +
>>>>>>  	/**
>>>>>>  	 * @atomic_pre_enable:
>>>>>>  	 *
>>>>>> @@ -361,6 +415,21 @@ struct drm_bridge_funcs {
>>>>>>  	void (*atomic_post_disable)(struct drm_bridge *bridge,
>>>>>>  				    struct drm_bridge_state *old_bridge_state);
>>>>>>  
>>>>>> +	/**
>>>>>> +	 * @atomic_late_disable:
>>>>>> +	 *
>>>>>> +	 * This callback should disable the bridge. It is called right after the
>>>>>> +	 * preceding element in the display pipe is disabled. If the preceding
>>>>>> +	 * element is a bridge this means it's called after that bridge's
>>>>>> +	 * @atomic_late_disable. If the preceding element is a &drm_crtc it's
>>>>>> +	 * called right after the crtc's &drm_crtc_helper_funcs.atomic_disable
>>>>>> +	 * hook.
>>>>>> +	 *
>>>>>> +	 * The @atomic_late_disable callback is optional.
>>>>>> +	 */
>>>>>> +	void (*atomic_late_disable)(struct drm_bridge *bridge,
>>>>>> +				    struct drm_bridge_state *old_bridge_state);
>>>>>> +
>>>>>
>>>>> But more importantly, I don't quite get the use case you're trying to
>>>>> solve here.
>>>>>
>>>>> If I got the rest of your series, the Cadence DSI bridge needs to be
>>>>> powered up before its source is started. You can't use atomic_enable or
>>>>> atomic_pre_enable because it would start the source before the DSI
>>>>> bridge. Is that correct?
>>>>>
>>>>
>>>> That's right. I cannot use bridge_atomic_pre_enable /
>>>> bridge_atomic_enable here. But that's because my source is CRTC, which
>>>> gets enabled via crtc_atomic_enable.
>>>>
>>>>
>>>>> If it is, then how is it different from what
>>>>> drm_atomic_bridge_chain_pre_enable is doing? The assumption there is
>>>>> that it starts enabling bridges last to first, to it should be enabled
>>>>> before anything starts.
>>>>>
>>>>> The whole bridge enabling order code starts to be a bit of a mess, so it
>>>>> would be great if you could list all the order variations we have
>>>>> currently, and why none work for cdns-dsi.
>>>>>
>>>>
>>>> Of course! I can elaborate on the order.
>>>>
>>>> Without my patches (and given there isn't any bridge setting the
>>>> "pre_enable_prev_first" flag) the order of enable for any single display
>>>> chain, looks like this -
>>>>
>>>> 	crtc_enable
>>>> 	
>>>> 	bridge[n]_pre_enable
>>>> 	---
>>>> 	bridge[1]_pre_enable
>>>>
>>>> 	encoder_enable
>>>>
>>>> 	bridge[1]_enable
>>>> 	---
>>>> 	bridge[n]_enable
>>>>
>>>> The tidss enables at the crtc_enable level, and hence is the first
>>>> entity with stream on. cdns-dsi doesn't stand a chance with
>>>> bridge_atmoic_pre_enable / bridge_atmoic_enable hooks. And there is no
>>>> bridge call happening before crtc currently.
>>>
>>> Thanks for filling the blanks :)
>>>
>>> I assume that since cdns-dsi is a bridge, and it only has a simple
>>> encoder implementation, for it to receive some video signal we need to
>>> enable the CRTC before the bridge.
>>>
>>> If so, I think that's the original intent between the bridge pre_enable.
>>> The original documentation had:
>>>
>>>   pre_enable: this contains things needed to be done for the bridge
>>>   before this contains things needed to be done for the bridge before
>>>   this contains things needed to be done for the bridge before.
>>>
>>> and the current one has:
>>>
>>>   The display pipe (i.e. clocks and timing signals) feeding this bridge
>>>   will not yet be running when this callback is called. The bridge must
>>>   not enable the display link feeding the next bridge in the chain (if
>>>   there is one) when this callback is called.
>>>
>>> I would say the CRTC is such a source, even more so now that the encoder
>>> is usually transparent, so I think we should instead move the crtc
>>> enable call after the bridge pre_enable.
>>
>> Hmm, if I understand you right, the newer sequence of calls will look
>> like this,
>>
>> 	bridge[n]_pre_enable
>> 	---
>> 	bridge[1]_pre_enable
>>
>> 	crtc_enable
>> 	encoder_enable
>>
>> 	bridge[1]_enable
>> 	---
>> 	bridge[n]_enable
> 
> Yes :)
> 
>> I do agree with this. This makes sense. CRTC is indeed such a source,
>> and should ideally be enabled after the bridges are pre_enabled.
>>
>>>
>>> Would that work?
>>>
>>
>> So, this could potentially work, yes. The cdns-dsi would get pre_enabled
>> after all bridges after cdns-dsi are pre_enabled. But over a quick test
>> with BBAI64 + RPi Panel, I don't see any issue.
>>
>> However, the one concern that I have right now, is about breaking any
>> existing (albeit faulty) implementation which relies on CRTC being
>> enabled before the bridges are pre_enabled. =)
> 
> I don't think it'll be a big deal. If there was a proper encoder driver,
> it was probably gating the signal until it's enabled. If there isn't,
> then yeah it might disrupt things, but it mostly means that the driver
> wasn't properly split between pre_enable and enable.
> 
> So I think it's worth trying, and we'll see the outcome.
> 

Alright! =)

Have made the changes as per your suggestions in v2. Thanks!

Regards
Aradhya

[PATCH 7/7] drm/bridge: cdns-dsi: Implement early_enable and late_disable

[Aradhya Bhatia]

The cdsn-dsi controller requires that it be turned on completely before
the input DPI's source has begun streaming.

Since tidss's videoport starts streaming via crtc enable hooks, we need
cdns-dsi to be up and running before that. Hence, use the early_enable
and late_disable hooks to get pass the color issues.

Signed-off-by: Aradhya Bhatia <a-bhatia1@ti.com>
---
 .../gpu/drm/bridge/cadence/cdns-dsi-core.c    | 36 ++++---------------
 1 file changed, 6 insertions(+), 30 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index c7519d18e94f..53fa20720089 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -656,8 +656,8 @@ cdns_dsi_bridge_mode_valid(struct drm_bridge *bridge,
 	return MODE_OK;
 }
 
-static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
-					   struct drm_bridge_state *old_bridge_state)
+static void cdns_dsi_bridge_atomic_late_disable(struct drm_bridge *bridge,
+						struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -677,15 +677,6 @@ static void cdns_dsi_bridge_atomic_disable(struct drm_bridge *bridge,
 	pm_runtime_put(dsi->base.dev);
 }
 
-static void cdns_dsi_bridge_post_disable(struct drm_bridge *bridge,
-					 struct drm_bridge_state *old_bridge_state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
-	pm_runtime_put(dsi->base.dev);
-}
-
 static void cdns_dsi_hs_init(struct cdns_dsi *dsi)
 {
 	struct cdns_dsi_output *output = &dsi->output;
@@ -755,8 +746,8 @@ static void cdns_dsi_init_link(struct cdns_dsi *dsi)
 	dsi->link_initialized = true;
 }
 
-static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
-					  struct drm_bridge_state *old_bridge_state)
+static void cdns_dsi_bridge_atomic_early_enable(struct drm_bridge *bridge,
+						struct drm_bridge_state *old_bridge_state)
 {
 	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
 	struct cdns_dsi *dsi = input_to_dsi(input);
@@ -907,19 +898,6 @@ static void cdns_dsi_bridge_atomic_enable(struct drm_bridge *bridge,
 	writel(tmp, dsi->regs + MCTL_MAIN_EN);
 }
 
-static void cdns_dsi_bridge_pre_enable(struct drm_bridge *bridge,
-				       struct drm_bridge_state *old_bridge_state)
-{
-	struct cdns_dsi_input *input = bridge_to_cdns_dsi_input(bridge);
-	struct cdns_dsi *dsi = input_to_dsi(input);
-
-	if (WARN_ON(pm_runtime_get_sync(dsi->base.dev) < 0))
-		return;
-
-	cdns_dsi_init_link(dsi);
-	cdns_dsi_hs_init(dsi);
-}
-
 static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 					       struct drm_bridge_state *bridge_state,
 					       struct drm_crtc_state *crtc_state,
@@ -968,10 +946,8 @@ static u32 *cdns_dsi_bridge_get_input_bus_fmts(struct drm_bridge *bridge,
 static const struct drm_bridge_funcs cdns_dsi_bridge_funcs = {
 	.attach = cdns_dsi_bridge_attach,
 	.mode_valid = cdns_dsi_bridge_mode_valid,
-	.atomic_disable = cdns_dsi_bridge_atomic_disable,
-	.atomic_pre_enable = cdns_dsi_bridge_atomic_pre_enable,
-	.atomic_enable = cdns_dsi_bridge_atomic_enable,
-	.atomic_post_disable = cdns_dsi_bridge_atomic_post_disable,
+	.atomic_early_enable = cdns_dsi_bridge_atomic_early_enable,
+	.atomic_late_disable = cdns_dsi_bridge_atomic_late_disable,
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
 	.atomic_reset = drm_atomic_helper_bridge_reset,
-- 
2.34.1