Re: [Intel-gfx] Critical regression in 4.7-rcX
[ Adding the proper people to the cc. ]
On Thu, 14 Jul 2016, Larry Finger wrote:
>
> To anyone keeping track of regressions in kernel 4.7, I call your attention to
> https://bugs.freedesktop.org/show_bug.cgi?id=96675.
>
> This bug causes driver i915 to fail to connect to the display, and results in
> a blank screen as soon as the kernel is loaded. The only way to operate with
> kernel 4.7 is to add "nomodeset" to the command line. The problem was bisected
> to commit f21a21983ef13a ("drm/i915: Splitting intel_dp_detect").
Daniel? Jani? Time to revert?
Linus
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] linux-next: build failure after merge of the drm tree
On 7/15/16, Stephen Rothwell wrote:
> Hi Dave,
>
> After merging the drm tree, today's linux-next build (x86_64 allmodconfig)
> failed like this:
>
> In file included from drivers/gpu/drm/i915/intel_opregion.c:34:0:
> drivers/gpu/drm/i915/intel_opregion.c: In function
> 'intel_opregion_get_panel_type':
> drivers/gpu/drm/i915/intel_opregion.c:1042:17: error: 'dev' undeclared
> (first use in this function)
> if (IS_SKYLAKE(dev)) {
> ^
> drivers/gpu/drm/i915/i915_drv.h:2603:43: note: in definition of macro
> '__I915__'
> if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
>^
> drivers/gpu/drm/i915/i915_drv.h:2670:26: note: in expansion of macro
> 'INTEL_INFO'
> #define IS_SKYLAKE(dev) (INTEL_INFO(dev)->is_skylake)
> ^
> drivers/gpu/drm/i915/intel_opregion.c:1042:6: note: in expansion of macro
> 'IS_SKYLAKE'
> if (IS_SKYLAKE(dev)) {
> ^
> drivers/gpu/drm/i915/intel_opregion.c:1042:17: note: each undeclared
> identifier is reported only once for each function it appears in
> if (IS_SKYLAKE(dev)) {
> ^
> drivers/gpu/drm/i915/i915_drv.h:2603:43: note: in definition of macro
> '__I915__'
> if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
>^
> drivers/gpu/drm/i915/i915_drv.h:2670:26: note: in expansion of macro
> 'INTEL_INFO'
> #define IS_SKYLAKE(dev) (INTEL_INFO(dev)->is_skylake)
> ^
> drivers/gpu/drm/i915/intel_opregion.c:1042:6: note: in expansion of macro
> 'IS_SKYLAKE'
> if (IS_SKYLAKE(dev)) {
> ^
>
> Caused by commit
>
> 6f9f4b7a2cf7 ("drm/i915/opregion: Convert to using native
> drm_i915_private")
>
> interacting with commit
>
> aeddda06c1a7 ("drm/i915: Ignore panel type from OpRegion on SKL")
>
> from the drm-intel-fixes tree.
>
> I applied this merge fix patch:
>
> From: Stephen Rothwell
> Date: Fri, 15 Jul 2016 13:34:05 +1000
> Subject: [PATCH] drm/i915/opregion: fix up for argument change
>
> Signed-off-by: Stephen Rothwell
> ---
> drivers/gpu/drm/i915/intel_opregion.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c
> b/drivers/gpu/drm/i915/intel_opregion.c
> index 21f19641e33e..dcdb346b596c 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -1039,7 +1039,7 @@ intel_opregion_get_panel_type(struct drm_i915_private
> *dev_priv)
>* vswing instead. Low vswing results in some display flickers, so
>* let's simply ignore the OpRegion panel type on SKL for now.
>*/
> - if (IS_SKYLAKE(dev)) {
> + if (IS_SKYLAKE(dev_priv)) {
> DRM_DEBUG_KMS("Ignoring OpRegion panel type (%d)\n", ret - 1);
> return -ENODEV;
> }
> --
> 2.8.1
>
I fell over the same issue when testing drm-intel-nightly and sent a patch.
- Sedat -
https://lists.freedesktop.org/archives/intel-gfx/2016-July/100691.html
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] linux-next: build failure after merge of the drm tree
Hi Dave,
After merging the drm tree, today's linux-next build (x86_64 allmodconfig)
failed like this:
In file included from drivers/gpu/drm/i915/intel_opregion.c:34:0:
drivers/gpu/drm/i915/intel_opregion.c: In function
'intel_opregion_get_panel_type':
drivers/gpu/drm/i915/intel_opregion.c:1042:17: error: 'dev' undeclared (first
use in this function)
if (IS_SKYLAKE(dev)) {
^
drivers/gpu/drm/i915/i915_drv.h:2603:43: note: in definition of macro '__I915__'
if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
^
drivers/gpu/drm/i915/i915_drv.h:2670:26: note: in expansion of macro
'INTEL_INFO'
#define IS_SKYLAKE(dev) (INTEL_INFO(dev)->is_skylake)
^
drivers/gpu/drm/i915/intel_opregion.c:1042:6: note: in expansion of macro
'IS_SKYLAKE'
if (IS_SKYLAKE(dev)) {
^
drivers/gpu/drm/i915/intel_opregion.c:1042:17: note: each undeclared identifier
is reported only once for each function it appears in
if (IS_SKYLAKE(dev)) {
^
drivers/gpu/drm/i915/i915_drv.h:2603:43: note: in definition of macro '__I915__'
if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
^
drivers/gpu/drm/i915/i915_drv.h:2670:26: note: in expansion of macro
'INTEL_INFO'
#define IS_SKYLAKE(dev) (INTEL_INFO(dev)->is_skylake)
^
drivers/gpu/drm/i915/intel_opregion.c:1042:6: note: in expansion of macro
'IS_SKYLAKE'
if (IS_SKYLAKE(dev)) {
^
Caused by commit
6f9f4b7a2cf7 ("drm/i915/opregion: Convert to using native drm_i915_private")
interacting with commit
aeddda06c1a7 ("drm/i915: Ignore panel type from OpRegion on SKL")
from the drm-intel-fixes tree.
I applied this merge fix patch:
From: Stephen Rothwell
Date: Fri, 15 Jul 2016 13:34:05 +1000
Subject: [PATCH] drm/i915/opregion: fix up for argument change
Signed-off-by: Stephen Rothwell
---
drivers/gpu/drm/i915/intel_opregion.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_opregion.c
b/drivers/gpu/drm/i915/intel_opregion.c
index 21f19641e33e..dcdb346b596c 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -1039,7 +1039,7 @@ intel_opregion_get_panel_type(struct drm_i915_private
*dev_priv)
* vswing instead. Low vswing results in some display flickers, so
* let's simply ignore the OpRegion panel type on SKL for now.
*/
- if (IS_SKYLAKE(dev)) {
+ if (IS_SKYLAKE(dev_priv)) {
DRM_DEBUG_KMS("Ignoring OpRegion panel type (%d)\n", ret - 1);
return -ENODEV;
}
--
2.8.1
--
Cheers,
Stephen Rothwell
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Acquire intel_runtime_pm for HD-Audio registers
Hi Wilson, > -Original Message- > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] > Sent: Thursday, July 14, 2016 10:34 PM > To: Mika Kuoppala ; intel- > g...@lists.freedesktop.org; Takashi Iwai ; Yang, Libin > > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Acquire intel_runtime_pm for HD- > Audio registers > > On Wed, Jul 13, 2016 at 05:01:02PM +0300, Marius Vlad wrote: > > Did try when you submitted the patch...but can't seem to replicate > > with latest nightly on other SKLs, and currently do not have access on > > the machine that caused it. > > So fwiw, Hans de Geode confirmed that only reverting 03b135cebc47 (which > enables the display powerwell as well) was enough. > > Libin, do you want to handle the revert or explain the mistake in What's the problem with the patch? I can find the details from the email thread. HSW/BSW need acquire the power because it is inside the gpu. So we limited need i915 power for controller to HSW/BDW. > > commit 03b135cebc47d75ea2dc346770374ab741966955 > Author: Libin Yang > Date: Wed Jun 3 09:30:15 2015 +0800 > > ALSA: hda - remove controller dependency on i915 power well for SKL > > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH d-i-n] drm/i915: Fix build failure in intel_opregion_get_panel_type()
Tested against drm-intel-nightly (2016y-07m-14d-20h-15m-29s UTC).
Fixes: aeddda06c1a704bb9 ("drm/i915: Ignore panel type from OpRegion on SKL")
CC: intel-gfx@lists.freedesktop.org
CC: Ville Syrjälä
CC: Daniel Vetter
Signed-off-by: Sedat Dilek
---
drivers/gpu/drm/i915/intel_opregion.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_opregion.c
b/drivers/gpu/drm/i915/intel_opregion.c
index c804630d10d2..adca262d591a 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -1078,7 +1078,7 @@ intel_opregion_get_panel_type(struct drm_i915_private
*dev_priv)
* vswing instead. Low vswing results in some display flickers, so
* let's simply ignore the OpRegion panel type on SKL for now.
*/
- if (IS_SKYLAKE(dev)) {
+ if (IS_SKYLAKE(dev_priv)) {
DRM_DEBUG_KMS("Ignoring OpRegion panel type (%d)\n", ret - 1);
return -ENODEV;
}
--
2.9.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Bump libdrm-intel dependency library to a newer version that support softpin.
On Thu, Jul 14, 2016 at 09:52:24PM +0100, Emil Velikov wrote: > On 14 July 2016 at 21:03, Chris Wilson wrote: > > On Thu, Jul 14, 2016 at 07:54:59PM +0100, Emil Velikov wrote: > >> On 14 July 2016 at 17:49, Chris Wilson wrote: > >> > On Thu, Jul 14, 2016 at 05:29:55PM +0100, Emil Velikov wrote: > >> >> Hi Marius, > >> >> > >> >> Just double-checking - this is an i-g-t patch, isn't it ? > >> >> > >> >> On 14 July 2016 at 11:39, Marius Vlad wrote: > >> >> > Required by commit 2603b98ca (aubdump: Support softpin bos). > >> >> > > >> >> > Signed-off-by: Marius Vlad > >> >> > CC: Kristian Høgsberg Kristensen > >> >> > --- > >> >> > configure.ac | 2 +- > >> >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> >> > > >> >> > diff --git a/configure.ac b/configure.ac > >> >> > index f05bcb0..ade9756 100644 > >> >> > --- a/configure.ac > >> >> > +++ b/configure.ac > >> >> > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then > >> >> > fi > >> >> > AC_SUBST(ASSEMBLER_WARN_CFLAGS) > >> >> > > >> >> > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) > >> >> > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) > >> >> Yes please. As you do that one can nuke most/all the "define LOCAL_" > >> >> and "struct local_" (in lib/ioctl_wrappers.h) > >> >> and replace with with the official symbols. A very nice plus imho ;-) > >> > > >> > Please don't. It makes running on older installations even more > >> > cumbersome. > >> Slightly confused here: are you against the libdrm_intel bump, or the > >> removal of the local symbols ? > > > > Local symbols. They save a lot of time if you can just get the test you > > need compiling and not worry about dependencies. One of the basic > > tenents is that we drop a new kernel into an old userspace and expect to > > have not broken anything. Being lazy, for smoke testing I just build > > in situ. > > > Fully agree. > > >> Admittedly sometimes distros don't bother/refuse to update libdrm > >> which could be an issue in the former case. Although if the package > >> (with all the definitions) is compulsory, how would that cause an > >> issue ? > > > > The package may not even exist when testing on v.old distro images. > > It is mostly a major of convenience, but since the work is already done > > to be independent, removing causes more work. > Seems that things are not completely independent, as illustrated by > the version bump. > > Thus was wondering what's the benefit of (or why object against > nuking) the local copies. Since even if they were needed before, they > won't be after the bump.That said, it seems that it's a picky/sore > topic so I won't dig into it. I don't have uptodate or even recent libdrm everywhere. Getting rid of the locals means another dependency chain to build. Just being completely lazy and saying "if ain't broke, don't try to fix it". -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Bump libdrm-intel dependency library to a newer version that support softpin.
On 14 July 2016 at 21:03, Chris Wilson wrote: > On Thu, Jul 14, 2016 at 07:54:59PM +0100, Emil Velikov wrote: >> On 14 July 2016 at 17:49, Chris Wilson wrote: >> > On Thu, Jul 14, 2016 at 05:29:55PM +0100, Emil Velikov wrote: >> >> Hi Marius, >> >> >> >> Just double-checking - this is an i-g-t patch, isn't it ? >> >> >> >> On 14 July 2016 at 11:39, Marius Vlad wrote: >> >> > Required by commit 2603b98ca (aubdump: Support softpin bos). >> >> > >> >> > Signed-off-by: Marius Vlad >> >> > CC: Kristian Høgsberg Kristensen >> >> > --- >> >> > configure.ac | 2 +- >> >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> >> > >> >> > diff --git a/configure.ac b/configure.ac >> >> > index f05bcb0..ade9756 100644 >> >> > --- a/configure.ac >> >> > +++ b/configure.ac >> >> > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then >> >> > fi >> >> > AC_SUBST(ASSEMBLER_WARN_CFLAGS) >> >> > >> >> > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) >> >> > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) >> >> Yes please. As you do that one can nuke most/all the "define LOCAL_" >> >> and "struct local_" (in lib/ioctl_wrappers.h) >> >> and replace with with the official symbols. A very nice plus imho ;-) >> > >> > Please don't. It makes running on older installations even more >> > cumbersome. >> Slightly confused here: are you against the libdrm_intel bump, or the >> removal of the local symbols ? > > Local symbols. They save a lot of time if you can just get the test you > need compiling and not worry about dependencies. One of the basic > tenents is that we drop a new kernel into an old userspace and expect to > have not broken anything. Being lazy, for smoke testing I just build > in situ. > Fully agree. >> Admittedly sometimes distros don't bother/refuse to update libdrm >> which could be an issue in the former case. Although if the package >> (with all the definitions) is compulsory, how would that cause an >> issue ? > > The package may not even exist when testing on v.old distro images. > It is mostly a major of convenience, but since the work is already done > to be independent, removing causes more work. Seems that things are not completely independent, as illustrated by the version bump. Thus was wondering what's the benefit of (or why object against nuking) the local copies. Since even if they were needed before, they won't be after the bump.That said, it seems that it's a picky/sore topic so I won't dig into it. If anything I'll just send patches to be acked/nacked. -Emil ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ Ro.CI.BAT: warning for Fixes for HPD (rev3)
On Wed, Jun 22, 2016 at 01:22:09PM -, Patchwork wrote: > == Series Details == > > Series: Fixes for HPD (rev3) > URL : https://patchwork.freedesktop.org/series/8870/ > State : warning > > == Summary == > > Series 8870v3 Fixes for HPD > http://patchwork.freedesktop.org/api/1.0/series/8870/revisions/3/mbox > > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-a: > skip -> DMESG-WARN (ro-bdw-i7-5557U) > skip -> DMESG-WARN (ro-bdw-i5-5250u) > Subgroup suspend-read-crc-pipe-c: > skip -> DMESG-WARN (ro-bdw-i7-5557U) > skip -> DMESG-WARN (ro-bdw-i5-5250u) Say hello to the bdw link training fail that's happening all over recently: https://bugs.freedesktop.org/show_bug.cgi?id=96614 Since it's not entirely clear (at least to me) whether the "enabling power well causes hpd" issue is still present that Ville reported, and since these patches seem stuck forever, I went ahead and applied. Lyude can't repro, and afaik Ville said that this spurious hpd is rather sporadic. -Daniel > > fi-skl-i7-6700k total:226 pass:188 dwarn:0 dfail:0 fail:1 skip:37 > ro-bdw-i5-5250u total:226 pass:197 dwarn:4 dfail:0 fail:1 skip:24 > ro-bdw-i7-5557U total:226 pass:198 dwarn:3 dfail:0 fail:1 skip:24 > ro-bdw-i7-5600u total:226 pass:185 dwarn:0 dfail:0 fail:1 skip:40 > ro-bsw-n3050 total:226 pass:170 dwarn:1 dfail:0 fail:4 skip:51 > ro-byt-n2820 total:226 pass:173 dwarn:0 dfail:0 fail:4 skip:49 > ro-hsw-i3-4010u total:226 pass:190 dwarn:0 dfail:0 fail:1 skip:35 > ro-hsw-i7-4770r total:226 pass:190 dwarn:0 dfail:0 fail:1 skip:35 > ro-ilk-i7-620lm total:226 pass:150 dwarn:0 dfail:0 fail:2 skip:74 > ro-ilk1-i5-650 total:221 pass:150 dwarn:0 dfail:0 fail:2 skip:69 > ro-ivb-i7-3770 total:226 pass:181 dwarn:0 dfail:0 fail:1 skip:44 > ro-ivb2-i7-3770 total:226 pass:185 dwarn:0 dfail:0 fail:1 skip:40 > ro-skl3-i5-6260u total:226 pass:201 dwarn:1 dfail:0 fail:1 skip:23 > ro-snb-i7-2620M total:226 pass:174 dwarn:0 dfail:0 fail:2 skip:50 > fi-hsw-i7-4770k failed to connect after reboot > fi-kbl-qkkr failed to connect after reboot > fi-skl-i5-6260u failed to connect after reboot > fi-snb-i7-2600 failed to connect after reboot > > Results at /archive/results/CI_IGT_test/RO_Patchwork_1267/ > > f1f5a05 drm-intel-nightly: 2016y-06m-22d-10h-45m-32s UTC integration manifest > 4f8a723 drm/i915: Enable polling when we don't have hpd > 690a832 drm/i915/vlv: Disable HPD in valleyview_crt_detect_hotplug() > 990a134 drm/i915/vlv: Reset the ADPA in vlv_display_power_well_init() > ae95081 drm/i915/vlv: Make intel_crt_reset() per-encoder > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PULL] drm-intel-fixes
Hi Dave, Just 2 regression fixes. I've also realized that a pile of hang fixes for kbl landed in next, and no one thought of backporting it to 4.7 - kbl has lost prelim_hw_support tagging in 4.7-rc1 already. Mika is prepping a topic branch for those, will send you a separate pull request since it's quite a bit (but should be all well restricted to kbl code, so similar to polaris in amdgpu). Cheers, Daniel The following changes since commit cab103274352721b77fc5923a631fc63350bef8e: drm/i915: Fix missing unlock on error in i915_ppgtt_info() (2016-06-30 13:30:15 +0300) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/drm-intel-fixes-2016-07-14 for you to fetch changes up to aeddda06c1a704bb97c8a7bfe7a472120193bd56: drm/i915: Ignore panel type from OpRegion on SKL (2016-07-14 16:08:04 +0200) Chris Wilson (1): drm/i915: Update ifdeffery for mutex->owner Ville Syrjälä (1): drm/i915: Ignore panel type from OpRegion on SKL drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 +- drivers/gpu/drm/i915/intel_opregion.c| 11 +++ 2 files changed, 12 insertions(+), 1 deletion(-) -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Bump libdrm-intel dependency library to a newer version that support softpin.
On Thu, Jul 14, 2016 at 07:54:59PM +0100, Emil Velikov wrote: > On 14 July 2016 at 17:49, Chris Wilson wrote: > > On Thu, Jul 14, 2016 at 05:29:55PM +0100, Emil Velikov wrote: > >> Hi Marius, > >> > >> Just double-checking - this is an i-g-t patch, isn't it ? > >> > >> On 14 July 2016 at 11:39, Marius Vlad wrote: > >> > Required by commit 2603b98ca (aubdump: Support softpin bos). > >> > > >> > Signed-off-by: Marius Vlad > >> > CC: Kristian Høgsberg Kristensen > >> > --- > >> > configure.ac | 2 +- > >> > 1 file changed, 1 insertion(+), 1 deletion(-) > >> > > >> > diff --git a/configure.ac b/configure.ac > >> > index f05bcb0..ade9756 100644 > >> > --- a/configure.ac > >> > +++ b/configure.ac > >> > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then > >> > fi > >> > AC_SUBST(ASSEMBLER_WARN_CFLAGS) > >> > > >> > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) > >> > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) > >> Yes please. As you do that one can nuke most/all the "define LOCAL_" > >> and "struct local_" (in lib/ioctl_wrappers.h) > >> and replace with with the official symbols. A very nice plus imho ;-) > > > > Please don't. It makes running on older installations even more > > cumbersome. > Slightly confused here: are you against the libdrm_intel bump, or the > removal of the local symbols ? Local symbols. They save a lot of time if you can just get the test you need compiling and not worry about dependencies. One of the basic tenents is that we drop a new kernel into an old userspace and expect to have not broken anything. Being lazy, for smoke testing I just build in situ. > Admittedly sometimes distros don't bother/refuse to update libdrm > which could be an issue in the former case. Although if the package > (with all the definitions) is compulsory, how would that cause an > issue ? The package may not even exist when testing on v.old distro images. It is mostly a major of convenience, but since the work is already done to be independent, removing causes more work. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Bump libdrm-intel dependency library to a newer version that support softpin.
On 14 July 2016 at 17:49, Chris Wilson wrote: > On Thu, Jul 14, 2016 at 05:29:55PM +0100, Emil Velikov wrote: >> Hi Marius, >> >> Just double-checking - this is an i-g-t patch, isn't it ? >> >> On 14 July 2016 at 11:39, Marius Vlad wrote: >> > Required by commit 2603b98ca (aubdump: Support softpin bos). >> > >> > Signed-off-by: Marius Vlad >> > CC: Kristian Høgsberg Kristensen >> > --- >> > configure.ac | 2 +- >> > 1 file changed, 1 insertion(+), 1 deletion(-) >> > >> > diff --git a/configure.ac b/configure.ac >> > index f05bcb0..ade9756 100644 >> > --- a/configure.ac >> > +++ b/configure.ac >> > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then >> > fi >> > AC_SUBST(ASSEMBLER_WARN_CFLAGS) >> > >> > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) >> > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) >> Yes please. As you do that one can nuke most/all the "define LOCAL_" >> and "struct local_" (in lib/ioctl_wrappers.h) >> and replace with with the official symbols. A very nice plus imho ;-) > > Please don't. It makes running on older installations even more > cumbersome. Slightly confused here: are you against the libdrm_intel bump, or the removal of the local symbols ? Admittedly sometimes distros don't bother/refuse to update libdrm which could be an issue in the former case. Although if the package (with all the definitions) is compulsory, how would that cause an issue ? Genuine question here, not trying to be smart/cheeky/etc. Thanks Emil ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [drm-intel:drm-intel-nightly 2/10] drivers/gpu/drm/i915/i915_drv.h:2695:26: note: in expansion of macro 'INTEL_INFO'
tree: git://anongit.freedesktop.org/drm-intel drm-intel-nightly
head: 2d854c67e3af36b190e8499a3bfad7cdccde0f67
commit: 01734c300fbff01e321d4ff1b3c91e24e0a3b90d [2/10] Merge remote-tracking
branch 'origin/drm-intel-next-fixes' into drm-intel-nightly
config: i386-allmodconfig (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
git checkout 01734c300fbff01e321d4ff1b3c91e24e0a3b90d
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from drivers/gpu/drm/i915/intel_opregion.c:34:0:
drivers/gpu/drm/i915/intel_opregion.c: In function
'intel_opregion_get_panel_type':
drivers/gpu/drm/i915/intel_opregion.c:1081:17: error: 'dev' undeclared
(first use in this function)
if (IS_SKYLAKE(dev)) {
^
drivers/gpu/drm/i915/i915_drv.h:2628:43: note: in definition of macro
'__I915__'
if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
^
>> drivers/gpu/drm/i915/i915_drv.h:2695:26: note: in expansion of macro
>> 'INTEL_INFO'
#define IS_SKYLAKE(dev) (INTEL_INFO(dev)->is_skylake)
^~
>> drivers/gpu/drm/i915/intel_opregion.c:1081:6: note: in expansion of macro
>> 'IS_SKYLAKE'
if (IS_SKYLAKE(dev)) {
^~
drivers/gpu/drm/i915/intel_opregion.c:1081:17: note: each undeclared
identifier is reported only once for each function it appears in
if (IS_SKYLAKE(dev)) {
^
drivers/gpu/drm/i915/i915_drv.h:2628:43: note: in definition of macro
'__I915__'
if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
^
>> drivers/gpu/drm/i915/i915_drv.h:2695:26: note: in expansion of macro
>> 'INTEL_INFO'
#define IS_SKYLAKE(dev) (INTEL_INFO(dev)->is_skylake)
^~
>> drivers/gpu/drm/i915/intel_opregion.c:1081:6: note: in expansion of macro
>> 'IS_SKYLAKE'
if (IS_SKYLAKE(dev)) {
^~
vim +/INTEL_INFO +2695 drivers/gpu/drm/i915/i915_drv.h
351e3db2 Brad Volkin2014-02-18 2622int count;
351e3db2 Brad Volkin2014-02-18 2623 };
351e3db2 Brad Volkin2014-02-18 2624
dbbe9127 Chris Wilson 2014-08-09 2625 /* Note that the (struct
drm_i915_private *) cast is just to shut up gcc. */
7312e2dd Chris Wilson 2014-08-13 2626 #define __I915__(p) ({ \
7312e2dd Chris Wilson 2014-08-13 2627struct drm_i915_private *__p; \
7312e2dd Chris Wilson 2014-08-13 @2628if
(__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
7312e2dd Chris Wilson 2014-08-13 2629__p = (struct
drm_i915_private *)p; \
7312e2dd Chris Wilson 2014-08-13 2630else if
(__builtin_types_compatible_p(typeof(*p), struct drm_device)) \
7312e2dd Chris Wilson 2014-08-13 2631__p = to_i915((struct
drm_device *)p); \
7312e2dd Chris Wilson 2014-08-13 2632else \
7312e2dd Chris Wilson 2014-08-13 2633BUILD_BUG(); \
7312e2dd Chris Wilson 2014-08-13 2634__p; \
7312e2dd Chris Wilson 2014-08-13 2635 })
dbbe9127 Chris Wilson 2014-08-09 2636 #define INTEL_INFO(p)
(&__I915__(p)->info)
3f10e82f Jani Nikula2016-04-07 2637 #define INTEL_GEN(p)
(INTEL_INFO(p)->gen)
87f1f465 Chris Wilson 2014-08-09 2638 #define INTEL_DEVID(p)
(INTEL_INFO(p)->device_id)
cae5852d Zou Nan hai2010-11-09 2639
e87a005d Jani Nikula2015-10-20 2640 #define REVID_FOREVER
0xff
091387c1 Chris Wilson 2016-06-24 2641 #define INTEL_REVID(p)
(__I915__(p)->drm.pdev->revision)
ac657f64 Tvrtko Ursulin 2016-05-10 2642
ac657f64 Tvrtko Ursulin 2016-05-10 2643 #define GEN_FOREVER (0)
ac657f64 Tvrtko Ursulin 2016-05-10 2644 /*
ac657f64 Tvrtko Ursulin 2016-05-10 2645 * Returns true if Gen is in
inclusive range [Start, End].
ac657f64 Tvrtko Ursulin 2016-05-10 2646 *
ac657f64 Tvrtko Ursulin 2016-05-10 2647 * Use GEN_FOREVER for unbound
start and or end.
ac657f64 Tvrtko Ursulin 2016-05-10 2648 */
ac657f64 Tvrtko Ursulin 2016-05-10 2649 #define IS_GEN(p, s, e) ({ \
ac657f64 Tvrtko Ursulin 2016-05-10 2650unsigned int __s = (s), __e =
(e); \
ac657f64 Tvrtko Ursulin 2016-05-10 2651
BUILD_BUG_ON(!__builtin_constant_p(s)); \
ac657f64 Tvrtko Ursulin 2016-05-10 2652
BUILD_BUG_ON(!__builtin_constant_p(e)); \
ac657f64 Tvrtko Ursulin 2016-05-10 2653if ((__s) != GEN_FOREVER) \
ac657f64 Tvrtko Ursulin 2016-05-10 2654__s = (s) - 1; \
ac657f64 Tvrtko Ursulin 2016-05-10 2655if ((__e) == GEN_FOREVER) \
ac657f64 Tvrtko Ursulin 2016-05-10 2656__e = BITS_PER_LONG -
1; \
ac657f64 Tvrtko Ursulin 2016-05-10 2657else \
ac657f64 Tvrtko Ursu
[Intel-gfx] [drm-intel:drm-intel-nightly 2/10] drivers/gpu/drm/i915/intel_opregion.c:1081:2: note: in expansion of macro 'if'
tree: git://anongit.freedesktop.org/drm-intel drm-intel-nightly
head: 2d854c67e3af36b190e8499a3bfad7cdccde0f67
commit: 01734c300fbff01e321d4ff1b3c91e24e0a3b90d [2/10] Merge remote-tracking
branch 'origin/drm-intel-next-fixes' into drm-intel-nightly
config: x86_64-randconfig-s1-07150032 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
git checkout 01734c300fbff01e321d4ff1b3c91e24e0a3b90d
# save the attached .config to linux build tree
make ARCH=x86_64
All warnings (new ones prefixed by >>):
In file included from include/linux/ioport.h:12:0,
from include/linux/acpi.h:25,
from drivers/gpu/drm/i915/intel_opregion.c:28:
drivers/gpu/drm/i915/intel_opregion.c: In function
'intel_opregion_get_panel_type':
drivers/gpu/drm/i915/intel_opregion.c:1081:17: error: 'dev' undeclared
(first use in this function)
if (IS_SKYLAKE(dev)) {
^
include/linux/compiler.h:151:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> drivers/gpu/drm/i915/intel_opregion.c:1081:2: note: in expansion of macro
>> 'if'
if (IS_SKYLAKE(dev)) {
^~
include/linux/compiler.h:149:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~
>> drivers/gpu/drm/i915/i915_drv.h:2628:2: note: in expansion of macro 'if'
if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
^~
>> drivers/gpu/drm/i915/i915_drv.h:2636:26: note: in expansion of macro
>> '__I915__'
#define INTEL_INFO(p) (&__I915__(p)->info)
^~~~
drivers/gpu/drm/i915/i915_drv.h:2695:26: note: in expansion of macro
'INTEL_INFO'
#define IS_SKYLAKE(dev) (INTEL_INFO(dev)->is_skylake)
^~
drivers/gpu/drm/i915/intel_opregion.c:1081:6: note: in expansion of macro
'IS_SKYLAKE'
if (IS_SKYLAKE(dev)) {
^~
drivers/gpu/drm/i915/intel_opregion.c:1081:17: note: each undeclared
identifier is reported only once for each function it appears in
if (IS_SKYLAKE(dev)) {
^
include/linux/compiler.h:151:30: note: in definition of macro '__trace_if'
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^~~~
>> drivers/gpu/drm/i915/intel_opregion.c:1081:2: note: in expansion of macro
>> 'if'
if (IS_SKYLAKE(dev)) {
^~
include/linux/compiler.h:149:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~
>> drivers/gpu/drm/i915/i915_drv.h:2628:2: note: in expansion of macro 'if'
if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
^~
>> drivers/gpu/drm/i915/i915_drv.h:2636:26: note: in expansion of macro
>> '__I915__'
#define INTEL_INFO(p) (&__I915__(p)->info)
^~~~
drivers/gpu/drm/i915/i915_drv.h:2695:26: note: in expansion of macro
'INTEL_INFO'
#define IS_SKYLAKE(dev) (INTEL_INFO(dev)->is_skylake)
^~
drivers/gpu/drm/i915/intel_opregion.c:1081:6: note: in expansion of macro
'IS_SKYLAKE'
if (IS_SKYLAKE(dev)) {
^~
vim +/if +1081 drivers/gpu/drm/i915/intel_opregion.c
a0562819 Ville Syrjälä 2016-04-11 1065 DRM_DEBUG_KMS("Invalid
OpRegion panel type 0x%x\n", ret);
a0562819 Ville Syrjälä 2016-04-11 1066 return -EINVAL;
a0562819 Ville Syrjälä 2016-04-11 1067 }
a0562819 Ville Syrjälä 2016-04-11 1068
a0562819 Ville Syrjälä 2016-04-11 1069 /* fall back to VBT panel type?
*/
a0562819 Ville Syrjälä 2016-04-11 1070 if (ret == 0x0) {
a0562819 Ville Syrjälä 2016-04-11 1071 DRM_DEBUG_KMS("No panel
type in OpRegion\n");
a0562819 Ville Syrjälä 2016-04-11 1072 return -ENODEV;
a0562819 Ville Syrjälä 2016-04-11 1073 }
a0562819 Ville Syrjälä 2016-04-11 1074
aeddda06 Ville Syrjälä 2016-07-12 1075 /*
aeddda06 Ville Syrjälä 2016-07-12 1076 * FIXME On Dell XPS 13 9350
the OpRegion panel type (0) gives us
aeddda06 Ville Syrjälä 2016-07-12 1077 * low vswing for eDP, whereas
the VBT panel type (2) gives us normal
aeddda06 Ville Syrjälä 2016-07-12 1078 * vswing instead. Low vswing
results in some display flickers, so
aeddda06 Ville Syrjälä 2016-07-12 1079 * let's simply ignore the
OpRegion panel type on SKL for now.
aeddda06 Ville Syrjälä 2016-07-12 1080 */
aeddda06 Ville Syrjälä 2016-07-12 @1081 if (IS_SKYLAKE(dev)) {
aeddda06 Ville Syrjälä 2016-07-12 1082 DRM_DEBUG_KMS("Ignoring
OpRegion panel type (%d)\n", ret - 1);
aeddda06 Ville Syrjälä 2016-07-12 1083 return -ENODEV
Re: [Intel-gfx] [PATCH] drm/i915: Fix legacy gamma lut updates in Linux 4.7-rc6
Ok, so legacy gamma table updates are completely broken for Intel on
Linux-4.7-rc7, the final release candidate.
The good news is that applying Lionel's patch
"drm/i915: add missing condition for committing planes on crtc"
from
https://patchwork.freedesktop.org/patch/89111/
fixes it nicely. The patch currently applies cleanly to drm-fixes and
drm-next and is
Reviewed-and-tested-by: Mario Kleiner
When we are at it, could somebody please look at that updated series of
my Displayport color depth fixes ("EDID/DP fixes for proper bpc
detection of displays.") i sent out a week ago?
Especially pulling patch 2/5 "[PATCH 2/5] drm/i915/dp: Revert
"drm/i915/dp: fall back to 18 bpp when sink capability is unknown" would
be important, as that bug introduced a regression for Intel + DP +
legacy DP converters into stable kernels which is very serious for users
of scientific/medical display equipment, especially as the failures can
easily go unnoticed during normal equipment tests, but would introduce
the equivalent of "silent data corruption" into their measured
scientific data, which is not a great experience given that collecting
such data can easily take half a year of work time and ten-thousands of
euros of wasted research funding.
Patches 3 and 4 contain changes Daniel asked me to do, patch 5 would be
good to safe-guard against similar issues in the future.
thanks,
-mario
On 07/12/2016 12:50 PM, Lionel Landwerlin wrote:
Hi Mario,
There was a couple of patch to fix this issue :
https://patchwork.freedesktop.org/series/5467/
https://patchwork.freedesktop.org/series/5466/
I tested this late last week on drm-intel-nightly, it seems a series of
revert fixed most of the issues.
Cheers,
-
Lionel
On 12/07/16 11:33, Mario Kleiner wrote:
Updating legacy gamma tables, e.g., via RandR doesn't work at all
as of Linux 4.7-rc6.
Reason seems to be that the required call to
drm_atomic_helper_commit_planes_on_crtc is skipped in
intel_atomic_commit after userspace set new gamma tables,
because neither crtc->state->planes_changed nor
update_pipe (= pipe_config->update_pipe) are true.
Removing the check for planes_changed || update_pipe fixes
gamma table updates.
The code for Linux 4.8 drm-next has changed a lot in that area
wrt. 4.7, but the new code for 4.8 also removed those checks
and calls drm_atomic_helper_commit_planes_on_crtc unconditionally,
and legacy gamma lut updates work on drm-next, so this seems to be
the right solution.
Tested also shutdown/reboot, suspend/resume, (un-)plugging displays,
mode switches for resolution/refresh rate, display rotation, and
page-flipping/pageflip timing on Intel HD Ironlake to confirm the
fix apparently doesn't break anything under X11.
Signed-off-by: Mario Kleiner
Cc: Maarten Lankhorst
Cc: Lionel Landwerlin
Cc: Daniel Vetter
---
drivers/gpu/drm/i915/intel_display.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 04452cf..eb8fb36 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13685,7 +13685,6 @@ static int intel_atomic_commit(struct
drm_device *dev,
bool modeset = needs_modeset(crtc->state);
struct intel_crtc_state *pipe_config =
to_intel_crtc_state(crtc->state);
-bool update_pipe = !modeset && pipe_config->update_pipe;
if (modeset && crtc->state->active) {
update_scanline_offset(to_intel_crtc(crtc));
@@ -13699,8 +13698,7 @@ static int intel_atomic_commit(struct
drm_device *dev,
drm_atomic_get_existing_plane_state(state, crtc->primary))
intel_fbc_enable(intel_crtc);
-if (crtc->state->active &&
-(crtc->state->planes_changed || update_pipe))
+if (crtc->state->active)
drm_atomic_helper_commit_planes_on_crtc(old_crtc_state);
if (pipe_config->base.active && needs_vblank_wait(pipe_config))
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/5] drm/i915: Check live status before reading edid
On Wed, Jul 13, 2016 at 01:17:40PM +0100, Chris Wilson wrote: > On Wed, Jul 13, 2016 at 01:09:28PM +0100, Chris Wilson wrote: > > On Wed, Jul 13, 2016 at 01:59:52PM +0200, Daniel Vetter wrote: > > > I think the proper solution should be to make all the probing and fbdev > > > restoring async. As soon as we have working async atomic commit for > > > modeset we should be able to do all that. > > > > We're not just blocked on the mode change here (indeed, we shouldn't be > > changing modes at resume at all, right?) but we appear to be doing a > > full detection cycle whereas the intent was just to tell userspace > > everything had changed, and for it to go figure out what to do about it. > > > > Also note that we can simply move this all out of the blocking resume > > path and still run this in parallel to userspace resuming (and of course > > serialised with whatever userspace wants to do). > > And to remind myself of conversations going on elsewhere, the more async > we make resume the more inaccurate the current method of measuing resume > time becomes (which more or less just looks at the initcall graph). We > need a metric that measures the time from resume to the time of first > pixel (first flip to a lit display preferably). I've shown how we can > get our "resume time" down to about 10ms - all because the metric is > subject to abuse. Good news on this front -- it seems that the SuspendResume tool can be adapted to measure our resume-time even if we "cheat", and the author offered to help with this. So we "just" need to decide what actually constitutes being done with resume. Kind regards, David ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] tests: Fix compilation when some gcc configurations
On Thu, Jul 14, 2016 at 06:44:59PM +0100, Chris Wilson wrote:
> On Thu, Jul 14, 2016 at 06:31:37PM +0100, Damien Lespiau wrote:
> > Depending how the gcc was compiled it may be necessary to enable SSE4
> > instructions explicitly. Otherwise:
> >
> > CC gem_gtt_speed.o
> > In file included from gem_gtt_speed.c:54:0:
> > /usr/lib/gcc/x86_64-linux-gnu/4.8/include/smmintrin.h:31:3: error: #error
> > "SSE4.1 instruction set not enabled"
> > # error "SSE4.1 instruction set not enabled"
>
> So the challenge is getting the function attribute applied to the
> include.
>
> Ah, can you try
> #pragma GCC target ("sse4.1")
> in those blocks instead?
Oh, and we probably need an #if GCC > 4.y to be fully backwards
compatible.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] tests: Fix compilation when some gcc configurations
On Thu, Jul 14, 2016 at 06:31:37PM +0100, Damien Lespiau wrote:
> Depending how the gcc was compiled it may be necessary to enable SSE4
> instructions explicitly. Otherwise:
>
> CC gem_gtt_speed.o
> In file included from gem_gtt_speed.c:54:0:
> /usr/lib/gcc/x86_64-linux-gnu/4.8/include/smmintrin.h:31:3: error: #error
> "SSE4.1 instruction set not enabled"
> # error "SSE4.1 instruction set not enabled"
So the challenge is getting the function attribute applied to the
include.
Ah, can you try
#pragma GCC target ("sse4.1")
in those blocks instead?
> @@ -76,6 +76,7 @@ gem_ctx_basic_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> gem_ctx_basic_LDADD = $(LDADD) -lpthread
> gem_ctx_thrash_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS)
> gem_ctx_thrash_LDADD = $(LDADD) -lpthread
> +gem_exec_flush_CFLAGS = $(AM_CFLAGS) -msse4
Note, it should be -msse4.1
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t] tests: Fix compilation when some gcc configurations
Depending how the gcc was compiled it may be necessary to enable SSE4 instructions explicitly. Otherwise: CC gem_gtt_speed.o In file included from gem_gtt_speed.c:54:0: /usr/lib/gcc/x86_64-linux-gnu/4.8/include/smmintrin.h:31:3: error: #error "SSE4.1 instruction set not enabled" # error "SSE4.1 instruction set not enabled" ^ gem_gtt_speed.c: In function ‘streaming_load’: gem_gtt_speed.c:59:2: error: unknown type name ‘__m128i’ __m128i tmp, *s = src; ^ gem_gtt_speed.c:65:3: error: implicit declaration of function ‘_mm_stream_load_si128’ [-Werror=implicit-function-declaration] tmp += _mm_stream_load_si128(s++); ^ gem_gtt_speed.c:65:3: warning: nested extern declaration of ‘_mm_stream_load_si128’ [-Wnested-externs] gem_gtt_speed.c:70:2: error: unknown type name ‘__m128i’ *(volatile __m128i *)src = tmp; ^ CC: Chris Wilson Signed-off-by: Damien Lespiau --- tests/Makefile.am | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/Makefile.am b/tests/Makefile.am index 102b8a6..8c6b0a3 100644 --- a/tests/Makefile.am +++ b/tests/Makefile.am @@ -76,6 +76,7 @@ gem_ctx_basic_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) gem_ctx_basic_LDADD = $(LDADD) -lpthread gem_ctx_thrash_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) gem_ctx_thrash_LDADD = $(LDADD) -lpthread +gem_exec_flush_CFLAGS = $(AM_CFLAGS) -msse4 gem_exec_parallel_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) gem_exec_parallel_LDADD = $(LDADD) -lpthread gem_fence_thrash_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) @@ -84,6 +85,7 @@ gem_fence_upload_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) gem_fence_upload_LDADD = $(LDADD) -lpthread gem_flink_race_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) gem_flink_race_LDADD = $(LDADD) -lpthread +gem_gtt_speed_CFLAGS = $(AM_CFLAGS) -msse4 gem_mmap_gtt_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) gem_mmap_gtt_LDADD = $(LDADD) -lpthread gem_mmap_wc_CFLAGS = $(AM_CFLAGS) $(THREAD_CFLAGS) -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Bump libdrm-intel dependency library to a newer version that support softpin.
On Thu, Jul 14, 2016 at 05:29:55PM +0100, Emil Velikov wrote: > Hi Marius, > > Just double-checking - this is an i-g-t patch, isn't it ? > > On 14 July 2016 at 11:39, Marius Vlad wrote: > > Required by commit 2603b98ca (aubdump: Support softpin bos). > > > > Signed-off-by: Marius Vlad > > CC: Kristian Høgsberg Kristensen > > --- > > configure.ac | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/configure.ac b/configure.ac > > index f05bcb0..ade9756 100644 > > --- a/configure.ac > > +++ b/configure.ac > > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then > > fi > > AC_SUBST(ASSEMBLER_WARN_CFLAGS) > > > > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) > > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) > Yes please. As you do that one can nuke most/all the "define LOCAL_" > and "struct local_" (in lib/ioctl_wrappers.h) > and replace with with the official symbols. A very nice plus imho ;-) Please don't. It makes running on older installations even more cumbersome. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Bump libdrm-intel dependency library to a newer version that support softpin.
Hi Marius, Just double-checking - this is an i-g-t patch, isn't it ? On 14 July 2016 at 11:39, Marius Vlad wrote: > Required by commit 2603b98ca (aubdump: Support softpin bos). > > Signed-off-by: Marius Vlad > CC: Kristian Høgsberg Kristensen > --- > configure.ac | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/configure.ac b/configure.ac > index f05bcb0..ade9756 100644 > --- a/configure.ac > +++ b/configure.ac > @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then > fi > AC_SUBST(ASSEMBLER_WARN_CFLAGS) > > -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) > +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) Yes please. As you do that one can nuke most/all the "define LOCAL_" and "struct local_" (in lib/ioctl_wrappers.h) and replace with with the official symbols. A very nice plus imho ;-) Side note: this will conflict with Robert Foss's work to make libdrm_intel an optional dependency. Have you/others had the chance to look at the series ? What can we do to get that moving/accepted ? Thanks Emil ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [drm-intel:drm-intel-nightly 2/10] drivers/gpu/drm/i915/intel_opregion.c:1081:17: error: 'dev' undeclared
tree: git://anongit.freedesktop.org/drm-intel drm-intel-nightly
head: 2d854c67e3af36b190e8499a3bfad7cdccde0f67
commit: 01734c300fbff01e321d4ff1b3c91e24e0a3b90d [2/10] Merge remote-tracking
branch 'origin/drm-intel-next-fixes' into drm-intel-nightly
config: i386-randconfig-i1-201628 (attached as .config)
compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430
reproduce:
git checkout 01734c300fbff01e321d4ff1b3c91e24e0a3b90d
# save the attached .config to linux build tree
make ARCH=i386
All errors (new ones prefixed by >>):
In file included from drivers/gpu/drm/i915/intel_opregion.c:34:0:
drivers/gpu/drm/i915/intel_opregion.c: In function
'intel_opregion_get_panel_type':
>> drivers/gpu/drm/i915/intel_opregion.c:1081:17: error: 'dev' undeclared
>> (first use in this function)
if (IS_SKYLAKE(dev)) {
^
drivers/gpu/drm/i915/i915_drv.h:2628:43: note: in definition of macro
'__I915__'
if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
^
drivers/gpu/drm/i915/i915_drv.h:2695:26: note: in expansion of macro
'INTEL_INFO'
#define IS_SKYLAKE(dev) (INTEL_INFO(dev)->is_skylake)
^~
drivers/gpu/drm/i915/intel_opregion.c:1081:6: note: in expansion of macro
'IS_SKYLAKE'
if (IS_SKYLAKE(dev)) {
^~
drivers/gpu/drm/i915/intel_opregion.c:1081:17: note: each undeclared
identifier is reported only once for each function it appears in
if (IS_SKYLAKE(dev)) {
^
drivers/gpu/drm/i915/i915_drv.h:2628:43: note: in definition of macro
'__I915__'
if (__builtin_types_compatible_p(typeof(*p), struct drm_i915_private)) \
^
drivers/gpu/drm/i915/i915_drv.h:2695:26: note: in expansion of macro
'INTEL_INFO'
#define IS_SKYLAKE(dev) (INTEL_INFO(dev)->is_skylake)
^~
drivers/gpu/drm/i915/intel_opregion.c:1081:6: note: in expansion of macro
'IS_SKYLAKE'
if (IS_SKYLAKE(dev)) {
^~
vim +/dev +1081 drivers/gpu/drm/i915/intel_opregion.c
a0562819 Ville Syrjälä 2016-04-11 1065 DRM_DEBUG_KMS("Invalid
OpRegion panel type 0x%x\n", ret);
a0562819 Ville Syrjälä 2016-04-11 1066 return -EINVAL;
a0562819 Ville Syrjälä 2016-04-11 1067 }
a0562819 Ville Syrjälä 2016-04-11 1068
a0562819 Ville Syrjälä 2016-04-11 1069 /* fall back to VBT panel type?
*/
a0562819 Ville Syrjälä 2016-04-11 1070 if (ret == 0x0) {
a0562819 Ville Syrjälä 2016-04-11 1071 DRM_DEBUG_KMS("No panel
type in OpRegion\n");
a0562819 Ville Syrjälä 2016-04-11 1072 return -ENODEV;
a0562819 Ville Syrjälä 2016-04-11 1073 }
a0562819 Ville Syrjälä 2016-04-11 1074
aeddda06 Ville Syrjälä 2016-07-12 1075 /*
aeddda06 Ville Syrjälä 2016-07-12 1076 * FIXME On Dell XPS 13 9350
the OpRegion panel type (0) gives us
aeddda06 Ville Syrjälä 2016-07-12 1077 * low vswing for eDP, whereas
the VBT panel type (2) gives us normal
aeddda06 Ville Syrjälä 2016-07-12 1078 * vswing instead. Low vswing
results in some display flickers, so
aeddda06 Ville Syrjälä 2016-07-12 1079 * let's simply ignore the
OpRegion panel type on SKL for now.
aeddda06 Ville Syrjälä 2016-07-12 1080 */
aeddda06 Ville Syrjälä 2016-07-12 @1081 if (IS_SKYLAKE(dev)) {
aeddda06 Ville Syrjälä 2016-07-12 1082 DRM_DEBUG_KMS("Ignoring
OpRegion panel type (%d)\n", ret - 1);
aeddda06 Ville Syrjälä 2016-07-12 1083 return -ENODEV;
aeddda06 Ville Syrjälä 2016-07-12 1084 }
aeddda06 Ville Syrjälä 2016-07-12 1085
a0562819 Ville Syrjälä 2016-04-11 1086 return ret - 1;
a0562819 Ville Syrjälä 2016-04-11 1087 }
:: The code at line 1081 was first introduced by commit
:: aeddda06c1a704bb97c8a7bfe7a472120193bd56 drm/i915: Ignore panel type
from OpRegion on SKL
:: TO: Ville Syrjälä
:: CC: Daniel Vetter
---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
.config.gz
Description: Binary data
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH driver/intel] uxa: Don't bother with xf86GARTCloseScreen
We only ever use xserver's AGP support from the i810 driver. Signed-off-by: Adam Jackson --- src/uxa/intel_driver.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/uxa/intel_driver.c b/src/uxa/intel_driver.c index 73d7f4f..62abdd2 100644 --- a/src/uxa/intel_driver.c +++ b/src/uxa/intel_driver.c @@ -1172,8 +1172,6 @@ static Bool I830CloseScreen(CLOSE_SCREEN_ARGS_DECL) intel_sync_close(screen); - xf86GARTCloseScreen(scrn->scrnIndex); - scrn->vtSema = FALSE; return TRUE; } -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
On Thu, Jul 14, 2016 at 04:36:37PM +0200, Daniel Vetter wrote: > On Thu, Jul 14, 2016 at 02:39:54PM +0100, Chris Wilson wrote: > > On Thu, Jul 14, 2016 at 02:23:04PM +0100, Chris Wilson wrote: > > > The biggest reason I had against going the sw_sync only route was that > > > vgem should provide unprivileged fences and that through the bookkeeping > > > in vgem we can keep them safe, ensure that we don't leak random buffers > > > or fences. (And I need a source of foriegn dma-buf with implicit fence > > > tracking with which I can try and break the driver.) > > > > And for testing passing around content + fences is more useful than > > passing fences alone. > > Yup, agreed. But having fences free-standing isn't a real issue since > their refcounted and the userspace parts (sync_file) will get cleaned up > on process exit latest. Ḯ'm not advocating for any behaviour change at > all, just for hiding these things in debugfs. It's just a choice of api. We could equally hide it behind a separate config flag. First question, are we happy that there is a legitimate usecase for fences on vgem? If so, what enforced timeout on the fence should we use? (I think that this ioctl api is correct, I don't forsee sw_sync being viable for unprivileged use.) Then we can restrict this patch to add the safe interface, enable a bunch more tests and get on with discussing how to break the kernel "safely"! -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring
On Thu, Jul 14, 2016 at 01:47:49PM +0300, Mika Kuoppala wrote: > Chris Wilson writes: > > > Since the suspend_work can arm itself if the console_lock() is currently > > held elsewhere, simply calling flush_work() doesn't guarantee that the > > work is idle upon return. To do so requires using cancel_work_sync(). > > > > Signed-off-by: Chris Wilson > > Cc: Daniel Vetter > > Cc: Mika Kuoppala > > Reviewed-by: Mika Kuoppala Cc: sta...@vger.kernel.org I presume? Checking for this should be part of the review ... -Daniel > > > --- > > drivers/gpu/drm/i915/intel_fbdev.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > b/drivers/gpu/drm/i915/intel_fbdev.c > > index 86b00c6db1a6..ef17d88a1bc7 100644 > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > @@ -768,7 +768,7 @@ void intel_fbdev_fini(struct drm_device *dev) > > if (!ifbdev) > > return; > > > > - flush_work(&dev_priv->fbdev_suspend_work); > > + cancel_work_sync(&dev_priv->fbdev_suspend_work); > > if (!current_is_async()) > > intel_fbdev_sync(ifbdev); > > > > -- > > 2.8.1 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring
On Thu, Jul 14, 2016 at 04:45:52PM +0200, Daniel Vetter wrote: > On Thu, Jul 14, 2016 at 01:47:49PM +0300, Mika Kuoppala wrote: > > Chris Wilson writes: > > > > > Since the suspend_work can arm itself if the console_lock() is currently > > > held elsewhere, simply calling flush_work() doesn't guarantee that the > > > work is idle upon return. To do so requires using cancel_work_sync(). > > > > > > Signed-off-by: Chris Wilson > > > Cc: Daniel Vetter > > > Cc: Mika Kuoppala > > > > Reviewed-by: Mika Kuoppala > > Cc: sta...@vger.kernel.org I presume? Checking for this should be part of > the review ... No known bug - unlikely since ~nobody unloads the module. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/fbdev: Check for the framebuffer before use
On Wed, Jul 13, 2016 at 06:34:45PM +0100, Chris Wilson wrote:
> If the fbdev probing fails, and in our error path we fail to clear the
> dev_priv->fbdev, then we can try and use a dangling fbdev pointer, and
> in particular a NULL fb. This could also happen in pathological cases
> where we try to operate on the fbdev prior to it being probed.
>
> Reported-by: Maarten Lankhorst
> Signed-off-by: Chris Wilson
> Cc: Maarten Lankhorst
> Cc: Daniel Vetter
> Cc: Mika Kuoppala
Thierry Redding has a patch series in flight for generic delayed fbdev
setup, whether it's delayed to due async init or because there's nothing
yet connected (which some of the embedded drivers hack together). With
those patches most of these checks should become unecessary again.
Adding Thierry so he's aware that there's more to do when rebasing, and so
he knows where he can find reviewers ;-)
Reviewed-by: Daniel Vetter
> ---
> drivers/gpu/drm/i915/intel_fbdev.c | 25 +++--
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
> b/drivers/gpu/drm/i915/intel_fbdev.c
> index ef17d88a1bc7..23129dcfba9d 100644
> --- a/drivers/gpu/drm/i915/intel_fbdev.c
> +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> @@ -782,7 +782,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int
> state, bool synchronous
> struct intel_fbdev *ifbdev = dev_priv->fbdev;
> struct fb_info *info;
>
> - if (!ifbdev)
> + if (!ifbdev || !ifbdev->fb)
> return;
>
> info = ifbdev->helper.fbdev;
> @@ -827,31 +827,28 @@ void intel_fbdev_set_suspend(struct drm_device *dev,
> int state, bool synchronous
>
> void intel_fbdev_output_poll_changed(struct drm_device *dev)
> {
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - if (dev_priv->fbdev)
> - drm_fb_helper_hotplug_event(&dev_priv->fbdev->helper);
> + struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
> +
> + if (ifbdev && ifbdev->fb)
> + drm_fb_helper_hotplug_event(&ifbdev->helper);
> }
>
> void intel_fbdev_restore_mode(struct drm_device *dev)
> {
> - int ret;
> - struct drm_i915_private *dev_priv = to_i915(dev);
> - struct intel_fbdev *ifbdev = dev_priv->fbdev;
> - struct drm_fb_helper *fb_helper;
> + struct intel_fbdev *ifbdev = to_i915(dev)->fbdev;
>
> if (!ifbdev)
> return;
>
> intel_fbdev_sync(ifbdev);
> + if (!ifbdev->fb)
> + return;
>
> - fb_helper = &ifbdev->helper;
> -
> - ret = drm_fb_helper_restore_fbdev_mode_unlocked(fb_helper);
> - if (ret) {
> + if (drm_fb_helper_restore_fbdev_mode_unlocked(&ifbdev->helper)) {
> DRM_DEBUG("failed to restore crtc mode\n");
> } else {
> - mutex_lock(&fb_helper->dev->struct_mutex);
> + mutex_lock(&dev->struct_mutex);
> intel_fb_obj_invalidate(ifbdev->fb->obj, ORIGIN_GTT);
> - mutex_unlock(&fb_helper->dev->struct_mutex);
> + mutex_unlock(&dev->struct_mutex);
> }
> }
> --
> 2.8.1
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Wait up to 3ms for the pcu to ack the cdclk change request on SKL
On Wed, Jul 13, 2016 at 04:32:03PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä
>
> Bspec tells us to keep bashing the PCU for up to 3ms when trying to
> inform it about an upcoming change in the cdclk frequency. Currently
> we only keep at it for 15*10usec (+ whatever delays gets added by
> the sandybridge_pcode_read() itself). Let's change the limit to 3ms.
>
> I decided to keep 10 usec delay per iteration for now, even though
> the spec doesn't really tell us to do that.
>
> Cc: David Weinehall
> Signed-off-by: Ville Syrjälä
Needs
Cc: sta...@vger.kernel.org
and probably also a Fixes: line?
-Daniel
> ---
> drivers/gpu/drm/i915/intel_display.c | 10 +-
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index be3b2cab2640..90f26f0e2571 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5691,15 +5691,7 @@ static bool skl_cdclk_pcu_ready(struct
> drm_i915_private *dev_priv)
>
> static bool skl_cdclk_wait_for_pcu_ready(struct drm_i915_private *dev_priv)
> {
> - unsigned int i;
> -
> - for (i = 0; i < 15; i++) {
> - if (skl_cdclk_pcu_ready(dev_priv))
> - return true;
> - udelay(10);
> - }
> -
> - return false;
> + return _wait_for(skl_cdclk_pcu_ready(dev_priv), 3000, 10) == 0;
> }
>
> static void skl_set_cdclk(struct drm_i915_private *dev_priv, int cdclk, int
> vco)
> --
> 2.7.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/6] drm/i915/huc: Add HuC fw loading support
On Thu, 14 Jul 2016, Daniel Vetter wrote: On Wed, Jul 13, 2016 at 03:52:39PM +0100, Peter Antoine wrote: On Wed, 13 Jul 2016, Daniel Vetter wrote: On Thu, Jun 23, 2016 at 02:52:41PM +0100, Peter Antoine wrote: On Thu, 23 Jun 2016, Dave Gordon wrote: On 22/06/16 09:31, Daniel Vetter wrote: No, the *correct* fix is to unify all the firmware loaders we have. There should just be ONE piece of code that can be used to fetch and load ANy firmware into ANY auxiliary microcontroller. NOT one per microcontroller, all different -- that way lies madness. We already had a unified loader for the HuC and GuC a year ago, but IIRC the party line then was "just make it (GuC) specific, then copypaste it for the second uC, and when we've got three versions we'll have learnt how we really want a unified loader to behave." Well. here's the copypaste, and we already have a different loader for the DMC/CSR, so it must be time for (re-)unification. .Dave. Just to add, if you uc_fw_fetch() has an error code you will still have to remember the state of the fetch or at each reset/resume/etc... or you will have to try the firmware load again and that can take a long time. So the state will have to be re-instated. Seeing this code was written with the given goals and were written in the same vane as code that was deemed acceptable, it seems weird at this late stage to change the design goals. Note: this is the third time that these patches have been posted and were only rejected (as far as I know) due to no open-source user. Which there is now, and is why I have reposted these patches. I never liked the guc firmware code, but figure for one copy it's not worth fighting over. Adding more copies (or perpetuating the design by making it generic) isn't what I'm looking for. Firmware loading shouldn't be that complicated, really. The unified firmware loader is called request_firmware. If that's not good enough, pls fix the core function, not paper code over in i915. In that regard DMC/CSR is unified, everything else isn't yet. Iirc the big issue is delayed firmware loading for built-in i915 and fw only available later on. This is an open issue in request_firmware() since years, and there's various patches floating around. If the problem is that Greg KH doesn't consider those patches, I can help with that. But not pushing the core fix forward isn't acceptable imo. Once that fix is landed we can treat request_firmware as reliable (it might take a while, hence must be run in an async work like DMC loading), with no need to ever retry anything. If fw loading fails we can just mark the entire render part of the gpu as dead by injecting the equivalent of a non-recoverable hang (async setup) or failing engine init with -EIO (if this is still synchronous, which I don't expect really). If there's another reason for this complexity, please explain since I'd like to understand why we need this. -Daniel I was not involved at the start and I am porting code that others have written, but as far as I understand this, you requested that the code be duplicated. See Dave's comment above as he was involved. The code uses request_firmware() to handle the load of the firmware into memory and the rest of this code manages the loading of that memory into the HuC's SRAM. This needs extra setup that should not really go into the generic firmware loader (one loader for uIA's make sense as this will futureproof the code). Also the SRAM is write-once memory and needs to be handled correctly. Also, the GuC needs to verify the HuC some this becomes a little be more fun. Also, again you are ignoring the point that not having firmware is not fatal. We remember the state of the load as this is required to save time when we come out of reset to not waste time trying to reload the firmware, if it has failed already. They are other mechinums to do this, but they will always need some form of history. Also, if request_firmware() is broken why has this not been fixed? If it has been broken for "years" why and how do you expect to to be fixed now? If the "not pushing the core fix is not acceptable" why has that not been done? Apparently because I'm a too nice maintainer and allowed half-solutions to get landed, under the expecations that people would indeed follow up and fix things. And yes, you care, you fix it, is how this works, whether you like it or not. -Daniel Daniel, I won't have to time to do this. So I'll leave the HuC/GuC rewrite for you to find someone else to take them on as this is likely to take more time than I will be employed by Intel. Peter. -- Peter Antoine (Android Graphics Driver Software Engineer) - Intel Corporation (UK) Limited Registered No. 1134945 (England) Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org htt
Re: [Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
On Thu, Jul 14, 2016 at 02:39:54PM +0100, Chris Wilson wrote: > On Thu, Jul 14, 2016 at 02:23:04PM +0100, Chris Wilson wrote: > > The biggest reason I had against going the sw_sync only route was that > > vgem should provide unprivileged fences and that through the bookkeeping > > in vgem we can keep them safe, ensure that we don't leak random buffers > > or fences. (And I need a source of foriegn dma-buf with implicit fence > > tracking with which I can try and break the driver.) > > And for testing passing around content + fences is more useful than > passing fences alone. Yup, agreed. But having fences free-standing isn't a real issue since their refcounted and the userspace parts (sync_file) will get cleaned up on process exit latest. Ḯ'm not advocating for any behaviour change at all, just for hiding these things in debugfs. Or maybe we could add a special (tainting) module option to vgem.ko which enables this interface? That would be even less work, can easily be integrated into igt (just set that knob at runtime, done), and with a stern enough warning in dmesg + tainting the point should be clear. Of course that switch would be off by default. Thoughts? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/6] drm/i915/huc: Add HuC fw loading support
On 14/07/16 15:16, Daniel Vetter wrote: On Wed, Jul 13, 2016 at 03:52:39PM +0100, Peter Antoine wrote: On Wed, 13 Jul 2016, Daniel Vetter wrote: On Thu, Jun 23, 2016 at 02:52:41PM +0100, Peter Antoine wrote: On Thu, 23 Jun 2016, Dave Gordon wrote: On 22/06/16 09:31, Daniel Vetter wrote: No, the *correct* fix is to unify all the firmware loaders we have. There should just be ONE piece of code that can be used to fetch and load ANy firmware into ANY auxiliary microcontroller. NOT one per microcontroller, all different -- that way lies madness. We already had a unified loader for the HuC and GuC a year ago, but IIRC the party line then was "just make it (GuC) specific, then copypaste it for the second uC, and when we've got three versions we'll have learnt how we really want a unified loader to behave." Well. here's the copypaste, and we already have a different loader for the DMC/CSR, so it must be time for (re-)unification. .Dave. Just to add, if you uc_fw_fetch() has an error code you will still have to remember the state of the fetch or at each reset/resume/etc... or you will have to try the firmware load again and that can take a long time. So the state will have to be re-instated. Seeing this code was written with the given goals and were written in the same vane as code that was deemed acceptable, it seems weird at this late stage to change the design goals. Note: this is the third time that these patches have been posted and were only rejected (as far as I know) due to no open-source user. Which there is now, and is why I have reposted these patches. I never liked the guc firmware code, but figure for one copy it's not worth fighting over. Adding more copies (or perpetuating the design by making it generic) isn't what I'm looking for. Firmware loading shouldn't be that complicated, really. The unified firmware loader is called request_firmware. If that's not good enough, pls fix the core function, not paper code over in i915. In that regard DMC/CSR is unified, everything else isn't yet. Iirc the big issue is delayed firmware loading for built-in i915 and fw only available later on. This is an open issue in request_firmware() since years, and there's various patches floating around. If the problem is that Greg KH doesn't consider those patches, I can help with that. But not pushing the core fix forward isn't acceptable imo. Once that fix is landed we can treat request_firmware as reliable (it might take a while, hence must be run in an async work like DMC loading), with no need to ever retry anything. If fw loading fails we can just mark the entire render part of the gpu as dead by injecting the equivalent of a non-recoverable hang (async setup) or failing engine init with -EIO (if this is still synchronous, which I don't expect really). If there's another reason for this complexity, please explain since I'd like to understand why we need this. -Daniel I was not involved at the start and I am porting code that others have written, but as far as I understand this, you requested that the code be duplicated. See Dave's comment above as he was involved. The code uses request_firmware() to handle the load of the firmware into memory and the rest of this code manages the loading of that memory into the HuC's SRAM. This needs extra setup that should not really go into the generic firmware loader (one loader for uIA's make sense as this will futureproof the code). Also the SRAM is write-once memory and needs to be handled correctly. Also, the GuC needs to verify the HuC some this becomes a little be more fun. Also, again you are ignoring the point that not having firmware is not fatal. We remember the state of the load as this is required to save time when we come out of reset to not waste time trying to reload the firmware, if it has failed already. They are other mechinums to do this, but they will always need some form of history. Also, if request_firmware() is broken why has this not been fixed? If it has been broken for "years" why and how do you expect to to be fixed now? If the "not pushing the core fix is not acceptable" why has that not been done? Apparently because I'm a too nice maintainer and allowed half-solutions to get landed, under the expecations that people would indeed follow up and fix things. And yes, you care, you fix it, is how this works, whether you like it or not. -Daniel AFAIK request_firmware() is *not* broken. It does what it says i.e. fetches a blob from the internal bucket or from the filesystem; and fails if it's not found. So it doesn't need fixing. What's broken (from the Android POV) is i915 needing to do too much too early i.e. before all filesystems are mounted. The answer to *that* is to defer *all* engine initialisation until (much) later; but that's nothing at all to do with Peter's patches here. .Dave. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org
Re: [Intel-gfx] [PATCH] drm/i915: Acquire intel_runtime_pm for HD-Audio registers
On Wed, Jul 13, 2016 at 05:01:02PM +0300, Marius Vlad wrote: > Did try when you submitted the patch...but can't seem to replicate with > latest nightly on other SKLs, and currently do not have access on > the machine that caused it. So fwiw, Hans de Geode confirmed that only reverting 03b135cebc47 (which enables the display powerwell as well) was enough. Libin, do you want to handle the revert or explain the mistake in commit 03b135cebc47d75ea2dc346770374ab741966955 Author: Libin Yang Date: Wed Jun 3 09:30:15 2015 +0800 ALSA: hda - remove controller dependency on i915 power well for SKL -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
On Thu, Jul 14, 2016 at 02:23:04PM +0100, Chris Wilson wrote: > On Thu, Jul 14, 2016 at 02:40:59PM +0200, Daniel Vetter wrote: > > On Thu, Jul 14, 2016 at 11:11:02AM +0100, Chris Wilson wrote: > > > On Thu, Jul 14, 2016 at 10:59:04AM +0100, Chris Wilson wrote: > > > > On Thu, Jul 14, 2016 at 10:12:17AM +0200, Daniel Vetter wrote: > > > > > On Thu, Jul 14, 2016 at 08:04:19AM +0100, Chris Wilson wrote: > > > > > > vGEM buffers are useful for passing data between software clients > > > > > > and > > > > > > hardware renders. By allowing the user to create and attach fences > > > > > > to > > > > > > the exported vGEM buffers (on the dma-buf), the user can implement a > > > > > > deferred renderer and queue hardware operations like flipping and > > > > > > then > > > > > > signal the buffer readiness (i.e. this allows the user to schedule > > > > > > operations out-of-order, but have them complete in-order). > > > > > > > > > > > > This also makes it much easier to write tightly controlled > > > > > > testcases for > > > > > > dma-buf fencing and signaling between hardware drivers. > > > > > > > > > > > > v2: Don't pretend the fences exist in an ordered timeline, but > > > > > > allocate > > > > > > a separate fence-context for each fence so that the fences are > > > > > > unordered. > > > > > > v3: Make the debug output more interesting, and so the signaled > > > > > > status. > > > > > > > > > > > > Testcase: igt/vgem_basic/dmabuf-fence > > > > > > Signed-off-by: Chris Wilson > > > > > > Cc: Sean Paul > > > > > > Cc: Zach Reizner > > > > > > Cc: Gustavo Padovan > > > > > > Cc: Daniel Vetter > > > > > > Acked-by: Zach Reizner > > > > > > > > > > One thing I completely forgotten: This allows userspace to hang kernel > > > > > drivers. i915 (and other gpu drivers) can recover using hangcheck, but > > > > > dumber drivers (v4l, if that ever happens) probably never except such > > > > > a > > > > > case. We've had a similar discusion with the userspace fences exposed > > > > > in > > > > > sw_fence, and decided to move all those ioctl into debugfs. I think we > > > > > should do the same for this vgem-based debugging of implicit sync. > > > > > Sorry > > > > > for realizing this this late. > > > > > > > > One of the very tests I make is to ensure that we recover from such a > > > > hang. I don't see the difference between this any of the other ways > > > > userspace can shoot itself (and others) in the foot. > > > > > > So one solution would be to make vgem fences automatically timeout (with > > > a flag for root to override for the sake of testing hang detection). > > > > The problem is other drivers. E.g. right now atomic helpers assume that > > fences will signal, and can't recover if they don't. This is why drivers > > where things might fail must have some recovery (hangcheck, timeout) to > > make sure dma_fences always signal. > > Urm, all the atomic helpers should work with fails. The waits on dma-buf > should be before any hardware is modified and so cancellation is trivial. > Anyone using a foriegn fence (or even native) must cope that it may not > meet some deadline. > > They have to. Anyone sharing a i915 dma-buf is susceptible to all kinds > of (unprivileged) fun. > > > Imo not even root should be allowed to break this, since it could put > > drivers into a non-recoverable state. I think this must be restricted to > > something known-unsafe-don't-enable-on-production like debugfs. > > Providing fences is extremely useful, even for software buffers. (For > the sake of argument, just imagine an asynchronous multithreaded llvmpipe > wanting to support client fences for deferred rendering.) The only > question in my mind is how much cotton wool to use. > > > Other solutions which I don't like: > > - Everyone needs to be able to recover. Given how much effort it is to > > just keep i915 hangcheck in working order I think that's totally > > illusionary to assume. At least once world+dog (atomic, v4l, ...) all > > consume/produce fences, subsystems where the usual assumption holds that > > async ops complete. > > > > - Really long timeouts are allowed for root in vgem. Could lead to even > > more fun in testing i915 hangchecks I think, so don't like that much > > either. > > The whole point is in testing our handling before we become suspectible > to real world fail - because as you point out, not everyone guarantees > that a fence will be signaled. I can't simply pass around i915 dma-buf > simply because we may unwind them and in the process completely curtail > being able to test a foriegn fence that hangs. I think that's where we differ in opinion: Right now we do have the guarantee that every fence gets signalled in finite time. For drivers where that is not just guaranteed there must be a hangcheck to force the completion. The only exception thus far is the debugfs-only sw_fence interface. -Daniel > > > I think the best option is to just do the same as we've
Re: [Intel-gfx] [PATCH 1/5] drm/i915: unify first-stage engine struct setup
On Wed, Jul 13, 2016 at 02:24:06PM +0100, Tvrtko Ursulin wrote:
>
> On 13/07/16 14:16, Tvrtko Ursulin wrote:
> >
> > On 13/07/16 13:23, Daniel Vetter wrote:
> > > On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
> > > > From: Dave Gordon
> > > >
> > > > intel_lrc.c has a table of "logical rings" (meaning engines), while
> > > > intel_ringbuffer.c has separately open-coded initialisation for each
> > > > engine. We can deduplicate this somewhat by using the same first-stage
> > > > engine-setup function for both modes.
> > > >
> > > > So here we expose the function that transfers information from the
> > > > static table of (all) known engines to the dev_priv->engine array of
> > > > engines available on this device (adjusting the names along the way)
> > > > and then embed calls to it in both the LRC and the legacy-mode setup.
> > > >
> > > > Signed-off-by: Dave Gordon
> > > > ---
> > > > drivers/gpu/drm/i915/intel_lrc.c| 40
> > > > +
> > > > drivers/gpu/drm/i915/intel_ringbuffer.c | 40
> > > > +
> > > > drivers/gpu/drm/i915/intel_ringbuffer.h | 5 +
> > > > 3 files changed, 41 insertions(+), 44 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> > > > b/drivers/gpu/drm/i915/intel_lrc.c
> > > > index 339d8041075f..ed017f1a07a2 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lrc.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lrc.c
> > > > @@ -1994,8 +1994,9 @@ logical_ring_default_vfuncs(struct
> > > > intel_engine_cs *engine)
> > > > }
> > > >
> > > > static inline void
> > > > -logical_ring_default_irqs(struct intel_engine_cs *engine, unsigned
> > > > shift)
> > > > +logical_ring_default_irqs(struct intel_engine_cs *engine)
> > > > {
> > > > +unsigned shift = engine->irq_shift;
> > > > engine->irq_enable_mask = GT_RENDER_USER_INTERRUPT << shift;
> > > > engine->irq_keep_mask = GT_CONTEXT_SWITCH_INTERRUPT << shift;
> > > > init_waitqueue_head(&engine->irq_queue);
> > > > @@ -2096,14 +2097,14 @@ static int logical_render_ring_init(struct
> > > > intel_engine_cs *engine)
> > > > return ret;
> > > > }
> > > >
> > > > -static const struct logical_ring_info {
> > > > +static const struct engine_info {
> > > > const char *name;
> > > > unsigned exec_id;
> > > > unsigned guc_id;
> > > > u32 mmio_base;
> > > > unsigned irq_shift;
> > > > int (*init)(struct intel_engine_cs *engine);
> > > > -} logical_rings[] = {
> > > > +} intel_engines[] = {
> > > > [RCS] = {
> > > > .name = "render ring",
> > > > .exec_id = I915_EXEC_RENDER,
> > > > @@ -2146,20 +2147,31 @@ static const struct logical_ring_info {
> > > > },
> > > > };
> > > >
> > > > -static struct intel_engine_cs *
> > > > -logical_ring_setup(struct drm_i915_private *dev_priv, enum
> > > > intel_engine_id id)
> > > > +struct intel_engine_cs *
> > > > +intel_engine_setup(struct drm_i915_private *dev_priv,
> > > > + enum intel_engine_id id)
> > >
> > > Kerneldoc for this would be nice. Also, we now have a mess between
> > > intel_lrc.c and intel_ringbuffer.c. Extracting intel_engine.c with the
> >
> > Yes that was already suggested by Chris and I am trybotting the
> > additions today, again on his explicit request to progress this series.
> >
> > > shared bits or something similar, plus cleanup up all the docs would be
> > > awesome as a follow up.
> >
> > What do you mean "all the docs"? :D
> >
> > >
> > > With the kerneldoc added:
> > >
> > > Reviewed-by: Daniel Vetter
> >
> > Thanks, will add.
>
> Actually the function becomes static later in the series, when the common
> stuff is moved into intel_engine_cs.c.
>
> Can I keep the r-b without adding kernel doc for it then?
Sure.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/6] drm/i915/huc: Add HuC fw loading support
On Thu, Jul 14, 2016 at 03:08:41PM +0100, Dave Gordon wrote:
> On 13/07/16 13:48, Daniel Vetter wrote:
> > On Thu, Jun 23, 2016 at 02:52:41PM +0100, Peter Antoine wrote:
> > > On Thu, 23 Jun 2016, Dave Gordon wrote:
> > > > On 22/06/16 09:31, Daniel Vetter wrote:
> > > > No, the *correct* fix is to unify all the firmware loaders we have.
> > > > There should just be ONE piece of code that can be used to fetch and
> > > > load ANy firmware into ANY auxiliary microcontroller. NOT one per
> > > > microcontroller, all different -- that way lies madness.
> > > >
> > > > We already had a unified loader for the HuC and GuC a year ago, but IIRC
> > > > the party line then was "just make it (GuC) specific, then copypaste it
> > > > for the second uC, and when we've got three versions we'll have learnt
> > > > how we really want a unified loader to behave."
> > > >
> > > > Well. here's the copypaste, and we already have a different loader for
> > > > the DMC/CSR, so it must be time for (re-)unification.
> > > >
> > > > .Dave.
> > >
> > > Just to add, if you uc_fw_fetch() has an error code you will still have to
> > > remember the state of the fetch or at each reset/resume/etc... or you will
> > > have to try the firmware load again and that can take a long time. So the
> > > state will have to be re-instated.
> > >
> > > Seeing this code was written with the given goals and were written in the
> > > same vane as code that was deemed acceptable, it seems weird at this late
> > > stage to change the design goals.
> > >
> > > Note: this is the third time that these patches have been posted and were
> > > only rejected (as far as I know) due to no open-source user. Which there
> > > is
> > > now, and is why I have reposted these patches.
> >
> > I never liked the guc firmware code, but figure for one copy it's not
> > worth fighting over. Adding more copies (or perpetuating the design by
> > making it generic) isn't what I'm looking for.
>
> *You* asked for more copies, back when we proposed a single unified solution
> last year. We already had a *single* GuC+HuC loader which could also have
> been extended to support the DMC as well, but at the time you wanted a
> GuC-specific version -- and by implication, a separate HuC loader -- *in
> addition to* the DMC loader.
>
> > Firmware loading shouldn't be that complicated, really.
>
> Maybe it shouldn't be, and maybe it isn't -- you may not be seeing how
> simple this code actually is. Fetch firmware, validate it, save it in a GEM
> object; later, DMA it to the h/w; at each stage keep track of status so we
> know what has been done and what is still to do (or redo, during reset).
>
> Any complications are because the h/w (e.g. write-once memory) makes them
> necessary, or artefacts of the GEM object system, or because of the driver's
> byzantine sequence of operations during load/reset/suspend/resume/unload.
>
> > The unified firmware loader is called request_firmware. If that's not good
> > enough, pls fix the core function, not paper code over in i915.
>
> That's exactly the function we call. Then we have to validate and save the
> blob. And remember that we've done so.
>
> > In that regard DMC/CSR is unified, everything else isn't yet.
>
> Unified with what? Maybe the "DMC" is unified with the "CSR" -- which AFAIK
> are the same thing -- and the software just randomly uses both names to
> maximise confusion?
>
> if (HAS_CSR(dev)) {
> struct intel_csr *csr = &dev_priv->csr;
>
> err_printf(m, "DMC loaded: %s\n",
> yesno(csr->dmc_payload != NULL));
> err_printf(m, "DMC fw version: %d.%d\n",
> CSR_VERSION_MAJOR(csr->version),
> CSR_VERSION_MINOR(csr->version));
> }
> ...
> if (!IS_GEN9(dev_priv)) {
> DRM_ERROR("No CSR support available for this platform\n");
> return;
> }
> if (!dev_priv->csr.dmc_payload) {
> DRM_ERROR("Tried to program CSR with empty payload\n");
> return;
> }
>
> And according to the comments in intel_csr.c -- but not the code --
>
> /*
> * Firmware loading status will be one of the below states:
> * FW_UNINITIALIZED, FW_LOADED, FW_FAILED.
> *
> * Once the firmware is written into the registers status will
> * be moved from FW_UNINITIALIZED to FW_LOADED and for any
> * erroneous condition status will be moved to FW_FAILED.
> */
>
> So I don't think you should hold this code up as a masterpiece of "unified"
> design -- which in any case you argued against last year, when we presented
> a unified loader. Specifically, you said, "In my experience trying to
> extract common code at all costs is harmful way too often."
>
> Also, the approach taken in the DMC loader -- which appears to have been
> copypasted from a /very early/ version of the GuC loader, before I fixed the
> async-load problems -- just wouldn't work for the HuC
Re: [Intel-gfx] [PATCH 3/6] drm/i915/huc: Add HuC fw loading support
On Wed, Jul 13, 2016 at 03:52:39PM +0100, Peter Antoine wrote: > On Wed, 13 Jul 2016, Daniel Vetter wrote: > > > On Thu, Jun 23, 2016 at 02:52:41PM +0100, Peter Antoine wrote: > > > On Thu, 23 Jun 2016, Dave Gordon wrote: > > > > On 22/06/16 09:31, Daniel Vetter wrote: > > > > No, the *correct* fix is to unify all the firmware loaders we have. > > > > There should just be ONE piece of code that can be used to fetch and > > > > load ANy firmware into ANY auxiliary microcontroller. NOT one per > > > > microcontroller, all different -- that way lies madness. > > > > > > > > We already had a unified loader for the HuC and GuC a year ago, but IIRC > > > > the party line then was "just make it (GuC) specific, then copypaste it > > > > for the second uC, and when we've got three versions we'll have learnt > > > > how we really want a unified loader to behave." > > > > > > > > Well. here's the copypaste, and we already have a different loader for > > > > the DMC/CSR, so it must be time for (re-)unification. > > > > > > > > .Dave. > > > > > > > > > > Just to add, if you uc_fw_fetch() has an error code you will still have to > > > remember the state of the fetch or at each reset/resume/etc... or you will > > > have to try the firmware load again and that can take a long time. So the > > > state will have to be re-instated. > > > > > > Seeing this code was written with the given goals and were written in the > > > same vane as code that was deemed acceptable, it seems weird at this late > > > stage to change the design goals. > > > > > > Note: this is the third time that these patches have been posted and were > > > only rejected (as far as I know) due to no open-source user. Which there > > > is > > > now, and is why I have reposted these patches. > > > > I never liked the guc firmware code, but figure for one copy it's not > > worth fighting over. Adding more copies (or perpetuating the design by > > making it generic) isn't what I'm looking for. Firmware loading shouldn't > > be that complicated, really. > > > > The unified firmware loader is called request_firmware. If that's not good > > enough, pls fix the core function, not paper code over in i915. In that > > regard DMC/CSR is unified, everything else isn't yet. > > > > Iirc the big issue is delayed firmware loading for built-in i915 and fw > > only available later on. This is an open issue in request_firmware() since > > years, and there's various patches floating around. If the problem is that > > Greg KH doesn't consider those patches, I can help with that. But not > > pushing the core fix forward isn't acceptable imo. Once that fix is landed > > we can treat request_firmware as reliable (it might take a while, hence > > must be run in an async work like DMC loading), with no need to ever retry > > anything. If fw loading fails we can just mark the entire render part of > > the gpu as dead by injecting the equivalent of a non-recoverable hang > > (async setup) or failing engine init with -EIO (if this is still > > synchronous, which I don't expect really). > > > > If there's another reason for this complexity, please explain since I'd > > like to understand why we need this. > > -Daniel > > > > I was not involved at the start and I am porting code that others have > written, but as far as I understand this, you requested that the code be > duplicated. See Dave's comment above as he was involved. > > The code uses request_firmware() to handle the load of the firmware into > memory and the rest of this code manages the loading of that memory into the > HuC's SRAM. This needs extra setup that should not really go into the > generic firmware loader (one loader for uIA's make sense as this will > futureproof the code). Also the SRAM is write-once memory and needs to be > handled correctly. Also, the GuC needs to verify the HuC some this becomes a > little be more fun. > > Also, again you are ignoring the point that not having firmware is not > fatal. We remember the state of the load as this is required to save time > when we come out of reset to not waste time trying to reload the firmware, > if it has failed already. They are other mechinums to do this, but they will > always need some form of history. > > Also, if request_firmware() is broken why has this not been fixed? If it has > been broken for "years" why and how do you expect to to be fixed now? If the > "not pushing the core fix is not acceptable" why has that not been done? Apparently because I'm a too nice maintainer and allowed half-solutions to get landed, under the expecations that people would indeed follow up and fix things. And yes, you care, you fix it, is how this works, whether you like it or not. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Remove misleading CSR firmware loading docs
I forgot to remove these when reworking the firmware loading sequence last year. The new sequence is that we load firmware, and if it's not there we entirely (and permanently) fail dmc setup. Reported-by: Dave Gordon Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_csr.c | 7 --- 1 file changed, 7 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c index c3b33a10c15c..1ea0e1f43397 100644 --- a/drivers/gpu/drm/i915/intel_csr.c +++ b/drivers/gpu/drm/i915/intel_csr.c @@ -32,13 +32,6 @@ * onwards to drive newly added DMC (Display microcontroller) in display * engine to save and restore the state of display engine when it enter into * low-power state and comes back to normal. - * - * Firmware loading status will be one of the below states: FW_UNINITIALIZED, - * FW_LOADED, FW_FAILED. - * - * Once the firmware is written into the registers status will be moved from - * FW_UNINITIALIZED to FW_LOADED and for any erroneous condition status will - * be moved to FW_FAILED. */ #define I915_CSR_KBL "i915/kbl_dmc_ver1_01.bin" -- 2.8.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Move hangcheck reset from out of the engines into the GPU reset
On Thu, Jul 14, 2016 at 04:59:56PM +0300, Mika Kuoppala wrote: > Chris Wilson writes: > > > Let's not reset the hangcheck as a side-effect of initialising the > > engine, but as part of our GPU reset. > > > > I think TDR will need both the init and the reset. It doesn't. This state is the purely global state for hangcheck. There's probably a better spot, such as hangcheck expiration, but at the moment this is better. TDR will be keeping its own data structures and should not stop the global hangcheck from stepping in and saying enough is enough. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/6] drm/i915/huc: Add HuC fw loading support
On 13/07/16 13:48, Daniel Vetter wrote:
On Thu, Jun 23, 2016 at 02:52:41PM +0100, Peter Antoine wrote:
On Thu, 23 Jun 2016, Dave Gordon wrote:
On 22/06/16 09:31, Daniel Vetter wrote:
No, the *correct* fix is to unify all the firmware loaders we have.
There should just be ONE piece of code that can be used to fetch and
load ANy firmware into ANY auxiliary microcontroller. NOT one per
microcontroller, all different -- that way lies madness.
We already had a unified loader for the HuC and GuC a year ago, but IIRC
the party line then was "just make it (GuC) specific, then copypaste it
for the second uC, and when we've got three versions we'll have learnt
how we really want a unified loader to behave."
Well. here's the copypaste, and we already have a different loader for
the DMC/CSR, so it must be time for (re-)unification.
.Dave.
Just to add, if you uc_fw_fetch() has an error code you will still have to
remember the state of the fetch or at each reset/resume/etc... or you will
have to try the firmware load again and that can take a long time. So the
state will have to be re-instated.
Seeing this code was written with the given goals and were written in the
same vane as code that was deemed acceptable, it seems weird at this late
stage to change the design goals.
Note: this is the third time that these patches have been posted and were
only rejected (as far as I know) due to no open-source user. Which there is
now, and is why I have reposted these patches.
I never liked the guc firmware code, but figure for one copy it's not
worth fighting over. Adding more copies (or perpetuating the design by
making it generic) isn't what I'm looking for.
*You* asked for more copies, back when we proposed a single unified
solution last year. We already had a *single* GuC+HuC loader which could
also have been extended to support the DMC as well, but at the time you
wanted a GuC-specific version -- and by implication, a separate HuC
loader -- *in addition to* the DMC loader.
> Firmware loading shouldn't be that complicated, really.
Maybe it shouldn't be, and maybe it isn't -- you may not be seeing how
simple this code actually is. Fetch firmware, validate it, save it in a
GEM object; later, DMA it to the h/w; at each stage keep track of status
so we know what has been done and what is still to do (or redo, during
reset).
Any complications are because the h/w (e.g. write-once memory) makes
them necessary, or artefacts of the GEM object system, or because of the
driver's byzantine sequence of operations during
load/reset/suspend/resume/unload.
The unified firmware loader is called request_firmware. If that's not good
enough, pls fix the core function, not paper code over in i915.
That's exactly the function we call. Then we have to validate and save
the blob. And remember that we've done so.
In that regard DMC/CSR is unified, everything else isn't yet.
Unified with what? Maybe the "DMC" is unified with the "CSR" -- which
AFAIK are the same thing -- and the software just randomly uses both
names to maximise confusion?
if (HAS_CSR(dev)) {
struct intel_csr *csr = &dev_priv->csr;
err_printf(m, "DMC loaded: %s\n",
yesno(csr->dmc_payload != NULL));
err_printf(m, "DMC fw version: %d.%d\n",
CSR_VERSION_MAJOR(csr->version),
CSR_VERSION_MINOR(csr->version));
}
...
if (!IS_GEN9(dev_priv)) {
DRM_ERROR("No CSR support available for this platform\n");
return;
}
if (!dev_priv->csr.dmc_payload) {
DRM_ERROR("Tried to program CSR with empty payload\n");
return;
}
And according to the comments in intel_csr.c -- but not the code --
/*
* Firmware loading status will be one of the below states:
* FW_UNINITIALIZED, FW_LOADED, FW_FAILED.
*
* Once the firmware is written into the registers status will
* be moved from FW_UNINITIALIZED to FW_LOADED and for any
* erroneous condition status will be moved to FW_FAILED.
*/
So I don't think you should hold this code up as a masterpiece of
"unified" design -- which in any case you argued against last year, when
we presented a unified loader. Specifically, you said, "In my experience
trying to extract common code at all costs is harmful way too often."
Also, the approach taken in the DMC loader -- which appears to have been
copypasted from a /very early/ version of the GuC loader, before I fixed
the async-load problems -- just wouldn't work for the HuC/GuC, where the
kernel needs to know when the firmware load has been completed so that
it can start sending work to the GuC. The DMC loader only works because
it doesn't actually matter when (or if) it's loaded. It would be
*completely wrong* to load the HuC/Guc that way.
Iirc the big issue is delayed firmware loading for built-in i915 and fw
only available l
Re: [Intel-gfx] [PATCH 7/8] drm/i915: Defer enabling rc6 til after we submit the first batch/context
On Thu, Jul 14, 2016 at 04:56:50PM +0300, Mika Kuoppala wrote: > Chris Wilson writes: > > > Some hardware requires a valid render context before it can initiate > > rc6 power gating of the GPU; the default state of the GPU is not > > sufficient and may lead to undefined behaviour. The first execution of > > any batch will load the "golden render state", at which point it is safe > > to enable rc6. As we do not forcibly load the kernel context at resume, > > we have to hook into the batch submission to be sure that the render > > state is setup before enabling rc6. > > > > However, since we don't enable powersaving until that first batch, we > > queued a delayed task in order to guarantee that the batch is indeed > > submitted. > > > > v2: Rearrange intel_disable_gt_powersave() to match. > > v3: Apply user specified cur_freq (or idle_freq if not set). > > v4: Give in, and supply a delayed work to autoenable rc6 > > v5: Mika suggested a couple of better names for delayed_resume_work > > v6: Rebalance rpm_put around the autoenable task > > > > Signed-off-by: Chris Wilson > > Cc: Mika Kuoppala > > Cc: Ville Syrjälä > > Ta for splitting stuff out from this patch(set) to smaller > bits. It was only in the other because I was waiting for you to come back from holiday! (And trying to progress the other means sending all patches up to that point so that CI has a hope of sanity checking them.) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [CI resend 1/2] drm/i915: compile-time consistency check on __EXEC_OBJECT flags
Two different sets of flag bits are stored in the 'flags' member of a
'struct drm_i915_gem_exec_object2', and they're defined in two different
source files, increasing the risk of an accidental clash.
Some flags in this field are supplied by the user; these are defined in
i915_drm.h, and they start from the LSB and work up.
Other flags are defined in i915_gem_execbuffer, for internal use within
that file only; they start from the MSB and work down.
So here we add a compile-time check that the two sets of flags do not
overlap, which would cause all sorts of confusion.
Signed-off-by: Dave Gordon
Reviewed-by: Daniel Vetter
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 12
include/uapi/drm/i915_drm.h| 11 ++-
2 files changed, 14 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1978633..1bb1f25 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -34,10 +34,11 @@
#include
#include
-#define __EXEC_OBJECT_HAS_PIN (1<<31)
-#define __EXEC_OBJECT_HAS_FENCE (1<<30)
-#define __EXEC_OBJECT_NEEDS_MAP (1<<29)
-#define __EXEC_OBJECT_NEEDS_BIAS (1<<28)
+#define __EXEC_OBJECT_HAS_PIN (1<<31)
+#define __EXEC_OBJECT_HAS_FENCE (1<<30)
+#define __EXEC_OBJECT_NEEDS_MAP (1<<29)
+#define __EXEC_OBJECT_NEEDS_BIAS (1<<28)
+#define __EXEC_OBJECT_INTERNAL_FLAGS (0xf<<28) /* all of the above */
#define BATCH_OFFSET_BIAS (256*1024)
@@ -1007,6 +1008,9 @@ static bool only_mappable_for_reloc(unsigned int flags)
unsigned invalid_flags;
int i;
+ /* INTERNAL flags must not overlap with external ones */
+ BUILD_BUG_ON(__EXEC_OBJECT_INTERNAL_FLAGS &
~__EXEC_OBJECT_UNKNOWN_FLAGS);
+
invalid_flags = __EXEC_OBJECT_UNKNOWN_FLAGS;
if (USES_FULL_PPGTT(dev))
invalid_flags |= EXEC_OBJECT_NEEDS_GTT;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index d7e81a3..51b9360 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -698,12 +698,13 @@ struct drm_i915_gem_exec_object2 {
*/
__u64 offset;
-#define EXEC_OBJECT_NEEDS_FENCE (1<<0)
-#define EXEC_OBJECT_NEEDS_GTT (1<<1)
-#define EXEC_OBJECT_WRITE (1<<2)
+#define EXEC_OBJECT_NEEDS_FENCE (1<<0)
+#define EXEC_OBJECT_NEEDS_GTT (1<<1)
+#define EXEC_OBJECT_WRITE (1<<2)
#define EXEC_OBJECT_SUPPORTS_48B_ADDRESS (1<<3)
-#define EXEC_OBJECT_PINNED (1<<4)
-#define __EXEC_OBJECT_UNKNOWN_FLAGS -(EXEC_OBJECT_PINNED<<1)
+#define EXEC_OBJECT_PINNED (1<<4)
+/* All remaining bits are MBZ and RESERVED FOR FUTURE USE */
+#define __EXEC_OBJECT_UNKNOWN_FLAGS(-(EXEC_OBJECT_PINNED<<1))
__u64 flags;
__u64 rsvd1;
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: refactor eb_get_batch()
On Thu, Jul 14, 2016 at 02:12:55PM +0100, Dave Gordon wrote: > On 13/07/16 13:44, Chris Wilson wrote: > >On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote: > >>On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote: > >>>Precursor for fix to secure batch execution. We will need to be able to > >>>retrieve the batch VMA (as well as the batch itself) from the eb list, > >>>so this patch extracts that part of eb_get_batch() into a separate > >>>function, and moves both parts to a more logical place in the file, near > >>>where the eb list is created. > >>> > >>>Also, it may not be obvious, but the current execbuffer2 ioctl interface > >>>requires that the buffer object containing the batch-to-be-executed be > >>>the LAST entry in the exec2_list[] array (I expected it to be the first!). > >>> > >>>To clarify this, we can replace the rather obscure construct > >>> "list_entry(eb->vmas.prev, ...)" > >>>in the old version of eb_get_batch() with the equivalent but more explicit > >>> "list_last_entry(&eb->vmas,...)" > >>>in the new eb_get_batch_vma() and of course add an explanatory comment. > >>> > >>>Signed-off-by: Dave Gordon > >> > >>I have no context on the secure batch fix you're talking about, but this > >>here makes sense as an independent cleanup. > > > >It won't help though, so this is just churn for no purpose. > >-Chris > > At the very least, it replaces a confusing construct with > a comprehensible one annotated with an explanatory comment. No. It deepens a confusion in the code that I've been trying to get removed over the last couple of years. > Separating finding the VMA for the batch from finding the batch itself > also improves clarity and costs nothing (compiler inlines it anyway). No. That's the confusion you have here. The object is irrelevant. > Comprehensibility -- and hence maintainability -- is always > a worthwhile purpose :) s/comprehensibility/greater confusion/ > BTW, do the comments in this code from patch > > d23db88 drm/i915: Prevent negative relocation deltas from wrapping > > still apply? 'Cos I think it's pretty ugly to be setting a flag > on a VMA as a side-effect of a "lookup" type operation :( Surely > cleaner to do that sort of think at the top level i.e. inside > i915_gem_do_execbuffer() ? The comment is wrong since the practice is more widespread and it is a particular hw bug on Ivybridge. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Move hangcheck reset from out of the engines into the GPU reset
Chris Wilson writes:
> Let's not reset the hangcheck as a side-effect of initialising the
> engine, but as part of our GPU reset.
>
I think TDR will need both the init and the reset.
-Mika
> Signed-off-by: Chris Wilson
> Cc: Mika Kuoppala
> Cc: Arun Siluvery
> Cc: Tvrtko Ursulin
> ---
> drivers/gpu/drm/i915/i915_gem.c | 1 +
> drivers/gpu/drm/i915/intel_lrc.c| 2 --
> drivers/gpu/drm/i915/intel_ringbuffer.c | 2 --
> 3 files changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index b788f97cd4cf..978fa6f6f747 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3169,6 +3169,7 @@ static void i915_gem_reset_engine_cleanup(struct
> intel_engine_cs *engine)
> }
>
> intel_ring_init_seqno(engine, engine->last_submitted_seqno);
> + intel_engine_init_hangcheck(engine);
> }
>
> void i915_gem_reset(struct drm_device *dev)
> diff --git a/drivers/gpu/drm/i915/intel_lrc.c
> b/drivers/gpu/drm/i915/intel_lrc.c
> index b6af635e3a0f..377cfbfb71fd 100644
> --- a/drivers/gpu/drm/i915/intel_lrc.c
> +++ b/drivers/gpu/drm/i915/intel_lrc.c
> @@ -1529,8 +1529,6 @@ static int gen8_init_common_ring(struct intel_engine_cs
> *engine)
> engine->next_context_status_buffer = next_context_status_buffer_hw;
> DRM_DEBUG_DRIVER("Execlists enabled for %s\n", engine->name);
>
> - intel_engine_init_hangcheck(engine);
> -
> return intel_mocs_init_engine(engine);
> }
>
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
> b/drivers/gpu/drm/i915/intel_ringbuffer.c
> index 94c8ef461721..8a9e035341a9 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
> @@ -627,8 +627,6 @@ static int init_ring_common(struct intel_engine_cs
> *engine)
> ringbuf->tail = I915_READ_TAIL(engine) & TAIL_ADDR;
> intel_ring_update_space(ringbuf);
>
> - intel_engine_init_hangcheck(engine);
> -
> out:
> intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>
> --
> 2.8.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/8] drm/i915: Defer enabling rc6 til after we submit the first batch/context
Chris Wilson writes:
> Some hardware requires a valid render context before it can initiate
> rc6 power gating of the GPU; the default state of the GPU is not
> sufficient and may lead to undefined behaviour. The first execution of
> any batch will load the "golden render state", at which point it is safe
> to enable rc6. As we do not forcibly load the kernel context at resume,
> we have to hook into the batch submission to be sure that the render
> state is setup before enabling rc6.
>
> However, since we don't enable powersaving until that first batch, we
> queued a delayed task in order to guarantee that the batch is indeed
> submitted.
>
> v2: Rearrange intel_disable_gt_powersave() to match.
> v3: Apply user specified cur_freq (or idle_freq if not set).
> v4: Give in, and supply a delayed work to autoenable rc6
> v5: Mika suggested a couple of better names for delayed_resume_work
> v6: Rebalance rpm_put around the autoenable task
>
> Signed-off-by: Chris Wilson
> Cc: Mika Kuoppala
> Cc: Ville Syrjälä
Ta for splitting stuff out from this patch(set) to smaller
bits. I think it's all done now.
Reviewed-by: Mika Kuoppala ---
> drivers/gpu/drm/i915/i915_drv.c | 11 +--
> drivers/gpu/drm/i915/i915_drv.h | 2 +-
> drivers/gpu/drm/i915/i915_gem.c | 3 +
> drivers/gpu/drm/i915/intel_display.c | 1 -
> drivers/gpu/drm/i915/intel_drv.h | 3 +-
> drivers/gpu/drm/i915/intel_pm.c | 136
> +--
> drivers/gpu/drm/i915/intel_uncore.c | 2 +-
> 7 files changed, 94 insertions(+), 64 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b9a811750ca8..c6cc01faaa36 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1343,7 +1343,7 @@ void i915_driver_unload(struct drm_device *dev)
> i915_destroy_error_state(dev);
>
> /* Flush any outstanding unpin_work. */
> - flush_workqueue(dev_priv->wq);
> + drain_workqueue(dev_priv->wq);
>
> intel_guc_fini(dev);
> i915_gem_fini(dev);
> @@ -1458,8 +1458,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>
> intel_guc_suspend(dev);
>
> - intel_suspend_gt_powersave(dev_priv);
> -
> intel_display_suspend(dev);
>
> intel_dp_mst_suspend(dev);
> @@ -1652,6 +1650,7 @@ static int i915_drm_resume(struct drm_device *dev)
>
> intel_opregion_notify_adapter(dev_priv, PCI_D0);
>
> + intel_autoenable_gt_powersave(dev_priv);
> drm_kms_helper_poll_enable(dev);
>
> enable_rpm_wakeref_asserts(dev_priv);
> @@ -1778,8 +1777,6 @@ int i915_reset(struct drm_i915_private *dev_priv)
> unsigned reset_counter;
> int ret;
>
> - intel_reset_gt_powersave(dev_priv);
> -
> mutex_lock(&dev->struct_mutex);
>
> /* Clear any previous failed attempts at recovery. Time to try again. */
> @@ -1835,8 +1832,7 @@ int i915_reset(struct drm_i915_private *dev_priv)
>* previous concerns that it doesn't respond well to some forms
>* of re-init after reset.
>*/
> - if (INTEL_INFO(dev)->gen > 5)
> - intel_enable_gt_powersave(dev_priv);
> + intel_autoenable_gt_powersave(dev_priv);
>
> return 0;
>
> @@ -2459,7 +2455,6 @@ static int intel_runtime_resume(struct device *device)
>* we can do is to hope that things will still work (and disable RPM).
>*/
> i915_gem_init_swizzling(dev);
> - gen6_update_ring_freq(dev_priv);
>
> intel_runtime_pm_enable_interrupts(dev_priv);
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fe85235367be..1fdca3bb7f33 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1188,7 +1188,7 @@ struct intel_gen6_power_mgmt {
> bool client_boost;
>
> bool enabled;
> - struct delayed_work delayed_resume_work;
> + struct delayed_work autoenable_work;
> unsigned boosts;
>
> struct intel_rps_client semaphores, mmioflips;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9d8c26f42dee..67c3bffb700d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2842,6 +2842,7 @@ static void i915_gem_mark_busy(const struct
> intel_engine_cs *engine)
> intel_runtime_pm_get_noresume(dev_priv);
> dev_priv->gt.awake = true;
>
> + intel_enable_gt_powersave(dev_priv);
> i915_update_gfx_val(dev_priv);
> if (INTEL_GEN(dev_priv) >= 6)
> gen6_rps_busy(dev_priv);
> @@ -4980,6 +4981,8 @@ i915_gem_suspend(struct drm_device *dev)
> struct drm_i915_private *dev_priv = to_i915(dev);
> int ret = 0;
>
> + intel_suspend_gt_powersave(dev_priv);
> +
> mutex_lock(&dev->struct_mutex);
> ret = i915_gem_wait_for_idle(dev_priv);
> if (ret)
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i91
[Intel-gfx] [CI resend 2/2] drm/i915: refactor eb_get_batch()
Precursor for fix to secure batch execution. We will need to be able to
retrieve the batch VMA (as well as the batch itself) from the eb list,
so this patch extracts that part of eb_get_batch() into a separate
function, and moves both parts to a more logical place in the file, near
where the eb list is created.
Also, it may not be obvious, but the current execbuffer2 ioctl interface
requires that the buffer object containing the batch-to-be-executed be
the LAST entry in the exec2_list[] array (I expected it to be the
first!).
To clarify this, we can replace the rather obscure construct
"list_entry(eb->vmas.prev, ...)"
in the old version of eb_get_batch() with the equivalent but more
explicit
"list_last_entry(&eb->vmas,...)"
in the new eb_get_batch_vma() and of course add an explanatory comment.
Signed-off-by: Dave Gordon
Reviewed-by: Daniel Vetter
---
drivers/gpu/drm/i915/i915_gem_execbuffer.c | 49 ++
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 1bb1f25..f6724ae 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -186,6 +186,35 @@ struct eb_vmas {
return ret;
}
+static inline struct i915_vma *
+eb_get_batch_vma(struct eb_vmas *eb)
+{
+ /* The batch is always the LAST item in the VMA list */
+ struct i915_vma *vma = list_last_entry(&eb->vmas, typeof(*vma),
exec_list);
+
+ return vma;
+}
+
+static struct drm_i915_gem_object *
+eb_get_batch(struct eb_vmas *eb)
+{
+ struct i915_vma *vma = eb_get_batch_vma(eb);
+
+ /*
+* SNA is doing fancy tricks with compressing batch buffers, which leads
+* to negative relocation deltas. Usually that works out ok since the
+* relocate address is still positive, except when the batch is placed
+* very low in the GTT. Ensure this doesn't happen.
+*
+* Note that actual hangs have only been observed on gen7, but for
+* paranoia do it everywhere.
+*/
+ if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
+ vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
+
+ return vma->obj;
+}
+
static struct i915_vma *eb_get_vma(struct eb_vmas *eb, unsigned long handle)
{
if (eb->and < 0) {
@@ -1341,26 +1370,6 @@ static bool only_mappable_for_reloc(unsigned int flags)
return file_priv->bsd_ring;
}
-static struct drm_i915_gem_object *
-eb_get_batch(struct eb_vmas *eb)
-{
- struct i915_vma *vma = list_entry(eb->vmas.prev, typeof(*vma),
exec_list);
-
- /*
-* SNA is doing fancy tricks with compressing batch buffers, which leads
-* to negative relocation deltas. Usually that works out ok since the
-* relocate address is still positive, except when the batch is placed
-* very low in the GTT. Ensure this doesn't happen.
-*
-* Note that actual hangs have only been observed on gen7, but for
-* paranoia do it everywhere.
-*/
- if ((vma->exec_entry->flags & EXEC_OBJECT_PINNED) == 0)
- vma->exec_entry->flags |= __EXEC_OBJECT_NEEDS_BIAS;
-
- return vma->obj;
-}
-
#define I915_USER_RINGS (4)
static const enum intel_engine_id user_ring_map[I915_USER_RINGS + 1] = {
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
On Thu, Jul 14, 2016 at 02:23:04PM +0100, Chris Wilson wrote: > The biggest reason I had against going the sw_sync only route was that > vgem should provide unprivileged fences and that through the bookkeeping > in vgem we can keep them safe, ensure that we don't leak random buffers > or fences. (And I need a source of foriegn dma-buf with implicit fence > tracking with which I can try and break the driver.) And for testing passing around content + fences is more useful than passing fences alone. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ Ro.CI.BAT: warning for drm/i915/guc: Protect against HAS_GUC_* returning true values other than one (rev4)
On 13/07/16 14:38, Patchwork wrote: == Series Details == Series: drm/i915/guc: Protect against HAS_GUC_* returning true values other than one (rev4) URL : https://patchwork.freedesktop.org/series/9473/ State : warning == Summary == Series 9473v4 drm/i915/guc: Protect against HAS_GUC_* returning true values other than one http://patchwork.freedesktop.org/api/1.0/series/9473/revisions/4/mbox Test gem_exec_suspend: Subgroup basic-s3: pass -> DMESG-WARN (ro-bdw-i7-5557U) Test kms_pipe_crc_basic: Subgroup suspend-read-crc-pipe-a: dmesg-warn -> PASS (ro-skl3-i5-6260u) skip -> DMESG-WARN (ro-bdw-i7-5557U) Both of these look like https://bugs.freedesktop.org/show_bug.cgi?id=96614 [BAT BDW] *ERROR* failed to enable link training/failed to start channel equalization .Dave. fi-kbl-qkkr total:237 pass:174 dwarn:27 dfail:2 fail:7 skip:27 fi-skl-i5-6260u total:237 pass:189 dwarn:27 dfail:2 fail:7 skip:12 fi-skl-i7-6700k total:237 pass:200 dwarn:2 dfail:0 fail:9 skip:26 ro-bdw-i5-5250u total:237 pass:210 dwarn:2 dfail:0 fail:9 skip:16 ro-bdw-i7-5557U total:237 pass:210 dwarn:3 dfail:0 fail:9 skip:15 ro-bsw-n3050 total:217 pass:170 dwarn:0 dfail:0 fail:4 skip:42 ro-skl3-i5-6260u total:237 pass:213 dwarn:3 dfail:0 fail:9 skip:12 fi-snb-i7-2600 failed to connect after reboot ro-bdw-i7-5600u failed to connect after reboot ro-byt-n2820 failed to connect after reboot ro-hsw-i3-4010u failed to connect after reboot ro-hsw-i7-4770r failed to connect after reboot ro-ilk1-i5-650 failed to connect after reboot ro-ilk-i7-620lm failed to connect after reboot ro-ivb-i7-3770 failed to connect after reboot ro-snb-i7-2620M failed to connect after reboot Results at /archive/results/CI_IGT_test/RO_Patchwork_1483/ e8b1f0b drm-intel-nightly: 2016y-07m-13d-10h-57m-19s UTC integration manifest 40c374e drm/i915/guc: symbolic names for user load/submission preferences ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
On Thu, Jul 14, 2016 at 02:40:59PM +0200, Daniel Vetter wrote: > On Thu, Jul 14, 2016 at 11:11:02AM +0100, Chris Wilson wrote: > > On Thu, Jul 14, 2016 at 10:59:04AM +0100, Chris Wilson wrote: > > > On Thu, Jul 14, 2016 at 10:12:17AM +0200, Daniel Vetter wrote: > > > > On Thu, Jul 14, 2016 at 08:04:19AM +0100, Chris Wilson wrote: > > > > > vGEM buffers are useful for passing data between software clients and > > > > > hardware renders. By allowing the user to create and attach fences to > > > > > the exported vGEM buffers (on the dma-buf), the user can implement a > > > > > deferred renderer and queue hardware operations like flipping and then > > > > > signal the buffer readiness (i.e. this allows the user to schedule > > > > > operations out-of-order, but have them complete in-order). > > > > > > > > > > This also makes it much easier to write tightly controlled testcases > > > > > for > > > > > dma-buf fencing and signaling between hardware drivers. > > > > > > > > > > v2: Don't pretend the fences exist in an ordered timeline, but > > > > > allocate > > > > > a separate fence-context for each fence so that the fences are > > > > > unordered. > > > > > v3: Make the debug output more interesting, and so the signaled > > > > > status. > > > > > > > > > > Testcase: igt/vgem_basic/dmabuf-fence > > > > > Signed-off-by: Chris Wilson > > > > > Cc: Sean Paul > > > > > Cc: Zach Reizner > > > > > Cc: Gustavo Padovan > > > > > Cc: Daniel Vetter > > > > > Acked-by: Zach Reizner > > > > > > > > One thing I completely forgotten: This allows userspace to hang kernel > > > > drivers. i915 (and other gpu drivers) can recover using hangcheck, but > > > > dumber drivers (v4l, if that ever happens) probably never except such a > > > > case. We've had a similar discusion with the userspace fences exposed in > > > > sw_fence, and decided to move all those ioctl into debugfs. I think we > > > > should do the same for this vgem-based debugging of implicit sync. Sorry > > > > for realizing this this late. > > > > > > One of the very tests I make is to ensure that we recover from such a > > > hang. I don't see the difference between this any of the other ways > > > userspace can shoot itself (and others) in the foot. > > > > So one solution would be to make vgem fences automatically timeout (with > > a flag for root to override for the sake of testing hang detection). > > The problem is other drivers. E.g. right now atomic helpers assume that > fences will signal, and can't recover if they don't. This is why drivers > where things might fail must have some recovery (hangcheck, timeout) to > make sure dma_fences always signal. Urm, all the atomic helpers should work with fails. The waits on dma-buf should be before any hardware is modified and so cancellation is trivial. Anyone using a foriegn fence (or even native) must cope that it may not meet some deadline. They have to. Anyone sharing a i915 dma-buf is susceptible to all kinds of (unprivileged) fun. > Imo not even root should be allowed to break this, since it could put > drivers into a non-recoverable state. I think this must be restricted to > something known-unsafe-don't-enable-on-production like debugfs. Providing fences is extremely useful, even for software buffers. (For the sake of argument, just imagine an asynchronous multithreaded llvmpipe wanting to support client fences for deferred rendering.) The only question in my mind is how much cotton wool to use. > Other solutions which I don't like: > - Everyone needs to be able to recover. Given how much effort it is to > just keep i915 hangcheck in working order I think that's totally > illusionary to assume. At least once world+dog (atomic, v4l, ...) all > consume/produce fences, subsystems where the usual assumption holds that > async ops complete. > > - Really long timeouts are allowed for root in vgem. Could lead to even > more fun in testing i915 hangchecks I think, so don't like that much > either. The whole point is in testing our handling before we become suspectible to real world fail - because as you point out, not everyone guarantees that a fence will be signaled. I can't simply pass around i915 dma-buf simply because we may unwind them and in the process completely curtail being able to test a foriegn fence that hangs. > I think the best option is to just do the same as we've done for sw_fence, > and move it to debugfs. We could reuse the debugfs sw_fence interface to > create them (gives us more control as a bonus), and just have an ioctl to > attach fences to vgem (which could be unpriviledged). The biggest reason I had against going the sw_sync only route was that vgem should provide unprivileged fences and that through the bookkeeping in vgem we can keep them safe, ensure that we don't leak random buffers or fences. (And I need a source of foriegn dma-buf with implicit fence tracking with which I can try and break the driver.) -Chris -- Chris Wilson, Intel Open Sou
Re: [Intel-gfx] [PATCH] Revert "drm: Resurrect atomic rmfb code"
On Thu, Jul 14, 2016 at 03:16:34PM +0200, Daniel Vetter wrote:
> This reverts commit 11c21e73f848844d439cbccb42a1018b8c560e5c.
>
> For reasons totally unclear this manages to wreak havoc with the audio
> rpm refcount:
>
> [ cut here ]
> WARNING: CPU: 0 PID: 215 at drivers/gpu/drm/i915/intel_runtime_pm.c:1729
> intel_display_power_put+0xe8/0x100 [i915]
> Use count on domain AUDIO is already zero
> Modules linked in: i915 ax88179_178a usbnet mii snd_hda_codec_hdmi
> snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec
> x86_pkg_temp_thermal snd_hwdep intel_powerclamp snd_hda_core co
> f_pclmul crc32_pclmul snd_pcm ghash_clmulni_intel mei_me mei e1000e ptp
> pps_core i2c_hid [last unloaded: i915]
> CPU: 0 PID: 215 Comm: kworker/0:2 Not tainted 4.7.0-rc6+ #44
> Hardware name: Intel Corporation Skylake Client platform/Skylake Halo DDR4
> RVP11, BIOS SKLSE2R1.R00.X106.B00.1601180206 01/18/2016
> Workqueue: events output_poll_execute
> 88045573fa38 813a2d6b 88045573fa88
> 88045573fa78 81075db6 06c15a59
> 88045a59a238 88045a590054 88045a59 88045a59
> Call Trace:
> [] dump_stack+0x4d/0x72
> [] __warn+0xc6/0xe0
> [] warn_slowpath_fmt+0x4a/0x50
> [] ? hsw_audio_codec_disable+0xdd/0x110 [i915]
> [] intel_display_power_put+0xe8/0x100 [i915]
> [] intel_disable_ddi+0x46/0x80 [i915]
> [] haswell_crtc_disable+0x16f/0x290 [i915]
> [] intel_atomic_commit_tail+0x153/0x10e0 [i915]
> [] ? drm_atomic_helper_swap_state+0x140/0x2d0
> [] intel_atomic_commit+0x3fd/0x520 [i915]
> [] ? drm_atomic_add_affected_connectors+0x22/0xf0
> [] drm_atomic_commit+0x32/0x50
> [] restore_fbdev_mode+0x147/0x260
> [] drm_fb_helper_restore_fbdev_mode_unlocked+0x2e/0x70
> [] drm_fb_helper_set_par+0x28/0x50
> [] drm_fb_helper_hotplug_event+0x143/0x180
> [] intel_fbdev_output_poll_changed+0x15/0x20 [i915]
> [] drm_kms_helper_hotplug_event+0x22/0x30
> [] output_poll_execute+0x192/0x1e0
> [] process_one_work+0x14c/0x480
> [] worker_thread+0x24a/0x4e0
> [] ? process_one_work+0x480/0x480
> [] ? process_one_work+0x480/0x480
> [] kthread+0xc4/0xe0
> [] ret_from_fork+0x1f/0x40
> [] ? kthread_worker_fn+0x180/0x180
> ---[ end trace 2d440da5f0c053e4 ]---
>
> Instead of scratching heads too much while CI is down, let's revert
> before more trouble is caused.
>
> Cc: Maarten Lankhorst
> Cc: Mika Kuoppala
> Cc: Ville Syrjälä
> Reported-by: Mika Kuoppala
> Reported-by: Ville Syrjälä
> Acked-by: Mika Kuoppala
> Acked-by: Ville Syrjälä
> Signed-off-by: Daniel Vetter
... and applied.
-Daniel
> ---
> drivers/gpu/drm/drm_atomic.c| 66
> -
> drivers/gpu/drm/drm_crtc.c | 6
> drivers/gpu/drm/drm_crtc_internal.h | 1 -
> 3 files changed, 73 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 34cdbe154540..6712278c73f0 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -1590,72 +1590,6 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
> }
> EXPORT_SYMBOL(drm_atomic_clean_old_fb);
>
> -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;
> - plane->fb = NULL;
> - }
> -
> - 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;
> - }
> -
> - if (ret || !plane_mask)
> - drm_atomic_state_free(state);
> -
> - drm_modeset_drop_locks(&ctx);
> - drm_modeset_acquire_fini(&ctx);
>
[Intel-gfx] [PATCH] Revert "drm: Resurrect atomic rmfb code"
This reverts commit 11c21e73f848844d439cbccb42a1018b8c560e5c.
For reasons totally unclear this manages to wreak havoc with the audio
rpm refcount:
[ cut here ]
WARNING: CPU: 0 PID: 215 at drivers/gpu/drm/i915/intel_runtime_pm.c:1729
intel_display_power_put+0xe8/0x100 [i915]
Use count on domain AUDIO is already zero
Modules linked in: i915 ax88179_178a usbnet mii snd_hda_codec_hdmi
snd_hda_codec_realtek snd_hda_codec_generic snd_hda_codec x86_pkg_temp_thermal
snd_hwdep intel_powerclamp snd_hda_core co
f_pclmul crc32_pclmul snd_pcm ghash_clmulni_intel mei_me mei e1000e ptp
pps_core i2c_hid [last unloaded: i915]
CPU: 0 PID: 215 Comm: kworker/0:2 Not tainted 4.7.0-rc6+ #44
Hardware name: Intel Corporation Skylake Client platform/Skylake Halo DDR4
RVP11, BIOS SKLSE2R1.R00.X106.B00.1601180206 01/18/2016
Workqueue: events output_poll_execute
88045573fa38 813a2d6b 88045573fa88
88045573fa78 81075db6 06c15a59
88045a59a238 88045a590054 88045a59 88045a59
Call Trace:
[] dump_stack+0x4d/0x72
[] __warn+0xc6/0xe0
[] warn_slowpath_fmt+0x4a/0x50
[] ? hsw_audio_codec_disable+0xdd/0x110 [i915]
[] intel_display_power_put+0xe8/0x100 [i915]
[] intel_disable_ddi+0x46/0x80 [i915]
[] haswell_crtc_disable+0x16f/0x290 [i915]
[] intel_atomic_commit_tail+0x153/0x10e0 [i915]
[] ? drm_atomic_helper_swap_state+0x140/0x2d0
[] intel_atomic_commit+0x3fd/0x520 [i915]
[] ? drm_atomic_add_affected_connectors+0x22/0xf0
[] drm_atomic_commit+0x32/0x50
[] restore_fbdev_mode+0x147/0x260
[] drm_fb_helper_restore_fbdev_mode_unlocked+0x2e/0x70
[] drm_fb_helper_set_par+0x28/0x50
[] drm_fb_helper_hotplug_event+0x143/0x180
[] intel_fbdev_output_poll_changed+0x15/0x20 [i915]
[] drm_kms_helper_hotplug_event+0x22/0x30
[] output_poll_execute+0x192/0x1e0
[] process_one_work+0x14c/0x480
[] worker_thread+0x24a/0x4e0
[] ? process_one_work+0x480/0x480
[] ? process_one_work+0x480/0x480
[] kthread+0xc4/0xe0
[] ret_from_fork+0x1f/0x40
[] ? kthread_worker_fn+0x180/0x180
---[ end trace 2d440da5f0c053e4 ]---
Instead of scratching heads too much while CI is down, let's revert
before more trouble is caused.
Cc: Maarten Lankhorst
Cc: Mika Kuoppala
Cc: Ville Syrjälä
Reported-by: Mika Kuoppala
Reported-by: Ville Syrjälä
Acked-by: Mika Kuoppala
Acked-by: Ville Syrjälä
Signed-off-by: Daniel Vetter
---
drivers/gpu/drm/drm_atomic.c| 66 -
drivers/gpu/drm/drm_crtc.c | 6
drivers/gpu/drm/drm_crtc_internal.h | 1 -
3 files changed, 73 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 34cdbe154540..6712278c73f0 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -1590,72 +1590,6 @@ void drm_atomic_clean_old_fb(struct drm_device *dev,
}
EXPORT_SYMBOL(drm_atomic_clean_old_fb);
-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;
- plane->fb = NULL;
- }
-
- 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;
- }
-
- if (ret || !plane_mask)
- drm_atomic_state_free(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.c b/drivers/gpu/drm/drm_crtc.c
index 1b7d31186f25..0098d5e6921a 100644
--- a/drivers/gpu/drm/drm
Re: [Intel-gfx] ✗ Ro.CI.BAT: failure for drm/i915: remove superfluous i915_gem_object_free_mmap_offset call
Merged, thanks for the patch and review. Regards, Joonas On to, 2016-07-14 at 13:14 +0100, Matthew Auld wrote: > On 5 July 2016 at 14:21, Patchwork wrote: > > > > == Series Details == > > > > Series: drm/i915: remove superfluous i915_gem_object_free_mmap_offset call > > URL : https://patchwork.freedesktop.org/series/9516/ > > State : failure > > > > == Summary == > > > > Series 9516v1 drm/i915: remove superfluous i915_gem_object_free_mmap_offset > > call > > http://patchwork.freedesktop.org/api/1.0/series/9516/revisions/1/mbox > > > > Test gem_exec_flush: > > Subgroup basic-batch-kernel-default-cmd: > > pass -> FAIL (ro-byt-n2820) > https://bugs.freedesktop.org/show_bug.cgi?id=95372 > > > > > Subgroup basic-batch-kernel-default-wb: > > pass -> DMESG-FAIL (ro-bdw-i7-5557U) > Created bug report https://bugs.freedesktop.org/show_bug.cgi?id=96927 > > > > > Test kms_pipe_crc_basic: > > Subgroup suspend-read-crc-pipe-a: > > skip -> DMESG-WARN (ro-bdw-i7-5557U) > Created bug report https://bugs.freedesktop.org/show_bug.cgi?id=96913 -- Joonas Lahtinen Open Source Technology Center Intel Corporation ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: refactor eb_get_batch()
On 13/07/16 13:44, Chris Wilson wrote: On Wed, Jul 13, 2016 at 02:38:16PM +0200, Daniel Vetter wrote: On Thu, Jun 30, 2016 at 04:12:49PM +0100, Dave Gordon wrote: Precursor for fix to secure batch execution. We will need to be able to retrieve the batch VMA (as well as the batch itself) from the eb list, so this patch extracts that part of eb_get_batch() into a separate function, and moves both parts to a more logical place in the file, near where the eb list is created. Also, it may not be obvious, but the current execbuffer2 ioctl interface requires that the buffer object containing the batch-to-be-executed be the LAST entry in the exec2_list[] array (I expected it to be the first!). To clarify this, we can replace the rather obscure construct "list_entry(eb->vmas.prev, ...)" in the old version of eb_get_batch() with the equivalent but more explicit "list_last_entry(&eb->vmas,...)" in the new eb_get_batch_vma() and of course add an explanatory comment. Signed-off-by: Dave Gordon I have no context on the secure batch fix you're talking about, but this here makes sense as an independent cleanup. It won't help though, so this is just churn for no purpose. -Chris At the very least, it replaces a confusing construct with a comprehensible one annotated with an explanatory comment. Separating finding the VMA for the batch from finding the batch itself also improves clarity and costs nothing (compiler inlines it anyway). Comprehensibility -- and hence maintainability -- is always a worthwhile purpose :) BTW, do the comments in this code from patch d23db88 drm/i915: Prevent negative relocation deltas from wrapping still apply? 'Cos I think it's pretty ugly to be setting a flag on a VMA as a side-effect of a "lookup" type operation :( Surely cleaner to do that sort of think at the top level i.e. inside i915_gem_do_execbuffer() ? .Dave. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Ignore panel type from OpRegion on SKL
On Tue, Jul 12, 2016 at 03:00:37PM +0300, ville.syrj...@linux.intel.com wrote:
> From: Ville Syrjälä
>
> Dell XPS 13 9350 apparently doesn't like it when we use the panel type
> from OpRegion. The OpRegion panel type (0) tells us to use use low
> vswing for eDP, whereas the VBT panel type (2) tells us to use normal
> vswing. The problem is that low vswing results in some display flickers.
> Since no one seems to know how this stuff is supposed to be handled,
> let's just ignore the OpRegion panel type on SKL for now.
>
> Reported-by: James Bottomley
> Cc: James Bottomley
> Cc: drm-intel-fi...@lists.freedesktop.org
> References:
> https://lists.freedesktop.org/archives/intel-gfx/2016-June/098826.html
> Fixes: a05628195a0d ("drm/i915: Get panel_type from OpRegion panel details")
> Signed-off-by: Ville Syrjälä
> ---
> drivers/gpu/drm/i915/intel_opregion.c | 11 +++
> 1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c
> b/drivers/gpu/drm/i915/intel_opregion.c
> index c27d5eb063d0..259760f12d16 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -1072,5 +1072,16 @@ intel_opregion_get_panel_type(struct drm_i915_private
> *dev_priv)
> return -ENODEV;
> }
>
> + /*
> + * FIXME On Dell XPS 13 9350 the OpRegion panel type (0) gives us
> + * low vswing for eDP, whereas the VBT panel type (2) gives us normal
> + * vswing instead. Low vswing results in some display flickers, so
> + * let's simply ignore the OpRegion panel type on SKL for now.
> + */
> + if (IS_SKYLAKE(dev_priv)) {
> + DRM_DEBUG_KMS("Ignoring OpRegion panel type (%d)\n", ret);
I did s/ret/ret - 1/ here to make the debug output correct, and pushed
the patch to dinq. Thanks for the review.
> + return -ENODEV;
> + }
> +
> return ret - 1;
> }
> --
> 2.7.4
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ Ro.CI.BAT: warning for drm/i915: Ignore panel type from OpRegion on SKL
On Tue, Jul 12, 2016 at 01:12:20PM -, Patchwork wrote: > == Series Details == > > Series: drm/i915: Ignore panel type from OpRegion on SKL > URL : https://patchwork.freedesktop.org/series/9752/ > State : warning > > == Summary == > > Series 9752v1 drm/i915: Ignore panel type from OpRegion on SKL > http://patchwork.freedesktop.org/api/1.0/series/9752/revisions/1/mbox > > Test kms_flip: > Subgroup basic-flip-vs-wf_vblank: > fail -> PASS (ro-byt-n2820) > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-a: > skip -> DMESG-WARN (ro-bdw-i7-5557U) This test regularly ping-pongs between warn and skip on this machine. [ 429.961534] [drm:intel_dp_link_training_clock_recovery [i915]] *ERROR* failed to enable link training [ 430.045093] [drm:intel_dp_start_link_train [i915]] *ERROR* failed to start channel equalization > > fi-kbl-qkkr total:237 pass:174 dwarn:28 dfail:1 fail:7 skip:27 > fi-skl-i5-6260u total:237 pass:218 dwarn:0 dfail:0 fail:7 skip:12 > fi-skl-i7-6700k total:237 pass:204 dwarn:0 dfail:0 fail:7 skip:26 > fi-snb-i7-2600 total:237 pass:190 dwarn:0 dfail:0 fail:7 skip:40 > ro-bdw-i5-5250u total:237 pass:213 dwarn:1 dfail:0 fail:7 skip:16 > ro-bdw-i7-5557U total:237 pass:213 dwarn:2 dfail:0 fail:7 skip:15 > ro-bdw-i7-5600u total:237 pass:199 dwarn:0 dfail:0 fail:7 skip:31 > ro-bsw-n3050 total:218 pass:170 dwarn:0 dfail:0 fail:5 skip:42 > ro-byt-n2820 total:237 pass:189 dwarn:0 dfail:0 fail:10 skip:38 > ro-hsw-i3-4010u total:237 pass:206 dwarn:0 dfail:0 fail:7 skip:24 > ro-hsw-i7-4770r total:237 pass:206 dwarn:0 dfail:0 fail:7 skip:24 > ro-ilk-i7-620lm total:237 pass:166 dwarn:0 dfail:0 fail:8 skip:63 > ro-ilk1-i5-650 total:232 pass:166 dwarn:0 dfail:0 fail:8 skip:58 > ro-ivb-i7-3770 total:237 pass:197 dwarn:0 dfail:0 fail:7 skip:33 > ro-skl3-i5-6260u total:237 pass:217 dwarn:1 dfail:0 fail:7 skip:12 > ro-snb-i7-2620M total:237 pass:188 dwarn:0 dfail:0 fail:8 skip:41 > > Results at /archive/results/CI_IGT_test/RO_Patchwork_1473/ > > 7d4e6e0 drm-intel-nightly: 2016y-07m-12d-11h-23m-08s UTC integration manifest > 5bd5ff5 drm/i915: Ignore panel type from OpRegion on SKL -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
On Thu, Jul 14, 2016 at 11:11:02AM +0100, Chris Wilson wrote: > On Thu, Jul 14, 2016 at 10:59:04AM +0100, Chris Wilson wrote: > > On Thu, Jul 14, 2016 at 10:12:17AM +0200, Daniel Vetter wrote: > > > On Thu, Jul 14, 2016 at 08:04:19AM +0100, Chris Wilson wrote: > > > > vGEM buffers are useful for passing data between software clients and > > > > hardware renders. By allowing the user to create and attach fences to > > > > the exported vGEM buffers (on the dma-buf), the user can implement a > > > > deferred renderer and queue hardware operations like flipping and then > > > > signal the buffer readiness (i.e. this allows the user to schedule > > > > operations out-of-order, but have them complete in-order). > > > > > > > > This also makes it much easier to write tightly controlled testcases for > > > > dma-buf fencing and signaling between hardware drivers. > > > > > > > > v2: Don't pretend the fences exist in an ordered timeline, but allocate > > > > a separate fence-context for each fence so that the fences are > > > > unordered. > > > > v3: Make the debug output more interesting, and so the signaled status. > > > > > > > > Testcase: igt/vgem_basic/dmabuf-fence > > > > Signed-off-by: Chris Wilson > > > > Cc: Sean Paul > > > > Cc: Zach Reizner > > > > Cc: Gustavo Padovan > > > > Cc: Daniel Vetter > > > > Acked-by: Zach Reizner > > > > > > One thing I completely forgotten: This allows userspace to hang kernel > > > drivers. i915 (and other gpu drivers) can recover using hangcheck, but > > > dumber drivers (v4l, if that ever happens) probably never except such a > > > case. We've had a similar discusion with the userspace fences exposed in > > > sw_fence, and decided to move all those ioctl into debugfs. I think we > > > should do the same for this vgem-based debugging of implicit sync. Sorry > > > for realizing this this late. > > > > One of the very tests I make is to ensure that we recover from such a > > hang. I don't see the difference between this any of the other ways > > userspace can shoot itself (and others) in the foot. > > So one solution would be to make vgem fences automatically timeout (with > a flag for root to override for the sake of testing hang detection). The problem is other drivers. E.g. right now atomic helpers assume that fences will signal, and can't recover if they don't. This is why drivers where things might fail must have some recovery (hangcheck, timeout) to make sure dma_fences always signal. Imo not even root should be allowed to break this, since it could put drivers into a non-recoverable state. I think this must be restricted to something known-unsafe-don't-enable-on-production like debugfs. Other solutions which I don't like: - Everyone needs to be able to recover. Given how much effort it is to just keep i915 hangcheck in working order I think that's totally illusionary to assume. At least once world+dog (atomic, v4l, ...) all consume/produce fences, subsystems where the usual assumption holds that async ops complete. - Really long timeouts are allowed for root in vgem. Could lead to even more fun in testing i915 hangchecks I think, so don't like that much either. I think the best option is to just do the same as we've done for sw_fence, and move it to debugfs. We could reuse the debugfs sw_fence interface to create them (gives us more control as a bonus), and just have an ioctl to attach fences to vgem (which could be unpriviledged). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Ro.CI.BAT: failure for drm/i915: Move hangcheck reset from out of the engines into the GPU reset
== Series Details == Series: drm/i915: Move hangcheck reset from out of the engines into the GPU reset URL : https://patchwork.freedesktop.org/series/9871/ State : failure == Summary == Series 9871v1 drm/i915: Move hangcheck reset from out of the engines into the GPU reset http://patchwork.freedesktop.org/api/1.0/series/9871/revisions/1/mbox Test drv_getparams_basic: Subgroup basic-eu-total: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-subslice-total: pass -> SKIP (fi-skl-i5-6260u) Test drv_hangman: Subgroup error-state-basic: pass -> SKIP (fi-skl-i5-6260u) Test drv_module_reload_basic: dmesg-warn -> TIMEOUT(fi-skl-i5-6260u) Test gem_basic: Subgroup bad-close: pass -> SKIP (fi-skl-i5-6260u) Subgroup create-close: pass -> SKIP (fi-skl-i5-6260u) Subgroup create-fd-close: pass -> SKIP (fi-skl-i5-6260u) Test gem_busy: Subgroup basic-blt: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-bsd: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-bsd1: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-bsd2: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-parallel-blt: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-parallel-bsd: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-parallel-bsd1: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-parallel-bsd2: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-parallel-render: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-parallel-vebox: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-render: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-vebox: pass -> SKIP (fi-skl-i5-6260u) Test gem_close_race: Subgroup basic-process: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-threads: pass -> SKIP (fi-skl-i5-6260u) Test gem_cs_tlb: Subgroup basic-default: pass -> SKIP (fi-skl-i5-6260u) Test gem_ctx_exec: Subgroup basic: pass -> SKIP (fi-skl-i5-6260u) Test gem_ctx_param: Subgroup basic: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-default: pass -> SKIP (fi-skl-i5-6260u) Test gem_exec_basic: Subgroup basic-blt: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-bsd: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-bsd1: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-bsd2: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-default: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-render: pass -> SKIP (fi-skl-i5-6260u) Subgroup basic-vebox: pass -> SKIP (fi-skl-i5-6260u) Subgroup gtt-blt: pass -> SKIP (fi-skl-i5-6260u) Subgroup gtt-bsd: pass -> SKIP (fi-skl-i5-6260u) Subgroup gtt-bsd1: pass -> SKIP (fi-skl-i5-6260u) Subgroup gtt-bsd2: pass -> SKIP (fi-skl-i5-6260u) Subgroup gtt-default: pass -> SKIP (fi-skl-i5-6260u) Subgroup gtt-render: pass -> SKIP (fi-skl-i5-6260u) Subgroup gtt-vebox: pass -> SKIP (fi-skl-i5-6260u) Subgroup readonly-blt: pass -> SKIP (fi-skl-i5-6260u) Subgroup readonly-bsd: pass -> SKIP (fi-skl-i5-6260u) Subgroup readonly-bsd1: pass -> SKIP (fi-skl-i5-6260u) Subgroup readonly-bsd2: pass -> SKIP (fi-skl-i5-6260u) Subgroup readonly-default: pass -> SKIP (fi-skl-i5-6260u) WARNING: Long output truncated Results at /archive/results/CI_IGT_test/RO_Patchwork_1491/ c504630 drm-intel-nightly: 2016y-07m-14d-10h-18m-17s UTC integration manifest 9f5f1b2 drm/i915: Move hangcheck reset from out of the engines into the GPU reset ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] drm-intel.git Committers: Reminder about tagging bugfixes
Hi all, Apparently this wasn't known to everyone, hence this small reminder about correctly tagging bugfixes when pushing them: - add a Bugzilla: or References: link for any bug reported anywhere (bugzilla or mailing list). Also add Reported-by:/Tested-by: for credits. - add Fixes: if it's a regression fix. Run "dim fixes $sha1" for convenience, to generate this correctly - add correct Cc: tags, Cc: stable for backporting to all kernels (including -rc), Cc: drm-intel-fixes if it's only needed in current -rc series. Special note about pre-production hw: We only backport fixes when the prelim_hw_support tag is lifted, before that point we don't bother. But after that _all_ bugfixes need one of the above tags, and right now both kbl and apl/bxt aren't preliminary any more! For full details see the docs: https://01.org/linuxgraphics/gfx-docs/maintainer-tools/drm-intel.html If you haven't read them in a while would be good to check them out again, quite a few improvements and clarifications over the past few months. Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
On Thu, Jul 14, 2016 at 11:11:02AM +0100, Chris Wilson wrote:
> So one solution would be to make vgem fences automatically timeout (with
> a flag for root to override for the sake of testing hang detection).
diff --git a/drivers/gpu/drm/vgem/vgem_fence.c
b/drivers/gpu/drm/vgem/vgem_fence.c
index b7da11419ad6..17c63c9a8ea0 100644
--- a/drivers/gpu/drm/vgem/vgem_fence.c
+++ b/drivers/gpu/drm/vgem/vgem_fence.c
@@ -28,6 +28,7 @@
struct vgem_fence {
struct fence base;
struct spinlock lock;
+ struct timer_list timer;
};
static const char *vgem_fence_get_driver_name(struct fence *fence)
@@ -50,6 +51,14 @@ static bool vgem_fence_enable_signaling(struct fence *fence)
return true;
}
+static void vgem_fence_release(struct fence *base)
+{
+ struct vgem_fence *fence = container_of(base, typeof(*fence), base);
+
+ del_timer_sync(&fence->timer);
+ fence_free(&fence->base);
+}
+
static void vgem_fence_value_str(struct fence *fence, char *str, int size)
{
snprintf(str, size, "%u", fence->seqno);
@@ -67,11 +76,21 @@ const struct fence_ops vgem_fence_ops = {
.enable_signaling = vgem_fence_enable_signaling,
.signaled = vgem_fence_signaled,
.wait = fence_default_wait,
+ .release = vgem_fence_release,
+
.fence_value_str = vgem_fence_value_str,
.timeline_value_str = vgem_fence_timeline_value_str,
};
-static struct fence *vgem_fence_create(struct vgem_file *vfile)
+static void vgem_fence_timeout(unsigned long data)
+{
+ struct vgem_fence *fence = (struct vgem_fence *)data;
+
+ fence_signal(&fence->base);
+}
+
+static struct fence *vgem_fence_create(struct vgem_file *vfile,
+ unsigned int flags)
{
struct vgem_fence *fence;
@@ -83,6 +102,12 @@ static struct fence *vgem_fence_create(struct vgem_file
*vfile)
fence_init(&fence->base, &vgem_fence_ops, &fence->lock,
fence_context_alloc(1), 1);
+ setup_timer(&fence->timer, vgem_fence_timeout, (unsigned long)fence);
+
+ /* We force the fence to expire within 10s to prevent driver hangs */
+ if (!(flags & VGEM_FENCE_NOTIMEOUT))
+ mod_timer(&fence->timer, 10*HZ);
+
return &fence->base;
}
@@ -114,9 +139,12 @@ int vgem_fence_attach_ioctl(struct drm_device *dev,
struct fence *fence;
int ret;
- if (arg->flags & ~VGEM_FENCE_WRITE)
+ if (arg->flags & ~(VGEM_FENCE_WRITE | VGEM_FENCE_NOTIMEOUT))
return -EINVAL;
+ if (arg->flags & VGEM_FENCE_NOTIMEOUT && !capable(CAP_SYS_ADMIN))
+ return -EPERM;
+
if (arg->pad)
return -EINVAL;
@@ -128,7 +156,7 @@ int vgem_fence_attach_ioctl(struct drm_device *dev,
if (ret)
goto out;
- fence = vgem_fence_create(vfile);
+ fence = vgem_fence_create(vfile, arg->flags);
if (!fence) {
ret = -ENOMEM;
goto out;
diff --git a/include/uapi/drm/vgem_drm.h b/include/uapi/drm/vgem_drm.h
index 352d2fae8de9..55fd08750773 100644
--- a/include/uapi/drm/vgem_drm.h
+++ b/include/uapi/drm/vgem_drm.h
@@ -45,7 +45,8 @@ extern "C" {
struct drm_vgem_fence_attach {
__u32 handle;
__u32 flags;
-#define VGEM_FENCE_WRITE 0x1
+#define VGEM_FENCE_WRITE 0x1
+#define VGEM_FENCE_NOTIMEOUT 0x2
__u32 out_fence;
__u32 pad;
};
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ Ro.CI.BAT: failure for drm/i915: remove superfluous i915_gem_object_free_mmap_offset call
On 5 July 2016 at 14:21, Patchwork wrote: > == Series Details == > > Series: drm/i915: remove superfluous i915_gem_object_free_mmap_offset call > URL : https://patchwork.freedesktop.org/series/9516/ > State : failure > > == Summary == > > Series 9516v1 drm/i915: remove superfluous i915_gem_object_free_mmap_offset > call > http://patchwork.freedesktop.org/api/1.0/series/9516/revisions/1/mbox > > Test gem_exec_flush: > Subgroup basic-batch-kernel-default-cmd: > pass -> FAIL (ro-byt-n2820) https://bugs.freedesktop.org/show_bug.cgi?id=95372 > Subgroup basic-batch-kernel-default-wb: > pass -> DMESG-FAIL (ro-bdw-i7-5557U) Created bug report https://bugs.freedesktop.org/show_bug.cgi?id=96927 > Test kms_pipe_crc_basic: > Subgroup suspend-read-crc-pipe-a: > skip -> DMESG-WARN (ro-bdw-i7-5557U) Created bug report https://bugs.freedesktop.org/show_bug.cgi?id=96913 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Move hangcheck reset from out of the engines into the GPU reset
Let's not reset the hangcheck as a side-effect of initialising the
engine, but as part of our GPU reset.
Signed-off-by: Chris Wilson
Cc: Mika Kuoppala
Cc: Arun Siluvery
Cc: Tvrtko Ursulin
---
drivers/gpu/drm/i915/i915_gem.c | 1 +
drivers/gpu/drm/i915/intel_lrc.c| 2 --
drivers/gpu/drm/i915/intel_ringbuffer.c | 2 --
3 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index b788f97cd4cf..978fa6f6f747 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3169,6 +3169,7 @@ static void i915_gem_reset_engine_cleanup(struct
intel_engine_cs *engine)
}
intel_ring_init_seqno(engine, engine->last_submitted_seqno);
+ intel_engine_init_hangcheck(engine);
}
void i915_gem_reset(struct drm_device *dev)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index b6af635e3a0f..377cfbfb71fd 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1529,8 +1529,6 @@ static int gen8_init_common_ring(struct intel_engine_cs
*engine)
engine->next_context_status_buffer = next_context_status_buffer_hw;
DRM_DEBUG_DRIVER("Execlists enabled for %s\n", engine->name);
- intel_engine_init_hangcheck(engine);
-
return intel_mocs_init_engine(engine);
}
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c
b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 94c8ef461721..8a9e035341a9 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -627,8 +627,6 @@ static int init_ring_common(struct intel_engine_cs *engine)
ringbuf->tail = I915_READ_TAIL(engine) & TAIL_ADDR;
intel_ring_update_space(ringbuf);
- intel_engine_init_hangcheck(engine);
-
out:
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
--
2.8.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/5] drm/i915: unify first-stage engine struct setup
On 13/07/16 14:16, Tvrtko Ursulin wrote:
On 13/07/16 13:23, Daniel Vetter wrote:
On Fri, Jul 01, 2016 at 05:47:11PM +0100, Tvrtko Ursulin wrote:
From: Dave Gordon
[snip]
{
-const struct logical_ring_info *info = &logical_rings[id];
+const struct engine_info *info = &intel_engines[id];
struct intel_engine_cs *engine = &dev_priv->engine[id];
-enum forcewake_domains fw_domains;
engine->id = id;
+engine->i915 = dev_priv;
engine->name = info->name;
engine->exec_id = info->exec_id;
-engine->guc_id = info->guc_id;
+engine->hw_id = engine->guc_id = info->guc_id;
Optional bikeshed: s/info->guc_id/info->hw_id/ makes sense imo in the new
context. Or nuking engine->guc_id.
Someone said we cannot be sure they will be the same in the future. So
maybe just rename to hw_id for now.
Regards,
Tvrtko
The GuC firmware *could* use a completely different enumeration of the
engines, but why would it? Since it's closely tied to the hardware, it
*ought* to use the same naming scheme as the h/w. So I have no objection
to getting rid of guc_id entirely, and changing existing uses to use
hw_id instead.
OTOH it seems little benefit and would certainly involve more work to
reverse. The firmware team might, after all, decide that they too want
to decouple the logical-engine-numbers used for the KMD interface from
whatever the hardware team decide is the best way to number engines --
which might after all change between generations or even different SKUs
of the same device!
SO, I think the "best" version of that line is probably:
+ engine->hw_id = info->hw_id;
+
+ /* Current GuC f/w uses hw_id not driver id */
+ engine->guc_id = info->hw_id;
and we'll add "info->guc_id" back again if it ever becomes necessary.
.Dave.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PACTH i-g-t] lib/igt_kms: Fix different order of properties and their name strings
Applied. Thanks!
On Mon, Jul 11, 2016 at 03:54:12PM -0400, Robert Foss wrote:
> This is a reminder of this series.
>
> On 2016-06-29 07:22 AM, robert.f...@collabora.com wrote:
> >From: Robert Foss
> >
> >igt_crtc_prop_names and igt_atomic_crtc_properties have different orders of
> >properties, which is fixed in this patch.
> >
> >Signed-off-by: Robert Foss
> >---
> > lib/igt_kms.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> >diff --git a/lib/igt_kms.c b/lib/igt_kms.c
> >index af72b90..dd4eb84 100644
> >--- a/lib/igt_kms.c
> >+++ b/lib/igt_kms.c
> >@@ -164,8 +164,8 @@ static const char
> >*igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = {
> >
> > static const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = {
> > "background_color",
> >-"DEGAMMA_LUT",
> > "CTM",
> >+"DEGAMMA_LUT",
> > "GAMMA_LUT",
> > };
> >
> >
signature.asc
Description: Digital signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH igt 01/17] lib: Start weaning off defunct intel_chipset.h
On Thu, Jul 14, 2016 at 11:31:08AM +0100, Emil Velikov wrote: > On 13 July 2016 at 14:34, Chris Wilson wrote: > > On Wed, Jul 13, 2016 at 02:40:49PM +0200, Daniel Vetter wrote: > >> On Wed, Jun 29, 2016 at 12:38:51PM +0100, Chris Wilson wrote: > >> > Several years ago we made the plan of only having one canonical source > >> > for i915_pciids.h, the kernel and everyone importing their definitions > >> > from that. For consistency, we style the intel_device_info after the > >> > kernel, most notably using a generation mask and a per-codename bitfield. > >> > > >> > This first step converts looking up the generation for a devid tree from > >> > a massive if(devid)-chain to a (cached) table lookup. > >> > > >> > Signed-off-by: Chris Wilson > >> > >> Should we extract the kernels list of chipset into intel_chipset.c too, to > >> make them 1:1 copies like with i915_pciids.h? With that this (entire > >> series) here has my ack. Without I'm not sold that much on the churn > >> really. > > > > Hmm, next up would be libdrm which is almost a duplication of igt (and > > can be massaged to be so). The next step would be feeding back the > > commonality into the kernel, so that we really do have a single location > > where we need to add new defines that can just percolate through. > > > > So I think you mean to have the device-info stanzas shared... > > Fwiw I would suggest the same thing - have this this code in a single > place (be that kernel or libdrm) accessible by everyone. Otherwise > others (libva/beignet/mesa?) will copy it and it'll be a constant > chance to keep it in sync. That's what we said many years ago! Still trying to get there. > About the patch itself here are a few small nitpicks: > - intel_i81x_info seems to be unused One day! One day we will have that universal kms driver! (It's required for the ddx at least, so I should hook up the ids as well but really needs inclusion in i915_pciids.h first.) > - s/aaglelake/eaglelake/ > - capitalise the first letter of each codename ? Code already utilized codenames with the lowercase as filenames (for register sets). I thought about making it call lowercase() - it was just easier to match captilisation. > - Wikipedia lists Bearlake and Lakeport in both Gen3 and Gen4. Is it > busted or ... Or a confusion between chipset and GPU. From bspec, 915G, Grantsdale-G, GDG 915GM, Alviso, ALV 945G, Lakport, LPT, 945GM, Callistoga, CST Bearlake-B is listed for gen3, but that chipset shared more in common with the early gen4 GMCH. > - Ironlake, Clarkdale is listed as Ironlake, while Ironlake, > Arrandale as Arrandale. Guess those are the names people are more used > to ? It's just DevILK in the bspec, I'd forgotten Clarkdale but remembered Arrandale as the mobile variant (and I wasn't actually sure if Clarkdale was just the GPU-less chips). But since it is DevILK, we probably should just use Ironlake, hmm or we list all the shortnames as well for better cross-referencing. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/fbdev: Drain the suspend worker on retiring
Chris Wilson writes: > Since the suspend_work can arm itself if the console_lock() is currently > held elsewhere, simply calling flush_work() doesn't guarantee that the > work is idle upon return. To do so requires using cancel_work_sync(). > > Signed-off-by: Chris Wilson > Cc: Daniel Vetter > Cc: Mika Kuoppala Reviewed-by: Mika Kuoppala > --- > drivers/gpu/drm/i915/intel_fbdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > b/drivers/gpu/drm/i915/intel_fbdev.c > index 86b00c6db1a6..ef17d88a1bc7 100644 > --- a/drivers/gpu/drm/i915/intel_fbdev.c > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > @@ -768,7 +768,7 @@ void intel_fbdev_fini(struct drm_device *dev) > if (!ifbdev) > return; > > - flush_work(&dev_priv->fbdev_suspend_work); > + cancel_work_sync(&dev_priv->fbdev_suspend_work); > if (!current_is_async()) > intel_fbdev_sync(ifbdev); > > -- > 2.8.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] Bump libdrm-intel dependency library to a newer version that support softpin.
Required by commit 2603b98ca (aubdump: Support softpin bos). Signed-off-by: Marius Vlad CC: Kristian Høgsberg Kristensen --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index f05bcb0..ade9756 100644 --- a/configure.ac +++ b/configure.ac @@ -119,7 +119,7 @@ if test "x$GCC" = "xyes"; then fi AC_SUBST(ASSEMBLER_WARN_CFLAGS) -PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.64 libdrm]) +PKG_CHECK_MODULES(DRM, [libdrm_intel >= 2.4.68 libdrm]) PKG_CHECK_MODULES(PCIACCESS, [pciaccess >= 0.10]) case "$target_cpu" in -- 2.8.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH igt 01/17] lib: Start weaning off defunct intel_chipset.h
On 13 July 2016 at 14:34, Chris Wilson wrote: > On Wed, Jul 13, 2016 at 02:40:49PM +0200, Daniel Vetter wrote: >> On Wed, Jun 29, 2016 at 12:38:51PM +0100, Chris Wilson wrote: >> > Several years ago we made the plan of only having one canonical source >> > for i915_pciids.h, the kernel and everyone importing their definitions >> > from that. For consistency, we style the intel_device_info after the >> > kernel, most notably using a generation mask and a per-codename bitfield. >> > >> > This first step converts looking up the generation for a devid tree from >> > a massive if(devid)-chain to a (cached) table lookup. >> > >> > Signed-off-by: Chris Wilson >> >> Should we extract the kernels list of chipset into intel_chipset.c too, to >> make them 1:1 copies like with i915_pciids.h? With that this (entire >> series) here has my ack. Without I'm not sold that much on the churn >> really. > > Hmm, next up would be libdrm which is almost a duplication of igt (and > can be massaged to be so). The next step would be feeding back the > commonality into the kernel, so that we really do have a single location > where we need to add new defines that can just percolate through. > > So I think you mean to have the device-info stanzas shared... Fwiw I would suggest the same thing - have this this code in a single place (be that kernel or libdrm) accessible by everyone. Otherwise others (libva/beignet/mesa?) will copy it and it'll be a constant chance to keep it in sync. About the patch itself here are a few small nitpicks: - intel_i81x_info seems to be unused - s/aaglelake/eaglelake/ - capitalise the first letter of each codename ? - Wikipedia lists Bearlake and Lakeport in both Gen3 and Gen4. Is it busted or ... - Ironlake, Clarkdale is listed as Ironlake, while Ironlake, Arrandale as Arrandale. Guess those are the names people are more used to ? -Emil ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/8] drm/i915: Define a separate variable and control for RPS waitboost frequency
Chris Wilson writes:
> To allow the user finer control over waitboosting, allow them to set the
> frequency we request for the boost. This also them allows to effectively
> disable the boosting by setting the boost request to a low frequency.
>
> Signed-off-by: Chris Wilson
Reviewed-by: Mika Kuoppala
> ---
> drivers/gpu/drm/i915/i915_debugfs.c | 2 ++
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_irq.c | 9 +
> drivers/gpu/drm/i915/i915_sysfs.c | 38
> +
> drivers/gpu/drm/i915/intel_pm.c | 5 -
> 5 files changed, 50 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
> b/drivers/gpu/drm/i915/i915_debugfs.c
> index 844fea795bae..d1ff4cb9b90e 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1381,6 +1381,8 @@ static int i915_frequency_info(struct seq_file *m, void
> *unused)
> intel_gpu_freq(dev_priv, dev_priv->rps.idle_freq));
> seq_printf(m, "Min freq: %d MHz\n",
> intel_gpu_freq(dev_priv, dev_priv->rps.min_freq));
> + seq_printf(m, "Boost freq: %d MHz\n",
> +intel_gpu_freq(dev_priv, dev_priv->rps.boost_freq));
> seq_printf(m, "Max freq: %d MHz\n",
> intel_gpu_freq(dev_priv, dev_priv->rps.max_freq));
> seq_printf(m,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index e76cfe2e2471..fe85235367be 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1170,6 +1170,7 @@ struct intel_gen6_power_mgmt {
> u8 max_freq_softlimit; /* Max frequency permitted by the driver */
> u8 max_freq;/* Maximum frequency, RP0 if not overclocking */
> u8 min_freq;/* AKA RPn. Minimum frequency */
> + u8 boost_freq; /* Frequency to request when wait boosting */
> u8 idle_freq; /* Frequency to request when we are idle */
> u8 efficient_freq; /* AKA RPe. Pre-determined balanced frequency */
> u8 rp1_freq;/* "less than" RP0 power/freqency */
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 1c2aec392412..c8ed36765301 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1105,9 +1105,10 @@ static void gen6_pm_rps_work(struct work_struct *work)
> new_delay = dev_priv->rps.cur_freq;
> min = dev_priv->rps.min_freq_softlimit;
> max = dev_priv->rps.max_freq_softlimit;
> -
> - if (client_boost) {
> - new_delay = dev_priv->rps.max_freq_softlimit;
> + if (client_boost || any_waiters(dev_priv))
> + max = dev_priv->rps.max_freq;
> + if (client_boost && new_delay < dev_priv->rps.boost_freq) {
> + new_delay = dev_priv->rps.boost_freq;
> adj = 0;
> } else if (pm_iir & GEN6_PM_RP_UP_THRESHOLD) {
> if (adj > 0)
> @@ -1122,7 +1123,7 @@ static void gen6_pm_rps_work(struct work_struct *work)
> new_delay = dev_priv->rps.efficient_freq;
> adj = 0;
> }
> - } else if (any_waiters(dev_priv)) {
> + } else if (client_boost || any_waiters(dev_priv)) {
> adj = 0;
> } else if (pm_iir & GEN6_PM_RP_DOWN_TIMEOUT) {
> if (dev_priv->rps.cur_freq > dev_priv->rps.efficient_freq)
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c
> b/drivers/gpu/drm/i915/i915_sysfs.c
> index d61829e54f93..8c045ff47f0e 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -318,6 +318,41 @@ static ssize_t gt_cur_freq_mhz_show(struct device *kdev,
> return snprintf(buf, PAGE_SIZE, "%d\n", ret);
> }
>
> +static ssize_t gt_boost_freq_mhz_show(struct device *kdev, struct
> device_attribute *attr, char *buf)
> +{
> + struct drm_minor *minor = dev_to_drm_minor(kdev);
> + struct drm_i915_private *dev_priv = to_i915(minor->dev);
> +
> + return snprintf(buf, PAGE_SIZE, "%d\n",
> + intel_gpu_freq(dev_priv, dev_priv->rps.boost_freq));
> +}
> +
> +static ssize_t gt_boost_freq_mhz_store(struct device *kdev,
> +struct device_attribute *attr,
> +const char *buf, size_t count)
> +{
> + struct drm_minor *minor = dev_to_drm_minor(kdev);
> + struct drm_device *dev = minor->dev;
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + u32 val;
> + ssize_t ret;
> +
> + ret = kstrtou32(buf, 0, &val);
> + if (ret)
> + return ret;
> +
> + /* Validate against (static) hardware limits */
> + val = intel_freq_opcode(dev_priv, val);
> + if (val < dev_priv->rps.min_freq || val > dev_priv->rps.max_freq)
> + return -EINVAL;
> +
Re: [Intel-gfx] ✓ Ro.CI.BAT: success for series starting with [1/7] drm/i915: unify first-stage engine struct setup
On 13/07/16 16:57, Patchwork wrote: == Series Details == Series: series starting with [1/7] drm/i915: unify first-stage engine struct setup URL : https://patchwork.freedesktop.org/series/9821/ State : success == Summary == Series 9821v1 Series without cover letter http://patchwork.freedesktop.org/api/1.0/series/9821/revisions/1/mbox Test prime_self_import: Subgroup basic-llseek-size: incomplete -> PASS (ro-byt-n2820) fi-kbl-qkkr total:237 pass:174 dwarn:25 dfail:4 fail:7 skip:27 fi-skl-i7-6700k total:237 pass:200 dwarn:2 dfail:0 fail:9 skip:26 fi-snb-i7-2600 total:237 pass:188 dwarn:0 dfail:0 fail:9 skip:40 ro-bdw-i5-5250u total:237 pass:210 dwarn:2 dfail:0 fail:9 skip:16 ro-bdw-i7-5557U total:237 pass:211 dwarn:1 dfail:0 fail:9 skip:16 ro-bdw-i7-5600u total:237 pass:195 dwarn:2 dfail:0 fail:9 skip:31 ro-bsw-n3050 total:217 pass:170 dwarn:0 dfail:0 fail:4 skip:42 ro-byt-n2820 total:237 pass:178 dwarn:0 dfail:0 fail:7 skip:38 ro-hsw-i3-4010u total:237 pass:199 dwarn:5 dfail:2 fail:7 skip:24 ro-hsw-i7-4770r total:237 pass:204 dwarn:0 dfail:0 fail:9 skip:24 ro-ilk-i7-620lm total:237 pass:166 dwarn:0 dfail:0 fail:8 skip:63 ro-ilk1-i5-650 total:232 pass:166 dwarn:0 dfail:0 fail:8 skip:58 ro-ivb-i7-3770 total:237 pass:197 dwarn:0 dfail:0 fail:7 skip:33 ro-skl3-i5-6260u total:237 pass:213 dwarn:3 dfail:0 fail:9 skip:12 ro-snb-i7-2620M total:237 pass:187 dwarn:0 dfail:0 fail:9 skip:41 Results at /archive/results/CI_IGT_test/RO_Patchwork_1486/ 5cd2699 drm-intel-nightly: 2016y-07m-13d-14h-43m-27s UTC integration manifest f0b1666 drm/i915: Pull out some more common engine init code be8c237 drm/i915: Move common engine setup into intel_engine_cs.c a4cb7bc drm/i915: Simplify intel_init_ring_buffer prototype d3dfae1 drm/i915: Make more use of the shared engine irq setup ef59023 drm/i915: Unify engine init loop ac58b8a drm/i915: Prepare for engine init unification 8120dcf drm/i915: unify first-stage engine struct setup Merged to dinq, thanks for the patches and review! Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
On Thu, Jul 14, 2016 at 10:59:04AM +0100, Chris Wilson wrote: > On Thu, Jul 14, 2016 at 10:12:17AM +0200, Daniel Vetter wrote: > > On Thu, Jul 14, 2016 at 08:04:19AM +0100, Chris Wilson wrote: > > > vGEM buffers are useful for passing data between software clients and > > > hardware renders. By allowing the user to create and attach fences to > > > the exported vGEM buffers (on the dma-buf), the user can implement a > > > deferred renderer and queue hardware operations like flipping and then > > > signal the buffer readiness (i.e. this allows the user to schedule > > > operations out-of-order, but have them complete in-order). > > > > > > This also makes it much easier to write tightly controlled testcases for > > > dma-buf fencing and signaling between hardware drivers. > > > > > > v2: Don't pretend the fences exist in an ordered timeline, but allocate > > > a separate fence-context for each fence so that the fences are > > > unordered. > > > v3: Make the debug output more interesting, and so the signaled status. > > > > > > Testcase: igt/vgem_basic/dmabuf-fence > > > Signed-off-by: Chris Wilson > > > Cc: Sean Paul > > > Cc: Zach Reizner > > > Cc: Gustavo Padovan > > > Cc: Daniel Vetter > > > Acked-by: Zach Reizner > > > > One thing I completely forgotten: This allows userspace to hang kernel > > drivers. i915 (and other gpu drivers) can recover using hangcheck, but > > dumber drivers (v4l, if that ever happens) probably never except such a > > case. We've had a similar discusion with the userspace fences exposed in > > sw_fence, and decided to move all those ioctl into debugfs. I think we > > should do the same for this vgem-based debugging of implicit sync. Sorry > > for realizing this this late. > > One of the very tests I make is to ensure that we recover from such a > hang. I don't see the difference between this any of the other ways > userspace can shoot itself (and others) in the foot. So one solution would be to make vgem fences automatically timeout (with a flag for root to override for the sake of testing hang detection). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/7] drm/i915: Move common engine setup into intel_engine_cs.c
On Thu, Jul 14, 2016 at 10:25:39AM +0100, Tvrtko Ursulin wrote:
>
> On 13/07/16 17:04, Chris Wilson wrote:
> >On Wed, Jul 13, 2016 at 04:03:40PM +0100, Tvrtko Ursulin wrote:
> >>+ /*
> >>+* Catch failures to update intel_engines table when the new engines
> >>+* are added to the driver by a warning and disabling the forgotten
> >>+* engines.
> >>+*/
> >>+ if (WARN_ON(mask != INTEL_INFO(dev_priv)->ring_mask)) {
> >>+ struct intel_device_info *info =
> >>+ (struct intel_device_info *)&dev_priv->info;
> >
> >I snuck in mkwrite_device_info(), so
> >
> >if (WARN_ON(mask != INTEL_INFO(dev_priv)->ring_mask))
> > mkwrite_device_info(dev_priv)->ring_mask = mask;
>
> This part is just code movement, the block you quote exists before
> this series even!
>
> Follow up patch to this series would be easiest then, or a solitary
> precursor if you insist. Dangers of code movement with edits huh?
> (94b4f3ba483ace6dd4a3f881e19cc18bdbafa6ef)
I was also on a checkpatch witchhunt.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/7] drm/i915: unify first-stage engine struct setup
On Thu, Jul 14, 2016 at 10:19:16AM +0100, Tvrtko Ursulin wrote: > > On 13/07/16 17:01, Chris Wilson wrote: > >On Wed, Jul 13, 2016 at 04:03:35PM +0100, Tvrtko Ursulin wrote: > >>From: Dave Gordon > >> > >>intel_lrc.c has a table of "logical rings" (meaning engines), while > >>intel_ringbuffer.c has separately open-coded initialisation for each > >>engine. We can deduplicate this somewhat by using the same first-stage > >>engine-setup function for both modes. > >> > >>So here we expose the function that transfers information from the > >>static table of (all) known engines to the dev_priv->engine array of > >>engines available on this device (adjusting the names along the way) > >>and then embed calls to it in both the LRC and the legacy-mode setup. > >> > >>Signed-off-by: Dave Gordon > > > >LGTM, I didn't see anything that would impede applying this series, > >so > >Reviewed-by: Chris Wilson > > > >(Goes off to double check...) > > R-b for the series or 1/7 ? Series, just a few more wishlist items for later. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 50/64] drm/i915: Prepare i915_gem_active for annotations
On Thu, Jul 14, 2016 at 10:32:05AM +0100, Tvrtko Ursulin wrote:
>
> On 13/07/16 16:58, Chris Wilson wrote:
> >On Wed, Jul 13, 2016 at 04:40:03PM +0100, Tvrtko Ursulin wrote:
>
> [snip]
>
> >>> } else {
> >>> for (i = 0; i < I915_NUM_ENGINES; i++) {
> >>> struct drm_i915_gem_request *req;
> >>>
> >>>- req = obj->last_read[i].request;
> >>>+ req = i915_gem_active_peek(&obj->last_read[i]);
> >>> if (req == NULL)
> >>> continue;
> >>>
> >>>- requests[n++] = i915_gem_request_get(req);
> >>>+ requests[n++] = req;
> >>> }
> >>> }
> >>>
> >>>@@ -2383,25 +2386,27 @@ void i915_vma_move_to_active(struct i915_vma *vma,
> >>> static void
> >>> i915_gem_object_retire__write(struct drm_i915_gem_object *obj)
> >>> {
> >>>- GEM_BUG_ON(!obj->last_write.request);
> >>>- GEM_BUG_ON(!(obj->active &
> >>>intel_engine_flag(obj->last_write.request->engine)));
> >>>+ GEM_BUG_ON(!__i915_gem_active_is_busy(&obj->last_write));
> >>>+ GEM_BUG_ON(!(obj->active &
> >>>intel_engine_flag(i915_gem_active_get_engine(&obj->last_write;
> >>>
> >>>- i915_gem_request_assign(&obj->last_write.request, NULL);
> >>>+ i915_gem_active_set(&obj->last_write, NULL);
> >>
> >>Aha!
> >
> >Drat. Didn't think I did that...
> >
> >Oh well, no excuses now but to go back in time and make the change
> >earlier. It does get removed eventually!
>
> Probably not worth it. You can have a special dispensation since I
> am reviewing all the same lines of code patch after patch anyway. :)
Too late, since this patch had to be fixed, I did the earlier fixup as
well.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
On Thu, Jul 14, 2016 at 10:12:17AM +0200, Daniel Vetter wrote: > On Thu, Jul 14, 2016 at 08:04:19AM +0100, Chris Wilson wrote: > > vGEM buffers are useful for passing data between software clients and > > hardware renders. By allowing the user to create and attach fences to > > the exported vGEM buffers (on the dma-buf), the user can implement a > > deferred renderer and queue hardware operations like flipping and then > > signal the buffer readiness (i.e. this allows the user to schedule > > operations out-of-order, but have them complete in-order). > > > > This also makes it much easier to write tightly controlled testcases for > > dma-buf fencing and signaling between hardware drivers. > > > > v2: Don't pretend the fences exist in an ordered timeline, but allocate > > a separate fence-context for each fence so that the fences are > > unordered. > > v3: Make the debug output more interesting, and so the signaled status. > > > > Testcase: igt/vgem_basic/dmabuf-fence > > Signed-off-by: Chris Wilson > > Cc: Sean Paul > > Cc: Zach Reizner > > Cc: Gustavo Padovan > > Cc: Daniel Vetter > > Acked-by: Zach Reizner > > One thing I completely forgotten: This allows userspace to hang kernel > drivers. i915 (and other gpu drivers) can recover using hangcheck, but > dumber drivers (v4l, if that ever happens) probably never except such a > case. We've had a similar discusion with the userspace fences exposed in > sw_fence, and decided to move all those ioctl into debugfs. I think we > should do the same for this vgem-based debugging of implicit sync. Sorry > for realizing this this late. One of the very tests I make is to ensure that we recover from such a hang. I don't see the difference between this any of the other ways userspace can shoot itself (and others) in the foot. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 50/64] drm/i915: Prepare i915_gem_active for annotations
On 13/07/16 16:58, Chris Wilson wrote:
On Wed, Jul 13, 2016 at 04:40:03PM +0100, Tvrtko Ursulin wrote:
[snip]
} else {
for (i = 0; i < I915_NUM_ENGINES; i++) {
struct drm_i915_gem_request *req;
- req = obj->last_read[i].request;
+ req = i915_gem_active_peek(&obj->last_read[i]);
if (req == NULL)
continue;
- requests[n++] = i915_gem_request_get(req);
+ requests[n++] = req;
}
}
@@ -2383,25 +2386,27 @@ void i915_vma_move_to_active(struct i915_vma *vma,
static void
i915_gem_object_retire__write(struct drm_i915_gem_object *obj)
{
- GEM_BUG_ON(!obj->last_write.request);
- GEM_BUG_ON(!(obj->active &
intel_engine_flag(obj->last_write.request->engine)));
+ GEM_BUG_ON(!__i915_gem_active_is_busy(&obj->last_write));
+ GEM_BUG_ON(!(obj->active &
intel_engine_flag(i915_gem_active_get_engine(&obj->last_write;
- i915_gem_request_assign(&obj->last_write.request, NULL);
+ i915_gem_active_set(&obj->last_write, NULL);
Aha!
Drat. Didn't think I did that...
Oh well, no excuses now but to go back in time and make the change
earlier. It does get removed eventually!
Probably not worth it. You can have a special dispensation since I am
reviewing all the same lines of code patch after patch anyway. :)
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/7] drm/i915: Move common engine setup into intel_engine_cs.c
On 13/07/16 17:04, Chris Wilson wrote:
On Wed, Jul 13, 2016 at 04:03:40PM +0100, Tvrtko Ursulin wrote:
+ /*
+* Catch failures to update intel_engines table when the new engines
+* are added to the driver by a warning and disabling the forgotten
+* engines.
+*/
+ if (WARN_ON(mask != INTEL_INFO(dev_priv)->ring_mask)) {
+ struct intel_device_info *info =
+ (struct intel_device_info *)&dev_priv->info;
I snuck in mkwrite_device_info(), so
if (WARN_ON(mask != INTEL_INFO(dev_priv)->ring_mask))
mkwrite_device_info(dev_priv)->ring_mask = mask;
This part is just code movement, the block you quote exists before this
series even!
Follow up patch to this series would be easiest then, or a solitary
precursor if you insist. Dangers of code movement with edits huh?
(94b4f3ba483ace6dd4a3f881e19cc18bdbafa6ef)
Regards,
Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/7] drm/i915: unify first-stage engine struct setup
On 13/07/16 17:01, Chris Wilson wrote: On Wed, Jul 13, 2016 at 04:03:35PM +0100, Tvrtko Ursulin wrote: From: Dave Gordon intel_lrc.c has a table of "logical rings" (meaning engines), while intel_ringbuffer.c has separately open-coded initialisation for each engine. We can deduplicate this somewhat by using the same first-stage engine-setup function for both modes. So here we expose the function that transfers information from the static table of (all) known engines to the dev_priv->engine array of engines available on this device (adjusting the names along the way) and then embed calls to it in both the LRC and the legacy-mode setup. Signed-off-by: Dave Gordon LGTM, I didn't see anything that would impede applying this series, so Reviewed-by: Chris Wilson (Goes off to double check...) R-b for the series or 1/7 ? Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 2/3] drm/i915/bxt: Fix inadvertent CPU snooping due to incorrect MOCS config
> -Original Message-
> From: Deak, Imre
> Sent: Friday, July 1, 2016 21:40
> To: intel-gfx@lists.freedesktop.org
> Cc: Ville Syrjälä ; Chris Wilson wilson.co.uk>; Yang, Rong R ; Zhao, Yakui
> ; Tamminen, Eero T
> Subject: [PATCH v3 2/3] drm/i915/bxt: Fix inadvertent CPU snooping due to
> incorrect MOCS config
>
> Setting a write-back cache policy in the MOCS entry definition also implies
> snooping, which has a considerable overhead. This is unexpected for a few
> reasons:
> - From user-space's point of view since it didn't want a coherent
> surface (it didn't set the buffer as such via the set caching IOCTL).
> - There is a separate MOCS entry field for snooping (which we never
> set).
> - This MOCS table is about caching in (e)LLC and there is no (e)LLC on
> BXT. There is a separate table for L3 cache control.
>
> Considering the above the current behavior of snooping looks like an
> unintentional side-effect of the WB setting. Changing it to be LLC-UC gets rid
> of the snooping without any ill-effects. For a coherent surface the
> application
> would use a separate MOCS entry at index 1 and call the set caching IOCTL to
> setup the PTE entries for the corresponding buffer to be snooped. In the
> future we could also add a new MOCS entry for coherent surfaces.
>
> This resulted in 70% improvement in synthetic texturing benchmarks.
>
> Kudos to Valtteri Rantala, Eero Tamminen and Michael T Frederick and Ville
> who helped to narrow the source of problem to the kernel and to the
> snooping behaviour in particular.
>
> With a follow-up change to adjust the 3rd entry value
> igt/gem_mocs_settings is passing after this change.
>
> v2:
> - Rebase on v2 of patch 1/2.
> v3:
> - Set the entry as LLC uncached instead of PTE-passthrough. This way
> we also keep snooping disabled, but we also make the cacheability/
> coherency setting indepent of the PTE which is managed by the
> kernel. (Chris)
About 20% improvement in OpenCL benchmark luxmark.
Add: Tested-by: Rong R Yang
> CC: Rong R Yang
> CC: Yakui Zhao
> CC: Valtteri Rantala
> CC: Eero Tamminen
> CC: Michael T Frederick
> CC: Ville Syrjälä
> CC: Chris Wilson
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/intel_mocs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_mocs.c
> b/drivers/gpu/drm/i915/intel_mocs.c
> index d36e609..927825f 100644
> --- a/drivers/gpu/drm/i915/intel_mocs.c
> +++ b/drivers/gpu/drm/i915/intel_mocs.c
> @@ -149,8 +149,8 @@ static const struct drm_i915_mocs_entry
> broxton_mocs_table[] = {
> .l3cc_value =L3_ESC(0) | L3_SCC(0) | L3_CACHEABILITY(L3_WB),
> },
> {
> - /* 0x003b */
> - .control_value = LE_CACHEABILITY(LE_WB) |
> + /* 0x0039 */
> + .control_value = LE_CACHEABILITY(LE_UC) |
> LE_TGT_CACHE(LE_TC_LLC_ELLC) |
> LE_LRUM(3) | LE_AOM(0) | LE_RSC(0) | LE_SCC(0) |
> LE_PFM(0) | LE_SCF(0),
> --
> 2.5.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PULL] topic/drm-misc
Hi Dave, I recovered dri-devel backlog from my vacation, more misc stuff: - of_put_node fixes from Peter Chen (not all yet) - more patches from Gustavo to use kms-native drm_crtc_vblank_* funcs - docs sphinxification from Lukas Wunner - bunch of fixes all over from Dan Carpenter - more follow up work from Chris register/unregister rework in various places - vgem dma-buf export (for writing testcases) - small things all over from tons of different people This is just the delta against the previous pull request, pls take them both. Cheers, Daniel The following changes since commit 2a3467063ae3b17264578626dec2377dd48cd1c3: Merge tag 'mediatek-drm-2016-06-20' of git://git.pengutronix.de/git/pza/linux into drm-next (2016-06-24 13:16:07 +1000) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/topic/drm-misc-2016-07-14 for you to fetch changes up to 01d3434a565ada5ca084c68ec1e087ada5a7b157: drm: Don't overwrite user ioctl arg unless requested (2016-07-14 10:12:50 +0200) Alexey Khoroshilov (1): drm_aux-dev: fix error handling in drm_dp_aux_dev_init() Benjamin Herrenschmidt (1): drm: Fix broken use of _PAGE_NO_CACHE on powerpc Bhaktipriya Shridhar (1): drm/qxl: Remove deprecated create_singlethread_workqueue Chris Wilson (8): drm/vgem: Fix mmaping drm/vgem: Enable dmabuf interface for export drm: Unexport drm_connector_register_all() drm: Do a full device unregister when unplugging drm/udl: Unplugging a device now unregisters it drm: Restore double clflush on the last partial cacheline drm/vgem: Use PAGE_KERNEL in place of x86-specific PAGE_KERNEL_IO drm: Don't overwrite user ioctl arg unless requested Dan Carpenter (3): drm/mediatek/mtk_mipi_tx: checking the wrong variable qxl: check for kmap failures qxl: silence uninitialized variable warning Daniel Vetter (1): drm: Resurrect atomic rmfb code Frank Binns (2): drm: fix some spelling mistakes drm/vmwgfx: Stop checking minor type directly Gustavo Padovan (8): drm: make drm_vblank_count_and_time() static drm/armada: use drm_crtc_handle_vblank() drm/atmel: use drm_crtc_handle_vblank() drm/nouveau: use drm_crtc_handle_vblank() drm/rcar-du: use drm_crtc_handle_vblank() drm/tilcdc: use drm_crtc_handle_vblank() MAINTAINERS: add entry for the Sync File Framework dma-buf/sync_file: improve Kconfig description for Sync Files Lukas Wunner (16): drm/nouveau: Don't leak runtime pm ref on driver unload drm/nouveau: Forbid runtime pm on driver unload drm/radeon: Don't leak runtime pm ref on driver unload drm/radeon: Don't leak runtime pm ref on driver load drm/radeon: Forbid runtime pm on driver unload drm/amdgpu: Don't leak runtime pm ref on driver unload drm/amdgpu: Don't leak runtime pm ref on driver load drm/amdgpu: Forbid runtime pm on driver unload drm: Add helpers to turn off CRTCs drm/nouveau: Turn off CRTCs on driver unload drm/radeon: Turn off CRTCs on driver unload drm/amdgpu: Turn off CRTCs on driver unload drm: Use helper to turn off CRTC drm/i2c/ch7006: Use helper to turn off CRTC drm/nouveau/dispnv04: Use helper to turn off CRTC vga_switcheroo: Sphinxify docs Masanari Iida (1): drm: Fix a typo in drm_ioctl.c Michel Dänzer (1): drm: Only handle _DRM_VBLANK_NEXTONMISS once Peter Chen (5): gpu: drm: sti_compositor: add missing of_node_put after calling of_parse_phandle gpu: drm: sti_vdo: add missing of_node_put after calling of_parse_phandle gpu: drm: sti_hqvdp: add missing of_node_put after calling of_parse_phandle gpu: drm: sti_vtg: add missing of_node_put after calling of_parse_phandle gpu: drm: rockchip_drm_drv: add missing of_node_put after calling of_parse_phandle Thierry Reding (2): drm/qxl: Remove dead code drm/dsi: Make set_tear_scanline command consistent Tobias Jakobi (1): drm/exynos: make fbdev support really optional Xinliang Liu (1): drm/hisilicon: Fix ADE vblank on/off handling Documentation/gpu/drm-internals.rst | 4 +- Documentation/gpu/vga-switcheroo.rst| 8 +- MAINTAINERS | 11 ++ drivers/dma-buf/Kconfig | 15 +- drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 12 +- drivers/gpu/drm/armada/armada_crtc.c| 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_crtc.c | 2 +- drivers/gpu/drm/drm_atomic.c| 66 +++ drivers/gpu/drm/drm_cache.c | 1 + drivers/gpu/drm/drm_crtc.c | 78 +--- drivers/gpu/drm/drm_crtc_internal.h | 1 + drivers/gpu/drm/drm_dp_aux_dev.c
[Intel-gfx] [PULL] drm-intel-next
Hi Dave, drm-intel-next-2016-07-11: - select igt testing depencies for CONFIG_DRM_I915_DEBUG (Chris) - track outputs in crtc state and clean up all our ad-hoc connector/encoder walking in modest code (Ville) - demidlayer drm_device/drm_i915_private (Chris Wilson) - thundering herd fix from Chris Wilson, with lots of help from Tvrtko Ursulin - piles of assorted clean and fallout from the thundering herd fix - documentation and more tuning for waitboosting (Chris) - pooled EU support on bxt (Arun Siluvery) - bxt support is no longer considered prelimary! - ring/engine vfunc cleanup from Tvrtko - introduce intel_wait_for_register helper (Chris) - opregion updates (Jani Nukla) - tuning and fixes for wait_for macros (Tvrkto&Imre) - more kabylake pci ids (Rodrigo) - pps cleanup and fixes for bxt (Imre) - move sink crc support over to atomic state (Maarten) - fix up async fbdev init ordering (Chris) - fbc fixes from Paulo and Chris Final feature pull request for 4.8. Cheers, Daniel The following changes since commit 2a3467063ae3b17264578626dec2377dd48cd1c3: Merge tag 'mediatek-drm-2016-06-20' of git://git.pengutronix.de/git/pza/linux into drm-next (2016-06-24 13:16:07 +1000) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/drm-intel-next-2016-07-11 for you to fetch changes up to 0b2c0582f1570bfc95aa9ac1cd340a215d8e8335: drm/i915: Update DRIVER_DATE to 20160711 (2016-07-11 09:18:31 +0200) - select igt testing depencies for CONFIG_DRM_I915_DEBUG (Chris) - track outputs in crtc state and clean up all our ad-hoc connector/encoder walking in modest code (Ville) - demidlayer drm_device/drm_i915_private (Chris Wilson) - thundering herd fix from Chris Wilson, with lots of help from Tvrtko Ursulin - piles of assorted clean and fallout from the thundering herd fix - documentation and more tuning for waitboosting (Chris) - pooled EU support on bxt (Arun Siluvery) - bxt support is no longer considered prelimary! - ring/engine vfunc cleanup from Tvrtko - introduce intel_wait_for_register helper (Chris) - opregion updates (Jani Nukla) - tuning and fixes for wait_for macros (Tvrkto&Imre) - more kabylake pci ids (Rodrigo) - pps cleanup and fixes for bxt (Imre) - move sink crc support over to atomic state (Maarten) - fix up async fbdev init ordering (Chris) - fbc fixes from Paulo and Chris Chris Wilson (149): drm/i915: Extract checking for backing struct pages to a helper drm/i915: pwrite/pread do not require obj->base.filp, just pages drm/i915: use ORIGIN_CPU for frontbuffer invalidation on WC mmaps drm/i915/fbdev: Perform async fbdev initialisation much later drm/i915/fbdev: Limit the global async-domain synchronization drm/i915/fbdev: Flush mode configuration before lastclose drm/i915/gvt: Mark i915.enable_gvt as false if loading fails drm/i915: Move panel's backlight setup next to panel init drm/i915: Move registration actions to connector->late_register drm/i915: Move backlight registration to connector registration drm/i915: Move connector registration to driver registration drm/i915: Register debugfs interface last drm/i915: Demidlayer driver loading drm/i915: Demidlayer driver unloading drm/i915: Remove redundant drm_connector_register_all() drm/i915: Start exploiting drm_device subclassing drm/i915: Merge i915_dma.c into i915_drv.c drm/i915: Remove user controllable DRM_ERROR for i915_getparam() drm/i915: Remove user controllable DRM_ERROR for intel_get_pipe_from_crtc_id() drm/i915: Split out the PCI driver interface to i915_pci.c drm/i915: Move module init/exit to i915_pci.c drm/i915: Skip idling an idle engine drm/i915: Move legacy kernel context pinning to intel_ringbuffer.c drm/i915: Treat kernel context as initialised drm/i915: Mark all default contexts as uninitialised after context loss drm/i915: No need to wait for idle on L3 remap drm/i915: Split idling from forcing context switch drm/i915: Only switch to default context when evicting from GGTT drm/i915: Remove request->reset_counter Revert "drm/i915: Use atomic commits for legacy page_flips" drm/i915: Use a hybrid scheme for fast register waits drm/i915: Convert sandybridge_pcode_*() to use intel_wait_for_register() drm/i915: Convert wait_for(I915_READ(reg)) to intel_wait_for_register() drm/i915: Convert wait_for(I915_READ(reg)) to intel_wait_for_register() drm/i915: Convert wait_for(I915_READ(reg)) to intel_wait_for_register() drm/i915: Convert wait_for(I915_READ(reg)) to intel_wait_for_register() drm/i915: Convert wait_for(I915_READ(reg)) to intel_wait_for_register() drm/i915: Convert wait_for(I915_READ(reg)) to intel_wait_for_register() drm/i
Re: [Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
On Thu, Jul 14, 2016 at 08:04:19AM +0100, Chris Wilson wrote:
> vGEM buffers are useful for passing data between software clients and
> hardware renders. By allowing the user to create and attach fences to
> the exported vGEM buffers (on the dma-buf), the user can implement a
> deferred renderer and queue hardware operations like flipping and then
> signal the buffer readiness (i.e. this allows the user to schedule
> operations out-of-order, but have them complete in-order).
>
> This also makes it much easier to write tightly controlled testcases for
> dma-buf fencing and signaling between hardware drivers.
>
> v2: Don't pretend the fences exist in an ordered timeline, but allocate
> a separate fence-context for each fence so that the fences are
> unordered.
> v3: Make the debug output more interesting, and so the signaled status.
>
> Testcase: igt/vgem_basic/dmabuf-fence
> Signed-off-by: Chris Wilson
> Cc: Sean Paul
> Cc: Zach Reizner
> Cc: Gustavo Padovan
> Cc: Daniel Vetter
> Acked-by: Zach Reizner
One thing I completely forgotten: This allows userspace to hang kernel
drivers. i915 (and other gpu drivers) can recover using hangcheck, but
dumber drivers (v4l, if that ever happens) probably never except such a
case. We've had a similar discusion with the userspace fences exposed in
sw_fence, and decided to move all those ioctl into debugfs. I think we
should do the same for this vgem-based debugging of implicit sync. Sorry
for realizing this this late.
-Daniel
> ---
> drivers/gpu/drm/vgem/Makefile | 2 +-
> drivers/gpu/drm/vgem/vgem_drv.c | 34 +++
> drivers/gpu/drm/vgem/vgem_drv.h | 16 +++
> drivers/gpu/drm/vgem/vgem_fence.c | 207
> ++
> include/uapi/drm/vgem_drm.h | 62
> 5 files changed, 320 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/vgem/vgem_fence.c
> create mode 100644 include/uapi/drm/vgem_drm.h
>
> diff --git a/drivers/gpu/drm/vgem/Makefile b/drivers/gpu/drm/vgem/Makefile
> index 3f4c7b842028..bfcdea1330e6 100644
> --- a/drivers/gpu/drm/vgem/Makefile
> +++ b/drivers/gpu/drm/vgem/Makefile
> @@ -1,4 +1,4 @@
> ccflags-y := -Iinclude/drm
> -vgem-y := vgem_drv.o
> +vgem-y := vgem_drv.o vgem_fence.o
>
> obj-$(CONFIG_DRM_VGEM) += vgem.o
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
> index 29c2aab3c1a7..c15bafb06665 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.c
> +++ b/drivers/gpu/drm/vgem/vgem_drv.c
> @@ -83,6 +83,34 @@ static const struct vm_operations_struct vgem_gem_vm_ops =
> {
> .close = drm_gem_vm_close,
> };
>
> +static int vgem_open(struct drm_device *dev, struct drm_file *file)
> +{
> + struct vgem_file *vfile;
> + int ret;
> +
> + vfile = kzalloc(sizeof(*vfile), GFP_KERNEL);
> + if (!vfile)
> + return -ENOMEM;
> +
> + file->driver_priv = vfile;
> +
> + ret = vgem_fence_open(vfile);
> + if (ret) {
> + kfree(vfile);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static void vgem_preclose(struct drm_device *dev, struct drm_file *file)
> +{
> + struct vgem_file *vfile = file->driver_priv;
> +
> + vgem_fence_close(vfile);
> + kfree(vfile);
> +}
> +
> /* ioctls */
>
> static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
> @@ -164,6 +192,8 @@ unref:
> }
>
> static struct drm_ioctl_desc vgem_ioctls[] = {
> + DRM_IOCTL_DEF_DRV(VGEM_FENCE_ATTACH, vgem_fence_attach_ioctl,
> DRM_AUTH|DRM_RENDER_ALLOW),
> + DRM_IOCTL_DEF_DRV(VGEM_FENCE_SIGNAL, vgem_fence_signal_ioctl,
> DRM_AUTH|DRM_RENDER_ALLOW),
> };
>
> static int vgem_mmap(struct file *filp, struct vm_area_struct *vma)
> @@ -271,9 +301,12 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
>
> static struct drm_driver vgem_driver = {
> .driver_features= DRIVER_GEM | DRIVER_PRIME,
> + .open = vgem_open,
> + .preclose = vgem_preclose,
> .gem_free_object_unlocked = vgem_gem_free_object,
> .gem_vm_ops = &vgem_gem_vm_ops,
> .ioctls = vgem_ioctls,
> + .num_ioctls = ARRAY_SIZE(vgem_ioctls),
> .fops = &vgem_driver_fops,
>
> .dumb_create= vgem_gem_dumb_create,
> @@ -328,5 +361,6 @@ module_init(vgem_init);
> module_exit(vgem_exit);
>
> MODULE_AUTHOR("Red Hat, Inc.");
> +MODULE_AUTHOR("Intel Corporation");
> MODULE_DESCRIPTION(DRIVER_DESC);
> MODULE_LICENSE("GPL and additional rights");
> diff --git a/drivers/gpu/drm/vgem/vgem_drv.h b/drivers/gpu/drm/vgem/vgem_drv.h
> index 988cbaae7588..1f8798ad329c 100644
> --- a/drivers/gpu/drm/vgem/vgem_drv.h
> +++ b/drivers/gpu/drm/vgem/vgem_drv.h
> @@ -32,9 +32,25 @@
> #include
> #include
>
> +#include
> +
> +struct vgem_file {
> + struct idr fence_idr;
> + struct mutex fence_m
Re: [Intel-gfx] [PATCH 1/8] drm/i915: Flush GT idle status upon reset
On Thu, Jul 14, 2016 at 10:53:20AM +0300, Joonas Lahtinen wrote:
> On ke, 2016-07-13 at 09:10 +0100, Chris Wilson wrote:
> > Upon resetting the GPU, we force the engines to be idle by clearing
> > their request lists. However, I neglected to clear the GT active status
> > and so the next request following the reset was not marking the device
> > as busy again. (We had to wait until any outstanding retire worker
> > finally ran and cleared the active status.)
> >
> > Fixes: 67d97da34917 ("drm/i915: Only start retire worker when idle")
> > Testcase: igt/pm_rps/reset
> > Signed-off-by: Chris Wilson
> > Cc: Joonas Lahtinen
>
> No FDO bug associated?
Unlikely, I had to rewrite the test case to hit it reliably.
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/8] drm/i915: Flush GT idle status upon reset
On ke, 2016-07-13 at 09:10 +0100, Chris Wilson wrote:
> Upon resetting the GPU, we force the engines to be idle by clearing
> their request lists. However, I neglected to clear the GT active status
> and so the next request following the reset was not marking the device
> as busy again. (We had to wait until any outstanding retire worker
> finally ran and cleared the active status.)
>
> Fixes: 67d97da34917 ("drm/i915: Only start retire worker when idle")
> Testcase: igt/pm_rps/reset
> Signed-off-by: Chris Wilson
> Cc: Joonas Lahtinen
No FDO bug associated?
Reviewed-by: Joonas Lahtinen
> ---
> drivers/gpu/drm/i915/i915_gem.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index adeca0ec4cfb..9d8c26f42dee 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3169,6 +3169,8 @@ static void i915_gem_reset_engine_cleanup(struct
> intel_engine_cs *engine)
> }
>
> intel_ring_init_seqno(engine, engine->last_submitted_seqno);
> +
> + engine->i915->gt.active_engines &= ~intel_engine_flag(engine);
> }
>
> void i915_gem_reset(struct drm_device *dev)
> @@ -3186,6 +3188,7 @@ void i915_gem_reset(struct drm_device *dev)
>
> for_each_engine(engine, dev_priv)
> i915_gem_reset_engine_cleanup(engine);
> + mod_delayed_work(dev_priv->wq, &dev_priv->gt.idle_work, 0);
>
> i915_gem_context_reset(dev);
>
--
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✗ Ro.CI.BAT: failure for drm/i915: set proper N/M in modeset
== Series Details == Series: drm/i915: set proper N/M in modeset URL : https://patchwork.freedesktop.org/series/9857/ State : failure == Summary == Series 9857v1 drm/i915: set proper N/M in modeset http://patchwork.freedesktop.org/api/1.0/series/9857/revisions/1/mbox Test drv_module_reload_basic: dmesg-warn -> SKIP (fi-skl-i5-6260u) Test kms_pipe_crc_basic: Subgroup suspend-read-crc-pipe-b: pass -> DMESG-WARN (ro-skl3-i5-6260u) Test prime_self_import: Subgroup basic-with_fd_dup: pass -> INCOMPLETE (ro-byt-n2820) Subgroup basic-with_one_bo_two_files: pass -> INCOMPLETE (ro-byt-n2820) Subgroup basic-with_two_bos: pass -> INCOMPLETE (ro-byt-n2820) fi-kbl-qkkr total:237 pass:174 dwarn:27 dfail:2 fail:7 skip:27 fi-skl-i5-6260u total:237 pass:189 dwarn:26 dfail:2 fail:7 skip:13 fi-skl-i7-6700k total:237 pass:200 dwarn:2 dfail:0 fail:9 skip:26 fi-snb-i7-2600 total:237 pass:188 dwarn:0 dfail:0 fail:9 skip:40 ro-bdw-i5-5250u total:237 pass:210 dwarn:2 dfail:0 fail:9 skip:16 ro-bdw-i7-5600u total:237 pass:195 dwarn:2 dfail:0 fail:9 skip:31 ro-bsw-n3050 total:217 pass:170 dwarn:0 dfail:0 fail:4 skip:42 ro-byt-n2820 total:237 pass:174 dwarn:0 dfail:0 fail:7 skip:38 ro-hsw-i3-4010u total:237 pass:199 dwarn:5 dfail:2 fail:7 skip:24 ro-hsw-i7-4770r total:237 pass:204 dwarn:0 dfail:0 fail:9 skip:24 ro-ilk-i7-620lm total:237 pass:166 dwarn:0 dfail:0 fail:8 skip:63 ro-ilk1-i5-650 total:232 pass:166 dwarn:0 dfail:0 fail:8 skip:58 ro-ivb-i7-3770 total:237 pass:197 dwarn:0 dfail:0 fail:7 skip:33 ro-skl3-i5-6260u total:237 pass:212 dwarn:4 dfail:0 fail:9 skip:12 ro-snb-i7-2620M total:237 pass:187 dwarn:0 dfail:0 fail:9 skip:41 ro-bdw-i7-5557U failed to connect after reboot Results at /archive/results/CI_IGT_test/RO_Patchwork_1490/ 5cd2699 drm-intel-nightly: 2016y-07m-13d-14h-43m-27s UTC integration manifest 9ea6eab drm/i915: set proper N/M in modeset ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: set proper N/M in modeset
From: Libin Yang
When modeset occurs and the LS_CLK is set to some
special values in DP mode, the N/M need to be set
manually if audio is playing.
Also, the patch applies
commit 7e8275c2f2bb ("drm/i915: set proper N/CTS in modeset")
to APL platform.
Signed-off-by: Libin Yang
---
drivers/gpu/drm/i915/i915_reg.h| 6 ++
drivers/gpu/drm/i915/intel_audio.c | 116 -
2 files changed, 108 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8bfde75..2f9d00e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -7351,6 +7351,12 @@ enum {
#define _HSW_AUD_CONFIG_B 0x65100
#define HSW_AUD_CFG(pipe) _MMIO_PIPE(pipe, _HSW_AUD_CONFIG_A,
_HSW_AUD_CONFIG_B)
+#define _HSW_AUD_M_CTS_ENABLE_A0x65028
+#define _HSW_AUD_M_CTS_ENABLE_B0x65128
+#define HSW_AUD_M_CTS_ENABLE(pipe) _MMIO_PIPE(pipe,
_HSW_AUD_M_CTS_ENABLE_A, _HSW_AUD_M_CTS_ENABLE_B)
+#define AUD_M_CTS_M_VALUE_INDEX (1 << 21)
+#define AUD_M_CTS_M_PROG_ENABLE (1 << 20)
+
#define _HSW_AUD_MISC_CTRL_A 0x65010
#define _HSW_AUD_MISC_CTRL_B 0x65110
#define HSW_AUD_MISC_CTRL(pipe)_MMIO_PIPE(pipe,
_HSW_AUD_MISC_CTRL_A, _HSW_AUD_MISC_CTRL_B)
diff --git a/drivers/gpu/drm/i915/intel_audio.c
b/drivers/gpu/drm/i915/intel_audio.c
index 6700a7b..e2e4d4b 100644
--- a/drivers/gpu/drm/i915/intel_audio.c
+++ b/drivers/gpu/drm/i915/intel_audio.c
@@ -98,6 +98,22 @@ static const struct {
{ 192000, TMDS_297M, 20480, 247500 },
};
+#define LC_533M 533250
+#define LC_148M 148500
+static const struct {
+ int sample_rate;
+ int clock;
+ int n;
+ int m;
+} aud_nm[] = {
+ {48000, LC_533M, 88875, 4096},
+ {44100, LC_533M, 148125, 6272},
+ {32000, LC_533M, 266625, 8192},
+ {48000, LC_148M, 12375, 2048},
+ {44100, LC_148M, 20625, 3136},
+ {32000, LC_148M, 37125, 4096},
+};
+
/* get AUD_CONFIG_PIXEL_CLOCK_HDMI_* value for mode */
static u32 audio_config_hdmi_pixel_clock(const struct drm_display_mode
*adjusted_mode)
{
@@ -121,20 +137,50 @@ static u32 audio_config_hdmi_pixel_clock(const struct
drm_display_mode *adjusted
return hdmi_audio_clock[i].config;
}
-static int audio_config_get_n(const struct drm_display_mode *mode, int rate)
+static int audio_config_get_n(struct intel_crtc *crtc,
+ const struct drm_display_mode *mode, int rate)
+{
+ int i;
+
+ if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_HDMI)) {
+ for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
+ if ((rate == aud_ncts[i].sample_rate) &&
+ (mode->clock == aud_ncts[i].clock)) {
+ return aud_ncts[i].n;
+ }
+ }
+ }
+
+ if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
+ for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
+ if ((rate == aud_nm[i].sample_rate) &&
+ (mode->clock == aud_nm[i].clock)) {
+ return aud_nm[i].n;
+ }
+ }
+ }
+ return 0;
+}
+
+static int audio_config_get_m(struct intel_crtc *crtc,
+ const struct drm_display_mode *mode, int rate)
{
int i;
- for (i = 0; i < ARRAY_SIZE(aud_ncts); i++) {
- if ((rate == aud_ncts[i].sample_rate) &&
- (mode->clock == aud_ncts[i].clock)) {
- return aud_ncts[i].n;
+ if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP)) {
+ for (i = 0; i < ARRAY_SIZE(aud_nm); i++) {
+ if ((rate == aud_nm[i].sample_rate) &&
+ (mode->clock == aud_nm[i].clock)) {
+ return aud_nm[i].m;
+ }
}
}
+
return 0;
}
-static uint32_t audio_config_setup_n_reg(int n, uint32_t val)
+static uint32_t audio_config_setup_n_reg(struct intel_crtc *crtc,
+int n, uint32_t val)
{
int n_low, n_up;
uint32_t tmp = val;
@@ -145,6 +191,23 @@ static uint32_t audio_config_setup_n_reg(int n, uint32_t
val)
tmp |= ((n_up << AUD_CONFIG_UPPER_N_SHIFT) |
(n_low << AUD_CONFIG_LOWER_N_SHIFT) |
AUD_CONFIG_N_PROG_ENABLE);
+ if (intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
+ tmp |= AUD_CONFIG_N_VALUE_INDEX;
+ return tmp;
+}
+
+static uint32_t audio_config_setup_m_reg(struct intel_crtc *crtc,
+int m, uint32_t val)
+{
+ uint32_t tmp = val;
+
+ if (!intel_crtc_has_type(crtc->config, INTEL_OUTPUT_DP))
+ return 0;
+
+
[Intel-gfx] [PATCH v3] drm/vgem: Attach sw fences to exported vGEM dma-buf (ioctl)
vGEM buffers are useful for passing data between software clients and
hardware renders. By allowing the user to create and attach fences to
the exported vGEM buffers (on the dma-buf), the user can implement a
deferred renderer and queue hardware operations like flipping and then
signal the buffer readiness (i.e. this allows the user to schedule
operations out-of-order, but have them complete in-order).
This also makes it much easier to write tightly controlled testcases for
dma-buf fencing and signaling between hardware drivers.
v2: Don't pretend the fences exist in an ordered timeline, but allocate
a separate fence-context for each fence so that the fences are
unordered.
v3: Make the debug output more interesting, and so the signaled status.
Testcase: igt/vgem_basic/dmabuf-fence
Signed-off-by: Chris Wilson
Cc: Sean Paul
Cc: Zach Reizner
Cc: Gustavo Padovan
Cc: Daniel Vetter
Acked-by: Zach Reizner
---
drivers/gpu/drm/vgem/Makefile | 2 +-
drivers/gpu/drm/vgem/vgem_drv.c | 34 +++
drivers/gpu/drm/vgem/vgem_drv.h | 16 +++
drivers/gpu/drm/vgem/vgem_fence.c | 207 ++
include/uapi/drm/vgem_drm.h | 62
5 files changed, 320 insertions(+), 1 deletion(-)
create mode 100644 drivers/gpu/drm/vgem/vgem_fence.c
create mode 100644 include/uapi/drm/vgem_drm.h
diff --git a/drivers/gpu/drm/vgem/Makefile b/drivers/gpu/drm/vgem/Makefile
index 3f4c7b842028..bfcdea1330e6 100644
--- a/drivers/gpu/drm/vgem/Makefile
+++ b/drivers/gpu/drm/vgem/Makefile
@@ -1,4 +1,4 @@
ccflags-y := -Iinclude/drm
-vgem-y := vgem_drv.o
+vgem-y := vgem_drv.o vgem_fence.o
obj-$(CONFIG_DRM_VGEM) += vgem.o
diff --git a/drivers/gpu/drm/vgem/vgem_drv.c b/drivers/gpu/drm/vgem/vgem_drv.c
index 29c2aab3c1a7..c15bafb06665 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.c
+++ b/drivers/gpu/drm/vgem/vgem_drv.c
@@ -83,6 +83,34 @@ static const struct vm_operations_struct vgem_gem_vm_ops = {
.close = drm_gem_vm_close,
};
+static int vgem_open(struct drm_device *dev, struct drm_file *file)
+{
+ struct vgem_file *vfile;
+ int ret;
+
+ vfile = kzalloc(sizeof(*vfile), GFP_KERNEL);
+ if (!vfile)
+ return -ENOMEM;
+
+ file->driver_priv = vfile;
+
+ ret = vgem_fence_open(vfile);
+ if (ret) {
+ kfree(vfile);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void vgem_preclose(struct drm_device *dev, struct drm_file *file)
+{
+ struct vgem_file *vfile = file->driver_priv;
+
+ vgem_fence_close(vfile);
+ kfree(vfile);
+}
+
/* ioctls */
static struct drm_gem_object *vgem_gem_create(struct drm_device *dev,
@@ -164,6 +192,8 @@ unref:
}
static struct drm_ioctl_desc vgem_ioctls[] = {
+ DRM_IOCTL_DEF_DRV(VGEM_FENCE_ATTACH, vgem_fence_attach_ioctl,
DRM_AUTH|DRM_RENDER_ALLOW),
+ DRM_IOCTL_DEF_DRV(VGEM_FENCE_SIGNAL, vgem_fence_signal_ioctl,
DRM_AUTH|DRM_RENDER_ALLOW),
};
static int vgem_mmap(struct file *filp, struct vm_area_struct *vma)
@@ -271,9 +301,12 @@ static int vgem_prime_mmap(struct drm_gem_object *obj,
static struct drm_driver vgem_driver = {
.driver_features= DRIVER_GEM | DRIVER_PRIME,
+ .open = vgem_open,
+ .preclose = vgem_preclose,
.gem_free_object_unlocked = vgem_gem_free_object,
.gem_vm_ops = &vgem_gem_vm_ops,
.ioctls = vgem_ioctls,
+ .num_ioctls = ARRAY_SIZE(vgem_ioctls),
.fops = &vgem_driver_fops,
.dumb_create= vgem_gem_dumb_create,
@@ -328,5 +361,6 @@ module_init(vgem_init);
module_exit(vgem_exit);
MODULE_AUTHOR("Red Hat, Inc.");
+MODULE_AUTHOR("Intel Corporation");
MODULE_DESCRIPTION(DRIVER_DESC);
MODULE_LICENSE("GPL and additional rights");
diff --git a/drivers/gpu/drm/vgem/vgem_drv.h b/drivers/gpu/drm/vgem/vgem_drv.h
index 988cbaae7588..1f8798ad329c 100644
--- a/drivers/gpu/drm/vgem/vgem_drv.h
+++ b/drivers/gpu/drm/vgem/vgem_drv.h
@@ -32,9 +32,25 @@
#include
#include
+#include
+
+struct vgem_file {
+ struct idr fence_idr;
+ struct mutex fence_mutex;
+};
+
#define to_vgem_bo(x) container_of(x, struct drm_vgem_gem_object, base)
struct drm_vgem_gem_object {
struct drm_gem_object base;
};
+int vgem_fence_open(struct vgem_file *file);
+int vgem_fence_attach_ioctl(struct drm_device *dev,
+ void *data,
+ struct drm_file *file);
+int vgem_fence_signal_ioctl(struct drm_device *dev,
+ void *data,
+ struct drm_file *file);
+void vgem_fence_close(struct vgem_file *file);
+
#endif
diff --git a/drivers/gpu/drm/vgem/vgem_fence.c
b/drivers/gpu/drm/vgem/vgem_fence.c
new file mode 100644
index ..b7da11419ad6
--- /dev/null
+++ b/drivers/gpu/drm/vgem/vgem_fe
