On Tue, Jan 30, 2018 at 03:06:03PM +0530, Shashank Sharma wrote:
> From: "Sharma, Shashank" <shashank.sha...@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.syrj...@linux.intel.com>
> Cc: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> Reviewed-by: Maarten Lankhorst <maarten.lankho...@linux.intel.com>
> Signed-off-by: Shashank Sharma <shashank.sha...@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.

>       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().

> +             }
> +     }
> +
>       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

> +{
> +     struct drm_display_info *info = &connector->display_info;

const

> +     struct drm_display_mode *mode = &config->base.adjusted_mode;

const ... 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;

How can we even get here?

> +             }
> +
> +             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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Reply via email to