| Message ID | 20180418165124.24237-3-stanislav.lisovskiy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 04/18/18 18:51, StanLis wrote: > From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > > Added encoding of drm content_type property from > drm_connector_state within AVI infoframe in order to properly handle > external HDMI TV content-type setting. > > Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> > --- > drivers/gpu/drm/i915/intel_atomic.c | 1 + > drivers/gpu/drm/i915/intel_hdmi.c | 3 +++ > 2 files changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c > index 40285d1b91b7..61ddb5871d8a 100644 > --- a/drivers/gpu/drm/i915/intel_atomic.c > +++ b/drivers/gpu/drm/i915/intel_atomic.c > @@ -124,6 +124,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, > if (new_conn_state->force_audio != old_conn_state->force_audio || > new_conn_state->broadcast_rgb != old_conn_state->broadcast_rgb || > new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio || > + new_conn_state->base.content_type != old_conn_state->base.content_type || > new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode) > crtc_state->mode_changed = true; > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index ee929f31f7db..5cc72da9c086 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -491,6 +491,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder, > intel_hdmi->rgb_quant_range_selectable, > is_hdmi2_sink); > > + frame.avi.content_type = connector->state->content_type; > + Is the ITC bit set in the AVI InfoFrame? The content type bits are only valid if the ITC bit is 1. And if there is no content type property, then ITC should be 0. > /* TODO: handle pixel repetition for YCBCR420 outputs */ > intel_write_infoframe(encoder, crtc_state, &frame); > } > @@ -2065,6 +2067,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c > intel_attach_force_audio_property(connector); > intel_attach_broadcast_rgb_property(connector); > intel_attach_aspect_ratio_property(connector); > + drm_connector_attach_content_type_property(connector); > connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > } > > Regards, Hans
Hi, Please see my reply inline:
On 04/18/18 14:01, Lisovskiy, Stanislav wrote: > Hi, > > Please see my reply inline: > ________________________________________ > From: dri-devel [dri-devel-bounces@lists.freedesktop.org] on behalf of Hans Verkuil [hverkuil@xs4all.nl] > Sent: Wednesday, April 18, 2018 2:35 PM > To: Lisovskiy, Stanislav; dri-devel@lists.freedesktop.org > Cc: intel-gfx@lists.freedesktop.org > Subject: Re: [PATCH v2 2/2] i915: content-type property for HDMI connector > > On 04/18/18 18:51, StanLis wrote: >> From: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> >> >> Added encoding of drm content_type property from >> drm_connector_state within AVI infoframe in order to properly handle >> external HDMI TV content-type setting. >> >> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com> >> --- >> drivers/gpu/drm/i915/intel_atomic.c | 1 + >> drivers/gpu/drm/i915/intel_hdmi.c | 3 +++ >> 2 files changed, 4 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c >> index 40285d1b91b7..61ddb5871d8a 100644 >> --- a/drivers/gpu/drm/i915/intel_atomic.c >> +++ b/drivers/gpu/drm/i915/intel_atomic.c >> @@ -124,6 +124,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, >> if (new_conn_state->force_audio != old_conn_state->force_audio || >> new_conn_state->broadcast_rgb != old_conn_state->broadcast_rgb || >> new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio || >> + new_conn_state->base.content_type != old_conn_state->base.content_type || >> new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode) >> crtc_state->mode_changed = true; >> >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index ee929f31f7db..5cc72da9c086 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -491,6 +491,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder, >> intel_hdmi->rgb_quant_range_selectable, >> is_hdmi2_sink); >> >> + frame.avi.content_type = connector->state->content_type; >> + > >> Is the ITC bit set in the AVI InfoFrame? The content type bits are only valid if >> the ITC bit is 1. And if there is no content type property, then ITC should be 0. > > That's a good question, as I understood it is not set, however the HDMI 1.4 spec > says: > > ITC CN1, CN0 Content Type > 0 0, 0 No Data > > 1 0, 0 IT content > > X 0, 1 Photo > X 1, 0 Cinema > X 1, 1 Game > > "X denotes don't care" > > So I wonder, should I probably add additional property to control the itc bit? Yeah, that's wrong in the HDMI Spec. The correct standard to use is the CEA (now CTA) 861. There it clearly defines that the CN bits are only valid if ITC=1. I know, I'm in the CTA-861 working group :-) I don't think anyone ever noticed this bug in the HDMI spec before (I didn't). The 2.1 HDMI spec kind of corrects it in Appendix G.1 where it copies the CTA-861-G text. Anyway, in V4L2 we implement this with an extra value "NO_ITC" which sets ITC to 0 and sets CN0 and CN1 to 0 as well: https://hverkuil.home.xs4all.nl/spec/uapi/v4l/extended-controls.html#digital-video-control-ids The NO_ITC case is usually used for YCbCr video encoding. Note that most displays will just ignore the content type. Regards, Hans > >> /* TODO: handle pixel repetition for YCBCR420 outputs */ >> intel_write_infoframe(encoder, crtc_state, &frame); >> } >> @@ -2065,6 +2067,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c >> intel_attach_force_audio_property(connector); >> intel_attach_broadcast_rgb_property(connector); >> intel_attach_aspect_ratio_property(connector); >> + drm_connector_attach_content_type_property(connector); >> connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >> } >> >> > > Regards, > > Hans > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel >
diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 40285d1b91b7..61ddb5871d8a 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -124,6 +124,7 @@ int intel_digital_connector_atomic_check(struct drm_connector *conn, if (new_conn_state->force_audio != old_conn_state->force_audio || new_conn_state->broadcast_rgb != old_conn_state->broadcast_rgb || new_conn_state->base.picture_aspect_ratio != old_conn_state->base.picture_aspect_ratio || + new_conn_state->base.content_type != old_conn_state->base.content_type || new_conn_state->base.scaling_mode != old_conn_state->base.scaling_mode) crtc_state->mode_changed = true; diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index ee929f31f7db..5cc72da9c086 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -491,6 +491,8 @@ static void intel_hdmi_set_avi_infoframe(struct drm_encoder *encoder, intel_hdmi->rgb_quant_range_selectable, is_hdmi2_sink); + frame.avi.content_type = connector->state->content_type; + /* TODO: handle pixel repetition for YCBCR420 outputs */ intel_write_infoframe(encoder, crtc_state, &frame); } @@ -2065,6 +2067,7 @@ intel_hdmi_add_properties(struct intel_hdmi *intel_hdmi, struct drm_connector *c intel_attach_force_audio_property(connector); intel_attach_broadcast_rgb_property(connector); intel_attach_aspect_ratio_property(connector); + drm_connector_attach_content_type_property(connector); connector->state->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; }