Re: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe
From: Dmitry Baryshkov
Date: Thu Feb 23 2023 - 09:59:33 EST
On Thu, 23 Feb 2023 at 15:57, Sankeerth Billakanti
<quic_sbillaka@xxxxxxxxxxx> wrote:
>
> The eDP panel is identified and enumerated during probe of the panel-edp
> driver. The current DP driver triggers this panel-edp driver probe while
> getting the panel-bridge associated with the eDP panel from the platform
> driver bind. If the panel-edp probe is deferred, then the whole bunch of
> MDSS parent and child drivers have to defer and roll back.
No, MDSS has not been rolled back since 5.19. Please shift your
development on top of the current msm-next.
>
> So, we want to move the panel enumeration from bind to probe so that the
> probe defer is easier to handle and also both the drivers appear consistent
> in panel enumeration. This change moves the DP driver panel-bridge
> enumeration to probe.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@xxxxxxxxxxx>
> ---
> drivers/gpu/drm/msm/dp/dp_aux.c | 149 ++++++++++++++++++++++++++--
> drivers/gpu/drm/msm/dp/dp_catalog.c | 12 +++
> drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
> drivers/gpu/drm/msm/dp/dp_display.c | 80 ++++++---------
> 4 files changed, 182 insertions(+), 60 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c b/drivers/gpu/drm/msm/dp/dp_aux.c
> index cc3efed593aa..5da95dfdeede 100644
> --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> @@ -110,7 +110,7 @@ static ssize_t dp_aux_write(struct dp_aux_private *aux,
> }
>
> static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> - struct drm_dp_aux_msg *msg)
> + struct drm_dp_aux_msg *msg, bool poll)
What is the reason for working in polled mode rather than always using
the interrupts?
> {
> ssize_t ret;
> unsigned long time_left;
> @@ -121,10 +121,16 @@ static ssize_t dp_aux_cmd_fifo_tx(struct dp_aux_private *aux,
> if (ret < 0)
> return ret;
>
> - time_left = wait_for_completion_timeout(&aux->comp,
> + if (!poll) {
> + time_left = wait_for_completion_timeout(&aux->comp,
> msecs_to_jiffies(250));
> - if (!time_left)
> - return -ETIMEDOUT;
> + if (!time_left)
> + return -ETIMEDOUT;
> + } else {
> + ret = dp_catalog_aux_poll_and_get_hw_interrupt(aux->catalog);
> + if (!ret)
> + dp_aux_isr(&aux->dp_aux);
> + }
>
> return ret;
> }
> @@ -238,7 +244,7 @@ static void dp_aux_update_offset_and_segment(struct dp_aux_private *aux,
> */
> static void dp_aux_transfer_helper(struct dp_aux_private *aux,
> struct drm_dp_aux_msg *input_msg,
> - bool send_seg)
> + bool send_seg, bool poll)
> {
> struct drm_dp_aux_msg helper_msg;
> u32 message_size = 0x10;
> @@ -278,7 +284,7 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
> helper_msg.address = segment_address;
> helper_msg.buffer = &aux->segment;
> helper_msg.size = 1;
> - dp_aux_cmd_fifo_tx(aux, &helper_msg);
> + dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
> }
>
> /*
> @@ -292,7 +298,7 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
> helper_msg.address = input_msg->address;
> helper_msg.buffer = &aux->offset;
> helper_msg.size = 1;
> - dp_aux_cmd_fifo_tx(aux, &helper_msg);
> + dp_aux_cmd_fifo_tx(aux, &helper_msg, poll);
>
> end:
> aux->offset += message_size;
> @@ -300,6 +306,122 @@ static void dp_aux_transfer_helper(struct dp_aux_private *aux,
> aux->segment = 0x0; /* reset segment at end of block */
> }
>
> +/*
> + * This function does the real job to process an AUX transaction.
> + * It will call aux_reset() function to reset the AUX channel,
> + * if the waiting is timeout.
> + */
> +static ssize_t dp_aux_transfer_init(struct drm_dp_aux *dp_aux,
> + struct drm_dp_aux_msg *msg)
> +{
> + ssize_t ret;
> + int const aux_cmd_native_max = 16;
> + int const aux_cmd_i2c_max = 128;
> + struct dp_aux_private *aux;
> +
> + aux = container_of(dp_aux, struct dp_aux_private, dp_aux);
> +
> + aux->native = msg->request & (DP_AUX_NATIVE_WRITE & DP_AUX_NATIVE_READ);
> +
> + /* Ignore address only message */
> + if (msg->size == 0 || !msg->buffer) {
> + msg->reply = aux->native ?
> + DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> + return msg->size;
> + }
> +
> + /* msg sanity check */
> + if ((aux->native && msg->size > aux_cmd_native_max) ||
> + msg->size > aux_cmd_i2c_max) {
> + DRM_ERROR("%s: invalid msg: size(%zu), request(%x)\n",
> + __func__, msg->size, msg->request);
> + return -EINVAL;
> + }
> +
> + mutex_lock(&aux->mutex);
> + if (!aux->initted) {
> + ret = -EIO;
> + goto exit;
> + }
> +
> + /*
> + * For eDP it's important to give a reasonably long wait here for HPD
> + * to be asserted. This is because the panel driver may have _just_
> + * turned on the panel and then tried to do an AUX transfer. The panel
> + * driver has no way of knowing when the panel is ready, so it's up
> + * to us to wait. For DP we never get into this situation so let's
> + * avoid ever doing the extra long wait for DP.
> + */
> + if (aux->is_edp) {
> + ret = dp_catalog_aux_wait_for_hpd_connect_state(aux->catalog);
> + if (ret) {
> + DRM_DEBUG_DP("Panel not ready for aux transactions\n");
> + goto exit;
> + }
> + }
> +
> + dp_aux_update_offset_and_segment(aux, msg);
> + dp_aux_transfer_helper(aux, msg, true, true);
> +
> + aux->read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
> + aux->cmd_busy = true;
> +
> + if (aux->read) {
> + aux->no_send_addr = true;
> + aux->no_send_stop = false;
> + } else {
> + aux->no_send_addr = true;
> + aux->no_send_stop = true;
> + }
> +
> + ret = dp_aux_cmd_fifo_tx(aux, msg, true);
> +
> + /* TODO: why is fifo_rx necessary here?
> + * Ideally fifo_rx need not be executed for an aux write
> + */
> + ret = dp_aux_cmd_fifo_rx(aux, msg);
> +
> + if (ret < 0) {
> + if (aux->native) {
> + aux->retry_cnt++;
> + if (!(aux->retry_cnt % MAX_AUX_RETRIES))
> + dp_catalog_aux_update_cfg(aux->catalog);
> + }
> + /* reset aux if link is in connected state */
> + if (dp_catalog_link_is_connected(aux->catalog))
> + dp_catalog_aux_reset(aux->catalog);
> + } else {
> + aux->retry_cnt = 0;
> + switch (aux->aux_error_num) {
> + case DP_AUX_ERR_NONE:
> + if (aux->read)
> + ret = dp_aux_cmd_fifo_rx(aux, msg);
> + msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_ACK : DP_AUX_I2C_REPLY_ACK;
> + break;
> + case DP_AUX_ERR_DEFER:
> + msg->reply = aux->native ?
> + DP_AUX_NATIVE_REPLY_DEFER : DP_AUX_I2C_REPLY_DEFER;
> + break;
> + case DP_AUX_ERR_PHY:
> + case DP_AUX_ERR_ADDR:
> + case DP_AUX_ERR_NACK:
> + case DP_AUX_ERR_NACK_DEFER:
> + msg->reply = aux->native ? DP_AUX_NATIVE_REPLY_NACK : DP_AUX_I2C_REPLY_NACK;
> + break;
> + case DP_AUX_ERR_TOUT:
> + ret = -ETIMEDOUT;
> + break;
> + }
> + }
> +
> + aux->cmd_busy = false;
> +
> +exit:
> + mutex_unlock(&aux->mutex);
> +
> + return ret;
> +}
> +
> /*
> * This function does the real job to process an AUX transaction.
> * It will call aux_reset() function to reset the AUX channel,
> @@ -355,7 +477,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> }
>
> dp_aux_update_offset_and_segment(aux, msg);
> - dp_aux_transfer_helper(aux, msg, true);
> + dp_aux_transfer_helper(aux, msg, true, false);
>
> aux->read = msg->request & (DP_AUX_I2C_READ & DP_AUX_NATIVE_READ);
> aux->cmd_busy = true;
> @@ -368,7 +490,7 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux *dp_aux,
> aux->no_send_stop = true;
> }
>
> - ret = dp_aux_cmd_fifo_tx(aux, msg);
> + ret = dp_aux_cmd_fifo_tx(aux, msg, false);
> if (ret < 0) {
> if (aux->native) {
> aux->retry_cnt++;
> @@ -535,6 +657,15 @@ struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog *catalog,
> aux->catalog = catalog;
> aux->retry_cnt = 0;
>
> + /*
> + * Use the drm_dp_aux_init() to use the aux adapter
> + * before registering aux with the DRM device.
> + */
> + aux->dp_aux.name = "dpu_dp_aux";
> + aux->dp_aux.dev = dev;
> + aux->dp_aux.transfer = dp_aux_transfer_init;
> + drm_dp_aux_init(&aux->dp_aux);
How do you use the aux adapter? It should not be used before
aux->drm_dev is setup.
> +
> return &aux->dp_aux;
> }
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c b/drivers/gpu/drm/msm/dp/dp_catalog.c
> index 676279d0ca8d..ccf0400176f0 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> @@ -258,6 +258,18 @@ int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog)
> 2000, 500000);
> }
>
> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog *dp_catalog)
> +{
> + u32 aux;
> + struct dp_catalog_private *catalog = container_of(dp_catalog,
> + struct dp_catalog_private, dp_catalog);
> +
> + return readl_poll_timeout(catalog->io->dp_controller.ahb.base +
> + REG_DP_INTR_STATUS,
> + aux, aux & DP_INTERRUPT_STATUS1,
> + 250, 250000);
> +}
> +
> static void dump_regs(void __iomem *base, int len)
> {
> int i;
> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.h b/drivers/gpu/drm/msm/dp/dp_catalog.h
> index 1f717f45c115..ad4a9e0f71f2 100644
> --- a/drivers/gpu/drm/msm/dp/dp_catalog.h
> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.h
> @@ -87,6 +87,7 @@ void dp_catalog_aux_enable(struct dp_catalog *dp_catalog, bool enable);
> void dp_catalog_aux_update_cfg(struct dp_catalog *dp_catalog);
> int dp_catalog_aux_wait_for_hpd_connect_state(struct dp_catalog *dp_catalog);
> u32 dp_catalog_aux_get_irq(struct dp_catalog *dp_catalog);
> +int dp_catalog_aux_poll_and_get_hw_interrupt(struct dp_catalog *dp_catalog);
>
> /* DP Controller APIs */
> void dp_catalog_ctrl_state_ctrl(struct dp_catalog *dp_catalog, u32 state);
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index bde1a7ce442f..a5dcef040b74 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -275,13 +275,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
> dp->dp_display.drm_dev = drm;
> priv->dp[dp->id] = &dp->dp_display;
>
> - rc = dp->parser->parse(dp->parser);
> - if (rc) {
> - DRM_ERROR("device tree parsing failed\n");
> - goto end;
> - }
> -
> -
> dp->drm_dev = drm;
> dp->aux->drm_dev = drm;
> rc = dp_aux_register(dp->aux);
> @@ -290,12 +283,6 @@ static int dp_display_bind(struct device *dev, struct device *master,
> goto end;
> }
>
> - rc = dp_power_client_init(dp->power);
> - if (rc) {
> - DRM_ERROR("Power client create failed\n");
> - goto end;
> - }
> -
> rc = dp_register_audio_driver(dev, dp->audio);
> if (rc) {
> DRM_ERROR("Audio registration Dp failed\n");
> @@ -787,6 +774,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
> goto error;
> }
>
> + rc = dp->parser->parse(dp->parser);
> + if (rc) {
> + DRM_ERROR("device tree parsing failed\n");
> + goto error;
> + }
> +
> dp->catalog = dp_catalog_get(dev, &dp->parser->io);
> if (IS_ERR(dp->catalog)) {
> rc = PTR_ERR(dp->catalog);
> @@ -803,6 +796,12 @@ static int dp_init_sub_modules(struct dp_display_private *dp)
> goto error;
> }
>
> + rc = dp_power_client_init(dp->power);
> + if (rc) {
> + DRM_ERROR("Power client create failed\n");
> + goto error;
> + }
> +
> dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
> if (IS_ERR(dp->aux)) {
> rc = PTR_ERR(dp->aux);
> @@ -1338,12 +1337,29 @@ static int dp_display_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, &dp->dp_display);
>
> + if (dp->dp_display.is_edp) {
> + dp_display_host_init(dp);
> + dp_display_host_phy_init(dp);
> + dp_catalog_ctrl_hpd_config(dp->catalog);
> +
> + rc = devm_of_dp_aux_populate_bus(dp->aux, NULL);
You should pass a real done_probing handler here, if you are going to
refactor this piece of code. The done_probing should then shut down
the resources and bind the component.
> +
> + dp_display_host_phy_exit(dp);
> + dp_display_host_deinit(dp);
> +
> + if (rc) {
> + DRM_ERROR("failed to initialize panel, rc = %d\n", rc);
> + goto error;
> + }
> + }
> +
> rc = component_add(&pdev->dev, &dp_display_comp_ops);
> if (rc) {
> DRM_ERROR("component add failed, rc=%d\n", rc);
> dp_display_deinit_sub_modules(dp);
> }
>
> +error:
> return rc;
> }
>
> @@ -1546,40 +1562,8 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
> {
> int rc;
> struct dp_display_private *dp_priv;
> - struct device_node *aux_bus;
> - struct device *dev;
>
> dp_priv = container_of(dp, struct dp_display_private, dp_display);
> - dev = &dp_priv->pdev->dev;
> - aux_bus = of_get_child_by_name(dev->of_node, "aux-bus");
> -
> - if (aux_bus && dp->is_edp) {
> - dp_display_host_init(dp_priv);
> - dp_catalog_ctrl_hpd_config(dp_priv->catalog);
> - dp_display_host_phy_init(dp_priv);
> - enable_irq(dp_priv->irq);
> -
> - /*
> - * The code below assumes that the panel will finish probing
> - * by the time devm_of_dp_aux_populate_ep_devices() returns.
> - * This isn't a great assumption since it will fail if the
> - * panel driver is probed asynchronously but is the best we
> - * can do without a bigger driver reorganization.
> - */
> - rc = of_dp_aux_populate_bus(dp_priv->aux, NULL);
> - of_node_put(aux_bus);
> - if (rc)
> - goto error;
> -
> - rc = devm_add_action_or_reset(dp->drm_dev->dev,
> - of_dp_aux_depopulate_bus_void,
> - dp_priv->aux);
> - if (rc)
> - goto error;
> - } else if (dp->is_edp) {
> - DRM_ERROR("eDP aux_bus not found\n");
> - return -ENODEV;
> - }
>
> /*
> * External bridges are mandatory for eDP interfaces: one has to
> @@ -1597,12 +1581,6 @@ static int dp_display_get_next_bridge(struct msm_dp *dp)
> return 0;
> }
>
> -error:
> - if (dp->is_edp) {
> - disable_irq(dp_priv->irq);
> - dp_display_host_phy_exit(dp_priv);
> - dp_display_host_deinit(dp_priv);
> - }
> return rc;
> }
>
> --
> 2.39.0
>
--
With best wishes
Dmitry