Re: [Intel-gfx] [PATCH 5/8] drm/i915: Check error return when converting pipe to connector

[Imre Deak]

On Wed, Apr 26, 2017 at 05:53:52PM +0300, Jani Nikula wrote:
> On Wed, 26 Apr 2017, Imre Deak  wrote:
> > On Wed, Apr 26, 2017 at 05:12:32PM +0300, Jani Nikula wrote:
> >> On Wed, 26 Apr 2017, Imre Deak  wrote:
> >> > An error from intel_get_pipe_from_connector() would mean a bug somewhere
> >> > else, but we still should check for it to prevent some other more
> >> > obscure bug later.
> >> 
> >> Do check for invalid pipe, but please just limp on instead of bailing
> >> out of the functions. See notes inline.
> >> 
> >> BR,
> >> Jani.
> >> 
> >> >
> >> > Signed-off-by: Imre Deak 
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_panel.c | 14 --
> >> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> >> > b/drivers/gpu/drm/i915/intel_panel.c
> >> > index cb50c52..dbe05ec 100644
> >> > --- a/drivers/gpu/drm/i915/intel_panel.c
> >> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct 
> >> > intel_connector *connector)
> >> >  struct drm_i915_private *dev_priv = 
> >> > to_i915(connector->base.dev);
> >> >  struct intel_panel *panel = >panel;
> >> >  enum pipe pipe = intel_get_pipe_from_connector(connector);
> >> > -enum transcoder cpu_transcoder =
> >> > -intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> >> > +enum transcoder cpu_transcoder;
> >> >  u32 cpu_ctl2, pch_ctl1, pch_ctl2;
> >> >  
> >> > +if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> >> > +return;
> >> 
> >> Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case,
> >> go on, and hope for the best. Do leave the warn in place though.
> >> 
> >> > +
> >> > +cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> >> > +
> >> >  cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> >> >  if (cpu_ctl2 & BLM_PWM_ENABLE) {
> >> >  DRM_DEBUG_KMS("cpu backlight already enabled\n");
> >> > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct 
> >> > intel_connector *connector)
> >> >  enum pipe pipe = intel_get_pipe_from_connector(connector);
> >> >  u32 ctl, ctl2, freq;
> >> >  
> >> > +if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> >> > +return;
> >> 
> >> Same here, please just assign e.g. PIPE_A here with the warning, and
> >> limp on.
> >> 
> >> > +
> >> >  ctl2 = I915_READ(BLC_PWM_CTL2);
> >> >  if (ctl2 & BLM_PWM_ENABLE) {
> >> >  DRM_DEBUG_KMS("backlight already enabled\n");
> >> > @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct 
> >> > intel_connector *connector)
> >> >  if (!panel->backlight.present)
> >> >  return;
> >> >  
> >> > +if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> >> > +return;
> >> 
> >> Here, pipe is only used for the debug logging, just skip that instead.
> >
> > Ok, can do. This would make things inconsistent wrt.
> > vlv_enable/disable_backlight() though, so are you ok if I change those
> > too accordingly?
> 
> Those are different in the sense that the *registers* are chosen based
> on the pipe, so it *must* be one of A or B. But in the paths changed
> here, pipe is only used for some bit fields of the registers being
> updated. IIRC they are not even crucial for fundamental operation of the
> backlight. So I don't want to bail out unless we really can't proceed.

Ok, will keep those as-is.

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

Re: [Intel-gfx] [PATCH 5/8] drm/i915: Check error return when converting pipe to connector

[Jani Nikula]

On Wed, 26 Apr 2017, Imre Deak  wrote:
> On Wed, Apr 26, 2017 at 05:12:32PM +0300, Jani Nikula wrote:
>> On Wed, 26 Apr 2017, Imre Deak  wrote:
>> > An error from intel_get_pipe_from_connector() would mean a bug somewhere
>> > else, but we still should check for it to prevent some other more
>> > obscure bug later.
>> 
>> Do check for invalid pipe, but please just limp on instead of bailing
>> out of the functions. See notes inline.
>> 
>> BR,
>> Jani.
>> 
>> >
>> > Signed-off-by: Imre Deak 
>> > ---
>> >  drivers/gpu/drm/i915/intel_panel.c | 14 --
>> >  1 file changed, 12 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_panel.c 
>> > b/drivers/gpu/drm/i915/intel_panel.c
>> > index cb50c52..dbe05ec 100644
>> > --- a/drivers/gpu/drm/i915/intel_panel.c
>> > +++ b/drivers/gpu/drm/i915/intel_panel.c
>> > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct 
>> > intel_connector *connector)
>> >struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>> >struct intel_panel *panel = >panel;
>> >enum pipe pipe = intel_get_pipe_from_connector(connector);
>> > -  enum transcoder cpu_transcoder =
>> > -  intel_pipe_to_cpu_transcoder(dev_priv, pipe);
>> > +  enum transcoder cpu_transcoder;
>> >u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>> >  
>> > +  if (WARN_ON_ONCE(pipe == INVALID_PIPE))
>> > +  return;
>> 
>> Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case,
>> go on, and hope for the best. Do leave the warn in place though.
>> 
>> > +
>> > +  cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
>> > +
>> >cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
>> >if (cpu_ctl2 & BLM_PWM_ENABLE) {
>> >DRM_DEBUG_KMS("cpu backlight already enabled\n");
>> > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct 
>> > intel_connector *connector)
>> >enum pipe pipe = intel_get_pipe_from_connector(connector);
>> >u32 ctl, ctl2, freq;
>> >  
>> > +  if (WARN_ON_ONCE(pipe == INVALID_PIPE))
>> > +  return;
>> 
>> Same here, please just assign e.g. PIPE_A here with the warning, and
>> limp on.
>> 
>> > +
>> >ctl2 = I915_READ(BLC_PWM_CTL2);
>> >if (ctl2 & BLM_PWM_ENABLE) {
>> >DRM_DEBUG_KMS("backlight already enabled\n");
>> > @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct 
>> > intel_connector *connector)
>> >if (!panel->backlight.present)
>> >return;
>> >  
>> > +  if (WARN_ON_ONCE(pipe == INVALID_PIPE))
>> > +  return;
>> 
>> Here, pipe is only used for the debug logging, just skip that instead.
>
> Ok, can do. This would make things inconsistent wrt.
> vlv_enable/disable_backlight() though, so are you ok if I change those
> too accordingly?

Those are different in the sense that the *registers* are chosen based
on the pipe, so it *must* be one of A or B. But in the paths changed
here, pipe is only used for some bit fields of the registers being
updated. IIRC they are not even crucial for fundamental operation of the
backlight. So I don't want to bail out unless we really can't proceed.

BR,
Jani.


>
> --Imre
>
>> 
>> > +
>> >DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
>> >  
>> >mutex_lock(_priv->backlight_lock);
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/8] drm/i915: Check error return when converting pipe to connector

[Imre Deak]

On Wed, Apr 26, 2017 at 05:12:32PM +0300, Jani Nikula wrote:
> On Wed, 26 Apr 2017, Imre Deak  wrote:
> > An error from intel_get_pipe_from_connector() would mean a bug somewhere
> > else, but we still should check for it to prevent some other more
> > obscure bug later.
> 
> Do check for invalid pipe, but please just limp on instead of bailing
> out of the functions. See notes inline.
> 
> BR,
> Jani.
> 
> >
> > Signed-off-by: Imre Deak 
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c | 14 --
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> > b/drivers/gpu/drm/i915/intel_panel.c
> > index cb50c52..dbe05ec 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct 
> > intel_connector *connector)
> > struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
> > struct intel_panel *panel = >panel;
> > enum pipe pipe = intel_get_pipe_from_connector(connector);
> > -   enum transcoder cpu_transcoder =
> > -   intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > +   enum transcoder cpu_transcoder;
> > u32 cpu_ctl2, pch_ctl1, pch_ctl2;
> >  
> > +   if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +   return;
> 
> Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case,
> go on, and hope for the best. Do leave the warn in place though.
> 
> > +
> > +   cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> > +
> > cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
> > if (cpu_ctl2 & BLM_PWM_ENABLE) {
> > DRM_DEBUG_KMS("cpu backlight already enabled\n");
> > @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct 
> > intel_connector *connector)
> > enum pipe pipe = intel_get_pipe_from_connector(connector);
> > u32 ctl, ctl2, freq;
> >  
> > +   if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +   return;
> 
> Same here, please just assign e.g. PIPE_A here with the warning, and
> limp on.
> 
> > +
> > ctl2 = I915_READ(BLC_PWM_CTL2);
> > if (ctl2 & BLM_PWM_ENABLE) {
> > DRM_DEBUG_KMS("backlight already enabled\n");
> > @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct 
> > intel_connector *connector)
> > if (!panel->backlight.present)
> > return;
> >  
> > +   if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> > +   return;
> 
> Here, pipe is only used for the debug logging, just skip that instead.

Ok, can do. This would make things inconsistent wrt.
vlv_enable/disable_backlight() though, so are you ok if I change those
too accordingly?

--Imre

> 
> > +
> > DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
> >  
> > mutex_lock(_priv->backlight_lock);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 5/8] drm/i915: Check error return when converting pipe to connector

[Jani Nikula]

On Wed, 26 Apr 2017, Imre Deak  wrote:
> An error from intel_get_pipe_from_connector() would mean a bug somewhere
> else, but we still should check for it to prevent some other more
> obscure bug later.

Do check for invalid pipe, but please just limp on instead of bailing
out of the functions. See notes inline.

BR,
Jani.

>
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 14 --
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> b/drivers/gpu/drm/i915/intel_panel.c
> index cb50c52..dbe05ec 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector 
> *connector)
>   struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
>   struct intel_panel *panel = >panel;
>   enum pipe pipe = intel_get_pipe_from_connector(connector);
> - enum transcoder cpu_transcoder =
> - intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> + enum transcoder cpu_transcoder;
>   u32 cpu_ctl2, pch_ctl1, pch_ctl2;
>  
> + if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> + return;

Please just assign e.g. TRANSCODER_EDP to cpu_transcoder in this case,
go on, and hope for the best. Do leave the warn in place though.

> +
> + cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
> +
>   cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
>   if (cpu_ctl2 & BLM_PWM_ENABLE) {
>   DRM_DEBUG_KMS("cpu backlight already enabled\n");
> @@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector 
> *connector)
>   enum pipe pipe = intel_get_pipe_from_connector(connector);
>   u32 ctl, ctl2, freq;
>  
> + if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> + return;

Same here, please just assign e.g. PIPE_A here with the warning, and
limp on.

> +
>   ctl2 = I915_READ(BLC_PWM_CTL2);
>   if (ctl2 & BLM_PWM_ENABLE) {
>   DRM_DEBUG_KMS("backlight already enabled\n");
> @@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct 
> intel_connector *connector)
>   if (!panel->backlight.present)
>   return;
>  
> + if (WARN_ON_ONCE(pipe == INVALID_PIPE))
> + return;

Here, pipe is only used for the debug logging, just skip that instead.

> +
>   DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
>  
>   mutex_lock(_priv->backlight_lock);

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/8] drm/i915: Check error return when converting pipe to connector

[Imre Deak]

An error from intel_get_pipe_from_connector() would mean a bug somewhere
else, but we still should check for it to prevent some other more
obscure bug later.

Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/i915/intel_panel.c | 14 --
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index cb50c52..dbe05ec 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -888,10 +888,14 @@ static void pch_enable_backlight(struct intel_connector 
*connector)
struct drm_i915_private *dev_priv = to_i915(connector->base.dev);
struct intel_panel *panel = >panel;
enum pipe pipe = intel_get_pipe_from_connector(connector);
-   enum transcoder cpu_transcoder =
-   intel_pipe_to_cpu_transcoder(dev_priv, pipe);
+   enum transcoder cpu_transcoder;
u32 cpu_ctl2, pch_ctl1, pch_ctl2;
 
+   if (WARN_ON_ONCE(pipe == INVALID_PIPE))
+   return;
+
+   cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe);
+
cpu_ctl2 = I915_READ(BLC_PWM_CPU_CTL2);
if (cpu_ctl2 & BLM_PWM_ENABLE) {
DRM_DEBUG_KMS("cpu backlight already enabled\n");
@@ -973,6 +977,9 @@ static void i965_enable_backlight(struct intel_connector 
*connector)
enum pipe pipe = intel_get_pipe_from_connector(connector);
u32 ctl, ctl2, freq;
 
+   if (WARN_ON_ONCE(pipe == INVALID_PIPE))
+   return;
+
ctl2 = I915_READ(BLC_PWM_CTL2);
if (ctl2 & BLM_PWM_ENABLE) {
DRM_DEBUG_KMS("backlight already enabled\n");
@@ -1093,6 +1100,9 @@ void intel_panel_enable_backlight(struct intel_connector 
*connector)
if (!panel->backlight.present)
return;
 
+   if (WARN_ON_ONCE(pipe == INVALID_PIPE))
+   return;
+
DRM_DEBUG_KMS("pipe %c\n", pipe_name(pipe));
 
mutex_lock(_priv->backlight_lock);
-- 
2.5.0

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