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
Since the bridges do not create the connector, the bus format and bus_flag is set after the format negotiation. Support format negotiations in the tfp410 and mhdp bridge drivers as a first step before moving the connector model.
Nikhil Devshatwar (6): drm: bridge: Propagate the bus flags from bridge->timings drm/bridge: tfp410: Support format negotiation hooks drm/bridge: mhdp8546: Add minimal format negotiation drm/tidss: Set bus_format correctly from bridge/connector drm/tidss: Move to newer connector model drm/bridge: cdns-mhdp8546: Fix the interrupt enable/disable
.../drm/bridge/cadence/cdns-mhdp8546-core.c | 69 +++++++++++++------ drivers/gpu/drm/bridge/ti-tfp410.c | 33 +++++++++ drivers/gpu/drm/drm_bridge.c | 8 +++ drivers/gpu/drm/tidss/tidss_drv.h | 3 + drivers/gpu/drm/tidss/tidss_encoder.c | 36 ++++------ drivers/gpu/drm/tidss/tidss_kms.c | 19 ++++- 6 files changed, 123 insertions(+), 45 deletions(-)
bus_flags can be specified by a bridge in the timings. If the bridge provides it, Override the bus_flags when propagating from next bridge.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com ---
Notes: changes from v2: * update comment changes from v1: * Check for timings * Prioritize timings flags over next bridge's flags
drivers/gpu/drm/drm_bridge.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 64f0effb52ac..13b67fc0dad3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -975,6 +975,14 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge, * duplicate the "dummy propagation" logic. */ bridge_state->input_bus_cfg.flags = output_flags; + + /* + * If legacy bus flags are provided in bridge->timings, use those as + * input flags instead of propagating the output flags. + */ + if (bridge->timings && bridge->timings->input_bus_flags) + bridge_state->input_bus_cfg.flags = + bridge->timings->input_bus_flags; }
/**
Hi Nikhil,
Thank you for the patch.
On Thu, Nov 19, 2020 at 09:31:29PM +0530, Nikhil Devshatwar wrote:
bus_flags can be specified by a bridge in the timings. If the bridge provides it, Override the bus_flags when propagating from next bridge.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Notes: changes from v2: * update comment changes from v1: * Check for timings * Prioritize timings flags over next bridge's flags
drivers/gpu/drm/drm_bridge.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 64f0effb52ac..13b67fc0dad3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -975,6 +975,14 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge, * duplicate the "dummy propagation" logic. */ bridge_state->input_bus_cfg.flags = output_flags;
- /*
* If legacy bus flags are provided in bridge->timings, use those as* input flags instead of propagating the output flags.*/- if (bridge->timings && bridge->timings->input_bus_flags)
bridge_state->input_bus_cfg.flags =bridge->timings->input_bus_flags;
Hasn't Boris commented in his review of v1 that bus flags should be set in atomic_check, even when they're static ? We're moving towards removing timings->input_bus_flags, so this patch goes in the wrong direction :-S
}
/**
On 30/11/2020 11:36, Laurent Pinchart wrote:
Hi Nikhil,
Thank you for the patch.
On Thu, Nov 19, 2020 at 09:31:29PM +0530, Nikhil Devshatwar wrote:
bus_flags can be specified by a bridge in the timings. If the bridge provides it, Override the bus_flags when propagating from next bridge.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Notes: changes from v2: * update comment changes from v1: * Check for timings * Prioritize timings flags over next bridge's flags
drivers/gpu/drm/drm_bridge.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 64f0effb52ac..13b67fc0dad3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -975,6 +975,14 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge, * duplicate the "dummy propagation" logic. */ bridge_state->input_bus_cfg.flags = output_flags;
- /*
* If legacy bus flags are provided in bridge->timings, use those as* input flags instead of propagating the output flags.*/- if (bridge->timings && bridge->timings->input_bus_flags)
bridge_state->input_bus_cfg.flags =bridge->timings->input_bus_flags;Hasn't Boris commented in his review of v1 that bus flags should be set in atomic_check, even when they're static ? We're moving towards removing timings->input_bus_flags, so this patch goes in the wrong direction :-S
We have atomic_check only if the bridge has implemented atomic funcs. And even if there's atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the bus_flags.
Tomi
Hi Tomi,
On Mon, Nov 30, 2020 at 11:46:31AM +0200, Tomi Valkeinen wrote:
On 30/11/2020 11:36, Laurent Pinchart wrote:
On Thu, Nov 19, 2020 at 09:31:29PM +0530, Nikhil Devshatwar wrote:
bus_flags can be specified by a bridge in the timings. If the bridge provides it, Override the bus_flags when propagating from next bridge.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Notes: changes from v2: * update comment changes from v1: * Check for timings * Prioritize timings flags over next bridge's flags
drivers/gpu/drm/drm_bridge.c | 8 ++++++++ 1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c index 64f0effb52ac..13b67fc0dad3 100644 --- a/drivers/gpu/drm/drm_bridge.c +++ b/drivers/gpu/drm/drm_bridge.c @@ -975,6 +975,14 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge, * duplicate the "dummy propagation" logic. */ bridge_state->input_bus_cfg.flags = output_flags;
- /*
* If legacy bus flags are provided in bridge->timings, use those as* input flags instead of propagating the output flags.*/- if (bridge->timings && bridge->timings->input_bus_flags)
bridge_state->input_bus_cfg.flags =bridge->timings->input_bus_flags;Hasn't Boris commented in his review of v1 that bus flags should be set in atomic_check, even when they're static ? We're moving towards removing timings->input_bus_flags, so this patch goes in the wrong direction :-S
We have atomic_check only if the bridge has implemented atomic funcs. And even if there's atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the bus_flags.
The second option is what we'd like to achieve. Wouldn't it be best to already start going in that direction ? We don't need to convert all bridge drivers in one go here, just the ones that are used by tidss.
On 30/11/2020 11:47, Laurent Pinchart wrote:
Hasn't Boris commented in his review of v1 that bus flags should be set in atomic_check, even when they're static ? We're moving towards removing timings->input_bus_flags, so this patch goes in the wrong direction :-S
We have atomic_check only if the bridge has implemented atomic funcs. And even if there's atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the bus_flags.
The second option is what we'd like to achieve. Wouldn't it be best to already start going in that direction ? We don't need to convert all bridge drivers in one go here, just the ones that are used by tidss.
I think that sounds fine, except that this is blocking the DisplayPort support for J7. We have everything in for DP except dts changes (can be added only when the drivers work), and the connector stuff.
The connector stuff includes this series (so that tidss supports the new connector model), and "[PATCH RESEND v3 0/2] drm: add DisplayPort connector", which adds the connector driver.
The bridges currently used (that I know of) with tidss are cdns-mhdp, tfp410 and sii9022. I don't expect converting those would be a huge job, but I'd still really like to get the DP working in upstream without starting to expand the scope of the patches we need to enable it.
That said, we missed 5.11 so perhaps we have the time.
Tomi
On 30/11/2020 12:02, Tomi Valkeinen wrote:
On 30/11/2020 11:47, Laurent Pinchart wrote:
Hasn't Boris commented in his review of v1 that bus flags should be set in atomic_check, even when they're static ? We're moving towards removing timings->input_bus_flags, so this patch goes in the wrong direction :-S
We have atomic_check only if the bridge has implemented atomic funcs. And even if there's atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the bus_flags.
The second option is what we'd like to achieve. Wouldn't it be best to already start going in that direction ? We don't need to convert all bridge drivers in one go here, just the ones that are used by tidss.
I think that sounds fine, except that this is blocking the DisplayPort support for J7. We have everything in for DP except dts changes (can be added only when the drivers work), and the connector stuff.
The connector stuff includes this series (so that tidss supports the new connector model), and "[PATCH RESEND v3 0/2] drm: add DisplayPort connector", which adds the connector driver.
The bridges currently used (that I know of) with tidss are cdns-mhdp, tfp410 and sii9022. I don't expect converting those would be a huge job, but I'd still really like to get the DP working in upstream without starting to expand the scope of the patches we need to enable it.
That said, we missed 5.11 so perhaps we have the time.
Looks like Boris was missing from Cc in this series. Adding him.
Tomi
Hi Tomi,
On Mon, Nov 30, 2020 at 12:04:27PM +0200, Tomi Valkeinen wrote:
On 30/11/2020 12:02, Tomi Valkeinen wrote:
On 30/11/2020 11:47, Laurent Pinchart wrote:
Hasn't Boris commented in his review of v1 that bus flags should be set in atomic_check, even when they're static ? We're moving towards removing timings->input_bus_flags, so this patch goes in the wrong direction :-S
We have atomic_check only if the bridge has implemented atomic funcs. And even if there's atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the bus_flags.
The second option is what we'd like to achieve. Wouldn't it be best to already start going in that direction ? We don't need to convert all bridge drivers in one go here, just the ones that are used by tidss.
I think that sounds fine, except that this is blocking the DisplayPort support for J7. We have everything in for DP except dts changes (can be added only when the drivers work), and the connector stuff.
The connector stuff includes this series (so that tidss supports the new connector model), and "[PATCH RESEND v3 0/2] drm: add DisplayPort connector", which adds the connector driver.
The bridges currently used (that I know of) with tidss are cdns-mhdp, tfp410 and sii9022. I don't expect converting those would be a huge job, but I'd still really like to get the DP working in upstream without starting to expand the scope of the patches we need to enable it.
That said, we missed 5.11 so perhaps we have the time.
If there's not enough time to address the bridges, I'm fine with this series assuming the bridge changes will go on top. If we have enough time, let's go for it :-)
Looks like Boris was missing from Cc in this series. Adding him.
On 20:59-20201130, Laurent Pinchart wrote:
Hi Tomi,
On Mon, Nov 30, 2020 at 12:04:27PM +0200, Tomi Valkeinen wrote:
On 30/11/2020 12:02, Tomi Valkeinen wrote:
On 30/11/2020 11:47, Laurent Pinchart wrote:
Hasn't Boris commented in his review of v1 that bus flags should be set in atomic_check, even when they're static ? We're moving towards removing timings->input_bus_flags, so this patch goes in the wrong direction :-S
We have atomic_check only if the bridge has implemented atomic funcs. And even if there's atomic_check, not all bridges set the bus_flags there. So we need to either 1) fix the issue for now as in this patch, or 2) convert all bridges to use atomic funcs and fix all the bridges to set the bus_flags.
The second option is what we'd like to achieve. Wouldn't it be best to already start going in that direction ? We don't need to convert all bridge drivers in one go here, just the ones that are used by tidss.
I took this as a future approach to eventually start supporting atomic_funcs. I will respin v4 of this series with updates to the other bridges supporting atomic functions.
Nikhil Devshatwar
I think that sounds fine, except that this is blocking the DisplayPort support for J7. We have everything in for DP except dts changes (can be added only when the drivers work), and the connector stuff.
The connector stuff includes this series (so that tidss supports the new connector model), and "[PATCH RESEND v3 0/2] drm: add DisplayPort connector", which adds the connector driver.
The bridges currently used (that I know of) with tidss are cdns-mhdp, tfp410 and sii9022. I don't expect converting those would be a huge job, but I'd still really like to get the DP working in upstream without starting to expand the scope of the patches we need to enable it.
That said, we missed 5.11 so perhaps we have the time.
If there's not enough time to address the bridges, I'm fine with this series assuming the bridge changes will go on top. If we have enough time, let's go for it :-)
Looks like Boris was missing from Cc in this series. Adding him.
-- Regards,
Laurent Pinchart
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.
Output format is expected to be MEDIA_BUS_FMT_FIXED Input format is the one selected by the bridge from DT properties.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com ---
Notes: changes from v1: * Use only MEDIA_BUS_FMT_FIXED for output
drivers/gpu/drm/bridge/ti-tfp410.c | 33 ++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c index ba3fa2a9b8a4..3def9acba86b 100644 --- a/drivers/gpu/drm/bridge/ti-tfp410.c +++ b/drivers/gpu/drm/bridge/ti-tfp410.c @@ -204,12 +204,45 @@ 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; + + /* + * Output of tfp410 is DVI, which is TMDS. + * There is no media format defined for this. + * Using MEDIA_BUS_FMT_FIXED for now. + */ + 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] = 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 = {
Hi Nikhil,
Thank you for the patch.
On Thu, Nov 19, 2020 at 09:31:30PM +0530, Nikhil Devshatwar wrote:
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.
Output format is expected to be MEDIA_BUS_FMT_FIXED Input format is the one selected by the bridge from DT properties.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com
Notes: changes from v1: * Use only MEDIA_BUS_FMT_FIXED for output
drivers/gpu/drm/bridge/ti-tfp410.c | 33 ++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+)
diff --git a/drivers/gpu/drm/bridge/ti-tfp410.c b/drivers/gpu/drm/bridge/ti-tfp410.c index ba3fa2a9b8a4..3def9acba86b 100644 --- a/drivers/gpu/drm/bridge/ti-tfp410.c +++ b/drivers/gpu/drm/bridge/ti-tfp410.c @@ -204,12 +204,45 @@ 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;
- /*
* Output of tfp410 is DVI, which is TMDS.* There is no media format defined for this.* Using MEDIA_BUS_FMT_FIXED for now.*/- 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] = 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 = {
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 nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com ---
Notes: changes from v1: * cosmetic fixes, commit message update
.../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 d0c65610ebb5..2cd809eed827 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c @@ -2078,6 +2078,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; + u32 default_bus_format = MEDIA_BUS_FMT_RGB121212_1X36; + + *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] = default_bus_format; + 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, @@ -2142,6 +2166,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,
Remove the old code to iterate over the bridge chain, as this is already done by the framework. The bridge state should have the negotiated bus format and flags. Use these from the bridge's state. If the bridge does not support format negotiation, error out and fail.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com ---
Notes: changes from v2: * Remove the old code and use the flags from the bridge state
drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++---------------- 1 file changed, 14 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c index e278a9c89476..08d5083c5508 100644 --- a/drivers/gpu/drm/tidss/tidss_encoder.c +++ b/drivers/gpu/drm/tidss/tidss_encoder.c @@ -21,37 +21,29 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder, { struct drm_device *ddev = encoder->dev; 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_state *bstate; 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; + /* Copy the bus_format and flags from the first bridge's state */ + bridge = drm_bridge_chain_get_first_bridge(encoder); + bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge); + if (bstate) { + tcrtc_state->bus_format = bstate->input_bus_cfg.format; + tcrtc_state->bus_flags = bstate->input_bus_cfg.flags; + } else { + dev_err(ddev->dev, "Could not get the bridge state\n"); + return -EINVAL; }
- if (!di->bus_formats || di->num_bus_formats == 0) { - dev_err(ddev->dev, "%s: No bus_formats in connected display\n", - __func__); + if (tcrtc_state->bus_format == 0 || + tcrtc_state->bus_format == MEDIA_BUS_FMT_FIXED) { + + dev_err(ddev->dev, "Bridge connected to the encoder did not specify media bus format\n"); 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; }
Hi Nikhil,
On 19/11/2020 18:01, Nikhil Devshatwar wrote:
Remove the old code to iterate over the bridge chain, as this is already done by the framework. The bridge state should have the negotiated bus format and flags. Use these from the bridge's state. If the bridge does not support format negotiation, error out and fail.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Notes: changes from v2: * Remove the old code and use the flags from the bridge state
drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++---------------- 1 file changed, 14 insertions(+), 22 deletions(-)
If a first bridge (after the crtc) supports two bus formats as input, how does tidss choose between those? This patch just picks bstate->input_bus_cfg.format, and it's not clear to me which one that is (the first one?). Also, we don't check if tidss actually supports the bus format.
Tomi
On 14:51-20201125, Tomi Valkeinen wrote:
Hi Nikhil,
On 19/11/2020 18:01, Nikhil Devshatwar wrote:
Remove the old code to iterate over the bridge chain, as this is already done by the framework. The bridge state should have the negotiated bus format and flags. Use these from the bridge's state. If the bridge does not support format negotiation, error out and fail.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Notes: changes from v2: * Remove the old code and use the flags from the bridge state
drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++---------------- 1 file changed, 14 insertions(+), 22 deletions(-)
If a first bridge (after the crtc) supports two bus formats as input, how does tidss choose between those? This patch just picks bstate->input_bus_cfg.format, and it's not clear to me which one that is (the first one?). Also, we don't check if tidss actually supports the bus format.
The selection is done by the framework in select_bus_fmt_recursive at drivers/gpu/drm/drm_bridge.c:810
My understanding is that currently, the format negotiation logic does not negotiate all the way till encoder, it stops only at the first_bridge.
Nikhil Devshatwar
Tomi
-- Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki. Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Hi Nikhil,
On Mon, Nov 30, 2020 at 12:05:03PM +0530, Nikhil Devshatwar wrote:
On 14:51-20201125, Tomi Valkeinen wrote:
On 19/11/2020 18:01, Nikhil Devshatwar wrote:
Remove the old code to iterate over the bridge chain, as this is already done by the framework. The bridge state should have the negotiated bus format and flags. Use these from the bridge's state. If the bridge does not support format negotiation, error out and fail.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Notes: changes from v2: * Remove the old code and use the flags from the bridge state
drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++---------------- 1 file changed, 14 insertions(+), 22 deletions(-)
If a first bridge (after the crtc) supports two bus formats as input, how does tidss choose between those? This patch just picks bstate->input_bus_cfg.format, and it's not clear to me which one that is (the first one?). Also, we don't check if tidss actually supports the bus format.
The selection is done by the framework in select_bus_fmt_recursive at drivers/gpu/drm/drm_bridge.c:810
My understanding is that currently, the format negotiation logic does not negotiate all the way till encoder, it stops only at the first_bridge.
Should we then implement a bridge in the tidss driver to model the internal encoder, in order to support format negotiation all the way to the tidss ?
On 11:46-20201130, Laurent Pinchart wrote:
Hi Nikhil,
On Mon, Nov 30, 2020 at 12:05:03PM +0530, Nikhil Devshatwar wrote:
On 14:51-20201125, Tomi Valkeinen wrote:
On 19/11/2020 18:01, Nikhil Devshatwar wrote:
Remove the old code to iterate over the bridge chain, as this is already done by the framework. The bridge state should have the negotiated bus format and flags. Use these from the bridge's state. If the bridge does not support format negotiation, error out and fail.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Notes: changes from v2: * Remove the old code and use the flags from the bridge state
drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++---------------- 1 file changed, 14 insertions(+), 22 deletions(-)
If a first bridge (after the crtc) supports two bus formats as input, how does tidss choose between those? This patch just picks bstate->input_bus_cfg.format, and it's not clear to me which one that is (the first one?). Also, we don't check if tidss actually supports the bus format.
The selection is done by the framework in select_bus_fmt_recursive at drivers/gpu/drm/drm_bridge.c:810
My understanding is that currently, the format negotiation logic does not negotiate all the way till encoder, it stops only at the first_bridge.
Should we then implement a bridge in the tidss driver to model the internal encoder, in order to support format negotiation all the way to the tidss ?
I am not sure. Scope of this series is to enable tidss with new connector model. As of now, there aren't any bridges that report unsupported format, so nothing is broken.
When the bridge drivers start reporting unsupported formats, we can evaluate if it makes sense to implement the internal encoder as a bridge.
Nikhi Devshatwar
-- Regards,
Laurent Pinchart
On 30/11/2020 11:46, Laurent Pinchart wrote:
Hi Nikhil,
On Mon, Nov 30, 2020 at 12:05:03PM +0530, Nikhil Devshatwar wrote:
On 14:51-20201125, Tomi Valkeinen wrote:
On 19/11/2020 18:01, Nikhil Devshatwar wrote:
Remove the old code to iterate over the bridge chain, as this is already done by the framework. The bridge state should have the negotiated bus format and flags. Use these from the bridge's state. If the bridge does not support format negotiation, error out and fail.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Notes: changes from v2: * Remove the old code and use the flags from the bridge state
drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++---------------- 1 file changed, 14 insertions(+), 22 deletions(-)
If a first bridge (after the crtc) supports two bus formats as input, how does tidss choose between those? This patch just picks bstate->input_bus_cfg.format, and it's not clear to me which one that is (the first one?). Also, we don't check if tidss actually supports the bus format.
The selection is done by the framework in select_bus_fmt_recursive at drivers/gpu/drm/drm_bridge.c:810
My understanding is that currently, the format negotiation logic does not negotiate all the way till encoder, it stops only at the first_bridge.
Should we then implement a bridge in the tidss driver to model the internal encoder, in order to support format negotiation all the way to the tidss ?
I don't know, but it feels perhaps a bit odd. Then we would have crtc + encoder + bridge, which are actually all about the same HW block. And this would have to be done for all DRM drivers.
Tomi
Hi Nikhil,
Thank you for the patch.
On Thu, Nov 19, 2020 at 09:31:32PM +0530, Nikhil Devshatwar wrote:
Remove the old code to iterate over the bridge chain, as this is already done by the framework. The bridge state should have the negotiated bus format and flags. Use these from the bridge's state. If the bridge does not support format negotiation, error out and fail.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Notes: changes from v2: * Remove the old code and use the flags from the bridge state
drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++---------------- 1 file changed, 14 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c index e278a9c89476..08d5083c5508 100644 --- a/drivers/gpu/drm/tidss/tidss_encoder.c +++ b/drivers/gpu/drm/tidss/tidss_encoder.c @@ -21,37 +21,29 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder, { struct drm_device *ddev = encoder->dev; 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_state *bstate; 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;
- /* Copy the bus_format and flags from the first bridge's state */
- bridge = drm_bridge_chain_get_first_bridge(encoder);
- bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
- if (bstate) {
tcrtc_state->bus_format = bstate->input_bus_cfg.format;tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;- } else {
dev_err(ddev->dev, "Could not get the bridge state\n");return -EINVAL;
I'd write this
bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge); if (!bstate) { dev_err(ddev->dev, "Could not get the bridge state\n"); return -EINVAL; }
tcrtc_state->bus_format = bstate->input_bus_cfg.format; tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;
- if (!di->bus_formats || di->num_bus_formats == 0) {
dev_err(ddev->dev, "%s: No bus_formats in connected display\n",__func__);
- if (tcrtc_state->bus_format == 0 ||
tcrtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {dev_err(ddev->dev, "Bridge connected to the encoder did not specify media bus format\n");
- // 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;
}
On 11:45-20201130, Laurent Pinchart wrote:
Hi Nikhil,
Thank you for the patch.
On Thu, Nov 19, 2020 at 09:31:32PM +0530, Nikhil Devshatwar wrote:
Remove the old code to iterate over the bridge chain, as this is already done by the framework. The bridge state should have the negotiated bus format and flags. Use these from the bridge's state. If the bridge does not support format negotiation, error out and fail.
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
Notes: changes from v2: * Remove the old code and use the flags from the bridge state
drivers/gpu/drm/tidss/tidss_encoder.c | 36 +++++++++++---------------- 1 file changed, 14 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c index e278a9c89476..08d5083c5508 100644 --- a/drivers/gpu/drm/tidss/tidss_encoder.c +++ b/drivers/gpu/drm/tidss/tidss_encoder.c @@ -21,37 +21,29 @@ static int tidss_encoder_atomic_check(struct drm_encoder *encoder, { struct drm_device *ddev = encoder->dev; 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_state *bstate; 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;
- /* Copy the bus_format and flags from the first bridge's state */
- bridge = drm_bridge_chain_get_first_bridge(encoder);
- bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge);
- if (bstate) {
tcrtc_state->bus_format = bstate->input_bus_cfg.format;tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;- } else {
dev_err(ddev->dev, "Could not get the bridge state\n");return -EINVAL;I'd write this
bstate = drm_atomic_get_new_bridge_state(crtc_state->state, bridge); if (!bstate) { dev_err(ddev->dev, "Could not get the bridge state\n"); return -EINVAL; }
tcrtc_state->bus_format = bstate->input_bus_cfg.format; tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;
Looks better this way. I'll update
Nikhil Devshatwar
- if (!di->bus_formats || di->num_bus_formats == 0) {
dev_err(ddev->dev, "%s: No bus_formats in connected display\n",__func__);
- if (tcrtc_state->bus_format == 0 ||
tcrtc_state->bus_format == MEDIA_BUS_FMT_FIXED) {dev_err(ddev->dev, "Bridge connected to the encoder did not specify media bus format\n");
- // 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;
}
-- Regards,
Laurent Pinchart
To be able to support connector operations across multiple bridges, it is recommended that the connector should be created by the SoC driver instead of the bridges.
Modify the tidss modesetting initialization sequence to create the connector and attach bridges with flag DRM_BRIDGE_ATTACH_NO_CONNECTOR
Signed-off-by: Nikhil Devshatwar nikhil.nd@ti.com Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com ---
Notes: changes from v1: * Add error handling
drivers/gpu/drm/tidss/tidss_drv.h | 3 +++ drivers/gpu/drm/tidss/tidss_kms.c | 19 ++++++++++++++++++- 2 files changed, 21 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/tidss/tidss_drv.h b/drivers/gpu/drm/tidss/tidss_drv.h index 7de4bba52e6f..cfbf85a4d92b 100644 --- a/drivers/gpu/drm/tidss/tidss_drv.h +++ b/drivers/gpu/drm/tidss/tidss_drv.h @@ -27,6 +27,9 @@ struct tidss_device { unsigned int num_planes; struct drm_plane *planes[TIDSS_MAX_PLANES];
+ unsigned int num_connectors; + struct drm_connector *connectors[TIDSS_MAX_PORTS]; + spinlock_t wait_lock; /* protects the irq masks */ dispc_irq_t irq_mask; /* enabled irqs in addition to wait_list */ }; diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c index 09485c7f0d6f..1f5ae153b114 100644 --- a/drivers/gpu/drm/tidss/tidss_kms.c +++ b/drivers/gpu/drm/tidss/tidss_kms.c @@ -7,6 +7,7 @@ #include <drm/drm_atomic.h> #include <drm/drm_atomic_helper.h> #include <drm/drm_bridge.h> +#include <drm/drm_bridge_connector.h> #include <drm/drm_crtc_helper.h> #include <drm/drm_fb_cma_helper.h> #include <drm/drm_fb_helper.h> @@ -192,6 +193,7 @@ 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_connector *connector; struct drm_encoder *enc; u32 hw_plane_id = feat->vid_order[tidss->num_planes]; int ret; @@ -222,11 +224,26 @@ static int tidss_dispc_modeset_init(struct tidss_device *tidss) return PTR_ERR(enc); }
- ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, 0); + ret = drm_bridge_attach(enc, pipes[i].bridge, NULL, + DRM_BRIDGE_ATTACH_NO_CONNECTOR); if (ret) { dev_err(tidss->dev, "bridge attach failed: %d\n", ret); return ret; } + + 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); + } + + tidss->connectors[tidss->num_connectors++] = connector; + + ret = drm_connector_attach_encoder(connector, enc); + if (ret) { + dev_err(tidss->dev, "attaching encoder to connector failed\n"); + return ret; + } }
/* create overlay planes of the leftover planes */
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 nikhil.nd@ti.com Reviewed-by: Swapnil Jakhade sjakhade@cadence.com ---
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
.../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 2cd809eed827..0442269aeb03 100644 --- a/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c +++ b/drivers/gpu/drm/bridge/cadence/cdns-mhdp8546-core.c @@ -52,6 +52,26 @@
#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; @@ -747,9 +767,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);
@@ -1689,8 +1707,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; } @@ -2140,23 +2157,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,
On 19/11/2020 18:01, Nikhil Devshatwar wrote:
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 nikhil.nd@ti.com Reviewed-by: Swapnil Jakhade sjakhade@cadence.com
Reviewed-by: Tomi Valkeinen tomi.valkeinen@ti.com
I think this is fine as a fix for this issue, but at some point the irq management needs some work. E.g. we call cdns_mhdp_bridge_hpd_enable when attaching/enabling the hw, but also via drm_bridge_funcs->hpd_enable. This doesn't make sense, as one of those calls doesn't achieve anything, as cdns_mhdp_bridge_hpd_enable has already been called.
Tomi
dri-devel@lists.freedesktop.org
-
Laurent Pinchart -
Nikhil Devshatwar -
Tomi Valkeinen