[04/14] drm/i915/tgl: Fix driver crash when update_active_dpll is called
diff mbox series

Message ID 20190913223251.354877-5-jose.souza@intel.com
State New
Headers show
Series
  • TGL TC enabling
Related show

Commit Message

José Roberto de Souza Sept. 13, 2019, 10:32 p.m. UTC
From: "Taylor, Clinton A" <clinton.a.taylor@intel.com>

TGL PLL function table doesn't include and update_active_pll function.
The driver attempts to make a call to this function and crashes during
PLL changes.

Signed-off-by: Taylor, Clinton A <clinton.a.taylor@intel.com>
---
 drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Lucas De Marchi Sept. 14, 2019, 6:32 a.m. UTC | #1
On Fri, Sep 13, 2019 at 3:33 PM José Roberto de Souza
<jose.souza@intel.com> wrote:
>
> From: "Taylor, Clinton A" <clinton.a.taylor@intel.com>
>
> TGL PLL function table doesn't include and update_active_pll function.
> The driver attempts to make a call to this function and crashes during
> PLL changes.

the crash would only occur if the port was initialized. The ordering
in this series means this is
not a "fix" but rather finishing the implementation for TC ports
before initializing them. So we may
want to adjust the commit message accordingly. The reason we missed
that for TGL is that its need came
in parallel to the TGL support hitting upstream.

My nit with this hook is that `update_active_dpll` is exclusively used
by *TC* ports on gen 11+ and
a) it's  not clear about that from the name and/or b) if it's generic
it should not crash when it's missing.

I think Imre had a patch to address it, at least renaming the hook, I
don't remember. +Imre.

Lucas De Marchi

>
> Signed-off-by: Taylor, Clinton A <clinton.a.taylor@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> index 98288edf88f0..84e734d44828 100644
> --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> @@ -3479,6 +3479,7 @@ static const struct intel_dpll_mgr tgl_pll_mgr = {
>         .dpll_info = tgl_plls,
>         .get_dplls = icl_get_dplls,
>         .put_dplls = icl_put_dplls,
> +       .update_active_dpll = icl_update_active_dpll,
>         .dump_hw_state = icl_dump_hw_state,
>  };
>
> --
> 2.23.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
José Roberto de Souza Sept. 17, 2019, 10:59 p.m. UTC | #2
On Fri, 2019-09-13 at 23:32 -0700, Lucas De Marchi wrote:
> On Fri, Sep 13, 2019 at 3:33 PM José Roberto de Souza
> <jose.souza@intel.com> wrote:
> > From: "Taylor, Clinton A" <clinton.a.taylor@intel.com>
> > 
> > TGL PLL function table doesn't include and update_active_pll
> > function.
> > The driver attempts to make a call to this function and crashes
> > during
> > PLL changes.
> 
> the crash would only occur if the port was initialized. The ordering
> in this series means this is
> not a "fix" but rather finishing the implementation for TC ports
> before initializing them. So we may
> want to adjust the commit message accordingly. The reason we missed
> that for TGL is that its need came
> in parallel to the TGL support hitting upstream.
> 

Okay will change commit message and description to:

drm/i915/tgl/pll: Set update_active_dpll

Commit 24a7bfe0c2d7 ("drm/i915: Keep the TypeC port mode fixed when the
port is active") added this new hook while in parallel TGL upstream was
happening and this was missed.
Without this driver will crash when TC DDI is added and driver is
preparing to do a full modeset.

Sounds good?


> My nit with this hook is that `update_active_dpll` is exclusively
> used
> by *TC* ports on gen 11+ and
> a) it's  not clear about that from the name and/or b) if it's generic
> it should not crash when it's missing.
> 
> I think Imre had a patch to address it, at least renaming the hook, I
> don't remember. +Imre.

Well we can do that on top or in Imre series not planing to address
this in here.

> 
> Lucas De Marchi

@Clint

Looks like git send-email don't like "," in the name and replaces your
email address to Taylor@freedesktop.org so I'm going to replace your
signed-off-by to "Clinton A Taylor <clinton.a.taylor@intel.com>" or do
you prefer something else?

> 
> > Signed-off-by: Taylor, Clinton A <clinton.a.taylor@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dpll_mgr.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > index 98288edf88f0..84e734d44828 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
> > @@ -3479,6 +3479,7 @@ static const struct intel_dpll_mgr
> > tgl_pll_mgr = {
> >         .dpll_info = tgl_plls,
> >         .get_dplls = icl_get_dplls,
> >         .put_dplls = icl_put_dplls,
> > +       .update_active_dpll = icl_update_active_dpll,
> >         .dump_hw_state = icl_dump_hw_state,
> >  };
> > 
> > --
> > 2.23.0
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
>

Patch
diff mbox series

diff --git a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
index 98288edf88f0..84e734d44828 100644
--- a/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/display/intel_dpll_mgr.c
@@ -3479,6 +3479,7 @@  static const struct intel_dpll_mgr tgl_pll_mgr = {
 	.dpll_info = tgl_plls,
 	.get_dplls = icl_get_dplls,
 	.put_dplls = icl_put_dplls,
+	.update_active_dpll = icl_update_active_dpll,
 	.dump_hw_state = icl_dump_hw_state,
 };