Re: [PATCH 0/2] OMAPDSS: DISPC: Improvements to DIGIT sync signal selection

[Ricardo Neri <ricardo.neri@ti.com>]

Hi Tomi, Archit,

Thanks for your feedback.

On 07/30/2012 06:48 AM, Archit Taneja wrote:
> On Monday 30 July 2012 04:41 PM, Tomi Valkeinen wrote:
>> On Fri, 2012-07-27 at 19:21 -0500, Ricardo Neri wrote:
>>> Hello,
>>>
>>> DSS code wrongly assumes that VENC is always available as source for
>>> the external
>>> sync signal for the display controller DIGIT channel. One cannot blindly
>>> rely only on the value of DSS_CONTROL register as certain processors
>>> may not
>>> have VENC (e.g., OMAP5).
>>>
>>> These patches add logic to correctly set/get the sync signal based on
>>> the
>>> available displays in the DIGIT channel.
>>>
>>> For the HDMI driver, handle the error in the selection of the source.
>>
>> Is there an actual problem seen that this resolves? If so, It'd be nice
>> to have a brief description of the problem.
>
> The issue is seen on OMAP5 with a 3.5 kernel. In 3.5 kernel, we had a
> hack where we used to divide the TV manager's height by 2 if the
> connected interface was VENC. On OMAP5, the bit 15 returns VENC (a zero)
> irrespective of what we write.
>
> This won't happen in 3.6 as we have included interlace as a part of the
> omap_video_timings struct, and we don't try to check the bit to see
> whether we are VENC or HDMI.
>
> I guess we should still put a check to not set the bit on OMAP5 as it
> may cause unknown HW behavior, i.e, make the select function like:
>
> void dss_select_hdmi_venc_clk_source()
> {
>      ...
>
>      displays =
>          dss_feat_get_supported_displays(OMAP_DSS_CHANNEL_DIGIT);
>
>      if ((displays & OMAP_DISPLAY_TYPE_VENC) == 0)
>          return;
>
>      REG_FLD_MOD(DSS_CONTROL, src, 15, 15);    /* VENC_HDMI_SWITCH */
> }

As Tomi mentions, perhaps this could be complemented with a BUG_ON could 
be used if someone tries to use VENC as source when this is not available.
>
> Archit
>
>>
>> Does the bit 15 in DSS_CONTROL still exist on OMAP5? What does it return
>> on OMAP5?
>>
>> I think it's clear that dss_get_hdmi_venc_clk_source() needs to return a
>> correct value so that DSS operates correctly. But I'm not sure we need
>> the extra code in dss_select_hdmi_venc_clk_source().

Yes, the main purpose of these patch was to avoid blindly reading 
DSS_CONTROL[15]. The additions to selection were just for completeness.
>>
>> dss_select_hdmi_venc_clk_source() should be called by venc or hdmi, and
>> there should be any possibility of using it in the wrong way. For extra
>> sanity checking there could be a BUG_ON() check in the
>> dss_select_hdmi_venc_clk_source() to point out if there's a bad bug in
>> venc or hdmi.
>>
>> I'd like to keep the lowest level dss-internal functions as simple as
>> possible, and written in the manner that they presume they are called
>> correctly. Otherwise we'll end up with lots of new code, checking for
>> errors that can never happen in practice.
Well, this was an error that happened in practice :).

I will resubmit based on these comments.

Thanks,

Ricardo
>>
>>   Tomi
>>
>