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

[Thierry Reding]

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.

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

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.

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140723/481aec68/attachment.sig>