On 10/15/2014 01:11 PM, Thierry Reding wrote:
> 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@xxxxxxxxxx>
>>>>>>>>>>>
>>>>>>>>>>> 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@xxxxxxxxxx>
>>>>>>>>>>> ---
>>>>>>>>>>> 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?
Yes.
Regards
Andrzej
> It seems like it
> would fit your hardware better than the current approach.
>
> Thierry
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
