[PATCH 1/3] drm/i915: Only show view type for GGTT VMAs

[PATCH 1/3] drm/i915: Only show view type for GGTT VMAs

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Printing it for PPGTT VMAs only adds noise since we have defined
view types are only applicable for GGTT.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index fece922..9d36be8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -156,13 +156,13 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 	if (obj->fence_reg != I915_FENCE_REG_NONE)
 		seq_printf(m, " (fence: %d)", obj->fence_reg);
 	list_for_each_entry(vma, &obj->vma_list, vma_link) {
-		if (!i915_is_ggtt(vma->vm))
-			seq_puts(m, " (pp");
+		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
+			   i915_is_ggtt(vma->vm) ? "g" : "pp",
+			   vma->node.start, vma->node.size);
+		if (i915_is_ggtt(vma->vm))
+			seq_printf(m, ", type: %u)", vma->ggtt_view.type);
 		else
-			seq_puts(m, " (g");
-		seq_printf(m, "gtt offset: %08llx, size: %08llx, type: %u)",
-			   vma->node.start, vma->node.size,
-			   vma->ggtt_view.type);
+			seq_puts(m, ")");
 	}
 	if (obj->stolen)
 		seq_printf(m, " (stolen: %08llx)", obj->stolen->start);
-- 
2.4.0

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

[PATCH 2/3] drm/i915: Show human readable GGTT view name in debugfs

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Makes it a little bit more debug friendly not having to
remember which number is what.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++-----
 drivers/gpu/drm/i915/i915_gem_gtt.c | 11 +++++++++++
 drivers/gpu/drm/i915/i915_gem_gtt.h |  3 +++
 3 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 9d36be8..e26799c 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -156,13 +156,14 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
 	if (obj->fence_reg != I915_FENCE_REG_NONE)
 		seq_printf(m, " (fence: %d)", obj->fence_reg);
 	list_for_each_entry(vma, &obj->vma_list, vma_link) {
-		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
-			   i915_is_ggtt(vma->vm) ? "g" : "pp",
-			   vma->node.start, vma->node.size);
 		if (i915_is_ggtt(vma->vm))
-			seq_printf(m, ", type: %u)", vma->ggtt_view.type);
+			seq_printf(m, " (ggtt %s",
+				   i915_ggtt_view_name(vma->ggtt_view.type));
 		else
-			seq_puts(m, ")");
+			seq_puts(m, " (ppgtt");
+
+		seq_printf(m, " offset: %08llx, size: %08llx)",
+			   vma->node.start, vma->node.size);
 	}
 	if (obj->stolen)
 		seq_printf(m, " (stolen: %08llx)", obj->stolen->start);
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 17b7df0..e330e6c 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2904,3 +2904,14 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		return obj->base.size;
 	}
 }
+
+const char *i915_ggtt_view_name(enum i915_ggtt_view_type type)
+{
+	static const char *names[3] = { "normal",
+					"rotated",
+					"partial" };
+
+	BUILD_BUG_ON(ARRAY_SIZE(names) != I915_GGTT_VIEW_LAST);
+
+	return names[type];
+}
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
index 0d46dd2..13442e0 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.h
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
@@ -119,6 +119,7 @@ enum i915_ggtt_view_type {
 	I915_GGTT_VIEW_NORMAL = 0,
 	I915_GGTT_VIEW_ROTATED,
 	I915_GGTT_VIEW_PARTIAL,
+	I915_GGTT_VIEW_LAST /* Do not use and keep last. */
 };
 
 struct intel_rotation_info {
@@ -514,4 +515,6 @@ size_t
 i915_ggtt_view_size(struct drm_i915_gem_object *obj,
 		    const struct i915_ggtt_view *view);
 
+const char *i915_ggtt_view_name(enum i915_ggtt_view_type);
+
 #endif
-- 
2.4.0

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

Re: [PATCH 2/3] drm/i915: Show human readable GGTT view name in debugfs

[Joonas Lahtinen]

On ke, 2015-05-27 at 10:52 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Makes it a little bit more debug friendly not having to
> remember which number is what.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++-----
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 11 +++++++++++
>  drivers/gpu/drm/i915/i915_gem_gtt.h |  3 +++
>  3 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 9d36be8..e26799c 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -156,13 +156,14 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>  	if (obj->fence_reg != I915_FENCE_REG_NONE)
>  		seq_printf(m, " (fence: %d)", obj->fence_reg);
>  	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> -		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
> -			   i915_is_ggtt(vma->vm) ? "g" : "pp",
> -			   vma->node.start, vma->node.size);
>  		if (i915_is_ggtt(vma->vm))
> -			seq_printf(m, ", type: %u)", vma->ggtt_view.type);
> +			seq_printf(m, " (ggtt %s",
> +				   i915_ggtt_view_name(vma->ggtt_view.type));
>  		else
> -			seq_puts(m, ")");
> +			seq_puts(m, " (ppgtt");
> +
> +		seq_printf(m, " offset: %08llx, size: %08llx)",
> +			   vma->node.start, vma->node.size);
>  	}
>  	if (obj->stolen)
>  		seq_printf(m, " (stolen: %08llx)", obj->stolen->start);
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 17b7df0..e330e6c 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2904,3 +2904,14 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>  		return obj->base.size;
>  	}
>  }
> +
> +const char *i915_ggtt_view_name(enum i915_ggtt_view_type type)
> +{
> +	static const char *names[3] = { "normal",
> +					"rotated",
> +					"partial" };
> +
> +	BUILD_BUG_ON(ARRAY_SIZE(names) != I915_GGTT_VIEW_LAST);
> +
> +	return names[type];
> +}

I think this is bit of an overkill thing to do, especially as this if
for debugging. Looking at the DRM code elsewhere, an array of
i915_ggtt_view_type_names should suffice?

> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h
> index 0d46dd2..13442e0 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.h
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h
> @@ -119,6 +119,7 @@ enum i915_ggtt_view_type {
>  	I915_GGTT_VIEW_NORMAL = 0,
>  	I915_GGTT_VIEW_ROTATED,
>  	I915_GGTT_VIEW_PARTIAL,
> +	I915_GGTT_VIEW_LAST /* Do not use and keep last. */
>  };
>  
>  struct intel_rotation_info {
> @@ -514,4 +515,6 @@ size_t
>  i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>  		    const struct i915_ggtt_view *view);
>  
> +const char *i915_ggtt_view_name(enum i915_ggtt_view_type);
> +
>  #endif


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

