Re: [PATCH 1/4] drm: Add I2C connector type

Den 31.01.2022 21.52, skrev Sam Ravnborg:

On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:

quoted

There isn't a connector type for display controllers accesed through I2C,
most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.

Add an I2C connector type to match the actual connector.

As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
type"), user-space should be able to cope with a connector type that does
not yet understand.

It turned out that I wasn't entirely correct here, mpv didn't cope with
unknown types. In the PR to add support Emil Velikov wondered if libdrm
should handle these connector names:
https://github.com/mpv-player/mpv/pull/8989#issuecomment-879187711

quoted

Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.

I had expected unknown-21??

quoted

Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>

Reviewed-by: Sam Ravnborg <redacted>

Sam, didn't you and Laurent discuss adding DRM_MODE_CONNECTOR_PANEL for
such a use case?

If someone adds parallel bus support to the MIPI DBI helper, there will
be one more connector type (I wonder what that one will be called).

Noralf.

quoted

---

 drivers/gpu/drm/drm_connector.c | 1 +
 include/uapi/drm/drm_mode.h     | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index a50c82bc2b2f..975a7525a508 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c

@@ -105,6 +105,7 @@ static struct drm_conn_prop_enum_list drm_connector_enum_list[] = {
 	{ DRM_MODE_CONNECTOR_WRITEBACK, "Writeback" },
 	{ DRM_MODE_CONNECTOR_SPI, "SPI" },
 	{ DRM_MODE_CONNECTOR_USB, "USB" },
+	{ DRM_MODE_CONNECTOR_I2C, "I2C" },
 };
 
 void drm_connector_ida_init(void)

diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index e1e351682872..d6d6288242db 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h

@@ -421,6 +421,7 @@ enum drm_mode_subconnector {
 #define DRM_MODE_CONNECTOR_WRITEBACK	18
 #define DRM_MODE_CONNECTOR_SPI		19
 #define DRM_MODE_CONNECTOR_USB		20
+#define DRM_MODE_CONNECTOR_I2C		21
 
 /**
  * struct drm_mode_get_connector - Get connector metadata.

-- 
2.34.1

Hello Noralf,

On 2/1/22 13:58, Noralf Trønnes wrote:

Den 31.01.2022 21.52, skrev Sam Ravnborg:

quoted

On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:

quoted

There isn't a connector type for display controllers accesed through I2C,
most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.

Add an I2C connector type to match the actual connector.

As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
type"), user-space should be able to cope with a connector type that does
not yet understand.

It turned out that I wasn't entirely correct here, mpv didn't cope with
unknown types. In the PR to add support Emil Velikov wondered if libdrm
should handle these connector names:
https://github.com/mpv-player/mpv/pull/8989#issuecomment-879187711

I see, thanks for the information. What should we do then, just use the type
DRM_MODE_CONNECTOR_Unknown then ?

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Den 01.02.2022 14.06, skrev Javier Martinez Canillas:

Hello Noralf,

On 2/1/22 13:58, Noralf Trønnes wrote:

quoted

Den 31.01.2022 21.52, skrev Sam Ravnborg:

quoted

On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:

quoted

There isn't a connector type for display controllers accesed through I2C,
most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.

Add an I2C connector type to match the actual connector.

As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
type"), user-space should be able to cope with a connector type that does
not yet understand.

It turned out that I wasn't entirely correct here, mpv didn't cope with
unknown types. In the PR to add support Emil Velikov wondered if libdrm
should handle these connector names:
https://github.com/mpv-player/mpv/pull/8989#issuecomment-879187711

I see, thanks for the information. What should we do then, just use the type
DRM_MODE_CONNECTOR_Unknown then ?

Not really, I just wanted to point out that it could be that not all
userspace will handle an unknown connector type (I just checked the DE's
at the time). I haven't seen any issues after adding the SPI type so it
can't be that many apps that has problems. Adding to that a tiny
monochrome display is limited in which applications it will encounter I
guess :) It was after adding the USB type that I discovered that mpv
didn't work.

Noralf.

On Tuesday, February 1st, 2022 at 13:58, Noralf Trønnes [off-list ref] wrote:

It turned out that I wasn't entirely correct here, mpv didn't cope with
unknown types. In the PR to add support Emil Velikov wondered if libdrm
should handle these connector names:

Opened this MR to try to make things easier for user-space:
https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/222

On 2/1/22 14:20, Noralf Trønnes wrote:

Den 01.02.2022 14.06, skrev Javier Martinez Canillas:

quoted

Hello Noralf,

On 2/1/22 13:58, Noralf Trønnes wrote:

quoted

Den 31.01.2022 21.52, skrev Sam Ravnborg:

quoted

On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:

quoted

There isn't a connector type for display controllers accesed through I2C,
most drivers use DRM_MODE_CONNECTOR_Unknown or DRM_MODE_CONNECTOR_VIRTUAL.

Add an I2C connector type to match the actual connector.

As Noralf Trønnes mentions in commit fc06bf1d76d6 ("drm: Add SPI connector
type"), user-space should be able to cope with a connector type that does
not yet understand.

It turned out that I wasn't entirely correct here, mpv didn't cope with
unknown types. In the PR to add support Emil Velikov wondered if libdrm
should handle these connector names:
https://github.com/mpv-player/mpv/pull/8989#issuecomment-879187711

I see, thanks for the information. What should we do then, just use the type
DRM_MODE_CONNECTOR_Unknown then ?

Not really, I just wanted to point out that it could be that not all
userspace will handle an unknown connector type (I just checked the DE's
at the time). I haven't seen any issues after adding the SPI type so it
can't be that many apps that has problems. Adding to that a tiny
monochrome display is limited in which applications it will encounter I
guess :) It was after adding the USB type that I discovered that mpv
didn't work.

Anything we do for this rather obscure hardware certainly won't be an
issue for most applications :)

But I wasn't sure if your previous comment meant that you were nacking
$subject. Glad that we can go ahead and describe the correct type then.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Den 01.02.2022 14.38, skrev Simon Ser:

On Tuesday, February 1st, 2022 at 13:58, Noralf Trønnes [off-list ref] wrote:

quoted

It turned out that I wasn't entirely correct here, mpv didn't cope with
unknown types. In the PR to add support Emil Velikov wondered if libdrm
should handle these connector names:

Opened this MR to try to make things easier for user-space:
https://gitlab.freedesktop.org/mesa/drm/-/merge_requests/222

Thanks Simon!

Hi Noralf,

since you're here, I'll just hijack the discussion to ask something only 
semi-related.

IIRC the gud driver doesn't update the display immediately during atomic 
commits. Instead, it instructs a helper thread to do the update. What's 
the rational behind this design? Is that something we should adopt for 
other drivers that operate over slow buses (USB, I2C, etc)? Would this 
be relevant for the ssd1307 driver?

Best regards
Thomas

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

Den 02.02.2022 10.14, skrev Thomas Zimmermann:

Hi Noralf,

since you're here, I'll just hijack the discussion to ask something only
semi-related.

IIRC the gud driver doesn't update the display immediately during atomic
commits. Instead, it instructs a helper thread to do the update. What's
the rational behind this design? Is that something we should adopt for
other drivers that operate over slow buses (USB, I2C, etc)? Would this
be relevant for the ssd1307 driver?

Async flushing is only necessary on multi display setups where there's
only one rendering loop for all the displays. I saw what tiny/gm12u320.c
did and Hans gave me the rationale. The SPI drivers run flushing inline.
Info on the gud wiki:
https://github.com/notro/gud/wiki/Linux-Host-Driver#asynchronous-flushing

Noralf.

On Wed, 2 Feb 2022 10:45:42 +0100
Noralf Trønnes [off-list ref] wrote:

Den 02.02.2022 10.14, skrev Thomas Zimmermann:

quoted

Hi Noralf,

since you're here, I'll just hijack the discussion to ask something only
semi-related.

IIRC the gud driver doesn't update the display immediately during atomic
commits. Instead, it instructs a helper thread to do the update. What's
the rational behind this design? Is that something we should adopt for
other drivers that operate over slow buses (USB, I2C, etc)? Would this
be relevant for the ssd1307 driver?
  

Async flushing is only necessary on multi display setups where there's
only one rendering loop for all the displays. I saw what tiny/gm12u320.c
did and Hans gave me the rationale. The SPI drivers run flushing inline.
Info on the gud wiki:
https://github.com/notro/gud/wiki/Linux-Host-Driver#asynchronous-flushing

Hi,

please also consider that userspace may throttle to the KMS pageflip
events. If the pageflip event is immediate from submitting a flip, that
could mean userspace will be repainting in a busy-loop, like 1 kHz.
However, I remember something about virtual KMS drivers doing exactly
this, and there being something that tells userspace to throttle itself
instead of depending on pageflip completions. I just forget how that is
supposed to work, and I'm fairly sure that e.g. Weston does not behave
well there.

Unfortunately, the pageflip event is also what synchronises FB usage.
Once flipping in a new FB completed, the old FB is free for re-use.
But, if the kernel is still copying out from the old FB, userspace may
partially overwrite the contents, temporarily leading to an incomplete
or too new image on screen. Do you have anything to prevent that?


Thanks,
pq

Den 02.02.2022 16.04, skrev Pekka Paalanen:

On Wed, 2 Feb 2022 10:45:42 +0100
Noralf Trønnes [off-list ref] wrote:

quoted

Den 02.02.2022 10.14, skrev Thomas Zimmermann:

quoted

Hi Noralf,

since you're here, I'll just hijack the discussion to ask something only
semi-related.

IIRC the gud driver doesn't update the display immediately during atomic
commits. Instead, it instructs a helper thread to do the update. What's
the rational behind this design? Is that something we should adopt for
other drivers that operate over slow buses (USB, I2C, etc)? Would this
be relevant for the ssd1307 driver?
  

Async flushing is only necessary on multi display setups where there's
only one rendering loop for all the displays. I saw what tiny/gm12u320.c
did and Hans gave me the rationale. The SPI drivers run flushing inline.
Info on the gud wiki:
https://github.com/notro/gud/wiki/Linux-Host-Driver#asynchronous-flushing

Hi,

please also consider that userspace may throttle to the KMS pageflip
events. If the pageflip event is immediate from submitting a flip, that
could mean userspace will be repainting in a busy-loop, like 1 kHz.
However, I remember something about virtual KMS drivers doing exactly
this, and there being something that tells userspace to throttle itself
instead of depending on pageflip completions. I just forget how that is
supposed to work, and I'm fairly sure that e.g. Weston does not behave
well there.

Unfortunately, the pageflip event is also what synchronises FB usage.
Once flipping in a new FB completed, the old FB is free for re-use.
But, if the kernel is still copying out from the old FB, userspace may
partially overwrite the contents, temporarily leading to an incomplete
or too new image on screen. Do you have anything to prevent that?

Unfortunately not. One solution would be to make a buffer copy during
the flip and do the USB transfer async but I haven't looked into that.
My plan is to wait and see what problems users report back before trying
to fix anything.

Noralf.