Re: [PATCH] drm/bridge: sii902x: fix get edid may fail

[Jani Nikula <jani.nikula@linux.intel.com>]

On Mon, 23 Jan 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jan 23, 2017 at 03:06:36PM +0100, Andrea Merello wrote:
>> On Mon, Jan 23, 2017 at 1:36 PM, Boris Brezillon
>> <boris.brezillon@free-electrons.com> wrote:
>> > On Mon, 23 Jan 2017 13:12:12 +0100
>> > Andrea Merello <andrea.merello@gmail.com> wrote:
>> >
>> >> On Mon, Jan 23, 2017 at 12:32 PM, Jani Nikula
>> >> <jani.nikula@linux.intel.com> wrote:
>> >> > On Mon, 23 Jan 2017, Boris Brezillon <boris.brezillon@free-electrons.com> wrote:
>> >> >> Hi Andrea,
>> >> >>
>> >> >> On Mon, 23 Jan 2017 11:00:02 +0100
>> >> >> Andrea Merello <andrea.merello@gmail.com> wrote:
>> >> >>
>> >> >>> From: Andrea Merello <andrea.merello@gmail.com>
>> >> >>>
>> >> >>> The standard DRM function to get the edid from the i2c bus performs
>> >> >>> (at least) two transfers.
>> >> >>>
>> >> >>> By experiments it seems that the sii9022a have problems with the
>> >> >>> 2nd I2C start, at least unless a wait is introduced detween the
>> >> >>
>> >> >>                                                     ^ between
>> >> >>
>> >> >>> two transfers.
>> >> >>>
>> >> >>> So we perform one single I2C transfer, and if the transfer must be
>> >> >>> split, then we introduce a delay.
>> >> >>
>> >> >> That's not exactly what this patch does: you're introducing a delay
>> >> >> between each retry. So, if the transceiver really requires a delay
>> >> >> between each transfer, you'll have to retry at least once on the 2nd
>> >> >> transfer.
>> >> >>
>> >> >> I guess a better solution would be to add a delay even in case of
>> >> >> success, or maybe modify drm_do_get_edid() to optionally wait for a
>> >> >> specified time between each transfer.
>> >> >
>> >> > Is the problem related specifically to EDID reads, or generally to I2C
>> >> > transfers? Perhaps this should be fixed at the adapter master_xfer layer
>> >> > instead? Does the master_xfer perhaps return -EAGAIN, have you looked
>> >> > into i2c_adapter timeout member? (See __i2c_transfer in i2c-core.c.)
>> >> >
>> >> > The intention of drm_do_get_edid() is to facilitate *alternative*
>> >> > methods to doing I2C, not to workaround quirks like this.
>> >
>> > Yes, I tend to agree with that.
>> >
>> >>
>> >> This problem seems not related to the I2C master itself. I saw the I2C
>> >> master to TX correctly by looking at the I2C bus before the sii9022a
>> >> with the scope, and I saw the same I2C transfer to be corrupted after
>> >> the sii9022a; actually the sii9022a seems to gate the bus damaging the
>> >> I2C start of the 2nd transfer, unless we place this delay.
>> >>
>> >> This happened with two different I2C master (the one on the SoC as
>> >> well as another different one implemented on FPGA).
>> >
>> > Actually, I faced the same problem on an at91-based platform, and came
>> > up with pretty much the same fix (add a delay after each transfer),
>> > except mine was uglier and I never took the time to clean it up and
>> > submit it.
>> >
>> >>
>> >> So IMHO this quirk should be done in the sii902x dirver. Said that, I
>> >> admit that I've not looked at i2c_adapter timeout member or other
>> >> details in upper layers.
>> >
>> > Well, let's sum up the situation: the sii902x device is acting both as
>> > a regular i2c device and as an i2c 'pass-through' device. In the
>> > pass-through mode, the si902x has a new constraint: the i2c transfers
>> > have to be separated by an idle period.
>> >
>> > Maybe we should expose the pass-through mode as separate i2c adapter,
>> > that would be registered when entering pass-through mode and
>> > de-registered when leaving this mode.
>> > This way, we can implement the ->master_xfer() as we need (add a delay
>> > after each xfer), and still use the generic drm_get_edid().
>> >
>> > What do you think?
>> 
>> IMHO this could be a good way to go.. Although honestly the current
>> quirk doesn't seem so much bad to me :)
>
> I agree with Jani that using drm_do_get_edid is the wrong thing to do
> here, it's really for cases where you have a hw edid-fetch engine that
> entirely hides the underlying i2c bus. If you do have the i2c bus exposed,
> then the right place to fix it is at the i2c_adapter level. There's a pile
> of userspace tools that also probe the ddc i2c bus (there's not just an
> edid there), and if you don't fix this at the i2c_adapter level those
> would still be broken.
>
> Since this seems unclear, can you pls also do a patch for the kerneldoc
> for the drm_do_get_edid function, so it's clear when and when not this
> should be used?

IMO it's perfectly clear in the kernel-doc.

BR,
Jani.


>
> Thanks, Daniel

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel