Re: [Freedreno] [PATCH 0/3] allow DRM drivers to limit creation of blobs
On Sat, Nov 9, 2019 at 1:01 AM wrote: > > On 2019-11-08 03:34, Daniel Vetter wrote: > > On Thu, Nov 07, 2019 at 02:39:11PM -0500, Steve Cohen wrote: > >> Fuzzers used in Android compliance testing repeatedly call the > >> create blob IOCTL which eventually exhausts the system memory. > >> This series adds a hook which allows drivers to impose their own > >> limitations on the size and/or number of blobs created. > > > > Pretty sure this isn't just a problem for msm/dpu alone, why this very > > limited approach? > > > I'm not familiar enough with the blob requirements for other > vendor's drivers to impose any restrictions on them. The idea > was to provide the hook for vendors to implement their own > checks. Support for msm/mdp* drivers will be added in v2 if this > approach is acceptable. Yeah, but I don't think different limits (and then maybe different tunables for these different limits) on drivers isn't a great idea. Plus I thought this kind of stuff was supposed to be catched by the kernel memory cgroup controller. Does that not work? The blob stuff is maybe a bit too direct way to allocate kernel memory, but there's many others I've thought. This all just feels a lot like busywork to check an android compliance checkbox, without really digging into the underlying problem and fixing it for real. One approach that would work I think would be: - Extended the blob property type to have min/max size (we might need a range for some), set it for all current blob properties. To do that we'd need to create a new property creation function for blobs, which takes those limits. There's not a hole lot of them, so should be doable. - In the create blob function we walk all property (or book-keep that somewhere) and check the blob isn't too big. - We also validate the size when setting the property, at least some of that code from various places. I think this would actually improve the situation here, instead of whack-a-mole. The cheaper cope-out would be if we simply limit the total property size to something big enough, but not unlimited, like 16MB or something like that. > > Also, why are your fuzzers not also allocating enormous amounts of gem > > buffers, which will also exhaust memory eventually? > > Excellent question... This will likely come in a follow-up series. Would be good to know that, so we can solve this for real, not just a one-off for the compliance checkbox. Also really wondering why cgroups doesn't catch this. -Daniel > > > -Daniel > > > >> > >> Steve Cohen (3): > >> drm: add driver hook for create blob limitations > >> drm/msm: add support for createblob_check driver hook > >> drm/msm/dpu: check blob limitations during create blob ioctl > >> > >> drivers/gpu/drm/drm_property.c | 7 +++ > >> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 14 ++ > >> drivers/gpu/drm/msm/msm_drv.c | 25 > >> + > >> drivers/gpu/drm/msm/msm_kms.h | 1 + > >> include/drm/drm_drv.h | 9 + > >> 5 files changed, 56 insertions(+) > >> > >> -- > >> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora > >> Forum, > >> a Linux Foundation Collaborative Project > >> > >> ___ > >> dri-devel mailing list > >> dri-de...@lists.freedesktop.org > >> https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Freedreno mailing list Freedreno@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] drm/msm: 'pp done time out' errors after async commit changes
On Sun, Nov 10, 2019 at 6:53 AM Brian Masney wrote:
>
> On Fri, Nov 08, 2019 at 07:56:25AM -0700, Jeffrey Hugo wrote:
> > On Thu, Nov 7, 2019 at 7:03 PM Rob Clark wrote:
> > >
> > > On Thu, Nov 7, 2019 at 9:40 AM Jeffrey Hugo
> > > wrote:
> > > >
> > > > On Thu, Nov 7, 2019 at 9:17 AM Rob Clark wrote:
> > > > >
> > > > > On Thu, Nov 7, 2019 at 3:10 AM Brian Masney
> > > > > wrote:
> > > > > >
> > > > > > On Wed, Nov 06, 2019 at 08:58:59AM -0800, Rob Clark wrote:
> > > > > > > On Wed, Nov 6, 2019 at 8:47 AM Jeffrey Hugo
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Wed, Nov 6, 2019 at 9:30 AM Rob Clark
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > On Wed, Nov 6, 2019 at 1:13 AM Brian Masney
> > > > > > > > > wrote:
> > > > > > > > > >
> > > > > > > > > > On Tue, Nov 05, 2019 at 08:23:27AM -0800, Rob Clark wrote:
> > > > > > > > > > > On Tue, Nov 5, 2019 at 2:08 AM Brian Masney
> > > > > > > > > > > wrote:
> > > > > > > > > > > > The 'pp done time out' errors go away if I revert the
> > > > > > > > > > > > following three
> > > > > > > > > > > > commits:
> > > > > > > > > > > >
> > > > > > > > > > > > cd6d923167b1 ("drm/msm/dpu: async commit support")
> > > > > > > > > > > > d934a712c5e6 ("drm/msm: add atomic traces")
> > > > > > > > > > > > 2d99ced787e3 ("drm/msm: async commit support")
> > > > > > > > > > > >
> > > > > > > > > > > > I reverted the first one to fix a compiler error, and
> > > > > > > > > > > > the second one so
> > > > > > > > > > > > that the last patch can be reverted without any merge
> > > > > > > > > > > > conflicts.
> > > > > > > > > > > >
> > > > > > > > > > > > I see that crtc_flush() calls mdp5_ctl_commit(). I
> > > > > > > > > > > > tried to use
> > > > > > > > > > > > crtc_flush_all() in mdp5_flush_commit() and the
> > > > > > > > > > > > contents of the frame
> > > > > > > > > > > > buffer dance around the screen like its out of sync. I
> > > > > > > > > > > > renamed
> > > > > > > > > > > > crtc_flush_all() to mdp5_crtc_flush_all() and removed
> > > > > > > > > > > > the static
> > > > > > > > > > > > declaration. Here's the relevant part of what I tried:
> > > > > > > > > > > >
> > > > > > > > > > > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > > > > > > > > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > > > > > > > > > @@ -171,7 +171,15 @@ static void
> > > > > > > > > > > > mdp5_prepare_commit(struct msm_kms *kms, struct
> > > > > > > > > > > > drm_atomic_state *st
> > > > > > > > > > > >
> > > > > > > > > > > > static void mdp5_flush_commit(struct msm_kms *kms,
> > > > > > > > > > > > unsigned crtc_mask)
> > > > > > > > > > > > {
> > > > > > > > > > > > - /* TODO */
> > > > > > > > > > > > + struct mdp5_kms *mdp5_kms =
> > > > > > > > > > > > to_mdp5_kms(to_mdp_kms(kms));
> > > > > > > > > > > > + struct drm_crtc *crtc;
> > > > > > > > > > > > +
> > > > > > > > > > > > + for_each_crtc_mask(mdp5_kms->dev, crtc,
> > > > > > > > > > > > crtc_mask) {
> > > > > > > > > > > > + if (!crtc->state->active)
> > > > > > > > > > > > + continue;
> > > > > > > > > > > > +
> > > > > > > > > > > > + mdp5_crtc_flush_all(crtc);
> > > > > > > > > > > > + }
> > > > > > > > > > > > }
> > > > > > > > > > > >
> > > > > > > > > > > > Any tips would be appreciated.
> > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > I think this is along the lines of what we need to enable
> > > > > > > > > > > async commit
> > > > > > > > > > > for mdp5 (but also removing the flush from the
> > > > > > > > > > > atomic-commit path)..
> > > > > > > > > > > the principle behind the async commit is to do all the
> > > > > > > > > > > atomic state
> > > > > > > > > > > commit normally, but defer writing the flush bits. This
> > > > > > > > > > > way, if you
> > > > > > > > > > > get another async update before the next vblank, you just
> > > > > > > > > > > apply it
> > > > > > > > > > > immediately instead of waiting for vblank.
> > > > > > > > > > >
> > > > > > > > > > > But I guess you are on a command mode panel, if I
> > > > > > > > > > > remember? Which is
> > > > > > > > > > > a case I didn't have a way to test. And I'm not entirely
> > > > > > > > > > > about how
> > > > > > > > > > > kms_funcs->vsync_time() should be implemented for cmd
> > > > > > > > > > > mode panels.
> > > > > > > > > >
> > > > > > > > > > Yes, this is a command-mode panel and there's no hardware
> > > > > > > > > > frame counter
> > > > > > > > > > available. The key to getting the display working on this
> > > > > > > > > > phone was this
> > > > > > > > > > patch:
> > > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2bab52af6fe68c43b327a57e5ce5fc10eefdfadf
> > > > > > > > > >
> > > > > > > > > > > That all said, I think we should first fix what is
> > > > > > > > > > > broken, before
> > > > > > > > > > >
Re: [Freedreno] [PATCH v2 1/3] dt-bindings: power: Convert Generic Power Domain bindings to json-schema
On 10/2/19 6:06 PM, Krzysztof Kozlowski wrote: > Convert Generic Power Domain bindings to DT schema format using > json-schema. The consumer bindings are split to separate file. > > Signed-off-by: Krzysztof Kozlowski For the media bindings: Acked-by: Hans Verkuil Thanks! Hans > > --- > > Changes since v1: > 1. Select all nodes for consumers, > 2. Remove from consumers duplicated properties with dt-schema, > 3. Fix power domain pattern, > 4. Remove unneeded types. > --- > .../devicetree/bindings/arm/arm,scmi.txt | 2 +- > .../devicetree/bindings/arm/arm,scpi.txt | 2 +- > .../bindings/arm/freescale/fsl,scu.txt| 2 +- > .../bindings/clock/clk-exynos-audss.txt | 2 +- > .../bindings/clock/exynos5433-clock.txt | 4 +- > .../bindings/clock/renesas,cpg-mssr.txt | 2 +- > .../clock/renesas,r8a7778-cpg-clocks.txt | 2 +- > .../clock/renesas,r8a7779-cpg-clocks.txt | 2 +- > .../clock/renesas,rcar-gen2-cpg-clocks.txt| 2 +- > .../bindings/clock/renesas,rz-cpg-clocks.txt | 2 +- > .../bindings/clock/ti/davinci/psc.txt | 2 +- > .../bindings/display/etnaviv/etnaviv-drm.txt | 2 +- > .../devicetree/bindings/display/msm/dpu.txt | 2 +- > .../devicetree/bindings/display/msm/mdp5.txt | 2 +- > .../devicetree/bindings/dsp/fsl,dsp.yaml | 2 +- > .../firmware/nvidia,tegra186-bpmp.txt | 2 +- > .../bindings/media/imx7-mipi-csi2.txt | 3 +- > .../bindings/media/mediatek-jpeg-decoder.txt | 3 +- > .../bindings/media/mediatek-mdp.txt | 3 +- > .../bindings/opp/qcom-nvmem-cpufreq.txt | 2 +- > .../devicetree/bindings/pci/pci-keystone.txt | 2 +- > .../bindings/phy/ti,phy-am654-serdes.txt | 2 +- > .../bindings/power/amlogic,meson-gx-pwrc.txt | 2 +- > .../devicetree/bindings/power/fsl,imx-gpc.txt | 2 +- > .../bindings/power/fsl,imx-gpcv2.txt | 2 +- > .../power/power-domain-consumers.yaml | 105 + > .../bindings/power/power-domain.yaml | 134 > .../bindings/power/power_domain.txt | 205 -- > .../devicetree/bindings/power/qcom,rpmpd.txt | 2 +- > .../bindings/power/renesas,rcar-sysc.txt | 2 +- > .../bindings/power/renesas,sysc-rmobile.txt | 2 +- > .../bindings/power/xlnx,zynqmp-genpd.txt | 2 +- > .../bindings/soc/bcm/brcm,bcm2835-pm.txt | 2 +- > .../bindings/soc/mediatek/scpsys.txt | 2 +- > .../bindings/soc/ti/sci-pm-domain.txt | 2 +- > .../bindings/usb/nvidia,tegra124-xusb.txt | 4 +- > MAINTAINERS | 2 +- > 37 files changed, 278 insertions(+), 241 deletions(-) > create mode 100644 > Documentation/devicetree/bindings/power/power-domain-consumers.yaml > create mode 100644 Documentation/devicetree/bindings/power/power-domain.yaml > delete mode 100644 Documentation/devicetree/bindings/power/power_domain.txt > > diff --git a/Documentation/devicetree/bindings/arm/arm,scmi.txt > b/Documentation/devicetree/bindings/arm/arm,scmi.txt > index 083dbf96ee00..f493d69e6194 100644 > --- a/Documentation/devicetree/bindings/arm/arm,scmi.txt > +++ b/Documentation/devicetree/bindings/arm/arm,scmi.txt > @@ -100,7 +100,7 @@ Required sub-node properties: > > [0] http://infocenter.arm.com/help/topic/com.arm.doc.den0056a/index.html > [1] Documentation/devicetree/bindings/clock/clock-bindings.txt > -[2] Documentation/devicetree/bindings/power/power_domain.txt > +[2] Documentation/devicetree/bindings/power/power-domain.yaml > [3] Documentation/devicetree/bindings/thermal/thermal.txt > [4] Documentation/devicetree/bindings/sram/sram.txt > [5] Documentation/devicetree/bindings/reset/reset.txt > diff --git a/Documentation/devicetree/bindings/arm/arm,scpi.txt > b/Documentation/devicetree/bindings/arm/arm,scpi.txt > index 401831973638..7b83ef43b418 100644 > --- a/Documentation/devicetree/bindings/arm/arm,scpi.txt > +++ b/Documentation/devicetree/bindings/arm/arm,scpi.txt > @@ -110,7 +110,7 @@ Required properties: > [1] Documentation/devicetree/bindings/clock/clock-bindings.txt > [2] Documentation/devicetree/bindings/thermal/thermal.txt > [3] Documentation/devicetree/bindings/sram/sram.txt > -[4] Documentation/devicetree/bindings/power/power_domain.txt > +[4] Documentation/devicetree/bindings/power/power-domain.yaml > > Example: > > diff --git a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > index c149fadc6f47..6c8a61b971f1 100644 > --- a/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > +++ b/Documentation/devicetree/bindings/arm/freescale/fsl,scu.txt > @@ -124,7 +124,7 @@ Required properties for Pinctrl sub nodes: > CONFIG settings. > > [1] Documentation/devicetree/bindings/clock/clock-bindings.txt > -[2] Documentation/devicetree/bindings/power/power_domain.txt > +[2]
Re: [Freedreno] drm/msm: 'pp done time out' errors after async commit changes
On Fri, Nov 08, 2019 at 07:56:25AM -0700, Jeffrey Hugo wrote:
> On Thu, Nov 7, 2019 at 7:03 PM Rob Clark wrote:
> >
> > On Thu, Nov 7, 2019 at 9:40 AM Jeffrey Hugo
> > wrote:
> > >
> > > On Thu, Nov 7, 2019 at 9:17 AM Rob Clark wrote:
> > > >
> > > > On Thu, Nov 7, 2019 at 3:10 AM Brian Masney
> > > > wrote:
> > > > >
> > > > > On Wed, Nov 06, 2019 at 08:58:59AM -0800, Rob Clark wrote:
> > > > > > On Wed, Nov 6, 2019 at 8:47 AM Jeffrey Hugo
> > > > > > wrote:
> > > > > > >
> > > > > > > On Wed, Nov 6, 2019 at 9:30 AM Rob Clark
> > > > > > > wrote:
> > > > > > > >
> > > > > > > > On Wed, Nov 6, 2019 at 1:13 AM Brian Masney
> > > > > > > > wrote:
> > > > > > > > >
> > > > > > > > > On Tue, Nov 05, 2019 at 08:23:27AM -0800, Rob Clark wrote:
> > > > > > > > > > On Tue, Nov 5, 2019 at 2:08 AM Brian Masney
> > > > > > > > > > wrote:
> > > > > > > > > > > The 'pp done time out' errors go away if I revert the
> > > > > > > > > > > following three
> > > > > > > > > > > commits:
> > > > > > > > > > >
> > > > > > > > > > > cd6d923167b1 ("drm/msm/dpu: async commit support")
> > > > > > > > > > > d934a712c5e6 ("drm/msm: add atomic traces")
> > > > > > > > > > > 2d99ced787e3 ("drm/msm: async commit support")
> > > > > > > > > > >
> > > > > > > > > > > I reverted the first one to fix a compiler error, and the
> > > > > > > > > > > second one so
> > > > > > > > > > > that the last patch can be reverted without any merge
> > > > > > > > > > > conflicts.
> > > > > > > > > > >
> > > > > > > > > > > I see that crtc_flush() calls mdp5_ctl_commit(). I tried
> > > > > > > > > > > to use
> > > > > > > > > > > crtc_flush_all() in mdp5_flush_commit() and the contents
> > > > > > > > > > > of the frame
> > > > > > > > > > > buffer dance around the screen like its out of sync. I
> > > > > > > > > > > renamed
> > > > > > > > > > > crtc_flush_all() to mdp5_crtc_flush_all() and removed the
> > > > > > > > > > > static
> > > > > > > > > > > declaration. Here's the relevant part of what I tried:
> > > > > > > > > > >
> > > > > > > > > > > --- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > > > > > > > > +++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
> > > > > > > > > > > @@ -171,7 +171,15 @@ static void
> > > > > > > > > > > mdp5_prepare_commit(struct msm_kms *kms, struct
> > > > > > > > > > > drm_atomic_state *st
> > > > > > > > > > >
> > > > > > > > > > > static void mdp5_flush_commit(struct msm_kms *kms,
> > > > > > > > > > > unsigned crtc_mask)
> > > > > > > > > > > {
> > > > > > > > > > > - /* TODO */
> > > > > > > > > > > + struct mdp5_kms *mdp5_kms =
> > > > > > > > > > > to_mdp5_kms(to_mdp_kms(kms));
> > > > > > > > > > > + struct drm_crtc *crtc;
> > > > > > > > > > > +
> > > > > > > > > > > + for_each_crtc_mask(mdp5_kms->dev, crtc,
> > > > > > > > > > > crtc_mask) {
> > > > > > > > > > > + if (!crtc->state->active)
> > > > > > > > > > > + continue;
> > > > > > > > > > > +
> > > > > > > > > > > + mdp5_crtc_flush_all(crtc);
> > > > > > > > > > > + }
> > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > Any tips would be appreciated.
> > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > I think this is along the lines of what we need to enable
> > > > > > > > > > async commit
> > > > > > > > > > for mdp5 (but also removing the flush from the
> > > > > > > > > > atomic-commit path)..
> > > > > > > > > > the principle behind the async commit is to do all the
> > > > > > > > > > atomic state
> > > > > > > > > > commit normally, but defer writing the flush bits. This
> > > > > > > > > > way, if you
> > > > > > > > > > get another async update before the next vblank, you just
> > > > > > > > > > apply it
> > > > > > > > > > immediately instead of waiting for vblank.
> > > > > > > > > >
> > > > > > > > > > But I guess you are on a command mode panel, if I remember?
> > > > > > > > > > Which is
> > > > > > > > > > a case I didn't have a way to test. And I'm not entirely
> > > > > > > > > > about how
> > > > > > > > > > kms_funcs->vsync_time() should be implemented for cmd mode
> > > > > > > > > > panels.
> > > > > > > > >
> > > > > > > > > Yes, this is a command-mode panel and there's no hardware
> > > > > > > > > frame counter
> > > > > > > > > available. The key to getting the display working on this
> > > > > > > > > phone was this
> > > > > > > > > patch:
> > > > > > > > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2bab52af6fe68c43b327a57e5ce5fc10eefdfadf
> > > > > > > > >
> > > > > > > > > > That all said, I think we should first fix what is broken,
> > > > > > > > > > before
> > > > > > > > > > worrying about extending async commit support to mdp5..
> > > > > > > > > > which
> > > > > > > > > > shouldn't hit the async==true path, due to not implementing
> > > > > > > > > > kms_funcs->vsync_time().
> > > > > > > > > >
> > > > > > > > > > What I think is
