LinuxLists.cc - [PATCH v6 0/8] drm/tidss: Use new connector model for tidss

[permalink] [raw]

Subject: [PATCH v6 6/8] drm/bridge: sii902x: Set input_bus_flags in atomic_check

input_bus_flags are specified in drm_bridge_timings (legacy) as well
as drm_bridge_state->input_bus_cfg.flags

The flags from the timings will be deprecated. Bridges are supposed
to validate and set the bridge state flags from atomic_check.

Implement atomic_check hook for the same.

Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/bridge/sii902x.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index 662b6cb4aa62..4d1f10532fa6 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -495,6 +495,20 @@ static u32 *sii902x_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
return input_fmts;
}

+static int sii902x_bridge_atomic_check(struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
+{
+ /*
+ * There might be flags negotiation supported in future but
+ * set the bus flags in atomic_check statically for now.
+ */
+ bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
+
+ return 0;
+}
+
static const struct drm_bridge_funcs sii902x_bridge_funcs = {
.attach = sii902x_bridge_attach,
.mode_set = sii902x_bridge_mode_set,
@@ -506,6 +520,7 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = {
.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
.atomic_get_input_bus_fmts = sii902x_bridge_atomic_get_input_bus_fmts,
+ .atomic_check = sii902x_bridge_atomic_check,
};

static int sii902x_mute(struct sii902x *sii902x, bool mute)
--
2.40.1

2023-05-09 09:51:18

[permalink] [raw]

Subject: [PATCH v6 7/8] drm/tidss: Update encoder/bridge chain connect model

With the new encoder/bridge chain model, the display controller driver
is required to create a drm_connector entity instead of asking the
bridge to do so during drm_bridge_attach. Moreover, the controller
driver should create a drm_bridge entity to negotiate bus formats and a
'simple' drm_encoder entity to expose it to userspace.

Update the encoder/bridge initialization sequence in tidss as per the
new model.

Signed-off-by: Aradhya Bhatia <[email protected]>
---

Notes:

changes from v5:
* Dropped patches 5 and 6 from the original series.
* Instead added this patch that addresses Boris Brezillon's comments
of creating bridge, simple encoder and connector.

drivers/gpu/drm/tidss/tidss_encoder.c | 140 ++++++++++++++++----------
drivers/gpu/drm/tidss/tidss_encoder.h | 5 +-
drivers/gpu/drm/tidss/tidss_kms.c | 12 +--
3 files changed, 94 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
index 0d4865e9c03d..17a86bed8054 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.c
+++ b/drivers/gpu/drm/tidss/tidss_encoder.c
@@ -6,91 +6,125 @@

#include <linux/export.h>

+#include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
#include <drm/drm_crtc.h>
#include <drm/drm_modeset_helper_vtables.h>
#include <drm/drm_panel.h>
#include <drm/drm_of.h>
+#include <drm/drm_simple_kms_helper.h>

#include "tidss_crtc.h"
#include "tidss_drv.h"
#include "tidss_encoder.h"

-static int tidss_encoder_atomic_check(struct drm_encoder *encoder,
- struct drm_crtc_state *crtc_state,
- struct drm_connector_state *conn_state)
+struct tidss_encoder {
+ struct drm_bridge bridge;
+ struct drm_encoder encoder;
+ struct drm_connector *connector;
+ struct drm_bridge *next_bridge;
+ struct tidss_device *tidss;
+};
+
+static inline struct tidss_encoder
+*bridge_to_tidss_encoder(struct drm_bridge *b)
+{
+ return container_of(b, struct tidss_encoder, bridge);
+}
+
+static int tidss_bridge_attach(struct drm_bridge *bridge,
+ enum drm_bridge_attach_flags flags)
+{
+ struct tidss_encoder *t_enc = bridge_to_tidss_encoder(bridge);
+
+ return drm_bridge_attach(bridge->encoder, t_enc->next_bridge,
+ bridge, flags);
+}
+
+static int tidss_bridge_atomic_check(struct drm_bridge *bridge,
+ struct drm_bridge_state *bridge_state,
+ struct drm_crtc_state *crtc_state,
+ struct drm_connector_state *conn_state)
{
- struct drm_device *ddev = encoder->dev;
+ struct tidss_encoder *t_enc = bridge_to_tidss_encoder(bridge);
+ struct tidss_device *tidss = t_enc->tidss;
struct tidss_crtc_state *tcrtc_state = to_tidss_crtc_state(crtc_state);
struct drm_display_info *di = &conn_state->connector->display_info;
- struct drm_bridge *bridge;
- bool bus_flags_set = false;
-
- dev_dbg(ddev->dev, "%s\n", __func__);
-
- /*
- * Take the bus_flags from the first bridge that defines
- * bridge timings, or from the connector's display_info if no
- * bridge defines the timings.
- */
- drm_for_each_bridge_in_chain(encoder, bridge) {
- if (!bridge->timings)
- continue;
-
- tcrtc_state->bus_flags = bridge->timings->input_bus_flags;
- bus_flags_set = true;
- break;
- }
+ struct drm_bridge_state *next_bridge_state = NULL;
+
+ if (t_enc->next_bridge)
+ next_bridge_state = drm_atomic_get_new_bridge_state(crtc_state->state,
+ t_enc->next_bridge);

- if (!di->bus_formats || di->num_bus_formats == 0) {
- dev_err(ddev->dev, "%s: No bus_formats in connected display\n",
+ if (next_bridge_state) {
+ tcrtc_state->bus_flags = next_bridge_state->input_bus_cfg.flags;
+ tcrtc_state->bus_format = next_bridge_state->input_bus_cfg.format;
+ } else if (di->num_bus_formats) {
+ tcrtc_state->bus_format = di->bus_formats[0];
+ tcrtc_state->bus_flags = di->bus_flags;
+ } else {
+ dev_err(tidss->dev, "%s: No bus_formats in connected display\n",
__func__);
return -EINVAL;
}

- // XXX any cleaner way to set bus format and flags?
- tcrtc_state->bus_format = di->bus_formats[0];
- if (!bus_flags_set)
- tcrtc_state->bus_flags = di->bus_flags;
-
return 0;
}

-static void tidss_encoder_destroy(struct drm_encoder *encoder)
-{
- drm_encoder_cleanup(encoder);
- kfree(encoder);
-}
-
-static const struct drm_encoder_helper_funcs encoder_helper_funcs = {
- .atomic_check = tidss_encoder_atomic_check,
-};
-
-static const struct drm_encoder_funcs encoder_funcs = {
- .destroy = tidss_encoder_destroy,
+static const struct drm_bridge_funcs tidss_bridge_funcs = {
+ .attach = tidss_bridge_attach,
+ .atomic_check = tidss_bridge_atomic_check,
+ .atomic_reset = drm_atomic_helper_bridge_reset,
+ .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
};

-struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
- u32 encoder_type, u32 possible_crtcs)
+int tidss_encoder_create(struct tidss_device *tidss,
+ struct drm_bridge *next_bridge,
+ u32 encoder_type, u32 possible_crtcs)
{
+ struct tidss_encoder *t_enc;
struct drm_encoder *enc;
+ struct drm_connector *connector;
int ret;

- enc = kzalloc(sizeof(*enc), GFP_KERNEL);
- if (!enc)
- return ERR_PTR(-ENOMEM);
+ t_enc = drmm_simple_encoder_alloc(&tidss->ddev, struct tidss_encoder,
+ encoder, encoder_type);
+ if (IS_ERR(t_enc))
+ return PTR_ERR(t_enc);
+
+ t_enc->tidss = tidss;
+ t_enc->next_bridge = next_bridge;
+ t_enc->bridge.funcs = &tidss_bridge_funcs;

+ enc = &t_enc->encoder;
enc->possible_crtcs = possible_crtcs;

- ret = drm_encoder_init(&tidss->ddev, enc, &encoder_funcs,
- encoder_type, NULL);
- if (ret < 0) {
- kfree(enc);
- return ERR_PTR(ret);
+ /* Attaching first bridge to the encoder */
+ ret = drm_bridge_attach(enc, &t_enc->bridge, NULL,
+ DRM_BRIDGE_ATTACH_NO_CONNECTOR);
+ if (ret) {
+ dev_err(tidss->dev, "bridge attach failed: %d\n", ret);
+ return ret;
+ }
+
+ /* Initializing the connector at the end of bridge-chain */
+ connector = drm_bridge_connector_init(&tidss->ddev, enc);
+ if (IS_ERR(connector)) {
+ dev_err(tidss->dev, "bridge_connector create failed\n");
+ return PTR_ERR(connector);
+ }
+
+ ret = drm_connector_attach_encoder(connector, enc);
+ if (ret) {
+ dev_err(tidss->dev, "attaching encoder to connector failed\n");
+ return ret;
}

- drm_encoder_helper_add(enc, &encoder_helper_funcs);
+ t_enc->connector = connector;

dev_dbg(tidss->dev, "Encoder create done\n");

- return enc;
+ return ret;
}
diff --git a/drivers/gpu/drm/tidss/tidss_encoder.h b/drivers/gpu/drm/tidss/tidss_encoder.h
index ace877c0e0fd..3e561d6b1e83 100644
--- a/drivers/gpu/drm/tidss/tidss_encoder.h
+++ b/drivers/gpu/drm/tidss/tidss_encoder.h
@@ -11,7 +11,8 @@

struct tidss_device;

-struct drm_encoder *tidss_encoder_create(struct tidss_device *tidss,
- u32 encoder_type, u32 possible_crtcs);
+int tidss_encoder_create(struct tidss_device *tidss,
+ struct drm_bridge *next_bridge,
+ u32 encoder_type, u32 possible_crtcs);

#endif
diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
index ad2fa3c3d4a7..c979ad1af236 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -193,7 +193,6 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)
for (i = 0; i < num_pipes; ++i) {
struct tidss_plane *tplane;
struct tidss_crtc *tcrtc;
- struct drm_encoder *enc;
u32 hw_plane_id = feat->vid_order[tidss->num_planes];
int ret;

@@ -216,16 +215,13 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss)

tidss->crtcs[tidss->num_crtcs++] = &tcrtc->crtc;

- enc = tidss_encoder_create(tidss, pipes[i].enc_type,
+ ret = tidss_encoder_create(tidss, pipes[i].bridge,
+ pipes[i].enc_type,
1 << tcrtc->crtc.index);
- if (IS_ERR(enc)) {
+ if (ret) {
dev_err(tidss->dev, "encoder create failed\n");
- return PTR_ERR(enc);
- }
-
- ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0);
- if (ret)
return ret;
+ }
}

/* create overlay planes of the leftover planes */
--
2.40.1

2023-05-09 09:51:21

[permalink] [raw]

Subject: [PATCH v6 3/8] drm/bridge: mhdp8546: Add minimal format negotiation

From: Nikhil Devshatwar <[email protected]>

With new connector model, mhdp bridge will not create the connector and
SoC driver will rely on format negotiation to setup the encoder format.

Support minimal format negotiations hooks in the drm_bridge_funcs.
Complete format negotiation can be added based on EDID data.
This patch adds the minimal required support to avoid failure
after moving to new connector model.

Signed-off-by: Nikhil Devshatwar <[email protected]>
Reviewed-by: Tomi Valkeinen <[email protected]>
---

Notes:

changes from v1:
* cosmetic fixes, commit message update.

changes from v5:
* dropped the default_bus_format variable and directly assigned
MEDIA_BUS_FMT_RGB121212_1X36 to input_fmts.

.../drm/bridge/cadence/cdns-mhdp8546-core.c | 25 +++++++++++++++++++
1 file changed, 25 insertions(+)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index f6822dfa3805..623e4235c94f 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2146,6 +2146,30 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge *bridge)
return &cdns_mhdp_state->base;
}

+static u32 *cdns_mhdp_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)
+{
+ u32 *input_fmts;
+
+ *num_input_fmts = 0;
+
+ if (output_fmt != MEDIA_BUS_FMT_FIXED)
+ return NULL;
+
+ input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+ if (!input_fmts)
+ return NULL;
+
+ *num_input_fmts = 1;
+ input_fmts[0] = MEDIA_BUS_FMT_RGB121212_1X36;
+
+ return input_fmts;
+}
+
static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
struct drm_bridge_state *bridge_state,
struct drm_crtc_state *crtc_state,
@@ -2210,6 +2234,7 @@ static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
.atomic_duplicate_state = cdns_mhdp_bridge_atomic_duplicate_state,
.atomic_destroy_state = cdns_mhdp_bridge_atomic_destroy_state,
.atomic_reset = cdns_mhdp_bridge_atomic_reset,
+ .atomic_get_input_bus_fmts = cdns_mhdp_get_input_bus_fmts,
.detect = cdns_mhdp_bridge_detect,
.get_edid = cdns_mhdp_bridge_get_edid,
.hpd_enable = cdns_mhdp_bridge_hpd_enable,
--
2.40.1

