[Intel-gfx] [PATCH 2/4] drm/i915: Use drm_hdmi_avi_infoframe_quant_range() for SDVO HDMI as well

[Dhinakaran Pandiyan]

On Thu, 2018-12-13 at 15:09 -0800, Dhinakaran Pandiyan wrote:
> On Thu, 2018-12-13 at 07:18 +0200, Ville Syrjälä wrote:
> > On Wed, Dec 12, 2018 at 04:32:02PM -0800, Dhinakaran Pandiyan
> > wrote:
> > > On Tue, 2018-11-20 at 18:13 +0200, Ville Syrjala wrote:
> > > > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > 
> > > > Fill out the AVI infoframe quantization range bits using
> > > > drm_hdmi_avi_infoframe_quant_range() for SDVO HDMI encoder as
> > > > well.
> > > > 
> > > > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_sdvo.c | 19 ++++++++++---------
> > > >  1 file changed, 10 insertions(+), 9 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_sdvo.c
> > > > b/drivers/gpu/drm/i915/intel_sdvo.c
> > > > index 1277d31adb54..9c16e273fb8d 100644
> > > > --- a/drivers/gpu/drm/i915/intel_sdvo.c
> > > > +++ b/drivers/gpu/drm/i915/intel_sdvo.c
> > > > @@ -984,6 +984,8 @@ static bool
> > > > intel_sdvo_set_avi_infoframe(struct
> > > > intel_sdvo *intel_sdvo,
> > > >  					 const struct
> > > > intel_crtc_state
> > > > *pipe_config,
> > > >  					 const struct
> > > > drm_connector_state *conn_state)
> > > >  {
> > > > +	const struct drm_display_mode *adjusted_mode =
> > > > +		&pipe_config->base.adjusted_mode;
> > > >  	uint8_t sdvo_data[HDMI_INFOFRAME_SIZE(AVI)];
> > > >  	union hdmi_infoframe frame;
> > > >  	int ret;
> > > > @@ -991,20 +993,19 @@ static bool
> > > > intel_sdvo_set_avi_infoframe(struct
> > > > intel_sdvo *intel_sdvo,
> > > >  
> > > >  	ret =
> > > > drm_hdmi_avi_infoframe_from_display_mode(&frame.avi,
> > > >  						       conn_sta
> > > > te-
> > > > > connector,
> > > > 
> > > > -						       &pipe_co
> > > > nfig-
> > > > > base.adjusted_mode);
> > > > 
> > > > +						       adjusted
> > > > _mode);
> > > >  	if (ret < 0) {
> > > >  		DRM_ERROR("couldn't fill AVI infoframe\n");
> > > >  		return false;
> > > >  	}
> > > >  
> > > > -	if (intel_sdvo->rgb_quant_range_selectable) {
> > > > -		if (pipe_config->limited_color_range)
> > > > -			frame.avi.quantization_range =
> > > > -				HDMI_QUANTIZATION_RANGE_LIMITED
> > > > ;
> > > > -		else
> > > > -			frame.avi.quantization_range =
> > > > -				HDMI_QUANTIZATION_RANGE_FULL;
> > > > -	}
> > > > +	drm_hdmi_avi_infoframe_quant_range(&frame.avi,
> > > > +					   conn_state-
> > > > >connector,
> > > > +					   adjusted_mode,
> > > > +					   pipe_config-
> > > > > limited_color_range ?
> > > > 
> > > > +					   rgb_quant_range_sele
> > > > ctableTE
> > > > D :
> > > > +					   HDMI_QUANTIZATION_RA
> > > > NGE_FULL
> > > > ,
> > > > +					   intel_sdvo-
> > > > > rgb_quant_range_selectable);
> > > 
> > > Seems like avi.quantization_range can now get set to _LIMITED or
> > > _FULL
> > > even when ->rgb_quant_range_selectable == false, i.e., it is not
> > > _DEFAULT anymore. Is that change in behavior intended?
> > 
> > ->quant_range_selectable will be passed to
> > drm_hdmi_avi_infoframe_quant_range() which will do the right thing
> > with
> > it.
> > 
> > That said, there is a slight behavioural change in that it will set
> 
> Okay, I was indeed referring to this case.
> 
> > the Q bit even with QS==1 iff the quantization range matches the
> > default quantization range for the mode. I noted this in the radeon
> > patch but forgot to mention it here.
> 
> I'll let someone else with knowledge of HDMI to review this
> behavioral 
> change. I'm trying to get hold of the HDMI spec now and will review
> if
> this hasn't been looked at by then.
> 
Looks alright now that I went through the specs. With commit message
updated to make note of the Q value changes

Reviewed-by: Dhinakaran Pandiyan <dhinakaran.pandiyan at intel.com>
> 
> > 
> > > 
> > > 
> > > >  
> > > >  	len = hdmi_infoframe_pack(&frame, sdvo_data,
> > > > sizeof(sdvo_data));
> > > >  	if (len < 0)
> > 
> >