[Intel-gfx] [PATCH 5/5] drm/i915: Add YCBCR 4:2:0 support for LSPCON
Sharma, Shashank
shashank.sharma at intel.com
Wed Dec 20 05:01:54 UTC 2017
Thanks for the review Maarten,
My comments inline.
Regards
Shashank
On 12/19/2017 12:53 AM, Maarten Lankhorst wrote:
> Op 14-11-17 om 16:17 schreef Shashank Sharma:
>> LSPCON chips are capable of generating YCBCR 4:2:0 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
>>
>> This will indicate LSPCON FW to start scaling down from YCBCR 4:4:4
>> and generate YCBCR 4:2:0 output. Unlike HDMI 2.0 native case, as the
>> scaling is done by LSPCON device, we need not to reserve a scaler for
>> 4:2:0 outputs.
>>
>> 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 | 8 ++++++++
>> drivers/gpu/drm/i915/intel_display.c | 15 +++++++++++----
>> drivers/gpu/drm/i915/intel_dp.c | 7 ++++++-
>> drivers/gpu/drm/i915/intel_drv.h | 2 ++
>> drivers/gpu/drm/i915/intel_lspcon.c | 22 ++++++++++++++++++++++
>> 6 files changed, 51 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index f0f8f60..ea6ef5e 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -8495,6 +8495,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 9e2ab02..3fd839d 100644
>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>> @@ -1499,6 +1499,14 @@ void intel_ddi_set_pipe_settings(const struct intel_crtc_state *crtc_state)
>> break;
>> }
>>
>> + /*
>> + * To get YCBCR 420 output from LSPCON, we should send YCBCR 444
>> + * signals. And 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->lspcon_active && crtc_state->ycbcr420)
>> + 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 737de25..787119b 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4638,7 +4638,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>> */
>> need_scaling = src_w != dst_w || src_h != dst_h;
>>
>> - if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX)
>> + if (crtc_state->ycbcr420 && scaler_user == SKL_CRTC_INDEX &&
>> + !crtc_state->lspcon_active)
>> need_scaling = true;
>>
>> /*
>> @@ -8133,9 +8134,15 @@ static void haswell_set_pipemisc(struct drm_crtc *crtc)
>> val |= PIPEMISC_DITHER_ENABLE | PIPEMISC_DITHER_TYPE_SP;
>>
>> if (config->ycbcr420) {
>> - val |= PIPEMISC_OUTPUT_COLORSPACE_YUV |
>> - PIPEMISC_YUV420_ENABLE |
>> - PIPEMISC_YUV420_MODE_FULL_BLEND;
>> + val |= PIPEMISC_OUTPUT_COLORSPACE_YUV;
>> + /*
>> + * LSPCON doesn't need scaling/blending to be done in
>> + * pipe. It just needs YCBCR444 input and proper AVI
>> + * infoframes for 4:2:0 output enabling.
>> + */
>> + if (!config->lspcon_active)
>> + val |= PIPEMISC_YUV420_ENABLE |
>> + PIPEMISC_YUV420_MODE_FULL_BLEND;
>> }
>>
>> I915_WRITE(PIPEMISC(intel_crtc->pipe), val);
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index fa9e5e6..1d4b669 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1657,8 +1657,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>> intel_dp->max_link_rate);
>>
>> /* LSPCON needs special handling to drive YCBCR420 outputs */
>> - if (lspcon->active)
>> + if (lspcon->active) {
>> + struct drm_connector *connector = &intel_connector->base;
>> +
>> pipe_config->lspcon_active = true;
>> + pipe_config->ycbcr420 = lspcon_ycbcr420_config(connector,
>> + pipe_config);
>> + }
>>
>> /* No common link rates between source and sink */
>> WARN_ON(common_len <= 0);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index a468dd6..f271967 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 77f0687..ca77f50 100644
>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>> @@ -180,6 +180,26 @@ 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;
>> + }
> Seems to be always true if lspcon->active is true?
Actually you are correct, if HAS_LSPCON && lspcon->active, ycbcr_420
will be always allowed. But as we added this flag specifically to
control 4:2:0 support from connector, for HDMI 2.0 native controllers, I
though it would be more readable to use this flag. Do you think so ?
>> + config->port_clock /= 2;
>> + return true;
>> + }
>> +
>> + return false;
>> +}
>> +
>> static bool lspcon_probe(struct intel_lspcon *lspcon)
>> {
>> int retry;
>> @@ -516,6 +536,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");
>> @@ -540,6 +561,7 @@ bool lspcon_init(struct intel_digital_port *intel_dig_port)
>> return false;
>> }
>>
>> + connector->ycbcr_420_allowed = true;
> Is this flag different from lspcon->active?
Yes, when we designed HDMI 2.0 native controller from GLK and plus
platforms, we added this connector flag, so that, for the platforms
which do not support 4:2:0 outputs, 4:2:0 only modes will be rejected
from EDID's list of modes. This is the place where connector allows the
4:2:0 capability, based on the platform, whereas is_lspcon is fro the
encoder and state.
>> lspcon->active = true;
>> DRM_DEBUG_KMS("Success: LSPCON init\n");
>> return true;
> Comments from Ville still apply too. The pipe is set to ycbcr mode, so it makes more sense to have a ycbcr444 flag or something, and a separate flag to downconvert lspcon.
>
> It would be really nice if we could set the ycbcr444 flag in hw readout, and intel_pipe_config_compare. This way we know the sw mode matches the hw mode. :)
We will eventually need the right colorspace information while
programming the AVI IF.
Actually we are trying to accommodate LSPCON into HDMI 2.0 framework as
such. If you analyze the native 4:2:0 framework, there also we need
scaling down, for which we use PIPEMISC register. So the requirement for
GLK would be:
- one flag like YCBCR444 for pipe config (say state->444) to set CSC
- one flag in state to let modeset know that we have to program PIPEMISC
and reserve a pipe_scaler (say state->420)
- one flag to set avi_if colorspace to ycbcr420 (again state->420, here
we can't use 444, as we need accurate colorspace)
Now, in case of LSPCON,
- I still need state->444 to set CSC
- In modeset, I don't need to program PIPEMISC and scalar, as scaling
happens in LSPCON internally so I need to know if this is LSPCON (hence
state->lspcon)
- One flag to set appropriate avi_infoframes (so state->420)
So I realized even if I use state->444, I still need a state->420 flags
to set correct colorspace in the avi infoframes, so there is no benefit
of making this change.
Please let me know your opinion.
- Shashank
> ~Maarten
>
More information about the Intel-gfx
mailing list