[Intel-gfx] [PATCH v12 8/8] drm/i915: Add YCBCR 4:2:0/4:4:4 support for LSPCON

[Jani Nikula]

On Fri, 12 Oct 2018, Ville Syrjälä <ville.syrjala at linux.intel.com> wrote:
> On Sat, Oct 13, 2018 at 12:02:25AM +0530, Sharma, Shashank wrote:
>> >>>> +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 :-))
>
> Only the port_clock/=2 part. I never claimed the rest was dead.

The whole series pushed to dinq, dismissing this last part with Ville's
approval, to get some closure here. Thanks for the patches and
review. Further patches to clean up the existing and new dead code will
be appreciated.

BR,
Jani.



-- 
Jani Nikula, Intel Open Source Graphics Center