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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1.1: Type: text/plain, Size: 7922 bytes --]

Hi

Am 21.01.21 um 19:07 schrieb Noralf Trønnes:
> 
> 
> Den 21.01.2021 11.01, skrev Thomas Zimmermann:
>> Hi
>>
>> Am 21.01.21 um 09:27 schrieb Daniel Vetter:
>>> On Thu, Jan 21, 2021 at 8:45 AM Thomas Zimmermann
>>> <tzimmermann@suse.de> wrote:
>>>>
>>>> Hi Noralf,
>>>>
>>>> glad to hear from you! Welcome back!
> 
> Thanks Thomas!
> 
>>>>
>>>> Am 20.01.21 um 18:42 schrieb Daniel Vetter:
>>>>> On Wed, Jan 20, 2021 at 6:10 PM Noralf Trønnes <noralf@tronnes.org>
>>>>> wrote:
>>>>>>
>>>>>> Add a connector type for USB connected display panels.
>>>>>>
>>>>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
>>>>>> ---
> 
> I have forgotten to update drm_connector_enum_list which maps type to name.
> 
>>>>>>     include/uapi/drm/drm_mode.h | 1 +
>>>>>>     1 file changed, 1 insertion(+)
>>>>>>
>>>>>> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
>>>>>> index fed66a03c7ae..33024cc5d26e 100644
>>>>>> --- a/include/uapi/drm/drm_mode.h
>>>>>> +++ b/include/uapi/drm/drm_mode.h
>>>>>> @@ -367,6 +367,7 @@ enum drm_mode_subconnector {
>>>>>>     #define DRM_MODE_CONNECTOR_DPI         17
>>>>>>     #define DRM_MODE_CONNECTOR_WRITEBACK   18
>>>>>>     #define DRM_MODE_CONNECTOR_SPI         19
>>>>>> +#define DRM_MODE_CONNECTOR_USB         20
>>>>
>>>> I would not call it USB. I could imagine that at some point a generic
>>>> USB protocol could serve simple displays (i.e. in the sense of USB HID
>>>> or data or imaging). (Maybe Thunderbold already counts.) Anyway, USB
>>>> should be reserved for this case.
>>>
>>> We end up calling those DisplayPort, since that's what's being
>>> transported over thunderbolt or usb-C. So the usb connector would be
>>> called usb-C. I think the reason we don't do fancy connector names is
>>> that adding them is a bit a pain. Plus drm/i915 specifically has some
>>> very quirky connector enumerating that doesn't match much with reality
>>> unfortunately anyway :-/
>>
>> In the case of the other USB drivers, IIRC we use the connector type
>> that is at the output (i.e., HDMI in the case of udl). I think we should
>> do the same here. Or use 'Unknown'.
>>
> 
> There are 2 DRM USB drivers and they use:
> - udl: DRM_MODE_CONNECTOR_DVII

Mine has plain old VGA. Maybe we should change generally this to Unknown.

> - gm12u320: DRM_MODE_CONNECTOR_VGA
> 
> gm12u320 is a mini projector so it doesn't actually have a VGA
> connector. I have never seen a udl device but I assume it has a DVII
> connector?
> 
> For display adapters it makes sense to use the connector on the adapter
> as the reported connector, but for display panels that don't have any
> connector except for the cable that is connected to the hosts USB
> connector, why can't it be called a USB connector? That's the connector
> the user sees.

It's not the relevant connector for the display output. USB is the bus 
system. (Making your argument in terms of discrete GPUs, the connector 
would always be PCI then.)

> 
> 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.

BTW, can I try this out somehow? I do have an RPi3. Do I need a special 
disk image?

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 #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel