[PATCH] drm/i915/bdw: Populate lrc with aliasing ppgtt if required

[PATCH] drm/i915/bdw: Populate lrc with aliasing ppgtt if required

[Thomas Daniel]

A previous commit broke aliasing PPGTT for lrc, resulting in a kernel oops
on boot. Add a check so that is full PPGTT is not in use the context is
populated with the aliasing PPGTT.

Issue: VIZ-4278
Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c |    7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c096b9b..79a6b91 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1452,12 +1452,19 @@ static int
 populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_obj,
 		    struct intel_engine_cs *ring, struct intel_ringbuffer *ringbuf)
 {
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *ring_obj = ringbuf->obj;
 	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
 	struct page *page;
 	uint32_t *reg_state;
 	int ret;
 
+	if (USES_FULL_PPGTT(dev))
+		ppgtt = ctx->ppgtt;
+	else
+		ppgtt = dev_priv->mm.aliasing_ppgtt;
+
 	ret = i915_gem_object_set_to_cpu_domain(ctx_obj, true);
 	if (ret) {
 		DRM_DEBUG_DRIVER("Could not set to CPU domain\n");
-- 
1.7.9.5

Re: [PATCH] drm/i915/bdw: Populate lrc with aliasing ppgtt if required

[Damien Lespiau]

On Mon, Aug 18, 2014 at 03:54:02PM +0100, Thomas Daniel wrote:
> A previous commit broke aliasing PPGTT for lrc, resulting in a kernel oops
> on boot. Add a check so that is full PPGTT is not in use the context is
> populated with the aliasing PPGTT.
> 
> Issue: VIZ-4278
> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c |    7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index c096b9b..79a6b91 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1452,12 +1452,19 @@ static int
>  populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_obj,
>  		    struct intel_engine_cs *ring, struct intel_ringbuffer *ringbuf)
>  {
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *ring_obj = ringbuf->obj;
>  	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;

It's a bit weird to leave ppgtt initialized here when you're going to
always override it below.

>  	struct page *page;
>  	uint32_t *reg_state;
>  	int ret;
>  
> +	if (USES_FULL_PPGTT(dev))
> +		ppgtt = ctx->ppgtt;
> +	else
> +		ppgtt = dev_priv->mm.aliasing_ppgtt;
> +

The patch drom daniel you mention removes the usage of USES_FULL_PPGTT()
to directly test ctx->ppgtt. You may want to reproduce that here ie.

	ppgtt = ctx->ppgtt;
	if (!ppgtt)
		ppgtt = dev_priv->mm.aliasing_ppgtt;

doesn't really matter either way I guess, but can we at least not set
then always override ppgtt?

Thanks,

-- 
Damien

Re: [PATCH] drm/i915/bdw: Populate lrc with aliasing ppgtt if required

[Daniel Vetter]

Ok, things start to dawn upon me - let's blame it on the lack of
decent coffee here ;-) I mixed up USES_PPGTT with USES_FULL_PPGTT
(again) so didn't realize that execlist works with just aliasing
ppgtt. So  I guess there's still room to clean up these macros a bit,
maybe do an s/USES_PPGTT/USES_HW_PPGTT/ or so, to make it clear that
this isn't about multiple address spaces.

I do like Damien's suggestion though since imo depending upon runtime
state is less fragile than rechecking driver option.
-Daniel

On Mon, Aug 18, 2014 at 6:49 PM, Damien Lespiau
<damien.lespiau@intel.com> wrote:
> On Mon, Aug 18, 2014 at 03:54:02PM +0100, Thomas Daniel wrote:
>> A previous commit broke aliasing PPGTT for lrc, resulting in a kernel oops
>> on boot. Add a check so that is full PPGTT is not in use the context is
>> populated with the aliasing PPGTT.
>>
>> Issue: VIZ-4278
>> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_lrc.c |    7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
>> index c096b9b..79a6b91 100644
>> --- a/drivers/gpu/drm/i915/intel_lrc.c
>> +++ b/drivers/gpu/drm/i915/intel_lrc.c
>> @@ -1452,12 +1452,19 @@ static int
>>  populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_obj,
>>                   struct intel_engine_cs *ring, struct intel_ringbuffer *ringbuf)
>>  {
>> +     struct drm_device *dev = ring->dev;
>> +     struct drm_i915_private *dev_priv = dev->dev_private;
>>       struct drm_i915_gem_object *ring_obj = ringbuf->obj;
>>       struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
>
> It's a bit weird to leave ppgtt initialized here when you're going to
> always override it below.
>
>>       struct page *page;
>>       uint32_t *reg_state;
>>       int ret;
>>
>> +     if (USES_FULL_PPGTT(dev))
>> +             ppgtt = ctx->ppgtt;
>> +     else
>> +             ppgtt = dev_priv->mm.aliasing_ppgtt;
>> +
>
> The patch drom daniel you mention removes the usage of USES_FULL_PPGTT()
> to directly test ctx->ppgtt. You may want to reproduce that here ie.
>
>         ppgtt = ctx->ppgtt;
>         if (!ppgtt)
>                 ppgtt = dev_priv->mm.aliasing_ppgtt;
>
> doesn't really matter either way I guess, but can we at least not set
> then always override ppgtt?
>
> Thanks,
>
> --
> Damien
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915/bdw: Populate lrc with aliasing ppgtt if required

[Thomas Daniel]

A previous commit broke aliasing PPGTT for lrc, resulting in a kernel oops
on boot. Add a check so that is full PPGTT is not in use the context is
populated with the aliasing PPGTT.

Issue: VIZ-4278
Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index c096b9b..b54f312 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1452,12 +1452,17 @@ static int
 populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_obj,
 		    struct intel_engine_cs *ring, struct intel_ringbuffer *ringbuf)
 {
+	struct drm_device *dev = ring->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *ring_obj = ringbuf->obj;
 	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
 	struct page *page;
 	uint32_t *reg_state;
 	int ret;
 
+	if (!ppgtt)
+		ppgtt = dev_priv->mm.aliasing_ppgtt;
+
 	ret = i915_gem_object_set_to_cpu_domain(ctx_obj, true);
 	if (ret) {
 		DRM_DEBUG_DRIVER("Could not set to CPU domain\n");
-- 
1.7.9.5

Re: [PATCH] drm/i915/bdw: Populate lrc with aliasing ppgtt if required

[Damien Lespiau]

On Tue, Aug 19, 2014 at 10:13:36AM +0100, Thomas Daniel wrote:
> A previous commit broke aliasing PPGTT for lrc, resulting in a kernel oops
> on boot. Add a check so that is full PPGTT is not in use the context is
> populated with the aliasing PPGTT.

Usually, we add a bit of history to the patch (even for trivial things)
something like:

v2: blah blah blah blah

But doesn't really matter much this time.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> Issue: VIZ-4278
> Signed-off-by: Thomas Daniel <thomas.daniel@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_lrc.c |    5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
> index c096b9b..b54f312 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1452,12 +1452,17 @@ static int
>  populate_lr_context(struct intel_context *ctx, struct drm_i915_gem_object *ctx_obj,
>  		    struct intel_engine_cs *ring, struct intel_ringbuffer *ringbuf)
>  {
> +	struct drm_device *dev = ring->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_i915_gem_object *ring_obj = ringbuf->obj;
>  	struct i915_hw_ppgtt *ppgtt = ctx->ppgtt;
>  	struct page *page;
>  	uint32_t *reg_state;
>  	int ret;
>  
> +	if (!ppgtt)
> +		ppgtt = dev_priv->mm.aliasing_ppgtt;
> +
>  	ret = i915_gem_object_set_to_cpu_domain(ctx_obj, true);
>  	if (ret) {
>  		DRM_DEBUG_DRIVER("Could not set to CPU domain\n");
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/bdw: Populate lrc with aliasing ppgtt if required

[Daniel Vetter]

On Tue, Aug 19, 2014 at 10:44:55AM +0100, Damien Lespiau wrote:
> On Tue, Aug 19, 2014 at 10:13:36AM +0100, Thomas Daniel wrote:
> > A previous commit broke aliasing PPGTT for lrc, resulting in a kernel oops
> > on boot. Add a check so that is full PPGTT is not in use the context is
> > populated with the aliasing PPGTT.
> 
> Usually, we add a bit of history to the patch (even for trivial things)
> something like:
> 
> v2: blah blah blah blah
> 
> But doesn't really matter much this time.
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch