| Message ID | 1499685528-6926-11-git-send-email-shashank.sharma@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Jul 10, 2017 at 04:48:38PM +0530, Shashank Sharma wrote: > This patch checks encoder level support for YCBCR420 outputs. > The logic goes as simple as this: > If the input mode is YCBCR420-only mode: prepare HDMI for > YCBCR420 output, else continue with RGB output mode. > > It checks if the mode is YCBCR420 and source can support this > output then it marks the ycbcr_420 output indicator 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 > V5: Rebase > V6: Check and handle YCBCR420-only modes, discard the property > based approach (Ville) > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > --- > drivers/gpu/drm/i915/intel_display.c | 1 + > drivers/gpu/drm/i915/intel_drv.h | 3 +++ > drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++--- > 3 files changed, 43 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 4e03ca6..01900e1 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(ycbcr420); > > 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 d17a324..592243b 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 */ > + bool ycbcr420; > }; > > struct intel_crtc { > diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > index cc0d100..276d916 100644 > --- a/drivers/gpu/drm/i915/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > @@ -1305,7 +1305,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, > + bool output_ycbcr420) You alreayd have the crtc state, so no need to pass that stuff separately. > { > struct drm_i915_private *dev_priv = > to_i915(crtc_state->base.crtc->dev); > @@ -1330,6 +1331,13 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) > if (connector_state->crtc != crtc_state->base.crtc) > continue; > > + if (output_ycbcr420) { > + const struct drm_hdmi_info *hdmi = &info->hdmi; > + > + if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) > + return false; > + } > + else? > if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0) > return false; > } > @@ -1342,6 +1350,25 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) > return true; > } > > +static bool > +intel_hdmi_ycbcr420_config(struct drm_connector *connector, > + struct intel_crtc_state *config, > + int *clock_12bpc, int *clock_8bpc) > +{ > + > + if (!connector->ycbcr_420_allowed) { > + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); > + return false; > + } > + > + /* YCBCR420 TMDS rate requirement is half the pixel clock */ > + config->port_clock /= 2; > + *clock_12bpc /= 2; > + *clock_8bpc /= 2; > + config->ycbcr420 = true; > + return true; > +} > + > bool intel_hdmi_compute_config(struct intel_encoder *encoder, > struct intel_crtc_state *pipe_config, > struct drm_connector_state *conn_state) > @@ -1349,7 +1376,8 @@ 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; > @@ -1379,6 +1407,14 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder, > clock_12bpc *= 2; > } > > + if (drm_mode_is_420_only(&connector->display_info, adjusted_mode)) { > + if (!intel_hdmi_ycbcr420_config(connector, pipe_config, > + &clock_12bpc, &clock_8bpc)) { > + DRM_ERROR("Can't support YCBCR420 output\n"); > + return false; > + } > + } > + > if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv)) > pipe_config->has_pch_encoder = true; > > @@ -1398,7 +1434,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, pipe_config->ycbcr420)) { > DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n"); > desired_bpp = 12*3; > > -- > 2.7.4 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Regards Shashank On 7/12/2017 10:47 PM, Ville Syrjälä wrote: > On Mon, Jul 10, 2017 at 04:48:38PM +0530, Shashank Sharma wrote: >> This patch checks encoder level support for YCBCR420 outputs. >> The logic goes as simple as this: >> If the input mode is YCBCR420-only mode: prepare HDMI for >> YCBCR420 output, else continue with RGB output mode. >> >> It checks if the mode is YCBCR420 and source can support this >> output then it marks the ycbcr_420 output indicator 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 >> V5: Rebase >> V6: Check and handle YCBCR420-only modes, discard the property >> based approach (Ville) >> >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >> --- >> drivers/gpu/drm/i915/intel_display.c | 1 + >> drivers/gpu/drm/i915/intel_drv.h | 3 +++ >> drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++--- >> 3 files changed, 43 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 4e03ca6..01900e1 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(ycbcr420); >> >> 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 d17a324..592243b 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 */ >> + bool ycbcr420; >> }; >> >> struct intel_crtc { >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >> index cc0d100..276d916 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1305,7 +1305,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, >> + bool output_ycbcr420) > You alreayd have the crtc state, so no need to pass that stuff > separately. Ah, this was dumb, thanks ! >> { >> struct drm_i915_private *dev_priv = >> to_i915(crtc_state->base.crtc->dev); >> @@ -1330,6 +1331,13 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) >> if (connector_state->crtc != crtc_state->base.crtc) >> continue; >> >> + if (output_ycbcr420) { >> + const struct drm_hdmi_info *hdmi = &info->hdmi; >> + >> + if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) >> + return false; >> + } >> + > else? Oops, needs an else { break;} - Shashank > >> if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0) >> return false; >> } >> @@ -1342,6 +1350,25 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) >> return true; >> } >> >> +static bool >> +intel_hdmi_ycbcr420_config(struct drm_connector *connector, >> + struct intel_crtc_state *config, >> + int *clock_12bpc, int *clock_8bpc) >> +{ >> + >> + if (!connector->ycbcr_420_allowed) { >> + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); >> + return false; >> + } >> + >> + /* YCBCR420 TMDS rate requirement is half the pixel clock */ >> + config->port_clock /= 2; >> + *clock_12bpc /= 2; >> + *clock_8bpc /= 2; >> + config->ycbcr420 = true; >> + return true; >> +} >> + >> bool intel_hdmi_compute_config(struct intel_encoder *encoder, >> struct intel_crtc_state *pipe_config, >> struct drm_connector_state *conn_state) >> @@ -1349,7 +1376,8 @@ 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; >> @@ -1379,6 +1407,14 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder, >> clock_12bpc *= 2; >> } >> >> + if (drm_mode_is_420_only(&connector->display_info, adjusted_mode)) { >> + if (!intel_hdmi_ycbcr420_config(connector, pipe_config, >> + &clock_12bpc, &clock_8bpc)) { >> + DRM_ERROR("Can't support YCBCR420 output\n"); >> + return false; >> + } >> + } >> + >> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv)) >> pipe_config->has_pch_encoder = true; >> >> @@ -1398,7 +1434,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, pipe_config->ycbcr420)) { >> DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n"); >> desired_bpp = 12*3; >> >> -- >> 2.7.4 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jul 13, 2017 at 10:56:06AM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 7/12/2017 10:47 PM, Ville Syrjälä wrote: > > On Mon, Jul 10, 2017 at 04:48:38PM +0530, Shashank Sharma wrote: > >> This patch checks encoder level support for YCBCR420 outputs. > >> The logic goes as simple as this: > >> If the input mode is YCBCR420-only mode: prepare HDMI for > >> YCBCR420 output, else continue with RGB output mode. > >> > >> It checks if the mode is YCBCR420 and source can support this > >> output then it marks the ycbcr_420 output indicator 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 > >> V5: Rebase > >> V6: Check and handle YCBCR420-only modes, discard the property > >> based approach (Ville) > >> > >> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_display.c | 1 + > >> drivers/gpu/drm/i915/intel_drv.h | 3 +++ > >> drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++--- > >> 3 files changed, 43 insertions(+), 3 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >> index 4e03ca6..01900e1 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(ycbcr420); > >> > >> 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 d17a324..592243b 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 */ > >> + bool ycbcr420; > >> }; > >> > >> struct intel_crtc { > >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > >> index cc0d100..276d916 100644 > >> --- a/drivers/gpu/drm/i915/intel_hdmi.c > >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c > >> @@ -1305,7 +1305,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, > >> + bool output_ycbcr420) > > You alreayd have the crtc state, so no need to pass that stuff > > separately. > Ah, this was dumb, thanks ! > >> { > >> struct drm_i915_private *dev_priv = > >> to_i915(crtc_state->base.crtc->dev); > >> @@ -1330,6 +1331,13 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) > >> if (connector_state->crtc != crtc_state->base.crtc) > >> continue; > >> > >> + if (output_ycbcr420) { > >> + const struct drm_hdmi_info *hdmi = &info->hdmi; > >> + > >> + if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) > >> + return false; > >> + } > >> + > > else? > Oops, needs an else { break;} I was thinking 'else if ...' > > - Shashank > > > >> if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0) > >> return false; > >> } > >> @@ -1342,6 +1350,25 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) > >> return true; > >> } > >> > >> +static bool > >> +intel_hdmi_ycbcr420_config(struct drm_connector *connector, > >> + struct intel_crtc_state *config, > >> + int *clock_12bpc, int *clock_8bpc) > >> +{ > >> + > >> + if (!connector->ycbcr_420_allowed) { > >> + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); > >> + return false; > >> + } > >> + > >> + /* YCBCR420 TMDS rate requirement is half the pixel clock */ > >> + config->port_clock /= 2; > >> + *clock_12bpc /= 2; > >> + *clock_8bpc /= 2; > >> + config->ycbcr420 = true; > >> + return true; > >> +} > >> + > >> bool intel_hdmi_compute_config(struct intel_encoder *encoder, > >> struct intel_crtc_state *pipe_config, > >> struct drm_connector_state *conn_state) > >> @@ -1349,7 +1376,8 @@ 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; > >> @@ -1379,6 +1407,14 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder, > >> clock_12bpc *= 2; > >> } > >> > >> + if (drm_mode_is_420_only(&connector->display_info, adjusted_mode)) { > >> + if (!intel_hdmi_ycbcr420_config(connector, pipe_config, > >> + &clock_12bpc, &clock_8bpc)) { > >> + DRM_ERROR("Can't support YCBCR420 output\n"); > >> + return false; > >> + } > >> + } > >> + > >> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv)) > >> pipe_config->has_pch_encoder = true; > >> > >> @@ -1398,7 +1434,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, pipe_config->ycbcr420)) { > >> DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n"); > >> desired_bpp = 12*3; > >> > >> -- > >> 2.7.4 > >> > >> _______________________________________________ > >> dri-devel mailing list > >> dri-devel@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Regards Shashank On 7/13/2017 6:23 PM, Ville Syrjälä wrote: > On Thu, Jul 13, 2017 at 10:56:06AM +0530, Sharma, Shashank wrote: >> Regards >> >> Shashank >> >> >> On 7/12/2017 10:47 PM, Ville Syrjälä wrote: >>> On Mon, Jul 10, 2017 at 04:48:38PM +0530, Shashank Sharma wrote: >>>> This patch checks encoder level support for YCBCR420 outputs. >>>> The logic goes as simple as this: >>>> If the input mode is YCBCR420-only mode: prepare HDMI for >>>> YCBCR420 output, else continue with RGB output mode. >>>> >>>> It checks if the mode is YCBCR420 and source can support this >>>> output then it marks the ycbcr_420 output indicator 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 >>>> V5: Rebase >>>> V6: Check and handle YCBCR420-only modes, discard the property >>>> based approach (Ville) >>>> >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> >>>> --- >>>> drivers/gpu/drm/i915/intel_display.c | 1 + >>>> drivers/gpu/drm/i915/intel_drv.h | 3 +++ >>>> drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++--- >>>> 3 files changed, 43 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >>>> index 4e03ca6..01900e1 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(ycbcr420); >>>> >>>> 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 d17a324..592243b 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 */ >>>> + bool ycbcr420; >>>> }; >>>> >>>> struct intel_crtc { >>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c >>>> index cc0d100..276d916 100644 >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >>>> @@ -1305,7 +1305,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, >>>> + bool output_ycbcr420) >>> You alreayd have the crtc state, so no need to pass that stuff >>> separately. >> Ah, this was dumb, thanks ! >>>> { >>>> struct drm_i915_private *dev_priv = >>>> to_i915(crtc_state->base.crtc->dev); >>>> @@ -1330,6 +1331,13 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) >>>> if (connector_state->crtc != crtc_state->base.crtc) >>>> continue; >>>> >>>> + if (output_ycbcr420) { >>>> + const struct drm_hdmi_info *hdmi = &info->hdmi; >>>> + >>>> + if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) >>>> + return false; >>>> + } >>>> + >>> else? >> Oops, needs an else { break;} > I was thinking 'else if ...' Do we need else if for 12 BPC case, I was thinking of: if (!hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36) /* 12 BPC Y420 not possible */ return false; else /* output is going to be 420, and 12BPC is possible, so break the loop */ break; This will also allow the code to go through the WAR Added below. - Shashank > >> - Shashank >>>> if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0) >>>> return false; >>>> } >>>> @@ -1342,6 +1350,25 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) >>>> return true; >>>> } >>>> >>>> +static bool >>>> +intel_hdmi_ycbcr420_config(struct drm_connector *connector, >>>> + struct intel_crtc_state *config, >>>> + int *clock_12bpc, int *clock_8bpc) >>>> +{ >>>> + >>>> + if (!connector->ycbcr_420_allowed) { >>>> + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); >>>> + return false; >>>> + } >>>> + >>>> + /* YCBCR420 TMDS rate requirement is half the pixel clock */ >>>> + config->port_clock /= 2; >>>> + *clock_12bpc /= 2; >>>> + *clock_8bpc /= 2; >>>> + config->ycbcr420 = true; >>>> + return true; >>>> +} >>>> + >>>> bool intel_hdmi_compute_config(struct intel_encoder *encoder, >>>> struct intel_crtc_state *pipe_config, >>>> struct drm_connector_state *conn_state) >>>> @@ -1349,7 +1376,8 @@ 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; >>>> @@ -1379,6 +1407,14 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder, >>>> clock_12bpc *= 2; >>>> } >>>> >>>> + if (drm_mode_is_420_only(&connector->display_info, adjusted_mode)) { >>>> + if (!intel_hdmi_ycbcr420_config(connector, pipe_config, >>>> + &clock_12bpc, &clock_8bpc)) { >>>> + DRM_ERROR("Can't support YCBCR420 output\n"); >>>> + return false; >>>> + } >>>> + } >>>> + >>>> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv)) >>>> pipe_config->has_pch_encoder = true; >>>> >>>> @@ -1398,7 +1434,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, pipe_config->ycbcr420)) { >>>> DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n"); >>>> desired_bpp = 12*3; >>>> >>>> -- >>>> 2.7.4 >>>> >>>> _______________________________________________ >>>> dri-devel mailing list >>>> dri-devel@lists.freedesktop.org >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Thu, Jul 13, 2017 at 06:31:25PM +0530, Sharma, Shashank wrote: > Regards > > Shashank > > > On 7/13/2017 6:23 PM, Ville Syrjälä wrote: > > On Thu, Jul 13, 2017 at 10:56:06AM +0530, Sharma, Shashank wrote: > >> Regards > >> > >> Shashank > >> > >> > >> On 7/12/2017 10:47 PM, Ville Syrjälä wrote: > >>> On Mon, Jul 10, 2017 at 04:48:38PM +0530, Shashank Sharma wrote: > >>>> This patch checks encoder level support for YCBCR420 outputs. > >>>> The logic goes as simple as this: > >>>> If the input mode is YCBCR420-only mode: prepare HDMI for > >>>> YCBCR420 output, else continue with RGB output mode. > >>>> > >>>> It checks if the mode is YCBCR420 and source can support this > >>>> output then it marks the ycbcr_420 output indicator 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 > >>>> V5: Rebase > >>>> V6: Check and handle YCBCR420-only modes, discard the property > >>>> based approach (Ville) > >>>> > >>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> > >>>> --- > >>>> drivers/gpu/drm/i915/intel_display.c | 1 + > >>>> drivers/gpu/drm/i915/intel_drv.h | 3 +++ > >>>> drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++--- > >>>> 3 files changed, 43 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > >>>> index 4e03ca6..01900e1 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(ycbcr420); > >>>> > >>>> 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 d17a324..592243b 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 */ > >>>> + bool ycbcr420; > >>>> }; > >>>> > >>>> struct intel_crtc { > >>>> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c > >>>> index cc0d100..276d916 100644 > >>>> --- a/drivers/gpu/drm/i915/intel_hdmi.c > >>>> +++ b/drivers/gpu/drm/i915/intel_hdmi.c > >>>> @@ -1305,7 +1305,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, > >>>> + bool output_ycbcr420) > >>> You alreayd have the crtc state, so no need to pass that stuff > >>> separately. > >> Ah, this was dumb, thanks ! > >>>> { > >>>> struct drm_i915_private *dev_priv = > >>>> to_i915(crtc_state->base.crtc->dev); > >>>> @@ -1330,6 +1331,13 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) > >>>> if (connector_state->crtc != crtc_state->base.crtc) > >>>> continue; > >>>> > >>>> + if (output_ycbcr420) { > >>>> + const struct drm_hdmi_info *hdmi = &info->hdmi; > >>>> + > >>>> + if (!(hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36)) > >>>> + return false; > >>>> + } > >>>> + > >>> else? > >> Oops, needs an else { break;} > > I was thinking 'else if ...' > Do we need else if for 12 BPC case, I was thinking of: > if (!hdmi->y420_dc_modes & DRM_EDID_YCBCR420_DC_36) > /* 12 BPC Y420 not possible */ > return false; > else > /* output is going to be 420, and 12BPC is possible, so break the > loop */ > break; > > This will also allow the code to go through the WAR Added below. We don't want breaks in the loop. It's meant to go through all the connectors for the crtc. Granted on modern platforms there can only be one, but IMO assuming that just makes the whole thing look confusing. It's much clearer IMO if we do if (420) { check 420 dc modes; } else { check 444 dc modes; } > - Shashank > > > >> - Shashank > >>>> if ((info->edid_hdmi_dc_modes & DRM_EDID_HDMI_DC_36) == 0) > >>>> return false; > >>>> } > >>>> @@ -1342,6 +1350,25 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) > >>>> return true; > >>>> } > >>>> > >>>> +static bool > >>>> +intel_hdmi_ycbcr420_config(struct drm_connector *connector, > >>>> + struct intel_crtc_state *config, > >>>> + int *clock_12bpc, int *clock_8bpc) > >>>> +{ > >>>> + > >>>> + if (!connector->ycbcr_420_allowed) { > >>>> + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); > >>>> + return false; > >>>> + } > >>>> + > >>>> + /* YCBCR420 TMDS rate requirement is half the pixel clock */ > >>>> + config->port_clock /= 2; > >>>> + *clock_12bpc /= 2; > >>>> + *clock_8bpc /= 2; > >>>> + config->ycbcr420 = true; > >>>> + return true; > >>>> +} > >>>> + > >>>> bool intel_hdmi_compute_config(struct intel_encoder *encoder, > >>>> struct intel_crtc_state *pipe_config, > >>>> struct drm_connector_state *conn_state) > >>>> @@ -1349,7 +1376,8 @@ 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; > >>>> @@ -1379,6 +1407,14 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder, > >>>> clock_12bpc *= 2; > >>>> } > >>>> > >>>> + if (drm_mode_is_420_only(&connector->display_info, adjusted_mode)) { > >>>> + if (!intel_hdmi_ycbcr420_config(connector, pipe_config, > >>>> + &clock_12bpc, &clock_8bpc)) { > >>>> + DRM_ERROR("Can't support YCBCR420 output\n"); > >>>> + return false; > >>>> + } > >>>> + } > >>>> + > >>>> if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv)) > >>>> pipe_config->has_pch_encoder = true; > >>>> > >>>> @@ -1398,7 +1434,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, pipe_config->ycbcr420)) { > >>>> DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n"); > >>>> desired_bpp = 12*3; > >>>> > >>>> -- > >>>> 2.7.4 > >>>> > >>>> _______________________________________________ > >>>> dri-devel mailing list > >>>> dri-devel@lists.freedesktop.org > >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
On 7/13/2017 6:56 PM, Ville Syrjälä wrote: > We don't want breaks in the loop. It's meant to go through all the > connectors for the crtc. Granted on modern platforms there can only be > one, but IMO assuming that just makes the whole thing look confusing. > It's much clearer IMO if we do > > if (420) { > check 420 dc modes; > } else { > check 444 dc modes; > } I dint want to add another loop for the 420 stuff, hence was reusing the existing loop. Now, my steps were: - If there is a CRTC match, I got the right CRTC. - On this CRTC, if YCBCR420 output is enabled, I should just check DRM_EDID_YCBCR420_DC_36 for 420_12BPC, so if it supports DRM_EDID_YCBCR420_DC_36 say yes, else no. - But I also want to go through the WAR condition below, added for GLK. So I can't return from here. Do you prefer me adding another loop just for YCBCR420 ? - Shashank
On Thu, Jul 13, 2017 at 07:05:12PM +0530, Sharma, Shashank wrote: > On 7/13/2017 6:56 PM, Ville Syrjälä wrote: > > > We don't want breaks in the loop. It's meant to go through all the > > connectors for the crtc. Granted on modern platforms there can only be > > one, but IMO assuming that just makes the whole thing look confusing. > > It's much clearer IMO if we do > > > > if (420) { > > check 420 dc modes; > > } else { > > check 444 dc modes; > > } > I dint want to add another loop for the 420 stuff, hence was reusing the > existing loop. What I mean is for_each() { ... + if (420) { + if (!420_dc) + return false; + } else { if (!444_dc) return false; + } } > Now, my steps were: > - If there is a CRTC match, I got the right CRTC. > - On this CRTC, if YCBCR420 output is enabled, I should just check > DRM_EDID_YCBCR420_DC_36 > for 420_12BPC, so if it supports DRM_EDID_YCBCR420_DC_36 say yes, > else no. > - But I also want to go through the WAR condition below, added for GLK. > So I can't return from here. > > Do you prefer me adding another loop just for YCBCR420 ? > > - Shashank
On 7/13/2017 7:40 PM, Ville Syrjälä wrote: > What I mean is > > for_each() { > ... > + if (420) { > + if (!420_dc) > + return false; > + } else { > if (!444_dc) > return false; > + } > } Got it, Thanks !
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 4e03ca6..01900e1 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(ycbcr420); 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 d17a324..592243b 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 */ + bool ycbcr420; }; struct intel_crtc { diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index cc0d100..276d916 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -1305,7 +1305,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, + bool output_ycbcr420) { struct drm_i915_private *dev_priv = to_i915(crtc_state->base.crtc->dev); @@ -1330,6 +1331,13 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) if (connector_state->crtc != crtc_state->base.crtc) continue; + if (output_ycbcr420) { + const struct drm_hdmi_info *hdmi = &info->hdmi; + + if (!(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; } @@ -1342,6 +1350,25 @@ static bool hdmi_12bpc_possible(struct intel_crtc_state *crtc_state) return true; } +static bool +intel_hdmi_ycbcr420_config(struct drm_connector *connector, + struct intel_crtc_state *config, + int *clock_12bpc, int *clock_8bpc) +{ + + if (!connector->ycbcr_420_allowed) { + DRM_ERROR("Platform doesn't support YCBCR420 output\n"); + return false; + } + + /* YCBCR420 TMDS rate requirement is half the pixel clock */ + config->port_clock /= 2; + *clock_12bpc /= 2; + *clock_8bpc /= 2; + config->ycbcr420 = true; + return true; +} + bool intel_hdmi_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) @@ -1349,7 +1376,8 @@ 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; @@ -1379,6 +1407,14 @@ bool intel_hdmi_compute_config(struct intel_encoder *encoder, clock_12bpc *= 2; } + if (drm_mode_is_420_only(&connector->display_info, adjusted_mode)) { + if (!intel_hdmi_ycbcr420_config(connector, pipe_config, + &clock_12bpc, &clock_8bpc)) { + DRM_ERROR("Can't support YCBCR420 output\n"); + return false; + } + } + if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv)) pipe_config->has_pch_encoder = true; @@ -1398,7 +1434,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, pipe_config->ycbcr420)) { DRM_DEBUG_KMS("picking bpc to 12 for HDMI output\n"); desired_bpp = 12*3;
This patch checks encoder level support for YCBCR420 outputs. The logic goes as simple as this: If the input mode is YCBCR420-only mode: prepare HDMI for YCBCR420 output, else continue with RGB output mode. It checks if the mode is YCBCR420 and source can support this output then it marks the ycbcr_420 output indicator 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 V5: Rebase V6: Check and handle YCBCR420-only modes, discard the property based approach (Ville) Signed-off-by: Shashank Sharma <shashank.sharma@intel.com> --- drivers/gpu/drm/i915/intel_display.c | 1 + drivers/gpu/drm/i915/intel_drv.h | 3 +++ drivers/gpu/drm/i915/intel_hdmi.c | 42 +++++++++++++++++++++++++++++++++--- 3 files changed, 43 insertions(+), 3 deletions(-)