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: 9327 bytes --]

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

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

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