Re: [PATCH v2 01/15] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Andrzej Hajda <a.hajda@samsung.com>]

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

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