On Tue, Oct 02, 2018 at 12:59:18PM -0700, Jeykumar Sankaran wrote: > 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@xxxxxxxxxxxx> > > > > > > > > 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@xxxxxxxxxxxxxx> Awesome, thanks for your review. I've pushed to dpu-staging. Sean > > > > Can we clean up the usage of cur_master if possible in the dpms path? > > > > > > Thanks, > > > Jeykumar S. > > > > > > > > > > Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx> > > > > --- > > > > 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 -- Sean Paul, Software Engineer, Google / Chromium OS
