Re: [Freedreno] [PATCH igt v3 0/3] Initial igt tests for drm/msm ioctls
On Wed, Sep 08, 2021 at 11:02:42AM -0700, Rob Clark wrote: > On Mon, Aug 30, 2021 at 9:18 AM Rob Clark wrote: > > > > From: Rob Clark > > > > Add an initial set of tests for the gpu SUBMIT ioctl. There is > > plenty more we can add, but need to start somewhere. > > > > Rob Clark (3): > > drmtest: Add DRIVER_MSM support > > msm: Add helper library > > msm: Add submit ioctl tests > > If there are no more comments on this series, could somebody push it? Ah, I was expecting you to do it yourself. Merged now. -- Petri Latvala
Re: [Freedreno] [PATCH igt v3 0/3] Initial igt tests for drm/msm ioctls
On Mon, Aug 30, 2021 at 9:18 AM Rob Clark wrote: > > From: Rob Clark > > Add an initial set of tests for the gpu SUBMIT ioctl. There is > plenty more we can add, but need to start somewhere. > > Rob Clark (3): > drmtest: Add DRIVER_MSM support > msm: Add helper library > msm: Add submit ioctl tests If there are no more comments on this series, could somebody push it? BR, -R > .../igt-gpu-tools/igt-gpu-tools-docs.xml | 1 + > lib/drmtest.c | 3 + > lib/drmtest.h | 1 + > lib/igt_msm.c | 211 ++ > lib/igt_msm.h | 142 > lib/meson.build | 1 + > tests/meson.build | 1 + > tests/msm_submit.c| 194 > 8 files changed, 554 insertions(+) > create mode 100644 lib/igt_msm.c > create mode 100644 lib/igt_msm.h > create mode 100644 tests/msm_submit.c > > -- > 2.31.1 >
Re: [Freedreno] [PATCH v3 5/9] drm/msm: Add deadline based boost support
On Wed, Sep 8, 2021 at 10:48 AM Daniel Vetter wrote:
>
> On Fri, Sep 03, 2021 at 11:47:56AM -0700, Rob Clark wrote:
> > From: Rob Clark
> >
> > Signed-off-by: Rob Clark
>
> Why do you need a kthread_work here? Is this just to make sure you're
> running at realtime prio? Maybe a comment to that effect would be good.
Mostly because we are already using a kthread_worker for things the
GPU needs to kick off to a different context.. but I think this is
something we'd want at a realtime prio
BR,
-R
> -Daniel
>
> > ---
> > drivers/gpu/drm/msm/msm_fence.c | 76 +++
> > drivers/gpu/drm/msm/msm_fence.h | 20 +++
> > drivers/gpu/drm/msm/msm_gpu.h | 1 +
> > drivers/gpu/drm/msm/msm_gpu_devfreq.c | 20 +++
> > 4 files changed, 117 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/msm/msm_fence.c
> > b/drivers/gpu/drm/msm/msm_fence.c
> > index f2cece542c3f..67c2a96e1c85 100644
> > --- a/drivers/gpu/drm/msm/msm_fence.c
> > +++ b/drivers/gpu/drm/msm/msm_fence.c
> > @@ -8,6 +8,37 @@
> >
> > #include "msm_drv.h"
> > #include "msm_fence.h"
> > +#include "msm_gpu.h"
> > +
> > +static inline bool fence_completed(struct msm_fence_context *fctx,
> > uint32_t fence);
> > +
> > +static struct msm_gpu *fctx2gpu(struct msm_fence_context *fctx)
> > +{
> > + struct msm_drm_private *priv = fctx->dev->dev_private;
> > + return priv->gpu;
> > +}
> > +
> > +static enum hrtimer_restart deadline_timer(struct hrtimer *t)
> > +{
> > + struct msm_fence_context *fctx = container_of(t,
> > + struct msm_fence_context, deadline_timer);
> > +
> > + kthread_queue_work(fctx2gpu(fctx)->worker, &fctx->deadline_work);
> > +
> > + return HRTIMER_NORESTART;
> > +}
> > +
> > +static void deadline_work(struct kthread_work *work)
> > +{
> > + struct msm_fence_context *fctx = container_of(work,
> > + struct msm_fence_context, deadline_work);
> > +
> > + /* If deadline fence has already passed, nothing to do: */
> > + if (fence_completed(fctx, fctx->next_deadline_fence))
> > + return;
> > +
> > + msm_devfreq_boost(fctx2gpu(fctx), 2);
> > +}
> >
> >
> > struct msm_fence_context *
> > @@ -26,6 +57,13 @@ msm_fence_context_alloc(struct drm_device *dev, volatile
> > uint32_t *fenceptr,
> > fctx->fenceptr = fenceptr;
> > spin_lock_init(&fctx->spinlock);
> >
> > + hrtimer_init(&fctx->deadline_timer, CLOCK_MONOTONIC,
> > HRTIMER_MODE_ABS);
> > + fctx->deadline_timer.function = deadline_timer;
> > +
> > + kthread_init_work(&fctx->deadline_work, deadline_work);
> > +
> > + fctx->next_deadline = ktime_get();
> > +
> > return fctx;
> > }
> >
> > @@ -49,6 +87,8 @@ void msm_update_fence(struct msm_fence_context *fctx,
> > uint32_t fence)
> > {
> > spin_lock(&fctx->spinlock);
> > fctx->completed_fence = max(fence, fctx->completed_fence);
> > + if (fence_completed(fctx, fctx->next_deadline_fence))
> > + hrtimer_cancel(&fctx->deadline_timer);
> > spin_unlock(&fctx->spinlock);
> > }
> >
> > @@ -79,10 +119,46 @@ static bool msm_fence_signaled(struct dma_fence *fence)
> > return fence_completed(f->fctx, f->base.seqno);
> > }
> >
> > +static void msm_fence_set_deadline(struct dma_fence *fence, ktime_t
> > deadline)
> > +{
> > + struct msm_fence *f = to_msm_fence(fence);
> > + struct msm_fence_context *fctx = f->fctx;
> > + unsigned long flags;
> > + ktime_t now;
> > +
> > + spin_lock_irqsave(&fctx->spinlock, flags);
> > + now = ktime_get();
> > +
> > + if (ktime_after(now, fctx->next_deadline) ||
> > + ktime_before(deadline, fctx->next_deadline)) {
> > + fctx->next_deadline = deadline;
> > + fctx->next_deadline_fence =
> > + max(fctx->next_deadline_fence,
> > (uint32_t)fence->seqno);
> > +
> > + /*
> > + * Set timer to trigger boost 3ms before deadline, or
> > + * if we are already less than 3ms before the deadline
> > + * schedule boost work immediately.
> > + */
> > + deadline = ktime_sub(deadline, ms_to_ktime(3));
> > +
> > + if (ktime_after(now, deadline)) {
> > + kthread_queue_work(fctx2gpu(fctx)->worker,
> > + &fctx->deadline_work);
> > + } else {
> > + hrtimer_start(&fctx->deadline_timer, deadline,
> > + HRTIMER_MODE_ABS);
> > + }
> > + }
> > +
> > + spin_unlock_irqrestore(&fctx->spinlock, flags);
> > +}
> > +
> > static const struct dma_fence_ops msm_fence_ops = {
> > .get_driver_name = msm_fence_get_driver_name,
> > .get_timeline_name = msm_fence_get_timeline_name,
> > .signaled = msm_fence_signaled,
> > + .set_deadline = msm_fence_set_deadline,
> > };
> >
> > struct dma_fence *
> > diff --git a/d
Re: [Freedreno] [PATCH v3 5/9] drm/msm: Add deadline based boost support
On Fri, Sep 03, 2021 at 11:47:56AM -0700, Rob Clark wrote:
> From: Rob Clark
>
> Signed-off-by: Rob Clark
Why do you need a kthread_work here? Is this just to make sure you're
running at realtime prio? Maybe a comment to that effect would be good.
-Daniel
> ---
> drivers/gpu/drm/msm/msm_fence.c | 76 +++
> drivers/gpu/drm/msm/msm_fence.h | 20 +++
> drivers/gpu/drm/msm/msm_gpu.h | 1 +
> drivers/gpu/drm/msm/msm_gpu_devfreq.c | 20 +++
> 4 files changed, 117 insertions(+)
>
> diff --git a/drivers/gpu/drm/msm/msm_fence.c b/drivers/gpu/drm/msm/msm_fence.c
> index f2cece542c3f..67c2a96e1c85 100644
> --- a/drivers/gpu/drm/msm/msm_fence.c
> +++ b/drivers/gpu/drm/msm/msm_fence.c
> @@ -8,6 +8,37 @@
>
> #include "msm_drv.h"
> #include "msm_fence.h"
> +#include "msm_gpu.h"
> +
> +static inline bool fence_completed(struct msm_fence_context *fctx, uint32_t
> fence);
> +
> +static struct msm_gpu *fctx2gpu(struct msm_fence_context *fctx)
> +{
> + struct msm_drm_private *priv = fctx->dev->dev_private;
> + return priv->gpu;
> +}
> +
> +static enum hrtimer_restart deadline_timer(struct hrtimer *t)
> +{
> + struct msm_fence_context *fctx = container_of(t,
> + struct msm_fence_context, deadline_timer);
> +
> + kthread_queue_work(fctx2gpu(fctx)->worker, &fctx->deadline_work);
> +
> + return HRTIMER_NORESTART;
> +}
> +
> +static void deadline_work(struct kthread_work *work)
> +{
> + struct msm_fence_context *fctx = container_of(work,
> + struct msm_fence_context, deadline_work);
> +
> + /* If deadline fence has already passed, nothing to do: */
> + if (fence_completed(fctx, fctx->next_deadline_fence))
> + return;
> +
> + msm_devfreq_boost(fctx2gpu(fctx), 2);
> +}
>
>
> struct msm_fence_context *
> @@ -26,6 +57,13 @@ msm_fence_context_alloc(struct drm_device *dev, volatile
> uint32_t *fenceptr,
> fctx->fenceptr = fenceptr;
> spin_lock_init(&fctx->spinlock);
>
> + hrtimer_init(&fctx->deadline_timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
> + fctx->deadline_timer.function = deadline_timer;
> +
> + kthread_init_work(&fctx->deadline_work, deadline_work);
> +
> + fctx->next_deadline = ktime_get();
> +
> return fctx;
> }
>
> @@ -49,6 +87,8 @@ void msm_update_fence(struct msm_fence_context *fctx,
> uint32_t fence)
> {
> spin_lock(&fctx->spinlock);
> fctx->completed_fence = max(fence, fctx->completed_fence);
> + if (fence_completed(fctx, fctx->next_deadline_fence))
> + hrtimer_cancel(&fctx->deadline_timer);
> spin_unlock(&fctx->spinlock);
> }
>
> @@ -79,10 +119,46 @@ static bool msm_fence_signaled(struct dma_fence *fence)
> return fence_completed(f->fctx, f->base.seqno);
> }
>
> +static void msm_fence_set_deadline(struct dma_fence *fence, ktime_t deadline)
> +{
> + struct msm_fence *f = to_msm_fence(fence);
> + struct msm_fence_context *fctx = f->fctx;
> + unsigned long flags;
> + ktime_t now;
> +
> + spin_lock_irqsave(&fctx->spinlock, flags);
> + now = ktime_get();
> +
> + if (ktime_after(now, fctx->next_deadline) ||
> + ktime_before(deadline, fctx->next_deadline)) {
> + fctx->next_deadline = deadline;
> + fctx->next_deadline_fence =
> + max(fctx->next_deadline_fence, (uint32_t)fence->seqno);
> +
> + /*
> + * Set timer to trigger boost 3ms before deadline, or
> + * if we are already less than 3ms before the deadline
> + * schedule boost work immediately.
> + */
> + deadline = ktime_sub(deadline, ms_to_ktime(3));
> +
> + if (ktime_after(now, deadline)) {
> + kthread_queue_work(fctx2gpu(fctx)->worker,
> + &fctx->deadline_work);
> + } else {
> + hrtimer_start(&fctx->deadline_timer, deadline,
> + HRTIMER_MODE_ABS);
> + }
> + }
> +
> + spin_unlock_irqrestore(&fctx->spinlock, flags);
> +}
> +
> static const struct dma_fence_ops msm_fence_ops = {
> .get_driver_name = msm_fence_get_driver_name,
> .get_timeline_name = msm_fence_get_timeline_name,
> .signaled = msm_fence_signaled,
> + .set_deadline = msm_fence_set_deadline,
> };
>
> struct dma_fence *
> diff --git a/drivers/gpu/drm/msm/msm_fence.h b/drivers/gpu/drm/msm/msm_fence.h
> index 4783db528bcc..d34e853c555a 100644
> --- a/drivers/gpu/drm/msm/msm_fence.h
> +++ b/drivers/gpu/drm/msm/msm_fence.h
> @@ -50,6 +50,26 @@ struct msm_fence_context {
> volatile uint32_t *fenceptr;
>
> spinlock_t spinlock;
> +
> + /*
> + * TODO this doesn't really deal with multiple deadlines, like
> + * if userspace got multiple frames ahead.. OTOH atomic updates
> + * don't queue, so maybe that is
Re: [Freedreno] [PATCH 2/3] drm/msm/dpu1: Add MSM8998 to hw catalog
On Wed, Sep 8, 2021 at 2:26 AM Dmitry Baryshkov wrote: > > Hi, > > On Tue, 7 Sept 2021 at 22:13, Jeffrey Hugo wrote: > > > > On Wed, Sep 1, 2021 at 12:11 PM AngeloGioacchino Del Regno > > wrote: > > > > > > Bringup functionality for MSM8998 in the DPU, driver which is mostly > > > the same as SDM845 (just a few variations). > > > > > > Signed-off-by: AngeloGioacchino Del Regno > > > > > > > I don't seem to see a cover letter for this series. > > > > Eh, there are a fair number of differences between the MDSS versions > > for 8998 and 845. > > > > Probably a bigger question, why extend the DPU driver for 8998, when > > the MDP5 driver already supports it[1]? The MDP/DPU split is pretty > > dumb, but I don't see a valid reason for both drivers supporting the > > same target/display revision. IMO, if you want this support in DPU, > > remove it from MDP5. > > > > [1] > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.14&id=d6c7b2284b14c66a268a448a7a8d54f585d38785 > > I don't think that we should enforce such requirements. Having support > both in MDP5 and DPU would allow one to compare those two drivers, > performance, features, etc. > It might be that all MDP5-supported hardware would be also supported > by DPU, thus allowing us to remove the former driver. But until that > time I'd suggest leaving support in place. Well, then you have a host of problems to solve. Lets ignore the code duplication for a minute and assume we've gone with this grand experiment. Two drivers enter, one leaves the victor. How are the clients supposed to pick which driver to use in the mean time? We already have one DT binding for 8998 (which the MDP5 driver services). This series proposes a second. If we go forward with what you propose, we'll have two bindings for the same hardware, which IMO doesn't make sense in the context of DT, and the reason for that is to select which driver is "better". Driver selection is not supposed to be tied to DT like this. So, some boards think MDP5 is better, and some boards think DPU is better. At some point, we decide one of the drivers is the clear winner (lets assume DPU). Then what happens to the existing DTs that were using the MDP5 description? Are they really compatible with DPU? >From a DT perspective, there should be one description, but then how do you pick which driver to load? Both can't bind on the single description, and while you could argue that the users should build one driver or the other, but not both (thus picking which one at build time), that doesn't work for distros that want to build both drivers so that they can support all platforms with a single build (per arch). >From where I sit, your position starts with a good idea, but isn't fully thought out and leads to problems. If there is some reason why DPU is better for 8998, please enumerate it. Does DPU support some config that MDP5 doesn't, which is valuable to you? I'm ok with ripping out the MDP5 support, the reason I didn't go with DPU was that the DPU driver was clearly written only for 845 at the time, and needed significant rework to "downgrade" to an earlier hardware. However, the "reason" DPU exists separate from MDP5 is the claim that the MDP hardware underwent a significant rearchitecture, and thus it was too cumbersome to extend MDP5. While I disagree with the premise, that "rearch" started with 8998.
Re: [Freedreno] [PATCH] drm/msm: Disable frequency clamping on a630
On 08/09/2021 03:21, Bjorn Andersson wrote:
On Mon 09 Aug 10:26 PDT 2021, Akhil P Oommen wrote:
On 8/9/2021 9:48 PM, Caleb Connolly wrote:
On 09/08/2021 17:12, Rob Clark wrote:
On Mon, Aug 9, 2021 at 7:52 AM Akhil P Oommen
wrote:
[..]
I am a bit confused. We don't define a power domain for gpu in dt,
correct? Then what exactly set_opp do here? Do you think this usleep is
what is helping here somehow to mask the issue?
The power domains (for cx and gx) are defined in the GMU DT, the OPPs in
the GPU DT. For the sake of simplicity I'll refer to the lowest
frequency (25700) and OPP level (RPMH_REGULATOR_LEVEL_LOW_SVS) as
the "min" state, and the highest frequency (71000) and OPP level
(RPMH_REGULATOR_LEVEL_TURBO_L1) as the "max" state. These are defined in
sdm845.dtsi under the gpu node.
The new devfreq behaviour unmasks what I think is a driver bug, it
inadvertently puts much more strain on the GPU regulators than they
usually get. With the new behaviour the GPU jumps from it's min state to
the max state and back again extremely rapidly under workloads as small
as refreshing UI. Where previously the GPU would rarely if ever go above
342MHz when interacting with the device, it now jumps between min and
max many times per second.
If my understanding is correct, the current implementation of the GMU
set freq is the following:
- Get OPP for frequency to set
- Push the frequency to the GMU - immediately updating the core clock
- Call dev_pm_opp_set_opp() which triggers a notify chain, this winds
up somewhere in power management code and causes the gx regulator level
to be updated
Nope. dev_pm_opp_set_opp() sets the bandwidth for gpu and nothing else. We
were using a different api earlier which got deprecated -
dev_pm_opp_set_bw().
On the Lenovo Yoga C630 this is reproduced by starting alacritty and if
I'm lucky I managed to hit a few keys before it crashes, so I spent a
few hours looking into this as well...
As you say, the dev_pm_opp_set_opp() will only cast a interconnect vote.
The opp-level is just there for show and isn't used by anything, at
least not on 845.
Further more, I'm missing something in my tree, so the interconnect
doesn't hit sync_state, and as such we're not actually scaling the
buses. So the problem is not that Linux doesn't turn on the buses in
time.
So I suspect that the "AHB bus error" isn't saying that we turned off
the bus, but rather that the GPU becomes unstable or something of that
sort.
Lastly, I reverted 9bc95570175a ("drm/msm: Devfreq tuning") and ran
Aquarium for 20 minutes without a problem. I then switched the gpu
devfreq governor to "userspace" and ran the following:
while true; do
echo 25700 > /sys/class/devfreq/500.gpu/userspace/set_freq
echo 71000 > /sys/class/devfreq/500.gpu/userspace/set_freq
done
It took 19 iterations of this loop to crash the GPU.
So the problem doesn't seem to be Rob's change, it's just that prior to
it the chance to hitting it is way lower. Question is still what it is
that we're triggering.
Do the opp-levels in DTS represent how the hardware behaves? If so then it does
just
appear to be that whatever is responsible for scaling the GX rail voltage
has no time limits and will attempt to switch the regulator between min/max
voltage as often as we tell it to which is probably not something the hardware
expected.
Regards,
Bjorn
--
Kind Regards,
Caleb (they/them)
Re: [Freedreno] [PATCH v2 1/3] dt-bindings: msm: dsi: Add MSM8953 dsi phy
On Fri, Sep 03, 2021 at 10:38:42PM +0530, Sireesh Kodali wrote: > SoCs based on the MSM8953 platform use the 14nm DSI PHY driver > > Signed-off-by: Sireesh Kodali > --- > Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml > b/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml > index 72a00cce0147..d2cb19cf71d6 100644 > --- a/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml > +++ b/Documentation/devicetree/bindings/display/msm/dsi-phy-14nm.yaml > @@ -17,6 +17,8 @@ properties: > oneOf: >- const: qcom,dsi-phy-14nm >- const: qcom,dsi-phy-14nm-660 > + - const: qcom,dsi-phy-14nm-8953 > + This is going to conflict with v5.15-rc1, so you'll need to resend it. > >reg: > items: > -- > 2.33.0 > >
Re: [Freedreno] [PATCH 2/3] drm/msm/dpu1: Add MSM8998 to hw catalog
Hi, On Tue, 7 Sept 2021 at 22:13, Jeffrey Hugo wrote: > > On Wed, Sep 1, 2021 at 12:11 PM AngeloGioacchino Del Regno > wrote: > > > > Bringup functionality for MSM8998 in the DPU, driver which is mostly > > the same as SDM845 (just a few variations). > > > > Signed-off-by: AngeloGioacchino Del Regno > > > > I don't seem to see a cover letter for this series. > > Eh, there are a fair number of differences between the MDSS versions > for 8998 and 845. > > Probably a bigger question, why extend the DPU driver for 8998, when > the MDP5 driver already supports it[1]? The MDP/DPU split is pretty > dumb, but I don't see a valid reason for both drivers supporting the > same target/display revision. IMO, if you want this support in DPU, > remove it from MDP5. > > [1] > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.14&id=d6c7b2284b14c66a268a448a7a8d54f585d38785 I don't think that we should enforce such requirements. Having support both in MDP5 and DPU would allow one to compare those two drivers, performance, features, etc. It might be that all MDP5-supported hardware would be also supported by DPU, thus allowing us to remove the former driver. But until that time I'd suggest leaving support in place. -- With best wishes Dmitry
