Re: [RFC][PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Arnd Bergmann]

On Wed, Nov 29, 2017 at 6:05 AM, Archit Taneja  wrote:
> On 11/29/2017 03:02 AM, John Stultz wrote:
>> On Sun, Nov 26, 2017 at 4:56 AM, Archit Taneja  
>> wrote:
>>> Apart from this, we should also move adv7511_cec_init() up in the probe
>>> so that it's called before the drm_bridge is registered.
>>
>>
>> Hans has since reworked the patch w/ a new version. You might want to
>> check his patches and see if they fit what your imagining.
>
>
> Yes, I saw Hans's patch after I wrote to you. That patch looks perfect. I'll
> queue that to the drm-misc repo once it's rebased on 4.15-rc1.

It would be nice to get it merged into -rc2 if possible, as this currently
blocks all of the kselftest regression tracking that we're doing on Hikey,
since the arm64 defconfig fails to boot in both mainline and linux-next,
see https://bugs.linaro.org/show_bug.cgi?id=3345

 Arnd
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Archit Taneja]

On 11/29/2017 03:02 AM, John Stultz wrote:

On Sun, Nov 26, 2017 at 4:56 AM, Archit Taneja  wrote:


On 11/17/2017 04:29 AM, John Stultz wrote:

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 0e14f15..939c3b9 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1203,12 +1203,12 @@ static int adv7511_probe(struct i2c_client *i2c,
const struct i2c_device_id *id)
 #ifdef CONFIG_DRM_I2C_ADV7511_CEC
 ret = adv7511_cec_init(dev, adv7511, offset);
-   if (ret)
-   goto err_unregister_cec;
   #else
-   regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
-ADV7511_CEC_CTRL_POWER_DOWN);
+   ret = 1;
   #endif
+   if (ret)
+   regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL +
offset,
+ADV7511_CEC_CTRL_POWER_DOWN);



This would force CEC to be powered off even if adv7511_cec_init() returned
0, right?


I don't think so. The initent was its only powered off if
adv7511_cec_init returns an error or if  CONFIG_DRM_I2C_ADV7511_CEC is
not set.


Do we know why the call to adv7511_cec_init() is failing on the Hikey board?
If it's
because there isn't a "cec" clock specified in DT, it's not really a fatal
error, it
just means that the platform hasn't been set up to support CEC. In that


Yea. I believe this is the case w/ HiKey.  I don't have deeply
detailed docs on the board so I'm not sure if we will be able to
enable that or not (Xinliang/Guodong: Do you know if its possible?).
In the meantime though, we shouldn't regress.


case, we
should just power down the CEC block. So, if adv7511_cec_init() would return
a
-ENOENT, which we could use as a hint to power down CEC. So, maybe something
like this?:

#ifdef CONFIG_DRM_I2C_ADV7511_CEC
 ret = adv7511_cec_init(dev, adv7511, offset);
 if (ret && ret != -ENOENT)
 goto err_unregister_cec;
#endif
 if (ret)
 regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
 ADV7511_CEC_CTRL_POWER_DOWN);

Apart from this, we should also move adv7511_cec_init() up in the probe so
that
it's called before the drm_bridge is registered.


Hans has since reworked the patch w/ a new version. You might want to
check his patches and see if they fit what your imagining.


Yes, I saw Hans's patch after I wrote to you. That patch looks perfect. I'll
queue that to the drm-misc repo once it's rebased on 4.15-rc1.

Thanks,
Archit



thanks
-john



--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[John Stultz]

On Sun, Nov 26, 2017 at 4:56 AM, Archit Taneja  wrote:
>
> On 11/17/2017 04:29 AM, John Stultz wrote:
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index 0e14f15..939c3b9 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -1203,12 +1203,12 @@ static int adv7511_probe(struct i2c_client *i2c,
>> const struct i2c_device_id *id)
>> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
>> ret = adv7511_cec_init(dev, adv7511, offset);
>> -   if (ret)
>> -   goto err_unregister_cec;
>>   #else
>> -   regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>> -ADV7511_CEC_CTRL_POWER_DOWN);
>> +   ret = 1;
>>   #endif
>> +   if (ret)
>> +   regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL +
>> offset,
>> +ADV7511_CEC_CTRL_POWER_DOWN);
>
>
> This would force CEC to be powered off even if adv7511_cec_init() returned
> 0, right?

I don't think so. The initent was its only powered off if
adv7511_cec_init returns an error or if  CONFIG_DRM_I2C_ADV7511_CEC is
not set.

> Do we know why the call to adv7511_cec_init() is failing on the Hikey board?
> If it's
> because there isn't a "cec" clock specified in DT, it's not really a fatal
> error, it
> just means that the platform hasn't been set up to support CEC. In that

Yea. I believe this is the case w/ HiKey.  I don't have deeply
detailed docs on the board so I'm not sure if we will be able to
enable that or not (Xinliang/Guodong: Do you know if its possible?).
In the meantime though, we shouldn't regress.

> case, we
> should just power down the CEC block. So, if adv7511_cec_init() would return
> a
> -ENOENT, which we could use as a hint to power down CEC. So, maybe something
> like this?:
>
> #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> ret = adv7511_cec_init(dev, adv7511, offset);
> if (ret && ret != -ENOENT)
> goto err_unregister_cec;
> #endif
> if (ret)
> regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
> ADV7511_CEC_CTRL_POWER_DOWN);
>
> Apart from this, we should also move adv7511_cec_init() up in the probe so
> that
> it's called before the drm_bridge is registered.

Hans has since reworked the patch w/ a new version. You might want to
check his patches and see if they fit what your imagining.

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Archit Taneja]

Hi,

On 11/17/2017 04:29 AM, John Stultz wrote:

From: Arnd Bergmann 

An otherwise correct cleanup patch from Dan Carpenter turned a broken
failure handling from a feature patch by Hans Verkuil into a kernel
Oops, so bisection points to commit 7af35b0addbc ("drm/kirin: Checking
for IS_ERR() instead of NULL") rather than 3b1b975003e4 ("drm:
adv7511/33: add HDMI CEC support").

I've managed to piece together several partial problems, though
I'm still struggling with the bigger picture:

adv7511_probe() registers a drm_bridge structure that was allocated
with devm_kzalloc(). It calls adv7511_cec_init(), which fails for an
unknown reason, which in turn triggers the registered structure to be
removed.

Elsewhere, kirin_drm_platform_probe() gets called, which calls
of_graph_get_remote_node(), and that returns NULL. Before Dan's
patch we would go on with a NULL pointer here and register that,
now kirin_drm_platform_probe() fails with -ENODEV.

In a third driver, dsi_parse_dt() calls drm_of_find_panel_or_bridge(),
which after not finding a panel goes on to call of_drm_find_bridge(),
and that crashes due to the earlier list corruption.

This addresses the first issue by making sure that adv7511_probe()
does not completely fail when the adv7511_cec_init() function fails,
and instead we just disable the CEC feature. This avoids having the
driver entirely fail to load if just the CEC initialization fails.

Reported-by: Naresh Kamboju 
Cc: Xinliang Liu 
Cc: Dan Carpenter 
Cc: Sean Paul 
Cc: Hans Verkuil 
Cc: Archit Taneja 
Link: https://bugs.linaro.org/show_bug.cgi?id=3345
Link: https://lkft.validation.linaro.org/scheduler/job/48017#L3551
Fixes: 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL")
Fixes: 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support")
Signed-off-by: Arnd Bergmann 
[jstultz: Reworked so when adv7511_cec_init() fails, we disable the feature 
instead
of disabling the entire driver, which causes graphics to not funciton]
Signed-off-by: John Stultz 
---
Just wanted to send out my rework of Arnd's patch here.
Feedback would be welcome.

thanks
-john

  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 0e14f15..939c3b9 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -1203,12 +1203,12 @@ static int adv7511_probe(struct i2c_client *i2c, const 
struct i2c_device_id *id)
  
  #ifdef CONFIG_DRM_I2C_ADV7511_CEC

ret = adv7511_cec_init(dev, adv7511, offset);
-   if (ret)
-   goto err_unregister_cec;
  #else
-   regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
-ADV7511_CEC_CTRL_POWER_DOWN);
+   ret = 1;
  #endif
+   if (ret)
+   regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
+ADV7511_CEC_CTRL_POWER_DOWN);


This would force CEC to be powered off even if adv7511_cec_init() returned 0, 
right?
We wouldn't want that if we want to use CEC on a platform that supports it.

Do we know why the call to adv7511_cec_init() is failing on the Hikey board? If 
it's
because there isn't a "cec" clock specified in DT, it's not really a fatal 
error, it
just means that the platform hasn't been set up to support CEC. In that case, we
should just power down the CEC block. So, if adv7511_cec_init() would return a
-ENOENT, which we could use as a hint to power down CEC. So, maybe something 
like this?:

#ifdef CONFIG_DRM_I2C_ADV7511_CEC
ret = adv7511_cec_init(dev, adv7511, offset);
if (ret && ret != -ENOENT)  
goto err_unregister_cec;
#endif
if (ret)
regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
ADV7511_CEC_CTRL_POWER_DOWN);

Apart from this, we should also move adv7511_cec_init() up in the probe so that
it's called before the drm_bridge is registered.

Thanks,
Archit

  
  	return 0;
  



--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] [PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[John Stultz]

On Fri, Nov 17, 2017 at 12:43 AM, Hans Verkuil  wrote:
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 3a33075dbb22..56a6a1fa 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -1202,9 +1202,7 @@ static int adv7511_probe(struct i2c_client *i2c, const 
> struct i2c_device_id *id)
> offset = adv7511->type == ADV7533 ? ADV7533_REG_CEC_OFFSET : 0;
>
>  #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> -   ret = adv7511_cec_init(dev, adv7511, offset);
> -   if (ret)
> -   goto err_unregister_cec;
> +   adv7511_cec_init(dev, adv7511, offset);
>  #else
> regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
>  ADV7511_CEC_CTRL_POWER_DOWN);


One tiny nit-pick I realized I should have made in my patch...

In the !CONFIG_DRM_I2C_ADV7511_CEC, can you just define adv7511_cec_init() as
{
regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
ADV7511_CEC_CTRL_POWER_DOWN);
}

Then we can call it either way, and don't need to have the ufly
#ifdefs in the adv7511_probe function?

thanks
-john
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] [PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Hans Verkuil]

On 11/20/2017 04:05 PM, Hans Verkuil wrote:
> On 11/17/2017 09:43 AM, Hans Verkuil wrote:
>> If the device tree for a board did not specify a cec clock, then
>> adv7511_cec_init would return an error, which would cause adv7511_probe()
>> to fail and thus there is no HDMI output.
>>
>> There is no need to have adv7511_probe() fail if the CEC initialization
>> fails, so just change adv7511_cec_init() to a void function. In addition,
>> adv7511_cec_init() should just return silently if the cec clock isn't
>> found and show a message for any other errors.
>>
>> An otherwise correct cleanup patch from Dan Carpenter turned this broken
>> failure handling into a kernel Oops, so bisection points to commit
>> 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL") rather
>> than 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support").
>>
>> Based on earlier patches from Arnd and John.
>>
>> Reported-by: Naresh Kamboju 
>> Cc: Xinliang Liu 
>> Cc: Dan Carpenter 
>> Cc: Sean Paul 
>> Cc: Hans Verkuil 
>> Cc: Archit Taneja 
>> Cc: John Stultz 
>> Link: https://bugs.linaro.org/show_bug.cgi?id=3345
>> Link: https://lkft.validation.linaro.org/scheduler/job/48017#L3551
>> Fixes: 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL")
>> Fixes: 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support")
>> Signed-off-by: Hans Verkuil 
> 
> I tested this patch on a Dragonboard and a Renesas Koelsch board. I also 
> forced
> errors in parsing the dts or registering the CEC adapter to test those failure
> paths on both boards.
> 
> So:
> 
> Tested-by: Hans Verkuil 

As far as I am concerned the [RFC] part can be dropped from this patch.

Regards,

Hans
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC] [PATCH] drm: adv7511/33: Fix adv7511_cec_init() failure handling

[Hans Verkuil]

On 11/17/2017 09:43 AM, Hans Verkuil wrote:
> If the device tree for a board did not specify a cec clock, then
> adv7511_cec_init would return an error, which would cause adv7511_probe()
> to fail and thus there is no HDMI output.
> 
> There is no need to have adv7511_probe() fail if the CEC initialization
> fails, so just change adv7511_cec_init() to a void function. In addition,
> adv7511_cec_init() should just return silently if the cec clock isn't
> found and show a message for any other errors.
> 
> An otherwise correct cleanup patch from Dan Carpenter turned this broken
> failure handling into a kernel Oops, so bisection points to commit
> 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL") rather
> than 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support").
> 
> Based on earlier patches from Arnd and John.
> 
> Reported-by: Naresh Kamboju 
> Cc: Xinliang Liu 
> Cc: Dan Carpenter 
> Cc: Sean Paul 
> Cc: Hans Verkuil 
> Cc: Archit Taneja 
> Cc: John Stultz 
> Link: https://bugs.linaro.org/show_bug.cgi?id=3345
> Link: https://lkft.validation.linaro.org/scheduler/job/48017#L3551
> Fixes: 7af35b0addbc ("drm/kirin: Checking for IS_ERR() instead of NULL")
> Fixes: 3b1b975003e4 ("drm: adv7511/33: add HDMI CEC support")
> Signed-off-by: Hans Verkuil 

I tested this patch on a Dragonboard and a Renesas Koelsch board. I also forced
errors in parsing the dts or registering the CEC adapter to test those failure
paths on both boards.

So:

Tested-by: Hans Verkuil 

Regards,

Hans

> ---
> This rework of Arnd and John's patches goes a bit further and makes
> cec_init a void function and just silently exits if there is no cec clock
> defined in the dts. I'm sure that's the reason why the kirin board failed
> on this. BTW: if the kirin board DOES support cec, then it would be nice
> if it can be hooked up in the dts!
> 
> I'll test this with my two adv7511/33 boards on Monday.
> 
> Regards,
> 
>   Hans
> ---
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511.h 
> b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> index 543a5eb91624..bc17aa965e58 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511.h
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511.h
> @@ -374,8 +374,8 @@ struct adv7511 {
>  };
> 
>  #ifdef CONFIG_DRM_I2C_ADV7511_CEC
> -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511,
> -  unsigned int offset);
> +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511,
> +   unsigned int offset);
>  void adv7511_cec_irq_process(struct adv7511 *adv7511, unsigned int irq1);
>  #endif
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c 
> b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> index b33d730e4d73..c1cd471d31fa 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_cec.c
> @@ -300,18 +300,20 @@ static int adv7511_cec_parse_dt(struct device *dev, 
> struct adv7511 *adv7511)
>   return 0;
>  }
> 
> -int adv7511_cec_init(struct device *dev, struct adv7511 *adv7511,
> -  unsigned int offset)
> +void adv7511_cec_init(struct device *dev, struct adv7511 *adv7511,
> +   unsigned int offset)
>  {
>   int ret = adv7511_cec_parse_dt(dev, adv7511);
> 
>   if (ret)
> - return ret;
> + goto disable_cec;
> 
>   adv7511->cec_adap = cec_allocate_adapter(_cec_adap_ops,
>   adv7511, dev_name(dev), CEC_CAP_DEFAULTS, ADV7511_MAX_ADDRS);
> - if (IS_ERR(adv7511->cec_adap))
> - return PTR_ERR(adv7511->cec_adap);
> + if (IS_ERR(adv7511->cec_adap)) {
> + ret = PTR_ERR(adv7511->cec_adap);
> + goto fail;
> + }
> 
>   regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset, 0);
>   /* cec soft reset */
> @@ -329,9 +331,15 @@ int adv7511_cec_init(struct device *dev, struct adv7511 
> *adv7511,
>((adv7511->cec_clk_freq / 75) - 1) << 2);
> 
>   ret = cec_register_adapter(adv7511->cec_adap, dev);
> - if (ret) {
> - cec_delete_adapter(adv7511->cec_adap);
> - adv7511->cec_adap = NULL;
> - }
> - return ret;
> + if (!ret)
> + return;
> + cec_delete_adapter(adv7511->cec_adap);
> + adv7511->cec_adap = NULL;
> +
> +fail:
> + dev_info(dev, "Initializing CEC failed with error %d, disabling CEC\n",
> +  ret);
> +disable_cec:
> + regmap_write(adv7511->regmap, ADV7511_REG_CEC_CTRL + offset,
> +  ADV7511_CEC_CTRL_POWER_DOWN);
>  }
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c 
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 3a33075dbb22..56a6a1fa 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>