Message ID | 20221116-s905x_spi_ili9486-v1-2-630401cb62d5@baylibre.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:adf:f944:0:0:0:0:0 with SMTP id q4csp285960wrr; Thu, 17 Nov 2022 00:49:03 -0800 (PST) X-Google-Smtp-Source: AA0mqf7PZts7ZmZ/mAzO8KxtyIsNeqHOcHnShMsoS3rUbTG76VeUa0m+PnXCMt2DIACawhJQ54Pb X-Received: by 2002:a17:906:814f:b0:7b2:86d6:292d with SMTP id z15-20020a170906814f00b007b286d6292dmr1253322ejw.714.1668674943243; Thu, 17 Nov 2022 00:49:03 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668674943; cv=none; d=google.com; s=arc-20160816; b=JAT2239D0ibTA5WxNRwK0rkupgkF8BMxxyjhWmR/KUe5k+qfduFPHsdYNTtEHhVahq uhjr180Uh6/PCynUtEN//+eWEon9zv/EQhVGbnbVS1sppEopcj7F8KgRKiqmeZOodCEZ rbaeGSBwKdE3XGeAh390RgD9aOmNi3dXRC/Erp134FlPR2hPoWJVaC6I1Ujm9vwQAMpD xFesBOvALZEFjTlp0CVSsNBE5hfsc9s4nl3bX6hso980lINNkrHGEm23giuqmRQPZcco mRGOGHS7Q2egTtCqVYpWKiTP+ugIPFMEeaSTbH4qexITzT8w3tbG4RPDD1EKORvk1/Vw EEOA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:in-reply-to:references:message-id :content-transfer-encoding:mime-version:subject:date:from :dkim-signature; bh=90zm8/9EgVKMcm+GcVTwlECGEPSG+I41CMXM1RTutaE=; b=z0NXNgu3s+4oL2psYz90MI1cjJvLe6nVEgoST04oNlalL+x/NsH0YyREo5a1BKRFxo +q/SQoSqYgina4j69YHyHhWQmenReMVTc3pMKAfzlQ9Y+6S3EN6U62rBled9tRwUATtX 1R5IGDkjSnuFHGfRC9uuLVQmsLz48JpcthAIUSg4D0j5CCtcz/5/vi+a1N/WjrzCqVHA vZtPPDqQ8yLf2wvcIp/7YaPg/O59+WYll93quZ7Bvf36pqs3dG+llwke74BU5ffOEnrm sBBu4C2jI02q+piXOje6w4w/E288EPKAxSuiku5k2uN04bJFYpbXRqXyaY1RJinoOXVD hDGg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@baylibre-com.20210112.gappssmtp.com header.s=20210112 header.b=UnkavLeF; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id et19-20020a170907295300b0078d9cec6a5asi105216ejc.191.2022.11.17.00.48.39; Thu, 17 Nov 2022 00:49:03 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@baylibre-com.20210112.gappssmtp.com header.s=20210112 header.b=UnkavLeF; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S239665AbiKQIsC (ORCPT <rfc822;anhang610@gmail.com> + 99 others); Thu, 17 Nov 2022 03:48:02 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48430 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S239401AbiKQIr4 (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 17 Nov 2022 03:47:56 -0500 Received: from mail-ed1-x52b.google.com (mail-ed1-x52b.google.com [IPv6:2a00:1450:4864:20::52b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E45E9101F0 for <linux-kernel@vger.kernel.org>; Thu, 17 Nov 2022 00:47:54 -0800 (PST) Received: by mail-ed1-x52b.google.com with SMTP id a5so1501260edb.11 for <linux-kernel@vger.kernel.org>; Thu, 17 Nov 2022 00:47:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=baylibre-com.20210112.gappssmtp.com; s=20210112; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=90zm8/9EgVKMcm+GcVTwlECGEPSG+I41CMXM1RTutaE=; b=UnkavLeF63kWpP4dt3DH3oo2hhLFs7VsnslG4t92oUQapu5uMsW0U6XFO9MEi+swRX /ohsTvFbJwQ0scQ1IYNcwQ/z036JyCSL0TqusFH7ceidILUrvk9jSNNs0P9hFMLeNW+V FGnwjC0/0OhODR4CMHeGQJEp+HL0G5XlSqf8wbQ4bMqMUukKA+CgfLE/W1wbO98Xf7iN k06oxVsEINbbzZzifkn9cqEmMGs7wwacRRZd+8Ve+L2FKO7hiz1X87dxyTl4uoSBMU72 YNLwzVDpkjjxtc3OOUB8AyHI7mAPRzIMpufnKutmnF3+8449KHQfp5KX81vW6GMUPufg IU7g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=90zm8/9EgVKMcm+GcVTwlECGEPSG+I41CMXM1RTutaE=; b=rBdN8QRBbWRxqa3XmvqVC9sUn8TU+6dsITy6/mpq3YnUO234PlcrmN/V/qx8+z8tPV JfXeRKR8f8HD0xrkfP/KGt0XGeqWwuzur4/DgLHC7xEd7OgXSTumkbQYWg3vfo8Qpqt+ XK7AQ+9F3lvD8gxEwU1bOjKytUgDpexBn/b31KCqfp0+f2oR+8aiLAvRF830OgsFE5JT 5I3YRUCQwnhGKMLlyvtjk9kZc+2RubjJHf+FhT6+QU8t667FZ96I17q6hLob8W2kHoWC 6so6roc/TnHQHd8kPKFxdrMziGJFC7QYnhs8McAaN19tZWuJwhbuCMr0o7jzdEG51dZ7 9HZw== X-Gm-Message-State: ANoB5pnOr7wAL1JeRFAbJz9tUXGbi+Y1loycTArH+WQBlZQgUOJI1NcM fbjlwbGq0cfw/BjUrkSN6CUERg== X-Received: by 2002:aa7:d1c5:0:b0:461:dd11:c1bd with SMTP id g5-20020aa7d1c5000000b00461dd11c1bdmr1207044edp.406.1668674873463; Thu, 17 Nov 2022 00:47:53 -0800 (PST) Received: from [127.0.1.1] ([2a0e:41a:894f:0:7a60:27eb:954c:5ab1]) by smtp.gmail.com with ESMTPSA id s22-20020a1709067b9600b0077205dd15basm77332ejo.66.2022.11.17.00.47.52 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 17 Nov 2022 00:47:53 -0800 (PST) From: Carlo Caione <ccaione@baylibre.com> Date: Thu, 17 Nov 2022 09:47:40 +0100 Subject: [PATCH 2/3] drm/tiny: ili9486: Do not assume 8-bit only SPI controllers MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit Message-Id: <20221116-s905x_spi_ili9486-v1-2-630401cb62d5@baylibre.com> References: <20221116-s905x_spi_ili9486-v1-0-630401cb62d5@baylibre.com> In-Reply-To: <20221116-s905x_spi_ili9486-v1-0-630401cb62d5@baylibre.com> To: Kamlesh Gurudasani <kamlesh.gurudasani@gmail.com>, Mark Brown <broonie@kernel.org>, Neil Armstrong <neil.armstrong@linaro.org>, Jerome Brunet <jbrunet@baylibre.com>, David Airlie <airlied@gmail.com>, Martin Blumenstingl <martin.blumenstingl@googlemail.com>, Kevin Hilman <khilman@baylibre.com>, Daniel Vetter <daniel@ffwll.ch> Cc: linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org, linux-spi@vger.kernel.org, dri-devel@lists.freedesktop.org, Carlo Caione <ccaione@baylibre.com> X-Mailer: b4 0.10.1 X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749732497110556974?= X-GMAIL-MSGID: =?utf-8?q?1749732497110556974?= |
Series |
Fix SPICC and ILI9486 drivers
|
|
Commit Message
Carlo Caione
Nov. 17, 2022, 8:47 a.m. UTC
The ILI9486 driver is wrongly assuming that the SPI panel is interfaced
only with 8-bit SPI controllers and consequently that the pixel data
passed by the MIPI DBI subsystem are already swapped before being sent
over SPI using 8 bits-per-word.
This is not always true for all the SPI controllers.
Make the command function more general to not only support 8-bit only
SPI controllers and support sending un-swapped data over SPI using 16
bits-per-word when dealing with pixel data.
Signed-off-by: Carlo Caione <ccaione@baylibre.com>
---
drivers/gpu/drm/tiny/ili9486.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
Comments
On Thu, Nov 17, 2022 at 09:47:40AM +0100, Carlo Caione wrote: > The ILI9486 driver is wrongly assuming that the SPI panel is interfaced > only with 8-bit SPI controllers and consequently that the pixel data > passed by the MIPI DBI subsystem are already swapped before being sent > over SPI using 8 bits-per-word. > > This is not always true for all the SPI controllers. > > Make the command function more general to not only support 8-bit only > SPI controllers and support sending un-swapped data over SPI using 16 > bits-per-word when dealing with pixel data. I don't understand what the commit log is saying here. The meson-spicc driver advertises support for 8 bit words, if the driver is sending data formatted as a byte stream everything should be fine. It may be that there is some optimisation available from taking advantage of the hardware's ability to handle larger word sizes but there should be no data corruption issue. > + /* > + * Check whether pixel data bytes needs to be swapped or not > + */ > + if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes) > + bpw = 16; > + You should check the SPI controller compatibility here.
On 17/11/2022 12:09, Mark Brown wrote: > I don't understand what the commit log is saying here. The > meson-spicc driver advertises support for 8 bit words, if the driver > is sending data formatted as a byte stream everything should be fine. > It may be that there is some optimisation available from taking > advantage of the hardware's ability to handle larger word sizes but > there should be no data corruption issue. There is no data corruption but the 16-bit pixel data have per-pixel bytes swapped: for example 0x55AD is sent instead of 0xAD55 and this is causing the wrong color to be displayed on the panel. The problem is that the current code is sending data with an hardcoded bpw == 8 whether the data is swapped or not before the sending. For 8-bit only controllers the data is swapped by the MIPI DBI code but this is not true for controllers supporting 16-bit as well, but in both cases we are sending the data out the same way with an 8 bpw. So the same image is basically displayed differently whether the SPI controller supports 16 bpw or not. I'm trying to fix this by sending data with 16-bit bpw when the controller is supporting that. Please note that this is what it is done also by mipi_dbi_typec3_command(). >> + /* + * Check whether pixel data bytes needs to be swapped or not >> + */ + if (*cmd == MIPI_DCS_WRITE_MEMORY_START && >> !mipi->swap_bytes) + bpw = 16; + > > You should check the SPI controller compatibility here. This is already done in mipi_dbi_spi_init() by using spi_is_bpw_supported(). Cheers, -- Carlo Caione
On Thu, Nov 17, 2022 at 02:40:05PM +0100, Carlo Caione wrote: > On 17/11/2022 12:09, Mark Brown wrote: > > I don't understand what the commit log is saying here. The meson-spicc > > driver advertises support for 8 bit words, if the driver is sending data > > formatted as a byte stream everything should be fine. > > It may be that there is some optimisation available from taking > > advantage of the hardware's ability to handle larger word sizes but > > there should be no data corruption issue. > There is no data corruption but the 16-bit pixel data have per-pixel > bytes swapped: for example 0x55AD is sent instead of 0xAD55 and this is > causing the wrong color to be displayed on the panel. If the data is being unexpectedly byte swapped then clearly it is being corrupted. How is this byte swapping happening? SPI devices should default to doing 8 bit transfers, if things randomly get put into anything other than 8 bit mode without the client device explicitly asking for it then that seems really bad. > The problem is that the current code is sending data with an hardcoded > bpw == 8 whether the data is swapped or not before the sending. > For 8-bit only controllers the data is swapped by the MIPI DBI code but > this is not true for controllers supporting 16-bit as well, but in both > cases we are sending the data out the same way with an 8 bpw. > So the same image is basically displayed differently whether the SPI > controller supports 16 bpw or not. I'm trying to fix this by sending > data with 16-bit bpw when the controller is supporting that. So this is an issue in the MIPI DBI code where the interpretation of the buffer passed in depends on both the a caller parameter and the capabilities of the underlying SPI controller, meaning that a driver can suddenly become buggy when used with a new controller? I can't really tell what the bits per word being passed in along with the buffer is supposed to mean, I'd have expected it to correspond to the format of the buffer but it seems like perhaps the buffer is always formatted for 16 bits and the callers are needing to pass in the capabilities of the controller which is then also checked by the underlying code? This all seems extremely confusing, I'm not surprised there's bugs. At the very least your changelog needs to express clearly what is going on, the description doesn't appear to correspond to what you're changing. > Please note that this is what it is done also by mipi_dbi_typec3_command(). The core code does appear to have some checks for controller capabilities...
On 17/11/2022 15:59, Mark Brown wrote: > So this is an issue in the MIPI DBI code where the interpretation of > the buffer passed in depends on both the a caller parameter and the > capabilities of the underlying SPI controller, meaning that a driver > can suddenly become buggy when used with a new controller? The MIPI DBI code is fine, in fact it is doing the correct thing in the mipi_dbi_typec3_command() function. The problem is that the ILI9486 driver is hijacking that function installing its own hook that is wrong. > I can't really tell what the bits per word being passed in along > with the buffer is supposed to mean, I'd have expected it to > correspond to the format of the buffer but it seems like perhaps the > buffer is always formatted for 16 bits and the callers are needing to > pass in the capabilities of the controller which is then also checked > by the underlying code? This all seems extremely confusing, I'm not > surprised there's bugs. Correct, the buffer (pixel data) is always formatted for 16 bits and the bpw is to indicate how this data should be put on the bus (according to the controller capability). If the controller is only capable of 8-bit transfers, the 16-bit data needs to be swapped to account for the big endian bus, while this is not needed if the controller already supports 16-bit transfers. The decision to swap the data or not is taken in the MIPI DBI code by probing the controller capabilities, so if the controller only supports 8-bit the data is swapped, otherwise it is not. The problem arrives when your controller does support 16-bits, so your data is not swapped, but you still put the data on the bus with 8-bit transfers. > At the very least your changelog needs to express clearly what is > going on, the description doesn't appear to correspond to what > you're changing. Gotcha, I'll try to clarify that in the next revision. Thanks, -- Carlo Caione
On Fri, Nov 18, 2022 at 11:36:27AM +0100, Carlo Caione wrote: > On 17/11/2022 15:59, Mark Brown wrote: > > So this is an issue in the MIPI DBI code where the interpretation of the > > buffer passed in depends on both the a caller parameter and the > > capabilities of the underlying SPI controller, meaning that a driver can > > suddenly become buggy when used with a new controller? > The MIPI DBI code is fine, in fact it is doing the correct thing in the > mipi_dbi_typec3_command() function. The problem is that the ILI9486 > driver is hijacking that function installing its own hook that is wrong. Ah, I see - it's causing confusion because it peers into the internals of the underlying code. > The problem arrives when your controller does support 16-bits, so your > data is not swapped, but you still put the data on the bus with 8-bit > transfers. Why would you need to use 8 bit transfers if the controller supports 16 bits?
On 18/11/2022 16:44, Mark Brown wrote: >> The problem arrives when your controller does support 16-bits, so >> your data is not swapped, but you still put the data on the bus >> with 8-bit transfers. > > Why would you need to use 8 bit transfers if the controller supports > 16 bits? No idea why this driver is forcing 8-bit transfers when the controller supports 16-bits (this is what this patch is fixing). My theory is that this driver was written with the Raspberry Pi HATs in mind and (AFAICT) the RPi has an 8-bit only SPI controller so the driver author didn't bother with anything different. -- Carlo Caione
diff --git a/drivers/gpu/drm/tiny/ili9486.c b/drivers/gpu/drm/tiny/ili9486.c index bd37dfe8dd05..4d80a413338f 100644 --- a/drivers/gpu/drm/tiny/ili9486.c +++ b/drivers/gpu/drm/tiny/ili9486.c @@ -43,6 +43,7 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par, size_t num) { struct spi_device *spi = mipi->spi; + unsigned int bpw = 8; void *data = par; u32 speed_hz; int i, ret; @@ -56,8 +57,6 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par, * The displays are Raspberry Pi HATs and connected to the 8-bit only * SPI controller, so 16-bit command and parameters need byte swapping * before being transferred as 8-bit on the big endian SPI bus. - * Pixel data bytes have already been swapped before this function is - * called. */ buf[0] = cpu_to_be16(*cmd); gpiod_set_value_cansleep(mipi->dc, 0); @@ -71,12 +70,18 @@ static int waveshare_command(struct mipi_dbi *mipi, u8 *cmd, u8 *par, for (i = 0; i < num; i++) buf[i] = cpu_to_be16(par[i]); num *= 2; - speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num); data = buf; } + /* + * Check whether pixel data bytes needs to be swapped or not + */ + if (*cmd == MIPI_DCS_WRITE_MEMORY_START && !mipi->swap_bytes) + bpw = 16; + gpiod_set_value_cansleep(mipi->dc, 1); - ret = mipi_dbi_spi_transfer(spi, speed_hz, 8, data, num); + speed_hz = mipi_dbi_spi_cmd_max_speed(spi, num); + ret = mipi_dbi_spi_transfer(spi, speed_hz, bpw, data, num); free: kfree(buf);