[PATCH v2 01/15] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical
Thierry Reding
thierry.reding at gmail.com
Wed Oct 15 04:11:52 PDT 2014
On Wed, Oct 15, 2014 at 01:01:16PM +0200, Andrzej Hajda wrote:
> On 10/14/2014 04:16 PM, Thierry Reding wrote:
> > On Tue, Oct 14, 2014 at 03:53:26PM +0200, Andrzej Hajda wrote:
> >> On 10/14/2014 01:29 PM, Thierry Reding wrote:
> >>> On Tue, Oct 14, 2014 at 01:25:44PM +0200, Andrzej Hajda wrote:
> >>>> On 10/14/2014 12:57 PM, Thierry Reding wrote:
> >>>>> On Tue, Oct 14, 2014 at 12:38:15PM +0200, Andrzej Hajda wrote:
> >>>>>> On 10/14/2014 11:44 AM, Thierry Reding wrote:
> >>>>>>> On Tue, Oct 14, 2014 at 10:00:32AM +0200, Andrzej Hajda wrote:
> >>>>>>>> On 10/13/2014 12:16 PM, 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().
> >>>>>>>>>
> >>>>>>>>> The S6E8AA0 driver relies on the old asymmetric API and there's concern
> >>>>>>>>> that moving to the new API may be less efficient. Provide a new function
> >>>>>>>>> with the old semantics for those cases and make the S6E8AA0 driver use
> >>>>>>>>> it instead.
> >>>>>>>>>
> >>>>>>>>> Signed-off-by: Thierry Reding <treding at nvidia.com>
> >>>>>>>>> ---
> >>>>>>>>> Changes in v2:
> >>>>>>>>> - provide mipi_dsi_dcs_write_buffer() for backwards compatibility
> >>>>>>>>>
> >>>>>>>>> drivers/gpu/drm/drm_mipi_dsi.c | 127 +++++++++++++++++++++++++++++-----
> >>>>>>>>> drivers/gpu/drm/panel/panel-s6e8aa0.c | 2 +-
> >>>>>>>>> include/drm/drm_mipi_dsi.h | 6 +-
> >>>>>>>>> 3 files changed, 114 insertions(+), 21 deletions(-)
> >>>>>>>>>
> >>>>>>>>> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> >>>>>>>>> index eb6dfe52cab2..1702ffd07986 100644
> >>>>>>>>> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> >>>>>>>>> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> >>>>>>>>> @@ -199,33 +199,120 @@ int mipi_dsi_detach(struct mipi_dsi_device *dsi)
> >>>>>>>>> EXPORT_SYMBOL(mipi_dsi_detach);
> >>>>>>>>>
> >>>>>>>>> /**
> >>>>>>>>> - * mipi_dsi_dcs_write - send DCS write command
> >>>>>>>>> - * @dsi: DSI device
> >>>>>>>>> - * @data: pointer to the command followed by parameters
> >>>>>>>>> - * @len: length of @data
> >>>>>>>>> + * mipi_dsi_dcs_write_buffer() - transmit a DCS command with payload
> >>>>>>>>> + * @dsi: DSI peripheral device
> >>>>>>>>> + * @data: buffer containing data to be transmitted
> >>>>>>>>> + * @len: size of transmission buffer
> >>>>>>>>> + *
> >>>>>>>>> + * This function will automatically choose the right data type depending on
> >>>>>>>>> + * the command payload length.
> >>>>>>>>> + *
> >>>>>>>>> + * Return: The number of bytes successfully transmitted or a negative error
> >>>>>>>>> + * code on failure.
> >>>>>>>>> */
> >>>>>>>>> -ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const void *data,
> >>>>>>>>> - size_t len)
> >>>>>>>>> +ssize_t mipi_dsi_dcs_write_buffer(struct mipi_dsi_device *dsi,
> >>>>>>>>> + const void *data, size_t len)
> >>>>>>>>> {
> >>>>>>>>> - const struct mipi_dsi_host_ops *ops = dsi->host->ops;
> >>>>>>>>> struct mipi_dsi_msg msg = {
> >>>>>>>>> .channel = dsi->channel,
> >>>>>>>>> .tx_buf = data,
> >>>>>>>>> .tx_len = len
> >>>>>>>>> };
> >>>>>>>>>
> >>>>>>>>> - if (!ops || !ops->transfer)
> >>>>>>>>> + if (!dsi->host->ops || !dsi->host->ops->transfer)
> >>>>>>>>> return -ENOSYS;
> >>>>>>>>>
> >>>>>>>>> switch (len) {
> >>>>>>>>> case 0:
> >>>>>>>>> return -EINVAL;
> >>>>>>>>> +
> >>>>>>>>> case 1:
> >>>>>>>>> msg.type = MIPI_DSI_DCS_SHORT_WRITE;
> >>>>>>>>> break;
> >>>>>>>>> +
> >>>>>>>>> case 2:
> >>>>>>>>> msg.type = MIPI_DSI_DCS_SHORT_WRITE_PARAM;
> >>>>>>>>> break;
> >>>>>>>>> +
> >>>>>>>>> + default:
> >>>>>>>>> + msg.type = MIPI_DSI_DCS_LONG_WRITE;
> >>>>>>>>> + break;
> >>>>>>>>> + }
> >>>>>>>>> +
> >>>>>>>>> + return dsi->host->ops->transfer(dsi->host, &msg);
> >>>>>>>>> +}
> >>>>>>>>> +EXPORT_SYMBOL(mipi_dsi_dcs_write_buffer);
> >>>>>>>>> +
> >>>>>>>>> +/**
> >>>>>>>>> + * mipi_dsi_dcs_write() - send DCS write command
> >>>>>>>>> + * @dsi: DSI peripheral device
> >>>>>>>>> + * @cmd: DCS command
> >>>>>>>>> + * @data: buffer containing the command payload
> >>>>>>>>> + * @len: command payload length
> >>>>>>>>> + *
> >>>>>>>>> + * This function will automatically choose the right data type depending on
> >>>>>>>>> + * the command payload length.
> >>>>>>>>> + *
> >>>>>>>>> + * Return: The number of bytes successfully transmitted or a negative error
> >>>>>>>>> + * code on failure.
> >>>>>>>>> + */
> >>>>>>>>> +ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, u8 cmd,
> >>>>>>>>> + const void *data, size_t len)
> >>>>>>>>> +{
> >>>>>>>>> + struct mipi_dsi_msg msg;
> >>>>>>>>> + ssize_t err;
> >>>>>>>>> + size_t size;
> >>>>>>>>> + u8 *tx;
> >>>>>>>>> +
> >>>>>>>>> + if (!dsi->host->ops || !dsi->host->ops->transfer)
> >>>>>>>>> + return -ENOSYS;
> >>>>>>>>> +
> >>>>>>>>> + if (len > 0) {
> >>>>>>>>> + unsigned int offset = 0;
> >>>>>>>>> +
> >>>>>>>>> + /*
> >>>>>>>>> + * DCS long write packets contain the word count in the header
> >>>>>>>>> + * bytes 1 and 2 and have a payload containing the DCS command
> >>>>>>>>> + * byte folowed by word count minus one bytes.
> >>>>>>>>> + *
> >>>>>>>>> + * DCS short write packets encode the DCS command and up to
> >>>>>>>>> + * one parameter in header bytes 1 and 2.
> >>>>>>>>> + */
> >>>>>>>>> + if (len > 1)
> >>>>>>>>> + size = 3 + len;
> >>>>>>>>> + else
> >>>>>>>>> + size = 1 + len;
> >>>>>>>> I guess "size = 2" would be better here.
> >>>>>>> This is on purpose because it documents the format. If len > 1, then the
> >>>>>>> packet is a long write, so we need three bytes (command & word count) in
> >>>>>>> addition to the payload. For short writes, len <= 1 and we only need one
> >>>>>>> extra byte (command).
> >>>>>>>
> >>>>>>>>> +
> >>>>>>>>> + tx = kmalloc(size, GFP_KERNEL);
> >>>>>>>>> + if (!tx)
> >>>>>>>>> + return -ENOMEM;
> >>>>>>>>> +
> >>>>>>>>> + /* write word count to header for DCS long write packets */
> >>>>>>>>> + if (len > 1) {
> >>>>>>>>> + tx[offset++] = ((1 + len) >> 0) & 0xff;
> >>>>>>>>> + tx[offset++] = ((1 + len) >> 8) & 0xff;
> >>>>>>>>> + }
> >>>>>>>>> +
> >>>>>>>>> + /* write the DCS command byte followed by the payload */
> >>>>>>>>> + tx[offset++] = cmd;
> >>>>>>>>> + memcpy(tx + offset, data, len);
> >>>>>>>>> + } else {
> >>>>>>>>> + tx = &cmd;
> >>>>>>>>> + size = 1;
> >>>>>>>>> + }
> >>>>>>>> Contents of this conditional is incompatible with the current API.
> >>>>>>>> mipi_dsi_msg.tx_buf contains only data and mipi_dsi_msg.tx_len contains
> >>>>>>>> lenght of this data. Now you try to embed length of data into tx_buf and
> >>>>>>>> this breaks the API.
> >>>>>>> Huh? Of course the new API has different semantics, but that's the whole
> >>>>>>> point of it. The else branch here is to optimize for the case where a
> >>>>>>> command has no payload. In that case there is no need for allocating an
> >>>>>>> extra buffer, since the command byte is the only data transferred.
> >>>>>> If this is the whole point of it why patch description says nothing
> >>>>>> about it?
> >>>>> I thought the patch description said this. What do you think needs to be
> >>>>> added?
> >>>> In short, that new API assumes that transfer callback must interpret
> >>>> write buffer
> >>>> differently than before :) Ie that sometimes at the beginning of the buffer
> >>>> there will be additional bytes.
> >>> Right, we never defined exactly which part of code would take which data
> >>> and into what data it would be transformed. That obviously breaks as
> >>> soon as two implementations make different assumptions. =)
> >> In previous version of this patch [1] you have made different assumption,
> >> and in the discussion you were clearly aware of the current implementation,
> >> so your reaction here surprises me little bit. Maybe some misunderstanding.
> >> Anyway I am glad we are now both aware of the problem.
> >>
> >> [1]: http://permalink.gmane.org/gmane.comp.video.dri.devel/110647
> > It's possible I didn't realize the full complexity of the problem at the
> > time. To summarize, I think the helpers in the core should do as much
> > work as they possibly can, so that drivers don't have to duplicate the
> > same over and over again. That is, the DCS helpers that are under
> > discussion here should create a buffer that reflects the packet that is
> > to be sent to the DSI peripheral, including the specific layout of the
> > header. So a struct mipi_dsi_msg contains the following information:
> >
> > - .channel + .type make up the DI (Data ID) field in the packet
> > header
> >
> > for short packets:
> > - .tx_buf[0] and .tx_buf[1] correspond to Data 0 and Data 1
> > fields in the packet header (both of these are only present if
> > .tx_len is larger than 0 and 1, and default to 0 otherwise)
> >
> > for long packets:
> > - .tx_buf[0] and .tx_buf[1] correspond to the word count
> > - .tx_buf[2..] represent the payload (word count - 2 bytes)
> >
> > That's pretty much what section 8.4 General Packet Structure of the DSI
> > specification describes. This intentionally leaves out the header ECC
> > and 16-bit packet footer (checksum). These are typically implemented in
> > hardware, and if they aren't we can provide helpers that compute them
> > based on the header, and the payload in .tx_buf. That way all the packet
> > composition defined in the specification is handled by common code and a
> > driver only needs to have the hardware-specific knowledge, namely where
> > the various pieces need to be written in order to transmit them as
> > desired.
> >
> > Does that make sense?
> According to DSI specification we can describe DSI
> message/command/transaction
> on two levels:
> 1. Application layer - message is described by a triplet (data_type,
> channel_id, data).
> 2. Protocol layer - message is described as a four byte packet header,
> optionally
> followed by packet data (payload) and checksum (which we can skip it
> here as it should be generated by HW).
>
> In the current API the 1st approach is used. There is no defined
> standard how to program
> DSI host to generate specific message, so this approach seems to be the
> most natural in general case.
>
> On the other side all DSI hosts I looked at use approach based on
> protocol layer, ie.
> packet header is written to one FIFO register and payload to another
> (exynos, intel, omap) or the same (tegra).
>
> Your proposition is something close to 2nd approach, maybe it would be
> better to convert to completely to 2nd approach:
>
> struct mipi_dsi_msg {
> u8 header[4]; /* u32 header; ??? */
> void *payload; /* NULL in case of short packets */
> size_t payload_len;
> ...
> };
>
> Anyway, I think conversion to protocol layer should be done by helper
> but this helper should be called rather from dsi host,
> otherwise we can encounter some day dsi host which we need to feed with
> data differently and we will need to perform
> back-conversion from protocol layer to app layer, it will not be
> difficult it will be just ugly :)
>
> What about creating helpers to make dsi packets from current dsi
> message. Sth like:
>
> ... drm_mipi_create_packet(struct mipi_dsi_packet *pkt, struct
> mipi_dsi_msg *msg)
> {
> if (long packet) {
> pkt->header = ...;
> pkt->payload = msg->tx_buf;
> pkt->len = msg->tx_len;
> } else {
> pkt->header = ...;
> pkt->payload = NULL;
> pkt->len = 0;
> }
> }
>
> This way in dsi host we will have sth like:
>
> host_transfer(...msg) {
> struct mipi_dsi_packet pkt;
>
> drm_mipi_create_packet(&pkt, msg);
>
> writel(msg.header, REG_HDR);
>
> for (i = 0; i < pkt.len; i += 4)
> writel(pkt.payload[i..i+3], REG_PAYLOAD);
> }
>
> Please note that this way we can avoid dynamic memory
> allocation/copy/deallocation, I know it is cheap, but it does not seems
> to be necessary.
Yes, that sounds pretty reasonable on a quick glance. I guess the
mipi_dsi_create_packet() function could take an additional parameter at
some point to generate the header ECC and checksum for hardware that
can't do it on the fly.
I'll give this a shot to see how it's going to work in practice. Given
how Exynos currently uses the application layer interface it should be
possible to use the helper as a means to transition more easily. Do you
plan on converting to the helper once it's available? It seems like it
would fit your hardware better than the current approach.
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/20141015/c5f180f0/attachment.sig>
More information about the dri-devel
mailing list