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

From: "Sharma, Shashank" <shashank.sharma@intel.com>
To: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
Date: Thu, 18 Jan 2018 21:22:26 +0530	[thread overview]
Message-ID: <72404ca5-1cd3-7ce2-4da9-8d4778031fe4@intel.com> (raw)
In-Reply-To: <20180118153551.GX10981@intel.com>

Regards

Shashank


On 1/18/2018 9:05 PM, Ville Syrjälä wrote:
> On Thu, Jan 18, 2018 at 09:00:50PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 1/18/2018 7:33 PM, Ville Syrjälä wrote:
>>> On Thu, Jan 18, 2018 at 11:57:09AM +0530, Sharma, Shashank wrote:
>>>> 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.
>>> IMO either we have INVALID=0 so that we use it to catch readout
>>> fails, or we have no INVALID. Other options make little sense to me.
>> Its a minor thing, and doesn't really matter. I can change this.
>>>>>> +			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 ?
>>>> - What is the right place we should modify it ?
>>> I don't think you need it at all. Just consult the crtc state when
>>> needed.
>> I dont think CRTC state can cover all the cases, for example, what would
>> be do when we need
>> YCBCR 4:4:4 output from LSPCON ? As we have already used crtc_state->444
>> for LSPCON output
>> 420, we can't handle this.
> Like I said earlier, just add another thing to the state to indicate what
> LSPCON should do with the 444 data. Either pass it through or downsample
> it.
>
>> This is equivalent to your previous
>> suggestion of adding 'scaling_reqd',
>> but I added in LPSCON (instead of CRTC state) as this information is
>> required only in case of LSPCON.
> But we have no atomic lspcon state. And probably not worth adding one
> since there won't be much there. So just adding what's needed into the
> crtc state seems like the best approach.
Ok, So I guess the suggestion to move this bool into CRTC state is to 
maintain the atomic flow.
Sure, in that case I will add a new bool in crtc_state called 
"output_scaling_reqd" or something
similar.

- Shashank
>> In all native cases, scaling would be always required for 4:2:0 outputs.
>>
>> - Shashank
>>>> - 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

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx