Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM
On 10/3/20 2:45 AM, Daniel Vetter wrote:
On Sat, Oct 3, 2020 at 12:39 AM John Hubbard wrote:
On 10/2/20 10:53 AM, Daniel Vetter wrote:
For $reasons I've stumbled over this code and I'm not sure the change
to the new gup functions in 55a650c35fea ("mm/gup: frame_vector:
convert get_user_pages() --> pin_user_pages()") was entirely correct.
This here is used for long term buffers (not just quick I/O) like
RDMA, and John notes this in his patch. But I thought the rule for
these is that they need to add FOLL_LONGTERM, which John's patch
didn't do.
Yep. The earlier gup --> pup conversion patches were intended to not
have any noticeable behavior changes, and FOLL_LONGTERM, with it's
special cases and such, added some risk that I wasn't ready to take
on yet. Also, FOLL_LONGTERM rules are only *recently* getting firmed
up. So there was some doubt at least in my mind, about which sites
should have it.
But now that we're here, I think it's really good that you've brought
this up. It's definitely time to add FOLL_LONGTERM wherever it's missing.
So should I keep this patch, or will it collide with a series you're working on?
It doesn't collide with anything on my end yet, because I've been slow to
pick up on the need for changing callsites to add FOLL_LONGTERM. :)
And it looks like that's actually a problem, because:
Also with the firmed up rules, correct that I can also drop the
vma_is_fsdax check when the FOLL_LONGTERM flag is set?
That's the right direction to go *in general*, but I see that the
pin_user_pages code is still a bit stuck in the past. And this patch
won't actually work, with or without that vma_is_fsdax() check.
Because:
get_vaddr_frames(FOLL_LONGTERM)
pin_user_pages_locked()
if (WARN_ON_ONCE(gup_flags & FOLL_LONGTERM))
return -EINVAL;
So, again, pin_user_pages*() is at least partly behind the times here.
I can jump in and start fixing it up, but it depends on what you and
Oded and others are planning? Note: there is a particular combination of
dax and locking that we have to still avoid, within gup.c. That's
already covered, but needs to continue to be covered when we enable
FOLL_LONGTERM in the remaining pin_user_pages*() calling paths.
thanks,
--
John Hubbard
NVIDIA
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/gma500: fix double free of gma_connector
From: Tom Rix
clang static analysis reports this problem:
cdv_intel_dp.c:2101:2: warning: Attempt to free released memory
kfree(gma_connector);
^~~~
In cdv_intel_dp_init() when the call to cdv_intel_edp_panel_vdd_off()
fails, the handler calls cdv_intel_dp_destroy(connector) which does
the first free of gma_connector. So adjust the goto label and skip
the second free.
Fixes: d112a8163f83 ("gma500/cdv: Add eDP support")
Signed-off-by: Tom Rix
---
drivers/gpu/drm/gma500/cdv_intel_dp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/gma500/cdv_intel_dp.c
b/drivers/gpu/drm/gma500/cdv_intel_dp.c
index 720a767118c9..deb4fd13591d 100644
--- a/drivers/gpu/drm/gma500/cdv_intel_dp.c
+++ b/drivers/gpu/drm/gma500/cdv_intel_dp.c
@@ -2083,7 +2083,7 @@ cdv_intel_dp_init(struct drm_device *dev, struct
psb_intel_mode_device *mode_dev
DRM_INFO("failed to retrieve link info, disabling
eDP\n");
drm_encoder_cleanup(encoder);
cdv_intel_dp_destroy(connector);
- goto err_priv;
+ goto err_connector;
} else {
DRM_DEBUG_KMS("DPCD: Rev=%x LN_Rate=%x LN_CNT=%x
LN_DOWNSP=%x\n",
intel_dp->dpcd[0], intel_dp->dpcd[1],
--
2.18.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] dt-bindings: display: Add dsi-controller.yaml in DSI controller schemas
Hi,
On Fri, Oct 02, 2020 at 05:59:24PM -0500, Rob Herring wrote:
> Some DSI controllers are missing a reference to the recently added
> dsi-controller.yaml schema. Add it and we can drop the duplicate
> parts.
For the NWL part:
Reviewed-by: Guido Günther
>
> Cc: Maxime Ripard
> Cc: Chen-Yu Tsai
> Cc: Eric Anholt
> Cc: Nicolas Saenz Julienne
> Cc: Florian Fainelli
> Cc: Ray Jui
> Cc: Scott Branden
> Cc: bcm-kernel-feedback-l...@broadcom.com
> Cc: Maxime Coquelin
> Cc: Alexandre Torgue
> Cc: "Guido Gúnther"
> Cc: Robert Chiras
> Cc: Philippe Cornu
> Cc: Yannick Fertre
> Signed-off-by: Rob Herring
> ---
> .../display/allwinner,sun6i-a31-mipi-dsi.yaml | 11 ++---
> .../bindings/display/brcm,bcm2835-dsi0.yaml | 3 +++
> .../bindings/display/bridge/nwl-dsi.yaml | 11 -
> .../bindings/display/st,stm32-dsi.yaml| 23 ---
> 4 files changed, 14 insertions(+), 34 deletions(-)
>
> diff --git
> a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
> b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
> index 63f948175239..7aa330dabc44 100644
> ---
> a/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
> +++
> b/Documentation/devicetree/bindings/display/allwinner,sun6i-a31-mipi-dsi.yaml
> @@ -11,9 +11,6 @@ maintainers:
>- Maxime Ripard
>
> properties:
> - "#address-cells": true
> - "#size-cells": true
> -
>compatible:
> enum:
>- allwinner,sun6i-a31-mipi-dsi
> @@ -57,12 +54,7 @@ properties:
>port should be the input endpoint, usually coming from the
>associated TCON.
>
> -patternProperties:
> - "^panel@[0-9]+$": true
> -
> required:
> - - "#address-cells"
> - - "#size-cells"
>- compatible
>- reg
>- interrupts
> @@ -74,6 +66,7 @@ required:
>- port
>
> allOf:
> + - $ref: dsi-controller.yaml#
>- if:
>properties:
> compatible:
> @@ -99,7 +92,7 @@ allOf:
> clocks:
>minItems: 1
>
> -additionalProperties: false
> +unevaluatedProperties: false
>
> examples:
>- |
> diff --git a/Documentation/devicetree/bindings/display/brcm,bcm2835-dsi0.yaml
> b/Documentation/devicetree/bindings/display/brcm,bcm2835-dsi0.yaml
> index 3c643b227a70..eb44e072b6e5 100644
> --- a/Documentation/devicetree/bindings/display/brcm,bcm2835-dsi0.yaml
> +++ b/Documentation/devicetree/bindings/display/brcm,bcm2835-dsi0.yaml
> @@ -9,6 +9,9 @@ title: Broadcom VC4 (VideoCore4) DSI Controller
> maintainers:
>- Eric Anholt
>
> +allOf:
> + - $ref: dsi-controller.yaml#
> +
> properties:
>"#clock-cells":
> const: 1
> diff --git a/Documentation/devicetree/bindings/display/bridge/nwl-dsi.yaml
> b/Documentation/devicetree/bindings/display/bridge/nwl-dsi.yaml
> index b8ba6eb482a1..a125b2dd3a2f 100644
> --- a/Documentation/devicetree/bindings/display/bridge/nwl-dsi.yaml
> +++ b/Documentation/devicetree/bindings/display/bridge/nwl-dsi.yaml
> @@ -14,6 +14,9 @@ description: |
>NWL MIPI-DSI host controller found on i.MX8 platforms. This is a dsi
> bridge for
>the SOCs NWL MIPI-DSI host controller.
>
> +allOf:
> + - $ref: ../dsi-controller.yaml#
> +
> properties:
>compatible:
> const: fsl,imx8mq-nwl-dsi
> @@ -144,10 +147,6 @@ properties:
>
> additionalProperties: false
>
> -patternProperties:
> - "^panel@[0-9]+$":
> -type: object
> -
> required:
>- '#address-cells'
>- '#size-cells'
> @@ -163,7 +162,7 @@ required:
>- reset-names
>- resets
>
> -additionalProperties: false
> +unevaluatedProperties: false
>
> examples:
>- |
> @@ -172,7 +171,7 @@ examples:
> #include
> #include
>
> -mipi_dsi: mipi_dsi@30a0 {
> +dsi@30a0 {
>#address-cells = <1>;
>#size-cells = <0>;
>compatible = "fsl,imx8mq-nwl-dsi";
> diff --git a/Documentation/devicetree/bindings/display/st,stm32-dsi.yaml
> b/Documentation/devicetree/bindings/display/st,stm32-dsi.yaml
> index 69cc7e8bf15a..327a14d85df8 100644
> --- a/Documentation/devicetree/bindings/display/st,stm32-dsi.yaml
> +++ b/Documentation/devicetree/bindings/display/st,stm32-dsi.yaml
> @@ -13,6 +13,9 @@ maintainers:
> description:
>The STMicroelectronics STM32 DSI controller uses the Synopsys DesignWare
> MIPI-DSI host controller.
>
> +allOf:
> + - $ref: dsi-controller.yaml#
> +
> properties:
>compatible:
> const: st,stm32-dsi
> @@ -65,24 +68,6 @@ properties:
> description:
>DSI output port node, connected to a panel or a bridge input port"
>
> -patternProperties:
> - "^(panel|panel-dsi)@[0-9]$":
> -type: object
> -description:
> - A node containing the panel or bridge description as documented in
> - Documentation/devicetree/bindings/display/mipi-dsi-bus.txt
> -properties:
> - port:
> -type: object
> -description:
> - Panel or bridge port node,
Re: [RFC PATCH] drm: add support for taking drm atomic state snapshot
On Fri, Oct 2, 2020 at 9:27 PM Rob Clark wrote: > > On Fri, Oct 2, 2020 at 11:54 AM Daniel Vetter wrote: > > > > On Fri, Oct 02, 2020 at 10:22:42AM -0700, Rob Clark wrote: > > > On Fri, Oct 2, 2020 at 12:36 AM Daniel Vetter wrote: > > > > > > > > On Fri, Oct 2, 2020 at 3:47 AM Abhinav Kumar > > > > wrote: > > > > > > > > > > Add support to capture the drm atomic state snapshot which > > > > > can then be wired up with the devcoredump of the relevant display > > > > > errors to give useful information to debug the issues. > > > > > > > > > > Since the devcoredump is read by usermode and it is allowed > > > > > upto 5 minutes to read the coredump, capturing the snapshot that > > > > > time will not result in an accurate result. > > > > > > > > > > Rather we should capture the snapshot right after the error > > > > > happened. Hence add support for capturing this snapshot by > > > > > re-using the drm_atomic_helper_duplicate_state() API to support > > > > > a non-context version. > > > > > > > > > > Also add nolock versions of the drm_atomic_get_***_state() APIs > > > > > which can be used to get snapshot of the drm atomic state of > > > > > display drivers. > > > > > > > > > > Signed-off-by: Abhinav Kumar > > > > > > > > I guess this needs an example driver to show how this is used. > > > > > > fwiw, I suggested to Abhinav to send this early as an RFC, while he > > > finishes the rest of the driver part, just to get feedback on the > > > approach. > > > > > > The other option is to dump the state to a string buffer, and save > > > that until userspace reads out the devcoredump. This approach seems > > > less awkward, and lets us re-use drm_coredump_printer. > > > > Hm, I guess duplicating state is somewhat reasonable. Just make sure you > > take the locks. > > > > > > Another idea in this space is from Sean to implement a crash recorder > > > > of all the drm debug information. Which iirc already includes atomic > > > > state in some cases, but maybe not. The idea there was that userspace > > > > would dump that recording when something unexpected happens, since > > > > very often the kernel has no idea when something bad has happened, but > > > > the userspace compositor is a bit more in the loop on such things. I > > > > think ideally we have something that all fits together. > > > > > > We actually already have Sean's drm_trace stuff in CrOS kernel, and use > > > it. > > > > > > But at least in our case, the hw has error reporting (ie. things like > > > underflow irq's).. we want to use this to trigger dumping the current > > > state, plus a bunch of error related registers. The crash recorder > > > plays a role in this, but errors reported from the hw are the trigger, > > > and devcoredump is the mechanism. > > > > Uh if this is for production error interrupts then I really don't think > > the lockless games are a good idea. Laucnh a work and do it there, usually > > nothing really is happening anyway. Or have a threaded interrupt handler. > > > > > > The much bigger issue I'm seeing here is not taking locks. Ime that > > > > just crashes the kernel harder, and makes debugging harder. Somewhat > > > > ok for developer stuff in some cases, but devcoredump is more a > > > > production thing afaiui, so really doesn't sound like a good idea to > > > > me. > > > > > > I suppose *holding* the locks is a bigger issue than acquiring the > > > locks.. although it does mean it is not something we can do directly > > > from an irq context. Perhaps the driver part could be structured to > > > read hw state immediately, and then schedule work to snapshot the > > > atomic state. > > > > You can't, except if we do horrible stuff like make the pointer exchange > > exclude local interrupts, and then release the old memory with some kind > > of rcu. Really doesn't feel worth the trouble. You might think "hey it's > > only reading, that must be safe", but we're following all kinds of > > pointers and stuff, and I don't really want to make all that code > > perfectly paranoid and safe for lockless irq context with stuff > > disappearing underneath it at any moment. > > No, what I meant by that is snapshot the state (from worker) with > locks held (ie. drm_modeset_lock_all()) but then immediately drop the > locks after we have the snapshot, but before whenever userspace gets > around to reading out the state dump, where we would free the snapshot > state. Yeah I think that approach makes sense, sorry if that wasn't clear. We should probably rename the current drm_state_dump to something more scary, and make the "normal" version the one that takes locks. Also not modeset_lock_all, but modeset_lock_all_ctx (or the screaming macro to hide the retry loop). -Daniel > > BR, > -R > > > And yeah the various irq callers of drm_state_dump aren't nice, but right > > now you need to at least opt-in, so fairly clear it's for developers. > > > > That's at least my experience from deleting way too much code where people > > tho
[Bug 203439] amdgpu: [REG 4.20 -> 5.0] Brightness minimum level is too high
https://bugzilla.kernel.org/show_bug.cgi?id=203439 --- Comment #5 from acom...@gmail.com --- I forgot to mention about my device. I'm using HP Envy 13 x360(ag). -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 203439] amdgpu: [REG 4.20 -> 5.0] Brightness minimum level is too high
https://bugzilla.kernel.org/show_bug.cgi?id=203439 acom...@gmail.com changed: What|Removed |Added CC||acom...@gmail.com --- Comment #4 from acom...@gmail.com --- I also need this fix. As Błażej Szczygieł says, the minimum backlight level is too bright at night for me. That's why I'm staying at 4.19 instead of 5.x. I hope anyone considers better strategy like based on such device list. -- You are receiving this mail because: You are watching the assignee of the bug. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM
On Sat, Oct 3, 2020 at 12:39 AM John Hubbard wrote:
>
> On 10/2/20 10:53 AM, Daniel Vetter wrote:
> > For $reasons I've stumbled over this code and I'm not sure the change
> > to the new gup functions in 55a650c35fea ("mm/gup: frame_vector:
> > convert get_user_pages() --> pin_user_pages()") was entirely correct.
> >
> > This here is used for long term buffers (not just quick I/O) like
> > RDMA, and John notes this in his patch. But I thought the rule for
> > these is that they need to add FOLL_LONGTERM, which John's patch
> > didn't do.
>
> Yep. The earlier gup --> pup conversion patches were intended to not
> have any noticeable behavior changes, and FOLL_LONGTERM, with it's
> special cases and such, added some risk that I wasn't ready to take
> on yet. Also, FOLL_LONGTERM rules are only *recently* getting firmed
> up. So there was some doubt at least in my mind, about which sites
> should have it.
>
> But now that we're here, I think it's really good that you've brought
> this up. It's definitely time to add FOLL_LONGTERM wherever it's missing.
So should I keep this patch, or will it collide with a series you're working on?
Also with the firmed up rules, correct that I can also drop the
vma_is_fsdax check when the FOLL_LONGTERM flag is set?
Thanks, Daniel
>
> thanks,
> --
> John Hubbard
> NVIDIA
>
> >
> > There is already a dax specific check (added in b7f0554a56f2 ("mm:
> > fail get_vaddr_frames() for filesystem-dax mappings")), so this seems
> > like the prudent thing to do.
> >
> > Signed-off-by: Daniel Vetter
> > Cc: Andrew Morton
> > Cc: John Hubbard
> > Cc: Jérôme Glisse
> > Cc: Jan Kara
> > Cc: Dan Williams
> > Cc: linux...@kvack.org
> > Cc: linux-arm-ker...@lists.infradead.org
> > Cc: linux-samsung-...@vger.kernel.org
> > Cc: linux-me...@vger.kernel.org
> > ---
> > Hi all,
> >
> > I stumbled over this and figured typing this patch can't hurt. Really
> > just to maybe learn a few things about how gup/pup is supposed to be
> > used (we have a bit of that in drivers/gpu), this here isn't really
> > ralated to anything I'm doing.
> >
> > I'm also wondering whether the explicit dax check should be removed,
> > since FOLL_LONGTERM should take care of that already.
> > -Daniel
> > ---
> > mm/frame_vector.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/frame_vector.c b/mm/frame_vector.c
> > index 5d34c9047e9c..3507e09cb3ff 100644
> > --- a/mm/frame_vector.c
> > +++ b/mm/frame_vector.c
> > @@ -35,7 +35,7 @@ int get_vaddr_frames(unsigned long start, unsigned int
> > nr_frames,
> > {
> > struct mm_struct *mm = current->mm;
> > struct vm_area_struct *vma;
> > - unsigned int gup_flags = FOLL_WRITE | FOLL_FORCE;
> > + unsigned int gup_flags = FOLL_WRITE | FOLL_FORCE | FOLL_LONGTERM;
> > int ret = 0;
> > int err;
> > int locked;
> >
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM
On Sat, Oct 3, 2020 at 1:31 AM Jason Gunthorpe wrote:
>
> On Fri, Oct 02, 2020 at 08:16:48PM +0200, Daniel Vetter wrote:
> > On Fri, Oct 2, 2020 at 8:06 PM Jason Gunthorpe wrote:
> > > On Fri, Oct 02, 2020 at 07:53:03PM +0200, Daniel Vetter wrote:
> > > > For $reasons I've stumbled over this code and I'm not sure the change
> > > > to the new gup functions in 55a650c35fea ("mm/gup: frame_vector:
> > > > convert get_user_pages() --> pin_user_pages()") was entirely correct.
> > > >
> > > > This here is used for long term buffers (not just quick I/O) like
> > > > RDMA, and John notes this in his patch. But I thought the rule for
> > > > these is that they need to add FOLL_LONGTERM, which John's patch
> > > > didn't do.
> > > >
> > > > There is already a dax specific check (added in b7f0554a56f2 ("mm:
> > > > fail get_vaddr_frames() for filesystem-dax mappings")), so this seems
> > > > like the prudent thing to do.
> > > >
> > > > Signed-off-by: Daniel Vetter
> > > > Cc: Andrew Morton
> > > > Cc: John Hubbard
> > > > Cc: Jérôme Glisse
> > > > Cc: Jan Kara
> > > > Cc: Dan Williams
> > > > Cc: linux...@kvack.org
> > > > Cc: linux-arm-ker...@lists.infradead.org
> > > > Cc: linux-samsung-...@vger.kernel.org
> > > > Cc: linux-me...@vger.kernel.org
> > > > Hi all,
> > > >
> > > > I stumbled over this and figured typing this patch can't hurt. Really
> > > > just to maybe learn a few things about how gup/pup is supposed to be
> > > > used (we have a bit of that in drivers/gpu), this here isn't really
> > > > ralated to anything I'm doing.
> > >
> > > FOLL_FORCE is a pretty big clue it should be FOLL_LONGTERM, IMHO
> >
> > Since you're here ... I've noticed that ib sets FOLL_FORCE when the ib
> > verb access mode indicates possible writes. I'm not really clear on
> > why FOLL_WRITE isn't enough any why you need to be able to write
> > through a vma that's write protected currently.
>
> Ah, FOLL_FORCE | FOLL_WRITE means *read* confusingly enough, and the
> only reason you'd want this version for read is if you are doing
> longterm stuff. I wrote about this recently:
>
> https://lore.kernel.org/linux-mm/20200928235739.gu9...@ziepe.ca/
Ah, so essentially it serves as a FOLL_GET_COW_ISSUES_OUT_OF_MY_WAY. I
think documentation for this, and/or just automatically adding the
flag set combination would be really good. But I see John is already
on top of that it seems.
Thanks for the explainer.
> > > Since every driver does this wrong anything that uses this is creating
> > > terrifying security issues.
> > >
> > > IMHO this whole API should be deleted :(
> >
> > Yeah that part I just tried to conveniently ignore. I guess this dates
> > back to a time when ioremaps where at best fixed, and there wasn't
> > anything like a gpu driver dynamically managing vram around, resulting
> > in random entirely unrelated things possibly being mapped to that set
> > of pfns.
>
> No, it was always wrong. Prior to GPU like cases the lifetime of the
> PTE was tied to the vma and when the vma becomes free the driver can
> move the things in the PTEs to 'free'. Easy to trigger use-after-free
> issues and devices like RDMA have security contexts attached to these
> PTEs so it becomes a serious security bug to do something like this.
>
> > The underlying follow_pfn is also used in other places within
> > drivers/media, so this doesn't seem to be an accident, but actually
> > intentional.
>
> Looking closely, there are very few users, most *seem* pointless, but
> maybe there is a crazy reason?
>
> The sequence
> get_vaddr_frames();
> frame_vector_to_pages();
> sg_alloc_table_from_pages();
>
> Should be written
> pin_user_pages_fast(FOLL_LONGTERM);
> sg_alloc_table_from_pages()
Ok, that takes care of exynos and habanalabs. I'll try and wack
together a patch for exynos, that driver is a bit special - first arm
soc driver and we merged it fully well aware that it's full of warts,
just as a show to make it clear that drivers/gpu is also interested in
small gpu drivers ...
> There is some 'special' code in frame_vector_to_pages() that tries to
> get a struct page for things from a VM_IO or VM_PFNMAP...
>
> Oh snap, that is *completely* broken! If the first VMA is IO|PFNMAP
> then get_vaddr_frames() iterates over all VMAs in the range, of any
> kind and extracts the PTEs then blindly references them! This means it
> can be used to use after free normal RAM struct pages!! Gah!
>
> Wow. Okay. That has to go.
>
> So, I *think* we can assume there is no sane cases where
> frame_vector_to_pages() succeeds but pin_user_pages() wasn't called.
>
> That means the users here:
> - habanalabs: Hey Oded can you fix this up?
>
> - gpu/exynos: Daniel can you get someone there to stop using it?
>
> - media/videobuf via vb2_dma_sg_get_userptr()
>
> Should all be switched to the standard pin_user_pages sequence
> above.
>
> That leaves the only interesting places as vb2_dc_get_userptr() and
> vb2_vmalloc_get_userptr() which both completely fail t
Re: [PATCH v5 80/80] ARM: dts: bcm2711: Enable the display pipeline
Hi Tim, On Thu, Oct 01, 2020 at 11:15:46AM +0100, Tim Gover wrote: > hdmi_enable_4k60=1 causes the firmware to select 3.3 GHz for the PLLC > VCO to support a core-frequency of 550 MHz which is the minimum > frequency required by the HVS at 4Kp60. The side effect is that if the > display clock requirements are lower than 4Kp60 then you will see > different core frequencies selected by DVFS. > > If enable_uart=1 and the mini-uart is selected (default unless > bluetooth is disabled) then the firmware will pin the core-frequency > to either core_freq max (500 or 550). Although, I think there is a way > of pinning it to a lower fixed frequency. > > The table in overclocking.md defines options for setting the maximum > core frequency but unless core_freq_min is specified DVFS will > automatically pick the lowest idle frequency required by the display > resolution. I'm wondering if there's some way to detect this from Linux? I guess it would be nice to be able to at least detect a broken config to warn / prevent an user that their situation is not going to be reliable / work really well (like if they have a 4k display without hdmi_enable_4kp60 set, or the issue we're discussing here) Thanks! Maxime signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: WARNING in ieee80211_bss_info_change_notify
syzbot has bisected this issue to:
commit 135f971181d779c96ff3725c1a350a721785cc66
Author: Alex Deucher
Date: Mon Nov 20 22:49:53 2017 +
drm/amdgpu: don't skip attributes when powerplay is enabled
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=120f55bd90
start commit: fcadab74 Merge tag 'drm-fixes-2020-10-01-1' of git://anong..
git tree: upstream
final oops: https://syzkaller.appspot.com/x/report.txt?x=110f55bd90
console output: https://syzkaller.appspot.com/x/log.txt?x=160f55bd90
kernel config: https://syzkaller.appspot.com/x/.config?x=4e672827d2ffab1f
dashboard link: https://syzkaller.appspot.com/bug?extid=09d1cd2f71e6dd3bfd2c
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=161112eb90
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=124fc53390
Reported-by: syzbot+09d1cd2f71e6dd3bf...@syzkaller.appspotmail.com
Fixes: 135f971181d7 ("drm/amdgpu: don't skip attributes when powerplay is
enabled")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/msm/dp: fixes wrong connection state caused by failure of link train
Connection state is set incorrectly happen at either failure of link train
or cable plugged in while suspended. This patch fixes these problems.
This patch also replace ST_SUSPEND_PENDING with ST_DISPLAY_OFF.
Signed-off-by: Kuogee Hsieh
---
drivers/gpu/drm/msm/dp/dp_display.c | 52 ++---
drivers/gpu/drm/msm/dp/dp_panel.c | 5 +++
2 files changed, 31 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
b/drivers/gpu/drm/msm/dp/dp_display.c
index 431dff9de797..898c6cc1643a 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -45,7 +45,7 @@ enum {
ST_CONNECT_PENDING,
ST_CONNECTED,
ST_DISCONNECT_PENDING,
- ST_SUSPEND_PENDING,
+ ST_DISPLAY_OFF,
ST_SUSPENDED,
};
@@ -340,8 +340,6 @@ static int dp_display_process_hpd_high(struct
dp_display_private *dp)
}
dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
-
-
end:
return rc;
}
@@ -489,7 +487,7 @@ static int dp_hpd_plug_handle(struct dp_display_private
*dp, u32 data)
mutex_lock(&dp->event_mutex);
state = atomic_read(&dp->hpd_state);
- if (state == ST_SUSPEND_PENDING) {
+ if (state == ST_DISPLAY_OFF || state == ST_SUSPENDED) {
mutex_unlock(&dp->event_mutex);
return 0;
}
@@ -511,14 +509,14 @@ static int dp_hpd_plug_handle(struct dp_display_private
*dp, u32 data)
hpd->hpd_high = 1;
ret = dp_display_usbpd_configure_cb(&dp->pdev->dev);
- if (ret) { /* failed */
+ if (ret) { /* link train failed */
hpd->hpd_high = 0;
atomic_set(&dp->hpd_state, ST_DISCONNECTED);
+ } else {
+ /* start sentinel checking in case of missing uevent */
+ dp_add_event(dp, EV_CONNECT_PENDING_TIMEOUT, 0, tout);
}
- /* start sanity checking */
- dp_add_event(dp, EV_CONNECT_PENDING_TIMEOUT, 0, tout);
-
mutex_unlock(&dp->event_mutex);
/* uevent will complete connection part */
@@ -563,10 +561,6 @@ static int dp_hpd_unplug_handle(struct dp_display_private
*dp, u32 data)
mutex_lock(&dp->event_mutex);
state = atomic_read(&dp->hpd_state);
- if (state == ST_SUSPEND_PENDING) {
- mutex_unlock(&dp->event_mutex);
- return 0;
- }
if (state == ST_DISCONNECT_PENDING || state == ST_DISCONNECTED) {
mutex_unlock(&dp->event_mutex);
@@ -594,7 +588,7 @@ static int dp_hpd_unplug_handle(struct dp_display_private
*dp, u32 data)
*/
dp_display_usbpd_disconnect_cb(&dp->pdev->dev);
- /* start sanity checking */
+ /* start sentinel checking in case of missing uevent */
dp_add_event(dp, EV_DISCONNECT_PENDING_TIMEOUT, 0, DP_TIMEOUT_5_SECOND);
/* signal the disconnect event early to ensure proper teardown */
@@ -634,7 +628,7 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp,
u32 data)
/* irq_hpd can happen at either connected or disconnected state */
state = atomic_read(&dp->hpd_state);
- if (state == ST_SUSPEND_PENDING) {
+ if (state == ST_DISPLAY_OFF) {
mutex_unlock(&dp->event_mutex);
return 0;
}
@@ -1067,7 +1061,7 @@ static irqreturn_t dp_display_irq_handler(int irq, void
*dev_id)
}
if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) {
- /* delete connect pending event first */
+ /* delete sentinel connect pending checking */
dp_del_event(dp, EV_CONNECT_PENDING_TIMEOUT);
dp_add_event(dp, EV_IRQ_HPD_INT, 0, 0);
}
@@ -1186,19 +1180,19 @@ static int dp_pm_resume(struct device *dev)
dp = container_of(dp_display, struct dp_display_private, dp_display);
+ /* start from dis connection state */
+ atomic_set(&dp->hpd_state, ST_DISCONNECTED);
+
dp_display_host_init(dp);
dp_catalog_ctrl_hpd_config(dp->catalog);
status = dp_catalog_hpd_get_state_status(dp->catalog);
- if (status) {
+ if (status)
dp->dp_display.is_connected = true;
- } else {
+ else
dp->dp_display.is_connected = false;
- /* make sure next resume host_init be called */
- dp->core_initialized = false;
- }
return 0;
}
@@ -1214,6 +1208,9 @@ static int dp_pm_suspend(struct device *dev)
if (dp_display->power_on == true)
dp_display_disable(dp, 0);
+ /* host_init will be called at pm_resume */
+ dp->core_initialized = false;
+
atomic_set(&dp->hpd_state, ST_SUSPENDED);
return 0;
@@ -1343,6 +1340,9 @@ int msm_dp_display_enable(struct msm_dp *dp, struct
drm_encoder *encoder)
mutex_lock(&dp_display->event_mutex);
+ /* delete
Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM
On Fri, Oct 02, 2020 at 08:16:48PM +0200, Daniel Vetter wrote:
> On Fri, Oct 2, 2020 at 8:06 PM Jason Gunthorpe wrote:
> > On Fri, Oct 02, 2020 at 07:53:03PM +0200, Daniel Vetter wrote:
> > > For $reasons I've stumbled over this code and I'm not sure the change
> > > to the new gup functions in 55a650c35fea ("mm/gup: frame_vector:
> > > convert get_user_pages() --> pin_user_pages()") was entirely correct.
> > >
> > > This here is used for long term buffers (not just quick I/O) like
> > > RDMA, and John notes this in his patch. But I thought the rule for
> > > these is that they need to add FOLL_LONGTERM, which John's patch
> > > didn't do.
> > >
> > > There is already a dax specific check (added in b7f0554a56f2 ("mm:
> > > fail get_vaddr_frames() for filesystem-dax mappings")), so this seems
> > > like the prudent thing to do.
> > >
> > > Signed-off-by: Daniel Vetter
> > > Cc: Andrew Morton
> > > Cc: John Hubbard
> > > Cc: Jérôme Glisse
> > > Cc: Jan Kara
> > > Cc: Dan Williams
> > > Cc: linux...@kvack.org
> > > Cc: linux-arm-ker...@lists.infradead.org
> > > Cc: linux-samsung-...@vger.kernel.org
> > > Cc: linux-me...@vger.kernel.org
> > > Hi all,
> > >
> > > I stumbled over this and figured typing this patch can't hurt. Really
> > > just to maybe learn a few things about how gup/pup is supposed to be
> > > used (we have a bit of that in drivers/gpu), this here isn't really
> > > ralated to anything I'm doing.
> >
> > FOLL_FORCE is a pretty big clue it should be FOLL_LONGTERM, IMHO
>
> Since you're here ... I've noticed that ib sets FOLL_FORCE when the ib
> verb access mode indicates possible writes. I'm not really clear on
> why FOLL_WRITE isn't enough any why you need to be able to write
> through a vma that's write protected currently.
Ah, FOLL_FORCE | FOLL_WRITE means *read* confusingly enough, and the
only reason you'd want this version for read is if you are doing
longterm stuff. I wrote about this recently:
https://lore.kernel.org/linux-mm/20200928235739.gu9...@ziepe.ca/
> > Since every driver does this wrong anything that uses this is creating
> > terrifying security issues.
> >
> > IMHO this whole API should be deleted :(
>
> Yeah that part I just tried to conveniently ignore. I guess this dates
> back to a time when ioremaps where at best fixed, and there wasn't
> anything like a gpu driver dynamically managing vram around, resulting
> in random entirely unrelated things possibly being mapped to that set
> of pfns.
No, it was always wrong. Prior to GPU like cases the lifetime of the
PTE was tied to the vma and when the vma becomes free the driver can
move the things in the PTEs to 'free'. Easy to trigger use-after-free
issues and devices like RDMA have security contexts attached to these
PTEs so it becomes a serious security bug to do something like this.
> The underlying follow_pfn is also used in other places within
> drivers/media, so this doesn't seem to be an accident, but actually
> intentional.
Looking closely, there are very few users, most *seem* pointless, but
maybe there is a crazy reason?
The sequence
get_vaddr_frames();
frame_vector_to_pages();
sg_alloc_table_from_pages();
Should be written
pin_user_pages_fast(FOLL_LONGTERM);
sg_alloc_table_from_pages()
There is some 'special' code in frame_vector_to_pages() that tries to
get a struct page for things from a VM_IO or VM_PFNMAP...
Oh snap, that is *completely* broken! If the first VMA is IO|PFNMAP
then get_vaddr_frames() iterates over all VMAs in the range, of any
kind and extracts the PTEs then blindly references them! This means it
can be used to use after free normal RAM struct pages!! Gah!
Wow. Okay. That has to go.
So, I *think* we can assume there is no sane cases where
frame_vector_to_pages() succeeds but pin_user_pages() wasn't called.
That means the users here:
- habanalabs: Hey Oded can you fix this up?
- gpu/exynos: Daniel can you get someone there to stop using it?
- media/videobuf via vb2_dma_sg_get_userptr()
Should all be switched to the standard pin_user_pages sequence
above.
That leaves the only interesting places as vb2_dc_get_userptr() and
vb2_vmalloc_get_userptr() which both completely fail to follow the
REQUIRED behavior in the function's comment about checking PTEs. It
just DMA maps them. Badly broken.
Guessing this hackery is for some embedded P2P DMA transfer?
After he three places above should use pin_user_pages_fast(), then
this whole broken API should be moved into videobuf2-memops.c and a
big fat "THIS DOESN'T WORK" stuck on it.
videobuf2 should probably use P2P DMA buf for this instead.
Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] dt-bindings: Another round of adding missing 'additionalProperties'
On 10/2/20 4:41 PM, Rob Herring wrote: > Another round of wack-a-mole. The json-schema default is additional > unknown properties are allowed, but for DT all properties should be > defined. > > Cc: Thierry Reding > Cc: Linus Walleij > Cc: Stephen Boyd > Cc: Shawn Guo > Cc: Bjorn Andersson > Cc: Baolin Wang > Cc: Guenter Roeck > Cc: Jonathan Cameron > Cc: Mauro Carvalho Chehab > Cc: Laurent Pinchart > Cc: Lee Jones > Cc: Ulf Hansson > Cc: "David S. Miller" > Cc: Bjorn Helgaas > Cc: Vinod Koul > Cc: Liam Girdwood > Cc: Mark Brown > Cc: Greg Kroah-Hartman > Cc: Daniel Lezcano > Cc: linux-...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: linux-...@vger.kernel.org > Cc: linux-g...@vger.kernel.org > Cc: linux-hw...@vger.kernel.org For hwmon: Reviewed-by: Guenter Roeck > Cc: linux-...@vger.kernel.org > Cc: openipmi-develo...@lists.sourceforge.net > Cc: linux-l...@vger.kernel.org > Cc: linux-me...@vger.kernel.org > Cc: linux-rockc...@lists.infradead.org > Cc: linux-st...@st-md-mailman.stormreply.com > Cc: linux-m...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: net...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: linux...@vger.kernel.org > Cc: linux-remotep...@vger.kernel.org > Cc: linux-ser...@vger.kernel.org > Cc: alsa-de...@alsa-project.org > Cc: linux-...@vger.kernel.org > Signed-off-by: Rob Herring > --- > > I'll take this thru the DT tree. > > .../arm/bcm/raspberrypi,bcm2835-firmware.yaml | 2 ++ > .../arm/mediatek/mediatek,pericfg.yaml| 2 ++ > .../devicetree/bindings/arm/pmu.yaml | 2 ++ > .../devicetree/bindings/arm/primecell.yaml| 3 +++ > .../bindings/arm/samsung/sysreg.yaml | 2 ++ > .../arm/tegra/nvidia,tegra20-pmc.yaml | 2 ++ > .../bindings/bus/mti,mips-cdmm.yaml | 2 ++ > .../bus/socionext,uniphier-system-bus.yaml| 7 +++ > .../bindings/clock/arm,syscon-icst.yaml | 2 ++ > .../bindings/clock/idt,versaclock5.yaml | 20 ++- > .../bindings/clock/imx6q-clock.yaml | 2 ++ > .../bindings/clock/imx6sl-clock.yaml | 2 ++ > .../bindings/clock/imx6sll-clock.yaml | 2 ++ > .../bindings/clock/imx6sx-clock.yaml | 2 ++ > .../bindings/clock/imx6ul-clock.yaml | 2 ++ > .../bindings/clock/intel,cgu-lgm.yaml | 2 ++ > .../bindings/clock/qcom,gcc-sm8250.yaml | 2 ++ > .../bindings/clock/sprd,sc9863a-clk.yaml | 2 ++ > .../bindings/clock/ti,am654-ehrpwm-tbclk.yaml | 2 ++ > .../bindings/display/bridge/ite,it6505.yaml | 5 + > .../bindings/display/bridge/lvds-codec.yaml | 3 +++ > .../devicetree/bindings/display/msm/gmu.yaml | 2 ++ > .../devicetree/bindings/edac/dmc-520.yaml | 2 ++ > .../devicetree/bindings/fsi/ibm,fsi2spi.yaml | 2 ++ > .../gpio/socionext,uniphier-gpio.yaml | 2 ++ > .../bindings/hwmon/adi,axi-fan-control.yaml | 2 ++ > .../devicetree/bindings/hwmon/adt7475.yaml| 2 ++ > .../bindings/iio/accel/kionix,kxsd9.yaml | 4 > .../bindings/iio/adc/maxim,max1238.yaml | 2 ++ > .../bindings/iio/adc/maxim,max1363.yaml | 2 ++ > .../bindings/iio/adc/qcom,spmi-vadc.yaml | 4 > .../bindings/iio/adc/samsung,exynos-adc.yaml | 2 ++ > .../bindings/iio/adc/ti,ads8688.yaml | 4 > .../bindings/iio/amplifiers/adi,hmc425a.yaml | 2 ++ > .../bindings/iio/imu/invensense,icm42600.yaml | 6 ++ > .../bindings/iio/light/amstaos,tsl2563.yaml | 2 ++ > .../bindings/iio/light/dynaimage,al3010.yaml | 2 ++ > .../bindings/iio/light/dynaimage,al3320a.yaml | 2 ++ > .../bindings/iio/light/sharp,gp2ap002.yaml| 2 ++ > .../iio/magnetometer/asahi-kasei,ak8975.yaml | 2 ++ > .../iio/proximity/vishay,vcnl3020.yaml| 2 ++ > .../interrupt-controller/ingenic,intc.yaml| 2 ++ > .../loongson,pch-msi.yaml | 2 ++ > .../loongson,pch-pic.yaml | 2 ++ > .../devicetree/bindings/ipmi/ipmi-smic.yaml | 2 ++ > .../devicetree/bindings/leds/leds-lp55xx.yaml | 8 > .../bindings/media/i2c/chrontel,ch7322.yaml | 2 ++ > .../bindings/media/i2c/imi,rdacm2x-gmsl.yaml | 2 ++ > .../bindings/media/nxp,imx8mq-vpu.yaml| 2 ++ > .../bindings/media/qcom,msm8916-venus.yaml| 2 ++ > .../bindings/media/qcom,msm8996-venus.yaml| 2 ++ > .../bindings/media/qcom,sc7180-venus.yaml | 2 ++ > .../bindings/media/qcom,sdm845-venus-v2.yaml | 2 ++ > .../bindings/media/qcom,sdm845-venus.yaml | 2 ++ > .../bindings/memory-controllers/fsl/mmdc.yaml | 2 ++ > .../memory-controllers/st,stm32-fmc2-ebi.yaml | 2 ++ > .../bindings/mfd/gateworks-gsc.yaml | 2 ++ > .../bindings/mfd/xylon,logicvc.yaml | 14 +++-- > .../bindings/mips/ingenic/ingenic,cpu.yaml| 6 -- > .../bindings/mips/loongson/rs780e-acpi.yaml | 2 ++ > .../bindings/mmc/mmc-pwrseq-emmc.yaml | 2 ++ > .../bindings/mmc/mmc-pwrseq-sd8787.yaml
Re: [PATCH 0/4] Mediatek DRM driver detect CMDQ execution timeout by vblank IRQ
On Sun, Sep 27, 2020 at 6:04 PM Chun-Kuang Hu wrote: > > CMDQ helper provide timer to detect execution timeout, but DRM driver > could have a better way to detect execution timeout by vblank IRQ. > For DRM, CMDQ command should execute in vblank, so if it fail to > execute in next 2 vblank, timeout happen. Even though we could > calculate time between 2 vblank and use timer to delect, this would > make things more complicated. > > This introduce a series refinement for CMDQ mailbox controller and CMDQ > helper. Remove timer handler in helper function because different > client have different way to detect timeout. Use standard mailbox > callback instead of proprietary one to get the necessary data > in callback function. Remove struct cmdq_client to access client > instance data by struct mbox_client. > > Chun-Kuang Hu (4): > soc / drm: mediatek: cmdq: Remove timeout handler in helper function > mailbox / soc / drm: mediatek: Use mailbox rx_callback instead of > cmdq_task_cb > mailbox / soc / drm: mediatek: Remove struct cmdq_client > drm/mediatek: Detect CMDQ execution timeout > > drivers/gpu/drm/mediatek/mtk_drm_crtc.c | 54 ++--- > drivers/mailbox/mtk-cmdq-mailbox.c | 24 ++-- > drivers/soc/mediatek/mtk-cmdq-helper.c | 146 ++- > include/linux/mailbox/mtk-cmdq-mailbox.h | 25 +--- > include/linux/soc/mediatek/mtk-cmdq.h| 54 + > 5 files changed, 66 insertions(+), 237 deletions(-) > Please break this into two patchsets - one for mailbox and one for its users. Also, CC original author and recent major contributors to mtk-cmdq-mailbox.c Thanks. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM
On Fri, Oct 02, 2020 at 07:53:03PM +0200, Daniel Vetter wrote:
> For $reasons I've stumbled over this code and I'm not sure the change
> to the new gup functions in 55a650c35fea ("mm/gup: frame_vector:
> convert get_user_pages() --> pin_user_pages()") was entirely correct.
>
> This here is used for long term buffers (not just quick I/O) like
> RDMA, and John notes this in his patch. But I thought the rule for
> these is that they need to add FOLL_LONGTERM, which John's patch
> didn't do.
>
> There is already a dax specific check (added in b7f0554a56f2 ("mm:
> fail get_vaddr_frames() for filesystem-dax mappings")), so this seems
> like the prudent thing to do.
>
> Signed-off-by: Daniel Vetter
> Cc: Andrew Morton
> Cc: John Hubbard
> Cc: Jérôme Glisse
> Cc: Jan Kara
> Cc: Dan Williams
> Cc: linux...@kvack.org
> Cc: linux-arm-ker...@lists.infradead.org
> Cc: linux-samsung-...@vger.kernel.org
> Cc: linux-me...@vger.kernel.org
> Hi all,
>
> I stumbled over this and figured typing this patch can't hurt. Really
> just to maybe learn a few things about how gup/pup is supposed to be
> used (we have a bit of that in drivers/gpu), this here isn't really
> ralated to anything I'm doing.
FOLL_FORCE is a pretty big clue it should be FOLL_LONGTERM, IMHO
> I'm also wondering whether the explicit dax check should be removed,
> since FOLL_LONGTERM should take care of that already.
Yep! Confirms the above!
This get_vaddr_frames() thing looks impossible to use properly. How on
earth does a driver guarentee
"If @start belongs to VM_IO | VM_PFNMAP vma, we don't touch page
structures and the caller must make sure pfns aren't reused for
anything else while he is using them."
The only possible way to do that is if the driver restricts the VMAs
to ones it owns and interacts with the vm_private data to refcount
something.
Since every driver does this wrong anything that uses this is creating
terrifying security issues.
IMHO this whole API should be deleted :(
Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/msm/dp: fixes wrong connection state caused by failure of link train
Quoting Kuogee Hsieh (2020-10-02 15:09:19)
> Connection state is set incorrectly happen at either failure of link train
> or cable plugged in while suspended. This patch fixes these problems.
> This patch also replace ST_SUSPEND_PENDING with ST_DISPLAY_OFF.
>
> Signed-off-by: Kuogee Hsieh
Any Fixes: tag?
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 52 ++---
> drivers/gpu/drm/msm/dp/dp_panel.c | 5 +++
> 2 files changed, 31 insertions(+), 26 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index 431dff9de797..898c6cc1643a 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -340,8 +340,6 @@ static int dp_display_process_hpd_high(struct
> dp_display_private *dp)
> }
>
> dp_add_event(dp, EV_USER_NOTIFICATION, true, 0);
> -
> -
> end:
> return rc;
> }
Not sure we need this hunk
> @@ -1186,19 +1180,19 @@ static int dp_pm_resume(struct device *dev)
>
> dp = container_of(dp_display, struct dp_display_private, dp_display);
>
> + /* start from dis connection state */
disconnection? Or disconnected state?
> + atomic_set(&dp->hpd_state, ST_DISCONNECTED);
> +
> dp_display_host_init(dp);
>
> dp_catalog_ctrl_hpd_config(dp->catalog);
>
> status = dp_catalog_hpd_get_state_status(dp->catalog);
>
> - if (status) {
> + if (status)
> dp->dp_display.is_connected = true;
> - } else {
> + else
> dp->dp_display.is_connected = false;
> - /* make sure next resume host_init be called */
> - dp->core_initialized = false;
> - }
>
> return 0;
> }
> @@ -1214,6 +1208,9 @@ static int dp_pm_suspend(struct device *dev)
> if (dp_display->power_on == true)
> dp_display_disable(dp, 0);
>
> + /* host_init will be called at pm_resume */
> + dp->core_initialized = false;
> +
> atomic_set(&dp->hpd_state, ST_SUSPENDED);
>
> return 0;
> @@ -1343,6 +1340,9 @@ int msm_dp_display_enable(struct msm_dp *dp, struct
> drm_encoder *encoder)
>
> mutex_lock(&dp_display->event_mutex);
>
> + /* delete sentinel checking */
Stop sentinel checking?
> + dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
> +
> rc = dp_display_set_mode(dp, &dp_display->dp_mode);
> if (rc) {
> DRM_ERROR("Failed to perform a mode set, rc=%d\n", rc);
> @@ -1368,9 +1368,8 @@ int msm_dp_display_enable(struct msm_dp *dp, struct
> drm_encoder *encoder)
> dp_display_unprepare(dp);
> }
>
> - dp_del_event(dp_display, EV_CONNECT_PENDING_TIMEOUT);
> -
> - if (state == ST_SUSPEND_PENDING)
> + /* manual kick off plug event to train link */
> + if (state == ST_DISPLAY_OFF)
> dp_add_event(dp_display, EV_IRQ_HPD_INT, 0, 0);
>
> /* completed connection */
> @@ -1402,20 +1401,21 @@ int msm_dp_display_disable(struct msm_dp *dp, struct
> drm_encoder *encoder)
>
> mutex_lock(&dp_display->event_mutex);
>
> + /* delete sentinel checking */
Stop sentinel checking?
> + dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
> +
> dp_display_disable(dp_display, 0);
>
> rc = dp_display_unprepare(dp);
> if (rc)
> DRM_ERROR("DP display unprepare failed, rc=%d\n", rc);
>
> - dp_del_event(dp_display, EV_DISCONNECT_PENDING_TIMEOUT);
> -
> state = atomic_read(&dp_display->hpd_state);
> if (state == ST_DISCONNECT_PENDING) {
I don't understand the atomic nature of this hpd_state variable. Why is
it an atomic variable? Is taking a spinlock bad? What is to prevent the
atomic read here to not be interrupted and then this if condition check
be invalid because the variable has been updated somewhere else?
> /* completed disconnection */
> atomic_set(&dp_display->hpd_state, ST_DISCONNECTED);
> } else {
> - atomic_set(&dp_display->hpd_state, ST_SUSPEND_PENDING);
> + atomic_set(&dp_display->hpd_state, ST_DISPLAY_OFF);
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Make the kerneldoc a bit clearer
Hi,
On Fri, Oct 02, 2020 at 09:56:20AM +0200, Daniel Vetter wrote:
> Crank up the warning a notch and point at the right set of locking
> functions for atomic drivers.
>
> Signed-off-by: Daniel Vetter
> Cc: Maarten Lankhorst
> Cc: Maxime Ripard
> Cc: Thomas Zimmermann
> Cc: David Airlie
> Cc: Daniel Vetter
> ---
> drivers/gpu/drm/drm_atomic.c | 10 +-
> 1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index aac9122f1da2..b2d20eb6c807 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1642,11 +1642,11 @@ static void __drm_state_dump(struct drm_device *dev,
> struct drm_printer *p,
> * to dmesg in case of error irq's. (Hint, you probably want to
> * ratelimit this!)
> *
> - * The caller must drm_modeset_lock_all(), or if this is called
> - * from error irq handler, it should not be enabled by default.
> - * (Ie. if you are debugging errors you might not care that this
> - * is racey. But calling this without all modeset locks held is
> - * not inherently safe.)
> + * The caller must wrap this drm_modeset_lock_all_ctx() and
> + * drm_modeset_drop_locks(). If this is called from error irq handler, it
> should
> + * not be enabled by default - if you are debugging errors you might
> + * not care that this is racey, but calling this without all modeset locks
> held
> + * is inherently unsafe.
> */
> void drm_state_dump(struct drm_device *dev, struct drm_printer *p)
> {
For the comment itself:
Acked-by: Maxime Ripard
But maybe we should add some lockdep assertion to make sure we can catch
someone actually doing this?
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH rdma-next v4 1/4] lib/scatterlist: Add support in dynamic allocation of SG table from pages
On Sun, Sep 27, 2020 at 09:46:44AM +0300, Leon Romanovsky wrote:
> +struct scatterlist *__sg_alloc_table_from_pages(struct sg_table *sgt,
> + struct page **pages, unsigned int n_pages, unsigned int offset,
> + unsigned long size, unsigned int max_segment,
> + struct scatterlist *prv, unsigned int left_pages,
> + gfp_t gfp_mask)
> {
> - unsigned int chunks, cur_page, seg_len, i;
> + unsigned int chunks, cur_page, seg_len, i, prv_len = 0;
> + struct scatterlist *s = prv;
> + unsigned int table_size;
> + unsigned int tmp_nents;
> int ret;
> - struct scatterlist *s;
>
> if (WARN_ON(!max_segment || offset_in_page(max_segment)))
> - return -EINVAL;
> + return ERR_PTR(-EINVAL);
> + if (IS_ENABLED(CONFIG_ARCH_NO_SG_CHAIN) && prv)
> + return ERR_PTR(-EOPNOTSUPP);
> +
> + tmp_nents = prv ? sgt->nents : 0;
> +
> + if (prv &&
> + page_to_pfn(sg_page(prv)) + (prv->length >> PAGE_SHIFT) ==
This calculation of the end doesn't consider sg->offset
> + page_to_pfn(pages[0]))
> + prv_len = prv->length;
>
> /* compute number of contiguous chunks */
> chunks = 1;
> @@ -410,13 +461,17 @@ int __sg_alloc_table_from_pages(struct sg_table *sgt,
> struct page **pages,
> }
> }
>
> - ret = sg_alloc_table(sgt, chunks, gfp_mask);
> - if (unlikely(ret))
> - return ret;
> + if (!prv) {
> + /* Only the last allocation could be less than the maximum */
> + table_size = left_pages ? SG_MAX_SINGLE_ALLOC : chunks;
> + ret = sg_alloc_table(sgt, table_size, gfp_mask);
> + if (unlikely(ret))
> + return ERR_PTR(ret);
> + }
This is basically redundant right? Now that get_next_sg() can allocate
SGs it can just build them one by one, no need to preallocate.
Actually all the changes the the allocation seem like overkill, just
allocate a single new array directly in get_next_sg() whenever it
needs.
Something like this:
@@ -365,6 +372,37 @@ int sg_alloc_table(struct sg_table *table, unsigned int
nents, gfp_t gfp_mask)
}
EXPORT_SYMBOL(sg_alloc_table);
+static struct scatterlist *get_next_sg(struct sg_table *table,
+ struct scatterlist *cur, unsigned long needed_sges,
+ gfp_t gfp_mask)
+{
+ struct scatterlist *new_sg;
+ unsigned int alloc_size;
+
+ if (cur) {
+ struct scatterlist *next_sg = sg_next(cur);
+
+ /* Check if last entry should be keeped for chainning */
+ if (!sg_is_last(next_sg) || needed_sges == 1)
+ return next_sg;
+ }
+
+ alloc_size = min_t(unsigned long, needed_sges, SG_MAX_SINGLE_ALLOC);
+ new_sg = sg_kmalloc(alloc_size, gfp_mask);
+ if (!new_sg)
+ return ERR_PTR(-ENOMEM);
+ sg_init_table(new_sg, alloc_size);
+ if (cur) {
+ __sg_chain(cur, new_sg);
+ table->orig_nents += alloc_size - 1;
+ } else {
+ table->sgl = new_sg;
+ table->orig_nents = alloc_size;
+ table->nents = 0;
+ }
+ return new_sg;
+}
+
/**
* __sg_alloc_table_from_pages - Allocate and initialize an sg table from
* an array of pages
@@ -374,29 +412,64 @@ EXPORT_SYMBOL(sg_alloc_table);
* @offset: Offset from start of the first page to the start of a buffer
* @size:Number of valid bytes in the buffer (after offset)
* @max_segment: Maximum size of a scatterlist node in bytes (page aligned)
+ * @prv:Last populated sge in sgt
+ * @left_pages: Left pages caller have to set after this call
* @gfp_mask: GFP allocation mask
*
- * Description:
- *Allocate and initialize an sg table from a list of pages. Contiguous
- *ranges of the pages are squashed into a single scatterlist node up to the
- *maximum size specified in @max_segment. An user may provide an offset at
a
- *start and a size of valid data in a buffer specified by the page array.
- *The returned sg table is released by sg_free_table.
+ * Description:
+ *If @prv is NULL, allocate and initialize an sg table from a list of
pages,
+ *else reuse the scatterlist passed in at @prv.
+ *Contiguous ranges of the pages are squashed into a single scatterlist
+ *entry up to the maximum size specified in @max_segment. A user may
+ *provide an offset at a start and a size of valid data in a buffer
+ *specified by the page array.
*
* Returns:
- * 0 on success, negative error on failure
+ * Last SGE in sgt on success, PTR_ERR on otherwise.
+ * The allocation in @sgt must be released by sg_free_table.
+ *
+ * Notes:
+ * If this function returns non-0 (eg failure), the caller must call
+ * sg_free_table() to cleanup any leftover allocations.
*/
-int __sg_alloc_table_from_page
Re: [PATCH 2/3] drm/msm: add DRM_MSM_GEM_SYNC_CACHE for non-coherent cache maintenance
On 10/2/20 3:53 AM, Christoph Hellwig wrote:
@@ -8,6 +8,7 @@
#include
#include
#include
+#include
NAK, dma-noncoherent.h is not for driver use. And will in fact go
away in 5.10.
Not actually used, so can be removed.
#include
@@ -808,6 +809,20 @@ int msm_gem_cpu_fini(struct drm_gem_object *obj)
return 0;
}
+void msm_gem_sync_cache(struct drm_gem_object *obj, uint32_t flags,
+ size_t range_start, size_t range_end)
+{
+ struct msm_gem_object *msm_obj = to_msm_bo(obj);
+
+ /* TODO: sync only the required range, and don't invalidate on clean */
+
+ if (flags & MSM_GEM_SYNC_CACHE_CLEAN)
+ sync_for_device(msm_obj);
+
+ if (flags & MSM_GEM_SYNC_CACHE_INVALIDATE)
+ sync_for_cpu(msm_obj);
And make to these ones as well. They are complete abuses of the DMA
API, and while we had to live with that for now to not cause regressions
they absoutely must not be exposed in a userspace ABI like this.
How do you propose that cached non-coherent memory be implemented? It is
a useful feature for userspace.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/stm: Fix bus_flags handling
The drm_display_mode_to_videomode() does not populate DISPLAY_FLAGS_DE_LOW
or DISPLAY_FLAGS_PIXDATA_NEGEDGE flags in struct videomode. Therefore, no
matter what polarity the next bridge or display might require, these flags
are never set, and thus the LTDC GCR_DEPOL and GCR_PCPOL bits are never set,
and the LTDC behaves as if both DISPLAY_FLAGS_PIXDATA_POSEDGE and
DISPLAY_FLAGS_DE_HIGH were always set.
The fix for this problem is taken almost verbatim from MXSFB driver. In
case there is a bridge attached to the LTDC, the bridge might have extra
polarity requirements, so extract bus_flags from the bridge and use them
for LTDC configuration. Otherwise, extract bus_flags from the connector,
which is the display.
Fixes: b759012c5fa7 ("drm/stm: Add STM32 LTDC driver")
Signed-off-by: Marek Vasut
Cc: Alexandre Torgue
Cc: Antonio Borneo
Cc: Benjamin Gaignard
Cc: Maxime Coquelin
Cc: Philippe Cornu
Cc: Sam Ravnborg
Cc: Vincent Abriou
Cc: Yannick Fertre
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-st...@st-md-mailman.stormreply.com
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/stm/ltdc.c | 22 --
drivers/gpu/drm/stm/ltdc.h | 2 ++
2 files changed, 22 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/stm/ltdc.c b/drivers/gpu/drm/stm/ltdc.c
index 07c73079293c..a282a1553497 100644
--- a/drivers/gpu/drm/stm/ltdc.c
+++ b/drivers/gpu/drm/stm/ltdc.c
@@ -546,11 +546,17 @@ static void ltdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
struct drm_device *ddev = crtc->dev;
struct drm_display_mode *mode = &crtc->state->adjusted_mode;
struct videomode vm;
+ u32 bus_flags = 0;
u32 hsync, vsync, accum_hbp, accum_vbp, accum_act_w, accum_act_h;
u32 total_width, total_height;
u32 val;
int ret;
+ if (ldev->bridge)
+ bus_flags = ldev->bridge->timings->input_bus_flags;
+ else if (ldev->connector)
+ bus_flags = ldev->connector->display_info.bus_flags;
+
if (!pm_runtime_active(ddev->dev)) {
ret = pm_runtime_get_sync(ddev->dev);
if (ret) {
@@ -586,10 +592,10 @@ static void ltdc_crtc_mode_set_nofb(struct drm_crtc *crtc)
if (vm.flags & DISPLAY_FLAGS_VSYNC_HIGH)
val |= GCR_VSPOL;
- if (vm.flags & DISPLAY_FLAGS_DE_LOW)
+ if (bus_flags & DRM_BUS_FLAG_DE_LOW)
val |= GCR_DEPOL;
- if (vm.flags & DISPLAY_FLAGS_PIXDATA_NEGEDGE)
+ if (bus_flags & DRM_BUS_FLAG_PIXDATA_DRIVE_NEGEDGE)
val |= GCR_PCPOL;
reg_update_bits(ldev->regs, LTDC_GCR,
@@ -1098,6 +1104,8 @@ static const struct drm_encoder_helper_funcs
ltdc_encoder_helper_funcs = {
static int ltdc_encoder_init(struct drm_device *ddev, struct drm_bridge
*bridge)
{
+ struct ltdc_device *ldev = ddev->dev_private;
+ struct drm_connector_list_iter iter;
struct drm_encoder *encoder;
int ret;
@@ -1119,6 +1127,16 @@ static int ltdc_encoder_init(struct drm_device *ddev,
struct drm_bridge *bridge)
return -EINVAL;
}
+ ldev->bridge = bridge;
+
+ /*
+* Get hold of the connector. This is a bit of a hack, until the bridge
+* API gives us bus flags and formats.
+*/
+ drm_connector_list_iter_begin(ddev, &iter);
+ ldev->connector = drm_connector_list_iter_next(&iter);
+ drm_connector_list_iter_end(&iter);
+
DRM_DEBUG_DRIVER("Bridge encoder:%d created\n", encoder->base.id);
return 0;
diff --git a/drivers/gpu/drm/stm/ltdc.h b/drivers/gpu/drm/stm/ltdc.h
index f153b908c70e..d0d2c81de29a 100644
--- a/drivers/gpu/drm/stm/ltdc.h
+++ b/drivers/gpu/drm/stm/ltdc.h
@@ -38,6 +38,8 @@ struct ltdc_device {
u32 irq_status;
struct fps_info plane_fpsi[LTDC_MAX_LAYER];
struct drm_atomic_state *suspend_state;
+ struct drm_bridge *bridge;
+ struct drm_connector *connector;
};
int ltdc_load(struct drm_device *ddev);
--
2.28.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/atomic: Make the kerneldoc a bit clearer
On Fri, Oct 02, 2020 at 02:37:25PM +0200, Daniel Vetter wrote:
> On Fri, Oct 2, 2020 at 2:31 PM Maxime Ripard wrote:
> > On Fri, Oct 02, 2020 at 09:56:20AM +0200, Daniel Vetter wrote:
> > > Crank up the warning a notch and point at the right set of locking
> > > functions for atomic drivers.
> > >
> > > Signed-off-by: Daniel Vetter
> > > Cc: Maarten Lankhorst
> > > Cc: Maxime Ripard
> > > Cc: Thomas Zimmermann
> > > Cc: David Airlie
> > > Cc: Daniel Vetter
> > > ---
> > > drivers/gpu/drm/drm_atomic.c | 10 +-
> > > 1 file changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > > index aac9122f1da2..b2d20eb6c807 100644
> > > --- a/drivers/gpu/drm/drm_atomic.c
> > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > @@ -1642,11 +1642,11 @@ static void __drm_state_dump(struct drm_device
> > > *dev, struct drm_printer *p,
> > > * to dmesg in case of error irq's. (Hint, you probably want to
> > > * ratelimit this!)
> > > *
> > > - * The caller must drm_modeset_lock_all(), or if this is called
> > > - * from error irq handler, it should not be enabled by default.
> > > - * (Ie. if you are debugging errors you might not care that this
> > > - * is racey. But calling this without all modeset locks held is
> > > - * not inherently safe.)
> > > + * The caller must wrap this drm_modeset_lock_all_ctx() and
> > > + * drm_modeset_drop_locks(). If this is called from error irq handler,
> > > it should
> > > + * not be enabled by default - if you are debugging errors you might
> > > + * not care that this is racey, but calling this without all modeset
> > > locks held
> > > + * is inherently unsafe.
> > > */
> > > void drm_state_dump(struct drm_device *dev, struct drm_printer *p)
> > > {
> >
> > For the comment itself:
> > Acked-by: Maxime Ripard
>
> Thanks for taking a look, will merge.
>
> > But maybe we should add some lockdep assertion to make sure we can catch
> > someone actually doing this?
>
> I think it has some use for ad-hoc debugging in random places, or
> maybe as a an opt-in "tains your kernel" debug option. And then you
> really don't want your useful debug output burried in a few pages of
> lockdep splat first :-)
Yeah, but at the same time reducing the discoverability of this locking
expectation for the common case to favor some one-off debugging by
someone that has more chance to know better seems like not the best
trade-off.
Or maybe make the assertion conditional on drm.debug not being set or
something?
Maxime
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[RFC][PATCH] drm/bridge: lvds-codec: Add support for pixel data sampling edge select
The OnSemi FIN3385 Parallel-to-LVDS encoder has a dedicated input line to
select input pixel data sampling edge. Add DT property "pixelclk-active",
same as the one used by display timings, and configure bus flags based on
this DT property.
Signed-off-by: Marek Vasut
Cc: Alexandre Torgue
Cc: Andrzej Hajda
Cc: Antonio Borneo
Cc: Benjamin Gaignard
Cc: Biju Das
Cc: Laurent Pinchart
Cc: Maxime Coquelin
Cc: Philippe Cornu
Cc: Sam Ravnborg
Cc: Vincent Abriou
Cc: Yannick Fertre
Cc: linux-arm-ker...@lists.infradead.org
Cc: linux-st...@st-md-mailman.stormreply.com
To: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/bridge/lvds-codec.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/bridge/lvds-codec.c
b/drivers/gpu/drm/bridge/lvds-codec.c
index f52ccffc1bd1..bc941d4fb5b9 100644
--- a/drivers/gpu/drm/bridge/lvds-codec.c
+++ b/drivers/gpu/drm/bridge/lvds-codec.c
@@ -19,6 +19,7 @@ struct lvds_codec {
struct device *dev;
struct drm_bridge bridge;
struct drm_bridge *panel_bridge;
+ struct drm_bridge_timings timings;
struct regulator *vcc;
struct gpio_desc *powerdown_gpio;
u32 connector_type;
@@ -80,6 +81,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
struct device_node *panel_node;
struct drm_panel *panel;
struct lvds_codec *lvds_codec;
+ u32 val;
int ret;
lvds_codec = devm_kzalloc(dev, sizeof(*lvds_codec), GFP_KERNEL);
@@ -124,6 +126,12 @@ static int lvds_codec_probe(struct platform_device *pdev)
if (IS_ERR(lvds_codec->panel_bridge))
return PTR_ERR(lvds_codec->panel_bridge);
+ if (!of_property_read_u32(dev->of_node, "pixelclk-active", &val)) {
+ lvds_codec->timings.input_bus_flags = val ?
+ DRM_BUS_FLAG_PIXDATA_SAMPLE_NEGEDGE :
+ DRM_BUS_FLAG_PIXDATA_SAMPLE_POSEDGE;
+ }
+
/*
* The panel_bridge bridge is attached to the panel's of_node,
* but we need a bridge attached to our of_node for our user
@@ -131,6 +139,7 @@ static int lvds_codec_probe(struct platform_device *pdev)
*/
lvds_codec->bridge.of_node = dev->of_node;
lvds_codec->bridge.funcs = &funcs;
+ lvds_codec->bridge.timings = &lvds_codec->timings;
drm_bridge_add(&lvds_codec->bridge);
platform_set_drvdata(pdev, lvds_codec);
--
2.28.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH rdma-next v4 1/4] lib/scatterlist: Add support in dynamic allocation of SG table from pages
On Fri, Oct 02, 2020 at 07:11:33PM +0300, Maor Gottlieb wrote:
>
> On 10/2/2020 6:02 PM, Jason Gunthorpe wrote:
> > On Sun, Sep 27, 2020 at 09:46:44AM +0300, Leon Romanovsky wrote:
> > > +struct scatterlist *__sg_alloc_table_from_pages(struct sg_table *sgt,
> > > + struct page **pages, unsigned int n_pages, unsigned int offset,
> > > + unsigned long size, unsigned int max_segment,
> > > + struct scatterlist *prv, unsigned int left_pages,
> > > + gfp_t gfp_mask)
> > > {
> > > - unsigned int chunks, cur_page, seg_len, i;
> > > + unsigned int chunks, cur_page, seg_len, i, prv_len = 0;
> > > + struct scatterlist *s = prv;
> > > + unsigned int table_size;
> > > + unsigned int tmp_nents;
> > > int ret;
> > > - struct scatterlist *s;
> > >
> > > if (WARN_ON(!max_segment || offset_in_page(max_segment)))
> > > - return -EINVAL;
> > > + return ERR_PTR(-EINVAL);
> > > + if (IS_ENABLED(CONFIG_ARCH_NO_SG_CHAIN) && prv)
> > > + return ERR_PTR(-EOPNOTSUPP);
> > > +
> > > + tmp_nents = prv ? sgt->nents : 0;
> > > +
> > > + if (prv &&
> > > + page_to_pfn(sg_page(prv)) + (prv->length >> PAGE_SHIFT) ==
> > This calculation of the end doesn't consider sg->offset
>
> Right, should be fixed.
> >
> > > + page_to_pfn(pages[0]))
> > > + prv_len = prv->length;
> > >
> > > /* compute number of contiguous chunks */
> > > chunks = 1;
> > > @@ -410,13 +461,17 @@ int __sg_alloc_table_from_pages(struct sg_table
> > > *sgt, struct page **pages,
> > > }
> > > }
> > >
> > > - ret = sg_alloc_table(sgt, chunks, gfp_mask);
> > > - if (unlikely(ret))
> > > - return ret;
> > > + if (!prv) {
> > > + /* Only the last allocation could be less than the maximum */
> > > + table_size = left_pages ? SG_MAX_SINGLE_ALLOC : chunks;
> > > + ret = sg_alloc_table(sgt, table_size, gfp_mask);
> > > + if (unlikely(ret))
> > > + return ERR_PTR(ret);
> > > + }
> > This is basically redundant right? Now that get_next_sg() can allocate
> > SGs it can just build them one by one, no need to preallocate.
> >
> > Actually all the changes the the allocation seem like overkill, just
> > allocate a single new array directly in get_next_sg() whenever it
> > needs.
>
> No, only the last allocation could be less than maximum. (as written in the
> comment).
The point is that get_next_sg is fully redundent with
sg_alloc_table() because it is always used in cases when prv is
set. There is zero reason to call sg_alloc_table here in the one case
where prv is not set.
Further this cleans up the spagehtti goto in the middle of the for
loop and avoids allocating an extra chunk if the page list fully fits
in prv.
Given how much smaller it is I think you should look more carefully.
Jason
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH rdma-next v4 1/4] lib/scatterlist: Add support in dynamic allocation of SG table from pages
On 10/2/2020 6:02 PM, Jason Gunthorpe wrote:
On Sun, Sep 27, 2020 at 09:46:44AM +0300, Leon Romanovsky wrote:
+struct scatterlist *__sg_alloc_table_from_pages(struct sg_table *sgt,
+ struct page **pages, unsigned int n_pages, unsigned int offset,
+ unsigned long size, unsigned int max_segment,
+ struct scatterlist *prv, unsigned int left_pages,
+ gfp_t gfp_mask)
{
- unsigned int chunks, cur_page, seg_len, i;
+ unsigned int chunks, cur_page, seg_len, i, prv_len = 0;
+ struct scatterlist *s = prv;
+ unsigned int table_size;
+ unsigned int tmp_nents;
int ret;
- struct scatterlist *s;
if (WARN_ON(!max_segment || offset_in_page(max_segment)))
- return -EINVAL;
+ return ERR_PTR(-EINVAL);
+ if (IS_ENABLED(CONFIG_ARCH_NO_SG_CHAIN) && prv)
+ return ERR_PTR(-EOPNOTSUPP);
+
+ tmp_nents = prv ? sgt->nents : 0;
+
+ if (prv &&
+ page_to_pfn(sg_page(prv)) + (prv->length >> PAGE_SHIFT) ==
This calculation of the end doesn't consider sg->offset
Right, should be fixed.
+ page_to_pfn(pages[0]))
+ prv_len = prv->length;
/* compute number of contiguous chunks */
chunks = 1;
@@ -410,13 +461,17 @@ int __sg_alloc_table_from_pages(struct sg_table *sgt,
struct page **pages,
}
}
- ret = sg_alloc_table(sgt, chunks, gfp_mask);
- if (unlikely(ret))
- return ret;
+ if (!prv) {
+ /* Only the last allocation could be less than the maximum */
+ table_size = left_pages ? SG_MAX_SINGLE_ALLOC : chunks;
+ ret = sg_alloc_table(sgt, table_size, gfp_mask);
+ if (unlikely(ret))
+ return ERR_PTR(ret);
+ }
This is basically redundant right? Now that get_next_sg() can allocate
SGs it can just build them one by one, no need to preallocate.
Actually all the changes the the allocation seem like overkill, just
allocate a single new array directly in get_next_sg() whenever it
needs.
No, only the last allocation could be less than maximum. (as written in
the comment).
I am preferring to stick with the current implementation and fix the offset.
Something like this:
@@ -365,6 +372,37 @@ int sg_alloc_table(struct sg_table *table, unsigned int
nents, gfp_t gfp_mask)
}
EXPORT_SYMBOL(sg_alloc_table);
+static struct scatterlist *get_next_sg(struct sg_table *table,
+ struct scatterlist *cur, unsigned long needed_sges,
+ gfp_t gfp_mask)
+{
+ struct scatterlist *new_sg;
+ unsigned int alloc_size;
+
+ if (cur) {
+ struct scatterlist *next_sg = sg_next(cur);
+
+ /* Check if last entry should be keeped for chainning */
+ if (!sg_is_last(next_sg) || needed_sges == 1)
+ return next_sg;
+ }
+
+ alloc_size = min_t(unsigned long, needed_sges, SG_MAX_SINGLE_ALLOC);
+ new_sg = sg_kmalloc(alloc_size, gfp_mask);
+ if (!new_sg)
+ return ERR_PTR(-ENOMEM);
+ sg_init_table(new_sg, alloc_size);
+ if (cur) {
+ __sg_chain(cur, new_sg);
+ table->orig_nents += alloc_size - 1;
+ } else {
+ table->sgl = new_sg;
+ table->orig_nents = alloc_size;
+ table->nents = 0;
+ }
+ return new_sg;
+}
+
/**
* __sg_alloc_table_from_pages - Allocate and initialize an sg table from
* an array of pages
@@ -374,29 +412,64 @@ EXPORT_SYMBOL(sg_alloc_table);
* @offset: Offset from start of the first page to the start of a buffer
* @size:Number of valid bytes in the buffer (after offset)
* @max_segment: Maximum size of a scatterlist node in bytes (page aligned)
+ * @prv:Last populated sge in sgt
+ * @left_pages: Left pages caller have to set after this call
* @gfp_mask: GFP allocation mask
*
- * Description:
- *Allocate and initialize an sg table from a list of pages. Contiguous
- *ranges of the pages are squashed into a single scatterlist node up to the
- *maximum size specified in @max_segment. An user may provide an offset at
a
- *start and a size of valid data in a buffer specified by the page array.
- *The returned sg table is released by sg_free_table.
+ * Description:
+ *If @prv is NULL, allocate and initialize an sg table from a list of
pages,
+ *else reuse the scatterlist passed in at @prv.
+ *Contiguous ranges of the pages are squashed into a single scatterlist
+ *entry up to the maximum size specified in @max_segment. A user may
+ *provide an offset at a start and a size of valid data in a buffer
+ *specified by the page array.
*
* Returns:
- * 0 on success, negative error on failure
+ * Last SGE in sgt on success, PTR_ERR on otherwise.
+ *
Re: [PATCH v2 0/3] drm: commit_work scheduling
On 09/30/20 14:17, Rob Clark wrote: > From: Rob Clark > > The android userspace treats the display pipeline as a realtime problem. > And arguably, if your goal is to not miss frame deadlines (ie. vblank), > it is. (See https://lwn.net/Articles/809545/ for the best explaination > that I found.) > > But this presents a problem with using workqueues for non-blocking > atomic commit_work(), because the SCHED_FIFO userspace thread(s) can > preempt the worker. Which is not really the outcome you want.. once > the required fences are scheduled, you want to push the atomic commit > down to hw ASAP. For me thees 2 properties 1. Run ASAP 2. Finish the work un-interrupted Scream the workers need to be SCHED_FIFO by default. CFS can't give you these guarantees. IMO using sched_set_fifo() for these workers is the right thing. > > But the decision of whether commit_work should be RT or not really > depends on what userspace is doing. For a pure CFS userspace display > pipeline, commit_work() should remain SCHED_NORMAL. I'm not sure I agree with this. I think it's better to characterize tasks based on their properties/requirements rather than what the rest of the userspace is using. I do appreciate that maybe some of these tasks have varying requirements during their life time. e.g: they have RT property during specific critical section but otherwise are CFS tasks. I think the UI thread in Android behaves like that. It's worth IMO trying that approach I pointed out earlier to see if making RT try to pick an idle CPU rather than preempt CFS helps. Not sure if it'd be accepted but IMHO it's a better direction to consider and discuss. Or maybe you can wrap userspace pipeline critical section lock such that any task holding it will automatically be promoted to SCHED_FIFO and then demoted to CFS once it releases it. Haven't worked with display pipelines before, so hopefully this makes sense :-) Thanks -- Qais Yousef > > To handle this, convert non-blocking commit_work() to use per-CRTC > kthread workers, instead of system_unbound_wq. Per-CRTC workers are > used to avoid serializing commits when userspace is using a per-CRTC > update loop. And the last patch exposes the task id to userspace as > a CRTC property, so that userspace can adjust the priority and sched > policy to fit it's needs. > > > v2: Drop client cap and in-kernel setting of priority/policy in > favor of exposing the kworker tid to userspace so that user- > space can set priority/policy. > > Rob Clark (3): > drm/crtc: Introduce per-crtc kworker > drm/atomic: Use kthread worker for nonblocking commits > drm: Expose CRTC's kworker task id > > drivers/gpu/drm/drm_atomic_helper.c | 13 > drivers/gpu/drm/drm_crtc.c | 14 + > drivers/gpu/drm/drm_mode_config.c | 14 + > drivers/gpu/drm/drm_mode_object.c | 4 > include/drm/drm_atomic.h| 31 + > include/drm/drm_crtc.h | 8 > include/drm/drm_mode_config.h | 9 + > include/drm/drm_property.h | 9 + > 8 files changed, 98 insertions(+), 4 deletions(-) > > -- > 2.26.2 > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 2/2] mm/frame-vec: use FOLL_LONGTERM
On Sat, Oct 3, 2020 at 2:31 AM Jason Gunthorpe wrote:
>
> On Fri, Oct 02, 2020 at 08:16:48PM +0200, Daniel Vetter wrote:
> > On Fri, Oct 2, 2020 at 8:06 PM Jason Gunthorpe wrote:
> > > On Fri, Oct 02, 2020 at 07:53:03PM +0200, Daniel Vetter wrote:
> > > > For $reasons I've stumbled over this code and I'm not sure the change
> > > > to the new gup functions in 55a650c35fea ("mm/gup: frame_vector:
> > > > convert get_user_pages() --> pin_user_pages()") was entirely correct.
> > > >
> > > > This here is used for long term buffers (not just quick I/O) like
> > > > RDMA, and John notes this in his patch. But I thought the rule for
> > > > these is that they need to add FOLL_LONGTERM, which John's patch
> > > > didn't do.
> > > >
> > > > There is already a dax specific check (added in b7f0554a56f2 ("mm:
> > > > fail get_vaddr_frames() for filesystem-dax mappings")), so this seems
> > > > like the prudent thing to do.
> > > >
> > > > Signed-off-by: Daniel Vetter
> > > > Cc: Andrew Morton
> > > > Cc: John Hubbard
> > > > Cc: Jérôme Glisse
> > > > Cc: Jan Kara
> > > > Cc: Dan Williams
> > > > Cc: linux...@kvack.org
> > > > Cc: linux-arm-ker...@lists.infradead.org
> > > > Cc: linux-samsung-...@vger.kernel.org
> > > > Cc: linux-me...@vger.kernel.org
> > > > Hi all,
> > > >
> > > > I stumbled over this and figured typing this patch can't hurt. Really
> > > > just to maybe learn a few things about how gup/pup is supposed to be
> > > > used (we have a bit of that in drivers/gpu), this here isn't really
> > > > ralated to anything I'm doing.
> > >
> > > FOLL_FORCE is a pretty big clue it should be FOLL_LONGTERM, IMHO
> >
> > Since you're here ... I've noticed that ib sets FOLL_FORCE when the ib
> > verb access mode indicates possible writes. I'm not really clear on
> > why FOLL_WRITE isn't enough any why you need to be able to write
> > through a vma that's write protected currently.
>
> Ah, FOLL_FORCE | FOLL_WRITE means *read* confusingly enough, and the
> only reason you'd want this version for read is if you are doing
> longterm stuff. I wrote about this recently:
>
> https://lore.kernel.org/linux-mm/20200928235739.gu9...@ziepe.ca/
>
> > > Since every driver does this wrong anything that uses this is creating
> > > terrifying security issues.
> > >
> > > IMHO this whole API should be deleted :(
> >
> > Yeah that part I just tried to conveniently ignore. I guess this dates
> > back to a time when ioremaps where at best fixed, and there wasn't
> > anything like a gpu driver dynamically managing vram around, resulting
> > in random entirely unrelated things possibly being mapped to that set
> > of pfns.
>
> No, it was always wrong. Prior to GPU like cases the lifetime of the
> PTE was tied to the vma and when the vma becomes free the driver can
> move the things in the PTEs to 'free'. Easy to trigger use-after-free
> issues and devices like RDMA have security contexts attached to these
> PTEs so it becomes a serious security bug to do something like this.
>
> > The underlying follow_pfn is also used in other places within
> > drivers/media, so this doesn't seem to be an accident, but actually
> > intentional.
>
> Looking closely, there are very few users, most *seem* pointless, but
> maybe there is a crazy reason?
>
> The sequence
> get_vaddr_frames();
> frame_vector_to_pages();
> sg_alloc_table_from_pages();
>
> Should be written
> pin_user_pages_fast(FOLL_LONGTERM);
> sg_alloc_table_from_pages()
>
> There is some 'special' code in frame_vector_to_pages() that tries to
> get a struct page for things from a VM_IO or VM_PFNMAP...
>
> Oh snap, that is *completely* broken! If the first VMA is IO|PFNMAP
> then get_vaddr_frames() iterates over all VMAs in the range, of any
> kind and extracts the PTEs then blindly references them! This means it
> can be used to use after free normal RAM struct pages!! Gah!
>
> Wow. Okay. That has to go.
>
> So, I *think* we can assume there is no sane cases where
> frame_vector_to_pages() succeeds but pin_user_pages() wasn't called.
>
> That means the users here:
> - habanalabs: Hey Oded can you fix this up?
Yes, no problem, I'll do it very soon.
Thanks,
Oded
>
> - gpu/exynos: Daniel can you get someone there to stop using it?
>
> - media/videobuf via vb2_dma_sg_get_userptr()
>
> Should all be switched to the standard pin_user_pages sequence
> above.
>
> That leaves the only interesting places as vb2_dc_get_userptr() and
> vb2_vmalloc_get_userptr() which both completely fail to follow the
> REQUIRED behavior in the function's comment about checking PTEs. It
> just DMA maps them. Badly broken.
>
> Guessing this hackery is for some embedded P2P DMA transfer?
>
> After he three places above should use pin_user_pages_fast(), then
> this whole broken API should be moved into videobuf2-memops.c and a
> big fat "THIS DOESN'T WORK" stuck on it.
>
> videobuf2 should probably use P2P DMA buf for this instead.
>
> Jason
___
dri-devel mai
Re: [PATCH v3 2/2] tty/sysrq: Add configurable handler to execute a compound action
On Fri, Oct 02, 2020 at 05:06:54PM +0200, Andrzej Pietrasiewicz wrote: > Hi, > > W dniu 02.10.2020 o 16:02, Greg Kroah-Hartman pisze: > > On Fri, Oct 02, 2020 at 03:42:52PM +0200, Andrzej Pietrasiewicz wrote: > > > Hi, > > > > > > W dniu 02.10.2020 o 14:54, Greg Kroah-Hartman pisze: > > > > On Tue, Aug 18, 2020 at 01:28:25PM +0200, Andrzej Pietrasiewicz wrote: > > > > > Userland might want to execute e.g. 'w' (show blocked tasks), followed > > > > > by 's' (sync), followed by 1000 ms delay and then followed by 'c' > > > > > (crash) > > > > > upon a single magic SysRq. Or one might want to execute the famous > > > > > "Raising > > > > > Elephants Is So Utterly Boring" action. This patch adds a configurable > > > > > handler, triggered with 'C', for this exact purpose. The user > > > > > specifies the > > > > > composition of the compound action using syntax similar to getopt, > > > > > where > > > > > each letter corresponds to an individual action and a colon followed > > > > > by a > > > > > number corresponds to a delay of that many milliseconds, e.g.: > > > > > > > > > > ws:1000c > > > > > > > > > > or > > > > > > > > > > r:100eis:1000ub > > > > > > > > A macro language for sysrq commands, who would have thought... > > > > > > > > Anyway, _why_ would userland want to do something so crazy as this? > > > > What is the use-case here? > > > > > > > > > > A use-case is Chromebooks which do want to execute 'w', 's', > > > wait 1000ms and then 'c' under one key combination. Having that supported > > > upstream brings us one little step closer to those machines running > > > upstream kernel. > > > > Who is causing that to "execute"? Some daemon/program? > > No, as far as I know they patch the kernel to change the behavior > of Sysrq-x combination, so the "execution" is triggered by the user. So this isn't coming from the chromeos team, so there is no guarantee that they will switch to this if it is merged? > > > Another argument for such a "macro language" is when a machine's system > > > keeps degrading over time, possibly degrading (relatively) fast. > > > "Raising Elephants Is So Utterly Boring" consists of 6 actions, each > > > of which requires pressing several keys. The user might be unable > > > to complete all the 6 steps, while a "macro" requires user's involvement > > > for carrying out just one step. > > > > So you want to "preload" some commands ahead of time, for when you get > > in trouble > It can be said this way, yes. > > > > > These should just be debugging / last resort types of things, how > > regular are they being used in your systems? > > > > The "REISUB" itself is a kind of a last resort thing. > > It is true that it's not a very frequent situation, but does its being rare > preclude having such a function in the kernel? > > While preparing this patch I wanted it to be flexible, but perhaps it is > too flexible for some reason? If the permissions of the module_param's > sysfs entry were changed to 0444 would it be better? Then the compound > action would still be configurable but only at boot time rather than at > boot time _and_ runtime. I don't have an issue with it happening at runtime and boot time, just that this is adding additional complexity to the kernel (parsers are fun!) for no real-world user. thanks, greg k-h ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] dt-bindings: Another round of adding missing 'additionalProperties'
On Fri, Oct 02, 2020 at 06:41:43PM -0500, Rob Herring wrote: > Another round of wack-a-mole. The json-schema default is additional > unknown properties are allowed, but for DT all properties should be > defined. > > Cc: Thierry Reding > Cc: Linus Walleij > Cc: Stephen Boyd > Cc: Shawn Guo > Cc: Bjorn Andersson > Cc: Baolin Wang > Cc: Guenter Roeck > Cc: Jonathan Cameron > Cc: Mauro Carvalho Chehab > Cc: Laurent Pinchart > Cc: Lee Jones > Cc: Ulf Hansson > Cc: "David S. Miller" > Cc: Bjorn Helgaas > Cc: Vinod Koul > Cc: Liam Girdwood > Cc: Mark Brown > Cc: Greg Kroah-Hartman > Cc: Daniel Lezcano > Cc: linux-...@vger.kernel.org > Cc: dri-devel@lists.freedesktop.org > Cc: linux-...@vger.kernel.org > Cc: linux-g...@vger.kernel.org > Cc: linux-hw...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: openipmi-develo...@lists.sourceforge.net > Cc: linux-l...@vger.kernel.org > Cc: linux-me...@vger.kernel.org > Cc: linux-rockc...@lists.infradead.org > Cc: linux-st...@st-md-mailman.stormreply.com > Cc: linux-m...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: net...@vger.kernel.org > Cc: linux-...@vger.kernel.org > Cc: linux...@vger.kernel.org > Cc: linux-remotep...@vger.kernel.org > Cc: linux-ser...@vger.kernel.org > Cc: alsa-de...@alsa-project.org > Cc: linux-...@vger.kernel.org > Signed-off-by: Rob Herring > --- > > I'll take this thru the DT tree. For USB: Reviewed-by: Greg Kroah-Hartman ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
