[PATCH v2] drm/i915: Fix BUG in i915_gem.c when switch to console

[PATCH v2] drm/i915: Fix BUG in i915_gem.c when switch to console

[Xi Ruoyao]

In intel_crtc_page_flip, intel_display.c, the code changed the framebuffer
assigned to plane crtc->primary by

crtc->primary->fb = fb;

However, it forgot to change crtc->primary->state->fb. However, when we
switch to console, some kernel code will read crtc->primary->state->fb
to get the framebuffer assigned to crtc->primaty. Then a framebuffer
object can be unpinned twice and a kernel BUG will be produced in i915_gem.c.

So, update crtc->primary->state->fb in intel_display.c using
drm_atomic_set_fb_for_plane to fix the BUG.

Signed-off-by: Xi Ruoyao <xry111@outlook.com>
Fixed: Bug 93711 <https://bugzilla.kernel.org/show_bug.cgi?id=93711>
---
 Sorry, the previous patch is mangled by email client, so I am re-sending it.

 drivers/gpu/drm/i915/intel_display.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e730789..97083fd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -37,6 +37,7 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 #include "i915_trace.h"
+#include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_dp_helper.h>
 #include <drm/drm_crtc_helper.h>
@@ -9816,6 +9817,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
 	drm_gem_object_reference(&obj->base);
 
 	crtc->primary->fb = fb;
+	drm_atomic_set_fb_for_plane(crtc->primary->state, fb);
 
 	work->pending_flip_obj = obj;
 
-- 
1.9.1

Re: [PATCH v2] drm/i915: Fix BUG in i915_gem.c when switch to console

[Jani Nikula]

On Thu, 12 Mar 2015, Xi Ruoyao <xry111@outlook.com> wrote:
> In intel_crtc_page_flip, intel_display.c, the code changed the framebuffer
> assigned to plane crtc->primary by
>
> crtc->primary->fb = fb;
>
> However, it forgot to change crtc->primary->state->fb. However, when we
> switch to console, some kernel code will read crtc->primary->state->fb
> to get the framebuffer assigned to crtc->primaty. Then a framebuffer
> object can be unpinned twice and a kernel BUG will be produced in i915_gem.c.
>
> So, update crtc->primary->state->fb in intel_display.c using
> drm_atomic_set_fb_for_plane to fix the BUG.
>
> Signed-off-by: Xi Ruoyao <xry111@outlook.com>
> Fixed: Bug 93711 <https://bugzilla.kernel.org/show_bug.cgi?id=93711>

Is this a problem with drm-intel-nightly? In particular see

commit afd65eb4cc0578a9c07d621acdb8a570e2782bf7
Author: Matt Roper <matthew.d.roper@intel.com>
Date:   Tue Feb 3 13:10:04 2015 -0800

    drm/i915: Ensure plane->state->fb stays in sync with plane->fb

Matt, do you think this fixes the described issue? Can we backport to
drm-intel-fixes (and v4.0)?

BR,
Jani.



> ---
>  Sorry, the previous patch is mangled by email client, so I am re-sending it.
>
>  drivers/gpu/drm/i915/intel_display.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e730789..97083fd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -37,6 +37,7 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  #include "i915_trace.h"
> +#include <drm/drm_atomic.h>
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_dp_helper.h>
>  #include <drm/drm_crtc_helper.h>
> @@ -9816,6 +9817,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>  	drm_gem_object_reference(&obj->base);
>  
>  	crtc->primary->fb = fb;
> +	drm_atomic_set_fb_for_plane(crtc->primary->state, fb);
>  
>  	work->pending_flip_obj = obj;
>  
> -- 
> 1.9.1
>

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH v2] drm/i915: Fix BUG in i915_gem.c when switch to console

[Xi Ruoyao]

On 03/12/2015 04:28 PM, Jani Nikula wrote:
> On Thu, 12 Mar 2015, Xi Ruoyao <xry111@outlook.com> wrote:
>> In intel_crtc_page_flip, intel_display.c, the code changed the framebuffer
>> assigned to plane crtc->primary by
>>
>> crtc->primary->fb = fb;
>>
>> However, it forgot to change crtc->primary->state->fb. However, when we
>> switch to console, some kernel code will read crtc->primary->state->fb
>> to get the framebuffer assigned to crtc->primaty. Then a framebuffer
>> object can be unpinned twice and a kernel BUG will be produced in i915_gem.c.
>>
>> So, update crtc->primary->state->fb in intel_display.c using
>> drm_atomic_set_fb_for_plane to fix the BUG.
>>
>> Signed-off-by: Xi Ruoyao <xry111@outlook.com>
>> Fixed: Bug 93711 <https://bugzilla.kernel.org/show_bug.cgi?id=93711>
> Is this a problem with drm-intel-nightly? In particular see
>
> commit afd65eb4cc0578a9c07d621acdb8a570e2782bf7
> Author: Matt Roper <matthew.d.roper@intel.com>
> Date:   Tue Feb 3 13:10:04 2015 -0800
>
>      drm/i915: Ensure plane->state->fb stays in sync with plane->fb
>
> Matt, do you think this fixes the described issue? Can we backport to
> drm-intel-fixes (and v4.0)?
>
> BR,
> Jani.
>
>
>
>> ---
>>   Sorry, the previous patch is mangled by email client, so I am re-sending it.
>>
>>   drivers/gpu/drm/i915/intel_display.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index e730789..97083fd 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -37,6 +37,7 @@
>>   #include <drm/i915_drm.h>
>>   #include "i915_drv.h"
>>   #include "i915_trace.h"
>> +#include <drm/drm_atomic.h>
>>   #include <drm/drm_atomic_helper.h>
>>   #include <drm/drm_dp_helper.h>
>>   #include <drm/drm_crtc_helper.h>
>> @@ -9816,6 +9817,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
>>   	drm_gem_object_reference(&obj->base);
>>   
>>   	crtc->primary->fb = fb;
>> +	drm_atomic_set_fb_for_plane(crtc->primary->state, fb);
>>   
>>   	work->pending_flip_obj = obj;
>>   
>> -- 
>> 1.9.1
>>
Thank you to read my patch.

Matt's patch is better than mine since it fixed more conditions causing the
unsynchronize of plane->fb and plane->fb. But unfortunately, it cannot be
applied to mainline kernel (Linux 4.0.0-rc3) directly.

I'll try to apply Matt's changes to the mainline kernel.

Re: [PATCH v2] drm/i915: Fix BUG in i915_gem.c when switch to console

[Matt Roper]

On Thu, Mar 12, 2015 at 10:28:56AM +0200, Jani Nikula wrote:
> On Thu, 12 Mar 2015, Xi Ruoyao <xry111@outlook.com> wrote:
> > In intel_crtc_page_flip, intel_display.c, the code changed the framebuffer
> > assigned to plane crtc->primary by
> >
> > crtc->primary->fb = fb;
> >
> > However, it forgot to change crtc->primary->state->fb. However, when we
> > switch to console, some kernel code will read crtc->primary->state->fb
> > to get the framebuffer assigned to crtc->primaty. Then a framebuffer
> > object can be unpinned twice and a kernel BUG will be produced in i915_gem.c.
> >
> > So, update crtc->primary->state->fb in intel_display.c using
> > drm_atomic_set_fb_for_plane to fix the BUG.
> >
> > Signed-off-by: Xi Ruoyao <xry111@outlook.com>
> > Fixed: Bug 93711 <https://bugzilla.kernel.org/show_bug.cgi?id=93711>
> 
> Is this a problem with drm-intel-nightly? In particular see
> 
> commit afd65eb4cc0578a9c07d621acdb8a570e2782bf7
> Author: Matt Roper <matthew.d.roper@intel.com>
> Date:   Tue Feb 3 13:10:04 2015 -0800
> 
>     drm/i915: Ensure plane->state->fb stays in sync with plane->fb
> 
> Matt, do you think this fixes the described issue? Can we backport to
> drm-intel-fixes (and v4.0)?
> 
> BR,
> Jani.
> 

Yeah, Xi's patch should be the equivalent of my even earlier patch:

        commit db068420560511de80ac59222644f2bdf278c3d5
        Author: Matt Roper <matthew.d.roper@intel.com>
        Date:   Fri Jan 30 16:22:36 2015 -0800

            drm/i915: Keep plane->state updated on pageflip

so backporting either of those would be fine to solve the issue.  The
One you reference above also cleans up more places where we get out of
sync, but I'm not sure if any of those cause immediate problems; I think
those were just preparation for other -next work that was on the way.


Matt

> 
> 
> > ---
> >  Sorry, the previous patch is mangled by email client, so I am re-sending it.
> >
> >  drivers/gpu/drm/i915/intel_display.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index e730789..97083fd 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -37,6 +37,7 @@
> >  #include <drm/i915_drm.h>
> >  #include "i915_drv.h"
> >  #include "i915_trace.h"
> > +#include <drm/drm_atomic.h>
> >  #include <drm/drm_atomic_helper.h>
> >  #include <drm/drm_dp_helper.h>
> >  #include <drm/drm_crtc_helper.h>
> > @@ -9816,6 +9817,7 @@ static int intel_crtc_page_flip(struct drm_crtc *crtc,
> >  	drm_gem_object_reference(&obj->base);
> >  
> >  	crtc->primary->fb = fb;
> > +	drm_atomic_set_fb_for_plane(crtc->primary->state, fb);
> >  
> >  	work->pending_flip_obj = obj;
> >  
> > -- 
> > 1.9.1
> >
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795