Re: [Intel-gfx] [PATCH v5] drm/i915/dsi: add payload receiving code
Great. Thanks Jani for the comments.
I will update the patch based on your comments and submit a new version again.
-Original Message-
From: Jani Nikula
Sent: Tuesday, June 21, 2022 6:09 PM
To: Tseng, William ; intel-gfx@lists.freedesktop.org
Cc: Tseng, William ; Ville Syrjälä
; Kulkarni, Vandita
; Lee, Shawn C
Subject: Re: [PATCH v5] drm/i915/dsi: add payload receiving code
On Mon, 20 Jun 2022, William Tseng wrote:
> To support Host to read data from Peripheral after a DCS read command
> is sent over DSI.
So the spec isn't all that clear on all the small details here. Since this
pretty much doesn't interfere with other code, I'll put more weight on test
results. If it works, great. If not, needs more work.
Currently we don't have a device in CI that would use this; we need a Tested-by
from whoever has a device.
Detailed comments inline.
>
> v1: initial version.
> v2:
> - adding error message when failing to place BTA.
> - returning byte number instead of 0 for the read function
> dsi_read_pkt_payld().
> v3: fixing coding style warning.
> v4:
> - correcting the data type of returning data for the read function
> dsi_read_pkt_payld().
> v5: adding change history as part of commit messages.
>
> Cc: Jani Nikula
> Cc: Ville Syrjälä
> Cc: Vandita Kulkarni
> Cc: Lee Shawn C
> Signed-off-by: William Tseng
> ---
> drivers/gpu/drm/i915/display/icl_dsi.c | 75 +++--
> drivers/gpu/drm/i915/display/icl_dsi_regs.h | 13
> 2 files changed, 83 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> b/drivers/gpu/drm/i915/display/icl_dsi.c
> index 19bf717fd4cb..b2aa3c7902f3 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -201,6 +201,69 @@ static int dsi_send_pkt_hdr(struct intel_dsi_host *host,
> return 0;
> }
>
> +static int dsi_read_pkt_payld(struct intel_dsi_host *host,
> + u8 *rx_buf, size_t rx_len)
> +{
> + struct intel_dsi *intel_dsi = host->intel_dsi;
> + struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
> + enum transcoder dsi_trans = dsi_port_to_transcoder(host->port);
> + u32 tmp, /*hdr_data, */payld_data;
Please drop the commented out stuff.
> + u32 payld_dw;
> + size_t payld_read;
> + u8 i;
Please use int for loop variables.
> +
> + /* step2: place a BTA reque */
> + /* check if header credit available */
> + if (!wait_for_header_credits(dev_priv, dsi_trans, 1)) {
> + drm_err(_priv->drm, "not ready to recive payload\n");
> + return -EBUSY;
> + }
> +
> + /* place BTA request */
> + tmp = intel_de_read(dev_priv, DSI_LP_MSG(dsi_trans));
> + tmp |= LINK_BTA;
> + intel_de_write(dev_priv, DSI_LP_MSG(dsi_trans), tmp);
Please use intel_de_rmw() for read-modify-write. Ditto below.
> +
> + tmp = intel_de_read(dev_priv, DSI_LP_MSG(dsi_trans));
Please use intel_de_posting_read() for posting reads. Ditto below.
> +
> + /* step2a: */
> + /* step2ai: set Turn-Around Timeout */
> + tmp = intel_de_read(dev_priv, DSI_TA_TO(dsi_trans));
> + tmp &= ~TA_TIMEOUT_VALUE_MASK;
> + tmp |= TA_TIMEOUT_VALUE(intel_dsi->turn_arnd_val);
> + intel_de_write(dev_priv, DSI_TA_TO(dsi_trans), tmp);
> +
> + tmp = intel_de_read(dev_priv, DSI_TA_TO(dsi_trans));
> +
> + /* step2aii: set maximum allowed time */
> + tmp = intel_de_read(dev_priv, DSI_LPRX_HOST_TO(dsi_trans));
> + tmp &= ~LPRX_TIMEOUT_VALUE_MASK;
> + tmp |= LPRX_TIMEOUT_VALUE(intel_dsi->lp_rx_timeout);
> + intel_de_write(dev_priv, DSI_LPRX_HOST_TO(dsi_trans), tmp);
> +
> + tmp = intel_de_read(dev_priv, DSI_LPRX_HOST_TO(dsi_trans));
Bspec 20597 says, "Prior to this SW should have set up the following", meaning
the above should happen before DSI_LP_MSG update.
I think the whole BTA stuff should be split out to a separate function, keeping
the actual payload receive very clean, similar to dsi_send_pkt_payld().
> +
> + /* step4a: wait and read payload */
> + if (wait_for_us(((intel_de_read(dev_priv, DSI_CMD_RXCTL(dsi_trans)) &
> + NUMBER_RX_PLOAD_DW_MASK) >> NUMBER_RX_PLOAD_DW_SHIFT) > 0,
> 10)) {
> + drm_err(_priv->drm, "DSI fails to receive payload\n");
> + return -EBUSY;
> + }
> +
> + tmp = intel_de_read(dev_priv, DSI_CMD_RXCTL(dsi_trans));
> + payld_dw = (tmp & NUMBER_RX_PLOAD_DW_MASK) >> NUMBER_RX_PLOAD_DW_SHIFT;
> + payld_read = min(rx_len, (size_t)(4 * payld_dw));
> +
> + for (i = 0; i < payld_read; i++) {
> + if ((i % 4) == 0)
> + payld_data = intel_de_read(dev_priv,
> DSI_CMD_RXPYLD(dsi_trans));
Might be prudent to explicitly clear the READ_UNLOADS_DW bit of DSI_CMD_RXCTL
beforehand.
> +
> + *(rx_buf + i) = (payld_data >> (8 * (i % 4))) & 0xff;
> + }
Please use similar loop as in dsi_send_pkt_payld(). It's
Re: [Intel-gfx] [PATCH v5] drm/i915/dsi: add payload receiving code
On Mon, 20 Jun 2022, William Tseng wrote:
> To support Host to read data from Peripheral after
> a DCS read command is sent over DSI.
So the spec isn't all that clear on all the small details here. Since
this pretty much doesn't interfere with other code, I'll put more weight
on test results. If it works, great. If not, needs more work.
Currently we don't have a device in CI that would use this; we need a
Tested-by from whoever has a device.
Detailed comments inline.
>
> v1: initial version.
> v2:
> - adding error message when failing to place BTA.
> - returning byte number instead of 0 for the read
> function dsi_read_pkt_payld().
> v3: fixing coding style warning.
> v4:
> - correcting the data type of returning data for
> the read function dsi_read_pkt_payld().
> v5: adding change history as part of commit messages.
>
> Cc: Jani Nikula
> Cc: Ville Syrjälä
> Cc: Vandita Kulkarni
> Cc: Lee Shawn C
> Signed-off-by: William Tseng
> ---
> drivers/gpu/drm/i915/display/icl_dsi.c | 75 +++--
> drivers/gpu/drm/i915/display/icl_dsi_regs.h | 13
> 2 files changed, 83 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
> b/drivers/gpu/drm/i915/display/icl_dsi.c
> index 19bf717fd4cb..b2aa3c7902f3 100644
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -201,6 +201,69 @@ static int dsi_send_pkt_hdr(struct intel_dsi_host *host,
> return 0;
> }
>
> +static int dsi_read_pkt_payld(struct intel_dsi_host *host,
> + u8 *rx_buf, size_t rx_len)
> +{
> + struct intel_dsi *intel_dsi = host->intel_dsi;
> + struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
> + enum transcoder dsi_trans = dsi_port_to_transcoder(host->port);
> + u32 tmp, /*hdr_data, */payld_data;
Please drop the commented out stuff.
> + u32 payld_dw;
> + size_t payld_read;
> + u8 i;
Please use int for loop variables.
> +
> + /* step2: place a BTA reque */
> + /* check if header credit available */
> + if (!wait_for_header_credits(dev_priv, dsi_trans, 1)) {
> + drm_err(_priv->drm, "not ready to recive payload\n");
> + return -EBUSY;
> + }
> +
> + /* place BTA request */
> + tmp = intel_de_read(dev_priv, DSI_LP_MSG(dsi_trans));
> + tmp |= LINK_BTA;
> + intel_de_write(dev_priv, DSI_LP_MSG(dsi_trans), tmp);
Please use intel_de_rmw() for read-modify-write. Ditto below.
> +
> + tmp = intel_de_read(dev_priv, DSI_LP_MSG(dsi_trans));
Please use intel_de_posting_read() for posting reads. Ditto below.
> +
> + /* step2a: */
> + /* step2ai: set Turn-Around Timeout */
> + tmp = intel_de_read(dev_priv, DSI_TA_TO(dsi_trans));
> + tmp &= ~TA_TIMEOUT_VALUE_MASK;
> + tmp |= TA_TIMEOUT_VALUE(intel_dsi->turn_arnd_val);
> + intel_de_write(dev_priv, DSI_TA_TO(dsi_trans), tmp);
> +
> + tmp = intel_de_read(dev_priv, DSI_TA_TO(dsi_trans));
> +
> + /* step2aii: set maximum allowed time */
> + tmp = intel_de_read(dev_priv, DSI_LPRX_HOST_TO(dsi_trans));
> + tmp &= ~LPRX_TIMEOUT_VALUE_MASK;
> + tmp |= LPRX_TIMEOUT_VALUE(intel_dsi->lp_rx_timeout);
> + intel_de_write(dev_priv, DSI_LPRX_HOST_TO(dsi_trans), tmp);
> +
> + tmp = intel_de_read(dev_priv, DSI_LPRX_HOST_TO(dsi_trans));
Bspec 20597 says, "Prior to this SW should have set up the following",
meaning the above should happen before DSI_LP_MSG update.
I think the whole BTA stuff should be split out to a separate function,
keeping the actual payload receive very clean, similar to
dsi_send_pkt_payld().
> +
> + /* step4a: wait and read payload */
> + if (wait_for_us(((intel_de_read(dev_priv, DSI_CMD_RXCTL(dsi_trans)) &
> + NUMBER_RX_PLOAD_DW_MASK) >> NUMBER_RX_PLOAD_DW_SHIFT) > 0,
> 10)) {
> + drm_err(_priv->drm, "DSI fails to receive payload\n");
> + return -EBUSY;
> + }
> +
> + tmp = intel_de_read(dev_priv, DSI_CMD_RXCTL(dsi_trans));
> + payld_dw = (tmp & NUMBER_RX_PLOAD_DW_MASK) >> NUMBER_RX_PLOAD_DW_SHIFT;
> + payld_read = min(rx_len, (size_t)(4 * payld_dw));
> +
> + for (i = 0; i < payld_read; i++) {
> + if ((i % 4) == 0)
> + payld_data = intel_de_read(dev_priv,
> DSI_CMD_RXPYLD(dsi_trans));
Might be prudent to explicitly clear the READ_UNLOADS_DW bit of
DSI_CMD_RXCTL beforehand.
> +
> + *(rx_buf + i) = (payld_data >> (8 * (i % 4))) & 0xff;
> + }
Please use similar loop as in dsi_send_pkt_payld(). It's confusing to
have one (i = 0; i < len; i += 4) handling bytes within, while the other
is (i = 0; i < payld_read; i++) handling dwords within. Same for using
(*rx_buf + i) instead of *data++.
> +
> + return (int)payld_read;
> +}
> +
> void icl_dsi_frame_update(struct intel_crtc_state *crtc_state)
> {
> struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
> @@ -1078,8 +1141,8 @@
[Intel-gfx] [PATCH v5] drm/i915/dsi: add payload receiving code
To support Host to read data from Peripheral after
a DCS read command is sent over DSI.
v1: initial version.
v2:
- adding error message when failing to place BTA.
- returning byte number instead of 0 for the read
function dsi_read_pkt_payld().
v3: fixing coding style warning.
v4:
- correcting the data type of returning data for
the read function dsi_read_pkt_payld().
v5: adding change history as part of commit messages.
Cc: Jani Nikula
Cc: Ville Syrjälä
Cc: Vandita Kulkarni
Cc: Lee Shawn C
Signed-off-by: William Tseng
---
drivers/gpu/drm/i915/display/icl_dsi.c | 75 +++--
drivers/gpu/drm/i915/display/icl_dsi_regs.h | 13
2 files changed, 83 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c
b/drivers/gpu/drm/i915/display/icl_dsi.c
index 19bf717fd4cb..b2aa3c7902f3 100644
--- a/drivers/gpu/drm/i915/display/icl_dsi.c
+++ b/drivers/gpu/drm/i915/display/icl_dsi.c
@@ -201,6 +201,69 @@ static int dsi_send_pkt_hdr(struct intel_dsi_host *host,
return 0;
}
+static int dsi_read_pkt_payld(struct intel_dsi_host *host,
+ u8 *rx_buf, size_t rx_len)
+{
+ struct intel_dsi *intel_dsi = host->intel_dsi;
+ struct drm_i915_private *dev_priv = to_i915(intel_dsi->base.base.dev);
+ enum transcoder dsi_trans = dsi_port_to_transcoder(host->port);
+ u32 tmp, /*hdr_data, */payld_data;
+ u32 payld_dw;
+ size_t payld_read;
+ u8 i;
+
+ /* step2: place a BTA reque */
+ /* check if header credit available */
+ if (!wait_for_header_credits(dev_priv, dsi_trans, 1)) {
+ drm_err(_priv->drm, "not ready to recive payload\n");
+ return -EBUSY;
+ }
+
+ /* place BTA request */
+ tmp = intel_de_read(dev_priv, DSI_LP_MSG(dsi_trans));
+ tmp |= LINK_BTA;
+ intel_de_write(dev_priv, DSI_LP_MSG(dsi_trans), tmp);
+
+ tmp = intel_de_read(dev_priv, DSI_LP_MSG(dsi_trans));
+
+ /* step2a: */
+ /* step2ai: set Turn-Around Timeout */
+ tmp = intel_de_read(dev_priv, DSI_TA_TO(dsi_trans));
+ tmp &= ~TA_TIMEOUT_VALUE_MASK;
+ tmp |= TA_TIMEOUT_VALUE(intel_dsi->turn_arnd_val);
+ intel_de_write(dev_priv, DSI_TA_TO(dsi_trans), tmp);
+
+ tmp = intel_de_read(dev_priv, DSI_TA_TO(dsi_trans));
+
+ /* step2aii: set maximum allowed time */
+ tmp = intel_de_read(dev_priv, DSI_LPRX_HOST_TO(dsi_trans));
+ tmp &= ~LPRX_TIMEOUT_VALUE_MASK;
+ tmp |= LPRX_TIMEOUT_VALUE(intel_dsi->lp_rx_timeout);
+ intel_de_write(dev_priv, DSI_LPRX_HOST_TO(dsi_trans), tmp);
+
+ tmp = intel_de_read(dev_priv, DSI_LPRX_HOST_TO(dsi_trans));
+
+ /* step4a: wait and read payload */
+ if (wait_for_us(((intel_de_read(dev_priv, DSI_CMD_RXCTL(dsi_trans)) &
+ NUMBER_RX_PLOAD_DW_MASK) >> NUMBER_RX_PLOAD_DW_SHIFT) > 0,
10)) {
+ drm_err(_priv->drm, "DSI fails to receive payload\n");
+ return -EBUSY;
+ }
+
+ tmp = intel_de_read(dev_priv, DSI_CMD_RXCTL(dsi_trans));
+ payld_dw = (tmp & NUMBER_RX_PLOAD_DW_MASK) >> NUMBER_RX_PLOAD_DW_SHIFT;
+ payld_read = min(rx_len, (size_t)(4 * payld_dw));
+
+ for (i = 0; i < payld_read; i++) {
+ if ((i % 4) == 0)
+ payld_data = intel_de_read(dev_priv,
DSI_CMD_RXPYLD(dsi_trans));
+
+ *(rx_buf + i) = (payld_data >> (8 * (i % 4))) & 0xff;
+ }
+
+ return (int)payld_read;
+}
+
void icl_dsi_frame_update(struct intel_crtc_state *crtc_state)
{
struct intel_crtc *crtc = to_intel_crtc(crtc_state->uapi.crtc);
@@ -1078,8 +1141,8 @@ static void gen11_dsi_setup_timeouts(struct intel_encoder
*encoder,
mul = 8 * 100;
hs_tx_timeout = DIV_ROUND_UP(intel_dsi->hs_tx_timeout * mul,
divisor);
- lp_rx_timeout = DIV_ROUND_UP(intel_dsi->lp_rx_timeout * mul, divisor);
- ta_timeout = DIV_ROUND_UP(intel_dsi->turn_arnd_val * mul, divisor);
+ lp_rx_timeout = intel_dsi->lp_rx_timeout;
+ ta_timeout = intel_dsi->turn_arnd_val;
for_each_dsi_port(port, intel_dsi->ports) {
dsi_trans = dsi_port_to_transcoder(port);
@@ -1837,9 +1900,11 @@ static ssize_t gen11_dsi_host_transfer(struct
mipi_dsi_host *host,
if (ret < 0)
return ret;
- //TODO: add payload receive code if needed
-
- ret = sizeof(dsi_pkt.header) + dsi_pkt.payload_length;
+ /* add payload receive code if needed */
+ if (msg->rx_buf && msg->rx_len > 0)
+ ret = dsi_read_pkt_payld(intel_dsi_host, msg->rx_buf,
msg->rx_len);
+ else
+ ret = sizeof(dsi_pkt.header) + dsi_pkt.payload_length;
return ret;
}
diff --git a/drivers/gpu/drm/i915/display/icl_dsi_regs.h
b/drivers/gpu/drm/i915/display/icl_dsi_regs.h
index f78f28b8dd94..a77a49b42d60 100644
--- a/drivers/gpu/drm/i915/display/icl_dsi_regs.h
+++
