| Message ID | 20190717115817.30110-7-noralf@tronnes.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/tinydrm: Remove tinydrm.ko | expand |
Hi Noralf. On Wed, Jul 17, 2019 at 01:58:13PM +0200, Noralf Trønnes wrote: > This is only used by mipi-dbi drivers so move it there. > > The reason this isn't moved to the SPI subsystem is that it will in a > later patch pass a dummy rx buffer for SPI controllers that need this. > Low memory boards (64MB) can run into a problem allocating such a "large" > contiguous buffer on every transfer after a long up time. > This leaves a very specific use case, so we'll keep the function here. > mipi-dbi will first go through a refactoring though, before this will > be done. > > Remove SPI todo entry now that we're done with the tinydrm.ko SPI code. > > Additionally move the mipi_dbi_spi_init() declaration to the other SPI > functions. > > Cc: David Lechner <david@lechnology.com> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> With the few nitpics considered: Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h > index 51fc667beef7..576e9a7349ab 100644 > --- a/include/drm/tinydrm/mipi-dbi.h > +++ b/include/drm/tinydrm/mipi-dbi.h > @@ -67,8 +67,6 @@ static inline struct mipi_dbi *drm_to_mipi_dbi(struct drm_device *drm) > return container_of(drm, struct mipi_dbi, drm); > } > > -int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, > - struct gpio_desc *dc); Moving this prototype looks like it belongs in another patch? Sam
Den 17.07.2019 15.15, skrev Sam Ravnborg: > Hi Noralf. > > On Wed, Jul 17, 2019 at 01:58:13PM +0200, Noralf Trønnes wrote: >> This is only used by mipi-dbi drivers so move it there. >> >> The reason this isn't moved to the SPI subsystem is that it will in a >> later patch pass a dummy rx buffer for SPI controllers that need this. >> Low memory boards (64MB) can run into a problem allocating such a "large" >> contiguous buffer on every transfer after a long up time. >> This leaves a very specific use case, so we'll keep the function here. >> mipi-dbi will first go through a refactoring though, before this will >> be done. >> >> Remove SPI todo entry now that we're done with the tinydrm.ko SPI code. >> >> Additionally move the mipi_dbi_spi_init() declaration to the other SPI >> functions. >> >> Cc: David Lechner <david@lechnology.com> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > With the few nitpics considered: > Reviewed-by: Sam Ravnborg <sam@ravnborg.org> > > >> diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h >> index 51fc667beef7..576e9a7349ab 100644 >> --- a/include/drm/tinydrm/mipi-dbi.h >> +++ b/include/drm/tinydrm/mipi-dbi.h >> @@ -67,8 +67,6 @@ static inline struct mipi_dbi *drm_to_mipi_dbi(struct drm_device *drm) >> return container_of(drm, struct mipi_dbi, drm); >> } >> >> -int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, >> - struct gpio_desc *dc); > Moving this prototype looks like it belongs in another patch? > Strictly speaking it does, if you don't like I'll just drop it. Noralf.
On 7/17/19 6:58 AM, Noralf Trønnes wrote: > This is only used by mipi-dbi drivers so move it there. > > The reason this isn't moved to the SPI subsystem is that it will in a > later patch pass a dummy rx buffer for SPI controllers that need this. > Low memory boards (64MB) can run into a problem allocating such a "large" > contiguous buffer on every transfer after a long up time. > This leaves a very specific use case, so we'll keep the function here. > mipi-dbi will first go through a refactoring though, before this will > be done. > > Remove SPI todo entry now that we're done with the tinydrm.ko SPI code. > > Additionally move the mipi_dbi_spi_init() declaration to the other SPI > functions. > > Cc: David Lechner <david@lechnology.com> > Signed-off-by: Noralf Trønnes <noralf@tronnes.org> > --- Acked-by: : David Lechner <david@lechnology.com> I assume that the comments here might have something to do with the issue[1] I raised a while back? Should I dust that patch off and resend it after this series lands? [1]: https://lore.kernel.org/lkml/1519082461-32405-1-git-send-email-david@lechnology.com/
Den 17.07.2019 21.48, skrev David Lechner: > On 7/17/19 6:58 AM, Noralf Trønnes wrote: >> This is only used by mipi-dbi drivers so move it there. >> >> The reason this isn't moved to the SPI subsystem is that it will in a >> later patch pass a dummy rx buffer for SPI controllers that need this. >> Low memory boards (64MB) can run into a problem allocating such a "large" >> contiguous buffer on every transfer after a long up time. >> This leaves a very specific use case, so we'll keep the function here. >> mipi-dbi will first go through a refactoring though, before this will >> be done. >> >> Remove SPI todo entry now that we're done with the tinydrm.ko SPI code. >> >> Additionally move the mipi_dbi_spi_init() declaration to the other SPI >> functions. >> >> Cc: David Lechner <david@lechnology.com> >> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >> --- > > Acked-by: : David Lechner <david@lechnology.com> > > I assume that the comments here might have something to do with the > issue[1] I raised a while back? Should I dust that patch off and resend > it after this series lands? > > [1]: > https://lore.kernel.org/lkml/1519082461-32405-1-git-send-email-david@lechnology.com/ > Yep, that's the one. I want to refactor mipi-dbi first splitting struct mipi_dbi into an interface and display pipeline part. The helper is going to be moved to drivers/gpu/drm with the other helpers. Please wait until that is done, I want to see what kind of coupling I end up between the two structs and don't want another dependency to deal with if I can avoid it. Noralf.
Den 18.07.2019 14.14, skrev Noralf Trønnes: > > > Den 17.07.2019 21.48, skrev David Lechner: >> On 7/17/19 6:58 AM, Noralf Trønnes wrote: >>> This is only used by mipi-dbi drivers so move it there. >>> >>> The reason this isn't moved to the SPI subsystem is that it will in a >>> later patch pass a dummy rx buffer for SPI controllers that need this. >>> Low memory boards (64MB) can run into a problem allocating such a "large" >>> contiguous buffer on every transfer after a long up time. >>> This leaves a very specific use case, so we'll keep the function here. >>> mipi-dbi will first go through a refactoring though, before this will >>> be done. >>> >>> Remove SPI todo entry now that we're done with the tinydrm.ko SPI code. >>> >>> Additionally move the mipi_dbi_spi_init() declaration to the other SPI >>> functions. >>> >>> Cc: David Lechner <david@lechnology.com> >>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >>> --- >> >> Acked-by: : David Lechner <david@lechnology.com> >> >> I assume that the comments here might have something to do with the >> issue[1] I raised a while back? Should I dust that patch off and resend >> it after this series lands? >> >> [1]: >> https://lore.kernel.org/lkml/1519082461-32405-1-git-send-email-david@lechnology.com/ >> > > Yep, that's the one. I want to refactor mipi-dbi first splitting struct > mipi_dbi into an interface and display pipeline part. The helper is > going to be moved to drivers/gpu/drm with the other helpers. > Please wait until that is done, I want to see what kind of coupling I > end up between the two structs and don't want another dependency to deal > with if I can avoid it. > I've applied the series now. Do you have this problem only on the EV3 and with only one display/driver? If so I'm wondering if there's a way to implement this that doesn't affect the other drivers since you need a special use case to be hit by this. Noralf.
On 7/25/19 9:16 AM, Noralf Trønnes wrote: > > > Den 18.07.2019 14.14, skrev Noralf Trønnes: >> >> >> Den 17.07.2019 21.48, skrev David Lechner: >>> On 7/17/19 6:58 AM, Noralf Trønnes wrote: >>>> This is only used by mipi-dbi drivers so move it there. >>>> >>>> The reason this isn't moved to the SPI subsystem is that it will in a >>>> later patch pass a dummy rx buffer for SPI controllers that need this. >>>> Low memory boards (64MB) can run into a problem allocating such a "large" >>>> contiguous buffer on every transfer after a long up time. >>>> This leaves a very specific use case, so we'll keep the function here. >>>> mipi-dbi will first go through a refactoring though, before this will >>>> be done. >>>> >>>> Remove SPI todo entry now that we're done with the tinydrm.ko SPI code. >>>> >>>> Additionally move the mipi_dbi_spi_init() declaration to the other SPI >>>> functions. >>>> >>>> Cc: David Lechner <david@lechnology.com> >>>> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> >>>> --- >>> >>> Acked-by: : David Lechner <david@lechnology.com> >>> >>> I assume that the comments here might have something to do with the >>> issue[1] I raised a while back? Should I dust that patch off and resend >>> it after this series lands? >>> >>> [1]: >>> https://lore.kernel.org/lkml/1519082461-32405-1-git-send-email-david@lechnology.com/ >>> >> >> Yep, that's the one. I want to refactor mipi-dbi first splitting struct >> mipi_dbi into an interface and display pipeline part. The helper is >> going to be moved to drivers/gpu/drm with the other helpers. >> Please wait until that is done, I want to see what kind of coupling I >> end up between the two structs and don't want another dependency to deal >> with if I can avoid it. >> > > I've applied the series now. > > Do you have this problem only on the EV3 and with only one > display/driver? If so I'm wondering if there's a way to implement this > that doesn't affect the other drivers since you need a special use case > to be hit by this. > > Noralf. > I've let this sit running for several days and I don't see the error any more. So perhaps something was fixed in the DMA driver? Or maybe I just haven't seen the error because it is sitting idle and not under heavy memory usage? (I just ran some apt commands that crashed because of OOM and nothing happened with the display driver.) Interestingly, I was getting a similar allocation error from zram in the 5.2 kernel, usually triggered by starting a SSH session (totally unrelated to DRM). So, unless I start seeing the problem again, I think we can leave it alone for now.
diff --git a/Documentation/gpu/tinydrm.rst b/Documentation/gpu/tinydrm.rst index 33a41544f659..2c2860fa1510 100644 --- a/Documentation/gpu/tinydrm.rst +++ b/Documentation/gpu/tinydrm.rst @@ -11,9 +11,6 @@ Helpers .. kernel-doc:: include/drm/tinydrm/tinydrm-helpers.h :internal: -.. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c - :export: - .. kernel-doc:: drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c :export: diff --git a/Documentation/gpu/todo.rst b/Documentation/gpu/todo.rst index 3f6ecf846263..384199325304 100644 --- a/Documentation/gpu/todo.rst +++ b/Documentation/gpu/todo.rst @@ -456,9 +456,6 @@ tinydrm Tinydrm is the helper driver for really simple fb drivers. The goal is to make those drivers as simple as possible, so lots of room for refactoring: -- spi helpers, probably best put into spi core/helper code. Thierry said - the spi maintainer is fast&reactive, so shouldn't be a big issue. - - extract the mipi-dbi helper (well, the non-tinydrm specific parts at least) into a separate helper, like we have for mipi-dsi already. Or follow one of the ideas for having a shared dsi/dbi helper, abstracting away the diff --git a/drivers/gpu/drm/tinydrm/core/Makefile b/drivers/gpu/drm/tinydrm/core/Makefile index 01065e920aea..78e179127e55 100644 --- a/drivers/gpu/drm/tinydrm/core/Makefile +++ b/drivers/gpu/drm/tinydrm/core/Makefile @@ -1,4 +1,4 @@ # SPDX-License-Identifier: GPL-2.0-only -tinydrm-y := tinydrm-pipe.o tinydrm-helpers.o +tinydrm-y := tinydrm-pipe.o obj-$(CONFIG_DRM_TINYDRM) += tinydrm.o diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c b/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c deleted file mode 100644 index d95eb50fa9d4..000000000000 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c +++ /dev/null @@ -1,70 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0-or-later -/* - * Copyright (C) 2016 Noralf Trønnes - */ - -#include <linux/backlight.h> -#include <linux/dma-buf.h> -#include <linux/module.h> -#include <linux/pm.h> -#include <linux/spi/spi.h> -#include <linux/swab.h> - -#include <drm/drm_device.h> -#include <drm/drm_drv.h> -#include <drm/drm_fourcc.h> -#include <drm/drm_framebuffer.h> -#include <drm/drm_print.h> -#include <drm/drm_rect.h> -#include <drm/tinydrm/tinydrm-helpers.h> - -#if IS_ENABLED(CONFIG_SPI) - -/** - * tinydrm_spi_transfer - SPI transfer helper - * @spi: SPI device - * @speed_hz: Override speed (optional) - * @bpw: Bits per word - * @buf: Buffer to transfer - * @len: Buffer length - * - * This SPI transfer helper breaks up the transfer of @buf into chunks which - * the SPI controller driver can handle. - * - * Returns: - * Zero on success, negative error code on failure. - */ -int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, - u8 bpw, const void *buf, size_t len) -{ - size_t max_chunk = spi_max_transfer_size(spi); - struct spi_transfer tr = { - .bits_per_word = bpw, - .speed_hz = speed_hz, - }; - struct spi_message m; - size_t chunk; - int ret; - - spi_message_init_with_transfers(&m, &tr, 1); - - while (len) { - chunk = min(len, max_chunk); - - tr.tx_buf = buf; - tr.len = chunk; - buf += chunk; - len -= chunk; - - ret = spi_sync(spi, &m); - if (ret) - return ret; - } - - return 0; -} -EXPORT_SYMBOL(tinydrm_spi_transfer); - -#endif /* CONFIG_SPI */ - -MODULE_LICENSE("GPL"); diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c index ed798fd95152..a62d1dfe87f8 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c @@ -3,6 +3,8 @@ * Copyright (C) 2016 Noralf Trønnes */ +#include <linux/module.h> + #include <drm/drm_atomic_helper.h> #include <drm/drm_drv.h> #include <drm/drm_gem_framebuffer_helper.h> @@ -177,3 +179,5 @@ int tinydrm_display_pipe_init(struct drm_device *drm, format_count, modifiers, connector); } EXPORT_SYMBOL(tinydrm_display_pipe_init); + +MODULE_LICENSE("GPL"); diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c index 21677e3ed38b..62f29b2faf74 100644 --- a/drivers/gpu/drm/tinydrm/ili9225.c +++ b/drivers/gpu/drm/tinydrm/ili9225.c @@ -27,7 +27,6 @@ #include <drm/drm_rect.h> #include <drm/drm_vblank.h> #include <drm/tinydrm/mipi-dbi.h> -#include <drm/tinydrm/tinydrm-helpers.h> #define ILI9225_DRIVER_READ_CODE 0x00 #define ILI9225_DRIVER_OUTPUT_CONTROL 0x01 @@ -323,7 +322,7 @@ static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par, gpiod_set_value_cansleep(mipi->dc, 0); speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1); - ret = tinydrm_spi_transfer(spi, speed_hz, 8, cmd, 1); + ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, cmd, 1); if (ret || !num) return ret; @@ -333,7 +332,7 @@ static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par, gpiod_set_value_cansleep(mipi->dc, 1); speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num); - return tinydrm_spi_transfer(spi, speed_hz, bpw, par, num); + return mipi_dbi_spi_transfer(spi, speed_hz, bpw, par, num); } static const struct drm_simple_display_pipe_funcs ili9225_pipe_funcs = { diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c index 8fb6ce4ca6fc..6a8f2d66377f 100644 --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c @@ -926,7 +926,7 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd, gpiod_set_value_cansleep(mipi->dc, 0); speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1); - ret = tinydrm_spi_transfer(spi, speed_hz, 8, cmd, 1); + ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, cmd, 1); if (ret || !num) return ret; @@ -936,7 +936,7 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd, gpiod_set_value_cansleep(mipi->dc, 1); speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num); - return tinydrm_spi_transfer(spi, speed_hz, bpw, par, num); + return mipi_dbi_spi_transfer(spi, speed_hz, bpw, par, num); } /** @@ -1007,6 +1007,51 @@ int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, } EXPORT_SYMBOL(mipi_dbi_spi_init); +/** + * mipi_dbi_spi_transfer - SPI transfer helper + * @spi: SPI device + * @speed_hz: Override speed (optional) + * @bpw: Bits per word + * @buf: Buffer to transfer + * @len: Buffer length + * + * This SPI transfer helper breaks up the transfer of @buf into chunks which + * the SPI controller driver can handle. + * + * Returns: + * Zero on success, negative error code on failure. + */ +int mipi_dbi_spi_transfer(struct spi_device *spi, u32 speed_hz, + u8 bpw, const void *buf, size_t len) +{ + size_t max_chunk = spi_max_transfer_size(spi); + struct spi_transfer tr = { + .bits_per_word = bpw, + .speed_hz = speed_hz, + }; + struct spi_message m; + size_t chunk; + int ret; + + spi_message_init_with_transfers(&m, &tr, 1); + + while (len) { + chunk = min(len, max_chunk); + + tr.tx_buf = buf; + tr.len = chunk; + buf += chunk; + len -= chunk; + + ret = spi_sync(spi, &m); + if (ret) + return ret; + } + + return 0; +} +EXPORT_SYMBOL(mipi_dbi_spi_transfer); + #endif /* CONFIG_SPI */ #ifdef CONFIG_DEBUG_FS diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h index 51fc667beef7..576e9a7349ab 100644 --- a/include/drm/tinydrm/mipi-dbi.h +++ b/include/drm/tinydrm/mipi-dbi.h @@ -67,8 +67,6 @@ static inline struct mipi_dbi *drm_to_mipi_dbi(struct drm_device *drm) return container_of(drm, struct mipi_dbi, drm); } -int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, - struct gpio_desc *dc); int mipi_dbi_init(struct mipi_dbi *mipi, const struct drm_simple_display_pipe_funcs *funcs, const struct drm_display_mode *mode, unsigned int rotation); @@ -83,7 +81,12 @@ void mipi_dbi_hw_reset(struct mipi_dbi *mipi); bool mipi_dbi_display_is_on(struct mipi_dbi *mipi); int mipi_dbi_poweron_reset(struct mipi_dbi *mipi); int mipi_dbi_poweron_conditional_reset(struct mipi_dbi *mipi); + u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, size_t len); +int mipi_dbi_spi_init(struct spi_device *spi, struct mipi_dbi *mipi, + struct gpio_desc *dc); +int mipi_dbi_spi_transfer(struct spi_device *spi, u32 speed_hz, + u8 bpw, const void *buf, size_t len); int mipi_dbi_command_read(struct mipi_dbi *mipi, u8 cmd, u8 *val); int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len); diff --git a/include/drm/tinydrm/tinydrm-helpers.h b/include/drm/tinydrm/tinydrm-helpers.h index 708c5a7d51e0..8c5d20efeaa1 100644 --- a/include/drm/tinydrm/tinydrm-helpers.h +++ b/include/drm/tinydrm/tinydrm-helpers.h @@ -13,8 +13,6 @@ struct drm_framebuffer; struct drm_rect; struct drm_simple_display_pipe; struct drm_simple_display_pipe_funcs; -struct spi_transfer; -struct spi_device; struct device; /** @@ -41,7 +39,4 @@ int tinydrm_display_pipe_init(struct drm_device *drm, const struct drm_display_mode *mode, unsigned int rotation); -int tinydrm_spi_transfer(struct spi_device *spi, u32 speed_hz, - u8 bpw, const void *buf, size_t len); - #endif /* __LINUX_TINYDRM_HELPERS_H */
This is only used by mipi-dbi drivers so move it there. The reason this isn't moved to the SPI subsystem is that it will in a later patch pass a dummy rx buffer for SPI controllers that need this. Low memory boards (64MB) can run into a problem allocating such a "large" contiguous buffer on every transfer after a long up time. This leaves a very specific use case, so we'll keep the function here. mipi-dbi will first go through a refactoring though, before this will be done. Remove SPI todo entry now that we're done with the tinydrm.ko SPI code. Additionally move the mipi_dbi_spi_init() declaration to the other SPI functions. Cc: David Lechner <david@lechnology.com> Signed-off-by: Noralf Trønnes <noralf@tronnes.org> --- Documentation/gpu/tinydrm.rst | 3 - Documentation/gpu/todo.rst | 3 - drivers/gpu/drm/tinydrm/core/Makefile | 2 +- .../gpu/drm/tinydrm/core/tinydrm-helpers.c | 70 ------------------- drivers/gpu/drm/tinydrm/core/tinydrm-pipe.c | 4 ++ drivers/gpu/drm/tinydrm/ili9225.c | 5 +- drivers/gpu/drm/tinydrm/mipi-dbi.c | 49 ++++++++++++- include/drm/tinydrm/mipi-dbi.h | 7 +- include/drm/tinydrm/tinydrm-helpers.h | 5 -- 9 files changed, 59 insertions(+), 89 deletions(-) delete mode 100644 drivers/gpu/drm/tinydrm/core/tinydrm-helpers.c