2023-05-09 09:51:34

[permalink] [raw]

Subject: [PATCH v6 2/8] drm/bridge: tfp410: Set input_bus_flags in atomic_check

2023-05-09 09:52:02

[permalink] [raw]

Subject: [PATCH v6 1/8] drm/bridge: tfp410: Support format negotiation hooks

From: Nikhil Devshatwar <[email protected]>

With new connector model, tfp410 will not create the connector and
SoC driver will rely on format negotiation to setup the encoder format.

Support format negotiations hooks in the drm_bridge_funcs.
Use helper functions for state management.

Input format is the one selected by the bridge from DT properties.

Signed-off-by: Nikhil Devshatwar <[email protected]>
[a-bhatia1: Removed output fmt condition check]
Signed-off-by: Aradhya Bhatia <[email protected]>
---

Notes:
changes from v1:
* Use only MEDIA_BUS_FMT_FIXED for output

changes from V5:
* Dropped the output format check condition because the output
format for HDMI bridges should be RGB888_1X24 and not FIXED.
Hence, also dropped Tomi Valkeinen's and Laurent Pinchart's
R-b tags.

drivers/gpu/drm/bridge/ti-tfp410.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c
index ab63225cd635..7dacc7e03eee 100644
--- a/drivers/gpu/drm/bridge/ti-tfp410.c
+++ b/drivers/gpu/drm/bridge/ti-tfp410.c
@@ -206,12 +206,38 @@ static enum drm_mode_status tfp410_mode_valid(struct drm_bridge *bridge,
return MODE_OK;
}

+static u32 *tfp410_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 tfp410 *dvi = drm_bridge_to_tfp410(bridge);
+ u32 *input_fmts;
+
+ *num_input_fmts = 0;
+
+ input_fmts = kzalloc(sizeof(*input_fmts), GFP_KERNEL);
+ if (!input_fmts)
+ return NULL;
+
+ *num_input_fmts = 1;
+ input_fmts[0] = dvi->bus_format;
+
+ return input_fmts;
+}
+
static const struct drm_bridge_funcs tfp410_bridge_funcs = {
.attach = tfp410_attach,
.detach = tfp410_detach,
.enable = tfp410_enable,
.disable = tfp410_disable,
.mode_valid = tfp410_mode_valid,
+ .atomic_reset = drm_atomic_helper_bridge_reset,
+ .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+ .atomic_get_input_bus_fmts = tfp410_get_input_bus_fmts,
};

static const struct drm_bridge_timings tfp410_default_timings = {
--
2.40.1

2023-05-09 09:52:47

[permalink] [raw]

Subject: [PATCH v6 5/8] drm/bridge: sii902x: Support format negotiation hooks

With new connector model, sii902x will not create the connector, when
DRM_BRIDGE_ATTACH_NO_CONNECTOR is set and SoC driver will rely on format
negotiation to setup the encoder format.

Support format negotiations hooks in the drm_bridge_funcs.
Use helper functions for state management.

Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
the case with older model.

Signed-off-by: Aradhya Bhatia <[email protected]>
---
drivers/gpu/drm/bridge/sii902x.c | 26 ++++++++++++++++++++++++++
1 file changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
index ef66461e7f7c..662b6cb4aa62 100644
--- a/drivers/gpu/drm/bridge/sii902x.c
+++ b/drivers/gpu/drm/bridge/sii902x.c
@@ -473,6 +473,28 @@ static struct edid *sii902x_bridge_get_edid(struct drm_bridge *bridge,
return sii902x_get_edid(sii902x, connector);
}

+static u32 *sii902x_bridge_atomic_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)
+{
+ u32 *input_fmts;
+ u32 default_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
+
+ *num_input_fmts = 0;
+
+ input_fmts = kcalloc(1, sizeof(*input_fmts), GFP_KERNEL);
+ if (!input_fmts)
+ return NULL;
+
+ input_fmts[0] = default_bus_format;
+ *num_input_fmts = 1;
+
+ return input_fmts;
+}
+
static const struct drm_bridge_funcs sii902x_bridge_funcs = {
.attach = sii902x_bridge_attach,
.mode_set = sii902x_bridge_mode_set,
@@ -480,6 +502,10 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = {
.enable = sii902x_bridge_enable,
.detect = sii902x_bridge_detect,
.get_edid = sii902x_bridge_get_edid,
+ .atomic_reset = drm_atomic_helper_bridge_reset,
+ .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
+ .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
+ .atomic_get_input_bus_fmts = sii902x_bridge_atomic_get_input_bus_fmts,
};

static int sii902x_mute(struct sii902x *sii902x, bool mute)
--
2.40.1

2023-05-09 09:52:47

[permalink] [raw]

Subject: [PATCH v6 8/8] drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable

From: Nikhil Devshatwar <[email protected]>

When removing the tidss driver, there is a warning reported by
kernel about an unhandled interrupt for mhdp driver.

[ 43.238895] irq 31: nobody cared (try booting with the "irqpoll" option)
... [snipped backtrace]
[ 43.330735] handlers:
[ 43.333020] [<000000005367c4f9>] irq_default_primary_handler threaded [<000000007e02b601>]
cdns_mhdp_irq_handler [cdns_mhdp8546]
[ 43.344607] Disabling IRQ #31

This happens because as part of cdns_mhdp_bridge_hpd_disable, driver tries
to disable the interrupts. While disabling the SW_EVENT interrupts,
it accidentally enables the MBOX interrupts, which are not handled by
the driver.

Fix this with a read-modify-write to update only required bits.
Use the enable / disable function as required in other places.

Signed-off-by: Nikhil Devshatwar <[email protected]>
Reviewed-by: Swapnil Jakhade <[email protected]>
Reviewed-by: Tomi Valkeinen <[email protected]>
---

Notes:
changes from v2:
* Fix the interrupt enable logic at other places in code
* Reorder functions so that they can be used earlier in the program

changes from v5:
* Manual rebase to linux-next.

.../drm/bridge/cadence/cdns-mhdp8546-core.c | 44 +++++++++----------
1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index a677b1267525..66a771b140bc 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -54,6 +54,26 @@
#include "cdns-mhdp8546-hdcp.h"
#include "cdns-mhdp8546-j721e.h"

+static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
+{
+ struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
+
+ /* Enable SW event interrupts */
+ if (mhdp->bridge_attached)
+ writel(readl(mhdp->regs + CDNS_APB_INT_MASK) &
+ ~CDNS_APB_INT_MASK_SW_EVENT_INT,
+ mhdp->regs + CDNS_APB_INT_MASK);
+}
+
+static void cdns_mhdp_bridge_hpd_disable(struct drm_bridge *bridge)
+{
+ struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
+
+ writel(readl(mhdp->regs + CDNS_APB_INT_MASK) |
+ CDNS_APB_INT_MASK_SW_EVENT_INT,
+ mhdp->regs + CDNS_APB_INT_MASK);
+}
+
static int cdns_mhdp_mailbox_read(struct cdns_mhdp_device *mhdp)
{
int ret, empty;
@@ -749,9 +769,7 @@ static int cdns_mhdp_fw_activate(const struct firmware *fw,
* MHDP_HW_STOPPED happens only due to driver removal when
* bridge should already be detached.
*/
- if (mhdp->bridge_attached)
- writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
- mhdp->regs + CDNS_APB_INT_MASK);
+ cdns_mhdp_bridge_hpd_enable(&mhdp->bridge);

spin_unlock(&mhdp->start_lock);

@@ -1740,8 +1758,7 @@ static int cdns_mhdp_attach(struct drm_bridge *bridge,

/* Enable SW event interrupts */
if (hw_ready)
- writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
- mhdp->regs + CDNS_APB_INT_MASK);
+ cdns_mhdp_bridge_hpd_enable(bridge);

return 0;
aux_unregister:
@@ -2215,23 +2232,6 @@ static struct edid *cdns_mhdp_bridge_get_edid(struct drm_bridge *bridge,
return cdns_mhdp_get_edid(mhdp, connector);
}

-static void cdns_mhdp_bridge_hpd_enable(struct drm_bridge *bridge)
-{
- struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
-
- /* Enable SW event interrupts */
- if (mhdp->bridge_attached)
- writel(~(u32)CDNS_APB_INT_MASK_SW_EVENT_INT,
- mhdp->regs + CDNS_APB_INT_MASK);
-}
-
-static void cdns_mhdp_bridge_hpd_disable(struct drm_bridge *bridge)
-{
- struct cdns_mhdp_device *mhdp = bridge_to_mhdp(bridge);
-
- writel(CDNS_APB_INT_MASK_SW_EVENT_INT, mhdp->regs + CDNS_APB_INT_MASK);
-}
-
static const struct drm_bridge_funcs cdns_mhdp_bridge_funcs = {
.atomic_enable = cdns_mhdp_atomic_enable,
.atomic_disable = cdns_mhdp_atomic_disable,
--
2.40.1

2023-05-09 10:02:18

[permalink] [raw]

Subject: [PATCH v6 4/8] drm/bridge: mhdp8546: Set input_bus_flags from atomic_check

From: Nikhil Devshatwar <[email protected]>

input_bus_flags are specified in drm_bridge_timings (legacy) as well
as drm_bridge_state->input_bus_cfg.flags

The flags from the timings will be deprecated. Bridges are supposed
to validate and set the bridge state flags from atomic_check.

Signed-off-by: Nikhil Devshatwar <[email protected]>
[a-bhatia1: replace timings in cdns_mhdp_platform_info by input_bus_flags]
Signed-off-by: Aradhya Bhatia <[email protected]>
---

Notes:

changes from v5:
* removed the wrongly addded return statement in tfp410 driver.
* replaced the timings field in cdns_mhdp_platform_info by
input_bus_flags field, in order to get rid of bridge->timings
altogether.

drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 11 ++++++++---
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h | 2 +-
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c | 9 ++++-----
drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h | 2 +-
4 files changed, 14 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
index 623e4235c94f..a677b1267525 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
@@ -2189,6 +2189,13 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
return -EINVAL;
}

+ /*
+ * There might be flags negotiation supported in future.
+ * Set the bus flags in atomic_check statically for now.
+ */
+ if (mhdp->info)
+ bridge_state->input_bus_cfg.flags = *mhdp->info->input_bus_flags;
+
mutex_unlock(&mhdp->link_mutex);
return 0;
}
@@ -2554,8 +2561,6 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
DRM_BRIDGE_OP_HPD;
mhdp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
- if (mhdp->info)
- mhdp->bridge.timings = mhdp->info->timings;

ret = phy_init(mhdp->phy);
if (ret) {
@@ -2642,7 +2647,7 @@ static const struct of_device_id mhdp_ids[] = {
#ifdef CONFIG_DRM_CDNS_MHDP8546_J721E
{ .compatible = "ti,j721e-mhdp8546",
.data = &(const struct cdns_mhdp_platform_info) {
- .timings = &mhdp_ti_j721e_bridge_timings,
+ .input_bus_flags = &mhdp_ti_j721e_bridge_input_bus_flags,
.ops = &mhdp_ti_j721e_ops,
},
},
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
index bedddd510d17..bad2fc0c7306 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
@@ -336,7 +336,7 @@ struct cdns_mhdp_bridge_state {
};

struct cdns_mhdp_platform_info {
- const struct drm_bridge_timings *timings;
+ const u32 *input_bus_flags;
const struct mhdp_platform_ops *ops;
};

diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
index dfe1b59514f7..12d04be4e242 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
@@ -71,8 +71,7 @@ const struct mhdp_platform_ops mhdp_ti_j721e_ops = {
.disable = cdns_mhdp_j721e_disable,
};

-const struct drm_bridge_timings mhdp_ti_j721e_bridge_timings = {
- .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
- DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE |
- DRM_BUS_FLAG_DE_HIGH,
-};
+const u32
+mhdp_ti_j721e_bridge_input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
+ DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE |
+ DRM_BUS_FLAG_DE_HIGH;
diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
index 97d20d115a24..5ddca07a4255 100644
--- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
+++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
@@ -14,6 +14,6 @@
struct mhdp_platform_ops;

extern const struct mhdp_platform_ops mhdp_ti_j721e_ops;
-extern const struct drm_bridge_timings mhdp_ti_j721e_bridge_timings;
+extern const u32 mhdp_ti_j721e_bridge_input_bus_flags;

#endif /* !CDNS_MHDP8546_J721E_H */
--
2.40.1

2023-05-12 09:22:51

[permalink] [raw]

Subject: Re: [PATCH v6 0/8] drm/tidss: Use new connector model for tidss

On 09/05/2023 12:30, Aradhya Bhatia wrote:
> Hi all,
>
> I have picked up this long standing series from Nikhil Devshatwar[1].
>
> This series moves the tidss to using new connectoe model, where the SoC
> driver (tidss) creates the connector and all the bridges are attached
> with the flag DRM_BRIDGE_ATTACH_NO_CONNECTOR. It also now creates bridge
> to support format negotiation and and 'simple' encoder to expose it to
> the userspace.
>
> Since the bridges do not create the connector, the bus_format and
> bus_flag is set via atomic hooks.
>
> Support format negotiations in the tfp410, sii902x and mhdp-8546 bridge
> drivers as a first step before moving the connector model.
>
> These patches were tested on AM625-SK EVM, AM625 SoC based BeaglePlay,
> and J721E-SK. Display support for AM625 SoC has not been added upstream
> and is a WIP. To test this series on AM625 based platforms, basic
> display support patches, (for driver + devicetree), can be found in
> the "next_AttachNoConn" branch on my github fork[2].

Not exactly related to this series, but I was testing this with J7 EVM
and a Dell DP monitor.

I'm seeing DPCD errors quite often, which also sometimes cause link
training errors. E.g.:

cdns-mhdp8546 a000000.dp-bridge: Failed to read DPCD addr 0

cdns-mhdp8546 a000000.dp-bridge: Failed to adjust Link Training.

I tested retrying the read/write when it fails, and that seemed to help
a bit, but not always. Are you using the firmware that's on the upstream
linux-firmware repo?

Tomi

2023-05-12 09:23:28

[permalink] [raw]

Subject: Re: [PATCH v6 3/8] drm/bridge: mhdp8546: Add minimal format negotiation

On 09/05/2023 12:30, Aradhya Bhatia wrote:
> From: Nikhil Devshatwar <[email protected]>
>
> With new connector model, mhdp bridge will not create the connector and
> SoC driver will rely on format negotiation to setup the encoder format.
>
> Support minimal format negotiations hooks in the drm_bridge_funcs.
> Complete format negotiation can be added based on EDID data.
> This patch adds the minimal required support to avoid failure
> after moving to new connector model.
>
> Signed-off-by: Nikhil Devshatwar <[email protected]>
> Reviewed-by: Tomi Valkeinen <[email protected]>

You need to add your SoB to this and the other patches.

> ---
>
> Notes:
>
> changes from v1:
> * cosmetic fixes, commit message update.
>
> changes from v5:
> * dropped the default_bus_format variable and directly assigned
> MEDIA_BUS_FMT_RGB121212_1X36 to input_fmts.
>
> .../drm/bridge/cadence/cdns-mhdp8546-core.c | 25 +++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index f6822dfa3805..623e4235c94f 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2146,6 +2146,30 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge *bridge)
> return &cdns_mhdp_state->base;
> }
>
> +static u32 *cdns_mhdp_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)
> +{
> + u32 *input_fmts;
> +
> + *num_input_fmts = 0;
> +
> + if (output_fmt != MEDIA_BUS_FMT_FIXED)
> + return NULL;

The tfp410 and sii902x drivers don't have the above check. Why does mhdp
need it? Or the other way, why don't tfp410 and sii902x need it?

I guess at the moment we always do get MEDIA_BUS_FMT_FIXED as the out
fmt (in all three bridge drivers), don't we?

Tomi

2023-05-15 16:04:21

[permalink] [raw]

Subject: Re: [PATCH v6 3/8] drm/bridge: mhdp8546: Add minimal format negotiation

Hi Tomi,

On 12-May-23 14:45, Tomi Valkeinen wrote:
> On 09/05/2023 12:30, Aradhya Bhatia wrote:
>> From: Nikhil Devshatwar <[email protected]>
>>
>> With new connector model, mhdp bridge will not create the connector and
>> SoC driver will rely on format negotiation to setup the encoder format.
>>
>> Support minimal format negotiations hooks in the drm_bridge_funcs.
>> Complete format negotiation can be added based on EDID data.
>> This patch adds the minimal required support to avoid failure
>> after moving to new connector model.
>>
>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>> Reviewed-by: Tomi Valkeinen <[email protected]>
>
> You need to add your SoB to this and the other patches.

Okay!

>
>> ---
>>
>> Notes:
>>
>>      changes from v1:
>>      * cosmetic fixes, commit message update.
>>
>>      changes from v5:
>>      * dropped the default_bus_format variable and directly assigned
>>        MEDIA_BUS_FMT_RGB121212_1X36 to input_fmts.
>>
>> .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 25 +++++++++++++++++++
>> 1 file changed, 25 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> index f6822dfa3805..623e4235c94f 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> @@ -2146,6 +2146,30 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge
>> *bridge)
>>       return &cdns_mhdp_state->base;
>> }
>> +static u32 *cdns_mhdp_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)
>> +{
>> +    u32 *input_fmts;
>> +
>> +    *num_input_fmts = 0;
>> +
>> +    if (output_fmt != MEDIA_BUS_FMT_FIXED)
>> +        return NULL;
>
> The tfp410 and sii902x drivers don't have the above check. Why does mhdp
> need it? Or the other way, why don't tfp410 and sii902x need it?

I had removed this condition in order to follow status quo, from the
ITE-66121 HDMI bridge driver.

The idea would have been to drop this for MHDP as well, but I guess I
overlooked this one.

However...

> I guess at the moment we always do get MEDIA_BUS_FMT_FIXED as the out
> fmt (in all three bridge drivers), don't we?

... I tested again to ensure that the above is indeed the case. And
ended up catching some odd behavior.

It turns out that for all the HDMI bridges (TFP410, SII902X, ITE-66121),
the format negotiation doesn't stop at output_fmt = MEDIA_BUS_FMT_FIXED.
The {bridge}_get_input_format API gets called again with the output_fmt
= MEDIA_BUS_FMT_RGB24_1X24.

This doesn't happen with the MHDP driver. Format negotiation with MHDP
bridge stops after one round, at output_fmt = MEDIA_BUS_FMT_FIXED.

Regards
Aradhya

2023-05-16 07:27:27

[permalink] [raw]

Subject: Re: [PATCH v6 5/8] drm/bridge: sii902x: Support format negotiation hooks

On 09/05/2023 11:30, Aradhya Bhatia wrote:
> With new connector model, sii902x will not create the connector, when
> DRM_BRIDGE_ATTACH_NO_CONNECTOR is set and SoC driver will rely on format
> negotiation to setup the encoder format.
>
> Support format negotiations hooks in the drm_bridge_funcs.
> Use helper functions for state management.
>
> Input format is selected to MEDIA_BUS_FMT_RGB888_1X24 as default, as is
> the case with older model.
>
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> drivers/gpu/drm/bridge/sii902x.c | 26 ++++++++++++++++++++++++++
> 1 file changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index ef66461e7f7c..662b6cb4aa62 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -473,6 +473,28 @@ static struct edid *sii902x_bridge_get_edid(struct drm_bridge *bridge,
> return sii902x_get_edid(sii902x, connector);
> }
>
> +static u32 *sii902x_bridge_atomic_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)
> +{
> + u32 *input_fmts;
> + u32 default_bus_format = MEDIA_BUS_FMT_RGB888_1X24;
> +
> + *num_input_fmts = 0;
> +
> + input_fmts = kcalloc(1, sizeof(*input_fmts), GFP_KERNEL);
> + if (!input_fmts)
> + return NULL;
> +
> + input_fmts[0] = default_bus_format;
> + *num_input_fmts = 1;
> +
> + return input_fmts;
> +}
> +
> static const struct drm_bridge_funcs sii902x_bridge_funcs = {
> .attach = sii902x_bridge_attach,
> .mode_set = sii902x_bridge_mode_set,
> @@ -480,6 +502,10 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = {
> .enable = sii902x_bridge_enable,
> .detect = sii902x_bridge_detect,
> .get_edid = sii902x_bridge_get_edid,
> + .atomic_reset = drm_atomic_helper_bridge_reset,
> + .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> + .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> + .atomic_get_input_bus_fmts = sii902x_bridge_atomic_get_input_bus_fmts,
> };
>
> static int sii902x_mute(struct sii902x *sii902x, bool mute)

Reviewed-by: Neil Armstrong <[email protected]>

2023-05-16 07:37:09

[permalink] [raw]

Subject: Re: [PATCH v6 4/8] drm/bridge: mhdp8546: Set input_bus_flags from atomic_check

On 09/05/2023 11:30, Aradhya Bhatia wrote:
> From: Nikhil Devshatwar <[email protected]>
>
> input_bus_flags are specified in drm_bridge_timings (legacy) as well
> as drm_bridge_state->input_bus_cfg.flags
>
> The flags from the timings will be deprecated. Bridges are supposed
> to validate and set the bridge state flags from atomic_check.
>
> Signed-off-by: Nikhil Devshatwar <[email protected]>
> [a-bhatia1: replace timings in cdns_mhdp_platform_info by input_bus_flags]
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
>
> Notes:
>
> changes from v5:
> * removed the wrongly addded return statement in tfp410 driver.
> * replaced the timings field in cdns_mhdp_platform_info by
> input_bus_flags field, in order to get rid of bridge->timings
> altogether.
>
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 11 ++++++++---
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h | 2 +-
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c | 9 ++++-----
> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h | 2 +-
> 4 files changed, 14 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> index 623e4235c94f..a677b1267525 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
> @@ -2189,6 +2189,13 @@ static int cdns_mhdp_atomic_check(struct drm_bridge *bridge,
> return -EINVAL;
> }
>
> + /*
> + * There might be flags negotiation supported in future.
> + * Set the bus flags in atomic_check statically for now.
> + */
> + if (mhdp->info)
> + bridge_state->input_bus_cfg.flags = *mhdp->info->input_bus_flags;
> +
> mutex_unlock(&mhdp->link_mutex);
> return 0;
> }
> @@ -2554,8 +2561,6 @@ static int cdns_mhdp_probe(struct platform_device *pdev)
> mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
> DRM_BRIDGE_OP_HPD;
> mhdp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
> - if (mhdp->info)
> - mhdp->bridge.timings = mhdp->info->timings;

