From: Brian Masney <masneyb@onstation.org>
To: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
Cc: Rob Clark <robdclark@chromium.org>,
Rob Clark <robdclark@gmail.com>,
freedreno <freedreno@lists.freedesktop.org>,
lkml <linux-kernel@vger.kernel.org>,
"open list:DRM PANEL DRIVERS" <dri-devel@lists.freedesktop.org>,
MSM <linux-arm-msm@vger.kernel.org>, Sean Paul <sean@poorly.run>
Subject: Re: [PATCH] drm/msm/mdp5: enable autocommit
Date: Wed, 13 Nov 2019 06:23:34 -0500 [thread overview]
Message-ID: <20191113112334.GA18702@onstation.org> (raw)
In-Reply-To: <CAOCk7NosRhRp3vZxg2Nx8106PQ0ryo5b68cUv605XUzCm6gYPA@mail.gmail.com>
On Tue, Nov 12, 2019 at 08:38:27AM -0700, Jeffrey Hugo wrote:
> On Tue, Nov 12, 2019 at 3:49 AM Brian Masney <masneyb@onstation.org> wrote:
> >
> > Since the introduction of commit 2d99ced787e3 ("drm/msm: async commit
> > support"), command-mode panels began throwing the following errors:
> >
> > msm fd900000.mdss: pp done time out, lm=0
> >
> > Let's fix this by enabling the autorefresh feature that's available in
> > the MDP starting at version 1.0. This will cause the MDP to
> > automatically send a frame to the panel every time the panel invokes
> > the TE signal, which will trigger the PP_DONE IRQ. This requires not
> > sending a START signal for command-mode panels.
> >
> > This fixes the error and gives us a counter for command-mode panels that
> > we can use to implement async commit support for the MDP5 in a follow up
> > patch.
> >
> > Signed-off-by: Brian Masney <masneyb@onstation.org>
> > Suggested-by: Jeffrey Hugo <jeffrey.l.hugo@gmail.com>
> > ---
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c | 15 ++++++++++++++-
> > drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c | 9 +--------
> > 2 files changed, 15 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > index 05cc04f729d6..539348cb6331 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_crtc.c
> > @@ -456,6 +456,7 @@ static void mdp5_crtc_atomic_enable(struct drm_crtc *crtc,
> > {
> > struct mdp5_crtc *mdp5_crtc = to_mdp5_crtc(crtc);
> > struct mdp5_crtc_state *mdp5_cstate = to_mdp5_crtc_state(crtc->state);
> > + struct mdp5_pipeline *pipeline = &mdp5_cstate->pipeline;
> > struct mdp5_kms *mdp5_kms = get_kms(crtc);
> > struct device *dev = &mdp5_kms->pdev->dev;
> >
> > @@ -493,9 +494,21 @@ static void mdp5_crtc_atomic_enable(struct drm_crtc *crtc,
> >
> > mdp_irq_register(&mdp5_kms->base, &mdp5_crtc->err);
> >
> > - if (mdp5_cstate->cmd_mode)
> > + if (mdp5_cstate->cmd_mode) {
> > mdp_irq_register(&mdp5_kms->base, &mdp5_crtc->pp_done);
> >
> > + /*
> > + * Enable autorefresh so we get regular ping/pong IRQs.
> > + * - Bit 31 is the enable bit
> > + * - Bits 0-15 represent the frame count, specifically how many
> > + * TE events before the MDP sends a frame.
> > + */
> > + mdp5_write(mdp5_kms,
> > + REG_MDP5_PP_AUTOREFRESH_CONFIG(pipeline->mixer->pp),
> > + BIT(31) | BIT(0));
> > + crtc_flush_all(crtc);
> > + }
> > +
> > mdp5_crtc->enabled = true;
> > }
> >
> > diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> > index 030279d7b64b..aee295abada3 100644
> > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_ctl.c
> > @@ -187,14 +187,7 @@ static bool start_signal_needed(struct mdp5_ctl *ctl,
> > if (!ctl->encoder_enabled)
> > return false;
> >
> > - switch (intf->type) {
> > - case INTF_WB:
> > - return true;
> > - case INTF_DSI:
> > - return intf->mode == MDP5_INTF_DSI_MODE_COMMAND;
> > - default:
> > - return false;
> > - }
> > + return intf->type == INTF_WB;
> > }
>
> I don't think this fully works.
>
> The whole "flush" thing exists because the configuration is double
> buffered. You write to the flush register to tell the hardware to
> pickup the new configuration, but it doesn't do that automatically.
> It only picks up the new config on the next "vsync". When you have a
> video mode panel, you have the timing engine running, which drives
> that. With a command mode panel, you have either the start signal, or
> the auto refresh to do the same, but you have a bit of a chicken and
> egg situation where if you are programming the hardware from scratch,
> autorefresh isn't already enabled to then pickup the config to enable
> autorefresh. In this case, you'll need a single start to kick
> everything off. However, if say the bootloader already configured
> things and has autorefresh running, then you need to not do that start
> because you'll overload the DSI like you saw.
As part of my testing for this work, I added a log statement to
mdp5_crtc_pp_done_irq() and it shows that a PP_IRQ comes in consistently
every ~0.0166 seconds, which is about 60 HZ. Without this change, plus
the 3 commits I mentioned in an earlier email related to the async
commit support, the PP IRQs come in at a variety of times: between every
~0.0140 and ~0.2224 seconds. That's why I assumed that this was working.
If I call send_start_signal() inside mdp5_crtc_atomic_enable(), then the
display does not work properly.
> Nothing is simple, is it? :)
Ain't that the truth...
Brian
next prev parent reply other threads:[~2019-11-13 11:23 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-11-12 10:48 [PATCH] drm/msm/mdp5: enable autocommit Brian Masney
2019-11-12 15:38 ` Jeffrey Hugo
2019-11-13 11:23 ` Brian Masney [this message]
2019-12-03 1:40 ` Brian Masney
2019-12-03 14:18 ` Jeffrey Hugo
2019-12-13 2:40 ` Brian Masney
2019-12-16 18:57 ` Jeffrey Hugo
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=20191113112334.GA18702@onstation.org \
--to=masneyb@onstation.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=freedreno@lists.freedesktop.org \
--cc=jeffrey.l.hugo@gmail.com \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=robdclark@chromium.org \
--cc=robdclark@gmail.com \
--cc=sean@poorly.run \
/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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).