| Message ID | 1650551811-24319-3-git-send-email-quic_sbillaka@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for the eDP panel over aux_bus | expand |
Hi, On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti <quic_sbillaka@quicinc.com> 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 <quic_sbillaka@quicinc.com> > --- > 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? 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. If you make that change then all of a sudden the API isn't weird/wonky and you can just get rid of the comment I asked you to add. -Doug
Hi Doug, >On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti ><quic_sbillaka@quicinc.com> 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 <quic_sbillaka@quicinc.com> >> --- >> 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 (0x40000000) #define DP_DP_HPD_STATE_STATUS_PENDING (0x20000000) #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x00000000) #define DP_DP_HPD_STATE_STATUS_MASK (0xE0000000) 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 handle all the set interrupt bits. So, while handling aux transfer interrupts, we were proceeding to handle connect interrupt also as a consequence. >If you make that change then all of a sudden the API isn't weird/wonky and >you can just get rid of the comment I asked you to add. > > >-Doug
Hi, On Thu, Apr 21, 2022 at 9:39 AM Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com> wrote: > > Hi Doug, > > >On Thu, Apr 21, 2022 at 7:37 AM Sankeerth Billakanti > ><quic_sbillaka@quicinc.com> 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 <quic_sbillaka@quicinc.com> > >> --- > >> 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 (0x40000000) > #define DP_DP_HPD_STATE_STATUS_PENDING (0x20000000) > #define DP_DP_HPD_STATE_STATUS_DISCONNECTED (0x00000000) > #define DP_DP_HPD_STATE_STATUS_MASK (0xE0000000) > > 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 handle all the set interrupt bits. So, while handling aux transfer > interrupts, we were proceeding to handle connect interrupt also as a consequence. Ah, I see. There aren't other _interrupts_ in this register. There's just other status info that someone jammed into this register. Got it. So with this comment I'm happy with my Reviewed-by: /* * We only want to return interrupts that are unmasked to the caller. * However, the interrupt status field also contains other * informational bits about the HPD state status, so we only mask * out the part of the register that tells us about which interrupts * are pending. */ -Doug
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); } 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 3d03c77..874258d 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -683,7 +683,8 @@ static int dp_hpd_unplug_handle(struct dp_display_private *dp, u32 data) dp_display_handle_plugged_change(&dp->dp_display, false); /* enable HDP plug interrupt to prepare for next plugin */ - dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true); + if (!dp->dp_display.is_edp) + dp_catalog_hpd_config_intr(dp->catalog, DP_DP_HPD_PLUG_INT_MASK, true); DRM_DEBUG_DP("After, type=%d hpd_state=%d\n", dp->dp_display.connector_type, state); @@ -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); + /* Enable interrupt first time * we are leaving dp clocks on during disconnect * and never disable interrupt @@ -1381,6 +1389,12 @@ static int dp_pm_resume(struct device *dev) dp_catalog_ctrl_hpd_config(dp->catalog); + 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); + if (dp_catalog_link_is_connected(dp->catalog)) { /* * set sink to normal operation mode -- D0 @@ -1655,6 +1669,9 @@ void dp_bridge_enable(struct drm_bridge *drm_bridge) return; } + if (dp->is_edp) + dp_hpd_plug_handle(dp_display, 0); + mutex_lock(&dp_display->event_mutex); /* stop sentinel checking */ @@ -1719,6 +1736,9 @@ void dp_bridge_post_disable(struct drm_bridge *drm_bridge) dp_display = container_of(dp, struct dp_display_private, dp_display); + if (dp->is_edp) + dp_hpd_unplug_handle(dp_display, 0); + mutex_lock(&dp_display->event_mutex); /* stop sentinel checking */
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 <quic_sbillaka@quicinc.com> --- 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(-)