On Tue, 14 Mar 2023 at 12:23, Sankeerth Billakanti (QUIC) <quic_sbillaka@xxxxxxxxxxx> wrote: > > Hi Dmitry, > > >>>> > >>>> 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. > >>> > >> > >> Okay, I will move to the msm-next tip. > >> > >>>> > >>>> 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? > >>> > >> > >> The mdss interrupts will be enabled and can be used after msm_irq_install > >from msm_drm_bind. > >> We want to perform aux transactions during probe. The aux data > >> transmission is followed by an interrupt to indicate successful transmission > >status and reply information. > > > >This is the price of developing on, let me guess, 5.15. Nowadays MDSS > >interrupts are enabled and can be used during dp_display_probe() and > >dp_display_bind(). If they can not for some reason, this is an issue that must > >be fixed. Please reconsider your approach after rebasing onto msm-next or > >6.3-rc1. > > > > On the msm-next also, I see the mdss irq is enabled from bind (in msm_drv.c). > msm_drm_bind -> msm_drm_init -> msm_irq_install MDSS IRQ domain is registered from mdss_probe() -> msm_mdss_init() -> _msm_mdss_irq_domain_add(). Please note the difference between MDSS IRQs (handled by MDSS) vs MDP/DPU irq (registered by msm_irq_install()). > Currently, interrupts will not be available during the dp_display_probe. I can not agree with you here. > Is there a patch series which is enabling IRQs during mdss probe? https://patchwork.freedesktop.org/series/98525/, merged for 5.19. > > >> > >> As interrupts are not enabled, I took this polling approach for aux interrupts > >during probe. > >> > >>>> { > >>>> 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. > >>> > >> > >> The drm_dev registration happens during the bind. But we need to use aux > >during probe. > >> > >> The kernel doc for the drm_dp_aux_init function suggested we can use > >> the adapter before drm_dev registration. So, I used this function to enable > >the aux usage during probe. > > > >Then please also change __drm_printk() to use (drm) ? (drm->dev) : NULL as > >a first argument to dev_##level##type. Otherwise the first AUX transfer error > >before aux->drm_dev is set will oops the kernel. See how > >drm_err() is expanded. > > > > Okay, will add this also. Good. > > >> > >> /** > >> * drm_dp_aux_init() - minimally initialise an aux channel > >> * @aux: DisplayPort AUX channel > >> * > >> * If you need to use the drm_dp_aux's i2c adapter prior to registering it > >with > >> * the outside world, call drm_dp_aux_init() first. > >> > >>>> + > >>>> 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. > >>> > >> > >> I removed the resource enabling part from probe in the next patch where I > >implemented pm_runtime_ops. > >> I moved the host_init, phy_init and hpd_config into runtime_resume and > >calling pm_runtime_get_sync from aux_transfer. > > > >Next patch doesn't exist at this point. So, please, either reorder them, > >or make this patch correct per se. > > > >Also, the key part is a call to component_add(). It should come from > >done_probing callback. AUX bus probing is asynchronous. The kernel > >registers a device and then it might take some time for the correct > >driver to be loaded, etc. We clearly expect dp_display_bind to be used > >only when the panel has been probed. > > > > > Okay, will add the done_probe function Thank you. > > >> > >>>> + > >>>> + 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 > > > >-- > >With best wishes > >Dmitry > -- With best wishes Dmitry
