Re: [Freedreno] [RFC PATCH 1/2] drm/msm/dp: enumerate edp panel during driver probe
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
---
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(>comp,
+ if (!poll) {
+ time_left = wait_for_completion_timeout(>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(>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 = >segment;
helper_msg.size = 1;
- dp_aux_cmd_fifo_tx(aux, _msg);
+ dp_aux_cmd_fifo_tx(aux, _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 = >offset;
helper_msg.size = 1;
- dp_aux_cmd_fifo_tx(aux, _msg);
+ dp_aux_cmd_fifo_tx(aux, _msg, poll);
end:
aux->offset += message_size; @@ -300,6 +306,122 @@ static
void dp_aux_transfer_helper(struct
>>> dp_aux_private
Re: [Freedreno] [RFC PATCH 2/2] drm/msm/dp: enable pm_runtime support for dp driver
>> diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
>> b/drivers/gpu/drm/msm/dp/dp_aux.c
>[..]
>> +static int dp_runtime_resume(struct device *dev) {
>> +struct platform_device *pdev = to_platform_device(dev);
>> +struct msm_dp *dp_display = platform_get_drvdata(pdev);
>> +struct dp_display_private *dp;
>> +
>> +dp = container_of(dp_display, struct dp_display_private, dp_display);
>> +dp_display_host_init(dp);
>> +
>> +if (dp->dp_display.is_edp) {
>> +dp_display_host_phy_init(dp);
>> +} else {
>> +dp_catalog_hpd_config_intr(dp->catalog,
>> +DP_DP_HPD_PLUG_INT_MASK |
>> +DP_DP_HPD_UNPLUG_INT_MASK,
>> +true);
>
>I believe this is backwards.
>
>Only in the event that there's no "downstream" HPD handler should we use
>the internal HPD. This is signalled by the DRM framework by a call to
>dp_bridge_hpd_enable(). So we should use that to enable/disable the
>internal HPD handler.
>
>When this happens, we have a reason for keeping power on; i.e. call
>pm_runtime_get(). Once we have power/clocking, we'd call
>dp_catalog_hpd_config_intr(), from dp_bridge_hpd_enable().
>
>
>In the case that the internal HPD handling is not use,
>dp_bridge_hpd_enable() will not be called, instead once the downstream hpd
>handler switches state dp_bridge_hpd_notify() will be invoked.
>
>In this case, we need the DP controller to be powered/clocked between
>connector_status_connected and connector_status_disconnected.
>
>
>I believe this should allow the DP controller(s) to stay powered down in the
>case where we have external HPD handling (e.g. USB Type-C or gpio-based
>dp-connector).
>
>Regards,
>Bjorn
I agree with the approach. I am moving my dev to msm-next. Will make the
changes according to the HPD handling and repost
Thank you,
Sankeerth
Re: [Freedreno] [RFC PATCH 0/2] drm/msm/dp: refactor the msm dp driver resources
>> The DP driver resources are currently enabled and disabled directly based >on code flow. >> As mentioned in bug 230631602, we want to do the following: > >private bug tracker > Will remove the reference for this. >> >> 1) Refactor the dp/edp parsing code to move it to probe (it is currently done >in bind). > >This is good. I'd suggest splitting this into smaller chunks. First, move all >resource binding, then move the actual dp_aux handling. It would be easier to >review it this way. > Okay, will move the resource binding patch first. >> 2) Then bind all the power resources needed for AUX in pm_runtime_ops. >> >> 3) Handle EPROBE_DEFER cases of the panel-eDP aux device. > >This is not handled properly. The eDP aux probing is asynchronous, so you >should move the second stage into the done_probing() part, rather than >relying on the -EPROBE_DEFER. There can be cases, when the panel driver is >not available at the DP's probe time. In such cases we should leave the DP >driver probed, then wait for the panel before binding the component. > Okay, I will explore this. >> 4) Verify DP functionality is unaffected. >> >> These code changes will parse the resources and get the edp panel during >probe. >> All the necessary resources required for the aux transactions are moved to >pm_runtime ops. >> They are enabled or disabled via get/put sync functions. >> >> This is a RFC to verify with the community if the approach we are taking is >correct. >> >> https://partnerissuetracker.corp.google.com/issues/230631602 > >This link is useless, since its contents are not public. > Okay, I will remove it. >> >> Sankeerth Billakanti (2): >> drm/msm/dp: enumerate edp panel during driver probe >> drm/msm/dp: enable pm_runtime support for dp driver >> >> drivers/gpu/drm/msm/dp/dp_aux.c | 155 +-- >> 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 | 185 ++-- >> drivers/gpu/drm/msm/dp/dp_power.c | 7 -- >> 5 files changed, 250 insertions(+), 110 deletions(-) >> >> -- >> 2.39.0 >> > > >-- >With best wishes >Dmitry
Re: [Freedreno] [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
>> ---
>> 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(>comp,
>> + if (!poll) {
>> + time_left = wait_for_completion_timeout(>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(>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 = >segment;
>> helper_msg.size = 1;
>> - dp_aux_cmd_fifo_tx(aux, _msg);
>> + dp_aux_cmd_fifo_tx(aux, _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 = >offset;
>> helper_msg.size = 1;
>> - dp_aux_cmd_fifo_tx(aux, _msg);
>> + dp_aux_cmd_fifo_tx(aux, _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 =
Re: [Freedreno] [v3 3/5] drm/bridge: add psr support during panel bridge enable & disable sequence
Hi Dmitry,
>On 21/06/2022 13:53, Vinod Polimera wrote:
>> This change avoids panel prepare/unprepare based on self-refresh
>> state.
>>
>> Signed-off-by: Sankeerth Billakanti
>> Signed-off-by: Kalyan Thota
>> Signed-off-by: Vinod Polimera
>> ---
>> drivers/gpu/drm/bridge/panel.c | 102
>+++--
>> 1 file changed, 98 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/panel.c
>> b/drivers/gpu/drm/bridge/panel.c index 59a3496..6b09ae0 100644
>> --- a/drivers/gpu/drm/bridge/panel.c
>> +++ b/drivers/gpu/drm/bridge/panel.c
>> @@ -41,6 +41,40 @@ static int panel_bridge_connector_get_modes(struct
>drm_connector *connector)
>> return drm_panel_get_modes(panel_bridge->panel, connector);
>> }
>>
>> +static struct drm_crtc *bridge_drm_get_old_connector_crtc(struct
>drm_encoder *encoder,
>> +struct
>drm_atomic_state *state) {
>> +struct drm_connector *connector;
>> +struct drm_connector_state *conn_state;
>> +
>> +connector = drm_atomic_get_old_connector_for_encoder(state,
>encoder);
>> +if (!connector)
>> +return NULL;
>> +
>> +conn_state = drm_atomic_get_old_connector_state(state,
>connector);
>> +if (!conn_state)
>> +return NULL;
>> +
>> +return conn_state->crtc;
>> +}
>> +
>> +static struct drm_crtc *bridge_drm_get_new_connector_crtc(struct
>drm_encoder *encoder,
>> +struct
>drm_atomic_state *state) {
>> +struct drm_connector *connector;
>> +struct drm_connector_state *conn_state;
>> +
>> +connector = drm_atomic_get_new_connector_for_encoder(state,
>encoder);
>> +if (!connector)
>> +return NULL;
>> +
>> +conn_state = drm_atomic_get_new_connector_state(state,
>connector);
>> +if (!conn_state)
>> +return NULL;
>> +
>> +return conn_state->crtc;
>> +}
>
>As I wrote earlier, this should become generic drm helpers.
>
Yes, will move it.
>> +
>> static const struct drm_connector_helper_funcs
>> panel_bridge_connector_helper_funcs = {
>> .get_modes = panel_bridge_connector_get_modes, @@ -108,30
>+142,90
>> @@ static void panel_bridge_detach(struct drm_bridge *bridge)
>> drm_connector_cleanup(connector);
>> }
>>
>> -static void panel_bridge_atomic_pre_enable(struct drm_bridge *bridge)
>> +static void panel_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>> +struct drm_bridge_state
>*old_bridge_state)
>
>This must be a part of the previous patch?
>
Yes, it should be moved to that patch.
>> {
>> struct panel_bridge *panel_bridge =
>> drm_bridge_to_panel_bridge(bridge);
>> +struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> +struct drm_encoder *encoder = bridge->encoder;
>> +struct drm_crtc *crtc;
>> +struct drm_crtc_state *old_crtc_state;
>> +
>> +crtc = bridge_drm_get_new_connector_crtc(encoder, old_state);
>> +if (!crtc)
>> +return;
>
>Why? And why do you ask for the new crtc from the old state?
>
If the previous bridge disable and post_disable calls were issued just to enter
psr,
then the panel power and backlight will still be on.
We need to know the psr status of the old state of the crtc to decide whether to
enable the panel power or just early return.
old_state is the atomic_state object. Will change the variable name to
atomic_state.
>> +
>> +old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
>> +
>> +/* Don't touch the panel if we're coming back from self refresh state
>*/
>> +if (old_crtc_state && old_crtc_state->self_refresh_active)
>> +return;
>>
>> drm_panel_prepare(panel_bridge->panel);
>> }
>>
>> -static void panel_bridge_atomic_enable(struct drm_bridge *bridge)
>> +static void panel_bridge_atomic_enable(struct drm_bridge *bridge,
>> +struct drm_bridge_state
>*old_bridge_state)
>> {
>> struct panel_bridge *panel_bridge =
>> drm_bridge_to_panel_bridge(bridge);
>> +struct drm_atomic_state *old_state = old_bridge_state->base.state;
>> +struct drm_encoder *encoder = bridge->encoder;
>> +struct drm_crtc *crtc;
>> +struct drm_crtc_state *old_crtc_state;
>> +
>> +crtc = bridge_drm_get_new_connector_crtc(encoder, old_state);
>> +if (!crtc)
>> +return;
>> +
>> +old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
>> +
>> +/* Don't touch the panel if we're coming back from self refresh state
>*/
>> +if (old_crtc_state && old_crtc_state->self_refresh_active)
>> +return;
>>
>> drm_panel_enable(panel_bridge->panel);
>> }
>>
>> -static void panel_bridge_atomic_disable(struct drm_bridge *bridge)
>> +static void panel_bridge_atomic_disable(struct drm_bridge *bridge,
>> +struct drm_bridge_state
>*old_bridge_state)
>> {
>>
Re: [Freedreno] [v3 1/5] drm/msm/dp: Add basic PSR support for eDP
Hi Dmitry,
>On 21/06/2022 13:53, Vinod Polimera wrote:
>> Add support for basic panel self refresh (PSR) feature for eDP.
>> Add a new interface to set PSR state in the sink from DPU.
>> Program the eDP controller to issue PSR enter and exit SDP to the
>> sink.
>>
>> Signed-off-by: Sankeerth Billakanti
>> Signed-off-by: Vinod Polimera
>> ---
>> drivers/gpu/drm/msm/dp/dp_catalog.c | 81 ++
>> drivers/gpu/drm/msm/dp/dp_catalog.h | 4 +
>> drivers/gpu/drm/msm/dp/dp_ctrl.c| 76 -
>> drivers/gpu/drm/msm/dp/dp_ctrl.h| 3 +
>> drivers/gpu/drm/msm/dp/dp_display.c | 14 +++
>> drivers/gpu/drm/msm/dp/dp_display.h | 2 +
>> drivers/gpu/drm/msm/dp/dp_drm.c | 166
>+++-
>> drivers/gpu/drm/msm/dp/dp_link.c| 36
>> drivers/gpu/drm/msm/dp/dp_panel.c | 22 +
>> drivers/gpu/drm/msm/dp/dp_panel.h | 6 ++
>> drivers/gpu/drm/msm/dp/dp_reg.h | 27 ++
>> 11 files changed, 433 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index 7257515..b9021ed 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -47,6 +47,14 @@
>> #define DP_INTERRUPT_STATUS2_MASK \
>> (DP_INTERRUPT_STATUS2 << DP_INTERRUPT_STATUS_MASK_SHIFT)
>>
>> +#define DP_INTERRUPT_STATUS4 \
>> +(PSR_UPDATE_INT | PSR_CAPTURE_INT | PSR_EXIT_INT | \
>> +PSR_UPDATE_ERROR_INT | PSR_WAKE_ERROR_INT)
>> +
>> +#define DP_INTERRUPT_MASK4 \
>> +(PSR_UPDATE_MASK | PSR_CAPTURE_MASK | PSR_EXIT_MASK | \
>> +PSR_UPDATE_ERROR_MASK | PSR_WAKE_ERROR_MASK)
>> +
>> struct dp_catalog_private {
>> struct device *dev;
>> struct drm_device *drm_dev;
>> @@ -359,6 +367,24 @@ void dp_catalog_ctrl_lane_mapping(struct
>dp_catalog *dp_catalog)
>> ln_mapping);
>> }
>>
>> +void dp_catalog_ctrl_psr_mainlink_enable(struct dp_catalog *dp_catalog,
>> +bool enable)
>> +{
>> +u32 val;
>> +struct dp_catalog_private *catalog = container_of(dp_catalog,
>> +struct dp_catalog_private, dp_catalog);
>> +
>> +val = dp_read_link(catalog, REG_DP_MAINLINK_CTRL);
>> +val &= ~DP_MAINLINK_CTRL_ENABLE;
>> +
>> +if (enable)
>> +val |= DP_MAINLINK_CTRL_ENABLE;
>> +else
>> +val &= ~DP_MAINLINK_CTRL_ENABLE;
>> +
>> +dp_write_link(catalog, REG_DP_MAINLINK_CTRL, val); }
>> +
>> void dp_catalog_ctrl_mainlink_ctrl(struct dp_catalog *dp_catalog,
>> bool enable)
>> {
>> @@ -610,6 +636,47 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
>*dp_catalog)
>> dp_write_aux(catalog, REG_DP_DP_HPD_CTRL,
>DP_DP_HPD_CTRL_HPD_EN);
>> }
>>
>> +static void dp_catalog_enable_sdp(struct dp_catalog_private *catalog)
>> +{
>> +/* trigger sdp */
>> +dp_write_link(catalog, MMSS_DP_SDP_CFG3, UPDATE_SDP);
>> +dp_write_link(catalog, MMSS_DP_SDP_CFG3, !UPDATE_SDP); }
>> +
>> +void dp_catalog_ctrl_config_psr(struct dp_catalog *dp_catalog) {
>> +struct dp_catalog_private *catalog = container_of(dp_catalog,
>> +struct dp_catalog_private, dp_catalog);
>> +u32 config;
>> +
>> +/* enable PSR1 function */
>> +config = dp_read_link(catalog, REG_PSR_CONFIG);
>> +config |= PSR1_SUPPORTED;
>> +dp_write_link(catalog, REG_PSR_CONFIG, config);
>> +
>> +dp_write_ahb(catalog, REG_DP_INTR_MASK4,
>DP_INTERRUPT_MASK4);
>> +dp_catalog_enable_sdp(catalog);
>> +}
>> +
>> +void dp_catalog_ctrl_set_psr(struct dp_catalog *dp_catalog, bool
>> +enter) {
>> +struct dp_catalog_private *catalog = container_of(dp_catalog,
>> +struct dp_catalog_private, dp_catalog);
>> +u32 cmd;
>> +
>> +cmd = dp_read_link(catalog, REG_PSR_CMD);
>> +
>> +cmd &= ~(PSR_ENTER | PSR_EXIT);
>> +
>> +if (enter)
>> +cmd |= PSR_ENTER;
>> +else
>> +cmd |= PSR_EXIT;
>> +
>> +dp_catalog_enable_sdp(catalog);
>> +dp_write_link(catalog, REG_PSR_CMD, cmd); }
>> +
>> u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
>> {
>> struct dp_catalog_private *catalog = container_of(dp_catalog, @@
>> -645,6 +712,20 @@ u32 dp_catalog_hpd_get_intr_status(struct dp_catalog
>*dp_catalog)
>> return isr & (mask | ~DP_DP_HPD_INT_MASK);
>> }
>>
>> +int dp_catalog_ctrl_read_psr_interrupt_status(struct dp_catalog
>> +*dp_catalog) {
>> +struct dp_catalog_private *catalog = container_of(dp_catalog,
>> +struct dp_catalog_private, dp_catalog);
>> +u32 intr, intr_ack;
>> +
>> +intr = dp_read_ahb(catalog, REG_DP_INTR_STATUS4);
>> +intr_ack = (intr & DP_INTERRUPT_STATUS4)
>> +<< DP_INTERRUPT_STATUS_ACK_SHIFT;
>> +dp_write_ahb(catalog, REG_DP_INTR_STATUS4, intr_ack);
>> +
>> +return intr;
>> +}
>> +
>>
Re: [Freedreno] [PATCH 1/2] dt-bindings: msm/dp: List supplies in the bindings
>> >> Our internal power grid documents list the regulators as >> >> VDD_A_*_1P2 and VDD_A_*_0P9 for all the platforms. >> > >> >Do your internal power grid documents indicate what these supplies >> >are powering? The question is if these supplies power any of the >> >logic inside the eDP controller or if they only supply power to the >> >analog circuits in the eDP phy. If it's the eDP phy only then the >> >regulator usage in the eDP driver should be removed. I would suspect >> >this is the case because the controller is probably all digital logic >> >and runs at the typical 1.8V that the rest of the SoC uses. >> >Similarly, these are voltage references which sound like a PLL reference >voltage. >> > >> >Please clarify this further. >> > >> >> For the DP driver using the usb-dp combo phy, there were cases where >> the usb driver was turning off the phy and pll regulators whenever usb-dp >concurrent mode need not be supported. >> This caused phy and pll to be powered down causing aux transaction failures >and display blankouts. >> From then on, it became a practice for the controller driver to vote for the >phy and pll regulators also. >> > >That sounds like USB-DP combo phy driver had improper regulator power >management where aux transactions from DP didn't keep the power on to >the phy. Where does the power physically go? If the power isn't physically >going to the DP controller it shouldn't be controlled from the DP controller >driver. If the aux bus needs the DP phy enabled, the DP controller driver >should enable the phy power (via phy_power_on()?). Yes, it was limitation earlier when we did not have proper interface to interact with the combo phy. In this case, the power from the regulators go to the combo phy. Now that there is an interface for the controller to interact with the combo phy, the proposal to drop the phy regulator voting from the controller driver seems reasonable to me. The phy_power_on() is used for getting the phy out of low power state or getting it ready for data transfer. The controller driver needs to enable the phy power via the phy_init() before any interaction with the sink like the aux transactions or before sending the data. The controller can disable the regulators via the phy_exit() call. Thank you, Sankeerth
Re: [Freedreno] [PATCH 2/2] dt-bindings: phy: List supplies for qcom, edp-phy
>We're supposed to list the supplies in the dt bindings but there are none in >the eDP PHY bindings. > >Looking at the driver in Linux, I can see that there seem to be two relevant >supplies: "vdda-phy" and "vdda-pll". Let's add those to the bindings. > >NOTE: from looking at the Qualcomm datasheet for sc7280, it's not >immediately clear how to figure out how to fill in these supplies. The only two >eDP related supplies are simply described as "power for eDP 0.9V circuits" and >"power for eDP 1.2V circuits". From guessing and from comparing how a >similar PHY is hooked up on other similar Qualcomm boards, I'll make the >educated guess that the 1.2V supply goes to "vdda-phy" and the 0.9V supply >goes to "vdda-pll" and I'll use that in the example here. > >Signed-off-by: Douglas Anderson >--- > > Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml | 6 ++ > 1 file changed, 6 insertions(+) > >diff --git a/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml >b/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml >index a5850ff529f8..cf9e9b8011cb 100644 >--- a/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml >+++ b/Documentation/devicetree/bindings/phy/qcom,edp-phy.yaml >@@ -41,6 +41,9 @@ properties: > "#phy-cells": > const: 0 > >+ vdda-phy-supply: true >+ vdda-pll-supply: true >+ > required: > - compatible > - reg >@@ -65,5 +68,8 @@ examples: > > #clock-cells = <1>; > #phy-cells = <0>; >+ >+ vdda-phy-supply = <_a_edp_0_1p2>; >+ vdda-pll-supply = <_a_edp_0_0p9>; > }; > ... >-- >2.36.0.rc2.479.g8af0fa9b8e-goog Reviewed-by: Sankeerth Billakanti
Re: [Freedreno] [PATCH 1/2] dt-bindings: msm/dp: List supplies in the bindings
>Quoting Sankeerth Billakanti (2022-05-05 11:02:36) >> >> >> >> Quoting Douglas Anderson (2022-04-25 14:06:42) >> >> >> >> Having 'a' in 'vdda' typically means 'analog' for 'analog' >> >> circuits, so I'd expect this to only matter for the phy that >> >> contains the analog circuitry. It would be great to remove the >> >> regulator code from drivers/gpu/drm/msm/dp/dp_power.c and move >the >> >> regulator_set_load() call to the phy driver if possible. Hopefully >> >> qcom folks can help clarify here. >> > >> >Interesting. Oddly enough, the sc7280 datasheet doesn't list the "_A". >> >It calls these "VDD_VREF_0P9" and "VDD_VREF_1P2". However, on the >> >schematic in front of me someone labeled these pins on the sc7280 >> >with the "A". ...and the driver looks for a supply with the "a". :-/ >> > >> >It would be good to get clarification from someone with better >information. >> > >> >-Doug >> >> Our internal power grid documents list the regulators as VDD_A_*_1P2 >> and VDD_A_*_0P9 for all the platforms. > >Do your internal power grid documents indicate what these supplies are >powering? The question is if these supplies power any of the logic inside the >eDP controller or if they only supply power to the analog circuits in the eDP >phy. If it's the eDP phy only then the regulator usage in the eDP driver should >be removed. I would suspect this is the case because the controller is >probably all digital logic and runs at the typical 1.8V that the rest of the >SoC >uses. Similarly, these are voltage references which sound like a PLL reference >voltage. > >Please clarify this further. > For the DP driver using the usb-dp combo phy, there were cases where the usb driver was turning off the phy and pll regulators whenever usb-dp concurrent mode need not be supported. This caused phy and pll to be powered down causing aux transaction failures and display blankouts. From then on, it became a practice for the controller driver to vote for the phy and pll regulators also. >> >> So, as a practice, we put the same name in the DT files. Hence, >> >> Reviewed-by: Sankeerth Billakanti >>
Re: [Freedreno] [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
Hi Stephen, >> >> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) >> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c >> >> b/drivers/gpu/drm/msm/dp/dp_display.c >> >> index 055681a..dea4de9 100644 >> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> >> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct >> >dp_display_private *dp) >> >> dp_display_host_init(dp); >> >> dp_catalog_ctrl_hpd_config(dp->catalog); >> >> >> >> + /* Enable plug and unplug interrupts only for external >> >> DisplayPort */ >> >> + if (!dp->dp_display.is_edp) >> >> + dp_catalog_hpd_config_intr(dp->catalog, >> >> + DP_DP_HPD_PLUG_INT_MASK | >> >> + DP_DP_HPD_UNPLUG_INT_MASK, >> >> + true); >> >> + >> > >> >It seems like only the plug and unplug is enabled for DP here. Is >> >replug enabled for eDP when it shouldn't be? >> > >> >> By default, all the interrupts are masked. This function is not >> executed for eDP anymore because the host_init, phy_init and >> enable_irq are all done from modeset_init for eDP with aux_bus. So, >> none of the eDP hpd interrupts are enabled or unmasked before pre- >enable. >> >> The replug interrupt is unmasked for DP and eDP from the >> dp_hpd_plug_handle() and masked from dp_hpd_unplug_handle(). > >Why is replug enabled for eDP? As the eDP panel is assumed to be always connected, just enabling the IRQ_HPD is sufficient. The REPLUG is enabled or unmasked along with IRQ_HPD in code. I did not remove the REPLUG event support for eDP so that we have an option to see if the eDP panel can undergo a short disconnect/connect cycle after pre-enable (while the panel power is enabled). REPLUG can be generated for eDP if, a) the panel power turns off and on OR b) the sink itself issues a fast disconnect-connect. REPLUG event initiated by sink is rare and we observed it only during the DP compliance test. Some more information on HPD events generated by the source: Replug interrupt is something our controller HW supports and not part of the DP/eDP specification. The programmed values for HPD on the HW controller indicates the following: 1. The HOTPLUG interrupt will be generated if the HPD line is continuously high for 100ms. 2. Similarly, UNPLUG interrupt will be generated when the HPD line transitions from high to low and remains low for 100ms. 3. IRQ_HPD will be generated when the HPD line transitions from high to low and remains low for less than 2ms. 4. REPLUG will be generated if the HPD line remains low for more than 2ms but less than 100ms. According to the DP spec, replug event should be considered as a disconnect and then connect. To answer your question, I did not remove REPLUG support for eDP because I felt it will not affect the eDP normal functioning in anyway. Thank you, Sankeerth
Re: [Freedreno] [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
Hi Stephen, Quoting Sankeerth Billakanti (2022-04-22 02:11:03) > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index d7a19d6..055681a 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c Some nitpicks Reviewed-by: Stephen Boyd > @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp > *dp_display) > > dp_hpd_event_setup(dp); > > - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100); > + if (!dp_display->is_edp) > + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100); Did it turn out that in fact DP isn't ready still to setup even after delaying the irq? >>> >>> The host_init, config_hpd, phy_init and enable_irq are happening in >modeset_init already for eDP. >>> So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not >modifying the delay for DP. >> >> Cool. That didn't answer my question though. Why does DP still need >> the delay? I thought recent changes made it unnecessary. > >I'd say that if it is not necessary, it should be changed in the separate >commit. >The question is valid nevertheless. > Yes, that is right. The delay is unnecessary with the recent changes. Like Dmitry rightly suggested, we will remove the delay in a separate commit. > >-- >With best wishes >Dmitry
Re: [Freedreno] [PATCH v9 1/4] drm/msm/dp: Add eDP support via aux_bus
Hi Stephen,
>Quoting Sankeerth Billakanti (2022-04-22 02:11:03)
>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
>> b/drivers/gpu/drm/msm/dp/dp_display.c
>> index d7a19d6..055681a 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>
>Some nitpicks
>
>Reviewed-by: Stephen Boyd
>
>> @@ -1508,7 +1509,8 @@ void msm_dp_irq_postinstall(struct msm_dp
>> *dp_display)
>>
>> dp_hpd_event_setup(dp);
>>
>> - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>> + if (!dp_display->is_edp)
>> + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
>
>Did it turn out that in fact DP isn't ready still to setup even after delaying
>the
>irq?
>
The host_init, config_hpd, phy_init and enable_irq are happening in
modeset_init already for eDP.
So, I am not scheduling the EV_HPD_INIT_SETUP event for eDP. I am not modifying
the delay for DP.
>> }
>>
>> void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor
>> *minor) @@ -1530,6 +1532,65 @@ void msm_dp_debugfs_init(struct
>msm_dp *dp_display, struct drm_minor *minor)
>> }
>> }
>>
>> +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 = _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 = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>> + of_node_put(aux_bus);
>> + 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
>> +* provide at least an eDP panel (which gets wrapped into panel-
>bridge).
>> +*
>> +* For DisplayPort interfaces external bridges are optional, so
>> +* silently ignore an error if one is not present (-ENODEV).
>> +*/
>> + rc = dp_parser_find_next_bridge(dp_priv->parser);
>> + if (!dp->is_edp && rc == -ENODEV)
>> + return 0;
>> + else if (rc)
>
>Just an if because there's a return above.
>
Okay
>> + goto error;
>> +
>> + dp->next_bridge = dp_priv->parser->next_bridge;
>
>In which case it almost feels like it could be written
>
> if (!dp->is_edp && rc == -ENODEV)
> return 0;
> if (!rc) {
> dp->next_bridge = dp_priv->parser->next_bridge;
> return 0;
> }
>error:
> if (dp->is_edp) {
>
>but I'm not worried either way, besides removing the else from the else-if.
>
Okay
>> +
>> + 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;
>> +}
>> +
>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device
>*dev,
>> struct drm_encoder *encoder) { diff --git
>> a/drivers/gpu/drm/msm/dp/dp_parser.h
>> b/drivers/gpu/drm/msm/dp/dp_parser.h
>> index d371bae..950416c 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_parser.h
>> +++ b/drivers/gpu/drm/msm/dp/dp_parser.h
>> @@ -125,7 +125,7 @@ struct dp_parser {
>> u32 max_dp_lanes;
>> struct drm_bridge *next_bridge;
>>
>> - int (*parse)(struct dp_parser *parser, int connector_type);
>> + int (*parse)(struct dp_parser *parser);
>> };
>>
>> /**
>> @@ -141,4 +141,15 @@ struct dp_parser {
>> */
>> struct dp_parser *dp_parser_get(struct platform_device *pdev);
>>
>> +/**
>> + * dp_parser_find_next_bridge() - find an additional bridge to DP
>> + *
>> + * @parser: dp_parser data from client
>> + * return: 0 if able to get the bridge else return an error code
>
>return comes after the description below. Also should be capitalized.
>You can check this by compiling with W=1 I believe, or run the kernel doc
>format script on the file..
>
Okay
>> + *
>> + * This function is used to find any
Re: [Freedreno] [PATCH v9 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
Hi Stephen, >Quoting Sankeerth Billakanti (2022-04-22 02:11:04) >> The panel-edp enables the eDP panel power during probe, get_modes and >> pre-enable. The eDP connect and disconnect interrupts for the eDP/DP >> controller are directly dependent on panel power. As eDP display can >> be assumed as always connected, the controller driver can skip the eDP >> connect and disconnect interrupts. Any disruption in the link status >> will be indicated via the IRQ_HPD interrupts. >> >> So, the eDP controller driver can just enable the IRQ_HPD and replug >> interrupts. The DP controller driver still needs to enable all the >> interrupts. >> >> Signed-off-by: Sankeerth Billakanti > >The sprinkling of if conditions and manual driving of the DP plug/unplug state >machine is pretty convoluted. To make it better the driver needs an overhaul. >Anyway, it looks mostly fine to me except for this replug interrupt question >below. Otherwise > >Reviewed-by: Stephen Boyd > >> int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog) diff >> --git a/drivers/gpu/drm/msm/dp/dp_display.c >> b/drivers/gpu/drm/msm/dp/dp_display.c >> index 055681a..dea4de9 100644 >> --- a/drivers/gpu/drm/msm/dp/dp_display.c >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c >> @@ -1096,6 +1097,13 @@ static void dp_display_config_hpd(struct >dp_display_private *dp) >> dp_display_host_init(dp); >> dp_catalog_ctrl_hpd_config(dp->catalog); >> >> + /* Enable plug and unplug interrupts only for external DisplayPort */ >> + if (!dp->dp_display.is_edp) >> + dp_catalog_hpd_config_intr(dp->catalog, >> + DP_DP_HPD_PLUG_INT_MASK | >> + DP_DP_HPD_UNPLUG_INT_MASK, >> + true); >> + > >It seems like only the plug and unplug is enabled for DP here. Is replug >enabled for eDP when it shouldn't be? > By default, all the interrupts are masked. This function is not executed for eDP anymore because the host_init, phy_init and enable_irq are all done from modeset_init for eDP with aux_bus. So, none of the eDP hpd interrupts are enabled or unmasked before pre-enable. The replug interrupt is unmasked for DP and eDP from the dp_hpd_plug_handle() and masked from dp_hpd_unplug_handle(). >> /* Enable interrupt first time >> * we are leaving dp clocks on during disconnect >> * and never disable interrupt
Re: [Freedreno] [PATCH v8 2/4] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
Hi Doug,
>On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
> wrote:
>>
>> The panel-edp enables the eDP panel power during probe, get_modes and
>> pre-enable. The eDP connect and disconnect interrupts for the eDP/DP
>> controller are directly dependent on panel power. As eDP display can
>> be assumed as always connected, the controller driver can skip the eDP
>> connect and disconnect interrupts. Any disruption in the link status
>> will be indicated via the IRQ_HPD interrupts.
>>
>> So, the eDP controller driver can just enable the IRQ_HPD and replug
>> interrupts. The DP controller driver still needs to enable all the
>> interrupts.
>>
>> Signed-off-by: Sankeerth Billakanti
>> ---
>> Changes in v8:
>> - add comment explaining the interrupt status return
>>
>> Changes in v7:
>> - reordered the patch in the series
>> - modified the return statement for isr
>> - connector check modified to just check for eDP
>>
>> drivers/gpu/drm/msm/dp/dp_catalog.c | 18 --
>> drivers/gpu/drm/msm/dp/dp_display.c | 22 +-
>> 2 files changed, 33 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> index fac815f..3a298e9 100644
>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
>> @@ -569,10 +569,6 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
>> *dp_catalog)
>>
>> u32 reftimer = dp_read_aux(catalog, REG_DP_DP_HPD_REFTIMER);
>>
>> - /* enable HPD plug and unplug interrupts */
>> - dp_catalog_hpd_config_intr(dp_catalog,
>> - DP_DP_HPD_PLUG_INT_MASK |
>DP_DP_HPD_UNPLUG_INT_MASK, true);
>> -
>> /* Configure REFTIMER and enable it */
>> reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
>> dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer); @@
>> -599,13 +595,23 @@ u32 dp_catalog_hpd_get_intr_status(struct
>> dp_catalog *dp_catalog) {
>> struct dp_catalog_private *catalog = container_of(dp_catalog,
>> struct dp_catalog_private, dp_catalog);
>> - int isr = 0;
>> + int isr, mask;
>>
>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
>> (isr & DP_DP_HPD_INT_MASK));
>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
>>
>> - return isr;
>> + /*
>> +* REG_DP_DP_HPD_INT_STATUS reports the status of all interrupts
>> +* irrespective of their masked status. The HW interrupt will not be
>> +* generated if an interrupt is masked. However, the interrupt status
>> +* bit in the register will still be set. The eDP controller driver
>> +* masks the plug and unplug interrupts unlike DP controller which
>> +* unmasks all the interrupts. So, do not report the status of the
>> +* masked interrupts.
>> +*/
>> + return isr & (mask | ~DP_DP_HPD_INT_MASK);
>
>What's still missing in your comments is why you aren't just doing "return isr
>&
>mask;". In other words, why is the API for HPD bits different from the API for
>non-HPD bits? What code out there wants to know about non-HPD interrupts
>even if they are masked?
The mask register bitfields are different from the INT_STATUS register.
The INT_STATUS register has additional bits [31:28] which indicates the HPD
state machine
status and [3:0] indicates the actual generated/set interrupt. The fields [3:0]
are similar to
the mask and ack interrupts.
#define DP_DP_HPD_STATE_STATUS_CONNECTED(0x4000)
#define DP_DP_HPD_STATE_STATUS_PENDING (0x2000)
#define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x)
#define DP_DP_HPD_STATE_STATUS_MASK (0xE000)
So, earlier I returned isr & (mask | DP_DP_HPD_STATE_STATUS_MASK);
Actually, there is no reason to do this. We can just do the below:
return isr & mask;
>
>Actually, thinking about this more, my preference would be this:
>
>a) Rename the existing function to dp_catalog_hpd_get_intr_status_raw()
>
>b) Create a new function called dp_catalog_hpd_get_intr_status() whose
>implementation is:
>
> return dp_catalog_hpd_get_intr_status_raw() & mask;
>
>Then any callers who care about the raw status can be changed to call
>dp_catalog_hpd_get_intr_status_raw(). If no callers need
>dp_catalog_hpd_get_intr_status_raw() then you don't actually need to
>create this function.
>
There is no caller for raw status. All interrupts are unmasked for DP.
While handling the aux interrupts generated during the transfer call from panel
probe,
we read that the HPD interrupt status [3:0] bit0 is set and proceed to handle
connect like DP.
We experimented to find out that the connect interrupt is not generated but
just the status
bit is set.
As the interrupts are generated over a single MDSS line, the controller driver
has to read all the
interrupt status registers and
Re: [Freedreno] [PATCH v8 1/4] drm/msm/dp: Add eDP support via aux_bus
Hi Doug,
>On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti
> wrote:
>>
>> @@ -1530,6 +1532,61 @@ void msm_dp_debugfs_init(struct msm_dp
>*dp_display, struct drm_minor *minor)
>> }
>> }
>>
>> +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 = _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);
>> +
>> + rc = devm_of_dp_aux_populate_ep_devices(dp_priv->aux);
>
>I think a comment was requested above that line saying something like:
>
>/*
> * 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.
> */
>
>
Will add the comment
>> + of_node_put(aux_bus);
>> + if (rc)
>> + goto edp_error;
>> + } else if (dp->is_edp) {
>> + DRM_ERROR("eDP aux_bus not found\n");
>> + rc = -ENODEV;
>> + goto error;
>
>This goto doesn't add much. Just leave the above like it was in v7.
>return -ENODEV w/ no goto.
>
>
Okay
>> + }
>> +
>> + /*
>> +* External bridges are mandatory for eDP interfaces: one has to
>> +* provide at least an eDP panel (which gets wrapped into panel-
>bridge).
>> +*
>> +* For DisplayPort interfaces external bridges are optional, so
>> +* silently ignore an error if one is not present (-ENODEV).
>> +*/
>> + rc = dp_parser_find_next_bridge(dp_priv->parser);
>> + if (rc && dp->is_edp) {
>> + DRM_ERROR("eDP: cannot find the next bridge, rc = %d\n", rc);
>> + goto edp_error;
>> + } else if (rc && rc != -ENODEV) {
>> + DRM_ERROR("DP: cannot find the next bridge, rc = %d\n", rc);
>> + goto error;
>> + }
>
>The above wouldn't be my favorite way of doing this. Instead, I would have
>written:
>
> if (rc) {
>DRM_ERROR("Cannot find the next bridge, rc = %d\n", rc);
>goto err;
> }
> ...
>
>err:
> if (dp->is_edp) {
>disable_irq(...);
>dp_display_host_phy_exit(...);
>dp_display_host_deinit(...);
> }
> return rc;
>
If rc is ENODEV for DP, then we need to return 0. Shall I add like below ?
err:
if (dp->is_edp) {
disable_irq(...);
dp_display_host_phy_exit(...);
dp_display_host_deinit(...);
} else
If (rc == -ENODEV)
rc = 0;
return rc;
>> +
>> + dp->next_bridge = dp_priv->parser->next_bridge;
>> +
>> + return 0;
>> +
>> +edp_error:
>> + disable_irq(dp_priv->irq);
>> + dp_display_host_phy_exit(dp_priv);
>> + dp_display_host_deinit(dp_priv);
>> +error:
>> + return rc;
>> +}
>> +
>> int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device
>*dev,
>> struct drm_encoder *encoder) {
>
>With the above fixes, I'd be happy enough for my Reviewed-by tag with the
>expectation that continued work will happen to continue cleaning this up.
>
>
>-Doug
Re: [Freedreno] [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus
Hi Dmitry and Doug, > Hi, > > On Tue, Apr 5, 2022 at 10:36 AM Dmitry Baryshkov > wrote: > > > > On 05/04/2022 20:02, Doug Anderson wrote: > > > Hi, > > > > > > On Tue, Apr 5, 2022 at 5:54 AM Dmitry Baryshkov > > > wrote: > > >>> 3. For DP and eDP HPD means something a little different. > > >>> Essentially there are two concepts: a) is a display physically > > >>> connected and b) is the display powered up and ready. For DP, the > > >>> two are really tied together. From the kernel's point of view you > > >>> never "power down" a DP display and you can't detect that it's > > >>> physically connected until it's ready. Said another way, on you > > >>> tie "is a display there" to the HPD line and the moment a display > > >>> is there it's ready for you to do AUX transfers. For eDP, in the > > >>> lowest power state of a display it _won't_ assert its "HPD" > > >>> signal. However, it's still physically present. For eDP you simply > > >>> have to _assume_ it's present without any actual proof since you > > >>> can't get proof until you power it up. Thus for eDP, you report > > >>> that the display is there as soon as we're asked. We can't _talk_ > > >>> to the display yet, though. So in get_modes() we need to be able > > >>> to power the display on enough to talk over the AUX channel to it. > > >>> As part of this, we wait for the signal named "HPD" which really means > "panel finished powering on" in this context. > > >>> > > >>> NOTE: for aux transfer, we don't have the _display_ pipe and > > >>> clocks running. We only have enough stuff running to do the AUX > transfer. > > >>> We're not clocking out pixels. We haven't fully powered on the > > >>> display. The AUX transfer is designed to be something that can be > > >>> done early _before_ you turn on the display. > > >>> > > >>> > > >>> OK, so basically that was a longwinded way of saying: yes, we > > >>> could avoid the AUX transfer in probe, but we can't wait all the > > >>> way to enable. We have to be able to transfer in get_modes(). If > > >>> you think that's helpful I think it'd be a pretty easy patch to > > >>> write even if it would look a tad bit awkward IMO. Let me know if > > >>> you want me to post it up. > > >> > > >> I think it would be a good idea. At least it will allow us to > > >> judge, which is the more correct way. > > > > > > I'm still happy to prototype this, but the more I think about it the > > > more it feels like a workaround for the Qualcomm driver. The eDP > > > panel driver is actually given a pointer to the AUX bus at probe > > > time. It's really weird to say that we can't do a transfer on it > > > yet... As you said, this is a little sideband bus. It should be able > > > to be used without all the full blown infra of the rest of the driver. > > > > Yes, I have that feeling too. However I also have a feeling that just > > powering up the PHY before the bus probe is ... a hack. There are no > > obvious stopgaps for the driver not to power it down later. > > This is why I think we need to move to Runtime PM to manage this. Basically: > > 1. When an AUX transfer happens, you grab a PM runtime reference that > _that_ powers up the PHY. > > 2. At the end of the AUX transfer function, you do a "put_autosuspend". > > Then it becomes not a hack, right? > > pm runtime ops needs to be implemented for both eDP and DP. This change take good amount of planning and code changes as it affects DP also. Because this patch series consist of basic eDP changes for SC7280 bootup, shall we take this pm_runtime implementation in subsequent patch series? > > A cleaner design might be to split all hotplug event handling from the > > dp_display, provide a lightweight state machine for the eDP and select > > which state machine to use depending on the hardware connector type. > > The dp_display_bind/unbind would probably also be duplicated and > > receive correct code flows for calling dp_parser_get_next_bridge, etc. > > Basically that means that depending on the device data we'd use either > > dp_display_comp_ops or (new) edp_comp_ops. > > > > WDYT? > > I don't think I know the structure of the MSM DP code to make a definitive > answer here. I think I'd have to see a patch. However I'd agree in general > terms that we need some different flows for the two. > ;-) We definitely want to limit the differences but some of them will be > unavoidable... > > > -Doug Thank you, Sankeerth
Re: [Freedreno] [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp
Hi Dmitry, > > > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti > > > > > wrote: > > > > > > > > > > > > The panel-edp driver modes needs to be validated differently > > > > > > from DP because the link capabilities are not available for EDP by > that time. > > > > > > > > > > > > Signed-off-by: Sankeerth Billakanti > > > > > > > > > > > > > > > > This should not be necessary after > > > > > > > > > https://patchwork.freedesktop.org/patch/479261/?series=101682=1. > > > > > Could you please check? > > > > > > > > > > > > > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but > we > > > > need to return early for eDP because unlike DP, eDP context will > > > > not have the information about the number of lanes and link clock. > > > > > > > > So, I will modify the patch to return after the > > > > DP_MAX_PIXEL_CLK_KHZ > > > check if is_eDP is set. > > > > > > I haven't walked through all the relevant code but something you > > > said above sounds strange. You say that for eDP we don't have info > > > about the number of lanes? We _should_. > > > > > > It's certainly possible to have a panel that supports _either_ 1 or > > > 2 lanes but then only physically connect 1 lane to it. ...or you > > > could have a panel that supports 2 or 4 lanes and you only connect 1 lane. > > > See, for instance, ti_sn_bridge_parse_lanes. There we assume 4 lanes > > > but if a "data-lanes" property is present then we can use that to > > > know that fewer lanes are physically connected. > > > > > > It's also possible to connect more lanes to a panel than it supports. > > > You could connect 2 lanes to it but then it only supports 1. This > > > case needs to be handled as well... > > > > > > > I was referring to the checks we do for DP in dp_bridge_mode_valid. We > > check if the Link bandwidth can support the pixel bandwidth. For an > > external DP connection, the Initial DPCD/EDID read after cable > > connection will return the sink capabilities like link rate, lane > > count and bpp information that are used to we filter out the unsupported > modes from the list of modes from EDID. > > > > For eDP case, the dp driver performs the first dpcd read during > > bridge_enable. The dp_bridge_mode_valid function is executed before > > bridge_enable and hence does not have the full link or the sink > > capabilities information like external DP connection, by then. > > It sounds to me like we should emulate the HPD event for eDP to be handled > earlier than the get_modes()/prepare() calls are attempted. > However this might open another can of worms. > For DP, the HPD handler mainly initiates link training and gets the EDID. Before adding support for a separate eDP panel, we had discussed about this internally and decided to emulate eDP HPD during enable(). Main reason being, eDP power is guaranteed to be on only after bridge_enable(). So, eDP link training can happen and sustain only after bridge_enable(). Emulating HPD before/during get_modes will not have any effect because: 1. get_modes() will go to panel's get_modes() function to power on read EDID 2. panel power will be turned off after get_modes(). Panel power off will reset every write transaction in DPCD. anyway invalidating link training 3. mode_valid will land in dp driver but panel will not be powered on at that time and we cannot do aux transfers or DPCD read writes. > > So, we need to proceed with the reported mode for eDP. > > Well... Even if during the first call to get_modes() the DPCD is not read, > during subsequent calls the driver has necessary information, so it can > proceed with all the checks, can't it? > get_modes() currently does not land in DP driver. It gets executed in panel bridge. But the mode_valid() comes to DP driver to check the controller compatibility. > -- > With best wishes > Dmitry Thank you, Sankeerth
Re: [Freedreno] [PATCH v6 7/8] drm/msm/dp: Support edp/dp without hpd
> > > On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> > > wrote:
> > > >
> > > > Some eDP sinks or platform boards will not support hpd.
> > > > This patch adds support for those cases.
> > >
> > > You could say more, like:
> > >
> > > If we're not using HPD then _both_ the panel node and the eDP
> > > controller node will have "no-hpd". This tells the eDP panel code to
> > > hardcode the maximum possible delay for a panel to power up and
> > > tells the eDP driver that it should continue to do transfers even if HPD
> isn't asserted.
> > >
> >
> > Okay. I will add it
> >
> > >
> > > > Signed-off-by: Sankeerth Billakanti
> > > > ---
> > > > drivers/gpu/drm/msm/dp/dp_catalog.c | 15 ---
> > > > 1 file changed, 12 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > > b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > > index 1809ce2..8f1fc71 100644
> > > > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > > @@ -244,10 +244,17 @@ 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 state;
> > > > + u32 state, hpd_en;
> > > > struct dp_catalog_private *catalog = container_of(dp_catalog,
> > > > struct dp_catalog_private,
> > > > dp_catalog);
> > > >
> > > > + hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL);
> > > > + hpd_en &= DP_DP_HPD_CTRL_HPD_EN;
> > > > +
> > > > + /* no-hpd case */
> > > > + if (!hpd_en)
> > > > + return 0;
> > > > +
> > > > /* poll for hpd connected status every 2ms and timeout after
> 500ms */
> > > > return readl_poll_timeout(catalog->io->dp_controller.aux.base +
> > > > REG_DP_DP_HPD_INT_STATUS, @@
> > > > -586,8
> > > > +593,10 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
> > > *dp_catalog)
> > > > reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> > > > dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> > > >
> > > > - /* Enable HPD */
> > > > - dp_write_aux(catalog, REG_DP_DP_HPD_CTRL,
> > > DP_DP_HPD_CTRL_HPD_EN);
> > > > + /* Enable HPD if supported*/
> > > > + if (!of_property_read_bool(catalog->dev->of_node,
> > > > + "no-hpd"))
> > >
> > > I don't think this is a particularly lightweight operation. It's
> > > literally iterating through all of our device tree properties and doing
> > > string
> compares on them.
> > > ...but this function is called somewhat often, isn't it? It feels
> > > like the kind of thing that should happen at probe time and be stored in a
> boolean.
> > >
> > > ...and then you can use that same boolean in
> > > dp_catalog_aux_wait_for_hpd_connect_state() rather than reading the
> > > register value, right?
> > >
> > It is called twice for DP. Once while booting through a thread
> > scheduled from kms_obj_init and when resuming from PM suspend.
> >
> > With aux_bus addition, this function will be called thrice for eDP.
> > Once during bootup with aux_bus, then from scheduled event from
> kms_obj_init and pm resume like DP.
> >
> > I will check if we can use a no-hpd Boolean flag instead.
>
> As the driver has a separate dp_parser code, it might be a good fit to parse
> the DT once and then to use boolean flag afterwards.
>
Okay. Will add it.
> --
> With best wishes
> Dmitry
Re: [Freedreno] [PATCH v6 6/8] drm/msm/dp: remove unnecessary delay during boot
Hi Dmitry, > > > On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti > > > wrote: > > > > > > > > Remove the unnecessary delay in executing the EV_HPD_INIT_SETUP > > > event. > > > > > > Tell me more and put it in the commit message! Why did it used to be > > > necessary and why is it no longer necessary? Inquiring minds want to > know. > > > > > > > Okay. I will add proper description. The DP phy is shared with usb and > > executing the dp phy_init before the usb phy_init was causing usb devices > to not function. > > I always wondered, how does this work for the 4-lane DP dongles, where > there is no USB mode/lanes? > For 4 lane DP, the DP driver overrides the phy programming. For 2 lanes DP, the usb phy programming is leveraged. I believe the usb controller needs to enable both usb3 (qmp phy) and usb2 (hs phy) before DP phy_init. If one of the usb phy init fails, the usb controller will not enumerate and will cause usb2 devices (like keyboard or mouse) to not work. The usb controller probe was failing because DP was turning on (voting) a clock which was supposed to be initialized by the usb driver. We did not see any issue with DP because the programming anyway gets overwritten but due to usb controller failure, usb devices did not work. > > Earlier, enabling phy_init was done when the EV_HPD_INIT_SETUP event > was executed. > > So, we had scheduled it to execute after 10 second to let the usb driver > complete the phy_init first. > > > > Kuogee made the below change to move the DP phy_init to execute after > > the DP is connected > > https://patchwork.kernel.org/project/linux-arm-msm/patch/1642531648- > 84 > > 48-2-git-send-email-quic_khs...@quicinc.com/ > > > > So, there is no need for the DP driver to wait 10 seconds for the phy > initialization anymore. > > > > eDP PHY is not shared with usb. So, it can be programmed anytime, hence > not needing any delay. > > > > -- > With best wishes > Dmitry
Re: [Freedreno] [PATCH v6 7/8] drm/msm/dp: Support edp/dp without hpd
Hi Doug,
> On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> wrote:
> >
> > Some eDP sinks or platform boards will not support hpd.
> > This patch adds support for those cases.
>
> You could say more, like:
>
> If we're not using HPD then _both_ the panel node and the eDP controller
> node will have "no-hpd". This tells the eDP panel code to hardcode the
> maximum possible delay for a panel to power up and tells the eDP driver that
> it should continue to do transfers even if HPD isn't asserted.
>
Okay. I will add it
>
> > Signed-off-by: Sankeerth Billakanti
> > ---
> > drivers/gpu/drm/msm/dp/dp_catalog.c | 15 ---
> > 1 file changed, 12 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > index 1809ce2..8f1fc71 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > @@ -244,10 +244,17 @@ 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 state;
> > + u32 state, hpd_en;
> > struct dp_catalog_private *catalog = container_of(dp_catalog,
> > struct dp_catalog_private,
> > dp_catalog);
> >
> > + hpd_en = dp_read_aux(catalog, REG_DP_DP_HPD_CTRL);
> > + hpd_en &= DP_DP_HPD_CTRL_HPD_EN;
> > +
> > + /* no-hpd case */
> > + if (!hpd_en)
> > + return 0;
> > +
> > /* poll for hpd connected status every 2ms and timeout after 500ms
> > */
> > return readl_poll_timeout(catalog->io->dp_controller.aux.base +
> > REG_DP_DP_HPD_INT_STATUS, @@ -586,8
> > +593,10 @@ void dp_catalog_ctrl_hpd_config(struct dp_catalog
> *dp_catalog)
> > reftimer |= DP_DP_HPD_REFTIMER_ENABLE;
> > dp_write_aux(catalog, REG_DP_DP_HPD_REFTIMER, reftimer);
> >
> > - /* Enable HPD */
> > - dp_write_aux(catalog, REG_DP_DP_HPD_CTRL,
> DP_DP_HPD_CTRL_HPD_EN);
> > + /* Enable HPD if supported*/
> > + if (!of_property_read_bool(catalog->dev->of_node, "no-hpd"))
>
> I don't think this is a particularly lightweight operation. It's literally
> iterating
> through all of our device tree properties and doing string compares on them.
> ...but this function is called somewhat often, isn't it? It feels like the
> kind of
> thing that should happen at probe time and be stored in a boolean.
>
> ...and then you can use that same boolean in
> dp_catalog_aux_wait_for_hpd_connect_state() rather than reading the
> register value, right?
>
It is called twice for DP. Once while booting through a thread scheduled from
kms_obj_init
and when resuming from PM suspend.
With aux_bus addition, this function will be called thrice for eDP. Once during
bootup with
aux_bus, then from scheduled event from kms_obj_init and pm resume like DP.
I will check if we can use a no-hpd Boolean flag instead.
>
> -Doug
Thank you,
Sankeerth
Re: [Freedreno] [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp
Hi Doug, > On Wed, Mar 30, 2022 at 11:02 PM Sankeerth Billakanti (QUIC) > wrote: > > > > Hi Dmitry, > > > > > On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti > > > wrote: > > > > > > > > The panel-edp driver modes needs to be validated differently from > > > > DP because the link capabilities are not available for EDP by that time. > > > > > > > > Signed-off-by: Sankeerth Billakanti > > > > > > This should not be necessary after > > > > https://patchwork.freedesktop.org/patch/479261/?series=101682=1. > > > Could you please check? > > > > > > > The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but we > > need to return early for eDP because unlike DP, eDP context will not > > have the information about the number of lanes and link clock. > > > > So, I will modify the patch to return after the DP_MAX_PIXEL_CLK_KHZ > check if is_eDP is set. > > I haven't walked through all the relevant code but something you said above > sounds strange. You say that for eDP we don't have info about the number > of lanes? We _should_. > > It's certainly possible to have a panel that supports _either_ 1 or 2 lanes > but > then only physically connect 1 lane to it. ...or you could have a panel that > supports 2 or 4 lanes and you only connect 1 lane. > See, for instance, ti_sn_bridge_parse_lanes. There we assume 4 lanes but if > a "data-lanes" property is present then we can use that to know that fewer > lanes are physically connected. > > It's also possible to connect more lanes to a panel than it supports. > You could connect 2 lanes to it but then it only supports 1. This case needs > to > be handled as well... > I was referring to the checks we do for DP in dp_bridge_mode_valid. We check if the Link bandwidth can support the pixel bandwidth. For an external DP connection, the Initial DPCD/EDID read after cable connection will return the sink capabilities like link rate, lane count and bpp information that are used to we filter out the unsupported modes from the list of modes from EDID. For eDP case, the dp driver performs the first dpcd read during bridge_enable. The dp_bridge_mode_valid function is executed before bridge_enable and hence does not have the full link or the sink capabilities information like external DP connection, by then. So, we need to proceed with the reported mode for eDP. > > -Doug
Re: [Freedreno] [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
> On Thu, 31 Mar 2022 at 14:05, Sankeerth Billakanti
> wrote:
> >
> > Hi Dmitry,
> >
> > > On 31/03/2022 08:53, Sankeerth Billakanti (QUIC) wrote:
> > > > Hi Dmitry,
> > > >
> > > >> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> > > >> wrote:
> > > >>>
> > > >>> The interrupt register will still reflect the connect and
> > > >>> disconnect interrupt status without generating an actual HW
> interrupt.
> > > >>> The controller driver should not handle those masked interrupts.
> > > >>>
> > > >>> Signed-off-by: Sankeerth Billakanti
> > > >>> ---
> > > >>> drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
> > > >>> 1 file changed, 3 insertions(+), 2 deletions(-)
> > > >>>
> > > >>> diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> index 3c16f95..1809ce2 100644
> > > >>> --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > > >>> @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct
> > > >>> dp_catalog *dp_catalog) {
> > > >>> struct dp_catalog_private *catalog = container_of(dp_catalog,
> > > >>> struct dp_catalog_private,
> > > >>> dp_catalog);
> > > >>> - int isr = 0;
> > > >>> + int isr, mask;
> > > >>>
> > > >>> isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> > > >>> dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> > > >>> (isr & DP_DP_HPD_INT_MASK));
> > > >>> + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
> > > >>>
> > > >>> - return isr;
> > > >>> + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask);
> > > >>
> > > >> I suspect that the logic is inverted here. Shouldn't it be:
> > > >>
> > > >> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
> > > >>
> > > >> ?
> > > >>
> > > >
> > > > The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE000 and the
> > > value
> > > > of the read interrupt mask variable could be is 0xF.
> > > >
> > > > The mask value is indicated via the register,
> > > > REG_DP_DP_HPD_INT_MASK,
> > > bits 3:0.
> > > > The HPD status is indicated via a different read-only register
> > > REG_DP_DP_HPD_INT_STATUS, bits 31:29.
> > >
> > > I see. Maybe the following expression would be better?
> > >
> > > return isr & (mask & ~DP_DP_HPD_INT_MASK);
>
> Ugh, excuse me please. I meant:
>
> return isr & (mask | ~DP_DP_HPD_INT_MASK);
>
Okay. I will change it.
> > >
> >
> > I believe the confusion occurred because the
> > DP_DP_HPD_STATE_STATUS_CONNECTED and others were defined under
> the same register definition as REG_DP_DP_HPD_INT_MASK I will rearrange
> the definitions below.
> >
> > #define REG_DP_DP_HPD_INT_MASK (0x000C)
> > #define DP_DP_HPD_PLUG_INT_MASK (0x0001)
> > #define DP_DP_IRQ_HPD_INT_MASK (0x0002)
> > #define DP_DP_HPD_REPLUG_INT_MASK (0x0004)
> > #define DP_DP_HPD_UNPLUG_INT_MASK (0x0008)
> > #define DP_DP_HPD_INT_MASK (DP_DP_HPD_PLUG_INT_MASK | \
> > DP_DP_IRQ_HPD_INT_MASK | \
> > DP_DP_HPD_REPLUG_INT_MASK |
> > \
> >
> > DP_DP_HPD_UNPLUG_INT_MASK)
> >
> > Below are status bits from register REG_DP_DP_HPD_INT_STATUS
> >
> > #define DP_DP_HPD_STATE_STATUS_CONNECTED(0x4000)
> > #define DP_DP_HPD_STATE_STATUS_PENDING (0x2000)
> > #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x)
> > #define DP_DP_HPD_STATE_STATUS_MASK (0xE000)
> >
> > DP_DP_HPD_INT_MASK is 0xF and scope of mask variable is also 0xF (bits
> 3:0), mask & ~DP_DP_HPD_INT_MASK is 0 always.
> >
> > For DP, we want to enable all interrupts.
> > So the programmed mask value is 0xF. We want to return 0x4001 for
> > connect and 8 for disconnect
> >
> > For eDP, we want to disable the connect and disconnect interrupts. So,
> > the mask will be 0x6 (i.e. DP_DP_IRQ_HPD_INT_MASK |
> DP_DP_HPD_REPLUG_INT_MASK) We want to return 0x4000 (or
> 0x2000 based on hpd line status) and 0 for eDP connect and disconnect
> respectively.
> >
> > > >
> > > > isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
> > > >
> > > >>> }
> > > >>>
> > > >>> int dp_catalog_ctrl_get_interrupt(struct dp_catalog
> > > >>> *dp_catalog)
> > > >>> --
> > > >>> 2.7.4
> > > >>>
> > > >>
> > > >>
> > > >> --
> > > >> With best wishes
> > > >> Dmitry
> > > >
> > > > Thank you,
> > > > Sankeerth
> > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
> >
> > Thank you,
> > Sankeerth
>
>
>
> --
> With best wishes
> Dmitry
Re: [Freedreno] [PATCH v6 6/8] drm/msm/dp: remove unnecessary delay during boot
Hi Doug, > On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti > wrote: > > > > Remove the unnecessary delay in executing the EV_HPD_INIT_SETUP > event. > > Tell me more and put it in the commit message! Why did it used to be > necessary and why is it no longer necessary? Inquiring minds want to know. > Okay. I will add proper description. The DP phy is shared with usb and executing the dp phy_init before the usb phy_init was causing usb devices to not function. Earlier, enabling phy_init was done when the EV_HPD_INIT_SETUP event was executed. So, we had scheduled it to execute after 10 second to let the usb driver complete the phy_init first. Kuogee made the below change to move the DP phy_init to execute after the DP is connected https://patchwork.kernel.org/project/linux-arm-msm/patch/1642531648-8448-2-git-send-email-quic_khs...@quicinc.com/ So, there is no need for the DP driver to wait 10 seconds for the phy initialization anymore. eDP PHY is not shared with usb. So, it can be programmed anytime, hence not needing any delay. > -Doug Thank you, Sankeerth
Re: [Freedreno] [PATCH v6 3/8] drm/msm/dp: Support only IRQ_HPD and REPLUG interrupts for eDP
Hi Doug, > On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti > wrote: > > > > @@ -1374,6 +1382,12 @@ static int dp_pm_resume(struct device *dev) > > dp_catalog_ctrl_hpd_config(dp->catalog); > > > > > > + if (dp->dp_display.connector_type == > DRM_MODE_CONNECTOR_DisplayPort) > > + dp_catalog_hpd_config_intr(dp->catalog, > > + DP_DP_HPD_PLUG_INT_MASK | > > + DP_DP_HPD_UNPLUG_INT_MASK, > > + true); > > + > > nit: why are there two blank lines above? > > Will remove a blank line. > > @@ -1639,6 +1653,9 @@ void dp_bridge_enable(struct drm_bridge > *drm_bridge) > > return; > > } > > > > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > > + dp_hpd_plug_handle(dp_display, 0); > > + > > Should you add a "pre_enable" and do it there? That would make it more > symmetric with the fact that you have the countertpart in "post_disable". > We want to handle the screen off or eDP disable like a cable unplug so that it can be integrated into the dp code. So, we can move plug_handle into pre_enable and the unplug_handle to pre_disable. If we do unplug in post_disable, we need to handle the state changes also. It will complicate the driver code. > > Overall: I'm probably not familiar enough with this code to give it a full > review. I'm hoping that Dmitry knows it well enough... ;-) > > > -Doug Thank you, Sankeerth
Re: [Freedreno] [PATCH v6 2/8] drm/msm/dp: wait for hpd high before aux transaction
Hi Doug,
> On Wed, Mar 30, 2022 at 9:03 AM Sankeerth Billakanti
> wrote:
> >
> > The source device should ensure the sink is ready before proceeding to
> > read the sink capability or performing any aux transactions. The sink
>
> s/performing/perform
>
> > will indicate its readiness by asserting the HPD line. The controller
> > driver needs to wait for the hpd line to be asserted by the sink
> > before performing any aux transactions.
> >
> > The eDP sink is assumed to be always connected. It needs power from
> > the source and its HPD line will be asserted only after the panel is
> > powered on. The panel power will be enabled from the panel-edp driver
> > and only after that, the hpd line will be asserted.
> >
> > Whereas for DP, the sink can be hotplugged and unplugged anytime. The
> > hpd line gets asserted to indicate the sink is connected and ready.
> > Hence there is no need to wait for the hpd line to be asserted for a DP
> > sink.
> >
> > Signed-off-by: Sankeerth Billakanti
> > ---
> >
> > Changes in v6:
> > - Wait for hpd high only for eDP
> > - Split into smaller patches
> >
> > drivers/gpu/drm/msm/dp/dp_aux.c | 13 -
> > drivers/gpu/drm/msm/dp/dp_aux.h | 3 ++-
> > drivers/gpu/drm/msm/dp/dp_catalog.c | 13 +
> > drivers/gpu/drm/msm/dp/dp_catalog.h | 1 +
> > drivers/gpu/drm/msm/dp/dp_display.c | 3 ++-
> > 5 files changed, 30 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.c
> > b/drivers/gpu/drm/msm/dp/dp_aux.c index 6d36f63..a217c80 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_aux.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_aux.c
> > @@ -36,6 +36,7 @@ struct dp_aux_private {
> > bool initted;
> > u32 offset;
> > u32 segment;
> > + bool is_edp;
> >
> > struct drm_dp_aux dp_aux;
> > };
> > @@ -337,6 +338,14 @@ static ssize_t dp_aux_transfer(struct drm_dp_aux
> *dp_aux,
> > goto exit;
> > }
> >
> > + if (aux->is_edp) {
>
> Adding a comment about _why_ you're doing this just for eDP would
> probably be a good idea. Like maybe:
>
> /*
> * 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.
> */
>
>
Okay. Will add it
> > @@ -491,7 +500,8 @@ void dp_aux_unregister(struct drm_dp_aux
> *dp_aux)
> > drm_dp_aux_unregister(dp_aux); }
> >
> > -struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog
> > *catalog)
> > +struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog
> *catalog,
> > + bool is_edp)
>
> nit: I think your indentation of the 2nd line isn't quite right.
>
>
I moved bool is_edp into the next line. In vim , it was sowing fine. I'll check
> > diff --git a/drivers/gpu/drm/msm/dp/dp_aux.h
> > b/drivers/gpu/drm/msm/dp/dp_aux.h index 82afc8d..c99aeec 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_aux.h
> > +++ b/drivers/gpu/drm/msm/dp/dp_aux.h
> > @@ -16,7 +16,8 @@ void dp_aux_init(struct drm_dp_aux *dp_aux); void
> > dp_aux_deinit(struct drm_dp_aux *dp_aux); void dp_aux_reconfig(struct
> > drm_dp_aux *dp_aux);
> >
> > -struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog
> > *catalog);
> > +struct drm_dp_aux *dp_aux_get(struct device *dev, struct dp_catalog
> *catalog,
> > + bool is_edp);
>
> nit: I think your indentation of the 2nd line isn't quite right.
>
I'll check
>
> Things are pretty much nits, so FWIW:
>
> Reviewed-by: Douglas Anderson
Thank you,
Sankeerth
Re: [Freedreno] [PATCH v6 8/8] drm/msm/dp: Handle eDP mode_valid differently from dp
Hi Dmitry,
> On Wed, 30 Mar 2022 at 19:04, Sankeerth Billakanti
> wrote:
> >
> > The panel-edp driver modes needs to be validated differently from DP
> > because the link capabilities are not available for EDP by that time.
> >
> > Signed-off-by: Sankeerth Billakanti
>
> This should not be necessary after
> https://patchwork.freedesktop.org/patch/479261/?series=101682=1.
> Could you please check?
>
The check for DP_MAX_PIXEL_CLK_KHZ is not necessary anymore but we need
to return early for eDP because unlike DP, eDP context will not have the
information
about the number of lanes and link clock.
So, I will modify the patch to return after the DP_MAX_PIXEL_CLK_KHZ check if
is_eDP is set.
> > ---
> > drivers/gpu/drm/msm/dp/dp_display.c | 6 ++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 8bafdd0..f9c7d9a 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -1003,6 +1003,12 @@ enum drm_mode_status
> dp_bridge_mode_valid(struct drm_bridge *bridge,
> > return -EINVAL;
> > }
> >
> > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) {
> > + if (mode_pclk_khz > DP_MAX_PIXEL_CLK_KHZ)
> > + return MODE_CLOCK_HIGH;
> > + return MODE_OK;
> > + }
> > +
> > if ((dp->max_pclk_khz <= 0) ||
> > (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
> > (mode->clock > dp->max_pclk_khz))
> > --
> > 2.7.4
> >
>
>
> --
> With best wishes
> Dmitry
Re: [Freedreno] [PATCH v6 4/8] drm/msm/dp: avoid handling masked interrupts
Hi Dmitry,
> On Wed, 30 Mar 2022 at 19:03, Sankeerth Billakanti
> wrote:
> >
> > The interrupt register will still reflect the connect and disconnect
> > interrupt status without generating an actual HW interrupt.
> > The controller driver should not handle those masked interrupts.
> >
> > Signed-off-by: Sankeerth Billakanti
> > ---
> > drivers/gpu/drm/msm/dp/dp_catalog.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > index 3c16f95..1809ce2 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_catalog.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_catalog.c
> > @@ -608,13 +608,14 @@ u32 dp_catalog_hpd_get_intr_status(struct
> > dp_catalog *dp_catalog) {
> > struct dp_catalog_private *catalog = container_of(dp_catalog,
> > struct dp_catalog_private, dp_catalog);
> > - int isr = 0;
> > + int isr, mask;
> >
> > isr = dp_read_aux(catalog, REG_DP_DP_HPD_INT_STATUS);
> > dp_write_aux(catalog, REG_DP_DP_HPD_INT_ACK,
> > (isr & DP_DP_HPD_INT_MASK));
> > + mask = dp_read_aux(catalog, REG_DP_DP_HPD_INT_MASK);
> >
> > - return isr;
> > + return isr & (DP_DP_HPD_STATE_STATUS_MASK | mask);
>
> I suspect that the logic is inverted here. Shouldn't it be:
>
> return isr & DP_DP_HPD_STATE_STATUS_MASK & mask;
>
> ?
>
The value of DP_DP_HPD_STATE_STATUS_MASK is 0xE000 and the value of the read
interrupt mask variable could be is 0xF.
The mask value is indicated via the register, REG_DP_DP_HPD_INT_MASK, bits 3:0.
The HPD status is indicated via a different read-only register
REG_DP_DP_HPD_INT_STATUS, bits 31:29.
isr & DP_DP_HPD_STATE_STATUS_MASK & mask, will return 0 always.
> > }
> >
> > int dp_catalog_ctrl_get_interrupt(struct dp_catalog *dp_catalog)
> > --
> > 2.7.4
> >
>
>
> --
> With best wishes
> Dmitry
Thank you,
Sankeerth
Re: [Freedreno] [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink interaction
> -Original Message- > From: Doug Anderson > Sent: Saturday, March 19, 2022 5:26 AM > To: Stephen Boyd > Cc: Sankeerth Billakanti (QUIC) ; open list:OPEN > FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > ; dri-devel ; > freedreno ; linux-arm-msm m...@vger.kernel.org>; LKML ; Rob Clark > ; Sean Paul ; > quic_kalyant ; Abhinav Kumar (QUIC) > ; Kuogee Hsieh (QUIC) > ; Andy Gross ; > bjorn.anders...@linaro.org; Rob Herring ; > krzk...@kernel.org; Sean Paul ; David Airlie > ; Daniel Vetter ; Thierry Reding > ; Sam Ravnborg ; > dmitry.barysh...@linaro.org; quic_vproddut > Subject: Re: [PATCH v5 6/9] drm/msm/dp: wait for hpd high before any sink > interaction > > Hi, > > On Fri, Mar 18, 2022 at 4:27 PM Stephen Boyd > wrote: > > > > > > Pushing hpd state checking into aux transactions looks like the > > > > wrong direction. Also, as I said up above I am concerned that even > > > > checking the GPIO won't work and we need some way to ask the > > > > bridge if HPD is asserted or not and then fallback to the GPIO > > > > method if the display phy/controller doesn't have support to check > > > > HPD internally. Something on top of DRM_BRIDGE_OP_HPD? > > > > > > If we could somehow get the HPD status from the bridge in the panel > > > driver it definitely would be convenient. It does feel like that's > > > an improvement that could be done later, though. We've already > > > landed a few instances of doing what's done here, like for > > > parade-ps8640 and analogix_dp. I suspect designing a new mechanism > > > might not be the most trivial. > > > > What is done in the bridge drivers is to wait for a fixed timeout and > > assume aux is ready? Or is it something else? If there's just a fixed > > timeout for the eDP case it sounds OK to do that for now and we can > > fine tune it later to actually check HPD status register before the > > panel tries to read EDID. > > Right. For the parade chip (which is only used for eDP as far as I know--never > DP) waits for up to 200 ms. See ps8640_ensure_hpd(). > > So I guess tl;dr to Sankeerth that it's OK for his patch to have the wait in > the > aux transfer function, but only for eDP. Other discussions here are about > how we could make it better in future patches. > > The aux transactions for external DP are initiated by the dp_display driver only after the display is hot plugged to the connector. The phy_init is necessary for the aux transactions to take place. So, for the DP case, like Doug mentioned below, this patch is introducing an overhead of three register reads to detect hpd_high before performing aux transactions. So, we felt this was okay to do for DP. On the other hand, for eDP, it is necessary to wait for panel ready through this hpd connect status. Currently there is no way to know which type of connector it is in the dp_aux sub-module. However, as the discussion suggested, to have the wait only for eDP, I am thinking to pass the connector_type information to aux sub-module and register different aux_transfer functions for eDP and DP. The eDP transfer function will wait for hpd_high and the DP transfer function will be same as the one before this patch. What do you think? > > > I haven't actually tried it, but I suspect that to get something > > > like what you're talking about we'd have to get the rest of drm to > > > know that for eDP ports that it should assume something is connected > > > _regardless_ of what the "detect" / "HPD" options say. Then we'd > > > have to extend the edp-panel code to be able to be able to query the > > > next bridge in the chain if a GPIO wasn't provided. > > > > Can the panel interrogate the bridge chain somehow? It feels like > > either something in the chain should know the status of HPD (the case > > here where display controller in the SoC knows) or it should be a gpio > > to the panel (trogdor case). The bridge ops can implement > > DRM_BRIDGE_OP_HPD and the first bridge from the encoder that supports > > HPD can implement some sort of "wait for hpd asserted" function that > > the panel then uses once it powers up the panel during probe. If the > > panel has a gpio and nothing else in the chain can detect hpd then the > > panel polls the gpio, or it waits for the amount of time delay after > > powering on the panel if the panel's hpd function is called. > > Yeah, there ought to be some way to make something like that work. I don't > think it's just DRM_BRIDGE_OP_HPD, though, for a few reasons: > > 1. That operation actually specifically means that HPD c
Re: [Freedreno] [PATCH v5 5/9] drm/msm/dp: Add eDP support via aux_bus
> -Original Message-
> From: Stephen Boyd
> Sent: Friday, March 18, 2022 3:08 AM
> To: Sankeerth Billakanti (QUIC) ;
> devicet...@vger.kernel.org; dri-de...@lists.freedesktop.org;
> freedreno@lists.freedesktop.org; linux-arm-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Cc: robdcl...@gmail.com; seanp...@chromium.org; quic_kalyant
> ; Abhinav Kumar (QUIC)
> ; diand...@chromium.org; Kuogee Hsieh
> (QUIC) ; agr...@kernel.org;
> bjorn.anders...@linaro.org; robh...@kernel.org; krzk...@kernel.org;
> s...@poorly.run; airl...@linux.ie; dan...@ffwll.ch;
> thierry.red...@gmail.com; s...@ravnborg.org;
> dmitry.barysh...@linaro.org; quic_vproddut
> Subject: Re: [PATCH v5 5/9] drm/msm/dp: Add eDP support via aux_bus
>
> Quoting Sankeerth Billakanti (2022-03-16 10:35:50)
> > This patch adds support for generic eDP sink through aux_bus.
>
> Please unindent commit text paragraphs. This isn't a book.
>
Okay. Will change it.
> > The eDP/DP controller driver should support aux transactions
> > originating from the panel-edp driver and hence should be initialized and
> ready.
> >
> > The panel bridge supporting the panel should be ready before
> > the bridge connector is initialized. The generic panel probe needs the
> > controller resources to be enabled to support aux tractions
> > originating
>
> s/tractions/transactions/
>
Will correct it
> > from it. So, the host_init and phy_init are moved to execute before
> > the panel probe.
> >
> > The host_init has to return early if the core is already
> > initialized so that the regulator and clock votes for the controller
> > resources are balanced.
> >
> > EV_HPD_INIT_SETUP needs to execute immediately to enable the
> > interrupts for the aux transactions from panel-edp to get the modes
> > supported.
>
> There are a lot of things going on in this patch. Can it be split up?
>
I can split them up.
> >
> > Signed-off-by: Sankeerth Billakanti
> > ---
> > drivers/gpu/drm/msm/dp/dp_display.c | 65
> +++--
> > drivers/gpu/drm/msm/dp/dp_drm.c | 10 +++---
> > drivers/gpu/drm/msm/dp/dp_parser.c | 21 +---
> > drivers/gpu/drm/msm/dp/dp_parser.h | 1 +
> > 4 files changed, 70 insertions(+), 27 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> > b/drivers/gpu/drm/msm/dp/dp_display.c
> > index 382b3aa..688bbed 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -10,6 +10,7 @@
> > #include
> > #include
> > #include
> > +#include
> >
> > #include "msm_drv.h"
> > #include "msm_kms.h"
> > @@ -265,8 +266,6 @@ static int dp_display_bind(struct device *dev, struct
> device *master,
> > goto end;
> > }
> >
> > - dp->dp_display.next_bridge = dp->parser->next_bridge;
> > -
> > dp->aux->drm_dev = drm;
> > rc = dp_aux_register(dp->aux);
> > if (rc) {
> > @@ -421,6 +420,11 @@ static void dp_display_host_init(struct
> dp_display_private *dp)
> > dp->dp_display.connector_type, dp->core_initialized,
> > dp->phy_initialized);
> >
> > + if (dp->core_initialized) {
> > + DRM_DEBUG_DP("DP core already initialized\n");
> > + return;
> > + }
> > +
> > dp_power_init(dp->power, false);
> > dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
> > dp_aux_init(dp->aux);
> > @@ -433,6 +437,11 @@ static void dp_display_host_deinit(struct
> dp_display_private *dp)
> > dp->dp_display.connector_type, dp->core_initialized,
> > dp->phy_initialized);
> >
> > + if (!dp->core_initialized) {
> > + DRM_DEBUG_DP("DP core not initialized\n");
> > + return;
> > + }
> > +
> > dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
> > dp_aux_deinit(dp->aux);
> > dp_power_deinit(dp->power);
> > @@ -1502,7 +1511,7 @@ void msm_dp_irq_postinstall(struct msm_dp
> > *dp_display)
> >
> > dp_hpd_event_setup(dp);
> >
> > - dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 100);
> > + dp_add_event(dp, EV_HPD_INIT_SETUP, 0, 0);
> > }
> >
> > void msm_dp_debugfs_init(struct msm_dp *dp_display, struct drm_minor
> > *minor) @@ -1524,6
Re: [Freedreno] [PATCH v5 2/9] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD
> -Original Message-
> From: Doug Anderson
> Sent: Friday, March 18, 2022 10:51 PM
> To: Sankeerth Billakanti (QUIC)
> Cc: dri-devel ; linux-arm-msm m...@vger.kernel.org>; freedreno ;
> LKML ; open list:OPEN FIRMWARE AND
> FLATTENED DEVICE TREE BINDINGS ; Rob Clark
> ; Sean Paul ; Stephen
> Boyd ; quic_kalyant ;
> Abhinav Kumar (QUIC) ; Kuogee Hsieh (QUIC)
> ; Andy Gross ;
> bjorn.anders...@linaro.org; Rob Herring ;
> krzk...@kernel.org; Sean Paul ; David Airlie
> ; Daniel Vetter ; Thierry Reding
> ; Sam Ravnborg ;
> dmitry.barysh...@linaro.org; quic_vproddut
> Subject: Re: [PATCH v5 2/9] arm64: dts: qcom: sc7280: Add support for eDP
> panel on CRD
>
> Hi,
>
> On Wed, Mar 16, 2022 at 10:36 AM Sankeerth Billakanti
> wrote:
> >
> > Enable support for eDP interface via aux_bus on CRD platform.
> >
> > Signed-off-by: Sankeerth Billakanti
> > ---
> >
> > Changes in v5:
> > - Change the order of patches
> > - Remove the backlight nodes
> > - Remove the bias setting
> > - Fix compilation issue
> > - Model VREG_EDP_BP for backlight power
> >
> > Changes in v4:
> > - Create new patch for name changes
> > - Remove output-low
> >
> > Changes in v3:
> > - Sort the nodes alphabetically
> > - Use - instead of _ as node names
> > - Place the backlight and panel nodes under root
> > - Change the name of edp_out to mdss_edp_out
> > - Change the names of regulator nodes
> > - Delete unused properties in the board file
> >
> >
> > Changes in v2:
> > - Sort node references alphabetically
> > - Improve readability
> > - Move the pwm pinctrl to pwm node
> > - Move the regulators to root
> > - Define backlight power
> > - Remove dummy regulator node
> > - Cleanup pinctrl definitions
> >
> > arch/arm64/boot/dts/qcom/sc7280-crd.dts | 93
> > +
> > 1 file changed, 93 insertions(+)
>
> At a high level, I'd expect your patch to be based upon Matthias's series, AKA
> the 4 patches from:
>
> https://lore.kernel.org/r/20220316172814.v1.1.I2deda8f2cd6adfbb525a97d8f
> ee008a8477b7b0e@changeid/
>
> I'll leave it up to you about whether you care to support eDP on the old
> CRD1/2 or just on CRD3. Personally I'd think CRD3 would be enough.
>
> Then, I'd expect your patch to mostly incorporate
> <https://crrev.com/c/3379844>, though that patch was written before aux-
> bus support so the panel would need to go in a different place.
>
> Stephen already gave some comments and basing on Matthias's patches will
> be a pretty big change, so I probably won't comment lots more.
>
>
I rebased my change on top of Matthias's changes now. We are discussing about
the qcard changes internally to understand the way ahead.
I believe all my current changes are localized to the crd-r3 files only for the
qyalcomm crd3.1
I want to have a different series for c and dt changes to expedite review
process. May I separate the c changes from this series?
> > +_edp {
> > + status = "okay";
> > +
> > + data-lanes = <0 1 2 3>;
> > + vdda-1p2-supply = <_l6b_1p2>;
> > + vdda-0p9-supply = <_l10c_0p8>;
> > +
> > + aux-bus {
> > + edp_panel: edp-panel {
>
> As Stephen pointed out, it should be called "panel".
Okay. Will make that change
Re: [Freedreno] [PATCH v5 3/9] arm64: dts: qcom: sc7280: Enable backlight for eDP panel
> -Original Message-
> From: Stephen Boyd
> Sent: Friday, March 18, 2022 2:58 AM
> To: Sankeerth Billakanti (QUIC) ;
> devicet...@vger.kernel.org; dri-de...@lists.freedesktop.org;
> freedreno@lists.freedesktop.org; linux-arm-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Cc: robdcl...@gmail.com; seanp...@chromium.org; quic_kalyant
> ; Abhinav Kumar (QUIC)
> ; diand...@chromium.org; Kuogee Hsieh
> (QUIC) ; agr...@kernel.org;
> bjorn.anders...@linaro.org; robh...@kernel.org; krzk...@kernel.org;
> s...@poorly.run; airl...@linux.ie; dan...@ffwll.ch;
> thierry.red...@gmail.com; s...@ravnborg.org;
> dmitry.barysh...@linaro.org; quic_vproddut
> Subject: Re: [PATCH v5 3/9] arm64: dts: qcom: sc7280: Enable backlight for
> eDP panel
>
> Quoting Sankeerth Billakanti (2022-03-16 10:35:48)
> > Enable backlight support for eDP panel on CRD platform for sc7280.
> >
> > Signed-off-by: Sankeerth Billakanti
> > ---
> >
> > Changes in v5:
> > - Separate out backlight nodes
> >
> > arch/arm64/boot/dts/qcom/sc7280-crd.dts | 18 ++
> > 1 file changed, 18 insertions(+)
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > index 2df654e..16d1a5b 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > @@ -37,6 +37,15 @@
> > pinctrl-0 = <_panel_power>;
> > };
> >
> > + edp_backlight: edp-backlight {
>
> Does this also move to qcard.dtsi? Why can't this be combined with the
> previous patch?
>
The nodes related to pwm are dependent on
https://patchwork.kernel.org/project/linux-arm-msm/list/?series=620127=*
We moved them to different patch so that the other patch can be merged without
depending on above series. I will rearrange to get backlight definitions also
here.
> > + compatible = "pwm-backlight";
> > +
> > + power-supply = <_edp_bp>;
> > + pwms = <_pwm 3 65535>;
> > +
> > + enable-gpios = <_gpios 7 GPIO_ACTIVE_HIGH>;
> > + };
> > +
> > vreg_edp_bp: vreg-edp-bp-regulator {
> > compatible = "regulator-fixed";
> > regulator-name = "vreg_edp_bp"; @@ -123,7 +132,9 @@
> > ap_ts_pen_1v8: {
> > edp_panel: edp-panel {
> > compatible = "edp-panel";
> >
> > + backlight = <_backlight>;
> > power-supply = <_3v3_regulator>;
> > +
>
> Nitpick: Remove this newline from this hunk and put it in when power-supply
> is introduced.
>
Okay, will make that change.
> > ports {
> > #address-cells = <1>;
> > #size-cells = <0>; @@ -172,6 +183,13
> > @@ ap_ts_pen_1v8: {
> > };
> > };
> >
> > +_pwm {
> > + status = "okay";
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <_bl_pwm>;
>
> I see the pinctrl is used now but it would be easier to review this patch if
> the
> pinctrl was in this patch.
Okay. I will rearrange the hunks from this and the previous patch.
Re: [Freedreno] [PATCH v5 2/9] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD
> -Original Message-
> From: Stephen Boyd
> Sent: Friday, March 18, 2022 2:53 AM
> To: Sankeerth Billakanti (QUIC) ;
> devicet...@vger.kernel.org; dri-de...@lists.freedesktop.org;
> freedreno@lists.freedesktop.org; linux-arm-...@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Cc: robdcl...@gmail.com; seanp...@chromium.org; quic_kalyant
> ; Abhinav Kumar (QUIC)
> ; diand...@chromium.org; Kuogee Hsieh
> (QUIC) ; agr...@kernel.org;
> bjorn.anders...@linaro.org; robh...@kernel.org; krzk...@kernel.org;
> s...@poorly.run; airl...@linux.ie; dan...@ffwll.ch;
> thierry.red...@gmail.com; s...@ravnborg.org;
> dmitry.barysh...@linaro.org; quic_vproddut
> Subject: Re: [PATCH v5 2/9] arm64: dts: qcom: sc7280: Add support for eDP
> panel on CRD
>
> Quoting Sankeerth Billakanti (2022-03-16 10:35:47)
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > index e2efbdd..2df654e 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > @@ -7,6 +7,7 @@
> >
> > /dts-v1/;
> >
> > +#include
> > #include "sc7280-idp.dtsi"
> > #include "sc7280-idp-ec-h1.dtsi"
> >
> > @@ -21,6 +22,27 @@
> > chosen {
> > stdout-path = "serial0:115200n8";
> > };
> > +
> > + edp_3v3_regulator: edp-3v3-regulator {
> > + compatible = "regulator-fixed";
> > + regulator-name = "edp_3v3_regulator";
> > +
> > + regulator-min-microvolt = <330>;
> > + regulator-max-microvolt = <330>;
> > +
> > + gpio = < 80 GPIO_ACTIVE_HIGH>;
> > + enable-active-high;
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <_panel_power>;
> > + };
> > +
> > + vreg_edp_bp: vreg-edp-bp-regulator {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vreg_edp_bp";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + };
> > };
> >
> > _rsc {
> > @@ -76,6 +98,58 @@ ap_ts_pen_1v8: {
> > };
> > };
> >
> > + {
> > + status = "okay";
> > +};
> > +
> > +_dp {
> > + status = "okay";
> > +
> > + pinctrl-names = "default";
> > + pinctrl-0 = <_hot_plug_det>;
> > + data-lanes = <0 1>;
> > + vdda-1p2-supply = <_l6b_1p2>;
> > + vdda-0p9-supply = <_l1b_0p8>; };
> > +
> > +_edp {
> > + status = "okay";
> > +
> > + data-lanes = <0 1 2 3>;
>
> Is this property necessary? It looks like the default.
>
Will remove it
> > + vdda-1p2-supply = <_l6b_1p2>;
> > + vdda-0p9-supply = <_l10c_0p8>;
> > +
> > + aux-bus {
>
> Can this move to sc7280.dtsi and get a phandle?
>
Okay, I will move this to sc7280.dtsi like below.
Shall I define the required properties under _edp_panel node in the
sc7280-crd3.dts?
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3283,6 +3283,18 @@
status = "disabled";
+ aux-bus {
+ mdss_edp_panel: panel {
+ compatible = "edp-panel";
+
+ port {
+ mdss_edp_panel_in:
endpoint {
+ remote-endpoint
= <_edp_out>;
+ };
+ };
+ };
+ };
+
ports {
#address-cells = <1>;
#size-cells = <0>;
@@ -3296,7 +3308,9 @@
port@1 {
reg = <1>;
- mdss_edp_out: endpoint { };
+ mdss_edp_out: endpoint {
+ remote-endpoint =
<_edp_panel_in>;
+ };
Re: [Freedreno] [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with drm_bridge_connector
> Subject: Re: [RFC PATCH v2 4/5] drm/msm/dp: replace dp_connector with
> drm_bridge_connector
> Date: Wed, 23 Feb 2022 16:40:56 -0800
> From: Kuogee Hsieh
> To: Stephen Boyd , Dmitry Baryshkov
>
> CC: Abhinav Kumar , Bjorn Andersson
> , Rob Clark , Sean Paul
> , David Airlie , Daniel Vetter
> , linux-arm-...@vger.kernel.org, dri-
> de...@lists.freedesktop.org, freedreno@lists.freedesktop.org
>
>
> On 2/23/2022 1:33 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-02-23 10:27:26)
> >> On 2/23/2022 10:22 AM, Dmitry Baryshkov wrote:
> >>> On 23/02/2022 20:21, Kuogee Hsieh wrote:
> >>>
> >>> In the panel device node.
> >>>
> >>> Can you please share it too?
> >>
> >> {
> >> edp_power_supply: edp_power {
> >> compatible = "regulator-fixed";
> >> regulator-name = "edp_backlight_power";
> >>
> >> regulator-always-on;
> >> regulator-boot-on;
> >> };
> >>
> >> edp_backlight: edp_backlight {
> >> compatible = "pwm-backlight";
> >>
> >> pwms = <_pwm 3 65535>;
> >> power-supply = <_power_supply>;
> >> enable-gpio = <_gpios 7 GPIO_ACTIVE_HIGH>;
> >>
> >> pinctrl-names = "default";
> >> pinctrl-0 = <_pwm_default>;
> >> };
> >>
> >> edp_panel: edp_panel {
> >> compatible = "sharp_lq140m1jw46";
> > Is that supposed to be sharp,lq140m1jw46 with a comma instead of an
> > underscore?
>
> Stephen,
>
> This is our internal branch which does not have patches up to date yet.
>
> I will cherry-pick newer edp related patches which are under review now to
> re test it.
Tested-by: Sankeerth Billakanti
Detect returned eDP not connected because the panel power was not on and
mode_valid was failing because the DP mode_valid is different from eDP
Re: [Freedreno] [RFC PATCH v2 5/5] drm/msm/dp: remove extra wrappers and public functions
> Subject: [RFC PATCH v2 5/5] drm/msm/dp: remove extra wrappers and
> public functions
> Date: Sat, 12 Feb 2022 01:40:06 +0300
> From: Dmitry Baryshkov
> To: Bjorn Andersson , Rob Clark
> , Sean Paul , Abhinav Kumar
> , Kuogee Hsieh
> CC: Stephen Boyd , David Airlie ,
> Daniel Vetter , linux-arm-...@vger.kernel.org, dri-
> de...@lists.freedesktop.org, freedreno@lists.freedesktop.org
>
> dp_bridge's functions are thin wrappers around the msm_dp_display_*
> family. Squash dp_bridge callbacks into respective msm_dp_display
> functions, removing the latter functions from public space.
>
> Signed-off-by: Dmitry Baryshkov
Tested-by: Sankeerth Billakanti
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 54 +++---
> drivers/gpu/drm/msm/dp/dp_display.h | 1 -
> drivers/gpu/drm/msm/dp/dp_drm.c | 72 ++---
> drivers/gpu/drm/msm/dp/dp_drm.h | 22 -
> drivers/gpu/drm/msm/msm_drv.h | 31 -
> 5 files changed, 60 insertions(+), 120 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 59e5e5b8e5b4..a9b641a68bff 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -10,7 +10,6 @@
> #include
> #include
> #include
> -#include
>#include "msm_drv.h"
> #include "msm_kms.h"
> @@ -945,18 +944,36 @@ int dp_display_set_plugged_cb(struct msm_dp
> *dp_display,
> return 0;
> }
> -int dp_display_validate_mode(struct msm_dp *dp, u32 mode_pclk_khz)
> +/**
> + * dp_bridge_mode_valid - callback to determine if specified mode is
> +valid
> + * @bridge: Pointer to drm bridge structure
> + * @info: display info
> + * @mode: Pointer to drm mode structure
> + * Returns: Validity status for specified mode */ enum drm_mode_status
> +dp_bridge_mode_valid(struct drm_bridge *bridge,
> + const struct drm_display_info *info,
> + const struct drm_display_mode
> *mode)
> {
> const u32 num_components = 3, default_bpp = 24;
> struct dp_display_private *dp_display;
> struct dp_link_info *link_info;
> u32 mode_rate_khz = 0, supported_rate_khz = 0, mode_bpp = 0;
> + struct msm_dp *dp;
> + int mode_pclk_khz = mode->clock;
> +
> + dp = to_dp_bridge(bridge)->dp_display;
> if (!dp || !mode_pclk_khz || !dp->connector) {
> DRM_ERROR("invalid params\n");
> return -EINVAL;
> }
> + if ((dp->max_pclk_khz <= 0) ||
> + (dp->max_pclk_khz > DP_MAX_PIXEL_CLK_KHZ) ||
> + (mode->clock > dp->max_pclk_khz))
> + return MODE_BAD;
> +
> dp_display = container_of(dp, struct dp_display_private,
> dp_display);
> link_info = _display->panel->link_info;
> @@ -1501,7 +1518,7 @@ int msm_dp_modeset_init(struct msm_dp
> *dp_display, struct drm_device *dev,
> dp_display->encoder = encoder;
> - dp_display->bridge = msm_dp_bridge_init(dp_display, dev,
> encoder);
> + dp_display->bridge = dp_bridge_init(dp_display, dev, encoder);
> if (IS_ERR(dp_display->bridge)) {
> ret = PTR_ERR(dp_display->bridge);
> DRM_DEV_ERROR(dev->dev,
> @@ -1528,8 +1545,10 @@ int msm_dp_modeset_init(struct msm_dp
> *dp_display, struct drm_device *dev,
> return 0;
> }
> -int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder
> *encoder)
> +void dp_bridge_enable(struct drm_bridge *drm_bridge)
> {
> + struct msm_dp_bridge *dp_bridge = to_dp_bridge(drm_bridge);
> + struct msm_dp *dp = dp_bridge->dp_display;
> int rc = 0;
> struct dp_display_private *dp_display;
> u32 state;
> @@ -1537,7 +1556,7 @@ int msm_dp_display_enable(struct msm_dp *dp,
> struct drm_encoder *encoder)
> dp_display = container_of(dp, struct dp_display_private,
> dp_display);
> if (!dp_display->dp_mode.drm_mode.clock) {
> DRM_ERROR("invalid params\n");
> - return -EINVAL;
> + return;
> }
> mutex_lock(_display->event_mutex);
> @@ -1549,14 +1568,14 @@ int msm_dp_display_enable(struct msm_dp *dp,
> struct drm_encoder *encoder)
> if (rc) {
> DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
> mutex_unlock(_display->event_mutex);
> - return rc;
> + return;
> }
> rc = dp_display_prepare(dp);
> if (rc) {
> DRM_ERROR("DP display prepare failed, rc=%d\n", rc);
> mutex_unlock(_display->event_mutex);
> - return rc;
> + return;
> }
> state = dp_display->hpd_state;
> @@ -1581,23 +1600,23 @@ int msm_dp_display_enable(struct msm_dp *dp,
> struct drm_encoder *encoder)
> dp_display->hpd_state = ST_CONNECTED;
> mutex_unlock(_display->event_mutex);
> -
> - return rc;
> }
> -int msm_dp_display_pre_disable(struct
Re: [Freedreno] [PATCH v3 4/4] drm/msm/dp: Add driver support to utilize drm panel
Hi Stephen,
Will make the changes.
-Original Message-
From: Stephen Boyd
Sent: Thursday, February 10, 2022 6:52 AM
To: Sankeerth Billakanti (QUIC) ; agr...@kernel.org;
airl...@linux.ie; bjorn.anders...@linaro.org; dan...@ffwll.ch;
devicet...@vger.kernel.org; diand...@chromium.org;
dri-de...@lists.freedesktop.org; freedreno@lists.freedesktop.org;
krzysztof.kozlow...@canonical.com; linux-arm-...@vger.kernel.org;
linux-ker...@vger.kernel.org; robdcl...@gmail.com; robh...@kernel.org;
s...@ravnborg.org; seanp...@chromium.org; thierry.red...@gmail.com
Cc: quic_kalyant ; Abhinav Kumar (QUIC)
; Kuogee Hsieh (QUIC) ;
quic_mkrishn ; quic_vproddut
; dmitry.barysh...@linaro.org
Subject: Re: [PATCH v3 4/4] drm/msm/dp: Add driver support to utilize drm panel
Quoting Sankeerth Billakanti (2022-02-09 00:55:32)
> Add support in the DP driver to utilize the custom eDP panels from
> drm/panels.
>
> An eDP panel is always connected to the platform. So, the eDP
> connector can be reported as always connected. The display mode will
> be sourced from the panel. The panel mode will be set after the link
> training is completed.
>
> Signed-off-by: Sankeerth Billakanti
> ---
>
> Changes in v3:
> None
>
> drivers/gpu/drm/msm/dp/dp_display.c | 8 ++
> drivers/gpu/drm/msm/dp/dp_drm.c | 54
> +
> drivers/gpu/drm/msm/dp/dp_parser.h | 3 +++
> 3 files changed, 60 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 7cc4d21..410fda4 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1513,6 +1513,10 @@ int msm_dp_display_enable(struct msm_dp *dp, struct
> drm_encoder *encoder)
> return -EINVAL;
> }
>
> + /* handle eDP on */
This comment is obvious. Please remove.
> + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
> + dp_hpd_plug_handle(dp_display, 0);
> +
> mutex_lock(_display->event_mutex);
>
> /* stop sentinel checking */
> @@ -1577,6 +1581,10 @@ int msm_dp_display_disable(struct msm_dp *dp,
> struct drm_encoder *encoder)
>
> dp_display = container_of(dp, struct dp_display_private,
> dp_display);
>
> + /* handle edp off */
This comment is obvious. Please remove.
> + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
> + dp_hpd_unplug_handle(dp_display, 0);
> +
> mutex_lock(_display->event_mutex);
>
> /* stop sentinel checking */
> diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c
> b/drivers/gpu/drm/msm/dp/dp_drm.c index d4d360d..12fa8c1 100644
> --- a/drivers/gpu/drm/msm/dp/dp_drm.c
> +++ b/drivers/gpu/drm/msm/dp/dp_drm.c
> @@ -39,6 +39,10 @@ static enum drm_connector_status
> dp_connector_detect(struct drm_connector *conn,
>
> dp = to_dp_connector(conn)->dp_display;
>
> + /* eDP is always connected */
> + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP)
> + return connector_status_connected;
Why not implement different connector ops for eDP and then not implement this
function at all in that case?
> +
> DRM_DEBUG_DP("is_connected = %s\n",
> (dp->is_connected) ? "true" : "false");
>
> @@ -123,6 +127,35 @@ static enum drm_mode_status dp_connector_mode_valid(
> return dp_display_validate_mode(dp_disp, mode->clock); }
>
> +static int edp_connector_get_modes(struct drm_connector *connector) {
> + struct msm_dp *dp;
> +
> + if (!connector)
Is this check really necessary? Why doesn't drm do it in higher layers?
> + return 0;
> +
> + dp = to_dp_connector(connector)->dp_display;
> +
> + return drm_bridge_get_modes(dp->panel_bridge, connector); }
> +
> +static enum drm_mode_status edp_connector_mode_valid(
> + struct drm_connector *connector,
> + struct drm_display_mode *mode) {
> + struct msm_dp *dp;
> +
> + if (!connector)
Is this check really necessary? Why doesn't drm do it in higher layers?
> + return 0;
> +
> + dp = to_dp_connector(connector)->dp_display;
> +
> + if (mode->clock > EDP_MAX_PIXEL_CLK_KHZ)
> + return MODE_BAD;
Why not return MODE_CLOCK_HIGH?
> +
> + return MODE_OK;
> +}
> +
> static const struct drm_connector_funcs dp_connector_funcs = {
> .detect = dp_connector_detect,
> .fill_modes = drm_helper_probe_single_connector_modes,
> @@ -137,6 +170,12 @@ static const struct drm_connector_helper_funcs
> dp_connector_helper_funcs =
Re: [Freedreno] [PATCH v3 3/4] drm/panel-edp: Add eDP sharp panel support
Hi Stephen, Thank you for the review. I will share the new patch. -Original Message- From: Stephen Boyd Sent: Thursday, February 10, 2022 6:47 AM To: Sankeerth Billakanti (QUIC) ; agr...@kernel.org; airl...@linux.ie; bjorn.anders...@linaro.org; dan...@ffwll.ch; devicet...@vger.kernel.org; diand...@chromium.org; dri-de...@lists.freedesktop.org; freedreno@lists.freedesktop.org; krzysztof.kozlow...@canonical.com; linux-arm-...@vger.kernel.org; linux-ker...@vger.kernel.org; robdcl...@gmail.com; robh...@kernel.org; s...@ravnborg.org; seanp...@chromium.org; thierry.red...@gmail.com Cc: quic_kalyant ; Abhinav Kumar (QUIC) ; Kuogee Hsieh (QUIC) ; quic_mkrishn ; quic_vproddut Subject: Re: [PATCH v3 3/4] drm/panel-edp: Add eDP sharp panel support Quoting Sankeerth Billakanti (2022-02-09 00:55:31) > Add support for the 14" sharp,lq140m1jw46 eDP panel. > > Signed-off-by: Sankeerth Billakanti > --- Can you share the edid-decode for this panel here under the cut "---"? It would help to see the timings and make sure everything matches.
Re: [Freedreno] [PATCH v3 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD
Hi Stephen,
Will implement all the suggested changes.
Thank you,
Sankeerth
-Original Message-
From: Stephen Boyd
Sent: Thursday, February 10, 2022 6:45 AM
To: Sankeerth Billakanti (QUIC) ; agr...@kernel.org;
airl...@linux.ie; bjorn.anders...@linaro.org; dan...@ffwll.ch;
devicet...@vger.kernel.org; diand...@chromium.org;
dri-de...@lists.freedesktop.org; freedreno@lists.freedesktop.org;
krzysztof.kozlow...@canonical.com; linux-arm-...@vger.kernel.org;
linux-ker...@vger.kernel.org; robdcl...@gmail.com; robh...@kernel.org;
s...@ravnborg.org; seanp...@chromium.org; thierry.red...@gmail.com
Cc: quic_kalyant ; Abhinav Kumar (QUIC)
; Kuogee Hsieh (QUIC) ;
quic_mkrishn ; quic_vproddut
Subject: Re: [PATCH v3 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel
on CRD
Quoting Sankeerth Billakanti (2022-02-09 00:55:30)
> Enable the eDP display panel support without HPD on sc7280 platform.
>
> Signed-off-by: Sankeerth Billakanti
> ---
>
> Changes in v3:
> - Sort the nodes alphabetically
> - Use - instead of _ as node names
> - Place the backlight and panel nodes under root
> - Change the name of edp_out to mdss_edp_out
> - Change the names of regulator nodes
> - Delete unused properties in the board file
>
>
> Changes in v2:
> - Sort node references alphabetically
> - Improve readability
> - Move the pwm pinctrl to pwm node
> - Move the regulators to root
> - Define backlight power
> - Remove dummy regulator node
> - Cleanup pinctrl definitions
>
> arch/arm64/boot/dts/qcom/sc7280-crd.dts | 122
>
> arch/arm64/boot/dts/qcom/sc7280.dtsi| 2 +-
> 2 files changed, 123 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> index e2efbdd..2a490a0 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> @@ -21,6 +21,59 @@
> chosen {
> stdout-path = "serial0:115200n8";
> };
> +
> + backlight_3v3_regulator: backlight-3v3-regulator {
> + compatible = "regulator-fixed";
> + regulator-name = "backlight_3v3_regulator";
> +
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> +
> + gpio = <_gpios 7 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_bl_power>;
> + };
> +
> + edp_3v3_regulator: edp-3v3-regulator {
> + compatible = "regulator-fixed";
> + regulator-name = "edp_3v3_regulator";
> +
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> +
> + gpio = < 80 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_panel_power>;
> + };
> +
> + edp_backlight: edp-backlight {
> + compatible = "pwm-backlight";
> +
> + power-supply = <_3v3_regulator>;
> + pwms = <_pwm 3 65535>;
> + };
> +
> + edp_panel: edp-panel {
> + compatible = "sharp,lq140m1jw46";
> +
> + power-supply = <_3v3_regulator>;
> + backlight = <_backlight>;
> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + port@0 {
> + reg = <0>;
> + edp_panel_in: endpoint {
> + remote-endpoint = <_out>;
Where is edp_out now?
> + };
> + };
> + };
> + };
> };
>
> _rsc {
> @@ -76,6 +129,44 @@ ap_ts_pen_1v8: {
> };
> };
>
> + {
> + status = "okay";
> +};
> +
> +_dp {
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_hot_plug_det>;
> + data-lanes = <0 1>;
> + vdda-1p2-supply = <_l6b_1p2>;
> + vdda-0p9-supply = <_l1b_0p8>; };
> +
> +_edp {
> + status = "okay";
> +
> + vdda-1p2-supply = <_l6b_1p2>;
> + vdda-0p9-supply = <_l10c_0p8>;
> + /delete-property/ pinctrl-names;
> + /dele
Re: [Freedreno] [PATCH v2 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD
Hi Bjorn,
1. I will change the edp_out label to mdss_edp_out.
2. The "pm8350c_pwm" node is part of the dependent series mentioned in the
cover letter. Below is the patch for the same:
https://patchwork.kernel.org/project/linux-arm-msm/patch/1637917920-22041-4-git-send-email-quic_c_ska...@quicinc.com/
3. I will move the edp_backlight and edp_panel nodes to the root.
Thank you,
Sankeerth
-Original Message-
From: Bjorn Andersson
Sent: Wednesday, February 9, 2022 5:23 AM
To: Sankeerth Billakanti (QUIC)
Cc: dri-de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
freedreno@lists.freedesktop.org; linux-ker...@vger.kernel.org;
devicet...@vger.kernel.org; agr...@kernel.org; robh...@kernel.org;
robdcl...@gmail.com; seanp...@chromium.org; swb...@chromium.org;
diand...@chromium.org; krzysztof.kozlow...@canonical.com;
thierry.red...@gmail.com; s...@ravnborg.org; airl...@linux.ie; dan...@ffwll.ch;
quic_kalyant ; Abhinav Kumar (QUIC)
; Kuogee Hsieh (QUIC) ;
quic_mkrishn
Subject: Re: [PATCH v2 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel
on CRD
On Tue 08 Feb 07:18 PST 2022, Sankeerth Billakanti wrote:
> Enable the eDP display panel support without HPD on sc7280 platform.
>
> Signed-off-by: Sankeerth Billakanti
> ---
>
> Changes in v2:
> - sort node references alphabetically
> - improve readability
> - move the pwm pinctrl to pwm node
> - move the regulators to root
> - define backlight power
> - remove dummy regulator node
> - cleanup pinctrl definitions
>
> arch/arm64/boot/dts/qcom/sc7280-crd.dts | 122
>
> arch/arm64/boot/dts/qcom/sc7280.dtsi| 2 -
> 2 files changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> index e2efbdd..bff2707 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> @@ -21,6 +21,34 @@
> chosen {
> stdout-path = "serial0:115200n8";
> };
> +
> + backlight_power: backlight-power {
> + compatible = "regulator-fixed";
> + regulator-name = "backlight_power";
> +
> + regulator-min-microvolt = <180>;
> + regulator-max-microvolt = <180>;
> +
> + gpio = <_gpios 7 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_bl_power>;
> + };
> +
> + edp_power: edp-power {
> + compatible = "regulator-fixed";
> + regulator-name = "edp_power";
> +
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> +
> + gpio = < 80 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_panel_power>;
> + };
> };
>
> _rsc {
> @@ -76,6 +104,42 @@ ap_ts_pen_1v8: {
> };
> };
>
> +_out {
Sorry for missing this while merging changes in sc7280.dtsi. But it would be
really nice if this was labeled mdss_edp_out instead (or possibly defined
within the _edp node).
Now you will have _out and _out floating around away from the edp and dp
nodes...
> + remote-endpoint = <_panel_in>;
> +};
> +
> + {
> + status = "okay";
> +};
> +
> +_edp {
> + status = "okay";
> +
> + vdda-1p2-supply = <_l6b_1p2>;
> + vdda-0p9-supply = <_l10c_0p8>;
> +};
> +
> +_edp_phy {
> + status = "okay";
> +
> + vdda-1p2-supply = <_l6b_1p2>;
> + vdda-0p9-supply = <_l10c_0p8>;
> +};
> +
> +_dp {
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_hot_plug_det>;
> + data-lanes = <0 1>;
> + vdda-1p2-supply = <_l6b_1p2>;
> + vdda-0p9-supply = <_l1b_0p8>;
> +};
> +
> +_mdp {
> + status = "okay";
> +};
> +
> _3v3_regulator {
> gpio = < 51 GPIO_ACTIVE_HIGH>;
> };
> @@ -84,7 +148,65 @@ ap_ts_pen_1v8: {
> pins = "gpio51";
> };
>
> +_pwm {
This label doesn't exist, so I won't be able to merge this patch.
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_bl_pwm>;
> +};
> +
> +_gpios {
> + edp_bl_power: edp-bl-power {
> + pins = "gpio7";
> + function = "normal&quo
Re: [Freedreno] [PATCH v2 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD
Hi Matthias,
I will implement the changes.
Thank you,
Sankeerth
-Original Message-
From: Matthias Kaehlcke
Sent: Wednesday, February 9, 2022 3:54 AM
To: Sankeerth Billakanti (QUIC)
Cc: dri-de...@lists.freedesktop.org; linux-arm-...@vger.kernel.org;
freedreno@lists.freedesktop.org; linux-ker...@vger.kernel.org;
devicet...@vger.kernel.org; agr...@kernel.org; bjorn.anders...@linaro.org;
robh...@kernel.org; robdcl...@gmail.com; seanp...@chromium.org;
swb...@chromium.org; diand...@chromium.org; krzysztof.kozlow...@canonical.com;
thierry.red...@gmail.com; s...@ravnborg.org; airl...@linux.ie; dan...@ffwll.ch;
quic_kalyant ; Abhinav Kumar (QUIC)
; Kuogee Hsieh (QUIC) ;
quic_mkrishn
Subject: Re: [PATCH v2 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel
on CRD
On Tue, Feb 08, 2022 at 08:48:43PM +0530, Sankeerth Billakanti wrote:
> Enable the eDP display panel support without HPD on sc7280 platform.
>
> Signed-off-by: Sankeerth Billakanti
> ---
>
> Changes in v2:
> - sort node references alphabetically
> - improve readability
> - move the pwm pinctrl to pwm node
> - move the regulators to root
> - define backlight power
> - remove dummy regulator node
> - cleanup pinctrl definitions
>
> arch/arm64/boot/dts/qcom/sc7280-crd.dts | 122
>
> arch/arm64/boot/dts/qcom/sc7280.dtsi| 2 -
> 2 files changed, 122 insertions(+), 2 deletions(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> index e2efbdd..bff2707 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> @@ -21,6 +21,34 @@
> chosen {
> stdout-path = "serial0:115200n8";
> };
> +
> + backlight_power: backlight-power {
nit: the other fixed regulator in sc7280-idp.dtsi is called
'nvme_3v3_regulator', if you wanted to be consistent you could call this
backlight_3v3_regulator.
> + compatible = "regulator-fixed";
> + regulator-name = "backlight_power";
> +
> + regulator-min-microvolt = <180>;
> + regulator-max-microvolt = <180>;
> +
> + gpio = <_gpios 7 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_bl_power>;
> + };
> +
> + edp_power: edp-power {
nit: see above
> + compatible = "regulator-fixed";
> + regulator-name = "edp_power";
> +
> + regulator-min-microvolt = <330>;
> + regulator-max-microvolt = <330>;
> +
> + gpio = < 80 GPIO_ACTIVE_HIGH>;
> + enable-active-high;
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_panel_power>;
> + };
> };
>
> _rsc {
> @@ -76,6 +104,42 @@ ap_ts_pen_1v8: {
> };
> };
>
> +_out {
> + remote-endpoint = <_panel_in>;
> +};
> +
> + {
> + status = "okay";
> +};
> +
> +_edp {
> + status = "okay";
> +
> + vdda-1p2-supply = <_l6b_1p2>;
> + vdda-0p9-supply = <_l10c_0p8>;
> +};
> +
> +_edp_phy {
> + status = "okay";
> +
> + vdda-1p2-supply = <_l6b_1p2>;
> + vdda-0p9-supply = <_l10c_0p8>;
> +};
> +
> +_dp {
should be before 'mdss_edp'.
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_hot_plug_det>;
> + data-lanes = <0 1>;
> + vdda-1p2-supply = <_l6b_1p2>;
> + vdda-0p9-supply = <_l1b_0p8>;
> +};
> +
> +_mdp {
> + status = "okay";
> +};
> +
> _3v3_regulator {
> gpio = < 51 GPIO_ACTIVE_HIGH>;
> };
> @@ -84,7 +148,65 @@ ap_ts_pen_1v8: {
> pins = "gpio51";
> };
>
> +_pwm {
> + status = "okay";
> +
> + pinctrl-names = "default";
> + pinctrl-0 = <_bl_pwm>;
> +};
> +
> +_gpios {
should be before 'pm8350c_pwm'
> + edp_bl_power: edp-bl-power {
> + pins = "gpio7";
> + function = "normal";
> + qcom,drive-strength = ;
> + bias-disable;
> + output-low;
> + };
> +
> + edp_bl_pwm: edp-bl-pwm {
> + pins = "gpio8";
> + function = "func1";
> + qcom,drive-strength = ;
> + bias-disable;
> + output-low;
> + };
> +};
