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
Regards
Andrzej
>
> I'm glad we noticed the disconnect this early, where it's still pretty
> easy to fix.
>
>>>> It has nothing to do with helpers symmetry, this is serious API change.
>>> No, it's not. It replaces an existing API, mipi_dsi_dcs_write() with a
>>> different one, mipi_dsi_dcs_write_buffer() and converts the one call
>>> site where the function is used.
>>>
>>> Then it introduces a new function that behaves the same but uses a
>>> different signature that takes the DCS command byte as an explicit
>>> parameter instead of embedding the DCS command byte into the "payload"
>>> buffer.
>>>
>>> I don't understand why you think we're changing anything fundamental
>>> here.
>>>
>>>>>> And of course changing API would require also changing current users of
>>>>>> the API.
>>>>> There's a single user of this function and this patch switches it over
>>>>> to the compatibility function mipi_dsi_dcs_write_buffer().
>>>> Mostly panels are users of these functions and these functions uses
>>>> transfer callback internally. If we allow different semantic for
>>>> transfer msg
>>>> we will end up with panels cooperating only with specific dsi hosts.
>>>> I do not think it is good direction.
>>> That's not at all the intention. Both functions are supposed to keep
>>> working the same way. mipi_dsi_dcs_write_buffer() becomes what was
>>> previously called mipi_dsi_dcs_write() and mipi_dsi_dcs_write() is a
>>> new helper that concatenates a command byte with payload and passes
>>> it into mipi_dsi_dcs_write_buffer().
>>>
>>> So if you find that mipi_dsi_dcs_write_buffer() now does the wrong thing
>>> and breaks the s6e8aa0 panel, please point out what exactly is going
>>> wrong so that I can fix it.
>> The only wrong thing is that s6e8aa0 cannot work with tegra host
>> and lq101r1sx01 cannot work with Exynos. Not big deal at the moment
>> but clearly bad design. I guess there is non-zero chance that we will have
>> a panel which should cooperate with both dsi hosts.
> Agreed. The set of bytes transferred to the DSI host should be
> rigorously defined to make sure they all end up sending the same
> commands to the panels.
>
> Thierry
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
