diff mbox series

drm/i915/display: Fix shared dpll mismatch for bigjoiner slave

Message ID 20210714223414.9849-1-manasi.d.navare@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/display: Fix shared dpll mismatch for bigjoiner slave | expand

Commit Message

Navare, Manasi July 14, 2021, 10:34 p.m. UTC 
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@linux.intel.com>
Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
Tested-By: Swati Sharma <swati2.sharma@intel.com>
Signed-off-by: Manasi Navare <manasi.d.navare@intel.com>
---
 drivers/gpu/drm/i915/display/intel_display.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Nautiyal, Ankit K July 19, 2021, 6:17 a.m. UTC | #1 
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@linux.intel.com>
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Tested-By: Swati Sharma <swati2.sharma@intel.com>
checkpatch warning about tested-by tag.
> Signed-off-by: Manasi Navare <manasi.d.navare@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.

In any case:

Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>

>   	intel_pipe_config_sanity_check(dev_priv, pipe_config);
>   
>   	if (!intel_pipe_config_compare(new_crtc_state,
Navare, Manasi July 19, 2021, 6:58 p.m. UTC | #2 
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@linux.intel.com>
> >Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >Tested-By: Swati Sharma <swati2.sharma@intel.com>
> checkpatch warning about tested-by tag.
> >Signed-off-by: Manasi Navare <manasi.d.navare@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

> 
> In any case:
> 
> Reviewed-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> 
> >  	intel_pipe_config_sanity_check(dev_priv, pipe_config);
> >  	if (!intel_pipe_config_compare(new_crtc_state,
Nautiyal, Ankit K July 20, 2021, 7:23 a.m. UTC | #3 
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@linux.intel.com>
>>> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>> Tested-By: Swati Sharma <swati2.sharma@intel.com>
>> checkpatch warning about tested-by tag.
>>> Signed-off-by: Manasi Navare <manasi.d.navare@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@intel.com>
>>
>>>   	intel_pipe_config_sanity_check(dev_priv, pipe_config);
>>>   	if (!intel_pipe_config_compare(new_crtc_state,
Navare, Manasi July 21, 2021, 5:42 p.m. UTC | #4 
Pushed to drm-intel-next, tahnks for the reviews.

Manasi

On Tue, Jul 20, 2021 at 12:53:23PM +0530, Nautiyal, Ankit K wrote:
> 
> 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@linux.intel.com>
> >>>Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> >>>Tested-By: Swati Sharma <swati2.sharma@intel.com>
> >>checkpatch warning about tested-by tag.
> >>>Signed-off-by: Manasi Navare <manasi.d.navare@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@intel.com>
> >>
> >>>  	intel_pipe_config_sanity_check(dev_priv, pipe_config);
> >>>  	if (!intel_pipe_config_compare(new_crtc_state,
Jani Nikula Aug. 10, 2021, 1:19 p.m. UTC | #5 
On Wed, 14 Jul 2021, Manasi Navare <manasi.d.navare@intel.com> 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@linux.intel.com>
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Tested-By: Swati Sharma <swati2.sharma@intel.com>
> Signed-off-by: Manasi Navare <manasi.d.navare@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;
> +

I know it's been merged already, but feels wrong to have this in
verify_crtc_state(). Kind of out of place.

BR,
Jani.

>  	intel_pipe_config_sanity_check(dev_priv, pipe_config);
>  
>  	if (!intel_pipe_config_compare(new_crtc_state,
diff mbox series

Patch

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;
+
 	intel_pipe_config_sanity_check(dev_priv, pipe_config);
 
 	if (!intel_pipe_config_compare(new_crtc_state,