Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[Philippe CORNU]

Hi Andrzej,


On 01/25/2018 12:07 PM, Andrzej Hajda wrote:
> 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
> 
> 
Many thanks for the clarification. Good to know your preference here for 
memcpy. So then if someone decides to rework this piece of code, all 
these arguments in this email thread will help.

Many thanks
Philippe :-)

>> * 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 :-)
> 
>