[v4,5/7] drm/tidss: Set bus_format correctly from bridge/connector

Commit Message

Nikhil Devshatwar Dec. 1, 2020, 12:18 p.m. UTC

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 <[e-mail]>
Reviewed-by: Tomi Valkeinen <[e-mail]>
---

Notes:
    changes from v3:
    * cosmetic updates
    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(-)

Comments

Boris Brezillon Dec. 4, 2020, 10:50 a.m. UTC | #1

On Tue, 1 Dec 2020 17:48:28 +0530
Nikhil Devshatwar <[e-mail]> 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.

That'd be even better if you implement the bridge interface instead of
the encoder one so we can get rid of the encoder_{helper}_funcs and use
drm_simple_encoder_init().

> 
> Signed-off-by: Nikhil Devshatwar <[e-mail]>
> Reviewed-by: Tomi Valkeinen <[e-mail]>
> ---
> 
> Notes:
>     changes from v3:
>     * cosmetic updates
>     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..5deb8102e4d3 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) {
> +		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__);
> +	tcrtc_state->bus_format = bstate->input_bus_cfg.format;
> +	tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;
> +
> +	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;
>  }
>

Tomi Valkeinen Dec. 4, 2020, 10:56 a.m. UTC | #2

Hi Boris,

On 04/12/2020 12:50, Boris Brezillon wrote:
> On Tue, 1 Dec 2020 17:48:28 +0530
> Nikhil Devshatwar <[e-mail]> 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.
> 
> That'd be even better if you implement the bridge interface instead of
> the encoder one so we can get rid of the encoder_{helper}_funcs and use
> drm_simple_encoder_init().

I'm a bit confused here. What should be the design here...

These formats need to be handled by the crtc (well, the display controller, which is modeled as the
crtc). Should we have a tidss_crtc.c file, which implements a crtc, a simple encoder and a bridge?
And just consider those three DRM components as different API parts of the display controller?

 Tomi

Boris Brezillon Dec. 4, 2020, 11:12 a.m. UTC | #3

On Fri, 4 Dec 2020 12:56:27 +0200
Tomi Valkeinen <[e-mail]> wrote:

> Hi Boris,
> 
> On 04/12/2020 12:50, Boris Brezillon wrote:
> > On Tue, 1 Dec 2020 17:48:28 +0530
> > Nikhil Devshatwar <[e-mail]> 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.  
> > 
> > That'd be even better if you implement the bridge interface instead of
> > the encoder one so we can get rid of the encoder_{helper}_funcs and use
> > drm_simple_encoder_init().  
> 
> I'm a bit confused here. What should be the design here...
> 
> These formats need to be handled by the crtc (well, the display controller, which is modeled as the
> crtc). Should we have a tidss_crtc.c file, which implements a crtc, a simple encoder and a bridge?
> And just consider those three DRM components as different API parts of the display controller?

The idea is to hide the encoder abstraction from drivers as much as we
can. We have to keep the encoder concept because that's what we expose
to userspace, but drivers shouldn't have to worry about the distinction
between the first bridge in the chain (what we currently call encoders)
and other bridges. The bridge interface provides pretty much the same
functionality, so all you need to do is turn your encoder driver into a
bridge driver (see what we did for
drivers/gpu/drm/imx/parallel-display.c), the only particularity here is
that the bridge knows it's the first in the chain, and has access to
the CRTC it's directly connected to.

IMHO, the less interfaces we keep the clearer it gets for our users.
Getting rid of the encoder_{helper_}funcs in favor or bridge_ops would
clarify the fact that any kind of encoder, no matter if it's the first
in the chain or not, should be represented as a bridge object.

Tomi Valkeinen Dec. 4, 2020, 11:47 a.m. UTC | #4

On 04/12/2020 13:12, Boris Brezillon wrote:

>>> That'd be even better if you implement the bridge interface instead of
>>> the encoder one so we can get rid of the encoder_{helper}_funcs and use
>>> drm_simple_encoder_init().  
>>
>> I'm a bit confused here. What should be the design here...
>>
>> These formats need to be handled by the crtc (well, the display controller, which is modeled as the
>> crtc). Should we have a tidss_crtc.c file, which implements a crtc, a simple encoder and a bridge?
>> And just consider those three DRM components as different API parts of the display controller?
> 
> The idea is to hide the encoder abstraction from drivers as much as we
> can. We have to keep the encoder concept because that's what we expose
> to userspace, but drivers shouldn't have to worry about the distinction
> between the first bridge in the chain (what we currently call encoders)
> and other bridges. The bridge interface provides pretty much the same
> functionality, so all you need to do is turn your encoder driver into a
> bridge driver (see what we did for
> drivers/gpu/drm/imx/parallel-display.c), the only particularity here is
> that the bridge knows it's the first in the chain, and has access to
> the CRTC it's directly connected to.

With a quick look, the imx parallel-display.c seems to be really part of the crtc. Shouldn't we then
take one more step forward, and just combine the crtc, encoder and bridge (somehow)? That's kind of
what parallel-display.c is doing, isn't it? It's directly poking the crtc state, but the code just
happens to be in a separate file from the crtc.

Thinking about the TI HW, we have a bunch of internal IPs which are clearly bridges: HDMI, DSI,
which get the pixel data from the display controller, and they have their own registers, so clearly
independent bridges.

Then we have MIPI DPI, which doesn't really have its own registers, as it's just plain parallel RGB,
the same as what is sent to e.g. HDMI and DSI bridges. However, there might be some muxes or
regulators to set up to get the external signals working. So a bridge would be ok here too to handle
that external side.

But in all the above cases, we have the display controller (crtc), which in all the cases needs to
do e.g. pixel/bus format configuration. So why add direct crtc poking into the first bridges (HDMI,
DSI, DPI), when we could just model the crtc as a bridge, and thus the first bridges wouldn't need
to touch the crtc.

 Tomi

Boris Brezillon Dec. 4, 2020, 1:54 p.m. UTC | #5

On Fri, 4 Dec 2020 13:47:05 +0200
Tomi Valkeinen <[e-mail]> wrote:

> On 04/12/2020 13:12, Boris Brezillon wrote:
> 
> >>> That'd be even better if you implement the bridge interface instead of
> >>> the encoder one so we can get rid of the encoder_{helper}_funcs and use
> >>> drm_simple_encoder_init().    
> >>
> >> I'm a bit confused here. What should be the design here...
> >>
> >> These formats need to be handled by the crtc (well, the display controller, which is modeled as the
> >> crtc). Should we have a tidss_crtc.c file, which implements a crtc, a simple encoder and a bridge?
> >> And just consider those three DRM components as different API parts of the display controller?  
> > 
> > The idea is to hide the encoder abstraction from drivers as much as we
> > can. We have to keep the encoder concept because that's what we expose
> > to userspace, but drivers shouldn't have to worry about the distinction
> > between the first bridge in the chain (what we currently call encoders)
> > and other bridges. The bridge interface provides pretty much the same
> > functionality, so all you need to do is turn your encoder driver into a
> > bridge driver (see what we did for
> > drivers/gpu/drm/imx/parallel-display.c), the only particularity here is
> > that the bridge knows it's the first in the chain, and has access to
> > the CRTC it's directly connected to.  
> 
> With a quick look, the imx parallel-display.c seems to be really part of the crtc. Shouldn't we then
> take one more step forward, and just combine the crtc, encoder and bridge (somehow)? That's kind of
> what parallel-display.c is doing, isn't it? It's directly poking the crtc state, but the code just
> happens to be in a separate file from the crtc.

Right. If we want to keep the code split, the logic should probably be
reversed, with the CRTC driver checking the first bridge state to setup
its internal state. Those DPI encoders are always a bit special, since
they tend to be directly embedded in the block responsible for timing
control (what we call CRTCs), so you're right, maybe we should model
that as a CRTC+bridge pair.

> 
> Thinking about the TI HW, we have a bunch of internal IPs which are clearly bridges: HDMI, DSI,
> which get the pixel data from the display controller, and they have their own registers, so clearly
> independent bridges.
> 
> Then we have MIPI DPI, which doesn't really have its own registers, as it's just plain parallel RGB,
> the same as what is sent to e.g. HDMI and DSI bridges.

I still consider that one a bridge, even if the translation is almost
transparent from a HW PoV.

> However, there might be some muxes or
> regulators to set up to get the external signals working. So a bridge would be ok here too to handle
> that external side.

Exactly.

> 
> But in all the above cases, we have the display controller (crtc), which in all the cases needs to
> do e.g. pixel/bus format configuration. So why add direct crtc poking into the first bridges (HDMI,
> DSI, DPI), when we could just model the crtc as a bridge, and thus the first bridges wouldn't need
> to touch the crtc.

Yes, that's an option. I mean, you can already modify your CRTC
logic to implement the bridge and CRTC interface and have your
driver-specific CRTC object embed both a drm_crtc and a drm_bridge.

Tomi Valkeinen Dec. 4, 2020, 2:19 p.m. UTC | #6

On 04/12/2020 15:54, Boris Brezillon wrote:
> On Fri, 4 Dec 2020 13:47:05 +0200
> Tomi Valkeinen <[e-mail]> wrote:
> 
>> On 04/12/2020 13:12, Boris Brezillon wrote:
>>
>>>>> That'd be even better if you implement the bridge interface instead of
>>>>> the encoder one so we can get rid of the encoder_{helper}_funcs and use
>>>>> drm_simple_encoder_init().    
>>>>
>>>> I'm a bit confused here. What should be the design here...
>>>>
>>>> These formats need to be handled by the crtc (well, the display controller, which is modeled as the
>>>> crtc). Should we have a tidss_crtc.c file, which implements a crtc, a simple encoder and a bridge?
>>>> And just consider those three DRM components as different API parts of the display controller?  
>>>
>>> The idea is to hide the encoder abstraction from drivers as much as we
>>> can. We have to keep the encoder concept because that's what we expose
>>> to userspace, but drivers shouldn't have to worry about the distinction
>>> between the first bridge in the chain (what we currently call encoders)
>>> and other bridges. The bridge interface provides pretty much the same
>>> functionality, so all you need to do is turn your encoder driver into a
>>> bridge driver (see what we did for
>>> drivers/gpu/drm/imx/parallel-display.c), the only particularity here is
>>> that the bridge knows it's the first in the chain, and has access to
>>> the CRTC it's directly connected to.  
>>
>> With a quick look, the imx parallel-display.c seems to be really part of the crtc. Shouldn't we then
>> take one more step forward, and just combine the crtc, encoder and bridge (somehow)? That's kind of
>> what parallel-display.c is doing, isn't it? It's directly poking the crtc state, but the code just
>> happens to be in a separate file from the crtc.
> 
> Right. If we want to keep the code split, the logic should probably be
> reversed, with the CRTC driver checking the first bridge state to setup
> its internal state. Those DPI encoders are always a bit special, since
> they tend to be directly embedded in the block responsible for timing
> control (what we call CRTCs), so you're right, maybe we should model
> that as a CRTC+bridge pair.
> 
>>
>> Thinking about the TI HW, we have a bunch of internal IPs which are clearly bridges: HDMI, DSI,
>> which get the pixel data from the display controller, and they have their own registers, so clearly
>> independent bridges.
>>
>> Then we have MIPI DPI, which doesn't really have its own registers, as it's just plain parallel RGB,
>> the same as what is sent to e.g. HDMI and DSI bridges.
> 
> I still consider that one a bridge, even if the translation is almost
> transparent from a HW PoV.
> 
>> However, there might be some muxes or
>> regulators to set up to get the external signals working. So a bridge would be ok here too to handle
>> that external side.
> 
> Exactly.
> 
>>
>> But in all the above cases, we have the display controller (crtc), which in all the cases needs to
>> do e.g. pixel/bus format configuration. So why add direct crtc poking into the first bridges (HDMI,
>> DSI, DPI), when we could just model the crtc as a bridge, and thus the first bridges wouldn't need
>> to touch the crtc.
> 
> Yes, that's an option. I mean, you can already modify your CRTC
> logic to implement the bridge and CRTC interface and have your
> driver-specific CRTC object embed both a drm_crtc and a drm_bridge.

Alright, thanks for the clarifications!

I think the best first step here is what you proposed, use the bridge interface and
drm_simple_encoder_init(), as that should solve the issue at hand quite neatly.

 Tomi

Francesco Dolcini March 30, 2023, 5:17 p.m. UTC | #7

Hello,
chiming in in this *old* email thread.

Adding in copy a few more *@ti.com people recently involved in other tidss
changes [1]


[1] https://lore.kernel.org/all/[e-mail]/


On Fri, Dec 04, 2020 at 11:50:30AM +0100, Boris Brezillon wrote:
> On Tue, 1 Dec 2020 17:48:28 +0530
> Nikhil Devshatwar <[e-mail]> 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.
> 
> That'd be even better if you implement the bridge interface instead of
> the encoder one so we can get rid of the encoder_{helper}_funcs and use
> drm_simple_encoder_init().

Did anything happened on this old discussion? I was working on a very
similar change and after a while I realized about this thread.

Thanks,
Francesco

Aradhya Bhatia April 14, 2023, 7:49 a.m. UTC | #8

Hi Francesco,