Won't this cause a breakage because at this point in time bridge.timings->input_bus_flags
seems to be still used by tidss right ?

Neil

>
> ret = phy_init(mhdp->phy);
> if (ret) {
> @@ -2642,7 +2647,7 @@ static const struct of_device_id mhdp_ids[] = {
> #ifdef CONFIG_DRM_CDNS_MHDP8546_J721E
> { .compatible = "ti,j721e-mhdp8546",
> .data = &(const struct cdns_mhdp_platform_info) {
> - .timings = &mhdp_ti_j721e_bridge_timings,
> + .input_bus_flags = &mhdp_ti_j721e_bridge_input_bus_flags,
> .ops = &mhdp_ti_j721e_ops,
> },
> },
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> index bedddd510d17..bad2fc0c7306 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
> @@ -336,7 +336,7 @@ struct cdns_mhdp_bridge_state {
> };
>
> struct cdns_mhdp_platform_info {
> - const struct drm_bridge_timings *timings;
> + const u32 *input_bus_flags;
> const struct mhdp_platform_ops *ops;
> };
>
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
> index dfe1b59514f7..12d04be4e242 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
> @@ -71,8 +71,7 @@ const struct mhdp_platform_ops mhdp_ti_j721e_ops = {
> .disable = cdns_mhdp_j721e_disable,
> };
>
> -const struct drm_bridge_timings mhdp_ti_j721e_bridge_timings = {
> - .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> - DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE |
> - DRM_BUS_FLAG_DE_HIGH,
> -};
> +const u32
> +mhdp_ti_j721e_bridge_input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
> + DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE |
> + DRM_BUS_FLAG_DE_HIGH;
> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
> index 97d20d115a24..5ddca07a4255 100644
> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
> @@ -14,6 +14,6 @@
> struct mhdp_platform_ops;
>
> extern const struct mhdp_platform_ops mhdp_ti_j721e_ops;
> -extern const struct drm_bridge_timings mhdp_ti_j721e_bridge_timings;
> +extern const u32 mhdp_ti_j721e_bridge_input_bus_flags;
>
> #endif /* !CDNS_MHDP8546_J721E_H */

2023-05-16 07:38:25

[permalink] [raw]

Subject: Re: [PATCH v6 6/8] drm/bridge: sii902x: Set input_bus_flags in atomic_check

On 09/05/2023 11:30, Aradhya Bhatia wrote:
> input_bus_flags are specified in drm_bridge_timings (legacy) as well
> as drm_bridge_state->input_bus_cfg.flags
>
> The flags from the timings will be deprecated. Bridges are supposed
> to validate and set the bridge state flags from atomic_check.
>
> Implement atomic_check hook for the same.
>
> Signed-off-by: Aradhya Bhatia <[email protected]>
> ---
> drivers/gpu/drm/bridge/sii902x.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/sii902x.c b/drivers/gpu/drm/bridge/sii902x.c
> index 662b6cb4aa62..4d1f10532fa6 100644
> --- a/drivers/gpu/drm/bridge/sii902x.c
> +++ b/drivers/gpu/drm/bridge/sii902x.c
> @@ -495,6 +495,20 @@ static u32 *sii902x_bridge_atomic_get_input_bus_fmts(struct drm_bridge *bridge,
> return input_fmts;
> }
>
> +static int sii902x_bridge_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state *conn_state)
> +{
> + /*
> + * There might be flags negotiation supported in future but
> + * set the bus flags in atomic_check statically for now.
> + */
> + bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
> +
> + return 0;
> +}
> +
> static const struct drm_bridge_funcs sii902x_bridge_funcs = {
> .attach = sii902x_bridge_attach,
> .mode_set = sii902x_bridge_mode_set,
> @@ -506,6 +520,7 @@ static const struct drm_bridge_funcs sii902x_bridge_funcs = {
> .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
> .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
> .atomic_get_input_bus_fmts = sii902x_bridge_atomic_get_input_bus_fmts,
> + .atomic_check = sii902x_bridge_atomic_check,
> };
>
> static int sii902x_mute(struct sii902x *sii902x, bool mute)

Reviewed-by: Neil Armstrong <[email protected]>

2023-05-16 07:44:41

[permalink] [raw]

Subject: Re: [PATCH v6 3/8] drm/bridge: mhdp8546: Add minimal format negotiation

On 15/05/2023 17:59, Aradhya Bhatia wrote:
> Hi Tomi,
>
> On 12-May-23 14:45, Tomi Valkeinen wrote:
>> On 09/05/2023 12:30, Aradhya Bhatia wrote:
>>> From: Nikhil Devshatwar <[email protected]>
>>>
>>> With new connector model, mhdp bridge will not create the connector and
>>> SoC driver will rely on format negotiation to setup the encoder format.
>>>
>>> Support minimal format negotiations hooks in the drm_bridge_funcs.
>>> Complete format negotiation can be added based on EDID data.
>>> This patch adds the minimal required support to avoid failure
>>> after moving to new connector model.
>>>
>>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>>> Reviewed-by: Tomi Valkeinen <[email protected]>
>>
>> You need to add your SoB to this and the other patches.
>
> Okay!
>
>>
>>> ---
>>>
>>> Notes:
>>>
>>>      changes from v1:
>>>      * cosmetic fixes, commit message update.
>>>
>>>      changes from v5:
>>>      * dropped the default_bus_format variable and directly assigned
>>>        MEDIA_BUS_FMT_RGB121212_1X36 to input_fmts.
>>>
>>> .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 25 +++++++++++++++++++
>>> 1 file changed, 25 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> index f6822dfa3805..623e4235c94f 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> @@ -2146,6 +2146,30 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge
>>> *bridge)
>>>       return &cdns_mhdp_state->base;
>>> }
>>> +static u32 *cdns_mhdp_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)
>>> +{
>>> +    u32 *input_fmts;
>>> +
>>> +    *num_input_fmts = 0;
>>> +
>>> +    if (output_fmt != MEDIA_BUS_FMT_FIXED)
>>> +        return NULL;
>>
>> The tfp410 and sii902x drivers don't have the above check. Why does mhdp
>> need it? Or the other way, why don't tfp410 and sii902x need it?
>
> I had removed this condition in order to follow status quo, from the
> ITE-66121 HDMI bridge driver.
>
> The idea would have been to drop this for MHDP as well, but I guess I
> overlooked this one.
>
> However...
>
>> I guess at the moment we always do get MEDIA_BUS_FMT_FIXED as the out
>> fmt (in all three bridge drivers), don't we?
>
> ... I tested again to ensure that the above is indeed the case. And
> ended up catching some odd behavior.
>
> It turns out that for all the HDMI bridges (TFP410, SII902X, ITE-66121),
> the format negotiation doesn't stop at output_fmt = MEDIA_BUS_FMT_FIXED.
> The {bridge}_get_input_format API gets called again with the output_fmt
> = MEDIA_BUS_FMT_RGB24_1X24.
>
> This doesn't happen with the MHDP driver. Format negotiation with MHDP
> bridge stops after one round, at output_fmt = MEDIA_BUS_FMT_FIXED.

This is because the bridge negociation logic will test with all possible
output formats from the chain, and won't stop at first working test.

If your bridge only supports a single input format, it should return the
same format whatever output_fmt is tried.

So indeed remove this test on mhdp aswell, or filter out invalid output
formats.

The MEDIA_BUS_FMT_FIXED is when there's no output format to test, so this
should be always supported.

Neil

>
>
> Regards
> Aradhya

2023-05-16 07:52:42

[permalink] [raw]

Subject: Re: [PATCH v6 2/8] drm/bridge: tfp410: Set input_bus_flags in atomic_check

2023-05-16 14:36:41

[permalink] [raw]

Subject: Re: [PATCH v6 3/8] drm/bridge: mhdp8546: Add minimal format negotiation

Hi Neil,

Thank you for reviewing the patch.

On 16-May-23 12:51, Neil Armstrong wrote:
> On 15/05/2023 17:59, Aradhya Bhatia wrote:
>> Hi Tomi,
>>
>> On 12-May-23 14:45, Tomi Valkeinen wrote:
>>> On 09/05/2023 12:30, Aradhya Bhatia wrote:
>>>> From: Nikhil Devshatwar <[email protected]>
>>>>
>>>> With new connector model, mhdp bridge will not create the connector and
>>>> SoC driver will rely on format negotiation to setup the encoder format.
>>>>
>>>> Support minimal format negotiations hooks in the drm_bridge_funcs.
>>>> Complete format negotiation can be added based on EDID data.
>>>> This patch adds the minimal required support to avoid failure
>>>> after moving to new connector model.
>>>>
>>>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>>>> Reviewed-by: Tomi Valkeinen <[email protected]>
>>>
>>> You need to add your SoB to this and the other patches.
>>
>> Okay!
>>
>>>
>>>> ---
>>>>
>>>> Notes:
>>>>
>>>>       changes from v1:
>>>>       * cosmetic fixes, commit message update.
>>>>
>>>>       changes from v5:
>>>>       * dropped the default_bus_format variable and directly assigned
>>>>         MEDIA_BUS_FMT_RGB121212_1X36 to input_fmts.
>>>>
>>>>    .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 25
>>>> +++++++++++++++++++
>>>>    1 file changed, 25 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> index f6822dfa3805..623e4235c94f 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> @@ -2146,6 +2146,30 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge
>>>> *bridge)
>>>>        return &cdns_mhdp_state->base;
>>>>    }
>>>>    +static u32 *cdns_mhdp_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)
>>>> +{
>>>> +    u32 *input_fmts;
>>>> +
>>>> +    *num_input_fmts = 0;
>>>> +
>>>> +    if (output_fmt != MEDIA_BUS_FMT_FIXED)
>>>> +        return NULL;
>>>
>>> The tfp410 and sii902x drivers don't have the above check. Why does mhdp
>>> need it? Or the other way, why don't tfp410 and sii902x need it?
>>
>> I had removed this condition in order to follow status quo, from the
>> ITE-66121 HDMI bridge driver.
>>
>> The idea would have been to drop this for MHDP as well, but I guess I
>> overlooked this one.
>>
>> However...
>>
>>> I guess at the moment we always do get MEDIA_BUS_FMT_FIXED as the out
>>> fmt (in all three bridge drivers), don't we?
>>
>> ... I tested again to ensure that the above is indeed the case. And
>> ended up catching some odd behavior.
>>
>> It turns out that for all the HDMI bridges (TFP410, SII902X, ITE-66121),
>> the format negotiation doesn't stop at output_fmt = MEDIA_BUS_FMT_FIXED.
>> The {bridge}_get_input_format API gets called again with the output_fmt
>> = MEDIA_BUS_FMT_RGB24_1X24.
>>
>> This doesn't happen with the MHDP driver. Format negotiation with MHDP
>> bridge stops after one round, at output_fmt = MEDIA_BUS_FMT_FIXED.
>
> This is because the bridge negociation logic will test with all possible
> output formats from the chain, and won't stop at first working test.
>
Okay..

> If your bridge only supports a single input format, it should return the
> same format whatever output_fmt is tried.
>
> So indeed remove this test on mhdp aswell, or filter out invalid output
> formats.
Agreed.

I have been looking into the code deeper and trying to understand the
logic flow around the format negotiation in the framework. Here are the
2 points that I want to mention. Please let me know if I have missed
something with my understanding.

Firstly, the mhdp-8546 output connects to the display-connector (with
the compatible, "dp-connector") in the devicetree.

When the negotiation begins at 'drm_atomic_bridge_chain_select_bus_fmts'
the display-connector bridge *should* act as the 'last_bridge', and the
atomic_get_output_bus_fmts hook of the display-connector should get
called. However, for some reason I am not yet sure of, the condition

:: if (last_bridge->funcs->atomic_get_output_bus_fmts)

fails and the 'select_bus_fmt_recursive' function gets called instead,
(with MEDIA_BUS_FMT_FIXED as output_fmt), which in turn calls the
atomic_get_input_bus_fmts hook of the mhdp-8546. This entirely skips the
display-connector out of the format negotiation.

This doesn't happen when the HDMI bridges are in concern, even though,
they too are connected with display-connector (with compatible
"hdmi-connector").

I looked into the display-connector driver hoping to find if the 2 types
of connectors are being treated differently wrt format negotiation, but
I did not find any clue.

Please let me know if you have any idea about this.

Secondly, as mentioned in the display-connector driver, this bridge is
essentially a pass-through. And hence to reflect that, both the format
negotiation hooks of display-connector driver call their counter-parts
from the previous bridge if they are available, and if not, the formats
are assigned MEDIA_BUS_FMT_FIXED.

While this makes sense for the atomic_get_output_bus_fmts hook, it seems
to me that, the same may not hold true for the atomic_get_input_bus_fmts
hook.
If the bridge is indeed a pass-through, should it not also pass the
output_format as its input format (which it actually got from the output
of previous bridge)?

This way all the following will remain same.

1. output_fmt of prev_bridge,
2. input_fmt of display-connector, and
3. output_fmt of display-connector.

Currently, since the atomic_get_input_bus_fmts hook of display-connector
calls its counter-part from the prev_bridge, the input_fmt it passes
(for HDMI bridges) is MEDIA_BUS_FMT_RGB888_1X24. The
atomic_get_ouput_bus_fmts hook of the HDMI bridge has to, then, set an
input bus format considering MEDIA_BUS_FMT_RGB888_1X24 as its output
instead of MEDIA_BUS_FMT_FIXED.

Let me know what you think!

Regards
Aradhya

2023-05-16 15:04:15

[permalink] [raw]

Subject: Re: [PATCH v6 2/8] drm/bridge: tfp410: Set input_bus_flags in atomic_check

Hi Neil,

On 16-May-23 12:55, Neil Armstrong wrote:
> On 09/05/2023 11:30, Aradhya Bhatia wrote:
>> From: Nikhil Devshatwar <[email protected]>
>>
>> input_bus_flags are specified in drm_bridge_timings (legacy) as well
>> as drm_bridge_state->input_bus_cfg.flags
>>
>> The flags from the timings will be deprecated. Bridges are supposed
>> to validate and set the bridge state flags from atomic_check.
>>
>> Implement atomic_check hook for the same.
>>
>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>> ---
>>
>> Notes:
>>      changes from v4:
>>      * fix a warning Reported-by: kernel test robot <[email protected]>
>>
>>      changes from v5:
>>      * Moved the return statement here from patch 4 (where it was added
>>        by mistake).
>>
>> drivers/gpu/drm/bridge/ti-tfp410.c | 16 ++++++++++++++++
>> 1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c
>> b/drivers/gpu/drm/bridge/ti-tfp410.c
>> index 7dacc7e03eee..631ae8f11a77 100644
>> --- a/drivers/gpu/drm/bridge/ti-tfp410.c
>> +++ b/drivers/gpu/drm/bridge/ti-tfp410.c
>> @@ -228,6 +228,21 @@ static u32 *tfp410_get_input_bus_fmts(struct
>> drm_bridge *bridge,
>>       return input_fmts;
>> }
>> +static int tfp410_atomic_check(struct drm_bridge *bridge,
>> +                   struct drm_bridge_state *bridge_state,
>> +                   struct drm_crtc_state *crtc_state,
>> +                   struct drm_connector_state *conn_state)
>> +{
>> +    struct tfp410 *dvi = drm_bridge_to_tfp410(bridge);
>> +
>> +    /*
>> +     * There might be flags negotiation supported in future.
>> +     * Set the bus flags in atomic_check statically for now.
>> +     */
>> +    bridge_state->input_bus_cfg.flags = dvi->timings.input_bus_flags;
>
> A newline here before return would look better
Yup! Will add one.

>
>> +    return 0;
>> +}
>> +
>> static const struct drm_bridge_funcs tfp410_bridge_funcs = {
>>       .attach        = tfp410_attach,
>>       .detach        = tfp410_detach,
>> @@ -238,6 +253,7 @@ static const struct drm_bridge_funcs
>> tfp410_bridge_funcs = {
>>       .atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
>>       .atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
>>       .atomic_get_input_bus_fmts = tfp410_get_input_bus_fmts,
>> +    .atomic_check = tfp410_atomic_check,
>> };
>> static const struct drm_bridge_timings tfp410_default_timings = {
>
> With that fixed:
>
> Reviewed-by: Neil Armstrong <[email protected]>

Thank you!

Regards
Aradhya

2023-05-17 06:00:25

[permalink] [raw]

Subject: Re: [PATCH v6 4/8] drm/bridge: mhdp8546: Set input_bus_flags from atomic_check

Hi Neil,

On 16-May-23 12:54, Neil Armstrong wrote:
> On 09/05/2023 11:30, Aradhya Bhatia wrote:
>> From: Nikhil Devshatwar <[email protected]>
>>
>> input_bus_flags are specified in drm_bridge_timings (legacy) as well
>> as drm_bridge_state->input_bus_cfg.flags
>>
>> The flags from the timings will be deprecated. Bridges are supposed
>> to validate and set the bridge state flags from atomic_check.
>>
>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>> [a-bhatia1: replace timings in cdns_mhdp_platform_info by
>> input_bus_flags]
>> Signed-off-by: Aradhya Bhatia <[email protected]>
>> ---
>>
>> Notes:
>>
>>      changes from v5:
>>      * removed the wrongly addded return statement in tfp410 driver.
>>      * replaced the timings field in cdns_mhdp_platform_info by
>>        input_bus_flags field, in order to get rid of bridge->timings
>>        altogether.
>>
>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 11 ++++++++---
>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h | 2 +-
>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c | 9 ++++-----
>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h | 2 +-
>> 4 files changed, 14 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> index 623e4235c94f..a677b1267525 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>> @@ -2189,6 +2189,13 @@ static int cdns_mhdp_atomic_check(struct
>> drm_bridge *bridge,
>>           return -EINVAL;
>>       }
>> +    /*
>> +     * There might be flags negotiation supported in future.
>> +     * Set the bus flags in atomic_check statically for now.
>> +     */
>> +    if (mhdp->info)
>> +        bridge_state->input_bus_cfg.flags =
>> *mhdp->info->input_bus_flags;
>> +
>>       mutex_unlock(&mhdp->link_mutex);
>>       return 0;
>> }
>> @@ -2554,8 +2561,6 @@ static int cdns_mhdp_probe(struct
>> platform_device *pdev)
>>       mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
>>                  DRM_BRIDGE_OP_HPD;
>>       mhdp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>> -    if (mhdp->info)
>> -        mhdp->bridge.timings = mhdp->info->timings;
>
> Won't this cause a breakage because at this point in time
> bridge.timings->input_bus_flags
> seems to be still used by tidss right ?
>

tidss was using the bridge.timings->input_bus_flags before the 7th
patch[1] in this series.

After the patch, it only relies on bridge_state and display_info for bus
flags and formats.

Regards
Aradhya

[1]: https://lore.kernel.org/all/[email protected]/

>
>>       ret = phy_init(mhdp->phy);
>>       if (ret) {
>> @@ -2642,7 +2647,7 @@ static const struct of_device_id mhdp_ids[] = {
>> #ifdef CONFIG_DRM_CDNS_MHDP8546_J721E
>>       { .compatible = "ti,j721e-mhdp8546",
>>         .data = &(const struct cdns_mhdp_platform_info) {
>> -          .timings = &mhdp_ti_j721e_bridge_timings,
>> +          .input_bus_flags = &mhdp_ti_j721e_bridge_input_bus_flags,
>>             .ops = &mhdp_ti_j721e_ops,
>>         },
>>       },
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> index bedddd510d17..bad2fc0c7306 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>> @@ -336,7 +336,7 @@ struct cdns_mhdp_bridge_state {
>> };
>> struct cdns_mhdp_platform_info {
>> -    const struct drm_bridge_timings *timings;
>> +    const u32 *input_bus_flags;
>>       const struct mhdp_platform_ops *ops;
>> };
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>> index dfe1b59514f7..12d04be4e242 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>> @@ -71,8 +71,7 @@ const struct mhdp_platform_ops mhdp_ti_j721e_ops = {
>>       .disable = cdns_mhdp_j721e_disable,
>> };
>> -const struct drm_bridge_timings mhdp_ti_j721e_bridge_timings = {
>> -    .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
>> -               DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE |
>> -               DRM_BUS_FLAG_DE_HIGH,
>> -};
>> +const u32
>> +mhdp_ti_j721e_bridge_input_bus_flags =
>> DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
>> +                       DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE |
>> +                       DRM_BUS_FLAG_DE_HIGH;
>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>> index 97d20d115a24..5ddca07a4255 100644
>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>> @@ -14,6 +14,6 @@
>> struct mhdp_platform_ops;
>> extern const struct mhdp_platform_ops mhdp_ti_j721e_ops;
>> -extern const struct drm_bridge_timings mhdp_ti_j721e_bridge_timings;
>> +extern const u32 mhdp_ti_j721e_bridge_input_bus_flags;
>> #endif /* !CDNS_MHDP8546_J721E_H */
>

2023-05-22 08:20:20

[permalink] [raw]

Subject: Re: [PATCH v6 4/8] drm/bridge: mhdp8546: Set input_bus_flags from atomic_check

On 17/05/2023 07:48, Aradhya Bhatia wrote:
> Hi Neil,
>
> On 16-May-23 12:54, Neil Armstrong wrote:
>> On 09/05/2023 11:30, Aradhya Bhatia wrote:
>>> From: Nikhil Devshatwar <[email protected]>
>>>
>>> input_bus_flags are specified in drm_bridge_timings (legacy) as well
>>> as drm_bridge_state->input_bus_cfg.flags
>>>
>>> The flags from the timings will be deprecated. Bridges are supposed
>>> to validate and set the bridge state flags from atomic_check.
>>>
>>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>>> [a-bhatia1: replace timings in cdns_mhdp_platform_info by
>>> input_bus_flags]
>>> Signed-off-by: Aradhya Bhatia <[email protected]>
>>> ---
>>>
>>> Notes:
>>>
>>>      changes from v5:
>>>      * removed the wrongly addded return statement in tfp410 driver.
>>>      * replaced the timings field in cdns_mhdp_platform_info by
>>>        input_bus_flags field, in order to get rid of bridge->timings
>>>        altogether.
>>>
>>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 11 ++++++++---
>>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h | 2 +-
>>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c | 9 ++++-----
>>> drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h | 2 +-
>>> 4 files changed, 14 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> index 623e4235c94f..a677b1267525 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>> @@ -2189,6 +2189,13 @@ static int cdns_mhdp_atomic_check(struct
>>> drm_bridge *bridge,
>>>           return -EINVAL;
>>>       }
>>> +    /*
>>> +     * There might be flags negotiation supported in future.
>>> +     * Set the bus flags in atomic_check statically for now.
>>> +     */
>>> +    if (mhdp->info)
>>> +        bridge_state->input_bus_cfg.flags =
>>> *mhdp->info->input_bus_flags;
>>> +
>>>       mutex_unlock(&mhdp->link_mutex);
>>>       return 0;
>>> }
>>> @@ -2554,8 +2561,6 @@ static int cdns_mhdp_probe(struct
>>> platform_device *pdev)
>>>       mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
>>>                  DRM_BRIDGE_OP_HPD;
>>>       mhdp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>>> -    if (mhdp->info)
>>> -        mhdp->bridge.timings = mhdp->info->timings;
>>
>> Won't this cause a breakage because at this point in time
>> bridge.timings->input_bus_flags
>> seems to be still used by tidss right ?
>>
>
> tidss was using the bridge.timings->input_bus_flags before the 7th
> patch[1] in this series.
>
> After the patch, it only relies on bridge_state and display_info for bus
> flags and formats.

OK thanks, then:

Reviewed-by: Neil Armstrong <[email protected]>

if you resend a new version, please add this info in the commit message.

Neil

>
>
> Regards
> Aradhya
>
> [1]: https://lore.kernel.org/all/[email protected]/
>
>
>>
>>>       ret = phy_init(mhdp->phy);
>>>       if (ret) {
>>> @@ -2642,7 +2647,7 @@ static const struct of_device_id mhdp_ids[] = {
>>> #ifdef CONFIG_DRM_CDNS_MHDP8546_J721E
>>>       { .compatible = "ti,j721e-mhdp8546",
>>>         .data = &(const struct cdns_mhdp_platform_info) {
>>> -          .timings = &mhdp_ti_j721e_bridge_timings,
>>> +          .input_bus_flags = &mhdp_ti_j721e_bridge_input_bus_flags,
>>>             .ops = &mhdp_ti_j721e_ops,
>>>         },
>>>       },
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>> index bedddd510d17..bad2fc0c7306 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>> @@ -336,7 +336,7 @@ struct cdns_mhdp_bridge_state {
>>> };
>>> struct cdns_mhdp_platform_info {
>>> -    const struct drm_bridge_timings *timings;
>>> +    const u32 *input_bus_flags;
>>>       const struct mhdp_platform_ops *ops;
>>> };
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>>> index dfe1b59514f7..12d04be4e242 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>>> @@ -71,8 +71,7 @@ const struct mhdp_platform_ops mhdp_ti_j721e_ops = {
>>>       .disable = cdns_mhdp_j721e_disable,
>>> };
>>> -const struct drm_bridge_timings mhdp_ti_j721e_bridge_timings = {
>>> -    .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
>>> -               DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE |
>>> -               DRM_BUS_FLAG_DE_HIGH,
>>> -};
>>> +const u32
>>> +mhdp_ti_j721e_bridge_input_bus_flags =
>>> DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
>>> +                       DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE |
>>> +                       DRM_BUS_FLAG_DE_HIGH;
>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>>> index 97d20d115a24..5ddca07a4255 100644
>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>>> @@ -14,6 +14,6 @@
>>> struct mhdp_platform_ops;
>>> extern const struct mhdp_platform_ops mhdp_ti_j721e_ops;
>>> -extern const struct drm_bridge_timings mhdp_ti_j721e_bridge_timings;
>>> +extern const u32 mhdp_ti_j721e_bridge_input_bus_flags;
>>> #endif /* !CDNS_MHDP8546_J721E_H */
>>

2023-05-23 06:02:42

[permalink] [raw]

Subject: Re: [PATCH v6 4/8] drm/bridge: mhdp8546: Set input_bus_flags from atomic_check

Hi Neil,

On 22-May-23 13:35, [email protected] wrote:
> On 17/05/2023 07:48, Aradhya Bhatia wrote:
>> Hi Neil,
>>
>> On 16-May-23 12:54, Neil Armstrong wrote:
>>> On 09/05/2023 11:30, Aradhya Bhatia wrote:
>>>> From: Nikhil Devshatwar <[email protected]>
>>>>
>>>> input_bus_flags are specified in drm_bridge_timings (legacy) as well
>>>> as drm_bridge_state->input_bus_cfg.flags
>>>>
>>>> The flags from the timings will be deprecated. Bridges are supposed
>>>> to validate and set the bridge state flags from atomic_check.
>>>>
>>>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>>>> [a-bhatia1: replace timings in cdns_mhdp_platform_info by
>>>> input_bus_flags]
>>>> Signed-off-by: Aradhya Bhatia <[email protected]>
>>>> ---
>>>>
>>>> Notes:
>>>>
>>>>       changes from v5:
>>>>       * removed the wrongly addded return statement in tfp410 driver.
>>>>       * replaced the timings field in cdns_mhdp_platform_info by
>>>>         input_bus_flags field, in order to get rid of bridge->timings
>>>>         altogether.
>>>>
>>>>    drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c | 11
>>>> ++++++++---
>>>>    drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h | 2 +-
>>>>    drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c | 9 ++++-----
>>>>    drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h | 2 +-
>>>>    4 files changed, 14 insertions(+), 10 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> index 623e4235c94f..a677b1267525 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>> @@ -2189,6 +2189,13 @@ static int cdns_mhdp_atomic_check(struct
>>>> drm_bridge *bridge,
>>>>            return -EINVAL;
>>>>        }
>>>>    +    /*
>>>> +     * There might be flags negotiation supported in future.
>>>> +     * Set the bus flags in atomic_check statically for now.
>>>> +     */
>>>> +    if (mhdp->info)
>>>> +        bridge_state->input_bus_cfg.flags =
>>>> *mhdp->info->input_bus_flags;
>>>> +
>>>>        mutex_unlock(&mhdp->link_mutex);
>>>>        return 0;
>>>>    }
>>>> @@ -2554,8 +2561,6 @@ static int cdns_mhdp_probe(struct
>>>> platform_device *pdev)
>>>>        mhdp->bridge.ops = DRM_BRIDGE_OP_DETECT | DRM_BRIDGE_OP_EDID |
>>>>                   DRM_BRIDGE_OP_HPD;
>>>>        mhdp->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>>>> -    if (mhdp->info)
>>>> -        mhdp->bridge.timings = mhdp->info->timings;
>>>
>>> Won't this cause a breakage because at this point in time
>>> bridge.timings->input_bus_flags
>>> seems to be still used by tidss right ?
>>>
>>
>> tidss was using the bridge.timings->input_bus_flags before the 7th
>> patch[1] in this series.
>>
>> After the patch, it only relies on bridge_state and display_info for bus
>> flags and formats.
>
> OK thanks, then:
>
> Reviewed-by: Neil Armstrong <[email protected]>
>
> if you resend a new version, please add this info in the commit message.

Thank you! Will do so.

Regards
Aradhya

>
> Neil
>
>>
>>
>> Regards
>> Aradhya
>>
>> [1]: https://lore.kernel.org/all/[email protected]/
>>
>>
>>>
>>>>          ret = phy_init(mhdp->phy);
>>>>        if (ret) {
>>>> @@ -2642,7 +2647,7 @@ static const struct of_device_id mhdp_ids[] = {
>>>>    #ifdef CONFIG_DRM_CDNS_MHDP8546_J721E
>>>>        { .compatible = "ti,j721e-mhdp8546",
>>>>          .data = &(const struct cdns_mhdp_platform_info) {
>>>> -          .timings = &mhdp_ti_j721e_bridge_timings,
>>>> +          .input_bus_flags = &mhdp_ti_j721e_bridge_input_bus_flags,
>>>>              .ops = &mhdp_ti_j721e_ops,
>>>>          },
>>>>        },
>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>>> index bedddd510d17..bad2fc0c7306 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.h
>>>> @@ -336,7 +336,7 @@ struct cdns_mhdp_bridge_state {
>>>>    };
>>>>    struct cdns_mhdp_platform_info {
>>>> -    const struct drm_bridge_timings *timings;
>>>> +    const u32 *input_bus_flags;
>>>>        const struct mhdp_platform_ops *ops;
>>>>    };
>>>>    diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>>>> index dfe1b59514f7..12d04be4e242 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.c
>>>> @@ -71,8 +71,7 @@ const struct mhdp_platform_ops mhdp_ti_j721e_ops = {
>>>>        .disable = cdns_mhdp_j721e_disable,
>>>>    };
>>>>    -const struct drm_bridge_timings mhdp_ti_j721e_bridge_timings = {
>>>> -    .input_bus_flags = DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
>>>> -               DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE |
>>>> -               DRM_BUS_FLAG_DE_HIGH,
>>>> -};
>>>> +const u32
>>>> +mhdp_ti_j721e_bridge_input_bus_flags =
>>>> DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE |
>>>> +                       DRM_BUS_FLAG_SYNC_SAMPLE_NEGEDGE |
>>>> +                       DRM_BUS_FLAG_DE_HIGH;
>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>>>> index 97d20d115a24..5ddca07a4255 100644
>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-j721e.h
>>>> @@ -14,6 +14,6 @@
>>>>    struct mhdp_platform_ops;
>>>>    extern const struct mhdp_platform_ops mhdp_ti_j721e_ops;
>>>> -extern const struct drm_bridge_timings mhdp_ti_j721e_bridge_timings;
>>>> +extern const u32 mhdp_ti_j721e_bridge_input_bus_flags;
>>>>    #endif /* !CDNS_MHDP8546_J721E_H */
>>>
>

2023-05-26 09:59:45

[permalink] [raw]

Subject: Re: [PATCH v6 3/8] drm/bridge: mhdp8546: Add minimal format negotiation

On 16/05/2023 17:25, Aradhya Bhatia wrote:
> Hi Neil,
>
> Thank you for reviewing the patch.
>
> On 16-May-23 12:51, Neil Armstrong wrote:
>> On 15/05/2023 17:59, Aradhya Bhatia wrote:
>>> Hi Tomi,
>>>
>>> On 12-May-23 14:45, Tomi Valkeinen wrote:
>>>> On 09/05/2023 12:30, Aradhya Bhatia wrote:
>>>>> From: Nikhil Devshatwar <[email protected]>
>>>>>
>>>>> With new connector model, mhdp bridge will not create the connector and
>>>>> SoC driver will rely on format negotiation to setup the encoder format.
>>>>>
>>>>> Support minimal format negotiations hooks in the drm_bridge_funcs.
>>>>> Complete format negotiation can be added based on EDID data.
>>>>> This patch adds the minimal required support to avoid failure
>>>>> after moving to new connector model.
>>>>>
>>>>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>>>>> Reviewed-by: Tomi Valkeinen <[email protected]>
>>>>
>>>> You need to add your SoB to this and the other patches.
>>>
>>> Okay!
>>>
>>>>
>>>>> ---
>>>>>
>>>>> Notes:
>>>>>
>>>>>       changes from v1:
>>>>>       * cosmetic fixes, commit message update.
>>>>>
>>>>>       changes from v5:
>>>>>       * dropped the default_bus_format variable and directly assigned
>>>>>         MEDIA_BUS_FMT_RGB121212_1X36 to input_fmts.
>>>>>
>>>>>    .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 25
>>>>> +++++++++++++++++++
>>>>>    1 file changed, 25 insertions(+)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>>> index f6822dfa3805..623e4235c94f 100644
>>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>>> @@ -2146,6 +2146,30 @@ cdns_mhdp_bridge_atomic_reset(struct drm_bridge
>>>>> *bridge)
>>>>>        return &cdns_mhdp_state->base;
>>>>>    }
>>>>>    +static u32 *cdns_mhdp_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)
>>>>> +{
>>>>> +    u32 *input_fmts;
>>>>> +
>>>>> +    *num_input_fmts = 0;
>>>>> +
>>>>> +    if (output_fmt != MEDIA_BUS_FMT_FIXED)
>>>>> +        return NULL;
>>>>
>>>> The tfp410 and sii902x drivers don't have the above check. Why does mhdp
>>>> need it? Or the other way, why don't tfp410 and sii902x need it?
>>>
>>> I had removed this condition in order to follow status quo, from the
>>> ITE-66121 HDMI bridge driver.
>>>
>>> The idea would have been to drop this for MHDP as well, but I guess I
>>> overlooked this one.
>>>
>>> However...
>>>
>>>> I guess at the moment we always do get MEDIA_BUS_FMT_FIXED as the out
>>>> fmt (in all three bridge drivers), don't we?
>>>
>>> ... I tested again to ensure that the above is indeed the case. And
>>> ended up catching some odd behavior.
>>>
>>> It turns out that for all the HDMI bridges (TFP410, SII902X, ITE-66121),
>>> the format negotiation doesn't stop at output_fmt = MEDIA_BUS_FMT_FIXED.
>>> The {bridge}_get_input_format API gets called again with the output_fmt
>>> = MEDIA_BUS_FMT_RGB24_1X24.
>>>
>>> This doesn't happen with the MHDP driver. Format negotiation with MHDP
>>> bridge stops after one round, at output_fmt = MEDIA_BUS_FMT_FIXED.
>>
>> This is because the bridge negociation logic will test with all possible
>> output formats from the chain, and won't stop at first working test.
>>
> Okay..
>
>> If your bridge only supports a single input format, it should return the
>> same format whatever output_fmt is tried.
>>
>> So indeed remove this test on mhdp aswell, or filter out invalid output
>> formats.
> Agreed.
>
> I have been looking into the code deeper and trying to understand the
> logic flow around the format negotiation in the framework. Here are the
> 2 points that I want to mention. Please let me know if I have missed
> something with my understanding.
>
>
> Firstly, the mhdp-8546 output connects to the display-connector (with
> the compatible, "dp-connector") in the devicetree.
>
> When the negotiation begins at 'drm_atomic_bridge_chain_select_bus_fmts'
> the display-connector bridge *should* act as the 'last_bridge', and the
> atomic_get_output_bus_fmts hook of the display-connector should get
> called. However, for some reason I am not yet sure of, the condition
>
> :: if (last_bridge->funcs->atomic_get_output_bus_fmts)
>
> fails and the 'select_bus_fmt_recursive' function gets called instead,
> (with MEDIA_BUS_FMT_FIXED as output_fmt), which in turn calls the
> atomic_get_input_bus_fmts hook of the mhdp-8546. This entirely skips the
> display-connector out of the format negotiation.
>
> This doesn't happen when the HDMI bridges are in concern, even though,
> they too are connected with display-connector (with compatible
> "hdmi-connector").
>
> I looked into the display-connector driver hoping to find if the 2 types
> of connectors are being treated differently wrt format negotiation, but
> I did not find any clue.
>
> Please let me know if you have any idea about this.

The display connector is probed, but not attached to the bridge chain,
so the last bridge is the mdhp. You need to call drm_bridge_attach in
the mhdp driver to attach the next bridge. See e.g. tfp410's
tfp410_attach().

Also, I think the support in mhdp for the
!DRM_BRIDGE_ATTACH_NO_CONNECTOR case should be dropped.

> Secondly, as mentioned in the display-connector driver, this bridge is
> essentially a pass-through. And hence to reflect that, both the format
> negotiation hooks of display-connector driver call their counter-parts
> from the previous bridge if they are available, and if not, the formats
> are assigned MEDIA_BUS_FMT_FIXED.
>
> While this makes sense for the atomic_get_output_bus_fmts hook, it seems
> to me that, the same may not hold true for the atomic_get_input_bus_fmts
> hook.
> If the bridge is indeed a pass-through, should it not also pass the
> output_format as its input format (which it actually got from the output
> of previous bridge)?
>
> This way all the following will remain same.
>
> 1. output_fmt of prev_bridge,
> 2. input_fmt of display-connector, and
> 3. output_fmt of display-connector.
>
> Currently, since the atomic_get_input_bus_fmts hook of display-connector
> calls its counter-part from the prev_bridge, the input_fmt it passes
> (for HDMI bridges) is MEDIA_BUS_FMT_RGB888_1X24. The
> atomic_get_ouput_bus_fmts hook of the HDMI bridge has to, then, set an
> input bus format considering MEDIA_BUS_FMT_RGB888_1X24 as its output
> instead of MEDIA_BUS_FMT_FIXED.
>
> Let me know what you think!

Yes, it does sound odd to me. Returning the same from the
display-connector's get-input-fmt and get-output-fmt makes more sense.

Btw, we seem to be missing get-output-fmt from the mdhp driver.

Tomi

2023-05-29 05:50:51

[permalink] [raw]

Subject: Re: [PATCH v6 3/8] drm/bridge: mhdp8546: Add minimal format negotiation

Hi Tomi,

Thank you for taking a look at this.

On 26/05/23 14:59, Tomi Valkeinen wrote:
> On 16/05/2023 17:25, Aradhya Bhatia wrote:
>> Hi Neil,
>>
>> Thank you for reviewing the patch.
>>
>> On 16-May-23 12:51, Neil Armstrong wrote:
>>> On 15/05/2023 17:59, Aradhya Bhatia wrote:
>>>> Hi Tomi,
>>>>
>>>> On 12-May-23 14:45, Tomi Valkeinen wrote:
>>>>> On 09/05/2023 12:30, Aradhya Bhatia wrote:
>>>>>> From: Nikhil Devshatwar <[email protected]>
>>>>>>
>>>>>> With new connector model, mhdp bridge will not create the
>>>>>> connector and
>>>>>> SoC driver will rely on format negotiation to setup the encoder
>>>>>> format.
>>>>>>
>>>>>> Support minimal format negotiations hooks in the drm_bridge_funcs.
>>>>>> Complete format negotiation can be added based on EDID data.
>>>>>> This patch adds the minimal required support to avoid failure
>>>>>> after moving to new connector model.
>>>>>>
>>>>>> Signed-off-by: Nikhil Devshatwar <[email protected]>
>>>>>> Reviewed-by: Tomi Valkeinen <[email protected]>
>>>>>
>>>>> You need to add your SoB to this and the other patches.
>>>>
>>>> Okay!
>>>>
>>>>>
>>>>>> ---
>>>>>>
>>>>>> Notes:
>>>>>>
>>>>>>        changes from v1:
>>>>>>        * cosmetic fixes, commit message update.
>>>>>>
>>>>>>        changes from v5:
>>>>>>        * dropped the default_bus_format variable and directly
>>>>>> assigned
>>>>>>          MEDIA_BUS_FMT_RGB121212_1X36 to input_fmts.
>>>>>>
>>>>>>     .../drm/bridge/cadence/cdns-mhdp8546-core.c   | 25
>>>>>> +++++++++++++++++++
>>>>>>     1 file changed, 25 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>>>> b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>>>> index f6822dfa3805..623e4235c94f 100644
>>>>>> --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>>>> +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c
>>>>>> @@ -2146,6 +2146,30 @@ cdns_mhdp_bridge_atomic_reset(struct
>>>>>> drm_bridge
>>>>>> *bridge)
>>>>>>         return &cdns_mhdp_state->base;
>>>>>>     }
>>>>>>     +static u32 *cdns_mhdp_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)
>>>>>> +{
>>>>>> +    u32 *input_fmts;
>>>>>> +
>>>>>> +    *num_input_fmts = 0;
>>>>>> +
>>>>>> +    if (output_fmt != MEDIA_BUS_FMT_FIXED)
>>>>>> +        return NULL;
>>>>>
>>>>> The tfp410 and sii902x drivers don't have the above check. Why does
>>>>> mhdp
>>>>> need it? Or the other way, why don't tfp410 and sii902x need it?
>>>>
>>>> I had removed this condition in order to follow status quo, from the
>>>> ITE-66121 HDMI bridge driver.
>>>>
>>>> The idea would have been to drop this for MHDP as well, but I guess I
>>>> overlooked this one.
>>>>
>>>> However...
>>>>
>>>>> I guess at the moment we always do get MEDIA_BUS_FMT_FIXED as the out
>>>>> fmt (in all three bridge drivers), don't we?
>>>>
>>>> ... I tested again to ensure that the above is indeed the case. And
>>>> ended up catching some odd behavior.
>>>>
>>>> It turns out that for all the HDMI bridges (TFP410, SII902X,
>>>> ITE-66121),
>>>> the format negotiation doesn't stop at output_fmt =
>>>> MEDIA_BUS_FMT_FIXED.
>>>> The {bridge}_get_input_format API gets called again with the output_fmt
>>>> = MEDIA_BUS_FMT_RGB24_1X24.
>>>>
>>>> This doesn't happen with the MHDP driver. Format negotiation with MHDP
>>>> bridge stops after one round, at output_fmt = MEDIA_BUS_FMT_FIXED.
>>>
>>> This is because the bridge negociation logic will test with all possible
>>> output formats from the chain, and won't stop at first working test.
>>>
>> Okay..
>>
>>> If your bridge only supports a single input format, it should return the
>>> same format whatever output_fmt is tried.
>>>
>>> So indeed remove this test on mhdp aswell, or filter out invalid output
>>> formats.
>> Agreed.
>>
>> I have been looking into the code deeper and trying to understand the
>> logic flow around the format negotiation in the framework. Here are the
>> 2 points that I want to mention. Please let me know if I have missed
>> something with my understanding.
>>
>>
>> Firstly, the mhdp-8546 output connects to the display-connector (with
>> the compatible, "dp-connector") in the devicetree.
>>
>> When the negotiation begins at 'drm_atomic_bridge_chain_select_bus_fmts'
>> the display-connector bridge *should* act as the 'last_bridge', and the
>> atomic_get_output_bus_fmts hook of the display-connector should get
>> called. However, for some reason I am not yet sure of, the condition
>>
>> :: if (last_bridge->funcs->atomic_get_output_bus_fmts)
>>
>> fails and the 'select_bus_fmt_recursive' function gets called instead,
>> (with MEDIA_BUS_FMT_FIXED as output_fmt), which in turn calls the
>> atomic_get_input_bus_fmts hook of the mhdp-8546. This entirely skips the
>> display-connector out of the format negotiation.
>>
>> This doesn't happen when the HDMI bridges are in concern, even though,
>> they too are connected with display-connector (with compatible
>> "hdmi-connector").
>>
>> I looked into the display-connector driver hoping to find if the 2 types
>> of connectors are being treated differently wrt format negotiation, but
>> I did not find any clue.
>>
>> Please let me know if you have any idea about this.
>
> The display connector is probed, but not attached to the bridge chain,
> so the last bridge is the mdhp. You need to call drm_bridge_attach in
> the mhdp driver to attach the next bridge. See e.g. tfp410's
> tfp410_attach().

Okay, understood. Thank you!

>
> Also, I think the support in mhdp for the
> !DRM_BRIDGE_ATTACH_NO_CONNECTOR case should be dropped.

Agreed. I will send a separate series for this and drm_bridge_attach
add.

>
>> Secondly, as mentioned in the display-connector driver, this bridge is
>> essentially a pass-through. And hence to reflect that, both the format
>> negotiation hooks of display-connector driver call their counter-parts
>> from the previous bridge if they are available, and if not, the formats
>> are assigned MEDIA_BUS_FMT_FIXED.
>>
>> While this makes sense for the atomic_get_output_bus_fmts hook, it seems
>> to me that, the same may not hold true for the atomic_get_input_bus_fmts
>> hook.
>> If the bridge is indeed a pass-through, should it not also pass the
>> output_format as its input format (which it actually got from the output
>> of previous bridge)?
>>
>> This way all the following will remain same.
>>
>> 1. output_fmt of prev_bridge,
>> 2. input_fmt of display-connector, and
>> 3. output_fmt of display-connector.
>>
>> Currently, since the atomic_get_input_bus_fmts hook of display-connector
>> calls its counter-part from the prev_bridge, the input_fmt it passes
>> (for HDMI bridges) is MEDIA_BUS_FMT_RGB888_1X24. The
>> atomic_get_ouput_bus_fmts hook of the HDMI bridge has to, then, set an
>> input bus format considering MEDIA_BUS_FMT_RGB888_1X24 as its output
>> instead of MEDIA_BUS_FMT_FIXED.
>>
>> Let me know what you think!
>
> Yes, it does sound odd to me. Returning the same from the
> display-connector's get-input-fmt and get-output-fmt makes more sense.

Yes, it does make more sense! I can send a separate fix for this.

>
> Btw, we seem to be missing get-output-fmt from the mdhp driver.

Yes, we are.

With the drm_bridge_attach call added, the display-connector bridge will
assign MEDIA_BUS_FMT_FIXED as the default output format. And most
bridges support only their primary output bus format in their
get-output-fmt hooks. I suppose it would be RGB121212_1X36 in mhdp8546's
case.

Do we require this when there is no comprehensive way to determine if
another bus format may be more suitable (depending on the hardware
configurations)?

Regards
Aradhya

2023-05-29 11:18:29