Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

Maarten Lankhorst
2018-01-18T09:30:20Z

Op 18-01-18 om 07:27 schreef Sharma, Shashank:
> Regards
>
> Shashank
>
>
> On 1/17/2018 11:57 PM, Ville Syrjälä wrote:
>> On Fri, Jan 05, 2018 at 03:15:35PM +0530, Shashank Sharma wrote:
>>> LSPCON chips can generate YCBCR outputs, if asked nicely :).
>>>
>>> In order to generate YCBCR 4:2:0 outputs, a source must:
>>> - send YCBCR 4:4:4 signals to LSPCON
>>> - program color space as 4:2:0 in AVI infoframes
>>>
>>> Whereas for YCBCR 4:4:4 outputs, the source must:
>>> - send YCBCR 4:4:4 signals to LSPCON
>>> - program color space as 4:4:4 in AVI infoframes
>>>
>>> So for both 4:2:0 as well as 4:4:4 outputs, we are driving the
>>> pipe for YCBCR 4:4:4 output, but AVI infoframe's color space
>>> information indicates LSPCON FW to start scaling down from YCBCR
>>> 4:4:4 and generate YCBCR 4:2:0 output. As the scaling is done by
>>> LSPCON device, we need not to reserve a scaler for 4:2:0 outputs.
>>>
>>> V2: rebase
>>> V3: Addressed review comments from Ville
>>>      - add enum crtc_output_format instead of bool ycbcr420
>>>      - use crtc_output_format=4:4:4 for modeset of LSPCON 4:2:0 output
>>>        cases in this way we will have YCBCR 4:4:4 framework ready (except
>>>        the ABI part)
>>>
>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>> Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/i915_reg.h     |  2 ++
>>>   drivers/gpu/drm/i915/intel_ddi.c    |  7 +++++++
>>>   drivers/gpu/drm/i915/intel_dp.c     | 10 ++++++++++
>>>   drivers/gpu/drm/i915/intel_drv.h    |  2 ++
>>>   drivers/gpu/drm/i915/intel_lspcon.c | 28 ++++++++++++++++++++++++++++
>>>   5 files changed, 49 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>> index 966e4df..45ee264 100644
>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>> @@ -8547,6 +8547,8 @@ enum skl_power_gate {
>>>   #define TRANS_MSA_MISC(tran) _MMIO_TRANS2(tran, _TRANSA_MSA_MISC)
>>>     #define  TRANS_MSA_SYNC_CLK        (1<<0)
>>> +#define  TRANS_MSA_SAMPLING_444        (2<<1)
>>> +#define  TRANS_MSA_CLRSP_YCBCR        (2<<3)
>>>   #define  TRANS_MSA_6_BPC        (0<<5)
>>>   #define  TRANS_MSA_8_BPC        (1<<5)
>>>   #define  TRANS_MSA_10_BPC        (2<<5)
>>> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
>>> index 7b89f2a..7616f6f 100644
>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>> @@ -1499,6 +1499,13 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>>>           break;
>>>       }
>>>   +    /*
>>> +     * As per DP 1.2 spec section 2.3.4.3 while sending
>>> +     * YCBCR 444 signals we should program MSA MISC1/0 fields with
>>> +     * colorspace information.
>>> +     */
>>> +    if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>>> +        temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>> This fails to state that we're indicating BT.601 encoding. I think we
>> should spell that out explicitly.
> Agree, I will add this in comments.
>>>       I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>>   }
>>>   diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>> index 35c5299..3bf82ea 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -1613,6 +1613,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>       struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>>>       struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>>>       struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>>> +    struct intel_lspcon *lspcon = enc_to_intel_lspcon(&encoder->base);
>>>       enum port port = encoder->port;
>>>       struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
>>>       struct intel_connector *intel_connector = intel_dp->attached_connector;
>>> @@ -1642,6 +1643,15 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>       if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
>>>           pipe_config->has_pch_encoder = true;
>>>   +    if (lspcon->active) {
>>> +        struct drm_connector *connector = &intel_connector->base;
>>> +
>>> +        if (lspcon_ycbcr420_config(connector, pipe_config)) {
>>> +            pipe_config->output_format = CRTC_OUTPUT_YCBCR444;
>> I think I'd like to see all compute_config hooks explicitly set the
>> outout_format (to RGB usually obviously).
> That's the one I was talking about in previous patch. If we keep output_format_RGB = 0, we need
> not to do this, as reset of the pipe_config will automatically make it RGB.
>>> +            lspcon->output_format = CRTC_OUTPUT_YCBCR420;
>> You should not modify any non-atomic state like that in compute_config.
> Please help me to understand this better, Can you elaborate a bit more on:
> - Why is this a non-atomic state ?
Because lspcon->output_format is modified even if a commit is TEST_ONLY..

This will break if you do a nonblocking modeset vs TEST_ONLY, it could commit the wrong lspcon->output_format. :)
> - What is the right place we should modify it ?
intel_digital_connector_state probably.

~Maarten
>
> - Shashank
>>> +        }
>>> +    }
>>> +
>>>       pipe_config->has_drrs = false;
>>>       if (IS_G4X(dev_priv) || port == PORT_A)
>>>           pipe_config->has_audio = false;
>>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>>> index 2de6b41..5edba06 100644
>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>> @@ -2047,6 +2047,8 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>                  const struct drm_connector_state *conn_state);
>>>   bool lspcon_infoframe_enabled(struct drm_encoder *encoder,
>>>                     const struct intel_crtc_state *pipe_config);
>>> +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>>> +                 struct intel_crtc_state *config);
>>>     /* intel_pipe_crc.c */
>>>   int intel_pipe_crc_create(struct drm_minor *minor);
>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>>> index 066ea91..cb88138 100644
>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>> @@ -180,6 +180,25 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>>>       return true;
>>>   }
>>>   +bool lspcon_ycbcr420_config(struct drm_connector *connector,
>>> +                 struct intel_crtc_state *config)
>>> +{
>>> +    struct drm_display_info *info = &connector->display_info;
>>> +    struct drm_display_mode *mode = &config->base.adjusted_mode;
>>> +
>>> +    if (drm_mode_is_420_only(info, mode)) {
>>> +        if (!connector->ycbcr_420_allowed) {
>>> +            DRM_ERROR("Platform doesn't support YCBCR420 output\n");
>>> +            return false;
>>> +        }
>>> +
>>> +        config->port_clock /= 2;
>>> +        return true;
>>> +    }
>>> +
>>> +    return false;
>>> +}
>>> +
>>>   static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>   {
>>>       int retry;
>>> @@ -459,6 +478,13 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>           return;
>>>       }
>>>   +    if (lspcon->output_format == CRTC_OUTPUT_YCBCR420)
>>> +        frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>> +    else if (lspcon->output_format == CRTC_OUTPUT_YCBCR444)
>>> +        frame.avi.colorspace = HDMI_COLORSPACE_YUV444;
>>> +    else
>>> +        frame.avi.colorspace = HDMI_COLORSPACE_RGB;
>>> +
>>>       drm_hdmi_avi_infoframe_quant_range(&frame.avi, mode,
>>>                          crtc_state->limited_color_range ?
>>>                          HDMI_QUANTIZATION_RANGE_LIMITED :
>>> @@ -512,6 +538,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>>       struct intel_lspcon *lspcon = &intel_dig_port->lspcon;
>>>       struct drm_device *dev = intel_dig_port->base.base.dev;
>>>       struct drm_i915_private *dev_priv = to_i915(dev);
>>> +    struct drm_connector *connector = &dp->attached_connector->base;
>>>         if (!HAS_LSPCON(dev_priv)) {
>>>           DRM_ERROR("LSPCON is not supported on this platform\n");
>>> @@ -536,6 +563,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>>>           return false;
>>>       }
>>>   +    connector->ycbcr_420_allowed = true;
>>>       lspcon->active = true;
>>>       DRM_DEBUG_KMS("Success: LSPCON init\n");
>>>       return true;
>>> --
>>> 2.7.4
>