All of lore.kernel.org
 help / color / mirror / Atom feed
From: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
To: "Noralf Trønnes" <noralf@tronnes.org>
Cc: david@lechnology.com, dri-devel@lists.freedesktop.org
Subject: Re: [PATCH] tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers
Date: Fri, 22 Feb 2019 17:58:01 +0200	[thread overview]
Message-ID: <20190222155801.GI9224@smile.fi.intel.com> (raw)
In-Reply-To: <20190222124329.23046-1-noralf@tronnes.org>

On Fri, Feb 22, 2019 at 01:43:29PM +0100, Noralf Trønnes wrote:
> Buffers passed to spi_sync() must be dma-safe even for tiny buffers since
> some SPI controllers use DMA for all transfers.
> 
> Example splat with CONFIG_DMA_API_DEBUG enabled:
> 
> [   23.750467] DMA-API: dw_dmac_pci 0000:00:15.0: device driver maps memory from stack [probable addr=000000001e49185d]
> [   23.750529] WARNING: CPU: 1 PID: 1296 at kernel/dma/debug.c:1161 check_for_stack+0xb7/0x190
> [   23.750533] Modules linked in: mmc_block(+) spi_pxa2xx_platform(+) pwm_lpss_pci pwm_lpss spi_pxa2xx_pci sdhci_pci cqhci intel_mrfld_pwrbtn extcon_intel_mrfld sdhci intel_mrfld_adc led_class mmc_core ili9341 mipi_dbi tinydrm backlight ti_ads7950 industrialio_triggered_buffer kfifo_buf intel_soc_pmic_mrfld hci_uart btbcm
> [   23.750599] CPU: 1 PID: 1296 Comm: modprobe Not tainted 5.0.0-rc7+ #236
> [   23.750605] Hardware name: Intel Corporation Merrifield/BODEGA BAY, BIOS 542 2015.01.21:18.19.48
> [   23.750620] RIP: 0010:check_for_stack+0xb7/0x190
> [   23.750630] Code: 8b 6d 50 4d 85 ed 75 04 4c 8b 6d 10 48 89 ef e8 2f 8b 44 00 48 89 c6 4a 8d 0c 23 4c 89 ea 48 c7 c7 88 d0 82 b4 e8 40 7c f9 ff <0f> 0b 8b 05 79 00 4b 01 85 c0 74 07 5b 5d 41 5c 41 5d c3 8b 05 54
> [   23.750637] RSP: 0000:ffff97bbc0292fa0 EFLAGS: 00010286
> [   23.750646] RAX: 0000000000000000 RBX: ffff97bbc0290000 RCX: 0000000000000006
> [   23.750652] RDX: 0000000000000007 RSI: 0000000000000002 RDI: ffff94b33e115450
> [   23.750658] RBP: ffff94b33c8578b0 R08: 0000000000000002 R09: 00000000000201c0
> [   23.750664] R10: 00000006ecb0ccc6 R11: 0000000000034f38 R12: 000000000000316c
> [   23.750670] R13: ffff94b33c84b250 R14: ffff94b33dedd5a0 R15: 0000000000000001
> [   23.750679] FS:  0000000000000000(0000) GS:ffff94b33e100000(0063) knlGS:00000000f7faf690
> [   23.750686] CS:  0010 DS: 002b ES: 002b CR0: 0000000080050033
> [   23.750691] CR2: 00000000f7f54faf CR3: 000000000722c000 CR4: 00000000001006e0
> [   23.750696] Call Trace:
> [   23.750713]  debug_dma_map_sg+0x100/0x340
> [   23.750727]  ? dma_direct_map_sg+0x3b/0xb0
> [   23.750739]  spi_map_buf+0x25a/0x300
> [   23.750751]  __spi_pump_messages+0x2a4/0x680
> [   23.750762]  __spi_sync+0x1dd/0x1f0
> [   23.750773]  spi_sync+0x26/0x40
> [   23.750790]  mipi_dbi_typec3_command_read+0x14d/0x240 [mipi_dbi]
> [   23.750802]  ? spi_finalize_current_transfer+0x10/0x10
> [   23.750821]  mipi_dbi_typec3_command+0x1bc/0x1d0 [mipi_dbi]
> 

After few runs I don't see the warning anymore. So,
Tested-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>

