RE: [PATCH v6 01/10] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc

[Vinod Polimera <vpolimer@qti.qualcomm.com>]

> -----Original Message-----
> From: Doug Anderson <dianders@chromium.org>
> Sent: Friday, July 29, 2022 5:48 AM
> To: Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>
> Cc: dri-devel <dri-devel@lists.freedesktop.org>; linux-arm-msm <linux-arm-
> msm@vger.kernel.org>; freedreno <freedreno@lists.freedesktop.org>;
> open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS
> <devicetree@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>; Rob
> Clark <robdclark@gmail.com>; Stephen Boyd <swboyd@chromium.org>;
> Kalyan Thota (QUIC) <quic_kalyant@quicinc.com>;
> dmitry.baryshkov@linaro.org; Kuogee Hsieh (QUIC)
> <quic_khsieh@quicinc.com>; Vishnuvardhan Prodduturi (QUIC)
> <quic_vproddut@quicinc.com>; bjorn.andersson@linaro.org; Aravind
> Venkateswaran (QUIC) <quic_aravindh@quicinc.com>; Abhinav Kumar
> (QUIC) <quic_abhinavk@quicinc.com>; Sankeerth Billakanti (QUIC)
> <quic_sbillaka@quicinc.com>
> Subject: Re: [PATCH v6 01/10] drm/msm/disp/dpu: clear dpu_assign_crtc and
> get crtc from connector state instead of dpu_enc
> 
> WARNING: This email originated from outside of Qualcomm. Please be wary
> of any links or attachments, and do not enable macros.
> 
> Hi,
> 
> On Mon, Jul 11, 2022 at 5:57 AM Vinod Polimera
> <quic_vpolimer@quicinc.com> wrote:
> >
> > Update crtc retrieval from dpu_enc to dpu_enc connector state,
> > since new links get set as part of the dpu enc virt mode set.
> > The dpu_enc->crtc cache is no more needed, hence cleaning it as
> > part of this change.
> 
> I don't know this driver terribly well, but _why_ is it no longer
> needed? According to the kernel-doc for the "crtc" variable you're
> removing it was because we used to need it after the disable()
> callback. Maybe that's no longer the case after commit a796ba2cb3dd
> ("drm/msm: dpu: Separate crtc assignment from vblank enable")?

drm encoder already has crtc and the same link is copied into dpu encoder which appears redundant. Dmitry also pointed out the same thing in earlier comments. Hence it was removed.
> 
> 
> > Signed-off-by: Vinod Polimera <quic_vpolimer@quicinc.com>
> > ---
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c    |  4 ----
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 30 ++++++++++++++-
> --------------
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h |  8 --------
> >  3 files changed, 14 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > index b56f777..f91e3d1 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> > @@ -972,7 +972,6 @@ static void dpu_crtc_disable(struct drm_crtc *crtc,
> >                  */
> >                 if (dpu_encoder_get_intf_mode(encoder) == INTF_MODE_VIDEO)
> >                         release_bandwidth = true;
> > -               dpu_encoder_assign_crtc(encoder, NULL);
> >         }
> >
> >         /* wait for frame_event_done completion */
> > @@ -1042,9 +1041,6 @@ static void dpu_crtc_enable(struct drm_crtc *crtc,
> >         trace_dpu_crtc_enable(DRMID(crtc), true, dpu_crtc);
> >         dpu_crtc->enabled = true;
> >
> > -       drm_for_each_encoder_mask(encoder, crtc->dev, crtc->state-
> >encoder_mask)
> > -               dpu_encoder_assign_crtc(encoder, crtc);
> > -
> >         /* Enable/restore vblank irq handling */
> >         drm_crtc_vblank_on(crtc);
> >  }
> > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > index 52516eb..0fddc9d 100644
> > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> > @@ -181,7 +181,6 @@ struct dpu_encoder_virt {
> >
> >         bool intfs_swapped;
> >
> > -       struct drm_crtc *crtc;
> 
> This structure is documented by kernel-doc. That means you need to
> remove the documentation for "crtc".
> 
> 
> >         struct drm_connector *connector;
> >
> >         struct dentry *debugfs_root;
> > @@ -1245,6 +1244,7 @@ static void dpu_encoder_vblank_callback(struct
> drm_encoder *drm_enc,
> >                 struct dpu_encoder_phys *phy_enc)
> >  {
> >         struct dpu_encoder_virt *dpu_enc = NULL;
> > +       struct drm_crtc *crtc;
> >         unsigned long lock_flags;
> >
> >         if (!drm_enc || !phy_enc)
> > @@ -1253,9 +1253,14 @@ static void dpu_encoder_vblank_callback(struct
> drm_encoder *drm_enc,
> >         DPU_ATRACE_BEGIN("encoder_vblank_callback");
> >         dpu_enc = to_dpu_encoder_virt(drm_enc);
> >
> > +       if (!dpu_enc->connector || !dpu_enc->connector->state)
> > +               return;
> 
> FWIW: your patch doesn't apply cleanly to msm-next. It conflicts with
> commit c28d76d360f9 ("drm/msm/dpu: Increment vsync_cnt before waking
> up userspace").
> 
> I suspect that you'll want your changes to come _after_ the increment
> (AKA you want to increment even if the connector is NULL), but dunno
> for sure.
> 
> 
> > +
> > +       crtc = dpu_enc->connector->state->crtc;
> > +
> >         spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > -       if (dpu_enc->crtc)
> > -               dpu_crtc_vblank_callback(dpu_enc->crtc);
> > +       if (crtc)
> > +               dpu_crtc_vblank_callback(crtc);
> 
> Effectively you are checking for NULLness at 3 levels:
> 
> 1. dpu_enc->connector
> 2. dpu_enc->connector->state
> 3. dpu_enc->connector->state->crtc
> 
> You check two of those things outside of the spinlock and one of those
> things inside the spinlock. Why? Should they all be inside the
> spinlock, or can they all be outside of the spinlock, or is there some
> reason it is the way it is?
> 
> 
> >  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);
> > +       struct drm_crtc *new_crtc;
> >         unsigned long lock_flags;
> >         int i;
> >
> >         trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable);
> >
> > +       if (!dpu_enc->connector || !dpu_enc->connector->state)
> > +               return;
> > +
> > +       new_crtc = dpu_enc->connector->state->crtc;
> >         spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags);
> > -       if (dpu_enc->crtc != crtc) {
> > +       if (!new_crtc || new_crtc != crtc) {
> >                 spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags);
> 
> Even if there was some reason for your choice of where you did the
> spinlock in the previous case, I'm 95% sure that this one is absurd.
> You're locking a spinlock around a test of local variables? I'm pretty
> sure nobody else could be messing with your local variables...
> 
> 
> -Doug

Thanks,
Vinod P.