Hi Brian,
On 01/11/2018 12:16 PM, Philippe CORNU wrote:
> Hi Brian,
>
> On 01/09/2018 07:55 PM, Brian Norris wrote:
>> Hi Philippe,
>>
>> On Tue, Jan 09, 2018 at 10:48:43AM +0000, Philippe CORNU wrote:
>>> Hi Brian,
>>>
>>> And many thanks for implementing these TODOs.
>>
>> And thanks for adding them; it gave me a better option than just adding
>> yet another switch case (MIPI_DSI_GENERIC_LONG_WRITE) ;)
>>
>>> On 01/06/2018 01:38 AM, Brian Norris wrote:
>>>> This takes care of 2 TODOs in this driver, by using the common DSI
>>>> packet-marshalling code instead of our custom short/long write code.
>>>> This both saves us some duplicated code and gets us free support for
>>>> command types that weren't already part of our switch block (e.g.,
>>>> MIPI_DSI_GENERIC_LONG_WRITE).
>>>>
>>>> The code logic stays mostly intact, except that it becomes unnecessary
>>>> to split the short/long write functions, and we have to copy data a bit
>>>> more.
>>>>
>>>> Along the way, I noticed that loop bounds were a little odd:
>>>>
>>>> ????while (DIV_ROUND_UP(len, pld_data_bytes))
>>>>
>>>> This really was just supposed to be 'len != 0', so I made that more
>>>> clear.
>>>>
>>>> Tested on RK3399 with some pending refactoring patches by Nickey Yang,
>>>> to make the Rockchip DSI driver wrap this common driver.
>>>>
>>>> Signed-off-by: Brian Norris <briannorris at chromium.org>
>>>> ---
>>>> Could use an extra look from folks. This looks like the correct trivial
>>>> transformation, but I'm not that familiar with DSI.
>>>>
>>>> ?? drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 78
>>>> ++++++---------------------
>>>> ?? 1 file changed, 16 insertions(+), 62 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>>> index d9cca4fd66ec..2fed20e44dfe 100644
>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>>> @@ -136,10 +136,6 @@
>>>> ??????????????????????? GEN_SW_0P_TX_LP)
>>>> ?? #define DSI_GEN_HDR??????????? 0x6c
>>>> -/* TODO These 2 defines will be reworked thanks to
>>>> mipi_dsi_create_packet() */
>>>> -#define GEN_HDATA(data)??????????? (((data) & 0xffff) << 8)
>>>> -#define GEN_HTYPE(type)??????????? (((type) & 0xff) << 0)
>>>> -
>>>> ?? #define DSI_GEN_PLD_DATA??????? 0x70
>>>> ?? #define DSI_CMD_PKT_STATUS??????? 0x74
>>>> @@ -359,44 +355,15 @@ static int
>>>> dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
>>>> ?????? return 0;
>>>> ?? }
>>>> -static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
>>>> -?????????????????????? const struct mipi_dsi_msg *msg)
>>>> -{
>>>> -??? const u8 *tx_buf = msg->tx_buf;
>>>> -??? u16 data = 0;
>>>> -??? u32 val;
>>>> -
>>>> -??? if (msg->tx_len > 0)
>>>> -??????? data |= tx_buf[0];
>>>> -??? if (msg->tx_len > 1)
>>>> -??????? data |= tx_buf[1] << 8;
>>>> -
>>>> -??? if (msg->tx_len > 2) {
>>>> -??????? dev_err(dsi->dev, "too long tx buf length %zu for short
>>>> write\n",
>>>> -??????????? msg->tx_len);
>>>> -??????? return -EINVAL;
>>>> -??? }
>>>> -
>>>> -??? val = GEN_HDATA(data) | GEN_HTYPE(msg->type);
>>>> -??? return dw_mipi_dsi_gen_pkt_hdr_write(dsi, val);
>>>> -}
>>>> -
>>>> -static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
>>>> -????????????????????? const struct mipi_dsi_msg *msg)
>>>> +static int dw_mipi_dsi_dcs_write(struct dw_mipi_dsi *dsi,
>>>> +???????????????? const struct mipi_dsi_packet *packet)
>>>
>>> Both DCS and Generic dsi transfers are managed by drm_mipi_dsi.c
>>> helpers. So maybe dw_mipi_dsi_dcs_write() should be renamed
>>> dw_mipi_dsi_write()...
>>
>> Ah, good point. I really meant to remove the _dcs naming too, but I
>> guess I missed it. Will follow up.
>>
>>>> ?? {
>>>> -??? const u8 *tx_buf = msg->tx_buf;
>>>> -??? int len = msg->tx_len, pld_data_bytes = sizeof(u32), ret;
>>>> -??? u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type);
>>>> +??? const u8 *tx_buf = packet->payload;
>>>> +??? int len = packet->payload_length, pld_data_bytes = sizeof(u32),
>>>> ret;
>>>> ?????? u32 remainder;
>>>> ?????? u32 val;
>>>> -??? if (msg->tx_len < 3) {
>>>> -??????? dev_err(dsi->dev, "wrong tx buf length %zu for long write\n",
>>>> -??????????? msg->tx_len);
>>>> -??????? return -EINVAL;
>>>> -??? }
>>>> -
>>>> -??? while (DIV_ROUND_UP(len, pld_data_bytes)) {
>>>> +??? while (len) {
>>>> ?????????? if (len < pld_data_bytes) {
>>>> ?????????????? remainder = 0;
>>>> ?????????????? memcpy(&remainder, tx_buf, len);
>>>> @@ -419,40 +386,27 @@ static int dw_mipi_dsi_dcs_long_write(struct
>>>> dw_mipi_dsi *dsi,
>>>> ?????????? }
>>>> ?????? }
>>>> -??? return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val);
>>>> +??? remainder = 0;
>>>> +??? memcpy(&remainder, packet->header, sizeof(packet->header));
>>
>> By the way: I don't think it's an issue that should block this patch,
>> since if I'm right, this function already is "broken", but isn't this
>> actually a bad way to handle byte-to-word marshalling? Particularly,
>> we're copying bytes into a word in LE ordering, but then we later write
>> them to IO registers with writel() (which does endian swapping).
>>
>> So I think we have an endianness problem on BE systems.
>>
>> One solution would be to write these to IO registers with a non-swapped
>> writel() (e.g., __raw_writel()? but that's not very nice...). Another
>> would be to avoid memcpy, and just read this out a word at a time --
>> that works fine for the aligned pieces, but not so well for any
>> non-aligned bits ('if (len < pld_data_bytes)' above) I think?
>>
>> WDYT?
>>
>
> 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?
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.
* Returned value with number of bytes received/transferred: there is a
small patch to write
* 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?
Many thanks,
Philippe :-)
>
> Many thanks
> Philippe :-)
>
>
>>>> +??? return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder);
>>>> ?? }
>>>> ?? static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>>>> ??????????????????????? const struct mipi_dsi_msg *msg)
>>>> ?? {
>>>> ?????? struct dw_mipi_dsi *dsi = host_to_dsi(host);
>>>> +??? struct mipi_dsi_packet packet;
>>>> ?????? int ret;
>>>> -??? /*
>>>> -???? * TODO dw drv improvements
>>>> -???? * use mipi_dsi_create_packet() instead of all following
>>>> -???? * functions and code (no switch cases, no
>>>> -???? * dw_mipi_dsi_dcs_short_write(), only the loop in long_write...)
>>>> -???? * and use packet.header...
>>>> -???? */
>>>> -??? dw_mipi_message_config(dsi, msg);
>>>> -
>>>> -??? switch (msg->type) {
>>>> -??? case MIPI_DSI_DCS_SHORT_WRITE:
>>>> -??? case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
>>>> -??? case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
>>>> -??????? ret = dw_mipi_dsi_dcs_short_write(dsi, msg);
>>>> -??????? break;
>>>> -??? case MIPI_DSI_DCS_LONG_WRITE:
>>>> -??????? ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
>>>> -??????? break;
>>>> -??? default:
>>>> -??????? dev_err(dsi->dev, "unsupported message type 0x%02x\n",
>>>> -??????????? msg->type);
>>>> -??????? ret = -EINVAL;
>>>> +??? ret = mipi_dsi_create_packet(&packet, msg);
>>>> +??? if (ret) {
>>>> +??????? dev_err(dsi->dev, "failed to create packet: %d\n", ret);
>>>> +??????? return ret;
>>>> ?????? }
>>>> -??? return ret;
>>>> +??? dw_mipi_message_config(dsi, msg);
>>>> +
>>>> +??? return dw_mipi_dsi_dcs_write(dsi, &packet);
>>>> ?? }
>>>> ?? static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
>>>>
>>>
>>> I performed some tests tracing all DSI_GEN_HDR & DSI_GEN_PLD_DATA reg
>>> writes with panel/panel-orisetech-otm8009a.c (using long dcs commands)
>>> before and after your patch and this is "100% perfect"!
>>>
>>> So, apart the un-important "dcs" in dw_mipi_dsi_dcs_write() function
>>> name:
>>>
>>> Reviewed-by: Philippe Cornu <philippe.cornu at st.com>
>>> Tested-by: Philippe Cornu <philippe.cornu at st.com>
>>>
>>> This clean-up will help a lot to add the dsi read feature in the future.
>>>
>>> Very good patch Brian and big "thank you" !
>>
>> Thanks for the review and test! I'll likely send a v2 with only the
>> naming change + your tags, and I'll see about what do about the
>> endianness issues I noticed as a follow-up.
>>
>> Brian
>>
