[RFC][PATCH 0/3] adv7511 EDID probing improvements

[RFC][PATCH 0/3] adv7511 EDID probing improvements

[John Stultz]

I had been seeing some EDID probing issues with the adv7511 driver
on HiKey recently. After talking with Archit and spending some time
reading the programming guide, I put together the following patch
set which seems to resolve the EDID probing issues.

I wanted to send these out for some early review and feedback

thanks
-john

Cc: David Airlie <airlied@linux.ie>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org

Archit Taneja (1):
  drm/bridge: adv7511: Enable HPD interrupts to support hotplug and
    improve monitor detection

John Stultz (2):
  drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be
    reused internally
  drm/bridge: adv7511: Add 200ms delay on power-on

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

-- 
2.7.4

[RFC][PATCH 0/3] adv7511 EDID probing improvements

[John Stultz]

I had been seeing some EDID probing issues with the adv7511 driver
on HiKey recently. After talking with Archit and spending some time
reading the programming guide, I put together the following patch
set which seems to resolve the EDID probing issues.

I wanted to send these out for some early review and feedback

thanks
-john

Cc: David Airlie <airlied@linux.ie>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org

Archit Taneja (1):
  drm/bridge: adv7511: Enable HPD interrupts to support hotplug and
    improve monitor detection

John Stultz (2):
  drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be
    reused internally
  drm/bridge: adv7511: Add 200ms delay on power-on

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

-- 
2.7.4

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

[RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

In chasing down issues with EDID probing, I found some
duplicated but incomplete logic used to power the chip on and
off.

This patch refactors the adv7511_power_on/off functions, so
they can be used for internal needs, and replaces duplicative
logic that powers the chip on and off around the EDID probing
with the common logic.

Cc: David Airlie <airlied@linux.ie>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 30 +++++++++++++---------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 8dba729..b240e05 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511 *adv7511,
 	adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
 }
 
-static void adv7511_power_on(struct adv7511 *adv7511)
+static void __adv7511_power_on(struct adv7511 *adv7511)
 {
 	adv7511->current_edid_segment = -1;
 
@@ -343,6 +343,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 			     ADV7511_INT1_DDC_ERROR);
 	}
 
+
 	/*
 	 * Per spec it is allowed to pulse the HPD signal to indicate that the
 	 * EDID information has changed. Some monitors do this when they wakeup
@@ -362,11 +363,15 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 
 	if (adv7511->type == ADV7533)
 		adv7533_dsi_power_on(adv7511);
+}
 
+static void adv7511_power_on(struct adv7511 *adv7511)
+{
+	__adv7511_power_on(adv7511);
 	adv7511->powered = true;
 }
 
-static void adv7511_power_off(struct adv7511 *adv7511)
+static void __adv7511_power_off(struct adv7511 *adv7511)
 {
 	/* TODO: setup additional power down modes */
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
@@ -376,7 +381,11 @@ static void adv7511_power_off(struct adv7511 *adv7511)
 
 	if (adv7511->type == ADV7533)
 		adv7533_dsi_power_off(adv7511);
+}
 
+static void adv7511_power_off(struct adv7511 *adv7511)
+{
+	__adv7511_power_off(adv7511);
 	adv7511->powered = false;
 }
 
@@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
 	unsigned int count;
 
 	/* Reading the EDID only works if the device is powered */
-	if (!adv7511->powered) {
-		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
-				   ADV7511_POWER_POWER_DOWN, 0);
-		if (adv7511->i2c_main->irq) {
-			regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
-				     ADV7511_INT0_EDID_READY);
-			regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
-				     ADV7511_INT1_DDC_ERROR);
-		}
-		adv7511->current_edid_segment = -1;
-	}
+	if (!adv7511->powered)
+		__adv7511_power_on(adv7511);
 
 	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
 
 	if (!adv7511->powered)
-		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
-				   ADV7511_POWER_POWER_DOWN,
-				   ADV7511_POWER_POWER_DOWN);
+		__adv7511_power_off(adv7511);
 
 	kfree(adv7511->edid);
 	adv7511->edid = edid;
-- 
2.7.4

[RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

In chasing down issues with EDID probing, I found some
duplicated but incomplete logic used to power the chip on and
off.

This patch refactors the adv7511_power_on/off functions, so
they can be used for internal needs, and replaces duplicative
logic that powers the chip on and off around the EDID probing
with the common logic.

Cc: David Airlie <airlied@linux.ie>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 30 +++++++++++++---------------
 1 file changed, 14 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 8dba729..b240e05 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511 *adv7511,
 	adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
 }
 
-static void adv7511_power_on(struct adv7511 *adv7511)
+static void __adv7511_power_on(struct adv7511 *adv7511)
 {
 	adv7511->current_edid_segment = -1;
 
@@ -343,6 +343,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 			     ADV7511_INT1_DDC_ERROR);
 	}
 
+
 	/*
 	 * Per spec it is allowed to pulse the HPD signal to indicate that the
 	 * EDID information has changed. Some monitors do this when they wakeup
@@ -362,11 +363,15 @@ static void adv7511_power_on(struct adv7511 *adv7511)
 
 	if (adv7511->type == ADV7533)
 		adv7533_dsi_power_on(adv7511);
+}
 
+static void adv7511_power_on(struct adv7511 *adv7511)
+{
+	__adv7511_power_on(adv7511);
 	adv7511->powered = true;
 }
 
-static void adv7511_power_off(struct adv7511 *adv7511)
+static void __adv7511_power_off(struct adv7511 *adv7511)
 {
 	/* TODO: setup additional power down modes */
 	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
@@ -376,7 +381,11 @@ static void adv7511_power_off(struct adv7511 *adv7511)
 
 	if (adv7511->type == ADV7533)
 		adv7533_dsi_power_off(adv7511);
+}
 
+static void adv7511_power_off(struct adv7511 *adv7511)
+{
+	__adv7511_power_off(adv7511);
 	adv7511->powered = false;
 }
 
@@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
 	unsigned int count;
 
 	/* Reading the EDID only works if the device is powered */
-	if (!adv7511->powered) {
-		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
-				   ADV7511_POWER_POWER_DOWN, 0);
-		if (adv7511->i2c_main->irq) {
-			regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
-				     ADV7511_INT0_EDID_READY);
-			regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
-				     ADV7511_INT1_DDC_ERROR);
-		}
-		adv7511->current_edid_segment = -1;
-	}
+	if (!adv7511->powered)
+		__adv7511_power_on(adv7511);
 
 	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
 
 	if (!adv7511->powered)
-		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
-				   ADV7511_POWER_POWER_DOWN,
-				   ADV7511_POWER_POWER_DOWN);
+		__adv7511_power_off(adv7511);
 
 	kfree(adv7511->edid);
 	adv7511->edid = edid;
-- 
2.7.4

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

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[Laurent Pinchart]

Hi John,

Thank you for the patch.

On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
> In chasing down issues with EDID probing, I found some
> duplicated but incomplete logic used to power the chip on and
> off.
> 
> This patch refactors the adv7511_power_on/off functions, so
> they can be used for internal needs, and replaces duplicative
> logic that powers the chip on and off around the EDID probing
> with the common logic.
> 
> Cc: David Airlie <airlied@linux.ie>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 30 +++++++++++--------------
>  1 file changed, 14 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b240e05
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511
> *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
>  }
> 
> -static void adv7511_power_on(struct adv7511 *adv7511)
> +static void __adv7511_power_on(struct adv7511 *adv7511)
>  {
>  	adv7511->current_edid_segment = -1;
> 
> @@ -343,6 +343,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
>  			     ADV7511_INT1_DDC_ERROR);
>  	}
> 
> +

This isn't needed.

>  	/*
>  	 * Per spec it is allowed to pulse the HPD signal to indicate that the
>  	 * EDID information has changed. Some monitors do this when they 
wakeup
> @@ -362,11 +363,15 @@ static void adv7511_power_on(struct adv7511 *adv7511)
> 
>  	if (adv7511->type == ADV7533)
>  		adv7533_dsi_power_on(adv7511);
> +}
> 
> +static void adv7511_power_on(struct adv7511 *adv7511)
> +{
> +	__adv7511_power_on(adv7511);
>  	adv7511->powered = true;
>  }
> 
> -static void adv7511_power_off(struct adv7511 *adv7511)
> +static void __adv7511_power_off(struct adv7511 *adv7511)
>  {
>  	/* TODO: setup additional power down modes */
>  	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> @@ -376,7 +381,11 @@ static void adv7511_power_off(struct adv7511 *adv7511)
> 
>  	if (adv7511->type == ADV7533)
>  		adv7533_dsi_power_off(adv7511);
> +}
> 
> +static void adv7511_power_off(struct adv7511 *adv7511)
> +{
> +	__adv7511_power_off(adv7511);
>  	adv7511->powered = false;
>  }
> 
> @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>  	unsigned int count;
> 
>  	/* Reading the EDID only works if the device is powered */
> -	if (!adv7511->powered) {
> -		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> -				   ADV7511_POWER_POWER_DOWN, 0);
> -		if (adv7511->i2c_main->irq) {
> -			regmap_write(adv7511->regmap, 
ADV7511_REG_INT_ENABLE(0),
> -				     ADV7511_INT0_EDID_READY);
> -			regmap_write(adv7511->regmap, 
ADV7511_REG_INT_ENABLE(1),
> -				     ADV7511_INT1_DDC_ERROR);
> -		}
> -		adv7511->current_edid_segment = -1;
> -	}
> +	if (!adv7511->powered)
> +		__adv7511_power_on(adv7511);

The __adv7511_power_on() function does more than the above, in particular it 
performs an expensive regcache_sync() and calls adv7533_dsi_power_on() for the 
ADV7533. Don't those operations have side effects that are either not wanted 
or not needed here ? In any case this patch modifies the behaviour of the 
driver, which needs to be documented in the kernel message.

>  	edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
> 
>  	if (!adv7511->powered)
> -		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> -				   ADV7511_POWER_POWER_DOWN,
> -				   ADV7511_POWER_POWER_DOWN);
> +		__adv7511_power_off(adv7511);
> 
>  	kfree(adv7511->edid);
>  	adv7511->edid = edid;

-- 
Regards,

Laurent Pinchart

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi John,
>
> Thank you for the patch.
>
> On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
>> In chasing down issues with EDID probing, I found some
>> duplicated but incomplete logic used to power the chip on and
>> off.
>>
>> This patch refactors the adv7511_power_on/off functions, so
>> they can be used for internal needs, and replaces duplicative
>> logic that powers the chip on and off around the EDID probing
>> with the common logic.
>>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 30 +++++++++++--------------
>>  1 file changed, 14 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b240e05
>> 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511
>> *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
>>  }
>>
>> -static void adv7511_power_on(struct adv7511 *adv7511)
>> +static void __adv7511_power_on(struct adv7511 *adv7511)
>>  {
>>       adv7511->current_edid_segment = -1;
>>
>> @@ -343,6 +343,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
>>                            ADV7511_INT1_DDC_ERROR);
>>       }
>>
>> +
>
> This isn't needed.

Apologies. I saw this right after I sent it!

>>       /*
>>        * Per spec it is allowed to pulse the HPD signal to indicate that the
>>        * EDID information has changed. Some monitors do this when they
> wakeup
>> @@ -362,11 +363,15 @@ static void adv7511_power_on(struct adv7511 *adv7511)
>>
>>       if (adv7511->type == ADV7533)
>>               adv7533_dsi_power_on(adv7511);
>> +}
>>
>> +static void adv7511_power_on(struct adv7511 *adv7511)
>> +{
>> +     __adv7511_power_on(adv7511);
>>       adv7511->powered = true;
>>  }
>>
>> -static void adv7511_power_off(struct adv7511 *adv7511)
>> +static void __adv7511_power_off(struct adv7511 *adv7511)
>>  {
>>       /* TODO: setup additional power down modes */
>>       regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>> @@ -376,7 +381,11 @@ static void adv7511_power_off(struct adv7511 *adv7511)
>>
>>       if (adv7511->type == ADV7533)
>>               adv7533_dsi_power_off(adv7511);
>> +}
>>
>> +static void adv7511_power_off(struct adv7511 *adv7511)
>> +{
>> +     __adv7511_power_off(adv7511);
>>       adv7511->powered = false;
>>  }
>>
>> @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>>       unsigned int count;
>>
>>       /* Reading the EDID only works if the device is powered */
>> -     if (!adv7511->powered) {
>> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>> -                                ADV7511_POWER_POWER_DOWN, 0);
>> -             if (adv7511->i2c_main->irq) {
>> -                     regmap_write(adv7511->regmap,
> ADV7511_REG_INT_ENABLE(0),
>> -                                  ADV7511_INT0_EDID_READY);
>> -                     regmap_write(adv7511->regmap,
> ADV7511_REG_INT_ENABLE(1),
>> -                                  ADV7511_INT1_DDC_ERROR);
>> -             }
>> -             adv7511->current_edid_segment = -1;
>> -     }
>> +     if (!adv7511->powered)
>> +             __adv7511_power_on(adv7511);
>
> The __adv7511_power_on() function does more than the above, in particular it
> performs an expensive regcache_sync() and calls adv7533_dsi_power_on() for the
> ADV7533. Don't those operations have side effects that are either not wanted
> or not needed here ? In any case this patch modifies the behaviour of the
> driver, which needs to be documented in the kernel message.

Sorry, what do you mean by kernel message? Commit message, maybe?

Fair point, I'll review the adv7533_dsi_power_on bits and see if they
should move out to the external function rather then the internal one.
Similarly for the regcache_sync.

Thanks so much for the review!

thanks
-john

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi John,
>
> Thank you for the patch.
>
> On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
>> In chasing down issues with EDID probing, I found some
>> duplicated but incomplete logic used to power the chip on and
>> off.
>>
>> This patch refactors the adv7511_power_on/off functions, so
>> they can be used for internal needs, and replaces duplicative
>> logic that powers the chip on and off around the EDID probing
>> with the common logic.
>>
>> Cc: David Airlie <airlied@linux.ie>
>> Cc: Archit Taneja <architt@codeaurora.org>
>> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
>> Cc: Lars-Peter Clausen <lars@metafoo.de>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Cc: dri-devel@lists.freedesktop.org
>> Signed-off-by: John Stultz <john.stultz@linaro.org>
>> ---
>>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 30 +++++++++++--------------
>>  1 file changed, 14 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b240e05
>> 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511
>> *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
>>  }
>>
>> -static void adv7511_power_on(struct adv7511 *adv7511)
>> +static void __adv7511_power_on(struct adv7511 *adv7511)
>>  {
>>       adv7511->current_edid_segment = -1;
>>
>> @@ -343,6 +343,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
>>                            ADV7511_INT1_DDC_ERROR);
>>       }
>>
>> +
>
> This isn't needed.

Apologies. I saw this right after I sent it!

>>       /*
>>        * Per spec it is allowed to pulse the HPD signal to indicate that the
>>        * EDID information has changed. Some monitors do this when they
> wakeup
>> @@ -362,11 +363,15 @@ static void adv7511_power_on(struct adv7511 *adv7511)
>>
>>       if (adv7511->type == ADV7533)
>>               adv7533_dsi_power_on(adv7511);
>> +}
>>
>> +static void adv7511_power_on(struct adv7511 *adv7511)
>> +{
>> +     __adv7511_power_on(adv7511);
>>       adv7511->powered = true;
>>  }
>>
>> -static void adv7511_power_off(struct adv7511 *adv7511)
>> +static void __adv7511_power_off(struct adv7511 *adv7511)
>>  {
>>       /* TODO: setup additional power down modes */
>>       regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>> @@ -376,7 +381,11 @@ static void adv7511_power_off(struct adv7511 *adv7511)
>>
>>       if (adv7511->type == ADV7533)
>>               adv7533_dsi_power_off(adv7511);
>> +}
>>
>> +static void adv7511_power_off(struct adv7511 *adv7511)
>> +{
>> +     __adv7511_power_off(adv7511);
>>       adv7511->powered = false;
>>  }
>>
>> @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>>       unsigned int count;
>>
>>       /* Reading the EDID only works if the device is powered */
>> -     if (!adv7511->powered) {
>> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>> -                                ADV7511_POWER_POWER_DOWN, 0);
>> -             if (adv7511->i2c_main->irq) {
>> -                     regmap_write(adv7511->regmap,
> ADV7511_REG_INT_ENABLE(0),
>> -                                  ADV7511_INT0_EDID_READY);
>> -                     regmap_write(adv7511->regmap,
> ADV7511_REG_INT_ENABLE(1),
>> -                                  ADV7511_INT1_DDC_ERROR);
>> -             }
>> -             adv7511->current_edid_segment = -1;
>> -     }
>> +     if (!adv7511->powered)
>> +             __adv7511_power_on(adv7511);
>
> The __adv7511_power_on() function does more than the above, in particular it
> performs an expensive regcache_sync() and calls adv7533_dsi_power_on() for the
> ADV7533. Don't those operations have side effects that are either not wanted
> or not needed here ? In any case this patch modifies the behaviour of the
> driver, which needs to be documented in the kernel message.

Sorry, what do you mean by kernel message? Commit message, maybe?

Fair point, I'll review the adv7533_dsi_power_on bits and see if they
should move out to the external function rather then the internal one.
Similarly for the regcache_sync.

Thanks so much for the review!

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

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[Laurent Pinchart]

Hi John,

On Tuesday 22 Nov 2016 00:16:55 John Stultz wrote:
> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote:
> > On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
> >> In chasing down issues with EDID probing, I found some
> >> duplicated but incomplete logic used to power the chip on and
> >> off.
> >> 
> >> This patch refactors the adv7511_power_on/off functions, so
> >> they can be used for internal needs, and replaces duplicative
> >> logic that powers the chip on and off around the EDID probing
> >> with the common logic.
> >> 
> >> Cc: David Airlie <airlied@linux.ie>
> >> Cc: Archit Taneja <architt@codeaurora.org>
> >> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >> Cc: Lars-Peter Clausen <lars@metafoo.de>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Signed-off-by: John Stultz <john.stultz@linaro.org>
> >> ---
> >> 
> >>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 30 +++++++++-------------
> >>  1 file changed, 14 insertions(+), 16 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b240e05
> >> 100644
> >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> @@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511
> >> *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
> >> 
> >>  }
> >> 
> >> -static void adv7511_power_on(struct adv7511 *adv7511)
> >> +static void __adv7511_power_on(struct adv7511 *adv7511)
> >>  {
> >>       adv7511->current_edid_segment = -1;
> >> 
> >> @@ -343,6 +343,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
> >>                            ADV7511_INT1_DDC_ERROR);
> >>       }
> >> 
> >> +
> > 
> > This isn't needed.
> 
> Apologies. I saw this right after I sent it!
> 
> >>       /*
> >>        * Per spec it is allowed to pulse the HPD signal to indicate that
> >>        the
> >>        * EDID information has changed. Some monitors do this when they
> >> wakeup
> >> @@ -362,11 +363,15 @@ static void adv7511_power_on(struct adv7511
> >> *adv7511)
> >> 
> >>       if (adv7511->type == ADV7533)
> >>               adv7533_dsi_power_on(adv7511);
> >> +}
> >> 
> >> +static void adv7511_power_on(struct adv7511 *adv7511)
> >> +{
> >> +     __adv7511_power_on(adv7511);
> >>       adv7511->powered = true;
> >>  }
> >> 
> >> -static void adv7511_power_off(struct adv7511 *adv7511)
> >> +static void __adv7511_power_off(struct adv7511 *adv7511)
> >>  {
> >>       /* TODO: setup additional power down modes */
> >>       regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> >> @@ -376,7 +381,11 @@ static void adv7511_power_off(struct adv7511
> >> *adv7511)
> >> 
> >>       if (adv7511->type == ADV7533)
> >>               adv7533_dsi_power_off(adv7511);
> >> +}
> >> 
> >> +static void adv7511_power_off(struct adv7511 *adv7511)
> >> +{
> >> +     __adv7511_power_off(adv7511);
> >>       adv7511->powered = false;
> >>  }
> >> 
> >> @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511
> >> *adv7511,
> >>       unsigned int count;
> >>       
> >>       /* Reading the EDID only works if the device is powered */
> >> -     if (!adv7511->powered) {
> >> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> >> -                                ADV7511_POWER_POWER_DOWN, 0);
> >> -             if (adv7511->i2c_main->irq) {
> >> -                     regmap_write(adv7511->regmap,
> > 
> > ADV7511_REG_INT_ENABLE(0),
> > 
> >> -                                  ADV7511_INT0_EDID_READY);
> >> -                     regmap_write(adv7511->regmap,
> >> ADV7511_REG_INT_ENABLE(1),
> >> -                                  ADV7511_INT1_DDC_ERROR);
> >> -             }
> >> -             adv7511->current_edid_segment = -1;
> >> -     }
> >> +     if (!adv7511->powered)
> >> +             __adv7511_power_on(adv7511);
> > 
> > The __adv7511_power_on() function does more than the above, in particular
> > it performs an expensive regcache_sync() and calls adv7533_dsi_power_on()
> > for the ADV7533. Don't those operations have side effects that are either
> > not wanted or not needed here ? In any case this patch modifies the
> > behaviour of the driver, which needs to be documented in the kernel
> > message.
> 
> Sorry, what do you mean by kernel message? Commit message, maybe?

Sorry, yes, I meant commit message.

> Fair point, I'll review the adv7533_dsi_power_on bits and see if they
> should move out to the external function rather then the internal one.
> Similarly for the regcache_sync.
> 
> Thanks so much for the review!

-- 
Regards,

Laurent Pinchart

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[Laurent Pinchart]

Hi John,

On Tuesday 22 Nov 2016 00:16:55 John Stultz wrote:
> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote:
> > On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
> >> In chasing down issues with EDID probing, I found some
> >> duplicated but incomplete logic used to power the chip on and
> >> off.
> >> 
> >> This patch refactors the adv7511_power_on/off functions, so
> >> they can be used for internal needs, and replaces duplicative
> >> logic that powers the chip on and off around the EDID probing
> >> with the common logic.
> >> 
> >> Cc: David Airlie <airlied@linux.ie>
> >> Cc: Archit Taneja <architt@codeaurora.org>
> >> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> >> Cc: Lars-Peter Clausen <lars@metafoo.de>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Cc: dri-devel@lists.freedesktop.org
> >> Signed-off-by: John Stultz <john.stultz@linaro.org>
> >> ---
> >> 
> >>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 30 +++++++++-------------
> >>  1 file changed, 14 insertions(+), 16 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 8dba729..b240e05
> >> 100644
> >> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> >> @@ -325,7 +325,7 @@ static void adv7511_set_link_config(struct adv7511
> >> *adv7511, adv7511->rgb = config->input_colorspace == HDMI_COLORSPACE_RGB;
> >> 
> >>  }
> >> 
> >> -static void adv7511_power_on(struct adv7511 *adv7511)
> >> +static void __adv7511_power_on(struct adv7511 *adv7511)
> >>  {
> >>       adv7511->current_edid_segment = -1;
> >> 
> >> @@ -343,6 +343,7 @@ static void adv7511_power_on(struct adv7511 *adv7511)
> >>                            ADV7511_INT1_DDC_ERROR);
> >>       }
> >> 
> >> +
> > 
> > This isn't needed.
> 
> Apologies. I saw this right after I sent it!
> 
> >>       /*
> >>        * Per spec it is allowed to pulse the HPD signal to indicate that
> >>        the
> >>        * EDID information has changed. Some monitors do this when they
> >> wakeup
> >> @@ -362,11 +363,15 @@ static void adv7511_power_on(struct adv7511
> >> *adv7511)
> >> 
> >>       if (adv7511->type == ADV7533)
> >>               adv7533_dsi_power_on(adv7511);
> >> +}
> >> 
> >> +static void adv7511_power_on(struct adv7511 *adv7511)
> >> +{
> >> +     __adv7511_power_on(adv7511);
> >>       adv7511->powered = true;
> >>  }
> >> 
> >> -static void adv7511_power_off(struct adv7511 *adv7511)
> >> +static void __adv7511_power_off(struct adv7511 *adv7511)
> >>  {
> >>       /* TODO: setup additional power down modes */
> >>       regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> >> @@ -376,7 +381,11 @@ static void adv7511_power_off(struct adv7511
> >> *adv7511)
> >> 
> >>       if (adv7511->type == ADV7533)
> >>               adv7533_dsi_power_off(adv7511);
> >> +}
> >> 
> >> +static void adv7511_power_off(struct adv7511 *adv7511)
> >> +{
> >> +     __adv7511_power_off(adv7511);
> >>       adv7511->powered = false;
> >>  }
> >> 
> >> @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511
> >> *adv7511,
> >>       unsigned int count;
> >>       
> >>       /* Reading the EDID only works if the device is powered */
> >> -     if (!adv7511->powered) {
> >> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> >> -                                ADV7511_POWER_POWER_DOWN, 0);
> >> -             if (adv7511->i2c_main->irq) {
> >> -                     regmap_write(adv7511->regmap,
> > 
> > ADV7511_REG_INT_ENABLE(0),
> > 
> >> -                                  ADV7511_INT0_EDID_READY);
> >> -                     regmap_write(adv7511->regmap,
> >> ADV7511_REG_INT_ENABLE(1),
> >> -                                  ADV7511_INT1_DDC_ERROR);
> >> -             }
> >> -             adv7511->current_edid_segment = -1;
> >> -     }
> >> +     if (!adv7511->powered)
> >> +             __adv7511_power_on(adv7511);
> > 
> > The __adv7511_power_on() function does more than the above, in particular
> > it performs an expensive regcache_sync() and calls adv7533_dsi_power_on()
> > for the ADV7533. Don't those operations have side effects that are either
> > not wanted or not needed here ? In any case this patch modifies the
> > behaviour of the driver, which needs to be documented in the kernel
> > message.
> 
> Sorry, what do you mean by kernel message? Commit message, maybe?

Sorry, yes, I meant commit message.

> Fair point, I'll review the adv7533_dsi_power_on bits and see if they
> should move out to the external function rather then the internal one.
> Similarly for the regcache_sync.
> 
> Thanks so much for the review!

-- 
Regards,

Laurent Pinchart

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

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
 @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>>       unsigned int count;
>>
>>       /* Reading the EDID only works if the device is powered */
>> -     if (!adv7511->powered) {
>> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>> -                                ADV7511_POWER_POWER_DOWN, 0);
>> -             if (adv7511->i2c_main->irq) {
>> -                     regmap_write(adv7511->regmap,
> ADV7511_REG_INT_ENABLE(0),
>> -                                  ADV7511_INT0_EDID_READY);
>> -                     regmap_write(adv7511->regmap,
> ADV7511_REG_INT_ENABLE(1),
>> -                                  ADV7511_INT1_DDC_ERROR);
>> -             }
>> -             adv7511->current_edid_segment = -1;
>> -     }
>> +     if (!adv7511->powered)
>> +             __adv7511_power_on(adv7511);
>
> The __adv7511_power_on() function does more than the above, in particular it
> performs an expensive regcache_sync() and calls adv7533_dsi_power_on() for the
> ADV7533. Don't those operations have side effects that are either not wanted
> or not needed here ? In any case this patch modifies the behaviour of the
> driver, which needs to be documented in the kernel message.

So yes, while the adv7533 bits aren't needed in the internal function,
I'm finding the logic to pulse the HPD and the regcache_sync call seem
to be needed side effects, as without that logic, I get i2c_transfer()
errors in adv7511_get_edid_block().

I'll try to rework this patch to split the two changes of reworking
the power_on/off function to be re-used (with no logic chage), and the
patch to reuse it in get_modes() which resolves a bug.

Thanks so much for the review!
-john

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
 @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>>       unsigned int count;
>>
>>       /* Reading the EDID only works if the device is powered */
>> -     if (!adv7511->powered) {
>> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>> -                                ADV7511_POWER_POWER_DOWN, 0);
>> -             if (adv7511->i2c_main->irq) {
>> -                     regmap_write(adv7511->regmap,
> ADV7511_REG_INT_ENABLE(0),
>> -                                  ADV7511_INT0_EDID_READY);
>> -                     regmap_write(adv7511->regmap,
> ADV7511_REG_INT_ENABLE(1),
>> -                                  ADV7511_INT1_DDC_ERROR);
>> -             }
>> -             adv7511->current_edid_segment = -1;
>> -     }
>> +     if (!adv7511->powered)
>> +             __adv7511_power_on(adv7511);
>
> The __adv7511_power_on() function does more than the above, in particular it
> performs an expensive regcache_sync() and calls adv7533_dsi_power_on() for the
> ADV7533. Don't those operations have side effects that are either not wanted
> or not needed here ? In any case this patch modifies the behaviour of the
> driver, which needs to be documented in the kernel message.

So yes, while the adv7533 bits aren't needed in the internal function,
I'm finding the logic to pulse the HPD and the regcache_sync call seem
to be needed side effects, as without that logic, I get i2c_transfer()
errors in adv7511_get_edid_block().

I'll try to rework this patch to split the two changes of reworking
the power_on/off function to be re-used (with no logic chage), and the
patch to reuse it in get_modes() which resolves a bug.

Thanks so much for the review!
-john
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[Laurent Pinchart]

Hi John,

On Tuesday 22 Nov 2016 09:25:22 John Stultz wrote:
> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote:
> > On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
>  @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
> >>       unsigned int count;
> >>       
> >>       /* Reading the EDID only works if the device is powered */
> >> 
> >> -     if (!adv7511->powered) {
> >> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> >> -                                ADV7511_POWER_POWER_DOWN, 0);
> >> -             if (adv7511->i2c_main->irq) {
> >> -                     regmap_write(adv7511->regmap,
> >> ADV7511_REG_INT_ENABLE(0),
> >> -                                  ADV7511_INT0_EDID_READY);
> >> -                     regmap_write(adv7511->regmap,
> >> ADV7511_REG_INT_ENABLE(1),
> >> -                                  ADV7511_INT1_DDC_ERROR);
> >> -             }
> >> -             adv7511->current_edid_segment = -1;
> >> -     }
> >> +     if (!adv7511->powered)
> >> +             __adv7511_power_on(adv7511);
> > 
> > The __adv7511_power_on() function does more than the above, in particular
> > it performs an expensive regcache_sync() and calls adv7533_dsi_power_on()
> > for the ADV7533. Don't those operations have side effects that are either
> > not wanted or not needed here ? In any case this patch modifies the
> > behaviour of the driver, which needs to be documented in the kernel
> > message.
> 
> So yes, while the adv7533 bits aren't needed in the internal function,
> I'm finding the logic to pulse the HPD and the regcache_sync call seem
> to be needed side effects, as without that logic, I get i2c_transfer()
> errors in adv7511_get_edid_block().

Does this patch fix the problem without requiring the 200ms delay ?

> I'll try to rework this patch to split the two changes of reworking
> the power_on/off function to be re-used (with no logic chage), and the
> patch to reuse it in get_modes() which resolves a bug.

Have you identified which register write fixes your problem here ?

> Thanks so much for the review!

-- 
Regards,

Laurent Pinchart

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[Laurent Pinchart]

Hi John,

On Tuesday 22 Nov 2016 09:25:22 John Stultz wrote:
> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote:
> > On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
>  @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
> >>       unsigned int count;
> >>       
> >>       /* Reading the EDID only works if the device is powered */
> >> 
> >> -     if (!adv7511->powered) {
> >> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
> >> -                                ADV7511_POWER_POWER_DOWN, 0);
> >> -             if (adv7511->i2c_main->irq) {
> >> -                     regmap_write(adv7511->regmap,
> >> ADV7511_REG_INT_ENABLE(0),
> >> -                                  ADV7511_INT0_EDID_READY);
> >> -                     regmap_write(adv7511->regmap,
> >> ADV7511_REG_INT_ENABLE(1),
> >> -                                  ADV7511_INT1_DDC_ERROR);
> >> -             }
> >> -             adv7511->current_edid_segment = -1;
> >> -     }
> >> +     if (!adv7511->powered)
> >> +             __adv7511_power_on(adv7511);
> > 
> > The __adv7511_power_on() function does more than the above, in particular
> > it performs an expensive regcache_sync() and calls adv7533_dsi_power_on()
> > for the ADV7533. Don't those operations have side effects that are either
> > not wanted or not needed here ? In any case this patch modifies the
> > behaviour of the driver, which needs to be documented in the kernel
> > message.
> 
> So yes, while the adv7533 bits aren't needed in the internal function,
> I'm finding the logic to pulse the HPD and the regcache_sync call seem
> to be needed side effects, as without that logic, I get i2c_transfer()
> errors in adv7511_get_edid_block().

Does this patch fix the problem without requiring the 200ms delay ?

> I'll try to rework this patch to split the two changes of reworking
> the power_on/off function to be re-used (with no logic chage), and the
> patch to reuse it in get_modes() which resolves a bug.

Have you identified which register write fixes your problem here ?

> Thanks so much for the review!

-- 
Regards,

Laurent Pinchart

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

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi John,
>
> On Tuesday 22 Nov 2016 09:25:22 John Stultz wrote:
>> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote:
>> > On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
>>  @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>> >>       unsigned int count;
>> >>
>> >>       /* Reading the EDID only works if the device is powered */
>> >>
>> >> -     if (!adv7511->powered) {
>> >> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>> >> -                                ADV7511_POWER_POWER_DOWN, 0);
>> >> -             if (adv7511->i2c_main->irq) {
>> >> -                     regmap_write(adv7511->regmap,
>> >> ADV7511_REG_INT_ENABLE(0),
>> >> -                                  ADV7511_INT0_EDID_READY);
>> >> -                     regmap_write(adv7511->regmap,
>> >> ADV7511_REG_INT_ENABLE(1),
>> >> -                                  ADV7511_INT1_DDC_ERROR);
>> >> -             }
>> >> -             adv7511->current_edid_segment = -1;
>> >> -     }
>> >> +     if (!adv7511->powered)
>> >> +             __adv7511_power_on(adv7511);
>> >
>> > The __adv7511_power_on() function does more than the above, in particular
>> > it performs an expensive regcache_sync() and calls adv7533_dsi_power_on()
>> > for the ADV7533. Don't those operations have side effects that are either
>> > not wanted or not needed here ? In any case this patch modifies the
>> > behaviour of the driver, which needs to be documented in the kernel
>> > message.
>>
>> So yes, while the adv7533 bits aren't needed in the internal function,
>> I'm finding the logic to pulse the HPD and the regcache_sync call seem
>> to be needed side effects, as without that logic, I get i2c_transfer()
>> errors in adv7511_get_edid_block().
>
> Does this patch fix the problem without requiring the 200ms delay ?

Partially, but not completely as I just explained in the other mail thread.

>> I'll try to rework this patch to split the two changes of reworking
>> the power_on/off function to be re-used (with no logic chage), and the
>> patch to reuse it in get_modes() which resolves a bug.
>
> Have you identified which register write fixes your problem here ?

So I initially thought it was the HPD pulse, but I'm finding that
without the regmap_sync I still see the i2c_transfer errors.

I need to figure out how to decompose the regmap_sync to try to narrow
it down further.

thanks
-john

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi John,
>
> On Tuesday 22 Nov 2016 09:25:22 John Stultz wrote:
>> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote:
>> > On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
>>  @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>> >>       unsigned int count;
>> >>
>> >>       /* Reading the EDID only works if the device is powered */
>> >>
>> >> -     if (!adv7511->powered) {
>> >> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>> >> -                                ADV7511_POWER_POWER_DOWN, 0);
>> >> -             if (adv7511->i2c_main->irq) {
>> >> -                     regmap_write(adv7511->regmap,
>> >> ADV7511_REG_INT_ENABLE(0),
>> >> -                                  ADV7511_INT0_EDID_READY);
>> >> -                     regmap_write(adv7511->regmap,
>> >> ADV7511_REG_INT_ENABLE(1),
>> >> -                                  ADV7511_INT1_DDC_ERROR);
>> >> -             }
>> >> -             adv7511->current_edid_segment = -1;
>> >> -     }
>> >> +     if (!adv7511->powered)
>> >> +             __adv7511_power_on(adv7511);
>> >
>> > The __adv7511_power_on() function does more than the above, in particular
>> > it performs an expensive regcache_sync() and calls adv7533_dsi_power_on()
>> > for the ADV7533. Don't those operations have side effects that are either
>> > not wanted or not needed here ? In any case this patch modifies the
>> > behaviour of the driver, which needs to be documented in the kernel
>> > message.
>>
>> So yes, while the adv7533 bits aren't needed in the internal function,
>> I'm finding the logic to pulse the HPD and the regcache_sync call seem
>> to be needed side effects, as without that logic, I get i2c_transfer()
>> errors in adv7511_get_edid_block().
>
> Does this patch fix the problem without requiring the 200ms delay ?

Partially, but not completely as I just explained in the other mail thread.

>> I'll try to rework this patch to split the two changes of reworking
>> the power_on/off function to be re-used (with no logic chage), and the
>> patch to reuse it in get_modes() which resolves a bug.
>
> Have you identified which register write fixes your problem here ?

So I initially thought it was the HPD pulse, but I'm finding that
without the regmap_sync I still see the i2c_transfer errors.

I need to figure out how to decompose the regmap_sync to try to narrow
it down further.

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

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi John,
>
> On Tuesday 22 Nov 2016 09:25:22 John Stultz wrote:
>> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote:
>> > On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
>>  @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>> >>       unsigned int count;
>> >>
>> >>       /* Reading the EDID only works if the device is powered */
>> >>
>> >> -     if (!adv7511->powered) {
>> >> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>> >> -                                ADV7511_POWER_POWER_DOWN, 0);
>> >> -             if (adv7511->i2c_main->irq) {
>> >> -                     regmap_write(adv7511->regmap,
>> >> ADV7511_REG_INT_ENABLE(0),
>> >> -                                  ADV7511_INT0_EDID_READY);
>> >> -                     regmap_write(adv7511->regmap,
>> >> ADV7511_REG_INT_ENABLE(1),
>> >> -                                  ADV7511_INT1_DDC_ERROR);
>> >> -             }
>> >> -             adv7511->current_edid_segment = -1;
>> >> -     }
>> >> +     if (!adv7511->powered)
>> >> +             __adv7511_power_on(adv7511);
>> >
>> > The __adv7511_power_on() function does more than the above, in particular
>> > it performs an expensive regcache_sync() and calls adv7533_dsi_power_on()
>> > for the ADV7533. Don't those operations have side effects that are either
>> > not wanted or not needed here ? In any case this patch modifies the
>> > behaviour of the driver, which needs to be documented in the kernel
>> > message.
>>
>> So yes, while the adv7533 bits aren't needed in the internal function,
>> I'm finding the logic to pulse the HPD and the regcache_sync call seem
>> to be needed side effects, as without that logic, I get i2c_transfer()
>> errors in adv7511_get_edid_block().
>
> Does this patch fix the problem without requiring the 200ms delay ?
>
>> I'll try to rework this patch to split the two changes of reworking
>> the power_on/off function to be re-used (with no logic chage), and the
>> patch to reuse it in get_modes() which resolves a bug.
>
> Have you identified which register write fixes your problem here ?

So I basically used regmap_sync_region() to bisect through the
registers to try to figure out which one calling sync on helps avoid
the issue, and I've narrowed it down to 0x43
(ADV7511_REG_EDID_I2C_ADDR).

If instead of the proposed patch here, I use the following patch
(copy/paste whitespace damage, apologies) seems to make things work
reliably:

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 8dba729..87403d7 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c

@@ -555,14 +557,18 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
                                     ADV7511_INT1_DDC_ERROR);
                }
                adv7511->current_edid_segment = -1;
+               regcache_sync_region(adv7511->regmap, 0x43, 0x43);
+
        }

        edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);

-       if (!adv7511->powered)
+       if (!adv7511->powered) {
                regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
                                   ADV7511_POWER_POWER_DOWN,
                                   ADV7511_POWER_POWER_DOWN);
+               regcache_mark_dirty(adv7511->regmap);
+       }

        kfree(adv7511->edid);
        adv7511->edid = edid;


But I suspect this isn't a proper fix. I tried adding
ADV7511_REG_EDID_I2C_ADDR to the volatile register list, but that
didn't seem to effect anything (and I still see problematic behavior
if I'm not explictly syncing it as above).

Thoughts on what the right thing is to do?
-john

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[Archit Taneja]

On 11/23/2016 01:16 AM, John Stultz wrote:
> On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>> Hi John,
>>
>> On Tuesday 22 Nov 2016 09:25:22 John Stultz wrote:
>>> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote:
>>>> On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
>>>  @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>>>>>       unsigned int count;
>>>>>
>>>>>       /* Reading the EDID only works if the device is powered */
>>>>>
>>>>> -     if (!adv7511->powered) {
>>>>> -             regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>>>>> -                                ADV7511_POWER_POWER_DOWN, 0);
>>>>> -             if (adv7511->i2c_main->irq) {
>>>>> -                     regmap_write(adv7511->regmap,
>>>>> ADV7511_REG_INT_ENABLE(0),
>>>>> -                                  ADV7511_INT0_EDID_READY);
>>>>> -                     regmap_write(adv7511->regmap,
>>>>> ADV7511_REG_INT_ENABLE(1),
>>>>> -                                  ADV7511_INT1_DDC_ERROR);
>>>>> -             }
>>>>> -             adv7511->current_edid_segment = -1;
>>>>> -     }
>>>>> +     if (!adv7511->powered)
>>>>> +             __adv7511_power_on(adv7511);
>>>>
>>>> The __adv7511_power_on() function does more than the above, in particular
>>>> it performs an expensive regcache_sync() and calls adv7533_dsi_power_on()
>>>> for the ADV7533. Don't those operations have side effects that are either
>>>> not wanted or not needed here ? In any case this patch modifies the
>>>> behaviour of the driver, which needs to be documented in the kernel
>>>> message.
>>>
>>> So yes, while the adv7533 bits aren't needed in the internal function,
>>> I'm finding the logic to pulse the HPD and the regcache_sync call seem
>>> to be needed side effects, as without that logic, I get i2c_transfer()
>>> errors in adv7511_get_edid_block().
>>
>> Does this patch fix the problem without requiring the 200ms delay ?
>>
>>> I'll try to rework this patch to split the two changes of reworking
>>> the power_on/off function to be re-used (with no logic chage), and the
>>> patch to reuse it in get_modes() which resolves a bug.
>>
>> Have you identified which register write fixes your problem here ?
>
> So I basically used regmap_sync_region() to bisect through the
> registers to try to figure out which one calling sync on helps avoid
> the issue, and I've narrowed it down to 0x43
> (ADV7511_REG_EDID_I2C_ADDR).

