dri-devel July 2021

dri-devel@lists.freedesktop.org

321 participants
829 discussions

Re: [PATCH v2 09/14] vfio/pci: Change vfio_pci_try_bus_reset() to use the dev_set
by Jason Gunthorpe 23 Jul '21

23 Jul '21

On Fri, Jul 23, 2021 at 10:05:43AM +0200, Christoph Hellwig wrote: > On Tue, Jul 20, 2021 at 02:42:55PM -0300, Jason Gunthorpe wrote: > > Keep track of all the vfio_devices that have been added to the device set > > and use this list in vfio_pci_try_bus_reset() instead of trying to work > > backwards from the pci_device. > > > > The dev_set->lock directly prevents devices from joining/leaving the set, > > which further implies the pci_device cannot change drivers or that the > > vfio_device be freed, eliminating the need for get/put's. > > > > Completeness of the device set can be directly measured by checking if > > every PCI device in the reset group is also in the device set - which > > proves that VFIO drivers are attached to everything. > > > > This restructuring corrects a call to pci_dev_driver() without holding the > > device_lock() and removes a hard wiring to &vfio_pci_driver. > > > > Signed-off-by: Jason Gunthorpe <jgg(a)nvidia.com> > > I think the addition of the list to the dev_set should be a different > patch. Or maybe even go into the one adding the dev_set concept. OK > > +static int vfio_pci_check_all_devices_bound(struct pci_dev *pdev, void *data) > > { > > + struct vfio_device_set *dev_set = data; > > + struct vfio_device *cur; > > > > + lockdep_assert_held(&dev_set->lock); > > > > + list_for_each_entry(cur, &dev_set->device_list, dev_set_list) > > + if (cur->dev == &pdev->dev) > > + return 0; > > + return -EBUSY; > > I don't understand this logic. If there is any device in the set that > does now have the same struct device we're in trouble? Please clearly > document what this is trying to do. If the bound in the name makes sense > you probably want to check the driver instead. The PCI reset this code is tring to do effects a set of PCI devices, due to how the HW works. Along with the vfio_pci_for_each_slot_or_bus() this is computing a set wise 'is superset' between the list of pci_dev's the reset will affect (the reset group) and the list of vfio_devices that we have locking control over to sequence the reset (the dev_set). If every PCI device we will reset is under the dev_set then we directly know it is safe to trigger the reset. If any PCI device is not in this dev_set then we cannot use the reset as we can't know what will happen to the device that we don't control. Let's use a different word than bound? vfio_pci_check_device_in_set()? > > static void vfio_pci_try_bus_reset(struct vfio_pci_device *vdev) > > { > > + /* All VFIO devices have a closed FD */ > > + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) > > + if (cur->vdev.open_count) > > + return; > > + > > + /* All devices in the group to be reset need VFIO devices */ > > + if (vfio_pci_for_each_slot_or_bus( > > + vdev->pdev, vfio_pci_check_all_devices_bound, dev_set, > > + !pci_probe_reset_slot(vdev->pdev->slot))) > > + return; > > > > /* Does at least one need a reset? */ > > These checks look a little strange, and the comments don't make much > sense. What about an incremental patch like this? Sure, it can go in a function > diff --git a/drivers/vfio/pci/vfio_pci.c b/drivers/vfio/pci/vfio_pci.c > index fbc20f6d2dd412..d8375a5e77e07c 100644 > +++ b/drivers/vfio/pci/vfio_pci.c > @@ -2188,10 +2188,34 @@ static int vfio_pci_try_zap_and_vma_lock_cb(struct pci_dev *pdev, void *data) > return 0; > } > > +static struct pci_dev *vfio_pci_reset_target(struct vfio_pci_device *vdev) > +{ > + struct vfio_device_set *dev_set = vdev->vdev.dev_set; > + struct vfio_pci_device *cur; > + > + /* none of the device is allowed to be currently open: */ > + list_for_each_entry(cur, &dev_set->device_list, vdev.dev_set_list) > + if (cur->vdev.open_count) > + return NULL; > + > + /* all devices in the group to be reset need to be VFIO devices: */ It is not "need to be VFIO devices" it is "need to be in our dev_set". Have the PCI dev bound to, say, a mdev VFIO device isn't good enough. Thanks, Jason

1 0

Re: [PATCH v2 06/14] vfio/fsl: Move to the device set infrastructure
by Jason Gunthorpe 23 Jul '21

23 Jul '21

On Fri, Jul 23, 2021 at 02:29:03PM +0200, Christoph Hellwig wrote: > On Fri, Jul 23, 2021 at 09:22:27AM -0300, Jason Gunthorpe wrote: > > > But do we even need the else part? Assingning &mc_dev->dev is > > > equivalent to the default per-device set anyway, isn't it? > > > > Not quite, the default is this: > > > > if (!device->dev_set) > > vfio_assign_device_set(device, device); > > > > Where 'device' is the vfio_device itself, the above is connecting to > > the struct fsl_mc_device. > > Isn't there a 1:1 relation? Yes, but one is a struct device * and the other is a struct vfio_device *. They don't have the same pointer value. The above default code has the goal of creating a singleton dev_set because no other driver can reasonably obtain the 'struct vfio_device *'. FSL is using a 'struct device *' as the key and the interesting case is when two drivers are loaded such that: mc_dev->dev.parent == &mc_dev->dev Then they will have the same dev_set and the locking works out. The vfio_device * can never be == &mc_dev->dev because it is a different allocation. Jason

1 0

Re: [PATCH v2 08/14] vfio/pci: Move to the device set infrastructure
by Jason Gunthorpe 23 Jul '21

23 Jul '21

On Fri, Jul 23, 2021 at 09:47:49AM +0200, Christoph Hellwig wrote: > > @@ -2020,12 +2004,17 @@ static int vfio_pci_probe(struct pci_dev *pdev, const struct pci_device_id *id) > > INIT_LIST_HEAD(&vdev->vma_list); > > init_rwsem(&vdev->memory_lock); > > > > - ret = vfio_pci_reflck_attach(vdev); > > + if (pci_is_root_bus(pdev->bus)) > > + ret = vfio_assign_device_set(&vdev->vdev, vdev); > > + else if (!pci_probe_reset_slot(pdev->slot)) > > + ret = vfio_assign_device_set(&vdev->vdev, pdev->slot); > > + else > > + ret = vfio_assign_device_set(&vdev->vdev, pdev->bus); > > Maybe invert this and add a comment: > > if (pci_is_root_bus(pdev->bus)) > ret = vfio_assign_device_set(&vdev->vdev, vdev); > /* > * If there is no slot reset support for this device, the whole > * bus needs to be grouped together to support bus-wide resets. > */ I like the comment > else if (pci_probe_reset_slot(pdev->slot) < 0) > ret = vfio_assign_device_set(&vdev->vdev, pdev->bus); > else > ret = vfio_assign_device_set(&vdev->vdev, pdev->slot); The consensus idiom here is the !: drivers/pci/pci.c: return (!pci_probe_reset_slot(pdev->slot)) ? drivers/vfio/pci/vfio_pci.c: if (!pci_probe_reset_slot(vdev->pdev->slot)) drivers/vfio/pci/vfio_pci.c: if (!pci_probe_reset_slot(vdev->pdev->slot)) drivers/vfio/pci/vfio_pci.c: bool slot = !pci_probe_reset_slot(vdev->pdev->slot); drivers/vfio/pci/vfio_pci.c: if (!pci_probe_reset_slot(vdev->pdev->slot)) It reads quite poorly either way, IMHO, I'd rather switch it to a readable bool, but not for this series Thanks, Jason

1 0

[PATCH] backlight: pwm_bl: Avoid backlight flicker if backlight control GPIO is input
by Marek Vasut 23 Jul '21

23 Jul '21

If the backlight enable GPIO is configured as input, the driver currently unconditionally forces the GPIO to output-enable. This can cause backlight flicker on boot e.g. in case the GPIO should not be enabled before the PWM is configured and is correctly pulled low by external resistor. Fix this by extending the current check to differentiate between backlight GPIO enable set as input and set as direction unknown. In case of input, read the GPIO value to determine the pull resistor placement, set the GPIO as output, and drive that exact value it was pulled to. In case of unknown direction, retain previous behavior, that is set the GPIO as output-enable. Fixes: 3698d7e7d221 ("backlight: pwm_bl: Avoid backlight flicker when probed from DT") Signed-off-by: Marek Vasut <marex(a)denx.de> Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> Cc: Daniel Thompson <daniel.thompson(a)linaro.org> Cc: Heiko Stuebner <heiko(a)sntech.de> Cc: Philipp Zabel <p.zabel(a)pengutronix.de> Cc: Thierry Reding <treding(a)nvidia.com> Cc: linux-pwm(a)vger.kernel.org Cc: linux-fbdev(a)vger.kernel.org To: dri-devel(a)lists.freedesktop.org --- NOTE: I think this whole auto-detection scheme should just be replaced by a DT prop, because it is very fragile. --- drivers/video/backlight/pwm_bl.c | 35 +++++++++++++++++++++++--------- 1 file changed, 25 insertions(+), 10 deletions(-) diff --git a/drivers/video/backlight/pwm_bl.c b/drivers/video/backlight/pwm_bl.c index e48fded3e414..7ec992b722eb 100644 --- a/drivers/video/backlight/pwm_bl.c +++ b/drivers/video/backlight/pwm_bl.c @@ -445,7 +445,7 @@ static int pwm_backlight_probe(struct platform_device *pdev) struct device_node *node = pdev->dev.of_node; struct pwm_bl_data *pb; struct pwm_state state; - unsigned int i; + unsigned int i, dir, val; int ret; if (!data) { @@ -487,16 +487,31 @@ static int pwm_backlight_probe(struct platform_device *pdev) } /* - * If the GPIO is not known to be already configured as output, that - * is, if gpiod_get_direction returns either 1 or -EINVAL, change the - * direction to output and set the GPIO as active. - * Do not force the GPIO to active when it was already output as it - * could cause backlight flickering or we would enable the backlight too - * early. Leave the decision of the initial backlight state for later. + * If the GPIO is not known to be already configured as output, then: + * - if the GPIO direction is input, read its current value to find out + * whether the pin is pulled high or low (it is backlight control, so + * it cannot be floating), change the direction to output and set the + * GPIO such that it drives this strapped value. + * Do not force the GPIO to state which is different than that to + * which the GPIO was pulled to, this could cause backlight flicker + * on boot e.g. in case the PWM is not ready yet. + * - if the GPIO direction is unknown, tahat is, if gpiod_get_direction + * returns -EINVAL, change the direction to output and set the GPIO + * as active. + * Do not force the GPIO to active when it was already output as it + * could cause backlight flickering or we would enable the backlight + * too early. Leave the decision of the initial backlight state for + * later. */ - if (pb->enable_gpio && - gpiod_get_direction(pb->enable_gpio) != 0) - gpiod_direction_output(pb->enable_gpio, 1); + if (pb->enable_gpio) { + dir = gpiod_get_direction(pb->enable_gpio); + if (dir != 0) { + val = 1; + if (dir == 1) + val = gpiod_get_value_cansleep(pb->enable_gpio); + gpiod_direction_output(pb->enable_gpio, val); + } + } pb->power_supply = devm_regulator_get(&pdev->dev, "power"); if (IS_ERR(pb->power_supply)) { -- 2.30.2

2 14

Re: [PATCH v2 07/14] vfio/platform: Use open_device() instead of open coding a refcnt scheme
by Jason Gunthorpe 23 Jul '21

23 Jul '21

On Fri, Jul 23, 2021 at 09:45:21AM +0200, Christoph Hellwig wrote: > Looks good, > > Reviewed-by: Christoph Hellwig <hch(a)lst.de> > > On Tue, Jul 20, 2021 at 02:42:53PM -0300, Jason Gunthorpe wrote: > > Platform simply wants to run some code when the device is first > > opened/last closed. Use the core framework and locking for this. Aside > > from removing a bit of code this narrows the locking scope from a global > > lock. > > > > Signed-off-by: Yishai Hadas <yishaih(a)nvidia.com> > > Btw, this seems to miss the second from line for Yishai, which should > match the first signoff. Hum, I think we ended up co-developing this on, I can reorder the signed off's Jason

1 0

Re: [PATCH v2 06/14] vfio/fsl: Move to the device set infrastructure
by Jason Gunthorpe 23 Jul '21

23 Jul '21

On Fri, Jul 23, 2021 at 09:44:35AM +0200, Christoph Hellwig wrote: > On Tue, Jul 20, 2021 at 02:42:52PM -0300, Jason Gunthorpe wrote: > > .write = vfio_fsl_mc_write, > > @@ -625,13 +526,15 @@ static int vfio_fsl_mc_probe(struct fsl_mc_device *mc_dev) > > vdev->mc_dev = mc_dev; > > mutex_init(&vdev->igate); > > > > + ret = vfio_assign_device_set(&vdev->vdev, is_fsl_mc_bus_dprc(mc_dev) ? > > + &mc_dev->dev : > > + mc_dev->dev.parent); > > A good old if/else would be much cleaner here. Ok > But do we even need the else part? Assingning &mc_dev->dev is > equivalent to the default per-device set anyway, isn't it? Not quite, the default is this: if (!device->dev_set) vfio_assign_device_set(device, device); Where 'device' is the vfio_device itself, the above is connecting to the struct fsl_mc_device. Thanks, Jason

1 0

[PATCH v6] Documentation: gpu: Mention the requirements for new properties
by Maxime Ripard 23 Jul '21

23 Jul '21

New KMS properties come with a bunch of requirements to avoid each driver from running their own, inconsistent, set of properties, eventually leading to issues like property conflicts, inconsistencies between drivers and semantics, etc. Let's document what we expect. Cc: Alexandre Belloni <alexandre.belloni(a)bootlin.com> Cc: Alexandre Torgue <alexandre.torgue(a)foss.st.com> Cc: Alex Deucher <alexander.deucher(a)amd.com> Cc: Alison Wang <alison.wang(a)nxp.com> Cc: Alyssa Rosenzweig <alyssa.rosenzweig(a)collabora.com> Cc: Andrew Jeffery <andrew(a)aj.id.au> Cc: Andrzej Hajda <a.hajda(a)samsung.com> Cc: Anitha Chrisanthus <anitha.chrisanthus(a)intel.com> Cc: Benjamin Gaignard <benjamin.gaignard(a)linaro.org> Cc: Ben Skeggs <bskeggs(a)redhat.com> Cc: Boris Brezillon <bbrezillon(a)kernel.org> Cc: Brian Starkey <brian.starkey(a)arm.com> Cc: Chen Feng <puck.chen(a)hisilicon.com> Cc: Chen-Yu Tsai <wens(a)csie.org> Cc: Christian Gmeiner <christian.gmeiner(a)gmail.com> Cc: "Christian König" <christian.koenig(a)amd.com> Cc: Chun-Kuang Hu <chunkuang.hu(a)kernel.org> Cc: Edmund Dea <edmund.j.dea(a)intel.com> Cc: Eric Anholt <eric(a)anholt.net> Cc: Fabio Estevam <festevam(a)gmail.com> Cc: Gerd Hoffmann <kraxel(a)redhat.com> Cc: Haneen Mohammed <hamohammed.sa(a)gmail.com> Cc: Hans de Goede <hdegoede(a)redhat.com> Cc: "Heiko Stübner" <heiko(a)sntech.de> Cc: Huang Rui <ray.huang(a)amd.com> Cc: Hyun Kwon <hyun.kwon(a)xilinx.com> Cc: Inki Dae <inki.dae(a)samsung.com> Cc: Jani Nikula <jani.nikula(a)linux.intel.com> Cc: Jernej Skrabec <jernej.skrabec(a)siol.net> Cc: Jerome Brunet <jbrunet(a)baylibre.com> Cc: John Stultz <john.stultz(a)linaro.org> Cc: Jonas Karlman <jonas(a)kwiboo.se> Cc: Jonathan Hunter <jonathanh(a)nvidia.com> Cc: Joonas Lahtinen <joonas.lahtinen(a)linux.intel.com> Cc: Joonyoung Shim <jy0922.shim(a)samsung.com> Cc: Jyri Sarha <jyri.sarha(a)iki.fi> Cc: Kevin Hilman <khilman(a)baylibre.com> Cc: Kieran Bingham <kieran.bingham+renesas(a)ideasonboard.com> Cc: Krzysztof Kozlowski <krzysztof.kozlowski(a)canonical.com> Cc: Kyungmin Park <kyungmin.park(a)samsung.com> Cc: Laurent Pinchart <Laurent.pinchart(a)ideasonboard.com> Cc: Linus Walleij <linus.walleij(a)linaro.org> Cc: Liviu Dudau <liviu.dudau(a)arm.com> Cc: Lucas Stach <l.stach(a)pengutronix.de> Cc: Ludovic Desroches <ludovic.desroches(a)microchip.com> Cc: Marek Vasut <marex(a)denx.de> Cc: Martin Blumenstingl <martin.blumenstingl(a)googlemail.com> Cc: Matthias Brugger <matthias.bgg(a)gmail.com> Cc: Maxime Coquelin <mcoquelin.stm32(a)gmail.com> Cc: Maxime Ripard <mripard(a)kernel.org> Cc: Melissa Wen <melissa.srw(a)gmail.com> Cc: Neil Armstrong <narmstrong(a)baylibre.com> Cc: Nicolas Ferre <nicolas.ferre(a)microchip.com> Cc: "Noralf Trønnes" <noralf(a)tronnes.org> Cc: NXP Linux Team <linux-imx(a)nxp.com> Cc: Oleksandr Andrushchenko <oleksandr_andrushchenko(a)epam.com> Cc: Patrik Jakobsson <patrik.r.jakobsson(a)gmail.com> Cc: Paul Cercueil <paul(a)crapouillou.net> Cc: Pekka Paalanen <pekka.paalanen(a)collabora.com> Cc: Pengutronix Kernel Team <kernel(a)pengutronix.de> Cc: Philippe Cornu <philippe.cornu(a)foss.st.com> Cc: Philipp Zabel <p.zabel(a)pengutronix.de> Cc: Qiang Yu <yuq825(a)gmail.com> Cc: Rob Clark <robdclark(a)gmail.com> Cc: Robert Foss <robert.foss(a)linaro.org> Cc: Rob Herring <robh(a)kernel.org> Cc: Rodrigo Siqueira <rodrigosiqueiramelo(a)gmail.com> Cc: Rodrigo Vivi <rodrigo.vivi(a)intel.com> Cc: Roland Scheidegger <sroland(a)vmware.com> Cc: Russell King <linux(a)armlinux.org.uk> Cc: Sam Ravnborg <sam(a)ravnborg.org> Cc: Sandy Huang <hjc(a)rock-chips.com> Cc: Sascha Hauer <s.hauer(a)pengutronix.de> Cc: Sean Paul <sean(a)poorly.run> Cc: Seung-Woo Kim <sw0312.kim(a)samsung.com> Cc: Shawn Guo <shawnguo(a)kernel.org> Cc: Simon Ser <contact(a)emersion.fr> Cc: Stefan Agner <stefan(a)agner.ch> Cc: Steven Price <steven.price(a)arm.com> Cc: Sumit Semwal <sumit.semwal(a)linaro.org> Cc: Thierry Reding <thierry.reding(a)gmail.com> Cc: Tian Tao <tiantao6(a)hisilicon.com> Cc: Tomeu Vizoso <tomeu.vizoso(a)collabora.com> Cc: Tomi Valkeinen <tomba(a)kernel.org> Cc: VMware Graphics <linux-graphics-maintainer(a)vmware.com> Cc: Xinliang Liu <xinliang.liu(a)linaro.org> Cc: Xinwei Kong <kong.kongxinwei(a)hisilicon.com> Cc: Yannick Fertre <yannick.fertre(a)foss.st.com> Cc: Zack Rusin <zackr(a)vmware.com> Reviewed-by: Pekka Paalanen <pekka.paalanen(a)collabora.com> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> Signed-off-by: Maxime Ripard <maxime(a)cerno.tech> --- Changes from v5: - Typos fixes suggested by Pekka and Daniel - Added Pekka Reviewed-by Changes from v4: - Changes suggested by Pekka Changes from v3: - Roll back to the v2 - Add Simon and Pekka in Cc Changes from v2: - Take into account the feedback from Laurent and Lidiu to no longer force generic properties, but prefix vendor-specific properties with the vendor name Changes from v1: - Typos and wording reported by Daniel and Alex --- Documentation/gpu/drm-kms.rst | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-kms.rst index 87e5023e3f55..12e25119e563 100644 --- a/Documentation/gpu/drm-kms.rst +++ b/Documentation/gpu/drm-kms.rst @@ -463,6 +463,35 @@ KMS Properties This section of the documentation is primarily aimed at user-space developers. For the driver APIs, see the other sections. +Requirements +------------ + +KMS drivers might need to add extra properties to support new features. Each +new property introduced in a driver needs to meet a few requirements, in +addition to the one mentioned above: + +* It must be standardized, documenting: + + * The full, exact, name string; + * If the property is an enum, all the valid value name strings; + * What values are accepted, and what these values mean; + * What the property does and how it can be used; + * How the property might interact with other, existing properties. + +* It must provide a generic helper in the core code to register that + property on the object it attaches to. + +* Its content must be decoded by the core and provided in the object's + associated state structure. That includes anything drivers might want + to precompute, like struct drm_clip_rect for planes. + +* Its initial state must match the behavior prior to the property + introduction. This might be a fixed value matching what the hardware + does, or it may be inherited from the state the firmware left the + system in during boot. + +* An IGT test must be submitted where reasonable. + Property Types and Blob Property Support ---------------------------------------- -- 2.31.1

1 1

[PATCH] drm/panfrost: devfreq: Don't display error for EPROBE_DEFER
by Chris Morgan 23 Jul '21

23 Jul '21

From: Chris Morgan <macromorgan(a)hotmail.com> Set a condition for the message of "Couldn't set OPP regulators" to not display if the error code is EPROBE_DEFER. Note that I used an if statement to capture the condition instead of the dev_err_probe function because I didn't want to change the DRM_DEV_ERROR usage. Signed-off-by: Chris Morgan <macromorgan(a)hotmail.com> --- drivers/gpu/drm/panfrost/panfrost_devfreq.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/panfrost/panfrost_devfreq.c b/drivers/gpu/drm/panfrost/panfrost_devfreq.c index 3644652f726f..194af7f607a6 100644 --- a/drivers/gpu/drm/panfrost/panfrost_devfreq.c +++ b/drivers/gpu/drm/panfrost/panfrost_devfreq.c @@ -106,7 +106,8 @@ int panfrost_devfreq_init(struct panfrost_device *pfdev) if (ret) { /* Continue if the optional regulator is missing */ if (ret != -ENODEV) { - DRM_DEV_ERROR(dev, "Couldn't set OPP regulators\n"); + if (ret != -EPROBE_DEFER) + DRM_DEV_ERROR(dev, "Couldn't set OPP regulators\n"); return ret; } } -- 2.25.1

2 1

[PATCH v4 1/2] drm/i915: document caching related bits
by Matthew Auld 23 Jul '21

23 Jul '21

Try to document the object caching related bits, like cache_coherent and cache_dirty. v2(Ville): - As pointed out by Ville, fix the completely incorrect assumptions about the "partial" coherency on shared LLC platforms. v3(Daniel): - Fix nonsense about "dirtying" the cache with reads. v4(Daniel): - Various improvements, including adding some more details for WT. Suggested-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> Signed-off-by: Matthew Auld <matthew.auld(a)intel.com> Cc: Ville Syrjälä <ville.syrjala(a)linux.intel.com> Cc: Mika Kuoppala <mika.kuoppala(a)linux.intel.com> Reviewed-by: Daniel Vetter <daniel.vetter(a)ffwll.ch> --- .../gpu/drm/i915/gem/i915_gem_object_types.h | 187 +++++++++++++++++- drivers/gpu/drm/i915/i915_drv.h | 9 - 2 files changed, 183 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h index afbadfc5516b..79de925aecfd 100644 --- a/drivers/gpu/drm/i915/gem/i915_gem_object_types.h +++ b/drivers/gpu/drm/i915/gem/i915_gem_object_types.h @@ -92,6 +92,86 @@ struct drm_i915_gem_object_ops { const char *name; /* friendly name for debug, e.g. lockdep classes */ }; +/** + * enum i915_cache_level - The supported GTT caching values for system memory + * pages. + * + * These translate to some special GTT PTE bits when binding pages into some + * address space. It also determines whether an object, or rather its pages are + * coherent with the GPU, when also reading or writing through the CPU cache + * with those pages. + * + * Userspace can also control this through struct drm_i915_gem_caching. + */ +enum i915_cache_level { + /** + * @I915_CACHE_NONE: + * + * GPU access is not coherent with the CPU cache. If the cache is dirty + * and we need the underlying pages to be coherent with some later GPU + * access then we need to manually flush the pages. + * + * On shared LLC platforms reads and writes through the CPU cache are + * still coherent even with this setting. See also + * &drm_i915_gem_object.cache_coherent for more details. Due to this we + * should only ever use uncached for scanout surfaces, otherwise we end + * up over-flushing in some places. + * + * This is the default on non-LLC platforms. + */ + I915_CACHE_NONE = 0, + /** + * @I915_CACHE_LLC: + * + * GPU access is coherent with the CPU cache. If the cache is dirty, + * then the GPU will ensure that access remains coherent, when both + * reading and writing through the CPU cache. GPU writes can dirty the + * CPU cache. + * + * Not used for scanout surfaces. + * + * Applies to both platforms with shared LLC(HAS_LLC), and snooping + * based platforms(HAS_SNOOP). + * + * This is the default on shared LLC platforms. The only exception is + * scanout objects, where the display engine is not coherent with the + * CPU cache. For such objects I915_CACHE_NONE or I915_CACHE_WT is + * automatically applied by the kernel in pin_for_display, if userspace + * has not done so already. + */ + I915_CACHE_LLC, + /** + * @I915_CACHE_L3_LLC: + * + * Explicitly enable the Gfx L3 cache, with coherent LLC. + * + * The Gfx L3 sits between the domain specific caches, e.g + * sampler/render caches, and the larger LLC. LLC is coherent with the + * GPU, but L3 is only visible to the GPU, so likely needs to be flushed + * when the workload completes. + * + * Not used for scanout surfaces. + * + * Only exposed on some gen7 + GGTT. More recent hardware has dropped + * this explicit setting, where it should now be enabled by default. + */ + I915_CACHE_L3_LLC, + /** + * @I915_CACHE_WT: + * + * Write-through. Used for scanout surfaces. + * + * The GPU can utilise the caches, while still having the display engine + * be coherent with GPU writes, as a result we don't need to flush the + * CPU caches when moving out of the render domain. This is the default + * setting chosen by the kernel, if supported by the HW, otherwise we + * fallback to I915_CACHE_NONE. On the CPU side writes through the CPU + * cache still need to be flushed, to remain coherent with the display + * engine. + */ + I915_CACHE_WT, +}; + enum i915_map_type { I915_MAP_WB = 0, I915_MAP_WC, @@ -229,14 +309,113 @@ struct drm_i915_gem_object { unsigned int mem_flags; #define I915_BO_FLAG_STRUCT_PAGE BIT(0) /* Object backed by struct pages */ #define I915_BO_FLAG_IOMEM BIT(1) /* Object backed by IO memory */ - /* - * Is the object to be mapped as read-only to the GPU - * Only honoured if hardware has relevant pte bit + /** + * @cache_level: The desired GTT caching level. + * + * See enum i915_cache_level for possible values, along with what + * each does. */ unsigned int cache_level:3; - unsigned int cache_coherent:2; + /** + * @cache_coherent: + * + * Track whether the pages are coherent with the GPU if reading or + * writing through the CPU caches. The largely depends on the + * @cache_level setting. + * + * On platforms which don't have the shared LLC(HAS_SNOOP), like on Atom + * platforms, coherency must be explicitly requested with some special + * GTT caching bits(see enum i915_cache_level). When enabling coherency + * it does come at a performance and power cost on such platforms. On + * the flip side the kernel does not need to manually flush any buffers + * which need to be coherent with the GPU, if the object is not coherent + * i.e @cache_coherent is zero. + * + * On platforms that share the LLC with the CPU(HAS_LLC), all GT memory + * access will automatically snoop the CPU caches(even with CACHE_NONE). + * The one exception is when dealing with the display engine, like with + * scanout surfaces. To handle this the kernel will always flush the + * surface out of the CPU caches when preparing it for scanout. Also + * note that since scanout surfaces are only ever read by the display + * engine we only need to care about flushing any writes through the CPU + * cache, reads on the other hand will always be coherent. + * + * Something strange here is why @cache_coherent is not a simple + * boolean, i.e coherent vs non-coherent. The reasoning for this is back + * to the display engine not being fully coherent. As a result scanout + * surfaces will either be marked as I915_CACHE_NONE or I915_CACHE_WT. + * In the case of seeing I915_CACHE_NONE the kernel makes the assumption + * that this is likely a scanout surface, and will set @cache_coherent + * as only I915_BO_CACHE_COHERENT_FOR_READ, on platforms with the shared + * LLC. The kernel uses this to always flush writes through the CPU + * cache as early as possible, where it can, in effect keeping + * @cache_dirty clean, so we can potentially avoid stalling when + * flushing the surface just before doing the scanout. This does mean + * we might unnecessarily flush non-scanout objects in some places, but + * the default assumption is that all normal objects should be using + * I915_CACHE_LLC, at least on platforms with the shared LLC. + * + * Supported values: + * + * I915_BO_CACHE_COHERENT_FOR_READ: + * + * On shared LLC platforms, we use this for special scanout surfaces, + * where the display engine is not coherent with the CPU cache. As such + * we need to ensure we flush any writes before doing the scanout. As an + * optimisation we try to flush any writes as early as possible to avoid + * stalling later. + * + * Thus for scanout surfaces using I915_CACHE_NONE, on shared LLC + * platforms, we use: + * + * cache_coherent = I915_BO_CACHE_COHERENT_FOR_READ + * + * While for normal objects that are fully coherent, including special + * scanout surfaces marked as I915_CACHE_WT, we use: + * + * cache_coherent = I915_BO_CACHE_COHERENT_FOR_READ | + * I915_BO_CACHE_COHERENT_FOR_WRITE + * + * And then for objects that are not coherent at all we use: + * + * cache_coherent = 0 + * + * I915_BO_CACHE_COHERENT_FOR_WRITE: + * + * When writing through the CPU cache, the GPU is still coherent. Note + * that this also implies I915_BO_CACHE_COHERENT_FOR_READ. + */ #define I915_BO_CACHE_COHERENT_FOR_READ BIT(0) #define I915_BO_CACHE_COHERENT_FOR_WRITE BIT(1) + unsigned int cache_coherent:2; + + /** + * @cache_dirty: + * + * Track if we are we dirty with writes through the CPU cache for this + * object. As a result reading directly from main memory might yield + * stale data. + * + * This also ties into whether the kernel is tracking the object as + * coherent with the GPU, as per @cache_coherent, as it determines if + * flushing might be needed at various points. + * + * Another part of @cache_dirty is managing flushing when first + * acquiring the pages for system memory, at this point the pages are + * considered foreign, so the default assumption is that the cache is + * dirty, for example the page zeroing done by the kernel might leave + * writes though the CPU cache, or swapping-in, while the actual data in + * main memory is potentially stale. Note that this is a potential + * security issue when dealing with userspace objects and zeroing. Now, + * whether we actually need apply the big sledgehammer of flushing all + * the pages on acquire depends on if @cache_coherent is marked as + * I915_BO_CACHE_COHERENT_FOR_WRITE, i.e that the GPU will be coherent + * for both reads and writes though the CPU cache. + * + * Note that on shared LLC platforms we still apply the heavy flush for + * I915_CACHE_NONE objects, under the assumption that this is going to + * be used for scanout. + */ unsigned int cache_dirty:1; /** diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index d118834a4ed9..f65e0441fc99 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -394,15 +394,6 @@ struct drm_i915_display_funcs { void (*read_luts)(struct intel_crtc_state *crtc_state); }; -enum i915_cache_level { - I915_CACHE_NONE = 0, - I915_CACHE_LLC, /* also used for snoopable memory on non-LLC */ - I915_CACHE_L3_LLC, /* gen7+, L3 sits between the domain specifc - caches, eg sampler/render caches, and the - large Last-Level-Cache. LLC is coherent with - the CPU, but L3 is only visible to the GPU. */ - I915_CACHE_WT, /* hsw:gt3e WriteThrough for scanouts */ -}; #define I915_COLOR_UNEVICTABLE (-1) /* a non-vma sharing the address space */ -- 2.26.3

1 1

[PATCH v4 0/6] Add Mediatek Soc DRM (vdosys0) support for mt8195
by jason-jh.lin 23 Jul '21

23 Jul '21

The hardware path of vdosys0 with eDP panel output need to go through by several modules, such as, OVL, RDMA, COLOR, CCORR, AAL, GAMMA, DITHER, DSC and MERGE. Change in v4: - extract dt-binding patches to another patch series https://patchwork.kernel.org/project/linux-mediatek/list/?series=519597 - squash DSC module into mtk_drm_ddp_comp.c - add coment and simplify MERGE config function Change in v3: - change mmsys and display dt-bindings document from txt to yaml - add MERGE additional description in display dt-bindings document - fix mboxes-cells number of vdosys0 node in dts - drop mutex eof convert define - remove pm_runtime apis in DSC and MERGE - change DSC and MERGE enum to alphabetic order Change in v2: - add DSC yaml file - add mt8195 drm driver porting parts in to one patch - remove useless define, variable, structure member and function - simplify DSC and MERGE file and switch threre order jason-jh.lin (6): arm64: dts: mt8195: add display node for vdosys0 soc: mediatek: add mtk-mmsys support for mt8195 vdosys0 soc: mediatek: add mtk-mutex support for mt8195 vdosys0 drm/mediatek: add mediatek-drm of vdosys0 support for mt8195 drm/mediatek: add DSC support for mt8195 drm/mediatek: add MERGE support for mt8195 arch/arm64/boot/dts/mediatek/mt8195.dtsi | 111 ++++++++ drivers/gpu/drm/mediatek/Makefile | 1 + drivers/gpu/drm/mediatek/mtk_disp_drv.h | 8 + drivers/gpu/drm/mediatek/mtk_disp_merge.c | 277 ++++++++++++++++++++ drivers/gpu/drm/mediatek/mtk_disp_rdma.c | 6 + drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.c | 62 +++++ drivers/gpu/drm/mediatek/mtk_drm_ddp_comp.h | 2 + drivers/gpu/drm/mediatek/mtk_drm_drv.c | 32 ++- drivers/gpu/drm/mediatek/mtk_drm_drv.h | 1 + drivers/soc/mediatek/mt8195-mmsys.h | 191 ++++++++++++++ drivers/soc/mediatek/mtk-mmsys.c | 11 + drivers/soc/mediatek/mtk-mutex.c | 93 ++++++- include/linux/soc/mediatek/mtk-mmsys.h | 9 + 13 files changed, 800 insertions(+), 4 deletions(-) create mode 100644 drivers/gpu/drm/mediatek/mtk_disp_merge.c create mode 100644 drivers/soc/mediatek/mt8195-mmsys.h -- 2.18.0

2 7

← Newer
1
...
36
37
38
39
40
41
42
...
83
Older →

Jump to page:

2025

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

dri-devel July 2021