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.
Along with that, this series aims to fix the color-shift issue that has been
going on with the DSI controller. This 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. The fix happens in
2 steps.
1. The bridge pre_enable calls have been shifted before the crtc_enable and
the bridge post_disable calls have been shifted after the crtc_disable.
This has been done as per the definition of bridge pre_enable.
"The display pipe (i.e. clocks and timing signals) feeding this bridge
will not yet be running when this callback is called".
Since CRTC is also a source feeding the bridge, it should not be enabled
before the bridges in the pipeline are pre_enabled.
The sequence of enable after this patch will look like:
bridge[n]_pre_enable
...
bridge[1]_pre_enable
crtc_enable
encoder_enable
bridge[1]_enable
...
bridge[n]_enable
and vice-versa for the bridge chain disable sequence.
2. The cdns-dsi enable / disable sequences have now been moved to pre_enable
and post_disable sequences. This is the only way to have cdns-dsi drivers
be up and ready before the previous entity is enables its streaming.
The DSI also spec 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 to be 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-v2-tests" 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 Procedure" [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-v2-tests
Change Log:
- Changes in v2:
- Drop patch "drm/tidss: Add CRTC mode_fixup"
- Split patch "drm/bridge: cdns-dsi: Fix minor bugs" into 4 separate ones
- Drop support for early_enable/late_disable APIs and instead re-order the
pre_enable / post_disable APIs to be called before / after crtc_enable /
crtc_disable.
- Drop support for early_enable/late_disable in cdns-dsi and use
pre_enable/post_disable APIs instead to do bridge enable/disable.
Previous versions:
v1: https://lore.kernel.org/all/[email protected]/
Aradhya Bhatia (9):
drm/bridge: cdns-dsi: Fix OF node pointer
drm/bridge: cdns-dsi: Fix the phy_initialized variable
drm/bridge: cdns-dsi: Fix the link and phy init order
drm/bridge: cdns-dsi: Fix the clock variable for mode_valid()
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/atomic-helper: Re-order bridge chain pre-enable and post-disable
drm/bridge: cdns-dsi: Use pre_enable/post_disable to enable/disable
.../gpu/drm/bridge/cadence/cdns-dsi-core.c | 91 ++++++++++++++-----
drivers/gpu/drm/drm_atomic_helper.c | 70 +++++++++++++-
2 files changed, 135 insertions(+), 26 deletions(-)
base-commit: 9d99040b1bc8dbf385a8aa535e9efcdf94466e19
--
2.34.1
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: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
1 file changed, 1 insertion(+), 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 371a3453970c..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);
--
2.34.1
Change the existing (and deprecated) 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 <[email protected]>
---
.../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..6a2223ecff7d 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_atomic_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_atomic_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
The cdns-dsi controller requires that it be turned on completely before
the input DPI's source has begun streaming[0]. Not having that, allows
for a small window before cdns-dsi enable and after cdns-dsi disable
where the previous entity (in this case tidss's videoport) to continue
streaming DPI video signals. This small window where cdns-dsi is
disabled but is still receiving signals causes the input FIFO of
cdns-dsi to get corrupted. This causes the colors to shift on the output
display. The colors can either shift by one color component (R->G, G->B,
B->R), or by two color components (R->B, G->R, B->G).
Since tidss's videoport starts streaming via crtc enable hooks, we need
cdns-dsi to be up and running before that. Now that the bridges are
pre_enabled before crtc is enabled, and post_disabled after crtc is
disabled, use the pre_enable and post_disable hooks to get cdns-dsi
ready and running before the tidss videoport to get pass the color shift
issues.
[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
TRM Link: http://www.ti.com/lit/pdf/spruil1
Signed-off-by: Aradhya Bhatia <[email protected]>
---
.../gpu/drm/bridge/cadence/cdns-dsi-core.c | 32 +++----------------
1 file changed, 4 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index 6a2223ecff7d..1eedc7053ef8 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_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);
@@ -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_atomic_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_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);
@@ -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_atomic_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,9 +946,7 @@ 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_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
--
2.34.1
Move the bridge pre_enable call before crtc enable, and the bridge
post_disable call after the crtc disable.
The sequence of enable after this patch will look like:
bridge[n]_pre_enable
...
bridge[1]_pre_enable
crtc_enable
encoder_enable
bridge[1]_enable
...
bridge[n]__enable
and vice-versa for the bridge chain disable sequence.
The definition of bridge pre_enable hook says that,
"The display pipe (i.e. clocks and timing signals) feeding this bridge
will not yet be running when this callback is called".
Since CRTC is also a source feeding the bridge, it should not be enabled
before the bridges in the pipeline are pre_enabled. Fix that by
re-ordering the sequence of bridge pre_enable and bridge post_disable.
Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/drm_atomic_helper.c | 70 +++++++++++++++++++++++++++--
1 file changed, 67 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index fb97b51b38f1..02e093950218 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1186,8 +1186,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
else if (funcs->dpms)
funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
}
-
- drm_atomic_bridge_chain_post_disable(bridge, old_state);
}
for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
@@ -1234,6 +1232,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_post_disable(bridge, old_state);
+ }
}
/**
@@ -1469,6 +1510,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_pre_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;
@@ -1514,7 +1579,6 @@ void drm_atomic_helper_commit_modeset_enables(struct drm_device *dev,
* it away), so we won't call enable hooks twice.
*/
bridge = drm_bridge_chain_get_first_bridge(encoder);
- drm_atomic_bridge_chain_pre_enable(bridge, old_state);
if (funcs) {
if (funcs->atomic_enable)
--
2.34.1
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.
Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
index b016f2ba06bb..42565e253b2d 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -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
The order of init of DSI link and DSI phy is wrong. The DSI link needs
to be configured before the DSI phy is getting configured. Otherwise,
the D-Phy is unable to lock in on the incoming PLL Reference clock[0].
Fix the order of inits.
[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
TRM Link: http://www.ti.com/lit/pdf/spruil1
Fixes: fced5a364dee ("drm/bridge: cdns: Convert to phy framework")
Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
1 file changed, 1 insertion(+), 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 42565e253b2d..371a3453970c 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -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);
--
2.34.1
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.
[0]: See section 12.6.5.7.3 "Start-up Procedure" in J721E SoC TRM
TRM Link: http://www.ti.com/lit/pdf/spruil1
Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Signed-off-by: Aradhya Bhatia <[email protected]>
---
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
Fix the OF node pointer passed to the of_drm_find_bridge() call to find
the next bridge in the display chain.
Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
1 file changed, 1 insertion(+), 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 7457d38622b0..b016f2ba06bb 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
@@ -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);
}
--
2.34.1
Allow the DCS Write FIFO in the cdns-dsi controller to reset before any
DCS packet is transmitted to the DSI sink device.
The DCS FIFO reset is optional. Not all panels require it. But at
least one of the DSI based panel that uses Ilitek ILI9881C (DSI to DPI
bridge) doesn't work with without this reset.
Signed-off-by: Aradhya Bhatia <[email protected]>
---
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
On Thu, May 30, 2024 at 03:06:13PM +0530, Aradhya Bhatia wrote:
> Fix the OF node pointer passed to the of_drm_find_bridge() call to find
> the next bridge in the display chain.
Please describe why, not what. In other words, please describe why you
have to pass np, no device's of_node.
>
> Fixes: e19233955d9e ("drm/bridge: Add Cadence DSI driver")
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c | 2 +-
> 1 file changed, 1 insertion(+), 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 7457d38622b0..b016f2ba06bb 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-dsi-core.c
> @@ -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);
> }
> --
> 2.34.1
>
--
With best wishes
Dmitry
On Thu, May 30, 2024 at 03:06:19PM +0530, Aradhya Bhatia wrote:
> Change the existing (and deprecated) 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 <[email protected]>
> ---
> .../gpu/drm/bridge/cadence/cdns-dsi-core.c | 70 ++++++++++++++++---
> 1 file changed, 62 insertions(+), 8 deletions(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
Minor nit below.
>
> @@ -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)
> +{
This code below looks pretty generic. Would be logical to extract it to
a helper and allow it to be used by other DSI host bridges?
> + 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;
> +}
> +
--
With best wishes
Dmitry
On Thu, May 30, 2024 at 03:06:20PM +0530, Aradhya Bhatia wrote:
> Move the bridge pre_enable call before crtc enable, and the bridge
> post_disable call after the crtc disable.
>
> The sequence of enable after this patch will look like:
>
> bridge[n]_pre_enable
> ...
> bridge[1]_pre_enable
>
> crtc_enable
> encoder_enable
>
> bridge[1]_enable
> ...
> bridge[n]__enable
>
> and vice-versa for the bridge chain disable sequence.
>
> The definition of bridge pre_enable hook says that,
> "The display pipe (i.e. clocks and timing signals) feeding this bridge
> will not yet be running when this callback is called".
>
> Since CRTC is also a source feeding the bridge, it should not be enabled
> before the bridges in the pipeline are pre_enabled. Fix that by
> re-ordering the sequence of bridge pre_enable and bridge post_disable.
>
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 70 +++++++++++++++++++++++++++--
> 1 file changed, 67 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> index fb97b51b38f1..02e093950218 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1186,8 +1186,6 @@ disable_outputs(struct drm_device *dev, struct drm_atomic_state *old_state)
> else if (funcs->dpms)
> funcs->dpms(encoder, DRM_MODE_DPMS_OFF);
> }
> -
> - drm_atomic_bridge_chain_post_disable(bridge, old_state);
> }
>
> for_each_oldnew_crtc_in_state(old_state, crtc, old_crtc_state, new_crtc_state, i) {
> @@ -1234,6 +1232,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);
This duplicates the code in the loop around
drm_atomic_bridge_chain_disable(). Can it be extracted to a separate
function? Code duplication is nearly always a bad idea.
The same comment applies to the next chunk too.
> +
> + /*
> + * 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_post_disable(bridge, old_state);
> + }
> }
>
> /**
--
With best wishes
Dmitry
Hi Dmitry,
Thank you for reviewing the patches.
On 31/05/24 04:51, Dmitry Baryshkov wrote:
> On Thu, May 30, 2024 at 03:06:19PM +0530, Aradhya Bhatia wrote:
>> Change the existing (and deprecated) 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 <[email protected]>
>> ---
>> .../gpu/drm/bridge/cadence/cdns-dsi-core.c | 70 ++++++++++++++++---
>> 1 file changed, 62 insertions(+), 8 deletions(-)
>
> Reviewed-by: Dmitry Baryshkov <[email protected]>
>
> Minor nit below.
>
>>
>> @@ -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)
>> +{
>
> This code below looks pretty generic. Would be logical to extract it to
> a helper and allow it to be used by other DSI host bridges?
I agree, it would indeed make sense.
I just tried to make a helper function that could directly be passed to
the drm_bridge_funcs list - like we do with
"drm_atomic_helper_bridge_propagate_bus_fmt". This would have been ideal
in my opinion.
But, that doesn't seem possible today. The parameter "output_fmt"
doesn't describe any of the DSI pixel formats from "enum
mipi_dsi_pixel_format", which is what's required to ascertain the input
bus format for dsi hosts. Neither do the drm_bridge{_state} help with
that.
For now, I am thinking to just add a common function that accepts the
dsi bus output format and returns the corresponding input bus format.
With this, every dsi host will still need to implement their own
get_input_fmt hook and call that function.
If you had something else in mind, do let me know! =)
Regards
Aradhya
>
>> + 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;
>> +}
>> +
>
On Mon, Jun 10, 2024 at 06:02:41PM +0530, Aradhya Bhatia wrote:
> Hi Dmitry,
>
> Thank you for reviewing the patches.
>
> On 31/05/24 04:51, Dmitry Baryshkov wrote:
> > On Thu, May 30, 2024 at 03:06:19PM +0530, Aradhya Bhatia wrote:
> >> Change the existing (and deprecated) 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 <[email protected]>
> >> ---
> >> .../gpu/drm/bridge/cadence/cdns-dsi-core.c | 70 ++++++++++++++++---
> >> 1 file changed, 62 insertions(+), 8 deletions(-)
> >
> > Reviewed-by: Dmitry Baryshkov <[email protected]>
> >
> > Minor nit below.
> >
> >>
> >> @@ -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)
> >> +{
> >
> > This code below looks pretty generic. Would be logical to extract it to
> > a helper and allow it to be used by other DSI host bridges?
>
> I agree, it would indeed make sense.
>
> I just tried to make a helper function that could directly be passed to
> the drm_bridge_funcs list - like we do with
> "drm_atomic_helper_bridge_propagate_bus_fmt". This would have been ideal
> in my opinion.
>
> But, that doesn't seem possible today. The parameter "output_fmt"
> doesn't describe any of the DSI pixel formats from "enum
> mipi_dsi_pixel_format", which is what's required to ascertain the input
> bus format for dsi hosts. Neither do the drm_bridge{_state} help with
> that.
>
> For now, I am thinking to just add a common function that accepts the
> dsi bus output format and returns the corresponding input bus format.
> With this, every dsi host will still need to implement their own
> get_input_fmt hook and call that function.
>
> If you had something else in mind, do let me know! =)
No, the code that you have described looks pretty good. Please proceed
with the implementation!
>
> Regards
> Aradhya
>
> >
> >> + 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;
> >> +}
> >> +
> >
--
With best wishes
Dmitry