From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751527AbeAYLHN (ORCPT ); Thu, 25 Jan 2018 06:07:13 -0500 Received: from mailout1.w1.samsung.com ([210.118.77.11]:56614 "EHLO mailout1.w1.samsung.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750851AbeAYLHL (ORCPT ); Thu, 25 Jan 2018 06:07:11 -0500 DKIM-Filter: OpenDKIM Filter v2.11.0 mailout1.w1.samsung.com 20180125110708euoutp015a2745ac101e7070dc1d1b6b81d1e312~NCOTkJ57_0446704467euoutp01M X-AuditID: cbfec7f2-f793b6d000003243-12-5a69ba5bc41e 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 , Brian Norris Cc: Archit Taneja , 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" From: Andrzej Hajda 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: Content-transfer-encoding: 8bit Content-language: en-US X-Brightmail-Tracker: H4sIAAAAAAAAA01SaUhUYRTtm7fMc2jqOZpdNBKnIgyyRaMPNwoqXmDUj6KaKBvy4VhuzKgt P2ost2Zc0rHU0fbNImjSdEywTMtxKUscNQNNkTQLc0uZyiXHN4H/zv3Ouffcc/kYQjZIuTMR 0XG8OloZKaclZHnd7w/rj1RGKDYONXjhjOYGEb6YNkThktFhClsnhmlsMzUSeHIqlcKXs++J cWtlEY3HemYJPG7qQ/j+358I9yR9o3CX6T3C79qqSdz+aViEUy8n0/hmTwWxzZkr1LaQXGtm hoh7YewSc4VpBRRnnuyhuC96i4grvXeBmza+IrmvtiqCGy9ZuU+ikASG8ZERCbx6Q/Bxiepl dSEVe0N+xjZZSmrRdQ8dYhhg/eDNiFqHnOagG3zsfkrrkISRsfcR2L4mk0IxjuC7uZcSVH6Q 2/tQJBAPEFwteya2E1LWGWyGbtKOCdYbvv3KcXT3I0gsHKXtdi7sfhh4vNWucWUPQKa1ed6O YK0U1BrTCTtBzzVPl3bSwtBg6LhbPD+UZNdA8+wUsuNl7CHQ9VfOb+TE+sNM5x+HsSe8tg44 8HK4lNw5vwSwdWIwpVeRQoQd0FdgcMRxge+W52IBr4BWg97RoEcwllUvFopcBDMj+YSgCoBa SwslWCyBnPI8QrikFNJSZIKEg/68bId8O+irzY5TlBAwm/WDuII8jQtOZlxwMuOCFMYFKW4h 8jFy5eM1UeG8xtdHo4zSxEeH+5yIiSpBcx+wacYyVoEm6v1rEMsg+WIpk6VSyChlguZsVA0C hpC7SltT556kYcqz53h1TKg6PpLX1CAPhpQvlwYpUg7L2HBlHH+K52N59X9WxDi5a5FMljVY PSQ9SJLtCksiVxy5X++y+Yn7o9vuJi/fgBttdUWn95yXrLY+qtjSwTYGTTsPnAy57hm0R7rU +2Os7ljO4ZDQgnzLhrWrdmdraxeZfd1u7np7u8ZgUFFPj1r2lnU1KYpf30nbWWSWq9pmygYP HSQ6th1optWfA3OcSq8laeWkRqXctI5Qa5T/AGoaMdd8AwAA X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrDIsWRmVeSWpSXmKPExsVy+t/xy7rRuzKjDI5PZrLoPXeSyaKp4y2r xaaP71ktrnx9z2bxY8MpZotvf9pZLTonLmG3uLxrDpvFpwf/mS0+b3jMaLH09ztGiwctL1gt 7m44y2hx5uoBFotrN94zWbR3trJZzH+wg9lB0GN2w0UWj8t9vUweO2fdZfeY3TGT1WP7twes Hve7jzN5bF5S7/F31n4Wj6c/9jJ7fN4kF8AVxWWTkpqTWZZapG+XwJWx78Bs1oJ5ShU/vm1m aWCcK93FyMkhIWAiMeXhciYIW0ziwr31bF2MXBxCAksYJe7/Xc4KkuAVEJT4MfkeSxcjBwez gLrElCm5EDXPGCUOv3zLBBIXFgiReL7KHKRcRCBU4vCyNiaQGmaBG6wSLxc9Yodo2MQssebo OXaQKjYBTYm/m2+yQSywk7i+eAULiM0ioCpx7v8fRhBbVCBComnmXLAjOAWsJP7d/AVWwywg L3HwynMoW1yiufUmywRGwVlIbp2FcOssJB2zkHQsYGRZxSiSWlqcm55bbKRXnJhbXJqXrpec n7uJERin24793LKDsetd8CFGAQ5GJR7ehIkZUUKsiWXFlbmHGCU4mJVEeC+3A4V4UxIrq1KL 8uOLSnNSiw8xSnOwKInz9u5ZHSkkkJ5YkpqdmlqQWgSTZeLglGpgdMpIE3p4WMuO8/Hplue6 go+5M3fN5V330nDvycZlV0uSs1t5/dbFeNtsVsp7fqmxI6/uvK46Z1NaxlzmuiNLTDdnTdnA tvPBj7hDEw9/St3Hu1Pxp+pUkX2RPD+mSUX4pKt63zqxQVnjwqPWvO9efmf3Nfg5/Nubn3L4 9GIG2y2PPI9N3vhNXImlOCPRUIu5qDgRAEKhF97PAgAA X-CMS-MailID: 20180125110707eucas1p2bd16916a5c89e2b1336900c932c73c45 X-Msg-Generator: CA CMS-TYPE: 201P X-CMS-RootMailID: 20180124095152epcas2p3c1ece403435d3b6093b46954da8cd66e X-RootMTR: 20180124095152epcas2p3c1ece403435d3b6093b46954da8cd66e References: <20180106003813.4816-1-briannorris@chromium.org> <715bb58c-efa6-7944-f186-c186d7fae569@st.com> <20180109185512.GA73309@google.com> <023c159e-0a7f-7d1b-a2d8-8ef19033a1b3@st.com> <20180123211505.xvpu3putjjbqnl2b@ban.mtv.corp.google.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 :-)