On Wed, 2017-06-21 at 21:19 +0530, Sharma, Shashank wrote:
> Regards
>
> Shashank
>
>
> On 6/20/2017 7:50 PM, Ander Conselvan De Oliveira wrote:
> > On Mon, 2017-06-19 at 21:38 +0530, Shashank Sharma wrote:
> > > This patch checks encoder level support for HDMI YCBCR outputs.
> > > HDMI output mode is a connector property, this patch checks if
> > > source and sink can support the HDMI output type selected by user.
> > > And if they both can, it commits the hdmi output type into crtc state,
> > > for further staging in driver.
> > >
> > > V2: Split the patch into two, kept helper functions in DRM layer.
> > > V3: Changed the compute_config function based on new DRM API.
> > > V4: Rebase
> > >
> > > Cc: Ville Syrjala <ville.syrjala@xxxxxxxxxxxxxxx>
> > > Cc: Daniel Vetter <daniel.vetter@xxxxxxxxx>
> > > Cc: Ander Conselvan De Oliveira <ander.conselvan.de.oliveira@xxxxxxxxx>
> > > Signed-off-by: Shashank Sharma <shashank.sharma@xxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/i915/intel_display.c | 1 +
> > > drivers/gpu/drm/i915/intel_drv.h | 3 ++
> > > drivers/gpu/drm/i915/intel_hdmi.c | 93 ++++++++++++++++++++++++++++++++++--
> > > 3 files changed, 94 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > > index f9bf0d5..da29536 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -11930,6 +11930,7 @@ intel_pipe_config_compare(struct drm_i915_private *dev_priv,
> > > PIPE_CONF_CHECK_I(hdmi_scrambling);
> > > PIPE_CONF_CHECK_I(hdmi_high_tmds_clock_ratio);
> > > PIPE_CONF_CHECK_I(has_infoframe);
> > > + PIPE_CONF_CHECK_I(hdmi_output);
> > >
> > > PIPE_CONF_CHECK_I(has_audio);
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> > > index 1727350..38fe56c 100644
> > > --- a/drivers/gpu/drm/i915/intel_drv.h
> > > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > > @@ -780,6 +780,9 @@ struct intel_crtc_state {
> > >
> > > /* HDMI High TMDS char rate ratio */
> > > bool hdmi_high_tmds_clock_ratio;
> > > +
> > > + /* HDMI output type */
> > > + enum drm_hdmi_output_type hdmi_output;
> > > };
> > >
> > > struct intel_crtc {
> > > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c
> > > index 170abc4..22da5cd 100644
> > > --- a/drivers/gpu/drm/i915/intel_hdmi.c
> > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c
> > > @@ -1317,7 +1317,8 @@ intel_hdmi_mode_valid(struct drm_connector *connector,
> > > return status;
> > > }
> > >
> > > -static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
> > > +static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state,
> > > + enum drm_hdmi_output_type hdmi_out)
> > > {
> > > struct drm_i915_private *dev_priv =
> > > to_i915(crtc_state->base.crtc->dev);
> > > @@ -1329,6 +1330,16 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
> > > if (HAS_GMCH_DISPLAY(dev_priv))
> > > return false;
> > >
> > > + if (hdmi_out == DRM_HDMI_OUTPUT_YCBCR422) {
> > > + /*
> > > + * HDMI spec says YCBCR422 is 12bpc, but its not a deep
> > > + * color format. So respect the spec, and not allow this
> > > + * to be deep color
> > > + */
> > > + DRM_DEBUG_KMS("Not allowing deep color for YCBCR422 output\n");
> > > + return false;
> > > + }
> > > +
> >
> > I suppose this is about the remark in section 6.2.4 of the HDMI 1.4 spec. If I'm
> > interpreting that correctly, the relevant information for us is that the TMDS
> > clock doesn't need to be multiplied by 1.5, unlike other 12 bpc scenarios.
> >
> > To me it seems misleading to say 12 bpc is not possible in that case. I'm
> > wondering if it makes more sense to split this in two parts:
> >
> > (i) What is the TMDS clock scaling for a given output format?
> > (ii) Is the format and scaled TMDS clock supported by the source and the sink?
>
> My interpretation of the spec was YCBCR422 is a 12 bit encoding scheme,
> but its not a deep color mode (I guess that's what Ville also thought)
What is the definition of deep color? Section 6.2.4 of HDMI 1.4 says "YCBCR
4:2:2 is also 36-bit mode but does not require the further use of the Deep Color
modes described in section 6.5.2 and 6.5.3." Looking at those sections, I
believe the only relevant bit for the driver is whether to change the TMDS
clock. Is there something I'm missing?
> In I915, 12_bpc is always considered as deep color mode case, whereas I
> have seen EDIDs which indicate support for YCBCR422 but they do not
> indicate deep color support.
I see two issues here. One is that we have a function that is call
hdmi_12bpc_possible() and it returns false for YCBCR422 with a comment stating
that is actually a 12 bpc mode. Should this be called hdmi_deep_color_possible()
instead?
Now the other issue is how the return value of that function is used. If it is
true then the bpp in pipe config is updated to 36 and the TMDS clock is scaled
by 1.5.
With the current code none of that is a problem, but as the new format is
introduced the lines get blurry. So what I would like to see is this refactored
to acknowledge that different formats may be supported and that they have
different requirements. For instance, if we ever support 16 bpc, the TMDS clock
should be scaled by 2 instead of 1.5. For YCBCR422, the scale factor is 1. If I
understand correctly, for YCBCR420 the scaling is 0.5.
So what I propose is to have a function that tells if a given output format is
supported, perhaps returning the TMDS scale factor for it. Then the logic of
choosing the output format can be somewhat decoupled, just checking if the
desired format and clock is supported, with a fall back perhaps. Or go through a
sorted listed of preferred formats.
Ander
> Combining these information above, we thought its safe to consider that
> 422 is not a deep color mode, and hence this call.
> >
> > > /*
> > > * HDMI 12bpc affects the clocks, so it's only possible
> > > * when not cloning with other encoder types.
> > > @@ -1342,6 +1353,12 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
> > > if (connector_state->crtc != crtc_state->base.crtc)
> > > continue;
> > >
> > > + if (hdmi_out == DRM_HDMI_OUTPUT_YCBCR420) {
> > > + if (!(info->hdmi.y420_dc_modes &
> > > + DRM_EDID_YCBCR420_DC_36))
> > > + return false;
> > > + }
> > > +
> > > if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0)
> > > return false;
> > > }
> > > @@ -1354,6 +1371,65 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state)
> > > return true;
> > > }
> > >
> > > +static u8
> > > +intel_hdmi_get_src_output_support(struct drm_connector *connector)
> > > +{
> > > + struct drm_i915_private *dev_priv = connector->dev->dev_private;
> > > + u8 supported_outputs = DRM_COLOR_FORMAT_RGB444;
> > > +
> > > + if (dev_priv->info.gen < 7)
> > > + return supported_outputs;
> > > +
> > > + /* Gen 7 and above support HDMI 1.4b outputs */
> > > + supported_outputs |= (DRM_COLOR_FORMAT_YCRCB444 |
> > > + DRM_COLOR_FORMAT_YCRCB422);
> > > +
> > > + if (IS_GEMINILAKE(dev_priv))
> > > + supported_outputs |= DRM_COLOR_FORMAT_YCRCB420;
> > > +
> > > + return supported_outputs;
> > > +}
> > > +
> > > +static enum drm_hdmi_output_type
> > > +intel_hdmi_compute_ycbcr_config(struct drm_connector_state *conn_state,
> > > + struct intel_crtc_state *config,
> > > + int *clock_12bpc, int *clock_8bpc)
> > > +{
> > > + struct drm_connector *connector = conn_state->connector;
> > > + struct drm_display_mode *mode = &config->base.adjusted_mode;
> > > + enum drm_hdmi_output_type type = conn_state->hdmi_output;
> > > + u8 src_output_cap = intel_hdmi_get_src_output_support(connector);
> > > +
> > > + /*
> > > + * Can we support any YCBCR output based on these parameters ?
> > > + * current source capabilities +
> > > + * current sink capabilities +
> > > + * given video mode +
> > > + * current user's choice for HDMI output (from connector property)
> > > + */
> > > + type = drm_find_hdmi_output_type(connector, mode, type, src_output_cap);
> > > + if ((type == DRM_HDMI_OUTPUT_INVALID) ||
> >
> > Unnecessary parenthesis.
>
> Got it
> >
> > > + (type == DRM_HDMI_OUTPUT_DEFAULT_RGB)) {
> >
> > Ditto, plus wrong indentation.
>
> Got it
> >
> > > + DRM_DEBUG_KMS("Can't support YCBCR output\n");
> > > + return type;
> > > + }
> > > +
> > > + if (type == DRM_HDMI_OUTPUT_YCBCR420) {
> > > +
> > > + /* YCBCR420 TMDS rate requirement is half the pixel clock */
> > > + config->hdmi_output = DRM_HDMI_OUTPUT_YCBCR420;
> > > + config->port_clock /= 2;
> > > + *clock_12bpc /= 2;
> > > + *clock_8bpc /= 2;
> > > +
> > > + }
> >
> > I think with the idea I described above, this could be done in in
> > intel_hdmi_comput_config().
>
> Might need more discussion on idea above.
> > > +
> > > + /* Encoder is capable of this output, lets commit to CRTC */
> > > + config->hdmi_output = type;
> > > + DRM_DEBUG_KMS("HDMI output: %s\n", drm_get_hdmi_output_name(type));
> > > + return type;
> >
> > Couldn't this function just return true or false? The value is returned by
> > editing config->hdmi_output. Not a big fan of having DRM_HDMI_OUTPUT_INVALID to
> > mean that something failed.
>
> Sure, Can do that, Its easier to pass the value coming from lower
> function, thats why chose current return type :-)
> > Ander
> >
> > > +}
> > > +
> > > bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > > struct intel_crtc_state *pipe_config,
> > > struct drm_connector_state *conn_state)
> > > @@ -1361,13 +1437,15 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > > struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(&encoder->base);
> > > struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> > > struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> > > - struct drm_scdc *scdc = &conn_state->connector->display_info.hdmi.scdc;
> > > + struct drm_connector *connector = conn_state->connector;
> > > + struct drm_scdc *scdc = &connector->display_info.hdmi.scdc;
> > > struct intel_digital_connector_state *intel_conn_state =
> > > to_intel_digital_connector_state(conn_state);
> > > int clock_8bpc = pipe_config->base.adjusted_mode.crtc_clock;
> > > int clock_12bpc = clock_8bpc * 3 / 2;
> > > int desired_bpp;
> > > bool force_dvi = intel_conn_state->force_audio == HDMI_AUDIO_OFF_DVI;
> > > + enum drm_hdmi_output_type hdmi_out = conn_state->hdmi_output;
> > >
> > > pipe_config->has_hdmi_sink = !force_dvi && intel_hdmi->has_hdmi_sink;
> > >
> > > @@ -1391,6 +1469,15 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > > clock_12bpc *= 2;
> > > }
> > >
> > > + /* Check if YCBCR HDMI output handling is required */
> > > + hdmi_out = intel_hdmi_compute_ycbcr_config(conn_state,
> > > + pipe_config, &clock_12bpc,
> > > + &clock_8bpc);
> > > + if (hdmi_out == DRM_HDMI_OUTPUT_INVALID) {
> > > + DRM_ERROR("Can't support desired HDMI output\n");
> > > + return false;
> > > + }
> > > +
> > > if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv))
> > > pipe_config->has_pch_encoder = true;
> > >
> > > @@ -1410,7 +1497,7 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder,
> > > */
> > > if (pipe_config->pipe_bpp > 8*3 && pipe_config->has_hdmi_sink && !force_dvi &&
> > > hdmi_port_clock_valid(intel_hdmi, clock_12bpc, true, force_dvi) == MODE_OK &&
> > > - hdmi_12bpc_possible(pipe_config)) {
> > > + hdmi_12bpc_possible(pipe_config, hdmi_out)) {
> > > DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n");
> > > desired_bpp = 12*3;
> > >
>
>
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
