[PATCH] imx-drm: imx-drm-core: add suspend/resume support

[Shawn Guo]

On Thu, Jul 24, 2014 at 12:38:59PM +0200, Daniel Vetter wrote:
> > > > +static int imx_drm_suspend(struct device *dev)
> > > > +{
> > > > +   struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > > +   struct drm_connector *connector;
> > > > +
> > > > +   drm_kms_helper_poll_disable(drm_dev);
> > > > +
> > > 
> > > That's ok.
> > > 
> > > > +   drm_modeset_lock_all(drm_dev);
> > > > +   list_for_each_entry(connector, 
> > > > _dev->mode_config.connector_list, head) {
> > > > +   if (connector->funcs->dpms)
> > > > +   connector->funcs->dpms(connector, 
> > > > DRM_MODE_DPMS_OFF);
> > > > +   }
> > > 
> > > Don't touch DPMS state here. See below.
> > 
> > In which case, how else does the hardware get placed into a low power
> > mode on suspend?
> > 
> > DRM has nothing provided, and this is left up to each DRM driver to
> > implement (probably because it tends to be very driver specific.)
> > i915 for example calls the CRTC DPMS function with DRM_MODE_DPMS_OFF.
> > 
> > This provides a similar mechanism, but also informs the connector, any
> > bridge, and encoder associated with the connector as well as the CRTC
> > to place themselves into a low power mode (which is what
> > DRM_MODE_DPMS_OFF should be doing anyway.)
> 
> Well you need to call internal functions to make sure you can restore the
> state again. Not sure any more how that all works with the crtc helpers
> and whether they restore dpms state properly at all. i915 uses something
> completely different nowadays.

Is it okay to do what exynos driver does, i.e. saving/restoring the dpms
state before/after calling connector .dpms function?

list_for_each_entry(connector, _dev->mode_config.connector_list, 
head) {
int old_dpms = connector->dpms;

if (connector->funcs->dpms)
connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);

/* Set the old mode back to the connector for resume */
connector->dpms = old_dpms;
}

Shawn

[PATCH] gpu: drm: omapdrm: Use %pad format for values of the dma_addr_t type

[mat...@sai.msu.ru]

From: "Matwey V. Kornilov" 

The format change is to fix the following compilation issue:

../drivers/gpu/drm/omapdrm/omap_plane.c: In function 'omap_plane_pre_apply':
../drivers/gpu/drm/omapdrm/omap_plane.c:145:2: error: format '%x' expects 
argument of type 'unsigned int', but argument 5 has type 'dma_addr_t' 
[-Werror=format=]
  DBG("%d,%d %08x %08x", info->pos_x, info->pos_y,
  ^
../drivers/gpu/drm/omapdrm/omap_plane.c:145:2: error: format '%x' expects 
argument of type 'unsigned int', but argument 6 has type 'dma_addr_t' 
[-Werror=format=]
cc1: all warnings being treated as errors
../scripts/Makefile.build:273: recipe for target 
'drivers/gpu/drm/omapdrm/omap_plane.o' failed

Signed-off-by: Matwey V. Kornilov 
---
 drivers/gpu/drm/omapdrm/omap_gem.c   | 10 +-
 drivers/gpu/drm/omapdrm/omap_plane.c |  4 ++--
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c 
b/drivers/gpu/drm/omapdrm/omap_gem.c
index 95dbce2..44305c6 100644
--- a/drivers/gpu/drm/omapdrm/omap_gem.c
+++ b/drivers/gpu/drm/omapdrm/omap_gem.c
@@ -791,7 +791,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
omap_obj->paddr = tiler_ssptr(block);
omap_obj->block = block;

-   DBG("got paddr: %08x", omap_obj->paddr);
+   DBG("got paddr: %pad", &(omap_obj->paddr));
}

omap_obj->paddr_cnt++;
@@ -985,9 +985,9 @@ void omap_gem_describe(struct drm_gem_object *obj, struct 
seq_file *m)

off = drm_vma_node_start(>vma_node);

-   seq_printf(m, "%08x: %2d (%2d) %08llx %08Zx (%2d) %p %4d",
+   seq_printf(m, "%08x: %2d (%2d) %08llx %pad (%2d) %p %4d",
omap_obj->flags, obj->name, 
obj->refcount.refcount.counter,
-   off, omap_obj->paddr, omap_obj->paddr_cnt,
+   off, &(omap_obj->paddr), omap_obj->paddr_cnt,
omap_obj->vaddr, omap_obj->roll);

if (omap_obj->flags & OMAP_BO_TILED) {
@@ -1467,8 +1467,8 @@ void omap_gem_init(struct drm_device *dev)
entry->paddr = tiler_ssptr(block);
entry->block = block;

-   DBG("%d:%d: %dx%d: paddr=%08x stride=%d", i, j, w, h,
-   entry->paddr,
+   DBG("%d:%d: %dx%d: paddr=%pad stride=%d", i, j, w, h,
+   &(entry->paddr),
usergart[i].stride_pfn << PAGE_SHIFT);
}
}
diff --git a/drivers/gpu/drm/omapdrm/omap_plane.c 
b/drivers/gpu/drm/omapdrm/omap_plane.c
index 3cf31ee..aaa9a2c 100644
--- a/drivers/gpu/drm/omapdrm/omap_plane.c
+++ b/drivers/gpu/drm/omapdrm/omap_plane.c
@@ -142,8 +142,8 @@ static void omap_plane_pre_apply(struct omap_drm_apply 
*apply)
DBG("%dx%d -> %dx%d (%d)", info->width, info->height,
info->out_width, info->out_height,
info->screen_width);
-   DBG("%d,%d %08x %08x", info->pos_x, info->pos_y,
-   info->paddr, info->p_uv_addr);
+   DBG("%d,%d %pad %pad", info->pos_x, info->pos_y,
+   &(info->paddr), &(info->p_uv_addr));

/* TODO: */
ilace = false;
-- 
1.8.1.4

[PATCH v2 00/25] AMDKFD kernel driver

[Oded Gabbay]

On 24/07/14 21:47, Jerome Glisse wrote:
> On Thu, Jul 24, 2014 at 01:35:53PM -0400, Alex Deucher wrote:
>> On Thu, Jul 24, 2014 at 11:44 AM, Jerome Glisse  
>> wrote:
>>> On Thu, Jul 24, 2014 at 01:01:41AM +0300, Oded Gabbay wrote:
 On 24/07/14 00:46, Bridgman, John wrote:
>
>> -Original Message- From: dri-devel
>> [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of Jesse
>> Barnes Sent: Wednesday, July 23, 2014 5:00 PM To:
>> dri-devel at lists.freedesktop.org Subject: Re: [PATCH v2 00/25]
>> AMDKFD kernel driver
>>
>> On Mon, 21 Jul 2014 19:05:46 +0200 daniel at ffwll.ch (Daniel
>> Vetter) wrote:
>>
>>> On Mon, Jul 21, 2014 at 11:58:52AM -0400, Jerome Glisse wrote:
 On Mon, Jul 21, 2014 at 05:25:11PM +0200, Daniel Vetter wrote:
> On Mon, Jul 21, 2014 at 03:39:09PM +0200, Christian K?nig
> wrote:
>> Am 21.07.2014 14:36, schrieb Oded Gabbay:
>>> On 20/07/14 20:46, Jerome Glisse wrote:
>>
>> [snip!!]
> My BlackBerry thumb thanks you ;)
>>
>>
>> The main questions here are if it's avoid able to pin down
>> the memory and if the memory is pinned down at driver load,
>> by request from userspace or by anything else.
>>
>> As far as I can see only the "mqd per userspace queue"
>> might be a bit questionable, everything else sounds
>> reasonable.
>
> Aside, i915 perspective again (i.e. how we solved this):
> When scheduling away from contexts we unpin them and put them
> into the lru. And in the shrinker we have a last-ditch
> callback to switch to a default context (since you can't ever
> have no context once you've started) which means we can evict
> any context object if it's
>> getting in the way.

 So Intel hardware report through some interrupt or some channel
 when it is not using a context ? ie kernel side get
 notification when some user context is done executing ?
>>>
>>> Yes, as long as we do the scheduling with the cpu we get
>>> interrupts for context switches. The mechanic is already
>>> published in the execlist patches currently floating around. We
>>> get a special context switch interrupt.
>>>
>>> But we have this unpin logic already on the current code where
>>> we switch contexts through in-line cs commands from the kernel.
>>> There we obviously use the normal batch completion events.
>>
>> Yeah and we can continue that going forward.  And of course if your
>> hw can do page faulting, you don't need to pin the normal data
>> buffers.
>>
>> Usually there are some special buffers that need to be pinned for
>> longer periods though, anytime the context could be active.  Sounds
>> like in this case the userland queues, which makes some sense.  But
>> maybe for smaller systems the size limit could be clamped to
>> something smaller than 128M.  Or tie it into the rlimit somehow,
>> just like we do for mlock() stuff.
>>
> Yeah, even the queues are in pageable memory, it's just a ~256 byte
> structure per queue (the Memory Queue Descriptor) that describes the
> queue to hardware, plus a couple of pages for each process using HSA
> to hold things like doorbells. Current thinking is to limit #
> processes using HSA to ~256 and #queues per process to ~1024 by
> default in the initial code, although my guess is that we could take
> the #queues per process default limit even lower.
>

 So my mistake. struct cik_mqd is actually 604 bytes, and it is allocated
 on 256 boundary.
 I had in mind to reserve 64MB of gart by default, which translates to
 512 queues per process, with 128 processes. Add 2 kernel module
 parameters, # of max-queues-per-process and # of max-processes (default
 is, as I said, 512 and 128) for better control of system admin.

>>>
>>> So as i said somewhere else in this thread, this should not be reserved
>>> but use a special allocation. Any HSA GPU use virtual address space for
>>> userspace so only issue is for kernel side GTT.
>>>
>>> What i would like is seeing radeon pinned GTT allocation at bottom of
>>> GTT space (ie all ring buffer and the ib pool buffer). Then have an
>>> allocator that allocate new queue from top of GTT address space and
>>> grow to the bottom.
>>>
>>> It should not staticly reserved 64M or anything. When doing allocation
>>> it should move any ttm buffer that are in the region it want to allocate
>>> to a different location.
>>>
>>>
>>> As this needs some work, i am not against reserving some small amount
>>> (couple MB) as a first stage but anything more would need a proper solution
>>> like the one i just described.
>>
>> It's still a trade off.  Even if we reserve a couple of megs it'll be
>> wasted if we are not running HSA apps. 

[PATCH 2/2] drm/radeon: add new info ioctl query for hawaii accel

[Alex Deucher]

We need to make sure we have a new enough firmware with
the fixed nop packet.

Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/radeon/radeon_kms.c | 6 ++
 include/uapi/drm/radeon_drm.h   | 1 +
 2 files changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/radeon/radeon_kms.c 
b/drivers/gpu/drm/radeon/radeon_kms.c
index 35d9318..3ea9f59 100644
--- a/drivers/gpu/drm/radeon/radeon_kms.c
+++ b/drivers/gpu/drm/radeon/radeon_kms.c
@@ -529,6 +529,12 @@ static int radeon_info_ioctl(struct drm_device *dev, void 
*data, struct drm_file
else
*value = 1;
break;
+   case RADEON_INFO_HAWAII_ACCEL:
+   if (rdev->new_fw)
+   *value = 1;
+   else
+   *value = 0;
+   break;
default:
DRM_DEBUG_KMS("Invalid request %d\n", info->request);
return -EINVAL;
diff --git a/include/uapi/drm/radeon_drm.h b/include/uapi/drm/radeon_drm.h
index 509b2d7..a41dec3 100644
--- a/include/uapi/drm/radeon_drm.h
+++ b/include/uapi/drm/radeon_drm.h
@@ -1010,6 +1010,7 @@ struct drm_radeon_cs {
 #define RADEON_INFO_VRAM_USAGE 0x1e
 #define RADEON_INFO_GTT_USAGE  0x1f
 #define RADEON_INFO_ACTIVE_CU_COUNT0x20
+#define RADEON_INFO_HAWAII_ACCEL   0x21

 struct drm_radeon_info {
uint32_trequest;
-- 
1.8.3.1

[PATCH 1/2] drm/radeon: fix cut and paste issue for hawaii.

[Alex Deucher]

From: Jerome Glisse 

This is a halfway fix for hawaii acceleration. More fixes to come
but hopefully isolated to userspace.

Signed-off-by: J?r?me Glisse 
Cc: stable at vger.kernel.org
Signed-off-by: Alex Deucher 
---
 drivers/gpu/drm/radeon/cik.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
index b5e1ce3..44d25da 100644
--- a/drivers/gpu/drm/radeon/cik.c
+++ b/drivers/gpu/drm/radeon/cik.c
@@ -2515,6 +2515,7 @@ static void cik_tiling_mode_table_init(struct 
radeon_device *rdev)
gb_tile_moden = 0;
break;
}
+   rdev->config.cik.macrotile_mode_array[reg_offset] = 
gb_tile_moden;
WREG32(GB_MACROTILE_MODE0 + (reg_offset * 4), 
gb_tile_moden);
}
} else if (num_pipe_configs == 8) {
-- 
1.8.3.1

[PATCH] [rfc] drm: close race in connector registration

[Dave Airlie]

>
> This is the same we do for minor-objects, so fine with me. I'd prefer
> if the registration is done last in drm_connector_register(), not
> first, but I'm not sure the debugfs hooks work without the connector
> available in the lookup tables. Maybe worth a try.

I was worried sysfs registration would trigger something in userspace
to life, which wuold then fail because the connector wasn't there.

Dave.

[Bug 81683] [r600g] graphic glitches in Skyrim - failed to build shader

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

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

--- Comment #3 from 640bugs at gmail.com ---
Created attachment 103405
  --> https://bugs.freedesktop.org/attachment.cgi?id=103405=edit
increase GPR limit up to 128 in r600 driver

-- 
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/20140724/61f12cff/attachment.html>

[Bug 81683] [r600g] graphic glitches in Skyrim - failed to build shader

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

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

--- Comment #2 from 640bugs at gmail.com ---
Created attachment 103403
  --> https://bugs.freedesktop.org/attachment.cgi?id=103403=edit
Screenshot of skyrim with patch

I manually increased the GPR limit up to 128, which solves the problem as you
can see in the screenshot. Of course I am not sure, if this is a valid patch
and doesnt cause other problems.
I also attached 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/20140724/378e754b/attachment.html>

[PATCH] gpio: extend gpiod_get*() with flags parameter

[Laurent Pinchart]

Hi Alexandre,

Thank you for the patch.

On Friday 25 July 2014 00:04:58 Alexandre Courbot wrote:
> The huge majority of GPIOs have their direction and initial value set
> right after being obtained by one of the gpiod_get() functions. The
> integer GPIO API had gpio_request_one() that took a convenience flags
> parameter allowing to specify an direction and value applied to the
> returned GPIO. This feature greatly simplifies client code and ensures
> errors are always handled properly.
> 
> A similar feature has been requested for the gpiod API. Since GPIOs need
> a direction to be used anyway, we prefer to extend the existing
> functions instead of introducing new functions that would raise the
> number of gpiod getters to 16 (!).
> 
> The drawback of this approach is that all gpiod clients need to be
> updated, but there aren't that many and the moment and this results in
> smaller (and hopefully safer) code.
> 
> Signed-off-by: Alexandre Courbot 
> ---
> This change will be difficult to apply without breaking things, but
> let's try to do it right. Hopefully the benefit will outweight the
> disturbance.
> 
> This is a patch against -next to list and update all current gpiod
> consumers. Updates are trivial at first sight, but it would be nice to
> get as many acks as possible from the respective subsystem maintainers.
> 
> I'm not sure how this could be applied harmlessly though - maybe through
> a dedicated branch for -next? Problem is that a lot of new code is not
> yet merged into mainline, and conflicts are very likely to occur. Linus,
> do you have any suggestion as to how this can be done without blood being
> spilled?
> 
>  Documentation/gpio/consumer.txt| 26 ---
>  drivers/gpio/devres.c  | 24 ++
>  drivers/gpio/gpiolib.c | 53 +--
>  drivers/gpu/drm/panel/panel-ld9040.c   |  7 +--
>  drivers/gpu/drm/panel/panel-s6e8aa0.c  |  7 +--
>  drivers/gpu/drm/panel/panel-simple.c   | 16 ++-
>  drivers/hsi/clients/nokia-modem.c  |  7 +--
>  drivers/i2c/muxes/i2c-mux-pca954x.c|  4 +-
>  drivers/iio/accel/kxcjk-1013.c |  6 +--
>  drivers/input/keyboard/clps711x-keypad.c   |  6 +--
>  drivers/input/misc/gpio-beeper.c   |  6 +--
>  drivers/input/misc/soc_button_array.c  |  2 +-
>  drivers/media/i2c/adv7604.c|  6 +--
>  drivers/mfd/intel_soc_pmic_core.c  |  2 +-
>  drivers/mmc/core/slot-gpio.c   |  6 +--
>  drivers/net/phy/at803x.c   |  4 +-
>  drivers/power/reset/gpio-poweroff.c| 21 ++---
>  drivers/tty/serial/serial_mctrl_gpio.c |  2 +-
>  drivers/video/backlight/pwm_bl.c   |  6 +--
>  drivers/video/fbdev/omap2/displays-new/panel-dpi.c | 12 ++---
>  .../omap2/displays-new/panel-lgphilips-lb035q02.c  |  6 +--
>  .../omap2/displays-new/panel-sharp-ls037v7dw01.c   |  7 +--
>  include/linux/gpio/consumer.h  | 38 
>  net/rfkill/rfkill-gpio.c   | 16 ++-
>  sound/soc/codecs/adau1977.c| 11 ++---
>  sound/soc/codecs/cs4265.c  |  5 +-
>  sound/soc/codecs/sta350.c  |  9 ++--
>  sound/soc/codecs/tas2552.c |  4 +-
>  sound/soc/jz4740/qi_lb60.c | 10 +---
>  sound/soc/omap/rx51.c  | 29 +++-
>  sound/soc/soc-jack.c   |  9 ++--
>  31 files changed, 160 insertions(+), 207 deletions(-)
> 
> diff --git a/Documentation/gpio/consumer.txt
> b/Documentation/gpio/consumer.txt index 7ff30d2..a3fb1d7 100644
> --- a/Documentation/gpio/consumer.txt
> +++ b/Documentation/gpio/consumer.txt
> @@ -29,13 +29,24 @@ gpiod_get() functions. Like many other kernel
> subsystems, gpiod_get() takes the device that will use the GPIO and the
> function the requested GPIO is supposed to fulfill:
> 
> - struct gpio_desc *gpiod_get(struct device *dev, const char *con_id)
> + struct gpio_desc *gpiod_get(struct device *dev, const char *con_id,
> + enum gpio_flags flags)

I assume you mean enum gpiod_flags here and in the rest of the documentation.

>  If a function is implemented by using several GPIOs together (e.g. a simple
> LED device that displays digits), an additional index argument can be
> specified:
> 
>   struct gpio_desc *gpiod_get_index(struct device *dev,
> -   const char *con_id, unsigned int idx)
> +   const char *con_id, unsigned int idx,
> +   enum gpio_flags flags)
> +
> +The flags parameter is used to optionally specify a direction and initial
> value +for the GPIO. Values can be:
> 

[PATCH] gpio: extend gpiod_get*() with flags parameter

[Arnd Bergmann]

On Friday 25 July 2014 00:04:58 Alexandre Courbot wrote:
> I'm not sure how this could be applied harmlessly though - maybe through
> a dedicated branch for -next? Problem is that a lot of new code is not
> yet merged into mainline, and conflicts are very likely to occur. Linus,
> do you have any suggestion as to how this can be done without blood being
> spilled?

There is a trick that we sometime use in this situation,
though it has to be done carefully:

> diff --git a/Documentation/gpio/consumer.txt b/Documentation/gpio/consumer.txt
> index 7ff30d2..a3fb1d7 100644
> --- a/Documentation/gpio/consumer.txt
> +++ b/Documentation/gpio/consumer.txt
> @@ -29,13 +29,24 @@ gpiod_get() functions. Like many other kernel subsystems, 
> gpiod_get() takes the
>  device that will use the GPIO and the function the requested GPIO is 
> supposed to
>  fulfill:
>  
> -   struct gpio_desc *gpiod_get(struct device *dev, const char *con_id)
> +   struct gpio_desc *gpiod_get(struct device *dev, const char *con_id,
> +   enum gpio_flags flags)
>  
> 


-   struct gpio_desc *gpiod_get(struct device *dev, const char *con_id)
+   struct gpio_desc *__gpiod_get(struct device *dev, const char *con_id,
+   enum gpio_flags flags);
+
+#define __gpiod_get(dev, con_id, flags, ...) __gpiod_get(dev, con_id, flags)
+#define gpiod_get(varargs ...) __gpiod_get(varargs, 0)

This will allow both variants to be called, and any users of the three-argument
version will pass zero as the fourth argument (or whatever you choose there).

Once the conversion is complete, the macros can be removed.

ARnd

[PATCH] imx-drm: imx-drm-core: add suspend/resume support

[Shawn Guo]

HDMI currently stops working after a system suspend/resume cycle.  It
turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get
called from anywhere across suspend/resume cycle.

The patch follows what exynos drm driver does to walk the list of
connectors and call their .dpms function from suspend/resume hook.  And
the connectors' .dpms function will in turn filter down to the .dpms
hooks of encoders and CRTCs.

With this change, HDMI can continue working after a suspend/resume
cycle.

Signed-off-by: Shawn Guo 
---
Tested with HDMI and LVDS.  It'd be great if someone can help test TVE
to ensure the patch doesn't break anything.

 drivers/staging/imx-drm/imx-drm-core.c | 39 ++
 1 file changed, 39 insertions(+)

diff --git a/drivers/staging/imx-drm/imx-drm-core.c 
b/drivers/staging/imx-drm/imx-drm-core.c
index def8280d7ee6..b0ea1f0ed32f 100644
--- a/drivers/staging/imx-drm/imx-drm-core.c
+++ b/drivers/staging/imx-drm/imx-drm-core.c
@@ -696,6 +696,44 @@ static int imx_drm_platform_remove(struct platform_device 
*pdev)
return 0;
 }

+#if CONFIG_PM_SLEEP
+static int imx_drm_suspend(struct device *dev)
+{
+   struct drm_device *drm_dev = dev_get_drvdata(dev);
+   struct drm_connector *connector;
+
+   drm_kms_helper_poll_disable(drm_dev);
+
+   drm_modeset_lock_all(drm_dev);
+   list_for_each_entry(connector, _dev->mode_config.connector_list, 
head) {
+   if (connector->funcs->dpms)
+   connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
+   }
+   drm_modeset_unlock_all(drm_dev);
+
+   return 0;
+}
+
+static int imx_drm_resume(struct device *dev)
+{
+   struct drm_device *drm_dev = dev_get_drvdata(dev);
+   struct drm_connector *connector;
+
+   drm_modeset_lock_all(drm_dev);
+   list_for_each_entry(connector, _dev->mode_config.connector_list, 
head) {
+   if (connector->funcs->dpms)
+   connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
+   }
+   drm_modeset_unlock_all(drm_dev);
+
+   drm_kms_helper_poll_enable(drm_dev);
+
+   return 0;
+}
+#endif
+
+static SIMPLE_DEV_PM_OPS(imx_drm_pm_ops, imx_drm_suspend, imx_drm_resume);
+
 static const struct of_device_id imx_drm_dt_ids[] = {
{ .compatible = "fsl,imx-display-subsystem", },
{ /* sentinel */ },
@@ -708,6 +746,7 @@ static struct platform_driver imx_drm_pdrv = {
.driver = {
.owner  = THIS_MODULE,
.name   = "imx-drm",
+   .pm = _drm_pm_ops,
.of_match_table = imx_drm_dt_ids,
},
 };
-- 
1.9.1

[PATCH] drm/radeon: fix cut and paste issue for hawaii.

[j.gli...@gmail.com]

From: Jerome Glisse 

This is a halfway fix for hawaii acceleration. More fixes to come
but hopefully isolated to userspace.

Signed-off-by: J?r?me Glisse 
Cc: stable at vger.kernel.org
---
 drivers/gpu/drm/radeon/cik.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/radeon/cik.c b/drivers/gpu/drm/radeon/cik.c
index fc560b0..2b87edb 100644
--- a/drivers/gpu/drm/radeon/cik.c
+++ b/drivers/gpu/drm/radeon/cik.c
@@ -2521,6 +2521,7 @@ static void cik_tiling_mode_table_init(struct 
radeon_device *rdev)
gb_tile_moden = 0;
break;
}
+   rdev->config.cik.macrotile_mode_array[reg_offset] = 
gb_tile_moden;
WREG32(GB_MACROTILE_MODE0 + (reg_offset * 4), 
gb_tile_moden);
}
} else if (num_pipe_configs == 8) {
-- 
1.8.3.1

[PATCH 1/1] Revert "drm/i915: drop i915_ prefix from enable_rc6, enable_fbc, enable_ppgtt parameters"

[Amit Shah]

On (Thu) 17 Jul 2014 [14:50:12], Amit Shah wrote:
> On (Thu) 17 Jul 2014 [11:11:15], Daniel Vetter wrote:
> > On Thu, Jul 17, 2014 at 02:32:41PM +0530, Amit Shah wrote:
> > > On (Thu) 17 Jul 2014 [09:35:20], Daniel Vetter wrote:
> > > > On Wed, Jul 16, 2014 at 9:54 PM, Linus Torvalds
> > > >  wrote:
> > > > > Sorry for the top post, I'm on the road..
> > > > >
> > > > > In wondering if we couldn't just keep both the old an the new names 
> > > > > and have
> > > > > them both point at the same variable? Remove the description for the 
> > > > > old
> > > > > name, but keep it working?
> > > > 
> > > > I'm really surprised here ... We have rc6 enabled by default
> > > > everywhere, and all the additional rc6 levels that users try to enable
> > > > are known to hard-hang machines.
> > > 
> > > I haven't had this problem on my hardware (ThinkPad T420s, lspci
> > > below) for a few kernel versions.  I think I added the enable_rc6=
> > > setting back from the time the deeper states were enabled and then
> > > reverted for SandyBridge.
> > > 
> > > Nevertheless, with the current state, RC6p and RC6pp states are not
> > > used.
> > 
> > Yeah, on snb they cause crashes and instability and also don't provide
> > measurable power benefits (afaik). So I recommend you drop that one.
> 
> Not for me -- there have been no crashes / hangs / lockups as I
> mentioned.
> 
> > > > I actually have plans to taint the
> > > > kernel if you set any of them since I'm fed up with the random crash
> > > > reports. Same for fbc, even more so or the ppgtt knob. My stance is
> > > > that if you know about these knobs you _really_ should know the driver
> > > > to its depths and so also be able to follow module parameter
> > > > renamings.
> > > 
> > > I also remember there being bugzillas about power consumption, and
> > > using this setting was recommended (for Fedora, I think).  I know
> > > a few people are using this setting.
> > 
> > I know, google is littered with such entries. Unfortunately by the time
> > google thinks something is important (which usually takes a few months)
> > it's already badly outdated: i915 graphics developement is charging ahead
> > at a really brisk pace - we merge a few hundred patches per release for
> > i915 alone.
> 
> But for SNB, there's really no "improvement" for the RC6 states, is
> there?
> 
> > > > > On Jul 16, 2014 8:34 AM, "Amit Shah"  wrote:
> > > > >>
> > > > >> This reverts commit 3adee7a7976012a20f1d3b5a529a3c105e29fef1.
> > > > >>
> > > > >> After upgrading to v3.15, my laptop's battery started draining quite
> > > > >> fast.  Powertop pointed to the deep RC6 states not being used.  The
> > > > >> kernel param I had put to enable them had stopped working the way it
> > > > >> used to; so I disagree with the 'not maintaing ABI' part of the param
> > > > >> name change.
> > > > >>
> > > > >> However weird the names may be, they're in active use and changing 
> > > > >> them
> > > > >> only causes pain for users.  This also isn't advertised (marked
> > > > >> deprecated, big warning shown, etc.), so just reverting now.
> > > > >>
> > > > >> CC: Daniel Vetter 
> > > > >> CC: Jani Nikula 
> > > > >> CC: David Airlie 
> > > > >> CC:  # v3.15+
> > > > >> Signed-off-by: Amit Shah 
> > > > 
> > > > Anyway we need to figure out what went wrong here. Please share your
> > > > exact kernelcmdline and lspci -nn. Also stats for before/after from
> > > > powertop when idle please.
> > > 
> > > Powertop stats for idle are a little difficult -- since this is my
> > > primary laptop.
> > 
> > Now I'm a bit confused: How have you measured that the lack of rc6p/pp is
> > the reason for your power consumption regression?
> > -Daniel
> 
> What I meant was rebooting in the middle of something is a pain
> (usually a week or two between trying these things); and also for a
> fair comparison, the workloads have to be similar for both the
> powertop ratings.

