linux-kernel - Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180123211505.xvpu3putjjbqnl2b@ban.mtv.corp.google.com>
Date:   Tue, 23 Jan 2018 13:15:10 -0800
From:   Brian Norris <briannorris@...omium.org>
To:     Philippe CORNU <philippe.cornu@...com>
Cc:     Archit Taneja <architt@...eaurora.org>,
        Andrzej Hajda <a.hajda@...sung.com>,
        Laurent Pinchart <Laurent.pinchart@...asonboard.com>,
        David Airlie <airlied@...ux.ie>,
        Yannick FERTRE <yannick.fertre@...com>,
        Vincent ABRIOU <vincent.abriou@...com>,
        "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Sean Paul <seanpaul@...omium.org>,
        Nickey Yang <nickey.yang@...k-chips.com>,
        "hl@...k-chips.com" <hl@...k-chips.com>,
        "linux-rockchip@...ts.infradead.org" 
        <linux-rockchip@...ts.infradead.org>,
        "mka@...omium.org" <mka@...omium.org>,
        "hoegsberg@...il.com" <hoegsberg@...il.com>,
        "zyw@...k-chips.com" <zyw@...k-chips.com>,
        "xbl@...k-chips.com" <xbl@...k-chips.com>
Subject: Re: [PATCH] drm/bridge/synopsys: dsi: use common
 mipi_dsi_create_packet()

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.

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