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

Sharma, Shashank
2018-02-06T09:17:22Z

Regards

Shashank

On 2/2/2018 7:22 PM, Ville Syrjälä wrote:
> On Fri, Feb 02, 2018 at 11:44:01AM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 2/2/2018 12:39 AM, Ville Syrjälä wrote:
>>> On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote:
>>>> From: "Sharma, Shashank" <shashank.sharma@intel.com>
>>>>
>>>> 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)
>>>> V4: Added r-b from Maarten (for v3)
>>>> Addressed review comments from Ville:
>>>> - Do not add a non-atomic state variable to determine lspcon output.
>>>> Instead add bool in CRTC state to indicate lspcon based scaling.
>>>>
>>>> Cc: Ville Syrjala <ville.syrjala@linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>>>> Reviewed-by: 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_display.c | 11 ++++++++++-
>>>> drivers/gpu/drm/i915/intel_dp.c | 10 ++++++++++
>>>> drivers/gpu/drm/i915/intel_drv.h | 5 +++++
>>>> drivers/gpu/drm/i915/intel_lspcon.c | 30 ++++++++++++++++++++++++++++++
>>>> 6 files changed, 64 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 64933fd..e383b05 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -8721,6 +8721,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 510bb77..db7d940 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. The output colorspace encoding is BT601.
>>>> + */
>>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444)
>>>> + temp |= TRANS_MSA_SAMPLING_444 | TRANS_MSA_CLRSP_YCBCR;
>>>> I915_WRITE(TRANS_MSA_MISC(cpu_transcoder), temp);
>>>> }
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>>>> index 4a40de9..b26f4a9 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -9228,6 +9228,7 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>>> pipe_config->gamma_mode =
>>>> I915_READ(GAMMA_MODE(crtc->pipe)) & GAMMA_MODE_MODE_MASK;
>>>>
>>>> + pipe_config->external_scaling = false;
>>> I really don't like the name of that. Makes me think we're actually
>>> scaling the entire output or something. 'external_ycbcr_420_downsample'
>>> or something like that would be better.
>> ok, let me rename this to something in those lines.
>>>> if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>>>> u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
>>>> enum intel_output_format output_format = CRTC_OUTPUT_RGB;
>>>> @@ -9247,8 +9248,16 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc,
>>>> output_format = CRTC_OUTPUT_YCBCR420;
>>>> } else {
>>>> output_format = CRTC_OUTPUT_YCBCR444;
>>>> - }
>>>>
>>>> + /*
>>>> + * As there is no interface defined (yet)
>>>> + * to get the user's preference for output
>>>> + * format, YCBCR444 output format is only
>>>> + * possible with LSPCON YCBCR420 output,
>>>> + * which uses LSPCON's (external )scaler.
>>>> + */
>>>> + pipe_config->external_scaling = true;
>>>> + }
>>>> }
>>>>
>>>> pipe_config->output_format = output_format;
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index a2e8879..be19579 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -1640,6 +1640,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;
>>>> @@ -1669,6 +1670,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;
>>>> + pipe_config->external_scaling = true;
>>> Why split the work between two functions like this? I think it would be
>>> cleaner to either inline all the code here, or move it all into
>>> lspcon_ycbcr420_config().
>> Sure, makes sense.
>>>> + }
>>>> + }
>>>> +
>>>> 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 efaa331..f23129b 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -882,6 +882,9 @@ struct intel_crtc_state {
>>>>
>>>> /* Output format RGB/YCBCR etc */
>>>> enum intel_output_format output_format;
>>>> +
>>>> + /* Output up/down scaling is done in external device */
>>>> + bool external_scaling;
>>>> };
>>>>
>>>> struct intel_crtc {
>>>> @@ -2139,6 +2142,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..8d6a9b2 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)
>>> crtc_state
>> ok
>>>> +{
>>>> + struct drm_display_info *info = &connector->display_info;
>>> const
>> ok
>>>> + struct drm_display_mode *mode = &config->base.adjusted_mode;
>>> const ... adjusted_mode;
>> ok
>>>> +
>>>> + if (drm_mode_is_420_only(info, mode)) {
>>>> + if (!connector->ycbcr_420_allowed) {
>>>> + DRM_ERROR("Platform doesn't support YCBCR420 output\n");
>>>> + return false;
>>> How can we even get here?
>> This code flow is coming from CRTC/encoder side, whereas 420 capability
>> is set in connector_init, this is the first time we are checking
>> connector's capabilities. So yes, its possible to reach here.
> Oh right, we don't yet use the .mode_valid() stuff for modesets.
>
> One thing that bothers me is that the code here is structured slightly
> differently than the native HDMI case. Would be nice if we could make
> the two codepaths at least follow the same general flow.
Very valid point, but the difference mostly is due to LSPCON still
running like a DP encoder, whereas native HDMI case is in the HDMI side.
>
> Oh and the DRM_ERROR() has to go since it's possibly user triggerable.
> Looks like one snuck into the native HDMI code as well. That too should
> be killed off.
Agree, will prune it !
- Shashank
>
>> - Shashank
>>>> + }
>>>> +
>>>> + config->port_clock /= 2;
>>>> + return true;
>>>> + }
>>>> +
>>>> + return false;
>>>> +}
>>>> +
>>>> static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>> {
>>>> int retry;
>>>> @@ -459,6 +478,15 @@ void lspcon_set_infoframes(struct drm_encoder *encoder,
>>>> return;
>>>> }
>>>>
>>>> + if (crtc_state->output_format == CRTC_OUTPUT_YCBCR444) {
>>>> + if (crtc_state->external_scaling)
>>>> + frame.avi.colorspace = HDMI_COLORSPACE_YUV420;
>>>> + else
>>>> + 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 +540,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 +565,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