[PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Andrzej Hajda]

On 07/23/2014 03:37 PM, Thierry Reding wrote:
> On Wed, Jul 23, 2014 at 12:59:46PM +0200, Andrzej Hajda wrote:
>> On 07/23/2014 09:51 AM, Thierry Reding wrote:
>>> On Tue, Jul 22, 2014 at 11:33:11AM +0200, Andrzej Hajda wrote:
>>>> On 07/22/2014 10:12 AM, Thierry Reding wrote:
>>>>> On Tue, Jul 22, 2014 at 09:32:58AM +0200, Andrzej Hajda wrote:
>>>>>> On 07/22/2014 09:12 AM, Thierry Reding wrote:
>>>>>>> From: Thierry Reding <treding at nvidia.com>
>>>>>>>
>>>>>>> Currently the mipi_dsi_dcs_write() function requires the DCS command
>>>>>>> byte to be embedded within the write buffer whereas mipi_dsi_dcs_read()
>>>>>>> has a separate parameter. Make them more symmetrical by adding an extra
>>>>>>> command parameter to mipi_dsi_dcs_write().
>>>>>>>
>>>>>>> Also update the S6E8AA0 panel driver (the only user of this API) to
>>>>>>> adapt to this new API.
>>>>>> I do not agree with that. DCS read and write are not symmetrical by design:
>>>>>> - write just sends data,
>>>>>> - read sends data then reads response.
>>>>>>
>>>>>> Forcing API to be symmetrical complicates the implementation without real gain.
>>>>> Why does it complicate anything?
>>>> Why? Lets see:
>>>>
>>>>  drivers/gpu/drm/drm_mipi_dsi.c        | 45 ++++++++++++++++++++++++-----------
>>>>  drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
>>>>  include/drm/drm_mipi_dsi.h            |  4 ++--
>>>>  3 files changed, 35 insertions(+), 18 deletions(-)
>>>>
>>>>
>>>> Original function has two vars, one 'if', one 'switch' and one operation
>>>> call, nothing more.
>>>> You proposes to add new vars, kmalloc with no memory handling, memcpy,
>>>> playing with indices, conditional kfree. Isn't it enough to call it more
>>>> complicated? :)
>>> It certainly makes the implementation of mipi_dsi_dcs_write() slightly
>>> more complicated, but the point is that it makes it easier for users of
>>> the API. So the complexity moves into one central location rather than
>>> each call site. Ultimately that will reduce overall complexity.
>> I guess it will increase complexity. If you look at the s6e8aa0 or any
>> other driver using DCS commands you will see the most DCS commands have
>> constant arguments, so driver uses small static arrays without copying
>> them to temporary variables. With your approach every time you will have
>> to allocate tiny memory bufs, memcpy to them and deallocate them later.
> Yes, there are certainly additional costs. But they give us more
> consistency in return. The whole point of having MIPI DCS helpers is so
> that they can take away some of the work that drivers have to do. This
> is core code
>
>> I terms of drivers code simplicity, current version with proposed macros
>> do better job.
> Quite frankly, I think they result in horrible code. The s6e8aa0 driver
> is currently not much more than a huge set of tables that are dumped to
> hardware.
>
> And of course that's "simple", but it's also completely unreadable and
> makes it really difficult to factor out common pieces of code. Of course
> the driver then also goes and uses these macros to execute standard DCS
> commands instead of doing the right thing and writing proper functions
> so that other drivers can reuse them. Yes, that's simple for the
> individual drivers but it doesn't help us at all in the big picture.

Hmm, I think you look at the wrong driver :) panel-s6e8aa0.c uses only
four DCS commands:
        s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
        s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
        s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
        s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_OFF);
I see it rather readable :)

The rest of DSI transactions are MCS commands,  completely undocumented
- I had no access to datasheet for this
panel/IC driver during driver development. How do you imagine 'proper
functions' in such situation?

>>>>> Some of this could possibly be alleviated by adding separate functions
>>>>> for standard commands. But either way, I think having this kind of
>>>>> symmetry within an API is always good (it's consistent). By the law of
>>>>> least surprise people will actually expect mipi_dsi_dcs_write() to take
>>>>> a command as a second parameter.
>>>> As I wrote earlier I do not see symmetry here: dcs-read is in fact write
>>>> and read,
>>>> dcs-write is just write. Hiding this fact in API does not seems to me
>>>> good, but I guess
>>>> it is rather matter of taste.
>>> The symmetry isn't about the physical transfers. It's a logical
>>> symmetry. Each DCS command is identified by a command, whether it's a
>>> read or a write.
>> If you insist on it maybe it will be better to leave my version of
>> mipi_dsi_dcs_write, maybe renamed to mipi_dsi_dcs_write_buf or sth
>> similar and add your version using internally my version. This way
>> you will have your symmetry and I will keep my simplicity :)
> Maybe that would be an alternative. I'll think about it.
>
>>> There's a similar dissymmetry in how the payload length is handled.
>>> Currently peripheral drivers need to encode that within the payload
>>> buffer, and DSI host drivers need to parse it out of that depending on
>>> the type of write. That makes it needlessly difficult for host drivers
>>> and I think the interface would be much easier to use if peripheral
>>> drivers specified the payload (and its length) only and let drivers
>>> obtain the length of writes from the .tx_len field.
>> I am not sure if I understand correctly. Where the peripherals encodes
>> payload length in payload buffer?
> Heh, it's not encoded in the payload buffer, which makes this even
> weirder. Because now we have this binary buffer that the DSI host's
> .transfer() function needs to take apart and put together differently
> depending on the type of packet.
>
> So this whole DCS business isn't really thought out very well. I'll go
> play with it some more to see how we can possibly improve it. We seem to
> have a similar issue for reads, where currently every host driver needs
> to parse returned packets itself in order to return data to the caller.
> That completely annuls the purpose of the DCS functions. They should
> really be making it easier for both host and peripheral drivers by
> translating between DCS and the raw DSI packet data.

I guess this could be an area of improvement, but it would be good
to have more than one implementation of dsi transfer in mainline
to have possibility of looking for common patterns and for differences.

>
> So what I'm currently thinking is that we need a better definition for
> exactly what data should go into a struct mipi_dsi_msg. I think it
> should be raw DSI data (that is, including the header and the payload)
> so that the only job of drivers is to write it into the corresponding
> FIFO registers and start the transmission.

This or just helpers in DSI host to calculate raw DSI data.
What about ECC? Should it be present also in tx buffer?

Andrzej

>
> Similarly, when receiving data the .transfer() function should pass up a
> complete buffer (including the header and payload) to the receive buffer
> and let some upper layer handle this. That way we can layer things in
> helpers rather than having to duplicate the same code in each DSI host
> driver.
>
> Thierry