[2/3] drm/tiny: ili9486: Do not assume 8-bit only SPI controllers

  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(-)
  

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