| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <72b9c34357a443df8cbb55c6eddd8a21@quicinc.com>
Date: Wed, 1 Mar 2023 08:13:30 +0000
From: "Sankeerth Billakanti (QUIC)" <quic_sbillaka@...cinc.com>
To: "dmitry.baryshkov@...aro.org" <dmitry.baryshkov@...aro.org>
CC: "dri-devel@...ts.freedesktop.org" <dri-devel@...ts.freedesktop.org>,
"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
"freedreno@...ts.freedesktop.org" <freedreno@...ts.freedesktop.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"robdclark@...il.com" <robdclark@...il.com>,
"seanpaul@...omium.org" <seanpaul@...omium.org>,
"swboyd@...omium.org" <swboyd@...omium.org>,
"Kalyan Thota (QUIC)" <quic_kalyant@...cinc.com>,
"Abhinav Kumar (QUIC)" <quic_abhinavk@...cinc.com>,
"dianders@...omium.org" <dianders@...omium.org>,
"Kuogee Hsieh (QUIC)" <quic_khsieh@...cinc.com>,
"Bjorn Andersson (QUIC)" <quic_bjorande@...cinc.com>,
"sean@...rly.run" <sean@...rly.run>,
"airlied@...ux.ie" <airlied@...ux.ie>,
"daniel@...ll.ch" <daniel@...ll.ch>,
"Vishnuvardhan Prodduturi (QUIC)" <quic_vproddut@...cinc.com>
Subject: RE: [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver
probe
>>
>> 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@...cinc.com>
>> ---
>> 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.
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.
/**
* 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.
>> +
>> + 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
Powered by blists - more mailing lists