Re: [PATCH] drm/komeda: Fix komeda driver build error
On Wed, Nov 13, 2019 at 2:32 AM james qian wang (Arm Technology China)
wrote:
>
> Fix the build errors lead by
>
> 'commit 4039f0293bbd ("drm/komeda: Add option to print WARN- and INFO-level
> IRQ events")'
>
> Signed-off-by: james qian wang (Arm Technology China)
>
Reviewed-by: Daniel Vetter
Please also re-enable the komde driver (including all options) in all
the defconfigs in drm-rerere. And maybe type some scripts to compile
test before pushing :-)
-Daniel
> ---
> drivers/gpu/drm/arm/display/komeda/komeda_dev.h | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> index 15f52e304c08..d406a4d83352 100644
> --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> @@ -51,12 +51,12 @@
>
> #define KOMEDA_WARN_EVENTS KOMEDA_ERR_CSCE
>
> -#define KOMEDA_INFO_EVENTS ({0 \
> +#define KOMEDA_INFO_EVENTS (0 \
> | KOMEDA_EVENT_VSYNC \
> | KOMEDA_EVENT_FLIP \
> | KOMEDA_EVENT_EOW \
> | KOMEDA_EVENT_MODE \
> - })
> + )
>
> /* malidp device id */
> enum {
> --
> 2.20.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge: anx6345: Fix compilation breakage on systems without CONFIG_OF
On Wed, Nov 13, 2019 at 8:56 AM Maxime Ripard wrote:
>
> The driver assumes that the platform uses the device tree, and thus relies
> on some fields (of_node) being declared in some structures (drm_bridge).
>
> This isn't true for all platforms, so make sure we can only compile the
> ANX6345 on platforms where DT support is selected.
>
> Cc: Torsten Duwe
> Fixes: 6aa192698089 ("drm/bridge: Add Analogix anx6345 support")
> Signed-off-by: Maxime Ripard
Acked-by: Daniel Vetter
> ---
> drivers/gpu/drm/bridge/analogix/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/Kconfig
> b/drivers/gpu/drm/bridge/analogix/Kconfig
> index 1425a96a28c3..e1fa7d820373 100644
> --- a/drivers/gpu/drm/bridge/analogix/Kconfig
> +++ b/drivers/gpu/drm/bridge/analogix/Kconfig
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
> config DRM_ANALOGIX_ANX6345
> tristate "Analogix ANX6345 bridge"
> + depends on OF
> select DRM_ANALOGIX_DP
> select DRM_KMS_HELPER
> select REGMAP_I2C
> --
> 2.23.0
>
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Drm: mgag200. Video adapter issue with 5.4.0-rc3 ; no graphics
Hi John Am 12.11.19 um 20:13 schrieb John Donnelly: > > In short . I started with : > > git bisect start > > git bisect bad be8454afc50f > > git bisect good fec88ab0af97 > > And at the the end of bisects showed this was the offending commit : > > c0a74c732568 > > commit c0a74c732568ad347f7b3de281922808dab30504 (refs/bisect/bad) > Author: Jani Nikula > Date: Fri May 24 20:35:22 2019 +0300 > > drm/i915: Update DRIVER_DATE to 20190524 > > Signed-off-by: Jani Nikula > > That does not have any real relevance No, that doesn't look right. The other bad commits you list below also don't seem to be related. You could also bisect between your original working commit (v4.18?) and the one you want to upgrade to (v5.3?). It's only a few more builds but might be might give better results. BTW, are you also converting Gnome from X11 to Wayland. I found that Gnome's Wayland code is so slow on mgag200 that it's unusable. They recently added a shadow FB [1] to improve performance, but it won't be available before Gnome 3.36. Best regards Thomas [1] https://gitlab.gnome.org/GNOME/mutter/merge_requests/877/diffs > > > I am not sure if I did the bisects correctly . After each test I did : > > > #1 git bisect bad 827440a90146 > > #2 git bisect bad f5b07b04e5f0 > > #3 git bisect bad c0a74c732568 > > #4 git bisect good 818f5cb3e8fb > > #5 git bisect good 6cfe7ec02e85 > > #6 git bisect good f71e01a78bee > > #7 git bisect good 09a93ef3d60f > > #8 git bisect good f1e6b336bafa > > #9 git bisect good eaf20e6933dc > > #10 git bisect good 63e8dcdb4f8e > > #11 git bisect good 397049a03022 > > I’ve restarted the bisect without appending the after a the > “bad|good “ , and so far git is showing the same selections. > > > > > > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer signature.asc Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/gem: Fix mmap fake offset handling for drm_gem_object_funcs.mmap
On Wed, Nov 13, 2019 at 8:39 AM Gerd Hoffmann wrote: > > Hi, > > > > > >> VRAM helpers use drm_gem_ttm_mmap(), which wraps ttm_bo_mmap_obj(). > > > > >> These changes should be transparent. > > > > > > > > > > There's still the issue that for dma-buf mmap vs drm mmap you use > > > > > different f_mapping, which means ttm's pte shootdown won't work > > > > > correctly > > > > > for dma-buf mmaps. But yeah normal operation for ttm/vram helpers > > > > > should > > > > > be fine. > > > > > > > > VRAM helpers don't support dmabufs. > > > > > > It's not that simple. Even when not supporting dma-buf export and > > > import it is still possible to create dma-bufs, import them into the > > > same driver (which doesn't actually export+import but just grabs a gem > > > object reference instead) and also to mmap them via prime/dma-buf code > > > path ... > > ... but after looking again I think we are all green here. Given that > only self-import works we'll only see vram gem objects in the mmap code > path, which should have everything set up correctly. Same goes for qxl. > > All other ttm drivers still use the old mmap code path, so all green > there too I think. Also I somehow doubt dma-buf mmap vs. drm mmap ends > up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open() > which would fire should that be the case. > > Do imported dma-bufs hit the drm mmap code path in the first place? > Wouldn't mmap be handled by the exporting driver? drm_gem_dmabuf_mmap -> obj->funcs->mmap -> ttm_bo_mmap_obj And I'm not seeing anyone adjusting vm_file->f_mapping anywhere here at all. Note to hit this you need userspace to - handle2fd on a buffer to create a dma-buf fd - call mmap directly on that dma-buf fd > > > Can ttm use the same trick msm uses? Now that ttm bo's are a gem object > > > superclass I think we should be able to switch vma->vm_file to > > > bo->base.filp, simliar to msm_gem_mmap_obj(), probably best done in > > > drm_gem_ttm_mmap(). > > > > bo->base.filp is the shmem inode file, and I'm not too sure how much shmem > > approves of us misappropriating the mapping. For shmem only objects it > > probably doesn't matter (since both gem mmaps and shmem mmaps will point > > at the same underlying struct page), but for vram/ttm/anything else the > > gem mmap might point into iomem, and shmem then might go boom trying to do > > stuff with that. > > Yes, agreeing here after wading through the code ... > > > I think having our own mapping would be the cleanest > > long-term approach ... > > You mean using per drm object (instead of per drm device) address spaces? Yup. But I think that would be quite a pile of work, since we need an anon inode for each gem bo. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 00/23] mm/gup: track dma-pinned pages: FOLL_PIN, FOLL_LONGTERM
On Tue, Nov 12, 2019 at 10:10 PM John Hubbard wrote: > > On 11/12/19 12:38 PM, Jason Gunthorpe wrote: > > On Mon, Nov 11, 2019 at 04:06:37PM -0800, John Hubbard wrote: > >> Hi, > >> > >> The cover letter is long, so the more important stuff is first: > >> > >> * Jason, if you or someone could look at the the VFIO cleanup (patch 8) > >> and conversion to FOLL_PIN (patch 18), to make sure it's use of > >> remote and longterm gup matches what we discussed during the review > >> of v2, I'd appreciate it. > >> > >> * Also for Jason and IB: as noted below, in patch 11, I am (too?) boldly > >> converting from put_user_pages() to release_pages(). > > > > Why are we doing this? I think things got confused here someplace, as > > > Because: > > a) These need put_page() calls, and > > b) there is no put_pages() call, but there is a release_pages() call that > is, arguably, what put_pages() would be. > > > > the comment still says: > > > > /** > > * put_user_page() - release a gup-pinned page > > * @page:pointer to page to be released > > * > > * Pages that were pinned via get_user_pages*() must be released via > > * either put_user_page(), or one of the put_user_pages*() routines > > * below. > > > Ohhh, I missed those comments. They need to all be changed over to > say "pages that were pinned via pin_user_pages*() or > pin_longterm_pages*() must be released via put_user_page*()." > > The get_user_pages*() pages must still be released via put_page. > > The churn is due to a fairly significant change in strategy, whis > is: instead of changing all get_user_pages*() sites to call > put_user_page(), change selected sites to call pin_user_pages*() or > pin_longterm_pages*(), plus put_user_page(). Can't we call this unpin_user_page then, for some symmetry? Or is that even more churn? Looking from afar the naming here seems really confusing. -Daniel > That allows incrementally converting the kernel over to using the > new pin APIs, without taking on the huge risk of a big one-shot > conversion. > > So, I've ended up with one place that actually needs to get reverted > back to get_user_pages(), and that's the IB ODP code. > > > > > I feel like if put_user_pages() is not the correct way to undo > > get_user_pages() then it needs to be deleted. > > > > Yes, you're right. I'll fix the put_user_page comments() as described. > > > thanks, > > John Hubbard > NVIDIA -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 00/23] mm/gup: track dma-pinned pages: FOLL_PIN, FOLL_LONGTERM
On 11/13/19 12:22 AM, Daniel Vetter wrote: ... Why are we doing this? I think things got confused here someplace, as Because: a) These need put_page() calls, and b) there is no put_pages() call, but there is a release_pages() call that is, arguably, what put_pages() would be. the comment still says: /** * put_user_page() - release a gup-pinned page * @page:pointer to page to be released * * Pages that were pinned via get_user_pages*() must be released via * either put_user_page(), or one of the put_user_pages*() routines * below. Ohhh, I missed those comments. They need to all be changed over to say "pages that were pinned via pin_user_pages*() or pin_longterm_pages*() must be released via put_user_page*()." The get_user_pages*() pages must still be released via put_page. The churn is due to a fairly significant change in strategy, whis is: instead of changing all get_user_pages*() sites to call put_user_page(), change selected sites to call pin_user_pages*() or pin_longterm_pages*(), plus put_user_page(). Can't we call this unpin_user_page then, for some symmetry? Or is that even more churn? Looking from afar the naming here seems really confusing. That look from afar is valuable, because I'm too close to the problem to see how the naming looks. :) unpin_user_page() sounds symmetrical. It's true that it would cause more churn (which is why I started off with a proposal that avoids changing the names of put_user_page*() APIs). But OTOH, the amount of churn is proportional to the change in direction here, and it's really only 10 or 20 lines changed, in the end. So I'm open to changing to that naming. It would be nice to hear what others prefer, too... thanks, -- John Hubbard NVIDIA ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/4] drm/udl: Replace fbdev code with generic emulation
Hi
Am 12.11.19 um 16:13 schrieb Daniel Vetter:
> On Tue, Nov 12, 2019 at 3:51 PM Thomas Zimmermann wrote:
>>
>> Hi
>>
>> Am 12.11.19 um 15:31 schrieb Daniel Vetter:
>>> On Tue, Nov 12, 2019 at 3:03 PM Thomas Zimmermann
>>> wrote:
Hi
Am 12.11.19 um 14:40 schrieb Daniel Vetter:
> On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann
> wrote:
>>
>> Hi
>>
>> Am 08.11.19 um 16:37 schrieb Noralf Trønnes:
>>>
>>>
>>> Den 08.11.2019 13.33, skrev Thomas Zimmermann:
The udl driver can use the generic fbdev implementation. Convert it.
Signed-off-by: Thomas Zimmermann
---
>>>
diff --git a/drivers/gpu/drm/udl/udl_drv.c
b/drivers/gpu/drm/udl/udl_drv.c
index 563cc5809e56..55c0f9dfee29 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
>>>
@@ -47,6 +48,8 @@ static struct drm_driver driver = {
.driver_features = DRIVER_MODESET | DRIVER_GEM,
.release = udl_driver_release,
+.lastclose = drm_fb_helper_lastclose,
+
>>>
>>> No need to set this, it's already wired up:
>>>
>>> drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
>>> drm_fb_helper_lastclose
>>>
/* gem hooks */
.gem_create_object = udl_driver_gem_create_object,
>>>
diff --git a/drivers/gpu/drm/udl/udl_fb.c
b/drivers/gpu/drm/udl/udl_fb.c
index f8153b726343..afe74f892a2b 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -20,19 +20,9 @@
#include "udl_drv.h"
-#define DL_DEFIO_WRITE_DELAY(HZ/20) /* fb_deferred_io.delay in
jiffies */
-
-static int fb_defio = 0; /* Optionally enable experimental fb_defio
mmap support */
static int fb_bpp = 16;
module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
>>>
>>> Maybe fb_bpp can be dropped too?
>>
>> Sure, makes sense.
>>
>> The driver exposes a preferred color depth of 24 bpp, which we may want
>> to change to 16 then. The internal framebuffer is only 16 bpp anyway.
>
> Just something that crossed my mind: Should we ensure that the
> preferred format of the primary plane (should be the first in the
> format array) matches up with the preferred bpp setting? Maybe even
> enforce that for drivers with an explicit primary plane (i.e. atomic
> drivers). I think tiny drivers get this right already.
IMHO that makes if the userspace can handle it. The preferred bpp could
also be retrieved from the formats array automatically. What about HW
with multiple CRTCs with different format defaults (sounds weird, I know)?
>>>
>>> Ime I haven't seen such a case yet. What I have seen is that the most
>>> preferred format might be some fancy compressed format, which not all
>>> formats support. But which you can't render into without mesa anyway,
>>> so really doesn't matter for preferred bpp.
>>>
WRT udl: For v3 of this patchset I've set the preferred color depth to
32 bpp; although the internal FB is always at 16 bpp. Because when I
tested with a dual-screen setup (radeon + udl) X11 didn't support the 16
bpp output on the second screen (the one driven by udl). Only setting
both screen to 32 bpp worked out of the box. And the preferred 24 bpp
are not even supported by udl.
>>>
>>> Uh, if we can only set preferred bpp to make X happy, and X can only
>>> support one preferred bpp, then everyone needs to set 32bit. Which
>>> defeats the point (and we'd need to hardcode it to 32bpp). Is this
>>> really the case?
>>
>> I guess it would have worked if both screens preferred 16 bpp.
>
> Just noticed that current depth is 24 bpp ... does that work with
> current X? If not I think we should actually set it to 16 bpp, and fix
> up X. Not as in "make it handle multi-bpp", that's too hard, but make
> it pick a common format that works for all drivers (which usually
> means go with 32bpp). Since if we just go with 32bpp to paper over X,
> then the preferred bpp uapi becomes meaningless.
The good news is that it apparently works with planes correctly configured.
I tested with udl being converted to simple pipe and universal planes.
It exposes RGB565, XRGB and preferred_depth=16. X did the correct
thing and choose a format the works with radeon and udl.
For the next iteration of the fbdev conversion, I'll just keep the
current values. And once converted to universal planes, we can set the
optimal values instead.
Best regards
Thomas
> -Daniel
>
>>
>> Best regards
>> Thomas
>>
>>> -Daniel
>>>
Best regards
Thomas
> -Daniel
>
>>
>> Best regards
>> Thomas
>>
>>>
>>> It's possible
[PATCH] drm/vmwgfx: Use coherent memory if there are dma mapping size restrictions
From: Thomas Hellstrom
We're gradually moving towards using DMA coherent memory in most
situations, although TTM interactions with the DMA layers is still a
work-in-progress. Meanwhile, use coherent memory when there are size
restrictions meaning that there is a chance that streaming dma mapping
of large buffer objects may fail.
Also move DMA mask settings to the vmw_dma_select_mode function, since
it's important that we set the correct DMA masks before calling the
dma_max_mapping_size() function.
Cc: Christoph Hellwig
Signed-off-by: Thomas Hellstrom
Reviewed-by: Brian Paul
---
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 31 +++--
1 file changed, 7 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index fc0283659c41..1e1de83908fe 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -569,7 +569,10 @@ static int vmw_dma_select_mode(struct vmw_private
*dev_priv)
[vmw_dma_map_populate] = "Caching DMA mappings.",
[vmw_dma_map_bind] = "Giving up DMA mappings early."};
- if (vmw_force_coherent)
+ (void) dma_set_mask_and_coherent(dev_priv->dev->dev, DMA_BIT_MASK(64));
+
+ if (vmw_force_coherent ||
+ dma_max_mapping_size(dev_priv->dev->dev) != SIZE_MAX)
dev_priv->map_mode = vmw_dma_alloc_coherent;
else if (vmw_restrict_iommu)
dev_priv->map_mode = vmw_dma_map_bind;
@@ -582,30 +585,15 @@ static int vmw_dma_select_mode(struct vmw_private
*dev_priv)
return -EINVAL;
DRM_INFO("DMA map mode: %s\n", names[dev_priv->map_mode]);
- return 0;
-}
-/**
- * vmw_dma_masks - set required page- and dma masks
- *
- * @dev: Pointer to struct drm-device
- *
- * With 32-bit we can only handle 32 bit PFNs. Optionally set that
- * restriction also for 64-bit systems.
- */
-static int vmw_dma_masks(struct vmw_private *dev_priv)
-{
- struct drm_device *dev = dev_priv->dev;
- int ret = 0;
-
- ret = dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(64));
if (dev_priv->map_mode != vmw_dma_phys &&
(sizeof(unsigned long) == 4 || vmw_restrict_dma_mask)) {
DRM_INFO("Restricting DMA addresses to 44 bits.\n");
- return dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(44));
+ return dma_set_mask_and_coherent(dev_priv->dev->dev,
+DMA_BIT_MASK(44));
}
- return ret;
+ return 0;
}
static int vmw_driver_load(struct drm_device *dev, unsigned long chipset)
@@ -674,7 +662,6 @@ static int vmw_driver_load(struct drm_device *dev, unsigned
long chipset)
dev_priv->capabilities2 = vmw_read(dev_priv, SVGA_REG_CAP2);
}
-
ret = vmw_dma_select_mode(dev_priv);
if (unlikely(ret != 0)) {
DRM_INFO("Restricting capabilities due to IOMMU setup.\n");
@@ -746,10 +733,6 @@ static int vmw_driver_load(struct drm_device *dev,
unsigned long chipset)
if (dev_priv->capabilities & SVGA_CAP_CAP2_REGISTER)
vmw_print_capabilities2(dev_priv->capabilities2);
- ret = vmw_dma_masks(dev_priv);
- if (unlikely(ret != 0))
- goto out_err0;
-
dma_set_max_seg_size(dev->dev, min_t(unsigned int, U32_MAX & PAGE_MASK,
SCATTERLIST_MAX_SEGMENT));
--
2.21.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/vmwgfx: remove set but not used variable 'srf'
From: YueHaibing
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c:339:22:
warning: variable srf set but not used [-Wunused-but-set-variable]
'srf' is never used, so can be removed.
Signed-off-by: YueHaibing
Reviewed-by: Thomas Hellstrom
---
drivers/gpu/drm/vmwgfx/vmwgfx_surface.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
index 29d8794f0421..de0530b4dc1b 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_surface.c
@@ -336,7 +336,6 @@ static void vmw_hw_surface_destroy(struct vmw_resource *res)
{
struct vmw_private *dev_priv = res->dev_priv;
- struct vmw_surface *srf;
void *cmd;
if (res->func->destroy == vmw_gb_surface_destroy) {
@@ -360,7 +359,6 @@ static void vmw_hw_surface_destroy(struct vmw_resource *res)
*/
mutex_lock(&dev_priv->cmdbuf_mutex);
- srf = vmw_res_to_srf(res);
dev_priv->used_memory_size -= res->backup_size;
mutex_unlock(&dev_priv->cmdbuf_mutex);
}
--
2.21.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/vmwgfx: Use dma-coherent memory for high-bandwidth port messaging
From: Thomas Hellstrom
With AMD-SEV high-bandwidth port messaging runs into trouble since the
message content is encrypted using the vm-specific key, and the
hypervisor is unable to read it.
So use unencrypted dma-coherent bounce buffers for temporary message
storage space. Allocating that memory is expensive so a future
optimization might include a static unencrypted memory area for messages.
Signed-off-by: Thomas Hellstrom
Reviewed-by: Brian Paul
---
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 4 +--
drivers/gpu/drm/vmwgfx/vmwgfx_drv.h | 4 +--
drivers/gpu/drm/vmwgfx/vmwgfx_msg.c | 45 +
3 files changed, 31 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index 81a95651643f..fc0283659c41 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -908,13 +908,13 @@ static int vmw_driver_load(struct drm_device *dev,
unsigned long chipset)
snprintf(host_log, sizeof(host_log), "vmwgfx: %s-%s",
VMWGFX_REPO, VMWGFX_GIT_VERSION);
- vmw_host_log(host_log);
+ vmw_host_log(dev->dev, host_log);
memset(host_log, 0, sizeof(host_log));
snprintf(host_log, sizeof(host_log), "vmwgfx: Module Version: %d.%d.%d",
VMWGFX_DRIVER_MAJOR, VMWGFX_DRIVER_MINOR,
VMWGFX_DRIVER_PATCHLEVEL);
- vmw_host_log(host_log);
+ vmw_host_log(dev->dev, host_log);
if (dev_priv->enable_fb) {
vmw_fifo_resource_inc(dev_priv);
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
index b18842f73081..a77bf72cb9ac 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.h
@@ -1389,9 +1389,9 @@ int vmw_bo_cpu_blit(struct ttm_buffer_object *dst,
struct vmw_diff_cpy *diff);
/* Host messaging -vmwgfx_msg.c: */
-int vmw_host_get_guestinfo(const char *guest_info_param,
+int vmw_host_get_guestinfo(struct device *dev, const char *guest_info_param,
char *buffer, size_t *length);
-int vmw_host_log(const char *log);
+int vmw_host_log(struct device *dev, const char *log);
/* VMW logging */
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
index b6c5e4c2ac3c..f439b7afa3a5 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_msg.c
@@ -304,8 +304,8 @@ STACK_FRAME_NON_STANDARD(vmw_send_msg);
* @msg: [OUT] message received from the host
* @msg_len: message length
*/
-static int vmw_recv_msg(struct rpc_channel *channel, void **msg,
- size_t *msg_len)
+static int vmw_recv_msg(struct device *dev, struct rpc_channel *channel,
+ void **msg, size_t *msg_len, dma_addr_t *dma_handle)
{
unsigned long eax, ebx, ecx, edx, si, di;
char *reply;
@@ -339,7 +339,8 @@ static int vmw_recv_msg(struct rpc_channel *channel, void
**msg,
return 0;
reply_len = ebx;
- reply = kzalloc(reply_len + 1, GFP_KERNEL);
+ reply = dma_alloc_coherent(dev, reply_len + 1, dma_handle,
+ GFP_KERNEL);
if (!reply) {
DRM_ERROR("Cannot allocate memory for host message
reply.\n");
return -ENOMEM;
@@ -350,7 +351,7 @@ static int vmw_recv_msg(struct rpc_channel *channel, void
**msg,
ebx = vmw_port_hb_in(channel, reply, reply_len,
!!(HIGH_WORD(ecx) & MESSAGE_STATUS_HB));
if ((HIGH_WORD(ebx) & MESSAGE_STATUS_SUCCESS) == 0) {
- kfree(reply);
+ dma_free_coherent(dev, reply_len + 1, reply,
*dma_handle);
reply = NULL;
if ((HIGH_WORD(ebx) & MESSAGE_STATUS_CPT) != 0) {
/* A checkpoint occurred. Retry. */
@@ -374,7 +375,7 @@ static int vmw_recv_msg(struct rpc_channel *channel, void
**msg,
eax, ebx, ecx, edx, si, di);
if ((HIGH_WORD(ecx) & MESSAGE_STATUS_SUCCESS) == 0) {
- kfree(reply);
+ dma_free_coherent(dev, reply_len + 1, reply,
*dma_handle);
reply = NULL;
if ((HIGH_WORD(ecx) & MESSAGE_STATUS_CPT) != 0) {
/* A checkpoint occurred. Retry. */
@@ -404,18 +405,21 @@ STACK_FRAME_NON_STANDARD(vmw_recv_msg);
* Gets the value of a GuestInfo.* parameter. The value returned will be in
* a string, and it is up to the caller to post-process.
*
+ * @dev: Pointer to struct device used for coherent memory allocation
* @guest_info_param: Parameter to get, e.g. GuestInfo.svga.gl3
* @buffer: if NULL, *reply_len will contain reply size.
* @length: size of the reply_buf. Set to s
Re: [PATCH] drm/gma500: Fixup fbdev stolen size usage evaluation
On Tue, Nov 12, 2019 at 4:50 PM Paul Kocialkowski
wrote:
>
> Hi,
>
> On Tue 12 Nov 19, 16:11, Paul Kocialkowski wrote:
> > Hi,
> >
> > On Tue 12 Nov 19, 11:20, Patrik Jakobsson wrote:
> > > On Thu, Nov 7, 2019 at 4:30 PM Paul Kocialkowski
> > > wrote:
> > > >
> > > > psbfb_probe performs an evaluation of the required size from the stolen
> > > > GTT memory, but gets it wrong in two distinct ways:
> > > > - The resulting size must be page-size-aligned;
> > > > - The size to allocate is derived from the surface dimensions, not the
> > > > fb
> > > > dimensions.
> > > >
> > > > When two connectors are connected with different modes, the smallest
> > > > will
> > > > be stored in the fb dimensions, but the size that needs to be allocated
> > > > must
> > > > match the largest (surface) dimensions. This is what is used in the
> > > > actual
> > > > allocation code.
> > > >
> > > > Fix this by correcting the evaluation to conform to the two points
> > > > above.
> > > > It allows correctly switching to 16bpp when one connector is e.g.
> > > > 1920x1080
> > > > and the other is 1024x768.
> > > >
> > > > Signed-off-by: Paul Kocialkowski
> > > > ---
> > > > drivers/gpu/drm/gma500/framebuffer.c | 8 ++--
> > > > 1 file changed, 6 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/gma500/framebuffer.c
> > > > b/drivers/gpu/drm/gma500/framebuffer.c
> > > > index 218f3bb15276..90237abee088 100644
> > > > --- a/drivers/gpu/drm/gma500/framebuffer.c
> > > > +++ b/drivers/gpu/drm/gma500/framebuffer.c
> > > > @@ -462,6 +462,7 @@ static int psbfb_probe(struct drm_fb_helper *helper,
> > > > container_of(helper, struct psb_fbdev, psb_fb_helper);
> > > > struct drm_device *dev = psb_fbdev->psb_fb_helper.dev;
> > > > struct drm_psb_private *dev_priv = dev->dev_private;
> > > > + unsigned int fb_size;
> > > > int bytespp;
> > > >
> > > > bytespp = sizes->surface_bpp / 8;
> > > > @@ -471,8 +472,11 @@ static int psbfb_probe(struct drm_fb_helper
> > > > *helper,
> > > > /* If the mode will not fit in 32bit then switch to 16bit to get
> > > >a console on full resolution. The X mode setting server will
> > > >allocate its own 32bit GEM framebuffer */
> > > > - if (ALIGN(sizes->fb_width * bytespp, 64) * sizes->fb_height >
> > > > - dev_priv->vram_stolen_size) {
> > > > + fb_size = ALIGN(sizes->surface_width * bytespp, 64) *
> > > > + sizes->surface_height;
> > > > + fb_size = ALIGN(fb_size, PAGE_SIZE);
> > > > +
> > > > + if (fb_size > dev_priv->vram_stolen_size) {
> > >
> > > psb_gtt_alloc_range() already aligns by PAGE_SIZE for us. Looks like
> > > we align a couple of times extra for luck. This needs cleaning up
> > > instead of adding even more aligns.
> >
> > I'm not sure this is really for luck. As far as I can see, we need to do it
> > properly for this size estimation since it's the final size that will be
> > allocated (and thus needs to be available in whole).
Ok now I understand what you meant. Actually vram_stolen_size is
always page aligned so fb_size doesn't need any page alignment here.
There is also no need to align for psbfb_create() since it also takes
care of this.
> >
> > For the other times there is explicit alignment, they seem justified too:
> > - in psb_gem_create: it is common to pass the aligned size when creating the
> > associated GEM object with drm_gem_object_init, even though it's probably
> > not
> > crucial given that this is not where allocation actually happens;
> > - in psbfb_create: the full size is apparently only really used to memset 0
> > the allocated buffer. I think this makes sense for security reasons (and
> > not
> > leak previous contents in the additional space required for alignment).
What I would prefer is to have a single place where the alignment is
made so any hardware requirements would be transparent to the rest of
the code.
Best would be if alignment is only made in psb_gtt_alloc_range() and
then store the actual size in struct gtt_range. That way we can just
pass along that value to memset() and drm_gem_object_init() without
caring about how it is adjusted.
> >
> > What strikes me however is that each call to psb_gtt_alloc_range takes the
> > alignment as a parameter when it's really always PAGE_SIZE, so it should
> > probably just be hardcoded in the call to allocate_resource.
This is a remnant from trying to add support for 2D and/or overlay
planes (don't really remember). Doesn't matter if it stays or goes
away.
> >
> > What do you think?
I suppose most of this is outside the scope of what you're trying to
do so we can just leave it as is and I can clean it up later.
> >
> > > Your size calculation looks correct and indeed makes my 1024x600 +
> > > 1920x1080 setup actually display something, but for some reason I get
> > > an incorrect panning on the smaller screen and st
Re: [PATCH v3 00/23] mm/gup: track dma-pinned pages: FOLL_PIN, FOLL_LONGTERM
On Wed 13-11-19 01:02:02, John Hubbard wrote: > On 11/13/19 12:22 AM, Daniel Vetter wrote: > ... > > > > Why are we doing this? I think things got confused here someplace, as > > > > > > > > > Because: > > > > > > a) These need put_page() calls, and > > > > > > b) there is no put_pages() call, but there is a release_pages() call that > > > is, arguably, what put_pages() would be. > > > > > > > > > > the comment still says: > > > > > > > > /** > > > > * put_user_page() - release a gup-pinned page > > > > * @page:pointer to page to be released > > > > * > > > > * Pages that were pinned via get_user_pages*() must be released via > > > > * either put_user_page(), or one of the put_user_pages*() routines > > > > * below. > > > > > > > > > Ohhh, I missed those comments. They need to all be changed over to > > > say "pages that were pinned via pin_user_pages*() or > > > pin_longterm_pages*() must be released via put_user_page*()." > > > > > > The get_user_pages*() pages must still be released via put_page. > > > > > > The churn is due to a fairly significant change in strategy, whis > > > is: instead of changing all get_user_pages*() sites to call > > > put_user_page(), change selected sites to call pin_user_pages*() or > > > pin_longterm_pages*(), plus put_user_page(). > > > > Can't we call this unpin_user_page then, for some symmetry? Or is that > > even more churn? > > > > Looking from afar the naming here seems really confusing. > > > That look from afar is valuable, because I'm too close to the problem to see > how the naming looks. :) > > unpin_user_page() sounds symmetrical. It's true that it would cause more > churn (which is why I started off with a proposal that avoids changing the > names of put_user_page*() APIs). But OTOH, the amount of churn is proportional > to the change in direction here, and it's really only 10 or 20 lines changed, > in the end. > > So I'm open to changing to that naming. It would be nice to hear what others > prefer, too... FWIW I'd find unpin_user_page() also better than put_user_page() as a counterpart to pin_user_pages(). Honza -- Jan Kara SUSE Labs, CR ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 09/23] mm/gup: introduce pin_user_pages*() and FOLL_PIN
On Tue 12-11-19 20:26:56, John Hubbard wrote:
> Introduce pin_user_pages*() variations of get_user_pages*() calls,
> and also pin_longterm_pages*() variations.
>
> These variants all set FOLL_PIN, which is also introduced, and
> thoroughly documented.
>
> The pin_longterm*() variants also set FOLL_LONGTERM, in addition
> to FOLL_PIN:
>
> pin_user_pages()
> pin_user_pages_remote()
> pin_user_pages_fast()
>
> pin_longterm_pages()
> pin_longterm_pages_remote()
> pin_longterm_pages_fast()
>
> All pages that are pinned via the above calls, must be unpinned via
> put_user_page().
>
> The underlying rules are:
>
> * These are gup-internal flags, so the call sites should not directly
> set FOLL_PIN nor FOLL_LONGTERM. That behavior is enforced with
> assertions, for the new FOLL_PIN flag. However, for the pre-existing
> FOLL_LONGTERM flag, which has some call sites that still directly
> set FOLL_LONGTERM, there is no assertion yet.
>
> * Call sites that want to indicate that they are going to do DirectIO
> ("DIO") or something with similar characteristics, should call a
> get_user_pages()-like wrapper call that sets FOLL_PIN. These wrappers
> will:
> * Start with "pin_user_pages" instead of "get_user_pages". That
> makes it easy to find and audit the call sites.
> * Set FOLL_PIN
>
> * For pages that are received via FOLL_PIN, those pages must be returned
> via put_user_page().
>
> Thanks to Jan Kara and Vlastimil Babka for explaining the 4 cases
> in this documentation. (I've reworded it and expanded upon it.)
>
> Reviewed-by: Mike Rapoport # Documentation
> Reviewed-by: Jérôme Glisse
> Cc: Jonathan Corbet
> Cc: Ira Weiny
> Signed-off-by: John Hubbard
Thanks for the documentation. It looks great!
> diff --git a/mm/gup.c b/mm/gup.c
> index 83702b2e86c8..4409e84dff51 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -201,6 +201,10 @@ static struct page *follow_page_pte(struct
> vm_area_struct *vma,
> spinlock_t *ptl;
> pte_t *ptep, pte;
>
> + /* FOLL_GET and FOLL_PIN are mutually exclusive. */
> + if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
> + (FOLL_PIN | FOLL_GET)))
> + return ERR_PTR(-EINVAL);
> retry:
> if (unlikely(pmd_bad(*pmd)))
> return no_page_table(vma, flags);
How does FOLL_PIN result in grabbing (at least normal, for now) page reference?
I didn't find that anywhere in this patch but it is a prerequisite to
converting any user to pin_user_pages() interface, right?
> +/**
> + * pin_user_pages_fast() - pin user pages in memory without taking locks
> + *
> + * Nearly the same as get_user_pages_fast(), except that FOLL_PIN is set. See
> + * get_user_pages_fast() for documentation on the function arguments, because
> + * the arguments here are identical.
> + *
> + * FOLL_PIN means that the pages must be released via put_user_page(). Please
> + * see Documentation/vm/pin_user_pages.rst for further details.
> + *
> + * This is intended for Case 1 (DIO) in Documentation/vm/pin_user_pages.rst.
> It
> + * is NOT intended for Case 2 (RDMA: long-term pins).
> + */
> +int pin_user_pages_fast(unsigned long start, int nr_pages,
> + unsigned int gup_flags, struct page **pages)
> +{
> + /* FOLL_GET and FOLL_PIN are mutually exclusive. */
> + if (WARN_ON_ONCE(gup_flags & FOLL_GET))
> + return -EINVAL;
> +
> + gup_flags |= FOLL_PIN;
> + return internal_get_user_pages_fast(start, nr_pages, gup_flags, pages);
> +}
> +EXPORT_SYMBOL_GPL(pin_user_pages_fast);
I was somewhat wondering about the number of functions you add here. So we
have:
pin_user_pages()
pin_user_pages_fast()
pin_user_pages_remote()
and then longterm variants:
pin_longterm_pages()
pin_longterm_pages_fast()
pin_longterm_pages_remote()
and obviously we have gup like:
get_user_pages()
get_user_pages_fast()
get_user_pages_remote()
... and some other gup variants ...
I think we really should have pin_* vs get_* variants as they are very
different in terms of guarantees and after conversion, any use of get_*
variant in non-mm code should be closely scrutinized. OTOH pin_longterm_*
don't look *that* useful to me and just using pin_* instead with
FOLL_LONGTERM flag would look OK to me and somewhat reduce the number of
functions which is already large enough? What do people think? I don't feel
too strongly about this but wanted to bring this up.
Honza
--
Jan Kara
SUSE Labs, CR
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 04/23] mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages
On Tue 12-11-19 20:26:51, John Hubbard wrote:
> An upcoming patch changes and complicates the refcounting and
> especially the "put page" aspects of it. In order to keep
> everything clean, refactor the devmap page release routines:
>
> * Rename put_devmap_managed_page() to page_is_devmap_managed(),
> and limit the functionality to "read only": return a bool,
> with no side effects.
>
> * Add a new routine, put_devmap_managed_page(), to handle checking
> what kind of page it is, and what kind of refcount handling it
> requires.
>
> * Rename __put_devmap_managed_page() to free_devmap_managed_page(),
> and limit the functionality to unconditionally freeing a devmap
> page.
>
> This is originally based on a separate patch by Ira Weiny, which
> applied to an early version of the put_user_page() experiments.
> Since then, Jérôme Glisse suggested the refactoring described above.
>
> Suggested-by: Jérôme Glisse
> Signed-off-by: Ira Weiny
> Signed-off-by: John Hubbard
Looks good to me. You can add:
Reviewed-by: Jan Kara
Honza
> ---
> include/linux/mm.h | 27 ---
> mm/memremap.c | 67 --
> 2 files changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a2adf95b3f9c..96228376139c 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -967,9 +967,10 @@ static inline bool is_zone_device_page(const struct page
> *page)
> #endif
>
> #ifdef CONFIG_DEV_PAGEMAP_OPS
> -void __put_devmap_managed_page(struct page *page);
> +void free_devmap_managed_page(struct page *page);
> DECLARE_STATIC_KEY_FALSE(devmap_managed_key);
> -static inline bool put_devmap_managed_page(struct page *page)
> +
> +static inline bool page_is_devmap_managed(struct page *page)
> {
> if (!static_branch_unlikely(&devmap_managed_key))
> return false;
> @@ -978,7 +979,6 @@ static inline bool put_devmap_managed_page(struct page
> *page)
> switch (page->pgmap->type) {
> case MEMORY_DEVICE_PRIVATE:
> case MEMORY_DEVICE_FS_DAX:
> - __put_devmap_managed_page(page);
> return true;
> default:
> break;
> @@ -986,6 +986,27 @@ static inline bool put_devmap_managed_page(struct page
> *page)
> return false;
> }
>
> +static inline bool put_devmap_managed_page(struct page *page)
> +{
> + bool is_devmap = page_is_devmap_managed(page);
> +
> + if (is_devmap) {
> + int count = page_ref_dec_return(page);
> +
> + /*
> + * devmap page refcounts are 1-based, rather than 0-based: if
> + * refcount is 1, then the page is free and the refcount is
> + * stable because nobody holds a reference on the page.
> + */
> + if (count == 1)
> + free_devmap_managed_page(page);
> + else if (!count)
> + __put_page(page);
> + }
> +
> + return is_devmap;
> +}
> +
> #else /* CONFIG_DEV_PAGEMAP_OPS */
> static inline bool put_devmap_managed_page(struct page *page)
> {
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 03ccbdfeb697..bc7e2a27d025 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -410,48 +410,39 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn,
> EXPORT_SYMBOL_GPL(get_dev_pagemap);
>
> #ifdef CONFIG_DEV_PAGEMAP_OPS
> -void __put_devmap_managed_page(struct page *page)
> +void free_devmap_managed_page(struct page *page)
> {
> - int count = page_ref_dec_return(page);
> + /* Clear Active bit in case of parallel mark_page_accessed */
> + __ClearPageActive(page);
> + __ClearPageWaiters(page);
> +
> + mem_cgroup_uncharge(page);
>
> /*
> - * If refcount is 1 then page is freed and refcount is stable as nobody
> - * holds a reference on the page.
> + * When a device_private page is freed, the page->mapping field
> + * may still contain a (stale) mapping value. For example, the
> + * lower bits of page->mapping may still identify the page as
> + * an anonymous page. Ultimately, this entire field is just
> + * stale and wrong, and it will cause errors if not cleared.
> + * One example is:
> + *
> + * migrate_vma_pages()
> + *migrate_vma_insert_page()
> + * page_add_new_anon_rmap()
> + *__page_set_anon_rmap()
> + * ...checks page->mapping, via PageAnon(page) call,
> + *and incorrectly concludes that the page is an
> + *anonymous page. Therefore, it incorrectly,
> + *silently fails to set up the new anon rmap.
> + *
> + * For other types of ZONE_DEVICE pages, migration is either
> + * handled differently or not done at all, so there is no need
> + * to clear page->mapping.
>*/
> - if (count == 1) {
>
Re: [PATCH v4 03/23] mm/gup: move try_get_compound_head() to top, fix minor issues
On Tue 12-11-19 20:26:50, John Hubbard wrote:
> An upcoming patch uses try_get_compound_head() more widely,
> so move it to the top of gup.c.
>
> Also fix a tiny spelling error and a checkpatch.pl warning.
>
> Signed-off-by: John Hubbard
Looks good. You can add:
Reviewed-by: Jan Kara
Honza
> ---
> mm/gup.c | 29 +++--
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 199da99e8ffc..933524de6249 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -29,6 +29,21 @@ struct follow_page_context {
> unsigned int page_mask;
> };
>
> +/*
> + * Return the compound head page with ref appropriately incremented,
> + * or NULL if that failed.
> + */
> +static inline struct page *try_get_compound_head(struct page *page, int refs)
> +{
> + struct page *head = compound_head(page);
> +
> + if (WARN_ON_ONCE(page_ref_count(head) < 0))
> + return NULL;
> + if (unlikely(!page_cache_add_speculative(head, refs)))
> + return NULL;
> + return head;
> +}
> +
> /**
> * put_user_pages_dirty_lock() - release and optionally dirty gup-pinned
> pages
> * @pages: array of pages to be maybe marked dirty, and definitely released.
> @@ -1793,20 +1808,6 @@ static void __maybe_unused undo_dev_pagemap(int *nr,
> int nr_start,
> }
> }
>
> -/*
> - * Return the compund head page with ref appropriately incremented,
> - * or NULL if that failed.
> - */
> -static inline struct page *try_get_compound_head(struct page *page, int refs)
> -{
> - struct page *head = compound_head(page);
> - if (WARN_ON_ONCE(page_ref_count(head) < 0))
> - return NULL;
> - if (unlikely(!page_cache_add_speculative(head, refs)))
> - return NULL;
> - return head;
> -}
> -
> #ifdef CONFIG_ARCH_HAS_PTE_SPECIAL
> static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
>unsigned int flags, struct page **pages, int *nr)
> --
> 2.24.0
>
--
Jan Kara
SUSE Labs, CR
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 01/23] mm/gup: pass flags arg to __gup_device_* functions
On Tue 12-11-19 20:26:48, John Hubbard wrote:
> A subsequent patch requires access to gup flags, so
> pass the flags argument through to the __gup_device_*
> functions.
>
> Also placate checkpatch.pl by shortening a nearby line.
>
> Reviewed-by: Jérôme Glisse
> Reviewed-by: Ira Weiny
> Cc: Kirill A. Shutemov
> Signed-off-by: John Hubbard
Looks good! You can add:
Reviewed-by: Jan Kara
Honza
> ---
> mm/gup.c | 28 ++--
> 1 file changed, 18 insertions(+), 10 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 8f236a335ae9..85caf76b3012 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1890,7 +1890,8 @@ static int gup_pte_range(pmd_t pmd, unsigned long addr,
> unsigned long end,
>
> #if defined(CONFIG_ARCH_HAS_PTE_DEVMAP) &&
> defined(CONFIG_TRANSPARENT_HUGEPAGE)
> static int __gup_device_huge(unsigned long pfn, unsigned long addr,
> - unsigned long end, struct page **pages, int *nr)
> + unsigned long end, unsigned int flags,
> + struct page **pages, int *nr)
> {
> int nr_start = *nr;
> struct dev_pagemap *pgmap = NULL;
> @@ -1916,13 +1917,14 @@ static int __gup_device_huge(unsigned long pfn,
> unsigned long addr,
> }
>
> static int __gup_device_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
> - unsigned long end, struct page **pages, int *nr)
> + unsigned long end, unsigned int flags,
> + struct page **pages, int *nr)
> {
> unsigned long fault_pfn;
> int nr_start = *nr;
>
> fault_pfn = pmd_pfn(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT);
> - if (!__gup_device_huge(fault_pfn, addr, end, pages, nr))
> + if (!__gup_device_huge(fault_pfn, addr, end, flags, pages, nr))
> return 0;
>
> if (unlikely(pmd_val(orig) != pmd_val(*pmdp))) {
> @@ -1933,13 +1935,14 @@ static int __gup_device_huge_pmd(pmd_t orig, pmd_t
> *pmdp, unsigned long addr,
> }
>
> static int __gup_device_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,
> - unsigned long end, struct page **pages, int *nr)
> + unsigned long end, unsigned int flags,
> + struct page **pages, int *nr)
> {
> unsigned long fault_pfn;
> int nr_start = *nr;
>
> fault_pfn = pud_pfn(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT);
> - if (!__gup_device_huge(fault_pfn, addr, end, pages, nr))
> + if (!__gup_device_huge(fault_pfn, addr, end, flags, pages, nr))
> return 0;
>
> if (unlikely(pud_val(orig) != pud_val(*pudp))) {
> @@ -1950,14 +1953,16 @@ static int __gup_device_huge_pud(pud_t orig, pud_t
> *pudp, unsigned long addr,
> }
> #else
> static int __gup_device_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr,
> - unsigned long end, struct page **pages, int *nr)
> + unsigned long end, unsigned int flags,
> + struct page **pages, int *nr)
> {
> BUILD_BUG();
> return 0;
> }
>
> static int __gup_device_huge_pud(pud_t pud, pud_t *pudp, unsigned long addr,
> - unsigned long end, struct page **pages, int *nr)
> + unsigned long end, unsigned int flags,
> + struct page **pages, int *nr)
> {
> BUILD_BUG();
> return 0;
> @@ -2062,7 +2067,8 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp,
> unsigned long addr,
> if (pmd_devmap(orig)) {
> if (unlikely(flags & FOLL_LONGTERM))
> return 0;
> - return __gup_device_huge_pmd(orig, pmdp, addr, end, pages, nr);
> + return __gup_device_huge_pmd(orig, pmdp, addr, end, flags,
> + pages, nr);
> }
>
> refs = 0;
> @@ -2092,7 +2098,8 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp,
> unsigned long addr,
> }
>
> static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr,
> - unsigned long end, unsigned int flags, struct page **pages, int
> *nr)
> + unsigned long end, unsigned int flags,
> + struct page **pages, int *nr)
> {
> struct page *head, *page;
> int refs;
> @@ -2103,7 +2110,8 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp,
> unsigned long addr,
> if (pud_devmap(orig)) {
> if (unlikely(flags & FOLL_LONGTERM))
> return 0;
> - return __gup_device_huge_pud(orig, pudp, addr, end, pages, nr);
> + return __gup_device_huge_pud(orig, pudp, addr, end, flags,
> + pages, nr);
> }
>
> refs = 0;
> --
> 2.24.0
>
--
Jan Kara
SUSE Labs, CR
___
dri-devel mailing list
dri-devel@lis
Re: [PATCH 4/5] power: avs: smartreflex: Remove superfluous cast in debugfs_create_file() call
On Monday, October 21, 2019 4:51:48 PM CET Geert Uytterhoeven wrote:
> There is no need to cast a typed pointer to a void pointer when calling
> a function that accepts the latter. Remove it, as the cast prevents
> further compiler checks.
>
> Signed-off-by: Geert Uytterhoeven
> ---
> drivers/power/avs/smartreflex.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/power/avs/smartreflex.c b/drivers/power/avs/smartreflex.c
> index 4684e7df833a81e9..5376f3d22f31eade 100644
> --- a/drivers/power/avs/smartreflex.c
> +++ b/drivers/power/avs/smartreflex.c
> @@ -905,7 +905,7 @@ static int omap_sr_probe(struct platform_device *pdev)
> sr_info->dbg_dir = debugfs_create_dir(sr_info->name, sr_dbg_dir);
>
> debugfs_create_file("autocomp", S_IRUGO | S_IWUSR, sr_info->dbg_dir,
> - (void *)sr_info, &pm_sr_fops);
> + sr_info, &pm_sr_fops);
> debugfs_create_x32("errweight", S_IRUGO, sr_info->dbg_dir,
> &sr_info->err_weight);
> debugfs_create_x32("errmaxlimit", S_IRUGO, sr_info->dbg_dir,
>
Applying as 5.5 material, thanks!
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 02/23] mm/gup: factor out duplicate code from four routines
On Tue 12-11-19 20:26:49, John Hubbard wrote:
> There are four locations in gup.c that have a fair amount of code
> duplication. This means that changing one requires making the same
> changes in four places, not to mention reading the same code four
> times, and wondering if there are subtle differences.
>
> Factor out the common code into static functions, thus reducing the
> overall line count and the code's complexity.
>
> Also, take the opportunity to slightly improve the efficiency of the
> error cases, by doing a mass subtraction of the refcount, surrounded
> by get_page()/put_page().
>
> Also, further simplify (slightly), by waiting until the the successful
> end of each routine, to increment *nr.
>
> Reviewed-by: Jérôme Glisse
> Cc: Ira Weiny
> Cc: Christoph Hellwig
> Cc: Aneesh Kumar K.V
> Signed-off-by: John Hubbard
> diff --git a/mm/gup.c b/mm/gup.c
> index 85caf76b3012..199da99e8ffc 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1969,6 +1969,34 @@ static int __gup_device_huge_pud(pud_t pud, pud_t
> *pudp, unsigned long addr,
> }
> #endif
>
> +static int __record_subpages(struct page *page, unsigned long addr,
> + unsigned long end, struct page **pages, int nr)
> +{
> + int nr_recorded_pages = 0;
> +
> + do {
> + pages[nr] = page;
> + nr++;
> + page++;
> + nr_recorded_pages++;
> + } while (addr += PAGE_SIZE, addr != end);
> + return nr_recorded_pages;
> +}
Why don't you pass in already pages + nr?
> +
> +static void put_compound_head(struct page *page, int refs)
> +{
> + /* Do a get_page() first, in case refs == page->_refcount */
> + get_page(page);
> + page_ref_sub(page, refs);
> + put_page(page);
> +}
> +
> +static void __huge_pt_done(struct page *head, int nr_recorded_pages, int *nr)
> +{
> + *nr += nr_recorded_pages;
> + SetPageReferenced(head);
> +}
I don't find this last helper very useful. It seems to muddy water more
than necessary...
Other than that the cleanup looks nice to me.
Honza
--
Jan Kara
SUSE Labs, CR
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH][next] backlight: qcom-wled: fix spelling mistake "trigged" -> "triggered"
On Tue, 12 Nov 2019, Colin King wrote: > From: Colin Ian King > > There is a spelling mistake in a dev_err error message. Fix it. > > Signed-off-by: Colin Ian King > --- > drivers/video/backlight/qcom-wled.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied, thanks. -- Lee Jones [李琼斯] Linaro Services Technical Lead Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCHv2 3/4] drm/komeda: use afbc helpers
On Wed, Nov 13, 2019 at 02:01:53AM +, james qian wang (Arm Technology
China) wrote:
> On Fri, Nov 08, 2019 at 04:09:54PM +, Ayan Halder wrote:
> > On Mon, Nov 04, 2019 at 11:12:27PM +0100, Andrzej Pietrasiewicz wrote:
> > > There are afbc helpers available.
> > >
> > > Signed-off-by: Andrzej Pietrasiewicz
> > > ---
> > > .../arm/display/komeda/komeda_format_caps.h | 1 -
> > > .../arm/display/komeda/komeda_framebuffer.c | 44 +++
> > > 2 files changed, 17 insertions(+), 28 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> > > b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> > > index 32273cf18f7c..607eea80e60c 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_format_caps.h
> > > @@ -33,7 +33,6 @@
> > >
> > > #define AFBC_TH_LAYOUT_ALIGNMENT 8
> > > #define AFBC_HEADER_SIZE 16
> > > -#define AFBC_SUPERBLK_ALIGNMENT 128
> > > #define AFBC_SUPERBLK_PIXELS 256
> > > #define AFBC_BODY_START_ALIGNMENT1024
> > > #define AFBC_TH_BODY_START_ALIGNMENT 4096
> > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
> > > b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
> > > index 1b01a625f40e..e9c87551a5b8 100644
> > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
> > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_framebuffer.c
> > > @@ -4,6 +4,7 @@
> > > * Author: James.Qian.Wang
> > > *
> > > */
> > > +#include
> > > #include
> > > #include
> > > #include
> > > @@ -43,8 +44,7 @@ komeda_fb_afbc_size_check(struct komeda_fb *kfb, struct
> > > drm_file *file,
> > > struct drm_framebuffer *fb = &kfb->base;
> > > const struct drm_format_info *info = fb->format;
> > > struct drm_gem_object *obj;
> > > - u32 alignment_w = 0, alignment_h = 0, alignment_header, n_blocks, bpp;
> > > - u64 min_size;
> > > + u32 alignment_w = 0, alignment_h = 0, alignment_header, bpp;
> > >
> > > obj = drm_gem_object_lookup(file, mode_cmd->handles[0]);
> > > if (!obj) {
> > > @@ -52,19 +52,15 @@ komeda_fb_afbc_size_check(struct komeda_fb *kfb,
> > > struct drm_file *file,
> > > return -ENOENT;
> > > }
> > >
> > > - switch (fb->modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK) {
> > > - case AFBC_FORMAT_MOD_BLOCK_SIZE_32x8:
> > > - alignment_w = 32;
> > > - alignment_h = 8;
> > > - break;
> > > - case AFBC_FORMAT_MOD_BLOCK_SIZE_16x16:
> > > - alignment_w = 16;
> > > - alignment_h = 16;
> > > - break;
> > > - default:
> > > - WARN(1, "Invalid AFBC_FORMAT_MOD_BLOCK_SIZE: %lld.\n",
> > > - fb->modifier & AFBC_FORMAT_MOD_BLOCK_SIZE_MASK);
> > > - break;
> > > + if (!drm_afbc_get_superblk_wh(fb->modifier, &alignment_w, &alignment_h))
> > > + return -EINVAL;
> > > +
> > > + if ((alignment_w != 16 || alignment_h != 16) &&
> > > + (alignment_w != 32 || alignment_h != 8)) {
> > > + DRM_DEBUG_KMS("Unsupported afbc tile w/h [%d/%d]\n",
> > > + alignment_w, alignment_h);
> > > +
> > > + return -EINVAL;
> > To be honest, the previous code looks much more readable
> > > }
> > >
> > > /* tiled header afbc */
> > > @@ -84,20 +80,14 @@ komeda_fb_afbc_size_check(struct komeda_fb *kfb,
> > > struct drm_file *file,
> > > goto check_failed;
> > > }
> > >
> > > - n_blocks = (kfb->aligned_w * kfb->aligned_h) / AFBC_SUPERBLK_PIXELS;
> > > - kfb->offset_payload = ALIGN(n_blocks * AFBC_HEADER_SIZE,
> > > - alignment_header);
> > > -
> > > bpp = komeda_get_afbc_format_bpp(info, fb->modifier);
> > > - kfb->afbc_size = kfb->offset_payload + n_blocks *
> > > - ALIGN(bpp * AFBC_SUPERBLK_PIXELS / 8,
> > > -AFBC_SUPERBLK_ALIGNMENT);
> > > - min_size = kfb->afbc_size + fb->offsets[0];
> > > - if (min_size > obj->size) {
> > > - DRM_DEBUG_KMS("afbc size check failed, obj_size: 0x%zx.
> > > min_size 0x%llx.\n",
> > > - obj->size, min_size);
> > We need kfb->offset_payload and kfb->afbc_size to set some registers
> > in d71_layer_update(). At this moment I feel like punching myself for
> > making the suggestion to consider abstracting some of the komeda's afbc
> > checks. To me it does not look like komeda(in the current shape) can take
> > much advantage of the generic _afbc_fb_check() function (as was suggested
> > previously by Danvet).
> >
> > However, I will let james.qian.w...@arm.com,
> > mihail.atanas...@arm.com, ben.da...@arm.com comment here to see if
> > there could be a way of abstracting the afbc bits from komeda.
> >
>
> Hi all:
>
> Since the current generic drm_afbc helpers only support afbc_1.1, but
> komeda needs support both afbc1.1/1.2, so I think we can:
> - Add afbc1.2 support to drm afbc helpers.
> - for the afbc_pay
Re: [PATCH 1/4] drm/udl: Replace fbdev code with generic emulation
On Wed, Nov 13, 2019 at 10:51:51AM +0100, Thomas Zimmermann wrote:
> Hi
>
> Am 12.11.19 um 16:13 schrieb Daniel Vetter:
> > On Tue, Nov 12, 2019 at 3:51 PM Thomas Zimmermann
> > wrote:
> >>
> >> Hi
> >>
> >> Am 12.11.19 um 15:31 schrieb Daniel Vetter:
> >>> On Tue, Nov 12, 2019 at 3:03 PM Thomas Zimmermann
> >>> wrote:
>
> Hi
>
> Am 12.11.19 um 14:40 schrieb Daniel Vetter:
> > On Tue, Nov 12, 2019 at 12:55 PM Thomas Zimmermann
> > wrote:
> >>
> >> Hi
> >>
> >> Am 08.11.19 um 16:37 schrieb Noralf Trønnes:
> >>>
> >>>
> >>> Den 08.11.2019 13.33, skrev Thomas Zimmermann:
> The udl driver can use the generic fbdev implementation. Convert it.
>
> Signed-off-by: Thomas Zimmermann
> ---
> >>>
> diff --git a/drivers/gpu/drm/udl/udl_drv.c
> b/drivers/gpu/drm/udl/udl_drv.c
> index 563cc5809e56..55c0f9dfee29 100644
> --- a/drivers/gpu/drm/udl/udl_drv.c
> +++ b/drivers/gpu/drm/udl/udl_drv.c
> >>>
> @@ -47,6 +48,8 @@ static struct drm_driver driver = {
> .driver_features = DRIVER_MODESET | DRIVER_GEM,
> .release = udl_driver_release,
>
> +.lastclose = drm_fb_helper_lastclose,
> +
> >>>
> >>> No need to set this, it's already wired up:
> >>>
> >>> drm_lastclose -> drm_client_dev_restore -> drm_fbdev_client_restore ->
> >>> drm_fb_helper_lastclose
> >>>
> /* gem hooks */
> .gem_create_object = udl_driver_gem_create_object,
>
> >>>
> diff --git a/drivers/gpu/drm/udl/udl_fb.c
> b/drivers/gpu/drm/udl/udl_fb.c
> index f8153b726343..afe74f892a2b 100644
> --- a/drivers/gpu/drm/udl/udl_fb.c
> +++ b/drivers/gpu/drm/udl/udl_fb.c
> @@ -20,19 +20,9 @@
>
> #include "udl_drv.h"
>
> -#define DL_DEFIO_WRITE_DELAY(HZ/20) /* fb_deferred_io.delay in
> jiffies */
> -
> -static int fb_defio = 0; /* Optionally enable experimental
> fb_defio mmap support */
> static int fb_bpp = 16;
>
> module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
> >>>
> >>> Maybe fb_bpp can be dropped too?
> >>
> >> Sure, makes sense.
> >>
> >> The driver exposes a preferred color depth of 24 bpp, which we may want
> >> to change to 16 then. The internal framebuffer is only 16 bpp anyway.
> >
> > Just something that crossed my mind: Should we ensure that the
> > preferred format of the primary plane (should be the first in the
> > format array) matches up with the preferred bpp setting? Maybe even
> > enforce that for drivers with an explicit primary plane (i.e. atomic
> > drivers). I think tiny drivers get this right already.
>
> IMHO that makes if the userspace can handle it. The preferred bpp could
> also be retrieved from the formats array automatically. What about HW
> with multiple CRTCs with different format defaults (sounds weird, I
> know)?
> >>>
> >>> Ime I haven't seen such a case yet. What I have seen is that the most
> >>> preferred format might be some fancy compressed format, which not all
> >>> formats support. But which you can't render into without mesa anyway,
> >>> so really doesn't matter for preferred bpp.
> >>>
> WRT udl: For v3 of this patchset I've set the preferred color depth to
> 32 bpp; although the internal FB is always at 16 bpp. Because when I
> tested with a dual-screen setup (radeon + udl) X11 didn't support the 16
> bpp output on the second screen (the one driven by udl). Only setting
> both screen to 32 bpp worked out of the box. And the preferred 24 bpp
> are not even supported by udl.
> >>>
> >>> Uh, if we can only set preferred bpp to make X happy, and X can only
> >>> support one preferred bpp, then everyone needs to set 32bit. Which
> >>> defeats the point (and we'd need to hardcode it to 32bpp). Is this
> >>> really the case?
> >>
> >> I guess it would have worked if both screens preferred 16 bpp.
> >
> > Just noticed that current depth is 24 bpp ... does that work with
> > current X? If not I think we should actually set it to 16 bpp, and fix
> > up X. Not as in "make it handle multi-bpp", that's too hard, but make
> > it pick a common format that works for all drivers (which usually
> > means go with 32bpp). Since if we just go with 32bpp to paper over X,
> > then the preferred bpp uapi becomes meaningless.
>
> The good news is that it apparently works with planes correctly configured.
>
> I tested with udl being converted to simple pipe and universal planes.
> It exposes RGB565, XRGB and preferred_depth=16. X did the correct
> thing and choose a format the works with radeon and udl.
Awesome!
> For the next iteration of the fbdev conversion, I'll just k
Re: [PATCH v3 00/23] mm/gup: track dma-pinned pages: FOLL_PIN, FOLL_LONGTERM
On Wed, Nov 13, 2019 at 11:12:10AM +0100, Jan Kara wrote: > On Wed 13-11-19 01:02:02, John Hubbard wrote: > > On 11/13/19 12:22 AM, Daniel Vetter wrote: > > ... > > > > > Why are we doing this? I think things got confused here someplace, as > > > > > > > > > > > > Because: > > > > > > > > a) These need put_page() calls, and > > > > > > > > b) there is no put_pages() call, but there is a release_pages() call > > > > that > > > > is, arguably, what put_pages() would be. > > > > > > > > > > > > > the comment still says: > > > > > > > > > > /** > > > > > * put_user_page() - release a gup-pinned page > > > > > * @page:pointer to page to be released > > > > > * > > > > > * Pages that were pinned via get_user_pages*() must be released via > > > > > * either put_user_page(), or one of the put_user_pages*() routines > > > > > * below. > > > > > > > > > > > > Ohhh, I missed those comments. They need to all be changed over to > > > > say "pages that were pinned via pin_user_pages*() or > > > > pin_longterm_pages*() must be released via put_user_page*()." > > > > > > > > The get_user_pages*() pages must still be released via put_page. > > > > > > > > The churn is due to a fairly significant change in strategy, whis > > > > is: instead of changing all get_user_pages*() sites to call > > > > put_user_page(), change selected sites to call pin_user_pages*() or > > > > pin_longterm_pages*(), plus put_user_page(). > > > > > > Can't we call this unpin_user_page then, for some symmetry? Or is that > > > even more churn? > > > > > > Looking from afar the naming here seems really confusing. > > > > > > That look from afar is valuable, because I'm too close to the problem to see > > how the naming looks. :) > > > > unpin_user_page() sounds symmetrical. It's true that it would cause more > > churn (which is why I started off with a proposal that avoids changing the > > names of put_user_page*() APIs). But OTOH, the amount of churn is > > proportional > > to the change in direction here, and it's really only 10 or 20 lines > > changed, > > in the end. > > > > So I'm open to changing to that naming. It would be nice to hear what others > > prefer, too... > > FWIW I'd find unpin_user_page() also better than put_user_page() as a > counterpart to pin_user_pages(). One more point from afar on pin/unpin: We use that a lot in graphics for permanently pinned graphics buffer objects. Which really only should be used for scanout. So at least graphics folks should have an appropriate mindset and try to make sure we don't overuse this stuff. -Daniel -- 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
[PATCH v4 2/3] drm/fb-helper: Remove drm_fb_helper_unlink_fbi()
There are no callers of drm_fb_helper_unlink_fbi() left. Remove the
function.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/drm_fb_helper.c | 16 +---
include/drm/drm_fb_helper.h | 6 --
2 files changed, 1 insertion(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 1038a2f0639e..eb97f34a15ea 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -563,8 +563,7 @@ EXPORT_SYMBOL(drm_fb_helper_unregister_fbi);
* drm_fb_helper_fini - finialize a &struct drm_fb_helper
* @fb_helper: driver-allocated fbdev helper, can be NULL
*
- * This cleans up all remaining resources associated with @fb_helper. Must be
- * called after drm_fb_helper_unlink_fbi() was called.
+ * This cleans up all remaining resources associated with @fb_helper.
*/
void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
{
@@ -604,19 +603,6 @@ void drm_fb_helper_fini(struct drm_fb_helper *fb_helper)
}
EXPORT_SYMBOL(drm_fb_helper_fini);
-/**
- * drm_fb_helper_unlink_fbi - wrapper around unlink_framebuffer
- * @fb_helper: driver-allocated fbdev helper, can be NULL
- *
- * A wrapper around unlink_framebuffer implemented by fbdev core
- */
-void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
-{
- if (fb_helper && fb_helper->fbdev)
- unlink_framebuffer(fb_helper->fbdev);
-}
-EXPORT_SYMBOL(drm_fb_helper_unlink_fbi);
-
static bool drm_fbdev_use_shadow_fb(struct drm_fb_helper *fb_helper)
{
struct drm_device *dev = fb_helper->dev;
diff --git a/include/drm/drm_fb_helper.h b/include/drm/drm_fb_helper.h
index e3a75ff07390..1c2e0c3cf857 100644
--- a/include/drm/drm_fb_helper.h
+++ b/include/drm/drm_fb_helper.h
@@ -231,8 +231,6 @@ void drm_fb_helper_fill_info(struct fb_info *info,
struct drm_fb_helper *fb_helper,
struct drm_fb_helper_surface_size *sizes);
-void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper);
-
void drm_fb_helper_deferred_io(struct fb_info *info,
struct list_head *pagelist);
@@ -356,10 +354,6 @@ static inline int drm_fb_helper_ioctl(struct fb_info
*info, unsigned int cmd,
return 0;
}
-static inline void drm_fb_helper_unlink_fbi(struct drm_fb_helper *fb_helper)
-{
-}
-
static inline void drm_fb_helper_deferred_io(struct fb_info *info,
struct list_head *pagelist)
{
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 0/3] drm/udl: Replace fbdev by generic emulation
The udl driver can use the generic fbdev emulation. After conversion, a number of cleanups can be applied. The fbdev conversion is in patch 1. As udl was the only remaining external user of unlink_framebuffer(), that function now becomes an internal interface of the fbdev code (Patches 2 + 3). V4 of the patchset keeps udl_gem_object_free_object(). The function is still required to clean up after the damage handler mapped a BO. The patchset has been tested by running the console, X11 and Weston on a DisplayLink adapter. v4: * go back to 24 bpp by default, 16 bpp for console * keep udl_gem_object_free_object(); required by damage handler v3: * use 32 bpp by default * use defaults for several callback functions * remove all fb module parameters * remove udl_fbdev_init() v2: * converted udl to SHMEM and recreated fbdev patchset on top Thomas Zimmermann (3): drm/udl: Replace fbdev code with generic emulation drm/fb-helper: Remove drm_fb_helper_unlink_fbi() fbdev: Unexport unlink_framebuffer() drivers/gpu/drm/drm_fb_helper.c | 16 +- drivers/gpu/drm/udl/udl_drv.c | 1 - drivers/gpu/drm/udl/udl_drv.h | 6 - drivers/gpu/drm/udl/udl_fb.c | 282 -- drivers/gpu/drm/udl/udl_main.c| 5 +- drivers/gpu/drm/udl/udl_modeset.c | 1 - drivers/video/fbdev/core/fbmem.c | 3 +- include/drm/drm_fb_helper.h | 6 - include/linux/fb.h| 1 - 9 files changed, 4 insertions(+), 317 deletions(-) -- 2.23.0 ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v4 1/3] drm/udl: Replace fbdev code with generic emulation
The udl driver can use the generic fbdev implementation. Convert it.
v4:
* hardcode console bpp to 16
v3:
* remove module parameter fb_bpp in favor of fbdev's video
* call drm_fbdev_generic_setup() directly; remove udl_fbdev_init()
* use default for struct drm_mode_config_funcs.output_poll_changed
* use default for struct drm_driver.lastclose
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/udl/udl_drv.c | 1 -
drivers/gpu/drm/udl/udl_drv.h | 6 -
drivers/gpu/drm/udl/udl_fb.c | 282 --
drivers/gpu/drm/udl/udl_main.c| 5 +-
drivers/gpu/drm/udl/udl_modeset.c | 1 -
5 files changed, 2 insertions(+), 293 deletions(-)
diff --git a/drivers/gpu/drm/udl/udl_drv.c b/drivers/gpu/drm/udl/udl_drv.c
index 563cc5809e56..2b6d8f6b2e06 100644
--- a/drivers/gpu/drm/udl/udl_drv.c
+++ b/drivers/gpu/drm/udl/udl_drv.c
@@ -119,7 +119,6 @@ static void udl_usb_disconnect(struct usb_interface
*interface)
struct drm_device *dev = usb_get_intfdata(interface);
drm_kms_helper_poll_disable(dev);
- udl_fbdev_unplug(dev);
udl_drop_usb(dev);
drm_dev_unplug(dev);
drm_dev_put(dev);
diff --git a/drivers/gpu/drm/udl/udl_drv.h b/drivers/gpu/drm/udl/udl_drv.h
index 987d99ae2dfa..be585e3e572d 100644
--- a/drivers/gpu/drm/udl/udl_drv.h
+++ b/drivers/gpu/drm/udl/udl_drv.h
@@ -47,8 +47,6 @@ struct urb_list {
size_t size;
};
-struct udl_fbdev;
-
struct udl_device {
struct drm_device drm;
struct device *dev;
@@ -62,7 +60,6 @@ struct udl_device {
struct urb_list urbs;
atomic_t lost_pixels; /* 1 = a render op failed. Need screen refresh */
- struct udl_fbdev *fbdev;
char mode_buf[1024];
uint32_t mode_buf_len;
atomic_t bytes_rendered; /* raw pixel-bytes driver asked to render */
@@ -97,9 +94,6 @@ void udl_urb_completion(struct urb *urb);
int udl_init(struct udl_device *udl);
void udl_fini(struct drm_device *dev);
-int udl_fbdev_init(struct drm_device *dev);
-void udl_fbdev_cleanup(struct drm_device *dev);
-void udl_fbdev_unplug(struct drm_device *dev);
struct drm_framebuffer *
udl_fb_user_fb_create(struct drm_device *dev,
struct drm_file *file,
diff --git a/drivers/gpu/drm/udl/udl_fb.c b/drivers/gpu/drm/udl/udl_fb.c
index f8153b726343..8fe4d8cf3212 100644
--- a/drivers/gpu/drm/udl/udl_fb.c
+++ b/drivers/gpu/drm/udl/udl_fb.c
@@ -13,27 +13,12 @@
#include
#include
-#include
#include
#include
#include
#include "udl_drv.h"
-#define DL_DEFIO_WRITE_DELAY(HZ/20) /* fb_deferred_io.delay in jiffies */
-
-static int fb_defio = 0; /* Optionally enable experimental fb_defio mmap
support */
-static int fb_bpp = 16;
-
-module_param(fb_bpp, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
-module_param(fb_defio, int, S_IWUSR | S_IRUSR | S_IWGRP | S_IRGRP);
-
-struct udl_fbdev {
- struct drm_fb_helper helper; /* must be first */
- struct udl_framebuffer ufb;
- int fb_count;
-};
-
#define DL_ALIGN_UP(x, a) ALIGN(x, a)
#define DL_ALIGN_DOWN(x, a) ALIGN_DOWN(x, a)
@@ -156,123 +141,6 @@ int udl_handle_damage(struct udl_framebuffer *fb, int x,
int y,
return 0;
}
-static int udl_fb_mmap(struct fb_info *info, struct vm_area_struct *vma)
-{
- unsigned long start = vma->vm_start;
- unsigned long size = vma->vm_end - vma->vm_start;
- unsigned long offset;
- unsigned long page, pos;
-
- if (vma->vm_pgoff > (~0UL >> PAGE_SHIFT))
- return -EINVAL;
-
- offset = vma->vm_pgoff << PAGE_SHIFT;
-
- if (offset > info->fix.smem_len || size > info->fix.smem_len - offset)
- return -EINVAL;
-
- pos = (unsigned long)info->fix.smem_start + offset;
-
- pr_debug("mmap() framebuffer addr:%lu size:%lu\n",
- pos, size);
-
- /* We don't want the framebuffer to be mapped encrypted */
- vma->vm_page_prot = pgprot_decrypted(vma->vm_page_prot);
-
- while (size > 0) {
- page = vmalloc_to_pfn((void *)pos);
- if (remap_pfn_range(vma, start, page, PAGE_SIZE, PAGE_SHARED))
- return -EAGAIN;
-
- start += PAGE_SIZE;
- pos += PAGE_SIZE;
- if (size > PAGE_SIZE)
- size -= PAGE_SIZE;
- else
- size = 0;
- }
-
- /* VM_IO | VM_DONTEXPAND | VM_DONTDUMP are set by remap_pfn_range() */
- return 0;
-}
-
-/*
- * It's common for several clients to have framebuffer open simultaneously.
- * e.g. both fbcon and X. Makes things interesting.
- * Assumes caller is holding info->lock (for open and release at least)
- */
-static int udl_fb_open(struct fb_info *info, int user)
-{
- struct udl_fbdev *ufbdev = info->par;
- struct drm_device *dev = ufbdev->ufb.base.dev;
- struct udl_device *udl = to_udl(dev);
-
- /* If the USB devic
[PATCH v4 3/3] fbdev: Unexport unlink_framebuffer()
There are no external callers of unlink_framebuffer() left. Make the
function an internal interface.
Signed-off-by: Thomas Zimmermann
---
drivers/video/fbdev/core/fbmem.c | 3 +--
include/linux/fb.h | 1 -
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 95c32952fa8a..86b06a599f96 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1673,7 +1673,7 @@ static void unbind_console(struct fb_info *fb_info)
console_unlock();
}
-void unlink_framebuffer(struct fb_info *fb_info)
+static void unlink_framebuffer(struct fb_info *fb_info)
{
int i;
@@ -1692,7 +1692,6 @@ void unlink_framebuffer(struct fb_info *fb_info)
fb_info->dev = NULL;
}
-EXPORT_SYMBOL(unlink_framebuffer);
static void do_unregister_framebuffer(struct fb_info *fb_info)
{
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 41e0069eca0a..a6ad528990de 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -606,7 +606,6 @@ extern ssize_t fb_sys_write(struct fb_info *info, const
char __user *buf,
/* drivers/video/fbmem.c */
extern int register_framebuffer(struct fb_info *fb_info);
extern void unregister_framebuffer(struct fb_info *fb_info);
-extern void unlink_framebuffer(struct fb_info *fb_info);
extern int remove_conflicting_pci_framebuffers(struct pci_dev *pdev,
const char *name);
extern int remove_conflicting_framebuffers(struct apertures_struct *a,
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[git pull] vmwgfx-coherent
Dave, Daniel, Take 2 of the coherent memory patches are now ready for pulling. Following the discussion we had after the last merge attempt that had to be reverted, it might make sense to send this to Linus as a pull request separate from other DRM stuff. This time Linus has been heavily involved in the outcome of the -mm patches, and Andrew Morton has acked them for merging through DRM: https://lore.kernel.org/r/20191105195114.f75be5e76763da5546121...@linux-foundation.org/ If you think it's too late for the 5.5 merge window, deferring to 5.6 is not a problem. There is a dependency on drm-misc: This needs to be merged after drm-misc caa478af4812, to avoid a compilation error. Cover message: Graphics APIs like OpenGL 4.4 and Vulkan require the graphics driver to provide coherent graphics memory, meaning that the GPU sees any content written to the coherent memory on the next GPU operation that touches that memory, and the CPU sees any content written by the GPU to that memory immediately after any fence object trailing the GPU operation is signaled. Paravirtual drivers that otherwise require explicit synchronization needs to do this by hooking up dirty tracking to pagefault handlers and buffer object validation. Provide mm helpers needed for this and that also allow for huge pmd- and pud entries (patch 1-3), and the associated vmwgfx code (patch 4-7). The code has been tested and exercised by a tailored version of mesa where we disable all explicit synchronization and assume graphics memory is coherent. The performance loss varies of course; a typical number is around 5%. The following changes since commit 7aef29f4d4613570f413301b0807ea66a21f5e3b: drm/ttm: Convert vm callbacks to helpers (2019-11-06 13:02:00 +0100) are available in the Git repository at: git://people.freedesktop.org/~thomash/linux vmwgfx-coherent for you to fetch changes up to 9ca7d19ff8ba6207bccab46536814fe4839df80a: drm/vmwgfx: Add surface dirty-tracking callbacks (2019-11-06 15:45:32 +0100) Thomas Hellstrom (8): mm: Remove BUG_ON mmap_sem not held from xxx_trans_huge_lock() mm: pagewalk: Take the pagetable lock in walk_pte_range() mm: Add a walk_page_mapping() function to the pagewalk code mm: Add write-protect and clean utilities for address space ranges drm/vmwgfx: Implement an infrastructure for write-coherent resources drm/vmwgfx: Use an RBtree instead of linked list for MOB resources drm/vmwgfx: Implement an infrastructure for read-coherent resources drm/vmwgfx: Add surface dirty-tracking callbacks drivers/gpu/drm/vmwgfx/Kconfig | 1 + drivers/gpu/drm/vmwgfx/Makefile| 2 +- .../drm/vmwgfx/device_include/svga3d_surfacedefs.h | 233 +- drivers/gpu/drm/vmwgfx/vmwgfx_bo.c | 10 +- drivers/gpu/drm/vmwgfx/vmwgfx_drv.h| 44 +- drivers/gpu/drm/vmwgfx/vmwgfx_execbuf.c| 1 - drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c | 488 + drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 193 +++- drivers/gpu/drm/vmwgfx/vmwgfx_resource_priv.h | 13 + drivers/gpu/drm/vmwgfx/vmwgfx_surface.c| 395 - drivers/gpu/drm/vmwgfx/vmwgfx_ttm_glue.c | 15 +- drivers/gpu/drm/vmwgfx/vmwgfx_validation.c | 74 +++- drivers/gpu/drm/vmwgfx/vmwgfx_validation.h | 16 +- include/linux/huge_mm.h| 2 - include/linux/mm.h | 13 +- include/linux/pagewalk.h | 9 + include/uapi/drm/vmwgfx_drm.h | 4 +- mm/Kconfig | 3 + mm/Makefile| 1 + mm/mapping_dirty_helpers.c | 315 + mm/pagewalk.c | 99 - 21 files changed, 1875 insertions(+), 56 deletions(-) create mode 100644 drivers/gpu/drm/vmwgfx/vmwgfx_page_dirty.c create mode 100644 mm/mapping_dirty_helpers.c ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v7 2/7] dt-bindings: display, renesas, du: Document cmms property
Hi Jacopo,
Sorry for spoiling your v7 ;-)
On Wed, Nov 13, 2019 at 11:04 AM Jacopo Mondi wrote:
> Document the newly added 'cmms' property which accepts a list of phandle
renesas,cmms
> and channel index pairs that point to the CMM units available for each
> Display Unit output video channel.
>
> Reviewed-by: Rob Herring
> Reviewed-by: Kieran Bingham
> Reviewed-by: Laurent Pinchart
> Signed-off-by: Jacopo Mondi
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 5/5] drm/komeda: add rate limiting disable to err_verbosity
On Tuesday, 12 November 2019 18:24:16 GMT Daniel Vetter wrote:
> On Tue, Nov 12, 2019 at 2:00 PM Mihail Atanassov
> wrote:
> >
> > On Monday, 11 November 2019 15:53:14 GMT Liviu Dudau wrote:
> > > On Thu, Nov 07, 2019 at 11:42:44AM +, Mihail Atanassov wrote:
> > > > It's possible to get multiple events in a single frame/flip, so add an
> > > > option to print them all.
> > > >
> > > > Reviewed-by: James Qian Wang (Arm Technology China)
> > > >
> > > > Signed-off-by: Mihail Atanassov
> > >
> > > For the whole series:
> > >
> > > Acked-by: Liviu Dudau
> >
> > Thanks, applied to drm-misc-next.
>
> And now komeda doesn't even compile anymore. I'm ... impressed.
>
Mea culpa! Sorry about the breakage, I did build-test but apparently
not the right checkout :/. I'll adjust my workflow to make sure this
doesn't happen again.
> I mean generally people break other people's driver, not their own.
> -Daniel
>
> > >
> > > Best regards,
> > > Liviu
> > >
> > > > ---
> > > >
> > > > v2: Clean up continuation line warning from checkpatch.
> > > >
> > > > drivers/gpu/drm/arm/display/komeda/komeda_dev.h | 2 ++
> > > > drivers/gpu/drm/arm/display/komeda/komeda_event.c | 2 +-
> > > > 2 files changed, 3 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > > b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > > index d9fc9c48859a..15f52e304c08 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_dev.h
> > > > @@ -224,6 +224,8 @@ struct komeda_dev {
> > > > #define KOMEDA_DEV_PRINT_INFO_EVENTS BIT(2)
> > > > /* Dump DRM state on an error or warning event. */
> > > > #define KOMEDA_DEV_PRINT_DUMP_STATE_ON_EVENT BIT(8)
> > > > + /* Disable rate limiting of event prints (normally one per commit)
> > > > */
> > > > +#define KOMEDA_DEV_PRINT_DISABLE_RATELIMIT BIT(12)
> > > > };
> > > >
> > > > static inline bool
> > > > diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_event.c
> > > > b/drivers/gpu/drm/arm/display/komeda/komeda_event.c
> > > > index 7fd624761a2b..bf269683f811 100644
> > > > --- a/drivers/gpu/drm/arm/display/komeda/komeda_event.c
> > > > +++ b/drivers/gpu/drm/arm/display/komeda/komeda_event.c
> > > > @@ -119,7 +119,7 @@ void komeda_print_events(struct komeda_events
> > > > *evts, struct drm_device *dev)
> > > > /* reduce the same msg print, only print the first evt for one
> > > > frame */
> > > > if (evts->global || is_new_frame(evts))
> > > > en_print = true;
> > > > - if (!en_print)
> > > > + if (!(err_verbosity & KOMEDA_DEV_PRINT_DISABLE_RATELIMIT) &&
> > > > !en_print)
> > > > return;
> > > >
> > > > if (err_verbosity & KOMEDA_DEV_PRINT_ERR_EVENTS)
> > > > --
> > > > 2.23.0
> > > >
> > >
> > > --
> > >
> > > | I would like to |
> > > | fix the world, |
> > > | but they're not |
> > > | giving me the |
> > > \ source code! /
> > > ---
> > > ¯\_(ツ)_/¯
> > >
> >
> >
> > --
> > Mihail
> >
> >
> >
>
>
>
--
Mihail
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge: ti-tfp410: switch to using fwnode_gpiod_get_index()
On Mon, Oct 14, 2019 at 8:43 PM Dmitry Torokhov wrote: > Instead of fwnode_get_named_gpiod() that I plan to hide away, let's use > the new fwnode_gpiod_get_index() that mimics gpiod_get_index(), but > works with arbitrary firmware node. > > Reviewed-by: Laurent Pinchart > Signed-off-by: Dmitry Torokhov I applied this with some ACKs to the GPIO devel branch for v5.5. Yours, Linus Walleij ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/gem: Fix mmap fake offset handling for drm_gem_object_funcs.mmap
Hi, > > ... but after looking again I think we are all green here. Given that > > only self-import works we'll only see vram gem objects in the mmap code > > path, which should have everything set up correctly. Same goes for qxl. > > > > All other ttm drivers still use the old mmap code path, so all green > > there too I think. Also I somehow doubt dma-buf mmap vs. drm mmap ends > > up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open() > > which would fire should that be the case. > > > > Do imported dma-bufs hit the drm mmap code path in the first place? > > Wouldn't mmap be handled by the exporting driver? > > drm_gem_dmabuf_mmap -> obj->funcs->mmap -> ttm_bo_mmap_obj > > And I'm not seeing anyone adjusting vm_file->f_mapping anywhere here at all. [ some more code browsing ] Ok, I see. dma-bufs get their own file, their own anon inode and thereby their own address space. So that it used when mmaping the dma-buf. drm filehandle's get the shared address space instead, drm_open() sets it. So, yes, I see the problem. It's not new though, as far I can see the old dma-buf mmap code path doesn't adjust f_mapping anywhere either ... > Note to hit this you need userspace to > - handle2fd on a buffer to create a dma-buf fd > - call mmap directly on that dma-buf fd Hmm, seems for handle2fd I need a dummy gem_prime_get_sg_table function wired up even when not actually exporting/importing anything. So I think neither qxl nor any of the vram drivers allow to trigger that (and no other ttm driver uses the new ttm mmap code yet). So, $subject patch should not make things worse in ttm land. When hacking the bochs driver to have export callbacks (without supporting actual exports) handle2fd + mmap() callback works fine. Didn't verify yet I actually get the correct pages mapped. But maybe mmap() isn't the problem when the correct address space is important for unmap only. Is there a good test case? cheers, Gerd ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge: ti-tfp410: switch to using fwnode_gpiod_get_index()
On Tue, Nov 5, 2019 at 4:41 PM Daniel Vetter wrote: > On Tue, Nov 5, 2019 at 4:29 PM Linus Walleij wrote: > > On Tue, Nov 5, 2019 at 1:40 AM Dmitry Torokhov > > wrote: > > I'm happy to merge it into the GPIO tree if some DRM maintainer can > > provide an ACK. > > Ack. Thanks! > > Getting ACK from DRM people is problematic and a bit of friction in the > > community, DVetter usually advice to seek mutual reviews etc, but IMO > > it would be better if some people felt more compelled to review stuff > > eventually. (And that has the problem that it doesn't scale.) > > This has a review already plus if you merge your implied review. Yeah I missed Laurent's review tag. I needed some kund of consent to take it into the GPIO tree I suppose. > That's more than good enough imo, so not seeing the issue here? No issue. What freaked me out was the option of having to pull in an immutable branch from my GPIO tree into drm-misc. That would have been scary. Keeping it all in my tree works fine. Yours, Linus Walleij ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/gem: Fix mmap fake offset handling for drm_gem_object_funcs.mmap
On Wed, Nov 13, 2019 at 2:18 AM Daniel Vetter wrote: > > On Wed, Nov 13, 2019 at 8:39 AM Gerd Hoffmann wrote: > > > > Hi, > > > > > > > >> VRAM helpers use drm_gem_ttm_mmap(), which wraps ttm_bo_mmap_obj(). > > > > > >> These changes should be transparent. > > > > > > > > > > > > There's still the issue that for dma-buf mmap vs drm mmap you use > > > > > > different f_mapping, which means ttm's pte shootdown won't work > > > > > > correctly > > > > > > for dma-buf mmaps. But yeah normal operation for ttm/vram helpers > > > > > > should > > > > > > be fine. > > > > > > > > > > VRAM helpers don't support dmabufs. > > > > > > > > It's not that simple. Even when not supporting dma-buf export and > > > > import it is still possible to create dma-bufs, import them into the > > > > same driver (which doesn't actually export+import but just grabs a gem > > > > object reference instead) and also to mmap them via prime/dma-buf code > > > > path ... > > > > ... but after looking again I think we are all green here. Given that > > only self-import works we'll only see vram gem objects in the mmap code > > path, which should have everything set up correctly. Same goes for qxl. > > > > All other ttm drivers still use the old mmap code path, so all green > > there too I think. Also I somehow doubt dma-buf mmap vs. drm mmap ends > > up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open() > > which would fire should that be the case. > > > > Do imported dma-bufs hit the drm mmap code path in the first place? > > Wouldn't mmap be handled by the exporting driver? > > drm_gem_dmabuf_mmap -> obj->funcs->mmap -> ttm_bo_mmap_obj > > And I'm not seeing anyone adjusting vm_file->f_mapping anywhere here at all. > > Note to hit this you need userspace to > - handle2fd on a buffer to create a dma-buf fd > - call mmap directly on that dma-buf fd That was exactly the vgem IGT test that prompted $subject fix. (With vgem modified to use shmem helpers) Rob ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/ttm: fix mmap refcounting
When mapping ttm objects via drm_gem_ttm_mmap() helper
drm_gem_mmap_obj() will take an object reference. That gets
never released due to ttm having its own reference counting.
Fix that by dropping the gem object reference once the ttm mmap
completed (and ttm refcount got bumped).
For that to work properly the drm_gem_object_get() call in
drm_gem_ttm_mmap() must be moved so it happens before calling
obj->funcs->mmap(), otherwise the gem refcount would go down
to zero.
Fixes: 231927d939f0 ("drm/ttm: add drm_gem_ttm_mmap()")
Signed-off-by: Gerd Hoffmann
---
drivers/gpu/drm/drm_gem.c| 24 ++--
drivers/gpu/drm/drm_gem_ttm_helper.c | 13 -
2 files changed, 26 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
index 2f2b889096b0..000fa4a1899f 100644
--- a/drivers/gpu/drm/drm_gem.c
+++ b/drivers/gpu/drm/drm_gem.c
@@ -1105,21 +1105,33 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj,
unsigned long obj_size,
if (obj_size < vma->vm_end - vma->vm_start)
return -EINVAL;
+ /* Take a ref for this mapping of the object, so that the fault
+* handler can dereference the mmap offset's pointer to the object.
+* This reference is cleaned up by the corresponding vm_close
+* (which should happen whether the vma was created by this call, or
+* by a vm_open due to mremap or partial unmap or whatever).
+*/
+ drm_gem_object_get(obj);
+
if (obj->funcs && obj->funcs->mmap) {
/* Remove the fake offset */
vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
ret = obj->funcs->mmap(obj, vma);
- if (ret)
+ if (ret) {
+ drm_gem_object_put_unlocked(obj);
return ret;
+ }
WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
} else {
if (obj->funcs && obj->funcs->vm_ops)
vma->vm_ops = obj->funcs->vm_ops;
else if (dev->driver->gem_vm_ops)
vma->vm_ops = dev->driver->gem_vm_ops;
- else
+ else {
+ drm_gem_object_put_unlocked(obj);
return -EINVAL;
+ }
vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND |
VM_DONTDUMP;
vma->vm_page_prot =
pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
@@ -1128,14 +1140,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj,
unsigned long obj_size,
vma->vm_private_data = obj;
- /* Take a ref for this mapping of the object, so that the fault
-* handler can dereference the mmap offset's pointer to the object.
-* This reference is cleaned up by the corresponding vm_close
-* (which should happen whether the vma was created by this call, or
-* by a vm_open due to mremap or partial unmap or whatever).
-*/
- drm_gem_object_get(obj);
-
return 0;
}
EXPORT_SYMBOL(drm_gem_mmap_obj);
diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c
b/drivers/gpu/drm/drm_gem_ttm_helper.c
index 7412bfc5c05a..605a8a3da7f9 100644
--- a/drivers/gpu/drm/drm_gem_ttm_helper.c
+++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
@@ -64,8 +64,19 @@ int drm_gem_ttm_mmap(struct drm_gem_object *gem,
struct vm_area_struct *vma)
{
struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
+ int ret;
- return ttm_bo_mmap_obj(vma, bo);
+ ret = ttm_bo_mmap_obj(vma, bo);
+ if (ret < 0)
+ return ret;
+
+ /*
+* ttm has its own object refcounting, so drop gem reference
+* to avoid double accounting counting.
+*/
+ drm_gem_object_put_unlocked(gem);
+
+ return 0;
}
EXPORT_SYMBOL(drm_gem_ttm_mmap);
--
2.18.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/sun4i: tcon: Set min division of TCON0_DCLK to 1.
Hi, On Wed, Nov 13, 2019 at 01:27:25PM +, Tian Yunhao wrote: > The datasheet of V3s (and various other chips) wrote > that TCON0_DCLK_DIV can be >= 1 if only dclk is used, > and must >= 6 if dclk1 or dclk2 is used. As currently > neither dclk1 nor dclk2 is used (no writes to these > bits), let's set minimal division to 1. > > If this minimal division is 6, some common dot clock > frequencies can't be produced (e.g. 30MHz will not be > possible and will fallback to 25MHz), which is > obviously not an expected behaviour. > > Signed-off-by: Yunhao Tian Applied, thanks. I had to update your author name to match the one in the Signed-off-by. You probably want to check your git configuration to remain consistent. Maxime signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PULL] drm-misc-fixes
Hi Dave, Daniel, Here's a PR for this week's drm-misc-fixes. Maxime drm-misc-fixes-2019-11-13: - One fix to the dotclock dividers range for sun4i The following changes since commit 105401b659b7eb9cb42d6b5b75d5c049ad4b3dca: drm/shmem: Add docbook comments for drm_gem_shmem_object madvise fields (2019-11-06 17:57:42 -0600) are available in the Git repository at: git://anongit.freedesktop.org/drm/drm-misc tags/drm-misc-fixes-2019-11-13 for you to fetch changes up to 0b8e7bbde5e7e2c419567e1ee29587dae3b78ee3: drm/sun4i: tcon: Set min division of TCON0_DCLK to 1. (2019-11-13 15:20:33 +0100) - One fix to the dotclock dividers range for sun4i Yunhao Tian (1): drm/sun4i: tcon: Set min division of TCON0_DCLK to 1. drivers/gpu/drm/sun4i/sun4i_tcon.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) signature.asc Description: PGP signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 3/5] drm/vmwgfx: drop DRM_AUTH for render ioctls
On Tue, 12 Nov 2019 at 12:54, Thomas Hellstrom wrote:
>
> On 11/1/19 2:05 PM, Emil Velikov wrote:
> > From: Emil Velikov
> >
> > With earlier commit 9c84aeba67cc ("drm/vmwgfx: Kill unneeded legacy
> > security features") we removed the no longer applicable validation, as
> > we now have isolation of primary clients from different master realms.
> >
> > As of last commit, we're explicitly checking for authentication in the
> > only render ioctls which care about one.
> >
> > With those in place, the DRM_AUTH token serves no real purpose. Let's
> > drop it.
> >
> > Cc: VMware Graphics
> > Cc: Thomas Hellstrom
> > Signed-off-by: Emil Velikov
> > ---
> > drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 28 ++--
> > 1 file changed, 14 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > index 81a95651643f..253fae160175 100644
> > --- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
> > @@ -165,9 +165,9 @@
> >
> > static const struct drm_ioctl_desc vmw_ioctls[] = {
> > VMW_IOCTL_DEF(VMW_GET_PARAM, vmw_getparam_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_ALLOC_DMABUF, vmw_bo_alloc_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_UNREF_DMABUF, vmw_bo_unref_ioctl,
> > DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_CURSOR_BYPASS,
> > @@ -182,16 +182,16 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
> > DRM_MASTER),
> >
> > VMW_IOCTL_DEF(VMW_CREATE_CONTEXT, vmw_context_define_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_UNREF_CONTEXT, vmw_context_destroy_ioctl,
> > DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_CREATE_SURFACE, vmw_surface_define_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_UNREF_SURFACE, vmw_surface_destroy_ioctl,
> > DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_REF_SURFACE, vmw_surface_reference_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > - VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl, DRM_AUTH |
> > + DRM_RENDER_ALLOW),
> > + VMW_IOCTL_DEF(VMW_EXECBUF, vmw_execbuf_ioctl,
> > DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_FENCE_WAIT, vmw_fence_obj_wait_ioctl,
> > DRM_RENDER_ALLOW),
> > @@ -201,9 +201,9 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
> > VMW_IOCTL_DEF(VMW_FENCE_UNREF, vmw_fence_obj_unref_ioctl,
> > DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_FENCE_EVENT, vmw_fence_event_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_GET_3D_CAP, vmw_get_cap_3d_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> >
> > /* these allow direct access to the framebuffers mark as master only
> > */
> > VMW_IOCTL_DEF(VMW_PRESENT, vmw_present_ioctl,
> > @@ -221,28 +221,28 @@ static const struct drm_ioctl_desc vmw_ioctls[] = {
> > DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_CREATE_SHADER,
> > vmw_shader_define_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_UNREF_SHADER,
> > vmw_shader_destroy_ioctl,
> > DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_GB_SURFACE_CREATE,
> > vmw_gb_surface_define_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_GB_SURFACE_REF,
> > vmw_gb_surface_reference_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_SYNCCPU,
> > vmw_user_bo_synccpu_ioctl,
> > DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_CREATE_EXTENDED_CONTEXT,
> > vmw_extended_context_define_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_GB_SURFACE_CREATE_EXT,
> > vmw_gb_surface_define_ext_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> > VMW_IOCTL_DEF(VMW_GB_SURFACE_REF_EXT,
> > vmw_gb_surface_reference_ext_ioctl,
> > - DRM_AUTH | DRM_RENDER_ALLOW),
> > + DRM_RENDER_ALLOW),
> > };
> >
> > static const struct pci_device_id vmw_pci_id
Re: [PATCH v2 1/4] drm: bridge: dw_mipi_dsi: access registers via a regmap
On Wed, 6 Nov 2019 at 16:30, Adrian Ratiu wrote: > > Convert the common bridge code and the two rockchip & stm drivers > which currently use it to the regmap API in anticipation for further > changes to make it more generic and add older DSI host controller > support as found on i.mx6 based devices. > > The regmap becomes an internal state of the bridge. No functional > changes other than requiring the platform drivers to use the > pre-configured regmap supplied by the bridge after its probe() call > instead of ioremp'ing the registers themselves. > > In subsequent commits the bridge will become able to detect the > DSI host core version and init the regmap with different register > layouts. The platform drivers will continue to use the regmap without > modifications or worrying about the specific layout in use (in other > words the layout is abstracted away via the regmap). > > Suggested-by: Boris Brezillon > Reviewed-by: Neil Armstrong > Reviewed-by: Emil Velikov I should have been clearer earlier - I didn't quite review the patch. Is is now though. Reviewed-by: Emil Velikov Admittedly a couple of nitpicks (DRIVER_NAME, zero initialize of val) could have been left out. It's not a big deal, there's no need to polish those. -Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 2/4] drm: bridge: dw_mipi_dsi: abstract register access using reg_fields
Hi Adrian,
On Wed, 6 Nov 2019 at 16:31, Adrian Ratiu wrote:
>
> Register existence, address/offsets, field layouts, reserved bits and
> so on differ between MIPI-DSI versions and between SoC vendor boards.
> Despite these differences the hw IP and protocols are mostly the same
> so the generic driver can be made to compensate these differences.
>
> The current Rockchip and STM drivers hardcoded a lot of their common
> definitions in the bridge code because they're based on DSI v1.30 and
> 1.31 which are relatively close, but in order to support older/future
> versions with more diverging layouts like the v1.01 present on imx6,
> we abstract some of the register accesses via the regmap field APIs.
>
> The bridge detects the DSI core version and initializes the required
> regmap register layout, so platform drivers will continue to use the
> regmap as before. Other DSI versions / register layouts can easily be
> added in the future by only changing the bridge code.
>
> An additional benefit of using the reg_field API is that much of the
> bit-shifting and masking boilerplate is removed because it's now
> handled automatically by the regmap subsystem.
>
> Not all register accesses have been converted: only the minimum diff
> between the three host controller versions supported by the current
> vendor platform drivers (rockchip, stm and now imx), more can be added
> in the future as needed.
>
> Suggested-by: Boris Brezillon
> Reviewed-by: Neil Armstrong
> Reviewed-by: Emil Velikov
With the extra const mentioned below the patch is:
Reviewed-by: Emil Velikov
> +
> +static struct dw_mipi_dsi_variant dw_mipi_dsi_v130_v131_layout = {
It's a non-const here, while we consider it a const below. I'd add the explicit
const declaration here.
> +#define INIT_FIELD(f) INIT_FIELD_CFG(field_##f, cfg_##f)
> +#define INIT_FIELD_CFG(f, conf)
> \
> + do {\
> + dsi->f = devm_regmap_field_alloc(dsi->dev, dsi->regs, \
> +variant->conf);\
> + if (IS_ERR(dsi->f)) \
> + dev_warn(dsi->dev, "Ignoring regmap field " #f "\n");
> \
> + } while (0)
> +
> +static int dw_mipi_dsi_regmap_fields_init(struct dw_mipi_dsi *dsi)
> +{
> + const struct dw_mipi_dsi_variant *variant =
> &dw_mipi_dsi_v130_v131_layout;
> +
Having a closer look at the const-ness thing:
devm_regmap_field_alloc() uses a read/write copy of the reg_field struct (5
unsigned ints), even though it only uses them as read-only. A sensible way to
improve is is to pass a "const struct reg_field *" instead.
But that for another day ... might be worth adding a newbie regmap task for, if
you can see where to file that.
-Emil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 0/7] various '-Wunused-but-set-variable' gcc warning fixes
Applied the series, although a couple of the patches were already applied from a previous patch set. Thanks, Alex On Wed, Nov 13, 2019 at 9:12 AM yu kuai wrote: > > This patch set fixes various unrelated gcc '-Wunused-but-set-variable' > warnings. > > yu kuai (7): > drm/amdgpu: remove set but not used variable 'mc_shared_chmap' from > 'gfx_v6_0.c' and 'gfx_v7_0.c' > drm/amdgpu: remove set but not used variable 'amdgpu_connector' > drm/amdgpu: remove set but not used variable 'count' > drm/amdgpu: remove set but not used variable 'invalid' > drm/amdgpu: remove set but not used variable 'threshold' > drm/amdgpu: remove set but not used variable 'state' > drm/amdgpu: remove set but not used variable 'us_mvdd' > > drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c| 4 ++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 -- > drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c | 3 +-- > drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 3 +-- > drivers/gpu/drm/amd/amdkfd/kfd_device.c | 4 ++-- > drivers/gpu/drm/amd/powerplay/smumgr/fiji_smumgr.c | 7 ++- > drivers/gpu/drm/amd/powerplay/smumgr/vegam_smumgr.c | 12 > 7 files changed, 8 insertions(+), 27 deletions(-) > > -- > 2.7.4 > > ___ > amd-gfx mailing list > amd-...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 2/3] drm/fb-helper: Remove drm_fb_helper_unlink_fbi()
Den 13.11.2019 12.52, skrev Thomas Zimmermann: > There are no callers of drm_fb_helper_unlink_fbi() left. Remove the > function. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Noralf Trønnes ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/3] drm/udl: Replace fbdev code with generic emulation
Den 13.11.2019 12.52, skrev Thomas Zimmermann: > The udl driver can use the generic fbdev implementation. Convert it. > > v4: > * hardcode console bpp to 16 > v3: > * remove module parameter fb_bpp in favor of fbdev's video > * call drm_fbdev_generic_setup() directly; remove udl_fbdev_init() > * use default for struct drm_mode_config_funcs.output_poll_changed > * use default for struct drm_driver.lastclose > > Signed-off-by: Thomas Zimmermann > --- > diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c > index 4e854e017390..3be0c0cec49e 100644 > --- a/drivers/gpu/drm/udl/udl_main.c > +++ b/drivers/gpu/drm/udl/udl_main.c > @@ -9,6 +9,7 @@ > */ > > #include > +#include > #include > #include > > @@ -338,7 +339,7 @@ int udl_init(struct udl_device *udl) > if (ret) > goto err; > > - ret = udl_fbdev_init(dev); > + ret = drm_fbdev_generic_setup(dev, 16); I suggest you put this after drm_dev_register() in _probe() since fbdev is a client, a user of the driver, not part of it as such. Either way you choose: Reviewed-by: Noralf Trønnes Btw, nice diffstat! > if (ret) > goto err; > > @@ -367,6 +368,4 @@ void udl_fini(struct drm_device *dev) > > if (udl->urbs.count) > udl_free_urb_list(dev); > - > - udl_fbdev_cleanup(dev); > } ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 3/3] fbdev: Unexport unlink_framebuffer()
Den 13.11.2019 12.52, skrev Thomas Zimmermann: > There are no external callers of unlink_framebuffer() left. Make the > function an internal interface. > > Signed-off-by: Thomas Zimmermann > --- Reviewed-by: Noralf Trønnes ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/3] drm/udl: Replace fbdev code with generic emulation
Hi Am 13.11.19 um 16:26 schrieb Noralf Trønnes: > > > Den 13.11.2019 12.52, skrev Thomas Zimmermann: >> The udl driver can use the generic fbdev implementation. Convert it. >> >> v4: >> * hardcode console bpp to 16 >> v3: >> * remove module parameter fb_bpp in favor of fbdev's video >> * call drm_fbdev_generic_setup() directly; remove udl_fbdev_init() >> * use default for struct drm_mode_config_funcs.output_poll_changed >> * use default for struct drm_driver.lastclose >> >> Signed-off-by: Thomas Zimmermann >> --- > >> diff --git a/drivers/gpu/drm/udl/udl_main.c b/drivers/gpu/drm/udl/udl_main.c >> index 4e854e017390..3be0c0cec49e 100644 >> --- a/drivers/gpu/drm/udl/udl_main.c >> +++ b/drivers/gpu/drm/udl/udl_main.c >> @@ -9,6 +9,7 @@ >> */ >> >> #include >> +#include >> #include >> #include >> >> @@ -338,7 +339,7 @@ int udl_init(struct udl_device *udl) >> if (ret) >> goto err; >> >> -ret = udl_fbdev_init(dev); >> +ret = drm_fbdev_generic_setup(dev, 16); > > I suggest you put this after drm_dev_register() in _probe() since fbdev > is a client, a user of the driver, not part of it as such. Good point. I'll change it. > > Either way you choose: Glad you like it :) > > Reviewed-by: Noralf Trønnes Thanks for the review. Best regards Thomas > > Btw, nice diffstat! > >> if (ret) >> goto err; >> >> @@ -367,6 +368,4 @@ void udl_fini(struct drm_device *dev) >> >> if (udl->urbs.count) >> udl_free_urb_list(dev); >> - >> -udl_fbdev_cleanup(dev); >> } > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer signature.asc Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 3/4] drm: imx: Add i.MX 6 MIPI DSI host driver
On Wed, 6 Nov 2019 at 16:31, Adrian Ratiu wrote:
>
> This adds support for the Synopsis DesignWare MIPI DSI v1.01 host
> controller which is embedded in i.MX 6 SoCs.
>
> Based on following patches, but updated/extended to work with existing
> support found in the kernel:
>
> - drm: imx: Support Synopsys DesignWare MIPI DSI host controller
> Signed-off-by: Liu Ying
>
> - ARM: dtsi: imx6qdl: Add support for MIPI DSI host controller
> Signed-off-by: Liu Ying
>
> Reviewed-by: Neil Armstrong
> Reviewed-by: Emil Velikov
With the const nitpick below, the patch is:
Reviewed-by: Emil Velikov
Aside, for the future consider having a change log in the patch itself.
What I tend to do is:
- v2: Keep DW version specifics in dw-mipi-dsi.c (Emil)
> +static struct dw_mipi_dsi_variant dw_mipi_dsi_v101_layout = {
Nit: make this a const.
> + .cfg_dpi_vid = REG_FIELD(DSI_DPI_CFG, 0, 1),
> + .cfg_dpi_color_coding = REG_FIELD(DSI_DPI_CFG, 2, 4),
> + .cfg_dpi_18loosely_en = REG_FIELD(DSI_DPI_CFG, 10, 10),
> + .cfg_dpi_vsync_active_low = REG_FIELD(DSI_DPI_CFG, 6, 6),
> + .cfg_dpi_hsync_active_low = REG_FIELD(DSI_DPI_CFG, 7, 7),
> + .cfg_cmd_mode_en = REG_FIELD(DSI_CMD_MODE_CFG_V101, 0,
> 0),
> + .cfg_cmd_mode_all_lp_en = REG_FIELD(DSI_CMD_MODE_CFG_V101, 1,
> 12),
> + .cfg_cmd_mode_ack_rqst_en = REG_FIELD(DSI_CMD_MODE_CFG_V101, 13,
> 13),
> + .cfg_cmd_pkt_status = REG_FIELD(DSI_CMD_PKT_STATUS_V101, 0,
> 14),
> + .cfg_vid_mode_en = REG_FIELD(DSI_VID_MODE_CFG_V101, 0,
> 0),
> + .cfg_vid_mode_type =REG_FIELD(DSI_VID_MODE_CFG_V101, 1,
> 2),
> + .cfg_vid_mode_low_power = REG_FIELD(DSI_VID_MODE_CFG_V101, 3,
> 8),
> + .cfg_vid_pkt_size = REG_FIELD(DSI_VID_PKT_CFG, 0, 10),
> + .cfg_vid_hsa_time = REG_FIELD(DSI_TMR_LINE_CFG, 0, 8),
> + .cfg_vid_hbp_time = REG_FIELD(DSI_TMR_LINE_CFG, 9, 17),
> + .cfg_vid_hline_time = REG_FIELD(DSI_TMR_LINE_CFG, 18, 31),
> + .cfg_vid_vsa_time = REG_FIELD(DSI_VTIMING_CFG, 0, 3),
> + .cfg_vid_vbp_time = REG_FIELD(DSI_VTIMING_CFG, 4, 9),
> + .cfg_vid_vfp_time = REG_FIELD(DSI_VTIMING_CFG, 10, 15),
> + .cfg_vid_vactive_time = REG_FIELD(DSI_VTIMING_CFG, 16, 26),
> + .cfg_phy_txrequestclkhs = REG_FIELD(DSI_PHY_IF_CTRL, 0, 0),
> + .cfg_phy_bta_time = REG_FIELD(DSI_PHY_TMR_CFG_V101, 0,
> 11),
> + .cfg_phy_lp2hs_time = REG_FIELD(DSI_PHY_TMR_CFG_V101, 12,
> 19),
> + .cfg_phy_hs2lp_time = REG_FIELD(DSI_PHY_TMR_CFG_V101, 20,
> 27),
> + .cfg_phy_testclr = REG_FIELD(DSI_PHY_TST_CTRL0_V101, 0,
> 0),
> + .cfg_phy_unshutdownz = REG_FIELD(DSI_PHY_RSTZ_V101, 0, 0),
> + .cfg_phy_unrstz = REG_FIELD(DSI_PHY_RSTZ_V101, 1, 1),
> + .cfg_phy_enableclk =REG_FIELD(DSI_PHY_RSTZ_V101, 2, 2),
> + .cfg_phy_nlanes = REG_FIELD(DSI_PHY_IF_CFG_V101, 0, 1),
> + .cfg_phy_stop_wait_time = REG_FIELD(DSI_PHY_IF_CFG_V101, 2, 9),
> + .cfg_phy_status = REG_FIELD(DSI_PHY_STATUS_V101, 0, 0),
> + .cfg_pckhdl_cfg = REG_FIELD(DSI_PCKHDL_CFG_V101, 0, 4),
> + .cfg_hstx_timeout_counter = REG_FIELD(DSI_TO_CNT_CFG_V101, 0, 15),
> + .cfg_lprx_timeout_counter = REG_FIELD(DSI_TO_CNT_CFG_V101, 16,
> 31),
> + .cfg_int_stat0 =REG_FIELD(DSI_ERROR_ST0_V101, 0, 20),
> + .cfg_int_stat1 =REG_FIELD(DSI_ERROR_ST1_V101, 0, 17),
> + .cfg_int_mask0 =REG_FIELD(DSI_ERROR_MSK0_V101, 0, 20),
> + .cfg_int_mask1 =REG_FIELD(DSI_ERROR_MSK1_V101, 0, 17),
> + .cfg_gen_hdr = REG_FIELD(DSI_GEN_HDR_V101, 0, 31),
> + .cfg_gen_payload = REG_FIELD(DSI_GEN_PLD_DATA_V101, 0,
> 31),
> +};
if we start getting a lot of these, one way to keep things brief is to
reuse the GEN._FEATURES approach in gpu/drm/i915/i915_pci.c
Namely:
#define 100_FEATURES \
.foo = ... \
.bar = ...
v100_layout = {
100_FEATURES,
};
... v101_layout = {
100_FEATURES,
// extra 101 changes
.foo = ...101, \
.bar = ...101
};
But that for another day.
HTH
-Emil
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 1/3] drm/udl: Replace fbdev code with generic emulation
Am 13.11.19 um 16:41 schrieb Thomas Zimmermann: >> Either way you choose: > > Glad you like it :) > more like this >> >> Btw, nice diffstat! Glad you like it :) >> >>> if (ret) >>> goto err; >>> >>> @@ -367,6 +368,4 @@ void udl_fini(struct drm_device *dev) >>> >>> if (udl->urbs.count) >>> udl_free_urb_list(dev); >>> - >>> - udl_fbdev_cleanup(dev); >>> } >> ___ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/dri-devel >> > -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 Nürnberg, Germany (HRB 36809, AG Nürnberg) Geschäftsführer: Felix Imendörffer signature.asc Description: OpenPGP digital signature ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 12/12] drm/connector: Hookup the new drm_cmdline_mode panel_orientation member
Hi,
On 12-11-2019 14:47, Daniel Vetter wrote:
On Tue, Nov 12, 2019 at 2:39 PM Hans de Goede wrote:
Hi,
On 12-11-2019 14:32, Daniel Vetter wrote:
On Tue, Nov 12, 2019 at 11:43 AM Hans de Goede wrote:
Hi,
On 12-11-2019 10:47, Daniel Vetter wrote:
On Sun, Nov 10, 2019 at 04:41:01PM +0100, Hans de Goede wrote:
If the new video=... panel_orientation option is set for a connector, honor
it and setup a matching "panel orientation" property on the connector.
BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83
Signed-off-by: Hans de Goede
Don't we need one more patch here to create the panel orientation property
if the driver doesn't do it? Otherwise this won't happen, and userspace
can't look at it (only the internal fbdev stuff, which has it's own
limitations wrt rotation).
That is what patch 11/12 is for, that patch renames
drm_connector_init_panel_orientation
to drm_set_panel_orientation and makes it both set, init and attach the property
(unless called with DRM_MODE_PANEL_ORIENTATION_UNKNOWN in which case it is a
no-op).
Patch 11/12 also makes drm_set_panel_orientation do the set only once, which is
why
the orientation to set is now passed instead of stored directly in the
connector,
so that a second set call (panel_orientation of the connector already !=
DRM_MODE_PANEL_ORIENTATION_UNKNOWN) can be skipped, so that the cmdline
overrides
driver detecion code for this.
Oh, that's what I get for not reading the entire series ... Only risk
with that is that drivers set this property after driver loading is
done - we can't attach/create properties after drm_dev_register. But
I've added the corresponding WARN_ON to these, so we should be all
fine I think. So looks all good to me.
Can I take that as your Acked-by for this patch and perhaps also for 11/12 ?
Uh I didn't really read the details, ack feels a bit thin to land this ...
Ok, I will wait a bit for others to review this then. Note that Maxime has
reviewed patches 1-9, with one small remark on patch 9 which I'm still awaiting
an answer for. So most of the cmdline parsing stuff has been reviewed and
if you are ok with the non cmdline parsing changes...
Also what about your remarks to 10/12? I'm happy to do a v2 with a memset,
as said my main reason for dropping the specified=false in the early path
is that we never init bpp_specified or refresh_specified explicitly to false
I'm all for being explicit about that, but then lets just zero out the entire
passed in drm_cmdline_mode struct.
Hm yeah, clearing it all might be a good idea.
Ok I will submit a v2 with this change.
Regards,
Hans
---
drivers/gpu/drm/drm_connector.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 40a985c411a6..591914f01cd4 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -140,6 +140,13 @@ static void drm_connector_get_cmdline_mode(struct
drm_connector *connector)
connector->force = mode->force;
}
+if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) {
+DRM_INFO("setting connector %s panel_orientation to %d\n",
+ connector->name, mode->panel_orientation);
+drm_connector_set_panel_orientation(connector,
+mode->panel_orientation);
+}
+
DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
connector->name, mode->name,
mode->xres, mode->yres,
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 0/2] drm/ast: Remove load/unload callbacks
This patchset removes the load/unload callback functions from ast. Tested
on AST 2100 HW.
Thomas Zimmermann (2):
drm/ast: Replace drm_get_pci_device() and drm_put_dev()
drm/ast: Call struct drm_driver.{load,unload} before registering
device
drivers/gpu/drm/ast/ast_drv.c | 42 ++-
1 file changed, 37 insertions(+), 5 deletions(-)
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 1/2] drm/ast: Replace drm_get_pci_device() and drm_put_dev()
Both functions are deprecated. Open-code them them in preparation
of removing struct drm_driver.{load,unload}.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_drv.c | 31 +--
1 file changed, 29 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index d763da6f0834..78c90a3c903b 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -86,9 +86,35 @@ static void ast_kick_out_firmware_fb(struct pci_dev *pdev)
static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
{
+ struct drm_device *dev;
+ int ret;
+
ast_kick_out_firmware_fb(pdev);
- return drm_get_pci_dev(pdev, ent, &driver);
+ ret = pci_enable_device(pdev);
+ if (ret)
+ return ret;
+
+ dev = drm_dev_alloc(&driver, &pdev->dev);
+ if (IS_ERR(dev)) {
+ ret = PTR_ERR(dev);
+ goto err_pci_disable_device;
+ }
+
+ dev->pdev = pdev;
+ pci_set_drvdata(pdev, dev);
+
+ ret = drm_dev_register(dev, ent->driver_data);
+ if (ret)
+ goto err_drm_dev_put;
+
+ return 0;
+
+err_drm_dev_put:
+ drm_dev_put(dev);
+err_pci_disable_device:
+ pci_disable_device(pdev);
+ return ret;
}
static void
@@ -96,7 +122,8 @@ ast_pci_remove(struct pci_dev *pdev)
{
struct drm_device *dev = pci_get_drvdata(pdev);
- drm_put_dev(dev);
+ drm_dev_unregister(dev);
+ drm_dev_put(dev);
}
static int ast_drm_freeze(struct drm_device *dev)
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH 2/2] drm/ast: Call struct drm_driver.{load, unload} before registering device
Both callbacks are deprecated. Remove them and call functions explicitly.
Signed-off-by: Thomas Zimmermann
---
drivers/gpu/drm/ast/ast_drv.c | 13 +
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 78c90a3c903b..9da26750a22d 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -104,17 +104,24 @@ static int ast_pci_probe(struct pci_dev *pdev, const
struct pci_device_id *ent)
dev->pdev = pdev;
pci_set_drvdata(pdev, dev);
- ret = drm_dev_register(dev, ent->driver_data);
+ ret = ast_driver_load(dev, ent->driver_data);
if (ret)
goto err_drm_dev_put;
+ ret = drm_dev_register(dev, ent->driver_data);
+ if (ret)
+ goto err_ast_driver_unload;
+
return 0;
+err_ast_driver_unload:
+ ast_driver_unload(dev);
err_drm_dev_put:
drm_dev_put(dev);
err_pci_disable_device:
pci_disable_device(pdev);
return ret;
+
}
static void
@@ -123,6 +130,7 @@ ast_pci_remove(struct pci_dev *pdev)
struct drm_device *dev = pci_get_drvdata(pdev);
drm_dev_unregister(dev);
+ ast_driver_unload(dev);
drm_dev_put(dev);
}
@@ -228,9 +236,6 @@ static struct drm_driver driver = {
DRIVER_GEM |
DRIVER_MODESET,
- .load = ast_driver_load,
- .unload = ast_driver_unload,
-
.fops = &ast_fops,
.name = DRIVER_NAME,
.desc = DRIVER_DESC,
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 4/4] dt-bindings: display: add IMX MIPI DSI host controller doc
On Wed, 6 Nov 2019 at 16:31, Adrian Ratiu wrote: > > Reviewed-by: Neil Armstrong > Reviewed-by: Emil Velikov Please drop this one. I'm not that experienced in DT to provide meaningful review. Actually, I've just noticed that respective maintainers/lists are not CC'd on the series. Please use the get_maintainer.pl script got get the correct info. Personally, I read through the output, adding only relevant people as CC in the commit message itself. In particular, I don't think adding the "maintainer: DRM DRIVER" or the "ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" are required. On the other hand "DRM DRIVERS FOR FREESCALE IMX" and "OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" seems pretty accurate for what you're doing here. HTH Emil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 09/12] drm/modes: parse_cmdline: Add support for specifying panel_orientation
On Mon, Nov 11, 2019 at 06:25:33PM +0100, Hans de Goede wrote:
> Hi,
>
> On 11-11-2019 13:53, Maxime Ripard wrote:
> > Hi Hans,
> >
> > Thanks for this series (and thanks for bouncing the mails too).
> >
> > All the previous patches are
> > Acked-by: Maxime Ripard
>
> Thank you for the review.
>
> > On Sun, Nov 10, 2019 at 04:40:58PM +0100, Hans de Goede wrote:
> > > Sometimes we want to override a connector's panel_orientation from the
> > > kernel commandline. Either for testing and for special cases, e.g. a kiosk
> > > like setup which uses a TV mounted in portrait mode.
> > >
> > > Users can already specify a "rotate" option through a video= kernel
> > > cmdline
> > > option. But that only supports 0/180 degrees (see drm_client_modeset TODO)
> > > and only works for in kernel modeset clients, not for userspace kms users.
> > >
> > > The "panel-orientation" connector property OTOH does support 90/270
> > > degrees
> > > as it leaves dealing with the rotation up to userspace and this does work
> > > for userspace kms clients (at least those which support this property).
> > >
> > > BugLink:
> > > https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83
> > > Signed-off-by: Hans de Goede
> > > ---
> > > Documentation/fb/modedb.rst | 3 ++
> > > drivers/gpu/drm/drm_modes.c | 32 +++
> > > .../gpu/drm/selftests/drm_cmdline_selftests.h | 1 +
> > > .../drm/selftests/test-drm_cmdline_parser.c | 22 +
> > > include/drm/drm_connector.h | 8 +
> > > 5 files changed, 66 insertions(+)
> > >
> > > diff --git a/Documentation/fb/modedb.rst b/Documentation/fb/modedb.rst
> > > index 9c4e3fd39e6d..624d08fd2856 100644
> > > --- a/Documentation/fb/modedb.rst
> > > +++ b/Documentation/fb/modedb.rst
> > > @@ -65,6 +65,9 @@ Valid options are::
> > > - reflect_y (boolean): Perform an axial symmetry on the Y axis
> > > - rotate (integer): Rotate the initial framebuffer by x
> > > degrees. Valid values are 0, 90, 180 and 270.
> > > + - panel_orientation, one of "normal", "upside_down", "left_side_up", or
> > > +"right_side_up". For KMS drivers only, this sets the "panel
> > > orientation"
> > > +property on the kms connector as hint for kms users.
> >
> > Even though the semantic is a bit different, I think we should remain
> > consistent and have the same argument than for rotate (ie, steps in
> > clockwise rotation in steps of 90 degrees).
>
> Well the kernel kms defines for rotation also talk about 90 / 180 / 270":
>
> #define DRM_MODE_ROTATE_0 (1<<0)
> #define DRM_MODE_ROTATE_90 (1<<1)
> #define DRM_MODE_ROTATE_180 (1<<2)
> #define DRM_MODE_ROTATE_270 (1<<3)
>
> Where as the panel orientation uses strings like right_side_up, which means
> that the side of the panel which normally is the right side of the panel
> is now pointing up as the panel is mounted 90 degrees rotated with its
> original right side now pointing up. This IMHO is much clearer then
> 90 / 270 degrees which are ambiguous and perhaps more importantly this
> matches the kernel defines for panel-orientation and matches the
> String values enumerated by the enum type panel-orientation connector
> property:
>
> static const struct drm_prop_enum_list drm_panel_orientation_enum_list[] = {
> { DRM_MODE_PANEL_ORIENTATION_NORMAL,"Normal"},
> { DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP, "Upside Down" },
> { DRM_MODE_PANEL_ORIENTATION_LEFT_UP, "Left Side Up" },
> { DRM_MODE_PANEL_ORIENTATION_RIGHT_UP, "Right Side Up" },
> };
>
> So I would prefer to stick to the strings.
Ok, that works for me then
Maxime
signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Bug 111123] Display issues AMD RAVEN Ryzen 5 3400G APU
https://bugs.freedesktop.org/show_bug.cgi?id=23 --- Comment #4 from Leonardo da Silva --- i have the same problem, I bought 2x 3400g with A320-S2H, one machine running ubuntu 19.10 and the other Arch Linux, both with the same problem described above when using integrated graphics. -- You are receiving this mail because: You are the assignee for the bug.___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2] drm/gem: Fix mmap fake offset handling for drm_gem_object_funcs.mmap
On Wed, Nov 13, 2019 at 02:51:45PM +0100, Gerd Hoffmann wrote: > Hi, > > > > ... but after looking again I think we are all green here. Given that > > > only self-import works we'll only see vram gem objects in the mmap code > > > path, which should have everything set up correctly. Same goes for qxl. > > > > > > All other ttm drivers still use the old mmap code path, so all green > > > there too I think. Also I somehow doubt dma-buf mmap vs. drm mmap ends > > > up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open() > > > which would fire should that be the case. > > > > > > Do imported dma-bufs hit the drm mmap code path in the first place? > > > Wouldn't mmap be handled by the exporting driver? > > > > drm_gem_dmabuf_mmap -> obj->funcs->mmap -> ttm_bo_mmap_obj > > > > And I'm not seeing anyone adjusting vm_file->f_mapping anywhere here at all. > > [ some more code browsing ] > > Ok, I see. dma-bufs get their own file, their own anon inode and > thereby their own address space. So that it used when mmaping the > dma-buf. > > drm filehandle's get the shared address space instead, drm_open() sets > it. > > So, yes, I see the problem. It's not new though, as far I can see the > old dma-buf mmap code path doesn't adjust f_mapping anywhere either ... > > > Note to hit this you need userspace to > > - handle2fd on a buffer to create a dma-buf fd > > - call mmap directly on that dma-buf fd > > Hmm, seems for handle2fd I need a dummy gem_prime_get_sg_table function > wired up even when not actually exporting/importing anything. So I > think neither qxl nor any of the vram drivers allow to trigger that (and > no other ttm driver uses the new ttm mmap code yet). > > So, $subject patch should not make things worse in ttm land. > > When hacking the bochs driver to have export callbacks (without > supporting actual exports) handle2fd + mmap() callback works fine. > Didn't verify yet I actually get the correct pages mapped. But maybe > mmap() isn't the problem when the correct address space is important for > unmap only. > > Is there a good test case? You need memory pressure, to force ttm to unmap the bo, not userspace. So roughly 1. create bo 2. mmap it through drm fd, write some stuff 3. export to dma-buf, mmap it, verify stuff is there 4. create a pile more bo, mmap them write to them 5. once you've thrashed all of vram enough, recheck your original bo. If I'm right you should get the following: - drm fd mmap still show right content - dma-buf fd mmap shows random crap that you've written into other buffers Ofc you need to make sure that an mmap actually forces the buffer into vram. So might need a combo of modeset+mmap, to make that happen. Plain mmap might just give you ptes that point into system memory, which is not managed by ttm like vram. munmap called by userspace isn't a problem, since that starts from the right struct mm_struct. It's when you start with the object (i.e. in the driver), and need to figure out where all the various vma that mmap it are where this matters. -Daniel -- 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 v2] drm/gem: Fix mmap fake offset handling for drm_gem_object_funcs.mmap
On Wed, Nov 13, 2019 at 07:53:56AM -0600, Rob Herring wrote: > On Wed, Nov 13, 2019 at 2:18 AM Daniel Vetter wrote: > > > > On Wed, Nov 13, 2019 at 8:39 AM Gerd Hoffmann wrote: > > > > > > Hi, > > > > > > > > > >> VRAM helpers use drm_gem_ttm_mmap(), which wraps > > > > > > >> ttm_bo_mmap_obj(). > > > > > > >> These changes should be transparent. > > > > > > > > > > > > > > There's still the issue that for dma-buf mmap vs drm mmap you use > > > > > > > different f_mapping, which means ttm's pte shootdown won't work > > > > > > > correctly > > > > > > > for dma-buf mmaps. But yeah normal operation for ttm/vram helpers > > > > > > > should > > > > > > > be fine. > > > > > > > > > > > > VRAM helpers don't support dmabufs. > > > > > > > > > > It's not that simple. Even when not supporting dma-buf export and > > > > > import it is still possible to create dma-bufs, import them into the > > > > > same driver (which doesn't actually export+import but just grabs a gem > > > > > object reference instead) and also to mmap them via prime/dma-buf code > > > > > path ... > > > > > > ... but after looking again I think we are all green here. Given that > > > only self-import works we'll only see vram gem objects in the mmap code > > > path, which should have everything set up correctly. Same goes for qxl. > > > > > > All other ttm drivers still use the old mmap code path, so all green > > > there too I think. Also I somehow doubt dma-buf mmap vs. drm mmap ends > > > up using different f_mapping, ttm code has a WARN_ON in ttm_bo_vm_open() > > > which would fire should that be the case. > > > > > > Do imported dma-bufs hit the drm mmap code path in the first place? > > > Wouldn't mmap be handled by the exporting driver? > > > > drm_gem_dmabuf_mmap -> obj->funcs->mmap -> ttm_bo_mmap_obj > > > > And I'm not seeing anyone adjusting vm_file->f_mapping anywhere here at all. > > > > Note to hit this you need userspace to > > - handle2fd on a buffer to create a dma-buf fd > > - call mmap directly on that dma-buf fd > > That was exactly the vgem IGT test that prompted $subject fix. (With > vgem modified to use shmem helpers) See my other mail, this only hits the right mmap paths. For the unmap side you need even more (and that part is driver dependent, and vgem would need some debug tricks to expose that for testing). -Daniel -- 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] drm/ttm: fix mmap refcounting
On Wed, Nov 13, 2019 at 02:56:12PM +0100, Gerd Hoffmann wrote:
> When mapping ttm objects via drm_gem_ttm_mmap() helper
> drm_gem_mmap_obj() will take an object reference. That gets
> never released due to ttm having its own reference counting.
>
> Fix that by dropping the gem object reference once the ttm mmap
> completed (and ttm refcount got bumped).
>
> For that to work properly the drm_gem_object_get() call in
> drm_gem_ttm_mmap() must be moved so it happens before calling
> obj->funcs->mmap(), otherwise the gem refcount would go down
> to zero.
>
> Fixes: 231927d939f0 ("drm/ttm: add drm_gem_ttm_mmap()")
Since the offending patch is in drm-next and we're in the merge window
freeze past -rc6 please remember to apply this to drm-misc-next-fixes.
Otherwise it'll miss the merge window.
> Signed-off-by: Gerd Hoffmann
I was wondering whether we'd need a cc: stable, in case someone is really
fast and gets the vm_close in before we finish the mmap. But I think we
should be serialized by mmap_sem here enough ...
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_gem.c| 24 ++--
> drivers/gpu/drm/drm_gem_ttm_helper.c | 13 -
> 2 files changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c
> index 2f2b889096b0..000fa4a1899f 100644
> --- a/drivers/gpu/drm/drm_gem.c
> +++ b/drivers/gpu/drm/drm_gem.c
> @@ -1105,21 +1105,33 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj,
> unsigned long obj_size,
> if (obj_size < vma->vm_end - vma->vm_start)
> return -EINVAL;
>
> + /* Take a ref for this mapping of the object, so that the fault
> + * handler can dereference the mmap offset's pointer to the object.
> + * This reference is cleaned up by the corresponding vm_close
> + * (which should happen whether the vma was created by this call, or
> + * by a vm_open due to mremap or partial unmap or whatever).
> + */
> + drm_gem_object_get(obj);
> +
> if (obj->funcs && obj->funcs->mmap) {
> /* Remove the fake offset */
> vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node);
>
> ret = obj->funcs->mmap(obj, vma);
> - if (ret)
> + if (ret) {
> + drm_gem_object_put_unlocked(obj);
> return ret;
> + }
> WARN_ON(!(vma->vm_flags & VM_DONTEXPAND));
> } else {
> if (obj->funcs && obj->funcs->vm_ops)
> vma->vm_ops = obj->funcs->vm_ops;
> else if (dev->driver->gem_vm_ops)
> vma->vm_ops = dev->driver->gem_vm_ops;
> - else
> + else {
> + drm_gem_object_put_unlocked(obj);
> return -EINVAL;
> + }
>
> vma->vm_flags |= VM_IO | VM_PFNMAP | VM_DONTEXPAND |
> VM_DONTDUMP;
> vma->vm_page_prot =
> pgprot_writecombine(vm_get_page_prot(vma->vm_flags));
> @@ -1128,14 +1140,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj,
> unsigned long obj_size,
>
> vma->vm_private_data = obj;
>
> - /* Take a ref for this mapping of the object, so that the fault
> - * handler can dereference the mmap offset's pointer to the object.
> - * This reference is cleaned up by the corresponding vm_close
> - * (which should happen whether the vma was created by this call, or
> - * by a vm_open due to mremap or partial unmap or whatever).
> - */
> - drm_gem_object_get(obj);
> -
> return 0;
> }
> EXPORT_SYMBOL(drm_gem_mmap_obj);
> diff --git a/drivers/gpu/drm/drm_gem_ttm_helper.c
> b/drivers/gpu/drm/drm_gem_ttm_helper.c
> index 7412bfc5c05a..605a8a3da7f9 100644
> --- a/drivers/gpu/drm/drm_gem_ttm_helper.c
> +++ b/drivers/gpu/drm/drm_gem_ttm_helper.c
> @@ -64,8 +64,19 @@ int drm_gem_ttm_mmap(struct drm_gem_object *gem,
>struct vm_area_struct *vma)
> {
> struct ttm_buffer_object *bo = drm_gem_ttm_of_gem(gem);
> + int ret;
>
> - return ttm_bo_mmap_obj(vma, bo);
> + ret = ttm_bo_mmap_obj(vma, bo);
> + if (ret < 0)
> + return ret;
> +
> + /*
> + * ttm has its own object refcounting, so drop gem reference
> + * to avoid double accounting counting.
> + */
> + drm_gem_object_put_unlocked(gem);
> +
> + return 0;
> }
> EXPORT_SYMBOL(drm_gem_ttm_mmap);
>
> --
> 2.18.1
>
--
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 1/2] drm/ast: Replace drm_get_pci_device() and drm_put_dev()
On Wed, Nov 13, 2019 at 04:58:56PM +0100, Thomas Zimmermann wrote:
> Both functions are deprecated. Open-code them them in preparation
> of removing struct drm_driver.{load,unload}.
>
> Signed-off-by: Thomas Zimmermann
> ---
> drivers/gpu/drm/ast/ast_drv.c | 31 +--
> 1 file changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index d763da6f0834..78c90a3c903b 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -86,9 +86,35 @@ static void ast_kick_out_firmware_fb(struct pci_dev *pdev)
>
> static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id
> *ent)
> {
> + struct drm_device *dev;
> + int ret;
> +
> ast_kick_out_firmware_fb(pdev);
>
> - return drm_get_pci_dev(pdev, ent, &driver);
> + ret = pci_enable_device(pdev);
> + if (ret)
> + return ret;
> +
> + dev = drm_dev_alloc(&driver, &pdev->dev);
Would be nice to also switch over to embedding drm_device and
drm_dev_init, but for another patch. This is
Reviewed-by: Daniel Vetter
> + if (IS_ERR(dev)) {
> + ret = PTR_ERR(dev);
> + goto err_pci_disable_device;
> + }
> +
> + dev->pdev = pdev;
> + pci_set_drvdata(pdev, dev);
> +
> + ret = drm_dev_register(dev, ent->driver_data);
> + if (ret)
> + goto err_drm_dev_put;
> +
> + return 0;
> +
> +err_drm_dev_put:
> + drm_dev_put(dev);
> +err_pci_disable_device:
> + pci_disable_device(pdev);
> + return ret;
> }
>
> static void
> @@ -96,7 +122,8 @@ ast_pci_remove(struct pci_dev *pdev)
> {
> struct drm_device *dev = pci_get_drvdata(pdev);
>
> - drm_put_dev(dev);
> + drm_dev_unregister(dev);
> + drm_dev_put(dev);
> }
>
> static int ast_drm_freeze(struct drm_device *dev)
> --
> 2.23.0
>
--
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 v2 1/4] drm: bridge: dw_mipi_dsi: access registers via a regmap
On Wed, 13 Nov 2019, Emil Velikov wrote: On Wed, 6 Nov 2019 at 16:30, Adrian Ratiu wrote: Convert the common bridge code and the two rockchip & stm drivers which currently use it to the regmap API in anticipation for further changes to make it more generic and add older DSI host controller support as found on i.mx6 based devices. The regmap becomes an internal state of the bridge. No functional changes other than requiring the platform drivers to use the pre-configured regmap supplied by the bridge after its probe() call instead of ioremp'ing the registers themselves. In subsequent commits the bridge will become able to detect the DSI host core version and init the regmap with different register layouts. The platform drivers will continue to use the regmap without modifications or worrying about the specific layout in use (in other words the layout is abstracted away via the regmap). Suggested-by: Boris Brezillon Reviewed-by: Neil Armstrong Reviewed-by: Emil Velikov I should have been clearer earlier - I didn't quite review the patch. Is is now though. Reviewed-by: Emil Velikov Sorry about that, I got confused and thought you reviewed it all. Admittedly a couple of nitpicks (DRIVER_NAME, zero initialize of val) could have been left out. It's not a big deal, there's no need to polish those. I'll address them in v3 as well as updating your mail address. Thanks for reviewing! -Emil ___ linux-arm-kernel mailing list linux-arm-ker...@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/2] drm/ast: Replace drm_get_pci_device() and drm_put_dev()
On Wed, Nov 13, 2019 at 05:35:41PM +0100, Daniel Vetter wrote:
> On Wed, Nov 13, 2019 at 04:58:56PM +0100, Thomas Zimmermann wrote:
> > Both functions are deprecated. Open-code them them in preparation
> > of removing struct drm_driver.{load,unload}.
> >
> > Signed-off-by: Thomas Zimmermann
> > ---
> > drivers/gpu/drm/ast/ast_drv.c | 31 +--
> > 1 file changed, 29 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> > index d763da6f0834..78c90a3c903b 100644
> > --- a/drivers/gpu/drm/ast/ast_drv.c
> > +++ b/drivers/gpu/drm/ast/ast_drv.c
> > @@ -86,9 +86,35 @@ static void ast_kick_out_firmware_fb(struct pci_dev
> > *pdev)
> >
> > static int ast_pci_probe(struct pci_dev *pdev, const struct pci_device_id
> > *ent)
> > {
> > + struct drm_device *dev;
> > + int ret;
> > +
> > ast_kick_out_firmware_fb(pdev);
> >
> > - return drm_get_pci_dev(pdev, ent, &driver);
> > + ret = pci_enable_device(pdev);
> > + if (ret)
> > + return ret;
> > +
> > + dev = drm_dev_alloc(&driver, &pdev->dev);
>
> Would be nice to also switch over to embedding drm_device and
> drm_dev_init, but for another patch. This is
Even better: devm_drm_dev_init, and dropping the drm_dev_put/kfree at the
bottom of the pci_remove function.
-Daniel
>
> Reviewed-by: Daniel Vetter
>
> > + if (IS_ERR(dev)) {
> > + ret = PTR_ERR(dev);
> > + goto err_pci_disable_device;
> > + }
> > +
> > + dev->pdev = pdev;
> > + pci_set_drvdata(pdev, dev);
> > +
> > + ret = drm_dev_register(dev, ent->driver_data);
> > + if (ret)
> > + goto err_drm_dev_put;
> > +
> > + return 0;
> > +
> > +err_drm_dev_put:
> > + drm_dev_put(dev);
> > +err_pci_disable_device:
> > + pci_disable_device(pdev);
> > + return ret;
> > }
> >
> > static void
> > @@ -96,7 +122,8 @@ ast_pci_remove(struct pci_dev *pdev)
> > {
> > struct drm_device *dev = pci_get_drvdata(pdev);
> >
> > - drm_put_dev(dev);
> > + drm_dev_unregister(dev);
> > + drm_dev_put(dev);
> > }
> >
> > static int ast_drm_freeze(struct drm_device *dev)
> > --
> > 2.23.0
> >
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
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] drm/ast: Call struct drm_driver.{load, unload} before registering device
On Wed, Nov 13, 2019 at 04:58:57PM +0100, Thomas Zimmermann wrote:
> Both callbacks are deprecated. Remove them and call functions explicitly.
>
> Signed-off-by: Thomas Zimmermann
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/ast/ast_drv.c | 13 +
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
> index 78c90a3c903b..9da26750a22d 100644
> --- a/drivers/gpu/drm/ast/ast_drv.c
> +++ b/drivers/gpu/drm/ast/ast_drv.c
> @@ -104,17 +104,24 @@ static int ast_pci_probe(struct pci_dev *pdev, const
> struct pci_device_id *ent)
> dev->pdev = pdev;
> pci_set_drvdata(pdev, dev);
>
> - ret = drm_dev_register(dev, ent->driver_data);
> + ret = ast_driver_load(dev, ent->driver_data);
> if (ret)
> goto err_drm_dev_put;
>
> + ret = drm_dev_register(dev, ent->driver_data);
> + if (ret)
> + goto err_ast_driver_unload;
> +
> return 0;
>
> +err_ast_driver_unload:
> + ast_driver_unload(dev);
> err_drm_dev_put:
> drm_dev_put(dev);
> err_pci_disable_device:
> pci_disable_device(pdev);
> return ret;
> +
> }
>
> static void
> @@ -123,6 +130,7 @@ ast_pci_remove(struct pci_dev *pdev)
> struct drm_device *dev = pci_get_drvdata(pdev);
>
> drm_dev_unregister(dev);
> + ast_driver_unload(dev);
> drm_dev_put(dev);
> }
>
> @@ -228,9 +236,6 @@ static struct drm_driver driver = {
> DRIVER_GEM |
> DRIVER_MODESET,
>
> - .load = ast_driver_load,
> - .unload = ast_driver_unload,
> -
> .fops = &ast_fops,
> .name = DRIVER_NAME,
> .desc = DRIVER_DESC,
> --
> 2.23.0
>
--
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
[PATCH v2 02/13] drm/modes: parse_cmdline: Make various char pointers const
We are not supposed to modify the passed in string, make char pointers
used in drm_mode_parse_cmdline_options() const char * where possible.
Acked-by: Maxime Ripard
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_modes.c | 10 +-
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 3c3c7435225f..654d4b6fecb3 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1591,15 +1591,15 @@ static int drm_mode_parse_cmdline_int(const char
*delim, unsigned int *int_ret)
return 0;
}
-static int drm_mode_parse_cmdline_options(char *str, size_t len,
+static int drm_mode_parse_cmdline_options(const char *str, size_t len,
const struct drm_connector *connector,
struct drm_cmdline_mode *mode)
{
unsigned int deg, margin, rotation = 0;
- char *sep = str;
+ const char *sep = str;
while ((sep = strchr(sep, ','))) {
- char *delim, *option;
+ const char *delim, *option;
option = sep + 1;
delim = strchr(option, '=');
@@ -1718,8 +1718,8 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
bool named_mode = false, parse_extras = false;
unsigned int bpp_off = 0, refresh_off = 0, options_off = 0;
unsigned int mode_end = 0;
- char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
- char *options_ptr = NULL;
+ const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
+ const char *options_ptr = NULL;
char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
int ret;
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 05/13] drm/modes: parse_cmdline: Rework drm_mode_parse_cmdline_options()
Refactor drm_mode_parse_cmdline_options() so that it takes a pointer
to the first option, rather then a pointer to the ',' before the first
option.
This is a preparation patch for allowing parsing of stand-alone options
without a mode before them, e.g.: video=HDMI-1:margin_right=14,...
Acked-by: Maxime Ripard
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_modes.c | 21 +
1 file changed, 9 insertions(+), 12 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index f49401124727..25e8edf4cfb8 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1591,23 +1591,21 @@ static int drm_mode_parse_cmdline_int(const char
*delim, unsigned int *int_ret)
return 0;
}
-static int drm_mode_parse_cmdline_options(const char *str, size_t len,
+static int drm_mode_parse_cmdline_options(const char *str,
const struct drm_connector *connector,
struct drm_cmdline_mode *mode)
{
unsigned int deg, margin, rotation = 0;
- const char *sep = str;
+ const char *delim, *option, *sep;
- while ((sep = strchr(sep, ','))) {
- const char *delim, *option;
-
- option = sep + 1;
+ option = str;
+ do {
delim = strchr(option, '=');
if (!delim) {
delim = strchr(option, ',');
if (!delim)
- delim = str + len;
+ delim = option + strlen(option);
}
if (!strncmp(option, "rotate", delim - option)) {
@@ -1661,8 +1659,9 @@ static int drm_mode_parse_cmdline_options(const char
*str, size_t len,
} else {
return -EINVAL;
}
- sep = delim;
- }
+ sep = strchr(delim, ',');
+ option = sep + 1;
+ } while (sep);
mode->rotation_reflection = rotation;
@@ -1855,9 +1854,7 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
}
if (options_ptr) {
- int len = strlen(name) - (options_ptr - name);
-
- ret = drm_mode_parse_cmdline_options(options_ptr, len,
+ ret = drm_mode_parse_cmdline_options(options_ptr + 1,
connector, mode);
if (ret)
return false;
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 01/13] drm/modes: parse_cmdline: Fix possible reference past end of string
Before this commit, if the last option of a video=... option is for
example "rotate" without a "=" after it then delim will point to
the terminating 0 of the string, and value which is sets to
will point one position past the end of the string.
This commit fixes this by enforcing that the contents of delim equals '='
as it should be for options which take a value, this check is done in a
new drm_mode_parse_cmdline_int helper function which factors out the
common integer parsing code for all the options which take an int.
Acked-by: Maxime Ripard
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_modes.c | 68 -
1 file changed, 30 insertions(+), 38 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 88232698d7a0..3c3c7435225f 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1568,11 +1568,34 @@ static int drm_mode_parse_cmdline_res_mode(const char
*str, unsigned int length,
return 0;
}
+static int drm_mode_parse_cmdline_int(const char *delim, unsigned int *int_ret)
+{
+ const char *value;
+ char *endp;
+
+ /*
+* delim must point to the '=', otherwise it is a syntax error and
+* if delim points to the terminating zero, then delim + 1 wil point
+* past the end of the string.
+*/
+ if (*delim != '=')
+ return -EINVAL;
+
+ value = delim + 1;
+ *int_ret = simple_strtol(value, &endp, 10);
+
+ /* Make sure we have parsed something */
+ if (endp == value)
+ return -EINVAL;
+
+ return 0;
+}
+
static int drm_mode_parse_cmdline_options(char *str, size_t len,
const struct drm_connector *connector,
struct drm_cmdline_mode *mode)
{
- unsigned int rotation = 0;
+ unsigned int deg, margin, rotation = 0;
char *sep = str;
while ((sep = strchr(sep, ','))) {
@@ -1588,13 +1611,7 @@ static int drm_mode_parse_cmdline_options(char *str,
size_t len,
}
if (!strncmp(option, "rotate", delim - option)) {
- const char *value = delim + 1;
- unsigned int deg;
-
- deg = simple_strtol(value, &sep, 10);
-
- /* Make sure we have parsed something */
- if (sep == value)
+ if (drm_mode_parse_cmdline_int(delim, °))
return -EINVAL;
switch (deg) {
@@ -1619,57 +1636,32 @@ static int drm_mode_parse_cmdline_options(char *str,
size_t len,
}
} else if (!strncmp(option, "reflect_x", delim - option)) {
rotation |= DRM_MODE_REFLECT_X;
- sep = delim;
} else if (!strncmp(option, "reflect_y", delim - option)) {
rotation |= DRM_MODE_REFLECT_Y;
- sep = delim;
} else if (!strncmp(option, "margin_right", delim - option)) {
- const char *value = delim + 1;
- unsigned int margin;
-
- margin = simple_strtol(value, &sep, 10);
-
- /* Make sure we have parsed something */
- if (sep == value)
+ if (drm_mode_parse_cmdline_int(delim, &margin))
return -EINVAL;
mode->tv_margins.right = margin;
} else if (!strncmp(option, "margin_left", delim - option)) {
- const char *value = delim + 1;
- unsigned int margin;
-
- margin = simple_strtol(value, &sep, 10);
-
- /* Make sure we have parsed something */
- if (sep == value)
+ if (drm_mode_parse_cmdline_int(delim, &margin))
return -EINVAL;
mode->tv_margins.left = margin;
} else if (!strncmp(option, "margin_top", delim - option)) {
- const char *value = delim + 1;
- unsigned int margin;
-
- margin = simple_strtol(value, &sep, 10);
-
- /* Make sure we have parsed something */
- if (sep == value)
+ if (drm_mode_parse_cmdline_int(delim, &margin))
return -EINVAL;
mode->tv_margins.top = margin;
} else if (!strncmp(option, "margin_bottom", delim - option)) {
- const char *value = delim + 1;
- unsigned int margin;
-
- margin = simple_strtol(value, &sep, 10);
-
- /* Make sure we have parsed something */
-
[PATCH v2 03/13] drm/modes: parse_cmdline: Stop parsing extras after bpp / refresh at ', '
Before this commit it was impossible to add an extra mode argument after
a bpp or refresh specifier, combined with an option, e.g.
video=HDMI-1:720x480-24e,rotate=180 would not work, either the "e" to
force enable would need to be dropped or the ",rotate=180", otherwise
the mode_option would not be accepted.
This commit fixes this by fixing the length calculation if extras_ptr
is set to stop the extra parsing at the start of the options (stop at the
',' options_ptr points to).
Acked-by: Maxime Ripard
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_modes.c | 10 ---
.../gpu/drm/selftests/drm_cmdline_selftests.h | 1 +
.../drm/selftests/test-drm_cmdline_parser.c | 26 +++
3 files changed, 33 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 654d4b6fecb3..a8aa7955fd45 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1721,7 +1721,7 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
const char *options_ptr = NULL;
char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
- int ret;
+ int i, len, ret;
#ifdef CONFIG_FB
if (!mode_option)
@@ -1841,9 +1841,11 @@ bool drm_mode_parse_command_line_for_connector(const
char *mode_option,
else if (refresh_ptr)
extra_ptr = refresh_end_ptr;
- if (extra_ptr &&
- extra_ptr != options_ptr) {
- int len = strlen(name) - (extra_ptr - name);
+ if (extra_ptr) {
+ if (options_ptr)
+ len = options_ptr - extra_ptr;
+ else
+ len = strlen(extra_ptr);
ret = drm_mode_parse_cmdline_extra(extra_ptr, len, false,
connector, mode);
diff --git a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
index 6d61a0eb5d64..ca1fc7a78953 100644
--- a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
+++ b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
@@ -60,3 +60,4 @@ cmdline_test(drm_cmdline_test_vmirror)
cmdline_test(drm_cmdline_test_margin_options)
cmdline_test(drm_cmdline_test_multiple_options)
cmdline_test(drm_cmdline_test_invalid_option)
+cmdline_test(drm_cmdline_test_bpp_extra_and_option)
diff --git a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
index 013de9d27c35..5b8dea922257 100644
--- a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
+++ b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
@@ -992,6 +992,32 @@ static int drm_cmdline_test_invalid_option(void *ignored)
return 0;
}
+static int drm_cmdline_test_bpp_extra_and_option(void *ignored)
+{
+ struct drm_cmdline_mode mode = { };
+
+
FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480-24e,rotate=180",
+ &no_connector,
+ &mode));
+ FAIL_ON(!mode.specified);
+ FAIL_ON(mode.xres != 720);
+ FAIL_ON(mode.yres != 480);
+ FAIL_ON(mode.rotation_reflection != DRM_MODE_ROTATE_180);
+
+ FAIL_ON(mode.refresh_specified);
+
+ FAIL_ON(!mode.bpp_specified);
+ FAIL_ON(mode.bpp != 24);
+
+ FAIL_ON(mode.rb);
+ FAIL_ON(mode.cvt);
+ FAIL_ON(mode.interlace);
+ FAIL_ON(mode.margins);
+ FAIL_ON(mode.force != DRM_FORCE_ON);
+
+ return 0;
+}
+
#include "drm_selftest.c"
static int __init test_drm_cmdline_init(void)
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 09/13] drm/modes: parse_cmdline: Add support for specifying panel_orientation (v2)
Sometimes we want to override a connector's panel_orientation from the
kernel commandline. Either for testing and for special cases, e.g. a kiosk
like setup which uses a TV mounted in portrait mode.
Users can already specify a "rotate" option through a video= kernel cmdline
option. But that only supports 0/180 degrees (see drm_client_modeset TODO)
and only works for in kernel modeset clients, not for userspace kms users.
The "panel-orientation" connector property OTOH does support 90/270 degrees
as it leaves dealing with the rotation up to userspace and this does work
for userspace kms clients (at least those which support this property).
Changes in v2:
-Add missing ':' after @panel_orientation (reported by kbuild test robot)
BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83
Signed-off-by: Hans de Goede
---
Documentation/fb/modedb.rst | 3 ++
drivers/gpu/drm/drm_modes.c | 32 +++
.../gpu/drm/selftests/drm_cmdline_selftests.h | 1 +
.../drm/selftests/test-drm_cmdline_parser.c | 22 +
include/drm/drm_connector.h | 8 +
5 files changed, 66 insertions(+)
diff --git a/Documentation/fb/modedb.rst b/Documentation/fb/modedb.rst
index 9c4e3fd39e6d..624d08fd2856 100644
--- a/Documentation/fb/modedb.rst
+++ b/Documentation/fb/modedb.rst
@@ -65,6 +65,9 @@ Valid options are::
- reflect_y (boolean): Perform an axial symmetry on the Y axis
- rotate (integer): Rotate the initial framebuffer by x
degrees. Valid values are 0, 90, 180 and 270.
+ - panel_orientation, one of "normal", "upside_down", "left_side_up", or
+"right_side_up". For KMS drivers only, this sets the "panel orientation"
+property on the kms connector as hint for kms users.
-
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 2e82603f5d0a..119fed7ab815 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1591,6 +1591,33 @@ static int drm_mode_parse_cmdline_int(const char *delim,
unsigned int *int_ret)
return 0;
}
+static int drm_mode_parse_panel_orientation(const char *delim,
+ struct drm_cmdline_mode *mode)
+{
+ const char *value;
+
+ if (*delim != '=')
+ return -EINVAL;
+
+ value = delim + 1;
+ delim = strchr(value, ',');
+ if (!delim)
+ delim = value + strlen(value);
+
+ if (!strncmp(value, "normal", delim - value))
+ mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_NORMAL;
+ else if (!strncmp(value, "upside_down", delim - value))
+ mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_BOTTOM_UP;
+ else if (!strncmp(value, "left_side_up", delim - value))
+ mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_LEFT_UP;
+ else if (!strncmp(value, "right_side_up", delim - value))
+ mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_RIGHT_UP;
+ else
+ return -EINVAL;
+
+ return 0;
+}
+
static int drm_mode_parse_cmdline_options(const char *str,
bool freestanding,
const struct drm_connector *connector,
@@ -1657,6 +1684,9 @@ static int drm_mode_parse_cmdline_options(const char *str,
return -EINVAL;
mode->tv_margins.bottom = margin;
+ } else if (!strncmp(option, "panel_orientation", delim -
option)) {
+ if (drm_mode_parse_panel_orientation(delim, mode))
+ return -EINVAL;
} else {
return -EINVAL;
}
@@ -1715,6 +1745,8 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
int i, len, ret;
+ mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
+
#ifdef CONFIG_FB
if (!mode_option)
mode_option = fb_mode_option;
diff --git a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
index aee92ac2cc21..ceac7af9a172 100644
--- a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
+++ b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
@@ -64,3 +64,4 @@ cmdline_test(drm_cmdline_test_bpp_extra_and_option)
cmdline_test(drm_cmdline_test_extra_and_option)
cmdline_test(drm_cmdline_test_freestanding_options)
cmdline_test(drm_cmdline_test_freestanding_force_e_and_options)
+cmdline_test(drm_cmdline_test_panel_orientation)
diff --git a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
index 8248d4aa5aaa..520f3e66a384 100644
--- a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
+++ b/drivers/gpu/dr
[PATCH v2 11/13] drm/modes: parse_cmdline: Explicitly memset the passed in drm_cmdline_mode struct
Instead of only setting mode->specified on false on an early exit and
leaving e.g. mode->bpp_specified and mode->refresh_specified as is,
lets be consistent and just zero out the entire passed in struct at
the top of drm_mode_parse_command_line_for_connector()
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_modes.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index beb764efe6b3..1fee4a71eff7 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1745,12 +1745,11 @@ bool drm_mode_parse_command_line_for_connector(const
char *mode_option,
char *bpp_end_ptr = NULL, *refresh_end_ptr = NULL;
int i, len, ret;
+ memset(mode, 0, sizeof(*mode));
mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
- if (!mode_option) {
+ if (!mode_option)
mode->specified = false;
- return false;
- }
name = mode_option;
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 06/13] drm/modes: parse_cmdline: Add freestanding argument to drm_mode_parse_cmdline_options()
Add a freestanding function argument to drm_mode_parse_cmdline_options()
similar to how drm_mode_parse_cmdline_extra() already has this.
This is a preparation patch for allowing parsing of stand-alone options
without a mode before them, e.g.: video=HDMI-1:margin_right=14,...
Acked-by: Maxime Ripard
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_modes.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 25e8edf4cfb8..80cb247c83c7 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1592,6 +1592,7 @@ static int drm_mode_parse_cmdline_int(const char *delim,
unsigned int *int_ret)
}
static int drm_mode_parse_cmdline_options(const char *str,
+ bool freestanding,
const struct drm_connector *connector,
struct drm_cmdline_mode *mode)
{
@@ -1663,6 +1664,9 @@ static int drm_mode_parse_cmdline_options(const char *str,
option = sep + 1;
} while (sep);
+ if (rotation && freestanding)
+ return -EINVAL;
+
mode->rotation_reflection = rotation;
return 0;
@@ -1855,6 +1859,7 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
if (options_ptr) {
ret = drm_mode_parse_cmdline_options(options_ptr + 1,
+false,
connector, mode);
if (ret)
return false;
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 07/13] drm/modes: parse_cmdline: Set bpp/refresh_specified after successful parsing
drm_connector_get_cmdline_mode() calls
drm_mode_parse_command_line_for_connector() with &connector->cmdline_mode
as mode argument, so anything which we store in the mode arguments gets
kept even if we return false.
Avoid storing a possibly false-postive bpp/refresh_specified setting
in connector->cmdline_mode by moving the setting of these to after
successful parsing of the bpp/refresh parts of the video= argument.
Acked-by: Maxime Ripard
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_modes.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 80cb247c83c7..72828fa9fc91 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1771,10 +1771,8 @@ bool drm_mode_parse_command_line_for_connector(const
char *mode_option,
/* Try to locate the bpp and refresh specifiers, if any */
bpp_ptr = strchr(name, '-');
- if (bpp_ptr) {
+ if (bpp_ptr)
bpp_off = bpp_ptr - name;
- mode->bpp_specified = true;
- }
refresh_ptr = strchr(name, '@');
if (refresh_ptr) {
@@ -1782,7 +1780,6 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
return false;
refresh_off = refresh_ptr - name;
- mode->refresh_specified = true;
}
/* Locate the start of named options */
@@ -1825,6 +1822,8 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
ret = drm_mode_parse_cmdline_bpp(bpp_ptr, &bpp_end_ptr, mode);
if (ret)
return false;
+
+ mode->bpp_specified = true;
}
if (refresh_ptr) {
@@ -1832,6 +1831,8 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
&refresh_end_ptr, mode);
if (ret)
return false;
+
+ mode->refresh_specified = true;
}
/*
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 04/13] drm/modes: parse_cmdline: Accept extras directly after mode combined with options
Before this commit it was impossible to combine an extra mode argument
specified directly after the resolution with an option, e.g.
video=HDMI-1:720x480e,rotate=180 would not work, either the "e" to force
enable would need to be dropped or the ",rotate=180", otherwise the
mode_option would not be accepted.
This commit fixes this by setting parse_extras to true in this case, so
that drm_mode_parse_cmdline_res_mode() parses the extra arguments directly
after the resolution.
Acked-by: Maxime Ripard
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_modes.c | 1 +
.../gpu/drm/selftests/drm_cmdline_selftests.h | 1 +
.../drm/selftests/test-drm_cmdline_parser.c | 24 +++
3 files changed, 26 insertions(+)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index a8aa7955fd45..f49401124727 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1794,6 +1794,7 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
mode_end = refresh_off;
} else if (options_ptr) {
mode_end = options_off;
+ parse_extras = true;
} else {
mode_end = strlen(name);
parse_extras = true;
diff --git a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
index ca1fc7a78953..003e2c3ffc39 100644
--- a/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
+++ b/drivers/gpu/drm/selftests/drm_cmdline_selftests.h
@@ -61,3 +61,4 @@ cmdline_test(drm_cmdline_test_margin_options)
cmdline_test(drm_cmdline_test_multiple_options)
cmdline_test(drm_cmdline_test_invalid_option)
cmdline_test(drm_cmdline_test_bpp_extra_and_option)
+cmdline_test(drm_cmdline_test_extra_and_option)
diff --git a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
index 5b8dea922257..bc4db017e993 100644
--- a/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
+++ b/drivers/gpu/drm/selftests/test-drm_cmdline_parser.c
@@ -1018,6 +1018,30 @@ static int drm_cmdline_test_bpp_extra_and_option(void
*ignored)
return 0;
}
+static int drm_cmdline_test_extra_and_option(void *ignored)
+{
+ struct drm_cmdline_mode mode = { };
+
+
FAIL_ON(!drm_mode_parse_command_line_for_connector("720x480e,rotate=180",
+ &no_connector,
+ &mode));
+ FAIL_ON(!mode.specified);
+ FAIL_ON(mode.xres != 720);
+ FAIL_ON(mode.yres != 480);
+ FAIL_ON(mode.rotation_reflection != DRM_MODE_ROTATE_180);
+
+ FAIL_ON(mode.refresh_specified);
+ FAIL_ON(mode.bpp_specified);
+
+ FAIL_ON(mode.rb);
+ FAIL_ON(mode.cvt);
+ FAIL_ON(mode.interlace);
+ FAIL_ON(mode.margins);
+ FAIL_ON(mode.force != DRM_FORCE_ON);
+
+ return 0;
+}
+
#include "drm_selftest.c"
static int __init test_drm_cmdline_init(void)
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 12/13] drm/connector: Split out orientation quirk detection (v2)
From: Derek Basehore
Not every platform needs quirk detection for panel orientation, so
split the drm_connector_init_panel_orientation_property into two
functions. One for platforms without the need for quirks, and the
other for platforms that need quirks.
Hans de Goede (changes in v2):
Rename the function from drm_connector_init_panel_orientation_property
to drm_connector_set_panel_orientation[_with_quirk] and pass in the
panel-orientation to set.
Beside the rename, also make the function set the passed in value
only once, if the value was set before (to a value other then
DRM_MODE_PANEL_ORIENTATION_UNKNOWN) make any further set calls a no-op.
This change is preparation for allowing the user to override the
panel-orientation for any connector from the kernel commandline.
When the panel-orientation is overridden this way, then we must ignore
the panel-orientation detection done by the driver.
Signed-off-by: Derek Basehore
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_connector.c | 74 ++---
drivers/gpu/drm/i915/display/icl_dsi.c | 5 +-
drivers/gpu/drm/i915/display/intel_dp.c | 9 ++-
drivers/gpu/drm/i915/display/vlv_dsi.c | 5 +-
include/drm/drm_connector.h | 9 ++-
5 files changed, 71 insertions(+), 31 deletions(-)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 4c766624b20d..40a985c411a6 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -1114,7 +1114,8 @@ static const struct drm_prop_enum_list hdmi_colorspaces[]
= {
* coordinates, so if userspace rotates the picture to adjust for
* the orientation it must also apply the same transformation to the
* touchscreen input coordinates. This property is initialized by calling
- * drm_connector_init_panel_orientation_property().
+ * drm_connector_set_panel_orientation() or
+ * drm_connector_set_panel_orientation_with_quirk()
*
* scaling mode:
* This property defines how a non-native mode is upscaled to the native
@@ -1986,38 +1987,41 @@ void drm_connector_set_vrr_capable_property(
EXPORT_SYMBOL(drm_connector_set_vrr_capable_property);
/**
- * drm_connector_init_panel_orientation_property -
- * initialize the connecters panel_orientation property
- * @connector: connector for which to init the panel-orientation property.
- * @width: width in pixels of the panel, used for panel quirk detection
- * @height: height in pixels of the panel, used for panel quirk detection
+ * drm_connector_set_panel_orientation - sets the connecter's panel_orientation
+ * @connector: connector for which to set the panel-orientation property.
+ * @panel_orientation: drm_panel_orientation value to set
+ *
+ * This function sets the connector's panel_orientation and attaches
+ * a "panel orientation" property to the connector.
*
- * This function should only be called for built-in panels, after setting
- * connector->display_info.panel_orientation first (if known).
+ * Calling this function on a connector where the panel_orientation has
+ * already been set is a no-op (e.g. the orientation has been overridden with
+ * a kernel commandline option).
*
- * This function will check for platform specific (e.g. DMI based) quirks
- * overriding display_info.panel_orientation first, then if panel_orientation
- * is not DRM_MODE_PANEL_ORIENTATION_UNKNOWN it will attach the
- * "panel orientation" property to the connector.
+ * It is allowed to call this function with a panel_orientation of
+ * DRM_MODE_PANEL_ORIENTATION_UNKNOWN, in which case it is a no-op.
*
* Returns:
* Zero on success, negative errno on failure.
*/
-int drm_connector_init_panel_orientation_property(
- struct drm_connector *connector, int width, int height)
+int drm_connector_set_panel_orientation(
+ struct drm_connector *connector,
+ enum drm_panel_orientation panel_orientation)
{
struct drm_device *dev = connector->dev;
struct drm_display_info *info = &connector->display_info;
struct drm_property *prop;
- int orientation_quirk;
- orientation_quirk = drm_get_panel_orientation_quirk(width, height);
- if (orientation_quirk != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
- info->panel_orientation = orientation_quirk;
+ /* Already set? */
+ if (info->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
+ return 0;
- if (info->panel_orientation == DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
+ /* Don't attach the property if the orientation is unknown */
+ if (panel_orientation == DRM_MODE_PANEL_ORIENTATION_UNKNOWN)
return 0;
+ info->panel_orientation = panel_orientation;
+
prop = dev->mode_config.panel_orientation_property;
if (!prop) {
prop = drm_property_create_enum(dev, DRM_MODE_PROP_IMMUTABLE,
@@ -2034,7 +2038,37 @@ int drm_connector_init_panel_orientation_property(
[PATCH v2 08/13] drm/modes: parse_cmdline: Allow specifying stand-alone options
Some options which can be specified on the commandline, such as
margin_right=..., margin_left=..., etc. are applied not only to the
specified mode, but to all modes. As such it would be nice if the user
can simply say e.g.
video=HDMI-1:margin_right=14,margin_left=24,margin_bottom=36,margin_top=42
This commit refactors drm_mode_parse_command_line_for_connector() to
add support for this, and as a nice side effect also cleans up the
function a bit.
Acked-by: Maxime Ripard
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_modes.c | 92 +++
.../gpu/drm/selftests/drm_cmdline_selftests.h | 2 +
.../drm/selftests/test-drm_cmdline_parser.c | 50 ++
3 files changed, 86 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 72828fa9fc91..2e82603f5d0a 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1677,17 +1677,6 @@ static const char * const drm_named_modes_whitelist[] = {
"PAL",
};
-static bool drm_named_mode_is_in_whitelist(const char *mode, unsigned int size)
-{
- int i;
-
- for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++)
- if (!strncmp(mode, drm_named_modes_whitelist[i], size))
- return true;
-
- return false;
-}
-
/**
* drm_mode_parse_command_line_for_connector - parse command line modeline for
connector
* @mode_option: optional per connector mode option
@@ -1718,7 +1707,7 @@ bool drm_mode_parse_command_line_for_connector(const char
*mode_option,
struct drm_cmdline_mode *mode)
{
const char *name;
- bool named_mode = false, parse_extras = false;
+ bool freestanding = false, parse_extras = false;
unsigned int bpp_off = 0, refresh_off = 0, options_off = 0;
unsigned int mode_end = 0;
const char *bpp_ptr = NULL, *refresh_ptr = NULL, *extra_ptr = NULL;
@@ -1738,49 +1727,14 @@ bool drm_mode_parse_command_line_for_connector(const
char *mode_option,
name = mode_option;
- /*
-* This is a bit convoluted. To differentiate between the
-* named modes and poorly formatted resolutions, we need a
-* bunch of things:
-* - We need to make sure that the first character (which
-* would be our resolution in X) is a digit.
-* - If not, then it's either a named mode or a force on/off.
-* To distinguish between the two, we need to run the
-* extra parsing function, and if not, then we consider it
-* a named mode.
-*
-* If this isn't enough, we should add more heuristics here,
-* and matching unit-tests.
-*/
- if (!isdigit(name[0]) && name[0] != 'x') {
- unsigned int namelen = strlen(name);
-
- /*
-* Only the force on/off options can be in that case,
-* and they all take a single character.
-*/
- if (namelen == 1) {
- ret = drm_mode_parse_cmdline_extra(name, namelen, true,
- connector, mode);
- if (!ret)
- return true;
- }
-
- named_mode = true;
- }
-
/* Try to locate the bpp and refresh specifiers, if any */
bpp_ptr = strchr(name, '-');
if (bpp_ptr)
bpp_off = bpp_ptr - name;
refresh_ptr = strchr(name, '@');
- if (refresh_ptr) {
- if (named_mode)
- return false;
-
+ if (refresh_ptr)
refresh_off = refresh_ptr - name;
- }
/* Locate the start of named options */
options_ptr = strchr(name, ',');
@@ -1800,23 +1754,45 @@ bool drm_mode_parse_command_line_for_connector(const
char *mode_option,
parse_extras = true;
}
- if (named_mode) {
- if (mode_end + 1 > DRM_DISPLAY_MODE_LEN)
- return false;
+ /* First check for a named mode */
+ for (i = 0; i < ARRAY_SIZE(drm_named_modes_whitelist); i++) {
+ ret = str_has_prefix(name, drm_named_modes_whitelist[i]);
+ if (ret == mode_end) {
+ if (refresh_ptr)
+ return false; /* named + refresh is invalid */
- if (!drm_named_mode_is_in_whitelist(name, mode_end))
- return false;
+ strcpy(mode->name, drm_named_modes_whitelist[i]);
+ mode->specified = true;
+ break;
+ }
+ }
- strscpy(mode->name, name, mode_end + 1);
- } else {
+ /* No named mode? Check for a normal mode argument, e.g. 1024x768 */
+ if (!mode->specified && isdigit(name[0])) {
ret
[PATCH v2 10/13] drm/modes: parse_cmdline: Remove some unnecessary code (v2)
fb_get_options() will return fb_mode_option if no video=
argument is present on the kernel commandline, so there is no need to also
do this in drm_mode_parse_command_line_for_connector() as our only caller
uses fb_get_options() to get the mode_option argument.
Changes in v2:
-Split out the changes dealing with the initialization of the mode struct
into a separate patch
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_modes.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c
index 119fed7ab815..beb764efe6b3 100644
--- a/drivers/gpu/drm/drm_modes.c
+++ b/drivers/gpu/drm/drm_modes.c
@@ -1747,11 +1747,6 @@ bool drm_mode_parse_command_line_for_connector(const
char *mode_option,
mode->panel_orientation = DRM_MODE_PANEL_ORIENTATION_UNKNOWN;
-#ifdef CONFIG_FB
- if (!mode_option)
- mode_option = fb_mode_option;
-#endif
-
if (!mode_option) {
mode->specified = false;
return false;
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH v2 13/13] drm/connector: Hookup the new drm_cmdline_mode panel_orientation member
If the new video=... panel_orientation option is set for a connector, honor
it and setup a matching "panel orientation" property on the connector.
BugLink: https://gitlab.freedesktop.org/plymouth/plymouth/merge_requests/83
Signed-off-by: Hans de Goede
---
drivers/gpu/drm/drm_connector.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/drm_connector.c b/drivers/gpu/drm/drm_connector.c
index 40a985c411a6..591914f01cd4 100644
--- a/drivers/gpu/drm/drm_connector.c
+++ b/drivers/gpu/drm/drm_connector.c
@@ -140,6 +140,13 @@ static void drm_connector_get_cmdline_mode(struct
drm_connector *connector)
connector->force = mode->force;
}
+ if (mode->panel_orientation != DRM_MODE_PANEL_ORIENTATION_UNKNOWN) {
+ DRM_INFO("setting connector %s panel_orientation to %d\n",
+connector->name, mode->panel_orientation);
+ drm_connector_set_panel_orientation(connector,
+ mode->panel_orientation);
+ }
+
DRM_DEBUG_KMS("cmdline mode for connector %s %s %dx%d@%dHz%s%s%s\n",
connector->name, mode->name,
mode->xres, mode->yres,
--
2.23.0
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/7] drm/amdgpu: remove set but not used variable 'mc_shared_chmap' from 'gfx_v6_0.c' and 'gfx_v7_0.c'
On Wed, 2019-11-13 at 20:44 +0800, yu kuai wrote: > Fixes gcc '-Wunused-but-set-variable' warning: > > drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c: In function > ‘gfx_v6_0_constants_init’: > drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c:1579:6: warning: variable > ‘mc_shared_chmap’ set but not used [-Wunused-but-set-variable] [] > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c > b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c [] > @@ -1678,7 +1678,6 @@ static void gfx_v6_0_constants_init(struct > amdgpu_device *adev) > > WREG32(mmBIF_FB_EN, BIF_FB_EN__FB_READ_EN_MASK | > BIF_FB_EN__FB_WRITE_EN_MASK); > > - mc_shared_chmap = RREG32(mmMC_SHARED_CHMAP); I do not know the hardware but frequently hardware like this has read ordering requirements and various registers can not be read in a random order. Does removing this read have no effect on the hardware? > adev->gfx.config.mc_arb_ramcfg = RREG32(mmMC_ARB_RAMCFG); > mc_arb_ramcfg = adev->gfx.config.mc_arb_ramcfg; > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c > b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c [] > @@ -4336,7 +4336,6 @@ static void gfx_v7_0_gpu_early_init(struct > amdgpu_device *adev) > break; > } > > - mc_shared_chmap = RREG32(mmMC_SHARED_CHMAP); > adev->gfx.config.mc_arb_ramcfg = RREG32(mmMC_ARB_RAMCFG); > mc_arb_ramcfg = adev->gfx.config.mc_arb_ramcfg; Same question. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: UDL device cannot get its own screen
2019. 11. 12. 17:41 keltezéssel, Ilia Mirkin írta: On Tue, Nov 12, 2019 at 9:23 AM Böszörményi Zoltán wrote: But no, all GPU devices (now only one, the UDL device) have screen 0 (a.k.a. DISPLAY=:0.0) set when AutoBindGPU is true: [ 2444.576] xf86AutoConfigOutputDevices: xf86NumScreens 2 xf86NumGPUScreens 1 [ 2444.576] xf86AutoConfigOutputDevices: GPU #0 driver 'modesetting' 'modeset' scrnIndex 256 origIndex 257 pScreen->myNum 256 confScreen->screennum 0 confScreen->device->identifier 'Intel0' confScreen->device->screen 0 confScreen->device->myScreenSection->screennum 0 confScreen->device->myScreenSection->device->screen 0 Somehow, Option "Device" should ensure that the UDL device is actually treated as a framebuffer that can be rendered into (i.e. to be modeset(2) instead of modeset(Gn)) and it should be woken up automatically. This is what AutoBindGPU is supposed to do, isn't it? But instead of assigning to screen 0, it should be assigned to whatever screen number it is configured as. I know it's not a common use case nowadays, but I really want separate fullscreen apps on their independent screens, including a standalone UDL device, instead of having the latters as a Xinerama extension to some other device. If you see a "G", that means it's being treated as a GPU device, which is *not* what you want if you want separate screens. You need to try to convince things to *not* set the devices up as GPU devices, but instead put each device (and each one of its heads, via ZaphodHeads) no a separate device, which in turn will have a separate screen. I created a merge request that finally made it possible what I wanted. https://gitlab.freedesktop.org/xorg/xserver/merge_requests/334 Now, no matter if I use the intel or modesetting drivers for the Device sections using the Intel heads, or AutoBindGPU set to true or false, the UDL device is correctly matched with its Option "kmsdev" setting to the plaform device's device path. This patch seems to be a slight layering violation, but since the modesetting driver is built into the Xorg server sources, the patch may get away with it. Best regards, Zoltán Böszörményi ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/vmwgfx: Use coherent memory if there are dma mapping size restrictions
Hi,
On 11/13/19 3:16 PM, Christoph Hellwig wrote:
On Wed, Nov 13, 2019 at 10:51:43AM +0100, Thomas Hellström (VMware) wrote:
From: Thomas Hellstrom
We're gradually moving towards using DMA coherent memory in most
situations, although TTM interactions with the DMA layers is still a
work-in-progress. Meanwhile, use coherent memory when there are size
restrictions meaning that there is a chance that streaming dma mapping
of large buffer objects may fail.
Also move DMA mask settings to the vmw_dma_select_mode function, since
it's important that we set the correct DMA masks before calling the
dma_max_mapping_size() function.
Cc: Christoph Hellwig
Signed-off-by: Thomas Hellstrom
Reviewed-by: Brian Paul
---
drivers/gpu/drm/vmwgfx/vmwgfx_drv.c | 31 +++--
1 file changed, 7 insertions(+), 24 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
index fc0283659c41..1e1de83908fe 100644
--- a/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
+++ b/drivers/gpu/drm/vmwgfx/vmwgfx_drv.c
@@ -569,7 +569,10 @@ static int vmw_dma_select_mode(struct vmw_private
*dev_priv)
[vmw_dma_map_populate] = "Caching DMA mappings.",
[vmw_dma_map_bind] = "Giving up DMA mappings early."};
- if (vmw_force_coherent)
+ (void) dma_set_mask_and_coherent(dev_priv->dev->dev, DMA_BIT_MASK(64));
Please don't use void cast on returns. Also this genercally can't
fail, so if it fails anyway it is fatal, and you should actually
return an error.
OK. Will fix.
+ if (vmw_force_coherent ||
+ dma_max_mapping_size(dev_priv->dev->dev) != SIZE_MAX)
I don't think this is the right check to see if swiotlb would be
used. You probably want to call dma_addressing_limited(). Please
also add a comment explaining the call here.
If I understand things correctly, dma_addressing_limited() would always
return false on vmwgfx, at least if the "restrict to 44 bit" option is
removed. The "odd" modes we want to catch is someone setting
swiotlb=force, or someone enabling SEV. In both cases, vmwgfx would
break down due to limited DMA space even if streaming DMA was fixed with
appropriate sync calls.
The same holds for vmw_pvscsi (which we discussed before) where the
large queue depth will typically fill the SWIOTLB causing excessive
non-fatal error logging.
dma_max_mapping_size() currently catch these modes, but best I guess
would be dma_swiotlb_forced() function that could be used in cases like
this?
if (dev_priv->map_mode != vmw_dma_phys &&
AFAIK vmw_dma_phys can't even be set in current mainline and is dead
code. Can you check if I'm missing something? Certainly all three
branches above don't set it.
I'll remove that dead code for v2.
(sizeof(unsigned long) == 4 || vmw_restrict_dma_mask)) {
DRM_INFO("Restricting DMA addresses to 44 bits.\n");
- return dma_set_mask_and_coherent(dev->dev, DMA_BIT_MASK(44));
+ return dma_set_mask_and_coherent(dev_priv->dev->dev,
+DMA_BIT_MASK(44));
Can you keep setting the DMA mask together?
E.g. make the whole thing something like:
static int vmw_dma_select_mode(struct vmw_private *dev_priv)
{
if (dma_addressing_limited(dev_priv->dev->dev) || vmw_force_coherent) {
/*
* XXX: what about AMD iommu? virtio-iommu? Also
* swiotlb should be always available where we can
* address more than 32-bit of memory..
*/
if (!IS_ENABLED(CONFIG_SWIOTLB) &&
!IS_ENABLED(CONFIG_INTEL_IOMMU))
return -EINVAL;
The above check is only to see if coherent memory functionality is
really compiled in into TTM. We have a patchset that got lost in the
last merge window to fix this properly and avoid confusion about this.
I'll rebase on that.
dev_priv->map_mode = vmw_dma_alloc_coherent;
} else if (vmw_restrict_iommu) {
dev_priv->map_mode = vmw_dma_map_bind;
} else {
dev_priv->map_mode = vmw_dma_map_populate;
}
/*
* On 32-bit systems we can only handle 32 bit PFNs. Optionally set
* that restriction also for 64-bit systems.
*/
return dma_set_mask_and_coherent(dev->dev,
(IS_ENABLED(CONFIG_64BIT) && !vmw_restrict_dma_mask) ?
64 : 44);
}
Thanks,
Thomas
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: UDL device cannot get its own screen
On Wed, Nov 13, 2019 at 11:59 AM Böszörményi Zoltán wrote: > > 2019. 11. 12. 17:41 keltezéssel, Ilia Mirkin írta: > > On Tue, Nov 12, 2019 at 9:23 AM Böszörményi Zoltán wrote: > >> But no, all GPU devices (now only one, the UDL device) have screen 0 > >> (a.k.a. DISPLAY=:0.0) set when AutoBindGPU is true: > >> > >> [ 2444.576] xf86AutoConfigOutputDevices: xf86NumScreens 2 > >> xf86NumGPUScreens 1 > >> [ 2444.576] xf86AutoConfigOutputDevices: GPU #0 driver 'modesetting' > >> 'modeset' scrnIndex > >> 256 origIndex 257 pScreen->myNum 256 confScreen->screennum 0 > >> confScreen->device->identifier 'Intel0' > >>confScreen->device->screen 0 > >> confScreen->device->myScreenSection->screennum 0 > >> confScreen->device->myScreenSection->device->screen 0 > >> > >> Somehow, Option "Device" should ensure that the UDL device is actually > >> treated as a framebuffer that can be rendered into (i.e. to be modeset(2) > >> instead of modeset(Gn)) and it should be woken up automatically. > >> > >> This is what AutoBindGPU is supposed to do, isn't it? > >> > >> But instead of assigning to screen 0, it should be assigned to whatever > >> screen number it is configured as. > >> > >> I know it's not a common use case nowadays, but I really want separate > >> fullscreen apps on their independent screens, including a standalone UDL > >> device, instead of having the latters as a Xinerama extension to some > >> other device. > > > > If you see a "G", that means it's being treated as a GPU device, which > > is *not* what you want if you want separate screens. You need to try > > to convince things to *not* set the devices up as GPU devices, but > > instead put each device (and each one of its heads, via ZaphodHeads) > > no a separate device, which in turn will have a separate screen. > > I created a merge request that finally made it possible what I wanted. > > https://gitlab.freedesktop.org/xorg/xserver/merge_requests/334 > > Now, no matter if I use the intel or modesetting drivers for the > Device sections using the Intel heads, or AutoBindGPU set to true or > false, the UDL device is correctly matched with its Option "kmsdev" > setting to the plaform device's device path. > > This patch seems to be a slight layering violation, but since the > modesetting driver is built into the Xorg server sources, the patch > may get away with it. Have you looked at setting AutoAddGPU to false? AutoBindGPU is too late -- that's when you already have a GPU, whether to bind it to the primary device (/screen/whatever). You need to not have a GPU in the first place. -ilia ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/bridge: ti-tfp410: switch to using fwnode_gpiod_get_index()
On Wed, Nov 13, 2019 at 02:52:23PM +0100, Linus Walleij wrote: > On Tue, Nov 5, 2019 at 4:41 PM Daniel Vetter wrote: > > On Tue, Nov 5, 2019 at 4:29 PM Linus Walleij > > wrote: > > > On Tue, Nov 5, 2019 at 1:40 AM Dmitry Torokhov > > > wrote: > > > > I'm happy to merge it into the GPIO tree if some DRM maintainer can > > > provide an ACK. > > > > Ack. > > Thanks! > > > > Getting ACK from DRM people is problematic and a bit of friction in the > > > community, DVetter usually advice to seek mutual reviews etc, but IMO > > > it would be better if some people felt more compelled to review stuff > > > eventually. (And that has the problem that it doesn't scale.) > > > > This has a review already plus if you merge your implied review. > > Yeah I missed Laurent's review tag. I needed some kund of consent > to take it into the GPIO tree I suppose. > > > That's more than good enough imo, so not seeing the issue here? > > No issue. > > What freaked me out was the option of having to pull in an > immutable branch from my GPIO tree into drm-misc. That would > have been scary. Keeping it all in my tree works fine. For next time around, just ping one of the drm-misc (or drm maintainers) for an ack to merge it through a different tree. Or ask them what they want to do. committer model isn't 100% free-wheeling, for cross-tree stuff you still have maintainers who're supposed to do their jobs :-) But by default everyone will assume you'll just commit, so you need to poke them explicitly (like you've done here). -Daniel -- 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: Drm: mgag200. Video adapter issue with 5.4.0-rc3 ; no graphics
Hi Thomas: See below > On Nov 13, 2019, at 2:17 AM, Thomas Zimmermann wrote: > > Hi John > > Am 12.11.19 um 20:13 schrieb John Donnelly: >> >> In short . I started with : >> >> git bisect start >> >> git bisect bad be8454afc50f >> >> git bisect good fec88ab0af97 >> >> And at the the end of bisects showed this was the offending commit : >> >> c0a74c732568 >> >> commit c0a74c732568ad347f7b3de281922808dab30504 (refs/bisect/bad) >> Author: Jani Nikula >> Date: Fri May 24 20:35:22 2019 +0300 >> >>drm/i915: Update DRIVER_DATE to 20190524 >> >>Signed-off-by: Jani Nikula >> >> That does not have any real relevance > > No, that doesn't look right. The other bad commits you list below also > don't seem to be related. Thank you for your patience ; I started over and the bisect took to me to this change that certainly reflects the behavior I am seeing : commit 81da87f63a1edebcf8cbb811d387e353d9f89c7a (refs/bisect/bad) Author: Thomas Zimmermann Date: Tue May 21 13:08:29 2019 +0200 drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin The push-to-system function forces a buffer out of video RAM. This decision should rather be made by the memory manager. By replacing the function with calls to the kunmap and unpin functions, the buffer's memory becomes available, but the buffer remains in VRAM until it's evicted by a pin operation. This patch replaces the remaining instances of drm_gem_vram_push_to_system() in ast and mgag200, and removes the function from DRM. My 1st impression is we need a method that restores the previous behavior that pushes the content to the device . > > You could also bisect between your original working commit (v4.18?) and > the one you want to upgrade to (v5.3?). It's only a few more builds but > might be might give better results. > > BTW, are you also converting Gnome from X11 to Wayland. I found that > Gnome's Wayland code is so slow on mgag200 that it's unusable. They > recently added a shadow FB [1] to improve performance, but it won't be > available before Gnome 3.36. I found this issue using gnome-desktop3-3.28.2-1.el8.x86_64 If there is a more specific. RPM I can look at for guidance I will . > > Best regards > Thomas > > [1] https://gitlab.gnome.org/GNOME/mutter/merge_requests/877/diffs > >> >> >> I am not sure if I did the bisects correctly . After each test I did : >> >> >> #1 git bisect bad 827440a90146 >> >> #2 git bisect bad f5b07b04e5f0 >> >> #3 git bisect bad c0a74c732568 >> >> #4 git bisect good 818f5cb3e8fb >> >> #5 git bisect good 6cfe7ec02e85 >> >> #6 git bisect good f71e01a78bee >> >> #7 git bisect good 09a93ef3d60f >> >> #8 git bisect good f1e6b336bafa >> >> #9 git bisect good eaf20e6933dc >> >> #10 git bisect good 63e8dcdb4f8e >> >> #11 git bisect good 397049a03022 >> >> I’ve restarted the bisect without appending the after a the >> “bad|good “ , and so far git is showing the same selections. snip ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: UDL device cannot get its own screen
2019. 11. 13. 18:25 keltezéssel, Ilia Mirkin írta:
On Wed, Nov 13, 2019 at 11:59 AM Böszörményi Zoltán wrote:
2019. 11. 12. 17:41 keltezéssel, Ilia Mirkin írta:
On Tue, Nov 12, 2019 at 9:23 AM Böszörményi Zoltán wrote:
But no, all GPU devices (now only one, the UDL device) have screen 0
(a.k.a. DISPLAY=:0.0) set when AutoBindGPU is true:
[ 2444.576] xf86AutoConfigOutputDevices: xf86NumScreens 2 xf86NumGPUScreens 1
[ 2444.576] xf86AutoConfigOutputDevices: GPU #0 driver 'modesetting' 'modeset'
scrnIndex
256 origIndex 257 pScreen->myNum 256 confScreen->screennum 0
confScreen->device->identifier 'Intel0'
confScreen->device->screen 0 confScreen->device->myScreenSection->screennum 0
confScreen->device->myScreenSection->device->screen 0
Somehow, Option "Device" should ensure that the UDL device is actually
treated as a framebuffer that can be rendered into (i.e. to be modeset(2)
instead of modeset(Gn)) and it should be woken up automatically.
This is what AutoBindGPU is supposed to do, isn't it?
But instead of assigning to screen 0, it should be assigned to whatever
screen number it is configured as.
I know it's not a common use case nowadays, but I really want separate
fullscreen apps on their independent screens, including a standalone UDL
device, instead of having the latters as a Xinerama extension to some
other device.
If you see a "G", that means it's being treated as a GPU device, which
is *not* what you want if you want separate screens. You need to try
to convince things to *not* set the devices up as GPU devices, but
instead put each device (and each one of its heads, via ZaphodHeads)
no a separate device, which in turn will have a separate screen.
I created a merge request that finally made it possible what I wanted.
https://gitlab.freedesktop.org/xorg/xserver/merge_requests/334
Now, no matter if I use the intel or modesetting drivers for the
Device sections using the Intel heads, or AutoBindGPU set to true or
false, the UDL device is correctly matched with its Option "kmsdev"
setting to the plaform device's device path.
This patch seems to be a slight layering violation, but since the
modesetting driver is built into the Xorg server sources, the patch
may get away with it.
Have you looked at setting AutoAddGPU to false? AutoBindGPU is too
late -- that's when you already have a GPU, whether to bind it to the
primary device (/screen/whatever). You need to not have a GPU in the
first place.
Yes, I tried AutoAddGPU=false. Then the UDL device was not set up at all.
What I noticed in debugging Xorg via GDB is that the UDL device was
matched to the wrong platform device in xf86platformProbeDev.
xf86_platform_devices[0] == Intel, /dev/dri/card1, primary platform device
xf86_platform_devices[1] == UDL, /dev/dri/card0
devList[0] == "Intel0"
devList[1] == "Intel1"
devList[2] == "UDL"
devList[3] == "Intel2" (GPU device)
Since the device path was not matched and the PCI ID did not match,
(after all, the UDL device is NOT PCI), this code was executed:
else {
/* for non-seat0 servers assume first device is the master */
if (ServerIsNotSeat0())
break;
if (xf86IsPrimaryPlatform(&xf86_platform_devices[j]))
break;
}
So, probeSingleDevice() was called with xf86_platform_devices[0] and
devList[2], resulting in the UDL device set up as a GPU device and not
a framebuffer on its own right.
My MR modifies this so if there is an explicit Option "kmsdev" setting,
it's matched first. The final else branch is only executed in the default
case with no explicit configuration.
With this MR, the explicit configuration for UDL works, regardless the
AutoBindGPU value.
Best regards,
Zoltán Böszörményi
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 09/23] mm/gup: introduce pin_user_pages*() and FOLL_PIN
> > +/**
> > + * pin_user_pages_fast() - pin user pages in memory without taking locks
> > + *
> > + * Nearly the same as get_user_pages_fast(), except that FOLL_PIN is set.
> > See
> > + * get_user_pages_fast() for documentation on the function arguments,
> > because
> > + * the arguments here are identical.
> > + *
> > + * FOLL_PIN means that the pages must be released via put_user_page().
> > Please
> > + * see Documentation/vm/pin_user_pages.rst for further details.
> > + *
> > + * This is intended for Case 1 (DIO) in
> > Documentation/vm/pin_user_pages.rst. It
> > + * is NOT intended for Case 2 (RDMA: long-term pins).
> > + */
> > +int pin_user_pages_fast(unsigned long start, int nr_pages,
> > + unsigned int gup_flags, struct page **pages)
> > +{
> > + /* FOLL_GET and FOLL_PIN are mutually exclusive. */
> > + if (WARN_ON_ONCE(gup_flags & FOLL_GET))
> > + return -EINVAL;
> > +
> > + gup_flags |= FOLL_PIN;
> > + return internal_get_user_pages_fast(start, nr_pages, gup_flags, pages);
> > +}
> > +EXPORT_SYMBOL_GPL(pin_user_pages_fast);
>
> I was somewhat wondering about the number of functions you add here. So we
> have:
>
> pin_user_pages()
> pin_user_pages_fast()
> pin_user_pages_remote()
>
> and then longterm variants:
>
> pin_longterm_pages()
> pin_longterm_pages_fast()
> pin_longterm_pages_remote()
>
> and obviously we have gup like:
> get_user_pages()
> get_user_pages_fast()
> get_user_pages_remote()
> ... and some other gup variants ...
>
> I think we really should have pin_* vs get_* variants as they are very
> different in terms of guarantees and after conversion, any use of get_*
> variant in non-mm code should be closely scrutinized. OTOH pin_longterm_*
> don't look *that* useful to me and just using pin_* instead with
> FOLL_LONGTERM flag would look OK to me and somewhat reduce the number of
> functions which is already large enough? What do people think? I don't feel
> too strongly about this but wanted to bring this up.
I'm a bit concerned with the function explosion myself. I think what you
suggest is a happy medium. So I'd be ok with that.
Ira
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 03/23] mm/gup: move try_get_compound_head() to top, fix minor issues
On Tue, Nov 12, 2019 at 08:26:50PM -0800, John Hubbard wrote:
> An upcoming patch uses try_get_compound_head() more widely,
> so move it to the top of gup.c.
>
> Also fix a tiny spelling error and a checkpatch.pl warning.
>
> Signed-off-by: John Hubbard
Simple enough...
Reviewed-by: Ira Weiny
> ---
> mm/gup.c | 29 +++--
> 1 file changed, 15 insertions(+), 14 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 199da99e8ffc..933524de6249 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -29,6 +29,21 @@ struct follow_page_context {
> unsigned int page_mask;
> };
>
> +/*
> + * Return the compound head page with ref appropriately incremented,
> + * or NULL if that failed.
> + */
> +static inline struct page *try_get_compound_head(struct page *page, int refs)
> +{
> + struct page *head = compound_head(page);
> +
> + if (WARN_ON_ONCE(page_ref_count(head) < 0))
> + return NULL;
> + if (unlikely(!page_cache_add_speculative(head, refs)))
> + return NULL;
> + return head;
> +}
> +
> /**
> * put_user_pages_dirty_lock() - release and optionally dirty gup-pinned
> pages
> * @pages: array of pages to be maybe marked dirty, and definitely released.
> @@ -1793,20 +1808,6 @@ static void __maybe_unused undo_dev_pagemap(int *nr,
> int nr_start,
> }
> }
>
> -/*
> - * Return the compund head page with ref appropriately incremented,
> - * or NULL if that failed.
> - */
> -static inline struct page *try_get_compound_head(struct page *page, int refs)
> -{
> - struct page *head = compound_head(page);
> - if (WARN_ON_ONCE(page_ref_count(head) < 0))
> - return NULL;
> - if (unlikely(!page_cache_add_speculative(head, refs)))
> - return NULL;
> - return head;
> -}
> -
> #ifdef CONFIG_ARCH_HAS_PTE_SPECIAL
> static int gup_pte_range(pmd_t pmd, unsigned long addr, unsigned long end,
>unsigned int flags, struct page **pages, int *nr)
> --
> 2.24.0
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] video: fbdev: atyfb: only use ioremap_uc() on i386 and ia64
On Wed, Nov 13, 2019 at 11:31:54AM +0200, Andy Shevchenko wrote: > On Wed, Nov 13, 2019 at 08:38:15AM +0100, Arnd Bergmann wrote: > > On Wed, Nov 13, 2019 at 8:27 AM Christoph Hellwig wrote: > > > > > > On Tue, Nov 12, 2019 at 10:24:23PM +, Luis Chamberlain wrote: > > > > I think this would be possible if we could flop ioremap_nocache() to UC > > > > instead of UC- on x86. Otherwise, I can't see how we can remove this by > > > > still not allowing direct MTRR calls. > > > > > > If everything goes well ioremap_nocache will be gone as of 5.5. > > > > As ioremap_nocache() just an alias for ioremap(), I suppose the idea would > > then be to make x86 ioremap be UC instead of UC-, again matching what the > > other architectures do already. > > I think it's right thing to do, i.e. assume that ioremap() always does strong > UC independently on MTRR settings. Agreed wholeheartedly. What are the blockers from making that happen? Do we have any left? Luis ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 09/23] mm/gup: introduce pin_user_pages*() and FOLL_PIN
On Wed, Nov 13, 2019 at 2:43 AM Jan Kara wrote:
>
> On Tue 12-11-19 20:26:56, John Hubbard wrote:
> > Introduce pin_user_pages*() variations of get_user_pages*() calls,
> > and also pin_longterm_pages*() variations.
> >
> > These variants all set FOLL_PIN, which is also introduced, and
> > thoroughly documented.
> >
> > The pin_longterm*() variants also set FOLL_LONGTERM, in addition
> > to FOLL_PIN:
> >
> > pin_user_pages()
> > pin_user_pages_remote()
> > pin_user_pages_fast()
> >
> > pin_longterm_pages()
> > pin_longterm_pages_remote()
> > pin_longterm_pages_fast()
> >
> > All pages that are pinned via the above calls, must be unpinned via
> > put_user_page().
> >
> > The underlying rules are:
> >
> > * These are gup-internal flags, so the call sites should not directly
> > set FOLL_PIN nor FOLL_LONGTERM. That behavior is enforced with
> > assertions, for the new FOLL_PIN flag. However, for the pre-existing
> > FOLL_LONGTERM flag, which has some call sites that still directly
> > set FOLL_LONGTERM, there is no assertion yet.
> >
> > * Call sites that want to indicate that they are going to do DirectIO
> > ("DIO") or something with similar characteristics, should call a
> > get_user_pages()-like wrapper call that sets FOLL_PIN. These wrappers
> > will:
> > * Start with "pin_user_pages" instead of "get_user_pages". That
> > makes it easy to find and audit the call sites.
> > * Set FOLL_PIN
> >
> > * For pages that are received via FOLL_PIN, those pages must be returned
> > via put_user_page().
> >
> > Thanks to Jan Kara and Vlastimil Babka for explaining the 4 cases
> > in this documentation. (I've reworded it and expanded upon it.)
> >
> > Reviewed-by: Mike Rapoport # Documentation
> > Reviewed-by: Jérôme Glisse
> > Cc: Jonathan Corbet
> > Cc: Ira Weiny
> > Signed-off-by: John Hubbard
>
> Thanks for the documentation. It looks great!
>
> > diff --git a/mm/gup.c b/mm/gup.c
> > index 83702b2e86c8..4409e84dff51 100644
> > --- a/mm/gup.c
> > +++ b/mm/gup.c
> > @@ -201,6 +201,10 @@ static struct page *follow_page_pte(struct
> > vm_area_struct *vma,
> > spinlock_t *ptl;
> > pte_t *ptep, pte;
> >
> > + /* FOLL_GET and FOLL_PIN are mutually exclusive. */
> > + if (WARN_ON_ONCE((flags & (FOLL_PIN | FOLL_GET)) ==
> > + (FOLL_PIN | FOLL_GET)))
> > + return ERR_PTR(-EINVAL);
> > retry:
> > if (unlikely(pmd_bad(*pmd)))
> > return no_page_table(vma, flags);
>
> How does FOLL_PIN result in grabbing (at least normal, for now) page
> reference?
> I didn't find that anywhere in this patch but it is a prerequisite to
> converting any user to pin_user_pages() interface, right?
>
> > +/**
> > + * pin_user_pages_fast() - pin user pages in memory without taking locks
> > + *
> > + * Nearly the same as get_user_pages_fast(), except that FOLL_PIN is set.
> > See
> > + * get_user_pages_fast() for documentation on the function arguments,
> > because
> > + * the arguments here are identical.
> > + *
> > + * FOLL_PIN means that the pages must be released via put_user_page().
> > Please
> > + * see Documentation/vm/pin_user_pages.rst for further details.
> > + *
> > + * This is intended for Case 1 (DIO) in
> > Documentation/vm/pin_user_pages.rst. It
> > + * is NOT intended for Case 2 (RDMA: long-term pins).
> > + */
> > +int pin_user_pages_fast(unsigned long start, int nr_pages,
> > + unsigned int gup_flags, struct page **pages)
> > +{
> > + /* FOLL_GET and FOLL_PIN are mutually exclusive. */
> > + if (WARN_ON_ONCE(gup_flags & FOLL_GET))
> > + return -EINVAL;
> > +
> > + gup_flags |= FOLL_PIN;
> > + return internal_get_user_pages_fast(start, nr_pages, gup_flags,
> > pages);
> > +}
> > +EXPORT_SYMBOL_GPL(pin_user_pages_fast);
>
> I was somewhat wondering about the number of functions you add here. So we
> have:
>
> pin_user_pages()
> pin_user_pages_fast()
> pin_user_pages_remote()
>
> and then longterm variants:
>
> pin_longterm_pages()
> pin_longterm_pages_fast()
> pin_longterm_pages_remote()
>
> and obviously we have gup like:
> get_user_pages()
> get_user_pages_fast()
> get_user_pages_remote()
> ... and some other gup variants ...
>
> I think we really should have pin_* vs get_* variants as they are very
> different in terms of guarantees and after conversion, any use of get_*
> variant in non-mm code should be closely scrutinized. OTOH pin_longterm_*
> don't look *that* useful to me and just using pin_* instead with
> FOLL_LONGTERM flag would look OK to me and somewhat reduce the number of
> functions which is already large enough? What do people think? I don't feel
> too strongly about this but wanted to bring this up.
I'd vote for FOLL_LONGTERM should obviate the need for
{get,pin}_user_pages_longterm(). It's a property that is passed by the
call site, not an internal flag.
___
dri-devel mailing
Re: [PATCH v4 08/23] vfio, mm: fix get_user_pages_remote() and FOLL_LONGTERM
On Tue, Nov 12, 2019 at 08:26:55PM -0800, John Hubbard wrote:
> As it says in the updated comment in gup.c: current FOLL_LONGTERM
> behavior is incompatible with FAULT_FLAG_ALLOW_RETRY because of the
> FS DAX check requirement on vmas.
>
> However, the corresponding restriction in get_user_pages_remote() was
> slightly stricter than is actually required: it forbade all
> FOLL_LONGTERM callers, but we can actually allow FOLL_LONGTERM callers
> that do not set the "locked" arg.
>
> Update the code and comments accordingly, and update the VFIO caller
> to take advantage of this, fixing a bug as a result: the VFIO caller
> is logically a FOLL_LONGTERM user.
>
> Also, remove an unnessary pair of calls that were releasing and
> reacquiring the mmap_sem. There is no need to avoid holding mmap_sem
> just in order to call page_to_pfn().
>
> Also, move the DAX check ("if a VMA is DAX, don't allow long term
> pinning") from the VFIO call site, all the way into the internals
> of get_user_pages_remote() and __gup_longterm_locked(). That is:
> get_user_pages_remote() calls __gup_longterm_locked(), which in turn
> calls check_dax_vmas(). It's lightly explained in the comments as well.
>
> Thanks to Jason Gunthorpe for pointing out a clean way to fix this,
> and to Dan Williams for helping clarify the DAX refactoring.
>
> Suggested-by: Jason Gunthorpe
> Cc: Dan Williams
> Cc: Jerome Glisse
> Cc: Ira Weiny
Reviewed-by: Ira Weiny
> Signed-off-by: John Hubbard
> ---
> drivers/vfio/vfio_iommu_type1.c | 25 ++---
> mm/gup.c| 27 ++-
> 2 files changed, 24 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/vfio/vfio_iommu_type1.c b/drivers/vfio/vfio_iommu_type1.c
> index d864277ea16f..7301b710c9a4 100644
> --- a/drivers/vfio/vfio_iommu_type1.c
> +++ b/drivers/vfio/vfio_iommu_type1.c
> @@ -340,7 +340,6 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned
> long vaddr,
> {
> struct page *page[1];
> struct vm_area_struct *vma;
> - struct vm_area_struct *vmas[1];
> unsigned int flags = 0;
> int ret;
>
> @@ -348,33 +347,13 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned
> long vaddr,
> flags |= FOLL_WRITE;
>
> down_read(&mm->mmap_sem);
> - if (mm == current->mm) {
> - ret = get_user_pages(vaddr, 1, flags | FOLL_LONGTERM, page,
> - vmas);
> - } else {
> - ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags, page,
> - vmas, NULL);
> - /*
> - * The lifetime of a vaddr_get_pfn() page pin is
> - * userspace-controlled. In the fs-dax case this could
> - * lead to indefinite stalls in filesystem operations.
> - * Disallow attempts to pin fs-dax pages via this
> - * interface.
> - */
> - if (ret > 0 && vma_is_fsdax(vmas[0])) {
> - ret = -EOPNOTSUPP;
> - put_page(page[0]);
> - }
> - }
> - up_read(&mm->mmap_sem);
> -
> + ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags | FOLL_LONGTERM,
> + page, NULL, NULL);
> if (ret == 1) {
> *pfn = page_to_pfn(page[0]);
> return 0;
> }
>
> - down_read(&mm->mmap_sem);
> -
> vaddr = untagged_addr(vaddr);
>
> vma = find_vma_intersection(mm, vaddr, vaddr + 1);
> diff --git a/mm/gup.c b/mm/gup.c
> index 933524de6249..83702b2e86c8 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -29,6 +29,13 @@ struct follow_page_context {
> unsigned int page_mask;
> };
>
> +static __always_inline long __gup_longterm_locked(struct task_struct *tsk,
> + struct mm_struct *mm,
> + unsigned long start,
> + unsigned long nr_pages,
> + struct page **pages,
> + struct vm_area_struct **vmas,
> + unsigned int flags);
> /*
> * Return the compound head page with ref appropriately incremented,
> * or NULL if that failed.
> @@ -1167,13 +1174,23 @@ long get_user_pages_remote(struct task_struct *tsk,
> struct mm_struct *mm,
> struct vm_area_struct **vmas, int *locked)
> {
> /*
> - * FIXME: Current FOLL_LONGTERM behavior is incompatible with
> + * Parts of FOLL_LONGTERM behavior are incompatible with
>* FAULT_FLAG_ALLOW_RETRY because of the FS DAX check requirement on
> - * vmas. As there are no users of this flag in this call we simply
> - * disallow this option for now.
> + * vmas. However, this only comes up if locked is set, and there are
> + * callers that do request FOLL_LONGT
Re: [PATCH v4 09/23] mm/gup: introduce pin_user_pages*() and FOLL_PIN
On Tue, Nov 12, 2019 at 08:26:56PM -0800, John Hubbard wrote: > Introduce pin_user_pages*() variations of get_user_pages*() calls, > and also pin_longterm_pages*() variations. > > These variants all set FOLL_PIN, which is also introduced, and > thoroughly documented. > > The pin_longterm*() variants also set FOLL_LONGTERM, in addition > to FOLL_PIN: > > pin_user_pages() > pin_user_pages_remote() > pin_user_pages_fast() > > pin_longterm_pages() > pin_longterm_pages_remote() > pin_longterm_pages_fast() At some point in this conversation I thought we were going to put in "unpin_*" versions of these. Is that still in the plans? Ira ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 20/23] mm/gup_benchmark: use proper FOLL_WRITE flags instead of hard-coding "1"
On Tue, Nov 12, 2019 at 08:27:07PM -0800, John Hubbard wrote:
> Fix the gup benchmark flags to use the symbolic FOLL_WRITE,
> instead of a hard-coded "1" value.
>
> Also, clean up the filtering of gup flags a little, by just doing
> it once before issuing any of the get_user_pages*() calls. This
> makes it harder to overlook, instead of having little "gup_flags & 1"
> phrases in the function calls.
>
> Signed-off-by: John Hubbard
Reviewed-by: Ira Weiny
> ---
> mm/gup_benchmark.c | 9 ++---
> tools/testing/selftests/vm/gup_benchmark.c | 6 +-
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index 7dd602d7f8db..7fc44d25eca7 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -48,18 +48,21 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
> nr = (next - addr) / PAGE_SIZE;
> }
>
> + /* Filter out most gup flags: only allow a tiny subset here: */
> + gup->flags &= FOLL_WRITE;
> +
> switch (cmd) {
> case GUP_FAST_BENCHMARK:
> - nr = get_user_pages_fast(addr, nr, gup->flags & 1,
> + nr = get_user_pages_fast(addr, nr, gup->flags,
>pages + i);
> break;
> case GUP_LONGTERM_BENCHMARK:
> nr = get_user_pages(addr, nr,
> - (gup->flags & 1) | FOLL_LONGTERM,
> + gup->flags | FOLL_LONGTERM,
> pages + i, NULL);
> break;
> case GUP_BENCHMARK:
> - nr = get_user_pages(addr, nr, gup->flags & 1, pages + i,
> + nr = get_user_pages(addr, nr, gup->flags, pages + i,
> NULL);
> break;
> default:
> diff --git a/tools/testing/selftests/vm/gup_benchmark.c
> b/tools/testing/selftests/vm/gup_benchmark.c
> index 485cf06ef013..389327e9b30a 100644
> --- a/tools/testing/selftests/vm/gup_benchmark.c
> +++ b/tools/testing/selftests/vm/gup_benchmark.c
> @@ -18,6 +18,9 @@
> #define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
> #define GUP_BENCHMARK_IOWR('g', 3, struct gup_benchmark)
>
> +/* Just the flags we need, copied from mm.h: */
> +#define FOLL_WRITE 0x01/* check pte is writable */
> +
> struct gup_benchmark {
> __u64 get_delta_usec;
> __u64 put_delta_usec;
> @@ -85,7 +88,8 @@ int main(int argc, char **argv)
> }
>
> gup.nr_pages_per_call = nr_pages;
> - gup.flags = write;
> + if (write)
> + gup.flags |= FOLL_WRITE;
>
> fd = open("/sys/kernel/debug/gup_benchmark", O_RDWR);
> if (fd == -1)
> --
> 2.24.0
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 21/23] mm/gup_benchmark: support pin_user_pages() and related calls
On Tue, Nov 12, 2019 at 08:27:08PM -0800, John Hubbard wrote:
> Up until now, gup_benchmark supported testing of the
> following kernel functions:
>
> * get_user_pages(): via the '-U' command line option
> * get_user_pages_longterm(): via the '-L' command line option
> * get_user_pages_fast(): as the default (no options required)
>
> Add test coverage for the new corresponding pin_*() functions:
>
> * pin_user_pages(): via the '-c' command line option
> * pin_longterm_pages(): via the '-b' command line option
> * pin_user_pages_fast(): via the '-a' command line option
>
> Also, add an option for clarity: '-u' for what is now (still) the
> default choice: get_user_pages_fast().
>
> Also, for the three commands that set FOLL_PIN, verify that the pages
> really are dma-pinned, via the new is_dma_pinned() routine.
> Those commands are:
>
> PIN_FAST_BENCHMARK : calls pin_user_pages_fast()
> PIN_LONGTERM_BENCHMARK : calls pin_longterm_pages()
> PIN_BENCHMARK : calls pin_user_pages()
>
> In between the calls to pin_*() and put_user_pages(),
> check each page: if page_dma_pinned() returns false, then
> WARN and return.
>
> Do this outside of the benchmark timestamps, so that it doesn't
> affect reported times.
>
> Signed-off-by: John Hubbard
Reviewed-by: Ira Weiny
> ---
> mm/gup_benchmark.c | 73 --
> tools/testing/selftests/vm/gup_benchmark.c | 23 ++-
> 2 files changed, 90 insertions(+), 6 deletions(-)
>
> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index 7fc44d25eca7..8f980d91dbf5 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -8,6 +8,9 @@
> #define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_benchmark)
> #define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
> #define GUP_BENCHMARK_IOWR('g', 3, struct gup_benchmark)
> +#define PIN_FAST_BENCHMARK _IOWR('g', 4, struct gup_benchmark)
> +#define PIN_LONGTERM_BENCHMARK _IOWR('g', 5, struct gup_benchmark)
> +#define PIN_BENCHMARK_IOWR('g', 6, struct gup_benchmark)
>
> struct gup_benchmark {
> __u64 get_delta_usec;
> @@ -19,6 +22,44 @@ struct gup_benchmark {
> __u64 expansion[10];/* For future use */
> };
>
> +static void put_back_pages(int cmd, struct page **pages, unsigned long
> nr_pages)
> +{
> + int i;
> +
> + switch (cmd) {
> + case GUP_FAST_BENCHMARK:
> + case GUP_LONGTERM_BENCHMARK:
> + case GUP_BENCHMARK:
> + for (i = 0; i < nr_pages; i++)
> + put_page(pages[i]);
> + break;
> +
> + case PIN_FAST_BENCHMARK:
> + case PIN_LONGTERM_BENCHMARK:
> + case PIN_BENCHMARK:
> + put_user_pages(pages, nr_pages);
> + break;
> + }
> +}
> +
> +static void verify_dma_pinned(int cmd, struct page **pages,
> + unsigned long nr_pages)
> +{
> + int i;
> +
> + switch (cmd) {
> + case PIN_FAST_BENCHMARK:
> + case PIN_LONGTERM_BENCHMARK:
> + case PIN_BENCHMARK:
> + for (i = 0; i < nr_pages; i++) {
> + if (WARN(!page_dma_pinned(pages[i]),
> + "pages[%d] is NOT dma-pinned\n", i))
> + break;
> + }
> + break;
> + }
> +}
> +
> static int __gup_benchmark_ioctl(unsigned int cmd,
> struct gup_benchmark *gup)
> {
> @@ -65,6 +106,18 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
> nr = get_user_pages(addr, nr, gup->flags, pages + i,
> NULL);
> break;
> + case PIN_FAST_BENCHMARK:
> + nr = pin_user_pages_fast(addr, nr, gup->flags,
> + pages + i);
> + break;
> + case PIN_LONGTERM_BENCHMARK:
> + nr = pin_longterm_pages(addr, nr, gup->flags, pages + i,
> + NULL);
> + break;
> + case PIN_BENCHMARK:
> + nr = pin_user_pages(addr, nr, gup->flags, pages + i,
> + NULL);
> + break;
> default:
> return -1;
> }
> @@ -75,15 +128,22 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
> }
> end_time = ktime_get();
>
> + /* Shifting the meaning of nr_pages: now it is actual number pinned: */
> + nr_pages = i;
> +
> gup->get_delta_usec = ktime_us_delta(end_time, start_time);
> gup->size = addr - gup->addr;
>
> + /*
> + * Take an un-benchmark-timed moment to verify DMA pinned
> + * state: print a warning if any non-dma-pinned pages are found:
> + */
> + verify_dma_pinned(cmd, pages, nr_pages);
> +
> start_time = ktime_get();
> - for (i = 0; i < n
Re: [PATCH v4 22/23] selftests/vm: run_vmtests: invoke gup_benchmark with basic FOLL_PIN coverage
On Tue, Nov 12, 2019 at 08:27:09PM -0800, John Hubbard wrote: > It's good to have basic unit test coverage of the new FOLL_PIN > behavior. Fortunately, the gup_benchmark unit test is extremely > fast (a few milliseconds), so adding it the the run_vmtests suite > is going to cause no noticeable change in running time. > > So, add two new invocations to run_vmtests: > > 1) Run gup_benchmark with normal get_user_pages(). > > 2) Run gup_benchmark with pin_user_pages(). This is much like > the first call, except that it sets FOLL_PIN. > > Running these two in quick succession also provide a visual > comparison of the running times, which is convenient. > > The new invocations are fairly early in the run_vmtests script, > because with test suites, it's usually preferable to put the > shorter, faster tests first, all other things being equal. > > Signed-off-by: John Hubbard Reviewed-by: Ira Weiny > --- > tools/testing/selftests/vm/run_vmtests | 22 ++ > 1 file changed, 22 insertions(+) > > diff --git a/tools/testing/selftests/vm/run_vmtests > b/tools/testing/selftests/vm/run_vmtests > index 951c507a27f7..93e8dc9a7cad 100755 > --- a/tools/testing/selftests/vm/run_vmtests > +++ b/tools/testing/selftests/vm/run_vmtests > @@ -104,6 +104,28 @@ echo "NOTE: The above hugetlb tests provide minimal > coverage. Use" > echo " https://github.com/libhugetlbfs/libhugetlbfs.git for" > echo " hugetlb regression testing." > > +echo "" > +echo "running 'gup_benchmark -U' (normal/slow gup)" > +echo "" > +./gup_benchmark -U > +if [ $? -ne 0 ]; then > + echo "[FAIL]" > + exitcode=1 > +else > + echo "[PASS]" > +fi > + > +echo "--" > +echo "running gup_benchmark -c (pin_user_pages)" > +echo "--" > +./gup_benchmark -c > +if [ $? -ne 0 ]; then > + echo "[FAIL]" > + exitcode=1 > +else > + echo "[PASS]" > +fi > + > echo "---" > echo "running userfaultfd" > echo "---" > -- > 2.24.0 > ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 23/23] mm/gup: remove support for gup(FOLL_LONGTERM)
On Tue, Nov 12, 2019 at 08:27:10PM -0800, John Hubbard wrote:
> Now that all other kernel callers of get_user_pages(FOLL_LONGTERM)
> have been converted to pin_longterm_pages(), lock it down:
>
> 1) Add an assertion to get_user_pages(), preventing callers from
>passing FOLL_LONGTERM (in addition to the existing assertion that
>prevents FOLL_PIN).
>
> 2) Remove the associated GUP_LONGTERM_BENCHMARK test.
>
> Signed-off-by: John Hubbard
> ---
> mm/gup.c | 8
> mm/gup_benchmark.c | 9 +
> tools/testing/selftests/vm/gup_benchmark.c | 7 ++-
> 3 files changed, 7 insertions(+), 17 deletions(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 82e7e4ce5027..90f5f95ee7ac 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -1756,11 +1756,11 @@ long get_user_pages(unsigned long start, unsigned
> long nr_pages,
> struct vm_area_struct **vmas)
> {
> /*
> - * FOLL_PIN must only be set internally by the pin_user_page*() and
> - * pin_longterm_*() APIs, never directly by the caller, so enforce that
> - * with an assertion:
> + * FOLL_PIN and FOLL_LONGTERM must only be set internally by the
> + * pin_user_page*() and pin_longterm_*() APIs, never directly by the
> + * caller, so enforce that with an assertion:
>*/
> - if (WARN_ON_ONCE(gup_flags & FOLL_PIN))
> + if (WARN_ON_ONCE(gup_flags & (FOLL_PIN | FOLL_LONGTERM)))
Don't we want to block FOLL_LONGTERM in get_user_pages_fast() as well after all
this?
Ira
> return -EINVAL;
>
> return __gup_longterm_locked(current, current->mm, start, nr_pages,
> diff --git a/mm/gup_benchmark.c b/mm/gup_benchmark.c
> index 8f980d91dbf5..679f0e6a0adb 100644
> --- a/mm/gup_benchmark.c
> +++ b/mm/gup_benchmark.c
> @@ -6,7 +6,7 @@
> #include
>
> #define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_benchmark)
> -#define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
> +/* Command 2 has been deleted. */
> #define GUP_BENCHMARK_IOWR('g', 3, struct gup_benchmark)
> #define PIN_FAST_BENCHMARK _IOWR('g', 4, struct gup_benchmark)
> #define PIN_LONGTERM_BENCHMARK _IOWR('g', 5, struct gup_benchmark)
> @@ -28,7 +28,6 @@ static void put_back_pages(int cmd, struct page **pages,
> unsigned long nr_pages)
>
> switch (cmd) {
> case GUP_FAST_BENCHMARK:
> - case GUP_LONGTERM_BENCHMARK:
> case GUP_BENCHMARK:
> for (i = 0; i < nr_pages; i++)
> put_page(pages[i]);
> @@ -97,11 +96,6 @@ static int __gup_benchmark_ioctl(unsigned int cmd,
> nr = get_user_pages_fast(addr, nr, gup->flags,
>pages + i);
> break;
> - case GUP_LONGTERM_BENCHMARK:
> - nr = get_user_pages(addr, nr,
> - gup->flags | FOLL_LONGTERM,
> - pages + i, NULL);
> - break;
> case GUP_BENCHMARK:
> nr = get_user_pages(addr, nr, gup->flags, pages + i,
> NULL);
> @@ -159,7 +153,6 @@ static long gup_benchmark_ioctl(struct file *filep,
> unsigned int cmd,
>
> switch (cmd) {
> case GUP_FAST_BENCHMARK:
> - case GUP_LONGTERM_BENCHMARK:
> case GUP_BENCHMARK:
> case PIN_FAST_BENCHMARK:
> case PIN_LONGTERM_BENCHMARK:
> diff --git a/tools/testing/selftests/vm/gup_benchmark.c
> b/tools/testing/selftests/vm/gup_benchmark.c
> index 03928e47a86f..836b7082db13 100644
> --- a/tools/testing/selftests/vm/gup_benchmark.c
> +++ b/tools/testing/selftests/vm/gup_benchmark.c
> @@ -15,7 +15,7 @@
> #define PAGE_SIZE sysconf(_SC_PAGESIZE)
>
> #define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_benchmark)
> -#define GUP_LONGTERM_BENCHMARK _IOWR('g', 2, struct gup_benchmark)
> +/* Command 2 has been deleted. */
> #define GUP_BENCHMARK_IOWR('g', 3, struct gup_benchmark)
>
> /*
> @@ -49,7 +49,7 @@ int main(int argc, char **argv)
> char *file = "/dev/zero";
> char *p;
>
> - while ((opt = getopt(argc, argv, "m:r:n:f:abctTLUuwSH")) != -1) {
> + while ((opt = getopt(argc, argv, "m:r:n:f:abctTUuwSH")) != -1) {
> switch (opt) {
> case 'a':
> cmd = PIN_FAST_BENCHMARK;
> @@ -75,9 +75,6 @@ int main(int argc, char **argv)
> case 'T':
> thp = 0;
> break;
> - case 'L':
> - cmd = GUP_LONGTERM_BENCHMARK;
> - break;
> case 'U':
> cmd = GUP_BENCHMARK;
> break;
> --
> 2.24.0
>
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/li
Re: [PATCH v4 04/23] mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages
On Tue, Nov 12, 2019 at 08:26:51PM -0800, John Hubbard wrote:
> An upcoming patch changes and complicates the refcounting and
> especially the "put page" aspects of it. In order to keep
> everything clean, refactor the devmap page release routines:
>
> * Rename put_devmap_managed_page() to page_is_devmap_managed(),
> and limit the functionality to "read only": return a bool,
> with no side effects.
>
> * Add a new routine, put_devmap_managed_page(), to handle checking
> what kind of page it is, and what kind of refcount handling it
> requires.
>
> * Rename __put_devmap_managed_page() to free_devmap_managed_page(),
> and limit the functionality to unconditionally freeing a devmap
> page.
>
> This is originally based on a separate patch by Ira Weiny, which
> applied to an early version of the put_user_page() experiments.
> Since then, Jérôme Glisse suggested the refactoring described above.
>
> Suggested-by: Jérôme Glisse
> Signed-off-by: Ira Weiny
> Signed-off-by: John Hubbard
Reviewed-by: Jérôme Glisse
> ---
> include/linux/mm.h | 27 ---
> mm/memremap.c | 67 --
> 2 files changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a2adf95b3f9c..96228376139c 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -967,9 +967,10 @@ static inline bool is_zone_device_page(const struct page
> *page)
> #endif
>
> #ifdef CONFIG_DEV_PAGEMAP_OPS
> -void __put_devmap_managed_page(struct page *page);
> +void free_devmap_managed_page(struct page *page);
> DECLARE_STATIC_KEY_FALSE(devmap_managed_key);
> -static inline bool put_devmap_managed_page(struct page *page)
> +
> +static inline bool page_is_devmap_managed(struct page *page)
> {
> if (!static_branch_unlikely(&devmap_managed_key))
> return false;
> @@ -978,7 +979,6 @@ static inline bool put_devmap_managed_page(struct page
> *page)
> switch (page->pgmap->type) {
> case MEMORY_DEVICE_PRIVATE:
> case MEMORY_DEVICE_FS_DAX:
> - __put_devmap_managed_page(page);
> return true;
> default:
> break;
> @@ -986,6 +986,27 @@ static inline bool put_devmap_managed_page(struct page
> *page)
> return false;
> }
>
> +static inline bool put_devmap_managed_page(struct page *page)
> +{
> + bool is_devmap = page_is_devmap_managed(page);
> +
> + if (is_devmap) {
> + int count = page_ref_dec_return(page);
> +
> + /*
> + * devmap page refcounts are 1-based, rather than 0-based: if
> + * refcount is 1, then the page is free and the refcount is
> + * stable because nobody holds a reference on the page.
> + */
> + if (count == 1)
> + free_devmap_managed_page(page);
> + else if (!count)
> + __put_page(page);
> + }
> +
> + return is_devmap;
> +}
> +
> #else /* CONFIG_DEV_PAGEMAP_OPS */
> static inline bool put_devmap_managed_page(struct page *page)
> {
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 03ccbdfeb697..bc7e2a27d025 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -410,48 +410,39 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn,
> EXPORT_SYMBOL_GPL(get_dev_pagemap);
>
> #ifdef CONFIG_DEV_PAGEMAP_OPS
> -void __put_devmap_managed_page(struct page *page)
> +void free_devmap_managed_page(struct page *page)
> {
> - int count = page_ref_dec_return(page);
> + /* Clear Active bit in case of parallel mark_page_accessed */
> + __ClearPageActive(page);
> + __ClearPageWaiters(page);
> +
> + mem_cgroup_uncharge(page);
>
> /*
> - * If refcount is 1 then page is freed and refcount is stable as nobody
> - * holds a reference on the page.
> + * When a device_private page is freed, the page->mapping field
> + * may still contain a (stale) mapping value. For example, the
> + * lower bits of page->mapping may still identify the page as
> + * an anonymous page. Ultimately, this entire field is just
> + * stale and wrong, and it will cause errors if not cleared.
> + * One example is:
> + *
> + * migrate_vma_pages()
> + *migrate_vma_insert_page()
> + * page_add_new_anon_rmap()
> + *__page_set_anon_rmap()
> + * ...checks page->mapping, via PageAnon(page) call,
> + *and incorrectly concludes that the page is an
> + *anonymous page. Therefore, it incorrectly,
> + *silently fails to set up the new anon rmap.
> + *
> + * For other types of ZONE_DEVICE pages, migration is either
> + * handled differently or not done at all, so there is no need
> + * to clear page->mapping.
>*/
> - if (count == 1) {
> - /* Clear Active bit in case of parallel mark_page_accessed */
> -
Re: [PATCH v4 08/23] vfio, mm: fix get_user_pages_remote() and FOLL_LONGTERM
On Wed, Nov 13, 2019 at 09:02:02AM -0400, Jason Gunthorpe wrote:
> On Tue, Nov 12, 2019 at 08:26:55PM -0800, John Hubbard wrote:
> > As it says in the updated comment in gup.c: current FOLL_LONGTERM
> > behavior is incompatible with FAULT_FLAG_ALLOW_RETRY because of the
> > FS DAX check requirement on vmas.
> >
> > However, the corresponding restriction in get_user_pages_remote() was
> > slightly stricter than is actually required: it forbade all
> > FOLL_LONGTERM callers, but we can actually allow FOLL_LONGTERM callers
> > that do not set the "locked" arg.
> >
> > Update the code and comments accordingly, and update the VFIO caller
> > to take advantage of this, fixing a bug as a result: the VFIO caller
> > is logically a FOLL_LONGTERM user.
> >
> > Also, remove an unnessary pair of calls that were releasing and
> > reacquiring the mmap_sem. There is no need to avoid holding mmap_sem
> > just in order to call page_to_pfn().
> >
> > Also, move the DAX check ("if a VMA is DAX, don't allow long term
> > pinning") from the VFIO call site, all the way into the internals
> > of get_user_pages_remote() and __gup_longterm_locked(). That is:
> > get_user_pages_remote() calls __gup_longterm_locked(), which in turn
> > calls check_dax_vmas(). It's lightly explained in the comments as well.
> >
> > Thanks to Jason Gunthorpe for pointing out a clean way to fix this,
> > and to Dan Williams for helping clarify the DAX refactoring.
> >
> > Suggested-by: Jason Gunthorpe
> > Cc: Dan Williams
> > Cc: Jerome Glisse
> > Cc: Ira Weiny
> > Signed-off-by: John Hubbard
> > drivers/vfio/vfio_iommu_type1.c | 25 ++---
> > mm/gup.c| 27 ++-
> > 2 files changed, 24 insertions(+), 28 deletions(-)
> >
> > diff --git a/drivers/vfio/vfio_iommu_type1.c
> > b/drivers/vfio/vfio_iommu_type1.c
> > index d864277ea16f..7301b710c9a4 100644
> > +++ b/drivers/vfio/vfio_iommu_type1.c
> > @@ -340,7 +340,6 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned
> > long vaddr,
> > {
> > struct page *page[1];
> > struct vm_area_struct *vma;
> > - struct vm_area_struct *vmas[1];
> > unsigned int flags = 0;
> > int ret;
> >
> > @@ -348,33 +347,13 @@ static int vaddr_get_pfn(struct mm_struct *mm,
> > unsigned long vaddr,
> > flags |= FOLL_WRITE;
> >
> > down_read(&mm->mmap_sem);
> > - if (mm == current->mm) {
> > - ret = get_user_pages(vaddr, 1, flags | FOLL_LONGTERM, page,
> > -vmas);
> > - } else {
> > - ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags, page,
> > - vmas, NULL);
> > - /*
> > -* The lifetime of a vaddr_get_pfn() page pin is
> > -* userspace-controlled. In the fs-dax case this could
> > -* lead to indefinite stalls in filesystem operations.
> > -* Disallow attempts to pin fs-dax pages via this
> > -* interface.
> > -*/
> > - if (ret > 0 && vma_is_fsdax(vmas[0])) {
> > - ret = -EOPNOTSUPP;
> > - put_page(page[0]);
> > - }
> > - }
> > - up_read(&mm->mmap_sem);
> > -
> > + ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags | FOLL_LONGTERM,
> > + page, NULL, NULL);
> > if (ret == 1) {
> > *pfn = page_to_pfn(page[0]);
> > return 0;
>
> Mind the return with the lock held this needs some goto unwind
Ah yea... retract my reviewed by... :-(
Ira
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 1/7] drm/amdgpu: remove set but not used variable 'mc_shared_chmap' from 'gfx_v6_0.c' and 'gfx_v7_0.c'
On Wed, Nov 13, 2019 at 11:56 AM Joe Perches wrote: > > On Wed, 2019-11-13 at 20:44 +0800, yu kuai wrote: > > Fixes gcc '-Wunused-but-set-variable' warning: > > > > drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c: In function > > ‘gfx_v6_0_constants_init’: > > drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c:1579:6: warning: variable > > ‘mc_shared_chmap’ set but not used [-Wunused-but-set-variable] > [] > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c > > b/drivers/gpu/drm/amd/amdgpu/gfx_v6_0.c > [] > > @@ -1678,7 +1678,6 @@ static void gfx_v6_0_constants_init(struct > > amdgpu_device *adev) > > > > WREG32(mmBIF_FB_EN, BIF_FB_EN__FB_READ_EN_MASK | > > BIF_FB_EN__FB_WRITE_EN_MASK); > > > > - mc_shared_chmap = RREG32(mmMC_SHARED_CHMAP); > > I do not know the hardware but frequently hardware like > this has read ordering requirements and various registers > can not be read in a random order. > > Does removing this read have no effect on the hardware? There is no dependency. It's safe. Same thing below. Alex > > > adev->gfx.config.mc_arb_ramcfg = RREG32(mmMC_ARB_RAMCFG); > > mc_arb_ramcfg = adev->gfx.config.mc_arb_ramcfg; > > > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c > > b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c > [] > > @@ -4336,7 +4336,6 @@ static void gfx_v7_0_gpu_early_init(struct > > amdgpu_device *adev) > > break; > > } > > > > - mc_shared_chmap = RREG32(mmMC_SHARED_CHMAP); > > adev->gfx.config.mc_arb_ramcfg = RREG32(mmMC_ARB_RAMCFG); > > mc_arb_ramcfg = adev->gfx.config.mc_arb_ramcfg; > > Same question. > > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 04/23] mm: devmap: refactor 1-based refcounting for ZONE_DEVICE pages
On Tue, Nov 12, 2019 at 8:27 PM John Hubbard wrote:
>
> An upcoming patch changes and complicates the refcounting and
> especially the "put page" aspects of it. In order to keep
> everything clean, refactor the devmap page release routines:
>
> * Rename put_devmap_managed_page() to page_is_devmap_managed(),
> and limit the functionality to "read only": return a bool,
> with no side effects.
>
> * Add a new routine, put_devmap_managed_page(), to handle checking
> what kind of page it is, and what kind of refcount handling it
> requires.
>
> * Rename __put_devmap_managed_page() to free_devmap_managed_page(),
> and limit the functionality to unconditionally freeing a devmap
> page.
>
> This is originally based on a separate patch by Ira Weiny, which
> applied to an early version of the put_user_page() experiments.
> Since then, Jérôme Glisse suggested the refactoring described above.
>
> Suggested-by: Jérôme Glisse
> Signed-off-by: Ira Weiny
> Signed-off-by: John Hubbard
> ---
> include/linux/mm.h | 27 ---
> mm/memremap.c | 67 --
> 2 files changed, 53 insertions(+), 41 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a2adf95b3f9c..96228376139c 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -967,9 +967,10 @@ static inline bool is_zone_device_page(const struct page
> *page)
> #endif
>
> #ifdef CONFIG_DEV_PAGEMAP_OPS
> -void __put_devmap_managed_page(struct page *page);
> +void free_devmap_managed_page(struct page *page);
> DECLARE_STATIC_KEY_FALSE(devmap_managed_key);
> -static inline bool put_devmap_managed_page(struct page *page)
> +
> +static inline bool page_is_devmap_managed(struct page *page)
> {
> if (!static_branch_unlikely(&devmap_managed_key))
> return false;
> @@ -978,7 +979,6 @@ static inline bool put_devmap_managed_page(struct page
> *page)
> switch (page->pgmap->type) {
> case MEMORY_DEVICE_PRIVATE:
> case MEMORY_DEVICE_FS_DAX:
> - __put_devmap_managed_page(page);
> return true;
> default:
> break;
> @@ -986,6 +986,27 @@ static inline bool put_devmap_managed_page(struct page
> *page)
> return false;
> }
>
> +static inline bool put_devmap_managed_page(struct page *page)
> +{
> + bool is_devmap = page_is_devmap_managed(page);
> +
> + if (is_devmap) {
> + int count = page_ref_dec_return(page);
> +
> + /*
> +* devmap page refcounts are 1-based, rather than 0-based: if
> +* refcount is 1, then the page is free and the refcount is
> +* stable because nobody holds a reference on the page.
> +*/
> + if (count == 1)
> + free_devmap_managed_page(page);
> + else if (!count)
> + __put_page(page);
> + }
> +
> + return is_devmap;
> +}
> +
> #else /* CONFIG_DEV_PAGEMAP_OPS */
> static inline bool put_devmap_managed_page(struct page *page)
> {
> diff --git a/mm/memremap.c b/mm/memremap.c
> index 03ccbdfeb697..bc7e2a27d025 100644
> --- a/mm/memremap.c
> +++ b/mm/memremap.c
> @@ -410,48 +410,39 @@ struct dev_pagemap *get_dev_pagemap(unsigned long pfn,
> EXPORT_SYMBOL_GPL(get_dev_pagemap);
>
> #ifdef CONFIG_DEV_PAGEMAP_OPS
> -void __put_devmap_managed_page(struct page *page)
> +void free_devmap_managed_page(struct page *page)
> {
> - int count = page_ref_dec_return(page);
> + /* Clear Active bit in case of parallel mark_page_accessed */
> + __ClearPageActive(page);
> + __ClearPageWaiters(page);
> +
> + mem_cgroup_uncharge(page);
Ugh, when did all this HMM specific manipulation sneak into the
generic ZONE_DEVICE path? It used to be gated by pgmap type with its
own put_zone_device_private_page(). For example it's certainly
unnecessary and might be broken (would need to check) to call
mem_cgroup_uncharge() on a DAX page. ZONE_DEVICE users are not a
monolith and the HMM use case leaks pages into code paths that DAX
explicitly avoids.
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Drm: mgag200. Video adapter issue with 5.4.0-rc3 ; no graphics
> On Nov 13, 2019, at 11:42 AM, John Donnelly > wrote: > > Hi Thomas: See below > >> On Nov 13, 2019, at 2:17 AM, Thomas Zimmermann wrote: >> >> Hi John >> >> Am 12.11.19 um 20:13 schrieb John Donnelly: >>> >>> In short . I started with : >>> >>> git bisect start >>> >>> git bisect bad be8454afc50f >>> >>> git bisect good fec88ab0af97 >>> >>> And at the the end of bisects showed this was the offending commit : >>> >>> c0a74c732568 >>> >>> commit c0a74c732568ad347f7b3de281922808dab30504 (refs/bisect/bad) >>> Author: Jani Nikula >>> Date: Fri May 24 20:35:22 2019 +0300 >>> >>> drm/i915: Update DRIVER_DATE to 20190524 >>> >>> Signed-off-by: Jani Nikula >>> >>> That does not have any real relevance >> >> No, that doesn't look right. The other bad commits you list below also >> don't seem to be related. > > > > Thank you for your patience ; I started over and the bisect took to me to > this change that certainly reflects the behavior I am seeing : > > commit 81da87f63a1edebcf8cbb811d387e353d9f89c7a (refs/bisect/bad) > Author: Thomas Zimmermann > Date: Tue May 21 13:08:29 2019 +0200 > >drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin > >The push-to-system function forces a buffer out of video RAM. This decision >should rather be made by the memory manager. By replacing the function with >calls to the kunmap and unpin functions, the buffer's memory becomes > available, >but the buffer remains in VRAM until it's evicted by a pin operation. > >This patch replaces the remaining instances of > drm_gem_vram_push_to_system() >in ast and mgag200, and removes the function from DRM. > > > My 1st impression is we need a method that restores the previous behavior > that pushes the content to the device . It appears many of the code changes that were involved in 81da87f63a1 ( above ) have been superseded by : commit 5d17718997367c435dbe5341a8e270d9b19478d3 Author: Thomas Zimmermann Date: Thu Jun 27 10:09:09 2019 +0200 drm/mgag200: Replace struct mga_framebuffer with GEM framebuffer helpers Are there any other methods (tools ) you could recommend outside of starting Gnome that I could use to get a display activity shown on the console ? ===. snipped === ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: Drm: mgag200. Video adapter issue with 5.4.0-rc3 ; no graphics
On Wed, Nov 13, 2019 at 6:42 PM John Donnelly wrote: > > Hi Thomas: See below > > > On Nov 13, 2019, at 2:17 AM, Thomas Zimmermann wrote: > > > > Hi John > > > > Am 12.11.19 um 20:13 schrieb John Donnelly: > >> > >> In short . I started with : > >> > >> git bisect start > >> > >> git bisect bad be8454afc50f > >> > >> git bisect good fec88ab0af97 > >> > >> And at the the end of bisects showed this was the offending commit : > >> > >> c0a74c732568 > >> > >> commit c0a74c732568ad347f7b3de281922808dab30504 (refs/bisect/bad) > >> Author: Jani Nikula > >> Date: Fri May 24 20:35:22 2019 +0300 > >> > >>drm/i915: Update DRIVER_DATE to 20190524 > >> > >>Signed-off-by: Jani Nikula > >> > >> That does not have any real relevance > > > > No, that doesn't look right. The other bad commits you list below also > > don't seem to be related. > > > > Thank you for your patience ; I started over and the bisect took to me to > this change that certainly reflects the behavior I am seeing : > > commit 81da87f63a1edebcf8cbb811d387e353d9f89c7a (refs/bisect/bad) > Author: Thomas Zimmermann > Date: Tue May 21 13:08:29 2019 +0200 > > drm: Replace drm_gem_vram_push_to_system() with kunmap + unpin > > The push-to-system function forces a buffer out of video RAM. This > decision > should rather be made by the memory manager. By replacing the function > with > calls to the kunmap and unpin functions, the buffer's memory becomes > available, > but the buffer remains in VRAM until it's evicted by a pin operation. > > This patch replaces the remaining instances of > drm_gem_vram_push_to_system() > in ast and mgag200, and removes the function from DRM. Yeah that looks a lot more plausible as the culprit. > My 1st impression is we need a method that restores the previous behavior > that pushes the content to the device . Can you pls grab the debugsfs for the above broken kernel (without any of the later reworks on top), both for console mode and after you started gnome. Plus I guess booting with drm.debug=0xfe and full dmesg (please note the timestamp when you started gnome, that helps with sifting through it all, it's going to be a lot). You might need to grab the full dmesg from logs on disk, please make sure it includes everything from boot-up. Thanks, Daniel > > > > > > > > You could also bisect between your original working commit (v4.18?) and > > the one you want to upgrade to (v5.3?). It's only a few more builds but > > might be might give better results. > > > > BTW, are you also converting Gnome from X11 to Wayland. I found that > > Gnome's Wayland code is so slow on mgag200 that it's unusable. They > > recently added a shadow FB [1] to improve performance, but it won't be > > available before Gnome 3.36. > > > I found this issue using > > gnome-desktop3-3.28.2-1.el8.x86_64 > > If there is a more specific. RPM I can look at for guidance I will . > > > > > > Best regards > > Thomas > > > > [1] https://gitlab.gnome.org/GNOME/mutter/merge_requests/877/diffs > > > >> > >> > >> I am not sure if I did the bisects correctly . After each test I did : > >> > >> > >> #1 git bisect bad 827440a90146 > >> > >> #2 git bisect bad f5b07b04e5f0 > >> > >> #3 git bisect bad c0a74c732568 > >> > >> #4 git bisect good 818f5cb3e8fb > >> > >> #5 git bisect good 6cfe7ec02e85 > >> > >> #6 git bisect good f71e01a78bee > >> > >> #7 git bisect good 09a93ef3d60f > >> > >> #8 git bisect good f1e6b336bafa > >> > >> #9 git bisect good eaf20e6933dc > >> > >> #10 git bisect good 63e8dcdb4f8e > >> > >> #11 git bisect good 397049a03022 > >> > >> I’ve restarted the bisect without appending the after a the > >> “bad|good “ , and so far git is showing the same selections. > > > snip > ___ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: drm/vmwgfx: Use dma-coherent memory for high-bandwidth port messaging
Hi, Nathan, On 11/13/19 9:01 PM, Nathan Chancellor wrote: On Wed, Nov 13, 2019 at 10:51:42AM +0100, Thomas Hellström (VMware) wrote: From: Thomas Hellstrom With AMD-SEV high-bandwidth port messaging runs into trouble since the message content is encrypted using the vm-specific key, and the hypervisor is unable to read it. So use unencrypted dma-coherent bounce buffers for temporary message storage space. Allocating that memory is expensive so a future optimization might include a static unencrypted memory area for messages. Signed-off-by: Thomas Hellstrom Reviewed-by: Brian Paul Hi Thomas, The 0day team has been doing clang builds for us and sending the results to our mailing list for triage; this patch causes the following warning. Seems legitimate, mind taking a look at it and resolving it how you see fit? Cheers, Nathan This should be harmless as dma_free_coherent() with reply == NULL is a nop, but anyway I'll respin to silence the warning. Thanks, Thomas On Thu, Nov 14, 2019 at 03:36:44AM +0800, kbuild test robot wrote: CC: kbuild-...@lists.01.org In-Reply-To: <20191113095144.2981-1-thomas...@shipmail.org> References: <20191113095144.2981-1-thomas...@shipmail.org> TO: "Thomas Hellström (VMware)" CC: dri-devel@lists.freedesktop.org, Thomas Hellstrom , Brian Paul , Thomas Hellstrom , Brian Paul CC: Thomas Hellstrom , Brian Paul Hi "Thomas, I love your patch! Perhaps something to improve: [auto build test WARNING on linus/master] [also build test WARNING on v5.4-rc7 next-20191113] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system. BTW, we also suggest to use '--base' option to specify the base tree in git format-patch, please see https://stackoverflow.com/a/37406982] url: https://github.com/0day-ci/linux/commits/Thomas-Hellstr-m-VMware/drm-vmwgfx-Use-dma-coherent-memory-for-high-bandwidth-port-messaging/20191114-020818 base: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git 0e3f1ad80fc8cb0c517fd9a9afb22752b741fa76 config: x86_64-rhel-7.6 (attached as .config) compiler: clang version 10.0.0 (git://gitmirror/llvm_project 335ac2eb662ce5f1888e2a50310b01fba2d40d68) reproduce: # save the attached .config to linux build tree make ARCH=x86_64 If you fix the issue, kindly add following tag Reported-by: kbuild test robot All warnings (new ones prefixed by >>): drivers/gpu/drm/vmwgfx/vmwgfx_msg.c:441:6: warning: variable 'reply_handle' is used uninitialized whenever '||' condition is true [-Wsometimes-uninitialized] if (vmw_send_msg(&channel, msg) || ^~~ drivers/gpu/drm/vmwgfx/vmwgfx_msg.c:467:47: note: uninitialized use occurs here dma_free_coherent(dev, reply_len + 1, reply, reply_handle); ^~~~ drivers/gpu/drm/vmwgfx/vmwgfx_msg.c:441:6: note: remove the '||' if its condition is always false if (vmw_send_msg(&channel, msg) || ^~ drivers/gpu/drm/vmwgfx/vmwgfx_msg.c:421:37: note: initialize the variable 'reply_handle' to silence this warning dma_addr_t req_handle, reply_handle; ^ = 0 1 warning generated. vim +441 drivers/gpu/drm/vmwgfx/vmwgfx_msg.c 89da76fde68de1 Sinclair Yeh 2016-04-27 400 89da76fde68de1 Sinclair Yeh 2016-04-27 401 89da76fde68de1 Sinclair Yeh 2016-04-27 402 /** 89da76fde68de1 Sinclair Yeh 2016-04-27 403 * vmw_host_get_guestinfo: Gets a GuestInfo parameter 89da76fde68de1 Sinclair Yeh 2016-04-27 404 * 89da76fde68de1 Sinclair Yeh 2016-04-27 405 * Gets the value of a GuestInfo.* parameter. The value returned will be in 89da76fde68de1 Sinclair Yeh 2016-04-27 406 * a string, and it is up to the caller to post-process. 89da76fde68de1 Sinclair Yeh 2016-04-27 407 * 6bdb21230a2a01 Thomas Hellstrom 2019-11-13 408 * @dev: Pointer to struct device used for coherent memory allocation 89da76fde68de1 Sinclair Yeh 2016-04-27 409 * @guest_info_param: Parameter to get, e.g. GuestInfo.svga.gl3 89da76fde68de1 Sinclair Yeh 2016-04-27 410 * @buffer: if NULL, *reply_len will contain reply size. 89da76fde68de1 Sinclair Yeh 2016-04-27 411 * @length: size of the reply_buf. Set to size of reply upon return 89da76fde68de1 Sinclair Yeh 2016-04-27 412 * 89da76fde68de1 Sinclair Yeh 2016-04-27 413 * Returns: 0 on success 89da76fde68de1 Sinclair Yeh 2016-04-27 414 */ 6bdb21230a2a01 Thomas Hellstrom 2019-11-13 415 int vmw_host_get_guestinfo(struct device *dev, const char *guest_info_param, 89da76fde68de1 Sinclair Yeh 2016-04-27 416 char *buffer,
Re: [PATCH v4 08/23] vfio, mm: fix get_user_pages_remote() and FOLL_LONGTERM
On 11/13/19 11:17 AM, Ira Weiny wrote:
...
@@ -348,33 +347,13 @@ static int vaddr_get_pfn(struct mm_struct *mm, unsigned
long vaddr,
flags |= FOLL_WRITE;
down_read(&mm->mmap_sem);
- if (mm == current->mm) {
- ret = get_user_pages(vaddr, 1, flags | FOLL_LONGTERM, page,
-vmas);
- } else {
- ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags, page,
- vmas, NULL);
- /*
-* The lifetime of a vaddr_get_pfn() page pin is
-* userspace-controlled. In the fs-dax case this could
-* lead to indefinite stalls in filesystem operations.
-* Disallow attempts to pin fs-dax pages via this
-* interface.
-*/
- if (ret > 0 && vma_is_fsdax(vmas[0])) {
- ret = -EOPNOTSUPP;
- put_page(page[0]);
- }
- }
- up_read(&mm->mmap_sem);
-
+ ret = get_user_pages_remote(NULL, mm, vaddr, 1, flags | FOLL_LONGTERM,
+ page, NULL, NULL);
if (ret == 1) {
*pfn = page_to_pfn(page[0]);
return 0;
Mind the return with the lock held this needs some goto unwind
Ah yea... retract my reviewed by... :-(
ooops, embarrassed that I missed that, good catch. Will repost with it fixed.
thanks,
--
John Hubbard
NVIDIA
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v4 09/23] mm/gup: introduce pin_user_pages*() and FOLL_PIN
On 11/13/19 10:59 AM, Ira Weiny wrote: On Tue, Nov 12, 2019 at 08:26:56PM -0800, John Hubbard wrote: Introduce pin_user_pages*() variations of get_user_pages*() calls, and also pin_longterm_pages*() variations. These variants all set FOLL_PIN, which is also introduced, and thoroughly documented. The pin_longterm*() variants also set FOLL_LONGTERM, in addition to FOLL_PIN: pin_user_pages() pin_user_pages_remote() pin_user_pages_fast() pin_longterm_pages() pin_longterm_pages_remote() pin_longterm_pages_fast() At some point in this conversation I thought we were going to put in "unpin_*" versions of these. Is that still in the plans? Why yes it is! :) Daniel Vetter and Jan Kara both already weighed in [1], in favor of "unpin_user_page*()", rather than "put_user_page*()". I'll change those names. [1] https://lore.kernel.org/r/20191113101210.gd6...@quack2.suse.cz thanks, -- John Hubbard NVIDIA ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v3 00/23] mm/gup: track dma-pinned pages: FOLL_PIN, FOLL_LONGTERM
On 11/13/19 3:43 AM, Daniel Vetter wrote: ... Can't we call this unpin_user_page then, for some symmetry? Or is that even more churn? Looking from afar the naming here seems really confusing. That look from afar is valuable, because I'm too close to the problem to see how the naming looks. :) unpin_user_page() sounds symmetrical. It's true that it would cause more churn (which is why I started off with a proposal that avoids changing the names of put_user_page*() APIs). But OTOH, the amount of churn is proportional to the change in direction here, and it's really only 10 or 20 lines changed, in the end. So I'm open to changing to that naming. It would be nice to hear what others prefer, too... FWIW I'd find unpin_user_page() also better than put_user_page() as a counterpart to pin_user_pages(). One more point from afar on pin/unpin: We use that a lot in graphics for permanently pinned graphics buffer objects. Which really only should be used for scanout. So at least graphics folks should have an appropriate mindset and try to make sure we don't overuse this stuff. -Daniel OK, Ira also likes "unpin", and so far no one has said *anything* in favor of the "put_user_page" names, so I think we have a winner! I'll change the names to unpin_user_page*(). thanks, -- John Hubbard NVIDIA ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
