diff mbox series

[3/5] drm: bridge: Propagate the bus flags from bridge->timings

Message ID 20201016103917.26838-4-nikhil.nd@ti.com
State New, archived
Headers show
Series drm/tidss: Use new connector model for tidss | expand

Commit Message

Nikhil Devshatwar Oct. 16, 2020, 10:39 a.m. UTC
When the next bridge does not specify any bus flags, use the
bridge->timings->input_bus_flags as fallback when propagating
bus flags from next bridge to current bridge.

Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
---
 drivers/gpu/drm/drm_bridge.c | 7 +++++++
 1 file changed, 7 insertions(+)

Comments

Tomi Valkeinen Oct. 21, 2020, 11:31 a.m. UTC | #1
On 16/10/2020 13:39, Nikhil Devshatwar wrote:
> When the next bridge does not specify any bus flags, use the
> bridge->timings->input_bus_flags as fallback when propagating
> bus flags from next bridge to current bridge.
> 
> Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> ---
>  drivers/gpu/drm/drm_bridge.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> index 64f0effb52ac..8353723323ab 100644
> --- a/drivers/gpu/drm/drm_bridge.c
> +++ b/drivers/gpu/drm/drm_bridge.c
> @@ -975,6 +975,13 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
>  	 * duplicate the "dummy propagation" logic.
>  	 */
>  	bridge_state->input_bus_cfg.flags = output_flags;
> +
> +	/*
> +	 * Use the bridge->timings->input_bus_flags as fallback if the next bridge
> +	 * does not specify the flags
> +	 */
> +	if (!bridge_state->input_bus_cfg.flags)
> +		bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;

According to docs, timings can be NULL.

And, hmm... It's too easy to get confused with these, but... If the bridge defines timings, and
timings->input_bus_flags != 0, should we always pick that, even if we got something via
output_flags? Logic being, if this bridge defines timings->input_bus_flags, it probably wants that
to be used regardless whether we got something from the next bridge.

 Tomi
Nikhil Devshatwar Oct. 28, 2020, 2:34 p.m. UTC | #2
On 14:31-20201021, Tomi Valkeinen wrote:
> On 16/10/2020 13:39, Nikhil Devshatwar wrote:
> > When the next bridge does not specify any bus flags, use the
> > bridge->timings->input_bus_flags as fallback when propagating
> > bus flags from next bridge to current bridge.
> > 
> > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > ---
> >  drivers/gpu/drm/drm_bridge.c | 7 +++++++
> >  1 file changed, 7 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > index 64f0effb52ac..8353723323ab 100644
> > --- a/drivers/gpu/drm/drm_bridge.c
> > +++ b/drivers/gpu/drm/drm_bridge.c
> > @@ -975,6 +975,13 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
> >  	 * duplicate the "dummy propagation" logic.
> >  	 */
> >  	bridge_state->input_bus_cfg.flags = output_flags;
> > +
> > +	/*
> > +	 * Use the bridge->timings->input_bus_flags as fallback if the next bridge
> > +	 * does not specify the flags
> > +	 */
> > +	if (!bridge_state->input_bus_cfg.flags)
> > +		bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
> 
> According to docs, timings can be NULL.
> 
> And, hmm... It's too easy to get confused with these, but... If the bridge defines timings, and
> timings->input_bus_flags != 0, should we always pick that, even if we got something via
> output_flags? Logic being, if this bridge defines timings->input_bus_flags, it probably wants that
> to be used regardless whether we got something from the next bridge.

Got it, the flags from timings if present should be prioritized rather
than treating them as fallback.

Nikhil D

> 
>  Tomi
> 
> -- 
> Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
> Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
Laurent Pinchart Oct. 29, 2020, 10:48 p.m. UTC | #3
Hello,