Attached are the two powertop runs with 'powertop --html'.  Both are
taken on a fresh reboot on the same kernel, just the diff

[PATCH v2 00/25] AMDKFD kernel driver

[Jerome Glisse]

On Thu, Jul 24, 2014 at 09:57:16PM +0300, Oded Gabbay wrote:
> On 24/07/14 21:47, Jerome Glisse wrote:
> > On Thu, Jul 24, 2014 at 01:35:53PM -0400, Alex Deucher wrote:
> >> On Thu, Jul 24, 2014 at 11:44 AM, Jerome Glisse  
> >> wrote:
> >>> On Thu, Jul 24, 2014 at 01:01:41AM +0300, Oded Gabbay wrote:
>  On 24/07/14 00:46, Bridgman, John wrote:
> >
> >> -Original Message- From: dri-devel
> >> [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of Jesse
> >> Barnes Sent: Wednesday, July 23, 2014 5:00 PM To:
> >> dri-devel at lists.freedesktop.org Subject: Re: [PATCH v2 00/25]
> >> AMDKFD kernel driver
> >>
> >> On Mon, 21 Jul 2014 19:05:46 +0200 daniel at ffwll.ch (Daniel
> >> Vetter) wrote:
> >>
> >>> On Mon, Jul 21, 2014 at 11:58:52AM -0400, Jerome Glisse wrote:
>  On Mon, Jul 21, 2014 at 05:25:11PM +0200, Daniel Vetter wrote:
> > On Mon, Jul 21, 2014 at 03:39:09PM +0200, Christian K?nig
> > wrote:
> >> Am 21.07.2014 14:36, schrieb Oded Gabbay:
> >>> On 20/07/14 20:46, Jerome Glisse wrote:
> >>
> >> [snip!!]
> > My BlackBerry thumb thanks you ;)
> >>
> >>
> >> The main questions here are if it's avoid able to pin down
> >> the memory and if the memory is pinned down at driver load,
> >> by request from userspace or by anything else.
> >>
> >> As far as I can see only the "mqd per userspace queue"
> >> might be a bit questionable, everything else sounds
> >> reasonable.
> >
> > Aside, i915 perspective again (i.e. how we solved this):
> > When scheduling away from contexts we unpin them and put them
> > into the lru. And in the shrinker we have a last-ditch
> > callback to switch to a default context (since you can't ever
> > have no context once you've started) which means we can evict
> > any context object if it's
> >> getting in the way.
> 
>  So Intel hardware report through some interrupt or some channel
>  when it is not using a context ? ie kernel side get
>  notification when some user context is done executing ?
> >>>
> >>> Yes, as long as we do the scheduling with the cpu we get
> >>> interrupts for context switches. The mechanic is already
> >>> published in the execlist patches currently floating around. We
> >>> get a special context switch interrupt.
> >>>
> >>> But we have this unpin logic already on the current code where
> >>> we switch contexts through in-line cs commands from the kernel.
> >>> There we obviously use the normal batch completion events.
> >>
> >> Yeah and we can continue that going forward.  And of course if your
> >> hw can do page faulting, you don't need to pin the normal data
> >> buffers.
> >>
> >> Usually there are some special buffers that need to be pinned for
> >> longer periods though, anytime the context could be active.  Sounds
> >> like in this case the userland queues, which makes some sense.  But
> >> maybe for smaller systems the size limit could be clamped to
> >> something smaller than 128M.  Or tie it into the rlimit somehow,
> >> just like we do for mlock() stuff.
> >>
> > Yeah, even the queues are in pageable memory, it's just a ~256 byte
> > structure per queue (the Memory Queue Descriptor) that describes the
> > queue to hardware, plus a couple of pages for each process using HSA
> > to hold things like doorbells. Current thinking is to limit #
> > processes using HSA to ~256 and #queues per process to ~1024 by
> > default in the initial code, although my guess is that we could take
> > the #queues per process default limit even lower.
> >
> 
>  So my mistake. struct cik_mqd is actually 604 bytes, and it is allocated
>  on 256 boundary.
>  I had in mind to reserve 64MB of gart by default, which translates to
>  512 queues per process, with 128 processes. Add 2 kernel module
>  parameters, # of max-queues-per-process and # of max-processes (default
>  is, as I said, 512 and 128) for better control of system admin.
> 
> >>>
> >>> So as i said somewhere else in this thread, this should not be reserved
> >>> but use a special allocation. Any HSA GPU use virtual address space for
> >>> userspace so only issue is for kernel side GTT.
> >>>
> >>> What i would like is seeing radeon pinned GTT allocation at bottom of
> >>> GTT space (ie all ring buffer and the ib pool buffer). Then have an
> >>> allocator that allocate new queue from top of GTT address space and
> >>> grow to the bottom.
> >>>
> >>> It should not staticly reserved 64M or anything. When doing allocation
> >>> it should move any ttm buffer that are in the region it want to allocate
> >>> to a different location.
> >>>
> >>>
> >>> As this needs some work, i am not against 

[PATCH 0/8] tilcdc-panel: Backlight and GPIO devicetree support

[Darren Etheridge]

On 07/11/2014 09:18 AM, Ezequiel Garcia wrote:
> Hello all,
>
> This patchset adds the required changes to support an optional backlight
> and GPIO for the tilcdc panel driver.
>
> There was some code to support a backlight, but it was somewhat broken
> and undocumented. I've followed the nice implementation in panel-simple
> and added a similar one here.
>
> The enable GPIO is required to turn on and off devices with such capability.
> Also here, I've followed panel-simple which looks correct.
>
> In addition to this there are very minor cosmetic cleanups and a larger
> error path fix in tilcdc's DRM driver .load error path.
>
> This patchset applies on top of drm-next branch which contains the latest
> tilcdc pushed by Guido.
>
> If at all possible, I'd like to get this merged for v3.17. If a pull request
> is needed, don't hesitate to ask and I'll prepare one.
>
> Comments and tests welcome!
>

All of the changes seem to make sense and I tested on AM335x-EVM both 
with and without the addition "backlight = " in the dts for the panel node.

I see no issues in either case, continued to work as before.

Tested against 3.16-rc6 with this patchset and the earlier patchset from 
Guido applied.

Also tested on BeagleBone Black even though it doesn't have a panel, 
just to make sure nothing changed there.

For the series:
Tested-by: Darren Etheridge 

> Ezequiel Garcia (8):
>drm/tilcdc: Fix the error path in tilcdc_load()
>drm/tilcdc: panel: Add missing of_node_put
>drm/tilcdc: panel: Remove unused variable
>drm/tilcdc: panel: Spurious whitespace removal
>drm/tilcdc: panel: Use devm_kzalloc to simplify the error path
>drm/tilcdc: panel: Fix backlight devicetree support
>drm/tilcdc: panel: Set return value explicitly
>drm/tilcdc: panel: Add support for enable GPIO
>
>   .../devicetree/bindings/drm/tilcdc/panel.txt   |  7 ++
>   drivers/gpu/drm/tilcdc/tilcdc_drv.c| 60 +++---
>   drivers/gpu/drm/tilcdc/tilcdc_panel.c  | 74 
> +-
>   3 files changed, 114 insertions(+), 27 deletions(-)
>

[RESEND PATCH V5 01/12] drm/exynos: Move DP setup out of hotplug workqueue

[Sean Paul]

On Wed, Jul 23, 2014 at 11:18 AM, Ajay kumar  wrote:
> On Wed, Jul 23, 2014 at 8:12 PM, Sean Paul  wrote:
>> On Wed, Jul 23, 2014 at 7:22 AM, Ajay kumar  wrote:
>>> Sean,
>>>
>>> On Tue, Jul 22, 2014 at 8:29 PM, Sean Paul  wrote:
 On Thu, Jul 17, 2014 at 4:43 PM, Ajay Kumar  
 wrote:
> Move the DP training and video enable from the hotplug handler into
> a seperate function and call the same during dpms ON.
>
> With existing code, DP HPD should be generated just few ms before
> calling enable_irq in dp_poweron.
>
> This patch removes that stringent time constraint.
>
> Signed-off-by: Ajay Kumar 


 This looks eerily familiar to:
 https://chromium-review.googlesource.com/#/c/65782/

 In fact, I think it's probably better to do this in commit, rather than 
 poweron.

 Sean
>>> Your are right. This patch contains a subset of changes from the patch
>>> you have mentioned.
>>> But, If I provide commit calback to dp, then it would cause dp to
>>> reinitialize twice
>>> in quick successions - during dpms_on and during commit.
>>> For the user, it would look like a glitch. So, I chose not to provide
>>> a commit callback.
>>>
>>
>> Interesting. At least in my testing, the absence of the commit
>> callback causes my simple test app (which just does drmModeSetCrtc) to
>> fail. In this case, by fail, I mean that the screen just shows black.
>>
>> Sean
> I tried running modetest. It is running fine.
> Also, exynos_drm_encoder_commit calls both dpms_on and
> commit for the encoder. So, there should be no problem I guess!
>
> Your testapp stops working only after adding this patch?


Sorry for taking a while to get back to you, I didn't get the chance
to test this until now.

Yes, my testapp stops working after adding this patch (but works with
the patch which does it in commit).

Sean



>
> Ajay
>

> ---
>  drivers/gpu/drm/exynos/exynos_dp_core.c |   11 ++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_dp_core.c 
> b/drivers/gpu/drm/exynos/exynos_dp_core.c
> index 845d766..a94b114 100644
> --- a/drivers/gpu/drm/exynos/exynos_dp_core.c
> +++ b/drivers/gpu/drm/exynos/exynos_dp_core.c
> @@ -875,10 +875,18 @@ static irqreturn_t exynos_dp_irq_handler(int irq, 
> void *arg)
>  static void exynos_dp_hotplug(struct work_struct *work)
>  {
> struct exynos_dp_device *dp;
> -   int ret;
>
> dp = container_of(work, struct exynos_dp_device, hotplug_work);
>
> +   if (dp->drm_dev)
> +   drm_helper_hpd_irq_event(dp->drm_dev);
> +}
> +
> +static void exynos_dp_setup(void *in_ctx)
> +{
> +   struct exynos_dp_device *dp = in_ctx;
> +   int ret;
> +
> ret = exynos_dp_detect_hpd(dp);
> if (ret) {
> /* Cable has been disconnected, we're done */
> @@ -1059,6 +1067,7 @@ static void exynos_dp_poweron(struct 
> exynos_dp_device *dp)
> exynos_dp_phy_init(dp);
> exynos_dp_init_dp(dp);
> enable_irq(dp->irq);
> +   exynos_dp_setup(dp);
>  }
>
>  static void exynos_dp_poweroff(struct exynos_dp_device *dp)
> --
> 1.7.9.5
>

[PATCH 0/7] prepare for atomic.. the great propertyification

[Daniel Vetter]

On Thu, Jul 24, 2014 at 09:36:20AM -0400, Rob Clark wrote:
> On Thu, Jul 24, 2014 at 8:25 AM, Daniel Vetter  wrote:
> > On Wed, Jul 23, 2014 at 03:38:13PM -0400, Rob Clark wrote:
> >> This is mostly just a rebase+resend.  Was going to send it a bit earlier
> >> but had a few things to fix up as a result of the rebase.
> >>
> >> At this point, I think next steps are roughly:
> >> 1) introduce plane->mutex
> >> 2) decide what we want to do about events
> >> 3) add actual ioctl
> >>
> >> I think we could shoot for merging this series next, and then adding
> >> plane->mutex in 3.18?
> >>
> >> Before we add the ioctl, I think we want to sort out events for updates
> >> to non-primary layers, and what the interface to drivers should look like.
> >> Ie. just add event to ->update_plane() or should we completely ditch
> >> ->page_flip() and ->update_plane()?
> >>
> >> Technically, I think we could get away without a new API and just let
> >> drivers grab all the events in their ->atomic_commit(), but I suspect
> >> core could provide something more useful to drivers.  I guess it would
> >> be useful to have a few more drivers converted over to see what makes
> >> sense.
> >
> > Ok so we've had a lot of discussions about this on irc and overall I'm not
> > terribly happy with what this looks like. I have a few concerns:
> >
> > - The entire atomic conversion for drivers is monolithic, at least at the
> >   interface level. So if you want to do this step-by-step you're forced to
> >   add hacks and e.g. bypass pageflips until that's implemented. E.g. on
> >   i915 I see no chance that we'll get atomic ready for all cases (so
> >   nonblocking flips and multi-crtc and everything on all platforms) in one
> >   go.
> 
> So, there interface is *intended* to be monolithic, in a way..  many
> years ago, I started by adding side by side atomic vs legacy paths,
> and it was pretty ugly in core.  I'm much happier with the current
> iteration.
> 
> A slightly later iteration preserved atomic helpers (so drivers could
> do completely their own thing).  I ended up dropping that because most
> of what atomic does is manage the interface to whatever is doing
> modeset/pageflip/whatever.  I completely expect some changes around
> the commit stuff..  that part is intended to either be
> wrapped/extended by the driver or replaced.  Possibly it could be made
> more clear what drivers are expected to use vs extend or replace.  I
> expect this to evolve a bit as more drivers are converted.
> 
> Note that the current atomic "layer" results in 1:1 conversion from
> old userspace API to legacy vfuncs.  This way what is on top of atomic
> API and what is beneath (ie. the driver) has same code paths either
> way (old or new userspace, converted or not driver).  The edge cases
> don't come in until atomic ioctl is exposed, and for that I expect a
> driver flag to enable the new ioctl.  You could even set the flag
> based on hw generation if you want.
> 
> > - Related to that is that we force legacy ioctls through atomic. E.g. on
> >   older i915 platforms I very much expect that we won't ever convert the
> >   pageflip code to atomic for them and just not support atomic flips of
> >   cursor + primary plane. At least not at the beginning. I know that's
> >   different from my stance a year ago, but I've become a bit more
> >   realistic.
> >
> > - The entire conversion is monolithic and we can't do it on a
> >   driver-by-driver basis. Everyone has to go through the new atomic
> >   interfaces on a flag day. drm is much bigger now and forcing everyone to
> >   convert at the same time is really risky. Again I know I've changed my
> >   mind again, but drm is a lot bigger and there's a lot more drivers that
> >   implement pageflip and cursors, i.e. stuff that's special.
> 
> the only flag day part is plugging in atomic functions and couple
> vfunc API changes around set_prop().. the current design allows for
> conversion on driver by driver basis.
> 
> > - The separation between interface marshalling code in the drm core and
> >   helper functions for drivers to implement stuff is fuzzy at best. Thus
> >   far we've had an excellent seperation in this are for kms, I don't think
> >   it's vise to throw that out.
> 
> Interface marshalling should not be helper.  Everyone needs the same
> thing, since what is on top is the same.

Erhm I've certainly not meant the interface marshalling to be a helper.
This started with "The separation ..." after all, so I want inteface
marshilling as fixed code in the drm core, and everything else outside in
a separate drm_atomic_helper.c module.

> > So here's my proposal for how to get this in without breaking the world
> >
> > 1. We add a complete new set of ->atomic_foo driver entry points to all
> > relevant objects. So instead of changing all the set_prop functions in a
> > flag-day like operation we just add a new interface. I haven't double
> > checked, but I guess that means per-obj ->atomic_set_prop 

[PATCH RFA 2/2] drm/i2c: tda998x: add component support

[Russell King]

Add component helper support to the tda998x driver.  This permits the
TDA998x to be declared as a separate device in device tree, and bound
at the appropriate moment with a co-operating card driver.

The existing slave_encoder interfaces are kept while there are existing
users of it in order to prevent regressions.

Signed-off-by: Russell King 
---
RFA = request for acks, but tested-by's are also welcome, particularly
for tilcdc hardware.

 drivers/gpu/drm/i2c/tda998x_drv.c | 212 +++---
 1 file changed, 198 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 64e120c5299a..3b07ff7e6348 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -15,8 +15,7 @@
  * this program.  If not, see .
  */

-
-
+#include 
 #include 
 #include 
 #include 
@@ -1252,18 +1251,6 @@ static struct drm_encoder_slave_funcs 
tda998x_encoder_slave_funcs = {

 /* I2C driver functions */

-static int
-tda998x_probe(struct i2c_client *client, const struct i2c_device_id *id)
-{
-   return 0;
-}
-
-static int
-tda998x_remove(struct i2c_client *client)
-{
-   return 0;
-}
-
 static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 {
struct device_node *np = client->dev.of_node;
@@ -1412,6 +1399,203 @@ static int tda998x_encoder_init(struct i2c_client 
*client,
return 0;
 }

+struct tda998x_priv2 {
+   struct tda998x_priv base;
+   struct drm_encoder encoder;
+   struct drm_connector connector;
+};
+
+#define conn_to_tda998x_priv2(x) \
+   container_of(x, struct tda998x_priv2, connector);
+
+#define enc_to_tda998x_priv2(x) \
+   container_of(x, struct tda998x_priv2, encoder);
+
+static void tda998x_encoder2_dpms(struct drm_encoder *encoder, int mode)
+{
+   struct tda998x_priv2 *priv = enc_to_tda998x_priv2(encoder);
+
+   tda998x_encoder_dpms(>base, mode);
+}
+
+static void tda998x_encoder_prepare(struct drm_encoder *encoder)
+{
+   tda998x_encoder2_dpms(encoder, DRM_MODE_DPMS_OFF);
+}
+
+static void tda998x_encoder_commit(struct drm_encoder *encoder)
+{
+   tda998x_encoder2_dpms(encoder, DRM_MODE_DPMS_ON);
+}
+
+static void tda998x_encoder2_mode_set(struct drm_encoder *encoder,
+ struct drm_display_mode *mode,
+ struct drm_display_mode *adjusted_mode)
+{
+   struct tda998x_priv2 *priv = enc_to_tda998x_priv2(encoder);
+
+   tda998x_encoder_mode_set(>base, mode, adjusted_mode);
+}
+
+static const struct drm_encoder_helper_funcs tda998x_encoder_helper_funcs = {
+   .dpms = tda998x_encoder2_dpms,
+   .save = tda998x_encoder_save,
+   .restore = tda998x_encoder_restore,
+   .mode_fixup = tda998x_encoder_mode_fixup,
+   .prepare = tda998x_encoder_prepare,
+   .commit = tda998x_encoder_commit,
+   .mode_set = tda998x_encoder2_mode_set,
+};
+
+static void tda998x_encoder_destroy(struct drm_encoder *encoder)
+{
+   struct tda998x_priv2 *priv = enc_to_tda998x_priv2(encoder);
+
+   tda998x_destroy(>base);
+   drm_encoder_cleanup(encoder);
+}
+
+static const struct drm_encoder_funcs tda998x_encoder_funcs = {
+   .destroy = tda998x_encoder_destroy,
+};
+
+static int tda998x_connector_get_modes(struct drm_connector *connector)
+{
+   struct tda998x_priv2 *priv = conn_to_tda998x_priv2(connector);
+
+   return tda998x_encoder_get_modes(>base, connector);
+}
+
+static int tda998x_connector_mode_valid(struct drm_connector *connector,
+   struct drm_display_mode *mode)
+{
+   struct tda998x_priv2 *priv = conn_to_tda998x_priv2(connector);
+
+   return tda998x_encoder_mode_valid(>base, mode);
+}
+
+static struct drm_encoder *
+tda998x_connector_best_encoder(struct drm_connector *connector)
+{
+   struct tda998x_priv2 *priv = conn_to_tda998x_priv2(connector);
+
+   return >encoder;
+}
+
+static
+const struct drm_connector_helper_funcs tda998x_connector_helper_funcs = {
+   .get_modes = tda998x_connector_get_modes,
+   .mode_valid = tda998x_connector_mode_valid,
+   .best_encoder = tda998x_connector_best_encoder,
+};
+
+static enum drm_connector_status
+tda998x_connector_detect(struct drm_connector *connector, bool force)
+{
+   struct tda998x_priv2 *priv = conn_to_tda998x_priv2(connector);
+
+   return tda998x_encoder_detect(>base);
+}
+
+static void tda998x_connector_destroy(struct drm_connector *connector)
+{
+   drm_sysfs_connector_remove(connector);
+   drm_connector_cleanup(connector);
+}
+
+static const struct drm_connector_funcs tda998x_connector_funcs = {
+   .dpms = drm_helper_connector_dpms,
+   .fill_modes = drm_helper_probe_single_connector_modes,
+   .detect = tda998x_connector_detect,
+   .destroy = tda998x_connector_destroy,
+};
+
+static int 

[PATCH RFA 1/2] drm/i2c: tda998x: allow re-use of tda998x support code

[Russell King]

Re-jig the TDA998x code so that we separate the functionality from the
drm slave encoder implementation.  In several places, this is pretty
clearly the correct thing to do, because we can avoid repetitively
having to convert from the drm_encoder to the TDA998x private
structure, particularly with the driver internal functions.

The main motivation behind this change is to allow the code to be
re-used with a standard drm_encoder and drm_connector implementation
based on the component helpers, rather than the slave_encoder system.
The addition of this will be in the following patch.

We keep the slave_encoder interface as there are existing users of
this; we need to give them time to convert and test.

Signed-off-by: Russell King 
---
RFA = request for acks, but tested-by's would also be great,
particularly from the tilcdc folk.

 drivers/gpu/drm/i2c/tda998x_drv.c | 179 +++---
 1 file changed, 110 insertions(+), 69 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c 
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 5a738ad0c241..64e120c5299a 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -730,12 +730,9 @@ tda998x_configure_audio(struct tda998x_priv *priv,

 /* DRM encoder functions */

-static void
-tda998x_encoder_set_config(struct drm_encoder *encoder, void *params)
+static void tda998x_encoder_set_config(struct tda998x_priv *priv,
+  const struct tda998x_encoder_params *p)
 {
-   struct tda998x_priv *priv = to_tda998x_priv(encoder);
-   struct tda998x_encoder_params *p = params;
-
priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
(p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
VIP_CNTRL_0_SWAP_B(p->swap_b) |
@@ -752,11 +749,8 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, 
void *params)
priv->params = *p;
 }

-static void
-tda998x_encoder_dpms(struct drm_encoder *encoder, int mode)
+static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode)
 {
-   struct tda998x_priv *priv = to_tda998x_priv(encoder);
-
/* we only care about on or off: */
if (mode != DRM_MODE_DPMS_ON)
mode = DRM_MODE_DPMS_OFF;
@@ -806,9 +800,8 @@ tda998x_encoder_mode_fixup(struct drm_encoder *encoder,
return true;
 }

-static int
-tda998x_encoder_mode_valid(struct drm_encoder *encoder,
- struct drm_display_mode *mode)
+static int tda998x_encoder_mode_valid(struct tda998x_priv *priv,
+ struct drm_display_mode *mode)
 {
if (mode->clock > 15)
return MODE_CLOCK_HIGH;
@@ -820,11 +813,10 @@ tda998x_encoder_mode_valid(struct drm_encoder *encoder,
 }

 static void
-tda998x_encoder_mode_set(struct drm_encoder *encoder,
-   struct drm_display_mode *mode,
-   struct drm_display_mode *adjusted_mode)
+tda998x_encoder_mode_set(struct tda998x_priv *priv,
+struct drm_display_mode *mode,
+struct drm_display_mode *adjusted_mode)
 {
-   struct tda998x_priv *priv = to_tda998x_priv(encoder);
uint16_t ref_pix, ref_line, n_pix, n_line;
uint16_t hs_pix_s, hs_pix_e;
uint16_t vs1_pix_s, vs1_pix_e, vs1_line_s, vs1_line_e;
@@ -1012,20 +1004,16 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
 }

 static enum drm_connector_status
-tda998x_encoder_detect(struct drm_encoder *encoder,
- struct drm_connector *connector)
+tda998x_encoder_detect(struct tda998x_priv *priv)
 {
-   struct tda998x_priv *priv = to_tda998x_priv(encoder);
uint8_t val = cec_read(priv, REG_CEC_RXSHPDLEV);

return (val & CEC_RXSHPDLEV_HPD) ? connector_status_connected :
connector_status_disconnected;
 }

-static int
-read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
+static int read_edid_block(struct tda998x_priv *priv, uint8_t *buf, int blk)
 {
-   struct tda998x_priv *priv = to_tda998x_priv(encoder);
uint8_t offset, segptr;
int ret, i;

@@ -1079,10 +1067,8 @@ read_edid_block(struct drm_encoder *encoder, uint8_t 
*buf, int blk)
return 0;
 }

-static uint8_t *
-do_get_edid(struct drm_encoder *encoder)
+static uint8_t *do_get_edid(struct tda998x_priv *priv)
 {
-   struct tda998x_priv *priv = to_tda998x_priv(encoder);
int j, valid_extensions = 0;
uint8_t *block, *new;
bool print_bad_edid = drm_debug & DRM_UT_KMS;
@@ -1094,7 +1080,7 @@ do_get_edid(struct drm_encoder *encoder)
reg_clear(priv, REG_TX4, TX4_PD_RAM);

/* base block fetch */
-   if (read_edid_block(encoder, block, 0))
+   if (read_edid_block(priv, block, 0))
goto fail;

if (!drm_edid_block_valid(block, 0, print_bad_edid))
@@ -,7 +1097,7 @@ do_get_edid(struct 

[Nouveau] [PATCH 09/17] drm/radeon: use common fence implementation for fences

[Christian König]

Hi Maarten,

try to implement this as a replacement for specifying the 
RADEON_FENCE_JIFFIES_TIMEOUT on wait_event_*. And reset the timeout 
every time radeon_fence_process is called and not only when any of the 
sequences increment.

I don't have the time right now to look deeper into it or help with the 
patch, but the general approach sounds valid to me.

Regards,
Christian.

Am 23.07.2014 16:05, schrieb Maarten Lankhorst:
> op 23-07-14 15:16, Maarten Lankhorst schreef:
>> op 23-07-14 14:36, Christian K?nig schreef:
>>> Am 23.07.2014 12:52, schrieb Daniel Vetter:
 On Wed, Jul 23, 2014 at 12:13 PM, Christian K?nig
  wrote:
>> And the dma-buf would still have fences belonging to both drivers, and it
>> would still call from outside the driver.
> Calling from outside the driver is fine as long as the driver can do
> everything necessary to complete it's work and isn't forced into any ugly
> hacks and things that are not 100% reliable.
>
> So I don't see much other approach as integrating recovery code for not
> firing interrupts and some kind of lockup handling into the fence code as
> well.
 That approach doesn't really work at that well since every driver has
 it's own reset semantics. And we're trying to move away from global
 reset to fine-grained reset. So stop-the-world reset is out of
 fashion, at least for i915. As you said, reset is normal in gpus and
 we're trying to make reset less invasive. I really don't see a point
 in imposing a reset scheme upon all drivers and I think you have about
 as much motivation to convert radeon to the scheme used by i915 as
 I'll have for converting to the one used by radeon. If it would fit at
 all.
>>> Oh my! No, I didn't wanted to suggest any global reset infrastructure.
>>>
>>> My idea was more that the fence framework provides a 
>>> fence->process_signaling callback that is periodically called after 
>>> enable_signaling is called to trigger manual signal processing in the 
>>> driver.
>>>
>>> This would both be suitable as a fallback in case of not working interrupts 
>>> as well as a chance for any driver to do necessary lockup handling.
>> I managed to do it without needing it to be part of the interface? I'm not 
>> sure whether radeon_fence_driver_recheck needs exclusive_lock, but if so 
>> it's a small change..
>>
>> diff --git a/drivers/gpu/drm/radeon/radeon.h 
>> b/drivers/gpu/drm/radeon/radeon.h
>> index 7fbfd41479f1..51b646b9c8bb 100644
>> --- a/drivers/gpu/drm/radeon/radeon.h
>> +++ b/drivers/gpu/drm/radeon/radeon.h
>> @@ -345,6 +345,9 @@ struct radeon_fence_driver {
>>  uint64_tsync_seq[RADEON_NUM_RINGS];
>>  atomic64_t  last_seq;
>>  boolinitialized;
>> +struct delayed_work work;
>> +struct radeon_device *rdev;
>> +unsigned ring;
>>   };
>>   
>>   struct radeon_fence_cb {
>> diff --git a/drivers/gpu/drm/radeon/radeon_fence.c 
>> b/drivers/gpu/drm/radeon/radeon_fence.c
>> index da83f36dd708..955c825946ad 100644
>> --- a/drivers/gpu/drm/radeon/radeon_fence.c
>> +++ b/drivers/gpu/drm/radeon/radeon_fence.c
>> @@ -231,6 +231,9 @@ static bool __radeon_fence_process(struct radeon_device 
>> *rdev, int ring)
>>  }
>>  } while (atomic64_xchg(>fence_drv[ring].last_seq, seq) > seq);
>>   
>> +if (!wake && last_seq < last_emitted)
>> +schedule_delayed_work(>fence_drv[ring].work, 
>> jiffies_to_msecs(10));
>> +
>>
> When trying this: if (seq < last_emitted) is probably a better check.
>
> ~Maarten
>

[PATCH] drm: Unlink dead file_priv from list of active files first

[David Herrmann]

Hi

On Thu, Jul 24, 2014 at 3:23 PM, Chris Wilson  
wrote:
> In order to prevent external observers walking the list of open DRM
> files from seeing an invalid drm_file_private in the process of being
> torndown, the first operation we need to take is to unlink the
> drm_file_private from that list.
>
> general protection fault:  [#1] PREEMPT SMP
> Modules linked in: i915 i2c_algo_bit drm_kms_helper drm lpc_ich 
> mfd_core nls_iso8859_1 i2c_hid video hid_generic usbhid hid e1000e ahci ptp 
> libahci pps_core
> CPU: 3 PID: 8220 Comm: cat Not tainted 3.16.0-rc6+ #4
> Hardware name: Intel Corporation Shark Bay Client platform/WhiteTip 
> Mountain 1, BIOS HSWLPTU1.86C.0119.R00.1303230105 03/23/2013
> task: 8800219642c0 ti: 880047024000 task.ti: 880047024000
> RIP: 0010:[]  [] 
> per_file_stats+0x110/0x160 [i915]
> RSP: 0018:880047027d48  EFLAGS: 00010246
> RAX: 6b6b6b6b6b6b6b6b RBX: 880047027e30 RCX: 
> RDX: 0001 RSI:  RDI: 88003a05cd00
> RBP: 880047027d58 R08: 0001 R09: 
> R10: 8800219642c0 R11:  R12: 88003a05cd00
> R13:  R14: 88003a05cd00 R15: 880047027d88
> FS:  7f5f73a13740() GS:88014e38() 
> knlGS:
> CS:  0010 DS:  ES:  CR0: 80050033
> CR2: 023ff038 CR3: 21a4b000 CR4: 001407e0
> Stack:
>  0001  880047027dc8 813438e4
>  880047027e30 a0137b60 880021a8af58 880021a8f1a0
>  8800a2061fb0 8800a2062048 8800a2061fb0 8800a1e23478
> Call Trace:
>  [] idr_for_each+0xf4/0x180
>  [] ? i915_gem_stolen_list_info+0x1f0/0x1f0 [i915]
>  [] i915_gem_object_info+0x5ca/0x6a0 [i915]
>  [] seq_read+0xf5/0x3a0
>  [] vfs_read+0x90/0x150
>  [] SyS_read+0x49/0xb0
>  [] tracesys+0xd0/0xd5
> Code: 01 00 00 49 39 84 24 08 01 00 00 74 55 49 8b 84 24 b8 00 00 00 
> 48 01 43 18 31 c0 5b 41 5c 5d c3 0f 1f 00 49 8b 44 24 08 4c 89 e7 <48> 8b 70 
> 28 48 81 c6 48 80 00 00 e8 80 14 01 00 84 c0 74 bc 49
> RIP  [] per_file_stats+0x110/0x160 [i915]
> RSP 
>
> Reported-by: "Ursulin, Tvrtko" 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81712
> Signed-off-by: Chris Wilson 
> Cc: "Ursulin, Tvrtko" 

I have the same change in my local drm_file cleanup. This is:

Reviewed-by: David Herrmann 

Thanks
David

> ---
>  drivers/gpu/drm/drm_fops.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index a0c63cf..aff8217 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -416,6 +416,10 @@ int drm_release(struct inode *inode, struct file *filp)
>
> DRM_DEBUG("open_count = %d\n", dev->open_count);
>
> +   mutex_lock(>struct_mutex);
> +   list_del(_priv->lhead);
> +   mutex_unlock(>struct_mutex);
> +
> if (dev->driver->preclose)
> dev->driver->preclose(dev, file_priv);
>
> @@ -509,10 +513,6 @@ int drm_release(struct inode *inode, struct file *filp)
> file_priv->is_master = 0;
> mutex_unlock(>master_mutex);
>
> -   mutex_lock(>struct_mutex);
> -   list_del(_priv->lhead);
> -   mutex_unlock(>struct_mutex);
> -
> if (dev->driver->postclose)
> dev->driver->postclose(dev, file_priv);
>
> --
> 1.9.1
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 00/25] AMDKFD kernel driver

[Jerome Glisse]

On Thu, Jul 24, 2014 at 01:35:53PM -0400, Alex Deucher wrote:
> On Thu, Jul 24, 2014 at 11:44 AM, Jerome Glisse  wrote:
> > On Thu, Jul 24, 2014 at 01:01:41AM +0300, Oded Gabbay wrote:
> >> On 24/07/14 00:46, Bridgman, John wrote:
> >> >
> >> >> -Original Message- From: dri-devel
> >> >> [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of Jesse
> >> >> Barnes Sent: Wednesday, July 23, 2014 5:00 PM To:
> >> >> dri-devel at lists.freedesktop.org Subject: Re: [PATCH v2 00/25]
> >> >> AMDKFD kernel driver
> >> >>
> >> >> On Mon, 21 Jul 2014 19:05:46 +0200 daniel at ffwll.ch (Daniel
> >> >> Vetter) wrote:
> >> >>
> >> >>> On Mon, Jul 21, 2014 at 11:58:52AM -0400, Jerome Glisse wrote:
> >>  On Mon, Jul 21, 2014 at 05:25:11PM +0200, Daniel Vetter wrote:
> >> > On Mon, Jul 21, 2014 at 03:39:09PM +0200, Christian K?nig
> >> > wrote:
> >> >> Am 21.07.2014 14:36, schrieb Oded Gabbay:
> >> >>> On 20/07/14 20:46, Jerome Glisse wrote:
> >> >>
> >> >> [snip!!]
> >> > My BlackBerry thumb thanks you ;)
> >> >>
> >> >>
> >> >> The main questions here are if it's avoid able to pin down
> >> >> the memory and if the memory is pinned down at driver load,
> >> >> by request from userspace or by anything else.
> >> >>
> >> >> As far as I can see only the "mqd per userspace queue"
> >> >> might be a bit questionable, everything else sounds
> >> >> reasonable.
> >> >
> >> > Aside, i915 perspective again (i.e. how we solved this):
> >> > When scheduling away from contexts we unpin them and put them
> >> > into the lru. And in the shrinker we have a last-ditch
> >> > callback to switch to a default context (since you can't ever
> >> > have no context once you've started) which means we can evict
> >> > any context object if it's
> >> >> getting in the way.
> >> 
> >>  So Intel hardware report through some interrupt or some channel
> >>  when it is not using a context ? ie kernel side get
> >>  notification when some user context is done executing ?
> >> >>>
> >> >>> Yes, as long as we do the scheduling with the cpu we get
> >> >>> interrupts for context switches. The mechanic is already
> >> >>> published in the execlist patches currently floating around. We
> >> >>> get a special context switch interrupt.
> >> >>>
> >> >>> But we have this unpin logic already on the current code where
> >> >>> we switch contexts through in-line cs commands from the kernel.
> >> >>> There we obviously use the normal batch completion events.
> >> >>
> >> >> Yeah and we can continue that going forward.  And of course if your
> >> >> hw can do page faulting, you don't need to pin the normal data
> >> >> buffers.
> >> >>
> >> >> Usually there are some special buffers that need to be pinned for
> >> >> longer periods though, anytime the context could be active.  Sounds
> >> >> like in this case the userland queues, which makes some sense.  But
> >> >> maybe for smaller systems the size limit could be clamped to
> >> >> something smaller than 128M.  Or tie it into the rlimit somehow,
> >> >> just like we do for mlock() stuff.
> >> >>
> >> > Yeah, even the queues are in pageable memory, it's just a ~256 byte
> >> > structure per queue (the Memory Queue Descriptor) that describes the
> >> > queue to hardware, plus a couple of pages for each process using HSA
> >> > to hold things like doorbells. Current thinking is to limit #
> >> > processes using HSA to ~256 and #queues per process to ~1024 by
> >> > default in the initial code, although my guess is that we could take
> >> > the #queues per process default limit even lower.
> >> >
> >>
> >> So my mistake. struct cik_mqd is actually 604 bytes, and it is allocated
> >> on 256 boundary.
> >> I had in mind to reserve 64MB of gart by default, which translates to
> >> 512 queues per process, with 128 processes. Add 2 kernel module
> >> parameters, # of max-queues-per-process and # of max-processes (default
> >> is, as I said, 512 and 128) for better control of system admin.
> >>
> >
> > So as i said somewhere else in this thread, this should not be reserved
> > but use a special allocation. Any HSA GPU use virtual address space for
> > userspace so only issue is for kernel side GTT.
> >
> > What i would like is seeing radeon pinned GTT allocation at bottom of
> > GTT space (ie all ring buffer and the ib pool buffer). Then have an
> > allocator that allocate new queue from top of GTT address space and
> > grow to the bottom.
> >
> > It should not staticly reserved 64M or anything. When doing allocation
> > it should move any ttm buffer that are in the region it want to allocate
> > to a different location.
> >
> >
> > As this needs some work, i am not against reserving some small amount
> > (couple MB) as a first stage but anything more would need a proper solution
> > like the one i just described.
> 
> It's still a trade off.  Even if we reserve a couple of megs it'll be
> wasted if we are not 

[PATCH RFA 2/2] drm/i2c: tda998x: add component support

[Darren Etheridge]

On 07/24/2014 09:57 AM, Russell King wrote:
> Add component helper support to the tda998x driver.  This permits the
> TDA998x to be declared as a separate device in device tree, and bound
> at the appropriate moment with a co-operating card driver.
>
> The existing slave_encoder interfaces are kept while there are existing
> users of it in order to prevent regressions.
>
> Signed-off-by: Russell King 
> ---
> RFA = request for acks, but tested-by's are also welcome, particularly
> for tilcdc hardware.
>

Works fine on BeagleBone Black (tilcdc + tda998x)

Tested against 3.16-rc6, using unmodified tilcdc driver, unmodified dts.

Version details:
Linux am335x-evm 3.16.0-rc6-2-gf8a096f-dirty #40 SMP Thu Jul 24 
13:49:42 CDT 2014 armv7l GNU/Linux

Tried modetest with a variety of modes, all looked correct.
Tested as both built-in and modules.

Tested-by: Darren Etheridge 

[PATCH RFA 1/2] drm/i2c: tda998x: allow re-use of tda998x support code

[Darren Etheridge]

On 07/24/2014 09:57 AM, Russell King wrote:
> Re-jig the TDA998x code so that we separate the functionality from the
> drm slave encoder implementation.  In several places, this is pretty
> clearly the correct thing to do, because we can avoid repetitively
> having to convert from the drm_encoder to the TDA998x private
> structure, particularly with the driver internal functions.
>
> The main motivation behind this change is to allow the code to be
> re-used with a standard drm_encoder and drm_connector implementation
> based on the component helpers, rather than the slave_encoder system.
> The addition of this will be in the following patch.
>
> We keep the slave_encoder interface as there are existing users of
> this; we need to give them time to convert and test.
>
> Signed-off-by: Russell King 
> ---
> RFA = request for acks, but tested-by's would also be great,
> particularly from the tilcdc folk.
>

Works fine on BeagleBone Black (tilcdc + tda998x)

Tested against 3.16-rc6, using unmodified tilcdc driver, unmodified dts.

Version details:
Linux am335x-evm 3.16.0-rc6-2-gf8a096f-dirty #40 SMP Thu Jul 24 
13:49:42 CDT 2014 armv7l GNU/Linux

Tried modetest with a variety of modes, all looked correct.
Tested as both built-in and modules.

Tested-by: Darren Etheridge 

[PATCH 12/12] drm: make sysfs device always available for minors

[Daniel Vetter]

On Thu, Jul 24, 2014 at 12:48:49PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Jul 24, 2014 at 12:36 PM, Daniel Vetter  wrote:
> > On Wed, Jul 23, 2014 at 05:26:47PM +0200, David Herrmann wrote:
> >> For each minor we allocate a sysfs device as minor->kdev. Currently, this
> >> is allocated and registered in drm_minor_register(). This makes it
> >> impossible to add sysfs-attributes to the device before it is registered.
> >> Therefore, they are not added atomically, nor can we move device_add()
> >> *after* ->load() is called.
> >>
> >> This patch makes minor->kdev available early, but only adds the device
> >> during minor-registration.
> >>
> >> Signed-off-by: David Herrmann 
> >
> > Some diff got confused with this one. One comment below, but this is
> >
> > Reviewed-by: Daniel Vetter 
> >
> >> ---
> >>  drivers/gpu/drm/drm_stub.c  | 22 ---
> >>  drivers/gpu/drm/drm_sysfs.c | 90 
> >> +++--
> >>  include/drm/drmP.h  |  3 +-
> >>  3 files changed, 54 insertions(+), 61 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> >> index 8b24db5..09c6bfb 100644
> >> --- a/drivers/gpu/drm/drm_stub.c
> >> +++ b/drivers/gpu/drm/drm_stub.c
> >> @@ -281,9 +281,19 @@ static int drm_minor_alloc(struct drm_device *dev, 
> >> unsigned int type)
> >>
> >>   minor->index = r;
> >>
> >> + minor->kdev = drm_sysfs_minor_alloc(minor);
> >> + if (IS_ERR(minor->kdev)) {
> >> + r = PTR_ERR(minor->kdev);
> >> + goto err_index;
> >> + }
> >> +
> >>   *drm_minor_get_slot(dev, type) = minor;
> >>   return 0;
> >>
> >> +err_index:
> >> + spin_lock_irqsave(_minor_lock, flags);
> >> + idr_remove(_minors_idr, minor->index);
> >> + spin_unlock_irqrestore(_minor_lock, flags);
> >>  err_free:
> >>   kfree(minor);
> >>   return r;
> >> @@ -300,6 +310,7 @@ static void drm_minor_free(struct drm_device *dev, 
> >> unsigned int type)
> >>   return;
> >>
> >>   drm_mode_group_destroy(>mode_group);
> >> + put_device(minor->kdev);
> >>
> >>   spin_lock_irqsave(_minor_lock, flags);
> >>   idr_remove(_minors_idr, minor->index);
> >> @@ -327,11 +338,9 @@ static int drm_minor_register(struct drm_device *dev, 
> >> unsigned int type)
> >>   return ret;
> >>   }
> >>
> >> - ret = drm_sysfs_device_add(minor);
> >> - if (ret) {
> >> - DRM_ERROR("DRM: Error sysfs_device_add.\n");
> >> + ret = device_add(minor->kdev);
> >> + if (ret)
> >>   goto err_debugfs;
> >> - }
> >>
> >>   /* replace NULL with @minor so lookups will succeed from now on */
> >>   spin_lock_irqsave(_minor_lock, flags);
> >> @@ -352,7 +361,7 @@ static void drm_minor_unregister(struct drm_device 
> >> *dev, unsigned int type)
> >>   unsigned long flags;
> >>
> >>   minor = *drm_minor_get_slot(dev, type);
> >> - if (!minor || !minor->kdev)
> >> + if (!minor || !device_is_registered(minor->kdev))
> >>   return;
> >>
> >>   /* replace @minor with NULL so lookups will fail from now on */
> >> @@ -360,8 +369,9 @@ static void drm_minor_unregister(struct drm_device 
> >> *dev, unsigned int type)
> >>   idr_replace(_minors_idr, NULL, minor->index);
> >>   spin_unlock_irqrestore(_minor_lock, flags);
> >>
> >> + device_del(minor->kdev);
> >> + dev_set_drvdata(minor->kdev, NULL); /* safety belt */
> >>   drm_debugfs_cleanup(minor);
> >> - drm_sysfs_device_remove(minor);
> >>  }
> >>
> >>  /**
> >> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> >> index 7827dad..ab1a5f6 100644
> >> --- a/drivers/gpu/drm/drm_sysfs.c
> >> +++ b/drivers/gpu/drm/drm_sysfs.c
> >> @@ -493,71 +493,55 @@ static void drm_sysfs_release(struct device *dev)
> >>  }
> >>
> >>  /**
> >> - * drm_sysfs_device_add - adds a class device to sysfs for a character 
> >> driver
> >> - * @dev: DRM device to be added
> >> - * @head: DRM head in question
> >> + * drm_sysfs_minor_alloc() - Allocate sysfs device for given minor
> >> + * @minor: minor to allocate sysfs device for
> >>   *
> >> - * Add a DRM device to the DRM's device model class.  We use @dev's PCI 
> >> device
> >> - * as the parent for the Linux device, and make sure it has a file 
> >> containing
> >> - * the driver we're using (for userspace compatibility).
> >> + * This allocates a new sysfs device for @minor and returns it. The 
> >> device is
> >> + * not registered nor linked. The caller has to use device_add() and
> >> + * device_del() to register and unregister it.
> >> + *
> >> + * Note that dev_get_drvdata() on the new device will return the minor.
> >> + * However, the device does not hold a ref-count to the minor nor to the
> >> + * underlying drm_device. This is unproblematic as long as you access the
> >> + * private data only in sysfs callbacks. device_del() disables those
> >> + * synchronously, so they cannot be called 

[PATCH 11/12] drm: make minor->index available early

[Daniel Vetter]

On Thu, Jul 24, 2014 at 12:16:59PM +0200, David Herrmann wrote:
> Hi
> 
> On Thu, Jul 24, 2014 at 12:03 PM, Daniel Vetter  wrote:
> > On Wed, Jul 23, 2014 at 05:26:46PM +0200, David Herrmann wrote:
> >> Instead of allocating the minor-index during registration, we now do this
> >> during allocation. This way, debug-messages between minor-allocation and
> >> minor-registration will now use the correct minor instead of 0. Same is
> >> done for unregistration vs. free, so debug-messages between
> >> device-shutdown and device-destruction show proper indices.
> >>
> >> Even though minor-indices are allocated early, we don't enable minor
> >> lookup early. Instead, we keep the entry set to NULL and replace it during
> >> registration / unregistration. This way, the index is allocated but lookup
> >> only works if registered.
> >>
> >> Signed-off-by: David Herrmann 
> >> ---
> >>  drivers/gpu/drm/drm_stub.c | 84 
> >> +-
> >>  1 file changed, 46 insertions(+), 38 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> >> index b249f14..8b24db5 100644
> >> --- a/drivers/gpu/drm/drm_stub.c
> >> +++ b/drivers/gpu/drm/drm_stub.c
> >> @@ -256,6 +256,8 @@ static struct drm_minor **drm_minor_get_slot(struct 
> >> drm_device *dev,
> >>  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
> >>  {
> >>   struct drm_minor *minor;
> >> + unsigned long flags;
> >> + int r;
> >>
> >>   minor = kzalloc(sizeof(*minor), GFP_KERNEL);
> >>   if (!minor)
> >> @@ -264,57 +266,68 @@ static int drm_minor_alloc(struct drm_device *dev, 
> >> unsigned int type)
> >>   minor->type = type;
> >>   minor->dev = dev;
> >>
> >> + idr_preload(GFP_KERNEL);
> >> + spin_lock_irqsave(_minor_lock, flags);
> >> + r = idr_alloc(_minors_idr,
> >> +   NULL,
> >> +   64 * type,
> >> +   64 * (type + 1),
> >> +   GFP_NOWAIT);
> >> + spin_unlock_irqrestore(_minor_lock, flags);
> >> + idr_preload_end();
> >> +
> >> + if (r < 0)
> >> + goto err_free;
> >> +
> >> + minor->index = r;
> >> +
> >>   *drm_minor_get_slot(dev, type) = minor;
> >>   return 0;
> >> +
> >> +err_free:
> >> + kfree(minor);
> >> + return r;
> >>  }
> >>
> >>  static void drm_minor_free(struct drm_device *dev, unsigned int type)
> >>  {
> >> - struct drm_minor **slot;
> >> + struct drm_minor **slot, *minor;
> >> + unsigned long flags;
> >>
> >>   slot = drm_minor_get_slot(dev, type);
> >> - if (*slot) {
> >> - drm_mode_group_destroy(&(*slot)->mode_group);
> >> - kfree(*slot);
> >> - *slot = NULL;
> >> - }
> >> + minor = *slot;
> >> + if (!minor)
> >> + return;
> >> +
> >> + drm_mode_group_destroy(>mode_group);
> >> +
> >> + spin_lock_irqsave(_minor_lock, flags);
> >> + idr_remove(_minors_idr, minor->index);
> >> + spin_unlock_irqrestore(_minor_lock, flags);
> >
> > I don't understand why you do the idr removal in stages too. Otherwise
> > this looks good.
> 
> If you call drm_minor_unregister(), we now disable lookup but keep
> minor->index. If I also released the ID, a new drm_minor might get
> access to it before we call drm_minor_free on the old one. This might
> cause misleading debug-messages as both minor objects have the same
> index. This is not really a problem, but kinda ugly.

Ah, I see. Makes sense. Reviewed-by: Daniel Vetter 
> 
> > Aside: If two drivers load concurrently (i.e. in the brave new world
> > withou drm_global_mutex) we might end up with interleaved minor ids. Dunno
> > whether we'll care since userspace should use udev/sysfs lookups anyway.
> > igt sometimes doesn't ;-)
> 
> I did post a patch some time ago that makes minor-ID allocations
> predictable. I got a NACK from you, so this one is one you ;) But I
> agree, we really should fix user-space instead of making random IDs
> predictable.

/me remembers again ...

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

[PATCH 0/7] prepare for atomic.. the great propertyification

[Daniel Vetter]

On Wed, Jul 23, 2014 at 03:38:13PM -0400, Rob Clark wrote:
> This is mostly just a rebase+resend.  Was going to send it a bit earlier
> but had a few things to fix up as a result of the rebase.
> 
> At this point, I think next steps are roughly:
> 1) introduce plane->mutex
> 2) decide what we want to do about events
> 3) add actual ioctl
> 
> I think we could shoot for merging this series next, and then adding
> plane->mutex in 3.18?
> 
> Before we add the ioctl, I think we want to sort out events for updates
> to non-primary layers, and what the interface to drivers should look like.
> Ie. just add event to ->update_plane() or should we completely ditch
> ->page_flip() and ->update_plane()?
> 
> Technically, I think we could get away without a new API and just let
> drivers grab all the events in their ->atomic_commit(), but I suspect
> core could provide something more useful to drivers.  I guess it would
> be useful to have a few more drivers converted over to see what makes
> sense.

Ok so we've had a lot of discussions about this on irc and overall I'm not
terribly happy with what this looks like. I have a few concerns:

- The entire atomic conversion for drivers is monolithic, at least at the
  interface level. So if you want to do this step-by-step you're forced to
  add hacks and e.g. bypass pageflips until that's implemented. E.g. on
  i915 I see no chance that we'll get atomic ready for all cases (so
  nonblocking flips and multi-crtc and everything on all platforms) in one
  go.

- Related to that is that we force legacy ioctls through atomic. E.g. on
  older i915 platforms I very much expect that we won't ever convert the
  pageflip code to atomic for them and just not support atomic flips of
  cursor + primary plane. At least not at the beginning. I know that's
  different from my stance a year ago, but I've become a bit more
  realistic.

- The entire conversion is monolithic and we can't do it on a
  driver-by-driver basis. Everyone has to go through the new atomic
  interfaces on a flag day. drm is much bigger now and forcing everyone to
  convert at the same time is really risky. Again I know I've changed my
  mind again, but drm is a lot bigger and there's a lot more drivers that
  implement pageflip and cursors, i.e. stuff that's special.

- The separation between interface marshalling code in the drm core and
  helper functions for drivers to implement stuff is fuzzy at best. Thus
  far we've had an excellent seperation in this are for kms, I don't think
  it's vise to throw that out.

So here's my proposal for how to get this in without breaking the world

1. We add a complete new set of ->atomic_foo driver entry points to all
relevant objects. So instead of changing all the set_prop functions in a
flag-day like operation we just add a new interface. I haven't double
checked, but I guess that means per-obj ->atomic_set_prop functions plus a
global ->atomic_check and ->atomic_commit.

2. We add a new drm_atomic_helper.c library which provides functions to
implement all the legacy interfaces (page_flip, update_plane, set_prop)
using atomic interfaces. We should be able to 1:1 reuse Rob's code for
this, but instead of changing the actual current interface code we put
that into helpers.

We don't need anything for cursor ioctls since cursor plane helpers
already map the legacy cursor ioctls.

3. Rob's current code reuses the existing ->update_plane, ->pageflip and
other entry points to implement the atomic commit helper functions. Imo
that's a bad layering violation, and if we plug helpers in there we can't
use them.

Instead I think we should add a new per-plane ->atomic_commit functions
clearly marked as optional. Maybe even as part of an opaque
plane_helper_funcs pointer like we have with crtc/encoder/connector and
crtc helpers. msm would then implement it's atomic commit function in
there (since it's the only driver currently supporting atomic for real).

3b. We could adjust crtc helpers to use the plane commit hook instead of
the set_base function when avialable.

4. We do a pimped atomic helper based upon crtc helpers and the above
plane commit function added in 3. It would essentially implement what
Rob's current helper does, i.e.

Step 1: Shut down all crtcs which change using the crtc helpers. This step
is obviously optional and ommitted when there's nothing to do.

Step 2: Loop over all planes and call ->plane_commit or whatever we will
call the interface added in 3. above.

Step 3: Enable all crtcs that need enabling.

5. We start converting drivers. Every driver can convert at it's own pace,
opting in for atomic behaviour step-by-step.

6. We optionally use the atomic interface in the fb helper. It's imo
important to keep the code here parallel so that drivers can convert at
their own leisure.

7. We add the atomic ioctl.

8. Various cleanups once 5. is completed for all drivers - which will
likely take at least a year:
- Remove ->set_base from crtc helpers.
- Remove legacy 

[PATCH] drm: Unlink dead file_priv from list of active files first

[Chris Wilson]

In order to prevent external observers walking the list of open DRM
files from seeing an invalid drm_file_private in the process of being
torndown, the first operation we need to take is to unlink the
drm_file_private from that list.

general protection fault:  [#1] PREEMPT SMP
Modules linked in: i915 i2c_algo_bit drm_kms_helper drm lpc_ich 
mfd_core nls_iso8859_1 i2c_hid video hid_generic usbhid hid e1000e ahci ptp 
libahci pps_core
CPU: 3 PID: 8220 Comm: cat Not tainted 3.16.0-rc6+ #4
Hardware name: Intel Corporation Shark Bay Client platform/WhiteTip 
Mountain 1, BIOS HSWLPTU1.86C.0119.R00.1303230105 03/23/2013
task: 8800219642c0 ti: 880047024000 task.ti: 880047024000
RIP: 0010:[]  [] 
per_file_stats+0x110/0x160 [i915]
RSP: 0018:880047027d48  EFLAGS: 00010246
RAX: 6b6b6b6b6b6b6b6b RBX: 880047027e30 RCX: 
RDX: 0001 RSI:  RDI: 88003a05cd00
RBP: 880047027d58 R08: 0001 R09: 
R10: 8800219642c0 R11:  R12: 88003a05cd00
R13:  R14: 88003a05cd00 R15: 880047027d88
FS:  7f5f73a13740() GS:88014e38() 
knlGS:
CS:  0010 DS:  ES:  CR0: 80050033
CR2: 023ff038 CR3: 21a4b000 CR4: 001407e0
Stack:
 0001  880047027dc8 813438e4
 880047027e30 a0137b60 880021a8af58 880021a8f1a0
 8800a2061fb0 8800a2062048 8800a2061fb0 8800a1e23478
Call Trace:
 [] idr_for_each+0xf4/0x180
 [] ? i915_gem_stolen_list_info+0x1f0/0x1f0 [i915]
 [] i915_gem_object_info+0x5ca/0x6a0 [i915]
 [] seq_read+0xf5/0x3a0
 [] vfs_read+0x90/0x150
 [] SyS_read+0x49/0xb0
 [] tracesys+0xd0/0xd5
Code: 01 00 00 49 39 84 24 08 01 00 00 74 55 49 8b 84 24 b8 00 00 00 48 
01 43 18 31 c0 5b 41 5c 5d c3 0f 1f 00 49 8b 44 24 08 4c 89 e7 <48> 8b 70 28 48 
81 c6 48 80 00 00 e8 80 14 01 00 84 c0 74 bc 49
RIP  [] per_file_stats+0x110/0x160 [i915]
RSP 

Reported-by: "Ursulin, Tvrtko" 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=81712
Signed-off-by: Chris Wilson 
Cc: "Ursulin, Tvrtko" 
---
 drivers/gpu/drm/drm_fops.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index a0c63cf..aff8217 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -416,6 +416,10 @@ int drm_release(struct inode *inode, struct file *filp)

DRM_DEBUG("open_count = %d\n", dev->open_count);

+   mutex_lock(>struct_mutex);
+   list_del(_priv->lhead);
+   mutex_unlock(>struct_mutex);
+
if (dev->driver->preclose)
dev->driver->preclose(dev, file_priv);

@@ -509,10 +513,6 @@ int drm_release(struct inode *inode, struct file *filp)
file_priv->is_master = 0;
mutex_unlock(>master_mutex);

-   mutex_lock(>struct_mutex);
-   list_del(_priv->lhead);
-   mutex_unlock(>struct_mutex);
-
if (dev->driver->postclose)
dev->driver->postclose(dev, file_priv);

-- 
1.9.1

[pull] radeon drm-fixes-3.16

[Alex Deucher]

Hi Dave,

Just a couple more small radeon fixes.

The following changes since commit ec8a362f2e6e380e7a1f66a6c9a7f6c237ab3520:

  Merge branch 'drm-fixes-3.16' of git://people.freedesktop.org/~agd5f/linux 
into drm-fixes (2014-07-22 10:44:10 +1000)

are available in the git repository at:


  git://people.freedesktop.org/~agd5f/linux drm-fixes-3.16

for you to fetch changes up to e8c214d22e76dd0ead38f97f8d2dc09aac70d651:

  drm/radeon: fix irq ring buffer overflow handling (2014-07-23 11:35:36 -0400)


Christian K?nig (2):
  drm/radeon: fix error handling in radeon_vm_bo_set_addr
  drm/radeon: fix irq ring buffer overflow handling

 drivers/gpu/drm/radeon/cik.c   | 1 +
 drivers/gpu/drm/radeon/evergreen.c | 1 +
 drivers/gpu/drm/radeon/r600.c  | 1 +
 drivers/gpu/drm/radeon/radeon_vm.c | 4 
 drivers/gpu/drm/radeon/si.c| 1 +
 5 files changed, 8 insertions(+)

[PATCH v2 00/25] AMDKFD kernel driver

[Alex Deucher]

On Thu, Jul 24, 2014 at 11:44 AM, Jerome Glisse  wrote:
> On Thu, Jul 24, 2014 at 01:01:41AM +0300, Oded Gabbay wrote:
>> On 24/07/14 00:46, Bridgman, John wrote:
>> >
>> >> -Original Message- From: dri-devel
>> >> [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of Jesse
>> >> Barnes Sent: Wednesday, July 23, 2014 5:00 PM To:
>> >> dri-devel at lists.freedesktop.org Subject: Re: [PATCH v2 00/25]
>> >> AMDKFD kernel driver
>> >>
>> >> On Mon, 21 Jul 2014 19:05:46 +0200 daniel at ffwll.ch (Daniel
>> >> Vetter) wrote:
>> >>
>> >>> On Mon, Jul 21, 2014 at 11:58:52AM -0400, Jerome Glisse wrote:
>>  On Mon, Jul 21, 2014 at 05:25:11PM +0200, Daniel Vetter wrote:
>> > On Mon, Jul 21, 2014 at 03:39:09PM +0200, Christian K?nig
>> > wrote:
>> >> Am 21.07.2014 14:36, schrieb Oded Gabbay:
>> >>> On 20/07/14 20:46, Jerome Glisse wrote:
>> >>
>> >> [snip!!]
>> > My BlackBerry thumb thanks you ;)
>> >>
>> >>
>> >> The main questions here are if it's avoid able to pin down
>> >> the memory and if the memory is pinned down at driver load,
>> >> by request from userspace or by anything else.
>> >>
>> >> As far as I can see only the "mqd per userspace queue"
>> >> might be a bit questionable, everything else sounds
>> >> reasonable.
>> >
>> > Aside, i915 perspective again (i.e. how we solved this):
>> > When scheduling away from contexts we unpin them and put them
>> > into the lru. And in the shrinker we have a last-ditch
>> > callback to switch to a default context (since you can't ever
>> > have no context once you've started) which means we can evict
>> > any context object if it's
>> >> getting in the way.
>> 
>>  So Intel hardware report through some interrupt or some channel
>>  when it is not using a context ? ie kernel side get
>>  notification when some user context is done executing ?
>> >>>
>> >>> Yes, as long as we do the scheduling with the cpu we get
>> >>> interrupts for context switches. The mechanic is already
>> >>> published in the execlist patches currently floating around. We
>> >>> get a special context switch interrupt.
>> >>>
>> >>> But we have this unpin logic already on the current code where
>> >>> we switch contexts through in-line cs commands from the kernel.
>> >>> There we obviously use the normal batch completion events.
>> >>
>> >> Yeah and we can continue that going forward.  And of course if your
>> >> hw can do page faulting, you don't need to pin the normal data
>> >> buffers.
>> >>
>> >> Usually there are some special buffers that need to be pinned for
>> >> longer periods though, anytime the context could be active.  Sounds
>> >> like in this case the userland queues, which makes some sense.  But
>> >> maybe for smaller systems the size limit could be clamped to
>> >> something smaller than 128M.  Or tie it into the rlimit somehow,
>> >> just like we do for mlock() stuff.
>> >>
>> > Yeah, even the queues are in pageable memory, it's just a ~256 byte
>> > structure per queue (the Memory Queue Descriptor) that describes the
>> > queue to hardware, plus a couple of pages for each process using HSA
>> > to hold things like doorbells. Current thinking is to limit #
>> > processes using HSA to ~256 and #queues per process to ~1024 by
>> > default in the initial code, although my guess is that we could take
>> > the #queues per process default limit even lower.
>> >
>>
>> So my mistake. struct cik_mqd is actually 604 bytes, and it is allocated
>> on 256 boundary.
>> I had in mind to reserve 64MB of gart by default, which translates to
>> 512 queues per process, with 128 processes. Add 2 kernel module
>> parameters, # of max-queues-per-process and # of max-processes (default
>> is, as I said, 512 and 128) for better control of system admin.
>>
>
> So as i said somewhere else in this thread, this should not be reserved
> but use a special allocation. Any HSA GPU use virtual address space for
> userspace so only issue is for kernel side GTT.
>
> What i would like is seeing radeon pinned GTT allocation at bottom of
> GTT space (ie all ring buffer and the ib pool buffer). Then have an
> allocator that allocate new queue from top of GTT address space and
> grow to the bottom.
>
> It should not staticly reserved 64M or anything. When doing allocation
> it should move any ttm buffer that are in the region it want to allocate
> to a different location.
>
>
> As this needs some work, i am not against reserving some small amount
> (couple MB) as a first stage but anything more would need a proper solution
> like the one i just described.

It's still a trade off.  Even if we reserve a couple of megs it'll be
wasted if we are not running HSA apps. And even today if we run a
compute job using the current interfaces we could end up in the same
case.  So while I think it's definitely a good goal to come up with
some solution for fragmentation, I don't think it should be a
show-stopper right 

[PATCH 02/12] drm: drop unused "struct drm_queue"

[David Herrmann]

Hi

On Wed, Jul 23, 2014 at 9:35 PM, Daniel Vetter  wrote:
> On Wed, Jul 23, 2014 at 05:26:37PM +0200, David Herrmann wrote:
>> This object is unused, drop it.
>>
>> Signed-off-by: David Herrmann 
>
> Funny how after even all my "kill stuff with fire" series there's still
> such low hanging fruit left ;-)
>
> Reviewed-by: Daniel Vetter 

I think the problem is, they all depend on each other. With this patch
applied, "drm_waitlist" becomes unused.. noticed it just now. I
already have a followup.

Thanks
David

[PATCH 1/7] drm: add atomic fxns

["Stéphane Viau"]

Hi,

>>  {
>>  int ret = -EINVAL;
>> -struct drm_connector *connector = obj_to_connector(obj);
>>
>>  /* Do DPMS ourselves */
>>  if (property == connector->dev->mode_config.dpms_property) {
>>  if (connector->funcs->dpms)
>>  (*connector->funcs->dpms)(connector, (int)value);
>>  ret = 0;
>> -} else if (connector->funcs->set_property)
>> -ret = connector->funcs->set_property(connector, property,
> value);
>> +} else if (connector->funcs->set_property) {
>> +ret = connector->funcs->set_property(connector, state,
>> +property, value, blob_data);
>> +}
>
> Why the extra braces here? There's still only one statement in the
> block.
>

According to Documentation/kernel-doc-nano-HOWTO.txt, these extra braces are
supposed to be present:

"
Chapter 3: Placing Braces and Spaces
[...]
Do not unnecessarily use braces where a single statement will do.
[...]
This does not apply if only one branch of a conditional statement is a single
statement; in the latter case use braces in both branches:

if (condition) {
do_this();
do_that();
} else {
otherwise();
}
"

St?phane.

>>  /**
>> - * drm_mode_getproperty_ioctl - get the current value of a object's
> property
>> + * drm_mode_obj_get_properties_ioctl - get the current value of a
> object's property
>>   * @dev: DRM device
>>   * @data: ioctl data
>>   * @file_priv: DRM file info
>
> This isn't really introduced by this patch, but isn't this kerneldoc
> comment wrong? drm_mode_obj_get_properties_ioctl() seems to return the
> values of all properties of an object rather than just one.
>
> Thierry
>


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 12/12] drm: make sysfs device always available for minors

[David Herrmann]

Hi

On Thu, Jul 24, 2014 at 12:36 PM, Daniel Vetter  wrote:
> On Wed, Jul 23, 2014 at 05:26:47PM +0200, David Herrmann wrote:
>> For each minor we allocate a sysfs device as minor->kdev. Currently, this
>> is allocated and registered in drm_minor_register(). This makes it
>> impossible to add sysfs-attributes to the device before it is registered.
>> Therefore, they are not added atomically, nor can we move device_add()
>> *after* ->load() is called.
>>
>> This patch makes minor->kdev available early, but only adds the device
>> during minor-registration.
>>
>> Signed-off-by: David Herrmann 
>
> Some diff got confused with this one. One comment below, but this is
>
> Reviewed-by: Daniel Vetter 
>
>> ---
>>  drivers/gpu/drm/drm_stub.c  | 22 ---
>>  drivers/gpu/drm/drm_sysfs.c | 90 
>> +++--
>>  include/drm/drmP.h  |  3 +-
>>  3 files changed, 54 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
>> index 8b24db5..09c6bfb 100644
>> --- a/drivers/gpu/drm/drm_stub.c
>> +++ b/drivers/gpu/drm/drm_stub.c
>> @@ -281,9 +281,19 @@ static int drm_minor_alloc(struct drm_device *dev, 
>> unsigned int type)
>>
>>   minor->index = r;
>>
>> + minor->kdev = drm_sysfs_minor_alloc(minor);
>> + if (IS_ERR(minor->kdev)) {
>> + r = PTR_ERR(minor->kdev);
>> + goto err_index;
>> + }
>> +
>>   *drm_minor_get_slot(dev, type) = minor;
>>   return 0;
>>
>> +err_index:
>> + spin_lock_irqsave(_minor_lock, flags);
>> + idr_remove(_minors_idr, minor->index);
>> + spin_unlock_irqrestore(_minor_lock, flags);
>>  err_free:
>>   kfree(minor);
>>   return r;
>> @@ -300,6 +310,7 @@ static void drm_minor_free(struct drm_device *dev, 
>> unsigned int type)
>>   return;
>>
>>   drm_mode_group_destroy(>mode_group);
>> + put_device(minor->kdev);
>>
>>   spin_lock_irqsave(_minor_lock, flags);
>>   idr_remove(_minors_idr, minor->index);
>> @@ -327,11 +338,9 @@ static int drm_minor_register(struct drm_device *dev, 
>> unsigned int type)
>>   return ret;
>>   }
>>
>> - ret = drm_sysfs_device_add(minor);
>> - if (ret) {
>> - DRM_ERROR("DRM: Error sysfs_device_add.\n");
>> + ret = device_add(minor->kdev);
>> + if (ret)
>>   goto err_debugfs;
>> - }
>>
>>   /* replace NULL with @minor so lookups will succeed from now on */
>>   spin_lock_irqsave(_minor_lock, flags);
>> @@ -352,7 +361,7 @@ static void drm_minor_unregister(struct drm_device *dev, 
>> unsigned int type)
>>   unsigned long flags;
>>
>>   minor = *drm_minor_get_slot(dev, type);
>> - if (!minor || !minor->kdev)
>> + if (!minor || !device_is_registered(minor->kdev))
>>   return;
>>
>>   /* replace @minor with NULL so lookups will fail from now on */
>> @@ -360,8 +369,9 @@ static void drm_minor_unregister(struct drm_device *dev, 
>> unsigned int type)
>>   idr_replace(_minors_idr, NULL, minor->index);
>>   spin_unlock_irqrestore(_minor_lock, flags);
>>
>> + device_del(minor->kdev);
>> + dev_set_drvdata(minor->kdev, NULL); /* safety belt */
>>   drm_debugfs_cleanup(minor);
>> - drm_sysfs_device_remove(minor);
>>  }
>>
>>  /**
>> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
>> index 7827dad..ab1a5f6 100644
>> --- a/drivers/gpu/drm/drm_sysfs.c
>> +++ b/drivers/gpu/drm/drm_sysfs.c
>> @@ -493,71 +493,55 @@ static void drm_sysfs_release(struct device *dev)
>>  }
>>
>>  /**
>> - * drm_sysfs_device_add - adds a class device to sysfs for a character 
>> driver
>> - * @dev: DRM device to be added
>> - * @head: DRM head in question
>> + * drm_sysfs_minor_alloc() - Allocate sysfs device for given minor
>> + * @minor: minor to allocate sysfs device for
>>   *
>> - * Add a DRM device to the DRM's device model class.  We use @dev's PCI 
>> device
>> - * as the parent for the Linux device, and make sure it has a file 
>> containing
>> - * the driver we're using (for userspace compatibility).
>> + * This allocates a new sysfs device for @minor and returns it. The device 
>> is
>> + * not registered nor linked. The caller has to use device_add() and
>> + * device_del() to register and unregister it.
>> + *
>> + * Note that dev_get_drvdata() on the new device will return the minor.
>> + * However, the device does not hold a ref-count to the minor nor to the
>> + * underlying drm_device. This is unproblematic as long as you access the
>> + * private data only in sysfs callbacks. device_del() disables those
>> + * synchronously, so they cannot be called after you cleanup a minor.
>>   */
>> -int drm_sysfs_device_add(struct drm_minor *minor)
>> +struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
>>  {
>> - char *minor_str;
>> + const char *minor_str;
>> + struct device *kdev;
>>   int r;
>>
>>   if (minor->type == DRM_MINOR_CONTROL)
>>  

[PATCH 2/2] drm/exynos: dsi: add LPM (Low Power Mode) transfer support

[Andrzej Hajda]

+CC: Thierry and Alexandre

On 07/18/2014 12:56 PM, Inki Dae wrote:
> This patch adds LPM transfer support for video or command data.
>
> With this patch, Exynos MIPI DSI controller can transfer command or
> video data with HS or LP mode in accordance with mode_flags set
> by LCD Panel driver.
>
> Signed-off-by: Inki Dae 
> Acked-by: Kyungmin Park 
> ---
>  drivers/gpu/drm/exynos/exynos_drm_dsi.c |   22 --
>  1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_dsi.c 
> b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> index 2df3592..b120554 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_dsi.c
> @@ -426,13 +426,28 @@ static int exynos_dsi_enable_clock(struct exynos_dsi 
> *dsi)
>   | DSIM_ESC_PRESCALER(esc_div)
>   | DSIM_LANE_ESC_CLK_EN_CLK
>   | DSIM_LANE_ESC_CLK_EN_DATA(BIT(dsi->lanes) - 1)
> - | DSIM_BYTE_CLK_SRC(0)
> - | DSIM_TX_REQUEST_HSCLK;
> + | DSIM_BYTE_CLK_SRC(0);
> +
> + if (!(dsi->mode_flags & MIPI_DSI_MODE_CMD_LPM))
> + reg |= DSIM_TX_REQUEST_HSCLK;
> +

Maybe I missed sth but it seems to me slightly unrelated to LPM flag.
According to specs some panels can require this clock even in LPM mode.
I wonder if recently introduced  MIPI_DSI_CLOCK_NON_CONTINUOUS wouldn't
handle it.

>   writel(reg, dsi->reg_base + DSIM_CLKCTRL_REG);
>  
>   return 0;
>  }
>  
> +static void exynos_dsi_enable_hs_clock(struct exynos_dsi *dsi,
> + bool enable)
> +{
> + u32 reg = readl(dsi->reg_base + DSIM_CLKCTRL_REG);
> +
> + reg &= ~DSIM_TX_REQUEST_HSCLK;
> + if (enable)
> + reg |= DSIM_TX_REQUEST_HSCLK;
> +
> + writel(reg, dsi->reg_base + DSIM_CLKCTRL_REG);
> +}
> +
>  static void exynos_dsi_disable_clock(struct exynos_dsi *dsi)
>  {
>   u32 reg;
> @@ -575,6 +590,9 @@ static void exynos_dsi_set_display_enable(struct 
> exynos_dsi *dsi, bool enable)
>  {
>   u32 reg;
>  
> + if (!(dsi->mode_flags & MIPI_DSI_MODE_VIDEO_LPM))
> + exynos_dsi_enable_hs_clock(dsi, true);
> +

It does not care about enable argument of the function, I guess it should.

Regards
Andrzej

>   reg = readl(dsi->reg_base + DSIM_MDRESOL_REG);
>   if (enable)
>   reg |= DSIM_MAIN_STAND_BY;

[PATCH] imx-drm: imx-drm-core: add suspend/resume support

[Daniel Vetter]

On Thu, Jul 24, 2014 at 10:54:30AM +0100, Russell King - ARM Linux wrote:
> On Thu, Jul 24, 2014 at 11:47:55AM +0200, Lucas Stach wrote:
> > Am Donnerstag, den 24.07.2014, 17:17 +0800 schrieb Shawn Guo:
> > > HDMI currently stops working after a system suspend/resume cycle.  It
> > > turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get
> > > called from anywhere across suspend/resume cycle.
> > > 
> > > The patch follows what exynos drm driver does to walk the list of
> > > connectors and call their .dpms function from suspend/resume hook.  And
> > > the connectors' .dpms function will in turn filter down to the .dpms
> > > hooks of encoders and CRTCs.
> > > 
> > > With this change, HDMI can continue working after a suspend/resume
> > > cycle.
> > > 
> > > Signed-off-by: Shawn Guo 
> > > ---
> > > Tested with HDMI and LVDS.  It'd be great if someone can help test TVE
> > > to ensure the patch doesn't break anything.
> > > 
> > >  drivers/staging/imx-drm/imx-drm-core.c | 39 
> > > ++
> > >  1 file changed, 39 insertions(+)
> > > 
> > > diff --git a/drivers/staging/imx-drm/imx-drm-core.c 
> > > b/drivers/staging/imx-drm/imx-drm-core.c
> > > index def8280d7ee6..b0ea1f0ed32f 100644
> > > --- a/drivers/staging/imx-drm/imx-drm-core.c
> > > +++ b/drivers/staging/imx-drm/imx-drm-core.c
> > > @@ -696,6 +696,44 @@ static int imx_drm_platform_remove(struct 
> > > platform_device *pdev)
> > >   return 0;
> > >  }
> > >  
> > > +#if CONFIG_PM_SLEEP
> > 
> > use #ifdef
> > 
> > > +static int imx_drm_suspend(struct device *dev)
> > > +{
> > > + struct drm_device *drm_dev = dev_get_drvdata(dev);
> > > + struct drm_connector *connector;
> > > +
> > > + drm_kms_helper_poll_disable(drm_dev);
> > > +
> > 
> > That's ok.
> > 
> > > + drm_modeset_lock_all(drm_dev);
> > > + list_for_each_entry(connector, _dev->mode_config.connector_list, 
> > > head) {
> > > + if (connector->funcs->dpms)
> > > + connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
> > > + }
> > 
> > Don't touch DPMS state here. See below.
> 
> In which case, how else does the hardware get placed into a low power
> mode on suspend?
> 
> DRM has nothing provided, and this is left up to each DRM driver to
> implement (probably because it tends to be very driver specific.)
> i915 for example calls the CRTC DPMS function with DRM_MODE_DPMS_OFF.
> 
> This provides a similar mechanism, but also informs the connector, any
> bridge, and encoder associated with the connector as well as the CRTC
> to place themselves into a low power mode (which is what
> DRM_MODE_DPMS_OFF should be doing anyway.)

Well you need to call internal functions to make sure you can restore the
state again. Not sure any more how that all works with the crtc helpers
and whether they restore dpms state properly at all. i915 uses something
completely different nowadays.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 12/12] drm: make sysfs device always available for minors

[Daniel Vetter]

On Wed, Jul 23, 2014 at 05:26:47PM +0200, David Herrmann wrote:
> For each minor we allocate a sysfs device as minor->kdev. Currently, this
> is allocated and registered in drm_minor_register(). This makes it
> impossible to add sysfs-attributes to the device before it is registered.
> Therefore, they are not added atomically, nor can we move device_add()
> *after* ->load() is called.
> 
> This patch makes minor->kdev available early, but only adds the device
> during minor-registration.
> 
> Signed-off-by: David Herrmann 

Some diff got confused with this one. One comment below, but this is

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/drm_stub.c  | 22 ---
>  drivers/gpu/drm/drm_sysfs.c | 90 
> +++--
>  include/drm/drmP.h  |  3 +-
>  3 files changed, 54 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index 8b24db5..09c6bfb 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -281,9 +281,19 @@ static int drm_minor_alloc(struct drm_device *dev, 
> unsigned int type)
>  
>   minor->index = r;
>  
> + minor->kdev = drm_sysfs_minor_alloc(minor);
> + if (IS_ERR(minor->kdev)) {
> + r = PTR_ERR(minor->kdev);
> + goto err_index;
> + }
> +
>   *drm_minor_get_slot(dev, type) = minor;
>   return 0;
>  
> +err_index:
> + spin_lock_irqsave(_minor_lock, flags);
> + idr_remove(_minors_idr, minor->index);
> + spin_unlock_irqrestore(_minor_lock, flags);
>  err_free:
>   kfree(minor);
>   return r;
> @@ -300,6 +310,7 @@ static void drm_minor_free(struct drm_device *dev, 
> unsigned int type)
>   return;
>  
>   drm_mode_group_destroy(>mode_group);
> + put_device(minor->kdev);
>  
>   spin_lock_irqsave(_minor_lock, flags);
>   idr_remove(_minors_idr, minor->index);
> @@ -327,11 +338,9 @@ static int drm_minor_register(struct drm_device *dev, 
> unsigned int type)
>   return ret;
>   }
>  
> - ret = drm_sysfs_device_add(minor);
> - if (ret) {
> - DRM_ERROR("DRM: Error sysfs_device_add.\n");
> + ret = device_add(minor->kdev);
> + if (ret)
>   goto err_debugfs;
> - }
>  
>   /* replace NULL with @minor so lookups will succeed from now on */
>   spin_lock_irqsave(_minor_lock, flags);
> @@ -352,7 +361,7 @@ static void drm_minor_unregister(struct drm_device *dev, 
> unsigned int type)
>   unsigned long flags;
>  
>   minor = *drm_minor_get_slot(dev, type);
> - if (!minor || !minor->kdev)
> + if (!minor || !device_is_registered(minor->kdev))
>   return;
>  
>   /* replace @minor with NULL so lookups will fail from now on */
> @@ -360,8 +369,9 @@ static void drm_minor_unregister(struct drm_device *dev, 
> unsigned int type)
>   idr_replace(_minors_idr, NULL, minor->index);
>   spin_unlock_irqrestore(_minor_lock, flags);
>  
> + device_del(minor->kdev);
> + dev_set_drvdata(minor->kdev, NULL); /* safety belt */
>   drm_debugfs_cleanup(minor);
> - drm_sysfs_device_remove(minor);
>  }
>  
>  /**
> diff --git a/drivers/gpu/drm/drm_sysfs.c b/drivers/gpu/drm/drm_sysfs.c
> index 7827dad..ab1a5f6 100644
> --- a/drivers/gpu/drm/drm_sysfs.c
> +++ b/drivers/gpu/drm/drm_sysfs.c
> @@ -493,71 +493,55 @@ static void drm_sysfs_release(struct device *dev)
>  }
>  
>  /**
> - * drm_sysfs_device_add - adds a class device to sysfs for a character driver
> - * @dev: DRM device to be added
> - * @head: DRM head in question
> + * drm_sysfs_minor_alloc() - Allocate sysfs device for given minor
> + * @minor: minor to allocate sysfs device for
>   *
> - * Add a DRM device to the DRM's device model class.  We use @dev's PCI 
> device
> - * as the parent for the Linux device, and make sure it has a file containing
> - * the driver we're using (for userspace compatibility).
> + * This allocates a new sysfs device for @minor and returns it. The device is
> + * not registered nor linked. The caller has to use device_add() and
> + * device_del() to register and unregister it.
> + *
> + * Note that dev_get_drvdata() on the new device will return the minor.
> + * However, the device does not hold a ref-count to the minor nor to the
> + * underlying drm_device. This is unproblematic as long as you access the
> + * private data only in sysfs callbacks. device_del() disables those
> + * synchronously, so they cannot be called after you cleanup a minor.
>   */
> -int drm_sysfs_device_add(struct drm_minor *minor)
> +struct device *drm_sysfs_minor_alloc(struct drm_minor *minor)
>  {
> - char *minor_str;
> + const char *minor_str;
> + struct device *kdev;
>   int r;
>  
>   if (minor->type == DRM_MINOR_CONTROL)
>   minor_str = "controlD%d";
> -else if (minor->type == DRM_MINOR_RENDER)
> -minor_str = "renderD%d";
> -else
> -

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

[Andrzej Hajda]

Hi Inki,

+CC: Thierry and Alexandre

On 07/18/2014 12:56 PM, 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: low power command transfer
> MIPI_DSI_MODE_VIDEO_LPM: low power video transfer
What is the difference between these two?
Why not just MIPI_DSI_MODE_LPM combined optionally with
MIPI_DSI_MODE_VIDEO ?
Anyway as I understand the only role of this flag is to always trigger
MIPI_DSI_MSG_USE_LPM flag in DSI message. Maybe better is to check both
flags in DSI host, using some helper function.

>
> MIPI DSI spec says,
>  "the host processor controls the desired mode of clock operation.
>   Host protocol and applications control Clock Lane operating mode
>   (High Speed or Low Power mode). System designers are responsible
>   for understanding the clock requirements for peripherals attached
>   to DSI and controlling clock behavior in accordance with those
>   requirements."
>
> Some LCD Panel devices, nt35502a, would need LPM transfer support
> because they should receive some initial commands with LPM by default
> hardware setting.

It would be good to see usage of this flag in the driver. Is it possible
to post it?

I have posted few months ago TC358764 driver[1] which also uses LPM for
initialization, have you look at it.

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

Regards
Andrzej

>
> Signed-off-by: Inki Dae 
> Acked-by: Kyungmin Park 
> ---
>  drivers/gpu/drm/drm_mipi_dsi.c |3 +++
>  include/drm/drm_mipi_dsi.h |4 
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_mipi_dsi.c b/drivers/gpu/drm/drm_mipi_dsi.c
> index e633df2..6b2bbda 100644
> --- a/drivers/gpu/drm/drm_mipi_dsi.c
> +++ b/drivers/gpu/drm/drm_mipi_dsi.c
> @@ -232,6 +232,9 @@ int mipi_dsi_dcs_write(struct mipi_dsi_device *dsi, 
> unsigned int channel,
>   break;
>   }
>  
> + if (dsi->mode_flags & MIPI_DSI_MODE_CMD_LPM)
> + msg.flags = MIPI_DSI_MSG_USE_LPM;
> +
>   return ops->transfer(dsi->host, );
>  }
>  EXPORT_SYMBOL(mipi_dsi_dcs_write);
> diff --git a/include/drm/drm_mipi_dsi.h b/include/drm/drm_mipi_dsi.h
> index 944f33f..1c41e49 100644
> --- a/include/drm/drm_mipi_dsi.h
> +++ b/include/drm/drm_mipi_dsi.h
> @@ -94,6 +94,10 @@ void mipi_dsi_host_unregister(struct mipi_dsi_host *host);
>  #define MIPI_DSI_MODE_VSYNC_FLUSHBIT(8)
>  /* disable EoT packets in HS mode */
>  #define MIPI_DSI_MODE_EOT_PACKET BIT(9)
> +/* command low power mode */
> +#define MIPI_DSI_MODE_CMD_LPMBIT(10)
> +/* video low power mode */
> +#define MIPI_DSI_MODE_VIDEO_LPM  BIT(11)
>  
>  enum mipi_dsi_pixel_format {
>   MIPI_DSI_FMT_RGB888,

[PATCH 04/12] drm: extract legacy ctxbitmap flushing

[Daniel Vetter]

On Thu, Jul 24, 2014 at 11:34:45AM +0200, David Herrmann wrote:
> Hi
> 
> On Wed, Jul 23, 2014 at 9:26 PM, Daniel Vetter  wrote:
> > On Wed, Jul 23, 2014 at 05:26:39PM +0200, David Herrmann wrote:
> >> The ctxbitmap code is only used by legacy drivers so lets try to keep it
> >> as separated as possible. Furthermore, the locking is non-obvious and
> >> kinda weird with ctxlist_mutex *and* struct_mutex. Keeping all ctxbitmap
> >> access in one file is much easier to review and makes drm_release() more
> >> readable.
> >>
> >> Signed-off-by: David Herrmann 
> >
> > I've started to sprinkle _legacy_ over all the functions only used for
> > non-kms drivers, so that the dragon dungeons are clearly marked off. With
> > that this is Reviewed-by: Daniel Vetter .
> 
> Yeah, that sounds good. But I'd prefer if we keep this patch as-is and
> I will send a follow-up which renames all "drm_ctxbitmap_*" functions
> to "drm_legacy_ctxbitmap_*". All those are internal to DRM-core so I
> can rename them all together and add drivers/gpu/drm/drm-legacy.h.

Sounds like a good plan, so r-b on the patch as-is.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 03/12] drm: call ->firstopen() before ->open()

