* [PATCH] drm/msm: dpu: Don't reset dpu_enc->cur_master on .disable()
@ 2018-09-17 20:49 Sean Paul
[not found] ` <20180917204959.205802-1-sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>
0 siblings, 1 reply; 6+ messages in thread
From: Sean Paul @ 2018-09-17 20:49 UTC (permalink / raw)
To: freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
linux-arm-msm-u79uwXL29TY76Z2rM5mHXA
Cc: jsanka-sgV2jX0FEOL9JmXXK+q4OQ, Jordan Crouse, Sean Paul,
Rob Clark, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ
From: Sean Paul <seanpaul@chromium.org>
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.
Signed-off-by: Sean Paul <seanpaul@chromium.org>
---
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;
--
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
Freedreno mailing list
Freedreno@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/freedreno
^ permalink raw reply related [flat|nested] 6+ messages in thread[parent not found: <20180917204959.205802-1-sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>]
* Re: [PATCH] drm/msm: dpu: Don't reset dpu_enc->cur_master on .disable() [not found] ` <20180917204959.205802-1-sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org> @ 2018-10-02 15:23 ` Bruce Wang 2018-10-02 18:09 ` Jeykumar Sankaran 1 sibling, 0 replies; 6+ messages in thread From: Bruce Wang @ 2018-10-02 15:23 UTC (permalink / raw) To: sean-p7yTbzM4H96eqtR555YLDQ Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, jcrouse-sgV2jX0FEOL9JmXXK+q4OQ, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, robdclark-Re5JQEeQqe8AvxtiuMwx3w, seanpaul-F7+t8E8rja9g9hUCZPvPmw, jsanka-sgV2jX0FEOL9JmXXK+q4OQ, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On Mon, Sep 17, 2018 at 4:50 PM Sean Paul <sean@poorly.run> wrote: > > From: Sean Paul <seanpaul@chromium.org> > > 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. > > Signed-off-by: Sean Paul <seanpaul@chromium.org> Tested-by: Bruce Wang <bzwang@chromium.org> > --- > 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; > -- > Sean Paul, Software Engineer, Google / Chromium OS > _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/msm: dpu: Don't reset dpu_enc->cur_master on .disable() [not found] ` <20180917204959.205802-1-sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org> 2018-10-02 15:23 ` Bruce Wang @ 2018-10-02 18:09 ` Jeykumar Sankaran [not found] ` <0238e2d56bb56f92a1a4ec9d861496bd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 1 sibling, 1 reply; 6+ messages in thread From: Jeykumar Sankaran @ 2018-10-02 18:09 UTC (permalink / raw) To: Sean Paul Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Jordan Crouse, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, Rob Clark, Sean Paul, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 2018-09-17 13:49, Sean Paul wrote: > From: Sean Paul <seanpaul@chromium.org> > > 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? Can we clean up the usage of cur_master if possible in the dpms path? Thanks, Jeykumar S. > Signed-off-by: Sean Paul <seanpaul@chromium.org> > --- > 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 _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <0238e2d56bb56f92a1a4ec9d861496bd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]
* Re: [PATCH] drm/msm: dpu: Don't reset dpu_enc->cur_master on .disable() [not found] ` <0238e2d56bb56f92a1a4ec9d861496bd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> @ 2018-10-02 19:17 ` Sean Paul 2018-10-02 19:59 ` Jeykumar Sankaran 0 siblings, 1 reply; 6+ messages in thread From: Sean Paul @ 2018-10-02 19:17 UTC (permalink / raw) To: Jeykumar Sankaran Cc: Sean Paul, Jordan Crouse, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, Rob Clark, Sean Paul, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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@chromium.org> > > > > 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@chromium.org> > > --- > > 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 _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] drm/msm: dpu: Don't reset dpu_enc->cur_master on .disable() 2018-10-02 19:17 ` Sean Paul @ 2018-10-02 19:59 ` Jeykumar Sankaran [not found] ` <9a68ad97a1db25d7a7e37df0e43ad5f0-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 0 siblings, 1 reply; 6+ messages in thread From: Jeykumar Sankaran @ 2018-10-02 19:59 UTC (permalink / raw) To: Sean Paul Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, Jordan Crouse, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, Rob Clark, Sean Paul, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On 2018-10-02 12:17, Sean Paul wrote: > 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@chromium.org> >> > >> > 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 > Got it! Misread the commit text. Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org> >> Can we clean up the usage of cur_master if possible in the dpms path? >> >> Thanks, >> Jeykumar S. >> >> >> > Signed-off-by: Sean Paul <seanpaul@chromium.org> >> > --- >> > 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 -- Jeykumar S _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 6+ messages in thread
[parent not found: <9a68ad97a1db25d7a7e37df0e43ad5f0-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>]
* Re: [PATCH] drm/msm: dpu: Don't reset dpu_enc->cur_master on .disable() [not found] ` <9a68ad97a1db25d7a7e37df0e43ad5f0-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> @ 2018-10-02 20:22 ` Sean Paul 0 siblings, 0 replies; 6+ messages in thread From: Sean Paul @ 2018-10-02 20:22 UTC (permalink / raw) To: Jeykumar Sankaran Cc: Sean Paul, Jordan Crouse, abhinavk-sgV2jX0FEOL9JmXXK+q4OQ, Rob Clark, Sean Paul, linux-arm-msm-u79uwXL29TY76Z2rM5mHXA, freedreno-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW On Tue, Oct 02, 2018 at 12:59:18PM -0700, Jeykumar Sankaran wrote: > On 2018-10-02 12:17, Sean Paul wrote: > > 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@chromium.org> > > > > > > > > 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 > > > Got it! Misread the commit text. > > Reviewed-by: Jeykumar Sankaran <jsanka@codeaurora.org> Awesome, thanks for your review. I've pushed to dpu-staging. 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@chromium.org> > > > > --- > > > > 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 > > -- > Jeykumar S -- Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-10-02 20:22 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-09-17 20:49 [PATCH] drm/msm: dpu: Don't reset dpu_enc->cur_master on .disable() Sean Paul
[not found] ` <20180917204959.205802-1-sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>
2018-10-02 15:23 ` Bruce Wang
2018-10-02 18:09 ` Jeykumar Sankaran
[not found] ` <0238e2d56bb56f92a1a4ec9d861496bd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2018-10-02 19:17 ` Sean Paul
2018-10-02 19:59 ` Jeykumar Sankaran
[not found] ` <9a68ad97a1db25d7a7e37df0e43ad5f0-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
2018-10-02 20:22 ` Sean Paul
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.