Re: [PATCH 5/5 v3] drm/bridge: adv7511: Reuse __adv7511_power_on/off() when probing EDID

From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: John Stultz <john.stultz@linaro.org>
Cc: "dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	lkml <linux-kernel@vger.kernel.org>,
	Wolfram Sang <wsa+renesas@sang-engineering.com>
Subject: Re: [PATCH 5/5 v3] drm/bridge: adv7511: Reuse __adv7511_power_on/off() when probing EDID
Date: Tue, 17 Jan 2017 00:25:31 +0200	[thread overview]
Message-ID: <1668382.5d9jBvLc1l@avalon> (raw)
In-Reply-To: <CALAqxLVN=-9eUFaYWtu_MxTixfHZt_gRvWO7TDDFeDaDQ-uFZg@mail.gmail.com>

Hi John,

On Monday 16 Jan 2017 12:14:48 John Stultz wrote:
> On Mon, Jan 16, 2017 at 8:03 AM, Laurent Pinchart wrote:
> > On Tuesday 03 Jan 2017 11:41:42 John Stultz wrote:
> >> I've found that by just turning the chip on and off via the
> >> POWER_DOWN register, I end up getting i2c_transfer errors
> >> on HiKey.
> >> 
> >> Investigating further, it seems some of the register state
> >> in the regmap cache is somehow getting lost. Using the logic
> >> in __adv7511_power_on/off() which syncs and dirtys the cache
> >> avoids this issue.
> >> 
> >> Thus this patch changes the EDID probing logic so that we
> >> re-use the __adv7511_power_on/off() calls.
> > 
> > regcache_sync() is quite costly as it will write a bunch of registers.
> > Wouldn't it be more efficient to only write the registers that are needed
> > for EDID access ?
> 
> So yes, you've mentioned this concern before, and I did spend some
> time to narrow which lost-register state (0x43
>  - ADV7511_REG_EDID_I2C_ADDR) was causing the trouble with i2c
> trasnfer errors I was seeing:
>   https://lkml.org/lkml/2016/11/22/677
> 
> However, I didn't get much feedback on that, and it seems (to me at
> least) concerning that we are losing the underlying state of a
> register in the cache, so just syncing that one register back to the
> hardware might solve the issue I was seeing, but I worry what other
> registers might also be out of sync.
>
> The comment above the regmap_sync in adv7511_power_on after all states:
>    "Most of the registers are reset during power down or when HPD is low."

You're right that most registers will be out of sync.

> So it seems like if we're setting the power down (and setting HPD in
> cases where Archit had a patch to add HPD pulsing to the
> adv7511_get_modes path), it seems reasonable to do the same
> regmap_sync()?

It would be if we had to keep the device powered up, but we're powering it 
down right after reading the EDID. I don't think there's a need to reconfigure 
it completely, only setting the registers needed to read the EDID should be 
enough.

> But, I'm not really picky here, and I'm very open to other approaches
> (including something like the patch in the link above) if you have
> suggestions/preferences. I just want it to work reliably on my
> hardware. :)
>
> And just so I can better understand it, can you explain some about the
> impact of your efficiency concerns?

I'm not too picky either :-) If we can't find a reliable way to read the EDID 
by just configuring the registers we need, we could go for a full 
reconfiguration. However, restoring the value of all cached registers will 
result in lots of I2C writes, which are time-consuming operations. EDID read 
would be sped up if we could avoid that.

-- 
Regards,

Laurent Pinchart