Re: [PATCH 6/8] omapdrm: hdmi4: refcount hdmi_power_on/off_core

[Hans Verkuil]

On 04/28/17 13:30, Tomi Valkeinen wrote:
> On 14/04/17 13:25, Hans Verkuil wrote:
>> From: Hans Verkuil 
>>
>> 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 
>> ---
>>  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();
> 
> 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
> 

Re: [PATCH 6/8] omapdrm: hdmi4: refcount hdmi_power_on/off_core

[Tomi Valkeinen]

On 14/04/17 13:25, Hans Verkuil wrote:
> From: Hans Verkuil 
> 
> 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 
> ---
>  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?

>   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();

I'd like to have the powerdown_disable as a separate patch. Also, now
that you call it here, I believe it can be dropped from hdmi4_configure().

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.

 Tomi



signature.asc
Description: OpenPGP digital signature

[PATCH 6/8] omapdrm: hdmi4: refcount hdmi_power_on/off_core

[Hans Verkuil]

From: Hans Verkuil 

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 
---
 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;
+
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();
+
/* Make selection of HDMI in DSS */
dss_select_hdmi_venc_clk_source(DSS_HDMI_M_PCLK);
 
@@ -141,12 +146,17 @@ static int hdmi_power_on_core(struct omap_dss_device 
*dssdev)
 
 err_runtime_get:
regulator_disable(hdmi.vdda_reg);
+err_reg_enable:
+   hdmi.core.core_pwr_cnt--;
 
return r;
 }
 
 static void hdmi_power_off_core(struct omap_dss_device *dssdev)
 {
+   if (--hdmi.core.core_pwr_cnt)
+   return;
+
hdmi.core_enabled = false;
 
hdmi_runtime_put();
-- 
2.11.0