On Wed, Oct 28, 2020 at 08:04:53PM +0530, Nikhil Devshatwar wrote:
> On 14:31-20201021, Tomi Valkeinen wrote:
> > On 16/10/2020 13:39, Nikhil Devshatwar wrote:
> > > When the next bridge does not specify any bus flags, use the
> > > bridge->timings->input_bus_flags as fallback when propagating
> > > bus flags from next bridge to current bridge.
> > > 
> > > Signed-off-by: Nikhil Devshatwar <nikhil.nd@ti.com>
> > > ---
> > >  drivers/gpu/drm/drm_bridge.c | 7 +++++++
> > >  1 file changed, 7 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
> > > index 64f0effb52ac..8353723323ab 100644
> > > --- a/drivers/gpu/drm/drm_bridge.c
> > > +++ b/drivers/gpu/drm/drm_bridge.c
> > > @@ -975,6 +975,13 @@ drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
> > >  	 * duplicate the "dummy propagation" logic.
> > >  	 */
> > >  	bridge_state->input_bus_cfg.flags = output_flags;
> > > +
> > > +	/*
> > > +	 * Use the bridge->timings->input_bus_flags as fallback if the next bridge
> > > +	 * does not specify the flags
> > > +	 */
> > > +	if (!bridge_state->input_bus_cfg.flags)
> > > +		bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
> > 
> > According to docs, timings can be NULL.

Correct.

> > And, hmm... It's too easy to get confused with these, but... If the bridge defines timings, and
> > timings->input_bus_flags != 0, should we always pick that, even if we got something via
> > output_flags? Logic being, if this bridge defines timings->input_bus_flags, it probably wants that
> > to be used regardless whether we got something from the next bridge.

timings->input_bus_flags is an API that predates format and flags
propagation along the pipeline. I would assume that drivers that
implement propagation should do so in a way that prioritize that API,
and either not report flags in the timings (in which case giving
priority to one or another wouldn't make a difference as both would
never be provided together), or would report flags in the timings as a
best effort fallback for display controller drivers that would look at
them exclusively without supporting the new API. I would thus think that
the flags obtained through format negotiation should be prioritized.

> Got it, the flags from timings if present should be prioritized rather
> than treating them as fallback.
Tomi Valkeinen Oct. 30, 2020, 7:30 a.m. UTC | #4
On 30/10/2020 00:48, Laurent Pinchart wrote:

>>> And, hmm... It's too easy to get confused with these, but... If the bridge defines timings, and
>>> timings->input_bus_flags != 0, should we always pick that, even if we got something via
>>> output_flags? Logic being, if this bridge defines timings->input_bus_flags, it probably wants that
>>> to be used regardless whether we got something from the next bridge.
> 
> timings->input_bus_flags is an API that predates format and flags
> propagation along the pipeline. I would assume that drivers that
> implement propagation should do so in a way that prioritize that API,
> and either not report flags in the timings (in which case giving
> priority to one or another wouldn't make a difference as both would
> never be provided together), or would report flags in the timings as a
> best effort fallback for display controller drivers that would look at
> them exclusively without supporting the new API. I would thus think that
> the flags obtained through format negotiation should be prioritized.

What do you mean "drivers that implement propagation"? Doesn't that come from the framework, not
from the drivers?

Say, we have two bridges, A -> B. A has timings->input_bus_flags.

When propagating the flags, we get something as B's input flags. Should A use B's input flags as A's
input flags, or should A use its timings->input_bus_flags? I was suggesting the latter. Nikhil's
patch does the latter, but only if B's input flags was 0.

A can override its input flags manually in atomic_check, but if the timings->input_bus_flags exists,
isn't it a sane choice to just pick that by default?

 Tomi
Boris Brezillon Oct. 30, 2020, 8:08 a.m. UTC | #5
On Fri, 30 Oct 2020 09:30:01 +0200
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> On 30/10/2020 00:48, Laurent Pinchart wrote:
> 
> >>> And, hmm... It's too easy to get confused with these, but... If the bridge defines timings, and
> >>> timings->input_bus_flags != 0, should we always pick that, even if we got something via
> >>> output_flags? Logic being, if this bridge defines timings->input_bus_flags, it probably wants that
> >>> to be used regardless whether we got something from the next bridge.  
> > 
> > timings->input_bus_flags is an API that predates format and flags
> > propagation along the pipeline. I would assume that drivers that
> > implement propagation should do so in a way that prioritize that API,
> > and either not report flags in the timings (in which case giving
> > priority to one or another wouldn't make a difference as both would
> > never be provided together), or would report flags in the timings as a
> > best effort fallback for display controller drivers that would look at
> > them exclusively without supporting the new API. I would thus think that
> > the flags obtained through format negotiation should be prioritized.  
> 
> What do you mean "drivers that implement propagation"? Doesn't that come from the framework, not
> from the drivers?
> 
> Say, we have two bridges, A -> B. A has timings->input_bus_flags.
> 
> When propagating the flags, we get something as B's input flags. Should A use B's input flags as A's
> input flags, or should A use its timings->input_bus_flags? I was suggesting the latter. Nikhil's
> patch does the latter, but only if B's input flags was 0.