Though I would like to give few more days to monitor the state.
(I believe it's fixed)

> Reported-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Signed-off-by: Noralf Trønnes <noralf@tronnes.org>
> ---
>  drivers/gpu/drm/tinydrm/ili9225.c  |  6 ++--
>  drivers/gpu/drm/tinydrm/mipi-dbi.c | 58 +++++++++++++++++++++---------
>  include/drm/tinydrm/mipi-dbi.h     |  5 +--
>  3 files changed, 48 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/gpu/drm/tinydrm/ili9225.c b/drivers/gpu/drm/tinydrm/ili9225.c
> index 3f59cfbd31ba..a87078667e04 100644
> --- a/drivers/gpu/drm/tinydrm/ili9225.c
> +++ b/drivers/gpu/drm/tinydrm/ili9225.c
> @@ -299,7 +299,7 @@ static void ili9225_pipe_disable(struct drm_simple_display_pipe *pipe)
>  	mipi->enabled = false;
>  }
>  
> -static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 cmd, u8 *par,
> +static int ili9225_dbi_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par,
>  			       size_t num)
>  {
>  	struct spi_device *spi = mipi->spi;
> @@ -309,11 +309,11 @@ 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, NULL, 8, &cmd, 1);
> +	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
>  	if (ret || !num)
>  		return ret;
>  
> -	if (cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
> +	if (*cmd == ILI9225_WRITE_DATA_TO_GRAM && !mipi->swap_bytes)
>  		bpw = 16;
>  
>  	gpiod_set_value_cansleep(mipi->dc, 1);
> diff --git a/drivers/gpu/drm/tinydrm/mipi-dbi.c b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> index 246e708a9ff7..4a4cd7e72938 100644
> --- a/drivers/gpu/drm/tinydrm/mipi-dbi.c
> +++ b/drivers/gpu/drm/tinydrm/mipi-dbi.c
> @@ -153,16 +153,42 @@ EXPORT_SYMBOL(mipi_dbi_command_read);
>   */
>  int mipi_dbi_command_buf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
>  {
> +	u8 *cmdbuf;
>  	int ret;
>  
> +	/* SPI requires dma-safe buffers */
> +	cmdbuf = kmemdup(&cmd, 1, GFP_KERNEL);
> +	if (!cmdbuf)
> +		return -ENOMEM;
> +
>  	mutex_lock(&mipi->cmdlock);
> -	ret = mipi->command(mipi, cmd, data, len);
> +	ret = mipi->command(mipi, cmdbuf, data, len);
>  	mutex_unlock(&mipi->cmdlock);
>  
> +	kfree(cmdbuf);
> +
>  	return ret;
>  }
>  EXPORT_SYMBOL(mipi_dbi_command_buf);
>  
> +/* This should only be used by mipi_dbi_command() */
> +int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len)
> +{
> +	u8 *buf;
> +	int ret;
> +
> +	buf = kmemdup(data, len, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +
> +	ret = mipi_dbi_command_buf(mipi, cmd, buf, len);
> +
> +	kfree(buf);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL(mipi_dbi_command_stackbuf);
> +
>  /**
>   * mipi_dbi_buf_copy - Copy a framebuffer, transforming it if necessary
>   * @dst: The destination buffer
> @@ -772,18 +798,18 @@ static int mipi_dbi_spi1_transfer(struct mipi_dbi *mipi, int dc,
>  	return 0;
>  }
>  
> -static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
> +static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 *cmd,
>  				   u8 *parameters, size_t num)
>  {
> -	unsigned int bpw = (cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
> +	unsigned int bpw = (*cmd == MIPI_DCS_WRITE_MEMORY_START) ? 16 : 8;
>  	int ret;
>  
> -	if (mipi_dbi_command_is_read(mipi, cmd))
> +	if (mipi_dbi_command_is_read(mipi, *cmd))
>  		return -ENOTSUPP;
>  
> -	MIPI_DBI_DEBUG_COMMAND(cmd, parameters, num);
> +	MIPI_DBI_DEBUG_COMMAND(*cmd, parameters, num);
>  
> -	ret = mipi_dbi_spi1_transfer(mipi, 0, &cmd, 1, 8);
> +	ret = mipi_dbi_spi1_transfer(mipi, 0, cmd, 1, 8);
>  	if (ret || !num)
>  		return ret;
>  
> @@ -792,7 +818,7 @@ static int mipi_dbi_typec1_command(struct mipi_dbi *mipi, u8 cmd,
>  
>  /* MIPI DBI Type C Option 3 */
>  
> -static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
> +static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 *cmd,
>  					u8 *data, size_t len)
>  {
>  	struct spi_device *spi = mipi->spi;
> @@ -801,7 +827,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>  	struct spi_transfer tr[2] = {
>  		{
>  			.speed_hz = speed_hz,
> -			.tx_buf = &cmd,
> +			.tx_buf = cmd,
>  			.len = 1,
>  		}, {
>  			.speed_hz = speed_hz,
> @@ -819,8 +845,8 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>  	 * Support non-standard 24-bit and 32-bit Nokia read commands which
>  	 * start with a dummy clock, so we need to read an extra byte.
>  	 */
> -	if (cmd == MIPI_DCS_GET_DISPLAY_ID ||
> -	    cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
> +	if (*cmd == MIPI_DCS_GET_DISPLAY_ID ||
> +	    *cmd == MIPI_DCS_GET_DISPLAY_STATUS) {
>  		if (!(len == 3 || len == 4))
>  			return -EINVAL;
>  
> @@ -850,7 +876,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>  			data[i] = (buf[i] << 1) | !!(buf[i + 1] & BIT(7));
>  	}
>  
> -	MIPI_DBI_DEBUG_COMMAND(cmd, data, len);
> +	MIPI_DBI_DEBUG_COMMAND(*cmd, data, len);
>  
>  err_free:
>  	kfree(buf);
> @@ -858,7 +884,7 @@ static int mipi_dbi_typec3_command_read(struct mipi_dbi *mipi, u8 cmd,
>  	return ret;
>  }
>  
> -static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
> +static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 *cmd,
>  				   u8 *par, size_t num)
>  {
>  	struct spi_device *spi = mipi->spi;
> @@ -866,18 +892,18 @@ static int mipi_dbi_typec3_command(struct mipi_dbi *mipi, u8 cmd,
>  	u32 speed_hz;
>  	int ret;
>  
> -	if (mipi_dbi_command_is_read(mipi, cmd))
> +	if (mipi_dbi_command_is_read(mipi, *cmd))
>  		return mipi_dbi_typec3_command_read(mipi, cmd, par, num);
>  
> -	MIPI_DBI_DEBUG_COMMAND(cmd, par, num);
> +	MIPI_DBI_DEBUG_COMMAND(*cmd, par, num);
>  
>  	gpiod_set_value_cansleep(mipi->dc, 0);
>  	speed_hz = mipi_dbi_spi_cmd_max_speed(spi, 1);
> -	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, &cmd, 1);
> +	ret = tinydrm_spi_transfer(spi, speed_hz, NULL, 8, cmd, 1);
>  	if (ret || !num)
>  		return ret;
>  
> -	if (cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
> +	if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes)
>  		bpw = 16;
>  
>  	gpiod_set_value_cansleep(mipi->dc, 1);
> diff --git a/include/drm/tinydrm/mipi-dbi.h b/include/drm/tinydrm/mipi-dbi.h
> index ad7e6bedab5f..464a47d1013d 100644
> --- a/include/drm/tinydrm/mipi-dbi.h
> +++ b/include/drm/tinydrm/mipi-dbi.h
> @@ -43,7 +43,7 @@ struct mipi_dbi {
>  	struct spi_device *spi;
>  	bool enabled;
>  	struct mutex cmdlock;
> -	int (*command)(struct mipi_dbi *mipi, u8 cmd, u8 *param, size_t num);
> +	int (*command)(struct mipi_dbi *mipi, u8 *cmd, u8 *param, size_t num);
>  	const u8 *read_commands;
>  	struct gpio_desc *dc;
>  	u16 *tx_buf;
> @@ -83,6 +83,7 @@ u32 mipi_dbi_spi_cmd_max_speed(struct spi_device *spi, 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);
> +int mipi_dbi_command_stackbuf(struct mipi_dbi *mipi, u8 cmd, u8 *data, size_t len);
>  int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>  		      struct drm_rect *clip, bool swap);
>  /**
> @@ -100,7 +101,7 @@ int mipi_dbi_buf_copy(void *dst, struct drm_framebuffer *fb,
>  #define mipi_dbi_command(mipi, cmd, seq...) \
>  ({ \
>  	u8 d[] = { seq }; \
> -	mipi_dbi_command_buf(mipi, cmd, d, ARRAY_SIZE(d)); \
> +	mipi_dbi_command_stackbuf(mipi, cmd, d, ARRAY_SIZE(d)); \
>  })
>  
>  #ifdef CONFIG_DEBUG_FS
> -- 
> 2.20.1
> 

-- 
With Best Regards,
Andy Shevchenko


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

  reply	other threads:[~2019-02-22 15:58 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-02-22 12:43 [PATCH] tinydrm/mipi-dbi: Use dma-safe buffers for all SPI transfers Noralf Trønnes
2019-02-22 15:58 ` Andy Shevchenko [this message]
2019-03-04 14:45   ` Noralf Trønnes
2019-03-04 15:10     ` Andy Shevchenko
2019-03-04 17:51       ` Noralf Trønnes
2019-03-05  7:32         ` Andy Shevchenko

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190222155801.GI9224@smile.fi.intel.com \
    --to=andriy.shevchenko@linux.intel.com \
    --cc=david@lechnology.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=noralf@tronnes.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.