* Re: [PATCH] drm/i915: ensure HDMI port is disabled inside set_infoframes
2012-06-12 14:36 [PATCH] drm/i915: ensure HDMI port is disabled inside set_infoframes Daniel Vetter
@ 2012-06-12 14:35 ` Paulo Zanoni
2012-06-12 15:53 ` Eugeni Dodonov
2012-06-12 15:43 ` Eugeni Dodonov
1 sibling, 1 reply; 5+ messages in thread
From: Paulo Zanoni @ 2012-06-12 14:35 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Paulo Zanoni
Hi
2012/6/12 Daniel Vetter <daniel.vetter@ffwll.ch>:
> This function is supposed to be used at mode set time, so prevent
> against future mistakes by adding a WARN().
>
> Based on a patch by Paulo Zanoni, with the check extracted into a
> little assert_hdmi_port_disabled helper added to make things self
> documenting and move the assert stuff out of line.
I was going to write the patch, but thanks for doing it :)
Comments below:
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
> drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++++++++++
> 1 files changed, 23 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> index 69637db..aac9a5d 100644
> --- a/drivers/gpu/drm/i915/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> @@ -37,6 +37,19 @@
> #include "i915_drm.h"
> #include "i915_drv.h"
>
> +static void
> +assert_hdmi_port_disabled(struct intel_hdmi *intel_hdmi)
> +{
> + struct drm_device *dev = intel_hdmi->base.base.dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + uint32_t enabled_bits;
> +
> + enabled_bits = IS_HASWELL(dev) ? DDI_BUF_CTL_ENABLE : SDVO_ENABLE;
My crystal ball tells me a check for IS_HASWELL_OR_NEWER would prevent
more changes to this code in the future.
> +
> + WARN(I915_READ(intel_hdmi->sdvox_reg) & enabled_bits,
> + "HDMI port enabled, expectin disabled\n");
expectinG?
> +}
> +
> struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder)
> {
> return container_of(encoder, struct intel_hdmi, base.base);
> @@ -334,6 +347,8 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
> u32 val = I915_READ(reg);
> u32 port;
>
> + assert_hdmi_port_disabled(intel_hdmi);
> +
> /* If the registers were not initialized yet, they might be zeroes,
> * which means we're selecting the AVI DIP and we're setting its
> * frequency to once. This seems to really confuse the HW and make
> @@ -395,6 +410,8 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
> u32 val = I915_READ(reg);
> u32 port;
>
> + assert_hdmi_port_disabled(intel_hdmi);
> +
> /* See the big comment in g4x_set_infoframes() */
> val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
>
> @@ -451,6 +468,8 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
> u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
> u32 val = I915_READ(reg);
>
> + assert_hdmi_port_disabled(intel_hdmi);
> +
> /* See the big comment in g4x_set_infoframes() */
> val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
>
> @@ -484,6 +503,8 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
> u32 reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
> u32 val = I915_READ(reg);
>
> + assert_hdmi_port_disabled(intel_hdmi);
> +
> /* See the big comment in g4x_set_infoframes() */
> val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
>
> @@ -516,6 +537,8 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
> u32 reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
> u32 val = I915_READ(reg);
>
> + assert_hdmi_port_disabled(intel_hdmi);
> +
> if (!intel_hdmi->has_hdmi_sink) {
> I915_WRITE(reg, 0);
> POSTING_READ(reg);
> --
> 1.7.7.6
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] drm/i915: ensure HDMI port is disabled inside set_infoframes
@ 2012-06-12 14:36 Daniel Vetter
2012-06-12 14:35 ` Paulo Zanoni
2012-06-12 15:43 ` Eugeni Dodonov
0 siblings, 2 replies; 5+ messages in thread
From: Daniel Vetter @ 2012-06-12 14:36 UTC (permalink / raw)
To: Intel Graphics Development; +Cc: Daniel Vetter, Paulo Zanoni
This function is supposed to be used at mode set time, so prevent
against future mistakes by adding a WARN().
Based on a patch by Paulo Zanoni, with the check extracted into a
little assert_hdmi_port_disabled helper added to make things self
documenting and move the assert stuff out of line.
Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
drivers/gpu/drm/i915/intel_hdmi.c | 23 +++++++++++++++++++++++
1 files changed, 23 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 69637db..aac9a5d 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -37,6 +37,19 @@
#include "i915_drm.h"
#include "i915_drv.h"
+static void
+assert_hdmi_port_disabled(struct intel_hdmi *intel_hdmi)
+{
+ struct drm_device *dev = intel_hdmi->base.base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ uint32_t enabled_bits;
+
+ enabled_bits = IS_HASWELL(dev) ? DDI_BUF_CTL_ENABLE : SDVO_ENABLE;
+
+ WARN(I915_READ(intel_hdmi->sdvox_reg) & enabled_bits,
+ "HDMI port enabled, expectin disabled\n");
+}
+
struct intel_hdmi *enc_to_intel_hdmi(struct drm_encoder *encoder)
{
return container_of(encoder, struct intel_hdmi, base.base);
@@ -334,6 +347,8 @@ static void g4x_set_infoframes(struct drm_encoder *encoder,
u32 val = I915_READ(reg);
u32 port;
+ assert_hdmi_port_disabled(intel_hdmi);
+
/* If the registers were not initialized yet, they might be zeroes,
* which means we're selecting the AVI DIP and we're setting its
* frequency to once. This seems to really confuse the HW and make
@@ -395,6 +410,8 @@ static void ibx_set_infoframes(struct drm_encoder *encoder,
u32 val = I915_READ(reg);
u32 port;
+ assert_hdmi_port_disabled(intel_hdmi);
+
/* See the big comment in g4x_set_infoframes() */
val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
@@ -451,6 +468,8 @@ static void cpt_set_infoframes(struct drm_encoder *encoder,
u32 reg = TVIDEO_DIP_CTL(intel_crtc->pipe);
u32 val = I915_READ(reg);
+ assert_hdmi_port_disabled(intel_hdmi);
+
/* See the big comment in g4x_set_infoframes() */
val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
@@ -484,6 +503,8 @@ static void vlv_set_infoframes(struct drm_encoder *encoder,
u32 reg = VLV_TVIDEO_DIP_CTL(intel_crtc->pipe);
u32 val = I915_READ(reg);
+ assert_hdmi_port_disabled(intel_hdmi);
+
/* See the big comment in g4x_set_infoframes() */
val |= VIDEO_DIP_SELECT_AVI | VIDEO_DIP_FREQ_VSYNC;
@@ -516,6 +537,8 @@ static void hsw_set_infoframes(struct drm_encoder *encoder,
u32 reg = HSW_TVIDEO_DIP_CTL(intel_crtc->pipe);
u32 val = I915_READ(reg);
+ assert_hdmi_port_disabled(intel_hdmi);
+
if (!intel_hdmi->has_hdmi_sink) {
I915_WRITE(reg, 0);
POSTING_READ(reg);
--
1.7.7.6
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: ensure HDMI port is disabled inside set_infoframes
2012-06-12 14:36 [PATCH] drm/i915: ensure HDMI port is disabled inside set_infoframes Daniel Vetter
2012-06-12 14:35 ` Paulo Zanoni
@ 2012-06-12 15:43 ` Eugeni Dodonov
2012-06-12 17:21 ` Daniel Vetter
1 sibling, 1 reply; 5+ messages in thread
From: Eugeni Dodonov @ 2012-06-12 15:43 UTC (permalink / raw)
To: Daniel Vetter; +Cc: Intel Graphics Development, Paulo Zanoni
On 06/12/2012 11:36 AM, Daniel Vetter wrote:
> This function is supposed to be used at mode set time, so prevent
> against future mistakes by adding a WARN().
>
> Based on a patch by Paulo Zanoni, with the check extracted into a
> little assert_hdmi_port_disabled helper added to make things self
> documenting and move the assert stuff out of line.
>
> Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Apart from a typo in 'expectin' below:
Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> + WARN(I915_READ(intel_hdmi->sdvox_reg) & enabled_bits,
> + "HDMI port enabled, expectin disabled\n");
^^
Eugeni
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: ensure HDMI port is disabled inside set_infoframes
2012-06-12 14:35 ` Paulo Zanoni
@ 2012-06-12 15:53 ` Eugeni Dodonov
0 siblings, 0 replies; 5+ messages in thread
From: Eugeni Dodonov @ 2012-06-12 15:53 UTC (permalink / raw)
To: Paulo Zanoni; +Cc: Daniel Vetter, Intel Graphics Development, Paulo Zanoni
On 06/12/2012 11:35 AM, Paulo Zanoni wrote:
> My crystal ball tells me a check for IS_HASWELL_OR_NEWER would prevent
> more changes to this code in the future.
I think that a .has_ddi (or similar) flag would be better than checking
for post-Haswell variants, as not necessarily all of them could work in
the same way. But we are still far far away from having to face this
case case anyway :).
Eugeni
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/i915: ensure HDMI port is disabled inside set_infoframes
2012-06-12 15:43 ` Eugeni Dodonov
@ 2012-06-12 17:21 ` Daniel Vetter
0 siblings, 0 replies; 5+ messages in thread
From: Daniel Vetter @ 2012-06-12 17:21 UTC (permalink / raw)
To: eugeni.dodonov; +Cc: Daniel Vetter, Intel Graphics Development, Paulo Zanoni
On Tue, Jun 12, 2012 at 12:43:42PM -0300, Eugeni Dodonov wrote:
> On 06/12/2012 11:36 AM, Daniel Vetter wrote:
> > This function is supposed to be used at mode set time, so prevent
> > against future mistakes by adding a WARN().
> >
> > Based on a patch by Paulo Zanoni, with the check extracted into a
> > little assert_hdmi_port_disabled helper added to make things self
> > documenting and move the assert stuff out of line.
> >
> > Cc: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > Signed-Off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>
> Apart from a typo in 'expectin' below:
>
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Queued for -next with the 'g' thrown in, thanks for the review.
-Daniel
--
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2012-06-12 17:19 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-06-12 14:36 [PATCH] drm/i915: ensure HDMI port is disabled inside set_infoframes Daniel Vetter
2012-06-12 14:35 ` Paulo Zanoni
2012-06-12 15:53 ` Eugeni Dodonov
2012-06-12 15:43 ` Eugeni Dodonov
2012-06-12 17:21 ` Daniel Vetter
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.