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) * 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 :-) _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
