Hi,
Tomi Valkeinen wrote:
> On Tue, 2010-11-16 at 12:22 +0100, ext Taneja, Archit wrote:
>> Hi,
>>
>> Tomi Valkeinen wrote:
>>> Hi,
>>>
>>
>> [snip]
>>
>>>> diff --git a/arch/arm/plat-omap/include/plat/display.h
>>>> b/arch/arm/plat-omap/include/plat/display.h
>>>> index 586944d..3e6eec1 100644
>>>> --- a/arch/arm/plat-omap/include/plat/display.h
>>>> +++ b/arch/arm/plat-omap/include/plat/display.h
>>>> @@ -434,6 +434,7 @@ struct omap_dss_device {
>>>> struct omap_overlay_manager *manager;
>>>>
>>>> enum omap_dss_display_state state;
>>>> + enum omap_channel channel;
>>>
>>> Isn't this the same as dssdev->manager->id?
>>>
>>> Yes, this channel stuff is a bit confusing, even the enum "omap_channel"
>>> is badly named (should at least have "dss" in it). But
>>> omap_overlay_manager should represent the output pipe, so I don't
>>> think there's need for channel field in dss_device.
>>
>> The panel somehow needs to tell which manager it is connected to. We
>> went with with the way of declaring a new member 'channel' and setting
>> that parameter up in the board file.
>>
>> The current way to relate the manager and device is done by checking
>> the dssdev->type in dss_recheck_connections() and then calling set_device()
>>
>> This is not sufficient any more since two of the managers can support
>> similar types of displays.
>>
>> So in the channel approach the following happens:
>>
>> -mgr->id's are initialized at bootup.
>> -devices and managers are connected using 'channel'.
>> -mgr->id automatically becomes equal to channel.
>>
>> Can we set something like dssdev->manager->id in the board file itself?
>
> Right, now I see.
>
> The dss_recheck_connections() felt like a hack when I wrote it =).
> Clearly we need some way to define in the board file which
> panel connects to which manager.
>
It wasn't needed probably for OMAP3 since all non-venc panels connect
to LCD and venc to VENC type.
Now that I think of it, what channel would we mention if the panel is
used in dsi l4 update mode or dma mode? It should have no manager at all.
> Perhaps move the channel-field up, just below "enum
> omap_display_type type". The lines in the beginning of
> omap_dss_device are things which define the interface etc,
> and later there are miscallaneous dynamic things. And this belongs to the
> former.
>
> Now, if we have the channel defined in this way in the
> omap_dss_device, I'm wondering if:
> 1) the manager pointer is needed at all, as the channel tells which manager
> it is? 2) if we keep the manager pointer (as a helper shortcut),
> should the code use generally use dssdev->channel or dssdev->manager->id?
I think manager->id makes more sense considering your logic below.
> 3) having this channel field requires a change to every board
> file, because the channel has to be defined?
>
Yes, that is something that has to be done for all 'DIGIT'
panels across all boards.
> And answering to myself, I guess the manager pointer is
> needed, because
> DSS2 supports (at least in theory) multiple panels in the
> same interface (not at the same time, of course). This means
> that we could have to panels, with the same interface and
> channel, but only one would be in use. So the one in use
> should have manager pointing to the correct manager, and the
> other would have manager NULL.
>
Yes, passing dssdev->channel would make things worse if 2 panels
are connected to same interface.
> And thus perhaps using dssdev->channel only for connecting
> the right manager to right panel, and using
> dssdev->manager->id for other uses would be the best? The
> values would of course be the same, but at least we'd get a
> crash if somehow an unconnected display is being used for configuration.
>
Even in the current kernel on 3430sdp, the board file adds sharp_ls, venc and generic panels
If I boot up with venc as default display, mgr0 has dvi as its display and mgr1 has tv.
So if I try to enable sharp_ls panel I get a crash when we first try to access dssdev->manager
in dpi_display_enable(). We should have a way to prevent enabling a panel if it isn't connected
to a manager. We should also have a way to allow a panel to have no channel at all (something like
OMAP_DSS_CHANNEL_NONE) if we are using something like dsi l4 update.
I won't add this extra channel now though, we need to think of a better way later.
> Does this make sense?
Yes, it does, I'll make these changes, and more if you can think of any.
Archit--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