I guess if this register loses its state, then i2c errors are expected.

>
> If instead of the proposed patch here, I use the following patch
> (copy/paste whitespace damage, apologies) seems to make things work
> reliably:
>
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> index 8dba729..87403d7 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>
> @@ -555,14 +557,18 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>                                      ADV7511_INT1_DDC_ERROR);
>                 }
>                 adv7511->current_edid_segment = -1;
> +               regcache_sync_region(adv7511->regmap, 0x43, 0x43);

So, we're losing register state when get_modes() is called, or sometime before it.
Could you try to read a register with a known programmed value at the beginning of
adv7511_get_modes(), and check if it has already lost the state or not?

It's also possible that, in adv7511_get_modes(), when the chip is powered on, i.e,
when we call:

	regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
				   ADV7511_POWER_POWER_DOWN, 0);

the monitor wakes up, but there is some additional hpd toggling, which results
in registers to lose their state.

The patch below is something that was originally there in Lars's initial patch
for ADV7533 support. I'd dropped it since it didn't have much to do with ADV7533
itself. It should at least discard any HPD toggling caused by powering on the
chip in the next line.


diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
index ed6d25d..5ee40a4 100644
--- a/drivers/gpu/drm/i2c/adv7511.c
+++ b/drivers/gpu/drm/i2c/adv7511.c
@@ -654,6 +654,9 @@ static int adv7511_get_modes(struct adv7511 *adv7511,

  	/* Reading the EDID only works if the device is powered */
  	if (!adv7511->powered) {
+		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,
+				   ADV7511_REG_POWER2_HPD_SRC_MASK,
+				   ADV7511_REG_POWER2_HPD_SRC_NONE);
  		regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
  				   ADV7511_POWER_POWER_DOWN, 0);
  		if (adv7511->i2c_main->irq) {

> +
>         }
>
>         edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511);
>
> -       if (!adv7511->powered)
> +       if (!adv7511->powered) {
>                 regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>                                    ADV7511_POWER_POWER_DOWN,
>                                    ADV7511_POWER_POWER_DOWN);
> +               regcache_mark_dirty(adv7511->regmap);
> +       }
>
>         kfree(adv7511->edid);
>         adv7511->edid = edid;
>
>
> But I suspect this isn't a proper fix. I tried adding
> ADV7511_REG_EDID_I2C_ADDR to the volatile register list, but that
> didn't seem to effect anything (and I still see problematic behavior
> if I'm not explictly syncing it as above).
>
> Thoughts on what the right thing is to do?
> -john
>

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

On Tue, Nov 22, 2016 at 7:50 PM, Archit Taneja <architt@codeaurora.org> wrote:
> On 11/23/2016 01:16 AM, John Stultz wrote:
>> On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com> wrote:
>>> On Tuesday 22 Nov 2016 09:25:22 John Stultz wrote:
>>>> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote:
>>>>> On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
>>>>
>>>> I'll try to rework this patch to split the two changes of reworking
>>>> the power_on/off function to be re-used (with no logic chage), and the
>>>> patch to reuse it in get_modes() which resolves a bug.
>>>
>>>
>>> Have you identified which register write fixes your problem here ?
>>
>>
>> So I basically used regmap_sync_region() to bisect through the
>> registers to try to figure out which one calling sync on helps avoid
>> the issue, and I've narrowed it down to 0x43
>> (ADV7511_REG_EDID_I2C_ADDR).
>
>
> I guess if this register loses its state, then i2c errors are expected.
>
>>
>> If instead of the proposed patch here, I use the following patch
>> (copy/paste whitespace damage, apologies) seems to make things work
>> reliably:
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index 8dba729..87403d7 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>
>> @@ -555,14 +557,18 @@ static int adv7511_get_modes(struct adv7511
>> *adv7511,
>>                                      ADV7511_INT1_DDC_ERROR);
>>                 }
>>                 adv7511->current_edid_segment = -1;
>> +               regcache_sync_region(adv7511->regmap, 0x43, 0x43);
>
>
> So, we're losing register state when get_modes() is called, or sometime
> before it.
> Could you try to read a register with a known programmed value at the
> beginning of
> adv7511_get_modes(), and check if it has already lost the state or not?
>
> It's also possible that, in adv7511_get_modes(), when the chip is powered
> on, i.e,
> when we call:
>
>         regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>                                    ADV7511_POWER_POWER_DOWN, 0);
>
> the monitor wakes up, but there is some additional hpd toggling, which
> results
> in registers to lose their state.
>
> The patch below is something that was originally there in Lars's initial
> patch
> for ADV7533 support. I'd dropped it since it didn't have much to do with
> ADV7533
> itself. It should at least discard any HPD toggling caused by powering on
> the
> chip in the next line.
>
>
> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
> index ed6d25d..5ee40a4 100644
> --- a/drivers/gpu/drm/i2c/adv7511.c
> +++ b/drivers/gpu/drm/i2c/adv7511.c
> @@ -654,6 +654,9 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>
>         /* Reading the EDID only works if the device is powered */
>         if (!adv7511->powered) {
> +               regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,
> +                                  ADV7511_REG_POWER2_HPD_SRC_MASK,
> +                                  ADV7511_REG_POWER2_HPD_SRC_NONE);
>                 regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>                                    ADV7511_POWER_POWER_DOWN, 0);
>                 if (adv7511->i2c_main->irq) {


So this patch is what got me started on the re-using the
adv7511_power_on() logic, since it already had the bit to pulse the
HPD signal. It might be helpful, but seems like a separate issue from
the register state being lost. Unless I'm just not getting at
something more subtle that you're suggesting.

thanks
-john

Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally

[John Stultz]

On Tue, Nov 22, 2016 at 7:50 PM, Archit Taneja <architt@codeaurora.org> wrote:
> On 11/23/2016 01:16 AM, John Stultz wrote:
>> On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart
>> <laurent.pinchart@ideasonboard.com> wrote:
>>> On Tuesday 22 Nov 2016 09:25:22 John Stultz wrote:
>>>> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote:
>>>>> On Monday 21 Nov 2016 16:37:30 John Stultz wrote:
>>>>
>>>> I'll try to rework this patch to split the two changes of reworking
>>>> the power_on/off function to be re-used (with no logic chage), and the
>>>> patch to reuse it in get_modes() which resolves a bug.
>>>
>>>
>>> Have you identified which register write fixes your problem here ?
>>
>>
>> So I basically used regmap_sync_region() to bisect through the
>> registers to try to figure out which one calling sync on helps avoid
>> the issue, and I've narrowed it down to 0x43
>> (ADV7511_REG_EDID_I2C_ADDR).
>
>
> I guess if this register loses its state, then i2c errors are expected.
>
>>
>> If instead of the proposed patch here, I use the following patch
>> (copy/paste whitespace damage, apologies) seems to make things work
>> reliably:
>>
>> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> index 8dba729..87403d7 100644
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>>
>> @@ -555,14 +557,18 @@ static int adv7511_get_modes(struct adv7511
>> *adv7511,
>>                                      ADV7511_INT1_DDC_ERROR);
>>                 }
>>                 adv7511->current_edid_segment = -1;
>> +               regcache_sync_region(adv7511->regmap, 0x43, 0x43);
>
>
> So, we're losing register state when get_modes() is called, or sometime
> before it.
> Could you try to read a register with a known programmed value at the
> beginning of
> adv7511_get_modes(), and check if it has already lost the state or not?
>
> It's also possible that, in adv7511_get_modes(), when the chip is powered
> on, i.e,
> when we call:
>
>         regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>                                    ADV7511_POWER_POWER_DOWN, 0);
>
> the monitor wakes up, but there is some additional hpd toggling, which
> results
> in registers to lose their state.
>
> The patch below is something that was originally there in Lars's initial
> patch
> for ADV7533 support. I'd dropped it since it didn't have much to do with
> ADV7533
> itself. It should at least discard any HPD toggling caused by powering on
> the
> chip in the next line.
>
>
> diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c
> index ed6d25d..5ee40a4 100644
> --- a/drivers/gpu/drm/i2c/adv7511.c
> +++ b/drivers/gpu/drm/i2c/adv7511.c
> @@ -654,6 +654,9 @@ static int adv7511_get_modes(struct adv7511 *adv7511,
>
>         /* Reading the EDID only works if the device is powered */
>         if (!adv7511->powered) {
> +               regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2,
> +                                  ADV7511_REG_POWER2_HPD_SRC_MASK,
> +                                  ADV7511_REG_POWER2_HPD_SRC_NONE);
>                 regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER,
>                                    ADV7511_POWER_POWER_DOWN, 0);
>                 if (adv7511->i2c_main->irq) {


So this patch is what got me started on the re-using the
adv7511_power_on() logic, since it already had the bit to pulse the
HPD signal. It might be helpful, but seems like a separate issue from
the register state being lost. Unless I'm just not getting at
something more subtle that you're suggesting.

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

[RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[John Stultz]

Secton 4.1 of the adv7511 programming guide advises one waits
200ms after powering on the chip before trying to communicate
with it via i2c. Not doing so can cause reliability issues when
probing the EDID.

See:
http://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_Programming_Guide.pdf

So this patch simply adds a 200ms sleep at the end of the
power_on path. This greatly improves EDID probing reliabilty
on hotplug with the HiKey device.

Cc: David Airlie <airlied@linux.ie>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index b240e05..2114a4c 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -361,6 +361,8 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
 	 */
 	regcache_sync(adv7511->regmap);
 
+	msleep(200);
+
 	if (adv7511->type == ADV7533)
 		adv7533_dsi_power_on(adv7511);
 }
-- 
2.7.4

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[Laurent Pinchart]

Hi John,

Thank you for the patch.

On Monday 21 Nov 2016 16:37:31 John Stultz wrote:
> Secton 4.1 of the adv7511 programming guide advises one waits
> 200ms after powering on the chip before trying to communicate
> with it via i2c. Not doing so can cause reliability issues when
> probing the EDID.
> 
> See:
> http://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_P
> rogramming_Guide.pdf
> 
> So this patch simply adds a 200ms sleep at the end of the
> power_on path. This greatly improves EDID probing reliabilty
> on hotplug with the HiKey device.
> 
> Cc: David Airlie <airlied@linux.ie>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index b240e05..2114a4c
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -361,6 +361,8 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
>  	 */
>  	regcache_sync(adv7511->regmap);
> 
> +	msleep(200);
> +

The documentation states that

"The user should wait 200ms for the address to be decided, after the power 
supplies are high, before attempting to communicate with the ADV7511W using 
I2C."

The hardware user's guide further states that

"When initially powered up, there is a 200ms period before the device is ready 
to be addressed."

Not only the delay you add comes after lots of I2C communication, but the 
driver doesn't handle regulators, and thus doesn't power down the device at 
the hardware level. The initial power up should thus be long gone when this 
code is reached.

Could it be that, on the HiKey board, the power supply is controlled through 
another mean that doesn't comply with the 200ms rule ?

>  	if (adv7511->type == ADV7533)
>  		adv7533_dsi_power_on(adv7511);
>  }

-- 
Regards,

Laurent Pinchart

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[Laurent Pinchart]

Hi John,

Thank you for the patch.

On Monday 21 Nov 2016 16:37:31 John Stultz wrote:
> Secton 4.1 of the adv7511 programming guide advises one waits
> 200ms after powering on the chip before trying to communicate
> with it via i2c. Not doing so can cause reliability issues when
> probing the EDID.
> 
> See:
> http://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_P
> rogramming_Guide.pdf
> 
> So this patch simply adds a 200ms sleep at the end of the
> power_on path. This greatly improves EDID probing reliabilty
> on hotplug with the HiKey device.
> 
> Cc: David Airlie <airlied@linux.ie>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: John Stultz <john.stultz@linaro.org>
> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index b240e05..2114a4c
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -361,6 +361,8 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
>  	 */
>  	regcache_sync(adv7511->regmap);
> 
> +	msleep(200);
> +

The documentation states that

"The user should wait 200ms for the address to be decided, after the power 
supplies are high, before attempting to communicate with the ADV7511W using 
I2C."

The hardware user's guide further states that

"When initially powered up, there is a 200ms period before the device is ready 
to be addressed."

Not only the delay you add comes after lots of I2C communication, but the 
driver doesn't handle regulators, and thus doesn't power down the device at 
the hardware level. The initial power up should thus be long gone when this 
code is reached.

Could it be that, on the HiKey board, the power supply is controlled through 
another mean that doesn't comply with the 200ms rule ?

>  	if (adv7511->type == ADV7533)
>  		adv7533_dsi_power_on(adv7511);
>  }

-- 
Regards,

Laurent Pinchart

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

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[John Stultz]

On Tue, Nov 22, 2016 at 12:25 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 21 Nov 2016 16:37:31 John Stultz wrote:
>> Secton 4.1 of the adv7511 programming guide advises one waits
>> 200ms after powering on the chip before trying to communicate
>> with it via i2c. Not doing so can cause reliability issues when
>> probing the EDID.
>>
>> See:
>> http://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_P
>> rogramming_Guide.pdf
>>
>> So this patch simply adds a 200ms sleep at the end of the
>> power_on path. This greatly improves EDID probing reliabilty
>> on hotplug with the HiKey device.
>>
[snip]
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -361,6 +361,8 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
>>        */
>>       regcache_sync(adv7511->regmap);
>>
>> +     msleep(200);
>> +
>
> The documentation states that
>
> "The user should wait 200ms for the address to be decided, after the power
> supplies are high, before attempting to communicate with the ADV7511W using
> I2C."
>
> The hardware user's guide further states that
>
> "When initially powered up, there is a 200ms period before the device is ready
> to be addressed."
>
> Not only the delay you add comes after lots of I2C communication, but the
> driver doesn't handle regulators, and thus doesn't power down the device at
> the hardware level. The initial power up should thus be long gone when this
> code is reached.
>
> Could it be that, on the HiKey board, the power supply is controlled through
> another mean that doesn't comply with the 200ms rule ?

Thanks so much for the clarifications. Hopefully my confusion around
flipping the POWER_DOWN register and the actual power to the chip is
understandable. :)

Hrm...  So apparently I was mixing the result of this change up with
the refactoring of the power_on logic in the previous patch.

So initially I found this as I was seeing i2c_transfer() frequently
return errors when trying to read the EDID, after turning off the
ADV7511_POWER_POWER_DOWN register. The explanation in the guide to
wait 200ms made sense, and seemed to make the i2c_transfer errors go
away. But it ends up the i2c_tranfer errors are fixed by re-using the
power_on logic in the patch before.

However this patch still improves things, as without it I'm seeing the
DDCController status in adv7511_get_edid_block() being 1 ("reading
edid") and the adv7511_wait_for_edid() regularly times out.  It seems
like we don't get the irq and set the edid_read bit we're waiting on
until much later (after we decide there's no edid and try to start
using with 800x600).

Interestingly, without the msleep added in this patch, removing the
wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
and using the polling loop seems to make things just as reliable. So
maybe something is off with the irq handling here instead?

thanks
-john

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[John Stultz]

On Tue, Nov 22, 2016 at 12:25 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Monday 21 Nov 2016 16:37:31 John Stultz wrote:
>> Secton 4.1 of the adv7511 programming guide advises one waits
>> 200ms after powering on the chip before trying to communicate
>> with it via i2c. Not doing so can cause reliability issues when
>> probing the EDID.
>>
>> See:
>> http://www.analog.com/media/en/technical-documentation/user-guides/ADV7511_P
>> rogramming_Guide.pdf
>>
>> So this patch simply adds a 200ms sleep at the end of the
>> power_on path. This greatly improves EDID probing reliabilty
>> on hotplug with the HiKey device.
>>
[snip]
>> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
>> @@ -361,6 +361,8 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
>>        */
>>       regcache_sync(adv7511->regmap);
>>
>> +     msleep(200);
>> +
>
> The documentation states that
>
> "The user should wait 200ms for the address to be decided, after the power
> supplies are high, before attempting to communicate with the ADV7511W using
> I2C."
>
> The hardware user's guide further states that
>
> "When initially powered up, there is a 200ms period before the device is ready
> to be addressed."
>
> Not only the delay you add comes after lots of I2C communication, but the
> driver doesn't handle regulators, and thus doesn't power down the device at
> the hardware level. The initial power up should thus be long gone when this
> code is reached.
>
> Could it be that, on the HiKey board, the power supply is controlled through
> another mean that doesn't comply with the 200ms rule ?

Thanks so much for the clarifications. Hopefully my confusion around
flipping the POWER_DOWN register and the actual power to the chip is
understandable. :)

Hrm...  So apparently I was mixing the result of this change up with
the refactoring of the power_on logic in the previous patch.

So initially I found this as I was seeing i2c_transfer() frequently
return errors when trying to read the EDID, after turning off the
ADV7511_POWER_POWER_DOWN register. The explanation in the guide to
wait 200ms made sense, and seemed to make the i2c_transfer errors go
away. But it ends up the i2c_tranfer errors are fixed by re-using the
power_on logic in the patch before.

However this patch still improves things, as without it I'm seeing the
DDCController status in adv7511_get_edid_block() being 1 ("reading
edid") and the adv7511_wait_for_edid() regularly times out.  It seems
like we don't get the irq and set the edid_read bit we're waiting on
until much later (after we decide there's no edid and try to start
using with 800x600).

Interestingly, without the msleep added in this patch, removing the
wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
and using the polling loop seems to make things just as reliable. So
maybe something is off with the irq handling here instead?

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

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[John Stultz]

On Tue, Nov 22, 2016 at 9:38 AM, John Stultz <john.stultz@linaro.org> wrote:
>
> Interestingly, without the msleep added in this patch, removing the
> wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
> and using the polling loop seems to make things just as reliable. So
> maybe something is off with the irq handling here instead?

Ahhhh.. So I think the trouble here is the that when we fail waiting
for the irq, the backtrace is as follows:

[    8.318654] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
[    8.318661] [<ffffff8008087ddc>] show_stack+0x14/0x20
[    8.318671] [<ffffff80084344f0>] dump_stack+0x90/0xb0
[    8.318680] [<ffffff8008534650>] adv7511_get_edid_block+0x2c8/0x320
[    8.318687] [<ffffff80085214a8>] drm_do_get_edid+0x78/0x280
[    8.318693] [<ffffff8008534728>] adv7511_get_modes+0x80/0xd8
[    8.318700] [<ffffff8008534794>] adv7511_connector_get_modes+0x14/0x20
[    8.318710] [<ffffff8008500a54>]
drm_helper_probe_single_connector_modes+0x2bc/0x500
[    8.318718] [<ffffff800850e400>] drm_fb_helper_hotplug_event+0x130/0x188
[    8.318726] [<ffffff800850ee68>] drm_fbdev_cma_hotplug_event+0x10/0x20
[    8.318733] [<ffffff8008535718>] kirin_fbdev_output_poll_changed+0x20/0x58
[    8.318740] [<ffffff8008500cc0>] drm_kms_helper_hotplug_event+0x28/0x38
[    8.318748] [<ffffff80085010d8>] drm_helper_hpd_irq_event+0x138/0x180
[    8.318754] [<ffffff8008533850>] adv7511_irq_process+0x78/0xd8
[    8.318761] [<ffffff80085338c4>] adv7511_irq_handler+0x14/0x28
[    8.318769] [<ffffff8008100060>] irq_thread_fn+0x28/0x68
[    8.318775] [<ffffff8008100350>] irq_thread+0x128/0x1e8
[    8.318782] [<ffffff80080d2e68>] kthread+0xd0/0xe8
[    8.318788] [<ffffff8008082e80>] ret_from_fork+0x10/0x50

So we're actually in irq handling the hotplug interrupt, which is why
we never get the irq notification when the edid is read.

I suspect we need to use a workqueue to do the hotplug handling out of irq.

thanks
-john

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[John Stultz]

On Tue, Nov 22, 2016 at 9:38 AM, John Stultz <john.stultz@linaro.org> wrote:
>
> Interestingly, without the msleep added in this patch, removing the
> wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
> and using the polling loop seems to make things just as reliable. So
> maybe something is off with the irq handling here instead?

Ahhhh.. So I think the trouble here is the that when we fail waiting
for the irq, the backtrace is as follows:

[    8.318654] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
[    8.318661] [<ffffff8008087ddc>] show_stack+0x14/0x20
[    8.318671] [<ffffff80084344f0>] dump_stack+0x90/0xb0
[    8.318680] [<ffffff8008534650>] adv7511_get_edid_block+0x2c8/0x320
[    8.318687] [<ffffff80085214a8>] drm_do_get_edid+0x78/0x280
[    8.318693] [<ffffff8008534728>] adv7511_get_modes+0x80/0xd8
[    8.318700] [<ffffff8008534794>] adv7511_connector_get_modes+0x14/0x20
[    8.318710] [<ffffff8008500a54>]
drm_helper_probe_single_connector_modes+0x2bc/0x500
[    8.318718] [<ffffff800850e400>] drm_fb_helper_hotplug_event+0x130/0x188
[    8.318726] [<ffffff800850ee68>] drm_fbdev_cma_hotplug_event+0x10/0x20
[    8.318733] [<ffffff8008535718>] kirin_fbdev_output_poll_changed+0x20/0x58
[    8.318740] [<ffffff8008500cc0>] drm_kms_helper_hotplug_event+0x28/0x38
[    8.318748] [<ffffff80085010d8>] drm_helper_hpd_irq_event+0x138/0x180
[    8.318754] [<ffffff8008533850>] adv7511_irq_process+0x78/0xd8
[    8.318761] [<ffffff80085338c4>] adv7511_irq_handler+0x14/0x28
[    8.318769] [<ffffff8008100060>] irq_thread_fn+0x28/0x68
[    8.318775] [<ffffff8008100350>] irq_thread+0x128/0x1e8
[    8.318782] [<ffffff80080d2e68>] kthread+0xd0/0xe8
[    8.318788] [<ffffff8008082e80>] ret_from_fork+0x10/0x50

So we're actually in irq handling the hotplug interrupt, which is why
we never get the irq notification when the edid is read.

I suspect we need to use a workqueue to do the hotplug handling out of irq.

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

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[Laurent Pinchart]

Hi John,

(CC'ing Daniel)

On Tuesday 22 Nov 2016 10:07:53 John Stultz wrote:
> On Tue, Nov 22, 2016 at 9:38 AM, John Stultz <john.stultz@linaro.org> wrote:
> > Interestingly, without the msleep added in this patch, removing the
> > wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
> > and using the polling loop seems to make things just as reliable. So
> > maybe something is off with the irq handling here instead?
> 
> Ahhhh.. So I think the trouble here is the that when we fail waiting
> for the irq, the backtrace is as follows:
> 
> [    8.318654] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
> [    8.318661] [<ffffff8008087ddc>] show_stack+0x14/0x20
> [    8.318671] [<ffffff80084344f0>] dump_stack+0x90/0xb0
> [    8.318680] [<ffffff8008534650>] adv7511_get_edid_block+0x2c8/0x320
> [    8.318687] [<ffffff80085214a8>] drm_do_get_edid+0x78/0x280
> [    8.318693] [<ffffff8008534728>] adv7511_get_modes+0x80/0xd8
> [    8.318700] [<ffffff8008534794>] adv7511_connector_get_modes+0x14/0x20
> [    8.318710] [<ffffff8008500a54>]
> drm_helper_probe_single_connector_modes+0x2bc/0x500
> [    8.318718] [<ffffff800850e400>] drm_fb_helper_hotplug_event+0x130/0x188
> [    8.318726] [<ffffff800850ee68>] drm_fbdev_cma_hotplug_event+0x10/0x20
> [    8.318733] [<ffffff8008535718>]
> kirin_fbdev_output_poll_changed+0x20/0x58
> [    8.318740] [<ffffff8008500cc0>] drm_kms_helper_hotplug_event+0x28/0x38
> [    8.318748] [<ffffff80085010d8>] drm_helper_hpd_irq_event+0x138/0x180
> [    8.318754] [<ffffff8008533850>] adv7511_irq_process+0x78/0xd8
> [    8.318761] [<ffffff80085338c4>] adv7511_irq_handler+0x14/0x28
> [    8.318769] [<ffffff8008100060>] irq_thread_fn+0x28/0x68
> [    8.318775] [<ffffff8008100350>] irq_thread+0x128/0x1e8
> [    8.318782] [<ffffff80080d2e68>] kthread+0xd0/0xe8
> [    8.318788] [<ffffff8008082e80>] ret_from_fork+0x10/0x50
> 
> So we're actually in irq handling the hotplug interrupt, which is why
> we never get the irq notification when the edid is read.
> 
> I suspect we need to use a workqueue to do the hotplug handling out of irq.

Lovely :-)

Quoting the DRM documentation:

/**
 * drm_helper_hpd_irq_event - hotplug processing
 * @dev: drm_device
 *
 * Drivers can use this helper function to run a detect cycle on all 
connectors
 * which have the DRM_CONNECTOR_POLL_HPD flag set in their &polled member. All
 * other connectors are ignored, which is useful to avoid reprobing fixed
 * panels.
 *
 * This helper function is useful for drivers which can't or don't track 
hotplug
 * interrupts for each connector.
 *
 * Drivers which support hotplug interrupts for each connector individually 
and
 * which have a more fine-grained detect logic should bypass this code and
 * directly call drm_kms_helper_hotplug_event() in case the connector state
 * changed.
 *
 * This function must be called from process context with no mode
 * setting locks held.
 *
 * Note that a connector can be both polled and probed from the hotplug 
handler,
 * in case the hotplug interrupt is known to be unreliable.
 */

So it looks like we should use drm_kms_helper_hotplug_event() instead.

/**
 * drm_kms_helper_hotplug_event - fire off KMS hotplug events
 * @dev: drm_device whose connector state changed
 *
 * This function fires off the uevent for userspace and also calls the
 * output_poll_changed function, which is most commonly used to inform the 
fbdev
 * emulation code and allow it to update the fbcon output configuration.
 *
 * Drivers should call this from their hotplug handling code when a change is
 * detected. Note that this function does not do any output detection of its
 * own, like drm_helper_hpd_irq_event() does - this is assumed to be done by 
the
 * driver already.
 *
 * This function must be called from process context with no mode
 * setting locks held.
 */

The function suffers from the same problem though, that it must be called from 
process context.

Daniel, why do we have an API the is clearly related to interrupt handling but 
requires the caller to implement a workqueue ?

-- 
Regards,

Laurent Pinchart

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[Laurent Pinchart]

Hi John,

(CC'ing Daniel)

On Tuesday 22 Nov 2016 10:07:53 John Stultz wrote:
> On Tue, Nov 22, 2016 at 9:38 AM, John Stultz <john.stultz@linaro.org> wrote:
> > Interestingly, without the msleep added in this patch, removing the
> > wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
> > and using the polling loop seems to make things just as reliable. So
> > maybe something is off with the irq handling here instead?
> 
> Ahhhh.. So I think the trouble here is the that when we fail waiting
> for the irq, the backtrace is as follows:
> 
> [    8.318654] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
> [    8.318661] [<ffffff8008087ddc>] show_stack+0x14/0x20
> [    8.318671] [<ffffff80084344f0>] dump_stack+0x90/0xb0
> [    8.318680] [<ffffff8008534650>] adv7511_get_edid_block+0x2c8/0x320
> [    8.318687] [<ffffff80085214a8>] drm_do_get_edid+0x78/0x280
> [    8.318693] [<ffffff8008534728>] adv7511_get_modes+0x80/0xd8
> [    8.318700] [<ffffff8008534794>] adv7511_connector_get_modes+0x14/0x20
> [    8.318710] [<ffffff8008500a54>]
> drm_helper_probe_single_connector_modes+0x2bc/0x500
> [    8.318718] [<ffffff800850e400>] drm_fb_helper_hotplug_event+0x130/0x188
> [    8.318726] [<ffffff800850ee68>] drm_fbdev_cma_hotplug_event+0x10/0x20
> [    8.318733] [<ffffff8008535718>]
> kirin_fbdev_output_poll_changed+0x20/0x58
> [    8.318740] [<ffffff8008500cc0>] drm_kms_helper_hotplug_event+0x28/0x38
> [    8.318748] [<ffffff80085010d8>] drm_helper_hpd_irq_event+0x138/0x180
> [    8.318754] [<ffffff8008533850>] adv7511_irq_process+0x78/0xd8
> [    8.318761] [<ffffff80085338c4>] adv7511_irq_handler+0x14/0x28
> [    8.318769] [<ffffff8008100060>] irq_thread_fn+0x28/0x68
> [    8.318775] [<ffffff8008100350>] irq_thread+0x128/0x1e8
> [    8.318782] [<ffffff80080d2e68>] kthread+0xd0/0xe8
> [    8.318788] [<ffffff8008082e80>] ret_from_fork+0x10/0x50
> 
> So we're actually in irq handling the hotplug interrupt, which is why
> we never get the irq notification when the edid is read.
> 
> I suspect we need to use a workqueue to do the hotplug handling out of irq.

Lovely :-)

Quoting the DRM documentation:

/**
 * drm_helper_hpd_irq_event - hotplug processing
 * @dev: drm_device
 *
 * Drivers can use this helper function to run a detect cycle on all 
connectors
 * which have the DRM_CONNECTOR_POLL_HPD flag set in their &polled member. All
 * other connectors are ignored, which is useful to avoid reprobing fixed
 * panels.
 *
 * This helper function is useful for drivers which can't or don't track 
hotplug
 * interrupts for each connector.
 *
 * Drivers which support hotplug interrupts for each connector individually 
and
 * which have a more fine-grained detect logic should bypass this code and
 * directly call drm_kms_helper_hotplug_event() in case the connector state
 * changed.
 *
 * This function must be called from process context with no mode
 * setting locks held.
 *
 * Note that a connector can be both polled and probed from the hotplug 
handler,
 * in case the hotplug interrupt is known to be unreliable.
 */

So it looks like we should use drm_kms_helper_hotplug_event() instead.

/**
 * drm_kms_helper_hotplug_event - fire off KMS hotplug events
 * @dev: drm_device whose connector state changed
 *
 * This function fires off the uevent for userspace and also calls the
 * output_poll_changed function, which is most commonly used to inform the 
fbdev
 * emulation code and allow it to update the fbcon output configuration.
 *
 * Drivers should call this from their hotplug handling code when a change is
 * detected. Note that this function does not do any output detection of its
 * own, like drm_helper_hpd_irq_event() does - this is assumed to be done by 
the
 * driver already.
 *
 * This function must be called from process context with no mode
 * setting locks held.
 */

The function suffers from the same problem though, that it must be called from 
process context.

Daniel, why do we have an API the is clearly related to interrupt handling but 
requires the caller to implement a workqueue ?

-- 
Regards,

Laurent Pinchart

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

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[John Stultz]

On Tue, Nov 22, 2016 at 10:23 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tuesday 22 Nov 2016 10:07:53 John Stultz wrote:
>> On Tue, Nov 22, 2016 at 9:38 AM, John Stultz <john.stultz@linaro.org> wrote:
>> > Interestingly, without the msleep added in this patch, removing the
>> > wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
>> > and using the polling loop seems to make things just as reliable. So
>> > maybe something is off with the irq handling here instead?
>>
>> Ahhhh.. So I think the trouble here is the that when we fail waiting
>> for the irq, the backtrace is as follows:
>>
>> [    8.318654] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
>> [    8.318661] [<ffffff8008087ddc>] show_stack+0x14/0x20
>> [    8.318671] [<ffffff80084344f0>] dump_stack+0x90/0xb0
>> [    8.318680] [<ffffff8008534650>] adv7511_get_edid_block+0x2c8/0x320
>> [    8.318687] [<ffffff80085214a8>] drm_do_get_edid+0x78/0x280
>> [    8.318693] [<ffffff8008534728>] adv7511_get_modes+0x80/0xd8
>> [    8.318700] [<ffffff8008534794>] adv7511_connector_get_modes+0x14/0x20
>> [    8.318710] [<ffffff8008500a54>]
>> drm_helper_probe_single_connector_modes+0x2bc/0x500
>> [    8.318718] [<ffffff800850e400>] drm_fb_helper_hotplug_event+0x130/0x188
>> [    8.318726] [<ffffff800850ee68>] drm_fbdev_cma_hotplug_event+0x10/0x20
>> [    8.318733] [<ffffff8008535718>]
>> kirin_fbdev_output_poll_changed+0x20/0x58
>> [    8.318740] [<ffffff8008500cc0>] drm_kms_helper_hotplug_event+0x28/0x38
>> [    8.318748] [<ffffff80085010d8>] drm_helper_hpd_irq_event+0x138/0x180
>> [    8.318754] [<ffffff8008533850>] adv7511_irq_process+0x78/0xd8
>> [    8.318761] [<ffffff80085338c4>] adv7511_irq_handler+0x14/0x28
>> [    8.318769] [<ffffff8008100060>] irq_thread_fn+0x28/0x68
>> [    8.318775] [<ffffff8008100350>] irq_thread+0x128/0x1e8
>> [    8.318782] [<ffffff80080d2e68>] kthread+0xd0/0xe8
>> [    8.318788] [<ffffff8008082e80>] ret_from_fork+0x10/0x50
>>
>> So we're actually in irq handling the hotplug interrupt, which is why
>> we never get the irq notification when the edid is read.
>>
>> I suspect we need to use a workqueue to do the hotplug handling out of irq.

So yea, using schedule_work on a work_struct to defer the hotplug
handling seems to solve this issue.  Thanks again for pushing back on
the msleep approach.   :)

> Lovely :-)
>
> Quoting the DRM documentation:
>
> /**
>  * drm_helper_hpd_irq_event - hotplug processing
>  * @dev: drm_device
>  *
>  * Drivers can use this helper function to run a detect cycle on all
> connectors
>  * which have the DRM_CONNECTOR_POLL_HPD flag set in their &polled member. All
>  * other connectors are ignored, which is useful to avoid reprobing fixed
>  * panels.
>  *
>  * This helper function is useful for drivers which can't or don't track
> hotplug
>  * interrupts for each connector.
>  *
>  * Drivers which support hotplug interrupts for each connector individually
> and
>  * which have a more fine-grained detect logic should bypass this code and
>  * directly call drm_kms_helper_hotplug_event() in case the connector state
>  * changed.
>  *
>  * This function must be called from process context with no mode
>  * setting locks held.
>  *
>  * Note that a connector can be both polled and probed from the hotplug
> handler,
>  * in case the hotplug interrupt is known to be unreliable.
>  */
>
> So it looks like we should use drm_kms_helper_hotplug_event() instead.

Ok. I'll switch to this in my patch set as well. It doesn't seem to
have any behavioral effect that I can see right off.

thanks
-john

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[John Stultz]

On Tue, Nov 22, 2016 at 10:23 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> On Tuesday 22 Nov 2016 10:07:53 John Stultz wrote:
>> On Tue, Nov 22, 2016 at 9:38 AM, John Stultz <john.stultz@linaro.org> wrote:
>> > Interestingly, without the msleep added in this patch, removing the
>> > wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
>> > and using the polling loop seems to make things just as reliable. So
>> > maybe something is off with the irq handling here instead?
>>
>> Ahhhh.. So I think the trouble here is the that when we fail waiting
>> for the irq, the backtrace is as follows:
>>
>> [    8.318654] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
>> [    8.318661] [<ffffff8008087ddc>] show_stack+0x14/0x20
>> [    8.318671] [<ffffff80084344f0>] dump_stack+0x90/0xb0
>> [    8.318680] [<ffffff8008534650>] adv7511_get_edid_block+0x2c8/0x320
>> [    8.318687] [<ffffff80085214a8>] drm_do_get_edid+0x78/0x280
>> [    8.318693] [<ffffff8008534728>] adv7511_get_modes+0x80/0xd8
>> [    8.318700] [<ffffff8008534794>] adv7511_connector_get_modes+0x14/0x20
>> [    8.318710] [<ffffff8008500a54>]
>> drm_helper_probe_single_connector_modes+0x2bc/0x500
>> [    8.318718] [<ffffff800850e400>] drm_fb_helper_hotplug_event+0x130/0x188
>> [    8.318726] [<ffffff800850ee68>] drm_fbdev_cma_hotplug_event+0x10/0x20
>> [    8.318733] [<ffffff8008535718>]
>> kirin_fbdev_output_poll_changed+0x20/0x58
>> [    8.318740] [<ffffff8008500cc0>] drm_kms_helper_hotplug_event+0x28/0x38
>> [    8.318748] [<ffffff80085010d8>] drm_helper_hpd_irq_event+0x138/0x180
>> [    8.318754] [<ffffff8008533850>] adv7511_irq_process+0x78/0xd8
>> [    8.318761] [<ffffff80085338c4>] adv7511_irq_handler+0x14/0x28
>> [    8.318769] [<ffffff8008100060>] irq_thread_fn+0x28/0x68
>> [    8.318775] [<ffffff8008100350>] irq_thread+0x128/0x1e8
>> [    8.318782] [<ffffff80080d2e68>] kthread+0xd0/0xe8
>> [    8.318788] [<ffffff8008082e80>] ret_from_fork+0x10/0x50
>>
>> So we're actually in irq handling the hotplug interrupt, which is why
>> we never get the irq notification when the edid is read.
>>
>> I suspect we need to use a workqueue to do the hotplug handling out of irq.

So yea, using schedule_work on a work_struct to defer the hotplug
handling seems to solve this issue.  Thanks again for pushing back on
the msleep approach.   :)

> Lovely :-)
>
> Quoting the DRM documentation:
>
> /**
>  * drm_helper_hpd_irq_event - hotplug processing
>  * @dev: drm_device
>  *
>  * Drivers can use this helper function to run a detect cycle on all
> connectors
>  * which have the DRM_CONNECTOR_POLL_HPD flag set in their &polled member. All
>  * other connectors are ignored, which is useful to avoid reprobing fixed
>  * panels.
>  *
>  * This helper function is useful for drivers which can't or don't track
> hotplug
>  * interrupts for each connector.
>  *
>  * Drivers which support hotplug interrupts for each connector individually
> and
>  * which have a more fine-grained detect logic should bypass this code and
>  * directly call drm_kms_helper_hotplug_event() in case the connector state
>  * changed.
>  *
>  * This function must be called from process context with no mode
>  * setting locks held.
>  *
>  * Note that a connector can be both polled and probed from the hotplug
> handler,
>  * in case the hotplug interrupt is known to be unreliable.
>  */
>
> So it looks like we should use drm_kms_helper_hotplug_event() instead.

Ok. I'll switch to this in my patch set as well. It doesn't seem to
have any behavioral effect that I can see right off.

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

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[Daniel Vetter]

On Tue, Nov 22, 2016 at 08:23:38PM +0200, Laurent Pinchart wrote:
> Hi John,
> 
> (CC'ing Daniel)
> 
> On Tuesday 22 Nov 2016 10:07:53 John Stultz wrote:
> > On Tue, Nov 22, 2016 at 9:38 AM, John Stultz <john.stultz@linaro.org> wrote:
> > > Interestingly, without the msleep added in this patch, removing the
> > > wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
> > > and using the polling loop seems to make things just as reliable. So
> > > maybe something is off with the irq handling here instead?
> > 
> > Ahhhh.. So I think the trouble here is the that when we fail waiting
> > for the irq, the backtrace is as follows:
> > 
> > [    8.318654] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
> > [    8.318661] [<ffffff8008087ddc>] show_stack+0x14/0x20
> > [    8.318671] [<ffffff80084344f0>] dump_stack+0x90/0xb0
> > [    8.318680] [<ffffff8008534650>] adv7511_get_edid_block+0x2c8/0x320
> > [    8.318687] [<ffffff80085214a8>] drm_do_get_edid+0x78/0x280
> > [    8.318693] [<ffffff8008534728>] adv7511_get_modes+0x80/0xd8
> > [    8.318700] [<ffffff8008534794>] adv7511_connector_get_modes+0x14/0x20
> > [    8.318710] [<ffffff8008500a54>]
> > drm_helper_probe_single_connector_modes+0x2bc/0x500
> > [    8.318718] [<ffffff800850e400>] drm_fb_helper_hotplug_event+0x130/0x188
> > [    8.318726] [<ffffff800850ee68>] drm_fbdev_cma_hotplug_event+0x10/0x20
> > [    8.318733] [<ffffff8008535718>]
> > kirin_fbdev_output_poll_changed+0x20/0x58
> > [    8.318740] [<ffffff8008500cc0>] drm_kms_helper_hotplug_event+0x28/0x38
> > [    8.318748] [<ffffff80085010d8>] drm_helper_hpd_irq_event+0x138/0x180
> > [    8.318754] [<ffffff8008533850>] adv7511_irq_process+0x78/0xd8
> > [    8.318761] [<ffffff80085338c4>] adv7511_irq_handler+0x14/0x28
> > [    8.318769] [<ffffff8008100060>] irq_thread_fn+0x28/0x68
> > [    8.318775] [<ffffff8008100350>] irq_thread+0x128/0x1e8
> > [    8.318782] [<ffffff80080d2e68>] kthread+0xd0/0xe8
> > [    8.318788] [<ffffff8008082e80>] ret_from_fork+0x10/0x50
> > 
> > So we're actually in irq handling the hotplug interrupt, which is why
> > we never get the irq notification when the edid is read.
> > 
> > I suspect we need to use a workqueue to do the hotplug handling out of irq.
> 
> Lovely :-)
> 
> Quoting the DRM documentation:
> 
> /**
>  * drm_helper_hpd_irq_event - hotplug processing
>  * @dev: drm_device
>  *
>  * Drivers can use this helper function to run a detect cycle on all 
> connectors
>  * which have the DRM_CONNECTOR_POLL_HPD flag set in their &polled member. All
>  * other connectors are ignored, which is useful to avoid reprobing fixed
>  * panels.
>  *
>  * This helper function is useful for drivers which can't or don't track 
> hotplug
>  * interrupts for each connector.
>  *
>  * Drivers which support hotplug interrupts for each connector individually 
> and
>  * which have a more fine-grained detect logic should bypass this code and
>  * directly call drm_kms_helper_hotplug_event() in case the connector state
>  * changed.
>  *
>  * This function must be called from process context with no mode
>  * setting locks held.
>  *
>  * Note that a connector can be both polled and probed from the hotplug 
> handler,
>  * in case the hotplug interrupt is known to be unreliable.
>  */
> 
> So it looks like we should use drm_kms_helper_hotplug_event() instead.
> 
> /**
>  * drm_kms_helper_hotplug_event - fire off KMS hotplug events
>  * @dev: drm_device whose connector state changed
>  *
>  * This function fires off the uevent for userspace and also calls the
>  * output_poll_changed function, which is most commonly used to inform the 
> fbdev
>  * emulation code and allow it to update the fbcon output configuration.
>  *
>  * Drivers should call this from their hotplug handling code when a change is
>  * detected. Note that this function does not do any output detection of its
>  * own, like drm_helper_hpd_irq_event() does - this is assumed to be done by 
> the
>  * driver already.
>  *
>  * This function must be called from process context with no mode
>  * setting locks held.
>  */
> 
> The function suffers from the same problem though, that it must be called from 
> process context.
> 
> Daniel, why do we have an API the is clearly related to interrupt handling but 
> requires the caller to implement a workqueue ?

Because in general you need that workqueue anyway, and up to now there was
no driver ever who didn't have a work-queue already. Nesting workqueues
within workqueues seemed beyond silly, hence why I removed them in:

commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Oct 23 18:23:34 2012 +0000

    drm: run the hpd irq event code directly

I guess we could talk about re-introducing a work-item based version of
drm_helper_hpd_irq_event. But for drm_kms_helper_hotplug_event I think it
doesn't make sense - if you call that you've probably just done a pile of
i2c transactions, and those can sleep. If you haven't done i2c
transactions, then it's not an external panel, and why exactly are you
handling hpd for them?

-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[Daniel Vetter]

On Tue, Nov 22, 2016 at 08:23:38PM +0200, Laurent Pinchart wrote:
> Hi John,
> 
> (CC'ing Daniel)
> 
> On Tuesday 22 Nov 2016 10:07:53 John Stultz wrote:
> > On Tue, Nov 22, 2016 at 9:38 AM, John Stultz <john.stultz@linaro.org> wrote:
> > > Interestingly, without the msleep added in this patch, removing the
> > > wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
> > > and using the polling loop seems to make things just as reliable. So
> > > maybe something is off with the irq handling here instead?
> > 
> > Ahhhh.. So I think the trouble here is the that when we fail waiting
> > for the irq, the backtrace is as follows:
> > 
> > [    8.318654] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
> > [    8.318661] [<ffffff8008087ddc>] show_stack+0x14/0x20
> > [    8.318671] [<ffffff80084344f0>] dump_stack+0x90/0xb0
> > [    8.318680] [<ffffff8008534650>] adv7511_get_edid_block+0x2c8/0x320
> > [    8.318687] [<ffffff80085214a8>] drm_do_get_edid+0x78/0x280
> > [    8.318693] [<ffffff8008534728>] adv7511_get_modes+0x80/0xd8
> > [    8.318700] [<ffffff8008534794>] adv7511_connector_get_modes+0x14/0x20
> > [    8.318710] [<ffffff8008500a54>]
> > drm_helper_probe_single_connector_modes+0x2bc/0x500
> > [    8.318718] [<ffffff800850e400>] drm_fb_helper_hotplug_event+0x130/0x188
> > [    8.318726] [<ffffff800850ee68>] drm_fbdev_cma_hotplug_event+0x10/0x20
> > [    8.318733] [<ffffff8008535718>]
> > kirin_fbdev_output_poll_changed+0x20/0x58
> > [    8.318740] [<ffffff8008500cc0>] drm_kms_helper_hotplug_event+0x28/0x38
> > [    8.318748] [<ffffff80085010d8>] drm_helper_hpd_irq_event+0x138/0x180
> > [    8.318754] [<ffffff8008533850>] adv7511_irq_process+0x78/0xd8
> > [    8.318761] [<ffffff80085338c4>] adv7511_irq_handler+0x14/0x28
> > [    8.318769] [<ffffff8008100060>] irq_thread_fn+0x28/0x68
> > [    8.318775] [<ffffff8008100350>] irq_thread+0x128/0x1e8
> > [    8.318782] [<ffffff80080d2e68>] kthread+0xd0/0xe8
> > [    8.318788] [<ffffff8008082e80>] ret_from_fork+0x10/0x50
> > 
> > So we're actually in irq handling the hotplug interrupt, which is why
> > we never get the irq notification when the edid is read.
> > 
> > I suspect we need to use a workqueue to do the hotplug handling out of irq.
> 
> Lovely :-)
> 
> Quoting the DRM documentation:
> 
> /**
>  * drm_helper_hpd_irq_event - hotplug processing
>  * @dev: drm_device
>  *
>  * Drivers can use this helper function to run a detect cycle on all 
> connectors
>  * which have the DRM_CONNECTOR_POLL_HPD flag set in their &polled member. All
>  * other connectors are ignored, which is useful to avoid reprobing fixed
>  * panels.
>  *
>  * This helper function is useful for drivers which can't or don't track 
> hotplug
>  * interrupts for each connector.
>  *
>  * Drivers which support hotplug interrupts for each connector individually 
> and
>  * which have a more fine-grained detect logic should bypass this code and
>  * directly call drm_kms_helper_hotplug_event() in case the connector state
>  * changed.
>  *
>  * This function must be called from process context with no mode
>  * setting locks held.
>  *
>  * Note that a connector can be both polled and probed from the hotplug 
> handler,
>  * in case the hotplug interrupt is known to be unreliable.
>  */
> 
> So it looks like we should use drm_kms_helper_hotplug_event() instead.
> 
> /**
>  * drm_kms_helper_hotplug_event - fire off KMS hotplug events
>  * @dev: drm_device whose connector state changed
>  *
>  * This function fires off the uevent for userspace and also calls the
>  * output_poll_changed function, which is most commonly used to inform the 
> fbdev
>  * emulation code and allow it to update the fbcon output configuration.
>  *
>  * Drivers should call this from their hotplug handling code when a change is
>  * detected. Note that this function does not do any output detection of its
>  * own, like drm_helper_hpd_irq_event() does - this is assumed to be done by 
> the
>  * driver already.
>  *
>  * This function must be called from process context with no mode
>  * setting locks held.
>  */
> 
> The function suffers from the same problem though, that it must be called from 
> process context.
> 
> Daniel, why do we have an API the is clearly related to interrupt handling but 
> requires the caller to implement a workqueue ?

Because in general you need that workqueue anyway, and up to now there was
no driver ever who didn't have a work-queue already. Nesting workqueues
within workqueues seemed beyond silly, hence why I removed them in:

commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue Oct 23 18:23:34 2012 +0000

    drm: run the hpd irq event code directly

I guess we could talk about re-introducing a work-item based version of
drm_helper_hpd_irq_event. But for drm_kms_helper_hotplug_event I think it
doesn't make sense - if you call that you've probably just done a pile of
i2c transactions, and those can sleep. If you haven't done i2c
transactions, then it's not an external panel, and why exactly are you
handling hpd for them?

-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[Laurent Pinchart]

Hi Daniel,

On Wednesday 23 Nov 2016 08:55:37 Daniel Vetter wrote:
> On Tue, Nov 22, 2016 at 08:23:38PM +0200, Laurent Pinchart wrote:
> > On Tuesday 22 Nov 2016 10:07:53 John Stultz wrote:
> >> On Tue, Nov 22, 2016 at 9:38 AM, John Stultz wrote:
> >>> Interestingly, without the msleep added in this patch, removing the
> >>> wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
> >>> and using the polling loop seems to make things just as reliable. So
> >>> maybe something is off with the irq handling here instead?
> >> 
> >> Ahhhh.. So I think the trouble here is the that when we fail waiting
> >> for the irq, the backtrace is as follows:
> >> 
> >> [    8.318654] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
> >> [    8.318661] [<ffffff8008087ddc>] show_stack+0x14/0x20
> >> [    8.318671] [<ffffff80084344f0>] dump_stack+0x90/0xb0
> >> [    8.318680] [<ffffff8008534650>] adv7511_get_edid_block+0x2c8/0x320
> >> [    8.318687] [<ffffff80085214a8>] drm_do_get_edid+0x78/0x280
> >> [    8.318693] [<ffffff8008534728>] adv7511_get_modes+0x80/0xd8
> >> [    8.318700] [<ffffff8008534794>]
> >> adv7511_connector_get_modes+0x14/0x20
> >> [    8.318710] [<ffffff8008500a54>]
> >> drm_helper_probe_single_connector_modes+0x2bc/0x500
> >> [    8.318718] [<ffffff800850e400>]
> >> drm_fb_helper_hotplug_event+0x130/0x188
> >> [    8.318726] [<ffffff800850ee68>]
> >> drm_fbdev_cma_hotplug_event+0x10/0x20
> >> [    8.318733] [<ffffff8008535718>]
> >> kirin_fbdev_output_poll_changed+0x20/0x58
> >> [    8.318740] [<ffffff8008500cc0>]
> >> drm_kms_helper_hotplug_event+0x28/0x38
> >> [    8.318748] [<ffffff80085010d8>] drm_helper_hpd_irq_event+0x138/0x180
> >> [    8.318754] [<ffffff8008533850>] adv7511_irq_process+0x78/0xd8
> >> [    8.318761] [<ffffff80085338c4>] adv7511_irq_handler+0x14/0x28
> >> [    8.318769] [<ffffff8008100060>] irq_thread_fn+0x28/0x68
> >> [    8.318775] [<ffffff8008100350>] irq_thread+0x128/0x1e8
> >> [    8.318782] [<ffffff80080d2e68>] kthread+0xd0/0xe8
> >> [    8.318788] [<ffffff8008082e80>] ret_from_fork+0x10/0x50
> >> 
> >> So we're actually in irq handling the hotplug interrupt, which is why
> >> we never get the irq notification when the edid is read.
> >> 
> >> I suspect we need to use a workqueue to do the hotplug handling out of
> >> irq.
> > 
> > Lovely :-)
> > 
> > Quoting the DRM documentation:
> > 
> > /**
> >  * drm_helper_hpd_irq_event - hotplug processing
> >  * @dev: drm_device
> >  *
> >  * Drivers can use this helper function to run a detect cycle on all
> > connectors
> >  * which have the DRM_CONNECTOR_POLL_HPD flag set in their &polled member.
> > All
> >  * other connectors are ignored, which is useful to avoid reprobing fixed
> >  * panels.
> >  *
> >  * This helper function is useful for drivers which can't or don't track
> > hotplug
> >  * interrupts for each connector.
> >  *
> >  * Drivers which support hotplug interrupts for each connector
> > individually and
> >  * which have a more fine-grained detect logic should bypass this code and
> >  * directly call drm_kms_helper_hotplug_event() in case the connector
> > state
> >  * changed.
> >  *
> >  * This function must be called from process context with no mode
> >  * setting locks held.
> >  *
> >  * Note that a connector can be both polled and probed from the hotplug
> > handler,
> >  * in case the hotplug interrupt is known to be unreliable.
> >  */
> > 
> > So it looks like we should use drm_kms_helper_hotplug_event() instead.
> > 
> > /**
> >  * drm_kms_helper_hotplug_event - fire off KMS hotplug events
> >  * @dev: drm_device whose connector state changed
> >  *
> >  * This function fires off the uevent for userspace and also calls the
> >  * output_poll_changed function, which is most commonly used to inform the
> > fbdev
> >  * emulation code and allow it to update the fbcon output configuration.
> >  *
> >  * Drivers should call this from their hotplug handling code when a change
> > is
> >  * detected. Note that this function does not do any output detection of
> > its
> >  * own, like drm_helper_hpd_irq_event() does - this is assumed to be done
> > by the
> >  * driver already.
> >  *
> >  * This function must be called from process context with no mode
> >  * setting locks held.
> >  */
> > 
> > The function suffers from the same problem though, that it must be called
> > from process context.
> > 
> > Daniel, why do we have an API the is clearly related to interrupt handling
> > but requires the caller to implement a workqueue ?
> 
> Because in general you need that workqueue anyway, and up to now there was
> no driver ever who didn't have a work-queue already.

None of the bridge drivers in drivers/gpu/drm/bridge/ have workqueues. They 
call the HPD helpers from a threaded interrupt handler though. Sleeping in 
that context is fine, calling functions that might rely on interrupts from the 
same device to signal completion (such as reading EDID through .get_modes()) 
isn't.

> Nesting workqueues
> within workqueues seemed beyond silly, hence why I removed them in:
> 
> commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Tue Oct 23 18:23:34 2012 +0000
> 
>     drm: run the hpd irq event code directly
> 
> I guess we could talk about re-introducing a work-item based version of
> drm_helper_hpd_irq_event. But for drm_kms_helper_hotplug_event I think it
> doesn't make sense - if you call that you've probably just done a pile of
> i2c transactions, and those can sleep. If you haven't done i2c
> transactions, then it's not an external panel, and why exactly are you
> handling hpd for them?

-- 
Regards,

Laurent Pinchart

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[Laurent Pinchart]

Hi Daniel,

On Wednesday 23 Nov 2016 08:55:37 Daniel Vetter wrote:
> On Tue, Nov 22, 2016 at 08:23:38PM +0200, Laurent Pinchart wrote:
> > On Tuesday 22 Nov 2016 10:07:53 John Stultz wrote:
> >> On Tue, Nov 22, 2016 at 9:38 AM, John Stultz wrote:
> >>> Interestingly, without the msleep added in this patch, removing the
> >>> wait_event_interruptible_timeout() method in adv7511_wait_for_edid()
> >>> and using the polling loop seems to make things just as reliable. So
> >>> maybe something is off with the irq handling here instead?
> >> 
> >> Ahhhh.. So I think the trouble here is the that when we fail waiting
> >> for the irq, the backtrace is as follows:
> >> 
> >> [    8.318654] [<ffffff8008087c28>] dump_backtrace+0x0/0x1a0
> >> [    8.318661] [<ffffff8008087ddc>] show_stack+0x14/0x20
> >> [    8.318671] [<ffffff80084344f0>] dump_stack+0x90/0xb0
> >> [    8.318680] [<ffffff8008534650>] adv7511_get_edid_block+0x2c8/0x320
> >> [    8.318687] [<ffffff80085214a8>] drm_do_get_edid+0x78/0x280
> >> [    8.318693] [<ffffff8008534728>] adv7511_get_modes+0x80/0xd8
> >> [    8.318700] [<ffffff8008534794>]
> >> adv7511_connector_get_modes+0x14/0x20
> >> [    8.318710] [<ffffff8008500a54>]
> >> drm_helper_probe_single_connector_modes+0x2bc/0x500
> >> [    8.318718] [<ffffff800850e400>]
> >> drm_fb_helper_hotplug_event+0x130/0x188
> >> [    8.318726] [<ffffff800850ee68>]
> >> drm_fbdev_cma_hotplug_event+0x10/0x20
> >> [    8.318733] [<ffffff8008535718>]
> >> kirin_fbdev_output_poll_changed+0x20/0x58
> >> [    8.318740] [<ffffff8008500cc0>]
> >> drm_kms_helper_hotplug_event+0x28/0x38
> >> [    8.318748] [<ffffff80085010d8>] drm_helper_hpd_irq_event+0x138/0x180
> >> [    8.318754] [<ffffff8008533850>] adv7511_irq_process+0x78/0xd8
> >> [    8.318761] [<ffffff80085338c4>] adv7511_irq_handler+0x14/0x28
> >> [    8.318769] [<ffffff8008100060>] irq_thread_fn+0x28/0x68
> >> [    8.318775] [<ffffff8008100350>] irq_thread+0x128/0x1e8
> >> [    8.318782] [<ffffff80080d2e68>] kthread+0xd0/0xe8
> >> [    8.318788] [<ffffff8008082e80>] ret_from_fork+0x10/0x50
> >> 
> >> So we're actually in irq handling the hotplug interrupt, which is why
> >> we never get the irq notification when the edid is read.
> >> 
> >> I suspect we need to use a workqueue to do the hotplug handling out of
> >> irq.
> > 
> > Lovely :-)
> > 
> > Quoting the DRM documentation:
> > 
> > /**
> >  * drm_helper_hpd_irq_event - hotplug processing
> >  * @dev: drm_device
> >  *
> >  * Drivers can use this helper function to run a detect cycle on all
> > connectors
> >  * which have the DRM_CONNECTOR_POLL_HPD flag set in their &polled member.
> > All
> >  * other connectors are ignored, which is useful to avoid reprobing fixed
> >  * panels.
> >  *
> >  * This helper function is useful for drivers which can't or don't track
> > hotplug
> >  * interrupts for each connector.
> >  *
> >  * Drivers which support hotplug interrupts for each connector
> > individually and
> >  * which have a more fine-grained detect logic should bypass this code and
> >  * directly call drm_kms_helper_hotplug_event() in case the connector
> > state
> >  * changed.
> >  *
> >  * This function must be called from process context with no mode
> >  * setting locks held.
> >  *
> >  * Note that a connector can be both polled and probed from the hotplug
> > handler,
> >  * in case the hotplug interrupt is known to be unreliable.
> >  */
> > 
> > So it looks like we should use drm_kms_helper_hotplug_event() instead.
> > 
> > /**
> >  * drm_kms_helper_hotplug_event - fire off KMS hotplug events
> >  * @dev: drm_device whose connector state changed
> >  *
> >  * This function fires off the uevent for userspace and also calls the
> >  * output_poll_changed function, which is most commonly used to inform the
> > fbdev
> >  * emulation code and allow it to update the fbcon output configuration.
> >  *
> >  * Drivers should call this from their hotplug handling code when a change
> > is
> >  * detected. Note that this function does not do any output detection of
> > its
> >  * own, like drm_helper_hpd_irq_event() does - this is assumed to be done
> > by the
> >  * driver already.
> >  *
> >  * This function must be called from process context with no mode
> >  * setting locks held.
> >  */
> > 
> > The function suffers from the same problem though, that it must be called
> > from process context.
> > 
> > Daniel, why do we have an API the is clearly related to interrupt handling
> > but requires the caller to implement a workqueue ?
> 
> Because in general you need that workqueue anyway, and up to now there was
> no driver ever who didn't have a work-queue already.

None of the bridge drivers in drivers/gpu/drm/bridge/ have workqueues. They 
call the HPD helpers from a threaded interrupt handler though. Sleeping in 
that context is fine, calling functions that might rely on interrupts from the 
same device to signal completion (such as reading EDID through .get_modes()) 
isn't.

> Nesting workqueues
> within workqueues seemed beyond silly, hence why I removed them in:
> 
> commit 69787f7da6b2adc4054357a661aaa1701a9ca76f
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Tue Oct 23 18:23:34 2012 +0000
> 
>     drm: run the hpd irq event code directly
> 
> I guess we could talk about re-introducing a work-item based version of
> drm_helper_hpd_irq_event. But for drm_kms_helper_hotplug_event I think it
> doesn't make sense - if you call that you've probably just done a pile of
> i2c transactions, and those can sleep. If you haven't done i2c
> transactions, then it's not an external panel, and why exactly are you
> handling hpd for them?

-- 
Regards,

Laurent Pinchart

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

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[Daniel Vetter]

On Fri, Nov 25, 2016 at 1:23 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> > Daniel, why do we have an API the is clearly related to interrupt handling
>> > but requires the caller to implement a workqueue ?
>>
>> Because in general you need that workqueue anyway, and up to now there was
>> no driver ever who didn't have a work-queue already.
>
> None of the bridge drivers in drivers/gpu/drm/bridge/ have workqueues. They
> call the HPD helpers from a threaded interrupt handler though. Sleeping in
> that context is fine, calling functions that might rely on interrupts from the
> same device to signal completion (such as reading EDID through .get_modes())
> isn't.

Hm, as long as they all use the bit-banging interfaces they'll
probably be all fine. For everyone else you need multiple layers of
work items to make sure you never end up stalling in an interrupt vs.
holding-mode_config.mutex deadlock. So still not convinced we need
this ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on

[Daniel Vetter]

On Fri, Nov 25, 2016 at 1:23 AM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>> > Daniel, why do we have an API the is clearly related to interrupt handling
>> > but requires the caller to implement a workqueue ?
>>
>> Because in general you need that workqueue anyway, and up to now there was
>> no driver ever who didn't have a work-queue already.
>
> None of the bridge drivers in drivers/gpu/drm/bridge/ have workqueues. They
> call the HPD helpers from a threaded interrupt handler though. Sleeping in
> that context is fine, calling functions that might rely on interrupts from the
> same device to signal completion (such as reading EDID through .get_modes())
> isn't.

Hm, as long as they all use the bit-banging interfaces they'll
probably be all fine. For everyone else you need multiple layers of
work items to make sure you never end up stalling in an interrupt vs.
holding-mode_config.mutex deadlock. So still not convinced we need
this ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[RFC][PATCH 3/3] drm/bridge: adv7511: Enable HPD interrupts to support hotplug and improve monitor detection

[John Stultz]

From: Archit Taneja <architt@codeaurora.org>

On some adv7511 implementations, we can get some spurious
disconnect signals which can cause monitor probing to fail.

This patch enables HPD (hot plug detect) interrupt support
which allows the monitor to be properly re-initialized when
the spurious disconnect signal goes away.

This also enables proper hotplug support.

Cc: David Airlie <airlied@linux.ie>
Cc: Archit Taneja <architt@codeaurora.org>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
Cc: Lars-Peter Clausen <lars@metafoo.de>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: dri-devel@lists.freedesktop.org
Originally-by: Archit Taneja <architt@codeaurora.org>
[jstultz: Added proper commit message]
Signed-off-by: John Stultz <john.stultz@linaro.org>
---
 drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
index 2114a4c..889cf36 100644
--- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
+++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
@@ -338,7 +338,7 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
 		 * Still, let's be safe and stick to the documentation.
 		 */
 		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
-			     ADV7511_INT0_EDID_READY);
+			     ADV7511_INT0_EDID_READY | ADV7511_INT0_HPD);
 		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
 			     ADV7511_INT1_DDC_ERROR);
 	}
@@ -825,6 +825,10 @@ static int adv7511_bridge_attach(struct drm_bridge *bridge)
 	if (adv->type == ADV7533)
 		ret = adv7533_attach_dsi(adv);
 
+	if (adv->i2c_main->irq)
+		regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0),
+				ADV7511_INT0_HPD);
+
 	return ret;
 }
 
-- 
2.7.4

Re: [RFC][PATCH 3/3] drm/bridge: adv7511: Enable HPD interrupts to support hotplug and improve monitor detection

[Laurent Pinchart]

Hi John,

Thank you for the patch.

On Monday 21 Nov 2016 16:37:32 John Stultz wrote:
> From: Archit Taneja <architt@codeaurora.org>
> 
> On some adv7511 implementations, we can get some spurious
> disconnect signals which can cause monitor probing to fail.
> 
> This patch enables HPD (hot plug detect) interrupt support
> which allows the monitor to be properly re-initialized when
> the spurious disconnect signal goes away.
> 
> This also enables proper hotplug support.
> 
> Cc: David Airlie <airlied@linux.ie>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
> Originally-by: Archit Taneja <architt@codeaurora.org>
> [jstultz: Added proper commit message]
> Signed-off-by: John Stultz <john.stultz@linaro.org>

This looks good to me.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 2114a4c..889cf36
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -338,7 +338,7 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
>  		 * Still, let's be safe and stick to the documentation.
>  		 */
>  		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
> -			     ADV7511_INT0_EDID_READY);
> +			     ADV7511_INT0_EDID_READY | ADV7511_INT0_HPD);
>  		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
>  			     ADV7511_INT1_DDC_ERROR);
>  	}
> @@ -825,6 +825,10 @@ static int adv7511_bridge_attach(struct drm_bridge
> *bridge) if (adv->type == ADV7533)
>  		ret = adv7533_attach_dsi(adv);
> 
> +	if (adv->i2c_main->irq)
> +		regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0),
> +				ADV7511_INT0_HPD);
> +
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart

Re: [RFC][PATCH 3/3] drm/bridge: adv7511: Enable HPD interrupts to support hotplug and improve monitor detection

[Laurent Pinchart]

Hi John,

Thank you for the patch.

On Monday 21 Nov 2016 16:37:32 John Stultz wrote:
> From: Archit Taneja <architt@codeaurora.org>
> 
> On some adv7511 implementations, we can get some spurious
> disconnect signals which can cause monitor probing to fail.
> 
> This patch enables HPD (hot plug detect) interrupt support
> which allows the monitor to be properly re-initialized when
> the spurious disconnect signal goes away.
> 
> This also enables proper hotplug support.
> 
> Cc: David Airlie <airlied@linux.ie>
> Cc: Archit Taneja <architt@codeaurora.org>
> Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: Lars-Peter Clausen <lars@metafoo.de>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Cc: dri-devel@lists.freedesktop.org
> Originally-by: Archit Taneja <architt@codeaurora.org>
> [jstultz: Added proper commit message]
> Signed-off-by: John Stultz <john.stultz@linaro.org>

This looks good to me.

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/gpu/drm/bridge/adv7511/adv7511_drv.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c index 2114a4c..889cf36
> 100644
> --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c
> @@ -338,7 +338,7 @@ static void __adv7511_power_on(struct adv7511 *adv7511)
>  		 * Still, let's be safe and stick to the documentation.
>  		 */
>  		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(0),
> -			     ADV7511_INT0_EDID_READY);
> +			     ADV7511_INT0_EDID_READY | ADV7511_INT0_HPD);
>  		regmap_write(adv7511->regmap, ADV7511_REG_INT_ENABLE(1),
>  			     ADV7511_INT1_DDC_ERROR);
>  	}
> @@ -825,6 +825,10 @@ static int adv7511_bridge_attach(struct drm_bridge
> *bridge) if (adv->type == ADV7533)
>  		ret = adv7533_attach_dsi(adv);
> 
> +	if (adv->i2c_main->irq)
> +		regmap_write(adv->regmap, ADV7511_REG_INT_ENABLE(0),
> +				ADV7511_INT0_HPD);
> +
>  	return ret;
>  }

-- 
Regards,

Laurent Pinchart

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