diff mbox

[9/9] drm/i915: Assert dpll running in intel_crtc_load_lut() on pre-PCH platforms

Message ID 1370342947-20757-10-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä June 4, 2013, 10:49 a.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Rodrigo Vivi June 5, 2013, 7:41 p.m. UTC | #1 
why is this needed?
anyways: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

On Tue, Jun 4, 2013 at 7:49 AM,  <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 90d02c7..3be69bc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -6340,6 +6340,9 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
>         if (!crtc->enabled || !intel_crtc->active)
>                 return;
>
> +       if (!HAS_PCH_SPLIT(dev_priv->dev))
> +               assert_pll_enabled(dev_priv, pipe);
> +
>         /* use legacy palette for Ironlake */
>         if (HAS_PCH_SPLIT(dev))
>                 palreg = LGC_PALETTE(pipe);
> --
> 1.8.1.5
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä June 5, 2013, 7:58 p.m. UTC | #2 
On Wed, Jun 05, 2013 at 04:41:54PM -0300, Rodrigo Vivi wrote:
> why is this needed?

The spec says that on some hardware you need to PLL running before you
can poke at the palette registers. I didn't actually try to anger the
hardware so I'm not really sure what would happen otherwise, but IIRC
Jesse said something about a hard system hang...

> anyways: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> 
> On Tue, Jun 4, 2013 at 7:49 AM,  <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 90d02c7..3be69bc 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6340,6 +6340,9 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
> >         if (!crtc->enabled || !intel_crtc->active)
> >                 return;
> >
> > +       if (!HAS_PCH_SPLIT(dev_priv->dev))
> > +               assert_pll_enabled(dev_priv, pipe);
> > +
> >         /* use legacy palette for Ironlake */
> >         if (HAS_PCH_SPLIT(dev))
> >                 palreg = LGC_PALETTE(pipe);
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
Daniel Vetter June 5, 2013, 8:40 p.m. UTC | #3 
On Wed, Jun 05, 2013 at 04:41:54PM -0300, Rodrigo Vivi wrote:
> why is this needed?
> anyways: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>

An r-b is a formal statement like a s-o-b. From
Documentaion/SubmittingPatches:

	Reviewer's statement of oversight

	By offering my Reviewed-by: tag, I state that:

 	 (a) I have carried out a technical review of this patch to
	     evaluate its appropriateness and readiness for inclusion into
	     the mainline kernel.

	 (b) Any problems, concerns, or questions relating to the patch
	     have been communicated back to the submitter.  I am satisfied
	     with the submitter's response to my comments.

	 (c) While there may be things that could be improved with this
	     submission, I believe that it is, at this time, (1) a
	     worthwhile modification to the kernel, and (2) free of known
	     issues which would argue against its inclusion.

	 (d) While I have reviewed the patch and believe it to be sound, I
	     do not (unless explicitly stated elsewhere) make any
	     warranties or guarantees that it will achieve its stated
	     purpose or function properly in any given situation.

If you're asking "why is this needed?" it does _not_ deserve an r-b on
a pretty fundamental level since it clearly violates c) 1) above.

Please don't just go through the motions when doing review.

Thanks, Daniel

> 
> On Tue, Jun 4, 2013 at 7:49 AM,  <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 3 +++
> >  1 file changed, 3 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 90d02c7..3be69bc 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6340,6 +6340,9 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
> >         if (!crtc->enabled || !intel_crtc->active)
> >                 return;
> >
> > +       if (!HAS_PCH_SPLIT(dev_priv->dev))
> > +               assert_pll_enabled(dev_priv, pipe);
> > +
> >         /* use legacy palette for Ironlake */
> >         if (HAS_PCH_SPLIT(dev))
> >                 palreg = LGC_PALETTE(pipe);
> > --
> > 1.8.1.5
> >
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> 
> 
> -- 
> Rodrigo Vivi
> Blog: http://blog.vivi.eng.br
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Daniel Vetter June 6, 2013, 11:59 a.m. UTC | #4 
On Wed, Jun 05, 2013 at 10:58:06PM +0300, Ville Syrjälä wrote:
> On Wed, Jun 05, 2013 at 04:41:54PM -0300, Rodrigo Vivi wrote:
> > why is this needed?
> 
> The spec says that on some hardware you need to PLL running before you
> can poke at the palette registers. I didn't actually try to anger the
> hardware so I'm not really sure what would happen otherwise, but IIRC
> Jesse said something about a hard system hang...

I've added this to the commit message and merged the entire pile. Thanks
for patches&review.
-Daniel

> 
> > anyways: Reviewed-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> > 
> > On Tue, Jun 4, 2013 at 7:49 AM,  <ville.syrjala@linux.intel.com> wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index 90d02c7..3be69bc 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -6340,6 +6340,9 @@ void intel_crtc_load_lut(struct drm_crtc *crtc)
> > >         if (!crtc->enabled || !intel_crtc->active)
> > >                 return;
> > >
> > > +       if (!HAS_PCH_SPLIT(dev_priv->dev))
> > > +               assert_pll_enabled(dev_priv, pipe);
> > > +
> > >         /* use legacy palette for Ironlake */
> > >         if (HAS_PCH_SPLIT(dev))
> > >                 palreg = LGC_PALETTE(pipe);
> > > --
> > > 1.8.1.5
> > >
> > > _______________________________________________
> > > Intel-gfx mailing list
> > > Intel-gfx@lists.freedesktop.org
> > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 
> > 
> > 
> > -- 
> > Rodrigo Vivi
> > Blog: http://blog.vivi.eng.br
> 
> -- 
> Ville Syrjälä
> Intel OTC
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 90d02c7..3be69bc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6340,6 +6340,9 @@  void intel_crtc_load_lut(struct drm_crtc *crtc)
 	if (!crtc->enabled || !intel_crtc->active)
 		return;
 
+	if (!HAS_PCH_SPLIT(dev_priv->dev))
+		assert_pll_enabled(dev_priv, pipe);
+
 	/* use legacy palette for Ironlake */
 	if (HAS_PCH_SPLIT(dev))
 		palreg = LGC_PALETTE(pipe);