Re: [PATCH 4/4] omapdss: features: fixed supported outputs for OMAP4

[Archit Taneja <archit@ti.com>]

On Tuesday 12 March 2013 07:59 PM, Tomi Valkeinen wrote:
> On 2013-03-12 16:01, Archit Taneja wrote:
>> On Tuesday 12 March 2013 07:07 PM, Tomi Valkeinen wrote:
>
>>> So, I don't disagree with you. But I don't quite understand why we could
>>> not use the fixed channels for now? They should work in all the boards
>>> we have, right? Or is there something with DRM that forces the driver to
>>> select the channel dynamically?
>>
>> I think we can use fixed channels, but if the number of different fixed
>> channels crosses the number of crtcs the kernel wants, then we would
>> need to atleast change the channels of some of the outputs.
>>
>> For example, suppose omapdrm is asked to use only 2 crtcs, and it picks
>> up LCD2 and TV managers. Now if there is some panel which says it's
>> recommended channel is LCD, then things won't work.
>
> Are you saying omapdrm picks the managers for the crtcs before knowing
> what panels there are? That can't work right... We need to know what
> outputs are to be used before we can select the managers. Or, we always
> need crtcs for all the managers.

That's how it is right now.

>
> If we do know the panels, and thus outputs, then the managers to be used
> are found easily from output->dispc_channel.

Yes, my patch tries to do the same, but it could assign a manger which 
isn't the recommended channel. It can pick one from the list of 
supported channels. I've explained it a bit more below.

>
> But, of course, the crtc to manager mapping could be changed (if omapdrm
> supports this). If omapdrm is asked to use only 1 crtc, but there are
> two panels, then only one panel can be used at a time, and the manager
> for the crtc needs to be changed when the panel to be used is changed.
> But even in this case used manager is clear, it comes from
> output->dispc_channel.

This is something I don't know that can be done or not, or if it can be 
done easily. A crtc isn't purely an overlay manager. It also needs to 
have one plane associated to it. So, if we want to change the overlay 
manager tied to a crtc on the fly, we should make sure that it's still 
connected to a plane pointing to the same buffer. This needs a better 
understanding of drm internals. I guess Rob could answer this better.

>
>> At the moment, omapdrm maps a crtc with a manger using a function called
>> pipe2chan() which just selects a manager with the biggest channel no. So
>> if the kernel is configured to have num_crtcs as 1. The single crtc will
>> be mapped to LCD2. This method is wrong, as it doesn't even look at the
>> type of panels at all. For an omap5 panda, the most suitable manager to
>> map to the crtc would be TV(for hdmi).
>>
>> I think what we probably need to do is to combine both the methods. I.e,
>> make each output connectible to only one channel, and also iterate
>> through the panels in omapdrm to find the most suitable channels. So in
>> my patch, instead of looking at all the supported managers for an
>> output(checking with dss_feat_get_supported_outputs() on each manager),
>> I just look at the recommended channel, and try to map that manager.
>
> I don't know, I feel like I'm not understanding something here =).

:)

I think what I said above is equivalent to what you said here:

"If we do know the panels, and thus outputs, then the managers to be 
used are found easily from output->dispc_channel."

Instead of using output->dispc_channel, the patch I made tried to get 
the first supported channel(in increasing order of channel index) for an 
output. If that channel was already taken/reserved by a previous output, 
it tries to take the next supported channel if there are enough crtcs left.

My patch would break things if it chooses a manager for an output for 
which we haven't written the necessary code yet(switching clock sources 
etc).

So, what I'm saying is that we should stick to output->dispc_channel. We 
iterate through all the panels, and by using output->dispc_channel, we 
get the manager for an output, and map that manager to a crtc, and make 
sure the number of unique managers we finally use is equal to NUM_CRTC.

Does that sound good?

Archit