A should definitely use timings->input_bus_flags in that case.

> 
> A can override its input flags manually in atomic_check, but if the timings->input_bus_flags exists,
> isn't it a sane choice to just pick that by default?

The "propagate output flags" and soon to be added "use
timing->input_flags if present" logic should only be used as a fallback
for bridges that do not support dynamic bus format/flags negotiation
IMHO. Ideally we'd want to convert all bridges to do this dynamic bus
format/flags negotiation and get rid of timings->input_bus_flags once
this is done, but that's likely to take time. So, if your driver
implements the ->atomic_check() hook and needs specific input flags,
I'd recommend setting the input flags there instead of specifying it
through timings->input_bus_flags.
Tomi Valkeinen Oct. 30, 2020, 8:40 a.m. UTC | #6
Hi Boris,

On 30/10/2020 10:08, Boris Brezillon wrote:
> The "propagate output flags" and soon to be added "use
> timing->input_flags if present" logic should only be used as a fallback
> for bridges that do not support dynamic bus format/flags negotiation
> IMHO. Ideally we'd want to convert all bridges to do this dynamic bus
> format/flags negotiation and get rid of timings->input_bus_flags once
> this is done, but that's likely to take time. So, if your driver
> implements the ->atomic_check() hook and needs specific input flags,
> I'd recommend setting the input flags there instead of specifying it
> through timings->input_bus_flags.

What is bus flags negotiation? Don't we have negotiation only for bus formats? Bus flags are just
set, and the previous bridge in the chain has to use those flags.

Or do you just refer to setting the bus flags dynamically in atomic_check, versus static in
input_bus_flags?

 Tomi
Boris Brezillon Oct. 30, 2020, 8:50 a.m. UTC | #7
On Fri, 30 Oct 2020 10:40:46 +0200
Tomi Valkeinen <tomi.valkeinen@ti.com> wrote:

> Hi Boris,
> 
> On 30/10/2020 10:08, Boris Brezillon wrote:
> > The "propagate output flags" and soon to be added "use
> > timing->input_flags if present" logic should only be used as a fallback
> > for bridges that do not support dynamic bus format/flags negotiation
> > IMHO. Ideally we'd want to convert all bridges to do this dynamic bus
> > format/flags negotiation and get rid of timings->input_bus_flags once
> > this is done, but that's likely to take time. So, if your driver
> > implements the ->atomic_check() hook and needs specific input flags,
> > I'd recommend setting the input flags there instead of specifying it
> > through timings->input_bus_flags.  
> 
> What is bus flags negotiation? Don't we have negotiation only for bus formats? Bus flags are just
> set, and the previous bridge in the chain has to use those flags.

Well, there's currently no such negotiation, but I don't see why there
wouldn't be one at some point if bridges can configure it dynamically
(in you A -> B example, A output flags must match B input flags, and if
both A and B can configure those dynamically, they need to negotiate
that part too).

> 
> Or do you just refer to setting the bus flags dynamically in atomic_check, versus static in
> input_bus_flags?

Yes, that's what I suggest, even if those flags are static right now.
diff mbox series

Patch

diff --git a/drivers/gpu/drm/drm_bridge.c b/drivers/gpu/drm/drm_bridge.c
index 64f0effb52ac..8353723323ab 100644
--- a/drivers/gpu/drm/drm_bridge.c
+++ b/drivers/gpu/drm/drm_bridge.c
@@ -975,6 +975,13 @@  drm_atomic_bridge_propagate_bus_flags(struct drm_bridge *bridge,
 	 * duplicate the "dummy propagation" logic.
 	 */
 	bridge_state->input_bus_cfg.flags = output_flags;
+
+	/*
+	 * Use the bridge->timings->input_bus_flags as fallback if the next bridge
+	 * does not specify the flags
+	 */
+	if (!bridge_state->input_bus_cfg.flags)
+		bridge_state->input_bus_cfg.flags = bridge->timings->input_bus_flags;
 }
 
 /**