[Intel-gfx] [PATCH v12 8/8] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
Sharma, Shashank
shashank.sharma at intel.com
Fri Oct 12 18:32:25 UTC 2018
Regards
Shashank
On 10/12/2018 11:39 PM, Ville Syrjälä wrote:
> On Fri, Oct 12, 2018 at 11:21:56PM +0530, Sharma, Shashank wrote:
>> Regards
>>
>> Shashank
>>
>>
>> On 10/12/2018 7:28 PM, Ville Syrjälä wrote:
>>> On Fri, Oct 12, 2018 at 11:53:14AM +0530, Shashank Sharma wrote:
>>>> From: Shashank Sharma <shashank.sharma at linux.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.
>>>> V5: Addressed review comments from Ville:
>>>> - Change the state bool name from external scaling to something more
>>>> relavent.
>>>> - Keep the info and adjusted_mode structures const.
>>>> - use crtc_state instead of pipe_config.
>>>> - Push all the config change into lspcon_ycbcr420_config function.
>>>> V6: Rebase, small changes to accommodate changes in patch 2.
>>>> V7: Fixed checkpatch warnings for alignment
>>>> V8: Rebase
>>>>
>>>> PS: Ignored following warnings to match the current formatting:
>>>> drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON
>>>> -:53: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
>>>> #53: FILE: drivers/gpu/drm/i915/i915_reg.h:8721:
>>>> +#define TRANS_MSA_SAMPLING_444 (2<<1)
>>>> ^
>>>> -:54: CHECK:SPACING: spaces preferred around that '<<' (ctx:VxV)
>>>> #54: FILE: drivers/gpu/drm/i915/i915_reg.h:8722:
>>>> +#define TRANS_MSA_CLRSP_YCBCR (2<<3)
>>>> V9: Rebase
>>>> V10: Rebase
>>>> V11: Rebase
>>>>
>>>> Cc: Ville Syrjala <ville.syrjala at linux.intel.com>
>>>> Cc: Maarten Lankhorst <maarten.lankhorst at linux.intel.com>
>>>> Reviewed-by: 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_display.c | 12 ++++++++++++
>>>> drivers/gpu/drm/i915/intel_dp.c | 4 ++++
>>>> drivers/gpu/drm/i915/intel_drv.h | 5 +++++
>>>> drivers/gpu/drm/i915/intel_lspcon.c | 26 ++++++++++++++++++++++++++
>>>> 6 files changed, 56 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>>>> index 2078541..1e13e51 100644
>>>> --- a/drivers/gpu/drm/i915/i915_reg.h
>>>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>>>> @@ -9229,6 +9229,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 be21131..186111a 100644
>>>> --- a/drivers/gpu/drm/i915/intel_ddi.c
>>>> +++ b/drivers/gpu/drm/i915/intel_ddi.c
>>>> @@ -1784,6 +1784,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 == INTEL_OUTPUT_FORMAT_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 3317c6c..41abd03 100644
>>>> --- a/drivers/gpu/drm/i915/intel_display.c
>>>> +++ b/drivers/gpu/drm/i915/intel_display.c
>>>> @@ -7797,6 +7797,8 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc,
>>>> struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
>>>> enum intel_output_format output = INTEL_OUTPUT_FORMAT_RGB;
>>>>
>>>> + pipe_config->lspcon_downsampling = false;
>>>> +
>>>> if (IS_BROADWELL(dev_priv) || INTEL_GEN(dev_priv) >= 9) {
>>>> u32 tmp = I915_READ(PIPEMISC(crtc->pipe));
>>>>
>>>> @@ -7814,6 +7816,16 @@ static void intel_get_crtc_ycbcr_config(struct intel_crtc *crtc,
>>>> else
>>>> output = INTEL_OUTPUT_FORMAT_YCBCR420;
>>>> } else {
>>>> + /*
>>>> + * Currently there is no interface defined to
>>>> + * check user preference between RGB/YCBCR444
>>>> + * or YCBCR420. So the only possible case for
>>>> + * YCBCR444 usage is driving YCBCR420 output
>>>> + * with LSPCON, when pipe is configured for
>>>> + * YCBCR444 output and LSPCON takes care of
>>>> + * downsampling it.
>>>> + */
>>>> + pipe_config->lspcon_downsampling = true;
>>>> output = INTEL_OUTPUT_FORMAT_YCBCR444;
>>>> }
>>>> }
>>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>>>> index c3f6330..1156d59 100644
>>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>>> @@ -2074,6 +2074,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;
>>>> @@ -2086,6 +2087,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>>>> pipe_config->has_pch_encoder = true;
>>>>
>>>> pipe_config->output_format = INTEL_OUTPUT_FORMAT_RGB;
>>>> + if (lspcon->active)
>>>> + lspcon_ycbcr420_config(&intel_connector->base, pipe_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 b8e5f95..95e252e 100644
>>>> --- a/drivers/gpu/drm/i915/intel_drv.h
>>>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>>>> @@ -908,6 +908,9 @@ struct intel_crtc_state {
>>>>
>>>> /* Output format RGB/YCBCR etc */
>>>> enum intel_output_format output_format;
>>>> +
>>>> + /* Output down scaling is done in LSPCON device */
>>>> + bool lspcon_downsampling;
>>>> };
>>>>
>>>> struct intel_crtc {
>>>> @@ -2225,6 +2228,8 @@ void lspcon_set_infoframes(struct intel_encoder *encoder,
>>>> const struct drm_connector_state *conn_state);
>>>> bool lspcon_infoframe_enabled(struct intel_encoder *encoder,
>>>> const struct intel_crtc_state *pipe_config);
>>>> +void lspcon_ycbcr420_config(struct drm_connector *connector,
>>>> + struct intel_crtc_state *crtc_state);
>>>>
>>>> /* intel_pipe_crc.c */
>>>> #ifdef CONFIG_DEBUG_FS
>>>> diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
>>>> index 829c40a..fff32b3 100644
>>>> --- a/drivers/gpu/drm/i915/intel_lspcon.c
>>>> +++ b/drivers/gpu/drm/i915/intel_lspcon.c
>>>> @@ -180,6 +180,21 @@ static bool lspcon_wake_native_aux_ch(struct intel_lspcon *lspcon)
>>>> return true;
>>>> }
>>>>
>>>> +void lspcon_ycbcr420_config(struct drm_connector *connector,
>>>> + struct intel_crtc_state *crtc_state)
>>>> +{
>>>> + const struct drm_display_info *info = &connector->display_info;
>>>> + const struct drm_display_mode *adjusted_mode =
>>>> + &crtc_state->base.adjusted_mode;
>>>> +
>>>> + if (drm_mode_is_420_only(info, adjusted_mode) &&
>>>> + connector->ycbcr_420_allowed) {
>>>> + crtc_state->port_clock /= 2;
>>> This looks bogus. We're talking about DP here so we should not be
>>> frobbing port_clock. And since we output 4:4:4 from the port
>>> anyway we don't even have to take 4:2:0 into consideration
>>> when calculating port_clock.
>> I agree on this.
>>> Ah, this guy is called before we even calculate port_clock.
>>> That explains why this didn't cause any problems. So this is
>>> just dead code here.
>>>
>>> And looks like the port_clock adjustment in intel_hdmi_ycbcr420_config()
>>> also dead code since the caller overwrites it there as well.
>> I am not sure if that's the case for Native HDMI 2.0.
>> In intel_hdmi_420_config we are updating the config->port_clock, which
>> is the same thing being updated for 8/12/16 BPC deep color too, just
>> after this function.
>> I have tested the output with HDMI 2.0 analyzer, which reported right
>> pixel clock (297Mhz). Now, as any of the port clock calculations do not
>> get information
>> about HDMI output type, there is no other way they will calculate the
>> right pixel clock (594/2) for 4:2:0 outputs. So I believe that code is
>> active.
> It effectively does this:
>
> 1. port_clock = who knows
> 2. port_clock /= 2;
Here, in intel_hdmi_compute_ycbcr_config() function, we are additionally
doing this also:
clock_8bpc /= 2;
clock_10bpc /= 2;
clock_12bpc /= 2;
> 3. if (12bpc)
> port_clock = clock_12bpc;
> else if (10bpc)
> port_clock = clock_10bpc;
> else
> port_clock = clock_8bpc;
This means effectively the selected clock is /2, so the code is not dead
(thankfully :-))
- Shashank
>
> Ie. step 2. is dead code.
>
>> - Shashank
>>>> + crtc_state->output_format = INTEL_OUTPUT_FORMAT_YCBCR444;
>>>> + crtc_state->lspcon_downsampling = true;
>>>> + }
>>>> +}
>>>> +
>>>> static bool lspcon_probe(struct intel_lspcon *lspcon)
>>>> {
>>>> int retry;
>>>> @@ -464,6 +479,15 @@ void lspcon_set_infoframes(struct intel_encoder *encoder,
>>>> return;
>>>> }
>>>>
>>>> + if (crtc_state->output_format == INTEL_OUTPUT_FORMAT_YCBCR444) {
>>>> + if (crtc_state->lspcon_downsampling)
>>>> + 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 :
>>>> @@ -517,6 +541,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");
>>>> @@ -541,6 +566,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
More information about the Intel-gfx
mailing list