| Message ID | 20201119160134.9244-5-nikhil.nd@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tidss: Use new connector model for tidss | expand |
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, 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"); > 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 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: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"); > > 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; > > } > > > > -- > Regards, > > Laurent Pinchart
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
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; }