[PATCH] drm: idiot-proof vblank

[Daniel Vetter]

On Wed, Aug 06, 2014 at 01:16:59PM -0400, Rob Clark wrote:
> After spending slightly more time than I'd care to admit debugging the
> various and presumably spectacular way things fail when you pass too low
> a value to drm_vblank_init() (thanks console-lock for not letting me see
> the carnage!), I decided it might be a good idea to add some sanity
> checking.
> 
> Signed-off-by: Rob Clark 

Queued for -next, thanks for the patch.
-Daniel
> ---
> btw, I wonder if we could add a module param hack to toss initial modeset
> off to a workqueue to sneak it out from the tyranny of console_lock?
> 
>  drivers/gpu/drm/drm_irq.c | 24 
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 0de123a..6f16a104 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -730,6 +730,8 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp);
>   */
>  u32 drm_vblank_count(struct drm_device *dev, int crtc)
>  {
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return 0;
>   return atomic_read(>vblank[crtc].count);
>  }
>  EXPORT_SYMBOL(drm_vblank_count);
> @@ -752,6 +754,9 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int 
> crtc,
>  {
>   u32 cur_vblank;
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return 0;
> +
>   /* Read timestamp from slot of _vblank_time ringbuffer
>* that corresponds to current vblank count. Retry if
>* count has incremented during readout. This works like
> @@ -927,6 +932,9 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
>   unsigned long irqflags;
>   int ret = 0;
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return -EINVAL;
> +
>   spin_lock_irqsave(>vbl_lock, irqflags);
>   /* Going from 0->1 means we have to enable interrupts again */
>   if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>  {
>   BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return;
> +
>   /* Last user schedules interrupt disable */
>   if (atomic_dec_and_test(>vblank[crtc].refcount) &&
>   (drm_vblank_offdelay > 0))
> @@ -1019,6 +1030,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>   unsigned long irqflags;
>   unsigned int seq;
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return;
> +
>   spin_lock_irqsave(>vbl_lock, irqflags);
>   vblank_disable_and_save(dev, crtc);
>   wake_up(>vblank[crtc].queue);
> @@ -1078,6 +1092,9 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
>  {
>   unsigned long irqflags;
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return;
> +
>   spin_lock_irqsave(>vbl_lock, irqflags);
>   /* re-enable interrupts if there's are users left */
>   if (atomic_read(>vblank[crtc].refcount) != 0)
> @@ -1131,6 +1148,10 @@ void drm_vblank_pre_modeset(struct drm_device *dev, 
> int crtc)
>   /* vblank is not initialized (IRQ not installed ?), or has been freed */
>   if (!dev->num_crtcs)
>   return;
> +
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return;
> +
>   /*
>* To avoid all the problems that might happen if interrupts
>* were enabled/disabled around or between these calls, we just
> @@ -1439,6 +1460,9 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
>   if (!dev->num_crtcs)
>   return false;
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return false;
> +
>   /* Need timestamp lock to prevent concurrent execution with
>* vblank enable/disable, as this would cause inconsistent
>* or corrupted timestamps and vblank counts.
> -- 
> 1.9.3
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

i915 kernel warning

[Daniel Vetter]

On Wed, Aug 06, 2014 at 10:03:46PM +0300, Mihai Don?u wrote:
> Hi,
> 
> This just happened to me:
> 
> Aug  6 21:37:37 mdontu-l kernel: [ cut here ]
> Aug  6 21:37:37 mdontu-l kernel: WARNING: CPU: 3 PID: 4823 at 
> drivers/gpu/drm/i915/intel_display.c:3313 
> intel_crtc_wait_for_pending_flips+0x16c/0x180()
> Aug  6 21:37:37 mdontu-l kernel: Modules linked in: hdaps(O) tp_smapi(O) 
> thinkpad_ec(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) iwldvm iwlwifi e1000e
> Aug  6 21:37:37 mdontu-l kernel: CPU: 3 PID: 4823 Comm: X Tainted: G  
>  O  3.16.0-gentoo #2
> Aug  6 21:37:37 mdontu-l kernel: Hardware name: LENOVO 4178A4G/4178A4G, BIOS 
> 83ET76WW (1.46 ) 07/05/2013
> Aug  6 21:37:37 mdontu-l kernel:  0009 88041d63fbf8 
> 93c212a4 
> Aug  6 21:37:37 mdontu-l kernel:  88041d63fc30 930e3a2d 
>  880427b7c000
> Aug  6 21:37:37 mdontu-l kernel:  880426f380a8 880426ddf800 
> 880426ddf800 88041d63fc40
> Aug  6 21:37:37 mdontu-l kernel: Call Trace:
> Aug  6 21:37:37 mdontu-l kernel:  [] dump_stack+0x45/0x56
> Aug  6 21:37:37 mdontu-l kernel:  [] 
> warn_slowpath_common+0x7d/0xa0
> Aug  6 21:37:37 mdontu-l kernel:  [] 
> warn_slowpath_null+0x1a/0x20
> Aug  6 21:37:37 mdontu-l kernel:  [] 
> intel_crtc_wait_for_pending_flips+0x16c/0x180
> Aug  6 21:37:37 mdontu-l kernel:  [] ? 
> __wake_up_sync+0x20/0x20
> Aug  6 21:37:37 mdontu-l kernel:  [] 
> intel_crtc_disable_planes+0x33/0x1c0
> Aug  6 21:37:37 mdontu-l kernel:  [] 
> ironlake_crtc_disable+0x50/0x970
> Aug  6 21:37:37 mdontu-l kernel:  [] ? 
> drm_modeset_lock+0x2f/0xd0
> Aug  6 21:37:37 mdontu-l kernel:  [] ? mutex_lock+0x12/0x30
> Aug  6 21:37:37 mdontu-l kernel:  [] 
> intel_crtc_update_dpms+0x6f/0xa0
> Aug  6 21:37:37 mdontu-l kernel:  [] 
> intel_connector_dpms+0x59/0x70
> Aug  6 21:37:37 mdontu-l kernel:  [] 
> drm_mode_obj_set_property_ioctl+0x399/0x3a0
> Aug  6 21:37:37 mdontu-l kernel:  [] 
> drm_mode_connector_property_set_ioctl+0x30/0x40
> Aug  6 21:37:37 mdontu-l kernel:  [] drm_ioctl+0x1df/0x680
> Aug  6 21:37:37 mdontu-l kernel:  [] ? 
> backlight_generate_event+0x5c/0x80
> Aug  6 21:37:37 mdontu-l kernel:  [] ? 
> brightness_store+0x91/0xd0
> Aug  6 21:37:37 mdontu-l kernel:  [] ? fsnotify+0x27c/0x350
> Aug  6 21:37:37 mdontu-l kernel:  [] 
> do_vfs_ioctl+0x2c8/0x490
> Aug  6 21:37:37 mdontu-l kernel:  [] ? 
> __sb_end_write+0x31/0x60
> Aug  6 21:37:37 mdontu-l kernel:  [] ? vfs_write+0x1c2/0x200
> Aug  6 21:37:37 mdontu-l kernel:  [] SyS_ioctl+0x41/0x80
> Aug  6 21:37:37 mdontu-l kernel:  [] 
> system_call_fastpath+0x16/0x1b
> Aug  6 21:37:37 mdontu-l kernel: ---[ end trace 131b19886670715a ]---

If you can reproduce this somehow, please boot with drm.debug=0xe and
attach the complete dmesg up to the WARNING. We seem to loose track of
pageflips under obscure circumstances sometimes. We have more band-aids
queued up, but fixing the underlying issue would be better ofc.

Thanks, Daniel

> 
> X appeared to have stopped rendering (or nothing goes through), but the
> applications worked fine (I still had sound from a video). My video
> card is:
> 
> 00:02.0 VGA compatible controller: Intel Corporation 2nd Generation Core 
> Processor Family Integrated Graphics Controller (rev 09) (prog-if 00 [VGA 
> controller])
> Subsystem: Lenovo Device 21d0
> Flags: bus master, fast devsel, latency 0, IRQ 43
> Memory at f140 (64-bit, non-prefetchable) [size=4M]
> Memory at e000 (64-bit, prefetchable) [size=256M]
> I/O ports at 6000 [size=64]
> Expansion ROM at  [disabled]
> Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit-
> Capabilities: [d0] Power Management version 2
> Capabilities: [a4] PCI Advanced Features
> Kernel driver in use: i915
> 
> I was able to switch to the terminal and reboot.
> 
> I suspect this is new in 3.16 and I've noticed some other oddities
> since I upgraded from 3.15.8:
>  * when adjusting the screen brightness, the minimum setting is now
>"LCD off";
>  * when watching a video in fullscreen, I can see a flicker every 10s
>or so.
> 
> Thanks,
> 
> -- 
> Mihai Don?u
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH] drm: idiot-proof vblank

[Daniel Vetter]

On Wed, Aug 06, 2014 at 03:06:40PM -0400, Rob Clark wrote:
> On Wed, Aug 6, 2014 at 2:12 PM, Ville Syrj?l?
>  wrote:
> > On Wed, Aug 06, 2014 at 01:16:59PM -0400, Rob Clark wrote:
> >> After spending slightly more time than I'd care to admit debugging the
> >> various and presumably spectacular way things fail when you pass too low
> >> a value to drm_vblank_init() (thanks console-lock for not letting me see
> >> the carnage!), I decided it might be a good idea to add some sanity
> >> checking.
> >
> > Hmm. Could we instead do some kind of cross checking in
> > drm_vblank_init() and drm_crtc_init() to avoid having to sprinkle this
> > stuff all over the place? I guess the checks would need to happen both
> > ways since the driver might call those in either order.
> 
> it would be nice if we could just drop the arg to drm_vblank_init() to
> have one less place to screw things up.. I suspect that is some UMS
> relic..

It is.

> We could possibly co-check in vblank_init() each crtc_init()..  I
> guess it would be marginally fewer lines of diffstat, and in theory
> the driver could still manage to screw up by just passing bogus pipe
> #'s.  The vblank code already has these sort of checks for things that
> are potentially userspace facing, so adding the same check (plus
> WARN_ON()) to the internal fxns seemed like the logical and
> straightforward solution.

The problem is that currently don't have a point where we know that all
crtc are set up, so we can't just chuck the right drm_vblank_init call
into a modeset setup function.

But we could add a drm_mode_set_vblank_init which looks at
dev->mode_config.num_crtcs and for paranoia sets a flag which makes all
subsequent crtc_init fail loud.

In a perfect world we'd just move the vblank data into drm_crtc, but we're
not yet there. Ville' has already neatly split things up (mostly), and
I've started to consistently add and use vblank functions which take a
drm_crtc * instead of a pipe integer. But really not there yet by far.
-Daniel

> 
> BR,
> -R
> 
> >>
> >> Signed-off-by: Rob Clark 
> >> ---
> >> btw, I wonder if we could add a module param hack to toss initial modeset
> >> off to a workqueue to sneak it out from the tyranny of console_lock?
> >>
> >>  drivers/gpu/drm/drm_irq.c | 24 
> >>  1 file changed, 24 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> >> index 0de123a..6f16a104 100644
> >> --- a/drivers/gpu/drm/drm_irq.c
> >> +++ b/drivers/gpu/drm/drm_irq.c
> >> @@ -730,6 +730,8 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp);
> >>   */
> >>  u32 drm_vblank_count(struct drm_device *dev, int crtc)
> >>  {
> >> + if (WARN_ON(crtc >= dev->num_crtcs))
> >> + return 0;
> >>   return atomic_read(>vblank[crtc].count);
> >>  }
> >>  EXPORT_SYMBOL(drm_vblank_count);
> >> @@ -752,6 +754,9 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, 
> >> int crtc,
> >>  {
> >>   u32 cur_vblank;
> >>
> >> + if (WARN_ON(crtc >= dev->num_crtcs))
> >> + return 0;
> >> +
> >>   /* Read timestamp from slot of _vblank_time ringbuffer
> >>* that corresponds to current vblank count. Retry if
> >>* count has incremented during readout. This works like
> >> @@ -927,6 +932,9 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
> >>   unsigned long irqflags;
> >>   int ret = 0;
> >>
> >> + if (WARN_ON(crtc >= dev->num_crtcs))
> >> + return -EINVAL;
> >> +
> >>   spin_lock_irqsave(>vbl_lock, irqflags);
> >>   /* Going from 0->1 means we have to enable interrupts again */
> >>   if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
> >> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
> >>  {
> >>   BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);
> >>
> >> + if (WARN_ON(crtc >= dev->num_crtcs))
> >> + return;
> >> +
> >>   /* Last user schedules interrupt disable */
> >>   if (atomic_dec_and_test(>vblank[crtc].refcount) &&
> >>   (drm_vblank_offdelay > 0))
> >> @@ -1019,6 +1030,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
> >>   unsigned long irqflags;
> >>   unsigned int seq;
> >>
> >> + if (WARN_ON(crtc >= dev->num_crtcs))
> >> + return;
> >> +
> >>   spin_lock_irqsave(>vbl_lock, irqflags);
> >>   vblank_disable_and_save(dev, crtc);
> >>   wake_up(>vblank[crtc].queue);
> >> @@ -1078,6 +1092,9 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
> >>  {
> >>   unsigned long irqflags;
> >>
> >> + if (WARN_ON(crtc >= dev->num_crtcs))
> >> + return;
> >> +
> >>   spin_lock_irqsave(>vbl_lock, irqflags);
> >>   /* re-enable interrupts if there's are users left */
> >>   if (atomic_read(>vblank[crtc].refcount) != 0)
> >> @@ -1131,6 +1148,10 @@ void drm_vblank_pre_modeset(struct drm_device *dev, 
> >> int crtc)
> >>   /* vblank is not initialized (IRQ not installed ?), or 

[PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2

[Daniel Vetter]

On Wed, Aug 06, 2014 at 02:34:16PM -0400, Jerome Glisse wrote:
> On Wed, Aug 06, 2014 at 07:17:25PM +0200, Christian K?nig wrote:
> > Am 06.08.2014 um 18:08 schrieb Jerome Glisse:
> > >On Wed, Aug 06, 2014 at 08:55:28AM +0200, Christian K?nig wrote:
> > >>Am 06.08.2014 um 00:13 schrieb Jerome Glisse:
> > >>>On Tue, Aug 05, 2014 at 07:45:21PM +0200, Christian K?nig wrote:
> > Am 05.08.2014 um 19:39 schrieb Jerome Glisse:
> > >On Tue, Aug 05, 2014 at 06:05:29PM +0200, Christian K?nig wrote:
> > >>From: Christian K?nig 
> > >>
> > >>Avoid problems with writeback by limiting userptr to anonymous memory.
> > >>
> > >>v2: add commit and code comments
> > >I guess, i have not expressed myself clearly. This is bogus, you 
> > >pretend
> > >you want to avoid writeback issue but you still allow userspace to map
> > >file backed pages (which by the way might be a regular bo object from
> > >another device for instance and that would be fun).
> > >
> > >So this patch is a no go and i would rather see that this userptr to
> > >be restricted to anon vma only no matter what. No flags here.
> > Mapping of non anonymous memory (e.g. everything get_user_pages won't 
> > fail
> > with) is restricted to read only access by the GPU.
> > 
> > I'm fine with making it a hard requirement for all mappings if you say 
> > it's
> > a must have.
> > 
> > >>>Well for time being you should force read only. The way you implement 
> > >>>write
> > >>>is broken. Here is how it can abuse to allow write to a file backed mmap.
> > >>>
> > >>>mmap(fixaddress,fixedsize,NOFD)
> > >>>userptr_ioctl(fixedaddress, RADEON_GEM_USERPTR_ANONONLY)
> > >>>// bo is created successfully because fixedaddress is part of anonvma
> > >>>munmap(fixedaddress,fixedsize)
> > >>>// radeon get mmu_notifier_range_start callback and unbind page from the
> > >>>// bo but radeon does not know there was an unmap.
> > >>>mmap(fixaddress,fixedsize,fd_to_this_read_only_file_i_want_to_write_to)
> > >>>radeon_ioctl_use_my_userptrbo
> > >>>// bo is bind again by radeon and because all flag are set at creation
> > >>>// it is map with write permission allowing someone to write to a file
> > >>>// that might be read only for the user.
> > >>>//
> > >>>// Script kiddies it's time to learn about gpu ...
> > >>>
> > >>>Of course if you this patch (kind of selling my own junk here) :
> > >>>
> > >>>http://www.spinics.net/lists/linux-mm/msg75878.html
> > >>>
> > >>>then you could know inside the range_start that you should remove the
> > >>>write permission and that it should be rechecked on next bind.
> > >>>
> > >>>Note that i have not read much of your code so maybe you handle this
> > >>>case somehow.
> > >>I've stumbled over this attack vector as well and it's the reason why I've
> > >>moved checking the access rights to the bind callback instead of BO 
> > >>creation
> > >>time with V5 of the patch.
> > >>
> > >>This way you get an -EFAULT if you try something like this on command
> > >>submission time.
> > >So you seem immune to that issue but you are still not checking if the anon
> > >vma is writeable which you should again security concern here.
> > 
> > We check the access rights of the pointer using:
> > >if (!access_ok(write ? VERIFY_WRITE : VERIFY_READ,
> > >(long)gtt->userptr,
> > >   ttm->num_pages * PAGE_SIZE))
> > >return -EFAULT;
> > 
> > Shouldn't that be enough?
> 
> No, access_ok only check against special area on some architecture and i am
> pretty sure on x86 the VERIFY_WRITE or VERIFY_READ is just flat out ignored.
> 
> What you need to test is the vma vm_flags somethings like
> 
> if (write && !(vma->vm_flags VM_WRITE))
>return -EPERM;
> 
> Which need to happen on all bind.

access_ok is _only_ valid in combination with copy_from/to_user and
friends and is an optimization of the access checks depending upon
architecture. You always need them both, one alone is useless.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

i915 kernel warning

[Mihai Donțu]

Hi,

This just happened to me:

Aug  6 21:37:37 mdontu-l kernel: [ cut here ]
Aug  6 21:37:37 mdontu-l kernel: WARNING: CPU: 3 PID: 4823 at 
drivers/gpu/drm/i915/intel_display.c:3313 
intel_crtc_wait_for_pending_flips+0x16c/0x180()
Aug  6 21:37:37 mdontu-l kernel: Modules linked in: hdaps(O) tp_smapi(O) 
thinkpad_ec(O) vboxnetadp(O) vboxnetflt(O) vboxdrv(O) iwldvm iwlwifi e1000e
Aug  6 21:37:37 mdontu-l kernel: CPU: 3 PID: 4823 Comm: X Tainted: G   
O  3.16.0-gentoo #2
Aug  6 21:37:37 mdontu-l kernel: Hardware name: LENOVO 4178A4G/4178A4G, BIOS 
83ET76WW (1.46 ) 07/05/2013
Aug  6 21:37:37 mdontu-l kernel:  0009 88041d63fbf8 
93c212a4 
Aug  6 21:37:37 mdontu-l kernel:  88041d63fc30 930e3a2d 
 880427b7c000
Aug  6 21:37:37 mdontu-l kernel:  880426f380a8 880426ddf800 
880426ddf800 88041d63fc40
Aug  6 21:37:37 mdontu-l kernel: Call Trace:
Aug  6 21:37:37 mdontu-l kernel:  [] dump_stack+0x45/0x56
Aug  6 21:37:37 mdontu-l kernel:  [] 
warn_slowpath_common+0x7d/0xa0
Aug  6 21:37:37 mdontu-l kernel:  [] 
warn_slowpath_null+0x1a/0x20
Aug  6 21:37:37 mdontu-l kernel:  [] 
intel_crtc_wait_for_pending_flips+0x16c/0x180
Aug  6 21:37:37 mdontu-l kernel:  [] ? 
__wake_up_sync+0x20/0x20
Aug  6 21:37:37 mdontu-l kernel:  [] 
intel_crtc_disable_planes+0x33/0x1c0
Aug  6 21:37:37 mdontu-l kernel:  [] 
ironlake_crtc_disable+0x50/0x970
Aug  6 21:37:37 mdontu-l kernel:  [] ? 
drm_modeset_lock+0x2f/0xd0
Aug  6 21:37:37 mdontu-l kernel:  [] ? mutex_lock+0x12/0x30
Aug  6 21:37:37 mdontu-l kernel:  [] 
intel_crtc_update_dpms+0x6f/0xa0
Aug  6 21:37:37 mdontu-l kernel:  [] 
intel_connector_dpms+0x59/0x70
Aug  6 21:37:37 mdontu-l kernel:  [] 
drm_mode_obj_set_property_ioctl+0x399/0x3a0
Aug  6 21:37:37 mdontu-l kernel:  [] 
drm_mode_connector_property_set_ioctl+0x30/0x40
Aug  6 21:37:37 mdontu-l kernel:  [] drm_ioctl+0x1df/0x680
Aug  6 21:37:37 mdontu-l kernel:  [] ? 
backlight_generate_event+0x5c/0x80
Aug  6 21:37:37 mdontu-l kernel:  [] ? 
brightness_store+0x91/0xd0
Aug  6 21:37:37 mdontu-l kernel:  [] ? fsnotify+0x27c/0x350
Aug  6 21:37:37 mdontu-l kernel:  [] do_vfs_ioctl+0x2c8/0x490
Aug  6 21:37:37 mdontu-l kernel:  [] ? 
__sb_end_write+0x31/0x60
Aug  6 21:37:37 mdontu-l kernel:  [] ? vfs_write+0x1c2/0x200
Aug  6 21:37:37 mdontu-l kernel:  [] SyS_ioctl+0x41/0x80
Aug  6 21:37:37 mdontu-l kernel:  [] 
system_call_fastpath+0x16/0x1b
Aug  6 21:37:37 mdontu-l kernel: ---[ end trace 131b19886670715a ]---

X appeared to have stopped rendering (or nothing goes through), but the
applications worked fine (I still had sound from a video). My video
card is:

00:02.0 VGA compatible controller: Intel Corporation 2nd Generation Core 
Processor Family Integrated Graphics Controller (rev 09) (prog-if 00 [VGA 
controller])
Subsystem: Lenovo Device 21d0
Flags: bus master, fast devsel, latency 0, IRQ 43
Memory at f140 (64-bit, non-prefetchable) [size=4M]
Memory at e000 (64-bit, prefetchable) [size=256M]
I/O ports at 6000 [size=64]
Expansion ROM at  [disabled]
Capabilities: [90] MSI: Enable+ Count=1/1 Maskable- 64bit-
Capabilities: [d0] Power Management version 2
Capabilities: [a4] PCI Advanced Features
Kernel driver in use: i915

I was able to switch to the terminal and reboot.

I suspect this is new in 3.16 and I've noticed some other oddities
since I upgraded from 3.15.8:
 * when adjusting the screen brightness, the minimum setting is now
   "LCD off";
 * when watching a video in fullscreen, I can see a flicker every 10s
   or so.

Thanks,

-- 
Mihai Don?u

[PATCH] drm: Don't grab an fb reference for the idr

[Daniel Vetter]

On Wed, Aug 06, 2014 at 02:59:29PM -0400, Rob Clark wrote:
> On Wed, Aug 6, 2014 at 10:07 AM, Daniel Vetter  wrote:
> > On Wed, Aug 06, 2014 at 09:12:42AM -0400, Rob Clark wrote:
> >> On Wed, Aug 6, 2014 at 8:37 AM, Daniel Vetter  wrote:
> >> > On Wed, Aug 06, 2014 at 07:11:28AM -0400, Rob Clark wrote:
> >> >> On Wed, Aug 6, 2014 at 3:10 AM, Daniel Vetter  >> >> ffwll.ch> wrote:
> >> >> > The current refcounting scheme is that the fb lookup idr also holds a
> >> >> > reference. This works out nicely bacause thus far we've always
> >> >> > explicitly cleaned up idr entries for framebuffers:
> >> >> > - Userspace fbs get removed in the rmfb ioctl or when the drm file
> >> >> >   gets closed.
> >> >> > - Kernel fbs (for fbdev emulation) get cleaned up by the driver code
> >> >> >   at module unload time.
> >> >> >
> >> >> > But now i915 also reconstructs the bios fbs for a smooth transition.
> >> >> > And that fb is purely transitional and should get removed immmediately
> >> >> > once all crtcs stop using it. Of course if the i915 fbdev code decides
> >> >> > to reuse it as the main fbdev fb then it shouldn't be cleaned up, but
> >> >> > in that case the fbdev code will grab it's own reference.
> >> >> >
> >> >> > The problem is now that we also want to register that takeover fb in
> >> >> > the idr, so that userspace can do a smooth transition (animated maybe
> >> >> > even!) itself. But currently we have no one who will clean up the idr
> >> >> > reference once that fb isn't useful any more, and so essentially leak
> >> >> > it.
> >> >>
> >> >> ewww..  couldn't you do some scheme on lastclose to check if no more
> >> >> crtc's are scanning out that fb, and if not then remove the idr?
> >> >
> >> > There's no natural point really but when we drop the last reference for
> >> > it. Going the weak reference route looked the most natural. And I 
> >> > honestly
> >> > expect other drivers to eventually do the same - forcing a modeset on
> >> > boot-up is kinda not too pretty, and permanently reserving a big
> >> > framebuffer just for the bios doesn't sound good either. This approach
> >> > would nicely solve it for everyone.
> >>
> >> hmm, maybe somebody switched my coffee with decaf, but why isn't
> >> lastclose a natural point?
> >
> > There is no lastclose for the bios ;-)
> >
> > Let me elaborate on what happens:
> >
> > 1. BIOS sets up an initial config with a framebuffer in stolen.
> >
> > 2. i915 takes over and reconstructs all the state, so now we have all the
> > crtcs enabled using a framebuffer for all of them which wraps the bios
> > allocation.
> >
> > 2b. (optional) reuse that framebuffer for fbdev.
> >
> > -> That special bios fb has the following references:
> > - 1 reference for each crtc that's using it
> > - 1 optional reference if it's reused as the fbdev fb
> > - 1 reference for the idr
> >
> > 3. Userspace takes over, potentially doing a getfb on the current
> > (bios-inherited) fb for smooth transition, but then does a modeset to its
> > own fb.
> >
> > -> After this all the we've dropped the crtc references and we also want
> > to drop the idr reference (since no one will ever use this framebuffer
> > again). But there's simply no good place to do that. Lastclose might only
> > happen before we shut down the system again, which is a bit too late.
> 
> Well, you could still cleanup your idr reference on current
> userspace's lastclose.. that doesn't do much good, I suppose, if
> current userspace never exits.  But if first userspace is plymouth or
> something like that, you would get cleaned up on the
> splash->{x11/wayland} transition..

So plymouth starts, but it doesn't exit until X starts (since otherwise
the fb gets removed and the crtc shut off and we switch to fbcon). So
again no lastclose to link into. So I don't think any check in a close
callback will cut it.

> if that isn't sufficient, then yeah I guess the more fancy weak-ref
> stuff needed..

Trust me I don't like it either ;-) But for this we really have no natural
point to reap the lookup reference like with gem objects, or normal
userspace framebuffers.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[Bug 78453] [HAWAII] Get acceleration working

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=78453

--- Comment #141 from Luzipher  ---
The two patches fixing Unigine Heaven and Valley you posted today on the
mailing list, Marek, also solve the gpu crashes I have with World of Warcraft,
described briefly above.

Patches are in this mailing list thread:
http://lists.freedesktop.org/archives/mesa-dev/2014-August/064919.html

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/309ca314/attachment.html>

[PATCH] drm: idiot-proof vblank

[Ville Syrjälä]

On Wed, Aug 06, 2014 at 01:16:59PM -0400, Rob Clark wrote:
> After spending slightly more time than I'd care to admit debugging the
> various and presumably spectacular way things fail when you pass too low
> a value to drm_vblank_init() (thanks console-lock for not letting me see
> the carnage!), I decided it might be a good idea to add some sanity
> checking.

Hmm. Could we instead do some kind of cross checking in
drm_vblank_init() and drm_crtc_init() to avoid having to sprinkle this
stuff all over the place? I guess the checks would need to happen both
ways since the driver might call those in either order.

> 
> Signed-off-by: Rob Clark 
> ---
> btw, I wonder if we could add a module param hack to toss initial modeset
> off to a workqueue to sneak it out from the tyranny of console_lock?
> 
>  drivers/gpu/drm/drm_irq.c | 24 
>  1 file changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 0de123a..6f16a104 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -730,6 +730,8 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp);
>   */
>  u32 drm_vblank_count(struct drm_device *dev, int crtc)
>  {
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return 0;
>   return atomic_read(>vblank[crtc].count);
>  }
>  EXPORT_SYMBOL(drm_vblank_count);
> @@ -752,6 +754,9 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int 
> crtc,
>  {
>   u32 cur_vblank;
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return 0;
> +
>   /* Read timestamp from slot of _vblank_time ringbuffer
>* that corresponds to current vblank count. Retry if
>* count has incremented during readout. This works like
> @@ -927,6 +932,9 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
>   unsigned long irqflags;
>   int ret = 0;
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return -EINVAL;
> +
>   spin_lock_irqsave(>vbl_lock, irqflags);
>   /* Going from 0->1 means we have to enable interrupts again */
>   if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>  {
>   BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return;
> +
>   /* Last user schedules interrupt disable */
>   if (atomic_dec_and_test(>vblank[crtc].refcount) &&
>   (drm_vblank_offdelay > 0))
> @@ -1019,6 +1030,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>   unsigned long irqflags;
>   unsigned int seq;
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return;
> +
>   spin_lock_irqsave(>vbl_lock, irqflags);
>   vblank_disable_and_save(dev, crtc);
>   wake_up(>vblank[crtc].queue);
> @@ -1078,6 +1092,9 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
>  {
>   unsigned long irqflags;
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return;
> +
>   spin_lock_irqsave(>vbl_lock, irqflags);
>   /* re-enable interrupts if there's are users left */
>   if (atomic_read(>vblank[crtc].refcount) != 0)
> @@ -1131,6 +1148,10 @@ void drm_vblank_pre_modeset(struct drm_device *dev, 
> int crtc)
>   /* vblank is not initialized (IRQ not installed ?), or has been freed */
>   if (!dev->num_crtcs)
>   return;
> +
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return;
> +
>   /*
>* To avoid all the problems that might happen if interrupts
>* were enabled/disabled around or between these calls, we just
> @@ -1439,6 +1460,9 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
>   if (!dev->num_crtcs)
>   return false;
>  
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return false;
> +
>   /* Need timestamp lock to prevent concurrent execution with
>* vblank enable/disable, as this would cause inconsistent
>* or corrupted timestamps and vblank counts.
> -- 
> 1.9.3
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj?l?
Intel OTC

[Bug 81644] Random crashes on RadeonSI with Chromium.

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=81644

--- Comment #43 from jackdachef at gmail.com ---
(In reply to comment #42)
> I've been able to get lucky and not get a hard lock most of the time. You
> HAVE to wait 10 seconds for the drm to reset the GPU. The patch didn't
> improve or change anything, I've seen those results for a while.

any special commands you append to the radeon driver ? defaults (no special
settings) ?

is it compiled into the kernel ? initramfs ?


thanks, will wait next time for 10+ seconds and crossing fingers

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/eeaf534b/attachment.html>

[Bug 81644] Random crashes on RadeonSI with Chromium.

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=81644

--- Comment #42 from Aaron B  ---
I've been able to get lucky and not get a hard lock most of the time. You HAVE
to wait 10 seconds for the drm to reset the GPU. The patch didn't improve or
change anything, I've seen those results for a while.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/0125c97f/attachment.html>

[PATCH libdrm] drm: Implement drmCheckModesettingSupported() for DragonFly

[Francois Tigeot]

On Sat, Jul 26, 2014 at 01:39:58PM +0200, Fran?ois Tigeot wrote:
> For the sake of simplicity, KMS support can always be considered
> present on DragonFly.
> 
> If some particular version doesn't support KMS yet, appropriate
> checks are already done in Dports's x11-drivers/ Makefiles and
> KMS-enabled driver packages don't get built.
> 
> Signed-off-by: Fran?ois Tigeot 
> ---
>  xf86drmMode.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/xf86drmMode.c b/xf86drmMode.c
> index 7ca89b3..60ce369 100644
> --- a/xf86drmMode.c
> +++ b/xf86drmMode.c
> @@ -806,6 +806,8 @@ int drmCheckModesettingSupported(const char *busid)
>   return -EINVAL;
>   return (modesetting ? 0 : -ENOSYS);
>   }
> +#elif defined(__DragonFly__)
> + return 0;
>  #endif
>   return -ENOSYS;
>  
> -- 
> 2.0.0

Anyone willing to push this commit ?

-- 
Francois Tigeot

[PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2

[Christian König]

Am 06.08.2014 um 18:08 schrieb Jerome Glisse:
> On Wed, Aug 06, 2014 at 08:55:28AM +0200, Christian K?nig wrote:
>> Am 06.08.2014 um 00:13 schrieb Jerome Glisse:
>>> On Tue, Aug 05, 2014 at 07:45:21PM +0200, Christian K?nig wrote:
 Am 05.08.2014 um 19:39 schrieb Jerome Glisse:
> On Tue, Aug 05, 2014 at 06:05:29PM +0200, Christian K?nig wrote:
>> From: Christian K?nig 
>>
>> Avoid problems with writeback by limiting userptr to anonymous memory.
>>
>> v2: add commit and code comments
> I guess, i have not expressed myself clearly. This is bogus, you pretend
> you want to avoid writeback issue but you still allow userspace to map
> file backed pages (which by the way might be a regular bo object from
> another device for instance and that would be fun).
>
> So this patch is a no go and i would rather see that this userptr to
> be restricted to anon vma only no matter what. No flags here.
 Mapping of non anonymous memory (e.g. everything get_user_pages won't fail
 with) is restricted to read only access by the GPU.

 I'm fine with making it a hard requirement for all mappings if you say it's
 a must have.

>>> Well for time being you should force read only. The way you implement write
>>> is broken. Here is how it can abuse to allow write to a file backed mmap.
>>>
>>> mmap(fixaddress,fixedsize,NOFD)
>>> userptr_ioctl(fixedaddress, RADEON_GEM_USERPTR_ANONONLY)
>>> // bo is created successfully because fixedaddress is part of anonvma
>>> munmap(fixedaddress,fixedsize)
>>> // radeon get mmu_notifier_range_start callback and unbind page from the
>>> // bo but radeon does not know there was an unmap.
>>> mmap(fixaddress,fixedsize,fd_to_this_read_only_file_i_want_to_write_to)
>>> radeon_ioctl_use_my_userptrbo
>>> // bo is bind again by radeon and because all flag are set at creation
>>> // it is map with write permission allowing someone to write to a file
>>> // that might be read only for the user.
>>> //
>>> // Script kiddies it's time to learn about gpu ...
>>>
>>> Of course if you this patch (kind of selling my own junk here) :
>>>
>>> http://www.spinics.net/lists/linux-mm/msg75878.html
>>>
>>> then you could know inside the range_start that you should remove the
>>> write permission and that it should be rechecked on next bind.
>>>
>>> Note that i have not read much of your code so maybe you handle this
>>> case somehow.
>> I've stumbled over this attack vector as well and it's the reason why I've
>> moved checking the access rights to the bind callback instead of BO creation
>> time with V5 of the patch.
>>
>> This way you get an -EFAULT if you try something like this on command
>> submission time.
> So you seem immune to that issue but you are still not checking if the anon
> vma is writeable which you should again security concern here.

We check the access rights of the pointer using:
> if (!access_ok(write ? VERIFY_WRITE : VERIFY_READ, 
> (long)gtt->userptr,
>ttm->num_pages * PAGE_SIZE))
> return -EFAULT;

Shouldn't that be enough?

Christian.

>
> Cheers,
> J?r?me

[PULL] DRM Cleanups

[Dave Airlie]

On 6 August 2014 16:58, David Herrmann  wrote:
> Hi Dave
>
> A bunch of cleanups that are all reviewed by Daniel and Alex. Has survived the
> compile/runtime test bots for some weeks now, so should all be fine. Nothing
> critical, though.
>
> This series includes:
>  * hide ctxbitmap harder so newer drivers don't use it
>  * drop redundant drm_file->is_master
>  * move code out of drm_drv.c
>  * prepare sysfs/minor handling to be ready to drop drm_global_mutex
>
> Feel free to rename drm_stub.c to drm_drv.c (acked by Alex and Daniel) after
> you applied these patches.
>

I pulled this, but your git url below is kinda wrong :-)

If I was Linus, you'd be getting yelled at, to fix your scripts!

Dave.

> Thanks
> David
>
>
> The following changes since commit a91576d7916f6cce76d30303e60e1ac47cf4a76d:
>
>   drm/ttm: Pass GFP flags in order to avoid deadlock. (2014-08-05 10:54:19 
> +1000)
>
> are available in the git repository at:
>
>   ssh://dvdhrm at people.freedesktop.org/~dvdhrm/linux
>
> for you to fetch changes up to e7b96070dd9e51a8b16340411a8643d8c7d5a001:
>
>   drm: mark drm_context support as legacy (2014-08-05 19:38:12 +0200)
>
> 
> David Herrmann (8):
>   drm: extract legacy ctxbitmap flushing
>   drm: drop redundant drm_file->is_master
>   drm: don't de-authenticate clients on master-close
>   drm: move module initialization to drm_stub.c
>   drm: merge drm_drv.c into drm_ioctl.c
>   drm: make minor->index available early
>   drm: make sysfs device always available for minors
>   drm: mark drm_context support as legacy
>
>  drivers/gpu/drm/Makefile   |   2 +-
>  drivers/gpu/drm/drm_context.c  | 102 ---
>  drivers/gpu/drm/drm_crtc.c |   2 +-
>  drivers/gpu/drm/drm_drv.c  | 471 
> -
>  drivers/gpu/drm/drm_fops.c |  77 +
>  drivers/gpu/drm/drm_ioctl.c| 370 +-
>  drivers/gpu/drm/drm_legacy.h   |  51 
>  drivers/gpu/drm/drm_lock.c |   3 +-
>  drivers/gpu/drm/drm_stub.c | 238 +++
>  drivers/gpu/drm/drm_sysfs.c|  90 +++---
>  drivers/gpu/drm/i915/i915_gem_execbuffer.c |   4 +-
>  drivers/gpu/drm/vmwgfx/vmwgfx_drv.c|   2 +-
>  drivers/staging/imx-drm/imx-drm-core.c |   2 +-
>  include/drm/drmP.h |  61 ++--
>  14 files changed, 727 insertions(+), 748 deletions(-)
>  delete mode 100644 drivers/gpu/drm/drm_drv.c
>  create mode 100644 drivers/gpu/drm/drm_legacy.h

[PATCH 4/5] drm/radeon: add userptr flag to register MMU notifier v3

[Christian König]

Am 06.08.2014 um 17:16 schrieb Jerome Glisse:
> On Tue, Aug 05, 2014 at 06:05:31PM +0200, Christian K?nig wrote:
>> From: Christian K?nig 
>>
>> Whenever userspace mapping related to our userptr change
>> we wait for it to become idle and unmap it from GTT.
>>
>> v2: rebased, fix mutex unlock in error path
>> v3: improve commit message
> Why in hell do you not register the mmu_notifier on first userptr bo ?
The MMU notifier is registered on the first userptr bo that uses it.

Using the notifier is only optional for readonly BOs because it has 
quite an overhead and most uses of userptr are snapshot like uses 
anyway. E.g. buffer uploads that just needs the data from that user 
address at a specific point in time and are not interested in further 
updates are created without registering the notifier.

Christian.

>
>> Signed-off-by: Christian K?nig 
>> ---
>>   drivers/gpu/drm/Kconfig|   1 +
>>   drivers/gpu/drm/radeon/Makefile|   2 +-
>>   drivers/gpu/drm/radeon/radeon.h|  12 ++
>>   drivers/gpu/drm/radeon/radeon_device.c |   2 +
>>   drivers/gpu/drm/radeon/radeon_gem.c|   9 +-
>>   drivers/gpu/drm/radeon/radeon_mn.c | 272 
>> +
>>   drivers/gpu/drm/radeon/radeon_object.c |   1 +
>>   include/uapi/drm/radeon_drm.h  |   1 +
>>   8 files changed, 298 insertions(+), 2 deletions(-)
>>   create mode 100644 drivers/gpu/drm/radeon/radeon_mn.c
>>
>> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
>> index 9b2eedc..2745284 100644
>> --- a/drivers/gpu/drm/Kconfig
>> +++ b/drivers/gpu/drm/Kconfig
>> @@ -115,6 +115,7 @@ config DRM_RADEON
>>  select HWMON
>>  select BACKLIGHT_CLASS_DEVICE
>>  select INTERVAL_TREE
>> +select MMU_NOTIFIER
>>  help
>>Choose this option if you have an ATI Radeon graphics card.  There
>>are both PCI and AGP versions.  You don't need to choose this to
>> diff --git a/drivers/gpu/drm/radeon/Makefile 
>> b/drivers/gpu/drm/radeon/Makefile
>> index 0013ad0..c7fa1ae 100644
>> --- a/drivers/gpu/drm/radeon/Makefile
>> +++ b/drivers/gpu/drm/radeon/Makefile
>> @@ -80,7 +80,7 @@ radeon-y += radeon_device.o radeon_asic.o radeon_kms.o \
>>  r600_dpm.o rs780_dpm.o rv6xx_dpm.o rv770_dpm.o rv730_dpm.o rv740_dpm.o \
>>  rv770_smc.o cypress_dpm.o btc_dpm.o sumo_dpm.o sumo_smc.o trinity_dpm.o 
>> \
>>  trinity_smc.o ni_dpm.o si_smc.o si_dpm.o kv_smc.o kv_dpm.o ci_smc.o \
>> -ci_dpm.o dce6_afmt.o radeon_vm.o radeon_ucode.o radeon_ib.o
>> +ci_dpm.o dce6_afmt.o radeon_vm.o radeon_ucode.o radeon_ib.o radeon_mn.o
>>   
>>   # add async DMA block
>>   radeon-y += \
>> diff --git a/drivers/gpu/drm/radeon/radeon.h 
>> b/drivers/gpu/drm/radeon/radeon.h
>> index 3c6999e..511191f 100644
>> --- a/drivers/gpu/drm/radeon/radeon.h
>> +++ b/drivers/gpu/drm/radeon/radeon.h
>> @@ -65,6 +65,7 @@
>>   #include 
>>   #include 
>>   #include 
>> +#include 
>>   
>>   #include 
>>   #include 
>> @@ -487,6 +488,9 @@ struct radeon_bo {
>>   
>>  struct ttm_bo_kmap_obj  dma_buf_vmap;
>>  pid_t   pid;
>> +
>> +struct radeon_mn*mn;
>> +struct interval_tree_node   mn_it;
>>   };
>>   #define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, 
>> gem_base)
>>   
>> @@ -1725,6 +1729,11 @@ void radeon_test_ring_sync(struct radeon_device *rdev,
>> struct radeon_ring *cpB);
>>   void radeon_test_syncing(struct radeon_device *rdev);
>>   
>> +/*
>> + * MMU Notifier
>> + */
>> +int radeon_mn_register(struct radeon_bo *bo, unsigned long addr);
>> +void radeon_mn_unregister(struct radeon_bo *bo);
>>   
>>   /*
>>* Debugfs
>> @@ -2372,6 +2381,9 @@ struct radeon_device {
>>  /* tracking pinned memory */
>>  u64 vram_pin_size;
>>  u64 gart_pin_size;
>> +
>> +struct mutexmn_lock;
>> +DECLARE_HASHTABLE(mn_hash, 7);
>>   };
>>   
>>   bool radeon_is_px(struct drm_device *dev);
>> diff --git a/drivers/gpu/drm/radeon/radeon_device.c 
>> b/drivers/gpu/drm/radeon/radeon_device.c
>> index c8ea050..c58f84f 100644
>> --- a/drivers/gpu/drm/radeon/radeon_device.c
>> +++ b/drivers/gpu/drm/radeon/radeon_device.c
>> @@ -1270,6 +1270,8 @@ int radeon_device_init(struct radeon_device *rdev,
>>  init_rwsem(>pm.mclk_lock);
>>  init_rwsem(>exclusive_lock);
>>  init_waitqueue_head(>irq.vblank_queue);
>> +mutex_init(>mn_lock);
>> +hash_init(rdev->mn_hash);
>>  r = radeon_gem_init(rdev);
>>  if (r)
>>  return r;
>> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
>> b/drivers/gpu/drm/radeon/radeon_gem.c
>> index 4506560..2a6fbf1 100644
>> --- a/drivers/gpu/drm/radeon/radeon_gem.c
>> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
>> @@ -291,7 +291,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, 
>> void *data,
>>   
>>  /* reject unknown flag values */
>>  if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
>> -

[PATCH] drm: Renaming DP training vswing/pre-emph defines

[sonika.jin...@intel.com]

From: Sonika Jindal 

Rename the defines to have levels instead of values for vswing and pre-emph
levels as the values may differ in other scenarios like low vswing of eDP 1.4
where the values are different.

v2: Keeping old and new defines (Danvet), adding description in the commit
message

Cc: dri-devel at lists.freedesktop.org
Signed-off-by: Sonika Jindal 
---
 include/drm/drm_dp_helper.h |   26 ++
 1 file changed, 18 insertions(+), 8 deletions(-)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index a21568b..df1e262 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -190,16 +190,26 @@
 # define DP_TRAIN_VOLTAGE_SWING_MASK   0x3
 # define DP_TRAIN_VOLTAGE_SWING_SHIFT  0
 # define DP_TRAIN_MAX_SWING_REACHED(1 << 2)
-# define DP_TRAIN_VOLTAGE_SWING_400(0 << 0)
-# define DP_TRAIN_VOLTAGE_SWING_600(1 << 0)
-# define DP_TRAIN_VOLTAGE_SWING_800(2 << 0)
-# define DP_TRAIN_VOLTAGE_SWING_1200   (3 << 0)
+# define DP_TRAIN_VOLTAGE_SWING_400(0 << 0)
+# define DP_TRAIN_VOLTAGE_SWING_600(1 << 0)
+# define DP_TRAIN_VOLTAGE_SWING_800(2 << 0)
+# define DP_TRAIN_VOLTAGE_SWING_1200   (3 << 0)
+
+# define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0)
+# define DP_TRAIN_VOLTAGE_SWING_LEVEL_1 (1 << 0)
+# define DP_TRAIN_VOLTAGE_SWING_LEVEL_2 (2 << 0)
+# define DP_TRAIN_VOLTAGE_SWING_LEVEL_3 (3 << 0)

 # define DP_TRAIN_PRE_EMPHASIS_MASK(3 << 3)
-# define DP_TRAIN_PRE_EMPHASIS_0   (0 << 3)
-# define DP_TRAIN_PRE_EMPHASIS_3_5 (1 << 3)
-# define DP_TRAIN_PRE_EMPHASIS_6   (2 << 3)
-# define DP_TRAIN_PRE_EMPHASIS_9_5 (3 << 3)
+# define DP_TRAIN_PRE_EMPHASIS_0   (0 << 3)
+# define DP_TRAIN_PRE_EMPHASIS_3_5 (1 << 3)
+# define DP_TRAIN_PRE_EMPHASIS_6   (2 << 3)
+# define DP_TRAIN_PRE_EMPHASIS_9_5 (3 << 3)
+
+# define DP_TRAIN_PRE_EMPHASIS_LEVEL_0  (0 << 3)
+# define DP_TRAIN_PRE_EMPHASIS_LEVEL_1  (1 << 3)
+# define DP_TRAIN_PRE_EMPHASIS_LEVEL_2  (2 << 3)
+# define DP_TRAIN_PRE_EMPHASIS_LEVEL_3  (3 << 3)

 # define DP_TRAIN_PRE_EMPHASIS_SHIFT   3
 # define DP_TRAIN_MAX_PRE_EMPHASIS_REACHED  (1 << 5)
-- 
1.7.10.4

[PATCH 9/9] ARM: OMAP2+: omap2plus_defconfig: Enable BeagleBone Black HDMI audio support

[Jyri Sarha]

Select CONFIG_SND_EDMA_SOC=m and CONFIG_SND_AM335X_SOC_NXPTDA_EVM=m
for BBB HDMI audio.

Signed-off-by: Jyri Sarha 
---
 arch/arm/configs/omap2plus_defconfig |2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/configs/omap2plus_defconfig 
b/arch/arm/configs/omap2plus_defconfig
index 8b07fda..73dbfad 100644
--- a/arch/arm/configs/omap2plus_defconfig
+++ b/arch/arm/configs/omap2plus_defconfig
@@ -222,7 +222,9 @@ CONFIG_SND_DEBUG=y
 CONFIG_SND_USB_AUDIO=m
 CONFIG_SND_SOC=m
 CONFIG_SND_OMAP_SOC=m
+CONFIG_SND_EDMA_SOC=m
 CONFIG_SND_AM33XX_SOC_EVM=m
+CONFIG_SND_AM335X_SOC_NXPTDA_EVM=m
 CONFIG_SND_DAVINCI_SOC=m
 CONFIG_SND_OMAP_SOC_OMAP_TWL4030=m
 CONFIG_SND_OMAP_SOC_OMAP_ABE_TWL6040=m
-- 
1.7.9.5

[PATCH 8/9] ARM: OMAP2+: omap2plus_defconfig: TDA998X HDMI trough tilcdc, slave

[Jyri Sarha]

Select CONFIG_DRM=m, CONFIG_DRM_I2C_NXP_TDA998X=m, and
CONFIG_DRM_TILCDC=m, for Beaglebone-Black HDMI video support.

Signed-off-by: Jyri Sarha 
---
 arch/arm/configs/omap2plus_defconfig |3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/configs/omap2plus_defconfig 
b/arch/arm/configs/omap2plus_defconfig
index 536a137..8b07fda 100644
--- a/arch/arm/configs/omap2plus_defconfig
+++ b/arch/arm/configs/omap2plus_defconfig
@@ -191,6 +191,9 @@ CONFIG_REGULATOR_TPS65217=y
 CONFIG_REGULATOR_TPS65910=y
 CONFIG_REGULATOR_TWL4030=y
 CONFIG_REGULATOR_PBIAS=y
+CONFIG_DRM=m
+CONFIG_DRM_I2C_NXP_TDA998X=m
+CONFIG_DRM_TILCDC=m
 CONFIG_FB=y
 CONFIG_FIRMWARE_EDID=y
 CONFIG_FB_MODE_HELPERS=y
-- 
1.7.9.5

[PATCH 7/9] ARM: dts: am335x-boneblack: Add HDMI audio support

[Jyri Sarha]

Adds mcasp0_pins, clk_mcasp0_fixed, clk_mcasp0, mcasp0, hdmi_audio,
and sound nodes.

Signed-off-by: Jyri Sarha 
---
 arch/arm/boot/dts/am335x-boneblack.dts |   54 
 1 file changed, 54 insertions(+)

diff --git a/arch/arm/boot/dts/am335x-boneblack.dts 
b/arch/arm/boot/dts/am335x-boneblack.dts
index 305975d..c3c4618 100644
--- a/arch/arm/boot/dts/am335x-boneblack.dts
+++ b/arch/arm/boot/dts/am335x-boneblack.dts
@@ -59,12 +59,50 @@
0x1b0 0x03  /* xdma_event_intr0, OMAP_MUX_MODE3 | 
AM33XX_PIN_OUTPUT */
>;
};
+
+   mcasp0_pins: mcasp0_pins {
+   pinctrl-single,pins = <
+   0x1ac (PIN_INPUT_PULLUP | MUX_MODE0)/* 
mcasp0_ahclkx.mcasp0_ahclkx */
+   0x19c (PIN_OUTPUT_PULLDOWN | MUX_MODE2) /* 
mcasp0_ahclkr.mcasp0_axr2 */
+   0x194 (PIN_OUTPUT_PULLUP | MUX_MODE0)   /* 
mcasp0_fsx.mcasp0_fsx */
+   0x190 (PIN_OUTPUT_PULLDOWN | MUX_MODE0) /* 
mcasp0_aclkx.mcasp0_aclkx */
+   0x06c (PIN_OUTPUT_PULLDOWN | MUX_MODE7) /* 
gpmc_a11.GPIO1_27 */
+   >;
+   };
 };

  {
status = "okay";
 };

+{
+   pinctrl-names = "default";
+   pinctrl-0 = <_pins>;
+   status = "okay";
+   op-mode = <0>;  /* MCASP_IIS_MODE */
+   tdm-slots = <2>;
+   serial-dir = <  /* 0: INACTIVE, 1: TX, 2: RX */
+   0 0 1 0
+   >;
+   tx-num-evt = <1>;
+   rx-num-evt = <1>;
+};
+
+_clocks {
+   clk_mcasp0_fixed: clk_mcasp0_fixed {
+ #clock-cells = <0>;
+ compatible = "fixed-clock";
+ clock-frequency = <24576000>;
+   };
+
+   clk_mcasp0: clk_mcasp0 {
+ #clock-cells = <0>;
+ compatible = "gpio-clock";
+ clocks = <_mcasp0_fixed>;
+ enable-gpios = < 27 0>; /* BeagleBone Black Clk enable on 
GPIO1_27 */
+   };
+};
+
 / {
hdmi {
compatible = "ti,tilcdc,slave";
@@ -74,4 +112,20 @@
pinctrl-1 = <_hdmi_bonelt_off_pins>;
status = "okay";
};
+
+   hdmi_audio: hdmi_audio at 0 {
+  compatible = "linux,hdmi-audio";
+  status = "okay";
+   };
+
+   sound {
+   compatible = "ti,am335x-beaglebone-black-audio";
+   ti,model = "TI BeagleBone Black";
+   ti,audio-codec = <_audio>;
+   ti,mcasp-controller = <>;
+   ti,audio-routing =
+   "HDMI Out", "TX";
+   clocks = <_mcasp0>;
+   clock-names = "mclk";
+   };
 };
-- 
1.7.9.5

[PATCH 6/9] ARM: dts: am33xx: Add external clock provider

[Jyri Sarha]

Add external clock provaider for am33xx devices.

Signed-off-by: Jyri Sarha 
---
 arch/arm/boot/dts/am33xx.dtsi |   10 ++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/boot/dts/am33xx.dtsi b/arch/arm/boot/dts/am33xx.dtsi
index 4a4e02d..5093c64 100644
--- a/arch/arm/boot/dts/am33xx.dtsi
+++ b/arch/arm/boot/dts/am33xx.dtsi
@@ -92,6 +92,16 @@
pinctrl-single,function-mask = <0x7f>;
};

+   ext_clocks {
+   compatible = "ti,external-clocks";
+
+   ext_clocks: clocks {
+   };
+
+   ext_clockdomains: clockdomains {
+   };
+   };
+
/*
 * XXX: Use a flat representation of the AM33XX interconnect.
 * The real AM33XX interconnect network is quite complex. Since
-- 
1.7.9.5

[PATCH 5/9] ASoC: davinci: HDMI audio build for AM33XX and TDA998x

[Jyri Sarha]

Adds configuration option for HDMI audio support for AM33XX based
boards with NXP TDA998x HDMI transmitter. The audio is connected to
NXP TDA998x trough McASP running in i2s mode.

Signed-off-by: Jyri Sarha 
---
 sound/soc/davinci/Kconfig |   12 
 1 file changed, 12 insertions(+)

diff --git a/sound/soc/davinci/Kconfig b/sound/soc/davinci/Kconfig
index b310dd3..59c61a0 100644
--- a/sound/soc/davinci/Kconfig
+++ b/sound/soc/davinci/Kconfig
@@ -37,6 +37,18 @@ config SND_AM33XX_SOC_EVM
  AM335X-EVMSK, and BeagelBone with AudioCape boards have this
  setup.

+config SND_AM335X_SOC_NXPTDA_EVM
+   tristate "HDMI Audio for the AM33XX chip based boards with TDA998x"
+   depends on SND_EDMA_SOC && SOC_AM33XX && DRM_I2C_NXP_TDA998X
+   select SND_SOC_HDMI_CODEC
+   select SND_DAVINCI_SOC_MCASP
+   select SND_DAVINCI_SOC_GENERIC_EVM
+   help
+ Say Y or M if you want to add support for HDMI SoC audio on
+ AM33XX boards with NXP TDA998x HDMI transmitter. For example
+ BeagleBoneBack. The audio is connected to NXP TDA998x trough
+ McASP running in i2s mode.
+
 config SND_DAVINCI_SOC_EVM
tristate "SoC Audio support for DaVinci DM6446, DM355 or DM365 EVM"
depends on SND_DAVINCI_SOC && I2C
-- 
1.7.9.5

[PATCH 4/9] ASoC: davinci-evm: HDMI audio support for TDA998x trough McASP I2S bus

[Jyri Sarha]

Add machine driver support for BeagleBone-Black HDMI audio. BBB has
NXP TDA998X HDMI transmitter connected to McASP port in I2S mode. The
44100 Hz sample-rate and it's multiples can not be accurately produced
on BBB. The only supported sample format is SNDRV_PCM_FORMAT_S32_LE.
The 8 least significant bits are ignored.

Signed-off-by: Jyri Sarha 
---
 .../bindings/sound/davinci-evm-audio.txt   |4 +-
 sound/soc/davinci/davinci-evm.c|   82 +++-
 2 files changed, 83 insertions(+), 3 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/davinci-evm-audio.txt 
b/Documentation/devicetree/bindings/sound/davinci-evm-audio.txt
index 963e100..c137436 100644
--- a/Documentation/devicetree/bindings/sound/davinci-evm-audio.txt
+++ b/Documentation/devicetree/bindings/sound/davinci-evm-audio.txt
@@ -1,7 +1,9 @@
 * Texas Instruments SoC audio setups with TLV320AIC3X Codec

 Required properties:
-- compatible : "ti,da830-evm-audio" : forDM365/DA8xx/OMAPL1x/AM33xx
+- compatible : "ti,da830-evm-audio" : for DM365/DA8xx/OMAPL1x/AM33xx
+   "ti,am335x-beaglebone-black-audio" : for Beaglebone-black HDMI
+audio
 - ti,model : The user-visible name of this sound complex.
 - ti,audio-codec : The phandle of the TLV320AIC3x audio codec
 - ti,mcasp-controller : The phandle of the McASP controller
diff --git a/sound/soc/davinci/davinci-evm.c b/sound/soc/davinci/davinci-evm.c
index a50010e..9b98667 100644
--- a/sound/soc/davinci/davinci-evm.c
+++ b/sound/soc/davinci/davinci-evm.c
@@ -21,6 +21,7 @@
 #include 
 #include 
 #include 
+#include 

 #include 
 #include 
@@ -83,12 +84,46 @@ static int evm_hw_params(struct snd_pcm_substream 
*substream,
return 0;
 }

+/* If changing sample format the tda998x configuration (REG_CTS_N) needs
+   to be changed. */
+#define TDA998X_SAMPLE_FORMAT SNDRV_PCM_FORMAT_S32_LE
+static int evm_tda998x_startup(struct snd_pcm_substream *substream)
+{
+   struct snd_pcm_runtime *runtime = substream->runtime;
+   struct snd_mask *fmt = constrs_mask(>hw_constraints,
+   SNDRV_PCM_HW_PARAM_FORMAT);
+   snd_mask_none(fmt);
+   snd_mask_set(fmt, TDA998X_SAMPLE_FORMAT);
+
+   return evm_startup(substream);
+}
+
+static int evm_tda998x_hw_params(struct snd_pcm_substream *substream,
+struct snd_pcm_hw_params *params)
+{
+   struct snd_soc_pcm_runtime *rtd = substream->private_data;
+   struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+   struct snd_soc_card *soc_card = rtd->card;
+   struct snd_soc_card_drvdata_davinci *drvdata =
+   snd_soc_card_get_drvdata(soc_card);
+
+   return snd_soc_dai_set_sysclk(cpu_dai, 0, drvdata->sysclk,
+ SND_SOC_CLOCK_IN);
+}
+
 static struct snd_soc_ops evm_ops = {
.startup = evm_startup,
.shutdown = evm_shutdown,
.hw_params = evm_hw_params,
 };

+
+static struct snd_soc_ops evm_tda998x_ops = {
+   .startup = evm_tda998x_startup,
+   .shutdown = evm_shutdown,
+   .hw_params = evm_tda998x_hw_params,
+};
+
 /* davinci-evm machine dapm widgets */
 static const struct snd_soc_dapm_widget aic3x_dapm_widgets[] = {
SND_SOC_DAPM_HP("Headphone Jack", NULL),
@@ -149,6 +184,35 @@ static int evm_aic3x_init(struct snd_soc_pcm_runtime *rtd)
return 0;
 }

+static const struct snd_soc_dapm_widget tda998x_dapm_widgets[] = {
+   SND_SOC_DAPM_OUTPUT("HDMI Out"),
+};
+
+static int evm_tda998x_init(struct snd_soc_pcm_runtime *rtd)
+{
+   struct snd_soc_dai *cpu_dai = rtd->cpu_dai;
+   struct snd_soc_dapm_context *dapm = >codec->dapm;
+   struct snd_soc_card *soc_card = rtd->card;
+   int ret;
+
+   ret = snd_soc_dai_set_clkdiv(cpu_dai, 0, 1);
+   if (ret < 0)
+   return ret;
+
+   snd_soc_dapm_new_controls(dapm, tda998x_dapm_widgets,
+ ARRAY_SIZE(tda998x_dapm_widgets));
+
+   ret = snd_soc_of_parse_audio_routing(soc_card, "ti,audio-routing");
+
+   /* not connected */
+   snd_soc_dapm_disable_pin(dapm, "RX");
+
+   /* always connected */
+   snd_soc_dapm_enable_pin(dapm, "HDMI Out");
+
+   return 0;
+}
+
 /* davinci-evm digital audio interface glue - connects codec <--> CPU */
 static struct snd_soc_dai_link dm6446_evm_dai = {
.name = "TLV320AIC3X",
@@ -334,7 +398,7 @@ static struct snd_soc_card da850_snd_soc_card = {
 #if defined(CONFIG_OF)

 /*
- * The struct is used as place holder. It will be completely
+ * The structs are used as place holders. They will be completely
  * filled with data from dt node.
  */
 static struct snd_soc_dai_link evm_dai_tlv320aic3x = {
@@ -347,10 +411,24 @@ static struct snd_soc_dai_link evm_dai_tlv320aic3x = {
   SND_SOC_DAIFMT_IB_NF,
 };

+static struct snd_soc_dai_link evm_dai_tda998x_hdmi = {
+   .name   = 

[PATCH 3/9] drm/tilcdc: Add I2S HDMI audio config for tda998x

[Jyri Sarha]

The configuration is needed for HDMI audio. The "swap" and "mirr"
parameters have to be correctly set in the configuration in order to
have proper colors in the HDMI picture.

Signed-off-by: Jyri Sarha 
---
 drivers/gpu/drm/tilcdc/tilcdc_slave.c |   24 +++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/tilcdc/tilcdc_slave.c 
b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
index 595068b..e43240a 100644
--- a/drivers/gpu/drm/tilcdc/tilcdc_slave.c
+++ b/drivers/gpu/drm/tilcdc/tilcdc_slave.c
@@ -19,6 +19,7 @@
 #include 
 #include 
 #include 
+#include 

 #include "tilcdc_drv.h"

@@ -111,8 +112,29 @@ static const struct drm_encoder_helper_funcs 
slave_encoder_helper_funcs = {
.restore= drm_i2c_encoder_restore,
 };

+static struct tda998x_encoder_params tda998x_pdata = {
+   .swap_b = 0x3,
+   .mirr_b = 0x0,
+   .swap_a = 0x2,
+   .mirr_a = 0x0,
+   .swap_d = 0x1,
+   .mirr_d = 0x0,
+   .swap_c = 0x0,
+   .mirr_c = 0x0,
+   .swap_f = 0x5,
+   .mirr_f = 0x0,
+   .swap_e = 0x4,
+   .mirr_e = 0x0,
+   .audio_cfg = 0x3,   /* I2S mode */
+   .audio_clk_cfg = 1, /* select clock pin */
+   .audio_frame[1] = 1,/* channels - 1 */
+   .audio_format = AFMT_I2S,
+   .audio_sample_rate = 48000,
+};
+
 static const struct i2c_board_info info = {
-   I2C_BOARD_INFO("tda998x", 0x70)
+   I2C_BOARD_INFO("tda998x", 0x70),
+   .platform_data = _pdata,
 };

 static struct drm_encoder *slave_encoder_create(struct drm_device *dev,
-- 
1.7.9.5

[PATCH 2/9] clk: add gpio controlled clock

[Jyri Sarha]

The added clk-gpio is a basic clock that can be enabled and disabled
trough a gpio output. The DT binding document for the clock is also
added. For EPROBE_DEFER handling the registering of the clock has to
be delayed until of_clk_get() call time.

Signed-off-by: Jyri Sarha 
---
 .../devicetree/bindings/clock/gpio-clock.txt   |   21 ++
 drivers/clk/Makefile   |1 +
 drivers/clk/clk-gpio.c |  206 
 include/linux/clk-provider.h   |   23 +++
 4 files changed, 251 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/clock/gpio-clock.txt
 create mode 100644 drivers/clk/clk-gpio.c

diff --git a/Documentation/devicetree/bindings/clock/gpio-clock.txt 
b/Documentation/devicetree/bindings/clock/gpio-clock.txt
new file mode 100644
index 000..54fea39
--- /dev/null
+++ b/Documentation/devicetree/bindings/clock/gpio-clock.txt
@@ -0,0 +1,21 @@
+Binding for simple gpio controlled clock.
+
+This binding uses the common clock binding[1].
+
+[1] Documentation/devicetree/bindings/clock/clock-bindings.txt
+
+Required properties:
+- compatible : shall be "gpio-clock".
+- #clock-cells : from common clock binding; shall be set to 0.
+- enable-gpios : GPIO reference for enabling and disabling the clock.
+
+Optional properties:
+- clocks: Maximum of one parent clock is supported.
+
+Example:
+   clock {
+   compatible = "gpio-clock";
+   clocks = <>;
+   #clock-cells = <0>;
+   enable-gpios = < 1 GPIO_ACTIVE_HIGH>;
+   };
diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
index 567f102..bde8fdf 100644
--- a/drivers/clk/Makefile
+++ b/drivers/clk/Makefile
@@ -9,6 +9,7 @@ obj-$(CONFIG_COMMON_CLK)+= clk-gate.o
 obj-$(CONFIG_COMMON_CLK)   += clk-mux.o
 obj-$(CONFIG_COMMON_CLK)   += clk-composite.o
 obj-$(CONFIG_COMMON_CLK)   += clk-fractional-divider.o
+obj-$(CONFIG_COMMON_CLK)   += clk-gpio.o

 # hardware specific clock types
 # please keep this section sorted lexicographically by file/directory path name
diff --git a/drivers/clk/clk-gpio.c b/drivers/clk/clk-gpio.c
new file mode 100644
index 000..1eff97c
--- /dev/null
+++ b/drivers/clk/clk-gpio.c
@@ -0,0 +1,206 @@
+/*
+ * Copyright (C) 2012 - 2014 Texas Instruments Incorporated - http://www.ti.com
+ * Author: Jyri Sarha 
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * Gpio controlled clock implementation
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+/**
+ * DOC: basic gpio controlled clock which can be enabled and disabled
+ *  with gpio output
+ * Traits of this clock:
+ * prepare - clk_(un)prepare only ensures parent is (un)prepared
+ * enable - clk_enable and clk_disable are functional & control gpio
+ * rate - inherits rate from parent.  No clk_set_rate support
+ * parent - fixed parent.  No clk_set_parent support
+ */
+
+#define to_clk_gpio(_hw) container_of(_hw, struct clk_gpio, hw)
+
+static int clk_gpio_enable(struct clk_hw *hw)
+{
+   struct clk_gpio *clk = to_clk_gpio(hw);
+
+   gpiod_set_value(clk->gpiod, 1);
+
+   return 0;
+}
+
+static void clk_gpio_disable(struct clk_hw *hw)
+{
+   struct clk_gpio *clk = to_clk_gpio(hw);
+
+   gpiod_set_value(clk->gpiod, 0);
+}
+
+static int clk_gpio_is_enabled(struct clk_hw *hw)
+{
+   struct clk_gpio *clk = to_clk_gpio(hw);
+
+   return gpiod_get_value(clk->gpiod);
+}
+
+const struct clk_ops clk_gpio_ops = {
+   .enable = clk_gpio_enable,
+   .disable = clk_gpio_disable,
+   .is_enabled = clk_gpio_is_enabled,
+};
+EXPORT_SYMBOL_GPL(clk_gpio_ops);
+
+/**
+ * clk_register_gpio - register a gpip clock with the clock framework
+ * @dev: device that is registering this clock
+ * @name: name of this clock
+ * @parent_name: name of this clock's parent
+ * @gpiod: gpio descriptor to control this clock
+ */
+struct clk *clk_register_gpio(struct device *dev, const char *name,
+   const char *parent_name, struct gpio_desc *gpiod,
+   unsigned long flags)
+{
+   struct clk_gpio *clk_gpio = NULL;
+   struct clk *clk = ERR_PTR(-EINVAL);
+   struct clk_init_data init = { NULL };
+   unsigned long gpio_flags;
+   int err;
+
+   if (gpiod_is_active_low(gpiod))
+   gpio_flags = GPIOF_OUT_INIT_HIGH;
+   else
+   gpio_flags = GPIOF_OUT_INIT_LOW;
+
+   if (dev)
+   err = devm_gpio_request_one(dev, desc_to_gpio(gpiod),
+   gpio_flags, name);
+   else
+   err = gpio_request_one(desc_to_gpio(gpiod), gpio_flags, name);
+
+   if (err) {
+   pr_err("%s: %s: Error requesting clock control gpio %u\n",
+  __func__, name, desc_to_gpio(gpiod));

[PATCH 1/9] ASoC: mcasp: Fix implicit BLCK divider setting

[Jyri Sarha]

The implicit BLCK divider setting was broken by "ASoC: mcasp: don't
override bclk divider if it was provided by the machine"-patch. After
the BCLK divider is implicitly set for the first time the
mcasp->bclk_div gets a non zero value and the implicit setting is
"turned off".

Signed-off-by: Jyri Sarha 
---
 sound/soc/davinci/davinci-mcasp.c |   14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/sound/soc/davinci/davinci-mcasp.c 
b/sound/soc/davinci/davinci-mcasp.c
index c28508d..6a6b2ff 100644
--- a/sound/soc/davinci/davinci-mcasp.c
+++ b/sound/soc/davinci/davinci-mcasp.c
@@ -403,7 +403,8 @@ out:
return ret;
 }

-static int davinci_mcasp_set_clkdiv(struct snd_soc_dai *dai, int div_id, int 
div)
+static int __davinci_mcasp_set_clkdiv(struct snd_soc_dai *dai, int div_id,
+ int div, bool explicit)
 {
struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(dai);

@@ -420,7 +421,8 @@ static int davinci_mcasp_set_clkdiv(struct snd_soc_dai 
*dai, int div_id, int div
   ACLKXDIV(div - 1), ACLKXDIV_MASK);
mcasp_mod_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG,
   ACLKRDIV(div - 1), ACLKRDIV_MASK);
-   mcasp->bclk_div = div;
+   if (explicit)
+   mcasp->bclk_div = div;
break;

case 2: /* BCLK/LRCLK ratio */
@@ -434,6 +436,12 @@ static int davinci_mcasp_set_clkdiv(struct snd_soc_dai 
*dai, int div_id, int div
return 0;
 }

+static int davinci_mcasp_set_clkdiv(struct snd_soc_dai *dai, int div_id,
+   int div)
+{
+   return __davinci_mcasp_set_clkdiv(dai, div_id, div, 1);
+}
+
 static int davinci_mcasp_set_sysclk(struct snd_soc_dai *dai, int clk_id,
unsigned int freq, int dir)
 {
@@ -738,7 +746,7 @@ static int davinci_mcasp_hw_params(struct snd_pcm_substream 
*substream,
 "Inaccurate BCLK: %u Hz / %u != %u Hz\n",
 mcasp->sysclk_freq, div, bclk_freq);
}
-   davinci_mcasp_set_clkdiv(cpu_dai, 1, div);
+   __davinci_mcasp_set_clkdiv(cpu_dai, 1, div, 0);
}

ret = mcasp_common_hw_param(mcasp, substream->stream,
-- 
1.7.9.5

[PATCH 0/9] Beaglebone-Black HDMI audio

[Jyri Sarha]

The code has a functional dependency to:
http://www.mail-archive.com/linux-omap at vger.kernel.org/msg108199.html

Without the above patch the audio card does not probe.

The code has been rebased on top of Linux 3.16 merged with 
git://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
with http://www.mail-archive.com/linux-omap at vger.kernel.org/msg108199.html
cherry-picked on top.

All patches have gone trough some cleaning up since the RFC version of
these patches over half a year ago.

Jyri Sarha (9):
  ASoC: mcasp: Fix implicit BLCK divider setting
  clk: add gpio controlled clock
  drm/tilcdc: Add I2S HDMI audio config for tda998x
  ASoC: davinci-evm: HDMI audio support for TDA998x trough McASP I2S
bus
  ASoC: davinci: HDMI audio build for AM33XX and TDA998x
  ARM: dts: am33xx: Add external clock provider
  ARM: dts: am335x-boneblack: Add HDMI audio support
  ARM: OMAP2+: omap2plus_defconfig: TDA998X HDMI trough tilcdc,slave
  ARM: OMAP2+: omap2plus_defconfig: Enable BeagleBone Black HDMI audio
support

 .../devicetree/bindings/clock/gpio-clock.txt   |   21 ++
 .../bindings/sound/davinci-evm-audio.txt   |4 +-
 arch/arm/boot/dts/am335x-boneblack.dts |   54 +
 arch/arm/boot/dts/am33xx.dtsi  |   10 +
 arch/arm/configs/omap2plus_defconfig   |5 +
 drivers/clk/Makefile   |1 +
 drivers/clk/clk-gpio.c |  206 
 drivers/gpu/drm/tilcdc/tilcdc_slave.c  |   24 ++-
 include/linux/clk-provider.h   |   23 +++
 sound/soc/davinci/Kconfig  |   12 ++
 sound/soc/davinci/davinci-evm.c|   82 +++-
 sound/soc/davinci/davinci-mcasp.c  |   14 +-
 12 files changed, 449 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/clock/gpio-clock.txt
 create mode 100644 drivers/clk/clk-gpio.c

-- 
1.7.9.5

[Intel-gfx] [PATCH 17/19] drm/i915: Clear .last vblank count before drm_vblank_off() when sanitizing crtc state

[Ville Syrjälä]

On Wed, Aug 06, 2014 at 03:30:17PM +0200, Daniel Vetter wrote:
> On Wed, Aug 06, 2014 at 02:50:00PM +0300, ville.syrjala at linux.intel.com 
> wrote:
> > From: Ville Syrj?l? 
> > 
> > We call drm_vblank_off() during crtc sanitation to make sure the
> > software and hardware states agree. However drm_vblank_off() will
> > try to update the vblank timestamp and sequence number which lands
> > us in some trouble.
> > 
> > As the pipe is disabled the hardware frame counter query will
> > return 0. If the .last doesn't match the code will try to add the
> > difference to the user visible sequence number. During driver init
> > that's OK as .last == 0, but during resume .last may be anything.
> > So we should make sure we don't try to apply the diff here by zeroing
> > .last beforehand. Otherwise there maybe be a random jump in the user
> > visible sequence number across suspend.
> > 
> > Signed-off-by: Ville Syrj?l? 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c 
> > b/drivers/gpu/drm/i915/intel_display.c
> > index ae5f20d..c00bcd0 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -12918,8 +12918,11 @@ static void intel_sanitize_crtc(struct intel_crtc 
> > *crtc)
> > /* restore vblank interrupts to correct state */
> > if (crtc->active)
> > drm_vblank_on(dev, crtc->pipe);
> > -   else
> > +   else {
> > +   /* avoid random jumps in seq/ts */
> > +   dev->vblank[crtc->pipe].last = 0;
> 
> Should we have a drm_vblank_reset for this? I guess other drivers will
> have similar issues with "sorry, just lost it all" kind of transitions.
> 
> Also this case should only happen when we enter the system resume with the
> pipes in dpms off state. In that case we should have sampled something in
> drm_vblank_off already and resampling doesn't look like a good idea.
> 
> Do we instead need some protection in drm_vblank_off to avoid re-sampling?

Yeah, I suppose I could try to fix it in drm vblank code somehow. Just
resetting .last=0 in drm_vblank_off() would avoid the seq jump but would
still leave the vblank counter query in place. Admittedly the resulting
debug spam about vblank counter queries on disabled crtcs is rather
annoying.

> -Daniel
> 
> > drm_vblank_off(dev, crtc->pipe);
> > +   }
> >  
> > /* We need to sanitize the plane -> pipe mapping first because this will
> >  * disable the crtc (and hence change the state) if it is wrong. Note
> > -- 
> > 1.8.5.5
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx at 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

-- 
Ville Syrj?l?
Intel OTC

[PATCH] drm/panel/simple: add optronics B101XTN01.0

[Rob Clark]

On Wed, Aug 6, 2014 at 4:31 PM, Rob Clark  wrote:
> LVDS panel, make/model described as:
>
> AU Optronics Corporation - B101XTN01.0 (H/W:0A)
>
> See:
> http://www.encore-electronic.com/media/B101XTN01.0.pdf
>
> Signed-off-by: Rob Clark 
> ---
>  drivers/gpu/drm/panel/panel-simple.c | 27 +++
>  1 file changed, 27 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c 
> b/drivers/gpu/drm/panel/panel-simple.c
> index a251361..16119ba 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -447,6 +447,29 @@ static const struct panel_desc samsung_ltn101nt05 = {
> },
>  };
>
> +static const struct drm_display_mode optronics_lvds_mode = {
> +   .clock = 72000,
> +   .hdisplay = 1366,
> +   .hsync_start = 1366 + 20,
> +   .hsync_end = 1366 + 20 + 70,
> +   .htotal = 1366 + 20 + 70,
> +   .vdisplay = 768,
> +   .vsync_start = 768 + 14,
> +   .vsync_end = 768 + 14 + 42,
> +   .vtotal = 768 + 14 + 42,
> +   .vrefresh = 60,
> +   .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> +};
> +
> +static const struct panel_desc optronics_lvds = {
> +   .modes = _lvds_mode,
> +   .num_modes = 1,
> +   .size = {
> +   .width = 1366,
> +   .height = 768,
> +   },
> +};
> +
>  static const struct of_device_id platform_of_match[] = {
> {
> .compatible = "auo,b101aw03",
> @@ -476,6 +499,10 @@ static const struct of_device_id platform_of_match[] = {
> .compatible = "samsung,ltn101nt05",
> .data = _ltn101nt05,
> }, {
> +   .compatible = "optronics,b101xtn01",
> +   .data = _lvds,
> +   }, {
> +   }, {

erm, ignore the duplicate '}, {'.. opps..

BR,
-R

> .compatible = "simple-panel",
> }, {
> /* sentinel */
> --
> 1.9.3
>

[Intel-gfx] [PATCH 09/19] drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()

[Ville Syrjälä]

On Wed, Aug 06, 2014 at 03:23:01PM +0200, Daniel Vetter wrote:
> On Wed, Aug 06, 2014 at 02:49:52PM +0300, ville.syrjala at linux.intel.com 
> wrote:
> > From: Ville Syrj?l? 
> > 
> > Currently it's possible that the following will happen:
> > 1. drm_wait_vblank() calls drm_vblank_get()
> > 2. drm_vblank_off() gets called
> > 3. drm_wait_vblank() calls drm_queue_vblank_event() which
> >adds the event to the queue event though vblank interrupts
> >are currently disabled (and may not be re-enabled ever again).
> > 
> > To fix the problem, add another vblank->enabled check into
> > drm_queue_vblank_event().
> > 
> > drm_vblank_off() holds event_lock around the vblank disable,
> > so no further locking needs to be added to drm_queue_vblank_event().
> > vblank disable from another source is not possible since
> > drm_wait_vblank() already holds a vblank reference.
> > 
> > Reviewed-by: Matt Roper 
> > Signed-off-by: Ville Syrj?l? 
> 
> I guess the window is too small here to make this reproducible in a test?

I must admit that I didn't even try. I supposed I could try it now.

> Especially since each attempt will take a few hundred ms ...
> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_irq.c | 13 +
> >  1 file changed, 13 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 9353609..b2428cb 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -1270,6 +1270,7 @@ static int drm_queue_vblank_event(struct drm_device 
> > *dev, int pipe,
> >   union drm_wait_vblank *vblwait,
> >   struct drm_file *file_priv)
> >  {
> > +   struct drm_vblank_crtc *vblank = >vblank[pipe];
> > struct drm_pending_vblank_event *e;
> > struct timeval now;
> > unsigned long flags;
> > @@ -1293,6 +1294,18 @@ static int drm_queue_vblank_event(struct drm_device 
> > *dev, int pipe,
> >  
> > spin_lock_irqsave(>event_lock, flags);
> >  
> > +   /*
> > +* drm_vblank_off() might have been called after we called
> > +* drm_vblank_get(). drm_vblank_off() holds event_lock
> > +* around the vblank disable, so no need for further locking.
> > +* The reference from drm_vblank_get() protects against
> > +* vblank disable from another source.
> > +*/
> > +   if (!vblank->enabled) {
> > +   ret = -EINVAL;
> > +   goto err_unlock;
> > +   }
> > +
> > if (file_priv->event_space < sizeof e->event) {
> > ret = -EBUSY;
> > goto err_unlock;
> > -- 
> > 1.8.5.5
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx at 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

-- 
Ville Syrj?l?
Intel OTC

[PATCH] drm: idiot-proof vblank

[Rob Clark]

On Wed, Aug 6, 2014 at 4:28 PM, Daniel Vetter  wrote:
> On Wed, Aug 06, 2014 at 03:06:40PM -0400, Rob Clark wrote:
>> On Wed, Aug 6, 2014 at 2:12 PM, Ville Syrj?l?
>>  wrote:
>> > On Wed, Aug 06, 2014 at 01:16:59PM -0400, Rob Clark wrote:
>> >> After spending slightly more time than I'd care to admit debugging the
>> >> various and presumably spectacular way things fail when you pass too low
>> >> a value to drm_vblank_init() (thanks console-lock for not letting me see
>> >> the carnage!), I decided it might be a good idea to add some sanity
>> >> checking.
>> >
>> > Hmm. Could we instead do some kind of cross checking in
>> > drm_vblank_init() and drm_crtc_init() to avoid having to sprinkle this
>> > stuff all over the place? I guess the checks would need to happen both
>> > ways since the driver might call those in either order.
>>
>> it would be nice if we could just drop the arg to drm_vblank_init() to
>> have one less place to screw things up.. I suspect that is some UMS
>> relic..
>
> It is.
>
>> We could possibly co-check in vblank_init() each crtc_init()..  I
>> guess it would be marginally fewer lines of diffstat, and in theory
>> the driver could still manage to screw up by just passing bogus pipe
>> #'s.  The vblank code already has these sort of checks for things that
>> are potentially userspace facing, so adding the same check (plus
>> WARN_ON()) to the internal fxns seemed like the logical and
>> straightforward solution.
>
> The problem is that currently don't have a point where we know that all
> crtc are set up, so we can't just chuck the right drm_vblank_init call
> into a modeset setup function.
>
> But we could add a drm_mode_set_vblank_init which looks at
> dev->mode_config.num_crtcs and for paranoia sets a flag which makes all
> subsequent crtc_init fail loud.
>
> In a perfect world we'd just move the vblank data into drm_crtc, but we're
> not yet there. Ville' has already neatly split things up (mostly), and
> I've started to consistently add and use vblank functions which take a
> drm_crtc * instead of a pipe integer. But really not there yet by far.

yeah, in the mean time we should add my simple patch to make drm
developers not hate console_lock quite so much :-)

BR,
-R

> -Daniel
>
>>
>> BR,
>> -R
>>
>> >>
>> >> Signed-off-by: Rob Clark 
>> >> ---
>> >> btw, I wonder if we could add a module param hack to toss initial modeset
>> >> off to a workqueue to sneak it out from the tyranny of console_lock?
>> >>
>> >>  drivers/gpu/drm/drm_irq.c | 24 
>> >>  1 file changed, 24 insertions(+)
>> >>
>> >> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> >> index 0de123a..6f16a104 100644
>> >> --- a/drivers/gpu/drm/drm_irq.c
>> >> +++ b/drivers/gpu/drm/drm_irq.c
>> >> @@ -730,6 +730,8 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp);
>> >>   */
>> >>  u32 drm_vblank_count(struct drm_device *dev, int crtc)
>> >>  {
>> >> + if (WARN_ON(crtc >= dev->num_crtcs))
>> >> + return 0;
>> >>   return atomic_read(>vblank[crtc].count);
>> >>  }
>> >>  EXPORT_SYMBOL(drm_vblank_count);
>> >> @@ -752,6 +754,9 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, 
>> >> int crtc,
>> >>  {
>> >>   u32 cur_vblank;
>> >>
>> >> + if (WARN_ON(crtc >= dev->num_crtcs))
>> >> + return 0;
>> >> +
>> >>   /* Read timestamp from slot of _vblank_time ringbuffer
>> >>* that corresponds to current vblank count. Retry if
>> >>* count has incremented during readout. This works like
>> >> @@ -927,6 +932,9 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
>> >>   unsigned long irqflags;
>> >>   int ret = 0;
>> >>
>> >> + if (WARN_ON(crtc >= dev->num_crtcs))
>> >> + return -EINVAL;
>> >> +
>> >>   spin_lock_irqsave(>vbl_lock, irqflags);
>> >>   /* Going from 0->1 means we have to enable interrupts again */
>> >>   if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
>> >> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>> >>  {
>> >>   BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);
>> >>
>> >> + if (WARN_ON(crtc >= dev->num_crtcs))
>> >> + return;
>> >> +
>> >>   /* Last user schedules interrupt disable */
>> >>   if (atomic_dec_and_test(>vblank[crtc].refcount) &&
>> >>   (drm_vblank_offdelay > 0))
>> >> @@ -1019,6 +1030,9 @@ void drm_vblank_off(struct drm_device *dev, int 
>> >> crtc)
>> >>   unsigned long irqflags;
>> >>   unsigned int seq;
>> >>
>> >> + if (WARN_ON(crtc >= dev->num_crtcs))
>> >> + return;
>> >> +
>> >>   spin_lock_irqsave(>vbl_lock, irqflags);
>> >>   vblank_disable_and_save(dev, crtc);
>> >>   wake_up(>vblank[crtc].queue);
>> >> @@ -1078,6 +1092,9 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
>> >>  {
>> >>   unsigned long irqflags;
>> >>
>> >> + if (WARN_ON(crtc >= dev->num_crtcs))
>> >> + return;
>> >> +
>> >>   

[PATCH] drm/panel/simple: add optronics B101XTN01.0

[Rob Clark]

LVDS panel, make/model described as:

AU Optronics Corporation - B101XTN01.0 (H/W:0A)

See:
http://www.encore-electronic.com/media/B101XTN01.0.pdf

Signed-off-by: Rob Clark 
---
 drivers/gpu/drm/panel/panel-simple.c | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/panel/panel-simple.c 
b/drivers/gpu/drm/panel/panel-simple.c
index a251361..16119ba 100644
--- a/drivers/gpu/drm/panel/panel-simple.c
+++ b/drivers/gpu/drm/panel/panel-simple.c
@@ -447,6 +447,29 @@ static const struct panel_desc samsung_ltn101nt05 = {
},
 };

+static const struct drm_display_mode optronics_lvds_mode = {
+   .clock = 72000,
+   .hdisplay = 1366,
+   .hsync_start = 1366 + 20,
+   .hsync_end = 1366 + 20 + 70,
+   .htotal = 1366 + 20 + 70,
+   .vdisplay = 768,
+   .vsync_start = 768 + 14,
+   .vsync_end = 768 + 14 + 42,
+   .vtotal = 768 + 14 + 42,
+   .vrefresh = 60,
+   .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+};
+
+static const struct panel_desc optronics_lvds = {
+   .modes = _lvds_mode,
+   .num_modes = 1,
+   .size = {
+   .width = 1366,
+   .height = 768,
+   },
+};
+
 static const struct of_device_id platform_of_match[] = {
{
.compatible = "auo,b101aw03",
@@ -476,6 +499,10 @@ static const struct of_device_id platform_of_match[] = {
.compatible = "samsung,ltn101nt05",
.data = _ltn101nt05,
}, {
+   .compatible = "optronics,b101xtn01",
+   .data = _lvds,
+   }, {
+   }, {
.compatible = "simple-panel",
}, {
/* sentinel */
-- 
1.9.3

[Intel-gfx] [PATCH 15/19] drm: Update vblank->last in drm_update_vblank_count()

[Ville Syrjälä]

On Wed, Aug 06, 2014 at 03:08:25PM +0200, Daniel Vetter wrote:
> On Wed, Aug 06, 2014 at 02:49:58PM +0300, ville.syrjala at linux.intel.com 
> wrote:
> > From: Ville Syrj?l? 
> > 
> > We should update the last in drm_update_vblank_count() to avoid applying
> > the diff more than once. This could occur eg. if drm_vblank_off() gets
> > called multiple times for the crtc.
> > 
> > Signed-off-by: Ville Syrj?l? 
> 
> Currently we update ->last when disabling the vblank and use it when
> re-enabling it. Those calls should be symmetric, except for driver bugs.
> Imo would be better to tighten up the checks for that.
> 
> Or do I completely misunderstand what's going on here?

The issue is that we want to call drm_vblank_off() in
intel_sanitize_crtc() for already disabled crtcs in order to
to bump the refcount and set inmodeset=1 so that
drm_vblank_get() won't try to enable vblank interrupts if someone calls
drm_vblank_get() on it. So during suspend+resume we can get two
drm_vblank_off() calls for the same crtc w/o a drm_vblank_on() in
between.

Although this patch doesn't actually fix the problem really since
vblank counter query during sanitize will just return 0 because the pipe
isn't active, so there's a later patch to just override .last=0 in
intel_sanitize_crtc(). Another approach might be to set .last=0 in
drm_vblank_off() itself (after vblank_disable_and_save()), mainly
just to hide the details from the caller.


> -Daniel
> 
> > ---
> >  drivers/gpu/drm/drm_irq.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 0523f5b..67507a4 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -109,6 +109,8 @@ static void drm_update_vblank_count(struct drm_device 
> > *dev, int crtc)
> > if (diff == 0)
> > return;
> >  
> > +   vblank->last = cur_vblank;
> > +
> > /* Reinitialize corresponding vblank timestamp if high-precision query
> >  * available. Skip this step if query unsupported or failed. Will
> >  * reinitialize delayed at next vblank interrupt in that case.
> > -- 
> > 1.8.5.5
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx at 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

-- 
Ville Syrj?l?
Intel OTC

[Intel-gfx] [PATCH 15/19] drm: Update vblank->last in drm_update_vblank_count()

[Daniel Vetter]

On Wed, Aug 06, 2014 at 04:30:29PM +0300, Ville Syrj?l? wrote:
> On Wed, Aug 06, 2014 at 03:08:25PM +0200, Daniel Vetter wrote:
> > On Wed, Aug 06, 2014 at 02:49:58PM +0300, ville.syrjala at linux.intel.com 
> > wrote:
> > > From: Ville Syrj?l? 
> > > 
> > > We should update the last in drm_update_vblank_count() to avoid applying
> > > the diff more than once. This could occur eg. if drm_vblank_off() gets
> > > called multiple times for the crtc.
> > > 
> > > Signed-off-by: Ville Syrj?l? 
> > 
> > Currently we update ->last when disabling the vblank and use it when
> > re-enabling it. Those calls should be symmetric, except for driver bugs.
> > Imo would be better to tighten up the checks for that.
> > 
> > Or do I completely misunderstand what's going on here?
> 
> The issue is that we want to call drm_vblank_off() in
> intel_sanitize_crtc() for already disabled crtcs in order to
> to bump the refcount and set inmodeset=1 so that
> drm_vblank_get() won't try to enable vblank interrupts if someone calls
> drm_vblank_get() on it. So during suspend+resume we can get two
> drm_vblank_off() calls for the same crtc w/o a drm_vblank_on() in
> between.
> 
> Although this patch doesn't actually fix the problem really since
> vblank counter query during sanitize will just return 0 because the pipe
> isn't active, so there's a later patch to just override .last=0 in
> intel_sanitize_crtc(). Another approach might be to set .last=0 in
> drm_vblank_off() itself (after vblank_disable_and_save()), mainly
> just to hide the details from the caller.

Resetting ->last (if it works, need to be careful with drivers calling
_off multiple times perhaps) sounds like the much simpler option, and
probably does what everyone expects anyway.
-Daniel

> 
> 
> > -Daniel
> > 
> > > ---
> > >  drivers/gpu/drm/drm_irq.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > > index 0523f5b..67507a4 100644
> > > --- a/drivers/gpu/drm/drm_irq.c
> > > +++ b/drivers/gpu/drm/drm_irq.c
> > > @@ -109,6 +109,8 @@ static void drm_update_vblank_count(struct drm_device 
> > > *dev, int crtc)
> > >   if (diff == 0)
> > >   return;
> > >  
> > > + vblank->last = cur_vblank;
> > > +
> > >   /* Reinitialize corresponding vblank timestamp if high-precision query
> > >* available. Skip this step if query unsupported or failed. Will
> > >* reinitialize delayed at next vblank interrupt in that case.
> > > -- 
> > > 1.8.5.5
> > > 
> > > ___
> > > Intel-gfx mailing list
> > > Intel-gfx at 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
> 
> -- 
> Ville Syrj?l?
> Intel OTC

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

[PATCH igt] tests: Add kms_flip_event_leak test

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

kms_flip_event_leak will issue a page flip and close the file
descriptor before the flip has finished. This may cause the kernel
to leak the page flip event. The test itself won't actually fail but
if the kernel notices the leak and WARNs piglit will report a failure.

Signed-off-by: Ville Syrj?l? 
---
 tests/Makefile.sources  |   1 +
 tests/kms_flip_event_leak.c | 132 
 2 files changed, 133 insertions(+)
 create mode 100644 tests/kms_flip_event_leak.c

diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 0eb9369..698e290 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -66,6 +66,7 @@ TESTS_progs_M = \
kms_cursor_crc \
kms_fbc_crc \
kms_flip \
+   kms_flip_event_leak \
kms_flip_tiling \
kms_mmio_vs_cs_flip \
kms_pipe_crc_basic \
diff --git a/tests/kms_flip_event_leak.c b/tests/kms_flip_event_leak.c
new file mode 100644
index 000..9924333
--- /dev/null
+++ b/tests/kms_flip_event_leak.c
@@ -0,0 +1,132 @@
+/*
+ * Copyright ? 2014 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include 
+#include 
+#include 
+#include 
+
+#include "drmtest.h"
+#include "igt_debugfs.h"
+#include "igt_kms.h"
+#include "intel_chipset.h"
+#include "intel_batchbuffer.h"
+#include "ioctl_wrappers.h"
+
+typedef struct {
+   int drm_fd;
+   igt_display_t display;
+} data_t;
+
+/*
+ * This test tries to provoke the kernel to leak a pending page flip event
+ * when the fd is closed before the flip has completed. The test itself won't
+ * fail even if the kernel leaks the event, but the resulting dmesg WARN
+ * will cause piglit to report a failure.
+ */
+static bool test(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+   igt_plane_t *primary;
+   drmModeModeInfo *mode;
+   struct igt_fb fb[2];
+   int fd, ret;
+
+   /* select the pipe we want to use */
+   igt_output_set_pipe(output, pipe);
+   igt_display_commit(>display);
+
+   if (!output->valid) {
+   igt_output_set_pipe(output, PIPE_ANY);
+   igt_display_commit(>display);
+   return false;
+   }
+
+   primary = igt_output_get_plane(output, IGT_PLANE_PRIMARY);
+   mode = igt_output_get_mode(output);
+
+   igt_create_color_fb(data->drm_fd, mode->hdisplay, mode->vdisplay,
+   DRM_FORMAT_XRGB,
+   true, /* tiled */
+   0.0, 0.0, 0.0, [0]);
+
+   igt_plane_set_fb(primary, [0]);
+   igt_display_commit2(>display, COMMIT_LEGACY);
+
+   fd = drm_open_any();
+
+   ret = drmDropMaster(data->drm_fd);
+   igt_assert(ret == 0);
+
+   ret = drmSetMaster(fd);
+   igt_assert(ret == 0);
+
+   igt_create_color_fb(fd, mode->hdisplay, mode->vdisplay,
+   DRM_FORMAT_XRGB,
+   true, /* tiled */
+   0.0, 0.0, 0.0, [1]);
+   ret = drmModePageFlip(fd, output->config.crtc->crtc_id,
+ fb[1].fb_id, DRM_MODE_PAGE_FLIP_EVENT,
+ data);
+   igt_assert(ret == 0);
+
+   ret = close(fd);
+   igt_assert(ret == 0);
+
+   ret = drmSetMaster(data->drm_fd);
+   igt_assert(ret == 0);
+
+   igt_plane_set_fb(primary, NULL);
+   igt_output_set_pipe(output, PIPE_ANY);
+   igt_display_commit(>display);
+
+   igt_remove_fb(data->drm_fd, [0]);
+
+   return true;
+}
+
+igt_simple_main
+{
+   data_t data = {};
+   igt_output_t *output;
+   int valid_tests = 0;
+   enum pipe pipe;
+
+   igt_skip_on_simulation();
+
+   data.drm_fd = drm_open_any();
+   igt_set_vt_graphics_mode();
+
+   igt_display_init(, data.drm_fd);
+
+   for (pipe = 0; pipe 

[PATCH v2 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support

[Inki Dae]

Thanks for comments.


On 2014? 08? 05? 20:12, Thierry Reding wrote:
> On Mon, Jul 28, 2014 at 06:09:58PM +0200, Andrzej Hajda wrote:
>> On 07/28/2014 04:00 AM, Inki Dae wrote:
>>> This patch adds below two flags for LPM transfer, and it attaches LPM flags
>>> to a msg in accordance with master's mode_flags set by LCD Panel driver.
>>>
>>> MIPI_DSI_MODE_CMD_LPM
>>> - If this flag is set by Panel driver, MIPI-DSI controller will tranfer
>>> command data to Panel device in Low Power Mode.
>>
>> What do you mean by command data? It could be:
>> - all transfer in command mode of operations,
>> - transfer initialized by the driver by writing to DSIM registers.
>>
>>>
>>> MIPI_DSI_MODE_VIDEO_LPM
>>> - If this flag is set by Panel driver, MIPI-DSI controller will tranfer
>>> image data to Panel device in Low Power Mode.
>>
>> What is the meaning of this flag in case of command mode of operation?
>>
>> Maybe it would be better to create flags based on source of data/FIFOs:
>> - commands send by SFR registers,
>> - commands generated from data sent from Display Controller.
> 
> I have no idea what SFR is. But it sounds like you're talking about two
> ways to generate command packets here. We have something similar on
> Tegra, where it's called "host-driven command mode" and "DC-driven
> command mode".
> 
> In host-driven command mode the driver needs to explicitly program some
> of the DSI controller registers to send command packets. This is
> essentially a FIFO register and a control register to trigger
> transmission and poll for completion.
> 
> DC-driven command mode is typically used to update contents of a remote
> framebuffer (what MIPI calls "Type 1 Display Architecture"). This is
> done by programming a different set of registers that cause the DSI
> controller to take data output by the display controller and wrap it
> into DCS commands (e.g. write_memory_start and write_memory_continue).
> 

Exynos also supports above two command mode but we have never used
DC-driven command mode.

> I think that low power mode is more often used for command transmission
> (in host-driven mode). I'm not sure how much sense it really makes to
> transmit video data in low power mode. It also seems like low power mode
> is what all peripherals need to support (if they can do command mode).
> Hence I'd like to propose the attached patch that makes all command
> messages use low power mode.

To use low power mode, I think SoC drivers should add more codes:
checking xxx_MSG_LPM, and maybe disabling HS clock. My patch does
exactly that,
http://www.spinics.net/lists/linux-samsung-soc/msg34866.html

And what I and Andrzej don't make sure is non-continuous clock mode. Do
you know how non-continuous clock mode is related to HS clock?

> 
> The .transfer() function was really designed with initialization
> commands in mind, so it doesn't deal with mixing video data and commands
> anyway and for initialization low-power mode should be fast enough. The
> downside is that it may not be optimal for some peripherals, but it
> gives us a good solution for the general case since it should support
> all devices.
> 
> If we absolutely must have faster initialization, or if we come across a
> device that can only initialize in high speed mode, then I think we
> should introduce a new flag to allow DSI host controllers to optimize in
> those cases.

Originally, mipi-dsi framework enforces implicitly using HS mode for
video and command data as default so I added LPM relevant flags - for
video and also command data
- for host driver can consider Low Power Mode. However, your patch makes
it use
Low Power Mode for command data as default. So your patch would mean
that default transfer mode for command data is Low Power Mode, but HS
mode for video data.

Do you intend to control transfer mode - HS or LPM - only for command
data? If so, we would need only one flag, i.e., MIPI_DSI_MODE_HS.

Thanks,
Inki Dae

> 
> Note that this is based on some of my local patches, so it won't apply
> as-is. But if anybody wants to give this a go it should be easy to apply
> manually as well.
> 
> Thierry
> 

[Intel-gfx] [PATCH 2/2] drm/i915: Free pending page flip events at .preclose()

[Daniel Vetter]

On Wed, Aug 06, 2014 at 02:02:51PM +0300, ville.syrjala at linux.intel.com 
wrote:
> From: Ville Syrj?l? 
> 
> If there are pending page flips when the fd gets closed those page
> flips may have events associated to them. When the page flip eventually
> completes it will queue the event to file_priv->event_list, but that
> may be too late and file_priv->event_list has already been cleaned up.
> Thus we leak a bit of kernel memory in the form of the event structure.
> 
> To avoid such problems clear out such pending events from
> intel_crtc->unpin_work at ->preclose(). Any event that already made it
> to file_priv->event_list will get cleaned up by the drm_release_events()
> a bit later.
> 
> We can ignore the file_priv->event_space accounting since file_priv is
> going away. This is already how drm core deals with pending vblank
> events, which are maintained by the drm core.
> 
> What saves us from a total disaster (ie. dereferencing and alrady
> freed file_priv) is the fact that the fb descruction triggers a modeset
> and there we wait for pending flips.
> 
> Signed-off-by: Ville Syrj?l? 
> ---
>  drivers/gpu/drm/i915/i915_dma.c  |  3 +++
>  drivers/gpu/drm/i915/intel_display.c | 22 ++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 2e7f03a..c965698 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1981,6 +1981,9 @@ void i915_driver_preclose(struct drm_device *dev, 
> struct drm_file *file)
>   i915_gem_context_close(dev, file);
>   i915_gem_release(dev, file);
>   mutex_unlock(>struct_mutex);
> +
> + if (drm_core_check_feature(dev, DRIVER_MODESET))
> + intel_modeset_preclose(dev, file);
>  }
>  
>  void i915_driver_postclose(struct drm_device *dev, struct drm_file *file)
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 883af0b..4230e4a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13416,3 +13416,25 @@ intel_display_print_error_state(struct 
> drm_i915_error_state_buf *m,
>   err_printf(m, "  VSYNC: %08x\n", error->transcoder[i].vsync);
>   }
>  }
> +
> +void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
> +{
> + struct intel_crtc *crtc;
> +
> + for_each_intel_crtc(dev, crtc) {
> + struct intel_unpin_work *work;
> + unsigned long irqflags;
> +
> + spin_lock_irqsave(>event_lock, irqflags);
> +
> + work = crtc->unpin_work;
> +
> + if (work && work->event &&
> + work->event->base.file_priv == file) {
> + kfree(work->event);
> + work->event = NULL;
> + }
> +
> + spin_unlock_irqrestore(>event_lock, irqflags);
> + }

I wonder whether we shouldn't do this in the drm core, with a per-file
event list. Anyway, good for now together with the igt, we can pimp this
later.

Queued for -next, thanks for the patch.
-Daniel

> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 28d185d..8f04ba8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -888,6 +888,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode 
> *mode,
>struct intel_crtc_config *pipe_config);
>  int intel_format_to_fourcc(int format);
>  void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
> +void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>  
>  
>  /* intel_dp.c */
> -- 
> 1.8.5.5
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at 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: Don't grab an fb reference for the idr

[Daniel Vetter]

On Wed, Aug 06, 2014 at 09:12:42AM -0400, Rob Clark wrote:
> On Wed, Aug 6, 2014 at 8:37 AM, Daniel Vetter  wrote:
> > On Wed, Aug 06, 2014 at 07:11:28AM -0400, Rob Clark wrote:
> >> On Wed, Aug 6, 2014 at 3:10 AM, Daniel Vetter  
> >> wrote:
> >> > The current refcounting scheme is that the fb lookup idr also holds a
> >> > reference. This works out nicely bacause thus far we've always
> >> > explicitly cleaned up idr entries for framebuffers:
> >> > - Userspace fbs get removed in the rmfb ioctl or when the drm file
> >> >   gets closed.
> >> > - Kernel fbs (for fbdev emulation) get cleaned up by the driver code
> >> >   at module unload time.
> >> >
> >> > But now i915 also reconstructs the bios fbs for a smooth transition.
> >> > And that fb is purely transitional and should get removed immmediately
> >> > once all crtcs stop using it. Of course if the i915 fbdev code decides
> >> > to reuse it as the main fbdev fb then it shouldn't be cleaned up, but
> >> > in that case the fbdev code will grab it's own reference.
> >> >
> >> > The problem is now that we also want to register that takeover fb in
> >> > the idr, so that userspace can do a smooth transition (animated maybe
> >> > even!) itself. But currently we have no one who will clean up the idr
> >> > reference once that fb isn't useful any more, and so essentially leak
> >> > it.
> >>
> >> ewww..  couldn't you do some scheme on lastclose to check if no more
> >> crtc's are scanning out that fb, and if not then remove the idr?
> >
> > There's no natural point really but when we drop the last reference for
> > it. Going the weak reference route looked the most natural. And I honestly
> > expect other drivers to eventually do the same - forcing a modeset on
> > boot-up is kinda not too pretty, and permanently reserving a big
> > framebuffer just for the bios doesn't sound good either. This approach
> > would nicely solve it for everyone.
> 
> hmm, maybe somebody switched my coffee with decaf, but why isn't
> lastclose a natural point?

There is no lastclose for the bios ;-)

Let me elaborate on what happens:

1. BIOS sets up an initial config with a framebuffer in stolen.

2. i915 takes over and reconstructs all the state, so now we have all the
crtcs enabled using a framebuffer for all of them which wraps the bios
allocation.

2b. (optional) reuse that framebuffer for fbdev.

-> That special bios fb has the following references:
- 1 reference for each crtc that's using it
- 1 optional reference if it's reused as the fbdev fb
- 1 reference for the idr

3. Userspace takes over, potentially doing a getfb on the current
(bios-inherited) fb for smooth transition, but then does a modeset to its
own fb.

-> After this all the we've dropped the crtc references and we also want
to drop the idr reference (since no one will ever use this framebuffer
again). But there's simply no good place to do that. Lastclose might only
happen before we shut down the system again, which is a bit too late.

Note that the getfb call creates a gem handle for the fb object, so the
backing storage might survive for a lot longer than the fb.

> ofc if that really doesn't work, the weak-ref thing seems like it
> would solve it nicely.  But if there were a simple solution that
> didn't involve making fb refcnting more complex, I guess I would
> prefer that

Well I didn't come up with anything else really. Plan b would be to add
hooks after any plane updates and manually check whether that special fb
has lost all but its idr reference, and if so clean it up. That seems to
be a lot more fragile and convoluted than converting the idr to a weak
reference.

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

[PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup()

[Daniel Vetter]

On Wed, Aug 06, 2014 at 01:51:41PM +0300, Ville Syrj?l? wrote:
> On Wed, Aug 06, 2014 at 03:22:46AM +0200, Mario Kleiner wrote:
> > Calling vblank_disable_fn() will cause that function to no-op
> > if !dev->vblank_disable_allowed for some kms drivers, e.g.,
> > on nouveau-kms. This can cause the gpu vblank irq's to not get
> > disabled before freeing the dev->vblank array, so if a
> > vblank irq fires and calls into drm_handle_vblank() after
> > drm_vblank_cleanup() completes, it will cause use-after-free
> > access to dev->vblank array.
> > 
> > Call vblank_disable_and_save unconditionally, so vblank irqs
> > are guaranteed to be off, before we delete the data structures
> > on which they operate.
> > 
> > Signed-off-by: Mario Kleiner 
> > Cc: stable at vger.kernel.org

Imo cc: stable isn't justified for these patches which fix stuff that
normal users don't really see (driver load failure and module reload for
kms drivers never tends to happen for normal users).

So I've dropped that and pulled the 2 patches Ville reviewd into my
topic/vblank-rework branch for 3.18.

Thanks, Daniel

> 
> No idea what games nouveau is playign with that flag, but this patch
> should be fine at least for drivers that don't do such things.
> 
> Reviewed-by: Ville Syrj?l? 
> 
> > ---
> >  drivers/gpu/drm/drm_irq.c | 5 -
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index 89e91e3..22e2bba9 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -164,6 +164,7 @@ static void vblank_disable_fn(unsigned long arg)
> >  void drm_vblank_cleanup(struct drm_device *dev)
> >  {
> > int crtc;
> > +   unsigned long irqflags;
> >  
> > /* Bail if the driver didn't call drm_vblank_init() */
> > if (dev->num_crtcs == 0)
> > @@ -171,7 +172,9 @@ void drm_vblank_cleanup(struct drm_device *dev)
> >  
> > for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
> > del_timer_sync(>vblank[crtc].disable_timer);
> > -   vblank_disable_fn((unsigned long)>vblank[crtc]);
> > +   spin_lock_irqsave(>vbl_lock, irqflags);
> > +   vblank_disable_and_save(dev, crtc);
> > +   spin_unlock_irqrestore(>vbl_lock, irqflags);
> > }
> >  
> > kfree(dev->vblank);
> > -- 
> > 1.9.1
> > 
> > ___
> > dri-devel mailing list
> > dri-devel at lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Ville Syrj?l?
> Intel OTC
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Bug 82201] [HAWAII] GPU doesn't reclock, poor 3D performance

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=82201

--- Comment #25 from Kai  ---
(In reply to comment #24)
> (In reply to comment #23)
> > (In reply to comment #22)
> > > do you have radeon.dpm=0 in smoe /etc/modprobe.d or somewhere like that 
> > > file?
> > 
> > No:
> > # grep -nHr radeon.dpm /etc/*
> > /etc/default/grub:9:GRUB_CMDLINE_LINUX_DEFAULT="quiet radeon.dpm=1"
> 
> What's the result of "cat /sys/module/radeon/parameters/dpm" when you don't
> specify the "radeon.dpm=1" on the kernel commandline?

$ cat /sys/module/radeon/parameters/dpm
-1

I verified with

$ dmesg | grep -i "command line" && cat /sys/module/radeon/parameters/dpm
[0.00] Command line:
BOOT_IMAGE=/vmlinuz-3.16.0-rc6-citadel+fdo-att-104101
root=/dev/mapper/citadel--vg-vol--root ro quiet
[0.00] Kernel command line:
BOOT_IMAGE=/vmlinuz-3.16.0-rc6-citadel+fdo-att-104101
root=/dev/mapper/citadel--vg-vol--root ro quiet

that I removed the radeon.dpm=1 from the kernel command line before booting.
Kernel version is Git:~agdf5/linux:drm-next-3.17-rebased-on-fixes:fa78380797 +
patch from attachment 104101.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/3f940f6f/attachment-0001.html>

[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=82162

--- Comment #18 from sarnex  ---
(In reply to comment #17)
> (In reply to comment #16)
> > Any chance of seeing this in the git?
> 
> Yeah, but we might want to investigate further.
> 
> Somehow the size for the CMASK buffer ends up beeing zero. It's good to
> catch that case, but we might want to figure out why it happened in the
> first place.

Anything I can do to help?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/063bc9d6/attachment.html>

[Intel-gfx] [PATCH 17/19] drm/i915: Clear .last vblank count before drm_vblank_off() when sanitizing crtc state

[Daniel Vetter]

On Wed, Aug 06, 2014 at 02:50:00PM +0300, ville.syrjala at linux.intel.com 
wrote:
> From: Ville Syrj?l? 
> 
> We call drm_vblank_off() during crtc sanitation to make sure the
> software and hardware states agree. However drm_vblank_off() will
> try to update the vblank timestamp and sequence number which lands
> us in some trouble.
> 
> As the pipe is disabled the hardware frame counter query will
> return 0. If the .last doesn't match the code will try to add the
> difference to the user visible sequence number. During driver init
> that's OK as .last == 0, but during resume .last may be anything.
> So we should make sure we don't try to apply the diff here by zeroing
> .last beforehand. Otherwise there maybe be a random jump in the user
> visible sequence number across suspend.
> 
> Signed-off-by: Ville Syrj?l? 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index ae5f20d..c00bcd0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -12918,8 +12918,11 @@ static void intel_sanitize_crtc(struct intel_crtc 
> *crtc)
>   /* restore vblank interrupts to correct state */
>   if (crtc->active)
>   drm_vblank_on(dev, crtc->pipe);
> - else
> + else {
> + /* avoid random jumps in seq/ts */
> + dev->vblank[crtc->pipe].last = 0;

Should we have a drm_vblank_reset for this? I guess other drivers will
have similar issues with "sorry, just lost it all" kind of transitions.

Also this case should only happen when we enter the system resume with the
pipes in dpms off state. In that case we should have sampled something in
drm_vblank_off already and resampling doesn't look like a good idea.

Do we instead need some protection in drm_vblank_off to avoid re-sampling?
-Daniel

>   drm_vblank_off(dev, crtc->pipe);
> + }
>  
>   /* We need to sanitize the plane -> pipe mapping first because this will
>* disable the crtc (and hence change the state) if it is wrong. Note
> -- 
> 1.8.5.5
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at 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

[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=82162

--- Comment #17 from Christian K?nig  ---
(In reply to comment #16)
> Any chance of seeing this in the git?

Yeah, but we might want to investigate further.

Somehow the size for the CMASK buffer ends up beeing zero. It's good to catch
that case, but we might want to figure out why it happened in the first place.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/5fa84b70/attachment.html>

[Intel-gfx] [PATCH 09/19] drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()

[Daniel Vetter]

On Wed, Aug 06, 2014 at 02:49:52PM +0300, ville.syrjala at linux.intel.com 
wrote:
> From: Ville Syrj?l? 
> 
> Currently it's possible that the following will happen:
> 1. drm_wait_vblank() calls drm_vblank_get()
> 2. drm_vblank_off() gets called
> 3. drm_wait_vblank() calls drm_queue_vblank_event() which
>adds the event to the queue event though vblank interrupts
>are currently disabled (and may not be re-enabled ever again).
> 
> To fix the problem, add another vblank->enabled check into
> drm_queue_vblank_event().
> 
> drm_vblank_off() holds event_lock around the vblank disable,
> so no further locking needs to be added to drm_queue_vblank_event().
> vblank disable from another source is not possible since
> drm_wait_vblank() already holds a vblank reference.
> 
> Reviewed-by: Matt Roper 
> Signed-off-by: Ville Syrj?l? 

I guess the window is too small here to make this reproducible in a test?
Especially since each attempt will take a few hundred ms ...
-Daniel

> ---
>  drivers/gpu/drm/drm_irq.c | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 9353609..b2428cb 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1270,6 +1270,7 @@ static int drm_queue_vblank_event(struct drm_device 
> *dev, int pipe,
> union drm_wait_vblank *vblwait,
> struct drm_file *file_priv)
>  {
> + struct drm_vblank_crtc *vblank = >vblank[pipe];
>   struct drm_pending_vblank_event *e;
>   struct timeval now;
>   unsigned long flags;
> @@ -1293,6 +1294,18 @@ static int drm_queue_vblank_event(struct drm_device 
> *dev, int pipe,
>  
>   spin_lock_irqsave(>event_lock, flags);
>  
> + /*
> +  * drm_vblank_off() might have been called after we called
> +  * drm_vblank_get(). drm_vblank_off() holds event_lock
> +  * around the vblank disable, so no need for further locking.
> +  * The reference from drm_vblank_get() protects against
> +  * vblank disable from another source.
> +  */
> + if (!vblank->enabled) {
> + ret = -EINVAL;
> + goto err_unlock;
> + }
> +
>   if (file_priv->event_space < sizeof e->event) {
>   ret = -EBUSY;
>   goto err_unlock;
> -- 
> 1.8.5.5
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at 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

[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=82162

--- Comment #16 from sarnex  ---
Any chance of seeing this in the git?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/44138098/attachment.html>

[Bug 82201] [HAWAII] GPU doesn't reclock, poor 3D performance

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=82201

--- Comment #24 from Christian K?nig  ---
(In reply to comment #23)
> (In reply to comment #22)
> > do you have radeon.dpm=0 in smoe /etc/modprobe.d or somewhere like that 
> > file?
> 
> No:
> # grep -nHr radeon.dpm /etc/*
> /etc/default/grub:9:GRUB_CMDLINE_LINUX_DEFAULT="quiet radeon.dpm=1"

What's the result of "cat /sys/module/radeon/parameters/dpm" when you don't
specify the "radeon.dpm=1" on the kernel commandline?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/a29aa51f/attachment.html>

[Bug 81680] [r600g] Firefox crashes with hardware acceleration turned on

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=81680

--- Comment #11 from Eugene  ---
addr2line 0x1ba0a1 -e /usr/lib/x86_64-linux-gnu/dri/r600_dri.so
/build/buildd/mesa-10.3~git1408060730.c40d7d+gallium/build/dri/src/mesa/../../../../src/mesa/state_tracker/st_cb_drawpixels.c:1102

addr2line 0x18c0b9 -e /usr/lib/x86_64-linux-gnu/dri/r600_dri.so
/build/buildd/mesa-10.3~git1408060730.c40d7d+gallium/build/dri/src/mesa/../../../../src/mesa/vbo/vbo_attrib_tmp.h:161

addr2line 0x1145dd -e /usr/lib/x86_64-linux-gnu/dri/r600_dri.so
/build/buildd/mesa-10.3~git1408060730.c40d7d+gallium/build/dri/src/mesa/../../../../src/mesa/main/glformats.c:418

addr2line 0x93e6b -e /usr/lib/x86_64-linux-gnu/dri/r600_dri.so
/build/buildd/mesa-10.3~git1408060730.c40d7d+gallium/build/dri/src/mesa/../../../../src/mesa/main/context.c:1255

addr2line 0x190220 -e /usr/lib/x86_64-linux-gnu/dri/r600_dri.so
/build/buildd/mesa-10.3~git1408060730.c40d7d+gallium/build/dri/src/mesa/../../../../src/gallium/auxiliary/util/u_format_r11g11b10f.h:112

addr2line 0x28ea1f -e /usr/lib/x86_64-linux-gnu/dri/r600_dri.so
/build/buildd/mesa-10.3~git1408060730.c40d7d+gallium/build/dri/src/glsl/../../../../src/glsl/list.h:440
(discriminator 2)

addr2line 0x2657be -e /usr/lib/x86_64-linux-gnu/dri/r600_dri.so
/build/buildd/mesa-10.3~git1408060730.c40d7d+gallium/build/dri/src/glsl/../../../../src/glsl/ir_clone.cpp:193


addr2line 0x2621e2 -e /usr/lib/x86_64-linux-gnu/dri/r600_dri.so
/build/buildd/mesa-10.3~git1408060730.c40d7d+gallium/build/dri/src/glsl/../../../../src/glsl/ir.cpp:632

addr2line 0x42cce -e /usr/lib/x86_64-linux-gnu/dri/r600_dri.so
??:0

addr2line 0x1c318 -e /usr/lib/x86_64-linux-gnu/dri/r600_dri.so
??:0

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/4fbffa31/attachment.html>

[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=82162

--- Comment #15 from sarnex  ---
(In reply to comment #14)
> (In reply to comment #13)
> > Sorry, it appears the issue has not been fixed. The message "radeon: Failed
> > to allocate a buffer" no longer prints, but the original
> > radeon_gem_object_create message still spams dmesg and kern.log.
> 
> Did you restart X after building Mesa with the patch?

Wow, I'm an idiot. Yeah, everything is completely fixed after restarting X. No
side effects or log spam. Thank you based Michel.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/a55853eb/attachment.html>

[Intel-gfx] [PATCH 16/19] drm: Store the vblank timestamp when adjusting the counter during disable

[Daniel Vetter]

On Wed, Aug 06, 2014 at 02:49:59PM +0300, ville.syrjala at linux.intel.com 
wrote:
> From: Ville Syrj?l? 
> 
> During vblank disable the code tries to guess based on the
> timestamps whether we just missed one vblank or not. And if so
> it increments the counter. However it forgets to store the new
> timestamp to the approriate slot in our timestamp ring buffer.
> So anyone querying the timestamp for the resulting sequence
> number would get a stale timestamp. Fix it up by storing the
> new timestamp.
> 
> Signed-off-by: Ville Syrj?l? 
> ---
>  drivers/gpu/drm/drm_irq.c | 7 +++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 67507a4..e927e5f 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -203,6 +203,13 @@ static void vblank_disable_and_save(struct drm_device 
> *dev, int crtc)
>* hope for the best.
>*/
>   if ((vblrc > 0) && (abs64(diff_ns) > 100)) {

We should use DRM_REDUNDANT_VBLIRQ_THRESH_NS here for symmtry. With that
addressed this is Reviewed-by: Daniel Vetter 
> + /* Store new timestamp in ringbuffer. */
> + vblanktimestamp(dev, crtc, vblcount + 1) = tvblank;
> +
> + /* Increment cooked vblank count. This also atomically commits
> +  * the timestamp computed above.
> +  */
> + smp_mb__before_atomic();
>   atomic_inc(>count);
>   smp_mb__after_atomic();
>   }
> -- 
> 1.8.5.5
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at 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

[Intel-gfx] [PATCH 14/19] drm: Don't update vblank timestamp when the counter didn't change

[Daniel Vetter]

On Wed, Aug 06, 2014 at 02:56:14PM +0200, Daniel Vetter wrote:
> On Wed, Aug 06, 2014 at 02:49:57PM +0300, ville.syrjala at linux.intel.com 
> wrote:
> > From: Ville Syrj?l? 
> > 
> > If we already have a timestamp for the current vblank counter, don't
> > update it with a new timestmap. Small errors can creep in between two
> > timestamp queries for the same vblank count, which could be confusing to
> > userspace when it queries the timestamp for the same vblank sequence
> > number twice.
> > 
> > This problem gets exposed when the vblank disable timer is not used
> > (or is set to expire quickly) and thus we can get multiple vblank
> > disable<->enable transition during the same frame which would all
> > attempt to update the timestamp with the latest estimate.
> > 
> > Testcase: igt/kms_flip/flip-vs-expired-vblank
> > Signed-off-by: Ville Syrj?l? 
> 
> Reviewed-by: Daniel Vetter 

On second consideration this seems to just paper over drivers that enable
vblanks a few too many times. Or a bug in the vblank_get handling in
drm_irq.c. If it's just that I think we should just tighten up the checks
to catch that and drop this patch here.
-Daniel

> > ---
> >  drivers/gpu/drm/drm_irq.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> > index af33df1..0523f5b 100644
> > --- a/drivers/gpu/drm/drm_irq.c
> > +++ b/drivers/gpu/drm/drm_irq.c
> > @@ -106,6 +106,9 @@ static void drm_update_vblank_count(struct drm_device 
> > *dev, int crtc)
> > DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
> >   crtc, diff);
> >  
> > +   if (diff == 0)
> > +   return;
> > +
> > /* Reinitialize corresponding vblank timestamp if high-precision query
> >  * available. Skip this step if query unsupported or failed. Will
> >  * reinitialize delayed at next vblank interrupt in that case.
> > -- 
> > 1.8.5.5
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx at 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

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

[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=82162

--- Comment #14 from Michel D?nzer  ---
(In reply to comment #13)
> Sorry, it appears the issue has not been fixed. The message "radeon: Failed
> to allocate a buffer" no longer prints, but the original
> radeon_gem_object_create message still spams dmesg and kern.log.

Did you restart X after building Mesa with the patch?

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/bde0754e/attachment-0001.html>

[Intel-gfx] [PATCH 15/19] drm: Update vblank->last in drm_update_vblank_count()

[Daniel Vetter]

On Wed, Aug 06, 2014 at 02:49:58PM +0300, ville.syrjala at linux.intel.com 
wrote:
> From: Ville Syrj?l? 
> 
> We should update the last in drm_update_vblank_count() to avoid applying
> the diff more than once. This could occur eg. if drm_vblank_off() gets
> called multiple times for the crtc.
> 
> Signed-off-by: Ville Syrj?l? 

Currently we update ->last when disabling the vblank and use it when
re-enabling it. Those calls should be symmetric, except for driver bugs.
Imo would be better to tighten up the checks for that.

Or do I completely misunderstand what's going on here?
-Daniel

> ---
>  drivers/gpu/drm/drm_irq.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 0523f5b..67507a4 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -109,6 +109,8 @@ static void drm_update_vblank_count(struct drm_device 
> *dev, int crtc)
>   if (diff == 0)
>   return;
>  
> + vblank->last = cur_vblank;
> +
>   /* Reinitialize corresponding vblank timestamp if high-precision query
>* available. Skip this step if query unsupported or failed. Will
>* reinitialize delayed at next vblank interrupt in that case.
> -- 
> 1.8.5.5
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at 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: idiot-proof vblank

[Rob Clark]

On Wed, Aug 6, 2014 at 2:12 PM, Ville Syrj?l?
 wrote:
> On Wed, Aug 06, 2014 at 01:16:59PM -0400, Rob Clark wrote:
>> After spending slightly more time than I'd care to admit debugging the
>> various and presumably spectacular way things fail when you pass too low
>> a value to drm_vblank_init() (thanks console-lock for not letting me see
>> the carnage!), I decided it might be a good idea to add some sanity
>> checking.
>
> Hmm. Could we instead do some kind of cross checking in
> drm_vblank_init() and drm_crtc_init() to avoid having to sprinkle this
> stuff all over the place? I guess the checks would need to happen both
> ways since the driver might call those in either order.

it would be nice if we could just drop the arg to drm_vblank_init() to
have one less place to screw things up.. I suspect that is some UMS
relic..

We could possibly co-check in vblank_init() each crtc_init()..  I
guess it would be marginally fewer lines of diffstat, and in theory
the driver could still manage to screw up by just passing bogus pipe
#'s.  The vblank code already has these sort of checks for things that
are potentially userspace facing, so adding the same check (plus
WARN_ON()) to the internal fxns seemed like the logical and
straightforward solution.

BR,
-R

>>
>> Signed-off-by: Rob Clark 
>> ---
>> btw, I wonder if we could add a module param hack to toss initial modeset
>> off to a workqueue to sneak it out from the tyranny of console_lock?
>>
>>  drivers/gpu/drm/drm_irq.c | 24 
>>  1 file changed, 24 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
>> index 0de123a..6f16a104 100644
>> --- a/drivers/gpu/drm/drm_irq.c
>> +++ b/drivers/gpu/drm/drm_irq.c
>> @@ -730,6 +730,8 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp);
>>   */
>>  u32 drm_vblank_count(struct drm_device *dev, int crtc)
>>  {
>> + if (WARN_ON(crtc >= dev->num_crtcs))
>> + return 0;
>>   return atomic_read(>vblank[crtc].count);
>>  }
>>  EXPORT_SYMBOL(drm_vblank_count);
>> @@ -752,6 +754,9 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, 
>> int crtc,
>>  {
>>   u32 cur_vblank;
>>
>> + if (WARN_ON(crtc >= dev->num_crtcs))
>> + return 0;
>> +
>>   /* Read timestamp from slot of _vblank_time ringbuffer
>>* that corresponds to current vblank count. Retry if
>>* count has incremented during readout. This works like
>> @@ -927,6 +932,9 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
>>   unsigned long irqflags;
>>   int ret = 0;
>>
>> + if (WARN_ON(crtc >= dev->num_crtcs))
>> + return -EINVAL;
>> +
>>   spin_lock_irqsave(>vbl_lock, irqflags);
>>   /* Going from 0->1 means we have to enable interrupts again */
>>   if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
>> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
>>  {
>>   BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);
>>
>> + if (WARN_ON(crtc >= dev->num_crtcs))
>> + return;
>> +
>>   /* Last user schedules interrupt disable */
>>   if (atomic_dec_and_test(>vblank[crtc].refcount) &&
>>   (drm_vblank_offdelay > 0))
>> @@ -1019,6 +1030,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>>   unsigned long irqflags;
>>   unsigned int seq;
>>
>> + if (WARN_ON(crtc >= dev->num_crtcs))
>> + return;
>> +
>>   spin_lock_irqsave(>vbl_lock, irqflags);
>>   vblank_disable_and_save(dev, crtc);
>>   wake_up(>vblank[crtc].queue);
>> @@ -1078,6 +1092,9 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
>>  {
>>   unsigned long irqflags;
>>
>> + if (WARN_ON(crtc >= dev->num_crtcs))
>> + return;
>> +
>>   spin_lock_irqsave(>vbl_lock, irqflags);
>>   /* re-enable interrupts if there's are users left */
>>   if (atomic_read(>vblank[crtc].refcount) != 0)
>> @@ -1131,6 +1148,10 @@ void drm_vblank_pre_modeset(struct drm_device *dev, 
>> int crtc)
>>   /* vblank is not initialized (IRQ not installed ?), or has been freed 
>> */
>>   if (!dev->num_crtcs)
>>   return;
>> +
>> + if (WARN_ON(crtc >= dev->num_crtcs))
>> + return;
>> +
>>   /*
>>* To avoid all the problems that might happen if interrupts
>>* were enabled/disabled around or between these calls, we just
>> @@ -1439,6 +1460,9 @@ bool drm_handle_vblank(struct drm_device *dev, int 
>> crtc)
>>   if (!dev->num_crtcs)
>>   return false;
>>
>> + if (WARN_ON(crtc >= dev->num_crtcs))
>> + return false;
>> +
>>   /* Need timestamp lock to prevent concurrent execution with
>>* vblank enable/disable, as this would cause inconsistent
>>* or corrupted timestamps and vblank counts.
>> --
>> 1.9.3
>>
>> ___
>> dri-devel mailing list
>> dri-devel at lists.freedesktop.org
>> 

[Bug 82201] [HAWAII] GPU doesn't reclock, poor 3D performance

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=82201

--- Comment #23 from Kai  ---
(In reply to comment #22)
> do you have radeon.dpm=0 in smoe /etc/modprobe.d or somewhere like that file?

No:
# grep -nHr radeon.dpm /etc/*
/etc/default/grub:9:GRUB_CMDLINE_LINUX_DEFAULT="quiet radeon.dpm=1"

And, just out of curiousity, that shouldn't matter with attachment 104101
applied, should it?

(In reply to comment #21)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > Maybe we'll get more useful feedback once more people start testing 
> > > hawaii.
> > 
> > That sounds like I failed to provide something? If you have any request,
> > what I should check, just let me know. Ie. trying a different compiler?
> 
> I didn't mean to imply that.  I can't think of anything else to provide. 
> I'm just thinking maybe someone will notice some small detail that I missed
> or something like that.

Ah, ok. I was more concerned I overlooked something you requested. I'm sure
it'll be resolved eventually.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/5b571254/attachment.html>

[Bug 82253] New: JanusVR Browser rendering misses floors on radeonsi, works on intel

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=82253

  Priority: medium
Bug ID: 82253
  Assignee: dri-devel at lists.freedesktop.org
   Summary: JanusVR Browser rendering misses floors on radeonsi,
works on intel
  Severity: normal
Classification: Unclassified
OS: All
  Reporter: haagch at frickel.club
  Hardware: Other
Status: NEW
   Version: git
 Component: Drivers/Gallium/radeonsi
   Product: Mesa

Application download is here: http://janusvr.com/

I made a short apitrace (26 megabyte): http://haagch.frickel.club/janusvr.trace

On intel Ivy Bridge it renders fine.
On radeonsi the floor is missing.

Using mesa git master with this patch:
https://bugs.freedesktop.org/show_bug.cgi?id=80880#c10 on a HD 7970M.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/77f5502b/attachment.html>

[PATCH] drm: Don't grab an fb reference for the idr

[Rob Clark]

On Wed, Aug 6, 2014 at 10:07 AM, Daniel Vetter  wrote:
> On Wed, Aug 06, 2014 at 09:12:42AM -0400, Rob Clark wrote:
>> On Wed, Aug 6, 2014 at 8:37 AM, Daniel Vetter  wrote:
>> > On Wed, Aug 06, 2014 at 07:11:28AM -0400, Rob Clark wrote:
>> >> On Wed, Aug 6, 2014 at 3:10 AM, Daniel Vetter  
>> >> wrote:
>> >> > The current refcounting scheme is that the fb lookup idr also holds a
>> >> > reference. This works out nicely bacause thus far we've always
>> >> > explicitly cleaned up idr entries for framebuffers:
>> >> > - Userspace fbs get removed in the rmfb ioctl or when the drm file
>> >> >   gets closed.
>> >> > - Kernel fbs (for fbdev emulation) get cleaned up by the driver code
>> >> >   at module unload time.
>> >> >
>> >> > But now i915 also reconstructs the bios fbs for a smooth transition.
>> >> > And that fb is purely transitional and should get removed immmediately
>> >> > once all crtcs stop using it. Of course if the i915 fbdev code decides
>> >> > to reuse it as the main fbdev fb then it shouldn't be cleaned up, but
>> >> > in that case the fbdev code will grab it's own reference.
>> >> >
>> >> > The problem is now that we also want to register that takeover fb in
>> >> > the idr, so that userspace can do a smooth transition (animated maybe
>> >> > even!) itself. But currently we have no one who will clean up the idr
>> >> > reference once that fb isn't useful any more, and so essentially leak
>> >> > it.
>> >>
>> >> ewww..  couldn't you do some scheme on lastclose to check if no more
>> >> crtc's are scanning out that fb, and if not then remove the idr?
>> >
>> > There's no natural point really but when we drop the last reference for
>> > it. Going the weak reference route looked the most natural. And I honestly
>> > expect other drivers to eventually do the same - forcing a modeset on
>> > boot-up is kinda not too pretty, and permanently reserving a big
>> > framebuffer just for the bios doesn't sound good either. This approach
>> > would nicely solve it for everyone.
>>
>> hmm, maybe somebody switched my coffee with decaf, but why isn't
>> lastclose a natural point?
>
> There is no lastclose for the bios ;-)
>
> Let me elaborate on what happens:
>
> 1. BIOS sets up an initial config with a framebuffer in stolen.
>
> 2. i915 takes over and reconstructs all the state, so now we have all the
> crtcs enabled using a framebuffer for all of them which wraps the bios
> allocation.
>
> 2b. (optional) reuse that framebuffer for fbdev.
>
> -> That special bios fb has the following references:
> - 1 reference for each crtc that's using it
> - 1 optional reference if it's reused as the fbdev fb
> - 1 reference for the idr
>
> 3. Userspace takes over, potentially doing a getfb on the current
> (bios-inherited) fb for smooth transition, but then does a modeset to its
> own fb.
>
> -> After this all the we've dropped the crtc references and we also want
> to drop the idr reference (since no one will ever use this framebuffer
> again). But there's simply no good place to do that. Lastclose might only
> happen before we shut down the system again, which is a bit too late.

Well, you could still cleanup your idr reference on current
userspace's lastclose.. that doesn't do much good, I suppose, if
current userspace never exits.  But if first userspace is plymouth or
something like that, you would get cleaned up on the
splash->{x11/wayland} transition..

if that isn't sufficient, then yeah I guess the more fancy weak-ref
stuff needed..

BR,
-R

> Note that the getfb call creates a gem handle for the fb object, so the
> backing storage might survive for a lot longer than the fb.
>
>> ofc if that really doesn't work, the weak-ref thing seems like it
>> would solve it nicely.  But if there were a simple solution that
>> didn't involve making fb refcnting more complex, I guess I would
>> prefer that
>
> Well I didn't come up with anything else really. Plan b would be to add
> hooks after any plane updates and manually check whether that special fb
> has lost all but its idr reference, and if so clean it up. That seems to
> be a lot more fragile and convoluted than converting the idr to a weak
> reference.
>
> Cheers, Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch

[Intel-gfx] [PATCH 14/19] drm: Don't update vblank timestamp when the counter didn't change

[Daniel Vetter]

On Wed, Aug 06, 2014 at 02:49:57PM +0300, ville.syrjala at linux.intel.com 
wrote:
> From: Ville Syrj?l? 
> 
> If we already have a timestamp for the current vblank counter, don't
> update it with a new timestmap. Small errors can creep in between two
> timestamp queries for the same vblank count, which could be confusing to
> userspace when it queries the timestamp for the same vblank sequence
> number twice.
> 
> This problem gets exposed when the vblank disable timer is not used
> (or is set to expire quickly) and thus we can get multiple vblank
> disable<->enable transition during the same frame which would all
> attempt to update the timestamp with the latest estimate.
> 
> Testcase: igt/kms_flip/flip-vs-expired-vblank
> Signed-off-by: Ville Syrj?l? 

Reviewed-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_irq.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index af33df1..0523f5b 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -106,6 +106,9 @@ static void drm_update_vblank_count(struct drm_device 
> *dev, int crtc)
>   DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
> crtc, diff);
>  
> + if (diff == 0)
> + return;
> +
>   /* Reinitialize corresponding vblank timestamp if high-precision query
>* available. Skip this step if query unsupported or failed. Will
>* reinitialize delayed at next vblank interrupt in that case.
> -- 
> 1.8.5.5
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at 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 2/2] drm: Perform cmdline mode parsing during connector initialisation

[Daniel Vetter]

From: Chris Wilson 

i915.ko has a custom fbdev initialisation routine that aims to preserve
the current mode set by the BIOS, unless overruled by the user. The
user's wishes are determined by what, if any, mode is specified on the
command line (via the video= parameter). However, that command line mode
is first parsed by drm_fb_helper_initial_config() which is called after
i915.ko's custom initial_config() as a fallback method. So in order for
us to honour it, we need to move the cmdline parser earlier. If we
perform the connector cmdline parsing as soon as we initialise the
connector, that cmdline mode and forced status is then available even if
the fbdev helper is not compiled in or never called.

We also then expose the cmdline user mode in the connector mode lists.

v2: Rebase after connector->name upheaval.

v3: Adapt mga200 to look for the cmdline mode in the new place. Nicely
simplifies things while at that.

v4: Fix checkpatch.

v5: Select FB_CMDLINE to adapt to the changed fbdev patch.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=73154
Signed-off-by: Chris Wilson  (v2)
Cc: Jesse Barnes 
Cc: Ville Syrj?l? 
Cc: Daniel Vetter 
Reviewed-by: Jesse Barnes  (v2)
Cc: dri-devel at lists.freedesktop.org
Cc: Julia Lemire 
Cc: Dave Airlie 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/Kconfig|  1 +
 drivers/gpu/drm/drm_crtc.c | 55 +
 drivers/gpu/drm/drm_fb_helper.c| 64 ++
 drivers/gpu/drm/drm_modes.c|  1 +
 drivers/gpu/drm/drm_probe_helper.c | 17 +
 drivers/gpu/drm/mgag200/mgag200_mode.c | 21 +++
 include/drm/drm_crtc.h |  1 +
 include/drm/drm_fb_helper.h|  1 -
 8 files changed, 83 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 31894c8c1773..367f5dd23291 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -8,6 +8,7 @@ menuconfig DRM
tristate "Direct Rendering Manager (XFree86 4.1.0 and higher DRI 
support)"
depends on (AGP || AGP=n) && !EMULATED_CMPXCHG && MMU && HAS_DMA
select HDMI
+   select FB_CMDLINE
select I2C
select I2C_ALGOBIT
select DMA_SHARED_BUFFER
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 33ff631c8d23..66d3bfb8d264 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -863,6 +863,59 @@ static void drm_mode_remove(struct drm_connector 
*connector,
 }

 /**
+ * drm_connector_get_cmdline_mode - reads the user's cmdline mode
+ * @connector: connector to quwery
+ * @mode: returned mode
+ *
+ * The kernel supports per-connector configration of its consoles through
+ * use of the video= parameter. This function parses that option and
+ * extracts the user's specified mode (or enable/disable status) for a
+ * particular connector. This is typically only used during the early fbdev
+ * setup.
+ */
+static void drm_connector_get_cmdline_mode(struct drm_connector *connector)
+{
+   struct drm_cmdline_mode *mode = >cmdline_mode;
+   char *option = NULL;
+
+   if (fb_get_options(connector->name, ))
+   return;
+
+   if (!drm_mode_parse_command_line_for_connector(option,
+  connector,
+  mode))
+   return;
+
+   if (mode->force) {
+   const char *s;
+
+   switch (mode->force) {
+   case DRM_FORCE_OFF:
+   s = "OFF";
+   break;
+   case DRM_FORCE_ON_DIGITAL:
+   s = "ON - dig";
+   break;
+   default:
+   case DRM_FORCE_ON:
+   s = "ON";
+   break;
+   }
+
+   DRM_INFO("forcing %s connector %s\n", connector->name, s);
+   connector->force = mode->force;
+   }
+
+   DRM_DEBUG_KMS("cmdline mode for connector %s %dx%d@%dHz%s%s%s\n",
+ connector->name,
+ mode->xres, mode->yres,
+ mode->refresh_specified ? mode->refresh : 60,
+ mode->rb ? " reduced blanking" : "",
+ mode->margins ? " with margins" : "",
+ mode->interlace ?  " interlaced" : "");
+}
+
+/**
  * drm_connector_init - Init a preallocated connector
  * @dev: DRM device
  * @connector: the connector to init
@@ -914,6 +967,8 @@ int drm_connector_init(struct drm_device *dev,
connector->edid_blob_ptr = NULL;
connector->status = connector_status_unknown;

+   drm_connector_get_cmdline_mode(connector);
+
list_add_tail(>head, >mode_config.connector_list);
dev->mode_config.num_connector++;

diff --git a/drivers/gpu/drm/drm_fb_helper.c 

[PATCH 1/2] video/fbdev: Always built-in video= cmdline parsing

[Daniel Vetter]

In drm/i915 we want to get at the video= cmdline modes even when we
don't have fbdev support enabled, so that users can always override
the kernel's initial mode selection.

But that gives us a direct depency upon the parsing code in the fbdev
subsystem. Since it's so little code just extract these 2 functions
and always build them in.

Whiel at it fix the checkpatch fail in this code.

v2: Also move fb_mode_option. Spotted by the kbuild.

v3: Review from Geert:
- Keep the old copyright notice from fb_mem.c, although I have no
idea what exactly applies.
- Only compile this when needed.

Cc: Geert Uytterhoeven 
Cc: Plagniol-Villard 
Cc: Tomi Valkeinen 
Cc: linux-fbdev at vger.kernel.org
Signed-off-by: Daniel Vetter 

--

I prefer if we can merge this through drm-next since we'll use it
there in follow-up patches.
-Daniel
---
 drivers/video/fbdev/Kconfig   |   4 ++
 drivers/video/fbdev/core/Makefile |   1 +
 drivers/video/fbdev/core/fb_cmdline.c | 110 ++
 drivers/video/fbdev/core/fbmem.c  |  92 
 drivers/video/fbdev/core/modedb.c |   3 -
 5 files changed, 115 insertions(+), 95 deletions(-)
 create mode 100644 drivers/video/fbdev/core/fb_cmdline.c

diff --git a/drivers/video/fbdev/Kconfig b/drivers/video/fbdev/Kconfig
index 59c98bfd5a8a..f1458c95a688 100644
--- a/drivers/video/fbdev/Kconfig
+++ b/drivers/video/fbdev/Kconfig
@@ -4,6 +4,7 @@

 menuconfig FB
tristate "Support for frame buffer devices"
+   select FB_CMDLINE
---help---
  The frame buffer device provides an abstraction for the graphics
  hardware. It represents the frame buffer of some video hardware and
@@ -52,6 +53,9 @@ config FIRMWARE_EDID
 combination with certain motherboards and monitors are known to
 suffer from this problem.

+config FB_CMDLINE
+   bool
+
 config FB_DDC
tristate
depends on FB
diff --git a/drivers/video/fbdev/core/Makefile 
b/drivers/video/fbdev/core/Makefile
index fa306538dac2..67f28e20a892 100644
--- a/drivers/video/fbdev/core/Makefile
+++ b/drivers/video/fbdev/core/Makefile
@@ -1,4 +1,5 @@
 obj-y += fb_notify.o
+obj-$(CONFIG_FB_CMDLINE)  += fb_cmdline.o
 obj-$(CONFIG_FB)  += fb.o
 fb-y  := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
  modedb.o fbcvt.o
diff --git a/drivers/video/fbdev/core/fb_cmdline.c 
b/drivers/video/fbdev/core/fb_cmdline.c
new file mode 100644
index ..39509ccd92f1
--- /dev/null
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -0,0 +1,110 @@
+/*
+ *  linux/drivers/video/fb_cmdline.c
+ *
+ *  Copyright (C) 2014 Intel Corp
+ *  Copyright (C) 1994 Martin Schaller
+ *
+ * 2001 - Documented with DocBook
+ * - Brad Douglas 
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive
+ * for more details.
+ *
+ * Authors:
+ *Vetter 
+ */
+#include 
+#include 
+
+static char *video_options[FB_MAX] __read_mostly;
+static int ofonly __read_mostly;
+
+const char *fb_mode_option;
+EXPORT_SYMBOL_GPL(fb_mode_option);
+
+/**
+ * fb_get_options - get kernel boot parameters
+ * @name:   framebuffer name as it would appear in
+ *  the boot parameter line
+ *  (video=:)
+ * @option: the option will be stored here
+ *
+ * NOTE: Needed to maintain backwards compatibility
+ */
+int fb_get_options(const char *name, char **option)
+{
+   char *opt, *options = NULL;
+   int retval = 0;
+   int name_len = strlen(name), i;
+
+   if (name_len && ofonly && strncmp(name, "offb", 4))
+   retval = 1;
+
+   if (name_len && !retval) {
+   for (i = 0; i < FB_MAX; i++) {
+   if (video_options[i] == NULL)
+   continue;
+   if (!video_options[i][0])
+   continue;
+   opt = video_options[i];
+   if (!strncmp(name, opt, name_len) &&
+   opt[name_len] == ':')
+   options = opt + name_len + 1;
+   }
+   }
+   /* No match, pass global option */
+   if (!options && option && fb_mode_option)
+   options = kstrdup(fb_mode_option, GFP_KERNEL);
+   if (options && !strncmp(options, "off", 3))
+   retval = 1;
+
+   if (option)
+   *option = options;
+
+   return retval;
+}
+EXPORT_SYMBOL(fb_get_options);
+
+/**
+ * video_setup - process command line options
+ * @options: string of options
+ *
+ * Process command line options for frame buffer subsystem.
+ *
+ * NOTE: This function is a __setup and __init function.
+ *It only stores the options.  Drivers have to call
+ *fb_get_options() as necessary.
+ *
+ * Returns zero.
+ *

[Intel-gfx] [PATCH 2/2] drm/i915: Free pending page flip events at .preclose()

[Daniel Vetter]

On Wed, Aug 06, 2014 at 02:02:51PM +0300, ville.syrjala at linux.intel.com 
wrote:
> From: Ville Syrj?l? 
> 
> If there are pending page flips when the fd gets closed those page
> flips may have events associated to them. When the page flip eventually
> completes it will queue the event to file_priv->event_list, but that
> may be too late and file_priv->event_list has already been cleaned up.
> Thus we leak a bit of kernel memory in the form of the event structure.
> 
> To avoid such problems clear out such pending events from
> intel_crtc->unpin_work at ->preclose(). Any event that already made it
> to file_priv->event_list will get cleaned up by the drm_release_events()
> a bit later.
> 
> We can ignore the file_priv->event_space accounting since file_priv is
> going away. This is already how drm core deals with pending vblank
> events, which are maintained by the drm core.
> 
> What saves us from a total disaster (ie. dereferencing and alrady
> freed file_priv) is the fact that the fb descruction triggers a modeset
> and there we wait for pending flips.
> 
> Signed-off-by: Ville Syrj?l? 

Do we have an igt for this?
-Daniel

> ---
>  drivers/gpu/drm/i915/i915_dma.c  |  3 +++
>  drivers/gpu/drm/i915/intel_display.c | 22 ++
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 2e7f03a..c965698 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1981,6 +1981,9 @@ void i915_driver_preclose(struct drm_device *dev, 
> struct drm_file *file)
>   i915_gem_context_close(dev, file);
>   i915_gem_release(dev, file);
>   mutex_unlock(>struct_mutex);
> +
> + if (drm_core_check_feature(dev, DRIVER_MODESET))
> + intel_modeset_preclose(dev, file);
>  }
>  
>  void i915_driver_postclose(struct drm_device *dev, struct drm_file *file)
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index 883af0b..4230e4a 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13416,3 +13416,25 @@ intel_display_print_error_state(struct 
> drm_i915_error_state_buf *m,
>   err_printf(m, "  VSYNC: %08x\n", error->transcoder[i].vsync);
>   }
>  }
> +
> +void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
> +{
> + struct intel_crtc *crtc;
> +
> + for_each_intel_crtc(dev, crtc) {
> + struct intel_unpin_work *work;
> + unsigned long irqflags;
> +
> + spin_lock_irqsave(>event_lock, irqflags);
> +
> + work = crtc->unpin_work;
> +
> + if (work && work->event &&
> + work->event->base.file_priv == file) {
> + kfree(work->event);
> + work->event = NULL;
> + }
> +
> + spin_unlock_irqrestore(>event_lock, irqflags);
> + }
> +}
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 28d185d..8f04ba8 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -888,6 +888,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode 
> *mode,
>struct intel_crtc_config *pipe_config);
>  int intel_format_to_fourcc(int format);
>  void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
> +void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>  
>  
>  /* intel_dp.c */
> -- 
> 1.8.5.5
> 
> ___
> Intel-gfx mailing list
> Intel-gfx at 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 igt] tests/kms_flip: Assert that vblank timestamps aren't zeroed

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

The kernel might mistakenly send out a zeroed vblank timestamp when
the vblank wait gets terminated early due to crtc disable. Add an
assertion to catch that.

Signed-off-by: Ville Syrj?l? 
---
 tests/kms_flip.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/tests/kms_flip.c b/tests/kms_flip.c
index 92f4eb5..e8a0b39 100644
--- a/tests/kms_flip.c
+++ b/tests/kms_flip.c
@@ -396,6 +396,9 @@ static int __wait_for_vblank(unsigned int flags, int 
crtc_idx,
ret = drmWaitVBlank(drm_fd, _vbl);

if (ret == 0) {
+   igt_assert_f(wait_vbl.reply.tval_sec != 0 ||
+wait_vbl.reply.tval_usec != 0,
+"zeroed vblank timestamp\n");
reply->ts.tv_sec = wait_vbl.reply.tval_sec;
reply->ts.tv_usec = wait_vbl.reply.tval_usec;
reply->sequence = wait_vbl.reply.sequence;
-- 
1.8.5.5

[PATCH 19/19] drm: Fix confusing debug message in drm_update_vblank_count()

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Now that drm_update_vblank_count() can be called even when we're not
about to enable the vblank interrupts we shouldn't print debug messages
stating otherwise.

Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index e927e5f..87abca3 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -103,7 +103,7 @@ static void drm_update_vblank_count(struct drm_device *dev, 
int crtc)
  crtc, vblank->last, cur_vblank, diff);
}

-   DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
+   DRM_DEBUG("updating vblank count on crtc %d, missed %d\n",
  crtc, diff);

if (diff == 0)
-- 
1.8.5.5

[PATCH 18/19] drm/i915: Update scanline_offset only for active crtcs

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

update_scanline_offset() in intel_sanitize_crtc() was supposed to
be called only for active crtcs. But due to some underrun patches it
now gets updated for all crtcs on gmch platforms.

Move the update_scanline_offset() to the very beginning of
intel_sanitize_crtc() where we update the vblank state. This seems like
a better place anyway since the scanline offset ought to be up to date
before we might need to consult it. So before any vblanky stuff happens.

Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/i915/intel_display.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index c00bcd0..9403b2f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12916,9 +12916,10 @@ static void intel_sanitize_crtc(struct intel_crtc 
*crtc)
I915_WRITE(reg, I915_READ(reg) & ~PIPECONF_FRAME_START_DELAY_MASK);

/* restore vblank interrupts to correct state */
-   if (crtc->active)
+   if (crtc->active) {
+   update_scanline_offset(crtc);
drm_vblank_on(dev, crtc->pipe);
-   else {
+   } else {
/* avoid random jumps in seq/ts */
dev->vblank[crtc->pipe].last = 0;
drm_vblank_off(dev, crtc->pipe);
@@ -13020,8 +13021,6 @@ static void intel_sanitize_crtc(struct intel_crtc *crtc)
 */
crtc->cpu_fifo_underrun_disabled = true;
crtc->pch_fifo_underrun_disabled = true;
-
-   update_scanline_offset(crtc);
}
 }

-- 
1.8.5.5

[PATCH 17/19] drm/i915: Clear .last vblank count before drm_vblank_off() when sanitizing crtc state

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

We call drm_vblank_off() during crtc sanitation to make sure the
software and hardware states agree. However drm_vblank_off() will
try to update the vblank timestamp and sequence number which lands
us in some trouble.

As the pipe is disabled the hardware frame counter query will
return 0. If the .last doesn't match the code will try to add the
difference to the user visible sequence number. During driver init
that's OK as .last == 0, but during resume .last may be anything.
So we should make sure we don't try to apply the diff here by zeroing
.last beforehand. Otherwise there maybe be a random jump in the user
visible sequence number across suspend.

Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/i915/intel_display.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index ae5f20d..c00bcd0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12918,8 +12918,11 @@ static void intel_sanitize_crtc(struct intel_crtc 
*crtc)
/* restore vblank interrupts to correct state */
if (crtc->active)
drm_vblank_on(dev, crtc->pipe);
-   else
+   else {
+   /* avoid random jumps in seq/ts */
+   dev->vblank[crtc->pipe].last = 0;
drm_vblank_off(dev, crtc->pipe);
+   }

/* We need to sanitize the plane -> pipe mapping first because this will
 * disable the crtc (and hence change the state) if it is wrong. Note
-- 
1.8.5.5

[PATCH 16/19] drm: Store the vblank timestamp when adjusting the counter during disable

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

During vblank disable the code tries to guess based on the
timestamps whether we just missed one vblank or not. And if so
it increments the counter. However it forgets to store the new
timestamp to the approriate slot in our timestamp ring buffer.
So anyone querying the timestamp for the resulting sequence
number would get a stale timestamp. Fix it up by storing the
new timestamp.

Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 67507a4..e927e5f 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -203,6 +203,13 @@ static void vblank_disable_and_save(struct drm_device 
*dev, int crtc)
 * hope for the best.
 */
if ((vblrc > 0) && (abs64(diff_ns) > 100)) {
+   /* Store new timestamp in ringbuffer. */
+   vblanktimestamp(dev, crtc, vblcount + 1) = tvblank;
+
+   /* Increment cooked vblank count. This also atomically commits
+* the timestamp computed above.
+*/
+   smp_mb__before_atomic();
atomic_inc(>count);
smp_mb__after_atomic();
}
-- 
1.8.5.5

[PATCH 15/19] drm: Update vblank->last in drm_update_vblank_count()

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

We should update the last in drm_update_vblank_count() to avoid applying
the diff more than once. This could occur eg. if drm_vblank_off() gets
called multiple times for the crtc.

Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 0523f5b..67507a4 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -109,6 +109,8 @@ static void drm_update_vblank_count(struct drm_device *dev, 
int crtc)
if (diff == 0)
return;

+   vblank->last = cur_vblank;
+
/* Reinitialize corresponding vblank timestamp if high-precision query
 * available. Skip this step if query unsupported or failed. Will
 * reinitialize delayed at next vblank interrupt in that case.
-- 
1.8.5.5

[PATCH 14/19] drm: Don't update vblank timestamp when the counter didn't change

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

If we already have a timestamp for the current vblank counter, don't
update it with a new timestmap. Small errors can creep in between two
timestamp queries for the same vblank count, which could be confusing to
userspace when it queries the timestamp for the same vblank sequence
number twice.

This problem gets exposed when the vblank disable timer is not used
(or is set to expire quickly) and thus we can get multiple vblank
disable<->enable transition during the same frame which would all
attempt to update the timestamp with the latest estimate.

Testcase: igt/kms_flip/flip-vs-expired-vblank
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index af33df1..0523f5b 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -106,6 +106,9 @@ static void drm_update_vblank_count(struct drm_device *dev, 
int crtc)
DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
  crtc, diff);

+   if (diff == 0)
+   return;
+
/* Reinitialize corresponding vblank timestamp if high-precision query
 * available. Skip this step if query unsupported or failed. Will
 * reinitialize delayed at next vblank interrupt in that case.
-- 
1.8.5.5

[PATCH v2 13/19] drm: Kick start vblank interrupts at drm_vblank_on()

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

If the user is interested in getting accurate vblank sequence
numbers all the time they may disable the vblank disable timer
entirely. In that case it seems appropriate to kick start the
vblank interrupts already from drm_vblank_on().

v2: Adapt to the drm_vblank_offdelay ==0 vs <0 changes

Reviewed-by: Matt Roper 
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 9 ++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 8dbcc3f..af33df1 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1126,9 +1126,12 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
vblank->last =
(dev->driver->get_vblank_counter(dev, crtc) - 1) &
dev->max_vblank_count;
-
-   /* re-enable interrupts if there's are users left */
-   if (atomic_read(>refcount) != 0)
+   /*
+* re-enable interrupts if there are users left, or the
+* user wishes vblank interrupts to be enabled all the time.
+*/
+   if (atomic_read(>refcount) != 0 ||
+   (!dev->vblank_disable_immediate && drm_vblank_offdelay == 0))
WARN_ON(drm_vblank_enable(dev, crtc));
spin_unlock_irqrestore(>vbl_lock, irqflags);
 }
-- 
1.8.5.5

[PATCH 12/19] drm/i915: Opt out of vblank disable timer on >gen2

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Now that the vblank races are plugged, we can opt out of using
the vblank disable timer and just let vblank interrupts get
disabled immediately when the last reference is dropped.

Gen2 is the exception since it has no hardware frame counter.

Reviewed-by: Matt Roper 
Reviewed-by: Daniel Vetter 
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/i915/i915_irq.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 87abe86..9fdf738 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -4682,6 +4682,14 @@ void intel_irq_init(struct drm_device *dev)
dev->max_vblank_count = 0xff; /* only 24 bits of frame 
count */
}

+   /*
+* Opt out of the vblank disable timer on everything except gen2.
+* Gen2 doesn't have a hardware frame counter and so depends on
+* vblank interrupts to produce sane vblank seuquence numbers.
+*/
+   if (!IS_GEN2(dev))
+   dev->vblank_disable_immediate = true;
+
if (drm_core_check_feature(dev, DRIVER_MODESET)) {
dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp;
dev->driver->get_scanout_position = i915_get_crtc_scanoutpos;
-- 
1.8.5.5

[PATCH v2 11/19] drm: Add dev->vblank_disable_immediate flag

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Add a flag to drm_device which will cause the vblank code to bypass the
disable timer and always disable the vblank interrupt immediately when
the last reference is dropped.

v2: Add some notes about the flag to the kernel doc

Reviewed-by: Matt Roper 
Reviewed-by: Daniel Vetter 
Signed-off-by: Ville Syrj?l? 
---
 Documentation/DocBook/drm.tmpl |  6 ++
 drivers/gpu/drm/drm_irq.c  |  2 +-
 include/drm/drmP.h | 10 ++
 3 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 1cbe2a0..3ec4b2b 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -3387,6 +3387,12 @@ void (*disable_vblank) (struct drm_device *dev, int 
crtc);
   module parameter or the drm_vblank_offdelay global
   variable and expressed in milliseconds. Its default value is 5000 ms.
   Zero means never disable, and a negative value means disable immediately.
+  Drivers may override the behaviour by setting the
+  drm_device
+  vblank_disable_immediate flag, which when set
+  causes vblank interrupts to be disabled immediately regardless of the
+  drm_vblank_offdelay value. The flag should only be set if there's a
+  properly working hardware vblank counter present.
 
 
   When a vertical blanking interrupt occurs drivers only need to call the
diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 99145c4..8dbcc3f 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -994,7 +994,7 @@ void drm_vblank_put(struct drm_device *dev, int crtc)

/* Last user schedules interrupt disable */
if (atomic_dec_and_test(>refcount)) {
-   if (drm_vblank_offdelay < 0)
+   if (dev->vblank_disable_immediate || drm_vblank_offdelay < 0)
vblank_disable_fn((unsigned long)vblank);
else if (drm_vblank_offdelay > 0)
mod_timer(>disable_timer,
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index cdcc60b..3c22c35 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1077,6 +1077,16 @@ struct drm_device {
 */
bool vblank_disable_allowed;

+   /*
+* If true, vblank interrupt will be disabled immediately when the
+* refcount drops to zero, as opposed to via the vblank disable
+* timer.
+* This can be set to true it the hardware has a working vblank
+* counter and the driver uses drm_vblank_on() and drm_vblank_off()
+* appropriately.
+*/
+   bool vblank_disable_immediate;
+
/* array of size num_crtcs */
struct drm_vblank_crtc *vblank;

-- 
1.8.5.5

[PATCH v2 10/19] drm: Disable vblank interrupt immediately when drm_vblank_offdelay<0

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Make drm_vblank_put() disable the vblank interrupt immediately when the
refcount drops to zero and drm_vblank_offdelay<0.

v2: Preserve the current drm_vblank_offdelay==0 'never disable' behaviur

Reviewed-by: Matt Roper 
Signed-off-by: Ville Syrj?l? 
---
 Documentation/DocBook/drm.tmpl |  1 +
 drivers/gpu/drm/drm_drv.c  |  4 ++--
 drivers/gpu/drm/drm_irq.c  | 11 +++
 include/drm/drmP.h |  2 +-
 4 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 9727594..1cbe2a0 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -3386,6 +3386,7 @@ void (*disable_vblank) (struct drm_device *dev, int 
crtc);
   by scheduling a timer. The delay is accessible through the vblankoffdelay
   module parameter or the drm_vblank_offdelay global
   variable and expressed in milliseconds. Its default value is 5000 ms.
+  Zero means never disable, and a negative value means disable immediately.
 
 
   When a vertical blanking interrupt occurs drivers only need to call the
diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
index 92bc6b1..db03e16 100644
--- a/drivers/gpu/drm/drm_drv.c
+++ b/drivers/gpu/drm/drm_drv.c
@@ -39,7 +39,7 @@
 unsigned int drm_debug = 0;/* 1 to enable debug output */
 EXPORT_SYMBOL(drm_debug);

-unsigned int drm_vblank_offdelay = 5000;/* Default to 5000 msecs. */
+int drm_vblank_offdelay = 5000;/* Default to 5000 msecs. */

 unsigned int drm_timestamp_precision = 20;  /* Default to 20 usecs. */

@@ -53,7 +53,7 @@ MODULE_AUTHOR(CORE_AUTHOR);
 MODULE_DESCRIPTION(CORE_DESC);
 MODULE_LICENSE("GPL and additional rights");
 MODULE_PARM_DESC(debug, "Enable debug output");
-MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable 
[msecs]");
+MODULE_PARM_DESC(vblankoffdelay, "Delay until vblank irq auto-disable [msecs] 
(0: never disable, <0: disable immediately)");
 MODULE_PARM_DESC(timestamp_precision_usec, "Max. error on timestamps [usecs]");
 MODULE_PARM_DESC(timestamp_monotonic, "Use monotonic timestamps");

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index b2428cb..99145c4 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -993,10 +993,13 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
BUG_ON(atomic_read(>refcount) == 0);

/* Last user schedules interrupt disable */
-   if (atomic_dec_and_test(>refcount) &&
-   (drm_vblank_offdelay > 0))
-   mod_timer(>disable_timer,
- jiffies + ((drm_vblank_offdelay * HZ)/1000));
+   if (atomic_dec_and_test(>refcount)) {
+   if (drm_vblank_offdelay < 0)
+   vblank_disable_fn((unsigned long)vblank);
+   else if (drm_vblank_offdelay > 0)
+   mod_timer(>disable_timer,
+ jiffies + ((drm_vblank_offdelay * HZ)/1000));
+   }
 }
 EXPORT_SYMBOL(drm_vblank_put);

diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index 895e166..cdcc60b 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -1359,7 +1359,7 @@ extern void drm_put_dev(struct drm_device *dev);
 extern void drm_unplug_dev(struct drm_device *dev);
 extern unsigned int drm_debug;

-extern unsigned int drm_vblank_offdelay;
+extern int drm_vblank_offdelay;
 extern unsigned int drm_timestamp_precision;
 extern unsigned int drm_timestamp_monotonic;

-- 
1.8.5.5

[PATCH 09/19] drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Currently it's possible that the following will happen:
1. drm_wait_vblank() calls drm_vblank_get()
2. drm_vblank_off() gets called
3. drm_wait_vblank() calls drm_queue_vblank_event() which
   adds the event to the queue event though vblank interrupts
   are currently disabled (and may not be re-enabled ever again).

To fix the problem, add another vblank->enabled check into
drm_queue_vblank_event().

drm_vblank_off() holds event_lock around the vblank disable,
so no further locking needs to be added to drm_queue_vblank_event().
vblank disable from another source is not possible since
drm_wait_vblank() already holds a vblank reference.

Reviewed-by: Matt Roper 
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 9353609..b2428cb 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1270,6 +1270,7 @@ static int drm_queue_vblank_event(struct drm_device *dev, 
int pipe,
  union drm_wait_vblank *vblwait,
  struct drm_file *file_priv)
 {
+   struct drm_vblank_crtc *vblank = >vblank[pipe];
struct drm_pending_vblank_event *e;
struct timeval now;
unsigned long flags;
@@ -1293,6 +1294,18 @@ static int drm_queue_vblank_event(struct drm_device 
*dev, int pipe,

spin_lock_irqsave(>event_lock, flags);

+   /*
+* drm_vblank_off() might have been called after we called
+* drm_vblank_get(). drm_vblank_off() holds event_lock
+* around the vblank disable, so no need for further locking.
+* The reference from drm_vblank_get() protects against
+* vblank disable from another source.
+*/
+   if (!vblank->enabled) {
+   ret = -EINVAL;
+   goto err_unlock;
+   }
+
if (file_priv->event_space < sizeof e->event) {
ret = -EBUSY;
goto err_unlock;
-- 
1.8.5.5

[PATCH 08/19] drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Currently both drm_irq.c and several drivers call drm_vblank_put()
while holding event_lock. Now that drm_vblank_put() can disable the
vblank interrupt directly it may need to grab vbl_lock and
vblank_time_lock. That causes deadlocks since we take the locks
in the opposite order in two places in drm_irq.c. So let's make
sure the locking order is always event_lock->vbl_lock->vblank_time_lock.

In drm_vblank_off() pull up event_lock from underneath vbl_lock. Hold
the event_lock across the whole operation to make sure we only send
out the events that were on the queue when we disabled the interrupt,
and not ones that got added just after (assuming drm_vblank_on() already
managed to get called somewhere between).

To sort the other deadlock pull the event_lock out from
drm_handle_vblank_events() into drm_handle_vblank() to be taken outside
vblank_time_lock. Add the appropriate assert_spin_locked() to
drm_handle_vblank_events().

Reviewed-by: Matt Roper 
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 47 +--
 1 file changed, 25 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index b4460bf..9353609 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1037,14 +1037,25 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
unsigned long irqflags;
unsigned int seq;

-   spin_lock_irqsave(>vbl_lock, irqflags);
+   spin_lock_irqsave(>event_lock, irqflags);
+
+   spin_lock(>vbl_lock);
vblank_disable_and_save(dev, crtc);
wake_up(>queue);

+   /*
+* Prevent subsequent drm_vblank_get() from re-enabling
+* the vblank interrupt by bumping the refcount.
+*/
+   if (!vblank->inmodeset) {
+   atomic_inc(>refcount);
+   vblank->inmodeset = 1;
+   }
+   spin_unlock(>vbl_lock);
+
/* Send any queued vblank events, lest the natives grow disquiet */
seq = drm_vblank_count_and_time(dev, crtc, );

-   spin_lock(>event_lock);
list_for_each_entry_safe(e, t, >vblank_event_list, base.link) {
if (e->pipe != crtc)
continue;
@@ -1055,18 +1066,7 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
drm_vblank_put(dev, e->pipe);
send_vblank_event(dev, e, seq, );
}
-   spin_unlock(>event_lock);
-
-   /*
-* Prevent subsequent drm_vblank_get() from re-enabling
-* the vblank interrupt by bumping the refcount.
-*/
-   if (!vblank->inmodeset) {
-   atomic_inc(>refcount);
-   vblank->inmodeset = 1;
-   }
-
-   spin_unlock_irqrestore(>vbl_lock, irqflags);
+   spin_unlock_irqrestore(>event_lock, irqflags);
 }
 EXPORT_SYMBOL(drm_vblank_off);

@@ -1446,12 +1446,11 @@ static void drm_handle_vblank_events(struct drm_device 
*dev, int crtc)
 {
struct drm_pending_vblank_event *e, *t;
struct timeval now;
-   unsigned long flags;
unsigned int seq;

-   seq = drm_vblank_count_and_time(dev, crtc, );
+   assert_spin_locked(>event_lock);

-   spin_lock_irqsave(>event_lock, flags);
+   seq = drm_vblank_count_and_time(dev, crtc, );

list_for_each_entry_safe(e, t, >vblank_event_list, base.link) {
if (e->pipe != crtc)
@@ -1467,8 +1466,6 @@ static void drm_handle_vblank_events(struct drm_device 
*dev, int crtc)
send_vblank_event(dev, e, seq, );
}

-   spin_unlock_irqrestore(>event_lock, flags);
-
trace_drm_vblank_event(crtc, seq);
 }

@@ -1491,15 +1488,18 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
if (!dev->num_crtcs)
return false;

+   spin_lock_irqsave(>event_lock, irqflags);
+
/* Need timestamp lock to prevent concurrent execution with
 * vblank enable/disable, as this would cause inconsistent
 * or corrupted timestamps and vblank counts.
 */
-   spin_lock_irqsave(>vblank_time_lock, irqflags);
+   spin_lock(>vblank_time_lock);

/* Vblank irq handling disabled. Nothing to do. */
if (!vblank->enabled) {
-   spin_unlock_irqrestore(>vblank_time_lock, irqflags);
+   spin_unlock(>vblank_time_lock);
+   spin_unlock_irqrestore(>event_lock, irqflags);
return false;
}

@@ -1539,10 +1539,13 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
  crtc, (int) diff_ns);
}

+   spin_unlock(>vblank_time_lock);
+
wake_up(>queue);
drm_handle_vblank_events(dev, crtc);

-   spin_unlock_irqrestore(>vblank_time_lock, irqflags);
+   spin_unlock_irqrestore(>event_lock, irqflags);
+
return true;
 }
 EXPORT_SYMBOL(drm_handle_vblank);
-- 
1.8.5.5

[PATCH v2 07/19] drm: Reduce the amount of dev->vblank[crtc] in the code

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Declare a local struct drm_vblank_crtc * and use that
instead of having to do dig it out via 'dev->vblank[crtc]'
everywhere.

Performed with the following coccinelle incantation,
and a few manual whitespace cleanups:

@@
identifier func,member;
expression num_crtcs;
struct drm_device *dev;
unsigned int crtc;
@@
func (...) {
+ struct drm_vblank_crtc *vblank;
...
if (crtc >= num_crtcs)
   return ...;
+ vblank = >vblank[crtc];
<+...
(
- dev->vblank[crtc].member
+ vblank->member
|
- &(dev->vblank[crtc])
+ vblank
)
...+>
}

@@
struct drm_device *dev;
int crtc;
identifier member;
expression num_crtcs;
@@
for (crtc = 0; crtc < num_crtcs; crtc++) {
+ struct drm_vblank_crtc *vblank = >vblank[crtc];
+
<+...
(
- dev->vblank[crtc].member
+ vblank->member
|
- &(dev->vblank[crtc])
+ vblank
)
...+>
}

@@
identifier func,member;
@@
func (struct drm_device *dev, int crtc, ...) {
+ struct drm_vblank_crtc *vblank = >vblank[crtc];
<+...
(
- dev->vblank[crtc].member
+ vblank->member
|
- &(dev->vblank[crtc])
+ vblank
)
...+>
}

v2: Rebased

Reviewed-by: Matt Roper 
Reviewed-by: Daniel Vetter 
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 134 +++---
 1 file changed, 79 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index fc1525a..b4460bf 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -73,6 +73,7 @@
  */
 static void drm_update_vblank_count(struct drm_device *dev, int crtc)
 {
+   struct drm_vblank_crtc *vblank = >vblank[crtc];
u32 cur_vblank, diff, tslot, rc;
struct timeval t_vblank;

@@ -94,12 +95,12 @@ static void drm_update_vblank_count(struct drm_device *dev, 
int crtc)
} while (cur_vblank != dev->driver->get_vblank_counter(dev, crtc));

/* Deal with counter wrap */
-   diff = cur_vblank - dev->vblank[crtc].last;
-   if (cur_vblank < dev->vblank[crtc].last) {
+   diff = cur_vblank - vblank->last;
+   if (cur_vblank < vblank->last) {
diff += dev->max_vblank_count;

DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => 
diff=0x%x\n",
- crtc, dev->vblank[crtc].last, cur_vblank, diff);
+ crtc, vblank->last, cur_vblank, diff);
}

DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
@@ -110,12 +111,12 @@ static void drm_update_vblank_count(struct drm_device 
*dev, int crtc)
 * reinitialize delayed at next vblank interrupt in that case.
 */
if (rc) {
-   tslot = atomic_read(>vblank[crtc].count) + diff;
+   tslot = atomic_read(>count) + diff;
vblanktimestamp(dev, crtc, tslot) = t_vblank;
}

smp_mb__before_atomic();
-   atomic_add(diff, >vblank[crtc].count);
+   atomic_add(diff, >count);
smp_mb__after_atomic();
 }

@@ -127,6 +128,7 @@ static void drm_update_vblank_count(struct drm_device *dev, 
int crtc)
  */
 static void vblank_disable_and_save(struct drm_device *dev, int crtc)
 {
+   struct drm_vblank_crtc *vblank = >vblank[crtc];
unsigned long irqflags;
u32 vblcount;
s64 diff_ns;
@@ -147,14 +149,14 @@ static void vblank_disable_and_save(struct drm_device 
*dev, int crtc)
 * count account for the entire time between drm_vblank_on() and
 * drm_vblank_off().
 */
-   if (!dev->vblank[crtc].enabled) {
+   if (!vblank->enabled) {
drm_update_vblank_count(dev, crtc);
spin_unlock_irqrestore(>vblank_time_lock, irqflags);
return;
}

dev->driver->disable_vblank(dev, crtc);
-   dev->vblank[crtc].enabled = false;
+   vblank->enabled = false;

/* No further vblank irq's will be processed after
 * this point. Get current hardware vblank count and
@@ -169,9 +171,9 @@ static void vblank_disable_and_save(struct drm_device *dev, 
int crtc)
 * delayed gpu counter increment.
 */
do {
-   dev->vblank[crtc].last = dev->driver->get_vblank_counter(dev, 
crtc);
+   vblank->last = dev->driver->get_vblank_counter(dev, crtc);
vblrc = drm_get_last_vbltimestamp(dev, crtc, , 0);
-   } while (dev->vblank[crtc].last != dev->driver->get_vblank_counter(dev, 
crtc) && (--count) && vblrc);
+   } while (vblank->last != dev->driver->get_vblank_counter(dev, crtc) && 
(--count) && vblrc);

if (!count)
vblrc = 0;
@@ -179,7 +181,7 @@ static void vblank_disable_and_save(struct drm_device *dev, 
int crtc)
/* Compute time difference to stored timestamp of last vblank
 * as updated by last invocation of drm_handle_vblank() in vblank irq.
 */
-   vblcount = atomic_read(>vblank[crtc].count);
+   vblcount = atomic_read(>count);
diff_ns = timeval_to_ns() -
  

[PATCH 06/19] drm: Avoid random vblank counter jumps if the hardware counter has been reset

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

When drm_vblank_on() is called the hardware vblank counter may have
been reset, so we can't trust that the old values sampled prior to
drm_vblank_off() have anything to do with the new values.

So update the .last count in drm_vblank_on() to make the first
drm_vblank_enable() consider that as the reference point. This
will correct the user space visible counter to account for the
time between drm_vblank_on() and the first drm_vblank_enable()
calls.

For extra safety subtract one from the .last count in drm_vblank_on()
to make sure that user space will never see the same counter value
before and after modeset.

Reviewed-by: Matt Roper 
Reviewed-by: Daniel Vetter 
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 1f86f6c..fc1525a 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1095,6 +1095,18 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
atomic_dec(>vblank[crtc].refcount);
dev->vblank[crtc].inmodeset = 0;
}
+
+   /*
+* sample the current counter to avoid random jumps
+* when drm_vblank_enable() applies the diff
+*
+* -1 to make sure user will never see the same
+* vblank counter value before and after a modeset
+*/
+   dev->vblank[crtc].last =
+   (dev->driver->get_vblank_counter(dev, crtc) - 1) &
+   dev->max_vblank_count;
+
/* re-enable interrupts if there's are users left */
if (atomic_read(>vblank[crtc].refcount) != 0)
WARN_ON(drm_vblank_enable(dev, crtc));
-- 
1.8.5.5

[PATCH 05/19] drm: Have the vblank counter account for the time between vblank irq disable and drm_vblank_off()

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

If the vblank irq has already been disabled (via the disable timer) when
we call drm_vblank_off() sample the counter and timestamp one last time.
This will make the sure that the user space visible counter will account
for time between vblank irq disable and drm_vblank_off().

Reviewed-by: Matt Roper 
Reviewed-by: Daniel Vetter 
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 13 +
 1 file changed, 13 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index af96517..1f86f6c 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -140,6 +140,19 @@ static void vblank_disable_and_save(struct drm_device 
*dev, int crtc)
 */
spin_lock_irqsave(>vblank_time_lock, irqflags);

+   /*
+* If the vblank interrupt was already disbled update the count
+* and timestamp to maintain the appearance that the counter
+* has been ticking all along until this time. This makes the
+* count account for the entire time between drm_vblank_on() and
+* drm_vblank_off().
+*/
+   if (!dev->vblank[crtc].enabled) {
+   drm_update_vblank_count(dev, crtc);
+   spin_unlock_irqrestore(>vblank_time_lock, irqflags);
+   return;
+   }
+
dev->driver->disable_vblank(dev, crtc);
dev->vblank[crtc].enabled = false;

-- 
1.8.5.5

[PATCH 04/19] drm: Move drm_update_vblank_count()

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Move drm_update_vblank_count() to avoid forward a declaration.
No functional change.

Reviewed-by: Matt Roper 
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 128 +++---
 1 file changed, 64 insertions(+), 64 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 65d2da9..af96517 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -55,6 +55,70 @@
  */
 #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 100

+/**
+ * drm_update_vblank_count - update the master vblank counter
+ * @dev: DRM device
+ * @crtc: counter to update
+ *
+ * Call back into the driver to update the appropriate vblank counter
+ * (specified by @crtc).  Deal with wraparound, if it occurred, and
+ * update the last read value so we can deal with wraparound on the next
+ * call if necessary.
+ *
+ * Only necessary when going from off->on, to account for frames we
+ * didn't get an interrupt for.
+ *
+ * Note: caller must hold dev->vbl_lock since this reads & writes
+ * device vblank fields.
+ */
+static void drm_update_vblank_count(struct drm_device *dev, int crtc)
+{
+   u32 cur_vblank, diff, tslot, rc;
+   struct timeval t_vblank;
+
+   /*
+* Interrupts were disabled prior to this call, so deal with counter
+* wrap if needed.
+* NOTE!  It's possible we lost a full dev->max_vblank_count events
+* here if the register is small or we had vblank interrupts off for
+* a long time.
+*
+* We repeat the hardware vblank counter & timestamp query until
+* we get consistent results. This to prevent races between gpu
+* updating its hardware counter while we are retrieving the
+* corresponding vblank timestamp.
+*/
+   do {
+   cur_vblank = dev->driver->get_vblank_counter(dev, crtc);
+   rc = drm_get_last_vbltimestamp(dev, crtc, _vblank, 0);
+   } while (cur_vblank != dev->driver->get_vblank_counter(dev, crtc));
+
+   /* Deal with counter wrap */
+   diff = cur_vblank - dev->vblank[crtc].last;
+   if (cur_vblank < dev->vblank[crtc].last) {
+   diff += dev->max_vblank_count;
+
+   DRM_DEBUG("last_vblank[%d]=0x%x, cur_vblank=0x%x => 
diff=0x%x\n",
+ crtc, dev->vblank[crtc].last, cur_vblank, diff);
+   }
+
+   DRM_DEBUG("enabling vblank interrupts on crtc %d, missed %d\n",
+ crtc, diff);
+
+   /* Reinitialize corresponding vblank timestamp if high-precision query
+* available. Skip this step if query unsupported or failed. Will
+* reinitialize delayed at next vblank interrupt in that case.
+*/
+   if (rc) {
+   tslot = atomic_read(>vblank[crtc].count) + diff;
+   vblanktimestamp(dev, crtc, tslot) = t_vblank;
+   }
+
+   smp_mb__before_atomic();
+   atomic_add(diff, >vblank[crtc].count);
+   smp_mb__after_atomic();
+}
+
 /*
  * Disable vblank irq's on crtc, make sure that last vblank count
  * of hardware and corresponding consistent software vblank counter
@@ -799,70 +863,6 @@ void drm_send_vblank_event(struct drm_device *dev, int 
crtc,
 EXPORT_SYMBOL(drm_send_vblank_event);

 /**
- * drm_update_vblank_count - update the master vblank counter
- * @dev: DRM device
- * @crtc: counter to update
- *
- * Call back into the driver to update the appropriate vblank counter
- * (specified by @crtc).  Deal with wraparound, if it occurred, and
- * update the last read value so we can deal with wraparound on the next
- * call if necessary.
- *
- * Only necessary when going from off->on, to account for frames we
- * didn't get an interrupt for.
- *
- * Note: caller must hold dev->vbl_lock since this reads & writes
- * device vblank fields.
- */
-static void drm_update_vblank_count(struct drm_device *dev, int crtc)
-{
-   u32 cur_vblank, diff, tslot, rc;
-   struct timeval t_vblank;
-
-   /*
-* Interrupts were disabled prior to this call, so deal with counter
-* wrap if needed.
-* NOTE!  It's possible we lost a full dev->max_vblank_count events
-* here if the register is small or we had vblank interrupts off for
-* a long time.
-*
-* We repeat the hardware vblank counter & timestamp query until
-* we get consistent results. This to prevent races between gpu
-* updating its hardware counter while we are retrieving the
-* corresponding vblank timestamp.
-*/
-   do {
-   cur_vblank = dev->driver->get_vblank_counter(dev, crtc);
-   rc = drm_get_last_vbltimestamp(dev, crtc, _vblank, 0);
-   } while (cur_vblank != dev->driver->get_vblank_counter(dev, crtc));
-
-   /* Deal with counter wrap */
-   diff = cur_vblank - dev->vblank[crtc].last;
-   if (cur_vblank < dev->vblank[crtc].last) {
- 

[PATCH 03/19] drm: Don't clear vblank timestamps when vblank interrupt is disabled

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Clearing the timestamps causes us to send zeroed timestamps to userspace
if they get sent out in response to the drm_vblank_off(). It's better
to send the very latest timestamp and count instead.

Testcase: igt/kms_flip/modeset-vs-vblank-race
Reviewed-by: Matt Roper 
Reviewed-by: Daniel Vetter 
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 11 ---
 1 file changed, 11 deletions(-)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index b16a636..65d2da9 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -56,14 +56,6 @@
 #define DRM_REDUNDANT_VBLIRQ_THRESH_NS 100

 /*
- * Clear vblank timestamp buffer for a crtc.
- */
-static void clear_vblank_timestamps(struct drm_device *dev, int crtc)
-{
-   memset(dev->vblank[crtc].time, 0, sizeof(dev->vblank[crtc].time));
-}
-
-/*
  * Disable vblank irq's on crtc, make sure that last vblank count
  * of hardware and corresponding consistent software vblank counter
  * are preserved, even if there are any spurious vblank irq's after
@@ -131,9 +123,6 @@ static void vblank_disable_and_save(struct drm_device *dev, 
int crtc)
smp_mb__after_atomic();
}

-   /* Invalidate all timestamps while vblank irq's are off. */
-   clear_vblank_timestamps(dev, crtc);
-
spin_unlock_irqrestore(>vblank_time_lock, irqflags);
 }

-- 
1.8.5.5

[PATCH v2 02/19] drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

v2: Drop the drm_vblank_off() (Daniel)
Use drm_crtc_vblank_{get,put}()

Reviewed-by: Matt Roper 
Reviewed-by: Daniel Vetter 
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/i915/intel_display.c | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 1b4cb0b..ae5f20d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1342,6 +1342,12 @@ static void assert_sprites_disabled(struct 
drm_i915_private *dev_priv,
}
 }

+static void assert_vblank_disabled(struct drm_crtc *crtc)
+{
+   if (WARN_ON(drm_crtc_vblank_get(crtc) == 0))
+   drm_crtc_vblank_put(crtc);
+}
+
 static void ibx_assert_pch_refclk_enabled(struct drm_i915_private *dev_priv)
 {
u32 val;
@@ -3909,6 +3915,8 @@ static void intel_crtc_enable_planes(struct drm_crtc 
*crtc)
int pipe = intel_crtc->pipe;
int plane = intel_crtc->plane;

+   assert_vblank_disabled(crtc);
+
drm_vblank_on(dev, pipe);

intel_enable_primary_hw_plane(dev_priv, plane, pipe);
@@ -3958,6 +3966,8 @@ static void intel_crtc_disable_planes(struct drm_crtc 
*crtc)
intel_frontbuffer_flip(dev, INTEL_FRONTBUFFER_ALL_MASK(pipe));

drm_vblank_off(dev, pipe);
+
+   assert_vblank_disabled(crtc);
 }

 static void ironlake_crtc_enable(struct drm_crtc *crtc)
-- 
1.8.5.5

[PATCH v2 01/19] drm: Always reject drm_vblank_get() after drm_vblank_off()

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Make sure drm_vblank_get() never succeeds when called between
drm_vblank_off() and drm_vblank_on(). Borrow a trick from the
old drm_vblank_{pre,post}_modeset() functions and just bump
the refcount in drm_vblank_off() and drop it in drm_vblank_on().

When drm_vblank_get() encounters a >0 refcount and the vblank
interrupt is already disabled it will simply return -EINVAL.

Hopefully the use of inmodeset won't conflict badly with
drm_vblank_{pre,post}_modeset().

For i915 there's a window between drm_vblank_off() and marking the
crtc as inactive where the current code still allows drm_vblank_get().

v2: Describe what drm_vblank_get() does to explain how
a simple refcount bump manages to fix things (Daniel)

Reviewed-by: Matt Roper 
Reviewed-by: Daniel Vetter 
Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_irq.c | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 0de123a..b16a636 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -1039,6 +1039,15 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
}
spin_unlock(>event_lock);

+   /*
+* Prevent subsequent drm_vblank_get() from re-enabling
+* the vblank interrupt by bumping the refcount.
+*/
+   if (!dev->vblank[crtc].inmodeset) {
+   atomic_inc(>vblank[crtc].refcount);
+   dev->vblank[crtc].inmodeset = 1;
+   }
+
spin_unlock_irqrestore(>vbl_lock, irqflags);
 }
 EXPORT_SYMBOL(drm_vblank_off);
@@ -1079,6 +1088,11 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
unsigned long irqflags;

spin_lock_irqsave(>vbl_lock, irqflags);
+   /* Drop our private "prevent drm_vblank_get" refcount */
+   if (dev->vblank[crtc].inmodeset) {
+   atomic_dec(>vblank[crtc].refcount);
+   dev->vblank[crtc].inmodeset = 0;
+   }
/* re-enable interrupts if there's are users left */
if (atomic_read(>vblank[crtc].refcount) != 0)
WARN_ON(drm_vblank_enable(dev, crtc));
-- 
1.8.5.5

[PATCH v2 00/19] drm: More vblank on/off work

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

This is mostly a repost of the earlier series [1]. Most of the patches
have been reviewed, but I have added quite a few new ones to the end to
fix various issues.

[1] http://lists.freedesktop.org/archives/dri-devel/2014-May/060518.html

Ville Syrj?l? (19):
  drm: Always reject drm_vblank_get() after drm_vblank_off()
  drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()
  drm: Don't clear vblank timestamps when vblank interrupt is disabled
  drm: Move drm_update_vblank_count()
  drm: Have the vblank counter account for the time between vblank irq
disable and drm_vblank_off()
  drm: Avoid random vblank counter jumps if the hardware counter has
been reset
  drm: Reduce the amount of dev->vblank[crtc] in the code
  drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock
  drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()
  drm: Disable vblank interrupt immediately when drm_vblank_offdelay<0
  drm: Add dev->vblank_disable_immediate flag
  drm/i915: Opt out of vblank disable timer on >gen2
  drm: Kick start vblank interrupts at drm_vblank_on()
  drm: Don't update vblank timestamp when the counter didn't change
  drm: Update vblank->last in drm_update_vblank_count()
  drm: Store the vblank timestamp when adjusting the counter during
disable
  drm/i915: Clear .last vblank count before drm_vblank_off() when
sanitizing crtc state
  drm/i915: Update scanline_offset only for active crtcs
  drm: Fix confusing debug message in drm_update_vblank_count()

 Documentation/DocBook/drm.tmpl   |   7 +
 drivers/gpu/drm/drm_drv.c|   4 +-
 drivers/gpu/drm/drm_irq.c| 344 ++-
 drivers/gpu/drm/i915/i915_irq.c  |   8 +
 drivers/gpu/drm/i915/intel_display.c |  20 +-
 include/drm/drmP.h   |  12 +-
 6 files changed, 259 insertions(+), 136 deletions(-)

-- 
1.8.5.5

[PATCH] video/fbdev: Always built-in video= cmdline parsing

[Daniel Vetter]

On Wed, Aug 06, 2014 at 12:27:32PM +0200, Geert Uytterhoeven wrote:
> On Wed, Aug 6, 2014 at 11:43 AM, Daniel Vetter  
> wrote:
> > In drm/i915 we want to get at the video= cmdline modes even when we
> > don't have fbdev support enabled, so that users can always override
> > the kernel's initial mode selection.
> >
> > But that gives us a direct depency upon the parsing code in the fbdev
> > subsystem. Since it's so little code just extract these 2 functions
> > and always build them in.
> 
> How much is "so little"? Think memory-constrained systems.
> 
> You can still build it depending on CONFIG_FB or CONFIG_DRM_I915.

I'll do it as an option selected by FB and DRM then.
> 
> > diff --git a/drivers/video/fbdev/core/Makefile 
> > b/drivers/video/fbdev/core/Makefile
> > index fa306538dac2..891c1f890e03 100644
> > --- a/drivers/video/fbdev/core/Makefile
> > +++ b/drivers/video/fbdev/core/Makefile
> > @@ -1,4 +1,4 @@
> > -obj-y += fb_notify.o
> 
> Oh, this is already unconditional. Who are its users?

Welcome in fbdev-land. Not going to fix this, since my dragon-slaying
sword is already broken.

> 
> > +obj-y += fb_notify.o fb_cmdline.o
> >  obj-$(CONFIG_FB)  += fb.o
> >  fb-y  := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
> >   modedb.o fbcvt.o
> > diff --git a/drivers/video/fbdev/core/fb_cmdline.c 
> > b/drivers/video/fbdev/core/fb_cmdline.c
> > new file mode 100644
> > index ..91503a43213e
> > --- /dev/null
> > +++ b/drivers/video/fbdev/core/fb_cmdline.c
> > @@ -0,0 +1,103 @@
> > +/*
> > + *  linux/drivers/video/fb_cmdline.c
> > + *
> > + *  Copyright (C) 2014 Intel Corp
> > + *
> > + * This file is subject to the terms and conditions of the GNU General 
> > Public
> > + * License.  See the file COPYING in the main directory of this archive
> > + * for more details.
> > + *
> > + * Authors:
> > + *Vetter 
> > + */
> 
> The above chunk doesn't sound appropriate for extracting existing code...

Well it all horribly predates git history, so no idea who actually wrote
this. I'll copy over the old header on top.
-Daniel
> 
> Gr{oetje,eeting}s,
> 
> Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 
> linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like 
> that.
> -- Linus Torvalds

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

[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=82162

--- Comment #13 from sarnex  ---
Sorry, it appears the issue has not been fixed. The message "radeon: Failed to
allocate a buffer" no longer prints, but the original radeon_gem_object_create
message still spams dmesg and kern.log.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/b6976766/attachment.html>

[PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2

[Jerome Glisse]

On Wed, Aug 06, 2014 at 02:34:16PM -0400, Jerome Glisse wrote:
> On Wed, Aug 06, 2014 at 07:17:25PM +0200, Christian K?nig wrote:
> > Am 06.08.2014 um 18:08 schrieb Jerome Glisse:
> > >On Wed, Aug 06, 2014 at 08:55:28AM +0200, Christian K?nig wrote:
> > >>Am 06.08.2014 um 00:13 schrieb Jerome Glisse:
> > >>>On Tue, Aug 05, 2014 at 07:45:21PM +0200, Christian K?nig wrote:
> > Am 05.08.2014 um 19:39 schrieb Jerome Glisse:
> > >On Tue, Aug 05, 2014 at 06:05:29PM +0200, Christian K?nig wrote:
> > >>From: Christian K?nig 
> > >>
> > >>Avoid problems with writeback by limiting userptr to anonymous memory.
> > >>
> > >>v2: add commit and code comments
> > >I guess, i have not expressed myself clearly. This is bogus, you 
> > >pretend
> > >you want to avoid writeback issue but you still allow userspace to map
> > >file backed pages (which by the way might be a regular bo object from
> > >another device for instance and that would be fun).
> > >
> > >So this patch is a no go and i would rather see that this userptr to
> > >be restricted to anon vma only no matter what. No flags here.
> > Mapping of non anonymous memory (e.g. everything get_user_pages won't 
> > fail
> > with) is restricted to read only access by the GPU.
> > 
> > I'm fine with making it a hard requirement for all mappings if you say 
> > it's
> > a must have.
> > 
> > >>>Well for time being you should force read only. The way you implement 
> > >>>write
> > >>>is broken. Here is how it can abuse to allow write to a file backed mmap.
> > >>>
> > >>>mmap(fixaddress,fixedsize,NOFD)
> > >>>userptr_ioctl(fixedaddress, RADEON_GEM_USERPTR_ANONONLY)
> > >>>// bo is created successfully because fixedaddress is part of anonvma
> > >>>munmap(fixedaddress,fixedsize)
> > >>>// radeon get mmu_notifier_range_start callback and unbind page from the
> > >>>// bo but radeon does not know there was an unmap.
> > >>>mmap(fixaddress,fixedsize,fd_to_this_read_only_file_i_want_to_write_to)
> > >>>radeon_ioctl_use_my_userptrbo
> > >>>// bo is bind again by radeon and because all flag are set at creation
> > >>>// it is map with write permission allowing someone to write to a file
> > >>>// that might be read only for the user.
> > >>>//
> > >>>// Script kiddies it's time to learn about gpu ...
> > >>>
> > >>>Of course if you this patch (kind of selling my own junk here) :
> > >>>
> > >>>http://www.spinics.net/lists/linux-mm/msg75878.html
> > >>>
> > >>>then you could know inside the range_start that you should remove the
> > >>>write permission and that it should be rechecked on next bind.
> > >>>
> > >>>Note that i have not read much of your code so maybe you handle this
> > >>>case somehow.
> > >>I've stumbled over this attack vector as well and it's the reason why I've
> > >>moved checking the access rights to the bind callback instead of BO 
> > >>creation
> > >>time with V5 of the patch.
> > >>
> > >>This way you get an -EFAULT if you try something like this on command
> > >>submission time.
> > >So you seem immune to that issue but you are still not checking if the anon
> > >vma is writeable which you should again security concern here.
> > 
> > We check the access rights of the pointer using:
> > >if (!access_ok(write ? VERIFY_WRITE : VERIFY_READ,
> > >(long)gtt->userptr,
> > >   ttm->num_pages * PAGE_SIZE))
> > >return -EFAULT;
> > 
> > Shouldn't that be enough?
> 
> No, access_ok only check against special area on some architecture and i am
> pretty sure on x86 the VERIFY_WRITE or VERIFY_READ is just flat out ignored.
> 
> What you need to test is the vma vm_flags somethings like
> 
> if (write && !(vma->vm_flags VM_WRITE))
>return -EPERM;
> 
> Which need to happen on all bind.
> 
> Cheers,
> J?r?me

I should add that access_ok should be done only once inside the gem userptr 
ioctl
as access_ok only check that the address is a valid userspace address and not 
some
special address (like an address inside the kernel address space or a non 
canonical
address on x86-64 ...). It it returns true the first time then it will always 
return
true.

Only vma need to be checked on each bind.

> 
> > 
> > Christian.
> > 
> > >
> > >Cheers,
> > >J?r?me
> > 

[PATCH] drm: Don't grab an fb reference for the idr

[Daniel Vetter]

On Wed, Aug 06, 2014 at 07:11:28AM -0400, Rob Clark wrote:
> On Wed, Aug 6, 2014 at 3:10 AM, Daniel Vetter  
> wrote:
> > The current refcounting scheme is that the fb lookup idr also holds a
> > reference. This works out nicely bacause thus far we've always
> > explicitly cleaned up idr entries for framebuffers:
> > - Userspace fbs get removed in the rmfb ioctl or when the drm file
> >   gets closed.
> > - Kernel fbs (for fbdev emulation) get cleaned up by the driver code
> >   at module unload time.
> >
> > But now i915 also reconstructs the bios fbs for a smooth transition.
> > And that fb is purely transitional and should get removed immmediately
> > once all crtcs stop using it. Of course if the i915 fbdev code decides
> > to reuse it as the main fbdev fb then it shouldn't be cleaned up, but
> > in that case the fbdev code will grab it's own reference.
> >
> > The problem is now that we also want to register that takeover fb in
> > the idr, so that userspace can do a smooth transition (animated maybe
> > even!) itself. But currently we have no one who will clean up the idr
> > reference once that fb isn't useful any more, and so essentially leak
> > it.
> 
> ewww..  couldn't you do some scheme on lastclose to check if no more
> crtc's are scanning out that fb, and if not then remove the idr?

There's no natural point really but when we drop the last reference for
it. Going the weak reference route looked the most natural. And I honestly
expect other drivers to eventually do the same - forcing a modeset on
boot-up is kinda not too pretty, and permanently reserving a big
framebuffer just for the bios doesn't sound good either. This approach
would nicely solve it for everyone.
-Daniel

> 
> BR,
> -R
> 
> 
> > Fix this by no longer holding a full fb reference for the idr, but
> > instead just have a weak reference using kref_get_unless_zero. But
> > that requires us to synchronize and clean up with the idr and fb_lock
> > in drm_framebuffer_free, so add that. It's a bit ugly that we have to
> > unconditionally grab the fb_lock, but without that someone might creep
> > through a race.
> >
> > This leak was caught by the fb leak check in drm_mode_config_cleanup.
> > Originally the leak was introduced in
> >
> > commit 46f297fb83d4f9a6f6891964beb184664341a28b
> > Author: Jesse Barnes 
> > Date:   Fri Mar 7 08:57:48 2014 -0800
> >
> > drm/i915: add plane_config fetching infrastructure v2
> >
> > Cc:  Jesse Barnes 
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=77511
> > Signed-off-by: Daniel Vetter 
> > ---
> >  drivers/gpu/drm/drm_crtc.c | 46 
> > --
> >  1 file changed, 28 insertions(+), 18 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> > index fa2be24c..33ff631c8d23 100644
> > --- a/drivers/gpu/drm/drm_crtc.c
> > +++ b/drivers/gpu/drm/drm_crtc.c
> > @@ -515,9 +515,6 @@ int drm_framebuffer_init(struct drm_device *dev, struct 
> > drm_framebuffer *fb,
> > if (ret)
> > goto out;
> >
> > -   /* Grab the idr reference. */
> > -   drm_framebuffer_reference(fb);
> > -
> > dev->mode_config.num_fb++;
> > list_add(>head, >mode_config.fb_list);
> >  out:
> > @@ -527,10 +524,34 @@ out:
> >  }
> >  EXPORT_SYMBOL(drm_framebuffer_init);
> >
> > +/* dev->mode_config.fb_lock must be held! */
> > +static void __drm_framebuffer_unregister(struct drm_device *dev,
> > +struct drm_framebuffer *fb)
> > +{
> > +   mutex_lock(>mode_config.idr_mutex);
> > +   idr_remove(>mode_config.crtc_idr, fb->base.id);
> > +   mutex_unlock(>mode_config.idr_mutex);
> > +
> > +   fb->base.id = 0;
> > +}
> > +
> >  static void drm_framebuffer_free(struct kref *kref)
> >  {
> > struct drm_framebuffer *fb =
> > container_of(kref, struct drm_framebuffer, 
> > refcount);
> > +   struct drm_device *dev = fb->dev;
> > +
> > +   /*
> > +* The lookup idr holds a weak reference, which has not necessarily 
> > been
> > +* removed at this point. Check for that.
> > +*/
> > +   mutex_lock(>mode_config.fb_lock);
> > +   if (fb->base.id) {
> > +   /* Mark fb as reaped and drop idr ref. */
> > +   __drm_framebuffer_unregister(dev, fb);
> > +   }
> > +   mutex_unlock(>mode_config.fb_lock);
> > +
> > fb->funcs->destroy(fb);
> >  }
> >
> > @@ -567,8 +588,10 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct 
> > drm_device *dev,
> >
> > mutex_lock(>mode_config.fb_lock);
> > fb = __drm_framebuffer_lookup(dev, id);
> > -   if (fb)
> > -   drm_framebuffer_reference(fb);
> > +   if (fb) {
> > +   if (!kref_get_unless_zero(>refcount))
> > +   fb = NULL;
> > +   }
> > mutex_unlock(>mode_config.fb_lock);
> >
> > return fb;
> > @@ -612,19 +635,6 @@ static void 

[PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2

[Jerome Glisse]

On Wed, Aug 06, 2014 at 07:17:25PM +0200, Christian K?nig wrote:
> Am 06.08.2014 um 18:08 schrieb Jerome Glisse:
> >On Wed, Aug 06, 2014 at 08:55:28AM +0200, Christian K?nig wrote:
> >>Am 06.08.2014 um 00:13 schrieb Jerome Glisse:
> >>>On Tue, Aug 05, 2014 at 07:45:21PM +0200, Christian K?nig wrote:
> Am 05.08.2014 um 19:39 schrieb Jerome Glisse:
> >On Tue, Aug 05, 2014 at 06:05:29PM +0200, Christian K?nig wrote:
> >>From: Christian K?nig 
> >>
> >>Avoid problems with writeback by limiting userptr to anonymous memory.
> >>
> >>v2: add commit and code comments
> >I guess, i have not expressed myself clearly. This is bogus, you pretend
> >you want to avoid writeback issue but you still allow userspace to map
> >file backed pages (which by the way might be a regular bo object from
> >another device for instance and that would be fun).
> >
> >So this patch is a no go and i would rather see that this userptr to
> >be restricted to anon vma only no matter what. No flags here.
> Mapping of non anonymous memory (e.g. everything get_user_pages won't fail
> with) is restricted to read only access by the GPU.
> 
> I'm fine with making it a hard requirement for all mappings if you say 
> it's
> a must have.
> 
> >>>Well for time being you should force read only. The way you implement write
> >>>is broken. Here is how it can abuse to allow write to a file backed mmap.
> >>>
> >>>mmap(fixaddress,fixedsize,NOFD)
> >>>userptr_ioctl(fixedaddress, RADEON_GEM_USERPTR_ANONONLY)
> >>>// bo is created successfully because fixedaddress is part of anonvma
> >>>munmap(fixedaddress,fixedsize)
> >>>// radeon get mmu_notifier_range_start callback and unbind page from the
> >>>// bo but radeon does not know there was an unmap.
> >>>mmap(fixaddress,fixedsize,fd_to_this_read_only_file_i_want_to_write_to)
> >>>radeon_ioctl_use_my_userptrbo
> >>>// bo is bind again by radeon and because all flag are set at creation
> >>>// it is map with write permission allowing someone to write to a file
> >>>// that might be read only for the user.
> >>>//
> >>>// Script kiddies it's time to learn about gpu ...
> >>>
> >>>Of course if you this patch (kind of selling my own junk here) :
> >>>
> >>>http://www.spinics.net/lists/linux-mm/msg75878.html
> >>>
> >>>then you could know inside the range_start that you should remove the
> >>>write permission and that it should be rechecked on next bind.
> >>>
> >>>Note that i have not read much of your code so maybe you handle this
> >>>case somehow.
> >>I've stumbled over this attack vector as well and it's the reason why I've
> >>moved checking the access rights to the bind callback instead of BO creation
> >>time with V5 of the patch.
> >>
> >>This way you get an -EFAULT if you try something like this on command
> >>submission time.
> >So you seem immune to that issue but you are still not checking if the anon
> >vma is writeable which you should again security concern here.
> 
> We check the access rights of the pointer using:
> >if (!access_ok(write ? VERIFY_WRITE : VERIFY_READ,
> >(long)gtt->userptr,
> >   ttm->num_pages * PAGE_SIZE))
> >return -EFAULT;
> 
> Shouldn't that be enough?

No, access_ok only check against special area on some architecture and i am
pretty sure on x86 the VERIFY_WRITE or VERIFY_READ is just flat out ignored.

What you need to test is the vma vm_flags somethings like

if (write && !(vma->vm_flags VM_WRITE))
   return -EPERM;

Which need to happen on all bind.

Cheers,
J?r?me

> 
> Christian.
> 
> >
> >Cheers,
> >J?r?me
> 

[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=82162

--- Comment #12 from sarnex  ---
(In reply to comment #11)
> Created attachment 104132 [details] [review]
> r600g/radeon: Don't try to allocate CMASK BO of size 0
> 
> This patch should at least work around the problem, but I'm not sure it's
> the proper fix.

Michel, the patch appears to fix the problem without any sideeffects. Is there
any chance of it getting into the main git? Thanks.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/078cdbef/attachment.html>

[PATCH] video/fbdev: Always built-in video= cmdline parsing

[Daniel Vetter]

In drm/i915 we want to get at the video= cmdline modes even when we
don't have fbdev support enabled, so that users can always override
the kernel's initial mode selection.

But that gives us a direct depency upon the parsing code in the fbdev
subsystem. Since it's so little code just extract these 2 functions
and always build them in.

Whiel at it fix the checkpatch fail in this code.

v2: Also move fb_mode_option. Spotted by the kbuild.

Cc: Plagniol-Villard 
Cc: Tomi Valkeinen 
Cc: linux-fbdev at vger.kernel.org
Signed-off-by: Daniel Vetter 

--

I prefer if we can merge this through drm-next since we'll use it
there in follow-up patches.
-Daniel
---
 drivers/video/fbdev/core/Makefile |   2 +-
 drivers/video/fbdev/core/fb_cmdline.c | 106 ++
 drivers/video/fbdev/core/fbmem.c  |  92 -
 drivers/video/fbdev/core/modedb.c |   3 -
 4 files changed, 107 insertions(+), 96 deletions(-)
 create mode 100644 drivers/video/fbdev/core/fb_cmdline.c

diff --git a/drivers/video/fbdev/core/Makefile 
b/drivers/video/fbdev/core/Makefile
index fa306538dac2..891c1f890e03 100644
--- a/drivers/video/fbdev/core/Makefile
+++ b/drivers/video/fbdev/core/Makefile
@@ -1,4 +1,4 @@
-obj-y += fb_notify.o
+obj-y += fb_notify.o fb_cmdline.o
 obj-$(CONFIG_FB)  += fb.o
 fb-y  := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
  modedb.o fbcvt.o
diff --git a/drivers/video/fbdev/core/fb_cmdline.c 
b/drivers/video/fbdev/core/fb_cmdline.c
new file mode 100644
index ..e8ce614bdd03
--- /dev/null
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -0,0 +1,106 @@
+/*
+ *  linux/drivers/video/fb_cmdline.c
+ *
+ *  Copyright (C) 2014 Intel Corp
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive
+ * for more details.
+ *
+ * Authors:
+ *Vetter 
+ */
+#include 
+#include 
+
+static char *video_options[FB_MAX] __read_mostly;
+static int ofonly __read_mostly;
+
+const char *fb_mode_option;
+EXPORT_SYMBOL_GPL(fb_mode_option);
+
+/**
+ * fb_get_options - get kernel boot parameters
+ * @name:   framebuffer name as it would appear in
+ *  the boot parameter line
+ *  (video=:)
+ * @option: the option will be stored here
+ *
+ * NOTE: Needed to maintain backwards compatibility
+ */
+int fb_get_options(const char *name, char **option)
+{
+   char *opt, *options = NULL;
+   int retval = 0;
+   int name_len = strlen(name), i;
+
+   if (name_len && ofonly && strncmp(name, "offb", 4))
+   retval = 1;
+
+   if (name_len && !retval) {
+   for (i = 0; i < FB_MAX; i++) {
+   if (video_options[i] == NULL)
+   continue;
+   if (!video_options[i][0])
+   continue;
+   opt = video_options[i];
+   if (!strncmp(name, opt, name_len) &&
+   opt[name_len] == ':')
+   options = opt + name_len + 1;
+   }
+   }
+   /* No match, pass global option */
+   if (!options && option && fb_mode_option)
+   options = kstrdup(fb_mode_option, GFP_KERNEL);
+   if (options && !strncmp(options, "off", 3))
+   retval = 1;
+
+   if (option)
+   *option = options;
+
+   return retval;
+}
+EXPORT_SYMBOL(fb_get_options);
+
+/**
+ * video_setup - process command line options
+ * @options: string of options
+ *
+ * Process command line options for frame buffer subsystem.
+ *
+ * NOTE: This function is a __setup and __init function.
+ *It only stores the options.  Drivers have to call
+ *fb_get_options() as necessary.
+ *
+ * Returns zero.
+ *
+ */
+static int __init video_setup(char *options)
+{
+   int i, global = 0;
+
+   if (!options || !*options)
+   global = 1;
+
+   if (!global && !strncmp(options, "ofonly", 6)) {
+   ofonly = 1;
+   global = 1;
+   }
+
+   if (!global && !strchr(options, ':')) {
+   fb_mode_option = options;
+   global = 1;
+   }
+
+   if (!global) {
+   for (i = 0; i < FB_MAX; i++) {
+   if (video_options[i] == NULL) {
+   video_options[i] = options;
+   break;
+   }
+   }
+   }
+
+   return 1;
+}
+__setup("video=", video_setup);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index b5e85f6c1c26..0705d8883ede 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1908,96 +1908,4 @@ int fb_new_modelist(struct fb_info *info)

[PATCH 2/2] drm/i915: Free pending page flip events at .preclose()

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

If there are pending page flips when the fd gets closed those page
flips may have events associated to them. When the page flip eventually
completes it will queue the event to file_priv->event_list, but that
may be too late and file_priv->event_list has already been cleaned up.
Thus we leak a bit of kernel memory in the form of the event structure.

To avoid such problems clear out such pending events from
intel_crtc->unpin_work at ->preclose(). Any event that already made it
to file_priv->event_list will get cleaned up by the drm_release_events()
a bit later.

We can ignore the file_priv->event_space accounting since file_priv is
going away. This is already how drm core deals with pending vblank
events, which are maintained by the drm core.

What saves us from a total disaster (ie. dereferencing and alrady
freed file_priv) is the fact that the fb descruction triggers a modeset
and there we wait for pending flips.

Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/i915/i915_dma.c  |  3 +++
 drivers/gpu/drm/i915/intel_display.c | 22 ++
 drivers/gpu/drm/i915/intel_drv.h |  1 +
 3 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 2e7f03a..c965698 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1981,6 +1981,9 @@ void i915_driver_preclose(struct drm_device *dev, struct 
drm_file *file)
i915_gem_context_close(dev, file);
i915_gem_release(dev, file);
mutex_unlock(>struct_mutex);
+
+   if (drm_core_check_feature(dev, DRIVER_MODESET))
+   intel_modeset_preclose(dev, file);
 }

 void i915_driver_postclose(struct drm_device *dev, struct drm_file *file)
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 883af0b..4230e4a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13416,3 +13416,25 @@ intel_display_print_error_state(struct 
drm_i915_error_state_buf *m,
err_printf(m, "  VSYNC: %08x\n", error->transcoder[i].vsync);
}
 }
+
+void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file)
+{
+   struct intel_crtc *crtc;
+
+   for_each_intel_crtc(dev, crtc) {
+   struct intel_unpin_work *work;
+   unsigned long irqflags;
+
+   spin_lock_irqsave(>event_lock, irqflags);
+
+   work = crtc->unpin_work;
+
+   if (work && work->event &&
+   work->event->base.file_priv == file) {
+   kfree(work->event);
+   work->event = NULL;
+   }
+
+   spin_unlock_irqrestore(>event_lock, irqflags);
+   }
+}
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 28d185d..8f04ba8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -888,6 +888,7 @@ void intel_mode_from_pipe_config(struct drm_display_mode 
*mode,
 struct intel_crtc_config *pipe_config);
 int intel_format_to_fourcc(int format);
 void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
+void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);


 /* intel_dp.c */
-- 
1.8.5.5

[PATCH 1/2] drm: Warn when leaking flip events on close

[ville.syrj...@linux.intel.com]

From: Ville Syrj?l? 

Warn when there are events on the file_priv->event_list just before
file_priv gets freed. This can occur if the driver doesn't clean up
pending page flip events in ->preclose().

Signed-off-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_fops.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 8f91062..0fa4dad 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -529,6 +529,8 @@ int drm_release(struct inode *inode, struct file *filp)
if (drm_core_check_feature(dev, DRIVER_PRIME))
drm_prime_destroy_file_private(_priv->prime);

+   WARN_ON(!list_empty(_priv->event_list));
+
put_pid(file_priv->pid);
kfree(file_priv);

-- 
1.8.5.5

[PATCH 2/3] drm: Fix emitted vblank timestamps in drm_vblank_off()

[Ville Syrjälä]

On Wed, Aug 06, 2014 at 03:22:45AM +0200, Mario Kleiner wrote:
> Move the query for vblank count and time before the
> vblank_disable_and_save(), because the disable fn
> will invalidate the vblank timestamps, so all emitted
> events would carry an invalid zero timestamp instead of
> the timestamp of the vblank of vblank disable. This could
> confuse clients.
> 
> Signed-off-by: Mario Kleiner 
> Cc: stable at vger.kernel.org
> ---
>  drivers/gpu/drm/drm_irq.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 553a58c..89e91e3 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -1019,13 +1019,14 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
>   unsigned long irqflags;
>   unsigned int seq;
>  
> + /* Get 'now' vblank ts before it gets cleared by vblank disable */
> + seq = drm_vblank_count_and_time(dev, crtc, );
> +

This will cause us to send out a potentially stale vblank seq/ts. I had
a different solution to this (not clearing the timestamps) in my pending
vblank series. I have a few more patches on top of that series lined up.
Let me send out the full series so we can discuss it if needed...

>   spin_lock_irqsave(>vbl_lock, irqflags);
>   vblank_disable_and_save(dev, crtc);
>   wake_up(>vblank[crtc].queue);
>  
>   /* Send any queued vblank events, lest the natives grow disquiet */
> - seq = drm_vblank_count_and_time(dev, crtc, );
> -
>   spin_lock(>event_lock);
>   list_for_each_entry_safe(e, t, >vblank_event_list, base.link) {
>   if (e->pipe != crtc)
> -- 
> 1.9.1
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj?l?
Intel OTC

[PATCH 1/3] drm: Remove drm_vblank_cleanup from drm_vblank_init error path.

[Ville Syrjälä]

On Wed, Aug 06, 2014 at 03:22:44AM +0200, Mario Kleiner wrote:
> drm_vblank_cleanup() would operate on non-existent dev->vblank
> data structure, as failure to allocate that data structure is
> what triggers the error path in the first place.
> 
> Signed-off-by: Mario Kleiner 
> Cc: stable at vger.kernel.org

Reviewed-by: Ville Syrj?l? 

> ---
>  drivers/gpu/drm/drm_irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 0de123a..553a58c 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -224,7 +224,7 @@ int drm_vblank_init(struct drm_device *dev, int num_crtcs)
>   return 0;
>  
>  err:
> - drm_vblank_cleanup(dev);
> + dev->num_crtcs = 0;
>   return ret;
>  }
>  EXPORT_SYMBOL(drm_vblank_init);
> -- 
> 1.9.1
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj?l?
Intel OTC

[PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup()

[Ville Syrjälä]

On Wed, Aug 06, 2014 at 03:22:46AM +0200, Mario Kleiner wrote:
> Calling vblank_disable_fn() will cause that function to no-op
> if !dev->vblank_disable_allowed for some kms drivers, e.g.,
> on nouveau-kms. This can cause the gpu vblank irq's to not get
> disabled before freeing the dev->vblank array, so if a
> vblank irq fires and calls into drm_handle_vblank() after
> drm_vblank_cleanup() completes, it will cause use-after-free
> access to dev->vblank array.
> 
> Call vblank_disable_and_save unconditionally, so vblank irqs
> are guaranteed to be off, before we delete the data structures
> on which they operate.
> 
> Signed-off-by: Mario Kleiner 
> Cc: stable at vger.kernel.org

No idea what games nouveau is playign with that flag, but this patch
should be fine at least for drivers that don't do such things.

Reviewed-by: Ville Syrj?l? 

> ---
>  drivers/gpu/drm/drm_irq.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
> index 89e91e3..22e2bba9 100644
> --- a/drivers/gpu/drm/drm_irq.c
> +++ b/drivers/gpu/drm/drm_irq.c
> @@ -164,6 +164,7 @@ static void vblank_disable_fn(unsigned long arg)
>  void drm_vblank_cleanup(struct drm_device *dev)
>  {
>   int crtc;
> + unsigned long irqflags;
>  
>   /* Bail if the driver didn't call drm_vblank_init() */
>   if (dev->num_crtcs == 0)
> @@ -171,7 +172,9 @@ void drm_vblank_cleanup(struct drm_device *dev)
>  
>   for (crtc = 0; crtc < dev->num_crtcs; crtc++) {
>   del_timer_sync(>vblank[crtc].disable_timer);
> - vblank_disable_fn((unsigned long)>vblank[crtc]);
> + spin_lock_irqsave(>vbl_lock, irqflags);
> + vblank_disable_and_save(dev, crtc);
> + spin_unlock_irqrestore(>vbl_lock, irqflags);
>   }
>  
>   kfree(dev->vblank);
> -- 
> 1.9.1
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj?l?
Intel OTC

[PATCH 6/6] drm: Resetting rotation property

[sonika.jin...@intel.com]

From: Sonika Jindal 

Reset rotation property to 0.

v2: Resetting after disabling the plane

Cc: dri-devel at lists.freedesktop.org
Signed-off-by: Sonika Jindal 
Reviewed-by: Ville Syrj?l? 
---
 drivers/gpu/drm/drm_fb_helper.c |9 -
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 3144db9..d139edd 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -345,10 +345,17 @@ static bool restore_fbdev_mode(struct drm_fb_helper 
*fb_helper)

drm_warn_on_modeset_not_all_locked(dev);

-   list_for_each_entry(plane, >mode_config.plane_list, head)
+   list_for_each_entry(plane, >mode_config.plane_list, head) {
if (plane->type != DRM_PLANE_TYPE_PRIMARY)
drm_plane_force_disable(plane);

+   if (dev->mode_config.rotation_property) {
+   drm_object_property_set_value(>base,
+   dev->mode_config.rotation_property,
+   BIT(DRM_ROTATE_0));
+   }
+   }
+
for (i = 0; i < fb_helper->crtc_count; i++) {
struct drm_mode_set *mode_set = 
_helper->crtc_info[i].mode_set;
struct drm_crtc *crtc = mode_set->crtc;
-- 
1.7.10.4

[PATCH] drm: idiot-proof vblank

[Rob Clark]

After spending slightly more time than I'd care to admit debugging the
various and presumably spectacular way things fail when you pass too low
a value to drm_vblank_init() (thanks console-lock for not letting me see
the carnage!), I decided it might be a good idea to add some sanity
checking.

Signed-off-by: Rob Clark 
---
btw, I wonder if we could add a module param hack to toss initial modeset
off to a workqueue to sneak it out from the tyranny of console_lock?

 drivers/gpu/drm/drm_irq.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c
index 0de123a..6f16a104 100644
--- a/drivers/gpu/drm/drm_irq.c
+++ b/drivers/gpu/drm/drm_irq.c
@@ -730,6 +730,8 @@ EXPORT_SYMBOL(drm_get_last_vbltimestamp);
  */
 u32 drm_vblank_count(struct drm_device *dev, int crtc)
 {
+   if (WARN_ON(crtc >= dev->num_crtcs))
+   return 0;
return atomic_read(>vblank[crtc].count);
 }
 EXPORT_SYMBOL(drm_vblank_count);
@@ -752,6 +754,9 @@ u32 drm_vblank_count_and_time(struct drm_device *dev, int 
crtc,
 {
u32 cur_vblank;

+   if (WARN_ON(crtc >= dev->num_crtcs))
+   return 0;
+
/* Read timestamp from slot of _vblank_time ringbuffer
 * that corresponds to current vblank count. Retry if
 * count has incremented during readout. This works like
@@ -927,6 +932,9 @@ int drm_vblank_get(struct drm_device *dev, int crtc)
unsigned long irqflags;
int ret = 0;

+   if (WARN_ON(crtc >= dev->num_crtcs))
+   return -EINVAL;
+
spin_lock_irqsave(>vbl_lock, irqflags);
/* Going from 0->1 means we have to enable interrupts again */
if (atomic_add_return(1, >vblank[crtc].refcount) == 1) {
@@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
 {
BUG_ON(atomic_read(>vblank[crtc].refcount) == 0);

+   if (WARN_ON(crtc >= dev->num_crtcs))
+   return;
+
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(>vblank[crtc].refcount) &&
(drm_vblank_offdelay > 0))
@@ -1019,6 +1030,9 @@ void drm_vblank_off(struct drm_device *dev, int crtc)
unsigned long irqflags;
unsigned int seq;

+   if (WARN_ON(crtc >= dev->num_crtcs))
+   return;
+
spin_lock_irqsave(>vbl_lock, irqflags);
vblank_disable_and_save(dev, crtc);
wake_up(>vblank[crtc].queue);
@@ -1078,6 +1092,9 @@ void drm_vblank_on(struct drm_device *dev, int crtc)
 {
unsigned long irqflags;

+   if (WARN_ON(crtc >= dev->num_crtcs))
+   return;
+
spin_lock_irqsave(>vbl_lock, irqflags);
/* re-enable interrupts if there's are users left */
if (atomic_read(>vblank[crtc].refcount) != 0)
@@ -1131,6 +1148,10 @@ void drm_vblank_pre_modeset(struct drm_device *dev, int 
crtc)
/* vblank is not initialized (IRQ not installed ?), or has been freed */
if (!dev->num_crtcs)
return;
+
+   if (WARN_ON(crtc >= dev->num_crtcs))
+   return;
+
/*
 * To avoid all the problems that might happen if interrupts
 * were enabled/disabled around or between these calls, we just
@@ -1439,6 +1460,9 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc)
if (!dev->num_crtcs)
return false;

+   if (WARN_ON(crtc >= dev->num_crtcs))
+   return false;
+
/* Need timestamp lock to prevent concurrent execution with
 * vblank enable/disable, as this would cause inconsistent
 * or corrupted timestamps and vblank counts.
-- 
1.9.3

[Bug 42878] black flash every time pm changes clocks

[bugzilla-dae...@freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=42878

almos  changed:

   What|Removed |Added

 Status|NEW |RESOLVED
 Resolution|--- |FIXED

--- Comment #2 from almos  ---
The new dpm method made this one obsolete. Closing.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-- next part --
An HTML attachment was scrubbed...
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/ed83531e/attachment.html>

[PATCH] video/fbdev: Always built-in video= cmdline parsing

[Geert Uytterhoeven]

On Wed, Aug 6, 2014 at 11:43 AM, Daniel Vetter  
wrote:
> In drm/i915 we want to get at the video= cmdline modes even when we
> don't have fbdev support enabled, so that users can always override
> the kernel's initial mode selection.
>
> But that gives us a direct depency upon the parsing code in the fbdev
> subsystem. Since it's so little code just extract these 2 functions
> and always build them in.

How much is "so little"? Think memory-constrained systems.

You can still build it depending on CONFIG_FB or CONFIG_DRM_I915.

> diff --git a/drivers/video/fbdev/core/Makefile 
> b/drivers/video/fbdev/core/Makefile
> index fa306538dac2..891c1f890e03 100644
> --- a/drivers/video/fbdev/core/Makefile
> +++ b/drivers/video/fbdev/core/Makefile
> @@ -1,4 +1,4 @@
> -obj-y += fb_notify.o

Oh, this is already unconditional. Who are its users?

> +obj-y += fb_notify.o fb_cmdline.o
>  obj-$(CONFIG_FB)  += fb.o
>  fb-y  := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
>   modedb.o fbcvt.o
> diff --git a/drivers/video/fbdev/core/fb_cmdline.c 
> b/drivers/video/fbdev/core/fb_cmdline.c
> new file mode 100644
> index ..91503a43213e
> --- /dev/null
> +++ b/drivers/video/fbdev/core/fb_cmdline.c
> @@ -0,0 +1,103 @@
> +/*
> + *  linux/drivers/video/fb_cmdline.c
> + *
> + *  Copyright (C) 2014 Intel Corp
> + *
> + * This file is subject to the terms and conditions of the GNU General Public
> + * License.  See the file COPYING in the main directory of this archive
> + * for more details.
> + *
> + * Authors:
> + *Vetter 
> + */

The above chunk doesn't sound appropriate for extracting existing code...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at 
linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

[PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2

[Jerome Glisse]

On Wed, Aug 06, 2014 at 08:55:28AM +0200, Christian K?nig wrote:
> Am 06.08.2014 um 00:13 schrieb Jerome Glisse:
> >On Tue, Aug 05, 2014 at 07:45:21PM +0200, Christian K?nig wrote:
> >>Am 05.08.2014 um 19:39 schrieb Jerome Glisse:
> >>>On Tue, Aug 05, 2014 at 06:05:29PM +0200, Christian K?nig wrote:
> From: Christian K?nig 
> 
> Avoid problems with writeback by limiting userptr to anonymous memory.
> 
> v2: add commit and code comments
> >>>I guess, i have not expressed myself clearly. This is bogus, you pretend
> >>>you want to avoid writeback issue but you still allow userspace to map
> >>>file backed pages (which by the way might be a regular bo object from
> >>>another device for instance and that would be fun).
> >>>
> >>>So this patch is a no go and i would rather see that this userptr to
> >>>be restricted to anon vma only no matter what. No flags here.
> >>Mapping of non anonymous memory (e.g. everything get_user_pages won't fail
> >>with) is restricted to read only access by the GPU.
> >>
> >>I'm fine with making it a hard requirement for all mappings if you say it's
> >>a must have.
> >>
> >Well for time being you should force read only. The way you implement write
> >is broken. Here is how it can abuse to allow write to a file backed mmap.
> >
> >mmap(fixaddress,fixedsize,NOFD)
> >userptr_ioctl(fixedaddress, RADEON_GEM_USERPTR_ANONONLY)
> >// bo is created successfully because fixedaddress is part of anonvma
> >munmap(fixedaddress,fixedsize)
> >// radeon get mmu_notifier_range_start callback and unbind page from the
> >// bo but radeon does not know there was an unmap.
> >mmap(fixaddress,fixedsize,fd_to_this_read_only_file_i_want_to_write_to)
> >radeon_ioctl_use_my_userptrbo
> >// bo is bind again by radeon and because all flag are set at creation
> >// it is map with write permission allowing someone to write to a file
> >// that might be read only for the user.
> >//
> >// Script kiddies it's time to learn about gpu ...
> >
> >Of course if you this patch (kind of selling my own junk here) :
> >
> >http://www.spinics.net/lists/linux-mm/msg75878.html
> >
> >then you could know inside the range_start that you should remove the
> >write permission and that it should be rechecked on next bind.
> >
> >Note that i have not read much of your code so maybe you handle this
> >case somehow.
> 
> I've stumbled over this attack vector as well and it's the reason why I've
> moved checking the access rights to the bind callback instead of BO creation
> time with V5 of the patch.
> 
> This way you get an -EFAULT if you try something like this on command
> submission time.

So you seem immune to that issue but you are still not checking if the anon
vma is writeable which you should again security concern here.

Cheers,
J?r?me

[PATCH] video/fbdev: Always built-in video= cmdline parsing

[Daniel Vetter]

In drm/i915 we want to get at the video= cmdline modes even when we
don't have fbdev support enabled, so that users can always override
the kernel's initial mode selection.

But that gives us a direct depency upon the parsing code in the fbdev
subsystem. Since it's so little code just extract these 2 functions
and always build them in.

Whiel at it fix the checkpatch fail in this code.

Cc: Plagniol-Villard 
Cc: Tomi Valkeinen 
Cc: linux-fbdev at vger.kernel.org
Signed-off-by: Daniel Vetter 

--

I prefer if we can merge this through drm-next since we'll use it
there in follow-up patches.
-Daniel
---
 drivers/video/fbdev/core/Makefile |   2 +-
 drivers/video/fbdev/core/fb_cmdline.c | 103 ++
 drivers/video/fbdev/core/fbmem.c  |  92 --
 3 files changed, 104 insertions(+), 93 deletions(-)
 create mode 100644 drivers/video/fbdev/core/fb_cmdline.c

diff --git a/drivers/video/fbdev/core/Makefile 
b/drivers/video/fbdev/core/Makefile
index fa306538dac2..891c1f890e03 100644
--- a/drivers/video/fbdev/core/Makefile
+++ b/drivers/video/fbdev/core/Makefile
@@ -1,4 +1,4 @@
-obj-y += fb_notify.o
+obj-y += fb_notify.o fb_cmdline.o
 obj-$(CONFIG_FB)  += fb.o
 fb-y  := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
  modedb.o fbcvt.o
diff --git a/drivers/video/fbdev/core/fb_cmdline.c 
b/drivers/video/fbdev/core/fb_cmdline.c
new file mode 100644
index ..91503a43213e
--- /dev/null
+++ b/drivers/video/fbdev/core/fb_cmdline.c
@@ -0,0 +1,103 @@
+/*
+ *  linux/drivers/video/fb_cmdline.c
+ *
+ *  Copyright (C) 2014 Intel Corp
+ *
+ * This file is subject to the terms and conditions of the GNU General Public
+ * License.  See the file COPYING in the main directory of this archive
+ * for more details.
+ *
+ * Authors:
+ *Vetter 
+ */
+#include 
+#include 
+
+static char *video_options[FB_MAX] __read_mostly;
+static int ofonly __read_mostly;
+
+/**
+ * fb_get_options - get kernel boot parameters
+ * @name:   framebuffer name as it would appear in
+ *  the boot parameter line
+ *  (video=:)
+ * @option: the option will be stored here
+ *
+ * NOTE: Needed to maintain backwards compatibility
+ */
+int fb_get_options(const char *name, char **option)
+{
+   char *opt, *options = NULL;
+   int retval = 0;
+   int name_len = strlen(name), i;
+
+   if (name_len && ofonly && strncmp(name, "offb", 4))
+   retval = 1;
+
+   if (name_len && !retval) {
+   for (i = 0; i < FB_MAX; i++) {
+   if (video_options[i] == NULL)
+   continue;
+   if (!video_options[i][0])
+   continue;
+   opt = video_options[i];
+   if (!strncmp(name, opt, name_len) &&
+   opt[name_len] == ':')
+   options = opt + name_len + 1;
+   }
+   }
+   /* No match, pass global option */
+   if (!options && option && fb_mode_option)
+   options = kstrdup(fb_mode_option, GFP_KERNEL);
+   if (options && !strncmp(options, "off", 3))
+   retval = 1;
+
+   if (option)
+   *option = options;
+
+   return retval;
+}
+EXPORT_SYMBOL(fb_get_options);
+
+/**
+ * video_setup - process command line options
+ * @options: string of options
+ *
+ * Process command line options for frame buffer subsystem.
+ *
+ * NOTE: This function is a __setup and __init function.
+ *It only stores the options.  Drivers have to call
+ *fb_get_options() as necessary.
+ *
+ * Returns zero.
+ *
+ */
+static int __init video_setup(char *options)
+{
+   int i, global = 0;
+
+   if (!options || !*options)
+   global = 1;
+
+   if (!global && !strncmp(options, "ofonly", 6)) {
+   ofonly = 1;
+   global = 1;
+   }
+
+   if (!global && !strchr(options, ':')) {
+   fb_mode_option = options;
+   global = 1;
+   }
+
+   if (!global) {
+   for (i = 0; i < FB_MAX; i++) {
+   if (video_options[i] == NULL) {
+   video_options[i] = options;
+   break;
+   }
+   }
+   }
+
+   return 1;
+}
+__setup("video=", video_setup);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index b5e85f6c1c26..0705d8883ede 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1908,96 +1908,4 @@ int fb_new_modelist(struct fb_info *info)
return err;
 }

-static char *video_options[FB_MAX] __read_mostly;
-static int ofonly __read_mostly;
-
-/**
- * fb_get_options - get kernel boot parameters
- * @name:   

[PATCH 4/5] drm/radeon: add userptr flag to register MMU notifier v3

[Jerome Glisse]

On Tue, Aug 05, 2014 at 06:05:31PM +0200, Christian K?nig wrote:
> From: Christian K?nig 
> 
> Whenever userspace mapping related to our userptr change
> we wait for it to become idle and unmap it from GTT.
> 
> v2: rebased, fix mutex unlock in error path
> v3: improve commit message

Why in hell do you not register the mmu_notifier on first userptr bo ?

> 
> Signed-off-by: Christian K?nig 
> ---
>  drivers/gpu/drm/Kconfig|   1 +
>  drivers/gpu/drm/radeon/Makefile|   2 +-
>  drivers/gpu/drm/radeon/radeon.h|  12 ++
>  drivers/gpu/drm/radeon/radeon_device.c |   2 +
>  drivers/gpu/drm/radeon/radeon_gem.c|   9 +-
>  drivers/gpu/drm/radeon/radeon_mn.c | 272 
> +
>  drivers/gpu/drm/radeon/radeon_object.c |   1 +
>  include/uapi/drm/radeon_drm.h  |   1 +
>  8 files changed, 298 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/radeon/radeon_mn.c
> 
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 9b2eedc..2745284 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -115,6 +115,7 @@ config DRM_RADEON
>   select HWMON
>   select BACKLIGHT_CLASS_DEVICE
>   select INTERVAL_TREE
> + select MMU_NOTIFIER
>   help
> Choose this option if you have an ATI Radeon graphics card.  There
> are both PCI and AGP versions.  You don't need to choose this to
> diff --git a/drivers/gpu/drm/radeon/Makefile b/drivers/gpu/drm/radeon/Makefile
> index 0013ad0..c7fa1ae 100644
> --- a/drivers/gpu/drm/radeon/Makefile
> +++ b/drivers/gpu/drm/radeon/Makefile
> @@ -80,7 +80,7 @@ radeon-y += radeon_device.o radeon_asic.o radeon_kms.o \
>   r600_dpm.o rs780_dpm.o rv6xx_dpm.o rv770_dpm.o rv730_dpm.o rv740_dpm.o \
>   rv770_smc.o cypress_dpm.o btc_dpm.o sumo_dpm.o sumo_smc.o trinity_dpm.o 
> \
>   trinity_smc.o ni_dpm.o si_smc.o si_dpm.o kv_smc.o kv_dpm.o ci_smc.o \
> - ci_dpm.o dce6_afmt.o radeon_vm.o radeon_ucode.o radeon_ib.o
> + ci_dpm.o dce6_afmt.o radeon_vm.o radeon_ucode.o radeon_ib.o radeon_mn.o
>  
>  # add async DMA block
>  radeon-y += \
> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h
> index 3c6999e..511191f 100644
> --- a/drivers/gpu/drm/radeon/radeon.h
> +++ b/drivers/gpu/drm/radeon/radeon.h
> @@ -65,6 +65,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  #include 
>  #include 
> @@ -487,6 +488,9 @@ struct radeon_bo {
>  
>   struct ttm_bo_kmap_obj  dma_buf_vmap;
>   pid_t   pid;
> +
> + struct radeon_mn*mn;
> + struct interval_tree_node   mn_it;
>  };
>  #define gem_to_radeon_bo(gobj) container_of((gobj), struct radeon_bo, 
> gem_base)
>  
> @@ -1725,6 +1729,11 @@ void radeon_test_ring_sync(struct radeon_device *rdev,
>  struct radeon_ring *cpB);
>  void radeon_test_syncing(struct radeon_device *rdev);
>  
> +/*
> + * MMU Notifier
> + */
> +int radeon_mn_register(struct radeon_bo *bo, unsigned long addr);
> +void radeon_mn_unregister(struct radeon_bo *bo);
>  
>  /*
>   * Debugfs
> @@ -2372,6 +2381,9 @@ struct radeon_device {
>   /* tracking pinned memory */
>   u64 vram_pin_size;
>   u64 gart_pin_size;
> +
> + struct mutexmn_lock;
> + DECLARE_HASHTABLE(mn_hash, 7);
>  };
>  
>  bool radeon_is_px(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/radeon/radeon_device.c 
> b/drivers/gpu/drm/radeon/radeon_device.c
> index c8ea050..c58f84f 100644
> --- a/drivers/gpu/drm/radeon/radeon_device.c
> +++ b/drivers/gpu/drm/radeon/radeon_device.c
> @@ -1270,6 +1270,8 @@ int radeon_device_init(struct radeon_device *rdev,
>   init_rwsem(>pm.mclk_lock);
>   init_rwsem(>exclusive_lock);
>   init_waitqueue_head(>irq.vblank_queue);
> + mutex_init(>mn_lock);
> + hash_init(rdev->mn_hash);
>   r = radeon_gem_init(rdev);
>   if (r)
>   return r;
> diff --git a/drivers/gpu/drm/radeon/radeon_gem.c 
> b/drivers/gpu/drm/radeon/radeon_gem.c
> index 4506560..2a6fbf1 100644
> --- a/drivers/gpu/drm/radeon/radeon_gem.c
> +++ b/drivers/gpu/drm/radeon/radeon_gem.c
> @@ -291,7 +291,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, void 
> *data,
>  
>   /* reject unknown flag values */
>   if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
> - RADEON_GEM_USERPTR_ANONONLY | RADEON_GEM_USERPTR_VALIDATE))
> + RADEON_GEM_USERPTR_ANONONLY | RADEON_GEM_USERPTR_VALIDATE |
> + RADEON_GEM_USERPTR_REGISTER))
>   return -EINVAL;
>  
>   /* readonly pages not tested on older hardware */
> @@ -312,6 +313,12 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev, 
> void *data,
>   if (r)
>   goto release_object;
>  
> + if (args->flags & RADEON_GEM_USERPTR_REGISTER) {
> + r = radeon_mn_register(bo, args->addr);
> + if (r)
> + goto release_object;
> + }
> 

[Intel-gfx] [PATCH 1/6] drm: Renaming DP training vswing/pre-emph defines

[Jindal, Sonika]

On 8/5/2014 6:00 PM, Daniel Vetter wrote:
> On Tue, Aug 5, 2014 at 1:33 PM, Jindal, Sonika  
> wrote:
>>
>>
>> On 8/5/2014 4:45 PM, Daniel Vetter wrote:
>>>
>>> On Tue, Aug 05, 2014 at 04:38:17PM +0530, sonika.jindal at intel.com wrote:

 From: Sonika Jindal 

 Renaming defines to have levels instead of nominal values.

 Signed-off-by: Sonika Jindal 
>>>
>>>
>>> You can't split up patches like this since this will break compilation.
>>> For larger stuff (and imo this is right above the cutoff) you first need
>>> to add the new functions/defines, then convert everyone over. And only
>>> when all the drivers are converted can we apply the patch to remove the
>>> old functions/defines.
>>> -Daniel
>>>
>> Got your concern. So, I will repost the first patch keeping both the defines
>> and an additional last patch for removing the extra defines.
>
> Yeah. Btw for the actual replacement I highly recommend to do it with
> a semantic patch and cocci. There's unfortunately not a hole lot of
> good documentation around, but we recently started and it helps a lot
> with regenerating patches (e.g. for newly merged drm drivers) and
> avoiding mistakes for manual conversions. If we do a cocci patch we
> put it into the commit message, grep for @@.
> -Daniel
>
I need to find out about cocci. Can we go ahead with this patch series only?
-Sonika

[PATCH 1/6] drm: Renaming DP training vswing/pre-emph defines

[Jingoo Han]

On Tuesday, August 05, 2014 8:08 PM, Sonika Jindal wrote:
> 
> From: Sonika Jindal 
> 
> Renaming defines to have levels instead of nominal values.

(+cc Daniel Vetter)

Hi Sonika Jindal,

Thank you for sending the patch. However, please add the reason
to this commit message, as you said at '[PATCH 0/6] Rename DP
training vswing/pre-emph defines'.

Best regards.
Jingoo Han

> 
> Signed-off-by: Sonika Jindal 
> ---
>  include/drm/drm_dp_helper.h |   16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
> index a21568b..70f362b 100644
> --- a/include/drm/drm_dp_helper.h
> +++ b/include/drm/drm_dp_helper.h
> @@ -190,16 +190,16 @@
>  # define DP_TRAIN_VOLTAGE_SWING_MASK 0x3
>  # define DP_TRAIN_VOLTAGE_SWING_SHIFT0
>  # define DP_TRAIN_MAX_SWING_REACHED  (1 << 2)
> -# define DP_TRAIN_VOLTAGE_SWING_400  (0 << 0)
> -# define DP_TRAIN_VOLTAGE_SWING_600  (1 << 0)
> -# define DP_TRAIN_VOLTAGE_SWING_800  (2 << 0)
> -# define DP_TRAIN_VOLTAGE_SWING_1200 (3 << 0)
> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_0 (0 << 0)
> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_1 (1 << 0)
> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_2 (2 << 0)
> +# define DP_TRAIN_VOLTAGE_SWING_LEVEL_3 (3 << 0)
> 
>  # define DP_TRAIN_PRE_EMPHASIS_MASK  (3 << 3)
> -# define DP_TRAIN_PRE_EMPHASIS_0 (0 << 3)
> -# define DP_TRAIN_PRE_EMPHASIS_3_5   (1 << 3)
> -# define DP_TRAIN_PRE_EMPHASIS_6 (2 << 3)
> -# define DP_TRAIN_PRE_EMPHASIS_9_5   (3 << 3)
> +# define DP_TRAIN_PRE_EMPHASIS_LEVEL_0  (0 << 3)
> +# define DP_TRAIN_PRE_EMPHASIS_LEVEL_1  (1 << 3)
> +# define DP_TRAIN_PRE_EMPHASIS_LEVEL_2  (2 << 3)
> +# define DP_TRAIN_PRE_EMPHASIS_LEVEL_3  (3 << 3)
> 
>  # define DP_TRAIN_PRE_EMPHASIS_SHIFT 3
>  # define DP_TRAIN_MAX_PRE_EMPHASIS_REACHED  (1 << 5)
> --
> 1.7.10.4