| Message ID | 20181120161345.15440-2-ville.syrjala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] drm/edid: Pass connector to AVI inforframe functions | expand |
On Tue, 2018-11-20 at 18:13 +0200, Ville Syrjala wrote: > From: Ville Syrjälä <ville.syrjala@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@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_state- > >connector, > - &pipe_config- > >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_selectableTE > D : > + HDMI_QUANTIZATION_RANGE_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? > > len = hdmi_infoframe_pack(&frame, sdvo_data, > sizeof(sdvo_data)); > if (len < 0)
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@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@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_state- > > >connector, > > - &pipe_config- > > >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_selectableTE > > D : > > + HDMI_QUANTIZATION_RANGE_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 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. > > > > > > len = hdmi_infoframe_pack(&frame, sdvo_data, > > sizeof(sdvo_data)); > > if (len < 0)
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@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@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_state- > > > > connector, > > > > > > - &pipe_config- > > > > 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_selectableTE > > > D : > > > + HDMI_QUANTIZATION_RANGE_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. > > > > > > > > > > > len = hdmi_infoframe_pack(&frame, sdvo_data, > > > sizeof(sdvo_data)); > > > if (len < 0) > >
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@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@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@intel.com> > > > > > > > > > > > > > > > > > len = hdmi_infoframe_pack(&frame, sdvo_data, > > > > sizeof(sdvo_data)); > > > > if (len < 0) > > > >
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_state->connector, - &pipe_config->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 ? + HDMI_QUANTIZATION_RANGE_LIMITED : + HDMI_QUANTIZATION_RANGE_FULL, + intel_sdvo->rgb_quant_range_selectable); len = hdmi_infoframe_pack(&frame, sdvo_data, sizeof(sdvo_data)); if (len < 0)