Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-version: 1.0
Content-type: text/plain; charset="utf-8"
Subject: Re: [PATCH] drm/bridge/synopsys: dsi: use common
 mipi_dsi_create_packet()
To:     Philippe CORNU <philippe.cornu@st.com>,
        Brian Norris <briannorris@chromium.org>
Cc:     Archit Taneja <architt@codeaurora.org>,
        Laurent Pinchart <Laurent.pinchart@ideasonboard.com>,
        David Airlie <airlied@linux.ie>,
        Yannick FERTRE <yannick.fertre@st.com>,
        Vincent ABRIOU <vincent.abriou@st.com>,
        "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Sean Paul <seanpaul@chromium.org>,
        Nickey Yang <nickey.yang@rock-chips.com>,
        "hl@rock-chips.com" <hl@rock-chips.com>,
        "linux-rockchip@lists.infradead.org" 
        <linux-rockchip@lists.infradead.org>,
        "mka@chromium.org" <mka@chromium.org>,
        "hoegsberg@gmail.com" <hoegsberg@gmail.com>,
        "zyw@rock-chips.com" <zyw@rock-chips.com>,
        "xbl@rock-chips.com" <xbl@rock-chips.com>
From:   Andrzej Hajda <a.hajda@samsung.com>
Message-id: <7dd644f0-697d-1545-cdc3-30e0708c93df@samsung.com>
Date:   Thu, 25 Jan 2018 12:07:05 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
        Thunderbird/52.5.0
In-reply-to: <a5c00d22-1df6-fe9c-d333-12a6b689acb3@st.com>
Content-transfer-encoding: 8bit
Content-language: en-US
CMS-TYPE: 201P
References: <20180106003813.4816-1-briannorris@chromium.org>
        <715bb58c-efa6-7944-f186-c186d7fae569@st.com>
        <20180109185512.GA73309@google.com>
        <c680f230-393f-8bfa-cc9c-3b8db87bd5fb@st.com>
        <023c159e-0a7f-7d1b-a2d8-8ef19033a1b3@st.com>
        <20180123211505.xvpu3putjjbqnl2b@ban.mtv.corp.google.com>
        <CGME20180124095152epcas2p3c1ece403435d3b6093b46954da8cd66e@epcas2p3.samsung.com>
        <a5c00d22-1df6-fe9c-d333-12a6b689acb3@st.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On 24.01.2018 10:51, Philippe CORNU wrote:
> Hi Brian,
>
> On 01/23/2018 10:15 PM, Brian Norris wrote:
>> Hi Philippe,
>>
>> On Thu, Jan 18, 2018 at 11:40:48AM +0000, Philippe CORNU wrote:
>>> On 01/11/2018 12:16 PM, Philippe CORNU wrote:
>>>> 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?
>> I'm sorry, I don't think I noticed your reply here. I'm trying to unbury
>> some email, but that's sometimes a losing battle...
>>
>> That code actually does look correct, and it's perhaps marginally
>> better-looking in my opinion. It's up to you if you want to propose
>> another patch :) At this point, it's only a matter of nice code, not
>> correctness I believe.
>>
>>> 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.
>> My opinion is above.
>>
> I do not know yet if I will send a patch but several reasons may push me 
> to do it:
> * Andrzej proposed a nicer code in his last review so it means the 
> actual code with memcpy's is "not so nice" (even if it works fine)

I was not against memcpy, I have just suggested to abstract the code out
to some helper function.
Regarding memcpy vs loop I would prefer memcpy - simpler code, but it is
looks less important that abstracting out.

Regards
Andrzej


> * Several dsi drivers use the Tegra-like loops (Tegra, intel,... ) and 
> in vc4/exynos/mtk drivers memcpy is not used, msm uses memcpy... well, 
> not sure it is then a good argument, different solutions for different hw...
> * Coming cadence dsi bridge driver uses Tegra-like loops.
> * I think my read function will also have Tegra-like loops, if it is the 
> case, it could be nice to have something homogeneous...
>
> Anyway, it is not an important point : )
>
>>> * Returned value with number of bytes received/transferred: there is a
>>> small patch to write
>> I don't think I followed that one very well. I'm not sure my opinion
>> really matters, as long as you get someone else to agree. I do not plan
>> to write any such patch in the near term.
>>
>>> * 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?
>> The warning would be nice to do now, regardless.
>>
>> Rockchip folks wrote up something for read support here [1], but it's
>> based on a semi-forked version of the driver (we're trying to clean up
>> the divergence, but it's not there yet). Perhaps it would provide useful
>> fodder for your work. I don't think Rockchip is immediately working on
>> upstreaming this particular patch, so it's totally fair to handle it
>> yourself. It's got the GPL sign-off ;)
>>
>> Brian
>>
>> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/863347
>>
> Very good information, I will have a look,
> many thanks
> Philippe :-)