[Daniel Vetter]

On Thu, Jul 24, 2014 at 11:31:05AM +0200, David Herrmann wrote:
> Hi
> 
> On Wed, Jul 23, 2014 at 9:25 PM, Daniel Vetter  wrote:
> > On Wed, Jul 23, 2014 at 05:26:38PM +0200, David Herrmann wrote:
> >> Lets order things correctly:
> >>  ->load()
> >>->fistopen()
> >>  ->open()
> >>  ->close()
> >>->lastclose()
> >>  ->unload()
> >>
> >> This doesn't change much as only savage and radeon use ->firstopen() and
> >> they just do map-initialization. Therefore, the global drm mutex makes
> >> sure there cannot be any other f_op between ->open() and ->firstopen().
> >> However, once we get rid of that lock, we really want ->firstopen() to
> >> initialize the device before ->open() is called.
> >>
> >> Furthermore, this fixes the clean-up path in drm_open(). We currently
> >> don't cleanup the drm_file object if ->firstopen() fails.
> >>
> >> Signed-off-by: David Herrmann 
> >> ---
> >>  drivers/gpu/drm/drm_fops.c | 139 
> >> +
> >>  include/drm/drmP.h |   2 +-
> >>  2 files changed, 66 insertions(+), 75 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> >> index 021fe5d..8e73519 100644
> >> --- a/drivers/gpu/drm/drm_fops.c
> >> +++ b/drivers/gpu/drm/drm_fops.c
> >> @@ -45,7 +45,7 @@ EXPORT_SYMBOL(drm_global_mutex);
> >>
> >>  static int drm_open_helper(struct file *filp, struct drm_minor *minor);
> >>
> >> -static int drm_setup(struct drm_device * dev)
> >> +static int drm_firstopen(struct drm_device * dev)
> >
> > All the stuff in here is only for legacy drivers. Imo we should rename
> > this to drm_legacy_setup and hide it harder.
> >
> > Also touching the init ordering for ums drivers is a bit risky, I'd advise
> > against it. firstopen is officially dead for kms driver and really there's
> > nothing legit you can do in there. imx abused until they've switched over
> > to the component framework.
> >
> > I'd just drop this one tbh.
> 
> I did a driver audit when writing this patch, there're only 2 drivers
> using firstopen:
> 
>  * radeon_cp: sets two fields of dev_private and calls drm_addmap() on
> those. In lastclose(), it calls drm_rmmap()
>  * savage: sets several fields in dev_private, calls
> arch_phys_wc_add() for some regions and requests a drm-map for those
> via drm_addmap(). On lastclose(), it reverts exactly those steps.
> 
> Taking into account that firstopen() just runs with drm_device
> context, not with drm_file context, I really think we should be
> calling it _before_ doing ->open(). I even think the drivers would
> fail horribly if we don't do this patch and some other user-context
> sneaks in between both calls (currently impossible thanks to
> drm_global_mutex).
> 
> Both ->firstopen() callbacks are fairly trivial. In favor of making
> the error-paths work in drm_open() I'd really like to get this in.
> This patch is preparing for my drm_file rework that can make
> drm_dev_unregister() stop all open files immediately.
> 
> But I honestly don't care much for UMS drivers, so if you NACK this,
> I'd just ignore the error code and add a comment that we don't do
> error-handling for UMS.

