From: Doug Anderson <dianders@chromium.org> To: Vinod Polimera <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>, quic_kalyant <quic_kalyant@quicinc.com>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, "Kuogee Hsieh (QUIC)" <quic_khsieh@quicinc.com>, quic_vproddut <quic_vproddut@quicinc.com>, Bjorn Andersson <bjorn.andersson@linaro.org>, "Aravind Venkateswaran (QUIC)" <quic_aravindh@quicinc.com>, "Abhinav Kumar (QUIC)" <quic_abhinavk@quicinc.com>, Sankeerth Billakanti <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 Date: Thu, 28 Jul 2022 17:18:06 -0700 [thread overview] Message-ID: <CAD=FV=WW1BxjW9B9Vg99UOeTBePE32J++O24unnddecXXcaZ+Q@mail.gmail.com> (raw) In-Reply-To: <1657544224-10680-2-git-send-email-quic_vpolimer@quicinc.com> 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")? > 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
WARNING: multiple messages have this Message-ID (diff)
From: Doug Anderson <dianders@chromium.org> To: Vinod Polimera <quic_vpolimer@quicinc.com> Cc: quic_kalyant <quic_kalyant@quicinc.com>, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" <devicetree@vger.kernel.org>, Sankeerth Billakanti <quic_sbillaka@quicinc.com>, "Abhinav Kumar \(QUIC\)" <quic_abhinavk@quicinc.com>, quic_vproddut <quic_vproddut@quicinc.com>, linux-arm-msm <linux-arm-msm@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>, dri-devel <dri-devel@lists.freedesktop.org>, Stephen Boyd <swboyd@chromium.org>, Bjorn Andersson <bjorn.andersson@linaro.org>, Dmitry Baryshkov <dmitry.baryshkov@linaro.org>, "Aravind Venkateswaran \(QUIC\)" <quic_aravindh@quicinc.com>, "Kuogee Hsieh \(QUIC\)" <quic_khsieh@quicinc.com>, freedreno <freedreno@lists.freedesktop.org> Subject: Re: [PATCH v6 01/10] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc Date: Thu, 28 Jul 2022 17:18:06 -0700 [thread overview] Message-ID: <CAD=FV=WW1BxjW9B9Vg99UOeTBePE32J++O24unnddecXXcaZ+Q@mail.gmail.com> (raw) In-Reply-To: <1657544224-10680-2-git-send-email-quic_vpolimer@quicinc.com> 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")? > 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
next prev parent reply other threads:[~2022-07-29 0:18 UTC|newest] Thread overview: 59+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-07-11 12:56 [PATCH v6 00/10] Add PSR support for eDP Vinod Polimera 2022-07-11 12:56 ` Vinod Polimera 2022-07-11 12:56 ` [PATCH v6 01/10] drm/msm/disp/dpu: clear dpu_assign_crtc and get crtc from connector state instead of dpu_enc Vinod Polimera 2022-07-11 12:56 ` Vinod Polimera 2022-07-14 5:08 ` Vinod Polimera 2022-07-14 5:08 ` Vinod Polimera 2022-07-14 5:08 ` Vinod Polimera 2022-07-29 0:18 ` Doug Anderson [this message] 2022-07-29 0:18 ` Doug Anderson 2022-09-13 14:52 ` Vinod Polimera 2022-09-13 14:52 ` Vinod Polimera 2022-07-11 12:56 ` [PATCH v6 02/10] drm: add helper functions to retrieve old and new crtc Vinod Polimera 2022-07-11 12:56 ` Vinod Polimera 2022-07-14 5:08 ` Vinod Polimera 2022-07-14 5:08 ` Vinod Polimera 2022-07-14 5:08 ` Vinod Polimera 2022-07-29 0:18 ` Doug Anderson 2022-07-29 0:18 ` Doug Anderson 2022-07-11 12:56 ` [PATCH v6 03/10] drm/msm/dp: use atomic callbacks for DP bridge ops Vinod Polimera 2022-07-11 12:56 ` Vinod Polimera 2022-07-11 14:20 ` Dmitry Baryshkov 2022-07-11 14:20 ` Dmitry Baryshkov 2022-07-29 0:18 ` Doug Anderson 2022-07-29 0:18 ` Doug Anderson 2022-07-11 12:56 ` [PATCH v6 04/10] drm/msm/dp: Add basic PSR support for eDP Vinod Polimera 2022-07-11 12:56 ` Vinod Polimera 2022-07-11 14:21 ` Dmitry Baryshkov 2022-07-11 14:21 ` Dmitry Baryshkov 2022-07-29 0:20 ` Doug Anderson 2022-07-29 0:20 ` Doug Anderson 2022-10-12 7:23 ` Sankeerth Billakanti 2022-10-12 7:23 ` Sankeerth Billakanti 2022-07-11 12:56 ` [PATCH v6 05/10] drm/msm/dp: use the eDP bridge ops to validate eDP modes Vinod Polimera 2022-07-11 12:56 ` Vinod Polimera 2022-07-11 14:18 ` Dmitry Baryshkov 2022-07-11 14:18 ` Dmitry Baryshkov 2022-07-11 12:57 ` [PATCH v6 06/10] drm/bridge: use atomic enable/disable callbacks for panel bridge Vinod Polimera 2022-07-11 12:57 ` Vinod Polimera 2022-07-11 12:57 ` [PATCH v6 07/10] drm/bridge: add psr support for panel bridge callbacks Vinod Polimera 2022-07-11 12:57 ` Vinod Polimera 2022-07-14 5:09 ` Vinod Polimera 2022-07-14 5:09 ` Vinod Polimera 2022-07-14 5:09 ` Vinod Polimera 2022-07-11 12:57 ` [PATCH v6 08/10] drm/msm/disp/dpu: use atomic enable/disable callbacks for encoder functions Vinod Polimera 2022-07-11 12:57 ` Vinod Polimera 2022-07-11 12:57 ` [PATCH v6 09/10] drm/msm/disp/dpu: add PSR support for eDP interface in dpu driver Vinod Polimera 2022-07-11 12:57 ` Vinod Polimera 2022-07-11 14:19 ` Dmitry Baryshkov 2022-07-11 14:19 ` Dmitry Baryshkov 2022-07-11 12:57 ` [PATCH v6 10/10] drm/msm/disp/dpu: check for crtc enable rather than crtc active to release shared resources Vinod Polimera 2022-07-11 12:57 ` Vinod Polimera 2022-07-11 14:19 ` Dmitry Baryshkov 2022-07-11 14:19 ` Dmitry Baryshkov 2022-07-29 0:22 ` [PATCH v6 00/10] Add PSR support for eDP Doug Anderson 2022-07-29 0:22 ` Doug Anderson 2022-08-04 16:21 ` Robert Foss 2022-08-04 16:21 ` Robert Foss 2022-08-04 17:54 ` Doug Anderson 2022-08-04 17:54 ` Doug Anderson
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to='CAD=FV=WW1BxjW9B9Vg99UOeTBePE32J++O24unnddecXXcaZ+Q@mail.gmail.com' \ --to=dianders@chromium.org \ --cc=bjorn.andersson@linaro.org \ --cc=devicetree@vger.kernel.org \ --cc=dmitry.baryshkov@linaro.org \ --cc=dri-devel@lists.freedesktop.org \ --cc=freedreno@lists.freedesktop.org \ --cc=linux-arm-msm@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=quic_abhinavk@quicinc.com \ --cc=quic_aravindh@quicinc.com \ --cc=quic_kalyant@quicinc.com \ --cc=quic_khsieh@quicinc.com \ --cc=quic_sbillaka@quicinc.com \ --cc=quic_vpolimer@quicinc.com \ --cc=quic_vproddut@quicinc.com \ --cc=robdclark@gmail.com \ --cc=swboyd@chromium.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.