On 30-Mar-23 22:47, Francesco Dolcini wrote:
> Hello,
> chiming in in this *old* email thread.
> 
> Adding in copy a few more *@ti.com people recently involved in other tidss
> changes [1]
> 
> 
> [1] https://lore.kernel.org/all/[e-mail]/
> 
> 
> On Fri, Dec 04, 2020 at 11:50:30AM +0100, Boris Brezillon wrote:
>> On Tue, 1 Dec 2020 17:48:28 +0530
>> Nikhil Devshatwar <[e-mail]> 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.
>>
>> That'd be even better if you implement the bridge interface instead of
>> the encoder one so we can get rid of the encoder_{helper}_funcs and use
>> drm_simple_encoder_init().
> 
> Did anything happened on this old discussion? I was working on a very
> similar change and after a while I realized about this thread.
> 
Nikhil has moved on from TI.

I will be taking up this patch series and implement the changes
requested.


Boris, Tomi

Are the changes discussed on this patch still relevant for the display
controller driver to support DRM_BRIDGE_ATTACH_NO_CONNECTOR?

Or are there some new changes in the framework that I should be looking
at?

Regards
Aradhya

Francesco Dolcini April 14, 2023, 8:30 a.m. UTC | #9

Hello Aradhya,

On Fri, Apr 14, 2023 at 01:19:38PM +0530, Aradhya Bhatia wrote:
> On 30-Mar-23 22:47, Francesco Dolcini wrote:
> > Hello,
> > chiming in in this *old* email thread.
> > 
> > Adding in copy a few more *@ti.com people recently involved in other tidss
> > changes [1]
> > 
> > 
> > [1] https://lore.kernel.org/all/[e-mail]/
> > 
> > 
> > On Fri, Dec 04, 2020 at 11:50:30AM +0100, Boris Brezillon wrote:
> >> On Tue, 1 Dec 2020 17:48:28 +0530
> >> Nikhil Devshatwar <[e-mail]> 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.
> >>
> >> That'd be even better if you implement the bridge interface instead of
> >> the encoder one so we can get rid of the encoder_{helper}_funcs and use
> >> drm_simple_encoder_init().
> > 
> > Did anything happened on this old discussion? I was working on a very
> > similar change and after a while I realized about this thread.
> > 
> Nikhil has moved on from TI.
> 
> I will be taking up this patch series and implement the changes
> requested.
Great!

What I was working on is really about having the media bus format taken
from the closest bridge, this is really required for proper functionality
when the display is connected through a bridge.

This [1] is what I was working on before realizing about this specific
patch here, in case you want to have a look.

Please keep me in CC, I can test/review patches.

Francesco

[1] https://github.com/dolcini/linux/commit/150b1390bd4122a77c873d87bf506024f9775755

Aradhya Bhatia April 14, 2023, 8:18 p.m. UTC | #10

Hi Francesco,

On 14-Apr-23 14:00, Francesco Dolcini wrote:
> Hello Aradhya,
> 
> On Fri, Apr 14, 2023 at 01:19:38PM +0530, Aradhya Bhatia wrote:
>> On 30-Mar-23 22:47, Francesco Dolcini wrote:
>>> Hello,
>>> chiming in in this *old* email thread.
>>>
>>> Adding in copy a few more *@ti.com people recently involved in other tidss
>>> changes [1]
>>>
>>>
>>> [1] https://lore.kernel.org/all/[e-mail]/
>>>
>>>
>>> On Fri, Dec 04, 2020 at 11:50:30AM +0100, Boris Brezillon wrote:
>>>> On Tue, 1 Dec 2020 17:48:28 +0530
>>>> Nikhil Devshatwar <[e-mail]> 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.
>>>>
>>>> That'd be even better if you implement the bridge interface instead of
>>>> the encoder one so we can get rid of the encoder_{helper}_funcs and use
>>>> drm_simple_encoder_init().
>>>
>>> Did anything happened on this old discussion? I was working on a very
>>> similar change and after a while I realized about this thread.
>>>
>> Nikhil has moved on from TI.
>>
>> I will be taking up this patch series and implement the changes
>> requested.
> Great!
> 
> What I was working on is really about having the media bus format taken
> from the closest bridge, this is really required for proper functionality
> when the display is connected through a bridge.

I agree with you there. The current code will not work well when the
media bus format required by the final sink is not same as the one tidss
needs to output.

> 
> This [1] is what I was working on before realizing about this specific
> patch here, in case you want to have a look.
> 
> Please keep me in CC, I can test/review patches.

Thank you! Will do. =)


Regards
Aradhya

Patch

diff --git a/drivers/gpu/drm/tidss/tidss_encoder.c b/drivers/gpu/drm/tidss/tidss_encoder.c
index e278a9c89476..5deb8102e4d3 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) {
+		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__);
+	tcrtc_state->bus_format = bstate->input_bus_cfg.format;
+	tcrtc_state->bus_flags = bstate->input_bus_cfg.flags;
+
+	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;
 }