Re: [PATCH] drm/msm: dpu: Don't reset dpu_enc->cur_master on .disable()

[Jeykumar Sankaran <jsanka-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]

On 2018-10-02 12:17, Sean Paul wrote:
> On Tue, Oct 02, 2018 at 11:09:47AM -0700, Jeykumar Sankaran wrote:
>> On 2018-09-17 13:49, Sean Paul wrote:
>> > From: Sean Paul <seanpaul@chromium.org>
>> >
>> > cur_master in dpu_encoder is assigned at modeset and cleared on
>> > .disable(). Unfortunately dpms (or enable/disable) does not guarantee
> a
>> > modeset, so cur_master is NULL when we try to re-enable it.
>> >
>> > This patch moves the NULL assignment to setup_display where it will be
>> > re-assigned later in the function.
>> >
>> The patch looks harmless to me. But the encoder->disable also calls
>> .disable() on
>> the physical encoders. If the DPMS is not invoking an enable, why does
>> it need to access cur_master when all the physical encoders are in the
>> disabled state?
> 
> DPMS is invoking enable, but cur_master is currently only set on 
> modeset
> which
> isn't invoked on DPMS.
> 
> Sean
> 
Got it! Misread the commit text.

Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org>

>> Can we clean up the usage of cur_master if possible in the dpms path?
>> 
>> Thanks,
>> Jeykumar S.
>> 
>> 
>> > Signed-off-by: Sean Paul <seanpaul@chromium.org>
>> > ---
>> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 4 ++--
>> >  1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > index c2e8985b4c54..ae03d8d43b27 100644
>> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
>> > @@ -1230,8 +1230,6 @@ static void dpu_encoder_virt_disable(struct
>> > drm_encoder *drm_enc)
>> >  			dpu_enc->phys_encs[i]->connector = NULL;
>> >  	}
>> >
>> > -	dpu_enc->cur_master = NULL;
>> > -
>> >  	DPU_DEBUG_ENC(dpu_enc, "encoder disabled\n");
>> >
>> >  	dpu_rm_release(&dpu_kms->rm, drm_enc);
>> > @@ -2072,6 +2070,8 @@ static int dpu_encoder_setup_display(struct
>> > dpu_encoder_virt *dpu_enc,
>> >  		return -EINVAL;
>> >  	}
>> >
>> > +	dpu_enc->cur_master = NULL;
>> > +
>> >  	memset(&phys_params, 0, sizeof(phys_params));
>> >  	phys_params.dpu_kms = dpu_kms;
>> >  	phys_params.parent = &dpu_enc->base;
>> 
>> --
>> Jeykumar S

-- 
Jeykumar S
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno