Re: [PATCH v4 1/3] drm/uapi: Add USB connector type

From: Thomas Zimmermann <tzimmermann@suse.de>
To: "Noralf Trønnes" <noralf@tronnes.org>, "Daniel Vetter" <daniel@ffwll.ch>
Cc: hudson@trmm.net, markus@raatikainen.cc,
	Sam Ravnborg <sam@ravnborg.org>,
	USB list <linux-usb@vger.kernel.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	Tyler Hardin <th020394@gmail.com>,
	Lubomir Rintel <lkundrak@v3.sk>,
	pontus.fuchs@gmail.com, peter@stuge.se
Subject: Re: [PATCH v4 1/3] drm/uapi: Add USB connector type
Date: Fri, 22 Jan 2021 13:47:51 +0100	[thread overview]
Message-ID: <9168c91b-f97d-5725-7d6f-8c97fb422d9a@suse.de> (raw)
In-Reply-To: <dfd02473-f0b5-e8b2-3399-d87063a2fd1f@tronnes.org>

[-- Attachment #1.1: Type: text/plain, Size: 7485 bytes --]

Hi

Am 22.01.21 um 12:44 schrieb Noralf Trønnes:
> 
> And wrt PCI it wouldn't be a PCI connector if the card has some other
> connector for the display, but if it was possible to connect a display
> directly to the PCI connector, then yes I would call that a PCI connector.

You're not connecting a display to the computer. You're connecting an 
RPi and then connect the display to the RPi. The RPi acts like an 
external graphics card.

> This begs the question: Why does the kernel provide info to userspace
> about the connector type?
> 
> My take is that it is so the user can know which display is connected to
> which port on the computer.

This exactly illustrates the problem with the current naming. For a 
single output the distinction between bus and connector might be fuzzy. 
As soon as a connected SoC contains multiple connectors. The user then 
sees names such as card1-USB-0 and card1-USB-1, which makes no sense.

> 
> What's your opinion?
> 
>>>
>>> Ofc as Daniel mentions it's a downside that userspace doesn't know about
>>> the connector type, and who knows when it will updated (if I don't do
>>> it).
>>> Weston will name it: "UNNAMED-%d"
>>> Mutter: "Unknown%d-%d"
>>> X: "Unknown%d-%d"
>>>
>>> Sam and Laurent has discussed adding a PANEL connector type instead of
>>> adding more connector types for panel connectors. I think that would
>>> have been a better choice instead of the SPI connector type that I added
>>> in 2019. But I think PANEL was meant for panels connected to an internal
>>> connector.
>>>
>>> Here's my protocol connector types and how it's mapped to DRM:
>>>
>>> #define GUD_CONNECTOR_TYPE_PANEL        0
>>> #define GUD_CONNECTOR_TYPE_VGA            1
>>> #define GUD_CONNECTOR_TYPE_COMPOSITE        2
>>> #define GUD_CONNECTOR_TYPE_SVIDEO        3
>>> #define GUD_CONNECTOR_TYPE_COMPONENT        4
>>> #define GUD_CONNECTOR_TYPE_DVI            5
>>> #define GUD_CONNECTOR_TYPE_DISPLAYPORT        6
>>> #define GUD_CONNECTOR_TYPE_HDMI            7
>>>
>>> static int gud_gadget_ctrl_get_connector(struct gud_gadget *gdg,
>>> unsigned int index,
>>>                       struct gud_connector_descriptor_req *desc)
>>> {
>>> ...
>>>      gconn = &gdg->connectors[index];
>>>
>>>      switch (gconn->connector->connector_type) {
>>>      case DRM_MODE_CONNECTOR_VGA:
>>>          desc->connector_type = GUD_CONNECTOR_TYPE_VGA;
>>>          break;
>>>      case DRM_MODE_CONNECTOR_DVII:
>>>          fallthrough;
>>>      case DRM_MODE_CONNECTOR_DVID:
>>>          fallthrough;
>>>      case DRM_MODE_CONNECTOR_DVIA:
>>>          desc->connector_type = GUD_CONNECTOR_TYPE_DVI;
>>>          break;
>>>      case DRM_MODE_CONNECTOR_Composite:
>>>          desc->connector_type = GUD_CONNECTOR_TYPE_COMPOSITE;
>>>          break;
>>>      case DRM_MODE_CONNECTOR_SVIDEO:
>>>          desc->connector_type = GUD_CONNECTOR_TYPE_SVIDEO;
>>>          break;
>>>      case DRM_MODE_CONNECTOR_Component:
>>>          desc->connector_type = GUD_CONNECTOR_TYPE_COMPONENT;
>>>          break;
>>>      case DRM_MODE_CONNECTOR_DisplayPort:
>>>          desc->connector_type = GUD_CONNECTOR_TYPE_DISPLAYPORT;
>>>          break;
>>>      case DRM_MODE_CONNECTOR_HDMIA:
>>>          fallthrough;
>>>      case DRM_MODE_CONNECTOR_HDMIB:
>>>          desc->connector_type = GUD_CONNECTOR_TYPE_HDMI;
>>>          break;
>>>      default:
>>>          desc->connector_type = GUD_CONNECTOR_TYPE_PANEL;
>>>          break;
>>>      };
>>>
>>>
>>> int gud_connector_create(struct gud_device *gdrm, unsigned int index)
>>> {
>>> ...
>>>      switch (desc.connector_type) {
>>>      case GUD_CONNECTOR_TYPE_PANEL:
>>>          connector_type = DRM_MODE_CONNECTOR_USB;
>>>          break;
>>>      case GUD_CONNECTOR_TYPE_VGA:
>>>          connector_type = DRM_MODE_CONNECTOR_VGA;
>>>          break;
>>>      case GUD_CONNECTOR_TYPE_DVI:
>>>          connector_type = DRM_MODE_CONNECTOR_DVID;
>>>          break;
>>>      case GUD_CONNECTOR_TYPE_COMPOSITE:
>>>          connector_type = DRM_MODE_CONNECTOR_Composite;
>>>          break;
>>>      case GUD_CONNECTOR_TYPE_SVIDEO:
>>>          connector_type = DRM_MODE_CONNECTOR_SVIDEO;
>>>          break;
>>>      case GUD_CONNECTOR_TYPE_COMPONENT:
>>>          connector_type = DRM_MODE_CONNECTOR_Component;
>>>          break;
>>>      case GUD_CONNECTOR_TYPE_DISPLAYPORT:
>>>          connector_type = DRM_MODE_CONNECTOR_DisplayPort;
>>>          break;
>>>      case GUD_CONNECTOR_TYPE_HDMI:
>>>          connector_type = DRM_MODE_CONNECTOR_HDMIA;
>>>          break;
>>>      default: /* future types */
>>>          connector_type = DRM_MODE_CONNECTOR_USB;
>>
>> The more I look at it the more I think it should be 'Unknown' here.
>>
> 
> I don't understand this, how will that be better for the user?

As I said before, the display is not connected via USB. The RPi (i.e., 
graphics card) is. The naming would be off.

Best regards
Thomas

> 
>> BTW, can I try this out somehow? I do have an RPi3. Do I need a special
>> disk image?
> 
> The Pi3 doesn'have a USB device/otg connector so I haven't made an image
> for that one. Only the Pi Zero, model A and Pi 4 have that.
> 
> The Pi2 and Pi3 have a USB hub on the soc's single USB port.
> 
> Noralf.
> 
>>
>> Best regards
>> Thomas
>>
>>>          break;
>>>      };
>>>
>>> Noralf.
>>>
>>>> Best regards
>>>> Thomas
>>>>
>>>>> -Daniel
>>>>>
>>>>>>
>>>>>> Best regards
>>>>>> Thomas
>>>>>>
>>>>>>>
>>>>>>> Beware, new connector types have in the past resulted in userspace
>>>>>>> burning&crashing. Maybe it's become better ...
>>>>>>>
>>>>>>> Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>>>>>>>
>>>>>>>>      /**
>>>>>>>>       * struct drm_mode_get_connector - Get connector metadata.
>>>>>>>> -- 
>>>>>>>> 2.23.0
>>>>>>>>
>>>>>>>> _______________________________________________
>>>>>>>> dri-devel mailing list
>>>>>>>> dri-devel@lists.freedesktop.org
>>>>>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> -- 
>>>>>> Thomas Zimmermann
>>>>>> Graphics Driver Developer
>>>>>> SUSE Software Solutions Germany GmbH
>>>>>> Maxfeldstr. 5, 90409 Nürnberg, Germany
>>>>>> (HRB 36809, AG Nürnberg)
>>>>>> Geschäftsführer: Felix Imendörffer
>>>>>>
>>>>>
>>>>>
>>>>
>>>>
>>>> _______________________________________________
>>>> dri-devel mailing list
>>>> dri-devel@lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>>
>>> _______________________________________________
>>> dri-devel mailing list
>>> dri-devel@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>>>
>>

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Felix Imendörffer

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]