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.
Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.
Signed-off-by: Javier Martinez Canillas <[email protected]>
---
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
On 1/31/22 21:52, Sam Ravnborg wrote:
> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>> 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.
>>
>> Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.
> I had expected unknown-21??
>
As you pointed out in patch 3/4, I forgot to change DRM_MODE_CONNECTOR_Unknown
to DRM_MODE_CONNECTOR_I2C after adding this patch...
>>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
> Reviewed-by: Sam Ravnborg <[email protected]>
>> ---
Thanks!
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:
>>
>>
>> Den 31.01.2022 21.52, skrev Sam Ravnborg:
>>> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>>>> 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 2/1/22 14:20, Noralf Trønnes wrote:
>
>
> Den 01.02.2022 14.06, skrev Javier Martinez Canillas:
>> Hello Noralf,
>>
>> On 2/1/22 13:58, Noralf Trønnes wrote:
>>>
>>>
>>> Den 31.01.2022 21.52, skrev Sam Ravnborg:
>>>> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>>>>> 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
Hello Noralf,
On 2/1/22 13:58, Noralf Trønnes wrote:
>
>
> Den 31.01.2022 21.52, skrev Sam Ravnborg:
>> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>>> 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
On 2/1/22 23:29, Simon Ser wrote:
> On Tuesday, February 1st, 2022 at 21:57, Sam Ravnborg <[email protected]> wrote:
>
>> As I wrote in another part of this thread(s) - typing the patch is easy.
>> But I do not understand the userspace implications so I need someone
>> else to say go.
>
> User-space shouldn't really use the connector for anything except making it
> easier for the user to understand where to plug the display cable. I think a
> generic "panel" connector type makes sense.
>
I'll drop this patch from the series. I didn't have all the context and thought
that was an opportunity to add an I2C type, since there was a SPI type already.
But seems to be more controversial than I expected and shouldn't really matter
for a tiny driver like this. I will just go ahead and use the Unknown type.
Best regards,
--
Javier Martinez Canillas
Linux Engineering
Red Hat
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.
Den 01.02.2022 14.38, skrev Simon Ser:
>
> On Tuesday, February 1st, 2022 at 13:58, Noralf Trønnes <[email protected]> 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
Thanks Simon!
Den 31.01.2022 21.52, skrev Sam Ravnborg:
> On Mon, Jan 31, 2022 at 09:12:21PM +0100, Javier Martinez Canillas wrote:
>> 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
>> Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.
> I had expected unknown-21??
>
>>
>> Signed-off-by: Javier Martinez Canillas <[email protected]>
> Reviewed-by: Sam Ravnborg <[email protected]>
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.
>> ---
>>
>> 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
Den 02.02.2022 16.04, skrev Pekka Paalanen:
> On Wed, 2 Feb 2022 10:45:42 +0100
> Noralf Trønnes <[email protected]> wrote:
>
>> 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
>
> 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.
On Tuesday, February 1st, 2022 at 21:57, Sam Ravnborg <[email protected]> wrote:
> As I wrote in another part of this thread(s) - typing the patch is easy.
> But I do not understand the userspace implications so I need someone
> else to say go.
User-space shouldn't really use the connector for anything except making it
easier for the user to understand where to plug the display cable. I think a
generic "panel" connector type makes sense.
On Tuesday, February 1st, 2022 at 13:58, Noralf Trønnes <[email protected]> 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 Wed, 2 Feb 2022 10:45:42 +0100
Noralf Trønnes <[email protected]> wrote:
> 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
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
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