Yeah, I prefer just labels for dragons instead of poking them ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 11/12] drm: make minor->index available early

[David Herrmann]

Hi

On Thu, Jul 24, 2014 at 12:03 PM, Daniel Vetter  wrote:
> On Wed, Jul 23, 2014 at 05:26:46PM +0200, David Herrmann wrote:
>> Instead of allocating the minor-index during registration, we now do this
>> during allocation. This way, debug-messages between minor-allocation and
>> minor-registration will now use the correct minor instead of 0. Same is
>> done for unregistration vs. free, so debug-messages between
>> device-shutdown and device-destruction show proper indices.
>>
>> Even though minor-indices are allocated early, we don't enable minor
>> lookup early. Instead, we keep the entry set to NULL and replace it during
>> registration / unregistration. This way, the index is allocated but lookup
>> only works if registered.
>>
>> Signed-off-by: David Herrmann 
>> ---
>>  drivers/gpu/drm/drm_stub.c | 84 
>> +-
>>  1 file changed, 46 insertions(+), 38 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
>> index b249f14..8b24db5 100644
>> --- a/drivers/gpu/drm/drm_stub.c
>> +++ b/drivers/gpu/drm/drm_stub.c
>> @@ -256,6 +256,8 @@ static struct drm_minor **drm_minor_get_slot(struct 
>> drm_device *dev,
>>  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>>  {
>>   struct drm_minor *minor;
>> + unsigned long flags;
>> + int r;
>>
>>   minor = kzalloc(sizeof(*minor), GFP_KERNEL);
>>   if (!minor)
>> @@ -264,57 +266,68 @@ static int drm_minor_alloc(struct drm_device *dev, 
>> unsigned int type)
>>   minor->type = type;
>>   minor->dev = dev;
>>
>> + idr_preload(GFP_KERNEL);
>> + spin_lock_irqsave(_minor_lock, flags);
>> + r = idr_alloc(_minors_idr,
>> +   NULL,
>> +   64 * type,
>> +   64 * (type + 1),
>> +   GFP_NOWAIT);
>> + spin_unlock_irqrestore(_minor_lock, flags);
>> + idr_preload_end();
>> +
>> + if (r < 0)
>> + goto err_free;
>> +
>> + minor->index = r;
>> +
>>   *drm_minor_get_slot(dev, type) = minor;
>>   return 0;
>> +
>> +err_free:
>> + kfree(minor);
>> + return r;
>>  }
>>
>>  static void drm_minor_free(struct drm_device *dev, unsigned int type)
>>  {
>> - struct drm_minor **slot;
>> + struct drm_minor **slot, *minor;
>> + unsigned long flags;
>>
>>   slot = drm_minor_get_slot(dev, type);
>> - if (*slot) {
>> - drm_mode_group_destroy(&(*slot)->mode_group);
>> - kfree(*slot);
>> - *slot = NULL;
>> - }
>> + minor = *slot;
>> + if (!minor)
>> + return;
>> +
>> + drm_mode_group_destroy(>mode_group);
>> +
>> + spin_lock_irqsave(_minor_lock, flags);
>> + idr_remove(_minors_idr, minor->index);
>> + spin_unlock_irqrestore(_minor_lock, flags);
>
> I don't understand why you do the idr removal in stages too. Otherwise
> this looks good.

If you call drm_minor_unregister(), we now disable lookup but keep
minor->index. If I also released the ID, a new drm_minor might get
access to it before we call drm_minor_free on the old one. This might
cause misleading debug-messages as both minor objects have the same
index. This is not really a problem, but kinda ugly.

> Aside: If two drivers load concurrently (i.e. in the brave new world
> withou drm_global_mutex) we might end up with interleaved minor ids. Dunno
> whether we'll care since userspace should use udev/sysfs lookups anyway.
> igt sometimes doesn't ;-)

I did post a patch some time ago that makes minor-ID allocations
predictable. I got a NACK from you, so this one is one you ;) But I
agree, we really should fix user-space instead of making random IDs
predictable.

Thanks
David

[PATCH] drm: Fix race when checking for fb in the generic kms obj lookup

[Daniel Vetter]

In my review of

commit 98f75de40e9d83c3a90d294b8fd25fa2874212a9
Author: Rob Clark 
Date:   Fri May 30 11:37:03 2014 -0400

drm: add object property typ

I asked for a check to make sure that we never leak an fb from the
generic mode object lookup since those have completely different
lifetime rules. Rob added it, but outside of the idr mutex, which
means that our dereference of obj->type can already chase free'd
memory.

Somehow I didn't spot this, so fix this asap.

v2: Simplify the conditionals as suggested by Chris.

Cc: Rob Clark 
Cc: Chris Wilson 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_crtc.c | 11 ++-
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f0a47907..d87df8836aa5 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -426,8 +426,12 @@ static struct drm_mode_object *_object_find(struct 
drm_device *dev,

mutex_lock(>mode_config.idr_mutex);
obj = idr_find(>mode_config.crtc_idr, id);
-   if (!obj || (type != DRM_MODE_OBJECT_ANY && obj->type != type) ||
-   (obj->id != id))
+   if (obj && type != DRM_MODE_OBJECT_ANY && obj->type != type)
+   obj = NULL;
+   if (obj && obj->id != id)
+   obj = NULL;
+   /* don't leak out unref'd fb's */
+   if (obj && (obj->type == DRM_MODE_OBJECT_FB))
obj = NULL;
mutex_unlock(>mode_config.idr_mutex);

@@ -454,9 +458,6 @@ struct drm_mode_object *drm_mode_object_find(struct 
drm_device *dev,
 * function.*/
WARN_ON(type == DRM_MODE_OBJECT_FB);
obj = _object_find(dev, id, type);
-   /* don't leak out unref'd fb's */
-   if (obj && (obj->type == DRM_MODE_OBJECT_FB))
-   obj = NULL;
return obj;
 }
 EXPORT_SYMBOL(drm_mode_object_find);
-- 
2.0.1

[PATCH 11/12] drm: make minor->index available early

[Daniel Vetter]

On Wed, Jul 23, 2014 at 05:26:46PM +0200, David Herrmann wrote:
> Instead of allocating the minor-index during registration, we now do this
> during allocation. This way, debug-messages between minor-allocation and
> minor-registration will now use the correct minor instead of 0. Same is
> done for unregistration vs. free, so debug-messages between
> device-shutdown and device-destruction show proper indices.
> 
> Even though minor-indices are allocated early, we don't enable minor
> lookup early. Instead, we keep the entry set to NULL and replace it during
> registration / unregistration. This way, the index is allocated but lookup
> only works if registered.
> 
> Signed-off-by: David Herrmann 
> ---
>  drivers/gpu/drm/drm_stub.c | 84 
> +-
>  1 file changed, 46 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_stub.c b/drivers/gpu/drm/drm_stub.c
> index b249f14..8b24db5 100644
> --- a/drivers/gpu/drm/drm_stub.c
> +++ b/drivers/gpu/drm/drm_stub.c
> @@ -256,6 +256,8 @@ static struct drm_minor **drm_minor_get_slot(struct 
> drm_device *dev,
>  static int drm_minor_alloc(struct drm_device *dev, unsigned int type)
>  {
>   struct drm_minor *minor;
> + unsigned long flags;
> + int r;
>  
>   minor = kzalloc(sizeof(*minor), GFP_KERNEL);
>   if (!minor)
> @@ -264,57 +266,68 @@ static int drm_minor_alloc(struct drm_device *dev, 
> unsigned int type)
>   minor->type = type;
>   minor->dev = dev;
>  
> + idr_preload(GFP_KERNEL);
> + spin_lock_irqsave(_minor_lock, flags);
> + r = idr_alloc(_minors_idr,
> +   NULL,
> +   64 * type,
> +   64 * (type + 1),
> +   GFP_NOWAIT);
> + spin_unlock_irqrestore(_minor_lock, flags);
> + idr_preload_end();
> +
> + if (r < 0)
> + goto err_free;
> +
> + minor->index = r;
> +
>   *drm_minor_get_slot(dev, type) = minor;
>   return 0;
> +
> +err_free:
> + kfree(minor);
> + return r;
>  }
>  
>  static void drm_minor_free(struct drm_device *dev, unsigned int type)
>  {
> - struct drm_minor **slot;
> + struct drm_minor **slot, *minor;
> + unsigned long flags;
>  
>   slot = drm_minor_get_slot(dev, type);
> - if (*slot) {
> - drm_mode_group_destroy(&(*slot)->mode_group);
> - kfree(*slot);
> - *slot = NULL;
> - }
> + minor = *slot;
> + if (!minor)
> + return;
> +
> + drm_mode_group_destroy(>mode_group);
> +
> + spin_lock_irqsave(_minor_lock, flags);
> + idr_remove(_minors_idr, minor->index);
> + spin_unlock_irqrestore(_minor_lock, flags);

I don't understand why you do the idr removal in stages too. Otherwise
this looks good.

Aside: If two drivers load concurrently (i.e. in the brave new world
withou drm_global_mutex) we might end up with interleaved minor ids. Dunno
whether we'll care since userspace should use udev/sysfs lookups anyway.
igt sometimes doesn't ;-)

> +
> + kfree(minor);
> + *slot = NULL;
>  }
>  
>  static int drm_minor_register(struct drm_device *dev, unsigned int type)
>  {
> - struct drm_minor *new_minor;
> + struct drm_minor *minor;
>   unsigned long flags;
>   int ret;
> - int minor_id;
>  
>   DRM_DEBUG("\n");
>  
> - new_minor = *drm_minor_get_slot(dev, type);
> - if (!new_minor)
> + minor = *drm_minor_get_slot(dev, type);
> + if (!minor)
>   return 0;
>  
> - idr_preload(GFP_KERNEL);
> - spin_lock_irqsave(_minor_lock, flags);
> - minor_id = idr_alloc(_minors_idr,
> -  NULL,
> -  64 * type,
> -  64 * (type + 1),
> -  GFP_NOWAIT);
> - spin_unlock_irqrestore(_minor_lock, flags);
> - idr_preload_end();
> -
> - if (minor_id < 0)
> - return minor_id;
> -
> - new_minor->index = minor_id;
> -
> - ret = drm_debugfs_init(new_minor, minor_id, drm_debugfs_root);
> + ret = drm_debugfs_init(minor, minor->index, drm_debugfs_root);
>   if (ret) {
>   DRM_ERROR("DRM: Failed to initialize /sys/kernel/debug/dri.\n");
> - goto err_id;
> + return ret;
>   }
>  
> - ret = drm_sysfs_device_add(new_minor);
> + ret = drm_sysfs_device_add(minor);
>   if (ret) {
>   DRM_ERROR("DRM: Error sysfs_device_add.\n");
>   goto err_debugfs;
> @@ -322,19 +335,14 @@ static int drm_minor_register(struct drm_device *dev, 
> unsigned int type)
>  
>   /* replace NULL with @minor so lookups will succeed from now on */
>   spin_lock_irqsave(_minor_lock, flags);
> - idr_replace(_minors_idr, new_minor, new_minor->index);
> + idr_replace(_minors_idr, minor, minor->index);
>   spin_unlock_irqrestore(_minor_lock, flags);
>  
> - DRM_DEBUG("new minor assigned %d\n", minor_id);
> + DRM_DEBUG("new minor 

[PATCH 1/7] drm: add atomic fxns

[Thierry Reding]

== connector->dev->mode_config.dpms_property) {
>   if (connector->funcs->dpms)
>   (*connector->funcs->dpms)(connector, (int)value);
>   ret = 0;
> - } else if (connector->funcs->set_property)
> - ret = connector->funcs->set_property(connector, property, 
> value);
> + } else if (connector->funcs->set_property) {
> + ret = connector->funcs->set_property(connector, state,
> + property, value, blob_data);
> + }

Why the extra braces here? There's still only one statement in the
block.

>  /**
> - * drm_mode_getproperty_ioctl - get the current value of a object's property
> + * drm_mode_obj_get_properties_ioctl - get the current value of a object's 
> property
>   * @dev: DRM device
>   * @data: ioctl data
>   * @file_priv: DRM file info

This isn't really introduced by this patch, but isn't this kerneldoc
comment wrong? drm_mode_obj_get_properties_ioctl() seems to return the
values of all properties of an object rather than just one.

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/20140724/5122b236/attachment-0001.sig>

[Bug 75276] Implement VGPR Register Spilling

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

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

--- Comment #30 from Christoph Haag  ---
(In reply to comment #28)
> > LLVM ERROR: ran out of registers during register allocation

I do not get this message on upstream llvm recent revisions.

But every demo segfaults. Mostly in LLVMBuildBitCast().

Running a demo looks like this:


$ DRI_PRIME=1 ./Effects
Using binned.
4.3.0-0+UE4 7038 3077 379 0
Signal 11 caught.
EngineCrashHandler: Signal=11


Exiting due to error
Starting ../../../Engine/Binaries/Linux/CrashReportClient
[1]10932 abort (core dumped)  DRI_PRIME=1 ./Effects


This is still a problem with register spilling that just looks different,
right?
Should I compile with debug symbols and get a complete backtrace or wouldn't
that provide any new information?


(By the way, applying this small patch makes it render almost completely
correct on intel: https://bugs.freedesktop.org/show_bug.cgi?id=78716#c10)

-- 
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/20140724/2a40d8a9/attachment.html>

[PATCH 08/12] drm: don't de-authenticate clients on master-close

[Daniel Vetter]

On Wed, Jul 23, 2014 at 05:26:43PM +0200, David Herrmann wrote:
> If an active DRM-Master closes its device, we deauthenticate all clients
> on that master. However, if an inactive DRM-Master closes its device, we
> do nothing. This is quite inconsistent and breaks several scenarios:
> 
>  1) If this was used as security mechanism, it fails horribly if a master
> closes a device while VT switched away. Furthermore, none of the few
> drivers using ->master_*() callbacks seems to require it, anyway.
> 
>  2) If you spawn weston (or any other non-UMS compositor) in background
> while another compositor is active, both will get assigned to the
> same "drm_master" object. If the foreground compositor now exits, all
> clients of both the foreground AND background compositor will be
> de-authenticated leading to unexpected behavior.
> 
> Stop this non-sense and keep clients authenticated. We don't do this when
> dropping DRM-Master (i.e., switching VTs) so don't do it on active-close
> either!
> 
> Signed-off-by: David Herrmann 

Yeah, we have a bit a confusion about what master does. Iirc back in the
dri days the idea was that the master completely gates access for clients,
but we've long since moved away from this model (if it ever really was
fully implemented in the drm core). Dropping this and making master a pure
priv flag for certain compositor operations (modeset, flink_open, ...)
makes a lot of sense.

Reviewed-by: Daniel Vetter 

> ---
>  drivers/gpu/drm/drm_fops.c | 13 ++---
>  include/drm/drmP.h |  1 -
>  2 files changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index f65087e..ff0a13f 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -252,8 +252,7 @@ static int drm_open_helper(struct file *filp, struct 
> drm_minor *minor)
>   priv->minor = minor;
>  
>   /* for compatibility root is always authenticated */
> - priv->always_authenticated = capable(CAP_SYS_ADMIN);
> - priv->authenticated = priv->always_authenticated;
> + priv->authenticated = capable(CAP_SYS_ADMIN);
>   priv->lock_count = 0;
>  
>   INIT_LIST_HEAD(>lhead);
> @@ -453,20 +452,12 @@ int drm_release(struct inode *inode, struct file *filp)
>  
>   if (drm_is_master(file_priv)) {
>   struct drm_master *master = file_priv->master;
> - struct drm_file *temp;
> -
> - mutex_lock(>struct_mutex);
> - list_for_each_entry(temp, >filelist, lhead) {
> - if ((temp->master == file_priv->master) &&
> - (temp != file_priv))
> - temp->authenticated = 
> temp->always_authenticated;
> - }
>  
>   /**
>* Since the master is disappearing, so is the
>* possibility to lock.
>*/
> -
> + mutex_lock(>struct_mutex);
>   if (master->lock.hw_lock) {
>   if (dev->sigdata.lock == master->lock.hw_lock)
>   dev->sigdata.lock = NULL;
> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index e1bb585..48d2fe7 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -385,7 +385,6 @@ struct drm_prime_file_private {
>  
>  /** File private data */
>  struct drm_file {
> - unsigned always_authenticated :1;
>   unsigned authenticated :1;
>   /* true when the client has asked us to expose stereo 3D mode flags */
>   unsigned stereo_allowed :1;
> -- 
> 2.0.2
> 

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

[PATCH] imx-drm: imx-drm-core: add suspend/resume support

[Marc Kleine-Budde]

On 07/24/2014 11:47 AM, Lucas Stach wrote:
> Am Donnerstag, den 24.07.2014, 17:17 +0800 schrieb Shawn Guo:
>> HDMI currently stops working after a system suspend/resume cycle.  It
>> turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get
>> called from anywhere across suspend/resume cycle.
>>
>> The patch follows what exynos drm driver does to walk the list of
>> connectors and call their .dpms function from suspend/resume hook.  And
>> the connectors' .dpms function will in turn filter down to the .dpms
>> hooks of encoders and CRTCs.
>>
>> With this change, HDMI can continue working after a suspend/resume
>> cycle.
>>
>> Signed-off-by: Shawn Guo 
>> ---
>> Tested with HDMI and LVDS.  It'd be great if someone can help test TVE
>> to ensure the patch doesn't break anything.
>>
>>  drivers/staging/imx-drm/imx-drm-core.c | 39 
>> ++
>>  1 file changed, 39 insertions(+)
>>
>> diff --git a/drivers/staging/imx-drm/imx-drm-core.c 
>> b/drivers/staging/imx-drm/imx-drm-core.c
>> index def8280d7ee6..b0ea1f0ed32f 100644
>> --- a/drivers/staging/imx-drm/imx-drm-core.c
>> +++ b/drivers/staging/imx-drm/imx-drm-core.c
>> @@ -696,6 +696,44 @@ static int imx_drm_platform_remove(struct 
>> platform_device *pdev)
>>  return 0;
>>  }
>>  
>> +#if CONFIG_PM_SLEEP
> 
> use #ifdef

...or remove #if/#ifdef and mark as __maybe_unused

Marc

-- 
Pengutronix e.K.  | Marc Kleine-Budde   |
Industrial Linux Solutions| Phone: +49-231-2826-924 |
Vertretung West/Dortmund  | Fax:   +49-5121-206917- |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |

-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 242 bytes
Desc: OpenPGP digital signature
URL: 
<http://lists.freedesktop.org/archives/dri-devel/attachments/20140724/453066e7/attachment-0001.sig>

[PATCH] [rfc] drm: close race in connector registration

[Dave Airlie]

From: Dave Airlie 

Daniel pointed out with hotplug that userspace could be trying to oops us
as root for lols, and that to be correct we shouldn't register the object
with the idr before we have fully set the connector object up.

His proposed solution was a lot more life changing, this seemed like a simpler
proposition to me, get the connector object id from the idr, but don't
register the object until the drm_connector_register callback.

The open question is whether the drm_mode_object_register needs a bigger lock
than just the idr one, but I can't see why it would, but I can be locking
challenged.

Signed-off-by: Dave Airlie 
---
 drivers/gpu/drm/drm_crtc.c | 25 +
 1 file changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 1ccf5cb..9f8cc1a 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -376,13 +376,14 @@ EXPORT_SYMBOL(drm_get_format_name);
  * New unique (relative to other objects in @dev) integer identifier for the
  * object.
  */
-int drm_mode_object_get(struct drm_device *dev,
-   struct drm_mode_object *obj, uint32_t obj_type)
+static int drm_mode_object_get_noreg(struct drm_device *dev,
+struct drm_mode_object *obj, uint32_t 
obj_type,
+bool noreg)
 {
int ret;

mutex_lock(>mode_config.idr_mutex);
-   ret = idr_alloc(>mode_config.crtc_idr, obj, 1, 0, GFP_KERNEL);
+   ret = idr_alloc(>mode_config.crtc_idr, noreg ? NULL : obj, 1, 0, 
GFP_KERNEL);
if (ret >= 0) {
/*
 * Set up the object linking under the protection of the idr
@@ -396,6 +397,20 @@ int drm_mode_object_get(struct drm_device *dev,
return ret < 0 ? ret : 0;
 }

+int drm_mode_object_get(struct drm_device *dev,
+   struct drm_mode_object *obj, uint32_t obj_type)
+{
+   return drm_mode_object_get_noreg(dev, obj, obj_type, false);
+}
+
+static void drm_mode_object_register(struct drm_device *dev,
+struct drm_mode_object *obj)
+{
+   mutex_lock(>mode_config.idr_mutex);
+   idr_replace(>mode_config.crtc_idr, obj, obj->id);
+   mutex_unlock(>mode_config.idr_mutex);
+}
+
 /**
  * drm_mode_object_put - free a modeset identifer
  * @dev: DRM device
@@ -849,7 +864,7 @@ int drm_connector_init(struct drm_device *dev,

drm_modeset_lock_all(dev);

-   ret = drm_mode_object_get(dev, >base, 
DRM_MODE_OBJECT_CONNECTOR);
+   ret = drm_mode_object_get_noreg(dev, >base, 
DRM_MODE_OBJECT_CONNECTOR, true);
if (ret)
goto out_unlock;

@@ -942,6 +957,8 @@ int drm_connector_register(struct drm_connector *connector)
 {
int ret;

+   drm_mode_object_register(connector->dev, >base);
+
ret = drm_sysfs_connector_add(connector);
if (ret)
return ret;
-- 
1.9.3

[PATCH 07/12] drm: drop redundant drm_file->is_master

[Daniel Vetter]

On Wed, Jul 23, 2014 at 05:26:42PM +0200, David Herrmann wrote:
> The drm_file->is_master field is redundant as it's equivalent to:
> drm_file->master && drm_file->master == drm_file->minor->master
> 
> 1) "=>"
>   Whenever we set drm_file->is_master, we also set:
>   drm_file->minor->master = drm_file->master;
> 
>   Whenever we clear drm_file->is_master, we also call:
>   drm_master_put(_file->minor->master);
>   which implicitly clears it to NULL.
> 
> 2) "<="
>   minor->master cannot be set if it is non-NULL. Therefore, it stays as
>   is unless a file drops it.
> 
>   If minor->master is NULL, it is only set by places that also adjust
>   drm_file->is_master.
> 
> Therefore, we can safely drop is_master and replace it by an inline helper
> that matches:
> drm_file->master && drm_file->master == drm_file->minor->master
> 
> Signed-off-by: David Herrmann 

Docbook for drm_is_master is missing, otherwise Reviewed-by: Daniel Vetter 


And one question below which doesn't really matter for this patch here.
See below
-Daniel

[snip]

> diff --git a/include/drm/drmP.h b/include/drm/drmP.h
> index d91e09f..e1bb585 100644
> --- a/include/drm/drmP.h
> +++ b/include/drm/drmP.h
> @@ -387,8 +387,6 @@ struct drm_prime_file_private {
>  struct drm_file {
>   unsigned always_authenticated :1;
>   unsigned authenticated :1;
> - /* Whether we're master for a minor. Protected by master_mutex */
> - unsigned is_master :1;
>   /* true when the client has asked us to expose stereo 3D mode flags */
>   unsigned stereo_allowed :1;
>   /*
> @@ -1034,7 +1032,7 @@ struct drm_device {
>   /** \name Locks */
>   /*@{ */
>   struct mutex struct_mutex;  /**< For others */
> - struct mutex master_mutex;  /**< For drm_minor::master and 
> drm_file::is_master */
> + struct mutex master_mutex;  /**< For drm_minor::master */
>   /*@} */
>  
>   /** \name Usage Counters */
> @@ -1172,6 +1170,11 @@ static inline bool drm_is_primary_client(const struct 
> drm_file *file_priv)
>   return file_priv->minor->type == DRM_MINOR_LEGACY;
>  }
>  

Docbook here please ...
> +static inline bool drm_is_master(const struct drm_file *file)
> +{

Hm, we don't have any means to synchronize is_master checks with
concurrent ioctls and stuff. Do we care? Orthogonal issue really.

> + return file->master && file->master == file->minor->master;
> +}
> +
>  /**/
>  /** \name Internal function definitions */
>  /*@{*/
> -- 
> 2.0.2
> 

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

[PATCH] imx-drm: imx-drm-core: add suspend/resume support

[Lucas Stach]

Am Donnerstag, den 24.07.2014, 17:17 +0800 schrieb Shawn Guo:
> HDMI currently stops working after a system suspend/resume cycle.  It
> turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get
> called from anywhere across suspend/resume cycle.
> 
> The patch follows what exynos drm driver does to walk the list of
> connectors and call their .dpms function from suspend/resume hook.  And
> the connectors' .dpms function will in turn filter down to the .dpms
> hooks of encoders and CRTCs.
> 
> With this change, HDMI can continue working after a suspend/resume
> cycle.
> 
> Signed-off-by: Shawn Guo 
> ---
> Tested with HDMI and LVDS.  It'd be great if someone can help test TVE
> to ensure the patch doesn't break anything.
> 
>  drivers/staging/imx-drm/imx-drm-core.c | 39 
> ++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/drivers/staging/imx-drm/imx-drm-core.c 
> b/drivers/staging/imx-drm/imx-drm-core.c
> index def8280d7ee6..b0ea1f0ed32f 100644
> --- a/drivers/staging/imx-drm/imx-drm-core.c
> +++ b/drivers/staging/imx-drm/imx-drm-core.c
> @@ -696,6 +696,44 @@ static int imx_drm_platform_remove(struct 
> platform_device *pdev)
>   return 0;
>  }
>  
> +#if CONFIG_PM_SLEEP

use #ifdef

> +static int imx_drm_suspend(struct device *dev)
> +{
> + struct drm_device *drm_dev = dev_get_drvdata(dev);
> + struct drm_connector *connector;
> +
> + drm_kms_helper_poll_disable(drm_dev);
> +

That's ok.

> + drm_modeset_lock_all(drm_dev);
> + list_for_each_entry(connector, _dev->mode_config.connector_list, 
> head) {
> + if (connector->funcs->dpms)
> + connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
> + }

Don't touch DPMS state here. See below.

> + drm_modeset_unlock_all(drm_dev);
> +
> + return 0;
> +}
> +
> +static int imx_drm_resume(struct device *dev)
> +{
> + struct drm_device *drm_dev = dev_get_drvdata(dev);
> + struct drm_connector *connector;
> +
> + drm_modeset_lock_all(drm_dev);
> + list_for_each_entry(connector, _dev->mode_config.connector_list, 
> head) {
> + if (connector->funcs->dpms)
> + connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
> + }
> + drm_modeset_unlock_all(drm_dev);
> +
This forcefully enables all connectors, which might not have been the
state before suspend. Have a look at simply using
drm_helper_resume_force_mode(), which should do the right thing.

> + drm_kms_helper_poll_enable(drm_dev);
> +
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(imx_drm_pm_ops, imx_drm_suspend, imx_drm_resume);
> +
>  static const struct of_device_id imx_drm_dt_ids[] = {
>   { .compatible = "fsl,imx-display-subsystem", },
>   { /* sentinel */ },
> @@ -708,6 +746,7 @@ static struct platform_driver imx_drm_pdrv = {
>   .driver = {
>   .owner  = THIS_MODULE,
>   .name   = "imx-drm",
> + .pm = _drm_pm_ops,
>   .of_match_table = imx_drm_dt_ids,
>   },
>  };

Regards,
Lucas
-- 
Pengutronix e.K. | Lucas Stach |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

[PATCH v2 00/25] AMDKFD kernel driver

[Jerome Glisse]

On Thu, Jul 24, 2014 at 01:01:41AM +0300, Oded Gabbay wrote:
> On 24/07/14 00:46, Bridgman, John wrote:
> > 
> >> -Original Message- From: dri-devel
> >> [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of Jesse
> >> Barnes Sent: Wednesday, July 23, 2014 5:00 PM To:
> >> dri-devel at lists.freedesktop.org Subject: Re: [PATCH v2 00/25]
> >> AMDKFD kernel driver
> >> 
> >> On Mon, 21 Jul 2014 19:05:46 +0200 daniel at ffwll.ch (Daniel
> >> Vetter) wrote:
> >> 
> >>> On Mon, Jul 21, 2014 at 11:58:52AM -0400, Jerome Glisse wrote:
>  On Mon, Jul 21, 2014 at 05:25:11PM +0200, Daniel Vetter wrote:
> > On Mon, Jul 21, 2014 at 03:39:09PM +0200, Christian K?nig
> > wrote:
> >> Am 21.07.2014 14:36, schrieb Oded Gabbay:
> >>> On 20/07/14 20:46, Jerome Glisse wrote:
> >> 
> >> [snip!!]
> > My BlackBerry thumb thanks you ;)
> >> 
> >> 
> >> The main questions here are if it's avoid able to pin down
> >> the memory and if the memory is pinned down at driver load,
> >> by request from userspace or by anything else.
> >> 
> >> As far as I can see only the "mqd per userspace queue"
> >> might be a bit questionable, everything else sounds
> >> reasonable.
> > 
> > Aside, i915 perspective again (i.e. how we solved this):
> > When scheduling away from contexts we unpin them and put them
> > into the lru. And in the shrinker we have a last-ditch
> > callback to switch to a default context (since you can't ever
> > have no context once you've started) which means we can evict
> > any context object if it's
> >> getting in the way.
>  
>  So Intel hardware report through some interrupt or some channel
>  when it is not using a context ? ie kernel side get
>  notification when some user context is done executing ?
> >>> 
> >>> Yes, as long as we do the scheduling with the cpu we get
> >>> interrupts for context switches. The mechanic is already
> >>> published in the execlist patches currently floating around. We
> >>> get a special context switch interrupt.
> >>> 
> >>> But we have this unpin logic already on the current code where
> >>> we switch contexts through in-line cs commands from the kernel.
> >>> There we obviously use the normal batch completion events.
> >> 
> >> Yeah and we can continue that going forward.  And of course if your
> >> hw can do page faulting, you don't need to pin the normal data
> >> buffers.
> >> 
> >> Usually there are some special buffers that need to be pinned for
> >> longer periods though, anytime the context could be active.  Sounds
> >> like in this case the userland queues, which makes some sense.  But
> >> maybe for smaller systems the size limit could be clamped to
> >> something smaller than 128M.  Or tie it into the rlimit somehow,
> >> just like we do for mlock() stuff.
> >> 
> > Yeah, even the queues are in pageable memory, it's just a ~256 byte
> > structure per queue (the Memory Queue Descriptor) that describes the
> > queue to hardware, plus a couple of pages for each process using HSA
> > to hold things like doorbells. Current thinking is to limit #
> > processes using HSA to ~256 and #queues per process to ~1024 by
> > default in the initial code, although my guess is that we could take
> > the #queues per process default limit even lower.
> > 
> 
> So my mistake. struct cik_mqd is actually 604 bytes, and it is allocated
> on 256 boundary.
> I had in mind to reserve 64MB of gart by default, which translates to
> 512 queues per process, with 128 processes. Add 2 kernel module
> parameters, # of max-queues-per-process and # of max-processes (default
> is, as I said, 512 and 128) for better control of system admin.
> 

So as i said somewhere else in this thread, this should not be reserved
but use a special allocation. Any HSA GPU use virtual address space for
userspace so only issue is for kernel side GTT.

What i would like is seeing radeon pinned GTT allocation at bottom of
GTT space (ie all ring buffer and the ib pool buffer). Then have an
allocator that allocate new queue from top of GTT address space and
grow to the bottom.

It should not staticly reserved 64M or anything. When doing allocation
it should move any ttm buffer that are in the region it want to allocate
to a different location.


As this needs some work, i am not against reserving some small amount
(couple MB) as a first stage but anything more would need a proper solution
like the one i just described.

Cheers,
J?r?me

[PATCH] imx-drm: imx-drm-core: add suspend/resume support

[Philipp Zabel]

Hi Shawn,

Am Donnerstag, den 24.07.2014, 17:17 +0800 schrieb Shawn Guo:
> HDMI currently stops working after a system suspend/resume cycle.  It
> turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get
> called from anywhere across suspend/resume cycle.
> 
> The patch follows what exynos drm driver does to walk the list of
> connectors and call their .dpms function from suspend/resume hook.  And
> the connectors' .dpms function will in turn filter down to the .dpms
> hooks of encoders and CRTCs.
> 
> With this change, HDMI can continue working after a suspend/resume
> cycle.
> 
> Signed-off-by: Shawn Guo 
> ---
> Tested with HDMI and LVDS.  It'd be great if someone can help test TVE
> to ensure the patch doesn't break anything.

Tested-by: Philipp Zabel 

on i.MX53-QSB with VGA via TVE.

regards
Philipp

[PATCH 04/12] drm: extract legacy ctxbitmap flushing

[David Herrmann]

Hi

On Wed, Jul 23, 2014 at 9:26 PM, Daniel Vetter  wrote:
> On Wed, Jul 23, 2014 at 05:26:39PM +0200, David Herrmann wrote:
>> The ctxbitmap code is only used by legacy drivers so lets try to keep it
>> as separated as possible. Furthermore, the locking is non-obvious and
>> kinda weird with ctxlist_mutex *and* struct_mutex. Keeping all ctxbitmap
>> access in one file is much easier to review and makes drm_release() more
>> readable.
>>
>> Signed-off-by: David Herrmann 
>
> I've started to sprinkle _legacy_ over all the functions only used for
> non-kms drivers, so that the dragon dungeons are clearly marked off. With
> that this is Reviewed-by: Daniel Vetter .

Yeah, that sounds good. But I'd prefer if we keep this patch as-is and
I will send a follow-up which renames all "drm_ctxbitmap_*" functions
to "drm_legacy_ctxbitmap_*". All those are internal to DRM-core so I
can rename them all together and add drivers/gpu/drm/drm-legacy.h.

Comments?
David

[PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Thierry Reding]

ruct mipi_dsi_msg. I think it
> > should be raw DSI data (that is, including the header and the payload)
> > so that the only job of drivers is to write it into the corresponding
> > FIFO registers and start the transmission.
> 
> This or just helpers in DSI host to calculate raw DSI data.

I think either way would work. I slightly prefer the direct layering
because we're implementing a standard protocol on top of another
standard protocol. So it's fairly similar to networking protocols or
even filesystems on top of the block layer.

Therefore I think it makes sense to have an API that takes DCS inputs,
such as command and payload, translates them to DSI and passes raw DSI
to the driver, since presumably that's what the driver needs. That way
we can move the protocol bits into common functions and let drivers do
what they do best: interface with the hardware.

> What about ECC? Should it be present also in tx buffer?

ECC and CS are somewhat special. Hardware will probably provide a way to
offload ECC and CS computations in the majority of cases, so it doesn't
make much sense to compute them by default. I see two ways to achieve
that: a) provide flags in struct mipi_dsi_host for supported features so
that the protocol code can compute it if necessary or b) provide helpers
that take a raw DSI packet and compute its CS or ECC.

While I couldn't find an explicit reference to a CS or ECC enable bit,
there also doesn't seem to be any code to compute it, so I assume it's
always enabled (or at least by default). Tegra can also compute them in
hardware, so I don't think we need to concern ourselves with that at
this point.

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/20140724/5d4a823f/attachment.sig>

[PATCH 03/12] drm: call ->firstopen() before ->open()

[David Herrmann]

Hi

On Wed, Jul 23, 2014 at 9:25 PM, Daniel Vetter  wrote:
> On Wed, Jul 23, 2014 at 05:26:38PM +0200, David Herrmann wrote:
>> Lets order things correctly:
>>  ->load()
>>->fistopen()
>>  ->open()
>>  ->close()
>>->lastclose()
>>  ->unload()
>>
>> This doesn't change much as only savage and radeon use ->firstopen() and
>> they just do map-initialization. Therefore, the global drm mutex makes
>> sure there cannot be any other f_op between ->open() and ->firstopen().
>> However, once we get rid of that lock, we really want ->firstopen() to
>> initialize the device before ->open() is called.
>>
>> Furthermore, this fixes the clean-up path in drm_open(). We currently
>> don't cleanup the drm_file object if ->firstopen() fails.
>>
>> Signed-off-by: David Herrmann 
>> ---
>>  drivers/gpu/drm/drm_fops.c | 139 
>> +
>>  include/drm/drmP.h |   2 +-
>>  2 files changed, 66 insertions(+), 75 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>> index 021fe5d..8e73519 100644
>> --- a/drivers/gpu/drm/drm_fops.c
>> +++ b/drivers/gpu/drm/drm_fops.c
>> @@ -45,7 +45,7 @@ EXPORT_SYMBOL(drm_global_mutex);
>>
>>  static int drm_open_helper(struct file *filp, struct drm_minor *minor);
>>
>> -static int drm_setup(struct drm_device * dev)
>> +static int drm_firstopen(struct drm_device * dev)
>
> All the stuff in here is only for legacy drivers. Imo we should rename
> this to drm_legacy_setup and hide it harder.
>
> Also touching the init ordering for ums drivers is a bit risky, I'd advise
> against it. firstopen is officially dead for kms driver and really there's
> nothing legit you can do in there. imx abused until they've switched over
> to the component framework.
>
> I'd just drop this one tbh.

I did a driver audit when writing this patch, there're only 2 drivers
using firstopen:

 * radeon_cp: sets two fields of dev_private and calls drm_addmap() on
those. In lastclose(), it calls drm_rmmap()
 * savage: sets several fields in dev_private, calls
arch_phys_wc_add() for some regions and requests a drm-map for those
via drm_addmap(). On lastclose(), it reverts exactly those steps.

Taking into account that firstopen() just runs with drm_device
context, not with drm_file context, I really think we should be
calling it _before_ doing ->open(). I even think the drivers would
fail horribly if we don't do this patch and some other user-context
sneaks in between both calls (currently impossible thanks to
drm_global_mutex).

Both ->firstopen() callbacks are fairly trivial. In favor of making
the error-paths work in drm_open() I'd really like to get this in.
This patch is preparing for my drm_file rework that can make
drm_dev_unregister() stop all open files immediately.

But I honestly don't care much for UMS drivers, so if you NACK this,
I'd just ignore the error code and add a comment that we don't do
error-handling for UMS.

Cheers
David

[PATCH] gpu: drm: omapdrm: Use %pad format for values of the dma_addr_t type

[Joe Perches]

On Thu, 2014-07-24 at 21:58 +0400, matwey at sai.msu.ru wrote:
> The format change is to fix the following compilation issue:

Just a trivial note:

> diff --git a/drivers/gpu/drm/omapdrm/omap_gem.c 
> b/drivers/gpu/drm/omapdrm/omap_gem.c
[]
> @@ -791,7 +791,7 @@ int omap_gem_get_paddr(struct drm_gem_object *obj,
>   omap_obj->paddr = tiler_ssptr(block);
>   omap_obj->block = block;
>  
> - DBG("got paddr: %08x", omap_obj->paddr);
> + DBG("got paddr: %pad", &(omap_obj->paddr));

>bar does not need parentheses.

Most uses in the kernel do not have them.
It seems to be ~25:1 without to with.

> @@ -985,9 +985,9 @@ void omap_gem_describe(struct drm_gem_object *obj, struct 
> seq_file *m)
>  
>   off = drm_vma_node_start(>vma_node);
>  
> - seq_printf(m, "%08x: %2d (%2d) %08llx %08Zx (%2d) %p %4d",
> + seq_printf(m, "%08x: %2d (%2d) %08llx %pad (%2d) %p %4d",
>   omap_obj->flags, obj->name, 
> obj->refcount.refcount.counter,
> - off, omap_obj->paddr, omap_obj->paddr_cnt,
> + off, &(omap_obj->paddr), omap_obj->paddr_cnt,

etc...

[PATCH] [rfc] drm: close race in connector registration

[David Herrmann]

Hi

On Thu, Jul 24, 2014 at 10:49 AM, Dave Airlie  wrote:
>>
>> This is the same we do for minor-objects, so fine with me. I'd prefer
>> if the registration is done last in drm_connector_register(), not
>> first, but I'm not sure the debugfs hooks work without the connector
>> available in the lookup tables. Maybe worth a try.
>
> I was worried sysfs registration would trigger something in userspace
> to life, which wuold then fail because the connector wasn't there.

Good point. So all fine with me the way it is.

Thanks
David

[PATCH 0/7] prepare for atomic.. the great propertyification

[Rob Clark]

On Thu, Jul 24, 2014 at 10:00 AM, Daniel Vetter  wrote:
> On Thu, Jul 24, 2014 at 09:36:20AM -0400, Rob Clark wrote:
>> On Thu, Jul 24, 2014 at 8:25 AM, Daniel Vetter  wrote:
>> > On Wed, Jul 23, 2014 at 03:38:13PM -0400, Rob Clark wrote:
>> >> This is mostly just a rebase+resend.  Was going to send it a bit earlier
>> >> but had a few things to fix up as a result of the rebase.
>> >>
>> >> At this point, I think next steps are roughly:
>> >> 1) introduce plane->mutex
>> >> 2) decide what we want to do about events
>> >> 3) add actual ioctl
>> >>
>> >> I think we could shoot for merging this series next, and then adding
>> >> plane->mutex in 3.18?
>> >>
>> >> Before we add the ioctl, I think we want to sort out events for updates
>> >> to non-primary layers, and what the interface to drivers should look like.
>> >> Ie. just add event to ->update_plane() or should we completely ditch
>> >> ->page_flip() and ->update_plane()?
>> >>
>> >> Technically, I think we could get away without a new API and just let
>> >> drivers grab all the events in their ->atomic_commit(), but I suspect
>> >> core could provide something more useful to drivers.  I guess it would
>> >> be useful to have a few more drivers converted over to see what makes
>> >> sense.
>> >
>> > Ok so we've had a lot of discussions about this on irc and overall I'm not
>> > terribly happy with what this looks like. I have a few concerns:
>> >
>> > - The entire atomic conversion for drivers is monolithic, at least at the
>> >   interface level. So if you want to do this step-by-step you're forced to
>> >   add hacks and e.g. bypass pageflips until that's implemented. E.g. on
>> >   i915 I see no chance that we'll get atomic ready for all cases (so
>> >   nonblocking flips and multi-crtc and everything on all platforms) in one
>> >   go.
>>
>> So, there interface is *intended* to be monolithic, in a way..  many
>> years ago, I started by adding side by side atomic vs legacy paths,
>> and it was pretty ugly in core.  I'm much happier with the current
>> iteration.
>>
>> A slightly later iteration preserved atomic helpers (so drivers could
>> do completely their own thing).  I ended up dropping that because most
>> of what atomic does is manage the interface to whatever is doing
>> modeset/pageflip/whatever.  I completely expect some changes around
>> the commit stuff..  that part is intended to either be
>> wrapped/extended by the driver or replaced.  Possibly it could be made
>> more clear what drivers are expected to use vs extend or replace.  I
>> expect this to evolve a bit as more drivers are converted.
>>
>> Note that the current atomic "layer" results in 1:1 conversion from
>> old userspace API to legacy vfuncs.  This way what is on top of atomic
>> API and what is beneath (ie. the driver) has same code paths either
>> way (old or new userspace, converted or not driver).  The edge cases
>> don't come in until atomic ioctl is exposed, and for that I expect a
>> driver flag to enable the new ioctl.  You could even set the flag
>> based on hw generation if you want.
>>
>> > - Related to that is that we force legacy ioctls through atomic. E.g. on
>> >   older i915 platforms I very much expect that we won't ever convert the
>> >   pageflip code to atomic for them and just not support atomic flips of
>> >   cursor + primary plane. At least not at the beginning. I know that's
>> >   different from my stance a year ago, but I've become a bit more
>> >   realistic.
>> >
>> > - The entire conversion is monolithic and we can't do it on a
>> >   driver-by-driver basis. Everyone has to go through the new atomic
>> >   interfaces on a flag day. drm is much bigger now and forcing everyone to
>> >   convert at the same time is really risky. Again I know I've changed my
>> >   mind again, but drm is a lot bigger and there's a lot more drivers that
>> >   implement pageflip and cursors, i.e. stuff that's special.
>>
>> the only flag day part is plugging in atomic functions and couple
>> vfunc API changes around set_prop().. the current design allows for
>> conversion on driver by driver basis.
>>
>> > - The separation between interface marshalling code in the drm core and
>> >   helper functions for drivers to implement stuff is fuzzy at best. Thus
>> >   far we've had an excellent seperation in this are for kms, I don't think
>> >   it's vise to throw that out.
>>
>> Interface marshalling should not be helper.  Everyone needs the same
>> thing, since what is on top is the same.
>
> Erhm I've certainly not meant the interface marshalling to be a helper.
> This started with "The separation ..." after all, so I want inteface
> marshilling as fixed code in the drm core, and everything else outside in
> a separate drm_atomic_helper.c module.

ok, it's entirely possibly that I'm missunderstanding a bit your proposal..

>> > So here's my proposal for how to get this in without breaking the world
>> >
>> > 1. We add a complete new set of ->atomic_foo driver entry points 

[PATCH] imx-drm: imx-drm-core: add suspend/resume support

[Russell King - ARM Linux]

On Thu, Jul 24, 2014 at 11:47:55AM +0200, Lucas Stach wrote:
> Am Donnerstag, den 24.07.2014, 17:17 +0800 schrieb Shawn Guo:
> > HDMI currently stops working after a system suspend/resume cycle.  It
> > turns out that the cause is the imx-hdmi encoder .dpms hook doesn't get
> > called from anywhere across suspend/resume cycle.
> > 
> > The patch follows what exynos drm driver does to walk the list of
> > connectors and call their .dpms function from suspend/resume hook.  And
> > the connectors' .dpms function will in turn filter down to the .dpms
> > hooks of encoders and CRTCs.
> > 
> > With this change, HDMI can continue working after a suspend/resume
> > cycle.
> > 
> > Signed-off-by: Shawn Guo 
> > ---
> > Tested with HDMI and LVDS.  It'd be great if someone can help test TVE
> > to ensure the patch doesn't break anything.
> > 
> >  drivers/staging/imx-drm/imx-drm-core.c | 39 
> > ++
> >  1 file changed, 39 insertions(+)
> > 
> > diff --git a/drivers/staging/imx-drm/imx-drm-core.c 
> > b/drivers/staging/imx-drm/imx-drm-core.c
> > index def8280d7ee6..b0ea1f0ed32f 100644
> > --- a/drivers/staging/imx-drm/imx-drm-core.c
> > +++ b/drivers/staging/imx-drm/imx-drm-core.c
> > @@ -696,6 +696,44 @@ static int imx_drm_platform_remove(struct 
> > platform_device *pdev)
> > return 0;
> >  }
> >  
> > +#if CONFIG_PM_SLEEP
> 
> use #ifdef
> 
> > +static int imx_drm_suspend(struct device *dev)
> > +{
> > +   struct drm_device *drm_dev = dev_get_drvdata(dev);
> > +   struct drm_connector *connector;
> > +
> > +   drm_kms_helper_poll_disable(drm_dev);
> > +
> 
> That's ok.
> 
> > +   drm_modeset_lock_all(drm_dev);
> > +   list_for_each_entry(connector, _dev->mode_config.connector_list, 
> > head) {
> > +   if (connector->funcs->dpms)
> > +   connector->funcs->dpms(connector, DRM_MODE_DPMS_OFF);
> > +   }
> 
> Don't touch DPMS state here. See below.

In which case, how else does the hardware get placed into a low power
mode on suspend?

DRM has nothing provided, and this is left up to each DRM driver to
implement (probably because it tends to be very driver specific.)
i915 for example calls the CRTC DPMS function with DRM_MODE_DPMS_OFF.

This provides a similar mechanism, but also informs the connector, any
bridge, and encoder associated with the connector as well as the CRTC
to place themselves into a low power mode (which is what
DRM_MODE_DPMS_OFF should be doing anyway.)

> > +static int imx_drm_resume(struct device *dev)
> > +{
> > +   struct drm_device *drm_dev = dev_get_drvdata(dev);
> > +   struct drm_connector *connector;
> > +
> > +   drm_modeset_lock_all(drm_dev);
> > +   list_for_each_entry(connector, _dev->mode_config.connector_list, 
> > head) {
> > +   if (connector->funcs->dpms)
> > +   connector->funcs->dpms(connector, DRM_MODE_DPMS_ON);
> > +   }
> > +   drm_modeset_unlock_all(drm_dev);
> > +
> This forcefully enables all connectors, which might not have been the
> state before suspend. Have a look at simply using
> drm_helper_resume_force_mode(), which should do the right thing.

Yes, we agree there.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

[PULL] drm-intel-fixes

[Daniel Vetter]

Hi Dave,

This time in time! Just 32bit-pae fix from Hugh, semaphores fun from Chris
and a fix for runtime pm cherry-picked from next.

Paulo is still working on a fix for runtime pm when X does cursor fun when
the display is off, but that one isn't ready yet.

Cheers, Daniel


The following changes since commit 9a3c4145af32125c5ee39c0272662b47307a8323:

  Linux 3.16-rc6 (2014-07-20 21:04:16 -0700)

are available in the git repository at:

  git://anongit.freedesktop.org/drm-intel tags/drm-intel-fixes-2014-07-24

for you to fetch changes up to eedd10f45bdcb2a5b2afa35f845e080c3bc984f2:

  drm/i915: Simplify i915_gem_release_all_mmaps() (2014-07-23 16:09:51 +0200)


Chris Wilson (2):
  drm/i915: Reorder the semaphore deadlock check, again
  drm/i915: Simplify i915_gem_release_all_mmaps()

Hugh Dickins (1):
  drm/i915: fix freeze with blank screen booting highmem

 drivers/gpu/drm/i915/i915_gem.c  | 25 +
 drivers/gpu/drm/i915/i915_gem_render_state.c |  4 ++--
 drivers/gpu/drm/i915/i915_irq.c  | 11 ---
 3 files changed, 15 insertions(+), 25 deletions(-)

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

[PATCH] [rfc] drm: close race in connector registration

[David Herrmann]

Hi

On Thu, Jul 24, 2014 at 3:53 AM, Dave Airlie  wrote:
> From: Dave Airlie 
>
> Daniel pointed out with hotplug that userspace could be trying to oops us
> as root for lols, and that to be correct we shouldn't register the object
> with the idr before we have fully set the connector object up.
>
> His proposed solution was a lot more life changing, this seemed like a simpler
> proposition to me, get the connector object id from the idr, but don't
> register the object until the drm_connector_register callback.
>
> The open question is whether the drm_mode_object_register needs a bigger lock
> than just the idr one, but I can't see why it would, but I can be locking
> challenged.
>
> Signed-off-by: Dave Airlie 
> ---
>  drivers/gpu/drm/drm_crtc.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 1ccf5cb..9f8cc1a 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -376,13 +376,14 @@ EXPORT_SYMBOL(drm_get_format_name);
>   * New unique (relative to other objects in @dev) integer identifier for the
>   * object.
>   */
> -int drm_mode_object_get(struct drm_device *dev,
> -   struct drm_mode_object *obj, uint32_t obj_type)
> +static int drm_mode_object_get_noreg(struct drm_device *dev,
> +struct drm_mode_object *obj, uint32_t 
> obj_type,
> +bool noreg)
>  {
> int ret;
>
> mutex_lock(>mode_config.idr_mutex);
> -   ret = idr_alloc(>mode_config.crtc_idr, obj, 1, 0, GFP_KERNEL);
> +   ret = idr_alloc(>mode_config.crtc_idr, noreg ? NULL : obj, 1, 0, 
> GFP_KERNEL);
> if (ret >= 0) {
> /*
>  * Set up the object linking under the protection of the idr
> @@ -396,6 +397,20 @@ int drm_mode_object_get(struct drm_device *dev,
> return ret < 0 ? ret : 0;
>  }
>
> +int drm_mode_object_get(struct drm_device *dev,
> +   struct drm_mode_object *obj, uint32_t obj_type)
> +{
> +   return drm_mode_object_get_noreg(dev, obj, obj_type, false);
> +}
> +
> +static void drm_mode_object_register(struct drm_device *dev,
> +struct drm_mode_object *obj)
> +{
> +   mutex_lock(>mode_config.idr_mutex);
> +   idr_replace(>mode_config.crtc_idr, obj, obj->id);
> +   mutex_unlock(>mode_config.idr_mutex);
> +}
> +
>  /**
>   * drm_mode_object_put - free a modeset identifer
>   * @dev: DRM device
> @@ -849,7 +864,7 @@ int drm_connector_init(struct drm_device *dev,
>
> drm_modeset_lock_all(dev);
>
> -   ret = drm_mode_object_get(dev, >base, 
> DRM_MODE_OBJECT_CONNECTOR);
> +   ret = drm_mode_object_get_noreg(dev, >base, 
> DRM_MODE_OBJECT_CONNECTOR, true);
> if (ret)
> goto out_unlock;
>
> @@ -942,6 +957,8 @@ int drm_connector_register(struct drm_connector 
> *connector)
>  {
> int ret;
>
> +   drm_mode_object_register(connector->dev, >base);
> +

This is the same we do for minor-objects, so fine with me. I'd prefer
if the registration is done last in drm_connector_register(), not
first, but I'm not sure the debugfs hooks work without the connector
available in the lookup tables. Maybe worth a try.

Maybe you also want to do the reverse in drm_connector_unregister()?
Making sure no user-space call can look them up anymore.

This patch is:
Reviewed-by: David Herrmann 

Thanks
David

> ret = drm_sysfs_connector_add(connector);
> if (ret)
> return ret;
> --
> 1.9.3
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 3/4] drm/dsi: Make mipi_dsi_dcs_{read, write}() symmetrical

[Andrzej Hajda]

On 07/23/2014 03:37 PM, Thierry Reding wrote:
> On Wed, Jul 23, 2014 at 12:59:46PM +0200, Andrzej Hajda wrote:
>> On 07/23/2014 09:51 AM, Thierry Reding wrote:
>>> On Tue, Jul 22, 2014 at 11:33:11AM +0200, Andrzej Hajda wrote:
 On 07/22/2014 10:12 AM, Thierry Reding wrote:
> On Tue, Jul 22, 2014 at 09:32:58AM +0200, Andrzej Hajda wrote:
>> On 07/22/2014 09:12 AM, Thierry Reding wrote:
>>> From: Thierry Reding 
>>>
>>> Currently the mipi_dsi_dcs_write() function requires the DCS command
>>> byte to be embedded within the write buffer whereas mipi_dsi_dcs_read()
>>> has a separate parameter. Make them more symmetrical by adding an extra
>>> command parameter to mipi_dsi_dcs_write().
>>>
>>> Also update the S6E8AA0 panel driver (the only user of this API) to
>>> adapt to this new API.
>> I do not agree with that. DCS read and write are not symmetrical by 
>> design:
>> - write just sends data,
>> - read sends data then reads response.
>>
>> Forcing API to be symmetrical complicates the implementation without 
>> real gain.
> Why does it complicate anything?
 Why? Lets see:

  drivers/gpu/drm/drm_mipi_dsi.c| 45 
 ---
  drivers/gpu/drm/panel/panel-s6e8aa0.c |  4 ++--
  include/drm/drm_mipi_dsi.h|  4 ++--
  3 files changed, 35 insertions(+), 18 deletions(-)


 Original function has two vars, one 'if', one 'switch' and one operation
 call, nothing more.
 You proposes to add new vars, kmalloc with no memory handling, memcpy,
 playing with indices, conditional kfree. Isn't it enough to call it more
 complicated? :)
>>> It certainly makes the implementation of mipi_dsi_dcs_write() slightly
>>> more complicated, but the point is that it makes it easier for users of
>>> the API. So the complexity moves into one central location rather than
>>> each call site. Ultimately that will reduce overall complexity.
>> I guess it will increase complexity. If you look at the s6e8aa0 or any
>> other driver using DCS commands you will see the most DCS commands have
>> constant arguments, so driver uses small static arrays without copying
>> them to temporary variables. With your approach every time you will have
>> to allocate tiny memory bufs, memcpy to them and deallocate them later.
> Yes, there are certainly additional costs. But they give us more
> consistency in return. The whole point of having MIPI DCS helpers is so
> that they can take away some of the work that drivers have to do. This
> is core code
>
>> I terms of drivers code simplicity, current version with proposed macros
>> do better job.
> Quite frankly, I think they result in horrible code. The s6e8aa0 driver
> is currently not much more than a huge set of tables that are dumped to
> hardware.
>
> And of course that's "simple", but it's also completely unreadable and
> makes it really difficult to factor out common pieces of code. Of course
> the driver then also goes and uses these macros to execute standard DCS
> commands instead of doing the right thing and writing proper functions
> so that other drivers can reuse them. Yes, that's simple for the
> individual drivers but it doesn't help us at all in the big picture.

Hmm, I think you look at the wrong driver :) panel-s6e8aa0.c uses only
four DCS commands:
s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_EXIT_SLEEP_MODE);
s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_ON);
s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_ENTER_SLEEP_MODE);
s6e8aa0_dcs_write_seq_static(ctx, MIPI_DCS_SET_DISPLAY_OFF);
I see it rather readable :)

The rest of DSI transactions are MCS commands,  completely undocumented
- I had no access to datasheet for this
panel/IC driver during driver development. How do you imagine 'proper
functions' in such situation?

> Some of this could possibly be alleviated by adding separate functions
> for standard commands. But either way, I think having this kind of
> symmetry within an API is always good (it's consistent). By the law of
> least surprise people will actually expect mipi_dsi_dcs_write() to take
> a command as a second parameter.
 As I wrote earlier I do not see symmetry here: dcs-read is in fact write
 and read,
 dcs-write is just write. Hiding this fact in API does not seems to me
 good, but I guess
 it is rather matter of taste.
>>> The symmetry isn't about the physical transfers. It's a logical
>>> symmetry. Each DCS command is identified by a command, whether it's a
>>> read or a write.
>> If you insist on it maybe it will be better to leave my version of
>> mipi_dsi_dcs_write, maybe renamed to mipi_dsi_dcs_write_buf or sth
>> similar and add your version using internally my version. This way
>> you will have your symmetry and I will keep my simplicity :)
> Maybe that would be an alternative. I'll think about it.
>
>>> There's a 

[PATCH] drm: Fix race when checking for fb in the generic kms obj lookup

[Daniel Vetter]

In my review of

commit 98f75de40e9d83c3a90d294b8fd25fa2874212a9
Author: Rob Clark 
Date:   Fri May 30 11:37:03 2014 -0400

drm: add object property typ

I asked for a check to make sure that we never leak an fb from the
generic mode object lookup since those have completely different
lifetime rules. Rob added it, but outside of the idr mutex, which
means that our dereference of obj->type can already chase free'd
memory.

Somehow I didn't spot this, so fix this asap.

Cc: Rob Clark 
Signed-off-by: Daniel Vetter 
---
 drivers/gpu/drm/drm_crtc.c  | 6 +++---
 drivers/gpu/drm/drm_fb_helper.c | 1 +
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index f0a47907..853ab9cad071 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -429,6 +429,9 @@ static struct drm_mode_object *_object_find(struct 
drm_device *dev,
if (!obj || (type != DRM_MODE_OBJECT_ANY && obj->type != type) ||
(obj->id != id))
obj = NULL;
+   /* don't leak out unref'd fb's */
+   if (obj && (obj->type == DRM_MODE_OBJECT_FB))
+   obj = NULL;
mutex_unlock(>mode_config.idr_mutex);

return obj;
@@ -454,9 +457,6 @@ struct drm_mode_object *drm_mode_object_find(struct 
drm_device *dev,
 * function.*/
WARN_ON(type == DRM_MODE_OBJECT_FB);
obj = _object_find(dev, id, type);
-   /* don't leak out unref'd fb's */
-   if (obj && (obj->type == DRM_MODE_OBJECT_FB))
-   obj = NULL;
return obj;
 }
 EXPORT_SYMBOL(drm_mode_object_find);
diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index d5d8cea1a679..ff586ae3d92a 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -303,6 +303,7 @@ static bool restore_fbdev_mode(struct drm_fb_helper 
*fb_helper)
}
return error;
 }
+
 /**
  * drm_fb_helper_restore_fbdev_mode - restore fbdev configuration
  * @fb_helper: fbcon to restore
-- 
2.0.1

[PATCH 03/12] drm: call ->firstopen() before ->open()

[Alex Deucher]

On Thu, Jul 24, 2014 at 5:31 AM, David Herrmann  
wrote:
> Hi
>
> On Wed, Jul 23, 2014 at 9:25 PM, Daniel Vetter  wrote:
>> On Wed, Jul 23, 2014 at 05:26:38PM +0200, David Herrmann wrote:
>>> Lets order things correctly:
>>>  ->load()
>>>->fistopen()
>>>  ->open()
>>>  ->close()
>>>->lastclose()
>>>  ->unload()
>>>
>>> This doesn't change much as only savage and radeon use ->firstopen() and
>>> they just do map-initialization. Therefore, the global drm mutex makes
>>> sure there cannot be any other f_op between ->open() and ->firstopen().
>>> However, once we get rid of that lock, we really want ->firstopen() to
>>> initialize the device before ->open() is called.
>>>
>>> Furthermore, this fixes the clean-up path in drm_open(). We currently
>>> don't cleanup the drm_file object if ->firstopen() fails.
>>>
>>> Signed-off-by: David Herrmann 
>>> ---
>>>  drivers/gpu/drm/drm_fops.c | 139 
>>> +
>>>  include/drm/drmP.h |   2 +-
>>>  2 files changed, 66 insertions(+), 75 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
>>> index 021fe5d..8e73519 100644
>>> --- a/drivers/gpu/drm/drm_fops.c
>>> +++ b/drivers/gpu/drm/drm_fops.c
>>> @@ -45,7 +45,7 @@ EXPORT_SYMBOL(drm_global_mutex);
>>>
>>>  static int drm_open_helper(struct file *filp, struct drm_minor *minor);
>>>
>>> -static int drm_setup(struct drm_device * dev)
>>> +static int drm_firstopen(struct drm_device * dev)
>>
>> All the stuff in here is only for legacy drivers. Imo we should rename
>> this to drm_legacy_setup and hide it harder.
>>
>> Also touching the init ordering for ums drivers is a bit risky, I'd advise
>> against it. firstopen is officially dead for kms driver and really there's
>> nothing legit you can do in there. imx abused until they've switched over
>> to the component framework.
>>
>> I'd just drop this one tbh.
>
> I did a driver audit when writing this patch, there're only 2 drivers
> using firstopen:
>
>  * radeon_cp: sets two fields of dev_private and calls drm_addmap() on
> those. In lastclose(), it calls drm_rmmap()
>  * savage: sets several fields in dev_private, calls
> arch_phys_wc_add() for some regions and requests a drm-map for those
> via drm_addmap(). On lastclose(), it reverts exactly those steps.
>
> Taking into account that firstopen() just runs with drm_device
> context, not with drm_file context, I really think we should be
> calling it _before_ doing ->open(). I even think the drivers would
> fail horribly if we don't do this patch and some other user-context
> sneaks in between both calls (currently impossible thanks to
> drm_global_mutex).
>
> Both ->firstopen() callbacks are fairly trivial. In favor of making
> the error-paths work in drm_open() I'd really like to get this in.
> This patch is preparing for my drm_file rework that can make
> drm_dev_unregister() stop all open files immediately.
>
> But I honestly don't care much for UMS drivers, so if you NACK this,
> I'd just ignore the error code and add a comment that we don't do
> error-handling for UMS.

Well, radeon UMS support is in the kernel deprecated and hasn't been
built by default for a while and I doubt anyone has tested it in
years.  I'm not sure what the current state of savage is.  I'm fine
either way.

Alex


>
> Cheers
> David
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 0/7] prepare for atomic.. the great propertyification

[Rob Clark]

On Thu, Jul 24, 2014 at 8:25 AM, Daniel Vetter  wrote:
> On Wed, Jul 23, 2014 at 03:38:13PM -0400, Rob Clark wrote:
>> This is mostly just a rebase+resend.  Was going to send it a bit earlier
>> but had a few things to fix up as a result of the rebase.
>>
>> At this point, I think next steps are roughly:
>> 1) introduce plane->mutex
>> 2) decide what we want to do about events
>> 3) add actual ioctl
>>
>> I think we could shoot for merging this series next, and then adding
>> plane->mutex in 3.18?
>>
>> Before we add the ioctl, I think we want to sort out events for updates
>> to non-primary layers, and what the interface to drivers should look like.
>> Ie. just add event to ->update_plane() or should we completely ditch
>> ->page_flip() and ->update_plane()?
>>
>> Technically, I think we could get away without a new API and just let
>> drivers grab all the events in their ->atomic_commit(), but I suspect
>> core could provide something more useful to drivers.  I guess it would
>> be useful to have a few more drivers converted over to see what makes
>> sense.
>
> Ok so we've had a lot of discussions about this on irc and overall I'm not
> terribly happy with what this looks like. I have a few concerns:
>
> - The entire atomic conversion for drivers is monolithic, at least at the
>   interface level. So if you want to do this step-by-step you're forced to
>   add hacks and e.g. bypass pageflips until that's implemented. E.g. on
>   i915 I see no chance that we'll get atomic ready for all cases (so
>   nonblocking flips and multi-crtc and everything on all platforms) in one
>   go.

So, there interface is *intended* to be monolithic, in a way..  many
years ago, I started by adding side by side atomic vs legacy paths,
and it was pretty ugly in core.  I'm much happier with the current
iteration.

A slightly later iteration preserved atomic helpers (so drivers could
do completely their own thing).  I ended up dropping that because most
of what atomic does is manage the interface to whatever is doing
modeset/pageflip/whatever.  I completely expect some changes around
the commit stuff..  that part is intended to either be
wrapped/extended by the driver or replaced.  Possibly it could be made
more clear what drivers are expected to use vs extend or replace.  I
expect this to evolve a bit as more drivers are converted.

Note that the current atomic "layer" results in 1:1 conversion from
old userspace API to legacy vfuncs.  This way what is on top of atomic
API and what is beneath (ie. the driver) has same code paths either
way (old or new userspace, converted or not driver).  The edge cases
don't come in until atomic ioctl is exposed, and for that I expect a
driver flag to enable the new ioctl.  You could even set the flag
based on hw generation if you want.

> - Related to that is that we force legacy ioctls through atomic. E.g. on
>   older i915 platforms I very much expect that we won't ever convert the
>   pageflip code to atomic for them and just not support atomic flips of
>   cursor + primary plane. At least not at the beginning. I know that's
>   different from my stance a year ago, but I've become a bit more
>   realistic.
>
> - The entire conversion is monolithic and we can't do it on a
>   driver-by-driver basis. Everyone has to go through the new atomic
>   interfaces on a flag day. drm is much bigger now and forcing everyone to
>   convert at the same time is really risky. Again I know I've changed my
>   mind again, but drm is a lot bigger and there's a lot more drivers that
>   implement pageflip and cursors, i.e. stuff that's special.

the only flag day part is plugging in atomic functions and couple
vfunc API changes around set_prop().. the current design allows for
conversion on driver by driver basis.

> - The separation between interface marshalling code in the drm core and
>   helper functions for drivers to implement stuff is fuzzy at best. Thus
>   far we've had an excellent seperation in this are for kms, I don't think
>   it's vise to throw that out.

Interface marshalling should not be helper.  Everyone needs the same
thing, since what is on top is the same.

> So here's my proposal for how to get this in without breaking the world
>
> 1. We add a complete new set of ->atomic_foo driver entry points to all
> relevant objects. So instead of changing all the set_prop functions in a
> flag-day like operation we just add a new interface. I haven't double
> checked, but I guess that means per-obj ->atomic_set_prop functions plus a
> global ->atomic_check and ->atomic_commit.

that sounds much worse

> 2. We add a new drm_atomic_helper.c library which provides functions to
> implement all the legacy interfaces (page_flip, update_plane, set_prop)
> using atomic interfaces. We should be able to 1:1 reuse Rob's code for
> this, but instead of changing the actual current interface code we put
> that into helpers.

So, I've started ripping out page_flip.. now w/ primary plane helpers
we can do 

[PATCH] [rfc] drm: close race in connector registration

[Daniel Vetter]

On Thu, Jul 24, 2014 at 11:53:45AM +1000, Dave Airlie wrote:
> From: Dave Airlie 
> 
> Daniel pointed out with hotplug that userspace could be trying to oops us
> as root for lols, and that to be correct we shouldn't register the object
> with the idr before we have fully set the connector object up.
> 
> His proposed solution was a lot more life changing, this seemed like a simpler
> proposition to me, get the connector object id from the idr, but don't
> register the object until the drm_connector_register callback.
> 
> The open question is whether the drm_mode_object_register needs a bigger lock
> than just the idr one, but I can't see why it would, but I can be locking
> challenged.
> 
> Signed-off-by: Dave Airlie 

The noreg=false double negation is a bit a bender, I'd invert the meaning
of that. But otherwise I couldn't poke holes into this (but found another
one around the idr_mutex). Fix for that is on its way.

With noreg negated and the kerneldoc/EXPORT_SYMBOL moved to the new place
this is Reviewed-by: Daniel Vetter 

Cheers, Daniel
> ---
>  drivers/gpu/drm/drm_crtc.c | 25 +
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 1ccf5cb..9f8cc1a 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -376,13 +376,14 @@ EXPORT_SYMBOL(drm_get_format_name);
>   * New unique (relative to other objects in @dev) integer identifier for the
>   * object.
>   */
> -int drm_mode_object_get(struct drm_device *dev,
> - struct drm_mode_object *obj, uint32_t obj_type)
> +static int drm_mode_object_get_noreg(struct drm_device *dev,
> +  struct drm_mode_object *obj, uint32_t 
> obj_type,
> +  bool noreg)
>  {
>   int ret;
>  
>   mutex_lock(>mode_config.idr_mutex);
> - ret = idr_alloc(>mode_config.crtc_idr, obj, 1, 0, GFP_KERNEL);
> + ret = idr_alloc(>mode_config.crtc_idr, noreg ? NULL : obj, 1, 0, 
> GFP_KERNEL);
>   if (ret >= 0) {
>   /*
>* Set up the object linking under the protection of the idr
> @@ -396,6 +397,20 @@ int drm_mode_object_get(struct drm_device *dev,
>   return ret < 0 ? ret : 0;
>  }
>  
> +int drm_mode_object_get(struct drm_device *dev,
> + struct drm_mode_object *obj, uint32_t obj_type)
> +{
> + return drm_mode_object_get_noreg(dev, obj, obj_type, false);
> +}
> +
> +static void drm_mode_object_register(struct drm_device *dev,
> +  struct drm_mode_object *obj)
> +{
> + mutex_lock(>mode_config.idr_mutex);
> + idr_replace(>mode_config.crtc_idr, obj, obj->id);
> + mutex_unlock(>mode_config.idr_mutex);
> +}
> +
>  /**
>   * drm_mode_object_put - free a modeset identifer
>   * @dev: DRM device
> @@ -849,7 +864,7 @@ int drm_connector_init(struct drm_device *dev,
>  
>   drm_modeset_lock_all(dev);
>  
> - ret = drm_mode_object_get(dev, >base, 
> DRM_MODE_OBJECT_CONNECTOR);
> + ret = drm_mode_object_get_noreg(dev, >base, 
> DRM_MODE_OBJECT_CONNECTOR, true);
>   if (ret)
>   goto out_unlock;
>  
> @@ -942,6 +957,8 @@ int drm_connector_register(struct drm_connector 
> *connector)
>  {
>   int ret;
>  
> + drm_mode_object_register(connector->dev, >base);
> +
>   ret = drm_sysfs_connector_add(connector);
>   if (ret)
>   return ret;
> -- 
> 1.9.3
> 
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[PATCH 00/83] AMD HSA kernel driver

[Daniel Vetter]

On Wed, Jul 23, 2014 at 04:57:48PM -0700, Jesse Barnes wrote:
> On Sun, 13 Jul 2014 12:40:32 -0400
> j.glisse at gmail.com (Jerome Glisse) wrote:
> 
> > On Sun, Jul 13, 2014 at 11:42:58AM +0200, Daniel Vetter wrote:
> > > On Sat, Jul 12, 2014 at 6:49 PM, Jerome Glisse  
> > > wrote:
> > > >> Hm, so the hsa part is a completely new driver/subsystem, not just an
> > > >> additional ioctl tacked onto radoen? The history of drm is littered 
> > > >> with
> > > >> "generic" ioctls that turned out to be useful for exactly one driver.
> > > >> Which is why _all_ the command submission is now done with 
> > > >> driver-private
> > > >> ioctls.
> > > >>
> > > >> I'd be quite a bit surprised if that suddenly works differently, so 
> > > >> before
> > > >> we bless a generic hsa interface I really want to see some 
> > > >> implementation
> > > >> from a different vendor (i.e. nvdidia or intel) using the same ioctls.
> > > >> Otherwise we just repeat history and I'm not terribly inclined to keep 
> > > >> on
> > > >> cleanup up cruft forever - one drm legacy is enough ;-)
> > > >>
> > > >> Jesse is the guy from our side to talk to about this.
> > > >> -Daniel
> > > >
> > > > I am not worried about that side, the hsa foundation has pretty strict
> > > > guidelines on what is hsa compliant hardware ie the hw needs to 
> > > > understand
> > > > the pm4 packet format of radeon (well small subset of it). But of course
> > > > this require hsa compliant hardware and from member i am guessing ARM 
> > > > Mali,
> > > > ImgTech, Qualcomm, ... so unless Intel and NVidia joins hsa you will not
> > > > see it for those hardware.
> > > >
> > > > So yes for once same ioctl would apply to different hardware. The only 
> > > > things
> > > > that is different is the shader isa. The hsafoundation site has some pdf
> > > > explaining all that but someone thought that slideshare would be a good 
> > > > idea
> > > > personnaly i would not register to any of the website just to get the 
> > > > pdf.
> > > >
> > > > So to sumup i am ok with having a new device file that present uniform 
> > > > set
> > > > of ioctl. It would actualy be lot easier for userspace, just open this 
> > > > fix
> > > > device file and ask for list of compliant hardware.
> > > >
> > > > Then radeon kernel driver would register itself as a provider. So all 
> > > > ioctl
> > > > decoding marshalling would be share which makes sense.
> > > 
> > > There's also the other side namely that preparing the cp ring in
> > > userspace and submitting the entire pile through a doorbell to the hw
> > > scheduler isn't really hsa exclusive. And for a solid platform with
> > > seamless gpu/cpu integration that means we need standard ways to set
> > > gpu context priorities and get at useful stats like gpu time used by a
> > > given context.
> > > 
> > > To get there I guess intel/nvidia need to reuse the hsa subsystem with
> > > the command submission adjusted a bit. Kinda like drm where kms and
> > > buffer sharing is common and cs driver specific.
> > 
> > HSA module would be for HSA compliant hardware and thus hardware would
> > need to follow HSA specification which again is pretty clear on what
> > the hardware need to provide. So if Intel and NVidia wants to join HSA
> > i am sure they would be welcome, the more the merrier :)
> > 
> > So i would not block HSA kernel ioctl design in order to please non HSA
> > hardware especialy if at this point in time nor Intel or NVidia can
> > share anything concret on the design and how this things should be setup
> > for there hardware.
> > 
> > When Intel or NVidia present their own API they should provide their
> > own set of ioctl through their own platform.
> 
> Yeah things are different enough that a uniform ioctl doesn't make
> sense.  If/when all the vendors decide on a single standard, we can use
> that, but until then I don't see a nice way to share our doorbell &
> submission scheme with HSA, and I assume nvidia is the same.
> 
> Using HSA as a basis for non-HSA systems seems like it would add a lot
> of complexity, since non-HSA hardware would have to intercept the queue
> writes and manage the submission requests etc as bytecodes in the
> kernel driver, or maybe as a shim layer library that wraps that stuff.
> 
> Probably not worth the effort given that the command sets themselves
> are all custom as well, driven by specific user level drivers like GL,
> CL, and libva.

Well I know that - drm also has the split between shared management stuff
like prime and driver private cmd submission. I still think that some
common interfaces would benefit us. I want things like a gputop (and also
perf counters and all that) to work the same way with the same tooling on
all svm/gpgpu stuff. So a shared namespace/create for svm contexts or
something like that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] gpio: extend gpiod_get*() with flags parameter

[Greg Kroah-Hartman]

On Fri, Jul 25, 2014 at 12:04:58AM +0900, Alexandre Courbot wrote:
> The huge majority of GPIOs have their direction and initial value set
> right after being obtained by one of the gpiod_get() functions. The
> integer GPIO API had gpio_request_one() that took a convenience flags
> parameter allowing to specify an direction and value applied to the
> returned GPIO. This feature greatly simplifies client code and ensures
> errors are always handled properly.
> 
> A similar feature has been requested for the gpiod API. Since GPIOs need
> a direction to be used anyway, we prefer to extend the existing
> functions instead of introducing new functions that would raise the
> number of gpiod getters to 16 (!).
> 
> The drawback of this approach is that all gpiod clients need to be
> updated, but there aren't that many and the moment and this results in
> smaller (and hopefully safer) code.
> 
> Signed-off-by: Alexandre Courbot 
> ---
> This change will be difficult to apply without breaking things, but
> let's try to do it right. Hopefully the benefit will outweight the
> disturbance.
> 
> This is a patch against -next to list and update all current gpiod
> consumers. Updates are trivial at first sight, but it would be nice to
> get as many acks as possible from the respective subsystem maintainers.
> 
> I'm not sure how this could be applied harmlessly though - maybe through
> a dedicated branch for -next? Problem is that a lot of new code is not
> yet merged into mainline, and conflicts are very likely to occur. Linus,
> do you have any suggestion as to how this can be done without blood being
> spilled?

Do this in 3 steps, not all at once.

Make a new function that takes the new argument, get that merged

Submit patches that convert drivers over to use the new function.

Once all of those are merged, remove the old function.

That way there are no "flag days" needed, and everyone is happy you
don't send out emails with 40+ people in the To: and Cc: lines :)

thanks,

greg k-h

[PATCH] drm: Fix race when checking for fb in the generic kms obj lookup

[Chris Wilson]

On Thu, Jul 24, 2014 at 09:54:27AM +0200, Daniel Vetter wrote:
> In my review of
> 
> commit 98f75de40e9d83c3a90d294b8fd25fa2874212a9
> Author: Rob Clark 
> Date:   Fri May 30 11:37:03 2014 -0400
> 
> drm: add object property typ
> 
> I asked for a check to make sure that we never leak an fb from the
> generic mode object lookup since those have completely different
> lifetime rules. Rob added it, but outside of the idr mutex, which
> means that our dereference of obj->type can already chase free'd
> memory.
> 
> Somehow I didn't spot this, so fix this asap.
> 
> Cc: Rob Clark 
> Signed-off-by: Daniel Vetter 
> ---
>  drivers/gpu/drm/drm_crtc.c  | 6 +++---
>  drivers/gpu/drm/drm_fb_helper.c | 1 +
>  2 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index f0a47907..853ab9cad071 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -429,6 +429,9 @@ static struct drm_mode_object *_object_find(struct 
> drm_device *dev,
>   if (!obj || (type != DRM_MODE_OBJECT_ANY && obj->type != type) ||
>   (obj->id != id))
>   obj = NULL;
> + /* don't leak out unref'd fb's */
> + if (obj && (obj->type == DRM_MODE_OBJECT_FB))
> + obj = NULL;

-   if (!obj || (type != DRM_MODE_OBJECT_ANY && obj->type != type) ||
-   (obj->id != id))
+   if (obj && obj->type != type && type != DRM_MODE_OBJECT_ANY)
+   obj = NULL;
+   if (obj && WARN_ON(obj->id != id))
+   obj = NULL;
+   if (obj && WARN_ON(obj->type == DRM_MODE_OBJECT_FB))
obj = NULL;

To break the checks up into simple steps and show that they are unlikely
errors?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[Bug 81001] radeon: fence wait failed (-35) after hybrid suspend, leading to GPU reset and hangs

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

https://bugzilla.kernel.org/show_bug.cgi?id=81001

Ivan Kalvachev  changed:

   What|Removed |Added

 CC||iive at yahoo.com
 Regression|No  |Yes

-- 
You are receiving this mail because:
You are watching the assignee of the bug.

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

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

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

--- Comment #2 from Michel D?nzer  ---
Can you resolve the r600_dri.so symbols?

-- 
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/20140724/c8ebc8de/attachment.html>

[PATCH v2 00/25] AMDKFD kernel driver

[Oded Gabbay]

On 24/07/14 00:46, Bridgman, John wrote:
> 
>> -Original Message- From: dri-devel
>> [mailto:dri-devel-bounces at lists.freedesktop.org] On Behalf Of Jesse
>> Barnes Sent: Wednesday, July 23, 2014 5:00 PM To:
>> dri-devel at lists.freedesktop.org Subject: Re: [PATCH v2 00/25]
>> AMDKFD kernel driver
>> 
>> On Mon, 21 Jul 2014 19:05:46 +0200 daniel at ffwll.ch (Daniel
>> Vetter) wrote:
>> 
>>> On Mon, Jul 21, 2014 at 11:58:52AM -0400, Jerome Glisse wrote:
 On Mon, Jul 21, 2014 at 05:25:11PM +0200, Daniel Vetter wrote:
> On Mon, Jul 21, 2014 at 03:39:09PM +0200, Christian K?nig
> wrote:
>> Am 21.07.2014 14:36, schrieb Oded Gabbay:
>>> On 20/07/14 20:46, Jerome Glisse wrote:
>> 
>> [snip!!]
> My BlackBerry thumb thanks you ;)
>> 
>> 
>> The main questions here are if it's avoid able to pin down
>> the memory and if the memory is pinned down at driver load,
>> by request from userspace or by anything else.
>> 
>> As far as I can see only the "mqd per userspace queue"
>> might be a bit questionable, everything else sounds
>> reasonable.
> 
> Aside, i915 perspective again (i.e. how we solved this):
> When scheduling away from contexts we unpin them and put them
> into the lru. And in the shrinker we have a last-ditch
> callback to switch to a default context (since you can't ever
> have no context once you've started) which means we can evict
> any context object if it's
>> getting in the way.
 
 So Intel hardware report through some interrupt or some channel
 when it is not using a context ? ie kernel side get
 notification when some user context is done executing ?
>>> 
>>> Yes, as long as we do the scheduling with the cpu we get
>>> interrupts for context switches. The mechanic is already
>>> published in the execlist patches currently floating around. We
>>> get a special context switch interrupt.
>>> 
>>> But we have this unpin logic already on the current code where
>>> we switch contexts through in-line cs commands from the kernel.
>>> There we obviously use the normal batch completion events.
>> 
>> Yeah and we can continue that going forward.  And of course if your
>> hw can do page faulting, you don't need to pin the normal data
>> buffers.
>> 
>> Usually there are some special buffers that need to be pinned for
>> longer periods though, anytime the context could be active.  Sounds
>> like in this case the userland queues, which makes some sense.  But
>> maybe for smaller systems the size limit could be clamped to
>> something smaller than 128M.  Or tie it into the rlimit somehow,
>> just like we do for mlock() stuff.
>> 
> Yeah, even the queues are in pageable memory, it's just a ~256 byte
> structure per queue (the Memory Queue Descriptor) that describes the
> queue to hardware, plus a couple of pages for each process using HSA
> to hold things like doorbells. Current thinking is to limit #
> processes using HSA to ~256 and #queues per process to ~1024 by
> default in the initial code, although my guess is that we could take
> the #queues per process default limit even lower.
> 

So my mistake. struct cik_mqd is actually 604 bytes, and it is allocated
on 256 boundary.
I had in mind to reserve 64MB of gart by default, which translates to
512 queues per process, with 128 processes. Add 2 kernel module
parameters, # of max-queues-per-process and # of max-processes (default
is, as I said, 512 and 128) for better control of system admin.

Oded

 The issue with radeon hardware AFAICT is that the hardware do
 not report any thing about the userspace context running ie you
 do not get notification when a context is not use. Well AFAICT.
 Maybe hardware
>> do provide that.
>>> 
>>> I'm not sure whether we can do the same trick with the hw
>>> scheduler. But then unpinning hw contexts will drain the pipeline
>>> anyway, so I guess we can just stop feeding the hw scheduler
>>> until it runs dry. And then unpin and evict.
>> 
>> Yeah we should have an idea which contexts have been fed to the
>> scheduler, at least with kernel based submission.  With userspace
>> submission we'll be in a tougher spot...  but as you say we can
>> always idle things and unpin everything under pressure.  That's a
>> really big hammer to apply though.
>> 
 Like the VMID is a limited resources so you have to dynamicly
 bind them so maybe we can only allocate pinned buffer for each
 VMID and then when binding a PASID to a VMID it also copy back
 pinned buffer to
>> pasid unpinned copy.
>>> 
>>> Yeah, pasid assignment will be fun. Not sure whether Jesse's
>>> patches will do this already. We _do_ already have fun with ctx
>>> id assigments though since we move them around (and the hw id is
>>> the ggtt address afaik). So we need to remap them already. Not
>>> sure on the details for pasid mapping, iirc it's a separate field
>>> somewhere in the context struct. Jesse knows the details.
>> 
>> 

[PATCH 1/7] drm: add atomic fxns

[Daniel Vetter]

On Wed, Jul 23, 2014 at 11:34:00PM +0200, Daniel Vetter wrote:
> On Wed, Jul 23, 2014 at 03:38:14PM -0400, Rob Clark wrote:
> > The 'atomic' mechanism allows for multiple properties to be updated,
> > checked, and commited atomically.  This will be the basis of atomic-
> > modeset and nuclear-pageflip.
> > 
> > The basic flow is:
> > 
> >state = dev->atomic_begin();
> >for (... one or more ...)
> >   obj->set_property(obj, state, prop, value);
> >if (dev->atomic_check(state))
> >   dev->atomic_commit(state);
> >dev->atomic_end(state);
> > 
> > The split of check and commit steps is to allow for ioctls with a
> > test-only flag (which would skip the commit step).
> > 
> > Signed-off-by: Rob Clark 
> 
> [snip]
> > +   if (flags & DRM_MODE_ATOMIC_NOLOCK)
> > +   acquire_flags |= DRM_MODESET_ACQUIRE_NOLOCK;
> > +   if (flags & DRM_MODE_ATOMIC_NONBLOCK)
> > +   acquire_flags |= DRM_MODESET_ACQUIRE_NONBLOCK;
> 
> Just a very quick reply. Can you please remove the code which does the
> NOLOCK/NONBLOCK stuff here? It's not really part of the property
> conversion and I'm still not sold on those concepts.
> 
> At least I want to review them once we add them, maybe at the very end
> where everything else is clear. Afaics nothing in this series actually
> uses this.

Self-correct: NOLOCK is used for the fbdev restore code. Imo we should
drop the modeset_lock_all this one uses and switch to using the atomic
locking scheme like everywhere. That way fbdev modeset changes work
exactly like any other modeset change.

fbdev state itself can be protected with dev->mode_config.mutex, which
wraps around all the ww_mutex dancing.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/7] drm: add atomic fxns

[Daniel Vetter]

On Wed, Jul 23, 2014 at 03:38:14PM -0400, Rob Clark wrote:
> The 'atomic' mechanism allows for multiple properties to be updated,
> checked, and commited atomically.  This will be the basis of atomic-
> modeset and nuclear-pageflip.
> 
> The basic flow is:
> 
>state = dev->atomic_begin();
>for (... one or more ...)
>   obj->set_property(obj, state, prop, value);
>if (dev->atomic_check(state))
>   dev->atomic_commit(state);
>dev->atomic_end(state);
> 
> The split of check and commit steps is to allow for ioctls with a
> test-only flag (which would skip the commit step).
> 
> Signed-off-by: Rob Clark 

[snip]
> + if (flags & DRM_MODE_ATOMIC_NOLOCK)
> + acquire_flags |= DRM_MODESET_ACQUIRE_NOLOCK;
> + if (flags & DRM_MODE_ATOMIC_NONBLOCK)
> + acquire_flags |= DRM_MODESET_ACQUIRE_NONBLOCK;

Just a very quick reply. Can you please remove the code which does the
NOLOCK/NONBLOCK stuff here? It's not really part of the property
conversion and I'm still not sold on those concepts.

At least I want to review them once we add them, maybe at the very end
where everything else is clear. Afaics nothing in this series actually
uses this.
-Daniel

> +
> + drm_modeset_acquire_init(>acquire_ctx, acquire_flags);
> +
> + state->dev = dev;
> + state->flags = flags;
> +
> + return state;
> +}
> +EXPORT_SYMBOL(drm_atomic_begin);
> +
> +/**
> + * drm_atomic_set_event - set a pending event on mode object
> + * @dev: DRM device
> + * @state: the driver state object
> + * @obj: the object to set the event on
> + * @event: the event to send back
> + *
> + * Set pending event for an update on the specified object.  The
> + * event is to be sent back to userspace after the update completes.
> + */
> +int drm_atomic_set_event(struct drm_device *dev,
> + struct drm_atomic_state *state, struct drm_mode_object *obj,
> + struct drm_pending_vblank_event *event)
> +{
> + return -EINVAL;  /* for now */
> +}
> +EXPORT_SYMBOL(drm_atomic_set_event);
> +
> +/**
> + * drm_atomic_check - validate state object
> + * @dev: DRM device
> + * @state: the driver state object
> + *
> + * Check the state object to see if the requested state is
> + * physically possible.
> + *
> + * RETURNS
> + * Zero for success or -errno
> + */
> +int drm_atomic_check(struct drm_device *dev, struct drm_atomic_state *state)
> +{
> + struct drm_atomic_state *a = state;
> + a->acquire_ctx.frozen = true;
> + return 0;  /* for now */
> +}
> +EXPORT_SYMBOL(drm_atomic_check);
> +
> +/* Note that we drop and re-acquire the locks w/ ww_mutex directly,
> + * since we keep the crtc in our list with in_atomic == true.
> + */
> +
> +static void drop_locks(struct drm_atomic_state *a,
> + struct ww_acquire_ctx *ww_ctx)
> +{
> + struct drm_modeset_acquire_ctx *ctx = >acquire_ctx;
> + struct drm_modeset_lock *lock;
> +
> + mutex_lock(>mutex);
> + list_for_each_entry(lock, >locked, head)
> + ww_mutex_unlock(>mutex);
> + mutex_unlock(>mutex);
> +
> + ww_acquire_fini(ww_ctx);
> +}
> +
> +static void grab_locks(struct drm_atomic_state *a,
> + struct ww_acquire_ctx *ww_ctx)
> +{
> + struct drm_modeset_acquire_ctx *ctx = >acquire_ctx;
> + struct drm_modeset_lock *lock, *slow_locked, *contended;
> + int ret;
> +
> + lock = slow_locked = contended = NULL;
> +
> +
> + ww_acquire_init(ww_ctx, _ww_class);
> +
> + /*
> +  * We need to do proper rain^Hww dance.. another context
> +  * could sneak in a grab the lock in order to check
> +  * crtc->in_atomic, and we get -EDEADLK.  But the winner
> +  * will realize the mistake when it sees crtc->in_atomic
> +  * already set, and then drop lock and return -EBUSY.
> +  * So we just need to keep dancing until we win.
> +  */
> +retry:
> + ret = 0;
> + list_for_each_entry(lock, >locked, head) {
> + if (lock == slow_locked) {
> + slow_locked = NULL;
> + continue;
> + }
> + contended = lock;
> + ret = ww_mutex_lock(>mutex, ww_ctx);
> + if (ret)
> + goto fail;
> + }
> +
> +fail:
> + if (ret == -EDEADLK) {
> + /* we lost out in a seqno race, backoff, lock and retry.. */
> +
> + list_for_each_entry(lock, >locked, head) {
> + if (lock == contended)
> + break;
> + ww_mutex_unlock(>mutex);
> + }
> +
> + if (slow_locked)
> + ww_mutex_unlock(_locked->mutex);
> +
> + ww_mutex_lock_slow(>mutex, ww_ctx);
> + slow_locked = contended;
> + goto retry;
> + }
> + WARN_ON(ret);   /* if we get EALREADY then something is fubar */
> +}
> +
> +static void commit_locks(struct drm_atomic_state *a,
> + struct ww_acquire_ctx *ww_ctx)
> +{
> +