| 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 |
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
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,
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
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,
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 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
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,
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!
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
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.
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,
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
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 <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
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 --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.
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(+)