From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755394AbeARLl2 (ORCPT ); Thu, 18 Jan 2018 06:41:28 -0500 Received: from mx08-00178001.pphosted.com ([91.207.212.93]:49082 "EHLO mx07-00178001.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754915AbeARLl0 (ORCPT ); Thu, 18 Jan 2018 06:41:26 -0500 From: Philippe CORNU To: Brian Norris CC: Archit Taneja , Andrzej Hajda , Laurent Pinchart , David Airlie , Yannick FERTRE , Vincent ABRIOU , "dri-devel@lists.freedesktop.org" , "linux-kernel@vger.kernel.org" , Sean Paul , "Nickey Yang" , "hl@rock-chips.com" , "linux-rockchip@lists.infradead.org" , "mka@chromium.org" , "hoegsberg@gmail.com" , "zyw@rock-chips.com" , "xbl@rock-chips.com" Subject: Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet() Thread-Topic: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet() Thread-Index: AQHThoap1QCkJ49j+0ukvgxd0HhaQKNrUQSAgACH7YCAAqSDgIALBxiA Date: Thu, 18 Jan 2018 11:40:48 +0000 Message-ID: <023c159e-0a7f-7d1b-a2d8-8ef19033a1b3@st.com> References: <20180106003813.4816-1-briannorris@chromium.org> <715bb58c-efa6-7944-f186-c186d7fae569@st.com> <20180109185512.GA73309@google.com> In-Reply-To: Accept-Language: fr-FR, en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: user-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.5.0 x-ms-exchange-messagesentrepresentingtype: 1 x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.75.127.46] Content-Type: text/plain; charset="utf-8" Content-ID: <51C9894F7993B34F8C1470E81454CEC6@st.com> MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2018-01-18_05:,, signatures=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Transfer-Encoding: 8bit X-MIME-Autoconverted: from base64 to 8bit by mail.home.local id w0IBfZho004616 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 >>>> --- >>>> 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 >>> Tested-by: Philippe Cornu >>> >>> 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 >>