Message-ID: <CANMq1KAZhV2FKNVyqOMs0K3ipjSQ=7CW47v4UPDdN2UYgTaVdQ@mail.gmail.com>
Date:	Wed, 22 Jun 2016 14:32:22 +0800
From:	Nicolas Boichat <drinkcat@...omium.org>
To:	Archit Taneja <architt@...eaurora.org>
Cc:	dri-devel@...ts.freedesktop.org,
	Stéphane Marchesin <marcheu@...omium.org>,
	linux-kernel@...r.kernel.org,
	Enric Balletbo i Serra <enric.balletbo@...labora.com>,
	Russell King <rmk+kernel@....linux.org.uk>,
	Thierry Reding <treding@...dia.com>, p.zabel@...gutronix.de
Subject: Re: [RFC PATCH 1/2] drm: bridge: anx7688: Add anx7688 bridge driver support.

+Philipp

On Wed, Jun 22, 2016 at 11:54 AM, Archit Taneja <architt@...eaurora.org> 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@...eaurora.org>
>> 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@...omium.org>
>>>> ---
>>>>
>>>> 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 = &reg,
>>>> +                },
>>>> +               {
>>>> +                .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

Powered by blists - more mailing lists