by Damian Kos

[permalink] [raw]

Subject: RE: [PATCH v4 5/5] drm: bridge: add support for Cadence MHDP DPI/DP bridge

Heiko,

> Hi Damian,
>
> Am Montag, 24. September 2018, 13:40:03 CEST schrieb Damian Kos:
> > > Am Donnerstag, 20. September 2018, 16:54:40 CEST schrieb Damian Kos:
> > > > From: Quentin Schulz <[email protected]>
> > > >
> > > > This adds basic support for Cadence MHDP DPI to DP bridge.
> > > >
> > > > Basically, it takes a DPI stream as input and output it encoded in
> > > > DP format. It's missing proper HPD, HDCP and currently supports
> > > > only SST mode.
> > > >
> > > > Changes made in the low level driver (cdn-dp-reg.*):
> > > > - moved it to from drivers/gpu/drm/rockchip to
> > > > drivers/gpu/drm/bridge/cdns-mhdp-common.*
> > > > - functions for sending/receiving commands are now public
> > > > - added functions for reading registers and link training
> > > > adjustment
> > > >
> > > > Changes made in RK's driver (cdn-dp-core.*):
> > > > - Moved audio_info and audio_pdev fields from cdn_dp_device to
> > > > cdns_mhdp_device structure.
> > > >
> > > > Signed-off-by: Quentin Schulz <[email protected]>
> > > > Signed-off-by: Damian Kos <[email protected]>
> > >
> > > [...]
> > >
> > > > diff --git a/drivers/gpu/drm/rockchip/Kconfig
> > > > b/drivers/gpu/drm/rockchip/Kconfig
> > > > index 0ccc76217ee4..129b0529f3e1 100644
> > > > --- a/drivers/gpu/drm/rockchip/Kconfig
> > > > +++ b/drivers/gpu/drm/rockchip/Kconfig
> > > > @@ -27,7 +27,9 @@ config ROCKCHIP_ANALOGIX_DP
> > > >
> > > > config ROCKCHIP_CDN_DP
> > > > bool "Rockchip cdn DP"
> > > > - depends on EXTCON=y || (EXTCON=m && DRM_ROCKCHIP=m)
> > > > + depends on DRM_ROCKCHIP=m
> > >
> > > Sorry, I wasn't fast enough in my reply to you mail to catch that before your v4, but I don't think this is necessary.
> > > Instead I do guess, the select below should do the right thing by making EXTCON=y if DRM_ROCKCHIP=y.
> > >
> > > Somewhat clumsily verified by making EXTCON=m in my defconfig and seeing get changed to y upon build, which I guess comes from a different "select" in the config.
> > >
> >
> > I've changed it to:
> >
> > config ROCKCHIP_CDN_DP
> > bool "Rockchip cdn DP"
> > depends on DRM_ROCKCHIP
> > select EXTCON if DRM_ROCKCHIP=y
> > select DRM_CDNS_MHDP
> > help...
> >
> > and it seems that there are no issues. At least for me.
> >
> > Please let me know if that's OK.
>
> nope, just do "select EXTCON" . It works nicely and the above would not select EXTCON at all if the the rockchip-drm driver gets built into the kernel.

That's weird. I'm pretty sure that I've tried that and got circular dependency error, but now it's working fine. Anyway:

config ROCKCHIP_CDN_DP
bool "Rockchip cdn DP"
depends on DRM_ROCKCHIP
select EXTCON
select DRM_CDNS_MHDP
help...

>
> To be sure I just did a little experiment and added:
>
> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig index 4ad85c046dcd..64e2b096afd8 100644
> --- a/drivers/gpu/drm/bridge/Kconfig
> +++ b/drivers/gpu/drm/bridge/Kconfig
> @@ -154,4 +154,11 @@ source "drivers/gpu/drm/bridge/ite/Kconfig"
>
> source "drivers/gpu/drm/bridge/synopsys/Kconfig"
>
> +config DRM_TMP1
> + tristate "option to select"
> +
> +config DRM_TMP2
> + tristate "option doing the selecting"
> + select DRM_TMP1
> +
>
> If I select DRM_TMP2=m as module I end up with CONFIG_DRM_TMP1=m CONFIG_DRM_TMP2=m in the defconfig and wit DRM_TMP2=y it changes to CONFIG_DRM_TMP1=y CONFIG_DRM_TMP2=y
>
> so all is well with just doing "select EXTCON" above and will select the correct "m" or "y" stance as needed.
>
>
> Heiko

--
Damian

2018-10-01 09:27:52

by Quentin Schulz

[permalink] [raw]

Subject: Re: [PATCH v4 2/5] drm/dp: fix link probing for devices supporting DP 1.4+

Hi Manasi,

On Fri, Sep 21, 2018 at 12:59:30AM -0700, Manasi Navare wrote:
> Thanks for the patch. Verified with the DP 1.4 spec and looks good to me.
> Also look at the related patch that makes use of the correct extended capabilities:
>
> https://patchwork.freedesktop.org/patch/249400/
>

If I'm not mistaken, you could get rid of the patch in the Intel driver
when we have this one merged. Is there any reason not to, have I missed
something?

Thanks,
Quentin

> Reviewed-by: Manasi Navare <[email protected]>
>
> Manasi
>
>
> On Thu, Sep 20, 2018 at 03:54:37PM +0100, Damian Kos wrote:
> > From: Quentin Schulz <[email protected]>
> >
> > DP 1.4 introduced a DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT bit in
> > DP_TRAINING_AUX_RD_INTERVAL register. If set, DPCD registers from
> > DP_DPCD_REV to DP_ADAPTER_CAP should be retrieved starting from
> > DP_DPCD_REV_EXTENDED. All registers are copied except DP_DPCD_REV,
> > DP_MAX_LINK_RATE and DP_DOWNSTREAMPORT_PRESENT which represent the
> > "true capabilities" of DPRX device.
> >
> > Original DP_DPCD_REV, DP_MAX_LINK_RATE and DP_DOWNSTREAMPORT_PRESENT
> > might falsely return lower capabilities to "avoid interoperability
> > issues with some of the existing DP Source devices that malfunction
> > when they discover the higher capabilities within those three
> > registers.".
> >
> > Before DP 1.4, DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT bit was reserved
> > and read 0 so it's safe to check against it even if DP revision is
> > <1.4
> >
> > Signed-off-by: Quentin Schulz <[email protected]>
> > Signed-off-by: Damian Kos <[email protected]>
> > ---
> > drivers/gpu/drm/drm_dp_helper.c | 30 +++++++++++++++++++++++++++++-
> > 1 file changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > index 8c6b9fd89f8a..735ebde5c2f0 100644
> > --- a/drivers/gpu/drm/drm_dp_helper.c
> > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > @@ -370,10 +370,38 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
> > {
> > u8 values[3];
> > int err;
> > + unsigned int addr;
> >
> > memset(link, 0, sizeof(*link));
> >
> > - err = drm_dp_dpcd_read(aux, DP_DPCD_REV, values, sizeof(values));
> > + /*
> > + * DP 1.4 introduced a DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT bit in
> > + * DP_TRAINING_AUX_RD_INTERVAL register. If set, DPCD registers from
> > + * DP_DPCD_REV to DP_ADAPTER_CAP should be retrieved starting from
> > + * DP_DPCD_REV_EXTENDED. All registers are copied except DP_DPCD_REV,
> > + * DP_MAX_LINK_RATE and DP_DOWNSTREAMPORT_PRESENT which represent the
> > + * "true capabilities" of DPRX device.
> > + *
> > + * Original DP_DPCD_REV, DP_MAX_LINK_RATE and DP_DOWNSTREAMPORT_PRESENT
> > + * might falsely return lower capabilities to "avoid interoperability
> > + * issues with some of the existing DP Source devices that malfunction
> > + * when they discover the higher capabilities within those three
> > + * registers.".
> > + *
> > + * Before DP 1.4, DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT bit was reserved
> > + * and read 0 so it's safe to check against it even if DP revision is
> > + * <1.4
> > + */
> > + err = drm_dp_dpcd_readb(aux, DP_TRAINING_AUX_RD_INTERVAL, values);
> > + if (err < 0)
> > + return err;
> > +
> > + if (values[0] & DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT)
> > + addr = DP_DP13_DPCD_REV;
> > + else
> > + addr = DP_DPCD_REV;
> > +
> > + err = drm_dp_dpcd_read(aux, addr, values, sizeof(values));
> > if (err < 0)
> > return err;
> >
> > --
> > 2.17.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > [email protected]
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel

Attachments:

(No filename) (3.81 kB)
signature.asc (849.00 B)
Download all attachments

2018-10-02 14:07:03

by Ville Syrjälä

[permalink] [raw]

Subject: Re: [PATCH v4 2/5] drm/dp: fix link probing for devices supporting DP 1.4+

On Mon, Oct 01, 2018 at 11:27:08AM +0200, Quentin Schulz wrote:
> Hi Manasi,
>
> On Fri, Sep 21, 2018 at 12:59:30AM -0700, Manasi Navare wrote:
> > Thanks for the patch. Verified with the DP 1.4 spec and looks good to me.
> > Also look at the related patch that makes use of the correct extended capabilities:
> >
> > https://patchwork.freedesktop.org/patch/249400/
> >
>
> If I'm not mistaken, you could get rid of the patch in the Intel driver
> when we have this one merged. Is there any reason not to, have I missed
> something?

i915 (and most other drivers) don't use this code. Also as mentioned in
that thread you should probably validate that the data in the extended
caps looks sane, otherwise the entire point of having it for compatibility
with older source devices is lost.

>
> Thanks,
> Quentin
>
> > Reviewed-by: Manasi Navare <[email protected]>
> >
> > Manasi
> >
> >
> > On Thu, Sep 20, 2018 at 03:54:37PM +0100, Damian Kos wrote:
> > > From: Quentin Schulz <[email protected]>
> > >
> > > DP 1.4 introduced a DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT bit in
> > > DP_TRAINING_AUX_RD_INTERVAL register. If set, DPCD registers from
> > > DP_DPCD_REV to DP_ADAPTER_CAP should be retrieved starting from
> > > DP_DPCD_REV_EXTENDED. All registers are copied except DP_DPCD_REV,
> > > DP_MAX_LINK_RATE and DP_DOWNSTREAMPORT_PRESENT which represent the
> > > "true capabilities" of DPRX device.
> > >
> > > Original DP_DPCD_REV, DP_MAX_LINK_RATE and DP_DOWNSTREAMPORT_PRESENT
> > > might falsely return lower capabilities to "avoid interoperability
> > > issues with some of the existing DP Source devices that malfunction
> > > when they discover the higher capabilities within those three
> > > registers.".
> > >
> > > Before DP 1.4, DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT bit was reserved
> > > and read 0 so it's safe to check against it even if DP revision is
> > > <1.4
> > >
> > > Signed-off-by: Quentin Schulz <[email protected]>
> > > Signed-off-by: Damian Kos <[email protected]>
> > > ---
> > > drivers/gpu/drm/drm_dp_helper.c | 30 +++++++++++++++++++++++++++++-
> > > 1 file changed, 29 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> > > index 8c6b9fd89f8a..735ebde5c2f0 100644
> > > --- a/drivers/gpu/drm/drm_dp_helper.c
> > > +++ b/drivers/gpu/drm/drm_dp_helper.c
> > > @@ -370,10 +370,38 @@ int drm_dp_link_probe(struct drm_dp_aux *aux, struct drm_dp_link *link)
> > > {
> > > u8 values[3];
> > > int err;
> > > + unsigned int addr;
> > >
> > > memset(link, 0, sizeof(*link));
> > >
> > > - err = drm_dp_dpcd_read(aux, DP_DPCD_REV, values, sizeof(values));
> > > + /*
> > > + * DP 1.4 introduced a DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT bit in
> > > + * DP_TRAINING_AUX_RD_INTERVAL register. If set, DPCD registers from
> > > + * DP_DPCD_REV to DP_ADAPTER_CAP should be retrieved starting from
> > > + * DP_DPCD_REV_EXTENDED. All registers are copied except DP_DPCD_REV,
> > > + * DP_MAX_LINK_RATE and DP_DOWNSTREAMPORT_PRESENT which represent the
> > > + * "true capabilities" of DPRX device.
> > > + *
> > > + * Original DP_DPCD_REV, DP_MAX_LINK_RATE and DP_DOWNSTREAMPORT_PRESENT
> > > + * might falsely return lower capabilities to "avoid interoperability
> > > + * issues with some of the existing DP Source devices that malfunction
> > > + * when they discover the higher capabilities within those three
> > > + * registers.".
> > > + *
> > > + * Before DP 1.4, DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT bit was reserved
> > > + * and read 0 so it's safe to check against it even if DP revision is
> > > + * <1.4
> > > + */
> > > + err = drm_dp_dpcd_readb(aux, DP_TRAINING_AUX_RD_INTERVAL, values);
> > > + if (err < 0)
> > > + return err;
> > > +
> > > + if (values[0] & DP_EXTENDED_RECEIVER_CAP_FIELD_PRESENT)
> > > + addr = DP_DP13_DPCD_REV;
> > > + else
> > > + addr = DP_DPCD_REV;
> > > +
> > > + err = drm_dp_dpcd_read(aux, addr, values, sizeof(values));
> > > if (err < 0)
> > > return err;
> > >
> > > --
> > > 2.17.1
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > [email protected]
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel

> _______________________________________________
> dri-devel mailing list
> [email protected]
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

--
Ville Syrj?l?
Intel