| Message ID | 20200224232126.3385250-11-sebastian.reichel@collabora.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/omap: Convert DSI code to use drm_mipi_dsi and drm_panel | expand |
Hi Sebastian, Thank you for the patch. On Tue, Feb 25, 2020 at 12:20:40AM +0100, Sebastian Reichel wrote: > 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. I wonder why this level of indirection was used, allocating "virtual VCs". A single virtual indirection should be enough :-) I may be missing some context though, I'll defer that to Tomi, but for me, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > Signed-off-by: Sebastian Reichel <sebastian.reichel@collabora.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 92f510a771fe..ba046a596044 100644 > --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c > @@ -769,19 +769,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; > } > @@ -1216,6 +1209,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 0990777a42f7..8c223b808740 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); > > @@ -4783,45 +4782,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; > } > > @@ -4832,7 +4805,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; > } > } > > @@ -4937,7 +4909,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, > @@ -5393,7 +5364,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 787e102eb068..587206c984a9 100644 > --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h > +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h > @@ -301,9 +301,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 */
On 25/02/2020 17:01, Laurent Pinchart wrote: > Hi Sebastian, > > Thank you for the patch. > > On Tue, Feb 25, 2020 at 12:20:40AM +0100, Sebastian Reichel wrote: >> 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. > > I wonder why this level of indirection was used, allocating "virtual > VCs". A single virtual indirection should be enough :-) I may be missing > some context though, I'll defer that to Tomi, but for me, I haven't reviewed the code yet, and it's been a long time since I wrote this code. But maybe this explains at least some: (I hope I remember this right) DSI packets have virtual channel IDs (VCID). That's number 0-3 that needs to be in the packets. DSI IP has virtual channel "blocks" (VC), with associated registers. So 4 VC register blocks. These are not related to DSI virtual channel IDs in any way. To do DSI transactions, you choose a VC, and program it. A VC can send data via video pipeline, or transmit and receive data messages created with CPU. And in both cases, you need to include the VCID in the transmissions, of course. So, I think a normal use case could be a single panel, with VCID 0. To send video data and control messages, you would use VC0 and VC1. VC0 would be configured for video data, and VC1 would be configured for control messages. And if I recall right, currently you first request a free VC from the IP with request_vc(). Then you use set_vc_id(channel, id) to set the VCID, used when doing transactions with that VC. So the virtual channel naming is pretty confusing in the DSI IP, in my opinion. Tomi
Hi Tomi, On Wed, Apr 01, 2020 at 02:30:25PM +0300, Tomi Valkeinen wrote: > On 25/02/2020 17:01, Laurent Pinchart wrote: > > On Tue, Feb 25, 2020 at 12:20:40AM +0100, Sebastian Reichel wrote: > >> 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. > > > > I wonder why this level of indirection was used, allocating "virtual > > VCs". A single virtual indirection should be enough :-) I may be missing > > some context though, I'll defer that to Tomi, but for me, > > I haven't reviewed the code yet, and it's been a long time since I wrote this code. But maybe this > explains at least some: > > (I hope I remember this right) > > DSI packets have virtual channel IDs (VCID). That's number 0-3 that needs to be in the packets. > > DSI IP has virtual channel "blocks" (VC), with associated registers. So 4 VC register blocks. These > are not related to DSI virtual channel IDs in any way. > > To do DSI transactions, you choose a VC, and program it. A VC can send data via video pipeline, or > transmit and receive data messages created with CPU. And in both cases, you need to include the VCID > in the transmissions, of course. > > So, I think a normal use case could be a single panel, with VCID 0. To send video data and control > messages, you would use VC0 and VC1. VC0 would be configured for video data, and VC1 would be > configured for control messages. > > And if I recall right, currently you first request a free VC from the IP with request_vc(). Then you > use set_vc_id(channel, id) to set the VCID, used when doing transactions with that VC. > > So the virtual channel naming is pretty confusing in the DSI IP, in my opinion. I wasn't aware of those details, thank you for the explanation. It's quite confusing indeed, let's try to document the architecture in a comment block at the beginning of the dsi.c file for later reference.
On 01/04/2020 14:33, Laurent Pinchart wrote: > Hi Tomi, > > On Wed, Apr 01, 2020 at 02:30:25PM +0300, Tomi Valkeinen wrote: >> On 25/02/2020 17:01, Laurent Pinchart wrote: >>> On Tue, Feb 25, 2020 at 12:20:40AM +0100, Sebastian Reichel wrote: >>>> 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. >>> >>> I wonder why this level of indirection was used, allocating "virtual >>> VCs". A single virtual indirection should be enough :-) I may be missing >>> some context though, I'll defer that to Tomi, but for me, >> >> I haven't reviewed the code yet, and it's been a long time since I wrote this code. But maybe this >> explains at least some: >> >> (I hope I remember this right) >> >> DSI packets have virtual channel IDs (VCID). That's number 0-3 that needs to be in the packets. >> >> DSI IP has virtual channel "blocks" (VC), with associated registers. So 4 VC register blocks. These >> are not related to DSI virtual channel IDs in any way. >> >> To do DSI transactions, you choose a VC, and program it. A VC can send data via video pipeline, or >> transmit and receive data messages created with CPU. And in both cases, you need to include the VCID >> in the transmissions, of course. >> >> So, I think a normal use case could be a single panel, with VCID 0. To send video data and control >> messages, you would use VC0 and VC1. VC0 would be configured for video data, and VC1 would be >> configured for control messages. >> >> And if I recall right, currently you first request a free VC from the IP with request_vc(). Then you >> use set_vc_id(channel, id) to set the VCID, used when doing transactions with that VC. >> >> So the virtual channel naming is pretty confusing in the DSI IP, in my opinion. > > I wasn't aware of those details, thank you for the explanation. It's > quite confusing indeed, let's try to document the architecture in a > comment block at the beginning of the dsi.c file for later reference. But also, I think there's much room for cleanups and improvements. I don't think we have ever really supported multiple DSI peripherals, even in theory. So just one peripheral, with VCID always 0. Even if we need two VCs to manage that single peripheral (I think that's often the case, we want one VC for video, one for control), we could fully hide that detail into the driver. This won't work with more than 2 DSI peripherals, but I think we can just say the driver supports a single peripheral, and that's it. But with a quick browsing of this patch, I don't think it does it right, as it looks to me that the patch makes VCID == VC. Tomi
diff --git a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c index 92f510a771fe..ba046a596044 100644 --- a/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c +++ b/drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c @@ -769,19 +769,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; } @@ -1216,6 +1209,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 0990777a42f7..8c223b808740 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); @@ -4783,45 +4782,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; } @@ -4832,7 +4805,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; } } @@ -4937,7 +4909,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, @@ -5393,7 +5364,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 787e102eb068..587206c984a9 100644 --- a/drivers/gpu/drm/omapdrm/dss/omapdss.h +++ b/drivers/gpu/drm/omapdrm/dss/omapdss.h @@ -301,9 +301,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 */
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> --- .../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(-)