| Message ID | 20221116-s905x_spi_ili9486-v1-2-630401cb62d5@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fix SPICC and ILI9486 drivers | expand |
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);
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(-)