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 Currently, interrupts will not be available during the dp_display_probe. Is there a patch series which is enabling IRQs during mdss probe? >> >> 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. >> >> /** >> * 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 >> >>>> + >>>> + 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