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