| Message ID | 1430834787-10255-4-git-send-email-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> -----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
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
> > > @@ -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.
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.
> > > > > @@ -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
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;