Re: [Intel-gfx] [PATCH 3/4] drm/qxl: Drop fbdev hwaccel flags
On Thu, Jul 06, 2017 at 02:57:34PM +0200, Daniel Vetter wrote: > It's not accelarated, just system memory. Note we don't even need to > set the default flag since that's now always 0. > > Cc: Dave Airlie > Cc: Gerd Hoffmann > Cc: virtualizat...@lists.linux-foundation.org > Signed-off-by: Daniel Vetter Merged with Dave's irc-ack + commit message amended that qxl once had accel, but that was removed in 2015. -Daniel > --- > drivers/gpu/drm/qxl/qxl_fb.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c > index 573e7e9a5f98..69e7359b562a 100644 > --- a/drivers/gpu/drm/qxl/qxl_fb.c > +++ b/drivers/gpu/drm/qxl/qxl_fb.c > @@ -275,7 +275,7 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev, > > drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->depth); > > - info->flags = FBINFO_DEFAULT | FBINFO_HWACCEL_COPYAREA | > FBINFO_HWACCEL_FILLRECT; > + info->flags = FBINFO_DEFAULT; > info->fbops = &qxlfb_ops; > > /* > -- > 2.13.2 > -- 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 02/12] drm/atomic: Change drm_atomic_helper_swap_state to return an error.
We want to change swap_state to wait indefinitely, but to do this swap_state should wait interruptibly. This requires propagating the error to each driver. Cc: dri-de...@lists.freedesktop.org Cc: linux-ker...@vger.kernel.org Cc: intel-gfx@lists.freedesktop.org Signed-off-by: Maarten Lankhorst Link: http://patchwork.freedesktop.org/patch/msgid/20170711143314.2148-3-maarten.lankho...@linux.intel.com Reviewed-by: Daniel Vetter [mlankhorst: Fix typos in swap_state documentation (seanpaul)] Reviewed-by: Sean Paul ] --- drivers/gpu/drm/drm_atomic_helper.c | 26 ++ include/drm/drm_atomic_helper.h | 3 +-- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 2f675112e225..8d8221a128d0 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1510,10 +1510,8 @@ int drm_atomic_helper_commit(struct drm_device *dev, if (!nonblock) { ret = drm_atomic_helper_wait_for_fences(dev, state, true); - if (ret) { - drm_atomic_helper_cleanup_planes(dev, state); - return ret; - } + if (ret) + goto err; } /* @@ -1522,7 +1520,9 @@ int drm_atomic_helper_commit(struct drm_device *dev, * the software side now. */ - drm_atomic_helper_swap_state(state, true); + ret = drm_atomic_helper_swap_state(state, true); + if (ret) + goto err; /* * Everything below can be run asynchronously without the need to grab @@ -1551,6 +1551,10 @@ int drm_atomic_helper_commit(struct drm_device *dev, commit_tail(state); return 0; + +err: + drm_atomic_helper_cleanup_planes(dev, state); + return ret; } EXPORT_SYMBOL(drm_atomic_helper_commit); @@ -2228,14 +2232,14 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes); /** * drm_atomic_helper_swap_state - store atomic state into current sw state * @state: atomic state - * @stall: stall for proceeding commits + * @stall: stall for preceeding commits * * This function stores the atomic state into the current state pointers in all * driver objects. It should be called after all failing steps have been done * and succeeded, but before the actual hardware state is committed. * * For cleanup and error recovery the current state for all changed objects will - * be swaped into @state. + * be swapped into @state. * * With that sequence it fits perfectly into the plane prepare/cleanup sequence: * @@ -2254,8 +2258,12 @@ EXPORT_SYMBOL(drm_atomic_helper_cleanup_planes); * the &drm_plane.state, &drm_crtc.state or &drm_connector.state pointer. With * the current atomic helpers this is almost always the case, since the helpers * don't pass the right state structures to the callbacks. + * + * Returns: + * + * Always returns 0, cannot fail yet. */ -void drm_atomic_helper_swap_state(struct drm_atomic_state *state, +int drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool stall) { int i; @@ -2339,6 +2347,8 @@ void drm_atomic_helper_swap_state(struct drm_atomic_state *state, state->private_objs[i].state = old_obj_state; obj->state = new_obj_state; } + + return 0; } EXPORT_SYMBOL(drm_atomic_helper_swap_state); diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index 7db3438ff735..547480692470 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -86,8 +86,7 @@ void drm_atomic_helper_disable_planes_on_crtc(struct drm_crtc_state *old_crtc_state, bool atomic); -void drm_atomic_helper_swap_state(struct drm_atomic_state *state, - bool stall); +int drm_atomic_helper_swap_state(struct drm_atomic_state *state, bool stall); /* nonblocking commit helpers */ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, -- 2.11.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] CONTRIBUTING: formalize review rules
On Tue, Jul 18, 2017 at 06:00:20PM +0200, Daniel Vetter wrote: > There's a bunch of reasons why I think we should formalize and enforce > our review rules for igt patches: > > - We have a lot of new engineers joining and review helps enormously > with mentoring and learning. But right now only patches from > contributors without commit rights are consistently subjected to > review, which makes this imbalanced and removes senior contributors > from the review pool. > > - We have a much bigger team and we need to make sure we're aligned on > where igt as a tool and testsuite needs to head towards. Getting > that alignment happens through reviewing each other's submission. > Pushing a contentious patch and then dealing with a heated irc > discussion is much less effective. > > - Finally igt becomes ever more important for our testing, making sure > the code quality is high is important. Review helps with that. > > v2: Improve wording a bit (Imre). > > Acked-by: Daniel Stone > Acked-by: Jani Nikula > Acked-by: Joonas Lahtinen > Acked-by: Maarten Lankhorst > Acked-by: Petri Latvala > Acked-by: Imre Deak > Acked-by: Robert Foss > Acked-by: Ben Widawsky > Acked-by: Tvrtko Ursulin > Acked-by: Mika Kuoppala > Signed-off-by: Daniel Vetter Acked-by: Arkadiusz Hiler With the recent growth in contributions it's a good thing :-) Thanks! -- Cheers, Arek ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Fix the port submission race during engine reset in execlists mode
During the engine reset, there is a race condition which can make the request submitted to HW twice. This is due to the irq tasklet function enabled too earliy which is just before init_hw callback. This patch will move the irq tasklet enabling after init_hw to resolve this race. Fixes: a1ef70e14453 ("drm/i915: Add support for per engine reset recovery") Cc: Michel Thierry Signed-off-by: Chuanxiao Dong --- drivers/gpu/drm/i915/i915_drv.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index d310d82..e2f0222 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1963,9 +1963,6 @@ int i915_reset_engine(struct intel_engine_cs *engine) /* Finally, reset just this engine. */ ret = intel_gpu_reset(engine->i915, intel_engine_flag(engine)); - - i915_gem_reset_finish_engine(engine); - if (ret) { /* If we fail here, we expect to fallback to a global reset */ DRM_DEBUG_DRIVER("Failed to reset %s, ret=%d\n", @@ -1984,6 +1981,8 @@ int i915_reset_engine(struct intel_engine_cs *engine) error->reset_engine_count[engine->id]++; out: + i915_gem_reset_finish_engine(engine); + return ret; } -- 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 0/2] Handle unsupported configuration with IF-ID
Mahesh Kumar schreef op di 18-07-2017 om 18:12 [+0530]: > Hi Daniel, > > On Monday 17 July 2017 12:56 PM, Daniel Vetter wrote: > > On Fri, Jun 30, 2017 at 05:40:58PM +0530, Mahesh Kumar wrote: > > > Gen9+ Interlace fetch mode doesn't support few plane > > > configurations & pipe scaling. > > > - Y-tile > > > - 90/270 rotation > > > - pipe/plane scaling > > > - 420 planar formats > > > > Do we have igt testcases that try to exercise all this? When fixing > > bugs, > > pls make sure you don't fix the bugs, but also make sure we have > > solid > > coverage. Given that this escaped for years, it's very likely our > > coverage > > is _really_ bad and needs to be improve a lot for testing > > interlaced modes > > ... > We have testdisplay with -y option to test Y-tiling (90/270 rotation > need Y-tiling so those are also covered). > But AFAIK we don't have any testcase to verify scaling with Interlace > mode & other combinations. > Should we extend existing IGT test for scaling/rotation/tiling/pixel- > format to include Interlace mode as well, or should we have a > separate Interlace mode specific IGT > which will exercise all combinations with all Interlace modes? We need separate tests. testdisplay is nice for testing, but it's not automated. Ideally something that runs on all supported platforms, regardless of having an actual interlaced display connected. We do allow mode override with igt_output_override_mode, which could be used for this. > As IF-ID mode doesn't support all the above combination but PF-ID > mode does support them from GEN9(scaling Y-tile 90/270 rotation etc). > So I submitted a patch to always enable PF-ID mode for Interlace > https://patchwork.freedesktop.org/patch/160275/ > But Ville suggested not to always enable PF-ID mode instead control > fetching mode based on property. > Currently there is no open source user for this property. What will > you suggest here? > > Thanks, > -Mahesh > > Thanks, Daniel > > > > > Changes since V2: > > > - Address review comments from ville > > > > > > Mahesh Kumar (2): > > > drm/i915/skl+: Check for supported plane configuration in > > > Interlace > > > mode > > > drm/i915/skl+: Scaling not supported in IF-ID Interlace mode > > > > > > drivers/gpu/drm/i915/intel_atomic_plane.c | 15 +++ > > > drivers/gpu/drm/i915/intel_display.c | 15 +++ > > > 2 files changed, 30 insertions(+) > > > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t 1/2] tests/chamelium: Skip suspend/resume test with unreliable hotplug event
On Tue, 2017-07-18 at 22:21 +0100, Chris Wilson wrote: > Quoting Paul Kocialkowski (2017-07-18 16:16:26) > > It may occur that a hotplug uevent is detected at resume, even > > though it > > does not indicate that an actual hotplug happened. This is the case > > when > > link training fails on any other connector. > > > > There is currently no way to distinguish what connector caused a > > hotplug > > uevent, nor what the reason for that uevent really is. This makes it > > impossible to find out whether the test actually passed or not. > > And you may get more than one and then this skips even though the test > passed. Looks like the patch is overcompensating. What you can do is > repeat the test a few times, and then look at all the different errors > you get. If the connector remains (no mst disappareance) once it goes > bad, it should remain bad and so not generate any new uevent. Or you > only repeat the test whilst link_status[old] != link_status[new]. I am not sure it is really desirable to repeat the test until we are fairly certain it succeeds. This involves suspend/resume, that is already long enough as it is. Also, a uevent will be generated everytime link training fails, regardless of whether it was already failing before (I just tested that to make sure). In my case, it's due to a DP-VGA bridge that will consistently fail link training in the first seconds after resume. So this is actually even worse that I thought, because there is no way to find out that this is why a uevent was generated if the link status was already bad before. So I don't see how we can manage with the current information at disposal. My main point here is that we need more information about what's going on than simply "HOTPLUG=1". These patches demonstrate that working around the lack of information is a pain for testing purposes and can only leads to semi-working hackish workarounds. Do you agree that this is what the problem really is? -- Paul Kocialkowski Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Explicit the connector name for DP link training result
On Tue, 2017-07-18 at 12:20 -0700, Manasi Navare wrote: > On Tue, Jul 18, 2017 at 05:25:36PM +0300, Paul Kocialkowski wrote: > > This adds the connector name when printing a debug message about the > > DP > > link training result. It is useful to figure out what connector is > > failing when multiple DP connectors are used. > > > > Thanks for the patch, this does make sense during the link training > failure debugging to know the connector name. > While at it feel free to change "Failed", "Link Rate, Lane Count", to > upper case > in the failure_handling case to be consistent with the pass case. Thanks for the review! Now that the patch was merged, do you feel like I should make a follow- up patch to fix consistency in the upper case use or is it enough of a detail that we can just forget about it? > Reviewed-by: Manasi Navare > > Manasi > > > Signed-off-by: Paul Kocialkowski > > --- > > drivers/gpu/drm/i915/intel_dp_link_training.c | 8 ++-- > > 1 file changed, 6 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c > > b/drivers/gpu/drm/i915/intel_dp_link_training.c > > index b79c1c0e404c..05907fa8a553 100644 > > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c > > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c > > @@ -321,12 +321,16 @@ intel_dp_start_link_train(struct intel_dp > > *intel_dp) > > if (!intel_dp_link_training_channel_equalization(intel_dp)) > > goto failure_handling; > > > > - DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane > > count = %d", > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training Passed at > > Link Rate = %d, Lane count = %d", > > + intel_connector->base.base.id, > > + intel_connector->base.name, > > intel_dp->link_rate, intel_dp->lane_count); > > return; > > > > failure_handling: > > - DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane > > count = %d", > > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at > > link rate = %d, lane count = %d", > > + intel_connector->base.base.id, > > + intel_connector->base.name, > > intel_dp->link_rate, intel_dp->lane_count); > > if (!intel_dp_get_link_train_fallback_values(intel_dp, > > intel_dp- > > >link_rate, > > -- > > 2.13.2 > > -- Paul Kocialkowski Intel Finland Oy - BIC 0357606-4 - Westendinkatu 7, 02160 Espoo, Finland ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Explicit the connector name for DP link training result
On Wed, 19 Jul 2017, Paul Kocialkowski wrote: > On Tue, 2017-07-18 at 12:20 -0700, Manasi Navare wrote: >> On Tue, Jul 18, 2017 at 05:25:36PM +0300, Paul Kocialkowski wrote: >> > This adds the connector name when printing a debug message about the >> > DP >> > link training result. It is useful to figure out what connector is >> > failing when multiple DP connectors are used. >> > >> >> Thanks for the patch, this does make sense during the link training >> failure debugging to know the connector name. >> While at it feel free to change "Failed", "Link Rate, Lane Count", to >> upper case >> in the failure_handling case to be consistent with the pass case. > > Thanks for the review! > > Now that the patch was merged, do you feel like I should make a follow- > up patch to fix consistency in the upper case use or is it enough of a > detail that we can just forget about it? If it bothers you, send a patch, otherwise wait for it to bother someone else enough to send a patch. ;) But please avoid the overuse of all caps for regular words, or capitalization except at the beginning of sentences. I'd go with something like this all over the place: "Link training failed, link rate %d, lane count %d\n" But it hasn't bothered me enough to send a patch. ;) BR, Jani. > >> Reviewed-by: Manasi Navare >> >> Manasi >> >> > Signed-off-by: Paul Kocialkowski >> > --- >> > drivers/gpu/drm/i915/intel_dp_link_training.c | 8 ++-- >> > 1 file changed, 6 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c >> > b/drivers/gpu/drm/i915/intel_dp_link_training.c >> > index b79c1c0e404c..05907fa8a553 100644 >> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c >> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c >> > @@ -321,12 +321,16 @@ intel_dp_start_link_train(struct intel_dp >> > *intel_dp) >> >if (!intel_dp_link_training_channel_equalization(intel_dp)) >> >goto failure_handling; >> > >> > - DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane >> > count = %d", >> > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training Passed at >> > Link Rate = %d, Lane count = %d", >> > +intel_connector->base.base.id, >> > +intel_connector->base.name, >> > intel_dp->link_rate, intel_dp->lane_count); >> >return; >> > >> > failure_handling: >> > - DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane >> > count = %d", >> > + DRM_DEBUG_KMS("[CONNECTOR:%d:%s] Link Training failed at >> > link rate = %d, lane count = %d", >> > +intel_connector->base.base.id, >> > +intel_connector->base.name, >> > intel_dp->link_rate, intel_dp->lane_count); >> >if (!intel_dp_get_link_train_fallback_values(intel_dp, >> > intel_dp- >> > >link_rate, >> > -- >> > 2.13.2 >> > -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Fix the port submission race during engine reset in execlists mode
== Series Details == Series: drm/i915: Fix the port submission race during engine reset in execlists mode URL : https://patchwork.freedesktop.org/series/27554/ State : success == Summary == Series 27554v1 drm/i915: Fix the port submission race during engine reset in execlists mode https://patchwork.freedesktop.org/api/1.0/series/27554/revisions/1/mbox/ Test gem_exec_flush: Subgroup basic-batch-kernel-default-uc: pass -> FAIL (fi-snb-2600) fdo#17 Test kms_flip: Subgroup basic-flip-vs-modeset: skip -> PASS (fi-skl-x1585l) fdo#101781 Test kms_pipe_crc_basic: Subgroup hang-read-crc-pipe-a: dmesg-warn -> PASS (fi-pnv-d510) fdo#101597 fdo#17 https://bugs.freedesktop.org/show_bug.cgi?id=17 fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781 fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597 fi-bdw-5557u total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:441s fi-bdw-gvtdvmtotal:279 pass:265 dwarn:0 dfail:0 fail:0 skip:14 time:435s fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:357s fi-bsw-n3050 total:279 pass:243 dwarn:0 dfail:0 fail:0 skip:36 time:537s fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:511s fi-byt-j1900 total:279 pass:254 dwarn:1 dfail:0 fail:0 skip:24 time:497s fi-byt-n2820 total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:486s fi-glk-2atotal:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:602s fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:439s fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:415s fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:421s fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:501s fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:468s fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:467s fi-kbl-7560u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:572s fi-kbl-r total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:586s fi-pnv-d510 total:279 pass:222 dwarn:2 dfail:0 fail:0 skip:55 time:562s fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:461s fi-skl-6700hqtotal:279 pass:262 dwarn:0 dfail:0 fail:0 skip:17 time:589s fi-skl-6700k total:279 pass:257 dwarn:4 dfail:0 fail:0 skip:18 time:469s fi-skl-6770hqtotal:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:486s fi-skl-gvtdvmtotal:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:443s fi-skl-x1585ltotal:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:487s fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:546s fi-snb-2600 total:279 pass:249 dwarn:0 dfail:0 fail:1 skip:29 time:407s 62bcb2c39617a50d33d30fc565a401f8e042fdfd drm-tip: 2017y-07m-19d-07h-42m-52s UTC integration manifest 43021f2 drm/i915: Fix the port submission race during engine reset in execlists mode == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5230/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 0/2] Handle unsupported configuration with IF-ID
On Wed, Jul 19, 2017 at 08:22:03AM +, Lankhorst, Maarten wrote: > Mahesh Kumar schreef op di 18-07-2017 om 18:12 [+0530]: > > Hi Daniel, > > > > On Monday 17 July 2017 12:56 PM, Daniel Vetter wrote: > > > On Fri, Jun 30, 2017 at 05:40:58PM +0530, Mahesh Kumar wrote: > > > > Gen9+ Interlace fetch mode doesn't support few plane > > > > configurations & pipe scaling. > > > > - Y-tile > > > > - 90/270 rotation > > > > - pipe/plane scaling > > > > - 420 planar formats > > > > > > Do we have igt testcases that try to exercise all this? When fixing > > > bugs, > > > pls make sure you don't fix the bugs, but also make sure we have > > > solid > > > coverage. Given that this escaped for years, it's very likely our > > > coverage > > > is _really_ bad and needs to be improve a lot for testing > > > interlaced modes > > > ... > > We have testdisplay with -y option to test Y-tiling (90/270 rotation > > need Y-tiling so those are also covered). > > But AFAIK we don't have any testcase to verify scaling with Interlace > > mode & other combinations. > > Should we extend existing IGT test for scaling/rotation/tiling/pixel- > > format to include Interlace mode as well, or should we have a > > separate Interlace mode specific IGT > > which will exercise all combinations with all Interlace modes? > > We need separate tests. testdisplay is nice for testing, but it's not > automated. Ideally something that runs on all supported platforms, > regardless of having an actual interlaced display connected. We do > allow mode override with igt_output_override_mode, which could be used > for this. +1, testdisplay is not fully automated validation. We should probably move it over to tools, really doesn't belong into the igt testcase folder. -Daniel > > > As IF-ID mode doesn't support all the above combination but PF-ID > > mode does support them from GEN9(scaling Y-tile 90/270 rotation etc). > > So I submitted a patch to always enable PF-ID mode for Interlace > > https://patchwork.freedesktop.org/patch/160275/ > > But Ville suggested not to always enable PF-ID mode instead control > > fetching mode based on property. > > Currently there is no open source user for this property. What will > > you suggest here? > > > > Thanks, > > -Mahesh > > > Thanks, Daniel > > > > > > > Changes since V2: > > > > - Address review comments from ville > > > > > > > > Mahesh Kumar (2): > > > > drm/i915/skl+: Check for supported plane configuration in > > > > Interlace > > > > mode > > > > drm/i915/skl+: Scaling not supported in IF-ID Interlace mode > > > > > > > > drivers/gpu/drm/i915/intel_atomic_plane.c | 15 +++ > > > > drivers/gpu/drm/i915/intel_display.c | 15 +++ > > > > 2 files changed, 30 insertions(+) > > > > -- 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] [RFC 11/14] drm/i915: Engine busy time tracking
On 18/07/2017 16:19, Chris Wilson wrote: Quoting Tvrtko Ursulin (2017-07-18 15:36:15) From: Tvrtko Ursulin Track total time requests have been executing on the hardware. To make this cheap it is hidden behind a static branch with the intention that it is only enabled when there is a consumer listening. This means that in the default off case the total cost of the tracking is just a few no-op instructions on the fast paths. +static inline void intel_engine_context_in(struct intel_engine_cs *engine) +{ + if (static_branch_unlikely(&i915_engine_stats_key)) { + unsigned long flags; + + spin_lock_irqsave(&engine->stats.lock, flags); What's the purpose of this lock? RMW is ordered by virtue of the tasklet (only one cpu can be doing the rmw at any time). Did I miss another user? Hm it should be a plain spin_lock and a _bh variant on the external API. But the purpose is to allow atomic sampling of accumulated busy time plus the current engine status. + if (engine->stats.ref++ == 0) + engine->stats.start = ktime_get_real_ns(); Use ktime_get_raw() and leave the conversion to ns to the caller. You mean both store in ktime_t and don't fetch the wall time but monotonic? Do you say this because perf pmu perhaps needs monotonic or for some other reason? What is the cost of a ktime nowadays? I don't see it in the profiles, so not much I think. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/16] drm/msm: Convert to use new iterator macros, v2.
On 07/12/2017 04:15 PM, Maarten Lankhorst wrote: Op 12-07-17 om 11:48 schreef Daniel Vetter: On Wed, Jul 12, 2017 at 10:13:42AM +0200, Maarten Lankhorst wrote: for_each_obj_in_state is about to be removed, so convert to the new iterator macros. Just like in omap, use crtc_state->active instead of crtc_state->enable when waiting for completion. Tested-by: Archit Taneja Signed-off-by: Maarten Lankhorst Cc: Rob Clark Cc: Archit Taneja Cc: Vincent Abriou Cc: Maarten Lankhorst Cc: Russell King Cc: Rob Herring Cc: Markus Elfring Cc: Sushmita Susheelendra Cc: linux-arm-...@vger.kernel.org Cc: freedr...@lists.freedesktop.org --- drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 4 ++-- drivers/gpu/drm/msm/msm_atomic.c| 16 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c index bcd1f5cac72c..f7f087419ed8 100644 --- a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c +++ b/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c @@ -114,7 +114,7 @@ static void mdp4_prepare_commit(struct msm_kms *kms, struct drm_atomic_state *st mdp4_enable(mdp4_kms); /* see 119ecb7fd */ - for_each_crtc_in_state(state, crtc, crtc_state, i) + for_each_new_crtc_in_state(state, crtc, crtc_state, i) drm_crtc_vblank_get(crtc); } @@ -126,7 +126,7 @@ static void mdp4_complete_commit(struct msm_kms *kms, struct drm_atomic_state *s struct drm_crtc_state *crtc_state; /* see 119ecb7fd */ - for_each_crtc_in_state(state, crtc, crtc_state, i) + for_each_new_crtc_in_state(state, crtc, crtc_state, i) drm_crtc_vblank_put(crtc); mdp4_disable(mdp4_kms); diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c index 9633a68b14d7..9d3cc1f5e31a 100644 --- a/drivers/gpu/drm/msm/msm_atomic.c +++ b/drivers/gpu/drm/msm/msm_atomic.c @@ -84,13 +84,13 @@ static void msm_atomic_wait_for_commit_done(struct drm_device *dev, struct drm_atomic_state *old_state) { struct drm_crtc *crtc; - struct drm_crtc_state *crtc_state; + struct drm_crtc_state *new_crtc_state; struct msm_drm_private *priv = old_state->dev->dev_private; struct msm_kms *kms = priv->kms; int i; - for_each_crtc_in_state(old_state, crtc, crtc_state, i) { - if (!crtc->state->enable) + for_each_new_crtc_in_state(old_state, crtc, new_crtc_state, i) { + if (!new_crtc_state->active) continue; kms->funcs->wait_for_crtc_commit_done(kms, crtc); @@ -195,7 +195,7 @@ int msm_atomic_commit(struct drm_device *dev, struct drm_crtc *crtc; struct drm_crtc_state *crtc_state; struct drm_plane *plane; - struct drm_plane_state *plane_state; + struct drm_plane_state *old_plane_state, *new_plane_state; int i, ret; ret = drm_atomic_helper_prepare_planes(dev, state); @@ -211,15 +211,15 @@ int msm_atomic_commit(struct drm_device *dev, /* * Figure out what crtcs we have: */ - for_each_crtc_in_state(state, crtc, crtc_state, i) + for_each_new_crtc_in_state(state, crtc, crtc_state, i) c->crtc_mask |= drm_crtc_mask(crtc); /* * Figure out what fence to wait for: */ - for_each_plane_in_state(state, plane, plane_state, i) { - if ((plane->state->fb != plane_state->fb) && plane_state->fb) { - struct drm_gem_object *obj = msm_framebuffer_bo(plane_state->fb, 0); + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { + if ((new_plane_state->fb != old_plane_state->fb) && new_plane_state->fb) { + struct drm_gem_object *obj = msm_framebuffer_bo(new_plane_state->fb, 0); struct msm_gem_object *msm_obj = to_msm_bo(obj); struct dma_fence *fence = reservation_object_get_excl_rcu(msm_obj->resv); Pretty sure this fails to compile, you've forgotten to do one more s/plane_state/new_plane_state/. With that fixed: Reviewed-by: Daniel Vetter --->8--- for_each_obj_in_state is about to be removed, so convert to the new iterator macros. Just like in omap, use crtc_state->active instead of crtc_state->enable when waiting for completion. Changes since v1: - Fix compilation. Signed-off-by: Maarten Lankhorst Cc: Rob Clark Cc: Archit Taneja Cc: Vincent Abriou Cc: Maarten Lankhorst Cc: Russell King Cc: Rob Herring Cc: Markus Elfring Cc: Sushmita Susheelendra Cc: linux-arm-...@vger.kernel.org Cc: freedr...@lists.freedesktop.org Reviewed-by: Daniel Vetter --- drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c | 4 ++-- drivers/gpu/drm/msm/msm_atomic.c| 18 +- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/msm/mdp/mdp4/mdp4_kms.c b/driver
Re: [Intel-gfx] [PATCH] drm/i915: Fix the port submission race during engine reset in execlists mode
Quoting Chuanxiao Dong (2017-07-19 09:14:14) > During the engine reset, there is a race condition which can make the > request submitted to HW twice. This is due to the irq tasklet function > enabled too earliy which is just before init_hw callback. This patch > will move the irq tasklet enabling after init_hw to resolve this race. This is not the only bug here. Please have a look at the series https://patchwork.freedesktop.org/series/27385/ -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 12/14] drm/i915: Interface for controling engine stats collection
On 18/07/2017 16:22, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2017-07-18 15:36:16) >> +u64 intel_engine_get_current_busy_ns(struct intel_engine_cs *engine) >> +{ >> + unsigned long flags; >> + u64 total; >> + >> + spin_lock_irqsave(&engine->stats.lock, flags); >> + >> + total = engine->stats.total; >> + >> + /* >> +* If the engine is executing something at the moment >> +* add it to the total. >> +*/ >> + if (engine->stats.ref) >> + total += ktime_get_real_ns() - engine->stats.start; >> + >> + spin_unlock_irqrestore(&engine->stats.lock, flags); > > Answers to another patch found here. I would say this is the other half > of the interface and should be kept together. Yes, it was an ugly split. On 18/07/2017 16:43, Chris Wilson wrote: > Quoting Tvrtko Ursulin (2017-07-18 15:36:16) >> +int intel_enable_engine_stats(struct drm_i915_private *dev_priv) >> +{ >> + if (!i915.enable_execlists) >> + return -ENODEV; >> + >> + mutex_lock(&i915_engine_stats_mutex); >> + if (i915_engine_stats_ref++ == 0) { >> + struct intel_engine_cs *engine; >> + enum intel_engine_id id; >> + >> + for_each_engine(engine, dev_priv, id) { >> + memset(&engine->stats, 0, sizeof(engine->stats)); >> + spin_lock_init(&engine->stats.lock); >> + } >> + >> + static_branch_enable(&i915_engine_stats_key); >> + } >> + mutex_unlock(&i915_engine_stats_mutex); > > I don't think static_branch_enable() is a might_sleep, so it looks like > you can rewrite this avoiding the mutex and thus not requiring the > worker and then can use the error code here to decide if you need to > use the timer instead. Perhaps I could get rid of the mutex though by using atomic_inc/dec_return. But there is a mutex in jump label handling, so I think the workers have to stay - and it is also beneficial to have delayed static branch disable, since the perf core seems to like calling start/stop on the events a lot. But it is recommended practice for static branches in any way. So from that angle I could perhaps even move the delayed disable to this patch so it is automatically shared by all callers. >> +static DEFINE_MUTEX(i915_engine_stats_mutex); >> +static int i915_engine_stats_ref; >> >> /* Haswell does have the CXT_SIZE register however it does not appear to be >>* valid. Now, docs explain in dwords what is in the context object. The >> full >> @@ -1340,6 +1342,57 @@ void intel_engines_mark_idle(struct drm_i915_private >> *i915) >> } >> } >> >> +int intel_enable_engine_stats(struct drm_i915_private *dev_priv) > > The pattern I've been trying to use here is > > intel_engine_* - operate on the named engine > > intel_engines_* - operate on all engines Ok. > Long term though having a global static key is going to be a nasty wart. > Joonas will definitely ask the question how much will it cost us to use > an engine->bool and what we can do to minimise that cost. Why you think it is nasty? Sounds pretty cool to me. But I think can re-organize the series to start with a normal branch and then add the static one on top if so is desired. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 12/14] drm/i915: Interface for controling engine stats collection
Hi Ben, On 18/07/2017 15:36, Tvrtko Ursulin wrote: From: Tvrtko Ursulin Enables other i915 components to enable and disable the facility as needed. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/intel_engine_cs.c | 53 + drivers/gpu/drm/i915/intel_ringbuffer.h | 5 2 files changed, 58 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 3e5e08c6b5ef..03e7459bad06 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -29,6 +29,8 @@ #include "intel_lrc.h" DEFINE_STATIC_KEY_FALSE(i915_engine_stats_key); +static DEFINE_MUTEX(i915_engine_stats_mutex); +static int i915_engine_stats_ref; /* Haswell does have the CXT_SIZE register however it does not appear to be * valid. Now, docs explain in dwords what is in the context object. The full @@ -1340,6 +1342,57 @@ void intel_engines_mark_idle(struct drm_i915_private *i915) } } +int intel_enable_engine_stats(struct drm_i915_private *dev_priv) +{ + if (!i915.enable_execlists) + return -ENODEV; + + mutex_lock(&i915_engine_stats_mutex); + if (i915_engine_stats_ref++ == 0) { + struct intel_engine_cs *engine; + enum intel_engine_id id; + + for_each_engine(engine, dev_priv, id) { + memset(&engine->stats, 0, sizeof(engine->stats)); + spin_lock_init(&engine->stats.lock); + } + + static_branch_enable(&i915_engine_stats_key); + } + mutex_unlock(&i915_engine_stats_mutex); + + return 0; +} + +void intel_disable_engine_stats(void) +{ + mutex_lock(&i915_engine_stats_mutex); + if (--i915_engine_stats_ref == 0) + static_branch_disable(&i915_engine_stats_key); + mutex_unlock(&i915_engine_stats_mutex); +} + +u64 intel_engine_get_current_busy_ns(struct intel_engine_cs *engine) +{ + unsigned long flags; + u64 total; + + spin_lock_irqsave(&engine->stats.lock, flags); + + total = engine->stats.total; + + /* +* If the engine is executing something at the moment +* add it to the total. +*/ + if (engine->stats.ref) + total += ktime_get_real_ns() - engine->stats.start; + + spin_unlock_irqrestore(&engine->stats.lock, flags); + + return total; +} + #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST) #include "selftests/mock_engine.c" #endif diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h index 2eb1e970ad06..e0f495a6d0d9 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.h +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h @@ -776,4 +776,9 @@ static inline void intel_engine_context_out(struct intel_engine_cs *engine) } } +int intel_enable_engine_stats(struct drm_i915_private *i915); +void intel_disable_engine_stats(void); + +u64 intel_engine_get_current_busy_ns(struct intel_engine_cs *engine); If we exported these symbols for other modules to use, what kind of API would they need? Presumably not per-engine but something to give the aggregated busyness of all engines? Or I have misunderstood you that there is this requirement? Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] CONTRIBUTING: formalize review rules
On Tue, 18 Jul 2017, Daniel Vetter wrote: > On Tue, Jul 18, 2017 at 10:34 PM, Lionel Landwerlin > wrote: >> Acked-by: Lionel Landwerlin >> >> I assume review cannot be provided by someone who doesn't already contribute >> or has a number of patches in already. >> >> What's the criteria to become a reviewer? >> Is there is going to be a list of people to go to for review? > > Just going out and getting the first r-b tag from the lowest bidder > would indeed be a bit silly, but I don't see any issue with new > contributors (who might not yet have pushed anything) doing review. > > Imo review is about a lot more than just code correctess, it's also > really great tool for aligning a team on the ideas in a code, for > mentoring and learning and all that stuff. So someone new reviewing > code of someone experienced, and making the code and docs better by > asking questions, sounds pretty perfect to me. > > Ofc two completely new contributors reviewing each another's stuff > would again defeat that, but then they need at least someone slightly > more experienced as committer again. To amend that, we put trust in people who we give commit access to, and they need to assess whether the review has been sufficient. Rules to cover all cases would be so long nobody would read them. This change in review rules should steer the committers towards requiring more formal reviewed-by on the list before pushing than has so far been the case, and we expect the committers to follow. BR, Jani. > > tldr; totally not worried nor seeing a need for criteria for reviewers. > > Cheers, Daniel > >> - >> Lionel >> >> >> On 18/07/17 17:00, Daniel Vetter wrote: >>> >>> There's a bunch of reasons why I think we should formalize and enforce >>> our review rules for igt patches: >>> >>> - We have a lot of new engineers joining and review helps enormously >>>with mentoring and learning. But right now only patches from >>>contributors without commit rights are consistently subjected to >>>review, which makes this imbalanced and removes senior contributors >>>from the review pool. >>> >>> - We have a much bigger team and we need to make sure we're aligned on >>>where igt as a tool and testsuite needs to head towards. Getting >>>that alignment happens through reviewing each other's submission. >>>Pushing a contentious patch and then dealing with a heated irc >>>discussion is much less effective. >>> >>> - Finally igt becomes ever more important for our testing, making sure >>>the code quality is high is important. Review helps with that. >>> >>> v2: Improve wording a bit (Imre). >>> >>> Acked-by: Daniel Stone >>> Acked-by: Jani Nikula >>> Acked-by: Joonas Lahtinen >>> Acked-by: Maarten Lankhorst >>> Acked-by: Petri Latvala >>> Acked-by: Imre Deak >>> Acked-by: Robert Foss >>> Acked-by: Ben Widawsky >>> Acked-by: Tvrtko Ursulin >>> Acked-by: Mika Kuoppala >>> Signed-off-by: Daniel Vetter >>> --- >>> CONTRIBUTING | 9 + >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/CONTRIBUTING b/CONTRIBUTING >>> index d2adcf03b7c3..561c5dd80bba 100644 >>> --- a/CONTRIBUTING >>> +++ b/CONTRIBUTING >>> @@ -26,10 +26,11 @@ A short list of contribution guidelines: >>> convenience macros provided by the igt library. The semantic patch >>> lib/igt.cocci >>> can help with the more automatic conversions. >>> -- There is no formal review requirement and regular contributors with >>> commit >>> - access can push patches right after submitting them to the mailing >>> lists. But >>> - invasive changes, new helper libraries and contributions from newcomers >>> should >>> - go through a proper review to ensure overall consistency in the >>> codebase. >>> +- Patches need to be reviewed on the mailing list. Exceptions only apply >>> for >>> + testcases and tooling for drivers with just a single contributor (e.g. >>> vc4). >>> + In this case patches must still be submitted to the mailing list first. >>> + Testcase should preferrably be cross-reviewed by the same people who >>> write and >>> + review the kernel feature itself. >>> - When patches from new contributors (without commit access) are >>> stuck, for >>> anything related to the regular releases, issues with packaging and >> >> >> -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix the port submission race during engine reset in execlists mode
> -Original Message- > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk] > Sent: Wednesday, July 19, 2017 5:26 PM > To: Dong, Chuanxiao ; intel- > g...@lists.freedesktop.org > Cc: intel-gvt-...@lists.freedesktop.org > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Fix the port submission race during > engine reset in execlists mode > > Quoting Chuanxiao Dong (2017-07-19 09:14:14) > > During the engine reset, there is a race condition which can make the > > request submitted to HW twice. This is due to the irq tasklet function > > enabled too earliy which is just before init_hw callback. This patch > > will move the irq tasklet enabling after init_hw to resolve this race. > > This is not the only bug here. Please have a look at the series > https://patchwork.freedesktop.org/series/27385/ > -Chris Thanks Chris for the sharing. I should did a search first before sending this patch :) . And your fixes has already included this one. This patch can be dropped. Thanks Chuanxiao ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 01/14] RFC drm/i915: Expose a PMU interface for perf queries
Can we reuse calc_residency defined in i915_sysfs.c On 7/18/2017 8:06 PM, Tvrtko Ursulin wrote: From: Chris Wilson The first goal is to be able to measure GPU (and invidual ring) busyness without having to poll registers from userspace. (Which not only incurs holding the forcewake lock indefinitely, perturbing the system, but also runs the risk of hanging the machine.) As an alternative we can use the perf event counter interface to sample the ring registers periodically and send those results to userspace. To be able to do so, we need to export the two symbols from kernel/events/core.c to register and unregister a PMU device. v2: Use a common timer for the ring sampling. Signed-off-by: Chris Wilson --- drivers/gpu/drm/i915/Makefile | 1 + drivers/gpu/drm/i915/i915_drv.c | 2 + drivers/gpu/drm/i915/i915_drv.h | 23 ++ drivers/gpu/drm/i915/i915_pmu.c | 452 drivers/gpu/drm/i915/intel_ringbuffer.h | 2 + include/uapi/drm/i915_drm.h | 41 +++ kernel/events/core.c| 1 + 7 files changed, 522 insertions(+) create mode 100644 drivers/gpu/drm/i915/i915_pmu.c diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile index f8227318dcaf..1c720013dc42 100644 --- a/drivers/gpu/drm/i915/Makefile +++ b/drivers/gpu/drm/i915/Makefile @@ -26,6 +26,7 @@ i915-y := i915_drv.o \ i915-$(CONFIG_COMPAT) += i915_ioc32.o i915-$(CONFIG_DEBUG_FS) += i915_debugfs.o intel_pipe_crc.o +i915-$(CONFIG_PERF_EVENTS) += i915_pmu.o # GEM code i915-y += i915_cmd_parser.o \ diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index d310d8245dca..f18ce519f6a2 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1194,6 +1194,7 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) struct drm_device *dev = &dev_priv->drm; i915_gem_shrinker_init(dev_priv); + i915_pmu_register(dev_priv); /* * Notify a valid surface after modesetting, @@ -1247,6 +1248,7 @@ static void i915_driver_unregister(struct drm_i915_private *dev_priv) intel_opregion_unregister(dev_priv); i915_perf_unregister(dev_priv); + i915_pmu_unregister(dev_priv); i915_teardown_sysfs(dev_priv); i915_guc_log_unregister(dev_priv); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 7c6fab08a2e6..de518503e033 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -40,6 +40,7 @@ #include #include #include +#include #include #include #include @@ -2093,6 +2094,12 @@ struct intel_cdclk_state { unsigned int cdclk, vco, ref; }; +enum { + __I915_SAMPLE_FREQ_ACT = 0, + __I915_SAMPLE_FREQ_REQ, + __I915_NUM_PMU_SAMPLERS +}; + struct drm_i915_private { struct drm_device drm; @@ -2591,6 +2598,13 @@ struct drm_i915_private { int irq; } lpe_audio; + struct { + struct pmu base; + struct hrtimer timer; + u64 enable; + u64 sample[__I915_NUM_PMU_SAMPLERS]; + } pmu; + /* * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch * will be rejected. Instead look for a better place. @@ -3760,6 +3774,15 @@ extern void i915_perf_fini(struct drm_i915_private *dev_priv); extern void i915_perf_register(struct drm_i915_private *dev_priv); extern void i915_perf_unregister(struct drm_i915_private *dev_priv); +/* i915_pmu.c */ +#ifdef CONFIG_PERF_EVENTS +extern void i915_pmu_register(struct drm_i915_private *i915); +extern void i915_pmu_unregister(struct drm_i915_private *i915); +#else +static inline void i915_pmu_register(struct drm_i915_private *i915) {} +static inline void i915_pmu_unregister(struct drm_i915_private *i915) {} +#endif + /* i915_suspend.c */ extern int i915_save_state(struct drm_i915_private *dev_priv); extern int i915_restore_state(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c new file mode 100644 index ..f03ddad44da6 --- /dev/null +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -0,0 +1,452 @@ +#include +#include + +#include "i915_drv.h" +#include "intel_ringbuffer.h" + +#define FREQUENCY 200 +#define PERIOD max_t(u64, 1, NSEC_PER_SEC / FREQUENCY) + +#define RING_MASK 0x +#define RING_MAX 32 + +static void engines_sample(struct drm_i915_private *dev_priv) +{ + struct intel_engine_cs *engine; + enum intel_engine_id id; + bool fw = false; + + if ((dev_priv->pmu.enable & RING_MASK) == 0) + return; + + if (!dev_priv->gt.awake) + return; + + if (!intel_runtime_pm_get_if_in_use(dev_priv)) + return; + + for_each_engine(engine, dev_priv, id) { + u32
[Intel-gfx] [PATCH] drm/i915: More stolen quirking
I've found a bios with an off-by-one at the other end. There's a pnp reservation for 0xc540-0xc7fe and we want stolen in 0xc600 through 0xc800. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99872 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98683 Cc: Martin Peres Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_stolen.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index a817b3e0b17e..c11c915382e7 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -254,9 +254,10 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv) * This is a BIOS w/a: Some BIOS wrap stolen in the root * PCI bus, but have an off-by-one error. Hence retry the * reservation starting from 1 instead of 0. +* There's also BIOS with off-by-one on the other end. */ r = devm_request_mem_region(dev_priv->drm.dev, base + 1, - ggtt->stolen_size - 1, + ggtt->stolen_size - 2, "Graphics Stolen Memory"); /* * GEN3 firmware likes to smash pci bridges into the stolen -- 2.13.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: More stolen quirking
On 19/07/17 13:00, Daniel Vetter wrote: I've found a bios with an off-by-one at the other end. There's a pnp reservation for 0xc540-0xc7fe and we want stolen in 0xc600 through 0xc800. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99872 Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98683 Cc: Martin Peres Signed-off-by: Daniel Vetter Looks good, and it will reduce the noise in CI. Thanks! Reviewed-by: Martin Peres --- drivers/gpu/drm/i915/i915_gem_stolen.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c b/drivers/gpu/drm/i915/i915_gem_stolen.c index a817b3e0b17e..c11c915382e7 100644 --- a/drivers/gpu/drm/i915/i915_gem_stolen.c +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c @@ -254,9 +254,10 @@ static dma_addr_t i915_stolen_to_dma(struct drm_i915_private *dev_priv) * This is a BIOS w/a: Some BIOS wrap stolen in the root * PCI bus, but have an off-by-one error. Hence retry the * reservation starting from 1 instead of 0. +* There's also BIOS with off-by-one on the other end. */ r = devm_request_mem_region(dev_priv->drm.dev, base + 1, - ggtt->stolen_size - 1, + ggtt->stolen_size - 2, "Graphics Stolen Memory"); /* * GEN3 firmware likes to smash pci bridges into the stolen ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 00/12] drm/atomic: Make drm_atomic_helper_swap_state waiting interruptible.
Op 14-07-17 om 17:01 schreef Sean Paul: > On Tue, Jul 11, 2017 at 04:33:02PM +0200, Maarten Lankhorst wrote: >> drm_atomic_helper_swap_state could previously never fail, in order to still >> continue it would set a limit of 10s on waiting for previous updates to >> complete, >> then it moved forward. This can be very bad when ignoring previous updates, >> because the hw state and sw state may get out of sync when for example a >> modeset >> is ignored. >> >> By converting the swap_state to interruptible and handling the error in each >> driver, >> we fix this issue because if a update takes forever it can be aborted >> through signals. >> >> Changes since first version: >> - Split out driver conversions. >> - Fix nouveau error handling first. >> >> Maarten Lankhorst (12): >> drm/nouveau: Fix error handling in nv50_disp_atomic_commit >> drm/atomic: Change drm_atomic_helper_swap_state to return an error. >> drm/nouveau: Handle drm_atomic_helper_swap_state failure >> drm/atmel-hlcdc: Handle drm_atomic_helper_swap_state failure >> drm/i915: Handle drm_atomic_helper_swap_state failure >> drm/mediatek: Handle drm_atomic_helper_swap_state failure >> drm/msm: Handle drm_atomic_helper_swap_state failure >> drm/tegra: Handle drm_atomic_helper_swap_state failure >> drm/tilcdc: Handle drm_atomic_helper_swap_state failure >> drm/vc4: Handle drm_atomic_helper_swap_state failure >> drm/atomic: Add __must_check to drm_atomic_helper_swap_state. >> drm/atomic: Allow drm_atomic_helper_swap_state to fail > Hi Maarten, > The set looks good to me. The core changes make sense and seem like a good > step > forward, and the driver changes seem like they do the right thing. > > Normally, I'd suggest waiting a week or so for maintainer acks to trickle in, > but since this is fixing behavior in i915, it makes sense to expedite it. > Perhaps ping individuals on IRC? > > For the whole set, > Reviewed-by: Sean Paul Series applied, thanks for review. :) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/atomic-helper: Fix leak in disable_all
Op 17-07-17 om 17:21 schreef Daniel Vetter: > On Mon, Jul 17, 2017 at 11:39:56AM +0200, Maarten Lankhorst wrote: >> Op 15-07-17 om 11:31 schreef Daniel Vetter: >>> The legacy plane->fb pointer is refcounted by calling >>> drm_atomic_clean_old_fb(). >>> >>> In practice this isn't a real problem because: >>> - The caller in the i915 gpu reset code restores the original state >>> again, which means the plane->fb pointer won't change, hence can't >>> leak. >>> - Drivers using drm_atomic_helper_shutdown call the fbdev cleanup >>> first, and that usually cleans up the fb through >>> drm_remove_framebuffer, which does this correctly. >>> - Without fbdev the only framebuffers are from userspace, and those >>> get cleaned up (again using drm_remove_framebuffer) befor the driver >>> can even be unloaded. >>> >>> But in i915 I've switched the cleanup sequence around so that the >>> _shutdown() calls happens after the drm_remove_framebuffer(), which is >>> how I discovered this issue. >>> >>> v2: My analysis why the current code was ok for gpu reset and >>> suspend/resume was correct, but then I totally failed to realize that >>> we better keep this symmetric. Thanksfully CI noticed that for >>> balance, a refcounting bug must exist at 2 places if previously there >>> was no issue ... >>> >>> v3: Don't be lazy and compute the plane_mask in >>> commit_duplicated_state properly too, instead of just using ~0U. >>> >>> Cc: martin.pe...@free.fr >>> Cc: ch...@chris-wilson.co.uk >>> Acked-by: Dave Airlie (v1) >>> Signed-off-by: Daniel Vetter >>> --- >>> drivers/gpu/drm/drm_atomic_helper.c | 18 -- >>> 1 file changed, 16 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/drm_atomic_helper.c >>> b/drivers/gpu/drm/drm_atomic_helper.c >>> index b07fc30372d3..545328a9a769 100644 >>> --- a/drivers/gpu/drm/drm_atomic_helper.c >>> +++ b/drivers/gpu/drm/drm_atomic_helper.c >>> @@ -2726,6 +2726,7 @@ int drm_atomic_helper_disable_all(struct drm_device >>> *dev, >>> struct drm_plane *plane; >>> struct drm_crtc_state *crtc_state; >>> struct drm_crtc *crtc; >>> + unsigned plane_mask = 0; >>> int ret, i; >>> >>> state = drm_atomic_state_alloc(dev); >>> @@ -2768,10 +2769,14 @@ int drm_atomic_helper_disable_all(struct drm_device >>> *dev, >>> goto free; >>> >>> drm_atomic_set_fb_for_plane(plane_state, NULL); >>> + plane_mask |= BIT(drm_plane_index(plane)); >>> + plane->old_fb = plane->fb; >>> } >>> >>> ret = drm_atomic_commit(state); >>> free: >>> + if (plane_mask) >>> + drm_atomic_clean_old_fb(dev, plane_mask, ret); >>> drm_atomic_state_put(state); >>> return ret; >>> } >>> @@ -2902,11 +2907,16 @@ int >>> drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, >>> struct drm_connector_state *new_conn_state; >>> struct drm_crtc *crtc; >>> struct drm_crtc_state *new_crtc_state; >>> + unsigned plane_mask = 0; >>> + struct drm_device *dev = state->dev; >>> + int ret; >>> >>> state->acquire_ctx = ctx; >>> >>> - for_each_new_plane_in_state(state, plane, new_plane_state, i) >>> + for_each_new_plane_in_state(state, plane, new_plane_state, i) { >>> + plane_mask |= BIT(drm_plane_index(plane)); >> We should really add a drm_plane_mask/drm_connector_mask at some point, to >> complement drm_crtc_mask. >>> state->planes[i].old_state = plane->state; >>> + } >>> >>> for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) >>> state->crtcs[i].old_state = crtc->state; >>> @@ -2914,7 +2924,11 @@ int drm_atomic_helper_commit_duplicated_state(struct >>> drm_atomic_state *state, >>> for_each_new_connector_in_state(state, connector, new_conn_state, i) >>> state->connectors[i].old_state = connector->state; >>> >>> - return drm_atomic_commit(state); >>> + ret = drm_atomic_commit(state); >>> + if (plane_mask) >>> + drm_atomic_clean_old_fb(dev, plane_mask, ret); >> Kill the if () part, and make it unconditional? On second thought, I should >> have done the same in drm_framebuffer.c > I'd prefer to not bikeshed this stuff too much and just go with what we do > everywhere else (i.e. rmfb code and atomic commit) for consistency. > clean_old_fb is definitely a horrible function with misleading kerneldoc > on top, and I think the best way to clean that up would be to: > > - Move the plane_mask computation in drm_atmic_commit and also put the > clean_old_fb call in there. Maybe give it a more descriptive name like > update_legacy_fb or whatever. Unexport them. > > - This would break the compat helpers, where drm_atomic_commit must not > update the legacy refcounts, because for set_config, page_flip and the > plane hooks the core does that already. Create a > drm_atomic_commit_legacy for these. > > But since one of my patches caused an existing issue to pop up as a > regression,
[Intel-gfx] [PATCH] drm/i915: Use AUX for backlight only if eDP 1.4 or later
With older panels, AUX pin for backlight doesn't work. On some panels, this causes backlight issues on S3 resume. Enable the feature only for eDP1.4 or later. Fix-suggested-by: Puthikorn Voravootivat Signed-off-by: Jenny TC --- drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 6 +- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c index b25cd88..421f31f 100644 --- a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c +++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c @@ -292,7 +292,7 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector, * via PWM pin or using AUX is better than using PWM pin. * * The heuristic to determine that using AUX pin is better than using PWM pin is - * that the panel support any of the feature list here. + * that the panel is eDP 1.4 or later and support any of the feature list here * - Regional backlight brightness adjustment * - Backlight PWM frequency set * - More than 8 bits resolution of brightness level @@ -310,6 +310,10 @@ static int intel_dp_aux_setup_backlight(struct intel_connector *connector, if (!(intel_dp->edp_dpcd[2] & DP_EDP_BACKLIGHT_BRIGHTNESS_PWM_PIN_CAP)) return true; + /* Enable this for eDP 1.4 panel or later. */ + if (intel_dp->edp_dpcd[0] < DP_EDP_14) + return false; + /* Panel supports regional backlight brightness adjustment */ if (drm_dp_dpcd_readb(&intel_dp->aux, DP_EDP_GENERAL_CAP_3, ®_val) != 1) { -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 11/14] drm/i915: Engine busy time tracking
Quoting Tvrtko Ursulin (2017-07-19 10:12:33) > > On 18/07/2017 16:19, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2017-07-18 15:36:15) > >> From: Tvrtko Ursulin > >> > >> Track total time requests have been executing on the hardware. > >> > >> To make this cheap it is hidden behind a static branch with the > >> intention that it is only enabled when there is a consumer > >> listening. This means that in the default off case the total > >> cost of the tracking is just a few no-op instructions on the > >> fast paths. > > > >> +static inline void intel_engine_context_in(struct intel_engine_cs *engine) > >> +{ > >> + if (static_branch_unlikely(&i915_engine_stats_key)) { > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&engine->stats.lock, flags); > > > > What's the purpose of this lock? RMW is ordered by virtue of the tasklet > > (only one cpu can be doing the rmw at any time). Did I miss another > > user? > > Hm it should be a plain spin_lock and a _bh variant on the external API. > > But the purpose is to allow atomic sampling of accumulated busy time > plus the current engine status. You can do that with a lockless read, if you treat it as a seqlock and the total as the latch. Something like u64 total, latch, start; total = engine->stats.total; do { latch = total; start = engine->stats.start; smp_rmb(); total = engine->stats.total; } while (total != latch); total += ktime_now() - latch; Assuming x86-64. If we only have 32b atomic reads, it is less fun. > >> + if (engine->stats.ref++ == 0) > >> + engine->stats.start = ktime_get_real_ns(); > > > > Use ktime_get_raw() and leave the conversion to ns to the caller. > > You mean both store in ktime_t and don't fetch the wall time but > monotonic? Do you say this because perf pmu perhaps needs monotonic or > for some other reason? We only want monotonic (otherwise we will observe time going backwards), but whether or not we want the clock source compensation? I was under the impression we could defer that compensation until later. > > What is the cost of a ktime nowadays? > > I don't see it in the profiles, so not much I think. readtsc is there, but not enough to worry. The effect on execution latency is in the noise on initial inspection. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v12 2/6] drm: Introduce RGB 64-bit 16:16:16:16 float format
The RGB 64-bit 16:16:16:16 float pixel format is needed by windows guest VM. This patch is to introduce the format to drm. v1: Suggested by Ville to submit this patch to dri-devel. Signed-off-by: Xiaoguang Chen Signed-off-by: Tina Zhang --- include/uapi/drm/drm_fourcc.h | 4 1 file changed, 4 insertions(+) diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 7586c46..3e002e3 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -113,6 +113,10 @@ extern "C" { #define DRM_FORMAT_AYUVfourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */ +/* 64 bpp RGB */ +#define DRM_FORMAT_XRGB161616 fourcc_code('X', 'R', '4', '8') /* [63:0] x:R:G:B 16:16:16:16 little endian */ +#define DRM_FORMAT_XBGR161616 fourcc_code('X', 'B', '4', '8') /* [63:0] x:B:G:R 16:16:16:16 little endian */ + /* * 2 plane RGB + A * index 0 = RGB plane, same format as the corresponding non _A8 format has -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v12 0/6] drm/i915/gvt: Dma-buf support for GVT-g
v11->v12: 1) add drm_format_mod back. (Gerd and Zhenyu) 2) add region_index. (Gerd) 3) refine the lifecycle of dmabuf. 4) send to dri-de...@lists.freedesktop.org. (Ville) v10->v11: 1) rename plane_type to drm_plane_type. (Gerd) 2) move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info. (Gerd) 3) remove drm_format_mod, start fields. (Daniel) 4) remove plane_id. v9->v10: 1) remove dma-buf management 2) refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE 3) track the dma-buf create and release in kernel mode v8->v9: 1) refine the dma-buf ioctl definition 2) add a lock to protect the dmabuf list 3) move drm format change to a separate patch 4) codes cleanup v7->v8: 1) refine framebuffer decoder code 2) fix a bug in decoding primary plane v6->v7: 1) release dma-buf related allocations in dma-buf's associated release function. 2) refine ioctl interface for querying plane info or create dma-buf 3) refine framebuffer decoder code 4) the patch series is based on 4.12.0-rc1 v5->v6: 1) align the dma-buf life cycle with the vfio device. 2) add the dma-buf releated operations in a separate patch. 3) i915 releated changes. v4->v5: 1) fix bug while checking whether the gem obj is gvt's dma-buf when user change caching mode or domains. Add a helper function to do it. 2) add definition for the query plane and create dma-buf. v3->v4: 1) fix bug while checking whether the gem obj is gvt's dma-buf when set caching mode or doamins. v2->v3: 1) add a field gvt_plane_info in the drm_i915_gem_obj structure to save the decoded plane information to avoid look up while need the plane info. 2) declare a new flag I915_GEM_OBJECT_IS_GVT_DMABUF in drm_i915_gem_object to represent the gem obj for gvt's dma-buf. The tiling mode, caching mode and domains can not be changed for this kind of gem object. 3) change dma-buf related information to be more generic. So other vendor can use the same interface. v1->v2: 1) create a management fd for dma-buf operations. 2) alloc gem object's backing storage in gem obj's get_pages() callback. This patch set adds the dma-buf support for intel GVT-g. dma-buf is a uniform mechanism to share DMA buffers across different devices and sub-systems. dma-buf for intel GVT-g is mainly used to share the vgpu's framebuffer to other users or sub-systems so they can use the dma-buf to show the desktop of a vm which uses intel vgpu. The main idea is we create a gem object and set vgpu's framebuffer as the backing storage of this gem object. And associate this gem obj to a dma-buf object then export this dma-buf at the meantime generate a file descriptor for this dma-buf. Finally deliver this file descriptor to user space. And user can use this dma-buf fd to do render or other operations. Tina Zhang (6): drm/i915/gvt: Add framebuffer decoder support drm: Introduce RGB 64-bit 16:16:16:16 float format drm/i915/gvt: Add RGB 64-bit 16:16:16:16 float format support drm/i915/gvt: add opregion support vfio: ABI for mdev display dma-buf operation drm/i915: Introduce GEM proxy drivers/gpu/drm/i915/gvt/Makefile | 3 +- drivers/gpu/drm/i915/gvt/display.c | 2 +- drivers/gpu/drm/i915/gvt/display.h | 2 + drivers/gpu/drm/i915/gvt/fb_decoder.c | 440 + drivers/gpu/drm/i915/gvt/fb_decoder.h | 175 + drivers/gpu/drm/i915/gvt/gvt.h | 1 + drivers/gpu/drm/i915/gvt/hypercall.h | 1 + drivers/gpu/drm/i915/gvt/kvmgt.c | 109 +++- drivers/gpu/drm/i915/gvt/mpt.h | 15 ++ drivers/gpu/drm/i915/gvt/opregion.c| 26 +- drivers/gpu/drm/i915/gvt/vgpu.c| 4 + drivers/gpu/drm/i915/i915_gem.c| 26 +- drivers/gpu/drm/i915/i915_gem_object.h | 9 + drivers/gpu/drm/i915/i915_gem_tiling.c | 5 + include/uapi/drm/drm_fourcc.h | 4 + include/uapi/linux/vfio.h | 28 +++ 16 files changed, 838 insertions(+), 12 deletions(-) create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v12 3/6] drm/i915/gvt: Add RGB 64-bit 16:16:16:16 float format support
The RGB 64-bit 16:16:16:16 float pixel format is needed by windows 10 guest VM. This patch is to add this pixel format support to gvt device model. Without this patch, some Apps, e.g. "DXGIGammaVM.exe", will crash and make guest screen black. Signed-off-by: Xiaoguang Chen Signed-off-by: Tina Zhang --- drivers/gpu/drm/i915/gvt/fb_decoder.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c b/drivers/gpu/drm/i915/gvt/fb_decoder.c index 2bd5b3c..739ca81 100644 --- a/drivers/gpu/drm/i915/gvt/fb_decoder.c +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c @@ -54,6 +54,8 @@ static struct pixel_format bdw_pixel_formats[PRIMARY_FORMAT_NUM] = { "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"}, [0xa] = {DRM_FORMAT_XRGB2101010, 32, "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"}, + [0xc] = {DRM_FORMAT_XRGB161616, 64, + "64-bit RGBX Floating Point(16:16:16:16 MSB-X:B:G:R)"}, [0xe] = {DRM_FORMAT_XBGR, 32, "32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"}, }; @@ -75,6 +77,10 @@ static struct pixel_format skl_pixel_formats[] = { {DRM_FORMAT_XBGR2101010, 32, "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"}, {DRM_FORMAT_XRGB2101010, 32, "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"}, + {DRM_FORMAT_XRGB161616, 64, + "64-bit XRGB (16:16:16:16 MSB-X:R:G:B)"}, + {DRM_FORMAT_XBGR161616, 64, + "64-bit XBGR (16:16:16:16 MSB-X:B:G:R)"}, /* non-supported format has bpp default to 0 */ {0, 0, NULL}, @@ -101,6 +107,9 @@ static int skl_format_to_drm(int format, bool rgb_order, bool alpha, case PLANE_CTL_FORMAT_XRGB_2101010: skl_pixel_formats_index = rgb_order ? 10 : 11; break; + case PLANE_CTL_FORMAT_XRGB_16161616F: + skl_pixel_formats_index = rgb_order ? 12 : 13; + break; case PLANE_CTL_FORMAT_YUV422: skl_pixel_formats_index = yuv_order >> 16; if (skl_pixel_formats_index > 3) @@ -321,6 +330,8 @@ static struct pixel_format sprite_pixel_formats[SPRITE_FORMAT_NUM] = { [0x0] = {DRM_FORMAT_YUV422, 16, "YUV 16-bit 4:2:2 packed"}, [0x1] = {DRM_FORMAT_XRGB2101010, 32, "RGB 32-bit 2:10:10:10"}, [0x2] = {DRM_FORMAT_XRGB, 32, "RGB 32-bit 8:8:8:8"}, + [0x3] = {DRM_FORMAT_XRGB161616, 64, + "RGB 64-bit 16:16:16:16 Floating Point"}, [0x4] = {DRM_FORMAT_AYUV, 32, "YUV 32-bit 4:4:4 packed (8:8:8:8 MSB-X:Y:U:V)"}, }; -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v12 1/6] drm/i915/gvt: Add framebuffer decoder support
Framebuffer decoder returns guest framebuffer information. Guest framebuffer includes primary, cursor and sprite plane. Signed-off-by: Xiaoguang Chen Signed-off-by: Tina Zhang --- drivers/gpu/drm/i915/gvt/Makefile | 3 +- drivers/gpu/drm/i915/gvt/display.c| 2 +- drivers/gpu/drm/i915/gvt/display.h| 2 + drivers/gpu/drm/i915/gvt/fb_decoder.c | 429 ++ drivers/gpu/drm/i915/gvt/fb_decoder.h | 175 ++ drivers/gpu/drm/i915/gvt/gvt.h| 1 + 6 files changed, 610 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile index f5486cb9..019d596 100644 --- a/drivers/gpu/drm/i915/gvt/Makefile +++ b/drivers/gpu/drm/i915/gvt/Makefile @@ -1,7 +1,8 @@ GVT_DIR := gvt GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \ interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \ - execlist.o scheduler.o sched_policy.o render.o cmd_parser.o + execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \ + fb_decoder.o ccflags-y += -I$(src) -I$(src)/$(GVT_DIR) i915-y += $(addprefix $(GVT_DIR)/, $(GVT_SOURCE)) diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c index 2deb05f..58d90cf 100644 --- a/drivers/gpu/drm/i915/gvt/display.c +++ b/drivers/gpu/drm/i915/gvt/display.c @@ -67,7 +67,7 @@ static int edp_pipe_is_enabled(struct intel_vgpu *vgpu) return 1; } -static int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe) +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe) { struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv; diff --git a/drivers/gpu/drm/i915/gvt/display.h b/drivers/gpu/drm/i915/gvt/display.h index d73de22..b46b868 100644 --- a/drivers/gpu/drm/i915/gvt/display.h +++ b/drivers/gpu/drm/i915/gvt/display.h @@ -179,4 +179,6 @@ int intel_vgpu_init_display(struct intel_vgpu *vgpu, u64 resolution); void intel_vgpu_reset_display(struct intel_vgpu *vgpu); void intel_vgpu_clean_display(struct intel_vgpu *vgpu); +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe); + #endif diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c b/drivers/gpu/drm/i915/gvt/fb_decoder.c new file mode 100644 index 000..2bd5b3c --- /dev/null +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c @@ -0,0 +1,429 @@ +/* + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + * + * Authors: + *Kevin Tian + * + * Contributors: + *Bing Niu + *Xu Han + *Ping Gao + *Xiaoguang Chen + *Yang Liu + *Tina Zhang + * + */ + +#include +#include "i915_drv.h" +#include "gvt.h" + +#define PRIMARY_FORMAT_NUM 16 +struct pixel_format { + int drm_format; /* Pixel format in DRM definition */ + int bpp;/* Bits per pixel, 0 indicates invalid */ + char *desc; /* The description */ +}; + +/* non-supported format has bpp default to 0 */ +static struct pixel_format bdw_pixel_formats[PRIMARY_FORMAT_NUM] = { + [0x2] = {DRM_FORMAT_C8, 8, "8-bit Indexed"}, + [0x5] = {DRM_FORMAT_RGB565, 16, "16-bit BGRX (5:6:5 MSB-R:G:B)"}, + [0x6] = {DRM_FORMAT_XRGB, 32, + "32-bit BGRX (8:8:8:8 MSB-X:R:G:B)"}, + [0x8] = {DRM_FORMAT_XBGR2101010, 32, + "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"}, + [0xa] = {DRM_FORMAT_XRGB2101010, 32, + "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"}, + [0xe] = {DRM_FORMAT_XBGR, 32, + "32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"}, +}; + +/* non-supported format has bpp default to 0 */ +static struct pixel_format skl_pixel_formats[] = { +
Re: [Intel-gfx] [PATCH] drm/i915/selftests: Attach a stub pm_domain
On 18 July 2017 at 18:30, Chris Wilson wrote: > Supply a pm_domain and its ops for our mock GEM device so that > device runtime pm doesn't complain even though we only want to mark it > permanently active! > > Signed-off-by: Chris Wilson Fixes it for me, so: Tested-by: Matthew Auld Reviewed-by: Matthew Auld ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v12 0/6] drm/i915/gvt: Dma-buf support for GVT-g
v11->v12: 1) add drm_format_mod back. (Gerd and Zhenyu) 2) add region_index. (Gerd) 3) refine the lifecycle of dmabuf. 4) send to dri-de...@lists.freedesktop.org. (Ville) v10->v11: 1) rename plane_type to drm_plane_type. (Gerd) 2) move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info. (Gerd) 3) remove drm_format_mod, start fields. (Daniel) 4) remove plane_id. v9->v10: 1) remove dma-buf management 2) refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE 3) track the dma-buf create and release in kernel mode v8->v9: 1) refine the dma-buf ioctl definition 2) add a lock to protect the dmabuf list 3) move drm format change to a separate patch 4) codes cleanup v7->v8: 1) refine framebuffer decoder code 2) fix a bug in decoding primary plane v6->v7: 1) release dma-buf related allocations in dma-buf's associated release function. 2) refine ioctl interface for querying plane info or create dma-buf 3) refine framebuffer decoder code 4) the patch series is based on 4.12.0-rc1 v5->v6: 1) align the dma-buf life cycle with the vfio device. 2) add the dma-buf releated operations in a separate patch. 3) i915 releated changes. v4->v5: 1) fix bug while checking whether the gem obj is gvt's dma-buf when user change caching mode or domains. Add a helper function to do it. 2) add definition for the query plane and create dma-buf. v3->v4: 1) fix bug while checking whether the gem obj is gvt's dma-buf when set caching mode or doamins. v2->v3: 1) add a field gvt_plane_info in the drm_i915_gem_obj structure to save the decoded plane information to avoid look up while need the plane info. 2) declare a new flag I915_GEM_OBJECT_IS_GVT_DMABUF in drm_i915_gem_object to represent the gem obj for gvt's dma-buf. The tiling mode, caching mode and domains can not be changed for this kind of gem object. 3) change dma-buf related information to be more generic. So other vendor can use the same interface. v1->v2: 1) create a management fd for dma-buf operations. 2) alloc gem object's backing storage in gem obj's get_pages() callback. This patch set adds the dma-buf support for intel GVT-g. dma-buf is a uniform mechanism to share DMA buffers across different devices and sub-systems. dma-buf for intel GVT-g is mainly used to share the vgpu's framebuffer to other users or sub-systems so they can use the dma-buf to show the desktop of a vm which uses intel vgpu. The main idea is we create a gem object and set vgpu's framebuffer as the backing storage of this gem object. And associate this gem obj to a dma-buf object then export this dma-buf at the meantime generate a file descriptor for this dma-buf. Finally deliver this file descriptor to user space. And user can use this dma-buf fd to do render or other operations. Tina Zhang (6): drm/i915/gvt: Add framebuffer decoder support drm: Introduce RGB 64-bit 16:16:16:16 float format drm/i915/gvt: Add RGB 64-bit 16:16:16:16 float format support drm/i915/gvt: add opregion support vfio: ABI for mdev display dma-buf operation drm/i915: Introduce GEM proxy drivers/gpu/drm/i915/gvt/Makefile | 3 +- drivers/gpu/drm/i915/gvt/display.c | 2 +- drivers/gpu/drm/i915/gvt/display.h | 2 + drivers/gpu/drm/i915/gvt/fb_decoder.c | 440 + drivers/gpu/drm/i915/gvt/fb_decoder.h | 175 + drivers/gpu/drm/i915/gvt/gvt.h | 1 + drivers/gpu/drm/i915/gvt/hypercall.h | 1 + drivers/gpu/drm/i915/gvt/kvmgt.c | 109 +++- drivers/gpu/drm/i915/gvt/mpt.h | 15 ++ drivers/gpu/drm/i915/gvt/opregion.c| 26 +- drivers/gpu/drm/i915/gvt/vgpu.c| 4 + drivers/gpu/drm/i915/i915_gem.c| 26 +- drivers/gpu/drm/i915/i915_gem_object.h | 9 + drivers/gpu/drm/i915/i915_gem_tiling.c | 5 + include/uapi/drm/drm_fourcc.h | 4 + include/uapi/linux/vfio.h | 28 +++ 16 files changed, 838 insertions(+), 12 deletions(-) create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v12 2/6] drm: Introduce RGB 64-bit 16:16:16:16 float format
The RGB 64-bit 16:16:16:16 float pixel format is needed by windows guest VM. This patch is to introduce the format to drm. v1: Suggested by Ville to submit this patch to dri-devel. Signed-off-by: Xiaoguang Chen Signed-off-by: Tina Zhang --- include/uapi/drm/drm_fourcc.h | 4 1 file changed, 4 insertions(+) diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 7586c46..3e002e3 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -113,6 +113,10 @@ extern "C" { #define DRM_FORMAT_AYUVfourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */ +/* 64 bpp RGB */ +#define DRM_FORMAT_XRGB161616 fourcc_code('X', 'R', '4', '8') /* [63:0] x:R:G:B 16:16:16:16 little endian */ +#define DRM_FORMAT_XBGR161616 fourcc_code('X', 'B', '4', '8') /* [63:0] x:B:G:R 16:16:16:16 little endian */ + /* * 2 plane RGB + A * index 0 = RGB plane, same format as the corresponding non _A8 format has -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v12 1/6] drm/i915/gvt: Add framebuffer decoder support
Framebuffer decoder returns guest framebuffer information. Guest framebuffer includes primary, cursor and sprite plane. Signed-off-by: Xiaoguang Chen Signed-off-by: Tina Zhang --- drivers/gpu/drm/i915/gvt/Makefile | 3 +- drivers/gpu/drm/i915/gvt/display.c| 2 +- drivers/gpu/drm/i915/gvt/display.h| 2 + drivers/gpu/drm/i915/gvt/fb_decoder.c | 429 ++ drivers/gpu/drm/i915/gvt/fb_decoder.h | 175 ++ drivers/gpu/drm/i915/gvt/gvt.h| 1 + 6 files changed, 610 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile index f5486cb9..019d596 100644 --- a/drivers/gpu/drm/i915/gvt/Makefile +++ b/drivers/gpu/drm/i915/gvt/Makefile @@ -1,7 +1,8 @@ GVT_DIR := gvt GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \ interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \ - execlist.o scheduler.o sched_policy.o render.o cmd_parser.o + execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \ + fb_decoder.o ccflags-y += -I$(src) -I$(src)/$(GVT_DIR) i915-y += $(addprefix $(GVT_DIR)/, $(GVT_SOURCE)) diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c index 2deb05f..58d90cf 100644 --- a/drivers/gpu/drm/i915/gvt/display.c +++ b/drivers/gpu/drm/i915/gvt/display.c @@ -67,7 +67,7 @@ static int edp_pipe_is_enabled(struct intel_vgpu *vgpu) return 1; } -static int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe) +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe) { struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv; diff --git a/drivers/gpu/drm/i915/gvt/display.h b/drivers/gpu/drm/i915/gvt/display.h index d73de22..b46b868 100644 --- a/drivers/gpu/drm/i915/gvt/display.h +++ b/drivers/gpu/drm/i915/gvt/display.h @@ -179,4 +179,6 @@ int intel_vgpu_init_display(struct intel_vgpu *vgpu, u64 resolution); void intel_vgpu_reset_display(struct intel_vgpu *vgpu); void intel_vgpu_clean_display(struct intel_vgpu *vgpu); +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe); + #endif diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c b/drivers/gpu/drm/i915/gvt/fb_decoder.c new file mode 100644 index 000..2bd5b3c --- /dev/null +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c @@ -0,0 +1,429 @@ +/* + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + * + * Authors: + *Kevin Tian + * + * Contributors: + *Bing Niu + *Xu Han + *Ping Gao + *Xiaoguang Chen + *Yang Liu + *Tina Zhang + * + */ + +#include +#include "i915_drv.h" +#include "gvt.h" + +#define PRIMARY_FORMAT_NUM 16 +struct pixel_format { + int drm_format; /* Pixel format in DRM definition */ + int bpp;/* Bits per pixel, 0 indicates invalid */ + char *desc; /* The description */ +}; + +/* non-supported format has bpp default to 0 */ +static struct pixel_format bdw_pixel_formats[PRIMARY_FORMAT_NUM] = { + [0x2] = {DRM_FORMAT_C8, 8, "8-bit Indexed"}, + [0x5] = {DRM_FORMAT_RGB565, 16, "16-bit BGRX (5:6:5 MSB-R:G:B)"}, + [0x6] = {DRM_FORMAT_XRGB, 32, + "32-bit BGRX (8:8:8:8 MSB-X:R:G:B)"}, + [0x8] = {DRM_FORMAT_XBGR2101010, 32, + "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"}, + [0xa] = {DRM_FORMAT_XRGB2101010, 32, + "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"}, + [0xe] = {DRM_FORMAT_XBGR, 32, + "32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"}, +}; + +/* non-supported format has bpp default to 0 */ +static struct pixel_format skl_pixel_formats[] = { +
[Intel-gfx] [PATCH v12 3/6] drm/i915/gvt: Add RGB 64-bit 16:16:16:16 float format support
The RGB 64-bit 16:16:16:16 float pixel format is needed by windows 10 guest VM. This patch is to add this pixel format support to gvt device model. Without this patch, some Apps, e.g. "DXGIGammaVM.exe", will crash and make guest screen black. Signed-off-by: Xiaoguang Chen Signed-off-by: Tina Zhang --- drivers/gpu/drm/i915/gvt/fb_decoder.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c b/drivers/gpu/drm/i915/gvt/fb_decoder.c index 2bd5b3c..739ca81 100644 --- a/drivers/gpu/drm/i915/gvt/fb_decoder.c +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c @@ -54,6 +54,8 @@ static struct pixel_format bdw_pixel_formats[PRIMARY_FORMAT_NUM] = { "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"}, [0xa] = {DRM_FORMAT_XRGB2101010, 32, "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"}, + [0xc] = {DRM_FORMAT_XRGB161616, 64, + "64-bit RGBX Floating Point(16:16:16:16 MSB-X:B:G:R)"}, [0xe] = {DRM_FORMAT_XBGR, 32, "32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"}, }; @@ -75,6 +77,10 @@ static struct pixel_format skl_pixel_formats[] = { {DRM_FORMAT_XBGR2101010, 32, "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"}, {DRM_FORMAT_XRGB2101010, 32, "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"}, + {DRM_FORMAT_XRGB161616, 64, + "64-bit XRGB (16:16:16:16 MSB-X:R:G:B)"}, + {DRM_FORMAT_XBGR161616, 64, + "64-bit XBGR (16:16:16:16 MSB-X:B:G:R)"}, /* non-supported format has bpp default to 0 */ {0, 0, NULL}, @@ -101,6 +107,9 @@ static int skl_format_to_drm(int format, bool rgb_order, bool alpha, case PLANE_CTL_FORMAT_XRGB_2101010: skl_pixel_formats_index = rgb_order ? 10 : 11; break; + case PLANE_CTL_FORMAT_XRGB_16161616F: + skl_pixel_formats_index = rgb_order ? 12 : 13; + break; case PLANE_CTL_FORMAT_YUV422: skl_pixel_formats_index = yuv_order >> 16; if (skl_pixel_formats_index > 3) @@ -321,6 +330,8 @@ static struct pixel_format sprite_pixel_formats[SPRITE_FORMAT_NUM] = { [0x0] = {DRM_FORMAT_YUV422, 16, "YUV 16-bit 4:2:2 packed"}, [0x1] = {DRM_FORMAT_XRGB2101010, 32, "RGB 32-bit 2:10:10:10"}, [0x2] = {DRM_FORMAT_XRGB, 32, "RGB 32-bit 8:8:8:8"}, + [0x3] = {DRM_FORMAT_XRGB161616, 64, + "RGB 64-bit 16:16:16:16 Floating Point"}, [0x4] = {DRM_FORMAT_AYUV, 32, "YUV 32-bit 4:4:4 packed (8:8:8:8 MSB-X:Y:U:V)"}, }; -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [intel-gfx][i-g-t] test kms_frontbuffer_tracking basic fail
Hi Arek This time it looks like a bug I run this test on i7-4770, by command `./igt/tests/kms_frontbuffer_tracking --run-subtest basic` I guess it may be caused by I have no monitor connected. But if that is the case it should be skiped rather than fail. Output shows below --- IGT-Version: 1.19-g4258cc8e (x86_64) (Linux: 4.12.0 x86_64) Primary screen: VGA 1024x768, crtc 0 FBC last action not supported Can't test PSR: no usable eDP screen. Sink CRC not supported: primary screen is not eDP Stack trace: #0 [_init+0x24ef9] #1 [_init+0x1a284] #2 [_init+0x27812] #3 [_init+0x27fc4] #4 [_init+0x8969] #5 [_init+0x10a3d] #6 [_init+0x2a70] #7 [__libc_start_main+0xf1] #8 [_init+0x4d42] #9 [+0x4d42] Subtest basic: FAIL (1.082s) Error "(kms_frontbuffer_tracking:4411) ioctl-wrappers-CRITICAL: Test assertion failure function gem_mmap__wc, file ioctl_wrappers.c:825: (kms_frontbuffer_tracking:4411) ioctl-wrappers-CRITICAL: Failed assertion: ptr (kms_frontbuffer_tracking:4411) ioctl-wrappers-CRITICAL: Last errno: 38, Function not implemented Subtest basic failed. DEBUG (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) ioctl-wrappers-DEBUG: Test requirement passed: err == 0 (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(width=1024, height=768, format=0x34325258, tiling=0x101, size=3145728) (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(handle=1, pitch=4096) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(width=64, height=64, format=0x34325241, tiling=0x0, size=16384) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(handle=2, pitch=256) (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(width=64, height=64, format=0x34325258, tiling=0x101, size=32768) (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(handle=3, pitch=512) (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(width=1024, height=1024, format=0x34325258, tiling=0x101, size=4194304) (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(handle=4, pitch=4096) (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(width=1524, height=2292, format=0x34325258, tiling=0x101, size=14106624) (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(handle=5, pitch=6144) (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(width=1024, height=768, format=0x34325258, tiling=0x101, size=3145728) (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(handle=6, pitch=4096) (kms_frontbuffer_tracking:4411) DEBUG: Blue CRC: pipe:[f571dfae ] sink:[unsupported!] (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(width=1024, height=768, format=0x34325258, tiling=0x101, size=3145728) (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(handle=6, pitch=4096) (kms_frontbuffer_tracking:4411) drmtest-DEBUG: Test requirement passed: is_i915_device(fd) && has_known_intel_chipset(fd) (kms_frontbuffer_tracking:4411) igt-fb-DEBUG: igt_create_fb_with_bo_size(width=1024, height=768, format=0x34
[Intel-gfx] [PATCH v12 1/6] drm/i915/gvt: Add framebuffer decoder support
Framebuffer decoder returns guest framebuffer information. Guest framebuffer includes primary, cursor and sprite plane. Signed-off-by: Xiaoguang Chen Signed-off-by: Tina Zhang --- drivers/gpu/drm/i915/gvt/Makefile | 3 +- drivers/gpu/drm/i915/gvt/display.c| 2 +- drivers/gpu/drm/i915/gvt/display.h| 2 + drivers/gpu/drm/i915/gvt/fb_decoder.c | 429 ++ drivers/gpu/drm/i915/gvt/fb_decoder.h | 175 ++ drivers/gpu/drm/i915/gvt/gvt.h| 1 + 6 files changed, 610 insertions(+), 2 deletions(-) create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h diff --git a/drivers/gpu/drm/i915/gvt/Makefile b/drivers/gpu/drm/i915/gvt/Makefile index f5486cb9..019d596 100644 --- a/drivers/gpu/drm/i915/gvt/Makefile +++ b/drivers/gpu/drm/i915/gvt/Makefile @@ -1,7 +1,8 @@ GVT_DIR := gvt GVT_SOURCE := gvt.o aperture_gm.o handlers.o vgpu.o trace_points.o firmware.o \ interrupt.o gtt.o cfg_space.o opregion.o mmio.o display.o edid.o \ - execlist.o scheduler.o sched_policy.o render.o cmd_parser.o + execlist.o scheduler.o sched_policy.o render.o cmd_parser.o \ + fb_decoder.o ccflags-y += -I$(src) -I$(src)/$(GVT_DIR) i915-y += $(addprefix $(GVT_DIR)/, $(GVT_SOURCE)) diff --git a/drivers/gpu/drm/i915/gvt/display.c b/drivers/gpu/drm/i915/gvt/display.c index 2deb05f..58d90cf 100644 --- a/drivers/gpu/drm/i915/gvt/display.c +++ b/drivers/gpu/drm/i915/gvt/display.c @@ -67,7 +67,7 @@ static int edp_pipe_is_enabled(struct intel_vgpu *vgpu) return 1; } -static int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe) +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe) { struct drm_i915_private *dev_priv = vgpu->gvt->dev_priv; diff --git a/drivers/gpu/drm/i915/gvt/display.h b/drivers/gpu/drm/i915/gvt/display.h index d73de22..b46b868 100644 --- a/drivers/gpu/drm/i915/gvt/display.h +++ b/drivers/gpu/drm/i915/gvt/display.h @@ -179,4 +179,6 @@ int intel_vgpu_init_display(struct intel_vgpu *vgpu, u64 resolution); void intel_vgpu_reset_display(struct intel_vgpu *vgpu); void intel_vgpu_clean_display(struct intel_vgpu *vgpu); +int pipe_is_enabled(struct intel_vgpu *vgpu, int pipe); + #endif diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c b/drivers/gpu/drm/i915/gvt/fb_decoder.c new file mode 100644 index 000..2bd5b3c --- /dev/null +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c @@ -0,0 +1,429 @@ +/* + * Copyright(c) 2011-2016 Intel Corporation. All rights reserved. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE + * SOFTWARE. + * + * Authors: + *Kevin Tian + * + * Contributors: + *Bing Niu + *Xu Han + *Ping Gao + *Xiaoguang Chen + *Yang Liu + *Tina Zhang + * + */ + +#include +#include "i915_drv.h" +#include "gvt.h" + +#define PRIMARY_FORMAT_NUM 16 +struct pixel_format { + int drm_format; /* Pixel format in DRM definition */ + int bpp;/* Bits per pixel, 0 indicates invalid */ + char *desc; /* The description */ +}; + +/* non-supported format has bpp default to 0 */ +static struct pixel_format bdw_pixel_formats[PRIMARY_FORMAT_NUM] = { + [0x2] = {DRM_FORMAT_C8, 8, "8-bit Indexed"}, + [0x5] = {DRM_FORMAT_RGB565, 16, "16-bit BGRX (5:6:5 MSB-R:G:B)"}, + [0x6] = {DRM_FORMAT_XRGB, 32, + "32-bit BGRX (8:8:8:8 MSB-X:R:G:B)"}, + [0x8] = {DRM_FORMAT_XBGR2101010, 32, + "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"}, + [0xa] = {DRM_FORMAT_XRGB2101010, 32, + "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"}, + [0xe] = {DRM_FORMAT_XBGR, 32, + "32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"}, +}; + +/* non-supported format has bpp default to 0 */ +static struct pixel_format skl_pixel_formats[] = { +
[Intel-gfx] [PATCH v12 0/6] drm/i915/gvt: Dma-buf support for GVT-g
v11->v12: 1) add drm_format_mod back. (Gerd and Zhenyu) 2) add region_index. (Gerd) 3) refine the lifecycle of dmabuf. 4) send to dri-de...@lists.freedesktop.org. (Ville) v10->v11: 1) rename plane_type to drm_plane_type. (Gerd) 2) move fields of vfio_device_query_gfx_plane to vfio_device_gfx_plane_info. (Gerd) 3) remove drm_format_mod, start fields. (Daniel) 4) remove plane_id. v9->v10: 1) remove dma-buf management 2) refine the ABI API VFIO_DEVICE_QUERY_GFX_PLANE 3) track the dma-buf create and release in kernel mode v8->v9: 1) refine the dma-buf ioctl definition 2) add a lock to protect the dmabuf list 3) move drm format change to a separate patch 4) codes cleanup v7->v8: 1) refine framebuffer decoder code 2) fix a bug in decoding primary plane v6->v7: 1) release dma-buf related allocations in dma-buf's associated release function. 2) refine ioctl interface for querying plane info or create dma-buf 3) refine framebuffer decoder code 4) the patch series is based on 4.12.0-rc1 v5->v6: 1) align the dma-buf life cycle with the vfio device. 2) add the dma-buf releated operations in a separate patch. 3) i915 releated changes. v4->v5: 1) fix bug while checking whether the gem obj is gvt's dma-buf when user change caching mode or domains. Add a helper function to do it. 2) add definition for the query plane and create dma-buf. v3->v4: 1) fix bug while checking whether the gem obj is gvt's dma-buf when set caching mode or doamins. v2->v3: 1) add a field gvt_plane_info in the drm_i915_gem_obj structure to save the decoded plane information to avoid look up while need the plane info. 2) declare a new flag I915_GEM_OBJECT_IS_GVT_DMABUF in drm_i915_gem_object to represent the gem obj for gvt's dma-buf. The tiling mode, caching mode and domains can not be changed for this kind of gem object. 3) change dma-buf related information to be more generic. So other vendor can use the same interface. v1->v2: 1) create a management fd for dma-buf operations. 2) alloc gem object's backing storage in gem obj's get_pages() callback. This patch set adds the dma-buf support for intel GVT-g. dma-buf is a uniform mechanism to share DMA buffers across different devices and sub-systems. dma-buf for intel GVT-g is mainly used to share the vgpu's framebuffer to other users or sub-systems so they can use the dma-buf to show the desktop of a vm which uses intel vgpu. The main idea is we create a gem object and set vgpu's framebuffer as the backing storage of this gem object. And associate this gem obj to a dma-buf object then export this dma-buf at the meantime generate a file descriptor for this dma-buf. Finally deliver this file descriptor to user space. And user can use this dma-buf fd to do render or other operations. Tina Zhang (6): drm/i915/gvt: Add framebuffer decoder support drm: Introduce RGB 64-bit 16:16:16:16 float format drm/i915/gvt: Add RGB 64-bit 16:16:16:16 float format support drm/i915/gvt: add opregion support vfio: ABI for mdev display dma-buf operation drm/i915: Introduce GEM proxy drivers/gpu/drm/i915/gvt/Makefile | 3 +- drivers/gpu/drm/i915/gvt/display.c | 2 +- drivers/gpu/drm/i915/gvt/display.h | 2 + drivers/gpu/drm/i915/gvt/fb_decoder.c | 440 + drivers/gpu/drm/i915/gvt/fb_decoder.h | 175 + drivers/gpu/drm/i915/gvt/gvt.h | 1 + drivers/gpu/drm/i915/gvt/hypercall.h | 1 + drivers/gpu/drm/i915/gvt/kvmgt.c | 109 +++- drivers/gpu/drm/i915/gvt/mpt.h | 15 ++ drivers/gpu/drm/i915/gvt/opregion.c| 26 +- drivers/gpu/drm/i915/gvt/vgpu.c| 4 + drivers/gpu/drm/i915/i915_gem.c| 26 +- drivers/gpu/drm/i915/i915_gem_object.h | 9 + drivers/gpu/drm/i915/i915_gem_tiling.c | 5 + include/uapi/drm/drm_fourcc.h | 4 + include/uapi/linux/vfio.h | 28 +++ 16 files changed, 838 insertions(+), 12 deletions(-) create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.c create mode 100644 drivers/gpu/drm/i915/gvt/fb_decoder.h -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v12 2/6] drm: Introduce RGB 64-bit 16:16:16:16 float format
The RGB 64-bit 16:16:16:16 float pixel format is needed by windows guest VM. This patch is to introduce the format to drm. v1: Suggested by Ville to submit this patch to dri-devel. Signed-off-by: Xiaoguang Chen Signed-off-by: Tina Zhang --- include/uapi/drm/drm_fourcc.h | 4 1 file changed, 4 insertions(+) diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index 7586c46..3e002e3 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -113,6 +113,10 @@ extern "C" { #define DRM_FORMAT_AYUVfourcc_code('A', 'Y', 'U', 'V') /* [31:0] A:Y:Cb:Cr 8:8:8:8 little endian */ +/* 64 bpp RGB */ +#define DRM_FORMAT_XRGB161616 fourcc_code('X', 'R', '4', '8') /* [63:0] x:R:G:B 16:16:16:16 little endian */ +#define DRM_FORMAT_XBGR161616 fourcc_code('X', 'B', '4', '8') /* [63:0] x:B:G:R 16:16:16:16 little endian */ + /* * 2 plane RGB + A * index 0 = RGB plane, same format as the corresponding non _A8 format has -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v12 3/6] drm/i915/gvt: Add RGB 64-bit 16:16:16:16 float format support
The RGB 64-bit 16:16:16:16 float pixel format is needed by windows 10 guest VM. This patch is to add this pixel format support to gvt device model. Without this patch, some Apps, e.g. "DXGIGammaVM.exe", will crash and make guest screen black. Signed-off-by: Xiaoguang Chen Signed-off-by: Tina Zhang --- drivers/gpu/drm/i915/gvt/fb_decoder.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/gvt/fb_decoder.c b/drivers/gpu/drm/i915/gvt/fb_decoder.c index 2bd5b3c..739ca81 100644 --- a/drivers/gpu/drm/i915/gvt/fb_decoder.c +++ b/drivers/gpu/drm/i915/gvt/fb_decoder.c @@ -54,6 +54,8 @@ static struct pixel_format bdw_pixel_formats[PRIMARY_FORMAT_NUM] = { "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"}, [0xa] = {DRM_FORMAT_XRGB2101010, 32, "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"}, + [0xc] = {DRM_FORMAT_XRGB161616, 64, + "64-bit RGBX Floating Point(16:16:16:16 MSB-X:B:G:R)"}, [0xe] = {DRM_FORMAT_XBGR, 32, "32-bit RGBX (8:8:8:8 MSB-X:B:G:R)"}, }; @@ -75,6 +77,10 @@ static struct pixel_format skl_pixel_formats[] = { {DRM_FORMAT_XBGR2101010, 32, "32-bit RGBX (2:10:10:10 MSB-X:B:G:R)"}, {DRM_FORMAT_XRGB2101010, 32, "32-bit BGRX (2:10:10:10 MSB-X:R:G:B)"}, + {DRM_FORMAT_XRGB161616, 64, + "64-bit XRGB (16:16:16:16 MSB-X:R:G:B)"}, + {DRM_FORMAT_XBGR161616, 64, + "64-bit XBGR (16:16:16:16 MSB-X:B:G:R)"}, /* non-supported format has bpp default to 0 */ {0, 0, NULL}, @@ -101,6 +107,9 @@ static int skl_format_to_drm(int format, bool rgb_order, bool alpha, case PLANE_CTL_FORMAT_XRGB_2101010: skl_pixel_formats_index = rgb_order ? 10 : 11; break; + case PLANE_CTL_FORMAT_XRGB_16161616F: + skl_pixel_formats_index = rgb_order ? 12 : 13; + break; case PLANE_CTL_FORMAT_YUV422: skl_pixel_formats_index = yuv_order >> 16; if (skl_pixel_formats_index > 3) @@ -321,6 +330,8 @@ static struct pixel_format sprite_pixel_formats[SPRITE_FORMAT_NUM] = { [0x0] = {DRM_FORMAT_YUV422, 16, "YUV 16-bit 4:2:2 packed"}, [0x1] = {DRM_FORMAT_XRGB2101010, 32, "RGB 32-bit 2:10:10:10"}, [0x2] = {DRM_FORMAT_XRGB, 32, "RGB 32-bit 8:8:8:8"}, + [0x3] = {DRM_FORMAT_XRGB161616, 64, + "RGB 64-bit 16:16:16:16 Floating Point"}, [0x4] = {DRM_FORMAT_AYUV, 32, "YUV 32-bit 4:4:4 packed (8:8:8:8 MSB-X:Y:U:V)"}, }; -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v12 5/6] vfio: ABI for mdev display dma-buf operation
Add VFIO_DEVICE_QUERY_GFX_PLANE ioctl command to let user mode query and get the plan and its related information. The dma-buf's life cycle is handled by user mode and tracked by kernel. The returned fd in struct vfio_device_query_gfx_plane can be a new fd or an old fd of a re-exported dma-buf. Host User mode can check the value of fd and to see if it needs to create new resource according to the new fd or just use the existed resource related to the old fd. Signed-off-by: Tina Zhang --- include/uapi/linux/vfio.h | 28 1 file changed, 28 insertions(+) diff --git a/include/uapi/linux/vfio.h b/include/uapi/linux/vfio.h index ae46105..827a230 100644 --- a/include/uapi/linux/vfio.h +++ b/include/uapi/linux/vfio.h @@ -502,6 +502,34 @@ struct vfio_pci_hot_reset { #define VFIO_DEVICE_PCI_HOT_RESET _IO(VFIO_TYPE, VFIO_BASE + 13) +/** + * VFIO_DEVICE_QUERY_GFX_PLANE - _IOW(VFIO_TYPE, VFIO_BASE + 14, struct vfio_device_query_gfx_plane) + * + * Set the drm_plane_type and retrieve information about the gfx plane. + * + * Return: 0 on success, -errno on failure. + */ +struct vfio_device_gfx_plane_info { + __u32 argsz; + __u32 flags; + /* in */ + __u32 drm_plane_type; /* type of plane: DRM_PLANE_TYPE_* */ + /* out */ + __u32 drm_format; /* drm format of plane */ + __u64 drm_format_mod; /* tiled mode */ + __u32 width;/* width of plane */ + __u32 height; /* height of plane */ + __u32 stride; /* stride of plane */ + __u32 size; /* size of plane in bytes, align on page*/ + __u32 x_pos;/* horizontal position of cursor plane, upper left corner in pixels */ + __u32 y_pos;/* vertical position of cursor plane, upper left corner in lines*/ + __u32 region_index; + __s32 fd; /* dma-buf fd */ +}; + +#define VFIO_DEVICE_QUERY_GFX_PLANE _IO(VFIO_TYPE, VFIO_BASE + 14) + + /* API for Type1 VFIO IOMMU */ /** -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v12 4/6] drm/i915/gvt: add opregion support
Windows guest UPT driver can use operegion to configure the setting for display. Without the opregion support, the display registers won't be set and this blocks display model to get the correct information of the guest display plane. Signed-off-by: Bing Niu Signed-off-by: Xiaoguang Chen Signed-off-by: Tina Zhang --- drivers/gpu/drm/i915/gvt/hypercall.h | 1 + drivers/gpu/drm/i915/gvt/kvmgt.c | 109 ++- drivers/gpu/drm/i915/gvt/mpt.h | 15 + drivers/gpu/drm/i915/gvt/opregion.c | 26 +++-- drivers/gpu/drm/i915/gvt/vgpu.c | 4 ++ 5 files changed, 146 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/gvt/hypercall.h b/drivers/gpu/drm/i915/gvt/hypercall.h index df7f33a..32c345c 100644 --- a/drivers/gpu/drm/i915/gvt/hypercall.h +++ b/drivers/gpu/drm/i915/gvt/hypercall.h @@ -55,6 +55,7 @@ struct intel_gvt_mpt { unsigned long mfn, unsigned int nr, bool map); int (*set_trap_area)(unsigned long handle, u64 start, u64 end, bool map); + int (*set_opregion)(void *vgpu); }; extern struct intel_gvt_mpt xengt_mpt; diff --git a/drivers/gpu/drm/i915/gvt/kvmgt.c b/drivers/gpu/drm/i915/gvt/kvmgt.c index fd0c85f..6b0a330 100644 --- a/drivers/gpu/drm/i915/gvt/kvmgt.c +++ b/drivers/gpu/drm/i915/gvt/kvmgt.c @@ -53,11 +53,23 @@ static const struct intel_gvt_ops *intel_gvt_ops; #define VFIO_PCI_INDEX_TO_OFFSET(index) ((u64)(index) << VFIO_PCI_OFFSET_SHIFT) #define VFIO_PCI_OFFSET_MASK(((u64)(1) << VFIO_PCI_OFFSET_SHIFT) - 1) +#define OPREGION_SIGNATURE "IntelGraphicsMem" + +struct vfio_region; +struct intel_vgpu_regops { + size_t (*rw)(struct intel_vgpu *vgpu, char *buf, + size_t count, loff_t *ppos, bool iswrite); + void (*release)(struct intel_vgpu *vgpu, + struct vfio_region *region); +}; + struct vfio_region { u32 type; u32 subtype; size_t size; u32 flags; + const struct intel_vgpu_regops *ops; + void*data; }; struct kvmgt_pgfn { @@ -430,6 +442,91 @@ static void kvmgt_protect_table_del(struct kvmgt_guest_info *info, } } +static size_t intel_vgpu_reg_rw_opregion(struct intel_vgpu *vgpu, char *buf, + size_t count, loff_t *ppos, bool iswrite) +{ + unsigned int i = VFIO_PCI_OFFSET_TO_INDEX(*ppos) - + VFIO_PCI_NUM_REGIONS; + void *base = vgpu->vdev.region[i].data; + loff_t pos = *ppos & VFIO_PCI_OFFSET_MASK; + + if (pos >= vgpu->vdev.region[i].size || iswrite) { + gvt_vgpu_err("invalid op or offset for Intel vgpu OpRegion\n"); + return -EINVAL; + } + count = min(count, (size_t)(vgpu->vdev.region[i].size - pos)); + memcpy(buf, base + pos, count); + + return count; +} + +static void intel_vgpu_reg_release_opregion(struct intel_vgpu *vgpu, + struct vfio_region *region) +{ + memunmap(region->data); +} + +static const struct intel_vgpu_regops intel_vgpu_regops_opregion = { + .rw = intel_vgpu_reg_rw_opregion, + .release = intel_vgpu_reg_release_opregion, +}; + +static int intel_vgpu_register_reg(struct intel_vgpu *vgpu, + unsigned int type, unsigned int subtype, + const struct intel_vgpu_regops *ops, + size_t size, u32 flags, void *data) +{ + struct vfio_region *region; + + region = krealloc(vgpu->vdev.region, + (vgpu->vdev.num_regions + 1) * sizeof(*region), + GFP_KERNEL); + if (!region) + return -ENOMEM; + + vgpu->vdev.region = region; + vgpu->vdev.region[vgpu->vdev.num_regions].type = type; + vgpu->vdev.region[vgpu->vdev.num_regions].subtype = subtype; + vgpu->vdev.region[vgpu->vdev.num_regions].ops = ops; + vgpu->vdev.region[vgpu->vdev.num_regions].size = size; + vgpu->vdev.region[vgpu->vdev.num_regions].flags = flags; + vgpu->vdev.region[vgpu->vdev.num_regions].data = data; + vgpu->vdev.num_regions++; + + return 0; +} + +static int kvmgt_set_opregion(void *p_vgpu) +{ + struct intel_vgpu *vgpu = (struct intel_vgpu *)p_vgpu; + unsigned int addr; + void *base; + int ret; + + addr = vgpu->gvt->opregion.opregion_pa; + if (!addr || !(~addr)) + return -ENODEV; + + base = memremap(addr, OPREGION_SIZE, MEMREMAP_WB); + if (!base) + return -ENOMEM; + + if (memcmp(base, OPREGION_SIGNATURE, 16)) { + memunmap(base); + return -EINVAL; + } + + ret = intel_vgpu_register_reg(vgpu, + PCI_VENDOR_ID_INTEL | VFIO_REGION_TYPE_PCI_VENDOR_TYPE, + VFIO_REGION_SUBTYPE_INT
[Intel-gfx] [PATCH v12 6/6] drm/i915: Introduce GEM proxy
Signed-off-by: Tina Zhang --- drivers/gpu/drm/i915/i915_gem.c| 26 +- drivers/gpu/drm/i915/i915_gem_object.h | 9 + drivers/gpu/drm/i915/i915_gem_tiling.c | 5 + 3 files changed, 39 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 1b2dfa8..ebacc04 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -1612,6 +1612,12 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void *data, if (!obj) return -ENOENT; + /* proxy gem object does not support setting domain */ + if (i915_gem_object_is_proxy(obj)) { + err = -EPERM; + goto out; + } + /* Try to flush the object off the GPU without holding the lock. * We will repeat the flush holding the lock in the normal manner * to catch cases where we are gazumped. @@ -1680,6 +1686,12 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void *data, if (!obj) return -ENOENT; + /* proxy gem obj does not support this operation */ + if (i915_gem_object_is_proxy(obj)) { + i915_gem_object_put(obj); + return -EPERM; + } + /* Pinned buffers may be scanout, so flush the cache */ i915_gem_object_flush_if_display(obj); i915_gem_object_put(obj); @@ -1730,7 +1742,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, */ if (!obj->base.filp) { i915_gem_object_put(obj); - return -EINVAL; + return -EPERM; } addr = vm_mmap(obj->base.filp, 0, args->size, @@ -3764,6 +3776,12 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, void *data, if (!obj) return -ENOENT; + /* proxy gem obj does not support setting caching mode */ + if (i915_gem_object_is_proxy(obj)) { + ret = -EPERM; + goto out; + } + if (obj->cache_level == level) goto out; @@ -4210,6 +4228,12 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void *data, if (!obj) return -ENOENT; + /* proxy gem obj does not support changing backding storage */ + if (i915_gem_object_is_proxy(obj)) { + err = -EPERM; + goto out; + } + err = mutex_lock_interruptible(&obj->mm.lock); if (err) goto out; diff --git a/drivers/gpu/drm/i915/i915_gem_object.h b/drivers/gpu/drm/i915/i915_gem_object.h index 5b19a49..c91e336 100644 --- a/drivers/gpu/drm/i915/i915_gem_object.h +++ b/drivers/gpu/drm/i915/i915_gem_object.h @@ -39,6 +39,7 @@ struct drm_i915_gem_object_ops { unsigned int flags; #define I915_GEM_OBJECT_HAS_STRUCT_PAGE BIT(0) #define I915_GEM_OBJECT_IS_SHRINKABLE BIT(1) +#define I915_GEM_OBJECT_IS_PROXY BIT(2) /* Interface between the GEM object and its backing storage. * get_pages() is called once prior to the use of the associated set @@ -198,6 +199,8 @@ struct drm_i915_gem_object { } userptr; unsigned long scratch; + + void *gvt_info; }; /** for phys allocated objects */ @@ -300,6 +303,12 @@ i915_gem_object_is_shrinkable(const struct drm_i915_gem_object *obj) } static inline bool +i915_gem_object_is_proxy(const struct drm_i915_gem_object *obj) +{ + return obj->ops->flags & I915_GEM_OBJECT_IS_PROXY; +} + +static inline bool i915_gem_object_is_active(const struct drm_i915_gem_object *obj) { return obj->active_count; diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c b/drivers/gpu/drm/i915/i915_gem_tiling.c index fb5231f..21ec066 100644 --- a/drivers/gpu/drm/i915/i915_gem_tiling.c +++ b/drivers/gpu/drm/i915/i915_gem_tiling.c @@ -345,6 +345,11 @@ i915_gem_set_tiling_ioctl(struct drm_device *dev, void *data, if (!obj) return -ENOENT; + if (i915_gem_object_is_proxy(obj)) { + err = -EPERM; + goto err; + } + if (!i915_tiling_ok(obj, args->tiling_mode, args->stride)) { err = -EINVAL; goto err; -- 2.7.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 12/14] drm/i915: Interface for controling engine stats collection
Quoting Tvrtko Ursulin (2017-07-19 10:30:13) > > On 18/07/2017 16:22, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2017-07-18 15:36:16) > >> +u64 intel_engine_get_current_busy_ns(struct intel_engine_cs *engine) > >> +{ > >> + unsigned long flags; > >> + u64 total; > >> + > >> + spin_lock_irqsave(&engine->stats.lock, flags); > >> + > >> + total = engine->stats.total; > >> + > >> + /* > >> +* If the engine is executing something at the moment > >> +* add it to the total. > >> +*/ > >> + if (engine->stats.ref) > >> + total += ktime_get_real_ns() - engine->stats.start; > >> + > >> + spin_unlock_irqrestore(&engine->stats.lock, flags); > > > > Answers to another patch found here. I would say this is the other half > > of the interface and should be kept together. > > Yes, it was an ugly split. > > On 18/07/2017 16:43, Chris Wilson wrote: > > Quoting Tvrtko Ursulin (2017-07-18 15:36:16) > >> +int intel_enable_engine_stats(struct drm_i915_private *dev_priv) > >> +{ > >> + if (!i915.enable_execlists) > >> + return -ENODEV; > >> + > >> + mutex_lock(&i915_engine_stats_mutex); > >> + if (i915_engine_stats_ref++ == 0) { > >> + struct intel_engine_cs *engine; > >> + enum intel_engine_id id; > >> + > >> + for_each_engine(engine, dev_priv, id) { > >> + memset(&engine->stats, 0, sizeof(engine->stats)); > >> + spin_lock_init(&engine->stats.lock); > >> + } > >> + > >> + static_branch_enable(&i915_engine_stats_key); > >> + } > >> + mutex_unlock(&i915_engine_stats_mutex); > > > > I don't think static_branch_enable() is a might_sleep, so it looks like > > you can rewrite this avoiding the mutex and thus not requiring the > > worker and then can use the error code here to decide if you need to > > use the timer instead. > > Perhaps I could get rid of the mutex though by using atomic_inc/dec_return. > > But there is a mutex in jump label handling, Totally missed it. I wonder why it is a mutex, certainly serialising enable/disable need something. The comments suggest that once upon a time (or different arch?) it was much more of a stop_machine(). > so I think the workers have > to stay - and it is also beneficial to have delayed static branch disable, > since the perf core seems to like calling start/stop on the events a lot. > But it is recommended practice for static branches in any way. Interesting there is a static_key_slow_dec_deferred. But honestly I think it is hard to defend a global static_key for a per-device interface. > So from that angle I could perhaps even move the delayed disable to this > patch so it is automatically shared by all callers. > > >> +static DEFINE_MUTEX(i915_engine_stats_mutex); > >> +static int i915_engine_stats_ref; > >> > >> /* Haswell does have the CXT_SIZE register however it does not appear to > >> be > >>* valid. Now, docs explain in dwords what is in the context object. The > >> full > >> @@ -1340,6 +1342,57 @@ void intel_engines_mark_idle(struct > >> drm_i915_private *i915) > >> } > >> } > >> > >> +int intel_enable_engine_stats(struct drm_i915_private *dev_priv) > > > > The pattern I've been trying to use here is > > > > intel_engine_* - operate on the named engine > > > > intel_engines_* - operate on all engines > > Ok. > > > Long term though having a global static key is going to be a nasty wart. > > Joonas will definitely ask the question how much will it cost us to use > > an engine->bool and what we can do to minimise that cost. > > Why you think it is nasty? Sounds pretty cool to me. If we enable sampling on one device (engine even!), it affects another. But the device is the more compelling argument against. > But I think can re-organize the series to start with a normal branch and > then add the static one on top if so is desired. Ok. I like the idea of dynamically patching in branches, and hate to be party pooper! -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t v4 2/7] chamelium: Calculate CRC from framebuffer instead of hardcoding it
On Mon, 2017-07-17 at 12:29 -0400, Lyude Paul wrote: > On Wed, 2017-07-12 at 17:50 +0300, Paul Kocialkowski wrote: > > This introduces CRC calculation for reference frames, instead of > > using > > hardcoded values for them. The rendering of reference frames may > > differ > > from machine to machine, especially due to font rendering, and the > > frame itself may change with subsequent IGT changes. > > > > These differences would cause the CRC checks to fail on different > > setups. This allows them to pass regardless of the setup. > > Just one question before I push this since I didn't think of testing > this previously and don't have access to my chamelium at the moment. > Have you made sure that this doesn't break things with igt if a test > gets interrupted due to failure in the middle of an asynchronous CRC > calculation? So I have now tested that a failed assert (in the main thread) in the middle of the threaded calculation will not cause any segfault. There is no reason why it should, because none of the ressources used for the calculation are liberated prior to the end of the process. Everything is really cleaned up when the process dies. On the other hand, it may happen, due to the call to igt_get_cairo_surface that an igt_assert fails in the calculation thread, which definitely causes a segfault. I will rework the patches so that the call to the function is made prior to starting the thread, thus avoiding any possibility of hitting a failed igt_assert in the thread itself. > Other then that, everything here looks good. > > > > Signed-off-by: Paul Kocialkowski > > --- > > lib/igt_chamelium.c | 143 > > > > lib/igt_chamelium.h | 5 ++ > > tests/chamelium.c | 77 +++- > > 3 files changed, 167 insertions(+), 58 deletions(-) > > > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c > > index 93392af7..baa6399c 100644 > > --- a/lib/igt_chamelium.c > > +++ b/lib/igt_chamelium.c > > @@ -94,6 +94,14 @@ struct chamelium_frame_dump { > > struct chamelium_port *port; > > }; > > > > +struct chamelium_fb_crc_async_data { > > + int fd; > > + struct igt_fb *fb; > > + > > + pthread_t thread_id; > > + igt_crc_t *ret; > > +}; > > + > > struct chamelium { > > xmlrpc_env env; > > xmlrpc_client *client; > > @@ -998,6 +1006,141 @@ int chamelium_get_frame_limit(struct > > chamelium > > *chamelium, > > return ret; > > } > > > > +static uint32_t chamelium_xrgb_hash16(const unsigned char *buffer, > > int width, > > + int height, int k, int m) > > +{ > > + unsigned char r, g, b; > > + uint64_t sum = 0; > > + uint64_t count = 0; > > + uint64_t value; > > + uint32_t hash; > > + int index; > > + int i; > > + > > + for (i=0; i < width * height; i++) { > > + if ((i % m) != k) > > + continue; > > + > > + index = i * 4; > > + > > + r = buffer[index + 2]; > > + g = buffer[index + 1]; > > + b = buffer[index + 0]; > > + > > + value = r | (g << 8) | (b << 16); > > + sum += ++count * value; > > + } > > + > > + hash = ((sum >> 0) ^ (sum >> 16) ^ (sum >> 32) ^ (sum >> > > 48)) & 0x; > > + > > + return hash; > > +} > > + > > +static void chamelium_do_calculate_fb_crc(int fd, struct igt_fb > > *fb, > > + igt_crc_t *out) > > +{ > > + unsigned char *buffer; > > + cairo_surface_t *fb_surface; > > + int n = 4; > > + int w, h; > > + int i, j; > > + > > + /* Get the cairo surface for the framebuffer */ > > + fb_surface = igt_get_cairo_surface(fd, fb); > > + > > + buffer = cairo_image_surface_get_data(fb_surface); > > + w = fb->width; > > + h = fb->height; > > + > > + for (i = 0; i < n; i++) { > > + j = n - i - 1; > > + out->crc[i] = chamelium_xrgb_hash16(buffer, w, h, > > j, > > n); > > + } > > + > > + out->n_words = n; > > +} > > + > > +/** > > + * chamelium_calculate_fb_crc: > > + * @fd: The drm file descriptor > > + * @fb: The framebuffer to calculate the CRC for > > + * > > + * Calculates the CRC for the provided framebuffer, using the > > Chamelium's CRC > > + * algorithm. This calculates the CRC in a synchronous fashion. > > + * > > + * Returns: The calculated CRC > > + */ > > +igt_crc_t *chamelium_calculate_fb_crc(int fd, struct igt_fb *fb) > > +{ > > + igt_crc_t *ret = calloc(1, sizeof(igt_crc_t)); > > + > > + chamelium_do_calculate_fb_crc(fd, fb, ret); > > + > > + return ret; > > +} > > + > > +static void *chamelium_calculate_fb_crc_async_work(void *data) > > +{ > > + struct chamelium_fb_crc_async_data *fb_crc; > > + > > + fb_crc = (struct chamelium_fb_crc_async_data *) data; > > + > > + chamelium_do_calculate_fb_crc(fb_crc->fd, fb_crc->fb, > > fb_crc->ret); > > + > > + return NULL; > > +} > > + > > +/** > > + * chamelium_calculate_fb_crc_launch: > > + * @fd: The drm file d
Re: [Intel-gfx] [PATCH v2] drm/i915: Drain the device workqueue on unload
Chris Wilson writes: > Workers on the i915->wq may rearm themselves so for completeness we need > to replace our flush_workqueue() with a call to drain_workqueue() before > unloading the device. > > v2: Reinforce the drain_workqueue with an preceeding rcu_barrier() as a > few of the tasks that need to be drained may first be armed by RCU. > > References: https://bugs.freedesktop.org/show_bug.cgi?id=101627 > Signed-off-by: Chris Wilson > Cc: Matthew Auld > Cc: Mika Kuoppala > --- > drivers/gpu/drm/i915/i915_drv.c | 6 ++ > drivers/gpu/drm/i915/i915_drv.h | 20 > drivers/gpu/drm/i915/selftests/mock_gem_device.c | 2 +- > 3 files changed, 23 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 4b62fd012877..41c5b11a7c8f 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -596,7 +596,8 @@ static const struct vga_switcheroo_client_ops > i915_switcheroo_ops = { > > static void i915_gem_fini(struct drm_i915_private *dev_priv) > { > - flush_workqueue(dev_priv->wq); > + /* Flush any outstanding unpin_work. */ > + i915_gem_drain_workqueue(dev_priv); > > mutex_lock(&dev_priv->drm.struct_mutex); > intel_uc_fini_hw(dev_priv); > @@ -1409,9 +1410,6 @@ void i915_driver_unload(struct drm_device *dev) > cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); > i915_reset_error_state(dev_priv); > > - /* Flush any outstanding unpin_work. */ > - drain_workqueue(dev_priv->wq); > - > i915_gem_fini(dev_priv); > intel_uc_fini_fw(dev_priv); > intel_fbc_cleanup_cfb(dev_priv); > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 667fb5c44483..e9a4b96dc775 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3300,6 +3300,26 @@ static inline void i915_gem_drain_freed_objects(struct > drm_i915_private *i915) > } while (flush_work(&i915->mm.free_work)); > } > > +static inline void i915_gem_drain_workqueue(struct drm_i915_private *i915) > +{ > + /* > + * Similar to objects above (see i915_gem_drain_freed-objects), in > + * general we have workers that are armed by RCU and then rearm > + * themselves in their callbacks. To be paranoid, we need to > + * drain the workqueue a second time after waiting for the RCU > + * grace period so that we catch work queued via RCU from the first > + * pass. As neither drain_workqueue() nor flush_workqueue() report > + * a result, we make an assumption that we only don't require more > + * than 2 passes to catch all recursive RCU delayed work. > + * > + */ > + int pass = 2; > + do { > + rcu_barrier(); > + drain_workqueue(i915->wq); I am fine with the paranoia, and it covers the case below. Still if we do: drain_workqueue(); rcu_barrier(); With drawining in progress, only chain queuing is allowed. I understand this so that when it returns, all the ctx pointers are now unreferenced but not freed. Thus the rcu_barrier() after it cleans the trash and we are good to be unloaded. With one pass. I guess it comes to how to understand the comment, so could you elaborate the 'we have workers that are armed by RCU and then rearm themselves'?. As from drain_workqueue desc, this should be covered. Thanks, -Mika > + } while (--pass); > +} > + > struct i915_vma * __must_check > i915_gem_object_ggtt_pin(struct drm_i915_gem_object *obj, >const struct i915_ggtt_view *view, > diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > index 47613d20bba8..7a468cb30946 100644 > --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c > +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c > @@ -57,7 +57,7 @@ static void mock_device_release(struct drm_device *dev) > > cancel_delayed_work_sync(&i915->gt.retire_work); > cancel_delayed_work_sync(&i915->gt.idle_work); > - flush_workqueue(i915->wq); > + i915_gem_drain_workqueue(i915); > > mutex_lock(&i915->drm.struct_mutex); > for_each_engine(engine, i915, id) > -- > 2.13.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v12 6/6] drm/i915: Introduce GEM proxy
s/GEM proxy/a GEM proxy object/ Quoting Tina Zhang (2017-07-19 11:59:05) Rationale goes here. > Signed-off-by: Tina Zhang > --- > drivers/gpu/drm/i915/i915_gem.c| 26 +- > drivers/gpu/drm/i915/i915_gem_object.h | 9 + > drivers/gpu/drm/i915/i915_gem_tiling.c | 5 + > 3 files changed, 39 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c > index 1b2dfa8..ebacc04 100644 > --- a/drivers/gpu/drm/i915/i915_gem.c > +++ b/drivers/gpu/drm/i915/i915_gem.c > @@ -1612,6 +1612,12 @@ i915_gem_set_domain_ioctl(struct drm_device *dev, void > *data, > if (!obj) > return -ENOENT; > > + /* proxy gem object does not support setting domain */ Yes, this is what the code is doing. The comment tells us why. /* * Proxy objects do not control access to the backing storage, ergo * they cannot be used as a means to manipulate the cache domain * tracking for that backing storage. The proxy object is always * considered to be outside of any cache domain. */ However, set-domain does have some other side-effects that includes waiting which should still be performed, i.e. this check should be after the lockless wait. > + if (i915_gem_object_is_proxy(obj)) { > + err = -EPERM; > + goto out; > + } > + > /* Try to flush the object off the GPU without holding the lock. > * We will repeat the flush holding the lock in the normal manner > * to catch cases where we are gazumped. > @@ -1680,6 +1686,12 @@ i915_gem_sw_finish_ioctl(struct drm_device *dev, void > *data, > if (!obj) > return -ENOENT; > A comment could explain that since proxy objects are barred from CPU access, we do not need to ban sw_finish as it is a nop. > + /* proxy gem obj does not support this operation */ > + if (i915_gem_object_is_proxy(obj)) { > + i915_gem_object_put(obj); > + return -EPERM; > + } > + > /* Pinned buffers may be scanout, so flush the cache */ > i915_gem_object_flush_if_display(obj); > i915_gem_object_put(obj); > @@ -1730,7 +1742,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data, > */ > if (!obj->base.filp) { > i915_gem_object_put(obj); > - return -EINVAL; > + return -EPERM; > } > > addr = vm_mmap(obj->base.filp, 0, args->size, > @@ -3764,6 +3776,12 @@ int i915_gem_set_caching_ioctl(struct drm_device *dev, > void *data, > if (!obj) > return -ENOENT; > > + /* proxy gem obj does not support setting caching mode */ More rationale. Also is the proxied object (its target) also banned from changing the PTE bits or do we inherit all such changes automatically? > + if (i915_gem_object_is_proxy(obj)) { > + ret = -EPERM; > + goto out; > + } > + > if (obj->cache_level == level) > goto out; > > @@ -4210,6 +4228,12 @@ i915_gem_madvise_ioctl(struct drm_device *dev, void > *data, > if (!obj) > return -ENOENT; > > + /* proxy gem obj does not support changing backding storage */ This one could be generalised to I915_GEM_OBJECT_IS_SHRINKABLE? > + if (i915_gem_object_is_proxy(obj)) { > + err = -EPERM; > + goto out; > + } > + > err = mutex_lock_interruptible(&obj->mm.lock); > if (err) > goto out; > diff --git a/drivers/gpu/drm/i915/i915_gem_object.h > b/drivers/gpu/drm/i915/i915_gem_object.h > index 5b19a49..c91e336 100644 > --- a/drivers/gpu/drm/i915/i915_gem_object.h > +++ b/drivers/gpu/drm/i915/i915_gem_object.h > @@ -39,6 +39,7 @@ struct drm_i915_gem_object_ops { > unsigned int flags; > #define I915_GEM_OBJECT_HAS_STRUCT_PAGE BIT(0) > #define I915_GEM_OBJECT_IS_SHRINKABLE BIT(1) > +#define I915_GEM_OBJECT_IS_PROXY BIT(2) > > /* Interface between the GEM object and its backing storage. > * get_pages() is called once prior to the use of the associated set > @@ -198,6 +199,8 @@ struct drm_i915_gem_object { > } userptr; > > unsigned long scratch; > + > + void *gvt_info; Unrelated chunk, this should go into the gvt patch to use the object. > }; > > /** for phys allocated objects */ > @@ -300,6 +303,12 @@ i915_gem_object_is_shrinkable(const struct > drm_i915_gem_object *obj) > } > > static inline bool > +i915_gem_object_is_proxy(const struct drm_i915_gem_object *obj) > +{ > + return obj->ops->flags & I915_GEM_OBJECT_IS_PROXY; > +} > + > +static inline bool > i915_gem_object_is_active(const struct drm_i915_gem_object *obj) > { > return obj->active_count; > diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c > b/drivers/gpu/drm/i915/i915_gem_tiling.c >
[Intel-gfx] [intel-gfx][i-g-t][help] gem_rungfill skiped
Hi~ all, There is one case will skip and prints "No known gpu found". There are some test cases will check on this, but they have not skiped. I think there may be something wrong. I run the test by command: ./igt/tests/gem_ringfill --run-subtest basic-default-interruptible ./igt/tests/gem_ringfill --run-subtest basic-default-forked" ./igt/tests/gem_ringfill --run-subtest basic-default-interruptible", And the out put shows below IGT-Version: 1.19-g4258cc8e (x86_64) (Linux: 4.9.0 x86_64) Test requirement not met in function drm_open_driver, file drmtest.c:359: Test requirement: !(fd<0) No known gpu found Last errno: 2, No such file or directory Subtest basic-default: SKIP I know It will call "fd=drm_open_driver(DRIVER_INTEL);" before the test program. But it doesn't skip here in other test cases. And here is the cpu info. A total of 8 processors - vendor_id : GenuineIntel cpu family : 6 model : 60 model name : Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz stepping: 3 microcode : 0x16 cpu MHz : 3392.130 cache size : 8192 KB physical id : 0 siblings: 8 core id : 3 cpu cores : 4 apicid : 7 initial apicid : 7 fpu : yes fpu_exception : yes cpuid level : 13 wp : yes --- Thank you Gu ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Drain the device workqueue on unload
Quoting Mika Kuoppala (2017-07-19 12:18:47) > Chris Wilson writes: > > > Workers on the i915->wq may rearm themselves so for completeness we need > > to replace our flush_workqueue() with a call to drain_workqueue() before > > unloading the device. > > > > v2: Reinforce the drain_workqueue with an preceeding rcu_barrier() as a > > few of the tasks that need to be drained may first be armed by RCU. > > > > References: https://bugs.freedesktop.org/show_bug.cgi?id=101627 > > Signed-off-by: Chris Wilson > > Cc: Matthew Auld > > Cc: Mika Kuoppala > > --- > > drivers/gpu/drm/i915/i915_drv.c | 6 ++ > > drivers/gpu/drm/i915/i915_drv.h | 20 > > drivers/gpu/drm/i915/selftests/mock_gem_device.c | 2 +- > > 3 files changed, 23 insertions(+), 5 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index 4b62fd012877..41c5b11a7c8f 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -596,7 +596,8 @@ static const struct vga_switcheroo_client_ops > > i915_switcheroo_ops = { > > > > static void i915_gem_fini(struct drm_i915_private *dev_priv) > > { > > - flush_workqueue(dev_priv->wq); > > + /* Flush any outstanding unpin_work. */ > > + i915_gem_drain_workqueue(dev_priv); > > > > mutex_lock(&dev_priv->drm.struct_mutex); > > intel_uc_fini_hw(dev_priv); > > @@ -1409,9 +1410,6 @@ void i915_driver_unload(struct drm_device *dev) > > cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); > > i915_reset_error_state(dev_priv); > > > > - /* Flush any outstanding unpin_work. */ > > - drain_workqueue(dev_priv->wq); > > - > > i915_gem_fini(dev_priv); > > intel_uc_fini_fw(dev_priv); > > intel_fbc_cleanup_cfb(dev_priv); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 667fb5c44483..e9a4b96dc775 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -3300,6 +3300,26 @@ static inline void > > i915_gem_drain_freed_objects(struct drm_i915_private *i915) > > } while (flush_work(&i915->mm.free_work)); > > } > > > > +static inline void i915_gem_drain_workqueue(struct drm_i915_private *i915) > > +{ > > + /* > > + * Similar to objects above (see i915_gem_drain_freed-objects), in > > + * general we have workers that are armed by RCU and then rearm > > + * themselves in their callbacks. To be paranoid, we need to > > + * drain the workqueue a second time after waiting for the RCU > > + * grace period so that we catch work queued via RCU from the first > > + * pass. As neither drain_workqueue() nor flush_workqueue() report > > + * a result, we make an assumption that we only don't require more > > + * than 2 passes to catch all recursive RCU delayed work. > > + * > > + */ > > + int pass = 2; > > + do { > > + rcu_barrier(); > > + drain_workqueue(i915->wq); > > I am fine with the paranoia, and it covers the case below. Still if we do: > > drain_workqueue(); > rcu_barrier(); > > With drawining in progress, only chain queuing is allowed. I understand > this so that when it returns, all the ctx pointers are now unreferenced > but not freed. > > Thus the rcu_barrier() after it cleans the trash and we are good to > be unloaded. With one pass. > > I guess it comes to how to understand the comment, so could you > elaborate the 'we have workers that are armed by RCU and then rearm > themselves'?. As from drain_workqueue desc, this should be covered. I'm considering that they may be rearmed via RCU in the general case, e.g. context close frees an object and so goes onto an RCU list that once processed kicks off a new worker and so requires another round of drain_workqueue. We are in module unload so a few extra delays to belts and braces are ok until somebody notices it takes a few minutes to run a reload test ;) -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [intel-gfx][i-g-t] test kms_frontbuffer_tracking basic fail
Quoting Gu, HailinX (2017-07-19 11:58:37) > Hi Arek > > This time it looks like a bug > > I run this test on i7-4770, by command `./igt/tests/kms_frontbuffer_tracking > --run-subtest basic` > > I guess it may be caused by I have no monitor connected. But if that is the > case it should be skiped rather than fail. It's because your kernel doesn't have a bugfix for WC domains and the test isn't checking that it can use mmap_wc first. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: More stolen quirking
== Series Details == Series: drm/i915: More stolen quirking URL : https://patchwork.freedesktop.org/series/27561/ State : success == Summary == Series 27561v1 drm/i915: More stolen quirking https://patchwork.freedesktop.org/api/1.0/series/27561/revisions/1/mbox/ Test kms_flip: Subgroup basic-flip-vs-modeset: skip -> PASS (fi-skl-x1585l) fdo#101781 Test kms_pipe_crc_basic: Subgroup suspend-read-crc-pipe-b: pass -> DMESG-WARN (fi-byt-j1900) fdo#101705 Test drv_module_reload: Subgroup basic-reload: dmesg-warn -> PASS (fi-skl-6700k) fdo#98683 +3 fdo#101781 https://bugs.freedesktop.org/show_bug.cgi?id=101781 fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705 fdo#98683 https://bugs.freedesktop.org/show_bug.cgi?id=98683 fi-bdw-5557u total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:445s fi-bdw-gvtdvmtotal:279 pass:265 dwarn:0 dfail:0 fail:0 skip:14 time:429s fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:360s fi-bsw-n3050 total:279 pass:243 dwarn:0 dfail:0 fail:0 skip:36 time:541s fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:509s fi-byt-j1900 total:279 pass:254 dwarn:1 dfail:0 fail:0 skip:24 time:500s fi-byt-n2820 total:279 pass:250 dwarn:1 dfail:0 fail:0 skip:28 time:491s fi-glk-2atotal:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:604s fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:439s fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:415s fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:419s fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:505s fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:486s fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:463s fi-kbl-7560u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:574s fi-kbl-r total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:586s fi-pnv-d510 total:279 pass:222 dwarn:2 dfail:0 fail:0 skip:55 time:560s fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:457s fi-skl-6700hqtotal:279 pass:262 dwarn:0 dfail:0 fail:0 skip:17 time:591s fi-skl-6700k total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:466s fi-skl-6770hqtotal:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:480s fi-skl-gvtdvmtotal:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:436s fi-skl-x1585ltotal:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:502s fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:543s fi-snb-2600 total:279 pass:250 dwarn:0 dfail:0 fail:0 skip:29 time:403s 541e75cb2ac9f85388e9e6f228d98f266506eebd drm-tip: 2017y-07m-19d-10h-10m-36s UTC integration manifest 59995ab drm/i915: More stolen quirking == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5231/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [intel-gfx][i-g-t][help] gem_rungfill skiped
Quoting Gu, HailinX (2017-07-19 12:28:25) > Hi~ all, > > > > There is one case will skip and prints “No known gpu found”. There are some > test cases will check on this, but they have not skiped. The test uses the vgem module to measure the ring size on your machine. The error message is still unhelpful. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915: Use AUX for backlight only if eDP 1.4 or later
== Series Details == Series: drm/i915: Use AUX for backlight only if eDP 1.4 or later URL : https://patchwork.freedesktop.org/series/27566/ State : success == Summary == Series 27566v1 drm/i915: Use AUX for backlight only if eDP 1.4 or later https://patchwork.freedesktop.org/api/1.0/series/27566/revisions/1/mbox/ Test kms_pipe_crc_basic: Subgroup hang-read-crc-pipe-a: dmesg-warn -> PASS (fi-pnv-d510) fdo#101597 Subgroup suspend-read-crc-pipe-b: pass -> DMESG-WARN (fi-byt-j1900) fdo#101705 fdo#101597 https://bugs.freedesktop.org/show_bug.cgi?id=101597 fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705 fi-bdw-5557u total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:441s fi-bdw-gvtdvmtotal:279 pass:265 dwarn:0 dfail:0 fail:0 skip:14 time:429s fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:358s fi-bsw-n3050 total:279 pass:243 dwarn:0 dfail:0 fail:0 skip:36 time:546s fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:514s fi-byt-j1900 total:279 pass:254 dwarn:1 dfail:0 fail:0 skip:24 time:496s fi-byt-n2820 total:279 pass:250 dwarn:1 dfail:0 fail:0 skip:28 time:487s fi-glk-2atotal:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:600s fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:437s fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:415s fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:418s fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:501s fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:471s fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:469s fi-kbl-7560u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:577s fi-kbl-r total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:578s fi-pnv-d510 total:279 pass:223 dwarn:1 dfail:0 fail:0 skip:55 time:558s fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:461s fi-skl-6700hqtotal:279 pass:262 dwarn:0 dfail:0 fail:0 skip:17 time:584s fi-skl-6700k total:279 pass:257 dwarn:4 dfail:0 fail:0 skip:18 time:471s fi-skl-6770hqtotal:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:474s fi-skl-gvtdvmtotal:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:434s fi-skl-x1585ltotal:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:471s fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:540s fi-snb-2600 total:279 pass:250 dwarn:0 dfail:0 fail:0 skip:29 time:403s 541e75cb2ac9f85388e9e6f228d98f266506eebd drm-tip: 2017y-07m-19d-10h-10m-36s UTC integration manifest b10d4f6 drm/i915: Use AUX for backlight only if eDP 1.4 or later == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5232/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Drain the device workqueue on unload
Chris Wilson writes: > Quoting Mika Kuoppala (2017-07-19 12:18:47) >> Chris Wilson writes: >> >> > Workers on the i915->wq may rearm themselves so for completeness we need >> > to replace our flush_workqueue() with a call to drain_workqueue() before >> > unloading the device. >> > >> > v2: Reinforce the drain_workqueue with an preceeding rcu_barrier() as a >> > few of the tasks that need to be drained may first be armed by RCU. >> > >> > References: https://bugs.freedesktop.org/show_bug.cgi?id=101627 >> > Signed-off-by: Chris Wilson >> > Cc: Matthew Auld >> > Cc: Mika Kuoppala >> > --- >> > drivers/gpu/drm/i915/i915_drv.c | 6 ++ >> > drivers/gpu/drm/i915/i915_drv.h | 20 >> > drivers/gpu/drm/i915/selftests/mock_gem_device.c | 2 +- >> > 3 files changed, 23 insertions(+), 5 deletions(-) >> > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.c >> > b/drivers/gpu/drm/i915/i915_drv.c >> > index 4b62fd012877..41c5b11a7c8f 100644 >> > --- a/drivers/gpu/drm/i915/i915_drv.c >> > +++ b/drivers/gpu/drm/i915/i915_drv.c >> > @@ -596,7 +596,8 @@ static const struct vga_switcheroo_client_ops >> > i915_switcheroo_ops = { >> > >> > static void i915_gem_fini(struct drm_i915_private *dev_priv) >> > { >> > - flush_workqueue(dev_priv->wq); >> > + /* Flush any outstanding unpin_work. */ >> > + i915_gem_drain_workqueue(dev_priv); >> > >> > mutex_lock(&dev_priv->drm.struct_mutex); >> > intel_uc_fini_hw(dev_priv); >> > @@ -1409,9 +1410,6 @@ void i915_driver_unload(struct drm_device *dev) >> > cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); >> > i915_reset_error_state(dev_priv); >> > >> > - /* Flush any outstanding unpin_work. */ >> > - drain_workqueue(dev_priv->wq); >> > - >> > i915_gem_fini(dev_priv); >> > intel_uc_fini_fw(dev_priv); >> > intel_fbc_cleanup_cfb(dev_priv); >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h >> > b/drivers/gpu/drm/i915/i915_drv.h >> > index 667fb5c44483..e9a4b96dc775 100644 >> > --- a/drivers/gpu/drm/i915/i915_drv.h >> > +++ b/drivers/gpu/drm/i915/i915_drv.h >> > @@ -3300,6 +3300,26 @@ static inline void >> > i915_gem_drain_freed_objects(struct drm_i915_private *i915) >> > } while (flush_work(&i915->mm.free_work)); >> > } >> > >> > +static inline void i915_gem_drain_workqueue(struct drm_i915_private *i915) >> > +{ >> > + /* >> > + * Similar to objects above (see i915_gem_drain_freed-objects), in >> > + * general we have workers that are armed by RCU and then rearm >> > + * themselves in their callbacks. To be paranoid, we need to >> > + * drain the workqueue a second time after waiting for the RCU >> > + * grace period so that we catch work queued via RCU from the first >> > + * pass. As neither drain_workqueue() nor flush_workqueue() report >> > + * a result, we make an assumption that we only don't require more >> > + * than 2 passes to catch all recursive RCU delayed work. >> > + * >> > + */ >> > + int pass = 2; >> > + do { >> > + rcu_barrier(); >> > + drain_workqueue(i915->wq); >> >> I am fine with the paranoia, and it covers the case below. Still if we do: >> >> drain_workqueue(); >> rcu_barrier(); >> >> With drawining in progress, only chain queuing is allowed. I understand >> this so that when it returns, all the ctx pointers are now unreferenced >> but not freed. >> >> Thus the rcu_barrier() after it cleans the trash and we are good to >> be unloaded. With one pass. >> >> I guess it comes to how to understand the comment, so could you >> elaborate the 'we have workers that are armed by RCU and then rearm >> themselves'?. As from drain_workqueue desc, this should be covered. > > I'm considering that they may be rearmed via RCU in the general case, > e.g. context close frees an object and so goes onto an RCU list that > once processed kicks off a new worker and so requires another round of > drain_workqueue. We are in module unload so a few extra delays to belts > and braces are ok until somebody notices it takes a few minutes to run a > reload test ;) Ok. Patch is Reviewed-by: Mika Kuoppala > -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v10] vfio: ABI for mdev display dma-buf operation
On 7/19/2017 11:55 AM, Gerd Hoffmann wrote: > On Wed, 2017-07-19 at 00:16 +, Zhang, Tina wrote: >>> -Original Message- >>> From: Gerd Hoffmann [mailto:kra...@redhat.com] >>> Sent: Monday, July 17, 2017 7:03 PM >>> To: Kirti Wankhede ; Zhang, Tina >>> ; Tian, Kevin ; linux- >>> ker...@vger.kernel.org; intel-gfx@lists.freedesktop.org; >>> alex.william...@redhat.com; zhen...@linux.intel.com; chris@chris- >>> wilson.co.uk; Lv, Zhiyuan ; intel-gvt- >>> d...@lists.freedesktop.org; Wang, Zhi A >>> Subject: Re: [PATCH v10] vfio: ABI for mdev display dma-buf >>> operation >>> >>> Hi, >>> No need of flag here. If vGPU driver is not loaded in the guest, there is no surface being managed by vGPU, in that case this size will be zero. >>> >>> Ok, we certainly have the same situation with intel. When the >>> guest driver is not >>> loaded (yet) there is no valid surface. >>> >>> We should cleanly define what the ioctl should do in that case, so >>> all drivers >>> behave the same way. >>> >>> I'd suggest that all fields defining the surface (drm_format, >>> width, height, stride, >>> size) should be set to zero in that case. >> >> Yeah, it's reasonable. How about the return value? Currently, the >> ioctl also returns "-ENODEV" in that situation. > > I think it should not return an error. Querying the plane parameters > worked fine. > Sounds good to me too. Thanks, Kirti ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Fix bad comparison in skl_compute_plane_wm.
Op 17-07-17 om 14:06 schreef Mahesh Kumar: > Reviewed-by: Mahesh Kumar Thanks, fix pushed. :) > On Monday 17 July 2017 04:43 PM, Maarten Lankhorst wrote: >> ddb_allocation && ddb_allocation / blocks_per_line >= 1 is the same >> as ddb_allocation >= blocks_per_line, so use the latter to simplify >> this. >> >> This fixes the following compiler warning: >> >> drivers/gpu/drm/i915/intel_pm.c:4467]: (warning) Comparison of a >> boolean expression with an integer other than 0 or 1. >> >> Signed-off-by: Maarten Lankhorst >> Fixes: d555cb5827d6 ("drm/i915/skl+: use linetime latency if ddb size is not >> available") >> Cc: "Mahesh Kumar" >> Reported-by: David Binderman >> Cc: David Binderman >> Cc: # v4.13-rc1+ >> --- >> drivers/gpu/drm/i915/intel_pm.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_pm.c >> b/drivers/gpu/drm/i915/intel_pm.c >> index 78b9acfc64c0..b9b3d8d45016 100644 >> --- a/drivers/gpu/drm/i915/intel_pm.c >> +++ b/drivers/gpu/drm/i915/intel_pm.c >> @@ -4681,8 +4681,8 @@ static int skl_compute_plane_wm(const struct >> drm_i915_private *dev_priv, >> if ((cpp * cstate->base.adjusted_mode.crtc_htotal / 512 < 1) && >> (plane_bytes_per_line / 512 < 1)) >> selected_result = method2; >> -else if ((ddb_allocation && ddb_allocation / >> -fixed_16_16_to_u32_round_up(plane_blocks_per_line)) >= 1) >> +else if (ddb_allocation >= >> + fixed_16_16_to_u32_round_up(plane_blocks_per_line)) >> selected_result = min_fixed_16_16(method1, method2); >> else if (latency >= linetime_us) >> selected_result = min_fixed_16_16(method1, method2); > ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: More stolen quirking
On Wed, Jul 19, 2017 at 01:05:45PM +0300, Martin Peres wrote: > On 19/07/17 13:00, Daniel Vetter wrote: > > I've found a bios with an off-by-one at the other end. There's a pnp > > reservation for 0xc540-0xc7fe and we want stolen in 0xc600 > > through 0xc800. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=99872 > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=98683 > > Cc: Martin Peres > > Signed-off-by: Daniel Vetter > > Looks good, and it will reduce the noise in CI. Thanks! Well, since this happens always it simply reduced coverage on that machine. Not being sensitive for dmesg noise for the module reload testcase makes that one rather useless. > > Reviewed-by: Martin Peres Thanks for your review, merged. -Daniel > > > --- > > drivers/gpu/drm/i915/i915_gem_stolen.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_stolen.c > > b/drivers/gpu/drm/i915/i915_gem_stolen.c > > index a817b3e0b17e..c11c915382e7 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_stolen.c > > +++ b/drivers/gpu/drm/i915/i915_gem_stolen.c > > @@ -254,9 +254,10 @@ static dma_addr_t i915_stolen_to_dma(struct > > drm_i915_private *dev_priv) > > * This is a BIOS w/a: Some BIOS wrap stolen in the root > > * PCI bus, but have an off-by-one error. Hence retry the > > * reservation starting from 1 instead of 0. > > +* There's also BIOS with off-by-one on the other end. > > */ > > r = devm_request_mem_region(dev_priv->drm.dev, base + 1, > > - ggtt->stolen_size - 1, > > + ggtt->stolen_size - 2, > > "Graphics Stolen Memory"); > > /* > > * GEN3 firmware likes to smash pci bridges into the stolen > > -- 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] [RFC 00/14] i915 PMU and engine busy stats
Quoting Tvrtko Ursulin (2017-07-18 15:36:04) > From: Tvrtko Ursulin > > Rough sketch of the idea I mentioned a few times to various people - merging > the engine busyness tracking with Chris i915 PMU RFC. > > First patch is the actual PMU RFC by Chris. It is followed by some cleanup > patches, then come a few improvements, cheap execlists engine busyness > tracking, > debugfs view for the same, and finally the i915 PMU is extended to use this > instead of timer based mmio sampling. > > This makes it cheaper and also more accurate since engine busyness is not > derived via sampling. > > But I haven't figure out the perf API yet. For example is it possible to > access > our events in an usable fashion via perf top/stat or something? Do we want to > make the events discoverable as I did (patch 8). In my dreams I have gpu activity in the same perf timechart as gpu activity. But that can be mostly by the request tracepoints, but still overlaying cpu/gpu activity is desirable and more importantly we want to coordinate with nouveau/amdgpu so that such interfaces are as agnostic as possible. There are definitely a bunch of global features in common for all (engine enumeration & activity, mempool enumeration, size & activty, power usage?). But the key question is how do we build for the future? Split the event id range into common/driver? > I could not find much (any?) kernel API level documentation for perf. There isn't much indeed. Given that we now have a second pair of eyes go over the sampling and improve its interaction with i915, we should start getting PeterZ involved to check the interaction with perf. > Btw patch series actually works since intel-gpu-overlay can use these events > when they are available. > > Chris Wilson (1): > RFC drm/i915: Expose a PMU interface for perf queries One thing I would like is for any future interface (including this engine/class/event id) to use the engine class/instance mapping. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/gvt: Dma-buf support for GVT-g
== Series Details == Series: drm/i915/gvt: Dma-buf support for GVT-g URL : https://patchwork.freedesktop.org/series/27572/ State : success == Summary == Series 27572v1 drm/i915/gvt: Dma-buf support for GVT-g https://patchwork.freedesktop.org/api/1.0/series/27572/revisions/1/mbox/ Test gem_exec_flush: Subgroup basic-batch-kernel-default-uc: pass -> FAIL (fi-snb-2600) fdo#17 Test kms_pipe_crc_basic: Subgroup suspend-read-crc-pipe-b: pass -> DMESG-WARN (fi-byt-j1900) fdo#101705 fdo#17 https://bugs.freedesktop.org/show_bug.cgi?id=17 fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705 fi-bdw-5557u total:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:453s fi-bdw-gvtdvmtotal:279 pass:265 dwarn:0 dfail:0 fail:0 skip:14 time:438s fi-blb-e6850 total:279 pass:224 dwarn:1 dfail:0 fail:0 skip:54 time:356s fi-bsw-n3050 total:279 pass:243 dwarn:0 dfail:0 fail:0 skip:36 time:545s fi-bxt-j4205 total:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:512s fi-byt-j1900 total:279 pass:254 dwarn:1 dfail:0 fail:0 skip:24 time:496s fi-byt-n2820 total:279 pass:250 dwarn:1 dfail:0 fail:0 skip:28 time:485s fi-glk-2atotal:279 pass:260 dwarn:0 dfail:0 fail:0 skip:19 time:606s fi-hsw-4770 total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:439s fi-hsw-4770r total:279 pass:263 dwarn:0 dfail:0 fail:0 skip:16 time:417s fi-ilk-650 total:279 pass:229 dwarn:0 dfail:0 fail:0 skip:50 time:417s fi-ivb-3520m total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:496s fi-ivb-3770 total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:472s fi-kbl-7500u total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:469s fi-kbl-7560u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:576s fi-kbl-r total:279 pass:261 dwarn:0 dfail:0 fail:0 skip:18 time:590s fi-pnv-d510 total:279 pass:222 dwarn:2 dfail:0 fail:0 skip:55 time:560s fi-skl-6260u total:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:463s fi-skl-6700hqtotal:279 pass:262 dwarn:0 dfail:0 fail:0 skip:17 time:582s fi-skl-6700k total:279 pass:257 dwarn:4 dfail:0 fail:0 skip:18 time:472s fi-skl-6770hqtotal:279 pass:269 dwarn:0 dfail:0 fail:0 skip:10 time:474s fi-skl-gvtdvmtotal:279 pass:266 dwarn:0 dfail:0 fail:0 skip:13 time:433s fi-skl-x1585ltotal:279 pass:268 dwarn:0 dfail:0 fail:0 skip:11 time:474s fi-snb-2520m total:279 pass:251 dwarn:0 dfail:0 fail:0 skip:28 time:546s fi-snb-2600 total:279 pass:249 dwarn:0 dfail:0 fail:1 skip:29 time:404s 541e75cb2ac9f85388e9e6f228d98f266506eebd drm-tip: 2017y-07m-19d-10h-10m-36s UTC integration manifest 2cd6b00 drm/i915: Introduce GEM proxy 6ca8e14 vfio: ABI for mdev display dma-buf operation fd3a206 drm/i915/gvt: add opregion support 11cb619 drm/i915/gvt: Add RGB 64-bit 16:16:16:16 float format support 38501de drm: Introduce RGB 64-bit 16:16:16:16 float format 879bb65 drm/i915/gvt: Add framebuffer decoder support == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5233/ ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/selftests: Attach a stub pm_domain
Quoting Matthew Auld (2017-07-19 11:51:51) > On 18 July 2017 at 18:30, Chris Wilson wrote: > > Supply a pm_domain and its ops for our mock GEM device so that > > device runtime pm doesn't complain even though we only want to mark it > > permanently active! > > > > Signed-off-by: Chris Wilson > Fixes it for me, so: > > Tested-by: Matthew Auld > Reviewed-by: Matthew Auld Took far too long to find a solution! Thanks, pushed. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Drain the device workqueue on unload
Quoting Mika Kuoppala (2017-07-19 12:51:04) > Chris Wilson writes: > > > Quoting Mika Kuoppala (2017-07-19 12:18:47) > >> Chris Wilson writes: > >> > >> > Workers on the i915->wq may rearm themselves so for completeness we need > >> > to replace our flush_workqueue() with a call to drain_workqueue() before > >> > unloading the device. > >> > > >> > v2: Reinforce the drain_workqueue with an preceeding rcu_barrier() as a > >> > few of the tasks that need to be drained may first be armed by RCU. > >> > > >> > References: https://bugs.freedesktop.org/show_bug.cgi?id=101627 > >> > Signed-off-by: Chris Wilson > >> > Cc: Matthew Auld > >> > Cc: Mika Kuoppala > >> > --- > >> > drivers/gpu/drm/i915/i915_drv.c | 6 ++ > >> > drivers/gpu/drm/i915/i915_drv.h | 20 > >> > > >> > drivers/gpu/drm/i915/selftests/mock_gem_device.c | 2 +- > >> > 3 files changed, 23 insertions(+), 5 deletions(-) > >> > > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.c > >> > b/drivers/gpu/drm/i915/i915_drv.c > >> > index 4b62fd012877..41c5b11a7c8f 100644 > >> > --- a/drivers/gpu/drm/i915/i915_drv.c > >> > +++ b/drivers/gpu/drm/i915/i915_drv.c > >> > @@ -596,7 +596,8 @@ static const struct vga_switcheroo_client_ops > >> > i915_switcheroo_ops = { > >> > > >> > static void i915_gem_fini(struct drm_i915_private *dev_priv) > >> > { > >> > - flush_workqueue(dev_priv->wq); > >> > + /* Flush any outstanding unpin_work. */ > >> > + i915_gem_drain_workqueue(dev_priv); > >> > > >> > mutex_lock(&dev_priv->drm.struct_mutex); > >> > intel_uc_fini_hw(dev_priv); > >> > @@ -1409,9 +1410,6 @@ void i915_driver_unload(struct drm_device *dev) > >> > cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work); > >> > i915_reset_error_state(dev_priv); > >> > > >> > - /* Flush any outstanding unpin_work. */ > >> > - drain_workqueue(dev_priv->wq); > >> > - > >> > i915_gem_fini(dev_priv); > >> > intel_uc_fini_fw(dev_priv); > >> > intel_fbc_cleanup_cfb(dev_priv); > >> > diff --git a/drivers/gpu/drm/i915/i915_drv.h > >> > b/drivers/gpu/drm/i915/i915_drv.h > >> > index 667fb5c44483..e9a4b96dc775 100644 > >> > --- a/drivers/gpu/drm/i915/i915_drv.h > >> > +++ b/drivers/gpu/drm/i915/i915_drv.h > >> > @@ -3300,6 +3300,26 @@ static inline void > >> > i915_gem_drain_freed_objects(struct drm_i915_private *i915) > >> > } while (flush_work(&i915->mm.free_work)); > >> > } > >> > > >> > +static inline void i915_gem_drain_workqueue(struct drm_i915_private > >> > *i915) > >> > +{ > >> > + /* > >> > + * Similar to objects above (see i915_gem_drain_freed-objects), in > >> > + * general we have workers that are armed by RCU and then rearm > >> > + * themselves in their callbacks. To be paranoid, we need to > >> > + * drain the workqueue a second time after waiting for the RCU > >> > + * grace period so that we catch work queued via RCU from the first > >> > + * pass. As neither drain_workqueue() nor flush_workqueue() report > >> > + * a result, we make an assumption that we only don't require more > >> > + * than 2 passes to catch all recursive RCU delayed work. > >> > + * > >> > + */ > >> > + int pass = 2; > >> > + do { > >> > + rcu_barrier(); > >> > + drain_workqueue(i915->wq); > >> > >> I am fine with the paranoia, and it covers the case below. Still if we do: > >> > >> drain_workqueue(); > >> rcu_barrier(); > >> > >> With drawining in progress, only chain queuing is allowed. I understand > >> this so that when it returns, all the ctx pointers are now unreferenced > >> but not freed. > >> > >> Thus the rcu_barrier() after it cleans the trash and we are good to > >> be unloaded. With one pass. > >> > >> I guess it comes to how to understand the comment, so could you > >> elaborate the 'we have workers that are armed by RCU and then rearm > >> themselves'?. As from drain_workqueue desc, this should be covered. > > > > I'm considering that they may be rearmed via RCU in the general case, > > e.g. context close frees an object and so goes onto an RCU list that > > once processed kicks off a new worker and so requires another round of > > drain_workqueue. We are in module unload so a few extra delays to belts > > and braces are ok until somebody notices it takes a few minutes to run a > > reload test ;) > > Ok. Patch is > Reviewed-by: Mika Kuoppala Thanks, I'm optimistic this will silence the bug, so marking it as resolved. Pushed, -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t v2 2/2] chamelium: Add support for VGA frame comparison testing
On Mon, 2017-07-17 at 13:35 -0400, Lyude Paul wrote: > Just one change for this patch below > > On Wed, 2017-07-12 at 17:57 +0300, Paul Kocialkowski wrote: > > This adds support for VGA frame comparison testing with the > > reference > > generated from cairo. The retrieved frame from the chamelium is > > first > > cropped, as it contains the blanking intervals, through a dedicated > > helper. Another helper function asserts that the analogue frame > > matches or dump it to png if not. > > > > Signed-off-by: Paul Kocialkowski > > --- > > lib/igt_chamelium.c | 164 > > ++-- > > lib/igt_chamelium.h | 7 ++- > > lib/igt_frame.c | 6 +- > > tests/chamelium.c | 57 ++ > > 4 files changed, 225 insertions(+), 9 deletions(-) > > > > diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c > > index df49936b..8701192e 100644 > > --- a/lib/igt_chamelium.c > > +++ b/lib/igt_chamelium.c > > @@ -938,6 +938,8 @@ static cairo_surface_t > > *convert_frame_dump_argb32(const struct chamelium_frame_d > > int w = dump->width, h = dump->height; > > uint32_t *bits_bgr = (uint32_t *) dump->bgr; > > unsigned char *bits_argb; > > + unsigned char *bits_target; > > + int size; > > > > image_bgr = pixman_image_create_bits( > > PIXMAN_b8g8r8, w, h, bits_bgr, > > @@ -947,9 +949,13 @@ static cairo_surface_t > > *convert_frame_dump_argb32(const struct chamelium_frame_d > > > > bits_argb = (unsigned char *) > > pixman_image_get_data(image_argb); > > > > - dump_surface = cairo_image_surface_create_for_data( > > - bits_argb, CAIRO_FORMAT_ARGB32, w, h, > > - PIXMAN_FORMAT_BPP(PIXMAN_x8r8g8b8) / 8 * w); > > + dump_surface = cairo_image_surface_create( > > + CAIRO_FORMAT_ARGB32, w, h); > > + > > + bits_target = cairo_image_surface_get_data(dump_surface); > > + size = cairo_image_surface_get_stride(dump_surface) * h; > > + memcpy(bits_target, bits_argb, size); > > + cairo_surface_mark_dirty(dump_surface); > > > > pixman_image_unref(image_argb); > > > > @@ -1055,6 +1061,154 @@ void chamelium_assert_crc_eq_or_dump(struct > > chamelium *chamelium, > > } > > > > /** > > + * chamelium_assert_analogue_frame_match_or_dump: > > + * @chamelium: The chamelium instance the frame dump belongs to > > + * @frame: The chamelium frame dump to match > > + * @fb: pointer to an #igt_fb structure > > + * > > + * Asserts that the provided captured frame matches the reference > > frame from > > + * the framebuffer. If they do not, this saves the reference and > > captured frames > > + * to a png file. > > + */ > > +void chamelium_assert_analogue_frame_match_or_dump(struct chamelium > > *chamelium, > > + struct > > chamelium_port *port, > > + const struct > > chamelium_frame_dump *frame, > > + struct igt_fb > > *fb) > > +{ > > + cairo_surface_t *reference; > > + cairo_surface_t *capture; > > + igt_crc_t *reference_crc; > > + igt_crc_t *capture_crc; > > + char *reference_suffix; > > + char *capture_suffix; > > + bool match; > > + > > + /* Grab the reference frame from framebuffer */ > > + reference = igt_get_cairo_surface(chamelium->drm_fd, fb); > > + > > + /* Grab the captured frame from chamelium */ > > + capture = convert_frame_dump_argb32(frame); > > + > > + match = igt_check_analogue_frame_match(reference, capture); > > + if (!match && igt_frame_dump_is_enabled()) { > > + reference_crc = > > chamelium_calculate_fb_crc(chamelium- > > > drm_fd, > > > > + fb); > > + capture_crc = chamelium_get_crc_for_area(chamelium, > > port, 0, 0, > > +0, 0); > > + > > + reference_suffix = > > igt_crc_to_string_extended(reference_crc, > > + '-', > > 2); > > + capture_suffix = > > igt_crc_to_string_extended(capture_crc, '-', > > + 2); > > + > > + /* Write reference and capture frames to png */ > > + igt_write_compared_frames_to_png(reference, capture, > > +reference_suffix, > > +capture_suffix); > > + > > + free(reference_suffix); > > + free(capture_suffix); > > + } > > + > > + cairo_surface_destroy(capture); > > + > > + igt_assert(match); > > +} > > + > > + > > +/** > > + * chamelium_analogue_frame_crop: > > + * @chamelium: The Chamelium instance to use > > + * @dump: The chamelium fram
[Intel-gfx] [PATCH 0/9] gpu reset vs modeset fix, plus page_flip removal
Hi all, This fixes the dreaded gpu reset vs. modeset deadlocks. And since the defunct legacy page_flip code is the reason for a bunch of special cases I did remove that too, on Maarten's request. Please review, this is currently blocking extended CI runs on our haswell farm rather badly. The critical patches are up to patch 5. Compared to last time around I've dropped two patches of dubious benefit, they broke gen3/4 reset a bit and aren't really needed. Thanks, Daniel Daniel Vetter (9): drm/i915: Nuke legacy flip queueing code drm/i915: Unbreak gpu reset vs. modeset locking drm/i915: Avoid the gpu reset vs. modeset deadlock drm/i915: Push i915_sw_fence_wait into the nonblocking atomic commit drm/i915: More surgically unbreak the modeset vs reset deadlock drm/i915: Rip out legacy page_flip completion/irq handling drm/i915: adjust has_pending_fb_unpin to atomic drm/i915: Remove intel_flip_work infrastructure drm/i915: Drop unpin stall in atomic_prepare_commit drivers/gpu/drm/i915/i915_debugfs.c | 70 --- drivers/gpu/drm/i915/i915_drv.c |1 - drivers/gpu/drm/i915/i915_drv.h | 10 +- drivers/gpu/drm/i915/i915_gem.c |2 - drivers/gpu/drm/i915/i915_irq.c | 151 + drivers/gpu/drm/i915/intel_display.c | 1129 +++--- drivers/gpu/drm/i915/intel_drv.h | 26 +- drivers/gpu/drm/i915/intel_sprite.c |8 +- 8 files changed, 94 insertions(+), 1303 deletions(-) -- 2.13.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/9] drm/i915: Avoid the gpu reset vs. modeset deadlock
... using the biggest hammer we have. This is essentially a weaponized version of the timeout-based wedging Chris added in commit 36703e79a982c8ce5a8e43833291f2719e92d0d1 Author: Chris Wilson Date: Thu Jun 22 11:56:25 2017 +0100 drm/i915: Break modeset deadlocks on reset Because defense-in-depth is good it's good to still have both. Also note that with the locking change we can now restrict this a lot (old gpus and special testing only), so this doesn't kill the TDR benefits on at least anything remotely modern. And futuremore with a few tricks it should be possible to make a much more educated guess about whether an atomic commit is stuck waiting on the gpu (atomic_t counting the pending i915_sw_fence used by the atomic modeset code should do it), so we can improve this. But for now just start with something that is guaranteed to recover faster, for much better CI througput. This defacto reverts TDR on these platforms, but there's not really a single commit to specify as the sole offender. Fixes: 4680816be336 ("drm/i915: Wait first for submission, before waiting for request completion") Fixes: 221fe7994554 ("drm/i915: Perform a direct reset of the GPU from the waiter") Cc: Chris Wilson Cc: Mika Kuoppala Cc: Joonas Lahtinen Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 5 + 1 file changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9ffa1566..010a1f3e000c 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3471,6 +3471,11 @@ void intel_prepare_reset(struct drm_i915_private *dev_priv) !gpu_reset_clobbers_display(dev_priv)) return; + /* We have a modeset vs reset deadlock, defensively unbreak it. +* +* FIXME: We can do a _lot_ better, this is just a first iteration.*/ + i915_gem_set_wedged(dev_priv); + /* * Need mode_config.mutex so that we don't * trample ongoing ->detect() and whatnot. -- 2.13.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/9] drm/i915: Nuke legacy flip queueing code
Just a very minimal patch to nuke that code. Lots of the flip interrupt handling stuff is still around. Cc: Maarten Lankhorst Cc: Ville Syrjälä Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_drv.h | 5 - drivers/gpu/drm/i915/intel_display.c | 656 --- 2 files changed, 661 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index f66c78d3a0a2..07e98b07c5bc 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -715,11 +715,6 @@ struct drm_i915_display_funcs { void (*fdi_link_train)(struct intel_crtc *crtc, const struct intel_crtc_state *crtc_state); void (*init_clock_gating)(struct drm_i915_private *dev_priv); - int (*queue_flip)(struct drm_device *dev, struct drm_crtc *crtc, - struct drm_framebuffer *fb, - struct drm_i915_gem_object *obj, - struct drm_i915_gem_request *req, - uint32_t flags); void (*hpd_irq_setup)(struct drm_i915_private *dev_priv); /* clock updates for mode set */ /* cursor updates */ diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 74b0ea1badc3..3349ca947173 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2664,20 +2664,6 @@ intel_alloc_initial_plane_obj(struct intel_crtc *crtc, return false; } -/* Update plane->state->fb to match plane->fb after driver-internal updates */ -static void -update_state_fb(struct drm_plane *plane) -{ - if (plane->fb == plane->state->fb) - return; - - if (plane->state->fb) - drm_framebuffer_unreference(plane->state->fb); - plane->state->fb = plane->fb; - if (plane->state->fb) - drm_framebuffer_reference(plane->state->fb); -} - static void intel_set_plane_visible(struct intel_crtc_state *crtc_state, struct intel_plane_state *plane_state, @@ -10163,35 +10149,6 @@ static void intel_crtc_destroy(struct drm_crtc *crtc) kfree(intel_crtc); } -static void intel_unpin_work_fn(struct work_struct *__work) -{ - struct intel_flip_work *work = - container_of(__work, struct intel_flip_work, unpin_work); - struct intel_crtc *crtc = to_intel_crtc(work->crtc); - struct drm_device *dev = crtc->base.dev; - struct drm_plane *primary = crtc->base.primary; - - if (is_mmio_work(work)) - flush_work(&work->mmio_work); - - mutex_lock(&dev->struct_mutex); - intel_unpin_fb_vma(work->old_vma); - i915_gem_object_put(work->pending_flip_obj); - mutex_unlock(&dev->struct_mutex); - - i915_gem_request_put(work->flip_queued_req); - - intel_frontbuffer_flip_complete(to_i915(dev), - to_intel_plane(primary)->frontbuffer_bit); - intel_fbc_post_update(crtc); - drm_framebuffer_unreference(work->old_fb); - - BUG_ON(atomic_read(&crtc->unpin_work_count) == 0); - atomic_dec(&crtc->unpin_work_count); - - kfree(work); -} - /* Is 'a' after or equal to 'b'? */ static bool g4x_flip_count_after_eq(u32 a, u32 b) { @@ -10336,346 +10293,6 @@ static inline void intel_mark_page_flip_active(struct intel_crtc *crtc, atomic_set(&work->pending, 1); } -static int intel_gen2_queue_flip(struct drm_device *dev, -struct drm_crtc *crtc, -struct drm_framebuffer *fb, -struct drm_i915_gem_object *obj, -struct drm_i915_gem_request *req, -uint32_t flags) -{ - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); - u32 flip_mask, *cs; - - cs = intel_ring_begin(req, 6); - if (IS_ERR(cs)) - return PTR_ERR(cs); - - /* Can't queue multiple flips, so wait for the previous -* one to finish before executing the next. -*/ - if (intel_crtc->plane) - flip_mask = MI_WAIT_FOR_PLANE_B_FLIP; - else - flip_mask = MI_WAIT_FOR_PLANE_A_FLIP; - *cs++ = MI_WAIT_FOR_EVENT | flip_mask; - *cs++ = MI_NOOP; - *cs++ = MI_DISPLAY_FLIP | MI_DISPLAY_FLIP_PLANE(intel_crtc->plane); - *cs++ = fb->pitches[0]; - *cs++ = intel_crtc->flip_work->gtt_offset; - *cs++ = 0; /* aux display base address, unused */ - - return 0; -} - -static int intel_gen3_queue_flip(struct drm_device *dev, -struct drm_crtc *crtc, -struct drm_framebuffer *fb, -struct drm_i915_gem_object *obj, -struct drm_i915_gem_request *req, -uint32_t flags) -{ - struct int
[Intel-gfx] [PATCH 2/9] drm/i915: Unbreak gpu reset vs. modeset locking
Taking the modeset locks unconditionally isn't the greatest idea, because atm that part is still broken and times out (and then atomic keels over). And there's really no reason to do so, the old code didn't do that either. To make the patch a bit simpler let's also nuke 2 cases that are only around for the old mmioflip paths. Atomic nonblocking workers will not die (minus bugs) when a gpu reset happens. And of course this doesn't fix any of the gpu reset vs. modeset deadlock fun, but it at least stop modern CI machines from keeling over all over the place for no reason at all. And we still have the explicit testcases to run the fake gpu reset, so coverage isn't that much worse. v2: Split out additional changes on top, restrict this to purely reducing the critical section of modeset locks. v2: Review from Maarten - update comments - don't oops when state is NULL in intel_finish_reset, but try to at least still drop locks properly. The hw is going to be toast anyway. Fixes: 739748939974 ("drm/i915: Fix modeset handling during gpu reset, v5.") Cc: Maarten Lankhorst Reviewed-by: Maarten Lankhorst Cc: Ville Syrjälä Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 60 +++- 1 file changed, 18 insertions(+), 42 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3349ca947173..9ffa1566 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3413,26 +3413,6 @@ static void intel_complete_page_flips(struct drm_i915_private *dev_priv) intel_finish_page_flip_cs(dev_priv, crtc->pipe); } -static void intel_update_primary_planes(struct drm_device *dev) -{ - struct drm_crtc *crtc; - - for_each_crtc(dev, crtc) { - struct intel_plane *plane = to_intel_plane(crtc->primary); - struct intel_plane_state *plane_state = - to_intel_plane_state(plane->base.state); - - if (plane_state->base.visible) { - trace_intel_update_plane(&plane->base, -to_intel_crtc(crtc)); - - plane->update_plane(plane, - to_intel_crtc_state(crtc->state), - plane_state); - } - } -} - static int __intel_display_resume(struct drm_device *dev, struct drm_atomic_state *state, @@ -3485,6 +3465,12 @@ void intel_prepare_reset(struct drm_i915_private *dev_priv) struct drm_atomic_state *state; int ret; + + /* reset doesn't touch the display */ + if (!i915.force_reset_modeset_test && + !gpu_reset_clobbers_display(dev_priv)) + return; + /* * Need mode_config.mutex so that we don't * trample ongoing ->detect() and whatnot. @@ -3498,12 +3484,6 @@ void intel_prepare_reset(struct drm_i915_private *dev_priv) drm_modeset_backoff(ctx); } - - /* reset doesn't touch the display, but flips might get nuked anyway, */ - if (!i915.force_reset_modeset_test && - !gpu_reset_clobbers_display(dev_priv)) - return; - /* * Disabling the crtcs gracefully seems nicer. Also the * g33 docs say we should at least disable all the planes. @@ -3533,6 +3513,14 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) struct drm_atomic_state *state = dev_priv->modeset_restore_state; int ret; + /* reset doesn't touch the display */ + if (!i915.force_reset_modeset_test && + !gpu_reset_clobbers_display(dev_priv)) + return; + + if (!state) + goto unlock; + /* * Flips in the rings will be nuked by the reset, * so complete all pending flips so that user space @@ -3544,22 +3532,10 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) /* reset doesn't touch the display */ if (!gpu_reset_clobbers_display(dev_priv)) { - if (!state) { - /* -* Flips in the rings have been nuked by the reset, -* so update the base address of all primary -* planes to the the last fb to make sure we're -* showing the correct fb after a reset. -* -* FIXME: Atomic will make this obsolete since we won't schedule -* CS-based flips (which might get lost in gpu resets) any more. -*/ - intel_update_primary_planes(dev); - } else { - ret = __intel_display_resume(dev, state, ctx); + /* for testing only restore the display */ + ret = __intel_display_resume(de
[Intel-gfx] [PATCH 5/9] drm/i915: More surgically unbreak the modeset vs reset deadlock
There's no reason to entirely wedge the gpu, for the minimal deadlock bugfix we only need to unbreak/decouple the atomic commit from the gpu reset. The simplest wait to fix that is by replacing the unconditional fence wait a the top of commit_tail by a wait which completes either when the fences are done (normal case, or when a reset doesn't need to touch the display state). Or when the gpu reset needs to force-unblock all pending modeset states. Note that in both cases TDR itself keeps working, so from a userspace pov this trickery isn't observable. Users themselvs might spot a short glitch while the rendering is catching up again, but that's still better than pre-TDR where we've thrown away all the rendering, including innocent batches. Also, this fixes the regression TDR introduced of making gpu resets deadlock-prone when we do need to touch the display. One thing I noticed is that gpu_error.flags seems to use both our own wait-queue in gpu_error.wait_queue, and the generic wait_on_bit facilities. Not entirely sure why this inconsistency exists, I just picked one style. A possible future avenue could be to insert the gpu reset in-between ongoing modeset changes, which would avoid the momentary glitch. But that's a lot more work to implement in the atomic commit machinery, and given that we only need this for pre-g4x hw, of questionable utility just for the sake of polishing gpu reset even more on those old boxes. It might be useful for other features though. v2: Rebase onto 4.13 with a s/wait_queue_t/struct wait_queue_entry/. Cc: Chris Wilson Cc: Mika Kuoppala Cc: Joonas Lahtinen Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/intel_display.c | 35 ++- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 07e98b07c5bc..369968539b40 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1564,6 +1564,7 @@ struct i915_gpu_error { unsigned long flags; #define I915_RESET_BACKOFF 0 #define I915_RESET_HANDOFF 1 +#define I915_RESET_MODESET 2 #define I915_WEDGED(BITS_PER_LONG - 1) #define I915_RESET_ENGINE (I915_WEDGED - I915_NUM_ENGINES) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 5aa7ca1ab592..4762f158032d 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -3471,10 +3471,9 @@ void intel_prepare_reset(struct drm_i915_private *dev_priv) !gpu_reset_clobbers_display(dev_priv)) return; - /* We have a modeset vs reset deadlock, defensively unbreak it. -* -* FIXME: We can do a _lot_ better, this is just a first iteration.*/ - i915_gem_set_wedged(dev_priv); + /* We have a modeset vs reset deadlock, defensively unbreak it. */ + set_bit(I915_RESET_MODESET, &dev_priv->gpu_error.flags); + wake_up_all(&dev_priv->gpu_error.wait_queue); /* * Need mode_config.mutex so that we don't @@ -3569,6 +3568,8 @@ void intel_finish_reset(struct drm_i915_private *dev_priv) drm_modeset_drop_locks(ctx); drm_modeset_acquire_fini(ctx); mutex_unlock(&dev->mode_config.mutex); + + clear_bit(I915_RESET_MODESET, &dev_priv->gpu_error.flags); } static bool abort_flip_on_reset(struct intel_crtc *crtc) @@ -12384,6 +12385,30 @@ static void intel_atomic_helper_free_state_worker(struct work_struct *work) intel_atomic_helper_free_state(dev_priv); } +static void intel_atomic_commit_fence_wait(struct intel_atomic_state *intel_state) +{ + struct wait_queue_entry wait_fence, wait_reset; + struct drm_i915_private *dev_priv = to_i915(intel_state->base.dev); + + init_wait_entry(&wait_fence, 0); + init_wait_entry(&wait_reset, 0); + for (;;) { + prepare_to_wait(&intel_state->commit_ready.wait, + &wait_fence, TASK_UNINTERRUPTIBLE); + prepare_to_wait(&dev_priv->gpu_error.wait_queue, + &wait_reset, TASK_UNINTERRUPTIBLE); + + + if (i915_sw_fence_done(&intel_state->commit_ready) + || (dev_priv->gpu_error.flags & I915_RESET_MODESET)) + break; + + schedule(); + } + finish_wait(&intel_state->commit_ready.wait, &wait_fence); + finish_wait(&dev_priv->gpu_error.wait_queue, &wait_reset); +} + static void intel_atomic_commit_tail(struct drm_atomic_state *state) { struct drm_device *dev = state->dev; @@ -12397,7 +12422,7 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) unsigned crtc_vblank_mask = 0; int i; - i915_sw_fence_wait(&intel_state->commit_ready); + intel_atomic_commit_fence_wait(intel_state); drm_atomic_helper_wait
[Intel-gfx] [PATCH 8/9] drm/i915: Remove intel_flip_work infrastructure
This gets rid of all the interactions between the legacy flip code and the modeset code. Yay! Cc: Maarten Lankhorst Cc: Ville Syrjälä Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_debugfs.c | 70 - drivers/gpu/drm/i915/i915_drv.c | 1 - drivers/gpu/drm/i915/i915_drv.h | 4 -- drivers/gpu/drm/i915/i915_gem.c | 2 - drivers/gpu/drm/i915/intel_display.c | 117 +-- drivers/gpu/drm/i915/intel_drv.h | 21 +-- drivers/gpu/drm/i915/intel_sprite.c | 8 +-- 7 files changed, 3 insertions(+), 220 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index 2ef75c1a6119..c25f42c60d61 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -543,75 +543,6 @@ static int i915_gem_gtt_info(struct seq_file *m, void *data) return 0; } -static int i915_gem_pageflip_info(struct seq_file *m, void *data) -{ - struct drm_i915_private *dev_priv = node_to_i915(m->private); - struct drm_device *dev = &dev_priv->drm; - struct intel_crtc *crtc; - int ret; - - ret = mutex_lock_interruptible(&dev->struct_mutex); - if (ret) - return ret; - - for_each_intel_crtc(dev, crtc) { - const char pipe = pipe_name(crtc->pipe); - const char plane = plane_name(crtc->plane); - struct intel_flip_work *work; - - spin_lock_irq(&dev->event_lock); - work = crtc->flip_work; - if (work == NULL) { - seq_printf(m, "No flip due on pipe %c (plane %c)\n", - pipe, plane); - } else { - u32 pending; - u32 addr; - - pending = atomic_read(&work->pending); - if (pending) { - seq_printf(m, "Flip ioctl preparing on pipe %c (plane %c)\n", - pipe, plane); - } else { - seq_printf(m, "Flip pending (waiting for vsync) on pipe %c (plane %c)\n", - pipe, plane); - } - if (work->flip_queued_req) { - struct intel_engine_cs *engine = work->flip_queued_req->engine; - - seq_printf(m, "Flip queued on %s at seqno %x, last submitted seqno %x [current breadcrumb %x], completed? %d\n", - engine->name, - work->flip_queued_req->global_seqno, - intel_engine_last_submit(engine), - intel_engine_get_seqno(engine), - i915_gem_request_completed(work->flip_queued_req)); - } else - seq_printf(m, "Flip not associated with any ring\n"); - seq_printf(m, "Flip queued on frame %d, (was ready on frame %d), now %d\n", - work->flip_queued_vblank, - work->flip_ready_vblank, - intel_crtc_get_vblank_counter(crtc)); - seq_printf(m, "%d prepares\n", atomic_read(&work->pending)); - - if (INTEL_GEN(dev_priv) >= 4) - addr = I915_HI_DISPBASE(I915_READ(DSPSURF(crtc->plane))); - else - addr = I915_READ(DSPADDR(crtc->plane)); - seq_printf(m, "Current scanout address 0x%08x\n", addr); - - if (work->pending_flip_obj) { - seq_printf(m, "New framebuffer address 0x%08lx\n", (long)work->gtt_offset); - seq_printf(m, "MMIO update completed? %d\n", addr == work->gtt_offset); - } - } - spin_unlock_irq(&dev->event_lock); - } - - mutex_unlock(&dev->struct_mutex); - - return 0; -} - static int i915_gem_batch_pool_info(struct seq_file *m, void *data) { struct drm_i915_private *dev_priv = node_to_i915(m->private); @@ -4854,7 +4785,6 @@ static const struct drm_info_list i915_debugfs_list[] = { {"i915_gem_gtt", i915_gem_gtt_info, 0}, {"i915_gem_pin_display", i915_gem_gtt_info, 0, (void *)1}, {"i915_gem_stolen", i915_gem_stolen_list_info }, - {"i915_gem_pageflip", i915_gem_pageflip_info, 0}, {"i915_gem_request", i915_gem_request_info, 0}, {"i915_gem_seqno", i915_gem_seqno_info, 0}, {"i915_gem_fence_regs", i915_gem_fence_regs_info, 0}, diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index d36ffcdbb89a..6b583dc2eb1f 10
[Intel-gfx] [PATCH 4/9] drm/i915: Push i915_sw_fence_wait into the nonblocking atomic commit
Blocking in a worker is ok, that's what the unbound_wq is for. And it unifies the paths between the blocking and nonblocking commit, giving me just one path where I have to implement the deadlock avoidance trickery in the next patch. Cc: Chris Wilson Cc: Mika Kuoppala Cc: Joonas Lahtinen Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 15 +++ 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 010a1f3e000c..5aa7ca1ab592 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -12397,6 +12397,8 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) unsigned crtc_vblank_mask = 0; int i; + i915_sw_fence_wait(&intel_state->commit_ready); + drm_atomic_helper_wait_for_dependencies(state); if (intel_state->modeset) @@ -12564,10 +12566,7 @@ intel_atomic_commit_ready(struct i915_sw_fence *fence, switch (notify) { case FENCE_COMPLETE: - if (state->base.commit_work.func) - queue_work(system_unbound_wq, &state->base.commit_work); break; - case FENCE_FREE: { struct intel_atomic_helper *helper = @@ -12671,14 +12670,14 @@ static int intel_atomic_commit(struct drm_device *dev, } drm_atomic_state_get(state); - INIT_WORK(&state->commit_work, - nonblock ? intel_atomic_commit_work : NULL); + INIT_WORK(&state->commit_work, intel_atomic_commit_work); i915_sw_fence_commit(&intel_state->commit_ready); - if (!nonblock) { - i915_sw_fence_wait(&intel_state->commit_ready); + if (nonblock) + queue_work(system_unbound_wq, &state->commit_work); + else intel_atomic_commit_tail(state); - } + return 0; } -- 2.13.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/9] drm/i915: Rip out legacy page_flip completion/irq handling
All these races and things are now solved through the vblank evasion trick, plus event handling is done using normal vblank even processing and drm_crtc_arm_vblank_event. We can get rid of all this complexity. Cc: Maarten Lankhorst Cc: Ville Syrjälä Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_irq.c | 151 drivers/gpu/drm/i915/intel_display.c | 215 --- drivers/gpu/drm/i915/intel_drv.h | 3 - 3 files changed, 22 insertions(+), 347 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index f0cb278cee8b..b64c89e0fbf1 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -1708,18 +1708,6 @@ static void gen9_guc_irq_handler(struct drm_i915_private *dev_priv, u32 gt_iir) } } -static bool intel_pipe_handle_vblank(struct drm_i915_private *dev_priv, -enum pipe pipe) -{ - bool ret; - - ret = drm_handle_vblank(&dev_priv->drm, pipe); - if (ret) - intel_finish_page_flip_mmio(dev_priv, pipe); - - return ret; -} - static void valleyview_pipestat_irq_ack(struct drm_i915_private *dev_priv, u32 iir, u32 pipe_stats[I915_MAX_PIPES]) { @@ -1784,12 +1772,8 @@ static void valleyview_pipestat_irq_handler(struct drm_i915_private *dev_priv, enum pipe pipe; for_each_pipe(dev_priv, pipe) { - if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS && - intel_pipe_handle_vblank(dev_priv, pipe)) - intel_check_page_flip(dev_priv, pipe); - - if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) - intel_finish_page_flip_cs(dev_priv, pipe); + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS) + drm_handle_vblank(&dev_priv->drm, pipe); if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS) i9xx_pipe_crc_irq_handler(dev_priv, pipe); @@ -2241,19 +2225,14 @@ static void ilk_display_irq_handler(struct drm_i915_private *dev_priv, DRM_ERROR("Poison interrupt\n"); for_each_pipe(dev_priv, pipe) { - if (de_iir & DE_PIPE_VBLANK(pipe) && - intel_pipe_handle_vblank(dev_priv, pipe)) - intel_check_page_flip(dev_priv, pipe); + if (de_iir & DE_PIPE_VBLANK(pipe)) + drm_handle_vblank(&dev_priv->drm, pipe); if (de_iir & DE_PIPE_FIFO_UNDERRUN(pipe)) intel_cpu_fifo_underrun_irq_handler(dev_priv, pipe); if (de_iir & DE_PIPE_CRC_DONE(pipe)) i9xx_pipe_crc_irq_handler(dev_priv, pipe); - - /* plane/pipes map 1:1 on ilk+ */ - if (de_iir & DE_PLANE_FLIP_DONE(pipe)) - intel_finish_page_flip_cs(dev_priv, pipe); } /* check event from PCH */ @@ -2315,13 +2294,8 @@ static void ivb_display_irq_handler(struct drm_i915_private *dev_priv, intel_opregion_asle_intr(dev_priv); for_each_pipe(dev_priv, pipe) { - if (de_iir & (DE_PIPE_VBLANK_IVB(pipe)) && - intel_pipe_handle_vblank(dev_priv, pipe)) - intel_check_page_flip(dev_priv, pipe); - - /* plane/pipes map 1:1 on ilk+ */ - if (de_iir & DE_PLANE_FLIP_DONE_IVB(pipe)) - intel_finish_page_flip_cs(dev_priv, pipe); + if (de_iir & (DE_PIPE_VBLANK_IVB(pipe))) + drm_handle_vblank(&dev_priv->drm, pipe); } /* check event from PCH */ @@ -2502,7 +2476,7 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl) } for_each_pipe(dev_priv, pipe) { - u32 flip_done, fault_errors; + u32 fault_errors; if (!(master_ctl & GEN8_DE_PIPE_IRQ(pipe))) continue; @@ -2516,18 +2490,8 @@ gen8_de_irq_handler(struct drm_i915_private *dev_priv, u32 master_ctl) ret = IRQ_HANDLED; I915_WRITE(GEN8_DE_PIPE_IIR(pipe), iir); - if (iir & GEN8_PIPE_VBLANK && - intel_pipe_handle_vblank(dev_priv, pipe)) - intel_check_page_flip(dev_priv, pipe); - - flip_done = iir; - if (INTEL_INFO(dev_priv)->gen >= 9) - flip_done &= GEN9_PIPE_PLANE1_FLIP_DONE; - else - flip_done &= GEN8_PIPE_PRIMARY_FLIP_DONE; - - if (flip_done) - intel_finish_page_flip_cs(dev_priv, pipe); + if (iir & GEN8_PIPE_VBLANK) + drm_handle_vblank(&dev_priv->drm, pipe); if (iir & GEN8_PIPE_CDCLK_CRC_DONE) h
[Intel-gfx] [PATCH 7/9] drm/i915: adjust has_pending_fb_unpin to atomic
A bit an oversight - the current code did nothing, since only legacy flips used the unpin_work_count and assorted logic. Cc: Maarten Lankhorst Cc: Ville Syrjälä Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 27 ++- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 02620f31374b..343883214113 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4140,21 +4140,22 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc) bool intel_has_pending_fb_unpin(struct drm_i915_private *dev_priv) { - struct intel_crtc *crtc; - - /* Note that we don't need to be called with mode_config.lock here -* as our list of CRTC objects is static for the lifetime of the -* device and so cannot disappear as we iterate. Similarly, we can -* happily treat the predicates as racy, atomic checks as userspace -* cannot claim and pin a new fb without at least acquring the -* struct_mutex and so serialising with us. -*/ - for_each_intel_crtc(&dev_priv->drm, crtc) { - if (atomic_read(&crtc->unpin_work_count) == 0) + struct drm_crtc *crtc; + bool cleanup_done; + + drm_for_each_crtc(crtc, &dev_priv->drm) { + struct drm_crtc_commit *commit; + spin_lock(&crtc->commit_lock); + commit = list_first_entry_or_null(&crtc->commit_list, + struct drm_crtc_commit, commit_entry); + cleanup_done = commit ? + try_wait_for_completion(&commit->cleanup_done) : true; + spin_unlock(&crtc->commit_lock); + + if (cleanup_done) continue; - if (crtc->flip_work) - intel_wait_for_vblank(dev_priv, crtc->pipe); + drm_crtc_wait_one_vblank(crtc); return true; } -- 2.13.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 9/9] drm/i915: Drop unpin stall in atomic_prepare_commit
The core already does this in setup_commit(). With this we can also remove the unpin_work_count since it's the last user. Cc: Maarten Lankhorst Cc: Ville Syrjälä Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/intel_display.c | 13 + drivers/gpu/drm/i915/intel_drv.h | 2 -- 2 files changed, 1 insertion(+), 14 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e52a2adbaaa5..351208b7b1ad 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -11825,18 +11825,7 @@ static int intel_atomic_check(struct drm_device *dev, static int intel_atomic_prepare_commit(struct drm_device *dev, struct drm_atomic_state *state) { - struct drm_i915_private *dev_priv = to_i915(dev); - struct drm_crtc_state *crtc_state; - struct drm_crtc *crtc; - int i, ret; - - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { - if (state->legacy_cursor_update) - continue; - - if (atomic_read(&to_intel_crtc(crtc)->unpin_work_count) >= 2) - flush_workqueue(dev_priv->wq); - } + int ret; ret = mutex_lock_interruptible(&dev->struct_mutex); if (ret) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 9cb7e781e863..96402c06e295 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -798,8 +798,6 @@ struct intel_crtc { unsigned long long enabled_power_domains; struct intel_overlay *overlay; - atomic_t unpin_work_count; - /* Display surface base address adjustement for pageflips. Note that on * gen4+ this only adjusts up to a tile, offsets within a tile are * handled in the hw itself (with the TILEOFF register). */ -- 2.13.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm/i915: Push i915_sw_fence_wait into the nonblocking atomic commit
Quoting Daniel Vetter (2017-07-19 13:54:57) > Blocking in a worker is ok, but needlessly inefficient, > that's what the unbound_wq is for. And it > unifies the paths between the blocking and nonblocking commit, giving > me just one path where I have to implement the deadlock avoidance > trickery in the next patch. For reference, I did that the other way by moving it all over to fences. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/9] drm/i915: adjust has_pending_fb_unpin to atomic
Quoting Daniel Vetter (2017-07-19 13:55:00) > A bit an oversight - the current code did nothing, since only > legacy flips used the unpin_work_count and assorted logic. > > Cc: Maarten Lankhorst > Cc: Ville Syrjälä > Signed-off-by: Daniel Vetter There's a fence deadlock testcase for this, kms_flip/fence? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 8/9] drm/i915: Remove intel_flip_work infrastructure
Quoting Daniel Vetter (2017-07-19 13:55:01) > This gets rid of all the interactions between the legacy flip code and > the modeset code. Yay! Including our missed vblank boosting. (That's been dead for a while.) -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 9/9] drm/i915: Drop unpin stall in atomic_prepare_commit
Quoting Daniel Vetter (2017-07-19 13:55:02) > The core already does this in setup_commit(). With this we can also > remove the unpin_work_count since it's the last user. Also note that the loop only existed for the legacy pageflip support, with that removed it becomes entirely redundant. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm/i915: Push i915_sw_fence_wait into the nonblocking atomic commit
On Wed, Jul 19, 2017 at 02:04:25PM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2017-07-19 13:54:57) > > Blocking in a worker is ok, > > but needlessly inefficient, > > > that's what the unbound_wq is for. And it > > unifies the paths between the blocking and nonblocking commit, giving > > me just one path where I have to implement the deadlock avoidance > > trickery in the next patch. > > For reference, I did that the other way by moving it all over to fences. Yeah the commit message fails to explain this here: "I first tried to implement the following patch without this rework, but force-completing i915_sw_fence creates some serious challenges around properly cleaning things up. So wasn't a feasible short-term approach. Another approach would be to simple keep track of all pending atomic commit work items and manually queue them from the reset code. With the caveat that double-queue in case we race with the i915_sw_fence must be avoided. Given all that, taking the cost of a double schedule in atomic for the short-term fix is the best approach, but can be changed in the future of course." Thanks, 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 7/9] drm/i915: adjust has_pending_fb_unpin to atomic
On Wed, Jul 19, 2017 at 02:06:43PM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2017-07-19 13:55:00) > > A bit an oversight - the current code did nothing, since only > > legacy flips used the unpin_work_count and assorted logic. > > > > Cc: Maarten Lankhorst > > Cc: Ville Syrjälä > > Signed-off-by: Daniel Vetter > > There's a fence deadlock testcase for this, kms_flip/fence? Crunching through them, but since I tend to test my stuff all mixed into one pile I've hit a bug in a different patch series of mine. Given that we've run without this for a while, not sure it's that critical really. -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] Fixes that failed to backport to v4.13-rc1
Another kernel, another list of failed backports. The following commits have been marked as Cc: stable or fixing something in v4.13-rc1 or earlier, but failed to cherry-pick to drm-intel-fixes. Please see if they are worth backporting, and please do so if they are. BR, Jani. 54d20ed1fff2 ("drm/i915: Fix bad comparison in skl_compute_plane_wm, v2.") -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 9/9] drm/i915: Drop unpin stall in atomic_prepare_commit
On Wed, Jul 19, 2017 at 02:09:02PM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2017-07-19 13:55:02) > > The core already does this in setup_commit(). With this we can also > > remove the unpin_work_count since it's the last user. > > Also note that the loop only existed for the legacy pageflip support, > with that removed it becomes entirely redundant. Yeah, I just wanted to make clear that removing this isn't a bit of code that we failed to move over to atomic. It's been dead since a while. -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 8/9] drm/i915: Remove intel_flip_work infrastructure
On Wed, Jul 19, 2017 at 02:07:36PM +0100, Chris Wilson wrote: > Quoting Daniel Vetter (2017-07-19 13:55:01) > > This gets rid of all the interactions between the legacy flip code and > > the modeset code. Yay! > > Including our missed vblank boosting. (That's been dead for a while.) Hm right, but should be easy to add (and bonus, for every display update, not just flips) in the intel_sw_fence_wait function. Can you pls point me at what function I should call to reverse-boost an i915_sw_fence (and only that)? Then we could queue up a vblank callback that fires on the next vblank for any of the CRTC in the update and boosts the i915_sw_fence. If we just boost the fence (instead of the context or something) it should also be easy to filter out boosting when the request has completed meanwhile. -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/9] drm/i915: Avoid the gpu reset vs. modeset deadlock
Quoting Daniel Vetter (2017-07-19 13:54:56) > ... using the biggest hammer we have. This is essentially a weaponized > version of the timeout-based wedging Chris added in > > commit 36703e79a982c8ce5a8e43833291f2719e92d0d1 > Author: Chris Wilson > Date: Thu Jun 22 11:56:25 2017 +0100 > > drm/i915: Break modeset deadlocks on reset > > Because defense-in-depth is good it's good to still have both. Also > note that with the locking change we can now restrict this a lot (old > gpus and special testing only), so this doesn't kill the TDR benefits > on at least anything remotely modern. > > And futuremore with a few tricks it should be possible to make a much > more educated guess about whether an atomic commit is stuck waiting on > the gpu (atomic_t counting the pending i915_sw_fence used by the > atomic modeset code should do it), so we can improve this. > > But for now just start with something that is guaranteed to recover > faster, for much better CI througput. > > This defacto reverts TDR on these platforms, but there's not really a > single commit to specify as the sole offender. > > Fixes: 4680816be336 ("drm/i915: Wait first for submission, before waiting for > request completion") > Fixes: 221fe7994554 ("drm/i915: Perform a direct reset of the GPU from the > waiter") > Cc: Chris Wilson > Cc: Mika Kuoppala > Cc: Joonas Lahtinen > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_display.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 9ffa1566..010a1f3e000c 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3471,6 +3471,11 @@ void intel_prepare_reset(struct drm_i915_private > *dev_priv) > !gpu_reset_clobbers_display(dev_priv)) > return; > > + /* We have a modeset vs reset deadlock, defensively unbreak it. > +* > +* FIXME: We can do a _lot_ better, this is just a first iteration.*/ You should keep the error message for wedging the device. If all goes well it is removed in a few patches, so shouldn't be an eyesore for long. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/9] drm/i915: More surgically unbreak the modeset vs reset deadlock
Quoting Daniel Vetter (2017-07-19 13:54:58) > There's no reason to entirely wedge the gpu, for the minimal deadlock > bugfix we only need to unbreak/decouple the atomic commit from the gpu > reset. The simplest wait to fix that is by replacing the > unconditional fence wait a the top of commit_tail by a wait which > completes either when the fences are done (normal case, or when a > reset doesn't need to touch the display state). Or when the gpu reset > needs to force-unblock all pending modeset states. > > Note that in both cases TDR itself keeps working, so from a userspace > pov this trickery isn't observable. Users themselvs might spot a short > glitch while the rendering is catching up again, but that's still > better than pre-TDR where we've thrown away all the rendering, > including innocent batches. Also, this fixes the regression TDR > introduced of making gpu resets deadlock-prone when we do need to > touch the display. > > One thing I noticed is that gpu_error.flags seems to use both our own > wait-queue in gpu_error.wait_queue, and the generic wait_on_bit > facilities. Not entirely sure why this inconsistency exists, I just > picked one style. > > A possible future avenue could be to insert the gpu reset in-between > ongoing modeset changes, which would avoid the momentary glitch. But > that's a lot more work to implement in the atomic commit machinery, > and given that we only need this for pre-g4x hw, of questionable > utility just for the sake of polishing gpu reset even more on those > old boxes. It might be useful for other features though. > > v2: Rebase onto 4.13 with a s/wait_queue_t/struct wait_queue_entry/. > > Cc: Chris Wilson > Cc: Mika Kuoppala > Cc: Joonas Lahtinen > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/intel_display.c | 35 ++- > 2 files changed, 31 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 07e98b07c5bc..369968539b40 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1564,6 +1564,7 @@ struct i915_gpu_error { > unsigned long flags; > #define I915_RESET_BACKOFF 0 > #define I915_RESET_HANDOFF 1 > +#define I915_RESET_MODESET 2 > #define I915_WEDGED(BITS_PER_LONG - 1) > #define I915_RESET_ENGINE (I915_WEDGED - I915_NUM_ENGINES) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 5aa7ca1ab592..4762f158032d 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -3471,10 +3471,9 @@ void intel_prepare_reset(struct drm_i915_private > *dev_priv) > !gpu_reset_clobbers_display(dev_priv)) > return; > > - /* We have a modeset vs reset deadlock, defensively unbreak it. > -* > -* FIXME: We can do a _lot_ better, this is just a first iteration.*/ > - i915_gem_set_wedged(dev_priv); > + /* We have a modeset vs reset deadlock, defensively unbreak it. */ > + set_bit(I915_RESET_MODESET, &dev_priv->gpu_error.flags); > + wake_up_all(&dev_priv->gpu_error.wait_queue); How are we breaking the modeset_lock -> struct_mutex -> wait_on_reset ? We wait the modeset_lock next which stops the reset from proceeding, and so the deadlock persists until the wedge-me timeout? -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/9] drm/i915: Avoid the gpu reset vs. modeset deadlock
On Wed, Jul 19, 2017 at 3:32 PM, Chris Wilson wrote: > Quoting Daniel Vetter (2017-07-19 13:54:56) >> ... using the biggest hammer we have. This is essentially a weaponized >> version of the timeout-based wedging Chris added in >> >> commit 36703e79a982c8ce5a8e43833291f2719e92d0d1 >> Author: Chris Wilson >> Date: Thu Jun 22 11:56:25 2017 +0100 >> >> drm/i915: Break modeset deadlocks on reset >> >> Because defense-in-depth is good it's good to still have both. Also >> note that with the locking change we can now restrict this a lot (old >> gpus and special testing only), so this doesn't kill the TDR benefits >> on at least anything remotely modern. >> >> And futuremore with a few tricks it should be possible to make a much >> more educated guess about whether an atomic commit is stuck waiting on >> the gpu (atomic_t counting the pending i915_sw_fence used by the >> atomic modeset code should do it), so we can improve this. >> >> But for now just start with something that is guaranteed to recover >> faster, for much better CI througput. >> >> This defacto reverts TDR on these platforms, but there's not really a >> single commit to specify as the sole offender. >> >> Fixes: 4680816be336 ("drm/i915: Wait first for submission, before waiting >> for request completion") >> Fixes: 221fe7994554 ("drm/i915: Perform a direct reset of the GPU from the >> waiter") >> Cc: Chris Wilson >> Cc: Mika Kuoppala >> Cc: Joonas Lahtinen >> Signed-off-by: Daniel Vetter >> --- >> drivers/gpu/drm/i915/intel_display.c | 5 + >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index 9ffa1566..010a1f3e000c 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -3471,6 +3471,11 @@ void intel_prepare_reset(struct drm_i915_private >> *dev_priv) >> !gpu_reset_clobbers_display(dev_priv)) >> return; >> >> + /* We have a modeset vs reset deadlock, defensively unbreak it. >> +* >> +* FIXME: We can do a _lot_ better, this is just a first iteration.*/ > > You should keep the error message for wedging the device. If all goes > well it is removed in a few patches, so shouldn't be an eyesore for > long. Yeah makes sense, just in case the next patches need to be reverted for some reasons. That's why I split it ou. Something like DRM_ERROR("Wedging gpu to unblock modesets\n"); and then remove that again 2 patches later? -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
[Intel-gfx] [PATCH i-g-t v5 4/7] Introduce common frame dumping configuration and helpers
This introduces a common FrameDumpPath configuration field, as well as helper functions in dedicated igt_frame for writing cairo surfaces to png files. Signed-off-by: Paul Kocialkowski --- lib/Makefile.sources | 2 + lib/igt.h| 1 + lib/igt_core.c | 12 + lib/igt_core.h | 2 +- lib/igt_frame.c | 137 +++ lib/igt_frame.h | 43 6 files changed, 196 insertions(+), 1 deletion(-) create mode 100644 lib/igt_frame.c create mode 100644 lib/igt_frame.h diff --git a/lib/Makefile.sources b/lib/Makefile.sources index 53fdb54c..c2e58809 100644 --- a/lib/Makefile.sources +++ b/lib/Makefile.sources @@ -83,6 +83,8 @@ lib_source_list = \ uwildmat/uwildmat.c \ igt_kmod.c \ igt_kmod.h \ + igt_frame.c \ + igt_frame.h \ $(NULL) .PHONY: version.h.tmp diff --git a/lib/igt.h b/lib/igt.h index a069deb3..d16a4991 100644 --- a/lib/igt.h +++ b/lib/igt.h @@ -34,6 +34,7 @@ #include "igt_draw.h" #include "igt_dummyload.h" #include "igt_fb.h" +#include "igt_frame.h" #include "igt_gt.h" #include "igt_kms.h" #include "igt_pm.h" diff --git a/lib/igt_core.c b/lib/igt_core.c index 1ba79361..5a3b00e8 100644 --- a/lib/igt_core.c +++ b/lib/igt_core.c @@ -235,6 +235,10 @@ * An example configuration follows: * * |[ + * # The common configuration secton follows. + * [Common] + * FrameDumpPath=/tmp # The path to dump frames that fail comparison checks + * * # The following section is used for configuring the Device Under Test. * # It is not mandatory and allows overriding default values. * [DUT] @@ -290,6 +294,7 @@ static struct { static pthread_mutex_t log_buffer_mutex = PTHREAD_MUTEX_INITIALIZER; GKeyFile *igt_key_file; +char *frame_dump_path; const char *igt_test_name(void) { @@ -621,6 +626,13 @@ static int config_parse(void) if (!igt_key_file) return 0; + frame_dump_path = getenv("IGT_FRAME_DUMP_PATH"); + + if (!frame_dump_path) + frame_dump_path = g_key_file_get_string(igt_key_file, "Common", + "FrameDumpPath", + &error); + rc = g_key_file_get_integer(igt_key_file, "DUT", "SuspendResumeDelay", &error); if (error && error->code == G_KEY_FILE_ERROR_INVALID_VALUE) diff --git a/lib/igt_core.h b/lib/igt_core.h index 0739ca83..1619a9d6 100644 --- a/lib/igt_core.h +++ b/lib/igt_core.h @@ -50,7 +50,7 @@ extern const char* __igt_test_description __attribute__((weak)); extern bool __igt_plain_output; extern GKeyFile *igt_key_file; - +extern char *frame_dump_path; /** * IGT_TEST_DESCRIPTION: diff --git a/lib/igt_frame.c b/lib/igt_frame.c new file mode 100644 index ..dfafe53d --- /dev/null +++ b/lib/igt_frame.c @@ -0,0 +1,137 @@ +/* + * Copyright © 2017 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + * Paul Kocialkowski + */ + +#include "config.h" + +#include +#include +#include + +#include "igt.h" + +/** + * SECTION:igt_frame + * @short_description: Library for frame-related tests + * @title: Frame + * @include: igt_frame.h + * + * This library contains helpers for frame-related tests. This includes common + * frame dumping as well as frame comparison helpers. + */ + +/** + * igt_frame_dump_is_enabled: + * + * Get whether frame dumping is enabled. + * + * Returns: A boolean indicating whether frame dumping is enabled + */ +bool igt_frame_dump_is_enabled(void) +{ + return frame_dump_path != NULL; +} + +static void igt_write_frame_to_png(cairo_surface_t *surface, int fd, + const char *qualifier, const char *suffix) +{ +
[Intel-gfx] [PATCH i-g-t v5 0/7] CRC testing with Chamelium improvements
Changes since v4: * Moved igt_get_cairo_surface out of the thread function to properly handle assert failure * Rebased on top of current master Changes since v3: * Renamed structure used by async crc calculation for more clarity * Used const qualifier for untouched buffer when hashing * Split actual calculation to a dedicated function * Reworked async functions names for more clarity * Reworked descriptions for better accuracy * Exported framebuffer cairo surface and use it directly instead of (unused) png dumping * Fix how the framebuffer cairo surface is obtained to avoid destroying it too early * Made CRC checking logic common * Added a check for the same number of words * Made frame dumping configuration and helpers common * Added an extended crc to string helper * Added a chamelium helper for crc checking and frame dumping * Split the merging of crc functions to a separate patch * Added support for frame dump path global variable * Added listing the dumped images in a file, possibly identified with an id global variable The latter allows having one "dump report" file per run, possibly identified with the id global variable, that indicates which files are the output, while keeping the possibility to have the same files for different runs. This allows saving significant disk space when the images are identified with e.g. their crc, so that duplicate results only lead to duplicate dump reports and not duplicate images. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v5 5/7] lib/igt_debugfs: Add extended helper to format crc to string
This introduces a igt_crc_to_string_extended helper that allows formatting a crc to a string with a given delimiter and size to print per crc word. Signed-off-by: Paul Kocialkowski --- lib/igt_debugfs.c | 27 +++ lib/igt_debugfs.h | 1 + 2 files changed, 24 insertions(+), 4 deletions(-) diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c index ef05dc77..2aa33586 100644 --- a/lib/igt_debugfs.c +++ b/lib/igt_debugfs.c @@ -348,28 +348,47 @@ bool igt_check_crc_equal(const igt_crc_t *a, const igt_crc_t *b) } /** - * igt_crc_to_string: + * igt_crc_to_string_extended: * @crc: pipe CRC value to print + * @delimiter: The delimiter to use between crc words + * @crc_size: the number of bytes to print per crc word (either 4 or 2) * - * This function allocates a string and formats @crc into it. + * This function allocates a string and formats @crc into it, depending on + * @delimiter and @crc_size. * The caller is responsible for freeing the string. * * This should only ever be used for diagnostic debug output. */ -char *igt_crc_to_string(igt_crc_t *crc) +char *igt_crc_to_string_extended(igt_crc_t *crc, char delimiter, int crc_size) { int i; char *buf = calloc(128, sizeof(char)); + const char *format[2] = { "%08x%c", "%04x%c" }; if (!buf) return NULL; for (i = 0; i < crc->n_words; i++) - sprintf(buf + strlen(buf), "%08x ", crc->crc[i]); + sprintf(buf + strlen(buf), format[crc_size == 2], crc->crc[i], + i == (crc->n_words - 1) ? '\0' : delimiter); return buf; } +/** + * igt_crc_to_string: + * @crc: pipe CRC value to print + * + * This function allocates a string and formats @crc into it. + * The caller is responsible for freeing the string. + * + * This should only ever be used for diagnostic debug output. + */ +char *igt_crc_to_string(igt_crc_t *crc) +{ + return igt_crc_to_string_extended(crc, ' ', 4); +} + #define MAX_CRC_ENTRIES 10 #define MAX_LINE_LEN (10 + 11 * MAX_CRC_ENTRIES + 1) diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h index fe355919..f1a76406 100644 --- a/lib/igt_debugfs.h +++ b/lib/igt_debugfs.h @@ -115,6 +115,7 @@ enum intel_pipe_crc_source { void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b); bool igt_check_crc_equal(const igt_crc_t *a, const igt_crc_t *b); +char *igt_crc_to_string_extended(igt_crc_t *crc, char delimiter, int crc_size); char *igt_crc_to_string(igt_crc_t *crc); void igt_require_pipe_crc(int fd); -- 2.13.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v5 2/7] chamelium: Calculate CRC from framebuffer instead of hardcoding it
This introduces CRC calculation for reference frames, instead of using hardcoded values for them. The rendering of reference frames may differ from machine to machine, especially due to font rendering, and the frame itself may change with subsequent IGT changes. These differences would cause the CRC checks to fail on different setups. This allows them to pass regardless of the setup. Signed-off-by: Paul Kocialkowski --- lib/igt_chamelium.c | 143 lib/igt_chamelium.h | 5 ++ tests/chamelium.c | 77 +++- 3 files changed, 167 insertions(+), 58 deletions(-) diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c index 93392af7..abcdc522 100644 --- a/lib/igt_chamelium.c +++ b/lib/igt_chamelium.c @@ -94,6 +94,13 @@ struct chamelium_frame_dump { struct chamelium_port *port; }; +struct chamelium_fb_crc_async_data { + cairo_surface_t *fb_surface; + + pthread_t thread_id; + igt_crc_t *ret; +}; + struct chamelium { xmlrpc_env env; xmlrpc_client *client; @@ -998,6 +1005,142 @@ int chamelium_get_frame_limit(struct chamelium *chamelium, return ret; } +static uint32_t chamelium_xrgb_hash16(const unsigned char *buffer, int width, + int height, int k, int m) +{ + unsigned char r, g, b; + uint64_t sum = 0; + uint64_t count = 0; + uint64_t value; + uint32_t hash; + int index; + int i; + + for (i=0; i < width * height; i++) { + if ((i % m) != k) + continue; + + index = i * 4; + + r = buffer[index + 2]; + g = buffer[index + 1]; + b = buffer[index + 0]; + + value = r | (g << 8) | (b << 16); + sum += ++count * value; + } + + hash = ((sum >> 0) ^ (sum >> 16) ^ (sum >> 32) ^ (sum >> 48)) & 0x; + + return hash; +} + +static void chamelium_do_calculate_fb_crc(cairo_surface_t *fb_surface, + igt_crc_t *out) +{ + unsigned char *buffer; + int n = 4; + int w, h; + int i, j; + + buffer = cairo_image_surface_get_data(fb_surface); + w = cairo_image_surface_get_width(fb_surface); + h = cairo_image_surface_get_height(fb_surface); + + for (i = 0; i < n; i++) { + j = n - i - 1; + out->crc[i] = chamelium_xrgb_hash16(buffer, w, h, j, n); + } + + out->n_words = n; +} + +/** + * chamelium_calculate_fb_crc: + * @fd: The drm file descriptor + * @fb: The framebuffer to calculate the CRC for + * + * Calculates the CRC for the provided framebuffer, using the Chamelium's CRC + * algorithm. This calculates the CRC in a synchronous fashion. + * + * Returns: The calculated CRC + */ +igt_crc_t *chamelium_calculate_fb_crc(int fd, struct igt_fb *fb) +{ + igt_crc_t *ret = calloc(1, sizeof(igt_crc_t)); + cairo_surface_t *fb_surface; + + /* Get the cairo surface for the framebuffer */ + fb_surface = igt_get_cairo_surface(fd, fb); + + chamelium_do_calculate_fb_crc(fb_surface, ret); + + return ret; +} + +static void *chamelium_calculate_fb_crc_async_work(void *data) +{ + struct chamelium_fb_crc_async_data *fb_crc; + + fb_crc = (struct chamelium_fb_crc_async_data *) data; + + chamelium_do_calculate_fb_crc(fb_crc->fb_surface, fb_crc->ret); + + return NULL; +} + +/** + * chamelium_calculate_fb_crc_launch: + * @fd: The drm file descriptor + * @fb: The framebuffer to calculate the CRC for + * + * Launches the CRC calculation for the provided framebuffer, using the + * Chamelium's CRC algorithm. This calculates the CRC in an asynchronous + * fashion. + * + * The returned structure should be passed to a subsequent call to + * chamelium_calculate_fb_crc_result. It should not be freed. + * + * Returns: An intermediate structure for the CRC calculation work. + */ +struct chamelium_fb_crc_async_data *chamelium_calculate_fb_crc_async_start(int fd, + struct igt_fb *fb) +{ + struct chamelium_fb_crc_async_data *fb_crc; + + fb_crc = calloc(1, sizeof(struct chamelium_fb_crc_async_data)); + fb_crc->ret = calloc(1, sizeof(igt_crc_t)); + + /* Get the cairo surface for the framebuffer */ + fb_crc->fb_surface = igt_get_cairo_surface(fd, fb); + + pthread_create(&fb_crc->thread_id, NULL, + chamelium_calculate_fb_crc_async_work, fb_crc); + + return fb_crc; +} + +/** + * chamelium_calculate_fb_crc_result: + * @fb_crc: An intermediate structure with thread-related information + * + * Blocks until the asynchronous CRC calculation is finished, and then returns + * its result. + * + * Returns: The calculated CRC + */ +igt_crc_t *chamelium_calculate_fb_crc_async_finish(struct chamelium_fb_crc_async_data *fb_crc)
[Intel-gfx] [PATCH i-g-t v5 7/7] tests/chamelium: Merge the crc testing functions into one
This merges the two test_display_crc_single and test_display_crc_multiple functions into one, with a variable number of frames to capture. This reduces code duplication. Signed-off-by: Paul Kocialkowski --- tests/chamelium.c | 72 +++ 1 file changed, 8 insertions(+), 64 deletions(-) diff --git a/tests/chamelium.c b/tests/chamelium.c index 1567386e..34448152 100644 --- a/tests/chamelium.c +++ b/tests/chamelium.c @@ -420,65 +420,7 @@ disable_output(data_t *data, } static void -test_display_crc_single(data_t *data, struct chamelium_port *port) -{ - igt_display_t display; - igt_output_t *output; - igt_plane_t *primary; - igt_crc_t *crc; - igt_crc_t *expected_crc; - struct chamelium_fb_crc_async_data *fb_crc; - struct igt_fb fb; - drmModeModeInfo *mode; - drmModeConnector *connector; - int fb_id, i, captured_frame_count; - - reset_state(data, port); - - output = prepare_output(data, &display, port); - connector = chamelium_port_get_connector(data->chamelium, port, false); - primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); - igt_assert(primary); - - for (i = 0; i < connector->count_modes; i++) { - mode = &connector->modes[i]; - fb_id = igt_create_color_pattern_fb(data->drm_fd, - mode->hdisplay, - mode->vdisplay, - DRM_FORMAT_XRGB, - LOCAL_DRM_FORMAT_MOD_NONE, - 0, 0, 0, &fb); - igt_assert(fb_id > 0); - - fb_crc = chamelium_calculate_fb_crc_async_start(data->drm_fd, - &fb); - enable_output(data, port, output, mode, &fb); - - igt_debug("Testing single CRC fetch\n"); - - chamelium_capture(data->chamelium, port, 0, 0, 0, 0, 1); - crc = chamelium_read_captured_crcs(data->chamelium, - &captured_frame_count); - - expected_crc = chamelium_calculate_fb_crc_async_finish(fb_crc); - - chamelium_assert_crc_eq_or_dump(data->chamelium, expected_crc, - crc, &fb, 0); - - igt_assert_crc_equal(crc, expected_crc); - free(expected_crc); - free(crc); - - disable_output(data, port, output); - igt_remove_fb(data->drm_fd, &fb); - } - - drmModeFreeConnector(connector); - igt_display_fini(&display); -} - -static void -test_display_crc_multiple(data_t *data, struct chamelium_port *port) +test_display_crc(data_t *data, struct chamelium_port *port, int count) { igt_display_t display; igt_output_t *output; @@ -517,10 +459,12 @@ test_display_crc_multiple(data_t *data, struct chamelium_port *port) * there's always the potential the driver isn't able to keep * the display running properly for very long */ - chamelium_capture(data->chamelium, port, 0, 0, 0, 0, 3); + chamelium_capture(data->chamelium, port, 0, 0, 0, 0, count); crc = chamelium_read_captured_crcs(data->chamelium, &captured_frame_count); + igt_assert(captured_frame_count == count); + igt_debug("Captured %d frames\n", captured_frame_count); expected_crc = chamelium_calculate_fb_crc_async_finish(fb_crc); @@ -737,10 +681,10 @@ igt_main edid_id, alt_edid_id); connector_subtest("dp-crc-single", DisplayPort) - test_display_crc_single(&data, port); + test_display_crc(&data, port, 1); connector_subtest("dp-crc-multiple", DisplayPort) - test_display_crc_multiple(&data, port); + test_display_crc(&data, port, 3); connector_subtest("dp-frame-dump", DisplayPort) test_display_frame_dump(&data, port); @@ -794,10 +738,10 @@ igt_main edid_id, alt_edid_id); connector_subtest("hdmi-crc-single", HDMIA) - test_display_crc_single(&data, port); + test_display_crc(&data, port, 1); connector_subtest("hdmi-crc-multiple", HDMIA) - test_display_crc_multiple(&data, port); + test_display_crc(&data, port, 3); connector_subtest("hdmi-frame-dump", HDMIA) test_display_fram
[Intel-gfx] [PATCH i-g-t v5 6/7] chamelium: Dump captured and reference frames to png on crc error
This adds support for dumping both the frame capture from the chamelium and the reference frame generated by cairo when the captured crc does not match the crc calculated from the reference, using common helpers. Getting a dump of the frames is quite useful in order to compare them. Signed-off-by: Paul Kocialkowski --- lib/igt_chamelium.c | 81 + lib/igt_chamelium.h | 4 +++ tests/chamelium.c | 14 ++--- 3 files changed, 95 insertions(+), 4 deletions(-) diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c index abcdc522..348d2176 100644 --- a/lib/igt_chamelium.c +++ b/lib/igt_chamelium.c @@ -929,6 +929,32 @@ static pixman_image_t *convert_frame_format(pixman_image_t *src, return converted; } +static cairo_surface_t *convert_frame_dump_argb32(const struct chamelium_frame_dump *dump) +{ + cairo_surface_t *dump_surface; + pixman_image_t *image_bgr; + pixman_image_t *image_argb; + int w = dump->width, h = dump->height; + uint32_t *bits_bgr = (uint32_t *) dump->bgr; + unsigned char *bits_argb; + + image_bgr = pixman_image_create_bits( + PIXMAN_b8g8r8, w, h, bits_bgr, + PIXMAN_FORMAT_BPP(PIXMAN_b8g8r8) / 8 * w); + image_argb = convert_frame_format(image_bgr, PIXMAN_x8r8g8b8); + pixman_image_unref(image_bgr); + + bits_argb = (unsigned char *) pixman_image_get_data(image_argb); + + dump_surface = cairo_image_surface_create_for_data( + bits_argb, CAIRO_FORMAT_ARGB32, w, h, + PIXMAN_FORMAT_BPP(PIXMAN_x8r8g8b8) / 8 * w); + + pixman_image_unref(image_argb); + + return dump_surface; +} + /** * chamelium_assert_frame_eq: * @chamelium: The chamelium instance the frame dump belongs to @@ -973,6 +999,61 @@ void chamelium_assert_frame_eq(const struct chamelium *chamelium, } /** + * chamelium_assert_crc_eq_or_dump: + * @chamelium: The chamelium instance the frame dump belongs to + * @reference_crc: The CRC for the reference frame + * @capture_crc: The CRC for the captured frame + * @fb: pointer to an #igt_fb structure + * + * Asserts that the CRC provided for both the reference and the captured frame + * are identical. If they are not, this grabs the captured frame and saves it + * along with the reference to a png file. + */ +void chamelium_assert_crc_eq_or_dump(struct chamelium *chamelium, +igt_crc_t *reference_crc, +igt_crc_t *capture_crc, struct igt_fb *fb, +int index) +{ + struct chamelium_frame_dump *frame; + cairo_surface_t *reference; + cairo_surface_t *capture; + char *reference_suffix; + char *capture_suffix; + bool eq; + + eq = igt_check_crc_equal(reference_crc, capture_crc); + if (!eq && igt_frame_dump_is_enabled()) { + /* Grab the reference frame from framebuffer */ + reference = igt_get_cairo_surface(chamelium->drm_fd, fb); + + /* Grab the captured frame from chamelium */ + frame = chamelium_read_captured_frame(chamelium, index); + igt_assert(frame); + + capture = convert_frame_dump_argb32(frame); + + reference_suffix = igt_crc_to_string_extended(reference_crc, + '-', 2); + capture_suffix = igt_crc_to_string_extended(capture_crc, '-', + 2); + + /* Write reference and capture frames to png */ + igt_write_compared_frames_to_png(reference, capture, +reference_suffix, +capture_suffix); + + free(reference_suffix); + free(capture_suffix); + + chamelium_destroy_frame_dump(frame); + + cairo_surface_destroy(capture); + } + + igt_assert(eq); +} + +/** * chamelium_get_frame_limit: * @chamelium: The Chamelium instance to use * @port: The port to check the frame limit on diff --git a/lib/igt_chamelium.h b/lib/igt_chamelium.h index 2bfbfdc7..80afcafa 100644 --- a/lib/igt_chamelium.h +++ b/lib/igt_chamelium.h @@ -105,6 +105,10 @@ int chamelium_get_frame_limit(struct chamelium *chamelium, void chamelium_assert_frame_eq(const struct chamelium *chamelium, const struct chamelium_frame_dump *dump, struct igt_fb *fb); +void chamelium_assert_crc_eq_or_dump(struct chamelium *chamelium, +igt_crc_t *reference_crc, +igt_crc_t *capture_crc, struct igt_fb *fb, +int index); void chamelium_destroy_frame_dump(struct chamelium_frame_dump *dump); #endif /* IGT_CHAMELIUM_H */ diff --git a/tests/ch
[Intel-gfx] [PATCH i-g-t v5 1/7] lib/igt_fb: Export the cairo surface instead of writing to a png
This removes the igt_write_fb_to_png function (that was unused thus far) and exports the igt_get_cairo_surface function to grab the matching cairo surface. Writing to a png is now handled by the common frame handling code in lib/igt_frame. This also fixes how the surface is retreived in chamelium code, which avoids destroying it too early. Signed-off-by: Paul Kocialkowski --- lib/igt_chamelium.c | 7 +-- lib/igt_fb.c| 36 +--- lib/igt_fb.h| 2 +- 3 files changed, 15 insertions(+), 30 deletions(-) diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c index bff08c0e..93392af7 100644 --- a/lib/igt_chamelium.c +++ b/lib/igt_chamelium.c @@ -936,17 +936,13 @@ void chamelium_assert_frame_eq(const struct chamelium *chamelium, const struct chamelium_frame_dump *dump, struct igt_fb *fb) { - cairo_t *cr; cairo_surface_t *fb_surface; pixman_image_t *reference_src, *reference_bgr; int w = dump->width, h = dump->height; bool eq; /* Get the cairo surface for the framebuffer */ - cr = igt_get_cairo_ctx(chamelium->drm_fd, fb); - fb_surface = cairo_get_target(cr); - cairo_surface_reference(fb_surface); - cairo_destroy(cr); + fb_surface = igt_get_cairo_surface(chamelium->drm_fd, fb); /* * Convert the reference image into the same format as the chamelium @@ -964,7 +960,6 @@ void chamelium_assert_frame_eq(const struct chamelium *chamelium, dump->size) == 0; pixman_image_unref(reference_bgr); - cairo_surface_destroy(fb_surface); igt_fail_on_f(!eq, "Chamelium frame dump didn't match reference image\n"); diff --git a/lib/igt_fb.c b/lib/igt_fb.c index d2b7e9e3..93e21c17 100644 --- a/lib/igt_fb.c +++ b/lib/igt_fb.c @@ -1124,7 +1124,18 @@ static void create_cairo_surface__gtt(int fd, struct igt_fb *fb) fb, destroy_cairo_surface__gtt); } -static cairo_surface_t *get_cairo_surface(int fd, struct igt_fb *fb) +/** + * igt_get_cairo_surface: + * @fd: open drm file descriptor + * @fb: pointer to an #igt_fb structure + * + * This function stores the contents of the supplied framebuffer into a cairo + * surface and returns it. + * + * Returns: + * A pointer to a cairo surface with the contents of the framebuffer. + */ +cairo_surface_t *igt_get_cairo_surface(int fd, struct igt_fb *fb) { if (fb->cairo_surface == NULL) { if (fb->tiling == LOCAL_I915_FORMAT_MOD_Y_TILED || @@ -1160,7 +1171,7 @@ cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb) cairo_surface_t *surface; cairo_t *cr; - surface = get_cairo_surface(fd, fb); + surface = igt_get_cairo_surface(fd, fb); cr = cairo_create(surface); cairo_surface_destroy(surface); igt_assert(cairo_status(cr) == CAIRO_STATUS_SUCCESS); @@ -1173,27 +1184,6 @@ cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb) } /** - * igt_write_fb_to_png: - * @fd: open i915 drm file descriptor - * @fb: pointer to an #igt_fb structure - * @filename: target name for the png image - * - * This function stores the contents of the supplied framebuffer into a png - * image stored at @filename. - */ -void igt_write_fb_to_png(int fd, struct igt_fb *fb, const char *filename) -{ - cairo_surface_t *surface; - cairo_status_t status; - - surface = get_cairo_surface(fd, fb); - status = cairo_surface_write_to_png(surface, filename); - cairo_surface_destroy(surface); - - igt_assert(status == CAIRO_STATUS_SUCCESS); -} - -/** * igt_remove_fb: * @fd: open i915 drm file descriptor * @fb: pointer to an #igt_fb structure diff --git a/lib/igt_fb.h b/lib/igt_fb.h index 4a680cef..f8a845cc 100644 --- a/lib/igt_fb.h +++ b/lib/igt_fb.h @@ -132,6 +132,7 @@ int igt_create_bo_with_dimensions(int fd, int width, int height, uint32_t format uint64_t igt_fb_mod_to_tiling(uint64_t modifier); /* cairo-based painting */ +cairo_surface_t *igt_get_cairo_surface(int fd, struct igt_fb *fb); cairo_t *igt_get_cairo_ctx(int fd, struct igt_fb *fb); void igt_paint_color(cairo_t *cr, int x, int y, int w, int h, double r, double g, double b); @@ -145,7 +146,6 @@ void igt_paint_color_gradient_range(cairo_t *cr, int x, int y, int w, int h, void igt_paint_test_pattern(cairo_t *cr, int width, int height); void igt_paint_image(cairo_t *cr, const char *filename, int dst_x, int dst_y, int dst_width, int dst_height); -void igt_write_fb_to_png(int fd, struct igt_fb *fb, const char *filename); int igt_cairo_printf_line(cairo_t *cr, enum igt_text_align align, double yspacing, const char *fmt, ...) __attribute__((format (printf, 4, 5))); -- 2.13.2 ___ Intel-gfx mai
[Intel-gfx] [PATCH i-g-t v5 3/7] lib/igt_debugfs: Introduce CRC check function, with logic made common
This introduces an igt_check_crc_equal function in addition to igt_assert_crc_equal and makes the CRC comparison logic from the latter common. In particular, an igt_find_crc_mismatch function indicates whether there is a mistmatch and at what index, so that the calling functions can print the diverging values. Signed-off-by: Paul Kocialkowski --- lib/igt_debugfs.c | 53 ++--- lib/igt_debugfs.h | 1 + 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c index 2702686a..ef05dc77 100644 --- a/lib/igt_debugfs.c +++ b/lib/igt_debugfs.c @@ -281,6 +281,26 @@ bool igt_debugfs_search(int device, const char *filename, const char *substring) * Pipe CRC */ +static bool igt_find_crc_mismatch(const igt_crc_t *a, const igt_crc_t *b, + int *index) +{ + int i; + + if (a->n_words != b->n_words) + return true; + + for (i = 0; i < a->n_words; i++) { + if (a->crc[i] != b->crc[i]) { + if (index) + *index = i; + + return true; + } + } + + return false; +} + /** * igt_assert_crc_equal: * @a: first pipe CRC value @@ -294,10 +314,37 @@ bool igt_debugfs_search(int device, const char *filename, const char *substring) */ void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b) { - int i; + int index; + bool mismatch; + + mismatch = igt_find_crc_mismatch(a, b, &index); + if (mismatch) + igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index, + a->crc[index], b->crc[index]); + + igt_assert(!mismatch); +} + +/** + * igt_check_crc_equal: + * @a: first pipe CRC value + * @b: second pipe CRC value + * + * Compares two CRC values and return whether they match. + * + * Returns: A boolean indicating whether the CRC values match + */ +bool igt_check_crc_equal(const igt_crc_t *a, const igt_crc_t *b) +{ + int index; + bool mismatch; + + mismatch = igt_find_crc_mismatch(a, b, &index); + if (mismatch) + igt_debug("CRC mismatch at index %d: 0x%x != 0x%x\n", index, + a->crc[index], b->crc[index]); - for (i = 0; i < a->n_words; i++) - igt_assert_eq_u32(a->crc[i], b->crc[i]); + return !mismatch; } /** diff --git a/lib/igt_debugfs.h b/lib/igt_debugfs.h index 7b846a83..fe355919 100644 --- a/lib/igt_debugfs.h +++ b/lib/igt_debugfs.h @@ -114,6 +114,7 @@ enum intel_pipe_crc_source { }; void igt_assert_crc_equal(const igt_crc_t *a, const igt_crc_t *b); +bool igt_check_crc_equal(const igt_crc_t *a, const igt_crc_t *b); char *igt_crc_to_string(igt_crc_t *crc); void igt_require_pipe_crc(int fd); -- 2.13.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v3 1/2] lib/igt_frame: Add support for analog frame comparison testing
This adds support for analog frame comparison check, as used in VGA. Since VGA uses a DAC-ADC chain, its data cannot be expected to be pixel perfect. Thus, it is impossible to uses a CRC check and full frames have to be analyzed instead. Such an analysis is implemented, based on both an absolute error threshold and a correlation with the expected error trend for a DAC-ADC chain. It was tested with a couple encoders and provides reliable error detection with few false positives. Signed-off-by: Paul Kocialkowski --- configure.ac| 1 + lib/Makefile.am | 2 + lib/igt_frame.c | 131 lib/igt_frame.h | 2 + 4 files changed, 136 insertions(+) diff --git a/configure.ac b/configure.ac index 5b41f333..db0015e5 100644 --- a/configure.ac +++ b/configure.ac @@ -178,6 +178,7 @@ if test x"$udev" = xyes; then AC_DEFINE(HAVE_UDEV,1,[Enable udev-based monitor hotplug detection]) fi PKG_CHECK_MODULES(GLIB, glib-2.0) +PKG_CHECK_MODULES(GSL, gsl) # for chamelium AC_ARG_ENABLE(chamelium, AS_HELP_STRING([--disable-chamelium], diff --git a/lib/Makefile.am b/lib/Makefile.am index d4f41128..fb922ced 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -35,6 +35,7 @@ AM_CFLAGS = \ $(DRM_CFLAGS) \ $(PCIACCESS_CFLAGS) \ $(LIBUNWIND_CFLAGS) \ + $(GSL_CFLAGS) \ $(KMOD_CFLAGS) \ $(PROCPS_CFLAGS) \ $(DEBUG_CFLAGS) \ @@ -54,6 +55,7 @@ libintel_tools_la_LIBADD = \ $(DRM_LIBS) \ $(PCIACCESS_LIBS) \ $(PROCPS_LIBS) \ + $(GSL_LIBS) \ $(KMOD_LIBS) \ $(CAIRO_LIBS) \ $(LIBUDEV_LIBS) \ diff --git a/lib/igt_frame.c b/lib/igt_frame.c index dfafe53d..222a45f8 100644 --- a/lib/igt_frame.c +++ b/lib/igt_frame.c @@ -29,6 +29,8 @@ #include #include #include +#include +#include #include "igt.h" @@ -135,3 +137,132 @@ void igt_write_compared_frames_to_png(cairo_surface_t *reference, close(fd); } + +/** + * igt_check_analog_frame_match: + * @reference: The reference cairo surface + * @capture: The captured cairo surface + * + * Checks that the analog image contained in the chamelium frame dump matches + * the given framebuffer. + * + * In order to determine whether the frame matches the reference, the following + * reasoning is implemented: + * 1. The absolute error for each color value of the reference is collected. + * 2. The average absolute error is calculated for each color value of the + *reference and must not go above 60 (23.5 % of the total range). + * 3. A linear fit for the average absolute error from the pixel value is + *calculated, as a DAC-ADC chain is expected to have a linear error curve. + * 4. The linear fit is correlated with the actual average absolute error for + *the frame and the correlation coefficient is checked to be > 0.985, + *indicating a match with the expected error trend. + * + * Most errors (e.g. due to scaling, rotation, color space, etc) can be + * reliably detected this way, with a minimized number of false-positives. + * However, the brightest values (250 and up) are ignored as the error trend + * is often not linear there in practice due to clamping. + * + * Returns: a boolean indicating whether the frames match + */ + +bool igt_check_analog_frame_match(cairo_surface_t *reference, + cairo_surface_t *capture) +{ + pixman_image_t *reference_src, *capture_src; + int w, h; + int error_count[3][256][2] = { 0 }; + double error_average[4][250]; + double error_trend[250]; + double c0, c1, cov00, cov01, cov11, sumsq; + double correlation; + unsigned char *reference_pixels, *capture_pixels; + unsigned char *p; + unsigned char *q; + bool match = true; + int diff; + int x, y; + int i, j; + + w = cairo_image_surface_get_width(reference); + h = cairo_image_surface_get_height(reference); + + reference_src = pixman_image_create_bits( + PIXMAN_x8r8g8b8, w, h, + (void*)cairo_image_surface_get_data(reference), + cairo_image_surface_get_stride(reference)); + reference_pixels = (unsigned char *) pixman_image_get_data(reference_src); + + capture_src = pixman_image_create_bits( + PIXMAN_x8r8g8b8, w, h, + (void*)cairo_image_surface_get_data(capture), + cairo_image_surface_get_stride(capture)); + capture_pixels = (unsigned char *) pixman_image_get_data(capture_src); + + /* Collect the absolute error for each color value */ + for (x = 0; x < w; x++) { + for (y = 0; y < h; y++) { + p = &capture_pixels[(x + y * w) * 4]; + q = &reference_pixels[(x + y * w) * 4]; + + for (i = 0; i < 3; i++) { + diff = (int) p[i] - q[i]; + if (diff < 0)
[Intel-gfx] [PATCH i-g-t v3 0/2] Analogue/VGA frame comparison support
Changes since v2: * Changed analogue in favor of analog * Integrated analog frame match fixup in the original commit * Rebased on top of the new revisions of the series this depends on Changes since v1: * Split analogue frame comparison to igt_frame, using cairo surfaces * Added a chamelium helper for analogue frame checking and frame dumping ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v3 2/2] chamelium: Add support for VGA frame comparison testing
This adds support for VGA frame comparison testing with the reference generated from cairo. The retrieved frame from the chamelium is first cropped, as it contains the blanking intervals, through a dedicated helper. Another helper function asserts that the analog frame matches or dump it to png if not. Signed-off-by: Paul Kocialkowski --- lib/igt_chamelium.c | 164 ++-- lib/igt_chamelium.h | 7 ++- tests/chamelium.c | 57 ++ 3 files changed, 222 insertions(+), 6 deletions(-) diff --git a/lib/igt_chamelium.c b/lib/igt_chamelium.c index 348d2176..dcd8855f 100644 --- a/lib/igt_chamelium.c +++ b/lib/igt_chamelium.c @@ -937,6 +937,8 @@ static cairo_surface_t *convert_frame_dump_argb32(const struct chamelium_frame_d int w = dump->width, h = dump->height; uint32_t *bits_bgr = (uint32_t *) dump->bgr; unsigned char *bits_argb; + unsigned char *bits_target; + int size; image_bgr = pixman_image_create_bits( PIXMAN_b8g8r8, w, h, bits_bgr, @@ -946,9 +948,13 @@ static cairo_surface_t *convert_frame_dump_argb32(const struct chamelium_frame_d bits_argb = (unsigned char *) pixman_image_get_data(image_argb); - dump_surface = cairo_image_surface_create_for_data( - bits_argb, CAIRO_FORMAT_ARGB32, w, h, - PIXMAN_FORMAT_BPP(PIXMAN_x8r8g8b8) / 8 * w); + dump_surface = cairo_image_surface_create( + CAIRO_FORMAT_ARGB32, w, h); + + bits_target = cairo_image_surface_get_data(dump_surface); + size = cairo_image_surface_get_stride(dump_surface) * h; + memcpy(bits_target, bits_argb, size); + cairo_surface_mark_dirty(dump_surface); pixman_image_unref(image_argb); @@ -1054,6 +1060,154 @@ void chamelium_assert_crc_eq_or_dump(struct chamelium *chamelium, } /** + * chamelium_assert_analog_frame_match_or_dump: + * @chamelium: The chamelium instance the frame dump belongs to + * @frame: The chamelium frame dump to match + * @fb: pointer to an #igt_fb structure + * + * Asserts that the provided captured frame matches the reference frame from + * the framebuffer. If they do not, this saves the reference and captured frames + * to a png file. + */ +void chamelium_assert_analog_frame_match_or_dump(struct chamelium *chamelium, +struct chamelium_port *port, +const struct chamelium_frame_dump *frame, +struct igt_fb *fb) +{ + cairo_surface_t *reference; + cairo_surface_t *capture; + igt_crc_t *reference_crc; + igt_crc_t *capture_crc; + char *reference_suffix; + char *capture_suffix; + bool match; + + /* Grab the reference frame from framebuffer */ + reference = igt_get_cairo_surface(chamelium->drm_fd, fb); + + /* Grab the captured frame from chamelium */ + capture = convert_frame_dump_argb32(frame); + + match = igt_check_analog_frame_match(reference, capture); + if (!match && igt_frame_dump_is_enabled()) { + reference_crc = chamelium_calculate_fb_crc(chamelium->drm_fd, + fb); + capture_crc = chamelium_get_crc_for_area(chamelium, port, 0, 0, +0, 0); + + reference_suffix = igt_crc_to_string_extended(reference_crc, + '-', 2); + capture_suffix = igt_crc_to_string_extended(capture_crc, '-', + 2); + + /* Write reference and capture frames to png */ + igt_write_compared_frames_to_png(reference, capture, +reference_suffix, +capture_suffix); + + free(reference_suffix); + free(capture_suffix); + } + + cairo_surface_destroy(capture); + + igt_assert(match); +} + + +/** + * chamelium_analog_frame_crop: + * @chamelium: The Chamelium instance to use + * @dump: The chamelium frame dump to crop + * @width: The cropped frame width + * @height: The cropped frame height + * + * Detects the corners of a chamelium frame and crops it to the requested + * width/height. This is useful for VGA frame dumps that also contain the + * pixels dumped during the blanking intervals. + * + * The detection is done on a brightness-threshold-basis, that is adapted + * to the reference frame used by i-g-t. It may not be as relevant for other + * frames. + */ +void chamelium_crop_analog_frame(struct chamelium_frame_dump *dump, int width, +int height) +{ + unsigned char *bgr; + unsigned char *p; + unsigned char *q; + int top, left; + int
[Intel-gfx] [PATCH i-g-t v3 0/1] tests/chamelium: Detect analogue bridges and handle EDID accordingly
This patch applies on top of: Analogue/VGA frame comparison support Changes since v2: * Changed analogue in favor of analog * Rebased on top of the new revisions of the series this depends on Changes since v1: * Rebased on top of the new revisions of the series this depends on ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v3] tests/chamelium: Detect analog bridges and handle EDID accordingly
Nowadays, many VGA connectors are not actually native VGA but use a discrete bridge to a digital connector. These bridges usually enforce their own EDID instead of the one supplied by the chamelium. Thus, the EDID read test for VGA is not relevant in that case and should be skipped. Reported modes may also go beyond what the chamelium can support. Thus, only supported resolutions should be tested for the frame dump test and others should be pruned. Signed-off-by: Paul Kocialkowski --- tests/chamelium.c | 78 +++ 1 file changed, 78 insertions(+) diff --git a/tests/chamelium.c b/tests/chamelium.c index 33ecc2e7..881b7fa9 100644 --- a/tests/chamelium.c +++ b/tests/chamelium.c @@ -130,6 +130,76 @@ wait_for_connector(data_t *data, struct chamelium_port *port, igt_assert(finished); } +static int chamelium_vga_modes[][2] = { + { 1600, 1200 }, + { 1920, 1200 }, + { 1920, 1080 }, + { 1680, 1050 }, + { 1280, 1024 }, + { 1280, 960 }, + { 1440, 900 }, + { 1280, 800 }, + { 1024, 768 }, + { 1360, 768 }, + { 1280, 720 }, + { 800, 600 }, + { 640, 480 }, + { -1, -1 }, +}; + +static bool +prune_vga_mode(data_t *data, drmModeModeInfo *mode) +{ + int i = 0; + + while (chamelium_vga_modes[i][0] != -1) { + if (mode->hdisplay == chamelium_vga_modes[i][0] && + mode->vdisplay == chamelium_vga_modes[i][1]) + return false; + + i++; + } + + return true; +} + +static bool +check_analog_bridge(data_t *data, struct chamelium_port *port) +{ + drmModePropertyBlobPtr edid_blob = NULL; + drmModeConnector *connector = chamelium_port_get_connector( + data->chamelium, port, false); + uint64_t edid_blob_id; + unsigned char *edid; + char edid_vendor[3]; + + if (chamelium_port_get_type(port) != DRM_MODE_CONNECTOR_VGA) + return false; + + igt_assert(kmstest_get_property(data->drm_fd, connector->connector_id, + DRM_MODE_OBJECT_CONNECTOR, "EDID", NULL, + &edid_blob_id, NULL)); + igt_assert(edid_blob = drmModeGetPropertyBlob(data->drm_fd, + edid_blob_id)); + + edid = (unsigned char *) edid_blob->data; + + edid_vendor[0] = ((edid[8] & 0x7c) >> 2) + '@'; + edid_vendor[1] = (((edid[8] & 0x03) << 3) | + ((edid[9] & 0xe0) >> 5)) + '@'; + edid_vendor[2] = (edid[9] & 0x1f) + '@'; + + /* Analog bridges provide their own EDID */ + if (edid_vendor[0] != 'I' || edid_vendor[1] != 'G' || + edid_vendor[0] != 'T') + return true; + + drmModeFreePropertyBlob(edid_blob); + drmModeFreeConnector(connector); + + return false; +} + static void reset_state(data_t *data, struct chamelium_port *port) { @@ -193,6 +263,8 @@ test_edid_read(data_t *data, struct chamelium_port *port, chamelium_plug(data->chamelium, port); wait_for_connector(data, port, DRM_MODE_CONNECTED); + igt_skip_on(check_analog_bridge(data, port)); + igt_assert(kmstest_get_property(data->drm_fd, connector->connector_id, DRM_MODE_OBJECT_CONNECTOR, "EDID", NULL, &edid_blob_id, NULL)); @@ -547,15 +619,21 @@ test_analog_frame_dump(data_t *data, struct chamelium_port *port) drmModeModeInfo *mode; drmModeConnector *connector; int fb_id, i; + bool bridge; output = prepare_output(data, &display, port); connector = chamelium_port_get_connector(data->chamelium, port, false); primary = igt_output_get_plane_type(output, DRM_PLANE_TYPE_PRIMARY); igt_assert(primary); + bridge = check_analog_bridge(data, port); + for (i = 0; i < connector->count_modes; i++) { mode = &connector->modes[i]; + if (bridge && prune_vga_mode(data, mode)) + continue; + fb_id = igt_create_color_pattern_fb(data->drm_fd, mode->hdisplay, mode->vdisplay, DRM_FORMAT_XRGB, -- 2.13.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 9/9] drm/i915: Drop unpin stall in atomic_prepare_commit
Op 19-07-17 om 14:55 schreef Daniel Vetter: > The core already does this in setup_commit(). With this we can also > remove the unpin_work_count since it's the last user. > > Cc: Maarten Lankhorst > Cc: Ville Syrjälä > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_display.c | 13 + > drivers/gpu/drm/i915/intel_drv.h | 2 -- > 2 files changed, 1 insertion(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index e52a2adbaaa5..351208b7b1ad 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -11825,18 +11825,7 @@ static int intel_atomic_check(struct drm_device *dev, > static int intel_atomic_prepare_commit(struct drm_device *dev, > struct drm_atomic_state *state) > { > - struct drm_i915_private *dev_priv = to_i915(dev); > - struct drm_crtc_state *crtc_state; > - struct drm_crtc *crtc; > - int i, ret; > - > - for_each_new_crtc_in_state(state, crtc, crtc_state, i) { > - if (state->legacy_cursor_update) > - continue; > - > - if (atomic_read(&to_intel_crtc(crtc)->unpin_work_count) >= 2) > - flush_workqueue(dev_priv->wq); > - } > + int ret; > > ret = mutex_lock_interruptible(&dev->struct_mutex); > if (ret) > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 9cb7e781e863..96402c06e295 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -798,8 +798,6 @@ struct intel_crtc { > unsigned long long enabled_power_domains; > struct intel_overlay *overlay; > > - atomic_t unpin_work_count; > - > /* Display surface base address adjustement for pageflips. Note that on >* gen4+ this only adjusts up to a tile, offsets within a tile are >* handled in the hw itself (with the TILEOFF register). */ I like red diffs.. For patch 1, 4 (with updated commit message), 6-9: Reviewed-by: Maarten Lankhorst ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/selftests: Mark contexts as lost during freeing of mock device
We need to unpin the last retired context early in the shutdown sequence so that its RCU free is done before we try to free the context ida. I included this in a later patch ("drm/i915: Keep a recent cache of freed contexts objects for reuse") and so missed that the selftests were broken in the meantime. Reported-by: Matthew Auld Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=101627 Fixes: 5f09a9c8ab6b ("drm/i915: Allow contexts to be unreferenced locklessly") Signed-off-by: Chris Wilson Cc: Joonas Lahtinen Cc: Tvrtko Ursulin Cc: Mika Kuoppala Cc: Matthew Auld --- drivers/gpu/drm/i915/selftests/mock_gem_device.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c index d451dfbe9bbb..dda413c95b89 100644 --- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c +++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c @@ -54,6 +54,7 @@ static void mock_device_release(struct drm_device *dev) mutex_lock(&i915->drm.struct_mutex); mock_device_flush(i915); + i915_gem_contexts_lost(i915); mutex_unlock(&i915->drm.struct_mutex); cancel_delayed_work_sync(&i915->gt.retire_work); -- 2.13.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Fixes that failed to backport to v4.13-rc1
Op 19-07-17 om 15:17 schreef Jani Nikula: > Another kernel, another list of failed backports. > > The following commits have been marked as Cc: stable or fixing something > in v4.13-rc1 or earlier, but failed to cherry-pick to > drm-intel-fixes. Please see if they are worth backporting, and please do > so if they are. > > BR, > Jani. > > 54d20ed1fff2 ("drm/i915: Fix bad comparison in skl_compute_plane_wm, v2.") > v1 of this patch will apply. It was the version from before the renames that conflict here. :) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/9] drm/i915: More surgically unbreak the modeset vs reset deadlock
On Wed, Jul 19, 2017 at 3:42 PM, Chris Wilson wrote: > Quoting Daniel Vetter (2017-07-19 13:54:58) >> diff --git a/drivers/gpu/drm/i915/intel_display.c >> b/drivers/gpu/drm/i915/intel_display.c >> index 5aa7ca1ab592..4762f158032d 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -3471,10 +3471,9 @@ void intel_prepare_reset(struct drm_i915_private >> *dev_priv) >> !gpu_reset_clobbers_display(dev_priv)) >> return; >> >> - /* We have a modeset vs reset deadlock, defensively unbreak it. >> -* >> -* FIXME: We can do a _lot_ better, this is just a first iteration.*/ >> - i915_gem_set_wedged(dev_priv); >> + /* We have a modeset vs reset deadlock, defensively unbreak it. */ >> + set_bit(I915_RESET_MODESET, &dev_priv->gpu_error.flags); >> + wake_up_all(&dev_priv->gpu_error.wait_queue); > > How are we breaking the > > modeset_lock -> struct_mutex -> wait_on_reset ? > > We wait the modeset_lock next which stops the reset from > proceeding, and so the deadlock persists until the wedge-me timeout? Hm indeed, I didn't check my logs carefully enough and there's still "i915_reset_device timed out" in it. But I also thought the only real wait we have left for the gpu is the one under i915_sw_fence. I think we could simply switch i915_mutex_lock_interruptible calls in atomic modeset over mutex_lock_interruptible? Or is there another can of worms I'm missing? -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 7/9] drm/i915: adjust has_pending_fb_unpin to atomic
Quoting Daniel Vetter (2017-07-19 14:15:44) > On Wed, Jul 19, 2017 at 02:06:43PM +0100, Chris Wilson wrote: > > Quoting Daniel Vetter (2017-07-19 13:55:00) > > > A bit an oversight - the current code did nothing, since only > > > legacy flips used the unpin_work_count and assorted logic. > > > > > > Cc: Maarten Lankhorst > > > Cc: Ville Syrjälä > > > Signed-off-by: Daniel Vetter > > > > There's a fence deadlock testcase for this, kms_flip/fence? > > Crunching through them, but since I tend to test my stuff all mixed into > one pile I've hit a bug in a different patch series of mine. Given that > we've run without this for a while, not sure it's that critical really. It's only going to affect gen2 (realistically using 14 fences in a gen3 batch is unlikely) if we can have 3 fb in the pipeline as userspace assumes 2 are reserved for the flip. Or at least if we may race while fb holds 3 fences due to the wq. EDEADLK is not something I've seen outside of buggy code, but it is certainly possible and this patch should fix it. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx