Re: [PATCH v2 3/9] drm/i915: Enable default_phase in GCP when possible

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

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.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx