[PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[Brian Norris]

This takes care of 2 TODOs in this driver, by using the common DSI
packet-marshalling code instead of our custom short/long write code.
This both saves us some duplicated code and gets us free support for
command types that weren't already part of our switch block (e.g.,
MIPI_DSI_GENERIC_LONG_WRITE).

The code logic stays mostly intact, except that it becomes unnecessary
to split the short/long write functions, and we have to copy data a bit
more.

Along the way, I noticed that loop bounds were a little odd:

	while (DIV_ROUND_UP(len, pld_data_bytes))

This really was just supposed to be 'len != 0', so I made that more
clear.

Tested on RK3399 with some pending refactoring patches by Nickey Yang,
to make the Rockchip DSI driver wrap this common driver.

Signed-off-by: Brian Norris <briannorris@chromium.org>
---
Could use an extra look from folks. This looks like the correct trivial
transformation, but I'm not that familiar with DSI.

 drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 78 ++++++---------------------
 1 file changed, 16 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
index d9cca4fd66ec..2fed20e44dfe 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
@@ -136,10 +136,6 @@
 					 GEN_SW_0P_TX_LP)
 
 #define DSI_GEN_HDR			0x6c
-/* TODO These 2 defines will be reworked thanks to mipi_dsi_create_packet() */
-#define GEN_HDATA(data)			(((data) & 0xffff) << 8)
-#define GEN_HTYPE(type)			(((type) & 0xff) << 0)
-
 #define DSI_GEN_PLD_DATA		0x70
 
 #define DSI_CMD_PKT_STATUS		0x74
@@ -359,44 +355,15 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
 	return 0;
 }
 
-static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
-				       const struct mipi_dsi_msg *msg)
-{
-	const u8 *tx_buf = msg->tx_buf;
-	u16 data = 0;
-	u32 val;
-
-	if (msg->tx_len > 0)
-		data |= tx_buf[0];
-	if (msg->tx_len > 1)
-		data |= tx_buf[1] << 8;
-
-	if (msg->tx_len > 2) {
-		dev_err(dsi->dev, "too long tx buf length %zu for short write\n",
-			msg->tx_len);
-		return -EINVAL;
-	}
-
-	val = GEN_HDATA(data) | GEN_HTYPE(msg->type);
-	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, val);
-}
-
-static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
-				      const struct mipi_dsi_msg *msg)
+static int dw_mipi_dsi_dcs_write(struct dw_mipi_dsi *dsi,
+				 const struct mipi_dsi_packet *packet)
 {
-	const u8 *tx_buf = msg->tx_buf;
-	int len = msg->tx_len, pld_data_bytes = sizeof(u32), ret;
-	u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type);
+	const u8 *tx_buf = packet->payload;
+	int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret;
 	u32 remainder;
 	u32 val;
 
-	if (msg->tx_len < 3) {
-		dev_err(dsi->dev, "wrong tx buf length %zu for long write\n",
-			msg->tx_len);
-		return -EINVAL;
-	}
-
-	while (DIV_ROUND_UP(len, pld_data_bytes)) {
+	while (len) {
 		if (len < pld_data_bytes) {
 			remainder = 0;
 			memcpy(&remainder, tx_buf, len);
@@ -419,40 +386,27 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
 		}
 	}
 
-	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val);
+	remainder = 0;
+	memcpy(&remainder, packet->header, sizeof(packet->header));
+	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder);
 }
 
 static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
 					 const struct mipi_dsi_msg *msg)
 {
 	struct dw_mipi_dsi *dsi = host_to_dsi(host);
+	struct mipi_dsi_packet packet;
 	int ret;
 
-	/*
-	 * TODO dw drv improvements
-	 * use mipi_dsi_create_packet() instead of all following
-	 * functions and code (no switch cases, no
-	 * dw_mipi_dsi_dcs_short_write(), only the loop in long_write...)
-	 * and use packet.header...
-	 */
-	dw_mipi_message_config(dsi, msg);
-
-	switch (msg->type) {
-	case MIPI_DSI_DCS_SHORT_WRITE:
-	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
-	case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
-		ret = dw_mipi_dsi_dcs_short_write(dsi, msg);
-		break;
-	case MIPI_DSI_DCS_LONG_WRITE:
-		ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
-		break;
-	default:
-		dev_err(dsi->dev, "unsupported message type 0x%02x\n",
-			msg->type);
-		ret = -EINVAL;
+	ret = mipi_dsi_create_packet(&packet, msg);
+	if (ret) {
+		dev_err(dsi->dev, "failed to create packet: %d\n", ret);
+		return ret;
 	}
 
-	return ret;
+	dw_mipi_message_config(dsi, msg);
+
+	return dw_mipi_dsi_dcs_write(dsi, &packet);
 }
 
 static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
-- 
2.16.0.rc0.223.g4a4ac83678-goog

Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[Philippe CORNU]

Hi Brian,

And many thanks for implementing these TODOs.

On 01/06/2018 01:38 AM, Brian Norris wrote:
> This takes care of 2 TODOs in this driver, by using the common DSI
> packet-marshalling code instead of our custom short/long write code.
> This both saves us some duplicated code and gets us free support for
> command types that weren't already part of our switch block (e.g.,
> MIPI_DSI_GENERIC_LONG_WRITE).
> 
> The code logic stays mostly intact, except that it becomes unnecessary
> to split the short/long write functions, and we have to copy data a bit
> more.
> 
> Along the way, I noticed that loop bounds were a little odd:
> 
> 	while (DIV_ROUND_UP(len, pld_data_bytes))
> 
> This really was just supposed to be 'len != 0', so I made that more
> clear.
> 
> Tested on RK3399 with some pending refactoring patches by Nickey Yang,
> to make the Rockchip DSI driver wrap this common driver.
> 
> Signed-off-by: Brian Norris <briannorris@chromium.org>
> ---
> Could use an extra look from folks. This looks like the correct trivial
> transformation, but I'm not that familiar with DSI.
> 
>   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 78 ++++++---------------------
>   1 file changed, 16 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index d9cca4fd66ec..2fed20e44dfe 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -136,10 +136,6 @@
>   					 GEN_SW_0P_TX_LP)
>   
>   #define DSI_GEN_HDR			0x6c
> -/* TODO These 2 defines will be reworked thanks to mipi_dsi_create_packet() */
> -#define GEN_HDATA(data)			(((data) & 0xffff) << 8)
> -#define GEN_HTYPE(type)			(((type) & 0xff) << 0)
> -
>   #define DSI_GEN_PLD_DATA		0x70
>   
>   #define DSI_CMD_PKT_STATUS		0x74
> @@ -359,44 +355,15 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
>   	return 0;
>   }
>   
> -static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
> -				       const struct mipi_dsi_msg *msg)
> -{
> -	const u8 *tx_buf = msg->tx_buf;
> -	u16 data = 0;
> -	u32 val;
> -
> -	if (msg->tx_len > 0)
> -		data |= tx_buf[0];
> -	if (msg->tx_len > 1)
> -		data |= tx_buf[1] << 8;
> -
> -	if (msg->tx_len > 2) {
> -		dev_err(dsi->dev, "too long tx buf length %zu for short write\n",
> -			msg->tx_len);
> -		return -EINVAL;
> -	}
> -
> -	val = GEN_HDATA(data) | GEN_HTYPE(msg->type);
> -	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, val);
> -}
> -
> -static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
> -				      const struct mipi_dsi_msg *msg)
> +static int dw_mipi_dsi_dcs_write(struct dw_mipi_dsi *dsi,
> +				 const struct mipi_dsi_packet *packet)

Both DCS and Generic dsi transfers are managed by drm_mipi_dsi.c 
helpers. So maybe dw_mipi_dsi_dcs_write() should be renamed 
dw_mipi_dsi_write()...


>   {
> -	const u8 *tx_buf = msg->tx_buf;
> -	int len = msg->tx_len, pld_data_bytes = sizeof(u32), ret;
> -	u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type);
> +	const u8 *tx_buf = packet->payload;
> +	int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret;
>   	u32 remainder;
>   	u32 val;
>   
> -	if (msg->tx_len < 3) {
> -		dev_err(dsi->dev, "wrong tx buf length %zu for long write\n",
> -			msg->tx_len);
> -		return -EINVAL;
> -	}
> -
> -	while (DIV_ROUND_UP(len, pld_data_bytes)) {
> +	while (len) {
>   		if (len < pld_data_bytes) {
>   			remainder = 0;
>   			memcpy(&remainder, tx_buf, len);
> @@ -419,40 +386,27 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
>   		}
>   	}
>   
> -	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val);
> +	remainder = 0;
> +	memcpy(&remainder, packet->header, sizeof(packet->header));
> +	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder);
>   }
>   
>   static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>   					 const struct mipi_dsi_msg *msg)
>   {
>   	struct dw_mipi_dsi *dsi = host_to_dsi(host);
> +	struct mipi_dsi_packet packet;
>   	int ret;
>   
> -	/*
> -	 * TODO dw drv improvements
> -	 * use mipi_dsi_create_packet() instead of all following
> -	 * functions and code (no switch cases, no
> -	 * dw_mipi_dsi_dcs_short_write(), only the loop in long_write...)
> -	 * and use packet.header...
> -	 */
> -	dw_mipi_message_config(dsi, msg);
> -
> -	switch (msg->type) {
> -	case MIPI_DSI_DCS_SHORT_WRITE:
> -	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> -	case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
> -		ret = dw_mipi_dsi_dcs_short_write(dsi, msg);
> -		break;
> -	case MIPI_DSI_DCS_LONG_WRITE:
> -		ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
> -		break;
> -	default:
> -		dev_err(dsi->dev, "unsupported message type 0x%02x\n",
> -			msg->type);
> -		ret = -EINVAL;
> +	ret = mipi_dsi_create_packet(&packet, msg);
> +	if (ret) {
> +		dev_err(dsi->dev, "failed to create packet: %d\n", ret);
> +		return ret;
>   	}
>   
> -	return ret;
> +	dw_mipi_message_config(dsi, msg);
> +
> +	return dw_mipi_dsi_dcs_write(dsi, &packet);
>   }
>   
>   static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
> 

I performed some tests tracing all DSI_GEN_HDR & DSI_GEN_PLD_DATA reg 
writes with panel/panel-orisetech-otm8009a.c (using long dcs commands) 
before and after your patch and this is "100% perfect"!

So, apart the un-important "dcs" in dw_mipi_dsi_dcs_write() function name:

Reviewed-by: Philippe Cornu <philippe.cornu@st.com>
Tested-by: Philippe Cornu <philippe.cornu@st.com>

This clean-up will help a lot to add the dsi read feature in the future.

Very good patch Brian and big "thank you" !

Philippe :-)

Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[Brian Norris]

Hi Philippe,

On Tue, Jan 09, 2018 at 10:48:43AM +0000, Philippe CORNU wrote:
> Hi Brian,
> 
> And many thanks for implementing these TODOs.

And thanks for adding them; it gave me a better option than just adding
yet another switch case (MIPI_DSI_GENERIC_LONG_WRITE) ;)

> On 01/06/2018 01:38 AM, Brian Norris wrote:
> > This takes care of 2 TODOs in this driver, by using the common DSI
> > packet-marshalling code instead of our custom short/long write code.
> > This both saves us some duplicated code and gets us free support for
> > command types that weren't already part of our switch block (e.g.,
> > MIPI_DSI_GENERIC_LONG_WRITE).
> > 
> > The code logic stays mostly intact, except that it becomes unnecessary
> > to split the short/long write functions, and we have to copy data a bit
> > more.
> > 
> > Along the way, I noticed that loop bounds were a little odd:
> > 
> > 	while (DIV_ROUND_UP(len, pld_data_bytes))
> > 
> > This really was just supposed to be 'len != 0', so I made that more
> > clear.
> > 
> > Tested on RK3399 with some pending refactoring patches by Nickey Yang,
> > to make the Rockchip DSI driver wrap this common driver.
> > 
> > Signed-off-by: Brian Norris <briannorris@chromium.org>
> > ---
> > Could use an extra look from folks. This looks like the correct trivial
> > transformation, but I'm not that familiar with DSI.
> > 
> >   drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 78 ++++++---------------------
> >   1 file changed, 16 insertions(+), 62 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > index d9cca4fd66ec..2fed20e44dfe 100644
> > --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> > @@ -136,10 +136,6 @@
> >   					 GEN_SW_0P_TX_LP)
> >   
> >   #define DSI_GEN_HDR			0x6c
> > -/* TODO These 2 defines will be reworked thanks to mipi_dsi_create_packet() */
> > -#define GEN_HDATA(data)			(((data) & 0xffff) << 8)
> > -#define GEN_HTYPE(type)			(((type) & 0xff) << 0)
> > -
> >   #define DSI_GEN_PLD_DATA		0x70
> >   
> >   #define DSI_CMD_PKT_STATUS		0x74
> > @@ -359,44 +355,15 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
> >   	return 0;
> >   }
> >   
> > -static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
> > -				       const struct mipi_dsi_msg *msg)
> > -{
> > -	const u8 *tx_buf = msg->tx_buf;
> > -	u16 data = 0;
> > -	u32 val;
> > -
> > -	if (msg->tx_len > 0)
> > -		data |= tx_buf[0];
> > -	if (msg->tx_len > 1)
> > -		data |= tx_buf[1] << 8;
> > -
> > -	if (msg->tx_len > 2) {
> > -		dev_err(dsi->dev, "too long tx buf length %zu for short write\n",
> > -			msg->tx_len);
> > -		return -EINVAL;
> > -	}
> > -
> > -	val = GEN_HDATA(data) | GEN_HTYPE(msg->type);
> > -	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, val);
> > -}
> > -
> > -static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
> > -				      const struct mipi_dsi_msg *msg)
> > +static int dw_mipi_dsi_dcs_write(struct dw_mipi_dsi *dsi,
> > +				 const struct mipi_dsi_packet *packet)
> 
> Both DCS and Generic dsi transfers are managed by drm_mipi_dsi.c 
> helpers. So maybe dw_mipi_dsi_dcs_write() should be renamed 
> dw_mipi_dsi_write()...

Ah, good point. I really meant to remove the _dcs naming too, but I
guess I missed it. Will follow up.

> >   {
> > -	const u8 *tx_buf = msg->tx_buf;
> > -	int len = msg->tx_len, pld_data_bytes = sizeof(u32), ret;
> > -	u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type);
> > +	const u8 *tx_buf = packet->payload;
> > +	int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret;
> >   	u32 remainder;
> >   	u32 val;
> >   
> > -	if (msg->tx_len < 3) {
> > -		dev_err(dsi->dev, "wrong tx buf length %zu for long write\n",
> > -			msg->tx_len);
> > -		return -EINVAL;
> > -	}
> > -
> > -	while (DIV_ROUND_UP(len, pld_data_bytes)) {
> > +	while (len) {
> >   		if (len < pld_data_bytes) {
> >   			remainder = 0;
> >   			memcpy(&remainder, tx_buf, len);
> > @@ -419,40 +386,27 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
> >   		}
> >   	}
> >   
> > -	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val);
> > +	remainder = 0;
> > +	memcpy(&remainder, packet->header, sizeof(packet->header));

By the way: I don't think it's an issue that should block this patch,
since if I'm right, this function already is "broken", but isn't this
actually a bad way to handle byte-to-word marshalling? Particularly,
we're copying bytes into a word in LE ordering, but then we later write
them to IO registers with writel() (which does endian swapping).

So I think we have an endianness problem on BE systems.

One solution would be to write these to IO registers with a non-swapped
writel() (e.g., __raw_writel()? but that's not very nice...). Another
would be to avoid memcpy, and just read this out a word at a time --
that works fine for the aligned pieces, but not so well for any
non-aligned bits ('if (len < pld_data_bytes)' above) I think?

WDYT?

> > +	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder);
> >   }
> >   
> >   static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
> >   					 const struct mipi_dsi_msg *msg)
> >   {
> >   	struct dw_mipi_dsi *dsi = host_to_dsi(host);
> > +	struct mipi_dsi_packet packet;
> >   	int ret;
> >   
> > -	/*
> > -	 * TODO dw drv improvements
> > -	 * use mipi_dsi_create_packet() instead of all following
> > -	 * functions and code (no switch cases, no
> > -	 * dw_mipi_dsi_dcs_short_write(), only the loop in long_write...)
> > -	 * and use packet.header...
> > -	 */
> > -	dw_mipi_message_config(dsi, msg);
> > -
> > -	switch (msg->type) {
> > -	case MIPI_DSI_DCS_SHORT_WRITE:
> > -	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
> > -	case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
> > -		ret = dw_mipi_dsi_dcs_short_write(dsi, msg);
> > -		break;
> > -	case MIPI_DSI_DCS_LONG_WRITE:
> > -		ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
> > -		break;
> > -	default:
> > -		dev_err(dsi->dev, "unsupported message type 0x%02x\n",
> > -			msg->type);
> > -		ret = -EINVAL;
> > +	ret = mipi_dsi_create_packet(&packet, msg);
> > +	if (ret) {
> > +		dev_err(dsi->dev, "failed to create packet: %d\n", ret);
> > +		return ret;
> >   	}
> >   
> > -	return ret;
> > +	dw_mipi_message_config(dsi, msg);
> > +
> > +	return dw_mipi_dsi_dcs_write(dsi, &packet);
> >   }
> >   
> >   static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
> > 
> 
> I performed some tests tracing all DSI_GEN_HDR & DSI_GEN_PLD_DATA reg 
> writes with panel/panel-orisetech-otm8009a.c (using long dcs commands) 
> before and after your patch and this is "100% perfect"!
> 
> So, apart the un-important "dcs" in dw_mipi_dsi_dcs_write() function name:
> 
> Reviewed-by: Philippe Cornu <philippe.cornu@st.com>
> Tested-by: Philippe Cornu <philippe.cornu@st.com>
> 
> This clean-up will help a lot to add the dsi read feature in the future.
> 
> Very good patch Brian and big "thank you" !

Thanks for the review and test! I'll likely send a v2 with only the
naming change + your tags, and I'll see about what do about the
endianness issues I noticed as a follow-up.

Brian

Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[Philippe CORNU]

Hi Brian,

On 01/09/2018 07:55 PM, Brian Norris wrote:
> Hi Philippe,
> 
> On Tue, Jan 09, 2018 at 10:48:43AM +0000, Philippe CORNU wrote:
>> Hi Brian,
>>
>> And many thanks for implementing these TODOs.
> 
> And thanks for adding them; it gave me a better option than just adding
> yet another switch case (MIPI_DSI_GENERIC_LONG_WRITE) ;)
> 
>> On 01/06/2018 01:38 AM, Brian Norris wrote:
>>> This takes care of 2 TODOs in this driver, by using the common DSI
>>> packet-marshalling code instead of our custom short/long write code.
>>> This both saves us some duplicated code and gets us free support for
>>> command types that weren't already part of our switch block (e.g.,
>>> MIPI_DSI_GENERIC_LONG_WRITE).
>>>
>>> The code logic stays mostly intact, except that it becomes unnecessary
>>> to split the short/long write functions, and we have to copy data a bit
>>> more.
>>>
>>> Along the way, I noticed that loop bounds were a little odd:
>>>
>>> 	while (DIV_ROUND_UP(len, pld_data_bytes))
>>>
>>> This really was just supposed to be 'len != 0', so I made that more
>>> clear.
>>>
>>> Tested on RK3399 with some pending refactoring patches by Nickey Yang,
>>> to make the Rockchip DSI driver wrap this common driver.
>>>
>>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>>> ---
>>> Could use an extra look from folks. This looks like the correct trivial
>>> transformation, but I'm not that familiar with DSI.
>>>
>>>    drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 78 ++++++---------------------
>>>    1 file changed, 16 insertions(+), 62 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>> index d9cca4fd66ec..2fed20e44dfe 100644
>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>> @@ -136,10 +136,6 @@
>>>    					 GEN_SW_0P_TX_LP)
>>>    
>>>    #define DSI_GEN_HDR			0x6c
>>> -/* TODO These 2 defines will be reworked thanks to mipi_dsi_create_packet() */
>>> -#define GEN_HDATA(data)			(((data) & 0xffff) << 8)
>>> -#define GEN_HTYPE(type)			(((type) & 0xff) << 0)
>>> -
>>>    #define DSI_GEN_PLD_DATA		0x70
>>>    
>>>    #define DSI_CMD_PKT_STATUS		0x74
>>> @@ -359,44 +355,15 @@ static int dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
>>>    	return 0;
>>>    }
>>>    
>>> -static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
>>> -				       const struct mipi_dsi_msg *msg)
>>> -{
>>> -	const u8 *tx_buf = msg->tx_buf;
>>> -	u16 data = 0;
>>> -	u32 val;
>>> -
>>> -	if (msg->tx_len > 0)
>>> -		data |= tx_buf[0];
>>> -	if (msg->tx_len > 1)
>>> -		data |= tx_buf[1] << 8;
>>> -
>>> -	if (msg->tx_len > 2) {
>>> -		dev_err(dsi->dev, "too long tx buf length %zu for short write\n",
>>> -			msg->tx_len);
>>> -		return -EINVAL;
>>> -	}
>>> -
>>> -	val = GEN_HDATA(data) | GEN_HTYPE(msg->type);
>>> -	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, val);
>>> -}
>>> -
>>> -static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
>>> -				      const struct mipi_dsi_msg *msg)
>>> +static int dw_mipi_dsi_dcs_write(struct dw_mipi_dsi *dsi,
>>> +				 const struct mipi_dsi_packet *packet)
>>
>> Both DCS and Generic dsi transfers are managed by drm_mipi_dsi.c
>> helpers. So maybe dw_mipi_dsi_dcs_write() should be renamed
>> dw_mipi_dsi_write()...
> 
> Ah, good point. I really meant to remove the _dcs naming too, but I
> guess I missed it. Will follow up.
> 
>>>    {
>>> -	const u8 *tx_buf = msg->tx_buf;
>>> -	int len = msg->tx_len, pld_data_bytes = sizeof(u32), ret;
>>> -	u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type);
>>> +	const u8 *tx_buf = packet->payload;
>>> +	int len = packet->payload_length, pld_data_bytes = sizeof(u32), ret;
>>>    	u32 remainder;
>>>    	u32 val;
>>>    
>>> -	if (msg->tx_len < 3) {
>>> -		dev_err(dsi->dev, "wrong tx buf length %zu for long write\n",
>>> -			msg->tx_len);
>>> -		return -EINVAL;
>>> -	}
>>> -
>>> -	while (DIV_ROUND_UP(len, pld_data_bytes)) {
>>> +	while (len) {
>>>    		if (len < pld_data_bytes) {
>>>    			remainder = 0;
>>>    			memcpy(&remainder, tx_buf, len);
>>> @@ -419,40 +386,27 @@ static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
>>>    		}
>>>    	}
>>>    
>>> -	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val);
>>> +	remainder = 0;
>>> +	memcpy(&remainder, packet->header, sizeof(packet->header));
> 
> By the way: I don't think it's an issue that should block this patch,
> since if I'm right, this function already is "broken", but isn't this
> actually a bad way to handle byte-to-word marshalling? Particularly,
> we're copying bytes into a word in LE ordering, but then we later write
> them to IO registers with writel() (which does endian swapping).
> 
> So I think we have an endianness problem on BE systems.
> 
> One solution would be to write these to IO registers with a non-swapped
> writel() (e.g., __raw_writel()? but that's not very nice...). Another
> would be to avoid memcpy, and just read this out a word at a time --
> that works fine for the aligned pieces, but not so well for any
> non-aligned bits ('if (len < pld_data_bytes)' above) I think?
> 
> WDYT?
> 

To be honest, I do not really like the memcpy here too and I agree with 
you regarding the BE issue.

My first "stm" driver (ie. before using this "freescale/rockchip" 
dw-mipi-dsi driver with the memcpy) used the "exact" same code as the 
Tegra dsi tegra_dsi_writesl() function with the 2 loops.

https://elixir.free-electrons.com/linux/v4.14/source/drivers/gpu/drm/tegra/dsi.c#L1248

IMHO, it is better than memcpy...
I added these 3 "documentation" lines, maybe we may reuse them or 
something similar...

/*
  * Write 8-bit payload data into the 32-bit payload data register.
  * ex: payload data "0x01, 0x02, 0x03, 0x04, 0x05, 0x06" will become
  * "0x04030201 0x00000605" 32-bit writes
  */

Not sure it helps to fix the BE issue but we may add a TODO stating that 
"this loop has not been tested on BE"...

What is your opinion?

Many thanks
Philippe :-)


>>> +	return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder);
>>>    }
>>>    
>>>    static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>>>    					 const struct mipi_dsi_msg *msg)
>>>    {
>>>    	struct dw_mipi_dsi *dsi = host_to_dsi(host);
>>> +	struct mipi_dsi_packet packet;
>>>    	int ret;
>>>    
>>> -	/*
>>> -	 * TODO dw drv improvements
>>> -	 * use mipi_dsi_create_packet() instead of all following
>>> -	 * functions and code (no switch cases, no
>>> -	 * dw_mipi_dsi_dcs_short_write(), only the loop in long_write...)
>>> -	 * and use packet.header...
>>> -	 */
>>> -	dw_mipi_message_config(dsi, msg);
>>> -
>>> -	switch (msg->type) {
>>> -	case MIPI_DSI_DCS_SHORT_WRITE:
>>> -	case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
>>> -	case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
>>> -		ret = dw_mipi_dsi_dcs_short_write(dsi, msg);
>>> -		break;
>>> -	case MIPI_DSI_DCS_LONG_WRITE:
>>> -		ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
>>> -		break;
>>> -	default:
>>> -		dev_err(dsi->dev, "unsupported message type 0x%02x\n",
>>> -			msg->type);
>>> -		ret = -EINVAL;
>>> +	ret = mipi_dsi_create_packet(&packet, msg);
>>> +	if (ret) {
>>> +		dev_err(dsi->dev, "failed to create packet: %d\n", ret);
>>> +		return ret;
>>>    	}
>>>    
>>> -	return ret;
>>> +	dw_mipi_message_config(dsi, msg);
>>> +
>>> +	return dw_mipi_dsi_dcs_write(dsi, &packet);
>>>    }
>>>    
>>>    static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
>>>
>>
>> I performed some tests tracing all DSI_GEN_HDR & DSI_GEN_PLD_DATA reg
>> writes with panel/panel-orisetech-otm8009a.c (using long dcs commands)
>> before and after your patch and this is "100% perfect"!
>>
>> So, apart the un-important "dcs" in dw_mipi_dsi_dcs_write() function name:
>>
>> Reviewed-by: Philippe Cornu <philippe.cornu@st.com>
>> Tested-by: Philippe Cornu <philippe.cornu@st.com>
>>
>> This clean-up will help a lot to add the dsi read feature in the future.
>>
>> Very good patch Brian and big "thank you" !
> 
> Thanks for the review and test! I'll likely send a v2 with only the
> naming change + your tags, and I'll see about what do about the
> endianness issues I noticed as a follow-up.
> 
> Brian
> 

Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[Philippe CORNU]

Hi Brian,

On 01/11/2018 12:16 PM, Philippe CORNU wrote:
> Hi Brian,
> 
> On 01/09/2018 07:55 PM, Brian Norris wrote:
>> Hi Philippe,
>>
>> On Tue, Jan 09, 2018 at 10:48:43AM +0000, Philippe CORNU wrote:
>>> Hi Brian,
>>>
>>> And many thanks for implementing these TODOs.
>>
>> And thanks for adding them; it gave me a better option than just adding
>> yet another switch case (MIPI_DSI_GENERIC_LONG_WRITE) ;)
>>
>>> On 01/06/2018 01:38 AM, Brian Norris wrote:
>>>> This takes care of 2 TODOs in this driver, by using the common DSI
>>>> packet-marshalling code instead of our custom short/long write code.
>>>> This both saves us some duplicated code and gets us free support for
>>>> command types that weren't already part of our switch block (e.g.,
>>>> MIPI_DSI_GENERIC_LONG_WRITE).
>>>>
>>>> The code logic stays mostly intact, except that it becomes unnecessary
>>>> to split the short/long write functions, and we have to copy data a bit
>>>> more.
>>>>
>>>> Along the way, I noticed that loop bounds were a little odd:
>>>>
>>>>     while (DIV_ROUND_UP(len, pld_data_bytes))
>>>>
>>>> This really was just supposed to be 'len != 0', so I made that more
>>>> clear.
>>>>
>>>> Tested on RK3399 with some pending refactoring patches by Nickey Yang,
>>>> to make the Rockchip DSI driver wrap this common driver.
>>>>
>>>> Signed-off-by: Brian Norris <briannorris@chromium.org>
>>>> ---
>>>> Could use an extra look from folks. This looks like the correct trivial
>>>> transformation, but I'm not that familiar with DSI.
>>>>
>>>>    drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c | 78 
>>>> ++++++---------------------
>>>>    1 file changed, 16 insertions(+), 62 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c 
>>>> b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>>> index d9cca4fd66ec..2fed20e44dfe 100644
>>>> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>>> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
>>>> @@ -136,10 +136,6 @@
>>>>                         GEN_SW_0P_TX_LP)
>>>>    #define DSI_GEN_HDR            0x6c
>>>> -/* TODO These 2 defines will be reworked thanks to 
>>>> mipi_dsi_create_packet() */
>>>> -#define GEN_HDATA(data)            (((data) & 0xffff) << 8)
>>>> -#define GEN_HTYPE(type)            (((type) & 0xff) << 0)
>>>> -
>>>>    #define DSI_GEN_PLD_DATA        0x70
>>>>    #define DSI_CMD_PKT_STATUS        0x74
>>>> @@ -359,44 +355,15 @@ static int 
>>>> dw_mipi_dsi_gen_pkt_hdr_write(struct dw_mipi_dsi *dsi, u32 hdr_val)
>>>>        return 0;
>>>>    }
>>>> -static int dw_mipi_dsi_dcs_short_write(struct dw_mipi_dsi *dsi,
>>>> -                       const struct mipi_dsi_msg *msg)
>>>> -{
>>>> -    const u8 *tx_buf = msg->tx_buf;
>>>> -    u16 data = 0;
>>>> -    u32 val;
>>>> -
>>>> -    if (msg->tx_len > 0)
>>>> -        data |= tx_buf[0];
>>>> -    if (msg->tx_len > 1)
>>>> -        data |= tx_buf[1] << 8;
>>>> -
>>>> -    if (msg->tx_len > 2) {
>>>> -        dev_err(dsi->dev, "too long tx buf length %zu for short 
>>>> write\n",
>>>> -            msg->tx_len);
>>>> -        return -EINVAL;
>>>> -    }
>>>> -
>>>> -    val = GEN_HDATA(data) | GEN_HTYPE(msg->type);
>>>> -    return dw_mipi_dsi_gen_pkt_hdr_write(dsi, val);
>>>> -}
>>>> -
>>>> -static int dw_mipi_dsi_dcs_long_write(struct dw_mipi_dsi *dsi,
>>>> -                      const struct mipi_dsi_msg *msg)
>>>> +static int dw_mipi_dsi_dcs_write(struct dw_mipi_dsi *dsi,
>>>> +                 const struct mipi_dsi_packet *packet)
>>>
>>> Both DCS and Generic dsi transfers are managed by drm_mipi_dsi.c
>>> helpers. So maybe dw_mipi_dsi_dcs_write() should be renamed
>>> dw_mipi_dsi_write()...
>>
>> Ah, good point. I really meant to remove the _dcs naming too, but I
>> guess I missed it. Will follow up.
>>
>>>>    {
>>>> -    const u8 *tx_buf = msg->tx_buf;
>>>> -    int len = msg->tx_len, pld_data_bytes = sizeof(u32), ret;
>>>> -    u32 hdr_val = GEN_HDATA(msg->tx_len) | GEN_HTYPE(msg->type);
>>>> +    const u8 *tx_buf = packet->payload;
>>>> +    int len = packet->payload_length, pld_data_bytes = sizeof(u32), 
>>>> ret;
>>>>        u32 remainder;
>>>>        u32 val;
>>>> -    if (msg->tx_len < 3) {
>>>> -        dev_err(dsi->dev, "wrong tx buf length %zu for long write\n",
>>>> -            msg->tx_len);
>>>> -        return -EINVAL;
>>>> -    }
>>>> -
>>>> -    while (DIV_ROUND_UP(len, pld_data_bytes)) {
>>>> +    while (len) {
>>>>            if (len < pld_data_bytes) {
>>>>                remainder = 0;
>>>>                memcpy(&remainder, tx_buf, len);
>>>> @@ -419,40 +386,27 @@ static int dw_mipi_dsi_dcs_long_write(struct 
>>>> dw_mipi_dsi *dsi,
>>>>            }
>>>>        }
>>>> -    return dw_mipi_dsi_gen_pkt_hdr_write(dsi, hdr_val);
>>>> +    remainder = 0;
>>>> +    memcpy(&remainder, packet->header, sizeof(packet->header));
>>
>> By the way: I don't think it's an issue that should block this patch,
>> since if I'm right, this function already is "broken", but isn't this
>> actually a bad way to handle byte-to-word marshalling? Particularly,
>> we're copying bytes into a word in LE ordering, but then we later write
>> them to IO registers with writel() (which does endian swapping).
>>
>> So I think we have an endianness problem on BE systems.
>>
>> One solution would be to write these to IO registers with a non-swapped
>> writel() (e.g., __raw_writel()? but that's not very nice...). Another
>> would be to avoid memcpy, and just read this out a word at a time --
>> that works fine for the aligned pieces, but not so well for any
>> non-aligned bits ('if (len < pld_data_bytes)' above) I think?
>>
>> WDYT?
>>
> 
> To be honest, I do not really like the memcpy here too and I agree with 
> you regarding the BE issue.
> 
> My first "stm" driver (ie. before using this "freescale/rockchip" 
> dw-mipi-dsi driver with the memcpy) used the "exact" same code as the 
> Tegra dsi tegra_dsi_writesl() function with the 2 loops.
> 
> https://elixir.free-electrons.com/linux/v4.14/source/drivers/gpu/drm/tegra/dsi.c#L1248 
> 
> 
> IMHO, it is better than memcpy...
> I added these 3 "documentation" lines, maybe we may reuse them or 
> something similar...
> 
> /*
>   * Write 8-bit payload data into the 32-bit payload data register.
>   * ex: payload data "0x01, 0x02, 0x03, 0x04, 0x05, 0x06" will become
>   * "0x04030201 0x00000605" 32-bit writes
>   */
> 
> Not sure it helps to fix the BE issue but we may add a TODO stating that 
> "this loop has not been tested on BE"...
> 
> What is your opinion?


As your patch has been merged, I have few short questions and for each 
related new patch, I would like to know if you prefer that I implement 
it or if you prefer to do it by yourself, it's really like you want, on 
my side, no problem to make them all, some or none, I don't want us to 
implement these in parallel :-)

* Do you have any opinion regarding Tegra-like loops vs the memcpy? (see 
my comment above) If you think the Tegra-like loops is a better approach 
than memcpy, there is a small patch to write.

* Returned value with number of bytes received/transferred: there is a 
small patch to write

* Regarding read operations: I propose to add a TODO + DRM_WARN in case 
someone want to use the API for read operations. Note that I plan to 
implement the read feature but I do not know yet when and maybe Rockchip 
people already have something ~ready?

Many thanks,
Philippe :-)

> 
> Many thanks
> Philippe :-)
> 
> 
>>>> +    return dw_mipi_dsi_gen_pkt_hdr_write(dsi, remainder);
>>>>    }
>>>>    static ssize_t dw_mipi_dsi_host_transfer(struct mipi_dsi_host *host,
>>>>                         const struct mipi_dsi_msg *msg)
>>>>    {
>>>>        struct dw_mipi_dsi *dsi = host_to_dsi(host);
>>>> +    struct mipi_dsi_packet packet;
>>>>        int ret;
>>>> -    /*
>>>> -     * TODO dw drv improvements
>>>> -     * use mipi_dsi_create_packet() instead of all following
>>>> -     * functions and code (no switch cases, no
>>>> -     * dw_mipi_dsi_dcs_short_write(), only the loop in long_write...)
>>>> -     * and use packet.header...
>>>> -     */
>>>> -    dw_mipi_message_config(dsi, msg);
>>>> -
>>>> -    switch (msg->type) {
>>>> -    case MIPI_DSI_DCS_SHORT_WRITE:
>>>> -    case MIPI_DSI_DCS_SHORT_WRITE_PARAM:
>>>> -    case MIPI_DSI_SET_MAXIMUM_RETURN_PACKET_SIZE:
>>>> -        ret = dw_mipi_dsi_dcs_short_write(dsi, msg);
>>>> -        break;
>>>> -    case MIPI_DSI_DCS_LONG_WRITE:
>>>> -        ret = dw_mipi_dsi_dcs_long_write(dsi, msg);
>>>> -        break;
>>>> -    default:
>>>> -        dev_err(dsi->dev, "unsupported message type 0x%02x\n",
>>>> -            msg->type);
>>>> -        ret = -EINVAL;
>>>> +    ret = mipi_dsi_create_packet(&packet, msg);
>>>> +    if (ret) {
>>>> +        dev_err(dsi->dev, "failed to create packet: %d\n", ret);
>>>> +        return ret;
>>>>        }
>>>> -    return ret;
>>>> +    dw_mipi_message_config(dsi, msg);
>>>> +
>>>> +    return dw_mipi_dsi_dcs_write(dsi, &packet);
>>>>    }
>>>>    static const struct mipi_dsi_host_ops dw_mipi_dsi_host_ops = {
>>>>
>>>
>>> I performed some tests tracing all DSI_GEN_HDR & DSI_GEN_PLD_DATA reg
>>> writes with panel/panel-orisetech-otm8009a.c (using long dcs commands)
>>> before and after your patch and this is "100% perfect"!
>>>
>>> So, apart the un-important "dcs" in dw_mipi_dsi_dcs_write() function 
>>> name:
>>>
>>> Reviewed-by: Philippe Cornu <philippe.cornu@st.com>
>>> Tested-by: Philippe Cornu <philippe.cornu@st.com>
>>>
>>> This clean-up will help a lot to add the dsi read feature in the future.
>>>
>>> Very good patch Brian and big "thank you" !
>>
>> Thanks for the review and test! I'll likely send a v2 with only the
>> naming change + your tags, and I'll see about what do about the
>> endianness issues I noticed as a follow-up.
>>
>> Brian
>>

Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[Brian Norris]

Hi Philippe,

On Thu, Jan 18, 2018 at 11:40:48AM +0000, Philippe CORNU wrote:
> On 01/11/2018 12:16 PM, Philippe CORNU wrote:
> > To be honest, I do not really like the memcpy here too and I agree with 
> > you regarding the BE issue.
> > 
> > My first "stm" driver (ie. before using this "freescale/rockchip" 
> > dw-mipi-dsi driver with the memcpy) used the "exact" same code as the 
> > Tegra dsi tegra_dsi_writesl() function with the 2 loops.
> > 
> > https://elixir.free-electrons.com/linux/v4.14/source/drivers/gpu/drm/tegra/dsi.c#L1248 
> > 
> > 
> > IMHO, it is better than memcpy...
> > I added these 3 "documentation" lines, maybe we may reuse them or 
> > something similar...
> > 
> > /*
> >   * Write 8-bit payload data into the 32-bit payload data register.
> >   * ex: payload data "0x01, 0x02, 0x03, 0x04, 0x05, 0x06" will become
> >   * "0x04030201 0x00000605" 32-bit writes
> >   */
> > 
> > Not sure it helps to fix the BE issue but we may add a TODO stating that 
> > "this loop has not been tested on BE"...
> > 
> > What is your opinion?

I'm sorry, I don't think I noticed your reply here. I'm trying to unbury
some email, but that's sometimes a losing battle...

That code actually does look correct, and it's perhaps marginally
better-looking in my opinion. It's up to you if you want to propose
another patch :) At this point, it's only a matter of nice code, not
correctness I believe.

> As your patch has been merged, I have few short questions and for each 
> related new patch, I would like to know if you prefer that I implement 
> it or if you prefer to do it by yourself, it's really like you want, on 
> my side, no problem to make them all, some or none, I don't want us to 
> implement these in parallel :-)
> 
> * Do you have any opinion regarding Tegra-like loops vs the memcpy? (see 
> my comment above) If you think the Tegra-like loops is a better approach 
> than memcpy, there is a small patch to write.

My opinion is above.

> * Returned value with number of bytes received/transferred: there is a 
> small patch to write

I don't think I followed that one very well. I'm not sure my opinion
really matters, as long as you get someone else to agree. I do not plan
to write any such patch in the near term.

> * Regarding read operations: I propose to add a TODO + DRM_WARN in case 
> someone want to use the API for read operations. Note that I plan to 
> implement the read feature but I do not know yet when and maybe Rockchip 
> people already have something ~ready?

The warning would be nice to do now, regardless.

Rockchip folks wrote up something for read support here [1], but it's
based on a semi-forked version of the driver (we're trying to clean up
the divergence, but it's not there yet). Perhaps it would provide useful
fodder for your work. I don't think Rockchip is immediately working on
upstreaming this particular patch, so it's totally fair to handle it
yourself. It's got the GPL sign-off ;)

Brian

[1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/863347

Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[Philippe CORNU]

Hi Brian,

On 01/23/2018 10:15 PM, Brian Norris wrote:
> Hi Philippe,
> 
> On Thu, Jan 18, 2018 at 11:40:48AM +0000, Philippe CORNU wrote:
>> On 01/11/2018 12:16 PM, Philippe CORNU wrote:
>>> To be honest, I do not really like the memcpy here too and I agree with
>>> you regarding the BE issue.
>>>
>>> My first "stm" driver (ie. before using this "freescale/rockchip"
>>> dw-mipi-dsi driver with the memcpy) used the "exact" same code as the
>>> Tegra dsi tegra_dsi_writesl() function with the 2 loops.
>>>
>>> https://elixir.free-electrons.com/linux/v4.14/source/drivers/gpu/drm/tegra/dsi.c#L1248
>>>
>>>
>>> IMHO, it is better than memcpy...
>>> I added these 3 "documentation" lines, maybe we may reuse them or
>>> something similar...
>>>
>>> /*
>>>    * Write 8-bit payload data into the 32-bit payload data register.
>>>    * ex: payload data "0x01, 0x02, 0x03, 0x04, 0x05, 0x06" will become
>>>    * "0x04030201 0x00000605" 32-bit writes
>>>    */
>>>
>>> Not sure it helps to fix the BE issue but we may add a TODO stating that
>>> "this loop has not been tested on BE"...
>>>
>>> What is your opinion?
> 
> I'm sorry, I don't think I noticed your reply here. I'm trying to unbury
> some email, but that's sometimes a losing battle...
> 
> That code actually does look correct, and it's perhaps marginally
> better-looking in my opinion. It's up to you if you want to propose
> another patch :) At this point, it's only a matter of nice code, not
> correctness I believe.
> 
>> As your patch has been merged, I have few short questions and for each
>> related new patch, I would like to know if you prefer that I implement
>> it or if you prefer to do it by yourself, it's really like you want, on
>> my side, no problem to make them all, some or none, I don't want us to
>> implement these in parallel :-)
>>
>> * Do you have any opinion regarding Tegra-like loops vs the memcpy? (see
>> my comment above) If you think the Tegra-like loops is a better approach
>> than memcpy, there is a small patch to write.
> 
> My opinion is above.
> 

I do not know yet if I will send a patch but several reasons may push me 
to do it:
* Andrzej proposed a nicer code in his last review so it means the 
actual code with memcpy's is "not so nice" (even if it works fine)
* Several dsi drivers use the Tegra-like loops (Tegra, intel,... ) and 
in vc4/exynos/mtk drivers memcpy is not used, msm uses memcpy... well, 
not sure it is then a good argument, different solutions for different hw...
* Coming cadence dsi bridge driver uses Tegra-like loops.
* I think my read function will also have Tegra-like loops, if it is the 
case, it could be nice to have something homogeneous...

Anyway, it is not an important point : )

>> * Returned value with number of bytes received/transferred: there is a
>> small patch to write
> 
> I don't think I followed that one very well. I'm not sure my opinion
> really matters, as long as you get someone else to agree. I do not plan
> to write any such patch in the near term.
> 
>> * Regarding read operations: I propose to add a TODO + DRM_WARN in case
>> someone want to use the API for read operations. Note that I plan to
>> implement the read feature but I do not know yet when and maybe Rockchip
>> people already have something ~ready?
> 
> The warning would be nice to do now, regardless.
> 
> Rockchip folks wrote up something for read support here [1], but it's
> based on a semi-forked version of the driver (we're trying to clean up
> the divergence, but it's not there yet). Perhaps it would provide useful
> fodder for your work. I don't think Rockchip is immediately working on
> upstreaming this particular patch, so it's totally fair to handle it
> yourself. It's got the GPL sign-off ;)
> 
> Brian
> 
> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/863347
> 

Very good information, I will have a look,
many thanks
Philippe :-)

Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[Andrzej Hajda]

On 24.01.2018 10:51, Philippe CORNU wrote:
> Hi Brian,
>
> On 01/23/2018 10:15 PM, Brian Norris wrote:
>> Hi Philippe,
>>
>> On Thu, Jan 18, 2018 at 11:40:48AM +0000, Philippe CORNU wrote:
>>> On 01/11/2018 12:16 PM, Philippe CORNU wrote:
>>>> To be honest, I do not really like the memcpy here too and I agree with
>>>> you regarding the BE issue.
>>>>
>>>> My first "stm" driver (ie. before using this "freescale/rockchip"
>>>> dw-mipi-dsi driver with the memcpy) used the "exact" same code as the
>>>> Tegra dsi tegra_dsi_writesl() function with the 2 loops.
>>>>
>>>> https://elixir.free-electrons.com/linux/v4.14/source/drivers/gpu/drm/tegra/dsi.c#L1248
>>>>
>>>>
>>>> IMHO, it is better than memcpy...
>>>> I added these 3 "documentation" lines, maybe we may reuse them or
>>>> something similar...
>>>>
>>>> /*
>>>>    * Write 8-bit payload data into the 32-bit payload data register.
>>>>    * ex: payload data "0x01, 0x02, 0x03, 0x04, 0x05, 0x06" will become
>>>>    * "0x04030201 0x00000605" 32-bit writes
>>>>    */
>>>>
>>>> Not sure it helps to fix the BE issue but we may add a TODO stating that
>>>> "this loop has not been tested on BE"...
>>>>
>>>> What is your opinion?
>> I'm sorry, I don't think I noticed your reply here. I'm trying to unbury
>> some email, but that's sometimes a losing battle...
>>
>> That code actually does look correct, and it's perhaps marginally
>> better-looking in my opinion. It's up to you if you want to propose
>> another patch :) At this point, it's only a matter of nice code, not
>> correctness I believe.
>>
>>> As your patch has been merged, I have few short questions and for each
>>> related new patch, I would like to know if you prefer that I implement
>>> it or if you prefer to do it by yourself, it's really like you want, on
>>> my side, no problem to make them all, some or none, I don't want us to
>>> implement these in parallel :-)
>>>
>>> * Do you have any opinion regarding Tegra-like loops vs the memcpy? (see
>>> my comment above) If you think the Tegra-like loops is a better approach
>>> than memcpy, there is a small patch to write.
>> My opinion is above.
>>
> I do not know yet if I will send a patch but several reasons may push me 
> to do it:
> * Andrzej proposed a nicer code in his last review so it means the 
> actual code with memcpy's is "not so nice" (even if it works fine)

I was not against memcpy, I have just suggested to abstract the code out
to some helper function.
Regarding memcpy vs loop I would prefer memcpy - simpler code, but it is
looks less important that abstracting out.

Regards
Andrzej


> * Several dsi drivers use the Tegra-like loops (Tegra, intel,... ) and 
> in vc4/exynos/mtk drivers memcpy is not used, msm uses memcpy... well, 
> not sure it is then a good argument, different solutions for different hw...
> * Coming cadence dsi bridge driver uses Tegra-like loops.
> * I think my read function will also have Tegra-like loops, if it is the 
> case, it could be nice to have something homogeneous...
>
> Anyway, it is not an important point : )
>
>>> * Returned value with number of bytes received/transferred: there is a
>>> small patch to write
>> I don't think I followed that one very well. I'm not sure my opinion
>> really matters, as long as you get someone else to agree. I do not plan
>> to write any such patch in the near term.
>>
>>> * Regarding read operations: I propose to add a TODO + DRM_WARN in case
>>> someone want to use the API for read operations. Note that I plan to
>>> implement the read feature but I do not know yet when and maybe Rockchip
>>> people already have something ~ready?
>> The warning would be nice to do now, regardless.
>>
>> Rockchip folks wrote up something for read support here [1], but it's
>> based on a semi-forked version of the driver (we're trying to clean up
>> the divergence, but it's not there yet). Perhaps it would provide useful
>> fodder for your work. I don't think Rockchip is immediately working on
>> upstreaming this particular patch, so it's totally fair to handle it
>> yourself. It's got the GPL sign-off ;)
>>
>> Brian
>>
>> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/863347
>>
> Very good information, I will have a look,
> many thanks
> Philippe :-)

Re: [PATCH] drm/bridge/synopsys: dsi: use common mipi_dsi_create_packet()

[Philippe CORNU]

Hi Andrzej,


On 01/25/2018 12:07 PM, Andrzej Hajda wrote:
> On 24.01.2018 10:51, Philippe CORNU wrote:
>> Hi Brian,
>>
>> On 01/23/2018 10:15 PM, Brian Norris wrote:
>>> Hi Philippe,
>>>
>>> On Thu, Jan 18, 2018 at 11:40:48AM +0000, Philippe CORNU wrote:
>>>> On 01/11/2018 12:16 PM, Philippe CORNU wrote:
>>>>> To be honest, I do not really like the memcpy here too and I agree with
>>>>> you regarding the BE issue.
>>>>>
>>>>> My first "stm" driver (ie. before using this "freescale/rockchip"
>>>>> dw-mipi-dsi driver with the memcpy) used the "exact" same code as the
>>>>> Tegra dsi tegra_dsi_writesl() function with the 2 loops.
>>>>>
>>>>> https://elixir.free-electrons.com/linux/v4.14/source/drivers/gpu/drm/tegra/dsi.c#L1248
>>>>>
>>>>>
>>>>> IMHO, it is better than memcpy...
>>>>> I added these 3 "documentation" lines, maybe we may reuse them or
>>>>> something similar...
>>>>>
>>>>> /*
>>>>>     * Write 8-bit payload data into the 32-bit payload data register.
>>>>>     * ex: payload data "0x01, 0x02, 0x03, 0x04, 0x05, 0x06" will become
>>>>>     * "0x04030201 0x00000605" 32-bit writes
>>>>>     */
>>>>>
>>>>> Not sure it helps to fix the BE issue but we may add a TODO stating that
>>>>> "this loop has not been tested on BE"...
>>>>>
>>>>> What is your opinion?
>>> I'm sorry, I don't think I noticed your reply here. I'm trying to unbury
>>> some email, but that's sometimes a losing battle...
>>>
>>> That code actually does look correct, and it's perhaps marginally
>>> better-looking in my opinion. It's up to you if you want to propose
>>> another patch :) At this point, it's only a matter of nice code, not
>>> correctness I believe.
>>>
>>>> As your patch has been merged, I have few short questions and for each
>>>> related new patch, I would like to know if you prefer that I implement
>>>> it or if you prefer to do it by yourself, it's really like you want, on
>>>> my side, no problem to make them all, some or none, I don't want us to
>>>> implement these in parallel :-)
>>>>
>>>> * Do you have any opinion regarding Tegra-like loops vs the memcpy? (see
>>>> my comment above) If you think the Tegra-like loops is a better approach
>>>> than memcpy, there is a small patch to write.
>>> My opinion is above.
>>>
>> I do not know yet if I will send a patch but several reasons may push me
>> to do it:
>> * Andrzej proposed a nicer code in his last review so it means the
>> actual code with memcpy's is "not so nice" (even if it works fine)
> 
> I was not against memcpy, I have just suggested to abstract the code out
> to some helper function.
> Regarding memcpy vs loop I would prefer memcpy - simpler code, but it is
> looks less important that abstracting out.
> 
> Regards
> Andrzej
> 
> 
Many thanks for the clarification. Good to know your preference here for 
memcpy. So then if someone decides to rework this piece of code, all 
these arguments in this email thread will help.

Many thanks
Philippe :-)

>> * Several dsi drivers use the Tegra-like loops (Tegra, intel,... ) and
>> in vc4/exynos/mtk drivers memcpy is not used, msm uses memcpy... well,
>> not sure it is then a good argument, different solutions for different hw...
>> * Coming cadence dsi bridge driver uses Tegra-like loops.
>> * I think my read function will also have Tegra-like loops, if it is the
>> case, it could be nice to have something homogeneous...
>>
>> Anyway, it is not an important point : )
>>
>>>> * Returned value with number of bytes received/transferred: there is a
>>>> small patch to write
>>> I don't think I followed that one very well. I'm not sure my opinion
>>> really matters, as long as you get someone else to agree. I do not plan
>>> to write any such patch in the near term.
>>>
>>>> * Regarding read operations: I propose to add a TODO + DRM_WARN in case
>>>> someone want to use the API for read operations. Note that I plan to
>>>> implement the read feature but I do not know yet when and maybe Rockchip
>>>> people already have something ~ready?
>>> The warning would be nice to do now, regardless.
>>>
>>> Rockchip folks wrote up something for read support here [1], but it's
>>> based on a semi-forked version of the driver (we're trying to clean up
>>> the divergence, but it's not there yet). Perhaps it would provide useful
>>> fodder for your work. I don't think Rockchip is immediately working on
>>> upstreaming this particular patch, so it's totally fair to handle it
>>> yourself. It's got the GPL sign-off ;)
>>>
>>> Brian
>>>
>>> [1] https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/863347
>>>
>> Very good information, I will have a look,
>> many thanks
>> Philippe :-)
> 
>