On 04/28/17 13:30, Tomi Valkeinen wrote:
> On 14/04/17 13:25, Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>>
>> The hdmi_power_on/off_core functions can be called multiple times:
>> when the HPD changes and when the HDMI CEC support needs to power
>> the HDMI core.
>>
>> So use a counter to know when to really power on or off the HDMI core.
>>
>> Also call hdmi4_core_powerdown_disable() in hdmi_power_on_core() to
>> power up the HDMI core (needed for CEC).
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>> ---
>> drivers/gpu/drm/omapdrm/dss/hdmi4.c | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/dss/hdmi4.c b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
>> index 4a164dc01f15..e371b47ff6ff 100644
>> --- a/drivers/gpu/drm/omapdrm/dss/hdmi4.c
>> +++ b/drivers/gpu/drm/omapdrm/dss/hdmi4.c
>> @@ -124,14 +124,19 @@ static int hdmi_power_on_core(struct omap_dss_device *dssdev)
>> {
>> int r;
>>
>> + if (hdmi.core.core_pwr_cnt++)
>> + return 0;
>> +
>
> How's the locking between the CEC side and the DRM side? Normally these
> functions are protected with the DRM modesetting locks, but CEC doesn't
> come from there. We have the hdmi.lock, did you check that it's held
> when CEC side calls shared functions?
Yes, the hdmi_power_on/off_core functions are all called from other functions
with the hdmi.lock taken. The CEC code calls those higher level functions
(hdmi4_core_enable/disable).
>
>> r = regulator_enable(hdmi.vdda_reg);
>> if (r)
>> - return r;
>> + goto err_reg_enable;
>>
>> r = hdmi_runtime_get();
>> if (r)
>> goto err_runtime_get;
>>
>> + hdmi4_core_powerdown_disable(&hdmi.core);
>
> I'd like to have the powerdown_disable as a separate patch.
Will do.
> Also, now
> that you call it here, I believe it can be dropped from hdmi4_configure().
I was a bit scared of messing with that function. But if you say it can
be removed, then who am I to argue? :-)
>
> Hmm, but in hdmi4_configure we call hdmi_core_swreset_assert() before
> hdmi4_core_powerdown_disable(). I wonder what exactly that does, and
> whether we end up resetting also the CEC parts when we change the videomode.
Good one. I'll attempt to check this.
Regards,
Hans
>
> Tomi
>
