Some eDP sinks or platform boards will not support hpd.
This patch adds support for those cases.
Signed-off-by: Sankeerth Billakanti <[email protected]>
---
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"))
+ dp_write_aux(catalog, REG_DP_DP_HPD_CTRL,
+ DP_DP_HPD_CTRL_HPD_EN);
}
u32 dp_catalog_link_is_connected(struct dp_catalog *dp_catalog)
--
2.7.4
Hi,
On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
<[email protected]> 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.
> Signed-off-by: Sankeerth Billakanti <[email protected]>
> ---
> 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?
-Doug
Hi Doug,
> On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Mon, 4 Apr 2022 at 21:32, Sankeerth Billakanti (QUIC)
<[email protected]> wrote:
>
> Hi Doug,
>
> > On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> > <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
--
With best wishes
Dmitry
> > > On Wed, Mar 30, 2022 at 9:04 AM Sankeerth Billakanti
> > > <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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