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@xxxxxxxxx>
> >>>> ---
> >>>> 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@xxxxxxxxxxxxxxxxxxxxx
> >>>> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
