All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stephen Boyd <swboyd@chromium.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Kuogee Hsieh <quic_khsieh@quicinc.com>,
	agross@kernel.org, airlied@linux.ie, bjorn.andersson@linaro.org,
	daniel@ffwll.ch, dianders@chromium.org, robdclark@gmail.com,
	sean@poorly.run, vkoul@kernel.org
Cc: quic_abhinavk@quicinc.com, quic_aravindh@quicinc.com,
	quic_sbillaka@quicinc.com, freedreno@lists.freedesktop.org,
	dri-devel@lists.freedesktop.org, linux-arm-msm@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH v5] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl()
Date: Wed, 11 May 2022 18:01:26 -0700	[thread overview]
Message-ID: <CAE-0n53zHrczQAOWM5f_tafUVJxg7JU2NuC4sGcBZddwKr-dGA@mail.gmail.com> (raw)
In-Reply-To: <e5c16697-5919-bafa-4ec5-294c94d7a740@linaro.org>

Quoting Dmitry Baryshkov (2022-05-11 17:41:50)
> On 12/05/2022 03:02, Kuogee Hsieh wrote:
> > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > index af7a80c..f3e333e 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > @@ -1389,8 +1389,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
> >
> >       dp_catalog_ctrl_reset(ctrl->catalog);
> >
> > -     if (enable)
> > -             dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
> > +     /*
> > +      * all dp controller programmable registers will not
> > +      * be reset to default value after DP_SW_RESET
> > +      * therefore interrupt mask bits have to be updated
> > +      * to enable/disable interrupts
> > +      */
> > +     dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
> >   }
> >
> >   void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index c388323..79439b8 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -98,6 +98,8 @@ struct dp_display_private {
> >       struct dp_ctrl    *ctrl;
> >       struct dp_debug   *debug;
> >
> > +     atomic_t suspended;
>
> I think it'd be better to protect it with event_lock rather than using
> atomics.

Agreed. I think the concern is that the event queue will have "stuff" in
it. If the event queue was all a threaded irq we could simply call
synchronize_irq() after disabling the irq bit in the DP hardware and
then we would know it is safe to power down the DP logic. Unfortunately
the event queue is a kthread so we can't do that and we have to rewrite
synchronize_irq() by checking that the event queue is empty and waiting
for it to empty out otherwise. It's not safe enough to simply do the
power operations underneath the event_lock because there's a queue in
the kthread that might be waiting to grab the event_lock to process.

WARNING: multiple messages have this Message-ID (diff)
From: Stephen Boyd <swboyd@chromium.org>
To: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>,
	Kuogee Hsieh <quic_khsieh@quicinc.com>,
	 agross@kernel.org, airlied@linux.ie, bjorn.andersson@linaro.org,
	 daniel@ffwll.ch, dianders@chromium.org, robdclark@gmail.com,
	sean@poorly.run,  vkoul@kernel.org
Cc: quic_sbillaka@quicinc.com, linux-arm-msm@vger.kernel.org,
	quic_abhinavk@quicinc.com, dri-devel@lists.freedesktop.org,
	linux-kernel@vger.kernel.org, quic_aravindh@quicinc.com,
	freedreno@lists.freedesktop.org
Subject: Re: [PATCH v5] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl()
Date: Wed, 11 May 2022 18:01:26 -0700	[thread overview]
Message-ID: <CAE-0n53zHrczQAOWM5f_tafUVJxg7JU2NuC4sGcBZddwKr-dGA@mail.gmail.com> (raw)
In-Reply-To: <e5c16697-5919-bafa-4ec5-294c94d7a740@linaro.org>

Quoting Dmitry Baryshkov (2022-05-11 17:41:50)
> On 12/05/2022 03:02, Kuogee Hsieh wrote:
> > diff --git a/drivers/gpu/drm/msm/dp/dp_ctrl.c b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > index af7a80c..f3e333e 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_ctrl.c
> > @@ -1389,8 +1389,13 @@ void dp_ctrl_reset_irq_ctrl(struct dp_ctrl *dp_ctrl, bool enable)
> >
> >       dp_catalog_ctrl_reset(ctrl->catalog);
> >
> > -     if (enable)
> > -             dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
> > +     /*
> > +      * all dp controller programmable registers will not
> > +      * be reset to default value after DP_SW_RESET
> > +      * therefore interrupt mask bits have to be updated
> > +      * to enable/disable interrupts
> > +      */
> > +     dp_catalog_ctrl_enable_irq(ctrl->catalog, enable);
> >   }
> >
> >   void dp_ctrl_phy_init(struct dp_ctrl *dp_ctrl)
> > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > index c388323..79439b8 100644
> > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > @@ -98,6 +98,8 @@ struct dp_display_private {
> >       struct dp_ctrl    *ctrl;
> >       struct dp_debug   *debug;
> >
> > +     atomic_t suspended;
>
> I think it'd be better to protect it with event_lock rather than using
> atomics.

Agreed. I think the concern is that the event queue will have "stuff" in
it. If the event queue was all a threaded irq we could simply call
synchronize_irq() after disabling the irq bit in the DP hardware and
then we would know it is safe to power down the DP logic. Unfortunately
the event queue is a kthread so we can't do that and we have to rewrite
synchronize_irq() by checking that the event queue is empty and waiting
for it to empty out otherwise. It's not safe enough to simply do the
power operations underneath the event_lock because there's a queue in
the kthread that might be waiting to grab the event_lock to process.

  reply	other threads:[~2022-05-12  1:01 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-12  0:02 [PATCH v5] drm/msm/dp: Always clear mask bits to disable interrupts at dp_ctrl_reset_irq_ctrl() Kuogee Hsieh
2022-05-12  0:02 ` Kuogee Hsieh
2022-05-12  0:41 ` Dmitry Baryshkov
2022-05-12  0:41   ` Dmitry Baryshkov
2022-05-12  1:01   ` Stephen Boyd [this message]
2022-05-12  1:01     ` Stephen Boyd
2022-05-12  1:03     ` Dmitry Baryshkov
2022-05-12  1:03       ` Dmitry Baryshkov
2022-05-12 16:02       ` Kuogee Hsieh
2022-05-12 16:02         ` Kuogee Hsieh
2022-05-12 19:33       ` Kuogee Hsieh
2022-05-12 19:33         ` Kuogee Hsieh

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=CAE-0n53zHrczQAOWM5f_tafUVJxg7JU2NuC4sGcBZddwKr-dGA@mail.gmail.com \
    --to=swboyd@chromium.org \
    --cc=agross@kernel.org \
    --cc=airlied@linux.ie \
    --cc=bjorn.andersson@linaro.org \
    --cc=daniel@ffwll.ch \
    --cc=dianders@chromium.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_khsieh@quicinc.com \
    --cc=quic_sbillaka@quicinc.com \
    --cc=robdclark@gmail.com \
    --cc=sean@poorly.run \
    --cc=vkoul@kernel.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: link
Be 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.