Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[Philippe CORNU]

Hi Brian,

On 01/09/2018 07:55 PM, Brian Norris wrote:
> Hi Philippe,
> 
> On Tue, Jan 09, 2018 at 10:48:43AM +0000, Philippe CORNU wrote:
>> Hi Brian,
>>
>> And many thanks for implementing these TODOs.
> 
> And thanks for adding them; it gave me a better option than just adding
> yet another switch case (MIPI_DSI_GENERIC_LONG_WRITE) ;)
> 
>> On 01/06/2018 01:38 AM, Brian Norris wrote:
>>> This takes care of 2 TODOs in this driver, by using the common DSI
>>> packet-marshalling code instead of our custom short/long write code.
>>> This both saves us some duplicated code and gets us free support for
>>> command types that weren't already part of our switch block (e.g.,
>>> MIPI_DSI_GENERIC_LONG_WRITE).
>>>
>>> The code logic stays mostly intact, except that it becomes unnecessary
>>> to split the short/long write functions, and we have to copy data a bit
>>> more.
>>>
>>> Along the way, I noticed that loop bounds were a little odd:
>>>
>>> 	while (DIV_ROUND_UP(len, pld_data_bytes))
>>>
>>> This really was just supposed to be 'len != 0', so I made that more
>>> clear.
>>>
>>> Tested on RK3399 with some pending refactoring patches by Nickey Yang,
>>> to make the Rockchip DSI driver wrap this common driver.
>>>
>>> Signed-off-by: Brian Norris <briannorris@xxxxxxxxxxxx>
>>> ---
>>> Could use an extra look from folks. This looks like the correct trivial
>>> transformation, but I'm not that familiar with DSI.
>>>
>>>    drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 78 ++++++---------------------
>>>    1 file changed, 16 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>> index d9cca4fd66ec..2fed20e44dfe 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>> @@ -136,10 +136,6 @@
>>>    					 GEN_SW_0P_TX_LP)
>>>    
>>>    #define DSI_GEN_HDR			0x6c
>>> -/* TODO These 2 defines will be reworked thanks to mipi_dsi_create_packet() */
>>> -#define GEN_HDATA(data)			(((data) & 0xffff) << 8)
>>> -#define GEN_HTYPE(type)			(((type) & 0xff) << 0)
>>> -
>>>    #define DSI_GEN_PLD_DATA		0x70
>>>    
>>>    #define DSI_CMD_PKT_STATUS		0x74
>>> @@ -359,44 +355,15 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
>>>    	return 0;
>>>    }
>>>    
>>> -static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
>>> -				       const struct mipi_dsi_msg *msg)
>>> -{
>>> -	const u8 *tx_buf = msg->tx_buf;
>>> -	u16 data = 0;
>>> -	u32 val;
>>> -
>>> -	if (msg->tx_len > 0)
>>> -		data |= tx_buf[0];
>>> -	if (msg->tx_len > 1)
>>> -		data |= tx_buf[1] << 8;
>>> -
>>> -	if (msg->tx_len > 2) {
>>> -		dev_err(dsi->dev, "too long tx buf length %zu for short write\n",
>>> -			msg->tx_len);
>>> -		return -EINVAL;
>>> -	}
>>> -
>>> -	val = GEN_HDATA(data) | GEN_HTYPE(msg->type);
>>> -	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, val);
>>> -}
>>> -
>>> -static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
>>> -				      const struct mipi_dsi_msg *msg)
>>> +static int dw_mipi_dsi_dcs_write(struct dw_mipi_dsi *dsi,
>>> +				 const struct mipi_dsi_packet *packet)
>>
>> Both DCS and Generic dsi transfers are managed by drm_mipi_dsi.c
>> helpers. So maybe dw_mipi_dsi_dcs_write() should be renamed
>> dw_mipi_dsi_write()...
> 
> Ah, good point. I really meant to remove the _dcs naming too, but I
> guess I missed it. Will follow up.
> 
>>>    {
>>> -	const u8 *tx_buf = msg->tx_buf;
>>> -	int len = msg->tx_len, pld_data_bytes = sizeof(u32), ret;
>>> -	u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type);
>>> +	const u8 *tx_buf = packet->payload;
>>> +	int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret;
>>>    	u32 remainder;
>>>    	u32 val;
>>>    
>>> -	if (msg->tx_len < 3) {
>>> -		dev_err(dsi->dev, "wrong tx buf length %zu for long write\n",
>>> -			msg->tx_len);
>>> -		return -EINVAL;
>>> -	}
>>> -
>>> -	while (DIV_ROUND_UP(len, pld_data_bytes)) {
>>> +	while (len) {
>>>    		if (len < pld_data_bytes) {
>>>    			remainder = 0;
>>>    			memcpy(&remainder, tx_buf, len);
>>> @@ -419,40 +386,27 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
>>>    		}
>>>    	}
>>>    
>>> -	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val);
>>> +	remainder = 0;
>>> +	memcpy(&remainder, packet->header, sizeof(packet->header));
> 
> By the way: I don't think it's an issue that should block this patch,
> since if I'm right, this function already is "broken", but isn't this
> actually a bad way to handle byte-to-word marshalling? Particularly,
> we're copying bytes into a word in LE ordering, but then we later write
> them to IO registers with writel() (which does endian swapping).
> 
> So I think we have an endianness problem on BE systems.
> 
> One solution would be to write these to IO registers with a non-swapped
> writel() (e.g., __raw_writel()? but that's not very nice...). Another
> would be to avoid memcpy, and just read this out a word at a time --
> that works fine for the aligned pieces, but not so well for any
> non-aligned bits ('if (len < pld_data_bytes)' above) I think?
> 
> WDYT?
> 

To be honest, I do not really like the memcpy here too and I agree with 
you regarding the BE issue.

My first "stm" driver (ie. before using this "freescale/rockchip" 
dw-mipi-dsi driver with the memcpy) used the "exact" same code as the 
Tegra dsi tegra_dsi_writesl() function with the 2 loops.

https://elixir.free-electrons.com/linux/v4.14/source/drivers/gpu/drm/tegra/dsi.c#L1248

IMHO, it is better than memcpy...
I added these 3 "documentation" lines, maybe we may reuse them or 
something similar...

/*
  * Write 8-bit payload data into the 32-bit payload data register.
  * ex: payload data "0x01, 0x02, 0x03, 0x04, 0x05, 0x06" will become
  * "0x04030201 0x00000605" 32-bit writes
  */

Not sure it helps to fix the BE issue but we may add a TODO stating that 
"this loop has not been tested on BE"...

What is your opinion?

Many thanks
Philippe :-)


