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 > 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 -- Sean Paul, Software Engineer, Google / Chromium OS
