diff mbox series

[v3,09/56] drm/omap: dsi: drop virtual channel logic

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

Commit Message

Tomi Valkeinen Nov. 5, 2020, 12:02 p.m. UTC 
From: Sebastian Reichel <sebastian.reichel@collabora.com>

This drops the virtual channel logic. Afterwards DSI clients
request their channel number and get the virtual channel with
the same number or -EBUSY if already in use.

Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 .../gpu/drm/omapdrm/displays/panel-dsi-cm.c   | 11 ++---
 drivers/gpu/drm/omapdrm/dss/dsi.c             | 46 ++++---------------
 drivers/gpu/drm/omapdrm/dss/omapdss.h         |  4 +-
 3 files changed, 12 insertions(+), 49 deletions(-)

Comments

Laurent Pinchart Nov. 9, 2020, 8:14 a.m. UTC | #1 
Hi Tomi and Sebastian,

Thank you for the patch.

On Thu, Nov 05, 2020 at 02:02:46PM +0200, Tomi Valkeinen wrote:
> From: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> This drops the virtual channel logic. Afterwards DSI clients
> request their channel number and get the virtual channel with
> the same number or -EBUSY if already in use.

It could be useful to explain the reason for this patch (which, if I
understand the patch correctly, is to replace dynamic allocation of
virtual channels with a fixed allocation by the DSI panel, I assume
because dynamic allocation isn't actually useful ?).

> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  .../gpu/drm/omapdrm/displays/panel-dsi-cm.c   | 11 ++---
>  drivers/gpu/drm/omapdrm/dss/dsi.c             | 46 ++++---------------
>  drivers/gpu/drm/omapdrm/dss/omapdss.h         |  4 +-
>  3 files changed, 12 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> index 8b2e80129bd8..ea76a89d1833 100644
> --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
> @@ -768,19 +768,12 @@ static int dsicm_connect(struct omap_dss_device *src,
>  	struct device *dev = &ddata->pdev->dev;
>  	int r;
>  
> -	r = src->ops->dsi.request_vc(src, &ddata->channel);
> +	r = src->ops->dsi.request_vc(src, ddata->channel);
>  	if (r) {
>  		dev_err(dev, "failed to get virtual channel\n");
>  		return r;
>  	}
>  
> -	r = src->ops->dsi.set_vc_id(src, ddata->channel, TCH);
> -	if (r) {
> -		dev_err(dev, "failed to set VC_ID\n");
> -		src->ops->dsi.release_vc(src, ddata->channel);
> -		return r;
> -	}
> -
>  	ddata->src = src;
>  	return 0;
>  }
> @@ -1215,6 +1208,8 @@ static int dsicm_probe_of(struct platform_device *pdev)
>  	struct display_timing timing;
>  	int err;
>  
> +	ddata->channel = TCH;

I'd drop the macro, but no big deal.

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

> +
>  	ddata->reset_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
>  	if (IS_ERR(ddata->reset_gpio)) {
>  		err = PTR_ERR(ddata->reset_gpio);
> diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
> index 2270730b16db..348be32505a2 100644
> --- a/drivers/gpu/drm/omapdrm/dss/dsi.c
> +++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
> @@ -350,7 +350,6 @@ struct dsi_data {
>  		struct omap_dss_device *dssdev;
>  		enum fifo_size tx_fifo_size;
>  		enum fifo_size rx_fifo_size;
> -		int vc_id;
>  	} vc[4];
>  
>  	struct mutex lock;
> @@ -2579,7 +2578,7 @@ static inline void dsi_vc_write_long_header(struct dsi_data *dsi, int channel,
>  
>  	WARN_ON(!dsi_bus_is_locked(dsi));
>  
> -	data_id = data_type | dsi->vc[channel].vc_id << 6;
> +	data_id = data_type | channel << 6;
>  
>  	val = FLD_VAL(data_id, 7, 0) | FLD_VAL(len, 23, 8) |
>  		FLD_VAL(ecc, 31, 24);
> @@ -2683,7 +2682,7 @@ static int dsi_vc_send_short(struct dsi_data *dsi, int channel, u8 data_type,
>  		return -EINVAL;
>  	}
>  
> -	data_id = data_type | dsi->vc[channel].vc_id << 6;
> +	data_id = data_type | channel << 6;
>  
>  	r = (data_id << 0) | (data << 8) | (ecc << 24);
>  
> @@ -4789,45 +4788,19 @@ static enum omap_channel dsi_get_channel(struct dsi_data *dsi)
>  	}
>  }
>  
> -static int dsi_request_vc(struct omap_dss_device *dssdev, int *channel)
> +static int dsi_request_vc(struct omap_dss_device *dssdev, int channel)
>  {
>  	struct dsi_data *dsi = to_dsi_data(dssdev);
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(dsi->vc); i++) {
> -		if (!dsi->vc[i].dssdev) {
> -			dsi->vc[i].dssdev = dssdev;
> -			*channel = i;
> -			return 0;
> -		}
> -	}
>  
> -	DSSERR("cannot get VC for display %s", dssdev->name);
> -	return -ENOSPC;
> -}
> -
> -static int dsi_set_vc_id(struct omap_dss_device *dssdev, int channel, int vc_id)
> -{
> -	struct dsi_data *dsi = to_dsi_data(dssdev);
> -
> -	if (vc_id < 0 || vc_id > 3) {
> -		DSSERR("VC ID out of range\n");
> -		return -EINVAL;
> -	}
> -
> -	if (channel < 0 || channel > 3) {
> -		DSSERR("Virtual Channel out of range\n");
> +	if (channel < 0 || channel > 3)
>  		return -EINVAL;
> -	}
>  
> -	if (dsi->vc[channel].dssdev != dssdev) {
> -		DSSERR("Virtual Channel not allocated to display %s\n",
> -			dssdev->name);
> -		return -EINVAL;
> +	if (dsi->vc[channel].dssdev) {
> +		DSSERR("cannot get VC for display %s", dssdev->name);
> +		return -EBUSY;
>  	}
>  
> -	dsi->vc[channel].vc_id = vc_id;
> -
> +	dsi->vc[channel].dssdev = dssdev;
>  	return 0;
>  }
>  
> @@ -4838,7 +4811,6 @@ static void dsi_release_vc(struct omap_dss_device *dssdev, int channel)
>  	if ((channel >= 0 && channel <= 3) &&
>  		dsi->vc[channel].dssdev == dssdev) {
>  		dsi->vc[channel].dssdev = NULL;
> -		dsi->vc[channel].vc_id = 0;
>  	}
>  }
>  
> @@ -4945,7 +4917,6 @@ static const struct omap_dss_device_ops dsi_ops = {
>  		.enable_te = dsi_enable_te,
>  
>  		.request_vc = dsi_request_vc,
> -		.set_vc_id = dsi_set_vc_id,
>  		.release_vc = dsi_release_vc,
>  
>  		.transfer = omap_dsi_transfer,
> @@ -5401,7 +5372,6 @@ static int dsi_probe(struct platform_device *pdev)
>  	for (i = 0; i < ARRAY_SIZE(dsi->vc); i++) {
>  		dsi->vc[i].source = DSI_VC_SOURCE_L4;
>  		dsi->vc[i].dssdev = NULL;
> -		dsi->vc[i].vc_id = 0;
>  	}
>  
>  	r = dsi_get_clocks(dsi);
> diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> index 9ead877cb24c..40e1ce9e256e 100644
> --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
> +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
> @@ -302,9 +302,7 @@ struct omapdss_dsi_ops {
>  	void (*disable_video_output)(struct omap_dss_device *dssdev,
>  			int channel);
>  
> -	int (*request_vc)(struct omap_dss_device *dssdev, int *channel);
> -	int (*set_vc_id)(struct omap_dss_device *dssdev, int channel,
> -			int vc_id);
> +	int (*request_vc)(struct omap_dss_device *dssdev, int channel);
>  	void (*release_vc)(struct omap_dss_device *dssdev, int channel);
>  
>  	/* data transfer */
Tomi Valkeinen Nov. 9, 2020, 8:18 a.m. UTC | #2 
On 05/11/2020 14:02, Tomi Valkeinen wrote:
> From: Sebastian Reichel <sebastian.reichel@collabora.com>
> 
> This drops the virtual channel logic. Afterwards DSI clients
> request their channel number and get the virtual channel with
> the same number or -EBUSY if already in use.
> 
> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>  .../gpu/drm/omapdrm/displays/panel-dsi-cm.c   | 11 ++---
>  drivers/gpu/drm/omapdrm/dss/dsi.c             | 46 ++++---------------
>  drivers/gpu/drm/omapdrm/dss/omapdss.h         |  4 +-
>  3 files changed, 12 insertions(+), 49 deletions(-)

A comment about this (and other virtual channel logic changes): When Sebastian sent the series in
Feb, I commented that the VC change is not correct. That's still true, and I did not fix it as I'm
not sure how to fix it, and however it is fixed has to be fully inside dsi.c (so it doesn't affect
e.g. the panel drivers).

However, it works just fine. The case where things will break is when we need to use multiple VCs
(OMAP DSI's "configuration blocks", not virtual channel ID numbers). Command mode panels don't need
this. Video mode panels might need it, but it's not quite clear to me if it's strictly needed, or is
it just an optimization.

I hope to get a board with video mode panel soonish, or help Nikolaus to debug this.

 Tomi
Tomi Valkeinen Nov. 9, 2020, 8:20 a.m. UTC | #3 
On 09/11/2020 10:14, Laurent Pinchart wrote:
> Hi Tomi and Sebastian,
> 
> Thank you for the patch.
> 
> On Thu, Nov 05, 2020 at 02:02:46PM +0200, Tomi Valkeinen wrote:
>> From: Sebastian Reichel <sebastian.reichel@collabora.com>
>>
>> This drops the virtual channel logic. Afterwards DSI clients
>> request their channel number and get the virtual channel with
>> the same number or -EBUSY if already in use.
> 
> It could be useful to explain the reason for this patch (which, if I
> understand the patch correctly, is to replace dynamic allocation of
> virtual channels with a fixed allocation by the DSI panel, I assume
> because dynamic allocation isn't actually useful ?).

Yes, I can elaborate on that in v4. I also just sent a separate mail about it.

>> Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.com>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
>> ---
>>  .../gpu/drm/omapdrm/displays/panel-dsi-cm.c   | 11 ++---
>>  drivers/gpu/drm/omapdrm/dss/dsi.c             | 46 ++++---------------
>>  drivers/gpu/drm/omapdrm/dss/omapdss.h         |  4 +-
>>  3 files changed, 12 insertions(+), 49 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
>> index 8b2e80129bd8..ea76a89d1833 100644
>> --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
>> +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
>> @@ -768,19 +768,12 @@ static int dsicm_connect(struct omap_dss_device *src,
>>  	struct device *dev = &ddata->pdev->dev;
>>  	int r;
>>  
>> -	r = src->ops->dsi.request_vc(src, &ddata->channel);
>> +	r = src->ops->dsi.request_vc(src, ddata->channel);
>>  	if (r) {
>>  		dev_err(dev, "failed to get virtual channel\n");
>>  		return r;
>>  	}
>>  
>> -	r = src->ops->dsi.set_vc_id(src, ddata->channel, TCH);
>> -	if (r) {
>> -		dev_err(dev, "failed to set VC_ID\n");
>> -		src->ops->dsi.release_vc(src, ddata->channel);
>> -		return r;
>> -	}
>> -
>>  	ddata->src = src;
>>  	return 0;
>>  }
>> @@ -1215,6 +1208,8 @@ static int dsicm_probe_of(struct platform_device *pdev)
>>  	struct display_timing timing;
>>  	int err;
>>  
>> +	ddata->channel = TCH;
> 
> I'd drop the macro, but no big deal.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

It's dropped in a later patch.

 Tomi
diff mbox series

Patch

diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
index 8b2e80129bd8..ea76a89d1833 100644
--- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
+++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c
@@ -768,19 +768,12 @@  static int dsicm_connect(struct omap_dss_device *src,
 	struct device *dev = &ddata->pdev->dev;
 	int r;
 
-	r = src->ops->dsi.request_vc(src, &ddata->channel);
+	r = src->ops->dsi.request_vc(src, ddata->channel);
 	if (r) {
 		dev_err(dev, "failed to get virtual channel\n");
 		return r;
 	}
 
-	r = src->ops->dsi.set_vc_id(src, ddata->channel, TCH);
-	if (r) {
-		dev_err(dev, "failed to set VC_ID\n");
-		src->ops->dsi.release_vc(src, ddata->channel);
-		return r;
-	}
-
 	ddata->src = src;
 	return 0;
 }
@@ -1215,6 +1208,8 @@  static int dsicm_probe_of(struct platform_device *pdev)
 	struct display_timing timing;
 	int err;
 
+	ddata->channel = TCH;
+
 	ddata->reset_gpio = devm_gpiod_get(&pdev->dev, "reset", GPIOD_OUT_LOW);
 	if (IS_ERR(ddata->reset_gpio)) {
 		err = PTR_ERR(ddata->reset_gpio);
diff --git a/drivers/gpu/drm/omapdrm/dss/dsi.c b/drivers/gpu/drm/omapdrm/dss/dsi.c
index 2270730b16db..348be32505a2 100644
--- a/drivers/gpu/drm/omapdrm/dss/dsi.c
+++ b/drivers/gpu/drm/omapdrm/dss/dsi.c
@@ -350,7 +350,6 @@  struct dsi_data {
 		struct omap_dss_device *dssdev;
 		enum fifo_size tx_fifo_size;
 		enum fifo_size rx_fifo_size;
-		int vc_id;
 	} vc[4];
 
 	struct mutex lock;
@@ -2579,7 +2578,7 @@  static inline void dsi_vc_write_long_header(struct dsi_data *dsi, int channel,
 
 	WARN_ON(!dsi_bus_is_locked(dsi));
 
-	data_id = data_type | dsi->vc[channel].vc_id << 6;
+	data_id = data_type | channel << 6;
 
 	val = FLD_VAL(data_id, 7, 0) | FLD_VAL(len, 23, 8) |
 		FLD_VAL(ecc, 31, 24);
@@ -2683,7 +2682,7 @@  static int dsi_vc_send_short(struct dsi_data *dsi, int channel, u8 data_type,
 		return -EINVAL;
 	}
 
-	data_id = data_type | dsi->vc[channel].vc_id << 6;
+	data_id = data_type | channel << 6;
 
 	r = (data_id << 0) | (data << 8) | (ecc << 24);
 
@@ -4789,45 +4788,19 @@  static enum omap_channel dsi_get_channel(struct dsi_data *dsi)
 	}
 }
 
-static int dsi_request_vc(struct omap_dss_device *dssdev, int *channel)
+static int dsi_request_vc(struct omap_dss_device *dssdev, int channel)
 {
 	struct dsi_data *dsi = to_dsi_data(dssdev);
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(dsi->vc); i++) {
-		if (!dsi->vc[i].dssdev) {
-			dsi->vc[i].dssdev = dssdev;
-			*channel = i;
-			return 0;
-		}
-	}
 
-	DSSERR("cannot get VC for display %s", dssdev->name);
-	return -ENOSPC;
-}
-
-static int dsi_set_vc_id(struct omap_dss_device *dssdev, int channel, int vc_id)
-{
-	struct dsi_data *dsi = to_dsi_data(dssdev);
-
-	if (vc_id < 0 || vc_id > 3) {
-		DSSERR("VC ID out of range\n");
-		return -EINVAL;
-	}
-
-	if (channel < 0 || channel > 3) {
-		DSSERR("Virtual Channel out of range\n");
+	if (channel < 0 || channel > 3)
 		return -EINVAL;
-	}
 
-	if (dsi->vc[channel].dssdev != dssdev) {
-		DSSERR("Virtual Channel not allocated to display %s\n",
-			dssdev->name);
-		return -EINVAL;
+	if (dsi->vc[channel].dssdev) {
+		DSSERR("cannot get VC for display %s", dssdev->name);
+		return -EBUSY;
 	}
 
-	dsi->vc[channel].vc_id = vc_id;
-
+	dsi->vc[channel].dssdev = dssdev;
 	return 0;
 }
 
@@ -4838,7 +4811,6 @@  static void dsi_release_vc(struct omap_dss_device *dssdev, int channel)
 	if ((channel >= 0 && channel <= 3) &&
 		dsi->vc[channel].dssdev == dssdev) {
 		dsi->vc[channel].dssdev = NULL;
-		dsi->vc[channel].vc_id = 0;
 	}
 }
 
@@ -4945,7 +4917,6 @@  static const struct omap_dss_device_ops dsi_ops = {
 		.enable_te = dsi_enable_te,
 
 		.request_vc = dsi_request_vc,
-		.set_vc_id = dsi_set_vc_id,
 		.release_vc = dsi_release_vc,
 
 		.transfer = omap_dsi_transfer,
@@ -5401,7 +5372,6 @@  static int dsi_probe(struct platform_device *pdev)
 	for (i = 0; i < ARRAY_SIZE(dsi->vc); i++) {
 		dsi->vc[i].source = DSI_VC_SOURCE_L4;
 		dsi->vc[i].dssdev = NULL;
-		dsi->vc[i].vc_id = 0;
 	}
 
 	r = dsi_get_clocks(dsi);
diff --git a/drivers/gpu/drm/omapdrm/dss/omapdss.h b/drivers/gpu/drm/omapdrm/dss/omapdss.h
index 9ead877cb24c..40e1ce9e256e 100644
--- a/drivers/gpu/drm/omapdrm/dss/omapdss.h
+++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h
@@ -302,9 +302,7 @@  struct omapdss_dsi_ops {
 	void (*disable_video_output)(struct omap_dss_device *dssdev,
 			int channel);
 
-	int (*request_vc)(struct omap_dss_device *dssdev, int *channel);
-	int (*set_vc_id)(struct omap_dss_device *dssdev, int channel,
-			int vc_id);
+	int (*request_vc)(struct omap_dss_device *dssdev, int channel);
 	void (*release_vc)(struct omap_dss_device *dssdev, int channel);
 
 	/* data transfer */