Re: [Intel-gfx] [PATCH] drm/i915: Add missing 'else' to intel_digital_port_connected()
On Thu, 11 Feb 2016, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä> > intel_digital_port_connected() lacks one 'else'. There's no > actual harm in not having it since each branch has an unconditional > return, so it can't accidentally end up in taking two branches instead > of just the one. But let's be consistent and add the 'else' anyway. > > Signed-off-by: Ville Syrjälä Reviewed-by: Jani Nikula > --- > drivers/gpu/drm/i915/intel_dp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index bbe18996efe6..040b86b9797f 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4551,7 +4551,7 @@ bool intel_digital_port_connected(struct > drm_i915_private *dev_priv, > { > if (HAS_PCH_IBX(dev_priv)) > return ibx_digital_port_connected(dev_priv, port); > - if (HAS_PCH_SPLIT(dev_priv)) > + else if (HAS_PCH_SPLIT(dev_priv)) > return cpt_digital_port_connected(dev_priv, port); > else if (IS_BROXTON(dev_priv)) > return bxt_digital_port_connected(dev_priv, port); -- Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 0/4] Scheduler tests
This patch set adds scheduler tests. Patch 1 adds library code used by the tests. There are other tests under development which are planned to reuse some of these libraries. Patch 2 adds some basic tests, read dependency tests and write dependency tests. Patch 3 Is the patch previously submitted by John Harrison to update gem_ctx_param_basic with ioctls to set context priorities. It is included as part of this patch set as Patch 4 is dependant on it. Patch 4 adds tests to check sheduler behaviour for batch buffers submitted at differing priorities. Derek Morton (3): lib/igt_bb_factory: Add igt_bb_factory library tests/gem_scheduler: Add gem_scheduler test tests/gem_scheduler: Add subtests to test batch priority behaviour John Harrison (1): igt/gem_ctx_param_basic: Updated to support scheduler priority interface lib/Makefile.sources| 2 + lib/igt.h | 1 + lib/igt_bb_factory.c| 401 + lib/igt_bb_factory.h| 47 + lib/ioctl_wrappers.h| 1 + tests/Makefile.sources | 1 + tests/gem_ctx_param_basic.c | 34 +++- tests/gem_scheduler.c | 431 8 files changed, 917 insertions(+), 1 deletion(-) create mode 100644 lib/igt_bb_factory.c create mode 100644 lib/igt_bb_factory.h create mode 100644 tests/gem_scheduler.c -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 2/4] tests/gem_scheduler: Add gem_scheduler test
This is intended to test the scheduler behaviour is correct. The subtests are -basic Tests that batch buffers of the same priority submitted to a ring execute in the order they are submitted. -read Submits a batch buffer with a read dependency to a buffer object to a ring which is held in the scheduler queue by a long running batch buffer. Submit batch buffers to other rings that have a read dependency to the same buffer object. Ensure they execute before the batch buffer being held up behind the long running batch buffer. -write Submits a batch buffer with a write dependency to a buffer object to a ring which is held in the scheduler queue by a long running batch buffer. Submit batch buffers to other rings that have a write dependency to the same buffer object. Submit batch buffers with no interdependencies to all rings. Ensure the batch buffers that have write dependencies are executed in submission order but the batch buffers without interdependencies do not get held up. Signed-off-by: Derek Morton--- tests/Makefile.sources | 1 + tests/gem_scheduler.c | 409 + 2 files changed, 410 insertions(+) create mode 100644 tests/gem_scheduler.c diff --git a/tests/Makefile.sources b/tests/Makefile.sources index df92586..439f62c 100644 --- a/tests/Makefile.sources +++ b/tests/Makefile.sources @@ -66,6 +66,7 @@ TESTS_progs_M = \ gem_request_retire \ gem_reset_stats \ gem_ringfill \ + gem_scheduler \ gem_set_tiling_vs_blt \ gem_softpin \ gem_stolen \ diff --git a/tests/gem_scheduler.c b/tests/gem_scheduler.c new file mode 100644 index 000..4824c13 --- /dev/null +++ b/tests/gem_scheduler.c @@ -0,0 +1,409 @@ +/* + * Copyright © 2016 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 + * + */ + +#include "igt.h" +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include + +IGT_TEST_DESCRIPTION("Check scheduler behaviour. Basic tests ensure independant " + "batch buffers of the same priority are executed in " + "submission order. Read-read tests ensure " + "batch buffers with a read dependency to the same buffer " + "object do not block each other. Write-write dependency " + "tests ensure batch buffers with a write dependency to a " + "buffer object will be executed in submission order but " + "will not block execution of other independant batch " + "buffers."); + +#define SEC_TO_NSEC (1000 * 1000 * 1000) + +struct ring { + const char *name; + int id; +} rings[] = { + { "render", I915_EXEC_RENDER }, + { "bsd",I915_EXEC_BSD }, + { "blt",I915_EXEC_BLT }, + { "vebox", I915_EXEC_VEBOX }, +}; + +#define NBR_RINGS (sizeof(rings)/sizeof(struct ring)) + +/* Basic test. Check batch buffers of the same priority and with no dependencies + * are executed in the order they are submitted. + */ +#define NBR_BASIC_FDs (3) +static void run_test_basic(int in_flight, int ringid) +{ + int fd[NBR_BASIC_FDs]; + int loop; + drm_intel_bufmgr *bufmgr[NBR_BASIC_FDs]; + uint32_t *delay_buf, *ts1_buf, *ts2_buf; + struct intel_batchbuffer *ts1_bb, *ts2_bb; + struct intel_batchbuffer **in_flight_bbs; + uint32_t calibrated_1s; + drm_intel_bo *delay_bo, *ts1_bo, *ts2_bo; + + in_flight_bbs = malloc(in_flight * sizeof(struct intel_batchbuffer *)); + igt_assert(in_flight_bbs); + + /* Need multiple i915 fd's. Scheduler will not change execution order of +* batch buffers from the same context. +*/ + for(loop=0; loop < NBR_BASIC_FDs; loop++)
[Intel-gfx] [PATCH i-g-t 1/4] lib/igt_bb_factory: Add igt_bb_factory library
Adds functions to create a number of different batch buffers to perform several functions including: Batch buffer which will run for a long duration to provide a delay on a specified ring. Function to calibrate the delay batch buffer to run for a specified period of time. Function to create a batch buffer which writes timestamps to a buffer object. Function to compare timestamps allowing for wrapping of the values. Intended for use by the gem_scheduler test initially but will be used by other tests in development. Signed-off-by: Derek Morton--- lib/Makefile.sources | 2 + lib/igt.h| 1 + lib/igt_bb_factory.c | 401 +++ lib/igt_bb_factory.h | 47 ++ 4 files changed, 451 insertions(+) create mode 100644 lib/igt_bb_factory.c create mode 100644 lib/igt_bb_factory.h diff --git a/lib/Makefile.sources b/lib/Makefile.sources index 4999868..c560b3e 100644 --- a/lib/Makefile.sources +++ b/lib/Makefile.sources @@ -7,6 +7,8 @@ libintel_tools_la_SOURCES = \ i915_reg.h \ i915_pciids.h \ igt.h \ + igt_bb_factory.c\ + igt_bb_factory.h\ igt_debugfs.c \ igt_debugfs.h \ igt_aux.c \ diff --git a/lib/igt.h b/lib/igt.h index 3be2551..0f29420 100644 --- a/lib/igt.h +++ b/lib/igt.h @@ -36,6 +36,7 @@ #include "igt_gt.h" #include "igt_kms.h" #include "igt_stats.h" +#include "igt_bb_factory.h" #include "instdone.h" #include "intel_batchbuffer.h" #include "intel_chipset.h" diff --git a/lib/igt_bb_factory.c b/lib/igt_bb_factory.c new file mode 100644 index 000..eea63c6 --- /dev/null +++ b/lib/igt_bb_factory.c @@ -0,0 +1,401 @@ +/* + * Copyright © 2016 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 + * + */ + +#include "igt.h" +#include "intel_batchbuffer.h" +#include +#include +#include + +#define SEC_TO_NSEC (1000 * 1000 * 1000) +#define DWORDS_TO_BYTES(x) ((x)*4) + +#define MI_STORE_REGISTER_MEM(LENGTH) ((0x024 << 23) | ((LENGTH - 2) & 0xff)) +#define MI_MATH(NrInst) ((0x01A << 23) | ((NrInst - 1) & 0x3f)) +#define MI_CONDITIONAL_BATCH_BUFFER_END ((0x036 << 23) | (1 << 21) | 2) +#define MI_COPY_MEM_MEM ((0x02E << 23) | (3)) + +#define ALU_LOAD(TO, FROM) ((0x080 << 20) | ((TO) << 10) | (FROM)) +#define ALU_SUB ( 0x101 << 20) +#define ALU_STORE(TO, FROM) ((0x180 << 20) | ((TO) << 10) | (FROM)) + +#define TIMESTAMP_offset (0x358) /* Elapsed time from system start */ +#define CTX_TIMESTAMP_offset (0x3A8) /* Elapsed Time from context creation */ +#define ALU_GPU_R0_LSB_offset (0x600) +#define ALU_GPU_R0_MSB_offset (0x604) +#define ALU_GPU_R1_LSB_offset (0x608) +#define ALU_GPU_R1_MSB_offset (0x60C) +#define ALU_GPU_R2_LSB_offset (0x610) +#define ALU_GPU_R2_MSB_offset (0x614) + +#define ALU_R0_ENCODING (0x00) +#define ALU_R1_ENCODING (0x01) +#define ALU_SRCA_ENCODING (0x20) +#define ALU_SRCB_ENCODING (0x21) +#define ALU_ACCU_ENCODING (0x31) + +/** + * SECTION:igt_bb_factory + * @short_description: Utility functions for creating batch buffers + * @title: Batch Buffer Factory + * @include: igt.h + * + * This library implements functions for creating batch buffers which may be + * useful to multiple tests. + */ + +static void check_gen_8(int fd) +{ + static bool checked = false; + if(!checked) { + igt_require(intel_gen(intel_get_drm_devid(fd)) >= 8); + checked = true; + } +} + +static int bb_address_size_dw(int fd) +{ + if (intel_gen(intel_get_drm_devid(fd)) >= 8) + return 2; + else + return 1; +} + +static uint32_t get_register_offset(int ringid) +{ + switch (ringid) { + case
[Intel-gfx] [PATCH i-g-t 3/4] igt/gem_ctx_param_basic: Updated to support scheduler priority interface
From: John HarrisonThe GPU scheduler has added an execution priority level to the context object. There is an IOCTL interface to allow user apps/libraries to set this priority. This patch updates the context paramter IOCTL test to include the new interface. For: VIZ-1587 Signed-off-by: John Harrison --- lib/ioctl_wrappers.h| 1 + tests/gem_ctx_param_basic.c | 34 +- 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/lib/ioctl_wrappers.h b/lib/ioctl_wrappers.h index 4d913c5..f1ef739 100644 --- a/lib/ioctl_wrappers.h +++ b/lib/ioctl_wrappers.h @@ -106,6 +106,7 @@ struct local_i915_gem_context_param { #define LOCAL_CONTEXT_PARAM_BAN_PERIOD 0x1 #define LOCAL_CONTEXT_PARAM_NO_ZEROMAP 0x2 #define LOCAL_CONTEXT_PARAM_GTT_SIZE 0x3 +#define LOCAL_CONTEXT_PARAM_PRIORITY 0x4 uint64_t value; }; void gem_context_require_ban_period(int fd); diff --git a/tests/gem_ctx_param_basic.c b/tests/gem_ctx_param_basic.c index b75800c..585a1a8 100644 --- a/tests/gem_ctx_param_basic.c +++ b/tests/gem_ctx_param_basic.c @@ -147,10 +147,42 @@ igt_main TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); } + ctx_param.param = LOCAL_CONTEXT_PARAM_PRIORITY; + + igt_subtest("priority-root-set") { + ctx_param.context = ctx; + ctx_param.value = 2048; + TEST_FAIL(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, EINVAL); + ctx_param.value = -2048; + TEST_FAIL(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, EINVAL); + ctx_param.value = 512; + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); + ctx_param.value = -512; + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); + ctx_param.value = 0; + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); + } + + igt_subtest("priority-non-root-set") { + igt_fork(child, 1) { + igt_drop_root(); + + ctx_param.context = ctx; + ctx_param.value = 512; + TEST_FAIL(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM, EPERM); + ctx_param.value = -512; + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); + ctx_param.value = 0; + TEST_SUCCESS(LOCAL_IOCTL_I915_GEM_CONTEXT_SETPARAM); + } + + igt_waitchildren(); + } + /* NOTE: This testcase intentionally tests for the next free parameter * to catch ABI extensions. Don't "fix" this testcase without adding all * the tests for the new param first. */ - ctx_param.param = LOCAL_CONTEXT_PARAM_GTT_SIZE + 1; + ctx_param.param = LOCAL_CONTEXT_PARAM_PRIORITY + 1; igt_subtest("invalid-param-get") { ctx_param.context = ctx; -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t 4/4] tests/gem_scheduler: Add subtests to test batch priority behaviour
Add subtests to test each ring to check batch buffers of a higher priority will be executed before batch buffers of a lower priority. Signed-off-by: Derek Morton--- tests/gem_scheduler.c | 34 -- 1 file changed, 28 insertions(+), 6 deletions(-) diff --git a/tests/gem_scheduler.c b/tests/gem_scheduler.c index 4824c13..febde01 100644 --- a/tests/gem_scheduler.c +++ b/tests/gem_scheduler.c @@ -39,7 +39,8 @@ IGT_TEST_DESCRIPTION("Check scheduler behaviour. Basic tests ensure independant " "batch buffers of the same priority are executed in " - "submission order. Read-read tests ensure " + "submission order. Priority tests ensure higher priority " + "batch buffers are executed first. Read-read tests ensure " "batch buffers with a read dependency to the same buffer " "object do not block each other. Write-write dependency " "tests ensure batch buffers with a write dependency to a " @@ -61,11 +62,13 @@ struct ring { #define NBR_RINGS (sizeof(rings)/sizeof(struct ring)) -/* Basic test. Check batch buffers of the same priority and with no dependencies - * are executed in the order they are submitted. +/* If 'priority' is set false, check batch buffers of the same priority and with + * no dependencies are executed in the order they are submitted. + * If 'priority' is set true, check batch buffers of higher priority are + * executed before batch buffers of lower priority. */ #define NBR_BASIC_FDs (3) -static void run_test_basic(int in_flight, int ringid) +static void run_test_basic(int in_flight, int ringid, bool priority) { int fd[NBR_BASIC_FDs]; int loop; @@ -95,6 +98,15 @@ static void run_test_basic(int in_flight, int ringid) intel_batchbuffer_free(noop_bb); } + if(priority) { + struct local_i915_gem_context_param param; + param.context = 0; /* Default context */ + param.size = 0; + param.param = LOCAL_CONTEXT_PARAM_PRIORITY; + param.value = 1000; + gem_context_set_param(fd[2], ); + } + /* Create buffer objects */ delay_bo = drm_intel_bo_alloc(bufmgr[0], "delay bo", BATCH_SZ, BATCH_SZ); igt_assert(delay_bo); @@ -146,7 +158,12 @@ static void run_test_basic(int in_flight, int ringid) igt_assert_f(igt_compare_timestamps(delay_buf[2], ts1_buf[0]), "Delay ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n", delay_buf[2], ts1_buf[0]); - igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts2_buf[0]), + if(priority) + igt_assert_f(igt_compare_timestamps(ts2_buf[0], ts1_buf[0]), +"TS2 ts (0x%08" PRIx32 ") > TS1 ts (0x%08" PRIx32 ")\n", +ts2_buf[0], ts1_buf[0]); + else + igt_assert_f(igt_compare_timestamps(ts1_buf[0], ts2_buf[0]), "TS1 ts (0x%08" PRIx32 ") > TS2 ts (0x%08" PRIx32 ")\n", ts1_buf[0], ts2_buf[0]); @@ -393,7 +410,12 @@ igt_main for (loop=0; loop < NBR_RINGS; loop++) igt_subtest_f("%s-basic", rings[loop].name) { - run_test_basic(in_flight, rings[loop].id); + run_test_basic(in_flight, rings[loop].id, false); + } + + for (loop=0; loop < NBR_RINGS; loop++) + igt_subtest_f("%s-priority", rings[loop].name) { + run_test_basic(in_flight, rings[loop].id, true); } for (loop=0; loop < NBR_RINGS; loop++) -- 1.9.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Avoid vblank counter for gen9+
On Thu, Feb 11, 2016 at 09:00:47AM -0800, Rodrigo Vivi wrote: > Framecounter register is read-only so DMC cannot restore it > after exiting DC5 and DC6. > > Easiest way to go is to avoid the counter and use vblank > interruptions for this platform and for all the following > ones since DMC came to stay. At least while we can't change > this register to read-write. > > Signed-off-by: Rodrigo Vivi> --- > drivers/gpu/drm/i915/i915_irq.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 25a8937..c294a4b 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -4556,7 +4556,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv) > > pm_qos_add_request(_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, > PM_QOS_DEFAULT_VALUE); > > - if (IS_GEN2(dev_priv)) { > + if (INTEL_INFO(dev_priv)->gen >= 9) { > + dev->max_vblank_count = 0; > + dev->driver->get_vblank_counter = g4x_get_vblank_counter; > + } else if (IS_GEN2(dev_priv)) { > dev->max_vblank_count = 0; > dev->driver->get_vblank_counter = i8xx_get_vblank_counter; > } else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) { > @@ -4572,7 +4575,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv) >* Gen2 doesn't have a hardware frame counter and so depends on >* vblank interrupts to produce sane vblank seuquence numbers. >*/ > - if (!IS_GEN2(dev_priv)) > + if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) I think this should be: if (INTEL_INFO(dev_priv)->gen < 9) If gen < 9, then IS_GEN2 is always true, also ! has higher precedence than >=, so you're essentially comparing whether the logical negation of INTEL_INFO(dev_priv)->gen is >= 9. Kind regards, David ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t v5] lib/igt_kms: Add COMIT_ATOMIC to igt_display_commit2()
From: Mayuresh GharpureCo-Author : Marius Vlad Co-Author : Pratik Vishwakarma So far we have had only two commit styles, COMMIT_LEGACY and COMMIT_UNIVERSAL. This patch adds another commit style COMMIT_ATOMIC which makes use of drmModeAtomicCommit() v2: (Marius) i)Set CRTC_ID to zero while disabling plane ii)Modified the log message in igt_atomic_prepare_plane_commit https://patchwork.freedesktop.org/patch/71945/ v3: (Marius)Set FB_ID to zero while disabling plane https://patchwork.freedesktop.org/patch/72179/ v4: (Maarten) Corrected the typo in commit message https://patchwork.freedesktop.org/patch/72598/ v5: Added check for DRM_CLIENT_CAP_ATOMIC in igt_display_init (Marius) i)Removed unused props from igt_display_init ii)Removed unused ret. Changed function to void iii)Declare the variable before checking if we have DRM_CLIENT_CAP_ATOMIC. Signed-off-by: Mayuresh Gharpure Signed-off-by: Pratik Vishwakarma --- lib/igt_kms.c | 317 +- lib/igt_kms.h | 71 - 2 files changed, 386 insertions(+), 2 deletions(-) diff --git a/lib/igt_kms.c b/lib/igt_kms.c index 90c8da7..8e201e8 100644 --- a/lib/igt_kms.c +++ b/lib/igt_kms.c @@ -145,6 +145,120 @@ const unsigned char* igt_kms_get_base_edid(void) * * Returns: an alternate edid block */ +static const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = { + "SRC_X", + "SRC_Y", + "SRC_W", + "SRC_H", + "CRTC_X", + "CRTC_Y", + "CRTC_W", + "CRTC_H", + "FB_ID", + "CRTC_ID", + "type", + "rotation" +}; + +static const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = { + "background_color" +}; + +static const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { + "scaling mode", + "DPMS" +}; + +/* + * Retrieve all the properies specified in props_name and store them into + * plane->atomic_props_plane. + */ +static void +igt_atomic_fill_plane_props(igt_display_t *display, igt_plane_t *plane, + int num_props, const char **prop_names) +{ + drmModeObjectPropertiesPtr props; + int i, j, fd; + + fd = display->drm_fd; + + props = drmModeObjectGetProperties(fd, plane->drm_plane->plane_id, DRM_MODE_OBJECT_PLANE); + igt_assert(props); + + for (i = 0; i < props->count_props; i++) { + drmModePropertyPtr prop = + drmModeGetProperty(fd, props->props[i]); + + for (j = 0; j < num_props; j++) { + if (strcmp(prop->name, prop_names[j]) != 0) + continue; + + plane->atomic_props_plane[j] = props->props[i]; + break; + } + + drmModeFreeProperty(prop); + } + + drmModeFreeObjectProperties(props); +} + +/* + * Retrieve all the properies specified in props_name and store them into + * config->atomic_props_crtc and config->atomic_props_connector. + */ +static void +igt_atomic_fill_props(igt_display_t *display, igt_output_t *output, + int num_crtc_props, const char **crtc_prop_names, + int num_connector_props, const char **conn_prop_names) +{ + drmModeObjectPropertiesPtr props; + int i, j, fd; + + fd = display->drm_fd; + + props = drmModeObjectGetProperties(fd, output->config.crtc->crtc_id, DRM_MODE_OBJECT_CRTC); + igt_assert(props); + + for (i = 0; i < props->count_props; i++) { + drmModePropertyPtr prop = + drmModeGetProperty(fd, props->props[i]); + + for (j = 0; j < num_crtc_props; j++) { + if (strcmp(prop->name, crtc_prop_names[j]) != 0) + continue; + + output->config.atomic_props_crtc[j] = props->props[i]; + break; + } + + drmModeFreeProperty(prop); + } + + drmModeFreeObjectProperties(props); + props = NULL; + props = drmModeObjectGetProperties(fd, output->config.connector->connector_id, DRM_MODE_OBJECT_CONNECTOR); + igt_assert(props); + + for (i = 0; i < props->count_props; i++) { + drmModePropertyPtr prop = + drmModeGetProperty(fd, props->props[i]); + + for (j = 0; j < num_connector_props; j++) { + if (strcmp(prop->name, conn_prop_names[j]) != 0) + continue; + + output->config.atomic_props_connector[j] = props->props[i]; + break; + } + + drmModeFreeProperty(prop); + } + +
Re: [Intel-gfx] [PATCH V4] drm/i915/skl: SKL CDCLK change on modeset tracking VCO
On Thu, Feb 11, 2016 at 03:22:08PM -0800, clinton.a.tay...@intel.com wrote: > From: Clint Taylor> > Track VCO frequency of SKL instead of the boot CDCLK and allow modeset > to set cdclk based on the max required pixel clock based on VCO > selected. > > The vco should be tracked at the atomic level and all CRTCs updated if > the required vco is changed. At this time the eDP pll is configured > inside the encoder which has no visibility into the atomic state. When > eDP v1.4 panel that require the 8640 vco are available this may need > to be investigated. > > V1: initial version > V2: add vco tracking in intel_dp_compute_config(), rename > skl_boot_cdclk. > V3: rebase, V2 feedback not possible as encoders are not aware of > atomic. > V4: track target vco is atomic state. modeset all CRTCs if vco changes > > Signed-off-by: Clint Taylor > Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= > --- > drivers/gpu/drm/i915/i915_drv.h |2 +- > drivers/gpu/drm/i915/intel_ddi.c |2 +- > drivers/gpu/drm/i915/intel_display.c | 97 > +- > drivers/gpu/drm/i915/intel_dp.c | 10 ++-- > drivers/gpu/drm/i915/intel_drv.h |4 ++ > 5 files changed, 97 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 8216665..f65dd1a 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1822,7 +1822,7 @@ struct drm_i915_private { > int num_fence_regs; /* 8 on pre-965, 16 otherwise */ > > unsigned int fsb_freq, mem_freq, is_ddr3; > - unsigned int skl_boot_cdclk; > + unsigned int skl_vco_freq; > unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq; > unsigned int max_dotclk_freq; > unsigned int hpll_freq; > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index 6d5b09f..285adab 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -2958,7 +2958,7 @@ void intel_ddi_pll_init(struct drm_device *dev) > int cdclk_freq; > > cdclk_freq = dev_priv->display.get_display_clock_speed(dev); > - dev_priv->skl_boot_cdclk = cdclk_freq; > + dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq); This should really read out the vco from the hardware. But I think we can leave that for later. The problem really is the 540MHz case since it can be using either vco frequency (IIRC). > if (skl_sanitize_cdclk(dev_priv)) > DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); > if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 9e2273b..ef4ac34 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5663,7 +5663,7 @@ static unsigned int skl_cdclk_decimal(unsigned int freq) > return (freq - 1000) / 500; > } > > -static unsigned int skl_cdclk_get_vco(unsigned int freq) > +unsigned int skl_cdclk_get_vco(unsigned int freq) > { > unsigned int i; > > @@ -5821,17 +5821,17 @@ void skl_uninit_cdclk(struct drm_i915_private > *dev_priv) > > void skl_init_cdclk(struct drm_i915_private *dev_priv) > { > - unsigned int required_vco; > - > /* DPLL0 not enabled (happens on early BIOS versions) */ > if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) { > /* enable DPLL0 */ > - required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk); > - skl_dpll0_enable(dev_priv, required_vco); > + if (dev_priv->skl_vco_freq != 8640) { > + dev_priv->skl_vco_freq = 8100; > + } > + skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq); > } > > /* set CDCLK to the frequency the BIOS chose */ > - skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk); > + skl_set_cdclk(dev_priv, (dev_priv->skl_vco_freq == 8100) ? 337500 : > 308570 ); We really shouldn't change the cdclk if there are active outputs. This whole area definitely needs more work, for BXT too I already have some BXT patches lined up in some branch that frob around these parts a bit, so maybe I should extend that stuff to SKL as well. In the meantime we don't want to cause a regression at least, so maybe we can just do something like: if (!LCPLL_ENABLE) { ... cdclk = vco == 8100 ? ...; } else { cdclk = dev_priv->cdclk_freq; } set_cdclk(cdclk); > > /* enable DBUF power */ > I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST); > @@ -5847,7 +5847,7 @@ int skl_sanitize_cdclk(struct drm_i915_private > *dev_priv) > { > uint32_t lcpll1 = I915_READ(LCPLL1_CTL); > uint32_t cdctl = I915_READ(CDCLK_CTL); > - int
Re: [Intel-gfx] [PATCH v10 2/4] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
On Fri, Feb 12, 2016 at 01:53:10PM +0200, Ville Syrjälä wrote: > On Thu, Jan 21, 2016 at 03:10:19PM -0800, Rafael Antognolli wrote: > > This module is heavily based on i2c-dev. Once loaded, it provides one > > dev node per DP AUX channel, named drm_dp_auxN, where N is an integer. > > > > It's possible to know which connector owns this aux channel by looking > > at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if > > the connector device pointer was correctly set in the aux helper struct. > > > > Two main operations are provided on the registers read and write. The > > address of the register to be read or written is given using lseek. The > > seek position is updated upon read or write. > > > > v2: > > - lseek is used to select the register to read/write > > - read/write are used instead of ioctl > > - no blocking_notifier is used, just a direct callback > > > > v3: > > - use drm_dp_aux_dev prefix for public functions > > - chardev is named drm_dp_auxN > > - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a > >time > > - remove notifier list from the implementation > > - option on menuconfig is now a boolean > > - add inline stub functions to avoid breakage when this option is disabled > > > > v4: > > - fix build system changes - actually disable this module when not > > selected. > > > > v5: > > - Use kref to avoid device closing while still in use > > - Don't use list, use an idr for storing aux_dev > > - Remove "connector" attribute > > - set aux.dev to the connector drm_connector device, instead of > >drm_device > > > > v6: > > - Use atomic_t for usage count > > - Use a mutex instead of spinlock for idr lock > > - Destroy chardev immediately on unregister > > - other minor suggestions from Ville > > > > v7: > > - style fixes > > - error handling fixes > > > > v8: > > - more error handling fixes > > > > v9: > > - remove module_init and module_exit, and add drm_dp_aux_dev_init/exit > > to drm_kms_helper_init/exit. > > > > Signed-off-by: Rafael Antognolli> > Only checked the init/exit stuff since I should have read the rest > many times by now. > > Reviewed-by: Ville Syrjälä Merged the first 2 patches to drm-misc, thanks. -Daniel > > > --- > > drivers/gpu/drm/Kconfig | 8 + > > drivers/gpu/drm/Makefile| 1 + > > drivers/gpu/drm/drm_dp_aux_dev.c| 368 > > > > drivers/gpu/drm/drm_dp_helper.c | 16 +- > > drivers/gpu/drm/drm_kms_helper_common.c | 15 +- > > include/drm/drm_dp_aux_dev.h| 62 ++ > > 6 files changed, 468 insertions(+), 2 deletions(-) > > create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c > > create mode 100644 include/drm/drm_dp_aux_dev.h > > > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > > index 59babd5..dff87ca 100644 > > --- a/drivers/gpu/drm/Kconfig > > +++ b/drivers/gpu/drm/Kconfig > > @@ -25,6 +25,14 @@ config DRM_MIPI_DSI > > bool > > depends on DRM > > > > +config DRM_DP_AUX_CHARDEV > > + bool "DRM DP AUX Interface" > > + depends on DRM > > + help > > + Choose this option to enable a /dev/drm_dp_auxN node that allows to > > + read and write values to arbitrary DPCD registers on the DP aux > > + channel. > > + > > config DRM_KMS_HELPER > > tristate > > depends on DRM > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > > index dfe513f..424fcb7 100644 > > --- a/drivers/gpu/drm/Makefile > > +++ b/drivers/gpu/drm/Makefile > > @@ -30,6 +30,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o > > drm_probe_helper.o \ > > drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o > > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > > +drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o > > > > obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o > > > > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c > > b/drivers/gpu/drm/drm_dp_aux_dev.c > > new file mode 100644 > > index 000..f73b38b > > --- /dev/null > > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c > > @@ -0,0 +1,368 @@ > > +/* > > + * 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
[Intel-gfx] [PULL] topic/drm-misc
Hi Dave, More drm-misc stuff: - vgaswitcheroo support for apple gmux from Lukas Wunner - checks for ->mode_fixup in non-atomic helpers from Carlos Palminha, plus removing dummy funcs from drivers. Carlos promised to follow up with more, since there's lots more silly dummy functions around. - dma-buf patches from Tiago, except the ioctl itself (that needed a respin to address review from David Herrmann) - encoder mask for atomic from Maarten - bunch of random things all over. Aside: the connector_mask stuff from Maarten that landed in 4.5 is blowing up in certain mst unplug cases. Maarten is looking into it. Cheers, Daniel The following changes since commit 10c1b6183a163aca59ba92b88f2b4c4cecd20d4c: drm/tegra: drop unused variable. (2016-02-09 11:17:37 +1000) are available in the git repository at: git://anongit.freedesktop.org/drm-intel tags/topic/drm-misc-2016-02-12 for you to fetch changes up to 382ab95d1af85381d8a5dff09b16a80c7e492534: drm/msm: remove unused variable (2016-02-11 11:48:39 +0100) Arnd Bergmann (1): drm/msm: remove unused variable Carlos Palminha (5): drm: fixes when i2c encoder slave mode_fixup is null. drm: fixes crct set_mode when encoder mode_fixup is null. drm/i2c/sil164: removed unnecessary code, mode_fixup is now optional. drm/i2c/tda998x: removed unnecessary code, mode_fixup is now optional. drm/bridge: removed dummy mode_fixup function from dw-hdmi. Daniel Thompson (1): drm: prime: Honour O_RDWR during prime-handle-to-fd Haixia Shi (1): drm/msm: remove the drm_device_is_unplugged check Insu Yun (1): ch7006: correctly handling failed allocation LABBE Corentin (1): drm: modes: add missing [drm] to message printing Lukas Wunner (13): vga_switcheroo: Add handler flags infrastructure vga_switcheroo: Add support for switching only the DDC apple-gmux: Track switch state apple-gmux: Add switch_ddc support drm/edid: Switch DDC when reading the EDID drm/i915: Switch DDC when reading the EDID drm/nouveau: Switch DDC when reading the EDID drm/radeon: Switch DDC when reading the EDID apple-gmux: Add helper for presence detect drm/i915: Defer probe if gmux is present but its driver isn't drm/nouveau: Defer probe if gmux is present but its driver isn't drm/radeon: Defer probe if gmux is present but its driver isn't apple-gmux: Fix build breakage if !CONFIG_ACPI Maarten Lankhorst (5): drm/core: Add drm_encoder_index. drm/core: Add drm_for_each_encoder_mask, v2. drm/i915: Do not touch best_encoder for load detect. drm/atomic: Do not unset crtc when an encoder is stolen drm/atomic: Add encoder_mask to crtc_state, v3. Rasmus Villemoes (1): drm/gma500: fix error path in gma_intel_setup_gmbus() Tiago Vignatti (3): dma-buf: Remove range-based flush drm/i915: Implement end_cpu_access drm/i915: Use CPU mapping for userspace dma-buf mmap() Ville Syrjälä (1): drm: Add drm_format_plane_width() and drm_format_plane_height() Documentation/DocBook/gpu.tmpl | 5 + Documentation/dma-buf-sharing.txt| 19 ++-- drivers/dma-buf/dma-buf.c| 13 +-- drivers/gpu/drm/amd/amdgpu/amdgpu_atpx_handler.c | 3 +- drivers/gpu/drm/bridge/dw-hdmi.c | 8 -- drivers/gpu/drm/drm_atomic_helper.c | 57 +-- drivers/gpu/drm/drm_crtc.c | 65 + drivers/gpu/drm/drm_crtc_helper.c| 10 +- drivers/gpu/drm/drm_edid.c | 26 + drivers/gpu/drm/drm_encoder_slave.c | 3 + drivers/gpu/drm/drm_modes.c | 3 +- drivers/gpu/drm/drm_prime.c | 10 +- drivers/gpu/drm/gma500/intel_gmbus.c | 2 +- drivers/gpu/drm/i2c/ch7006_drv.c | 2 + drivers/gpu/drm/i2c/sil164_drv.c | 9 -- drivers/gpu/drm/i2c/tda998x_drv.c| 9 -- drivers/gpu/drm/i915/i915_drv.c | 12 +++ drivers/gpu/drm/i915/i915_gem_dmabuf.c | 42 +++- drivers/gpu/drm/i915/intel_display.c | 5 +- drivers/gpu/drm/i915/intel_lvds.c| 8 +- drivers/gpu/drm/msm/msm_fbdev.c | 4 - drivers/gpu/drm/nouveau/nouveau_acpi.c | 2 +- drivers/gpu/drm/nouveau/nouveau_connector.c | 21 +++- drivers/gpu/drm/nouveau/nouveau_drm.c| 11 +++ drivers/gpu/drm/omapdrm/omap_gem_dmabuf.c| 4 +- drivers/gpu/drm/radeon/radeon_atpx_handler.c | 3 +- drivers/gpu/drm/radeon/radeon_connectors.c | 6 ++ drivers/gpu/drm/radeon/radeon_drv.c | 11 +++ drivers/gpu/drm/udl/udl_fb.c | 2 - drivers/gpu/vga/vga_switcheroo.c | 119 ++-
Re: [Intel-gfx] [PATCH i-g-t v5] lib/igt_kms: Add COMIT_ATOMIC to igt_display_commit2()
Please fix the typo in the subject. BR, Jani. On Fri, 12 Feb 2016, Pratik Vishwakarmawrote: > From: Mayuresh Gharpure > > Co-Author : Marius Vlad > Co-Author : Pratik Vishwakarma > > So far we have had only two commit styles, COMMIT_LEGACY > and COMMIT_UNIVERSAL. This patch adds another commit style > COMMIT_ATOMIC which makes use of drmModeAtomicCommit() > > v2: (Marius) > i)Set CRTC_ID to zero while disabling plane > ii)Modified the log message in igt_atomic_prepare_plane_commit > https://patchwork.freedesktop.org/patch/71945/ > > v3: (Marius)Set FB_ID to zero while disabling plane > https://patchwork.freedesktop.org/patch/72179/ > > v4: (Maarten) Corrected the typo in commit message > https://patchwork.freedesktop.org/patch/72598/ > > v5: Added check for DRM_CLIENT_CAP_ATOMIC in igt_display_init > (Marius) > i)Removed unused props from igt_display_init > ii)Removed unused ret. Changed function to void > iii)Declare the variable before checking if we have > DRM_CLIENT_CAP_ATOMIC. > > Signed-off-by: Mayuresh Gharpure > Signed-off-by: Pratik Vishwakarma > --- > lib/igt_kms.c | 317 > +- > lib/igt_kms.h | 71 - > 2 files changed, 386 insertions(+), 2 deletions(-) > > diff --git a/lib/igt_kms.c b/lib/igt_kms.c > index 90c8da7..8e201e8 100644 > --- a/lib/igt_kms.c > +++ b/lib/igt_kms.c > @@ -145,6 +145,120 @@ const unsigned char* igt_kms_get_base_edid(void) > * > * Returns: an alternate edid block > */ > +static const char *igt_plane_prop_names[IGT_NUM_PLANE_PROPS] = { > + "SRC_X", > + "SRC_Y", > + "SRC_W", > + "SRC_H", > + "CRTC_X", > + "CRTC_Y", > + "CRTC_W", > + "CRTC_H", > + "FB_ID", > + "CRTC_ID", > + "type", > + "rotation" > +}; > + > +static const char *igt_crtc_prop_names[IGT_NUM_CRTC_PROPS] = { > + "background_color" > +}; > + > +static const char *igt_connector_prop_names[IGT_NUM_CONNECTOR_PROPS] = { > + "scaling mode", > + "DPMS" > +}; > + > +/* > + * Retrieve all the properies specified in props_name and store them into > + * plane->atomic_props_plane. > + */ > +static void > +igt_atomic_fill_plane_props(igt_display_t *display, igt_plane_t *plane, > + int num_props, const char **prop_names) > +{ > + drmModeObjectPropertiesPtr props; > + int i, j, fd; > + > + fd = display->drm_fd; > + > + props = drmModeObjectGetProperties(fd, plane->drm_plane->plane_id, > DRM_MODE_OBJECT_PLANE); > + igt_assert(props); > + > + for (i = 0; i < props->count_props; i++) { > + drmModePropertyPtr prop = > + drmModeGetProperty(fd, props->props[i]); > + > + for (j = 0; j < num_props; j++) { > + if (strcmp(prop->name, prop_names[j]) != 0) > + continue; > + > + plane->atomic_props_plane[j] = props->props[i]; > + break; > + } > + > + drmModeFreeProperty(prop); > + } > + > + drmModeFreeObjectProperties(props); > +} > + > +/* > + * Retrieve all the properies specified in props_name and store them into > + * config->atomic_props_crtc and config->atomic_props_connector. > + */ > +static void > +igt_atomic_fill_props(igt_display_t *display, igt_output_t *output, > + int num_crtc_props, const char **crtc_prop_names, > + int num_connector_props, const char **conn_prop_names) > +{ > + drmModeObjectPropertiesPtr props; > + int i, j, fd; > + > + fd = display->drm_fd; > + > + props = drmModeObjectGetProperties(fd, output->config.crtc->crtc_id, > DRM_MODE_OBJECT_CRTC); > + igt_assert(props); > + > + for (i = 0; i < props->count_props; i++) { > + drmModePropertyPtr prop = > + drmModeGetProperty(fd, props->props[i]); > + > + for (j = 0; j < num_crtc_props; j++) { > + if (strcmp(prop->name, crtc_prop_names[j]) != 0) > + continue; > + > + output->config.atomic_props_crtc[j] = props->props[i]; > + break; > + } > + > + drmModeFreeProperty(prop); > + } > + > + drmModeFreeObjectProperties(props); > + props = NULL; > + props = drmModeObjectGetProperties(fd, > output->config.connector->connector_id, DRM_MODE_OBJECT_CONNECTOR); > + igt_assert(props); > + > + for (i = 0; i < props->count_props; i++) { > + drmModePropertyPtr prop = > + drmModeGetProperty(fd, props->props[i]); > + > + for (j = 0; j < num_connector_props; j++) { > + if (strcmp(prop->name,
[Intel-gfx] [PATCH v2] drm/i915: Execlist irq handler micro optimisations
From: Tvrtko UrsulinAssorted changes most likely without any practical effect apart from a tiny reduction in generated code for the interrupt handler and request submission. * Remove needless initialization. * Improve cache locality by reorganizing code and/or using branch hints to keep unexpected or error conditions out of line. * Favor busy submit path vs. empty queue. * Less branching in hot-paths. v2: * Avoid mmio reads when possible. (Chris Wilson) * Use natural integer size for csb indices. * Remove useless return value from execlists_update_context. * Extract 32-bit ppgtt PDPs update so it is out of line and shared with two callers. * Grab forcewake across all mmio operations to ease the load on uncore lock and use chepear mmio ops. Version 2 now makes the irq handling code path ~20% smaller on 48-bit PPGTT hardware, and a little bit less elsewhere. Hot paths are mostly in-line now and hammering on the uncore spinlock is greatly reduced together with mmio traffic to an extent. Signed-off-by: Tvrtko Ursulin Cc: Chris Wilson --- drivers/gpu/drm/i915/intel_lrc.c| 187 +--- drivers/gpu/drm/i915/intel_ringbuffer.h | 3 +- 2 files changed, 99 insertions(+), 91 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index a513a75d1fc9..a474e00be50e 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -269,6 +269,9 @@ logical_ring_init_platform_invariants(struct intel_engine_cs *ring) { struct drm_device *dev = ring->dev; + if (IS_GEN8(dev) || IS_GEN9(dev)) + ring->idle_lite_restore_wa = ~0; + ring->disable_lite_restore_wa = (IS_SKL_REVID(dev, 0, SKL_REVID_B0) || IS_BXT_REVID(dev, 0, BXT_REVID_A1)) && (ring->id == VCS || ring->id == VCS2); @@ -372,8 +375,6 @@ static void execlists_elsp_write(struct drm_i915_gem_request *rq0, rq0->elsp_submitted++; /* You must always write both descriptors in the order below. */ - spin_lock(_priv->uncore.lock); - intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL); I915_WRITE_FW(RING_ELSP(ring), upper_32_bits(desc[1])); I915_WRITE_FW(RING_ELSP(ring), lower_32_bits(desc[1])); @@ -383,11 +384,18 @@ static void execlists_elsp_write(struct drm_i915_gem_request *rq0, /* ELSP is a wo register, use another nearby reg for posting */ POSTING_READ_FW(RING_EXECLIST_STATUS_LO(ring)); - intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL); - spin_unlock(_priv->uncore.lock); } -static int execlists_update_context(struct drm_i915_gem_request *rq) +static void +execlists_update_context_pdps(struct i915_hw_ppgtt *ppgtt, u32 *reg_state) +{ + ASSIGN_CTX_PDP(ppgtt, reg_state, 3); + ASSIGN_CTX_PDP(ppgtt, reg_state, 2); + ASSIGN_CTX_PDP(ppgtt, reg_state, 1); + ASSIGN_CTX_PDP(ppgtt, reg_state, 0); +} + +static void execlists_update_context(struct drm_i915_gem_request *rq) { struct intel_engine_cs *ring = rq->ring; struct i915_hw_ppgtt *ppgtt = rq->ctx->ppgtt; @@ -395,19 +403,13 @@ static int execlists_update_context(struct drm_i915_gem_request *rq) reg_state[CTX_RING_TAIL+1] = rq->tail; - if (ppgtt && !USES_FULL_48BIT_PPGTT(ppgtt->base.dev)) { - /* True 32b PPGTT with dynamic page allocation: update PDP -* registers and point the unallocated PDPs to scratch page. -* PML4 is allocated during ppgtt init, so this is not needed -* in 48-bit mode. -*/ - ASSIGN_CTX_PDP(ppgtt, reg_state, 3); - ASSIGN_CTX_PDP(ppgtt, reg_state, 2); - ASSIGN_CTX_PDP(ppgtt, reg_state, 1); - ASSIGN_CTX_PDP(ppgtt, reg_state, 0); - } - - return 0; + /* True 32b PPGTT with dynamic page allocation: update PDP +* registers and point the unallocated PDPs to scratch page. +* PML4 is allocated during ppgtt init, so this is not needed +* in 48-bit mode. +*/ + if (ppgtt && !USES_FULL_48BIT_PPGTT(ppgtt->base.dev)) + execlists_update_context_pdps(ppgtt, reg_state); } static void execlists_submit_requests(struct drm_i915_gem_request *rq0, @@ -424,7 +426,7 @@ static void execlists_submit_requests(struct drm_i915_gem_request *rq0, static void execlists_context_unqueue(struct intel_engine_cs *ring) { struct drm_i915_gem_request *req0 = NULL, *req1 = NULL; - struct drm_i915_gem_request *cursor = NULL, *tmp = NULL; + struct drm_i915_gem_request *cursor, *tmp; assert_spin_locked(>execlist_lock); @@ -434,9 +436,6 @@ static void execlists_context_unqueue(struct intel_engine_cs *ring) */
Re: [Intel-gfx] ✗ Fi.CI.BAT: warning for Add drm_dp_aux chardev support. (rev5)
On Fri, Jan 22, 2016 at 09:15:31AM -, Patchwork wrote: > == Summary == > > Built on 8fe9e785ae04fa7c37f7935cff12d62e38054b60 drm-intel-nightly: > 2016y-01m-21d-11h-02m-42s UTC integration manifest > > Test kms_flip: > Subgroup basic-flip-vs-dpms: > pass -> DMESG-WARN (ilk-hp8440p) The usual ilk underrun https://bugs.freedesktop.org/show_bug.cgi?id=93787 > Subgroup basic-flip-vs-modeset: > pass -> DMESG-WARN (skl-i7k-2) This is the known "DMC doesn't know what state it's in" problem. > pass -> DMESG-WARN (ilk-hp8440p) Another ilk underrun. > > bdw-nuci7total:140 pass:131 dwarn:0 dfail:0 fail:0 skip:9 > bdw-ultratotal:143 pass:137 dwarn:0 dfail:0 fail:0 skip:6 > bsw-nuc-2total:143 pass:119 dwarn:0 dfail:0 fail:0 skip:24 > byt-nuc total:143 pass:128 dwarn:0 dfail:0 fail:0 skip:15 > hsw-brixbox total:143 pass:136 dwarn:0 dfail:0 fail:0 skip:7 > ilk-hp8440p total:143 pass:102 dwarn:3 dfail:0 fail:0 skip:38 > ivb-t430stotal:143 pass:137 dwarn:0 dfail:0 fail:0 skip:6 > skl-i5k-2total:143 pass:134 dwarn:1 dfail:0 fail:0 skip:8 > skl-i7k-2total:143 pass:133 dwarn:2 dfail:0 fail:0 skip:8 > snb-dellxps total:143 pass:129 dwarn:0 dfail:0 fail:0 skip:14 > snb-x220ttotal:143 pass:129 dwarn:0 dfail:0 fail:1 skip:13 > > Results at /archive/results/CI_IGT_test/Patchwork_1244/ > > ___ > 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 https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Set invert bit for hpd based on VBT
This patch sets the invert bit for hpd detection for each port based on VBT configuration. Since each AOB can be designed to depend on invert bit or not, it is expected if an AOB requires invert bit, the user will set respective bit in VBT. v2: Separated VBT parsing from the rest of the logic. (Jani) Signed-off-by: Sivakumar ThulasimaniSigned-off-by: Durgadoss R Signed-off-by: Shubhangi Shrivastava --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 43 +++ drivers/gpu/drm/i915/i915_reg.h | 9 drivers/gpu/drm/i915/intel_bios.c | 42 ++ 4 files changed, 95 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8216665..457f175 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3393,6 +3393,7 @@ extern void intel_i2c_reset(struct drm_device *dev); /* intel_bios.c */ int intel_bios_init(struct drm_i915_private *dev_priv); bool intel_bios_is_valid_vbt(const void *buf, size_t size); +bool intel_bios_is_port_hpd_inverted(struct drm_device *dev, enum port port); /* intel_opregion.c */ #ifdef CONFIG_ACPI diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 25a8937..fb95fb0 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -36,6 +36,7 @@ #include "i915_drv.h" #include "i915_trace.h" #include "intel_drv.h" +#include "intel_bios.h" /** * DOC: interrupt handling @@ -3424,6 +3425,47 @@ static void ibx_hpd_irq_setup(struct drm_device *dev) I915_WRITE(PCH_PORT_HOTPLUG, hotplug); } +/* + * For BXT invert bit has to be set based on AOB design + * for HPD detection logic, update it based on VBT fields. + */ +static void bxt_hpd_set_invert(struct drm_device *dev, u32 hotplug_port) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + int reg_val, val = 0; + enum port port; + + for (port = PORT_A; port <= PORT_C; port++) { + + /* Proceed only if invert bit is set */ + if (intel_bios_is_port_hpd_inverted(dev, port)) { + switch (port) { + case PORT_A: + if (hotplug_port & BXT_DE_PORT_HP_DDIA) + val |= BXT_DDIA_HPD_INVERT; + break; + case PORT_B: + if (hotplug_port & BXT_DE_PORT_HP_DDIB) + val |= BXT_DDIB_HPD_INVERT; + break; + case PORT_C: + if (hotplug_port & BXT_DE_PORT_HP_DDIC) + val |= BXT_DDIC_HPD_INVERT; + break; + default: + DRM_ERROR("HPD invert set for invalid port %d\n", + port); + break; + } + } + } + reg_val = I915_READ(BXT_HOTPLUG_CTL); + DRM_DEBUG_KMS("Invert bit setting: hp_ctl:%x hp_port:%x val:%x\n", + reg_val, hotplug_port, val); + reg_val &= ~BXT_DDI_HPD_INVERT_MASK; + I915_WRITE(BXT_HOTPLUG_CTL, reg_val | val); +} + static void spt_hpd_irq_setup(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -3494,6 +3536,7 @@ static void bxt_hpd_irq_setup(struct drm_device *dev) hotplug |= PORTC_HOTPLUG_ENABLE | PORTB_HOTPLUG_ENABLE | PORTA_HOTPLUG_ENABLE; I915_WRITE(PCH_PORT_HOTPLUG, hotplug); + bxt_hpd_set_invert(dev, enabled_irqs); } static void ibx_irq_postinstall(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 6732fc1..66cf92e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5940,6 +5940,15 @@ enum skl_disp_power_wells { #define GEN8_PCU_IIR _MMIO(0x444e8) #define GEN8_PCU_IER _MMIO(0x444ec) +/* BXT hotplug control */ +#define BXT_HOTPLUG_CTL_MMIO(0xC4030) +#define BXT_DDIA_HPD_INVERT(1 << 27) +#define BXT_DDIC_HPD_INVERT(1 << 11) +#define BXT_DDIB_HPD_INVERT(1 << 3) +#define BXT_DDI_HPD_INVERT_MASK(BXT_DDIA_HPD_INVERT | \ +BXT_DDIB_HPD_INVERT | \ +BXT_DDIC_HPD_INVERT) + #define ILK_DISPLAY_CHICKEN2 _MMIO(0x42004) /* Required on all Ironlake and Sandybridge according to the B-Spec. */ #define ILK_ELPIN_409_SELECT (1 << 25) diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index a26d4b4..24d0077 100644 ---
[Intel-gfx] [PATCH 1/2] drm/i915: Update VBT fields for child devices
This patch adds new fields that are not yet added in drm code in child devices struct Signed-off-by: Sivakumar ThulasimaniSigned-off-by: Durgadoss R Signed-off-by: Shubhangi Shrivastava --- drivers/gpu/drm/i915/intel_bios.c | 15 ++- drivers/gpu/drm/i915/intel_bios.h | 20 +--- 2 files changed, 27 insertions(+), 8 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index bf62a19..a26d4b4 100644 --- a/drivers/gpu/drm/i915/intel_bios.c +++ b/drivers/gpu/drm/i915/intel_bios.c @@ -1124,7 +1124,7 @@ static void parse_ddi_port(struct drm_i915_private *dev_priv, enum port port, } /* Parse the I_boost config for SKL and above */ - if (bdb->version >= 196 && (child->common.flags_1 & IBOOST_ENABLE)) { + if (bdb->version >= 196 && child->common.iboost) { info->dp_boost_level = translate_iboost(child->common.iboost_level & 0xF); DRM_DEBUG_KMS("VBT (e)DP boost level for port %c: %d\n", port_name(port), info->dp_boost_level); @@ -1250,6 +1250,19 @@ parse_device_mapping(struct drm_i915_private *dev_priv, */ memcpy(child_dev_ptr, p_child, min_t(size_t, p_defs->child_dev_size, sizeof(*p_child))); + + /* +* copied full block, now init values when they are not +* available in current version +*/ + if (bdb->version < 196) { + /* Set default values for bits added from v196 */ + child_dev_ptr->common.iboost = 0; + child_dev_ptr->common.hpd_invert = 0; + } + + if (bdb->version < 192) + child_dev_ptr->common.lspcon = 0; } return; } diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h index 350d4e0..833b82b 100644 --- a/drivers/gpu/drm/i915/intel_bios.h +++ b/drivers/gpu/drm/i915/intel_bios.h @@ -250,9 +250,6 @@ struct old_child_dev_config { * versions. Notice that the meaning of the contents contents may still change, * but at least the offsets are consistent. */ -/* Definitions for flags_1 */ -#define IBOOST_ENABLE (1<<3) - struct common_child_dev_config { u16 handle; u16 device_type; @@ -261,10 +258,19 @@ struct common_child_dev_config { u8 not_common2[2]; u8 ddc_pin; u16 edid_ptr; - u8 obsolete; - u8 flags_1; - u8 not_common3[13]; - u8 iboost_level; + u8 dvo_cfg; /* See DEVICE_CFG_* above */ + u8 efp_routed:1; + u8 lane_reversal:1; + u8 lspcon:1; + u8 iboost:1; + u8 hpd_invert:1; + u8 flag_reserved:3; + u8 hdmi_support:1; + u8 dp_support:1; + u8 tmds_support:1; + u8 support_reserved:5; + u8 not_common3[13]; + u8 iboost_level; } __packed; -- 2.6.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Set invert bit for hpd based on VBT
This patch sets the invert bit for hpd detection for each port based on VBT configuration. Since each AOB can be designed to depend on invert bit or not, it is expected if an AOB requires invert bit, the user will set respective bit in VBT. v2: Separated VBT parsing from the rest of the logic. (Jani) Signed-off-by: Sivakumar ThulasimaniSigned-off-by: Durgadoss R Signed-off-by: Shubhangi Shrivastava --- drivers/gpu/drm/i915/i915_drv.h | 1 + drivers/gpu/drm/i915/i915_irq.c | 43 +++ drivers/gpu/drm/i915/i915_reg.h | 9 drivers/gpu/drm/i915/intel_bios.c | 42 ++ 4 files changed, 95 insertions(+) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8216665..457f175 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -3393,6 +3393,7 @@ extern void intel_i2c_reset(struct drm_device *dev); /* intel_bios.c */ int intel_bios_init(struct drm_i915_private *dev_priv); bool intel_bios_is_valid_vbt(const void *buf, size_t size); +bool intel_bios_is_port_hpd_inverted(struct drm_device *dev, enum port port); /* intel_opregion.c */ #ifdef CONFIG_ACPI diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 25a8937..fb95fb0 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -36,6 +36,7 @@ #include "i915_drv.h" #include "i915_trace.h" #include "intel_drv.h" +#include "intel_bios.h" /** * DOC: interrupt handling @@ -3424,6 +3425,47 @@ static void ibx_hpd_irq_setup(struct drm_device *dev) I915_WRITE(PCH_PORT_HOTPLUG, hotplug); } +/* + * For BXT invert bit has to be set based on AOB design + * for HPD detection logic, update it based on VBT fields. + */ +static void bxt_hpd_set_invert(struct drm_device *dev, u32 hotplug_port) +{ + struct drm_i915_private *dev_priv = dev->dev_private; + int reg_val, val = 0; + enum port port; + + for (port = PORT_A; port <= PORT_C; port++) { + + /* Proceed only if invert bit is set */ + if (intel_bios_is_port_hpd_inverted(dev, port)) { + switch (port) { + case PORT_A: + if (hotplug_port & BXT_DE_PORT_HP_DDIA) + val |= BXT_DDIA_HPD_INVERT; + break; + case PORT_B: + if (hotplug_port & BXT_DE_PORT_HP_DDIB) + val |= BXT_DDIB_HPD_INVERT; + break; + case PORT_C: + if (hotplug_port & BXT_DE_PORT_HP_DDIC) + val |= BXT_DDIC_HPD_INVERT; + break; + default: + DRM_ERROR("HPD invert set for invalid port %d\n", + port); + break; + } + } + } + reg_val = I915_READ(BXT_HOTPLUG_CTL); + DRM_DEBUG_KMS("Invert bit setting: hp_ctl:%x hp_port:%x val:%x\n", + reg_val, hotplug_port, val); + reg_val &= ~BXT_DDI_HPD_INVERT_MASK; + I915_WRITE(BXT_HOTPLUG_CTL, reg_val | val); +} + static void spt_hpd_irq_setup(struct drm_device *dev) { struct drm_i915_private *dev_priv = dev->dev_private; @@ -3494,6 +3536,7 @@ static void bxt_hpd_irq_setup(struct drm_device *dev) hotplug |= PORTC_HOTPLUG_ENABLE | PORTB_HOTPLUG_ENABLE | PORTA_HOTPLUG_ENABLE; I915_WRITE(PCH_PORT_HOTPLUG, hotplug); + bxt_hpd_set_invert(dev, enabled_irqs); } static void ibx_irq_postinstall(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 6732fc1..66cf92e 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -5940,6 +5940,15 @@ enum skl_disp_power_wells { #define GEN8_PCU_IIR _MMIO(0x444e8) #define GEN8_PCU_IER _MMIO(0x444ec) +/* BXT hotplug control */ +#define BXT_HOTPLUG_CTL_MMIO(0xC4030) +#define BXT_DDIA_HPD_INVERT(1 << 27) +#define BXT_DDIC_HPD_INVERT(1 << 11) +#define BXT_DDIB_HPD_INVERT(1 << 3) +#define BXT_DDI_HPD_INVERT_MASK(BXT_DDIA_HPD_INVERT | \ +BXT_DDIB_HPD_INVERT | \ +BXT_DDIC_HPD_INVERT) + #define ILK_DISPLAY_CHICKEN2 _MMIO(0x42004) /* Required on all Ironlake and Sandybridge according to the B-Spec. */ #define ILK_ELPIN_409_SELECT (1 << 25) diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c index a26d4b4..24d0077 100644 ---
Re: [Intel-gfx] [PATCH V4] drm/i915/skl: SKL CDCLK change on modeset tracking VCO
On Thu, Feb 11, 2016 at 05:11:52PM -0800, Marc Herbert wrote: > [I'm cheating and doing this code review with the author watching over my > shoulder] > > On 11/02/16 15:22, clinton.a.tay...@intel.com wrote: > > From: Clint Taylor> > > > Track VCO frequency of SKL instead of the boot CDCLK and allow modeset > > to set cdclk based on the max required pixel clock based on VCO > > selected. > > Nit: the main point shouldn't come second. > > > The vco should be tracked at the atomic level and all CRTCs updated if > > the required vco is changed. At this time the eDP pll is configured > > inside the encoder which has no visibility into the atomic state. > > should be -> is > > > > When eDP v1.4 panel that require the 8640 vco are available this may need > > to be investigated. > > Just say that 8640 can't be tested yet. > > > V1: initial version > > V2: add vco tracking in intel_dp_compute_config(), rename > > skl_boot_cdclk. > > V3: rebase, V2 feedback not possible as encoders are not aware of > > atomic. > > V4: track target vco is atomic state. modeset all CRTCs if vco changes > > > > Signed-off-by: Clint Taylor > > Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= > > --- > > drivers/gpu/drm/i915/i915_drv.h |2 +- > > drivers/gpu/drm/i915/intel_ddi.c |2 +- > > drivers/gpu/drm/i915/intel_display.c | 97 > > +- > > drivers/gpu/drm/i915/intel_dp.c | 10 ++-- > > drivers/gpu/drm/i915/intel_drv.h |4 ++ > > 5 files changed, 97 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h > > b/drivers/gpu/drm/i915/i915_drv.h > > index 8216665..f65dd1a 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.h > > +++ b/drivers/gpu/drm/i915/i915_drv.h > > @@ -1822,7 +1822,7 @@ struct drm_i915_private { > > int num_fence_regs; /* 8 on pre-965, 16 otherwise */ > > > > unsigned int fsb_freq, mem_freq, is_ddr3; > > - unsigned int skl_boot_cdclk; > > + unsigned int skl_vco_freq; > > unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq; > > unsigned int max_dotclk_freq; > > unsigned int hpll_freq; > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > > b/drivers/gpu/drm/i915/intel_ddi.c > > index 6d5b09f..285adab 100644 > > --- a/drivers/gpu/drm/i915/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/intel_ddi.c > > @@ -2958,7 +2958,7 @@ void intel_ddi_pll_init(struct drm_device *dev) > > int cdclk_freq; > > > > cdclk_freq = dev_priv->display.get_display_clock_speed(dev); > > - dev_priv->skl_boot_cdclk = cdclk_freq; > > + dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq); > > - skl_cdclk_get_vco() and skl_cdclk_frequencies[] should probably be > renamed to: > + skl_get_bios_cdclk_vco() and skl_bios_cdclk_frequencies[] > > to avoid confusion with the (different) mapping used in the new > skl_modeset_calc_cdclk() > function below. Let's not. This stuff doesn't really have anything to do with the BIOS. We just want to read out the current hardware state, nothing more. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v10 2/4] drm/dp: Add a drm_aux-dev module for reading/writing dpcd registers.
On Thu, Jan 21, 2016 at 03:10:19PM -0800, Rafael Antognolli wrote: > This module is heavily based on i2c-dev. Once loaded, it provides one > dev node per DP AUX channel, named drm_dp_auxN, where N is an integer. > > It's possible to know which connector owns this aux channel by looking > at the respective sysfs /sys/class/drm_aux_dev/drm_dp_auxN/connector, if > the connector device pointer was correctly set in the aux helper struct. > > Two main operations are provided on the registers read and write. The > address of the register to be read or written is given using lseek. The > seek position is updated upon read or write. > > v2: > - lseek is used to select the register to read/write > - read/write are used instead of ioctl > - no blocking_notifier is used, just a direct callback > > v3: > - use drm_dp_aux_dev prefix for public functions > - chardev is named drm_dp_auxN > - read/write don't allocate a buffer anymore, and transfer up to 16 bytes a >time > - remove notifier list from the implementation > - option on menuconfig is now a boolean > - add inline stub functions to avoid breakage when this option is disabled > > v4: > - fix build system changes - actually disable this module when not selected. > > v5: > - Use kref to avoid device closing while still in use > - Don't use list, use an idr for storing aux_dev > - Remove "connector" attribute > - set aux.dev to the connector drm_connector device, instead of >drm_device > > v6: > - Use atomic_t for usage count > - Use a mutex instead of spinlock for idr lock > - Destroy chardev immediately on unregister > - other minor suggestions from Ville > > v7: > - style fixes > - error handling fixes > > v8: > - more error handling fixes > > v9: > - remove module_init and module_exit, and add drm_dp_aux_dev_init/exit > to drm_kms_helper_init/exit. > > Signed-off-by: Rafael AntognolliOnly checked the init/exit stuff since I should have read the rest many times by now. Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/Kconfig | 8 + > drivers/gpu/drm/Makefile| 1 + > drivers/gpu/drm/drm_dp_aux_dev.c| 368 > > drivers/gpu/drm/drm_dp_helper.c | 16 +- > drivers/gpu/drm/drm_kms_helper_common.c | 15 +- > include/drm/drm_dp_aux_dev.h| 62 ++ > 6 files changed, 468 insertions(+), 2 deletions(-) > create mode 100644 drivers/gpu/drm/drm_dp_aux_dev.c > create mode 100644 include/drm/drm_dp_aux_dev.h > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 59babd5..dff87ca 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -25,6 +25,14 @@ config DRM_MIPI_DSI > bool > depends on DRM > > +config DRM_DP_AUX_CHARDEV > + bool "DRM DP AUX Interface" > + depends on DRM > + help > + Choose this option to enable a /dev/drm_dp_auxN node that allows to > + read and write values to arbitrary DPCD registers on the DP aux > + channel. > + > config DRM_KMS_HELPER > tristate > depends on DRM > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index dfe513f..424fcb7 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -30,6 +30,7 @@ drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o > drm_probe_helper.o \ > drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > +drm_kms_helper-$(CONFIG_DRM_DP_AUX_CHARDEV) += drm_dp_aux_dev.o > > obj-$(CONFIG_DRM_KMS_HELPER) += drm_kms_helper.o > > diff --git a/drivers/gpu/drm/drm_dp_aux_dev.c > b/drivers/gpu/drm/drm_dp_aux_dev.c > new file mode 100644 > index 000..f73b38b > --- /dev/null > +++ b/drivers/gpu/drm/drm_dp_aux_dev.c > @@ -0,0 +1,368 @@ > +/* > + * 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
Re: [Intel-gfx] [PATCH v10 1/4] drm/kms_helper: Add a common place to call init and exit functions.
On Thu, Jan 21, 2016 at 03:10:18PM -0800, Rafael Antognolli wrote: > The module_init and module_exit functions will start here, and call the > subsequent init's and exit's. > > v10: > - Keep __init on drm_fb_helper init function. > - Move MODULE_* macros to the common file. > > Signed-off-by: Rafael AntognolliLooks all right to me. Reviewed-by: Ville Syrjälä > --- > drivers/gpu/drm/Makefile| 4 ++- > drivers/gpu/drm/drm_crtc_helper.c | 3 --- > drivers/gpu/drm/drm_fb_helper.c | 9 +++ > drivers/gpu/drm/drm_kms_helper_common.c | 47 > + > include/drm/drm_fb_helper.h | 6 + > 5 files changed, 60 insertions(+), 9 deletions(-) > create mode 100644 drivers/gpu/drm/drm_kms_helper_common.c > > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index f858aa2..dfe513f 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -24,7 +24,9 @@ drm-$(CONFIG_AGP) += drm_agpsupport.o > drm-y += $(drm-m) > > drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \ > - drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o > + drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \ > + drm_kms_helper_common.o > + > drm_kms_helper-$(CONFIG_DRM_LOAD_EDID_FIRMWARE) += drm_edid_load.o > drm_kms_helper-$(CONFIG_DRM_FBDEV_EMULATION) += drm_fb_helper.o > drm_kms_helper-$(CONFIG_DRM_KMS_CMA_HELPER) += drm_fb_cma_helper.o > diff --git a/drivers/gpu/drm/drm_crtc_helper.c > b/drivers/gpu/drm/drm_crtc_helper.c > index 5d4bc64..baac181 100644 > --- a/drivers/gpu/drm/drm_crtc_helper.c > +++ b/drivers/gpu/drm/drm_crtc_helper.c > @@ -73,9 +73,6 @@ > * _crtc_helper_funcs, struct _encoder_helper_funcs and struct > * _connector_helper_funcs. > */ > -MODULE_AUTHOR("David Airlie, Jesse Barnes"); > -MODULE_DESCRIPTION("DRM KMS helper"); > -MODULE_LICENSE("GPL and additional rights"); > > /** > * drm_helper_move_panel_connectors_to_head() - move panels to the front in > the > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 1e103c4..c27b964 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -2175,9 +2175,9 @@ EXPORT_SYMBOL(drm_fb_helper_hotplug_event); > * but the module doesn't depend on any fb console symbols. At least > * attempt to load fbcon to avoid leaving the system without a usable > console. > */ > -#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT) > -static int __init drm_fb_helper_modinit(void) > +int __init drm_fb_helper_modinit(void) > { > +#if defined(CONFIG_FRAMEBUFFER_CONSOLE_MODULE) && !defined(CONFIG_EXPERT) > const char *name = "fbcon"; > struct module *fbcon; > > @@ -2187,8 +2187,7 @@ static int __init drm_fb_helper_modinit(void) > > if (!fbcon) > request_module_nowait(name); > +#endif > return 0; > } > - > -module_init(drm_fb_helper_modinit); > -#endif > +EXPORT_SYMBOL(drm_fb_helper_modinit); > diff --git a/drivers/gpu/drm/drm_kms_helper_common.c > b/drivers/gpu/drm/drm_kms_helper_common.c > new file mode 100644 > index 000..d361005 > --- /dev/null > +++ b/drivers/gpu/drm/drm_kms_helper_common.c > @@ -0,0 +1,47 @@ > +/* > + * 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: > + *Rafael Antognolli > + * > + */ > + > +#include > +#include > + > +MODULE_AUTHOR("David Airlie, Jesse Barnes"); > +MODULE_DESCRIPTION("DRM KMS helper"); > +MODULE_LICENSE("GPL and additional rights"); > + > +static int __init drm_kms_helper_init(void) > +{ > + /* Call init functions from specific kms helpers here */ > +
Re: [Intel-gfx] [PATCH v4 8/8] drm/i915: Remove vblank wait from hsw_enable_ips.
Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu: > intel_post_plane_update did an extra vblank wait that's no longer > needed when enabling ips. > > Signed-off-by: Maarten Lankhorst> --- > drivers/gpu/drm/i915/intel_display.c | 3 --- > 1 file changed, 3 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 6bb1f5dbc7a0..19a8d376d63e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4569,9 +4569,6 @@ void hsw_enable_ips(struct intel_crtc *crtc) > if (!crtc->config->ips_enabled) > return; > > - /* We can only enable IPS after we enable a plane and wait > for a vblank */ Perhaps we can keep the comment, but change it into something like: /* We can only enable IPS after we enable a plane and wait for a vblank, but we don't need the wait here because of XYZ. */ > - intel_wait_for_vblank(dev, crtc->pipe); > - > assert_plane_enabled(dev_priv, crtc->plane); > if (IS_BROADWELL(dev)) { > mutex_lock(_priv->rps.hw_lock); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915/guc: Simplify code by keeping kmap of guc_client object
On 11/02/16 23:09, yu@intel.com wrote: From: Alex DaiGuC client object is always pinned during its life cycle. We cache the kmap of its first page, which includes guc_process_desc and doorbell. By doing so, we can simplify the code where we read from this page to get where GuC is progressing on work queue; and the code where driver program doorbell to send work queue item to GuC. As a result, this patch removes the kmap_atomic in wq_check_space, where usleep_range could be called while kmap_atomic is held. This fixes issue below. [ 34.098798] BUG: scheduling while atomic: gem_close_race/1941/0x0002 [ 34.098822] Modules linked in: hid_generic usbhid i915 asix usbnet libphy mii i2c_algo_bit drm_kms_helper cfbfillrect syscopyarea cfbimgblt sysfillrect sysimgblt fb_sys_fops cfbcopyarea drm coretemp i2c_hid hid video pinctrl_sunrisepoint pinctrl_intel acpi_pad nls_iso8859_1 e1000e ptp psmouse pps_core ahci libahci [ 34.098824] CPU: 0 PID: 1941 Comm: gem_close_race Tainted: G U 4.4.0-160121+ #123 [ 34.098824] Hardware name: Intel Corporation Skylake Client platform/Skylake AIO DDR3L RVP10, BIOS SKLSE2R1.R00.X100.B01.1509220551 09/22/2015 [ 34.098825] 00013e40 880166c27a78 81280d02 880172c13e40 [ 34.098826] 880166c27a88 810c203a 880166c27ac8 814ec808 [ 34.098827] 88016b7c6000 880166c28000 000f4240 0001 [ 34.098827] Call Trace: [ 34.098831] [] dump_stack+0x4b/0x79 [ 34.098833] [] __schedule_bug+0x41/0x4f [ 34.098834] [] __schedule+0x5a8/0x690 [ 34.098835] [] schedule+0x37/0x80 [ 34.098836] [] schedule_hrtimeout_range_clock+0xad/0x130 [ 34.098837] [] ? hrtimer_init+0x10/0x10 [ 34.098838] [] ? schedule_hrtimeout_range_clock+0xa1/0x130 [ 34.098839] [] schedule_hrtimeout_range+0xe/0x10 [ 34.098840] [] usleep_range+0x3b/0x40 [ 34.098853] [] i915_guc_wq_check_space+0x119/0x210 [i915] [ 34.098861] [] intel_logical_ring_alloc_request_extras+0x5c/0x70 [i915] [ 34.098869] [] i915_gem_request_alloc+0x91/0x170 [i915] [ 34.098875] [] i915_gem_do_execbuffer.isra.25+0xbc7/0x12a0 [i915] [ 34.098882] [] ? i915_gem_object_get_pages_gtt+0x225/0x3c0 [i915] [ 34.098889] [] ? i915_gem_pwrite_ioctl+0xd6/0x9f0 [i915] [ 34.098895] [] i915_gem_execbuffer2+0xa8/0x250 [i915] [ 34.098900] [] drm_ioctl+0x258/0x4f0 [drm] [ 34.098906] [] ? i915_gem_execbuffer+0x340/0x340 [i915] [ 34.098908] [] do_vfs_ioctl+0x2cd/0x4a0 [ 34.098909] [] ? __fget+0x72/0xb0 [ 34.098910] [] SyS_ioctl+0x3c/0x70 [ 34.098911] [] entry_SYSCALL_64_fastpath+0x12/0x6a [ 34.100208] [ cut here ] Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93847 Cc: Cc: Signed-off-by: Alex Dai --- drivers/gpu/drm/i915/i915_guc_submission.c | 39 +- drivers/gpu/drm/i915/intel_guc.h | 3 ++- 2 files changed, 14 insertions(+), 28 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c index d7543ef..d51015e 100644 --- a/drivers/gpu/drm/i915/i915_guc_submission.c +++ b/drivers/gpu/drm/i915/i915_guc_submission.c @@ -195,11 +195,9 @@ static int guc_ring_doorbell(struct i915_guc_client *gc) struct guc_process_desc *desc; union guc_doorbell_qw db_cmp, db_exc, db_ret; union guc_doorbell_qw *db; - void *base; int attempt = 2, ret = -EAGAIN; - base = kmap_atomic(i915_gem_object_get_page(gc->client_obj, 0)); - desc = base + gc->proc_desc_offset; + desc = gc->client_base + gc->proc_desc_offset; /* Update the tail so it is visible to GuC */ desc->tail = gc->wq_tail; @@ -215,7 +213,7 @@ static int guc_ring_doorbell(struct i915_guc_client *gc) db_exc.cookie = 1; /* pointer of current doorbell cacheline */ - db = base + gc->doorbell_offset; + db = gc->client_base + gc->doorbell_offset; while (attempt--) { /* lets ring the doorbell */ @@ -244,10 +242,6 @@ static int guc_ring_doorbell(struct i915_guc_client *gc) db_exc.cookie = 1; } - /* Finally, update the cached copy of the GuC's WQ head */ - gc->wq_head = desc->head; Did you mean to remove the above? - - kunmap_atomic(base); return ret; } @@ -341,10 +335,8 @@ static void guc_init_proc_desc(struct intel_guc *guc, struct i915_guc_client *client) { struct guc_process_desc *desc; - void *base; - base = kmap_atomic(i915_gem_object_get_page(client->client_obj, 0)); - desc = base + client->proc_desc_offset; + desc = client->client_base + client->proc_desc_offset; memset(desc, 0, sizeof(*desc)); @@ -361,8 +353,6 @@ static void guc_init_proc_desc(struct intel_guc *guc,
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Do not return stale status / remove impossible WARN
On 12/02/16 10:21, Chris Wilson wrote: On Fri, Feb 12, 2016 at 10:05:58AM +, Tvrtko Ursulin wrote: On 11/02/16 21:00, Chris Wilson wrote: On Thu, Feb 11, 2016 at 06:03:09PM +, Tvrtko Ursulin wrote: From: Tvrtko UrsulinOnly caller to get_context_status ensures read pointer stays in range so the WARN is impossible. Also, if the WARN would be triggered by a hypothetical new caller stale status would be returned to them. Maybe it is better to wrap the pointer in the function itself then to avoid both and even results in smaller code. Signed-off-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/intel_lrc.c | 15 ++- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 89eb892df4ae..951f1e6af947 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -507,17 +507,16 @@ static bool execlists_check_remove_request(struct intel_engine_cs *ring, return false; } -static void get_context_status(struct intel_engine_cs *ring, - u8 read_pointer, - u32 *status, u32 *context_id) +static u32 get_context_status(struct intel_engine_cs *ring, u8 read_pointer, + u32 *context_id) { struct drm_i915_private *dev_priv = ring->dev->dev_private; - if (WARN_ON(read_pointer >= GEN8_CSB_ENTRIES)) - return; + read_pointer %= GEN8_CSB_ENTRIES; - *status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer)); *context_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer)); Micro-optimising hat says not to even do the uncached, spinlocked mmio read when not required. You mean move the forcewake grab out from elsp write to cover all mmio reads? These two patches make the irq handler around 3.7% smaller and moving the forcewake/uncore lock shrinks that by a 1% more. Must be faster as well, if someone could measure it. :) If only we already have benchmarks capable of measuring such optimisations! We do? diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 1e7ccd0a6573..77a64008f53d 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -375,8 +375,6 @@ static void execlists_elsp_write(struct drm_i915_gem_request *rq0, rq0->elsp_submitted++; /* You must always write both descriptors in the order below. */ - spin_lock(_priv->uncore.lock); - intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL); I915_WRITE_FW(RING_ELSP(ring), upper_32_bits(desc[1])); I915_WRITE_FW(RING_ELSP(ring), lower_32_bits(desc[1])); @@ -386,8 +384,6 @@ static void execlists_elsp_write(struct drm_i915_gem_request *rq0, /* ELSP is a wo register, use another nearby reg for posting */ POSTING_READ_FW(RING_EXECLIST_STATUS_LO(ring)); - intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL); - spin_unlock(_priv->uncore.lock); } static int execlists_update_context(struct drm_i915_gem_request *rq) @@ -513,9 +509,9 @@ static u32 get_context_status(struct intel_engine_cs *ring, u8 read_pointer, read_pointer %= GEN8_CSB_ENTRIES; - *context_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer)); + *context_id = I915_READ_FW(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer)); I was also suggesting that we don't need this read in almost 25% of cases! :) Oh right, yeah I can do that. @@ -616,9 +619,16 @@ static int execlists_context_queue(struct drm_i915_gem_request *request) } list_add_tail(>execlist_link, >execlist_queue); - if (num_elements == 0) + if (num_elements == 0) { + spin_lock(_priv->uncore.lock); We need the irq variants here. No since the execlist lock further up already disables interrupts. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Set invert bit for hpd based on VBT
Hi Shubhangi, [auto build test ERROR on drm-intel/for-linux-next] [also build test ERROR on next-20160212] [cannot apply to v4.5-rc3] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Shubhangi-Shrivastava/drm-i915-Set-invert-bit-for-hpd-based-on-VBT/20160212-203937 base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-randconfig-x011-201606 (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/gpu/drm/i915/intel_bios.c: In function 'intel_bios_is_port_hpd_inverted': >> drivers/gpu/drm/i915/intel_bios.c:121:40: error: 'struct >> common_child_dev_config' has no member named 'hpd_invert' if (dev_priv->vbt.child_dev[i].common.hpd_invert == 1) { ^ vim +121 drivers/gpu/drm/i915/intel_bios.c 115 DRM_ERROR("Bit inversion is not required in this platform\n"); 116 return false; 117 } 118 119 for (i = 0; i < dev_priv->vbt.child_dev_num; i++) { 120 > 121 if (dev_priv->vbt.child_dev[i].common.hpd_invert == 1) { 122 123 switch (dev_priv->vbt.child_dev[i].common.dvo_port) { 124 case DVO_PORT_DPA: --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Execlist irq handler micro optimisations
On Fri, Feb 12, 2016 at 12:00:40PM +, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin> > Assorted changes most likely without any practical effect > apart from a tiny reduction in generated code for the interrupt > handler and request submission. > > * Remove needless initialization. > * Improve cache locality by reorganizing code and/or using >branch hints to keep unexpected or error conditions out >of line. > * Favor busy submit path vs. empty queue. > * Less branching in hot-paths. > > v2: > > * Avoid mmio reads when possible. (Chris Wilson) > * Use natural integer size for csb indices. > * Remove useless return value from execlists_update_context. > * Extract 32-bit ppgtt PDPs update so it is out of line and >shared with two callers. > * Grab forcewake across all mmio operations to ease the >load on uncore lock and use chepear mmio ops. > > Version 2 now makes the irq handling code path ~20% smaller on > 48-bit PPGTT hardware, and a little bit less elsewhere. Hot > paths are mostly in-line now and hammering on the uncore > spinlock is greatly reduced together with mmio traffic to an > extent. Did you notice that ring->next_context_status_buffer is redundant as we also have that information to hand in status_pointer? What's your thinking for if (req->elsp_submitted & ring->gen8_9) vs a plain if (req->elsp_submitted) ? The tidies look good. Be useful to double check whether gem_latency is behaving as a canary, it's a bit of a puzzle why that first dispatch latency would grow. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH i-g-t] tests/prime_mmap: Encapsulate check_for_dma_buf_mmap() in igt_fixture.
This unbreaks distcheck target that in turn runs each test with --list-subtests. Signed-off-by: Marius Vlad--- tests/prime_mmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/prime_mmap.c b/tests/prime_mmap.c index 29a0cfd..1ea61c2 100644 --- a/tests/prime_mmap.c +++ b/tests/prime_mmap.c @@ -504,10 +504,10 @@ igt_main igt_fixture { fd = drm_open_driver(DRIVER_INTEL); + igt_skip_on((check_for_dma_buf_mmap() != 0)); errno = 0; } - igt_skip_on((check_for_dma_buf_mmap() != 0)); for (i = 0; i < ARRAY_SIZE(tests); i++) { igt_subtest(tests[i].name) -- 2.7.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Set invert bit for hpd based on VBT
On Fri, Feb 12, 2016 at 06:39:44PM +0530, Shubhangi Shrivastava wrote: > This patch sets the invert bit for hpd detection for each port > based on VBT configuration. Since each AOB can be designed to > depend on invert bit or not, it is expected if an AOB requires > invert bit, the user will set respective bit in VBT. > > v2: Separated VBT parsing from the rest of the logic. (Jani) > > Signed-off-by: Sivakumar Thulasimani> Signed-off-by: Durgadoss R > Signed-off-by: Shubhangi Shrivastava > --- > drivers/gpu/drm/i915/i915_drv.h | 1 + > drivers/gpu/drm/i915/i915_irq.c | 43 > +++ > drivers/gpu/drm/i915/i915_reg.h | 9 > drivers/gpu/drm/i915/intel_bios.c | 42 ++ > 4 files changed, 95 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 8216665..457f175 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -3393,6 +3393,7 @@ extern void intel_i2c_reset(struct drm_device *dev); > /* intel_bios.c */ > int intel_bios_init(struct drm_i915_private *dev_priv); > bool intel_bios_is_valid_vbt(const void *buf, size_t size); > +bool intel_bios_is_port_hpd_inverted(struct drm_device *dev, enum port port); > > /* intel_opregion.c */ > #ifdef CONFIG_ACPI > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 25a8937..fb95fb0 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -36,6 +36,7 @@ > #include "i915_drv.h" > #include "i915_trace.h" > #include "intel_drv.h" > +#include "intel_bios.h" > > /** > * DOC: interrupt handling > @@ -3424,6 +3425,47 @@ static void ibx_hpd_irq_setup(struct drm_device *dev) > I915_WRITE(PCH_PORT_HOTPLUG, hotplug); > } > > +/* > + * For BXT invert bit has to be set based on AOB design > + * for HPD detection logic, update it based on VBT fields. > + */ > +static void bxt_hpd_set_invert(struct drm_device *dev, u32 hotplug_port) > +{ > + struct drm_i915_private *dev_priv = dev->dev_private; > + int reg_val, val = 0; > + enum port port; > + > + for (port = PORT_A; port <= PORT_C; port++) { > + > + /* Proceed only if invert bit is set */ > + if (intel_bios_is_port_hpd_inverted(dev, port)) { > + switch (port) { > + case PORT_A: > + if (hotplug_port & BXT_DE_PORT_HP_DDIA) > + val |= BXT_DDIA_HPD_INVERT; > + break; > + case PORT_B: > + if (hotplug_port & BXT_DE_PORT_HP_DDIB) > + val |= BXT_DDIB_HPD_INVERT; > + break; > + case PORT_C: > + if (hotplug_port & BXT_DE_PORT_HP_DDIC) > + val |= BXT_DDIC_HPD_INVERT; > + break; > + default: > + DRM_ERROR("HPD invert set for invalid port > %d\n", > + port); > + break; > + } > + } > + } > + reg_val = I915_READ(BXT_HOTPLUG_CTL); > + DRM_DEBUG_KMS("Invert bit setting: hp_ctl:%x hp_port:%x val:%x\n", > + reg_val, hotplug_port, val); > + reg_val &= ~BXT_DDI_HPD_INVERT_MASK; > + I915_WRITE(BXT_HOTPLUG_CTL, reg_val | val); > +} No RMW stuff please. Just set up the bits appropriately in bxt_hpd_irq_setup(). > + > static void spt_hpd_irq_setup(struct drm_device *dev) > { > struct drm_i915_private *dev_priv = dev->dev_private; > @@ -3494,6 +3536,7 @@ static void bxt_hpd_irq_setup(struct drm_device *dev) > hotplug |= PORTC_HOTPLUG_ENABLE | PORTB_HOTPLUG_ENABLE | > PORTA_HOTPLUG_ENABLE; > I915_WRITE(PCH_PORT_HOTPLUG, hotplug); > + bxt_hpd_set_invert(dev, enabled_irqs); > } > > static void ibx_irq_postinstall(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 6732fc1..66cf92e 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -5940,6 +5940,15 @@ enum skl_disp_power_wells { > #define GEN8_PCU_IIR _MMIO(0x444e8) > #define GEN8_PCU_IER _MMIO(0x444ec) > > +/* BXT hotplug control */ > +#define BXT_HOTPLUG_CTL _MMIO(0xC4030) We already have a name for that register. > +#define BXT_DDIA_HPD_INVERT (1 << 27) > +#define BXT_DDIC_HPD_INVERT (1 << 11) > +#define BXT_DDIB_HPD_INVERT (1 << 3) I was going to suggest parametrizing this stuff, but the bits are in a fairly nasty order so not so easy, and we haven't parametrized the rest
Re: [Intel-gfx] [PATCH v2] drm/i915: Execlist irq handler micro optimisations
On Fri, Feb 12, 2016 at 01:46:35PM +, Tvrtko Ursulin wrote: > > > On 12/02/16 12:00, Tvrtko Ursulin wrote: > >From: Tvrtko Ursulin> > > >Assorted changes most likely without any practical effect > >apart from a tiny reduction in generated code for the interrupt > >handler and request submission. > > > > * Remove needless initialization. > > * Improve cache locality by reorganizing code and/or using > >branch hints to keep unexpected or error conditions out > >of line. > > * Favor busy submit path vs. empty queue. > > * Less branching in hot-paths. > > > >v2: > > > > * Avoid mmio reads when possible. (Chris Wilson) > > * Use natural integer size for csb indices. > > * Remove useless return value from execlists_update_context. > > * Extract 32-bit ppgtt PDPs update so it is out of line and > >shared with two callers. > > * Grab forcewake across all mmio operations to ease the > >load on uncore lock and use chepear mmio ops. > > > >Version 2 now makes the irq handling code path ~20% smaller on > >48-bit PPGTT hardware, and a little bit less elsewhere. Hot > >paths are mostly in-line now and hammering on the uncore > >spinlock is greatly reduced together with mmio traffic to an > >extent. > > Is gem_latency an interesting benchmark for th Yes, we should be able to detect changes on the order of 100ns and if the results are stable and above the noise, then definitely. "./gem_latency" sends one batch and then waits up it, so I only the patch to directly affect the dispatch latency. I'd say the wake up latency is solely due to reducing the spinlock contextion. "./gem_latency -n 100" would queue 100 no-op batches before the measurement. That may help to look at the overhead of handling the context-switch interrupt. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW
On Fri, Feb 12, 2016 at 05:06:07PM +0200, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä> > Bspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or > up to 675 MHz on ULT, bu only if extra cooling is provided. There > don't seem to be any strap or VBT bits to tells us this however. > > But I did spot something potentially relevant in > VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass > the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware > knows what its doing and trust the max cdclk in SWF06 if it's higher > than the basic limit specified in Bspec. To avoid regressing anything > let's ignore SWF06 if it indicates a lower limit than Bspec. > > Cc: Clint Taylor > Cc: "Thulasimani, Sivakumar" > Signed-off-by: Ville Syrjälä > --- > > I'm not at all sure if this is the right way to go about it. Sivakumar, > since you seem to have some ideas on this maybe you can have a look. > I'm not aware of any complaints so far that people can't get the cdclk > as high is they should on specific machines, so not sure if this is really > even needed. > > The other open question is what we should do if the VBIOS limit is > lower than what we'd expect based on BSpec. Should we still trust it? > Sadly we can't verify the SWF06 cdclk value in any way since it > only has two bits and we have four possible cdclk values. Oh, and I forgot to mention that this is totally untested. > > drivers/gpu/drm/i915/intel_display.c | 60 > > 1 file changed, 47 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 836bbdc239b6..1d70f6bf2934 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device > *dev) > dev_priv->max_cdclk_freq = 45; > else > dev_priv->max_cdclk_freq = 337500; > - } else if (IS_BROADWELL(dev)) { > + } else if (IS_BROADWELL(dev)) { > + int bios_max_cdclk_freq, max_cdclk_freq; > + > /* > - * FIXME with extra cooling we can allow > - * 540 MHz for ULX and 675 Mhz for ULT. > - * How can we know if extra cooling is > - * available? PCI ID, VTB, something else? > + * With extra cooling we can allow 540 MHz for > + * ULX and 675 Mhz for ULT. Assume VBIOS/GOP > + * passes that information in SWF06. >*/ > - if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) > - dev_priv->max_cdclk_freq = 45; > - else if (IS_BDW_ULX(dev)) > - dev_priv->max_cdclk_freq = 45; > - else if (IS_BDW_ULT(dev)) > - dev_priv->max_cdclk_freq = 54; > - else > - dev_priv->max_cdclk_freq = 675000; > + switch (I915_READ(SWF_ILK(0x06)) & 0x3) { > + case 0: > + bios_max_cdclk_freq = 45; > + break; > + case 1: > + bios_max_cdclk_freq = 54; > + break; > + case 2: > + bios_max_cdclk_freq = 337500; > + break; > + case 3: > + bios_max_cdclk_freq = 675000; > + break; > + } > + > + if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) { > + if (WARN_ON(bios_max_cdclk_freq != 45)) > + bios_max_cdclk_freq = 45; > + max_cdclk_freq = 45; > + } else if (IS_BDW_ULX(dev_priv)) { > + if (WARN_ON(bios_max_cdclk_freq > 54)) > + bios_max_cdclk_freq = 54; > + max_cdclk_freq = 45; > + } else if (IS_BDW_ULT(dev_priv)) { > + max_cdclk_freq = 54; > + } else { > + max_cdclk_freq = 675000; > + } > + > + if (bios_max_cdclk_freq > max_cdclk_freq) { > + DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) higher than > basic limit (%d kHz), " > + "assuming extra cooling is present\n", > + bios_max_cdclk_freq, max_cdclk_freq); > + max_cdclk_freq = bios_max_cdclk_freq; > + } else if (bios_max_cdclk_freq < max_cdclk_freq) { > + DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) lower than > basic limit (%d kHz), " > + "ignoring it\n", > +
Re: [Intel-gfx] [PATCH 2/2] drm/i915: Set invert bit for hpd based on VBT
Hi Shubhangi, [auto build test WARNING on drm-intel/for-linux-next] [also build test WARNING on next-20160212] [cannot apply to v4.5-rc3] [if your patch is applied to the wrong git tree, please drop us a note to help improving the system] url: https://github.com/0day-ci/linux/commits/Shubhangi-Shrivastava/drm-i915-Set-invert-bit-for-hpd-based-on-VBT/20160212-203937 base: git://anongit.freedesktop.org/drm-intel for-linux-next config: x86_64-randconfig-x000-201606 (attached as .config) reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All warnings (new ones prefixed by >>): In file included from include/uapi/linux/stddef.h:1:0, from include/linux/stddef.h:4, from include/uapi/linux/posix_types.h:4, from include/uapi/linux/types.h:13, from include/linux/types.h:5, from include/drm/drm_dp_helper.h:26, from drivers/gpu/drm/i915/intel_bios.c:28: drivers/gpu/drm/i915/intel_bios.c: In function 'intel_bios_is_port_hpd_inverted': drivers/gpu/drm/i915/intel_bios.c:121:40: error: 'struct common_child_dev_config' has no member named 'hpd_invert' if (dev_priv->vbt.child_dev[i].common.hpd_invert == 1) { ^ include/linux/compiler.h:147:28: note: in definition of macro '__trace_if' if (__builtin_constant_p((cond)) ? !!(cond) : \ ^ >> drivers/gpu/drm/i915/intel_bios.c:121:3: note: in expansion of macro 'if' if (dev_priv->vbt.child_dev[i].common.hpd_invert == 1) { ^ drivers/gpu/drm/i915/intel_bios.c:121:40: error: 'struct common_child_dev_config' has no member named 'hpd_invert' if (dev_priv->vbt.child_dev[i].common.hpd_invert == 1) { ^ include/linux/compiler.h:147:40: note: in definition of macro '__trace_if' if (__builtin_constant_p((cond)) ? !!(cond) : \ ^ >> drivers/gpu/drm/i915/intel_bios.c:121:3: note: in expansion of macro 'if' if (dev_priv->vbt.child_dev[i].common.hpd_invert == 1) { ^ drivers/gpu/drm/i915/intel_bios.c:121:40: error: 'struct common_child_dev_config' has no member named 'hpd_invert' if (dev_priv->vbt.child_dev[i].common.hpd_invert == 1) { ^ include/linux/compiler.h:158:16: note: in definition of macro '__trace_if' __r = !!(cond); \ ^ >> drivers/gpu/drm/i915/intel_bios.c:121:3: note: in expansion of macro 'if' if (dev_priv->vbt.child_dev[i].common.hpd_invert == 1) { ^ vim +/if +121 drivers/gpu/drm/i915/intel_bios.c 22 * 23 * Authors: 24 *Eric Anholt <e...@anholt.net> 25 * 26 */ 27 > 28 #include 29 #include 30 #include 31 #include "i915_drv.h" 32 #include "intel_bios.h" 33 34 /** 35 * DOC: Video BIOS Table (VBT) 36 * 37 * The Video BIOS Table, or VBT, provides platform and board specific 38 * configuration information to the driver that is not discoverable or available 39 * through other means. The configuration is mostly related to display 40 * hardware. The VBT is available via the ACPI OpRegion or, on older systems, in 41 * the PCI ROM. 42 * 43 * The VBT consists of a VBT Header (defined as vbt_header), a BDB 44 * Header ( bdb_header), and a number of BIOS Data Blocks (BDB) that 45 * contain the actual configuration information. The VBT Header, and thus the 46 * VBT, begins with "$VBT" signature. The VBT Header contains the offset of the 47 * BDB Header. The data blocks are concatenated after the BDB Header. The data 48 * blocks have a 1-byte Block ID, 2-byte Block Size, and Block Size bytes of 49 * data. (Block 53, the MIPI Sequence Block is an exception.) 50 * 51 * The driver parses the VBT during load. The relevant information is stored in 52 * driver private data for ease of use, and the actual VBT is not read after 53 * that. 54 */ 55 56 #define SLAVE_ADDR1 0x70 57 #define SLAVE_ADDR2 0x72 58 59 static int panel_type; 60 61 /* Get BDB block size given a pointer to Block ID. */ 62 static u32 _get_blocksize(const u8 *block_base) 63 { 64 /* The MIPI Sequence Block v3+ has a separate size field. */ 65 if (*block_base == BDB_MIPI_SEQUENCE && *(block_base + 3) >= 3) 66 return *((const u32 *)(block_base + 4)); 67 else 68 return *((const u16 *)(block_base + 1)); 69 } 70 71 /* Get BDB block size give a pointer to data after Block ID and Block Size.
Re: [Intel-gfx] [PATCH v2] drm/i915: Execlist irq handler micro optimisations
On 12/02/16 12:00, Tvrtko Ursulin wrote: From: Tvrtko UrsulinAssorted changes most likely without any practical effect apart from a tiny reduction in generated code for the interrupt handler and request submission. * Remove needless initialization. * Improve cache locality by reorganizing code and/or using branch hints to keep unexpected or error conditions out of line. * Favor busy submit path vs. empty queue. * Less branching in hot-paths. v2: * Avoid mmio reads when possible. (Chris Wilson) * Use natural integer size for csb indices. * Remove useless return value from execlists_update_context. * Extract 32-bit ppgtt PDPs update so it is out of line and shared with two callers. * Grab forcewake across all mmio operations to ease the load on uncore lock and use chepear mmio ops. Version 2 now makes the irq handling code path ~20% smaller on 48-bit PPGTT hardware, and a little bit less elsewhere. Hot paths are mostly in-line now and hammering on the uncore spinlock is greatly reduced together with mmio traffic to an extent. Is gem_latency an interesting benchmark for this? Five runs on vanilla: 747693/1: 9.080us 2.000us 2.000us 121.840us 742108/1: 9.060us 2.520us 2.520us 122.645us 744097/1: 9.060us 2.000us 2.000us 122.372us 744056/1: 9.180us 1.980us 1.980us 122.394us 742610/1: 9.040us 2.560us 2.560us 122.525us Five runs with this patch series: 786532/1: 10.760us 1.520us 1.520us 115.705us 780735/1: 10.740us 1.580us 1.580us 116.558us 783706/1: 10.800us 1.460us 1.460us 116.280us 784135/1: 10.800us 1.520us 1.520us 116.151us 784037/1: 10.740us 1.520us 1.520us 116.250us So it looks all got better apart from dispatch latency. 5% more throughput, 30% better consumer and producer latencies, 5% less CPU usage, but 18% worse dispatch latency. Comments? Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Use SWF06 to figure out max cdclk for BDW
From: Ville SyrjäläBspec tells us that we can allow cdclk up to 540Mhz on BDW ULX, or up to 675 MHz on ULT, bu only if extra cooling is provided. There don't seem to be any strap or VBT bits to tells us this however. But I did spot something potentially relevant in VBIOS_GOP_Driver_SWF_Registers.pdf. Apparently VBIOS/GOP can pass the max cdclk frequeny in SWF06 to the driver. Let's assume the firmware knows what its doing and trust the max cdclk in SWF06 if it's higher than the basic limit specified in Bspec. To avoid regressing anything let's ignore SWF06 if it indicates a lower limit than Bspec. Cc: Clint Taylor Cc: "Thulasimani, Sivakumar" Signed-off-by: Ville Syrjälä --- I'm not at all sure if this is the right way to go about it. Sivakumar, since you seem to have some ideas on this maybe you can have a look. I'm not aware of any complaints so far that people can't get the cdclk as high is they should on specific machines, so not sure if this is really even needed. The other open question is what we should do if the VBIOS limit is lower than what we'd expect based on BSpec. Should we still trust it? Sadly we can't verify the SWF06 cdclk value in any way since it only has two bits and we have four possible cdclk values. drivers/gpu/drm/i915/intel_display.c | 60 1 file changed, 47 insertions(+), 13 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 836bbdc239b6..1d70f6bf2934 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5407,21 +5407,55 @@ static void intel_update_max_cdclk(struct drm_device *dev) dev_priv->max_cdclk_freq = 45; else dev_priv->max_cdclk_freq = 337500; - } else if (IS_BROADWELL(dev)) { + } else if (IS_BROADWELL(dev)) { + int bios_max_cdclk_freq, max_cdclk_freq; + /* -* FIXME with extra cooling we can allow -* 540 MHz for ULX and 675 Mhz for ULT. -* How can we know if extra cooling is -* available? PCI ID, VTB, something else? +* With extra cooling we can allow 540 MHz for +* ULX and 675 Mhz for ULT. Assume VBIOS/GOP +* passes that information in SWF06. */ - if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) - dev_priv->max_cdclk_freq = 45; - else if (IS_BDW_ULX(dev)) - dev_priv->max_cdclk_freq = 45; - else if (IS_BDW_ULT(dev)) - dev_priv->max_cdclk_freq = 54; - else - dev_priv->max_cdclk_freq = 675000; + switch (I915_READ(SWF_ILK(0x06)) & 0x3) { + case 0: + bios_max_cdclk_freq = 45; + break; + case 1: + bios_max_cdclk_freq = 54; + break; + case 2: + bios_max_cdclk_freq = 337500; + break; + case 3: + bios_max_cdclk_freq = 675000; + break; + } + + if (I915_READ(FUSE_STRAP) & HSW_CDCLK_LIMIT) { + if (WARN_ON(bios_max_cdclk_freq != 45)) + bios_max_cdclk_freq = 45; + max_cdclk_freq = 45; + } else if (IS_BDW_ULX(dev_priv)) { + if (WARN_ON(bios_max_cdclk_freq > 54)) + bios_max_cdclk_freq = 54; + max_cdclk_freq = 45; + } else if (IS_BDW_ULT(dev_priv)) { + max_cdclk_freq = 54; + } else { + max_cdclk_freq = 675000; + } + + if (bios_max_cdclk_freq > max_cdclk_freq) { + DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) higher than basic limit (%d kHz), " + "assuming extra cooling is present\n", + bios_max_cdclk_freq, max_cdclk_freq); + max_cdclk_freq = bios_max_cdclk_freq; + } else if (bios_max_cdclk_freq < max_cdclk_freq) { + DRM_DEBUG_KMS("VBIOS/GOP max cdclk (%d kHz) lower than basic limit (%d kHz), " + "ignoring it\n", + bios_max_cdclk_freq, max_cdclk_freq); + } + + dev_priv->max_cdclk_freq = max_cdclk_freq; } else if (IS_CHERRYVIEW(dev)) { dev_priv->max_cdclk_freq = 32; } else if
Re: [Intel-gfx] [PATCH v4 6/8] drm/i915: Nuke fbc members from intel_crtc->atomic, v2.
Em Qua, 2016-02-10 às 13:49 +0100, Maarten Lankhorst escreveu: > Factor out intel_fbc_supports_rotation ^ not anymore > and use it in > pre_plane_update as well. This leaves intel_crtc->atomic > empty, so remove it too. > > Changes since v1: > - Add a intel_fbc_supports_rotation helper. Changes since v2: - No more need for rotation special-casing. (I suppose you also have to edit the commit title to be v3) > > Signed-off-by: Maarten Lankhorst> --- > drivers/gpu/drm/i915/intel_display.c | 58 +- > -- > drivers/gpu/drm/i915/intel_drv.h | 15 -- > 2 files changed, 20 insertions(+), 53 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 54be8a255f1f..00cb261c6787 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4782,11 +4782,9 @@ static void intel_post_plane_update(struct > intel_crtc_state *old_crtc_state) > { > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state- > >base.crtc); > struct drm_atomic_state *old_state = old_crtc_state- > >base.state; > - struct intel_crtc_atomic_commit *atomic = >atomic; > struct intel_crtc_state *pipe_config = > to_intel_crtc_state(crtc->base.state); > struct drm_device *dev = crtc->base.dev; > - struct drm_i915_private *dev_priv = dev->dev_private; > struct drm_plane *primary = crtc->base.primary; > struct drm_plane_state *old_pri_state = > drm_atomic_get_existing_plane_state(old_state, > primary); > @@ -4798,22 +4796,19 @@ static void intel_post_plane_update(struct > intel_crtc_state *old_crtc_state) > if (pipe_config->wm_changed && pipe_config->base.active) > intel_update_watermarks(>base); > > - if (atomic->update_fbc) > - intel_fbc_post_update(crtc); > - > if (old_pri_state) { For a code reader that is not very familiar with all the atomic history and its details, it's not trivial to conclude that "if (drm_atomic_get_existing_plane_state(primary))", then we're updating the primary plane on this atomic commit. And before this patch, it's much much easier to conclude that update_fbc will be true when an atomic update is touching the primary plane because that's explicitly stated by the cod. So although "let's kill this redundant struct" sounds good, it seems to me that code clarity is going away with this patch, so I wonder if the benefits of the patch outweigh the downsides. Isn't there something else we could do about this, such as some renaming, or adding some function aliases or just extra commenting? > struct intel_plane_state *primary_state = > to_intel_plane_state(primary->state); > struct intel_plane_state *old_primary_state = > to_intel_plane_state(old_pri_state); > > + intel_fbc_post_update(crtc); > + > if (primary_state->visible && > (needs_modeset(_config->base) || > !old_primary_state->visible)) > intel_post_enable_primary(>base); > } > - > - memset(atomic, 0, sizeof(*atomic)); > } > > static void intel_pre_plane_update(struct intel_crtc_state > *old_crtc_state) > @@ -4821,7 +4816,6 @@ static void intel_pre_plane_update(struct > intel_crtc_state *old_crtc_state) > struct intel_crtc *crtc = to_intel_crtc(old_crtc_state- > >base.crtc); > struct drm_device *dev = crtc->base.dev; > struct drm_i915_private *dev_priv = dev->dev_private; > - struct intel_crtc_atomic_commit *atomic = >atomic; > struct intel_crtc_state *pipe_config = > to_intel_crtc_state(crtc->base.state); > struct drm_atomic_state *old_state = old_crtc_state- > >base.state; > @@ -4830,17 +4824,17 @@ static void intel_pre_plane_update(struct > intel_crtc_state *old_crtc_state) > drm_atomic_get_existing_plane_state(old_state, > primary); > bool modeset = needs_modeset(_config->base); > > - if (atomic->update_fbc) > - intel_fbc_pre_update(crtc); > - > if (old_pri_state) { > struct intel_plane_state *primary_state = > to_intel_plane_state(primary->state); > struct intel_plane_state *old_primary_state = > to_intel_plane_state(old_pri_state); > + bool turn_off = old_primary_state->visible && > + (modeset || !primary_state->visible); Not really related to the patch, but ok to keep since it's trivial... > + > + intel_fbc_pre_update(crtc); > > - if (old_primary_state->visible && > - (modeset || !primary_state->visible)) > + if (turn_off) > intel_pre_disable_primary(>base); > } > > @@ -11822,27 +11816,17 @@ int
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Do not return stale status / remove impossible WARN
On 11/02/16 21:00, Chris Wilson wrote: > On Thu, Feb 11, 2016 at 06:03:09PM +, Tvrtko Ursulin wrote: >> From: Tvrtko Ursulin>> >> Only caller to get_context_status ensures read pointer stays in >> range so the WARN is impossible. Also, if the WARN would be >> triggered by a hypothetical new caller stale status would be >> returned to them. >> >> Maybe it is better to wrap the pointer in the function itself >> then to avoid both and even results in smaller code. >> >> Signed-off-by: Tvrtko Ursulin >> --- >> drivers/gpu/drm/i915/intel_lrc.c | 15 ++- >> 1 file changed, 6 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c >> b/drivers/gpu/drm/i915/intel_lrc.c >> index 89eb892df4ae..951f1e6af947 100644 >> --- a/drivers/gpu/drm/i915/intel_lrc.c >> +++ b/drivers/gpu/drm/i915/intel_lrc.c >> @@ -507,17 +507,16 @@ static bool execlists_check_remove_request(struct >> intel_engine_cs *ring, >> return false; >> } >> >> -static void get_context_status(struct intel_engine_cs *ring, >> - u8 read_pointer, >> - u32 *status, u32 *context_id) >> +static u32 get_context_status(struct intel_engine_cs *ring, u8 read_pointer, >> + u32 *context_id) >> { >> struct drm_i915_private *dev_priv = ring->dev->dev_private; >> >> -if (WARN_ON(read_pointer >= GEN8_CSB_ENTRIES)) >> -return; >> +read_pointer %= GEN8_CSB_ENTRIES; >> >> -*status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer)); >> *context_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer)); > > Micro-optimising hat says not to even do the uncached, spinlocked mmio > read when not required. You mean move the forcewake grab out from elsp write to cover all mmio reads? These two patches make the irq handler around 3.7% smaller and moving the forcewake/uncore lock shrinks that by a 1% more. Must be faster as well, if someone could measure it. :) diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index 1e7ccd0a6573..77a64008f53d 100644 --- a/drivers/gpu/drm/i915/intel_lrc.c +++ b/drivers/gpu/drm/i915/intel_lrc.c @@ -375,8 +375,6 @@ static void execlists_elsp_write(struct drm_i915_gem_request *rq0, rq0->elsp_submitted++; /* You must always write both descriptors in the order below. */ - spin_lock(_priv->uncore.lock); - intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL); I915_WRITE_FW(RING_ELSP(ring), upper_32_bits(desc[1])); I915_WRITE_FW(RING_ELSP(ring), lower_32_bits(desc[1])); @@ -386,8 +384,6 @@ static void execlists_elsp_write(struct drm_i915_gem_request *rq0, /* ELSP is a wo register, use another nearby reg for posting */ POSTING_READ_FW(RING_EXECLIST_STATUS_LO(ring)); - intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL); - spin_unlock(_priv->uncore.lock); } static int execlists_update_context(struct drm_i915_gem_request *rq) @@ -513,9 +509,9 @@ static u32 get_context_status(struct intel_engine_cs *ring, u8 read_pointer, read_pointer %= GEN8_CSB_ENTRIES; - *context_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer)); + *context_id = I915_READ_FW(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer)); - return I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer)); + return I915_READ_FW(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer)); } /** @@ -535,15 +531,18 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring) u32 status_id; u32 submit_contexts = 0; - status_pointer = I915_READ(RING_CONTEXT_STATUS_PTR(ring)); + spin_lock(>execlist_lock); + + spin_lock(_priv->uncore.lock); + intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL); + + status_pointer = I915_READ_FW(RING_CONTEXT_STATUS_PTR(ring)); read_pointer = ring->next_context_status_buffer; write_pointer = GEN8_CSB_WRITE_PTR(status_pointer); if (read_pointer > write_pointer) write_pointer += GEN8_CSB_ENTRIES; - spin_lock(>execlist_lock); - while (read_pointer < write_pointer) { status = get_context_status(ring, ++read_pointer, _id); @@ -569,22 +568,26 @@ void intel_lrc_irq_handler(struct intel_engine_cs *ring) GEN8_CTX_STATUS_ACTIVE_IDLE execlists_context_unqueue(ring); - spin_unlock(>execlist_lock); - - if (unlikely(submit_contexts > 2)) - DRM_ERROR("More than two context complete events?\n"); - ring->next_context_status_buffer = write_pointer % GEN8_CSB_ENTRIES; /* Update the read pointer to the old write pointer. Manual ringbuffer * management ftw */ - I915_WRITE(RING_CONTEXT_STATUS_PTR(ring), - _MASKED_FIELD(GEN8_CSB_READ_PTR_MASK, -
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Do not return stale status / remove impossible WARN
On Fri, Feb 12, 2016 at 10:05:58AM +, Tvrtko Ursulin wrote: > > On 11/02/16 21:00, Chris Wilson wrote: > > On Thu, Feb 11, 2016 at 06:03:09PM +, Tvrtko Ursulin wrote: > >> From: Tvrtko Ursulin> >> > >> Only caller to get_context_status ensures read pointer stays in > >> range so the WARN is impossible. Also, if the WARN would be > >> triggered by a hypothetical new caller stale status would be > >> returned to them. > >> > >> Maybe it is better to wrap the pointer in the function itself > >> then to avoid both and even results in smaller code. > >> > >> Signed-off-by: Tvrtko Ursulin > >> --- > >> drivers/gpu/drm/i915/intel_lrc.c | 15 ++- > >> 1 file changed, 6 insertions(+), 9 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_lrc.c > >> b/drivers/gpu/drm/i915/intel_lrc.c > >> index 89eb892df4ae..951f1e6af947 100644 > >> --- a/drivers/gpu/drm/i915/intel_lrc.c > >> +++ b/drivers/gpu/drm/i915/intel_lrc.c > >> @@ -507,17 +507,16 @@ static bool execlists_check_remove_request(struct > >> intel_engine_cs *ring, > >>return false; > >> } > >> > >> -static void get_context_status(struct intel_engine_cs *ring, > >> - u8 read_pointer, > >> - u32 *status, u32 *context_id) > >> +static u32 get_context_status(struct intel_engine_cs *ring, u8 > >> read_pointer, > >> +u32 *context_id) > >> { > >>struct drm_i915_private *dev_priv = ring->dev->dev_private; > >> > >> - if (WARN_ON(read_pointer >= GEN8_CSB_ENTRIES)) > >> - return; > >> + read_pointer %= GEN8_CSB_ENTRIES; > >> > >> - *status = I915_READ(RING_CONTEXT_STATUS_BUF_LO(ring, read_pointer)); > >>*context_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer)); > > > > Micro-optimising hat says not to even do the uncached, spinlocked mmio > > read when not required. > > You mean move the forcewake grab out from elsp write to cover all mmio > reads? These two patches make the irq handler around 3.7% smaller and > moving the forcewake/uncore lock shrinks that by a 1% more. Must be > faster as well, if someone could measure it. :) If only we already have benchmarks capable of measuring such optimisations! > diff --git a/drivers/gpu/drm/i915/intel_lrc.c > b/drivers/gpu/drm/i915/intel_lrc.c > index 1e7ccd0a6573..77a64008f53d 100644 > --- a/drivers/gpu/drm/i915/intel_lrc.c > +++ b/drivers/gpu/drm/i915/intel_lrc.c > @@ -375,8 +375,6 @@ static void execlists_elsp_write(struct > drm_i915_gem_request *rq0, > rq0->elsp_submitted++; > > /* You must always write both descriptors in the order below. */ > - spin_lock(_priv->uncore.lock); > - intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL); > I915_WRITE_FW(RING_ELSP(ring), upper_32_bits(desc[1])); > I915_WRITE_FW(RING_ELSP(ring), lower_32_bits(desc[1])); > > @@ -386,8 +384,6 @@ static void execlists_elsp_write(struct > drm_i915_gem_request *rq0, > > /* ELSP is a wo register, use another nearby reg for posting */ > POSTING_READ_FW(RING_EXECLIST_STATUS_LO(ring)); > - intel_uncore_forcewake_put__locked(dev_priv, FORCEWAKE_ALL); > - spin_unlock(_priv->uncore.lock); > } > > static int execlists_update_context(struct drm_i915_gem_request *rq) > @@ -513,9 +509,9 @@ static u32 get_context_status(struct intel_engine_cs > *ring, u8 read_pointer, > > read_pointer %= GEN8_CSB_ENTRIES; > > - *context_id = I915_READ(RING_CONTEXT_STATUS_BUF_HI(ring, read_pointer)); > + *context_id = I915_READ_FW(RING_CONTEXT_STATUS_BUF_HI(ring, > read_pointer)); I was also suggesting that we don't need this read in almost 25% of cases! :) > @@ -616,9 +619,16 @@ static int execlists_context_queue(struct > drm_i915_gem_request *request) > } > > list_add_tail(>execlist_link, >execlist_queue); > - if (num_elements == 0) > + if (num_elements == 0) { > + spin_lock(_priv->uncore.lock); We need the irq variants here. > + intel_uncore_forcewake_get__locked(dev_priv, FORCEWAKE_ALL); -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT
On Mon, 2016-02-01 at 19:27 +0530, Durgadoss R wrote: > To support USB type C alternate DP mode, the display driver needs to > know the number of lanes required by the DP panel as well as number > of lanes that can be supported by the type-C cable. Sometimes, the > type-C cable may limit the bandwidth even if Panel can support > more lanes. To address these scenarios, the display driver will > start link training with max lanes, and if that fails, the driver > falls back to x2 lanes; and repeats this procedure for all > bandwidth/lane configurations. > > * Since link training is done before modeset only the port > (and not pipe/planes) and its associated PLLs are enabled. > * On DP hotplug: Directly start link training on the crtc > associated with the DP encoder. > * On Connected boot scenarios: When booted with an LFP and a DP, > typically, BIOS brings up DP. In these cases, we disable the > crtc, do upfront link training and then re-enable it back. > * All local changes made for upfront link training are reset > to their previous values once it is done; so that the > subsequent modeset is not aware of these changes. > Please always include a changelog when sending a new version of a patch. > Signed-off-by: Durgadoss R> --- > drivers/gpu/drm/i915/intel_ddi.c | 102 + > drivers/gpu/drm/i915/intel_display.c | 38 ++- > drivers/gpu/drm/i915/intel_dp.c | 122 ++ > - > drivers/gpu/drm/i915/intel_drv.h | 10 +++ > 4 files changed, 270 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c > b/drivers/gpu/drm/i915/intel_ddi.c > index 3fb9a03..cc5cad5 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -3217,6 +3217,108 @@ intel_ddi_init_hdmi_connector(struct > intel_digital_port *intel_dig_port) > return connector; > } > > +static void intel_ddi_commit_upfront_pll_config(struct intel_dp *intel_dp, > + struct intel_shared_dpll *pll) > +{ > + struct intel_shared_dpll_config tmp_config; > + > + /* > + * The shared_dpll_config is computed in intel_get_shared_dpll(). > + * It is committed to 'pll->config' here to be used in > + * intel_enable/disable_shared_dpll functions. Before committing. > + * save the existing config, so that once upfront link training is > + * done, the original value of 'pll->config' can be restored. > + */ > + tmp_config = pll->config; > + pll->config = intel_dp->upfront_pll_config; > + intel_dp->upfront_pll_config = tmp_config; > +} > + > +static struct intel_shared_dpll *intel_ddi_select_upfront_pll_config( > + struct intel_dp *intel_dp, struct intel_crtc *crtc) > +{ > + if (!intel_ddi_pll_select(crtc, crtc->config)) > + return NULL; > + > + return intel_crtc_to_shared_dpll(crtc); > +} > + > +static void intel_ddi_clear_upfront_pll_config(struct intel_dp *intel_dp, > + struct intel_shared_dpll *pll) > +{ > + pll->config = intel_dp->upfront_pll_config; > +} > + The shared pll interface is really getting in the way here. And BXT plls aren't even shared. We are jumping through hoops to get the pll that matches the given encoder and to call the code that calculates the dpll_hw_state based on the DP link rate. I'd like to get that interface fixed but I reckon it will be tricky to find something that works for all the platforms we support. That's the next thing on my todo list. If we have to live with some intermediary solution, I think it would be better to (almost) completely bypass the shared dpll stuff. First we would need to split bxt_ddi_pll_sel() so that we would have a function that takes the link bandwidth and spits out a dpll_hw_state without looking at crtc_state. Then we would just take the right pll based on dig_port->port, make sure it is not being used and program it with the hw state we get from that new function. I wrote a patch to do that split. With it, the PLL enable logic in the upfront link train logic would look a bit like below. There is some potential for problems with the state checker, but it should be fine as long as the old dpll hw state is saved and restored when everything is over. /* FIXME: this only works for BXT */ dpll_id = (enum intel_dpll_id) dig_port->port; pll = dev_priv->shared_dplls[dpll_id]; if (WARN_ON(pll->config.crtc_mask) || WARN_ON(pll->active)) goto exit; bxt_ddi_dp_pll_dividers(crtc->config->port_clock, _div); if (!bxt_ddi_set_dpll_hw_state(clock, _div, >config.hw_state)) { DRM_ERROR("Could not setup DPLL\n"); goto exit; } /* Enable PLL followed by port */ pll->enable(dev_priv, pll); encoder->pre_enable(encoder); This solution would
[Intel-gfx] [PATCH] drm/i915: Split bxt_ddi_pll_select()
Split out of bxt_ddi_pll_select() the logic that calculates the pll dividers and dpll_hw_state into a new function that doesn't depend on crtc state. This will be used for enabling the port pll when doing upfront link training. Signed-off-by: Ander Conselvan de Oliveira--- drivers/gpu/drm/i915/intel_ddi.c | 150 --- 1 file changed, 91 insertions(+), 59 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 00bb299..35a2d96 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1583,6 +1583,8 @@ struct bxt_clk_div { uint32_t m2_frac; bool m2_frac_en; uint32_t n; + + int vco; }; /* pre-calculated values for DP linkrates */ @@ -1597,54 +1599,61 @@ 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) +bxt_ddi_hdmi_pll_dividers(struct intel_crtc *intel_crtc, + struct intel_crtc_state *crtc_state, int clock, + struct bxt_clk_div *clk_div) { - struct intel_shared_dpll *pll; - struct bxt_clk_div clk_div = {0}; - int vco = 0; - uint32_t prop_coef, int_coef, gain_ctl, targ_cnt; - uint32_t lanestagger; - int clock = crtc_state->port_clock; + intel_clock_t best_clock; - if (intel_encoder->type == INTEL_OUTPUT_HDMI) { - intel_clock_t best_clock; + /* Calculate HDMI div */ + /* +* FIXME: tie the following calculation into +* i9xx_crtc_compute_clock +*/ + if (!bxt_find_best_dpll(crtc_state, clock, _clock)) { + DRM_DEBUG_DRIVER("no PLL dividers found for clock %d pipe %c\n", +clock, pipe_name(intel_crtc->pipe)); + return false; + } - /* Calculate HDMI div */ - /* -* FIXME: tie the following calculation into -* i9xx_crtc_compute_clock -*/ - if (!bxt_find_best_dpll(crtc_state, clock, _clock)) { - DRM_DEBUG_DRIVER("no PLL dividers found for clock %d pipe %c\n", -clock, pipe_name(intel_crtc->pipe)); - return false; - } + clk_div->p1 = best_clock.p1; + clk_div->p2 = best_clock.p2; + WARN_ON(best_clock.m1 != 2); + clk_div->n = best_clock.n; + clk_div->m2_int = best_clock.m2 >> 22; + clk_div->m2_frac = best_clock.m2 & ((1 << 22) - 1); + clk_div->m2_frac_en = clk_div->m2_frac != 0; - clk_div.p1 = best_clock.p1; - clk_div.p2 = best_clock.p2; - WARN_ON(best_clock.m1 != 2); - clk_div.n = best_clock.n; - clk_div.m2_int = best_clock.m2 >> 22; - clk_div.m2_frac = best_clock.m2 & ((1 << 22) - 1); - clk_div.m2_frac_en = clk_div.m2_frac != 0; + clk_div->vco = best_clock.vco; - vco = best_clock.vco; - } else if (intel_encoder->type == INTEL_OUTPUT_DISPLAYPORT || - intel_encoder->type == INTEL_OUTPUT_EDP) { - int i; + return true; +} - clk_div = bxt_dp_clk_val[0]; - for (i = 0; i < ARRAY_SIZE(bxt_dp_clk_val); ++i) { - if (bxt_dp_clk_val[i].clock == clock) { - clk_div = bxt_dp_clk_val[i]; - break; - } +static void +bxt_ddi_dp_pll_dividers(int clock, struct bxt_clk_div *clk_div) +{ + int i; + + *clk_div = bxt_dp_clk_val[0]; + for (i = 0; i < ARRAY_SIZE(bxt_dp_clk_val); ++i) { + if (bxt_dp_clk_val[i].clock == clock) { + *clk_div = bxt_dp_clk_val[i]; + break; } - vco = clock * 10 / 2 * clk_div.p1 * clk_div.p2; } + clk_div->vco = clock * 10 / 2 * clk_div->p1 * clk_div->p2; +} + +static bool +bxt_ddi_set_dpll_hw_state(int clock, + struct bxt_clk_div *clk_div, + struct intel_dpll_hw_state *dpll_hw_state) +{ + int vco = clk_div->vco; + uint32_t prop_coef, int_coef, gain_ctl, targ_cnt; + uint32_t lanestagger; + if (vco >= 620 && vco <= 670) { prop_coef = 4; int_coef = 9; @@ -1666,9 +1675,6 @@ bxt_ddi_pll_select(struct intel_crtc *intel_crtc, return false; } - memset(_state->dpll_hw_state, 0, - sizeof(crtc_state->dpll_hw_state)); - if (clock > 27) lanestagger = 0x18; else if (clock > 135000) @@ -1680,33 +1686,59 @@
Re: [Intel-gfx] [PATCH 7/7] sna: add an assert in the lengthy sna_drawable_use_bo
On Fri, Feb 12, 2016 at 06:31:29PM +0200, Martin Peres wrote: > I got lost in all the changes done on priv->flush and gpu_bo enough > not to be able to guarantee that if flush is non-null, so is gpu_bo. > > Caught by Klockwork. Nope, impossible. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/7] sna_video_sprite: add asserts to catch invalid pipe#
On Fri, Feb 12, 2016 at 06:31:28PM +0200, Martin Peres wrote: > Caught by Klockwork. This will be enough to catch those issues during > bringup. But you just said that klockwork didn't find a bug... :-p I did contemplate having this stored on the CRTC instead. Reviewed-by: Chris Wilson-Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCHv2 2/4] drm/i915: Make finding unused crtc as a generic function
On Mon, 2016-02-01 at 19:27 +0530, Durgadoss R wrote: > Looping over the crtc list and finding an unused crtc > has users other than load_detect(). Which other users? If there are other users they should be converted in this patch. If the use will only come in a future patch, please make that clear in the commit message. > Hence move it to > a common function so that we can re-use the logic. > > Signed-off-by: Durgadoss R> --- > drivers/gpu/drm/i915/intel_display.c | 37 ++- > - > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 24 insertions(+), 14 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c > b/drivers/gpu/drm/i915/intel_display.c > index 92cd5c6..af50e61 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -10417,6 +10417,27 @@ static int intel_modeset_setup_plane_state(struct > drm_atomic_state *state, > return 0; > } > > +struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder) > +{ This function is exported, so it needs some kernel doc. > + struct drm_crtc *possible_crtc; > + struct drm_crtc *crtc = NULL; > + struct drm_device *dev = encoder->dev; > + int i = -1; > + > + for_each_crtc(dev, possible_crtc) { > + i++; > + if (!(encoder->possible_crtcs & (1 << i))) > + continue; > + if (possible_crtc->state->enable) > + continue; > + > + crtc = possible_crtc; > + break; > + } > + > + return crtc; > +} > + > bool intel_get_load_detect_pipe(struct drm_connector *connector, > struct drm_display_mode *mode, > struct intel_load_detect_pipe *old, > @@ -10425,7 +10446,6 @@ bool intel_get_load_detect_pipe(struct drm_connector > *connector, > struct intel_crtc *intel_crtc; > struct intel_encoder *intel_encoder = > intel_attached_encoder(connector); > - struct drm_crtc *possible_crtc; > struct drm_encoder *encoder = _encoder->base; > struct drm_crtc *crtc = NULL; > struct drm_device *dev = encoder->dev; > @@ -10434,7 +10454,7 @@ bool intel_get_load_detect_pipe(struct drm_connector > *connector, > struct drm_atomic_state *state = NULL; > struct drm_connector_state *connector_state; > struct intel_crtc_state *crtc_state; > - int ret, i = -1; > + int ret; > > DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n", > connector->base.id, connector->name, > @@ -10476,21 +10496,10 @@ retry: > return true; > } > > - /* Find an unused one (if possible) */ > - for_each_crtc(dev, possible_crtc) { > - i++; > - if (!(encoder->possible_crtcs & (1 << i))) > - continue; > - if (possible_crtc->state->enable) > - continue; > - > - crtc = possible_crtc; > - break; > - } > - > /* >* If we didn't find an unused CRTC, don't use any. >*/ > + crtc = intel_get_unused_crtc(encoder); The comment above looks out of place now. It is pretty obvious anyway, so maybe just delete it. With those fixed, this is Reviewed-by: Ander Conselvan de Oliveira if (!crtc) { > DRM_DEBUG_KMS("no pipe available for load-detect\n"); > goto fail; > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index c7a6e32..9fe7c4b 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1106,6 +1106,7 @@ bool intel_get_load_detect_pipe(struct drm_connector > *connector, > void intel_release_load_detect_pipe(struct drm_connector *connector, > struct intel_load_detect_pipe *old, > struct drm_modeset_acquire_ctx *ctx); > +struct drm_crtc *intel_get_unused_crtc(struct drm_encoder *encoder); > int intel_pin_and_fence_fb_obj(struct drm_plane *plane, > struct drm_framebuffer *fb, > const struct drm_plane_state *plane_state); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/5] drm/i915: Cleaning up intel_dp_hpd_pulse
On Wed, 2016-02-10 at 14:34 +0530, Shubhangi Shrivastava wrote: > Current DP detection has DPCD operations split across > intel_dp_hpd_pulse and intel_dp_detect which contains > duplicates as well. Also intel_dp_detect is called > during modes enumeration as well which will result > in multiple dpcd operations. So this patch tries > to solve both these by bringing all DPCD operations > in one single function and make intel_dp_detect > use existing values instead of repeating same steps. > > v2: Pulled in a hunk from last patch of the series to > this patch. (Ander) > v3: Added MST hotplug handling. (Ander) The change log for this version of the patch is missing. With that, this is Reviewed-by: Ander Conselvan de Oliveira> > Tested-by: Nathan D Ciobanu > Signed-off-by: Sivakumar Thulasimani > Signed-off-by: Shubhangi Shrivastava > --- > drivers/gpu/drm/i915/intel_dp.c | 72 +-- > - > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 47 insertions(+), 26 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 042283a..ad5ec3b 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -4653,6 +4653,16 @@ intel_dp_long_pulse(struct intel_connector > *intel_connector) >*/ > status = connector_status_disconnected; > goto out; > + } else if (connector->status == connector_status_connected) { > + /* > + * If display was connected already and is still connected > + * check links status, there has been known issues of > + * link loss triggerring long pulse > + */ > + drm_modeset_lock(>mode_config.connection_mutex, NULL); > + intel_dp_check_link_status(intel_dp); > + drm_modeset_unlock(>mode_config.connection_mutex); > + goto out; > } > > /* > @@ -4666,6 +4676,7 @@ intel_dp_long_pulse(struct intel_connector > *intel_connector) > intel_dp_set_edid(intel_dp); > > status = connector_status_connected; > + intel_dp->detect_done = true; > > /* Try to read the source of the interrupt */ > if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 && > @@ -4682,8 +4693,21 @@ intel_dp_long_pulse(struct intel_connector > *intel_connector) > } > > out: > - if (status != connector_status_connected) > + if (status != connector_status_connected) { > intel_dp_unset_edid(intel_dp); > + /* > + * If we were in MST mode, and device is not there, > + * get out of MST mode > + */ > + if (intel_dp->is_mst) { > + DRM_DEBUG_KMS("MST device may have disappeared %d vs > %d\n", > + intel_dp->is_mst, intel_dp > ->mst_mgr.mst_state); > + intel_dp->is_mst = false; > + drm_dp_mst_topology_mgr_set_mst(_dp->mst_mgr, > + intel_dp->is_mst); > + } > + } > + > intel_display_power_put(to_i915(dev), power_domain); > return; > } > @@ -4707,7 +4731,11 @@ intel_dp_detect(struct drm_connector *connector, bool > force) > return connector_status_disconnected; > } > > - intel_dp_long_pulse(intel_dp->attached_connector); > + /* If full detect is not performed yet, do a full detect */ > + if (!intel_dp->detect_done) > + intel_dp_long_pulse(intel_dp->attached_connector); > + > + intel_dp->detect_done = false; > > if (intel_connector->detect_edid) > return connector_status_connected; > @@ -5040,25 +5068,25 @@ intel_dp_hpd_pulse(struct intel_digital_port > *intel_dig_port, bool long_hpd) > /* indicate that we need to restart link training */ > intel_dp->train_set_valid = false; > > - if (!intel_digital_port_connected(dev_priv, intel_dig_port)) > - goto mst_fail; > - > - if (!intel_dp_get_dpcd(intel_dp)) { > - goto mst_fail; > - } > - > - intel_dp_probe_oui(intel_dp); > + intel_dp_long_pulse(intel_dp->attached_connector); > + if (intel_dp->is_mst) > + ret = IRQ_HANDLED; > + goto put_power; > > - if (!intel_dp_probe_mst(intel_dp)) { > - drm_modeset_lock(>mode_config.connection_mutex, > NULL); > - intel_dp_check_link_status(intel_dp); > - drm_modeset_unlock( > ->mode_config.connection_mutex); > - goto mst_fail; > - } > } else { > if (intel_dp->is_mst) { > - if
Re: [Intel-gfx] [PATCH 4/5] drm/i915: Save sink_count for tracking changes to it and read sink_count dpcd always
I think I mentioned something about the patch summary (the title) being too big before. The kernel documentation on submitting patches [1] says: "the "summary" must be no more than 70-75 characters". [1] https://www.kernel.org/doc/Documentation/SubmittingPatches Cheers, Ander On Wed, 2016-02-10 at 14:34 +0530, Shubhangi Shrivastava wrote: > This patch reads sink_count dpcd always and removes its > read operation based on values in downstream port dpcd. > > SINK_COUNT dpcd is not dependent on DOWNSTREAM_PORT_PRESENT dpcd. > SINK_COUNT denotes if a display is attached, while > DOWNSTREAM_PORT_PRESET indicates how many ports are available > in the dongle where display can be attached. so it is possible > for sink count to change irrespective of value in downstream > port dpcd. > > Here is a table of possible values and scenarios > > sink_count downstream_port > present > 0 0 no display is attached > 0 1 dongle is connected without display > 1 0 display connected directly > 1 1 display connected through dongle > > v2: Storing value of intel_dp->sink_count that is ready > for consumption. (Ander) > Squashing two commits into one. (Ander) > > v3: Added comment to explain the need of early return when > sink count is 0. (Ander) > > Tested-by: Nathan D Ciobanu> Signed-off-by: Sivakumar Thulasimani > Signed-off-by: Shubhangi Shrivastava > Reviewed-by: Ander Conselvan de Oliveira > --- > drivers/gpu/drm/i915/intel_dp.c | 30 +++--- > drivers/gpu/drm/i915/intel_drv.h | 1 + > 2 files changed, 24 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 9fe78e1..a834c5f 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -3862,6 +3862,27 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > if (intel_dp->dpcd[DP_DPCD_REV] == 0) > return false; /* DPCD not present */ > > + if (intel_dp_dpcd_read_wake(_dp->aux, DP_SINK_COUNT, > + _dp->sink_count, 1) < 0) > + return false; > + > + /* > + * Sink count can change between short pulse hpd hence > + * a member variable in intel_dp will track any changes > + * between short pulse interrupts. > + */ > + intel_dp->sink_count = DP_GET_SINK_COUNT(intel_dp->sink_count); > + > + /* > + * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that > + * a dongle is present but no display. Unless we require to know > + * if a dongle is present or not, we don't need to update > + * downstream port information. So, an early return here saves > + * time from performing other operations which are not required. > + */ > + if (!intel_dp->sink_count) > + return false; > + > /* Check if the panel supports PSR */ > memset(intel_dp->psr_dpcd, 0, sizeof(intel_dp->psr_dpcd)); > if (is_edp(intel_dp)) { > @@ -4379,14 +4400,9 @@ intel_dp_detect_dpcd(struct intel_dp *intel_dp) > /* If we're HPD-aware, SINK_COUNT changes dynamically */ > if (intel_dp->dpcd[DP_DPCD_REV] >= 0x11 && > intel_dp->downstream_ports[0] & DP_DS_PORT_HPD) { > - uint8_t reg; > - > - if (intel_dp_dpcd_read_wake(_dp->aux, DP_SINK_COUNT, > - , 1) < 0) > - return connector_status_unknown; > > - return DP_GET_SINK_COUNT(reg) ? connector_status_connected > - : > connector_status_disconnected; > + return intel_dp->sink_count ? > + connector_status_connected : connector_status_disconnected; > } > > /* If no HPD, poke DDC gently */ > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index 3d003d6..c04db07 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -752,6 +752,7 @@ struct intel_dp { > uint32_t DP; > int link_rate; > uint8_t lane_count; > + uint8_t sink_count; > bool has_audio; > bool detect_done; > enum hdmi_force_audio force_audio; ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] kernel crash in snd_hda_intel
!!! This caused a regression in the i-g-t drv_module_reload_basic test. Reproducible easily on HSW (i5-4460) with: #rmmod snd_hda_intel The bisect shows this as the offending commit: commit 25e4abb33df3aafa7d1efba8f82f9178268efab1 Author: Libin YangDate: Tue Jan 12 11:13:27 2016 +0800 ALSA: hda - hdmi jack created based on pcm Jack is created based on pcm. Apply the acomp jack rule to dyn_pcm_assign. For dyn_pcm_assign: Driver does not use hda_jack. It operates snd_jack directly. snd_jack pointer will be stored in spec->pcm.jack instead of the current spec->acomp_jack. When pcm is assigned to pin, jack will be assigned to pin automatically. For !dyn_pcm_assign: Driver continues using hda_jack for less impact on the old cases. Pcm is statically assigned to pin. So is jack. spec->pcm.jack saves the snd_jack pointer created in hda_jack. Signed-off-by: Libin Yang Signed-off-by: Takashi Iwai [ 79.020523] BUG: unable to handle kernel paging request at 00015d80 [ 79.021314] IP: [] queued_spin_lock_slowpath+0xeb/0x180 [ 79.022125] PGD 0 [ 79.022881] Oops: 0002 [#1] SMP [ 79.023644] Modules linked in: snd_hda_codec_hdmi snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel(-) snd_hda_codec x86_pkg_temp_thermal snd_hwdep snd_hda_core i915 i2c_algo_bit drm_kms_helper syscopyarea sysfillrect sysimgblt ehci_pci fb_sys_fops r8169 ehci_hcd mii drm xhci_pci xhci_hcd video [ 79.026790] CPU: 0 PID: 1294 Comm: rmmod Tainted: G U 4.5.0-rc1+ #73 [ 79.028322] Hardware name: Dell Inc. Inspiron 3847/088DT1 , BIOS A06 01/15/2015 [ 79.029854] task: 8800dac9 ti: 88021270 task.ti: 88021270 [ 79.031384] RIP: 0010:[] [] queued_spin_lock_slowpath+0xeb/0x180 [ 79.032924] RSP: 0018:8802127039b0 EFLAGS: 00010002 [ 79.033698] RAX: 350c RBX: 8800d434ef90 RCX: 88021fa15d80 [ 79.035224] RDX: 00015d80 RSI: d434ef88 RDI: 8800d434ef90 [ 79.036751] RBP: 8802127039b0 R08: 0004 R09: [ 79.038277] R10: 8800daef8998 R11: 0004 R12: 0282 [ 79.039802] R13: R14: 0006 R15: 8800d434ef90 [ 79.041331] FS: 7f75be1ad700() GS:88021fa0() knlGS: [ 79.042863] CS: 0010 DS: ES: CR0: 80050033 [ 79.043638] CR2: 00015d80 CR3: 0002101b1000 CR4: 001406f0 [ 79.045166] Stack: [ 79.045926] 8802127039d0 81748e59 8800d434ed80 0005 [ 79.047462] 880212703a10 81597e73 817472d2 0003 [ 79.048995] 8800d434ecc0 0001 8800daef8800 [ 79.050528] Call Trace: [ 79.051293] [] _raw_spin_lock_irqsave+0x39/0x50 [ 79.052071] [] input_event+0x43/0x80 [ 79.052844] [] ? mutex_lock+0x12/0x30 [ 79.053619] [] snd_jack_report+0xee/0x110 [ 79.054396] [] hdmi_present_sense+0x13a/0x390 [snd_hda_codec_hdmi] [ 79.055921] [] ? regmap_unlock_mutex+0xe/0x10 [ 79.056699] [] generic_hdmi_resume+0x4d/0x60 [snd_hda_codec_hdmi] [ 79.058235] [] ? snd_hda_add_imux_item+0x120/0x120 [snd_hda_codec] [ 79.059771] [] hda_call_codec_resume+0xce/0x120 [snd_hda_codec] [ 79.061303] [] ? snd_hda_add_imux_item+0x120/0x120 [snd_hda_codec] [ 79.062835] [] hda_codec_runtime_resume+0x35/0x50 [snd_hda_codec] [ 79.064368] [] __rpm_callback+0x28/0x70 [ 79.065143] [] ? snd_hda_add_imux_item+0x120/0x120 [snd_hda_codec] [ 79.066676] [] rpm_callback+0x24/0x80 [ 79.067449] [] ? snd_hda_add_imux_item+0x120/0x120 [snd_hda_codec] [ 79.068983] [] rpm_resume+0x426/0x620 [ 79.069758] [] __pm_runtime_resume+0x4e/0x70 [ 79.070536] [] __device_release_driver+0x43/0x160 [ 79.071314] [] device_release_driver+0x23/0x30 [ 79.072085] [] bus_remove_device+0x101/0x170 [ 79.072862] [] device_del+0x139/0x270 [ 79.073638] [] ? widget_tree_free.isra.1+0x8b/0xa0 [snd_hda_core] [ 79.075170] [] snd_hdac_device_unregister+0x21/0x30 [snd_hda_core] [ 79.076703] [] snd_hda_codec_dev_free+0x1d/0x40 [snd_hda_codec] [ 79.078235] [] __snd_device_free+0x4c/0x80 [ 79.079010] [] snd_device_free_all+0x30/0x60 [ 79.079787] [] release_card_device+0x34/0x90 [ 79.080563] [] device_release+0x32/0x90 [ 79.081338] [] kobject_release+0x7a/0x190 [ 79.082113] [] kobject_put+0x27/0x50 [ 79.082886] [] put_device+0x17/0x20 [ 79.083659] [] snd_card_free+0x58/0x70 [ 79.084434] [] azx_remove+0x31/0x40 [snd_hda_intel] [ 79.085214] [] pci_device_remove+0x39/0xc0 [ 79.085990] [] __device_release_driver+0xa1/0x160 [ 79.086767] [] driver_detach+0xa6/0xb0 [ 79.087540] [] bus_remove_driver+0x55/0xd0 [ 79.088314] [] driver_unregister+0x2c/0x50 [ 79.089084] [] pci_unregister_driver+0x21/0x90
Re: [Intel-gfx] [PATCH 1/7] device: prevent a NULL pointer dereference in __intel_peek_fd
On Fri, Feb 12, 2016 at 06:31:23PM +0200, Martin Peres wrote: > This is not a big issue to return -1 since the only codepath that uses > it is for display purposes. Impossible. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 00/12] drm/i915: add missing display power refs
This patchset adds missing display power domain references during modeset HW readout, which I noticed via wakeref warnings the corresponding HW accesses triggered. The crt and ddi patches have a concrete bugzilla reference they fix, I don't know of reports that would be fixed by the rest of the patches in the set. On the way I also noticed other places which are potentially racy (debugfs CRC, VGA disabling, pipe_assert) so I fixed those up too. I left the IRQ setup and error capture code as-is: the former happens during driver loading and resume, so the power is guaranteed to stay on for the whole sequence. For error capture, we decided early on that we won't add any locking around these accesses to minimize the chance for locking inversions. Also for this we would need to grab a mutex which isn't possible on the error capture path. Mika came up with a very similar fix, since he suspects that it could also be related to recent DMC problems. We agreed that I send mine since it uses the more optimal rpm_get_if_in_use() helper (and looks more polished). Imre Deak (12): drm/i915: add helper to get a display power ref if it was already enabled drm/i915: ensure the HW is powered during display pipe HW readout drm/i915/ibx: ensure the HW is powered during PLL HW readout drm/i915: ensure the HW is powered when disabling VGA drm/i915: ensure the HW is powered during HW access in assert_pipe drm/i915/crt: ensure the HW is powered during HW state readout drm/i915/ddi: ensure the HW is powered during HW state readout drm/i915: ensure the HW is powered when accessing the CRC HW block drm/i915/dp: ensure the HW is powered during HW state readout drm/i915/dsi: ensure the HW is powered during HW state readout drm/i915/hdmi: ensure the HW is powered during HW state readout drm/i915/lvds: ensure the HW is powered during HW state readout drivers/gpu/drm/i915/i915_debugfs.c | 28 ++-- drivers/gpu/drm/i915/intel_crt.c| 13 +++- drivers/gpu/drm/i915/intel_ddi.c| 116 ++-- drivers/gpu/drm/i915/intel_display.c| 86 --- drivers/gpu/drm/i915/intel_dp.c | 18 +++-- drivers/gpu/drm/i915/intel_drv.h| 3 + drivers/gpu/drm/i915/intel_dsi.c| 13 +++- drivers/gpu/drm/i915/intel_hdmi.c | 14 +++- drivers/gpu/drm/i915/intel_lvds.c | 14 +++- drivers/gpu/drm/i915/intel_runtime_pm.c | 111 +- 10 files changed, 315 insertions(+), 101 deletions(-) -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 01/12] drm/i915: add helper to get a display power ref if it was already enabled
We have many places in the code where we check if a given display power domain is enabled and if so access registers backed by this power domain. We assumed that some modeset lock will prevent the power reference from vanishing in the middle of the HW access, but this assumption doesn't always hold. In such cases we get either the wakeref not held, or an unclaimed register access error message. To fix this in a future-proof way that's independent of other locks wrap any such access with a get_ref_if_enabled()/put_ref() pair. Kudos to Ville and Joonas for the ideas of this new interface. CC: Mika KuoppalaCC: Chris Wilson CC: Joonas Lahtinen CC: Ville Syrjälä Signed-off-by: Imre Deak --- drivers/gpu/drm/i915/intel_drv.h| 3 + drivers/gpu/drm/i915/intel_runtime_pm.c | 111 ++-- 2 files changed, 96 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 878172a..9380ffe 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1436,6 +1436,8 @@ bool __intel_display_power_is_enabled(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); void intel_display_power_get(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); +bool intel_display_power_get_if_enabled(struct drm_i915_private *dev_priv, + enum intel_display_power_domain domain); void intel_display_power_put(struct drm_i915_private *dev_priv, enum intel_display_power_domain domain); @@ -1522,6 +1524,7 @@ enable_rpm_wakeref_asserts(struct drm_i915_private *dev_priv) enable_rpm_wakeref_asserts(dev_priv) void intel_runtime_pm_get(struct drm_i915_private *dev_priv); +bool intel_runtime_pm_get_if_in_use(struct drm_i915_private *dev_priv); void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv); void intel_runtime_pm_put(struct drm_i915_private *dev_priv); diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index bbca527..a81f965 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -1435,39 +1435,90 @@ static void chv_pipe_power_well_disable(struct drm_i915_private *dev_priv, chv_set_pipe_power_well(dev_priv, power_well, false); } -/** - * intel_display_power_get - grab a power domain reference - * @dev_priv: i915 device instance - * @domain: power domain to reference - * - * This function grabs a power domain reference for @domain and ensures that the - * power domain and all its parents are powered up. Therefore users should only - * grab a reference to the innermost power domain they need. - * - * Any power domain reference obtained by this function must have a symmetric - * call to intel_display_power_put() to release the reference again. - */ -void intel_display_power_get(struct drm_i915_private *dev_priv, -enum intel_display_power_domain domain) +static void +__intel_display_power_get_domain(struct drm_i915_private *dev_priv, +enum intel_display_power_domain domain) { struct i915_power_domains *power_domains; struct i915_power_well *power_well; int i; - intel_runtime_pm_get(dev_priv); - power_domains = _priv->power_domains; - mutex_lock(_domains->lock); - for_each_power_well(i, power_well, BIT(domain), power_domains) { if (!power_well->count++) intel_power_well_enable(dev_priv, power_well); } power_domains->domain_use_count[domain]++; +} + +/** + * intel_display_power_get - grab a power domain reference + * @dev_priv: i915 device instance + * @domain: power domain to reference + * + * This function grabs a power domain reference for @domain and ensures that the + * power domain and all its parents are powered up. Therefore users should only + * grab a reference to the innermost power domain they need. + * + * Any power domain reference obtained by this function must have a symmetric + * call to intel_display_power_put() to release the reference again. + */ +void intel_display_power_get(struct drm_i915_private *dev_priv, +enum intel_display_power_domain domain) +{ + struct i915_power_domains *power_domains; + + intel_runtime_pm_get(dev_priv); + + power_domains = _priv->power_domains; + + mutex_lock(_domains->lock); + + __intel_display_power_get_domain(dev_priv, domain); + + mutex_unlock(_domains->lock); +} + +/** + * intel_display_power_get_if_enabled - grab a reference for an enabled display power domain + * @dev_priv: i915 device
[Intel-gfx] [PATCH 08/12] drm/i915: ensure the HW is powered when accessing the CRC HW block
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/i915_debugfs.c | 28 +--- 1 file changed, 21 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index ec0c2a05e..9e19cf0 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -825,8 +825,11 @@ static int i915_interrupt_info(struct seq_file *m, void *data) } for_each_pipe(dev_priv, pipe) { - if (!intel_display_power_is_enabled(dev_priv, - POWER_DOMAIN_PIPE(pipe))) { + enum intel_display_power_domain power_domain; + + power_domain = POWER_DOMAIN_PIPE(pipe); + if (!intel_display_power_get_if_enabled(dev_priv, + power_domain)) { seq_printf(m, "Pipe %c power disabled\n", pipe_name(pipe)); continue; @@ -840,6 +843,8 @@ static int i915_interrupt_info(struct seq_file *m, void *data) seq_printf(m, "Pipe %c IER:\t%08x\n", pipe_name(pipe), I915_READ(GEN8_DE_PIPE_IER(pipe))); + + intel_display_power_put(dev_priv, power_domain); } seq_printf(m, "Display Engine port interrupt mask:\t%08x\n", @@ -4004,6 +4009,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, struct intel_pipe_crc *pipe_crc = _priv->pipe_crc[pipe]; struct intel_crtc *crtc = to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe)); + enum intel_display_power_domain power_domain; u32 val = 0; /* shut up gcc */ int ret; @@ -4014,7 +4020,8 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, if (pipe_crc->source && source) return -EINVAL; - if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PIPE(pipe))) { + power_domain = POWER_DOMAIN_PIPE(pipe); + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) { DRM_DEBUG_KMS("Trying to capture CRC while pipe is off\n"); return -EIO; } @@ -4031,7 +4038,7 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, ret = ivb_pipe_crc_ctl_reg(dev, pipe, , ); if (ret != 0) - return ret; + goto out; /* none -> real source transition */ if (source) { @@ -4043,8 +4050,10 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, entries = kcalloc(INTEL_PIPE_CRC_ENTRIES_NR, sizeof(pipe_crc->entries[0]), GFP_KERNEL); - if (!entries) - return -ENOMEM; + if (!entries) { + ret = -ENOMEM; + goto out; + } /* * When IPS gets enabled, the pipe CRC changes. Since IPS gets @@ -4100,7 +4109,12 @@ static int pipe_crc_set_source(struct drm_device *dev, enum pipe pipe, hsw_enable_ips(crtc); } - return 0; + ret = 0; + +out: + intel_display_power_put(dev_priv, power_domain); + + return ret; } /* -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 09/12] drm/i915/dp: ensure the HW is powered during HW state readout
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_dp.c | 18 ++ 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 65d5084..34e644d 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2360,15 +2360,18 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder, struct drm_i915_private *dev_priv = dev->dev_private; enum intel_display_power_domain power_domain; u32 tmp; + bool ret; power_domain = intel_display_port_power_domain(encoder); - if (!intel_display_power_is_enabled(dev_priv, power_domain)) + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; + ret = false; + tmp = I915_READ(intel_dp->output_reg); if (!(tmp & DP_PORT_EN)) - return false; + goto out; if (IS_GEN7(dev) && port == PORT_A) { *pipe = PORT_TO_PIPE_CPT(tmp); @@ -2379,7 +2382,9 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder, u32 trans_dp = I915_READ(TRANS_DP_CTL(p)); if (TRANS_DP_PIPE_TO_PORT(trans_dp) == port) { *pipe = p; - return true; + ret = true; + + goto out; } } @@ -2391,7 +2396,12 @@ static bool intel_dp_get_hw_state(struct intel_encoder *encoder, *pipe = PORT_TO_PIPE(tmp); } - return true; + ret = true; + +out: + intel_display_power_put(dev_priv, power_domain); + + return ret; } static void intel_dp_get_config(struct intel_encoder *encoder, -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 04/12] drm/i915: ensure the HW is powered when disabling VGA
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_display.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index fe249ff..ca6efa6 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -15705,10 +15705,12 @@ void i915_redisable_vga(struct drm_device *dev) * level, just check if the power well is enabled instead of trying to * follow the "don't touch the power well if we don't need it" policy * the rest of the driver uses. */ - if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_VGA)) + if (!intel_display_power_get_if_enabled(dev_priv, POWER_DOMAIN_VGA)) return; i915_redisable_vga_power_on(dev); + + intel_display_power_put(dev_priv, POWER_DOMAIN_VGA); } static bool primary_get_hw_state(struct intel_plane *plane) -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 05/12] drm/i915: ensure the HW is powered during HW access in assert_pipe
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_display.c | 11 +++ 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ca6efa6..35f46a3 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -1343,18 +1343,21 @@ void assert_pipe(struct drm_i915_private *dev_priv, bool cur_state; enum transcoder cpu_transcoder = intel_pipe_to_cpu_transcoder(dev_priv, pipe); + enum intel_display_power_domain power_domain; /* if we need the pipe quirk it must be always on */ if ((pipe == PIPE_A && dev_priv->quirks & QUIRK_PIPEA_FORCE) || (pipe == PIPE_B && dev_priv->quirks & QUIRK_PIPEB_FORCE)) state = true; - if (!intel_display_power_is_enabled(dev_priv, - POWER_DOMAIN_TRANSCODER(cpu_transcoder))) { - cur_state = false; - } else { + power_domain = POWER_DOMAIN_TRANSCODER(cpu_transcoder); + if (intel_display_power_get_if_enabled(dev_priv, power_domain)) { u32 val = I915_READ(PIPECONF(cpu_transcoder)); cur_state = !!(val & PIPECONF_ENABLE); + + intel_display_power_put(dev_priv, power_domain); + } else { + cur_state = false; } I915_STATE_WARN(cur_state != state, -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCHv2 4/4] drm/i915/dp: Enable Upfront link training for typeC DP support on BXT
On Mon, Feb 01, 2016 at 07:27:53PM +0530, Durgadoss R wrote: > To support USB type C alternate DP mode, the display driver needs to > know the number of lanes required by the DP panel as well as number > of lanes that can be supported by the type-C cable. Sometimes, the > type-C cable may limit the bandwidth even if Panel can support > more lanes. To address these scenarios, the display driver will > start link training with max lanes, and if that fails, the driver > falls back to x2 lanes; and repeats this procedure for all > bandwidth/lane configurations. > > * Since link training is done before modeset only the port > (and not pipe/planes) and its associated PLLs are enabled. > * On DP hotplug: Directly start link training on the crtc > associated with the DP encoder. > * On Connected boot scenarios: When booted with an LFP and a DP, > typically, BIOS brings up DP. In these cases, we disable the > crtc, do upfront link training and then re-enable it back. > * All local changes made for upfront link training are reset > to their previous values once it is done; so that the > subsequent modeset is not aware of these changes. Glancing over this stuff, it doesn't look all that good on the locking front. Mainly there doesn't seem to locking. In general I'm not really convinced upfront link training is a very good idea if it needs a pipe. What happens if we fail to find a free pipe? I'd be much happier about this if we could make do without a pipe at least on some modern platforms and actually limit the feature to those platforms. PLLs are another concern, but I think modern platforms often have some kind of way to always get the standard DP frequencies from eg. the LCPLL. If we do go with the "hope you find a free pipe" approach, then it really needs to do that atomic locking stuff right. Otherwise I'd expect fireworks when someone plugs in a display while there's modeset happening. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] display: prevent a NULL pointer dereference in intel_set_scanout_pixmap
On Fri, Feb 12, 2016 at 06:31:24PM +0200, Martin Peres wrote: > Caught by Klockwork. > > Signed-off-by: Martin Peres> --- > src/uxa/intel_display.c | 5 + > 1 file changed, 5 insertions(+) > > diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c > index 8bf0184..44744a5 100644 > --- a/src/uxa/intel_display.c > +++ b/src/uxa/intel_display.c > @@ -688,6 +688,11 @@ intel_set_scanout_pixmap(xf86CrtcPtr crtc, PixmapPtr > ppix) > } > > bo = intel_get_pixmap_bo(ppix); > + if (!bo) { > + ErrorF("pixmap is not backed by a BO\n"); Just return FALSE. > + return FALSE; > + } > + > if (intel->front_buffer) { > ErrorF("have front buffer\n"); And let's add the return here as well instead of the ErrorF If the caller does make the mistake of passing in an invalid Pixmap, what's the likelihood of them checking the return? ;) -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/7] sna/cursor: add an assert on cursor->image
On Fri, Feb 12, 2016 at 06:31:27PM +0200, Martin Peres wrote: > Caught by Klockwork, but it was a false positive. However, better safe > than sorry. No. cursor->image can be NULL for old gen/kernels. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 06/12] drm/i915/crt: ensure the HW is powered during HW state readout
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93439 CC: Chris WilsonSigned-off-by: Imre Deak --- drivers/gpu/drm/i915/intel_crt.c | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c index ad5dfab..e686a91 100644 --- a/drivers/gpu/drm/i915/intel_crt.c +++ b/drivers/gpu/drm/i915/intel_crt.c @@ -71,22 +71,29 @@ static bool intel_crt_get_hw_state(struct intel_encoder *encoder, struct intel_crt *crt = intel_encoder_to_crt(encoder); enum intel_display_power_domain power_domain; u32 tmp; + bool ret; power_domain = intel_display_port_power_domain(encoder); - if (!intel_display_power_is_enabled(dev_priv, power_domain)) + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; + ret = false; + tmp = I915_READ(crt->adpa_reg); if (!(tmp & ADPA_DAC_ENABLE)) - return false; + goto out; if (HAS_PCH_CPT(dev)) *pipe = PORT_TO_PIPE_CPT(tmp); else *pipe = PORT_TO_PIPE(tmp); - return true; + ret = true; +out: + intel_display_power_put(dev_priv, power_domain); + + return ret; } static unsigned int intel_crt_get_flags(struct intel_encoder *encoder) -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 11/12] drm/i915/hdmi: ensure the HW is powered during HW state readout
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_hdmi.c | 14 +++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_hdmi.c b/drivers/gpu/drm/i915/intel_hdmi.c index edb7e90..80b44c0 100644 --- a/drivers/gpu/drm/i915/intel_hdmi.c +++ b/drivers/gpu/drm/i915/intel_hdmi.c @@ -880,15 +880,18 @@ static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, struct intel_hdmi *intel_hdmi = enc_to_intel_hdmi(>base); enum intel_display_power_domain power_domain; u32 tmp; + bool ret; power_domain = intel_display_port_power_domain(encoder); - if (!intel_display_power_is_enabled(dev_priv, power_domain)) + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; + ret = false; + tmp = I915_READ(intel_hdmi->hdmi_reg); if (!(tmp & SDVO_ENABLE)) - return false; + goto out; if (HAS_PCH_CPT(dev)) *pipe = PORT_TO_PIPE_CPT(tmp); @@ -897,7 +900,12 @@ static bool intel_hdmi_get_hw_state(struct intel_encoder *encoder, else *pipe = PORT_TO_PIPE(tmp); - return true; + ret = true; + +out: + intel_display_power_put(dev_priv, power_domain); + + return ret; } static void intel_hdmi_get_config(struct intel_encoder *encoder, -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 03/12] drm/i915/ibx: ensure the HW is powered during PLL HW readout
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_display.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 6abfc54..fe249ff 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -13699,7 +13699,7 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv, { uint32_t val; - if (!intel_display_power_is_enabled(dev_priv, POWER_DOMAIN_PLLS)) + if (!intel_display_power_get_if_enabled(dev_priv, POWER_DOMAIN_PLLS)) return false; val = I915_READ(PCH_DPLL(pll->id)); @@ -13707,6 +13707,8 @@ static bool ibx_pch_dpll_get_hw_state(struct drm_i915_private *dev_priv, hw_state->fp0 = I915_READ(PCH_FP0(pll->id)); hw_state->fp1 = I915_READ(PCH_FP1(pll->id)); + intel_display_power_put(dev_priv, POWER_DOMAIN_PLLS); + return val & DPLL_VCO_ENABLE; } -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 02/12] drm/i915: ensure the HW is powered during display pipe HW readout
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_display.c | 67 ++-- 1 file changed, 48 insertions(+), 19 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 836bbdc..6abfc54 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -8181,18 +8181,22 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; + enum intel_display_power_domain power_domain; uint32_t tmp; + bool ret; - if (!intel_display_power_is_enabled(dev_priv, - POWER_DOMAIN_PIPE(crtc->pipe))) + power_domain = POWER_DOMAIN_PIPE(crtc->pipe); + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe; pipe_config->shared_dpll = DPLL_ID_PRIVATE; + ret = false; + tmp = I915_READ(PIPECONF(crtc->pipe)); if (!(tmp & PIPECONF_ENABLE)) - return false; + goto out; if (IS_G4X(dev) || IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) { switch (tmp & PIPECONF_BPC_MASK) { @@ -8272,7 +8276,12 @@ static bool i9xx_get_pipe_config(struct intel_crtc *crtc, pipe_config->base.adjusted_mode.crtc_clock = pipe_config->port_clock / pipe_config->pixel_multiplier; - return true; + ret = true; + +out: + intel_display_power_put(dev_priv, power_domain); + + return ret; } static void ironlake_init_pch_refclk(struct drm_device *dev) @@ -9376,18 +9385,21 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc, { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; + enum intel_display_power_domain power_domain; uint32_t tmp; + bool ret; - if (!intel_display_power_is_enabled(dev_priv, - POWER_DOMAIN_PIPE(crtc->pipe))) + power_domain = POWER_DOMAIN_PIPE(crtc->pipe); + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe; pipe_config->shared_dpll = DPLL_ID_PRIVATE; + ret = false; tmp = I915_READ(PIPECONF(crtc->pipe)); if (!(tmp & PIPECONF_ENABLE)) - return false; + goto out; switch (tmp & PIPECONF_BPC_MASK) { case PIPECONF_6BPC: @@ -9450,7 +9462,12 @@ static bool ironlake_get_pipe_config(struct intel_crtc *crtc, ironlake_get_pfit_config(crtc, pipe_config); - return true; + ret = true; + +out: + intel_display_power_put(dev_priv, power_domain); + + return ret; } static void assert_can_disable_lcpll(struct drm_i915_private *dev_priv) @@ -9982,12 +,17 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, { struct drm_device *dev = crtc->base.dev; struct drm_i915_private *dev_priv = dev->dev_private; - enum intel_display_power_domain pfit_domain; + enum intel_display_power_domain power_domain; + unsigned long power_domain_mask; uint32_t tmp; + bool ret; - if (!intel_display_power_is_enabled(dev_priv, -POWER_DOMAIN_PIPE(crtc->pipe))) + power_domain = POWER_DOMAIN_PIPE(crtc->pipe); + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; + power_domain_mask = BIT(power_domain); + + ret = false; pipe_config->cpu_transcoder = (enum transcoder) crtc->pipe; pipe_config->shared_dpll = DPLL_ID_PRIVATE; @@ -10014,13 +10036,14 @@ static bool haswell_get_pipe_config(struct intel_crtc *crtc, pipe_config->cpu_transcoder = TRANSCODER_EDP; } - if (!intel_display_power_is_enabled(dev_priv, - POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder))) - return false; + power_domain = POWER_DOMAIN_TRANSCODER(pipe_config->cpu_transcoder); + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) + goto out; + power_domain_mask |= BIT(power_domain); tmp = I915_READ(PIPECONF(pipe_config->cpu_transcoder)); if (!(tmp & PIPECONF_ENABLE)) - return false; + goto out;
[Intel-gfx] [PATCH 12/12] drm/i915/lvds: ensure the HW is powered during HW state readout
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_lvds.c | 14 +++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c index 811ddf7..30a8403 100644 --- a/drivers/gpu/drm/i915/intel_lvds.c +++ b/drivers/gpu/drm/i915/intel_lvds.c @@ -76,22 +76,30 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder, struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(>base); enum intel_display_power_domain power_domain; u32 tmp; + bool ret; power_domain = intel_display_port_power_domain(encoder); - if (!intel_display_power_is_enabled(dev_priv, power_domain)) + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; + ret = false; + tmp = I915_READ(lvds_encoder->reg); if (!(tmp & LVDS_PORT_EN)) - return false; + goto out; if (HAS_PCH_CPT(dev)) *pipe = PORT_TO_PIPE_CPT(tmp); else *pipe = PORT_TO_PIPE(tmp); - return true; + ret = true; + +out: + intel_display_power_put(dev_priv, power_domain); + + return ret; } static void intel_lvds_get_config(struct intel_encoder *encoder, -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 10/12] drm/i915/dsi: ensure the HW is powered during HW state readout
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Signed-off-by: Imre Deak--- drivers/gpu/drm/i915/intel_dsi.c | 13 ++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_dsi.c b/drivers/gpu/drm/i915/intel_dsi.c index 378f879..fcd746c 100644 --- a/drivers/gpu/drm/i915/intel_dsi.c +++ b/drivers/gpu/drm/i915/intel_dsi.c @@ -664,13 +664,16 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, struct drm_device *dev = encoder->base.dev; enum intel_display_power_domain power_domain; enum port port; + bool ret; DRM_DEBUG_KMS("\n"); power_domain = intel_display_port_power_domain(encoder); - if (!intel_display_power_is_enabled(dev_priv, power_domain)) + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; + ret = false; + /* XXX: this only works for one DSI output */ for_each_dsi_port(port, intel_dsi->ports) { i915_reg_t ctrl_reg = IS_BROXTON(dev) ? @@ -691,12 +694,16 @@ static bool intel_dsi_get_hw_state(struct intel_encoder *encoder, if (dpi_enabled || (func & CMD_MODE_DATA_WIDTH_MASK)) { if (I915_READ(MIPI_DEVICE_READY(port)) & DEVICE_READY) { *pipe = port == PORT_A ? PIPE_A : PIPE_B; - return true; + ret = true; + + goto out; } } } +out: + intel_display_power_put(dev_priv, power_domain); - return false; + return ret; } static void intel_dsi_get_config(struct intel_encoder *encoder, -- 2.5.0 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 07/12] drm/i915/ddi: ensure the HW is powered during HW state readout
The assumption when adding the intel_display_power_is_enabled() checks was that if it returns success the power can't be turned off afterwards during the HW access, which is guaranteed by modeset locks. This isn't always true, so make sure we hold a dedicated reference for the time of the access. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=93441 CC: Chris WilsonSigned-off-by: Imre Deak --- drivers/gpu/drm/i915/intel_ddi.c | 116 +++ 1 file changed, 81 insertions(+), 35 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index cdf2e14..21a9b83 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -1913,13 +1913,16 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector) enum transcoder cpu_transcoder; enum intel_display_power_domain power_domain; uint32_t tmp; + bool ret; power_domain = intel_display_port_power_domain(intel_encoder); - if (!intel_display_power_is_enabled(dev_priv, power_domain)) + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; - if (!intel_encoder->get_hw_state(intel_encoder, )) - return false; + if (!intel_encoder->get_hw_state(intel_encoder, )) { + ret = false; + goto out; + } if (port == PORT_A) cpu_transcoder = TRANSCODER_EDP; @@ -1931,23 +1934,33 @@ bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector) switch (tmp & TRANS_DDI_MODE_SELECT_MASK) { case TRANS_DDI_MODE_SELECT_HDMI: case TRANS_DDI_MODE_SELECT_DVI: - return (type == DRM_MODE_CONNECTOR_HDMIA); + ret = type == DRM_MODE_CONNECTOR_HDMIA; + break; case TRANS_DDI_MODE_SELECT_DP_SST: - if (type == DRM_MODE_CONNECTOR_eDP) - return true; - return (type == DRM_MODE_CONNECTOR_DisplayPort); + ret = type == DRM_MODE_CONNECTOR_eDP || + type == DRM_MODE_CONNECTOR_DisplayPort; + break; + case TRANS_DDI_MODE_SELECT_DP_MST: /* if the transcoder is in MST state then * connector isn't connected */ - return false; + ret = false; + break; case TRANS_DDI_MODE_SELECT_FDI: - return (type == DRM_MODE_CONNECTOR_VGA); + ret = type == DRM_MODE_CONNECTOR_VGA; + break; default: - return false; + ret = false; + break; } + +out: + intel_display_power_put(dev_priv, power_domain); + + return ret; } bool intel_ddi_get_hw_state(struct intel_encoder *encoder, @@ -1959,15 +1972,18 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, enum intel_display_power_domain power_domain; u32 tmp; int i; + bool ret; power_domain = intel_display_port_power_domain(encoder); - if (!intel_display_power_is_enabled(dev_priv, power_domain)) + if (!intel_display_power_get_if_enabled(dev_priv, power_domain)) return false; + ret = false; + tmp = I915_READ(DDI_BUF_CTL(port)); if (!(tmp & DDI_BUF_CTL_ENABLE)) - return false; + goto out; if (port == PORT_A) { tmp = I915_READ(TRANS_DDI_FUNC_CTL(TRANSCODER_EDP)); @@ -1985,25 +2001,32 @@ bool intel_ddi_get_hw_state(struct intel_encoder *encoder, break; } - return true; - } else { - for (i = TRANSCODER_A; i <= TRANSCODER_C; i++) { - tmp = I915_READ(TRANS_DDI_FUNC_CTL(i)); - - if ((tmp & TRANS_DDI_PORT_MASK) - == TRANS_DDI_SELECT_PORT(port)) { - if ((tmp & TRANS_DDI_MODE_SELECT_MASK) == TRANS_DDI_MODE_SELECT_DP_MST) - return false; - - *pipe = i; - return true; - } + ret = true; + + goto out; + } + + for (i = TRANSCODER_A; i <= TRANSCODER_C; i++) { + tmp = I915_READ(TRANS_DDI_FUNC_CTL(i)); + + if ((tmp & TRANS_DDI_PORT_MASK) == TRANS_DDI_SELECT_PORT(port)) { + if ((tmp & TRANS_DDI_MODE_SELECT_MASK) == + TRANS_DDI_MODE_SELECT_DP_MST) + goto out; + + *pipe = i; + ret = true; + + goto out; } } DRM_DEBUG_KMS("No pipe for ddi port %c found\n", port_name(port)); -
Re: [Intel-gfx] [PATCH] drm/i915: Avoid vblank counter for gen9+
doh!! sent the wrong version... going to sent the right one now... On Fri, 2016-02-12 at 09:32 +0800, kbuild test robot wrote: > Hi Rodrigo, > > [auto build test WARNING on drm-intel/for-linux-next] > [also build test WARNING on v4.5-rc3 next-20160211] > [if your patch is applied to the wrong git tree, please drop us a > note to help improving the system] > > url:https://github.com/0day-ci/linux/commits/Rodrigo-Vivi/drm-i91 > 5-Avoid-vblank-counter-for-gen9/20160212-090608 > base: git://anongit.freedesktop.org/drm-intel for-linux-next > config: x86_64-randconfig-x000-201606 (attached as .config) > reproduce: > # save the attached .config to linux build tree > make ARCH=x86_64 > > All warnings (new ones prefixed by >>): > >In file included from include/uapi/linux/stddef.h:1:0, > from include/linux/stddef.h:4, > from include/uapi/linux/posix_types.h:4, > from include/uapi/linux/types.h:13, > from include/linux/types.h:5, > from include/linux/sysrq.h:18, > from drivers/gpu/drm/i915/i915_irq.c:31: >drivers/gpu/drm/i915/i915_irq.c: In function 'intel_irq_init': >drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: comparison of > constant '9' with boolean expression is always false [-Wbool-compare] > if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > ^ >include/linux/compiler.h:147:28: note: in definition of macro > '__trace_if' > if (__builtin_constant_p((cond)) ? !!(cond) : \ >^ > > > drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of > > > macro 'if' > if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > ^ >drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: logical not is > only applied to the left hand side of comparison [-Wlogical-not > -parentheses] > if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > ^ >include/linux/compiler.h:147:28: note: in definition of macro > '__trace_if' > if (__builtin_constant_p((cond)) ? !!(cond) : \ >^ > > > drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of > > > macro 'if' > if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > ^ >drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: comparison of > constant '9' with boolean expression is always false [-Wbool-compare] > if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > ^ >include/linux/compiler.h:147:40: note: in definition of macro > '__trace_if' > if (__builtin_constant_p((cond)) ? !!(cond) : \ >^ > > > drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of > > > macro 'if' > if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > ^ >drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: logical not is > only applied to the left hand side of comparison [-Wlogical-not > -parentheses] > if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > ^ >include/linux/compiler.h:147:40: note: in definition of macro > '__trace_if' > if (__builtin_constant_p((cond)) ? !!(cond) : \ >^ > > > drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of > > > macro 'if' > if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > ^ >drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: comparison of > constant '9' with boolean expression is always false [-Wbool-compare] > if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > ^ >include/linux/compiler.h:158:16: note: in definition of macro > '__trace_if' > __r = !!(cond); \ >^ > > > drivers/gpu/drm/i915/i915_irq.c:4578:2: note: in expansion of > > > macro 'if' > if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > ^ >drivers/gpu/drm/i915/i915_irq.c:4578:55: warning: logical not is > only applied to the left hand side of comparison [-Wlogical-not > -parentheses] > if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->ge
Re: [Intel-gfx] [PATCH] drm/i915: Avoid vblank counter for gen9+
On Thu, Feb 11, 2016 at 09:00:47AM -0800, Rodrigo Vivi wrote: > Framecounter register is read-only so DMC cannot restore it > after exiting DC5 and DC6. > > Easiest way to go is to avoid the counter and use vblank > interruptions for this platform and for all the following > ones since DMC came to stay. At least while we can't change > this register to read-write. > > Signed-off-by: Rodrigo Vivi> --- > drivers/gpu/drm/i915/i915_irq.c | 7 +-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c > index 25a8937..c294a4b 100644 > --- a/drivers/gpu/drm/i915/i915_irq.c > +++ b/drivers/gpu/drm/i915/i915_irq.c > @@ -4556,7 +4556,10 @@ void intel_irq_init(struct drm_i915_private *dev_priv) > > pm_qos_add_request(_priv->pm_qos, PM_QOS_CPU_DMA_LATENCY, > PM_QOS_DEFAULT_VALUE); > > - if (IS_GEN2(dev_priv)) { > + if (INTEL_INFO(dev_priv)->gen >= 9) { > + dev->max_vblank_count = 0; > + dev->driver->get_vblank_counter = g4x_get_vblank_counter; = i8xx_get_vblank_counter > + } else if (IS_GEN2(dev_priv)) { > dev->max_vblank_count = 0; > dev->driver->get_vblank_counter = i8xx_get_vblank_counter; > } else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) { > @@ -4572,7 +4575,7 @@ void intel_irq_init(struct drm_i915_private *dev_priv) >* Gen2 doesn't have a hardware frame counter and so depends on >* vblank interrupts to produce sane vblank seuquence numbers. >*/ > - if (!IS_GEN2(dev_priv)) > + if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > dev->vblank_disable_immediate = true; You can just drop this part. > > dev->driver->get_vblank_timestamp = i915_get_vblank_timestamp; > -- > 2.4.3 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Execlist irq handler micro optimisations
On 12/02/16 14:42, Chris Wilson wrote: On Fri, Feb 12, 2016 at 12:00:40PM +, Tvrtko Ursulin wrote: From: Tvrtko UrsulinAssorted changes most likely without any practical effect apart from a tiny reduction in generated code for the interrupt handler and request submission. * Remove needless initialization. * Improve cache locality by reorganizing code and/or using branch hints to keep unexpected or error conditions out of line. * Favor busy submit path vs. empty queue. * Less branching in hot-paths. v2: * Avoid mmio reads when possible. (Chris Wilson) * Use natural integer size for csb indices. * Remove useless return value from execlists_update_context. * Extract 32-bit ppgtt PDPs update so it is out of line and shared with two callers. * Grab forcewake across all mmio operations to ease the load on uncore lock and use chepear mmio ops. Version 2 now makes the irq handling code path ~20% smaller on 48-bit PPGTT hardware, and a little bit less elsewhere. Hot paths are mostly in-line now and hammering on the uncore spinlock is greatly reduced together with mmio traffic to an extent. Did you notice that ring->next_context_status_buffer is redundant as we also have that information to hand in status_pointer? I didn't and don't know that part that well. There might be some future proofing issues around it as well. What's your thinking for if (req->elsp_submitted & ring->gen8_9) vs a plain if (req->elsp_submitted) ? Another don't know this part that well. Is it not useful to not submit two noops if they are not needed? Do they still end up submitted to the GPU somehow? The tidies look good. Be useful to double check whether gem_latency is behaving as a canary, it's a bit of a puzzle why that first dispatch latency would grow. Yes a puzzle, no idea how and why. But "gem_latency -n 100" does not show this regression. I've done a hundred runs and these are the results: * Throughput up 4.04% * Dispatch latency down 0.37% * Consumer and producer latencies down 22.53% * CPU time down 2.25% So it all looks good. Regards, Tvrtko ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Avoid vblank counter for gen9+
Oh, actually please just ignore this patch completely. More work here need to be done. This apparently helped on the issue that I was facing here but doesnt' solve completely. On Fri, 2016-02-12 at 07:58 -0800, Rodrigo Vivi wrote: > On Fri, 2016-02-12 at 11:34 +0200, David Weinehall wrote: > > On Thu, Feb 11, 2016 at 09:00:47AM -0800, Rodrigo Vivi wrote: > > > Framecounter register is read-only so DMC cannot restore it > > > after exiting DC5 and DC6. > > > > > > Easiest way to go is to avoid the counter and use vblank > > > interruptions for this platform and for all the following > > > ones since DMC came to stay. At least while we can't change > > > this register to read-write. > > > > > > Signed-off-by: Rodrigo Vivi> > > --- > > > drivers/gpu/drm/i915/i915_irq.c | 7 +-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > > b/drivers/gpu/drm/i915/i915_irq.c > > > index 25a8937..c294a4b 100644 > > > --- a/drivers/gpu/drm/i915/i915_irq.c > > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > > @@ -4556,7 +4556,10 @@ void intel_irq_init(struct > > > drm_i915_private > > > *dev_priv) > > > > > > pm_qos_add_request(_priv->pm_qos, > > > PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE); > > > > > > - if (IS_GEN2(dev_priv)) { > > > + if (INTEL_INFO(dev_priv)->gen >= 9) { > > > + dev->max_vblank_count = 0; > > > + dev->driver->get_vblank_counter = > > > g4x_get_vblank_counter; > > > + } else if (IS_GEN2(dev_priv)) { > > > dev->max_vblank_count = 0; > > > dev->driver->get_vblank_counter = > > > i8xx_get_vblank_counter; > > > } else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen > > > > = 5) { > > > @@ -4572,7 +4575,7 @@ void intel_irq_init(struct drm_i915_private > > > *dev_priv) > > >* Gen2 doesn't have a hardware frame counter and so > > > depends on > > >* vblank interrupts to produce sane vblank seuquence > > > numbers. > > >*/ > > > - if (!IS_GEN2(dev_priv)) > > > + if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= > > > 9) > > > > I think this should be: > > > > if (INTEL_INFO(dev_priv)->gen < 9) > > Yeap, < 9 is better... > > > > > If gen < 9, then IS_GEN2 is always true, also ! has higher > > precedence > > than >=, so you're essentially comparing whether the logical > > negation > > of > > INTEL_INFO(dev_priv)->gen is >= 9. > > but it is also !gen2 > > So I believe the right is > > if (INTEL_INFO(dev_priv)->gen < 9 && !IS_GEN2(dev_priv)) > > agree? > > > > > > > Kind regards, David ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/7] sna: assert a pointer is non-NULL before dereferencing it
On Fri, Feb 12, 2016 at 06:31:26PM +0200, Martin Peres wrote: > Caught by Klockwork. Impossible. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Avoid vblank counter for gen9+
On Fri, 2016-02-12 at 11:34 +0200, David Weinehall wrote: > On Thu, Feb 11, 2016 at 09:00:47AM -0800, Rodrigo Vivi wrote: > > Framecounter register is read-only so DMC cannot restore it > > after exiting DC5 and DC6. > > > > Easiest way to go is to avoid the counter and use vblank > > interruptions for this platform and for all the following > > ones since DMC came to stay. At least while we can't change > > this register to read-write. > > > > Signed-off-by: Rodrigo Vivi> > --- > > drivers/gpu/drm/i915/i915_irq.c | 7 +-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_irq.c > > b/drivers/gpu/drm/i915/i915_irq.c > > index 25a8937..c294a4b 100644 > > --- a/drivers/gpu/drm/i915/i915_irq.c > > +++ b/drivers/gpu/drm/i915/i915_irq.c > > @@ -4556,7 +4556,10 @@ void intel_irq_init(struct drm_i915_private > > *dev_priv) > > > > pm_qos_add_request(_priv->pm_qos, > > PM_QOS_CPU_DMA_LATENCY, PM_QOS_DEFAULT_VALUE); > > > > - if (IS_GEN2(dev_priv)) { > > + if (INTEL_INFO(dev_priv)->gen >= 9) { > > + dev->max_vblank_count = 0; > > + dev->driver->get_vblank_counter = > > g4x_get_vblank_counter; > > + } else if (IS_GEN2(dev_priv)) { > > dev->max_vblank_count = 0; > > dev->driver->get_vblank_counter = > > i8xx_get_vblank_counter; > > } else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen > > >= 5) { > > @@ -4572,7 +4575,7 @@ void intel_irq_init(struct drm_i915_private > > *dev_priv) > > * Gen2 doesn't have a hardware frame counter and so > > depends on > > * vblank interrupts to produce sane vblank seuquence > > numbers. > > */ > > - if (!IS_GEN2(dev_priv)) > > + if (!IS_GEN2(dev_priv) && !INTEL_INFO(dev_priv)->gen >= 9) > > I think this should be: > > if (INTEL_INFO(dev_priv)->gen < 9) Yeap, < 9 is better... > > If gen < 9, then IS_GEN2 is always true, also ! has higher precedence > than >=, so you're essentially comparing whether the logical negation > of > INTEL_INFO(dev_priv)->gen is >= 9. but it is also !gen2 So I believe the right is if (INTEL_INFO(dev_priv)->gen < 9 && !IS_GEN2(dev_priv)) agree? > > > Kind regards, David ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 03/38] drm/i915: Prelude to splitting i915_gem_do_execbuffer in two
On 04/02/2016 17:01, Jesse Barnes wrote: On 01/11/2016 10:42 AM, john.c.harri...@intel.com wrote: From: John HarrisonThe scheduler decouples the submission of batch buffers to the driver with their submission to the hardware. This basically means splitting the execbuffer() function in half. This change rearranges some code ready for the split to occur. For: VIZ-1587 Signed-off-by: John Harrison --- drivers/gpu/drm/i915/i915_gem_execbuffer.c | 63 ++ drivers/gpu/drm/i915/intel_lrc.c | 18 ++--- 2 files changed, 51 insertions(+), 30 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c index bfc4c17..0eca2b6 100644 --- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c +++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c @@ -933,10 +933,7 @@ i915_gem_execbuffer_move_to_gpu(struct drm_i915_gem_request *req, if (flush_domains & I915_GEM_DOMAIN_GTT) wmb(); - /* Unconditionally invalidate gpu caches and ensure that we do flush -* any residual writes from the previous batch. -*/ - return intel_ring_invalidate_all_caches(req); + return 0; } static bool @@ -1189,17 +1186,6 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, u32 instp_mask; int ret; - ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas); - if (ret) - return ret; - - ret = i915_switch_context(params->request); - if (ret) - return ret; - - WARN(params->ctx->ppgtt && params->ctx->ppgtt->pd_dirty_rings & (1 name); - instp_mode = args->flags & I915_EXEC_CONSTANTS_MASK; instp_mask = I915_EXEC_CONSTANTS_MASK; switch (instp_mode) { @@ -1233,11 +1219,37 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, return -EINVAL; } + ret = i915_gem_execbuffer_move_to_gpu(params->request, vmas); + if (ret) + return ret; + + i915_gem_execbuffer_move_to_active(vmas, params->request); + + /* To be split into two functions here... */ + + intel_runtime_pm_get(dev_priv); + + /* +* Unconditionally invalidate gpu caches and ensure that we do flush +* any residual writes from the previous batch. +*/ + ret = intel_ring_invalidate_all_caches(params->request); + if (ret) + goto error; + + /* Switch to the correct context for the batch */ + ret = i915_switch_context(params->request); + if (ret) + goto error; + + WARN(params->ctx->ppgtt && params->ctx->ppgtt->pd_dirty_rings & (1 name); + if (ring == _priv->ring[RCS] && instp_mode != dev_priv->relative_constants_mode) { ret = intel_ring_begin(params->request, 4); if (ret) - return ret; + goto error; intel_ring_emit(ring, MI_NOOP); intel_ring_emit(ring, MI_LOAD_REGISTER_IMM(1)); @@ -1251,7 +1263,7 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, if (args->flags & I915_EXEC_GEN7_SOL_RESET) { ret = i915_reset_gen7_sol_offsets(dev, params->request); if (ret) - return ret; + goto error; } exec_len = args->batch_len; @@ -1262,14 +1274,20 @@ i915_gem_ringbuffer_submission(struct i915_execbuffer_params *params, exec_start, exec_len, params->dispatch_flags); if (ret) - return ret; + goto error; trace_i915_gem_ring_dispatch(params->request, params->dispatch_flags); - i915_gem_execbuffer_move_to_active(vmas, params->request); i915_gem_execbuffer_retire_commands(params); - return 0; +error: + /* +* intel_gpu_busy should also get a ref, so it will free when the device +* is really idle. +*/ + intel_runtime_pm_put(dev_priv); + + return ret; } /** @@ -1424,8 +1442,6 @@ i915_gem_do_execbuffer(struct drm_device *dev, void *data, dispatch_flags |= I915_DISPATCH_RS; } - intel_runtime_pm_get(dev_priv); - ret = i915_mutex_lock_interruptible(dev); if (ret) goto pre_mutex_err; @@ -1599,9 +1615,6 @@ err: mutex_unlock(>struct_mutex); pre_mutex_err: - /* intel_gpu_busy should also get a ref, so it will free when the device -* is really idle. */ - intel_runtime_pm_put(dev_priv); return ret; } diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c index e510730..4bf0ee6
[Intel-gfx] [PATCH 3/7] sna: check for NULL before dereferencing a pointer
Caught by Klockwork and I genuinely can't tell if it is safe without it, especially since all the surrounding code is checking for NULL. Signed-off-by: Martin Peres--- src/sna/sna_accel.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sna/sna_accel.c b/src/sna/sna_accel.c index 71a6207..8ceb1e1 100644 --- a/src/sna/sna_accel.c +++ b/src/sna/sna_accel.c @@ -3688,7 +3688,8 @@ sna_drawable_use_bo(DrawablePtr drawable, unsigned flags, const BoxRec *box, __FUNCTION__, priv->flush, priv->shm, priv->cpu, flags)); if ((flags & PREFER_GPU) == 0 && - (flags & (REPLACES | IGNORE_DAMAGE) || !priv->gpu_damage || !kgem_bo_is_busy(priv->gpu_bo))) { + (flags & (REPLACES | IGNORE_DAMAGE) || !priv->gpu_damage || +(priv->gpu_bo && !kgem_bo_is_busy(priv->gpu_bo { DBG(("%s: try cpu as GPU bo is idle\n", __FUNCTION__)); goto use_cpu_bo; } -- 2.7.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 7/7] sna: add an assert in the lengthy sna_drawable_use_bo
I got lost in all the changes done on priv->flush and gpu_bo enough not to be able to guarantee that if flush is non-null, so is gpu_bo. Caught by Klockwork. Signed-off-by: Martin Peres--- src/sna/sna_accel.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sna/sna_accel.c b/src/sna/sna_accel.c index f1c136a..595faa6 100644 --- a/src/sna/sna_accel.c +++ b/src/sna/sna_accel.c @@ -4074,6 +4074,7 @@ prefer_gpu_bo: } if (priv->flush) { assert(!priv->shm); + assert(priv->gpu_bo); sna_add_flush_pixmap(sna, priv, priv->gpu_bo); } -- 2.7.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 4/7] sna: assert a pointer is non-NULL before dereferencing it
Caught by Klockwork. Signed-off-by: Martin Peres--- src/sna/sna_accel.c | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sna/sna_accel.c b/src/sna/sna_accel.c index 8ceb1e1..f1c136a 100644 --- a/src/sna/sna_accel.c +++ b/src/sna/sna_accel.c @@ -4376,6 +4376,7 @@ sna_pixmap_move_to_gpu(PixmapPtr pixmap, unsigned flags) if (priv->shm) { assert(!priv->flush); + assert(priv->cpu_bo); sna_add_flush_pixmap(sna, priv, priv->cpu_bo); } -- 2.7.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/7] sna_video_sprite: add asserts to catch invalid pipe#
Caught by Klockwork. This will be enough to catch those issues during bringup. Signed-off-by: Martin Peres--- src/sna/sna_video_sprite.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/sna/sna_video_sprite.c b/src/sna/sna_video_sprite.c index 9e85049..ae08ef7 100644 --- a/src/sna/sna_video_sprite.c +++ b/src/sna/sna_video_sprite.c @@ -86,6 +86,7 @@ static int sna_video_sprite_stop(ddStopVideo_ARGS) int pipe; pipe = sna_crtc_pipe(crtc); + assert(pipe < ARRAY_SIZE(video->bo)); if (video->bo[pipe] == NULL) continue; @@ -260,6 +261,7 @@ sna_video_sprite_show(struct sna *sna, video->color_key_changed &= ~(1 << pipe); } + assert(pipe < ARRAY_SIZE(video->bo)); if (video->bo[pipe] == frame->bo) return true; @@ -415,6 +417,7 @@ static int sna_video_sprite_put_image(ddPutImage_ARGS) RegionIntersect(, , ); if (RegionNil()) { off: + assert(pipe < ARRAY_SIZE(video->bo)); if (video->bo[pipe]) { struct local_mode_set_plane s; memset(, 0, sizeof(s)); -- 2.7.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/7] display: prevent a NULL pointer dereference in intel_set_scanout_pixmap
Caught by Klockwork. Signed-off-by: Martin Peres--- src/uxa/intel_display.c | 5 + 1 file changed, 5 insertions(+) diff --git a/src/uxa/intel_display.c b/src/uxa/intel_display.c index 8bf0184..44744a5 100644 --- a/src/uxa/intel_display.c +++ b/src/uxa/intel_display.c @@ -688,6 +688,11 @@ intel_set_scanout_pixmap(xf86CrtcPtr crtc, PixmapPtr ppix) } bo = intel_get_pixmap_bo(ppix); + if (!bo) { + ErrorF("pixmap is not backed by a BO\n"); + return FALSE; + } + if (intel->front_buffer) { ErrorF("have front buffer\n"); } -- 2.7.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V5] drm/i915: edp resume/On time optimization.
On Fri, Jan 22, 2016 at 05:39:04PM -0800, abhay.ku...@intel.com wrote: > From: Abhay Kumar> > Make resume/on codepath not to wait for panel_power_cycle_delay(t11_t12) > if this time is already spent in suspend/poweron time. > > v2: Use CLOCK_BOOTTIME and remove jiffies for panel power cycle > delay calculation(Ville). > > v3: Addressed below comments > 1. Tracking time from where last powercycle is initiated. > 2. Used ktime_get_bootime() wrapper for boottime clock. > 3. Used ktime_ms_delta() to get time difference. > > v4: Updated v3 change log in detail. > > v5: Removed static from panel_power_on_time(Stéphane). > > Cc: Ville Syrjälä > Signed-off-by: Abhay Kumar > Reviewed-by: Ville Syrjälä Queued for -next, thanks for the patch. -Daniel > --- > drivers/gpu/drm/i915/intel_dp.c | 19 ++- > drivers/gpu/drm/i915/intel_drv.h | 2 +- > 2 files changed, 15 insertions(+), 6 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index e2bea710..29f21c1 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1811,12 +1811,21 @@ static void wait_panel_off(struct intel_dp *intel_dp) > > static void wait_panel_power_cycle(struct intel_dp *intel_dp) > { > + ktime_t panel_power_on_time; > + s64 panel_power_off_duration; > + > DRM_DEBUG_KMS("Wait for panel power cycle\n"); > > + /* take the difference of currrent time and panel power off time > + * and then make panel wait for t11_t12 if needed. */ > + panel_power_on_time = ktime_get_boottime(); > + panel_power_off_duration = ktime_ms_delta(panel_power_on_time, > intel_dp->panel_power_off_time); > + > /* When we disable the VDD override bit last we have to do the manual >* wait. */ > - wait_remaining_ms_from_jiffies(intel_dp->last_power_cycle, > -intel_dp->panel_power_cycle_delay); > + if (panel_power_off_duration < (s64)intel_dp->panel_power_cycle_delay) > + wait_remaining_ms_from_jiffies(jiffies, > +intel_dp->panel_power_cycle_delay - > panel_power_off_duration); > > wait_panel_status(intel_dp, IDLE_CYCLE_MASK, IDLE_CYCLE_VALUE); > } > @@ -1968,7 +1977,7 @@ static void edp_panel_vdd_off_sync(struct intel_dp > *intel_dp) > I915_READ(pp_stat_reg), I915_READ(pp_ctrl_reg)); > > if ((pp & POWER_TARGET_ON) == 0) > - intel_dp->last_power_cycle = jiffies; > + intel_dp->panel_power_off_time = ktime_get_boottime(); > > power_domain = intel_display_port_aux_power_domain(intel_encoder); > intel_display_power_put(dev_priv, power_domain); > @@ -2117,7 +2126,7 @@ static void edp_panel_off(struct intel_dp *intel_dp) > I915_WRITE(pp_ctrl_reg, pp); > POSTING_READ(pp_ctrl_reg); > > - intel_dp->last_power_cycle = jiffies; > + intel_dp->panel_power_off_time = ktime_get_boottime(); > wait_panel_off(intel_dp); > > /* We got a reference when we enabled the VDD. */ > @@ -5116,7 +5125,7 @@ intel_dp_add_properties(struct intel_dp *intel_dp, > struct drm_connector *connect > > static void intel_dp_init_panel_power_timestamps(struct intel_dp *intel_dp) > { > - intel_dp->last_power_cycle = jiffies; > + intel_dp->panel_power_off_time = ktime_get_boottime(); > intel_dp->last_power_on = jiffies; > intel_dp->last_backlight_off = jiffies; > } > diff --git a/drivers/gpu/drm/i915/intel_drv.h > b/drivers/gpu/drm/i915/intel_drv.h > index bc97012..137e40d 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -770,9 +770,9 @@ struct intel_dp { > int backlight_off_delay; > struct delayed_work panel_vdd_work; > bool want_panel_vdd; > - unsigned long last_power_cycle; > unsigned long last_power_on; > unsigned long last_backlight_off; > + ktime_t panel_power_off_time; > > struct notifier_block edp_notifier; > > -- > 1.9.1 > > ___ > 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 https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Execlist irq handler micro optimisations
On Fri, Feb 12, 2016 at 03:54:27PM +, Tvrtko Ursulin wrote: > > On 12/02/16 14:42, Chris Wilson wrote: > >On Fri, Feb 12, 2016 at 12:00:40PM +, Tvrtko Ursulin wrote: > >>From: Tvrtko Ursulin> >> > >>Assorted changes most likely without any practical effect > >>apart from a tiny reduction in generated code for the interrupt > >>handler and request submission. > >> > >> * Remove needless initialization. > >> * Improve cache locality by reorganizing code and/or using > >>branch hints to keep unexpected or error conditions out > >>of line. > >> * Favor busy submit path vs. empty queue. > >> * Less branching in hot-paths. > >> > >>v2: > >> > >> * Avoid mmio reads when possible. (Chris Wilson) > >> * Use natural integer size for csb indices. > >> * Remove useless return value from execlists_update_context. > >> * Extract 32-bit ppgtt PDPs update so it is out of line and > >>shared with two callers. > >> * Grab forcewake across all mmio operations to ease the > >>load on uncore lock and use chepear mmio ops. > >> > >>Version 2 now makes the irq handling code path ~20% smaller on > >>48-bit PPGTT hardware, and a little bit less elsewhere. Hot > >>paths are mostly in-line now and hammering on the uncore > >>spinlock is greatly reduced together with mmio traffic to an > >>extent. > > > >Did you notice that ring->next_context_status_buffer is redundant as we > >also have that information to hand in status_pointer? > > I didn't and don't know that part that well. There might be some > future proofing issues around it as well. Unlikely :-p > >What's your thinking for > > > > if (req->elsp_submitted & ring->gen8_9) > > > >vs a plain > > > > if (req->elsp_submitted) > >? > > Another don't know this part that well. Is it not useful to not > submit two noops if they are not needed? Do they still end up > submitted to the GPU somehow? The command streamer always has to execute them since they lie between the last dispatch TAIL and the next TAIL (in the lrc). All we do here is to tweak the request->tail value, that may or may not be used the next time we write the ELSP (depending on whether we are submitting the same live request again). (The next request's tail will include the noops before it's dispatch.) > >The tidies look good. Be useful to double check whether gem_latency is > >behaving as a canary, it's a bit of a puzzle why that first dispatch > >latency would grow. > > Yes a puzzle, no idea how and why. But "gem_latency -n 100" does not > show this regression. I've done a hundred runs and these are the > results: > > * Throughput up 4.04% > * Dispatch latency down 0.37% > * Consumer and producer latencies down 22.53% > * CPU time down 2.25% > > So it all looks good. Yup. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 5/7] sna/cursor: add an assert on cursor->image
Caught by Klockwork, but it was a false positive. However, better safe than sorry. Signed-off-by: Martin Peres--- src/sna/sna_display.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/sna/sna_display.c b/src/sna/sna_display.c index 9215b23..78937c2 100644 --- a/src/sna/sna_display.c +++ b/src/sna/sna_display.c @@ -5573,8 +5573,9 @@ static struct sna_cursor *__sna_get_cursor(struct sna *sna, xf86CrtcPtr crtc) argb = get_cursor_argb(sna->cursor.ref); pitch = BitmapBytePad(width); + assert(cursor->image); image = cursor->image; - if (image == NULL || transformed) { + if (transformed) { image = sna->cursor.scratch; cursor->last_width = cursor->last_height = size; } -- 2.7.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/7] device: prevent a NULL pointer dereference in __intel_peek_fd
This is not a big issue to return -1 since the only codepath that uses it is for display purposes. Caught by Klockwork. Signed-off-by: Martin Peres--- src/intel_device.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/intel_device.c b/src/intel_device.c index 54c1443..35e652a 100644 --- a/src/intel_device.c +++ b/src/intel_device.c @@ -650,7 +650,10 @@ int __intel_peek_fd(ScrnInfoPtr scrn) dev = intel_device(scrn); assert(dev && dev->fd != -1); - return dev->fd; + if (!dev) + return -1; + else + return dev->fd; } int intel_has_render_node(struct intel_device *dev) -- 2.7.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH DDX 0/7] Some issues found with Klockwork
The DDX clearly is making Klockwork unhappy but I found those potential issues. Let's use them as a basis for discussion on the real code that should be written. Martin Peres (7): device: prevent a NULL pointer dereference in __intel_peek_fd display: prevent a NULL pointer dereference in intel_set_scanout_pixmap sna: check for NULL before dereferencing a pointer sna: assert a pointer is non-NULL before dereferencing it sna/cursor: add an assert on cursor->image sna_video_sprite: add asserts to catch invalid pipe# sna: add an assert in the lengthy sna_drawable_use_bo src/intel_device.c | 5 - src/sna/sna_accel.c| 5 - src/sna/sna_display.c | 3 ++- src/sna/sna_video_sprite.c | 3 +++ src/uxa/intel_display.c| 5 + 5 files changed, 18 insertions(+), 3 deletions(-) -- 2.7.1 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH i-g-t] tests/prime_mmap: Encapsulate check_for_dma_buf_mmap() in igt_fixture.
I'm not sure this actually fix the root problem. With or without your patch applied, I'm seeing the following in lib/tests/test-suite.log: ../../tests/prime_mmap: Checking invalid option handling... Checking valid option handling... Checking subtest enumeration... (prime_mmap:24023) ioctl-wrappers-CRITICAL: Test assertion failure function gem_create, file ../../lib/ioctl_wrappers.c:501: (prime_mmap:24023) ioctl-wrappers-CRITICAL: Failed assertion: drmIoctl((fd), 2U|1U) << (((0+8)+8)+14)) | ((('d')) << (0+8)) | (((0x40 + 0x1b)) << 0) | sizeof(struct drm_i915_gem_create << ((0+8)+8, ()) == 0 (prime_mmap:24023) ioctl-wrappers-CRITICAL: Last errno: 25, Inappropriate ioctl for device (prime_mmap:24023) ioctl-wrappers-CRITICAL: error: -1 != 0 Test prime_mmap failed. DEBUG (prime_mmap:24023) ioctl-wrappers-CRITICAL: Test assertion failure function gem_create, file ../../lib/ioctl_wrappers.c:501: (prime_mmap:24023) ioctl-wrappers-CRITICAL: Failed assertion: drmIoctl((fd), 2U|1U) << (((0+8)+8)+14)) | ((('d')) << (0+8)) | (((0x40 + 0x1b)) << 0) | sizeof(struct drm_i915_gem_create << ((0+8)+8, ()) == 0 (prime_mmap:24023) ioctl-wrappers-CRITICAL: Last errno: 25, Inappropriate ioctl for device (prime_mmap:24023) ioctl-wrappers-CRITICAL: error: -1 != 0 END prime_mmap: ../../lib/igt_core.c:1014: igt_fail: Assertion `!test_with_subtests || in_fixture' failed. Received signal SIGABRT. Aborted (core dumped) Tiago On 02/12/2016 11:32 AM, Marius Vlad wrote: This unbreaks distcheck target that in turn runs each test with --list-subtests. Signed-off-by: Marius Vlad--- tests/prime_mmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/prime_mmap.c b/tests/prime_mmap.c index 29a0cfd..1ea61c2 100644 --- a/tests/prime_mmap.c +++ b/tests/prime_mmap.c @@ -504,10 +504,10 @@ igt_main igt_fixture { fd = drm_open_driver(DRIVER_INTEL); + igt_skip_on((check_for_dma_buf_mmap() != 0)); errno = 0; } - igt_skip_on((check_for_dma_buf_mmap() != 0)); for (i = 0; i < ARRAY_SIZE(tests); i++) { igt_subtest(tests[i].name) ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH V4] drm/i915/skl: SKL CDCLK change on modeset tracking VCO
On 02/12/2016 03:18 AM, Ville Syrjälä wrote: On Thu, Feb 11, 2016 at 03:22:08PM -0800, clinton.a.tay...@intel.com wrote: From: Clint TaylorTrack VCO frequency of SKL instead of the boot CDCLK and allow modeset to set cdclk based on the max required pixel clock based on VCO selected. The vco should be tracked at the atomic level and all CRTCs updated if the required vco is changed. At this time the eDP pll is configured inside the encoder which has no visibility into the atomic state. When eDP v1.4 panel that require the 8640 vco are available this may need to be investigated. V1: initial version V2: add vco tracking in intel_dp_compute_config(), rename skl_boot_cdclk. V3: rebase, V2 feedback not possible as encoders are not aware of atomic. V4: track target vco is atomic state. modeset all CRTCs if vco changes Signed-off-by: Clint Taylor Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= --- drivers/gpu/drm/i915/i915_drv.h |2 +- drivers/gpu/drm/i915/intel_ddi.c |2 +- drivers/gpu/drm/i915/intel_display.c | 97 +- drivers/gpu/drm/i915/intel_dp.c | 10 ++-- drivers/gpu/drm/i915/intel_drv.h |4 ++ 5 files changed, 97 insertions(+), 18 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8216665..f65dd1a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1822,7 +1822,7 @@ struct drm_i915_private { int num_fence_regs; /* 8 on pre-965, 16 otherwise */ unsigned int fsb_freq, mem_freq, is_ddr3; - unsigned int skl_boot_cdclk; + unsigned int skl_vco_freq; unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq; unsigned int max_dotclk_freq; unsigned int hpll_freq; diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 6d5b09f..285adab 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2958,7 +2958,7 @@ void intel_ddi_pll_init(struct drm_device *dev) int cdclk_freq; cdclk_freq = dev_priv->display.get_display_clock_speed(dev); - dev_priv->skl_boot_cdclk = cdclk_freq; + dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq); This should really read out the vco from the hardware. But I think we can leave that for later. The problem really is the 540MHz case since it can be using either vco frequency (IIRC). if (skl_sanitize_cdclk(dev_priv)) DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9e2273b..ef4ac34 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5663,7 +5663,7 @@ static unsigned int skl_cdclk_decimal(unsigned int freq) return (freq - 1000) / 500; } -static unsigned int skl_cdclk_get_vco(unsigned int freq) +unsigned int skl_cdclk_get_vco(unsigned int freq) { unsigned int i; @@ -5821,17 +5821,17 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv) void skl_init_cdclk(struct drm_i915_private *dev_priv) { - unsigned int required_vco; - /* DPLL0 not enabled (happens on early BIOS versions) */ if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) { /* enable DPLL0 */ - required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk); - skl_dpll0_enable(dev_priv, required_vco); + if (dev_priv->skl_vco_freq != 8640) { + dev_priv->skl_vco_freq = 8100; + } + skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq); } /* set CDCLK to the frequency the BIOS chose */ - skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk); + skl_set_cdclk(dev_priv, (dev_priv->skl_vco_freq == 8100) ? 337500 : 308570 ); We really shouldn't change the cdclk if there are active outputs. This whole area definitely needs more work, for BXT too I already have some BXT patches lined up in some branch that frob around these parts a bit, so maybe I should extend that stuff to SKL as well. In the meantime we don't want to cause a regression at least, so maybe we can just do something like: if (!LCPLL_ENABLE) { ... cdclk = vco == 8100 ? ...; } else { cdclk = dev_priv->cdclk_freq; } set_cdclk(cdclk); /* enable DBUF power */ I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST); @@ -5847,7 +5847,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) { uint32_t lcpll1 = I915_READ(LCPLL1_CTL); uint32_t cdctl = I915_READ(CDCLK_CTL); - int freq = dev_priv->skl_boot_cdclk; + int freq = dev_priv->cdclk_freq; /*
[Intel-gfx] [PATCH 2/3] drm/i915: Enable PSR by default on Valleyview and Cherryview.
With a reliable frontbuffer tracking and all instability corner cases solved for this platform let's re-enabled PSR by default. In case a new issue is found and PSR is the main suspect, please check if i915.enable_psr=0 really makes your problem go away, please report it at bugs.freedesktop.org. In a bugzilla entry for PSR is desirable: - dmesg (drm.debug=0xe) - output of /sys/kernel/debug/dri/0/i915_edp_psr_status - Platform information. Vendor, model, id, pci id. - Graphical environment: Gnome, KDE, openbox, etc... - Details how to reproduce. - Also good if you could run PSR test cases of Intel-gpu-tools - Please mention if forcing main link standby or main link off helps you. There are Intel-gpu-tools test cases that can be helpful to determine if PSR is working as expected: kms_psr_sink_crc and kms_psr_frontbuffer_tracking. Cc: Paulo ZanoniSigned-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/intel_psr.c | 5 - 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 655bdf6..12b5a7e 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -780,7 +780,10 @@ void intel_psr_init(struct drm_device *dev) /* Per platform default */ if (i915.enable_psr == -1) { - i915.enable_psr = 0; + if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) + i915.enable_psr = 1; + else + i915.enable_psr = 0; } /* Set link_standby x link_off defaults */ -- 2.4.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/3] Enable PSR by default in certain platforms
For a long time we are trying to enable PSR by default everwhere but we always end up in soem specific corner case in one platform or another and end up not enabling in older stable platforms that could benefit from the power saving it provides. So, let's change the approach here and try to enable it separated. The new blocking issue is for Skylake and Kabylake due to a vblank wait situation with DMC firmware: https://bugs.freedesktop.org/show_bug.cgi?id=94126 So that case will block PSR on SKL and KBL. But this doesn't affect at all other platforms that don't have DMC. Also I decided to split the rest of platforms in two groups since Valleyview/Cherryview has a complete different Hardware PSR implementation than the rest of the platorms. Also it uses only link standby what should be safier. So, 1 patch to add the possibility of enable per platform, 1 patch for VLV/CHV and 1 patch for HSW/BDW. I hope to get SKL/KBL ready soon. Thanks, Rodrigo. Rodrigo Vivi (3): drm/i915: Change i915.enable_psr parameter to use per platform default. drm/i915: Enable PSR by default on Valleyview and Cherryview. drm/i915: Enable PSR by default on Haswell and Broadwell. drivers/gpu/drm/i915/i915_params.c | 5 +++-- drivers/gpu/drm/i915/intel_psr.c | 9 + 2 files changed, 12 insertions(+), 2 deletions(-) -- 2.4.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/3] drm/i915: Enable PSR by default on Haswell and Broadwell.
With a reliable frontbuffer tracking and all instability corner cases on Haswell and Broadwell solved let's re-enabled PSR by default on these platforms. In case a new issue is found and PSR is the main suspect, please check if i915.enable_psr=0 really makes your problem go away. If this is the case PSR is the culprit so after that please check if i915.enable_psr=2 or i915.enable_psr=3 solves your issue and please let us know. There are many panels out there and not all implementations apparently work as we would expect. In case you needed to force it on standby or disabled or in case of any PSR related bug please report it at bugs.freedesktop.org. In a bugzilla entry for PSR is desirable: - dmesg (drm.debug=0xe) - output of /sys/kernel/debug/dri/0/i915_edp_psr_status - Platform information. Vendor, model, id, pci id. - Graphical environment: Gnome, KDE, openbox, etc... - Details how to reproduce. - Also good if you could run PSR test cases of Intel-gpu-tools - Please mention if forcing main link standby or main link off helps you. There are Intel-gpu-tools test cases that can be helpful to determine if PSR is working as expected: kms_psr_sink_crc and kms_psr_frontbuffer_tracking. Cc: Paulo ZanoniSigned-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/intel_psr.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 12b5a7e..0b42ada 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -780,7 +780,8 @@ void intel_psr_init(struct drm_device *dev) /* Per platform default */ if (i915.enable_psr == -1) { - if (IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) + if (IS_HASWELL(dev) || IS_BROADWELL(dev) || + IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev)) i915.enable_psr = 1; else i915.enable_psr = 0; -- 2.4.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ Fi.CI.BAT: warning for Add drm_dp_aux chardev support. (rev5)
On Fri, Feb 12, 2016 at 01:21:33PM -0800, Rafael Antognolli wrote: > On Fri, Feb 12, 2016 at 02:01:11PM +0200, Ville Syrjälä wrote: > > On Fri, Jan 22, 2016 at 09:15:31AM -, Patchwork wrote: > > > == Summary == > > > > > > Built on 8fe9e785ae04fa7c37f7935cff12d62e38054b60 drm-intel-nightly: > > > 2016y-01m-21d-11h-02m-42s UTC integration manifest > > > > > > Test kms_flip: > > > Subgroup basic-flip-vs-dpms: > > > pass -> DMESG-WARN (ilk-hp8440p) > > > > The usual ilk underrun > > https://bugs.freedesktop.org/show_bug.cgi?id=93787 > > > > > Subgroup basic-flip-vs-modeset: > > > pass -> DMESG-WARN (skl-i7k-2) > > > > This is the known "DMC doesn't know what state it's in" problem. > > Does it mean that I have to fix it on my patch, or is it something > expected from this test? It's unrelated. Hopefully Mika will have applied enough duct tape soon to hide it better. > > > > pass -> DMESG-WARN (ilk-hp8440p) > > > > Another ilk underrun. > > > > > > > > bdw-nuci7total:140 pass:131 dwarn:0 dfail:0 fail:0 skip:9 > > > > > > bdw-ultratotal:143 pass:137 dwarn:0 dfail:0 fail:0 skip:6 > > > > > > bsw-nuc-2total:143 pass:119 dwarn:0 dfail:0 fail:0 > > > skip:24 > > > byt-nuc total:143 pass:128 dwarn:0 dfail:0 fail:0 > > > skip:15 > > > hsw-brixbox total:143 pass:136 dwarn:0 dfail:0 fail:0 skip:7 > > > > > > ilk-hp8440p total:143 pass:102 dwarn:3 dfail:0 fail:0 > > > skip:38 > > > ivb-t430stotal:143 pass:137 dwarn:0 dfail:0 fail:0 skip:6 > > > > > > skl-i5k-2total:143 pass:134 dwarn:1 dfail:0 fail:0 skip:8 > > > > > > skl-i7k-2total:143 pass:133 dwarn:2 dfail:0 fail:0 skip:8 > > > > > > snb-dellxps total:143 pass:129 dwarn:0 dfail:0 fail:0 > > > skip:14 > > > snb-x220ttotal:143 pass:129 dwarn:0 dfail:0 fail:1 > > > skip:13 > > > > > > Results at /archive/results/CI_IGT_test/Patchwork_1244/ > > > > > > ___ > > > Intel-gfx mailing list > > > Intel-gfx@lists.freedesktop.org > > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v10 4/4] drm/radeon: Fix WARN_ON if DRM_DP_AUX_CHARDEV is enabled
On Thu, Jan 21, 2016 at 6:10 PM, Rafael Antognolliwrote: > From: Lukas Wunner > > Rafael Antognolli's new DRM_DP_AUX_CHARDEV feature causes a WARN_ON > if drm_dp_aux->dev == drm_connector->kdev and drm_dp_aux_unregister() > is called after drm_connector_unregister(). radeon is the only driver > affected by this besides i915. (amdgpu calls drm_dp_aux_unregister() > before drm_connector_unregister().) > > Cc: Rafael Antognolli > Cc: Ville Syrjälä > Cc: Alex Deucher > Signed-off-by: Lukas Wunner Applied. thanks! Alex > --- > drivers/gpu/drm/radeon/radeon_display.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/radeon_display.c > b/drivers/gpu/drm/radeon/radeon_display.c > index b3bb923..a885dae 100644 > --- a/drivers/gpu/drm/radeon/radeon_display.c > +++ b/drivers/gpu/drm/radeon/radeon_display.c > @@ -1684,6 +1684,9 @@ void radeon_modeset_fini(struct radeon_device *rdev) > radeon_fbdev_fini(rdev); > kfree(rdev->mode_info.bios_hardcoded_edid); > > + /* free i2c buses */ > + radeon_i2c_fini(rdev); > + > if (rdev->mode_info.mode_config_initialized) { > radeon_afmt_fini(rdev); > drm_kms_helper_poll_fini(rdev->ddev); > @@ -1691,8 +1694,6 @@ void radeon_modeset_fini(struct radeon_device *rdev) > drm_mode_config_cleanup(rdev->ddev); > rdev->mode_info.mode_config_initialized = false; > } > - /* free i2c buses */ > - radeon_i2c_fini(rdev); > } > > static bool is_hdtv_mode(const struct drm_display_mode *mode) > -- > 2.4.3 > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [REGRESSION] i915: No HDMI output with 4.4
On Fri, Feb 12, 2016 at 09:52:03AM +0200, Oleksandr Natalenko wrote: > Ville, > > I've applied patch you've provided and did couple of replugging with > intel_reg in between. Here are the results. > > I used additional VGA cable to see what actually I type in console :). > My life would have been a bit easier if you had included the reg dumps in the mail. Copy paste manually this time. > Both HDMI and VGA cables plugged: [1] (0x000c4000): 0x0008 (0x000c4004): 0xf1b5 (0x000c4008): 0x (0x000c400c): 0x (0x000c4030): 0x00101010 > Both HDMI and VGA cables unplugged: [2] (0x000c4000): 0x (0x000c4004): 0xf1b5 (0x000c4008): 0x (0x000c400c): 0x (0x000c4030): 0x00101010 > Only HDMI cable plugged: [3] (0x000c4000): 0x (0x000c4004): 0xf1b5 (0x000c4008): 0x (0x000c400c): 0x (0x000c4030): 0x00101010 > Only VGA cable plugged: [4] (0x000c4000): 0x0008 (0x000c4004): 0xf1b4 (0x000c4008): 0x (0x000c400c): 0x (0x000c4030): 0x00101010 What these show is that the live status for the digital ports never goes to 1, which is rather wtf. VGA gets reported correctly. Everything else looks normal to me. > And here goes dmesg with all the stuff logged: [5] лют 12 09:37:01 pfactum.lanet kernel: port C live status __ __ __ __ __ Same deal here. The live status never indicates anything being present during the 250ms that we poll it. Few other ideas: - Was the monitor sleeping when you tried this? Can you maybe push some button on it and then immediately run the intel_reg read command again? - Do you have another monitor to try? - Do you have another cable to try? - Maybe the pullup/down on the hpd line is misconfigured or something. Any chance of updating the BIOS on the machine? - What does 'intel_reg read 0xc2000 0xc2004 0xc2020' say? - The spec claims the TMDS vs. SDVO select has something to do with hpd generation. I can't see any difference on my IVB though, so not sure it's really true. What does 'intel_reg read 0xe1140 0xe1150 0xe1160' tell us? Let's try these anyway (with the cable plugged in): intel_reg write 0xe1140 0x0 intel_reg write 0xe1150 0x0 intel_reg write 0xe1160 0x0 sleep 1 intel_reg read 0xc4000 intel_reg write 0xe1140 0x800 intel_reg write 0xe1150 0x800 intel_reg write 0xe1160 0x800 sleep 1 intel_reg read 0xc4000 intel_reg write 0xe1140 0x800800 intel_reg write 0xe1150 0x800800 intel_reg write 0xe1160 0x800800 sleep 1 intel_reg read 0xc4000 intel_reg write 0xe1140 0x80 intel_reg write 0xe1150 0x80 intel_reg write 0xe1160 0x80 sleep 1 intel_reg read 0xc4000 > > Hope this helps. > > [1] https://gist.github.com/58a0eb50dcf84e104555 > [2] https://gist.github.com/7e8749a3e2cc58ea8aac > [3] https://gist.github.com/9d76930da7380634b845 > [4] https://gist.github.com/c0d2e2f64242ad4f01f2 > [5] https://gist.github.com/fda3b9fed3ca4d31cd20 > > 11.02.2016 16:01, Ville Syrjälä wrote: > > OK, so the hpd interrupt does happen, and yet the live status > > supposedly > > claims that nothing is there. Port C live status definitely works here > > on my IVB, so not sure what the deal is. > > > > Can you grab intel-gpu-tools and run > > intel_reg read 0xc4000 0xc4004 0xc4008 0xc400c 0xc4030 > > a couple of times after plugging the monitor in, and also run it when > > nothing is plugged in. > > > > Also you could try something like the following patch so we might > > observe the live status with a bit more detail. Though the fact that it > > doesn't seem to work for you even when the monitor was already plugged > > in is somewhat troubling: > > > > --- a/drivers/gpu/drm/i915/intel_hdmi.c > > +++ b/drivers/gpu/drm/i915/intel_hdmi.c > > @@ -1392,12 +1392,17 @@ intel_hdmi_detect(struct
Re: [Intel-gfx] [PATCH v10 3/4] drm/i915: Set aux.dev to the drm_connector device, instead of drm_device.
On Thu, 2016-01-21 at 15:10 -0800, Rafael Antognolli wrote: > So far, the i915 driver and some other drivers set it to the > drm_device, > which doesn't allow one to know which DP a given aux channel is > related > to. Changing this to be the drm_connector provides proper nesting, > still > allowing one to get the drm_device from it. Some drivers already set > it > to the drm_connector. > > This also removes the need to add a sysfs link for the i2c device > under > the connector, as it will already be there. Yes, having the i2c-dev only at one place under the connector is more logical and what we want imo. It's an ABI change but if other drivers already have it in this way then I assume it's fine. For HDMI connectors we still have them under the drm_device (and there is no symlink for them under the connector device) but this was inconsistent already before this patch and can be fixed up later. Adding Jani, in case he has comments on this. Below one more comment: > v9: > - As a side effect, drm_dp_aux_unregister() must be called before > intel_connector_unregister(), as both the aux.dev and the i2c > adapter > dev are children of the drm_connector device now. Calling > drm_dp_aux_unregister() before prevents them from being destroyed > twice. > > v10: > - move aux_fini() to connector_unregister(), instead of moving > drm_dp_aux_unregister() outside of connector_register(). > > Signed-off-by: Rafael Antognolli> --- > drivers/gpu/drm/i915/intel_dp.c | 18 ++ > 1 file changed, 2 insertions(+), 16 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c > b/drivers/gpu/drm/i915/intel_dp.c > index e2bea710..da704c6 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -1188,7 +1188,6 @@ intel_dp_aux_fini(struct intel_dp *intel_dp) > static int > intel_dp_aux_init(struct intel_dp *intel_dp, struct intel_connector > *connector) > { > - struct drm_device *dev = intel_dp_to_dev(intel_dp); > struct intel_digital_port *intel_dig_port = > dp_to_dig_port(intel_dp); > enum port port = intel_dig_port->port; > int ret; > @@ -1199,7 +1198,7 @@ intel_dp_aux_init(struct intel_dp *intel_dp, > struct intel_connector *connector) > if (!intel_dp->aux.name) > return -ENOMEM; > > - intel_dp->aux.dev = dev->dev; > + intel_dp->aux.dev = connector->base.kdev; > intel_dp->aux.transfer = intel_dp_aux_transfer; > > DRM_DEBUG_KMS("registering %s bus for %s\n", > @@ -1214,16 +1213,6 @@ intel_dp_aux_init(struct intel_dp *intel_dp, > struct intel_connector *connector) > return ret; > } > > - ret = sysfs_create_link(>base.kdev->kobj, > - _dp->aux.ddc.dev.kobj, > - intel_dp->aux.ddc.dev.kobj.name); > - if (ret < 0) { > - DRM_ERROR("sysfs_create_link() for %s failed > (%d)\n", > - intel_dp->aux.name, ret); > - intel_dp_aux_fini(intel_dp); > - return ret; > - } > - > return 0; > } > > @@ -1232,9 +1221,7 @@ intel_dp_connector_unregister(struct > intel_connector *intel_connector) > { > struct intel_dp *intel_dp = > intel_attached_dp(_connector->base); > > - if (!intel_connector->mst_port) > - sysfs_remove_link(_connector->base.kdev->kobj, > - intel_dp->aux.ddc.dev.kobj.name); > + intel_dp_aux_fini(intel_dp); Hrm, the mst_port check seems to have been misplaced at some point, this function isn't called for virtual MST connectors. So I think it's fine to remove it. The patch looks ok: Reviewed-by: Imre Deak > intel_connector_unregister(intel_connector); > } > > @@ -4868,7 +4855,6 @@ void intel_dp_encoder_destroy(struct > drm_encoder *encoder) > struct intel_digital_port *intel_dig_port = > enc_to_dig_port(encoder); > struct intel_dp *intel_dp = _dig_port->dp; > > - intel_dp_aux_fini(intel_dp); > intel_dp_mst_encoder_cleanup(intel_dig_port); > if (is_edp(intel_dp)) { > cancel_delayed_work_sync(_dp->panel_vdd_work); ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/3] drm/i915: Change i915.enable_psr parameter to use per platform default.
This will give us flexibility to enable PSR by default independently so issues and corner cases in one platform won't affect others were we have it working properly. Cc: Paulo ZanoniSigned-off-by: Rodrigo Vivi --- drivers/gpu/drm/i915/i915_params.c | 5 +++-- drivers/gpu/drm/i915/intel_psr.c | 5 + 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c index 8b9f368..1b40ee6 100644 --- a/drivers/gpu/drm/i915/i915_params.c +++ b/drivers/gpu/drm/i915/i915_params.c @@ -38,7 +38,7 @@ struct i915_params i915 __read_mostly = { .enable_execlists = -1, .enable_hangcheck = true, .enable_ppgtt = -1, - .enable_psr = 0, + .enable_psr = -1, .preliminary_hw_support = IS_ENABLED(CONFIG_DRM_I915_PRELIMINARY_HW_SUPPORT), .disable_power_well = -1, .enable_ips = 1, @@ -128,7 +128,8 @@ MODULE_PARM_DESC(enable_execlists, module_param_named_unsafe(enable_psr, i915.enable_psr, int, 0600); MODULE_PARM_DESC(enable_psr, "Enable PSR " -"(0=disabled [default], 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode)"); +"(0=disabled, 1=enabled - link mode chosen per-platform, 2=force link-standby mode, 3=force link-off mode) " +"Default: -1 (use per-chip default)"); module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, int, 0600); MODULE_PARM_DESC(preliminary_hw_support, diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c index 4ab7579..655bdf6 100644 --- a/drivers/gpu/drm/i915/intel_psr.c +++ b/drivers/gpu/drm/i915/intel_psr.c @@ -778,6 +778,11 @@ void intel_psr_init(struct drm_device *dev) dev_priv->psr_mmio_base = IS_HASWELL(dev_priv) ? HSW_EDP_PSR_BASE : BDW_EDP_PSR_BASE; + /* Per platform default */ + if (i915.enable_psr == -1) { + i915.enable_psr = 0; + } + /* Set link_standby x link_off defaults */ if (IS_HASWELL(dev) || IS_BROADWELL(dev)) /* HSW and BDW require workarounds that we don't implement. */ -- 2.4.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] ✗ Fi.CI.BAT: warning for Add drm_dp_aux chardev support. (rev5)
On Fri, Feb 12, 2016 at 02:01:11PM +0200, Ville Syrjälä wrote: > On Fri, Jan 22, 2016 at 09:15:31AM -, Patchwork wrote: > > == Summary == > > > > Built on 8fe9e785ae04fa7c37f7935cff12d62e38054b60 drm-intel-nightly: > > 2016y-01m-21d-11h-02m-42s UTC integration manifest > > > > Test kms_flip: > > Subgroup basic-flip-vs-dpms: > > pass -> DMESG-WARN (ilk-hp8440p) > > The usual ilk underrun > https://bugs.freedesktop.org/show_bug.cgi?id=93787 > > > Subgroup basic-flip-vs-modeset: > > pass -> DMESG-WARN (skl-i7k-2) > > This is the known "DMC doesn't know what state it's in" problem. Does it mean that I have to fix it on my patch, or is it something expected from this test? > > pass -> DMESG-WARN (ilk-hp8440p) > > Another ilk underrun. > > > > > bdw-nuci7total:140 pass:131 dwarn:0 dfail:0 fail:0 skip:9 > > bdw-ultratotal:143 pass:137 dwarn:0 dfail:0 fail:0 skip:6 > > bsw-nuc-2total:143 pass:119 dwarn:0 dfail:0 fail:0 skip:24 > > byt-nuc total:143 pass:128 dwarn:0 dfail:0 fail:0 skip:15 > > hsw-brixbox total:143 pass:136 dwarn:0 dfail:0 fail:0 skip:7 > > ilk-hp8440p total:143 pass:102 dwarn:3 dfail:0 fail:0 skip:38 > > ivb-t430stotal:143 pass:137 dwarn:0 dfail:0 fail:0 skip:6 > > skl-i5k-2total:143 pass:134 dwarn:1 dfail:0 fail:0 skip:8 > > skl-i7k-2total:143 pass:133 dwarn:2 dfail:0 fail:0 skip:8 > > snb-dellxps total:143 pass:129 dwarn:0 dfail:0 fail:0 skip:14 > > snb-x220ttotal:143 pass:129 dwarn:0 dfail:0 fail:1 skip:13 > > > > Results at /archive/results/CI_IGT_test/Patchwork_1244/ > > > > ___ > > 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 https://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] 4.3.3 / skylake / dual graphics: multiple CSR errors at boot from i915 driver
I'm testing a new thinkpad skylake based P70 with dual graphics legolas:~# lspci | grep VGA 00:02.0 VGA compatible controller: Intel Corporation Device 191b (rev 06) 01:00.0 VGA compatible controller: NVIDIA Corporation GM107GLM [Quadro M600M] (rev a2) I'm getting these warnings at boot, although X11 seems to come up ok. Should I worry about them? WARNING: CPU: 4 PID: 101 at drivers/gpu/drm/i915/intel_csr.c:458 assert_csr_loaded+0x2e/0xbe() CSR is not loaded. Modules linked in: rtsx_pci_ms(+) snd_hda_intel rtsx_pci_sdmmc memstick iTCO_wdt iTCO_vendor_support snd_hda_codec snd_hda_core snd_hwdep snd_pcm_oss snd_mixer_oss intel_rapl snd_pcm iosf_mbi thinkpad_acpi x86_pkg_temp_thermal nvram intel_powerclamp coretemp snd_seq_midi snd_seq_midi_event kvm_intel snd_rawmidi kvm snd_seq crct10dif_pclmul crc32_pclmul snd_seq_device ttm snd_timer iwlwifi microcode input_leds snd nvidiafb xhci_pci psmouse pcspkr vgastate serio_raw xhci_hcd cfg80211 i2c_i801 rtsx_pci sg soundcore fb_ddc wmi(+) usbcore rfkill battery ac tpm_crb(+) usb_common tpm fjes evdev shpchp processor sata_sil24 r8169 mii fuse fan raid456 multipath mmc_block mmc_core dm_crypt dm_mod async_raid6_recov async_pq async_xor async_memcpy async_tx blowfish_x86_64 blowfish_common ecb xts crc32c_intel aesni_intel e1000e aes_x86_64 glue_helper lrw gf128mul ablk_helper cryptd ptp pps_core thermal CPU: 4 PID: 101 Comm: kworker/4:1 Not tainted 4.3.3-amd64-i915-volpreempt-20150421 #2 Hardware name: LENOVO 20ERCTO1WW/20ERCTO1WW, BIOS N1DET41W (1.15 ) 12/31/2015 ahci :00:17.0: port does not support device sleep Workqueue: events azx_probe_work [snd_hda_intel] 88087aebbc48 8134150e 88087aebbc90 88087aebbc80 8105aa29 8146f5ee 88087ad5 81cd81d0 300f 88087ad45800 88087aebbce8 Call Trace: [] dump_stack+0x44/0x55 [] warn_slowpath_common+0x99/0xb2 [] ? assert_csr_loaded+0x2e/0xbe [] warn_slowpath_fmt+0x48/0x50 [] ? mutex_lock+0x12/0x2f [] ? mutex_unlock+0x16/0x18 [] assert_csr_loaded+0x2e/0xbe [] skl_set_power_well+0x198/0x7df [] skl_power_well_enable+0x13/0x15 [] intel_display_power_get+0x9e/0xc9 [] i915_audio_component_get_power+0x1e/0x20 [] snd_hdac_display_power+0x91/0xf1 [snd_hda_core] [] azx_probe_continue+0xa9/0x654 [snd_hda_intel] [] ? finish_task_switch+0xb0/0x13b [] azx_probe_work+0x15/0x17 [snd_hda_intel] [] process_one_work+0x15b/0x260 [] worker_thread+0x1f0/0x29d [] ? rescuer_thread+0x281/0x281 [] ? rescuer_thread+0x281/0x281 [] kthread+0xa5/0xad [] ? kthread_parkme+0x24/0x24 [] ret_from_fork+0x3f/0x70 [] ? kthread_parkme+0x24/0x24 ---[ end trace 904c3d98e9b53001 ]--- [ cut here ] WARNING: CPU: 4 PID: 101 at drivers/gpu/drm/i915/intel_csr.c:461 assert_csr_loaded+0x8c/0xbe() CSR SSP Base Not fine Modules linked in: rtsx_pci_ms(+) snd_hda_intel rtsx_pci_sdmmc memstick iTCO_wdt iTCO_vendor_support snd_hda_codec snd_hda_core snd_hwdep snd_pcm_oss snd_mixer_oss intel_rapl snd_pcm iosf_mbi thinkpad_acpi x86_pkg_temp_thermal nvram intel_powerclamp coretemp snd_seq_midi snd_seq_midi_event kvm_intel snd_rawmidi kvm snd_seq crct10dif_pclmul crc32_pclmul snd_seq_device ttm snd_timer iwlwifi microcode input_leds snd nvidiafb xhci_pci psmouse pcspkr vgastate serio_raw xhci_hcd cfg80211 i2c_i801 rtsx_pci sg soundcore fb_ddc wmi(+) usbcore rfkill battery ac tpm_crb(+) usb_common tpm fjes evdev shpchp processor sata_sil24 r8169 mii fuse fan raid456 multipath mmc_block mmc_core dm_crypt dm_mod async_raid6_recov async_pq async_xor async_memcpy async_tx blowfish_x86_64 blowfish_common ecb xts crc32c_intel aesni_intel e1000e aes_x86_64 glue_helper lrw gf128mul ablk_helper cryptd ptp pps_core thermal CPU: 4 PID: 101 Comm: kworker/4:1 Tainted: GW 4.3.3-amd64-i915-volpreempt-20150421 #2 Hardware name: LENOVO 20ERCTO1WW/20ERCTO1WW, BIOS N1DET41W (1.15 ) 12/31/2015 Workqueue: events azx_probe_work [snd_hda_intel] 88087aebbc48 8134150e 88087aebbc90 88087aebbc80 8105aa29 8146f64c 88087ad5 81cd81d0 300f 88087ad45800 88087aebbce8 Call Trace: [] dump_stack+0x44/0x55 [] warn_slowpath_common+0x99/0xb2 [] ? assert_csr_loaded+0x8c/0xbe [] warn_slowpath_fmt+0x48/0x50 [] ? gen9_read32+0x1b4/0x1c2 [] assert_csr_loaded+0x8c/0xbe [] skl_set_power_well+0x198/0x7df [] skl_power_well_enable+0x13/0x15 [] intel_display_power_get+0x9e/0xc9 [] i915_audio_component_get_power+0x1e/0x20 [] snd_hdac_display_power+0x91/0xf1 [snd_hda_core] [] azx_probe_continue+0xa9/0x654 [snd_hda_intel] [] ? finish_task_switch+0xb0/0x13b [] azx_probe_work+0x15/0x17 [snd_hda_intel] [] process_one_work+0x15b/0x260 [] worker_thread+0x1f0/0x29d [] ? rescuer_thread+0x281/0x281 [] ? rescuer_thread+0x281/0x281 [] kthread+0xa5/0xad [] ? kthread_parkme+0x24/0x24 [] ret_from_fork+0x3f/0x70 [] ? kthread_parkme+0x24/0x24 ---[ end trace
[Intel-gfx] [PATCH V5] drm/i915/skl: SKL CDCLK change on modeset tracking VCO
From: Clint TaylorSet cdclk based on the max required pixel clock based on VCO selected. Track boot vco instead of boot cdclk. The vco is now tracked at the atomic level and all CRTCs updated if the required vco is changed. Not tested with eDP v1.4 panels that require 8640 vco due to availability. V1: initial version V2: add vco tracking in intel_dp_compute_config(), rename skl_boot_cdclk. V3: rebase, V2 feedback not possible as encoders are not aware of atomic. V4: track target vco is atomic state. modeset all CRTCs if vco changes V5: rename atomic variable, cleaner if/else logic, use existing vco if encoder does not return a new vco value. check_patch.pl cleanup Signed-off-by: Clint Taylor Cc: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= --- drivers/gpu/drm/i915/i915_drv.h |2 +- drivers/gpu/drm/i915/intel_ddi.c |2 +- drivers/gpu/drm/i915/intel_display.c | 114 +- drivers/gpu/drm/i915/intel_dp.c |6 +- drivers/gpu/drm/i915/intel_drv.h |4 ++ 5 files changed, 111 insertions(+), 17 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 8216665..f65dd1a 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1822,7 +1822,7 @@ struct drm_i915_private { int num_fence_regs; /* 8 on pre-965, 16 otherwise */ unsigned int fsb_freq, mem_freq, is_ddr3; - unsigned int skl_boot_cdclk; + unsigned int skl_vco_freq; unsigned int cdclk_freq, max_cdclk_freq, atomic_cdclk_freq; unsigned int max_dotclk_freq; unsigned int hpll_freq; diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 6d5b09f..285adab 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -2958,7 +2958,7 @@ void intel_ddi_pll_init(struct drm_device *dev) int cdclk_freq; cdclk_freq = dev_priv->display.get_display_clock_speed(dev); - dev_priv->skl_boot_cdclk = cdclk_freq; + dev_priv->skl_vco_freq = skl_cdclk_get_vco(cdclk_freq); if (skl_sanitize_cdclk(dev_priv)) DRM_DEBUG_KMS("Sanitized cdclk programmed by pre-os\n"); if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 9e2273b..c283abd 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5663,7 +5663,7 @@ static unsigned int skl_cdclk_decimal(unsigned int freq) return (freq - 1000) / 500; } -static unsigned int skl_cdclk_get_vco(unsigned int freq) +unsigned int skl_cdclk_get_vco(unsigned int freq) { unsigned int i; @@ -5821,17 +5821,21 @@ void skl_uninit_cdclk(struct drm_i915_private *dev_priv) void skl_init_cdclk(struct drm_i915_private *dev_priv) { - unsigned int required_vco; + unsigned int cdclk; /* DPLL0 not enabled (happens on early BIOS versions) */ if (!(I915_READ(LCPLL1_CTL) & LCPLL_PLL_ENABLE)) { /* enable DPLL0 */ - required_vco = skl_cdclk_get_vco(dev_priv->skl_boot_cdclk); - skl_dpll0_enable(dev_priv, required_vco); + if (dev_priv->skl_vco_freq != 8640) + dev_priv->skl_vco_freq = 8100; + skl_dpll0_enable(dev_priv, dev_priv->skl_vco_freq); + cdclk = ((dev_priv->skl_vco_freq == 8100) ? 337500 : 308570); + } else { + cdclk = dev_priv->cdclk_freq; } - /* set CDCLK to the frequency the BIOS chose */ - skl_set_cdclk(dev_priv, dev_priv->skl_boot_cdclk); + /* set CDCLK to the lowest frequency, Modeset follows */ + skl_set_cdclk(dev_priv, cdclk); /* enable DBUF power */ I915_WRITE(DBUF_CTL, I915_READ(DBUF_CTL) | DBUF_POWER_REQUEST); @@ -5847,7 +5851,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) { uint32_t lcpll1 = I915_READ(LCPLL1_CTL); uint32_t cdctl = I915_READ(CDCLK_CTL); - int freq = dev_priv->skl_boot_cdclk; + int freq = dev_priv->cdclk_freq; /* * check if the pre-os intialized the display @@ -5871,11 +5875,7 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv) /* All well; nothing to sanitize */ return false; sanitize: - /* -* As of now initialize with max cdclk till -* we get dynamic cdclk support -* */ - dev_priv->skl_boot_cdclk = dev_priv->max_cdclk_freq; + skl_init_cdclk(dev_priv); /* we did have to sanitize */ @@ -9845,6 +9845,70 @@ static void broadwell_modeset_commit_cdclk(struct drm_atomic_state *old_state) broadwell_set_cdclk(dev, req_cdclk); } +static int skl_modeset_calc_cdclk(struct