diff mbox series

[v4,61/80] drm/omap: dsi: send nop instead of page & column

Message ID 20201124124538.660710-62-tomi.valkeinen@ti.com (mailing list archive)
State New
Headers show
Series Convert DSI code to use drm_mipi_dsi and drm_panel | expand

Commit Message

Tomi Valkeinen Nov. 24, 2020, 12:45 p.m. UTC
The OMAP DSI command mode panel driver used to send page & column
address before each frame update, and this code was moved into the DSI
host driver when converting it to the DRM bridge model.

However, it's not really required to send the page & column address
before each frame. It's also something that doesn't really belong to the
DSI host driver, so we should drop the code.

That said, frame updates break if we don't send _something_ between the
frames. A NOP command does the trick.

It is not clear if this behavior is as expected from a DSI command mode
frame transfer, or is it a feature/issue with OMAP DSI driver, or a
feature/issue in the command mode panel used. So this probably needs to
be revisited later.

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/dss/dsi.c | 41 +++++++++----------------------
 1 file changed, 12 insertions(+), 29 deletions(-)

Comments

Laurent Pinchart Nov. 30, 2020, 9:58 a.m. UTC | #1
Hi Tomi,

Thank you for the patch.

On Tue, Nov 24, 2020 at 02:45:19PM +0200, Tomi Valkeinen wrote:
> The OMAP DSI command mode panel driver used to send page & column
> address before each frame update, and this code was moved into the DSI
> host driver when converting it to the DRM bridge model.
> 
> However, it's not really required to send the page & column address
> before each frame. It's also something that doesn't really belong to the
> DSI host driver, so we should drop the code.
> 
> That said, frame updates break if we don't send _something_ between the
> frames. A NOP command does the trick.
> 
> It is not clear if this behavior is as expected from a DSI command mode
> frame transfer, or is it a feature/issue with OMAP DSI driver, or a
> feature/issue in the command mode panel used. So this probably needs to
> be revisited later.

This is important information, could you capture it in a comment in the
code ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  drivers/gpu/drm/omapdrm/dss/dsi.c | 41 +++++++++----------------------
>  1 file changed, 12 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index 7fee9cf8782d..746c2149fbbd 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -3884,35 +3884,19 @@ static int _dsi_update(struct dsi_data *dsi)
>  	return 0;
>  }
>  
> -static int _dsi_update_window(struct dsi_data *dsi, int channel,
> -			      int x, int y, int w, int h)
> -{
> -	int x1 = x, x2 = (x + w - 1);
> -	int y1 = y, y2 = (y + h - 1);
> -	u8 payloadX[5] = { MIPI_DCS_SET_COLUMN_ADDRESS,
> -			   x1 >> 8, x1 & 0xff, x2 >> 8, x2 & 0xff };
> -	u8 payloadY[5] = { MIPI_DCS_SET_PAGE_ADDRESS,
> -			   y1 >> 8, y1 & 0xff, y2 >> 8, y2 & 0xff };
> -	struct mipi_dsi_msg msgX = { 0 }, msgY = { 0 };
> -	int ret;
> +static int _dsi_send_nop(struct dsi_data *dsi, int channel)
> +{
> +	const u8 payload[] = { MIPI_DCS_NOP };
> +	const struct mipi_dsi_msg msg = {
> +		.channel = channel,
> +		.type = MIPI_DSI_DCS_SHORT_WRITE,
> +		.tx_len = 1,
> +		.tx_buf = payload,
> +	};
>  
>  	WARN_ON(!dsi_bus_is_locked(dsi));
>  
> -	msgX.type = MIPI_DSI_DCS_LONG_WRITE;
> -	msgX.channel = channel;
> -	msgX.tx_buf = payloadX;
> -	msgX.tx_len = sizeof(payloadX);
> -
> -	msgY.type = MIPI_DSI_DCS_LONG_WRITE;
> -	msgY.channel = channel;
> -	msgY.tx_buf = payloadY;
> -	msgY.tx_len = sizeof(payloadY);
> -
> -	ret = _omap_dsi_host_transfer(dsi, &msgX);
> -	if (ret != 0)
> -		return ret;
> -
> -	return _omap_dsi_host_transfer(dsi, &msgY);
> +	return _omap_dsi_host_transfer(dsi, &msg);
>  }
>  
>  static int dsi_update_channel(struct omap_dss_device *dssdev, int channel)
> @@ -3944,10 +3928,9 @@ static int dsi_update_channel(struct omap_dss_device *dssdev, int channel)
>  
>  	dsi_set_ulps_auto(dsi, false);
>  
> -	r = _dsi_update_window(dsi, channel, 0, 0, dsi->vm.hactive,
> -			       dsi->vm.vactive);
> +	r = _dsi_send_nop(dsi, channel);
>  	if (r < 0) {
> -		DSSWARN("window update error: %d\n", r);
> +		DSSWARN("failed to send nop between frames: %d\n", r);
>  		goto err;
>  	}
>
Tomi Valkeinen Dec. 1, 2020, 10:59 a.m. UTC | #2
On 30/11/2020 11:58, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Tue, Nov 24, 2020 at 02:45:19PM +0200, Tomi Valkeinen wrote:
>> The OMAP DSI command mode panel driver used to send page & column
>> address before each frame update, and this code was moved into the DSI
>> host driver when converting it to the DRM bridge model.
>>
>> However, it's not really required to send the page & column address
>> before each frame. It's also something that doesn't really belong to the
>> DSI host driver, so we should drop the code.
>>
>> That said, frame updates break if we don't send _something_ between the
>> frames. A NOP command does the trick.
>>
>> It is not clear if this behavior is as expected from a DSI command mode
>> frame transfer, or is it a feature/issue with OMAP DSI driver, or a
>> feature/issue in the command mode panel used. So this probably needs to
>> be revisited later.
> 
> This is important information, could you capture it in a comment in the
> code ?

I think this is related to the following (from DSI spec):

"To enable PHY synchronization the host processor should  periodically end HS transmission and drive
the Data Lanes to the LP state. This transition should take place at least once per frame"

And in OMAP TRM:

"Special care must be taken in the case of the last line of the frame. The LPS transition is
required when the link is in HS mode for the whole frame."

I'm not 100% sure about that, but I'll change the commit description and add a comment to reflect
the above text.

 Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
index 7fee9cf8782d..746c2149fbbd 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -3884,35 +3884,19 @@  static int _dsi_update(struct dsi_data *dsi)
 	return 0;
 }
 
-static int _dsi_update_window(struct dsi_data *dsi, int channel,
-			      int x, int y, int w, int h)
-{
-	int x1 = x, x2 = (x + w - 1);
-	int y1 = y, y2 = (y + h - 1);
-	u8 payloadX[5] = { MIPI_DCS_SET_COLUMN_ADDRESS,
-			   x1 >> 8, x1 & 0xff, x2 >> 8, x2 & 0xff };
-	u8 payloadY[5] = { MIPI_DCS_SET_PAGE_ADDRESS,
-			   y1 >> 8, y1 & 0xff, y2 >> 8, y2 & 0xff };
-	struct mipi_dsi_msg msgX = { 0 }, msgY = { 0 };
-	int ret;
+static int _dsi_send_nop(struct dsi_data *dsi, int channel)
+{
+	const u8 payload[] = { MIPI_DCS_NOP };
+	const struct mipi_dsi_msg msg = {
+		.channel = channel,
+		.type = MIPI_DSI_DCS_SHORT_WRITE,
+		.tx_len = 1,
+		.tx_buf = payload,
+	};
 
 	WARN_ON(!dsi_bus_is_locked(dsi));
 
-	msgX.type = MIPI_DSI_DCS_LONG_WRITE;
-	msgX.channel = channel;
-	msgX.tx_buf = payloadX;
-	msgX.tx_len = sizeof(payloadX);
-
-	msgY.type = MIPI_DSI_DCS_LONG_WRITE;
-	msgY.channel = channel;
-	msgY.tx_buf = payloadY;
-	msgY.tx_len = sizeof(payloadY);
-
-	ret = _omap_dsi_host_transfer(dsi, &msgX);
-	if (ret != 0)
-		return ret;
-
-	return _omap_dsi_host_transfer(dsi, &msgY);
+	return _omap_dsi_host_transfer(dsi, &msg);
 }
 
 static int dsi_update_channel(struct omap_dss_device *dssdev, int channel)
@@ -3944,10 +3928,9 @@  static int dsi_update_channel(struct omap_dss_device *dssdev, int channel)
 
 	dsi_set_ulps_auto(dsi, false);
 
-	r = _dsi_update_window(dsi, channel, 0, 0, dsi->vm.hactive,
-			       dsi->vm.vactive);
+	r = _dsi_send_nop(dsi, channel);
 	if (r < 0) {
-		DSSWARN("window update error: %d\n", r);
+		DSSWARN("failed to send nop between frames: %d\n", r);
 		goto err;
 	}