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

[Thierry Reding]

On Thu, Jul 24, 2014 at 09:57:04AM +0200, Andrzej Hajda wrote:
> 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 :)

When I was talking about readable I wasn't talking about these. These
are what I said should be generic functions. They aren't s6e8aa0
specific at all, therefore shouldn't be using s6e8aa0 specific
functions.

> The rest of DSI transactions are MCS commands,  completely undocumented
> - I had no access to datasheet for this
> panel/IC driver during driver development.

So did you just guess the right sequences? Somebody must have had access
to the documentation at some point.

> How do you imagine 'proper functions' in such situation?

I agree that that's a problem. However I think we should try really hard
to get access to proper documentation. We require other drivers to avoid
these kinds of magic values, why should panels be different?

While there may be circumstances where it's simply not possible to get
access to the documentation, providing an API that allows to pass in any
arbitrary binary buffer actively encourages this kind of behaviour. I
know that we can't really prevent it by extracting the command into a
separate function argument, but at least there's a slight chance that
it'll make people think for a second before just dumping some sequence
into a binary buffer and passing it to some function.

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

I'm currently working on exactly that, which is why I'm finding all
these inconsistencies. While it's always good to have more than one
implementation to look for common patterns, it's usually pretty bad
when there's only one, because then it's fairly likely that the code
won't work very well on a second implementation.

So what we need to be doing now is find ways to make this work equally
well for Exynos and Tegra (as well as across panel implementations) and
then hope it'll be sufficiently good so that we don't have to redesign
once again when the third implementation comes around.

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

I think either way would work. I slightly prefer the direct layering
because we're implementing a standard protocol on top of another
standard protocol. So it's fairly similar to networking protocols or
even filesystems on top of the block layer.

Therefore I think it makes sense to have an API that takes DCS inputs,
such as command and payload, translates them to DSI and passes raw DSI
to the driver, since presumably that's what the driver needs. That way
we can move the protocol bits into common functions and let drivers do
what they do best: interface with the hardware.

> What about ECC? Should it be present also in tx buffer?

ECC and CS are somewhat special. Hardware will probably provide a way to
offload ECC and CS computations in the majority of cases, so it doesn't
make much sense to compute them by default. I see two ways to achieve
that: a) provide flags in struct mipi_dsi_host for supported features so
that the protocol code can compute it if necessary or b) provide helpers
that take a raw DSI packet and compute its CS or ECC.

While I couldn't find an explicit reference to a CS or ECC enable bit,
there also doesn't seem to be any code to compute it, so I assume it's
always enabled (or at least by default). Tegra can also compute them in
hardware, so I don't think we need to concern ourselves with that at
this point.

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/20140724/5d4a823f/attachment.sig>