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

[Philippe CORNU]

Hi Brian,

On 01/11/2018 12:16 PM, Philippe CORNU wrote:
> 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?


As your patch has been merged, I have few short questions and for each 
related new patch, I would like to know if you prefer that I implement 
it or if you prefer to do it by yourself, it's really like you want, on 
my side, no problem to make them all, some or none, I don't want us to 
implement these in parallel :-)

* Do you have any opinion regarding Tegra-like loops vs the memcpy? (see 
my comment above) If you think the Tegra-like loops is a better approach 
than memcpy, there is a small patch to write.

* Returned value with number of bytes received/transferred: there is a 
small patch to write

* Regarding read operations: I propose to add a TODO + DRM_WARN in case 
someone want to use the API for read operations. Note that I plan to 
implement the read feature but I do not know yet when and maybe Rockchip 
people already have something ~ready?

Many thanks,
Philippe :-)

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