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@xxxxxxxxxxxx>
>>>> ---
>>>> 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@xxxxxx>
>>> Tested-by: Philippe Cornu <philippe.cornu@xxxxxx>
>>>
>>> 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
>>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