Re: [PATCH 2/3] drm/i915: Show human readable GGTT view name in debugfs

[Tvrtko Ursulin]

On 05/28/2015 12:11 PM, Joonas Lahtinen wrote:
> On ke, 2015-05-27 at 10:52 +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Makes it a little bit more debug friendly not having to
>> remember which number is what.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_debugfs.c | 11 ++++++-----
>>   drivers/gpu/drm/i915/i915_gem_gtt.c | 11 +++++++++++
>>   drivers/gpu/drm/i915/i915_gem_gtt.h |  3 +++
>>   3 files changed, 20 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
>> index 9d36be8..e26799c 100644
>> --- a/drivers/gpu/drm/i915/i915_debugfs.c
>> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
>> @@ -156,13 +156,14 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>>   	if (obj->fence_reg != I915_FENCE_REG_NONE)
>>   		seq_printf(m, " (fence: %d)", obj->fence_reg);
>>   	list_for_each_entry(vma, &obj->vma_list, vma_link) {
>> -		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
>> -			   i915_is_ggtt(vma->vm) ? "g" : "pp",
>> -			   vma->node.start, vma->node.size);
>>   		if (i915_is_ggtt(vma->vm))
>> -			seq_printf(m, ", type: %u)", vma->ggtt_view.type);
>> +			seq_printf(m, " (ggtt %s",
>> +				   i915_ggtt_view_name(vma->ggtt_view.type));
>>   		else
>> -			seq_puts(m, ")");
>> +			seq_puts(m, " (ppgtt");
>> +
>> +		seq_printf(m, " offset: %08llx, size: %08llx)",
>> +			   vma->node.start, vma->node.size);
>>   	}
>>   	if (obj->stolen)
>>   		seq_printf(m, " (stolen: %08llx)", obj->stolen->start);
>> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> index 17b7df0..e330e6c 100644
>> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
>> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
>> @@ -2904,3 +2904,14 @@ i915_ggtt_view_size(struct drm_i915_gem_object *obj,
>>   		return obj->base.size;
>>   	}
>>   }
>> +
>> +const char *i915_ggtt_view_name(enum i915_ggtt_view_type type)
>> +{
>> +	static const char *names[3] = { "normal",
>> +					"rotated",
>> +					"partial" };
>> +
>> +	BUILD_BUG_ON(ARRAY_SIZE(names) != I915_GGTT_VIEW_LAST);
>> +
>> +	return names[type];
>> +}
>
> I think this is bit of an overkill thing to do, especially as this if
> for debugging. Looking at the DRM code elsewhere, an array of
> i915_ggtt_view_type_names should suffice?

I just thought it is a handy way of keeping the BUILD_BUG_ON close to 
the array. One thing less scattered around since enum and and string 
array is unavoidable.

Regards,

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

[PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Tvrtko Ursulin]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

These are the display call sites so should use the proper helper.

Also requires intel_plane_obj_offset to assume normal view when
plane pointer is not available.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 8 +++++---
 drivers/gpu/drm/i915/intel_fbdev.c   | 9 +++++----
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 657a333..0dc872b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2626,7 +2626,8 @@ intel_find_initial_plane_obj(struct intel_crtc *intel_crtc,
 			continue;
 
 		obj = intel_fb_obj(fb);
-		if (i915_gem_obj_ggtt_offset(obj) == plane_config->base) {
+		if (intel_plane_obj_offset(to_intel_plane(c->primary), obj) ==
+		    plane_config->base) {
 			drm_framebuffer_reference(fb);
 			goto valid_fb;
 		}
@@ -2911,7 +2912,8 @@ unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
 {
 	const struct i915_ggtt_view *view = &i915_ggtt_view_normal;
 
-	if (intel_rotation_90_or_270(intel_plane->base.state->rotation))
+	if (intel_plane &&
+	    intel_rotation_90_or_270(intel_plane->base.state->rotation))
 		view = &i915_ggtt_view_rotated;
 
 	return i915_gem_obj_ggtt_offset_view(obj, view);
@@ -13661,7 +13663,7 @@ intel_commit_cursor_plane(struct drm_plane *plane,
 	if (!obj)
 		addr = 0;
 	else if (!INTEL_INFO(dev)->cursor_needs_physical)
-		addr = i915_gem_obj_ggtt_offset(obj);
+		addr = intel_plane_obj_offset(to_intel_plane(plane), obj);
 	else
 		addr = obj->phys_handle->busaddr;
 
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c b/drivers/gpu/drm/i915/intel_fbdev.c
index 4e7e7da..4338324 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -180,6 +180,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	struct fb_info *info;
 	struct drm_framebuffer *fb;
 	struct drm_i915_gem_object *obj;
+	unsigned long disp_addr;
 	int size, ret;
 	bool prealloc = false;
 
@@ -243,12 +244,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
 	info->apertures->ranges[0].base = dev->mode_config.fb_base;
 	info->apertures->ranges[0].size = dev_priv->gtt.mappable_end;
 
-	info->fix.smem_start = dev->mode_config.fb_base + i915_gem_obj_ggtt_offset(obj);
+	disp_addr = intel_plane_obj_offset(NULL, obj);
+	info->fix.smem_start = dev->mode_config.fb_base + disp_addr;
 	info->fix.smem_len = size;
 
 	info->screen_base =
-		ioremap_wc(dev_priv->gtt.mappable_base + i915_gem_obj_ggtt_offset(obj),
-			   size);
+		ioremap_wc(dev_priv->gtt.mappable_base + disp_addr, size);
 	if (!info->screen_base) {
 		ret = -ENOSPC;
 		goto out_unpin;
@@ -272,7 +273,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
 
 	DRM_DEBUG_KMS("allocated %dx%d fb: 0x%08lx, bo %p\n",
 		      fb->width, fb->height,
-		      i915_gem_obj_ggtt_offset(obj), obj);
+		      disp_addr, obj);
 
 	mutex_unlock(&dev->struct_mutex);
 	vga_switcheroo_client_fb_set(dev->pdev, info);
-- 
2.4.0

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

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Chris Wilson]

On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> These are the display call sites so should use the proper helper.
> 
> Also requires intel_plane_obj_offset to assume normal view when
> plane pointer is not available.

Eugh. If only the plane stored the offset, bonus marks for storing the
vma cookie, then we would not have to keep recomputing the view and
searching every single time...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Tvrtko Ursulin]

On 05/27/2015 10:15 PM, Chris Wilson wrote:
> On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> These are the display call sites so should use the proper helper.
>>
>> Also requires intel_plane_obj_offset to assume normal view when
>> plane pointer is not available.
>
> Eugh. If only the plane stored the offset, bonus marks for storing the
> vma cookie, then we would not have to keep recomputing the view and
> searching every single time...

Well, the patch even decreases the number of searches! :)

And we don't recompute when querying the offset - it just figures out 
what type of vma it should look for. So I think it doesn't prevent any 
future caching improvements. It actually makes it easier since it 
consolidates the query.

I'll need this, or something like it, for some future work. So at the 
very moment I am not too bothered if this goes in or not.

Regards,

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

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Daniel Vetter]

On Thu, May 28, 2015 at 09:58:30AM +0100, Tvrtko Ursulin wrote:
> 
> On 05/27/2015 10:15 PM, Chris Wilson wrote:
> >On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
> >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>
> >>These are the display call sites so should use the proper helper.
> >>
> >>Also requires intel_plane_obj_offset to assume normal view when
> >>plane pointer is not available.
> >
> >Eugh. If only the plane stored the offset, bonus marks for storing the
> >vma cookie, then we would not have to keep recomputing the view and
> >searching every single time...
> 
> Well, the patch even decreases the number of searches! :)
> 
> And we don't recompute when querying the offset - it just figures out what
> type of vma it should look for. So I think it doesn't prevent any future
> caching improvements. It actually makes it easier since it consolidates the
> query.
> 
> I'll need this, or something like it, for some future work. So at the very
> moment I am not too bothered if this goes in or not.

Yeah unfortunately we can't eliminate the view searches completely since
the view depends upon fb + plane_state. So not perfectly aligned with our
hw ... Otherwise I'd agree, caching the view in the fb would be neat.
-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

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Chris Wilson]

On Thu, May 28, 2015 at 02:24:40PM +0200, Daniel Vetter wrote:
> On Thu, May 28, 2015 at 09:58:30AM +0100, Tvrtko Ursulin wrote:
> > 
> > On 05/27/2015 10:15 PM, Chris Wilson wrote:
> > >On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
> > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > >>
> > >>These are the display call sites so should use the proper helper.
> > >>
> > >>Also requires intel_plane_obj_offset to assume normal view when
> > >>plane pointer is not available.
> > >
> > >Eugh. If only the plane stored the offset, bonus marks for storing the
> > >vma cookie, then we would not have to keep recomputing the view and
> > >searching every single time...
> > 
> > Well, the patch even decreases the number of searches! :)
> > 
> > And we don't recompute when querying the offset - it just figures out what
> > type of vma it should look for. So I think it doesn't prevent any future
> > caching improvements. It actually makes it easier since it consolidates the
> > query.
> > 
> > I'll need this, or something like it, for some future work. So at the very
> > moment I am not too bothered if this goes in or not.
> 
> Yeah unfortunately we can't eliminate the view searches completely since
> the view depends upon fb + plane_state. So not perfectly aligned with our
> hw ... Otherwise I'd agree, caching the view in the fb would be neat.

Not in the fb, in the plane_state. We acquire the vma for the plane
during prepare and then we should be using that vma reference for the
lifetime of that atomic plane state.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Daniel Vetter]

On Thu, May 28, 2015 at 01:36:54PM +0100, Chris Wilson wrote:
> On Thu, May 28, 2015 at 02:24:40PM +0200, Daniel Vetter wrote:
> > On Thu, May 28, 2015 at 09:58:30AM +0100, Tvrtko Ursulin wrote:
> > > 
> > > On 05/27/2015 10:15 PM, Chris Wilson wrote:
> > > >On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
> > > >>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > > >>
> > > >>These are the display call sites so should use the proper helper.
> > > >>
> > > >>Also requires intel_plane_obj_offset to assume normal view when
> > > >>plane pointer is not available.
> > > >
> > > >Eugh. If only the plane stored the offset, bonus marks for storing the
> > > >vma cookie, then we would not have to keep recomputing the view and
> > > >searching every single time...
> > > 
> > > Well, the patch even decreases the number of searches! :)
> > > 
> > > And we don't recompute when querying the offset - it just figures out what
> > > type of vma it should look for. So I think it doesn't prevent any future
> > > caching improvements. It actually makes it easier since it consolidates the
> > > query.
> > > 
> > > I'll need this, or something like it, for some future work. So at the very
> > > moment I am not too bothered if this goes in or not.
> > 
> > Yeah unfortunately we can't eliminate the view searches completely since
> > the view depends upon fb + plane_state. So not perfectly aligned with our
> > hw ... Otherwise I'd agree, caching the view in the fb would be neat.
> 
> Not in the fb, in the plane_state. We acquire the vma for the plane
> during prepare and then we should be using that vma reference for the
> lifetime of that atomic plane state.

Hm right that should work. And the pin will make sure it won't go poof
prematurely.
-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

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Tvrtko Ursulin]

On 05/28/2015 03:09 PM, Daniel Vetter wrote:
> On Thu, May 28, 2015 at 01:36:54PM +0100, Chris Wilson wrote:
>> On Thu, May 28, 2015 at 02:24:40PM +0200, Daniel Vetter wrote:
>>> On Thu, May 28, 2015 at 09:58:30AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 05/27/2015 10:15 PM, Chris Wilson wrote:
>>>>> On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>
>>>>>> These are the display call sites so should use the proper helper.
>>>>>>
>>>>>> Also requires intel_plane_obj_offset to assume normal view when
>>>>>> plane pointer is not available.
>>>>>
>>>>> Eugh. If only the plane stored the offset, bonus marks for storing the
>>>>> vma cookie, then we would not have to keep recomputing the view and
>>>>> searching every single time...
>>>>
>>>> Well, the patch even decreases the number of searches! :)
>>>>
>>>> And we don't recompute when querying the offset - it just figures out what
>>>> type of vma it should look for. So I think it doesn't prevent any future
>>>> caching improvements. It actually makes it easier since it consolidates the
>>>> query.
>>>>
>>>> I'll need this, or something like it, for some future work. So at the very
>>>> moment I am not too bothered if this goes in or not.
>>>
>>> Yeah unfortunately we can't eliminate the view searches completely since
>>> the view depends upon fb + plane_state. So not perfectly aligned with our
>>> hw ... Otherwise I'd agree, caching the view in the fb would be neat.
>>
>> Not in the fb, in the plane_state. We acquire the vma for the plane
>> during prepare and then we should be using that vma reference for the
>> lifetime of that atomic plane state.
>
> Hm right that should work. And the pin will make sure it won't go poof
> prematurely.

Maybe I could do this, but at the moment I have no idea how that would 
work from intelfb_alloc who calls pin_and_fence with no state.

I could keep intel_plan_obj_offsets at all call sites and either return 
cached (say plane_state->disp_addr) address or "compute" and cache it, 
but, how would I know cached address is valid or not on startup?

I assume plane state is all initialized to zeros? And zero as a display 
address is valid AFAIK?

Do we have a intel_plane_state "constructor" somewhere which could set 
"intel_plane_state->disp_addr = -1", and so later intel_plane_obj_offset 
would know what to do?

And whatever can be done can also be done on top of this patch, which 
actually fixes things from the current state.

Regards,

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

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Chris Wilson]

On Tue, Jun 16, 2015 at 11:17:11AM +0100, Tvrtko Ursulin wrote:
> 
> On 05/28/2015 03:09 PM, Daniel Vetter wrote:
> >On Thu, May 28, 2015 at 01:36:54PM +0100, Chris Wilson wrote:
> >>On Thu, May 28, 2015 at 02:24:40PM +0200, Daniel Vetter wrote:
> >>>On Thu, May 28, 2015 at 09:58:30AM +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>>On 05/27/2015 10:15 PM, Chris Wilson wrote:
> >>>>>On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
> >>>>>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>>
> >>>>>>These are the display call sites so should use the proper helper.
> >>>>>>
> >>>>>>Also requires intel_plane_obj_offset to assume normal view when
> >>>>>>plane pointer is not available.
> >>>>>
> >>>>>Eugh. If only the plane stored the offset, bonus marks for storing the
> >>>>>vma cookie, then we would not have to keep recomputing the view and
> >>>>>searching every single time...
> >>>>
> >>>>Well, the patch even decreases the number of searches! :)
> >>>>
> >>>>And we don't recompute when querying the offset - it just figures out what
> >>>>type of vma it should look for. So I think it doesn't prevent any future
> >>>>caching improvements. It actually makes it easier since it consolidates the
> >>>>query.
> >>>>
> >>>>I'll need this, or something like it, for some future work. So at the very
> >>>>moment I am not too bothered if this goes in or not.
> >>>
> >>>Yeah unfortunately we can't eliminate the view searches completely since
> >>>the view depends upon fb + plane_state. So not perfectly aligned with our
> >>>hw ... Otherwise I'd agree, caching the view in the fb would be neat.
> >>
> >>Not in the fb, in the plane_state. We acquire the vma for the plane
> >>during prepare and then we should be using that vma reference for the
> >>lifetime of that atomic plane state.
> >
> >Hm right that should work. And the pin will make sure it won't go poof
> >prematurely.
> 
> Maybe I could do this, but at the moment I have no idea how that
> would work from intelfb_alloc who calls pin_and_fence with no state.

fbdev is a little special. All that we actually require is a plain
ggtt_pin to ensure that the fb is still around for panics. We can leave
the plane state in modesetting. We have have full control over the
struct we associated with the ifbdev, so can easily stash the vma in
there as well.

> I could keep intel_plan_obj_offsets at all call sites and either
> return cached (say plane_state->disp_addr) address or "compute" and
> cache it, but, how would I know cached address is valid or not on
> startup?

They all disappear. Prepare gets the vma, then commit can just use
vma->node.start + offset (and when the plane_state is released, so is
the ggtt pinning for the fb).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Chris Wilson]

On Tue, Jun 16, 2015 at 12:02:00PM +0100, Chris Wilson wrote:
> fbdev is a little special. All that we actually require is a plain
> ggtt_pin to ensure that the fb is still around for panics. We can leave
> the plane state in modesetting. We have have full control over the
> struct we associated with the ifbdev, so can easily stash the vma in
> there as well.

Or even from the atomic perspective, the plane state for fbdev can be
kept alive for emergency restoration. We still need to keep the ggtt_pin
for the persistent mmaping we use for fbcon.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Tvrtko Ursulin]

On 06/16/2015 12:02 PM, Chris Wilson wrote:
> On Tue, Jun 16, 2015 at 11:17:11AM +0100, Tvrtko Ursulin wrote:
>>
>> On 05/28/2015 03:09 PM, Daniel Vetter wrote:
>>> On Thu, May 28, 2015 at 01:36:54PM +0100, Chris Wilson wrote:
>>>> On Thu, May 28, 2015 at 02:24:40PM +0200, Daniel Vetter wrote:
>>>>> On Thu, May 28, 2015 at 09:58:30AM +0100, Tvrtko Ursulin wrote:
>>>>>>
>>>>>> On 05/27/2015 10:15 PM, Chris Wilson wrote:
>>>>>>> On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
>>>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>>>
>>>>>>>> These are the display call sites so should use the proper helper.
>>>>>>>>
>>>>>>>> Also requires intel_plane_obj_offset to assume normal view when
>>>>>>>> plane pointer is not available.
>>>>>>>
>>>>>>> Eugh. If only the plane stored the offset, bonus marks for storing the
>>>>>>> vma cookie, then we would not have to keep recomputing the view and
>>>>>>> searching every single time...
>>>>>>
>>>>>> Well, the patch even decreases the number of searches! :)
>>>>>>
>>>>>> And we don't recompute when querying the offset - it just figures out what
>>>>>> type of vma it should look for. So I think it doesn't prevent any future
>>>>>> caching improvements. It actually makes it easier since it consolidates the
>>>>>> query.
>>>>>>
>>>>>> I'll need this, or something like it, for some future work. So at the very
>>>>>> moment I am not too bothered if this goes in or not.
>>>>>
>>>>> Yeah unfortunately we can't eliminate the view searches completely since
>>>>> the view depends upon fb + plane_state. So not perfectly aligned with our
>>>>> hw ... Otherwise I'd agree, caching the view in the fb would be neat.
>>>>
>>>> Not in the fb, in the plane_state. We acquire the vma for the plane
>>>> during prepare and then we should be using that vma reference for the
>>>> lifetime of that atomic plane state.
>>>
>>> Hm right that should work. And the pin will make sure it won't go poof
>>> prematurely.
>>
>> Maybe I could do this, but at the moment I have no idea how that
>> would work from intelfb_alloc who calls pin_and_fence with no state.
>
> fbdev is a little special. All that we actually require is a plain
> ggtt_pin to ensure that the fb is still around for panics. We can leave
> the plane state in modesetting. We have have full control over the
> struct we associated with the ifbdev, so can easily stash the vma in
> there as well.

What do you mean by "We can leave the plane state in modesetting." ?

Where is the connection between ifbdev and plane state ie. our modeset?

>> I could keep intel_plan_obj_offsets at all call sites and either
>> return cached (say plane_state->disp_addr) address or "compute" and
>> cache it, but, how would I know cached address is valid or not on
>> startup?
>
> They all disappear. Prepare gets the vma, then commit can just use
> vma->node.start + offset (and when the plane_state is released, so is
> the ggtt pinning for the fb).

Yeah they disappear once one figures out how to handle fbdev paths. I am 
not there yet. :)

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

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Chris Wilson]

On Tue, Jun 16, 2015 at 12:18:49PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/16/2015 12:02 PM, Chris Wilson wrote:
> >On Tue, Jun 16, 2015 at 11:17:11AM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 05/28/2015 03:09 PM, Daniel Vetter wrote:
> >>>On Thu, May 28, 2015 at 01:36:54PM +0100, Chris Wilson wrote:
> >>>>On Thu, May 28, 2015 at 02:24:40PM +0200, Daniel Vetter wrote:
> >>>>>On Thu, May 28, 2015 at 09:58:30AM +0100, Tvrtko Ursulin wrote:
> >>>>>>
> >>>>>>On 05/27/2015 10:15 PM, Chris Wilson wrote:
> >>>>>>>On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
> >>>>>>>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>>>>
> >>>>>>>>These are the display call sites so should use the proper helper.
> >>>>>>>>
> >>>>>>>>Also requires intel_plane_obj_offset to assume normal view when
> >>>>>>>>plane pointer is not available.
> >>>>>>>
> >>>>>>>Eugh. If only the plane stored the offset, bonus marks for storing the
> >>>>>>>vma cookie, then we would not have to keep recomputing the view and
> >>>>>>>searching every single time...
> >>>>>>
> >>>>>>Well, the patch even decreases the number of searches! :)
> >>>>>>
> >>>>>>And we don't recompute when querying the offset - it just figures out what
> >>>>>>type of vma it should look for. So I think it doesn't prevent any future
> >>>>>>caching improvements. It actually makes it easier since it consolidates the
> >>>>>>query.
> >>>>>>
> >>>>>>I'll need this, or something like it, for some future work. So at the very
> >>>>>>moment I am not too bothered if this goes in or not.
> >>>>>
> >>>>>Yeah unfortunately we can't eliminate the view searches completely since
> >>>>>the view depends upon fb + plane_state. So not perfectly aligned with our
> >>>>>hw ... Otherwise I'd agree, caching the view in the fb would be neat.
> >>>>
> >>>>Not in the fb, in the plane_state. We acquire the vma for the plane
> >>>>during prepare and then we should be using that vma reference for the
> >>>>lifetime of that atomic plane state.
> >>>
> >>>Hm right that should work. And the pin will make sure it won't go poof
> >>>prematurely.
> >>
> >>Maybe I could do this, but at the moment I have no idea how that
> >>would work from intelfb_alloc who calls pin_and_fence with no state.
> >
> >fbdev is a little special. All that we actually require is a plain
> >ggtt_pin to ensure that the fb is still around for panics. We can leave
> >the plane state in modesetting. We have have full control over the
> >struct we associated with the ifbdev, so can easily stash the vma in
> >there as well.
> 
> What do you mean by "We can leave the plane state in modesetting." ?
> 
> Where is the connection between ifbdev and plane state ie. our modeset?

There is none. I thought you were arguing that the use of
pin_and_fence_fb in fbdev was problematic and that you wanted to use its
offset for the subsequent modesetting.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Tvrtko Ursulin]

On 06/16/2015 12:22 PM, Chris Wilson wrote:
> On Tue, Jun 16, 2015 at 12:18:49PM +0100, Tvrtko Ursulin wrote:
>>
>> On 06/16/2015 12:02 PM, Chris Wilson wrote:
>>> On Tue, Jun 16, 2015 at 11:17:11AM +0100, Tvrtko Ursulin wrote:
>>>>
>>>> On 05/28/2015 03:09 PM, Daniel Vetter wrote:
>>>>> On Thu, May 28, 2015 at 01:36:54PM +0100, Chris Wilson wrote:
>>>>>> On Thu, May 28, 2015 at 02:24:40PM +0200, Daniel Vetter wrote:
>>>>>>> On Thu, May 28, 2015 at 09:58:30AM +0100, Tvrtko Ursulin wrote:
>>>>>>>>
>>>>>>>> On 05/27/2015 10:15 PM, Chris Wilson wrote:
>>>>>>>>> On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
>>>>>>>>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>>>>>>>>
>>>>>>>>>> These are the display call sites so should use the proper helper.
>>>>>>>>>>
>>>>>>>>>> Also requires intel_plane_obj_offset to assume normal view when
>>>>>>>>>> plane pointer is not available.
>>>>>>>>>
>>>>>>>>> Eugh. If only the plane stored the offset, bonus marks for storing the
>>>>>>>>> vma cookie, then we would not have to keep recomputing the view and
>>>>>>>>> searching every single time...
>>>>>>>>
>>>>>>>> Well, the patch even decreases the number of searches! :)
>>>>>>>>
>>>>>>>> And we don't recompute when querying the offset - it just figures out what
>>>>>>>> type of vma it should look for. So I think it doesn't prevent any future
>>>>>>>> caching improvements. It actually makes it easier since it consolidates the
>>>>>>>> query.
>>>>>>>>
>>>>>>>> I'll need this, or something like it, for some future work. So at the very
>>>>>>>> moment I am not too bothered if this goes in or not.
>>>>>>>
>>>>>>> Yeah unfortunately we can't eliminate the view searches completely since
>>>>>>> the view depends upon fb + plane_state. So not perfectly aligned with our
>>>>>>> hw ... Otherwise I'd agree, caching the view in the fb would be neat.
>>>>>>
>>>>>> Not in the fb, in the plane_state. We acquire the vma for the plane
>>>>>> during prepare and then we should be using that vma reference for the
>>>>>> lifetime of that atomic plane state.
>>>>>
>>>>> Hm right that should work. And the pin will make sure it won't go poof
>>>>> prematurely.
>>>>
>>>> Maybe I could do this, but at the moment I have no idea how that
>>>> would work from intelfb_alloc who calls pin_and_fence with no state.
>>>
>>> fbdev is a little special. All that we actually require is a plain
>>> ggtt_pin to ensure that the fb is still around for panics. We can leave
>>> the plane state in modesetting. We have have full control over the
>>> struct we associated with the ifbdev, so can easily stash the vma in
>>> there as well.
>>
>> What do you mean by "We can leave the plane state in modesetting." ?
>>
>> Where is the connection between ifbdev and plane state ie. our modeset?
>
> There is none. I thought you were arguing that the use of
> pin_and_fence_fb in fbdev was problematic and that you wanted to use its
> offset for the subsequent modesetting.

That is partially correct, I do see it as problematic since I assumed 
someone will modeset with this fb/object at some point, and there will 
be state available then, which won't have the cached display address at 
all since the state is not present during fbdev setup.

Does that never happens? I mean, the modeset with state using the 
fb/object prepared in intefb_alloc?

Regards,

Tvrtko




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

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Chris Wilson]

On Tue, Jun 16, 2015 at 12:31:23PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/16/2015 12:22 PM, Chris Wilson wrote:
> >On Tue, Jun 16, 2015 at 12:18:49PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 06/16/2015 12:02 PM, Chris Wilson wrote:
> >>>On Tue, Jun 16, 2015 at 11:17:11AM +0100, Tvrtko Ursulin wrote:
> >>>>
> >>>>On 05/28/2015 03:09 PM, Daniel Vetter wrote:
> >>>>>On Thu, May 28, 2015 at 01:36:54PM +0100, Chris Wilson wrote:
> >>>>>>On Thu, May 28, 2015 at 02:24:40PM +0200, Daniel Vetter wrote:
> >>>>>>>On Thu, May 28, 2015 at 09:58:30AM +0100, Tvrtko Ursulin wrote:
> >>>>>>>>
> >>>>>>>>On 05/27/2015 10:15 PM, Chris Wilson wrote:
> >>>>>>>>>On Wed, May 27, 2015 at 10:52:34AM +0100, Tvrtko Ursulin wrote:
> >>>>>>>>>>From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>>>>>>>>
> >>>>>>>>>>These are the display call sites so should use the proper helper.
> >>>>>>>>>>
> >>>>>>>>>>Also requires intel_plane_obj_offset to assume normal view when
> >>>>>>>>>>plane pointer is not available.
> >>>>>>>>>
> >>>>>>>>>Eugh. If only the plane stored the offset, bonus marks for storing the
> >>>>>>>>>vma cookie, then we would not have to keep recomputing the view and
> >>>>>>>>>searching every single time...
> >>>>>>>>
> >>>>>>>>Well, the patch even decreases the number of searches! :)
> >>>>>>>>
> >>>>>>>>And we don't recompute when querying the offset - it just figures out what
> >>>>>>>>type of vma it should look for. So I think it doesn't prevent any future
> >>>>>>>>caching improvements. It actually makes it easier since it consolidates the
> >>>>>>>>query.
> >>>>>>>>
> >>>>>>>>I'll need this, or something like it, for some future work. So at the very
> >>>>>>>>moment I am not too bothered if this goes in or not.
> >>>>>>>
> >>>>>>>Yeah unfortunately we can't eliminate the view searches completely since
> >>>>>>>the view depends upon fb + plane_state. So not perfectly aligned with our
> >>>>>>>hw ... Otherwise I'd agree, caching the view in the fb would be neat.
> >>>>>>
> >>>>>>Not in the fb, in the plane_state. We acquire the vma for the plane
> >>>>>>during prepare and then we should be using that vma reference for the
> >>>>>>lifetime of that atomic plane state.
> >>>>>
> >>>>>Hm right that should work. And the pin will make sure it won't go poof
> >>>>>prematurely.
> >>>>
> >>>>Maybe I could do this, but at the moment I have no idea how that
> >>>>would work from intelfb_alloc who calls pin_and_fence with no state.
> >>>
> >>>fbdev is a little special. All that we actually require is a plain
> >>>ggtt_pin to ensure that the fb is still around for panics. We can leave
> >>>the plane state in modesetting. We have have full control over the
> >>>struct we associated with the ifbdev, so can easily stash the vma in
> >>>there as well.
> >>
> >>What do you mean by "We can leave the plane state in modesetting." ?
> >>
> >>Where is the connection between ifbdev and plane state ie. our modeset?
> >
> >There is none. I thought you were arguing that the use of
> >pin_and_fence_fb in fbdev was problematic and that you wanted to use its
> >offset for the subsequent modesetting.
> 
> That is partially correct, I do see it as problematic since I
> assumed someone will modeset with this fb/object at some point, and
> there will be state available then, which won't have the cached
> display address at all since the state is not present during fbdev
> setup.
> 
> Does that never happens? I mean, the modeset with state using the
> fb/object prepared in intefb_alloc?

No. The setup in intelfb_alloc is only concerned with generating a GGTT
mmapping that is consistent with later use by modesetting. The important
detail is to make sure the alignment is correct (or else the modeset
will fail as it cannot move the object as it is already pinned).

As Ville has extracted the linear alignment, we can export that and all
pin_to_display directly so that we can set up the fbdev without the
confusion of calling intel_pin_and_fence_fb. Or we can just live with
the confustion and comment appropriately.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Tvrtko Ursulin]

On 06/16/2015 12:48 PM, Chris Wilson wrote:
> On Tue, Jun 16, 2015 at 12:31:23PM +0100, Tvrtko Ursulin wrote:
>> That is partially correct, I do see it as problematic since I
>> assumed someone will modeset with this fb/object at some point, and
>> there will be state available then, which won't have the cached
>> display address at all since the state is not present during fbdev
>> setup.
>>
>> Does that never happens? I mean, the modeset with state using the
>> fb/object prepared in intefb_alloc?
>
> No. The setup in intelfb_alloc is only concerned with generating a GGTT
> mmapping that is consistent with later use by modesetting. The important
> detail is to make sure the alignment is correct (or else the modeset
> will fail as it cannot move the object as it is already pinned).
>
> As Ville has extracted the linear alignment, we can export that and all
> pin_to_display directly so that we can set up the fbdev without the
> confusion of calling intel_pin_and_fence_fb. Or we can just live with
> the confustion and comment appropriately.

Ok, think I get it now. Will send three RFC patches shortly.

1/3 looks innocent but it actually a bugfix once display address caching 
come along.

2/3 is the caching itself.

3/3 is what is not yet needed today, but analogous to 1/3 it fixes a bug 
which will become apparent in the future.

If this looks more along the lines of what you had in mind I can polish 
the comments or whatnot. 80 char line breaks were especially ugly in 
some of them to very long variable names. :)

Regards,

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

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Chris Wilson]

On Tue, Jun 16, 2015 at 02:32:40PM +0100, Tvrtko Ursulin wrote:
> 
> On 06/16/2015 12:48 PM, Chris Wilson wrote:
> >On Tue, Jun 16, 2015 at 12:31:23PM +0100, Tvrtko Ursulin wrote:
> >>That is partially correct, I do see it as problematic since I
> >>assumed someone will modeset with this fb/object at some point, and
> >>there will be state available then, which won't have the cached
> >>display address at all since the state is not present during fbdev
> >>setup.
> >>
> >>Does that never happens? I mean, the modeset with state using the
> >>fb/object prepared in intefb_alloc?
> >
> >No. The setup in intelfb_alloc is only concerned with generating a GGTT
> >mmapping that is consistent with later use by modesetting. The important
> >detail is to make sure the alignment is correct (or else the modeset
> >will fail as it cannot move the object as it is already pinned).
> >
> >As Ville has extracted the linear alignment, we can export that and all
> >pin_to_display directly so that we can set up the fbdev without the
> >confusion of calling intel_pin_and_fence_fb. Or we can just live with
> >the confustion and comment appropriately.
> 
> Ok, think I get it now. Will send three RFC patches shortly.
> 
> 1/3 looks innocent but it actually a bugfix once display address
> caching come along.
> 
> 2/3 is the caching itself.
> 
> 3/3 is what is not yet needed today, but analogous to 1/3 it fixes a
> bug which will become apparent in the future.
> 
> If this looks more along the lines of what you had in mind I can
> polish the comments or whatnot. 80 char line breaks were especially
> ugly in some of them to very long variable names. :)

Not far enough. It's not actually about caching the display address,
it's about tracking the actual VMA reference allocated for the plane.

What I had in mind and suggested yonks ago is:

http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=2112f72b7adb94b0e038bbafe74a3c1ab6c851cf

Ignore the atomic interface changes, they are from a bygone era. The
important part is just in tracking vma and the
simplification/robustification that provides.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Tvrtko Ursulin]

On 06/16/2015 02:53 PM, Chris Wilson wrote:
> On Tue, Jun 16, 2015 at 02:32:40PM +0100, Tvrtko Ursulin wrote:
>>
>> On 06/16/2015 12:48 PM, Chris Wilson wrote:
>>> On Tue, Jun 16, 2015 at 12:31:23PM +0100, Tvrtko Ursulin wrote:
>>>> That is partially correct, I do see it as problematic since I
>>>> assumed someone will modeset with this fb/object at some point, and
>>>> there will be state available then, which won't have the cached
>>>> display address at all since the state is not present during fbdev
>>>> setup.
>>>>
>>>> Does that never happens? I mean, the modeset with state using the
>>>> fb/object prepared in intefb_alloc?
>>>
>>> No. The setup in intelfb_alloc is only concerned with generating a GGTT
>>> mmapping that is consistent with later use by modesetting. The important
>>> detail is to make sure the alignment is correct (or else the modeset
>>> will fail as it cannot move the object as it is already pinned).
>>>
>>> As Ville has extracted the linear alignment, we can export that and all
>>> pin_to_display directly so that we can set up the fbdev without the
>>> confusion of calling intel_pin_and_fence_fb. Or we can just live with
>>> the confustion and comment appropriately.
>>
>> Ok, think I get it now. Will send three RFC patches shortly.
>>
>> 1/3 looks innocent but it actually a bugfix once display address
>> caching come along.
>>
>> 2/3 is the caching itself.
>>
>> 3/3 is what is not yet needed today, but analogous to 1/3 it fixes a
>> bug which will become apparent in the future.
>>
>> If this looks more along the lines of what you had in mind I can
>> polish the comments or whatnot. 80 char line breaks were especially
>> ugly in some of them to very long variable names. :)
>
> Not far enough. It's not actually about caching the display address,
> it's about tracking the actual VMA reference allocated for the plane.
>
> What I had in mind and suggested yonks ago is:
>
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=2112f72b7adb94b0e038bbafe74a3c1ab6c851cf
>
> Ignore the atomic interface changes, they are from a bygone era. The
> important part is just in tracking vma and the
> simplification/robustification that provides.

Not bad, looks good to me on the high level. Especially the unpin path 
is much nicer with this approach. My only uncertainty is whether 
stuffing object pointers into the state is acceptable with the architect 
of those parts.

Regards,

Tvrtko



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

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Chris Wilson]

On Tue, Jun 16, 2015 at 04:10:19PM +0100, Tvrtko Ursulin wrote:
> 
> 
> On 06/16/2015 02:53 PM, Chris Wilson wrote:
> >On Tue, Jun 16, 2015 at 02:32:40PM +0100, Tvrtko Ursulin wrote:
> >>
> >>On 06/16/2015 12:48 PM, Chris Wilson wrote:
> >>>On Tue, Jun 16, 2015 at 12:31:23PM +0100, Tvrtko Ursulin wrote:
> >>>>That is partially correct, I do see it as problematic since I
> >>>>assumed someone will modeset with this fb/object at some point, and
> >>>>there will be state available then, which won't have the cached
> >>>>display address at all since the state is not present during fbdev
> >>>>setup.
> >>>>
> >>>>Does that never happens? I mean, the modeset with state using the
> >>>>fb/object prepared in intefb_alloc?
> >>>
> >>>No. The setup in intelfb_alloc is only concerned with generating a GGTT
> >>>mmapping that is consistent with later use by modesetting. The important
> >>>detail is to make sure the alignment is correct (or else the modeset
> >>>will fail as it cannot move the object as it is already pinned).
> >>>
> >>>As Ville has extracted the linear alignment, we can export that and all
> >>>pin_to_display directly so that we can set up the fbdev without the
> >>>confusion of calling intel_pin_and_fence_fb. Or we can just live with
> >>>the confustion and comment appropriately.
> >>
> >>Ok, think I get it now. Will send three RFC patches shortly.
> >>
> >>1/3 looks innocent but it actually a bugfix once display address
> >>caching come along.
> >>
> >>2/3 is the caching itself.
> >>
> >>3/3 is what is not yet needed today, but analogous to 1/3 it fixes a
> >>bug which will become apparent in the future.
> >>
> >>If this looks more along the lines of what you had in mind I can
> >>polish the comments or whatnot. 80 char line breaks were especially
> >>ugly in some of them to very long variable names. :)
> >
> >Not far enough. It's not actually about caching the display address,
> >it's about tracking the actual VMA reference allocated for the plane.
> >
> >What I had in mind and suggested yonks ago is:
> >
> >http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=2112f72b7adb94b0e038bbafe74a3c1ab6c851cf
> >
> >Ignore the atomic interface changes, they are from a bygone era. The
> >important part is just in tracking vma and the
> >simplification/robustification that provides.
> 
> Not bad, looks good to me on the high level. Especially the unpin
> path is much nicer with this approach. My only uncertainty is
> whether stuffing object pointers into the state is acceptable with
> the architect of those parts.

Make it a unsigned long cookie then! :)

I thought Maarten was thinking of doing callbacks for duplicating state
which we could use to keep the pin_count accurate etc.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915: Use intel_plane_obj_offset from more places

[Daniel Vetter]

On Tue, Jun 16, 2015 at 05:07:46PM +0100, Chris Wilson wrote:
> On Tue, Jun 16, 2015 at 04:10:19PM +0100, Tvrtko Ursulin wrote:
> > 
> > 
> > On 06/16/2015 02:53 PM, Chris Wilson wrote:
> > >On Tue, Jun 16, 2015 at 02:32:40PM +0100, Tvrtko Ursulin wrote:
> > >>
> > >>On 06/16/2015 12:48 PM, Chris Wilson wrote:
> > >>>On Tue, Jun 16, 2015 at 12:31:23PM +0100, Tvrtko Ursulin wrote:
> > >>>>That is partially correct, I do see it as problematic since I
> > >>>>assumed someone will modeset with this fb/object at some point, and
> > >>>>there will be state available then, which won't have the cached
> > >>>>display address at all since the state is not present during fbdev
> > >>>>setup.
> > >>>>
> > >>>>Does that never happens? I mean, the modeset with state using the
> > >>>>fb/object prepared in intefb_alloc?
> > >>>
> > >>>No. The setup in intelfb_alloc is only concerned with generating a GGTT
> > >>>mmapping that is consistent with later use by modesetting. The important
> > >>>detail is to make sure the alignment is correct (or else the modeset
> > >>>will fail as it cannot move the object as it is already pinned).
> > >>>
> > >>>As Ville has extracted the linear alignment, we can export that and all
> > >>>pin_to_display directly so that we can set up the fbdev without the
> > >>>confusion of calling intel_pin_and_fence_fb. Or we can just live with
> > >>>the confustion and comment appropriately.
> > >>
> > >>Ok, think I get it now. Will send three RFC patches shortly.
> > >>
> > >>1/3 looks innocent but it actually a bugfix once display address
> > >>caching come along.
> > >>
> > >>2/3 is the caching itself.
> > >>
> > >>3/3 is what is not yet needed today, but analogous to 1/3 it fixes a
> > >>bug which will become apparent in the future.
> > >>
> > >>If this looks more along the lines of what you had in mind I can
> > >>polish the comments or whatnot. 80 char line breaks were especially
> > >>ugly in some of them to very long variable names. :)
> > >
> > >Not far enough. It's not actually about caching the display address,
> > >it's about tracking the actual VMA reference allocated for the plane.
> > >
> > >What I had in mind and suggested yonks ago is:
> > >
> > >http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=2112f72b7adb94b0e038bbafe74a3c1ab6c851cf
> > >
> > >Ignore the atomic interface changes, they are from a bygone era. The
> > >important part is just in tracking vma and the
> > >simplification/robustification that provides.
> > 
> > Not bad, looks good to me on the high level. Especially the unpin
> > path is much nicer with this approach. My only uncertainty is
> > whether stuffing object pointers into the state is acceptable with
> > the architect of those parts.
> 
> Make it a unsigned long cookie then! :)
> 
> I thought Maarten was thinking of doing callbacks for duplicating state
> which we could use to keep the pin_count accurate etc.

plane_state holds a reference on the fb, which means that won't go poof
before the plane state disappears. And the fb makes sure the obj doesn't
go poof. Which means we just need pin_count to make sure the vma doesn't
disapper.

atomic states have very clear ownership rules and an objects tangling off
them are properly refcounted. I don't see any issues here at all.
-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

Re: [PATCH 1/3] drm/i915: Only show view type for GGTT VMAs

[Joonas Lahtinen]

On ke, 2015-05-27 at 10:52 +0100, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> 
> Printing it for PPGTT VMAs only adds noise since we have defined
> view types are only applicable for GGTT.
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Comment below.

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index fece922..9d36be8 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -156,13 +156,13 @@ describe_obj(struct seq_file *m, struct drm_i915_gem_object *obj)
>  	if (obj->fence_reg != I915_FENCE_REG_NONE)
>  		seq_printf(m, " (fence: %d)", obj->fence_reg);
>  	list_for_each_entry(vma, &obj->vma_list, vma_link) {
> -		if (!i915_is_ggtt(vma->vm))
> -			seq_puts(m, " (pp");
> +		seq_printf(m, " (%sgtt offset: %08llx, size: %08llx",
> +			   i915_is_ggtt(vma->vm) ? "g" : "pp",
> +			   vma->node.start, vma->node.size);
> +		if (i915_is_ggtt(vma->vm))
> +			seq_printf(m, ", type: %u)", vma->ggtt_view.type);

The amount of minimalism is quite high here, but it was already before
the patch.

Splitting full words is not of my preference, because it makes grepping
for them harder for example when you trace the output text back to
generating code.

Regards, Joonas

>  		else
> -			seq_puts(m, " (g");
> -		seq_printf(m, "gtt offset: %08llx, size: %08llx, type: %u)",
> -			   vma->node.start, vma->node.size,
> -			   vma->ggtt_view.type);
> +			seq_puts(m, ")");
>  	}
>  	if (obj->stolen)
>  		seq_printf(m, " (stolen: %08llx)", obj->stolen->start);


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

Re: [PATCH 1/3] drm/i915: Only show view type for GGTT VMAs

[Daniel Vetter]

On Thu, May 28, 2015 at 02:00:39PM +0300, Joonas Lahtinen wrote:
> On ke, 2015-05-27 at 10:52 +0100, Tvrtko Ursulin wrote:
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > 
> > Printing it for PPGTT VMAs only adds noise since we have defined
> > view types are only applicable for GGTT.
> > 
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> Comment below.
> 
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Queued for -next, thanks for the patch.
-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