[PATCH] drm/omap: move rotation property to drm core mode_config
Move rotation property to drm core mode_config. This allows us to ditch
the driver-private lastclose logic.
Cc: Rob Clark
Signed-off-by: Daniel Vetter
Signed-off-by: Tomi Valkeinen
---
drivers/gpu/drm/omapdrm/omap_crtc.c | 4 ++--
drivers/gpu/drm/omapdrm/omap_drv.c | 20
drivers/gpu/drm/omapdrm/omap_drv.h | 1 -
drivers/gpu/drm/omapdrm/omap_plane.c | 7 ---
4 files changed, 6 insertions(+), 26 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 2d28dc337cfb..1240fa61b397 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -385,9 +385,9 @@ static int omap_crtc_set_property(struct drm_crtc *crtc,
struct drm_property *property, uint64_t val)
{
struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
- struct omap_drm_private *priv = crtc->dev->dev_private;
+ struct drm_device *dev = crtc->dev;
- if (property == priv->rotation_prop) {
+ if (property == dev->mode_config.rotation_property) {
crtc->invert_dimensions =
!!(val & ((1LL << DRM_ROTATE_90) | (1LL <<
DRM_ROTATE_270)));
}
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
b/drivers/gpu/drm/omapdrm/omap_drv.c
index 862ba03c236c..282f5ec4f1a2 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -560,8 +560,6 @@ static int dev_open(struct drm_device *dev, struct drm_file
*file)
*/
static void dev_lastclose(struct drm_device *dev)
{
- int i;
-
/* we don't support vga-switcheroo.. so just make sure the fbdev
* mode is active
*/
@@ -570,24 +568,6 @@ static void dev_lastclose(struct drm_device *dev)
DBG("lastclose: dev=%p", dev);
- if (priv->rotation_prop) {
- /* need to restore default rotation state.. not sure
-* if there is a cleaner way to restore properties to
-* default state? Maybe a flag that properties should
-* automatically be restored to default state on
-* lastclose?
-*/
- for (i = 0; i < priv->num_crtcs; i++) {
- drm_object_property_set_value(&priv->crtcs[i]->base,
- priv->rotation_prop, 0);
- }
-
- for (i = 0; i < priv->num_planes; i++) {
- drm_object_property_set_value(&priv->planes[i]->base,
- priv->rotation_prop, 0);
- }
- }
-
ret = drm_fb_helper_restore_fbdev_mode_unlocked(priv->fbdev);
if (ret)
DBG("failed to restore crtc mode");
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
b/drivers/gpu/drm/omapdrm/omap_drv.h
index 60e47b33c801..aa596504e662 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -111,7 +111,6 @@ struct omap_drm_private {
bool has_dmm;
/* properties: */
- struct drm_property *rotation_prop;
struct drm_property *zorder_prop;
/* irq handling: */
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
b/drivers/gpu/drm/omapdrm/omap_plane.c
index 891a4dc608af..3b5fad2a359c 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -306,7 +306,7 @@ void omap_plane_install_properties(struct drm_plane *plane,
struct drm_property *prop;
if (priv->has_dmm) {
- prop = priv->rotation_prop;
+ prop = dev->mode_config.rotation_property;
if (!prop) {
prop = drm_mode_create_rotation_property(dev,
BIT(DRM_ROTATE_0) |
@@ -317,7 +317,7 @@ void omap_plane_install_properties(struct drm_plane *plane,
BIT(DRM_REFLECT_Y));
if (prop == NULL)
return;
- priv->rotation_prop = prop;
+ dev->mode_config.rotation_property = prop;
}
drm_object_attach_property(obj, prop, 0);
}
@@ -337,9 +337,10 @@ int omap_plane_set_property(struct drm_plane *plane,
{
struct omap_plane *omap_plane = to_omap_plane(plane);
struct omap_drm_private *priv = plane->dev->dev_private;
+ struct drm_device *dev = omap_plane->base.dev;
int ret = -EINVAL;
- if (property == priv->rotation_prop) {
+ if (property == dev->mode_config.rotation_property) {
DBG("%s: rotation: %02x", omap_plane->name, (uint32_t)val);
omap_plane->win.rotation = val;
ret = apply(plane);
--
2.1.1
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 by
[Bug 61269] Support libkms on FreeBSD
https://bugs.freedesktop.org/show_bug.cgi?id=61269 --- Comment #5 from emaste at freebsd.org --- What's the next step here? -- 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/ace2b7ab/attachment.html>
drm vblank fixes
Hi all, some small bug fixes for some small bugs i saw when looking at the current drm vblank handling code. All patches are rather simple, all compile-tested against drm-next, but only the drm_vblank_off() one (patch 2) tested in "real life" so far. thanks, -mario
[PATCH 1/3] drm: Remove drm_vblank_cleanup from drm_vblank_init error path.
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 --- 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
[PATCH 2/3] drm: Fix emitted vblank timestamps in drm_vblank_off()
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, &now);
+
spin_lock_irqsave(&dev->vbl_lock, irqflags);
vblank_disable_and_save(dev, crtc);
wake_up(&dev->vblank[crtc].queue);
/* Send any queued vblank events, lest the natives grow disquiet */
- seq = drm_vblank_count_and_time(dev, crtc, &now);
-
spin_lock(&dev->event_lock);
list_for_each_entry_safe(e, t, &dev->vblank_event_list, base.link) {
if (e->pipe != crtc)
--
1.9.1
[PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup()
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
---
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(&dev->vblank[crtc].disable_timer);
- vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
+ spin_lock_irqsave(&dev->vbl_lock, irqflags);
+ vblank_disable_and_save(dev, crtc);
+ spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
}
kfree(dev->vblank);
--
1.9.1
[PATCH 1/6] drm: Renaming DP training vswing/pre-emph defines
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
[Bug 81680] [r600g] Firefox crashes with hardware acceleration turned on
https://bugs.freedesktop.org/show_bug.cgi?id=81680 --- Comment #10 from Michel D?nzer --- In the crash report referenced in comment 7, frame 1 says r600_dri.so at 0x1ba0a1, so the address of frame 1 is 0x1ba0a1. -- 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/a076db01/attachment.html>
[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors
https://bugs.freedesktop.org/show_bug.cgi?id=82162 --- Comment #9 from Michel D?nzer --- You need to set a breakpoint in the function printing the messages before running glxgears. Something like b radeon_bomgr_create_bo if size == 0 should do the trick. Then get the backtrace when the breakpoint triggers. -- 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/074fd9be/attachment.html>
[Bug 61269] Support libkms on FreeBSD
https://bugs.freedesktop.org/show_bug.cgi?id=61269 --- Comment #6 from Emil Velikov --- Have either of these two made it to the mailing list [1] ? AFAIK most people do not look for and/or expect patches in bugzilla. Please use git am to generate both patches :) Thanks Emil [1] dri-devel at lists.freedesktop.org -- 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/225b5625/attachment.html>
[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors
https://bugs.freedesktop.org/show_bug.cgi?id=82162 --- Comment #10 from sarnex --- (In reply to comment #9) > You need to set a breakpoint in the function printing the messages before > running glxgears. Something like > > b radeon_bomgr_create_bo if size == 0 > > should do the trick. Then get the backtrace when the breakpoint triggers. Hi Michael, here's the backtrace from that breakpoint. Please let me know if you need anything I really want this bug fixed. http://pastebin.com/Lw6rtfg8 -- 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/b7e70e79/attachment.html>
[Bug 75241] radeon_compute_pll_avivo broken in 3.15-rc3
https://bugzilla.kernel.org/show_bug.cgi?id=75241 --- Comment #29 from Dan Merillat --- (In reply to Christian K?nig from comment #28) > Created attachment 142281 [details] > Possible fix v3. > > How about this one? Does it fixes the issue as well? Sorry for the long delay in getting back to you. 3.16 stock does not work on my monitor, this patch (alone) fixes it. I don't have a scope at my house, but at the office when this happens all signal lines on the VGA are idle. -- You are receiving this mail because: You are watching the assignee of the bug.
[pull] radeon drm-next-3.17
On Tue, Aug 5, 2014 at 7:32 PM, Christian K?nig wrote: >>> Christian wrote some patches to validate the interfaces, but I'm not sure >>> he ever sent them out. We haven't yet done a full implementation in the >>> usermode drivers to take advantage of this yet. >> >> Well right now I've consistently rejected all patches that don't yet >> come with the full thing (libdrm, usermode drivers and tests for it >> all as not ready). And I do that at least once per week since we have >> blob userspace separate from mesa, too. So if we toss that rule >> overboard (and my understanding is that Dave's been fairly strict >> here) I'll look rather bad. As in really, really bad. >> >> I strongly prefer that userptr gets postponed until it's ready. Dave? > > > That's just my fault. Wanted to wait with the mesa patches till we have the > kernel interface accepted. > > I've just send them out, Ah, seen them now, thanks. Personally I prefer if kernel and usermode parts get reviewed in parallel. But that's mostly since we have 2 separate teams. So no objection to userptr from my side any more ;-) Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
drm vblank fixes
On Wed, Aug 6, 2014 at 3:22 AM, Mario Kleiner wrote: > some small bug fixes for some small bugs i saw > when looking at the current drm vblank handling code. > > All patches are rather simple, all compile-tested against > drm-next, but only the drm_vblank_off() one (patch 2) > tested in "real life" so far. Ville has an older patch series touching the same code and afaict fixing some of the same bugs. Unfortunately it's not in yet. Have you looked at that one? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2
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.
Regards,
Christian.
>
> Cheers,
> J?r?me
>
>> Christian.
>>
>>> Cheers,
>>> J?r?me
>>>
Signed-off-by: Christian K?nig
---
drivers/gpu/drm/radeon/radeon_gem.c | 3 ++-
drivers/gpu/drm/radeon/radeon_ttm.c | 10 ++
include/uapi/drm/radeon_drm.h | 1 +
3 files changed, 13 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/radeon/radeon_gem.c
b/drivers/gpu/drm/radeon/radeon_gem.c
index 993ab22..032736b 100644
--- a/drivers/gpu/drm/radeon/radeon_gem.c
+++ b/drivers/gpu/drm/radeon/radeon_gem.c
@@ -290,7 +290,8 @@ int radeon_gem_userptr_ioctl(struct drm_device *dev,
void *data,
return -EACCES;
/* reject unknown flag values */
- if (args->flags & ~RADEON_GEM_USERPTR_READONLY)
+ if (args->flags & ~(RADEON_GEM_USERPTR_READONLY |
+ RADEON_GEM_USERPTR_ANONONLY))
return -EINVAL;
/* readonly pages not tested on older hardware */
diff --git a/drivers/gpu/drm/radeon/radeon_ttm.c
b/drivers/gpu/drm/radeon/radeon_ttm.c
index 0109090..54eb7bc 100644
--- a/drivers/gpu/drm/radeon/radeon_ttm.c
+++ b/drivers/gpu/drm/radeon/radeon_ttm.c
@@ -542,6 +542,16 @@ static int radeon_ttm_tt_pin_userptr(struct ttm_tt
*ttm)
ttm->num_pages * PAGE_SIZE))
return -EFAULT;
+ if (gtt->userflags & RADEON_GEM_USERPTR_ANONONLY) {
+ /* check that we only pin down anonymous memory
+ to prevent problems with writeback */
+ unsigned long end = gtt->userptr + ttm->num_pages * PAGE_SIZE;
+ struct vm_area_struct *vma;
+ vma = find_vma(gtt->usermm, gtt->userptr);
+ if (!vma || vma->vm_file || vma->vm_end < end)
+ return -EPERM;
+ }
+
do {
unsigned num_pages = ttm->num_pages - pinned;
uint64_t userptr = gtt->userptr + pinned * PAGE_SIZE;
diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
index 3a9f209..9720e1a 100644
--- a/include/uapi/drm/radeon_drm.h
+++ b/include/uapi/drm/radeon_drm.h
@@ -816,6 +816,7 @@ struct drm_radeon_gem_create {
* perform any operation.
*/
#define RADEON_GEM_USERPTR_READONLY (1 << 0)
+#define RADEON_GEM_USERPTR_ANONONLY (1 << 1)
struct drm_radeon_gem_userptr {
uint64_t
[PULL] DRM Cleanups
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. 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 v2 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support
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 >
[PULL] DRM Cleanups
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
[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors
https://bugs.freedesktop.org/show_bug.cgi?id=82162 --- Comment #11 from Michel D?nzer --- Created attachment 104132 --> https://bugs.freedesktop.org/attachment.cgi?id=104132&action=edit 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. -- 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/50b41cdb/attachment.html>
[PATCH v2 1/2] drm/mipi-dsi: add (LPM) Low Power Mode transfer support
al flag/quirk at that point. > 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. We already have that flag, it's called MIPI_DSI_MSG_USE_LPM. Given the above discussion I think it may still be worthwhile to invert the meaning of the flag and rename it MIPI_DSI_MSG_USE_HS, so that all messages are indeed sent in low power mode by default. Thierry -- next part -- A non-text attachment was scrubbed... Name: not available Type: application/pgp-signature Size: 819 bytes Desc: not available URL: <http://lists.freedesktop.org/archives/dri-devel/attachments/20140806/d3ed859e/attachment-0001.sig>
[PATCH] drm: Perform cmdline mode parsing during connector initialisation
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.
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/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 | 20 +++
include/drm/drm_crtc.h | 1 +
include/drm/drm_fb_helper.h| 1 -
7 files changed, 82 insertions(+), 77 deletions(-)
diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 3c4a62169f28..d3f1e0033475 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -852,6 +852,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 = &connector->cmdline_mode;
+ char *option = NULL;
+
+ if (fb_get_options(connector->name, &option))
+ 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
@@ -903,6 +956,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(&connector->head, &dev->mode_config.connector_list);
dev->mode_config.num_connector++;
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d139eddb3d61..9a91ee69af2b 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -171,60 +171,6 @@ int drm_fb_helper_remove_one_connector(struct
drm_fb_helper *fb_helper,
}
EXPORT_SYMBOL(drm_fb_helper_remove_one_connector);
-static int drm_fb_helper_parse_command_line(struct drm_fb_helper *fb_helper)
-{
- struct drm_fb_helper_connector *fb_helper_conn;
- int i;
-
- for (i = 0; i < fb_helper->connector_count; i++) {
- struct drm_cmdline_mode *mode;
- struct drm_connector *connector;
- ch
[PATCH] drm: Don't grab an fb reference for the idr
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.
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(&fb->head, &dev->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(&dev->mode_config.idr_mutex);
+ idr_remove(&dev->mode_config.crtc_idr, fb->base.id);
+ mutex_unlock(&dev->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(&dev->mode_config.fb_lock);
+ if (fb->base.id) {
+ /* Mark fb as reaped and drop idr ref. */
+ __drm_framebuffer_unregister(dev, fb);
+ }
+ mutex_unlock(&dev->mode_config.fb_lock);
+
fb->funcs->destroy(fb);
}
@@ -567,8 +588,10 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct
drm_device *dev,
mutex_lock(&dev->mode_config.fb_lock);
fb = __drm_framebuffer_lookup(dev, id);
- if (fb)
- drm_framebuffer_reference(fb);
+ if (fb) {
+ if (!kref_get_unless_zero(&fb->refcount))
+ fb = NULL;
+ }
mutex_unlock(&dev->mode_config.fb_lock);
return fb;
@@ -612,19 +635,6 @@ static void __drm_framebuffer_unreference(struct
drm_framebuffer *fb)
kref_put(&fb->refcount, drm_framebuffer_free_bug);
}
-/* dev->mode_config.fb_lock must be held! */
-static void __drm_framebuffer_unregister(struct drm_device *dev,
-struct drm_framebuffer *fb)
-{
- mutex_lock(&dev->mode_config.idr_mutex);
- idr_remove(&dev->mode_config.crtc_idr, fb->base.id);
- mutex_unlock(&dev->mode_config.idr_mutex);
-
- fb->base.id = 0;
-
- __drm_framebuffer_unreference(fb);
-}
-
/**
* drm_framebuffer_unregister_private - unregister a private fb from the
lookup idr
* @fb: fb to unregister
--
1.9.3
[Intel-gfx] [PATCH 6/6] drm: Resetting rotation property
On Wed, Aug 06, 2014 at 01:26:00PM +0530, sonika.jindal at intel.com wrote:
> 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?
I've pulled in these 2 simple drm core rotation patches into dinq with
Dave's ack for 3.18.
-Daniel
> ---
> 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, &dev->mode_config.plane_list, head)
> + list_for_each_entry(plane, &dev->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(&plane->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 =
> &fb_helper->crtc_info[i].mode_set;
> struct drm_crtc *crtc = mode_set->crtc;
> --
> 1.7.10.4
>
> ___
> 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/omap: move rotation property to drm core mode_config
This allows us to ditch the driver-private lastclose logic.
Cc: Tomi Valkeinen
Cc: Rob Clark
Signed-off-by: Daniel Vetter
--
Untested and atm only applies on top of drm-intel-nightly.
---
drivers/gpu/drm/omapdrm/omap_crtc.c | 4 ++--
drivers/gpu/drm/omapdrm/omap_drv.c | 9 ++---
drivers/gpu/drm/omapdrm/omap_drv.h | 1 -
drivers/gpu/drm/omapdrm/omap_plane.c | 6 +++---
4 files changed, 7 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
b/drivers/gpu/drm/omapdrm/omap_crtc.c
index 2d28dc337cfb..1240fa61b397 100644
--- a/drivers/gpu/drm/omapdrm/omap_crtc.c
+++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
@@ -385,9 +385,9 @@ static int omap_crtc_set_property(struct drm_crtc *crtc,
struct drm_property *property, uint64_t val)
{
struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
- struct omap_drm_private *priv = crtc->dev->dev_private;
+ struct drm_device *dev = crtc->dev;
- if (property == priv->rotation_prop) {
+ if (property == dev->mode_config.rotation_property) {
crtc->invert_dimensions =
!!(val & ((1LL << DRM_ROTATE_90) | (1LL <<
DRM_ROTATE_270)));
}
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c
b/drivers/gpu/drm/omapdrm/omap_drv.c
index 002b9721e85a..abfacca2e995 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -570,21 +570,16 @@ static void dev_lastclose(struct drm_device *dev)
DBG("lastclose: dev=%p", dev);
- if (priv->rotation_prop) {
+ if (dev->mode_config.rotation_property) {
/* need to restore default rotation state.. not sure
* if there is a cleaner way to restore properties to
* default state? Maybe a flag that properties should
* automatically be restored to default state on
* lastclose?
*/
- for (i = 0; i < priv->num_crtcs; i++) {
- drm_object_property_set_value(&priv->crtcs[i]->base,
- priv->rotation_prop, 0);
- }
-
for (i = 0; i < priv->num_planes; i++) {
drm_object_property_set_value(&priv->planes[i]->base,
- priv->rotation_prop, 0);
+ dev->mode_config.rotation_property, 0);
}
}
diff --git a/drivers/gpu/drm/omapdrm/omap_drv.h
b/drivers/gpu/drm/omapdrm/omap_drv.h
index b08a450d1b5d..0443190e7af3 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.h
+++ b/drivers/gpu/drm/omapdrm/omap_drv.h
@@ -110,7 +110,6 @@ struct omap_drm_private {
bool has_dmm;
/* properties: */
- struct drm_property *rotation_prop;
struct drm_property *zorder_prop;
/* irq handling: */
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c
b/drivers/gpu/drm/omapdrm/omap_plane.c
index 891a4dc608af..c1bcf631d04d 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -306,7 +306,7 @@ void omap_plane_install_properties(struct drm_plane *plane,
struct drm_property *prop;
if (priv->has_dmm) {
- prop = priv->rotation_prop;
+ prop = dev->mode_config.rotation_property;
if (!prop) {
prop = drm_mode_create_rotation_property(dev,
BIT(DRM_ROTATE_0) |
@@ -317,7 +317,7 @@ void omap_plane_install_properties(struct drm_plane *plane,
BIT(DRM_REFLECT_Y));
if (prop == NULL)
return;
- priv->rotation_prop = prop;
+ dev->mode_config.rotation_property = prop;
}
drm_object_attach_property(obj, prop, 0);
}
@@ -339,7 +339,7 @@ int omap_plane_set_property(struct drm_plane *plane,
struct omap_drm_private *priv = plane->dev->dev_private;
int ret = -EINVAL;
- if (property == priv->rotation_prop) {
+ if (property == dev->mode_config.rotation_property) {
DBG("%s: rotation: %02x", omap_plane->name, (uint32_t)val);
omap_plane->win.rotation = val;
ret = apply(plane);
--
2.0.1
[PATCH] video/fbdev: Always built-in video= cmdline parsing
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:
[pull] radeon drm-next-3.17
Hi Dave, Here's the radeon userptr patches. If you and Jerome are ok with them for 3.17, please pull. The following changes since commit 21d70354bba9965a098382fc4d7fb17e138111f3: drm: move drm_stub.c to drm_drv.c (2014-08-06 19:10:44 +1000) are available in the git repository at: git://people.freedesktop.org/~agd5f/linux drm-next-3.17 for you to fetch changes up to 1ddcb4f2d00479bd44008a64243457d00c9c49bf: drm/radeon: allow userptr write access under certain conditions (2014-08-06 05:33:11 -0400) Christian K?nig (5): drm/radeon: add userptr support v7 drm/radeon: add userptr flag to limit it to anonymous memory v2 drm/radeon: add userptr flag to directly validate the BO to GTT drm/radeon: add userptr flag to register MMU notifier v3 drm/radeon: allow userptr write access under certain conditions drivers/gpu/drm/Kconfig| 1 + drivers/gpu/drm/radeon/Makefile| 2 +- drivers/gpu/drm/radeon/radeon.h| 17 +++ drivers/gpu/drm/radeon/radeon_cs.c | 25 ++- drivers/gpu/drm/radeon/radeon_device.c | 2 + drivers/gpu/drm/radeon/radeon_drv.c| 5 +- drivers/gpu/drm/radeon/radeon_gem.c| 97 drivers/gpu/drm/radeon/radeon_kms.c| 1 + drivers/gpu/drm/radeon/radeon_mn.c | 272 + drivers/gpu/drm/radeon/radeon_object.c | 4 + drivers/gpu/drm/radeon/radeon_prime.c | 10 ++ drivers/gpu/drm/radeon/radeon_ttm.c| 149 ++ drivers/gpu/drm/radeon/radeon_vm.c | 3 + include/uapi/drm/radeon_drm.h | 19 +++ 14 files changed, 603 insertions(+), 4 deletions(-) create mode 100644 drivers/gpu/drm/radeon/radeon_mn.c
[Bug 18154] libdrm support for addressing >32 bits on 32 bit cores
https://bugs.freedesktop.org/show_bug.cgi?id=18154 Andrej Krutak changed: What|Removed |Added Version|XOrg CVS|unspecified Component|libdrm |DRM/other --- Comment #1 from Andrej Krutak --- This is a problem in the drm/ttm code, not libdri. -- 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/36db2b37/attachment.html>
[Bug 18154] DRM support for addressing >32 bits on 32 bit cores
https://bugs.freedesktop.org/show_bug.cgi?id=18154 Andrej Krutak changed: What|Removed |Added Summary|libdrm support for |DRM support for addressing |addressing >32 bits on 32 |>32 bits on 32 bit cores |bit cores | --- Comment #2 from Andrej Krutak --- I'm attaching a patch that resolves this problem at least with p3041 board + radeon card, where I saw similar problem (the mmio base being truncated). The issue was too small type of ttm_bus_placement.base. Also, the debug prints at the beginning only print 32 bits of the values (even if they are 64 bit). -- 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/e25750ba/attachment.html>
[Bug 18154] DRM support for addressing >32 bits on 32 bit cores
https://bugs.freedesktop.org/show_bug.cgi?id=18154 --- Comment #3 from Andrej Krutak --- Created attachment 104143 --> https://bugs.freedesktop.org/attachment.cgi?id=104143&action=edit Fix ttm_bus_placement.base type to accomodate >32 bit physical addresses -- 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/169d2c35/attachment.html>
[PATCH] video/fbdev: Always built-in video= cmdline parsing
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
[Bug 79980] Random radeonsi crashes
https://bugs.freedesktop.org/show_bug.cgi?id=79980 --- Comment #91 from agapito --- Created attachment 104145 --> https://bugs.freedesktop.org/attachment.cgi?id=104145&action=edit dmesg output drm-next-3.17 Using drm-next-3.17 my xorg crashes. I don't know if is the same bug. But here is my dmesg output (sorry about the quality) -- 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/8ef3b3b0/attachment.html>
[PATCH 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup()
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(&dev->vblank[crtc].disable_timer);
> - vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
> + spin_lock_irqsave(&dev->vbl_lock, irqflags);
> + vblank_disable_and_save(dev, crtc);
> + spin_unlock_irqrestore(&dev->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 1/3] drm: Remove drm_vblank_cleanup from drm_vblank_init error path.
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 2/3] drm: Fix emitted vblank timestamps in drm_vblank_off()
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, &now);
> +
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(&dev->vbl_lock, irqflags);
> vblank_disable_and_save(dev, crtc);
> wake_up(&dev->vblank[crtc].queue);
>
> /* Send any queued vblank events, lest the natives grow disquiet */
> - seq = drm_vblank_count_and_time(dev, crtc, &now);
> -
> spin_lock(&dev->event_lock);
> list_for_each_entry_safe(e, t, &dev->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 2/2] drm/i915: Free pending page flip events at .preclose()
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(&dev->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(&dev->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(&dev->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
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(&file_priv->prime); + WARN_ON(!list_empty(&file_priv->event_list)); + put_pid(file_priv->pid); kfree(file_priv); -- 1.8.5.5
[PATCH] drm: Don't grab an fb reference for the idr
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?
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(&fb->head, &dev->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(&dev->mode_config.idr_mutex);
> + idr_remove(&dev->mode_config.crtc_idr, fb->base.id);
> + mutex_unlock(&dev->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(&dev->mode_config.fb_lock);
> + if (fb->base.id) {
> + /* Mark fb as reaped and drop idr ref. */
> + __drm_framebuffer_unregister(dev, fb);
> + }
> + mutex_unlock(&dev->mode_config.fb_lock);
> +
> fb->funcs->destroy(fb);
> }
>
> @@ -567,8 +588,10 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct
> drm_device *dev,
>
> mutex_lock(&dev->mode_config.fb_lock);
> fb = __drm_framebuffer_lookup(dev, id);
> - if (fb)
> - drm_framebuffer_reference(fb);
> + if (fb) {
> + if (!kref_get_unless_zero(&fb->refcount))
> + fb = NULL;
> + }
> mutex_unlock(&dev->mode_config.fb_lock);
>
> return fb;
> @@ -612,19 +635,6 @@ static void __drm_framebuffer_unreference(struct
> drm_framebuffer *fb)
> kref_put(&fb->refcount, drm_framebuffer_free_bug);
> }
>
> -/* dev->mode_config.fb_lock must be held! */
> -static void __drm_framebuffer_unregister(struct drm_device *dev,
> -struct drm_framebuffer *fb)
> -{
> - mutex_lock(&dev->mode_config.idr_mutex);
> - idr_remove(&dev->mode_config.crtc_idr, fb->base.id);
> - mutex_unlock(&dev->mode_config.idr_mutex);
> -
> - fb->base.id = 0;
> -
> - __drm_framebuffer_unreference(fb);
> -}
> -
> /**
> * drm_framebuffer_unregister_private - unregister a private fb from
[PATCH v2 00/19] drm: More vblank on/off work
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 v2 01/19] drm: Always reject drm_vblank_get() after drm_vblank_off()
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(&dev->event_lock);
+ /*
+* Prevent subsequent drm_vblank_get() from re-enabling
+* the vblank interrupt by bumping the refcount.
+*/
+ if (!dev->vblank[crtc].inmodeset) {
+ atomic_inc(&dev->vblank[crtc].refcount);
+ dev->vblank[crtc].inmodeset = 1;
+ }
+
spin_unlock_irqrestore(&dev->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(&dev->vbl_lock, irqflags);
+ /* Drop our private "prevent drm_vblank_get" refcount */
+ if (dev->vblank[crtc].inmodeset) {
+ atomic_dec(&dev->vblank[crtc].refcount);
+ dev->vblank[crtc].inmodeset = 0;
+ }
/* re-enable interrupts if there's are users left */
if (atomic_read(&dev->vblank[crtc].refcount) != 0)
WARN_ON(drm_vblank_enable(dev, crtc));
--
1.8.5.5
[PATCH v2 02/19] drm/i915: Warn if drm_vblank_get() still works after drm_vblank_off()
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 03/19] drm: Don't clear vblank timestamps when vblank interrupt is disabled
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(&dev->vblank_time_lock, irqflags);
}
--
1.8.5.5
[PATCH 04/19] drm: Move drm_update_vblank_count()
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, &t_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(&dev->vblank[crtc].count) + diff;
+ vblanktimestamp(dev, crtc, tslot) = t_vblank;
+ }
+
+ smp_mb__before_atomic();
+ atomic_add(diff, &dev->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, &t_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
[PATCH 05/19] drm: Have the vblank counter account for the time between vblank irq disable and drm_vblank_off()
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(&dev->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(&dev->vblank_time_lock, irqflags);
+ return;
+ }
+
dev->driver->disable_vblank(dev, crtc);
dev->vblank[crtc].enabled = false;
--
1.8.5.5
[PATCH v2 07/19] drm: Reduce the amount of dev->vblank[crtc] in the code
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 = &dev->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 = &dev->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 = &dev->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 = &dev->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(&dev->vblank[crtc].count) + diff;
+ tslot = atomic_read(&vblank->count) + diff;
vblanktimestamp(dev, crtc, tslot) = t_vblank;
}
smp_mb__before_atomic();
- atomic_add(diff, &dev->vblank[crtc].count);
+ atomic_add(diff, &vblank->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 = &dev->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(&dev->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, &tvblank, 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(&dev->vblank[crtc].count);
+ vblcount = atomic_read(&vblank->count);
[PATCH 09/19] drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()
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 = &dev->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(&dev->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 v2 10/19] drm: Disable vblank interrupt immediately when drm_vblank_offdelay<0
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(&vblank->refcount) == 0);
/* Last user schedules interrupt disable */
- if (atomic_dec_and_test(&vblank->refcount) &&
- (drm_vblank_offdelay > 0))
- mod_timer(&vblank->disable_timer,
- jiffies + ((drm_vblank_offdelay * HZ)/1000));
+ if (atomic_dec_and_test(&vblank->refcount)) {
+ if (drm_vblank_offdelay < 0)
+ vblank_disable_fn((unsigned long)vblank);
+ else if (drm_vblank_offdelay > 0)
+ mod_timer(&vblank->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 08/19] drm: Fix deadlock between event_lock and vbl_lock/vblank_time_lock
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(&dev->vbl_lock, irqflags);
+ spin_lock_irqsave(&dev->event_lock, irqflags);
+
+ spin_lock(&dev->vbl_lock);
vblank_disable_and_save(dev, crtc);
wake_up(&vblank->queue);
+ /*
+* Prevent subsequent drm_vblank_get() from re-enabling
+* the vblank interrupt by bumping the refcount.
+*/
+ if (!vblank->inmodeset) {
+ atomic_inc(&vblank->refcount);
+ vblank->inmodeset = 1;
+ }
+ spin_unlock(&dev->vbl_lock);
+
/* Send any queued vblank events, lest the natives grow disquiet */
seq = drm_vblank_count_and_time(dev, crtc, &now);
- spin_lock(&dev->event_lock);
list_for_each_entry_safe(e, t, &dev->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, &now);
}
- spin_unlock(&dev->event_lock);
-
- /*
-* Prevent subsequent drm_vblank_get() from re-enabling
-* the vblank interrupt by bumping the refcount.
-*/
- if (!vblank->inmodeset) {
- atomic_inc(&vblank->refcount);
- vblank->inmodeset = 1;
- }
-
- spin_unlock_irqrestore(&dev->vbl_lock, irqflags);
+ spin_unlock_irqrestore(&dev->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, &now);
+ assert_spin_locked(&dev->event_lock);
- spin_lock_irqsave(&dev->event_lock, flags);
+ seq = drm_vblank_count_and_time(dev, crtc, &now);
list_for_each_entry_safe(e, t, &dev->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, &now);
}
- spin_unlock_irqrestore(&dev->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(&dev->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(&dev->vblank_time_lock, irqflags);
+ spin_lock(&dev->vblank_time_lock);
/* Vblank irq handling disabled. Nothing to do. */
if (!vblank->enabled) {
- spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
+ spin_unlock(&dev->vblank_time_lock);
+ spin_unlock_irqrestore(&dev->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(&dev->vblank_time_lock);
+
wake_up(&vblank->queue);
drm_handle_vblank_events(dev, crtc);
- spin_unlock_irqrestore(&dev->vblank_time_lock, irqflags);
+ spin_unlock_irqrestore(&dev->event_loc
[PATCH 06/19] drm: Avoid random vblank counter jumps if the hardware counter has been reset
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(&dev->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(&dev->vblank[crtc].refcount) != 0) WARN_ON(drm_vblank_enable(dev, crtc)); -- 1.8.5.5
[PATCH v2 11/19] drm: Add dev->vblank_disable_immediate flag
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(&vblank->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(&vblank->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 13/19] drm: Kick start vblank interrupts at drm_vblank_on()
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(&vblank->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(&vblank->refcount) != 0 || + (!dev->vblank_disable_immediate && drm_vblank_offdelay == 0)) WARN_ON(drm_vblank_enable(dev, crtc)); spin_unlock_irqrestore(&dev->vbl_lock, irqflags); } -- 1.8.5.5
[PATCH 14/19] drm: Don't update vblank timestamp when the counter didn't change
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 16/19] drm: Store the vblank timestamp when adjusting the counter during disable
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(&vblank->count);
smp_mb__after_atomic();
}
--
1.8.5.5
[PATCH 12/19] drm/i915: Opt out of vblank disable timer on >gen2
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 15/19] drm: Update vblank->last in drm_update_vblank_count()
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 17/19] drm/i915: Clear .last vblank count before drm_vblank_off() when sanitizing crtc state
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 igt] tests/kms_flip: Assert that vblank timestamps aren't zeroed
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, &wait_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()
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
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] video/fbdev: Always built-in video= cmdline parsing
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)
re
[Bug 42878] black flash every time pm changes clocks
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] drm: Don't grab an fb reference for the idr
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(&fb->head, &dev->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(&dev->mode_config.idr_mutex);
> > + idr_remove(&dev->mode_config.crtc_idr, fb->base.id);
> > + mutex_unlock(&dev->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(&dev->mode_config.fb_lock);
> > + if (fb->base.id) {
> > + /* Mark fb as reaped and drop idr ref. */
> > + __drm_framebuffer_unregister(dev, fb);
> > + }
> > + mutex_unlock(&dev->mode_config.fb_lock);
> > +
> > fb->funcs->destroy(fb);
> > }
> >
> > @@ -567,8 +588,10 @@ struct drm_framebuffer *drm_framebuffer_lookup(struct
> > drm_device *dev,
> >
> > mutex_lock(&dev->mode_config.fb_lock);
> > fb = __drm_framebuffer_lookup(dev, id);
> > - if (fb)
> > - drm_framebuffer_reference(fb);
> > + if (fb) {
> > + if (!kref_get_unless_zero(&fb->refcount))
> > + fb = NULL;
> > + }
> > mutex_unlock(&dev->mode_config.fb_lock);
> >
> > r
[PATCH] video/fbdev: Always built-in video= cmdline parsing
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
[Intel-gfx] [PATCH 2/2] drm/i915: Free pending page flip events at .preclose()
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(&dev->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(&dev->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(&dev->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 1/2] video/fbdev: Always built-in video= cmdline parsing
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.
+ *
[PATCH 2/2] drm: Perform cmdline mode parsing during connector initialisation
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 = &connector->cmdline_mode;
+ char *option = NULL;
+
+ if (fb_get_options(connector->name, &option))
+ 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(&connector->head, &dev->mode_config.connector_list);
dev->mode_config.num_connector++;
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_f
[Intel-gfx] [PATCH 14/19] drm: Don't update vblank timestamp when the counter didn't change
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
[Intel-gfx] [PATCH 15/19] drm: Update vblank->last in drm_update_vblank_count()
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
[Intel-gfx] [PATCH 14/19] drm: Don't update vblank timestamp when the counter didn't change
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
[Intel-gfx] [PATCH 16/19] drm: Store the vblank timestamp when adjusting the counter during disable
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(&vblank->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
[PATCH] drm: Don't grab an fb reference for the idr
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?
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
BR,
-R
> -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(&fb->head, &dev->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(&dev->mode_config.idr_mutex);
>> > + idr_remove(&dev->mode_config.crtc_idr, fb->base.id);
>> > + mutex_unlock(&dev->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(&dev->mode_config.fb_lock);
>> > + if (fb->base.id) {
>> > + /* Mark fb as reaped and drop idr ref. */
>> > + __drm_framebuffer_unregister(dev, fb);
>> > + }
>> > + mutex_unlock(&dev->mode_config.fb_lock);
>> > +
>> > fb->funcs->destroy(fb);
>> > }
>> >
>>
[PATCH igt] tests: Add kms_flip_event_leak test
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(&data->display);
+
+ if (!output->valid) {
+ igt_output_set_pipe(output, PIPE_ANY);
+ igt_display_commit(&data->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, &fb[0]);
+
+ igt_plane_set_fb(primary, &fb[0]);
+ igt_display_commit2(&data->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, &fb[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(&data->display);
+
+ igt_remove_fb(data->drm_fd, &fb[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.display, data.drm_fd);
+
+ for
[Intel-gfx] [PATCH 09/19] drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()
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 = &dev->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(&dev->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
[Intel-gfx] [PATCH 17/19] drm/i915: Clear .last vblank count before drm_vblank_off() when sanitizing crtc state
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
[Intel-gfx] [PATCH 15/19] drm: Update vblank->last in drm_update_vblank_count()
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 09/19] drm: Fix race between drm_vblank_off() and drm_queue_vblank_event()
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 = &dev->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(&dev->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
[Intel-gfx] [PATCH 17/19] drm/i915: Clear .last vblank count before drm_vblank_off() when sanitizing crtc state
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 3/3] drm: Use vblank_disable_and_save in drm_vblank_cleanup()
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(&dev->vblank[crtc].disable_timer);
> > - vblank_disable_fn((unsigned long)&dev->vblank[crtc]);
> > + spin_lock_irqsave(&dev->vbl_lock, irqflags);
> > + vblank_disable_and_save(dev, crtc);
> > + spin_unlock_irqrestore(&dev->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
[PATCH] drm: Don't grab an fb reference for the idr
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
[Intel-gfx] [PATCH 2/2] drm/i915: Free pending page flip events at .preclose()
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(&dev->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(&dev->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(&dev->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
[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors
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>
[Intel-gfx] [PATCH 15/19] drm: Update vblank->last in drm_update_vblank_count()
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
[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors
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>
[Bug 82253] New: JanusVR Browser rendering misses floors on radeonsi, works on intel
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>
[Bug 82201] [HAWAII] GPU doesn't reclock, poor 3D performance
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 82162] Syslog flooded by [drm:radeon_gem_object_create] errors
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>
[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors
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>
[Bug 81680] [r600g] Firefox crashes with hardware acceleration turned on
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 82201] [HAWAII] GPU doesn't reclock, poor 3D performance
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 82162] Syslog flooded by [drm:radeon_gem_object_create] errors
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>
[PATCH 4/5] drm/radeon: add userptr flag to register MMU notifier v3
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(&rdev->pm.mclk_lock);
> init_rwsem(&rdev->exclusive_lock);
> init_waitqueue_head(&rdev->irq.vblank_queue);
> + mutex_init(&rdev->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 rele
[PATCH 4/5] drm/radeon: add userptr flag to register MMU notifier v3
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(&rdev->pm.mclk_lock);
>> init_rwsem(&rdev->exclusive_lock);
>> init_waitqueue_head(&rdev->irq.vblank_queue);
>> +mutex_init(&rdev->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 |
>> -RAD
[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors
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>
[Bug 82162] Syslog flooded by [drm:radeon_gem_object_create] errors
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>
[Bug 82201] [HAWAII] GPU doesn't reclock, poor 3D performance
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>
[PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2
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] drm: idiot-proof vblank
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(&dev->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(&dev->vbl_lock, irqflags);
/* Going from 0->1 means we have to enable interrupts again */
if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
@@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
{
BUG_ON(atomic_read(&dev->vblank[crtc].refcount) == 0);
+ if (WARN_ON(crtc >= dev->num_crtcs))
+ return;
+
/* Last user schedules interrupt disable */
if (atomic_dec_and_test(&dev->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(&dev->vbl_lock, irqflags);
vblank_disable_and_save(dev, crtc);
wake_up(&dev->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(&dev->vbl_lock, irqflags);
/* re-enable interrupts if there's are users left */
if (atomic_read(&dev->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
[PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2
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
[PATCH libdrm] drm: Implement drmCheckModesettingSupported() for DragonFly
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] drm: idiot-proof vblank
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(&dev->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(&dev->vbl_lock, irqflags);
> /* Going from 0->1 means we have to enable interrupts again */
> if (atomic_add_return(1, &dev->vblank[crtc].refcount) == 1) {
> @@ -975,6 +983,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc)
> {
> BUG_ON(atomic_read(&dev->vblank[crtc].refcount) == 0);
>
> + if (WARN_ON(crtc >= dev->num_crtcs))
> + return;
> +
> /* Last user schedules interrupt disable */
> if (atomic_dec_and_test(&dev->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(&dev->vbl_lock, irqflags);
> vblank_disable_and_save(dev, crtc);
> wake_up(&dev->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(&dev->vbl_lock, irqflags);
> /* re-enable interrupts if there's are users left */
> if (atomic_read(&dev->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
[PATCH 2/5] drm/radeon: add userptr flag to limit it to anonymous memory v2
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 > >
