lkml.org 
[lkml]   [2018]   [Jan]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()
    From
    Date
    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 :-)


    \
     
     \ /
      Last update: 2018-01-25 12:07    [W:3.875 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site