| Message ID | 20231010225229.77027-1-dmitry.baryshkov@linaro.org |
|---|---|
| State | Accepted |
| Commit | f86955f2b1ff9fbc7ae4f6595112b2f896885366 |
| Headers | show |
| Series | soc: qcom: pmic_glink: fix connector type to be DisplayPort | expand |
Thanks for fixing this!
Acked-by: Simon Ser <contact@emersion.fr>
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 >
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
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.
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
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.
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
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).
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) {
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(-)