From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH v2 01/15] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical Date: Tue, 14 Oct 2014 16:16:36 +0200 Message-ID: <20141014141630.GB26015@ulmo> References: <1413195395-3355-1-git-send-email-thierry.reding@gmail.com> <543CD820.8080606@samsung.com> <20141014094400.GD18993@ulmo> <543CFD17.10806@samsung.com> <20141014105730.GH18993@ulmo> <543D0838.6060707@samsung.com> <20141014112956.GA5057@ulmo> <543D2AD6.4080602@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1518306534==" Return-path: Received: from mail-wg0-f42.google.com (mail-wg0-f42.google.com [74.125.82.42]) by gabe.freedesktop.org (Postfix) with ESMTP id 6E98689C82 for ; Tue, 14 Oct 2014 07:16:39 -0700 (PDT) Received: by mail-wg0-f42.google.com with SMTP id z12so10895632wgg.13 for ; Tue, 14 Oct 2014 07:16:38 -0700 (PDT) In-Reply-To: <543D2AD6.4080602@samsung.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Andrzej Hajda Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org --===============1518306534== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="tsOsTdHNUZQcU9Ye" Content-Disposition: inline --tsOsTdHNUZQcU9Ye Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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 > >>>>>>> > >>>>>>> Currently the mipi_dsi_dcs_write() function requires the DCS comm= and > >>>>>>> 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 c= oncern > >>>>>>> that moving to the new API may be less efficient. Provide a new f= unction > >>>>>>> with the old semantics for those cases and make the S6E8AA0 drive= r use > >>>>>>> it instead. > >>>>>>> > >>>>>>> Signed-off-by: Thierry Reding > >>>>>>> --- > >>>>>>> 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); > >>>>>>> =20 > >>>>>>> /** > >>>>>>> - * 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 pay= load > >>>>>>> + * @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 d= epending on > >>>>>>> + * the command payload length. > >>>>>>> + * > >>>>>>> + * Return: The number of bytes successfully transmitted or a neg= ative error > >>>>>>> + * code on failure. > >>>>>>> */ > >>>>>>> -ssize_t mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, const vo= id *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 =3D dsi->host->ops; > >>>>>>> struct mipi_dsi_msg msg =3D { > >>>>>>> .channel =3D dsi->channel, > >>>>>>> .tx_buf =3D data, > >>>>>>> .tx_len =3D len > >>>>>>> }; > >>>>>>> =20 > >>>>>>> - if (!ops || !ops->transfer) > >>>>>>> + if (!dsi->host->ops || !dsi->host->ops->transfer) > >>>>>>> return -ENOSYS; > >>>>>>> =20 > >>>>>>> switch (len) { > >>>>>>> case 0: > >>>>>>> return -EINVAL; > >>>>>>> + > >>>>>>> case 1: > >>>>>>> msg.type =3D MIPI_DSI_DCS_SHORT_WRITE; > >>>>>>> break; > >>>>>>> + > >>>>>>> case 2: > >>>>>>> msg.type =3D MIPI_DSI_DCS_SHORT_WRITE_PARAM; > >>>>>>> break; > >>>>>>> + > >>>>>>> + default: > >>>>>>> + msg.type =3D 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 d= epending on > >>>>>>> + * the command payload length. > >>>>>>> + * > >>>>>>> + * Return: The number of bytes successfully transmitted or a neg= ative 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 =3D 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 =3D 3 + len; > >>>>>>> + else > >>>>>>> + size =3D 1 + len; > >>>>>> I guess "size =3D 2" would be better here. > >>>>> This is on purpose because it documents the format. If len > 1, the= n the > >>>>> packet is a long write, so we need three bytes (command & word coun= t) in > >>>>> addition to the payload. For short writes, len <=3D 1 and we only n= eed one > >>>>> extra byte (command). > >>>>> > >>>>>>> + > >>>>>>> + tx =3D kmalloc(size, GFP_KERNEL); > >>>>>>> + if (!tx) > >>>>>>> + return -ENOMEM; > >>>>>>> + > >>>>>>> + /* write word count to header for DCS long write packets */ > >>>>>>> + if (len > 1) { > >>>>>>> + tx[offset++] =3D ((1 + len) >> 0) & 0xff; > >>>>>>> + tx[offset++] =3D ((1 + len) >> 8) & 0xff; > >>>>>>> + } > >>>>>>> + > >>>>>>> + /* write the DCS command byte followed by the payload */ > >>>>>>> + tx[offset++] =3D cmd; > >>>>>>> + memcpy(tx + offset, data, len); > >>>>>>> + } else { > >>>>>>> + tx =3D &cmd; > >>>>>>> + size =3D 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 con= tains > >>>>>> lenght of this data. Now you try to embed length of data into tx_b= uf 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 allocatin= g 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 b= uffer > >> 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. =3D) >=20 > In previous version of this patch [1] you have made different assumption, > and in the discussion you were clearly aware of the current implementatio= n, > so your reaction here surprises me little bit. Maybe some misunderstandin= g. > Anyway I am glad we are now both aware of the problem. >=20 > [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? Thierry --tsOsTdHNUZQcU9Ye Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUPTA+AAoJEN0jrNd/PrOhkxwP/A+BWICMh4ZleOfkjIZ/DOJa bTfWM4A8BwKqW69oJOGtMKObgPRGItMSLrk6Q1wbo3wkf7ktlXEc3gG9N1fCYeIi gcLjZ/ag+KGFwNUNoCyoyTCtq+fFKCY4JEqUAhx8CoZoovkOccK3TKg3RrCkvwCM 5+JaSWdFi3wf/PU5V8tuNKl0RVMfCTlq2yy/9TItfxV/JH4zlpC7PPMTHXVbzSr5 SpIHWbYuNAuIzkwgeQ/U2c9/XF3gLyyJ2KX05e8nYn1l8LUbo00vu9DTfKrO3Iif mwdTuNahEbR+SnV+qvhBeSTMV1Hv8fQIeoXQyay4UgVZuz7hiLy2r8CAkcN3TKGi SnkDQeth30GqS6/ZwiAbYlkJ9aRfILoiGr3QCgJ72exExsi7lKpqA4NcAiWHi2bl P9gPy3x+x63ARm1o94tiAvwoWIln7qAR7O2+q/kB2HDtGOHW0r+Hjo/DY0t0VonR Vwnx2qt/8AqUlel7kmfhl+FN9CBy5baKRVDKtT0EHku6qsbu7xODw3bUjkacydUU aPoKYfv/292LOJYHr4HeGZI96Fv1UwjbPMj2O3kLnVQtLQQ5Jle8MJjBmnHFPLbA ObcXnC3L5pzcGu/ePR2WUU4+3dgD/OtYarrFu12r9E8rJZi+eZnv4CnR8gGC8NNg Q3negI+7kh+XkzPfzmSp =+buz -----END PGP SIGNATURE----- --tsOsTdHNUZQcU9Ye-- --===============1518306534== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel --===============1518306534==--