[PATCH] soc: qcom: pmic_glink: fix connector type to be DisplayPort

[PATCH] soc: qcom: pmic_glink: fix connector type to be DisplayPort

[Dmitry Baryshkov]

As it was pointed out by Simon Ser, the DRM_MODE_CONNECTOR_USB connector
is reserved for the GUD devices. Other drivers (i915, amdgpu) use
DRM_MODE_CONNECTOR_DisplayPort even if the DP stream is handled by the
USB-C altmode. While we are still working on implementing the proper way
to let userspace know that the DP is wrapped into USB-C, change
connector type to be DRM_MODE_CONNECTOR_DisplayPort.

Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
Cc: Simon Ser <contact@emersion.fr>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
---
 drivers/soc/qcom/pmic_glink_altmode.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
index 9569d999391d..6f8b2f7ae3cc 100644
--- a/drivers/soc/qcom/pmic_glink_altmode.c
+++ b/drivers/soc/qcom/pmic_glink_altmode.c
@@ -467,7 +467,7 @@ static int pmic_glink_altmode_probe(struct auxiliary_device *adev,
 		alt_port->bridge.funcs = &pmic_glink_altmode_bridge_funcs;
 		alt_port->bridge.of_node = to_of_node(fwnode);
 		alt_port->bridge.ops = DRM_BRIDGE_OP_HPD;
-		alt_port->bridge.type = DRM_MODE_CONNECTOR_USB;
+		alt_port->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
 
 		ret = devm_drm_bridge_add(dev, &alt_port->bridge);
 		if (ret) {
-- 
2.39.2

Re: [PATCH] soc: qcom: pmic_glink: fix connector type to be DisplayPort

[Simon Ser]

Thanks for fixing this!

Acked-by: Simon Ser <contact@emersion.fr>

Re: [PATCH] soc: qcom: pmic_glink: fix connector type to be DisplayPort

[Neil Armstrong]

On 11/10/2023 00:52, Dmitry Baryshkov wrote:
> As it was pointed out by Simon Ser, the DRM_MODE_CONNECTOR_USB connector
> is reserved for the GUD devices. Other drivers (i915, amdgpu) use
> DRM_MODE_CONNECTOR_DisplayPort even if the DP stream is handled by the
> USB-C altmode. While we are still working on implementing the proper way
> to let userspace know that the DP is wrapped into USB-C, change
> connector type to be DRM_MODE_CONNECTOR_DisplayPort.
> 
> Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
> Cc: Simon Ser <contact@emersion.fr>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>   drivers/soc/qcom/pmic_glink_altmode.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
> index 9569d999391d..6f8b2f7ae3cc 100644
> --- a/drivers/soc/qcom/pmic_glink_altmode.c
> +++ b/drivers/soc/qcom/pmic_glink_altmode.c
> @@ -467,7 +467,7 @@ static int pmic_glink_altmode_probe(struct auxiliary_device *adev,
>   		alt_port->bridge.funcs = &pmic_glink_altmode_bridge_funcs;
>   		alt_port->bridge.of_node = to_of_node(fwnode);
>   		alt_port->bridge.ops = DRM_BRIDGE_OP_HPD;
> -		alt_port->bridge.type = DRM_MODE_CONNECTOR_USB;
> +		alt_port->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>   
>   		ret = devm_drm_bridge_add(dev, &alt_port->bridge);
>   		if (ret) {

Reviewed-by: Neil Armstrong <neil.armstrong@linaro.org>

Re: [PATCH] soc: qcom: pmic_glink: fix connector type to be DisplayPort

[Bjorn Andersson]

On Wed, Oct 11, 2023 at 01:52:29AM +0300, Dmitry Baryshkov wrote:
> As it was pointed out by Simon Ser, the DRM_MODE_CONNECTOR_USB connector
> is reserved for the GUD devices. Other drivers (i915, amdgpu) use
> DRM_MODE_CONNECTOR_DisplayPort even if the DP stream is handled by the
> USB-C altmode. While we are still working on implementing the proper way
> to let userspace know that the DP is wrapped into USB-C, change
> connector type to be DRM_MODE_CONNECTOR_DisplayPort.
> 
> Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
> Cc: Simon Ser <contact@emersion.fr>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

Applied, thanks.

> ---
>  drivers/soc/qcom/pmic_glink_altmode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
> index 9569d999391d..6f8b2f7ae3cc 100644
> --- a/drivers/soc/qcom/pmic_glink_altmode.c
> +++ b/drivers/soc/qcom/pmic_glink_altmode.c
> @@ -467,7 +467,7 @@ static int pmic_glink_altmode_probe(struct auxiliary_device *adev,
>  		alt_port->bridge.funcs = &pmic_glink_altmode_bridge_funcs;
>  		alt_port->bridge.of_node = to_of_node(fwnode);
>  		alt_port->bridge.ops = DRM_BRIDGE_OP_HPD;
> -		alt_port->bridge.type = DRM_MODE_CONNECTOR_USB;
> +		alt_port->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>  
>  		ret = devm_drm_bridge_add(dev, &alt_port->bridge);
>  		if (ret) {
> -- 
> 2.39.2
> 

Re: [PATCH] soc: qcom: pmic_glink: fix connector type to be DisplayPort

[Johan Hovold]

On Wed, Oct 11, 2023 at 01:52:29AM +0300, Dmitry Baryshkov wrote:
> As it was pointed out by Simon Ser, the DRM_MODE_CONNECTOR_USB connector
> is reserved for the GUD devices. Other drivers (i915, amdgpu) use
> DRM_MODE_CONNECTOR_DisplayPort even if the DP stream is handled by the
> USB-C altmode. While we are still working on implementing the proper way
> to let userspace know that the DP is wrapped into USB-C, change
> connector type to be DRM_MODE_CONNECTOR_DisplayPort.
> 
> Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
> Cc: Simon Ser <contact@emersion.fr>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> ---
>  drivers/soc/qcom/pmic_glink_altmode.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
> index 9569d999391d..6f8b2f7ae3cc 100644
> --- a/drivers/soc/qcom/pmic_glink_altmode.c
> +++ b/drivers/soc/qcom/pmic_glink_altmode.c
> @@ -467,7 +467,7 @@ static int pmic_glink_altmode_probe(struct auxiliary_device *adev,
>  		alt_port->bridge.funcs = &pmic_glink_altmode_bridge_funcs;
>  		alt_port->bridge.of_node = to_of_node(fwnode);
>  		alt_port->bridge.ops = DRM_BRIDGE_OP_HPD;
> -		alt_port->bridge.type = DRM_MODE_CONNECTOR_USB;
> +		alt_port->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>  
>  		ret = devm_drm_bridge_add(dev, &alt_port->bridge);
>  		if (ret) {

I was just going to post a patch fixing this after finally investigating
why the DisplayPort outputs on the X13s were annoyingly identified as
"Unknown20-1" and "Unknown20-2" instead of the expected "DP-1" and
"DP-2".

A lore search just before posting led me to this fix from two weeks ago.

I think the commit message should have mentioned something about the how
this change affects user space. My patch also had a CC stable, but I
guess we can ping the stable team once it hits mainline:

commit e5f55bf5ad4effdd59d4d06c839a0ac553a73c7d (HEAD -> work)
Author: Johan Hovold <johan+linaro@kernel.org>
Date:   Wed Oct 25 11:54:09 2023 +0200

    soc: qcom: pmic_glink_altmode: fix DP alt mode connector type
    
    The PMIC glink altmode bridge connector type should be "DisplayPort"
    rather than "USB", which is intended for custom USB display protocols
    (e.g. see 40e1a70b4aed ("drm: Add GUD USB Display driver")).
    
    This specifically makes the DisplayPort outputs on the Lenovo ThinkPad
    X13s show up as "DP-1" and "DP-2" rather than "Unknown20-1" and
    "Unknown20-2" with xrandr as expected (by users and tools):
    
      Screen 0: minimum 320 x 200, current 1920 x 1200, maximum 5120 x 4096
      eDP-1 connected primary 1920x1200+0+0 (normal left inverted right x axis y axis) 286mm x 178mm
         1920x1200     60.03*+
         1600x1200     60.00
      DP-1 disconnected (normal left inverted right x axis y axis)
      DP-2 connected (normal left inverted right x axis y axis)
         1920x1200     59.95 +
      ...
    
    Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
    Cc: stable@vger.kernel.org      # 6.3
    Signed-off-by: Johan Hovold <johan+linaro@kernel.org>

Johan

Re: [PATCH] soc: qcom: pmic_glink: fix connector type to be DisplayPort

[Simon Ser]

On Wednesday, October 25th, 2023 at 14:22, Johan Hovold <johan@kernel.org> wrote:

> I was just going to post a patch fixing this after finally investigating
> why the DisplayPort outputs on the X13s were annoyingly identified as
> "Unknown20-1" and "Unknown20-2" instead of the expected "DP-1" and
> "DP-2".

Note, ideally userspace should use drmModeGetConnectorTypeName() from
libdrm to figure out the proper name for a connector type. That way we
only need to update a single spot when adding a new connector type,
instead of patching a whole bunch of programs.

Re: [PATCH] soc: qcom: pmic_glink: fix connector type to be DisplayPort

[Johan Hovold]

On Wed, Oct 25, 2023 at 12:29:26PM +0000, Simon Ser wrote:
> On Wednesday, October 25th, 2023 at 14:22, Johan Hovold <johan@kernel.org> wrote:
> 
> > I was just going to post a patch fixing this after finally investigating
> > why the DisplayPort outputs on the X13s were annoyingly identified as
> > "Unknown20-1" and "Unknown20-2" instead of the expected "DP-1" and
> > "DP-2".
> 
> Note, ideally userspace should use drmModeGetConnectorTypeName() from
> libdrm to figure out the proper name for a connector type. That way we
> only need to update a single spot when adding a new connector type,
> instead of patching a whole bunch of programs.

Yeah, I only skimmed the xrandr code and these strings appear to
originate from some X library. So hopefully not that many places to
update.

Scripts and other tools may still be using these strings directly
however (e.g. as did my custom script to enable external displays).

Johan

Re: [PATCH] soc: qcom: pmic_glink: fix connector type to be DisplayPort

[Dmitry Baryshkov]

On 25/10/2023 15:29, Simon Ser wrote:
> On Wednesday, October 25th, 2023 at 14:22, Johan Hovold <johan@kernel.org> wrote:
> 
>> I was just going to post a patch fixing this after finally investigating
>> why the DisplayPort outputs on the X13s were annoyingly identified as
>> "Unknown20-1" and "Unknown20-2" instead of the expected "DP-1" and
>> "DP-2".
> 
> Note, ideally userspace should use drmModeGetConnectorTypeName() from
> libdrm to figure out the proper name for a connector type. That way we
> only need to update a single spot when adding a new connector type,
> instead of patching a whole bunch of programs.

X11 does its own thing. It further "renames" DP MST connectors. So on my 
laptop I end up with DP-1-1 in xrandr, but DP-3 in kernel.

-- 
With best wishes
Dmitry

Re: [PATCH] soc: qcom: pmic_glink: fix connector type to be DisplayPort

[Dmitry Baryshkov]

On 25/10/2023 15:23, Johan Hovold wrote:
> On Wed, Oct 11, 2023 at 01:52:29AM +0300, Dmitry Baryshkov wrote:
>> As it was pointed out by Simon Ser, the DRM_MODE_CONNECTOR_USB connector
>> is reserved for the GUD devices. Other drivers (i915, amdgpu) use
>> DRM_MODE_CONNECTOR_DisplayPort even if the DP stream is handled by the
>> USB-C altmode. While we are still working on implementing the proper way
>> to let userspace know that the DP is wrapped into USB-C, change
>> connector type to be DRM_MODE_CONNECTOR_DisplayPort.
>>
>> Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
>> Cc: Simon Ser <contact@emersion.fr>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>> ---
>>   drivers/soc/qcom/pmic_glink_altmode.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/soc/qcom/pmic_glink_altmode.c b/drivers/soc/qcom/pmic_glink_altmode.c
>> index 9569d999391d..6f8b2f7ae3cc 100644
>> --- a/drivers/soc/qcom/pmic_glink_altmode.c
>> +++ b/drivers/soc/qcom/pmic_glink_altmode.c
>> @@ -467,7 +467,7 @@ static int pmic_glink_altmode_probe(struct auxiliary_device *adev,
>>   		alt_port->bridge.funcs = &pmic_glink_altmode_bridge_funcs;
>>   		alt_port->bridge.of_node = to_of_node(fwnode);
>>   		alt_port->bridge.ops = DRM_BRIDGE_OP_HPD;
>> -		alt_port->bridge.type = DRM_MODE_CONNECTOR_USB;
>> +		alt_port->bridge.type = DRM_MODE_CONNECTOR_DisplayPort;
>>   
>>   		ret = devm_drm_bridge_add(dev, &alt_port->bridge);
>>   		if (ret) {
> 
> I was just going to post a patch fixing this after finally investigating
> why the DisplayPort outputs on the X13s were annoyingly identified as
> "Unknown20-1" and "Unknown20-2" instead of the expected "DP-1" and
> "DP-2".

Well, that depends on the userspace being updated to know about USB 
connectors or not. But you are right, we should probably mention that in 
the commit message.

> 
> A lore search just before posting led me to this fix from two weeks ago.
> 
> I think the commit message should have mentioned something about the how
> this change affects user space. My patch also had a CC stable, but I
> guess we can ping the stable team once it hits mainline:
> 
> commit e5f55bf5ad4effdd59d4d06c839a0ac553a73c7d (HEAD -> work)
> Author: Johan Hovold <johan+linaro@kernel.org>
> Date:   Wed Oct 25 11:54:09 2023 +0200
> 
>      soc: qcom: pmic_glink_altmode: fix DP alt mode connector type
>      
>      The PMIC glink altmode bridge connector type should be "DisplayPort"
>      rather than "USB", which is intended for custom USB display protocols
>      (e.g. see 40e1a70b4aed ("drm: Add GUD USB Display driver")).
>      
>      This specifically makes the DisplayPort outputs on the Lenovo ThinkPad
>      X13s show up as "DP-1" and "DP-2" rather than "Unknown20-1" and
>      "Unknown20-2" with xrandr as expected (by users and tools):
>      
>        Screen 0: minimum 320 x 200, current 1920 x 1200, maximum 5120 x 4096
>        eDP-1 connected primary 1920x1200+0+0 (normal left inverted right x axis y axis) 286mm x 178mm
>           1920x1200     60.03*+
>           1600x1200     60.00
>        DP-1 disconnected (normal left inverted right x axis y axis)
>        DP-2 connected (normal left inverted right x axis y axis)
>           1920x1200     59.95 +
>        ...
>      
>      Fixes: 080b4e24852b ("soc: qcom: pmic_glink: Introduce altmode support")
>      Cc: stable@vger.kernel.org      # 6.3
>      Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> 
> Johan

-- 
With best wishes
Dmitry

Re: [PATCH] soc: qcom: pmic_glink: fix connector type to be DisplayPort

[Simon Ser]

On Wednesday, October 25th, 2023 at 14:45, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote:

> On 25/10/2023 15:29, Simon Ser wrote:
> 
> > On Wednesday, October 25th, 2023 at 14:22, Johan Hovold johan@kernel.org wrote:
> > 
> > > I was just going to post a patch fixing this after finally investigating
> > > why the DisplayPort outputs on the X13s were annoyingly identified as
> > > "Unknown20-1" and "Unknown20-2" instead of the expected "DP-1" and
> > > "DP-2".
> > 
> > Note, ideally userspace should use drmModeGetConnectorTypeName() from
> > libdrm to figure out the proper name for a connector type. That way we
> > only need to update a single spot when adding a new connector type,
> > instead of patching a whole bunch of programs.
> 
> X11 does its own thing. It further "renames" DP MST connectors. So on my
> laptop I end up with DP-1-1 in xrandr, but DP-3 in kernel.

Right. But that function only takes in a connector type enum as input,
and returns a string for the type only. It doesn't include the suffix
with the number.

IOW: xserver could use drmModeGetConnectorTypeName() and then append
"-2" or "-1-1" or whatever.

But with the current appetite for xserver development this is probably
not going to happen (and doesn't matter very much).