[Intel-gfx] [PATCH] drm/i915/display: Fix shared dpll mismatch for bigjoiner slave

[Nautiyal, Ankit K]

On 7/20/2021 12:28 AM, Navare, Manasi wrote:
> On Mon, Jul 19, 2021 at 11:47:51AM +0530, Nautiyal, Ankit K wrote:
>> Patch looks good to me.
>>
>> Please find below some suggestions
>>
>> On 7/15/2021 4:04 AM, Manasi Navare wrote:
>>> Currently when we do the HW state readout, we dont set the shared dpll to NULL
>>> for the bigjoiner slave which should not have a DPLL assigned. So it has
>>> some garbage while the HW state readout is NULL. So explicitly reset
>>> the shared dpll for bigjoiner slave pipe.
>>>
>>> Bug: https://gitlab.freedesktop.org/drm/intel/-/issues/3465
>>> Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
>>> Cc: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>>> Tested-By: Swati Sharma <swati2.sharma at intel.com>
>> checkpatch warning about tested-by tag.
>>> Signed-off-by: Manasi Navare <manasi.d.navare at intel.com>
>>> ---
>>>   drivers/gpu/drm/i915/display/intel_display.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
>>> index 65ddb6ca16e6..c274bfb8e549 100644
>>> --- a/drivers/gpu/drm/i915/display/intel_display.c
>>> +++ b/drivers/gpu/drm/i915/display/intel_display.c
>>> @@ -9006,6 +9006,10 @@ verify_crtc_state(struct intel_crtc *crtc,
>>>   	if (!new_crtc_state->hw.active)
>>>   		return;
>>> +	if (new_crtc_state->bigjoiner_slave)
>>> +		/* No PLLs set for slave */
>>> +		pipe_config->shared_dpll = NULL;
>>> +
>> there is a check for bigjoiner_slave, couple of lines above this:
>>
>> if (new_crtc_state->bigjoiner_slave)
>>                  master = new_crtc_state->bigjoiner_linked_crtc;
>>
>> Perhaps it will make sense to club the set shared_dpll to NULL, along with
>> these lines.
> Yup, thats where I was resetting in earlier patch but then it actually gets overridden in a function call
> after this point so need to reset it after separately.
>
> Manasi

You are right. I missed that, pipe_config gets overwritten just before 
this point, so the change is at the right place.

Regards,

Ankit


>
>> In any case:
>>
>> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal at intel.com>
>>
>>>   	intel_pipe_config_sanity_check(dev_priv, pipe_config);
>>>   	if (!intel_pipe_config_compare(new_crtc_state,