drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping
Subsystems (3)
PATCH 1/1 drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the info->system_base mmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume as intel_fbdev_set_suspend() does a full clear over the whole scanout.
--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
drivers/gpu/drm/i915/intel_fbdev.c +15 -0
@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_object_ggtt_pin (obj,NULL ,0 ,PIN_MAPPABLE ); +if (ret) + goto out_unlock; + info =drm_fb_helper_alloc_fbi (helper);if (IS_ERR (info)) { ret =PTR_ERR (info);
@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi:drm_fb_helper_release_fbi (helper); out_unpin: +/* Once for info->screen_base mmaping... */ +i915_gem_object_ggtt_unpin (obj); +/* ...and once for the intel_fb */ i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base); out_unlock:
@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {static void intel_fbdev_destroy (struct drm_device *dev,struct intel_fbdev *ifbdev) { +/* Release the pinning for the info->screen_base mmaping. */ +i915_gem_object_ggtt_unpin (ifbdev->fb->obj);drm_fb_helper_unregister_fbi (&ifbdev->helper);drm_fb_helper_release_fbi (&ifbdev->helper);
Signature · 1 line
2.5.0
Quoted 11 lines · Chris Wilson
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.
Yup this is a critical fix :) by keeping the internal FB pinned we avoid alloc of buffer within same FB GTT offset
Quoted 41 lines
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)drivers/gpu/drm/i915/intel_fbdev.c +13
-0@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_object_ggtt_pin (obj,NULL ,0 ,PIN_MAPPABLE ); +if (ret) + goto out_unlock; + info =drm_fb_helper_alloc_fbi (helper);if (IS_ERR (info)) { ret =PTR_ERR (info);@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi:drm_fb_helper_release_fbi (helper); out_unpin: +/* Once for info->screen_base mmaping... */ +i915_gem_object_ggtt_unpin (obj); +/* ...and once for the intel_fb */ i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base); out_unlock:
drivers/gpu/drm/i915/intel_fbdev.c static const struct drm_fb_helper_funcs intel_fb_helper_funcs = { · line 534
Quoted 8 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-0@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {static void intel_fbdev_destroy (struct drm_device *dev,struct intel_fbdev *ifbdev) { +/* Release the pinning for the info->screen_base mmaping. */ +i915_gem_object_ggtt_unpin (ifbdev->fb->obj);drm_fb_helper_unregister_fbi (&ifbdev->helper);drm_fb_helper_release_fbi (&ifbdev->helper);
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 15 lines · Deepak
Quoted 11 lines · Chris Wilson
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.
Quoted 3 lines
Yup this is a critical fix :) by keeping the internal FB pinned we avoid alloc of buffer within same FB GTT offset
Reviewed-by Deepak S <[email protected]>
Daniel,
Is there anything else that needs to happen before this gets pulled in?
Thanks,
Wayne
Quoted 52 lines
Quoted 52 lines
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)drivers/gpu/drm/i915/intel_fbdev.c +10
-0@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_object_ggtt_pin (obj,NULL ,0 ,PIN_MAPPABLE ); +if (ret) + goto out_unlock; + info =drm_fb_helper_alloc_fbi (helper);if (IS_ERR (info)) {ret = PTR_ERR(info);drivers/gpu/drm/i915/intel_fbdev.c +3
-0@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi:drm_fb_helper_release_fbi (helper); out_unpin: +/* Once for info->screen_base mmaping... */ +i915_gem_object_ggtt_unpin (obj); +/* ...and once for the intel_fb */ i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base);
out_unlock:drivers/gpu/drm/i915/intel_fbdev.c +2
-0@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {static void intel_fbdev_destroy (struct drm_device *dev,struct intel_fbdev *ifbdev) { +/* Release the pinning for the info->screen_base mmaping. */ +i915_gem_object_ggtt_unpin (ifbdev->fb->obj);
Quoted 7 lines
Quoted 2 lines
drivers/gpu/drm/i915/intel_fbdev.c +0
-0drm_fb_helper_unregister_fbi (&ifbdev->helper);drm_fb_helper_release_fbi(&ifbdev->helper);_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 18 lines · Wayne
Quoted 15 lines · Deepak
Quoted 11 lines · Chris Wilson
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.
Quoted 7 lines
Quoted 3 lines
Yup this is a critical fix :) by keeping the internal FB pinned we avoid alloc of buffer within same FB GTT offset
Reviewed-by Deepak S <[email protected]>Daniel,
Is there anything else that needs to happen before this gets pulled in?
Someone needs to send out the rebased-on-upstream version. The patches have been merged but then had to be dropped again since they where based on top of Chris' tree.
-Daniel
Quoted 64 lines
Thanks,
WayneQuoted 59 lines
Quoted 53 lines
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)drivers/gpu/drm/i915/intel_fbdev.c +10
-0@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_object_ggtt_pin (obj,NULL ,0 ,PIN_MAPPABLE ); +if (ret) + goto out_unlock; + info =drm_fb_helper_alloc_fbi (helper);if (IS_ERR (info)) {ret = PTR_ERR(info);drivers/gpu/drm/i915/intel_fbdev.c +3
-0@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi:drm_fb_helper_release_fbi (helper); out_unpin: +/* Once for info->screen_base mmaping... */ +i915_gem_object_ggtt_unpin (obj); +/* ...and once for the intel_fb */ i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base);
out_unlock:drivers/gpu/drm/i915/intel_fbdev.c +2
-0@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {static void intel_fbdev_destroy (struct drm_device *dev,struct intel_fbdev *ifbdev) { +/* Release the pinning for the info->screen_base mmaping. */ +i915_gem_object_ggtt_unpin (ifbdev->fb->obj);drm_fb_helper_unregister_fbi (&ifbdev->helper);Quoted 1 line
drm_fb_helper_release_fbi(&ifbdev->helper);_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 1/1 drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping
From: Chris Wilson <[email protected]>
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the info->system_base mmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume as intel_fbdev_set_suspend() does a full clear over the whole scanout.
v2: rebased on latest nightly (Wayne)
--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
drivers/gpu/drm/i915/intel_fbdev.c +15 -0
@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_object_ggtt_pin (obj,NULL ,0 ,PIN_MAPPABLE ); +if (ret) + goto out_unlock; + info =drm_fb_helper_alloc_fbi (helper);if (IS_ERR (info)) { ret =PTR_ERR (info);
@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi:drm_fb_helper_release_fbi (helper); out_unpin: +/* Once for info->screen_base mmaping... */ +i915_gem_object_ggtt_unpin (obj); +/* ...and once for the intel_fb */ i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base); out_unlock:
@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {static void intel_fbdev_destroy (struct drm_device *dev,struct intel_fbdev *ifbdev) { +/* Release the pinning for the info->screen_base mmaping. */ +i915_gem_object_ggtt_unpin (ifbdev->fb->obj);drm_fb_helper_unregister_fbi (&ifbdev->helper);drm_fb_helper_release_fbi (&ifbdev->helper);
Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 32 lines · Wayne Boyer
From: Chris Wilson <[email protected]>
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.v2: rebased on latest nightly (Wayne)
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>Reviewed-by Deepak S <[email protected]>Signed-off-by Wayne Boyer <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)drivers/gpu/drm/i915/intel_fbdev.c +0
-0
drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_create(struct drm_fb_helper *helper, · line 224
Quoted 10 lines
drivers/gpu/drm/i915/intel_fbdev.c +7
-0@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_object_ggtt_pin (obj,NULL ,0 ,PIN_MAPPABLE );
This should be i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE); At which point I just rage quit.
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Quoted 36 lines · Chris Wilson
Quoted 35 lines · Wayne Boyer
From: Chris Wilson <[email protected]>
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.v2: rebased on latest nightly (Wayne)
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>Reviewed-by Deepak S <[email protected]>Signed-off-by Wayne Boyer <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)drivers/gpu/drm/i915/intel_fbdev.c +0
-0@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size;
Quoted 10 lines
drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_create(struct drm_fb_helper *helper, · line 224
Quoted 7 lines
drivers/gpu/drm/i915/intel_fbdev.c +7
-0+/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_object_ggtt_pin (obj,NULL ,0 ,PIN_MAPPABLE );This should be
i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE); At which point I just rage quit.
LOL GEM naming strikes again!
Jesse
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 1/1 drm/i915: Pin the ifbdev for the info->system_base GGTT mmapping
In reply to Chris WilsonFrom: Chris Wilson <[email protected]>
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the info->system_base mmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume as intel_fbdev_set_suspend() does a full clear over the whole scanout.
v2: rebased on latest nightly (Wayne)
v3: changed i915_gem_object_ggtt_pin() to i915_gem_obj_ggtt_pin() based on Chris' review. (Wayne)
--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)
drivers/gpu/drm/i915/intel_fbdev.c +15 -0
@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_obj_ggtt_pin (obj,0 ,PIN_MAPPABLE ); +if (ret) + goto out_unlock; + info =drm_fb_helper_alloc_fbi (helper);if (IS_ERR (info)) { ret =PTR_ERR (info);
@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi:drm_fb_helper_release_fbi (helper); out_unpin: +/* Once for info->screen_base mmaping... */ +i915_gem_object_ggtt_unpin (obj); +/* ...and once for the intel_fb */ i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base); out_unlock:
@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {static void intel_fbdev_destroy (struct drm_device *dev,struct intel_fbdev *ifbdev) { +/* Release the pinning for the info->screen_base mmaping. */ +i915_gem_object_ggtt_unpin (ifbdev->fb->obj);drm_fb_helper_unregister_fbi (&ifbdev->helper);drm_fb_helper_release_fbi (&ifbdev->helper);
Signature · 1 line
1.9.1
Quoted 14 lines · Wayne Boyer
From: Chris Wilson <[email protected]>
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.
Quoted 3 lines
v2: rebased on latest nightly (Wayne)
v3: changedi915_gem_object_ggtt_pin()toi915_gem_obj_ggtt_pin()based on Chris' review. (Wayne)
Note that this patch also depends on the
drm/i915: Set the map-and-fenceable flag for preallocated objects
fix as well
http://patchwork.freedesktop.org/patch/58026/
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Jani Nikula
maintainerQuoted 19 lines · Chris Wilson
Quoted 14 lines · Wayne Boyer
From: Chris Wilson <[email protected]>
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.Quoted 3 lines
v2: rebased on latest nightly (Wayne)
v3: changedi915_gem_object_ggtt_pin()toi915_gem_obj_ggtt_pin()based on Chris' review. (Wayne)
Quoted 6 lines
Note that this patch also depends on the
drm/i915: Set the map-and-fenceable flag for preallocated objects
Jesse, please provide your Tested-by on that plus this patch, since you reported the breakage [1] that got the two patches reverted in the first place.
Thanks,
Jani.
[1] http://mid.gmane.org/[email protected]
Quoted 8 lines
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 61 lines · Wayne Boyer
From: Chris Wilson <[email protected]>
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.v2: rebased on latest nightly (Wayne)
v3: changedi915_gem_object_ggtt_pin()toi915_gem_obj_ggtt_pin()based on Chris' review. (Wayne)Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>Reviewed-by Deepak S <[email protected]>Signed-off-by Wayne Boyer <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)drivers/gpu/drm/i915/intel_fbdev.c +13
-0@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_obj_ggtt_pin (obj,0 ,PIN_MAPPABLE ); +if (ret) + goto out_unlock; + info =drm_fb_helper_alloc_fbi (helper);if (IS_ERR (info)) { ret =PTR_ERR (info);@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi:drm_fb_helper_release_fbi (helper); out_unpin: +/* Once for info->screen_base mmaping... */ +i915_gem_object_ggtt_unpin (obj); +/* ...and once for the intel_fb */ i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base); out_unlock:
drivers/gpu/drm/i915/intel_fbdev.c static const struct drm_fb_helper_funcs intel_fb_helper_funcs = { · line 534
Quoted 8 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-0@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {static void intel_fbdev_destroy (struct drm_device *dev,struct intel_fbdev *ifbdev) { +/* Release the pinning for the info->screen_base mmaping. */ +i915_gem_object_ggtt_unpin (ifbdev->fb->obj);drm_fb_helper_unregister_fbi (&ifbdev->helper);drm_fb_helper_release_fbi (&ifbdev->helper);
Hmm .. pinning now done by i915_gem_obj_ggtt_pin(), but the unpinning function is i915_gem_object_ggtt_unpin(). Just the sort of asymmetry that helps everyone understand what's going on :(
Could we not have a mass rename of the various i915_gem_obj{ect} functions to ONE consistent naming convention? (Personally I prefer 'obj' because it's shorter, but consistency is more important than saving just 3 letters).
.Dave.
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 75 lines · Dave Gordon
Quoted 61 lines · Wayne Boyer
From: Chris Wilson <[email protected]>
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.v2: rebased on latest nightly (Wayne)
v3: changedi915_gem_object_ggtt_pin()toi915_gem_obj_ggtt_pin()based on Chris' review. (Wayne)Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>Reviewed-by Deepak S <[email protected]>Signed-off-by Wayne Boyer <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)drivers/gpu/drm/i915/intel_fbdev.c +13
-0@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_obj_ggtt_pin (obj,0 ,PIN_MAPPABLE ); +if (ret) + goto out_unlock; + info =drm_fb_helper_alloc_fbi (helper);if (IS_ERR (info)) { ret =PTR_ERR (info);@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi:drm_fb_helper_release_fbi (helper); out_unpin: +/* Once for info->screen_base mmaping... */ +i915_gem_object_ggtt_unpin (obj); +/* ...and once for the intel_fb */ i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base); out_unlock:drivers/gpu/drm/i915/intel_fbdev.c static const struct drm_fb_helper_funcs intel_fb_helper_funcs = { · line 534
Quoted 8 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-0@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {static void intel_fbdev_destroy (struct drm_device *dev,struct intel_fbdev *ifbdev) { +/* Release the pinning for the info->screen_base mmaping. */ +i915_gem_object_ggtt_unpin (ifbdev->fb->obj);drm_fb_helper_unregister_fbi (&ifbdev->helper);drm_fb_helper_release_fbi (&ifbdev->helper);Hmm .. pinning now done by
i915_gem_obj_ggtt_pin(), but the unpinning function isi915_gem_object_ggtt_unpin(). Just the sort of asymmetry that helps everyone understand what's going on :(
Quoted 3 lines
Could we not have a mass rename of the various i915_gem_obj{ect} functions to ONE consistent naming convention? (Personally I prefer 'obj' because it's shorter, but consistency is more important than saving just 3 letters).
Of course, just needs someone to do it, and make sure to not step onto too many toes. I'd love if more people actually take charge of gem instead of piling more on top.
-Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Quoted 76 lines · Daniel Vetter
Quoted 75 lines · Dave Gordon
Quoted 61 lines · Wayne Boyer
From: Chris Wilson <[email protected]>
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.v2: rebased on latest nightly (Wayne)
v3: changedi915_gem_object_ggtt_pin()toi915_gem_obj_ggtt_pin()based on Chris' review. (Wayne)Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>Reviewed-by Deepak S <[email protected]>Signed-off-by Wayne Boyer <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)drivers/gpu/drm/i915/intel_fbdev.c +13
-0@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_obj_ggtt_pin (obj,0 ,PIN_MAPPABLE ); +if (ret) + goto out_unlock; + info =drm_fb_helper_alloc_fbi (helper);if (IS_ERR (info)) { ret =PTR_ERR (info);@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi:drm_fb_helper_release_fbi (helper); out_unpin: +/* Once for info->screen_base mmaping... */ +i915_gem_object_ggtt_unpin (obj); +/* ...and once for the intel_fb */ i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base); out_unlock:drivers/gpu/drm/i915/intel_fbdev.c static const struct drm_fb_helper_funcs intel_fb_helper_funcs = { · line 534
Quoted 8 lines
drivers/gpu/drm/i915/intel_fbdev.c +2
-0@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {static void intel_fbdev_destroy (struct drm_device *dev,struct intel_fbdev *ifbdev) { +/* Release the pinning for the info->screen_base mmaping. */ +i915_gem_object_ggtt_unpin (ifbdev->fb->obj);drm_fb_helper_unregister_fbi (&ifbdev->helper);drm_fb_helper_release_fbi (&ifbdev->helper);Hmm .. pinning now done by
i915_gem_obj_ggtt_pin(), but the unpinning function isi915_gem_object_ggtt_unpin(). Just the sort of asymmetry that helps everyone understand what's going on :(
Quoted 7 lines
Quoted 3 lines
Could we not have a mass rename of the various i915_gem_obj{ect} functions to ONE consistent naming convention? (Personally I prefer 'obj' because it's shorter, but consistency is more important than saving just 3 letters).
Of course, just needs someone to do it, and make sure to not step onto too many toes. I'd love if more people actually take charge of gem instead of piling more on top.
I have patches to remove as much of the nonsense as I could. I have sent some of them before, but no one looked at them it seems. Now they are about 150 patches from the top of the queue.
-Chris
Signature · 5 lines
Chris Wilson, Intel Open Source Technology Centre _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Hi,
Quoted 36 lines · Wayne Boyer
From: Chris Wilson <[email protected]>
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.v2: rebased on latest nightly (Wayne)
v3: changedi915_gem_object_ggtt_pin()toi915_gem_obj_ggtt_pin()based on Chris' review. (Wayne)Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>Reviewed-by Deepak S <[email protected]>Signed-off-by Wayne Boyer <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)drivers/gpu/drm/i915/intel_fbdev.c +0
-0@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size;
drivers/gpu/drm/i915/intel_fbdev.c static int intelfb_create(struct drm_fb_helper *helper, · line 226
Quoted 10 lines
drivers/gpu/drm/i915/intel_fbdev.c +9
-0+/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_obj_ggtt_pin (obj,0 ,PIN_MAPPABLE ); +if (ret) + goto out_unlock;
I think a DRM_ERROR() should be added here, otherwise if this check fails, the user will never be notified thereof. It's not sufficient to just return a negative int because this just gets passed up the call stack to intel_fbdev_initial_config() which ignores it.
I'm adding DRM_ERROR() invocations to the two existing error conditions in this function with patch 4 of the series I just posted to intel-gfx:
Message-Id: <e08c113c04191ecd66904423da4f1908f48b5ef6.1445771693.git.lukas@wunner.de>
Best regards,
Lukas
Quoted 30 lines
drivers/gpu/drm/i915/intel_fbdev.c +6
-0+ info =drm_fb_helper_alloc_fbi (helper);if (IS_ERR (info)) { ret =PTR_ERR (info);@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi:drm_fb_helper_release_fbi (helper); out_unpin: +/* Once for info->screen_base mmaping... */ +i915_gem_object_ggtt_unpin (obj); +/* ...and once for the intel_fb */ i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base); out_unlock:@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {static void intel_fbdev_destroy (struct drm_device *dev,struct intel_fbdev *ifbdev) { +/* Release the pinning for the info->screen_base mmaping. */ +i915_gem_object_ggtt_unpin (ifbdev->fb->obj);drm_fb_helper_unregister_fbi (&ifbdev->helper);drm_fb_helper_release_fbi (&ifbdev->helper);Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Another observation that occurred to me only after sending away my previous message (sorry):
Quoted 13 lines · Wayne Boyer
From: Chris Wilson <[email protected]>
A long time ago (before 3.14) we relied on a permanent pinning of the ifbdev to lock the fb in place inside the GGTT. However, the introduction of stealing the BIOS framebuffer and reusing its address in the GGTT for the fbdev has muddied waters and we use an inherited fb. However, the inherited fb is only pinned whilst it is active and we no longer have an explicit pin for the
info->system_basemmapping used by the fbdev. The result is that after some aperture pressure the fbdev may be evicted, but we continue to write the fbcon into the same GGTT address - overwriting anything else that may be put into that offset. The effect is most pronounced across suspend/resume asintel_fbdev_set_suspend()does a full clear over the whole scanout.
Since this only concerns the case when the fb was inherited from BIOS, why don't you invoke i915_gem_obj_ggtt_pin() only in this particular case? It's discernible from the prealloc variable.
I think then you also don't need to call i915_gem_object_ggtt_unpin() twice.
Quoted 63 lines
v2: rebased on latest nightly (Wayne)
v3: changedi915_gem_object_ggtt_pin()toi915_gem_obj_ggtt_pin()based on Chris' review. (Wayne)Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>Reviewed-by Deepak S <[email protected]>Signed-off-by Wayne Boyer <[email protected]>--- drivers/gpu/drm/i915/intel_fbdev.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)drivers/gpu/drm/i915/intel_fbdev.c +15
-0@@ -215,6 +215,16 @@ static int intelfb_create(struct drm_fb_helper *helper, obj = intel_fb->obj; size = obj->base.size; +/* The fb constructor will have already pinned us (or inherited a +* GGTT region from the BIOS) suitable for a scanout, so +* this should just be a no-op and increment the pin count for the +* fbdev mmapping. It does have a useful side-effect of validating +* the pin for fbdev's use via a GGTT mmapping. +*/ + ret =i915_gem_obj_ggtt_pin (obj,0 ,PIN_MAPPABLE ); +if (ret) + goto out_unlock; + info =drm_fb_helper_alloc_fbi (helper);if (IS_ERR (info)) { ret =PTR_ERR (info);@@ -274,6 +284,9 @@ static int intelfb_create(struct drm_fb_helper *helper, out_destroy_fbi:drm_fb_helper_release_fbi (helper); out_unpin: +/* Once for info->screen_base mmaping... */ +i915_gem_object_ggtt_unpin (obj); +/* ...and once for the intel_fb */ i915_gem_object_ggtt_unpin (obj);drm_gem_object_unreference (&obj->base); out_unlock:@@ -514,6 +527,8 @@ static const struct drm_fb_helper_funcs intel_fb_helper_funcs = {static void intel_fbdev_destroy (struct drm_device *dev,struct intel_fbdev *ifbdev) { +/* Release the pinning for the info->screen_base mmaping. */ +i915_gem_object_ggtt_unpin (ifbdev->fb->obj);drm_fb_helper_unregister_fbi (&ifbdev->helper);drm_fb_helper_release_fbi (&ifbdev->helper);Signature · 6 lines
1.9.1 _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
PATCH 1/1 drm/i915: Set the map-and-fenceable flag for preallocated objects
In reply to Chris WilsonAs we mark the preallocated objects as bound, we should also flag them correctly as being map-and-fenceable (if appropriate!) so that latter users do not get confused and try and rebind the pinned vma in order to get a map-and-fenceable binding.
--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 43 +++++++++++++++++++++---------------- drivers/gpu/drm/i915/i915_gem_gtt.c | 1 + 3 files changed, 26 insertions(+), 19 deletions(-)
drivers/gpu/drm/i915/i915_drv.h +1 -0
@@ -2798,6 +2798,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,int i915_vma_bind (struct i915_vma *vma,enum i915_cache_level cache_level,u32 flags); +void __i915_vma_set_map_and_fenceable (struct i915_vma *vma);int __must_check i915_vma_unbind (struct i915_vma *vma);int i915_gem_object_put_pages (struct drm_i915_gem_object *obj);void i915_gem_release_all_mmaps (struct drm_i915_private *dev_priv);
drivers/gpu/drm/i915/i915_gem.c +24 -19
55 lines not on this page · raw message
drivers/gpu/drm/i915/i915_gem_gtt.c +1 -0
@@ -2586,6 +2586,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,return ret; } vma->bound |=GLOBAL_BIND ; +__i915_vma_set_map_and_fenceable (vma); }/* Clear any non-preallocated blocks */
Signature · 1 line
2.5.0
Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects
Quoted 10 lines · Chris Wilson
As we mark the preallocated objects as bound, we should also flag them correctly as being map-and-fenceable (if appropriate!) so that latter users do not get confused and try and rebind the pinned vma in order to get a map-and-fenceable binding.
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>
Jani, can you please pick up both? And some bugzilla references for either would be great too - Chris?
Oh and does patch 1 fix the execlist resume troubles? Execlist having bigger contexts might be enough explanations for the apparent regression.
And can we igt patch 1 somehow? E.g. with memory pressure plus doing an mmap on the legacy fbdev ...
-Daniel
Quoted 94 lines
--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 43 +++++++++++++++++++++---------------- drivers/gpu/drm/i915/i915_gem_gtt.c | 1 + 3 files changed, 26 insertions(+), 19 deletions(-)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -2798,6 +2798,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,int i915_vma_bind (struct i915_vma *vma,enum i915_cache_level cache_level,u32 flags); +void __i915_vma_set_map_and_fenceable (struct i915_vma *vma);int __must_check i915_vma_unbind (struct i915_vma *vma);int i915_gem_object_put_pages (struct drm_i915_gem_object *obj);void i915_gem_release_all_mmaps (struct drm_i915_private *dev_priv);drivers/gpu/drm/i915/i915_gem.c +24
-19@@ -3980,6 +3980,29 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)return false; } +void __i915_vma_set_map_and_fenceable (struct i915_vma *vma) +{ +struct drm_i915_gem_object *obj = vma->obj; +bool mappable, fenceable; +u32 fence_size, fence_alignment; + + fence_size =i915_gem_get_gtt_size (obj->base.dev, + obj->base.size, + obj->tiling_mode); + fence_alignment =i915_gem_get_gtt_alignment (obj->base.dev, + obj->base.size, + obj->tiling_mode, + true); + + fenceable = (vma->node.size == fence_size && + (vma->node.start & (fence_alignment -1 )) ==0 ); + + mappable = (vma->node.start + fence_size <= +to_i915 (obj->base.dev)->gtt.mappable_end); + + obj->map_and_fenceable = mappable && fenceable; +} + static int i915_gem_object_do_pin(struct drm_i915_gem_object *obj, struct i915_address_space *vm,@@ -4047,25 +4070,7 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,if (ggtt_view && ggtt_view->type ==I915_GGTT_VIEW_NORMAL && (bound ^ vma->bound) &GLOBAL_BIND ) { -bool mappable, fenceable; -u32 fence_size, fence_alignment; - - fence_size =i915_gem_get_gtt_size (obj->base.dev, - obj->base.size, - obj->tiling_mode); - fence_alignment =i915_gem_get_gtt_alignment (obj->base.dev, - obj->base.size, - obj->tiling_mode, - true); - - fenceable = (vma->node.size == fence_size && - (vma->node.start & (fence_alignment -1 )) ==0 ); - - mappable = (vma->node.start + fence_size <= - dev_priv->gtt.mappable_end); - - obj->map_and_fenceable = mappable && fenceable; - +__i915_vma_set_map_and_fenceable (vma);WARN_ON (flags &PIN_MAPPABLE && !obj->map_and_fenceable); }drivers/gpu/drm/i915/i915_gem_gtt.c +1
-0@@ -2586,6 +2586,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,return ret; } vma->bound |=GLOBAL_BIND ; +__i915_vma_set_map_and_fenceable (vma); }/* Clear any non-preallocated blocks */ Signature · 1 line
2.5.0
Signature · 3 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects
Quoted 14 lines · Daniel Vetter
Quoted 10 lines · Chris Wilson
As we mark the preallocated objects as bound, we should also flag them correctly as being map-and-fenceable (if appropriate!) so that latter users do not get confused and try and rebind the pinned vma in order to get a map-and-fenceable binding.
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>Reviewed-by Daniel Vetter <[email protected]>
Quoted 8 lines
Jani, can you please pick up both? And some bugzilla references for either would be great too - Chris?
Oh and does patch 1 fix the execlist resume troubles? Execlist having bigger contexts might be enough explanations for the apparent regression.
And can we igt patch 1 somehow? E.g. with memory pressure plus doing an mmap on the legacy fbdev ...
Actually add Jani ...
-Daniel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects
In reply to Daniel VetterQuoted 12 lines · Daniel Vetter
Quoted 10 lines · Chris Wilson
As we mark the preallocated objects as bound, we should also flag them correctly as being map-and-fenceable (if appropriate!) so that latter users do not get confused and try and rebind the pinned vma in order to get a map-and-fenceable binding.
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>
Quoted 4 lines
Reviewed-by Daniel Vetter <[email protected]>Jani, can you please pick up both? And some bugzilla references for either would be great too - Chris?
There's a few candidate "overwrite of address 0" bugs, but no way to really tell if that is the fbcon or userspace without trial and error.
Quoted 2 lines
Oh and does patch 1 fix the execlist resume troubles? Execlist having bigger contexts might be enough explanations for the apparent regression.
Hmm, plausible.
Quoted 2 lines
And can we igt patch 1 somehow? E.g. with memory pressure plus doing an mmap on the legacy fbdev ...
Look at i915_gem_framebuffer for the fbcon gtt address before/after aperture thrashing. Would also need to switch to a user framebuffer to drop the scanout pinning.
Or just assert that it is still pinned following a modeset away from fbcon.
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Jani Nikula
maintainerRe: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects
In reply to Daniel VetterQuoted 12 lines · Daniel Vetter
Quoted 10 lines · Chris Wilson
As we mark the preallocated objects as bound, we should also flag them correctly as being map-and-fenceable (if appropriate!) so that latter users do not get confused and try and rebind the pinned vma in order to get a map-and-fenceable binding.
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>
Quoted 4 lines
Reviewed-by Daniel Vetter <[email protected]>Jani, can you please pick up both? And some bugzilla references for either would be great too - Chris?
Both pushed to drm-intel-next-fixes. Thanks for the patches and review.
BR,
Jani.
Quoted 111 lines
Oh and does patch 1 fix the execlist resume troubles? Execlist having bigger contexts might be enough explanations for the apparent regression.
And can we igt patch 1 somehow? E.g. with memory pressure plus doing an mmap on the legacy fbdev ...
-DanielQuoted 94 lines
--- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_gem.c | 43 +++++++++++++++++++++---------------- drivers/gpu/drm/i915/i915_gem_gtt.c | 1 + 3 files changed, 26 insertions(+), 19 deletions(-)drivers/gpu/drm/i915/i915_drv.h +1
-0@@ -2798,6 +2798,7 @@ i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,int i915_vma_bind (struct i915_vma *vma,enum i915_cache_level cache_level,u32 flags); +void __i915_vma_set_map_and_fenceable (struct i915_vma *vma);int __must_check i915_vma_unbind (struct i915_vma *vma);int i915_gem_object_put_pages (struct drm_i915_gem_object *obj);void i915_gem_release_all_mmaps (struct drm_i915_private *dev_priv);drivers/gpu/drm/i915/i915_gem.c +24
-19@@ -3980,6 +3980,29 @@ i915_vma_misplaced(struct i915_vma *vma, uint32_t alignment, uint64_t flags)return false; } +void __i915_vma_set_map_and_fenceable (struct i915_vma *vma) +{ +struct drm_i915_gem_object *obj = vma->obj; +bool mappable, fenceable; +u32 fence_size, fence_alignment; + + fence_size =i915_gem_get_gtt_size (obj->base.dev, + obj->base.size, + obj->tiling_mode); + fence_alignment =i915_gem_get_gtt_alignment (obj->base.dev, + obj->base.size, + obj->tiling_mode, + true); + + fenceable = (vma->node.size == fence_size && + (vma->node.start & (fence_alignment -1 )) ==0 ); + + mappable = (vma->node.start + fence_size <= +to_i915 (obj->base.dev)->gtt.mappable_end); + + obj->map_and_fenceable = mappable && fenceable; +} + static int i915_gem_object_do_pin(struct drm_i915_gem_object *obj, struct i915_address_space *vm,@@ -4047,25 +4070,7 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj,if (ggtt_view && ggtt_view->type ==I915_GGTT_VIEW_NORMAL && (bound ^ vma->bound) &GLOBAL_BIND ) { -bool mappable, fenceable; -u32 fence_size, fence_alignment; - - fence_size =i915_gem_get_gtt_size (obj->base.dev, - obj->base.size, - obj->tiling_mode); - fence_alignment =i915_gem_get_gtt_alignment (obj->base.dev, - obj->base.size, - obj->tiling_mode, - true); - - fenceable = (vma->node.size == fence_size && - (vma->node.start & (fence_alignment -1 )) ==0 ); - - mappable = (vma->node.start + fence_size <= - dev_priv->gtt.mappable_end); - - obj->map_and_fenceable = mappable && fenceable; - +__i915_vma_set_map_and_fenceable (vma);WARN_ON (flags &PIN_MAPPABLE && !obj->map_and_fenceable); }drivers/gpu/drm/i915/i915_gem_gtt.c +1
-0@@ -2586,6 +2586,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev,return ret; } vma->bound |=GLOBAL_BIND ; +__i915_vma_set_map_and_fenceable (vma); }/* Clear any non-preallocated blocks */ Signature · 1 line
2.5.0Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects
Quoted 13 lines · Jani Nikula
Quoted 12 lines · Daniel Vetter
Quoted 10 lines · Chris Wilson
As we mark the preallocated objects as bound, we should also flag them correctly as being map-and-fenceable (if appropriate!) so that latter users do not get confused and try and rebind the pinned vma in order to get a map-and-fenceable binding.
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>
Quoted 6 lines
Quoted 4 lines
Reviewed-by Daniel Vetter <[email protected]>Jani, can you please pick up both? And some bugzilla references for either would be great too - Chris?
Both pushed to drm-intel-next-fixes. Thanks for the patches and review.
This one breaks my HSW. I hit the warn in
int
i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj,
const struct i915_ggtt_view *view,
uint32_t alignment,
uint64_t flags)
{
if (WARN_ONCE(!view, "no view specified"))
return -EINVAL;
return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view,
alignment, flags | PIN_GLOBAL);
}and the fb console doesn't come up. Is this a merge error somehow? I don't see how it could have worked... maybe w/o fbdev enabled or something?
Thanks,
Jesse
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects
Quoted 37 lines · Jesse Barnes
Quoted 13 lines · Jani Nikula
Quoted 12 lines · Daniel Vetter
Quoted 10 lines · Chris Wilson
As we mark the preallocated objects as bound, we should also flag them correctly as being map-and-fenceable (if appropriate!) so that latter users do not get confused and try and rebind the pinned vma in order to get a map-and-fenceable binding.
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>Quoted 6 lines
Quoted 4 lines
Reviewed-by Daniel Vetter <[email protected]>Jani, can you please pick up both? And some bugzilla references for either would be great too - Chris?
Both pushed to drm-intel-next-fixes. Thanks for the patches and review.
This one breaks my HSW. I hit the warn in
int
i915_gem_object_ggtt_pin(structdrm_i915_gem_object*obj,const struct i915_ggtt_view *view, uint32_t alignment, uint64_t flags) { if (WARN_ONCE(!view, "no view specified")) return -EINVAL; return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view, alignment, flags | PIN_GLOBAL); }
Quoted 1 line
and the fb console doesn't come up. Is this a merge error somehow? I don't see how it could have worked... maybe w/o fbdev enabled or something?
No, it was just written against a different pin interface.
Use i915_gem_obj_ggtt_pin() instead.
Exactly.
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Jani Nikula
maintainerRe: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects
Quoted 38 lines · Chris Wilson
Quoted 37 lines · Jesse Barnes
Quoted 13 lines · Jani Nikula
Quoted 12 lines · Daniel Vetter
Quoted 10 lines · Chris Wilson
As we mark the preallocated objects as bound, we should also flag them correctly as being map-and-fenceable (if appropriate!) so that latter users do not get confused and try and rebind the pinned vma in order to get a map-and-fenceable binding.
Signed-off-by Chris Wilson <[email protected]>Cc "Goel, Akash" <[email protected]>Cc Daniel Vetter <[email protected]>Cc Jesse Barnes <[email protected]>Quoted 6 lines
Quoted 4 lines
Reviewed-by Daniel Vetter <[email protected]>Jani, can you please pick up both? And some bugzilla references for either would be great too - Chris?
Both pushed to drm-intel-next-fixes. Thanks for the patches and review.
This one breaks my HSW. I hit the warn in
int
i915_gem_object_ggtt_pin(structdrm_i915_gem_object*obj,const struct i915_ggtt_view *view, uint32_t alignment, uint64_t flags) { if (WARN_ONCE(!view, "no view specified")) return -EINVAL; return i915_gem_object_do_pin(obj, i915_obj_to_ggtt(obj), view, alignment, flags | PIN_GLOBAL); }
Quoted 7 lines
Quoted 1 line
and the fb console doesn't come up. Is this a merge error somehow? I don't see how it could have worked... maybe w/o fbdev enabled or something?
No, it was just written against a different pin interface.
Use
i915_gem_obj_ggtt_pin()instead.Exactly.
Both patches dropped from drm-intel-next-fixes.
BR,
Jani.
Quoted 4 lines
-Chris
Signature · 1 line
Chris Wilson, Intel Open Source Technology Centre
Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center _______________________________________________ Intel-gfx mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [PATCH] drm/i915: Set the map-and-fenceable flag for preallocated objects
In reply to Chris WilsonTask id: 7262 -------------------------------------Summary-------------------------------------
Platform Delta drm-intel-nightly Series Applied
ILK -1 302/302 301/302
SNB 315/315 315/315 IVB 336/336 336/336 BYT 283/283 283/283 HSW 378/378 378/378
-------------------------------------Detailed-------------------------------------
Platform Test drm-intel-nightly Series Applied
*ILK igt@kms_flip@flip-vs-dpms-interruptible PASS(1) DMESG_WARN(1)
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx