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

[Konduru, Chandra]

> > > > > @@ -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 at 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