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

[Sharma, Shashank]

Regards

Shashank


On 10/15/2018 6:42 PM, Jani Nikula wrote:
> 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.
Thanks Jani, I will have a look at the series post merge, and send any 
follow up / cleanup patches if required.
- Shashank
> BR,
> Jani.
>
>
>