[GIT PULL FOR v3.19] R-Car DU changes

[Rob Clark]

On Mon, Nov 24, 2014 at 3:29 PM, Lars-Peter Clausen <lars at metafoo.de> wrote:
> On 11/24/2014 09:01 PM, Dave Airlie wrote:
>>
>> On 25 November 2014 at 00:18, Lars-Peter Clausen <lars at metafoo.de> wrote:
>>>
>>> On 11/24/2014 03:00 PM, Laurent Pinchart wrote:
>>>>
>>>>
>>>> Hi Daniel,
>>>>
>>>> (CC'ing Rob Clark and Lars-Peter. As a reminder we're discussing the
>>>> "drm:
>>>> Decouple EDID parsing from I2C adapter" patch available at
>>>> git://linuxtv.org/pinchartl/fbdev.git drm/next/du)
>>>>
>>>> On Monday 24 November 2014 14:09:39 Daniel Vetter wrote:
>>>>>
>>>>>
>>>>> On Mon, Nov 24, 2014 at 11:46:18AM +0200, Laurent Pinchart wrote:
>>>>>>>
>>>>>>>
>>>>>>> - the interface looks rather backwards: Either this still does i2c
>>>>>>>     reads, and then you'd just need a i2c-over-whatever adapter to
>>>>>>> make
>>>>>>> it
>>>>>>>     work. Or you have other magic means to optain an edid block, in
>>>>>>> which
>>>>>>>     case just do that and then feed the edid drm_add_edid_modes.
>>>>>>
>>>>>>
>>>>>>
>>>>>> I have a magic way to get EDID over I2C :-) Basically the ADV7511
>>>>>> controls
>>>>>> the DDC bus, and exposes EDID data over I2C using vendor commands. To
>>>>>> read an EDID block I have to write an ADV7511 register over I2C with
>>>>>> the
>>>>>> block number, wait for an interrupt, read a status register to check
>>>>>> whether EDID data is available or whether an error occurred, and then
>>>>>> read EDID data from the ADV7511 over I2C in 64-bytes chunks. This
>>>>>> needs
>>>>>> to be repeated for every block. I thus can't use drm_get_edid()
>>>>>> directly.
>>>>>
>>>>>
>>>>>
>>>>> Sounds familiar. See the special ddc-over-sdvo i2c bus we register in
>>>>> intel_sdvo.c, specifically look at intel_sdvo_init_ddc_proxy. It is a
>>>>> bit
>>>>> of boilerplate, but in the end just amounts to 3 small functions and
>>>>> one
>>>>> tiny vtable to wire it all up cleanly.
>>>>
>>>>
>>>>
>>>> That's what I would have done as well if I had a device-specific I2C
>>>> adapter
>>>> connected to the DDC bus, but in this case the interface exposed by the
>>>> ADV7511 to the SoC over I2C consists of higher level device-specific I2C
>>>> commands to read EDID data. There is no low-level I2C read/write
>>>> primitives
>>>> available. I would thus need to expose a fake adapter that would receive
>>>> I2C
>>>> commands, parse them to detect an EDID block read, retrieve the EDID
>>>> data
>>>> and
>>>> return them from the fake read. That doesn't make much sense to me.
>>>
>>>
>>>
>>> The intel sdvo looks just like a simple I2C mux which will just
>>> pass-through
>>> messages from the master to the EDID EEPROM. The ADV7511 is unfortunately
>>> a
>>> bit different. You tell it to fetch the EDID information, then it will do
>>> some magic and then you can read the EDID back. Abstracting this as a
>>> this
>>> as a I2C controller will, while possible, result in a fair amount of
>>> boiler
>>> plate code that will not look particularly pretty.
>>
>>
>> It sounds also a bit like DP auxch also, or even how on UDL we get the
>> edid
>> over USB.
>>
>> I'd rather see not pretty code that only one person had to look at though
>> :-)
>> with lots of comments on the hw design that demands ugly.
>
>
> I'd rather not see that if I'm the person who has to look at it ;)
>
> But looking around it appears as if this is a more common thing among
> external HDMI encoders though. E.g. the tda998x has the exact same issue and
> the current driver just copy&pastes drm_do_get_edid() and plugs in its own
> EDID block retrieval method.
>
> On the other hand the patch that makes it possible to use a different
> backend then a raw I2C adapter to fetch the EDID is extremely trivial and
> doesn't make the existing code more complex in any way.
>

I suppose the only real disadvantage of the patch is it gives driver
writers a bad alternative in cases where they really should expose
their ddc as an i2c_adapter.. but perhaps we should just handle that
via sufficient review ;-)

(I have seen enough downstream drivers that could use i2c_adapter but do not..)

BR,
-R

> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel