| Message ID | 20191127092523.5620-2-kraxel@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm: mmap fixups | expand |
On Wed, Nov 27, 2019 at 10:25:22AM +0100, Gerd Hoffmann wrote: > The fake offset is going to stay, so change the calling convention for > drm_gem_object_funcs.mmap to include the fake offset. Update all users > accordingly. > > Note that this reverts 83b8a6f242ea ("drm/gem: Fix mmap fake offset > handling for drm_gem_object_funcs.mmap") and on top then adds the fake > offset to drm_gem_prime_mmap to make sure all paths leading to > obj->funcs->mmap are consistent. > > v3: move fake-offset tweak in drm_gem_prime_mmap() so we have this code > only once in the function (Rob Herring). Now this series fails in Intel CI. Can't see why though. The difference between v2 and v3 is just the place where vma->vm_pgoff gets updated, and no code between the v2 and v3 location touches vma ... confused, Gerd
On Thu, Nov 28, 2019 at 12:39:30PM +0100, Gerd Hoffmann wrote: > On Wed, Nov 27, 2019 at 10:25:22AM +0100, Gerd Hoffmann wrote: > > The fake offset is going to stay, so change the calling convention for > > drm_gem_object_funcs.mmap to include the fake offset. Update all users > > accordingly. > > > > Note that this reverts 83b8a6f242ea ("drm/gem: Fix mmap fake offset > > handling for drm_gem_object_funcs.mmap") and on top then adds the fake > > offset to drm_gem_prime_mmap to make sure all paths leading to > > obj->funcs->mmap are consistent. > > > > v3: move fake-offset tweak in drm_gem_prime_mmap() so we have this code > > only once in the function (Rob Herring). > > Now this series fails in Intel CI. Can't see why though. The > difference between v2 and v3 is just the place where vma->vm_pgoff gets > updated, and no code between the v2 and v3 location touches vma ... Looks like unrelated flukes, this happens occasionally. If you're paranoid hit the retest button on patchwork to double-check. -Daniel
On Thu, Dec 05, 2019 at 11:15:23PM +0100, Daniel Vetter wrote: > On Thu, Nov 28, 2019 at 12:39:30PM +0100, Gerd Hoffmann wrote: > > On Wed, Nov 27, 2019 at 10:25:22AM +0100, Gerd Hoffmann wrote: > > > The fake offset is going to stay, so change the calling convention for > > > drm_gem_object_funcs.mmap to include the fake offset. Update all users > > > accordingly. > > > > > > Note that this reverts 83b8a6f242ea ("drm/gem: Fix mmap fake offset > > > handling for drm_gem_object_funcs.mmap") and on top then adds the fake > > > offset to drm_gem_prime_mmap to make sure all paths leading to > > > obj->funcs->mmap are consistent. > > > > > > v3: move fake-offset tweak in drm_gem_prime_mmap() so we have this code > > > only once in the function (Rob Herring). > > > > Now this series fails in Intel CI. Can't see why though. The > > difference between v2 and v3 is just the place where vma->vm_pgoff gets > > updated, and no code between the v2 and v3 location touches vma ... > > Looks like unrelated flukes, this happens occasionally. If you're paranoid > hit the retest button on patchwork to double-check. > -Daniel Guess you kicked CI? Just got CI mails, now reporting success, without doing anything. So I'll go push v3 to misc-next. cheers, Gerd
On Fri, Dec 6, 2019 at 11:07 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Thu, Dec 05, 2019 at 11:15:23PM +0100, Daniel Vetter wrote: > > On Thu, Nov 28, 2019 at 12:39:30PM +0100, Gerd Hoffmann wrote: > > > On Wed, Nov 27, 2019 at 10:25:22AM +0100, Gerd Hoffmann wrote: > > > > The fake offset is going to stay, so change the calling convention for > > > > drm_gem_object_funcs.mmap to include the fake offset. Update all users > > > > accordingly. > > > > > > > > Note that this reverts 83b8a6f242ea ("drm/gem: Fix mmap fake offset > > > > handling for drm_gem_object_funcs.mmap") and on top then adds the fake > > > > offset to drm_gem_prime_mmap to make sure all paths leading to > > > > obj->funcs->mmap are consistent. > > > > > > > > v3: move fake-offset tweak in drm_gem_prime_mmap() so we have this code > > > > only once in the function (Rob Herring). > > > > > > Now this series fails in Intel CI. Can't see why though. The > > > difference between v2 and v3 is just the place where vma->vm_pgoff gets > > > updated, and no code between the v2 and v3 location touches vma ... > > > > Looks like unrelated flukes, this happens occasionally. If you're paranoid > > hit the retest button on patchwork to double-check. > > -Daniel > > Guess you kicked CI? Just got CI mails, now reporting success, without > doing anything. So I'll go push v3 to misc-next. Yeah I kicked it, since iirc you need your patchwork account to be treated with the re-run powers first. Might want to get those with a quick ping on irc, it's useful. -Daniel
On Fri, Dec 06, 2019 at 11:07:24AM +0100, Gerd Hoffmann wrote: > On Thu, Dec 05, 2019 at 11:15:23PM +0100, Daniel Vetter wrote: > > Looks like unrelated flukes, this happens occasionally. If you're paranoid > > hit the retest button on patchwork to double-check. > > -Daniel > > Guess you kicked CI? Just got CI mails, now reporting success, without > doing anything. So I'll go push v3 to misc-next. Oops, spoke too soon. Next mail arrived. Fi.CI.BAT works, but Fi.CI.IGT still fails. Where is the "test again" button? Can I re-run the tests on v2? Right now I tend to commit v2 to have a fix committed, then go figure why v3 fails ... cheers, Gerd
On Fri, Dec 6, 2019 at 11:22 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > On Fri, Dec 06, 2019 at 11:07:24AM +0100, Gerd Hoffmann wrote: > > On Thu, Dec 05, 2019 at 11:15:23PM +0100, Daniel Vetter wrote: > > > Looks like unrelated flukes, this happens occasionally. If you're paranoid > > > hit the retest button on patchwork to double-check. > > > -Daniel > > > > Guess you kicked CI? Just got CI mails, now reporting success, without > > doing anything. So I'll go push v3 to misc-next. > > Oops, spoke too soon. Next mail arrived. Fi.CI.BAT works, but > Fi.CI.IGT still fails. > > Where is the "test again" button? Can I re-run the tests on v2? Right > now I tend to commit v2 to have a fix committed, then go figure why v3 > fails ... Top of the mail you get from CI is the link to the patchwork series. That contains all the same info like in the mail, plus the coveted button. If failures look similar then yes I guess v3 is somehow broken. But I've looked, looks like just 2x unrelated noise and you being unlucky, so imo totally fine to push v3. I'll give CI folks a heads-up (best done over irc usually). -Daniel
On Fri, Dec 06, 2019 at 12:10:15PM +0100, Daniel Vetter wrote: > On Fri, Dec 6, 2019 at 11:22 AM Gerd Hoffmann <kraxel@redhat.com> wrote: > > > > > Guess you kicked CI? Just got CI mails, now reporting success, without > > > doing anything. So I'll go push v3 to misc-next. > > > > Oops, spoke too soon. Next mail arrived. Fi.CI.BAT works, but > > Fi.CI.IGT still fails. > > > > Where is the "test again" button? Can I re-run the tests on v2? Right > > now I tend to commit v2 to have a fix committed, then go figure why v3 > > fails ... > > Top of the mail you get from CI is the link to the patchwork series. > That contains all the same info like in the mail, plus the coveted > button. If failures look similar then yes I guess v3 is somehow > broken. But I've looked, looks like just 2x unrelated noise and you > being unlucky, so imo totally fine to push v3. I'll give CI folks a > heads-up (best done over irc usually). Ok. Pushed now. cheers, Gerd
diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 97a48165642c..0b375069cd48 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -159,9 +159,7 @@ struct drm_gem_object_funcs { * * The callback is used by by both drm_gem_mmap_obj() and * drm_gem_prime_mmap(). When @mmap is present @vm_ops is not - * used, the @mmap callback must set vma->vm_ops instead. The @mmap - * callback is always called with a 0 offset. The caller will remove - * the fake offset as necessary. + * used, the @mmap callback must set vma->vm_ops instead. */ int (*mmap)(struct drm_gem_object *obj, struct vm_area_struct *vma); diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 2f2b889096b0..56f42e0f2584 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1106,9 +1106,6 @@ int drm_gem_mmap_obj(struct drm_gem_object *obj, unsigned long obj_size, return -EINVAL; if (obj->funcs && obj->funcs->mmap) { - /* Remove the fake offset */ - vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node); - ret = obj->funcs->mmap(obj, vma); if (ret) return ret; diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/drm_gem_shmem_helper.c index 0810d3ef6961..a421a2eed48a 100644 --- a/drivers/gpu/drm/drm_gem_shmem_helper.c +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c @@ -528,6 +528,9 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) struct drm_gem_shmem_object *shmem; int ret; + /* Remove the fake offset */ + vma->vm_pgoff -= drm_vma_node_start(&obj->vma_node); + shmem = to_drm_gem_shmem_obj(obj); ret = drm_gem_shmem_get_pages(shmem); diff --git a/drivers/gpu/drm/drm_prime.c b/drivers/gpu/drm/drm_prime.c index 0814211b0f3f..a0f929c7117b 100644 --- a/drivers/gpu/drm/drm_prime.c +++ b/drivers/gpu/drm/drm_prime.c @@ -713,6 +713,9 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) struct file *fil; int ret; + /* Add the fake offset */ + vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); + if (obj->funcs && obj->funcs->mmap) { ret = obj->funcs->mmap(obj, vma); if (ret) @@ -737,8 +740,6 @@ int drm_gem_prime_mmap(struct drm_gem_object *obj, struct vm_area_struct *vma) if (ret) goto out; - vma->vm_pgoff += drm_vma_node_start(&obj->vma_node); - ret = obj->dev->driver->fops->mmap(fil, vma); drm_vma_node_revoke(&obj->vma_node, priv); diff --git a/drivers/gpu/drm/ttm/ttm_bo_vm.c b/drivers/gpu/drm/ttm/ttm_bo_vm.c index e6495ca2630b..3e8c3de91ae4 100644 --- a/drivers/gpu/drm/ttm/ttm_bo_vm.c +++ b/drivers/gpu/drm/ttm/ttm_bo_vm.c @@ -514,13 +514,6 @@ EXPORT_SYMBOL(ttm_bo_mmap); int ttm_bo_mmap_obj(struct vm_area_struct *vma, struct ttm_buffer_object *bo) { ttm_bo_get(bo); - - /* - * FIXME: &drm_gem_object_funcs.mmap is called with the fake offset - * removed. Add it back here until the rest of TTM works without it. - */ - vma->vm_pgoff += drm_vma_node_start(&bo->base.vma_node); - ttm_bo_mmap_vma_setup(bo, vma); return 0; }
The fake offset is going to stay, so change the calling convention for drm_gem_object_funcs.mmap to include the fake offset. Update all users accordingly. Note that this reverts 83b8a6f242ea ("drm/gem: Fix mmap fake offset handling for drm_gem_object_funcs.mmap") and on top then adds the fake offset to drm_gem_prime_mmap to make sure all paths leading to obj->funcs->mmap are consistent. v3: move fake-offset tweak in drm_gem_prime_mmap() so we have this code only once in the function (Rob Herring). Fixes: 83b8a6f242ea ("drm/gem: Fix mmap fake offset handling for drm_gem_object_funcs.mmap") Cc: Daniel Vetter <daniel.vetter@ffwll.ch> Cc: Rob Herring <robh@kernel.org> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch> --- include/drm/drm_gem.h | 4 +--- drivers/gpu/drm/drm_gem.c | 3 --- drivers/gpu/drm/drm_gem_shmem_helper.c | 3 +++ drivers/gpu/drm/drm_prime.c | 5 +++-- drivers/gpu/drm/ttm/ttm_bo_vm.c | 7 ------- 5 files changed, 7 insertions(+), 15 deletions(-)