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: Wed, 15 Oct 2014 13:11:52 +0200 Message-ID: <20141015111151.GB12196@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> <20141014141630.GB26015@ulmo> <543E53FC.5030403@samsung.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1818832300==" Return-path: Received: from mail-wg0-f43.google.com (mail-wg0-f43.google.com [74.125.82.43]) by gabe.freedesktop.org (Postfix) with ESMTP id 22F906E185 for ; Wed, 15 Oct 2014 04:11:55 -0700 (PDT) Received: by mail-wg0-f43.google.com with SMTP id m15so1053627wgh.26 for ; Wed, 15 Oct 2014 04:11:54 -0700 (PDT) In-Reply-To: <543E53FC.5030403@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 --===============1818832300== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="xgyAXRrhYN0wYx8y" Content-Disposition: inline --xgyAXRrhYN0wYx8y Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Wed, Oct 15, 2014 at 01:01:16PM +0200, Andrzej Hajda wrote: > On 10/14/2014 04:16 PM, Thierry Reding wrote: > > 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 co= mmand > >>>>>>>>> byte to be embedded within the write buffer whereas mipi_dsi_dc= s_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 dri= ver use > >>>>>>>>> it instead. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Thierry Reding > >>>>>>>>> --- > >>>>>>>>> Changes in v2: > >>>>>>>>> - provide mipi_dsi_dcs_write_buffer() for backwards compatibili= ty > >>>>>>>>> > >>>>>>>>> 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/d= rm_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_devi= ce *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 p= ayload > >>>>>>>>> + * @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 n= egative 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 =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= depending on > >>>>>>>>> + * the command payload length. > >>>>>>>>> + * > >>>>>>>>> + * Return: The number of bytes successfully transmitted or a n= egative 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 head= er > >>>>>>>>> + * bytes 1 and 2 and have a payload containing the DCS comma= nd > >>>>>>>>> + * 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, t= hen the > >>>>>>> packet is a long write, so we need three bytes (command & word co= unt) in > >>>>>>> addition to the payload. For short writes, len <=3D 1 and we only= need 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 AP= I. > >>>>>>>> mipi_dsi_msg.tx_buf contains only data and mipi_dsi_msg.tx_len c= ontains > >>>>>>>> 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 th= e whole > >>>>>>> point of it. The else branch here is to optimize for the case whe= re a > >>>>>>> command has no payload. In that case there is no need for allocat= ing 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 d= ata > >>> and into what data it would be transformed. That obviously breaks as > >>> soon as two implementations make different assumptions. =3D) > >> In previous version of this patch [1] you have made different assumpti= on, > >> and in the discussion you were clearly aware of the current implementa= tion, > >> so your reaction here surprises me little bit. Maybe some misunderstan= ding. > >> Anyway I am glad we are now both aware of the problem. > >> > >> [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? > According to DSI specification we can describe DSI > message/command/transaction > on two levels: > 1. Application layer - message is described by a triplet (data_type, > channel_id, data). > 2. Protocol layer - message is described as a four byte packet header, > optionally > followed by packet data (payload) and checksum (which we can skip it > here as it should be generated by HW). >=20 > In the current API the 1st approach is used. There is no defined > standard how to program > DSI host to generate specific message, so this approach seems to be the > most natural in general case. >=20 > On the other side all DSI hosts I looked at use approach based on > protocol layer, ie. > packet header is written to one FIFO register and payload to another > (exynos, intel, omap) or the same (tegra). >=20 > Your proposition is something close to 2nd approach, maybe it would be > better to convert to completely to 2nd approach: >=20 > struct mipi_dsi_msg { > u8 header[4]; /* u32 header; ??? */ > void *payload; /* NULL in case of short packets */ > size_t payload_len; > ... > }; >=20 > Anyway, I think conversion to protocol layer should be done by helper > but this helper should be called rather from dsi host, > otherwise we can encounter some day dsi host which we need to feed with > data differently and we will need to perform > back-conversion from protocol layer to app layer, it will not be > difficult it will be just ugly :) >=20 > What about creating helpers to make dsi packets from current dsi > message. Sth like: >=20 > ... drm_mipi_create_packet(struct mipi_dsi_packet *pkt, struct > mipi_dsi_msg *msg) > { > if (long packet) { > pkt->header =3D ...; > pkt->payload =3D msg->tx_buf; > pkt->len =3D msg->tx_len; > } else { > pkt->header =3D ...; > pkt->payload =3D NULL; > pkt->len =3D 0; > } > } >=20 > This way in dsi host we will have sth like: >=20 > host_transfer(...msg) { > struct mipi_dsi_packet pkt; >=20 > drm_mipi_create_packet(&pkt, msg); >=20 > writel(msg.header, REG_HDR); >=20 > for (i =3D 0; i < pkt.len; i +=3D 4) > writel(pkt.payload[i..i+3], REG_PAYLOAD); > } >=20 > Please note that this way we can avoid dynamic memory > allocation/copy/deallocation, I know it is cheap, but it does not seems > to be necessary. Yes, that sounds pretty reasonable on a quick glance. I guess the mipi_dsi_create_packet() function could take an additional parameter at some point to generate the header ECC and checksum for hardware that can't do it on the fly. I'll give this a shot to see how it's going to work in practice. Given how Exynos currently uses the application layer interface it should be possible to use the helper as a means to transition more easily. Do you plan on converting to the helper once it's available? It seems like it would fit your hardware better than the current approach. Thierry --xgyAXRrhYN0wYx8y Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2 iQIcBAEBAgAGBQJUPlZ3AAoJEN0jrNd/PrOhcmYQAJqvSPEcxVdmqQDW6ixExq9f 2i+3dBfQT0CNMAjKmeCL0kaTTbhhtT6V0yCR4VwZ5GkrLaARLVwCZuIUU3FxtPbL dyFXCT2wI/knmPBTESrt41Z+dXLtAFpb3fmGFPpvQNXNktJb/UCCXKeAIheEH030 PvviikGOpo//ycSPb9wuDZsSbajKS56idI22byPtkSXdt3Wy+x06N753U6dxMyUd jCjTPFwsMCJ+0NeDAt92tfZHCYhlcyFC7xNdvq0iteGpl54P2E8xyRtF49WIQwUi QY1h+owDnaeyJWGOzGJ+MZsyDxe5KMkBmcpOn4FJISd9vyH+5MeMbEoBYfm98BaK buo1XqNkzL6QlkEwmgNvIESYxac1sjgOO1mIh12dykEF8ybGWw1XIgrhkmx4U6n7 wZ/7sCHy+k5jSglRU+55MBBWiy9heW3ubznZFFTsIHKGqkjWUa4LVyxFWvqzh4Ju uM4iaKDV+MB5Cm6iNjS9cD494/PD07jofMR4WdOJA1NadnOgmpk/flTkFeP7B+lO ZMAl02XyV9NtGq3I4DRKDc+fAvxzkiQuPJOJTW+2N7hnAwfufMEE17nYD0aZc7fj 71h1GMKP9o1icAag6OrWdoJli9WVB2aWvxJRyTiKbbGSsboiASaTpi80lZ1LNu/a CcI/gvdUUv8Kcqf9208z =8+nR -----END PGP SIGNATURE----- --xgyAXRrhYN0wYx8y-- --===============1818832300== 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 --===============1818832300==--