[Intel-gfx] [PATCH] drm/i915: Fix NULL plane->fb oops on SKL

Tue Jan 19 09:17:21 PST 2016

On Tue, Jan 19, 2016 at 04:30:48PM +0000, Tvrtko Ursulin wrote:
> 
> On 19/01/16 16:23, ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala at linux.intel.com>
> >
> > In this atomic age, we can't trust the plane->fb pointer anymore.
> > It might get update too late. Instead we are supposed to use the
> > plane_state->fb pointer instead. Let's do that in
> > intel_plane_obj_offset() and avoid problems from dereferencing the
> > potentially stale plane->fb pointer.
> 
> Sounds like in the atomic age :) plane->fb should not even exist then. 
> If it cannot be trusted from within skl_update_plane, which is at he 
> point state should be all ready for programming into the hardware, then 
> I see no point to it. At least to respect the principle of least surprise.

I think it mainly exists to keep existing drivers working until/if they
get converted over to atomic.

> 
> Only replying since it might have been me who put that code in and git 
> blame on intel_display.c takes ages.

It was Daniel in
commit ce7f17285639 ("drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly")
but as stated I tested that commit and it didn't hit the bug. So something
else must have changed since it went in.

> 
> Regards,
> 
> Tvrtko
> 
> > Paulo found this with 'kms_frontbuffer_tracking --show-hidden --run-subtest nop-1p-rte'
> > but it can be reproduced with just plain old kms_setplane.
> >
> > I was too lazy to bisect this, so not sure exactly when it broke. The
> > most obvious candidate
> > commit ce7f17285639 ("drm/i915: Fix i915_ggtt_view_equal to handle rotation correctly")
> > was actually still fine, so it must have broken some time after that.
> >
> > Here's the resulting fireworks:
> > BUG: unable to handle kernel NULL pointer dereference at           (null)
> > IP: [<ffffffffa02d2d9a>] intel_fill_fb_ggtt_view+0x1b/0x15a [i915]
> > PGD 8a5f6067 PUD 8a5f5067 PMD 0
> > Oops: 0000 [#1] PREEMPT SMP
> > Modules linked in: i915 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt fb_sys_fops drm intel_gtt agpgart netconsole mousedev hid_generic psmouse usbhid atkbd libps2 coretemp hwmon efi_pstore intel_rapl iosf_mbi x86_pkg_temp_thermal efivars pcspkr e1000e sdhci_pci ptp pps_core sdhci i2c_i801 mmc_core i2c_hid hid i8042 serio evdev sch_fq_codel ip_tables x_tables ipv6 autofs4
> > CPU: 1 PID: 260 Comm: kms_plane Not tainted 4.4.0-skl+ #171
> > Hardware name: Intel Corporation Skylake Client platform/Skylake Y LPDDR3 RVP3, BIOS SKLSE2R1.R00.B104.B00.1511030553 11/03/2015
> > task: ffff88008bde2d80 ti: ffff88008a6ec000 task.ti: ffff88008a6ec000
> > RIP: 0010:[<ffffffffa02d2d9a>]  [<ffffffffa02d2d9a>] intel_fill_fb_ggtt_view+0x1b/0x15a [i915]
> > RSP: 0018:ffff88008a6efa10  EFLAGS: 00010086
> > RAX: 0000000000000001 RBX: ffff8801674f4240 RCX: 0000000000000014
> > RDX: ffff88008a7440c0 RSI: 0000000000000000 RDI: ffff88008a6efa40
> > RBP: ffff88008a6efa30 R08: ffff88008bde3598 R09: 0000000000000001
> > R10: ffff88008b782000 R11: 0000000000000000 R12: 0000000000000000
> > R13: ffff88008a7440c0 R14: 0000000000000000 R15: ffff88008a7449c0
> > FS:  00007fa0c07a28c0(0000) GS:ffff88016ec40000(0000) knlGS:0000000000000000
> > CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > CR2: 0000000000000000 CR3: 000000008a6ff000 CR4: 00000000003406e0
> > DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
> > DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
> > Stack:
> >   ffff8801674f4240 0000000000000000 ffff88008a7440c0 0000000000000000
> >   ffff88008a6efaa0 ffffffffa02daf25 ffffffff814ec80e 0000000000070298
> >   ffff8800850d0000 ffff88008a6efaa0 ffffffffa02c49c2 0000000000000002
> > Call Trace:
> >   [<ffffffffa02daf25>] intel_plane_obj_offset+0x2d/0xa9 [i915]
> >   [<ffffffff814ec80e>] ? _raw_spin_unlock_irqrestore+0x4b/0x60
> >   [<ffffffffa02c49c2>] ? gen9_write32+0x2e8/0x3b8 [i915]
> >   [<ffffffffa02eecfc>] skl_update_plane+0x203/0x4c5 [i915]
> >   [<ffffffffa02ca1ab>] intel_plane_atomic_update+0x53/0x6a [i915]
> >   [<ffffffffa02494a4>] drm_atomic_helper_commit_planes_on_crtc+0x142/0x1d5 [drm_kms_helper]
> >   [<ffffffffa02de44b>] intel_atomic_commit+0x1262/0x1350 [i915]
> >   [<ffffffffa024a0ee>] ? __drm_atomic_helper_crtc_duplicate_state+0x2f/0x41 [drm_kms_helper]
> >   [<ffffffffa01ef089>] ? drm_atomic_check_only+0x3e3/0x552 [drm]
> >   [<ffffffffa01ef245>] drm_atomic_commit+0x4d/0x52 [drm]
> >   [<ffffffffa024996b>] drm_atomic_helper_update_plane+0xcb/0x118 [drm_kms_helper]
> >   [<ffffffffa01e42e8>] __setplane_internal+0x1c8/0x224 [drm]
> >   [<ffffffffa01e477f>] drm_mode_setplane+0x14e/0x172 [drm]
> >   [<ffffffffa01d8117>] drm_ioctl+0x265/0x3ad [drm]
> >   [<ffffffffa01e4631>] ? drm_mode_cursor_common+0x158/0x158 [drm]
> >   [<ffffffff810d00ab>] ? current_kernel_time64+0x5e/0x98
> >   [<ffffffff810a76ea>] ? trace_hardirqs_on_caller+0x17a/0x196
> >   [<ffffffff8119880f>] do_vfs_ioctl+0x42b/0x4ea
> >   [<ffffffff811a2b72>] ? __fget_light+0x4d/0x71
> >   [<ffffffff81198911>] SyS_ioctl+0x43/0x61
> >   [<ffffffff814ed057>] entry_SYSCALL_64_fastpath+0x12/0x6f
> >
> > Cc: drm-intel-fixes at lists.freedesktop.org
> > Cc: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Testcase: igt/kms_plane
> > Reported-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > ---
> >   drivers/gpu/drm/i915/intel_display.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index a851cb70479e..5bb960826cd1 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -2940,7 +2940,7 @@ u32 intel_plane_obj_offset(struct intel_plane *intel_plane,
> >   	struct i915_vma *vma;
> >   	u64 offset;
> >
> > -	intel_fill_fb_ggtt_view(&view, intel_plane->base.fb,
> > +	intel_fill_fb_ggtt_view(&view, intel_plane->base.state->fb,
> >   				intel_plane->base.state);
> >
> >   	vma = i915_gem_obj_to_ggtt_view(obj, &view);
> >

-- 
Ville Syrjälä
Intel OTC