[Intel-gfx] [PULL] drm-intel-fixes
Hi Dave - i915 fixes for v4.3. The revert dance could use some explanation: we had stuff fixed in -next, and initially backported one commit to v4.3. Now, turns out we need more fixes, and we could cherry-pick them all without conflicts if we reverted the backported one first. So did that to not have to edit and backport them all. BR, Jani. The following changes since commit 25cb62b76430a91cc6195f902e61c2cb84ade622: Linux 4.3-rc5 (2015-10-11 11:09:45 -0700) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/drm-intel-fixes-2015-10-16 for you to fetch changes up to 18e9345b0db9fe7bd18c3c43967789fe0a2fdb52: drm/i915: Add primary plane to mask if it's visible (2015-10-14 13:47:44 +0300) Chris Wilson (2): drm/i915: Flush pipecontrol post-sync writes drm/i915: Deny wrapping an userptr into a framebuffer Daniel Vetter (1): drm/i915: Fix kerneldoc for i915_gem_shrink_all Jani Nikula (1): Revert "drm/i915: Add primary plane to mask if it's visible" Maarten Lankhorst (1): drm/i915: Add primary plane to mask if it's visible Ville Syrjälä (4): drm/i915: Restore lost DPLL register write on gen2-4 drm/i915: Enable DPLL VGA mode before P1/P2 divider write drm/i915: Assign hwmode after encoder state readout drm/i915: Move sprite/cursor plane disable to intel_sanitize_crtc() drivers/gpu/drm/i915/i915_gem_shrinker.c | 2 +- drivers/gpu/drm/i915/i915_gem_userptr.c | 5 +- drivers/gpu/drm/i915/intel_display.c | 120 +-- drivers/gpu/drm/i915/intel_lrc.c | 1 + drivers/gpu/drm/i915/intel_ringbuffer.c | 2 + 5 files changed, 75 insertions(+), 55 deletions(-) -- 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] igt/kms_addfb_basic: New subtest to check for fb modifier and tiling mode mismatch
Hi Tvrtko, On Fri, 9 Oct 2015 09:34:25 +0100 Tvrtko Ursulin wrote: > > > On 08/10/15 09:55, Tvrtko Ursulin wrote: > > On 07/10/15 22:07, Vivek Kasireddy wrote: > >> > >> Hi Tvrtko, > >> > >> On Wed, 7 Oct 2015 15:07:30 +0100 > >> Tvrtko Ursulin wrote: > >> > >>> > >>> Hi, > >>> > >>> On 07/10/15 03:35, Vivek Kasireddy wrote: > This new subtest will validate a Y-tiled object's tiling mode > against its associated fb modifier. > > Cc: Tvrtko Ursulin > Signed-off-by: Vivek Kasireddy > --- > tests/kms_addfb_basic.c | 9 + > 1 file changed, 9 insertions(+) > > diff --git a/tests/kms_addfb_basic.c b/tests/kms_addfb_basic.c > index d466e4d..7ca1add 100644 > --- a/tests/kms_addfb_basic.c > +++ b/tests/kms_addfb_basic.c > @@ -373,6 +373,15 @@ static void addfb25_ytile(int fd, int gen) > f.handles[0] = gem_bo; > } > > +igt_subtest("addfb25-Y-tiled-X-modifier-mismatch") { > +igt_require(gen >= 9); > +igt_require_fb_modifiers(fd); > +gem_set_tiling(fd, gem_bo, I915_TILING_Y, 1024*4); > + > +f.modifier[0] = LOCAL_I915_FORMAT_MOD_X_TILED; > +igt_assert(drmIoctl(fd, > LOCAL_DRM_IOCTL_MODE_ADDFB2, &f) < 0 && errno == EINVAL); > +} > + > igt_subtest("addfb25-Y-tiled") { > igt_require_fb_modifiers(fd); > > >>> > >>> Wasn't the original WARN triggered by Y tiled object and Y fb > >>> modifier? > >> > >> Creating a new fb using a Y-tiled object and Y/X fb modifier will > >> not trigger the original WARN. It'll be triggered only if the fb > >> is going to be pinned -- and flipped. I am not sure how to get > >> that WARN to be triggered with the existing suite of igt tests. > > > > Ah yes, you would need to attempt display it, not even necessarily > > flip it. I am sure there are tests which do that. :) I know from > > recent activity kms_rotation_crc for example creates a Y tiled FB > > and displays it. So maybe borrow some code to start with from there. > > Even better, kms_flip_tiling does the majority of what is needed here > already. Just drop in a subtest which will do set_tiling and that > should be good. I have realized that I cannot get the the map_and_fenceable WARN to be triggered with any of the IGT tests. This is because the WARN is triggered only when pinning/fencing a Y-tiled fb that has a rotated view (90/270 degree rotation) that none of the IGT tests can do. I looked at and wrote a subtest in kms_rotation_crc but the WARN was not triggered because IGT does not support atomic flip/commit yet. Currently, since it does a SetPlane first, the object has a normal view and its map_and-fenceable bit is set, however, when the rotation property is applied, the object though has a rotated view, its map_and-fenceable bit never gets updated and stays 1 and hence the warning doesn't get triggered. I am copying the subtest code below for reference. From 5d7f6920663b2fd264aca6085170811276948a5b Mon Sep 17 00:00:00 2001 From: Vivek Kasireddy Date: Thu, 15 Oct 2015 17:48:50 -0700 Subject: [PATCH] igt/kms_rotation_crc: Add a new subtest to flip Y-tiled rotated fb --- tests/kms_rotation_crc.c | 70 1 file changed, 70 insertions(+) diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c index cc9847e..f10a770 100644 --- a/tests/kms_rotation_crc.c +++ b/tests/kms_rotation_crc.c @@ -264,6 +264,70 @@ static void test_plane_rotation(data_t *data, enum igt_plane plane_type) igt_require_f(valid_tests, "no valid crtc/connector combinations found\n"); } +static void test_plane_rotation_ytiled(data_t *data, enum igt_plane plane_type) +{ + igt_display_t *display = &data->display; + igt_output_t *output; + enum igt_commit_style commit = COMMIT_LEGACY; + drmModeModeInfo *mode; + unsigned int w, h; + igt_plane_t *plane; + int ret; + uint64_t tiling = LOCAL_I915_FORMAT_MOD_Y_TILED; + uint32_t format = DRM_FORMAT_XRGB; + int bpp = igt_drm_format_to_bpp(format); + + if (plane_type == IGT_PLANE_PRIMARY || plane_type == IGT_PLANE_CURSOR) { + igt_require(data->display.has_universal_planes); + commit = COMMIT_UNIVERSAL; + } + + for_each_connected_output(display, output) { + mode = igt_output_get_mode(output); + + w = mode->hdisplay; + h = mode->vdisplay; + + for (data->fb.stride = 512; data->fb.stride < (w * bpp / 8); data->fb.stride *= 2); + + for (data->fb.size = 1024*1024; data->fb.size < data->fb.stride * h; data->fb.size *= 2); + + data->fb.gem_handle = gem_create(data->gfx_fd, data->fb.size); + ret = __gem_set_tiling(data->gfx_fd, data->fb.gem_handle, I915_TILING_Y, data->fb.stride); + igt_asser
Re: [Intel-gfx] [PATCH] drm/i915: respect previous reg values on primary plane disable
On Thu, Oct 15, 2015 at 04:00:48PM -0700, Kevin Strasser wrote: > On Thu, Oct 15, 2015 at 11:20:59AM +0300, Ville Syrjälä wrote: > > On Wed, Oct 14, 2015 at 01:33:57PM -0700, Kevin Strasser wrote: > > > On Wed, Oct 14, 2015 at 10:48:52PM +0300, Ville Syrjälä wrote: > > > > Does it? I just tried it on IVB, and behaves just like you said. So not > > > > sure how far back this goes. > > > > > > Ah, so this test case is failing on IVB too? > > > > I just poked at the registers. I don't think we have specific test cases > > for this, so any currently failing test case is just bad luck. > > > > It would be good to write a small tool that just frobs the registers in > > specific ways, and tells us if the test machine suffers from this issue. > > Would be easy to run on any machine then. > > Agreed, it is somewhat painful getting the whole test suite built and running > in > some environments. > > > What I did manually was: > > intel_reg write 0x4a000 0x40 > > intel_reg write 0x70180 0x0 > > = intel_reg read 0x7019c > > intel_reg write 0x7019c > > > > and as a second test I tried: > > intel_reg write 0x70180 > = intel_reg read 0x7019c > > intel_reg write 0x7019c > > > > And the result was black in the first test, dark red in > > the second. > > > > On SKL I additionally tried to resize the plane to be smaller, and then > > tried the same thing. But as stated the palette seems to misbehave > > somehow, so there was no change in the output from changing entry 0 even > > though most of the screen was supposed to be black. > > > > > Are there any reporting tools we > > > can look at to find out what tests are passing for each platform? > > > > > > > And now I'm really wondering about platforms where the primary > > > > plane need not be fullscreen (gen2/3 and chv). > > > > > > > > I tried this on SKL too, but that confused me even more. The data not > > > > going through any plane seems to be gamma corrected regardless of any > > > > plane control bits, so that's good. However the legacy palette seems > > > > all fubar. Black input apparently doesn't map to palette entry 0. > > > > I wonder if you're seeing this on HSW too, or is your palette entry 0 > > > > supposed to be non-black? > > > > > > I also tried on BDW and it is passing the test with and without my patch > > > applied. > > > > > > I'm not really sure what 'palette entry 0' you are looking for. Could you > > > point > > > me to where in the code I can find out? > > > > Register 0x4a000, 0x4a800, 0x4b000, depending on which pipe you're > > using. > > I'm using pipe A, here is the output of 'intel_reg read 0x4a000': > (0x0004a000): 0x > > I suppose this means palette entry 0 is black for me. It means it's supposed to be black. But if you saw a difference between black when it went through the gamma and when it didn't, then I guess black is not hitting entry 0 on hsw either. So something seems fishy. > > > > > Looks like quite a bit more testing is needed to get to the bottom of > > > > this. > > > > > > Agreed, this does seem to extend beyond HSW. For now do you think my > > > patch is > > > the right approach at least for HSW alone? > > > > Yeah, looks that way. But we do need to figure out which bits behave > > this way. Ie. is it just gamma, or pipe csc too, or perhaps even some > > other bits? > > I did some testing and it seems that gamma and pipe csc are both needed to > pass > the test on HSW. v2 of the patch sets them explicitly. > > Thanks, > Kevin -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH igt] kms_cursor_crc: Add test for unthrottled cursor movement
We've had bugs in the past that caused cursor updates to be synced to vblank, resulting in sluggish cursor movement. Add a test to try to make sure we don't regress and reintroduce these bugs. Cc: kalyan.kondapa...@intel.com Signed-off-by: Matt Roper --- tests/kms_cursor_crc.c | 42 ++ 1 file changed, 42 insertions(+) diff --git a/tests/kms_cursor_crc.c b/tests/kms_cursor_crc.c index 3495b26..d1de450 100644 --- a/tests/kms_cursor_crc.c +++ b/tests/kms_cursor_crc.c @@ -55,6 +55,7 @@ typedef struct { igt_crc_t ref_crc; int left, right, top, bottom; int screenw, screenh; + int refresh; int curw, curh; /* cursor size */ int cursor_max_w, cursor_max_h; igt_pipe_crc_t *pipe_crc; @@ -327,6 +328,7 @@ static bool prepare_crtc(data_t *data, igt_output_t *output, data->screenh = mode->vdisplay; data->curw = cursor_w; data->curh = cursor_h; + data->refresh = mode->vrefresh; /* make sure cursor is disabled */ cursor_disable(data); @@ -479,6 +481,42 @@ static void test_cursor_size(data_t *data) } } +static void test_rapid_movement(data_t *data) +{ + struct timeval start, end, delta; + int x = 0, y = 0; + long usec; + int crtc_id = data->output->config.crtc->crtc_id; + + igt_assert_eq(drmModeSetCursor(data->drm_fd, crtc_id, + data->fb.gem_handle, data->curw, data->curh), 0); + + gettimeofday(&start, NULL); + for ( ; x < 100; x++) + igt_assert_eq(drmModeMoveCursor(data->drm_fd, crtc_id, x, y), 0); + for ( ; y < 100; y++) + igt_assert_eq(drmModeMoveCursor(data->drm_fd, crtc_id, x, y), 0); + for ( ; x > 0; x--) + igt_assert_eq(drmModeMoveCursor(data->drm_fd, crtc_id, x, y), 0); + for ( ; y > 0; y--) + igt_assert_eq(drmModeMoveCursor(data->drm_fd, crtc_id, x, y), 0); + gettimeofday(&end, NULL); + + /* +* We've done 400 cursor updates now. If we're being throttled to +* vblank, then that would take roughly 400/refresh seconds. If the +* elapsed time is greater than 90% of that value, we'll consider it +* a failure (since cursor updates shouldn't be throttled). +*/ + timersub(&end, &start, &delta); + usec = delta.tv_usec + 100 * delta.tv_sec; + igt_assert_lt(usec, 0.9 * 400 * 100 / data->refresh); + + igt_assert_eq(drmModeSetCursor(data->drm_fd, crtc_id, + 0, data->curw, data->curh), 0); + +} + static void run_test_generic(data_t *data) { int cursor_size; @@ -510,6 +548,10 @@ static void run_test_generic(data_t *data) data->flags = 0; } + igt_subtest_f("cursor-%dx%d-rapid-movement", w, h) { + run_test(data, test_rapid_movement, w, h); + } + igt_fixture igt_remove_fb(data->drm_fd, &data->fb); -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: respect previous reg values on primary plane disable
On Thu, Oct 15, 2015 at 11:20:59AM +0300, Ville Syrjälä wrote: > On Wed, Oct 14, 2015 at 01:33:57PM -0700, Kevin Strasser wrote: > > On Wed, Oct 14, 2015 at 10:48:52PM +0300, Ville Syrjälä wrote: > > > Does it? I just tried it on IVB, and behaves just like you said. So not > > > sure how far back this goes. > > > > Ah, so this test case is failing on IVB too? > > I just poked at the registers. I don't think we have specific test cases > for this, so any currently failing test case is just bad luck. > > It would be good to write a small tool that just frobs the registers in > specific ways, and tells us if the test machine suffers from this issue. > Would be easy to run on any machine then. Agreed, it is somewhat painful getting the whole test suite built and running in some environments. > What I did manually was: > intel_reg write 0x4a000 0x40 > intel_reg write 0x70180 0x0 > = intel_reg read 0x7019c > intel_reg write 0x7019c > > and as a second test I tried: > intel_reg write 0x70180 = intel_reg read 0x7019c > intel_reg write 0x7019c > > And the result was black in the first test, dark red in > the second. > > On SKL I additionally tried to resize the plane to be smaller, and then > tried the same thing. But as stated the palette seems to misbehave > somehow, so there was no change in the output from changing entry 0 even > though most of the screen was supposed to be black. > > > Are there any reporting tools we > > can look at to find out what tests are passing for each platform? > > > > > And now I'm really wondering about platforms where the primary > > > plane need not be fullscreen (gen2/3 and chv). > > > > > > I tried this on SKL too, but that confused me even more. The data not > > > going through any plane seems to be gamma corrected regardless of any > > > plane control bits, so that's good. However the legacy palette seems > > > all fubar. Black input apparently doesn't map to palette entry 0. > > > I wonder if you're seeing this on HSW too, or is your palette entry 0 > > > supposed to be non-black? > > > > I also tried on BDW and it is passing the test with and without my patch > > applied. > > > > I'm not really sure what 'palette entry 0' you are looking for. Could you > > point > > me to where in the code I can find out? > > Register 0x4a000, 0x4a800, 0x4b000, depending on which pipe you're > using. I'm using pipe A, here is the output of 'intel_reg read 0x4a000': (0x0004a000): 0x I suppose this means palette entry 0 is black for me. > > > Looks like quite a bit more testing is needed to get to the bottom of > > > this. > > > > Agreed, this does seem to extend beyond HSW. For now do you think my patch > > is > > the right approach at least for HSW alone? > > Yeah, looks that way. But we do need to figure out which bits behave > this way. Ie. is it just gamma, or pipe csc too, or perhaps even some > other bits? I did some testing and it seems that gamma and pipe csc are both needed to pass the test on HSW. v2 of the patch sets them explicitly. Thanks, Kevin ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915/skl: Eliminate usage of pipe_wm_parameters from SKL-style WM (v4)
Just pull the info out of the state structures rather than staging it in an additional set of structures. To make this more straightforward, we change the signature of several internal WM functions to take the crtc state as a parameter. v2: - Don't forget to skip cursor planes on a loop in the DDB allocation function to match original behavior. (Ander) - Change a use of intel_crtc->active to cstate->active. They should be identical, but it's better to be consistent. (Ander) - Rework more function signatures to pass states rather than crtc for consistency. (Ander) v3: - Add missing "+ 1" to skl_wm_plane_id()'s 'overlay' case. (Maarten) - Packed formats should pass '0' to drm_format_plane_cpp(), not 1. (Maarten) - Drop unwanted WARN_ON() for disabled planes when calculating data rate for SKL. (Maarten) v4: - Don't include cursor plane in total relative data rate calculation; we've already handled the cursor allocation earlier. - Fix 'bytes_per_pixel' calculation braindamage. Somehow I hardcoded the NV12 format as a parameter rather than the actual fb->pixel_format, and even then still managed to get the format plane wrong. (Ville) - Use plane->state->fb rather than plane->fb in skl_allocate_pipe_ddb(); the plane->fb pointer isn't updated until after we've done our watermark recalculation, so it has stale values. (Bob Paauwe) Signed-off-by: Matt Roper Reviewed-by(v3): Maarten Lankhorst Cc: Paauwe, Bob J Cc: Ville Syrjälä Cc: Paulo Zanoni References: http://lists.freedesktop.org/archives/intel-gfx/2015-September/077060.html References: http://lists.freedesktop.org/archives/intel-gfx/2015-October/077721.html --- Paulo, would you mind trying this patch out when you have some free time and see whether you still experience watermark problems? This is the patch that you bisected your problems to and there were at least three bugs in the version of this patch that was merged before; I'm hoping that with these three bugfixes your problems will be gone. drivers/gpu/drm/i915/intel_pm.c | 327 +++- 1 file changed, 152 insertions(+), 175 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index 9dda3ea..df22b9c 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -1708,13 +1708,6 @@ static uint32_t ilk_wm_fbc(uint32_t pri_val, uint32_t horiz_pixels, return DIV_ROUND_UP(pri_val * 64, horiz_pixels * bytes_per_pixel) + 2; } -struct skl_pipe_wm_parameters { - bool active; - uint32_t pipe_htotal; - uint32_t pixel_rate; /* in KHz */ - struct intel_plane_wm_parameters plane[I915_MAX_PLANES]; -}; - struct ilk_wm_maximums { uint16_t pri; uint16_t spr; @@ -2755,18 +2748,40 @@ static bool ilk_disable_lp_wm(struct drm_device *dev) #define SKL_DDB_SIZE 896 /* in blocks */ #define BXT_DDB_SIZE 512 +/* + * Return the index of a plane in the SKL DDB and wm result arrays. Primary + * plane is always in slot 0, cursor is always in slot I915_MAX_PLANES-1, and + * other universal planes are in indices 1..n. Note that this may leave unused + * indices between the top "sprite" plane and the cursor. + */ +static int +skl_wm_plane_id(const struct intel_plane *plane) +{ + switch (plane->base.type) { + case DRM_PLANE_TYPE_PRIMARY: + return 0; + case DRM_PLANE_TYPE_CURSOR: + return PLANE_CURSOR; + case DRM_PLANE_TYPE_OVERLAY: + return plane->plane + 1; + default: + MISSING_CASE(plane->base.type); + return plane->plane; + } +} + static void skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, - struct drm_crtc *for_crtc, + const struct intel_crtc_state *cstate, const struct intel_wm_config *config, - const struct skl_pipe_wm_parameters *params, struct skl_ddb_entry *alloc /* out */) { + struct drm_crtc *for_crtc = cstate->base.crtc; struct drm_crtc *crtc; unsigned int pipe_size, ddb_size; int nth_active_pipe; - if (!params->active) { + if (!cstate->base.active) { alloc->start = 0; alloc->end = 0; return; @@ -2832,19 +2847,29 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv, } static unsigned int -skl_plane_relative_data_rate(const struct intel_plane_wm_parameters *p, int y) +skl_plane_relative_data_rate(const struct intel_crtc_state *cstate, +const struct drm_plane_state *pstate, +int y) { + struct intel_crtc *intel_crtc = to_intel_crtc(cstate->base.crtc); + struct drm_framebuffer *fb = pstate->fb; /* for planar format */ -
[Intel-gfx] [PATCH v2 21/22] drm/i915: Rewrite fb rotation GTT handling
From: Ville Syrjälä Redo the fb rotation handling in order to: - eliminate the NV12 special casing - handle fb->offsets[] properly - make the rotation handling reasier for the plane code To achieve these goals we reduce intel_rotation_info to only contain (for each plane) the rotated view width,height,stride in tile units, and the page offset into the object where the plane starts. Each plane is handled exactly the same way, no special casing for NV12 or other formats. We then store the computed rotation_info under intel_framebuffer so that we don't have to recompute it again. To handle fb->offsets[] we treat them as a linear offsets and convert them to x/y offsets from the start of the relevant GTT mapping (either normal or rotated). We store the x/y offsets under intel_framebuffer, and for some extra convenience we also store the rotated pitch (ie. tile aligned plane height). So for each plane we have the normal x/y offsets, rotated x/y offsets, and the rotated pitch. The normal pitch is available already in fb->pitches[]. While we're gathering up all that extra information, we can also easily compute the storage requirements for the framebuffer, so that we can check that the object is big enough to hold it. When it comes time to deal with the plane source coordinates, we first rotate the clipped src coordinates to match the relevant GTT view orientation, then add to them the fb x/y offsets. Next we compute the aligned surface page offset, and as a result we're left with some residual x/y offsets. Finally, if required by the hardware, we convert the remaining x/y offsets into a linear offset. For gen2/3 we simply skip computing the final page offset, and just convert the src+fb x/y offsets directly into a linear offset since that's what the hardware wants. After this all platforms, incluing SKL+, compute these things in exactly the same way (excluding alignemnt differences). v2: Use BIT(DRM_ROTATE_270) instead of ROTATE_270 when rotating plane src coordinates Drop some spurious changes that got left behind during development Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.c | 66 ++ drivers/gpu/drm/i915/i915_gem_gtt.h | 14 +- drivers/gpu/drm/i915/intel_display.c | 448 --- drivers/gpu/drm/i915/intel_drv.h | 32 ++- drivers/gpu/drm/i915/intel_sprite.c | 102 5 files changed, 402 insertions(+), 260 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index bb95b86..98aee6c 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -3255,11 +3255,6 @@ rotate_pages(const dma_addr_t *in, unsigned int offset, unsigned int column, row; unsigned int src_idx; - if (!sg) { - st->nents = 0; - sg = st->sgl; - } - for (column = 0; column < width; column++) { src_idx = stride * (height - 1) + column; for (row = 0; row < height; row++) { @@ -3280,16 +3275,14 @@ rotate_pages(const dma_addr_t *in, unsigned int offset, } static struct sg_table * -intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info, +intel_rotate_fb_obj_pages(const struct intel_rotation_info *info, struct drm_i915_gem_object *obj) { - unsigned int size_pages = rot_info->size >> PAGE_SHIFT; - unsigned int size_pages_uv; + unsigned int size = intel_rotation_info_size(info); struct sg_page_iter sg_iter; unsigned long i; dma_addr_t *page_addr_list; struct sg_table *st; - unsigned int uv_start_page; struct scatterlist *sg; int ret = -ENOMEM; @@ -3299,57 +3292,32 @@ intel_rotate_fb_obj_pages(struct intel_rotation_info *rot_info, if (!page_addr_list) return ERR_PTR(ret); - /* Account for UV plane with NV12. */ - if (rot_info->pixel_format == DRM_FORMAT_NV12) - size_pages_uv = rot_info->size_uv >> PAGE_SHIFT; - else - size_pages_uv = 0; - /* Allocate target SG list. */ st = kmalloc(sizeof(*st), GFP_KERNEL); if (!st) goto err_st_alloc; - ret = sg_alloc_table(st, size_pages + size_pages_uv, GFP_KERNEL); + ret = sg_alloc_table(st, size, GFP_KERNEL); if (ret) goto err_sg_alloc; /* Populate source page list from the object. */ i = 0; for_each_sg_page(obj->pages->sgl, &sg_iter, obj->pages->nents, 0) { - page_addr_list[i] = sg_page_iter_dma_address(&sg_iter); - i++; + page_addr_list[i++] = sg_page_iter_dma_address(&sg_iter); } - /* Rotate the pages. */ - sg = rotate_pages(page_addr_list, 0, -rot_info->width_pages, rot_info->height_pages, -rot_info->width_pages, -st, NULL); + st->nents
Re: [Intel-gfx] [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation
On Thu, Oct 15, 2015 at 07:14:35PM +0200, Lukas Wunner wrote: > Hi Ville, > > On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote: > > On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote: > > > From: Tvrtko Ursulin > > > > > > We had two failure modes here: > > > > > > 1. > > > Deadlock in intelfb_alloc failure path where it calls > > > drm_framebuffer_remove, which grabs the struct mutex and intelfb_create > > > (caller of intelfb_alloc) was already holding it. > > > > > > 2. > > > Deadlock in intelfb_create failure path where it calls > > > drm_framebuffer_unreference, which grabs the struct mutex and > > > intelfb_create was already holding it. > > > > > > v2: > > >* Reformat commit msg to 72 chars. (Lukas Wunner) > > >* Add third failure mode. (Lukas Wunner) > > > > > > v3: > > >* On fb alloc failure, unref gem object where it gets refed, > > > fix double unref in separate commit. (Ville Syrjälä) > > > > > > v4: > > >* Lock struct mutex on unref. (Chris Wilson) > > > > > > v5: > > >* Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC, > > > rephrase commit message. (Jani Nicula) > > > > > > Tested-by: Pierre Moreau > > > [MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina] > > > Tested-by: Paul Hordiienko > > > [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina] > > > Tested-by: William Brown > > > [MBP 8,2 2011 intel SNB + amd turks pre-retina] > > > Tested-by: Lukas Wunner > > > [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina] > > > Tested-by: Bruno Bierbaumer > > > [MBP 11,3 2013 intel HSW + nvidia GK107 retina] > > > > > > Signed-off-by: Tvrtko Ursulin > > > Fixes: 60a5ca015ffd ("drm/i915: Add locking around > > > framebuffer_references--") > > > Reported-by: Lukas Wunner > > > [Lukas: Create v3 + v4 + v5 based on Tvrtko's v2] > > > Signed-off-by: Lukas Wunner > > > Cc: Chris Wilson > > > Cc: Ville Syrjälä > > > Cc: Jani Nikula > > > --- > > > drivers/gpu/drm/i915/intel_fbdev.c | 20 > > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > > b/drivers/gpu/drm/i915/intel_fbdev.c > > > index 96476d7..eee3306 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper, > > > { > > > struct intel_fbdev *ifbdev = > > > container_of(helper, struct intel_fbdev, helper); > > > - struct drm_framebuffer *fb; > > > + struct drm_framebuffer *fb = NULL; > > > struct drm_device *dev = helper->dev; > > > struct drm_mode_fb_cmd2 mode_cmd = {}; > > > struct drm_i915_gem_object *obj; > > > @@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper, > > > mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp, > > > sizes->surface_depth); > > > > > > + mutex_lock(&dev->struct_mutex); > > > + > > > size = mode_cmd.pitches[0] * mode_cmd.height; > > > size = PAGE_ALIGN(size); > > > obj = i915_gem_object_create_stolen(dev, size); > > > @@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper > > > *helper, > > > ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL); > > > if (ret) { > > > DRM_ERROR("failed to pin obj: %d\n", ret); > > > - goto out_fb; > > > + goto out_unref; > > > } > > > > > > + mutex_unlock(&dev->struct_mutex); > > > + > > > ifbdev->fb = to_intel_framebuffer(fb); > > > > > > return 0; > > > > > > -out_fb: > > > - drm_framebuffer_remove(fb); > > > out_unref: > > > drm_gem_object_unreference(&obj->base); > > > > If fb init succeeded it took over the ref, no? So drm_framebuffer_remove() > > will now attempt to unref one too many times. > > > > This taking over refs stuff is confusing. Maybe it would be better if > > everyone just took an extra ref when they stash the obj pointer > > somewhere, and everyone would then always release whatever ref they own > > and no longer need. > > > > > out: > > > + mutex_unlock(&dev->struct_mutex); > > > + if (fb) > > > + drm_framebuffer_remove(fb); > > > return ret; > > > } > > > > > Hm, why do you think we unref one too many times? Because the fb now owns the reference, so it gets unreffed by the fb .destroy() hook... I think. > > A bit further up in this function we call __intel_framebuffer_create() > which sets the refcount to 1. (It calls intel_framebuffer_init(), which > calls drm_framebuffer_init(), which calls kref_init(&fb->refcount).) > > So if intel_pin_and_fence_fb_obj() fails, we do need to unreference and > tear down the fb. Thus, drm_framebuffer_remove() seems right here to me. I wasn't complaining about the fb unref, but the bo unref. > > However, because of your objection I've noticed now that "if (fb)" seems > to be wrong, I think this sho
Re: [Intel-gfx] [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation
On Thu, Oct 15, 2015 at 07:14:35PM +0200, Lukas Wunner wrote: > Hi Ville, > > On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote: > > On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote: > > > From: Tvrtko Ursulin > > > > > > We had two failure modes here: > > > > > > 1. > > > Deadlock in intelfb_alloc failure path where it calls > > > drm_framebuffer_remove, which grabs the struct mutex and intelfb_create > > > (caller of intelfb_alloc) was already holding it. > > > > > > 2. > > > Deadlock in intelfb_create failure path where it calls > > > drm_framebuffer_unreference, which grabs the struct mutex and > > > intelfb_create was already holding it. > > > > > > v2: > > >* Reformat commit msg to 72 chars. (Lukas Wunner) > > >* Add third failure mode. (Lukas Wunner) > > > > > > v3: > > >* On fb alloc failure, unref gem object where it gets refed, > > > fix double unref in separate commit. (Ville Syrjälä) > > > > > > v4: > > >* Lock struct mutex on unref. (Chris Wilson) > > > > > > v5: > > >* Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC, > > > rephrase commit message. (Jani Nicula) > > > > > > Tested-by: Pierre Moreau > > > [MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina] > > > Tested-by: Paul Hordiienko > > > [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina] > > > Tested-by: William Brown > > > [MBP 8,2 2011 intel SNB + amd turks pre-retina] > > > Tested-by: Lukas Wunner > > > [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina] > > > Tested-by: Bruno Bierbaumer > > > [MBP 11,3 2013 intel HSW + nvidia GK107 retina] > > > > > > Signed-off-by: Tvrtko Ursulin > > > Fixes: 60a5ca015ffd ("drm/i915: Add locking around > > > framebuffer_references--") > > > Reported-by: Lukas Wunner > > > [Lukas: Create v3 + v4 + v5 based on Tvrtko's v2] > > > Signed-off-by: Lukas Wunner > > > Cc: Chris Wilson > > > Cc: Ville Syrjälä > > > Cc: Jani Nikula > > > --- > > > drivers/gpu/drm/i915/intel_fbdev.c | 20 > > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > > b/drivers/gpu/drm/i915/intel_fbdev.c > > > index 96476d7..eee3306 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > > @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper, > > > { > > > struct intel_fbdev *ifbdev = > > > container_of(helper, struct intel_fbdev, helper); > > > - struct drm_framebuffer *fb; > > > + struct drm_framebuffer *fb = NULL; > > > struct drm_device *dev = helper->dev; > > > struct drm_mode_fb_cmd2 mode_cmd = {}; > > > struct drm_i915_gem_object *obj; > > > @@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper, > > > mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp, > > > sizes->surface_depth); > > > > > > + mutex_lock(&dev->struct_mutex); > > > + > > > size = mode_cmd.pitches[0] * mode_cmd.height; > > > size = PAGE_ALIGN(size); > > > obj = i915_gem_object_create_stolen(dev, size); > > > @@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper > > > *helper, > > > ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL); > > > if (ret) { > > > DRM_ERROR("failed to pin obj: %d\n", ret); > > > - goto out_fb; > > > + goto out_unref; > > > } > > > > > > + mutex_unlock(&dev->struct_mutex); > > > + > > > ifbdev->fb = to_intel_framebuffer(fb); > > > > > > return 0; > > > > > > -out_fb: > > > - drm_framebuffer_remove(fb); > > > out_unref: > > > drm_gem_object_unreference(&obj->base); > > > > If fb init succeeded it took over the ref, no? So drm_framebuffer_remove() > > will now attempt to unref one too many times. > > > > This taking over refs stuff is confusing. Maybe it would be better if > > everyone just took an extra ref when they stash the obj pointer > > somewhere, and everyone would then always release whatever ref they own > > and no longer need. > > > > > out: > > > + mutex_unlock(&dev->struct_mutex); > > > + if (fb) > > > + drm_framebuffer_remove(fb); > > > return ret; > > > } > > > > > Hm, why do you think we unref one too many times? > > A bit further up in this function we call __intel_framebuffer_create() > which sets the refcount to 1. (It calls intel_framebuffer_init(), which > calls drm_framebuffer_init(), which calls kref_init(&fb->refcount).) > > So if intel_pin_and_fence_fb_obj() fails, we do need to unreference and > tear down the fb. Thus, drm_framebuffer_remove() seems right here to me. > > However, because of your objection I've noticed now that "if (fb)" seems > to be wrong, I think this should be "if (!IS_ERR_OR_NULL(fb))". > > Because if __intel_framebuffer_create() failed, fb will be a PTR_ERR(), > so not null, and we'd call drm_framebuffer_rem
Re: [Intel-gfx] [PATCH v5 1/2] drm/i915: Fix failure paths around initial fbdev allocation
Hi Ville, On Tue, Oct 13, 2015 at 06:04:40PM +0300, Ville Syrjälä wrote: > On Tue, Jun 30, 2015 at 10:06:27AM +0100, Lukas Wunner wrote: > > From: Tvrtko Ursulin > > > > We had two failure modes here: > > > > 1. > > Deadlock in intelfb_alloc failure path where it calls > > drm_framebuffer_remove, which grabs the struct mutex and intelfb_create > > (caller of intelfb_alloc) was already holding it. > > > > 2. > > Deadlock in intelfb_create failure path where it calls > > drm_framebuffer_unreference, which grabs the struct mutex and > > intelfb_create was already holding it. > > > > v2: > >* Reformat commit msg to 72 chars. (Lukas Wunner) > >* Add third failure mode. (Lukas Wunner) > > > > v3: > >* On fb alloc failure, unref gem object where it gets refed, > > fix double unref in separate commit. (Ville Syrjälä) > > > > v4: > >* Lock struct mutex on unref. (Chris Wilson) > > > > v5: > >* Rebase on drm-intel-nightly 2015y-09m-04d-08h-19m-35s UTC, > > rephrase commit message. (Jani Nicula) > > > > Tested-by: Pierre Moreau > > [MBP 5,3 2009 nvidia 9400M + 9600M GT pre-retina] > > Tested-by: Paul Hordiienko > > [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina] > > Tested-by: William Brown > > [MBP 8,2 2011 intel SNB + amd turks pre-retina] > > Tested-by: Lukas Wunner > > [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina] > > Tested-by: Bruno Bierbaumer > > [MBP 11,3 2013 intel HSW + nvidia GK107 retina] > > > > Signed-off-by: Tvrtko Ursulin > > Fixes: 60a5ca015ffd ("drm/i915: Add locking around > > framebuffer_references--") > > Reported-by: Lukas Wunner > > [Lukas: Create v3 + v4 + v5 based on Tvrtko's v2] > > Signed-off-by: Lukas Wunner > > Cc: Chris Wilson > > Cc: Ville Syrjälä > > Cc: Jani Nikula > > --- > > drivers/gpu/drm/i915/intel_fbdev.c | 20 > > 1 file changed, 12 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c > > b/drivers/gpu/drm/i915/intel_fbdev.c > > index 96476d7..eee3306 100644 > > --- a/drivers/gpu/drm/i915/intel_fbdev.c > > +++ b/drivers/gpu/drm/i915/intel_fbdev.c > > @@ -119,7 +119,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper, > > { > > struct intel_fbdev *ifbdev = > > container_of(helper, struct intel_fbdev, helper); > > - struct drm_framebuffer *fb; > > + struct drm_framebuffer *fb = NULL; > > struct drm_device *dev = helper->dev; > > struct drm_mode_fb_cmd2 mode_cmd = {}; > > struct drm_i915_gem_object *obj; > > @@ -137,6 +137,8 @@ static int intelfb_alloc(struct drm_fb_helper *helper, > > mode_cmd.pixel_format = drm_mode_legacy_fb_format(sizes->surface_bpp, > > sizes->surface_depth); > > > > + mutex_lock(&dev->struct_mutex); > > + > > size = mode_cmd.pitches[0] * mode_cmd.height; > > size = PAGE_ALIGN(size); > > obj = i915_gem_object_create_stolen(dev, size); > > @@ -158,18 +160,21 @@ static int intelfb_alloc(struct drm_fb_helper *helper, > > ret = intel_pin_and_fence_fb_obj(NULL, fb, NULL, NULL, NULL); > > if (ret) { > > DRM_ERROR("failed to pin obj: %d\n", ret); > > - goto out_fb; > > + goto out_unref; > > } > > > > + mutex_unlock(&dev->struct_mutex); > > + > > ifbdev->fb = to_intel_framebuffer(fb); > > > > return 0; > > > > -out_fb: > > - drm_framebuffer_remove(fb); > > out_unref: > > drm_gem_object_unreference(&obj->base); > > If fb init succeeded it took over the ref, no? So drm_framebuffer_remove() > will now attempt to unref one too many times. > > This taking over refs stuff is confusing. Maybe it would be better if > everyone just took an extra ref when they stash the obj pointer > somewhere, and everyone would then always release whatever ref they own > and no longer need. > > > out: > > + mutex_unlock(&dev->struct_mutex); > > + if (fb) > > + drm_framebuffer_remove(fb); > > return ret; > > } > > Hm, why do you think we unref one too many times? A bit further up in this function we call __intel_framebuffer_create() which sets the refcount to 1. (It calls intel_framebuffer_init(), which calls drm_framebuffer_init(), which calls kref_init(&fb->refcount).) So if intel_pin_and_fence_fb_obj() fails, we do need to unreference and tear down the fb. Thus, drm_framebuffer_remove() seems right here to me. However, because of your objection I've noticed now that "if (fb)" seems to be wrong, I think this should be "if (!IS_ERR_OR_NULL(fb))". Because if __intel_framebuffer_create() failed, fb will be a PTR_ERR(), so not null, and we'd call drm_framebuffer_remove() on this. Is that what you meant? > > @@ -187,8 +192,6 @@ static int intelfb_create(struct drm_fb_helper *helper, > > int size, ret; > > bool prealloc = false; > > > > - mutex_lock(&dev->struct_mutex); > > - > > if (intel_fb && > > (sizes->
Re: [Intel-gfx] [PATCH] drm/i915: Kill the leftover RMW from ivb_sprite_disable()
On Thu, Oct 15, 2015 at 05:04:04PM +0300, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > We still had one lingering RMW in ivb_sprite_disable(), all the other > RMWs were killed off from the sprite code some time ago. Kill the > straggler too. > > Signed-off-by: Ville Syrjälä Queued for -next, thanks for the patch. -Daniel > --- > drivers/gpu/drm/i915/intel_sprite.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index 7f0911e..a84cbf42 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -610,7 +610,7 @@ ivb_disable_plane(struct drm_plane *plane, struct > drm_crtc *crtc) > struct intel_plane *intel_plane = to_intel_plane(plane); > int pipe = intel_plane->pipe; > > - I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE); > + I915_WRITE(SPRCTL(pipe), 0); > /* Can't leave the scaler enabled... */ > if (intel_plane->can_scale) > I915_WRITE(SPRSCALE(pipe), 0); > -- > 2.4.9 > > ___ > 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 i-g-t v2] tests/core_prop_blob: Fix core_prop_blob for android
On 14 October 2015 at 17:23, Derek Morton wrote: > core_prop_blob was using ioctls not in the android kernel. Added a > igt_require_propblob() function and local defines/structures so the > test will compile and skip on kernels where the feature is unsupported. > > v2: moved igt_require_propblob() to core_prop_blob.c (Daniel Vetter) > Moved gem_blt.c to a seperate patch (Thomas Wood) > > Signed-off-by: Derek Morton > --- > tests/core_prop_blob.c | 71 > -- > 1 file changed, 52 insertions(+), 19 deletions(-) > > diff --git a/tests/core_prop_blob.c b/tests/core_prop_blob.c > index d704158..4dc6d7d 100644 > --- a/tests/core_prop_blob.c > +++ b/tests/core_prop_blob.c > @@ -25,18 +25,35 @@ > * Daniel Stone > */ > > +#include "igt.h" > #include > #include > #include > #include > > -#include "drmtest.h" > -#include "igt_debugfs.h" > -#include "igt_kms.h" > -#include "igt_aux.h" > - > IGT_TEST_DESCRIPTION("Tests behaviour of mass-data 'blob' properties."); > > +struct local_drm_mode_get_blob { > + uint32_t blob_id; > + uint32_t length; > + uint64_t data; > +}; > +struct local_drm_mode_create_blob { > + uint64_t data; > + uint32_t length; > + uint32_t blob_id; > +}; > +struct local_drm_mode_destroy_blob { > + uint32_t blob_id; > +}; > + > +#define LOCAL_DRM_IOCTL_MODE_GETPROPBLOB DRM_IOWR(0xAC, \ > + struct > local_drm_mode_get_blob) > +#define LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOBDRM_IOWR(0xBD, \ > + struct > local_drm_mode_create_blob) > +#define LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB DRM_IOWR(0xBE, \ > + struct > local_drm_mode_destroy_blob) > + > static const struct drm_mode_modeinfo test_mode_valid = { > .clock = 1234, > .hdisplay = 640, > @@ -61,22 +78,35 @@ static const struct drm_mode_modeinfo test_mode_valid = { > return errno; \ > } > > +static void igt_require_propblob(int fd) > +{ > + struct local_drm_mode_create_blob c; > + struct local_drm_mode_destroy_blob d; > + uint32_t blob_data; > + c.data = &blob_data; Thanks for the patch. I fixed the compiler warning here (integer from pointer) and pushed along with the gem_blt.c patch. > + c.length = sizeof(blob_data); > + > + igt_require(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOB, &c) == > 0); > + d.blob_id = c.blob_id; > + igt_require(drmIoctl(fd, LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB, &d) == > 0); > +} > + > static int > validate_prop(int fd, uint32_t prop_id) > { > - struct drm_mode_get_blob get; > + struct local_drm_mode_get_blob get; > struct drm_mode_modeinfo ret_mode; > > get.blob_id = prop_id; > get.length = 0; > get.data = (uintptr_t) 0; > - ioctl_or_ret_errno(fd, DRM_IOCTL_MODE_GETPROPBLOB, &get); > + ioctl_or_ret_errno(fd, LOCAL_DRM_IOCTL_MODE_GETPROPBLOB, &get); > > if (get.length != sizeof(test_mode_valid)) > return ENOMEM; > > get.data = (uintptr_t) &ret_mode; > - ioctl_or_ret_errno(fd, DRM_IOCTL_MODE_GETPROPBLOB, &get); > + ioctl_or_ret_errno(fd, LOCAL_DRM_IOCTL_MODE_GETPROPBLOB, &get); > > if (memcmp(&ret_mode, &test_mode_valid, sizeof(test_mode_valid)) != 0) > return EINVAL; > @@ -87,12 +117,12 @@ validate_prop(int fd, uint32_t prop_id) > static uint32_t > create_prop(int fd) > { > - struct drm_mode_create_blob create; > + struct local_drm_mode_create_blob create; > > create.length = sizeof(test_mode_valid); > create.data = (uintptr_t) &test_mode_valid; > > - do_ioctl(fd, DRM_IOCTL_MODE_CREATEPROPBLOB, &create); > + do_ioctl(fd, LOCAL_DRM_IOCTL_MODE_CREATEPROPBLOB, &create); > igt_assert_neq_u32(create.blob_id, 0); > > return create.blob_id; > @@ -101,10 +131,10 @@ create_prop(int fd) > static int > destroy_prop(int fd, uint32_t prop_id) > { > - struct drm_mode_destroy_blob destroy; > + struct local_drm_mode_destroy_blob destroy; > > destroy.blob_id = prop_id; > - ioctl_or_ret_errno(fd, DRM_IOCTL_MODE_DESTROYPROPBLOB, &destroy); > + ioctl_or_ret_errno(fd, LOCAL_DRM_IOCTL_MODE_DESTROYPROPBLOB, > &destroy); > > return 0; > } > @@ -112,8 +142,8 @@ destroy_prop(int fd, uint32_t prop_id) > static void > test_validate(int fd) > { > - struct drm_mode_create_blob create; > - struct drm_mode_get_blob get; > + struct local_drm_mode_create_blob create; > + struct local_drm_mode_get_blob get; > char too_small[32]; > uint32_t prop_id; > > @@ -122,24 +152,24 @@ test_validate(int fd) > /* Outlandish size. */ > create.length = 0x1; > create.data = (uintptr_t) &too_small; > - do_ioctl_err(fd, DRM_IOCTL_MODE_CREA
[Intel-gfx] [PATCH] drm/i915: Kill the leftover RMW from ivb_sprite_disable()
From: Ville Syrjälä We still had one lingering RMW in ivb_sprite_disable(), all the other RMWs were killed off from the sprite code some time ago. Kill the straggler too. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_sprite.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c index 7f0911e..a84cbf42 100644 --- a/drivers/gpu/drm/i915/intel_sprite.c +++ b/drivers/gpu/drm/i915/intel_sprite.c @@ -610,7 +610,7 @@ ivb_disable_plane(struct drm_plane *plane, struct drm_crtc *crtc) struct intel_plane *intel_plane = to_intel_plane(plane); int pipe = intel_plane->pipe; - I915_WRITE(SPRCTL(pipe), I915_READ(SPRCTL(pipe)) & ~SPRITE_ENABLE); + I915_WRITE(SPRCTL(pipe), 0); /* Can't leave the scaler enabled... */ if (intel_plane->can_scale) I915_WRITE(SPRSCALE(pipe), 0); -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: skl_update_scaler() wants a rotation bitmask instead of bit number
From: Ville Syrjälä Pass BIT(DRM_ROTATE_0) instead of DRM_ROTATE_0 to skl_update_scaler(). The former is a mask, the latter just the bit number. Fortunately the only thing skl_update_scaler() does with the rotation is check if it's 90/270 degrees or not, and so in this case it would still do the right thing. Cc: Chandra Konduru Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 7498c9d..02316d0 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4673,7 +4673,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state) intel_crtc->base.base.id, intel_crtc->pipe, SKL_CRTC_INDEX); return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX, - &state->scaler_state.scaler_id, DRM_ROTATE_0, + &state->scaler_state.scaler_id, BIT(DRM_ROTATE_0), state->pipe_src_w, state->pipe_src_h, adjusted_mode->crtc_hdisplay, adjusted_mode->crtc_vdisplay); } -- 2.4.9 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [drm-intel:topic/drm-misc 35/37] drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c:143:21: error: unused variable 'dev'
tree: git://anongit.freedesktop.org/drm-intel topic/drm-misc head: 1032f72564813b1dd00184897d8291ad71676a56 commit: 9da239e3b9538cd5dd883d8258a7c3e2e0499e13 [35/37] drm/gem: Drop struct_mutex requirement from drm_gem_mmap_obj config: arm-allmodconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git checkout 9da239e3b9538cd5dd883d8258a7c3e2e0499e13 # save the attached .config to linux build tree make.cross ARCH=arm All error/warnings (new ones prefixed by >>): drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c: In function 'omap_gem_dmabuf_mmap': >> drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c:143:21: error: unused variable >> 'dev' [-Werror=unused-variable] struct drm_device *dev = obj->dev; ^ cc1: all warnings being treated as errors -- drivers/gpu/drm/rockchip/rockchip_drm_gem.c: In function 'rockchip_gem_mmap_buf': >> drivers/gpu/drm/rockchip/rockchip_drm_gem.c:82:21: warning: unused variable >> 'drm' [-Wunused-variable] struct drm_device *drm = obj->dev; ^ vim +/dev +143 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c 6ad11bc3 drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-04-10 137 } 6ad11bc3 drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-04-10 138 8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17 139 static int omap_gem_dmabuf_mmap(struct dma_buf *buffer, 8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17 140 struct vm_area_struct *vma) 8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17 141 { 8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17 142 struct drm_gem_object *obj = buffer->priv; 4368dd84 drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c YoungJun Cho 2013-06-27 @143 struct drm_device *dev = obj->dev; 8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17 144 int ret = 0; 8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17 145 8b6b569e drivers/staging/omapdrm/omap_gem_dmabuf.c Rob Clark2012-05-17 146 if (WARN_ON(!obj->filp)) :: The code at line 143 was first introduced by commit :: 4368dd846d067ed7d11281050e7676ae614d6053 drm/gem: add mutex lock when using drm_gem_mmap_obj :: TO: YoungJun Cho :: CC: Dave Airlie --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Create crtc_clock_get function pointers
On Thu, Oct 15, 2015 at 06:52:48PM +0530, Vandana Kannan wrote: > There are separate functions i9xx_crtc_clock_get(), vlv_crtc_clock_get(), > chv_crtc_clock_get(). instead of calling these using if-else, making > func pointers. This will also be useful going forward when the implementation > for BXT is done. > > Signed-off-by: Vandana Kannan I general I much prefer direct callers if there's only one caller ever. The per-platform modeset code is already really hard to follow, no need to insert even more indirection imo. -Daniel > --- > drivers/gpu/drm/i915/i915_drv.h | 2 ++ > drivers/gpu/drm/i915/intel_display.c | 20 > 2 files changed, 14 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 8afda45..773f507 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -607,6 +607,8 @@ struct intel_limit; > struct dpll; > > struct drm_i915_display_funcs { > + void (*crtc_clock_get)(struct intel_crtc *crtc, > + struct intel_crtc_state *pipe_config); > int (*get_display_clock_speed)(struct drm_device *dev); > int (*get_fifo_size)(struct drm_device *dev, int plane); > /** > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index c28fb6a..d98385e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -8130,12 +8130,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc > *crtc, >DPLL_PORTB_READY_MASK); > } > > - if (IS_CHERRYVIEW(dev)) > - chv_crtc_clock_get(crtc, pipe_config); > - else if (IS_VALLEYVIEW(dev)) > - vlv_crtc_clock_get(crtc, pipe_config); > - else > - i9xx_crtc_clock_get(crtc, pipe_config); > + dev_priv->display.crtc_clock_get(crtc, pipe_config); > > /* >* Normally the dotclock is filled in by the encoder .get_config() > @@ -10579,9 +10574,10 @@ static void ironlake_pch_clock_get(struct intel_crtc > *crtc, > struct intel_crtc_state *pipe_config) > { > struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > > /* read out port_clock from the DPLL */ > - i9xx_crtc_clock_get(crtc, pipe_config); > + dev_priv->display.crtc_clock_get(crtc, &pipe_config); > > /* >* This value does not include pixel_multiplier. > @@ -10625,7 +10621,7 @@ struct drm_display_mode *intel_crtc_mode_get(struct > drm_device *dev, > pipe_config.dpll_hw_state.dpll = I915_READ(DPLL(pipe)); > pipe_config.dpll_hw_state.fp0 = I915_READ(FP0(pipe)); > pipe_config.dpll_hw_state.fp1 = I915_READ(FP1(pipe)); > - i9xx_crtc_clock_get(intel_crtc, &pipe_config); > + dev_priv->display.crtc_clock_get(intel_crtc, &pipe_config); > > mode->clock = pipe_config.port_clock / pipe_config.pixel_multiplier; > mode->hdisplay = (htot & 0x) + 1; > @@ -14413,6 +14409,7 @@ static void intel_init_display(struct drm_device *dev) > dev_priv->display.crtc_disable = haswell_crtc_disable; > dev_priv->display.update_primary_plane = > skylake_update_primary_plane; > + dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get; > } else if (HAS_DDI(dev)) { > dev_priv->display.get_pipe_config = haswell_get_pipe_config; > dev_priv->display.get_initial_plane_config = > @@ -14423,6 +14420,7 @@ static void intel_init_display(struct drm_device *dev) > dev_priv->display.crtc_disable = haswell_crtc_disable; > dev_priv->display.update_primary_plane = > ironlake_update_primary_plane; > + dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get; > } else if (HAS_PCH_SPLIT(dev)) { > dev_priv->display.get_pipe_config = ironlake_get_pipe_config; > dev_priv->display.get_initial_plane_config = > @@ -14433,6 +14431,7 @@ static void intel_init_display(struct drm_device *dev) > dev_priv->display.crtc_disable = ironlake_crtc_disable; > dev_priv->display.update_primary_plane = > ironlake_update_primary_plane; > + dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get; > } else if (IS_VALLEYVIEW(dev)) { > dev_priv->display.get_pipe_config = i9xx_get_pipe_config; > dev_priv->display.get_initial_plane_config = > @@ -14442,6 +14441,10 @@ static void intel_init_display(struct drm_device > *dev) > dev_priv->display.crtc_disable = i9xx_crtc_disable; > dev_priv->display.update_primary_plane = > i9xx_update_primary_plane; > + if (IS_CHERRYVIEW(dev)) > + dev_priv->display.crtc_clock_get
Re: [Intel-gfx] [PATCH 2/2] drm/i915/skl+: Enable pipe CSC on cursor planes. (v2)
On Thu, Oct 15, 2015 at 03:49:44PM +0300, Ville Syrjälä wrote: > On Mon, Aug 31, 2015 at 02:03:30PM -0700, Bob Paauwe wrote: > > Extend this to SKL and BXT as it's needed for these platforms as well. > > > > v2: Change if condition to HAS_DDI() instead of listing each platform > > Signed-off-by: Bob Paauwe > > --- > > drivers/gpu/drm/i915/intel_display.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 88f9764..ba180f6 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -10001,7 +10001,7 @@ static void i9xx_update_cursor(struct drm_crtc > > *crtc, u32 base) > > } > > cntl |= pipe << 28; /* Connect to correct pipe */ > > > > - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > > + if (HAS_DDI(dev)) > > Yeah, while cursors themselves don't change between non-DDI and DDI > platforms, the reason for this stuff is the fact that that we use > the pipe csc for HDMI color range mangling on DDI platforms, since > they no longer have any other pipe/port controls for it. > > So makese sense to check for DDI here too I think: > Reviewed-by: Ville Syrjälä Both patches applied, thanks. -Daniel > > > cntl |= CURSOR_PIPE_CSC_ENABLE; > > } > > > > -- > > 2.1.0 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > ___ > 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
[Intel-gfx] [PATCH 1/2] drm/i915: Create crtc_clock_get function pointers
There are separate functions i9xx_crtc_clock_get(), vlv_crtc_clock_get(), chv_crtc_clock_get(). instead of calling these using if-else, making func pointers. This will also be useful going forward when the implementation for BXT is done. Signed-off-by: Vandana Kannan --- drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_display.c | 20 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8afda45..773f507 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -607,6 +607,8 @@ struct intel_limit; struct dpll; struct drm_i915_display_funcs { + void (*crtc_clock_get)(struct intel_crtc *crtc, + struct intel_crtc_state *pipe_config); int (*get_display_clock_speed)(struct drm_device *dev); int (*get_fifo_size)(struct drm_device *dev, int plane); /** diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index c28fb6a..d98385e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8130,12 +8130,7 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, DPLL_PORTB_READY_MASK); } - if (IS_CHERRYVIEW(dev)) - chv_crtc_clock_get(crtc, pipe_config); - else if (IS_VALLEYVIEW(dev)) - vlv_crtc_clock_get(crtc, pipe_config); - else - i9xx_crtc_clock_get(crtc, pipe_config); + dev_priv->display.crtc_clock_get(crtc, pipe_config); /* * Normally the dotclock is filled in by the encoder .get_config() @@ -10579,9 +10574,10 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config) { struct drm_device *dev = crtc->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; /* read out port_clock from the DPLL */ - i9xx_crtc_clock_get(crtc, pipe_config); + dev_priv->display.crtc_clock_get(crtc, &pipe_config); /* * This value does not include pixel_multiplier. @@ -10625,7 +10621,7 @@ struct drm_display_mode *intel_crtc_mode_get(struct drm_device *dev, pipe_config.dpll_hw_state.dpll = I915_READ(DPLL(pipe)); pipe_config.dpll_hw_state.fp0 = I915_READ(FP0(pipe)); pipe_config.dpll_hw_state.fp1 = I915_READ(FP1(pipe)); - i9xx_crtc_clock_get(intel_crtc, &pipe_config); + dev_priv->display.crtc_clock_get(intel_crtc, &pipe_config); mode->clock = pipe_config.port_clock / pipe_config.pixel_multiplier; mode->hdisplay = (htot & 0x) + 1; @@ -14413,6 +14409,7 @@ static void intel_init_display(struct drm_device *dev) dev_priv->display.crtc_disable = haswell_crtc_disable; dev_priv->display.update_primary_plane = skylake_update_primary_plane; + dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get; } else if (HAS_DDI(dev)) { dev_priv->display.get_pipe_config = haswell_get_pipe_config; dev_priv->display.get_initial_plane_config = @@ -14423,6 +14420,7 @@ static void intel_init_display(struct drm_device *dev) dev_priv->display.crtc_disable = haswell_crtc_disable; dev_priv->display.update_primary_plane = ironlake_update_primary_plane; + dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get; } else if (HAS_PCH_SPLIT(dev)) { dev_priv->display.get_pipe_config = ironlake_get_pipe_config; dev_priv->display.get_initial_plane_config = @@ -14433,6 +14431,7 @@ static void intel_init_display(struct drm_device *dev) dev_priv->display.crtc_disable = ironlake_crtc_disable; dev_priv->display.update_primary_plane = ironlake_update_primary_plane; + dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get; } else if (IS_VALLEYVIEW(dev)) { dev_priv->display.get_pipe_config = i9xx_get_pipe_config; dev_priv->display.get_initial_plane_config = @@ -14442,6 +14441,10 @@ static void intel_init_display(struct drm_device *dev) dev_priv->display.crtc_disable = i9xx_crtc_disable; dev_priv->display.update_primary_plane = i9xx_update_primary_plane; + if (IS_CHERRYVIEW(dev)) + dev_priv->display.crtc_clock_get = chv_crtc_clock_get; + else + dev_priv->display.crtc_clock_get = vlv_crtc_clock_get; } else { dev_priv->display.get_pipe_config = i9xx_get_pipe_config; dev_priv->display.get_initial_plane_config = @@ -14451,6 +14454,7 @@ static void intel_init_display(struct drm
[Intel-gfx] [PATCH 2/2] drm/i915: Add crtc_clock_get for hsw, skl, bxt
Reusing the ddi_clock_get functions for hsw, skl, bxt and creating a common crtc_clock_get function. for BXT, there is a difference in clock between DSI and DDI. Taking care of this as well. Signed-off-by: Vandana Kannan --- drivers/gpu/drm/i915/intel_display.c | 43 +--- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index d98385e..c9fcadd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -34,6 +34,7 @@ #include #include #include "intel_drv.h" +#include "intel_dsi.h" #include #include "i915_drv.h" #include "i915_trace.h" @@ -112,6 +113,15 @@ static void skl_init_scalers(struct drm_device *dev, struct intel_crtc *intel_cr struct intel_crtc_state *crtc_state); static int i9xx_get_refclk(const struct intel_crtc_state *crtc_state, int num_connectors); +static void bxt_get_ddi_pll(struct drm_i915_private *dev_priv, + enum port port, + struct intel_crtc_state *pipe_config); +static void skylake_get_ddi_pll(struct drm_i915_private *dev_priv, + enum port port, + struct intel_crtc_state *pipe_config); +static void haswell_get_ddi_pll(struct drm_i915_private *dev_priv, + enum port port, + struct intel_crtc_state *pipe_config); static void skylake_pfit_enable(struct intel_crtc *crtc); static void ironlake_pfit_disable(struct intel_crtc *crtc, bool force); static void ironlake_pfit_enable(struct intel_crtc *crtc); @@ -8050,6 +8060,33 @@ static void chv_crtc_clock_get(struct intel_crtc *crtc, pipe_config->port_clock = chv_calc_dpll_params(refclk, &clock); } +static void haswell_crtc_clock_get(struct intel_crtc *crtc, + struct intel_crtc_state *pipe_config) +{ + struct drm_device *dev = crtc->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_encoder *encoder = NULL; + enum port port; + bool is_dsi = intel_pipe_has_type(crtc, INTEL_OUTPUT_DSI); + + for_each_encoder_on_crtc(dev, &crtc->base, encoder) { + port = intel_ddi_get_encoder_port(encoder); + if (IS_BROXTON(dev) && is_dsi) { + pipe_config->port_clock = bxt_get_dsi_pclk(encoder, + pipe_config->pipe_bpp); + break; + } + if (IS_SKYLAKE(dev)) + skylake_get_ddi_pll(dev_priv, port, pipe_config); + else if (IS_BROXTON(dev)) + bxt_get_ddi_pll(dev_priv, port, pipe_config); + else + haswell_get_ddi_pll(dev_priv, port, pipe_config); + + intel_ddi_clock_get(encoder, pipe_config); + } +} + static bool i9xx_get_pipe_config(struct intel_crtc *crtc, struct intel_crtc_state *pipe_config) { @@ -10577,7 +10614,7 @@ static void ironlake_pch_clock_get(struct intel_crtc *crtc, struct drm_i915_private *dev_priv = dev->dev_private; /* read out port_clock from the DPLL */ - dev_priv->display.crtc_clock_get(crtc, &pipe_config); + dev_priv->display.crtc_clock_get(crtc, pipe_config); /* * This value does not include pixel_multiplier. @@ -14409,7 +14446,7 @@ static void intel_init_display(struct drm_device *dev) dev_priv->display.crtc_disable = haswell_crtc_disable; dev_priv->display.update_primary_plane = skylake_update_primary_plane; - dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get; + dev_priv->display.crtc_clock_get = haswell_crtc_clock_get; } else if (HAS_DDI(dev)) { dev_priv->display.get_pipe_config = haswell_get_pipe_config; dev_priv->display.get_initial_plane_config = @@ -14420,7 +14457,7 @@ static void intel_init_display(struct drm_device *dev) dev_priv->display.crtc_disable = haswell_crtc_disable; dev_priv->display.update_primary_plane = ironlake_update_primary_plane; - dev_priv->display.crtc_clock_get = i9xx_crtc_clock_get; + dev_priv->display.crtc_clock_get = haswell_crtc_clock_get; } else if (HAS_PCH_SPLIT(dev)) { dev_priv->display.get_pipe_config = ironlake_get_pipe_config; dev_priv->display.get_initial_plane_config = -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/2] Introducing crtc_clock_get()
The implementation of crtc_clock_get() will not work for BXT. The 2 patches in this series, cleans up the code related to crtc_clock_get() and includes implementation for BXT (DSI and otherwise). A patch to correct crtc_mode_get() for BXT DSI will follow shortly. Vandana Kannan (2): drm/i915: Create crtc_clock_get function pointers drm/i915: Add crtc_clock_get for hsw, skl, bxt drivers/gpu/drm/i915/i915_drv.h | 2 ++ drivers/gpu/drm/i915/intel_display.c | 57 +++- 2 files changed, 51 insertions(+), 8 deletions(-) -- 1.9.1 ___ 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+: Enable pipe CSC on cursor planes. (v2)
On Mon, Aug 31, 2015 at 02:03:30PM -0700, Bob Paauwe wrote: > Extend this to SKL and BXT as it's needed for these platforms as well. > > v2: Change if condition to HAS_DDI() instead of listing each platform > Signed-off-by: Bob Paauwe > --- > drivers/gpu/drm/i915/intel_display.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 88f9764..ba180f6 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -10001,7 +10001,7 @@ static void i9xx_update_cursor(struct drm_crtc *crtc, > u32 base) > } > cntl |= pipe << 28; /* Connect to correct pipe */ > > - if (IS_HASWELL(dev) || IS_BROADWELL(dev)) > + if (HAS_DDI(dev)) Yeah, while cursors themselves don't change between non-DDI and DDI platforms, the reason for this stuff is the fact that that we use the pipe csc for HDMI color range mangling on DDI platforms, since they no longer have any other pipe/port controls for it. So makese sense to check for DDI here too I think: Reviewed-by: Ville Syrjälä > cntl |= CURSOR_PIPE_CSC_ENABLE; > } > > -- > 2.1.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [drm-intel:topic/drm-misc 35/38] drivers/gpu/drm/drm_gem_cma_helper.c:484:21: warning: unused variable 'dev'
tree: git://anongit.freedesktop.org/drm-intel topic/drm-misc head: 0f3609e5b5c53ed52b8e82993cbda72806ef9c69 commit: 9da239e3b9538cd5dd883d8258a7c3e2e0499e13 [35/38] drm/gem: Drop struct_mutex requirement from drm_gem_mmap_obj config: arm-at91_dt_defconfig (attached as .config) reproduce: wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross git checkout 9da239e3b9538cd5dd883d8258a7c3e2e0499e13 # save the attached .config to linux build tree make.cross ARCH=arm All warnings (new ones prefixed by >>): drivers/gpu/drm/drm_gem_cma_helper.c: In function 'drm_gem_cma_prime_mmap': >> drivers/gpu/drm/drm_gem_cma_helper.c:484:21: warning: unused variable 'dev' >> [-Wunused-variable] struct drm_device *dev = obj->dev; ^ vim +/dev +484 drivers/gpu/drm/drm_gem_cma_helper.c d7883f87 Thierry Reding 2014-11-03 468 /** d7883f87 Thierry Reding 2014-11-03 469 * drm_gem_cma_prime_mmap - memory-map an exported CMA GEM object d7883f87 Thierry Reding 2014-11-03 470 * @obj: GEM object d7883f87 Thierry Reding 2014-11-03 471 * @vma: VMA for the area to be mapped d7883f87 Thierry Reding 2014-11-03 472 * d7883f87 Thierry Reding 2014-11-03 473 * This function maps a buffer imported via DRM PRIME into a userspace d7883f87 Thierry Reding 2014-11-03 474 * process's address space. Drivers that use the CMA helpers should set this d7883f87 Thierry Reding 2014-11-03 475 * as their DRM driver's ->gem_prime_mmap() callback. d7883f87 Thierry Reding 2014-11-03 476 * d7883f87 Thierry Reding 2014-11-03 477 * Returns: d7883f87 Thierry Reding 2014-11-03 478 * 0 on success or a negative error code on failure. d7883f87 Thierry Reding 2014-11-03 479 */ 78467dc5 Joonyoung Shim 2013-06-28 480 int drm_gem_cma_prime_mmap(struct drm_gem_object *obj, 78467dc5 Joonyoung Shim 2013-06-28 481struct vm_area_struct *vma) 78467dc5 Joonyoung Shim 2013-06-28 482 { 78467dc5 Joonyoung Shim 2013-06-28 483 struct drm_gem_cma_object *cma_obj; 78467dc5 Joonyoung Shim 2013-06-28 @484 struct drm_device *dev = obj->dev; 78467dc5 Joonyoung Shim 2013-06-28 485 int ret; 78467dc5 Joonyoung Shim 2013-06-28 486 78467dc5 Joonyoung Shim 2013-06-28 487 ret = drm_gem_mmap_obj(obj, obj->size, vma); 78467dc5 Joonyoung Shim 2013-06-28 488 if (ret < 0) 78467dc5 Joonyoung Shim 2013-06-28 489 return ret; 78467dc5 Joonyoung Shim 2013-06-28 490 78467dc5 Joonyoung Shim 2013-06-28 491 cma_obj = to_drm_gem_cma_obj(obj); 78467dc5 Joonyoung Shim 2013-06-28 492 return drm_gem_cma_mmap_obj(cma_obj, vma); :: The code at line 484 was first introduced by commit :: 78467dc5f70fb9bee4a32c0c3714c99b0b5465c7 drm/cma: add low-level hook functions to use prime helpers :: TO: Joonyoung Shim :: CC: Dave Airlie --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/skl: Enable pipe gamma for sprite planes.
On Thu, Aug 27, 2015 at 01:46:30PM -0700, Bob Paauwe wrote: > Since SKL has universal planes, we should configure the sprite planes > and the primary plane the same. For the primary plane we do enable > the pipe gamma on the plane so do the same for the non-primary planes. > > Without this, the pipe CRC values will be different for something > displayed on the primary plane and something displayed on a sprite > plane when the ARGB format is used. > > Signed-off-by: Bob Paauwe Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/i915/intel_sprite.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/gpu/drm/i915/intel_sprite.c > b/drivers/gpu/drm/i915/intel_sprite.c > index 9d8af2f..048d397 100644 > --- a/drivers/gpu/drm/i915/intel_sprite.c > +++ b/drivers/gpu/drm/i915/intel_sprite.c > @@ -189,6 +189,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct > drm_crtc *crtc, > int scaler_id; > > plane_ctl = PLANE_CTL_ENABLE | > + PLANE_CTL_PIPE_GAMMA_ENABLE | > PLANE_CTL_PIPE_CSC_ENABLE; > > plane_ctl |= skl_plane_ctl_format(fb->pixel_format); > -- > 2.1.0 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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 v2] drm/i915/hsw: keep gamma and CSC enabled for primary plane disable
On Thu, Oct 15, 2015 at 02:31:09PM +0200, Daniel Vetter wrote: > > On Thu, Oct 15, 2015 at 12:51 AM, Kevin Strasser > wrote: > > On HSW the crc differs between black and disabled primary planes, causing an > > assert to fail in the kms_universal_plane test. It seems that gamma > > correction > > and color space conversion are causing the black primary plane case to > > result in > > a brighter color than the disabled primary plane case. > > > > Keep gamma and CSC bits enabled for plane disable path on HSW. > > > > v2: Avoid use of RMW > > Keep path unchanged for non-HSW users > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > > Testcase: igt/kms_universal_plane/universal-plane-pipe-A-functional > > Signed-off-by: Kevin Strasser > > With big discussions please add everyone who participated (Ville, Chris, > Jani, me) to the cc list of the sob section of the patch when doing a new > revisions. > > Bob did something eerily similarly a while ago to fix crc failures: > > http://lists.freedesktop.org/archives/intel-gfx/2015-August/074657.html > http://lists.freedesktop.org/archives/intel-gfx/2015-August/074828.html > > Unfortunately those patches did go nowhere :( Related? Those seem to be for active plane cases. I'll go review them... -- 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] drm/i915: restore ggtt double-bind avoidance
On Thu, Oct 15, 2015 at 02:23:01PM +0200, Daniel Vetter wrote: > This was accidentally lost in > > commit 75d04a3773ecee617847de963ae4195d6aa74c28 > Author: Mika Kuoppala > Date: Tue Apr 28 17:56:17 2015 +0300 > > drm/i915/gtt: Allocate va range only if vma is not bound > > While at it implement an improved version suggested by Chris which > avoids the double-bind irrespective of what type of bind is done > first. > > Note that this exact bug was already addressed in > > commit d0e30adc42d979e4adc36b6c112b57337423b70c > Author: Chris Wilson > Date: Wed Jul 29 20:02:48 2015 +0100 > > drm/i915: Mark PIN_USER binding as GLOBAL_BIND without the aliasing ppgtt > > but the problem is still that originally in > > commit 0875546c5318c85c13d07014af5350e9000bc9e9 > Author: Daniel Vetter > Date: Mon Apr 20 09:04:05 2015 -0700 > > drm/i915: Fix up the vma aliasing ppgtt binding > > if forgotten to take into account there case where we have a > GLOBAL_BIND before a LOCAL_BIND. This patch here fixes that. > > v2: Pimp commit message and revert the partial fix. > > v3: Split into two functions to specialize on aliasing_ppgtt y/n. > > v4: WARN_ON for paranoia in the init sequence, since the ggtt probe > and aliasing ppgtt setup are far apart. > > Cc: Chris Wilson > Cc: Michel Thierry > Cc: Mika Kuoppala > Link: > http://mid.gmane.org/1444894651-5332-1-git-send-email-daniel.vet...@ffwll.ch > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 49 > - > 1 file changed, 37 insertions(+), 12 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > b/drivers/gpu/drm/i915/i915_gem_gtt.c > index ff1004876f56..419687135f41 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -2502,6 +2502,39 @@ static int ggtt_bind_vma(struct i915_vma *vma, >enum i915_cache_level cache_level, >u32 flags) > { > + struct drm_i915_gem_object *obj = vma->obj; > + struct sg_table *pages = obj->pages; Time to drop this as it is now correctly set later from the ggtt view. As we then only use obj in one place, lets use it directly. (Having read-only and read-write vma is a posibility to consider, too bad the read-only flag didn't come into greater use by hw.) > + 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; > + > + Spare line we can trade in. > + vma->vm->insert_entries(vma->vm, pages, And since we only use pages here we can just use vma->gtt_view.pages instead. > + vma->node.start, > + cache_level, pte_flags); > + > + /* > + * Without aliasing PPGTT there's no difference between > + * GLOBAL/LOCAL_BIND, it's all the same ptes. Hence unconditionally > + * upgrade to both bound if we bind either to avoid double-binding. > + */ > + vma->bound |= GLOBAL_BIND | LOCAL_BIND; > + > + return 0; > +} > + > +static int aliasing_gtt_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; Could make similar contractions here but meh. Stylistic nitpicks aside, Reviewed-by: Chris Wilson -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 v2] drm/i915/hsw: keep gamma and CSC enabled for primary plane disable
On Thu, Oct 15, 2015 at 12:51 AM, Kevin Strasser wrote: > On HSW the crc differs between black and disabled primary planes, causing an > assert to fail in the kms_universal_plane test. It seems that gamma correction > and color space conversion are causing the black primary plane case to result > in > a brighter color than the disabled primary plane case. > > Keep gamma and CSC bits enabled for plane disable path on HSW. > > v2: Avoid use of RMW > Keep path unchanged for non-HSW users > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=89331 > Testcase: igt/kms_universal_plane/universal-plane-pipe-A-functional > Signed-off-by: Kevin Strasser With big discussions please add everyone who participated (Ville, Chris, Jani, me) to the cc list of the sob section of the patch when doing a new revisions. Bob did something eerily similarly a while ago to fix crc failures: http://lists.freedesktop.org/archives/intel-gfx/2015-August/074657.html http://lists.freedesktop.org/archives/intel-gfx/2015-August/074828.html Unfortunately those patches did go nowhere :( Related? -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/22] drm/i915: Don't treat differently sized rotated views as equal
On Thu, Oct 15, 2015 at 03:06:11PM +0300, Ville Syrjälä wrote: > On Thu, Oct 15, 2015 at 02:02:24PM +0200, Daniel Vetter wrote: > > On Thu, Oct 15, 2015 at 12:18:41PM +0100, Tvrtko Ursulin wrote: > > > > > > On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote: > > > >From: Ville Syrjälä > > > > > > > >In case we have multiple different rotated views into the same object, > > > >each one may need its own vma due to being of different sizes. So don't > > > >treat all rotated views as equal. > > > > > > > >Signed-off-by: Ville Syrjälä > > > >--- > > > > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ > > > > 1 file changed, 2 insertions(+) > > > > > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > > > >b/drivers/gpu/drm/i915/i915_gem_gtt.h > > > >index caa182f..68de734 100644 > > > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > > > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > > > >@@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > > > > > > > if (a->type != b->type) > > > > return false; > > > >+if (a->type == I915_GGTT_VIEW_ROTATED) > > > >+return !memcmp(&a->rotated, &b->rotated, > > > >sizeof(a->rotated)); > > > > if (a->type == I915_GGTT_VIEW_PARTIAL) > > > > return !memcmp(&a->partial, &b->partial, > > > > sizeof(a->partial)); > > > > return true; > > > > > > This is where anonymous union causes problems since you have to build a > > > switch statement before memcmp while the original goal was for memcmp to > > > elegantly avoid that. > > > > Yup, mentioned the same on irc. I much prefer we do the memcmp on type != > > NORMAL, to avoid mistakes when adding more views in the future. > > I just dislike the .partial. extra stuff all over. But whatever, it's a > minor detail in my book. We can go with the non-anon union if people > prefer that. Here it would be if (a->type != NORMA)) return !memcmp(&a->params, &b->params); Seems at least in this case cleaner. But yet everywhere else you'll get a params., but that can be alleviated a bit with local variables. -Daniel > > -- > Ville Syrjälä > Intel OTC -- 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] [drm-intel:topic/drm-misc 38/38] drivers/gpu/drm/vgem/vgem_drv.c:249:9: error: implicit declaration of function 'drm_vma_offset_exact_lookup'
tree: git://anongit.freedesktop.org/drm-intel topic/drm-misc head: 0f3609e5b5c53ed52b8e82993cbda72806ef9c69 commit: 0f3609e5b5c53ed52b8e82993cbda72806ef9c69 [38/38] drm/gem: Use kref_get_unless_zero for the weak mmap references config: i386-randconfig-s0-201541 (attached as .config) reproduce: git checkout 0f3609e5b5c53ed52b8e82993cbda72806ef9c69 # save the attached .config to linux build tree make ARCH=i386 All error/warnings (new ones prefixed by >>): drivers/gpu/drm/vgem/vgem_drv.c: In function 'vgem_drm_gem_mmap': >> drivers/gpu/drm/vgem/vgem_drv.c:249:9: error: implicit declaration of >> function 'drm_vma_offset_exact_lookup' >> [-Werror=implicit-function-declaration] node = drm_vma_offset_exact_lookup(dev->vma_offset_manager, ^ >> drivers/gpu/drm/vgem/vgem_drv.c:249:7: warning: assignment makes pointer >> from integer without a cast [-Wint-conversion] node = drm_vma_offset_exact_lookup(dev->vma_offset_manager, ^ cc1: some warnings being treated as errors vim +/drm_vma_offset_exact_lookup +249 drivers/gpu/drm/vgem/vgem_drv.c 502e95c6 Zach Reizner 2015-03-04 243 struct drm_gem_object *obj; 502e95c6 Zach Reizner 2015-03-04 244 struct drm_vgem_gem_object *vgem_obj; 502e95c6 Zach Reizner 2015-03-04 245 int ret = 0; 502e95c6 Zach Reizner 2015-03-04 246 502e95c6 Zach Reizner 2015-03-04 247 mutex_lock(&dev->struct_mutex); 502e95c6 Zach Reizner 2015-03-04 248 502e95c6 Zach Reizner 2015-03-04 @249 node = drm_vma_offset_exact_lookup(dev->vma_offset_manager, 502e95c6 Zach Reizner 2015-03-04 250 vma->vm_pgoff, 502e95c6 Zach Reizner 2015-03-04 251 vma_pages(vma)); 502e95c6 Zach Reizner 2015-03-04 252 if (!node) { :: The code at line 249 was first introduced by commit :: 502e95c6678505474f1056480310cd9382bacbac drm/vgem: implement virtual GEM :: TO: Zach Reizner :: CC: Dave Airlie --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: restore ggtt double-bind avoidance
This was accidentally lost in commit 75d04a3773ecee617847de963ae4195d6aa74c28 Author: Mika Kuoppala Date: Tue Apr 28 17:56:17 2015 +0300 drm/i915/gtt: Allocate va range only if vma is not bound While at it implement an improved version suggested by Chris which avoids the double-bind irrespective of what type of bind is done first. Note that this exact bug was already addressed in commit d0e30adc42d979e4adc36b6c112b57337423b70c Author: Chris Wilson Date: Wed Jul 29 20:02:48 2015 +0100 drm/i915: Mark PIN_USER binding as GLOBAL_BIND without the aliasing ppgtt but the problem is still that originally in commit 0875546c5318c85c13d07014af5350e9000bc9e9 Author: Daniel Vetter Date: Mon Apr 20 09:04:05 2015 -0700 drm/i915: Fix up the vma aliasing ppgtt binding if forgotten to take into account there case where we have a GLOBAL_BIND before a LOCAL_BIND. This patch here fixes that. v2: Pimp commit message and revert the partial fix. v3: Split into two functions to specialize on aliasing_ppgtt y/n. v4: WARN_ON for paranoia in the init sequence, since the ggtt probe and aliasing ppgtt setup are far apart. Cc: Chris Wilson Cc: Michel Thierry Cc: Mika Kuoppala Link: http://mid.gmane.org/1444894651-5332-1-git-send-email-daniel.vet...@ffwll.ch Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_gtt.c | 49 - 1 file changed, 37 insertions(+), 12 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index ff1004876f56..419687135f41 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2502,6 +2502,39 @@ static int ggtt_bind_vma(struct i915_vma *vma, enum i915_cache_level cache_level, u32 flags) { + 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; + + + vma->vm->insert_entries(vma->vm, pages, + vma->node.start, + cache_level, pte_flags); + + /* +* Without aliasing PPGTT there's no difference between +* GLOBAL/LOCAL_BIND, it's all the same ptes. Hence unconditionally +* upgrade to both bound if we bind either to avoid double-binding. +*/ + vma->bound |= GLOBAL_BIND | LOCAL_BIND; + + return 0; +} + +static int aliasing_gtt_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; @@ -2519,23 +2552,13 @@ static int ggtt_bind_vma(struct i915_vma *vma, pte_flags |= PTE_READ_ONLY; - if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) { + if (flags & GLOBAL_BIND) { vma->vm->insert_entries(vma->vm, pages, vma->node.start, cache_level, pte_flags); - - /* Note the inconsistency here is due to absence of the -* aliasing ppgtt on gen4 and earlier. Though we always -* request PIN_USER for execbuffer (translated to LOCAL_BIND), -* without the appgtt, we cannot honour that request and so -* must substitute it with a global binding. Since we do this -* behind the upper layers back, we need to explicitly set -* the bound flag ourselves. -*/ - vma->bound |= GLOBAL_BIND; } - if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) { + if (flags & LOCAL_BIND) { struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt; appgtt->base.insert_entries(&appgtt->base, pages, vma->node.start, @@ -2699,6 +2722,8 @@ static int i915_gem_setup_global_gtt(struct drm_device *dev, true); dev_priv->mm.aliasing_ppgtt = ppgtt; + WARN_ON(dev_priv->gtt.base.bind_vma != ggtt_bind_vma); + dev_priv->gtt.base.bind_vma = aliasing_gtt_bind_vma; } return 0; -- 2.5.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()
On Thu, Oct 15, 2015 at 12:30:44PM +0100, Tvrtko Ursulin wrote: > > On 15/10/15 12:17, Ville Syrjälä wrote: > > On Thu, Oct 15, 2015 at 12:10:32PM +0100, Tvrtko Ursulin wrote: > >> > >> Hi, > >> > >> On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote: > >>> From: Ville Syrjälä > >>> > >>> intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird > >>> rotation (to find the right GTT view for it), so no need to pass all > >>> kinds of plane stuff. > >>> > >>> Signed-off-by: Ville Syrjälä > >>> --- > >>>drivers/gpu/drm/i915/intel_display.c | 39 > >>> > >>>drivers/gpu/drm/i915/intel_drv.h | 5 ++--- > >>>drivers/gpu/drm/i915/intel_fbdev.c | 2 +- > >>>3 files changed, 20 insertions(+), 26 deletions(-) > >>> > >>> diff --git a/drivers/gpu/drm/i915/intel_display.c > >>> b/drivers/gpu/drm/i915/intel_display.c > >>> index 85e1473..80e9f2e 100644 > >>> --- a/drivers/gpu/drm/i915/intel_display.c > >>> +++ b/drivers/gpu/drm/i915/intel_display.c > >>> @@ -2275,8 +2275,9 @@ intel_fb_align_height(struct drm_device *dev, > >>> unsigned int height, > >>>} > >>> > >>>static int > >>> -intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct > >>> drm_framebuffer *fb, > >>> - const struct drm_plane_state *plane_state) > >>> +intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, > >>> + const struct drm_framebuffer *fb, > >>> + unsigned int rotation) > >>>{ > >>> struct drm_i915_private *dev_priv = to_i915(fb->dev); > >>> struct intel_rotation_info *info = &view->rotation_info; > >>> @@ -2284,10 +2285,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view > >>> *view, struct drm_framebuffer *fb, > >>> > >>> *view = i915_ggtt_view_normal; > >>> > >>> - if (!plane_state) > >>> - return 0; > >>> - > >>> - if (!intel_rotation_90_or_270(plane_state->rotation)) > >>> + if (!intel_rotation_90_or_270(rotation)) > >>> return 0; > >>> > >>> *view = i915_ggtt_view_rotated; > >>> @@ -2354,9 +2352,8 @@ static unsigned int intel_surf_alignment(const > >>> struct drm_i915_private *dev_priv > >>>} > >>> > >>>int > >>> -intel_pin_and_fence_fb_obj(struct drm_plane *plane, > >>> -struct drm_framebuffer *fb, > >>> -const struct drm_plane_state *plane_state, > >>> +intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, > >>> +unsigned int rotation, > >>> struct intel_engine_cs *pipelined, > >>> struct drm_i915_gem_request > >>> **pipelined_request) > >>>{ > >> > >> It feels like you are losing the benefit of cleaning this up by having > >> to pass in rotation anyway. So I think it makes more sense to keep > >> passing in plane_state and only get rid of the plane. Or vice-versa, not > >> really sure what is conceptually better. Possibly plane and then access > >> the state from it. > > > > The only thing we basically need is "which vma do we want". But just > > passing rotation directly looks nicer I think. The benefit really is > > eliminating the ugly 'if (!plane_state)' mess caused by intel_fbdev. > > We'll have to disagree there then because I find it really inelegant to > express the knowledge of what exact information is needed when preparing > the frame buffer for display into the function parameters. > > It is conceptually much more elegant to say "this fb for this plane - do > what is right". Only if the function is actually used for that. With fbdev it's not. Chris did sell the "vma in plane state" idea to me last night on irc, so when we get that we probably want a function that accepts the plane state. But that can basically just wrap the current thing, which means fbdev can keep using the lower level function that doesn't need the plane state. -- 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 14/22] drm/i915: Don't treat differently sized rotated views as equal
On Thu, Oct 15, 2015 at 02:02:24PM +0200, Daniel Vetter wrote: > On Thu, Oct 15, 2015 at 12:18:41PM +0100, Tvrtko Ursulin wrote: > > > > On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote: > > >From: Ville Syrjälä > > > > > >In case we have multiple different rotated views into the same object, > > >each one may need its own vma due to being of different sizes. So don't > > >treat all rotated views as equal. > > > > > >Signed-off-by: Ville Syrjälä > > >--- > > > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > > >b/drivers/gpu/drm/i915/i915_gem_gtt.h > > >index caa182f..68de734 100644 > > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > > >@@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > > > > > if (a->type != b->type) > > > return false; > > >+ if (a->type == I915_GGTT_VIEW_ROTATED) > > >+ return !memcmp(&a->rotated, &b->rotated, sizeof(a->rotated)); > > > if (a->type == I915_GGTT_VIEW_PARTIAL) > > > return !memcmp(&a->partial, &b->partial, sizeof(a->partial)); > > > return true; > > > > This is where anonymous union causes problems since you have to build a > > switch statement before memcmp while the original goal was for memcmp to > > elegantly avoid that. > > Yup, mentioned the same on irc. I much prefer we do the memcmp on type != > NORMAL, to avoid mistakes when adding more views in the future. I just dislike the .partial. extra stuff all over. But whatever, it's a minor detail in my book. We can go with the non-anon union if people prefer that. -- 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 14/22] drm/i915: Don't treat differently sized rotated views as equal
On Thu, Oct 15, 2015 at 12:18:41PM +0100, Tvrtko Ursulin wrote: > > On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote: > >From: Ville Syrjälä > > > >In case we have multiple different rotated views into the same object, > >each one may need its own vma due to being of different sizes. So don't > >treat all rotated views as equal. > > > >Signed-off-by: Ville Syrjälä > >--- > > drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ > > 1 file changed, 2 insertions(+) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h > >b/drivers/gpu/drm/i915/i915_gem_gtt.h > >index caa182f..68de734 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.h > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.h > >@@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, > > > > if (a->type != b->type) > > return false; > >+if (a->type == I915_GGTT_VIEW_ROTATED) > >+return !memcmp(&a->rotated, &b->rotated, sizeof(a->rotated)); > > if (a->type == I915_GGTT_VIEW_PARTIAL) > > return !memcmp(&a->partial, &b->partial, sizeof(a->partial)); > > return true; > > This is where anonymous union causes problems since you have to build a > switch statement before memcmp while the original goal was for memcmp to > elegantly avoid that. Yup, mentioned the same on irc. I much prefer we do the memcmp on type != NORMAL, to avoid mistakes when adding more views in the future. -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 12/22] drm/i915: Set i915_ggtt_view_normal type explicitly
On Thu, Oct 15, 2015 at 12:15:21PM +0100, Tvrtko Ursulin wrote: > > Hi, > > On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote: > >From: Ville Syrjälä > > > >Just for clarity set the type for i915_ggtt_view_normal explicitly. > > > >While at it fix the indentation fail for i915_ggtt_view_rotated. > > > >Signed-off-by: Ville Syrjälä > >--- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 6 -- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > >b/drivers/gpu/drm/i915/i915_gem_gtt.c > >index 620d57e..71acc71 100644 > >--- a/drivers/gpu/drm/i915/i915_gem_gtt.c > >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > >@@ -95,9 +95,11 @@ > > 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_normal = { > >+.type = I915_GGTT_VIEW_NORMAL, > >+}; > > AFAIR while this was developed Daniel was very keen on the normal view type > being implicitly zero. Can't remember at this very moment if some of the > current code actually depends on it though. We kzalloc vmas, which means any view in there is normal by default. Still very much keen on that ;-) -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 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()
On 15/10/15 12:17, Ville Syrjälä wrote: On Thu, Oct 15, 2015 at 12:10:32PM +0100, Tvrtko Ursulin wrote: Hi, On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird rotation (to find the right GTT view for it), so no need to pass all kinds of plane stuff. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 39 drivers/gpu/drm/i915/intel_drv.h | 5 ++--- drivers/gpu/drm/i915/intel_fbdev.c | 2 +- 3 files changed, 20 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 85e1473..80e9f2e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2275,8 +2275,9 @@ intel_fb_align_height(struct drm_device *dev, unsigned int height, } static int -intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, - const struct drm_plane_state *plane_state) +intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, + const struct drm_framebuffer *fb, + unsigned int rotation) { struct drm_i915_private *dev_priv = to_i915(fb->dev); struct intel_rotation_info *info = &view->rotation_info; @@ -2284,10 +2285,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, *view = i915_ggtt_view_normal; - if (!plane_state) - return 0; - - if (!intel_rotation_90_or_270(plane_state->rotation)) + if (!intel_rotation_90_or_270(rotation)) return 0; *view = i915_ggtt_view_rotated; @@ -2354,9 +2352,8 @@ static unsigned int intel_surf_alignment(const struct drm_i915_private *dev_priv } int -intel_pin_and_fence_fb_obj(struct drm_plane *plane, - struct drm_framebuffer *fb, - const struct drm_plane_state *plane_state, +intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, + unsigned int rotation, struct intel_engine_cs *pipelined, struct drm_i915_gem_request **pipelined_request) { It feels like you are losing the benefit of cleaning this up by having to pass in rotation anyway. So I think it makes more sense to keep passing in plane_state and only get rid of the plane. Or vice-versa, not really sure what is conceptually better. Possibly plane and then access the state from it. The only thing we basically need is "which vma do we want". But just passing rotation directly looks nicer I think. The benefit really is eliminating the ugly 'if (!plane_state)' mess caused by intel_fbdev. We'll have to disagree there then because I find it really inelegant to express the knowledge of what exact information is needed when preparing the frame buffer for display into the function parameters. It is conceptually much more elegant to say "this fb for this plane - do what is right". Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 14/22] drm/i915: Don't treat differently sized rotated views as equal
On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä In case we have multiple different rotated views into the same object, each one may need its own vma due to being of different sizes. So don't treat all rotated views as equal. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.h | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.h b/drivers/gpu/drm/i915/i915_gem_gtt.h index caa182f..68de734 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.h +++ b/drivers/gpu/drm/i915/i915_gem_gtt.h @@ -553,6 +553,8 @@ i915_ggtt_view_equal(const struct i915_ggtt_view *a, if (a->type != b->type) return false; + if (a->type == I915_GGTT_VIEW_ROTATED) + return !memcmp(&a->rotated, &b->rotated, sizeof(a->rotated)); if (a->type == I915_GGTT_VIEW_PARTIAL) return !memcmp(&a->partial, &b->partial, sizeof(a->partial)); return true; This is where anonymous union causes problems since you have to build a switch statement before memcmp while the original goal was for memcmp to elegantly avoid that. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()
On Thu, Oct 15, 2015 at 12:10:32PM +0100, Tvrtko Ursulin wrote: > > Hi, > > On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote: > > From: Ville Syrjälä > > > > intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird > > rotation (to find the right GTT view for it), so no need to pass all > > kinds of plane stuff. > > > > Signed-off-by: Ville Syrjälä > > --- > > drivers/gpu/drm/i915/intel_display.c | 39 > > > > drivers/gpu/drm/i915/intel_drv.h | 5 ++--- > > drivers/gpu/drm/i915/intel_fbdev.c | 2 +- > > 3 files changed, 20 insertions(+), 26 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > b/drivers/gpu/drm/i915/intel_display.c > > index 85e1473..80e9f2e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2275,8 +2275,9 @@ intel_fb_align_height(struct drm_device *dev, > > unsigned int height, > > } > > > > static int > > -intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct > > drm_framebuffer *fb, > > - const struct drm_plane_state *plane_state) > > +intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, > > + const struct drm_framebuffer *fb, > > + unsigned int rotation) > > { > > struct drm_i915_private *dev_priv = to_i915(fb->dev); > > struct intel_rotation_info *info = &view->rotation_info; > > @@ -2284,10 +2285,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, > > struct drm_framebuffer *fb, > > > > *view = i915_ggtt_view_normal; > > > > - if (!plane_state) > > - return 0; > > - > > - if (!intel_rotation_90_or_270(plane_state->rotation)) > > + if (!intel_rotation_90_or_270(rotation)) > > return 0; > > > > *view = i915_ggtt_view_rotated; > > @@ -2354,9 +2352,8 @@ static unsigned int intel_surf_alignment(const struct > > drm_i915_private *dev_priv > > } > > > > int > > -intel_pin_and_fence_fb_obj(struct drm_plane *plane, > > - struct drm_framebuffer *fb, > > - const struct drm_plane_state *plane_state, > > +intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, > > + unsigned int rotation, > >struct intel_engine_cs *pipelined, > >struct drm_i915_gem_request **pipelined_request) > > { > > It feels like you are losing the benefit of cleaning this up by having > to pass in rotation anyway. So I think it makes more sense to keep > passing in plane_state and only get rid of the plane. Or vice-versa, not > really sure what is conceptually better. Possibly plane and then access > the state from it. The only thing we basically need is "which vma do we want". But just passing rotation directly looks nicer I think. The benefit really is eliminating the ugly 'if (!plane_state)' mess caused by intel_fbdev. -- 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 12/22] drm/i915: Set i915_ggtt_view_normal type explicitly
Hi, On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä Just for clarity set the type for i915_ggtt_view_normal explicitly. While at it fix the indentation fail for i915_ggtt_view_rotated. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_gem_gtt.c | 6 -- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 620d57e..71acc71 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -95,9 +95,11 @@ 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_normal = { + .type = I915_GGTT_VIEW_NORMAL, +}; AFAIR while this was developed Daniel was very keen on the normal view type being implicitly zero. Can't remember at this very moment if some of the current code actually depends on it though. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()
Hi, On 14/10/15 17:29, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird rotation (to find the right GTT view for it), so no need to pass all kinds of plane stuff. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_display.c | 39 drivers/gpu/drm/i915/intel_drv.h | 5 ++--- drivers/gpu/drm/i915/intel_fbdev.c | 2 +- 3 files changed, 20 insertions(+), 26 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 85e1473..80e9f2e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -2275,8 +2275,9 @@ intel_fb_align_height(struct drm_device *dev, unsigned int height, } static int -intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, - const struct drm_plane_state *plane_state) +intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, + const struct drm_framebuffer *fb, + unsigned int rotation) { struct drm_i915_private *dev_priv = to_i915(fb->dev); struct intel_rotation_info *info = &view->rotation_info; @@ -2284,10 +2285,7 @@ intel_fill_fb_ggtt_view(struct i915_ggtt_view *view, struct drm_framebuffer *fb, *view = i915_ggtt_view_normal; - if (!plane_state) - return 0; - - if (!intel_rotation_90_or_270(plane_state->rotation)) + if (!intel_rotation_90_or_270(rotation)) return 0; *view = i915_ggtt_view_rotated; @@ -2354,9 +2352,8 @@ static unsigned int intel_surf_alignment(const struct drm_i915_private *dev_priv } int -intel_pin_and_fence_fb_obj(struct drm_plane *plane, - struct drm_framebuffer *fb, - const struct drm_plane_state *plane_state, +intel_pin_and_fence_fb_obj(struct drm_framebuffer *fb, + unsigned int rotation, struct intel_engine_cs *pipelined, struct drm_i915_gem_request **pipelined_request) { It feels like you are losing the benefit of cleaning this up by having to pass in rotation anyway. So I think it makes more sense to keep passing in plane_state and only get rid of the plane. Or vice-versa, not really sure what is conceptually better. Possibly plane and then access the state from it. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Split out aliasing-ppgtt from ggtt_bind_vma()
The presence of the aliasing-ppgtt (or rather its absence) causes some confusion on old generations where we still pretend that there is a difference between PIN_USER and PIN_GLOBAL even though we only have the global GTT. The confusion has resulted in a bug where we do not mark the vma as suitably bound for both types of access and so end up processing the binding twice. The double bind was accidentally introduced in commit 75d04a3773ecee617847de963ae4195d6aa74c28 Author: Mika Kuoppala Date: Tue Apr 28 17:56:17 2015 +0300 drm/i915/gtt: Allocate va range only if vma is not bound While at it implement an improved version suggested by Chris which avoids the double-bind irrespective of what type of bind is done first. Note that this exact bug was already addressed in commit d0e30adc42d979e4adc36b6c112b57337423b70c Author: Chris Wilson Date: Wed Jul 29 20:02:48 2015 +0100 drm/i915: Mark PIN_USER binding as GLOBAL_BIND without the aliasing ppgtt but the problem is still that originally in commit 0875546c5318c85c13d07014af5350e9000bc9e9 Author: Daniel Vetter Date: Mon Apr 20 09:04:05 2015 -0700 drm/i915: Fix up the vma aliasing ppgtt binding if forgotten to take into account there case where we have a GLOBAL_BIND before a LOCAL_BIND. Here we separate out the binding into the global GTT for machines where we have the aliasing-ppgtt and where we do not, so that we can perform the fixup without further confusion. Signed-off-by: Chris Wilson Cc: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_gtt.c | 62 +++-- 1 file changed, 45 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index 0796aa06e9f5..81f4628ae0e6 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2493,7 +2493,6 @@ static int ggtt_bind_vma(struct i915_vma *vma, enum i915_cache_level cache_level, u32 flags) { - struct drm_i915_private *dev_priv = to_i915(vma->vm->dev); u32 pte_flags; int ret; @@ -2506,26 +2505,49 @@ static int ggtt_bind_vma(struct i915_vma *vma, if (vma->obj->gt_ro) pte_flags |= PTE_READ_ONLY; - if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) { + vma->vm->insert_entries(vma->vm, + vma->ggtt_view.pages, + vma->node.start, + cache_level, pte_flags); + + /* Note the inconsistency here is due to absence of the +* aliasing ppgtt on gen5 and earlier. Though we always +* request PIN_USER for execbuffer (translated to LOCAL_BIND), +* without the appgtt, we cannot honour that request and so +* must substitute it with a global binding. Since we do this +* behind the upper layers back, we need to explicitly set +* the bound flag ourselves. +*/ + vma->bound |= GLOBAL_BIND | LOCAL_BIND; + return 0; +} + +static int agtt_bind_vma(struct i915_vma *vma, +enum i915_cache_level cache_level, +u32 flags) +{ + u32 pte_flags; + int ret; + + ret = i915_get_ggtt_vma_pages(vma); + if (ret) + return ret; + + /* Currently applicable only to VLV */ + pte_flags = 0; + if (vma->obj->gt_ro) + pte_flags |= PTE_READ_ONLY; + + if (flags & GLOBAL_BIND) { vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages, vma->node.start, cache_level, pte_flags); - - /* Note the inconsistency here is due to absence of the -* aliasing ppgtt on gen4 and earlier. Though we always -* request PIN_USER for execbuffer (translated to LOCAL_BIND), -* without the appgtt, we cannot honour that request and so -* must substitute it with a global binding. Since we do this -* behind the upper layers back, we need to explicitly set -* the bound flag ourselves. -*/ - vma->bound |= GLOBAL_BIND; - } - if (dev_priv->mm.aliasing_ppgtt && flags & LOCAL_BIND) { - struct i915_hw_ppgtt *appgtt = dev_priv->mm.aliasing_ppgtt; + if (flags & LOCAL_BIND) { + struct i915_hw_ppgtt *appgtt = + to_i915(vma->vm->dev)->mm.aliasing_ppgtt; appgtt->base.insert_entries(&appgtt->base, vma->ggtt_view.pages, vma->node.start, @@ -2959,7 +2981,10 @@ static int gen8_gmch_probe(struct drm_device *dev, dev_priv->gtt.base.clear_range = gen8_ggtt_clear_range; dev_priv->
Re: [Intel-gfx] [PATCH] drm/i915: restore ggtt double-bind avoidance
On Thu, Oct 15, 2015 at 09:48:26AM +0100, Chris Wilson wrote: > On Thu, Oct 15, 2015 at 09:37:31AM +0200, Daniel Vetter wrote: > > This was accidentally lost in > > > > commit 75d04a3773ecee617847de963ae4195d6aa74c28 > > Author: Mika Kuoppala > > Date: Tue Apr 28 17:56:17 2015 +0300 > > > > drm/i915/gtt: Allocate va range only if vma is not bound > > > > While at it implement an improved version suggested by Chris which > > avoids the double-bind irrespective of what type of bind is done > > first. > > > > Cc: Chris Wilson > > Cc: Michel Thierry > > Cc: Mika Kuoppala > > Signed-off-by: Daniel Vetter > > --- > > drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > > b/drivers/gpu/drm/i915/i915_gem_gtt.c > > index a3910fb656e7..e90c062e1122 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > > @@ -2542,6 +2542,9 @@ static int ggtt_bind_vma(struct i915_vma *vma, > > cache_level, pte_flags); > > } > > > > + if (!dev_priv->mm.aliasing_ppgtt) > > + vma->bound |= GLOBAL_BIND | LOCAL_BIND; > > + > > return 0; > > } > > Didn't we already add: > > if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) { > vma->vm->insert_entries(vma->vm, > vma->ggtt_view.pages, > vma->node.start, > cache_level, pte_flags); > > /* Note the inconsistency here is due to absence of the > * aliasing ppgtt on gen4 and earlier. Though we always > * request PIN_USER for execbuffer (translated to LOCAL_BIND), > * without the appgtt, we cannot honour that request and so > * must substitute it with a global binding. Since we do this > * behind the upper layers back, we need to explicitly set > * the bound flag ourselves. > */ > vma->bound |= GLOBAL_BIND; > > } > > to ggtt_bind_vma() ? Yeah, my patch superseeds this one (it's an old one, cherrished for a long time) - LOCAL_BIND also implies GLOBAL_BIND if we lack a ppgtt. This is something I've missed in commit 0875546c5318c85c13d07014af5350e9000bc9e9 Author: Daniel Vetter Date: Mon Apr 20 09:04:05 2015 -0700 drm/i915: Fix up the vma aliasing ppgtt binding I'll frob the patch a bit and ping you on irc, but first lunch. -Daniel > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre -- 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 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()
On Thu, Oct 15, 2015 at 12:36:43PM +0300, Ville Syrjälä wrote: > On Thu, Oct 15, 2015 at 10:08:53AM +0100, Chris Wilson wrote: > > On Wed, Oct 14, 2015 at 07:29:03PM +0300, ville.syrj...@linux.intel.com > > wrote: > > > From: Ville Syrjälä > > > > > > intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird > > > rotation (to find the right GTT view for it), so no need to pass all > > > kinds of plane stuff. > > > > imho this is a mistep, I think using the plane-state to not only pass > > the full description of the plane being bound (which may have additional > > information like the need for fencing for fbc as well as alternative > > views, i.e. it is a lot more versatile) but also allows us to track the > > binding for the plane-state and tie the VMA to lifetime of the plane. > > > > i.e. I think intel_pin_and_fence_fb_obj would be better described as > > intel_plane_state_pin_vma (and correspondingly > > intel_plane_state_unpin_vma). > > > > Yes, intel_fbdev.c is a wart to any proposed interface. > > The current code is just too ugly to live IMO (due to fbdev, yes), so > I think we want this for now. We can always wrap it up in fancier > clothing for users that actually have a plane state once someone comes > up with some real code that needs it. To handle this we could make intel_pin_and_fence_fb return the vma for callers to store in the plane state eventually, with errors encoded using ERR_PTR. That way we can keep intel_fbdev.c as is and still store the vma in the plane state. -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] HD 4000 ubuntu 14.04 driver
Hello, I am new to this list. I work on an OpenCL project. I try to find support to setup the environment: to install the ubuntu driver for my i5-3230M HD 4000 to run OpenCL kernels on it I've already downloaded the Intel openCL SDK for linux but I dont know how to use it as the HD4000 is not recognized (clinfo shows me only the CL_DEVICE_TYPE_CPU and not the CL_DEVICE_TYPE_GPU...) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()
On Thu, Oct 15, 2015 at 10:08:53AM +0100, Chris Wilson wrote: > On Wed, Oct 14, 2015 at 07:29:03PM +0300, ville.syrj...@linux.intel.com wrote: > > From: Ville Syrjälä > > > > intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird > > rotation (to find the right GTT view for it), so no need to pass all > > kinds of plane stuff. > > imho this is a mistep, I think using the plane-state to not only pass > the full description of the plane being bound (which may have additional > information like the need for fencing for fbc as well as alternative > views, i.e. it is a lot more versatile) but also allows us to track the > binding for the plane-state and tie the VMA to lifetime of the plane. > > i.e. I think intel_pin_and_fence_fb_obj would be better described as > intel_plane_state_pin_vma (and correspondingly > intel_plane_state_unpin_vma). > > Yes, intel_fbdev.c is a wart to any proposed interface. The current code is just too ugly to live IMO (due to fbdev, yes), so I think we want this for now. We can always wrap it up in fancier clothing for users that actually have a plane state once someone comes up with some real code that needs it. -- 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 11/22] drm/i915: Don't pass plane+plane_state to intel_pin_and_fence_fb_obj()
On Wed, Oct 14, 2015 at 07:29:03PM +0300, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > intel_pin_and_fence_fb_obj() only needs the framebuffer, and the desird > rotation (to find the right GTT view for it), so no need to pass all > kinds of plane stuff. imho this is a mistep, I think using the plane-state to not only pass the full description of the plane being bound (which may have additional information like the need for fencing for fbc as well as alternative views, i.e. it is a lot more versatile) but also allows us to track the binding for the plane-state and tie the VMA to lifetime of the plane. i.e. I think intel_pin_and_fence_fb_obj would be better described as intel_plane_state_pin_vma (and correspondingly intel_plane_state_unpin_vma). Yes, intel_fbdev.c is a wart to any proposed interface. -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] [RFCv2 DP-typeC 2/6] drm/i915/dp: Reuse shared DPLL if it exists already
>-Original Message- >From: Jani Nikula [mailto:jani.nik...@linux.intel.com] >Sent: Wednesday, October 14, 2015 6:53 PM >To: R, Durgadoss; intel-gfx@lists.freedesktop.org >Subject: Re: [Intel-gfx] [RFCv2 DP-typeC 2/6] drm/i915/dp: Reuse shared DPLL >if it exists already > >On Wed, 14 Oct 2015, Durgadoss R wrote: >> Do not call intel_get_shared_dpll() if there exists a >> valid shared DPLL already. >> >> Signed-off-by: Durgadoss R >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 70 >> >> drivers/gpu/drm/i915/intel_display.c | 2 +- >> drivers/gpu/drm/i915/intel_drv.h | 2 +- >> 3 files changed, 42 insertions(+), 32 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c >> b/drivers/gpu/drm/i915/intel_ddi.c >> index 9098c12..8e4ea36 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -1258,7 +1258,8 @@ hsw_ddi_calculate_wrpll(int clock /* in Hz */, >> static bool >> hsw_ddi_pll_select(struct intel_crtc *intel_crtc, >> struct intel_crtc_state *crtc_state, >> - struct intel_encoder *intel_encoder) >> + struct intel_encoder *intel_encoder, >> + bool find_dpll) >> { >> int clock = crtc_state->port_clock; >> >> @@ -1278,14 +1279,16 @@ hsw_ddi_pll_select(struct intel_crtc *intel_crtc, >> >> crtc_state->dpll_hw_state.wrpll = val; >> >> -pll = intel_get_shared_dpll(intel_crtc, crtc_state); >> -if (pll == NULL) { >> -DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", >> - pipe_name(intel_crtc->pipe)); >> -return false; >> -} >> +if (find_dpll) { >> +pll = intel_get_shared_dpll(intel_crtc, crtc_state); >> +if (pll == NULL) { >> +DRM_DEBUG_DRIVER("failed to find PLL for pipe >> %c\n", >> + pipe_name(intel_crtc->pipe)); >> +return false; >> +} > >You have to do a *lot* of passing around parameters here. I wonder (and >really, I don't know) if we could make intel_get_shared_dpll() grow >smarts about reusing the pll with intel_crtc_to_shared_dpll() when it's >there already. Sure. I will try it out and do this change if it works without issues. Thanks, Durga > >BR, >Jani. > > >> >> -crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id); >> +crtc_state->ddi_pll_sel = PORT_CLK_SEL_WRPLL(pll->id); >> +} >> } >> >> return true; >> @@ -1540,7 +1543,8 @@ skip_remaining_dividers: >> static bool >> skl_ddi_pll_select(struct intel_crtc *intel_crtc, >> struct intel_crtc_state *crtc_state, >> - struct intel_encoder *intel_encoder) >> + struct intel_encoder *intel_encoder, >> + bool find_dpll) >> { >> struct intel_shared_dpll *pll; >> uint32_t ctrl1, cfgcr1, cfgcr2; >> @@ -1594,15 +1598,17 @@ skl_ddi_pll_select(struct intel_crtc *intel_crtc, >> crtc_state->dpll_hw_state.cfgcr1 = cfgcr1; >> crtc_state->dpll_hw_state.cfgcr2 = cfgcr2; >> >> -pll = intel_get_shared_dpll(intel_crtc, crtc_state); >> -if (pll == NULL) { >> -DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", >> - pipe_name(intel_crtc->pipe)); >> -return false; >> -} >> +if (find_dpll) { >> +pll = intel_get_shared_dpll(intel_crtc, crtc_state); >> +if (pll == NULL) { >> +DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", >> + pipe_name(intel_crtc->pipe)); >> +return false; >> +} >> >> -/* shared DPLL id 0 is DPLL 1 */ >> -crtc_state->ddi_pll_sel = pll->id + 1; >> +/* shared DPLL id 0 is DPLL 1 */ >> +crtc_state->ddi_pll_sel = pll->id + 1; >> +} >> >> return true; >> } >> @@ -1632,7 +1638,8 @@ static const struct bxt_clk_div bxt_dp_clk_val[] = { >> static bool >> bxt_ddi_pll_select(struct intel_crtc *intel_crtc, >> struct intel_crtc_state *crtc_state, >> - struct intel_encoder *intel_encoder) >> + struct intel_encoder *intel_encoder, >> + bool find_pll) >> { >> struct intel_shared_dpll *pll; >> struct bxt_clk_div clk_div = {0}; >> @@ -1741,15 +1748,17 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc, >> crtc_state->dpll_hw_state.pcsdw12 = >> LANESTAGGER_STRAP_OVRD | lanestagger; >> >> -pll = intel_get_shared_dpll(intel_crtc, crtc_state); >> -if (pll == NULL) { >> -DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n", >> -pipe_name(intel_crtc->pipe)); >> -return false; >> -} >> +if (find_pll) { >> +
Re: [Intel-gfx] [PATCH] drm/i915: restore ggtt double-bind avoidance
On Thu, Oct 15, 2015 at 09:37:31AM +0200, Daniel Vetter wrote: > This was accidentally lost in > > commit 75d04a3773ecee617847de963ae4195d6aa74c28 > Author: Mika Kuoppala > Date: Tue Apr 28 17:56:17 2015 +0300 > > drm/i915/gtt: Allocate va range only if vma is not bound > > While at it implement an improved version suggested by Chris which > avoids the double-bind irrespective of what type of bind is done > first. > > Cc: Chris Wilson > Cc: Michel Thierry > Cc: Mika Kuoppala > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c > b/drivers/gpu/drm/i915/i915_gem_gtt.c > index a3910fb656e7..e90c062e1122 100644 > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c > @@ -2542,6 +2542,9 @@ static int ggtt_bind_vma(struct i915_vma *vma, > cache_level, pte_flags); > } > > + if (!dev_priv->mm.aliasing_ppgtt) > + vma->bound |= GLOBAL_BIND | LOCAL_BIND; > + > return 0; > } Didn't we already add: if (!dev_priv->mm.aliasing_ppgtt || flags & GLOBAL_BIND) { vma->vm->insert_entries(vma->vm, vma->ggtt_view.pages, vma->node.start, cache_level, pte_flags); /* Note the inconsistency here is due to absence of the * aliasing ppgtt on gen4 and earlier. Though we always * request PIN_USER for execbuffer (translated to LOCAL_BIND), * without the appgtt, we cannot honour that request and so * must substitute it with a global binding. Since we do this * behind the upper layers back, we need to explicitly set * the bound flag ourselves. */ vma->bound |= GLOBAL_BIND; } to ggtt_bind_vma() ? -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] drm/i915: respect previous reg values on primary plane disable
On Wed, Oct 14, 2015 at 01:33:57PM -0700, Kevin Strasser wrote: > On Wed, Oct 14, 2015 at 10:48:52PM +0300, Ville Syrjälä wrote: > > On Wed, Oct 14, 2015 at 11:59:59AM -0700, Kevin Strasser wrote: > [...] > > > Just to level set, these cases will produce different CRCs on HSW: > > > 1. Primary plane disabled, gamma correction disabled > > > 2. Primary plane disabled, gamma correction enabled > > > > > > Case 2 is visibly brighter than case 1 and looks more like the enabled > > > black > > > primary plane case. > > > > Ugh. That's weird. I thought data not going through any plane would > > bypass the gamma too. > > > > > The purpose of this patch is to get the behavior of a > > > disabled primary plane to match that of an enabled black plane, just as > > > it does > > > on non-HSW platforms. > > > > Does it? I just tried it on IVB, and behaves just like you said. So not > > sure how far back this goes. > > Ah, so this test case is failing on IVB too? I just poked at the registers. I don't think we have specific test cases for this, so any currently failing test case is just bad luck. It would be good to write a small tool that just frobs the registers in specific ways, and tells us if the test machine suffers from this issue. Would be easy to run on any machine then. What I did manually was: intel_reg write 0x4a000 0x40 intel_reg write 0x70180 0x0 = intel_reg read 0x7019c intel_reg write 0x7019c and as a second test I tried: intel_reg write 0x70180 = intel_reg read 0x7019c intel_reg write 0x7019c And the result was black in the first test, dark red in the second. On SKL I additionally tried to resize the plane to be smaller, and then tried the same thing. But as stated the palette seems to misbehave somehow, so there was no change in the output from changing entry 0 even though most of the screen was supposed to be black. > Are there any reporting tools we > can look at to find out what tests are passing for each platform? > > > And now I'm really wondering about platforms where the primary > > plane need not be fullscreen (gen2/3 and chv). > > > > I tried this on SKL too, but that confused me even more. The data not > > going through any plane seems to be gamma corrected regardless of any > > plane control bits, so that's good. However the legacy palette seems > > all fubar. Black input apparently doesn't map to palette entry 0. > > I wonder if you're seeing this on HSW too, or is your palette entry 0 > > supposed to be non-black? > > I also tried on BDW and it is passing the test with and without my patch > applied. > > I'm not really sure what 'palette entry 0' you are looking for. Could you > point > me to where in the code I can find out? Register 0x4a000, 0x4a800, 0x4b000, depending on which pipe you're using. > > > Looks like quite a bit more testing is needed to get to the bottom of > > this. > > Agreed, this does seem to extend beyond HSW. For now do you think my patch is > the right approach at least for HSW alone? Yeah, looks that way. But we do need to figure out which bits behave this way. Ie. is it just gamma, or pipe csc too, or perhaps even some other bits? -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t] tests: Add gem_exec_nop_concurrent test
This test is based on gem_exec_nop but submits nop batch buffers concurrently from different threads to check for ring hangs and other issues during concurrent submissions. Signed-off-by: Derek Morton --- tests/.gitignore| 1 + tests/Makefile.sources | 1 + tests/gem_exec_nop_concurrent.c | 172 3 files changed, 174 insertions(+) create mode 100644 tests/gem_exec_nop_concurrent.c diff --git a/tests/.gitignore b/tests/.gitignore index dc8bb53..0ad36f3 100644 --- a/tests/.gitignore +++ b/tests/.gitignore @@ -46,6 +46,7 @@ gem_exec_blt gem_exec_faulting_reloc gem_exec_lut_handle gem_exec_nop +gem_exec_nop_concurrent gem_exec_params gem_exec_parse gem_fd_exhaustion diff --git a/tests/Makefile.sources b/tests/Makefile.sources index 2e2e088..aece831 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -27,6 +27,7 @@ TESTS_progs_M = \ gem_exec_bad_domains \ gem_exec_faulting_reloc \ gem_exec_nop \ + gem_exec_nop_concurrent \ gem_exec_params \ gem_exec_parse \ gem_fenced_exec_thrash \ diff --git a/tests/gem_exec_nop_concurrent.c b/tests/gem_exec_nop_concurrent.c new file mode 100644 index 000..578f651 --- /dev/null +++ b/tests/gem_exec_nop_concurrent.c @@ -0,0 +1,172 @@ +/* + * Copyright © 2015 Intel Corporation + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS + * IN THE SOFTWARE. + * + * Authors: + *Derek Morton + * + * This test is based on gem_exec_nop but submits nop batch buffers concurrently + * from different threads to check for ring hangs and other issues during + * concurrent submissions. + * + */ + +#include "igt.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include "drm.h" + +#define LOCAL_I915_EXEC_NO_RELOC (1<<11) +#define LOCAL_I915_EXEC_HANDLE_LUT (1<<12) + +#define LOCAL_I915_EXEC_VEBOX (4<<0) + +IGT_TEST_DESCRIPTION( +"This Test will submit nop batch buffers concurrently to the same ring " + "and different rings in an attempt to trigger ring hangs."); + +const uint32_t batch[2] = {MI_BATCH_BUFFER_END}; + +struct ring +{ + unsigned ring_id; + const char *ring_name; + bool direction; +}; + +static void loop(int fd, uint32_t handle, int child_nbr, struct ring* ring, bool up) +{ + struct drm_i915_gem_execbuffer2 execbuf; + struct drm_i915_gem_exec_object2 gem_exec[1]; + int count; + int max_count = SLOW_QUICK(15, 4); + + gem_require_ring(fd, ring->ring_id); + + memset(&gem_exec, 0, sizeof(gem_exec)); + gem_exec[0].handle = handle; + + memset(&execbuf, 0, sizeof(execbuf)); + execbuf.buffers_ptr = (uintptr_t)gem_exec; + execbuf.buffer_count = 1; + execbuf.flags = ring->ring_id; + execbuf.flags |= LOCAL_I915_EXEC_HANDLE_LUT; + execbuf.flags |= LOCAL_I915_EXEC_NO_RELOC; + if (drmIoctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf)) { + execbuf.flags = ring->ring_id; + do_ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf); + } + gem_sync(fd, handle); + + for (count = 0; count <= max_count; count++) { + const int reps = 7; + int n, nbr_loops; + + if (up) + nbr_loops = 1 << count; + else + nbr_loops = 1 << (max_count - count); + + igt_info("Thread %d: starting submitting batches of %d batch buffers (ring=%s)\n", +child_nbr, nbr_loops, ring->ring_name); + fflush(stdout); + + for (n = 0; n < reps; n++) { + int loops = nbr_loops; + + while (loops--) + do_ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, &execbuf); + gem_
[Intel-gfx] [PATCH] drm/i915: restore ggtt double-bind avoidance
This was accidentally lost in commit 75d04a3773ecee617847de963ae4195d6aa74c28 Author: Mika Kuoppala Date: Tue Apr 28 17:56:17 2015 +0300 drm/i915/gtt: Allocate va range only if vma is not bound While at it implement an improved version suggested by Chris which avoids the double-bind irrespective of what type of bind is done first. Cc: Chris Wilson Cc: Michel Thierry Cc: Mika Kuoppala Signed-off-by: Daniel Vetter --- drivers/gpu/drm/i915/i915_gem_gtt.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c index a3910fb656e7..e90c062e1122 100644 --- a/drivers/gpu/drm/i915/i915_gem_gtt.c +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c @@ -2542,6 +2542,9 @@ static int ggtt_bind_vma(struct i915_vma *vma, cache_level, pte_flags); } + if (!dev_priv->mm.aliasing_ppgtt) + vma->bound |= GLOBAL_BIND | LOCAL_BIND; + return 0; } -- 2.5.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx