On Wed, Nov 14, 2018 at 11:43:51AM -0800, Jeykumar Sankaran wrote:
> On 2018-11-14 07:16, Sean Paul wrote:
> > On Tue, Nov 13, 2018 at 04:48:12PM -0800, Jeykumar Sankaran wrote:
> > > On 2018-11-13 12:52, Sean Paul wrote:
> > > > From: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > > >
> > > > Instead of assigning/clearing the crtc on vblank enable/disable, we
> > can
> > > > just assign and clear the crtc on modeset. That allows us to just
> > toggle
> > > > the encoder's vblank interrupts on vblank_enable.
> > > >
> > > > So why is this important? Previously the driver was using the legacy
> > > > pointers to assign/clear the crtc. Legacy pointers are cleared _after_
> > > Which pointers are you referring here as legacy pointers? CRTC?
> >
> > encoder->crtc in this case. The loop in vblank_enable_no_lock relies on
> > enc->crtc == crtc
> >
> > > > disabling the hardware, so the legacy pointer was valid during
> > > > vblank_disable, but that's not something we should rely on.
> > > >
> > > > Instead of relying on the core ordering the legacy pointer assignments
> > > > just so, we'll assign the crtc in dpu_crtc enable/disable. This is the
> > > > only place that mapping can change, so we're covered there.
> > > >
> > > > We're also taking advantage of drm_crtc_vblank_on/off. By using this,
> > we
> > > > ensure that vblank_enable/disable can never be called while the crtc
> > is
> > > > off (which means the assigned crtc will always be valid). As such, we
> > >
> > > What about the drm_vblank_enable/disable triggered by drm_vblank_get
> > when
> > > crtc
> > > is disabled? What is the expected behavior there? the vblank_requested
> > > variable removed by the next patch was introduced to cache the
> > > request.
> >
> > That case is covered by the modeset locks held when calling disable().
> >
> I am sure it will take care of drm_crtc_vblank_on/off triggered within
> crtc_disable.
> My question was what was the expected behaviour when DRM_IOCTL_WAIT_VBLANK
> is
> called when crtc is disabled? the ioctl will try to call drm_vblank_get and
> I
> don't see the patch checking for crtc being enabled in the path.
It's the same as any other drm_vblank_get call when the crtc is disabled, it's
gated internally by the core when vblank is turned off.
>
> Thanks and Regards,
> Jeykumar S.
>
>
> > >
> > > > don't need to use modeset locks or the crtc_lock in the
> > > > vblank_enable/disable routine to be sure state is consistent.
> > > >
> > > > ...I think.
> > > >
> > > > Signed-off-by: Sean Paul <seanpaul@xxxxxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c | 77
> > +++++++++------------
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 27 +++++---
> > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 10 +++
> > > > 3 files changed, 59 insertions(+), 55 deletions(-)
> > > >
/snip
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > index fd6514f681ae..5914ae70572c 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > > > @@ -1267,22 +1267,29 @@ void dpu_encoder_assign_crtc(struct
> > drm_encoder
> > > > *drm_enc, struct drm_crtc *crtc)
> > > > {
> > > > struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> > > > unsigned long lock_flags;
> > > > - bool enable;
> > > > - int i;
> > > > -
> > > > - enable = crtc ? true : false;
> > > > -
> > > > - if (!drm_enc) {
> > > > - DPU_ERROR("invalid encoder\n");
> > > > - return;
> > > > - }
> > > > - trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
> > > >
> > > > spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > > > /* crtc should always be cleared before re-assigning */
> > > > WARN_ON(crtc && dpu_enc->crtc);
> > > > dpu_enc->crtc = crtc;
> > > > spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> > > > +}
> > > > +
> > > > +void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc,
> > > > + struct drm_crtc *crtc, bool
> > > > enable)
> > > > +{
> > > > + struct dpu_encoder_virt *dpu_enc = to_dpu_encoder_virt(drm_enc);
> > > > + unsigned long lock_flags;
> > > > + int i;
> > > > +
> > > > + trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
> > > > +
> > > > + spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > > > + if (dpu_enc->crtc == crtc) {
> > > > + spin_unlock_irqrestore(&dpu_enc->enc_spinlock,
> > > > lock_flags);
> > > > + return;
> > > > + }
> > > Why are you returning when the crtc's are same?
> > > Won't they be same all the time between modesets?
> > >
Ugh, well that's embarrassing! This is supposed to be !=
> > > for both enable/disable crtc will be the same and you still need to
> > > call
> > the
> > > for loop below to enable/disable the phys encoders.
> > >
>
> Can you go through my comments above?
Shoot, sorry, I didn't see these.
Sean
>
> > > > + spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> > > >
> > > > for (i = 0; i < dpu_enc->num_phys_encs; i++) {
> > > > struct dpu_encoder_phys *phys = dpu_enc->phys_encs[i];
> > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > > index be1d80867834..6896ea2ab854 100644
> > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h
> > > > @@ -62,6 +62,16 @@ void dpu_encoder_get_hw_resources(struct
> > drm_encoder
> > > > *encoder,
> > > > void dpu_encoder_assign_crtc(struct drm_encoder *encoder,
> > > > struct drm_crtc *crtc);
> > > >
> > > > +/**
> > > > + * dpu_encoder_toggle_vblank_for_crtc - Toggles vblank interrupts on
> > or
> > > > off if
> > > > + * the encoder is assigned to the given crtc
> > > > + * @encoder: encoder pointer
> > > > + * @crtc: crtc pointer
> > > > + * @enable: true if vblank should be enabled
> > > > + */
> > > > +void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *encoder,
> > > > + struct drm_crtc *crtc, bool
> > > > enable);
> > > > +
> > > > /**
> > > > * dpu_encoder_register_frame_event_callback - provide callback to
> > > > encoder that
> > > > * will be called after the request is complete, or other events.
> > >
> > > --
> > > Jeykumar S
>
> --
> Jeykumar S
--
Sean Paul, Software Engineer, Google / Chromium OS
