[PATCH 05/10] mm: replace get_vaddr_frames() write/force parameters with gup_flags
This patch removes the write and force parameters from get_vaddr_frames() and replaces them with a gup_flags parameter to make the use of FOLL_FORCE explicit in callers as use of this flag can result in surprising behaviour (and hence bugs) within the mm subsystem. Signed-off-by: Lorenzo Stoakes --- drivers/gpu/drm/exynos/exynos_drm_g2d.c| 3 ++- drivers/media/platform/omap/omap_vout.c| 2 +- drivers/media/v4l2-core/videobuf2-memops.c | 6 +- include/linux/mm.h | 2 +- mm/frame_vector.c | 13 ++--- 5 files changed, 11 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/exynos/exynos_drm_g2d.c b/drivers/gpu/drm/exynos/exynos_drm_g2d.c index aa92dec..fbd13fa 100644 --- a/drivers/gpu/drm/exynos/exynos_drm_g2d.c +++ b/drivers/gpu/drm/exynos/exynos_drm_g2d.c @@ -488,7 +488,8 @@ static dma_addr_t *g2d_userptr_get_dma_addr(struct drm_device *drm_dev, goto err_free; } - ret = get_vaddr_frames(start, npages, true, true, g2d_userptr->vec); + ret = get_vaddr_frames(start, npages, FOLL_FORCE | FOLL_WRITE, + g2d_userptr->vec); if (ret != npages) { DRM_ERROR("failed to get user pages from userptr.\n"); if (ret < 0) diff --git a/drivers/media/platform/omap/omap_vout.c b/drivers/media/platform/omap/omap_vout.c index e668dde..a31b95c 100644 --- a/drivers/media/platform/omap/omap_vout.c +++ b/drivers/media/platform/omap/omap_vout.c @@ -214,7 +214,7 @@ static int omap_vout_get_userptr(struct videobuf_buffer *vb, u32 virtp, if (!vec) return -ENOMEM; - ret = get_vaddr_frames(virtp, 1, true, false, vec); + ret = get_vaddr_frames(virtp, 1, FOLL_WRITE, vec); if (ret != 1) { frame_vector_destroy(vec); return -EINVAL; diff --git a/drivers/media/v4l2-core/videobuf2-memops.c b/drivers/media/v4l2-core/videobuf2-memops.c index 3c3b517..1cd322e 100644 --- a/drivers/media/v4l2-core/videobuf2-memops.c +++ b/drivers/media/v4l2-core/videobuf2-memops.c @@ -42,6 +42,10 @@ struct frame_vector *vb2_create_framevec(unsigned long start, unsigned long first, last; unsigned long nr; struct frame_vector *vec; + unsigned int flags = FOLL_FORCE; + + if (write) + flags |= FOLL_WRITE; first = start >> PAGE_SHIFT; last = (start + length - 1) >> PAGE_SHIFT; @@ -49,7 +53,7 @@ struct frame_vector *vb2_create_framevec(unsigned long start, vec = frame_vector_create(nr); if (!vec) return ERR_PTR(-ENOMEM); - ret = get_vaddr_frames(start & PAGE_MASK, nr, write, true, vec); + ret = get_vaddr_frames(start & PAGE_MASK, nr, flags, vec); if (ret < 0) goto out_destroy; /* We accept only complete set of PFNs */ diff --git a/include/linux/mm.h b/include/linux/mm.h index 27ab538..5ff084f6 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1305,7 +1305,7 @@ struct frame_vector { struct frame_vector *frame_vector_create(unsigned int nr_frames); void frame_vector_destroy(struct frame_vector *vec); int get_vaddr_frames(unsigned long start, unsigned int nr_pfns, -bool write, bool force, struct frame_vector *vec); +unsigned int gup_flags, struct frame_vector *vec); void put_vaddr_frames(struct frame_vector *vec); int frame_vector_to_pages(struct frame_vector *vec); void frame_vector_to_pfns(struct frame_vector *vec); diff --git a/mm/frame_vector.c b/mm/frame_vector.c index 81b6749..db77dcb 100644 --- a/mm/frame_vector.c +++ b/mm/frame_vector.c @@ -11,10 +11,7 @@ * get_vaddr_frames() - map virtual addresses to pfns * @start: starting user address * @nr_frames: number of pages / pfns from start to map - * @write: whether pages will be written to by the caller - * @force: whether to force write access even if user mapping is - * readonly. See description of the same argument of - get_user_pages(). + * @gup_flags: flags modifying lookup behaviour * @vec: structure which receives pages / pfns of the addresses mapped. * It should have space for at least nr_frames entries. * @@ -34,23 +31,17 @@ * This function takes care of grabbing mmap_sem as necessary. */ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, -bool write, bool force, struct frame_vector *vec) +unsigned int gup_flags, struct frame_vector *vec) { struct mm_struct *mm = current->mm; struct vm_area_struct *vma; int ret = 0; int err; int locked; - unsigned int gup_flags = 0; if (nr_frames == 0) return 0; - if (write) - gup_flags |= FOLL_WRITE; - if (force) - gup_flags |= FOLL_FORCE; - if (WARN_ON_ONCE(nr_frames > vec->nr_allocated
[PATCH 01/10] mm: remove write/force parameters from __get_user_pages_locked()
This patch removes the write and force parameters from __get_user_pages_locked() to make the use of FOLL_FORCE explicit in callers as use of this flag can result in surprising behaviour (and hence bugs) within the mm subsystem. Signed-off-by: Lorenzo Stoakes --- mm/gup.c | 47 +-- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/mm/gup.c b/mm/gup.c index 96b2b2f..ba83942 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -729,7 +729,6 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk, struct mm_struct *mm, unsigned long start, unsigned long nr_pages, - int write, int force, struct page **pages, struct vm_area_struct **vmas, int *locked, bool notify_drop, @@ -747,10 +746,6 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk, if (pages) flags |= FOLL_GET; - if (write) - flags |= FOLL_WRITE; - if (force) - flags |= FOLL_FORCE; pages_done = 0; lock_dropped = false; @@ -846,9 +841,15 @@ long get_user_pages_locked(unsigned long start, unsigned long nr_pages, int write, int force, struct page **pages, int *locked) { + unsigned int flags = FOLL_TOUCH; + + if (write) + flags |= FOLL_WRITE; + if (force) + flags |= FOLL_FORCE; + return __get_user_pages_locked(current, current->mm, start, nr_pages, - write, force, pages, NULL, locked, true, - FOLL_TOUCH); + pages, NULL, locked, true, flags); } EXPORT_SYMBOL(get_user_pages_locked); @@ -869,9 +870,15 @@ __always_inline long __get_user_pages_unlocked(struct task_struct *tsk, struct m { long ret; int locked = 1; + + if (write) + gup_flags |= FOLL_WRITE; + if (force) + gup_flags |= FOLL_FORCE; + down_read(&mm->mmap_sem); - ret = __get_user_pages_locked(tsk, mm, start, nr_pages, write, force, - pages, NULL, &locked, false, gup_flags); + ret = __get_user_pages_locked(tsk, mm, start, nr_pages, pages, NULL, + &locked, false, gup_flags); if (locked) up_read(&mm->mmap_sem); return ret; @@ -963,9 +970,15 @@ long get_user_pages_remote(struct task_struct *tsk, struct mm_struct *mm, int write, int force, struct page **pages, struct vm_area_struct **vmas) { - return __get_user_pages_locked(tsk, mm, start, nr_pages, write, force, - pages, vmas, NULL, false, - FOLL_TOUCH | FOLL_REMOTE); + unsigned int flags = FOLL_TOUCH | FOLL_REMOTE; + + if (write) + flags |= FOLL_WRITE; + if (force) + flags |= FOLL_FORCE; + + return __get_user_pages_locked(tsk, mm, start, nr_pages, pages, vmas, + NULL, false, flags); } EXPORT_SYMBOL(get_user_pages_remote); @@ -979,9 +992,15 @@ long get_user_pages(unsigned long start, unsigned long nr_pages, int write, int force, struct page **pages, struct vm_area_struct **vmas) { + unsigned int flags = FOLL_TOUCH; + + if (write) + flags |= FOLL_WRITE; + if (force) + flags |= FOLL_FORCE; + return __get_user_pages_locked(current, current->mm, start, nr_pages, - write, force, pages, vmas, NULL, false, - FOLL_TOUCH); + pages, vmas, NULL, false, flags); } EXPORT_SYMBOL(get_user_pages); -- 2.10.0
[PATCH 07/10] mm: replace get_user_pages_remote() write/force parameters with gup_flags
This patch removes the write and force parameters from get_user_pages_remote() and replaces them with a gup_flags parameter to make the use of FOLL_FORCE explicit in callers as use of this flag can result in surprising behaviour (and hence bugs) within the mm subsystem. Signed-off-by: Lorenzo Stoakes --- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 +-- drivers/gpu/drm/i915/i915_gem_userptr.c | 6 +- drivers/infiniband/core/umem_odp.c | 7 +-- fs/exec.c | 9 +++-- include/linux/mm.h | 2 +- kernel/events/uprobes.c | 6 -- mm/gup.c| 22 +++--- mm/memory.c | 6 +- security/tomoyo/domain.c| 2 +- 9 files changed, 40 insertions(+), 27 deletions(-) diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.c b/drivers/gpu/drm/etnaviv/etnaviv_gem.c index 5ce3603..0370b84 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.c @@ -748,19 +748,22 @@ static struct page **etnaviv_gem_userptr_do_get_pages( int ret = 0, pinned, npages = etnaviv_obj->base.size >> PAGE_SHIFT; struct page **pvec; uintptr_t ptr; + unsigned int flags = 0; pvec = drm_malloc_ab(npages, sizeof(struct page *)); if (!pvec) return ERR_PTR(-ENOMEM); + if (!etnaviv_obj->userptr.ro) + flags |= FOLL_WRITE; + pinned = 0; ptr = etnaviv_obj->userptr.ptr; down_read(&mm->mmap_sem); while (pinned < npages) { ret = get_user_pages_remote(task, mm, ptr, npages - pinned, - !etnaviv_obj->userptr.ro, 0, - pvec + pinned, NULL); + flags, pvec + pinned, NULL); if (ret < 0) break; diff --git a/drivers/gpu/drm/i915/i915_gem_userptr.c b/drivers/gpu/drm/i915/i915_gem_userptr.c index e537930..c6f780f 100644 --- a/drivers/gpu/drm/i915/i915_gem_userptr.c +++ b/drivers/gpu/drm/i915/i915_gem_userptr.c @@ -508,6 +508,10 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) pvec = drm_malloc_gfp(npages, sizeof(struct page *), GFP_TEMPORARY); if (pvec != NULL) { struct mm_struct *mm = obj->userptr.mm->mm; + unsigned int flags = 0; + + if (!obj->userptr.read_only) + flags |= FOLL_WRITE; ret = -EFAULT; if (atomic_inc_not_zero(&mm->mm_users)) { @@ -517,7 +521,7 @@ __i915_gem_userptr_get_pages_worker(struct work_struct *_work) (work->task, mm, obj->userptr.ptr + pinned * PAGE_SIZE, npages - pinned, -!obj->userptr.read_only, 0, +flags, pvec + pinned, NULL); if (ret < 0) break; diff --git a/drivers/infiniband/core/umem_odp.c b/drivers/infiniband/core/umem_odp.c index 75077a0..1f0fe32 100644 --- a/drivers/infiniband/core/umem_odp.c +++ b/drivers/infiniband/core/umem_odp.c @@ -527,6 +527,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt, u64 off; int j, k, ret = 0, start_idx, npages = 0; u64 base_virt_addr; + unsigned int flags = 0; if (access_mask == 0) return -EINVAL; @@ -556,6 +557,9 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt, goto out_put_task; } + if (access_mask & ODP_WRITE_ALLOWED_BIT) + flags |= FOLL_WRITE; + start_idx = (user_virt - ib_umem_start(umem)) >> PAGE_SHIFT; k = start_idx; @@ -574,8 +578,7 @@ int ib_umem_odp_map_dma_pages(struct ib_umem *umem, u64 user_virt, u64 bcnt, */ npages = get_user_pages_remote(owning_process, owning_mm, user_virt, gup_num_pages, - access_mask & ODP_WRITE_ALLOWED_BIT, - 0, local_page_list, NULL); + flags, local_page_list, NULL); up_read(&owning_mm->mmap_sem); if (npages < 0) diff --git a/fs/exec.c b/fs/exec.c index 6fcfb3f..4e497b9 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -191,6 +191,7 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos, { struct page *page; int ret; + unsigned int gup_flags = FOLL_FORCE; #ifdef CONFIG_STACK_GROWSUP if (write) { @@ -199,12 +200,16 @@ static struct page *get_arg_page(struct linux_binprm *bprm, unsigned long pos,
[PATCH 04/10] mm: replace get_user_pages_locked() write/force parameters with gup_flags
This patch removes the write and force parameters from get_user_pages_locked() and replaces them with a gup_flags parameter to make the use of FOLL_FORCE explicit in callers as use of this flag can result in surprising behaviour (and hence bugs) within the mm subsystem. Signed-off-by: Lorenzo Stoakes --- include/linux/mm.h | 2 +- mm/frame_vector.c | 8 +++- mm/gup.c | 12 +++- mm/nommu.c | 5 - 4 files changed, 15 insertions(+), 12 deletions(-) diff --git a/include/linux/mm.h b/include/linux/mm.h index 6adc4bc..27ab538 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1282,7 +1282,7 @@ long get_user_pages(unsigned long start, unsigned long nr_pages, int write, int force, struct page **pages, struct vm_area_struct **vmas); long get_user_pages_locked(unsigned long start, unsigned long nr_pages, - int write, int force, struct page **pages, int *locked); + unsigned int gup_flags, struct page **pages, int *locked); long __get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm, unsigned long start, unsigned long nr_pages, struct page **pages, unsigned int gup_flags); diff --git a/mm/frame_vector.c b/mm/frame_vector.c index 381bb07..81b6749 100644 --- a/mm/frame_vector.c +++ b/mm/frame_vector.c @@ -41,10 +41,16 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, int ret = 0; int err; int locked; + unsigned int gup_flags = 0; if (nr_frames == 0) return 0; + if (write) + gup_flags |= FOLL_WRITE; + if (force) + gup_flags |= FOLL_FORCE; + if (WARN_ON_ONCE(nr_frames > vec->nr_allocated)) nr_frames = vec->nr_allocated; @@ -59,7 +65,7 @@ int get_vaddr_frames(unsigned long start, unsigned int nr_frames, vec->got_ref = true; vec->is_pfns = false; ret = get_user_pages_locked(start, nr_frames, - write, force, (struct page **)(vec->ptrs), &locked); + gup_flags, (struct page **)(vec->ptrs), &locked); goto out; } diff --git a/mm/gup.c b/mm/gup.c index cfcb014..7a0d033 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -838,18 +838,12 @@ static __always_inline long __get_user_pages_locked(struct task_struct *tsk, * up_read(&mm->mmap_sem); */ long get_user_pages_locked(unsigned long start, unsigned long nr_pages, - int write, int force, struct page **pages, + unsigned int gup_flags, struct page **pages, int *locked) { - unsigned int flags = FOLL_TOUCH; - - if (write) - flags |= FOLL_WRITE; - if (force) - flags |= FOLL_FORCE; - return __get_user_pages_locked(current, current->mm, start, nr_pages, - pages, NULL, locked, true, flags); + pages, NULL, locked, true, + gup_flags | FOLL_TOUCH); } EXPORT_SYMBOL(get_user_pages_locked); diff --git a/mm/nommu.c b/mm/nommu.c index 7e27add..842cfdd 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -176,9 +176,12 @@ long get_user_pages(unsigned long start, unsigned long nr_pages, EXPORT_SYMBOL(get_user_pages); long get_user_pages_locked(unsigned long start, unsigned long nr_pages, - int write, int force, struct page **pages, + unsigned int gup_flags, struct page **pages, int *locked) { + int write = gup_flags & FOLL_WRITE; + int force = gup_flags & FOLL_FORCE; + return get_user_pages(start, nr_pages, write, force, pages, NULL); } EXPORT_SYMBOL(get_user_pages_locked); -- 2.10.0
[PATCH 08/10] mm: replace __access_remote_vm() write parameter with gup_flags
This patch removes the write parameter from __access_remote_vm() and replaces it with a gup_flags parameter as use of this function previously _implied_ FOLL_FORCE, whereas after this patch callers explicitly pass this flag. We make this explicit as use of FOLL_FORCE can result in surprising behaviour (and hence bugs) within the mm subsystem. Signed-off-by: Lorenzo Stoakes --- mm/memory.c | 23 +++ mm/nommu.c | 9 ++--- 2 files changed, 21 insertions(+), 11 deletions(-) diff --git a/mm/memory.c b/mm/memory.c index 20a9adb..79ebed3 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3869,14 +3869,11 @@ EXPORT_SYMBOL_GPL(generic_access_phys); * given task for page fault accounting. */ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm, - unsigned long addr, void *buf, int len, int write) + unsigned long addr, void *buf, int len, unsigned int gup_flags) { struct vm_area_struct *vma; void *old_buf = buf; - unsigned int flags = FOLL_FORCE; - - if (write) - flags |= FOLL_WRITE; + int write = gup_flags & FOLL_WRITE; down_read(&mm->mmap_sem); /* ignore errors, just check how much was successfully transferred */ @@ -3886,7 +3883,7 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm, struct page *page = NULL; ret = get_user_pages_remote(tsk, mm, addr, 1, - flags, &page, &vma); + gup_flags, &page, &vma); if (ret <= 0) { #ifndef CONFIG_HAVE_IOREMAP_PROT break; @@ -3945,7 +3942,12 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm, int access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf, int len, int write) { - return __access_remote_vm(NULL, mm, addr, buf, len, write); + unsigned int flags = FOLL_FORCE; + + if (write) + flags |= FOLL_WRITE; + + return __access_remote_vm(NULL, mm, addr, buf, len, flags); } /* @@ -3958,12 +3960,17 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, { struct mm_struct *mm; int ret; + unsigned int flags = FOLL_FORCE; mm = get_task_mm(tsk); if (!mm) return 0; - ret = __access_remote_vm(tsk, mm, addr, buf, len, write); + if (write) + flags |= FOLL_WRITE; + + ret = __access_remote_vm(tsk, mm, addr, buf, len, flags); + mmput(mm); return ret; diff --git a/mm/nommu.c b/mm/nommu.c index 70cb844..bde7df3 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -1809,9 +1809,10 @@ void filemap_map_pages(struct fault_env *fe, EXPORT_SYMBOL(filemap_map_pages); static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm, - unsigned long addr, void *buf, int len, int write) + unsigned long addr, void *buf, int len, unsigned int gup_flags) { struct vm_area_struct *vma; + int write = gup_flags & FOLL_WRITE; down_read(&mm->mmap_sem); @@ -1853,7 +1854,8 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm, int access_remote_vm(struct mm_struct *mm, unsigned long addr, void *buf, int len, int write) { - return __access_remote_vm(NULL, mm, addr, buf, len, write); + return __access_remote_vm(NULL, mm, addr, buf, len, + write ? FOLL_WRITE : 0); } /* @@ -1871,7 +1873,8 @@ int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, in if (!mm) return 0; - len = __access_remote_vm(tsk, mm, addr, buf, len, write); + len = __access_remote_vm(tsk, mm, addr, buf, len, + write ? FOLL_WRITE : 0); mmput(mm); return len; -- 2.10.0
[PATCH 02/10] mm: remove write/force parameters from __get_user_pages_unlocked()
This patch removes the write and force parameters from __get_user_pages_unlocked() to make the use of FOLL_FORCE explicit in callers as use of this flag can result in surprising behaviour (and hence bugs) within the mm subsystem. Signed-off-by: Lorenzo Stoakes --- include/linux/mm.h | 3 +-- mm/gup.c | 17 + mm/nommu.c | 12 +--- mm/process_vm_access.c | 7 +-- virt/kvm/async_pf.c| 3 ++- virt/kvm/kvm_main.c| 11 --- 6 files changed, 34 insertions(+), 19 deletions(-) diff --git a/include/linux/mm.h b/include/linux/mm.h index e9caec6..2db98b6 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1285,8 +1285,7 @@ long get_user_pages_locked(unsigned long start, unsigned long nr_pages, int write, int force, struct page **pages, int *locked); long __get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm, unsigned long start, unsigned long nr_pages, - int write, int force, struct page **pages, - unsigned int gup_flags); + struct page **pages, unsigned int gup_flags); long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, int write, int force, struct page **pages); int get_user_pages_fast(unsigned long start, int nr_pages, int write, diff --git a/mm/gup.c b/mm/gup.c index ba83942..3d620dd 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -865,17 +865,11 @@ EXPORT_SYMBOL(get_user_pages_locked); */ __always_inline long __get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm, unsigned long start, unsigned long nr_pages, - int write, int force, struct page **pages, - unsigned int gup_flags) + struct page **pages, unsigned int gup_flags) { long ret; int locked = 1; - if (write) - gup_flags |= FOLL_WRITE; - if (force) - gup_flags |= FOLL_FORCE; - down_read(&mm->mmap_sem); ret = __get_user_pages_locked(tsk, mm, start, nr_pages, pages, NULL, &locked, false, gup_flags); @@ -905,8 +899,15 @@ EXPORT_SYMBOL(__get_user_pages_unlocked); long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, int write, int force, struct page **pages) { + unsigned int flags = FOLL_TOUCH; + + if (write) + flags |= FOLL_WRITE; + if (force) + flags |= FOLL_FORCE; + return __get_user_pages_unlocked(current, current->mm, start, nr_pages, -write, force, pages, FOLL_TOUCH); +pages, flags); } EXPORT_SYMBOL(get_user_pages_unlocked); diff --git a/mm/nommu.c b/mm/nommu.c index 95daf81..925dcc1 100644 --- a/mm/nommu.c +++ b/mm/nommu.c @@ -185,8 +185,7 @@ EXPORT_SYMBOL(get_user_pages_locked); long __get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm, unsigned long start, unsigned long nr_pages, - int write, int force, struct page **pages, - unsigned int gup_flags) + struct page **pages, unsigned int gup_flags) { long ret; down_read(&mm->mmap_sem); @@ -200,8 +199,15 @@ EXPORT_SYMBOL(__get_user_pages_unlocked); long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, int write, int force, struct page **pages) { + unsigned int flags = 0; + + if (write) + flags |= FOLL_WRITE; + if (force) + flags |= FOLL_FORCE; + return __get_user_pages_unlocked(current, current->mm, start, nr_pages, -write, force, pages, 0); +pages, flags); } EXPORT_SYMBOL(get_user_pages_unlocked); diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c index 07514d4..be8dc8d 100644 --- a/mm/process_vm_access.c +++ b/mm/process_vm_access.c @@ -88,12 +88,16 @@ static int process_vm_rw_single_vec(unsigned long addr, ssize_t rc = 0; unsigned long max_pages_per_loop = PVM_MAX_KMALLOC_PAGES / sizeof(struct pages *); + unsigned int flags = FOLL_REMOTE; /* Work out address and page range required */ if (len == 0) return 0; nr_pages = (addr + len - 1) / PAGE_SIZE - addr / PAGE_SIZE + 1; + if (vm_write) + flags |= FOLL_WRITE; + while (!rc && nr_pages && iov_iter_count(iter)) { int pages = min(nr_pages, max_pages_per_loop); size_t bytes; @@ -104,8 +108,7 @@ static
[PATCH 06/10] mm: replace get_user_pages() write/force parameters with gup_flags
This patch removes the write and force parameters from get_user_pages() and replaces them with a gup_flags parameter to make the use of FOLL_FORCE explicit in callers as use of this flag can result in surprising behaviour (and hence bugs) within the mm subsystem. Signed-off-by: Lorenzo Stoakes --- arch/cris/arch-v32/drivers/cryptocop.c | 4 +--- arch/ia64/kernel/err_inject.c | 2 +- arch/x86/mm/mpx.c | 5 ++--- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 7 +-- drivers/gpu/drm/radeon/radeon_ttm.c| 3 ++- drivers/gpu/drm/via/via_dmablit.c | 4 ++-- drivers/infiniband/core/umem.c | 6 +- drivers/infiniband/hw/mthca/mthca_memfree.c| 2 +- drivers/infiniband/hw/qib/qib_user_pages.c | 3 ++- drivers/infiniband/hw/usnic/usnic_uiom.c | 5 - drivers/media/v4l2-core/videobuf-dma-sg.c | 7 +-- drivers/misc/mic/scif/scif_rma.c | 3 +-- drivers/misc/sgi-gru/grufault.c| 2 +- drivers/platform/goldfish/goldfish_pipe.c | 3 ++- drivers/rapidio/devices/rio_mport_cdev.c | 3 ++- .../vc04_services/interface/vchiq_arm/vchiq_2835_arm.c | 3 +-- .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +-- drivers/virt/fsl_hypervisor.c | 4 ++-- include/linux/mm.h | 2 +- mm/gup.c | 12 +++- mm/mempolicy.c | 2 +- mm/nommu.c | 18 -- 22 files changed, 49 insertions(+), 54 deletions(-) diff --git a/arch/cris/arch-v32/drivers/cryptocop.c b/arch/cris/arch-v32/drivers/cryptocop.c index b5698c8..099e170 100644 --- a/arch/cris/arch-v32/drivers/cryptocop.c +++ b/arch/cris/arch-v32/drivers/cryptocop.c @@ -2722,7 +2722,6 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig err = get_user_pages((unsigned long int)(oper.indata + prev_ix), noinpages, 0, /* read access only for in data */ -0, /* no force */ inpages, NULL); @@ -2736,8 +2735,7 @@ static int cryptocop_ioctl_process(struct inode *inode, struct file *filp, unsig if (oper.do_cipher){ err = get_user_pages((unsigned long int)oper.cipher_outdata, nooutpages, -1, /* write access for out data */ -0, /* no force */ +FOLL_WRITE, /* write access for out data */ outpages, NULL); up_read(¤t->mm->mmap_sem); diff --git a/arch/ia64/kernel/err_inject.c b/arch/ia64/kernel/err_inject.c index 09f8457..5ed0ea9 100644 --- a/arch/ia64/kernel/err_inject.c +++ b/arch/ia64/kernel/err_inject.c @@ -142,7 +142,7 @@ store_virtual_to_phys(struct device *dev, struct device_attribute *attr, u64 virt_addr=simple_strtoull(buf, NULL, 16); int ret; - ret = get_user_pages(virt_addr, 1, VM_READ, 0, NULL, NULL); + ret = get_user_pages(virt_addr, 1, FOLL_WRITE, NULL, NULL); if (ret<=0) { #ifdef ERR_INJ_DEBUG printk("Virtual address %lx is not existing.\n",virt_addr); diff --git a/arch/x86/mm/mpx.c b/arch/x86/mm/mpx.c index 8047687..e4f8009 100644 --- a/arch/x86/mm/mpx.c +++ b/arch/x86/mm/mpx.c @@ -544,10 +544,9 @@ static int mpx_resolve_fault(long __user *addr, int write) { long gup_ret; int nr_pages = 1; - int force = 0; - gup_ret = get_user_pages((unsigned long)addr, nr_pages, write, - force, NULL, NULL); + gup_ret = get_user_pages((unsigned long)addr, nr_pages, + write ? FOLL_WRITE : 0, NULL, NULL); /* * get_user_pages() returns number of pages gotten. * 0 means we failed to fault in and get anything, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c index 887483b..dcaf691 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c @@ -555,10 +555,13 @@ struct amdgpu_ttm_tt { int amdgpu_ttm_tt_get_user_pages(struct ttm_tt *ttm, struct page **pages) { struct amdgpu_ttm_tt *gtt = (void *)ttm; - int write = !(gtt->userflags & AMDGPU_GEM_USERPTR_READONLY); + unsigned int flags = 0; unsigned pinned = 0; int r; + if (!(gtt->userflags & AMDGPU_GEM_USERPTR_READONLY)) + flags |= FOLL_WRITE; + if (gtt->userflags & AMDGPU_GEM_USERPTR_ANONONLY) {
[PATCH 09/10] mm: replace access_remote_vm() write parameter with gup_flags
This patch removes the write parameter from access_remote_vm() and replaces it with a gup_flags parameter as use of this function previously _implied_ FOLL_FORCE, whereas after this patch callers explicitly pass this flag. We make this explicit as use of FOLL_FORCE can result in surprising behaviour (and hence bugs) within the mm subsystem. Signed-off-by: Lorenzo Stoakes --- fs/proc/base.c | 19 +-- include/linux/mm.h | 2 +- mm/memory.c| 11 +++ mm/nommu.c | 7 +++ 4 files changed, 20 insertions(+), 19 deletions(-) diff --git a/fs/proc/base.c b/fs/proc/base.c index c2964d8..8e65446 100644 --- a/fs/proc/base.c +++ b/fs/proc/base.c @@ -252,7 +252,7 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf, * Inherently racy -- command line shares address space * with code and data. */ - rv = access_remote_vm(mm, arg_end - 1, &c, 1, 0); + rv = access_remote_vm(mm, arg_end - 1, &c, 1, FOLL_FORCE); if (rv <= 0) goto out_free_page; @@ -270,7 +270,8 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf, int nr_read; _count = min3(count, len, PAGE_SIZE); - nr_read = access_remote_vm(mm, p, page, _count, 0); + nr_read = access_remote_vm(mm, p, page, _count, + FOLL_FORCE); if (nr_read < 0) rv = nr_read; if (nr_read <= 0) @@ -305,7 +306,8 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf, bool final; _count = min3(count, len, PAGE_SIZE); - nr_read = access_remote_vm(mm, p, page, _count, 0); + nr_read = access_remote_vm(mm, p, page, _count, + FOLL_FORCE); if (nr_read < 0) rv = nr_read; if (nr_read <= 0) @@ -354,7 +356,8 @@ static ssize_t proc_pid_cmdline_read(struct file *file, char __user *buf, bool final; _count = min3(count, len, PAGE_SIZE); - nr_read = access_remote_vm(mm, p, page, _count, 0); + nr_read = access_remote_vm(mm, p, page, _count, + FOLL_FORCE); if (nr_read < 0) rv = nr_read; if (nr_read <= 0) @@ -832,6 +835,7 @@ static ssize_t mem_rw(struct file *file, char __user *buf, unsigned long addr = *ppos; ssize_t copied; char *page; + unsigned int flags = FOLL_FORCE; if (!mm) return 0; @@ -844,6 +848,9 @@ static ssize_t mem_rw(struct file *file, char __user *buf, if (!atomic_inc_not_zero(&mm->mm_users)) goto free; + if (write) + flags |= FOLL_WRITE; + while (count > 0) { int this_len = min_t(int, count, PAGE_SIZE); @@ -852,7 +859,7 @@ static ssize_t mem_rw(struct file *file, char __user *buf, break; } - this_len = access_remote_vm(mm, addr, page, this_len, write); + this_len = access_remote_vm(mm, addr, page, this_len, flags); if (!this_len) { if (!copied) copied = -EIO; @@ -965,7 +972,7 @@ static ssize_t environ_read(struct file *file, char __user *buf, this_len = min(max_len, this_len); retval = access_remote_vm(mm, (env_start + src), - page, this_len, 0); + page, this_len, FOLL_FORCE); if (retval <= 0) { ret = retval; diff --git a/include/linux/mm.h b/include/linux/mm.h index 2a481d3..3e5234e 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -1268,7 +1268,7 @@ static inline int fixup_user_fault(struct task_struct *tsk, extern int access_process_vm(struct task_struct *tsk, unsigned long addr, void *buf, int len, int write); extern int access_remote_vm(struct mm_struct *mm, unsigned long addr, - void *buf, int len, int write); + void *buf, int len, unsigned int gup_flags); long __get_user_pages(struct task_struct *tsk, struct mm_struct *mm, unsigned long start, unsigned long nr_pages, diff --git a/mm/memory.c b/mm/memory.c index 79ebed3..bac2d99 100644 --- a/mm/memory.c +++ b/mm/memory.c @@ -3935,19 +3935,14 @@ static int __access_remote_vm(struct task_struct *tsk, struct mm_struct *mm, * @addr: start address to access * @buf: source or destination buffer * @len: number of bytes to transfer - * @write: whether the access is a writ
[PATCH 03/10] mm: replace get_user_pages_unlocked() write/force parameters with gup_flags
This patch removes the write and force parameters from get_user_pages_unlocked() and replaces them with a gup_flags parameter to make the use of FOLL_FORCE explicit in callers as use of this flag can result in surprising behaviour (and hence bugs) within the mm subsystem. Signed-off-by: Lorenzo Stoakes --- arch/mips/mm/gup.c | 2 +- arch/s390/mm/gup.c | 3 ++- arch/sh/mm/gup.c | 3 ++- arch/sparc/mm/gup.c| 3 ++- arch/x86/mm/gup.c | 2 +- drivers/media/pci/ivtv/ivtv-udma.c | 4 ++-- drivers/media/pci/ivtv/ivtv-yuv.c | 5 +++-- drivers/scsi/st.c | 5 ++--- drivers/video/fbdev/pvr2fb.c | 4 ++-- include/linux/mm.h | 2 +- mm/gup.c | 14 -- mm/nommu.c | 11 ++- mm/util.c | 3 ++- net/ceph/pagevec.c | 2 +- 14 files changed, 27 insertions(+), 36 deletions(-) diff --git a/arch/mips/mm/gup.c b/arch/mips/mm/gup.c index 42d124f..d8c3c15 100644 --- a/arch/mips/mm/gup.c +++ b/arch/mips/mm/gup.c @@ -287,7 +287,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, pages += nr; ret = get_user_pages_unlocked(start, (end - start) >> PAGE_SHIFT, - write, 0, pages); + pages, write ? FOLL_WRITE : 0); /* Have to be a bit careful with return values */ if (nr > 0) { diff --git a/arch/s390/mm/gup.c b/arch/s390/mm/gup.c index adb0c34..18d4107 100644 --- a/arch/s390/mm/gup.c +++ b/arch/s390/mm/gup.c @@ -266,7 +266,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, /* Try to get the remaining pages with get_user_pages */ start += nr << PAGE_SHIFT; pages += nr; - ret = get_user_pages_unlocked(start, nr_pages - nr, write, 0, pages); + ret = get_user_pages_unlocked(start, nr_pages - nr, pages, + write ? FOLL_WRITE : 0); /* Have to be a bit careful with return values */ if (nr > 0) ret = (ret < 0) ? nr : ret + nr; diff --git a/arch/sh/mm/gup.c b/arch/sh/mm/gup.c index 40fa6c8..063c298 100644 --- a/arch/sh/mm/gup.c +++ b/arch/sh/mm/gup.c @@ -258,7 +258,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, pages += nr; ret = get_user_pages_unlocked(start, - (end - start) >> PAGE_SHIFT, write, 0, pages); + (end - start) >> PAGE_SHIFT, pages, + write ? FOLL_WRITE : 0); /* Have to be a bit careful with return values */ if (nr > 0) { diff --git a/arch/sparc/mm/gup.c b/arch/sparc/mm/gup.c index 4e06750..cd0e32b 100644 --- a/arch/sparc/mm/gup.c +++ b/arch/sparc/mm/gup.c @@ -238,7 +238,8 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, pages += nr; ret = get_user_pages_unlocked(start, - (end - start) >> PAGE_SHIFT, write, 0, pages); + (end - start) >> PAGE_SHIFT, pages, + write ? FOLL_WRITE : 0); /* Have to be a bit careful with return values */ if (nr > 0) { diff --git a/arch/x86/mm/gup.c b/arch/x86/mm/gup.c index b8b6a60..0d4fb3e 100644 --- a/arch/x86/mm/gup.c +++ b/arch/x86/mm/gup.c @@ -435,7 +435,7 @@ int get_user_pages_fast(unsigned long start, int nr_pages, int write, ret = get_user_pages_unlocked(start, (end - start) >> PAGE_SHIFT, - write, 0, pages); + pages, write ? FOLL_WRITE : 0); /* Have to be a bit careful with return values */ if (nr > 0) { diff --git a/drivers/media/pci/ivtv/ivtv-udma.c b/drivers/media/pci/ivtv/ivtv-udma.c index 4769469..2c9232e 100644 --- a/drivers/media/pci/ivtv/ivtv-udma.c +++ b/drivers/media/pci/ivtv/ivtv-udma.c @@ -124,8 +124,8 @@ int ivtv_udma_setup(struct ivtv *itv, unsigned long ivtv_dest_addr, } /* Get user pages for DMA Xfer */ - err = get_user_pages_unlocked(user_dma.uaddr, user_dma.page_count, 0, - 1, dma->map); + err = get_user_pages_unlocked(user_dma.uaddr, user_dma.page_count, + dma->map, FOLL_FORCE); if (user_dma.page_count != err) { IVTV_DEBUG_WARN("failed to map user pages, returned %d instead of %d\n", diff --git a/drivers/media/pci/ivtv/ivtv-yuv.c b/drivers/media/pci/ivtv/ivtv-yuv.c index b094054..f7299d3 100644 --- a/drivers/media/pci/ivtv/ivtv-yuv.c +++ b/drivers/media/pci/ivtv/ivtv-yuv.c @@ -76,11 +76,12 @@ static int ivtv_yuv_prep_user_dma(struct ivtv *itv, struct ivtv_user_dma *dma, /* Get user
[PATCH 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags
This patch series adjusts functions in the get_user_pages* family such that desired FOLL_* flags are passed as an argument rather than implied by flags. The purpose of this change is to make the use of FOLL_FORCE explicit so it is easier to grep for and clearer to callers that this flag is being used. The use of FOLL_FORCE is an issue as it overrides missing VM_READ/VM_WRITE flags for the VMA whose pages we are reading from/writing to, which can result in surprising behaviour. The patch series came out of the discussion around commit 38e0885, which addressed a BUG_ON() being triggered when a page was faulted in with PROT_NONE set but having been overridden by FOLL_FORCE. do_numa_page() was run on the assumption the page _must_ be one marked for NUMA node migration as an actual PROT_NONE page would have been dealt with prior to this code path, however FOLL_FORCE introduced a situation where this assumption did not hold. See https://marc.info/?l=linux-mm&m=147585445805166 for the patch proposal. Lorenzo Stoakes (10): mm: remove write/force parameters from __get_user_pages_locked() mm: remove write/force parameters from __get_user_pages_unlocked() mm: replace get_user_pages_unlocked() write/force parameters with gup_flags mm: replace get_user_pages_locked() write/force parameters with gup_flags mm: replace get_vaddr_frames() write/force parameters with gup_flags mm: replace get_user_pages() write/force parameters with gup_flags mm: replace get_user_pages_remote() write/force parameters with gup_flags mm: replace __access_remote_vm() write parameter with gup_flags mm: replace access_remote_vm() write parameter with gup_flags mm: replace access_process_vm() write parameter with gup_flags arch/alpha/kernel/ptrace.c | 9 ++-- arch/blackfin/kernel/ptrace.c | 5 ++- arch/cris/arch-v32/drivers/cryptocop.c | 4 +- arch/cris/arch-v32/kernel/ptrace.c | 4 +- arch/ia64/kernel/err_inject.c | 2 +- arch/ia64/kernel/ptrace.c | 14 +++--- arch/m32r/kernel/ptrace.c | 15 --- arch/mips/kernel/ptrace32.c| 5 ++- arch/mips/mm/gup.c | 2 +- arch/powerpc/kernel/ptrace32.c | 5 ++- arch/s390/mm/gup.c | 3 +- arch/score/kernel/ptrace.c | 10 +++-- arch/sh/mm/gup.c | 3 +- arch/sparc/kernel/ptrace_64.c | 24 +++ arch/sparc/mm/gup.c| 3 +- arch/x86/kernel/step.c | 3 +- arch/x86/mm/gup.c | 2 +- arch/x86/mm/mpx.c | 5 +-- arch/x86/um/ptrace_32.c| 3 +- arch/x86/um/ptrace_64.c| 3 +- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 7 ++- drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 ++- drivers/gpu/drm/exynos/exynos_drm_g2d.c| 3 +- drivers/gpu/drm/i915/i915_gem_userptr.c| 6 ++- drivers/gpu/drm/radeon/radeon_ttm.c| 3 +- drivers/gpu/drm/via/via_dmablit.c | 4 +- drivers/infiniband/core/umem.c | 6 ++- drivers/infiniband/core/umem_odp.c | 7 ++- drivers/infiniband/hw/mthca/mthca_memfree.c| 2 +- drivers/infiniband/hw/qib/qib_user_pages.c | 3 +- drivers/infiniband/hw/usnic/usnic_uiom.c | 5 ++- drivers/media/pci/ivtv/ivtv-udma.c | 4 +- drivers/media/pci/ivtv/ivtv-yuv.c | 5 ++- drivers/media/platform/omap/omap_vout.c| 2 +- drivers/media/v4l2-core/videobuf-dma-sg.c | 7 ++- drivers/media/v4l2-core/videobuf2-memops.c | 6 ++- drivers/misc/mic/scif/scif_rma.c | 3 +- drivers/misc/sgi-gru/grufault.c| 2 +- drivers/platform/goldfish/goldfish_pipe.c | 3 +- drivers/rapidio/devices/rio_mport_cdev.c | 3 +- drivers/scsi/st.c | 5 +-- .../interface/vchiq_arm/vchiq_2835_arm.c | 3 +- .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +- drivers/video/fbdev/pvr2fb.c | 4 +- drivers/virt/fsl_hypervisor.c | 4 +- fs/exec.c | 9 +++- fs/proc/base.c | 19 +--- include/linux/mm.h | 18 kernel/events/uprobes.c| 6 ++- kernel/ptrace.c| 16 --- mm/frame_vector.c | 9 ++-- mm/gup.c | 50 ++ mm/memory.c| 16 --
[PATCH 10/10] mm: replace access_process_vm() write parameter with gup_flags
This patch removes the write parameter from access_process_vm() and replaces it with a gup_flags parameter as use of this function previously _implied_ FOLL_FORCE, whereas after this patch callers explicitly pass this flag. We make this explicit as use of FOLL_FORCE can result in surprising behaviour (and hence bugs) within the mm subsystem. Signed-off-by: Lorenzo Stoakes --- arch/alpha/kernel/ptrace.c | 9 ++--- arch/blackfin/kernel/ptrace.c | 5 +++-- arch/cris/arch-v32/kernel/ptrace.c | 4 ++-- arch/ia64/kernel/ptrace.c | 14 +- arch/m32r/kernel/ptrace.c | 15 ++- arch/mips/kernel/ptrace32.c| 5 +++-- arch/powerpc/kernel/ptrace32.c | 5 +++-- arch/score/kernel/ptrace.c | 10 ++ arch/sparc/kernel/ptrace_64.c | 24 arch/x86/kernel/step.c | 3 ++- arch/x86/um/ptrace_32.c| 3 ++- arch/x86/um/ptrace_64.c| 3 ++- include/linux/mm.h | 3 ++- kernel/ptrace.c| 16 ++-- mm/memory.c| 8 ++-- mm/nommu.c | 6 +++--- mm/util.c | 5 +++-- 17 files changed, 84 insertions(+), 54 deletions(-) diff --git a/arch/alpha/kernel/ptrace.c b/arch/alpha/kernel/ptrace.c index d9ee817..940dfb4 100644 --- a/arch/alpha/kernel/ptrace.c +++ b/arch/alpha/kernel/ptrace.c @@ -157,14 +157,16 @@ put_reg(struct task_struct *task, unsigned long regno, unsigned long data) static inline int read_int(struct task_struct *task, unsigned long addr, int * data) { - int copied = access_process_vm(task, addr, data, sizeof(int), 0); + int copied = access_process_vm(task, addr, data, sizeof(int), + FOLL_FORCE); return (copied == sizeof(int)) ? 0 : -EIO; } static inline int write_int(struct task_struct *task, unsigned long addr, int data) { - int copied = access_process_vm(task, addr, &data, sizeof(int), 1); + int copied = access_process_vm(task, addr, &data, sizeof(int), + FOLL_FORCE | FOLL_WRITE); return (copied == sizeof(int)) ? 0 : -EIO; } @@ -281,7 +283,8 @@ long arch_ptrace(struct task_struct *child, long request, /* When I and D space are separate, these will need to be fixed. */ case PTRACE_PEEKTEXT: /* read word at location addr. */ case PTRACE_PEEKDATA: - copied = access_process_vm(child, addr, &tmp, sizeof(tmp), 0); + copied = access_process_vm(child, addr, &tmp, sizeof(tmp), + FOLL_FORCE); ret = -EIO; if (copied != sizeof(tmp)) break; diff --git a/arch/blackfin/kernel/ptrace.c b/arch/blackfin/kernel/ptrace.c index 8b8fe67..8d79286 100644 --- a/arch/blackfin/kernel/ptrace.c +++ b/arch/blackfin/kernel/ptrace.c @@ -271,7 +271,7 @@ long arch_ptrace(struct task_struct *child, long request, case BFIN_MEM_ACCESS_CORE: case BFIN_MEM_ACCESS_CORE_ONLY: copied = access_process_vm(child, addr, &tmp, - to_copy, 0); + to_copy, FOLL_FORCE); if (copied) break; @@ -324,7 +324,8 @@ long arch_ptrace(struct task_struct *child, long request, case BFIN_MEM_ACCESS_CORE: case BFIN_MEM_ACCESS_CORE_ONLY: copied = access_process_vm(child, addr, &data, - to_copy, 1); + to_copy, + FOLL_FORCE | FOLL_WRITE); break; case BFIN_MEM_ACCESS_DMA: if (safe_dma_memcpy(paddr, &data, to_copy)) diff --git a/arch/cris/arch-v32/kernel/ptrace.c b/arch/cris/arch-v32/kernel/ptrace.c index f085229..f0df654 100644 --- a/arch/cris/arch-v32/kernel/ptrace.c +++ b/arch/cris/arch-v32/kernel/ptrace.c @@ -147,7 +147,7 @@ long arch_ptrace(struct task_struct *child, long request, /* The trampoline page is globally mapped, no page table to traverse.*/ tmp = *(unsigned long*)addr; } else { - copied = access_process_vm(child, addr, &tmp, sizeof(tmp), 0); + copied = access_process_vm(child, addr, &tmp, sizeof(tmp), FOLL_FORCE); if (copied != sizeof(tmp)) break; @@ -279,7 +279,7 @@ static int insn_size(struct task_struct *child, unsigned long pc) int opsize = 0;
[PATCH v2] drm/bridge: analogix: protect power when get_modes or detect
The drm callback ->detect and ->get_modes seems is not power safe, they may be called when device is power off, do register access on detect or get_modes will cause system die. Here is the path call ->detect before analogix_dp power on [] analogix_dp_detect+0x44/0xdc [] drm_helper_probe_single_connector_modes_merge_bits+0xe8/0x41c [] drm_helper_probe_single_connector_modes+0x10/0x18 [] drm_mode_getconnector+0xf4/0x304 [] drm_ioctl+0x23c/0x390 [] do_vfs_ioctl+0x4b8/0x58c [] SyS_ioctl+0x60/0x88 Cc: Inki Dae Cc: Sean Paul Cc: Gustavo Padovan Cc: "Ville Syrjälä" Signed-off-by: Mark Yao --- Changes in v2: - remove sub device power on/off callback, use pm_runtime_get/put is enough to fix my problem, so will can avoid race to dpms. drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c index efac8ab..ff2d328 100644 --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c @@ -1062,11 +1062,15 @@ int analogix_dp_get_modes(struct drm_connector *connector) return 0; } + pm_runtime_get_sync(dp->dev); + if (analogix_dp_handle_edid(dp) == 0) { drm_mode_connector_update_edid_property(&dp->connector, edid); num_modes += drm_add_edid_modes(&dp->connector, edid); } + pm_runtime_put(dp->dev); + if (dp->plat_data->panel) num_modes += drm_panel_get_modes(dp->plat_data->panel); @@ -1106,9 +1110,13 @@ analogix_dp_detect(struct drm_connector *connector, bool force) return connector_status_disconnected; } + pm_runtime_get_sync(dp->dev); + if (!analogix_dp_detect_hpd(dp)) status = connector_status_connected; + pm_runtime_put(dp->dev); + ret = analogix_dp_prepare_panel(dp, false, false); if (ret) DRM_ERROR("Failed to unprepare panel (%d)\n", ret); -- 1.9.1
Question: Re: [PATCH] drm/bridge: analogix: protect power when get_modes or detect
On 2016å¹´10æ12æ¥ 22:51, Sean Paul wrote: > On Wed, Oct 12, 2016 at 6:22 AM, Mark yao wrote: >> I'm not familiar with the analogix driver, maybe use a power reference count >> would better then direct power on/off analogix_dp. >> >> Does anyone has the idea to protect detect and get_modes context? >> > I'm not sure a reference count is going to help here. The common > pattern is to call detect() followed by get_modes() and then modeset. > However, it's not guaranteed that any one of those functions will be > called after the other. So, if you leave things on after detect or > get_modes, you might be wasting power (or worse). > > I recently ran into this exact problem with a panel we're using. Check > out "0b8b059a7: drm/bridge: analogix_dp: Ensure the panel is properly > prepared/unprepared". Perhaps you can piggyback on that function to > add your pm_runtime and plat_data callbacks (since using dpms_mode > might be racey). > > Sean Hi Sean Thanks for your advice. I re-test the detect and get_modes, only use pm_runtime_get/put also can fix my problem. without plat_data callbacks can avoid race to dpms_mode. I had send v2 patch, you can review it. Thanks. > >> I found many other connector driver also direct access register on detect or >> get_modes, no problem for it? >> >> On 2016å¹´10æ12æ¥ 18:00, Mark Yao wrote: >> >> The drm callback ->detect and ->get_modes seems is not power safe, >> they may be called when device is power off, do register access on >> detect or get_modes will cause system die. >> >> Here is the path call ->detect before analogix_dp power on >> [] analogix_dp_detect+0x44/0xdc >> [] >> drm_helper_probe_single_connector_modes_merge_bits+0xe8/0x41c >> [] drm_helper_probe_single_connector_modes+0x10/0x18 >> [] drm_mode_getconnector+0xf4/0x304 >> [] drm_ioctl+0x23c/0x390 >> [] do_vfs_ioctl+0x4b8/0x58c >> [] SyS_ioctl+0x60/0x88 >> >> Cc: Inki Dae >> Cc: Sean Paul >> Cc: Gustavo Padovan >> Cc: "Ville Syrjälä" >> >> Signed-off-by: Mark Yao >> --- >> drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 28 >> ++ >> 1 file changed, 28 insertions(+) >> >> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> index efac8ab..09dece2 100644 >> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c >> @@ -1062,6 +1062,13 @@ int analogix_dp_get_modes(struct drm_connector >> *connector) >>return 0; >>} >> >> + if (dp->dpms_mode != DRM_MODE_DPMS_ON) { >> + pm_runtime_get_sync(dp->dev); >> + >> + if (dp->plat_data->power_on) >> + dp->plat_data->power_on(dp->plat_data); >> + } >> + >>if (analogix_dp_handle_edid(dp) == 0) { >>drm_mode_connector_update_edid_property(&dp->connector, edid); >>num_modes += drm_add_edid_modes(&dp->connector, edid); >> @@ -1073,6 +1080,13 @@ int analogix_dp_get_modes(struct drm_connector >> *connector) >>if (dp->plat_data->get_modes) >>num_modes += dp->plat_data->get_modes(dp->plat_data, connector); >> >> + if (dp->dpms_mode != DRM_MODE_DPMS_ON) { >> + if (dp->plat_data->power_off) >> + dp->plat_data->power_off(dp->plat_data); >> + >> + pm_runtime_put_sync(dp->dev); >> + } >> + >>ret = analogix_dp_prepare_panel(dp, false, false); >>if (ret) >>DRM_ERROR("Failed to unprepare panel (%d)\n", ret); >> @@ -1106,9 +1120,23 @@ analogix_dp_detect(struct drm_connector *connector, >> bool force) >>return connector_status_disconnected; >>} >> >> + if (dp->dpms_mode != DRM_MODE_DPMS_ON) { >> + pm_runtime_get_sync(dp->dev); >> + >> + if (dp->plat_data->power_on) >> + dp->plat_data->power_on(dp->plat_data); >> + } >> + >>if (!analogix_dp_detect_hpd(dp)) >>status = connector_status_connected; >> >> + if (dp->dpms_mode != DRM_MODE_DPMS_ON) { >> + if (dp->plat_data->power_off) >> + dp->plat_data->power_off(dp->plat_data); >> + >> + pm_runtime_put_sync(dp->dev); >> + } >> + >>ret = analogix_dp_prepare_panel(dp, false, false); >>if (ret) >>DRM_ERROR("Failed to unprepare panel (%d)\n", ret); >> >> >> >> -- >> ï¼ark Yao > > -- ï¼ark Yao
[PATCH 02/10] mm: remove write/force parameters from __get_user_pages_unlocked()
On 13/10/2016 02:20, Lorenzo Stoakes wrote: > This patch removes the write and force parameters from > __get_user_pages_unlocked() to make the use of FOLL_FORCE explicit in callers > as > use of this flag can result in surprising behaviour (and hence bugs) within > the > mm subsystem. > > Signed-off-by: Lorenzo Stoakes > --- > include/linux/mm.h | 3 +-- > mm/gup.c | 17 + > mm/nommu.c | 12 +--- > mm/process_vm_access.c | 7 +-- > virt/kvm/async_pf.c| 3 ++- > virt/kvm/kvm_main.c| 11 --- > 6 files changed, 34 insertions(+), 19 deletions(-) > > diff --git a/include/linux/mm.h b/include/linux/mm.h > index e9caec6..2db98b6 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -1285,8 +1285,7 @@ long get_user_pages_locked(unsigned long start, > unsigned long nr_pages, > int write, int force, struct page **pages, int *locked); > long __get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm, > unsigned long start, unsigned long nr_pages, > -int write, int force, struct page **pages, > -unsigned int gup_flags); > +struct page **pages, unsigned int gup_flags); > long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, > int write, int force, struct page **pages); > int get_user_pages_fast(unsigned long start, int nr_pages, int write, > diff --git a/mm/gup.c b/mm/gup.c > index ba83942..3d620dd 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -865,17 +865,11 @@ EXPORT_SYMBOL(get_user_pages_locked); > */ > __always_inline long __get_user_pages_unlocked(struct task_struct *tsk, > struct mm_struct *mm, > unsigned long start, unsigned > long nr_pages, > -int write, int force, struct > page **pages, > -unsigned int gup_flags) > +struct page **pages, unsigned > int gup_flags) > { > long ret; > int locked = 1; > > - if (write) > - gup_flags |= FOLL_WRITE; > - if (force) > - gup_flags |= FOLL_FORCE; > - > down_read(&mm->mmap_sem); > ret = __get_user_pages_locked(tsk, mm, start, nr_pages, pages, NULL, > &locked, false, gup_flags); > @@ -905,8 +899,15 @@ EXPORT_SYMBOL(__get_user_pages_unlocked); > long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, >int write, int force, struct page **pages) > { > + unsigned int flags = FOLL_TOUCH; > + > + if (write) > + flags |= FOLL_WRITE; > + if (force) > + flags |= FOLL_FORCE; > + > return __get_user_pages_unlocked(current, current->mm, start, nr_pages, > - write, force, pages, FOLL_TOUCH); > + pages, flags); > } > EXPORT_SYMBOL(get_user_pages_unlocked); > > diff --git a/mm/nommu.c b/mm/nommu.c > index 95daf81..925dcc1 100644 > --- a/mm/nommu.c > +++ b/mm/nommu.c > @@ -185,8 +185,7 @@ EXPORT_SYMBOL(get_user_pages_locked); > > long __get_user_pages_unlocked(struct task_struct *tsk, struct mm_struct *mm, > unsigned long start, unsigned long nr_pages, > -int write, int force, struct page **pages, > -unsigned int gup_flags) > +struct page **pages, unsigned int gup_flags) > { > long ret; > down_read(&mm->mmap_sem); > @@ -200,8 +199,15 @@ EXPORT_SYMBOL(__get_user_pages_unlocked); > long get_user_pages_unlocked(unsigned long start, unsigned long nr_pages, >int write, int force, struct page **pages) > { > + unsigned int flags = 0; > + > + if (write) > + flags |= FOLL_WRITE; > + if (force) > + flags |= FOLL_FORCE; > + > return __get_user_pages_unlocked(current, current->mm, start, nr_pages, > - write, force, pages, 0); > + pages, flags); > } > EXPORT_SYMBOL(get_user_pages_unlocked); > > diff --git a/mm/process_vm_access.c b/mm/process_vm_access.c > index 07514d4..be8dc8d 100644 > --- a/mm/process_vm_access.c > +++ b/mm/process_vm_access.c > @@ -88,12 +88,16 @@ static int process_vm_rw_single_vec(unsigned long addr, > ssize_t rc = 0; > unsigned long max_pages_per_loop = PVM_MAX_KMALLOC_PAGES > / sizeof(struct pages *); > + unsigned int flags = FOLL_REMOTE; > > /* Work out address and page range required */ > if (len == 0) > return 0; > nr_pages = (addr + len - 1) / PAGE_SIZE - addr / PAGE_SIZE + 1; > > + if (vm_write) > + flag
[bug report] drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7.
Please Review. Best Regards Rex -Original Message- From: Dan Carpenter [mailto:dan.carpen...@oracle.com] Sent: Wednesday, October 12, 2016 2:12 PM To: Zhu, Rex Cc: dri-devel at lists.freedesktop.org Subject: [bug report] drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7. Hello Rex Zhu, This is a semi-automatic email about new static checker warnings. The patch 599a7e9fe1b6: "drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7." from Sep 9, 2016, leads to the following Smatch complaint: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c:1463 smu7_get_evv_voltages() error: we previously assumed 'table_info' could be null (see line 1455) drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c 1454 1455 if (table_info != NULL) ^ Check for non-NULL. 1456 sclk_table = table_info->vdd_dep_on_sclk; 1457 1458 for (i = 0; i < SMU7_MAX_LEAKAGE_COUNT; i++) { 1459 vv_id = ATOM_VIRTUAL_VOLTAGE_ID0 + i; 1460 1461 if (data->vdd_gfx_control == SMU7_VOLTAGE_CONTROL_BY_SVID2) { 1462 if (0 == phm_get_sclk_for_voltage_evv(hwmgr, 1463 table_info->vddgfx_lookup_table, vv_id, &sclk)) { ^^^ Unchecked dereference. 1464 if (phm_cap_enabled(hwmgr->platform_descriptor.platformCaps, 1465 PHM_PlatformCaps_ClockStretcher)) { regards, dan carpenter -- next part -- A non-text attachment was scrubbed... Name: 0001-drm-amd-powerplay-fix-static-checker-warnings-in-smu.patch Type: application/octet-stream Size: 1105 bytes Desc: 0001-drm-amd-powerplay-fix-static-checker-warnings-in-smu.patch URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/7bf264a5/attachment-0001.obj>
[PATCH] drm/amd/powerplay: don't give up if DPM is already running
Hi all, The attached patches were also for this issue. Disable dpm when rmmod amdgpu. Please help to review. Best Regards Rex -Original Message- From: amd-gfx [mailto:amd-gfx-boun...@lists.freedesktop.org] On Behalf Of Zhu, Rex Sent: Wednesday, October 12, 2016 9:45 PM To: Alex Deucher; Grazvydas Ignotas Cc: amd-gfx list; Maling list - DRI developers Subject: RE: [PATCH] drm/amd/powerplay: don't give up if DPM is already running Hi Grazvydas and Alex, We needed to disable dpm when rmmod amdgpu for this issue. I am checking the function of disable dpm task. Best Regards Rex -Original Message- From: Alex Deucher [mailto:alexdeuc...@gmail.com] Sent: Wednesday, October 12, 2016 4:01 AM To: Grazvydas Ignotas; Zhu, Rex Cc: Maling list - DRI developers; amd-gfx list Subject: Re: [PATCH] drm/amd/powerplay: don't give up if DPM is already running +Rex to review this. Alex On Sun, Oct 9, 2016 at 3:23 PM, Grazvydas Ignotas wrote: > Currently the driver crashes if smu7_enable_dpm_tasks() returns early, > which it does if DPM is already active. It seems to be better just to > continue anyway, at least I haven't noticed any ill effects. It's also > unclear at what state the hardware was left by the previous driver, so > IMO it's better to always fully initialize. > > Way to reproduce: > $ modprobe amdgpu > $ rmmod amdgpu > $ modprobe amdgpu > ... > DPM is already running right now, no need to enable DPM! > ... > failed to send message 18b ret is 0 > BUG: unable to handle kernel paging request at ed01fc9ab21f Call > Trace: > smu7_set_power_state_tasks+0x499/0x1940 [amdgpu] > phm_set_power_state+0xcb/0x120 [amdgpu] > psm_adjust_power_state_dynamic+0x11e/0x1b0 [amdgpu] > pem_task_adjust_power_state+0xb9/0xd0 [amdgpu] > pem_excute_event_chain+0x7d/0xe0 [amdgpu] > pem_handle_event_unlocked+0x49/0x60 [amdgpu] > pem_handle_event+0xe/0x10 [amdgpu] > pp_dpm_dispatch_tasks+0xe0/0x190 [amdgpu] > amdgpu_pm_compute_clocks+0x10c/0xc60 [amdgpu] > dce_v11_0_crtc_dpms+0x7d/0x150 [amdgpu] > dce_v11_0_crtc_disable+0x90/0x4a0 [amdgpu] > drm_helper_disable_unused_functions+0x67/0x80 [drm_kms_helper] > amdgpu_fbdev_init+0x13e/0x170 [amdgpu] > amdgpu_device_init+0x1aeb/0x2490 [amdgpu] > > Signed-off-by: Grazvydas Ignotas > --- > drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c > b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c > index f6afa6a..327030b 100644 > --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c > +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c > @@ -1166,8 +1166,8 @@ static int smu7_enable_dpm_tasks(struct pp_hwmgr > *hwmgr) > > tmp_result = (!smum_is_dpm_running(hwmgr)) ? 0 : -1; > PP_ASSERT_WITH_CODE(tmp_result == 0, > - "DPM is already running right now, no need to enable > DPM!", > - return 0); > + "DPM is already running", > + ); > > if (smu7_voltage_control(hwmgr)) { > tmp_result = smu7_enable_voltage_control(hwmgr); > -- > 2.7.4 > > ___ > amd-gfx mailing list > amd-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx at lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx -- next part -- A non-text attachment was scrubbed... Name: 0001-drm-amdgpu-need-to-disable-dpm-first-when-rmmod-amdg.patch Type: application/octet-stream Size: 1469 bytes Desc: 0001-drm-amdgpu-need-to-disable-dpm-first-when-rmmod-amdg.patch URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/c2213003/attachment.obj> -- next part -- A non-text attachment was scrubbed... Name: 0002-drm-amd-powerplay-fix-bug-stop-dpm-can-t-work-on-Vi.patch Type: application/octet-stream Size: 3840 bytes Desc: 0002-drm-amd-powerplay-fix-bug-stop-dpm-can-t-work-on-Vi.patch URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/c2213003/attachment-0001.obj>
[bug report] drm/amd/powerplay: implement smu7 hwmgr to manager asics with smu ip version 7.
Patch is Reviewed-by: Christian König . Regards, Christian. Am 13.10.2016 um 09:43 schrieb Zhu, Rex: > Please Review. > > Best Regards > Rex > > -Original Message- > From: Dan Carpenter [mailto:dan.carpenter at oracle.com] > Sent: Wednesday, October 12, 2016 2:12 PM > To: Zhu, Rex > Cc: dri-devel at lists.freedesktop.org > Subject: [bug report] drm/amd/powerplay: implement smu7 hwmgr to manager > asics with smu ip version 7. > > Hello Rex Zhu, > > This is a semi-automatic email about new static checker warnings. > > The patch 599a7e9fe1b6: "drm/amd/powerplay: implement smu7 hwmgr to manager > asics with smu ip version 7." from Sep 9, 2016, leads to the following Smatch > complaint: > > drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c:1463 > smu7_get_evv_voltages() >error: we previously assumed 'table_info' could be null (see line 1455) > > drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/smu7_hwmgr.c >1454 >1455 if (table_info != NULL) > ^ > Check for non-NULL. > >1456 sclk_table = table_info->vdd_dep_on_sclk; >1457 >1458 for (i = 0; i < SMU7_MAX_LEAKAGE_COUNT; i++) { >1459 vv_id = ATOM_VIRTUAL_VOLTAGE_ID0 + i; >1460 >1461 if (data->vdd_gfx_control == > SMU7_VOLTAGE_CONTROL_BY_SVID2) { >1462 if (0 == > phm_get_sclk_for_voltage_evv(hwmgr, >1463 > table_info->vddgfx_lookup_table, vv_id, &sclk)) { > > ^^^ Unchecked dereference. > >1464 if > (phm_cap_enabled(hwmgr->platform_descriptor.platformCaps, >1465 > PHM_PlatformCaps_ClockStretcher)) { > > regards, > dan carpenter > > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/01752e1e/attachment.html>
[PATCH 00/10] mm: adjust get_user_pages* functions to explicitly pass FOLL_* flags
Am 13.10.2016 um 02:20 schrieb Lorenzo Stoakes: > This patch series adjusts functions in the get_user_pages* family such that > desired FOLL_* flags are passed as an argument rather than implied by flags. > > The purpose of this change is to make the use of FOLL_FORCE explicit so it is > easier to grep for and clearer to callers that this flag is being used. The > use > of FOLL_FORCE is an issue as it overrides missing VM_READ/VM_WRITE flags for > the > VMA whose pages we are reading from/writing to, which can result in surprising > behaviour. > > The patch series came out of the discussion around commit 38e0885, which > addressed a BUG_ON() being triggered when a page was faulted in with PROT_NONE > set but having been overridden by FOLL_FORCE. do_numa_page() was run on the > assumption the page _must_ be one marked for NUMA node migration as an actual > PROT_NONE page would have been dealt with prior to this code path, however > FOLL_FORCE introduced a situation where this assumption did not hold. > > See https://marc.info/?l=linux-mm&m=147585445805166 for the patch proposal. > > Lorenzo Stoakes (10): >mm: remove write/force parameters from __get_user_pages_locked() >mm: remove write/force parameters from __get_user_pages_unlocked() >mm: replace get_user_pages_unlocked() write/force parameters with gup_flags >mm: replace get_user_pages_locked() write/force parameters with gup_flags >mm: replace get_vaddr_frames() write/force parameters with gup_flags >mm: replace get_user_pages() write/force parameters with gup_flags >mm: replace get_user_pages_remote() write/force parameters with gup_flags >mm: replace __access_remote_vm() write parameter with gup_flags >mm: replace access_remote_vm() write parameter with gup_flags >mm: replace access_process_vm() write parameter with gup_flags Patch number 6 in this series (which touches drivers I co-maintain) is Acked-by: Christian König . In general looks like a very nice cleanup to me, but I'm not enlightened enough to full judge. Regards, Christian. > > arch/alpha/kernel/ptrace.c | 9 ++-- > arch/blackfin/kernel/ptrace.c | 5 ++- > arch/cris/arch-v32/drivers/cryptocop.c | 4 +- > arch/cris/arch-v32/kernel/ptrace.c | 4 +- > arch/ia64/kernel/err_inject.c | 2 +- > arch/ia64/kernel/ptrace.c | 14 +++--- > arch/m32r/kernel/ptrace.c | 15 --- > arch/mips/kernel/ptrace32.c| 5 ++- > arch/mips/mm/gup.c | 2 +- > arch/powerpc/kernel/ptrace32.c | 5 ++- > arch/s390/mm/gup.c | 3 +- > arch/score/kernel/ptrace.c | 10 +++-- > arch/sh/mm/gup.c | 3 +- > arch/sparc/kernel/ptrace_64.c | 24 +++ > arch/sparc/mm/gup.c| 3 +- > arch/x86/kernel/step.c | 3 +- > arch/x86/mm/gup.c | 2 +- > arch/x86/mm/mpx.c | 5 +-- > arch/x86/um/ptrace_32.c| 3 +- > arch/x86/um/ptrace_64.c| 3 +- > drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 7 ++- > drivers/gpu/drm/etnaviv/etnaviv_gem.c | 7 ++- > drivers/gpu/drm/exynos/exynos_drm_g2d.c| 3 +- > drivers/gpu/drm/i915/i915_gem_userptr.c| 6 ++- > drivers/gpu/drm/radeon/radeon_ttm.c| 3 +- > drivers/gpu/drm/via/via_dmablit.c | 4 +- > drivers/infiniband/core/umem.c | 6 ++- > drivers/infiniband/core/umem_odp.c | 7 ++- > drivers/infiniband/hw/mthca/mthca_memfree.c| 2 +- > drivers/infiniband/hw/qib/qib_user_pages.c | 3 +- > drivers/infiniband/hw/usnic/usnic_uiom.c | 5 ++- > drivers/media/pci/ivtv/ivtv-udma.c | 4 +- > drivers/media/pci/ivtv/ivtv-yuv.c | 5 ++- > drivers/media/platform/omap/omap_vout.c| 2 +- > drivers/media/v4l2-core/videobuf-dma-sg.c | 7 ++- > drivers/media/v4l2-core/videobuf2-memops.c | 6 ++- > drivers/misc/mic/scif/scif_rma.c | 3 +- > drivers/misc/sgi-gru/grufault.c| 2 +- > drivers/platform/goldfish/goldfish_pipe.c | 3 +- > drivers/rapidio/devices/rio_mport_cdev.c | 3 +- > drivers/scsi/st.c | 5 +-- > .../interface/vchiq_arm/vchiq_2835_arm.c | 3 +- > .../vc04_services/interface/vchiq_arm/vchiq_arm.c | 3 +- > drivers/video/fbdev/pvr2fb.c | 4 +- > drivers/virt/fsl_hypervisor.c | 4 +- > fs/exec.c
[PATCH libdrm v2 2/2] Add drmModePageFlipTarget
From: Michel Dänzer It supports the DRM_MODE_PAGE_FLIP_TARGET_* flags. Signed-off-by: Michel Dänzer --- See https://cgit.freedesktop.org/~daenzer/xf86-video-amdgpu/commit/?id=b8631a9ba49c0d0ebe5dcd1dbfb68fcfe907296f https://cgit.freedesktop.org/~daenzer/xf86-video-ati/commit/?id=fc884a8af25345c32bd4104c864ecfeb9bb3db9b for examples how this can be used. xf86drmMode.c | 16 xf86drmMode.h | 3 +++ 2 files changed, 19 insertions(+) diff --git a/xf86drmMode.c b/xf86drmMode.c index 228c6e4..fb22f68 100644 --- a/xf86drmMode.c +++ b/xf86drmMode.c @@ -948,6 +948,22 @@ int drmModePageFlip(int fd, uint32_t crtc_id, uint32_t fb_id, return DRM_IOCTL(fd, DRM_IOCTL_MODE_PAGE_FLIP, &flip); } +int drmModePageFlipTarget(int fd, uint32_t crtc_id, uint32_t fb_id, + uint32_t flags, void *user_data, + uint32_t target_vblank) +{ + struct drm_mode_crtc_page_flip_target flip_target; + + memclear(flip_target); + flip_target.fb_id = fb_id; + flip_target.crtc_id = crtc_id; + flip_target.user_data = VOID2U64(user_data); + flip_target.flags = flags; + flip_target.sequence = target_vblank; + + return DRM_IOCTL(fd, DRM_IOCTL_MODE_PAGE_FLIP, &flip_target); +} + int drmModeSetPlane(int fd, uint32_t plane_id, uint32_t crtc_id, uint32_t fb_id, uint32_t flags, int32_t crtc_x, int32_t crtc_y, diff --git a/xf86drmMode.h b/xf86drmMode.h index 1a02fed..b684967 100644 --- a/xf86drmMode.h +++ b/xf86drmMode.h @@ -473,6 +473,9 @@ extern int drmModeCrtcGetGamma(int fd, uint32_t crtc_id, uint32_t size, uint16_t *red, uint16_t *green, uint16_t *blue); extern int drmModePageFlip(int fd, uint32_t crtc_id, uint32_t fb_id, uint32_t flags, void *user_data); +extern int drmModePageFlipTarget(int fd, uint32_t crtc_id, uint32_t fb_id, +uint32_t flags, void *user_data, +uint32_t target_vblank); extern drmModePlaneResPtr drmModeGetPlaneResources(int fd); extern drmModePlanePtr drmModeGetPlane(int fd, uint32_t plane_id); -- 2.9.3
[PATCH libdrm v2 1/2] headers: Sync drm{,_mode}.h with the kernel
From: Michel Dänzer Generated using make headers_install, based on linus master commit b67be92feb486f800d80d72c67fd87b47b79b18e. Signed-off-by: Michel Dänzer --- include/drm/drm.h | 17 + include/drm/drm_mode.h | 49 ++--- 2 files changed, 63 insertions(+), 3 deletions(-) diff --git a/include/drm/drm.h b/include/drm/drm.h index b4ebaa9..f6fd5c2 100644 --- a/include/drm/drm.h +++ b/include/drm/drm.h @@ -59,6 +59,10 @@ typedef unsigned long drm_handle_t; #endif +#if defined(__cplusplus) +extern "C" { +#endif + #define DRM_NAME "drm" /**< Name in kernel, /dev, and /proc */ #define DRM_MIN_ORDER 5 /**< At least 2^5 bytes = 32 bytes */ #define DRM_MAX_ORDER 22/**< Up to 2^22 bytes = 4MB */ @@ -636,6 +640,7 @@ struct drm_gem_open { #define DRM_CAP_CURSOR_WIDTH 0x8 #define DRM_CAP_CURSOR_HEIGHT 0x9 #define DRM_CAP_ADDFB2_MODIFIERS 0x10 +#define DRM_CAP_PAGE_FLIP_TARGET 0x11 /** DRM_IOCTL_GET_CAP ioctl argument type */ struct drm_get_cap { @@ -685,8 +690,16 @@ struct drm_prime_handle { __s32 fd; }; +#if defined(__cplusplus) +} +#endif + #include "drm_mode.h" +#if defined(__cplusplus) +extern "C" { +#endif + #define DRM_IOCTL_BASE 'd' #define DRM_IO(nr) _IO(DRM_IOCTL_BASE,nr) #define DRM_IOR(nr,type) _IOR(DRM_IOCTL_BASE,nr,type) @@ -878,4 +891,8 @@ typedef struct drm_agp_info drm_agp_info_t; typedef struct drm_scatter_gather drm_scatter_gather_t; typedef struct drm_set_version drm_set_version_t; +#if defined(__cplusplus) +} +#endif + #endif diff --git a/include/drm/drm_mode.h b/include/drm/drm_mode.h index 7a7856e..df0e350 100644 --- a/include/drm/drm_mode.h +++ b/include/drm/drm_mode.h @@ -29,6 +29,10 @@ #include "drm.h" +#if defined(__cplusplus) +extern "C" { +#endif + #define DRM_DISPLAY_INFO_LEN 32 #define DRM_CONNECTOR_NAME_LEN 32 #define DRM_DISPLAY_MODE_LEN 32 @@ -202,6 +206,7 @@ struct drm_mode_get_plane_res { #define DRM_MODE_ENCODER_VIRTUAL 5 #define DRM_MODE_ENCODER_DSI 6 #define DRM_MODE_ENCODER_DPMST 7 +#define DRM_MODE_ENCODER_DPI 8 struct drm_mode_get_encoder { __u32 encoder_id; @@ -241,6 +246,7 @@ struct drm_mode_get_encoder { #define DRM_MODE_CONNECTOR_eDP 14 #define DRM_MODE_CONNECTOR_VIRTUAL 15 #define DRM_MODE_CONNECTOR_DSI 16 +#define DRM_MODE_CONNECTOR_DPI 17 struct drm_mode_get_connector { @@ -514,7 +520,13 @@ struct drm_color_lut { #define DRM_MODE_PAGE_FLIP_EVENT 0x01 #define DRM_MODE_PAGE_FLIP_ASYNC 0x02 -#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT|DRM_MODE_PAGE_FLIP_ASYNC) +#define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4 +#define DRM_MODE_PAGE_FLIP_TARGET_RELATIVE 0x8 +#define DRM_MODE_PAGE_FLIP_TARGET (DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE | \ + DRM_MODE_PAGE_FLIP_TARGET_RELATIVE) +#define DRM_MODE_PAGE_FLIP_FLAGS (DRM_MODE_PAGE_FLIP_EVENT | \ + DRM_MODE_PAGE_FLIP_ASYNC | \ + DRM_MODE_PAGE_FLIP_TARGET) /* * Request a page flip on the specified crtc. @@ -537,8 +549,7 @@ struct drm_color_lut { * 'as soon as possible', meaning that it not delay waiting for vblank. * This may cause tearing on the screen. * - * The reserved field must be zero until we figure out something - * clever to use it for. + * The reserved field must be zero. */ struct drm_mode_crtc_page_flip { @@ -549,6 +560,34 @@ struct drm_mode_crtc_page_flip { __u64 user_data; }; +/* + * Request a page flip on the specified crtc. + * + * Same as struct drm_mode_crtc_page_flip, but supports new flags and + * re-purposes the reserved field: + * + * The sequence field must be zero unless either of the + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is specified. When + * the ABSOLUTE flag is specified, the sequence field denotes the absolute + * vblank sequence when the flip should take effect. When the RELATIVE + * flag is specified, the sequence field denotes the relative (to the + * current one when the ioctl is called) vblank sequence when the flip + * should take effect. NOTE: DRM_IOCTL_WAIT_VBLANK must still be used to + * make sure the vblank sequence before the target one has passed before + * calling this ioctl. The purpose of the + * DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE/RELATIVE flags is merely to clarify + * the target for when code dealing with a page flip runs during a + * vertical blank period. + */ + +struct drm_mode_crtc_page_flip_target { + __u32 crtc_id; + __u32 fb_id; + __u32 flags; + __u32 sequence; + __u64 user_data; +}; + /* create a dumb scanout buffer */ struct drm_mode_create_dumb { __u32 height; @@ -621,4 +660,8 @@ struct drm_mode_destroy_blob { __u32 blob_id; }; +#if defined(__cplusplus) +} +#endif + #endif -- 2.9.3
[PATCH libdrm] Add support for DRM_MODE_PAGE_FLIP_TARGET_* flags
On 13/10/16 12:54 AM, Emil Velikov wrote: > Hi Michel, > > On 12 October 2016 at 10:41, Michel Dänzer wrote: >> From: Michel Dänzer >> >> Signed-off-by: Michel Dänzer >> --- >> >> The corresponding kernel changes have landed in Linus' tree. >> >> include/drm/drm.h | 1 + >> include/drm/drm_mode.h | 39 --- > For these two (patch 1/2) please follow the pre-existing pattern (git > log -- include/drm/), barring the top commit of course ;-) Namely: use > make headers_install & reference the tree/sha that files are based on. > > If you can skim through & handle radeon/amdgpu_drm.h that'll be > greatly appreciated. I'm leaving that to somebody who needs those to be synced. E.g. Marek might already have patches ready for amdgpu SI support. Other than that, the v2 series should address your comments, thanks. -- Earthling Michel Dänzer | http://www.amd.com Libre software enthusiast | Mesa and X developer
[PULL] topic/drm-misc
Hi Dave, Sumits undug himself from conference traveling, so here's one more pull request for 4.9, essentially just containing the reservation rcu fixes and polish from Chris. Btw plan is that between -rc1 and -rc2 we do a tree-wide s/fence/dma_fence/ since the current name is a bit too generic for what it does. And dma_fence also aligns much better with dma_buf. That rename is probably best if it lands through your drm-fixes tree (which we can do, now that sync_file is destaged since drm is defactor owning all that inter-device dma stuff anyway). Cheers, Daniel The following changes since commit a5bd451b6e6ece69be07a425381c4f3438eadba0: drm/crtc: constify drm_crtc_index parameter (2016-10-10 17:28:58 +0200) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/topic/drm-misc-2016-10-13 for you to fetch changes up to f7741aa75e76440f4e9ecfe512feebe9bce33ca8: drm/savage: dereferencing an error pointer (2016-10-13 07:56:14 +0200) Chris Wilson (8): drm/amdgpu: Remove call to reservation_object_test_signaled_rcu before wait drm/etnaviv: Remove manual call to reservation_object_test_signaled_rcu before wait drm/nouveau: Remove call to reservation_object_test_signaled_rcu before wait drm/vmwgfx: Remove call to reservation_object_test_signaled_rcu before wait dma-buf: Introduce fence_get_rcu_safe() dma-buf: Restart reservation_object_get_fences_rcu() after writes dma-buf: Restart reservation_object_wait_timeout_rcu() after writes dma-buf: Restart reservation_object_test_signaled_rcu() after writes Dan Carpenter (1): drm/savage: dereferencing an error pointer Jiang Biao (2): drm/gma500: remove useless comment drm/gma500: add comments for new parameters Shyam Saini (1): gpu: drm: gma500: Use vma_pages() drivers/dma-buf/reservation.c| 112 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 6 +- drivers/gpu/drm/etnaviv/etnaviv_gem.c| 24 +++ drivers/gpu/drm/gma500/framebuffer.c | 3 +- drivers/gpu/drm/gma500/gtt.c | 2 + drivers/gpu/drm/nouveau/nouveau_gem.c| 21 +++--- drivers/gpu/drm/savage/savage_state.c| 1 + drivers/gpu/drm/vmwgfx/vmwgfx_resource.c | 6 +- include/linux/fence.h| 56 ++-- 9 files changed, 124 insertions(+), 107 deletions(-) -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH] drm/crtc: constify drm_crtc_mask parameter
Now that drm_crtc_index takes a const, the same can be done for drm_crtc_mask. Signed-off-by: Maarten Lankhorst --- diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 0aa292526567..98ec40713921 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -1354,7 +1354,7 @@ static inline unsigned int drm_crtc_index(const struct drm_crtc *crtc) * Given a registered CRTC, return the mask bit of that CRTC for an * encoder's possible_crtcs field. */ -static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc) +static inline uint32_t drm_crtc_mask(const struct drm_crtc *crtc) { return 1 << drm_crtc_index(crtc); } diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h index 387e33a4d6ee..c7438ff0d609 100644 --- a/include/drm/drm_encoder.h +++ b/include/drm/drm_encoder.h @@ -189,7 +189,7 @@ static inline unsigned int drm_encoder_index(struct drm_encoder *encoder) } /* FIXME: We have an include file mess still, drm_crtc.h needs untangling. */ -static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc); +static inline uint32_t drm_crtc_mask(const struct drm_crtc *crtc); /** * drm_encoder_crtc_ok - can a given crtc drive a given encoder?
[patch] drm/imx: drm_dev_alloc() returns error pointers
We are checking for NULL here, when we should be checking for error pointers. Fixes: 54db5decce17 ("drm/imx: drop deprecated load/unload drm_driver ops") Signed-off-by: Dan Carpenter diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c index 98df09c..3acc976 100644 --- a/drivers/gpu/drm/imx/imx-drm-core.c +++ b/drivers/gpu/drm/imx/imx-drm-core.c @@ -357,8 +357,8 @@ static int imx_drm_bind(struct device *dev) int ret; drm = drm_dev_alloc(&imx_drm_driver, dev); - if (!drm) - return -ENOMEM; + if (IS_ERR(drm)) + return PTR_ERR(drm); imxdrm = devm_kzalloc(dev, sizeof(*imxdrm), GFP_KERNEL); if (!imxdrm) {
[patch] drm/vc4: Fix a couple error codes in vc4_cl_lookup_bos()
If the allocation fails the current code returns success. If copy_from_user() fails it returns the number of bytes remaining instead of -EFAULT. Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.") Signed-off-by: Dan Carpenter diff --git a/drivers/gpu/drm/vc4/vc4_gem.c b/drivers/gpu/drm/vc4/vc4_gem.c index 47a095f..303f23c 100644 --- a/drivers/gpu/drm/vc4/vc4_gem.c +++ b/drivers/gpu/drm/vc4/vc4_gem.c @@ -544,14 +544,15 @@ vc4_cl_lookup_bos(struct drm_device *dev, handles = drm_malloc_ab(exec->bo_count, sizeof(uint32_t)); if (!handles) { + ret = -ENOMEM; DRM_ERROR("Failed to allocate incoming GEM handles\n"); goto fail; } - ret = copy_from_user(handles, -(void __user *)(uintptr_t)args->bo_handles, -exec->bo_count * sizeof(uint32_t)); - if (ret) { + if (copy_from_user(handles, + (void __user *)(uintptr_t)args->bo_handles, + exec->bo_count * sizeof(uint32_t))) { + ret = -EFAULT; DRM_ERROR("Failed to copy in GEM handles\n"); goto fail; }
[patch] drm/i915: fix a read size argument
We want to read 3 bytes here, but because the parenthesis are in the wrong place we instead read: sizeof(intel_dp->edp_dpcd) == sizeof(intel_dp->edp_dpcd) which is one byte. Fixes: fe5a66f91c88 ("drm/i915: Read PSR caps/intermediate freqs/etc. only once on eDP") Signed-off-by: Dan Carpenter diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 14a3cf0..ee8aa95 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3551,8 +3551,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) /* Read the eDP Display control capabilities registers */ if ((intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE) && drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, -intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd) == -sizeof(intel_dp->edp_dpcd))) +intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd)) == +sizeof(intel_dp->edp_dpcd)) DRM_DEBUG_KMS("EDP DPCD : %*ph\n", (int) sizeof(intel_dp->edp_dpcd), intel_dp->edp_dpcd);
[PATCH v3 9/9] MAINTAINERS: Update HISILICON DRM entries
Signed-off-by: Rongrong Zou --- MAINTAINERS | 1 + 1 file changed, 1 insertion(+) diff --git a/MAINTAINERS b/MAINTAINERS index 4347bce..16f4569 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -4117,6 +4117,7 @@ F:drivers/gpu/drm/gma500/ DRM DRIVERS FOR HISILICON M: Xinliang Liu +M: Rongrong Zou R: Xinwei Kong R: Chen Feng L: dri-devel at lists.freedesktop.org -- 1.9.1
[PATCH v3 8/9] drm/hisilicon/hibmc: Add vblank interruput
Add vblank interrupt. Signed-off-by: Rongrong Zou --- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 47 - 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c index 7c88295..8c0d492 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c @@ -45,15 +45,45 @@ static int hibmc_enable_vblank(struct drm_device *dev, unsigned int pipe) { + struct hibmc_drm_device *hidev = + (struct hibmc_drm_device *)dev->dev_private; + + writel(HIBMC_RAW_INTERRUPT_EN_VBLANK(1), + hidev->mmio + HIBMC_RAW_INTERRUPT_EN); + return 0; } static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe) { + struct hibmc_drm_device *hidev = + (struct hibmc_drm_device *)dev->dev_private; + + writel(HIBMC_RAW_INTERRUPT_EN_VBLANK(0), + hidev->mmio + HIBMC_RAW_INTERRUPT_EN); +} + +irqreturn_t hibmc_drm_interrupt(int irq, void *arg) +{ + struct drm_device *dev = (struct drm_device *)arg; + struct hibmc_drm_device *hidev = + (struct hibmc_drm_device *)dev->dev_private; + struct drm_crtc *crtc = &hidev->crtc; + u32 status; + + status = readl(hidev->mmio + HIBMC_RAW_INTERRUPT); + + if (status & HIBMC_RAW_INTERRUPT_VBLANK(1)) { + writel(HIBMC_RAW_INTERRUPT_VBLANK(1), + hidev->mmio + HIBMC_RAW_INTERRUPT); + drm_crtc_handle_vblank(crtc); + } + + return IRQ_HANDLED; } static struct drm_driver hibmc_driver = { - .driver_features= DRIVER_GEM | + .driver_features= DRIVER_HAVE_IRQ | DRIVER_GEM | DRIVER_MODESET | DRIVER_ATOMIC, .fops = &hibmc_fops, .name = "hibmc", @@ -68,6 +98,7 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe) .dumb_create= hibmc_dumb_create, .dumb_map_offset= hibmc_dumb_mmap_offset, .dumb_destroy = drm_gem_dumb_destroy, + .irq_handler= hibmc_drm_interrupt, }; static int hibmc_pm_suspend(struct device *dev) @@ -287,6 +318,20 @@ static int hibmc_load(struct drm_device *dev, unsigned long flags) if (ret) goto err; + ret = drm_vblank_init(dev, dev->mode_config.num_crtc); + if (ret) { + DRM_ERROR("failed to initialize vblank.\n"); + return ret; + } + + if (pci_enable_msi(dev->pdev)) { + DRM_ERROR("Enabling MSI failed!\n"); + } else { + ret = drm_irq_install(dev, dev->pdev->irq); + if (ret) + DRM_ERROR("install irq failed , ret = %d\n", ret); + } + /* reset all the states of crtc/plane/encoder/connector */ drm_mode_config_reset(dev); -- 1.9.1
[PATCH v3 2/9] drm/hisilicon/hibmc: Add video memory management
Hibmc have 32m video memory which can be accessed through PCIe by host, we use ttm to manage these memory. Signed-off-by: Rongrong Zou --- drivers/gpu/drm/hisilicon/hibmc/Makefile| 2 +- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 12 +- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 44 +++ drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 501 4 files changed, 557 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile index 97cf4a0..d5c40b8 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile @@ -1,5 +1,5 @@ ccflags-y := -Iinclude/drm -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o hibmc_ttm.o obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o #obj-y += hibmc-drm.o diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c index 52c9353..de3b3c3 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c @@ -37,6 +37,7 @@ #ifdef CONFIG_COMPAT .compat_ioctl = drm_compat_ioctl, #endif + .mmap = hibmc_mmap, .poll = drm_poll, .read = drm_read, .llseek = no_llseek, @@ -52,7 +53,7 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe) } static struct drm_driver hibmc_driver = { - .driver_features= DRIVER_ATOMIC, + .driver_features= DRIVER_GEM | DRIVER_ATOMIC, .fops = &hibmc_fops, .name = "hibmc", .date = "20160828", @@ -62,6 +63,10 @@ static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe) .get_vblank_counter = drm_vblank_no_hw_counter, .enable_vblank = hibmc_enable_vblank, .disable_vblank = hibmc_disable_vblank, + .gem_free_object_unlocked = hibmc_gem_free_object, + .dumb_create= hibmc_dumb_create, + .dumb_map_offset= hibmc_dumb_mmap_offset, + .dumb_destroy = drm_gem_dumb_destroy, }; static int hibmc_pm_suspend(struct device *dev) @@ -202,6 +207,11 @@ static int hibmc_load(struct drm_device *dev, unsigned long flags) if (ret) goto err; + ret = hibmc_mm_init(hidev); + + if (ret) + goto err; + return 0; err: diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h index 3bc8e64..d5ef34b 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h @@ -37,5 +37,49 @@ struct hibmc_drm_device { /* drm */ struct drm_device *dev; + + /* ttm */ + struct { + struct drm_global_reference mem_global_ref; + struct ttm_bo_global_ref bo_global_ref; + struct ttm_bo_device bdev; + bool initialized; + } ttm; + + bool mm_inited; }; + +struct hibmc_bo { + struct ttm_buffer_object bo; + struct ttm_placement placement; + struct ttm_bo_kmap_obj kmap; + struct drm_gem_object gem; + struct ttm_place placements[3]; + int pin_count; +}; + +static inline struct hibmc_bo *hibmc_bo(struct ttm_buffer_object *bo) +{ + return container_of(bo, struct hibmc_bo, bo); +} + +static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem) +{ + return container_of(gem, struct hibmc_bo, gem); +} + +#define DRM_FILE_PAGE_OFFSET (0x1ULL >> PAGE_SHIFT) + +int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, +struct drm_gem_object **obj); + +int hibmc_mm_init(struct hibmc_drm_device *hibmc); +int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr); +void hibmc_gem_free_object(struct drm_gem_object *obj); +int hibmc_dumb_create(struct drm_file *file, struct drm_device *dev, + struct drm_mode_create_dumb *args); +int hibmc_dumb_mmap_offset(struct drm_file *file, struct drm_device *dev, + u32 handle, u64 *offset); +int hibmc_mmap(struct file *filp, struct vm_area_struct *vma); + #endif diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c new file mode 100644 index 000..6789970 --- /dev/null +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c @@ -0,0 +1,501 @@ +/* Hisilicon Hibmc SoC drm driver + * + * Based on the bochs drm driver. + * + * Copyright (c) 2016 Huawei Limited. + * + * Author: + * Rongrong Zou + * Rongrong Zou + * Jianhua Li + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU Genera
[PATCH v3 4/9] drm/hisilicon/hibmc: Add plane for DE
Add plane funcs and helper funcs for DE. Signed-off-by: Rongrong Zou --- drivers/gpu/drm/hisilicon/hibmc/Makefile| 2 +- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 188 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.h | 29 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 46 +- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 5 + drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 6 + 6 files changed, 274 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.h diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile index 810a37e..72e107e 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile @@ -1,5 +1,5 @@ ccflags-y := -Iinclude/drm -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_fbdev.o hibmc_drm_power.o hibmc_ttm.o +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_fbdev.o hibmc_drm_power.o hibmc_ttm.o obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o #obj-y += hibmc-drm.o diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c new file mode 100644 index 000..28d2e65 --- /dev/null +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c @@ -0,0 +1,188 @@ +/* Hisilicon Hibmc SoC drm driver + * + * Based on the bochs drm driver. + * + * Copyright (c) 2016 Huawei Limited. + * + * Author: + * Rongrong Zou + * Rongrong Zou + * Jianhua Li + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + */ + +#include +#include +#include +#include +#include +#include +#include + +#include "hibmc_drm_drv.h" +#include "hibmc_drm_regs.h" +#include "hibmc_drm_power.h" + +/* -- */ + +static int hibmc_plane_prepare_fb(struct drm_plane *plane, + const struct drm_plane_state *new_state) +{ + /* do nothing */ + return 0; +} + +static void hibmc_plane_cleanup_fb(struct drm_plane *plane, + const struct drm_plane_state *old_state) +{ + /* do nothing */ +} + +static int hibmc_plane_atomic_check(struct drm_plane *plane, + struct drm_plane_state *state) +{ + struct drm_framebuffer *fb = state->fb; + struct drm_crtc *crtc = state->crtc; + struct drm_crtc_state *crtc_state; + u32 src_x = state->src_x >> 16; + u32 src_y = state->src_y >> 16; + u32 src_w = state->src_w >> 16; + u32 src_h = state->src_h >> 16; + int crtc_x = state->crtc_x; + int crtc_y = state->crtc_y; + u32 crtc_w = state->crtc_w; + u32 crtc_h = state->crtc_h; + + if (!crtc || !fb) + return 0; + + crtc_state = drm_atomic_get_crtc_state(state->state, crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + + if (src_w != crtc_w || src_h != crtc_h) { + DRM_ERROR("Scale not support!!!\n"); + return -EINVAL; + } + + if (src_x + src_w > fb->width || + src_y + src_h > fb->height) + return -EINVAL; + + if (crtc_x < 0 || crtc_y < 0) + return -EINVAL; + + if (crtc_x + crtc_w > crtc_state->adjusted_mode.hdisplay || + crtc_y + crtc_h > crtc_state->adjusted_mode.vdisplay) + return -EINVAL; + + return 0; +} + +static void hibmc_plane_atomic_update(struct drm_plane *plane, + struct drm_plane_state *old_state) +{ + struct drm_plane_state *state = plane->state; + u32 reg; + int ret; + u64 gpu_addr = 0; + unsigned int line_l; + struct hibmc_drm_device *hidev = + (struct hibmc_drm_device *)plane->dev->dev_private; + + struct hibmc_framebuffer *hibmc_fb; + struct hibmc_bo *bo; + + hibmc_fb = to_hibmc_framebuffer(state->fb); + bo = gem_to_hibmc_bo(hibmc_fb->obj); + ret = ttm_bo_reserve(&bo->bo, true, false, NULL); + if (ret) + return; + + hibmc_bo_pin(bo, TTM_PL_FLAG_VRAM, &gpu_addr); + if (ret) { + ttm_bo_unreserve(&bo->bo); + return; + } + + ttm_bo_unreserve(&bo->bo); + + writel(gpu_addr, hidev->mmio + HIBMC_CRT_FB_ADDRESS); + + reg = state->fb->width * (state->fb->bits_per_pixel >> 3); + /* now line_pad is 16 */ + reg = PADDING(16, reg); + + line_l = state->fb->width * state->fb->bits_per_pixel / 8; + line_l = PADDING(16, line_l); + writel((HIBMC_CRT_FB_WIDTH_WIDTH(reg) & HIBMC_CRT_FB_WIDTH_WIDTH_MASK)
[PATCH v3 3/9] drm/hisilicon/hibmc: Add support for frame buffer
Add support for fbdev and kms fb management. Signed-off-by: Rongrong Zou --- drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 18 ++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 24 ++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c | 259 ++ drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 68 ++ 5 files changed, 370 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile index d5c40b8..810a37e 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile @@ -1,5 +1,5 @@ ccflags-y := -Iinclude/drm -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o hibmc_ttm.o +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_fbdev.o hibmc_drm_power.o hibmc_ttm.o obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o #obj-y += hibmc-drm.o diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c index de3b3c3..ba31ead 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c @@ -73,9 +73,16 @@ static int hibmc_pm_suspend(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct drm_device *drm_dev = pci_get_drvdata(pdev); + struct hibmc_drm_device *hidev = drm_dev->dev_private; drm_kms_helper_poll_disable(drm_dev); + if (hidev->fbdev.initialized) { + console_lock(); + drm_fb_helper_set_suspend(&hidev->fbdev.helper, 1); + console_unlock(); + } + return 0; } @@ -83,9 +90,16 @@ static int hibmc_pm_resume(struct device *dev) { struct pci_dev *pdev = to_pci_dev(dev); struct drm_device *drm_dev = pci_get_drvdata(pdev); + struct hibmc_drm_device *hidev = drm_dev->dev_private; drm_helper_resume_force_mode(drm_dev); + if (hidev->fbdev.initialized) { + console_lock(); + drm_fb_helper_set_suspend(&hidev->fbdev.helper, 0); + console_unlock(); + } + drm_kms_helper_poll_enable(drm_dev); return 0; @@ -187,6 +201,7 @@ static int hibmc_unload(struct drm_device *dev) { struct hibmc_drm_device *hidev = dev->dev_private; + hibmc_fbdev_fini(hidev); hibmc_hw_fini(hidev); dev->dev_private = NULL; return 0; @@ -208,7 +223,10 @@ static int hibmc_load(struct drm_device *dev, unsigned long flags) goto err; ret = hibmc_mm_init(hidev); + if (ret) + goto err; + ret = hibmc_fbdev_init(hidev); if (ret) goto err; diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h index d5ef34b..c37ce2b 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h @@ -28,6 +28,19 @@ #include #include +struct hibmc_framebuffer { + struct drm_framebuffer fb; + struct drm_gem_object *obj; + bool is_fbdev_fb; +}; + +struct hibmc_fbdev { + struct drm_fb_helper helper; + struct hibmc_framebuffer fb; + int size; + bool initialized; +}; + struct hibmc_drm_device { /* hw */ void __iomem *mmio; @@ -46,9 +59,13 @@ struct hibmc_drm_device { bool initialized; } ttm; + /* fbdev */ + struct hibmc_fbdev fbdev; bool mm_inited; }; +#define to_hibmc_framebuffer(x) container_of(x, struct hibmc_framebuffer, fb) + struct hibmc_bo { struct ttm_buffer_object bo; struct ttm_placement placement; @@ -70,8 +87,15 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem) #define DRM_FILE_PAGE_OFFSET (0x1ULL >> PAGE_SHIFT) +int hibmc_fbdev_init(struct hibmc_drm_device *hidev); +void hibmc_fbdev_fini(struct hibmc_drm_device *hidev); + int hibmc_gem_create(struct drm_device *dev, u32 size, bool iskernel, struct drm_gem_object **obj); +int hibmc_framebuffer_init(struct drm_device *dev, + struct hibmc_framebuffer *gfb, + const struct drm_mode_fb_cmd2 *mode_cmd, + struct drm_gem_object *obj); int hibmc_mm_init(struct hibmc_drm_device *hibmc); int hibmc_bo_pin(struct hibmc_bo *bo, u32 pl_flag, u64 *gpu_addr); diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c new file mode 100644 index 000..3219511 --- /dev/null +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c @@ -0,0 +1,259 @@ +/* Hisilicon Hibmc SoC drm driver + * + * Based on the bochs drm driver. + * + * Copyright (c) 2016 Huawei Limited. + * + * Author: + * Rongrong Zou + * Rongrong Zou + * Jianhua
[PATCH v3 0/9] Add DRM driver for Hisilicon Hibmc
This patch set adds a new drm driver for Hisilicon Hibmc. Hibmc is a BMC SoC with a display controller intergrated, usually it is used on server for Out-of-band management purpose. In this patch set, we just support basic function for Hibmc display subsystem. Hibmc display subsystem is connected to host CPU by PCIe as blow: +--+ +--+ | | PCIe | Hibmc | |host CPU( |<->| display | |arm64,x86)| |subsystem | +--+ +--+ Hardware Detail for Hibmc display subsystem --- The display subsystem of Hibmc is show as bellow: ++ ++ ++ ++ || || || || | FB |->| DE |->|VDAC|>|external| || || || | VGA| ++ ++ ++ ++ -DE(Display Engine) is the display controller. -VDAC(Video Digital-to-Analog converter) converts the RGB diaital data stream from DE to VGA analog signals. Change History Changes in v3: -enable KMS, in v2, only fbdev is enabled. -management video memory with ttm. -add vblank interrupt. -remove drm_connector_register_all() and drm_connector_unregister_all(). -I have a basic test with igt. Changes in v2: -Remove self-defined macros for bit operations. -Remove unused register. -Replace those deprecated functions with new version of them. -use drm_connector_register_all() to register connector after drm_dev_register(). The patch v2 is at https://lists.freedesktop.org/archives/dri-devel/2016-May/108661.html There are too many modifications, sorry for sending v3 sevaral months later. Rongrong Zou (9): drm/hisilicon/hibmc: Add hisilicon hibmc drm master driver drm/hisilicon/hibmc: Add video memory management drm/hisilicon/hibmc: Add support for frame buffer drm/hisilicon/hibmc: Add plane for DE drm/hisilicon/hibmc: Add crtc for DE drm/hisilicon/hibmc: Add encoder for VDAC drm/hisilicon/hibmc: Add connector for VDAC drm/hisilicon/hibmc: Add vblank interruput MAINTAINERS: Update HISILICON DRM entries MAINTAINERS | 1 + drivers/gpu/drm/hisilicon/Kconfig | 1 + drivers/gpu/drm/hisilicon/Makefile| 1 + drivers/gpu/drm/hisilicon/hibmc/Kconfig | 15 + drivers/gpu/drm/hisilicon/hibmc/Makefile | 5 + drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c| 506 +++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.h| 29 ++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 425 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 120 + drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c | 259 ++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c | 88 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h | 28 ++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h | 212 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 173 +++ drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c | 575 ++ 15 files changed, 2438 insertions(+) create mode 100644 drivers/gpu/drm/hisilicon/hibmc/Kconfig create mode 100644 drivers/gpu/drm/hisilicon/hibmc/Makefile create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.h create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_fbdev.c create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_ttm.c -- 1.9.1
[PATCH v3 1/9] drm/hisilicon/hibmc: Add hisilicon hibmc drm master driver
Add DRM master driver for Hisilicon Hibmc SoC which used for Out-of-band management. Blow is the general hardware connection, both the Hibmc and the host CPU are on the same mother board. +--+ +--+ | | PCIe | Hibmc | |host CPU( |<->| display | |arm64,x86)| |subsystem | +--+ +--+ Signed-off-by: Rongrong Zou --- drivers/gpu/drm/hisilicon/Kconfig | 1 + drivers/gpu/drm/hisilicon/Makefile| 1 + drivers/gpu/drm/hisilicon/hibmc/Kconfig | 15 ++ drivers/gpu/drm/hisilicon/hibmc/Makefile | 5 + drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 288 ++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 41 +++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c | 88 +++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h | 28 +++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h | 212 9 files changed, 679 insertions(+) create mode 100644 drivers/gpu/drm/hisilicon/hibmc/Kconfig create mode 100644 drivers/gpu/drm/hisilicon/hibmc/Makefile create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.c create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_power.h create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_regs.h diff --git a/drivers/gpu/drm/hisilicon/Kconfig b/drivers/gpu/drm/hisilicon/Kconfig index 558c61b..2fd2724 100644 --- a/drivers/gpu/drm/hisilicon/Kconfig +++ b/drivers/gpu/drm/hisilicon/Kconfig @@ -2,4 +2,5 @@ # hisilicon drm device configuration. # Please keep this list sorted alphabetically +source "drivers/gpu/drm/hisilicon/hibmc/Kconfig" source "drivers/gpu/drm/hisilicon/kirin/Kconfig" diff --git a/drivers/gpu/drm/hisilicon/Makefile b/drivers/gpu/drm/hisilicon/Makefile index e3f6d49..c8155bf 100644 --- a/drivers/gpu/drm/hisilicon/Makefile +++ b/drivers/gpu/drm/hisilicon/Makefile @@ -2,4 +2,5 @@ # Makefile for hisilicon drm drivers. # Please keep this list sorted alphabetically +obj-$(CONFIG_DRM_HISI_HIBMC) += hibmc/ obj-$(CONFIG_DRM_HISI_KIRIN) += kirin/ diff --git a/drivers/gpu/drm/hisilicon/hibmc/Kconfig b/drivers/gpu/drm/hisilicon/hibmc/Kconfig new file mode 100644 index 000..277f9c8 --- /dev/null +++ b/drivers/gpu/drm/hisilicon/hibmc/Kconfig @@ -0,0 +1,15 @@ +config DRM_HISI_HIBMC + tristate "DRM Support for Hisilicon Hibmc" + depends on DRM && PCI + select DRM_KMS_HELPER + select DRM_KMS_FB_HELPER + select DRM_GEM_CMA_HELPER + select DRM_KMS_CMA_HELPER + select FB_SYS_FILLRECT + select FB_SYS_COPYAREA + select FB_SYS_IMAGEBLIT + select DRM_TTM + + help + Choose this option if you have a Hisilicon Hibmc soc chipset. + If M is selected the module will be called hibmc-drm. diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile new file mode 100644 index 000..97cf4a0 --- /dev/null +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile @@ -0,0 +1,5 @@ +ccflags-y := -Iinclude/drm +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_power.o + +obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o +#obj-y += hibmc-drm.o diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c new file mode 100644 index 000..52c9353 --- /dev/null +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c @@ -0,0 +1,288 @@ +/* Hisilicon Hibmc SoC drm driver + * + * Based on the bochs drm driver. + * + * Copyright (c) 2016 Huawei Limited. + * + * Author: + * Rongrong Zou + * Rongrong Zou + * Jianhua Li + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + */ + +#include +#include +#include +#include +#include +#include +#include +#include + +#include "hibmc_drm_drv.h" +#include "hibmc_drm_regs.h" +#include "hibmc_drm_power.h" + +static const struct file_operations hibmc_fops = { + .owner = THIS_MODULE, + .open = drm_open, + .release= drm_release, + .unlocked_ioctl = drm_ioctl, +#ifdef CONFIG_COMPAT + .compat_ioctl = drm_compat_ioctl, +#endif + .poll = drm_poll, + .read = drm_read, + .llseek = no_llseek, +}; + +static int hibmc_enable_vblank(struct drm_device *dev, unsigned int pipe) +{ + return 0; +} + +static void hibmc_disable_vblank(struct drm_device *dev, unsigned int pipe) +{ +} + +static struct drm_driver hibmc_driver = { + .driver_features= DRIVER_ATOMIC, + .fops = &hibmc_fops, + .name = "hibmc", + .date
[Intel-gfx] [patch] drm/i915: fix a read size argument
On Thu, Oct 13, 2016 at 11:55:08AM +0300, Dan Carpenter wrote: > We want to read 3 bytes here, but because the parenthesis are in the > wrong place we instead read: > > sizeof(intel_dp->edp_dpcd) == sizeof(intel_dp->edp_dpcd) > > which is one byte. > > Fixes: fe5a66f91c88 ("drm/i915: Read PSR caps/intermediate freqs/etc. only > once on eDP") > Signed-off-by: Dan Carpenter Oops, does smatch catch this? I don't recall seeing the complaint in recent runs? Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre
[PATCH v3 7/9] drm/hisilicon/hibmc: Add connector for VDAC
Add connector funcs and helper funcs for VDAC. Signed-off-by: Rongrong Zou --- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 8 +++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 2 + drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 82 3 files changed, 92 insertions(+) diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c index f690798..7c88295 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c @@ -147,6 +147,14 @@ static int hibmc_kms_init(struct hibmc_drm_device *hidev) return ret; } + ret = hibmc_connector_init(hidev); + if (ret) { + DRM_ERROR("failed to init connector\n"); + return ret; + } + + drm_mode_connector_attach_encoder(&hidev->connector, + &hidev->encoder); return 0; } diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h index a888f5c..db4a872 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h @@ -53,6 +53,7 @@ struct hibmc_drm_device { struct drm_plane plane; struct drm_crtc crtc; struct drm_encoder encoder; + struct drm_connector connector; bool mode_config_initialized; /* ttm */ @@ -94,6 +95,7 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem) int hibmc_plane_init(struct hibmc_drm_device *hidev); int hibmc_crtc_init(struct hibmc_drm_device *hidev); int hibmc_encoder_init(struct hibmc_drm_device *hidev); +int hibmc_connector_init(struct hibmc_drm_device *hidev); int hibmc_fbdev_init(struct hibmc_drm_device *hidev); void hibmc_fbdev_fini(struct hibmc_drm_device *hidev); diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c index 4ae8677..30e04ba 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c @@ -89,3 +89,85 @@ int hibmc_encoder_init(struct hibmc_drm_device *hidev) drm_encoder_helper_add(encoder, &hibmc_encoder_helper_funcs); return 0; } + +static int hibmc_connector_get_modes(struct drm_connector *connector) +{ + int count; + + count = drm_add_modes_noedid(connector, 800, 600); + drm_set_preferred_mode(connector, defx, defy); + return count; +} + +static int hibmc_connector_mode_valid(struct drm_connector *connector, + struct drm_display_mode *mode) +{ + struct hibmc_drm_device *hiprivate = +container_of(connector, struct hibmc_drm_device, connector); + unsigned long size = mode->hdisplay * mode->vdisplay * 4; + + /* + * Make sure we can fit two framebuffers into video memory. + * This allows up to 1600x1200 with 16 MB (default size). + * If you want more try this: + * 'qemu -vga std -global VGA.vgamem_mb=32 $otherargs' + */ + if (size * 2 > hiprivate->fb_size) + return MODE_BAD; + + return MODE_OK; +} + +static struct drm_encoder * +hibmc_connector_best_encoder(struct drm_connector *connector) +{ + int enc_id = connector->encoder_ids[0]; + + /* pick the encoder ids */ + if (enc_id) + return drm_encoder_find(connector->dev, enc_id); + + return NULL; +} + +static enum drm_connector_status hibmc_connector_detect(struct drm_connector +*connector, bool force) +{ + return connector_status_connected; +} + +static const struct drm_connector_helper_funcs + hibmc_connector_connector_helper_funcs = { + .get_modes = hibmc_connector_get_modes, + .mode_valid = hibmc_connector_mode_valid, + .best_encoder = hibmc_connector_best_encoder, +}; + +static const struct drm_connector_funcs hibmc_connector_connector_funcs = { + .dpms = drm_atomic_helper_connector_dpms, + .detect = hibmc_connector_detect, + .fill_modes = drm_helper_probe_single_connector_modes, + .destroy = drm_connector_cleanup, + .reset = drm_atomic_helper_connector_reset, + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, +}; + +int hibmc_connector_init(struct hibmc_drm_device *hidev) +{ + struct drm_device *dev = hidev->dev; + struct drm_connector *connector = &hidev->connector; + int ret; + + ret = drm_connector_init(dev, connector, +&hibmc_connector_connector_funcs, +DRM_MODE_CONNECTOR_VGA); + if (ret) { + DRM_ERROR("failed to init connector\n"); + return ret; + } + drm_connector_helper_add(connect
[PATCH v3 6/9] drm/hisilicon/hibmc: Add encoder for VDAC
Add encoder funcs and helpers for VDAC. Signed-off-by: Rongrong Zou --- drivers/gpu/drm/hisilicon/hibmc/Makefile | 2 +- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 ++ drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 2 + drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c | 91 4 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c diff --git a/drivers/gpu/drm/hisilicon/hibmc/Makefile b/drivers/gpu/drm/hisilicon/hibmc/Makefile index 72e107e..e04f114 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/Makefile +++ b/drivers/gpu/drm/hisilicon/hibmc/Makefile @@ -1,5 +1,5 @@ ccflags-y := -Iinclude/drm -hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_fbdev.o hibmc_drm_power.o hibmc_ttm.o +hibmc-drm-y := hibmc_drm_drv.o hibmc_drm_de.o hibmc_drm_vdac.o hibmc_drm_fbdev.o hibmc_drm_power.o hibmc_ttm.o obj-$(CONFIG_DRM_HISI_HIBMC) +=hibmc-drm.o #obj-y += hibmc-drm.o diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c index b054b13..f690798 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c @@ -141,6 +141,12 @@ static int hibmc_kms_init(struct hibmc_drm_device *hidev) return ret; } + ret = hibmc_encoder_init(hidev); + if (ret) { + DRM_ERROR("failed to init encoder\n"); + return ret; + } + return 0; } diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h index b4b27c6..a888f5c 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h @@ -52,6 +52,7 @@ struct hibmc_drm_device { struct drm_device *dev; struct drm_plane plane; struct drm_crtc crtc; + struct drm_encoder encoder; bool mode_config_initialized; /* ttm */ @@ -92,6 +93,7 @@ static inline struct hibmc_bo *gem_to_hibmc_bo(struct drm_gem_object *gem) int hibmc_plane_init(struct hibmc_drm_device *hidev); int hibmc_crtc_init(struct hibmc_drm_device *hidev); +int hibmc_encoder_init(struct hibmc_drm_device *hidev); int hibmc_fbdev_init(struct hibmc_drm_device *hidev); void hibmc_fbdev_fini(struct hibmc_drm_device *hidev); diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c new file mode 100644 index 000..4ae8677 --- /dev/null +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_vdac.c @@ -0,0 +1,91 @@ +/* Hisilicon Hibmc SoC drm driver + * + * Based on the bochs drm driver. + * + * Copyright (c) 2016 Huawei Limited. + * + * Author: + * Rongrong Zou + * Rongrong Zou + * Jianhua Li + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + */ + +#include +#include +#include +#include + +#include "hibmc_drm_drv.h" +#include "hibmc_drm_regs.h" + +static int defx = 800; +static int defy = 600; + +module_param(defx, int, 0444); +module_param(defy, int, 0444); +MODULE_PARM_DESC(defx, "default x resolution"); +MODULE_PARM_DESC(defy, "default y resolution"); + +static void hibmc_encoder_disable(struct drm_encoder *encoder) +{ +} + +static void hibmc_encoder_enable(struct drm_encoder *encoder) +{ +} + +static void hibmc_encoder_mode_set(struct drm_encoder *encoder, + struct drm_display_mode *mode, + struct drm_display_mode *adj_mode) +{ + u32 reg; + struct drm_device *dev = encoder->dev; + struct hibmc_drm_device *hidev = dev->dev_private; + + /* just open DISPLAY_CONTROL_HISILE register bit 3:0*/ + reg = readl(hidev->mmio + DISPLAY_CONTROL_HISILE); + reg |= 0xf; + writel(reg, hidev->mmio + DISPLAY_CONTROL_HISILE); +} + +static int hibmc_encoder_atomic_check(struct drm_encoder *encoder, + struct drm_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + return 0; +} + +static const struct drm_encoder_helper_funcs hibmc_encoder_helper_funcs = { + .mode_set = hibmc_encoder_mode_set, + .disable = hibmc_encoder_disable, + .enable = hibmc_encoder_enable, + .atomic_check = hibmc_encoder_atomic_check, +}; + +static const struct drm_encoder_funcs hibmc_encoder_encoder_funcs = { + .destroy = drm_encoder_cleanup, +}; + +int hibmc_encoder_init(struct hibmc_drm_device *hidev) +{ + struct drm_device *dev = hidev->dev; + struct drm_encoder *encoder = &hidev->encoder; + int ret; + + encoder->possible_crtcs = 0x1; + ret = drm_encoder_init(dev, encoder, &hibmc_encod
[PATCH v3 5/9] drm/hisilicon/hibmc: Add crtc for DE
Add crtc funcs and helper funcs for DE. Signed-off-by: Rongrong Zou --- drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c | 318 drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.c | 6 + drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_drv.h | 2 + 3 files changed, 326 insertions(+) diff --git a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c index 28d2e65..5e7f171 100644 --- a/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c +++ b/drivers/gpu/drm/hisilicon/hibmc/hibmc_drm_de.c @@ -26,6 +26,7 @@ #include "hibmc_drm_drv.h" #include "hibmc_drm_regs.h" +#include "hibmc_drm_de.h" #include "hibmc_drm_power.h" /* -- */ @@ -186,3 +187,320 @@ int hibmc_plane_init(struct hibmc_drm_device *hidev) drm_plane_helper_add(plane, &hibmc_plane_helper_funcs); return 0; } + +static void hibmc_crtc_enable(struct drm_crtc *crtc) +{ + unsigned int reg; + /* power mode 0 is default. */ + struct hibmc_drm_device *hidev = crtc->dev->dev_private; + + hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_MODE0); + + /* Enable display power gate & LOCALMEM power gate*/ + reg = readl(hidev->mmio + HIBMC_CURRENT_GATE); + reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK; + reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK; + reg |= HIBMC_CURR_GATE_LOCALMEM(ON); + reg |= HIBMC_CURR_GATE_DISPLAY(ON); + hibmc_set_current_gate(hidev, reg); + drm_crtc_vblank_on(crtc); +} + +static void hibmc_crtc_disable(struct drm_crtc *crtc) +{ + unsigned int reg; + struct hibmc_drm_device *hidev = crtc->dev->dev_private; + + drm_crtc_vblank_off(crtc); + + hibmc_set_power_mode(hidev, HIBMC_PW_MODE_CTL_MODE_SLEEP); + + /* Enable display power gate & LOCALMEM power gate*/ + reg = readl(hidev->mmio + HIBMC_CURRENT_GATE); + reg &= ~HIBMC_CURR_GATE_LOCALMEM_MASK; + reg &= ~HIBMC_CURR_GATE_DISPLAY_MASK; + reg |= HIBMC_CURR_GATE_LOCALMEM(OFF); + reg |= HIBMC_CURR_GATE_DISPLAY(OFF); + hibmc_set_current_gate(hidev, reg); +} + +static int hibmc_crtc_atomic_check(struct drm_crtc *crtc, + struct drm_crtc_state *state) +{ + return 0; +} + +static unsigned int format_pll_reg(void) +{ + unsigned int pllreg = 0; + struct panel_pll pll = {0}; + + /* Note that all PLL's have the same format. Here, + * we just use Panel PLL parameter to work out the bit + * fields in the register.On returning a 32 bit number, the value can + * be applied to any PLL in the calling function. + */ + pllreg |= HIBMC_PLL_CTRL_BYPASS(OFF) & HIBMC_PLL_CTRL_BYPASS_MASK; + pllreg |= HIBMC_PLL_CTRL_POWER(ON) & HIBMC_PLL_CTRL_POWER_MASK; + pllreg |= HIBMC_PLL_CTRL_INPUT(OSC) & HIBMC_PLL_CTRL_INPUT_MASK; + pllreg |= HIBMC_PLL_CTRL_POD(pll.POD) & HIBMC_PLL_CTRL_POD_MASK; + pllreg |= HIBMC_PLL_CTRL_OD(pll.OD) & HIBMC_PLL_CTRL_OD_MASK; + pllreg |= HIBMC_PLL_CTRL_N(pll.N) & HIBMC_PLL_CTRL_N_MASK; + pllreg |= HIBMC_PLL_CTRL_M(pll.M) & HIBMC_PLL_CTRL_M_MASK; + + return pllreg; +} + +static void set_vclock_hisilicon(struct drm_device *dev, unsigned long pll) +{ + unsigned long tmp0, tmp1; + struct hibmc_drm_device *hidev = dev->dev_private; + +/* 1. outer_bypass_n=0 */ + tmp0 = readl(hidev->mmio + CRT_PLL1_HS); + tmp0 &= 0xBFFF; + writel(tmp0, hidev->mmio + CRT_PLL1_HS); + + /* 2. pll_pd=1?inter_bypass=1 */ + writel(0x2100, hidev->mmio + CRT_PLL1_HS); + + /* 3. config pll */ + writel(pll, hidev->mmio + CRT_PLL1_HS); + + /* 4. delay */ + mdelay(1); + + /* 5. pll_pd =0 */ + tmp1 = pll & ~0x0100; + writel(tmp1, hidev->mmio + CRT_PLL1_HS); + + /* 6. delay */ + mdelay(1); + + /* 7. inter_bypass=0 */ + tmp1 &= ~0x2000; + writel(tmp1, hidev->mmio + CRT_PLL1_HS); + + /* 8. delay */ + mdelay(1); + + /* 9. outer_bypass_n=1 */ + tmp1 |= 0x4000; + writel(tmp1, hidev->mmio + CRT_PLL1_HS); +} + +/* This function takes care the extra registers and bit fields required to +*setup a mode in board. +*Explanation about Display Control register: +*FPGA only supports 7 predefined pixel clocks, and clock select is +*in bit 4:0 of new register 0x802a8. +*/ +static unsigned int display_ctrl_adjust(struct drm_device *dev, + struct drm_display_mode *mode, + unsigned int ctrl) +{ + unsigned long x, y; + unsigned long pll1; /* bit[31:0] of PLL */ + unsigned long pll2; /* bit[63:32] of PLL */ + struct hibmc_drm_device *hidev = dev->dev_private; + + x = mode->hdisplay; + y = mode->vdisplay; + + /* Hisilicon has to set up a new register for PLL control +*(CRT
[patch] drm/i915: fix a read size argument
On Thu, Oct 13, 2016 at 11:55:08AM +0300, Dan Carpenter wrote: > We want to read 3 bytes here, but because the parenthesis are in the > wrong place we instead read: > > sizeof(intel_dp->edp_dpcd) == sizeof(intel_dp->edp_dpcd) > > which is one byte. > > Fixes: fe5a66f91c88 ("drm/i915: Read PSR caps/intermediate freqs/etc. only > once on eDP") > Signed-off-by: Dan Carpenter Good catch! What tool did you use to find it, or did you find it by inspection? Reviewed-by: Eric Engestrom (btw, there's a missing `---` here, between the commit msg and the diff) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 14a3cf0..ee8aa95 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3551,8 +3551,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) > /* Read the eDP Display control capabilities registers */ > if ((intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & > DP_DPCD_DISPLAY_CONTROL_CAPABLE) && > drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, > - intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd) == > - sizeof(intel_dp->edp_dpcd))) > + intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd)) == > + sizeof(intel_dp->edp_dpcd)) > DRM_DEBUG_KMS("EDP DPCD : %*ph\n", (int) > sizeof(intel_dp->edp_dpcd), > intel_dp->edp_dpcd);
[patch] drm/i915: fix a read size argument
Am 13.10.2016 10:55, schrieb Dan Carpenter: > We want to read 3 bytes here, but because the parenthesis are in the > wrong place we instead read: > > sizeof(intel_dp->edp_dpcd) == sizeof(intel_dp->edp_dpcd) > > which is one byte. > > Fixes: fe5a66f91c88 ("drm/i915: Read PSR caps/intermediate freqs/etc. only > once on eDP") > Signed-off-by: Dan Carpenter > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 14a3cf0..ee8aa95 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3551,8 +3551,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) > /* Read the eDP Display control capabilities registers */ > if ((intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & > DP_DPCD_DISPLAY_CONTROL_CAPABLE) && > drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, > - intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd) == > - sizeof(intel_dp->edp_dpcd))) > + intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd)) == > + sizeof(intel_dp->edp_dpcd)) perhaps we can morph that into something more readable ? I would suggest: if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & DP_DPCD_DISPLAY_CONTROL_CAPABLE) { size_t size=sizeof(intel_dp->edp_dpcd); /* == EDP_DISPLAY_CTL_CAP_SIZE */ int ret; ret=drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV,intel_dp->edp_dpcd,size); if (ret != size ) .. } this way it improves readability and reduces line length. > DRM_DEBUG_KMS("EDP DPCD : %*ph\n", (int) > sizeof(intel_dp->edp_dpcd), > intel_dp->edp_dpcd); > > -- > To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
[PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset
Add some additional comments to more explicitly describe the meaning and usage of the three CRTC modeset detection booleans: mode_changed, connectors_changed and active_changed. Suggested-by: Daniel Vetter Signed-off-by: Brian Starkey --- Hi Daniel, I guess I asked for this one :-), please just check my understanding is correct. Thanks, Brian drivers/gpu/drm/drm_atomic_helper.c |9 + include/drm/drm_atomic.h| 11 ++- include/drm/drm_crtc.h |5 + 3 files changed, 20 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 78ea735..fb4071a 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -458,10 +458,11 @@ mode_fixup(struct drm_atomic_state *state) * removed from the crtc. * crtc_state->active_changed is set when crtc_state->active changes, * which is used for dpms. + * See also: drm_atomic_crtc_needs_modeset() * * IMPORTANT: * - * Drivers which update ->mode_changed (e.g. in their ->atomic_check hooks if a + * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if a * plane update can't be done without a full modeset) _must_ call this function * afterwards after that change. It is permitted to call this function multiple * times for the same update, e.g. when the ->atomic_check functions depend upon @@ -510,9 +511,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, for_each_connector_in_state(state, connector, connector_state, i) { /* -* This only sets crtc->mode_changed for routing changes, -* drivers must set crtc->mode_changed themselves when connector -* properties need to be updated. +* This only sets crtc->connectors_changed for routing changes, +* drivers must set crtc->connectors_changed themselves when +* connector properties need to be updated. */ ret = update_connector_routing(state, connector, connector_state); diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h index d9aff06..1ce255f 100644 --- a/include/drm/drm_atomic.h +++ b/include/drm/drm_atomic.h @@ -368,8 +368,17 @@ int __must_check drm_atomic_nonblocking_commit(struct drm_atomic_state *state); * * To give drivers flexibility struct &drm_crtc_state has 3 booleans to track * whether the state CRTC changed enough to need a full modeset cycle: - * connectors_changed, mode_changed and active_change. This helper simply + * connectors_changed, mode_changed and active_changed. This helper simply * combines these three to compute the overall need for a modeset for @state. + * + * The atomic helper code sets these booleans, but drivers can and should + * change them appropriately to accurately represent whether a modeset is + * really needed. In general, drivers should avoid full modesets whenever + * possible. + * + * For example if the CRTC mode has changed, and the hardware is able to enact + * the requested mode change without going through a full modeset, the driver + * should clear mode_changed during its ->atomic_check. */ static inline bool drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index c4a3164..1f094d2 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -116,6 +116,11 @@ struct drm_plane_helper_funcs; * never return in a failure from the ->atomic_check callback. Userspace assumes * that a DPMS On will always succeed. In other words: @enable controls resource * assignment, @active controls the actual hardware state. + * + * The three booleans active_changed, connectors_changed and mode_changed are + * intended to indicate whether a full modeset is needed, rather than strictly + * describing what has changed in a commit. + * See also: drm_atomic_crtc_needs_modeset() */ struct drm_crtc_state { struct drm_crtc *crtc; -- 1.7.9.5
[Intel-gfx] [patch] drm/i915: fix a read size argument
On Thu, Oct 13, 2016 at 10:01:03AM +0100, Chris Wilson wrote: > On Thu, Oct 13, 2016 at 11:55:08AM +0300, Dan Carpenter wrote: > > We want to read 3 bytes here, but because the parenthesis are in the > > wrong place we instead read: > > > > sizeof(intel_dp->edp_dpcd) == sizeof(intel_dp->edp_dpcd) > > > > which is one byte. > > > > Fixes: fe5a66f91c88 ("drm/i915: Read PSR caps/intermediate freqs/etc. only > > once on eDP") > > Signed-off-by: Dan Carpenter > > Oops, does smatch catch this? Yeah. It does. regards, dan carpenter
[patch] drm/exynos: fix a timeout loop
We were trying to print an error message if we timed out here, but the loop actually ends with "tries" set to UINT_MAX and not zero. Fix this by changing from tries-- to --tries. A for loop would actually be the most natural way to do this. My fix means we only loop 99 times instead of 100 but that's probably ok. Fixes: a696394c5224 ('drm/exynos: mixer: simplify loop in vp_win_reset()') Signed-off-by: Dan Carpenter diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c index edb20a3..fcc7e4f 100644 --- a/drivers/gpu/drm/exynos/exynos_mixer.c +++ b/drivers/gpu/drm/exynos/exynos_mixer.c @@ -701,7 +701,7 @@ static void vp_win_reset(struct mixer_context *ctx) unsigned int tries = 100; vp_reg_write(res, VP_SRESET, VP_SRESET_PROCESSING); - while (tries--) { + while (--tries) { /* waiting until VP_SRESET_PROCESSING is 0 */ if (~vp_reg_read(res, VP_SRESET) & VP_SRESET_PROCESSING) break;
[patch] drm/exynos: fix a timeout loop
Hello Dan, sorry for the blunder! Patch looks good to me. Reviewed-by: Tobias Jakobi With best wishes, Tobias Dan Carpenter wrote: > We were trying to print an error message if we timed out here, but the > loop actually ends with "tries" set to UINT_MAX and not zero. Fix this > by changing from tries-- to --tries. > > A for loop would actually be the most natural way to do this. My fix > means we only loop 99 times instead of 100 but that's probably ok. > > Fixes: a696394c5224 ('drm/exynos: mixer: simplify loop in vp_win_reset()') > Signed-off-by: Dan Carpenter > > diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c > b/drivers/gpu/drm/exynos/exynos_mixer.c > index edb20a3..fcc7e4f 100644 > --- a/drivers/gpu/drm/exynos/exynos_mixer.c > +++ b/drivers/gpu/drm/exynos/exynos_mixer.c > @@ -701,7 +701,7 @@ static void vp_win_reset(struct mixer_context *ctx) > unsigned int tries = 100; > > vp_reg_write(res, VP_SRESET, VP_SRESET_PROCESSING); > - while (tries--) { > + while (--tries) { > /* waiting until VP_SRESET_PROCESSING is 0 */ > if (~vp_reg_read(res, VP_SRESET) & VP_SRESET_PROCESSING) > break; > -- > To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" > in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html >
[PATCH] virtio-gpu: fix vblank events
virtio-gpu sends vblank events in virtio_gpu_crtc_atomic_flush, and because of that it must be called for disabled planes too. Ask drm_atomic_helper_commit_planes to do that. Signed-off-by: Gerd Hoffmann --- drivers/gpu/drm/virtio/virtgpu_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/virtio/virtgpu_display.c b/drivers/gpu/drm/virtio/virtgpu_display.c index 4e192aa..75ec172 100644 --- a/drivers/gpu/drm/virtio/virtgpu_display.c +++ b/drivers/gpu/drm/virtio/virtgpu_display.c @@ -338,7 +338,7 @@ static void vgdev_atomic_commit_tail(struct drm_atomic_state *state) drm_atomic_helper_commit_modeset_disables(dev, state); drm_atomic_helper_commit_modeset_enables(dev, state); - drm_atomic_helper_commit_planes(dev, state, true); + drm_atomic_helper_commit_planes(dev, state, false); drm_atomic_helper_commit_hw_done(state); -- 1.8.3.1
[patch] drm/i915: fix a read size argument
On Thu, 13 Oct 2016, walter harms wrote: > Am 13.10.2016 10:55, schrieb Dan Carpenter: >> We want to read 3 bytes here, but because the parenthesis are in the >> wrong place we instead read: >> >> sizeof(intel_dp->edp_dpcd) == sizeof(intel_dp->edp_dpcd) >> >> which is one byte. >> >> Fixes: fe5a66f91c88 ("drm/i915: Read PSR caps/intermediate freqs/etc. only >> once on eDP") >> Signed-off-by: Dan Carpenter Dan, good catch, thank you. Luckily we only really use the first byte currently... Cc: >> diff --git a/drivers/gpu/drm/i915/intel_dp.c >> b/drivers/gpu/drm/i915/intel_dp.c >> index 14a3cf0..ee8aa95 100644 >> --- a/drivers/gpu/drm/i915/intel_dp.c >> +++ b/drivers/gpu/drm/i915/intel_dp.c >> @@ -3551,8 +3551,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp) >> /* Read the eDP Display control capabilities registers */ >> if ((intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & >> DP_DPCD_DISPLAY_CONTROL_CAPABLE) && >> drm_dp_dpcd_read(&intel_dp->aux, DP_EDP_DPCD_REV, >> - intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd) == >> - sizeof(intel_dp->edp_dpcd))) >> + intel_dp->edp_dpcd, sizeof(intel_dp->edp_dpcd)) == >> + sizeof(intel_dp->edp_dpcd)) > > > > perhaps we can morph that into something more readable ? I would suggest: > > > if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & > DP_DPCD_DISPLAY_CONTROL_CAPABLE) > { > size_t size=sizeof(intel_dp->edp_dpcd); /* == EDP_DISPLAY_CTL_CAP_SIZE */ > int ret; > > ret=drm_dp_dpcd_read(&intel_dp->aux, > DP_EDP_DPCD_REV,intel_dp->edp_dpcd,size); > > if (ret != size ) > .. > > } > > this way it improves readability and reduces line length. Not convinced, let's just take the simple brace fix now. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center
[PATCH] drm/crtc: constify drm_crtc_mask parameter
On Thu, 13 Oct 2016, Maarten Lankhorst wrote: > Now that drm_crtc_index takes a const, the same can be done for drm_crtc_mask. > > Signed-off-by: Maarten Lankhorst Reviewed-by: Jani Nikula > --- > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index 0aa292526567..98ec40713921 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -1354,7 +1354,7 @@ static inline unsigned int drm_crtc_index(const struct > drm_crtc *crtc) > * Given a registered CRTC, return the mask bit of that CRTC for an > * encoder's possible_crtcs field. > */ > -static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc) > +static inline uint32_t drm_crtc_mask(const struct drm_crtc *crtc) > { > return 1 << drm_crtc_index(crtc); > } > diff --git a/include/drm/drm_encoder.h b/include/drm/drm_encoder.h > index 387e33a4d6ee..c7438ff0d609 100644 > --- a/include/drm/drm_encoder.h > +++ b/include/drm/drm_encoder.h > @@ -189,7 +189,7 @@ static inline unsigned int drm_encoder_index(struct > drm_encoder *encoder) > } > > /* FIXME: We have an include file mess still, drm_crtc.h needs untangling. */ > -static inline uint32_t drm_crtc_mask(struct drm_crtc *crtc); > +static inline uint32_t drm_crtc_mask(const struct drm_crtc *crtc); > > /** > * drm_encoder_crtc_ok - can a given crtc drive a given encoder? > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Jani Nikula, Intel Open Source Technology Center
[patch] drm/imx: drm_dev_alloc() returns error pointers
Am Donnerstag, den 13.10.2016, 11:53 +0300 schrieb Dan Carpenter: > We are checking for NULL here, when we should be checking for error > pointers. > > Fixes: 54db5decce17 ("drm/imx: drop deprecated load/unload drm_driver > ops") > Signed-off-by: Dan Carpenter Reviewed-by: Lucas Stach > > diff --git a/drivers/gpu/drm/imx/imx-drm-core.c > b/drivers/gpu/drm/imx/imx-drm-core.c > index 98df09c..3acc976 100644 > --- a/drivers/gpu/drm/imx/imx-drm-core.c > +++ b/drivers/gpu/drm/imx/imx-drm-core.c > @@ -357,8 +357,8 @@ static int imx_drm_bind(struct device *dev) > Â int ret; > Â > Â drm = drm_dev_alloc(&imx_drm_driver, dev); > - if (!drm) > - return -ENOMEM; > + if (IS_ERR(drm)) > + return PTR_ERR(drm); > Â > Â imxdrm = devm_kzalloc(dev, sizeof(*imxdrm), GFP_KERNEL); > Â if (!imxdrm) {
[Bug 94354] R9285 Unigine Valley perf regression since radeonsi: use re-Z
https://bugs.freedesktop.org/show_bug.cgi?id=94354 --- Comment #1 from russianneuromancer at ya.ru --- https://lists.freedesktop.org/archives/mesa-dev/2016-October/131759.html -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/b112d3c0/attachment.html>
[bug report] drm/amd/powerplay: add vce state tables initialize for ppt v1.
Hello Rex Zhu, The patch 48d7b759a8bc: "drm/amd/powerplay: add vce state tables initialize for ppt v1." from Aug 31, 2016, leads to the following static checker warning: drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/process_pptables_v1_0.c:1211 ppt_get_num_of_vce_state_table_entries_v1_0() warn: 'vce_state_table' can't be NULL. drivers/gpu/drm/amd/amdgpu/../powerplay/hwmgr/process_pptables_v1_0.c 1200 1201 static int ppt_get_num_of_vce_state_table_entries_v1_0(struct pp_hwmgr *hwmgr) 1202 { 1203 const ATOM_Tonga_POWERPLAYTABLE *pp_table = get_powerplay_table(hwmgr); 1204 const ATOM_Tonga_VCE_State_Table *vce_state_table = 1205 (ATOM_Tonga_VCE_State_Table *)(((unsigned long)pp_table) + le16_to_cpu(pp_table->usVCEStateTableOffset)); pp_table can't be NULL because we're dereferencing it here. That means vce_state_table can't be NULL either. 1206 1207 if (vce_state_table == NULL) 1208 return 0; 1209 1210 return vce_state_table->ucNumEntries; 1211 } 1212 A cleaner way to write this is maybe: static int ppt_get_num_of_vce_state_table_entries_v1_0(struct pp_hwmgr *hwmgr) { const ATOM_Tonga_POWERPLAYTABLE *pp_table = get_powerplay_table(hwmgr); const ATOM_Tonga_VCE_State_Table *vce_state_table; if (!pp_table) return 0; vce_state_table = (void *)pp_table + le16_to_cpu(pp_table->usVCEStateTableOffset); return vce_state_table->ucNumEntries; } regards, dan carpenter
[Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values
Op 08-10-16 om 02:11 schreef Lyude: > Now that we've make skl_wm_levels make a little more sense, we can > remove all of the redundant wm information. Up until now we'd been > storing two copies of all of the skl watermarks: one being the > skl_pipe_wm structs, the other being the global wm struct in > drm_i915_private containing the raw register values. This is confusing > and problematic, since it means we're prone to accidentally letting the > two copies go out of sync. So, get rid of all of the functions > responsible for computing the register values and just use a single > helper, skl_write_wm_level(), to convert and write the new watermarks on > the fly. > > Changes since v1: > - Fixup skl_write_wm_level() > - Fixup skl_wm_level_from_reg_val() > - Don't forget to copy *active to intel_crtc->wm.active.skl > > Signed-off-by: Lyude > Reviewed-by: Maarten Lankhorst > Cc: Ville Syrjälä > Cc: Paulo Zanoni > --- > drivers/gpu/drm/i915/i915_drv.h | 2 - > drivers/gpu/drm/i915/intel_display.c | 14 ++- > drivers/gpu/drm/i915/intel_drv.h | 6 +- > drivers/gpu/drm/i915/intel_pm.c | 204 > --- > drivers/gpu/drm/i915/intel_sprite.c | 8 +- > 5 files changed, 90 insertions(+), 144 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 0287c93..76583b2 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1644,8 +1644,6 @@ struct skl_ddb_allocation { > struct skl_wm_values { > unsigned dirty_pipes; > struct skl_ddb_allocation ddb; > - uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; > - uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES]; > }; > > struct skl_wm_level { > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index a71d05a..39400a0 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3378,6 +3378,8 @@ static void skylake_update_primary_plane(struct > drm_plane *plane, > struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state->base.crtc); > struct drm_framebuffer *fb = plane_state->base.fb; > const struct skl_wm_values *wm = &dev_priv->wm.skl_results; > + const struct skl_plane_wm *p_wm = > + &crtc_state->wm.skl.optimal.planes[0]; > int pipe = intel_crtc->pipe; > u32 plane_ctl; > unsigned int rotation = plane_state->base.rotation; > @@ -3414,7 +3416,7 @@ static void skylake_update_primary_plane(struct > drm_plane *plane, > intel_crtc->adjusted_y = src_y; > > if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base)) > - skl_write_plane_wm(intel_crtc, wm, 0); > + skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0); > > I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); > I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x); > @@ -3448,6 +3450,8 @@ static void skylake_disable_primary_plane(struct > drm_plane *primary, > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state); > + const struct skl_plane_wm *p_wm = &cstate->wm.skl.optimal.planes[0]; > int pipe = intel_crtc->pipe; > > /* > @@ -3455,7 +3459,8 @@ static void skylake_disable_primary_plane(struct > drm_plane *primary, >* plane's visiblity isn't actually changing neither is its watermarks. >*/ > if (!crtc->primary->state->visible) > - skl_write_plane_wm(intel_crtc, &dev_priv->wm.skl_results, 0); > + skl_write_plane_wm(intel_crtc, p_wm, > +&dev_priv->wm.skl_results.ddb, 0); > > I915_WRITE(PLANE_CTL(pipe, 0), 0); > I915_WRITE(PLANE_SURF(pipe, 0), 0); > @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct drm_crtc > *crtc, u32 base, > struct drm_device *dev = crtc->dev; > struct drm_i915_private *dev_priv = to_i915(dev); > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct intel_crtc_state *cstate = to_intel_crtc_state(crtc->state); > const struct skl_wm_values *wm = &dev_priv->wm.skl_results; > + const struct skl_plane_wm *p_wm = > + &cstate->wm.skl.optimal.planes[PLANE_CURSOR]; > int pipe = intel_crtc->pipe; > uint32_t cntl = 0; > > if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & drm_crtc_mask(crtc)) > - skl_write_cursor_wm(intel_crtc, wm); > + skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb); > > if (plane_state && plane_state->base.visible) { > cntl = MCURSOR_GAMMA_ENABLE; > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index d684f4f..958dc72 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/inte
[PATCH] drm: Print device information again in debugfs
I was a bit over-eager in my cleanup in commit 95c081c17f284de50eaca60d4d55643a64d39019 Author: Daniel Vetter Date: Tue Jun 21 10:54:12 2016 +0200 drm: Move master pointer from drm_minor to drm_device Noticed by Chris Wilson. Fixes: 95c081c17f28 ("drm: Move master pointer from drm_minor to drm_device") Cc: Chris Wilson Cc: Chris Wilson Cc: Daniel Vetter Cc: Emil Velikov Cc: Julia Lawall Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_info.c | 4 1 file changed, 4 deletions(-) diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c index 1df2d33d0b40..ffb2ab389d1d 100644 --- a/drivers/gpu/drm/drm_info.c +++ b/drivers/gpu/drm/drm_info.c @@ -54,9 +54,6 @@ int drm_name_info(struct seq_file *m, void *data) mutex_lock(&dev->master_mutex); master = dev->master; - if (!master) - goto out_unlock; - seq_printf(m, "%s", dev->driver->name); if (dev->dev) seq_printf(m, " dev=%s", dev_name(dev->dev)); @@ -65,7 +62,6 @@ int drm_name_info(struct seq_file *m, void *data) if (dev->unique) seq_printf(m, " unique=%s", dev->unique); seq_printf(m, "\n"); -out_unlock: mutex_unlock(&dev->master_mutex); return 0; -- 2.9.3
[PATCH] drm: Print device information again in debugfs
On Thu, Oct 13, 2016 at 10:13 AM, Daniel Vetter wrote: > I was a bit over-eager in my cleanup in > > commit 95c081c17f284de50eaca60d4d55643a64d39019 > Author: Daniel Vetter > Date: Tue Jun 21 10:54:12 2016 +0200 > > drm: Move master pointer from drm_minor to drm_device > > Noticed by Chris Wilson. > > Fixes: 95c081c17f28 ("drm: Move master pointer from drm_minor to drm_device") > Cc: Chris Wilson > Cc: Chris Wilson > Cc: Daniel Vetter > Cc: Emil Velikov > Cc: Julia Lawall > Signed-off-by: Daniel Vetter Reviewed-by: Alex Deucher > --- > drivers/gpu/drm/drm_info.c | 4 > 1 file changed, 4 deletions(-) > > diff --git a/drivers/gpu/drm/drm_info.c b/drivers/gpu/drm/drm_info.c > index 1df2d33d0b40..ffb2ab389d1d 100644 > --- a/drivers/gpu/drm/drm_info.c > +++ b/drivers/gpu/drm/drm_info.c > @@ -54,9 +54,6 @@ int drm_name_info(struct seq_file *m, void *data) > > mutex_lock(&dev->master_mutex); > master = dev->master; > - if (!master) > - goto out_unlock; > - > seq_printf(m, "%s", dev->driver->name); > if (dev->dev) > seq_printf(m, " dev=%s", dev_name(dev->dev)); > @@ -65,7 +62,6 @@ int drm_name_info(struct seq_file *m, void *data) > if (dev->unique) > seq_printf(m, " unique=%s", dev->unique); > seq_printf(m, "\n"); > -out_unlock: > mutex_unlock(&dev->master_mutex); > > return 0; > -- > 2.9.3 > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: atomic: Clarify documentation around drm_atomic_crtc_needs_modeset
On Thu, Oct 13, 2016 at 5:47 AM, Brian Starkey wrote: > Add some additional comments to more explicitly describe the meaning and > usage of the three CRTC modeset detection booleans: mode_changed, > connectors_changed and active_changed. > > Suggested-by: Daniel Vetter > Signed-off-by: Brian Starkey > --- > > Hi Daniel, > > I guess I asked for this one :-), please just check my understanding > is correct. > The whole thread was very enlightening for me with respect to those flags as well. The patch looks good to me. Acked-by: Alex Deucher > Thanks, > Brian > > drivers/gpu/drm/drm_atomic_helper.c |9 + > include/drm/drm_atomic.h| 11 ++- > include/drm/drm_crtc.h |5 + > 3 files changed, 20 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c > b/drivers/gpu/drm/drm_atomic_helper.c > index 78ea735..fb4071a 100644 > --- a/drivers/gpu/drm/drm_atomic_helper.c > +++ b/drivers/gpu/drm/drm_atomic_helper.c > @@ -458,10 +458,11 @@ mode_fixup(struct drm_atomic_state *state) > * removed from the crtc. > * crtc_state->active_changed is set when crtc_state->active changes, > * which is used for dpms. > + * See also: drm_atomic_crtc_needs_modeset() > * > * IMPORTANT: > * > - * Drivers which update ->mode_changed (e.g. in their ->atomic_check hooks > if a > + * Drivers which set ->mode_changed (e.g. in their ->atomic_check hooks if a > * plane update can't be done without a full modeset) _must_ call this > function > * afterwards after that change. It is permitted to call this function > multiple > * times for the same update, e.g. when the ->atomic_check functions depend > upon > @@ -510,9 +511,9 @@ drm_atomic_helper_check_modeset(struct drm_device *dev, > > for_each_connector_in_state(state, connector, connector_state, i) { > /* > -* This only sets crtc->mode_changed for routing changes, > -* drivers must set crtc->mode_changed themselves when > connector > -* properties need to be updated. > +* This only sets crtc->connectors_changed for routing > changes, > +* drivers must set crtc->connectors_changed themselves when > +* connector properties need to be updated. > */ > ret = update_connector_routing(state, connector, >connector_state); > diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h > index d9aff06..1ce255f 100644 > --- a/include/drm/drm_atomic.h > +++ b/include/drm/drm_atomic.h > @@ -368,8 +368,17 @@ int __must_check drm_atomic_nonblocking_commit(struct > drm_atomic_state *state); > * > * To give drivers flexibility struct &drm_crtc_state has 3 booleans to track > * whether the state CRTC changed enough to need a full modeset cycle: > - * connectors_changed, mode_changed and active_change. This helper simply > + * connectors_changed, mode_changed and active_changed. This helper simply > * combines these three to compute the overall need for a modeset for @state. > + * > + * The atomic helper code sets these booleans, but drivers can and should > + * change them appropriately to accurately represent whether a modeset is > + * really needed. In general, drivers should avoid full modesets whenever > + * possible. > + * > + * For example if the CRTC mode has changed, and the hardware is able to > enact > + * the requested mode change without going through a full modeset, the driver > + * should clear mode_changed during its ->atomic_check. > */ > static inline bool > drm_atomic_crtc_needs_modeset(struct drm_crtc_state *state) > diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h > index c4a3164..1f094d2 100644 > --- a/include/drm/drm_crtc.h > +++ b/include/drm/drm_crtc.h > @@ -116,6 +116,11 @@ struct drm_plane_helper_funcs; > * never return in a failure from the ->atomic_check callback. Userspace > assumes > * that a DPMS On will always succeed. In other words: @enable controls > resource > * assignment, @active controls the actual hardware state. > + * > + * The three booleans active_changed, connectors_changed and mode_changed are > + * intended to indicate whether a full modeset is needed, rather than > strictly > + * describing what has changed in a commit. > + * See also: drm_atomic_crtc_needs_modeset() > */ > struct drm_crtc_state { > struct drm_crtc *crtc; > -- > 1.7.9.5 > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm: Print device information again in debugfs
On Thu, Oct 13, 2016 at 04:13:44PM +0200, Daniel Vetter wrote: > I was a bit over-eager in my cleanup in > > commit 95c081c17f284de50eaca60d4d55643a64d39019 > Author: Daniel Vetter > Date: Tue Jun 21 10:54:12 2016 +0200 > > drm: Move master pointer from drm_minor to drm_device > > Noticed by Chris Wilson. > > Fixes: 95c081c17f28 ("drm: Move master pointer from drm_minor to drm_device") > Cc: Chris Wilson > Cc: Chris Wilson > Cc: Daniel Vetter > Cc: Emil Velikov > Cc: Julia Lawall > Signed-off-by: Daniel Vetter Tested-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre
[PATCH] drm/amd/powerplay: don't give up if DPM is already running
On Thu, Oct 13, 2016 at 3:45 AM, Zhu, Rex wrote: > Hi all, > > The attached patches were also for this issue. > Disable dpm when rmmod amdgpu. > > Please help to review. Patch 1: +r = adev->ip_blocks[AMD_IP_BLOCK_TYPE_SMC].funcs->hw_fini((void *)adev); +if (r) +DRM_DEBUG("hw_fini of IP block <%s> failed %d\n", +adev->ip_blocks[AMD_IP_BLOCK_TYPE_SMC].funcs->name, r); + +adev->ip_block_status[AMD_IP_BLOCK_TYPE_SMC].hw = false; You can't use AMD_IP_BLOCK_TYPE_* as index. Some chips may not have the IP block. Maybe something like the attached patch. That said, I think longer term it makes more sense to allows the SOCs to specify the order for init, fini, etc. otherwise we'll have lots of variable logic in the common code. Patch 2: +if (1 == PHM_READ_VFPF_INDIRECT_FIELD(hwmgr->device, CGS_IND_REG__SMC, FEATURE_STATUS, AVS_ON)) { +PP_ASSERT_WITH_CODE((0 == smum_send_msg_to_smc(hwmgr->smumgr, PPSMC_MSG_DisableAvfs)), +"Failed to disable AVFS!", +return -1;); +} Use a proper error code there like -EINVAL or something. With that fixed: Reviewed-by: Alex Deucher Alex > > Best Regards > Rex > > -Original Message- > From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf Of > Zhu, Rex > Sent: Wednesday, October 12, 2016 9:45 PM > To: Alex Deucher; Grazvydas Ignotas > Cc: amd-gfx list; Maling list - DRI developers > Subject: RE: [PATCH] drm/amd/powerplay: don't give up if DPM is already > running > > Hi Grazvydas and Alex, > > We needed to disable dpm when rmmod amdgpu for this issue. > > I am checking the function of disable dpm task. > > Best Regards > Rex > > -Original Message- > From: Alex Deucher [mailto:alexdeucher at gmail.com] > Sent: Wednesday, October 12, 2016 4:01 AM > To: Grazvydas Ignotas; Zhu, Rex > Cc: Maling list - DRI developers; amd-gfx list > Subject: Re: [PATCH] drm/amd/powerplay: don't give up if DPM is already > running > > +Rex to review this. > > Alex > > On Sun, Oct 9, 2016 at 3:23 PM, Grazvydas Ignotas > wrote: >> Currently the driver crashes if smu7_enable_dpm_tasks() returns early, >> which it does if DPM is already active. It seems to be better just to >> continue anyway, at least I haven't noticed any ill effects. It's also >> unclear at what state the hardware was left by the previous driver, so >> IMO it's better to always fully initialize. >> >> Way to reproduce: >> $ modprobe amdgpu >> $ rmmod amdgpu >> $ modprobe amdgpu >> ... >> DPM is already running right now, no need to enable DPM! >> ... >> failed to send message 18b ret is 0 >> BUG: unable to handle kernel paging request at ed01fc9ab21f Call >> Trace: >> smu7_set_power_state_tasks+0x499/0x1940 [amdgpu] >> phm_set_power_state+0xcb/0x120 [amdgpu] >> psm_adjust_power_state_dynamic+0x11e/0x1b0 [amdgpu] >> pem_task_adjust_power_state+0xb9/0xd0 [amdgpu] >> pem_excute_event_chain+0x7d/0xe0 [amdgpu] >> pem_handle_event_unlocked+0x49/0x60 [amdgpu] >> pem_handle_event+0xe/0x10 [amdgpu] >> pp_dpm_dispatch_tasks+0xe0/0x190 [amdgpu] >> amdgpu_pm_compute_clocks+0x10c/0xc60 [amdgpu] >> dce_v11_0_crtc_dpms+0x7d/0x150 [amdgpu] >> dce_v11_0_crtc_disable+0x90/0x4a0 [amdgpu] >> drm_helper_disable_unused_functions+0x67/0x80 [drm_kms_helper] >> amdgpu_fbdev_init+0x13e/0x170 [amdgpu] >> amdgpu_device_init+0x1aeb/0x2490 [amdgpu] >> >> Signed-off-by: Grazvydas Ignotas >> --- >> drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c >> b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c >> index f6afa6a..327030b 100644 >> --- a/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c >> +++ b/drivers/gpu/drm/amd/powerplay/hwmgr/smu7_hwmgr.c >> @@ -1166,8 +1166,8 @@ static int smu7_enable_dpm_tasks(struct pp_hwmgr >> *hwmgr) >> >> tmp_result = (!smum_is_dpm_running(hwmgr)) ? 0 : -1; >> PP_ASSERT_WITH_CODE(tmp_result == 0, >> - "DPM is already running right now, no need to enable >> DPM!", >> - return 0); >> + "DPM is already running", >> + ); >> >> if (smu7_voltage_control(hwmgr)) { >> tmp_result = smu7_enable_voltage_control(hwmgr); >> -- >> 2.7.4 >> >> ____
[PATCH] drm/msm/mdp5: handle non-fullscreen base plane case
If the bottom-most layer is not fullscreen, we need to use the BASE mixer stage for solid fill (ie. MDP5_CTL_BLEND_OP_FLAG_BORDER_OUT). The blend_setup() code pretty much handled this already, we just had to figure this out in _atomic_check() and assign the stages appropriately. Signed-off-by: Rob Clark --- TODO mdp4 might need similar treatment? drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 44 1 file changed, 27 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c index fa2be7c..e42f62d 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c @@ -223,11 +223,6 @@ static void blend_setup(struct drm_crtc *crtc) plane_cnt++; } - /* - * If there is no base layer, enable border color. - * Although it's not possbile in current blend logic, - * put it here as a reminder. - */ if (!pstates[STAGE_BASE] && plane_cnt) { ctl_blend_flags |= MDP5_CTL_BLEND_OP_FLAG_BORDER_OUT; DBG("Border Color is enabled"); @@ -365,6 +360,15 @@ static int pstate_cmp(const void *a, const void *b) return pa->state->zpos - pb->state->zpos; } +/* is there a helper for this? */ +static bool is_fullscreen(struct drm_crtc_state *cstate, + struct drm_plane_state *pstate) +{ + return (pstate->crtc_x == 0) && (pstate->crtc_y == 0) && + (pstate->crtc_w == cstate->mode.hdisplay) && + (pstate->crtc_h == cstate->mode.vdisplay); +} + static int mdp5_crtc_atomic_check(struct drm_crtc *crtc, struct drm_crtc_state *state) { @@ -375,21 +379,11 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc, struct plane_state pstates[STAGE_MAX + 1]; const struct mdp5_cfg_hw *hw_cfg; const struct drm_plane_state *pstate; - int cnt = 0, i; + int cnt = 0, base = 0, i; DBG("%s: check", mdp5_crtc->name); - /* verify that there are not too many planes attached to crtc -* and that we don't have conflicting mixer stages: -*/ - hw_cfg = mdp5_cfg_get_hw_config(mdp5_kms->cfg); drm_atomic_crtc_state_for_each_plane_state(plane, pstate, state) { - if (cnt >= (hw_cfg->lm.nb_stages)) { - dev_err(dev->dev, "too many planes!\n"); - return -EINVAL; - } - - pstates[cnt].plane = plane; pstates[cnt].state = to_mdp5_plane_state(pstate); @@ -399,8 +393,24 @@ static int mdp5_crtc_atomic_check(struct drm_crtc *crtc, /* assign a stage based on sorted zpos property */ sort(pstates, cnt, sizeof(pstates[0]), pstate_cmp, NULL); + /* if the bottom-most layer is not fullscreen, we need to use +* it for solid-color: +*/ + if (!is_fullscreen(state, &pstates[0].state->base)) + base++; + + /* verify that there are not too many planes attached to crtc +* and that we don't have conflicting mixer stages: +*/ + hw_cfg = mdp5_cfg_get_hw_config(mdp5_kms->cfg); + + if ((cnt + base) >= hw_cfg->lm.nb_stages) { + dev_err(dev->dev, "too many planes!\n"); + return -EINVAL; + } + for (i = 0; i < cnt; i++) { - pstates[i].state->stage = STAGE_BASE + i; + pstates[i].state->stage = STAGE_BASE + i + base; DBG("%s: assign pipe %s on stage=%d", mdp5_crtc->name, pipe2name(mdp5_plane_pipe(pstates[i].plane)), pstates[i].state->stage); -- 2.7.4
[RFC] drm: add atomic state printing
Sometimes we just want to see the atomic state, without getting so many other debug traces. So add a new drm.debug bit for that. Note: it would be nice to put the helpers for printing plane/crtc state next to plane/crtc state structs.. so someone adding new stuff to the state structs is more likely to remember to update the corresponding print_state() fxn. But the header include order for that doesn't really work out. Also, just including the corresponding mdp bits as an example. Dumps out something like: [drm] plane[24]: RGB0 [drm] crtc=crtc-0 [drm] fb=35 [drm] crtc-pos=[0,0, 720x400] [drm] src-pos=[0,0, 720x400] [drm] rotation=0 [drm] premultiplied=0 [drm] zpos=1 [drm] alpha=255 [drm] stage=STAGE0 [drm] mode_changed=1 [drm] pending=0 [drm] crtc[27]: crtc-0 [drm] enable=1 [drm] active=0 [drm] planes_changed=1 [drm] mode_changed=1 [drm] active_changed=0 [drm] connectors_changed=1 [drm] color_mgmt_changed=0 [drm] plane_mask=1 [drm] connector_mask=1 [drm] encoder_mask=1 [drm] mode: 0:"1920x1080" 60 148500 1920 2008 2052 2200 1080 1084 1089 1125 0x48 0x5 --- drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 3 ++ drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 28 ++ drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 ++- include/drm/drmP.h| 47 ++- 4 files changed, 80 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c index e42f62d..da84179 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c @@ -435,6 +435,9 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc, DBG("%s: event: %p", mdp5_crtc->name, crtc->state->event); + DRM_DEBUG_STATE("crtc[%u]: %s\n", crtc->base.id, crtc->name); + drm_print_crtc_state(crtc->state); + WARN_ON(mdp5_crtc->event); spin_lock_irqsave(&dev->event_lock, flags); diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h index e4b3fb3..466acbc 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h @@ -92,6 +92,22 @@ struct mdp5_plane_state { #define to_mdp5_plane_state(x) \ container_of(x, struct mdp5_plane_state, base) +static inline const char *stage2name(enum mdp_mixer_stage_id stage); + +static inline void +mdp5_print_plane_state(const struct mdp5_plane_state *state) +{ + const struct drm_plane *plane = state->base.plane; + DRM_DEBUG_STATE("plane[%u]: %s\n", plane->base.id, plane->name); + drm_print_plane_state(&state->base); + DRM_DEBUG_STATE("\tpremultiplied=%u\n", state->premultiplied); + DRM_DEBUG_STATE("\tzpos=%u\n", state->zpos); + DRM_DEBUG_STATE("\talpha=%u\n", state->alpha); + DRM_DEBUG_STATE("\tstage=%s\n", stage2name(state->stage)); + DRM_DEBUG_STATE("\tmode_changed=%u\n", state->mode_changed); + DRM_DEBUG_STATE("\tpending=%u\n", state->pending); +} + enum mdp5_intf_mode { MDP5_INTF_MODE_NONE = 0, @@ -120,6 +136,18 @@ static inline u32 mdp5_read(struct mdp5_kms *mdp5_kms, u32 reg) return msm_readl(mdp5_kms->mmio + reg); } +static inline const char *stage2name(enum mdp_mixer_stage_id stage) +{ + static const char *names[] = { +#define NAME(n) [n] = #n + NAME(STAGE_UNUSED), NAME(STAGE_BASE), + NAME(STAGE0), NAME(STAGE1), NAME(STAGE2), + NAME(STAGE3), NAME(STAGE4), NAME(STAGE6), +#undef NAME + }; + return names[stage]; +} + static inline const char *pipe2name(enum mdp5_pipe pipe) { static const char *names[] = { diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c index 432c098..df301df 100644 --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c @@ -356,6 +356,8 @@ static void mdp5_plane_atomic_update(struct drm_plane *plane, DBG("%s: update", mdp5_plane->name); + mdp5_print_plane_state(to_mdp5_plane_state(state)); + if (!plane_enabled(state)) { to_mdp5_plane_state(state)->pending = true; } else if (to_mdp5_plane_state(state)->mode_changed) { @@ -904,7 +906,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev, type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY; ret = drm_universal_plane_init(dev, plane, 0xff, &mdp5_plane_funcs, mdp5_plane->formats, mdp5_plane->nformats, -type, NULL); +type, "%s", mdp5_plane->name); if (ret) goto fail; diff --git a/include/drm/drmP.h b/include/drm/drmP.h index 28d341a..0ee0aa4 100644 --- a/include/drm/drmP.h +++ b/include/drm/drmP.h @@ -133,6 +133,7 @@ struct dma_buf_attachment; #define DRM_UT_PRIME 0x08 #define
[RFC PATCH 00/11] Introduce writeback connectors
Brian Starkey writes: > Hi Eric, > > On Tue, Oct 11, 2016 at 12:01:14PM -0700, Eric Anholt wrote: >>Brian Starkey writes: >> >>> Hi, >>> >>> This RFC series introduces a new connector type: >>> DRM_MODE_CONNECTOR_WRITEBACK >>> It is a follow-on from a previous discussion: [1] >>> >>> Writeback connectors are used to expose the memory writeback engines >>> found in some display controllers, which can write a CRTC's >>> composition result to a memory buffer. >>> This is useful e.g. for testing, screen-recording, screenshots, >>> wireless display, display cloning, memory-to-memory composition. >>> >>> Patches 1-7 include the core framework changes required, and patches >>> 8-11 implement a writeback connector for the Mali-DP writeback engine. >>> The Mali-DP patches depend on this other series: [2]. >>> >>> The connector is given the FB_ID property for the output framebuffer, >>> and two new read-only properties: PIXEL_FORMATS and >>> PIXEL_FORMATS_SIZE, which expose the supported framebuffer pixel >>> formats of the engine. >>> >>> The EDID property is not exposed for writeback connectors. >>> >>> Writeback connector usage: >>> -- >>> Due to connector routing changes being treated as "full modeset" >>> operations, any client which wishes to use a writeback connector >>> should include the connector in every modeset. The writeback will not >>> actually become active until a framebuffer is attached. >>> >>> The writeback itself is enabled by attaching a framebuffer to the >>> FB_ID property of the connector. The driver must then ensure that the >>> CRTC content of that atomic commit is written into the framebuffer. >>> >>> The writeback works in a one-shot mode with each atomic commit. This >>> prevents the same content from being written multiple times. >>> In some cases (front-buffer rendering) there might be a desire for >>> continuous operation - I think a property could be added later for >>> this kind of control. >>> >>> Writeback can be disabled by setting FB_ID to zero. >> >>I think this sounds great, and the interface is just right IMO. >> > > Thanks, glad you like it! Hopefully you're equally agreeable with the > changes Daniel has been suggesting. Haven't seen anything objectionable there. >>> Known issues: >>> - >>> * I'm not sure what "DPMS" should mean for writeback connectors. >>>It could be used to disable writeback (even when a framebuffer is >>>attached), or it could be hidden entirely (which would break the >>>legacy DPMS call for writeback connectors). >>> * With Daniel's recent re-iteration of the userspace API rules, I >>>fully expect to provide some userspace code to support this. The >>>question is what, and where? We want to use writeback for testing, >>>so perhaps some tests in igt is suitable. >>> * Documentation. Probably some portion of this cover letter needs to >>>make it into Documentation/ >>> * Synchronisation. Our hardware will finish the writeback by the next >>>vsync. I've not implemented fence support here, but it would be an >>>obvious addition. >> >>My hardware won't necessarily finish by the next vsync -- it trickles >>out at whatever rate it can find memory bandwidth to get the job done, >>and fires an interrupt when it's finished. >> > > Is it bounded? You presumably have to finish the write-out before you > can change any input buffers? Yeah, I'm not sure what it would mean to try to swap my display list while write-out was happening. Each CRTC (each of which can only support one encoder at a time) has its own display list, though, so it could avoid blocking other modesets. >>So I would like some definition for how syncing works. One answer would >>be that these flips don't trigger their pageflip events until the >>writeback is done (so I need to collect both the vsync irq and the >>writeback irq before sending). Another would be that manage an >>independent fence for the writeback fb, so that you still immediately >>know when framebuffers from the previous scanout-only frame are idle. >> > > I much prefer the sound of the explicit fence approach. > > Hopefully we can agree that a new atomic commit can't be completed > whilst there's a writeback ongoing,
[PATCH] drm: Print device information again in debugfs
On 13 October 2016 at 15:13, Daniel Vetter wrote: > I was a bit over-eager in my cleanup in > > commit 95c081c17f284de50eaca60d4d55643a64d39019 > Author: Daniel Vetter > Date: Tue Jun 21 10:54:12 2016 +0200 > > drm: Move master pointer from drm_minor to drm_device > > Noticed by Chris Wilson. > > Fixes: 95c081c17f28 ("drm: Move master pointer from drm_minor to drm_device") > Cc: Chris Wilson > Cc: Chris Wilson > Cc: Daniel Vetter > Cc: Emil Velikov > Cc: Julia Lawall > Signed-off-by: Daniel Vetter Oops ;-) Worth updating/dropping the "Called when ..." comment above the function ? How about the rest of the file ? Either way, Reviewed-by: Emil Velikov -Emil
[patch] drm/vc4: Fix a couple error codes in vc4_cl_lookup_bos()
Dan Carpenter writes: > If the allocation fails the current code returns success. If > copy_from_user() fails it returns the number of bytes remaining instead > of -EFAULT. > > Fixes: d5b1a78a772f ("drm/vc4: Add support for drawing 3D frames.") > Signed-off-by: Dan Carpenter Looks good. Waiting for rc1 to show up to put together a -next. -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 800 bytes Desc: not available URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/08f212f9/attachment.sig>
[RFC] drm: add atomic state printing
On Thu, Oct 13, 2016 at 1:16 PM, Rob Clark wrote: > Sometimes we just want to see the atomic state, without getting so many > other debug traces. So add a new drm.debug bit for that. > > Note: it would be nice to put the helpers for printing plane/crtc state > next to plane/crtc state structs.. so someone adding new stuff to the > state structs is more likely to remember to update the corresponding > print_state() fxn. But the header include order for that doesn't really > work out. > > Also, just including the corresponding mdp bits as an example. Dumps > out something like: > > [drm] plane[24]: RGB0 > [drm] crtc=crtc-0 > [drm] fb=35 > [drm] crtc-pos=[0,0, 720x400] > [drm] src-pos=[0,0, 720x400] > [drm] rotation=0 > [drm] premultiplied=0 > [drm] zpos=1 > [drm] alpha=255 > [drm] stage=STAGE0 > [drm] mode_changed=1 > [drm] pending=0 > [drm] crtc[27]: crtc-0 > [drm] enable=1 > [drm] active=0 > [drm] planes_changed=1 > [drm] mode_changed=1 > [drm] active_changed=0 > [drm] connectors_changed=1 > [drm] color_mgmt_changed=0 > [drm] plane_mask=1 > [drm] connector_mask=1 > [drm] encoder_mask=1 > [drm] mode: 0:"1920x1080" 60 148500 1920 2008 2052 2200 1080 1084 1089 1125 > 0x48 0x5 > --- > drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 3 ++ > drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 28 ++ > drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 ++- > include/drm/drmP.h| 47 > ++- > 4 files changed, 80 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > index e42f62d..da84179 100644 > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > @@ -435,6 +435,9 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc, > > DBG("%s: event: %p", mdp5_crtc->name, crtc->state->event); > > + DRM_DEBUG_STATE("crtc[%u]: %s\n", crtc->base.id, crtc->name); > + drm_print_crtc_state(crtc->state); > + > WARN_ON(mdp5_crtc->event); > > spin_lock_irqsave(&dev->event_lock, flags); > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > index e4b3fb3..466acbc 100644 > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > @@ -92,6 +92,22 @@ struct mdp5_plane_state { > #define to_mdp5_plane_state(x) \ > container_of(x, struct mdp5_plane_state, base) > > +static inline const char *stage2name(enum mdp_mixer_stage_id stage); > + > +static inline void > +mdp5_print_plane_state(const struct mdp5_plane_state *state) > +{ > + const struct drm_plane *plane = state->base.plane; > + DRM_DEBUG_STATE("plane[%u]: %s\n", plane->base.id, plane->name); > + drm_print_plane_state(&state->base); > + DRM_DEBUG_STATE("\tpremultiplied=%u\n", state->premultiplied); > + DRM_DEBUG_STATE("\tzpos=%u\n", state->zpos); > + DRM_DEBUG_STATE("\talpha=%u\n", state->alpha); > + DRM_DEBUG_STATE("\tstage=%s\n", stage2name(state->stage)); > + DRM_DEBUG_STATE("\tmode_changed=%u\n", state->mode_changed); > + DRM_DEBUG_STATE("\tpending=%u\n", state->pending); > +} > + > enum mdp5_intf_mode { > MDP5_INTF_MODE_NONE = 0, > > @@ -120,6 +136,18 @@ static inline u32 mdp5_read(struct mdp5_kms *mdp5_kms, > u32 reg) > return msm_readl(mdp5_kms->mmio + reg); > } > > +static inline const char *stage2name(enum mdp_mixer_stage_id stage) > +{ > + static const char *names[] = { > +#define NAME(n) [n] = #n > + NAME(STAGE_UNUSED), NAME(STAGE_BASE), > + NAME(STAGE0), NAME(STAGE1), NAME(STAGE2), > + NAME(STAGE3), NAME(STAGE4), NAME(STAGE6), > +#undef NAME > + }; > + return names[stage]; > +} > + > static inline const char *pipe2name(enum mdp5_pipe pipe) > { > static const char *names[] = { > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > index 432c098..df301df 100644 > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > @@ -356,6 +356,8 @@ static void mdp5_plane_atomic_update(struct drm_plane > *plane, > > DBG("%s: update", mdp5_plane->name); > > + mdp5_print_plane_state(to_mdp5_plane_state(state)); > + I feel like if we add this, we should create a helper drm_atomic_helper_print_state() (or w/e) to dump all of the state at once (on flush), instead of sprinkling calls throughout the driver. So I guess that would require the new helper call + optional helper_funcs for driver-specific crtc/plane fields. Perhaps that's too involved, but IMO it seems like something that'd be easier to gain traction across drivers. Sean > if (!plane_enabled(state)) { > to_mdp5_plane_state(state)->pending = true; > } else if (to_mdp5_plane_state(sta
[Bug 93826] 2560x1440 @144Hz graphic glitches and bad refresh rate
https://bugs.freedesktop.org/show_bug.cgi?id=93826 --- Comment #31 from Yves W. --- (In reply to Michel Dänzer from comment #29) > (In reply to Yves W. from comment #27) > > - with default driver and default kernel (Linux Mint 18 -> 4.4.0-38-generi) > > it's working at 2560x1440 @144hz, no flickering issues. > > > > -> tried using the Ubuntu 4.8.0 kernel -> flickering at 2560x1440 @144hz,120 > > and 100hz. only 60hz works without flickering > > Can you bisect, or at least narrow down which version between 4.4.0 and > 4.8.0 introduced the problem? as I said, I only tried current linux mint Kernel which is 4.4.0-38-generic right now. With that one AND the default radeon driver module for amd card R9 290 4GB, I have no problems at all. I can do 2560x1440 resolution @ 144hz. Second I upgradedm, as I posted, to the ubuntu kernel 4.8.0 from ubuntu ppa, which gives errors (strong flickering in 2d mode, 3d works at some point but also gives few flickering problems) WITH 2560x1440 resolution, for all hz above 60hz (which means flickering at 100,120 and 144) Furthermore the amd beta driver amdgpu-pro gives in every kernel I tried (4.4 and 4.8) flickering problems with refreshrates above 60hz. -> but here note: I switched ( with kernel 4.4.0-38-generic + amdgpu-pro 16.30.3-315407) to 1080p resolution and 144,120 and 100hz gave NO errors at all, which means no flickering! Dunno why. maybe an 1440p issue. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/19ee09ef/attachment.html>
[RFC] drm: add atomic state printing
On Thu, Oct 13, 2016 at 1:38 PM, Sean Paul wrote: > On Thu, Oct 13, 2016 at 1:16 PM, Rob Clark wrote: >> Sometimes we just want to see the atomic state, without getting so many >> other debug traces. So add a new drm.debug bit for that. >> >> Note: it would be nice to put the helpers for printing plane/crtc state >> next to plane/crtc state structs.. so someone adding new stuff to the >> state structs is more likely to remember to update the corresponding >> print_state() fxn. But the header include order for that doesn't really >> work out. >> >> Also, just including the corresponding mdp bits as an example. Dumps >> out something like: >> >> [drm] plane[24]: RGB0 >> [drm] crtc=crtc-0 >> [drm] fb=35 >> [drm] crtc-pos=[0,0, 720x400] >> [drm] src-pos=[0,0, 720x400] >> [drm] rotation=0 >> [drm] premultiplied=0 >> [drm] zpos=1 >> [drm] alpha=255 >> [drm] stage=STAGE0 >> [drm] mode_changed=1 >> [drm] pending=0 >> [drm] crtc[27]: crtc-0 >> [drm] enable=1 >> [drm] active=0 >> [drm] planes_changed=1 >> [drm] mode_changed=1 >> [drm] active_changed=0 >> [drm] connectors_changed=1 >> [drm] color_mgmt_changed=0 >> [drm] plane_mask=1 >> [drm] connector_mask=1 >> [drm] encoder_mask=1 >> [drm] mode: 0:"1920x1080" 60 148500 1920 2008 2052 2200 1080 1084 1089 >> 1125 0x48 0x5 >> --- >> drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 3 ++ >> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 28 ++ >> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 ++- >> include/drm/drmP.h| 47 >> ++- >> 4 files changed, 80 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c >> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c >> index e42f62d..da84179 100644 >> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c >> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c >> @@ -435,6 +435,9 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc, >> >> DBG("%s: event: %p", mdp5_crtc->name, crtc->state->event); >> >> + DRM_DEBUG_STATE("crtc[%u]: %s\n", crtc->base.id, crtc->name); >> + drm_print_crtc_state(crtc->state); >> + >> WARN_ON(mdp5_crtc->event); >> >> spin_lock_irqsave(&dev->event_lock, flags); >> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h >> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h >> index e4b3fb3..466acbc 100644 >> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h >> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h >> @@ -92,6 +92,22 @@ struct mdp5_plane_state { >> #define to_mdp5_plane_state(x) \ >> container_of(x, struct mdp5_plane_state, base) >> >> +static inline const char *stage2name(enum mdp_mixer_stage_id stage); >> + >> +static inline void >> +mdp5_print_plane_state(const struct mdp5_plane_state *state) >> +{ >> + const struct drm_plane *plane = state->base.plane; >> + DRM_DEBUG_STATE("plane[%u]: %s\n", plane->base.id, plane->name); >> + drm_print_plane_state(&state->base); >> + DRM_DEBUG_STATE("\tpremultiplied=%u\n", state->premultiplied); >> + DRM_DEBUG_STATE("\tzpos=%u\n", state->zpos); >> + DRM_DEBUG_STATE("\talpha=%u\n", state->alpha); >> + DRM_DEBUG_STATE("\tstage=%s\n", stage2name(state->stage)); >> + DRM_DEBUG_STATE("\tmode_changed=%u\n", state->mode_changed); >> + DRM_DEBUG_STATE("\tpending=%u\n", state->pending); >> +} >> + >> enum mdp5_intf_mode { >> MDP5_INTF_MODE_NONE = 0, >> >> @@ -120,6 +136,18 @@ static inline u32 mdp5_read(struct mdp5_kms *mdp5_kms, >> u32 reg) >> return msm_readl(mdp5_kms->mmio + reg); >> } >> >> +static inline const char *stage2name(enum mdp_mixer_stage_id stage) >> +{ >> + static const char *names[] = { >> +#define NAME(n) [n] = #n >> + NAME(STAGE_UNUSED), NAME(STAGE_BASE), >> + NAME(STAGE0), NAME(STAGE1), NAME(STAGE2), >> + NAME(STAGE3), NAME(STAGE4), NAME(STAGE6), >> +#undef NAME >> + }; >> + return names[stage]; >> +} >> + >> static inline const char *pipe2name(enum mdp5_pipe pipe) >> { >> static const char *names[] = { >> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c >> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c >> index 432c098..df301df 100644 >> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c >> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c >> @@ -356,6 +356,8 @@ static void mdp5_plane_atomic_update(struct drm_plane >> *plane, >> >> DBG("%s: update", mdp5_plane->name); >> >> + mdp5_print_plane_state(to_mdp5_plane_state(state)); >> + > > I feel like if we add this, we should create a helper > drm_atomic_helper_print_state() (or w/e) to dump all of the state at > once (on flush), instead of sprinkling calls throughout the driver. So > I guess that would require the new helper call + optional helper_funcs > for driver-specific crtc/plane fields. > > Perhaps that's too involved, but IMO it seems like something that'd be > easi
[Intel-gfx] drm/i915: WARN_ON_ONCE(!crtc_clock || cdclk < crtc_clock)
[Adding Matt] On Wed, 2016-10-12 at 14:08 +0300, Joonas Lahtinen wrote: > Bisecting the offending commit between v4.8 and v4.8.1 would be a good > start. 0) Why use a personal notebook when one can just post any half baked idea to lkml? 1) I stumbled on https://bugzilla.redhat.com/show_bug.cgi?id=1361357 (s ame hardware, rather similar trace). 2) That report was about the first Fedora (rawhide) kernel release after this commit http://pkgs.fedoraproject.org/cgit/rpms/kernel.git/co mmit/?h=f25&id=7d2c2f2d91793b5da452bee9bea4fa32051c8608 ("Bring in patch-series from drm-next to fix skl_update_other_pipe_wm issues"). 3) That seventeen part series ended up in v4.8. The last commit of that series is commit 5b483747a925 ("drm/i915: Remove wm_config from dev_priv/intel_atomic_state"). 4) So, with a little bit of luck, my bisect might only need to look at those seventeen commits. Still, it will probably be next week before I have a day or two to actually perform that bisect. To be continued, Paul Bolle
[RFC] drm: add atomic state printing
On Thu, Oct 13, 2016 at 01:16:29PM -0400, Rob Clark wrote: > Sometimes we just want to see the atomic state, without getting so many > other debug traces. So add a new drm.debug bit for that. > > Note: it would be nice to put the helpers for printing plane/crtc state > next to plane/crtc state structs.. so someone adding new stuff to the > state structs is more likely to remember to update the corresponding > print_state() fxn. But the header include order for that doesn't really > work out. > > Also, just including the corresponding mdp bits as an example. Dumps > out something like: > > [drm] plane[24]: RGB0 > [drm] crtc=crtc-0 > [drm] fb=35 > [drm] crtc-pos=[0,0, 720x400] > [drm] src-pos=[0,0, 720x400] > [drm] rotation=0 > [drm] premultiplied=0 > [drm] zpos=1 > [drm] alpha=255 > [drm] stage=STAGE0 > [drm] mode_changed=1 > [drm] pending=0 > [drm] crtc[27]: crtc-0 > [drm] enable=1 > [drm] active=0 > [drm] planes_changed=1 > [drm] mode_changed=1 > [drm] active_changed=0 > [drm] connectors_changed=1 > [drm] color_mgmt_changed=0 > [drm] plane_mask=1 > [drm] connector_mask=1 > [drm] encoder_mask=1 > [drm] mode: 0:"1920x1080" 60 148500 1920 2008 2052 2200 1080 1084 > 1089 1125 0x48 0x5 > --- > drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 3 ++ > drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 28 ++ > drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 ++- > include/drm/drmP.h| 47 > ++- > 4 files changed, 80 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > index e42f62d..da84179 100644 > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > @@ -435,6 +435,9 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc, > > DBG("%s: event: %p", mdp5_crtc->name, crtc->state->event); > > + DRM_DEBUG_STATE("crtc[%u]: %s\n", crtc->base.id, crtc->name); > + drm_print_crtc_state(crtc->state); > + > WARN_ON(mdp5_crtc->event); > > spin_lock_irqsave(&dev->event_lock, flags); > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > index e4b3fb3..466acbc 100644 > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > @@ -92,6 +92,22 @@ struct mdp5_plane_state { > #define to_mdp5_plane_state(x) \ > container_of(x, struct mdp5_plane_state, base) > > +static inline const char *stage2name(enum mdp_mixer_stage_id stage); > + > +static inline void > +mdp5_print_plane_state(const struct mdp5_plane_state *state) > +{ > + const struct drm_plane *plane = state->base.plane; > + DRM_DEBUG_STATE("plane[%u]: %s\n", plane->base.id, plane->name); > + drm_print_plane_state(&state->base); > + DRM_DEBUG_STATE("\tpremultiplied=%u\n", state->premultiplied); > + DRM_DEBUG_STATE("\tzpos=%u\n", state->zpos); > + DRM_DEBUG_STATE("\talpha=%u\n", state->alpha); > + DRM_DEBUG_STATE("\tstage=%s\n", stage2name(state->stage)); > + DRM_DEBUG_STATE("\tmode_changed=%u\n", state->mode_changed); > + DRM_DEBUG_STATE("\tpending=%u\n", state->pending); > +} > + > enum mdp5_intf_mode { > MDP5_INTF_MODE_NONE = 0, > > @@ -120,6 +136,18 @@ static inline u32 mdp5_read(struct mdp5_kms *mdp5_kms, > u32 reg) > return msm_readl(mdp5_kms->mmio + reg); > } > > +static inline const char *stage2name(enum mdp_mixer_stage_id stage) > +{ > + static const char *names[] = { > +#define NAME(n) [n] = #n > + NAME(STAGE_UNUSED), NAME(STAGE_BASE), > + NAME(STAGE0), NAME(STAGE1), NAME(STAGE2), > + NAME(STAGE3), NAME(STAGE4), NAME(STAGE6), > +#undef NAME > + }; > + return names[stage]; > +} > + > static inline const char *pipe2name(enum mdp5_pipe pipe) > { > static const char *names[] = { > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > index 432c098..df301df 100644 > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > @@ -356,6 +356,8 @@ static void mdp5_plane_atomic_update(struct drm_plane > *plane, > > DBG("%s: update", mdp5_plane->name); > > + mdp5_print_plane_state(to_mdp5_plane_state(state)); > + > if (!plane_enabled(state)) { > to_mdp5_plane_state(state)->pending = true; > } else if (to_mdp5_plane_state(state)->mode_changed) { > @@ -904,7 +906,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev, > type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DRM_PLANE_TYPE_OVERLAY; > ret = drm_universal_plane_init(dev, plane, 0xff, &mdp5_plane_funcs, >mdp5_plane->formats, mdp5_plane->
[RFC] drm: add atomic state printing
On Thu, Oct 13, 2016 at 01:38:30PM -0400, Sean Paul wrote: > On Thu, Oct 13, 2016 at 1:16 PM, Rob Clark wrote: > > Sometimes we just want to see the atomic state, without getting so many > > other debug traces. So add a new drm.debug bit for that. > > > > Note: it would be nice to put the helpers for printing plane/crtc state > > next to plane/crtc state structs.. so someone adding new stuff to the > > state structs is more likely to remember to update the corresponding > > print_state() fxn. But the header include order for that doesn't really > > work out. > > > > Also, just including the corresponding mdp bits as an example. Dumps > > out something like: > > > > [drm] plane[24]: RGB0 > > [drm] crtc=crtc-0 > > [drm] fb=35 > > [drm] crtc-pos=[0,0, 720x400] > > [drm] src-pos=[0,0, 720x400] > > [drm] rotation=0 > > [drm] premultiplied=0 > > [drm] zpos=1 > > [drm] alpha=255 > > [drm] stage=STAGE0 > > [drm] mode_changed=1 > > [drm] pending=0 > > [drm] crtc[27]: crtc-0 > > [drm] enable=1 > > [drm] active=0 > > [drm] planes_changed=1 > > [drm] mode_changed=1 > > [drm] active_changed=0 > > [drm] connectors_changed=1 > > [drm] color_mgmt_changed=0 > > [drm] plane_mask=1 > > [drm] connector_mask=1 > > [drm] encoder_mask=1 > > [drm] mode: 0:"1920x1080" 60 148500 1920 2008 2052 2200 1080 1084 1089 > > 1125 0x48 0x5 > > --- > > drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 3 ++ > > drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 28 ++ > > drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 ++- > > include/drm/drmP.h| 47 > > ++- > > 4 files changed, 80 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > > index e42f62d..da84179 100644 > > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c > > @@ -435,6 +435,9 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc > > *crtc, > > > > DBG("%s: event: %p", mdp5_crtc->name, crtc->state->event); > > > > + DRM_DEBUG_STATE("crtc[%u]: %s\n", crtc->base.id, crtc->name); > > + drm_print_crtc_state(crtc->state); > > + > > WARN_ON(mdp5_crtc->event); > > > > spin_lock_irqsave(&dev->event_lock, flags); > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > > index e4b3fb3..466acbc 100644 > > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h > > @@ -92,6 +92,22 @@ struct mdp5_plane_state { > > #define to_mdp5_plane_state(x) \ > > container_of(x, struct mdp5_plane_state, base) > > > > +static inline const char *stage2name(enum mdp_mixer_stage_id stage); > > + > > +static inline void > > +mdp5_print_plane_state(const struct mdp5_plane_state *state) > > +{ > > + const struct drm_plane *plane = state->base.plane; > > + DRM_DEBUG_STATE("plane[%u]: %s\n", plane->base.id, plane->name); > > + drm_print_plane_state(&state->base); > > + DRM_DEBUG_STATE("\tpremultiplied=%u\n", state->premultiplied); > > + DRM_DEBUG_STATE("\tzpos=%u\n", state->zpos); > > + DRM_DEBUG_STATE("\talpha=%u\n", state->alpha); > > + DRM_DEBUG_STATE("\tstage=%s\n", stage2name(state->stage)); > > + DRM_DEBUG_STATE("\tmode_changed=%u\n", state->mode_changed); > > + DRM_DEBUG_STATE("\tpending=%u\n", state->pending); > > +} > > + > > enum mdp5_intf_mode { > > MDP5_INTF_MODE_NONE = 0, > > > > @@ -120,6 +136,18 @@ static inline u32 mdp5_read(struct mdp5_kms *mdp5_kms, > > u32 reg) > > return msm_readl(mdp5_kms->mmio + reg); > > } > > > > +static inline const char *stage2name(enum mdp_mixer_stage_id stage) > > +{ > > + static const char *names[] = { > > +#define NAME(n) [n] = #n > > + NAME(STAGE_UNUSED), NAME(STAGE_BASE), > > + NAME(STAGE0), NAME(STAGE1), NAME(STAGE2), > > + NAME(STAGE3), NAME(STAGE4), NAME(STAGE6), > > +#undef NAME > > + }; > > + return names[stage]; > > +} > > + > > static inline const char *pipe2name(enum mdp5_pipe pipe) > > { > > static const char *names[] = { > > diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > > b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > > index 432c098..df301df 100644 > > --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > > +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c > > @@ -356,6 +356,8 @@ static void mdp5_plane_atomic_update(struct drm_plane > > *plane, > > > > DBG("%s: update", mdp5_plane->name); > > > > + mdp5_print_plane_state(to_mdp5_plane_state(state)); > > + > > I feel like if we add this, we should create a helper > drm_atomic_helper_print_state() (or w/e) to dump all of the state at > once (on flush), instead of sprinkling calls throughout the driver. So > I guess that would require the new helper call + optional h
[PATCH v2] drm/bridge: analogix: protect power when get_modes or detect
On Wed, Oct 12, 2016 at 9:50 PM, Mark Yao wrote: > The drm callback ->detect and ->get_modes seems is not power safe, > they may be called when device is power off, do register access on > detect or get_modes will cause system die. > > Here is the path call ->detect before analogix_dp power on > [] analogix_dp_detect+0x44/0xdc > [] > drm_helper_probe_single_connector_modes_merge_bits+0xe8/0x41c > [] drm_helper_probe_single_connector_modes+0x10/0x18 > [] drm_mode_getconnector+0xf4/0x304 > [] drm_ioctl+0x23c/0x390 > [] do_vfs_ioctl+0x4b8/0x58c > [] SyS_ioctl+0x60/0x88 > > Cc: Inki Dae > Cc: Sean Paul > Cc: Gustavo Padovan > Cc: "Ville Syrjälä" > > Signed-off-by: Mark Yao Thanks for revising, this is much better. Reviewed-by: Sean Paul > --- > Changes in v2: > - remove sub device power on/off callback, use pm_runtime_get/put is enough > to fix my problem, so will can avoid race to dpms. > > drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 8 > 1 file changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > index efac8ab..ff2d328 100644 > --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c > @@ -1062,11 +1062,15 @@ int analogix_dp_get_modes(struct drm_connector > *connector) > return 0; > } > > + pm_runtime_get_sync(dp->dev); > + > if (analogix_dp_handle_edid(dp) == 0) { > drm_mode_connector_update_edid_property(&dp->connector, edid); > num_modes += drm_add_edid_modes(&dp->connector, edid); > } > > + pm_runtime_put(dp->dev); > + > if (dp->plat_data->panel) > num_modes += drm_panel_get_modes(dp->plat_data->panel); > > @@ -1106,9 +1110,13 @@ analogix_dp_detect(struct drm_connector *connector, > bool force) > return connector_status_disconnected; > } > > + pm_runtime_get_sync(dp->dev); > + > if (!analogix_dp_detect_hpd(dp)) > status = connector_status_connected; > > + pm_runtime_put(dp->dev); > + > ret = analogix_dp_prepare_panel(dp, false, false); > if (ret) > DRM_ERROR("Failed to unprepare panel (%d)\n", ret); > -- > 1.9.1 > >
[PATCH] drm/edid: Only print the bad edid when aborting
Currently, if drm.debug is enabled, we get a DRM_ERROR message on the intermediate edid reads. This causes transient failures in CI which flags up the sporadic EDID read failures, which are recovered by rereading the EDID automatically. This patch combines the reporting done by drm_do_get_edid() itself with the bad block printing from get_edid_block(), into a single warning associated with the connector once all attempts to retrieve the EDID fail. References: https://bugs.freedesktop.org/show_bug.cgi?id=98228 Signed-off-by: Chris Wilson --- drivers/gpu/drm/drm_edid.c | 82 +- 1 file changed, 44 insertions(+), 38 deletions(-) diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index ec77bd3e1f08..51dd10c65b53 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -1260,6 +1260,26 @@ drm_do_probe_ddc_edid(void *data, u8 *buf, unsigned int block, size_t len) return ret == xfers ? 0 : -1; } +static void connector_add_bad_edid(struct drm_connector *connector, + u8 *block, int num) +{ + if (connector->bad_edid_counter++ && !(drm_debug & DRM_UT_KMS)) + return; + + if (drm_edid_is_zero(block, EDID_LENGTH)) { + dev_warn(connector->dev->dev, +"%s: EDID block %d is all zeroes.\n", +connector->name, num); + } else { + dev_warn(connector->dev->dev, +"%s: EDID block %d invalid:\n", +connector->name, num); + print_hex_dump(KERN_WARNING, + " \t", DUMP_PREFIX_NONE, 16, 1, + block, EDID_LENGTH, false); + } +} + /** * drm_do_get_edid - get EDID data using a custom EDID block read function * @connector: connector we're probing @@ -1282,20 +1302,19 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, void *data) { int i, j = 0, valid_extensions = 0; - u8 *block, *new; - bool print_bad_edid = !connector->bad_edid_counter || (drm_debug & DRM_UT_KMS); + u8 *edid, *new; - if ((block = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) + if ((edid = kmalloc(EDID_LENGTH, GFP_KERNEL)) == NULL) return NULL; /* base block fetch */ for (i = 0; i < 4; i++) { - if (get_edid_block(data, block, 0, EDID_LENGTH)) + if (get_edid_block(data, edid, 0, EDID_LENGTH)) goto out; - if (drm_edid_block_valid(block, 0, print_bad_edid, + if (drm_edid_block_valid(edid, 0, false, &connector->edid_corrupt)) break; - if (i == 0 && drm_edid_is_zero(block, EDID_LENGTH)) { + if (i == 0 && drm_edid_is_zero(edid, EDID_LENGTH)) { connector->null_edid_counter++; goto carp; } @@ -1304,58 +1323,45 @@ struct edid *drm_do_get_edid(struct drm_connector *connector, goto carp; /* if there's no extensions, we're done */ - if (block[0x7e] == 0) - return (struct edid *)block; + if (edid[0x7e] == 0) + return (struct edid *)edid; - new = krealloc(block, (block[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL); + new = krealloc(edid, (edid[0x7e] + 1) * EDID_LENGTH, GFP_KERNEL); if (!new) goto out; - block = new; + edid = new; + + for (j = 1; j <= edid[0x7e]; j++) { + u8 *block = edid + (valid_extensions + 1) * EDID_LENGTH; - for (j = 1; j <= block[0x7e]; j++) { for (i = 0; i < 4; i++) { - if (get_edid_block(data, - block + (valid_extensions + 1) * EDID_LENGTH, - j, EDID_LENGTH)) + if (get_edid_block(data, block, j, EDID_LENGTH)) goto out; - if (drm_edid_block_valid(block + (valid_extensions + 1) -* EDID_LENGTH, j, -print_bad_edid, -NULL)) { + if (drm_edid_block_valid(block, j, false, NULL)) { valid_extensions++; break; } } - if (i == 4 && print_bad_edid) { - dev_warn(connector->dev->dev, -"%s: Ignoring invalid EDID block %d.\n", -connector->name, j); - - connector->bad_edid_counter++; - } + if (i == 4) + connector_add_bad_edid(connector, block, j); } - if (valid_
[PATCH] dma-buf/sw_sync: fix lockdep anger
We were holding the wrong lock to be using fence_is_signaled_locked(). And holding the child_list_lock over something that could end up calling fence cb's angers lockdep: == [ INFO: possible circular locking dependency detected ] 4.7.0-rc7+ #489 Not tainted --- surfaceflinger/2034 is trying to acquire lock: (&(&array->lock)->rlock){..}, at: [] fence_signal+0x5c/0xf8 but task is already holding lock: (&(&obj->child_list_lock)->rlock){..}, at: [] sw_sync_ioctl+0x228/0x3b0 which lock already depends on the new lock. the existing dependency chain (in reverse order) is: -> #1 (&(&obj->child_list_lock)->rlock){..}: [] __lock_acquire+0x173c/0x18d8 [] lock_acquire+0x4c/0x68 [] _raw_spin_lock_irqsave+0x54/0x70 [] fence_add_callback+0x3c/0x100 [] fence_array_enable_signaling+0x80/0x170 [] fence_add_callback+0xb8/0x100 [] sync_file_poll+0xd4/0xf0 [] do_sys_poll+0x220/0x438 [] SyS_ppoll+0x1b0/0x1d8 [] el0_svc_naked+0x24/0x28 -> #0 (&(&array->lock)->rlock){..}: [] print_circular_bug+0x80/0x2e0 [] __lock_acquire+0x17c4/0x18d8 [] lock_acquire+0x4c/0x68 [] _raw_spin_lock_irqsave+0x54/0x70 [] fence_signal+0x5c/0xf8 [] fence_array_cb_func+0x78/0x88 [] fence_signal_locked+0x80/0xe0 [] sw_sync_ioctl+0x2f8/0x3b0 [] do_vfs_ioctl+0xa4/0x790 [] SyS_ioctl+0x8c/0xa0 [] el0_svc_naked+0x24/0x28 other info that might help us debug this: Possible unsafe locking scenario: CPU0CPU1 lock(&(&obj->child_list_lock)->rlock); lock(&(&array->lock)->rlock); lock(&(&obj->child_list_lock)->rlock); lock(&(&array->lock)->rlock); *** DEADLOCK *** 1 lock held by surfaceflinger/2034: #0: (&(&obj->child_list_lock)->rlock){..}, at: [] sw_sync_ioctl+0x228/0x3b0 Signed-off-by: Rob Clark --- The fence_get()/_put() might be overkill.. wasn't sure if there was any path where the ref could get dropped while child_list_lock was released.. drivers/dma-buf/sw_sync.c | 11 ++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c index 62e8e6d..3bf8f5c 100644 --- a/drivers/dma-buf/sw_sync.c +++ b/drivers/dma-buf/sw_sync.c @@ -146,8 +146,17 @@ static void sync_timeline_signal(struct sync_timeline *obj, unsigned int inc) list_for_each_entry_safe(pt, next, &obj->active_list_head, active_list) { - if (fence_is_signaled_locked(&pt->base)) + struct fence *fence = fence_get(&pt->base); + bool signaled; + + spin_unlock_irqrestore(&obj->child_list_lock, flags); + signaled = fence_is_signaled(fence); + spin_lock_irqsave(&obj->child_list_lock, flags); + + if (signaled) list_del_init(&pt->active_list); + + fence_put(fence); } spin_unlock_irqrestore(&obj->child_list_lock, flags); -- 2.7.4
[RFC] drm: add atomic state printing
On Thu, Oct 13, 2016 at 2:24 PM, Ville Syrjälä wrote: > On Thu, Oct 13, 2016 at 01:16:29PM -0400, Rob Clark wrote: >> Sometimes we just want to see the atomic state, without getting so many >> other debug traces. So add a new drm.debug bit for that. >> >> Note: it would be nice to put the helpers for printing plane/crtc state >> next to plane/crtc state structs.. so someone adding new stuff to the >> state structs is more likely to remember to update the corresponding >> print_state() fxn. But the header include order for that doesn't really >> work out. >> >> Also, just including the corresponding mdp bits as an example. Dumps >> out something like: >> >> [drm] plane[24]: RGB0 >> [drm] crtc=crtc-0 >> [drm] fb=35 >> [drm] crtc-pos=[0,0, 720x400] >> [drm] src-pos=[0,0, 720x400] >> [drm] rotation=0 >> [drm] premultiplied=0 >> [drm] zpos=1 >> [drm] alpha=255 >> [drm] stage=STAGE0 >> [drm] mode_changed=1 >> [drm] pending=0 >> [drm] crtc[27]: crtc-0 >> [drm] enable=1 >> [drm] active=0 >> [drm] planes_changed=1 >> [drm] mode_changed=1 >> [drm] active_changed=0 >> [drm] connectors_changed=1 >> [drm] color_mgmt_changed=0 >> [drm] plane_mask=1 >> [drm] connector_mask=1 >> [drm] encoder_mask=1 >> [drm] mode: 0:"1920x1080" 60 148500 1920 2008 2052 2200 1080 1084 >> 1089 1125 0x48 0x5 >> --- >> drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c | 3 ++ >> drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h | 28 ++ >> drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c | 4 ++- >> include/drm/drmP.h| 47 >> ++- >> 4 files changed, 80 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c >> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c >> index e42f62d..da84179 100644 >> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c >> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_crtc.c >> @@ -435,6 +435,9 @@ static void mdp5_crtc_atomic_flush(struct drm_crtc *crtc, >> >> DBG("%s: event: %p", mdp5_crtc->name, crtc->state->event); >> >> + DRM_DEBUG_STATE("crtc[%u]: %s\n", crtc->base.id, crtc->name); >> + drm_print_crtc_state(crtc->state); >> + >> WARN_ON(mdp5_crtc->event); >> >> spin_lock_irqsave(&dev->event_lock, flags); >> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h >> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h >> index e4b3fb3..466acbc 100644 >> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h >> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_kms.h >> @@ -92,6 +92,22 @@ struct mdp5_plane_state { >> #define to_mdp5_plane_state(x) \ >> container_of(x, struct mdp5_plane_state, base) >> >> +static inline const char *stage2name(enum mdp_mixer_stage_id stage); >> + >> +static inline void >> +mdp5_print_plane_state(const struct mdp5_plane_state *state) >> +{ >> + const struct drm_plane *plane = state->base.plane; >> + DRM_DEBUG_STATE("plane[%u]: %s\n", plane->base.id, plane->name); >> + drm_print_plane_state(&state->base); >> + DRM_DEBUG_STATE("\tpremultiplied=%u\n", state->premultiplied); >> + DRM_DEBUG_STATE("\tzpos=%u\n", state->zpos); >> + DRM_DEBUG_STATE("\talpha=%u\n", state->alpha); >> + DRM_DEBUG_STATE("\tstage=%s\n", stage2name(state->stage)); >> + DRM_DEBUG_STATE("\tmode_changed=%u\n", state->mode_changed); >> + DRM_DEBUG_STATE("\tpending=%u\n", state->pending); >> +} >> + >> enum mdp5_intf_mode { >> MDP5_INTF_MODE_NONE = 0, >> >> @@ -120,6 +136,18 @@ static inline u32 mdp5_read(struct mdp5_kms *mdp5_kms, >> u32 reg) >> return msm_readl(mdp5_kms->mmio + reg); >> } >> >> +static inline const char *stage2name(enum mdp_mixer_stage_id stage) >> +{ >> + static const char *names[] = { >> +#define NAME(n) [n] = #n >> + NAME(STAGE_UNUSED), NAME(STAGE_BASE), >> + NAME(STAGE0), NAME(STAGE1), NAME(STAGE2), >> + NAME(STAGE3), NAME(STAGE4), NAME(STAGE6), >> +#undef NAME >> + }; >> + return names[stage]; >> +} >> + >> static inline const char *pipe2name(enum mdp5_pipe pipe) >> { >> static const char *names[] = { >> diff --git a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c >> b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c >> index 432c098..df301df 100644 >> --- a/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c >> +++ b/drivers/gpu/drm/msm/mdp/mdp5/mdp5_plane.c >> @@ -356,6 +356,8 @@ static void mdp5_plane_atomic_update(struct drm_plane >> *plane, >> >> DBG("%s: update", mdp5_plane->name); >> >> + mdp5_print_plane_state(to_mdp5_plane_state(state)); >> + >> if (!plane_enabled(state)) { >> to_mdp5_plane_state(state)->pending = true; >> } else if (to_mdp5_plane_state(state)->mode_changed) { >> @@ -904,7 +906,7 @@ struct drm_plane *mdp5_plane_init(struct drm_device *dev, >> type = private_plane ? DRM_PLANE_TYPE_PRIMARY : DR
[PATCH] dma-buf/sw_sync: fix lockdep anger
On Thu, Oct 13, 2016 at 04:04:05PM -0400, Rob Clark wrote: > We were holding the wrong lock to be using fence_is_signaled_locked(). > And holding the child_list_lock over something that could end up calling > fence cb's angers lockdep: > > == > [ INFO: possible circular locking dependency detected ] > 4.7.0-rc7+ #489 Not tainted > --- > surfaceflinger/2034 is trying to acquire lock: > (&(&array->lock)->rlock){..}, at: [] > fence_signal+0x5c/0xf8 > > but task is already holding lock: > (&(&obj->child_list_lock)->rlock){..}, at: [] > sw_sync_ioctl+0x228/0x3b0 > > which lock already depends on the new lock. > > the existing dependency chain (in reverse order) is: > > -> #1 (&(&obj->child_list_lock)->rlock){..}: >[] __lock_acquire+0x173c/0x18d8 >[] lock_acquire+0x4c/0x68 >[] _raw_spin_lock_irqsave+0x54/0x70 >[] fence_add_callback+0x3c/0x100 >[] fence_array_enable_signaling+0x80/0x170 >[] fence_add_callback+0xb8/0x100 >[] sync_file_poll+0xd4/0xf0 >[] do_sys_poll+0x220/0x438 >[] SyS_ppoll+0x1b0/0x1d8 >[] el0_svc_naked+0x24/0x28 > > -> #0 (&(&array->lock)->rlock){..}: >[] print_circular_bug+0x80/0x2e0 >[] __lock_acquire+0x17c4/0x18d8 >[] lock_acquire+0x4c/0x68 >[] _raw_spin_lock_irqsave+0x54/0x70 >[] fence_signal+0x5c/0xf8 >[] fence_array_cb_func+0x78/0x88 >[] fence_signal_locked+0x80/0xe0 >[] sw_sync_ioctl+0x2f8/0x3b0 >[] do_vfs_ioctl+0xa4/0x790 >[] SyS_ioctl+0x8c/0xa0 >[] el0_svc_naked+0x24/0x28 > > other info that might help us debug this: > > Possible unsafe locking scenario: > >CPU0CPU1 > > lock(&(&obj->child_list_lock)->rlock); >lock(&(&array->lock)->rlock); >lock(&(&obj->child_list_lock)->rlock); > lock(&(&array->lock)->rlock); > > *** DEADLOCK *** > > 1 lock held by surfaceflinger/2034: > #0: (&(&obj->child_list_lock)->rlock){..}, at: [] > sw_sync_ioctl+0x228/0x3b0 > > Signed-off-by: Rob Clark > --- > The fence_get()/_put() might be overkill.. wasn't sure if there was > any path where the ref could get dropped while child_list_lock was > released.. > > drivers/dma-buf/sw_sync.c | 11 ++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c > index 62e8e6d..3bf8f5c 100644 > --- a/drivers/dma-buf/sw_sync.c > +++ b/drivers/dma-buf/sw_sync.c > @@ -146,8 +146,17 @@ static void sync_timeline_signal(struct sync_timeline > *obj, unsigned int inc) > > list_for_each_entry_safe(pt, next, &obj->active_list_head, >active_list) { > - if (fence_is_signaled_locked(&pt->base)) > + struct fence *fence = fence_get(&pt->base); > + bool signaled; > + > + spin_unlock_irqrestore(&obj->child_list_lock, flags); Hmm. The obj->child_list_lock and fence->lock are one and the same ( fence->lock = &obj->child_list_lock). The problem lockdep is complaining about appears to nesting of identical lockclasses. (The current fence_cb design allows for unbounded recursion from the signal callbacks.) Aiui, this patch shouldn't be fixing anything as the fence_signal is still fired from under the very same obj->child_list_lock. -Chris -- Chris Wilson, Intel Open Source Technology Centre
[PATCH] dma-buf/sw_sync: fix lockdep anger
On Thu, Oct 13, 2016 at 4:17 PM, Chris Wilson wrote: > On Thu, Oct 13, 2016 at 04:04:05PM -0400, Rob Clark wrote: >> We were holding the wrong lock to be using fence_is_signaled_locked(). >> And holding the child_list_lock over something that could end up calling >> fence cb's angers lockdep: >> >> == >> [ INFO: possible circular locking dependency detected ] >> 4.7.0-rc7+ #489 Not tainted >> --- >> surfaceflinger/2034 is trying to acquire lock: >> (&(&array->lock)->rlock){..}, at: [] >> fence_signal+0x5c/0xf8 >> >> but task is already holding lock: >> (&(&obj->child_list_lock)->rlock){..}, at: [] >> sw_sync_ioctl+0x228/0x3b0 >> >> which lock already depends on the new lock. >> >> the existing dependency chain (in reverse order) is: >> >> -> #1 (&(&obj->child_list_lock)->rlock){..}: >>[] __lock_acquire+0x173c/0x18d8 >>[] lock_acquire+0x4c/0x68 >>[] _raw_spin_lock_irqsave+0x54/0x70 >>[] fence_add_callback+0x3c/0x100 >>[] fence_array_enable_signaling+0x80/0x170 >>[] fence_add_callback+0xb8/0x100 >>[] sync_file_poll+0xd4/0xf0 >>[] do_sys_poll+0x220/0x438 >>[] SyS_ppoll+0x1b0/0x1d8 >>[] el0_svc_naked+0x24/0x28 >> >> -> #0 (&(&array->lock)->rlock){..}: >>[] print_circular_bug+0x80/0x2e0 >>[] __lock_acquire+0x17c4/0x18d8 >>[] lock_acquire+0x4c/0x68 >>[] _raw_spin_lock_irqsave+0x54/0x70 >>[] fence_signal+0x5c/0xf8 >>[] fence_array_cb_func+0x78/0x88 >>[] fence_signal_locked+0x80/0xe0 >>[] sw_sync_ioctl+0x2f8/0x3b0 >>[] do_vfs_ioctl+0xa4/0x790 >>[] SyS_ioctl+0x8c/0xa0 >>[] el0_svc_naked+0x24/0x28 >> >> other info that might help us debug this: >> >> Possible unsafe locking scenario: >> >>CPU0CPU1 >> >> lock(&(&obj->child_list_lock)->rlock); >>lock(&(&array->lock)->rlock); >>lock(&(&obj->child_list_lock)->rlock); >> lock(&(&array->lock)->rlock); >> >> *** DEADLOCK *** >> >> 1 lock held by surfaceflinger/2034: >> #0: (&(&obj->child_list_lock)->rlock){..}, at: [] >> sw_sync_ioctl+0x228/0x3b0 >> >> Signed-off-by: Rob Clark >> --- >> The fence_get()/_put() might be overkill.. wasn't sure if there was >> any path where the ref could get dropped while child_list_lock was >> released.. >> >> drivers/dma-buf/sw_sync.c | 11 ++- >> 1 file changed, 10 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/dma-buf/sw_sync.c b/drivers/dma-buf/sw_sync.c >> index 62e8e6d..3bf8f5c 100644 >> --- a/drivers/dma-buf/sw_sync.c >> +++ b/drivers/dma-buf/sw_sync.c >> @@ -146,8 +146,17 @@ static void sync_timeline_signal(struct sync_timeline >> *obj, unsigned int inc) >> >> list_for_each_entry_safe(pt, next, &obj->active_list_head, >>active_list) { >> - if (fence_is_signaled_locked(&pt->base)) >> + struct fence *fence = fence_get(&pt->base); >> + bool signaled; >> + >> + spin_unlock_irqrestore(&obj->child_list_lock, flags); > > Hmm. The obj->child_list_lock and fence->lock are one and the same ( > fence->lock = &obj->child_list_lock). Oh, right.. I didn't see that.. > The problem lockdep is complaining > about appears to nesting of identical lockclasses. (The current fence_cb > design allows for unbounded recursion from the signal callbacks.) > Aiui, this patch shouldn't be fixing anything as the fence_signal is > still fired from under the very same obj->child_list_lock. hmm, I didn't see the lockdep splat.. but maybe that is coming down the the order of nesting fences under fence-array.. which might be what is confusing lockdep? BR, -R > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
[Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values
Your is SAGV related, and when we don't make the SAGV happy people's machines usually hang. So I'm definitely for your patches getting merged first On Thu, 2016-10-13 at 17:07 -0300, Paulo Zanoni wrote: > Em Qui, 2016-10-13 às 17:04 -0300, Paulo Zanoni escreveu: > > > > Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu: > > > > > > > > > Op 08-10-16 om 02:11 schreef Lyude: > > > > > > > > > > > > > > > > Now that we've make skl_wm_levels make a little more sense, we > > > > can > > > > remove all of the redundant wm information. Up until now we'd > > > > been > > > > storing two copies of all of the skl watermarks: one being the > > > > skl_pipe_wm structs, the other being the global wm struct in > > > > drm_i915_private containing the raw register values. This is > > > > confusing > > > > and problematic, since it means we're prone to accidentally > > > > letting > > > > the > > > > two copies go out of sync. So, get rid of all of the functions > > > > responsible for computing the register values and just use a > > > > single > > > > helper, skl_write_wm_level(), to convert and write the new > > > > watermarks on > > > > the fly. > > > > > > > > Changes since v1: > > > > - Fixup skl_write_wm_level() > > > > - Fixup skl_wm_level_from_reg_val() > > > > - Don't forget to copy *active to intel_crtc->wm.active.skl > > > > > > > > Signed-off-by: Lyude > > > > Reviewed-by: Maarten Lankhorst > > > om > > > > > > > > > > > > > > Cc: Ville Syrjälä > > > > Cc: Paulo Zanoni > > > > --- > > > >  drivers/gpu/drm/i915/i915_drv.h      |   2 - > > > >  drivers/gpu/drm/i915/intel_display.c |  14 ++- > > > >  drivers/gpu/drm/i915/intel_drv.h     |   6 +- > > > >  drivers/gpu/drm/i915/intel_pm.c      | 204 --- > > > > -- > > > > -- > > > > > > > >  drivers/gpu/drm/i915/intel_sprite.c  |   8 +- > > > >  5 files changed, 90 insertions(+), 144 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > > b/drivers/gpu/drm/i915/i915_drv.h > > > > index 0287c93..76583b2 100644 > > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > > @@ -1644,8 +1644,6 @@ struct skl_ddb_allocation { > > > >  struct skl_wm_values { > > > >  unsigned dirty_pipes; > > > >  struct skl_ddb_allocation ddb; > > > > - uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; > > > > - uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES]; > > > >  }; > > > >  > > > >  struct skl_wm_level { > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index a71d05a..39400a0 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -3378,6 +3378,8 @@ static void > > > > skylake_update_primary_plane(struct drm_plane *plane, > > > >  struct intel_crtc *intel_crtc = > > > > to_intel_crtc(crtc_state- > > > > > > > > > > > > > > > base.crtc); > > > >  struct drm_framebuffer *fb = plane_state->base.fb; > > > >  const struct skl_wm_values *wm = &dev_priv- > > > > > > > > > > > > > > > wm.skl_results; > > > > + const struct skl_plane_wm *p_wm = > > > > + &crtc_state->wm.skl.optimal.planes[0]; > > > >  int pipe = intel_crtc->pipe; > > > >  u32 plane_ctl; > > > >  unsigned int rotation = plane_state->base.rotation; > > > > @@ -3414,7 +3416,7 @@ static void > > > > skylake_update_primary_plane(struct drm_plane *plane, > > > >  intel_crtc->adjusted_y = src_y; > > > >  > > > >  if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc- > > > > >base)) > > > > - skl_write_plane_wm(intel_crtc, wm, 0); > > > > + skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, > > > > 0); > > > >  > > > >  I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); > > > >  I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | > > > > src_x); > > > > @@ -3448,6 +3450,8 @@ static void > > > > skylake_disable_primary_plane(struct drm_plane *primary, > > > >  struct drm_device *dev = crtc->dev; > > > >  struct drm_i915_private *dev_priv = to_i915(dev); > > > >  struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > > + struct intel_crtc_state *cstate = > > > > to_intel_crtc_state(crtc->state); > > > > + const struct skl_plane_wm *p_wm = &cstate- > > > > > > > > > > > > > > > wm.skl.optimal.planes[0]; > > > >  int pipe = intel_crtc->pipe; > > > >  > > > >  /* > > > > @@ -3455,7 +3459,8 @@ static void > > > > skylake_disable_primary_plane(struct drm_plane *primary, > > > >   * plane's visiblity isn't actually changing neither > > > > is > > > > its watermarks. > > > >   */ > > > >  if (!crtc->primary->state->visible) > > > > - skl_write_plane_wm(intel_crtc, &dev_priv- > > > > > > > > > > > > > > > wm.skl_results, 0);
[Bug 94354] R9285 Unigine Valley perf regression since radeonsi: use re-Z
https://bugs.freedesktop.org/show_bug.cgi?id=94354 Andy Furniss changed: What|Removed |Added Resolution|--- |FIXED Status|NEW |RESOLVED --- Comment #2 from Andy Furniss --- Fixed https://cgit.freedesktop.org/mesa/mesa/commit/?id=e12c1cab5d571c215f65bd9f3fc1629c090ed948 -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/ef8a51ba/attachment.html>
[Bug 98238] witcher 2: objects are black when changing lod
https://bugs.freedesktop.org/show_bug.cgi?id=98238 Bug ID: 98238 Summary: witcher 2: objects are black when changing lod Product: Mesa Version: git Hardware: Other OS: Linux (All) Status: NEW Severity: normal Priority: medium Component: Drivers/Gallium/radeonsi Assignee: dri-devel at lists.freedesktop.org Reporter: aaalmosss at gmail.com QA Contact: dri-devel at lists.freedesktop.org When walking around basically everything changes level of detail all the time, and when they do they are black for a few frames. This looks very nasty. I guess the texture upload is not as fast as the game expects. Or maybe the fade transition between the lods is bugged somehow? I tried to capture an apitrace, but when replaying glretrace only shows uninitialized window content. HW: R9 270x -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/fc9e5750/attachment.html>
[Bug 98239] saints row 3: performance is limited by flushes
https://bugs.freedesktop.org/show_bug.cgi?id=98239 Bug ID: 98239 Summary: saints row 3: performance is limited by flushes Product: Mesa Version: git Hardware: Other OS: All Status: NEW Severity: normal Priority: medium Component: Drivers/Gallium/radeonsi Assignee: dri-devel at lists.freedesktop.org Reporter: aaalmosss at gmail.com QA Contact: dri-devel at lists.freedesktop.org Created attachment 127280 --> https://bugs.freedesktop.org/attachment.cgi?id=127280&action=edit sr3 flush.png Performance is quite poor, while the gpu load is very low. The attached screenshot shows that gpu load is proportional to fps, and their pattern is the opposite of the amount of flushes. In typical scenes fps can barely reach 30 while gpu load is around 40% and cpu load is at 60% all the time. I turned off all special effects for this screenshot to show that this only depends on the scene complexity. In fact turning on all the effects don't affect performance much, because the gpu and the cpu are barely loaded. -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/a5b9a8df/attachment.html>
[Bug 92248] [KBL/SKL/BYT/BXT] igt/kms_plane_scaling fail
https://bugs.freedesktop.org/show_bug.cgi?id=92248 --- Comment #29 from Luis Botello --- Software Config: = Kernel: commit f35ed31aea66b3230c366fcba5f3456ae2cb956e Author: Jani Nikula Date: Mon Oct 10 14:29:09 2016 +0300 drm-intel-nightly: 2016y-10m-10d-11h-28m-51s UTC integration manifest drm: tag: libdrm-2.4.71 commit: a44c9c31b7b38b3eedf3d26648f9e68dcc377c4c mesa: tag: mesa-12.0.0 commit: 8b06176f310f65628ce136b90a99005278ba5e0d cairo: tag: 1.15.2 commit: db8a7f1697c49ae4942d2aa49eed52dd73dd9c7a xorg-server-macros: tag: util-macros-1.19.0-2-gd7acec2 commit: d7acec2d3a3abe79814ceb72e2c0d4d95ed31d37 xserver: tag: xorg-server-1.18.99.901-76-g97a8353 commit: 97a8353ec1192d8d3bd2ebb99e5687cb91427e09 xf86-video-intel: tag: 2.99.917-712-g696f58f commit: 696f58f69f2bac5717d19f7a1a2278fee50a083e libva: tag: libva-1.7.2-38-g3b7e499 commit: 3b7e450a04fabd42edbead8c2f24c6cdf3cf vaapi-intel-driver: tag: 1.7.2-133-gdd73514 commit: dd73514209d7942f2d8c8b0bbb541fe6884ea1bc Hardware Config: Platform: BXT-P Motherboard model : Broxton P Motherboard type: NOTEBOOK Hand Held Motherboard manufacturer: Intel Corp. CPU family : Other CPU information : 06/5c GPU Card: Intel Corporation Device 5a84 (rev 0a) (prog-if 00 [VGA controller]) -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161013/6b9ea255/attachment.html>
[Intel-gfx] [PATCH 09/10] drm/i915/gen9: Actually verify WM levels in verify_wm_state()
Em Qui, 2016-10-13 às 18:15 -0300, Paulo Zanoni escreveu: > Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu: > > > > Thanks to Paulo Zanoni for indirectly pointing this out. > > > > Looks like we never actually added any code for checking whether or > > not > > we actually wrote watermark levels properly. Let's fix that. > > Thanks for doing this! > > Reviewed-by: Paulo Zanoni > > A check that would have prevented one of the bugs I solved would be > "if > plane is active, then level 0 must be enabled, and DDB partitioning > size must be non-zero". I'll put this in my TODO list, but I won't > complain if somebody does it first :) Also, bikeshed: use %u instead of %d for the unsigned types (plane_res_b, plane_res_l). > > > > > > > Signed-off-by: Lyude > > Cc: Maarten Lankhorst > > Cc: Ville Syrjälä > > Cc: Paulo Zanoni > > --- > >  drivers/gpu/drm/i915/intel_display.c | 100 > > +-- > >  1 file changed, 84 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 39400a0..2c682bc 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -13444,30 +13444,66 @@ static void verify_wm_state(struct > > drm_crtc > > *crtc, > >  struct drm_device *dev = crtc->dev; > >  struct drm_i915_private *dev_priv = to_i915(dev); > >  struct skl_ddb_allocation hw_ddb, *sw_ddb; > > - struct skl_ddb_entry *hw_entry, *sw_entry; > > + struct skl_pipe_wm hw_wm, *sw_wm; > > + struct skl_plane_wm *hw_plane_wm, *sw_plane_wm; > > + struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry; > >  struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > >  const enum pipe pipe = intel_crtc->pipe; > > - int plane; > > + int plane, level, max_level = ilk_wm_max_level(dev); > >  > >  if (INTEL_INFO(dev)->gen < 9 || !new_state->active) > >  return; > >  > > + skl_pipe_wm_get_hw_state(crtc, &hw_wm); > > + sw_wm = &intel_crtc->wm.active.skl; > > + > >  skl_ddb_get_hw_state(dev_priv, &hw_ddb); > >  sw_ddb = &dev_priv->wm.skl_hw.ddb; > >  > >  /* planes */ > >  for_each_plane(dev_priv, pipe, plane) { > > - hw_entry = &hw_ddb.plane[pipe][plane]; > > - sw_entry = &sw_ddb->plane[pipe][plane]; > > + hw_plane_wm = &hw_wm.planes[plane]; > > + sw_plane_wm = &sw_wm->planes[plane]; > >  > > - if (skl_ddb_entry_equal(hw_entry, sw_entry)) > > - continue; > > + /* Watermarks */ > > + for (level = 0; level <= max_level; level++) { > > + if (skl_wm_level_equals(&hw_plane_wm- > > > > > > wm[level], > > + &sw_plane_wm- > > > > > > wm[level])) > > + continue; > > + > > + DRM_ERROR("mismatch in WM pipe %c plane %d > > level %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n", > > +   pipe_name(pipe), plane + 1, > > level, > > +   sw_plane_wm->wm[level].plane_en, > > +   sw_plane_wm- > > > > > > wm[level].plane_res_b, > > +   sw_plane_wm- > > > > > > wm[level].plane_res_l, > > +   hw_plane_wm->wm[level].plane_en, > > +   hw_plane_wm- > > > > > > wm[level].plane_res_b, > > +   hw_plane_wm- > > > > > > wm[level].plane_res_l); > > + } > >  > > - DRM_ERROR("mismatch in DDB state pipe %c plane %d > > " > > -   "(expected (%u,%u), found (%u,%u))\n", > > -   pipe_name(pipe), plane + 1, > > -   sw_entry->start, sw_entry->end, > > -   hw_entry->start, hw_entry->end); > > + if (!skl_wm_level_equals(&hw_plane_wm->trans_wm, > > +  &sw_plane_wm->trans_wm)) > > { > > + DRM_ERROR("mismatch in trans WM pipe %c > > plane %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n", > > +   pipe_name(pipe), plane + 1, > > +   sw_plane_wm->trans_wm.plane_en, > > +   sw_plane_wm- > > >trans_wm.plane_res_b, > > +   sw_plane_wm- > > >trans_wm.plane_res_l, > > +   hw_plane_wm->trans_wm.plane_en, > > +   hw_plane_wm- > > >trans_wm.plane_res_b, > > +   hw_plane_wm- > > > > > > trans_wm.plane_res_l); > > + } > > + > > + /* DDB */ > > + hw_ddb_entry = &hw_ddb.plane[pipe][plane]; > > + sw_ddb_entry = &sw_ddb->plane[pipe][plane]; > > + > > + if (!skl_ddb_entry_equal(hw_ddb_entry, > > sw_ddb_entry)) { > > + DRM_ERROR("mismatch in DDB state pipe %c > > plan
[PATCH 10/10] drm/i915/gen9: Don't wrap strings in verify_wm_state()
Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu: Bikesheding: it would be nice to write a commit message explaining why, even if the message just tells the user to read Documentation/CodingStyle. Reviewed-by: Paulo Zanoni > Signed-off-by: Lyude > Cc: Maarten Lankhorst > Cc: Ville Syrjälä > Cc: Paulo Zanoni > --- >  drivers/gpu/drm/i915/intel_display.c | 6 ++ >  1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 2c682bc..6191baf 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13498,8 +13498,7 @@ static void verify_wm_state(struct drm_crtc > *crtc, >  sw_ddb_entry = &sw_ddb->plane[pipe][plane]; >  >  if (!skl_ddb_entry_equal(hw_ddb_entry, > sw_ddb_entry)) { > - DRM_ERROR("mismatch in DDB state pipe %c > plane %d " > -   "(expected (%u,%u), found > (%u,%u))\n", > + DRM_ERROR("mismatch in DDB state pipe %c > plane %d (expected (%u,%u), found (%u,%u))\n", >    pipe_name(pipe), plane + 1, >    sw_ddb_entry->start, sw_ddb_entry- > >end, >    hw_ddb_entry->start, hw_ddb_entry- > >end); > @@ -13549,8 +13548,7 @@ static void verify_wm_state(struct drm_crtc > *crtc, >  sw_ddb_entry = &sw_ddb->plane[pipe][PLANE_CURSOR]; >  >  if (!skl_ddb_entry_equal(hw_ddb_entry, > sw_ddb_entry)) { > - DRM_ERROR("mismatch in DDB state pipe %c > cursor " > -   "(expected (%u,%u), found > (%u,%u))\n", > + DRM_ERROR("mismatch in DDB state pipe %c > cursor (expected (%u,%u), found (%u,%u))\n", >    pipe_name(pipe), >    sw_ddb_entry->start, sw_ddb_entry- > >end, >    hw_ddb_entry->start, hw_ddb_entry- > >end);
[Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values
Em Qui, 2016-10-13 às 17:04 -0300, Paulo Zanoni escreveu: > Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu: > > > > Op 08-10-16 om 02:11 schreef Lyude: > > > > > > > > > Now that we've make skl_wm_levels make a little more sense, we > > > can > > > remove all of the redundant wm information. Up until now we'd > > > been > > > storing two copies of all of the skl watermarks: one being the > > > skl_pipe_wm structs, the other being the global wm struct in > > > drm_i915_private containing the raw register values. This is > > > confusing > > > and problematic, since it means we're prone to accidentally > > > letting > > > the > > > two copies go out of sync. So, get rid of all of the functions > > > responsible for computing the register values and just use a > > > single > > > helper, skl_write_wm_level(), to convert and write the new > > > watermarks on > > > the fly. > > > > > > Changes since v1: > > > - Fixup skl_write_wm_level() > > > - Fixup skl_wm_level_from_reg_val() > > > - Don't forget to copy *active to intel_crtc->wm.active.skl > > > > > > Signed-off-by: Lyude > > > Reviewed-by: Maarten Lankhorst > > > > > > Cc: Ville Syrjälä > > > Cc: Paulo Zanoni > > > --- > > >  drivers/gpu/drm/i915/i915_drv.h      |   2 - > > >  drivers/gpu/drm/i915/intel_display.c |  14 ++- > > >  drivers/gpu/drm/i915/intel_drv.h     |   6 +- > > >  drivers/gpu/drm/i915/intel_pm.c      | 204 - > > > -- > > > > > >  drivers/gpu/drm/i915/intel_sprite.c  |   8 +- > > >  5 files changed, 90 insertions(+), 144 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > > b/drivers/gpu/drm/i915/i915_drv.h > > > index 0287c93..76583b2 100644 > > > --- a/drivers/gpu/drm/i915/i915_drv.h > > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > > @@ -1644,8 +1644,6 @@ struct skl_ddb_allocation { > > >  struct skl_wm_values { > > >  unsigned dirty_pipes; > > >  struct skl_ddb_allocation ddb; > > > - uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; > > > - uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES]; > > >  }; > > >  > > >  struct skl_wm_level { > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > b/drivers/gpu/drm/i915/intel_display.c > > > index a71d05a..39400a0 100644 > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > @@ -3378,6 +3378,8 @@ static void > > > skylake_update_primary_plane(struct drm_plane *plane, > > >  struct intel_crtc *intel_crtc = > > > to_intel_crtc(crtc_state- > > > > > > > > base.crtc); > > >  struct drm_framebuffer *fb = plane_state->base.fb; > > >  const struct skl_wm_values *wm = &dev_priv- > > > > > > > > wm.skl_results; > > > + const struct skl_plane_wm *p_wm = > > > + &crtc_state->wm.skl.optimal.planes[0]; > > >  int pipe = intel_crtc->pipe; > > >  u32 plane_ctl; > > >  unsigned int rotation = plane_state->base.rotation; > > > @@ -3414,7 +3416,7 @@ static void > > > skylake_update_primary_plane(struct drm_plane *plane, > > >  intel_crtc->adjusted_y = src_y; > > >  > > >  if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base)) > > > - skl_write_plane_wm(intel_crtc, wm, 0); > > > + skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, > > > 0); > > >  > > >  I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); > > >  I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | > > > src_x); > > > @@ -3448,6 +3450,8 @@ static void > > > skylake_disable_primary_plane(struct drm_plane *primary, > > >  struct drm_device *dev = crtc->dev; > > >  struct drm_i915_private *dev_priv = to_i915(dev); > > >  struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > + struct intel_crtc_state *cstate = > > > to_intel_crtc_state(crtc->state); > > > + const struct skl_plane_wm *p_wm = &cstate- > > > > > > > > wm.skl.optimal.planes[0]; > > >  int pipe = intel_crtc->pipe; > > >  > > >  /* > > > @@ -3455,7 +3459,8 @@ static void > > > skylake_disable_primary_plane(struct drm_plane *primary, > > >   * plane's visiblity isn't actually changing neither is > > > its watermarks. > > >   */ > > >  if (!crtc->primary->state->visible) > > > - skl_write_plane_wm(intel_crtc, &dev_priv- > > > > > > > > wm.skl_results, 0); > > > + skl_write_plane_wm(intel_crtc, p_wm, > > > +    &dev_priv- > > > >wm.skl_results.ddb, > > > 0); > > >  > > >  I915_WRITE(PLANE_CTL(pipe, 0), 0); > > >  I915_WRITE(PLANE_SURF(pipe, 0), 0); > > > @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct > > > drm_crtc *crtc, u32 base, > > >  struct drm_device *dev = crtc->dev; > > >  struct drm_i915_private *dev_priv = to_i915(dev); > > >  struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > + struct i
[Intel-gfx] [PATCH v2 1/4] drm: add picture aspect ratio flags
On Tue, Aug 09, 2016 at 08:25:47PM +0530, Shashank Sharma wrote: > This patch adds drm flag bits for aspect ratio information > > Currently drm flag bits don't have field for mode's picture > aspect ratio. This field will help the driver to pick mode with > right aspect ratio, and help in setting right VIC field in avi > infoframes. > > Signed-off-by: Shashank Sharma The changes make sense according to the relevant specs, and although I don't have the HW to test them the code looks right. Reviewed-by: Jim Bride > > V2: Addressed review comments from Sean > - Changed PAR-> PIC_AR > > Cc: Daniel Vetter > Cc: Emil Velikov > --- > include/uapi/drm/drm_mode.h | 18 +- > 1 file changed, 13 insertions(+), 5 deletions(-) > > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 49a7265..77c869d6 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -77,6 +77,19 @@ extern "C" { > #define DRM_MODE_FLAG_3D_TOP_AND_BOTTOM (7<<14) > #define DRM_MODE_FLAG_3D_SIDE_BY_SIDE_HALF (8<<14) > > +/* Picture aspect ratio options */ > +#define DRM_MODE_PICTURE_ASPECT_NONE 0 > +#define DRM_MODE_PICTURE_ASPECT_4_3 1 > +#define DRM_MODE_PICTURE_ASPECT_16_9 2 > + > +/* Aspect ratio flag bitmask (4 bits 22:19) */ > +#define DRM_MODE_FLAG_PIC_AR_MASK(0x0F<<19) > +#define DRM_MODE_FLAG_PIC_AR_NONE \ > + (DRM_MODE_PICTURE_ASPECT_NONE<<19) > +#define DRM_MODE_FLAG_PIC_AR_4_3 \ > + (DRM_MODE_PICTURE_ASPECT_4_3<<19) > +#define DRM_MODE_FLAG_PIC_AR_16_9 \ > + (DRM_MODE_PICTURE_ASPECT_16_9<<19) > > /* DPMS flags */ > /* bit compatible with the xorg definitions. */ > @@ -92,11 +105,6 @@ extern "C" { > #define DRM_MODE_SCALE_CENTER2 /* Centered, no scaling */ > #define DRM_MODE_SCALE_ASPECT3 /* Full screen, preserve > aspect */ > > -/* Picture aspect ratio options */ > -#define DRM_MODE_PICTURE_ASPECT_NONE 0 > -#define DRM_MODE_PICTURE_ASPECT_4_3 1 > -#define DRM_MODE_PICTURE_ASPECT_16_9 2 > - > /* Dithering mode options */ > #define DRM_MODE_DITHERING_OFF 0 > #define DRM_MODE_DITHERING_ON1 > -- > 1.9.1 > > ___ > Intel-gfx mailing list > Intel-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[PATCH 08/10] drm/i915/gen9: Add skl_wm_level_equals()
Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu: > Helper we're going to be using for implementing verification of the > wm > levels in skl_verify_wm_level(). > > Signed-off-by: Lyude Reviewed-by: Paulo Zanoni > Cc: Maarten Lankhorst > Cc: Ville Syrjälä > Cc: Paulo Zanoni > --- >  drivers/gpu/drm/i915/intel_drv.h |  2 ++ >  drivers/gpu/drm/i915/intel_pm.c  | 14 ++ >  2 files changed, 16 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 73a2d16d..3e6e9af 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1761,6 +1761,8 @@ void skl_pipe_wm_get_hw_state(struct drm_crtc > *crtc, >  bool intel_can_enable_sagv(struct drm_atomic_state *state); >  int intel_enable_sagv(struct drm_i915_private *dev_priv); >  int intel_disable_sagv(struct drm_i915_private *dev_priv); > +bool skl_wm_level_equals(const struct skl_wm_level *l1, > +  const struct skl_wm_level *l2); >  bool skl_ddb_allocation_equals(const struct skl_ddb_allocation *old, >         const struct skl_ddb_allocation *new, >         enum pipe pipe); > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index 27a520ce..6af1587 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -3853,6 +3853,20 @@ void skl_write_cursor_wm(struct intel_crtc > *intel_crtc, >      &ddb->plane[pipe][PLANE_CURSOR]); >  } >  > +bool skl_wm_level_equals(const struct skl_wm_level *l1, > +  const struct skl_wm_level *l2) > +{ > + if (l1->plane_en != l2->plane_en) > + return false; > + > + /* If both planes aren't enabled, the rest shouldn't matter > */ > + if (!l1->plane_en) > + return true; > + > + return (l1->plane_res_l == l2->plane_res_l && > + l1->plane_res_b == l2->plane_res_b); > +} > + >  static inline bool skl_ddb_entries_overlap(const struct > skl_ddb_entry *a, >     const struct > skl_ddb_entry *b) >  {
[PATCH v2 06/10] drm/i915/gen9: Add ddb changes to atomic debug output
Em Sex, 2016-10-07 à s 20:11 -0400, Lyude escreveu: > Finally, add some debugging output for ddb changes in the atomic > debug > output. This makes it a lot easier to spot bugs from incorrect ddb > allocations. > > Signed-off-by: Lyude > Reviewed-by: Maarten Lankhorst > Cc: Ville Syrjälä > Cc: Paulo Zanoni > --- >  drivers/gpu/drm/i915/intel_pm.c | 57 > + >  1 file changed, 57 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index 5cb537c..9e53ff7 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4040,6 +4040,61 @@ skl_copy_wm_for_pipe(struct skl_wm_values > *dst, >         sizeof(dst->ddb.plane[pipe])); >  } >  > +static void > +skl_print_wm_changes(const struct drm_atomic_state *state) > +{ > + const struct drm_device *dev = state->dev; > + const struct drm_i915_private *dev_priv = to_i915(dev); > + const struct intel_atomic_state *intel_state = > + to_intel_atomic_state(state); > + const struct drm_crtc *crtc; > + const struct drm_crtc_state *cstate; > + const struct drm_plane *plane; > + const struct intel_plane *intel_plane; > + const struct drm_plane_state *pstate; > + const struct skl_ddb_allocation *old_ddb = &dev_priv- > >wm.skl_hw.ddb; > + const struct skl_ddb_allocation *new_ddb = &intel_state- > >wm_results.ddb; > + enum pipe pipe; > + int id; > + int i, j; > + > + for_each_crtc_in_state(state, crtc, cstate, i) { > + if (!crtc->state) > + continue; Why exactly do we have this check? Everything else looks good. So with either an explanation or the check removed in case it's not needed: Reviewed-by: Paulo Zanoni > + > + pipe = to_intel_crtc(crtc)->pipe; > + > + for_each_plane_in_state(state, plane, pstate, j) { > + const struct skl_ddb_entry *old, *new; > + > + intel_plane = to_intel_plane(plane); > + id = skl_wm_plane_id(intel_plane); > + old = &old_ddb->plane[pipe][id]; > + new = &new_ddb->plane[pipe][id]; > + > + if (intel_plane->pipe != pipe) > + continue; > + > + if (skl_ddb_entry_equal(old, new)) > + continue; > + > + if (id != PLANE_CURSOR) { > + DRM_DEBUG_ATOMIC("[PLANE:%d:plane > %d%c] ddb (%d - %d) -> (%d - %d)\n", > +  plane->base.id, id > + 1, > +  pipe_name(pipe), > +  old->start, old- > >end, > +  new->start, new- > >end); > + } else { > + DRM_DEBUG_ATOMIC("[PLANE:%d:cursor > %c] ddb (%d - %d) -> (%d - %d)\n", > +  plane->base.id, > +  pipe_name(pipe), > +  old->start, old- > >end, > +  new->start, new- > >end); > + } > + } > + } > +} > + >  static int >  skl_compute_wm(struct drm_atomic_state *state) >  { > @@ -4101,6 +4156,8 @@ skl_compute_wm(struct drm_atomic_state *state) >  intel_cstate->update_wm_pre = true; >  } >  > + skl_print_wm_changes(state); > + >  return 0; >  } > Â
[PATCH 07/10] drm/i915/gen9: Make skl_pipe_wm_get_hw_state() reusable
Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu: > There's not much of a reason this should have the locations to read > out > the hardware state hardcoded, so allow the caller to specify the > location and add this function to intel_drv.h. As well, we're going > to > need this function to be reusable for the next patch. > > Signed-off-by: Lyude > Cc: Maarten Lankhorst > Cc: Ville Syrjälä > Cc: Paulo Zanoni > --- >  drivers/gpu/drm/i915/intel_drv.h |  2 ++ >  drivers/gpu/drm/i915/intel_pm.c  | 27 +-- >  2 files changed, 19 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 958dc72..73a2d16d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1756,6 +1756,8 @@ void ilk_wm_get_hw_state(struct drm_device > *dev); >  void skl_wm_get_hw_state(struct drm_device *dev); >  void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv, >    struct skl_ddb_allocation *ddb /* out */); > +void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, > +       struct skl_pipe_wm *out); >  bool intel_can_enable_sagv(struct drm_atomic_state *state); >  int intel_enable_sagv(struct drm_i915_private *dev_priv); >  int intel_disable_sagv(struct drm_i915_private *dev_priv); > diff --git a/drivers/gpu/drm/i915/intel_pm.c > b/drivers/gpu/drm/i915/intel_pm.c > index 9e53ff7..27a520ce 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -4287,15 +4287,13 @@ static inline void > skl_wm_level_from_reg_val(uint32_t val, >  PLANE_WM_LINES_MASK; >  } >  > -static void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc) > +void skl_pipe_wm_get_hw_state(struct drm_crtc *crtc, > +       struct skl_pipe_wm *out) >  { >  struct drm_device *dev = crtc->dev; >  struct drm_i915_private *dev_priv = to_i915(dev); > - struct skl_wm_values *hw = &dev_priv->wm.skl_hw; >  struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > - struct intel_crtc_state *cstate = to_intel_crtc_state(crtc- > >state); >  struct intel_plane *intel_plane; > - struct skl_pipe_wm *active = &cstate->wm.skl.optimal; >  struct skl_plane_wm *wm; >  enum pipe pipe = intel_crtc->pipe; >  int level, id, max_level = ilk_wm_max_level(dev); > @@ -4303,7 +4301,7 @@ static void skl_pipe_wm_get_hw_state(struct > drm_crtc *crtc) >  >  for_each_intel_plane_on_crtc(dev, intel_crtc, intel_plane) { >  id = skl_wm_plane_id(intel_plane); > - wm = &cstate->wm.skl.optimal.planes[id]; > + wm = &out->planes[id]; >  >  for (level = 0; level <= max_level; level++) { >  if (id != PLANE_CURSOR) > @@ -4325,20 +4323,29 @@ static void skl_pipe_wm_get_hw_state(struct > drm_crtc *crtc) >  if (!intel_crtc->active) >  return; >  > - hw->dirty_pipes |= drm_crtc_mask(crtc); > - active->linetime = I915_READ(PIPE_WM_LINETIME(pipe)); > - intel_crtc->wm.active.skl = *active; > + out->linetime = I915_READ(PIPE_WM_LINETIME(pipe)); >  } >  >  void skl_wm_get_hw_state(struct drm_device *dev) >  { >  struct drm_i915_private *dev_priv = to_i915(dev); > + struct skl_wm_values *hw = &dev_priv->wm.skl_hw; >  struct skl_ddb_allocation *ddb = &dev_priv->wm.skl_hw.ddb; >  struct drm_crtc *crtc; > + struct intel_crtc *intel_crtc; > + struct intel_crtc_state *cstate; >  >  skl_ddb_get_hw_state(dev_priv, ddb); > - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) > - skl_pipe_wm_get_hw_state(crtc); > + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) > { > + intel_crtc = to_intel_crtc(crtc); > + cstate = to_intel_crtc_state(crtc->state); > + > + skl_pipe_wm_get_hw_state(crtc, &cstate- > >wm.skl.optimal); > + intel_crtc->wm.active.skl = cstate->wm.skl.optimal; We're changing how the code behaves regarding intel_crtc- >wm.active.skl. Previously we would only set it if intel_crtc->active is true due to that return in skl_pipe_wm_get_hw_state(). Now we're always setting it. If this is some sort of fix it probably deserves to be in a separate commit with a nice commit message. > + > + if (!intel_crtc->active) > + hw->dirty_pipes |= drm_crtc_mask(crtc); Same here: previously we would not set dirty_pipes in case !intel_crtc- >active. Now we're doing the opposite. Didn't you mean "if (intel_crtc- >active)" here? > + } >  >  if (dev_priv->active_crtcs) { >  /* Fully recompute DDB on first atomic commit */
[Intel-gfx] [PATCH v2 05/10] drm/i915/gen9: Get rid of redundant watermark values
Em Qui, 2016-10-13 às 15:39 +0200, Maarten Lankhorst escreveu: > Op 08-10-16 om 02:11 schreef Lyude: > > > > Now that we've make skl_wm_levels make a little more sense, we can > > remove all of the redundant wm information. Up until now we'd been > > storing two copies of all of the skl watermarks: one being the > > skl_pipe_wm structs, the other being the global wm struct in > > drm_i915_private containing the raw register values. This is > > confusing > > and problematic, since it means we're prone to accidentally letting > > the > > two copies go out of sync. So, get rid of all of the functions > > responsible for computing the register values and just use a single > > helper, skl_write_wm_level(), to convert and write the new > > watermarks on > > the fly. > > > > Changes since v1: > > - Fixup skl_write_wm_level() > > - Fixup skl_wm_level_from_reg_val() > > - Don't forget to copy *active to intel_crtc->wm.active.skl > > > > Signed-off-by: Lyude > > Reviewed-by: Maarten Lankhorst > > Cc: Ville Syrjälä > > Cc: Paulo Zanoni > > --- > >  drivers/gpu/drm/i915/i915_drv.h      |   2 - > >  drivers/gpu/drm/i915/intel_display.c |  14 ++- > >  drivers/gpu/drm/i915/intel_drv.h     |   6 +- > >  drivers/gpu/drm/i915/intel_pm.c      | 204 --- > > > >  drivers/gpu/drm/i915/intel_sprite.c  |   8 +- > >  5 files changed, 90 insertions(+), 144 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 0287c93..76583b2 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1644,8 +1644,6 @@ struct skl_ddb_allocation { > >  struct skl_wm_values { > >  unsigned dirty_pipes; > >  struct skl_ddb_allocation ddb; > > - uint32_t plane[I915_MAX_PIPES][I915_MAX_PLANES][8]; > > - uint32_t plane_trans[I915_MAX_PIPES][I915_MAX_PLANES]; > >  }; > >  > >  struct skl_wm_level { > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index a71d05a..39400a0 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -3378,6 +3378,8 @@ static void > > skylake_update_primary_plane(struct drm_plane *plane, > >  struct intel_crtc *intel_crtc = to_intel_crtc(crtc_state- > > >base.crtc); > >  struct drm_framebuffer *fb = plane_state->base.fb; > >  const struct skl_wm_values *wm = &dev_priv- > > >wm.skl_results; > > + const struct skl_plane_wm *p_wm = > > + &crtc_state->wm.skl.optimal.planes[0]; > >  int pipe = intel_crtc->pipe; > >  u32 plane_ctl; > >  unsigned int rotation = plane_state->base.rotation; > > @@ -3414,7 +3416,7 @@ static void > > skylake_update_primary_plane(struct drm_plane *plane, > >  intel_crtc->adjusted_y = src_y; > >  > >  if (wm->dirty_pipes & drm_crtc_mask(&intel_crtc->base)) > > - skl_write_plane_wm(intel_crtc, wm, 0); > > + skl_write_plane_wm(intel_crtc, p_wm, &wm->ddb, 0); > >  > >  I915_WRITE(PLANE_CTL(pipe, 0), plane_ctl); > >  I915_WRITE(PLANE_OFFSET(pipe, 0), (src_y << 16) | src_x); > > @@ -3448,6 +3450,8 @@ static void > > skylake_disable_primary_plane(struct drm_plane *primary, > >  struct drm_device *dev = crtc->dev; > >  struct drm_i915_private *dev_priv = to_i915(dev); > >  struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > + struct intel_crtc_state *cstate = > > to_intel_crtc_state(crtc->state); > > + const struct skl_plane_wm *p_wm = &cstate- > > >wm.skl.optimal.planes[0]; > >  int pipe = intel_crtc->pipe; > >  > >  /* > > @@ -3455,7 +3459,8 @@ static void > > skylake_disable_primary_plane(struct drm_plane *primary, > >   * plane's visiblity isn't actually changing neither is > > its watermarks. > >   */ > >  if (!crtc->primary->state->visible) > > - skl_write_plane_wm(intel_crtc, &dev_priv- > > >wm.skl_results, 0); > > + skl_write_plane_wm(intel_crtc, p_wm, > > +    &dev_priv->wm.skl_results.ddb, > > 0); > >  > >  I915_WRITE(PLANE_CTL(pipe, 0), 0); > >  I915_WRITE(PLANE_SURF(pipe, 0), 0); > > @@ -10819,12 +10824,15 @@ static void i9xx_update_cursor(struct > > drm_crtc *crtc, u32 base, > >  struct drm_device *dev = crtc->dev; > >  struct drm_i915_private *dev_priv = to_i915(dev); > >  struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > + struct intel_crtc_state *cstate = > > to_intel_crtc_state(crtc->state); > >  const struct skl_wm_values *wm = &dev_priv- > > >wm.skl_results; > > + const struct skl_plane_wm *p_wm = > > + &cstate->wm.skl.optimal.planes[PLANE_CURSOR]; > >  int pipe = intel_crtc->pipe; > >  uint32_t cntl = 0; > >  > >  if (INTEL_GEN(dev_priv) >= 9 && wm->dirty_pipes & > > drm_crtc_mask(crtc)) > > - skl_write_cursor_wm(intel_crtc, wm); > > + skl_write_cursor_wm(intel_crtc, p_wm, &wm->ddb); >
[PATCH 09/10] drm/i915/gen9: Actually verify WM levels in verify_wm_state()
Em Sex, 2016-10-07 às 20:11 -0400, Lyude escreveu: > Thanks to Paulo Zanoni for indirectly pointing this out. > > Looks like we never actually added any code for checking whether or > not > we actually wrote watermark levels properly. Let's fix that. Thanks for doing this! Reviewed-by: Paulo Zanoni A check that would have prevented one of the bugs I solved would be "if plane is active, then level 0 must be enabled, and DDB partitioning size must be non-zero". I'll put this in my TODO list, but I won't complain if somebody does it first :) > > Signed-off-by: Lyude > Cc: Maarten Lankhorst > Cc: Ville Syrjälä > Cc: Paulo Zanoni > --- >  drivers/gpu/drm/i915/intel_display.c | 100 > +-- >  1 file changed, 84 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 39400a0..2c682bc 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -13444,30 +13444,66 @@ static void verify_wm_state(struct drm_crtc > *crtc, >  struct drm_device *dev = crtc->dev; >  struct drm_i915_private *dev_priv = to_i915(dev); >  struct skl_ddb_allocation hw_ddb, *sw_ddb; > - struct skl_ddb_entry *hw_entry, *sw_entry; > + struct skl_pipe_wm hw_wm, *sw_wm; > + struct skl_plane_wm *hw_plane_wm, *sw_plane_wm; > + struct skl_ddb_entry *hw_ddb_entry, *sw_ddb_entry; >  struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >  const enum pipe pipe = intel_crtc->pipe; > - int plane; > + int plane, level, max_level = ilk_wm_max_level(dev); >  >  if (INTEL_INFO(dev)->gen < 9 || !new_state->active) >  return; >  > + skl_pipe_wm_get_hw_state(crtc, &hw_wm); > + sw_wm = &intel_crtc->wm.active.skl; > + >  skl_ddb_get_hw_state(dev_priv, &hw_ddb); >  sw_ddb = &dev_priv->wm.skl_hw.ddb; >  >  /* planes */ >  for_each_plane(dev_priv, pipe, plane) { > - hw_entry = &hw_ddb.plane[pipe][plane]; > - sw_entry = &sw_ddb->plane[pipe][plane]; > + hw_plane_wm = &hw_wm.planes[plane]; > + sw_plane_wm = &sw_wm->planes[plane]; >  > - if (skl_ddb_entry_equal(hw_entry, sw_entry)) > - continue; > + /* Watermarks */ > + for (level = 0; level <= max_level; level++) { > + if (skl_wm_level_equals(&hw_plane_wm- > >wm[level], > + &sw_plane_wm- > >wm[level])) > + continue; > + > + DRM_ERROR("mismatch in WM pipe %c plane %d > level %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n", > +   pipe_name(pipe), plane + 1, level, > +   sw_plane_wm->wm[level].plane_en, > +   sw_plane_wm- > >wm[level].plane_res_b, > +   sw_plane_wm- > >wm[level].plane_res_l, > +   hw_plane_wm->wm[level].plane_en, > +   hw_plane_wm- > >wm[level].plane_res_b, > +   hw_plane_wm- > >wm[level].plane_res_l); > + } >  > - DRM_ERROR("mismatch in DDB state pipe %c plane %d " > -   "(expected (%u,%u), found (%u,%u))\n", > -   pipe_name(pipe), plane + 1, > -   sw_entry->start, sw_entry->end, > -   hw_entry->start, hw_entry->end); > + if (!skl_wm_level_equals(&hw_plane_wm->trans_wm, > +  &sw_plane_wm->trans_wm)) { > + DRM_ERROR("mismatch in trans WM pipe %c > plane %d (expected e=%d b=%d l=%d, got e=%d b=%d l=%d)\n", > +   pipe_name(pipe), plane + 1, > +   sw_plane_wm->trans_wm.plane_en, > +   sw_plane_wm->trans_wm.plane_res_b, > +   sw_plane_wm->trans_wm.plane_res_l, > +   hw_plane_wm->trans_wm.plane_en, > +   hw_plane_wm->trans_wm.plane_res_b, > +   hw_plane_wm- > >trans_wm.plane_res_l); > + } > + > + /* DDB */ > + hw_ddb_entry = &hw_ddb.plane[pipe][plane]; > + sw_ddb_entry = &sw_ddb->plane[pipe][plane]; > + > + if (!skl_ddb_entry_equal(hw_ddb_entry, > sw_ddb_entry)) { > + DRM_ERROR("mismatch in DDB state pipe %c > plane %d " > +   "(expected (%u,%u), found > (%u,%u))\n", > +   pipe_name(pipe), plane + 1, > +   sw_ddb_entry->start, sw_ddb_entry- > >end, > +   hw_ddb_entry->start, hw_ddb_entry- > >end); > + } >  } >  >  /* > @
[Intel-gfx] [PATCH v2 4/4] drm: Add and handle new aspect ratios in DRM layer
On Tue, Aug 09, 2016 at 08:25:50PM +0530, Shashank Sharma wrote: > HDMI 2.0/CEA-861-F introduces two new aspect ratios: > - 64:27 > - 256:135 > > This patch: > - Adds new DRM flags for to represent these new aspect ratios. > - Adds new cases to handle these aspect ratios while converting > from user->kernel mode or vise versa. > > V2: Rebase > > Signed-off-by: Shashank Sharma > Reviewed-by: Sean Paul > Cc: Daniel Vetter > Cc: Emil Velikov > --- > drivers/gpu/drm/drm_modes.c | 12 > include/uapi/drm/drm_mode.h | 6 ++ > 2 files changed, 18 insertions(+) > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index 9d8f00d..ed1b07b 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -1481,6 +1481,12 @@ void drm_mode_convert_to_umode(struct > drm_mode_modeinfo *out, > case HDMI_PICTURE_ASPECT_16_9: > out->flags |= DRM_MODE_FLAG_PIC_AR_16_9; > break; > + case HDMI_PICTURE_ASPECT_64_27: > + out->flags |= DRM_MODE_FLAG_PIC_AR_64_27; > + break; > + case DRM_MODE_PICTURE_ASPECT_256_135: > + out->flags |= DRM_MODE_FLAG_PIC_AR_256_135; > + break; > case HDMI_PICTURE_ASPECT_RESERVED: > default: > out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; > @@ -1542,6 +1548,12 @@ int drm_mode_convert_umode(struct drm_display_mode > *out, > case DRM_MODE_FLAG_PIC_AR_16_9: > out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; > break; > + case DRM_MODE_FLAG_PIC_AR_64_27: > + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27; > + break; > + case DRM_MODE_FLAG_PIC_AR_256_135: > + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135; > + break; > default: > out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > break; > diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h > index 77c869d6..4d3429b 100644 > --- a/include/uapi/drm/drm_mode.h > +++ b/include/uapi/drm/drm_mode.h > @@ -81,6 +81,8 @@ extern "C" { > #define DRM_MODE_PICTURE_ASPECT_NONE 0 > #define DRM_MODE_PICTURE_ASPECT_4_3 1 > #define DRM_MODE_PICTURE_ASPECT_16_9 2 > +#define DRM_MODE_PICTURE_ASPECT_64_273 > +#define DRM_MODE_PICTURE_ASPECT_256_135 4 Minor nit here, but in my tree the '4' above doesn't line up with the three previous definitions. I downloaded the series as a mbox from patchwork. Jim > > /* Aspect ratio flag bitmask (4 bits 22:19) */ > #define DRM_MODE_FLAG_PIC_AR_MASK(0x0F<<19) > @@ -90,6 +92,10 @@ extern "C" { > (DRM_MODE_PICTURE_ASPECT_4_3<<19) > #define DRM_MODE_FLAG_PIC_AR_16_9 \ > (DRM_MODE_PICTURE_ASPECT_16_9<<19) > +#define DRM_MODE_FLAG_PIC_AR_64_27 \ > + (DRM_MODE_PICTURE_ASPECT_64_27<<19) > +#define DRM_MODE_FLAG_PIC_AR_256_135 \ > + (DRM_MODE_PICTURE_ASPECT_256_135<<19) > > /* DPMS flags */ > /* bit compatible with the xorg definitions. */ > -- > 1.9.1 > > ___ > Intel-gfx mailing list > Intel-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 2/4] drm: Add aspect ratio parsing in DRM layer
On Tue, Aug 09, 2016 at 08:25:48PM +0530, Shashank Sharma wrote: > Current DRM layer functions don't parse aspect ratio information > while converting a user mode->kernel mode or vice versa. This > causes modeset to pick mode with wrong aspect ratio, eventually > causing failures in HDMI compliance test cases, due to wrong VIC. > > This patch adds aspect ratio information in DRM's mode conversion > and mode comparision functions, to make sure kernel picks mode > with right aspect ratio (as per the VIC). > > V2: Addressed review comments from Sean: > - Fix spellings/typo > - No need to handle aspect ratio none > - Add a break, for default case too > > Signed-off-by: Shashank Sharma > Signed-off-by: Lin, Jia > Signed-off-by: Akashdeep Sharma Reviewed-by: Jim Bride > > Cc: Daniel Vetter > Cc: Emil Velikov > --- > drivers/gpu/drm/drm_modes.c | 31 +++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c > index fc5040a..9d8f00d 100644 > --- a/drivers/gpu/drm/drm_modes.c > +++ b/drivers/gpu/drm/drm_modes.c > @@ -969,6 +969,7 @@ bool drm_mode_equal_no_clocks_no_stereo(const struct > drm_display_mode *mode1, > mode1->vsync_end == mode2->vsync_end && > mode1->vtotal == mode2->vtotal && > mode1->vscan == mode2->vscan && > + mode1->picture_aspect_ratio == mode2->picture_aspect_ratio && > (mode1->flags & ~DRM_MODE_FLAG_3D_MASK) == >(mode2->flags & ~DRM_MODE_FLAG_3D_MASK)) > return true; > @@ -1471,6 +1472,21 @@ void drm_mode_convert_to_umode(struct > drm_mode_modeinfo *out, > out->vrefresh = in->vrefresh; > out->flags = in->flags; > out->type = in->type; > + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; > + > + switch (in->picture_aspect_ratio) { > + case HDMI_PICTURE_ASPECT_4_3: > + out->flags |= DRM_MODE_FLAG_PIC_AR_4_3; > + break; > + case HDMI_PICTURE_ASPECT_16_9: > + out->flags |= DRM_MODE_FLAG_PIC_AR_16_9; > + break; > + case HDMI_PICTURE_ASPECT_RESERVED: > + default: > + out->flags |= DRM_MODE_FLAG_PIC_AR_NONE; > + break; > + } > + > strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); > out->name[DRM_DISPLAY_MODE_LEN-1] = 0; > } > @@ -1516,6 +1532,21 @@ int drm_mode_convert_umode(struct drm_display_mode > *out, > strncpy(out->name, in->name, DRM_DISPLAY_MODE_LEN); > out->name[DRM_DISPLAY_MODE_LEN-1] = 0; > > + /* Clearing picture aspect ratio bits from out flags */ > + out->flags &= ~DRM_MODE_FLAG_PIC_AR_MASK; > + > + switch (in->flags & DRM_MODE_FLAG_PIC_AR_MASK) { > + case DRM_MODE_FLAG_PIC_AR_4_3: > + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_4_3; > + break; > + case DRM_MODE_FLAG_PIC_AR_16_9: > + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; > + break; > + default: > + out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; > + break; > + } > + > out->status = drm_mode_validate_basic(out); > if (out->status != MODE_OK) > goto out; > -- > 1.9.1 > > ___ > Intel-gfx mailing list > Intel-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx