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

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8591 bytes --]

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?

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

> >> But in the first place it would be good to know why do you want to
> >> change the API? What are benefits of this solution?
> > I've already explained this elsewhere.
> 
> Where? I remember only one discussion where you claimed this is better
> solution
> for you [1], but without explanation.

I explained this in an earlier reply to you in this thread.

> Do you have patches/repo for tegra with transfer callback implemented
> with this semantic?
> Maybe looking at the code will be helpful.

I posted these patches for review to dri-devel yesterday, so they should
be available there. If you prefer patchwork, see:

	https://patchwork.kernel.org/patch/5075161/

And look for tegra_dsi_host_transfer().

Thierry

[-- Attachment #1.2: Type: application/pgp-signature, Size: 819 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel