[Intel-gfx] [PATCH v3 7/7] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
Ville Syrjälä
ville.syrjala at linux.intel.com
Thu Jan 18 15:35:51 UTC 2018
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 at linux.intel.com>
> >>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
> >>>> Signed-off-by: Shashank Sharma <shashank.sharma at 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.
> 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
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list