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  wrote:
> On 11/23/2016 01:16 AM, John Stultz wrote:
>> On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart
>>  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  wrote:
> On 11/23/2016 01:16 AM, John Stultz wrote:
>> On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart
>>  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

[Archit Taneja]

On 11/23/2016 01:16 AM, John Stultz wrote:

On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart
 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);
+   

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
 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);
+   }

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
 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

[John Stultz]

On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart
 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

[John Stultz]

On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart
 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
 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

[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

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
 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
 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

[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 
> >> Cc: Archit Taneja 
> >> Cc: Wolfram Sang 
> >> Cc: Lars-Peter Clausen 
> >> Cc: Laurent Pinchart 
> >> Cc: dri-de...@lists.freedesktop.org
> >> Signed-off-by: John Stultz 
> >> ---
> >> 
> >>  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 
> >> Cc: Archit Taneja 
> >> Cc: Wolfram Sang 
> >> Cc: Lars-Peter Clausen 
> >> Cc: Laurent Pinchart 
> >> Cc: dri-de...@lists.freedesktop.org
> >> Signed-off-by: John Stultz 
> >> ---
> >> 
> >>  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

[John Stultz]

On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart
 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 
>> Cc: Archit Taneja 
>> Cc: Wolfram Sang 
>> Cc: Lars-Peter Clausen 
>> Cc: Laurent Pinchart 
>> Cc: dri-de...@lists.freedesktop.org
>> Signed-off-by: John Stultz 
>> ---
>>  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
 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 
>> Cc: Archit Taneja 
>> Cc: Wolfram Sang 
>> Cc: Lars-Peter Clausen 
>> Cc: Laurent Pinchart 
>> Cc: dri-de...@lists.freedesktop.org
>> Signed-off-by: John Stultz 
>> ---
>>  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

[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 
> Cc: Archit Taneja 
> Cc: Wolfram Sang 
> Cc: Lars-Peter Clausen 
> Cc: Laurent Pinchart 
> Cc: dri-de...@lists.freedesktop.org
> Signed-off-by: John Stultz 
> ---
>  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

[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 
> Cc: Archit Taneja 
> Cc: Wolfram Sang 
> Cc: Lars-Peter Clausen 
> Cc: Laurent Pinchart 
> Cc: dri-de...@lists.freedesktop.org
> Signed-off-by: John Stultz 
> ---
>  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