On Mon, Nov 24, 2014 at 3:29 PM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote: > On 11/24/2014 09:01 PM, Dave Airlie wrote: >> >> On 25 November 2014 at 00:18, Lars-Peter Clausen <lars@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel
