diff mbox series

[1/4] drm: Add I2C connector type

Message ID 20220131201225.2324984-2-javierm@redhat.com (mailing list archive)
State Superseded, archived
Headers show
Series drm/tiny: Add driver for Solomon SSD1307 OLED displays | expand

Commit Message

Javier Martinez Canillas Jan. 31, 2022, 8:12 p.m. UTC
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 <javierm@redhat.com>
---

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

Comments

Sam Ravnborg Jan. 31, 2022, 8:52 p.m. UTC | #1
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??

> 
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> ---
> 
>  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
Javier Martinez Canillas Jan. 31, 2022, 11:26 p.m. UTC | #2
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 <javierm@redhat.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
>> ---

Thanks!

Best regards,
Noralf Trønnes Feb. 1, 2022, 12:58 p.m. UTC | #3
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 <javierm@redhat.com>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

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
Javier Martinez Canillas Feb. 1, 2022, 1:06 p.m. UTC | #4
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,
Noralf Trønnes Feb. 1, 2022, 1:20 p.m. UTC | #5
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.
Simon Ser Feb. 1, 2022, 1:38 p.m. UTC | #6
On Tuesday, February 1st, 2022 at 13:58, Noralf Trønnes <noralf@tronnes.org> 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
Javier Martinez Canillas Feb. 1, 2022, 1:55 p.m. UTC | #7
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,
Noralf Trønnes Feb. 1, 2022, 2:20 p.m. UTC | #8
Den 01.02.2022 14.38, skrev Simon Ser:
> 
> On Tuesday, February 1st, 2022 at 13:58, Noralf Trønnes <noralf@tronnes.org> 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!
Sam Ravnborg Feb. 1, 2022, 8:57 p.m. UTC | #9
Hi Noralf,

On Tue, Feb 01, 2022 at 01:58:16PM +0100, 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
> 
> >> Tested with `modetest -M ssd1307 -c` and shows the connector as unknown-1.
> > I had expected unknown-21??
> > 
> >>
> >> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> > Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> 
> Sam, didn't you and Laurent discuss adding DRM_MODE_CONNECTOR_PANEL for
> such a use case?

You have a splendid memory - yes we did. But no conclusions:
https://lore.kernel.org/dri-devel/?q=DRM_MODE_CONNECTOR_PANEL

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.

	Sam
Simon Ser Feb. 1, 2022, 10:29 p.m. UTC | #10
On Tuesday, February 1st, 2022 at 21:57, Sam Ravnborg <sam@ravnborg.org> 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.
Javier Martinez Canillas Feb. 2, 2022, 8:46 a.m. UTC | #11
On 2/1/22 23:29, Simon Ser wrote:
> On Tuesday, February 1st, 2022 at 21:57, Sam Ravnborg <sam@ravnborg.org> 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,
Thomas Zimmermann Feb. 2, 2022, 9:14 a.m. UTC | #12
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
Noralf Trønnes Feb. 2, 2022, 9:45 a.m. UTC | #13
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.
Pekka Paalanen Feb. 2, 2022, 3:04 p.m. UTC | #14
On Wed, 2 Feb 2022 10:45:42 +0100
Noralf Trønnes <noralf@tronnes.org> 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
Noralf Trønnes Feb. 2, 2022, 4 p.m. UTC | #15
Den 02.02.2022 16.04, skrev Pekka Paalanen:
> On Wed, 2 Feb 2022 10:45:42 +0100
> Noralf Trønnes <noralf@tronnes.org> 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.
diff mbox series

Patch

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.