[drm-intel:drm-intel-nightly 902/919] drivers/gpu/drm/drm_fb_cma_helper.c:312:20: error: 'struct drm_framebuffer' has no member named 'fomat'
tree: git://anongit.freedesktop.org/drm-intel drm-intel-nightly
head: 64353e762935a7ad82867be0e4e80ff2f7bc97e4
commit: ca984a998ad3a3b6bf8bf0d89861a6537551aaf2 [902/919] drm/fb_cma_helper:
Replace drm_format_info() with fb->format
config: m68k-allyesconfig (attached as .config)
compiler: m68k-linux-gcc (GCC) 4.9.0
reproduce:
wget
https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
-O ~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout ca984a998ad3a3b6bf8bf0d89861a6537551aaf2
# save the attached .config to linux build tree
make.cross ARCH=m68k
All errors (new ones prefixed by >>):
drivers/gpu/drm/drm_fb_cma_helper.c: In function 'drm_fb_cma_describe':
>> drivers/gpu/drm/drm_fb_cma_helper.c:312:20: error: 'struct drm_framebuffer'
>> has no member named 'fomat'
for (i = 0; i < fb->fomat->num_planes; i++) {
^
vim +312 drivers/gpu/drm/drm_fb_cma_helper.c
306 struct drm_fb_cma *fb_cma = to_fb_cma(fb);
307 int i;
308
309 seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width, fb->height,
310 (char *)&fb->pixel_format);
311
> 312 for (i = 0; i < fb->fomat->num_planes; i++) {
313 seq_printf(m, " %d: offset=%d pitch=%d, obj: ",
314 i, fb->offsets[i], fb->pitches[i]);
315 drm_gem_cma_describe(fb_cma->obj[i], m);
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-- next part --
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 38581 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/068967cf/attachment-0001.gz>
[bug report] drm/i915: Small compaction of the engine init code
Hello Tvrtko Ursulin,
The patch a19d6ff29a82: "drm/i915: Small compaction of the engine
init code" from Jun 23, 2016, leads to the following static checker
warning:
drivers/gpu/drm/i915/intel_lrc.c:1973 logical_render_ring_init()
warn: passing freed memory 'engine'
drivers/gpu/drm/i915/intel_lrc.c
1970
1971 ret = logical_ring_init(engine);
1972 if (ret) {
1973 lrc_destroy_wa_ctx_obj(engine);
The problem is that logical_ring_init() frees "engine" on the error
path so this is a use after free.
1974 }
1975
1976 return ret;
1977 }
regards,
dan carpenter
[drm-tip:drm-tip 891/920] htmldocs: drivers/gpu/drm/drm_modeset_helper.c:74: warning: No description found for parameter 'dev'
8-12 59list_splice(&panel_list,
&dev->mode_config.connector_list);
1de72faf Daniel Vetter2016-08-12 60 }
1de72faf Daniel Vetter2016-08-12 61
EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head);
1de72faf Daniel Vetter2016-08-12 62
1de72faf Daniel Vetter2016-08-12 63 /**
1de72faf Daniel Vetter2016-08-12 64 * drm_helper_mode_fill_fb_struct -
fill out framebuffer metadata
1de72faf Daniel Vetter2016-08-12 65 * @fb: drm_framebuffer object to
fill out
1de72faf Daniel Vetter2016-08-12 66 * @mode_cmd: metadata from the
userspace fb creation request
1de72faf Daniel Vetter2016-08-12 67 *
1de72faf Daniel Vetter2016-08-12 68 * This helper can be used in a
drivers fb_create callback to pre-fill the fb's
1de72faf Daniel Vetter2016-08-12 69 * metadata fields.
1de72faf Daniel Vetter2016-08-12 70 */
a3f913ca Ville Syrjälä2016-12-14 71 void
drm_helper_mode_fill_fb_struct(struct drm_device *dev,
a3f913ca Ville Syrjälä2016-12-14 72
struct drm_framebuffer *fb,
1de72faf Daniel Vetter2016-08-12 73
const struct drm_mode_fb_cmd2 *mode_cmd)
1de72faf Daniel Vetter2016-08-12 @74 {
488546fc Laurent Pinchart 2016-10-18 75const struct drm_format_info
*info;
1de72faf Daniel Vetter2016-08-12 76int i;
1de72faf Daniel Vetter2016-08-12 77
488546fc Laurent Pinchart 2016-10-18 78info =
drm_format_info(mode_cmd->pixel_format);
488546fc Laurent Pinchart 2016-10-18 79if (!info || !info->depth) {
b3c11ac2 Eric Engestrom 2016-11-12 80struct
drm_format_name_buf format_name;
488546fc Laurent Pinchart 2016-10-18 81
b3c11ac2 Eric Engestrom 2016-11-12 82DRM_DEBUG_KMS("non-RGB
pixel format %s\n",
:: The code at line 74 was first introduced by commit
:: 1de72faf10c367d80039761dca5f761b42abca01 drm/kms-helpers: Extract
drm_modeset_helper.[hc]
:: TO: Daniel Vetter
:: CC: Daniel Vetter
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-- next part --
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 6425 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/b8a7dfad/attachment-0001.gz>
Radeon X200M device suspend problem
I have a laptop with a Radeon X200M card in it. I use Radeon DRM driver for graphics, and it makes the system hang with display off when trying to suspend (either to disk or to RAM). Using /sys/power/pm_test interface revealed that it freezes when suspending devices. I have tried both Debian repository kernel (https://packages.debian.org/stable/linux-image-3.16.0-4-686-pae) and a custom-built vanilla 3.18.45 kernel with this driver both built-in and included as a module. The problem reproduces the same way. It stops to reproduce if I delete the module radeon.ko or otherwise prevent it from loading. The problem didn't appear before I started using DRM. dmitriy at laptop:~$ dmesg | grep -i radeon [ 18.520307] [drm] radeon kernel modesetting enabled. [ 18.521610] radeon :01:05.0: VRAM: 128M 0x7800 - 0x7FFF (128M used) [ 18.521667] radeon :01:05.0: GTT: 512M 0x8000 - 0x9FFF [ 18.526251] [drm] radeon: 128M of VRAM memory ready [ 18.526303] [drm] radeon: 512M of GTT memory ready. [ 18.558510] [drm] radeon: 1 quad pipes, 1 z pipes initialized. [ 18.558685] radeon :01:05.0: WB enabled [ 18.558739] radeon :01:05.0: fence driver on ring 0 use gpu addr 0x8000 and cpu addr 0xf48a6000 [ 18.558896] radeon :01:05.0: radeon: MSI limited to 32-bit [ 18.558983] [drm] radeon: irq initialized. [ 18.696262] [drm] radeon: ring at 0x80001000 [ 18.697892] [drm] Radeon Display Connectors [ 18.736547] radeon :01:05.0: fb0: radeondrmfb frame buffer device [ 18.736601] radeon :01:05.0: registered panic notifier [ 18.736660] [drm] Initialized radeon 2.40.0 20080528 for :01:05.0 on minor 0 dmitriy at laptop:~$ lspci | grep -i radeon 01:05.0 VGA compatible controller: Advanced Micro Devices, Inc. [AMD/ATI] RC410M [Mobility Radeon Xpress 200M] dmitriy at laptop:~$ lsmod Module Size Used by fbcon 42796 71 bitblit12545 1 fbcon softcursor 12333 1 bitblit tileblit 12517 1 fbcon ath9k_htc 50765 0 radeon 1438969 2 ath9k_common 21530 1 ath9k_htc ath9k_hw 369801 2 ath9k_common,ath9k_htc ath21707 3 ath9k_common,ath9k_htc,ath9k_hw cfbfillrect12474 1 radeon cfbimgblt 12335 1 radeon cfbcopyarea12334 1 radeon i2c_algo_bit 12640 1 radeon drm_kms_helper 71528 1 radeon ttm67750 1 radeon drm 207864 5 ttm,drm_kms_helper,radeon What additional information can I collect and how? What other kernel and driver versions can I try to see if the problem is already solved? What other ways can I help to solve the problem in?
[drm-tip:drm-tip 902/918] drivers/gpu/drm/drm_fb_cma_helper.c:312:20: error: 'struct drm_framebuffer' has no member named 'fomat'; did you mean 'format'?
tree: git://anongit.freedesktop.org/drm/drm-tip drm-tip
head: 067324d26ea0a913b18737a702eded1db4b255d0
commit: ca984a998ad3a3b6bf8bf0d89861a6537551aaf2 [902/918] drm/fb_cma_helper:
Replace drm_format_info() with fb->format
config: i386-randconfig-s1-201650 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
git checkout ca984a998ad3a3b6bf8bf0d89861a6537551aaf2
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
drivers/gpu/drm/drm_fb_cma_helper.c: In function 'drm_fb_cma_describe':
>> drivers/gpu/drm/drm_fb_cma_helper.c:312:20: error: 'struct drm_framebuffer'
>> has no member named 'fomat'; did you mean 'format'?
for (i = 0; i < fb->fomat->num_planes; i++) {
^~
vim +312 drivers/gpu/drm/drm_fb_cma_helper.c
306 struct drm_fb_cma *fb_cma = to_fb_cma(fb);
307 int i;
308
309 seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width, fb->height,
310 (char *)&fb->pixel_format);
311
> 312 for (i = 0; i < fb->fomat->num_planes; i++) {
313 seq_printf(m, " %d: offset=%d pitch=%d, obj: ",
314 i, fb->offsets[i], fb->pitches[i]);
315 drm_gem_cma_describe(fb_cma->obj[i], m);
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-- next part --
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/gzip
Size: 30762 bytes
Desc: not available
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/d337150a/attachment-0001.gz>
[PATCH v3 2/2] drm/panel: simple: Add support BOE nv101wxmn51
Reviewed-by: Stéphane Marchesin
On Tue, Dec 13, 2016 at 7:19 PM, Caesar Wang wrote:
> 10.1WXGA is a color active matrix TFT LCD module using amorphous silicon
> TFT's as an active switching devices. It can be supported by the
> simple-panel driver.
>
> Read the panel default edid information:
>
> EDID MODE DETAILS
> name =
> pixel_clock = 71900
> lvds_dual_channel = 0
> refresh = 0
> ha = 1280
> hbl = 160
> hso = 48
> hspw = 32
> hborder = 0
> va = 800
> vbl = 32
> vso = 3
> vspw = 5
> vborder = 0
> phsync = +
> pvsync = -
> x_mm = 0
> y_mm = 0
> drm_display_mode
> .hdisplay = 1280
> .hsync_start = 1328
> .hsync_end = 1360
> .htotal = 1440
> .vdisplay = 800
> .vsync_start = 803
> .vsync_end = 808
> .vtotal = 832
>
> There are two modes in the edid:
> Detailed mode1: Clock 71.900 MHz, 216 mm x 135 mm
>1280 1328 1360 1440 hborder 0
> 800 803 808 832 vborder 0
>+hsync -vsync
> Detailed mode2: Clock 57.500 MHz, 216 mm x 135 mm
>1280 1328 1360 1440 hborder 0
> 800 803 808 832 vborder 0
>+hsync -vsync
>
> Add the both edid to support more modes for BOE nv101wxmn51.
>
> Signed-off-by: Caesar Wang
> ---
>
> Changes in v3:
> - As Stéphane commented on https://patchwork.kernel.org/patch/9465911,
> add downclock mode for edid.
>
> Changes in v2:
> - fix the vsync_start and vsync_end from the edid.
> - change the commit.
>
> drivers/gpu/drm/panel/panel-simple.c | 45
>
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/gpu/drm/panel/panel-simple.c
> b/drivers/gpu/drm/panel/panel-simple.c
> index 06aaf79..1ce25b5 100644
> --- a/drivers/gpu/drm/panel/panel-simple.c
> +++ b/drivers/gpu/drm/panel/panel-simple.c
> @@ -668,6 +668,48 @@ static const struct panel_desc avic_tm070ddh03 = {
> },
> };
>
> +static const struct drm_display_mode boe_nv101wxmn51_modes[] = {
> + {
> + .clock = 71900,
> + .hdisplay = 1280,
> + .hsync_start = 1280 + 48,
> + .hsync_end = 1280 + 48 + 32,
> + .htotal = 1280 + 48 + 32 + 80,
> + .vdisplay = 800,
> + .vsync_start = 800 + 3,
> + .vsync_end = 800 + 3 + 5,
> + .vtotal = 800 + 3 + 5 + 24,
> + .vrefresh = 60,
> + },
> + {
> + .clock = 57500,
> + .hdisplay = 1280,
> + .hsync_start = 1280 + 48,
> + .hsync_end = 1280 + 48 + 32,
> + .htotal = 1280 + 48 + 32 + 80,
> + .vdisplay = 800,
> + .vsync_start = 800 + 3,
> + .vsync_end = 800 + 3 + 5,
> + .vtotal = 800 + 3 + 5 + 24,
> + .vrefresh = 48,
> + },
> +};
> +
> +static const struct panel_desc boe_nv101wxmn51 = {
> + .modes = boe_nv101wxmn51_modes,
> + .num_modes = ARRAY_SIZE(boe_nv101wxmn51_modes),
> + .bpc = 8,
> + .size = {
> + .width = 217,
> + .height = 136,
> + },
> + .delay = {
> + .prepare = 210,
> + .enable = 50,
> + .unprepare = 160,
> + },
> +};
> +
> static const struct drm_display_mode chunghwa_claa070wp03xg_mode = {
> .clock = 66770,
> .hdisplay = 800,
> @@ -1748,6 +1790,9 @@ static const struct of_device_id platform_of_match[] = {
> .compatible = "avic,tm070ddh03",
> .data = &avic_tm070ddh03,
> }, {
> + .compatible = "boe,nv101wxmn51",
> + .data = &boe_nv101wxmn51,
> + }, {
> .compatible = "chunghwa,claa070wp03xg",
> .data = &chunghwa_claa070wp03xg,
> }, {
> --
> 2.7.4
>
[bug report] drm/i915: Small compaction of the engine init code
On Thu, Dec 15, 2016 at 11:44:13PM +0300, Dan Carpenter wrote:
> Hello Tvrtko Ursulin,
>
> The patch a19d6ff29a82: "drm/i915: Small compaction of the engine
> init code" from Jun 23, 2016, leads to the following static checker
> warning:
>
> drivers/gpu/drm/i915/intel_lrc.c:1973 logical_render_ring_init()
> warn: passing freed memory 'engine'
>
> drivers/gpu/drm/i915/intel_lrc.c
> 1970
> 1971 ret = logical_ring_init(engine);
> 1972 if (ret) {
> 1973 lrc_destroy_wa_ctx_obj(engine);
>
> The problem is that logical_ring_init() frees "engine" on the error
> path so this is a use after free.
And calls lrc_destroy_wa_ctx_obj() in the process. So we can
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 067394b0a769..1c1bad8ae7b0 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1940,12 +1940,7 @@ int logical_render_ring_init(struct intel_engine_cs
*engine)
ret);
}
- ret = logical_ring_init(engine);
- if (ret) {
- lrc_destroy_wa_ctx_obj(engine);
- }
-
- return ret;
+ return logical_ring_init(engine);
}
int logical_xcs_ring_init(struct intel_engine_cs *engine)
--
Chris Wilson, Intel Open Source Technology Centre
[PATCH] kref: prefer atomic_inc_not_zero to atomic_add_unless
Hi Greg, On Thu, Dec 15, 2016 at 8:10 PM, Greg KH wrote: > I'll take it after 4.10-rc1 is out, thanks. Thank you! Jason
[PATCH] drm: Fix kernel docs for drm_helper_mode_fill_fb_struct()
On Thu, Dec 15, 2016 at 05:12:26PM +, Deucher, Alexander wrote:
> > -Original Message-
> > From: ville.syrjala at linux.intel.com [mailto:ville.syrjala at
> > linux.intel.com]
> > Sent: Thursday, December 15, 2016 12:01 PM
> > To: dri-devel at lists.freedesktop.org
> > Cc: Laurent Pinchart; Deucher, Alexander
> > Subject: [PATCH] drm: Fix kernel docs for drm_helper_mode_fill_fb_struct()
> >
> > From: Ville Syrjälä
> >
> > drm_modeset_helper.c:74: warning: No description found for parameter
> > 'dev'
> >
> > Cc: Laurent Pinchart
> > Cc: Alex Deucher
> > Fixes: a3f913ca9892 ("drm: Pass 'dev' to drm_helper_mode_fill_fb_struct()")
> > Signed-off-by: Ville Syrjälä
>
> Reviewed-by: Alex Deucher
Pushed to drm-misc-next. Thanks for the reviews.
>
> > ---
> > drivers/gpu/drm/drm_modeset_helper.c | 1 +
> > 1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/gpu/drm/drm_modeset_helper.c
> > b/drivers/gpu/drm/drm_modeset_helper.c
> > index f78df06a940d..4b31f5f70177 100644
> > --- a/drivers/gpu/drm/drm_modeset_helper.c
> > +++ b/drivers/gpu/drm/drm_modeset_helper.c
> > @@ -62,6 +62,7 @@
> > EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head);
> >
> > /**
> > * drm_helper_mode_fill_fb_struct - fill out framebuffer metadata
> > + * @dev: DRM device
> > * @fb: drm_framebuffer object to fill out
> > * @mode_cmd: metadata from the userspace fb creation request
> > *
> > --
> > 2.10.2
>
--
Ville Syrjälä
Intel OTC
[PATCH] kref: prefer atomic_inc_not_zero to atomic_add_unless
On most platforms, there exists this ifdef:
#define atomic_inc_not_zero(v) atomic_add_unless((v), 1, 0)
This makes this patch functionally useless. However, on PPC, there is
actually an explicit definition of atomic_inc_not_zero with its own
assembly that is slightly more optimized than atomic_add_unless. So,
this patch changes kref to use atomic_inc_not_zero instead, for PPC and
any future platforms that might provide an explicit implementation.
This also puts this usage of kref more in line with a verbatim reading
of the examples in Paul McKenney's paper [1] in the section titled "2.4
Atomic Counting With Check and Release Memory Barrier", which uses
atomic_inc_not_zero.
[1] http://open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2167.pdf
Signed-off-by: Jason A. Donenfeld
Reviewed-by: Thomas Hellstrom
Reviewed-by: Christoph Hellwig
---
Sorry to submit this again, but people keep reviewing it saying it's fine,
but then point to somebody else to actually merge this. At the end of the
chain of fingerpointing is usually Greg. "Just have Greg do it." At this
point I'm confused, but it's certainly been sufficiently reviewed and
accepted. So can one of you just respond saying "I'll take it!"
include/linux/kref.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/linux/kref.h b/include/linux/kref.h
index e15828fd71f1..62f0a84ae94e 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -133,6 +133,6 @@ static inline int kref_put_mutex(struct kref *kref,
*/
static inline int __must_check kref_get_unless_zero(struct kref *kref)
{
- return atomic_add_unless(&kref->refcount, 1, 0);
+ return atomic_inc_not_zero(&kref->refcount);
}
#endif /* _KREF_H_ */
--
2.11.0
[PATCH] drm: Fix kernel docs for drm_helper_mode_fill_fb_struct()
Hi Ville,
Thank you for the patch.
On Thursday 15 Dec 2016 19:01:28 ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä
>
> drm_modeset_helper.c:74: warning: No description found for parameter 'dev'
>
> Cc: Laurent Pinchart
> Cc: Alex Deucher
> Fixes: a3f913ca9892 ("drm: Pass 'dev' to drm_helper_mode_fill_fb_struct()")
> Signed-off-by: Ville Syrjälä
Reviewed-by: Laurent Pinchart
> ---
> drivers/gpu/drm/drm_modeset_helper.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c
> b/drivers/gpu/drm/drm_modeset_helper.c index f78df06a940d..4b31f5f70177
> 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -62,6 +62,7 @@ EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head);
>
> /**
> * drm_helper_mode_fill_fb_struct - fill out framebuffer metadata
> + * @dev: DRM device
> * @fb: drm_framebuffer object to fill out
> * @mode_cmd: metadata from the userspace fb creation request
> *
--
Regards,
Laurent Pinchart
Question about Radeon DRM and fan speed control
Hello I'm looking form some help. I need to write (very basic) tool to control radeon fan speed. I know i can use DRM. But where i can find any samples or example code? How to start? My os is GhostBSD (FreeBSD). Regards. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/0738ef98/attachment.html>
[PATCH] drm: Fix kernel docs for drm_helper_mode_fill_fb_struct()
From: Ville Syrjälä
drm_modeset_helper.c:74: warning: No description found for parameter 'dev'
Cc: Laurent Pinchart
Cc: Alex Deucher
Fixes: a3f913ca9892 ("drm: Pass 'dev' to drm_helper_mode_fill_fb_struct()")
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/drm_modeset_helper.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/gpu/drm/drm_modeset_helper.c
b/drivers/gpu/drm/drm_modeset_helper.c
index f78df06a940d..4b31f5f70177 100644
--- a/drivers/gpu/drm/drm_modeset_helper.c
+++ b/drivers/gpu/drm/drm_modeset_helper.c
@@ -62,6 +62,7 @@ EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head);
/**
* drm_helper_mode_fill_fb_struct - fill out framebuffer metadata
+ * @dev: DRM device
* @fb: drm_framebuffer object to fill out
* @mode_cmd: metadata from the userspace fb creation request
*
--
2.10.2
[PATCH v2] drm/amd/amdgpu: add check that shadow page tables are GPU-accessible
From: Nicolai Hähnle
Skip amdgpu_gem_va_update_vm otherwise. Also clean up the check for the
non-shadow page tables using the new helper function.
This fixes a crash with the stack trace:
amdgpu_gem_va_update_vm
-> amdgpu_vm_update_page_directory
-> amdgpu_ttm_bind
-> amdgpu_gtt_mgr_alloc
v2: actually check bo->shadow instead of just checking bo twice
Signed-off-by: Nicolai Hähnle
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 4e1eb05..9bd1b4e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -464,26 +464,29 @@ int amdgpu_gem_metadata_ioctl(struct drm_device *dev,
void *data,
unreserve:
amdgpu_bo_unreserve(robj);
out:
drm_gem_object_unreference_unlocked(gobj);
return r;
}
static int amdgpu_gem_va_check(void *param, struct amdgpu_bo *bo)
{
- unsigned domain = amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type);
-
/* if anything is swapped out don't swap it in here,
just abort and wait for the next CS */
+ if (!amdgpu_bo_gpu_accessible(bo))
+ return -ERESTARTSYS;
+
+ if (bo->shadow && !amdgpu_bo_gpu_accessible(bo->shadow))
+ return -ERESTARTSYS;
- return domain == AMDGPU_GEM_DOMAIN_CPU ? -ERESTARTSYS : 0;
+ return 0;
}
/**
* amdgpu_gem_va_update_vm -update the bo_va in its VM
*
* @adev: amdgpu_device pointer
* @bo_va: bo_va to update
*
* Update the bo_va directly after setting it's address. Errors are not
* vital here, so they are not reported back to userspace.
--
2.7.4
[PATCH 5/5] drm/amd/amdgpu: add check that shadow page tables are GPU-accessible
From: Nicolai Hähnle
Skip amdgpu_gem_va_update_vm otherwise. Also clean up the check for the
non-shadow page tables using the new helper function.
This fixes a crash with the stack trace:
amdgpu_gem_va_update_vm
-> amdgpu_vm_update_page_directory
-> amdgpu_ttm_bind
-> amdgpu_gtt_mgr_alloc
Signed-off-by: Nicolai Hähnle
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 9 ++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 4e1eb05..d91c80b 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -464,26 +464,29 @@ int amdgpu_gem_metadata_ioctl(struct drm_device *dev,
void *data,
unreserve:
amdgpu_bo_unreserve(robj);
out:
drm_gem_object_unreference_unlocked(gobj);
return r;
}
static int amdgpu_gem_va_check(void *param, struct amdgpu_bo *bo)
{
- unsigned domain = amdgpu_mem_type_to_domain(bo->tbo.mem.mem_type);
-
/* if anything is swapped out don't swap it in here,
just abort and wait for the next CS */
+ if (!amdgpu_bo_gpu_accessible(bo))
+ return -ERESTARTSYS;
+
+ if (bo->shadow && !amdgpu_bo_gpu_accessible(bo))
+ return -ERESTARTSYS;
- return domain == AMDGPU_GEM_DOMAIN_CPU ? -ERESTARTSYS : 0;
+ return 0;
}
/**
* amdgpu_gem_va_update_vm -update the bo_va in its VM
*
* @adev: amdgpu_device pointer
* @bo_va: bo_va to update
*
* Update the bo_va directly after setting it's address. Errors are not
* vital here, so they are not reported back to userspace.
--
2.7.4
[PATCH 4/5] drm/amd/amdgpu: add check that shadow page directory is GPU-accessible
From: Nicolai Hähnle
Skip amdgpu_gem_va_update_vm when shadow the page directory is swapped out.
Clean up the check for non-shadow BOs as well using the new helper function.
This fixes a crash with the stack trace:
amdgpu_gem_va_update_vm
-> amdgpu_vm_update_page_directory
-> amdgpu_ttm_bind
-> amdgpu_gtt_mgr_alloc
Signed-off-by: Nicolai Hähnle
---
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index cd62f6f..4e1eb05 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -489,44 +489,49 @@ static int amdgpu_gem_va_check(void *param, struct
amdgpu_bo *bo)
* vital here, so they are not reported back to userspace.
*/
static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
struct amdgpu_bo_va *bo_va,
uint32_t operation)
{
struct ttm_validate_buffer tv, *entry;
struct amdgpu_bo_list_entry vm_pd;
struct ww_acquire_ctx ticket;
struct list_head list, duplicates;
- unsigned domain;
int r;
INIT_LIST_HEAD(&list);
INIT_LIST_HEAD(&duplicates);
tv.bo = &bo_va->bo->tbo;
tv.shared = true;
list_add(&tv.head, &list);
amdgpu_vm_get_pd_bo(bo_va->vm, &list, &vm_pd);
/* Provide duplicates to avoid -EALREADY */
r = ttm_eu_reserve_buffers(&ticket, &list, true, &duplicates);
if (r)
goto error_print;
list_for_each_entry(entry, &list, head) {
- domain = amdgpu_mem_type_to_domain(entry->bo->mem.mem_type);
+ struct amdgpu_bo *bo =
+ container_of(entry->bo, struct amdgpu_bo, tbo);
+
/* if anything is swapped out don't swap it in here,
just abort and wait for the next CS */
- if (domain == AMDGPU_GEM_DOMAIN_CPU)
+ if (!amdgpu_bo_gpu_accessible(bo))
+ goto error_unreserve;
+
+ if (bo->shadow && !amdgpu_bo_gpu_accessible(bo->shadow))
goto error_unreserve;
}
+
r = amdgpu_vm_validate_pt_bos(adev, bo_va->vm, amdgpu_gem_va_check,
NULL);
if (r)
goto error_unreserve;
r = amdgpu_vm_update_page_directory(adev, bo_va->vm);
if (r)
goto error_unreserve;
r = amdgpu_vm_clear_freed(adev, bo_va->vm);
--
2.7.4
[PATCH 3/5] drm/amd/amdgpu: add amdgpu_bo_gpu_accessible helper function
From: Nicolai Hähnle
Signed-off-by: Nicolai Hähnle
---
drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 4306b2f..15a723a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -107,20 +107,29 @@ static inline unsigned
amdgpu_bo_gpu_page_alignment(struct amdgpu_bo *bo)
* amdgpu_bo_mmap_offset - return mmap offset of bo
* @bo:amdgpu object for which we query the offset
*
* Returns mmap offset of the object.
*/
static inline u64 amdgpu_bo_mmap_offset(struct amdgpu_bo *bo)
{
return drm_vma_node_offset_addr(&bo->tbo.vma_node);
}
+/**
+ * amdgpu_bo_gpu_accessible - return whether the bo is currently in memory that
+ * is accessible to the GPU.
+ */
+static inline bool amdgpu_bo_gpu_accessible(struct amdgpu_bo *bo)
+{
+ return bo->tbo.mem.mem_type != TTM_PL_SYSTEM;
+}
+
int amdgpu_bo_create(struct amdgpu_device *adev,
unsigned long size, int byte_align,
bool kernel, u32 domain, u64 flags,
struct sg_table *sg,
struct reservation_object *resv,
struct amdgpu_bo **bo_ptr);
int amdgpu_bo_create_restricted(struct amdgpu_device *adev,
unsigned long size, int byte_align,
bool kernel, u32 domain, u64 flags,
struct sg_table *sg,
--
2.7.4
[PATCH 2/5] drm/amd/amdgpu: move eviction counting to amdgpu_bo_move_notify
From: Nicolai Hähnle
This catches evictions of shadow page tables from the GART. Since shadow
page tables are always stored in system memory, amdgpu_bo_move is never
called for them.
This fixes a crash during command submission that occurs when only a shadow
page table and no other BOs were evicted since the last submission.
Fixes: 1baa439fb2f4e586 ("drm/amdgpu: allocate shadow for pd/pt bo V2")
Signed-off-by: Nicolai Hähnle
---
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 4
drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 4
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index c29db99..d94cdef 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -855,20 +855,24 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev);
struct amdgpu_bo *abo;
struct ttm_mem_reg *old_mem = &bo->mem;
if (!amdgpu_ttm_bo_is_amdgpu_bo(bo))
return;
abo = container_of(bo, struct amdgpu_bo, tbo);
amdgpu_vm_bo_invalidate(adev, abo);
+ /* remember the eviction */
+ if (evict)
+ atomic64_inc(&adev->num_evictions);
+
/* update statistics */
if (!new_mem)
return;
/* move_notify is called before move happens */
amdgpu_update_memory_usage(adev, &bo->mem, new_mem);
trace_amdgpu_ttm_bo_move(abo, new_mem->mem_type, old_mem->mem_type);
}
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
index 8f18b8e..80924c2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c
@@ -460,24 +460,20 @@ static int amdgpu_bo_move(struct ttm_buffer_object *bo,
struct ttm_mem_reg *old_mem = &bo->mem;
int r;
/* Can't move a pinned BO */
abo = container_of(bo, struct amdgpu_bo, tbo);
if (WARN_ON_ONCE(abo->pin_count > 0))
return -EINVAL;
adev = amdgpu_ttm_adev(bo->bdev);
- /* remember the eviction */
- if (evict)
- atomic64_inc(&adev->num_evictions);
-
if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) {
amdgpu_move_null(bo, new_mem);
return 0;
}
if ((old_mem->mem_type == TTM_PL_TT &&
new_mem->mem_type == TTM_PL_SYSTEM) ||
(old_mem->mem_type == TTM_PL_SYSTEM &&
new_mem->mem_type == TTM_PL_TT)) {
/* bind is enough */
amdgpu_move_null(bo, new_mem);
--
2.7.4
[PATCH 1/5] drm/ttm: add evict parameter to ttm_bo_driver::move_notify
From: Nicolai Hähnle
Ensure that the driver can listen to evictions even when they don't take the
path through ttm_bo_driver::move.
This is crucial for amdgpu, which relies on an eviction counter to skip
re-binding page tables when possible.
Signed-off-by: Nicolai Hähnle
---
drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 1 +
drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 3 ++-
drivers/gpu/drm/nouveau/nouveau_bo.c | 3 ++-
drivers/gpu/drm/qxl/qxl_ttm.c | 1 +
drivers/gpu/drm/radeon/radeon_object.c | 1 +
drivers/gpu/drm/radeon/radeon_object.h | 1 +
drivers/gpu/drm/ttm/ttm_bo.c | 8
drivers/gpu/drm/virtio/virtgpu_ttm.c | 1 +
drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c | 1 +
include/drm/ttm/ttm_bo_driver.h| 10 --
10 files changed, 22 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index bf79b73..c29db99 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -842,20 +842,21 @@ int amdgpu_bo_get_metadata(struct amdgpu_bo *bo, void
*buffer,
if (metadata_size)
*metadata_size = bo->metadata_size;
if (flags)
*flags = bo->metadata_flags;
return 0;
}
void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
+ bool evict,
struct ttm_mem_reg *new_mem)
{
struct amdgpu_device *adev = amdgpu_ttm_adev(bo->bdev);
struct amdgpu_bo *abo;
struct ttm_mem_reg *old_mem = &bo->mem;
if (!amdgpu_ttm_bo_is_amdgpu_bo(bo))
return;
abo = container_of(bo, struct amdgpu_bo, tbo);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 5cbf59e..4306b2f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -148,21 +148,22 @@ void amdgpu_bo_fini(struct amdgpu_device *adev);
int amdgpu_bo_fbdev_mmap(struct amdgpu_bo *bo,
struct vm_area_struct *vma);
int amdgpu_bo_set_tiling_flags(struct amdgpu_bo *bo, u64 tiling_flags);
void amdgpu_bo_get_tiling_flags(struct amdgpu_bo *bo, u64 *tiling_flags);
int amdgpu_bo_set_metadata (struct amdgpu_bo *bo, void *metadata,
uint32_t metadata_size, uint64_t flags);
int amdgpu_bo_get_metadata(struct amdgpu_bo *bo, void *buffer,
size_t buffer_size, uint32_t *metadata_size,
uint64_t *flags);
void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
- struct ttm_mem_reg *new_mem);
+ bool evict,
+ struct ttm_mem_reg *new_mem);
int amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object *bo);
void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence,
bool shared);
u64 amdgpu_bo_gpu_offset(struct amdgpu_bo *bo);
int amdgpu_bo_backup_to_shadow(struct amdgpu_device *adev,
struct amdgpu_ring *ring,
struct amdgpu_bo *bo,
struct reservation_object *resv,
struct dma_fence **fence, bool direct);
int amdgpu_bo_restore_from_shadow(struct amdgpu_device *adev,
diff --git a/drivers/gpu/drm/nouveau/nouveau_bo.c
b/drivers/gpu/drm/nouveau/nouveau_bo.c
index e0c0007..6fa1521 100644
--- a/drivers/gpu/drm/nouveau/nouveau_bo.c
+++ b/drivers/gpu/drm/nouveau/nouveau_bo.c
@@ -1187,21 +1187,22 @@ nouveau_bo_move_flips(struct ttm_buffer_object *bo,
bool evict, bool intr,
ret = nouveau_bo_move_m2mf(bo, true, intr, no_wait_gpu, new_mem);
if (ret)
goto out;
out:
ttm_bo_mem_put(bo, &tmp_mem);
return ret;
}
static void
-nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, struct ttm_mem_reg *new_mem)
+nouveau_bo_move_ntfy(struct ttm_buffer_object *bo, bool evict,
+struct ttm_mem_reg *new_mem)
{
struct nouveau_bo *nvbo = nouveau_bo(bo);
struct nvkm_vma *vma;
/* ttm can now (stupidly) pass the driver bos it didn't create... */
if (bo->destroy != nouveau_bo_del_ttm)
return;
list_for_each_entry(vma, &nvbo->vma_list, head) {
if (new_mem && new_mem->mem_type != TTM_PL_SYSTEM &&
diff --git a/drivers/gpu/drm/qxl/qxl_ttm.c b/drivers/gpu/drm/qxl/qxl_ttm.c
index 1176133..f3939a9 100644
--- a/drivers/gpu/drm/qxl/qxl_ttm.c
+++ b/drivers/gpu/drm/qxl/qxl_ttm.c
@@ -360,20 +360,21 @@ static int qxl_bo_move(struct ttm_buffer_object *bo,
if (old_mem->mem_type == TTM_PL_SYSTEM && bo->ttm == NULL) {
qxl_move_null(bo, new_mem);
return 0;
}
return ttm_bo_move_memcpy(bo, interruptible, no_wait_gpu,
[PATCH 0/5] drm/ttm, amdgpu: fix crashes due to shadow page table evictions
Fix a bunch of related crashes in amdgpu that occur when shadow page tables are kicked out of the GART. One of the issues was that during command submission, we rely on a device-global evictions counter to skip some of the work of page-table validation. The driver was never informed of evictions from GART, so this series adds an evict parameter to ttm_bo_driver::move_notify. There's still the evict parameter on ttm_bo_driver::move which is used by radeon and nouveau for the call to ttm_bo_move_accel_cleanup. The 'evict' parameter there should probably be more accurately called 'wait', but unless that wait can always be avoided, the evict parameter on ttm_bo_driver::move needs to stay. Please review! Nicolai --- drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c| 20 ++-- drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 5 + drivers/gpu/drm/amd/amdgpu/amdgpu_object.h | 12 +++- drivers/gpu/drm/amd/amdgpu/amdgpu_ttm.c| 4 drivers/gpu/drm/nouveau/nouveau_bo.c | 3 ++- drivers/gpu/drm/qxl/qxl_ttm.c | 1 + drivers/gpu/drm/radeon/radeon_object.c | 1 + drivers/gpu/drm/radeon/radeon_object.h | 1 + drivers/gpu/drm/ttm/ttm_bo.c | 8 drivers/gpu/drm/virtio/virtgpu_ttm.c | 1 + drivers/gpu/drm/vmwgfx/vmwgfx_buffer.c | 1 + include/drm/ttm/ttm_bo_driver.h| 10 -- 12 files changed, 49 insertions(+), 18 deletions(-)
[PATCH v2 3/6] drm/atomic: Clean up wait_for_vblanks, v2.
Op 15-12-16 om 16:55 schreef Daniel Vetter: > On Thu, Dec 15, 2016 at 12:51:42PM +0100, Maarten Lankhorst wrote: >> Stop relying on a per crtc_state last_vblank_count, we shouldn't touch >> crtc_state after commit. Move it to atomic_state->crtcs. >> >> Also stop re-using new_crtc_state->enable, we can now simply set a >> bitmask with crtc_crtc_mask. >> >> Changes since v1: >> - Keep last_vblank_count in __drm_crtc_state. > Just noticed: sob is missing. > > With that fixed Reviewed-by: Daniel Vetter Oh indeed, noticed it too when resending but forgot to re-add. Signed-off-by: Maarten Lankhorst
[PATCH] drm: Fix kernel docs for drm_helper_mode_fill_fb_struct()
> -Original Message-
> From: ville.syrjala at linux.intel.com [mailto:ville.syrjala at
> linux.intel.com]
> Sent: Thursday, December 15, 2016 12:01 PM
> To: dri-devel at lists.freedesktop.org
> Cc: Laurent Pinchart; Deucher, Alexander
> Subject: [PATCH] drm: Fix kernel docs for drm_helper_mode_fill_fb_struct()
>
> From: Ville Syrjälä
>
> drm_modeset_helper.c:74: warning: No description found for parameter
> 'dev'
>
> Cc: Laurent Pinchart
> Cc: Alex Deucher
> Fixes: a3f913ca9892 ("drm: Pass 'dev' to drm_helper_mode_fill_fb_struct()")
> Signed-off-by: Ville Syrjälä
Reviewed-by: Alex Deucher
> ---
> drivers/gpu/drm/drm_modeset_helper.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/drm_modeset_helper.c
> b/drivers/gpu/drm/drm_modeset_helper.c
> index f78df06a940d..4b31f5f70177 100644
> --- a/drivers/gpu/drm/drm_modeset_helper.c
> +++ b/drivers/gpu/drm/drm_modeset_helper.c
> @@ -62,6 +62,7 @@
> EXPORT_SYMBOL(drm_helper_move_panel_connectors_to_head);
>
> /**
> * drm_helper_mode_fill_fb_struct - fill out framebuffer metadata
> + * @dev: DRM device
> * @fb: drm_framebuffer object to fill out
> * @mode_cmd: metadata from the userspace fb creation request
> *
> --
> 2.10.2
[PATCH] drm/mediatek: Support UYVY and YUYV format for overlay
Hi Bibby, On Wed, Dec 14, 2016 at 1:14 PM, Bibby Hsieh wrote: > > MT8173 overlay can support UYVY and YUYV format, > we add the format in DRM driver. > > Signed-off-by: Bibby Hsieh > --- > drivers/gpu/drm/mediatek/mtk_disp_ovl.c | 6 ++ > drivers/gpu/drm/mediatek/mtk_drm_plane.c | 2 ++ > 2 files changed, 8 insertions(+) > > diff --git a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c > b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c > index 019b7ca..0a340f3 100644 > --- a/drivers/gpu/drm/mediatek/mtk_disp_ovl.c > +++ b/drivers/gpu/drm/mediatek/mtk_disp_ovl.c > @@ -44,6 +44,8 @@ > #define OVL_CON_CLRFMT_RGB888 (1 << 12) > #define OVL_CON_CLRFMT_RGBA(2 << 12) > #define OVL_CON_CLRFMT_ARGB(3 << 12) > +#define OVL_CON_CLRFMT_UYVY(4 << 12) > +#define OVL_CON_CLRFMT_YUYV(5 << 12) > #defineOVL_CON_AEN BIT(8) > #defineOVL_CON_ALPHA 0xff > > @@ -161,6 +163,10 @@ static unsigned int ovl_fmt_convert(unsigned int fmt) > case DRM_FORMAT_XBGR: > case DRM_FORMAT_ABGR: > return OVL_CON_CLRFMT_RGBA | OVL_CON_BYTE_SWAP; > + case DRM_FORMAT_YUYV: > + return OVL_CON_CLRFMT_YUYV; > + case DRM_FORMAT_UYVY: > + return OVL_CON_CLRFMT_UYVY; nit: probably better to alphabetize these (UYVY before YUVU). > } > } > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > index c461a23..b94c6ee 100644 > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c > @@ -28,6 +28,8 @@ > DRM_FORMAT_XRGB, > DRM_FORMAT_ARGB, > DRM_FORMAT_RGB565, > + DRM_FORMAT_YUYV, > + DRM_FORMAT_UYVY, nit: probably better to alphabetize these. Other than that, Reviewed-by: Daniel Kurtz > > }; > > static void mtk_plane_reset(struct drm_plane *plane) > -- > 1.9.1 >
[PATCH] drm: Convert all helpers to drm_connector_list_iter
Mostly nothing special (except making sure that really all error paths
and friends call iter_put).
v2: Don't forget the raw connector_list walking in
drm_helper_move_panel_connectors_to_head. That one unfortunately can't
be converted to the iterator helpers, but since it's just some list
splicing best to just wrap the entire thing up in one critical
section.
v3: Bail out after iter_put (Harry).
Cc: Harry Wentland
Reviewed-by: Harry Wentland
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_atomic_helper.c | 39
drivers/gpu/drm/drm_crtc_helper.c| 49
drivers/gpu/drm/drm_fb_helper.c | 12 ++---
drivers/gpu/drm/drm_modeset_helper.c | 2 ++
drivers/gpu/drm/drm_plane_helper.c | 5 +++-
drivers/gpu/drm/drm_probe_helper.c | 18 -
6 files changed, 92 insertions(+), 33 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index 23767df72615..e2e15a9903a9 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -94,9 +94,10 @@ static int handle_conflicting_encoders(struct
drm_atomic_state *state,
{
struct drm_connector_state *conn_state;
struct drm_connector *connector;
+ struct drm_connector_list_iter conn_iter;
struct drm_encoder *encoder;
unsigned encoder_mask = 0;
- int i, ret;
+ int i, ret = 0;
/*
* First loop, find all newly assigned encoders from the connectors
@@ -144,7 +145,8 @@ static int handle_conflicting_encoders(struct
drm_atomic_state *state,
* and the crtc is disabled if no encoder is left. This preserves
* compatibility with the legacy set_config behavior.
*/
- drm_for_each_connector(connector, state->dev) {
+ drm_connector_list_iter_get(state->dev, &conn_iter);
+ drm_for_each_connector_iter(connector, &conn_iter) {
struct drm_crtc_state *crtc_state;
if (drm_atomic_get_existing_connector_state(state, connector))
@@ -160,12 +162,15 @@ static int handle_conflicting_encoders(struct
drm_atomic_state *state,
connector->state->crtc->base.id,
connector->state->crtc->name,
connector->base.id, connector->name);
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
conn_state = drm_atomic_get_connector_state(state, connector);
- if (IS_ERR(conn_state))
- return PTR_ERR(conn_state);
+ if (IS_ERR(conn_state)) {
+ ret = PTR_ERR(conn_state);
+ goto out;
+ }
DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s],
disabling [CONNECTOR:%d:%s]\n",
encoder->base.id, encoder->name,
@@ -176,19 +181,21 @@ static int handle_conflicting_encoders(struct
drm_atomic_state *state,
ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
if (ret)
- return ret;
+ goto out;
if (!crtc_state->connector_mask) {
ret = drm_atomic_set_mode_prop_for_crtc(crtc_state,
NULL);
if (ret < 0)
- return ret;
+ goto out;
crtc_state->active = false;
}
}
+out:
+ drm_connector_list_iter_put(&conn_iter);
- return 0;
+ return ret;
}
static void
@@ -2442,6 +2449,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
{
struct drm_atomic_state *state;
struct drm_connector *conn;
+ struct drm_connector_list_iter conn_iter;
int err;
state = drm_atomic_state_alloc(dev);
@@ -2450,7 +2458,8 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
state->acquire_ctx = ctx;
- drm_for_each_connector(conn, dev) {
+ drm_connector_list_iter_get(dev, &conn_iter);
+ drm_for_each_connector_iter(conn, &conn_iter) {
struct drm_crtc *crtc = conn->state->crtc;
struct drm_crtc_state *crtc_state;
@@ -2468,6 +2477,7 @@ int drm_atomic_helper_disable_all(struct drm_device *dev,
err = drm_atomic_commit(state);
free:
+ drm_connector_list_iter_put(&conn_iter);
drm_atomic_state_put(state);
return err;
}
@@ -2840,6 +2850,7 @@ int drm_atomic_helper_connector_dpms(struct drm_connector
*connector,
struct drm_crtc_state *crtc_state;
struct drm_crtc *crtc;
struct drm_connector *tmp_connector;
+ struct drm_connector_list_iter conn_iter;
int ret;
bool active
[PATCH v4 14/22] drm: omapdrm: Keep vblank interrupt enabled while CRTC is active
On 15/12/16 16:51, Laurent Pinchart wrote: >>> + WARN_ON(drm_crtc_vblank_get(crtc) != 0); >> >> I don't like this style very much. I think WARN()s should be without >> side effects. >> >> Also, WARN only gives benefit when we don't know what the call stack is. >> Afaik, there's only one way omap_crtc_atomic_flush can be called, so >> it's just extra spam and dev_err or dev_warn should be enough. > > I've used it because the equivalent statements testing omap_crtc- >> vblank_irq.registered used WARN_ON() too. WARN_ON() is also a bit more >> vocal, > it really gets the point across. As the function really should not return an > error unless in case of a driver bug, I don't think it will generate any > spam. > I don't care too much though, I can replace it with a dev_err() if you insist. I don't mind using WARN_ON() that much. But that's the comment I've received a few times, so I shared it =). What I do care is the side effect inside WARN_ON. At least for me it's quite easy to miss that it's actually having a side effect, as I expect WARN_ON() to be just a check, like assert(). So when reading the code, I skip the WARN_ONs. Tomi -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/a142350b/attachment.sig>
[PATCH] drm/cma: Fix compile fail due to fomat->format typo
On Thu, 15 Dec 2016, Laurent Pinchart
wrote:
> Hi Ville,
>
> Thank you for the patch.
>
> On Thursday 15 Dec 2016 16:29:27 ville.syrjala at linux.intel.com wrote:
>> From: Ville Syrjälä
>>
>> Apparently my arm .config had reverted to CMA=n at some point, so I
>> failed to notice that I typoed the code. Fix it up so that the
>> cma helper will compile again.
>>
>> Reported-by: kbuild test robot
>> Cc: Laurent Pinchart
>> Fixes: ca984a998ad3 ("drm/fb_cma_helper: Replace drm_format_info() with
>> fb->format")
>> Signed-off-by: Ville Syrjälä
>
> Reviewed-by: Laurent Pinchart
>
> The kbuild test bot caught this pretty fast, but it was too late as the patch
> was already in a stable branch of a shared tree. Don't you have a git tree
> covered by the bot that you could push patches to during development ?
It was a randconfig result, so even that doesn't guarantee everything is
caught.
BR,
Jani.
>
>> ---
>> drivers/gpu/drm/drm_fb_cma_helper.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
>> b/drivers/gpu/drm/drm_fb_cma_helper.c index aab4465307ed..591f30ebc42a
>> 100644
>> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
>> @@ -309,7 +309,7 @@ static void drm_fb_cma_describe(struct drm_framebuffer
>> *fb, struct seq_file *m) seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width,
>> fb->height,
>> (char *)&fb->format->format);
>>
>> -for (i = 0; i < fb->fomat->num_planes; i++) {
>> +for (i = 0; i < fb->format->num_planes; i++) {
>> seq_printf(m, " %d: offset=%d pitch=%d, obj: ",
>> i, fb->offsets[i], fb->pitches[i]);
>> drm_gem_cma_describe(fb_cma->obj[i], m);
--
Jani Nikula, Intel Open Source Technology Center
[PATCH v2 3/6] drm/atomic: Clean up wait_for_vblanks, v2.
On Thu, Dec 15, 2016 at 12:51:42PM +0100, Maarten Lankhorst wrote:
> Stop relying on a per crtc_state last_vblank_count, we shouldn't touch
> crtc_state after commit. Move it to atomic_state->crtcs.
>
> Also stop re-using new_crtc_state->enable, we can now simply set a
> bitmask with crtc_crtc_mask.
>
> Changes since v1:
> - Keep last_vblank_count in __drm_crtc_state.
Just noticed: sob is missing.
With that fixed Reviewed-by: Daniel Vetter
> ---
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index d19563651e07..88c0986d226a 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -1110,19 +1110,19 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device
> *dev,
> struct drm_crtc *crtc;
> struct drm_crtc_state *old_crtc_state;
> int i, ret;
> + unsigned crtc_mask = 0;
>
> - for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> - /* No one cares about the old state, so abuse it for tracking
> - * and store whether we hold a vblank reference (and should do a
> - * vblank wait) in the ->enable boolean. */
> - old_crtc_state->enable = false;
> + /*
> + * Legacy cursor ioctls are completely unsynced, and userspace
> + * relies on that (by doing tons of cursor updates).
> + */
> + if (old_state->legacy_cursor_update)
> + return;
>
> - if (!crtc->state->active)
> - continue;
> + for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> + struct drm_crtc_state *new_crtc_state = crtc->state;
>
> - /* Legacy cursor ioctls are completely unsynced, and userspace
> - * relies on that (by doing tons of cursor updates). */
> - if (old_state->legacy_cursor_update)
> + if (!new_crtc_state->active)
> continue;
>
> if (!drm_atomic_helper_framebuffer_changed(dev,
> @@ -1133,16 +1133,16 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device
> *dev,
> if (ret != 0)
> continue;
>
> - old_crtc_state->enable = true;
> - old_crtc_state->last_vblank_count = drm_crtc_vblank_count(crtc);
> + crtc_mask |= drm_crtc_mask(crtc);
> + old_state->crtcs[i].last_vblank_count =
> drm_crtc_vblank_count(crtc);
> }
>
> for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
> - if (!old_crtc_state->enable)
> + if (!(crtc_mask & drm_crtc_mask(crtc)))
> continue;
>
> ret = wait_event_timeout(dev->vblank[i].queue,
> - old_crtc_state->last_vblank_count !=
> + old_state->crtcs[i].last_vblank_count !=
> drm_crtc_vblank_count(crtc),
> msecs_to_jiffies(50));
>
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index d6d241f63b9f..617f095e31ba 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -145,6 +145,7 @@ struct __drm_crtcs_state {
> struct drm_crtc_state *state;
> struct drm_crtc_commit *commit;
> s64 __user *out_fence_ptr;
> + unsigned last_vblank_count;
> };
>
> struct __drm_connnectors_state {
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 946672f97e1e..e03d194be086 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -93,8 +93,6 @@ struct drm_plane_helper_funcs;
> * @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
> * @connector_mask: bitmask of (1 << drm_connector_index(connector)) of
> attached connectors
> * @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached
> encoders
> - * @last_vblank_count: for helpers and drivers to capture the vblank of the
> - * update to ensure framebuffer cleanup isn't done too early
> * @adjusted_mode: for use by helpers and drivers to compute adjusted mode
> timings
> * @mode: current mode timings
> * @mode_blob: &drm_property_blob for @mode
> @@ -140,9 +138,6 @@ struct drm_crtc_state {
> u32 connector_mask;
> u32 encoder_mask;
>
> - /* last_vblank_count: for vblank waits before cleanup */
> - u32 last_vblank_count;
> -
> /* adjusted_mode: for use by helpers and drivers */
> struct drm_display_mode adjusted_mode;
>
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH v4 14/22] drm: omapdrm: Keep vblank interrupt enabled while CRTC is active
Hi Tomi,
On Thursday 15 Dec 2016 14:52:47 Tomi Valkeinen wrote:
> On 14/12/16 02:27, Laurent Pinchart wrote:
> > Instead of going through a complicated private IRQ registration
> > mechanism, handle the vblank interrupt activation with the standard
> > drm_crtc_vblank_get() and drm_crtc_vblank_put() mechanism. This will let
> > the DRM core keep the vblank interrupt enabled as long as needed to
> > update the frame counter.
> >
> > Signed-off-by: Laurent Pinchart
> > ---
> >
> > drivers/gpu/drm/omapdrm/omap_crtc.c | 38
> > drivers/gpu/drm/omapdrm/omap_drv.h | 1 +
> > drivers/gpu/drm/omapdrm/omap_irq.c | 4 +++-
> > 3 files changed, 18 insertions(+), 25 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > b/drivers/gpu/drm/omapdrm/omap_crtc.c index 827ac46a6d5e..1f5372042706
> > 100644
> > --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> > +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> > @@ -36,8 +36,6 @@ struct omap_crtc {
> >
> > struct videomode vm;
> >
> > - struct omap_drm_irq vblank_irq;
> > -
> > bool ignore_digit_sync_lost;
> > bool enabled;
> >
> > @@ -304,25 +302,24 @@ void omap_crtc_error_irq(struct drm_crtc *crtc,
> > uint32_t irqstatus)
> > DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name,
irqstatus);
> > }
> >
> > -static void omap_crtc_vblank_irq(struct omap_drm_irq *irq, uint32_t
> > irqstatus)
> > +void omap_crtc_vblank_irq(struct drm_crtc *crtc)
> > {
> > - struct omap_crtc *omap_crtc =
> > - container_of(irq, struct omap_crtc, vblank_irq);
> > - struct drm_device *dev = omap_crtc->base.dev;
> > - struct drm_crtc *crtc = &omap_crtc->base;
> > + struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> > + bool pending;
> >
> > if (dispc_mgr_go_busy(omap_crtc->channel))
> > return;
> >
> > DBG("%s: apply done", omap_crtc->name);
> >
> > - __omap_irq_unregister(dev, &omap_crtc->vblank_irq);
> > -
> > spin_lock(&crtc->dev->event_lock);
> >
> > - WARN_ON(!omap_crtc->pending);
> > + pending = omap_crtc->pending;
> >
> > omap_crtc->pending = false;
> > spin_unlock(&crtc->dev->event_lock);
> >
> > + if (pending)
> > + drm_crtc_vblank_put(crtc);
> > +
>
> I think there's a race.
>
> - irq: we get vblank irq
> - irq: GO is not set, so dispc_mgr_go_busy() check doesn't trigger
> - flush: we set GO
> - flush: we lock event_lock and set the pending & event, and unlock
> - irq: irq handler continues, sees pending and event, and thinks that
> the config has been taken into use, but in reality GO bit is set and
> it'll happen only on next vblank.
I think you're right. I'll try to fix it.
> > /* wake up userspace */
> > omap_crtc_complete_page_flip(&omap_crtc->base);
> >
> > @@ -340,8 +337,6 @@ static void omap_crtc_destroy(struct drm_crtc *crtc)
> >
> > DBG("%s", omap_crtc->name);
> >
> > - WARN_ON(omap_crtc->vblank_irq.registered);
> > -
> > drm_crtc_cleanup(crtc);
> >
> > kfree(omap_crtc);
> > @@ -353,14 +348,13 @@ static void omap_crtc_enable(struct drm_crtc *crtc)
> >
> > DBG("%s", omap_crtc->name);
> >
> > + drm_crtc_vblank_on(crtc);
> > + WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> > +
> > spin_lock_irq(&crtc->dev->event_lock);
> > WARN_ON(omap_crtc->pending);
> > omap_crtc->pending = true;
> > spin_unlock_irq(&crtc->dev->event_lock);
> > -
> > - omap_irq_register(crtc->dev, &omap_crtc->vblank_irq);
> > -
> > - drm_crtc_vblank_on(crtc);
> > }
> >
> > static void omap_crtc_disable(struct drm_crtc *crtc)
> > @@ -414,8 +408,6 @@ static void omap_crtc_atomic_flush(struct drm_crtc
> > *crtc,
> > {
> > struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> >
> > - WARN_ON(omap_crtc->vblank_irq.registered);
> > -
> > if (crtc->state->color_mgmt_changed) {
> > struct drm_color_lut *lut = NULL;
> > uint length = 0;
> > @@ -441,6 +433,10 @@ static void omap_crtc_atomic_flush(struct drm_crtc
> > *crtc,
> >
> > DBG("%s: GO", omap_crtc->name);
> >
> > + dispc_mgr_go(omap_crtc->channel);
> > +
> > + WARN_ON(drm_crtc_vblank_get(crtc) != 0);
>
> I don't like this style very much. I think WARN()s should be without
> side effects.
>
> Also, WARN only gives benefit when we don't know what the call stack is.
> Afaik, there's only one way omap_crtc_atomic_flush can be called, so
> it's just extra spam and dev_err or dev_warn should be enough.
I've used it because the equivalent statements testing omap_crtc-
>vblank_irq.registered used WARN_ON() too. WARN_ON() is also a bit more vocal,
it really gets the point across. As the function really should not return an
error unless in case of a driver bug, I don't think it will generate any spam.
I don't care too much though, I can replace it with a dev_err() if you insist.
--
Regards,
Laurent Pinchart
[PATCH] drm/cma: Fix compile fail due to fomat->format typo
On Thu, Dec 15, 2016 at 04:31:58PM +0200, Laurent Pinchart wrote:
> Hi Ville,
>
> Thank you for the patch.
>
> On Thursday 15 Dec 2016 16:29:27 ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä
> >
> > Apparently my arm .config had reverted to CMA=n at some point, so I
> > failed to notice that I typoed the code. Fix it up so that the
> > cma helper will compile again.
> >
> > Reported-by: kbuild test robot
> > Cc: Laurent Pinchart
> > Fixes: ca984a998ad3 ("drm/fb_cma_helper: Replace drm_format_info() with
> > fb->format")
> > Signed-off-by: Ville Syrjälä
>
> Reviewed-by: Laurent Pinchart
>
> The kbuild test bot caught this pretty fast, but it was too late as the patch
> was already in a stable branch of a shared tree. Don't you have a git tree
> covered by the bot that you could push patches to during development ?
Also there's 3 defconfigs covering drm drivers pretty well, and drm-misc
pushers are supposed to always use those before pushing. Would have caught
this here, because that's how I noticed it.
-Daniel
>
> > ---
> > drivers/gpu/drm/drm_fb_cma_helper.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > b/drivers/gpu/drm/drm_fb_cma_helper.c index aab4465307ed..591f30ebc42a
> > 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -309,7 +309,7 @@ static void drm_fb_cma_describe(struct drm_framebuffer
> > *fb, struct seq_file *m) seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width,
> > fb->height,
> > (char *)&fb->format->format);
> >
> > - for (i = 0; i < fb->fomat->num_planes; i++) {
> > + for (i = 0; i < fb->format->num_planes; i++) {
> > seq_printf(m, " %d: offset=%d pitch=%d, obj: ",
> > i, fb->offsets[i], fb->pitches[i]);
> > drm_gem_cma_describe(fb_cma->obj[i], m);
>
> --
> Regards,
>
> Laurent Pinchart
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[RFC] Using DC in amdgpu for upcoming GPU
Hi, I have been reading the ongoing discussion about what to do about AMD DC (Display Core) with great interest since I have started to put more time into developing OpenChrome DRM for VIA Technologies Chrome IGP. I particularly enjoyed reading what Tony Cheng wrote about what is going on inside AMD Radeon GPUs. As a graphics stack developer, I suppose I am still someone somewhat above a beginner level, and Chrome IGP might be considered garbage graphics to some (I do not really care what people say or think about it.), but since my background is really digital hardware design (self taught) rather than graphics device driver development, I will like to add my 2 cents (U.S.D.) to the discussion. I also consider myself an amateur semiconductor industry historian, and in particular, I have been a close watcher of Intel's business / hiring practice for many years. For some, what I am writing may not make sense or even offend some (my guess will be the people who work at Intel), but I will not pull any punches, and if you do not like what I write, let me know. (That does not mean I will necessarily take back my comment even if it offended you. I typically stand behind what I say, unless it is obvious that I am wrong.) While my understanding of DRM is still quite primitive, my simplistic understanding of why AMD is pushing DC is due to the following factors. 1) AMD is understaffed due to its precarious financial condition it is in right now (i.e., < $1 billion CoH and losing 7,000 employees since Year 2008 or so) 2) The complexity of the next generation ASIC is only getting worse due to the continuing process scaling = more transistors one has to use (i.e., TSMC 28 nm to GF 14 nm to probably Samsung / TSMC 10 nm or GF 7 nm) 3) Based on 1 and 2, unless the design productively can be improved, AMD will be late to market, and this can be the possible end to AMD as a corporation 4) Hence, in order to meet TtM and improve engineer productivity, AMD needs to reuse the existing pre-silicon / post-silicon bring up test code and share the code with the Windows side of the device driver developers 5) In addition, power is already the biggest design challenge, and very precise power management is crucial to the performance of the chip (i.e., it's not all about the laptop anymore, and desktop "monster" graphics cards also need power management for performance reasons, in order to manage heat generation) 6) AMD Radeon is really running an RTOS (Real Time Operating System) inside the GPU card, and they want to put the code to handle initialization / power management closer to the GPU rather than from the slower response x86 (or any other general purpose) microprocessor Since I will probably need to obtain "favors" down the road when I try to get OpenChrome DRM mainlined, I probably should not go into what I think of how Intel works on their graphics device driver stack (I do not mean to make this personal, but Intel is the "other" open source camp in the OSS x86 graphics world, so I find it a fair game to discuss the approach Intel takes from semiconductor industry perspective. I am probably going to overly generalize what is going on, so if you wanted to correct me, let me know.), but based on my understanding of how Intel works, Intel probably has more staffing resources than AMD when it comes to graphics device driver stack development. (and on the x86 microprocessor development side) Based on my understanding of where Intel stands financially, I feel like Intel is standing on very thin ice due to the following factors, and I will predict that they will eventually adopt AMD DC like design concept. (i.e., use of a HAL) Here is my logic. 1) PC (desktop and laptop) x86 processors are not selling very well, and my understanding is that since Year 2012 peak, x86 processor shipment is down 30% as of Year 2016 (I will say around $200 ASP) 2) Intel's margins are being propped up by the unnaturally high data center marketshare (99% for x86 data center microprocessors) and very high data center x86 processor ASP (Average Selling Price) of $600 (Up from $500 a few years ago due to AMD screwing up the Bulldozer microarchitecture. More on this later.) 3) Intel did a significant layoff in April 2016 where they targeted older (read "expensive"), experienced engineers 4) Like Cisco Systems (notorious for their annual summer time 5,000 layoff), Intel then turns around and goes in a hiring spree hiring from many graduate programs of U.S. second and third tier universities, bringing down the overall experience level of the engineering departments 5) While AMD is financially in a desperate shape, it will likely have one last chance in Zen microarchitecture to get back into the game (Zen will be the last chance for AMD, IMO.) 6) Since AMD is now fabless due to divestiture of the fabs in Year 2009 (GLOBALFOUNDRIES), it no longer has the financial burden of having to pay for the fab, wh
[PATCH] drm/cma: Fix compile fail due to fomat->format typo
On Thu, Dec 15, 2016 at 04:31:58PM +0200, Laurent Pinchart wrote:
> Hi Ville,
>
> Thank you for the patch.
>
> On Thursday 15 Dec 2016 16:29:27 ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä
> >
> > Apparently my arm .config had reverted to CMA=n at some point, so I
> > failed to notice that I typoed the code. Fix it up so that the
> > cma helper will compile again.
> >
> > Reported-by: kbuild test robot
> > Cc: Laurent Pinchart
> > Fixes: ca984a998ad3 ("drm/fb_cma_helper: Replace drm_format_info() with
> > fb->format")
> > Signed-off-by: Ville Syrjälä
>
> Reviewed-by: Laurent Pinchart
Thanks. Pushed to drm-misc-next.
>
> The kbuild test bot caught this pretty fast, but it was too late as the patch
> was already in a stable branch of a shared tree. Don't you have a git tree
> covered by the bot that you could push patches to during development ?
>
> > ---
> > drivers/gpu/drm/drm_fb_cma_helper.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > b/drivers/gpu/drm/drm_fb_cma_helper.c index aab4465307ed..591f30ebc42a
> > 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -309,7 +309,7 @@ static void drm_fb_cma_describe(struct drm_framebuffer
> > *fb, struct seq_file *m) seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width,
> > fb->height,
> > (char *)&fb->format->format);
> >
> > - for (i = 0; i < fb->fomat->num_planes; i++) {
> > + for (i = 0; i < fb->format->num_planes; i++) {
> > seq_printf(m, " %d: offset=%d pitch=%d, obj: ",
> > i, fb->offsets[i], fb->pitches[i]);
> > drm_gem_cma_describe(fb_cma->obj[i], m);
>
> --
> Regards,
>
> Laurent Pinchart
--
Ville Syrjälä
Intel OTC
[PATCH] drm/cma: Fix compile fail due to fomat->format typo
On Thu, Dec 15, 2016 at 04:31:58PM +0200, Laurent Pinchart wrote:
> Hi Ville,
>
> Thank you for the patch.
>
> On Thursday 15 Dec 2016 16:29:27 ville.syrjala at linux.intel.com wrote:
> > From: Ville Syrjälä
> >
> > Apparently my arm .config had reverted to CMA=n at some point, so I
> > failed to notice that I typoed the code. Fix it up so that the
> > cma helper will compile again.
> >
> > Reported-by: kbuild test robot
> > Cc: Laurent Pinchart
> > Fixes: ca984a998ad3 ("drm/fb_cma_helper: Replace drm_format_info() with
> > fb->format")
> > Signed-off-by: Ville Syrjälä
>
> Reviewed-by: Laurent Pinchart
>
> The kbuild test bot caught this pretty fast, but it was too late as the patch
> was already in a stable branch of a shared tree. Don't you have a git tree
> covered by the bot that you could push patches to during development ?
I seem to recall getting 0day reports for my github repo at some point
in the past. But maybe I imagined it.
Fengguang, assuming 0day has bandwidth for it adding my repo [1] to your
list might be nice. I tend to push most non-trivial patch series there
when I post the patches.
[1] git://github.com/vsyrjala/linux.git
>
> > ---
> > drivers/gpu/drm/drm_fb_cma_helper.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> > b/drivers/gpu/drm/drm_fb_cma_helper.c index aab4465307ed..591f30ebc42a
> > 100644
> > --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> > +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> > @@ -309,7 +309,7 @@ static void drm_fb_cma_describe(struct drm_framebuffer
> > *fb, struct seq_file *m) seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width,
> > fb->height,
> > (char *)&fb->format->format);
> >
> > - for (i = 0; i < fb->fomat->num_planes; i++) {
> > + for (i = 0; i < fb->format->num_planes; i++) {
> > seq_printf(m, " %d: offset=%d pitch=%d, obj: ",
> > i, fb->offsets[i], fb->pitches[i]);
> > drm_gem_cma_describe(fb_cma->obj[i], m);
>
> --
> Regards,
>
> Laurent Pinchart
--
Ville Syrjälä
Intel OTC
[PATCH] drm/cma: Fix compile fail due to fomat->format typo
Hi Ville,
Thank you for the patch.
On Thursday 15 Dec 2016 16:29:27 ville.syrjala at linux.intel.com wrote:
> From: Ville Syrjälä
>
> Apparently my arm .config had reverted to CMA=n at some point, so I
> failed to notice that I typoed the code. Fix it up so that the
> cma helper will compile again.
>
> Reported-by: kbuild test robot
> Cc: Laurent Pinchart
> Fixes: ca984a998ad3 ("drm/fb_cma_helper: Replace drm_format_info() with
> fb->format")
> Signed-off-by: Ville Syrjälä
Reviewed-by: Laurent Pinchart
The kbuild test bot caught this pretty fast, but it was too late as the patch
was already in a stable branch of a shared tree. Don't you have a git tree
covered by the bot that you could push patches to during development ?
> ---
> drivers/gpu/drm/drm_fb_cma_helper.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
> b/drivers/gpu/drm/drm_fb_cma_helper.c index aab4465307ed..591f30ebc42a
> 100644
> --- a/drivers/gpu/drm/drm_fb_cma_helper.c
> +++ b/drivers/gpu/drm/drm_fb_cma_helper.c
> @@ -309,7 +309,7 @@ static void drm_fb_cma_describe(struct drm_framebuffer
> *fb, struct seq_file *m) seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width,
> fb->height,
> (char *)&fb->format->format);
>
> - for (i = 0; i < fb->fomat->num_planes; i++) {
> + for (i = 0; i < fb->format->num_planes; i++) {
> seq_printf(m, " %d: offset=%d pitch=%d, obj: ",
> i, fb->offsets[i], fb->pitches[i]);
> drm_gem_cma_describe(fb_cma->obj[i], m);
--
Regards,
Laurent Pinchart
[PATCH] drm/cma: Fix compile fail due to fomat->format typo
From: Ville Syrjälä
Apparently my arm .config had reverted to CMA=n at some point, so I
failed to notice that I typoed the code. Fix it up so that the
cma helper will compile again.
Reported-by: kbuild test robot
Cc: Laurent Pinchart
Fixes: ca984a998ad3 ("drm/fb_cma_helper: Replace drm_format_info() with
fb->format")
Signed-off-by: Ville Syrjälä
---
drivers/gpu/drm/drm_fb_cma_helper.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/drm_fb_cma_helper.c
b/drivers/gpu/drm/drm_fb_cma_helper.c
index aab4465307ed..591f30ebc42a 100644
--- a/drivers/gpu/drm/drm_fb_cma_helper.c
+++ b/drivers/gpu/drm/drm_fb_cma_helper.c
@@ -309,7 +309,7 @@ static void drm_fb_cma_describe(struct drm_framebuffer *fb,
struct seq_file *m)
seq_printf(m, "fb: %dx%d@%4.4s\n", fb->width, fb->height,
(char *)&fb->format->format);
- for (i = 0; i < fb->fomat->num_planes; i++) {
+ for (i = 0; i < fb->format->num_planes; i++) {
seq_printf(m, " %d: offset=%d pitch=%d, obj: ",
i, fb->offsets[i], fb->pitches[i]);
drm_gem_cma_describe(fb_cma->obj[i], m);
--
2.10.2
[Intel-gfx] [PATCH 1/4] drm/atomic: Delete wrong comment.
On Thu, Dec 15, 2016 at 03:29:42PM +0100, Maarten Lankhorst wrote:
> drm_atomic_state_put is called unconditionally, so TEST_ONLY is no
> different from commit.
>
> Signed-off-by: Maarten Lankhorst
I think it'd be good to update the kerneldoc for the atomic_commit
callback to mention that drivers should grab their own references using
drm_atomic_state_get() when they need it.
Applied this one meanwhile, thanks.
-Daniel
> ---
> drivers/gpu/drm/drm_atomic.c | 4
> 1 file changed, 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 60697482b94c..d1d252261bf1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -2195,10 +2195,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
> goto out;
>
> if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
> - /*
> - * Unlike commit, check_only does not clean up state.
> - * Below we call drm_atomic_state_put for it.
> - */
> ret = drm_atomic_check_only(state);
> } else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
> ret = drm_atomic_nonblocking_commit(state);
> --
> 2.7.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
[PATCH] drivers/gpu/drm/ast: Fix infinite loop if read fails
ast_get_dram_info() configures a window in order to access BMC memory.
A BMC register can be configured to disallow this, and if so, causes
an infinite loop in the ast driver which renders the system unusable.
Fix this by erroring out if an error is detected. On powerpc systems with
EEH, this leads to the device being fenced and the system continuing to
operate.
Cc: # 3.10+
Signed-off-by: Russell Currey
---
drivers/gpu/drm/ast/ast_main.c | 7 +--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 904beaa..f75c642 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -223,7 +223,8 @@ static int ast_get_dram_info(struct drm_device *dev)
ast_write32(ast, 0x1, 0xfc600309);
do {
- ;
+ if (pci_channel_offline(dev->pdev))
+ return -EIO;
} while (ast_read32(ast, 0x1) != 0x01);
data = ast_read32(ast, 0x10004);
@@ -428,7 +429,9 @@ int ast_driver_load(struct drm_device *dev, unsigned long
flags)
ast_detect_chip(dev, &need_post);
if (ast->chip != AST1180) {
- ast_get_dram_info(dev);
+ ret = ast_get_dram_info(dev);
+ if (ret)
+ goto out_free;
ast->vram_size = ast_get_vram_info(dev);
DRM_INFO("dram %d %d %d %08x\n", ast->mclk, ast->dram_type,
ast->dram_bus_width, ast->vram_size);
}
--
2.10.2
[PATCH] MAINTAINERS: add myself as maintainer of fbdev
I would like to help with fbdev maintenance. I can dedicate some time for reviewing and handling patches but won't have time for much more. The subsystem will remain in maintenance mode (no new drivers will be added to it). Cc: Tomi Valkeinen Cc: Daniel Vetter Signed-off-by: Bartlomiej Zolnierkiewicz --- MAINTAINERS |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) Index: b/MAINTAINERS === --- a/MAINTAINERS 2016-12-15 15:40:31.441918231 +0100 +++ b/MAINTAINERS 2016-12-15 15:40:50.569918713 +0100 @@ -5077,9 +5077,11 @@ F: drivers/net/wan/dlci.c F: drivers/net/wan/sdla.c FRAMEBUFFER LAYER +M: Bartlomiej Zolnierkiewicz L: linux-fbdev at vger.kernel.org +T: git git://github.com/bzolnier/linux.git Q: http://patchwork.kernel.org/project/linux-fbdev/list/ -S: Orphan +S: Maintained F: Documentation/fb/ F: drivers/video/ F: include/video/
[Intel-gfx] [PATCH v2 00/37] drm: Deduplicate fb format information (v2)
On Wed, Dec 14, 2016 at 11:37:46PM +0200, Ville Syrjälä wrote:
> On Fri, Nov 18, 2016 at 09:52:36PM +0200, ville.syrjala at linux.intel.com
> wrote:
> > From: Ville Syrjälä
> >
> > Second installment of my effort to remove the duplicated
> > depth/bpp/pixel_format from drm_framebuffer and just use
> > struct drm_format_info instead.
> >
> > I tried to address all of the review feedback, and collect
> > up all the r-bs I already got. Thanks for the review, guys.
> >
> > Changes since the last version are roughly:
> > * drm_framebuffer_init() now fails if the fb isn't properly prepared
> > * Applied mode cocciry all over to use fb->format more extensively
> > * Dropped a few i915 specific patches that were taken care of the
> > previous item
> > * Took up Laurent's idea that we can just compare the fb->format
> > pointers instead of comparing the fb->format->format values
> > * Added a new .get_format_info() hooks for drivers to provide custom
> > format information + an quick example patch how we'd hook it up
> > for i915 render compression support
> >
> > Link to the previous version:
> > https://lists.freedesktop.org/archives/dri-devel/2016-November/124135.html
> >
> > Entire series is available here:
> > git://github.com/vsyrjala/linux.git fb_format_dedup_2
> >
> > Cc: Alex Deucher
> > Cc: Alexey Brodkin
> > Cc: Ben Skeggs
> > Cc: Ben Widawsky
> > Cc: Brian Starkey
> > Cc: "Christian König"
> > Cc: Dave Airlie
> > Cc: Gerd Hoffmann
> > Cc: intel-gfx at lists.freedesktop.org
> > Cc: Laurent Pinchart
> > Cc: linux-graphics-maintainer at vmware.com
> > Cc: Liviu Dudau
> > Cc: Mali DP Maintainers
> > Cc: Patrik Jakobsson
> > Cc: Paulo Zanoni
> > Cc: Sinclair Yeh
> > Cc: Thomas Hellstrom
> >
> > Ville Syrjälä (37):
> > drm/i915: Add local 'fb' variables
> > drm/radeon: Add local 'fb' variables
> > drm/radeon: Use DIV_ROUND_UP()
> > drm/mgag200: Add local 'fb' variable
> > drm/ast: Add local 'fb' variables
> > drm/gma500: Add some local 'fb' variables
> > drm/cirrus: Add some local 'fb' variables
> > drm/arcpgu: Add local 'fb' variables
> > drm/arm: Add local 'fb' variables
> > drm/nouveau: Fix crtc->primary->fb vs. drm_fb fail
> > drm/nouveau: Add local 'fb' variables
>
> I've pushed up to here to drm-misc-next. Thanks for the reviews.
>
> I re-ran spatch to regenerate some of the later patches as there had
> been a bit of churn in the code. I've reposted the changed patches,
> and if no one screams I'll be pushing the rest tomorrowish.
>
> > drm/vmwgfx: Populate fb->dev before drm_framebuffer_init()
> > drm: Pass 'dev' to drm_helper_mode_fill_fb_struct()
> > drm/vmwgfx: Populate fb->pixel_format
> > drm/qxl: Call drm_helper_mode_fill_fb_struct() before
> > drm_framebuffer_init()
> > drm/virtio: Call drm_helper_mode_fill_fb_struct() before
> > drm_framebuffer_init()
> > drm/i915: Set fb->dev early on for inherited fbs
> > drm: Populate fb->dev from drm_helper_mode_fill_fb_struct()
> > drm: Store a pointer to drm_format_info under drm_framebuffer
> > drm/vmwgfx: Populate fb->format correctly
> > drm/i915: Populate fb->format early for inherited fbs
> > drm: Reject fbs w/o format info in drm_framebuffer_init()
> > drm: Replace drm_format_num_planes() with fb->format->num_planes
> > drm/i915: Eliminate the ugly 'fb?:' constructs from the ilk/skl wm
> > code
> > drm: Replace drm_format_plane_cpp() with fb->format->cpp[]
> > drm/fb_cma_helper: Replace drm_format_info() with fb->format
> > drm/nouveau: Use fb->format rather than drm_format_info()
> > drm/i915: Store a pointer to the pixel format info for fbc
> > drm: Add drm_framebuffer_plane_{width,height}()
> > drm/i915: Use drm_framebuffer_plane_{width,height}() where possible
> > drm: Nuke fb->depth
> > drm: Nuke fb->bits_per_pixel
> > drm: Nuke fb->pixel_format
> > drm: Replace 'format->format' comparisons to just 'format' comparisons
> > drm: Eliminate the useless "non-RGB fb" debug message
And I've just pushed up to here (minus the vmvgfx patches which dropped
out due to Daniel's earlier refactorin).
> > drm: Add mode_config .get_format_info() hook
> > drm/i915: Implement .get_format_info() hook for CCS
I'll hang on to these until we get the i915 CCS thing into shape.
Thanks for the reviews everyone.
> >
> > drivers/gpu/drm/amd/amdgpu/amdgpu_display.c | 2 +-
> > drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 4 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 6 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 6 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 6 +-
> > drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 6 +-
> > drivers/gpu/drm/arc/arcpgu_crtc.c | 3 +-
> > drivers/gpu/drm/arm/hdlcd_crtc.c| 18 ++--
> > drivers/gpu/drm/arm/malidp_planes.c | 10 +--
> > drivers/gpu/drm/armada/armada_crtc.c| 6 +-
> > driver
[PATCH 4/4] drm: Resurrect atomic rmfb code, v2
From: Daniel Vetter
This was somehow lost between v3 and the merged version in Maarten's
patch merged as:
commit f2d580b9a8149735cbc4b59c4a8df60173658140
Author: Maarten Lankhorst
Date: Wed May 4 14:38:26 2016 +0200
drm/core: Do not preserve framebuffer on rmfb, v4.
Actual code copied from Maarten's patch, but with the slight change to
just use dev->mode_config.funcs->atomic_commit to decide whether to
use the atomic path or not.
v2:
- Remove plane->fb assignment, done by drm_atomic_clean_old_fb.
- Add WARN_ON when atomic_remove_fb fails.
- Always call drm_atomic_state_put.
Signed-off-by: Daniel Vetter
Signed-off-by: Daniel Vetter
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/drm_atomic.c| 64 +
drivers/gpu/drm/drm_crtc_internal.h | 1 +
drivers/gpu/drm/drm_framebuffer.c | 7
3 files changed, 72 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index d1d252261bf1..23a3845542e1 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2059,6 +2059,70 @@ static void complete_crtc_signaling(struct drm_device
*dev,
kfree(fence_state);
}
+int drm_atomic_remove_fb(struct drm_framebuffer *fb)
+{
+ struct drm_modeset_acquire_ctx ctx;
+ struct drm_device *dev = fb->dev;
+ struct drm_atomic_state *state;
+ struct drm_plane *plane;
+ int ret = 0;
+ unsigned plane_mask;
+
+ state = drm_atomic_state_alloc(dev);
+ if (!state)
+ return -ENOMEM;
+
+ drm_modeset_acquire_init(&ctx, 0);
+ state->acquire_ctx = &ctx;
+
+retry:
+ plane_mask = 0;
+ ret = drm_modeset_lock_all_ctx(dev, &ctx);
+ if (ret)
+ goto unlock;
+
+ drm_for_each_plane(plane, dev) {
+ struct drm_plane_state *plane_state;
+
+ if (plane->state->fb != fb)
+ continue;
+
+ plane_state = drm_atomic_get_plane_state(state, plane);
+ if (IS_ERR(plane_state)) {
+ ret = PTR_ERR(plane_state);
+ goto unlock;
+ }
+
+ drm_atomic_set_fb_for_plane(plane_state, NULL);
+ ret = drm_atomic_set_crtc_for_plane(plane_state, NULL);
+ if (ret)
+ goto unlock;
+
+ plane_mask |= BIT(drm_plane_index(plane));
+
+ plane->old_fb = plane->fb;
+ }
+
+ if (plane_mask)
+ ret = drm_atomic_commit(state);
+
+unlock:
+ if (plane_mask)
+ drm_atomic_clean_old_fb(dev, plane_mask, ret);
+
+ if (ret == -EDEADLK) {
+ drm_modeset_backoff(&ctx);
+ goto retry;
+ }
+
+ drm_atomic_state_put(state);
+
+ drm_modeset_drop_locks(&ctx);
+ drm_modeset_acquire_fini(&ctx);
+
+ return ret;
+}
+
int drm_mode_atomic_ioctl(struct drm_device *dev,
void *data, struct drm_file *file_priv)
{
diff --git a/drivers/gpu/drm/drm_crtc_internal.h
b/drivers/gpu/drm/drm_crtc_internal.h
index cdf6860c9d22..121e250853d2 100644
--- a/drivers/gpu/drm/drm_crtc_internal.h
+++ b/drivers/gpu/drm/drm_crtc_internal.h
@@ -178,6 +178,7 @@ int drm_atomic_get_property(struct drm_mode_object *obj,
struct drm_property *property, uint64_t *val);
int drm_mode_atomic_ioctl(struct drm_device *dev,
void *data, struct drm_file *file_priv);
+int drm_atomic_remove_fb(struct drm_framebuffer *fb);
/* drm_plane.c */
diff --git a/drivers/gpu/drm/drm_framebuffer.c
b/drivers/gpu/drm/drm_framebuffer.c
index cbf0c893f426..c358bf8280a8 100644
--- a/drivers/gpu/drm/drm_framebuffer.c
+++ b/drivers/gpu/drm/drm_framebuffer.c
@@ -770,6 +770,12 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
* in this manner.
*/
if (drm_framebuffer_read_refcount(fb) > 1) {
+ if (dev->mode_config.funcs->atomic_commit) {
+ int ret = drm_atomic_remove_fb(fb);
+ WARN(ret, "atomic remove_fb failed with %i\n", ret);
+ goto out;
+ }
+
drm_modeset_lock_all(dev);
/* remove from any CRTC */
drm_for_each_crtc(crtc, dev) {
@@ -787,6 +793,7 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
drm_modeset_unlock_all(dev);
}
+out:
drm_framebuffer_unreference(fb);
}
EXPORT_SYMBOL(drm_framebuffer_remove);
--
2.7.4
[PATCH 3/4] drm/i915: Disable all crtcs during driver unload.
We may keep the crtc's enabled when userspace unsets all framebuffers but keeps the crtc active. This exposes a WARN in fbc_global disable, and a lot of bugs in our hardware readout code. Solve this by disabling all crtc's for now. Signed-off-by: Maarten Lankhorst --- drivers/gpu/drm/i915/i915_drv.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 6428588518aa..bb0d7517b678 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -43,6 +43,7 @@ #include #include +#include #include #include "i915_drv.h" @@ -1282,6 +1283,10 @@ void i915_driver_unload(struct drm_device *dev) intel_display_power_get(dev_priv, POWER_DOMAIN_INIT); + drm_modeset_lock_all(dev); + drm_atomic_helper_disable_all(dev, dev->mode_config.acquire_ctx); + drm_modeset_unlock_all(dev); + i915_driver_unregister(dev_priv); drm_vblank_cleanup(dev); -- 2.7.4
[PATCH 2/4] drm/i915: Fix POWER_DOMAIN_AUDIO refcounting.
If the crtc was brought up with audio before the driver loads,
then crtc_disable will remove a refcount to audio that doesn't exist
before.
Fortunately we already set power domains on readout, so we can just add
the power domain handling to get_crtc_power_domains, which will update
the power domains correctly in all cases.
This was found when testing module reload on CI with the crtc enabled,
which resulted in the following warn after module reload + modeset:
[ 24.197041] [ cut here ]
[ 24.197075] WARNING: CPU: 0 PID: 99 at
drivers/gpu/drm/i915/intel_runtime_pm.c:1790
intel_display_power_put+0x134/0x140 [i915]
[ 24.197076] Use count on domain AUDIO is already zero
[ 24.197098] CPU: 0 PID: 99 Comm: kworker/u8:2 Not tainted
4.9.0-CI-Trybot_393+ #1
[ 24.197099] Hardware name: /NUC6i5SYB, BIOS
SYSKLi35.86A.0042.2016.0409.1246 04/09/2016
[ 24.197102] Workqueue: events_unbound async_run_entry_fn
[ 24.197105] c93c7688 81435b35 c93c76d8
[ 24.197107] c93c76c8 8107e4d6 06fe5dc36f28
88025dc30054
[ 24.197109] 88025dc36f28 88025dc3 88025dc3
0015
[ 24.197110] Call Trace:
[ 24.197113] [] dump_stack+0x67/0x92
[ 24.197116] [] __warn+0xc6/0xe0
[ 24.197118] [] warn_slowpath_fmt+0x4a/0x50
[ 24.197149] [] intel_display_power_put+0x134/0x140 [i915]
[ 24.197187] [] intel_disable_ddi+0x4d/0x80 [i915]
[ 24.197223] [] intel_encoders_disable.isra.74+0x7f/0x90
[i915]
[ 24.197257] [] haswell_crtc_disable+0x55/0x170 [i915]
[ 24.197292] [] intel_atomic_commit_tail+0x108/0xfd0 [i915]
[ 24.197295] [] ? __lock_is_held+0x66/0x90
[ 24.197330] [] intel_atomic_commit+0x429/0x560 [i915]
[ 24.197332] []
?drm_atomic_add_affected_connectors+0x56/0xf0
[ 24.197334] [] drm_atomic_commit+0x46/0x50
[ 24.197336] [] restore_fbdev_mode+0x147/0x270
[ 24.197337] []
drm_fb_helper_restore_fbdev_mode_unlocked+0x2e/0x70
[ 24.197339] [] drm_fb_helper_set_par+0x28/0x50
[ 24.197374] [] intel_fbdev_set_par+0x13/0x70 [i915]
[ 24.197376] [] fbcon_init+0x57a/0x600
[ 24.197379] [] visual_init+0xd1/0x130
[ 24.197381] [] do_bind_con_driver+0x1bc/0x3a0
[ 24.197384] [] do_take_over_console+0x111/0x180
[ 24.197386] [] do_fbcon_takeover+0x52/0xb0
[ 24.197387] [] fbcon_event_notify+0x723/0x850
[ 24.197390] [] ?__blocking_notifier_call_chain+0x30/0x70
[ 24.197392] [] notifier_call_chain+0x34/0xa0
[ 24.197394] [] __blocking_notifier_call_chain+0x48/0x70
[ 24.197397] [] blocking_notifier_call_chain+0x11/0x20
[ 24.197398] [] fb_notifier_call_chain+0x16/0x20
[ 24.197400] [] register_framebuffer+0x24c/0x330
[ 24.197402] [] drm_fb_helper_initial_config+0x219/0x3c0
[ 24.197436] [] intel_fbdev_initial_config+0x13/0x30 [i915]
[ 24.197438] [] async_run_entry_fn+0x34/0x140
[ 24.197440] [] process_one_work+0x1ec/0x6b0
[ 24.197442] [] ? process_one_work+0x166/0x6b0
[ 24.197445] [] worker_thread+0x49/0x490
[ 24.197447] [] ? process_one_work+0x6b0/0x6b0
[ 24.197448] [] kthread+0xeb/0x110
[ 24.197451] [] ? kthread_park+0x60/0x60
[ 24.197453] [] ret_from_fork+0x27/0x40
[ 24.197476] ---[ end trace bda64b683b8e8162 ]---
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/i915/intel_ddi.c | 14 ++
drivers/gpu/drm/i915/intel_display.c | 4
drivers/gpu/drm/i915/intel_dp_mst.c | 9 ++---
3 files changed, 8 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index d808a2ccc29e..8c9ce850760b 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -1835,8 +1835,6 @@ static void intel_enable_ddi(struct intel_encoder
*intel_encoder,
struct drm_connector_state *conn_state)
{
struct drm_encoder *encoder = &intel_encoder->base;
- struct drm_crtc *crtc = encoder->crtc;
- struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
struct drm_i915_private *dev_priv = to_i915(encoder->dev);
enum port port = intel_ddi_get_encoder_port(intel_encoder);
int type = intel_encoder->type;
@@ -1863,10 +1861,8 @@ static void intel_enable_ddi(struct intel_encoder
*intel_encoder,
intel_edp_drrs_enable(intel_dp, pipe_config);
}
- if (intel_crtc->config->has_audio) {
- intel_display_power_get(dev_priv, POWER_DOMAIN_AUDIO);
+ if (pipe_config->has_audio)
intel_audio_codec_enable(intel_encoder, pipe_config,
conn_state);
- }
}
static void intel_disable_ddi(struct intel_encoder *intel_encoder,
@@ -1874,16 +1870,10 @@ static void intel_disable_ddi(struct intel_encoder
*intel_encoder,
struct drm_connector_state *old_conn_state)
{
struct drm_encoder *encoder = &intel_encoder->base;
- struct drm_crtc *crtc = encoder->crtc;
- struct
[PATCH 1/4] drm/atomic: Delete wrong comment.
drm_atomic_state_put is called unconditionally, so TEST_ONLY is no
different from commit.
Signed-off-by: Maarten Lankhorst
---
drivers/gpu/drm/drm_atomic.c | 4
1 file changed, 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 60697482b94c..d1d252261bf1 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -2195,10 +2195,6 @@ int drm_mode_atomic_ioctl(struct drm_device *dev,
goto out;
if (arg->flags & DRM_MODE_ATOMIC_TEST_ONLY) {
- /*
-* Unlike commit, check_only does not clean up state.
-* Below we call drm_atomic_state_put for it.
-*/
ret = drm_atomic_check_only(state);
} else if (arg->flags & DRM_MODE_ATOMIC_NONBLOCK) {
ret = drm_atomic_nonblocking_commit(state);
--
2.7.4
[PATCH 0/4] drm: Resurrect atomic rmfb code.
There were some issues in i915 preventing rmfb from working correctly. A lot of them are related to hw readout, some others due to keeping crtc active during driver unload: https://intel-gfx-ci.01.org/CI/Trybot_394/ I fixed the POWER_DOMAIN_AUDIO issue because it was easy to do so, but ignored the other failures by disabling all crtc's during driver unload. Daniel Vetter (1): drm: Resurrect atomic rmfb code, v2 Maarten Lankhorst (3): drm/atomic: Delete wrong comment. drm/i915: Fix POWER_DOMAIN_AUDIO refcounting. drm/i915: Disable all crtcs during driver unload. drivers/gpu/drm/drm_atomic.c | 68 +--- drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_framebuffer.c| 7 drivers/gpu/drm/i915/i915_drv.c | 5 +++ drivers/gpu/drm/i915/intel_ddi.c | 14 ++-- drivers/gpu/drm/i915/intel_display.c | 4 +++ drivers/gpu/drm/i915/intel_dp_mst.c | 9 ++--- 7 files changed, 85 insertions(+), 23 deletions(-) -- 2.7.4
[PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD handles (v3)
Am 15.12.2016 um 13:33 schrieb Nath, Arindam:
>> -Original Message-
>> From: Grazvydas Ignotas [mailto:notasas at gmail.com]
>> Sent: Thursday, December 15, 2016 5:52 PM
>> To: Nath, Arindam
>> Cc: Emil Velikov; David Airlie; Deucher, Alexander; ML dri-devel; amd-gfx
>> mailing list; Koenig, Christian
>> Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
>> handles (v3)
>>
>> On Thu, Dec 15, 2016 at 1:47 PM, Nath, Arindam
>> wrote:
-Original Message-
From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
Sent: Thursday, December 15, 2016 5:01 PM
To: Nath, Arindam
Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
Koenig, Christian
Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
handles (v3)
On 15 December 2016 at 07:30, Nath, Arindam
wrote:
>> -Original Message-
>> From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>> Sent: Wednesday, December 14, 2016 9:26 PM
>> To: Nath, Arindam
>> Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
>> Koenig, Christian
>> Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used
>> UVD
>> handles (v3)
>>
>> On 12 December 2016 at 18:49, wrote:
>>> From: Arindam Nath
>>>
>>> Change History
>>> --
>>>
>>> v3: changes suggested by Christian
>>> - Add a check for UVD IP block using AMDGPU_HW_IP_UVD
>>>query type.
>>> - Add a check for asic_type to be less than
>>>CHIP_POLARIS10 since starting Polaris, we support
>>>unlimited UVD instances.
>>> - Add kerneldoc style comment for
>>>amdgpu_uvd_used_handles().
>>>
>>> v2: as suggested by Christian
>>> - Add a new query AMDGPU_INFO_NUM_HANDLES
>>> - Create a helper function to return the number
>>>of currently used UVD handles.
>>> - Modify the logic to count the number of used
>>>UVD handles since handles can be freed in
>>>non-linear fashion.
>>>
>>> v1:
>>> - User might want to query the maximum number of UVD
>>>instances supported by firmware. In addition to that,
>>>if there are multiple applications using UVD handles
>>>at the same time, he might also want to query the
>>>currently used number of handles.
>>>
>>>For this we add two variables max_handles and
>>>used_handles inside drm_amdgpu_info_hw_ip. So now
>>>an application (or libdrm) can use AMDGPU_INFO IOCTL
>>>with AMDGPU_INFO_HW_IP_INFO query type to get these
>>>values.
>>>
>>> Signed-off-by: Arindam Nath
>>> Reviewed-by: Christian König
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 21
>> +
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 25
>> +
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.h | 1 +
>>> include/uapi/drm/amdgpu_drm.h | 9 +
>>> 4 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index 174eb59..3273d8c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -570,6 +570,27 @@ static int amdgpu_info_ioctl(struct drm_device
>> *dev, void *data, struct drm_file
>>> return -EINVAL;
>>> }
>>> }
>>> + case AMDGPU_INFO_NUM_HANDLES: {
>>> + struct drm_amdgpu_info_num_handles handle;
>>> +
>>> + switch (info->query_hw_ip.type) {
>>> + case AMDGPU_HW_IP_UVD:
>>> + /* Starting Polaris, we support unlimited UVD
>>> handles */
>>> + if (adev->asic_type < CHIP_POLARIS10) {
>>> + handle.uvd_max_handles =
>>> adev->uvd.max_handles;
>>> + handle.uvd_used_handles =
>> amdgpu_uvd_used_handles(adev);
>>> +
>>> + return copy_to_user(out, &handle,
>>> + min((size_t)size,
>>> sizeof(handle))) ? -EFAULT : 0;
>>> + } else {
>>> + return -EINVAL;
>> Using EINVAL doesn't seem right here. As per man 3 ioctl
>>
>> EINVAL The request or arg argument is not valid for this device.
>>
>> A bit further down you can see the one you want.
>>
>> ENODEV The fildes argument refers to a valid STREAMS device, but
>> the corresponding device driver does not support the ioctl() function.
> Emil, ENODEV would mean the driver does not support ioctl() itself. But in
our case ioctl() is supported.
> Since we extract the query
[PATCH] drm/bridge: analogix_dp: set the DPCD600 during disabling the psr
On 12/14/2016 03:30 PM, Sean Paul wrote:
> On Wed, Dec 14, 2016 at 12:03 AM, Archit Taneja
> wrote:
>> Hi,
>>
>> On 12/12/2016 08:28 PM, Sean Paul wrote:
>>>
>>> On Fri, Dec 9, 2016 at 9:49 PM, Caesar Wang wrote:
Look likes, the BOE panel FW didn't ack the DPCD600 signal from the host
device, that will cause the panel hang on the startup display.
The root cause we use the fast link mode during enter and exit the psr,
this issue is gone if switching the fast link to main link mode.
>>>
>>> Cc: Archit Taneja
>>
>>
>> Do we want this as a fix in 4.10? Or is it okay to get it in 4.11?
>> In other words, should this go to drm-misc-next or drm-misc-fixes?
>>
>
> Hi Archit,
> 4.11 is totally fine.
Thanks. Queued after some commit message cleanup.
Archit
>
> Sean
>
>
>> Thanks,
>> Archit
>>
>>
>>>
Signed-off-by: Caesar Wang
---
drivers/gpu/drm/bridge/analogix/analogix_dp_core.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 6e0447f..6a5347b 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -133,6 +133,7 @@ int analogix_dp_disable_psr(struct device *dev)
{
struct analogix_dp_device *dp = dev_get_drvdata(dev);
struct edp_vsc_psr psr_vsc;
+ int ret;
if (!dp->psr_support)
return -EINVAL;
@@ -147,6 +148,10 @@ int analogix_dp_disable_psr(struct device *dev)
psr_vsc.DB0 = 0;
psr_vsc.DB1 = 0;
+ ret = drm_dp_dpcd_writeb(&dp->aux, DP_SET_POWER,
DP_SET_POWER_D0);
+ if (ret != 1)
+ dev_err(dp->dev, "Failed to set DP Power0 %d\n", ret);
+
analogix_dp_send_psr_spd(dp, &psr_vsc);
return 0;
}
--
2.7.4
>>
>> --
>> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
>> a Linux Foundation Collaborative Project
--
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
Passing multi-screen layout to KMS driver
Hello Gerd, 14.12.2016 11:11, Gerd Hoffmann wrote: >> So I would be interested to know whether anyone else has thought about >> this problem, and possibly even about an interface to let the compositor >> pass the information. If not, would people be open to the idea? I >> would much rather have something generally agreed on than hack something up. > > I think the best way to tackle this is to have multiple tablets, one per > display device (touchscreen-style setup). > > qemu can do that, with input routing (must configure on the host which > display belongs to which tablet). Here is some info on that: > > http://git.qemu-project.org/?p=qemu.git;a=blob;f=docs/multiseat.txt;hb=HEAD > > The setup on the guest side is completely manual. You have to use > "xinput --map-to-output" to tell Xorg which tablet belongs to which > display. Maybe it is also possible to stick that into xorg.conf. > Should be improved, and it surely makes sense that qemu and virtualbox > use the same approach here. > > Not sure if and how this works automatically with physical touchscreens. > Any clues are welcome. Thanks for the answer. That was the direction I was initially expecting to go too. In theory libinput lets you map input devices to heads[1] using a udev property[2], though I have yet to test whether anyone supports that yet (couldn't find it in the Mutter/GNOME Shell source, but I'm not familiar with it). As the API description says, the default without that property is still to map the input device to all screens like X.Org on Linux does. [1] https://cgit.freedesktop.org/wayland/libinput/tree/src/udev-seat.c?id=1.5.3#n97 [2] https://wayland.freedesktop.org/libinput/doc/0.99.1/group__device.html#gaf48626f6190e9c9bc14abb704e66cc22 > Another option is to use a guest agent, spice does that since years to > handle multihead. The guest agent queries the display layout using > xrandr, gets x + y + displayid from the spice client and generates > pointer events from that. But I expect that scheme breaks with wayland > because wayland is by design alot more restrictive, so spice-agent > probably isn't allowed to send pointer events. So not really an option > these days ... We actually do something similar in Windows guests: older versions provided the layout information to the driver directly, but at least Windows 10 does not, so we query it with a user-space agent which passes it to the driver. (We send our pointer events from a driver, in all supported guest types, which works fine with Wayland too.) It would be nice though as I said if the compositor (or whoever is controlling the display) could just provide the layout information to the driver itself. We already have "suggested X" and "suggested Y" for the other direction, and for now I have solved it by always providing "suggested X" and "suggested Y" hints in the driver. That which works well enough in a first approximation - if the user changes the layout inside the virtual machine the mapping breaks, and as soon as they change it outside it mends again. So my idea was to try to have people agree on on interface for that. Regards Michael > cheers, > Gerd -- Michael Thayer | VirtualBox engineer ORACLE Deutschland B.V. & Co. KG | Werkstr. 24 | D-71384 Weinstadt ORACLE Deutschland B.V. & Co. KG Hauptverwaltung: RiesstraÃe 25, D-80992 München Registergericht: Amtsgericht München, HRA 95603 Komplementärin: ORACLE Deutschland Verwaltung B.V. Hertogswetering 163/167, 3543 AS Utrecht, Niederlande Handelsregister der Handelskammer Midden-Nederland, Nr. 30143697 Geschäftsführer: Alexander van der Ven, Jan Schultheiss, Val Maher
[PATCH v4 21/22] drm: omapdrm: Use sizeof(*var) instead of sizeof(type) for structures
On 14/12/16 02:27, Laurent Pinchart wrote: > By linking the sizeof to a variable type the code will be less prone to > bugs due to future type changes of variables. > > Signed-off-by: Laurent Pinchart > Reviewed-by: Daniel Vetter > --- > drivers/gpu/drm/omapdrm/displays/panel-dsi-cm.c | 2 +- > drivers/gpu/drm/omapdrm/dss/omapdss-boot-init.c | 3 +-- > drivers/gpu/drm/omapdrm/omap_connector.c| 4 ++-- > drivers/gpu/drm/omapdrm/omap_dmm_tiler.c| 4 ++-- > drivers/gpu/drm/omapdrm/omap_encoder.c | 2 +- > 5 files changed, 7 insertions(+), 8 deletions(-) Reviewed-by: Tomi Valkeinen Tomi -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/fe3ab8b5/attachment.sig>
[PATCH v4 17/22] drm: omapdrm: Don't call DISPC power handling in IRQ wait functions
On 14/12/16 02:27, Laurent Pinchart wrote: > The IRQ wait functions are called from the DSS enable and disable > operations only, where the DISPC is guaranteed to be enabled. There's no > need for manual DISPC power management there. > > Signed-off-by: Laurent Pinchart > --- > drivers/gpu/drm/omapdrm/omap_irq.c | 4 > 1 file changed, 4 deletions(-) Reviewed-by: Tomi Valkeinen Tomi -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/6c1985fc/attachment.sig>
[PATCH v4 16/22] drm: omapdrm: Remove unused parameter from omap_drm_irq handler
On 14/12/16 02:27, Laurent Pinchart wrote: > The only omap_drm_irq handler doesn't use the irqstatus parameter passed > to the function. Remove it. > > Signed-off-by: Laurent Pinchart > --- > Changes since v1: > > - New patch > --- > drivers/gpu/drm/omapdrm/omap_drv.h | 2 +- > drivers/gpu/drm/omapdrm/omap_irq.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) Reviewed-by: Tomi Valkeinen Tomi -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/24bc7262/attachment-0001.sig>
[PATCH v4 15/22] drm: omapdrm: Don't expose the omap_irq_(un)register() functions
On 14/12/16 02:27, Laurent Pinchart wrote: > The IRQ registration functions are not used outside of their compilation > unit, make them static. As the __omap_irq_(un)register() functions are > only called by their omap_irq_(un)register() counterparts, merge them > together. > > Signed-off-by: Laurent Pinchart > --- > Changes since v1: > > - Split the omap_drm_irq irqstatus parameter removal change out > --- > drivers/gpu/drm/omapdrm/omap_drv.h | 4 > drivers/gpu/drm/omapdrm/omap_irq.c | 23 +-- > 2 files changed, 5 insertions(+), 22 deletions(-) Reviewed-by: Tomi Valkeinen Tomi -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/54eba3d3/attachment.sig>
[PATCH v4 14/22] drm: omapdrm: Keep vblank interrupt enabled while CRTC is active
On 14/12/16 02:27, Laurent Pinchart wrote:
> Instead of going through a complicated private IRQ registration
> mechanism, handle the vblank interrupt activation with the standard
> drm_crtc_vblank_get() and drm_crtc_vblank_put() mechanism. This will let
> the DRM core keep the vblank interrupt enabled as long as needed to
> update the frame counter.
>
> Signed-off-by: Laurent Pinchart
> ---
> drivers/gpu/drm/omapdrm/omap_crtc.c | 38
> ++---
> drivers/gpu/drm/omapdrm/omap_drv.h | 1 +
> drivers/gpu/drm/omapdrm/omap_irq.c | 4 +++-
> 3 files changed, 18 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c
> b/drivers/gpu/drm/omapdrm/omap_crtc.c
> index 827ac46a6d5e..1f5372042706 100644
> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c
> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c
> @@ -36,8 +36,6 @@ struct omap_crtc {
>
> struct videomode vm;
>
> - struct omap_drm_irq vblank_irq;
> -
> bool ignore_digit_sync_lost;
>
> bool enabled;
> @@ -304,25 +302,24 @@ void omap_crtc_error_irq(struct drm_crtc *crtc,
> uint32_t irqstatus)
> DRM_ERROR_RATELIMITED("%s: errors: %08x\n", omap_crtc->name, irqstatus);
> }
>
> -static void omap_crtc_vblank_irq(struct omap_drm_irq *irq, uint32_t
> irqstatus)
> +void omap_crtc_vblank_irq(struct drm_crtc *crtc)
> {
> - struct omap_crtc *omap_crtc =
> - container_of(irq, struct omap_crtc, vblank_irq);
> - struct drm_device *dev = omap_crtc->base.dev;
> - struct drm_crtc *crtc = &omap_crtc->base;
> + struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
> + bool pending;
>
> if (dispc_mgr_go_busy(omap_crtc->channel))
> return;
>
> DBG("%s: apply done", omap_crtc->name);
>
> - __omap_irq_unregister(dev, &omap_crtc->vblank_irq);
> -
> spin_lock(&crtc->dev->event_lock);
> - WARN_ON(!omap_crtc->pending);
> + pending = omap_crtc->pending;
> omap_crtc->pending = false;
> spin_unlock(&crtc->dev->event_lock);
>
> + if (pending)
> + drm_crtc_vblank_put(crtc);
> +
I think there's a race.
- irq: we get vblank irq
- irq: GO is not set, so dispc_mgr_go_busy() check doesn't trigger
- flush: we set GO
- flush: we lock event_lock and set the pending & event, and unlock
- irq: irq handler continues, sees pending and event, and thinks that
the config has been taken into use, but in reality GO bit is set and
it'll happen only on next vblank.
> /* wake up userspace */
> omap_crtc_complete_page_flip(&omap_crtc->base);
>
> @@ -340,8 +337,6 @@ static void omap_crtc_destroy(struct drm_crtc *crtc)
>
> DBG("%s", omap_crtc->name);
>
> - WARN_ON(omap_crtc->vblank_irq.registered);
> -
> drm_crtc_cleanup(crtc);
>
> kfree(omap_crtc);
> @@ -353,14 +348,13 @@ static void omap_crtc_enable(struct drm_crtc *crtc)
>
> DBG("%s", omap_crtc->name);
>
> + drm_crtc_vblank_on(crtc);
> + WARN_ON(drm_crtc_vblank_get(crtc) != 0);
> +
> spin_lock_irq(&crtc->dev->event_lock);
> WARN_ON(omap_crtc->pending);
> omap_crtc->pending = true;
> spin_unlock_irq(&crtc->dev->event_lock);
> -
> - omap_irq_register(crtc->dev, &omap_crtc->vblank_irq);
> -
> - drm_crtc_vblank_on(crtc);
> }
>
> static void omap_crtc_disable(struct drm_crtc *crtc)
> @@ -414,8 +408,6 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
> {
> struct omap_crtc *omap_crtc = to_omap_crtc(crtc);
>
> - WARN_ON(omap_crtc->vblank_irq.registered);
> -
> if (crtc->state->color_mgmt_changed) {
> struct drm_color_lut *lut = NULL;
> uint length = 0;
> @@ -441,6 +433,10 @@ static void omap_crtc_atomic_flush(struct drm_crtc *crtc,
>
> DBG("%s: GO", omap_crtc->name);
>
> + dispc_mgr_go(omap_crtc->channel);
> +
> + WARN_ON(drm_crtc_vblank_get(crtc) != 0);
I don't like this style very much. I think WARN()s should be without
side effects.
Also, WARN only gives benefit when we don't know what the call stack is.
Afaik, there's only one way omap_crtc_atomic_flush can be called, so
it's just extra spam and dev_err or dev_warn should be enough.
Tomi
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/059ad801/attachment.sig>
[PATCH 20/34] drm/i915: Build DRM range manager selftests for CI
On ma, 2016-12-12 at 11:53 +, Chris Wilson wrote: > Build the struct drm_mm selftests so that we can trivially run them > within our CI. > > Signed-off-by: Chris Wilson "Enable debug, become developer." Reviewed-by: Joonas Lahtinen Regards, Joonas -- Joonas Lahtinen Open Source Technology Center Intel Corporation
[PATCH] drm: Limit individual events to 1 KiB
On Thu, 15 Dec 2016, Thierry Reding wrote:
> From: Thierry Reding
>
> Ever since support for vblank handling and event processing was added to
> libdrm (v2.4.16), events were read in 1 KiB chunks in drmHandleEvent().
> In addition, the implementation of drm_read() will not unqueue events if
> they can't be successfully passed to userspace. The result is that it's
> possible for a driver to send applications into a busy loop by creating
> events larger than 1 KiB.
>
> There is a limit to the event space that each DRM device can carry, but
> it is set to 4 KiB.
>
> While no driver currently produces events larger than 1 KiB, and it's
> fairly unlikely that anyone ever will, it's best to err on the side of
> caution and limit the size of individual events to 1 KiB. This ensures
> that libdrm will never run into the busy loop. In order to allow for
> the DRM device to queue multiple events, the event space limit is kept
> at 4 KiB.
>
> A complementary patch will be applied to libdrm to bump the read size
> for events to 4 KiB to ensure that newer versions of libdrm running on
> kernels without this fix will not run into the busy loop either.
>
> Signed-off-by: Thierry Reding
> ---
> drivers/gpu/drm/drm_fops.c | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 48e106557c92..3792e4434319 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -582,6 +582,15 @@ EXPORT_SYMBOL(drm_poll);
> * This is the locked version of drm_event_reserve_init() for callers which
> * already hold dev->event_lock.
> *
> + * Note that libdrm has been reading events in chunks of 1 KiB since forever
> + * (see drmHandleEvent()) and up to and including v2.4.74. Unfortunately the
Maybe it's a case of post-lunch catatonia, but I actually read that a
few times, and thought it made no sense, until I realized the version
number refers to libdrm, not kernel... Would it be too verbose to use
"libdrm v2.4.74" explicitly here and below?
BR,
Jani.
> + * drm_read() implementation won't return an error if an event doesn't fit
> + * into that 1 KiB and instead these events will simply be queued again. If
> + * a driver was to send out an event larger than 1 KiB, an application that
> + * polls the DRM file descriptor and calling drmHandleEvent() to process the
> + * events would go into a busy loop. This is caused by drm_poll() always
> + * return immediately, but drm_read() never being able to dequeue the event.
> + *
> * RETURNS:
> *
> * 0 on success or a negative error code on failure.
> @@ -591,6 +600,15 @@ int drm_event_reserve_init_locked(struct drm_device *dev,
> struct drm_pending_event *p,
> struct drm_event *e)
> {
> + /*
> + * Limit events to 1 KiB, which is the read size of libdrm (up to
> + * to and including v2.4.74) when processing events, in order to
> + * prevent any of the applications using libdrm from potentially
> + * going into a busy loop.
> + */
> + if (e->length > 1024)
> + return -EINVAL;
> +
> if (file_priv->event_space < e->length)
> return -ENOMEM;
--
Jani Nikula, Intel Open Source Technology Center
[PATCH 33/34] drm: Fix drm_mm search and insertion
On ma, 2016-12-12 at 11:53 +, Chris Wilson wrote:
> The drm_mm range manager claimed to support top-down insertion, but it
> was neither searching for the top-most hole that could fit the
> allocation request nor fitting the request to the hole correctly.
>
> In order to search the range efficiently, we create a secondary index
> for the holes using either their size or their address. This index
> allows us to find the smallest hole or the hole at the bottom or top of
> the range efficiently, whilst keeping the hole stack to rapidly service
> evictions.
>
> Signed-off-by: Chris Wilson
> +static void rm_hole(struct drm_mm_node *node)
> +{
> + if (!node->hole_size)
> + return;
I've actively tried to remove conditions that cause asymmetry between
add_/rm_, create_/destroy_ etc. So I think this should be
DRM_MM_BUG_ON() too.
> +static struct drm_mm_node *best_hole(struct drm_mm *mm, u64 size)
> Â {
> - struct drm_mm *mm = hole_node->mm;
> - u64 hole_start = drm_mm_hole_node_start(hole_node);
> - u64 hole_end = drm_mm_hole_node_end(hole_node);
> - u64 adj_start = hole_start;
> - u64 adj_end = hole_end;
> + struct rb_node *best = NULL;
> + struct rb_node **link = &mm->holes_size.rb_node;
> + while (*link) {
> + struct rb_node *rb = *link;
> + if (size <= rb_hole_size(rb))
> + link = &rb->rb_left, best = rb;
Single assignment per line, by coding style. And
link = &(best = rb)->rb_left is not better :P
> -int drm_mm_insert_node_in_range_generic(struct drm_mm *mm, struct
> drm_mm_node *node,
> +int drm_mm_insert_node_in_range_generic(struct drm_mm * const mm,
> + struct drm_mm_node * const node,
I really have no stance on the const's, I'll defer to higher powers on
this.
> +void drm_mm_remove_node(struct drm_mm_node *node)
> Â {
> - return best;
> + rm_hole(prev_node);
> + add_hole(prev_node);
update_hole?
Â
> @@ -799,7 +706,7 @@ bool drm_mm_scan_add_block(struct drm_mm_scan *scan,
> Â if (adj_end <= adj_start || adj_end - adj_start < scan->size)
> Â return false;
> Â
> - if (scan->flags == DRM_MM_CREATE_TOP)
> + if (scan->flags == DRM_MM_INSERT_HIGH)
Flags are usually checked with & if somebody wants to add them later.
Otherwise you could call it "mode".
Somebody else could give this a glance too.
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
[PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD handles (v3)
On Thu, Dec 15, 2016 at 1:47 PM, Nath, Arindam wrote:
>>-Original Message-
>>From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>>Sent: Thursday, December 15, 2016 5:01 PM
>>To: Nath, Arindam
>>Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
>>Koenig, Christian
>>Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
>>handles (v3)
>>
>>On 15 December 2016 at 07:30, Nath, Arindam
>>wrote:
-Original Message-
From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
Sent: Wednesday, December 14, 2016 9:26 PM
To: Nath, Arindam
Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
Koenig, Christian
Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
handles (v3)
On 12 December 2016 at 18:49, wrote:
> From: Arindam Nath
>
> Change History
> --
>
> v3: changes suggested by Christian
> - Add a check for UVD IP block using AMDGPU_HW_IP_UVD
> query type.
> - Add a check for asic_type to be less than
> CHIP_POLARIS10 since starting Polaris, we support
> unlimited UVD instances.
> - Add kerneldoc style comment for
> amdgpu_uvd_used_handles().
>
> v2: as suggested by Christian
> - Add a new query AMDGPU_INFO_NUM_HANDLES
> - Create a helper function to return the number
> of currently used UVD handles.
> - Modify the logic to count the number of used
> UVD handles since handles can be freed in
> non-linear fashion.
>
> v1:
> - User might want to query the maximum number of UVD
> instances supported by firmware. In addition to that,
> if there are multiple applications using UVD handles
> at the same time, he might also want to query the
> currently used number of handles.
>
> For this we add two variables max_handles and
> used_handles inside drm_amdgpu_info_hw_ip. So now
> an application (or libdrm) can use AMDGPU_INFO IOCTL
> with AMDGPU_INFO_HW_IP_INFO query type to get these
> values.
>
> Signed-off-by: Arindam Nath
> Reviewed-by: Christian König
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 21
+
> drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 25
+
> drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.h | 1 +
> include/uapi/drm/amdgpu_drm.h | 9 +
> 4 files changed, 56 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 174eb59..3273d8c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -570,6 +570,27 @@ static int amdgpu_info_ioctl(struct drm_device
*dev, void *data, struct drm_file
> return -EINVAL;
> }
> }
> + case AMDGPU_INFO_NUM_HANDLES: {
> + struct drm_amdgpu_info_num_handles handle;
> +
> + switch (info->query_hw_ip.type) {
> + case AMDGPU_HW_IP_UVD:
> + /* Starting Polaris, we support unlimited UVD
> handles */
> + if (adev->asic_type < CHIP_POLARIS10) {
> + handle.uvd_max_handles =
> adev->uvd.max_handles;
> + handle.uvd_used_handles =
amdgpu_uvd_used_handles(adev);
> +
> + return copy_to_user(out, &handle,
> + min((size_t)size,
> sizeof(handle))) ? -EFAULT : 0;
> + } else {
> + return -EINVAL;
Using EINVAL doesn't seem right here. As per man 3 ioctl
EINVAL The request or arg argument is not valid for this device.
A bit further down you can see the one you want.
ENODEV The fildes argument refers to a valid STREAMS device, but
the corresponding device driver does not support the ioctl() function.
>>>
>>> Emil, ENODEV would mean the driver does not support ioctl() itself. But in
>>our case ioctl() is supported.
>>>
>>> Since we extract the query type from arg passed to ioctl(), and it is this
>>query AMDGPU_INFO_NUM_HANDLES which is not supported by driver (for
>>> Polaris), doesnât returning EINVAL make more sense here?
>>>
>>Unless I'm reading the code incorrectly - CHIP_POLARIS10 and older do
>>not have support the query. Thus ENODEV is the one you want to use.
>>Furthermore EINVAL is (mostly) used to indicate incorrect input
>>(failed input validation) which userspace uses to check if kernel is
>>too old/does not support X.
>
> Actually, the code says only asics older than CHIP_POLARIS10 support the
> query, beyond it doesnât.
Wouldn't it be cleaner to return INT_MAX or something like tha
[PATCH v4 12/22] drm: omapdrm: Prevent processing the same event multiple times
On 14/12/16 02:27, Laurent Pinchart wrote: > The vblank interrupt is disabled after one occurrence, preventing the > atomic update event from being processed twice. However, this also > prevents the software frame counter from being updated correctly that > would require vblank interrupts to be kept enabled while the CRTC is > active. > > In preparation for vblank interrupt fixes, make sure that the atomic > update event will be processed once only when the vblank interrupt will > be kept enabled. > > Signed-off-by: Laurent Pinchart > --- > Changes since v3: > > - Don't release and reacquire the spinlock for just one return > - Remove unneeded (and unbalanced) drm_crtc_vblank_get() > - Store the event in the atomic flush handler to avoid race condition > - Use spin_lock_irq instead of spin_lock_irsave in flush handler > --- > drivers/gpu/drm/omapdrm/omap_crtc.c | 23 +++ > 1 file changed, 15 insertions(+), 8 deletions(-) Reviewed-by: Tomi Valkeinen Tomi -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/52a6e22e/attachment.sig>
BUG: 'list_empty(&vgdev->free_vbufs)' is true!
On 11/16/2016, 02:12 PM, Gerd Hoffmann wrote: > On Fr, 2016-11-11 at 17:28 +0100, Jiri Slaby wrote: >> On 11/09/2016, 09:01 AM, Gerd Hoffmann wrote: >>> On Di, 2016-11-08 at 22:37 +0200, Michael S. Tsirkin wrote: On Mon, Nov 07, 2016 at 09:43:24AM +0100, Jiri Slaby wrote: > Hi, > > I can relatively easily reproduce this bug: >>> >>> How? >> >> Run dmesg -w in the qemu window (virtio_gpu) to see a lot of output. >> Run pps [1] without exit(0); on e.g. serial console. >> Wait a bit. The lot of output causes the BUG. >> >> [1] https://github.com/jirislaby/collected_sources/blob/master/pps.c > > Doesn't reproduce here. > > Running "while true; do dmesg; done" on the virtio-gpu fbcon. > Running the pps fork bomb on the serial console. > > Can watch dmesg printing the kernel messages over and over, until the > shell can't spawn dmesg any more due to the fork bomb hitting the > process limit. No BUG() triggered. > > Tried spice, gtk and sdl. > > Hmm. > > Any ideas what else might be needed to reproduce it? I can reproduce even with count = 32 :(. And without the fork bomb (i.e. with the code from the repository). This is how I start qemu: /usr/bin/qemu-system-x86_64 -machine accel=kvm -k en-us -smp 4 -m 2371 -usb -device virtio-rng-pci -drive file=/home/new/suse-fact.img,format=raw,discard=unmap,if=none,id=hd -device virtio-scsi-pci,id=scsi -device scsi-hd,drive=hd -soundhw hda -net user,tftp=/home/xslaby/tftp,bootfile=/pxelinux.0,hostfwd=tcp::-:22,hostfwd=tcp::3632-:3632 -net nic,model=virtio -serial pty -balloon virtio -device virtio-tablet-pci -vga virtio -kernel /home/latest/my/arch/x86/boot/bzImage -append root=/dev/sda1 console=ttyS0,115200 loglevel=debug -snapshot I do dmesg -w # on the console and on serial console: while :; do for aa in `seq 1 10`; do ./pps & done; wait; done Note the latter can cause interrupt "storm" (~ 700 irqs per second) as much output is generated. This can lead to some race condition. serial is on IRQ4 and virtio gpu on IRQ10 which has lower priority AFAIK. thanks, -- js suse labs
[PATCH] drm: Limit individual events to 1 KiB
On Thu, Dec 15, 2016 at 12:48:34PM +0100, Thierry Reding wrote:
> From: Thierry Reding
>
> Ever since support for vblank handling and event processing was added to
> libdrm (v2.4.16), events were read in 1 KiB chunks in drmHandleEvent().
> In addition, the implementation of drm_read() will not unqueue events if
> they can't be successfully passed to userspace. The result is that it's
> possible for a driver to send applications into a busy loop by creating
> events larger than 1 KiB.
>
> There is a limit to the event space that each DRM device can carry, but
> it is set to 4 KiB.
>
> While no driver currently produces events larger than 1 KiB, and it's
> fairly unlikely that anyone ever will, it's best to err on the side of
> caution and limit the size of individual events to 1 KiB. This ensures
> that libdrm will never run into the busy loop. In order to allow for
> the DRM device to queue multiple events, the event space limit is kept
> at 4 KiB.
>
> A complementary patch will be applied to libdrm to bump the read size
> for events to 4 KiB to ensure that newer versions of libdrm running on
> kernels without this fix will not run into the busy loop either.
>
> Signed-off-by: Thierry Reding
> ---
> drivers/gpu/drm/drm_fops.c | 18 ++
> 1 file changed, 18 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
> index 48e106557c92..3792e4434319 100644
> --- a/drivers/gpu/drm/drm_fops.c
> +++ b/drivers/gpu/drm/drm_fops.c
> @@ -582,6 +582,15 @@ EXPORT_SYMBOL(drm_poll);
> * This is the locked version of drm_event_reserve_init() for callers which
> * already hold dev->event_lock.
> *
> + * Note that libdrm has been reading events in chunks of 1 KiB since forever
> + * (see drmHandleEvent()) and up to and including v2.4.74. Unfortunately the
> + * drm_read() implementation won't return an error if an event doesn't fit
> + * into that 1 KiB and instead these events will simply be queued again. If
> + * a driver was to send out an event larger than 1 KiB, an application that
> + * polls the DRM file descriptor and calling drmHandleEvent() to process the
> + * events would go into a busy loop. This is caused by drm_poll() always
> + * return immediately, but drm_read() never being able to dequeue the event.
> + *
> * RETURNS:
> *
> * 0 on success or a negative error code on failure.
> @@ -591,6 +600,15 @@ int drm_event_reserve_init_locked(struct drm_device *dev,
> struct drm_pending_event *p,
> struct drm_event *e)
> {
> + /*
> + * Limit events to 1 KiB, which is the read size of libdrm (up to
> + * to and including v2.4.74) when processing events, in order to
> + * prevent any of the applications using libdrm from potentially
> + * going into a busy loop.
> + */
> + if (e->length > 1024)
> + return -EINVAL;
Are we really expecting events that large? A much lower limit,
accompanied by a WARN/BUG, is what I'd go for.
> +
> if (file_priv->event_space < e->length)
> return -ENOMEM;
>
> --
> 2.10.2
>
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel OTC
[PATCH] drm/i915: use udelay for very short delays
On Thu, Dec 15, 2016 at 10:34:00AM +, Nicholas Mc Guire wrote: > On Thu, Dec 15, 2016 at 12:20:01PM +0200, Jani Nikula wrote: > > On Thu, 15 Dec 2016, Ville Syrjälä > > wrote: > > > On Thu, Dec 15, 2016 at 11:52:34AM +0200, Jani Nikula wrote: > > >> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > > >> > On Thu, Dec 15, 2016 at 11:08:49AM +0200, Jani Nikula wrote: > > >> >> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > > >> >> > Even on fast systems a 2 microsecond delay is most likely more > > >> >> > efficient > > >> >> > as a busy-wait loop. The overhead of a hrtimer does not seem > > >> >> > warranted - > > >> >> > change this to a udelay(2). > > >> >> > > >> >> Similar concerns as in [1]. We don't need the accuracy of udelay() > > >> >> here, > > >> >> so this boils down to which is the better use of CPU. We could > > >> >> probably > > >> >> relax the max delay more if that was helpful. But I'm not immediately > > >> >> sold on "is most likely more efficient" which sounds like a gut > > >> >> feeling. > > >> >> > > >> >> I'm sorry it's not clear in my other reply that I do appreciate > > >> >> addressing incorrect/silly use of usleep_range(); I'm just not (yet) > > >> >> convinced udelay() is the answer. > > >> > > > >> > if the delay is not critical and all that is needed > > >> > is an assurance that it is greater than X us then > > >> > usleep_range is fine with a relaxed limit. > > >> > So from what you wrote my patch proposal is wrong - the > > >> > udelay() is not the way to got. > > >> > My intent is to get rid of very small usleep_range() cases > > >> > so if usleep_range(20,50) causes no issues with this driver > > >> > and does not induce any performance penalty then that would > > >> > be the way to go I think. > > >> > > >> Okay, so I looked at the code, and I looked at our spec, and I looked at > > >> the MIPI D-PHY spec, and I cried a little. > > >> > > >> Long story short, I think usleep_range(10, 50) will be fine. > > > > > > Note that I really want to see a comment next to every delay like this > > > documenting the actual hardware requirement, if the delay used by the > > > code doesn't 100% match it. > > > > Our spec says, "Wait for 2us for ULPS to complete". That's a simplistic > > view wrt D-PHY, and our code doesn't even match the spec. Hence the > > tears. Want to propose a wording for the comment so we can apply this > > change, without going for a full rewrite of the sequence? > > > is that suitable or am I overdoing it ? > > - usleep_range(2, 3); > + /* delay for at least 2us - relaxed to 10-50 to allow > +* hrtimer subsystem to optimize uncritical timer handling > +*/ That's entirely too verbose IMO, and the reason for using usleep_range() is pretty obvious without spelling it out. All we really want to know is what the spec says is the minimum acceptable delay. > + usleep_range(10, 50); > > thx! > hofrat -- Ville Syrjälä Intel OTC
[PATCH] drm/i915: use udelay for very short delays
On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > On Thu, Dec 15, 2016 at 12:20:01PM +0200, Jani Nikula wrote: >> On Thu, 15 Dec 2016, Ville Syrjälä >> wrote: >> > On Thu, Dec 15, 2016 at 11:52:34AM +0200, Jani Nikula wrote: >> >> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: >> >> > On Thu, Dec 15, 2016 at 11:08:49AM +0200, Jani Nikula wrote: >> >> >> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: >> >> >> > Even on fast systems a 2 microsecond delay is most likely more >> >> >> > efficient >> >> >> > as a busy-wait loop. The overhead of a hrtimer does not seem >> >> >> > warranted - >> >> >> > change this to a udelay(2). >> >> >> >> >> >> Similar concerns as in [1]. We don't need the accuracy of udelay() >> >> >> here, >> >> >> so this boils down to which is the better use of CPU. We could probably >> >> >> relax the max delay more if that was helpful. But I'm not immediately >> >> >> sold on "is most likely more efficient" which sounds like a gut >> >> >> feeling. >> >> >> >> >> >> I'm sorry it's not clear in my other reply that I do appreciate >> >> >> addressing incorrect/silly use of usleep_range(); I'm just not (yet) >> >> >> convinced udelay() is the answer. >> >> > >> >> > if the delay is not critical and all that is needed >> >> > is an assurance that it is greater than X us then >> >> > usleep_range is fine with a relaxed limit. >> >> > So from what you wrote my patch proposal is wrong - the >> >> > udelay() is not the way to got. >> >> > My intent is to get rid of very small usleep_range() cases >> >> > so if usleep_range(20,50) causes no issues with this driver >> >> > and does not induce any performance penalty then that would >> >> > be the way to go I think. >> >> >> >> Okay, so I looked at the code, and I looked at our spec, and I looked at >> >> the MIPI D-PHY spec, and I cried a little. >> >> >> >> Long story short, I think usleep_range(10, 50) will be fine. >> > >> > Note that I really want to see a comment next to every delay like this >> > documenting the actual hardware requirement, if the delay used by the >> > code doesn't 100% match it. >> >> Our spec says, "Wait for 2us for ULPS to complete". That's a simplistic >> view wrt D-PHY, and our code doesn't even match the spec. Hence the >> tears. Want to propose a wording for the comment so we can apply this >> change, without going for a full rewrite of the sequence? >> > is that suitable or am I overdoing it ? > > - usleep_range(2, 3); > + /* delay for at least 2us - relaxed to 10-50 to allow > +* hrtimer subsystem to optimize uncritical timer handling > +*/ > + usleep_range(10, 50); I'm fine with that. Or maybe just make it "relaxed to allow" without the values. Jani. > > thx! > hofrat -- Jani Nikula, Intel Open Source Technology Center
[PATCH v2] drm/etnaviv: Set up initial PULSE_EATER register
Set up the PULSE_EATER register (0x0010C) in etnaviv_gpu_hw_init. This
ports three mostly undocumented model/revision-specific register
overrides from the Vivante kernel driver.
This is relevant as at least the "disable internal DFS" for revisions >
0x5420 has shown to have a huge impact on shader performance (sped up
memory read performance by 7.5x and write performance by 1.5x) on an
affected GPU.
Signed-off-by: Wladimir J. van der Laan
---
Changes since v1:
- Add Signed-off-by line
- Move functionality to function
- Remove #define
- Collapse comments
drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 34 ++
1 file changed, 34 insertions(+)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
index cbb969e..6990a5d 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
@@ -548,6 +548,37 @@ void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32
address, u16 prefetch)
VIVS_FE_COMMAND_CONTROL_PREFETCH(prefetch));
}
+static void etnaviv_gpu_setup_pulse_eater(struct etnaviv_gpu *gpu)
+{
+ /*
+* base value for VIVS_PM_PULSE_EATER register on models where it
cannot be read,
+* from vivante kernel driver
+*/
+ u32 pulse_eater = 0x01590880;
+
+ if (etnaviv_is_model_rev(gpu, GC4000, 0x5208) ||
+ etnaviv_is_model_rev(gpu, GC4000, 0x5222)) {
+ pulse_eater |= BIT(23);
+ gpu_write(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+ }
+
+ if (etnaviv_is_model_rev(gpu, GC1000, 0x5039) ||
+ etnaviv_is_model_rev(gpu, GC1000, 0x5040)) {
+ pulse_eater &= ~BIT(16);
+ pulse_eater |= BIT(17);
+ gpu_write(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+ }
+
+ if ((gpu->identity.revision > 0x5420) &&
+(gpu->identity.features & chipFeatures_PIPE_3D))
+ {
+ /* disable internal DFS: this is extremely important to
performance */
+ pulse_eater = gpu_read(gpu, VIVS_PM_PULSE_EATER);
+ pulse_eater |= BIT(18);
+ gpu_write(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
+ }
+}
+
static void etnaviv_gpu_hw_init(struct etnaviv_gpu *gpu)
{
u16 prefetch;
@@ -588,6 +619,9 @@ static void etnaviv_gpu_hw_init(struct etnaviv_gpu *gpu)
gpu_write(gpu, VIVS_MC_BUS_CONFIG, bus_config);
}
+ /* setup the pulse eater */
+ etnaviv_gpu_setup_pulse_eater(gpu);
+
/* setup the MMU */
etnaviv_iommu_restore(gpu);
--
2.7.4
[PATCH] drm: Fix typo in drm_event_reserve_init() kerneldoc
On Thu, Dec 15, 2016 at 12:36:02PM +0100, Thierry Reding wrote: > From: Thierry Reding > > drm_event_reserve_init_locked() is the correct function to call when > already holding the dev->event_lock lock. > > Signed-off-by: Thierry Reding Pushed to drm-misc-next, thanks. -Daniel > --- > drivers/gpu/drm/drm_fops.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c > index 5d96de40b63f..48e106557c92 100644 > --- a/drivers/gpu/drm/drm_fops.c > +++ b/drivers/gpu/drm/drm_fops.c > @@ -622,7 +622,7 @@ EXPORT_SYMBOL(drm_event_reserve_init_locked); > * kmalloc and @p must be the first member element. > * > * Callers which already hold dev->event_lock should use > - * drm_event_reserve_init() instead. > + * drm_event_reserve_init_locked() instead. > * > * RETURNS: > * > -- > 2.10.2 > > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[PATCH/RFC 0/7] Remove the omapdrm device from platform code
On 15/12/16 12:07, Laurent Pinchart wrote: >> Or do you have any other ideas how to pass flags to the driver based on >> the SoC revision? > > Or retrieve the SoC revision in the driver. I know this is a bad thing to do > in general, but when handling errata that are specific to certain ES > versions, > it's hard to avoid. https://patchwork.kernel.org/patch/9141381/ has been > developed for that (or at least a very similar) purpose. Ah, that should work. Tomi -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/29af53df/attachment.sig>
[PATCH 17/34] drm: kselftest for drm_mm and color adjustment
On ma, 2016-12-12 at 11:53 +, Chris Wilson wrote:
> Check that after applying the driver's color adjustment, fitting of the
> node and its alignment are still correct.
>
> Signed-off-by: Chris Wilson
> +static void no_color_touching(struct drm_mm_node *node,
> + Â Â Â Â Â Â unsigned long color,
> + Â Â Â Â Â Â u64 *start,
> + Â Â Â Â Â Â u64 *end)
Function name made me think of a bool returning one.
Rather call it "{separate,space}_adjacent_colors" or so.
> +{
> + if (node->allocated && node->color != color)
> + ++*start;
> +
> + node = list_next_entry(node, node_list);
> + if (node->allocated && node->color != color)
> + --*end;
> +}
> +
> +static int igt_color(void *ignored)
> +{
> + node->start += n + 1;
> + rem = node->start;
> + rem %= n + count;
rem = div64...?
If I could keep with the loop variables, should be good;
Reviewed-by: Joonas Lahtinen
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
[PATCH v2] drm/etnaviv: Add new GC3000 sensitive states
- Add PS.INST_ADDR (0x01028) and VS.INST_ADDR (0x0086C): GC3000 loads
shader code from these addresses if ICACHE is used.
- Add new NFE vertex stream addresses (0x14600).
- Add PE Multple Render Target pipe addresses (0x14800).
- Add TS Multiple Render Target pipe addresses (0x017C0, 0x17E0).
Signed-off-by: Wladimir J. van der Laan
---
In comparison to v1 this adds a signed-off-by line and adds MRT
states.
drivers/gpu/drm/etnaviv/etnaviv_cmd_parser.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/etnaviv/etnaviv_cmd_parser.c
b/drivers/gpu/drm/etnaviv/etnaviv_cmd_parser.c
index 2a2e5e3..6e3bbcf 100644
--- a/drivers/gpu/drm/etnaviv/etnaviv_cmd_parser.c
+++ b/drivers/gpu/drm/etnaviv/etnaviv_cmd_parser.c
@@ -56,6 +56,8 @@ static const struct {
ST(0x0644, 1),
ST(0x064c, 1),
ST(0x0680, 8),
+ ST(0x086c, 1),
+ ST(0x1028, 1),
ST(0x1410, 1),
ST(0x1430, 1),
ST(0x1458, 1),
@@ -73,8 +75,12 @@ static const struct {
ST(0x16c0, 8),
ST(0x16e0, 8),
ST(0x1740, 8),
+ ST(0x17c0, 8),
+ ST(0x17e0, 8),
ST(0x2400, 14 * 16),
ST(0x10800, 32 * 16),
+ ST(0x14600, 16),
+ ST(0x14800, 8 * 8),
#undef ST
};
--
2.7.4
[PATCH 33/34] drm: Fix drm_mm search and insertion
On Thu, Dec 15, 2016 at 02:28:32PM +0200, Joonas Lahtinen wrote: > On ma, 2016-12-12 at 11:53 +, Chris Wilson wrote: > > @@ -799,7 +706,7 @@ bool drm_mm_scan_add_block(struct drm_mm_scan *scan, > > Â if (adj_end <= adj_start || adj_end - adj_start < scan->size) > > Â return false; > > Â > > - if (scan->flags == DRM_MM_CREATE_TOP) > > + if (scan->flags == DRM_MM_INSERT_HIGH) > > Flags are usually checked with & if somebody wants to add them later. > Otherwise you could call it "mode". Once upon a time, they were intended to be flags. They have since devolved back into a mode. The only suitable argument for my laziness was what if I wanted to add a flag later! -Chris -- Chris Wilson, Intel Open Source Technology Centre
[PATCH v2 3/6] drm/atomic: Clean up wait_for_vblanks, v2.
Stop relying on a per crtc_state last_vblank_count, we shouldn't touch
crtc_state after commit. Move it to atomic_state->crtcs.
Also stop re-using new_crtc_state->enable, we can now simply set a
bitmask with crtc_crtc_mask.
Changes since v1:
- Keep last_vblank_count in __drm_crtc_state.
---
diff --git a/drivers/gpu/drm/drm_atomic_helper.c
b/drivers/gpu/drm/drm_atomic_helper.c
index d19563651e07..88c0986d226a 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1110,19 +1110,19 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device
*dev,
struct drm_crtc *crtc;
struct drm_crtc_state *old_crtc_state;
int i, ret;
+ unsigned crtc_mask = 0;
- for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
- /* No one cares about the old state, so abuse it for tracking
-* and store whether we hold a vblank reference (and should do a
-* vblank wait) in the ->enable boolean. */
- old_crtc_state->enable = false;
+/*
+ * Legacy cursor ioctls are completely unsynced, and userspace
+ * relies on that (by doing tons of cursor updates).
+ */
+ if (old_state->legacy_cursor_update)
+ return;
- if (!crtc->state->active)
- continue;
+ for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
+ struct drm_crtc_state *new_crtc_state = crtc->state;
- /* Legacy cursor ioctls are completely unsynced, and userspace
-* relies on that (by doing tons of cursor updates). */
- if (old_state->legacy_cursor_update)
+ if (!new_crtc_state->active)
continue;
if (!drm_atomic_helper_framebuffer_changed(dev,
@@ -1133,16 +1133,16 @@ drm_atomic_helper_wait_for_vblanks(struct drm_device
*dev,
if (ret != 0)
continue;
- old_crtc_state->enable = true;
- old_crtc_state->last_vblank_count = drm_crtc_vblank_count(crtc);
+ crtc_mask |= drm_crtc_mask(crtc);
+ old_state->crtcs[i].last_vblank_count =
drm_crtc_vblank_count(crtc);
}
for_each_crtc_in_state(old_state, crtc, old_crtc_state, i) {
- if (!old_crtc_state->enable)
+ if (!(crtc_mask & drm_crtc_mask(crtc)))
continue;
ret = wait_event_timeout(dev->vblank[i].queue,
- old_crtc_state->last_vblank_count !=
+ old_state->crtcs[i].last_vblank_count !=
drm_crtc_vblank_count(crtc),
msecs_to_jiffies(50));
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index d6d241f63b9f..617f095e31ba 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -145,6 +145,7 @@ struct __drm_crtcs_state {
struct drm_crtc_state *state;
struct drm_crtc_commit *commit;
s64 __user *out_fence_ptr;
+ unsigned last_vblank_count;
};
struct __drm_connnectors_state {
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 946672f97e1e..e03d194be086 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -93,8 +93,6 @@ struct drm_plane_helper_funcs;
* @plane_mask: bitmask of (1 << drm_plane_index(plane)) of attached planes
* @connector_mask: bitmask of (1 << drm_connector_index(connector)) of
attached connectors
* @encoder_mask: bitmask of (1 << drm_encoder_index(encoder)) of attached
encoders
- * @last_vblank_count: for helpers and drivers to capture the vblank of the
- * update to ensure framebuffer cleanup isn't done too early
* @adjusted_mode: for use by helpers and drivers to compute adjusted mode
timings
* @mode: current mode timings
* @mode_blob: &drm_property_blob for @mode
@@ -140,9 +138,6 @@ struct drm_crtc_state {
u32 connector_mask;
u32 encoder_mask;
- /* last_vblank_count: for vblank waits before cleanup */
- u32 last_vblank_count;
-
/* adjusted_mode: for use by helpers and drivers */
struct drm_display_mode adjusted_mode;
[PATCH] drm: Limit individual events to 1 KiB
From: Thierry Reding
Ever since support for vblank handling and event processing was added to
libdrm (v2.4.16), events were read in 1 KiB chunks in drmHandleEvent().
In addition, the implementation of drm_read() will not unqueue events if
they can't be successfully passed to userspace. The result is that it's
possible for a driver to send applications into a busy loop by creating
events larger than 1 KiB.
There is a limit to the event space that each DRM device can carry, but
it is set to 4 KiB.
While no driver currently produces events larger than 1 KiB, and it's
fairly unlikely that anyone ever will, it's best to err on the side of
caution and limit the size of individual events to 1 KiB. This ensures
that libdrm will never run into the busy loop. In order to allow for
the DRM device to queue multiple events, the event space limit is kept
at 4 KiB.
A complementary patch will be applied to libdrm to bump the read size
for events to 4 KiB to ensure that newer versions of libdrm running on
kernels without this fix will not run into the busy loop either.
Signed-off-by: Thierry Reding
---
drivers/gpu/drm/drm_fops.c | 18 ++
1 file changed, 18 insertions(+)
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 48e106557c92..3792e4434319 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -582,6 +582,15 @@ EXPORT_SYMBOL(drm_poll);
* This is the locked version of drm_event_reserve_init() for callers which
* already hold dev->event_lock.
*
+ * Note that libdrm has been reading events in chunks of 1 KiB since forever
+ * (see drmHandleEvent()) and up to and including v2.4.74. Unfortunately the
+ * drm_read() implementation won't return an error if an event doesn't fit
+ * into that 1 KiB and instead these events will simply be queued again. If
+ * a driver was to send out an event larger than 1 KiB, an application that
+ * polls the DRM file descriptor and calling drmHandleEvent() to process the
+ * events would go into a busy loop. This is caused by drm_poll() always
+ * return immediately, but drm_read() never being able to dequeue the event.
+ *
* RETURNS:
*
* 0 on success or a negative error code on failure.
@@ -591,6 +600,15 @@ int drm_event_reserve_init_locked(struct drm_device *dev,
struct drm_pending_event *p,
struct drm_event *e)
{
+ /*
+* Limit events to 1 KiB, which is the read size of libdrm (up to
+* to and including v2.4.74) when processing events, in order to
+* prevent any of the applications using libdrm from potentially
+* going into a busy loop.
+*/
+ if (e->length > 1024)
+ return -EINVAL;
+
if (file_priv->event_space < e->length)
return -ENOMEM;
--
2.10.2
[Intel-gfx] [PATCH] drm/i915: use udelay for very short delays
On Thu, Dec 15, 2016 at 11:51 AM, Nicholas Mc Guire wrote: > On Thu, Dec 15, 2016 at 10:27:59AM +0100, Daniel Vetter wrote: >> On Thu, Dec 15, 2016 at 10:25:19AM +0100, Daniel Vetter wrote: >> > On Thu, Dec 15, 2016 at 11:08:49AM +0200, Jani Nikula wrote: >> > > On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: >> > > > Even on fast systems a 2 microsecond delay is most likely more >> > > > efficient >> > > > as a busy-wait loop. The overhead of a hrtimer does not seem warranted >> > > > - >> > > > change this to a udelay(2). >> > > >> > > Similar concerns as in [1]. We don't need the accuracy of udelay() here, >> > > so this boils down to which is the better use of CPU. We could probably >> > > relax the max delay more if that was helpful. But I'm not immediately >> > > sold on "is most likely more efficient" which sounds like a gut feeling. >> > > >> > > I'm sorry it's not clear in my other reply that I do appreciate >> > > addressing incorrect/silly use of usleep_range(); I'm just not (yet) >> > > convinced udelay() is the answer. >> > >> > So one reason why we unconditionally use *sleep variants is the >> > might_sleep check. Because in the past people have used udelay and mdelay, >> > those delays had to be increased a lot because hw, and at the same time >> > someone added users of these functions to our irq helper, resulting in irq >> > handling times measures in multiple ms. That's not good. >> > >> > So until someone can demonstrate that there's a real benefit (which let's >> > be honest, for modeset code, will never be the case) I very highly prefer >> > to use *sleep* functions. They prevent some silly things from happening by >> > accident. Micro-optimizing modeset code and hampering maitainability in >> > the process is not good. >> >> Also, the entire premise seems backwards: usleep_range is inefficient for >> certain parameter ranges and better replaced with udelay. That makes >> sense. >> >> But why exactly do we not fix udelay_range then, but instead do a cocci >> job crawling through all the thousands of callers? Just fix usleep(_range) >> to use udelay for very small values (and still keep the might_sleep check >> ofc) if that's more efficient! > > its actually not thousands more like a few dozen: There's 1k + usleep* calls you need to audit and teach people how to exactly use it. > usleep_range(min,max) in linux-stable 4.9.0 > > 1648 calls total > 1488 pass numeric values only (90.29%) > 27 min below 10us (1.81%) > 40 min above 10ms (2.68%) > min out of spec 4.50% > 76 preprocessor constants (4.61%) >1 min below 10us (1.31%) >8 min above 10ms (10.52%) > min out of spec 11.84% > 85 expressions (5.15%) > 1(0) min below 10us (1.50%)* > 6(2) min above 10ms (7.50%)* > min out of spec 9.0% > Errors: > 23 where min==max (1.39%) >0 where max < min (0.00%) > > Total: > Bugs: 6.48%-10.70%* > Crit: 3.09%-3.15%* (min < 10 and min==max and max < min) > Detectable by coccinelle: > Bugs: 74/103 (71.8%) > Crit: 50/52 (96.1%) > > *based on estimats as runtime values not known. > > > there is no usleep() as noted in Documentation/timers/timers-howto.txt > - Why not usleep? > On slower systems, (embedded, OR perhaps a speed- > stepped PC!) the overhead of setting up the hrtimers > for usleep *may* not be worth it. Such an evaluation > will obviously depend on your specific situation, but > it is something to be aware of. > > and it suggests to use different API for different ranges - sounds sane > to me and seems to cover the needs of drivers. git grep usleep seems to disagree, at least I see a bunch of usleep() calls. And per Rusty's api usability scale the ultimate fucntion is dwim(). It just feels to me like pushing such trade-off decisions to drivers is bad design because a) the tradeoffs depend upon the cpu you're running on b) driver writers are pretty good at getting such tradeoffs wrong in general. Aiming for a more dwim()-like api for this here makes sense, instead of an eternal fight to correct drivers that get it wrong all the time. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch
[PATCH] drm: Fix typo in drm_event_reserve_init() kerneldoc
From: Thierry Reding drm_event_reserve_init_locked() is the correct function to call when already holding the dev->event_lock lock. Signed-off-by: Thierry Reding --- drivers/gpu/drm/drm_fops.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c index 5d96de40b63f..48e106557c92 100644 --- a/drivers/gpu/drm/drm_fops.c +++ b/drivers/gpu/drm/drm_fops.c @@ -622,7 +622,7 @@ EXPORT_SYMBOL(drm_event_reserve_init_locked); * kmalloc and @p must be the first member element. * * Callers which already hold dev->event_lock should use - * drm_event_reserve_init() instead. + * drm_event_reserve_init_locked() instead. * * RETURNS: * -- 2.10.2
[PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD handles (v3)
>-Original Message-
>From: Grazvydas Ignotas [mailto:notasas at gmail.com]
>Sent: Thursday, December 15, 2016 5:52 PM
>To: Nath, Arindam
>Cc: Emil Velikov; David Airlie; Deucher, Alexander; ML dri-devel; amd-gfx
>mailing list; Koenig, Christian
>Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
>handles (v3)
>
>On Thu, Dec 15, 2016 at 1:47 PM, Nath, Arindam
>wrote:
>>>-Original Message-
>>>From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>>>Sent: Thursday, December 15, 2016 5:01 PM
>>>To: Nath, Arindam
>>>Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
>>>Koenig, Christian
>>>Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
>>>handles (v3)
>>>
>>>On 15 December 2016 at 07:30, Nath, Arindam
>>>wrote:
>-Original Message-
>From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>Sent: Wednesday, December 14, 2016 9:26 PM
>To: Nath, Arindam
>Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
>Koenig, Christian
>Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used
>UVD
>handles (v3)
>
>On 12 December 2016 at 18:49, wrote:
>> From: Arindam Nath
>>
>> Change History
>> --
>>
>> v3: changes suggested by Christian
>> - Add a check for UVD IP block using AMDGPU_HW_IP_UVD
>> query type.
>> - Add a check for asic_type to be less than
>> CHIP_POLARIS10 since starting Polaris, we support
>> unlimited UVD instances.
>> - Add kerneldoc style comment for
>> amdgpu_uvd_used_handles().
>>
>> v2: as suggested by Christian
>> - Add a new query AMDGPU_INFO_NUM_HANDLES
>> - Create a helper function to return the number
>> of currently used UVD handles.
>> - Modify the logic to count the number of used
>> UVD handles since handles can be freed in
>> non-linear fashion.
>>
>> v1:
>> - User might want to query the maximum number of UVD
>> instances supported by firmware. In addition to that,
>> if there are multiple applications using UVD handles
>> at the same time, he might also want to query the
>> currently used number of handles.
>>
>> For this we add two variables max_handles and
>> used_handles inside drm_amdgpu_info_hw_ip. So now
>> an application (or libdrm) can use AMDGPU_INFO IOCTL
>> with AMDGPU_INFO_HW_IP_INFO query type to get these
>> values.
>>
>> Signed-off-by: Arindam Nath
>> Reviewed-by: Christian König
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 21
>+
>> drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 25
>+
>> drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.h | 1 +
>> include/uapi/drm/amdgpu_drm.h | 9 +
>> 4 files changed, 56 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index 174eb59..3273d8c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -570,6 +570,27 @@ static int amdgpu_info_ioctl(struct drm_device
>*dev, void *data, struct drm_file
>> return -EINVAL;
>> }
>> }
>> + case AMDGPU_INFO_NUM_HANDLES: {
>> + struct drm_amdgpu_info_num_handles handle;
>> +
>> + switch (info->query_hw_ip.type) {
>> + case AMDGPU_HW_IP_UVD:
>> + /* Starting Polaris, we support unlimited UVD
>> handles */
>> + if (adev->asic_type < CHIP_POLARIS10) {
>> + handle.uvd_max_handles =
>> adev->uvd.max_handles;
>> + handle.uvd_used_handles =
>amdgpu_uvd_used_handles(adev);
>> +
>> + return copy_to_user(out, &handle,
>> + min((size_t)size,
>> sizeof(handle))) ? -EFAULT : 0;
>> + } else {
>> + return -EINVAL;
>Using EINVAL doesn't seem right here. As per man 3 ioctl
>
> EINVAL The request or arg argument is not valid for this device.
>
>A bit further down you can see the one you want.
>
> ENODEV The fildes argument refers to a valid STREAMS device, but
>the corresponding device driver does not support the ioctl() function.
Emil, ENODEV would mean the driver does not support ioctl() itself. But in
>>>our case ioctl() is supported.
Since we extract the query type from arg passed to ioctl(), and it is this
>>>query AMDGPU_INFO_NUM_HANDLES which is not supported by driver
>(for
Polaris), doesnât returning EINVAL make more sense here?
>>>Unless I'm reading the cod
[PATCH] drm/etnaviv: Set up initial PULSE_EATER register
Am Mittwoch, den 14.12.2016, 09:39 +0100 schrieb Wladimir J. van der
Laan:
> Set up the PULSE_EATER register (0x0010C) in etnaviv_gpu_hw_init. This
> ports three mostly undocumented model/revision-specific register
> overrides from the Vivante kernel driver.
>
> This is relevant as at least the "disable internal DFS" for revisions >
> 0x5420 has shown to have a huge impact on shader performance (sped up
> memory read performance by 7.5x and write performance by 1.5x) on an
> affected GPU.
Missing Singed-off-by
> ---
> drivers/gpu/drm/etnaviv/etnaviv_gpu.c | 30 ++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> index 1e0ec81..e1a9d64 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gpu.c
> @@ -548,6 +548,9 @@ void etnaviv_gpu_start_fe(struct etnaviv_gpu *gpu, u32
> address, u16 prefetch)
> VIVS_FE_COMMAND_CONTROL_PREFETCH(prefetch));
> }
>
> +/* base value for VIVS_PM_PULSE_EATER register on models where it cannot be
> read */
> +#define PULSE_EATER_BASE_VALUE (0x01590880)
> +
Please don't introduce freestanding defines.
> static void etnaviv_gpu_hw_init(struct etnaviv_gpu *gpu)
> {
> u16 prefetch;
> @@ -588,6 +591,33 @@ static void etnaviv_gpu_hw_init(struct etnaviv_gpu *gpu)
> gpu_write(gpu, VIVS_MC_BUS_CONFIG, bus_config);
> }
Please split this out into a separate function like the mlgc setup.
>
> + /* set up the pulse eater */
> + if (etnaviv_is_model_rev(gpu, GC4000, 0x5208) ||
> + etnaviv_is_model_rev(gpu, GC4000, 0x5222)) {
> + /* undocumented workaround from vivante kernel driver */
> + u32 pulse_eater = PULSE_EATER_BASE_VALUE;
If this is in a separate function, the definition of this variable can
move out of the individual blocks. Just have a single
u32 pulse_eater = 0x01590880;
at the top of the function with a comment where this value comes from.
> + pulse_eater |= BIT(23);
> + gpu_write(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
> + }
> +
> + if (etnaviv_is_model_rev(gpu, GC1000, 0x5039) ||
> + etnaviv_is_model_rev(gpu, GC1000, 0x5040)) {
> + /* undocumented workaround from vivante kernel driver */
I'm not sure how much value this comment holds. Most of the PM register
setup consists of undocumented workarounds extracted from the Vivante
driver. But that's a matter of taste, so feel free to remove or keep it.
> + u32 pulse_eater = PULSE_EATER_BASE_VALUE;
> + pulse_eater &= ~BIT(16);
> + pulse_eater |= BIT(17);
> + gpu_write(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
> + }
> +
> + if ((gpu->identity.revision > 0x5420) &&
> +(gpu->identity.features & chipFeatures_PIPE_3D))
> + {
> + /* disable internal DFS: this is extremely important to
> performance */
> + u32 pulse_eater = gpu_read(gpu, VIVS_PM_PULSE_EATER);
> + pulse_eater |= BIT(18);
> + gpu_write(gpu, VIVS_PM_PULSE_EATER, pulse_eater);
> + }
> +
> /* setup the MMU */
> etnaviv_iommu_restore(gpu);
>
Regards,
Lucas
[PATCH] drm/etnaviv: Add new GC3000 sensitive states
Am Mittwoch, den 14.12.2016, 09:53 +0100 schrieb Wladimir J. van der
Laan:
> - Add PS.INST_ADDR and VS.INST_ADDR: GC3000 loads shader code from these
> addresses if ICACHE
> is enabled.
>
> - Add new NFE vertex stream addresses.
Please resend with a proper Signed-off-by line. I can't take a patch for
mainline inclusion without it.
> ---
> drivers/gpu/drm/etnaviv/etnaviv_cmd_parser.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_cmd_parser.c
> b/drivers/gpu/drm/etnaviv/etnaviv_cmd_parser.c
> index 2a2e5e3..abb37c6 100644
> --- a/drivers/gpu/drm/etnaviv/etnaviv_cmd_parser.c
> +++ b/drivers/gpu/drm/etnaviv/etnaviv_cmd_parser.c
> @@ -56,6 +56,8 @@ static const struct {
> ST(0x0644, 1),
> ST(0x064c, 1),
> ST(0x0680, 8),
> + ST(0x086c, 1),
> + ST(0x1028, 1),
> ST(0x1410, 1),
> ST(0x1430, 1),
> ST(0x1458, 1),
> @@ -75,6 +77,7 @@ static const struct {
> ST(0x1740, 8),
> ST(0x2400, 14 * 16),
> ST(0x10800, 32 * 16),
> + ST(0x14600, 16),
> #undef ST
> };
>
[PATCH] drm/i915: use udelay for very short delays
On Thu, 15 Dec 2016, Ville Syrjälä wrote: > On Thu, Dec 15, 2016 at 11:52:34AM +0200, Jani Nikula wrote: >> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: >> > On Thu, Dec 15, 2016 at 11:08:49AM +0200, Jani Nikula wrote: >> >> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: >> >> > Even on fast systems a 2 microsecond delay is most likely more efficient >> >> > as a busy-wait loop. The overhead of a hrtimer does not seem warranted - >> >> > change this to a udelay(2). >> >> >> >> Similar concerns as in [1]. We don't need the accuracy of udelay() here, >> >> so this boils down to which is the better use of CPU. We could probably >> >> relax the max delay more if that was helpful. But I'm not immediately >> >> sold on "is most likely more efficient" which sounds like a gut feeling. >> >> >> >> I'm sorry it's not clear in my other reply that I do appreciate >> >> addressing incorrect/silly use of usleep_range(); I'm just not (yet) >> >> convinced udelay() is the answer. >> > >> > if the delay is not critical and all that is needed >> > is an assurance that it is greater than X us then >> > usleep_range is fine with a relaxed limit. >> > So from what you wrote my patch proposal is wrong - the >> > udelay() is not the way to got. >> > My intent is to get rid of very small usleep_range() cases >> > so if usleep_range(20,50) causes no issues with this driver >> > and does not induce any performance penalty then that would >> > be the way to go I think. >> >> Okay, so I looked at the code, and I looked at our spec, and I looked at >> the MIPI D-PHY spec, and I cried a little. >> >> Long story short, I think usleep_range(10, 50) will be fine. > > Note that I really want to see a comment next to every delay like this > documenting the actual hardware requirement, if the delay used by the > code doesn't 100% match it. Our spec says, "Wait for 2us for ULPS to complete". That's a simplistic view wrt D-PHY, and our code doesn't even match the spec. Hence the tears. Want to propose a wording for the comment so we can apply this change, without going for a full rewrite of the sequence? BR, Jani. -- Jani Nikula, Intel Open Source Technology Center
[PATCH] fbdev: remove current maintainer
On Thu, 8 Dec 2016 10:34:12 +0200 Tomi Valkeinen wrote: > Remove Tomi Valkeinen from fbdev maintainer and mark fbdev as orphan. > > ... > > I just don't have time to even pretend I maintain fbdev, so mark it as Orphan. > > Anyone want to pick fbdev maintainership? I'll keep an eye on the mailing list, try to ensure that things keep ticking along.
[PATCH] drm/i915: use udelay for very short delays
On Thu, Dec 15, 2016 at 11:52:34AM +0200, Jani Nikula wrote: > On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > > On Thu, Dec 15, 2016 at 11:08:49AM +0200, Jani Nikula wrote: > >> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > >> > Even on fast systems a 2 microsecond delay is most likely more efficient > >> > as a busy-wait loop. The overhead of a hrtimer does not seem warranted - > >> > change this to a udelay(2). > >> > >> Similar concerns as in [1]. We don't need the accuracy of udelay() here, > >> so this boils down to which is the better use of CPU. We could probably > >> relax the max delay more if that was helpful. But I'm not immediately > >> sold on "is most likely more efficient" which sounds like a gut feeling. > >> > >> I'm sorry it's not clear in my other reply that I do appreciate > >> addressing incorrect/silly use of usleep_range(); I'm just not (yet) > >> convinced udelay() is the answer. > > > > if the delay is not critical and all that is needed > > is an assurance that it is greater than X us then > > usleep_range is fine with a relaxed limit. > > So from what you wrote my patch proposal is wrong - the > > udelay() is not the way to got. > > My intent is to get rid of very small usleep_range() cases > > so if usleep_range(20,50) causes no issues with this driver > > and does not induce any performance penalty then that would > > be the way to go I think. > > Okay, so I looked at the code, and I looked at our spec, and I looked at > the MIPI D-PHY spec, and I cried a little. > > Long story short, I think usleep_range(10, 50) will be fine. Note that I really want to see a comment next to every delay like this documenting the actual hardware requirement, if the delay used by the code doesn't 100% match it. -- Ville Syrjälä Intel OTC
[PATCH/RFC 0/7] Remove the omapdrm device from platform code
Hi Tomi, On Thursday 15 Dec 2016 10:08:47 Tomi Valkeinen wrote: > On 14/12/16 17:05, Tony Lindgren wrote: > > * Laurent Pinchart [161213 15:38]: > >> The series will be annoying to merge given how interleaved the ARM and > >> driver patches are. The easiest solution would be to merge everything > >> through the ARM tree (as the risk of conflict on the DRM side is low), > >> in which case some patches could be squashed together if desired > >> (especially the last two that wouldn't require renaming the driver > >> internally anymore). > > > > Maybe Tomi can set up an immutable branch once the patches have been > > reviewed? That way also I can merge it in too as needed. > > Yes, I think that's a good option. Then the series doesn't have to be so > artificially split into linux-omap and drm parts. > > I don't think there are much chances with conflicts on the linux-omap > side, as the only files touched are display.c and drm.c (well, and a > small change in Makefile). > > I like the series in general, but I still need to go through it in detail. > > And speaking of removing of platform data... > > Tony, the only big reason we still have the omapdss platform data > (include/linux/platform_data/omapdss.h) is the omapdss_version, which is > based on the OMAP SoC version. > > We need that in the driver, as the DSS IP revision hasn't been updated > in a couple of cases, or the issue comes from outside DSS. But there are > only a few of these cases, mostly we would do just fine with the DSS IP > revision. > > What do you think of a scheme, where we'd drop the platform data, but at > early platform init code we would inject a DT property or two into DSS's > DT data in those problematic cases? > > Or do you have any other ideas how to pass flags to the driver based on > the SoC revision? Or retrieve the SoC revision in the driver. I know this is a bad thing to do in general, but when handling errata that are specific to certain ES versions, it's hard to avoid. https://patchwork.kernel.org/patch/9141381/ has been developed for that (or at least a very similar) purpose. -- Regards, Laurent Pinchart
[PATCH] drm/i915: use udelay for very short delays
On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > On Thu, Dec 15, 2016 at 11:08:49AM +0200, Jani Nikula wrote: >> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: >> > Even on fast systems a 2 microsecond delay is most likely more efficient >> > as a busy-wait loop. The overhead of a hrtimer does not seem warranted - >> > change this to a udelay(2). >> >> Similar concerns as in [1]. We don't need the accuracy of udelay() here, >> so this boils down to which is the better use of CPU. We could probably >> relax the max delay more if that was helpful. But I'm not immediately >> sold on "is most likely more efficient" which sounds like a gut feeling. >> >> I'm sorry it's not clear in my other reply that I do appreciate >> addressing incorrect/silly use of usleep_range(); I'm just not (yet) >> convinced udelay() is the answer. > > if the delay is not critical and all that is needed > is an assurance that it is greater than X us then > usleep_range is fine with a relaxed limit. > So from what you wrote my patch proposal is wrong - the > udelay() is not the way to got. > My intent is to get rid of very small usleep_range() cases > so if usleep_range(20,50) causes no issues with this driver > and does not induce any performance penalty then that would > be the way to go I think. Okay, so I looked at the code, and I looked at our spec, and I looked at the MIPI D-PHY spec, and I cried a little. Long story short, I think usleep_range(10, 50) will be fine. BR, Jani. -- Jani Nikula, Intel Open Source Technology Center
[PATCH 14/34] drm: kselftest for drm_mm and range restricted eviction
On ma, 2016-12-12 at 11:53 +, Chris Wilson wrote:
> Check that we add arbitrary blocks to a restrited eviction scanner in
> order to find the first minimal hole that matches our request.
>
> Signed-off-by: Chris Wilson
> +static int igt_evict_range(void *ignored)
> +{
> + drm_for_each_prime(n, range_size) {
> + LIST_HEAD(evict_list);
> + struct evict_node *e, *en;
> + struct drm_mm_node tmp;
> + int nsize = (range_size - n + 1) / 2;
> + int err;
DRM_MM_BUG_ON(!nsize);
Same comment aplies about reserve vs insert (the amount of tests after
each, can be resolved with helpers).
Reviewed-by: Joonas Lahtinen
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
[PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD handles (v3)
>-Original Message-
>From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>Sent: Thursday, December 15, 2016 5:01 PM
>To: Nath, Arindam
>Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
>Koenig, Christian
>Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
>handles (v3)
>
>On 15 December 2016 at 07:30, Nath, Arindam
>wrote:
>>>-Original Message-
>>>From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>>>Sent: Wednesday, December 14, 2016 9:26 PM
>>>To: Nath, Arindam
>>>Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
>>>Koenig, Christian
>>>Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
>>>handles (v3)
>>>
>>>On 12 December 2016 at 18:49, wrote:
From: Arindam Nath
Change History
--
v3: changes suggested by Christian
- Add a check for UVD IP block using AMDGPU_HW_IP_UVD
query type.
- Add a check for asic_type to be less than
CHIP_POLARIS10 since starting Polaris, we support
unlimited UVD instances.
- Add kerneldoc style comment for
amdgpu_uvd_used_handles().
v2: as suggested by Christian
- Add a new query AMDGPU_INFO_NUM_HANDLES
- Create a helper function to return the number
of currently used UVD handles.
- Modify the logic to count the number of used
UVD handles since handles can be freed in
non-linear fashion.
v1:
- User might want to query the maximum number of UVD
instances supported by firmware. In addition to that,
if there are multiple applications using UVD handles
at the same time, he might also want to query the
currently used number of handles.
For this we add two variables max_handles and
used_handles inside drm_amdgpu_info_hw_ip. So now
an application (or libdrm) can use AMDGPU_INFO IOCTL
with AMDGPU_INFO_HW_IP_INFO query type to get these
values.
Signed-off-by: Arindam Nath
Reviewed-by: Christian König
---
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 21
>>>+
drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 25
>>>+
drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.h | 1 +
include/uapi/drm/amdgpu_drm.h | 9 +
4 files changed, 56 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>>b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index 174eb59..3273d8c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -570,6 +570,27 @@ static int amdgpu_info_ioctl(struct drm_device
>>>*dev, void *data, struct drm_file
return -EINVAL;
}
}
+ case AMDGPU_INFO_NUM_HANDLES: {
+ struct drm_amdgpu_info_num_handles handle;
+
+ switch (info->query_hw_ip.type) {
+ case AMDGPU_HW_IP_UVD:
+ /* Starting Polaris, we support unlimited UVD
handles */
+ if (adev->asic_type < CHIP_POLARIS10) {
+ handle.uvd_max_handles =
adev->uvd.max_handles;
+ handle.uvd_used_handles =
>>>amdgpu_uvd_used_handles(adev);
+
+ return copy_to_user(out, &handle,
+ min((size_t)size, sizeof(handle)))
? -EFAULT : 0;
+ } else {
+ return -EINVAL;
>>>Using EINVAL doesn't seem right here. As per man 3 ioctl
>>>
>>> EINVAL The request or arg argument is not valid for this device.
>>>
>>>A bit further down you can see the one you want.
>>>
>>> ENODEV The fildes argument refers to a valid STREAMS device, but
>>>the corresponding device driver does not support the ioctl() function.
>>
>> Emil, ENODEV would mean the driver does not support ioctl() itself. But in
>our case ioctl() is supported.
>>
>> Since we extract the query type from arg passed to ioctl(), and it is this
>query AMDGPU_INFO_NUM_HANDLES which is not supported by driver (for
>> Polaris), doesnât returning EINVAL make more sense here?
>>
>Unless I'm reading the code incorrectly - CHIP_POLARIS10 and older do
>not have support the query. Thus ENODEV is the one you want to use.
>Furthermore EINVAL is (mostly) used to indicate incorrect input
>(failed input validation) which userspace uses to check if kernel is
>too old/does not support X.
Actually, the code says only asics older than CHIP_POLARIS10 support the query,
beyond it doesnât.
>
>If in doubt I recommend checking how other drivers are handling
>things. From memory at least i915 uses the above.
I will check how other drivers handle this.
Thanks,
Arindam
>
>Thanks
>Emil
[PATCH] drm/i915: use udelay for very small delays
On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:
> On Thu, Dec 15, 2016 at 10:47:57AM +0200, Jani Nikula wrote:
>> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:
>> > usleep_range() is intended for delays in the 10us to 10ms range that need
>> > good precision. a useleep_range(1, will effectively be no more than an
>> > imprecise udelay with some added cache disruption as it will fire more or
>> > less immediately - use udelay() here.
>> >
>> > Fixes: commit be4fc046bed3 ("drm/i915: add VLV DSI PLL Calculations")
>> > Signed-off-by: Nicholas Mc Guire
>> > ---
>> >
>> > Problem located by coccinelle
>> >
>> > The requirement of waiting at least 0.5 us is assured with the udelay(1)
>> > here which should be more effective than a usleep_range() - would
>> > ndelay(500) make sense here ?
>>
>> This is in the modeset path, i.e. pretty slow anyway. In this case, the
>> point is not to try hard to minimize the wait, the point is to guarantee
>> "at least 0.5 us" has passed. If the CPU can do something else,
>> including dozing off, in the mean time, great. I think we should stick
>> with usleep_range().
>
> well in that case maybe an acceptable solution would be to set it to
> some suitable range 10,20 us ? or if not critical preferably even with a large
> upper limit.
I'd be fine with 10, 50 here. Please do send the patch, Cc: me.
>>
>> I think the question is, how do we express this in code? IMO udelay() is
>> not the answer.
>
> if the delay need to be kept short then no - then its not the answer
> but usleep_ranges(1,2) I think is effectively just an inefficient version
> of udelay(1), by the time the timer is setup and the task gives
> up the cpu the timer would fire.
>
>>
>> And why doesn't usleep_range() kernel-doc mention anything about the
>> ranges?
>>
>
> interesting - that might be part of the reason there are many findings
> Documentation/timers/timers-howto.txt does
>
> SLEEPING FOR ~USECS OR SMALL MSECS ( 10us - 20ms):
> * Use usleep_range
I'd appreciate short additions to the kernel-doc documentation of each
function to document the approximate range it's appropriate for. People
will know where to look if their use doesn't fall in that range.
Thanks,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
[PATCH] drm: Convert all helpers to drm_connector_list_iter
Reviewed-by: Harry Wentland:
On 2016-12-15 10:58 AM, Daniel Vetter wrote:
> Mostly nothing special (except making sure that really all error paths
> and friends call iter_put).
>
> v2: Don't forget the raw connector_list walking in
> drm_helper_move_panel_connectors_to_head. That one unfortunately can't
> be converted to the iterator helpers, but since it's just some list
> splicing best to just wrap the entire thing up in one critical
> section.
>
> v3: Bail out after iter_put (Harry).
>
> Cc: Harry Wentland
> Reviewed-by: Harry Wentland
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_atomic_helper.c | 39
> drivers/gpu/drm/drm_crtc_helper.c| 49
>
> drivers/gpu/drm/drm_fb_helper.c | 12 ++---
> drivers/gpu/drm/drm_modeset_helper.c | 2 ++
> drivers/gpu/drm/drm_plane_helper.c | 5 +++-
> drivers/gpu/drm/drm_probe_helper.c | 18 -
> 6 files changed, 92 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 23767df72615..e2e15a9903a9 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -94,9 +94,10 @@ static int handle_conflicting_encoders(struct
> drm_atomic_state *state,
> {
> struct drm_connector_state *conn_state;
> struct drm_connector *connector;
> + struct drm_connector_list_iter conn_iter;
> struct drm_encoder *encoder;
> unsigned encoder_mask = 0;
> - int i, ret;
> + int i, ret = 0;
>
> /*
>* First loop, find all newly assigned encoders from the connectors
> @@ -144,7 +145,8 @@ static int handle_conflicting_encoders(struct
> drm_atomic_state *state,
>* and the crtc is disabled if no encoder is left. This preserves
>* compatibility with the legacy set_config behavior.
>*/
> - drm_for_each_connector(connector, state->dev) {
> + drm_connector_list_iter_get(state->dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> struct drm_crtc_state *crtc_state;
>
> if (drm_atomic_get_existing_connector_state(state, connector))
> @@ -160,12 +162,15 @@ static int handle_conflicting_encoders(struct
> drm_atomic_state *state,
>connector->state->crtc->base.id,
>connector->state->crtc->name,
>connector->base.id, connector->name);
> - return -EINVAL;
> + ret = -EINVAL;
> + goto out;
> }
>
> conn_state = drm_atomic_get_connector_state(state, connector);
> - if (IS_ERR(conn_state))
> - return PTR_ERR(conn_state);
> + if (IS_ERR(conn_state)) {
> + ret = PTR_ERR(conn_state);
> + goto out;
> + }
>
> DRM_DEBUG_ATOMIC("[ENCODER:%d:%s] in use on [CRTC:%d:%s],
> disabling [CONNECTOR:%d:%s]\n",
>encoder->base.id, encoder->name,
> @@ -176,19 +181,21 @@ static int handle_conflicting_encoders(struct
> drm_atomic_state *state,
>
> ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> if (ret)
> - return ret;
> + goto out;
>
> if (!crtc_state->connector_mask) {
> ret = drm_atomic_set_mode_prop_for_crtc(crtc_state,
> NULL);
> if (ret < 0)
> - return ret;
> + goto out;
>
> crtc_state->active = false;
> }
> }
> +out:
> + drm_connector_list_iter_put(&conn_iter);
>
> - return 0;
> + return ret;
> }
>
> static void
> @@ -2442,6 +2449,7 @@ int drm_atomic_helper_disable_all(struct drm_device
> *dev,
> {
> struct drm_atomic_state *state;
> struct drm_connector *conn;
> + struct drm_connector_list_iter conn_iter;
> int err;
>
> state = drm_atomic_state_alloc(dev);
> @@ -2450,7 +2458,8 @@ int drm_atomic_helper_disable_all(struct drm_device
> *dev,
>
> state->acquire_ctx = ctx;
>
> - drm_for_each_connector(conn, dev) {
> + drm_connector_list_iter_get(dev, &conn_iter);
> + drm_for_each_connector_iter(conn, &conn_iter) {
> struct drm_crtc *crtc = conn->state->crtc;
> struct drm_crtc_state *crtc_state;
>
> @@ -2468,6 +2477,7 @@ int drm_atomic_helper_disable_all(struct drm_device
> *dev,
>
> err = drm_atomic_commit(state);
> free:
> + drm_connector_list_iter_put(&conn_iter);
> drm_atomic_state_put(state);
> return err;
> }
> @@ -2840,6 +2850,7 @@ int drm_atomic_helper_connector_dpms(struct
> drm_connector *connector,
> st
[PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD handles (v3)
On 15 December 2016 at 07:30, Nath, Arindam wrote:
>>-Original Message-
>>From: Emil Velikov [mailto:emil.l.velikov at gmail.com]
>>Sent: Wednesday, December 14, 2016 9:26 PM
>>To: Nath, Arindam
>>Cc: David Airlie; Deucher, Alexander; amd-gfx mailing list; ML dri-devel;
>>Koenig, Christian
>>Subject: Re: [PATCH 1/1] drm/amd/amdgpu: get maximum and used UVD
>>handles (v3)
>>
>>On 12 December 2016 at 18:49, wrote:
>>> From: Arindam Nath
>>>
>>> Change History
>>> --
>>>
>>> v3: changes suggested by Christian
>>> - Add a check for UVD IP block using AMDGPU_HW_IP_UVD
>>> query type.
>>> - Add a check for asic_type to be less than
>>> CHIP_POLARIS10 since starting Polaris, we support
>>> unlimited UVD instances.
>>> - Add kerneldoc style comment for
>>> amdgpu_uvd_used_handles().
>>>
>>> v2: as suggested by Christian
>>> - Add a new query AMDGPU_INFO_NUM_HANDLES
>>> - Create a helper function to return the number
>>> of currently used UVD handles.
>>> - Modify the logic to count the number of used
>>> UVD handles since handles can be freed in
>>> non-linear fashion.
>>>
>>> v1:
>>> - User might want to query the maximum number of UVD
>>> instances supported by firmware. In addition to that,
>>> if there are multiple applications using UVD handles
>>> at the same time, he might also want to query the
>>> currently used number of handles.
>>>
>>> For this we add two variables max_handles and
>>> used_handles inside drm_amdgpu_info_hw_ip. So now
>>> an application (or libdrm) can use AMDGPU_INFO IOCTL
>>> with AMDGPU_INFO_HW_IP_INFO query type to get these
>>> values.
>>>
>>> Signed-off-by: Arindam Nath
>>> Reviewed-by: Christian König
>>> ---
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 21
>>+
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 25
>>+
>>> drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.h | 1 +
>>> include/uapi/drm/amdgpu_drm.h | 9 +
>>> 4 files changed, 56 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> index 174eb59..3273d8c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>>> @@ -570,6 +570,27 @@ static int amdgpu_info_ioctl(struct drm_device
>>*dev, void *data, struct drm_file
>>> return -EINVAL;
>>> }
>>> }
>>> + case AMDGPU_INFO_NUM_HANDLES: {
>>> + struct drm_amdgpu_info_num_handles handle;
>>> +
>>> + switch (info->query_hw_ip.type) {
>>> + case AMDGPU_HW_IP_UVD:
>>> + /* Starting Polaris, we support unlimited UVD
>>> handles */
>>> + if (adev->asic_type < CHIP_POLARIS10) {
>>> + handle.uvd_max_handles =
>>> adev->uvd.max_handles;
>>> + handle.uvd_used_handles =
>>amdgpu_uvd_used_handles(adev);
>>> +
>>> + return copy_to_user(out, &handle,
>>> + min((size_t)size, sizeof(handle)))
>>> ? -EFAULT : 0;
>>> + } else {
>>> + return -EINVAL;
>>Using EINVAL doesn't seem right here. As per man 3 ioctl
>>
>> EINVAL The request or arg argument is not valid for this device.
>>
>>A bit further down you can see the one you want.
>>
>> ENODEV The fildes argument refers to a valid STREAMS device, but
>>the corresponding device driver does not support the ioctl() function.
>
> Emil, ENODEV would mean the driver does not support ioctl() itself. But in
> our case ioctl() is supported.
>
> Since we extract the query type from arg passed to ioctl(), and it is this
> query AMDGPU_INFO_NUM_HANDLES which is not supported by driver (for >
> Polaris), doesnât returning EINVAL make more sense here?
>
Unless I'm reading the code incorrectly - CHIP_POLARIS10 and older do
not have support the query. Thus ENODEV is the one you want to use.
Furthermore EINVAL is (mostly) used to indicate incorrect input
(failed input validation) which userspace uses to check if kernel is
too old/does not support X.
If in doubt I recommend checking how other drivers are handling
things. From memory at least i915 uses the above.
Thanks
Emil
[PATCH 13/34] drm: kselftest for drm_mm and eviction
On ma, 2016-12-12 at 11:53 +, Chris Wilson wrote:
> Check that we add arbitrary blocks to the eviction scanner in order to
> find the first minimal hole that matches our request.
>
> Signed-off-by: Chris Wilson
> + if ((int)tmp.start % n || tmp.size != nsize ||
> tmp.hole_follows) {
> + pr_err("Inserted did not fill the eviction hole:
> size=%lld [%d], align=%d [rem=%d] (prime), start=%llx, hole-follows?=%d\n",
> + Â Â Â Â Â Â Â tmp.size, nsize, n, (int)tmp.start % n,
> tmp.start, tmp.hole_follows);
> +
> + drm_mm_remove_node(&tmp);
> + goto out;
> + }
> +
> + drm_mm_remove_node(&tmp);
> + list_for_each_entry(e, &evict_list, link) {
> + err = drm_mm_reserve_node(&mm, &e->node);
Using helpers, could repeat the ordering tests for reserve vs insert.
Reviewed-by: Joonas Lahtinen
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
[PATCH v2.2 2/4] drm/atomic: Add accessor macros for the current state, v3.
With checks! This will allow safe access to the current state,
while ensuring that the correct locks are held.
Changes since v1:
- Constify returned states.
- Require plane lock to return plane state, don't allow crtc_lock to
be sufficient.
Changes since v2:
- Include drmP.h for drm_device, change the macro
drm_atomic_get_current_connector_state back to a function.
Signed-off-by: Maarten Lankhorst
---
include/drm/drm_atomic.h | 62 ++
include/drm/drm_modeset_lock.h | 9 ++
2 files changed, 71 insertions(+)
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index ca9b32d157bd..63b5fc8e1fdc 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -29,6 +29,7 @@
#define DRM_ATOMIC_H_
#include
+#include
/**
* struct drm_crtc_commit - track modeset commits on a CRTC
@@ -335,6 +336,67 @@ __drm_atomic_get_current_plane_state(struct
drm_atomic_state *state,
return plane->state;
}
+
+/**
+ * drm_atomic_get_current_plane_state - get current plane state
+ * @plane: plane to grab
+ *
+ * This function returns the current plane state for the given plane,
+ * with extra locking checks to make sure that the plane state can be
+ * retrieved safely.
+ *
+ * Returns:
+ *
+ * Pointer to the current plane state.
+ */
+static inline const struct drm_plane_state *
+drm_atomic_get_current_plane_state(struct drm_plane *plane)
+{
+ drm_modeset_lock_assert_held(&plane->mutex);
+
+ return plane->state;
+}
+
+/**
+ * drm_atomic_get_current_crtc_state - get current crtc state
+ * @crtc: crtc to grab
+ *
+ * This function returns the current crtc state for the given crtc,
+ * with extra locking checks to make sure that the crtc state can be
+ * retrieved safely.
+ *
+ * Returns:
+ *
+ * Pointer to the current crtc state.
+ */
+static inline const struct drm_crtc_state *
+drm_atomic_get_current_crtc_state(struct drm_crtc *crtc)
+{
+ drm_modeset_lock_assert_held(&crtc->mutex);
+
+ return crtc->state;
+}
+
+/**
+ * drm_atomic_get_current_connector_state - get current connector state
+ * @connector: connector to grab
+ *
+ * This function returns the current connector state for the given connector,
+ * with extra locking checks to make sure that the connector state can be
+ * retrieved safely.
+ *
+ * Returns:
+ *
+ * Pointer to the current connector state.
+ */
+static inline const struct drm_connector_state *
+drm_atomic_get_current_connector_state(struct drm_connector *connector)
+{
+
drm_modeset_lock_assert_held(&connector->dev->mode_config.connection_mutex);
+
+ return connector->state;
+}
+
int __must_check
drm_atomic_set_mode_for_crtc(struct drm_crtc_state *state,
struct drm_display_mode *mode);
diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h
index d918ce45ec2c..3ca4ee838b07 100644
--- a/include/drm/drm_modeset_lock.h
+++ b/include/drm/drm_modeset_lock.h
@@ -109,6 +109,15 @@ static inline bool drm_modeset_is_locked(struct
drm_modeset_lock *lock)
return ww_mutex_is_locked(&lock->mutex);
}
+static inline void drm_modeset_lock_assert_held(struct drm_modeset_lock *lock)
+{
+#ifdef CONFIG_LOCKDEP
+ lockdep_assert_held(&lock->mutex.base);
+#else
+ WARN_ON(!drm_modeset_is_locked(lock));
+#endif
+}
+
int drm_modeset_lock(struct drm_modeset_lock *lock,
struct drm_modeset_acquire_ctx *ctx);
int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock,
--
2.7.4
[PATCH] kref: prefer atomic_inc_not_zero to atomic_add_unless
On Thu, Dec 15, 2016 at 07:55:54PM +0100, Jason A. Donenfeld wrote: > On most platforms, there exists this ifdef: > > #define atomic_inc_not_zero(v) atomic_add_unless((v), 1, 0) > > This makes this patch functionally useless. However, on PPC, there is > actually an explicit definition of atomic_inc_not_zero with its own > assembly that is slightly more optimized than atomic_add_unless. So, > this patch changes kref to use atomic_inc_not_zero instead, for PPC and > any future platforms that might provide an explicit implementation. > > This also puts this usage of kref more in line with a verbatim reading > of the examples in Paul McKenney's paper [1] in the section titled "2.4 > Atomic Counting With Check and Release Memory Barrier", which uses > atomic_inc_not_zero. > > [1] http://open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2167.pdf > > Signed-off-by: Jason A. Donenfeld > Reviewed-by: Thomas Hellstrom > Reviewed-by: Christoph Hellwig > --- > Sorry to submit this again, but people keep reviewing it saying it's fine, > but then point to somebody else to actually merge this. At the end of the > chain of fingerpointing is usually Greg. "Just have Greg do it." At this > point I'm confused, but it's certainly been sufficiently reviewed and > accepted. So can one of you just respond saying "I'll take it!" Well, the crazies over in drm land were the ones that merged this new api, so they should be the ones responsible for it. But that was way back in 2012, odds are they don't remember it given the lunacy that is their subsystem... I'll take it after 4.10-rc1 is out, thanks. greg k-h
[PATCH] drm/i915: use udelay for very short delays
On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > Even on fast systems a 2 microsecond delay is most likely more efficient > as a busy-wait loop. The overhead of a hrtimer does not seem warranted - > change this to a udelay(2). Similar concerns as in [1]. We don't need the accuracy of udelay() here, so this boils down to which is the better use of CPU. We could probably relax the max delay more if that was helpful. But I'm not immediately sold on "is most likely more efficient" which sounds like a gut feeling. I'm sorry it's not clear in my other reply that I do appreciate addressing incorrect/silly use of usleep_range(); I'm just not (yet) convinced udelay() is the answer. BR, Jani. [1] http://lkml.kernel.org/r/8737hpr32a.fsf at intel.com > > Signed-off-by: Nicholas Mc Guire > --- > > Problem found by coccinelle: > > Patch was compile tested with: x86_64_defconfig (implies CONFIG_DRM_I915) > > Patch is against 4.9.0 (localversion-next is next-20161214) > > drivers/gpu/drm/i915/intel_dsi.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c > b/drivers/gpu/drm/i915/intel_dsi.c > index 5b72c50..19fe86b 100644 > --- a/drivers/gpu/drm/i915/intel_dsi.c > +++ b/drivers/gpu/drm/i915/intel_dsi.c > @@ -379,7 +379,7 @@ static void bxt_dsi_device_ready(struct intel_encoder > *encoder) > val &= ~ULPS_STATE_MASK; > val |= (ULPS_STATE_ENTER | DEVICE_READY); > I915_WRITE(MIPI_DEVICE_READY(port), val); > - usleep_range(2, 3); > + udelay(2); > > /* 3. Exit ULPS */ > val = I915_READ(MIPI_DEVICE_READY(port)); -- Jani Nikula, Intel Open Source Technology Center
[PATCH v4 06/22] drm: omapdrm: Handle FIFO underflow IRQs internally
On 14/12/16 13:48, Laurent Pinchart wrote:
> Hi Tomi,
>
> On Wednesday 14 Dec 2016 12:22:29 Tomi Valkeinen wrote:
>> On 14/12/16 02:27, Laurent Pinchart wrote:
>>> As the FIFO underflow IRQ handler just prints an error message to the
>>> kernel log, simplify the code by not registering one IRQ handler per
>>> plane but print the messages directly from the main IRQ handler.
>>>
>>> Signed-off-by: Laurent Pinchart
>>> Reviewed-by: Tomi Valkeinen
>>> ---
>>>
>>> +static void omap_irq_fifo_underflow(struct omap_drm_private *priv,
>>> + u32 irqstatus)
>>> +{
>>> + static DEFINE_RATELIMIT_STATE(_rs, DEFAULT_RATELIMIT_INTERVAL,
>>> + DEFAULT_RATELIMIT_BURST);
>>> + static const struct {
>>> + const char *name;
>>> + u32 mask;
>>> + } sources[] = {
>>> + { "gfx", DISPC_IRQ_GFX_FIFO_UNDERFLOW },
>>> + { "vid1", DISPC_IRQ_VID1_FIFO_UNDERFLOW },
>>> + { "vid2", DISPC_IRQ_VID2_FIFO_UNDERFLOW },
>>> + { "vid3", DISPC_IRQ_VID3_FIFO_UNDERFLOW },
>>> + };
>>> +
>>> + const u32 mask = DISPC_IRQ_GFX_FIFO_UNDERFLOW
>>> + | DISPC_IRQ_VID1_FIFO_UNDERFLOW
>>> + | DISPC_IRQ_VID2_FIFO_UNDERFLOW
>>> + | DISPC_IRQ_VID3_FIFO_UNDERFLOW;
>>> + unsigned int i;
>>> +
>>> + spin_lock(&list_lock);
>>> + irqstatus &= priv->irq_mask & mask;
>>> + spin_unlock(&list_lock);
>>> +
>>> + if (!irqstatus)
>>> + return;
>>
>> This is called every time we get any DSS interrupt, so I think it would
>> be good to have a fast-path here without the lock: irqstatus & mask.
>>
>> Or maybe store the enabled underflow irq bits separately from irq_mask,
>> as the underflow bits are never changed after the initial setup, and
>> then there's no need for locking.
>
> I'd prefer going for the former, but I'm a bit concerned that an IRQ bit
> defined as FIFO overflow on one platform could be defined as something else
> on
> another platform and be mistaken.
>
> Given that we already take the same lock in the IRQ handler to call the wait
> handlers, do you think this is really an issue ?
Yep, I think it's fine for the time being.
Reviewed-by: Tomi Valkeinen
Tomi
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL:
<https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/f0fa6071/attachment.sig>
[PATCH 12/34] drm: kselftest for drm_mm and alignment
On ma, 2016-12-12 at 11:53 +, Chris Wilson wrote:
> Check that we can request alignment to any power-of-two or prime using a
> plain drm_mm_node_insert(), and also handle a reasonable selection of
> primes.
>
> Signed-off-by: Chris Wilson
> +static int igt_align(void *ignored)
> +{
> + struct drm_mm mm;
> + struct drm_mm_node *node, *next;
> + int ret = -EINVAL;
> + int prime;
> +
> + drm_mm_init(&mm, 1, U64_MAX - 1);
> +
> + drm_for_each_prime(prime, 8192) {
> + u64 size;
> + int err;
> +
> + node = kzalloc(sizeof(*node), GFP_KERNEL);
> + if (!node) {
> + ret = -ENOMEM;
> + goto out;
> + }
If the amount of primes would be predictable (pun intended), we could
malloc the nodes at one go.
Other than that, looks like I reviewed this already;
Reviewed-by: Joonas Lahtinen
Regards, Joonas
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
[PATCH] drm/i915: use udelay for very small delays
On Thu, 15 Dec 2016, Jani Nikula wrote:
> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:
>> usleep_range() is intended for delays in the 10us to 10ms range that need
>> good precision. a useleep_range(1, will effectively be no more than an
>> imprecise udelay with some added cache disruption as it will fire more or
>> less immediately - use udelay() here.
>>
>> Fixes: commit be4fc046bed3 ("drm/i915: add VLV DSI PLL Calculations")
>> Signed-off-by: Nicholas Mc Guire
>> ---
>>
>> Problem located by coccinelle
>>
>> The requirement of waiting at least 0.5 us is assured with the udelay(1)
>> here which should be more effective than a usleep_range() - would
>> ndelay(500) make sense here ?
>
> This is in the modeset path, i.e. pretty slow anyway. In this case, the
> point is not to try hard to minimize the wait, the point is to guarantee
> "at least 0.5 us" has passed. If the CPU can do something else,
> including dozing off, in the mean time, great. I think we should stick
> with usleep_range().
>
> I think the question is, how do we express this in code? IMO udelay() is
> not the answer.
>
> And why doesn't usleep_range() kernel-doc mention anything about the
> ranges?
>
>
> BR,
> Jani.
>
>
>>
>> Patch was compile tested with: x86_64_defconfig (implies CONFIG_DRM_I915)
>>
>> Patch is against 4.9.0 (localvrsion-next is next-20161214)
>>
>> drivers/gpu/drm/i915/intel_dsi_pll.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c
>> b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> index 56eff60..0ec040e 100644
>> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
>> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
>> @@ -157,7 +157,7 @@ static void vlv_enable_dsi_pll(struct intel_encoder
>> *encoder,
>>config->dsi_pll.ctrl & ~DSI_PLL_VCO_EN);
>>
>> /* wait at least 0.5 us after ungating before enabling VCO */
>> -usleep_range(1, 10);
>> +udelay(1);
>>
>> vlv_cck_write(dev_priv, CCK_REG_DSI_PLL_CONTROL, config->dsi_pll.ctrl);
PS. This vlv_cck_write() call will do sideband communication with
millisecond range timeouts.
--
Jani Nikula, Intel Open Source Technology Center
[PATCH 1/2] drm/panel: Add support for S6E3HA2 panel driver on TM2 board
On 12/14/2016 11:14 PM, Andrzej Hajda wrote: > On 14.12.2016 07:04, Hoegeun Kwon wrote: >> This patch add support for MIPI-DSI based S6E3HA2 AMOLED panel >> driver. This panel has 1440x2560 resolution in 5.7-inch physical >> panel in the TM2 device. >> >> Signed-off-by: Donghwa Lee >> Signed-off-by: Hyungwon Hwang >> Signed-off-by: Hoegeun Kwon > Hi Hoegeun Kwon, > > Is there a reason to upstream obsolete driver? Current version of the > driver is available at [1]. It differs significantly and contains > multiple fixes and improvements. I guess it still requires some > polishing but it is better base for start. > > [1]: > https://review.tizen.org/git/?p=platform/kernel/linux-exynos.git;a=blob;f=drivers/gpu/drm/panel/panel-s6e3ha2.c;hb=refs/heads/tizen > > Regards > Andrzej Hi Andrzej Hajda, The current driver focus on basic functionality. Therefore, VR and HMT modes in [1] have been removed from the patch. VR and HMT modes are added after the basic functions are reflected. Best Regards, Hoegeun Kwon > ___ > dri-devel mailing list > dri-devel at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
[Intel-gfx] [PATCH] drm/i915: use udelay for very short delays
On Thu, Dec 15, 2016 at 10:27:59AM +0100, Daniel Vetter wrote: > On Thu, Dec 15, 2016 at 10:25:19AM +0100, Daniel Vetter wrote: > > On Thu, Dec 15, 2016 at 11:08:49AM +0200, Jani Nikula wrote: > > > On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > > > > Even on fast systems a 2 microsecond delay is most likely more efficient > > > > as a busy-wait loop. The overhead of a hrtimer does not seem warranted - > > > > change this to a udelay(2). > > > > > > Similar concerns as in [1]. We don't need the accuracy of udelay() here, > > > so this boils down to which is the better use of CPU. We could probably > > > relax the max delay more if that was helpful. But I'm not immediately > > > sold on "is most likely more efficient" which sounds like a gut feeling. > > > > > > I'm sorry it's not clear in my other reply that I do appreciate > > > addressing incorrect/silly use of usleep_range(); I'm just not (yet) > > > convinced udelay() is the answer. > > > > So one reason why we unconditionally use *sleep variants is the > > might_sleep check. Because in the past people have used udelay and mdelay, > > those delays had to be increased a lot because hw, and at the same time > > someone added users of these functions to our irq helper, resulting in irq > > handling times measures in multiple ms. That's not good. > > > > So until someone can demonstrate that there's a real benefit (which let's > > be honest, for modeset code, will never be the case) I very highly prefer > > to use *sleep* functions. They prevent some silly things from happening by > > accident. Micro-optimizing modeset code and hampering maitainability in > > the process is not good. > > Also, the entire premise seems backwards: usleep_range is inefficient for > certain parameter ranges and better replaced with udelay. That makes > sense. > > But why exactly do we not fix udelay_range then, but instead do a cocci > job crawling through all the thousands of callers? Just fix usleep(_range) > to use udelay for very small values (and still keep the might_sleep check > ofc) if that's more efficient! its actually not thousands more like a few dozen: usleep_range(min,max) in linux-stable 4.9.0 1648 calls total 1488 pass numeric values only (90.29%) 27 min below 10us (1.81%) 40 min above 10ms (2.68%) min out of spec 4.50% 76 preprocessor constants (4.61%) 1 min below 10us (1.31%) 8 min above 10ms (10.52%) min out of spec 11.84% 85 expressions (5.15%) 1(0) min below 10us (1.50%)* 6(2) min above 10ms (7.50%)* min out of spec 9.0% Errors: 23 where min==max (1.39%) 0 where max < min (0.00%) Total: Bugs: 6.48%-10.70%* Crit: 3.09%-3.15%* (min < 10 and min==max and max < min) Detectable by coccinelle: Bugs: 74/103 (71.8%) Crit: 50/52 (96.1%) *based on estimats as runtime values not known. there is no usleep() as noted in Documentation/timers/timers-howto.txt - Why not usleep? On slower systems, (embedded, OR perhaps a speed- stepped PC!) the overhead of setting up the hrtimers for usleep *may* not be worth it. Such an evaluation will obviously depend on your specific situation, but it is something to be aware of. and it suggests to use different API for different ranges - sounds sane to me and seems to cover the needs of drivers. thx! hofrat
[PATCH] drm/i915: use udelay for very small delays
On Thu, 15 Dec 2016, Nicholas Mc Guire wrote:
> usleep_range() is intended for delays in the 10us to 10ms range that need
> good precision. a useleep_range(1, will effectively be no more than an
> imprecise udelay with some added cache disruption as it will fire more or
> less immediately - use udelay() here.
>
> Fixes: commit be4fc046bed3 ("drm/i915: add VLV DSI PLL Calculations")
> Signed-off-by: Nicholas Mc Guire
> ---
>
> Problem located by coccinelle
>
> The requirement of waiting at least 0.5 us is assured with the udelay(1)
> here which should be more effective than a usleep_range() - would
> ndelay(500) make sense here ?
This is in the modeset path, i.e. pretty slow anyway. In this case, the
point is not to try hard to minimize the wait, the point is to guarantee
"at least 0.5 us" has passed. If the CPU can do something else,
including dozing off, in the mean time, great. I think we should stick
with usleep_range().
I think the question is, how do we express this in code? IMO udelay() is
not the answer.
And why doesn't usleep_range() kernel-doc mention anything about the
ranges?
BR,
Jani.
>
> Patch was compile tested with: x86_64_defconfig (implies CONFIG_DRM_I915)
>
> Patch is against 4.9.0 (localvrsion-next is next-20161214)
>
> drivers/gpu/drm/i915/intel_dsi_pll.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dsi_pll.c
> b/drivers/gpu/drm/i915/intel_dsi_pll.c
> index 56eff60..0ec040e 100644
> --- a/drivers/gpu/drm/i915/intel_dsi_pll.c
> +++ b/drivers/gpu/drm/i915/intel_dsi_pll.c
> @@ -157,7 +157,7 @@ static void vlv_enable_dsi_pll(struct intel_encoder
> *encoder,
> config->dsi_pll.ctrl & ~DSI_PLL_VCO_EN);
>
> /* wait at least 0.5 us after ungating before enabling VCO */
> - usleep_range(1, 10);
> + udelay(1);
>
> vlv_cck_write(dev_priv, CCK_REG_DSI_PLL_CONTROL, config->dsi_pll.ctrl);
--
Jani Nikula, Intel Open Source Technology Center
[PATCH 07/13] drm: Clean up connectors by unreferencing them
On 2016-12-13 06:08 PM, Daniel Vetter wrote:
> Only static connectors should be left at this point, and we should be
> able to clean them out by simply dropping that last reference still
> around from drm_connector_init.
>
> If that leaves anything behind then we have a driver bug.
>
> Doing the final cleanup this way also allows us to use
> drm_connector_iter, removing the very last place where we walk
> connector_list explicitly in drm core&helpers.
>
> Signed-off-by: Daniel Vetter
> ---
> drivers/gpu/drm/drm_mode_config.c | 15 +++
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_mode_config.c
> b/drivers/gpu/drm/drm_mode_config.c
> index 747a26df0e90..a942536abd60 100644
> --- a/drivers/gpu/drm/drm_mode_config.c
> +++ b/drivers/gpu/drm/drm_mode_config.c
> @@ -397,7 +397,8 @@ EXPORT_SYMBOL(drm_mode_config_init);
> */
> void drm_mode_config_cleanup(struct drm_device *dev)
> {
> - struct drm_connector *connector, *ot;
> + struct drm_connector *connector;
> + struct drm_connector_list_iter conn_iter;
> struct drm_crtc *crtc, *ct;
> struct drm_encoder *encoder, *enct;
> struct drm_framebuffer *fb, *fbt;
> @@ -410,10 +411,16 @@ void drm_mode_config_cleanup(struct drm_device *dev)
> encoder->funcs->destroy(encoder);
> }
>
> - list_for_each_entry_safe(connector, ot,
> - &dev->mode_config.connector_list, head) {
> - connector->funcs->destroy(connector);
> + drm_connector_list_iter_get(dev, &conn_iter);
> + drm_for_each_connector_iter(connector, &conn_iter) {
> + /* drm_connector_list_iter holds an full reference to the
> + * current connector itself, which means it is inherently safe
> + * against unreferencing the current connector - but not against
> + * deleting it right away. */
> + drm_connector_unreference(connector);
> }
> + drm_connector_list_iter_put(&conn_iter);
> + WARN_ON(!list_empty(&dev->mode_config.connector_list));
>
> list_for_each_entry_safe(property, pt, &dev->mode_config.property_list,
>head) {
>
Reviewed-by: Harry Wentland
Harry
[PATCH 11/34] drm: kselftest for drm_mm_insert_node_in_range()
On ma, 2016-12-12 at 11:53 +, Chris Wilson wrote: > Exercise drm_mm_insert_node_in_range(), check that we only allocate from > the specified range. > > Signed-off-by: Chris Wilson With the stylistic changes I proposed to whole series; Reviewed-by: Joonas Lahtinen Regards, Joonas -- Joonas Lahtinen Open Source Technology Center Intel Corporation
[PATCH 12/23] drm: omapdrm: plane: update fifo size on atomic update
On 14/12/16 23:36, Laurent Pinchart wrote: >> I guess a first step is Peter Ujfalusi's series: >> https://lkml.org/lkml/2016/9/1/267 > > Peter, do you plan to respin that patch series ? That series has been merged already. Tomi -- next part -- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 819 bytes Desc: OpenPGP digital signature URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/9979dc4b/attachment-0001.sig>
[Bug 96600] [RV630]: a lot of artifacts appears on a screen playing some videos through VDPAU with hardware acceleration
https://bugs.freedesktop.org/show_bug.cgi?id=96600 --- Comment #11 from russianneuromancer at ya.ru --- Hi Any progress on this? -- You are receiving this mail because: You are the assignee for the bug. -- next part -- An HTML attachment was scrubbed... URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20161215/5d6ac1fb/attachment.html>
[PATCH] drm/i915: use udelay for very short delays
On Thu, Dec 15, 2016 at 12:20:01PM +0200, Jani Nikula wrote: > On Thu, 15 Dec 2016, Ville Syrjälä wrote: > > On Thu, Dec 15, 2016 at 11:52:34AM +0200, Jani Nikula wrote: > >> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > >> > On Thu, Dec 15, 2016 at 11:08:49AM +0200, Jani Nikula wrote: > >> >> On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > >> >> > Even on fast systems a 2 microsecond delay is most likely more > >> >> > efficient > >> >> > as a busy-wait loop. The overhead of a hrtimer does not seem > >> >> > warranted - > >> >> > change this to a udelay(2). > >> >> > >> >> Similar concerns as in [1]. We don't need the accuracy of udelay() here, > >> >> so this boils down to which is the better use of CPU. We could probably > >> >> relax the max delay more if that was helpful. But I'm not immediately > >> >> sold on "is most likely more efficient" which sounds like a gut feeling. > >> >> > >> >> I'm sorry it's not clear in my other reply that I do appreciate > >> >> addressing incorrect/silly use of usleep_range(); I'm just not (yet) > >> >> convinced udelay() is the answer. > >> > > >> > if the delay is not critical and all that is needed > >> > is an assurance that it is greater than X us then > >> > usleep_range is fine with a relaxed limit. > >> > So from what you wrote my patch proposal is wrong - the > >> > udelay() is not the way to got. > >> > My intent is to get rid of very small usleep_range() cases > >> > so if usleep_range(20,50) causes no issues with this driver > >> > and does not induce any performance penalty then that would > >> > be the way to go I think. > >> > >> Okay, so I looked at the code, and I looked at our spec, and I looked at > >> the MIPI D-PHY spec, and I cried a little. > >> > >> Long story short, I think usleep_range(10, 50) will be fine. > > > > Note that I really want to see a comment next to every delay like this > > documenting the actual hardware requirement, if the delay used by the > > code doesn't 100% match it. > > Our spec says, "Wait for 2us for ULPS to complete". That's a simplistic > view wrt D-PHY, and our code doesn't even match the spec. Hence the > tears. Want to propose a wording for the comment so we can apply this > change, without going for a full rewrite of the sequence? > is that suitable or am I overdoing it ? - usleep_range(2, 3); + /* delay for at least 2us - relaxed to 10-50 to allow +* hrtimer subsystem to optimize uncritical timer handling +*/ + usleep_range(10, 50); thx! hofrat
[Intel-gfx] [PATCH] drm/i915: use udelay for very short delays
On Thu, Dec 15, 2016 at 10:25:19AM +0100, Daniel Vetter wrote: > On Thu, Dec 15, 2016 at 11:08:49AM +0200, Jani Nikula wrote: > > On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > > > Even on fast systems a 2 microsecond delay is most likely more efficient > > > as a busy-wait loop. The overhead of a hrtimer does not seem warranted - > > > change this to a udelay(2). > > > > Similar concerns as in [1]. We don't need the accuracy of udelay() here, > > so this boils down to which is the better use of CPU. We could probably > > relax the max delay more if that was helpful. But I'm not immediately > > sold on "is most likely more efficient" which sounds like a gut feeling. > > > > I'm sorry it's not clear in my other reply that I do appreciate > > addressing incorrect/silly use of usleep_range(); I'm just not (yet) > > convinced udelay() is the answer. > > So one reason why we unconditionally use *sleep variants is the > might_sleep check. Because in the past people have used udelay and mdelay, > those delays had to be increased a lot because hw, and at the same time > someone added users of these functions to our irq helper, resulting in irq > handling times measures in multiple ms. That's not good. > > So until someone can demonstrate that there's a real benefit (which let's > be honest, for modeset code, will never be the case) I very highly prefer > to use *sleep* functions. They prevent some silly things from happening by > accident. Micro-optimizing modeset code and hampering maitainability in > the process is not good. Also, the entire premise seems backwards: usleep_range is inefficient for certain parameter ranges and better replaced with udelay. That makes sense. But why exactly do we not fix udelay_range then, but instead do a cocci job crawling through all the thousands of callers? Just fix usleep(_range) to use udelay for very small values (and still keep the might_sleep check ofc) if that's more efficient! -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
[Intel-gfx] [PATCH] drm/i915: use udelay for very short delays
On Thu, Dec 15, 2016 at 11:08:49AM +0200, Jani Nikula wrote: > On Thu, 15 Dec 2016, Nicholas Mc Guire wrote: > > Even on fast systems a 2 microsecond delay is most likely more efficient > > as a busy-wait loop. The overhead of a hrtimer does not seem warranted - > > change this to a udelay(2). > > Similar concerns as in [1]. We don't need the accuracy of udelay() here, > so this boils down to which is the better use of CPU. We could probably > relax the max delay more if that was helpful. But I'm not immediately > sold on "is most likely more efficient" which sounds like a gut feeling. > > I'm sorry it's not clear in my other reply that I do appreciate > addressing incorrect/silly use of usleep_range(); I'm just not (yet) > convinced udelay() is the answer. So one reason why we unconditionally use *sleep variants is the might_sleep check. Because in the past people have used udelay and mdelay, those delays had to be increased a lot because hw, and at the same time someone added users of these functions to our irq helper, resulting in irq handling times measures in multiple ms. That's not good. So until someone can demonstrate that there's a real benefit (which let's be honest, for modeset code, will never be the case) I very highly prefer to use *sleep* functions. They prevent some silly things from happening by accident. Micro-optimizing modeset code and hampering maitainability in the process is not good. -Daniel > > BR, > Jani. > > > [1] http://lkml.kernel.org/r/8737hpr32a.fsf at intel.com > > > > > > Signed-off-by: Nicholas Mc Guire > > --- > > > > Problem found by coccinelle: > > > > Patch was compile tested with: x86_64_defconfig (implies CONFIG_DRM_I915) > > > > Patch is against 4.9.0 (localversion-next is next-20161214) > > > > drivers/gpu/drm/i915/intel_dsi.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dsi.c > > b/drivers/gpu/drm/i915/intel_dsi.c > > index 5b72c50..19fe86b 100644 > > --- a/drivers/gpu/drm/i915/intel_dsi.c > > +++ b/drivers/gpu/drm/i915/intel_dsi.c > > @@ -379,7 +379,7 @@ static void bxt_dsi_device_ready(struct intel_encoder > > *encoder) > > val &= ~ULPS_STATE_MASK; > > val |= (ULPS_STATE_ENTER | DEVICE_READY); > > I915_WRITE(MIPI_DEVICE_READY(port), val); > > - usleep_range(2, 3); > > + udelay(2); > > > > /* 3. Exit ULPS */ > > val = I915_READ(MIPI_DEVICE_READY(port)); > > -- > Jani Nikula, Intel Open Source Technology Center > ___ > Intel-gfx mailing list > Intel-gfx at lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch
