+Philipp On Wed, Jun 22, 2016 at 11:54 AM, Archit Taneja <architt@xxxxxxxxxxxxxx> wrote: > > > On 6/22/2016 8:14 AM, Nicolas Boichat wrote: >> >> Hi Archit, >> >> Thanks for your reply. >> >> On Tue, Jun 21, 2016 at 11:39 PM, Archit Taneja <architt@xxxxxxxxxxxxxx> >> wrote: >>> >>> Hi, >>> >>> On 6/20/2016 12:44 PM, Nicolas Boichat wrote: >>>> >>>> >>>> ANX7688 is a HDMI to DP converter (as well as USB-C port controller), >>>> that has an internal microcontroller. >>>> >>>> The only reason a Linux kernel driver is necessary is to reject >>>> resolutions that require more bandwidth than what is available on >>>> the DP side. DP bandwidth and lane count are reported by the bridge >>>> via 2 registers on I2C. >>> >>> >>> >>> How does the chip know when to enable/disable itself? Does it shutoff >>> itself if there isn't anything on the HDMI link? >> >> >> Not 100% sure of the internals (there is a closed source firmware in >> the chip), but I believe the HDMI/DP part of the chip is switched on >> whenever there is a DP over USB-C connector plugged in. >> >>>> >>>> Signed-off-by: Nicolas Boichat <drinkcat@xxxxxxxxxxxx> >>>> --- >>>> >>>> Hi, >>>> >>>> I tested this driver using the Mediatek HDMI controller (MT8173) >>>> upstream >>>> of the ANX bridge chip (Phillip sent a PULL request on June 13: >>>> git://git.pengutronix.de/git/pza/linux.git tags/mediatek-drm-2016-06-13 >>>> ). >>>> >>>> I have 2 concerns, that I'm not sure how to address within the kernel >>>> DRM >>>> framework: >>>> 1. All other bridge drivers also have a connector attached to it. >>>> However, in >>>> this case, we cannot read the monitor EDID directly, so I'm not >>>> sure >>>> what >>>> I could put in a "get_modes" function. Instead, ANX7688 provides a >>>> I2C >>>> passthru/repeater, so the EDID is read on the Mediatek HDMI >>>> controller side. >>>> >>>> That leads to a somewhat strange layout, where we have: >>>> - MTK HDMI bridge/encoder >>>> - MTK connector (HDMI) >>>> - ANX7688 bridge >>>> - No connector >>> >>> >>> >>> >>> You should ideally have one DP connector at the end of the chain: >>> >>> - MTK HDMI bridge/encoder >>> - ANX7688 bridge >>> - Connector (DP) >>> >>> In the dt-bindings for this board, hdmi's output port shouldn't be >>> connected to a hdmi connector, but the input port of the ANX7688 >>> DP bridge. The output port of the bridge should be the one that >>> connects to the DP connector. >> >> >> Yes that's what I do (in the device tree). >> >> Actually, experimenting a bit more with the code, I realized that the >> connector is always attached to the encoder, not the bridge, so the 2 >> layouts above are actually identical (from the userspace point of >> view). Except that the connector name should be HDMI in one case, and >> DP in the other. But I think that's mostly a cosmetic difference? > > > Yeah, probably. I don't know what exactly the userspace does with > the connector type, but it would be nice to represent it as a DP > connector in case it makes any decisions based on it. AFAICT does not matter... And, in any case, USB-C to HDMI adapters are far more common (so we'd do HDMI->(DP over USB-C)->HDMI....), so there is a high change that advertising as DP would be wrong (and advertising as HDMI would be correct by chance...). >> >>> One way I can think of fixing this is to make make the MTK hdmi >>> encoder driver a bit smarter by observing the DT connections. If >>> it's output port is connected to just a hdmi-connector, then >>> things should be as before. If the output is connected to the DP >>> bridge, then it should create a DP connector. The connector ops >>> for the DP connector can still be the same as that of the HDMI >>> connector before, but the phandle to the DDC bus would be in the >>> DP device node in this case. >> >> >> I think it'd be a bit weird to have the DDC bus phandle on the DP >> connector, as we're not reading the EDID on the DP side of the bridge, >> but on the HDMI side (and the bridge can do all sort of things to the >> EDID: At the very least, I think it caches it). > > > On the board, do the DDC lines join directly from the SoC pins to the > DP connector, or does it go via the ANX7688 chip? The DDC/I2C lines go to the ANX7688 chip. (DP has AUX channel instead) > Even with the current bindings (referred from the link below), the > hdmi connector has the DDC node. Shouldn't it be the same in the DP > case too? The DP connector, like before, is still manged by the HDMI > driver, the only difference being the name and that it's two hops away > in the DT bindings. > > https://patchwork.kernel.org/patch/9137089/ True, the bindings say so, but the current code actually looks for ddc-i2c-bus property in whatever is connected on the endpoint (be it a bridge or a connector). And again, a bit odd as DP connector does not have true I2C lines... Phillip? Any opinion? >> >>> This way, you can get around having the correct layout. >>> >>> Ideally, a bridge driver shouldn't be the one that creates a >>> connector. It may contain some of the connector functionality, but >>> the connector creation should be managed by the kms driver. >>> Almost all bridge drivers creating a connector in their .attach >>> callbacks since they own some of the connector functionality (like >>> reading EDID). That's something we're trying to fix by providing >>> some more bridge api that kms drivers can use. >>> >>> Since this bridge driver doesn't have any connector functionality >>> anyway, you should be okay. >> >> >> Great, thanks for clarifying. >> >>>> >>>> Resolution filtering works fine though, thanks to mode_fixup >>>> callback >>>> on the >>>> bridge. It also helps that Mediatek HDMI bridge calls mode_fixup >>>> from >>>> its >>>> connector mode_valid callback, so that invalid modes are not even >>>> presented >>>> to userspace. >>>> >>>> 2. In the bandwidth computation, I hard-code 8-bit per channel (bpc). >>>> bpc does >>>> not seem to be included in the mode setting itself. We could >>>> possibly >>>> iterate >>>> over connectors on the DRM device, but then, IIUC, >>>> connector->display_info.bpc >>>> indicates the _maximum_ bpc supported by the monitor. >>> >>> >>> >>> I'm not clear about this either. Some drivers set a bus format >>> on the connector via drm_display_info_set_bus_formats in their >>> get_modes connector op, and then retrieve it later. >> >> >> Ah, interesting... Seems like we'd need a big switch/case to convert >> from bus_format to bpp, though. >> >>>> >>>> Any pointers? Thanks! >>>> >>>> Best, >>>> >>>> Nicolas >>>> >>>> drivers/gpu/drm/bridge/Kconfig | 9 ++ >>>> drivers/gpu/drm/bridge/Makefile | 1 + >>>> drivers/gpu/drm/bridge/analogix-anx7688.c | 227 >>>> ++++++++++++++++++++++++++++++ >>>> 3 files changed, 237 insertions(+) >>>> create mode 100644 drivers/gpu/drm/bridge/analogix-anx7688.c >>>> >>>> diff --git a/drivers/gpu/drm/bridge/Kconfig >>>> b/drivers/gpu/drm/bridge/Kconfig >>>> index 8f7423f..0c1eb41 100644 >>>> --- a/drivers/gpu/drm/bridge/Kconfig >>>> +++ b/drivers/gpu/drm/bridge/Kconfig >>>> @@ -7,6 +7,15 @@ config DRM_BRIDGE >>>> menu "Display Interface Bridges" >>>> depends on DRM && DRM_BRIDGE >>>> >>>> +config DRM_ANALOGIX_ANX7688 >>>> + tristate "Analogix ANX7688 bridge" >>>> + depends on DRM >>>> + select DRM_KMS_HELPER >>>> + ---help--- >>>> + ANX7688 is a transmitter to support DisplayPort over USB-C for >>>> + smartphone and tablets. >>>> + This driver only supports the HDMI to DP component of the >>>> chip. >>>> + >>>> config DRM_ANALOGIX_ANX78XX >>>> tristate "Analogix ANX78XX bridge" >>>> select DRM_KMS_HELPER >>>> diff --git a/drivers/gpu/drm/bridge/Makefile >>>> b/drivers/gpu/drm/bridge/Makefile >>>> index 96b13b3..d744c6c 100644 >>>> --- a/drivers/gpu/drm/bridge/Makefile >>>> +++ b/drivers/gpu/drm/bridge/Makefile >>>> @@ -1,5 +1,6 @@ >>>> ccflags-y := -Iinclude/drm >>>> >>>> +obj-$(CONFIG_DRM_ANALOGIX_ANX7688) += analogix-anx7688.o >>>> obj-$(CONFIG_DRM_ANALOGIX_ANX78XX) += analogix-anx78xx.o >>>> obj-$(CONFIG_DRM_DW_HDMI) += dw-hdmi.o >>>> obj-$(CONFIG_DRM_DW_HDMI_AHB_AUDIO) += dw-hdmi-ahb-audio.o >>>> diff --git a/drivers/gpu/drm/bridge/analogix-anx7688.c >>>> b/drivers/gpu/drm/bridge/analogix-anx7688.c >>>> new file mode 100644 >>>> index 0000000..2c34029 >>>> --- /dev/null >>>> +++ b/drivers/gpu/drm/bridge/analogix-anx7688.c >>>> @@ -0,0 +1,227 @@ >>>> +/* >>>> + * ANX7688 HDMI->DP bridge driver >>>> + * >>>> + * Copyright (C) 2016 Google, Inc. >>>> + * >>>> + * This software is licensed under the terms of the GNU General Public >>>> + * License version 2, as published by the Free Software Foundation, and >>>> + * may be copied, distributed, and modified under those terms. >>>> + * >>>> + * This program is distributed in the hope that it will be useful, >>>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of >>>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the >>>> + * GNU General Public License for more details. >>>> + */ >>>> + >>>> +#include <linux/delay.h> >>>> +#include <linux/gpio.h> >>>> +#include <linux/i2c.h> >>>> +#include <linux/module.h> >>>> +#include <linux/of.h> >>>> +#include <linux/of_gpio.h> >>>> +#include <drm/drmP.h> >>> >>> >>> >>> The 3 headers above aren't needed. >> >> >> Nor gpio.h. Removed. >> >>> >>>> +#include <drm/drm_crtc.h> >>>> + >>>> +/* Register addresses */ >>>> +#define VENDOR_ID_REG 0x00 >>>> +#define DEVICE_ID_REG 0x02 >>>> + >>>> +#define FW_VERSION_REG 0x80 >>>> + >>>> +#define DP_BANDWIDTH_REG 0x85 >>>> +#define DP_LANE_COUNT_REG 0x86 >>>> + >>>> +#define VENDOR_ID 0x1f29 >>>> +#define DEVICE_ID 0x7688 >>>> + >>>> +/* First supported firmware version (0.85) */ >>>> +#define MINIMUM_FW_VERSION 0x0085 >>>> + >>>> +struct anx7688 { >>>> + struct drm_bridge bridge; >>>> + struct i2c_client *client; >>>> + >>>> + bool filter; >>>> +}; >>>> + >>>> +static int anx7688_read(struct i2c_client *client, u8 reg, u8 *data, >>>> + u16 data_len) >>>> +{ >>>> + int ret; >>>> + struct i2c_msg msgs[] = { >>>> + { >>>> + .addr = client->addr, >>>> + .flags = 0, >>>> + .len = 1, >>>> + .buf = ®, >>>> + }, >>>> + { >>>> + .addr = client->addr, >>>> + .flags = I2C_M_RD, >>>> + .len = data_len, >>>> + .buf = data, >>>> + } >>>> + }; >>>> + >>>> + ret = i2c_transfer(client->adapter, msgs, 2); >>>> + >>>> + if (ret == 2) >>>> + return 0; >>>> + if (ret < 0) >>>> + return ret; >>>> + else >>>> + return -EIO; >>>> +} >>>> + >>>> +static inline struct anx7688 *bridge_to_anx7688(struct drm_bridge >>>> *bridge) >>>> +{ >>>> + return container_of(bridge, struct anx7688, bridge); >>>> +} >>>> + >>>> +static bool anx7688_bridge_mode_fixup(struct drm_bridge *bridge, >>>> + const struct drm_display_mode >>>> *mode, >>>> + struct drm_display_mode >>>> *adjusted_mode) >>>> +{ >>>> + struct anx7688 *anx7688 = bridge_to_anx7688(bridge); >>>> + u8 regs[2]; >>>> + int totalbw, requiredbw; >>> >>> >>> >>> It might make sense to use a u32 or long or something here to prevent >>> risk of overflow. >> >> >> Well, mode->clock is already an int (and there won't be any overflow >> in requiredbw unless we go for THz clocks ,-)) > > > Ah okay, missed that. > >> >> totalbw could do with a bit more of sanity checking (e.g. check that >> regs[0] * regs[1] is not absurd). Like, we know regs[1] <= 2 on this >> chip. Will fix. >> >>>> + int ret; >>>> + >>>> + if (!anx7688->filter) >>>> + return true; >>>> + >>>> + /* Read both regs 0x85 (bandwidth) and 0x86 (lane count). */ >>>> + ret = anx7688_read(anx7688->client, DP_BANDWIDTH_REG, regs, 2); >>> >>> >>> >>> Who programmed these registers in the first place? Is the lane count >>> some sort of reset value? Or is it something that changes dynamically? >> >> >> The ANX7688 OCM (on-chip microcontroller) sets it. It changes >> depending on the downstream (DP) bandwidth/lane count, so it changes >> on each plug event (after DP link training presumably). > > > Okay. Makes sense, then. > > Thanks, > Archit > > > -- > Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, > a Linux Foundation Collaborative Project _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
