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 <jani.nikula@xxxxxxxxxxxxxxx>
Sent: Tuesday, June 21, 2022 6:09 PM
To: Tseng, William <william.tseng@xxxxxxxxx>; intel-gfx@xxxxxxxxxxxxxxxxxxxxx
Cc: Tseng, William <william.tseng@xxxxxxxxx>; Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>; Kulkarni, Vandita <vandita.kulkarni@xxxxxxxxx>; Lee, Shawn C <shawn.c.lee@xxxxxxxxx>
Subject: Re: [PATCH v5] drm/i915/dsi: add payload receiving code
On Mon, 20 Jun 2022, William Tseng <william.tseng@xxxxxxxxx> 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 <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx>
> Cc: Vandita Kulkarni <vandita.kulkarni@xxxxxxxxx>
> Cc: Lee Shawn C <shawn.c.lee@xxxxxxxxx>
> Signed-off-by: William Tseng <william.tseng@xxxxxxxxx>
> ---
> 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(&dev_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, 100000)) {
> + drm_err(&dev_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 @@ static void gen11_dsi_setup_timeouts(struct intel_encoder *encoder,
> mul = 8 * 1000000;
> 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;
This is an unrelated change that needs to be a separate patch.
>
> 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 */
Just drop the comment altogether.
> + if (msg->rx_buf && msg->rx_len > 0)
It should probably be an error to have rx_len > 0 && !rx_buf.
> + 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
> +++ b/drivers/gpu/drm/i915/display/icl_dsi_regs.h
> @@ -251,6 +251,18 @@
> #define NUMBER_RX_PLOAD_DW_MASK (0xff << 0)
> #define NUMBER_RX_PLOAD_DW_SHIFT 0
>
> +#define _DSI_CMD_RXHDR_0 0x6b0e0
> +#define _DSI_CMD_RXHDR_1 0x6b8e0
> +#define DSI_CMD_RXHDR(tc) _MMIO_DSI(tc, \
> + _DSI_CMD_RXHDR_0,\
> + _DSI_CMD_RXHDR_1)
> +
> +#define _DSI_CMD_RXPYLD_0 0x6b0e4
> +#define _DSI_CMD_RXPYLD_1 0x6b8e4
> +#define DSI_CMD_RXPYLD(tc) _MMIO_DSI(tc, \
> + _DSI_CMD_RXPYLD_0,\
> + _DSI_CMD_RXPYLD_1)
> +
> #define _DSI_CMD_TXCTL_0 0x6b0d0
> #define _DSI_CMD_TXCTL_1 0x6b8d0
> #define DSI_CMD_TXCTL(tc) _MMIO_DSI(tc, \
> @@ -294,6 +306,7 @@
> #define LPTX_IN_PROGRESS (1 << 17)
> #define LINK_IN_ULPS (1 << 16)
> #define LINK_ULPS_TYPE_LP11 (1 << 8)
> +#define LINK_BTA (1 << 1)
> #define LINK_ENTER_ULPS (1 << 0)
>
> /* DSI timeout registers */
--
Jani Nikula, Intel Open Source Graphics Center
