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

[Tomi Valkeinen]

On 04/12/2020 15:54, Boris Brezillon wrote:
> On Fri, 4 Dec 2020 13:47:05 +0200
> Tomi Valkeinen <tomi.valkeinen at ti.com> 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

-- 
Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki