| Message ID | 1657040445-13067-2-git-send-email-quic_vpolimer@quicinc.com |
|---|---|
| State | New |
| Headers | show |
| Series | Add PSR support for eDP | expand |
> NAK. Quoting the documentation: > > only really meaningful for non-atomic drivers. Atomic drivers should > instead check &drm_connector_state.crtc. > > Please adjust according to the documentation. drm_enc gets the crtc info already from new connector state as part of drm_atomic_helper_update_legacy_modeset_state. So drm_enc->crtc will be valid as we access it as part of modeset enable/disable. ```connector->encoder->crtc = new_conn_state->crtc;```. Thanks, Vinod P. > -----Original Message----- > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Sent: Wednesday, July 6, 2022 12:34 AM > To: Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>; dri- > devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org; > freedreno@lists.freedesktop.org; devicetree@vger.kernel.org > Cc: linux-kernel@vger.kernel.org; robdclark@gmail.com; > dianders@chromium.org; swboyd@chromium.org; Kalyan Thota (QUIC) > <quic_kalyant@quicinc.com>; 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 v4 1/7] drm/msm/disp/dpu1: clear dpu_assign_crtc and > get crtc from drm_enc 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. > > On 05/07/2022 20:00, Vinod Polimera wrote: > > Update crtc retrieval from dpu_enc to drm_enc, since new links get set > > as part of the update legacy mode set. The dpu_enc->crtc cache is no > > more needed, hence cleaning it as part of this change. > > NAK. Quoting the documentation: > > only really meaningful for non-atomic drivers. Atomic drivers should > instead check &drm_connector_state.crtc. > > Please adjust according to the documentation. > > > > 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 | 18 +++--------------- > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 -------- > > 3 files changed, 3 insertions(+), 27 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..5629c0b 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > @@ -1254,8 +1254,8 @@ static void dpu_encoder_vblank_callback(struct > drm_encoder *drm_enc, > > dpu_enc = to_dpu_encoder_virt(drm_enc); > > > > spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > > - if (dpu_enc->crtc) > > - dpu_crtc_vblank_callback(dpu_enc->crtc); > > + if (drm_enc->crtc) > > + dpu_crtc_vblank_callback(drm_enc->crtc); > > spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); > > > > atomic_inc(&phy_enc->vsync_cnt); > > @@ -1280,18 +1280,6 @@ static void > dpu_encoder_underrun_callback(struct drm_encoder *drm_enc, > > DPU_ATRACE_END("encoder_underrun_callback"); > > } > > > > -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; > > - > > - 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) > > { > > @@ -1302,7 +1290,7 @@ void dpu_encoder_toggle_vblank_for_crtc(struct > drm_encoder *drm_enc, > > trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); > > > > spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > > - if (dpu_enc->crtc != crtc) { > > + if (drm_enc->crtc != crtc) { > > spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); > > return; > > } > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > index 781d41c..edba815 100644 > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > @@ -39,14 +39,6 @@ struct msm_display_info { > > }; > > > > /** > > - * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to > > - * @encoder: encoder pointer > > - * @crtc: crtc pointer > > - */ > > -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 > > > -- > With best wishes > Dmitry
On Thu, 7 Jul 2022 at 17:39, Vinod Polimera <vpolimer@qti.qualcomm.com> wrote: > > > NAK. Quoting the documentation: > > > > only really meaningful for non-atomic drivers. Atomic drivers should > > instead check &drm_connector_state.crtc. > > > > Please adjust according to the documentation. > drm_enc gets the crtc info already from new connector state as part of drm_atomic_helper_update_legacy_modeset_state. > So drm_enc->crtc will be valid as we access it as part of modeset enable/disable. > ```connector->encoder->crtc = new_conn_state->crtc;```. drm_atomic_helper_update_legacy_modeset_state(): * Since these updates are not synchronized with lockings, only code paths * called from &drm_mode_config_helper_funcs.atomic_commit_tail can look at the * legacy state filled out by this helper. Defacto this means this helper and * the legacy state pointers are only really useful for transitioning an * existing driver to the atomic world. So please change the code to use drm_connector_state.crtc. Dixi. > > Thanks, > Vinod P. > > > -----Original Message----- > > From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > > Sent: Wednesday, July 6, 2022 12:34 AM > > To: Vinod Polimera (QUIC) <quic_vpolimer@quicinc.com>; dri- > > devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org; > > freedreno@lists.freedesktop.org; devicetree@vger.kernel.org > > Cc: linux-kernel@vger.kernel.org; robdclark@gmail.com; > > dianders@chromium.org; swboyd@chromium.org; Kalyan Thota (QUIC) > > <quic_kalyant@quicinc.com>; 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 v4 1/7] drm/msm/disp/dpu1: clear dpu_assign_crtc and > > get crtc from drm_enc 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. > > > > On 05/07/2022 20:00, Vinod Polimera wrote: > > > Update crtc retrieval from dpu_enc to drm_enc, since new links get set > > > as part of the update legacy mode set. The dpu_enc->crtc cache is no > > > more needed, hence cleaning it as part of this change. > > > > NAK. Quoting the documentation: > > > > only really meaningful for non-atomic drivers. Atomic drivers should > > instead check &drm_connector_state.crtc. > > > > Please adjust according to the documentation. > > > > > > 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 | 18 +++--------------- > > > drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 -------- > > > 3 files changed, 3 insertions(+), 27 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..5629c0b 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c > > > @@ -1254,8 +1254,8 @@ static void dpu_encoder_vblank_callback(struct > > drm_encoder *drm_enc, > > > dpu_enc = to_dpu_encoder_virt(drm_enc); > > > > > > spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > > > - if (dpu_enc->crtc) > > > - dpu_crtc_vblank_callback(dpu_enc->crtc); > > > + if (drm_enc->crtc) > > > + dpu_crtc_vblank_callback(drm_enc->crtc); > > > spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); > > > > > > atomic_inc(&phy_enc->vsync_cnt); > > > @@ -1280,18 +1280,6 @@ static void > > dpu_encoder_underrun_callback(struct drm_encoder *drm_enc, > > > DPU_ATRACE_END("encoder_underrun_callback"); > > > } > > > > > > -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; > > > - > > > - 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) > > > { > > > @@ -1302,7 +1290,7 @@ void dpu_encoder_toggle_vblank_for_crtc(struct > > drm_encoder *drm_enc, > > > trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); > > > > > > spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); > > > - if (dpu_enc->crtc != crtc) { > > > + if (drm_enc->crtc != crtc) { > > > spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); > > > return; > > > } > > > diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > > index 781d41c..edba815 100644 > > > --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > > +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h > > > @@ -39,14 +39,6 @@ struct msm_display_info { > > > }; > > > > > > /** > > > - * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to > > > - * @encoder: encoder pointer > > > - * @crtc: crtc pointer > > > - */ > > > -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 > > > > > > -- > > With best wishes > > Dmitry
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..5629c0b 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c @@ -1254,8 +1254,8 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc, dpu_enc = to_dpu_encoder_virt(drm_enc); spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc) - dpu_crtc_vblank_callback(dpu_enc->crtc); + if (drm_enc->crtc) + dpu_crtc_vblank_callback(drm_enc->crtc); spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); atomic_inc(&phy_enc->vsync_cnt); @@ -1280,18 +1280,6 @@ static void dpu_encoder_underrun_callback(struct drm_encoder *drm_enc, DPU_ATRACE_END("encoder_underrun_callback"); } -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; - - 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) { @@ -1302,7 +1290,7 @@ void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc, trace_dpu_enc_vblank_cb(DRMID(drm_enc), enable); spin_lock_irqsave(&dpu_enc->enc_spinlock, lock_flags); - if (dpu_enc->crtc != crtc) { + if (drm_enc->crtc != crtc) { spin_unlock_irqrestore(&dpu_enc->enc_spinlock, lock_flags); return; } diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h index 781d41c..edba815 100644 --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h @@ -39,14 +39,6 @@ struct msm_display_info { }; /** - * dpu_encoder_assign_crtc - Link the encoder to the crtc it's assigned to - * @encoder: encoder pointer - * @crtc: crtc pointer - */ -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
Update crtc retrieval from dpu_enc to drm_enc, since new links get set as part of the update legacy mode set. The dpu_enc->crtc cache is no more needed, hence cleaning it as part of this change. 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 | 18 +++--------------- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.h | 8 -------- 3 files changed, 3 insertions(+), 27 deletions(-)