subject:"\[Intel\-gfx\] \[PATCH\] drm\/i915\: Don't try to use negative pll

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

2017-11-01 Thread Rodrigo Vivi

On Wed, Nov 01, 2017 at 07:08:52PM +, Manasi Navare wrote:
> On Wed, Nov 01, 2017 at 11:44:58AM -0700, Rodrigo Vivi wrote:
> > It is unlikely we are getting the -1 here.
> > But if we propagate that pll_id -1 to the rest of the code
> > we might have funny calculations on link_clock and who
> > knows what registers we end up accessing.
> > 
> > Better to protect the code.
> > 
> > Also better with errno number instead of generic -1.
> > 
> > Cc: Manasi Navare 
> > Signed-off-by: Rodrigo Vivi 
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c  | 10 ++
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c |  2 +-
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index ace674cd79b9..2c6abdbf33ea 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1283,6 +1283,11 @@ static void cnl_ddi_clock_get(struct intel_encoder 
> > *encoder,
> >  
> > pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
> >  
> > +   if (pll_id < 0) {
> > +   DRM_ERROR("PLL not found\n");
> > +   return;
> > +   }
> > +
> 
> The function intel_get_shared_dpll_id() already has this WARN_ON:
> if (WARN_ON(pll < dev_priv->shared_dplls))
> and it return pll_id = (pll - dev_priv->shared_dplls);
> So shouldnt this already throw a warning in case pll < dev_priv->shared_dplls 
> eventually
> resulting into a negative pll_id?

yeap, we through a warning and return -1.

Then we use the -1 to do some reg access and bits...

> 
> Manasi
> 
> > cfgcr0 = I915_READ(CNL_DPLL_CFGCR0(pll_id));
> >  
> > if (cfgcr0 & DPLL_CFGCR0_HDMI_MODE) {
> > @@ -1337,6 +1342,11 @@ static void skl_ddi_clock_get(struct intel_encoder 
> > *encoder,
> >  
> > pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
> >  
> > +   if (pll_id < 0) {
> > +   DRM_ERROR("PLL not found\n");
> > +   return;
> > +   }
> > +
> > dpll_ctl1 = I915_READ(DPLL_CTRL1);
> >  
> > if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(pll_id)) {
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c 
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index a83bf1c38e05..c4a7f39e173a 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -102,7 +102,7 @@ intel_get_shared_dpll_id(struct drm_i915_private 
> > *dev_priv,
> >  {
> > if (WARN_ON(pll < dev_priv->shared_dplls||
> > pll > _priv->shared_dplls[dev_priv->num_shared_dpll]))
> > -   return -1;
> > +   return -ENOENT;
> >  
> > return (enum intel_dpll_id) (pll - dev_priv->shared_dplls);
> >  }
> > -- 
> > 2.13.6
> > 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

2017-11-01 Thread Ville Syrjälä

On Wed, Nov 01, 2017 at 12:13:42PM -0700, Rodrigo Vivi wrote:
> On Wed, Nov 01, 2017 at 06:56:55PM +, Ville Syrjälä wrote:
> > On Wed, Nov 01, 2017 at 11:44:58AM -0700, Rodrigo Vivi wrote:
> > > It is unlikely we are getting the -1 here.
> > > But if we propagate that pll_id -1 to the rest of the code
> > > we might have funny calculations on link_clock and who
> > > knows what registers we end up accessing.
> > > 
> > > Better to protect the code.
> > > 
> > > Also better with errno number instead of generic -1.
> > > 
> > > Cc: Manasi Navare 
> > > Signed-off-by: Rodrigo Vivi 
> > > ---
> > >  drivers/gpu/drm/i915/intel_ddi.c  | 10 ++
> > >  drivers/gpu/drm/i915/intel_dpll_mgr.c |  2 +-
> > >  2 files changed, 11 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> > > b/drivers/gpu/drm/i915/intel_ddi.c
> > > index ace674cd79b9..2c6abdbf33ea 100644
> > > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > > @@ -1283,6 +1283,11 @@ static void cnl_ddi_clock_get(struct intel_encoder 
> > > *encoder,
> > >  
> > >   pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
> > >  
> > > + if (pll_id < 0) {
> > > + DRM_ERROR("PLL not found\n");
> > > + return;
> > > + }
> > > +
> > >   cfgcr0 = I915_READ(CNL_DPLL_CFGCR0(pll_id));
> > 
> > Don't we have the dpll state already read out?
> > 
> > crtc_state->dll_hw_state.cfgcr0 etc.?
> 
> oh! we do!
> Also during calc_wrpll_link we should be using this,
> shouldn't we?!
> 
> Probably my bad when doing this code looking to skl code...
> 
> Same thing on skl case, right?!

On all platforms I'd say. This is all probably just leftovers from before
we had the dpll manager.

I suspect we should just move all these foo_calc_wrpll_link() etc.
functions into the dpll manager. Not sure if we could just directly
fill out the port_clock in the dpll manager actually. Hmm. I guess not
in the .get_hw_state() at least since we don't pass the full crtc state
there. But we could perhaps have some kind of .clock_get() function
that'd be similar to the i9xx/vlv/chv_clock_get(), except we'd just call
it from the DDI code where we currently call these hand rolled
functions.

> > 
> > >  
> > >   if (cfgcr0 & DPLL_CFGCR0_HDMI_MODE) {
> > > @@ -1337,6 +1342,11 @@ static void skl_ddi_clock_get(struct intel_encoder 
> > > *encoder,
> > >  
> > >   pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
> > >  
> > > + if (pll_id < 0) {
> > > + DRM_ERROR("PLL not found\n");
> > > + return;
> > > + }
> > > +
> > >   dpll_ctl1 = I915_READ(DPLL_CTRL1);
> > >  
> > >   if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(pll_id)) {
> > > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c 
> > > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > index a83bf1c38e05..c4a7f39e173a 100644
> > > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > > @@ -102,7 +102,7 @@ intel_get_shared_dpll_id(struct drm_i915_private 
> > > *dev_priv,
> > >  {
> > >   if (WARN_ON(pll < dev_priv->shared_dplls||
> > >   pll > _priv->shared_dplls[dev_priv->num_shared_dpll]))
> > > - return -1;
> > > + return -ENOENT;
> > >  
> > >   return (enum intel_dpll_id) (pll - dev_priv->shared_dplls);
> > >  }
> > > -- 
> > > 2.13.6
> > > 
> > > ___
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > -- 
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

2017-11-01 Thread Rodrigo Vivi

On Wed, Nov 01, 2017 at 06:56:55PM +, Ville Syrjälä wrote:
> On Wed, Nov 01, 2017 at 11:44:58AM -0700, Rodrigo Vivi wrote:
> > It is unlikely we are getting the -1 here.
> > But if we propagate that pll_id -1 to the rest of the code
> > we might have funny calculations on link_clock and who
> > knows what registers we end up accessing.
> > 
> > Better to protect the code.
> > 
> > Also better with errno number instead of generic -1.
> > 
> > Cc: Manasi Navare 
> > Signed-off-by: Rodrigo Vivi 
> > ---
> >  drivers/gpu/drm/i915/intel_ddi.c  | 10 ++
> >  drivers/gpu/drm/i915/intel_dpll_mgr.c |  2 +-
> >  2 files changed, 11 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> > b/drivers/gpu/drm/i915/intel_ddi.c
> > index ace674cd79b9..2c6abdbf33ea 100644
> > --- a/drivers/gpu/drm/i915/intel_ddi.c
> > +++ b/drivers/gpu/drm/i915/intel_ddi.c
> > @@ -1283,6 +1283,11 @@ static void cnl_ddi_clock_get(struct intel_encoder 
> > *encoder,
> >  
> > pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
> >  
> > +   if (pll_id < 0) {
> > +   DRM_ERROR("PLL not found\n");
> > +   return;
> > +   }
> > +
> > cfgcr0 = I915_READ(CNL_DPLL_CFGCR0(pll_id));
> 
> Don't we have the dpll state already read out?
> 
> crtc_state->dll_hw_state.cfgcr0 etc.?

oh! we do!
Also during calc_wrpll_link we should be using this,
shouldn't we?!

Probably my bad when doing this code looking to skl code...

Same thing on skl case, right?!

> 
> >  
> > if (cfgcr0 & DPLL_CFGCR0_HDMI_MODE) {
> > @@ -1337,6 +1342,11 @@ static void skl_ddi_clock_get(struct intel_encoder 
> > *encoder,
> >  
> > pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
> >  
> > +   if (pll_id < 0) {
> > +   DRM_ERROR("PLL not found\n");
> > +   return;
> > +   }
> > +
> > dpll_ctl1 = I915_READ(DPLL_CTRL1);
> >  
> > if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(pll_id)) {
> > diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c 
> > b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > index a83bf1c38e05..c4a7f39e173a 100644
> > --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> > @@ -102,7 +102,7 @@ intel_get_shared_dpll_id(struct drm_i915_private 
> > *dev_priv,
> >  {
> > if (WARN_ON(pll < dev_priv->shared_dplls||
> > pll > _priv->shared_dplls[dev_priv->num_shared_dpll]))
> > -   return -1;
> > +   return -ENOENT;
> >  
> > return (enum intel_dpll_id) (pll - dev_priv->shared_dplls);
> >  }
> > -- 
> > 2.13.6
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Ville Syrjälä
> Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

2017-11-01 Thread Manasi Navare

On Wed, Nov 01, 2017 at 11:44:58AM -0700, Rodrigo Vivi wrote:
> It is unlikely we are getting the -1 here.
> But if we propagate that pll_id -1 to the rest of the code
> we might have funny calculations on link_clock and who
> knows what registers we end up accessing.
> 
> Better to protect the code.
> 
> Also better with errno number instead of generic -1.
> 
> Cc: Manasi Navare 
> Signed-off-by: Rodrigo Vivi 
> ---
>  drivers/gpu/drm/i915/intel_ddi.c  | 10 ++
>  drivers/gpu/drm/i915/intel_dpll_mgr.c |  2 +-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> b/drivers/gpu/drm/i915/intel_ddi.c
> index ace674cd79b9..2c6abdbf33ea 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1283,6 +1283,11 @@ static void cnl_ddi_clock_get(struct intel_encoder 
> *encoder,
>  
>   pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
>  
> + if (pll_id < 0) {
> + DRM_ERROR("PLL not found\n");
> + return;
> + }
> +

The function intel_get_shared_dpll_id() already has this WARN_ON:
if (WARN_ON(pll < dev_priv->shared_dplls))
and it return pll_id = (pll - dev_priv->shared_dplls);
So shouldnt this already throw a warning in case pll < dev_priv->shared_dplls 
eventually
resulting into a negative pll_id?

Manasi

>   cfgcr0 = I915_READ(CNL_DPLL_CFGCR0(pll_id));
>  
>   if (cfgcr0 & DPLL_CFGCR0_HDMI_MODE) {
> @@ -1337,6 +1342,11 @@ static void skl_ddi_clock_get(struct intel_encoder 
> *encoder,
>  
>   pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
>  
> + if (pll_id < 0) {
> + DRM_ERROR("PLL not found\n");
> + return;
> + }
> +
>   dpll_ctl1 = I915_READ(DPLL_CTRL1);
>  
>   if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(pll_id)) {
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c 
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index a83bf1c38e05..c4a7f39e173a 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -102,7 +102,7 @@ intel_get_shared_dpll_id(struct drm_i915_private 
> *dev_priv,
>  {
>   if (WARN_ON(pll < dev_priv->shared_dplls||
>   pll > _priv->shared_dplls[dev_priv->num_shared_dpll]))
> - return -1;
> + return -ENOENT;
>  
>   return (enum intel_dpll_id) (pll - dev_priv->shared_dplls);
>  }
> -- 
> 2.13.6
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

2017-11-01 Thread Ville Syrjälä

On Wed, Nov 01, 2017 at 11:44:58AM -0700, Rodrigo Vivi wrote:
> It is unlikely we are getting the -1 here.
> But if we propagate that pll_id -1 to the rest of the code
> we might have funny calculations on link_clock and who
> knows what registers we end up accessing.
> 
> Better to protect the code.
> 
> Also better with errno number instead of generic -1.
> 
> Cc: Manasi Navare 
> Signed-off-by: Rodrigo Vivi 
> ---
>  drivers/gpu/drm/i915/intel_ddi.c  | 10 ++
>  drivers/gpu/drm/i915/intel_dpll_mgr.c |  2 +-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c 
> b/drivers/gpu/drm/i915/intel_ddi.c
> index ace674cd79b9..2c6abdbf33ea 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -1283,6 +1283,11 @@ static void cnl_ddi_clock_get(struct intel_encoder 
> *encoder,
>  
>   pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
>  
> + if (pll_id < 0) {
> + DRM_ERROR("PLL not found\n");
> + return;
> + }
> +
>   cfgcr0 = I915_READ(CNL_DPLL_CFGCR0(pll_id));

Don't we have the dpll state already read out?

crtc_state->dll_hw_state.cfgcr0 etc.?

>  
>   if (cfgcr0 & DPLL_CFGCR0_HDMI_MODE) {
> @@ -1337,6 +1342,11 @@ static void skl_ddi_clock_get(struct intel_encoder 
> *encoder,
>  
>   pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
>  
> + if (pll_id < 0) {
> + DRM_ERROR("PLL not found\n");
> + return;
> + }
> +
>   dpll_ctl1 = I915_READ(DPLL_CTRL1);
>  
>   if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(pll_id)) {
> diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c 
> b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> index a83bf1c38e05..c4a7f39e173a 100644
> --- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
> +++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
> @@ -102,7 +102,7 @@ intel_get_shared_dpll_id(struct drm_i915_private 
> *dev_priv,
>  {
>   if (WARN_ON(pll < dev_priv->shared_dplls||
>   pll > _priv->shared_dplls[dev_priv->num_shared_dpll]))
> - return -1;
> + return -ENOENT;
>  
>   return (enum intel_dpll_id) (pll - dev_priv->shared_dplls);
>  }
> -- 
> 2.13.6
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

2017-11-01 Thread Rodrigo Vivi

It is unlikely we are getting the -1 here.
But if we propagate that pll_id -1 to the rest of the code
we might have funny calculations on link_clock and who
knows what registers we end up accessing.

Better to protect the code.

Also better with errno number instead of generic -1.

Cc: Manasi Navare 
Signed-off-by: Rodrigo Vivi 
---
 drivers/gpu/drm/i915/intel_ddi.c  | 10 ++
 drivers/gpu/drm/i915/intel_dpll_mgr.c |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index ace674cd79b9..2c6abdbf33ea 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1283,6 +1283,11 @@ static void cnl_ddi_clock_get(struct intel_encoder 
*encoder,
 
pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
 
+   if (pll_id < 0) {
+   DRM_ERROR("PLL not found\n");
+   return;
+   }
+
cfgcr0 = I915_READ(CNL_DPLL_CFGCR0(pll_id));
 
if (cfgcr0 & DPLL_CFGCR0_HDMI_MODE) {
@@ -1337,6 +1342,11 @@ static void skl_ddi_clock_get(struct intel_encoder 
*encoder,
 
pll_id = intel_get_shared_dpll_id(dev_priv, pipe_config->shared_dpll);
 
+   if (pll_id < 0) {
+   DRM_ERROR("PLL not found\n");
+   return;
+   }
+
dpll_ctl1 = I915_READ(DPLL_CTRL1);
 
if (dpll_ctl1 & DPLL_CTRL1_HDMI_MODE(pll_id)) {
diff --git a/drivers/gpu/drm/i915/intel_dpll_mgr.c 
b/drivers/gpu/drm/i915/intel_dpll_mgr.c
index a83bf1c38e05..c4a7f39e173a 100644
--- a/drivers/gpu/drm/i915/intel_dpll_mgr.c
+++ b/drivers/gpu/drm/i915/intel_dpll_mgr.c
@@ -102,7 +102,7 @@ intel_get_shared_dpll_id(struct drm_i915_private *dev_priv,
 {
if (WARN_ON(pll < dev_priv->shared_dplls||
pll > _priv->shared_dplls[dev_priv->num_shared_dpll]))
-   return -1;
+   return -ENOENT;
 
return (enum intel_dpll_id) (pll - dev_priv->shared_dplls);
 }
-- 
2.13.6

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

Re: [Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

[Intel-gfx] [PATCH] drm/i915: Don't try to use negative pll_id.

6 matches

Site Navigation

Mail list logo

Footer information