>>> +	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder);
>>>    }
>>>    
>>>    static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>>>    					 const struct mipi_dsi_msg *msg)
>>>    {
>>>    	struct dw_mipi_dsi *dsi = host_to_dsi(host);
>>> +	struct mipi_dsi_packet packet;
>>>    	int ret;
>>>    
>>> -	/*
>>> -	 * TODO dw drv improvements
>>> -	 * use mipi_dsi_create_packet() instead of all following
>>> -	 * functions and code (no switch cases, no
>>> -	 * dw_mipi_dsi_dcs_short_write(), only the loop in long_write...)
>>> -	 * and use packet.header...
>>> -	 */
>>> -	dw_mipi_message_config(dsi, msg);
>>> -
>>> -	switch (msg->type) {
>>> -	case MIPI_DSI_DCS_SHORT_WRITE:
>>> -	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
>>> -	case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
>>> -		ret = dw_mipi_dsi_dcs_short_write(dsi, msg);
>>> -		break;
>>> -	case MIPI_DSI_DCS_LONG_WRITE:
>>> -		ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
>>> -		break;
>>> -	default:
>>> -		dev_err(dsi->dev, "unsupported message type 0x%02x\n",
>>> -			msg->type);
>>> -		ret = -EINVAL;
>>> +	ret = mipi_dsi_create_packet(&packet, msg);
>>> +	if (ret) {
>>> +		dev_err(dsi->dev, "failed to create packet: %d\n", ret);
>>> +		return ret;
>>>    	}
>>>    
>>> -	return ret;
>>> +	dw_mipi_message_config(dsi, msg);
>>> +
>>> +	return dw_mipi_dsi_dcs_write(dsi, &packet);
>>>    }
>>>    
>>>    static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
>>>
>>
>> I performed some tests tracing all DSI_GEN_HDR & DSI_GEN_PLD_DATA reg
>> writes with panel/panel-orisetech-otm8009a.c (using long dcs commands)
>> before and after your patch and this is "100% perfect"!
>>
>> So, apart the un-important "dcs" in dw_mipi_dsi_dcs_write() function name:
>>
>> Reviewed-by: Philippe Cornu <philippe.cornu@xxxxxx>
>> Tested-by: Philippe Cornu <philippe.cornu@xxxxxx>
>>
>> This clean-up will help a lot to add the dsi read feature in the future.
>>
>> Very good patch Brian and big "thank you" !
> 
> Thanks for the review and test! I'll likely send a v2 with only the
> naming change + your tags, and I'll see about what do about the
> endianness issues I noticed as a follow-up.
> 
> Brian
>