Re: [Intel-gfx] [PATCH] drm/i915: Workaround to avoid lite restore with HEAD==TAIL
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6171 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -3 270/270 267/270 ILK 303/303 303/303 SNB -21 304/304 283/304 IVB 337/337 337/337 BYT -1 287/287 286/287 HSW 361/361 361/361 BDW 309/309 309/309 -Detailed- Platform Testdrm-intel-nightly Series Applied *PNV igt@gem_fence_thrash@bo-write-verify-none PASS(2) FAIL(1)PASS(1) *PNV igt@gem_fence_thrash@bo-write-verify-threaded-none PASS(3) CRASH(1)PASS(1) PNV igt@gem_userptr_blits@coherency-sync CRASH(5)PASS(3) CRASH(1)PASS(1) SNB igt@kms_cursor_crc@cursor-size-change NSPT(3)PASS(1) NSPT(2) SNB igt@kms_mmio_vs_cs_flip@setcrtc_vs_cs_flip NSPT(3)PASS(1) NSPT(2) SNB igt@kms_mmio_vs_cs_flip@setplane_vs_cs_flip NSPT(3)PASS(1) NSPT(2) SNB igt@kms_rotation_crc@primary-rotation NSPT(3)PASS(1) NSPT(2) SNB igt@kms_rotation_crc@sprite-rotation NSPT(3)PASS(3) NSPT(2) SNB igt@pm_rpm@cursor NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@cursor-dpms NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@dpms-mode-unset-non-lpsp NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@dpms-non-lpsp NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@drm-resources-equal NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@fences NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@fences-dpms NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@gem-execbuf NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@gem-mmap-cpu NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@gem-mmap-gtt NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@gem-pread NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@i2c NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@modeset-non-lpsp NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@modeset-non-lpsp-stress-no-wait NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@pci-d3-state NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@rte NSPT(3)PASS(1) NSPT(2) *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(3) FAIL(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/19] drm/i915: HSW cdclk support
Thanks for the update Mika. The issue will be that audio plays faster or slower than normal. i.e it will be 1x or 1x. can you confirm if audible sound plays after CD Clock change at 1x speed ? regards, Sivakumar On 4/14/2015 12:06 PM, Mika Kahola wrote: I tested this patch with the audio in place. With this setup in my HSW machine I can hear the pink noise played back with DP-HDMI cable attatched. speaker-test -c 2 -r 48000 -F S16_LE -t pink --device=plughw:0,7 Cheers, Mika On Tue, 2015-04-07 at 14:06 +0530, Sivakumar Thulasimani wrote: where can i check this (audio driver) ? since there was no need for them to check CD clock value till now i don't think they will be doing it. also this needs to be changed in Display Audio controller, so not sure if audio driver has access to it in the first place. will be good to confirm this before merging as it will break display audio if it is not programmed. if any one has setup this can be confirmed by just switching CD clock after boot and playing an audio file. regards, Sivakumar On 4/7/2015 1:59 PM, Ville Syrjälä wrote: On Tue, Apr 07, 2015 at 12:33:40PM +0530, Sivakumar Thulasimani wrote: sorry if i am missing something, HSW and BDW requires display audio controller to be updated with new values once CD clock is modified. how is this accomplished here ? I'm hoping the audio driver will query the cdclk frequency after every modeset. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 13/14] drm/i915: skylake primary plane scaling using shared scalers
On Tue, Apr 07, 2015 at 03:28:46PM -0700, Chandra Konduru wrote: This patch enables skylake primary plane scaling using shared scalers atomic desgin. v2: -use single copy of scaler limits (Matt) v3: -move detach_scalers to crtc commit path (Matt) -use values in plane_state-src as regular integers (me) v4: -changes to align with updated scaler structures (Matt, me) -keep plane src rect in 16.16 format (Matt, Daniel) Signed-off-by: Chandra Konduru chandra.kond...@intel.com --- drivers/gpu/drm/i915/intel_atomic.c |5 +- drivers/gpu/drm/i915/intel_display.c | 96 -- drivers/gpu/drm/i915/intel_drv.h |1 + drivers/gpu/drm/i915/intel_sprite.c |9 4 files changed, 105 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c index 2fc04ec..8f759c6 100644 --- a/drivers/gpu/drm/i915/intel_atomic.c +++ b/drivers/gpu/drm/i915/intel_atomic.c @@ -167,7 +167,7 @@ int intel_atomic_commit(struct drm_device *dev, plane-state-state = NULL; } - /* swap crtc_state */ + /* swap crtc_scaler_state */ for (i = 0; i dev-mode_config.num_crtc; i++) { struct drm_crtc *crtc = state-crtcs[i]; if (!crtc) { @@ -176,6 +176,9 @@ int intel_atomic_commit(struct drm_device *dev, to_intel_crtc(crtc)-config-scaler_state = to_intel_crtc_state(state-crtc_states[i])-scaler_state; + + if (INTEL_INFO(dev)-gen = 9) + skl_detach_scalers(to_intel_crtc(crtc)); } drm_atomic_helper_commit_planes(dev, state); diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index aa4da1f..c7ee232 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2978,6 +2978,14 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc, And one real piece of review feedback: This function is now definitely too long. Same holds for the sprite update function below. Can you (or Sonika) please follow up with a few patches to extract subroutines to make this a bit easier to read? Thanks, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/19] drm/i915: HSW cdclk support
With pink noise you can't tell if the audio is played faster or slower. I tested this by playing ITU speech codec test sample aplay --device=plughw:0,7 male.wav and audio played back on 1x speed. Cheers, Mika On Tue, 2015-04-14 at 12:27 +0530, Sivakumar Thulasimani wrote: Thanks for the update Mika. The issue will be that audio plays faster or slower than normal. i.e it will be 1x or 1x. can you confirm if audible sound plays after CD Clock change at 1x speed ? regards, Sivakumar On 4/14/2015 12:06 PM, Mika Kahola wrote: I tested this patch with the audio in place. With this setup in my HSW machine I can hear the pink noise played back with DP-HDMI cable attatched. speaker-test -c 2 -r 48000 -F S16_LE -t pink --device=plughw:0,7 Cheers, Mika On Tue, 2015-04-07 at 14:06 +0530, Sivakumar Thulasimani wrote: where can i check this (audio driver) ? since there was no need for them to check CD clock value till now i don't think they will be doing it. also this needs to be changed in Display Audio controller, so not sure if audio driver has access to it in the first place. will be good to confirm this before merging as it will break display audio if it is not programmed. if any one has setup this can be confirmed by just switching CD clock after boot and playing an audio file. regards, Sivakumar On 4/7/2015 1:59 PM, Ville Syrjälä wrote: On Tue, Apr 07, 2015 at 12:33:40PM +0530, Sivakumar Thulasimani wrote: sorry if i am missing something, HSW and BDW requires display audio controller to be updated with new values once CD clock is modified. how is this accomplished here ? I'm hoping the audio driver will query the cdclk frequency after every modeset. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm: Make integer overflow checking cover universal cursor updates (v2)
On Mon, Apr 13, 2015 at 11:06:13AM -0700, Matt Roper wrote: Our legacy SetPlane updates perform integer overflow checking on a plane's destination rectangle in drm_mode_setplane(), and atomic updates handled as part of a drm_atomic_state transaction do the same checking in drm_atomic_plane_check(). However legacy cursor updates that get routed through universal plane interfaces may bypass this overflow checking if the driver's .update_plane is serviced by the transitional plane helpers rather than the full atomic plane helpers. Move the check for destination rectangle integer overflow from the drm_mode_setplane() to __setplane_internal() so that it also covers cursor operations. This fixes an issue first noticed with i915 commit: commit ff42e093e9c9c17a6e1d6aab24875a36795f926e Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Mon Mar 2 16:35:20 2015 +0100 Revert drm/i915: Switch planes from transitional helpers to full atomic helpers The above revert switched us from full atomic helpers back to the transitional helpers, and in doing so we lost the overflow checking here for universal cursor updates. Even though such extreme cursor positions are unlikely to actually happen in the wild, we still don't want there to be a change of behavior when drivers switch from transitional helpers to full helpers. v2: Move check from setplane ioctl to setplane_internal rather than adding an additional copy of the checks to the transitional plane helpers. (Daniel) Cc: Daniel Vetter dan...@ffwll.ch Testcase: igt/kms_cursor_crc Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=84269 Signed-off-by: Matt Roper matthew.d.ro...@intel.com Applied to drm-misc, thanks. -Daniel --- drivers/gpu/drm/drm_crtc.c | 22 +++--- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index b914882..160647a 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -2507,6 +2507,17 @@ static int __setplane_internal(struct drm_plane *plane, goto out; } + /* Give drivers some help against integer overflows */ + if (crtc_w INT_MAX || + crtc_x INT_MAX - (int32_t) crtc_w || + crtc_h INT_MAX || + crtc_y INT_MAX - (int32_t) crtc_h) { + DRM_DEBUG_KMS(Invalid CRTC coordinates %ux%u+%d+%d\n, + crtc_w, crtc_h, crtc_x, crtc_y); + return -ERANGE; + } + + fb_width = fb-width 16; fb_height = fb-height 16; @@ -2591,17 +2602,6 @@ int drm_mode_setplane(struct drm_device *dev, void *data, if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EINVAL; - /* Give drivers some help against integer overflows */ - if (plane_req-crtc_w INT_MAX || - plane_req-crtc_x INT_MAX - (int32_t) plane_req-crtc_w || - plane_req-crtc_h INT_MAX || - plane_req-crtc_y INT_MAX - (int32_t) plane_req-crtc_h) { - DRM_DEBUG_KMS(Invalid CRTC coordinates %ux%u+%d+%d\n, - plane_req-crtc_w, plane_req-crtc_h, - plane_req-crtc_x, plane_req-crtc_y); - return -ERANGE; - } - /* * First, find the plane, crtc, and fb objects. If not available, * we don't bother to call the driver. -- 1.8.5.1 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 15/19] drm/i915: HSW cdclk support
I tested this patch with the audio in place. With this setup in my HSW machine I can hear the pink noise played back with DP-HDMI cable attatched. speaker-test -c 2 -r 48000 -F S16_LE -t pink --device=plughw:0,7 Cheers, Mika On Tue, 2015-04-07 at 14:06 +0530, Sivakumar Thulasimani wrote: where can i check this (audio driver) ? since there was no need for them to check CD clock value till now i don't think they will be doing it. also this needs to be changed in Display Audio controller, so not sure if audio driver has access to it in the first place. will be good to confirm this before merging as it will break display audio if it is not programmed. if any one has setup this can be confirmed by just switching CD clock after boot and playing an audio file. regards, Sivakumar On 4/7/2015 1:59 PM, Ville Syrjälä wrote: On Tue, Apr 07, 2015 at 12:33:40PM +0530, Sivakumar Thulasimani wrote: sorry if i am missing something, HSW and BDW requires display audio controller to be updated with new values once CD clock is modified. how is this accomplished here ? I'm hoping the audio driver will query the cdclk frequency after every modeset. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915: Add support for stealing purgable stolen pages
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6183 -Summary- Platform Delta drm-intel-nightly Series Applied PNV 270/270 270/270 ILK 303/303 303/303 SNB -21 304/304 283/304 IVB 337/337 337/337 BYT -1 287/287 286/287 HSW 361/361 361/361 BDW 309/309 309/309 -Detailed- Platform Testdrm-intel-nightly Series Applied SNB igt@kms_cursor_crc@cursor-size-change NSPT(3)PASS(1) NSPT(2) SNB igt@kms_mmio_vs_cs_flip@setcrtc_vs_cs_flip NSPT(3)PASS(1) NSPT(2) SNB igt@kms_mmio_vs_cs_flip@setplane_vs_cs_flip NSPT(3)PASS(1) NSPT(2) SNB igt@kms_rotation_crc@primary-rotation NSPT(3)PASS(1) NSPT(2) SNB igt@kms_rotation_crc@sprite-rotation NSPT(3)PASS(3) NSPT(2) SNB igt@pm_rpm@cursor NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@cursor-dpms NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@dpms-mode-unset-non-lpsp NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@dpms-non-lpsp NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@drm-resources-equal NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@fences NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@fences-dpms NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@gem-execbuf NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@gem-mmap-cpu NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@gem-mmap-gtt NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@gem-pread NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@i2c NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@modeset-non-lpsp NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@modeset-non-lpsp-stress-no-wait NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@pci-d3-state NSPT(3)PASS(1) NSPT(2) SNB igt@pm_rpm@rte NSPT(3)PASS(1) NSPT(2) *BYT igt@gem_exec_bad_domains@conflicting-write-domain PASS(4) FAIL(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Remove illogical/bogus Automatic mode from Broadcast RGB property
On Tue, Apr 14, 2015 at 02:22:53PM +0800, Tom Yan wrote: It's not about whether it follows every line of the spec, but whether it makes sense to follow. But I'm not going to argue about this anymore, I've written enough to show what's the logical error, yet seems I am the only one in the world who sees a problem in switching color range according to resolution and refresh rate, which I don't see any hardware vendor would/should follow either. So if you don't see a problem here, do whatever you like. You assume no one follows the spec, but all I have to do is look at the TV next to me and see that you're wrong. It's extremely unfortunate that displays can apparently get the appropriate logo without following the spec. I can only conclude that whatever conformance testing is done is insufficient. One option I was thinking would be to find some other hint in the EDID that could tell us whether the device is likely to be spec compliant, but sadly I've not found anything that would appear to work. On 13 April 2015 at 22:22, Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Mon, Apr 13, 2015 at 03:13:12PM +0100, Damien Lespiau wrote: On Wed, Apr 08, 2015 at 07:18:06PM +0800, Tom Yan wrote: https://bugzilla.kernel.org/show_bug.cgi?id=94921 As mentioned in the above bug report, switching output color range Automatically according to current mode does not make sense in computer use case. Current code seems correct to me after re-reading CEA-861-E again. However can we do better? Maybe! From the spec: The QS (AVI Q support) bit of byte 3 allows a display to declare that it supports the reception of either type of quantization range for any video format, under the direction of InfoFrame Q data (see Section 6.4 for information concerning bits Q1 and Q0). This allows a source to override the default quantization range for any video format. If the sink declares a selectable RGB Quantization Range (QS=1) then it shall expect limited range pixel values if it receives Q=1 and it shall expect full range pixel values if it receives Q=2 (see section 6.4). For other values of Q, the sink shall expect pixel values with the default range for the transmitted video format. So, for sinks that support it, we could default to sending the full range picture and overriding the quantization bit in the AVI infoframe. You could you try to run edid-decode [1] on your sink EDID to check if it supports overriding the quantization level (I added decoding the VCDB a while back). Ville, what do you think? Sure, if you can actually find a display that supports the Q bit. I've not seen one yet :( They should have just made it mandatory, otherwise I fear it's never going to catch on. -- Ville Syrjälä Intel OTC -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] eDP is black after screen rotate in kernel 3.19
On one Broadwell machine with Ubuntu 15.04, eDP is black after running xrandr -output eDP1 -rotate inverted. Only 3.19 kernel has such issue, 4.0 kernel doesn't have such issue. When this issue happens, I see the following call trace in dmesg: [ 108.777612] [drm:drm_mode_setcrtc] [CRTC:9] [ 108.777621] [drm:drm_mode_setcrtc] [CONNECTOR:20:eDP-1] [ 108.777627] [drm:intel_crtc_set_config] [CRTC:9] [FB:42] #connectors=1 (x y) (0 0) [ 108.777634] [drm:intel_set_config_compute_mode_changes] computed changes for [CRTC:9], mode_changed=0, fb_changed=0 [ 108.777639] [drm:intel_modeset_stage_output_state] [CONNECTOR:20:eDP-1] to [CRTC:9] [ 108.777659] [drm:intel_modeset_affected_pipes] set mode pipe masks: modeset: 1, prepare: 1, disable: 0 [ 108.777667] [drm:connected_sink_compute_bpp] [CONNECTOR:20:eDP-1] checking for sink bpp constrains [ 108.777675] [drm:intel_dp_compute_config] DP link computation with max lane count 4 max bw 14 pixel clock 361310KHz [ 108.777679] [drm:intel_dp_compute_config] clamping bpp for eDP panel to BIOS-provided 18 [ 108.777684] [drm:intel_dp_compute_config] DP link bw 14 lane count 4 clock 54 bpp 18 [ 108.777687] [drm:intel_dp_compute_config] DP link bw required 650358 available 1728000 [ 108.777694] [drm:intel_modeset_pipe_config] plane bpp: 24, pipe bpp: 18, dithering: 1 [ 108.777698] [drm:intel_dump_pipe_config] [CRTC:9][modeset] config for pipe A [ 108.02] [drm:intel_dump_pipe_config] cpu_transcoder: D [ 108.05] [drm:intel_dump_pipe_config] pipe bpp: 18, dithering: 1 [ 108.10] [drm:intel_dump_pipe_config] fdi/pch: 0, lanes: 0, gmch_m: 0, gmch_n: 0, link_m: 0, link_n: 0, tu: 0 [ 108.15] [drm:intel_dump_pipe_config] dp: 1, gmch_m: 3157174, gmch_n: 8388608, link_m: 701594, link_n: 1048576, tu: 64 [ 108.19] [drm:intel_dump_pipe_config] dp: 1, gmch_m2: 0, gmch_n2: 0, link_m2: 0, link_n2: 0, tu2: 0 [ 108.23] [drm:intel_dump_pipe_config] audio: 0, infoframes: 0 [ 108.26] [drm:intel_dump_pipe_config] requested mode: [ 108.33] [drm:drm_mode_debug_printmodeline] Modeline 0: 0 361310 3200 3248 3280 3316 1800 1802 1807 1816 0x0 0xa [ 108.36] [drm:intel_dump_pipe_config] adjusted mode: [ 108.42] [drm:drm_mode_debug_printmodeline] Modeline 0:3200x1800 60 361310 3200 3248 3280 3316 1800 1802 1807 1816 0x48 0xa [ 108.48] [drm:intel_dump_crtc_timings] crtc timings: 361310 3200 3248 3280 3316 1800 1802 1807 1816, type: 0x48 flags: 0xa [ 108.52] [drm:intel_dump_pipe_config] port clock: 54 [ 108.55] [drm:intel_dump_pipe_config] pipe src size: 3200x1800 [ 108.59] [drm:intel_dump_pipe_config] gmch pfit: control: 0x, ratios: 0x, lvds border: 0x [ 108.63] [drm:intel_dump_pipe_config] pch pfit: pos: 0x, size: 0x, disabled [ 108.67] [drm:intel_dump_pipe_config] ips: 1 [ 108.70] [drm:intel_dump_pipe_config] double wide: 0 [ 108.877697] [ cut here ] [ 108.877749] WARNING: CPU: 0 PID: 0 at /home/kernel/COD/linux/drivers/gpu/drm/i915/intel_display.c:9713 intel_check_page_flip+0xe6/0xf0 [i915]() [ 108.877751] Kicking stuck page flip: queued at 6212, now 6217 [ 108.877753] Modules linked in: binfmt_misc rfcomm bnep nls_iso8859_1 ax88179_178a usbnet mii hid_sensor_press hid_sensor_prox hid_sensor_als joydev hid_sensor_magn_3d hid_sensor_accel_3d hid_sensor_rotation hid_sensor_gyro_3d hid_sensor_incl_3d hid_sensor_trigger industrialio_triggered_buffer kfifo_buf industrialio hid_sensor_iio_common snd_soc_sst_broadwell snd_soc_sst_haswell_pcm hid_sensor_hub hid_multitouch snd_soc_sst_dsp btusb intel_rapl bluetooth iosf_mbi x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel arc4 kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel iwlmvm aes_x86_64 lrw gf128mul glue_helper ablk_helper mac80211 cryptd serio_raw snd_hda_codec_hdmi iwlwifi uvcvideo videobuf2_vmalloc videobuf2_memops cfg80211 videobuf2_core v4l2_common videodev media snd_hda_intel [ 108.877791] snd_hda_controller lpc_ich shpchp snd_hda_codec mei_me mei snd_hwdep processor_thermal_device i915 drm_kms_helper drm i2c_algo_bit mac_hid int3403_thermal soc_button_array winbond_cir rc_core 8250_fintek snd_soc_rt286 snd_soc_core snd_compress snd_pcm_dmaengine snd_pcm snd_seq_midi snd_seq_midi_event snd_rawmidi snd_seq snd_seq_device snd_timer i2c_hid snd hid soundcore snd_soc_sst_acpi video acpi_pad rfkill_gpio int3402_thermal i2c_designware_platform int3400_thermal i2c_designware_core dw_dmac spi_pxa2xx_platform 8250_dw dw_dmac_core acpi_thermal_rel parport_pc ppdev lp parport autofs4 e1000e ahci libahci ptp sdhci_pci pps_core sdhci_acpi sdhci [ 108.877837] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.19.4-031904-generic #201504131440 [ 108.877839] Hardware name: Intel Corporation Broadwell Client platform/Wilson Beach SDS, BIOS BDW-E2R1.86C.0080.R00.1406082006 06/08/2014 [ 108.877841] 25f1 88024e403d78
Re: [Intel-gfx] [PATCH 15/19] drm/i915: HSW cdclk support
Thanks for the confirmation, Mika :). then this change is fine not sure if it is still relevant but you can add rb if possible Reviewed-by: Sivakumar Thulasimani sivakumar.thulasim...@intel.com On 4/14/2015 12:36 PM, Mika Kahola wrote: With pink noise you can't tell if the audio is played faster or slower. I tested this by playing ITU speech codec test sample aplay --device=plughw:0,7 male.wav and audio played back on 1x speed. Cheers, Mika On Tue, 2015-04-14 at 12:27 +0530, Sivakumar Thulasimani wrote: Thanks for the update Mika. The issue will be that audio plays faster or slower than normal. i.e it will be 1x or 1x. can you confirm if audible sound plays after CD Clock change at 1x speed ? regards, Sivakumar On 4/14/2015 12:06 PM, Mika Kahola wrote: I tested this patch with the audio in place. With this setup in my HSW machine I can hear the pink noise played back with DP-HDMI cable attatched. speaker-test -c 2 -r 48000 -F S16_LE -t pink --device=plughw:0,7 Cheers, Mika On Tue, 2015-04-07 at 14:06 +0530, Sivakumar Thulasimani wrote: where can i check this (audio driver) ? since there was no need for them to check CD clock value till now i don't think they will be doing it. also this needs to be changed in Display Audio controller, so not sure if audio driver has access to it in the first place. will be good to confirm this before merging as it will break display audio if it is not programmed. if any one has setup this can be confirmed by just switching CD clock after boot and playing an audio file. regards, Sivakumar On 4/7/2015 1:59 PM, Ville Syrjälä wrote: On Tue, Apr 07, 2015 at 12:33:40PM +0530, Sivakumar Thulasimani wrote: sorry if i am missing something, HSW and BDW requires display audio controller to be updated with new values once CD clock is modified. how is this accomplished here ? I'm hoping the audio driver will query the cdclk frequency after every modeset. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/8] drm/i915/skl: Add support to load SKL CSR firmware
On 04/13/2015 10:52 PM, Damien Lespiau wrote: On Mon, Apr 13, 2015 at 08:15:29PM +0300, Imre Deak wrote: Ok, I haven't seen that. One question is if we need to support multiple interface versions or just the latest one. I would say only the latest one (for each platform) and so I915_CSR_SKL should be this latest firmware image filename, in this case i915/skl_dmc_ver4.bin. Yup, I think just supporting the latest one in the driver is what we want. The filename versioning is there so different kernel versions, supporting different interfaces, can all boot with the same userspace, each kernel loading the appropriate firmware. Can we have a symbolic link which can be hardcoded in intel_csr.c and option will be given to user during installing the firmware to create a symbolic link for the firmware wanted to load. Want to avoid version number mentioned in firmware file name as it getting change with latest fixes, bot not any API/inteface changes. Agree as bxt and skl both are gen9 we can name as skl_dmc_gen9.bin for now and discussion is going to finalize the name. Is it ok? ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Dont enable CS_PARSER_ERROR interrupts at all
Daniel Vetter daniel.vet...@ffwll.ch writes: We stopped handling them in commit aaecdf611a05cac26a94713bad25297e60225c29 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Tue Nov 4 15:52:22 2014 +0100 drm/i915: Stop gathering error states for CS error interrupts but just clearing is apparently not enough: A sufficiently dead gpu left behind by firmware (*cough* coreboot *cough*) can keep the gpu in an endless loop of such interrupts, eventually leading to the nmi firing. And definitely to what looks like a machine hang. Since we don't even enable these interrupts on gen5+ let's do the same on earlier platforms. Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=93171 Signed-off-by: Daniel Vetter daniel.vet...@intel.com Reviewed-by: Mika Kuoppala mika.kuopp...@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 8 ++-- 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 14ecb4d13a1a..6d494432b19f 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -3598,14 +3598,12 @@ static int i8xx_irq_postinstall(struct drm_device *dev) ~(I915_DISPLAY_PIPE_A_EVENT_INTERRUPT | I915_DISPLAY_PIPE_B_EVENT_INTERRUPT | I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT | - I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT | - I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT); + I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT); I915_WRITE16(IMR, dev_priv-irq_mask); I915_WRITE16(IER, I915_DISPLAY_PIPE_A_EVENT_INTERRUPT | I915_DISPLAY_PIPE_B_EVENT_INTERRUPT | - I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT | I915_USER_INTERRUPT); POSTING_READ16(IER); @@ -3767,14 +3765,12 @@ static int i915_irq_postinstall(struct drm_device *dev) I915_DISPLAY_PIPE_A_EVENT_INTERRUPT | I915_DISPLAY_PIPE_B_EVENT_INTERRUPT | I915_DISPLAY_PLANE_A_FLIP_PENDING_INTERRUPT | - I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT | - I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT); + I915_DISPLAY_PLANE_B_FLIP_PENDING_INTERRUPT); enable_mask = I915_ASLE_INTERRUPT | I915_DISPLAY_PIPE_A_EVENT_INTERRUPT | I915_DISPLAY_PIPE_B_EVENT_INTERRUPT | - I915_RENDER_COMMAND_PARSER_ERROR_INTERRUPT | I915_USER_INTERRUPT; if (I915_HAS_HOTPLUG(dev)) { -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for takeover
On Mon, Apr 13, 2015 at 04:03:03PM +0100, Tvrtko Ursulin wrote: From: Tvrtko Ursulin tvrtko.ursu...@intel.com intel_user_framebuffer_destroy() requires the struct_mutex for its object bookkeeping, so this means that all calls to drm_framebuffer_unreference must be held without that lock. This is a simplified version of the identically named patch by Chris Wilson. Regression from commit ab8d66752a9c28cd6c94fa173feacdfc1554aa03 Author: Tvrtko Ursulin tvrtko.ursu...@intel.com Date: Mon Feb 2 15:44:15 2015 + drm/i915: Track old framebuffer instead of object v2: Bikeshedding. References: https://bugs.freedesktop.org/show_bug.cgi?id=89166 Cc: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com Cc: Daniel Vetter daniel.vet...@ffwll.ch Signed-off-by: Tvrtko Ursulin tvrtko.ursu...@intel.com Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_display.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 97922fb..5fb11bc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14739,6 +14739,7 @@ void intel_modeset_gem_init(struct drm_device *dev) struct drm_i915_private *dev_priv = dev-dev_private; struct drm_crtc *c; struct drm_i915_gem_object *obj; + int ret; mutex_lock(dev-struct_mutex); intel_init_gt_powersave(dev); @@ -14763,16 +14764,18 @@ void intel_modeset_gem_init(struct drm_device *dev) * pinned fenced. When we do the allocation it's too early * for this. */ - mutex_lock(dev-struct_mutex); for_each_crtc(dev, c) { obj = intel_fb_obj(c-primary-fb); if (obj == NULL) continue; - if (intel_pin_and_fence_fb_obj(c-primary, -c-primary-fb, -c-primary-state, -NULL)) { + mutex_lock(dev-struct_mutex); + ret = intel_pin_and_fence_fb_obj(c-primary, + c-primary-fb, + c-primary-state, + NULL); + mutex_unlock(dev-struct_mutex); + if (ret) { DRM_ERROR(failed to pin boot fb on pipe %d\n, to_intel_crtc(c)-pipe); drm_framebuffer_unreference(c-primary-fb); @@ -14780,7 +14783,6 @@ void intel_modeset_gem_init(struct drm_device *dev) update_state_fb(c-primary); } } - mutex_unlock(dev-struct_mutex); intel_backlight_register(dev); } -- 2.3.5 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 26/49] drm/i915/bxt: Add BXT support in gen8_irq functions
On Mon, 13 Apr 2015, Imre Deak imre.d...@intel.com wrote: From: Shashank Sharma shashank.sha...@intel.com This patch adds conditional checks in gen8_irq functions to support BXT. Most of the checks just look for PCH split availability, and block the call to PCH interrupt functions if not available. v2: (jani) - drop redundant TODO comment about PCH IRQ flags on BXT - check HAS_PCH_SPLIT instead of IS_BROXTON when handling PCH specific IRQ events in gen8_irq_handler() - check HAS_PCH_SPLIT before calling the function instead of a corresponding early return within the called function for ibx_irq_reset(), ibx_irq_pre_postinstall(), ibx_irq_postinstall() v3: (jani) - in ironlake_irq_postinstall() and ironlake_irq_reset() HAS_PCH_SPLIT is always true, so drop the check for it Reviewed-by: Satheeshakrishna Msatheeshakrishn...@intel.com Signed-off-by: Damien Lespiau damien.lesp...@intel.com Signed-off-by: Shashank Sharma ppashank.sha...@intel.com (v1) Signed-off-by: Imre Deak imre.d...@intel.com Reviewed-by: Jani Nikula jani.nik...@intel.com --- drivers/gpu/drm/i915/i915_irq.c | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index b06364f..b0cd7a9 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2297,7 +2297,8 @@ static irqreturn_t gen8_irq_handler(int irq, void *arg) DRM_ERROR(The master control interrupt lied (DE PIPE)!\n); } - if (!HAS_PCH_NOP(dev) master_ctl GEN8_DE_PCH_IRQ) { + if (HAS_PCH_SPLIT(dev) !HAS_PCH_NOP(dev) + master_ctl GEN8_DE_PCH_IRQ) { /* * FIXME(BDW): Assume for now that the new interrupt handling * scheme also closed the SDE interrupt handling race we've seen @@ -3133,7 +3134,8 @@ static void gen8_irq_reset(struct drm_device *dev) GEN5_IRQ_RESET(GEN8_DE_MISC_); GEN5_IRQ_RESET(GEN8_PCU_); - ibx_irq_reset(dev); + if (HAS_PCH_SPLIT(dev)) + ibx_irq_reset(dev); } void gen8_irq_power_well_post_enable(struct drm_i915_private *dev_priv, @@ -3545,12 +3547,14 @@ static int gen8_irq_postinstall(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev-dev_private; - ibx_irq_pre_postinstall(dev); + if (HAS_PCH_SPLIT(dev)) + ibx_irq_pre_postinstall(dev); gen8_gt_irq_postinstall(dev_priv); gen8_de_irq_postinstall(dev_priv); - ibx_irq_postinstall(dev); + if (HAS_PCH_SPLIT(dev)) + ibx_irq_postinstall(dev); I915_WRITE(GEN8_MASTER_IRQ, DE_MASTER_IRQ_CONTROL); POSTING_READ(GEN8_MASTER_IRQ); -- 1.9.1 -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Remove illogical/bogus Automatic mode from Broadcast RGB property
It's not about whether it follows every line of the spec, but whether it makes sense to follow. But I'm not going to argue about this anymore, I've written enough to show what's the logical error, yet seems I am the only one in the world who sees a problem in switching color range according to resolution and refresh rate, which I don't see any hardware vendor would/should follow either. So if you don't see a problem here, do whatever you like. On 13 April 2015 at 22:22, Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Mon, Apr 13, 2015 at 03:13:12PM +0100, Damien Lespiau wrote: On Wed, Apr 08, 2015 at 07:18:06PM +0800, Tom Yan wrote: https://bugzilla.kernel.org/show_bug.cgi?id=94921 As mentioned in the above bug report, switching output color range Automatically according to current mode does not make sense in computer use case. Current code seems correct to me after re-reading CEA-861-E again. However can we do better? Maybe! From the spec: The QS (AVI Q support) bit of byte 3 allows a display to declare that it supports the reception of either type of quantization range for any video format, under the direction of InfoFrame Q data (see Section 6.4 for information concerning bits Q1 and Q0). This allows a source to override the default quantization range for any video format. If the sink declares a selectable RGB Quantization Range (QS=1) then it shall expect limited range pixel values if it receives Q=1 and it shall expect full range pixel values if it receives Q=2 (see section 6.4). For other values of Q, the sink shall expect pixel values with the default range for the transmitted video format. So, for sinks that support it, we could default to sending the full range picture and overriding the quantization bit in the AVI infoframe. You could you try to run edid-decode [1] on your sink EDID to check if it supports overriding the quantization level (I added decoding the VCDB a while back). Ville, what do you think? Sure, if you can actually find a display that supports the Q bit. I've not seen one yet :( They should have just made it mandatory, otherwise I fear it's never going to catch on. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation
On 4/14/2015 5:09 AM, Matt Roper wrote: On Mon, Apr 13, 2015 at 01:49:22PM +0300, Ville Syrjälä wrote: On Mon, Apr 13, 2015 at 03:53:22PM +0530, Jindal, Sonika wrote: On 4/13/2015 3:40 PM, Ville Syrjälä wrote: On Mon, Apr 13, 2015 at 09:36:02AM +0530, Jindal, Sonika wrote: On 4/10/2015 8:14 PM, Ville Syrjälä wrote: On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote: On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote: v2: Moving creation of property in a function, checking for 90/270 rotation simultaneously (Chris) Letting primary plane to be positioned v3: Adding if/else for 90/270 and rest params programming, adding check for pixel_format, some cleanup (review comments) v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset and size programming (Ville) v5: Rebased on -nightly and Tvrtko's series for gtt remapping. v6: Rebased on -nightly (Tvrtko's series merged) v7: Moving pixel_format check to intel_atomic_plane_check (Matt) Signed-off-by: Sonika Jindal sonika.jin...@intel.com Reviewed-by: Matt Roper matthew.d.ro...@intel.com Patches are missing the Testcase: tag, please add that. Also, are all the igt committed or not yet? Pulled these two in meanwhile. Things are going somewhat broken because you didn't apply my plane state stuff. Hmm. Actually it sort of looks that it might work by luck as long as the primary plane doesn't get clipped since this is bashing the user state directly into the hardware registers and not the derived state (ie. clipped coordinates). I was hoping your changes get merged, but not sure why they are held up. Also I see my review comment about the 90 vs. 270 rotation mixup was ignored at least. I never really got the to understand the need of reversing 90 and 270 :) The last discussion was not concluded, AFAIR. Things look correct to me and work fine with the testcase also. Is there something completely different which I am unable to get? BSpec gives me the impression the hw rotation is cw, whereas the drm one is ccw. Yes, HW rotation is cw as per bspec. In drm, where do we consider it as anti-cw? I assume it is cw because all the macros work as expected, ie cw. The ccw rule was inherited from XRandR. I'm not sure what macros you mean, but drm_rect_rotate() will certainly give you wrong results if you assume cw. It seems like this information needs to be added to the property section of the DRM DocBook; continuing to follow XRandR probably makes sense if that's what's agreed on, but there's no indication of that design philosophy in the actual DRM documentation today, so we're in danger of having different driver authors use conflicting interpretations. Ok, I will create a DocBook patch with description for 90/270 rotation (Anyways 90/270 rotation is not added in the rotation property in documentation). Will there be any other place for this or i915's rotation property? Also, I will send a patch to flip 90/270 in i915. I am just worried, that it will look confusing to check for DRM_ROTATE_90 and use PLANE_CTL_ROTATE_270 for programming. I will add a comment though. Sounds good? -Sonika 90/270 rotation is already supported by existing drivers (omap for several years now and atmel-hlcdc just recently). I think it's too late to try to redefine the meaning of rotation property values that are already in active use, so we probably need to check whether omap is using a cw or ccw scheme and follow (and document!) that. Matt -Sonika -Daniel --- drivers/gpu/drm/i915/i915_reg.h |2 + drivers/gpu/drm/i915/intel_atomic_plane.c | 24 drivers/gpu/drm/i915/intel_display.c | 88 - drivers/gpu/drm/i915/intel_drv.h |6 ++ drivers/gpu/drm/i915/intel_sprite.c | 52 - 5 files changed, 131 insertions(+), 41 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index b522eb6..564bbd5 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -4854,7 +4854,9 @@ enum skl_disp_power_wells { #define PLANE_CTL_ALPHA_HW_PREMULTIPLY( 3 4) #define PLANE_CTL_ROTATE_MASK 0x3 #define PLANE_CTL_ROTATE_00x0 +#define PLANE_CTL_ROTATE_90 0x1 #define PLANE_CTL_ROTATE_180 0x2 +#define PLANE_CTL_ROTATE_270 0x3 #define _PLANE_STRIDE_1_A 0x70188 #define _PLANE_STRIDE_2_A 0x70288 #define _PLANE_STRIDE_3_A 0x70388 diff --git a/drivers/gpu/drm/i915/intel_atomic_plane.c b/drivers/gpu/drm/i915/intel_atomic_plane.c index 976b891..a27ee8c 100644 --- a/drivers/gpu/drm/i915/intel_atomic_plane.c +++ b/drivers/gpu/drm/i915/intel_atomic_plane.c @@ -162,6 +162,30 @@ static int intel_plane_atomic_check(struct drm_plane *plane, (1
[Intel-gfx] (no subject)
This series is revised based on Jani's good comments. In this series the patch which read out DP link training parameters from VBT is discarded as based on the comments that I received. Files changed: drivers/gpu/drm/i915/intel_dp.c drivers/gpu/drm/i915/intel_drv.h ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: DP link training optimization
This is a first of series patches that optimize DP link training. The first patch is for eDP only where we reuse the previously trained link training values from cache i.e. voltage swing and pre-emphasis levels. In case we are not able to train the link by reusing the known values, the link training parameters are set to zero and training is restarted. V2: - flag that indicates if DP link is trained and valid renamed from 'link_trained' to 'train_set_valid' - removed routine 'intel_dp_reuse_link_train' Signed-off-by: Mika Kahola mika.kah...@intel.com --- drivers/gpu/drm/i915/intel_dp.c | 28 ++-- drivers/gpu/drm/i915/intel_drv.h | 1 + 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 14cdd00..b6f1092 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3474,7 +3474,8 @@ static bool intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, uint8_t dp_train_pat) { - memset(intel_dp-train_set, 0, sizeof(intel_dp-train_set)); + if (!intel_dp-train_set_valid) + memset(intel_dp-train_set, 0, sizeof(intel_dp-train_set)); intel_dp_set_signal_levels(intel_dp, DP); return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); } @@ -3587,6 +3588,23 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) break; } + /* +* if we used previously trained voltage and pre-emphasis values +* and we don't get clock recovery, reset link training values +*/ + if (intel_dp-train_set_valid) { + DRM_DEBUG_KMS(clock recovery not ok, reset); + /* clear the flag as we are not reusing train set */ + intel_dp-train_set_valid = false; + if (!intel_dp_reset_link_train(intel_dp, DP, + DP_TRAINING_PATTERN_1 | + DP_LINK_SCRAMBLING_DISABLE)) { + DRM_ERROR(failed to enable link training\n); + return; + } + continue; + } + /* Check to see if we've tried the max voltage */ for (i = 0; i intel_dp-lane_count; i++) if ((intel_dp-train_set[i] DP_TRAIN_MAX_SWING_REACHED) == 0) @@ -3664,6 +3682,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) /* Make sure clock is still ok */ if (!drm_dp_clock_recovery_ok(link_status, intel_dp-lane_count)) { + intel_dp-train_set_valid = false; intel_dp_start_link_train(intel_dp); intel_dp_set_link_train(intel_dp, DP, training_pattern | @@ -3679,6 +3698,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) /* Try 5 times, then try clock recovery if that fails */ if (tries 5) { + intel_dp-train_set_valid = false; intel_dp_start_link_train(intel_dp); intel_dp_set_link_train(intel_dp, DP, training_pattern | @@ -3700,8 +3720,10 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) intel_dp-DP = DP; - if (channel_eq) + if (channel_eq) { DRM_DEBUG_KMS(Channel EQ done. DP Training successful\n); + intel_dp-train_set_valid = is_edp(intel_dp); + } } @@ -4682,6 +4704,8 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) intel_display_power_get(dev_priv, power_domain); if (long_hpd) { + /* indicate that we need to restart link training */ + intel_dp-train_set_valid = false; if (HAS_PCH_SPLIT(dev)) { if (!ibx_digital_port_connected(dev_priv, intel_dig_port)) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 6a2ee0c..2845fb9 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -739,6 +739,7 @@ struct intel_dp { bool has_aux_irq, int send_bytes, uint32_t aux_clock_divider); + bool train_set_valid; }; struct intel_digital_port { -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/chv: Implement WaDisableShadowRegForCpd
From: Deepak S deepa...@linux.intel.com This WA disable usage of shadow register during CPD/RC6 transactions on CHV Signed-off-by: Deepak S deepa...@linux.intel.com --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 4 2 files changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 9c97842..bcdb16b 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6206,6 +6206,7 @@ enum skl_disp_power_wells { #define GTFIFOCTL 0x120008 #defineGT_FIFO_FREE_ENTRIES_MASK 0x7f #defineGT_FIFO_NUM_RESERVED_ENTRIES20 +#define GT_FIFO_CTL_BLOCK_POLICY (311) #define HSW_IDICR 0x9008 #defineIDIHASHMSK(x) (((x) 0x3f) 16) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 4dd8b41..b9d3c00 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6431,6 +6431,10 @@ static void cherryview_init_clock_gating(struct drm_device *dev) /* WaDisableSDEUnitClockGating:chv */ I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) | GEN8_SDEUNIT_CLOCK_GATE_DISABLE); + + /* WaDisableShadowRegForCpd */ + I915_WRITE(GTFIFOCTL, I915_READ(GTFIFOCTL) | + GT_FIFO_CTL_BLOCK_POLICY); } static void g4x_init_clock_gating(struct drm_device *dev) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v5] tests/gem_mmap_gtt: add huge BO test
Add a straightforward test that allocates a BO that is bigger than (by 1 page currently) the mappable aperture, tests mmap access to it by CPU directly and through GTT in sequence. Currently it is expected for the GTT access to gracefully fail as all objects are attempted to get pinned to GTT completely for mmap access. Once the partial view support is merged to kernel, the test should pass for all parts. v2: - Corrected BO domain handling (Chris Wilson) - Check again after GTT access for added paranoia (Chris Wilson) v3: - Avoid flush by using pread (Chris Wilson) - Free gtt_pattern buffer too. v4: - Add more comments (Tvrtko Ursulin) - Use igt_require (Tvrtko Ursulin) v5: - Remove wrong message from igt_require_f (Tvrtko Ursulin) - After digging deeper to it, just igt_assert that the CPU mapping needs to succeed. Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com Cc: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Joonas Lahtinen joonas.lahti...@linux.intel.com --- tests/gem_mmap_gtt.c | 106 +++ 1 file changed, 106 insertions(+) diff --git a/tests/gem_mmap_gtt.c b/tests/gem_mmap_gtt.c index 115e398..487eecf 100644 --- a/tests/gem_mmap_gtt.c +++ b/tests/gem_mmap_gtt.c @@ -41,6 +41,10 @@ #include drmtest.h #include igt_debugfs.h +#ifndef PAGE_SIZE +#define PAGE_SIZE 4096 +#endif + static int OBJECT_SIZE = 16*1024*1024; static void @@ -55,6 +59,20 @@ set_domain_cpu(int fd, uint32_t handle) gem_set_domain(fd, handle, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU); } +static void +pread_bo(int fd, int handle, void *buf, int offset, int size) +{ + struct drm_i915_gem_pread gem_pread; + + memset(gem_pread, 0, sizeof(gem_pread)); + gem_pread.handle = handle; + gem_pread.data_ptr = (uintptr_t)buf; + gem_pread.size = size; + gem_pread.offset = offset; + + igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_PREAD, gem_pread) == 0); +} + static void * mmap_bo(int fd, uint32_t handle) { @@ -265,6 +283,92 @@ test_write_gtt(int fd) } static void +test_huge_bo(int fd) +{ + uint32_t bo; + char *ptr_cpu; + char *ptr_gtt; + char *cpu_pattern; + char *gtt_pattern; + uint64_t mappable_aperture_pages = gem_mappable_aperture_size() / + PAGE_SIZE; + uint64_t huge_object_size = (mappable_aperture_pages + 1) * PAGE_SIZE; + uint64_t last_offset = huge_object_size - PAGE_SIZE; + + cpu_pattern = malloc(PAGE_SIZE); + gtt_pattern = malloc(PAGE_SIZE); + igt_assert(cpu_pattern gtt_pattern); + memset(cpu_pattern, 0xaa, PAGE_SIZE); + memset(gtt_pattern, ~0xaa, PAGE_SIZE); + + bo = gem_create(fd, huge_object_size); + + /* Obtain CPU mapping for the object. */ + ptr_cpu = gem_mmap__cpu(fd, bo, 0, huge_object_size, + PROT_READ | PROT_WRITE); + igt_assert(ptr_cpu); + + set_domain_cpu(fd, bo); + + /* Write first page through the mapping and assert reading it back +* works. */ + memcpy(ptr_cpu, cpu_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_cpu, cpu_pattern, PAGE_SIZE) == 0); + + /* Write last page through the mapping and assert reading it back +* works. */ + memcpy(ptr_cpu + last_offset, cpu_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_cpu + last_offset, cpu_pattern, PAGE_SIZE) == 0); + + /* Cross check that accessing two simultaneous pages works. */ + igt_assert(memcmp(ptr_cpu, ptr_cpu + last_offset, PAGE_SIZE) == 0); + + munmap(ptr_cpu, huge_object_size); + ptr_cpu = NULL; + + /* Obtain mapping for the object through GTT. */ + ptr_gtt = gem_mmap__gtt(fd, bo, huge_object_size, + PROT_READ | PROT_WRITE); + igt_require_f(ptr_gtt, Huge BO GTT mapping not supported.\n); + + set_domain_gtt(fd, bo); + + /* Access through GTT should still provide the CPU written values. */ + igt_assert(memcmp(ptr_gtt , cpu_pattern, PAGE_SIZE) == 0); + igt_assert(memcmp(ptr_gtt + last_offset, cpu_pattern, PAGE_SIZE) == 0); + + /* Try replacing first page through GTT mapping and make sure other page +* stays intact. */ + memcpy(ptr_gtt, gtt_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_gtt , gtt_pattern, PAGE_SIZE) == 0); + igt_assert(memcmp(ptr_gtt + last_offset, cpu_pattern, PAGE_SIZE) == 0); + + /* And make sure that after writing, both pages contain what they +* should.*/ + memcpy(ptr_gtt + last_offset, gtt_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_gtt , gtt_pattern, PAGE_SIZE) == 0); + igt_assert(memcmp(ptr_gtt + last_offset, gtt_pattern, PAGE_SIZE) == 0); + + munmap(ptr_gtt, huge_object_size); + ptr_gtt = NULL; + + /* Verify the page contents after GTT writes by reading without mapping. +*
Re: [Intel-gfx] [PATCH 07/11] drm/i915: Update intel_dp_hpd_pulse() for non-MST operation
2015-04-10 13:12 GMT-03:00 Todd Previte tprev...@gmail.com: Update the hot plug function to handle the SST case. Instead of placing the SST case within the long/short pulse block, it is now handled after determining that MST mode is not in use. This way, the topology management layer can handle any MST-related operations while SST operations are still correctly handled afterwards. This patch also corrects the problem of SST mode only being handled in the case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes both short and long pulses are used by the different tests, thus both cases need to be addressed for SST. This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the previous compliance testing patch sequence. Review feedback on that patch indicated that updating intel_dp_hot_plug() was not the correct place for the test handler. For the SST case, the main stream is disabled for long HPD pulses as this generally indicates either a connect/disconnect event or link failure. For a number of case in compliance testing, the source is required to disable the main link upon detection of a long HPD. V2: - N/A V3: - Place the SST mode link status check into the mst_fail case - Remove obsolete comment regarding SST mode operation - Removed an erroneous line of code that snuck in during rebasing V4: - Added a disable of the main stream (DP transport) for the long pulse case for SST to support compliance testing V5: - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 V6: - Reformatted a comment Signed-off-by: Todd Previte tprev...@gmail.com --- drivers/gpu/drm/i915/intel_dp.c | 19 --- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 77b6b15..ba2da44 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4572,29 +4572,26 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) if (intel_dp_check_mst_status(intel_dp) == -EINVAL) goto mst_fail; } - - if (!intel_dp-is_mst) { - /* -* we'll check the link status via the normal hot plug path later - -* but for short hpds we should check it now -*/ - drm_modeset_lock(dev-mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); - drm_modeset_unlock(dev-mode_config.connection_mutex); - } } Shouldn't the code be moved to exactly this spot instead of after the put_power label? Why would we want to call check_link_status in case we goto mst_fail? In case there is a valid reason, maybe it would be better to do a big reorganization of this function because it's going to start looking very weird - or at least rename the labels. Also, for the long_hpd case, I see that check_link_status() will redo some of the stuff we already did on this function, such as get_dpcd(). And if you follow my advice on patch 2, you will end up having even more repeated code. I think you could try to do a careful analysis here to make sure we're not calling stuff twice here, especially since some of those operations are potentially slow. ret = IRQ_HANDLED; goto put_power; mst_fail: - /* if we were in MST mode, and device is not there get out of MST mode */ if (intel_dp-is_mst) { + /* if we were in MST mode, and device is not there get out of MST mode */ I don't see the need for changes such as the one above - I saw similar cases in other patches you submitted. I often use git blame on comments in order to be able to see the whole context of the change, and a simple change like this makes it harder to blame. Also, you're not even fixing the 80 column problem here. And I do prefer the comment on top of the if statement. DRM_DEBUG_KMS(MST device may have disappeared %d vs %d\n, intel_dp-is_mst, intel_dp-mst_mgr.mst_state); intel_dp-is_mst = false; drm_dp_mst_topology_mgr_set_mst(intel_dp-mst_mgr, intel_dp-is_mst); } put_power: + /* SST mode - handle short/long pulses here */ + if (!intel_dp-is_mst) { + drm_modeset_lock(dev-mode_config.connection_mutex, NULL); + intel_dp_check_link_status(intel_dp); + drm_modeset_unlock(dev-mode_config.connection_mutex); + ret = IRQ_HANDLED; + } intel_display_power_put(dev_priv, power_domain); return ret; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni
[Intel-gfx] [PATCH] drm/i915/skl: Add back HDMI translation table
The HDMI translation table is added back to bspec, so adding it, and defaulting the 800mV+0dB entry. Cc: Damien Lespiau damien.lesp...@intel.com Signed-off-by: Sonika Jindal sonika.jin...@intel.com --- drivers/gpu/drm/i915/intel_ddi.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 5b50484..b974f8e 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -155,8 +155,17 @@ static const struct ddi_buf_trans skl_ddi_translations_edp[] = { static const struct ddi_buf_trans skl_ddi_translations_hdmi[] = { - /* Idx NT mV T mVdb */ - { 0x4014, 0x0087 }, /* 0: 800 10002 */ + { 0x0018, 0x00ac }, + { 0x5012, 0x009d }, + { 0x7011, 0x0088 }, + { 0x0018, 0x00a1 }, + { 0x0018, 0x0098 }, + { 0x4013, 0x0088 }, + { 0x6012, 0x0087 }, + { 0x0018, 0x00df }, + { 0x3015, 0x0087 }, + { 0x3015, 0x00c7 }, + { 0x0018, 0x00c7 }, }; enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder) @@ -214,16 +223,9 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port) n_edp_entries = ARRAY_SIZE(skl_ddi_translations_dp); } - /* -* On SKL, the recommendation from the hw team is to always use -* a certain type of level shifter (and thus the corresponding -* 800mV+2dB entry). Given that's the only validated entry, we -* override what is in the VBT, at least until further notice. -*/ - hdmi_level = 0; ddi_translations_hdmi = skl_ddi_translations_hdmi; n_hdmi_entries = ARRAY_SIZE(skl_ddi_translations_hdmi); - hdmi_default_entry = 0; + hdmi_default_entry = 7; } else if (IS_BROADWELL(dev)) { ddi_translations_fdi = bdw_ddi_translations_fdi; ddi_translations_dp = bdw_ddi_translations_dp; -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/8] drm/i915/skl: Implement enable/disable for Display C5 sttate.
On Mon, Apr 13, 2015 at 04:51:37PM +0300, Imre Deak wrote: On ma, 2015-04-13 at 14:17 +0100, Damien Lespiau wrote: On Thu, Apr 02, 2015 at 06:58:22PM +0300, Imre Deak wrote: On ke, 2015-04-01 at 16:22 +0530, Animesh Manna wrote: From: A.Sunil Kamath sunil.kam...@intel.com This patch just implements the basic enable and disable functions of DC5 state which is needed for both SKL and BXT. Reviewed-by: Imre Deak imre.d...@intel.com For the record, this patch generates compilation warnings when applied on its own: drivers/gpu/drm/i915/intel_runtime_pm.c:368:13: warning: ‘gen9_enable_dc5’ defined but not used [-Wunused-function] static void gen9_enable_dc5(struct drm_i915_private *dev_priv) ^ drivers/gpu/drm/i915/intel_runtime_pm.c:386:13: warning: ‘gen9_disable_dc5’ defined but not used [-Wunused-function] static void gen9_disable_dc5(struct drm_i915_private *dev_priv) ^ Generally speaking, in a series, each step should compile without warning and result in a working driver (for bisectability). Yes, agreed. Splitting the changes into patches could've been done in better a way. I also commented on a related topic of adding something in the patchset and removing it later. We'll try to pay more attention to this in the future. Animesh, if you resend this patchset anyway I think you could switch the order of 2/8 and 3/8 and add the calls to the above functions in this patch to get rid of the warnings. Also please make sure that things compile without a warning after each patch as Damien suggested. Yeah generally I don't like when patches add functions and structures and don't use them - in the end it's fairly hard to review something if you don't know how it's getting called used, which means you can't read the patches linearly. And the point of splitting them is to give reviewers small digestable chunks instead of the full thing. Imo it's better to split things the other way round, i.e. wire up stub functions at first, then fill them out. Instead of the first patch adding the implementation and the 2nd one wiring things up. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Move drm_framebuffer_unreference out of struct_mutex for takeover
On Tue, 14 Apr 2015, Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Mon, Apr 13, 2015 at 04:03:03PM +0100, Tvrtko Ursulin wrote: From: Tvrtko Ursulin tvrtko.ursu...@intel.com intel_user_framebuffer_destroy() requires the struct_mutex for its object bookkeeping, so this means that all calls to drm_framebuffer_unreference must be held without that lock. This is a simplified version of the identically named patch by Chris Wilson. Regression from commit ab8d66752a9c28cd6c94fa173feacdfc1554aa03 Author: Tvrtko Ursulin tvrtko.ursu...@intel.com Date: Mon Feb 2 15:44:15 2015 + drm/i915: Track old framebuffer instead of object v2: Bikeshedding. References: https://bugs.freedesktop.org/show_bug.cgi?id=89166 Cc: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com Cc: Daniel Vetter daniel.vet...@ffwll.ch Signed-off-by: Tvrtko Ursulin tvrtko.ursu...@intel.com Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com Pushed to drm-intel-next-fixes, thanks for the patch and review. BR, Jani. --- drivers/gpu/drm/i915/intel_display.c | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 97922fb..5fb11bc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -14739,6 +14739,7 @@ void intel_modeset_gem_init(struct drm_device *dev) struct drm_i915_private *dev_priv = dev-dev_private; struct drm_crtc *c; struct drm_i915_gem_object *obj; +int ret; mutex_lock(dev-struct_mutex); intel_init_gt_powersave(dev); @@ -14763,16 +14764,18 @@ void intel_modeset_gem_init(struct drm_device *dev) * pinned fenced. When we do the allocation it's too early * for this. */ -mutex_lock(dev-struct_mutex); for_each_crtc(dev, c) { obj = intel_fb_obj(c-primary-fb); if (obj == NULL) continue; -if (intel_pin_and_fence_fb_obj(c-primary, - c-primary-fb, - c-primary-state, - NULL)) { +mutex_lock(dev-struct_mutex); +ret = intel_pin_and_fence_fb_obj(c-primary, + c-primary-fb, + c-primary-state, + NULL); +mutex_unlock(dev-struct_mutex); +if (ret) { DRM_ERROR(failed to pin boot fb on pipe %d\n, to_intel_crtc(c)-pipe); drm_framebuffer_unreference(c-primary-fb); @@ -14780,7 +14783,6 @@ void intel_modeset_gem_init(struct drm_device *dev) update_state_fb(c-primary); } } -mutex_unlock(dev-struct_mutex); intel_backlight_register(dev); } -- 2.3.5 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3 1/8] drm/i915/skl: Add support to load SKL CSR firmware
On Tue, Apr 14, 2015 at 02:46:56PM +0530, Animesh Manna wrote: On 04/13/2015 10:52 PM, Damien Lespiau wrote: On Mon, Apr 13, 2015 at 08:15:29PM +0300, Imre Deak wrote: Ok, I haven't seen that. One question is if we need to support multiple interface versions or just the latest one. I would say only the latest one (for each platform) and so I915_CSR_SKL should be this latest firmware image filename, in this case i915/skl_dmc_ver4.bin. Yup, I think just supporting the latest one in the driver is what we want. The filename versioning is there so different kernel versions, supporting different interfaces, can all boot with the same userspace, each kernel loading the appropriate firmware. Can we have a symbolic link which can be hardcoded in intel_csr.c and option will be given to user during installing the firmware to create a symbolic link for the firmware wanted to load. Want to avoid version number mentioned in firmware file name as it getting change with latest fixes, bot not any API/inteface changes. Agree as bxt and skl both are gen9 we can name as skl_dmc_gen9.bin for now and discussion is going to finalize the name. Is it ok? Why would we need a symlink? what would be its name? skl_dmc_gen9.bin does not answer the requirement that we need to encode the API/interface version in the filename. The firmware on 01.org skl_dmc_ver4.bin seems to be what we want. HTH, -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t v5] tests/gem_mmap_gtt: add huge BO test
On 04/14/2015 12:14 PM, Joonas Lahtinen wrote: Add a straightforward test that allocates a BO that is bigger than (by 1 page currently) the mappable aperture, tests mmap access to it by CPU directly and through GTT in sequence. Currently it is expected for the GTT access to gracefully fail as all objects are attempted to get pinned to GTT completely for mmap access. Once the partial view support is merged to kernel, the test should pass for all parts. v2: - Corrected BO domain handling (Chris Wilson) - Check again after GTT access for added paranoia (Chris Wilson) v3: - Avoid flush by using pread (Chris Wilson) - Free gtt_pattern buffer too. v4: - Add more comments (Tvrtko Ursulin) - Use igt_require (Tvrtko Ursulin) v5: - Remove wrong message from igt_require_f (Tvrtko Ursulin) - After digging deeper to it, just igt_assert that the CPU mapping needs to succeed. Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com Cc: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Joonas Lahtinen joonas.lahti...@linux.intel.com --- tests/gem_mmap_gtt.c | 106 +++ 1 file changed, 106 insertions(+) diff --git a/tests/gem_mmap_gtt.c b/tests/gem_mmap_gtt.c index 115e398..487eecf 100644 --- a/tests/gem_mmap_gtt.c +++ b/tests/gem_mmap_gtt.c @@ -41,6 +41,10 @@ #include drmtest.h #include igt_debugfs.h +#ifndef PAGE_SIZE +#define PAGE_SIZE 4096 +#endif + static int OBJECT_SIZE = 16*1024*1024; static void @@ -55,6 +59,20 @@ set_domain_cpu(int fd, uint32_t handle) gem_set_domain(fd, handle, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU); } +static void +pread_bo(int fd, int handle, void *buf, int offset, int size) +{ + struct drm_i915_gem_pread gem_pread; + + memset(gem_pread, 0, sizeof(gem_pread)); + gem_pread.handle = handle; + gem_pread.data_ptr = (uintptr_t)buf; + gem_pread.size = size; + gem_pread.offset = offset; + + igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_PREAD, gem_pread) == 0); +} + static void * mmap_bo(int fd, uint32_t handle) { @@ -265,6 +283,92 @@ test_write_gtt(int fd) } static void +test_huge_bo(int fd) +{ + uint32_t bo; + char *ptr_cpu; + char *ptr_gtt; + char *cpu_pattern; + char *gtt_pattern; + uint64_t mappable_aperture_pages = gem_mappable_aperture_size() / + PAGE_SIZE; + uint64_t huge_object_size = (mappable_aperture_pages + 1) * PAGE_SIZE; + uint64_t last_offset = huge_object_size - PAGE_SIZE; + + cpu_pattern = malloc(PAGE_SIZE); + gtt_pattern = malloc(PAGE_SIZE); + igt_assert(cpu_pattern gtt_pattern); + memset(cpu_pattern, 0xaa, PAGE_SIZE); + memset(gtt_pattern, ~0xaa, PAGE_SIZE); + + bo = gem_create(fd, huge_object_size); + + /* Obtain CPU mapping for the object. */ + ptr_cpu = gem_mmap__cpu(fd, bo, 0, huge_object_size, + PROT_READ | PROT_WRITE); + igt_assert(ptr_cpu); + + set_domain_cpu(fd, bo); + + /* Write first page through the mapping and assert reading it back +* works. */ + memcpy(ptr_cpu, cpu_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_cpu, cpu_pattern, PAGE_SIZE) == 0); + + /* Write last page through the mapping and assert reading it back +* works. */ + memcpy(ptr_cpu + last_offset, cpu_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_cpu + last_offset, cpu_pattern, PAGE_SIZE) == 0); + + /* Cross check that accessing two simultaneous pages works. */ + igt_assert(memcmp(ptr_cpu, ptr_cpu + last_offset, PAGE_SIZE) == 0); + + munmap(ptr_cpu, huge_object_size); + ptr_cpu = NULL; + + /* Obtain mapping for the object through GTT. */ + ptr_gtt = gem_mmap__gtt(fd, bo, huge_object_size, + PROT_READ | PROT_WRITE); + igt_require_f(ptr_gtt, Huge BO GTT mapping not supported.\n); + + set_domain_gtt(fd, bo); + + /* Access through GTT should still provide the CPU written values. */ + igt_assert(memcmp(ptr_gtt , cpu_pattern, PAGE_SIZE) == 0); + igt_assert(memcmp(ptr_gtt + last_offset, cpu_pattern, PAGE_SIZE) == 0); + + /* Try replacing first page through GTT mapping and make sure other page +* stays intact. */ + memcpy(ptr_gtt, gtt_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_gtt , gtt_pattern, PAGE_SIZE) == 0); + igt_assert(memcmp(ptr_gtt + last_offset, cpu_pattern, PAGE_SIZE) == 0); + + /* And make sure that after writing, both pages contain what they +* should.*/ + memcpy(ptr_gtt + last_offset, gtt_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_gtt , gtt_pattern, PAGE_SIZE) == 0); + igt_assert(memcmp(ptr_gtt + last_offset, gtt_pattern, PAGE_SIZE) == 0); + + munmap(ptr_gtt, huge_object_size); + ptr_gtt = NULL; + + /* Verify the page contents
[Intel-gfx] [PATCH 2/2] drm/i915: DP link training optimization
This patch adds DP link training optimization by reusing the previously trained values. v2: - rebase Signed-off-by: Mika Kahola mika.kah...@intel.com --- drivers/gpu/drm/i915/intel_dp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index b6f1092..0d2284b 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3722,7 +3722,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) if (channel_eq) { DRM_DEBUG_KMS(Channel EQ done. DP Training successful\n); - intel_dp-train_set_valid = is_edp(intel_dp); + intel_dp-train_set_valid = true; } } -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/chv: Implement WaDisableShadowRegForCpd
On Tue, Apr 14, 2015 at 03:58:54PM +0530, deepa...@linux.intel.com wrote: From: Deepak S deepa...@linux.intel.com This WA disable usage of shadow register during CPD/RC6 transactions on CHV I suppose is a workaround for the shadow vs. wake FIFO problem... Yeah hsd seems to agree (after a bit of extra digging). Signed-off-by: Deepak S deepa...@linux.intel.com --- drivers/gpu/drm/i915/i915_reg.h | 1 + drivers/gpu/drm/i915/intel_pm.c | 4 2 files changed, 5 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 9c97842..bcdb16b 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -6206,6 +6206,7 @@ enum skl_disp_power_wells { #define GTFIFOCTL 0x120008 #defineGT_FIFO_FREE_ENTRIES_MASK 0x7f #defineGT_FIFO_NUM_RESERVED_ENTRIES 20 +#define GT_FIFO_CTL_BLOCK_POLICY (311) GT_FIFO_CTL_BLOCK_ALL_POLICY_STALL (1 12) GT_FIFO_CTL_RC6_POLICY_STALL(1 11) perhaps? #define HSW_IDICR 0x9008 #defineIDIHASHMSK(x) (((x) 0x3f) 16) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 4dd8b41..b9d3c00 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -6431,6 +6431,10 @@ static void cherryview_init_clock_gating(struct drm_device *dev) /* WaDisableSDEUnitClockGating:chv */ I915_WRITE(GEN8_UCGCTL6, I915_READ(GEN8_UCGCTL6) | GEN8_SDEUNIT_CLOCK_GATE_DISABLE); + + /* WaDisableShadowRegForCpd */ + I915_WRITE(GTFIFOCTL, I915_READ(GTFIFOCTL) | + GT_FIFO_CTL_BLOCK_POLICY); I think __intel_uncore_early_sanitize() might be a better place for this. } static void g4x_init_clock_gating(struct drm_device *dev) -- 1.9.1 -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/3] drm/i915/bxt: Add WaForceContextSaveRestoreNonCoherent
Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 6173 -Summary- Platform Delta drm-intel-nightly Series Applied PNV 276/276 276/276 ILK 302/302 302/302 SNB -34 313/313 279/313 IVB 337/337 337/337 BYT 286/286 286/286 HSW 395/395 395/395 BDW 321/321 321/321 -Detailed- Platform Testdrm-intel-nightly Series Applied *SNB igt@gem_mmap_gtt@read-no-prefault PASS(5) DMESG_WARN(1)PASS(1) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting SNB igt@kms_cursor_crc@cursor-size-change DMESG_WARN(5)PASS(3) DMESG_WARN(2) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting SNB igt@kms_flip@bo-too-big DMESG_WARN(5)PASS(3) DMESG_WARN(2) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting SNB igt@kms_flip@bo-too-big-interruptible DMESG_WARN(6)PASS(3) DMESG_WARN(2) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting SNB igt@kms_flip_event_leak DMESG_WARN(6)PASS(3) DMESG_WARN(2) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting SNB igt@kms_flip@flip-vs-dpms-off-vs-modeset DMESG_WARN(6)PASS(3) DMESG_WARN(2) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting SNB igt@kms_flip@flip-vs-dpms-off-vs-modeset-interruptible DMESG_WARN(6)PASS(3) DMESG_WARN(2) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting SNB igt@kms_flip@nonexisting-fb DMESG_WARN(6)PASS(3) DMESG_WARN(2) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting SNB igt@kms_flip@nonexisting-fb-interruptible DMESG_WARN(6)PASS(3) DMESG_WARN(2) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting SNB igt@kms_flip@single-buffer-flip-vs-dpms-off-vs-modeset DMESG_WARN(6)PASS(3) DMESG_WARN(2) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting SNB igt@kms_flip@single-buffer-flip-vs-dpms-off-vs-modeset-interruptible DMESG_WARN(6)PASS(3) DMESG_WARN(2) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting *SNB igt@kms_flip_tiling@flip-changes-tiling DMESG_WARN(6)PASS(3) FAIL(1)DMESG_WARN(1) SNB igt@kms_mmio_vs_cs_flip@setcrtc_vs_cs_flip DMESG_WARN(6)PASS(3) DMESG_WARN(2) (dmesg patch applied)drm:intel_dp_start_link_train[i915]]*ERROR*too_many_voltage_retries,give_up@too many voltage .* give up drm:intel_dp_complete_link_train[i915]]*ERROR*failed_to_train_DP,aborting@failed to train .* aborting SNB igt@kms_mmio_vs_cs_flip@setplane_vs_cs_flip DMESG_WARN(6)PASS(3) DMESG_WARN(2) (dmesg patch
[Intel-gfx] [PATCH i-g-t 2/2] quick_dump: Fix undefined symbols from libunwind
From: Ville Syrjälä ville.syrj...@linux.intel.com ../../lib/.libs/libintel_tools.a(igt_core.o): In function `print_backtrace': intel-gpu-tools/lib/igt_core.c:981: undefined reference to `_Ux86_64_getcontext' intel-gpu-tools/lib/igt_core.c:982: undefined reference to `_ULx86_64_init_local' intel-gpu-tools/lib/igt_core.c:983: undefined reference to `_ULx86_64_step' intel-gpu-tools/lib/igt_core.c:987: undefined reference to `_ULx86_64_get_proc_name' Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- tools/quick_dump/Makefile.am | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/quick_dump/Makefile.am b/tools/quick_dump/Makefile.am index fd023e7..7b06ec3 100644 --- a/tools/quick_dump/Makefile.am +++ b/tools/quick_dump/Makefile.am @@ -13,6 +13,7 @@ I915ChipsetPython_la_LIBADD = \ $(PCIACCESS_LIBS) \ $(DRM_LIBS) \ $(CAIRO_LIBS) \ + $(LIBUNWIND_LIBS) \ $(NULL) chipset.py: chipset_wrap_python.c -- 2.0.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/13] drm/i915: Support EDID compliance tests with the intel_dp_autotest_edid() function
2015-04-10 14:42 GMT-03:00 Todd Previte tprev...@gmail.com: Updates the EDID compliance test function to perform the EDID read as required by the tests. This read needs to take place in the kernel for reasons of speed and efficiency. The results of the EDID read operations are handed off to userspace so that the userspace app can set the display mode appropriately for the test response. The compliance_test_active flag now appears at the end of the individual test handling functions. This is so that the kernel-side operations can be completed without the risk of interruption from the userspace app that is polling on that flag. V2: - Addressed mailing list feedback - Removed excess debug messages - Removed extraneous comments - Fixed formatting issues (line length 80) - Updated the debug message in compute_edid_checksum to output hex values instead of decimal V3: - Addressed more list feedback - Added the test_active flag to the autotest function - Removed test_active flag from handler - Added failsafe check on the compliance test active flag at the end of the test handler - Fixed checkpatch.pl issues V4: - Removed the checksum computation function and its use as it has been rendered superfluous by changes to the core DRM EDID functions - Updated to use the raw header corruption detection mechanism - Moved the declaration of the test_data variable here V5: - Update test active flag variable name to match the change in the first patch of the series. - Relocated the test active flag declaration and initialization to this patch V6: - Updated to use the new flag for raw EDID header corruption - Removed the extra EDID read from the autotest function - Added the edid_checksum variable to struct intel_dp so that the autotest function can write it to the sink device - Moved the update to the hpd_pulse function to another patch - Removed extraneous constants V7: - Fixed erroneous placement of the checksum assignment. In some cases such as when the EDID read fails and is NULL, this causes a NULL ptr dereference in the kernel. Bad news. Fixed now. Signed-off-by: Todd Previte tprev...@gmail.com --- drivers/gpu/drm/i915/intel_dp.c | 46 drivers/gpu/drm/i915/intel_drv.h | 4 2 files changed, 50 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index ba2da44..3bfdc40 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -41,6 +41,12 @@ #define DP_LINK_CHECK_TIMEOUT (10 * 1000) +/* Compliance test status bits */ +#define INTEL_DP_RESOLUTION_SHIFT_MASK 4 +#define INTEL_DP_RESOLUTION_PREFERRED (1 INTEL_DP_RESOLUTION_SHIFT_MASK) +#define INTEL_DP_RESOLUTION_STANDARD (2 INTEL_DP_RESOLUTION_SHIFT_MASK) +#define INTEL_DP_RESOLUTION_FAILSAFE (3 INTEL_DP_RESOLUTION_SHIFT_MASK) + struct dp_link_dpll { int link_bw; struct dpll dpll; @@ -3770,6 +3776,35 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp) { uint8_t test_result = DP_TEST_NAK; + uint32_t ret = 0; + + if (intel_dp-compliance_edid_invalid || + intel_dp-aux.i2c_defer_count 6) { + /* Check for NACKs/DEFERs, use failsafe if detected +* (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) +*/ + if (intel_dp-aux.i2c_nack_count 0 || + intel_dp-aux.i2c_defer_count 0) + DRM_DEBUG_KMS(EDID read had %d NACKs, %d DEFERs\n, + intel_dp-aux.i2c_nack_count, + intel_dp-aux.i2c_defer_count); + intel_dp-compliance_test_data = INTEL_DP_RESOLUTION_FAILSAFE; Since this case is expected to happen, shouldn't we return something that's not DP_TEST_NAK here? Also, we should try to write the checksum on this case too, shouldn't we? I see on 4.2.2.6 that the only happy case involves setting the failsafe resolution, and optionally sending the checksum. (sorry for not catching this earlier... I just didn't spot the problem) + } else { + ret = drm_dp_dpcd_write(intel_dp-aux, + DP_TEST_EDID_CHECKSUM, + intel_dp-compliance_edid_checksum, 1); + if (ret = 0) + DRM_DEBUG_DRIVER(Failed to write EDID checksum\n); + else + DRM_DEBUG_DRIVER(EDID checksum written to sink\n); + + test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE; + intel_dp-compliance_test_data = INTEL_DP_RESOLUTION_STANDARD; + } + + /* Set test active flag here so userspace doesn't interrupt things */ + intel_dp-compliance_test_active = 1; +
Re: [Intel-gfx] [PATCH 2/5] drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic
-Original Message- From: Vivi, Rodrigo Sent: Friday, April 10, 2015 11:45 PM To: intel-gfx@lists.freedesktop.org Cc: Vivi, Rodrigo; R, Durgadoss; Runyan, Arthur J Subject: [PATCH 2/5] drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic Since the beginning there is a missunderstanding on the meaning of this dpcd bit. This bit shouldn't indicate whether to use link standby or not, but just be used to configure TP1, TP2 and TP3 times and tell hw aux should be skiped since HW is the responsible one. Even with help of frontbuffer tracking, HW is still fully responsible for PSR exit logic with/without DP training. DP_PSR_NO_TRAIN_ON_EXIT means the source doesn't need to do the training, but it doesn't tell to avoid TP patterns, so we will send minimal TP1 and avoid TP2. It also means that sink itself can take up to 5 idle frames for training. 6 in our case since we might be off by 1. So we also increment idle_frames by 4 here. v2: Fix and improve commit message (Durga). v3: Use minimal TP1 time avoiding TP2 and increase idle frame. Cc: Durgadoss R durgados...@intel.com Cc: Arthur Runyan arthur.j.run...@intel.com Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com --- drivers/gpu/drm/i915/intel_psr.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index db95b39..0e3b652 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -264,11 +264,17 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) uint32_t val = 0x0; const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; - if (dev_priv-psr.link_standby) { + if (dev_priv-psr.link_standby) val |= EDP_PSR_LINK_STANDBY; + + if (intel_dp-psr_dpcd[1] DP_PSR_NO_TRAIN_ON_EXIT) { + /* It doesn't mean we shouldn't send TPS patters, so let's + send the minimal TP1 possible and skip TP2. */ We can fix the multi-line comment style though, Reviewed-by: Durgadoss R durgados...@intel.com Thanks, Durga + val |= EDP_PSR_TP1_TIME_100us; val |= EDP_PSR_TP2_TP3_TIME_0us; - val |= EDP_PSR_TP1_TIME_0us; val |= EDP_PSR_SKIP_AUX_EXIT; + /* Sink should be able to train with the 5 or 6 idle patterns */ + idle_frames += 4; } I915_WRITE(EDP_PSR_CTL(dev), val | @@ -381,8 +387,7 @@ void intel_psr_enable(struct intel_dp *intel_dp) /* First we check VBT, but we must respect sink and source * known restrictions */ dev_priv-psr.link_standby = dev_priv-vbt.psr.full_link; - if ((intel_dp-psr_dpcd[1] DP_PSR_NO_TRAIN_ON_EXIT) || - (IS_BROADWELL(dev) intel_dig_port-port != PORT_A)) + if (IS_BROADWELL(dev) intel_dig_port-port != PORT_A) dev_priv-psr.link_standby = true; dev_priv-psr.busy_frontbuffer_bits = 0; -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic
-Original Message- From: Vivi, Rodrigo Sent: Friday, April 10, 2015 11:40 PM To: intel-gfx@lists.freedesktop.org Cc: Vivi, Rodrigo; R, Durgadoss; Runyan, Arthur J Subject: [PATCH] drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic Since the beginning there is a missunderstanding on the meaning of this dpcd bit. This bit shouldn't indicate whether to use link standby or not, but just be used to configure TP1, TP2 and TP3 times and tell hw aux should be skiped since HW is the responsible one. Even with help of frontbuffer tracking, HW is still fully responsible for PSR exit logic with/without DP training. DP_PSR_NO_TRAIN_ON_EXIT means the source doesn't need to do the training, but it doesn't tell to avoid TP patterns, so we will send minimal TP1 and avoid TP2. It also means that sink itself can take up to 5 idle frames for training. 6 in our case since we might be off by 1. So we also increment idle_frames by 4 here. v2: Fix and improve commit message (Durga). v3: Use minimal TP1 time avoiding TP2 and increase idle frame. Cc: Durgadoss R durgados...@intel.com Cc: Arthur Runyan arthur.j.run...@intel.com Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com Reviewed-by: Durgadoss R durgados...@intel.com Thanks, Durga --- drivers/gpu/drm/i915/intel_psr.c | 13 + 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index db95b39..0e3b652 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -264,11 +264,17 @@ static void hsw_psr_enable_source(struct intel_dp *intel_dp) uint32_t val = 0x0; const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES; - if (dev_priv-psr.link_standby) { + if (dev_priv-psr.link_standby) val |= EDP_PSR_LINK_STANDBY; + + if (intel_dp-psr_dpcd[1] DP_PSR_NO_TRAIN_ON_EXIT) { + /* It doesn't mean we shouldn't send TPS patters, so let's + send the minimal TP1 possible and skip TP2. */ + val |= EDP_PSR_TP1_TIME_100us; val |= EDP_PSR_TP2_TP3_TIME_0us; - val |= EDP_PSR_TP1_TIME_0us; val |= EDP_PSR_SKIP_AUX_EXIT; + /* Sink should be able to train with the 5 or 6 idle patterns */ + idle_frames += 4; } I915_WRITE(EDP_PSR_CTL(dev), val | @@ -381,8 +387,7 @@ void intel_psr_enable(struct intel_dp *intel_dp) /* First we check VBT, but we must respect sink and source * known restrictions */ dev_priv-psr.link_standby = dev_priv-vbt.psr.full_link; - if ((intel_dp-psr_dpcd[1] DP_PSR_NO_TRAIN_ON_EXIT) || - (IS_BROADWELL(dev) intel_dig_port-port != PORT_A)) + if (IS_BROADWELL(dev) intel_dig_port-port != PORT_A) dev_priv-psr.link_standby = true; dev_priv-psr.busy_frontbuffer_bits = 0; -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 1/2] quick_dump: Don't allow undefined symbols in _chipset.so
From: Ville Syrjälä ville.syrj...@linux.intel.com Every time _chipset.so has undefined symbols we fail to notice it at build time and then get to wonder why quick_dump fails to actually work. Pass -Wl,--no-undefined to the linker to get a build time error instead of the current runtime error. Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- tools/quick_dump/Makefile.am | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tools/quick_dump/Makefile.am b/tools/quick_dump/Makefile.am index 641066d..fd023e7 100644 --- a/tools/quick_dump/Makefile.am +++ b/tools/quick_dump/Makefile.am @@ -4,7 +4,8 @@ dist_bin_SCRIPTS = quick_dump.py reg_access.py bin_SCRIPTS = chipset.py lib_LTLIBRARIES = I915ChipsetPython.la -I915ChipsetPython_la_LDFLAGS = -module -avoid-version $(PYTHON_LDFLAGS) +I915ChipsetPython_la_LDFLAGS = -module -avoid-version $(PYTHON_LDFLAGS) \ + -Wl,--no-undefined I915ChipsetPython_la_SOURCES = chipset_macro_wrap.c nodist_I915ChipsetPython_la_SOURCES = chipset_wrap_python.c I915ChipsetPython_la_LIBADD = \ -- 2.0.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t v5] tests/gem_mmap_gtt: add huge BO test
On 14 April 2015 at 12:14, Joonas Lahtinen joonas.lahti...@linux.intel.com wrote: Add a straightforward test that allocates a BO that is bigger than (by 1 page currently) the mappable aperture, tests mmap access to it by CPU directly and through GTT in sequence. Currently it is expected for the GTT access to gracefully fail as all objects are attempted to get pinned to GTT completely for mmap access. Once the partial view support is merged to kernel, the test should pass for all parts. v2: - Corrected BO domain handling (Chris Wilson) - Check again after GTT access for added paranoia (Chris Wilson) v3: - Avoid flush by using pread (Chris Wilson) - Free gtt_pattern buffer too. v4: - Add more comments (Tvrtko Ursulin) - Use igt_require (Tvrtko Ursulin) v5: - Remove wrong message from igt_require_f (Tvrtko Ursulin) - After digging deeper to it, just igt_assert that the CPU mapping needs to succeed. Cc: Tvrtko Ursulin tvrtko.ursu...@linux.intel.com Cc: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Joonas Lahtinen joonas.lahti...@linux.intel.com --- tests/gem_mmap_gtt.c | 106 +++ 1 file changed, 106 insertions(+) diff --git a/tests/gem_mmap_gtt.c b/tests/gem_mmap_gtt.c index 115e398..487eecf 100644 --- a/tests/gem_mmap_gtt.c +++ b/tests/gem_mmap_gtt.c @@ -41,6 +41,10 @@ #include drmtest.h #include igt_debugfs.h +#ifndef PAGE_SIZE +#define PAGE_SIZE 4096 +#endif + static int OBJECT_SIZE = 16*1024*1024; static void @@ -55,6 +59,20 @@ set_domain_cpu(int fd, uint32_t handle) gem_set_domain(fd, handle, I915_GEM_DOMAIN_CPU, I915_GEM_DOMAIN_CPU); } +static void +pread_bo(int fd, int handle, void *buf, int offset, int size) +{ + struct drm_i915_gem_pread gem_pread; + + memset(gem_pread, 0, sizeof(gem_pread)); + gem_pread.handle = handle; + gem_pread.data_ptr = (uintptr_t)buf; + gem_pread.size = size; + gem_pread.offset = offset; + + igt_assert(ioctl(fd, DRM_IOCTL_I915_GEM_PREAD, gem_pread) == 0); +} + static void * mmap_bo(int fd, uint32_t handle) { @@ -265,6 +283,92 @@ test_write_gtt(int fd) } static void +test_huge_bo(int fd) +{ + uint32_t bo; + char *ptr_cpu; + char *ptr_gtt; + char *cpu_pattern; + char *gtt_pattern; + uint64_t mappable_aperture_pages = gem_mappable_aperture_size() / + PAGE_SIZE; + uint64_t huge_object_size = (mappable_aperture_pages + 1) * PAGE_SIZE; + uint64_t last_offset = huge_object_size - PAGE_SIZE; + + cpu_pattern = malloc(PAGE_SIZE); + gtt_pattern = malloc(PAGE_SIZE); + igt_assert(cpu_pattern gtt_pattern); + memset(cpu_pattern, 0xaa, PAGE_SIZE); + memset(gtt_pattern, ~0xaa, PAGE_SIZE); + + bo = gem_create(fd, huge_object_size); + + /* Obtain CPU mapping for the object. */ + ptr_cpu = gem_mmap__cpu(fd, bo, 0, huge_object_size, + PROT_READ | PROT_WRITE); + igt_assert(ptr_cpu); + + set_domain_cpu(fd, bo); + + /* Write first page through the mapping and assert reading it back +* works. */ + memcpy(ptr_cpu, cpu_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_cpu, cpu_pattern, PAGE_SIZE) == 0); + + /* Write last page through the mapping and assert reading it back +* works. */ + memcpy(ptr_cpu + last_offset, cpu_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_cpu + last_offset, cpu_pattern, PAGE_SIZE) == 0); + + /* Cross check that accessing two simultaneous pages works. */ + igt_assert(memcmp(ptr_cpu, ptr_cpu + last_offset, PAGE_SIZE) == 0); + + munmap(ptr_cpu, huge_object_size); + ptr_cpu = NULL; + + /* Obtain mapping for the object through GTT. */ + ptr_gtt = gem_mmap__gtt(fd, bo, huge_object_size, + PROT_READ | PROT_WRITE); + igt_require_f(ptr_gtt, Huge BO GTT mapping not supported.\n); + + set_domain_gtt(fd, bo); + + /* Access through GTT should still provide the CPU written values. */ + igt_assert(memcmp(ptr_gtt , cpu_pattern, PAGE_SIZE) == 0); + igt_assert(memcmp(ptr_gtt + last_offset, cpu_pattern, PAGE_SIZE) == 0); + + /* Try replacing first page through GTT mapping and make sure other page +* stays intact. */ + memcpy(ptr_gtt, gtt_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_gtt , gtt_pattern, PAGE_SIZE) == 0); + igt_assert(memcmp(ptr_gtt + last_offset, cpu_pattern, PAGE_SIZE) == 0); + + /* And make sure that after writing, both pages contain what they +* should.*/ + memcpy(ptr_gtt + last_offset, gtt_pattern, PAGE_SIZE); + igt_assert(memcmp(ptr_gtt , gtt_pattern, PAGE_SIZE) == 0); +
Re: [Intel-gfx] [PATCH v3] i-g-t: Adding test case to test background color.
On 30 March 2015 at 21:44, Chandra Konduru chandra.kond...@intel.com wrote: From: chandra konduru chandra.kond...@intel.com Adding i-g-t test case to test display crtc background color. v2: - Added IGT_TEST_DESCRIPTION() (Thomas Wood) - Added to .gitignore (Thomas Wood) - Added additional details to function header (Thomas Wood) - Simplified igt_main (Thomas Wood) v3: - rebased to latest master (me) - took sleep calls out (Daniel) - use new tiled types when calling igt_create_fb (me) I've pushed this patch and the panel fitting and plane scaling test cases, but I have switched them all to use igt_simple_main and remove the single subtest, which I think was your intention in v2 of this patch. Signed-off-by: chandra konduru chandra.kond...@intel.com --- lib/igt_kms.c | 61 lib/igt_kms.h | 4 + tests/.gitignore | 1 + tests/Android.mk | 1 + tests/Makefile.sources| 1 + tests/kms_crtc_background_color.c | 195 ++ 6 files changed, 263 insertions(+) create mode 100644 tests/kms_crtc_background_color.c diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 9c131f0..9c46e18 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -940,6 +940,22 @@ igt_plane_set_property(igt_plane_t *plane, uint32_t prop_id, uint64_t value) DRM_MODE_OBJECT_PLANE, prop_id, value); } +static bool +get_crtc_property(int drm_fd, uint32_t crtc_id, const char *name, + uint32_t *prop_id /* out */, uint64_t *value /* out */, + drmModePropertyPtr *prop /* out */) +{ + return kmstest_get_property(drm_fd, crtc_id, DRM_MODE_OBJECT_CRTC, + name, prop_id, value, prop); +} + +static void +igt_crtc_set_property(igt_output_t *output, uint32_t prop_id, uint64_t value) +{ + drmModeObjectSetProperty(output-display-drm_fd, + output-config.crtc-crtc_id, DRM_MODE_OBJECT_CRTC, prop_id, value); +} + /* * Walk a plane's property list to determine its type. If we don't * find a type property, then the kernel doesn't support universal @@ -1097,6 +1113,7 @@ void igt_display_init(igt_display_t *display, int drm_fd) igt_assert(display-outputs); for (i = 0; i display-n_outputs; i++) { + int j; igt_output_t *output = display-outputs[i]; /* @@ -1108,6 +1125,19 @@ void igt_display_init(igt_display_t *display, int drm_fd) output-display = display; igt_output_refresh(output); + + for (j = 0; j display-n_pipes; j++) { + uint64_t prop_value; + igt_pipe_t *pipe = display-pipes[j]; + if (output-config.crtc) { + get_crtc_property(display-drm_fd, output-config.crtc-crtc_id, + background_color, + pipe-background_property, + prop_value, + NULL); + pipe-background = (uint32_t)prop_value; + } + } } drmModeFreePlaneResources(plane_resources); @@ -1527,6 +1557,13 @@ static int igt_output_commit(igt_output_t *output, pipe = igt_output_get_driving_pipe(output); + if (pipe-background_changed) { + igt_crtc_set_property(output, pipe-background_property, + pipe-background); + + pipe-background_changed = false; + } + for (i = 0; i pipe-n_planes; i++) { igt_plane_t *plane = pipe-planes[i]; @@ -1779,6 +1816,30 @@ void igt_plane_set_rotation(igt_plane_t *plane, igt_rotation_t rotation) plane-rotation_changed = true; } +/** + * igt_crtc_set_background: + * @pipe: pipe pointer to which background color to be set + * @background: background color value in BGR 16bpc + * + * Sets background color for requested pipe. Color value provided here + * will be actually submitted at output commit time via background_color + * property. + * For example to get red as background, set background = 0x. + */ +void igt_crtc_set_background(igt_pipe_t *pipe, uint64_t background) +{ + igt_display_t *display = pipe-display; + + LOG(display, %s.%d: crtc_set_background(%lu)\n, + kmstest_pipe_name(pipe-pipe), + pipe-pipe, background); + + pipe-background = background; + + pipe-background_changed = true; +} + + void igt_wait_for_vblank(int drm_fd, enum pipe pipe) { drmVBlank wait_vbl; diff --git a/lib/igt_kms.h b/lib/igt_kms.h index 565df14..84e8456 100644 ---
Re: [Intel-gfx] [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt
On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote: On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote: We load the ppgtt ptes once per gpu reset/driver load/resume and that's all that's needed. Note that this only blows up when we're using the allocate_va_range funcs and not the special-purpose ones used. With this change we can get rid of that duplication. Honestly, I would prefer the test to be rewritten so that the information on which vm was being used was passed along and not that magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly what vm it bound the objects into, and yet towards the end we decide to guess again. Also, I would rather the execbuffer test be moved to i915_gem_context since it is scattering internal knowledge about. Yeah I agree that there's more room for cleanup. I've done this minimal patch purely to shut up the bogus WARN_ONs when I tried to unify the gen6/7 pt alloc code in the next patch. Since it's bogus. But I agree that the current pd reloading is hard to understand, and might even be the reason why gen7 full ppgtt doesn't quite work. Who knows, but otoh the code isnt' too harmful either (until we decide to can gen7 full ppgtt for real and remove it all). -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915/skl: Support for 90/270 rotation
On Tue, Apr 14, 2015 at 05:49:40PM +0530, Jindal, Sonika wrote: On 4/14/2015 5:09 AM, Matt Roper wrote: On Mon, Apr 13, 2015 at 01:49:22PM +0300, Ville Syrjälä wrote: On Mon, Apr 13, 2015 at 03:53:22PM +0530, Jindal, Sonika wrote: On 4/13/2015 3:40 PM, Ville Syrjälä wrote: On Mon, Apr 13, 2015 at 09:36:02AM +0530, Jindal, Sonika wrote: On 4/10/2015 8:14 PM, Ville Syrjälä wrote: On Fri, Apr 10, 2015 at 04:17:17PM +0200, Daniel Vetter wrote: On Fri, Apr 10, 2015 at 02:37:29PM +0530, Sonika Jindal wrote: v2: Moving creation of property in a function, checking for 90/270 rotation simultaneously (Chris) Letting primary plane to be positioned v3: Adding if/else for 90/270 and rest params programming, adding check for pixel_format, some cleanup (review comments) v4: Adding right pixel_formats, using src_* params instead of crtc_* for offset and size programming (Ville) v5: Rebased on -nightly and Tvrtko's series for gtt remapping. v6: Rebased on -nightly (Tvrtko's series merged) v7: Moving pixel_format check to intel_atomic_plane_check (Matt) Signed-off-by: Sonika Jindal sonika.jin...@intel.com Reviewed-by: Matt Roper matthew.d.ro...@intel.com Patches are missing the Testcase: tag, please add that. Also, are all the igt committed or not yet? Pulled these two in meanwhile. Things are going somewhat broken because you didn't apply my plane state stuff. Hmm. Actually it sort of looks that it might work by luck as long as the primary plane doesn't get clipped since this is bashing the user state directly into the hardware registers and not the derived state (ie. clipped coordinates). I was hoping your changes get merged, but not sure why they are held up. Also I see my review comment about the 90 vs. 270 rotation mixup was ignored at least. I never really got the to understand the need of reversing 90 and 270 :) The last discussion was not concluded, AFAIR. Things look correct to me and work fine with the testcase also. Is there something completely different which I am unable to get? BSpec gives me the impression the hw rotation is cw, whereas the drm one is ccw. Yes, HW rotation is cw as per bspec. In drm, where do we consider it as anti-cw? I assume it is cw because all the macros work as expected, ie cw. The ccw rule was inherited from XRandR. I'm not sure what macros you mean, but drm_rect_rotate() will certainly give you wrong results if you assume cw. It seems like this information needs to be added to the property section of the DRM DocBook; continuing to follow XRandR probably makes sense if that's what's agreed on, but there's no indication of that design philosophy in the actual DRM documentation today, so we're in danger of having different driver authors use conflicting interpretations. Ok, I will create a DocBook patch with description for 90/270 rotation (Anyways 90/270 rotation is not added in the rotation property in documentation). Will there be any other place for this or i915's rotation property? Also, I will send a patch to flip 90/270 in i915. I am just worried, that it will look confusing to check for DRM_ROTATE_90 and use PLANE_CTL_ROTATE_270 for programming. I will add a comment though. Just add a comment stating that drm is ccw (to stay compatible with Xrandr) and i915 hw is cw. That should explain it all. There's other places where the linux kernel and intel hw people disagree on definitions, it just happens. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] intel: Use WAIT for wait-rendering
Using WAIT is preferrable to SET_DOMAIN as it doesn't have any domain management side-effects - but has the same flushing semantics. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Daniel Vetter daniel.vet...@ffwll.ch --- intel/intel_bufmgr_gem.c | 18 ++ 1 file changed, 18 insertions(+) diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index ecbf8ee..a938441 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -1740,6 +1740,24 @@ drm_intel_gem_bo_get_subdata(drm_intel_bo *bo, unsigned long offset, static void drm_intel_gem_bo_wait_rendering(drm_intel_bo *bo) { + drm_intel_bufmgr_gem *bufmgr_gem = (drm_intel_bufmgr_gem *) bo-bufmgr; + + /* Using WAIT is preferrable to SET_DOMAIN as it doesn't have +* any domain management side-effects - but has the same flushing +* semantics. +*/ + if (bufmgr_gem-has_wait_timeout) { + struct drm_i915_gem_wait wait; + + memclear(wait); + wait.bo_handle = bo-handle; + wait.timeout_ns = -1; + if (drmIoctl(bufmgr_gem-fd, +DRM_IOCTL_I915_GEM_WAIT, +wait) == 0) + return; + } + drm_intel_gem_bo_start_gtt_access(bo, 1); } -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/17] drm/i915: Unify aliasing ppgtt handling
With the dynamic pagetable alloc code aliasing ppgtt special-cases where again mixed in all over the place with the low-level init code. Extract the va preallocation and clearing again into the common code where aliasing ppgtt gets set up. Note that with this we don't set the size of the aliasing ppgtt to the size of the parent ggtt address space. Which isn't required at all since except for the ppgtt setup/cleanup code no one ever looks at this. Signed-off-by: Daniel Vetter daniel.vet...@intel.com --- drivers/gpu/drm/i915/i915_gem_gtt.c | 134 +++- 1 file changed, 24 insertions(+), 110 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index abb11f139d25..75787f1d2751 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -387,50 +387,6 @@ fail_bitmap: return ERR_PTR(ret); } -/** - * alloc_pt_range() - Allocate a multiple page tables - * @pd:The page directory which will have at least @count entries - * available to point to the allocated page tables. - * @pde: First page directory entry for which we are allocating. - * @count: Number of pages to allocate. - * @dev: DRM device. - * - * Allocates multiple page table pages and sets the appropriate entries in the - * page table structure within the page directory. Function cleans up after - * itself on any failures. - * - * Return: 0 if allocation succeeded. - */ -static int alloc_pt_range(struct i915_page_directory *pd, uint16_t pde, size_t count, - struct drm_device *dev) -{ - int i, ret; - - /* 512 is the max page tables per page_directory on any platform. */ - if (WARN_ON(pde + count I915_PDES)) - return -EINVAL; - - for (i = pde; i pde + count; i++) { - struct i915_page_table *pt = alloc_pt_single(dev); - - if (IS_ERR(pt)) { - ret = PTR_ERR(pt); - goto err_out; - } - WARN(pd-page_table[i], -Leaking page directory entry %d (%p)\n, -i, pd-page_table[i]); - pd-page_table[i] = pt; - } - - return 0; - -err_out: - while (i-- pde) - unmap_and_free_pt(pd-page_table[i], dev); - return ret; -} - static void unmap_and_free_pd(struct i915_page_directory *pd, struct drm_device *dev) { @@ -971,7 +927,7 @@ err_out: * space. * */ -static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size) +static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) { ppgtt-scratch_pt = alloc_pt_single(ppgtt-base.dev); if (IS_ERR(ppgtt-scratch_pt)) @@ -985,8 +941,9 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size) gen8_initialize_pd(ppgtt-base, ppgtt-scratch_pd); ppgtt-base.start = 0; - ppgtt-base.total = size; + ppgtt-base.total = 1ULL 32; ppgtt-base.cleanup = gen8_ppgtt_cleanup; + ppgtt-base.allocate_va_range = gen8_alloc_va_range; ppgtt-base.insert_entries = gen8_ppgtt_insert_entries; ppgtt-base.clear_range = gen8_ppgtt_clear_range; ppgtt-base.unbind_vma = ppgtt_unbind_vma; @@ -997,46 +954,6 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size) return 0; } -static int gen8_aliasing_ppgtt_init(struct i915_hw_ppgtt *ppgtt) -{ - struct drm_device *dev = ppgtt-base.dev; - struct drm_i915_private *dev_priv = dev-dev_private; - uint64_t start = 0, size = dev_priv-gtt.base.total; - int ret; - - ret = gen8_ppgtt_init_common(ppgtt, dev_priv-gtt.base.total); - if (ret) - return ret; - - /* Aliasing PPGTT has to always work and be mapped because of the way we -* use RESTORE_INHIBIT in the context switch. This will be fixed -* eventually. */ - ret = gen8_alloc_va_range(ppgtt-base, start, size); - if (ret) { - unmap_and_free_pd(ppgtt-scratch_pd, ppgtt-base.dev); - unmap_and_free_pt(ppgtt-scratch_pt, ppgtt-base.dev); - return ret; - } - - ppgtt-base.allocate_va_range = NULL; - ppgtt-base.clear_range(ppgtt-base, 0, ppgtt-base.total, true); - - return 0; -} - -static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) -{ - int ret; - - ret = gen8_ppgtt_init_common(ppgtt, (1ULL 32)); - if (ret) - return ret; - - ppgtt-base.allocate_va_range = gen8_alloc_va_range; - - return 0; -} - static void gen6_dump_ppgtt(struct i915_hw_ppgtt *ppgtt, struct seq_file *m) { struct i915_address_space *vm = ppgtt-base; @@ -1533,7 +1450,7 @@ static void gen6_scratch_va_range(struct i915_hw_ppgtt *ppgtt, ppgtt-pd.page_table[pde] = ppgtt-scratch_pt; } -static int
[Intel-gfx] [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space
They change with the address space and not with each vma, so move them into the right pile of vfuncs. Save 2 pointers per vma and clarifies the code. Signed-off-by: Daniel Vetter daniel.vet...@intel.com --- drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/i915_gem_gtt.c | 28 drivers/gpu/drm/i915/i915_gem_gtt.h | 15 +++ 3 files changed, 20 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 8ce363aa492c..4578772c5509 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3035,7 +3035,7 @@ int i915_vma_unbind(struct i915_vma *vma) trace_i915_vma_unbind(vma); - vma-unbind_vma(vma); + vma-vm-unbind_vma(vma); list_del_init(vma-mm_list); if (i915_is_ggtt(vma-vm)) { diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 1c8ef7c143aa..290db48faf27 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -995,6 +995,8 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size) ppgtt-base.cleanup = gen8_ppgtt_cleanup; ppgtt-base.insert_entries = gen8_ppgtt_insert_entries; ppgtt-base.clear_range = gen8_ppgtt_clear_range; + ppgtt-base.unbind_vma = ppgtt_unbind_vma; + ppgtt-base.bind_vma = ppgtt_bind_vma; ppgtt-switch_mm = gen8_mm_switch; @@ -1579,6 +1581,8 @@ static int gen6_ppgtt_init(struct i915_hw_ppgtt *ppgtt, bool aliasing) ppgtt-base.allocate_va_range = aliasing ? NULL : gen6_alloc_va_range; ppgtt-base.clear_range = gen6_ppgtt_clear_range; ppgtt-base.insert_entries = gen6_ppgtt_insert_entries; + ppgtt-base.unbind_vma = ppgtt_unbind_vma; + ppgtt-base.bind_vma = ppgtt_bind_vma; ppgtt-base.cleanup = gen6_ppgtt_cleanup; ppgtt-base.start = 0; ppgtt-base.total = I915_PDES * GEN6_PTES * PAGE_SIZE; @@ -2573,6 +2577,8 @@ static int gen8_gmch_probe(struct drm_device *dev, dev_priv-gtt.base.clear_range = gen8_ggtt_clear_range; dev_priv-gtt.base.insert_entries = gen8_ggtt_insert_entries; + dev_priv-gtt.base.bind_vma = ggtt_bind_vma; + dev_priv-gtt.base.unbind_vma = ggtt_unbind_vma; return ret; } @@ -2613,6 +2619,8 @@ static int gen6_gmch_probe(struct drm_device *dev, dev_priv-gtt.base.clear_range = gen6_ggtt_clear_range; dev_priv-gtt.base.insert_entries = gen6_ggtt_insert_entries; + dev_priv-gtt.base.bind_vma = ggtt_bind_vma; + dev_priv-gtt.base.unbind_vma = ggtt_unbind_vma; return ret; } @@ -2645,6 +2653,8 @@ static int i915_gmch_probe(struct drm_device *dev, dev_priv-gtt.do_idle_maps = needs_idle_maps(dev_priv-dev); dev_priv-gtt.base.clear_range = i915_ggtt_clear_range; + dev_priv-gtt.base.bind_vma = i915_ggtt_bind_vma; + dev_priv-gtt.base.unbind_vma = i915_ggtt_unbind_vma; if (unlikely(dev_priv-gtt.do_idle_maps)) DRM_INFO(applying Ironlake quirks for intel_iommu\n); @@ -2732,22 +2742,8 @@ __i915_gem_vma_create(struct drm_i915_gem_object *obj, vma-vm = vm; vma-obj = obj; - if (INTEL_INFO(vm-dev)-gen = 6) { - if (i915_is_ggtt(vm)) { - vma-ggtt_view = *ggtt_view; - - vma-unbind_vma = ggtt_unbind_vma; - vma-bind_vma = ggtt_bind_vma; - } else { - vma-unbind_vma = ppgtt_unbind_vma; - vma-bind_vma = ppgtt_bind_vma; - } - } else { - BUG_ON(!i915_is_ggtt(vm)); + if (i915_is_ggtt(vm)) vma-ggtt_view = *ggtt_view; - vma-unbind_vma = i915_ggtt_unbind_vma; - vma-bind_vma = i915_ggtt_bind_vma; - } list_add_tail(vma-vma_link, obj-vma_list); if (!i915_is_ggtt(vm)) @@ -2957,7 +2953,7 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, return ret; } - vma-bind_vma(vma, cache_level, flags); + vma-vm-bind_vma(vma, cache_level, flags); return 0; } diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 29de64d1164e..12d0ded0d823 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -196,14 +196,6 @@ struct i915_vma { * bits with absolutely no headroom. So use 4 bits. */ unsigned int pin_count:4; #define DRM_I915_GEM_OBJECT_MAX_PIN_COUNT 0xf - - /** Unmap an object from an address space. This usually consists of -* setting the valid PTE entries to a reserved scratch page. */ - void (*unbind_vma)(struct i915_vma *vma); - /* Map an object into an address space with the given cache flags. */ - void (*bind_vma)(struct i915_vma *vma, -
[Intel-gfx] [PATCH 00/17] i915_gem_gtt.c polish
Hi all, I ended up reading a bit of i915_gem_gtt.c and spotted a few things to clean up after the dynamic pagetable stuff landed. I haven't done the checkpatch polish and kerneldoc, Mika/Michel will be doing that, but overall I think the code looks fairly tidy now. I also untangled the vma binding logic a bit since it's related, which means we can finally enable the gen7 cmd parser. Btw my idea is that we'll move the higher level vma related code in i915_gem_gtt.c out into a new i915_gem_vma.c file, together with the other vma code sprinkled in various places. But that's probably better to do after the partial mmap support from Joonas has landed. With that reorg i915_gem_gtt.c would only concern itself with the low-level pagetable handling. Survived light testing on my snb here. Commentsreview highly welcome. Cheers, Daniel Daniel Vetter (17): drm/i915: Move gen8 clear_range vfunc setup into common code drm/i915: Move vma vfuns to adddress_space drm/i915: Clean up aliasing ppgtt correctly on error paths drm/i915: Unify aliasing ppgtt handling drm/i915: Move PTE_READ_ONLY to -pte_encode vfunc drm/i915: Dont clear PIN_GLOBAL in the execbuf pinning fallback drm/i915: Drop redundant GGTT rebinding drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt drm/i915: Don't use atomics for pg_dirty_rings drm/i915: Remove misleading comment around bind_to_vm drm/i915: Fix up the vma aliasing ppgtt binding drm/i915: Arm cmd parser with aliasng ppgtt only drm/i915: move i915_gem_restore_gtt_mappings around drm/i915: Move ppgtt_bind/unbind around drm/i915: Unduplicate i915_ggtt_unbind/bind_vma drm/i915: Don't try to outsmart gcc in i915_gem_gtt.c drm/i915: Move i915_get_ggtt_vma_pages into ggtt_bind_vma drivers/gpu/drm/i915/i915_drv.h| 11 +- drivers/gpu/drm/i915/i915_gem.c| 17 +- drivers/gpu/drm/i915/i915_gem_context.c| 33 +- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 23 +- drivers/gpu/drm/i915/i915_gem_gtt.c| 502 +++-- drivers/gpu/drm/i915/i915_gem_gtt.h| 18 +- 6 files changed, 225 insertions(+), 379 deletions(-) -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/17] drm/i915: Move PTE_READ_ONLY to -pte_encode vfunc
It's only used as a flag there, so unconfuse things a bit. Also separate the bind_vma flag space from the pte_encode flag space in the code. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_gtt.c | 15 +-- drivers/gpu/drm/i915/i915_gem_gtt.h | 3 ++- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 75787f1d2751..4e2caef83772 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1610,14 +1610,16 @@ void i915_ppgtt_release(struct kref *kref) static void ppgtt_bind_vma(struct i915_vma *vma, enum i915_cache_level cache_level, - u32 flags) + u32 unused) { + u32 pte_flags = 0; + /* Currently applicable only to VLV */ if (vma-obj-gt_ro) - flags |= PTE_READ_ONLY; + pte_flags |= PTE_READ_ONLY; vma-vm-insert_entries(vma-vm, vma-obj-pages, vma-node.start, - cache_level, flags); + cache_level, pte_flags); } static void ppgtt_unbind_vma(struct i915_vma *vma) @@ -1986,10 +1988,11 @@ static void ggtt_bind_vma(struct i915_vma *vma, struct drm_i915_private *dev_priv = dev-dev_private; struct drm_i915_gem_object *obj = vma-obj; struct sg_table *pages = obj-pages; + u32 pte_flags = 0; /* Currently applicable only to VLV */ if (obj-gt_ro) - flags |= PTE_READ_ONLY; + pte_flags |= PTE_READ_ONLY; if (i915_is_ggtt(vma-vm)) pages = vma-ggtt_view.pages; @@ -2010,7 +2013,7 @@ static void ggtt_bind_vma(struct i915_vma *vma, (cache_level != obj-cache_level)) { vma-vm-insert_entries(vma-vm, pages, vma-node.start, - cache_level, flags); + cache_level, pte_flags); vma-bound |= GLOBAL_BIND; } } @@ -2021,7 +2024,7 @@ static void ggtt_bind_vma(struct i915_vma *vma, struct i915_hw_ppgtt *appgtt = dev_priv-mm.aliasing_ppgtt; appgtt-base.insert_entries(appgtt-base, pages, vma-node.start, - cache_level, flags); + cache_level, pte_flags); vma-bound |= LOCAL_BIND; } } diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index 12d0ded0d823..fb0a04aa5363 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -158,7 +158,6 @@ struct i915_vma { /** Flags and address space this VMA is bound to */ #define GLOBAL_BIND(10) #define LOCAL_BIND (11) -#define PTE_READ_ONLY (12) unsigned int bound : 4; /** @@ -261,6 +260,8 @@ struct i915_address_space { gen6_pte_t (*pte_encode)(dma_addr_t addr, enum i915_cache_level level, bool valid, u32 flags); /* Create a valid PTE */ + /* flags for pte_encode */ +#define PTE_READ_ONLY (10) int (*allocate_va_range)(struct i915_address_space *vm, uint64_t start, uint64_t length); -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/17] drm/i915: Clean up aliasing ppgtt correctly on error paths
While at it inline the free functions - they don't actually free the ppgtt, just clean up the allocations done for it. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_gtt.c | 30 ++ 1 file changed, 10 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 290db48faf27..abb11f139d25 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -672,8 +672,10 @@ static void gen8_free_page_tables(struct i915_page_directory *pd, struct drm_dev } } -static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt) +static void gen8_ppgtt_cleanup(struct i915_address_space *vm) { + struct i915_hw_ppgtt *ppgtt = + container_of(vm, struct i915_hw_ppgtt, base); int i; for_each_set_bit(i, ppgtt-pdp.used_pdpes, GEN8_LEGACY_PDPES) { @@ -688,14 +690,6 @@ static void gen8_ppgtt_free(struct i915_hw_ppgtt *ppgtt) unmap_and_free_pt(ppgtt-scratch_pt, ppgtt-base.dev); } -static void gen8_ppgtt_cleanup(struct i915_address_space *vm) -{ - struct i915_hw_ppgtt *ppgtt = - container_of(vm, struct i915_hw_ppgtt, base); - - gen8_ppgtt_free(ppgtt); -} - /** * gen8_ppgtt_alloc_pagetabs() - Allocate page tables for VA range. * @ppgtt: Master ppgtt structure. @@ -1454,11 +1448,16 @@ unwind_out: return ret; } -static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt) +static void gen6_ppgtt_cleanup(struct i915_address_space *vm) { + struct i915_hw_ppgtt *ppgtt = + container_of(vm, struct i915_hw_ppgtt, base); struct i915_page_table *pt; uint32_t pde; + + drm_mm_remove_node(ppgtt-node); + gen6_for_all_pdes(pt, ppgtt, pde) { if (pt != ppgtt-scratch_pt) unmap_and_free_pt(pt, ppgtt-base.dev); @@ -1468,16 +1467,6 @@ static void gen6_ppgtt_free(struct i915_hw_ppgtt *ppgtt) unmap_and_free_pd(ppgtt-pd, ppgtt-base.dev); } -static void gen6_ppgtt_cleanup(struct i915_address_space *vm) -{ - struct i915_hw_ppgtt *ppgtt = - container_of(vm, struct i915_hw_ppgtt, base); - - drm_mm_remove_node(ppgtt-node); - - gen6_ppgtt_free(ppgtt); -} - static int gen6_ppgtt_allocate_page_directories(struct i915_hw_ppgtt *ppgtt) { struct drm_device *dev = ppgtt-base.dev; @@ -2268,6 +2257,7 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev, ret = __hw_ppgtt_init(dev, ppgtt, true); if (ret) { + ppgtt-base.cleanup(ppgtt-base); kfree(ppgtt); return ret; } -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 07/17] drm/i915: Drop redundant GGTT rebinding
Since commit bf3d149b25f67f241735b91a56b7f070bc0a5407 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Fri Feb 14 14:01:12 2014 +0100 drm/i915: split PIN_GLOBAL out from PIN_MAPPABLE i915_gem_obj_ggtt_pin always binds into the ggtt, but I've forgotten to remove the now redundant additional bind call later on. Fix this up. Signed-off-by: Daniel Vetter daniel.vet...@intel.com --- drivers/gpu/drm/i915/i915_gem_context.c | 11 --- 1 file changed, 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index e4c57a3981b3..dd5bff657c9c 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -638,7 +638,6 @@ static int do_switch(struct intel_engine_cs *ring, struct intel_context *from = ring-last_context; u32 hw_flags = 0; bool uninitialized = false; - struct i915_vma *vma; int ret, i; if (from != NULL ring == dev_priv-ring[RCS]) { @@ -696,16 +695,6 @@ static int do_switch(struct intel_engine_cs *ring, if (ret) goto unpin_out; - vma = i915_gem_obj_to_ggtt(to-legacy_hw_ctx.rcs_state); - if (!(vma-bound GLOBAL_BIND)) { - ret = i915_vma_bind(vma, - to-legacy_hw_ctx.rcs_state-cache_level, - GLOBAL_BIND); - /* This shouldn't ever fail. */ - if (WARN_ONCE(ret, GGTT context bind failed!)) - goto unpin_out; - } - if (!to-legacy_hw_ctx.initialized) { hw_flags |= MI_RESTORE_INHIBIT; /* NB: If we inhibit the restore, the context is not allowed to -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/17] drm/i915: Dont clear PIN_GLOBAL in the execbuf pinning fallback
PIN_GLOBAL is set only when userspace asked for it, and that is only the case for the gen6 PIPE_CONTROL workaround. We're not allowed to just clear this. The important part of the fallback is to drop the restriction to the mappable range. This issue has been introduced in commit edf4427b8055dc93eb5222d8174b07a75ba24fb5 Author: Chris Wilson ch...@chris-wilson.co.uk Date: Wed Jan 14 11:20:56 2015 + drm/i915: Fallback to using CPU relocations for large batch buffers Cc: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 48ac5608481e..a7ed9f695586 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -600,7 +600,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, only_mappable_for_reloc(entry-flags)) ret = i915_gem_object_pin(obj, vma-vm, entry-alignment, - flags ~(PIN_GLOBAL | PIN_MAPPABLE)); + flags ~PIN_MAPPABLE); if (ret) return ret; -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt
We load the ppgtt ptes once per gpu reset/driver load/resume and that's all that's needed. Note that this only blows up when we're using the allocate_va_range funcs and not the special-purpose ones used. With this change we can get rid of that duplication. Signed-off-by: Daniel Vetter daniel.vet...@intel.com --- drivers/gpu/drm/i915/i915_gem_context.c| 6 -- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 3 --- 2 files changed, 9 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index dd5bff657c9c..6b0962db2cf7 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -575,8 +575,6 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring, struct intel_context *from, struct intel_context *to) { - struct drm_i915_private *dev_priv = ring-dev-dev_private; - if (to-remap_slice) return false; @@ -584,10 +582,6 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring, if (from == to !test_bit(ring-id, to-ppgtt-pd_dirty_rings)) return true; - } else if (dev_priv-mm.aliasing_ppgtt) { - if (from == to !test_bit(ring-id, - dev_priv-mm.aliasing_ppgtt-pd_dirty_rings)) - return true; } return false; diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index a7ed9f695586..1d5badf1b887 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1250,9 +1250,6 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file, if (ctx-ppgtt) WARN(ctx-ppgtt-pd_dirty_rings (1ring-id), %s didn't clear reload\n, ring-name); - else if (dev_priv-mm.aliasing_ppgtt) - WARN(dev_priv-mm.aliasing_ppgtt-pd_dirty_rings - (1ring-id), %s didn't clear reload\n, ring-name); instp_mode = args-flags I915_EXEC_CONSTANTS_MASK; instp_mask = I915_EXEC_CONSTANTS_MASK; -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/17] drm/i915: Remove misleading comment around bind_to_vm
It's true that we might need to context switch, but both the signalling and implementation of the same are a few source files away. Remove it. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_gem.c | 4 1 file changed, 4 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 4578772c5509..10e873c8957f 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -4183,10 +4183,6 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj, bound = vma ? vma-bound : 0; if (vma == NULL || !drm_mm_node_allocated(vma-node)) { - /* In true PPGTT, bind has possibly changed PDEs, which -* means we must do a context switch before the GPU can -* accurately read some of the VMAs. -*/ vma = i915_gem_object_bind_to_vm(obj, vm, ggtt_view, alignment, flags); if (IS_ERR(vma)) -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/17] drm/i915: Fix up the vma aliasing ppgtt binding
Currently we have the problem that the decision whether ptes need to be (re)written is splattered all over the codebase. Move all that into i915_vma_bind. This needs a few changes: - Just reuse the PIN_* flags for i915_vma_bind and do the conversion to vma-bound in there to avoid duplicating the conversion code all over. - We need to make binding for EXECBUF (i.e. pick aliasing ppgtt if around) explicit, add PIN_EXECBUF for that. - Two callers want to update ptes, give them a PIN_UPDATE for that. Of course we still want to avoid double-binding, but that should be taken care of: - A ppgtt vma will only ever see PIN_EXECBUF, so no issue with double-binding. - A ggtt vma with aliasing ppgtt needs both types of binding, and we track that properly now. - A ggtt vma without aliasing ppgtt could be bound twice. In the lower-level -bind_vma functions hence unconditionally set GLOBAL_BIND when writing the ggtt ptes. There's still a bit room for cleanup, but that's for follow-up patches. v2: Fixup fumbles. Signed-off-by: Daniel Vetter daniel.vet...@intel.com --- drivers/gpu/drm/i915/i915_drv.h| 11 +++-- drivers/gpu/drm/i915/i915_gem.c| 11 ++--- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 ++-- drivers/gpu/drm/i915/i915_gem_gtt.c| 65 -- 4 files changed, 40 insertions(+), 54 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 47be4a57e6a9..eaeae041fed9 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -2640,10 +2640,13 @@ void i915_init_vm(struct drm_i915_private *dev_priv, void i915_gem_free_object(struct drm_gem_object *obj); void i915_gem_vma_destroy(struct i915_vma *vma); -#define PIN_MAPPABLE 0x1 -#define PIN_NONBLOCK 0x2 -#define PIN_GLOBAL 0x4 -#define PIN_OFFSET_BIAS 0x8 +/* Flags used by pin/bindfriends. */ +#define PIN_MAPPABLE (10) +#define PIN_NONBLOCK (11) +#define PIN_GLOBAL (12) +#define PIN_OFFSET_BIAS(13) +#define PIN_EXECBUF(14) +#define PIN_UPDATE (15) #define PIN_OFFSET_MASK (~4095) int __must_check i915_gem_object_pin(struct drm_i915_gem_object *obj, diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 10e873c8957f..047629b08697 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -3557,8 +3557,7 @@ search_free: goto err_remove_node; trace_i915_vma_bind(vma, flags); - ret = i915_vma_bind(vma, obj-cache_level, - flags PIN_GLOBAL ? GLOBAL_BIND : 0); + ret = i915_vma_bind(vma, obj-cache_level, flags); if (ret) goto err_finish_gtt; @@ -3784,7 +3783,7 @@ int i915_gem_object_set_cache_level(struct drm_i915_gem_object *obj, list_for_each_entry(vma, obj-vma_list, vma_link) if (drm_mm_node_allocated(vma-node)) { ret = i915_vma_bind(vma, cache_level, - vma-bound GLOBAL_BIND); + PIN_UPDATE); if (ret) return ret; } @@ -4187,10 +4186,8 @@ i915_gem_object_do_pin(struct drm_i915_gem_object *obj, flags); if (IS_ERR(vma)) return PTR_ERR(vma); - } - - if (flags PIN_GLOBAL !(vma-bound GLOBAL_BIND)) { - ret = i915_vma_bind(vma, obj-cache_level, GLOBAL_BIND); + } else { + ret = i915_vma_bind(vma, obj-cache_level, flags); if (ret) return ret; } diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 0b06c6de27de..f005f3151147 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -400,10 +400,9 @@ i915_gem_execbuffer_relocate_entry(struct drm_i915_gem_object *obj, * pipe_control writes because the gpu doesn't properly redirect them * through the ppgtt for non_secure batchbuffers. */ if (unlikely(IS_GEN6(dev) - reloc-write_domain == I915_GEM_DOMAIN_INSTRUCTION - !(target_vma-bound GLOBAL_BIND))) { + reloc-write_domain == I915_GEM_DOMAIN_INSTRUCTION)) { ret = i915_vma_bind(target_vma, target_i915_obj-cache_level, - GLOBAL_BIND); + PIN_GLOBAL); if (WARN_ONCE(ret, Unexpected failure to bind target VMA!)) return ret; } @@ -585,7 +584,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, uint64_t flags; int ret; - flags = 0; + flags = PIN_EXECBUF; if (!drm_mm_node_allocated(vma-node)) {
[Intel-gfx] [PATCH 14/17] drm/i915: Move ppgtt_bind/unbind around
Again avoids some forward declarations. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_gtt.c | 44 - 1 file changed, 19 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 00c4a336733f..f07659042928 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -145,8 +145,25 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) static void ppgtt_bind_vma(struct i915_vma *vma, enum i915_cache_level cache_level, - u32 flags); -static void ppgtt_unbind_vma(struct i915_vma *vma); + u32 unused) +{ + u32 pte_flags = 0; + + /* Currently applicable only to VLV */ + if (vma-obj-gt_ro) + pte_flags |= PTE_READ_ONLY; + + vma-vm-insert_entries(vma-vm, vma-obj-pages, vma-node.start, + cache_level, pte_flags); +} + +static void ppgtt_unbind_vma(struct i915_vma *vma) +{ + vma-vm-clear_range(vma-vm, +vma-node.start, +vma-obj-base.size, +true); +} static inline gen8_pte_t gen8_pte_encode(dma_addr_t addr, enum i915_cache_level level, @@ -1604,29 +1621,6 @@ void i915_ppgtt_release(struct kref *kref) kfree(ppgtt); } -static void -ppgtt_bind_vma(struct i915_vma *vma, - enum i915_cache_level cache_level, - u32 unused) -{ - u32 pte_flags = 0; - - /* Currently applicable only to VLV */ - if (vma-obj-gt_ro) - pte_flags |= PTE_READ_ONLY; - - vma-vm-insert_entries(vma-vm, vma-obj-pages, vma-node.start, - cache_level, pte_flags); -} - -static void ppgtt_unbind_vma(struct i915_vma *vma) -{ - vma-vm-clear_range(vma-vm, -vma-node.start, -vma-obj-base.size, -true); -} - extern int intel_iommu_gfx_mapped; /* Certain Gen5 chipsets require require idling the GPU before * unmapping anything from the GTT when VT-d is enabled. -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 15/17] drm/i915: Unduplicate i915_ggtt_unbind/bind_vma
ggtt_bind/unbind_vma already has checks for aliasing ppgtt or not, there's nothing else magic they do. Resurrect i915_ggtt_insert_entries to make the reuse possibel. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_gtt.c | 27 --- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index f07659042928..2f8a113cbfb9 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -1876,19 +1876,16 @@ static void gen6_ggtt_clear_range(struct i915_address_space *vm, readl(gtt_base); } - -static void i915_ggtt_bind_vma(struct i915_vma *vma, - enum i915_cache_level cache_level, - u32 unused) +static void i915_ggtt_insert_entries(struct i915_address_space *vm, +struct sg_table *pages, +uint64_t start, +enum i915_cache_level cache_level, u32 unused) { - const unsigned long entry = vma-node.start PAGE_SHIFT; unsigned int flags = (cache_level == I915_CACHE_NONE) ? AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY; - BUG_ON(!i915_is_ggtt(vma-vm)); - intel_gtt_insert_sg_entries(vma-ggtt_view.pages, entry, flags); + intel_gtt_insert_sg_entries(pages, start PAGE_SHIFT, flags); - vma-bound |= GLOBAL_BIND; } static void i915_ggtt_clear_range(struct i915_address_space *vm, @@ -1901,15 +1898,6 @@ static void i915_ggtt_clear_range(struct i915_address_space *vm, intel_gtt_clear_range(first_entry, num_entries); } -static void i915_ggtt_unbind_vma(struct i915_vma *vma) -{ - const unsigned int first = vma-node.start PAGE_SHIFT; - const unsigned int size = vma-obj-base.size PAGE_SHIFT; - - BUG_ON(!i915_is_ggtt(vma-vm)); - intel_gtt_clear_range(first, size); -} - static void ggtt_bind_vma(struct i915_vma *vma, enum i915_cache_level cache_level, u32 flags) @@ -2471,9 +2459,10 @@ static int i915_gmch_probe(struct drm_device *dev, intel_gtt_get(gtt_total, stolen, mappable_base, mappable_end); dev_priv-gtt.do_idle_maps = needs_idle_maps(dev_priv-dev); + dev_priv-gtt.base.insert_entries = i915_ggtt_insert_entries; dev_priv-gtt.base.clear_range = i915_ggtt_clear_range; - dev_priv-gtt.base.bind_vma = i915_ggtt_bind_vma; - dev_priv-gtt.base.unbind_vma = i915_ggtt_unbind_vma; + dev_priv-gtt.base.bind_vma = ggtt_bind_vma; + dev_priv-gtt.base.unbind_vma = ggtt_unbind_vma; if (unlikely(dev_priv-gtt.do_idle_maps)) DRM_INFO(applying Ironlake quirks for intel_iommu\n); -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/17] drm/i915: Don't use atomics for pg_dirty_rings
It's already protected by the bkl^Wdev-struct_mutex. While at it realign some related code. Signed-off-by: Daniel Vetter daniel.vet...@intel.com --- drivers/gpu/drm/i915/i915_gem_context.c| 16 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 5 ++--- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index 6b0962db2cf7..5a47eb5e3c5d 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -578,11 +578,9 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring, if (to-remap_slice) return false; - if (to-ppgtt) { - if (from == to !test_bit(ring-id, - to-ppgtt-pd_dirty_rings)) - return true; - } + if (to-ppgtt from == to + !(intel_ring_flag(ring) to-ppgtt-pd_dirty_rings)) + return true; return false; } @@ -668,7 +666,7 @@ static int do_switch(struct intel_engine_cs *ring, goto unpin_out; /* Doing a PD load always reloads the page dirs */ - clear_bit(ring-id, to-ppgtt-pd_dirty_rings); + to-ppgtt-pd_dirty_rings = ~intel_ring_flag(ring); } if (ring != dev_priv-ring[RCS]) { @@ -696,12 +694,14 @@ static int do_switch(struct intel_engine_cs *ring, * space. This means we must enforce that a page table load * occur when this occurs. */ } else if (to-ppgtt - test_and_clear_bit(ring-id, to-ppgtt-pd_dirty_rings)) + (intel_ring_flag(ring) to-ppgtt-pd_dirty_rings)) { hw_flags |= MI_FORCE_RESTORE; + to-ppgtt-pd_dirty_rings = ~intel_ring_flag(ring); + } /* We should never emit switch_mm more than once */ WARN_ON(needs_pd_load_pre(ring, to) - needs_pd_load_post(ring, to, hw_flags)); + needs_pd_load_post(ring, to, hw_flags)); ret = mi_set_context(ring, to, hw_flags); if (ret) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 1d5badf1b887..0b06c6de27de 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1247,9 +1247,8 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file, if (ret) goto error; - if (ctx-ppgtt) - WARN(ctx-ppgtt-pd_dirty_rings (1ring-id), - %s didn't clear reload\n, ring-name); + WARN(ctx-ppgtt ctx-ppgtt-pd_dirty_rings (1ring-id), +%s didn't clear reload\n, ring-name); instp_mode = args-flags I915_EXEC_CONSTANTS_MASK; instp_mask = I915_EXEC_CONSTANTS_MASK; -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 16/17] drm/i915: Don't try to outsmart gcc in i915_gem_gtt.c
Sprinkling static inline all over the place is carg-culting. Remove it. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_gtt.c | 38 ++--- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 2f8a113cbfb9..458819b99a0b 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -165,9 +165,9 @@ static void ppgtt_unbind_vma(struct i915_vma *vma) true); } -static inline gen8_pte_t gen8_pte_encode(dma_addr_t addr, -enum i915_cache_level level, -bool valid) +static gen8_pte_t gen8_pte_encode(dma_addr_t addr, + enum i915_cache_level level, + bool valid) { gen8_pte_t pte = valid ? _PAGE_PRESENT | _PAGE_RW : 0; pte |= addr; @@ -187,9 +187,9 @@ static inline gen8_pte_t gen8_pte_encode(dma_addr_t addr, return pte; } -static inline gen8_pde_t gen8_pde_encode(struct drm_device *dev, - dma_addr_t addr, - enum i915_cache_level level) +static gen8_pde_t gen8_pde_encode(struct drm_device *dev, + dma_addr_t addr, + enum i915_cache_level level) { gen8_pde_t pde = _PAGE_PRESENT | _PAGE_RW; pde |= addr; @@ -299,8 +299,8 @@ static gen6_pte_t iris_pte_encode(dma_addr_t addr, #define i915_dma_unmap_single(px, dev) \ __i915_dma_unmap_single((px)-daddr, dev) -static inline void __i915_dma_unmap_single(dma_addr_t daddr, - struct drm_device *dev) +static void __i915_dma_unmap_single(dma_addr_t daddr, + struct drm_device *dev) { struct device *device = dev-pdev-dev; @@ -321,9 +321,9 @@ static inline void __i915_dma_unmap_single(dma_addr_t daddr, #define i915_dma_map_single(px, dev) \ i915_dma_map_page_single((px)-page, (dev), (px)-daddr) -static inline int i915_dma_map_page_single(struct page *page, - struct drm_device *dev, - dma_addr_t *daddr) +static int i915_dma_map_page_single(struct page *page, + struct drm_device *dev, + dma_addr_t *daddr) { struct device *device = dev-pdev-dev; @@ -1268,7 +1268,7 @@ static void gen6_ppgtt_insert_entries(struct i915_address_space *vm, * are switching between contexts with the same LRCA, we also must do a force * restore. */ -static inline void mark_tlbs_dirty(struct i915_hw_ppgtt *ppgtt) +static void mark_tlbs_dirty(struct i915_hw_ppgtt *ppgtt) { /* If current vm != vm, */ ppgtt-pd_dirty_rings = INTEL_INFO(ppgtt-base.dev)-ring_mask; @@ -1625,7 +1625,7 @@ extern int intel_iommu_gfx_mapped; /* Certain Gen5 chipsets require require idling the GPU before * unmapping anything from the GTT when VT-d is enabled. */ -static inline bool needs_idle_maps(struct drm_device *dev) +static bool needs_idle_maps(struct drm_device *dev) { #ifdef CONFIG_INTEL_IOMMU /* Query intel_iommu to see if we need the workaround. Presumably that @@ -1731,7 +1731,7 @@ int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj) return 0; } -static inline void gen8_set_pte(void __iomem *addr, gen8_pte_t pte) +static void gen8_set_pte(void __iomem *addr, gen8_pte_t pte) { #ifdef writeq writeq(pte, addr); @@ -2154,14 +2154,14 @@ static void teardown_scratch_page(struct drm_device *dev) __free_page(page); } -static inline unsigned int gen6_get_total_gtt_size(u16 snb_gmch_ctl) +static unsigned int gen6_get_total_gtt_size(u16 snb_gmch_ctl) { snb_gmch_ctl = SNB_GMCH_GGMS_SHIFT; snb_gmch_ctl = SNB_GMCH_GGMS_MASK; return snb_gmch_ctl 20; } -static inline unsigned int gen8_get_total_gtt_size(u16 bdw_gmch_ctl) +static unsigned int gen8_get_total_gtt_size(u16 bdw_gmch_ctl) { bdw_gmch_ctl = BDW_GMCH_GGMS_SHIFT; bdw_gmch_ctl = BDW_GMCH_GGMS_MASK; @@ -2177,7 +2177,7 @@ static inline unsigned int gen8_get_total_gtt_size(u16 bdw_gmch_ctl) return bdw_gmch_ctl 20; } -static inline unsigned int chv_get_total_gtt_size(u16 gmch_ctrl) +static unsigned int chv_get_total_gtt_size(u16 gmch_ctrl) { gmch_ctrl = SNB_GMCH_GGMS_SHIFT; gmch_ctrl = SNB_GMCH_GGMS_MASK; @@ -2188,14 +2188,14 @@ static inline unsigned int chv_get_total_gtt_size(u16 gmch_ctrl) return 0; } -static inline size_t gen6_get_stolen_size(u16 snb_gmch_ctl) +static size_t gen6_get_stolen_size(u16 snb_gmch_ctl) { snb_gmch_ctl = SNB_GMCH_GMS_SHIFT; snb_gmch_ctl = SNB_GMCH_GMS_MASK; return
Re: [Intel-gfx] [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space
On Tue, Apr 14, 2015 at 05:12:30PM +0100, Chris Wilson wrote: On Tue, Apr 14, 2015 at 05:09:27PM +0100, Chris Wilson wrote: On Tue, Apr 14, 2015 at 05:35:12PM +0200, Daniel Vetter wrote: They change with the address space and not with each vma, so move them into the right pile of vfuncs. Save 2 pointers per vma and clarifies the code. Using per-vma vfunc allows you make, for example, the pagetables themselves an ordinary vma in the GGTT and so operate identically wrt to the shrinker and eviction logic - removing some very fragile code in the process. A vma-ops would be an interesting compromise. Tbh still not really sold on this idea, since the complexit tends to be in the recursion. E.g. see all the fun we had with gpu_idle and the default context. So for now I still prefer things to be explicit. Also that would be a new op to first (try) to unuse the dma. If we don't do this and it fails then the shrinker gets annoyed since the nice hole-punching doesn't work correctly. There's also the fairness question, we'd need to make sure that e.g. the hw context gets cycled through the active list even when we don't switch contexts. Given all that I don't think this patch here is a blocker for doing other vma ops. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v3] i-g-t: Adding test case to test background color.
-Original Message- From: Thomas Wood [mailto:thomas.w...@intel.com] Sent: Tuesday, April 14, 2015 10:02 AM To: Konduru, Chandra Cc: Intel Graphics Development Subject: Re: [Intel-gfx] [PATCH v3] i-g-t: Adding test case to test background color. On 30 March 2015 at 21:44, Chandra Konduru chandra.kond...@intel.com wrote: From: chandra konduru chandra.kond...@intel.com Adding i-g-t test case to test display crtc background color. v2: - Added IGT_TEST_DESCRIPTION() (Thomas Wood) - Added to .gitignore (Thomas Wood) - Added additional details to function header (Thomas Wood) - Simplified igt_main (Thomas Wood) v3: - rebased to latest master (me) - took sleep calls out (Daniel) - use new tiled types when calling igt_create_fb (me) I've pushed this patch and the panel fitting and plane scaling test cases, but I have switched them all to use igt_simple_main and remove the single subtest, which I think was your intention in v2 of this patch. That should be ok. Thanks for merging. Signed-off-by: chandra konduru chandra.kond...@intel.com --- lib/igt_kms.c | 61 lib/igt_kms.h | 4 + tests/.gitignore | 1 + tests/Android.mk | 1 + tests/Makefile.sources| 1 + tests/kms_crtc_background_color.c | 195 ++ 6 files changed, 263 insertions(+) create mode 100644 tests/kms_crtc_background_color.c diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 9c131f0..9c46e18 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -940,6 +940,22 @@ igt_plane_set_property(igt_plane_t *plane, uint32_t prop_id, uint64_t value) DRM_MODE_OBJECT_PLANE, prop_id, value); } +static bool +get_crtc_property(int drm_fd, uint32_t crtc_id, const char *name, + uint32_t *prop_id /* out */, uint64_t *value /* out */, + drmModePropertyPtr *prop /* out */) { + return kmstest_get_property(drm_fd, crtc_id, DRM_MODE_OBJECT_CRTC, + name, prop_id, value, prop); } + +static void +igt_crtc_set_property(igt_output_t *output, uint32_t prop_id, +uint64_t value) { + drmModeObjectSetProperty(output-display-drm_fd, + output-config.crtc-crtc_id, DRM_MODE_OBJECT_CRTC, +prop_id, value); } + /* * Walk a plane's property list to determine its type. If we don't * find a type property, then the kernel doesn't support universal @@ -1097,6 +1113,7 @@ void igt_display_init(igt_display_t *display, int drm_fd) igt_assert(display-outputs); for (i = 0; i display-n_outputs; i++) { + int j; igt_output_t *output = display-outputs[i]; /* @@ -1108,6 +1125,19 @@ void igt_display_init(igt_display_t *display, int drm_fd) output-display = display; igt_output_refresh(output); + + for (j = 0; j display-n_pipes; j++) { + uint64_t prop_value; + igt_pipe_t *pipe = display-pipes[j]; + if (output-config.crtc) { + get_crtc_property(display-drm_fd, output-config.crtc- crtc_id, + background_color, + pipe-background_property, + prop_value, + NULL); + pipe-background = (uint32_t)prop_value; + } + } } drmModeFreePlaneResources(plane_resources); @@ -1527,6 +1557,13 @@ static int igt_output_commit(igt_output_t *output, pipe = igt_output_get_driving_pipe(output); + if (pipe-background_changed) { + igt_crtc_set_property(output, pipe-background_property, + pipe-background); + + pipe-background_changed = false; + } + for (i = 0; i pipe-n_planes; i++) { igt_plane_t *plane = pipe-planes[i]; @@ -1779,6 +1816,30 @@ void igt_plane_set_rotation(igt_plane_t *plane, igt_rotation_t rotation) plane-rotation_changed = true; } +/** + * igt_crtc_set_background: + * @pipe: pipe pointer to which background color to be set + * @background: background color value in BGR 16bpc + * + * Sets background color for requested pipe. Color value provided +here + * will be actually submitted at output commit time via background_color + * property. + * For example to get red as background, set background = 0x. + */ +void igt_crtc_set_background(igt_pipe_t *pipe, uint64_t background) { + igt_display_t *display =
Re: [Intel-gfx] [PATCH 06/17] drm/i915: Dont clear PIN_GLOBAL in the execbuf pinning fallback
On Tue, Apr 14, 2015 at 05:35:16PM +0200, Daniel Vetter wrote: PIN_GLOBAL is set only when userspace asked for it, and that is only the case for the gen6 PIPE_CONTROL workaround. We're not allowed to just clear this. Nope. See only_mappable_for_reloc(). There is an issue here, but this is not it. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 06/17] drm/i915: Dont clear PIN_GLOBAL in the execbuf pinning fallback
On Tue, Apr 14, 2015 at 04:53:24PM +0100, Chris Wilson wrote: On Tue, Apr 14, 2015 at 05:35:16PM +0200, Daniel Vetter wrote: PIN_GLOBAL is set only when userspace asked for it, and that is only the case for the gen6 PIPE_CONTROL workaround. We're not allowed to just clear this. Nope. See only_mappable_for_reloc(). There is an issue here, but this is not it. Less cyptic, I think you want: diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index a60bfeadc4fb..8599cd87cce5 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -735,14 +735,14 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, int ret; flags = 0; + if (entry-flags EXEC_OBJECT_NEEDS_GTT) + flags |= PIN_GLOBAL; if (!drm_mm_node_allocated(vma-node)) { if (entry-flags __EXEC_OBJECT_NEEDS_MAP) { flags |= PIN_GLOBAL | PIN_MAPPABLE; if (only_mappable_for_reloc(entry-flags)) flags |= PIN_NONBLOCK; } - if (entry-flags EXEC_OBJECT_NEEDS_GTT) - flags |= PIN_GLOBAL; if (entry-flags __EXEC_OBJECT_NEEDS_BIAS) flags |= BATCH_OFFSET_BIAS | PIN_OFFSET_BIAS; if (entry-flags EXEC_OBJECT_PINNED) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] intel: Leak the userptr test bo
In order to use userptr, the kernel tracks the owner's mm with a mmu_notifier. Setting that is very expensive - it involves taking all mm_locks and a stop_machine(). This tracking lives only for as long as the client is using userptr objects - so if the client allocates then frees a userptr in a loop, we will be executing that heavyweight setup everytime. To ammoritize this cost, just leak the test bo and the single backing page we use for detecting userptr. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Tvrtko Ursulin tvrtko.ursu...@intel.com --- intel/intel_bufmgr_gem.c | 15 ++- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index a938441..51f8afe 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -976,15 +976,12 @@ retry: return false; } - memclear(close_bo); - close_bo.handle = userptr.handle; - ret = drmIoctl(bufmgr_gem-fd, DRM_IOCTL_GEM_CLOSE, close_bo); - free(ptr); - if (ret) { - fprintf(stderr, Failed to release test userptr object! (%d) - i915 kernel driver may not be sane!\n, errno); - return false; - } + /* We don't release the userptr bo here as we want to keep the +* kernel mm tracking alive for our lifetime. The first time we +* create a userptr object the kernel has to install a mmu_notifer +* which is a heavyweight operation (e.g. it requires taking all +* mm_locks and stop_machine()). +*/ return true; } -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Dont clear PIN_GLOBAL in the execbuf pinning fallback
PIN_GLOBAL is set only when userspace asked for it, and that is only the case for the gen6 PIPE_CONTROL workaround. We're not allowed to just clear this. The important part of the fallback is to drop the restriction to the mappable range. This issue has been introduced in commit edf4427b8055dc93eb5222d8174b07a75ba24fb5 Author: Chris Wilson ch...@chris-wilson.co.uk Date: Wed Jan 14 11:20:56 2015 + drm/i915: Fallback to using CPU relocations for large batch buffers v2: Chris pointed out that we also miss to set PIN_GLOBAL when the buffer is already bound. Fix this up too. Cc: Chris Wilson ch...@chris-wilson.co.uk Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index 48ac5608481e..564425fce1a7 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -586,11 +586,12 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, int ret; flags = 0; + if (entry-flags EXEC_OBJECT_NEEDS_GTT) + flags |= PIN_GLOBAL; + if (!drm_mm_node_allocated(vma-node)) { if (entry-flags __EXEC_OBJECT_NEEDS_MAP) flags |= PIN_GLOBAL | PIN_MAPPABLE; - if (entry-flags EXEC_OBJECT_NEEDS_GTT) - flags |= PIN_GLOBAL; if (entry-flags __EXEC_OBJECT_NEEDS_BIAS) flags |= BATCH_OFFSET_BIAS | PIN_OFFSET_BIAS; } @@ -600,7 +601,7 @@ i915_gem_execbuffer_reserve_vma(struct i915_vma *vma, only_mappable_for_reloc(entry-flags)) ret = i915_gem_object_pin(obj, vma-vm, entry-alignment, - flags ~(PIN_GLOBAL | PIN_MAPPABLE)); + flags ~PIN_MAPPABLE); if (ret) return ret; -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ns2501 DVO - success at last
On Mon, Apr 13, 2015 at 09:00:48PM +0200, Thomas Richter wrote: Hi Daniel, hi Ville, some success at last. I couldn't stop myself playing with the NatSemi 2501 DVO in my Fujitsu S6010 and I believe I finally got a hang on this chip. I believe I understand now most of the undocumented registers. There are also a couple of additional features that are, apparently, not used by the video BIOS of the S6010, namely the chip has a ditherer on board - quite like the Intel Video Controller hub in the IBM R31. Unfortunately, to enable the scaler, the bypass must be turned off, and hence, parameters for a 1:1 through-mapping of the scaler are required. After quite some experimenting, I believe I found now the right settings to enable the scaler and configure it to pass the 1024x768 input to the output. The chip is really a bit weird. It not only requires the scaling factors, but also the input timings, (sync width, front/back porch for both horizontal and vertical) and the output timing, and the configuration of its PLL to sample the incoming data. Currently, most of the data I obtained by trail and error, at least for the 1024x768 mode in which the bios configures the DVO in bypass mode. It turned out we forgot to configure a couple of registers (and some others are pretty much blank). Thus, my question at this time is whether there is any interface how to get the precise timing of the loaded video mode from the i915 module directly instead of second-guessing the parameters, i.e. dimensions of the frame, porch sizes, size of the sync pulses, pixel clock and so on. Other than that, I'll try to clean up the code I have to so far in the next days and release it. In the mode structure that gets passed to your dvo driver look for the crtc_* values, those are the exact timings you need to set up. You want to look at the adjusted_mode since that's the one that actually gets sent to the dvo port, the other mode is the one userspace request and will get munged a bit. btw the sdvo code works really similar and also has input and output timings for the transcoder chip. You could peak at that code to see how it's all done. Cheers, Daniel PS: You're replies are still attached to some random thread, which makes them harder to spot and not miss ... -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/11] drm/i915: Move the DPCD read further up in intel_dp_check_link_status()
Trying again and adding the list this time instead of just replying to myself This is just an artifact of moving things around, as it likely was in the other patches. The only reason I will move comments is to clarify what they pertain to if code is moving around it. In any case, it's back where it belongs in the updated patch. -T On 4/10/15 9:12 AM, Todd Previte wrote: Move the DPCD read to the top and check for an interrupt from the sink to catch Displayport automated testing requests necessary to support Displayport compliance testing. The checks for active connectors and link status are moved below the check for the interrupt. The main reason for doing this is to make sure that a test request isn't missed. Checking for the status of the encoder/crtc isn't necessary for some test cases (AUX channel tests are one example) and without moving the check for the interrupt, these tests may not execute if one of those checks fails. Additionally, if reading the DPCD fails, regardless of whether or not testing is happening, there's no way to train the link since configurations and status can't be read, nor can link training parameters be written. V1: - This is the second part of the single-patch split previously mentioned. Signed-off-by: Todd Previte tprev...@gmail.com --- drivers/gpu/drm/i915/intel_dp.c | 15 ++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 30cd433..23184b0 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3913,13 +3913,26 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) drm_dp_dpcd_writeb(intel_dp-aux, DP_DEVICE_SERVICE_IRQ_VECTOR, sink_irq_vector); - if (sink_irq_vector DP_AUTOMATED_TEST_REQUEST) intel_dp_handle_test_request(intel_dp); if (sink_irq_vector (DP_CP_IRQ | DP_SINK_SPECIFIC_IRQ)) DRM_DEBUG_DRIVER(CP or sink specific irq unhandled\n); } + if (!intel_encoder-connectors_active) + return; + + if (WARN_ON(!intel_encoder-base.crtc)) + return; + + if (!to_intel_crtc(intel_encoder-base.crtc)-active) + return; + + /* Try to read receiver status if the link appears to be up */ + if (!intel_dp_get_link_status(intel_dp, link_status)) { + return; + } + if (!drm_dp_channel_eq_ok(link_status, intel_dp-lane_count)) { DRM_DEBUG_KMS(%s: channel EQ not ok, retraining\n, intel_encoder-base.name); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt
On Tue, Apr 14, 2015 at 07:11:25PM +0200, Daniel Vetter wrote: On Tue, Apr 14, 2015 at 05:06:36PM +0100, Chris Wilson wrote: On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote: We load the ppgtt ptes once per gpu reset/driver load/resume and that's all that's needed. Note that this only blows up when we're using the allocate_va_range funcs and not the special-purpose ones used. With this change we can get rid of that duplication. Honestly, I would prefer the test to be rewritten so that the information on which vm was being used was passed along and not that magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly what vm it bound the objects into, and yet towards the end we decide to guess again. Also, I would rather the execbuffer test be moved to i915_gem_context since it is scattering internal knowledge about. Yeah I agree that there's more room for cleanup. I've done this minimal patch purely to shut up the bogus WARN_ONs when I tried to unify the gen6/7 pt alloc code in the next patch. Since it's bogus. How about: diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c index ed9232126644..be0f475ee1e5 100644 --- a/drivers/gpu/drm/i915/i915_gem_context.c +++ b/drivers/gpu/drm/i915/i915_gem_context.c @@ -638,22 +638,14 @@ mi_set_context(struct intel_engine_cs *ring, static inline bool should_skip_switch(struct intel_engine_cs *ring, struct intel_context *from, - struct intel_context *to) + struct intel_context *to, + struct i915_hw_ppgtt *ppgtt) { - struct drm_i915_private *dev_priv = ring-dev-dev_private; - if (to-remap_slice) return false; - if (to-ppgtt) { - if (from == to !test_bit(ring-id, - to-ppgtt-pd_dirty_rings)) - return true; - } else if (dev_priv-mm.aliasing_ppgtt) { - if (from == to !test_bit(ring-id, - dev_priv-mm.aliasing_ppgtt-pd_dirty_rings)) - return true; - } + if (from == to !test_bit(ring-id, ppgtt-pd_dirty_rings)) + return true; return false; } @@ -661,15 +653,13 @@ static inline bool should_skip_switch(struct intel_engine_cs *ring, static bool needs_pd_load_pre(struct intel_engine_cs *ring, struct intel_context *to) { - struct drm_i915_private *dev_priv = ring-dev-dev_private; - if (!to-ppgtt) return false; if (INTEL_INFO(ring-dev)-gen 8) return true; - if (ring != dev_priv-ring[RCS]) + if (ring-id != RCS) return true; return false; @@ -679,15 +669,13 @@ static bool needs_pd_load_post(struct intel_engine_cs *ring, struct intel_context *to, u32 hw_flags) { - struct drm_i915_private *dev_priv = ring-dev-dev_private; - if (!to-ppgtt) return false; if (!IS_GEN8(ring-dev)) return false; - if (ring != dev_priv-ring[RCS]) + if (ring-id != RCS) return false; if (hw_flags MI_RESTORE_INHIBIT) @@ -701,14 +689,15 @@ static int do_switch(struct intel_engine_cs *ring, { struct drm_i915_private *dev_priv = ring-dev-dev_private; struct intel_context *from = ring-last_context; + struct i915_hw_ppgtt *ppgtt = to-ppgtt ?: dev_priv-mm.aliasing_ppgtt; u32 hw_flags = 0; bool uninitialized = false; int ret, i; - if (from != NULL ring == dev_priv-ring[RCS]) + if (from != NULL ring-id == RCS) BUG_ON(from-legacy_hw_ctx.rcs_vma == NULL); - if (should_skip_switch(ring, from, to)) + if (should_skip_switch(ring, from, to, ppgtt)) return 0; /* Trying to pin first makes error handling easier. */ @@ -851,6 +840,9 @@ done: i915_gem_context_reference(to); ring-last_context = to; + WARN(ppgtt-pd_dirty_rings (1ring-id), +%s didn't clear reload\n, ring-name); + if (uninitialized) { if (ring-init_context) { ret = ring-init_context(ring, to); diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index ef644e7eaac0..595daecb3283 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1385,13 +1385,6 @@ i915_gem_ringbuffer_submission(struct drm_device *dev, struct drm_file *file, if (ret) goto error; - if (ctx-ppgtt) - WARN(ctx-ppgtt-pd_dirty_rings (1ring-id), - %s didn't clear reload\n, ring-name); - else if (dev_priv-mm.aliasing_ppgtt) -
Re: [Intel-gfx] [PATCH 04/13] drm/i915: Add EDID read in intel_dp_check_link_status() for Link CTS 4.2.2.1
2015-04-13 11:53 GMT-03:00 Todd Previte tprev...@gmail.com: Adds in an EDID read after the DPCD read to accommodate test 4.2.2.1 in the Displayport Link CTS Core 1.2 rev1.1. This test requires an EDID read for all HPD plug events. To reduce the amount of code, this EDID read is also used for Link CTS tests 4.2.2.3, 4.2.2.4, 4.2.2.5 and 4.2.2.6. Actual support for these tests is implemented in later patches in this series. V2: - Fixed compilation error introduced during rework Signed-off-by: Todd Previte tprev...@gmail.com --- drivers/gpu/drm/i915/intel_dp.c | 11 +++ 1 file changed, 11 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 23184b0..75df3e2 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -3890,6 +3890,9 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) { struct drm_device *dev = intel_dp_to_dev(intel_dp); struct intel_encoder *intel_encoder = dp_to_dig_port(intel_dp)-base; + struct drm_connector *connector = intel_dp-attached_connector-base; + struct i2c_adapter *adapter = intel_dp-aux.ddc; + struct edid *edid_read = NULL; u8 sink_irq_vector; u8 link_status[DP_LINK_STATUS_SIZE]; @@ -3906,6 +3909,14 @@ intel_dp_check_link_status(struct intel_dp *intel_dp) return; } + /* Displayport Link CTS Core 1.2 rev1.1 EDID testing +* 4.2.2.1 - EDID read required for all HPD events + */ +edid_read = drm_get_edid(connector, adapter); +if (!edid_read) { +DRM_DEBUG_DRIVER(Invalid EDID detected\n); +} + We already briefly discussed this patch in private, so I'm going to summarize the discussion and also add some more points here. Frist, the actual detailed review: the indentation here is using spaces and we're leaking the EDID. This will cause rebases to a few of the next patches. Back to the hight level architecture: your initial versions of the series contained just 1 extra EDID read, and it was contained inside the compliance testing function. Then the versions submitted a few days ago had 2 extra EDID reads, then after some discussion you reduced to 1 extra EDID read (the one on this patch). I previously asked But what about the automatic EDID read we do when we get a hotplug? Can't we just rely on it?. I got some answers to the question, but I was not really convinced. Yesterday I was arguing that this extra EDID read is going to add a small delay to every hotplug event we get, so my initial suggestion was to organize the compliance testing in a way that would require the user space program to call the GetResources() IOCTL to force the EDID when needed. Your argument was that then the DP compliance testing procedure would be testing our app for compliance, not the Kernel. But today I decided to finally do some debugging regarding this, and I was able to confirm that we do follow the DP requirements: we do have an automatic EDID read done by the Kernel whenever we do a hotplug: i915_hotplug_work_func() calls intel_dp_detect(), which ends calling drm_get_edid() at some point. This function also does other stuff that is required by the compliance testing, such as the DPCD reads. Now there's a problem with using i915_hotplug_work_func(), which could the reason why you rejected it: it only happens after intel_dp_hpd_pulse(), which means that we only really do the EDID read after intel_dp_handle_test_request(). I consider i915_hotplug_work_func() a fundamental part of our DP framework, and the DP compliance testing seems to be just ignoring its existence. So my idea for a solution here would be to make intel_dp_handle_test_request() run on its own delayed work function. It would wait for both i915_digport_work_func() and i915_hotplug_work_func() to finish, and only then it would do the normal processing. With this, we would be able to avoid the edid read on this patch, we would maybe be able to avoid at least part of patch 2, we would maybe be able to completely avoid patch 7, and then on patch 8 we would start touching intel_dp_get_edid() instead. I know this is sort of a fundamental change that is being requested a little late in the review process, and it can be frustrating, but this aspect of the code only recently changed (I was fine with the EDID reads just in the compliance testing function), and since the DP compliance code is quite complex, it took me a while to realize everything that's going on and what is the purpose of each piece. I also think that, since this idea will allow the compliance testing to take into consideration the work done by i915_hotplug_work_func(), compliance testing will better reflect the behavior that is actually done by the Kernel when DP devices are plugged/unplugged. And I did ask about those new EDID reads as soon as I started reviewing the patch that introduced them. Now, since I know how
Re: [Intel-gfx] [PATCH] igt/gem_ctx_exec: Add lrc lite restore subtest
On Tue, Apr 14, 2015 at 04:42:58PM +0100, Michel Thierry wrote: Exercise lite-restore (re-submit a context that is currently running), by queueing several small batchbuffers. This test helps to validate WaIdleLiteRestore. Signed-off-by: Michel Thierry michel.thie...@intel.com Applied, thanks. -Daniel --- tests/gem_ctx_exec.c | 30 +- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/gem_ctx_exec.c b/tests/gem_ctx_exec.c index 43b38a2..3df939c 100644 --- a/tests/gem_ctx_exec.c +++ b/tests/gem_ctx_exec.c @@ -155,7 +155,7 @@ static void big_exec(int fd, uint32_t handle, int ring) uint32_t handle; uint32_t batch[2] = {0, MI_BATCH_BUFFER_END}; -uint32_t ctx_id; +uint32_t ctx_id, ctx_id2; int fd; igt_main @@ -215,4 +215,32 @@ igt_main gem_context_destroy(fd, ctx_id); } + + igt_subtest(lrc-lite-restore) { + int i, j; + + /* + * Need 2 contexts to be able to replicate a lite restore, + * i.e. a running context is resubmitted. + */ + ctx_id = gem_context_create(fd); + ctx_id2 = gem_context_create(fd); + + /* + * Queue several small batchbuffers to be sure we'll send execlists + * with 2 valid context, and likely cause a lite restore when ctxB + * is resubmitted at the top of the new execlist. + */ + for (i = 0; i 20; i++) { + for (j = 0; j 200; j++) { + igt_assert(exec(fd, handle, I915_EXEC_RENDER, ctx_id) == 0); + igt_assert(exec(fd, handle, I915_EXEC_RENDER, ctx_id2) == 0); + } + + gem_sync(fd, handle); + } + + gem_context_destroy(fd, ctx_id); + gem_context_destroy(fd, ctx_id2); + } } -- 2.3.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space
On Tue, Apr 14, 2015 at 07:08:35PM +0200, Daniel Vetter wrote: On Tue, Apr 14, 2015 at 05:12:30PM +0100, Chris Wilson wrote: On Tue, Apr 14, 2015 at 05:09:27PM +0100, Chris Wilson wrote: On Tue, Apr 14, 2015 at 05:35:12PM +0200, Daniel Vetter wrote: They change with the address space and not with each vma, so move them into the right pile of vfuncs. Save 2 pointers per vma and clarifies the code. Using per-vma vfunc allows you make, for example, the pagetables themselves an ordinary vma in the GGTT and so operate identically wrt to the shrinker and eviction logic - removing some very fragile code in the process. A vma-ops would be an interesting compromise. Tbh still not really sold on this idea, since the complexit tends to be in the recursion. E.g. see all the fun we had with gpu_idle and the default context. So for now I still prefer things to be explicit. Ah, you mean the fun that gpu_idle is broken at the moment. I think that lends weight to my argument tbh. Also that would be a new op to first (try) to unuse the dma. If we don't do this and it fails then the shrinker gets annoyed since the nice hole-punching doesn't work correctly. There's also the fairness question, we'd need to make sure that e.g. the hw context gets cycled through the active list even when we don't switch contexts. Yes, all that was taken into account by my patches to sanitize request handling. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/11] drm/i915: Update intel_dp_hpd_pulse() for non-MST operation
On 4/14/15 4:29 AM, Paulo Zanoni wrote: 2015-04-10 13:12 GMT-03:00 Todd Previte tprev...@gmail.com: Update the hot plug function to handle the SST case. Instead of placing the SST case within the long/short pulse block, it is now handled after determining that MST mode is not in use. This way, the topology management layer can handle any MST-related operations while SST operations are still correctly handled afterwards. This patch also corrects the problem of SST mode only being handled in the case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes both short and long pulses are used by the different tests, thus both cases need to be addressed for SST. This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the previous compliance testing patch sequence. Review feedback on that patch indicated that updating intel_dp_hot_plug() was not the correct place for the test handler. For the SST case, the main stream is disabled for long HPD pulses as this generally indicates either a connect/disconnect event or link failure. For a number of case in compliance testing, the source is required to disable the main link upon detection of a long HPD. V2: - N/A V3: - Place the SST mode link status check into the mst_fail case - Remove obsolete comment regarding SST mode operation - Removed an erroneous line of code that snuck in during rebasing V4: - Added a disable of the main stream (DP transport) for the long pulse case for SST to support compliance testing V5: - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 V6: - Reformatted a comment Signed-off-by: Todd Previte tprev...@gmail.com --- drivers/gpu/drm/i915/intel_dp.c | 19 --- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 77b6b15..ba2da44 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4572,29 +4572,26 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) if (intel_dp_check_mst_status(intel_dp) == -EINVAL) goto mst_fail; } - - if (!intel_dp-is_mst) { - /* -* we'll check the link status via the normal hot plug path later - -* but for short hpds we should check it now -*/ - drm_modeset_lock(dev-mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); - drm_modeset_unlock(dev-mode_config.connection_mutex); - } } Shouldn't the code be moved to exactly this spot instead of after the put_power label? Why would we want to call check_link_status in case we goto mst_fail? In case there is a valid reason, maybe it would be better to do a big reorganization of this function because it's going to start looking very weird - or at least rename the labels. No because then you don't get long pulses, only short ones. The put_power case is where this belongs, unless you want to duplicate code in both the long_pulse and the else clause. There is a separate mst_check_link_status call so this one is specific to SST mode. There is also a check to make sure it doesn't get called when MST is active and MST has hit a failure mode, so that is a non-issue. Also, for the long_hpd case, I see that check_link_status() will redo some of the stuff we already did on this function, such as get_dpcd(). And if you follow my advice on patch 2, you will end up having even more repeated code. I think you could try to do a careful analysis here to make sure we're not calling stuff twice here, especially since some of those operations are potentially slow. I see a couple places where the code is duplicated, specifically the connection check (which I encapsulated in a function and I'll likely roll forward into this one since it makes things more clear) and the DPCD read in the long pulse case. I removed the code in check_link_status for both of these things and it still passes compliance. Good catch Paulo. This has been fixed and tested and will be in the updated patch posted shortly. ret = IRQ_HANDLED; goto put_power; mst_fail: - /* if we were in MST mode, and device is not there get out of MST mode */ if (intel_dp-is_mst) { + /* if we were in MST mode, and device is not there get out of MST mode */ I don't see the need for changes such as the one above - I saw similar cases in other patches you submitted. I often use git blame on comments in order to be able to see the whole context of the change, and a simple change like this makes it harder to blame. Also, you're not even fixing the 80 column problem here. And I do prefer the comment on top of the if statement. This is just an artifact of moving things around, as it
Re: [Intel-gfx] [PATCH 08/13] drm/i915: Support EDID compliance tests with the intel_dp_autotest_edid() function
On 4/14/15 6:35 AM, Paulo Zanoni wrote: 2015-04-10 14:42 GMT-03:00 Todd Previte tprev...@gmail.com: Updates the EDID compliance test function to perform the EDID read as required by the tests. This read needs to take place in the kernel for reasons of speed and efficiency. The results of the EDID read operations are handed off to userspace so that the userspace app can set the display mode appropriately for the test response. The compliance_test_active flag now appears at the end of the individual test handling functions. This is so that the kernel-side operations can be completed without the risk of interruption from the userspace app that is polling on that flag. V2: - Addressed mailing list feedback - Removed excess debug messages - Removed extraneous comments - Fixed formatting issues (line length 80) - Updated the debug message in compute_edid_checksum to output hex values instead of decimal V3: - Addressed more list feedback - Added the test_active flag to the autotest function - Removed test_active flag from handler - Added failsafe check on the compliance test active flag at the end of the test handler - Fixed checkpatch.pl issues V4: - Removed the checksum computation function and its use as it has been rendered superfluous by changes to the core DRM EDID functions - Updated to use the raw header corruption detection mechanism - Moved the declaration of the test_data variable here V5: - Update test active flag variable name to match the change in the first patch of the series. - Relocated the test active flag declaration and initialization to this patch V6: - Updated to use the new flag for raw EDID header corruption - Removed the extra EDID read from the autotest function - Added the edid_checksum variable to struct intel_dp so that the autotest function can write it to the sink device - Moved the update to the hpd_pulse function to another patch - Removed extraneous constants V7: - Fixed erroneous placement of the checksum assignment. In some cases such as when the EDID read fails and is NULL, this causes a NULL ptr dereference in the kernel. Bad news. Fixed now. Signed-off-by: Todd Previte tprev...@gmail.com --- drivers/gpu/drm/i915/intel_dp.c | 46 drivers/gpu/drm/i915/intel_drv.h | 4 2 files changed, 50 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index ba2da44..3bfdc40 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -41,6 +41,12 @@ #define DP_LINK_CHECK_TIMEOUT (10 * 1000) +/* Compliance test status bits */ +#define INTEL_DP_RESOLUTION_SHIFT_MASK 4 +#define INTEL_DP_RESOLUTION_PREFERRED (1 INTEL_DP_RESOLUTION_SHIFT_MASK) +#define INTEL_DP_RESOLUTION_STANDARD (2 INTEL_DP_RESOLUTION_SHIFT_MASK) +#define INTEL_DP_RESOLUTION_FAILSAFE (3 INTEL_DP_RESOLUTION_SHIFT_MASK) + struct dp_link_dpll { int link_bw; struct dpll dpll; @@ -3770,6 +3776,35 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp) static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp) { uint8_t test_result = DP_TEST_NAK; + uint32_t ret = 0; + + if (intel_dp-compliance_edid_invalid || + intel_dp-aux.i2c_defer_count 6) { + /* Check for NACKs/DEFERs, use failsafe if detected +* (DP CTS 1.2 Core Rev 1.1, 4.2.2.4, 4.2.2.5) +*/ + if (intel_dp-aux.i2c_nack_count 0 || + intel_dp-aux.i2c_defer_count 0) + DRM_DEBUG_KMS(EDID read had %d NACKs, %d DEFERs\n, + intel_dp-aux.i2c_nack_count, + intel_dp-aux.i2c_defer_count); + intel_dp-compliance_test_data = INTEL_DP_RESOLUTION_FAILSAFE; Since this case is expected to happen, shouldn't we return something that's not DP_TEST_NAK here? The spec does not state whether or not we should ACK or NAK this case. This value is written to the sink device as the response, so for compliance testing purposes, the test device does not appear to care whether it's ACK or NAK. Also, we should try to write the checksum on this case too, shouldn't we? I see on 4.2.2.6 that the only happy case involves setting the failsafe resolution, and optionally sending the checksum. (sorry for not catching this earlier... I just didn't spot the problem) Correct. Writing the bad checksum is optional, so I opted not to. Less code, less AUX transactions. + } else { + ret = drm_dp_dpcd_write(intel_dp-aux, + DP_TEST_EDID_CHECKSUM, + intel_dp-compliance_edid_checksum, 1); + if (ret = 0) + DRM_DEBUG_DRIVER(Failed to write EDID checksum\n); + else + DRM_DEBUG_DRIVER(EDID checksum written to sink\n);
Re: [Intel-gfx] [PATCH 4/5] drm/i915: PSR VLV: Add single frame update.
On Fri, Apr 10, 2015 at 11:15:10AM -0700, Rodrigo Vivi wrote: According to spec: In PSR HW or SW mode, SW set this bit before writing registers for a flip. It will be self-clear when it gets to the PSR active state. Some versions of spec mention that this is needed when in Persistent mode but define it as same as SW mode. Since this fix the page flip case let's assume this is exactly what we need. Cc: Dhinakaran Pandiyan dhinakaran.pandi...@intel.com Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com Merged up to this one to dinq, will wait with the final enable until the bug Matthew reported is a bit clearer. -Daniel --- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_frontbuffer.c | 2 ++ drivers/gpu/drm/i915/intel_psr.c | 42 3 files changed, 45 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 7a0aa24..9c5d1cd 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1220,6 +1220,7 @@ void intel_psr_invalidate(struct drm_device *dev, void intel_psr_flush(struct drm_device *dev, unsigned frontbuffer_bits); void intel_psr_init(struct drm_device *dev); +void intel_psr_single_frame_update(struct drm_device *dev); /* intel_runtime_pm.c */ int intel_power_domains_init(struct drm_i915_private *); diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c index a20cffb..57095f5 100644 --- a/drivers/gpu/drm/i915/intel_frontbuffer.c +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c @@ -243,6 +243,8 @@ void intel_frontbuffer_flip_prepare(struct drm_device *dev, /* Remove stale busy bits due to the old buffer. */ dev_priv-fb_tracking.busy_bits = ~frontbuffer_bits; mutex_unlock(dev_priv-fb_tracking.lock); + + intel_psr_single_frame_update(dev); } /** diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 5cd374b..5ee0fa5 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -594,6 +594,48 @@ static void intel_psr_exit(struct drm_device *dev) } /** + * intel_psr_single_frame_update - Single Frame Update + * @dev: DRM device + * + * Some platforms support a single frame update feature that is used to + * send and update only one frame on Remote Frame Buffer. + * So far it is only implemented for Valleyview and Cherryview because + * hardware requires this to be done before a page flip. + */ +void intel_psr_single_frame_update(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev-dev_private; + struct drm_crtc *crtc; + enum pipe pipe; + u32 val; + + /* + * Single frame update is already supported on BDW+ but it requires + * many W/A and it isn't really needed. + */ + if (!IS_VALLEYVIEW(dev)) + return; + + mutex_lock(dev_priv-psr.lock); + if (!dev_priv-psr.enabled) { + mutex_unlock(dev_priv-psr.lock); + return; + } + + crtc = dp_to_dig_port(dev_priv-psr.enabled)-base.base.crtc; + pipe = to_intel_crtc(crtc)-pipe; + val = I915_READ(VLV_PSRCTL(pipe)); + + /* + * We need to set this bit before writing registers for a flip. + * This bit will be self-clear when it gets to the PSR active state. + */ + I915_WRITE(VLV_PSRCTL(pipe), val | VLV_EDP_PSR_SINGLE_FRAME_UPDATE); + + mutex_unlock(dev_priv-psr.lock); +} + +/** * intel_psr_invalidate - Invalidade PSR * @dev: DRM device * @frontbuffer_bits: frontbuffer plane tracking bits -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 61/70] drm/i915: Make fb_tracking.lock a spinlock
On Tue, Apr 14, 2015 at 03:52:09PM +0100, Tvrtko Ursulin wrote: /* Delay flushing when rings are still busy.*/ -mutex_lock(dev_priv-fb_tracking.lock); +spin_lock(dev_priv-fb_tracking.lock); frontbuffer_bits = ~dev_priv-fb_tracking.busy_bits; -mutex_unlock(dev_priv-fb_tracking.lock); +spin_unlock(dev_priv-fb_tracking.lock); Looks like you could just remove the lock here in process. ...as in we are always protected by struct_mutex? I think Daniel was planning for a future where that was guaranteed. Anyway my v2 patch does: void __intel_fb_obj_invalidate(struct drm_i915_gem_object *obj, struct intel_engine_cs *ring, enum fb_op_origin origin); static inline void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj, struct intel_engine_cs *ring, enum fb_op_origin origin) { if (!obj-frontbuffer_bits || !obj-pin_display) return; __intel_fb_obj_invalidate(obj, ring, origin); } As the function call overhead itself was annoying me in the execbuffer profiles. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/17] drm/i915: Drop redundant GGTT rebinding
On Tue, Apr 14, 2015 at 05:35:17PM +0200, Daniel Vetter wrote: Since commit bf3d149b25f67f241735b91a56b7f070bc0a5407 Author: Daniel Vetter daniel.vet...@ffwll.ch Date: Fri Feb 14 14:01:12 2014 +0100 drm/i915: split PIN_GLOBAL out from PIN_MAPPABLE i915_gem_obj_ggtt_pin always binds into the ggtt, but I've forgotten to remove the now redundant additional bind call later on. Fix this up. Signed-off-by: Daniel Vetter daniel.vet...@intel.com Reviewed-by: Chris Wilson ch...@chris-wilson.co.uk -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space
On Tue, Apr 14, 2015 at 05:35:12PM +0200, Daniel Vetter wrote: They change with the address space and not with each vma, so move them into the right pile of vfuncs. Save 2 pointers per vma and clarifies the code. Using per-vma vfunc allows you make, for example, the pagetables themselves an ordinary vma in the GGTT and so operate identically wrt to the shrinker and eviction logic - removing some very fragile code in the process. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/17] drm/i915: Move gen8 clear_range vfunc setup into common code
Signed-off-by: Daniel Vetter daniel.vet...@intel.com --- drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 9041f3dfdfb4..1c8ef7c143aa 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -994,6 +994,7 @@ static int gen8_ppgtt_init_common(struct i915_hw_ppgtt *ppgtt, uint64_t size) ppgtt-base.total = size; ppgtt-base.cleanup = gen8_ppgtt_cleanup; ppgtt-base.insert_entries = gen8_ppgtt_insert_entries; + ppgtt-base.clear_range = gen8_ppgtt_clear_range; ppgtt-switch_mm = gen8_mm_switch; @@ -1022,7 +1023,6 @@ static int gen8_aliasing_ppgtt_init(struct i915_hw_ppgtt *ppgtt) } ppgtt-base.allocate_va_range = NULL; - ppgtt-base.clear_range = gen8_ppgtt_clear_range; ppgtt-base.clear_range(ppgtt-base, 0, ppgtt-base.total, true); return 0; @@ -1037,7 +1037,6 @@ static int gen8_ppgtt_init(struct i915_hw_ppgtt *ppgtt) return ret; ppgtt-base.allocate_va_range = gen8_alloc_va_range; - ppgtt-base.clear_range = gen8_ppgtt_clear_range; return 0; } -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] intel: Defer setting madv on the bo cache
Convert the bo-cache into 2 phases: 1. A two second cache of recently used buffers, keep untouched. 2. A two second cache of older buffers, marked for eviction. This helps reduce ioctl traffic on a rapid turnover in working sets. The issue is that even a 2 second window is enough for an application to fill all of memory with inactive buffers (and we would rely on the oom-killer identifying the right culprit). Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Daniel Vetter daniel.vet...@ffwll.ch Cc: Ben Widawsky benjamin.widaw...@intel.com --- intel/intel_bufmgr_gem.c | 42 +++--- 1 file changed, 35 insertions(+), 7 deletions(-) diff --git a/intel/intel_bufmgr_gem.c b/intel/intel_bufmgr_gem.c index f5d6130..ecbf8ee 100644 --- a/intel/intel_bufmgr_gem.c +++ b/intel/intel_bufmgr_gem.c @@ -211,6 +211,11 @@ struct _drm_intel_bo_gem { bool used_as_reloc_target; /** +* Are we keeping this buffer around in semi-permenant cache? +*/ + bool dontneed; + + /** * Boolean of whether we have encountered an error whilst building the relocation tree. */ bool has_error; @@ -714,7 +719,8 @@ retry: } if (alloc_from_cache) { - if (!drm_intel_gem_bo_madvise_internal + if (bo_gem-dontneed + !drm_intel_gem_bo_madvise_internal (bufmgr_gem, bo_gem, I915_MADV_WILLNEED)) { drm_intel_gem_bo_free(bo_gem-bo); drm_intel_gem_bo_cache_purge_bucket(bufmgr_gem, @@ -1150,6 +1156,20 @@ drm_intel_gem_bo_mark_mmaps_incoherent(drm_intel_bo *bo) #endif } +static inline void __splice(const drmMMListHead *list, + drmMMListHead *prev, + drmMMListHead *next) +{ + drmMMListHead *first = list-next; + drmMMListHead *last = list-prev; + + first-prev = prev; + prev-next = first; + + last-next = next; + next-prev = last; +} + /** Frees all cached buffers significantly older than @time. */ static void drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time) @@ -1162,19 +1182,29 @@ drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time) for (i = 0; i bufmgr_gem-num_buckets; i++) { struct drm_intel_gem_bo_bucket *bucket = bufmgr_gem-cache_bucket[i]; + drmMMListHead madv; + DRMINITLISTHEAD(madv); while (!DRMLISTEMPTY(bucket-head)) { drm_intel_bo_gem *bo_gem; bo_gem = DRMLISTENTRY(drm_intel_bo_gem, bucket-head.next, head); - if (time - bo_gem-free_time = 1) + if (time - bo_gem-free_time 2*(1+bo_gem-dontneed)) break; DRMLISTDEL(bo_gem-head); - - drm_intel_gem_bo_free(bo_gem-bo); + if (bo_gem-dontneed) { + drm_intel_gem_bo_free(bo_gem-bo); + } else { + drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem, + I915_MADV_DONTNEED); + bo_gem-dontneed = 1; + DRMLISTADDTAIL(bo_gem-head, madv); + } } + if (!DRMLISTEMPTY(madv)) + __splice(madv, bucket-head, bucket-head.next); } bufmgr_gem-time = time; @@ -1285,9 +1315,7 @@ drm_intel_gem_bo_unreference_final(drm_intel_bo *bo, time_t time) bucket = drm_intel_gem_bo_bucket_for_size(bufmgr_gem, bo-size); /* Put the buffer into our internal cache for reuse if we can. */ - if (bufmgr_gem-bo_reuse bo_gem-reusable bucket != NULL - drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem, - I915_MADV_DONTNEED)) { + if (bufmgr_gem-bo_reuse bo_gem-reusable bucket != NULL) { bo_gem-free_time = time; bo_gem-name = NULL; -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 61/70] drm/i915: Make fb_tracking.lock a spinlock
On 04/14/2015 04:05 PM, Chris Wilson wrote: On Tue, Apr 14, 2015 at 03:52:09PM +0100, Tvrtko Ursulin wrote: /* Delay flushing when rings are still busy.*/ - mutex_lock(dev_priv-fb_tracking.lock); + spin_lock(dev_priv-fb_tracking.lock); frontbuffer_bits = ~dev_priv-fb_tracking.busy_bits; - mutex_unlock(dev_priv-fb_tracking.lock); + spin_unlock(dev_priv-fb_tracking.lock); Looks like you could just remove the lock here in process. ...as in we are always protected by struct_mutex? I think Daniel was planning for a future where that was guaranteed. No, it always looks to be updated with a single write - so I don't see why have a lock for this read? Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 13/17] drm/i915: move i915_gem_restore_gtt_mappings around
Avoids 2 forward declarations. Signed-off-by: Daniel Vetter daniel.vet...@ffwll.ch --- drivers/gpu/drm/i915/i915_gem_gtt.c | 109 ++-- 1 file changed, 53 insertions(+), 56 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 44827090a13f..00c4a336733f 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -97,9 +97,6 @@ const struct i915_ggtt_view i915_ggtt_view_rotated = { .type = I915_GGTT_VIEW_ROTATED }; -static void bdw_setup_private_ppat(struct drm_i915_private *dev_priv); -static void chv_setup_private_ppat(struct drm_i915_private *dev_priv); - static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) { bool has_aliasing_ppgtt; @@ -1727,59 +1724,6 @@ void i915_gem_suspend_gtt_mappings(struct drm_device *dev) i915_ggtt_flush(dev_priv); } -void i915_gem_restore_gtt_mappings(struct drm_device *dev) -{ - struct drm_i915_private *dev_priv = dev-dev_private; - struct drm_i915_gem_object *obj; - struct i915_address_space *vm; - - i915_check_and_clear_faults(dev); - - /* First fill our portion of the GTT with scratch pages */ - dev_priv-gtt.base.clear_range(dev_priv-gtt.base, - dev_priv-gtt.base.start, - dev_priv-gtt.base.total, - true); - - list_for_each_entry(obj, dev_priv-mm.bound_list, global_list) { - struct i915_vma *vma = i915_gem_obj_to_vma(obj, - dev_priv-gtt.base); - if (!vma) - continue; - - i915_gem_clflush_object(obj, obj-pin_display); - WARN_ON(i915_vma_bind(vma, obj-cache_level, PIN_UPDATE)); - } - - - if (INTEL_INFO(dev)-gen = 8) { - if (IS_CHERRYVIEW(dev) || IS_BROXTON(dev)) - chv_setup_private_ppat(dev_priv); - else - bdw_setup_private_ppat(dev_priv); - - return; - } - - if (USES_PPGTT(dev)) { - list_for_each_entry(vm, dev_priv-vm_list, global_link) { - /* TODO: Perhaps it shouldn't be gen6 specific */ - - struct i915_hw_ppgtt *ppgtt = - container_of(vm, struct i915_hw_ppgtt, -base); - - if (i915_is_ggtt(vm)) - ppgtt = dev_priv-mm.aliasing_ppgtt; - - gen6_write_page_range(dev_priv, ppgtt-pd, - 0, ppgtt-base.total); - } - } - - i915_ggtt_flush(dev_priv); -} - int i915_gem_gtt_prepare_object(struct drm_i915_gem_object *obj) { if (obj-has_dma_mapping) @@ -2603,6 +2547,59 @@ int i915_gem_gtt_init(struct drm_device *dev) return 0; } +void i915_gem_restore_gtt_mappings(struct drm_device *dev) +{ + struct drm_i915_private *dev_priv = dev-dev_private; + struct drm_i915_gem_object *obj; + struct i915_address_space *vm; + + i915_check_and_clear_faults(dev); + + /* First fill our portion of the GTT with scratch pages */ + dev_priv-gtt.base.clear_range(dev_priv-gtt.base, + dev_priv-gtt.base.start, + dev_priv-gtt.base.total, + true); + + list_for_each_entry(obj, dev_priv-mm.bound_list, global_list) { + struct i915_vma *vma = i915_gem_obj_to_vma(obj, + dev_priv-gtt.base); + if (!vma) + continue; + + i915_gem_clflush_object(obj, obj-pin_display); + WARN_ON(i915_vma_bind(vma, obj-cache_level, PIN_UPDATE)); + } + + + if (INTEL_INFO(dev)-gen = 8) { + if (IS_CHERRYVIEW(dev) || IS_BROXTON(dev)) + chv_setup_private_ppat(dev_priv); + else + bdw_setup_private_ppat(dev_priv); + + return; + } + + if (USES_PPGTT(dev)) { + list_for_each_entry(vm, dev_priv-vm_list, global_link) { + /* TODO: Perhaps it shouldn't be gen6 specific */ + + struct i915_hw_ppgtt *ppgtt = + container_of(vm, struct i915_hw_ppgtt, +base); + + if (i915_is_ggtt(vm)) + ppgtt = dev_priv-mm.aliasing_ppgtt; + + gen6_write_page_range(dev_priv, ppgtt-pd, + 0, ppgtt-base.total); + } + } + +
[Intel-gfx] [PATCH 17/17] drm/i915: Move i915_get_ggtt_vma_pages into ggtt_bind_vma
We have this neat abstraction between ppgtt and ggtt for (un)bind_vma and didn't end up using it really. What a shame, so fix this and make the -bind_vma hook a bit more useful. Signed-off-by: Daniel Vetter daniel.vet...@intel.com --- drivers/gpu/drm/i915/i915_gem_gtt.c | 41 ++--- drivers/gpu/drm/i915/i915_gem_gtt.h | 6 +++--- 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 458819b99a0b..763c17dd2118 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -92,6 +92,9 @@ * */ +static int +i915_get_ggtt_vma_pages(struct i915_vma *vma); + const struct i915_ggtt_view i915_ggtt_view_normal; const struct i915_ggtt_view i915_ggtt_view_rotated = { .type = I915_GGTT_VIEW_ROTATED @@ -143,9 +146,9 @@ static int sanitize_enable_ppgtt(struct drm_device *dev, int enable_ppgtt) return has_aliasing_ppgtt ? 1 : 0; } -static void ppgtt_bind_vma(struct i915_vma *vma, - enum i915_cache_level cache_level, - u32 unused) +static int ppgtt_bind_vma(struct i915_vma *vma, + enum i915_cache_level cache_level, + u32 unused) { u32 pte_flags = 0; @@ -155,6 +158,8 @@ static void ppgtt_bind_vma(struct i915_vma *vma, vma-vm-insert_entries(vma-vm, vma-obj-pages, vma-node.start, cache_level, pte_flags); + + return 0; } static void ppgtt_unbind_vma(struct i915_vma *vma) @@ -1898,22 +1903,26 @@ static void i915_ggtt_clear_range(struct i915_address_space *vm, intel_gtt_clear_range(first_entry, num_entries); } -static void ggtt_bind_vma(struct i915_vma *vma, - enum i915_cache_level cache_level, - u32 flags) +static int ggtt_bind_vma(struct i915_vma *vma, +enum i915_cache_level cache_level, +u32 flags) { struct drm_device *dev = vma-vm-dev; struct drm_i915_private *dev_priv = dev-dev_private; struct drm_i915_gem_object *obj = vma-obj; struct sg_table *pages = obj-pages; u32 pte_flags = 0; + int ret; + + ret = i915_get_ggtt_vma_pages(vma); + if (ret) + return ret; + pages = vma-ggtt_view.pages; /* Currently applicable only to VLV */ if (obj-gt_ro) pte_flags |= PTE_READ_ONLY; - if (i915_is_ggtt(vma-vm)) - pages = vma-ggtt_view.pages; if (!dev_priv-mm.aliasing_ppgtt || flags GLOBAL_BIND) { vma-vm-insert_entries(vma-vm, pages, @@ -1929,6 +1938,8 @@ static void ggtt_bind_vma(struct i915_vma *vma, vma-node.start, cache_level, pte_flags); } + + return 0; } static void ggtt_unbind_vma(struct i915_vma *vma) @@ -2749,7 +2760,7 @@ err_st_alloc: return ERR_PTR(ret); } -static inline int +static int i915_get_ggtt_vma_pages(struct i915_vma *vma) { int ret = 0; @@ -2793,8 +2804,8 @@ i915_get_ggtt_vma_pages(struct i915_vma *vma) int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, u32 flags) { + int ret = 0; u32 bind_flags = 0; - int ret; if (vma-vm-allocate_va_range) { trace_i915_va_alloc(vma-vm, vma-node.start, @@ -2808,12 +2819,6 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, return ret; } - if (i915_is_ggtt(vma-vm)) { - ret = i915_get_ggtt_vma_pages(vma); - if (ret) - return 0; - } - if (flags PIN_GLOBAL) bind_flags |= GLOBAL_BIND; if (flags PIN_EXECBUF) @@ -2825,7 +2830,9 @@ int i915_vma_bind(struct i915_vma *vma, enum i915_cache_level cache_level, bind_flags = ~vma-bound; if (bind_flags) - vma-vm-bind_vma(vma, cache_level, bind_flags); + ret = vma-vm-bind_vma(vma, cache_level, bind_flags); + if (ret) + return ret; vma-bound |= bind_flags; diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index fb0a04aa5363..4e6cac575cd8 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -278,9 +278,9 @@ struct i915_address_space { * setting the valid PTE entries to a reserved scratch page. */ void (*unbind_vma)(struct i915_vma *vma); /* Map an object into an address space with the given cache flags. */ - void (*bind_vma)(struct i915_vma *vma, -enum i915_cache_level cache_level, -u32 flags); + int (*bind_vma)(struct i915_vma
[Intel-gfx] [PATCH 12/17] drm/i915: Arm cmd parser with aliasng ppgtt only
With the binding regression from the original full ppgtt patches fixed we can throw the switch. Yay! Signed-off-by: Daniel Vetter daniel.vet...@intel.com --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 6 +- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index f005f3151147..819f2b2317ff 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -1557,12 +1557,8 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, * dispatch_execbuffer implementations. We specifically * don't want that set when the command parser is * enabled. -* -* FIXME: with aliasing ppgtt, buffers that should only -* be in ggtt still end up in the aliasing ppgtt. remove -* this check when that is fixed. */ - if (USES_FULL_PPGTT(dev)) + if (USES_PPGTT(dev)) dispatch_flags |= I915_DISPATCH_SECURE; exec_start = 0; -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [i915] assert_device_not_suspended
I forgot to mention that this is linux 4.0.0 Samuel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Remove illogical/bogus Automatic mode from Broadcast RGB property
Are you sure it can only handle limited range in CEA modes and only full range in non CEA modes? Or is it the fact that it doesn't handle full range at all and its (or your) prefer mode happens to be a CEA mode? Or the whole series of this model only supports full range and the model you got happen to have a non CEA prefer mode, while the other model in the same series with CEA prefer mode would suffer anyway? Yes I can't tell how every vendor made their hardware, but it's silly for them to follow this spec and it's even sillier for you to make a driver specifically for those vendors. Because for example, if a user want to buy a monitor for photo editing, he must carefully avoid the ones with a prefer cea mode (or DP/HDMI ports). If one day cea declare that every mode belongs to them, the user will have nowhere to go. For a device with HDMI port, it might SEEM to work anyway because of convention. But for one with only DP (and DVI/VGA), which means it's literally a pure PC monitor, I wonder how many of them have ANY support to limited range at all. (Neither should they, think about what a PC monitor is targeted at.) The EDID does have something can imply whether a device supports limited range, which is its capabilities of YCbCr color space(s), which are inheritly of limited range. P.S. In the case of NVIDIA blob, they locks the output range to limited if it reads the HDMI vendor number in the EDID (while a switch is available for VDPAU only which acts in a weird reverse way), while keeping DP untouched. This is not a very good reference but it seems necessary for the YCbCr output in their implementation, yet even this makes more sense to the mode/range mechanism. On 14 April 2015 at 16:38, Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Tue, Apr 14, 2015 at 02:22:53PM +0800, Tom Yan wrote: It's not about whether it follows every line of the spec, but whether it makes sense to follow. But I'm not going to argue about this anymore, I've written enough to show what's the logical error, yet seems I am the only one in the world who sees a problem in switching color range according to resolution and refresh rate, which I don't see any hardware vendor would/should follow either. So if you don't see a problem here, do whatever you like. You assume no one follows the spec, but all I have to do is look at the TV next to me and see that you're wrong. It's extremely unfortunate that displays can apparently get the appropriate logo without following the spec. I can only conclude that whatever conformance testing is done is insufficient. One option I was thinking would be to find some other hint in the EDID that could tell us whether the device is likely to be spec compliant, but sadly I've not found anything that would appear to work. On 13 April 2015 at 22:22, Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Mon, Apr 13, 2015 at 03:13:12PM +0100, Damien Lespiau wrote: On Wed, Apr 08, 2015 at 07:18:06PM +0800, Tom Yan wrote: https://bugzilla.kernel.org/show_bug.cgi?id=94921 As mentioned in the above bug report, switching output color range Automatically according to current mode does not make sense in computer use case. Current code seems correct to me after re-reading CEA-861-E again. However can we do better? Maybe! From the spec: The QS (AVI Q support) bit of byte 3 allows a display to declare that it supports the reception of either type of quantization range for any video format, under the direction of InfoFrame Q data (see Section 6.4 for information concerning bits Q1 and Q0). This allows a source to override the default quantization range for any video format. If the sink declares a selectable RGB Quantization Range (QS=1) then it shall expect limited range pixel values if it receives Q=1 and it shall expect full range pixel values if it receives Q=2 (see section 6.4). For other values of Q, the sink shall expect pixel values with the default range for the transmitted video format. So, for sinks that support it, we could default to sending the full range picture and overriding the quantization bit in the AVI infoframe. You could you try to run edid-decode [1] on your sink EDID to check if it supports overriding the quantization level (I added decoding the VCDB a while back). Ville, what do you think? Sure, if you can actually find a display that supports the Q bit. I've not seen one yet :( They should have just made it mandatory, otherwise I fear it's never going to catch on. -- Ville Syrjälä Intel OTC -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] Remove illogical/bogus Automatic mode from Broadcast RGB property
I feel silly that I write another mail about this, but I really want who ever read this mail list would have a chance to at least think about why does the CEA modes-limited range mechanism ever exist and where should it (not) apply. For those who feel thrown away from his comfort zone when told a spec is non-sense. (Yes, it probably make sense SOMEwhere) First, what are CEA modes? Why are certain modes considered special by the Consumer Electronic Association? Compare the two lists and you should understand: http://lxr.free-electrons.com/source/drivers/gpu/drm/drm_edid.c#L606 http://en.wikipedia.org/wiki/Blu-ray_Disc#Video Yes, these modes are standard in video discs like Blu-ray or DVD (or TV broadcasting). Okay, so why the spec said these mode should be use with limited color range? There's another reason I compare the mode list with the table in the wiki's section of Blu-ray Video -- because video discs / files are encoded in limited color range. (Google the reason yourself :P) Then why didn't the spec require limited color range to be used for every possible modes exists in the world? A possbile reason is, they want to make sure av products (i think tv, a sink, might be the only possible case) have a rule to follow if the vendor want it to support both full and limited range but without introducing a setting that requires the user to understand this whole thing. That might be the reason why someone claim that their device follow this rule. Yes it might be true, because the vendors (of a TV) might want to give their user best user experience by keeping their user ignorant, at the same time stopping user who need full range with CEA modes to pick it himself. (Time to think: should we follow these vendors then?) So even if the hardware support both range, there is no option to change manually. But the thing is this rule PRIORITIZE video disc/file playback (assume it is the case whenever the source is in CEA modes, hence use limited color range), and use full range for other case (because limited range is rarely used/necessary/appropriate for other cases). So what's the problem if we write a graphic card driver following this rule for HDMI and DP? By following this rule, it means the graphic card (pc) is PRIORITZED to video disc/file playback, while leaving other jobs as second-class citizens. Remember, your PC is not a Blu-ray Player even if you mainly use it for video playback, not everyone use it like this. Worsestill, video playback software might not actually deal with limited range output appropriately because they might have assume PC monitor supports full range only. So even if your monitor or TV support it, you might still experience a Limited(Source File)-Full(Interpolated by the player)-Limited(Compressed again by the gfx driver) conversion. For the case of HDMI, at least for the TVs, following the rule might not cause a problem because they are mostly expected to deal with source like Blu-ray/DVD players, which is totally acceptable if they could only output limited range. The only weakness would be making you to suffer the lossy-compressed limited color range. But if some HDMI PC monitors does not assume it should be used with source the those players, they might fail to support limited range. Yet I wouldn't call this a problem of them because if it's made as a PC monitor, it's fine that they support only full range, which is used for general purpose. The worst case is to follow the rule with DisplayPort. Sorry I can't help this time. I can only say that VESA is stupid, or actually greedy to adopt this rule. The only reason I can think of for them to follow this rule is, at the beginning (or still) they think that DisplayPort is gonna replace HDMI and be the only connector in the PC and AV worlds, so they try to have as many as possible the same basic properties as HDMI, so that not to upset any AV product vendors. But the thing is, nobody shows VESA a damn about this. If you see a Blu-ray Player, or even a TV with DisplayPort, don't forget to take a picture (and send me please I beg you). And in the PC world, no body wants limited range when you do photo editing, gaming or even web browsing and coding. Why? It's simple substraction. So in the end, with the help of some graphic card vendors, you and I are successfully pissed by VESA's greed if we got a DP monitor which happen have a CEA prefer mode. On 14 April 2015 at 19:35, Tom Yan tom.t...@gmail.com wrote: By the way it's extremely FORTUNATE that many vendors does not follow this silly rule from CEA, because doing so means nothing else than letting CEA to make certains mode proprietary. It is of course ideal to make devices support both range. But when the cost is considered, the only sane compromise would be picking one which is suitable for the targeted area (av playback only/general pc usage), not to follow that stupid rule like making this support limited range only:
Re: [Intel-gfx] [PATCH] Remove illogical/bogus Automatic mode from Broadcast RGB property
By the way it's extremely FORTUNATE that many vendors does not follow this silly rule from CEA, because doing so means nothing else than letting CEA to make certains mode proprietary. It is of course ideal to make devices support both range. But when the cost is considered, the only sane compromise would be picking one which is suitable for the targeted area (av playback only/general pc usage), not to follow that stupid rule like making this support limited range only: http://www.eizoglobal.com/products/flexscan/ev2336w/index.html and this support full range only: http://www.eizoglobal.com/products/flexscan/ev2436w/index.html while both is obviously PC monitors and belongs to the same series. (Luckily in reality eizo DIDN'T follow that rule) On 14 April 2015 at 19:18, Tom Yan tom.t...@gmail.com wrote: Are you sure it can only handle limited range in CEA modes and only full range in non CEA modes? Or is it the fact that it doesn't handle full range at all and its (or your) prefer mode happens to be a CEA mode? Or the whole series of this model only supports full range and the model you got happen to have a non CEA prefer mode, while the other model in the same series with CEA prefer mode would suffer anyway? Yes I can't tell how every vendor made their hardware, but it's silly for them to follow this spec and it's even sillier for you to make a driver specifically for those vendors. Because for example, if a user want to buy a monitor for photo editing, he must carefully avoid the ones with a prefer cea mode (or DP/HDMI ports). If one day cea declare that every mode belongs to them, the user will have nowhere to go. For a device with HDMI port, it might SEEM to work anyway because of convention. But for one with only DP (and DVI/VGA), which means it's literally a pure PC monitor, I wonder how many of them have ANY support to limited range at all. (Neither should they, think about what a PC monitor is targeted at.) The EDID does have something can imply whether a device supports limited range, which is its capabilities of YCbCr color space(s), which are inheritly of limited range. P.S. In the case of NVIDIA blob, they locks the output range to limited if it reads the HDMI vendor number in the EDID (while a switch is available for VDPAU only which acts in a weird reverse way), while keeping DP untouched. This is not a very good reference but it seems necessary for the YCbCr output in their implementation, yet even this makes more sense to the mode/range mechanism. On 14 April 2015 at 16:38, Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Tue, Apr 14, 2015 at 02:22:53PM +0800, Tom Yan wrote: It's not about whether it follows every line of the spec, but whether it makes sense to follow. But I'm not going to argue about this anymore, I've written enough to show what's the logical error, yet seems I am the only one in the world who sees a problem in switching color range according to resolution and refresh rate, which I don't see any hardware vendor would/should follow either. So if you don't see a problem here, do whatever you like. You assume no one follows the spec, but all I have to do is look at the TV next to me and see that you're wrong. It's extremely unfortunate that displays can apparently get the appropriate logo without following the spec. I can only conclude that whatever conformance testing is done is insufficient. One option I was thinking would be to find some other hint in the EDID that could tell us whether the device is likely to be spec compliant, but sadly I've not found anything that would appear to work. On 13 April 2015 at 22:22, Ville Syrjälä ville.syrj...@linux.intel.com wrote: On Mon, Apr 13, 2015 at 03:13:12PM +0100, Damien Lespiau wrote: On Wed, Apr 08, 2015 at 07:18:06PM +0800, Tom Yan wrote: https://bugzilla.kernel.org/show_bug.cgi?id=94921 As mentioned in the above bug report, switching output color range Automatically according to current mode does not make sense in computer use case. Current code seems correct to me after re-reading CEA-861-E again. However can we do better? Maybe! From the spec: The QS (AVI Q support) bit of byte 3 allows a display to declare that it supports the reception of either type of quantization range for any video format, under the direction of InfoFrame Q data (see Section 6.4 for information concerning bits Q1 and Q0). This allows a source to override the default quantization range for any video format. If the sink declares a selectable RGB Quantization Range (QS=1) then it shall expect limited range pixel values if it receives Q=1 and it shall expect full range pixel values if it receives Q=2 (see section 6.4). For other values of Q, the sink shall expect pixel values with the default range for the transmitted video format. So, for sinks that support it, we could default to sending the full range picture and
[Intel-gfx] [i915] assert_device_not_suspended
Hello, Yesterday, as usual I ran xlock before the night. In the morning, I typed a key to wake it up, it didn't. I had to reboot. I could however read in the logs: Apr 14 05:00:13 type kernel: [33976.116204] WARNING: CPU: 2 PID: 63 at drivers/gpu/drm/i915/intel_uncore.c:69 assert_device_not_suspended+0x45/0x4e [i915]() Apr 14 05:00:13 type kernel: [33976.116206] Device suspended Apr 14 05:00:13 type kernel: [33976.116206] Modules linked in: tun xt_REDIRECT nf_nat_redirect xt_tcpudp ipt_MASQUERADE nf_nat_masquerade_ipv4 iptable_nat nf_conntrack_ipv4 nf_defrag_ipv4 nf_nat_ipv4 nf_nat nf_conntrack ip_tables x_tables binfmt_misc nfsd cpufreq_powersave auth_rpcgss cpufreq_stats oid_registry nfs_acl cpufreq_userspace lockd cpufreq_conservative bbswitch(O) grace sunrpc joydev hid_generic usbhid hid cdc_mbim cdc_ncm usbnet mii cdc_acm cdc_wdm arc4 brcmsmac cordic brcmutil b43 nls_utf8 nls_cp437 mac80211 vfat fat cfg80211 x86_pkg_temp_thermal i915 kvm_intel kvm snd_hda_codec_hdmi ssb rng_core pcmcia pcmcia_core crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel aes_x86_64 dell_wmi lrw sparse_keymap gf128mul drm_kms_helper dell_laptop glue_helper rfkill ablk_helper cryptd drm psmouse dcdbas evdev serio_raw bcma i2c_i801 acpi_cpufreq tpm_tis wmi i2c_algo_bit tpm i2c_core 8250_fintek video ac battery processor button snd_hda_codec_idt snd_hda_codec_generic ledtrig_timer ledtrig_oneshot ledtrig_heartbeat ledtrig_default_on snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep snd_pcm_oss snd_mixer_oss snd_pcm snd_timer snd soundcore coretemp ohci_hcd ledtrig_backlight pcspkr i8k firewire_sbp2 firewire_core crc_itu_t loop fuse parport_pc ppdev lp parport autofs4 microcode crc32c_intel ehci_pci ehci_hcd sdhci_pci sr_mod sdhci cdrom sg mmc_core usbcore e1000e ptp usb_common pps_core thermal thermal_sys Apr 14 05:00:13 type kernel: [33976.116266] CPU: 2 PID: 63 Comm: kswapd0 Tainted: G O4.0.0 #87 Apr 14 05:00:13 type kernel: [33976.116267] Hardware name: Dell Inc. Latitude E6420/ , BIOS A14 07/11/2012 Apr 14 05:00:13 type kernel: [33976.116268] 0009 8144544b 8800b1313a88 Apr 14 05:00:13 type kernel: [33976.116270] 8104a9bc 0002 a0848b37 ea00026dffa0 Apr 14 05:00:13 type kernel: [33976.116272] 8801388b 00100028 8801388b0068 0010002c Apr 14 05:00:13 type kernel: [33976.116274] Call Trace: Apr 14 05:00:13 type kernel: [33976.116280] [8144544b] ? dump_stack+0x40/0x50 Apr 14 05:00:13 type kernel: [33976.116284] [8104a9bc] ? warn_slowpath_common+0x98/0xb0 Apr 14 05:00:13 type kernel: [33976.116295] [a0848b37] ? assert_device_not_suspended+0x45/0x4e [i915] Apr 14 05:00:13 type kernel: [33976.116297] [8104aa19] ? warn_slowpath_fmt+0x45/0x4a Apr 14 05:00:13 type kernel: [33976.116308] [a0848b37] ? assert_device_not_suspended+0x45/0x4e [i915] Apr 14 05:00:13 type kernel: [33976.116317] [a0849186] ? gen6_write32+0x32/0x83 [i915] Apr 14 05:00:13 type kernel: [33976.116328] [a0830772] ? i915_gem_write_fence+0x270/0x46d [i915] Apr 14 05:00:13 type kernel: [33976.116337] [a08309af] ? i915_gem_object_update_fence+0x40/0xa6 [i915] Apr 14 05:00:13 type kernel: [33976.116346] [a0830ab7] ? i915_gem_object_put_fence+0xa2/0xac [i915] Apr 14 05:00:13 type kernel: [33976.116355] [a0830b65] ? i915_vma_unbind+0xa4/0x1a6 [i915] Apr 14 05:00:13 type kernel: [33976.116364] [a0830e7e] ? i915_gem_shrink+0x11e/0x178 [i915] Apr 14 05:00:13 type kernel: [33976.116373] [a0831597] ? i915_gem_shrinker_scan+0x60/0x82 [i915] Apr 14 05:00:13 type kernel: [33976.116376] [810ef6ad] ? shrink_slab.part.53.constprop.71+0x1a6/0x2ac Apr 14 05:00:13 type kernel: [33976.116379] [810f1a38] ? shrink_zone+0x6b/0x131 Apr 14 05:00:13 type kernel: [33976.116381] [810f26ca] ? kswapd+0x57a/0x740 Apr 14 05:00:13 type kernel: [33976.116383] [810f2150] ? zone_reclaim+0x1cb/0x1cb Apr 14 05:00:13 type kernel: [33976.116386] [81060503] ? kthread+0xab/0xb3 Apr 14 05:00:13 type kernel: [33976.116388] [81060458] ? __kthread_parkme+0x5d/0x5d Apr 14 05:00:13 type kernel: [33976.116389] [81448d58] ? ret_from_fork+0x58/0x90 Apr 14 05:00:13 type kernel: [33976.116391] [81060458] ? __kthread_parkme+0x5d/0x5d Apr 14 05:00:13 type kernel: [33976.116392] ---[ end trace de34697426b2c759 ]--- So it seems it's the reclaiming that triggered an i915 operation while the graphic card was suspended. It's probably worth mentioning that I use on my i915 card: echo auto /sys/bus/pci/devices/:00:02.0/power/control In the following days, I'll be trying the attached patch. Samuel --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -5258,6 +5258,11 @@ i915_gem_shrinker_scan(struct shrinker * if
[Intel-gfx] [PATCH v2] drm/i915: Workaround to avoid lite restore with HEAD==TAIL
WaIdleLiteRestore is an execlists-only workaround, and requires the driver to ensure that any context always has HEAD!=TAIL when attempting lite restore. Add two extra MI_NOOP instructions at the end of each request, but keep the requests tail pointing before the MI_NOOPs. We may not need to executed them, and this is why request-tail must be sampled before adding these extra instructions. If we submit a context to the ELSP which has previously been submitted, move the tail pointer past the MI_NOOPs. This ensures HEAD!=TAIL. v2: Move overallocation to gen8_emit_request, and added note about sampling request-tail in commit message (Chris). Cc: Chris Wilson ch...@chris-wilson.co.uk Signed-off-by: Thomas Daniel thomas.dan...@intel.com Signed-off-by: Michel Thierry michel.thie...@intel.com --- drivers/gpu/drm/i915/i915_gem.c | 15 ++- drivers/gpu/drm/i915/intel_lrc.c | 27 ++- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index 6dd0d57..dc94984 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -2364,14 +2364,27 @@ int __i915_add_request(struct intel_engine_cs *ring, ret = ring-emit_request(ringbuf, request); if (ret) return ret; + + request-tail = intel_ring_get_tail(ringbuf); + + if (IS_GEN8(ring-dev) || IS_GEN9(ring-dev)) { + /* +* Here we add two extra NOOPs as padding to avoid +* lite restore of a context with HEAD==TAIL. +*/ + intel_logical_ring_emit(ringbuf, MI_NOOP); + intel_logical_ring_emit(ringbuf, MI_NOOP); + intel_logical_ring_advance(ringbuf); + } } else { ret = ring-add_request(ring); if (ret) return ret; + + request-tail = intel_ring_get_tail(ringbuf); } request-head = request_start; - request-tail = intel_ring_get_tail(ringbuf); /* Whilst this request exists, batch_obj will be on the * active_list, and so will hold the active reference. Only when this diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 2747b02..1614425 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -427,6 +427,26 @@ static void execlists_context_unqueue(struct intel_engine_cs *ring) } } + if (IS_GEN8(ring-dev) || IS_GEN9(ring-dev)) { + /* +* WaIdleLiteRestore: make sure we never cause a lite +* restore with HEAD==TAIL +*/ + if (req0 req0-elsp_submitted == 1) { + /* +* Consume the buffer NOOPs to ensure HEAD != TAIL when +* submitting. elsp_submitted can only be 1 after +* reset, in which case we don't need the workaround as +* a lite restore will not occur. +*/ + struct intel_ringbuffer *ringbuf; + + ringbuf = req0-ctx-engine[ring-id].ringbuf; + req0-tail += 8; + req0-tail = ringbuf-size - 1; + } + } + WARN_ON(req1 req1-elsp_submitted); execlists_submit_contexts(ring, req0-ctx, req0-tail, @@ -1272,7 +1292,12 @@ static int gen8_emit_request(struct intel_ringbuffer *ringbuf, u32 cmd; int ret; - ret = intel_logical_ring_begin(ringbuf, request-ctx, 6); + /* +* Reserve space for 2 NOOPs at the end of each request to be +* used as a workaround for not being allowed to do lite +* restore with HEAD==TAIL. +*/ + ret = intel_logical_ring_begin(ringbuf, request-ctx, 8); if (ret) return ret; -- 2.1.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] igt/gem_ctx_exec: Add lrc lite restore subtest
Exercise lite-restore (re-submit a context that is currently running), by queueing several small batchbuffers. This test helps to validate WaIdleLiteRestore. Signed-off-by: Michel Thierry michel.thie...@intel.com --- tests/gem_ctx_exec.c | 30 +- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/tests/gem_ctx_exec.c b/tests/gem_ctx_exec.c index 43b38a2..3df939c 100644 --- a/tests/gem_ctx_exec.c +++ b/tests/gem_ctx_exec.c @@ -155,7 +155,7 @@ static void big_exec(int fd, uint32_t handle, int ring) uint32_t handle; uint32_t batch[2] = {0, MI_BATCH_BUFFER_END}; -uint32_t ctx_id; +uint32_t ctx_id, ctx_id2; int fd; igt_main @@ -215,4 +215,32 @@ igt_main gem_context_destroy(fd, ctx_id); } + + igt_subtest(lrc-lite-restore) { + int i, j; + + /* +* Need 2 contexts to be able to replicate a lite restore, +* i.e. a running context is resubmitted. +*/ + ctx_id = gem_context_create(fd); + ctx_id2 = gem_context_create(fd); + + /* +* Queue several small batchbuffers to be sure we'll send execlists +* with 2 valid context, and likely cause a lite restore when ctxB +* is resubmitted at the top of the new execlist. +*/ + for (i = 0; i 20; i++) { + for (j = 0; j 200; j++) { + igt_assert(exec(fd, handle, I915_EXEC_RENDER, ctx_id) == 0); + igt_assert(exec(fd, handle, I915_EXEC_RENDER, ctx_id2) == 0); + } + + gem_sync(fd, handle); + } + + gem_context_destroy(fd, ctx_id); + gem_context_destroy(fd, ctx_id2); + } } -- 2.3.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 61/70] drm/i915: Make fb_tracking.lock a spinlock
On 04/07/2015 05:28 PM, Chris Wilson wrote: We only need a very lightweight mechanism here as the locking is only used for co-ordinating a bitfield. Also double check that the object is still pinned to the display plane before processing the state change. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- drivers/gpu/drm/i915/i915_drv.h | 2 +- drivers/gpu/drm/i915/i915_gem.c | 2 +- drivers/gpu/drm/i915/intel_frontbuffer.c | 40 +--- 3 files changed, 23 insertions(+), 21 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 97372869097f..eeffefa10612 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1545,7 +1545,7 @@ struct intel_pipe_crc { }; struct i915_frontbuffer_tracking { - struct mutex lock; + spinlock_t lock; /* * Tracking bits for delayed frontbuffer flushing du to gpu activity or diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c index e9f2d2b102de..43baac2c1e20 100644 --- a/drivers/gpu/drm/i915/i915_gem.c +++ b/drivers/gpu/drm/i915/i915_gem.c @@ -5260,7 +5260,7 @@ i915_gem_load(struct drm_device *dev) i915_gem_shrinker_init(dev_priv); - mutex_init(dev_priv-fb_tracking.lock); + spin_lock_init(dev_priv-fb_tracking.lock); } void i915_gem_release(struct drm_device *dev, struct drm_file *file) diff --git a/drivers/gpu/drm/i915/intel_frontbuffer.c b/drivers/gpu/drm/i915/intel_frontbuffer.c index a20cffb78c0f..28ce2ab94189 100644 --- a/drivers/gpu/drm/i915/intel_frontbuffer.c +++ b/drivers/gpu/drm/i915/intel_frontbuffer.c @@ -139,16 +139,14 @@ void intel_fb_obj_invalidate(struct drm_i915_gem_object *obj, WARN_ON(!mutex_is_locked(dev-struct_mutex)); - if (!obj-frontbuffer_bits) + if (!obj-frontbuffer_bits || !obj-pin_display) return; if (ring) { - mutex_lock(dev_priv-fb_tracking.lock); - dev_priv-fb_tracking.busy_bits - |= obj-frontbuffer_bits; - dev_priv-fb_tracking.flip_bits - = ~obj-frontbuffer_bits; - mutex_unlock(dev_priv-fb_tracking.lock); + spin_lock(dev_priv-fb_tracking.lock); + dev_priv-fb_tracking.busy_bits |= obj-frontbuffer_bits; + dev_priv-fb_tracking.flip_bits = ~obj-frontbuffer_bits; + spin_unlock(dev_priv-fb_tracking.lock); } intel_mark_fb_busy(dev, obj-frontbuffer_bits, ring); @@ -175,9 +173,12 @@ void intel_frontbuffer_flush(struct drm_device *dev, struct drm_i915_private *dev_priv = dev-dev_private; /* Delay flushing when rings are still busy.*/ - mutex_lock(dev_priv-fb_tracking.lock); + spin_lock(dev_priv-fb_tracking.lock); frontbuffer_bits = ~dev_priv-fb_tracking.busy_bits; - mutex_unlock(dev_priv-fb_tracking.lock); + spin_unlock(dev_priv-fb_tracking.lock); Looks like you could just remove the lock here in process. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/17] drm/i915: Don't look at pg_dirty_rings for aliasing ppgtt
On Tue, Apr 14, 2015 at 05:35:18PM +0200, Daniel Vetter wrote: We load the ppgtt ptes once per gpu reset/driver load/resume and that's all that's needed. Note that this only blows up when we're using the allocate_va_range funcs and not the special-purpose ones used. With this change we can get rid of that duplication. Honestly, I would prefer the test to be rewritten so that the information on which vm was being used was passed along and not that magic sprinkled in the middle of nowhere. e.g. execbuffer knows exactly what vm it bound the objects into, and yet towards the end we decide to guess again. Also, I would rather the execbuffer test be moved to i915_gem_context since it is scattering internal knowledge about. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/17] drm/i915: Move vma vfuns to adddress_space
On Tue, Apr 14, 2015 at 05:09:27PM +0100, Chris Wilson wrote: On Tue, Apr 14, 2015 at 05:35:12PM +0200, Daniel Vetter wrote: They change with the address space and not with each vma, so move them into the right pile of vfuncs. Save 2 pointers per vma and clarifies the code. Using per-vma vfunc allows you make, for example, the pagetables themselves an ordinary vma in the GGTT and so operate identically wrt to the shrinker and eviction logic - removing some very fragile code in the process. A vma-ops would be an interesting compromise. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] tests/kms_psr_sink_crc: Make render visible to human eyes
This will allow manual tests when crc isn't available. v2: Remove unused and non-sense buf-size and decrease buf-stride a bit as suggested by Daniel. v3: Fix v2 mistake and get buf-size back with a value that makes more sense. TBD: to be changed for variable size depending on modified fb size on following patch Cc: Daniel Vetter daniel.vet...@ffwll.ch Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com --- tests/kms_psr_sink_crc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c index 0a56705..94d74ec 100644 --- a/tests/kms_psr_sink_crc.c +++ b/tests/kms_psr_sink_crc.c @@ -139,7 +139,7 @@ static void scratch_buf_init(struct igt_buf *buf, drm_intel_bo *bo) buf-bo = bo; buf-stride = 4096; buf-tiling = I915_TILING_X; - buf-size = 4096; + buf-size = 64 * 4096; } static void fill_render(data_t *data, uint32_t handle, unsigned char color) @@ -155,7 +155,7 @@ static void fill_render(data_t *data, uint32_t handle, unsigned char color) dst = gem_handle_to_libdrm_bo(data-bufmgr, data-drm_fd, , handle); igt_assert(dst); - src = drm_intel_bo_alloc(data-bufmgr, , 4096, 4096); + src = drm_intel_bo_alloc(data-bufmgr, , 64 * 4096, 4096); igt_assert(src); gem_write(data-drm_fd, src-handle, 0, buf, 4); @@ -167,7 +167,7 @@ static void fill_render(data_t *data, uint32_t handle, unsigned char color) igt_assert(batch); rendercopy(batch, NULL, - src_buf, 0, 0, 1, 1, + src_buf, 0, 0, 0xff, 0xff, dst_buf, 0, 0); intel_batchbuffer_free(batch); -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/4] drm/i915: Switch to full atomic helpers for plane updates/disable, take two
-Original Message- From: Intel-gfx [mailto:intel-gfx-boun...@lists.freedesktop.org] On Behalf Of Matt Roper Sent: Thursday, April 09, 2015 11:04 AM To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH 3/4] drm/i915: Switch to full atomic helpers for plane updates/disable, take two On Thu, Apr 09, 2015 at 02:46:19PM +0200, Daniel Vetter wrote: On Wed, Apr 08, 2015 at 06:56:53PM -0700, Matt Roper wrote: Switch from our plane update/disable entrypoints to use the full atomic helpers (which generate a top-level atomic transaction) rather than the transitional helpers (which only create/manipulate orphaned plane states independent of a top-level transaction). Various upcoming work (SKL scalers, atomic watermarks, etc.) requires a full atomic transaction to behave properly/cleanly. Last time we tried this, we had to back out the change because we still call the drm_plane vfuncs directly from within our legacy modesetting code. This potentially results in nested atomic transactions, locking collisions, and other failures. To avoid that problem again, we sidestep the issue by calling the transitional helpers directly (rather than through a vfunc) when we're nested inside of other legacy modesetting code. Matt, I rebased scaler patch 13/14 on top of 90/270 but hit into issues. After much debugging, found below: By keeping transitional helpers for some scenarios as you mentioned, this is leaving intel_plane_state-src/dst rect values with 0s. I recall having a src/dst rects copy in intel_plane_state is to hold modified values due to any clipping etc. And modified values will be used in platform specific update function (i.e., skylake_update_primary_plane()). From these paths, src/dst rects in both drm and intel plane states are same and works by accessing drm_plane_state to make it work, but that isn't true in other cases and should use intel_plane_state src/rects only. Working on a fix but any suggestions/comments welcome... However this does allow legacy SetPlane() ioctl's to process an entire drm_atomic_state transaction, which is important for upcoming patches. Cc: Chandra Konduru chandra.kond...@intel.com Signed-off-by: Matt Roper matthew.d.ro...@intel.com Maarten is working to fix this properly (by wiring up plane states to the transitional drm_atomic_state scaffolding from Ander), but seems like a good interim idea to get back some solid testing for our atomic code. Can I apply this without patches 12 right away or is there a tricky depency? -Daniel I think this one can be applied independently of 12. I think this one might be a prereq for #4 to fully work as advertised though...without using the full atomic helpers, we simply don't have a 'start of transaction' point at which we can clear the existing atomic flags when using legacy plane ioctls. Matt --- drivers/gpu/drm/i915/intel_display.c | 24 drivers/gpu/drm/i915/intel_sprite.c | 10 +- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1cf91ad..3a74923 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5576,7 +5576,7 @@ static void intel_crtc_disable(struct drm_crtc *crtc) dev_priv-display.crtc_disable(crtc); dev_priv-display.off(crtc); - crtc-primary-funcs-disable_plane(crtc-primary); + drm_plane_helper_disable(crtc-primary); /* Update computed state. */ list_for_each_entry(connector, dev-mode_config.connector_list, head) { @@ -11731,11 +11731,11 @@ static int __intel_set_mode(struct drm_crtc *crtc, int vdisplay, hdisplay; drm_crtc_get_hv_timing(mode, hdisplay, vdisplay); - ret = primary-funcs-update_plane(primary, intel_crtc-base, -fb, 0, 0, -hdisplay, vdisplay, -x 16, y 16, -hdisplay 16, vdisplay 16); + ret = drm_plane_helper_update(primary, intel_crtc-base, + fb, 0, 0, + hdisplay, vdisplay, + x 16, y 16, + hdisplay 16, vdisplay 16); } /* Now enable the clocks, plane, pipe, and connectors that we set up. */ @@ -12286,10 +12286,10 @@ static int intel_crtc_set_config(struct drm_mode_set *set) int vdisplay, hdisplay; drm_crtc_get_hv_timing(set-mode, hdisplay, vdisplay); - ret = primary-funcs-update_plane(primary, set-crtc, set-fb, -0, 0,
Re: [Intel-gfx] [PATCH] drm: Defer disabling the vblank IRQ until the next interrupt (for instant-off)
On 04/02/2015 01:34 PM, Chris Wilson wrote: On vblank instant-off systems, we can get into a situation where the cost of enabling and disabling the vblank IRQ around a drmWaitVblank query dominates. However, we know that if the user wants the current vblank counter, they are also very likely to immediately queue a vblank wait and so we can keep the interrupt around and only turn it off if we have no further vblank requests in the interrupt interval. After vblank event delivery there is a shadow of one vblank where the interrupt is kept alive for the user to query and queue another vblank event. Similarly, if the user is using blocking drmWaitVblanks, the interrupt will be disabled on the IRQ following the wait completion. However, if the user is simply querying the current vblank counter and timestamp, the interrupt will be disabled after every IRQ and the user will enabled it again on the first query following the IRQ. Testcase: igt/kms_vblank Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk Cc: Ville Syrjälä ville.syrj...@linux.intel.com Cc: Daniel Vetter dan...@ffwll.ch Cc: Michel Dänzer mic...@daenzer.net Cc: Laurent Pinchart laurent.pinch...@ideasonboard.com Cc: Dave Airlie airl...@redhat.com, Cc: Mario Kleiner mario.kleiner...@gmail.com --- drivers/gpu/drm/drm_irq.c | 15 +-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/drm_irq.c b/drivers/gpu/drm/drm_irq.c index c8a34476570a..6f5dc18779e2 100644 --- a/drivers/gpu/drm/drm_irq.c +++ b/drivers/gpu/drm/drm_irq.c @@ -1091,9 +1091,9 @@ void drm_vblank_put(struct drm_device *dev, int crtc) if (atomic_dec_and_test(vblank-refcount)) { if (drm_vblank_offdelay == 0) return; - else if (dev-vblank_disable_immediate || drm_vblank_offdelay 0) + else if (drm_vblank_offdelay 0) vblank_disable_fn((unsigned long)vblank); - else + else if (!dev-vblank_disable_immediate) mod_timer(vblank-disable_timer, jiffies + ((drm_vblank_offdelay * HZ)/1000)); } @@ -1697,6 +1697,17 @@ bool drm_handle_vblank(struct drm_device *dev, int crtc) spin_lock_irqsave(dev-event_lock, irqflags); You could move the code before the spin_lock_irqsave(dev-event_lock, irqflags); i think it doesn't need that lock? + if (dev-vblank_disable_immediate !atomic_read(vblank-refcount)) { Also check for (drm_vblank_offdelay 0) to make sure we have a way out of instant disable here, and the same meaning of of drm_vblank_offdelay like we have in the current implementation. This hunk ... + unsigned long vbl_lock_irqflags; + + spin_lock_irqsave(dev-vbl_lock, vbl_lock_irqflags); + if (atomic_read(vblank-refcount) == 0 vblank-enabled) { + DRM_DEBUG(disabling vblank on crtc %d\n, crtc); + vblank_disable_and_save(dev, crtc); + } + spin_unlock_irqrestore(dev-vbl_lock, vbl_lock_irqflags); ... is the same as a call to vblank_disable_fn((unsigned long) vblank); Maybe replace by that call? You could also return here already, as the code below will just take a lock, realize vblanks are now disabled and then release the locks and exit. + } + /* Need timestamp lock to prevent concurrent execution with * vblank enable/disable, as this would cause inconsistent * or corrupted timestamps and vblank counts. I think the logic itself is fine and at least basic testing of the patch on a Intel HD Ironlake didn't show problems, so with the above taken into account it would have my slightly uneasy reviewed-by. One thing that worries me a little bit about the disable inside vblank irq are the potential races between the disable code and the display engine which could cause really bad off-by-one errors for clients on a imperfect driver. These races can only happen if vblank enable or disable happens close to or inside the vblank. This approach lets the instant disable happen exactly inside vblank when there is the highest chance of triggering that condition. This doesn't seem to be a problem for intel kms, but other drivers don't have instant disable yet, so we don't know how well we could do it there. Additionally things like dynamic power management tend to operate inside vblank, sometimes with funny side effects to other stuff, e.g., dpm on AMD, as i remember from some long debug session with Michel and Alex last summer where dpm played a role. Therefore it seems more safe to me to avoid actions inside vblank that could be done outside. E.g., instead of doing the disable inside the vblank irq one could maybe just schedule an exact timer to do the disable a few milliseconds later in the middle of active scanout to avoid these potential issues? -mario
Re: [Intel-gfx] [PATCH 07/11] drm/i915: Update intel_dp_hpd_pulse() for non-MST operation
On 4/14/15 12:00 PM, Paulo Zanoni wrote: 2015-04-14 14:36 GMT-03:00 Todd Previte tprev...@gmail.com: On 4/14/15 4:29 AM, Paulo Zanoni wrote: 2015-04-10 13:12 GMT-03:00 Todd Previte tprev...@gmail.com: Update the hot plug function to handle the SST case. Instead of placing the SST case within the long/short pulse block, it is now handled after determining that MST mode is not in use. This way, the topology management layer can handle any MST-related operations while SST operations are still correctly handled afterwards. This patch also corrects the problem of SST mode only being handled in the case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes both short and long pulses are used by the different tests, thus both cases need to be addressed for SST. This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the previous compliance testing patch sequence. Review feedback on that patch indicated that updating intel_dp_hot_plug() was not the correct place for the test handler. For the SST case, the main stream is disabled for long HPD pulses as this generally indicates either a connect/disconnect event or link failure. For a number of case in compliance testing, the source is required to disable the main link upon detection of a long HPD. V2: - N/A V3: - Place the SST mode link status check into the mst_fail case - Remove obsolete comment regarding SST mode operation - Removed an erroneous line of code that snuck in during rebasing V4: - Added a disable of the main stream (DP transport) for the long pulse case for SST to support compliance testing V5: - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 V6: - Reformatted a comment Signed-off-by: Todd Previte tprev...@gmail.com --- drivers/gpu/drm/i915/intel_dp.c | 19 --- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 77b6b15..ba2da44 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4572,29 +4572,26 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) if (intel_dp_check_mst_status(intel_dp) == -EINVAL) goto mst_fail; } - - if (!intel_dp-is_mst) { - /* -* we'll check the link status via the normal hot plug path later - -* but for short hpds we should check it now -*/ - drm_modeset_lock(dev-mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); - drm_modeset_unlock(dev-mode_config.connection_mutex); - } } Shouldn't the code be moved to exactly this spot instead of after the put_power label? Why would we want to call check_link_status in case we goto mst_fail? In case there is a valid reason, maybe it would be better to do a big reorganization of this function because it's going to start looking very weird - or at least rename the labels. No because then you don't get long pulses, only short ones. No, what I mean is: if (long_hpd) { ... code ... } else { ... code } if (!intel_dp-is_mst) { drm_modeset_lock(...) ... code ... } mst_fail: ... code ... The other problem I point is: imagine we're SST and we get a long_hpd. Then we run ibx_digital_port_connected(), and since the monitor is disconnected we goto mst_fail. We'll end up running intel_dp_check_link_status() before returning, but we really shouldn't run it since we know the monitor is disconnected. I see what you're saying, however under normal operation for SST (when connected and everything is working) the code will hit this line: if (!intel_dp_probe_mst(intel_dp)) And proceed to the mst_fail block, thus skipping that block of code entirely and missing the SST handler. The result is a missed long pulse for the SST case. Your second point has validity though. This can be addressed with a connected flag just before the if (long_pulse) statement: connected = intel_dp_digital_port_connected(intel_dp); if (!connected) goto mst_fail; The pulse handler for the most part can then be skipped, since the device is gone. In mst_fail, the MST topology manager is updated though, so that still has to happen. With the SST pulse handler in put_power, it can simply fall through now and hit the updated if-statement there which is: if (!intel_dp-is_mst connected) { ... code ... } And all should be well for a disconnected device as well as normal operational modes of SST and MST. Oh the intel_dp_digital_port_connected() function is just the encapsulation of this code from the long_pulse segment: if (HAS_PCH_SPLIT(dev)) { if (!ibx_digital_port_connected(dev_priv, intel_dig_port)) goto mst_fail; } else { if
[Intel-gfx] [PATCH i-g-t] tests/kms_psr_sink_crc: Make render size and stride based on modified fb size
Signed-off-by: Rodrigo Vivi rodrigo.v...@intel.com --- tests/kms_psr_sink_crc.c | 15 +-- 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c index 928552f..f5f95f4 100644 --- a/tests/kms_psr_sink_crc.c +++ b/tests/kms_psr_sink_crc.c @@ -84,6 +84,7 @@ typedef struct { struct igt_fb fb_green, fb_white; igt_plane_t *primary, *sprite, *cursor; int mod_size; + int mod_stride; drmModeModeInfo *mode; igt_output_t *output; } data_t; @@ -160,12 +161,13 @@ static void fill_blt(data_t *data, uint32_t handle, unsigned char color) gem_bo_busy(data-drm_fd, handle); } -static void scratch_buf_init(struct igt_buf *buf, drm_intel_bo *bo) +static void scratch_buf_init(struct igt_buf *buf, drm_intel_bo *bo, +int size, int stride) { buf-bo = bo; - buf-stride = 4096; + buf-stride = stride; buf-tiling = I915_TILING_X; - buf-size = 64 * 4096; + buf-size = size; } static void fill_render(data_t *data, uint32_t handle, unsigned char color) @@ -181,13 +183,13 @@ static void fill_render(data_t *data, uint32_t handle, unsigned char color) dst = gem_handle_to_libdrm_bo(data-bufmgr, data-drm_fd, , handle); igt_assert(dst); - src = drm_intel_bo_alloc(data-bufmgr, , 64 * 4096, 4096); + src = drm_intel_bo_alloc(data-bufmgr, , data-mod_size, 4096); igt_assert(src); gem_write(data-drm_fd, src-handle, 0, buf, 4); - scratch_buf_init(src_buf, src); - scratch_buf_init(dst_buf, dst); + scratch_buf_init(src_buf, src, data-mod_size, data-mod_stride); + scratch_buf_init(dst_buf, dst, data-mod_size, data-mod_stride); batch = intel_batchbuffer_alloc(data-bufmgr, data-devid); igt_assert(batch); @@ -488,6 +490,7 @@ static void setup_test_plane(data_t *data) /* Ignoring pitch and bpp to avoid changing full screen */ data-mod_size = white_h * white_v; + data-mod_stride = white_h * 4; switch (data-test_plane) { case SPRITE: -- 2.1.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/skl: Add back HDMI translation table
The HDMI translation table is added back to bspec, so adding it, and defaulting the 800mV+0dB entry. The HDMI translation table was removed by following commit as per HW team's recommendation: commit 7ff446708bd1 (drm/i915/skl: Only use the 800mV+2bB HDMI translation entry) v2: Adding reference to commit which removed this table (Jani) Cc: Damien Lespiau damien.lesp...@intel.com Signed-off-by: Sonika Jindal sonika.jin...@intel.com --- drivers/gpu/drm/i915/intel_ddi.c | 22 -- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 5b50484..b974f8e 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -155,8 +155,17 @@ static const struct ddi_buf_trans skl_ddi_translations_edp[] = { static const struct ddi_buf_trans skl_ddi_translations_hdmi[] = { - /* Idx NT mV T mVdb */ - { 0x4014, 0x0087 }, /* 0: 800 10002 */ + { 0x0018, 0x00ac }, + { 0x5012, 0x009d }, + { 0x7011, 0x0088 }, + { 0x0018, 0x00a1 }, + { 0x0018, 0x0098 }, + { 0x4013, 0x0088 }, + { 0x6012, 0x0087 }, + { 0x0018, 0x00df }, + { 0x3015, 0x0087 }, + { 0x3015, 0x00c7 }, + { 0x0018, 0x00c7 }, }; enum port intel_ddi_get_encoder_port(struct intel_encoder *intel_encoder) @@ -214,16 +223,9 @@ static void intel_prepare_ddi_buffers(struct drm_device *dev, enum port port) n_edp_entries = ARRAY_SIZE(skl_ddi_translations_dp); } - /* -* On SKL, the recommendation from the hw team is to always use -* a certain type of level shifter (and thus the corresponding -* 800mV+2dB entry). Given that's the only validated entry, we -* override what is in the VBT, at least until further notice. -*/ - hdmi_level = 0; ddi_translations_hdmi = skl_ddi_translations_hdmi; n_hdmi_entries = ARRAY_SIZE(skl_ddi_translations_hdmi); - hdmi_default_entry = 0; + hdmi_default_entry = 7; } else if (IS_BROADWELL(dev)) { ddi_translations_fdi = bdw_ddi_translations_fdi; ddi_translations_dp = bdw_ddi_translations_dp; -- 1.7.10.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 18/70] drm/i915: Implement inter-engine read-read optimisations
On 04/07/2015 04:20 PM, Chris Wilson wrote: Currently, we only track the last request globally across all engines. This prevents us from issuing concurrent read requests on e.g. the RCS and BCS engines (or more likely the render and media engines). Without semaphores, we incur costly stalls as we synchronise between rings - greatly impacting the current performance of Broadwell versus Haswell in certain workloads (like video decode). With the introduction of reference counted requests, it is much easier to track the last request per ring, as well as the last global write request so that we can optimise inter-engine read read requests (as well as better optimise certain CPU waits). v2: Fix inverted readonly condition for nonblocking waits. v3: Handle non-continguous engine array after waits v4: Rebase, tidy, rewrite ring list debugging v5: Use obj-active as a bitfield, it looks cool v6: Micro-optimise, mostly involving moving code around v7: Fix retire-requests-upto for execlists (and multiple rq-ringbuf) v8: Rebase I am still slightly concerned with the sequential ring req waiting in combination with optimistic spinning, but other than that looks good to me: Reviewed-by: Tvrtko Ursulin tvrtko.ursu...@intel.com Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 18/70] drm/i915: Implement inter-engine read-read optimisations
On Tue, Apr 14, 2015 at 02:51:37PM +0100, Tvrtko Ursulin wrote: On 04/07/2015 04:20 PM, Chris Wilson wrote: Currently, we only track the last request globally across all engines. This prevents us from issuing concurrent read requests on e.g. the RCS and BCS engines (or more likely the render and media engines). Without semaphores, we incur costly stalls as we synchronise between rings - greatly impacting the current performance of Broadwell versus Haswell in certain workloads (like video decode). With the introduction of reference counted requests, it is much easier to track the last request per ring, as well as the last global write request so that we can optimise inter-engine read read requests (as well as better optimise certain CPU waits). v2: Fix inverted readonly condition for nonblocking waits. v3: Handle non-continguous engine array after waits v4: Rebase, tidy, rewrite ring list debugging v5: Use obj-active as a bitfield, it looks cool v6: Micro-optimise, mostly involving moving code around v7: Fix retire-requests-upto for execlists (and multiple rq-ringbuf) v8: Rebase I am still slightly concerned with the sequential ring req waiting in combination with optimistic spinning, but other than that looks good to me: I hear you, I don't yet have a scenario where I care but with a little more refactoring (see next version) extending i915_wait_request to work on an array of requests will be a reasonalbly easy task. Reviewed-by: Tvrtko Ursulin tvrtko.ursu...@intel.com Thanks, but I have a new version on its way with minor changes. Spotted an issue with Ironlake and do_idling() as well as slight refactoring. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] gem_tiled_fence_blits and OOM
We're running i-g-t on Chrome OS and seeing gem_tiled_fence_blits failures. More specifically, the test is getting killed by the OOM killer, sometimes leading to hangs and reboots. This happens consistently when the UI is running. When the UI is stopped, the test passes. This makes sense since there's more memory available. I noticed the test uses intel_get_total_ram_mb() in the calculation that determines how many buffers to allocate. Excuse the perhaps naïve question, but shouldn't that be intel_get_avail_ram_mb()? When I change the test to use that call it passes consistently with no OOM issues. I realize that might be the wrong fix. I just want to understand why. Thanks, Mike Mason Chrome OS Validation Engineer SSG/OTC - Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/11] drm/i915: Update intel_dp_hpd_pulse() for non-MST operation
2015-04-14 14:36 GMT-03:00 Todd Previte tprev...@gmail.com: On 4/14/15 4:29 AM, Paulo Zanoni wrote: 2015-04-10 13:12 GMT-03:00 Todd Previte tprev...@gmail.com: Update the hot plug function to handle the SST case. Instead of placing the SST case within the long/short pulse block, it is now handled after determining that MST mode is not in use. This way, the topology management layer can handle any MST-related operations while SST operations are still correctly handled afterwards. This patch also corrects the problem of SST mode only being handled in the case of a short (0.5ms - 1.0ms) HPD pulse. For compliance testing purposes both short and long pulses are used by the different tests, thus both cases need to be addressed for SST. This patch replaces [PATCH 10/10] drm/i915: Fix intel_dp_hot_plug() in the previous compliance testing patch sequence. Review feedback on that patch indicated that updating intel_dp_hot_plug() was not the correct place for the test handler. For the SST case, the main stream is disabled for long HPD pulses as this generally indicates either a connect/disconnect event or link failure. For a number of case in compliance testing, the source is required to disable the main link upon detection of a long HPD. V2: - N/A V3: - Place the SST mode link status check into the mst_fail case - Remove obsolete comment regarding SST mode operation - Removed an erroneous line of code that snuck in during rebasing V4: - Added a disable of the main stream (DP transport) for the long pulse case for SST to support compliance testing V5: - Reworked SST handling to support tests 4.2.2.7 and 4.2.2.8 V6: - Reformatted a comment Signed-off-by: Todd Previte tprev...@gmail.com --- drivers/gpu/drm/i915/intel_dp.c | 19 --- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 77b6b15..ba2da44 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -4572,29 +4572,26 @@ intel_dp_hpd_pulse(struct intel_digital_port *intel_dig_port, bool long_hpd) if (intel_dp_check_mst_status(intel_dp) == -EINVAL) goto mst_fail; } - - if (!intel_dp-is_mst) { - /* -* we'll check the link status via the normal hot plug path later - -* but for short hpds we should check it now -*/ - drm_modeset_lock(dev-mode_config.connection_mutex, NULL); - intel_dp_check_link_status(intel_dp); - drm_modeset_unlock(dev-mode_config.connection_mutex); - } } Shouldn't the code be moved to exactly this spot instead of after the put_power label? Why would we want to call check_link_status in case we goto mst_fail? In case there is a valid reason, maybe it would be better to do a big reorganization of this function because it's going to start looking very weird - or at least rename the labels. No because then you don't get long pulses, only short ones. No, what I mean is: if (long_hpd) { ... code ... } else { ... code } if (!intel_dp-is_mst) { drm_modeset_lock(...) ... code ... } mst_fail: ... code ... The other problem I point is: imagine we're SST and we get a long_hpd. Then we run ibx_digital_port_connected(), and since the monitor is disconnected we goto mst_fail. We'll end up running intel_dp_check_link_status() before returning, but we really shouldn't run it since we know the monitor is disconnected. The put_power case is where this belongs, unless you want to duplicate code in both the long_pulse and the else clause. There is a separate mst_check_link_status call so this one is specific to SST mode. There is also a check to make sure it doesn't get called when MST is active and MST has hit a failure mode, so that is a non-issue. Also, for the long_hpd case, I see that check_link_status() will redo some of the stuff we already did on this function, such as get_dpcd(). And if you follow my advice on patch 2, you will end up having even more repeated code. I think you could try to do a careful analysis here to make sure we're not calling stuff twice here, especially since some of those operations are potentially slow. I see a couple places where the code is duplicated, specifically the connection check (which I encapsulated in a function and I'll likely roll forward into this one since it makes things more clear) and the DPCD read in the long pulse case. I removed the code in check_link_status for both of these things and it still passes compliance. Good catch Paulo. This has been fixed and tested and will be in the updated patch posted shortly. ret = IRQ_HANDLED; goto put_power; mst_fail: - /* if we were in MST mode, and device
Re: [Intel-gfx] [PATCH 1/2] intel: Defer setting madv on the bo cache
On Tue, Apr 14, 2015 at 12:38:56PM -0700, Ben Widawsky wrote: /** Frees all cached buffers significantly older than @time. */ static void drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time) @@ -1162,19 +1182,29 @@ drm_intel_gem_cleanup_bo_cache(drm_intel_bufmgr_gem *bufmgr_gem, time_t time) for (i = 0; i bufmgr_gem-num_buckets; i++) { struct drm_intel_gem_bo_bucket *bucket = bufmgr_gem-cache_bucket[i]; + drmMMListHead madv; + DRMINITLISTHEAD(madv); while (!DRMLISTEMPTY(bucket-head)) { drm_intel_bo_gem *bo_gem; bo_gem = DRMLISTENTRY(drm_intel_bo_gem, bucket-head.next, head); - if (time - bo_gem-free_time = 1) + if (time - bo_gem-free_time 2*(1+bo_gem-dontneed)) Something somewhere will complain about mixing bools and ints, I'd guess. Also perhaps for perf bisection maybe do a patch before this changing the expiration time to 2 (or 4), then add the extra dontneed state? It is already 2, since it uses = and the patch just converts it to to be consistent after the multiplication. break; DRMLISTDEL(bo_gem-head); - - drm_intel_gem_bo_free(bo_gem-bo); + if (bo_gem-dontneed) { + drm_intel_gem_bo_free(bo_gem-bo); + } else { + drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem, + I915_MADV_DONTNEED); + bo_gem-dontneed = 1; + DRMLISTADDTAIL(bo_gem-head, madv); + } } Maybe add a comment here? /* Objects which have elapsed 2s of disuse should go to the top of the bucket. */ + if (!DRMLISTEMPTY(madv)) + __splice(madv, bucket-head, bucket-head.next); } bufmgr_gem-time = time; @@ -1285,9 +1315,7 @@ drm_intel_gem_bo_unreference_final(drm_intel_bo *bo, time_t time) bucket = drm_intel_gem_bo_bucket_for_size(bufmgr_gem, bo-size); /* Put the buffer into our internal cache for reuse if we can. */ - if (bufmgr_gem-bo_reuse bo_gem-reusable bucket != NULL - drm_intel_gem_bo_madvise_internal(bufmgr_gem, bo_gem, - I915_MADV_DONTNEED)) { + if (bufmgr_gem-bo_reuse bo_gem-reusable bucket != NULL) { bo_gem-free_time = time; bo_gem-name = NULL; In general, it looks fine to me. Like I said above wrt adding the patch before this, I am curious how much difference you see just messing with the expiration times versus the two state model. The issue I was looking at was simply struct_mutex contention in madvise (then busy). For busy we can tweak the ordering to reduce the contention slightly, but madvise is trickier (though madvise is a candidate for one of the first per-object locks). This just moves the contention elsewhere, but madvise/busy calls tend to dominate overall and trimming them reduces the intersection. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] lib: Cache static queries
We frequently check for device capabilities, for which we can safely assume that there is but one on a system and so cache the first query value and return it for all future queries. The benefit is to reduce dmesg debug spam which helps when either bringing up a test or trying to track down why a test fails. Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk --- lib/igt_gt.c | 2 +- lib/ioctl_wrappers.c | 130 ++- lib/ioctl_wrappers.h | 1 + 3 files changed, 89 insertions(+), 44 deletions(-) diff --git a/lib/igt_gt.c b/lib/igt_gt.c index 84aa5d3..deb5560 100644 --- a/lib/igt_gt.c +++ b/lib/igt_gt.c @@ -59,7 +59,7 @@ */ void igt_require_hang_ring(int fd, int ring) { - gem_context_require_param(fd, LOCAL_CONTEXT_PARAM_BAN_PERIOD); + gem_context_require_ban_period(fd); igt_require(intel_gen(intel_get_drm_devid(fd)) = 5); } diff --git a/lib/ioctl_wrappers.c b/lib/ioctl_wrappers.c index 000d394..05f696f 100644 --- a/lib/ioctl_wrappers.c +++ b/lib/ioctl_wrappers.c @@ -724,6 +724,24 @@ void gem_context_require_param(int fd, uint64_t param) igt_require(drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM, p) == 0); } +void gem_context_require_ban_period(int fd) +{ + static int has_ban_period = -1; + + if (has_ban_period 0) { + struct local_i915_gem_context_param p; + + p.context = 0; + p.param = LOCAL_CONTEXT_PARAM_BAN_PERIOD; + p.value = 0; + p.size = 0; + + has_ban_period = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_CONTEXT_GETPARAM, p) == 0; + } + + igt_require(has_ban_period); +} + /** * gem_sw_finish: * @fd: open i915 drm file descriptor @@ -807,34 +825,40 @@ bool gem_uses_aliasing_ppgtt(int fd) */ int gem_available_fences(int fd) { - struct drm_i915_getparam gp; - int val = 0; + static int num_fences = -1; - memset(gp, 0, sizeof(gp)); - gp.param = I915_PARAM_NUM_FENCES_AVAIL; - gp.value = val; + if (num_fences 0) { + struct drm_i915_getparam gp; - if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, gp, sizeof(gp))) - return 0; + memset(gp, 0, sizeof(gp)); + gp.param = I915_PARAM_NUM_FENCES_AVAIL; + gp.value = num_fences; - errno = 0; - return val; + num_fences = 0; + ioctl(fd, DRM_IOCTL_I915_GETPARAM, gp, sizeof(gp)); + errno = 0; + } + + return num_fences; } bool gem_has_llc(int fd) { - struct drm_i915_getparam gp; - int val = 0; + static int has_llc = -1; - memset(gp, 0, sizeof(gp)); - gp.param = I915_PARAM_HAS_LLC; - gp.value = val; + if (has_llc 0) { + struct drm_i915_getparam gp; - if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, gp, sizeof(gp))) - return 0; + memset(gp, 0, sizeof(gp)); + gp.param = I915_PARAM_HAS_LLC; + gp.value = has_llc; - errno = 0; - return val; + has_llc = 0; + ioctl(fd, DRM_IOCTL_I915_GETPARAM, gp, sizeof(gp)); + errno = 0; + } + + return has_llc; } /** @@ -851,24 +875,26 @@ bool gem_has_llc(int fd) */ int gem_get_num_rings(int fd) { - int num_rings = 1; /* render ring is always available */ + static int num_rings = -1; - if (gem_has_bsd(fd)) - num_rings++; - else - goto skip; + if (num_rings 0) { + num_rings = 1; /* render ring is always available */ - if (gem_has_blt(fd)) - num_rings++; - else - goto skip; - - if (gem_has_vebox(fd)) - num_rings++; - else - goto skip; + if (gem_has_bsd(fd)) + num_rings++; + else + goto skip; + if (gem_has_blt(fd)) + num_rings++; + else + goto skip; + if (gem_has_vebox(fd)) + num_rings++; + else + goto skip; + } skip: return num_rings; } @@ -911,7 +937,10 @@ bool gem_has_enable_ring(int fd,int param) */ bool gem_has_bsd(int fd) { - return gem_has_enable_ring(fd,I915_PARAM_HAS_BSD); + static int has_bsd = -1; + if (has_bsd 0) + has_bsd = gem_has_enable_ring(fd,I915_PARAM_HAS_BSD); + return has_bsd; } /** @@ -927,7 +956,10 @@ bool gem_has_bsd(int fd) */ bool gem_has_blt(int fd) { - return gem_has_enable_ring(fd,I915_PARAM_HAS_BLT); + static int has_blt = -1; + if (has_blt 0) + has_blt = gem_has_enable_ring(fd,I915_PARAM_HAS_BLT); + return has_blt; } #define LOCAL_I915_PARAM_HAS_VEBOX 22 @@
Re: [Intel-gfx] [PATCH] drm/i915: Add missing MacBook Pro models with dual channel LVDS
Hi Jani, I think this begs the question, why don't we assume dual link lvds when we know the native mode of the panel necessitates it? Yes that's a great idea, that way we can avoid hardcoding the 17 models. Coming up in a separate e-mail is a new version of the patch. Single channel LVDS maxes out at 112 MHz, according to section 2.3 of this document: https://01.org/linuxgraphics/sites/default/files/documentation/ivb_ihd_os_vol3_part4.pdf The resolution of the 17 models -- 1920x1200 -- always needs more bandwidth than 112 MHz (at 60 Hz it's 193 MHz), thus requires dual channels. The 15 models are a bit more complicated: By default they shipped with 1440x900, that's 106 MHz so single channel LVDS would be sufficient. As a BTO option however they were available with 1680x1050, that's 119 MHz and requires dual channels. It turns out Apple apparently used dual channels on *both* 15 versions, presumably to reduce the number of different parts, i.e. use identical mainboards and display cabling on both versions and the only differing component is the panel. E.g. the Chi Mei N154C6-L04 panel with 1440x900... http://www.ebay.com/itm/-/400690878560 ... is a replacement panel for all A1286 models, and that model number encompasses the MacBookPro6,2 / 8,2 / 9,1... http://www.everymac.com/ultimate-mac-lookup/?search_keywords=A1286 ... and page 17 of the panel's datasheet shows it's driven with dual channel LVDS: http://www.taopanel.com/chimei/datasheet/N154C6-L04.pdf Those three 15 models, MacBookPro6,2 / 8,2 / 9,1, are the only ones with i915 graphics and dual channel LVDS, so that list should be complete. And the 8,2 is already in intel_lvds.c. Tested on a MacBookPro9,1 with 1680x1050. Kind regards, Lukas ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx