[v2,3/9] drm/i915: Enable default_phase in GCP when possible
diff mbox

Message ID 1430834787-10255-4-git-send-email-ville.syrjala@linux.intel.com
State New
Headers show

Commit Message

Ville Syrjälä May 5, 2015, 2:06 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

When the video timings are suitably aligned so that all different
periods start at phase 0 (ie. none of the periods start mid-pixel)
we can inform the sink about this. Supposedly the sink can then
optimize certain things. Obviously this is only relevant when
outputting >8bpc data since otherwise there are no mid-pixel phases.

v2: Rebased due to crtc->config changes

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

Comments

Chandra Konduru June 1, 2015, 9:49 p.m. UTC | #1
> -----Original Message-----

> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of

> ville.syrjala@linux.intel.com

> Sent: Tuesday, May 05, 2015 7:06 AM

> To: intel-gfx@lists.freedesktop.org

> Subject: [Intel-gfx] [PATCH v2 3/9] drm/i915: Enable default_phase in GCP when

> possible

> 

> From: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 

> When the video timings are suitably aligned so that all different periods start at

> phase 0 (ie. none of the periods start mid-pixel) we can inform the sink about

> this. Supposedly the sink can then optimize certain things. Obviously this is only

> relevant when outputting >8bpc data since otherwise there are no mid-pixel

> phases.

> 

> v2: Rebased due to crtc->config changes

> 

> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---

>  drivers/gpu/drm/i915/intel_hdmi.c | 48

> +++++++++++++++++++++++++++++++++++++++

>  1 file changed, 48 insertions(+)

> 

> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c

> b/drivers/gpu/drm/i915/intel_hdmi.c

> index 87c4905..2e98e33 100644

> --- a/drivers/gpu/drm/i915/intel_hdmi.c

> +++ b/drivers/gpu/drm/i915/intel_hdmi.c

> @@ -560,6 +560,49 @@ static bool hdmi_sink_is_deep_color(struct

> drm_encoder *encoder)

>  	return false;

>  }

> 

> +/*

> + * Determine if default_phase=1 can be indicated in the GCP infoframe.

> + *

> + * From HDMI specification 1.4a:

> + * - The first pixel of each Video Data Period shall always have a

> +pixel packing phase of 0

> + * - The first pixel following each Video Data Period shall have a

> +pixel packing phase of 0

> + * - The PP bits shall be constant for all GCPs and will be equal to

> +the last packing phase

> + * - The first pixel following every transition of HSYNC or VSYNC shall have a

> pixel packing

> + *   phase of 0

> + */

> +static bool gcp_default_phase_possible(int pipe_bpp,

> +				       const struct drm_display_mode *mode) {

> +	unsigned int pixels_per_group;

> +

> +	switch (pipe_bpp) {

> +	case 30:

> +		/* 4 pixels in 5 clocks */

> +		pixels_per_group = 4;

> +		break;

> +	case 36:

> +		/* 2 pixels in 3 clocks */

> +		pixels_per_group = 2;

> +		break;

> +	case 48:

> +		/* 1 pixel in 2 clocks */

> +		pixels_per_group = 1;

> +		break;

> +	default:

> +		/* phase information not relevant for 8bpc */

> +		return false;

> +	}

> +

> +	return mode->crtc_hdisplay % pixels_per_group == 0 &&

> +		mode->crtc_htotal % pixels_per_group == 0 &&

> +		mode->crtc_hblank_start % pixels_per_group == 0 &&

> +		mode->crtc_hblank_end % pixels_per_group == 0 &&

> +		mode->crtc_hsync_start % pixels_per_group == 0 &&

> +		mode->crtc_hsync_end % pixels_per_group == 0 &&


For hsync, bspec says Hsync is an even number.
Isn't it above check should be something like (hsync_end - hsync_start) % 2 == 0?
And similarly for front & back porches, right? 

> +		((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0 ||

> +		 mode->crtc_htotal/2 % pixels_per_group == 0); }

> +

>  static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)  {

>  	struct drm_i915_private *dev_priv = encoder->dev->dev_private; @@ -

> 579,6 +622,11 @@ static bool intel_hdmi_set_gcp_infoframe(struct

> drm_encoder *encoder)

>  	if (hdmi_sink_is_deep_color(encoder))

>  		val |= GCP_COLOR_INDICATION;

> 

> +	/* Enable default_phase whenever the display mode is suitably aligned

> */

> +	if (gcp_default_phase_possible(crtc->config->pipe_bpp,

> +				       &crtc->config->base.adjusted_mode))

> +		val |= GCP_DEFAULT_PHASE_ENABLE;

> +

>  	I915_WRITE(reg, val);

> 

>  	return val != 0;

> --

> 2.0.5

> 

> _______________________________________________

> Intel-gfx mailing list

> Intel-gfx@lists.freedesktop.org

> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä June 2, 2015, 11:46 a.m. UTC | #2
On Mon, Jun 01, 2015 at 09:49:53PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf Of
> > ville.syrjala@linux.intel.com
> > Sent: Tuesday, May 05, 2015 7:06 AM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH v2 3/9] drm/i915: Enable default_phase in GCP when
> > possible
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > When the video timings are suitably aligned so that all different periods start at
> > phase 0 (ie. none of the periods start mid-pixel) we can inform the sink about
> > this. Supposedly the sink can then optimize certain things. Obviously this is only
> > relevant when outputting >8bpc data since otherwise there are no mid-pixel
> > phases.
> > 
> > v2: Rebased due to crtc->config changes
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_hdmi.c | 48
> > +++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 48 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c
> > b/drivers/gpu/drm/i915/intel_hdmi.c
> > index 87c4905..2e98e33 100644
> > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > @@ -560,6 +560,49 @@ static bool hdmi_sink_is_deep_color(struct
> > drm_encoder *encoder)
> >  	return false;
> >  }
> > 
> > +/*
> > + * Determine if default_phase=1 can be indicated in the GCP infoframe.
> > + *
> > + * From HDMI specification 1.4a:
> > + * - The first pixel of each Video Data Period shall always have a
> > +pixel packing phase of 0
> > + * - The first pixel following each Video Data Period shall have a
> > +pixel packing phase of 0
> > + * - The PP bits shall be constant for all GCPs and will be equal to
> > +the last packing phase
> > + * - The first pixel following every transition of HSYNC or VSYNC shall have a
> > pixel packing
> > + *   phase of 0
> > + */
> > +static bool gcp_default_phase_possible(int pipe_bpp,
> > +				       const struct drm_display_mode *mode) {
> > +	unsigned int pixels_per_group;
> > +
> > +	switch (pipe_bpp) {
> > +	case 30:
> > +		/* 4 pixels in 5 clocks */
> > +		pixels_per_group = 4;
> > +		break;
> > +	case 36:
> > +		/* 2 pixels in 3 clocks */
> > +		pixels_per_group = 2;
> > +		break;
> > +	case 48:
> > +		/* 1 pixel in 2 clocks */
> > +		pixels_per_group = 1;
> > +		break;
> > +	default:
> > +		/* phase information not relevant for 8bpc */
> > +		return false;
> > +	}
> > +
> > +	return mode->crtc_hdisplay % pixels_per_group == 0 &&
> > +		mode->crtc_htotal % pixels_per_group == 0 &&
> > +		mode->crtc_hblank_start % pixels_per_group == 0 &&
> > +		mode->crtc_hblank_end % pixels_per_group == 0 &&
> > +		mode->crtc_hsync_start % pixels_per_group == 0 &&
> > +		mode->crtc_hsync_end % pixels_per_group == 0 &&
> 
> For hsync, bspec says Hsync is an even number.
> Isn't it above check should be something like (hsync_end - hsync_start) % 2 == 0?
> And similarly for front & back porches, right? 

If X and Y are even then (X - Y) is even too. Also the text in bspec is
less informative than the text in HDMI spec, which is why I quited the
HDMI spec instead.

> 
> > +		((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0 ||
> > +		 mode->crtc_htotal/2 % pixels_per_group == 0); }
> > +
> >  static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)  {
> >  	struct drm_i915_private *dev_priv = encoder->dev->dev_private; @@ -
> > 579,6 +622,11 @@ static bool intel_hdmi_set_gcp_infoframe(struct
> > drm_encoder *encoder)
> >  	if (hdmi_sink_is_deep_color(encoder))
> >  		val |= GCP_COLOR_INDICATION;
> > 
> > +	/* Enable default_phase whenever the display mode is suitably aligned
> > */
> > +	if (gcp_default_phase_possible(crtc->config->pipe_bpp,
> > +				       &crtc->config->base.adjusted_mode))
> > +		val |= GCP_DEFAULT_PHASE_ENABLE;
> > +
> >  	I915_WRITE(reg, val);
> > 
> >  	return val != 0;
> > --
> > 2.0.5
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Chandra Konduru June 2, 2015, 6:21 p.m. UTC | #3
> > > @@ -560,6 +560,49 @@ static bool hdmi_sink_is_deep_color(struct
> > > drm_encoder *encoder)
> > >  	return false;
> > >  }
> > >
> > > +/*
> > > + * Determine if default_phase=1 can be indicated in the GCP infoframe.
> > > + *
> > > + * From HDMI specification 1.4a:
> > > + * - The first pixel of each Video Data Period shall always have a
> > > +pixel packing phase of 0
> > > + * - The first pixel following each Video Data Period shall have a
> > > +pixel packing phase of 0
> > > + * - The PP bits shall be constant for all GCPs and will be equal to
> > > +the last packing phase
> > > + * - The first pixel following every transition of HSYNC or VSYNC shall have a
> > > pixel packing
> > > + *   phase of 0
> > > + */
> > > +static bool gcp_default_phase_possible(int pipe_bpp,
> > > +				       const struct drm_display_mode *mode) {
> > > +	unsigned int pixels_per_group;
> > > +
> > > +	switch (pipe_bpp) {
> > > +	case 30:
> > > +		/* 4 pixels in 5 clocks */
> > > +		pixels_per_group = 4;
> > > +		break;
> > > +	case 36:
> > > +		/* 2 pixels in 3 clocks */
> > > +		pixels_per_group = 2;
> > > +		break;
> > > +	case 48:
> > > +		/* 1 pixel in 2 clocks */
> > > +		pixels_per_group = 1;
> > > +		break;
> > > +	default:
> > > +		/* phase information not relevant for 8bpc */
> > > +		return false;
> > > +	}
> > > +
> > > +	return mode->crtc_hdisplay % pixels_per_group == 0 &&
> > > +		mode->crtc_htotal % pixels_per_group == 0 &&
> > > +		mode->crtc_hblank_start % pixels_per_group == 0 &&
> > > +		mode->crtc_hblank_end % pixels_per_group == 0 &&
> > > +		mode->crtc_hsync_start % pixels_per_group == 0 &&
> > > +		mode->crtc_hsync_end % pixels_per_group == 0 &&
> >
> > For hsync, bspec says Hsync is an even number.
> > Isn't it above check should be something like (hsync_end - hsync_start) % 2 ==
> 0?
> > And similarly for front & back porches, right?
> 
> If X and Y are even then (X - Y) is even too. Also the text in bspec is
> less informative than the text in HDMI spec, which is why I quited the
> HDMI spec instead.

Sure, if X and Y are even X - Y is even, but it is more restrictive check than
needed. Because if both X and Y are odd, X - Y is even, and in that case
above code doesn't allow to use default phase. Which may be OK, but
it didn't truly allow default phase when possible.
Ville Syrjälä June 3, 2015, 9:34 a.m. UTC | #4
On Tue, Jun 02, 2015 at 06:21:59PM +0000, Konduru, Chandra wrote:
> > > > @@ -560,6 +560,49 @@ static bool hdmi_sink_is_deep_color(struct
> > > > drm_encoder *encoder)
> > > >  	return false;
> > > >  }
> > > >
> > > > +/*
> > > > + * Determine if default_phase=1 can be indicated in the GCP infoframe.
> > > > + *
> > > > + * From HDMI specification 1.4a:
> > > > + * - The first pixel of each Video Data Period shall always have a
> > > > +pixel packing phase of 0
> > > > + * - The first pixel following each Video Data Period shall have a
> > > > +pixel packing phase of 0
> > > > + * - The PP bits shall be constant for all GCPs and will be equal to
> > > > +the last packing phase
> > > > + * - The first pixel following every transition of HSYNC or VSYNC shall have a
> > > > pixel packing
> > > > + *   phase of 0
> > > > + */
> > > > +static bool gcp_default_phase_possible(int pipe_bpp,
> > > > +				       const struct drm_display_mode *mode) {
> > > > +	unsigned int pixels_per_group;
> > > > +
> > > > +	switch (pipe_bpp) {
> > > > +	case 30:
> > > > +		/* 4 pixels in 5 clocks */
> > > > +		pixels_per_group = 4;
> > > > +		break;
> > > > +	case 36:
> > > > +		/* 2 pixels in 3 clocks */
> > > > +		pixels_per_group = 2;
> > > > +		break;
> > > > +	case 48:
> > > > +		/* 1 pixel in 2 clocks */
> > > > +		pixels_per_group = 1;
> > > > +		break;
> > > > +	default:
> > > > +		/* phase information not relevant for 8bpc */
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	return mode->crtc_hdisplay % pixels_per_group == 0 &&
> > > > +		mode->crtc_htotal % pixels_per_group == 0 &&
> > > > +		mode->crtc_hblank_start % pixels_per_group == 0 &&
> > > > +		mode->crtc_hblank_end % pixels_per_group == 0 &&
> > > > +		mode->crtc_hsync_start % pixels_per_group == 0 &&
> > > > +		mode->crtc_hsync_end % pixels_per_group == 0 &&
> > >
> > > For hsync, bspec says Hsync is an even number.
> > > Isn't it above check should be something like (hsync_end - hsync_start) % 2 ==
> > 0?
> > > And similarly for front & back porches, right?
> > 
> > If X and Y are even then (X - Y) is even too. Also the text in bspec is
> > less informative than the text in HDMI spec, which is why I quited the
> > HDMI spec instead.
> 
> Sure, if X and Y are even X - Y is even, but it is more restrictive check than
> needed. Because if both X and Y are odd, X - Y is even, and in that case
> above code doesn't allow to use default phase. Which may be OK, but
> it didn't truly allow default phase when possible.

Default phase should not be enabled in that case. As the HDMI spec says
"The first pixel following every transition of HSYNC or VSYNC shall have
a pixel packing phase of 0", so having a misaligned hsync_start or
hsync_end is not allowed.

Or you can also read bspec. While the text is less clear there IMO it
does disallow the case you outlined. What bspec does say is this:
"Htotal is an even number
 Hactive is an even number
 Hsync is an even number
 Front and back porches for Hsync are even numbers
 Vsync always starts on an even-numbered pixel within a line in
 interlaced modes (starting counting with 0)"

That doesn't allow hsync_start or hsync_end to be odd either.
Chandra Konduru June 3, 2015, 8:38 p.m. UTC | #5
> > > > > @@ -560,6 +560,49 @@ static bool hdmi_sink_is_deep_color(struct
> > > > > drm_encoder *encoder)
> > > > >  	return false;
> > > > >  }
> > > > >
> > > > > +/*
> > > > > + * Determine if default_phase=1 can be indicated in the GCP infoframe.
> > > > > + *
> > > > > + * From HDMI specification 1.4a:
> > > > > + * - The first pixel of each Video Data Period shall always have a
> > > > > +pixel packing phase of 0
> > > > > + * - The first pixel following each Video Data Period shall have a
> > > > > +pixel packing phase of 0
> > > > > + * - The PP bits shall be constant for all GCPs and will be equal to
> > > > > +the last packing phase
> > > > > + * - The first pixel following every transition of HSYNC or VSYNC shall
> have a
> > > > > pixel packing
> > > > > + *   phase of 0
> > > > > + */
> > > > > +static bool gcp_default_phase_possible(int pipe_bpp,
> > > > > +				       const struct drm_display_mode
> *mode) {
> > > > > +	unsigned int pixels_per_group;
> > > > > +
> > > > > +	switch (pipe_bpp) {
> > > > > +	case 30:
> > > > > +		/* 4 pixels in 5 clocks */
> > > > > +		pixels_per_group = 4;
> > > > > +		break;
> > > > > +	case 36:
> > > > > +		/* 2 pixels in 3 clocks */
> > > > > +		pixels_per_group = 2;
> > > > > +		break;
> > > > > +	case 48:
> > > > > +		/* 1 pixel in 2 clocks */
> > > > > +		pixels_per_group = 1;
> > > > > +		break;
> > > > > +	default:
> > > > > +		/* phase information not relevant for 8bpc */
> > > > > +		return false;
> > > > > +	}
> > > > > +
> > > > > +	return mode->crtc_hdisplay % pixels_per_group == 0 &&
> > > > > +		mode->crtc_htotal % pixels_per_group == 0 &&
> > > > > +		mode->crtc_hblank_start % pixels_per_group == 0 &&
> > > > > +		mode->crtc_hblank_end % pixels_per_group == 0 &&
> > > > > +		mode->crtc_hsync_start % pixels_per_group == 0 &&
> > > > > +		mode->crtc_hsync_end % pixels_per_group == 0 &&
> > > >
> > > > For hsync, bspec says Hsync is an even number.
> > > > Isn't it above check should be something like (hsync_end - hsync_start) % 2
> ==
> > > 0?
> > > > And similarly for front & back porches, right?
> > >
> > > If X and Y are even then (X - Y) is even too. Also the text in bspec is
> > > less informative than the text in HDMI spec, which is why I quited the
> > > HDMI spec instead.
> >
> > Sure, if X and Y are even X - Y is even, but it is more restrictive check than
> > needed. Because if both X and Y are odd, X - Y is even, and in that case
> > above code doesn't allow to use default phase. Which may be OK, but
> > it didn't truly allow default phase when possible.
> 
> Default phase should not be enabled in that case. As the HDMI spec says
> "The first pixel following every transition of HSYNC or VSYNC shall have
> a pixel packing phase of 0", so having a misaligned hsync_start or
> hsync_end is not allowed.

OK. Thanks for clarification!
With that, it gets
Reviewed-by: Chandra Konduru <Chandra.konduru@intel.com>

> 
> Or you can also read bspec. While the text is less clear there IMO it
> does disallow the case you outlined. What bspec does say is this:
> "Htotal is an even number
>  Hactive is an even number
>  Hsync is an even number
>  Front and back porches for Hsync are even numbers
>  Vsync always starts on an even-numbered pixel within a line in
>  interlaced modes (starting counting with 0)"
> 
> That doesn't allow hsync_start or hsync_end to be odd either.
> 
> --
> Ville Syrjälä
> Intel OTC

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
index 87c4905..2e98e33 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -560,6 +560,49 @@  static bool hdmi_sink_is_deep_color(struct drm_encoder *encoder)
 	return false;
 }
 
+/*
+ * Determine if default_phase=1 can be indicated in the GCP infoframe.
+ *
+ * From HDMI specification 1.4a:
+ * - The first pixel of each Video Data Period shall always have a pixel packing phase of 0
+ * - The first pixel following each Video Data Period shall have a pixel packing phase of 0
+ * - The PP bits shall be constant for all GCPs and will be equal to the last packing phase
+ * - The first pixel following every transition of HSYNC or VSYNC shall have a pixel packing
+ *   phase of 0
+ */
+static bool gcp_default_phase_possible(int pipe_bpp,
+				       const struct drm_display_mode *mode)
+{
+	unsigned int pixels_per_group;
+
+	switch (pipe_bpp) {
+	case 30:
+		/* 4 pixels in 5 clocks */
+		pixels_per_group = 4;
+		break;
+	case 36:
+		/* 2 pixels in 3 clocks */
+		pixels_per_group = 2;
+		break;
+	case 48:
+		/* 1 pixel in 2 clocks */
+		pixels_per_group = 1;
+		break;
+	default:
+		/* phase information not relevant for 8bpc */
+		return false;
+	}
+
+	return mode->crtc_hdisplay % pixels_per_group == 0 &&
+		mode->crtc_htotal % pixels_per_group == 0 &&
+		mode->crtc_hblank_start % pixels_per_group == 0 &&
+		mode->crtc_hblank_end % pixels_per_group == 0 &&
+		mode->crtc_hsync_start % pixels_per_group == 0 &&
+		mode->crtc_hsync_end % pixels_per_group == 0 &&
+		((mode->flags & DRM_MODE_FLAG_INTERLACE) == 0 ||
+		 mode->crtc_htotal/2 % pixels_per_group == 0);
+}
+
 static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
 {
 	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
@@ -579,6 +622,11 @@  static bool intel_hdmi_set_gcp_infoframe(struct drm_encoder *encoder)
 	if (hdmi_sink_is_deep_color(encoder))
 		val |= GCP_COLOR_INDICATION;
 
+	/* Enable default_phase whenever the display mode is suitably aligned */
+	if (gcp_default_phase_possible(crtc->config->pipe_bpp,
+				       &crtc->config->base.adjusted_mode))
+		val |= GCP_DEFAULT_PHASE_ENABLE;
+
 	I915_WRITE(reg, val);
 
 	return val != 0;