Message-ID: <023c159e-0a7f-7d1b-a2d8-8ef19033a1b3@st.com>
Date:   Thu, 18 Jan 2018 11:40:48 +0000
From:   Philippe CORNU <philippe.cornu@...com>
To:     Brian Norris <briannorris@...omium.org>
CC:     Archit Taneja <architt@...eaurora.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        David Airlie <airlied@...ux.ie>,
        Yannick FERTRE <yannick.fertre@...com>,
        Vincent ABRIOU <vincent.abriou@...com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Sean Paul <seanpaul@...omium.org>,
        "Nickey Yang" <nickey.yang@...k-chips.com>,
        "hl@...k-chips.com" <hl@...k-chips.com>,
        "linux-rockchip@...ts.infradead.org" 
        <linux-rockchip@...ts.infradead.org>,
        "mka@...omium.org" <mka@...omium.org>,
        "hoegsberg@...il.com" <hoegsberg@...il.com>,
        "zyw@...k-chips.com" <zyw@...k-chips.com>,
        "xbl@...k-chips.com" <xbl@...k-chips.com>
Subject: Re: [PATCH] drm/bridge/synopsys: dsi: use common
 mipi_dsi_create_packet()

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@...omium.org>
>>>> ---
>>>> 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@...com>
>>> Tested-by: Philippe Cornu <philippe.cornu@...com>
>>>
>>> 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
>>

Powered by blists - more mailing lists