[PATCH] drm/i915: Remove redundant get_pages call

[PATCH] drm/i915: Remove redundant get_pages call

[ankitprasad.r.sharma]

From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>

A call to i915_gem_obj_ggtt_pin is being made after this, which again
calls the get_pages function. Hence removing the redundant call to
get_pages.

Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
---
 drivers/gpu/drm/i915/i915_guc_submission.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
index 792d0b9..30237e2 100644
--- a/drivers/gpu/drm/i915/i915_guc_submission.c
+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
@@ -649,11 +649,6 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
 	if (!obj)
 		return NULL;
 
-	if (i915_gem_object_get_pages(obj)) {
-		drm_gem_object_unreference(&obj->base);
-		return NULL;
-	}
-
 	if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
 			PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
 		drm_gem_object_unreference(&obj->base);
-- 
1.9.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Remove redundant get_pages call

[Dave Gordon]

On 28/10/15 12:08, ankitprasad.r.sharma@intel.com wrote:
> From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
>
> A call to i915_gem_obj_ggtt_pin is being made after this, which again
> calls the get_pages function. Hence removing the redundant call to
> get_pages.
>
> Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_guc_submission.c | 5 -----
>   1 file changed, 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> index 792d0b9..30237e2 100644
> --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> @@ -649,11 +649,6 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
>   	if (!obj)
>   		return NULL;
>
> -	if (i915_gem_object_get_pages(obj)) {
> -		drm_gem_object_unreference(&obj->base);
> -		return NULL;
> -	}
> -
>   	if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
>   			PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
>   		drm_gem_object_unreference(&obj->base);

I suppose it is technically redundant, but it's actually quite difficult 
to verify that the call to i915_gem_obj_ggtt_pin() *will* actually take 
the path that, *six levels deeper*, includes the call to 
i915_gem_object_get_pages().

Is there any advantage to calling i915_gem_object_get_pages() later (or 
later)? Does it improve/worsen the chances of hitting a failure path? 
Handling an error from get_pages here is simple, whereas it looks like 
backing out of a failure in the middle of (six levels of) ggtt_pin might 
not be?

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Remove redundant get_pages call

[Daniel Vetter]

On Tue, Dec 01, 2015 at 06:11:16PM +0000, Dave Gordon wrote:
> On 28/10/15 12:08, ankitprasad.r.sharma@intel.com wrote:
> >From: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> >
> >A call to i915_gem_obj_ggtt_pin is being made after this, which again
> >calls the get_pages function. Hence removing the redundant call to
> >get_pages.
> >
> >Signed-off-by: Ankitprasad Sharma <ankitprasad.r.sharma@intel.com>
> >---
> >  drivers/gpu/drm/i915/i915_guc_submission.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> >index 792d0b9..30237e2 100644
> >--- a/drivers/gpu/drm/i915/i915_guc_submission.c
> >+++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> >@@ -649,11 +649,6 @@ static struct drm_i915_gem_object *gem_allocate_guc_obj(struct drm_device *dev,
> >  	if (!obj)
> >  		return NULL;
> >
> >-	if (i915_gem_object_get_pages(obj)) {
> >-		drm_gem_object_unreference(&obj->base);
> >-		return NULL;
> >-	}
> >-
> >  	if (i915_gem_obj_ggtt_pin(obj, PAGE_SIZE,
> >  			PIN_OFFSET_BIAS | GUC_WOPCM_TOP)) {
> >  		drm_gem_object_unreference(&obj->base);
> 
> I suppose it is technically redundant, but it's actually quite difficult to
> verify that the call to i915_gem_obj_ggtt_pin() *will* actually take the
> path that, *six levels deeper*, includes the call to
> i915_gem_object_get_pages().
> 
> Is there any advantage to calling i915_gem_object_get_pages() later (or
> later)? Does it improve/worsen the chances of hitting a failure path?
> Handling an error from get_pages here is simple, whereas it looks like
> backing out of a failure in the middle of (six levels of) ggtt_pin might not
> be?

pin into ggtt pretty much means that the backing storage must be there.
Yes we totally suck at documenting GEM, but then people also get dragged
away from upstream again before there's time to write docs or clean up
some of the incidental confusion that's sprinkled on top of the
fundamental design ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx