Re: [Intel-gfx] [PATCH 2/7] drm/i915: factor out valleyview_pipestat_irq_handler
On Wed, 05 Feb 2014, Daniel Vetter wrote:
> On Tue, Feb 04, 2014 at 09:35:46PM +0200, Imre Deak wrote:
>> This will be used by other platforms too, so factor it out.
>>
>> The only functional change is the reordeing of gmbus_irq_handler() wrt.
>> the hotplug handling, but since it only schedules a work, it isn't an
>> issue.
>>
>> Signed-off-by: Imre Deak
>> ---
>> drivers/gpu/drm/i915/i915_irq.c | 76
>> +++--
>> 1 file changed, 42 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 137ac65..b5524ea 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -1477,15 +1477,53 @@ static void gen6_rps_irq_handler(struct
>> drm_i915_private *dev_priv, u32 pm_iir)
>> }
>> }
>>
>> +static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
>> +{
>> +drm_i915_private_t *dev_priv = dev->dev_private;
>
> typedefs for structs are frowned upon. I've fixed this while applying.
sed -i -e 's/drm_i915_private_t/struct drm_i915_private/' *.[ch]
plus a little hand-editing and be done with it...?
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Request for feedback - Sprite flip notification support
On 2/5/2014 10:18 PM, Ville Syrjälä wrote: On Wed, Feb 05, 2014 at 09:25:36PM +0530, Vijay Purushothaman wrote: On 2/5/2014 8:43 PM, Ville Syrjälä wrote: On Wed, Feb 05, 2014 at 08:35:11PM +0530, Vijay Purushothaman wrote: Hello, In our current driver implementation we support flip notifications only for primary plane. So, in a full screen video playback scenario where only one sprite plane is active, the user space is forced to rely on primary plane flip notification even though there is no real need for this plane to be active. Ideally we should be able to support flip notifications for any given plane. Switching off the primary plane (when not used) will help in better memory self refresh & decent power savings.. We do have a hack in android product trees which supports flip notifications for one sprite plane. unfortunately this hack in its current form cannot be considered for up streaming... My current thinking is to have an array of unpin_work items to match the number of planes. Is anyone working on this or thought about this scenario in detail? Any pointers / restrictions that needs to considered for a generic implementation of this feature? The plan is to implement the nuclear page flip which will take care of all planes in the same way. Thanks Ville. If the nuclear page flip is part of your bigger atomic mode set framework, is there a way you can split this into smaller sets for merge? Multiple product trees will benefit from the nuclear page flip. I've split things up already somewhat. Some has landed some has not. Currently I have my minimal "atomic update of sprite+primary during setplane" series I need to get in. It shouldn't need major work anymore, just some minor tweaks. But I realized I need to limit this to just pch platforms for now. Making it work reliably on gmch platforms require some extra interrupt related work. The main thing here is that it adds the mechanism to do the update atomically for the entire pipe. After that I need to post the last bits of my watermark update saga. This too will initially be limited to pch platforms only. Obviously watermarks need to updated correctly to avoid underruns when planes get shuffled around. Is there anything that i can help with? Like testing your patches with android user space? There's nothing to test at this point unless you want to test my old branch from year ago or something. What needs to be done: - review the latest atomic framework patches from Rob Clark - expose primary/cursor planes as drm_planes * this could in theory be skipped, but it'll lead to cruft in the API we need to maintain until the end of time. Also I think restructing stuff internally to this direction will be a good idea anyway to make the nuclear flip code neater. This more or less involves collecting the plane state to some plane_config type of structure, and being able to pre-compute that - try to collect the necessary missing bits from my last atomic branch to implement the nuclear flip * the flip helper thing to update an arbitrary collection of planes atomically, maybe could be simplified a bit * mechanism to queue nuclear flips and make them wait for the GPU to finish writing to all the relevant buffers before issuing the actual flips/updates - finally hook up the atomic ioctl to do the nuclear flip * pre-pin all buffers, pre-compute plane configs, pre-compute watermarks, check everything, wait for the GPU, and finally do the update For the atomic modeset side some of that's the same really. There too we also need to pre-compute the plane configs and pre-pin buffers. Most of the rest we already pre-compute via the pipe config. One major thing left out of the pipe config pre-compute currently is PLLs. We compute that stuff way too late still. We also need to massage the modeset code some more to make it capable of modesetting multiple pipes at once. Thanks for the detailed answer. This should solve most of the display issues in the product trees.. considering the amount of work involved this looks more like a long term solution. Would it be okay if we submit a short term solution for sprite flip notifications? This would help us to force a standard approach across multiple android product kernels. We can revert this fix once the atomic mode set / nuclear page flip is ready. Thanks, Vijay ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Add reset subtest to pm_rps
On Wed, Feb 05, 2014 at 01:21:34PM -0600, Jeff McGee wrote: > Gentle ping on this patch set for igt. Both series pulled in, sorry for slacking off a bit. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 00/22] Gen7 batch buffer command parser
On Wed, Feb 05, 2014 at 11:17:25AM -0800, Daniel Vetter wrote: > On Wed, Feb 5, 2014 at 8:00 PM, Volkin, Bradley D > wrote: > > To test/merge, we'd have to change the series to take out the part where > > patch 02/13 sets I915_DISPATCH_SECURE to avoid a BUG_ON() when > > i915.enable_cmd_parser=1. > > But yes, otherwise the parsing works and I think should be sufficient for > > what Chris indicated he wants to test. > > Oh, I didn't spot this but this needs to be moved way back in the > series - we can only set the bit once we have the batchbuffer copy > logic in place. Otherwise there's a security hole open since userspace > is free to frob the batch residing in the ppgtt, which we just can't > prevent. Good point. I'll take it out and we can add it as part of the batch copy work. > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v3 6/7] drm/i915: vlv: fix mapping of pipestat enable to status bits
At least on VLV we can't get at the pipestat status bits by simply right
shifting the corresponding enable bits. The mapping between enable and
status bits for the sprite0,1 flip done and the PSR events don't follow
this rule, so we need to map them separately.
The PSR enable for pipe A is DPFLIPSTAT[22], but I haven't added support
for this, since there is no user of it atm. Until support is added WARN
if someone tries to enable PSR interrupts, or tries to enable the same
(1 << 6) bit on pipe B, which MBZ.
v2:
- inline the status->enable mask mapping (Ville)
- don't check for invalid PSR bit on platforms other than VLV (Ville)
v3:
- fix bogus use of status bits in enable mask (Ville)
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/i915_irq.c | 34 --
drivers/gpu/drm/i915/i915_reg.h | 3 +++
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 37fe12d..68c942f 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -515,13 +515,39 @@ __i915_disable_pipestat(struct drm_i915_private
*dev_priv, enum pipe pipe,
POSTING_READ(reg);
}
+static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32
status_mask)
+{
+ u32 enable_mask = status_mask << 16;
+
+ /*
+* On pipe A we don't support the PSR interrupt yet, on pipe B the
+* same bit MBZ.
+*/
+ if (WARN_ON_ONCE(status_mask & PIPE_A_PSR_STATUS_VLV))
+ return 0;
+
+ enable_mask &= ~(PIPE_FIFO_UNDERRUN_STATUS |
+SPRITE0_FLIP_DONE_INT_EN_VLV |
+SPRITE1_FLIP_DONE_INT_EN_VLV);
+ if (status_mask & SPRITE0_FLIP_DONE_INT_STATUS_VLV)
+ enable_mask |= SPRITE0_FLIP_DONE_INT_EN_VLV;
+ if (status_mask & SPRITE1_FLIP_DONE_INT_STATUS_VLV)
+ enable_mask |= SPRITE1_FLIP_DONE_INT_EN_VLV;
+
+ return enable_mask;
+}
+
void
i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
u32 status_mask)
{
u32 enable_mask;
- enable_mask = status_mask << 16;
+ if (IS_VALLEYVIEW(dev_priv->dev))
+ enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
+ status_mask);
+ else
+ enable_mask = status_mask << 16;
__i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
}
@@ -531,7 +557,11 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv,
enum pipe pipe,
{
u32 enable_mask;
- enable_mask = status_mask << 16;
+ if (IS_VALLEYVIEW(dev_priv->dev))
+ enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
+ status_mask);
+ else
+ enable_mask = status_mask << 16;
__i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
}
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0e69c5c..c5e301e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3240,6 +3240,7 @@
#define PIPE_LEGACY_BLC_EVENT_ENABLE (1UL<<22)
#define PIPE_ODD_FIELD_INTERRUPT_ENABLE (1UL<<21)
#define PIPE_EVEN_FIELD_INTERRUPT_ENABLE (1UL<<20)
+#define PIPE_B_PSR_INTERRUPT_ENABLE_VLV (1UL<<19)
#define PIPE_HOTPLUG_TV_INTERRUPT_ENABLE (1UL<<18) /* pre-965 */
#define PIPE_START_VBLANK_INTERRUPT_ENABLE (1UL<<18) /* 965 or later */
#define PIPE_VBLANK_INTERRUPT_ENABLE (1UL<<17)
@@ -3256,8 +3257,10 @@
#define PIPE_DISPLAY_LINE_COMPARE_STATUS (1UL<<8)
#define PIPE_DPST_EVENT_STATUS (1UL<<7)
#define PIPE_LEGACY_BLC_EVENT_STATUS (1UL<<6)
+#define PIPE_A_PSR_STATUS_VLV(1UL<<6)
#define PIPE_ODD_FIELD_INTERRUPT_STATUS (1UL<<5)
#define PIPE_EVEN_FIELD_INTERRUPT_STATUS (1UL<<4)
+#define PIPE_B_PSR_STATUS_VLV(1UL<<3)
#define PIPE_HOTPLUG_TV_INTERRUPT_STATUS (1UL<<2) /* pre-965 */
#define PIPE_START_VBLANK_INTERRUPT_STATUS (1UL<<2) /* 965 or later */
#define PIPE_VBLANK_INTERRUPT_STATUS (1UL<<1)
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 00/22] Gen7 batch buffer command parser
On Wed, Feb 5, 2014 at 8:00 PM, Volkin, Bradley D wrote: > To test/merge, we'd have to change the series to take out the part where > patch 02/13 sets I915_DISPATCH_SECURE to avoid a BUG_ON() when > i915.enable_cmd_parser=1. > But yes, otherwise the parsing works and I think should be sufficient for > what Chris indicated he wants to test. Oh, I didn't spot this but this needs to be moved way back in the series - we can only set the bit once we have the batchbuffer copy logic in place. Otherwise there's a security hole open since userspace is free to frob the batch residing in the ppgtt, which we just can't prevent. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Add two new subtests to pm_rps
Gentle ping on this patch set for igt. Thanks, Jeff ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Add reset subtest to pm_rps
Gentle ping on this patch set for igt. Thanks, Jeff ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2 6/7] drm/i915: vlv: fix mapping of pipestat enable to status bits
On Wed, Feb 05, 2014 at 08:55:07PM +0200, Imre Deak wrote:
> At least on VLV we can't get at the pipestat status bits by simply right
> shifting the corresponding enable bits. The mapping between enable and
> status bits for the sprite0,1 flip done and the PSR events don't follow
> this rule, so we need to map them separately.
>
> The PSR enable for pipe A is DPFLIPSTAT[22], but I haven't added support
> for this, since there is no user of it atm. Until support is added WARN
> if someone tries to enable PSR interrupts, or tries to enable the same
> (1 << 6) bit on pipe B, which MBZ.
>
> v2:
> - inline the status->enable mask mapping (Ville)
> - don't check for invalid PSR bit on platforms other than VLV (Ville)
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_irq.c | 34 --
> drivers/gpu/drm/i915/i915_reg.h | 3 +++
> 2 files changed, 35 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 37fe12d..6166dd9 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -515,13 +515,39 @@ __i915_disable_pipestat(struct drm_i915_private
> *dev_priv, enum pipe pipe,
> POSTING_READ(reg);
> }
>
> +static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32
> status_mask)
> +{
> + u32 enable_mask = status_mask << 16;
> +
> + /*
> + * On pipe A we don't support the PSR interrupt yet, on pipe B the
> + * same bit MBZ.
> + */
> + if (WARN_ON_ONCE(status_mask & PIPE_A_PSR_STATUS_VLV))
> + return 0;
> +
> + enable_mask &= ~(PIPE_FIFO_UNDERRUN_STATUS |
> + SPRITE0_FLIP_DONE_INT_STATUS_VLV |
> + SPRITE1_FLIP_DONE_INT_STATUS_VLV);
Those two should have been ENABLE bits.
The rest looks fine. So fix that, and you can add (for the entire series):
Reviewed-by: Ville Syrjälä
> + if (status_mask & SPRITE0_FLIP_DONE_INT_STATUS_VLV)
> + enable_mask |= SPRITE0_FLIP_DONE_INT_EN_VLV;
> + if (status_mask & SPRITE1_FLIP_DONE_INT_STATUS_VLV)
> + enable_mask |= SPRITE1_FLIP_DONE_INT_EN_VLV;
> +
> + return enable_mask;
> +}
> +
> void
> i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
>u32 status_mask)
> {
> u32 enable_mask;
>
> - enable_mask = status_mask << 16;
> + if (IS_VALLEYVIEW(dev_priv->dev))
> + enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
> +status_mask);
> + else
> + enable_mask = status_mask << 16;
> __i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
> }
>
> @@ -531,7 +557,11 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv,
> enum pipe pipe,
> {
> u32 enable_mask;
>
> - enable_mask = status_mask << 16;
> + if (IS_VALLEYVIEW(dev_priv->dev))
> + enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
> +status_mask);
> + else
> + enable_mask = status_mask << 16;
> __i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
> }
>
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0e69c5c..c5e301e 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3240,6 +3240,7 @@
> #define PIPE_LEGACY_BLC_EVENT_ENABLE (1UL<<22)
> #define PIPE_ODD_FIELD_INTERRUPT_ENABLE(1UL<<21)
> #define PIPE_EVEN_FIELD_INTERRUPT_ENABLE (1UL<<20)
> +#define PIPE_B_PSR_INTERRUPT_ENABLE_VLV(1UL<<19)
> #define PIPE_HOTPLUG_TV_INTERRUPT_ENABLE (1UL<<18) /* pre-965 */
> #define PIPE_START_VBLANK_INTERRUPT_ENABLE (1UL<<18) /* 965 or later */
> #define PIPE_VBLANK_INTERRUPT_ENABLE (1UL<<17)
> @@ -3256,8 +3257,10 @@
> #define PIPE_DISPLAY_LINE_COMPARE_STATUS (1UL<<8)
> #define PIPE_DPST_EVENT_STATUS (1UL<<7)
> #define PIPE_LEGACY_BLC_EVENT_STATUS (1UL<<6)
> +#define PIPE_A_PSR_STATUS_VLV (1UL<<6)
> #define PIPE_ODD_FIELD_INTERRUPT_STATUS(1UL<<5)
> #define PIPE_EVEN_FIELD_INTERRUPT_STATUS (1UL<<4)
> +#define PIPE_B_PSR_STATUS_VLV (1UL<<3)
> #define PIPE_HOTPLUG_TV_INTERRUPT_STATUS (1UL<<2) /* pre-965 */
> #define PIPE_START_VBLANK_INTERRUPT_STATUS (1UL<<2) /* 965 or later */
> #define PIPE_VBLANK_INTERRUPT_STATUS (1UL<<1)
> --
> 1.8.4
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] tests/kms_psr_sink_crc: Create test to test PSR by checking panel CRC.
v2: Wait psr enable with timeout and more subtest added.
v3: Add wait for v_blank leeting test more reliable and preparing to
add Baytrail per-pipe tests.
v4: Call busy_ioctl on mmap_gtt to match the real usage and remove the need
of inactivate on set_domain, what was semantically wrong.
v5: Adding more test cases to cover mmap_gtt with and without followed by
busy ioctl and also without busy and waiting 10 seconds between
set_domain and actual write.
Cc: Chris Wilson
Signed-off-by: Rodrigo Vivi
---
tests/Android.mk | 1 +
tests/Makefile.sources | 1 +
tests/kms_psr_sink_crc.c | 535 +++
3 files changed, 537 insertions(+)
create mode 100644 tests/kms_psr_sink_crc.c
diff --git a/tests/Android.mk b/tests/Android.mk
index fa9e9d7..81906be 100644
--- a/tests/Android.mk
+++ b/tests/Android.mk
@@ -95,6 +95,7 @@ skip_tests_list := \
kms_cursor_crc \
kms_flip \
kms_pipe_crc_basic \
+kms_psr_sink_crc \
kms_fbc_crc \
kms_render \
kms_setmode \
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 5fe68ae..fdb1e41 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -58,6 +58,7 @@ TESTS_progs_M = \
kms_fbc_crc \
kms_flip \
kms_pipe_crc_basic \
+ kms_psr_sink_crc \
kms_render \
kms_setmode \
kms_sink_crc_basic \
diff --git a/tests/kms_psr_sink_crc.c b/tests/kms_psr_sink_crc.c
new file mode 100644
index 000..782c785
--- /dev/null
+++ b/tests/kms_psr_sink_crc.c
@@ -0,0 +1,535 @@
+/*
+ * Copyright © 2013 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.
+ *
+ */
+
+#include
+#include
+#include
+#include
+
+#include "drm_fourcc.h"
+
+#include "drmtest.h"
+#include "igt_debugfs.h"
+#include "igt_kms.h"
+#include "rendercopy.h"
+
+enum test_mode {
+ TEST_PAGE_FLIP,
+ TEST_MMAP_CPU,
+ TEST_MMAP_GTT,
+ TEST_MMAP_GTT_NO_BUSY,
+ TEST_MMAP_GTT_WAITING_NO_BUSY,
+ TEST_BLT,
+ TEST_RENDER,
+ TEST_CONTEXT,
+ TEST_PAGE_FLIP_AND_MMAP_CPU,
+ TEST_PAGE_FLIP_AND_MMAP_GTT,
+ TEST_PAGE_FLIP_AND_BLT,
+ TEST_PAGE_FLIP_AND_RENDER,
+ TEST_PAGE_FLIP_AND_CONTEXT,
+ TEST_CURSOR_MOVE,
+};
+
+typedef struct {
+ struct kmstest_connector_config config;
+ drmModeModeInfo mode;
+ struct kmstest_fb fb[2];
+} connector_t;
+
+typedef struct {
+ int drm_fd;
+ igt_debugfs_t debugfs;
+ drmModeRes *resources;
+ drm_intel_bufmgr *bufmgr;
+ drm_intel_context *ctx[2];
+ uint32_t devid;
+ uint32_t handle[2];
+ uint32_t crtc_id;
+ uint32_t crtc_idx;
+ uint32_t fb_id[3];
+} data_t;
+
+static const char *test_mode_str(enum test_mode mode)
+{
+ static const char * const test_modes[] = {
+ [TEST_PAGE_FLIP] = "page_flip",
+ [TEST_MMAP_CPU] = "mmap_cpu",
+ [TEST_MMAP_GTT] = "mmap_gtt",
+ [TEST_MMAP_GTT_NO_BUSY] = "mmap_gtt_no_busy",
+ [TEST_MMAP_GTT_WAITING_NO_BUSY] = "mmap_gtt_waiting_no_busy",
+ [TEST_BLT] = "blt",
+ [TEST_RENDER] = "render",
+ [TEST_CONTEXT] = "context",
+ [TEST_PAGE_FLIP_AND_MMAP_CPU] = "page_flip_and_mmap_cpu",
+ [TEST_PAGE_FLIP_AND_MMAP_GTT] = "page_flip_and_mmap_gtt",
+ [TEST_PAGE_FLIP_AND_BLT] = "page_flip_and_blt",
+ [TEST_PAGE_FLIP_AND_RENDER] = "page_flip_and_render",
+ [TEST_PAGE_FLIP_AND_CONTEXT] = "page_flip_and_context",
+ [TEST_CURSOR_MOVE] = "cursor_move",
+ };
+
+ return test_modes[mode];
+}
+
+static uint32_t create_fb(data_t *data,
+ int w, int h,
+ double r, double g, double b,
+ struct kmstest_fb *fb)
+{
+ uint
[Intel-gfx] [PATCH 1/2] drm/i915: Update PSR on resume.
Since now on update is also been called out of set_base let's use
a mutex to protec psr state changes.
Signed-off-by: Rodrigo Vivi
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_suspend.c | 4
drivers/gpu/drm/i915/intel_dp.c | 17 ++---
3 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7c53d4d..21470be 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -747,6 +747,7 @@ struct i915_psr {
bool sink_support;
bool source_ok;
bool setup_done;
+ struct mutex lock;
};
enum intel_pch {
diff --git a/drivers/gpu/drm/i915/i915_suspend.c
b/drivers/gpu/drm/i915/i915_suspend.c
index 56785e8..ffcba21 100644
--- a/drivers/gpu/drm/i915/i915_suspend.c
+++ b/drivers/gpu/drm/i915/i915_suspend.c
@@ -288,6 +288,10 @@ static void i915_restore_display(struct drm_device *dev)
I915_WRITE(PP_CONTROL, dev_priv->regfile.savePP_CONTROL);
}
+ /* Force a full PSR setup on resume */
+ dev_priv->psr.setup_done = false;
+ intel_edp_psr_update(dev);
+
/* only restore FBC info on the platform that supports FBC*/
intel_disable_fbc(dev);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 30d4350..80054bb 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1633,6 +1633,8 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
if (dev_priv->psr.setup_done)
return;
+ mutex_init(&dev_priv->psr.lock);
+
/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
memset(&psr_vsc, 0, sizeof(psr_vsc));
psr_vsc.sdp_header.HB0 = 0;
@@ -1777,9 +1779,6 @@ static void intel_edp_psr_do_enable(struct intel_dp
*intel_dp)
intel_edp_is_psr_enabled(dev))
return;
- /* Setup PSR once */
- intel_edp_psr_setup(intel_dp);
-
/* Enable PSR on the panel */
intel_edp_psr_enable_sink(intel_dp);
@@ -1790,10 +1789,16 @@ static void intel_edp_psr_do_enable(struct intel_dp
*intel_dp)
void intel_edp_psr_enable(struct intel_dp *intel_dp)
{
struct drm_device *dev = intel_dp_to_dev(intel_dp);
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ /* Setup PSR once */
+ intel_edp_psr_setup(intel_dp);
+
+ mutex_lock(&dev_priv->psr.lock);
if (intel_edp_psr_match_conditions(intel_dp) &&
!intel_edp_is_psr_enabled(dev))
intel_edp_psr_do_enable(intel_dp);
+ mutex_unlock(&dev_priv->psr.lock);
}
void intel_edp_psr_disable(struct intel_dp *intel_dp)
@@ -1815,9 +1820,13 @@ void intel_edp_psr_disable(struct intel_dp *intel_dp)
void intel_edp_psr_update(struct drm_device *dev)
{
+ struct drm_i915_private *dev_priv = dev->dev_private;
struct intel_encoder *encoder;
struct intel_dp *intel_dp = NULL;
+ if (!dev_priv->psr.setup_done)
+ return;
+
list_for_each_entry(encoder, &dev->mode_config.encoder_list, base.head)
if (encoder->type == INTEL_OUTPUT_EDP) {
intel_dp = enc_to_intel_dp(&encoder->base);
@@ -1825,11 +1834,13 @@ void intel_edp_psr_update(struct drm_device *dev)
if (!is_edp_psr(dev))
return;
+ mutex_lock(&dev_priv->psr.lock);
if (!intel_edp_psr_match_conditions(intel_dp))
intel_edp_psr_disable(intel_dp);
else
if (!intel_edp_is_psr_enabled(dev))
intel_edp_psr_do_enable(intel_dp);
+ mutex_unlock(&dev_priv->psr.lock);
}
}
--
1.8.1.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Add Baytrail PSR Support.
This patch adds PSR Support to Baytrail.
Baytrail cannot easily detect screen updates and force PSR exit.
So we inactivate it on {busy_ioctl, set_domain, sw_finish and mark_busy}
and update to enable it back on next display mark_idle.
v2: Also inactivate PSR on cursor update.
v3: Inactivate PSR on mark_busy, dset_domain and sw_finish_ioctl, and
early on page flip besides avoid initializing inactive/active flag
more than once.
v4: Fix identation issues.
v5: Rebase and add Baytrail per pipe support although leaving PIPE_B
support disabled by for now since it isn't working properly yet.
v6: Removing forgotten comment and useless clkgating definition.
v7: Remove inactivate from set_domain. Chris warned this was semanticaly
wrong.
v8: Accept Ville's suggestions: Use register's names matching spec and
warn if transition took longer than it should.
v9: New version with delayed work to get PSR back. Disabling it on
set_domain but not rescheduing it back until next finish_page_flip.
Cc: Chris Wilson
Cc: Ville Syrjälä
Signed-off-by: Rodrigo Vivi
---
drivers/gpu/drm/i915/i915_debugfs.c | 36 -
drivers/gpu/drm/i915/i915_drv.h | 5 +-
drivers/gpu/drm/i915/i915_gem.c | 12 ++
drivers/gpu/drm/i915/i915_reg.h | 37 +
drivers/gpu/drm/i915/i915_suspend.c | 2 +-
drivers/gpu/drm/i915/intel_display.c | 18 ++-
drivers/gpu/drm/i915/intel_dp.c | 256 ++-
drivers/gpu/drm/i915/intel_drv.h | 1 +
8 files changed, 323 insertions(+), 44 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index bc8707f..2949c48 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1900,6 +1900,8 @@ static int i915_edp_psr_status(struct seq_file *m, void
*data)
struct drm_device *dev = node->minor->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
u32 psrperf = 0;
+ u32 statA = 0;
+ u32 statB = 0;
bool enabled = false;
intel_runtime_pm_get(dev_priv);
@@ -1907,14 +1909,38 @@ static int i915_edp_psr_status(struct seq_file *m, void
*data)
seq_printf(m, "Sink_Support: %s\n", yesno(dev_priv->psr.sink_support));
seq_printf(m, "Source_OK: %s\n", yesno(dev_priv->psr.source_ok));
- enabled = HAS_PSR(dev) &&
- I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE;
- seq_printf(m, "Enabled: %s\n", yesno(enabled));
+ if (HAS_PSR(dev)) {
+ if (IS_VALLEYVIEW(dev)) {
+ statA = I915_READ(VLV_PSRSTAT(PIPE_A)) &
+ VLV_EDP_PSR_CURR_STATE_MASK;
+ statB = I915_READ(VLV_PSRSTAT(PIPE_B)) &
+ VLV_EDP_PSR_CURR_STATE_MASK;
+ enabled = ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) ||
+ (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE) ||
+ (statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) ||
+ (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE));
+ } else
+ enabled = I915_READ(EDP_PSR_CTL(dev)) & EDP_PSR_ENABLE;
+ }
+ seq_printf(m, "Enabled: %s", yesno(enabled));
- if (HAS_PSR(dev))
+ if (IS_VALLEYVIEW(dev)) {
+ if ((statA == VLV_EDP_PSR_ACTIVE_NORFB_UP) ||
+ (statA == VLV_EDP_PSR_ACTIVE_SF_UPDATE))
+ seq_puts(m, " pipe A");
+ if ((statB == VLV_EDP_PSR_ACTIVE_NORFB_UP) ||
+ (statB == VLV_EDP_PSR_ACTIVE_SF_UPDATE))
+ seq_puts(m, " pipe B");
+ }
+
+ seq_puts(m, "\n");
+
+ /* VLV PSR has no kind of performance counter */
+ if (HAS_PSR(dev) && !IS_VALLEYVIEW(dev)) {
psrperf = I915_READ(EDP_PSR_PERF_CNT(dev)) &
EDP_PSR_PERF_CNT_MASK;
- seq_printf(m, "Performance_Counter: %u\n", psrperf);
+ seq_printf(m, "Performance_Counter: %u\n", psrperf);
+ }
intel_runtime_pm_put(dev_priv);
return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 21470be..87c346a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -747,7 +747,9 @@ struct i915_psr {
bool sink_support;
bool source_ok;
bool setup_done;
+ bool active;
struct mutex lock;
+ struct delayed_work work;
};
enum intel_pch {
@@ -1867,7 +1869,8 @@ struct drm_i915_file_private {
#define HAS_DDI(dev) (INTEL_INFO(dev)->has_ddi)
#define HAS_FPGA_DBG_UNCLAIMED(dev)(INTEL_INFO(dev)->has_fpga_dbg)
-#define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev))
+#define HAS_PSR(dev) (IS_HASWELL(dev) || IS_BROADWELL(dev) || \
+IS_VALLEYVIEW(dev))
#define HAS_PC8(dev) (IS_HASWELL(dev)) /* XX
Re: [Intel-gfx] [RFC 00/22] Gen7 batch buffer command parser
On Wed, Feb 05, 2014 at 10:30:00AM -0800, Daniel Vetter wrote: > On Wed, Feb 5, 2014 at 7:18 PM, Volkin, Bradley D > wrote: > > On Wed, Feb 05, 2014 at 02:28:29AM -0800, Chris Wilson wrote: > >> On Tue, Nov 26, 2013 at 08:51:17AM -0800, bradley.d.vol...@intel.com wrote: > >> > From: Brad Volkin > >> > > >> > Certain OpenGL features (e.g. transform feedback, performance monitoring) > >> > require userspace code to submit batches containing commands such as > >> > MI_LOAD_REGISTER_IMM to access various registers. Unfortunately, some > >> > generations of the hardware will noop these commands in "unsecure" > >> > batches > >> > (which includes all userspace batches submitted via i915) even though the > >> > commands may be safe and represent the intended programming model of the > >> > device. > >> > > >> > This series introduces a software command parser similar in operation to > >> > the > >> > command parsing done in hardware for unsecure batches. However, the > >> > software > >> > parser allows some operations that would be noop'd by hardware, if the > >> > parser > >> > determines the operation is safe, and submits the batch as "secure" to > >> > prevent > >> > hardware parsing. Currently the series implements this on IVB and HSW. > >> > >> Just one more question... Do you have a branch for people to test? > > > > Not at the moment. And as mentioned in the v2 cover letter, it's actually > > not > > particularly testable (or mergeable for that matter) right now because of a > > regression in secure dispatch on nightly. > > The command parser itself should still work, even with the regression > in -nightly. The copying and secure dispatch are obviously fail atm. > That still leaves regression testing of current userspace and > micro-optimizing the checker itself as possible things to do. Otoh not > sure what exactly Chris wanted to test. To test/merge, we'd have to change the series to take out the part where patch 02/13 sets I915_DISPATCH_SECURE to avoid a BUG_ON() when i915.enable_cmd_parser=1. But yes, otherwise the parsing works and I think should be sufficient for what Chris indicated he wants to test. - Brad > -Daniel > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/13] drm/i915: Enable PPGTT command parser checks
[snip]
On Wed, Feb 05, 2014 at 07:37:51AM -0800, Jani Nikula wrote:
> On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote:
> > int i915_needs_cmd_parser(struct intel_ring_buffer *ring)
> > {
> > + drm_i915_private_t *dev_priv =
> > + (drm_i915_private_t *)ring->dev->dev_private;
> > +
> > /* No command tables indicates a platform without parsing */
> > if (!ring->cmd_tables)
> > return 0;
> >
> > + /*
> > +* XXX: VLV is Gen7 and therefore has cmd_tables, but has PPGTT
> > +* disabled. That will cause all of the parser's PPGTT checks to
> > +* fail. For now, disable parsing when PPGTT is off.
> > +*/
> > + if(!dev_priv->mm.aliasing_ppgtt)
> ^ missing space.
Oops
>
> > + return 0;
> > +
>
> Hmm, shouldn't this belong to some other patch, much earlier in the
> series? Like patch 2 or 3?
Not necessarily. It's only because we've added the PPGTT checks without
somehow making them conditional on aliasing_ppgtt==true that we have a problem,
and that only happens with this patch. The parser works, though is less useful,
on !aliasing_ppgtt platforms up to this point.
Chris suggested that we just fix it up so that the PPGTT checks are conditional
on PPGTT actually enabled, so I'm going to look at that.
- Brad
>
> > return i915.enable_cmd_parser;
> > }
> >
> > @@ -675,6 +782,16 @@ int i915_parse_cmds(struct intel_ring_buffer *ring,
> > u32 dword = cmd[desc->bits[i].offset] &
> > desc->bits[i].mask;
> >
> > + if (desc->bits[i].condition_mask != 0) {
> > + u32 offset =
> > + desc->bits[i].condition_offset;
> > + u32 condition = cmd[offset] &
> > + desc->bits[i].condition_mask;
> > +
> > + if (condition == 0)
> > + continue;
> > + }
> > +
> > if (dword != desc->bits[i].expected) {
> > DRM_DEBUG_DRIVER("CMD: Rejected command
> > 0x%08X for bitmask 0x%08X (exp=0x%08X act=0x%08X) (ring=%d)\n",
> > *cmd,
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 8aed80f..2d1d2ef 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1829,11 +1829,17 @@ struct drm_i915_cmd_descriptor {
> > * compared against an expected value. If the command does not match
> > * the expected value, the parser rejects it. Only valid if flags has
> > * the CMD_DESC_BITMASK bit set.
> > +*
> > +* If the check specifies a non-zero condition_mask then the parser
> > +* only performs the check when the bits specified by condition_mask
> > +* are non-zero.
> > */
> > struct {
> > u32 offset;
> > u32 mask;
> > u32 expected;
> > + u32 condition_offset;
> > + u32 condition_mask;
> > } bits[MAX_CMD_DESC_BITMASKS];
> > /** Number of valid entries in the bits array */
> > int bits_count;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index c2e4898..ff263f4 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -179,6 +179,8 @@
> > * Memory interface instructions used by the kernel
> > */
> > #define MI_INSTR(opcode, flags) (((opcode) << 23) | (flags))
> > +/* Many MI commands use bit 22 of the header dword for GGTT vs PPGTT */
> > +#define MI_GLOBAL_GTT(1<<22)
> >
> > #define MI_NOOPMI_INSTR(0, 0)
> > #define MI_USER_INTERRUPT MI_INSTR(0x02, 0)
> > @@ -258,6 +260,7 @@
> > #define MI_FLUSH_DW_STORE_INDEX (1<<21)
> > #define MI_INVALIDATE_TLB(1<<18)
> > #define MI_FLUSH_DW_OP_STOREDW (1<<14)
> > +#define MI_FLUSH_DW_OP_MASK (3<<14)
> > #define MI_FLUSH_DW_NOTIFY (1<<8)
> > #define MI_INVALIDATE_BSD(1<<7)
> > #define MI_FLUSH_DW_USE_GTT (1<<2)
> > @@ -324,6 +327,7 @@
> > #define PIPE_CONTROL_CS_STALL(1<<20)
> > #define PIPE_CONTROL_TLB_INVALIDATE (1<<18)
> > #define PIPE_CONTROL_QW_WRITE(1<<14)
> > +#define PIPE_CONTROL_POST_SYNC_OP_MASK(3<<14)
> > #define PIPE_CONTROL_DEPTH_STALL (1<<13)
> > #define PIPE_CONTROL_WRITE_FLUSH (1<<12)
> > #define PIPE_CONTROL_RENDER_TARGET_CACHE_FLUSH (1<<12) /* gen6+ */
> > @@ -352,6 +356,8 @@
> > #define MI_URB_CLEARMI_INSTR(0x19, 0)
> > #define MI_UPDATE_GTT MI_INSTR(0x23, 0)
> > #define MI_CLFLUSH
[Intel-gfx] [PATCH v2 6/7] drm/i915: vlv: fix mapping of pipestat enable to status bits
At least on VLV we can't get at the pipestat status bits by simply right
shifting the corresponding enable bits. The mapping between enable and
status bits for the sprite0,1 flip done and the PSR events don't follow
this rule, so we need to map them separately.
The PSR enable for pipe A is DPFLIPSTAT[22], but I haven't added support
for this, since there is no user of it atm. Until support is added WARN
if someone tries to enable PSR interrupts, or tries to enable the same
(1 << 6) bit on pipe B, which MBZ.
v2:
- inline the status->enable mask mapping (Ville)
- don't check for invalid PSR bit on platforms other than VLV (Ville)
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/i915_irq.c | 34 --
drivers/gpu/drm/i915/i915_reg.h | 3 +++
2 files changed, 35 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 37fe12d..6166dd9 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -515,13 +515,39 @@ __i915_disable_pipestat(struct drm_i915_private
*dev_priv, enum pipe pipe,
POSTING_READ(reg);
}
+static u32 vlv_get_pipestat_enable_mask(struct drm_device *dev, u32
status_mask)
+{
+ u32 enable_mask = status_mask << 16;
+
+ /*
+* On pipe A we don't support the PSR interrupt yet, on pipe B the
+* same bit MBZ.
+*/
+ if (WARN_ON_ONCE(status_mask & PIPE_A_PSR_STATUS_VLV))
+ return 0;
+
+ enable_mask &= ~(PIPE_FIFO_UNDERRUN_STATUS |
+SPRITE0_FLIP_DONE_INT_STATUS_VLV |
+SPRITE1_FLIP_DONE_INT_STATUS_VLV);
+ if (status_mask & SPRITE0_FLIP_DONE_INT_STATUS_VLV)
+ enable_mask |= SPRITE0_FLIP_DONE_INT_EN_VLV;
+ if (status_mask & SPRITE1_FLIP_DONE_INT_STATUS_VLV)
+ enable_mask |= SPRITE1_FLIP_DONE_INT_EN_VLV;
+
+ return enable_mask;
+}
+
void
i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
u32 status_mask)
{
u32 enable_mask;
- enable_mask = status_mask << 16;
+ if (IS_VALLEYVIEW(dev_priv->dev))
+ enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
+ status_mask);
+ else
+ enable_mask = status_mask << 16;
__i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
}
@@ -531,7 +557,11 @@ i915_disable_pipestat(struct drm_i915_private *dev_priv,
enum pipe pipe,
{
u32 enable_mask;
- enable_mask = status_mask << 16;
+ if (IS_VALLEYVIEW(dev_priv->dev))
+ enable_mask = vlv_get_pipestat_enable_mask(dev_priv->dev,
+ status_mask);
+ else
+ enable_mask = status_mask << 16;
__i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
}
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0e69c5c..c5e301e 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3240,6 +3240,7 @@
#define PIPE_LEGACY_BLC_EVENT_ENABLE (1UL<<22)
#define PIPE_ODD_FIELD_INTERRUPT_ENABLE (1UL<<21)
#define PIPE_EVEN_FIELD_INTERRUPT_ENABLE (1UL<<20)
+#define PIPE_B_PSR_INTERRUPT_ENABLE_VLV (1UL<<19)
#define PIPE_HOTPLUG_TV_INTERRUPT_ENABLE (1UL<<18) /* pre-965 */
#define PIPE_START_VBLANK_INTERRUPT_ENABLE (1UL<<18) /* 965 or later */
#define PIPE_VBLANK_INTERRUPT_ENABLE (1UL<<17)
@@ -3256,8 +3257,10 @@
#define PIPE_DISPLAY_LINE_COMPARE_STATUS (1UL<<8)
#define PIPE_DPST_EVENT_STATUS (1UL<<7)
#define PIPE_LEGACY_BLC_EVENT_STATUS (1UL<<6)
+#define PIPE_A_PSR_STATUS_VLV(1UL<<6)
#define PIPE_ODD_FIELD_INTERRUPT_STATUS (1UL<<5)
#define PIPE_EVEN_FIELD_INTERRUPT_STATUS (1UL<<4)
+#define PIPE_B_PSR_STATUS_VLV(1UL<<3)
#define PIPE_HOTPLUG_TV_INTERRUPT_STATUS (1UL<<2) /* pre-965 */
#define PIPE_START_VBLANK_INTERRUPT_STATUS (1UL<<2) /* 965 or later */
#define PIPE_VBLANK_INTERRUPT_STATUS (1UL<<1)
--
1.8.4
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2 7/7] drm/i915: vlv: handle only enabled pipestat interrupt events
Atm we call the handlers for pending pipestat interrupt events even if
they aren't explicitly enabled by i915_enable_pipestat(). This isn't an
issue for events other than the vblank start event, since those are
always enabled anyways. Otoh, we enable the vblank start event
on-demand, so we'll end up calling the vblank handler at times when they
are disabled.
I haven't checked if this causes any real problem, but for consistency
and to remove some overhead we should still fix this by clearing /
handling only the enabled interrupt events. Also this is a dependency
for the upcoming VLV power domain patchset where we need to disable all
the pipestat interrupts whenever the display power well is off.
v2: make sure the underrun status is always ignored if its reporting is
disabled (Ville)
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_irq.c | 35 ---
drivers/gpu/drm/i915/i915_reg.h | 4
3 files changed, 37 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c8225ac..2751490a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1427,6 +1427,7 @@ typedef struct drm_i915_private {
};
u32 gt_irq_mask;
u32 pm_irq_mask;
+ u32 pipestat_irq_mask[I915_MAX_PIPES];
struct work_struct hotplug_work;
bool enable_hotplug_processing;
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6166dd9..b456356 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -419,6 +419,16 @@ done:
return ret;
}
+static bool __cpu_fifo_underrun_reporting_enabled(struct drm_device *dev,
+ enum pipe pipe)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
+ struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+
+ return !intel_crtc->cpu_fifo_underrun_disabled;
+}
+
/**
* intel_set_pch_fifo_underrun_reporting - enable/disable FIFO underrun
messages
* @dev: drm device
@@ -488,6 +498,8 @@ __i915_enable_pipestat(struct drm_i915_private *dev_priv,
enum pipe pipe,
if ((pipestat & enable_mask) == enable_mask)
return;
+ dev_priv->pipestat_irq_mask[pipe] |= status_mask;
+
/* Enable the interrupt, clear any pending status */
pipestat |= enable_mask | status_mask;
I915_WRITE(reg, pipestat);
@@ -510,6 +522,8 @@ __i915_disable_pipestat(struct drm_i915_private *dev_priv,
enum pipe pipe,
if ((pipestat & enable_mask) == 0)
return;
+ dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
+
pipestat &= ~enable_mask;
I915_WRITE(reg, pipestat);
POSTING_READ(reg);
@@ -1540,18 +1554,33 @@ static void gen6_rps_irq_handler(struct
drm_i915_private *dev_priv, u32 pm_iir)
static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
{
drm_i915_private_t *dev_priv = dev->dev_private;
- u32 pipe_stats[I915_MAX_PIPES];
+ u32 pipe_stats[I915_MAX_PIPES] = { };
int pipe;
spin_lock(&dev_priv->irq_lock);
for_each_pipe(pipe) {
- int reg = PIPESTAT(pipe);
+ int reg;
+ u32 mask;
+
+ if (!dev_priv->pipestat_irq_mask[pipe] &&
+ !__cpu_fifo_underrun_reporting_enabled(dev, pipe))
+ continue;
+
+ reg = PIPESTAT(pipe);
pipe_stats[pipe] = I915_READ(reg);
/*
* Clear the PIPE*STAT regs before the IIR
*/
- if (pipe_stats[pipe] & 0x8000)
+ mask = PIPESTAT_INT_ENABLE_MASK;
+ if (__cpu_fifo_underrun_reporting_enabled(dev, pipe))
+ mask |= PIPE_FIFO_UNDERRUN_STATUS;
+ if (iir & I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe))
+ mask |= dev_priv->pipestat_irq_mask[pipe];
+ pipe_stats[pipe] &= mask;
+
+ if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
+ PIPESTAT_INT_STATUS_MASK))
I915_WRITE(reg, pipe_stats[pipe]);
}
spin_unlock(&dev_priv->irq_lock);
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c5e301e..81b4edb 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -997,6 +997,10 @@
#define I915_DISPLAY_PIPE_A_EVENT_INTERRUPT(1<<6)
#define I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT (1<<5)
#define I915_DISPLAY_PIPE_B_EVENT_INTERRUPT(1<<4)
+#define I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe)
\
+ ((pipe) == PIPE_A ? I915_DISPLAY_PIPE_A_EVENT_INTERRUPT : \
+
[Intel-gfx] [PATCH v2 5/7] drm/i915: pass status instead of enable flags to i915_enable_pipestat
There isn't any PSR interrupt enable bit for pipe A, so we couldn't
enable it through the current API. Passing the corresponding status bits
solves this and also makes the mapping between enable and status bits
simpler on VLV (addressed in an upcoming patch).
Except of checking for invalid status bit arguments, no functional
change.
v2: split out the low level parts of i915_enable_pipestat accepting
separate enabled and status masks, to make the non-standard mapping
between those masks stand out more (added in the next patch)
(Jesse,Daniel)
Signed-off-by: Imre Deak
---
drivers/gpu/drm/i915/i915_drv.h | 6 ++-
drivers/gpu/drm/i915/i915_irq.c | 82 -
drivers/gpu/drm/i915/i915_reg.h | 3 ++
drivers/gpu/drm/i915/intel_tv.c | 8 ++--
4 files changed, 67 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index e908c99..c8225ac 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1996,10 +1996,12 @@ extern void intel_uncore_check_errors(struct drm_device
*dev);
extern void intel_uncore_fini(struct drm_device *dev);
void
-i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask);
+i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
+u32 status_mask);
void
-i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask);
+i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
+ u32 status_mask);
/* i915_gem.c */
int i915_gem_init_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 6135976..37fe12d 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -473,38 +473,68 @@ done:
void
-i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask)
+__i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
+ u32 enable_mask, u32 status_mask)
{
u32 reg = PIPESTAT(pipe);
- u32 pipestat = I915_READ(reg) & 0x7fff;
+ u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
assert_spin_locked(&dev_priv->irq_lock);
- if ((pipestat & mask) == mask)
+ if (WARN_ON_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
+status_mask & ~PIPESTAT_INT_STATUS_MASK))
+ return;
+
+ if ((pipestat & enable_mask) == enable_mask)
return;
/* Enable the interrupt, clear any pending status */
- pipestat |= mask | (mask >> 16);
+ pipestat |= enable_mask | status_mask;
I915_WRITE(reg, pipestat);
POSTING_READ(reg);
}
void
-i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask)
+__i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
+ u32 enable_mask, u32 status_mask)
{
u32 reg = PIPESTAT(pipe);
- u32 pipestat = I915_READ(reg) & 0x7fff;
+ u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
assert_spin_locked(&dev_priv->irq_lock);
- if ((pipestat & mask) == 0)
+ if (WARN_ON_ONCE(enable_mask & ~PIPESTAT_INT_ENABLE_MASK ||
+status_mask & ~PIPESTAT_INT_STATUS_MASK))
return;
- pipestat &= ~mask;
+ if ((pipestat & enable_mask) == 0)
+ return;
+
+ pipestat &= ~enable_mask;
I915_WRITE(reg, pipestat);
POSTING_READ(reg);
}
+void
+i915_enable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
+u32 status_mask)
+{
+ u32 enable_mask;
+
+ enable_mask = status_mask << 16;
+ __i915_enable_pipestat(dev_priv, pipe, enable_mask, status_mask);
+}
+
+void
+i915_disable_pipestat(struct drm_i915_private *dev_priv, enum pipe pipe,
+ u32 status_mask)
+{
+ u32 enable_mask;
+
+ enable_mask = status_mask << 16;
+ __i915_disable_pipestat(dev_priv, pipe, enable_mask, status_mask);
+}
+
/**
* i915_enable_asle_pipestat - enable ASLE pipestat for OpRegion
*/
@@ -518,10 +548,10 @@ static void i915_enable_asle_pipestat(struct drm_device
*dev)
spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
- i915_enable_pipestat(dev_priv, PIPE_B, PIPE_LEGACY_BLC_EVENT_ENABLE);
+ i915_enable_pipestat(dev_priv, PIPE_B, PIPE_LEGACY_BLC_EVENT_STATUS);
if (INTEL_INFO(dev)->gen >= 4)
i915_enable_pipestat(dev_priv, PIPE_A,
-PIPE_LEGACY_BLC_EVENT_ENABLE);
+PIPE_LEGACY_BLC_EVENT_STATUS);
spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
}
@@ -2270,10 +2300,10 @@ static int i915_enable_vblank(struct drm_device *dev,
int pipe)
spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
if (INTEL_INFO(dev)->
Re: [Intel-gfx] [PATCH 08/13] drm/i915: Enable register whitelist checks
On Wed, Feb 05, 2014 at 07:33:28AM -0800, Jani Nikula wrote:
> On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote:
> > From: Brad Volkin
> >
> > MI_STORE_REGISTER_MEM, MI_LOAD_REGISTER_MEM, and MI_LOAD_REGISTER_IMM
> > commands allow userspace access to registers. Only certain registers
> > should be allowed for such access, so enable checking for those commands.
> > Each ring gets its own register whitelist.
> >
> > MI_LOAD_REGISTER_REG on HSW also allows register access but is currently
> > unused by userspace components. Leave it rejected.
> >
> > PIPE_CONTROL and MEDIA_VFE_STATE allow register access based on certain
> > bits being set. Reject those as well.
> >
> > OTC-Tracker: AXIA-4631
> > Change-Id: Ie614a2f0eb2e5917de809e5a17957175d24cc44f
> > Signed-off-by: Brad Volkin
> > ---
> > drivers/gpu/drm/i915/i915_cmd_parser.c | 23 ---
> > drivers/gpu/drm/i915/i915_reg.h| 3 +++
> > 2 files changed, 23 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index 296e322..5d3e303 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -63,9 +63,12 @@ static const struct drm_i915_cmd_descriptor
> > common_cmds[] = {
> > CMD( MI_SUSPEND_FLUSH, SMI,F, 1, S ),
> > CMD( MI_SEMAPHORE_MBOX,SMI, !F, 0xFF, R ),
> > CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, R ),
> > - CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, R ),
> > - CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, R ),
> > - CMD( MI_LOAD_REGISTER_MEM, SMI, !F, 0xFF, R ),
> > + CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
> > + .reg = { .offset = 1, .mask = 0x007C } ),
> > + CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, W,
> > + .reg = { .offset = 1, .mask = 0x007C } ),
> > + CMD( MI_LOAD_REGISTER_MEM, SMI, !F, 0xFF, W,
> > + .reg = { .offset = 1, .mask = 0x007C } ),
> > CMD( MI_BATCH_BUFFER_START,SMI, !F, 0xFF, S ),
> > };
> >
> > @@ -82,9 +85,23 @@ static const struct drm_i915_cmd_descriptor
> > render_cmds[] = {
> > CMD( MI_CONDITIONAL_BATCH_BUFFER_END, SMI, !F, 0xFF, S ),
> > CMD( GFX_OP_3DSTATE_VF_STATISTICS, S3D,F, 1, S ),
> > CMD( PIPELINE_SELECT, S3D,F, 1, S ),
> > + CMD( MEDIA_VFE_STATE, S3D, !F, 0x, B,
> > + .bits = {{
> > + .offset = 2,
> > + .mask = MEDIA_VFE_STATE_MMIO_ACCESS_MASK,
> > + .expected = 0
> > + }},
> > + .bits_count = 1 ),
>
> From my bikeshedding dept.: here too I think it would be beneficial to
> have the count decided by an empty element, or a .valid = 1 field or
> something.
I see your point. I'll look at doing a .valid=1 or .mask!=0 check.
- Brad
>
>
> > CMD( GPGPU_OBJECT, S3D, !F, 0xFF, S ),
> > CMD( GPGPU_WALKER, S3D, !F, 0xFF, S ),
> > CMD( GFX_OP_3DSTATE_SO_DECL_LIST, S3D, !F, 0x1FF, S ),
> > + CMD( GFX_OP_PIPE_CONTROL(5), S3D, !F, 0xFF, B,
> > + .bits = {{
> > + .offset = 1,
> > + .mask = PIPE_CONTROL_MMIO_WRITE,
> > + .expected = 0
> > + }},
> > + .bits_count = 1 ),
> > };
> >
> > static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index b99bacf..6592d0d 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -319,6 +319,7 @@
> > #define DISPLAY_PLANE_B (1<<20)
> > #define GFX_OP_PIPE_CONTROL(len) ((0x3<<29)|(0x3<<27)|(0x2<<24)|(len-2))
> > #define PIPE_CONTROL_GLOBAL_GTT_IVB (1<<24) /*
> > gen7+ */
> > +#define PIPE_CONTROL_MMIO_WRITE (1<<23)
> > #define PIPE_CONTROL_CS_STALL(1<<20)
> > #define PIPE_CONTROL_TLB_INVALIDATE (1<<18)
> > #define PIPE_CONTROL_QW_WRITE(1<<14)
> > @@ -359,6 +360,8 @@
> >
> > #define PIPELINE_SELECT
> > ((0x3<<29)|(0x1<<27)|(0x1<<24)|(0x4<<16))
> > #define GFX_OP_3DSTATE_VF_STATISTICS
> > ((0x3<<29)|(0x1<<27)|(0x0<<24)|(0xB<<16))
> > +#define MEDIA_VFE_STATE
> > ((0x3<<29)|(0x2<<27)|(0x0<<24)|(0x0<<16))
> > +#define MEDIA_VFE_STATE_MMIO_ACCESS_MASK (0x18)
> > #define GPGPU_OBJECT
> > ((0x3<<29)|(0x2<<27)|(0x1<<24)|(0x4<<16))
> > #define GPGPU_WALKER
Re: [Intel-gfx] [PATCH 06/13] drm/i915: Add register whitelists for mesa
On Wed, Feb 05, 2014 at 07:29:12AM -0800, Jani Nikula wrote:
> On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote:
> > From: Brad Volkin
> >
> > These registers are currently used by mesa for blitting,
> > transform feedback extensions, and performance monitoring
> > extensions.
> >
> > Signed-off-by: Brad Volkin
> > ---
> > drivers/gpu/drm/i915/i915_cmd_parser.c | 55
> > ++
> > drivers/gpu/drm/i915/i915_reg.h| 20 +
> > 2 files changed, 75 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > index 88456638..18d5b05 100644
> > --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> > @@ -185,6 +185,55 @@ static const struct drm_i915_cmd_table
> > hsw_blt_ring_cmds[] = {
> > { hsw_blt_cmds, ARRAY_SIZE(hsw_blt_cmds) },
> > };
> >
> > +/*
> > + * Register whitelists, sorted by increasing register offset.
> > + *
> > + * Some registers that userspace accesses are 64 bits. The register
> > + * access commands only allow 32-bit accesses. Hence, we have to include
> > + * entries for both halves of the 64-bit registers.
> > + */
>
> Seems like it would be useful to have a helper macro here.
>
> #define FOO64(addr) (addr), (addr + 4)
>
> With a better name, hopefully. My imagination fails me now.
REG64(addr)?
Or maybe just
#define REG_UPPER_DW(addr) (addr + 4)
- Brad
>
> > +
> > +static const u32 gen7_render_regs[] = {
> > + HS_INVOCATION_COUNT,
> > + HS_INVOCATION_COUNT + sizeof(u32),
> > + DS_INVOCATION_COUNT,
> > + DS_INVOCATION_COUNT + sizeof(u32),
> > + IA_VERTICES_COUNT,
> > + IA_VERTICES_COUNT + sizeof(u32),
> > + IA_PRIMITIVES_COUNT,
> > + IA_PRIMITIVES_COUNT + sizeof(u32),
> > + VS_INVOCATION_COUNT,
> > + VS_INVOCATION_COUNT + sizeof(u32),
> > + GS_INVOCATION_COUNT,
> > + GS_INVOCATION_COUNT + sizeof(u32),
> > + GS_PRIMITIVES_COUNT,
> > + GS_PRIMITIVES_COUNT + sizeof(u32),
> > + CL_INVOCATION_COUNT,
> > + CL_INVOCATION_COUNT + sizeof(u32),
> > + CL_PRIMITIVES_COUNT,
> > + CL_PRIMITIVES_COUNT + sizeof(u32),
> > + PS_INVOCATION_COUNT,
> > + PS_INVOCATION_COUNT + sizeof(u32),
> > + PS_DEPTH_COUNT,
> > + PS_DEPTH_COUNT + sizeof(u32),
> > + GEN7_SO_NUM_PRIMS_WRITTEN(0),
> > + GEN7_SO_NUM_PRIMS_WRITTEN(0) + sizeof(u32),
> > + GEN7_SO_NUM_PRIMS_WRITTEN(1),
> > + GEN7_SO_NUM_PRIMS_WRITTEN(1) + sizeof(u32),
> > + GEN7_SO_NUM_PRIMS_WRITTEN(2),
> > + GEN7_SO_NUM_PRIMS_WRITTEN(2) + sizeof(u32),
> > + GEN7_SO_NUM_PRIMS_WRITTEN(3),
> > + GEN7_SO_NUM_PRIMS_WRITTEN(3) + sizeof(u32),
> > + GEN7_SO_WRITE_OFFSET(0),
> > + GEN7_SO_WRITE_OFFSET(1),
> > + GEN7_SO_WRITE_OFFSET(2),
> > + GEN7_SO_WRITE_OFFSET(3),
> > +};
> > +
> > +static const u32 gen7_blt_regs[] = {
> > + BCS_SWCTRL,
> > +};
> > +
> > #define CLIENT_MASK 0xE000
> > #define SUBCLIENT_MASK 0x1800
> > #define MI_CLIENT0x
> > @@ -313,6 +362,9 @@ void i915_cmd_parser_init_ring(struct intel_ring_buffer
> > *ring)
> > ring->cmd_table_count = ARRAY_SIZE(gen7_render_cmds);
> > }
> >
> > + ring->reg_table = gen7_render_regs;
> > + ring->reg_count = ARRAY_SIZE(gen7_render_regs);
> > +
> > ring->get_cmd_length_mask = gen7_render_get_cmd_length_mask;
> > break;
> > case VCS:
> > @@ -329,6 +381,9 @@ void i915_cmd_parser_init_ring(struct intel_ring_buffer
> > *ring)
> > ring->cmd_table_count = ARRAY_SIZE(gen7_blt_cmds);
> > }
> >
> > + ring->reg_table = gen7_blt_regs;
> > + ring->reg_count = ARRAY_SIZE(gen7_blt_regs);
> > +
> > ring->get_cmd_length_mask = gen7_blt_get_cmd_length_mask;
> > break;
> > case VECS:
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 2b7c26e..b99bacf 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -385,6 +385,26 @@
> > #define SRC_COPY_BLT ((0x2<<29)|(0x43<<22))
> >
> > /*
> > + * Registers used only by the command parser
> > + */
> > +#define BCS_SWCTRL 0x22200
> > +
> > +#define HS_INVOCATION_COUNT 0x2300
> > +#define DS_INVOCATION_COUNT 0x2308
> > +#define IA_VERTICES_COUNT 0x2310
> > +#define IA_PRIMITIVES_COUNT 0x2318
> > +#define VS_INVOCATION_COUNT 0x2320
> > +#define GS_INVOCATION_COUNT 0x2328
> > +#define GS_PRIMITIVES_COUNT 0x2330
> > +#define CL_INVOCATION_COUNT 0x2338
> > +#define CL_PRIMITIVES_COUNT 0x2340
> > +#define PS_INVOCATION_COUNT 0x2348
> > +#define PS_DEPTH_COUNT 0x2350
> > +
> > +/* There are the 4 64-bit counter registers, one for each stream output */
> > +#define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8)
> > +
> > +/*
> > * Reset registers
> > */
> > #define DEBUG_RESET_I830 0x6070
> > --
> > 1.8.5.2
> >
> > _
Re: [Intel-gfx] [PATCH 04/13] drm/i915: Reject privileged commands
[snip] On Wed, Feb 05, 2014 at 07:22:33AM -0800, Jani Nikula wrote: > On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote: > > diff --git a/drivers/gpu/drm/i915/i915_reg.h > > b/drivers/gpu/drm/i915/i915_reg.h > > index 13ed6ed..2b7c26e 100644 > > --- a/drivers/gpu/drm/i915/i915_reg.h > > +++ b/drivers/gpu/drm/i915/i915_reg.h > > @@ -339,21 +339,22 @@ > > /* > > * Commands used only by the command parser > > */ > > -#define MI_SET_PREDICATE MI_INSTR(0x01, 0) > > -#define MI_ARB_CHECK MI_INSTR(0x05, 0) > > -#define MI_RS_CONTROL MI_INSTR(0x06, 0) > > -#define MI_URB_ATOMIC_ALLOCMI_INSTR(0x09, 0) > > -#define MI_PREDICATE MI_INSTR(0x0C, 0) > > -#define MI_RS_CONTEXT MI_INSTR(0x0F, 0) > > -#define MI_TOPOLOGY_FILTER MI_INSTR(0x0D, 0) > > -#define MI_URB_CLEAR MI_INSTR(0x19, 0) > > -#define MI_UPDATE_GTT MI_INSTR(0x23, 0) > > -#define MI_CLFLUSH MI_INSTR(0x27, 0) > > -#define MI_LOAD_REGISTER_MEM MI_INSTR(0x29, 0) > > -#define MI_LOAD_REGISTER_REG MI_INSTR(0x2A, 0) > > -#define MI_RS_STORE_DATA_IMM MI_INSTR(0x2B, 0) > > -#define MI_LOAD_URB_MEMMI_INSTR(0x2C, 0) > > -#define MI_STORE_URB_MEM MI_INSTR(0x2D, 0) > > +#define MI_SET_PREDICATEMI_INSTR(0x01, 0) > > +#define MI_ARB_CHECKMI_INSTR(0x05, 0) > > +#define MI_RS_CONTROL MI_INSTR(0x06, 0) > > +#define MI_URB_ATOMIC_ALLOC MI_INSTR(0x09, 0) > > +#define MI_PREDICATEMI_INSTR(0x0C, 0) > > +#define MI_RS_CONTEXT MI_INSTR(0x0F, 0) > > +#define MI_TOPOLOGY_FILTER MI_INSTR(0x0D, 0) > > +#define MI_LOAD_SCAN_LINES_EXCL MI_INSTR(0x13, 0) > > +#define MI_URB_CLEARMI_INSTR(0x19, 0) > > +#define MI_UPDATE_GTT MI_INSTR(0x23, 0) > > +#define MI_CLFLUSH MI_INSTR(0x27, 0) > > +#define MI_LOAD_REGISTER_MEMMI_INSTR(0x29, 0) > > +#define MI_LOAD_REGISTER_REGMI_INSTR(0x2A, 0) > > +#define MI_RS_STORE_DATA_IMMMI_INSTR(0x2B, 0) > > +#define MI_LOAD_URB_MEM MI_INSTR(0x2C, 0) > > +#define MI_STORE_URB_MEMMI_INSTR(0x2D, 0) > > Superfluous whitespace change hunk. It adds MI_LOAD_SCAN_LINES_EXCL and adjusts the whitespace to line up. I see that the whitespace change makes the actual change less obvious. I'll try to clean that up. - Brad > > > > #define MI_CONDITIONAL_BATCH_BUFFER_END MI_INSTR(0x36, 0) > > > > #define PIPELINE_SELECT > > ((0x3<<29)|(0x1<<27)|(0x1<<24)|(0x4<<16)) > > -- > > 1.8.5.2 > > > > ___ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Jani Nikula, Intel Open Source Technology Center ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 00/22] Gen7 batch buffer command parser
On Wed, Feb 5, 2014 at 7:18 PM, Volkin, Bradley D wrote: > On Wed, Feb 05, 2014 at 02:28:29AM -0800, Chris Wilson wrote: >> On Tue, Nov 26, 2013 at 08:51:17AM -0800, bradley.d.vol...@intel.com wrote: >> > From: Brad Volkin >> > >> > Certain OpenGL features (e.g. transform feedback, performance monitoring) >> > require userspace code to submit batches containing commands such as >> > MI_LOAD_REGISTER_IMM to access various registers. Unfortunately, some >> > generations of the hardware will noop these commands in "unsecure" batches >> > (which includes all userspace batches submitted via i915) even though the >> > commands may be safe and represent the intended programming model of the >> > device. >> > >> > This series introduces a software command parser similar in operation to >> > the >> > command parsing done in hardware for unsecure batches. However, the >> > software >> > parser allows some operations that would be noop'd by hardware, if the >> > parser >> > determines the operation is safe, and submits the batch as "secure" to >> > prevent >> > hardware parsing. Currently the series implements this on IVB and HSW. >> >> Just one more question... Do you have a branch for people to test? > > Not at the moment. And as mentioned in the v2 cover letter, it's actually not > particularly testable (or mergeable for that matter) right now because of a > regression in secure dispatch on nightly. The command parser itself should still work, even with the regression in -nightly. The copying and secure dispatch are obviously fail atm. That still leaves regression testing of current userspace and micro-optimizing the checker itself as possible things to do. Otoh not sure what exactly Chris wanted to test. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic
On Wed, Feb 05, 2014 at 07:15:35AM -0800, Jani Nikula wrote: > On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote: > > From: Brad Volkin > > > > The command parser scans batch buffers submitted via execbuffer ioctls > > before > > the driver submits them to hardware. At a high level, it looks for several > > things: > > > > 1) Commands which are explicitly defined as privileged or which should only > > be > >used by the kernel driver. The parser generally rejects such commands, > > with > >the provision that it may allow some from the drm master process. > > 2) Commands which access registers. To support correct/enhanced userspace > >functionality, particularly certain OpenGL extensions, the parser > > provides a > >whitelist of registers which userspace may safely access (for both > > normal and > >drm master processes). > > 3) Commands which access privileged memory (i.e. GGTT, HWS page, etc). The > >parser always rejects such commands. > > > > Each ring maintains tables of commands and registers which the parser uses > > in > > scanning batch buffers submitted to that ring. > > > > The set of commands that the parser must check for is significantly smaller > > than the number of commands supported, especially on the render ring. As > > such, > > the parser tables (built up in subsequent patches) contain only those > > commands > > required by the parser. This generally works because command opcode ranges > > have > > standard command length encodings. So for commands that the parser does not > > need > > to check, it can easily skip them. This is implementated via a per-ring > > length > > decoding vfunc. > > > > Unfortunately, there are a number of commands that do not follow the > > standard > > length encoding for their opcode range, primarily amongst the MI_* > > commands. To > > handle this, the parser provides a way to define explicit "skip" entries in > > the > > per-ring command tables. > > > > Other command table entries will map fairly directly to high level > > categories > > mentioned above: rejected, master-only, register whitelist. A number of > > checks, > > including the privileged memory checks, are implemented via a general > > bitmasking > > mechanism. > > > > OTC-Tracker: AXIA-4631 > > Change-Id: I50b98c71c6655893291c78a2d1b8954577b37a30 > > Signed-off-by: Brad Volkin > > --- > > drivers/gpu/drm/i915/Makefile | 3 +- > > drivers/gpu/drm/i915/i915_cmd_parser.c | 404 > > + > > drivers/gpu/drm/i915/i915_drv.h| 94 +++ > > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 17 ++ > > drivers/gpu/drm/i915/i915_params.c | 5 + > > drivers/gpu/drm/i915/intel_ringbuffer.c| 2 + > > drivers/gpu/drm/i915/intel_ringbuffer.h| 32 +++ > > 7 files changed, 556 insertions(+), 1 deletion(-) > > create mode 100644 drivers/gpu/drm/i915/i915_cmd_parser.c > > > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > > index 4850494..2da81bf 100644 > > --- a/drivers/gpu/drm/i915/Makefile > > +++ b/drivers/gpu/drm/i915/Makefile > > @@ -47,7 +47,8 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \ > > dvo_tfp410.o \ > > dvo_sil164.o \ > > dvo_ns2501.o \ > > - i915_gem_dmabuf.o > > + i915_gem_dmabuf.o \ > > + i915_cmd_parser.o > > If you add this anywhere but last, you only need to touch one line > instead of two. It's nitpicky, but helps with things like git blame > (which would now point at you for i915_gem_dmabuf.o too ;). Sounds good > > > > > i915-$(CONFIG_COMPAT) += i915_ioc32.o > > > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c > > b/drivers/gpu/drm/i915/i915_cmd_parser.c > > new file mode 100644 > > index 000..7639dbc > > --- /dev/null > > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > > @@ -0,0 +1,404 @@ > > +/* > > + * Copyright © 2013 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,
Re: [Intel-gfx] [RFC 00/22] Gen7 batch buffer command parser
On Wed, Feb 05, 2014 at 02:28:29AM -0800, Chris Wilson wrote: > On Tue, Nov 26, 2013 at 08:51:17AM -0800, bradley.d.vol...@intel.com wrote: > > From: Brad Volkin > > > > Certain OpenGL features (e.g. transform feedback, performance monitoring) > > require userspace code to submit batches containing commands such as > > MI_LOAD_REGISTER_IMM to access various registers. Unfortunately, some > > generations of the hardware will noop these commands in "unsecure" batches > > (which includes all userspace batches submitted via i915) even though the > > commands may be safe and represent the intended programming model of the > > device. > > > > This series introduces a software command parser similar in operation to the > > command parsing done in hardware for unsecure batches. However, the software > > parser allows some operations that would be noop'd by hardware, if the > > parser > > determines the operation is safe, and submits the batch as "secure" to > > prevent > > hardware parsing. Currently the series implements this on IVB and HSW. > > Just one more question... Do you have a branch for people to test? Not at the moment. And as mentioned in the v2 cover letter, it's actually not particularly testable (or mergeable for that matter) right now because of a regression in secure dispatch on nightly. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 00/22] Gen7 batch buffer command parser
On Wed, Feb 05, 2014 at 10:18:44AM -0800, Volkin, Bradley D wrote: > On Wed, Feb 05, 2014 at 02:28:29AM -0800, Chris Wilson wrote: > > On Tue, Nov 26, 2013 at 08:51:17AM -0800, bradley.d.vol...@intel.com wrote: > > > From: Brad Volkin > > > > > > Certain OpenGL features (e.g. transform feedback, performance monitoring) > > > require userspace code to submit batches containing commands such as > > > MI_LOAD_REGISTER_IMM to access various registers. Unfortunately, some > > > generations of the hardware will noop these commands in "unsecure" batches > > > (which includes all userspace batches submitted via i915) even though the > > > commands may be safe and represent the intended programming model of the > > > device. > > > > > > This series introduces a software command parser similar in operation to > > > the > > > command parsing done in hardware for unsecure batches. However, the > > > software > > > parser allows some operations that would be noop'd by hardware, if the > > > parser > > > determines the operation is safe, and submits the batch as "secure" to > > > prevent > > > hardware parsing. Currently the series implements this on IVB and HSW. > > > > Just one more question... Do you have a branch for people to test? > > Not at the moment. And as mentioned in the v2 cover letter, it's actually not > particularly testable (or mergeable for that matter) right now because of a > regression in secure dispatch on nightly. At this moment, I just want to be sure that the fixed dispatch overhead has been minimised. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v9
On Wed, Feb 05, 2014 at 05:30:48PM +, Jesse Barnes wrote:
> +static bool intel_fbdev_init_bios(struct drm_device *dev,
> + struct intel_fbdev *ifbdev)
> +{
> + struct intel_framebuffer *fb = NULL;
> + struct drm_crtc *crtc;
> + struct intel_crtc *intel_crtc;
> + struct intel_plane_config *plane_config = NULL;
> + unsigned int last_size = 0, max_size = 0, tmp;
> +
> + /* Find the largest framebuffer to use, then free the others */
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> + intel_crtc = to_intel_crtc(crtc);
> +
> + if (!intel_crtc->active || !intel_crtc->plane_config.fb) {
> + DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
> + pipe_name(intel_crtc->pipe));
> + continue;
> + }
> +
> + tmp = intel_crtc->config.adjusted_mode.crtc_hdisplay *
> + intel_crtc->config.adjusted_mode.crtc_vdisplay *
> + intel_crtc->plane_config.fb->base.pitches[0];
This reads as width * height * stride. I presume you meant bpp/8 here,
but since we keep the fb and its pitch intact, you need height * stride.
Actually, it preserves a completely different fb, so this estimate of
size is incomplete.
> + max_size = max(max_size, tmp);
> +
> + /*
> + * Make sure the fb will fit the biggest active pipe. If
> + * not, clear out any previous usage.
> + */
> + if (intel_crtc->plane_config.size > last_size &&
> + intel_crtc->plane_config.size >= max_size) {
> + plane_config = &intel_crtc->plane_config;
> + last_size = plane_config->size;
> + fb = plane_config->fb;
> + } else {
> + plane_config = NULL;
> + fb = NULL;
> + }
Two CRTCs sharing a plane_config will now end up with plane_config/fb =
NULL, and so bail out. This is confusing me. If we here just found the
largest fb and then did a second pass confirming that all CRTC and planes
fitted into it, I would find it easier to understand.
> + }
> +
> + /* Free unused fbs */
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> + struct intel_framebuffer *cur_fb;
> +
> + intel_crtc = to_intel_crtc(crtc);
> + cur_fb = intel_crtc->plane_config.fb;
> +
> + if (cur_fb && cur_fb != fb)
> + intel_framebuffer_fini(cur_fb);
> + }
> +
> + if (!fb) {
> + DRM_DEBUG_KMS("no active pipes found, not using BIOS config\n");
> + goto out;
> + }
> +
> + ifbdev->preferred_bpp = plane_config->fb->base.bits_per_pixel;
> + ifbdev->helper.funcs = &intel_fb_helper_funcs;
> + ifbdev->helper.funcs->initial_config = intel_fb_initial_config;
> + ifbdev->fb = fb;
> +
> + /* Assuming a single fb across all pipes here */
> + list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> + intel_crtc = to_intel_crtc(crtc);
> +
> + if (!intel_crtc->active)
> + continue;
> +
> + /*
> + * This should only fail on the first one so we don't need
> + * to cleanup any secondary crtc->fbs
> + */
> + if (intel_pin_and_fence_fb_obj(dev, fb->obj, NULL))
> + goto out_unref_obj;
> +
> + crtc->fb = &fb->base;
> + drm_gem_object_reference(&fb->obj->base);
> + drm_framebuffer_reference(&fb->base);
> + }
> +
> + DRM_DEBUG_KMS("using BIOS fb for initial console\n");
> + return true;
Somewhere around here we must disable all CRTCs that are active and have
no fb.
> +
> +out_unref_obj:
> + intel_framebuffer_fini(fb);
> +out:
> +
> + return false;
> +}
> +
> int intel_fbdev_init(struct drm_device *dev)
> {
> struct intel_fbdev *ifbdev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> int ret;
A useful assertion here would be:
if (WARN_ON(INTEL_INFO(dev)->num_pipes == 0)) return -ENODEV;
> - ifbdev = kzalloc(sizeof(*ifbdev), GFP_KERNEL);
> - if (!ifbdev)
> + ifbdev = kzalloc(sizeof(struct intel_fbdev), GFP_KERNEL);
> + if (ifbdev == NULL)
> return -ENOMEM;
>
> - dev_priv->fbdev = ifbdev;
> ifbdev->helper.funcs = &intel_fb_helper_funcs;
> + dev_priv->fbdev = ifbdev;
> +
> + if (!i915.fastboot || !intel_fbdev_init_bios(dev, ifbdev))
> + ifbdev->preferred_bpp = 32;
Bikeshed: move i915.fastboot to intel_fbdev_init_bios and rearrange like
if (!intel_fbdev_init(intel_fbdev_init_bios(dev, ifbdev)) {
ifbdev->helper.funcs = &intel_fb_helper_funcs;
ifbdev->preferred_bpp = 32;
}
(then dev_priv->fbdev = ifbdev can be done last on success as before)
[Intel-gfx] [PATCH 4/6] drm/i915: alloc intel_fb in the intel_fbdev struct
Allocate this struct instead, so we can re-use another allocated
elsewhere if needed.
Signed-off-by: Jesse Barnes
---
drivers/gpu/drm/i915/intel_display.c |4 ++--
drivers/gpu/drm/i915/intel_drv.h |2 +-
drivers/gpu/drm/i915/intel_fbdev.c | 27 +++
3 files changed, 22 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index f93ea6e..c02f032 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7975,11 +7975,11 @@ mode_fits_in_fbdev(struct drm_device *dev,
if (dev_priv->fbdev == NULL)
return NULL;
- obj = dev_priv->fbdev->ifb.obj;
+ obj = dev_priv->fbdev->fb->obj;
if (obj == NULL)
return NULL;
- fb = &dev_priv->fbdev->ifb.base;
+ fb = &dev_priv->fbdev->fb->base;
if (fb->pitches[0] < intel_framebuffer_pitch_for_width(mode->hdisplay,
fb->bits_per_pixel))
return NULL;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 1e5a0a6..8f5e798 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -110,7 +110,7 @@ struct intel_framebuffer {
struct intel_fbdev {
struct drm_fb_helper helper;
- struct intel_framebuffer ifb;
+ struct intel_framebuffer *fb;
struct list_head fbdev_list;
struct drm_display_mode *our_mode;
};
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
b/drivers/gpu/drm/i915/intel_fbdev.c
index d6a8a71..fb07ba6 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -62,11 +62,20 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
{
struct intel_fbdev *ifbdev =
container_of(helper, struct intel_fbdev, helper);
+ struct intel_framebuffer *fb;
struct drm_device *dev = helper->dev;
struct drm_mode_fb_cmd2 mode_cmd = {};
struct drm_i915_gem_object *obj;
int size, ret;
+ fb = kzalloc(sizeof(*fb), GFP_KERNEL);
+ if (!fb) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ ifbdev->fb = fb;
+
/* we don't do packed 24bpp */
if (sizes->surface_bpp == 24)
sizes->surface_bpp = 32;
@@ -97,7 +106,7 @@ static int intelfb_alloc(struct drm_fb_helper *helper,
goto out_unref;
}
- ret = intel_framebuffer_init(dev, &ifbdev->ifb, &mode_cmd, obj);
+ ret = intel_framebuffer_init(dev, ifbdev->fb, &mode_cmd, obj);
if (ret)
goto out_unpin;
@@ -116,7 +125,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
{
struct intel_fbdev *ifbdev =
container_of(helper, struct intel_fbdev, helper);
- struct intel_framebuffer *intel_fb = &ifbdev->ifb;
+ struct intel_framebuffer *intel_fb = ifbdev->fb;
struct drm_device *dev = helper->dev;
struct drm_i915_private *dev_priv = dev->dev_private;
struct fb_info *info;
@@ -126,11 +135,12 @@ static int intelfb_create(struct drm_fb_helper *helper,
mutex_lock(&dev->struct_mutex);
- if (!intel_fb->obj) {
+ if (!intel_fb || !intel_fb->obj) {
DRM_DEBUG_KMS("no BIOS fb, allocating a new one\n");
ret = intelfb_alloc(helper, sizes);
if (ret)
goto out_unlock;
+ intel_fb = ifbdev->fb;
} else {
DRM_DEBUG_KMS("re-using BIOS fb\n");
sizes->fb_width = intel_fb->base.width;
@@ -148,7 +158,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
info->par = helper;
- fb = &ifbdev->ifb.base;
+ fb = &ifbdev->fb->base;
ifbdev->helper.fb = fb;
ifbdev->helper.fbdev = info;
@@ -194,7 +204,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
* If the object is stolen however, it will be full of whatever
* garbage was left in there.
*/
- if (ifbdev->ifb.obj->stolen)
+ if (ifbdev->fb->obj->stolen)
memset_io(info->screen_base, 0, info->screen_size);
/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
@@ -258,8 +268,9 @@ static void intel_fbdev_destroy(struct drm_device *dev,
drm_fb_helper_fini(&ifbdev->helper);
- drm_framebuffer_unregister_private(&ifbdev->ifb.base);
- intel_framebuffer_fini(&ifbdev->ifb);
+ drm_framebuffer_unregister_private(&ifbdev->fb->base);
+ intel_framebuffer_fini(ifbdev->fb);
+ kfree(ifbdev->fb);
}
int intel_fbdev_init(struct drm_device *dev)
@@ -322,7 +333,7 @@ void intel_fbdev_set_suspend(struct drm_device *dev, int
state)
* been restored from swap. If the object is stolen however, it will be
* full of whatever garbage was left
[Intel-gfx] [PATCH 1/6] drm/i915: split aligned height calculation out
For use by get_plane_config.
Signed-off-by: Jesse Barnes
---
drivers/gpu/drm/i915/intel_display.c | 15 +++
1 file changed, 11 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 4d4a0d9..e2f1db6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -1929,6 +1929,14 @@ static bool need_vtd_wa(struct drm_device *dev)
return false;
}
+static int intel_align_height(struct drm_device *dev, int height, bool tiled)
+{
+ int tile_height;
+
+ tile_height = IS_GEN2(dev) ? 16 : 8;
+ return ALIGN(height, tiled ? tile_height : 1);
+}
+
int
intel_pin_and_fence_fb_obj(struct drm_device *dev,
struct drm_i915_gem_object *obj,
@@ -10559,7 +10567,7 @@ int intel_framebuffer_init(struct drm_device *dev,
struct drm_mode_fb_cmd2 *mode_cmd,
struct drm_i915_gem_object *obj)
{
- int aligned_height, tile_height;
+ int aligned_height;
int pitch_limit;
int ret;
@@ -10653,9 +10661,8 @@ int intel_framebuffer_init(struct drm_device *dev,
if (mode_cmd->offsets[0] != 0)
return -EINVAL;
- tile_height = IS_GEN2(dev) ? 16 : 8;
- aligned_height = ALIGN(mode_cmd->height,
- obj->tiling_mode ? tile_height : 1);
+ aligned_height = intel_align_height(dev, mode_cmd->height,
+ obj->tiling_mode);
/* FIXME drm helper for size checks (especially planar formats)? */
if (obj->base.size < aligned_height * mode_cmd->pitches[0])
return -EINVAL;
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] tests/gem_userptr_benchmark: Benchmarking userptr surfaces and impact
On Wed, Feb 05, 2014 at 05:33:06PM +, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > This adds a small benchmark for the new userptr functionality. > > Apart from basic surface creation and destruction, also tested is the > impact of having userptr surfaces in the process address space. Reason > for that is the impact of MMU notifiers on common address space > operations like munmap() which is per process. > > v2: > * Moved to benchmarks. I'd just keep it as an igt testcase, beating on the kernel a bit can't hurt. And we have piles of other benchmark-like testcase already around. Chris' comment on irc that I should create some better benchmark infrastructure was probably more aimed to use all the microbenchmarks we have to catch regressions. Atm we have zero infrastructure for tests to expose performance numbers, so can't really do that. But that's work for another day (month/year). -Daniel > * Added pointer read/write tests. > * Changed output to say iterations per second instead of > operations per second. > * Multiply result by batch size for multi-create* tests > for a more comparable number with create-destroy test. > > Signed-off-by: Tvrtko Ursulin > Cc: Chris Wilson > --- > Android.mk | 3 +- > benchmarks/.gitignore | 1 + > benchmarks/Android.mk | 36 +++ > benchmarks/Makefile.am | 7 +- > benchmarks/Makefile.sources| 6 + > benchmarks/gem_userptr_benchmark.c | 513 > + > 6 files changed, 558 insertions(+), 8 deletions(-) > create mode 100644 benchmarks/Android.mk > create mode 100644 benchmarks/Makefile.sources > create mode 100644 benchmarks/gem_userptr_benchmark.c > > diff --git a/Android.mk b/Android.mk > index 8aeb2d4..0c969b8 100644 > --- a/Android.mk > +++ b/Android.mk > @@ -1,2 +1 @@ > -include $(call all-named-subdir-makefiles, lib tests tools) > - > +include $(call all-named-subdir-makefiles, lib tests tools benchmarks) > diff --git a/benchmarks/.gitignore b/benchmarks/.gitignore > index ddea6f7..09e5bd8 100644 > --- a/benchmarks/.gitignore > +++ b/benchmarks/.gitignore > @@ -1,3 +1,4 @@ > +gem_userptr_benchmark > intel_upload_blit_large > intel_upload_blit_large_gtt > intel_upload_blit_large_map > diff --git a/benchmarks/Android.mk b/benchmarks/Android.mk > new file mode 100644 > index 000..5bb8ef5 > --- /dev/null > +++ b/benchmarks/Android.mk > @@ -0,0 +1,36 @@ > +LOCAL_PATH := $(call my-dir) > + > +include $(LOCAL_PATH)/Makefile.sources > + > +## > + > +define add_benchmark > +include $(CLEAR_VARS) > + > +LOCAL_SRC_FILES := $1.c > + > +LOCAL_CFLAGS += -DHAVE_STRUCT_SYSINFO_TOTALRAM > +LOCAL_CFLAGS += -DANDROID -UNDEBUG -include "check-ndebug.h" > +LOCAL_CFLAGS += -std=c99 > +# FIXME: drop once Bionic correctly annotates "noreturn" on pthread_exit > +LOCAL_CFLAGS += -Wno-error=return-type > +# Excessive complaining for established cases. Rely on the Linux version > warnings. > +LOCAL_CFLAGS += -Wno-sign-compare > + > +LOCAL_MODULE := $1 > +LOCAL_MODULE_TAGS := optional > + > +LOCAL_STATIC_LIBRARIES := libintel_gpu_tools > + > +LOCAL_SHARED_LIBRARIES := libpciaccess \ > + libdrm\ > + libdrm_intel > + > +include $(BUILD_EXECUTABLE) > +endef > + > +## > + > +benchmark_list := $(bin_PROGRAMS) > + > +$(foreach item,$(benchmark_list),$(eval $(call add_benchmark,$(item > diff --git a/benchmarks/Makefile.am b/benchmarks/Makefile.am > index e2ad784..d173bf4 100644 > --- a/benchmarks/Makefile.am > +++ b/benchmarks/Makefile.am > @@ -1,9 +1,4 @@ > - > -bin_PROGRAMS = \ > - intel_upload_blit_large \ > - intel_upload_blit_large_gtt \ > - intel_upload_blit_large_map \ > - intel_upload_blit_small > +include Makefile.sources > > AM_CPPFLAGS = -I$(top_srcdir) -I$(top_srcdir)/lib > AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS) > diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources > new file mode 100644 > index 000..fd6c107 > --- /dev/null > +++ b/benchmarks/Makefile.sources > @@ -0,0 +1,6 @@ > +bin_PROGRAMS = \ > +intel_upload_blit_large \ > +intel_upload_blit_large_gtt \ > +intel_upload_blit_large_map \ > +intel_upload_blit_small \ > +gem_userptr_benchmark > diff --git a/benchmarks/gem_userptr_benchmark.c > b/benchmarks/gem_userptr_benchmark.c > new file mode 100644 > index 000..dc36f59 > --- /dev/null > +++ b/benchmarks/gem_userptr_benchmark.c > @@ -0,0 +1,513 @@ > +/* > + * Copyright © 2014 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
Re: [Intel-gfx] [PATCH 1/6] drm/i915: split aligned height calculation out
On Wed, Feb 05, 2014 at 05:30:43PM +, Jesse Barnes wrote:
> For use by get_plane_config.
>
> Signed-off-by: Jesse Barnes
> ---
> drivers/gpu/drm/i915/intel_display.c | 15 +++
> 1 file changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 4d4a0d9..e2f1db6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -1929,6 +1929,14 @@ static bool need_vtd_wa(struct drm_device *dev)
> return false;
> }
>
> +static int intel_align_height(struct drm_device *dev, int height, bool tiled)
> +{
> + int tile_height;
> +
> + tile_height = IS_GEN2(dev) ? 16 : 8;
> + return ALIGN(height, tiled ? tile_height : 1);
Might as well take a moment here to move that ?: outside of the macro.
tile_height = tiled ? IS_GEN2(dev) ? 16 : 8 : 1;
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 3/6] drm/i915: get_plane_config support for ILK+
This should allow BIOS fb inheritance to work on ILK+ machines too.
Signed-off-by: Jesse Barnes
---
drivers/gpu/drm/i915/intel_display.c | 92 ++
1 file changed, 92 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 2eb1108..f93ea6e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -6571,6 +6571,96 @@ static void ironlake_get_pfit_config(struct intel_crtc
*crtc,
}
}
+static void ironlake_get_plane_config(struct intel_crtc *crtc,
+ struct intel_plane_config *plane_config)
+{
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_i915_gem_object *obj = NULL;
+ struct drm_mode_fb_cmd2 mode_cmd = { 0 };
+ u32 val, base, offset;
+ int pipe = crtc->pipe, plane = crtc->plane;
+ int fourcc, pixel_format;
+ int aligned_height;
+
+ plane_config->fb = kzalloc(sizeof(*plane_config->fb), GFP_KERNEL);
+ if (!plane_config->fb) {
+ DRM_DEBUG_KMS("failed to alloc fb\n");
+ return;
+ }
+
+ val = I915_READ(DSPCNTR(plane));
+
+ if (INTEL_INFO(dev)->gen >= 4)
+ if (val & DISPPLANE_TILED)
+ plane_config->tiled = true;
+
+ pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
+ fourcc = intel_format_to_fourcc(pixel_format);
+ plane_config->fb->base.pixel_format = fourcc;
+ plane_config->fb->base.bits_per_pixel =
+ drm_format_plane_cpp(fourcc, 0) * 8;
+
+ base = I915_READ(DSPSURF(plane)) & 0xf000;
+ if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
+ offset = I915_READ(DSPOFFSET(plane));
+ } else {
+ if (plane_config->tiled)
+ offset = I915_READ(DSPTILEOFF(plane));
+ else
+ offset = I915_READ(DSPLINOFF(plane));
+ }
+
+ val = I915_READ(PIPESRC(pipe));
+ plane_config->fb->base.width = ((val >> 16) & 0xfff) + 1;
+ plane_config->fb->base.height = ((val >> 0) & 0xfff) + 1;
+
+ val = I915_READ(DSPSTRIDE(pipe));
+ plane_config->fb->base.pitches[0] = val & 0xff80;
+
+ aligned_height = intel_align_height(dev, plane_config->fb->base.height,
+ plane_config->tiled);
+
+ plane_config->size = ALIGN(plane_config->fb->base.pitches[0] *
+ aligned_height, PAGE_SIZE);
+
+ DRM_DEBUG_KMS("pipe/plane %d/%d with fb: size=%dx%d@%d, offset=%x,
pitch %d, size 0x%x\n",
+ pipe, plane, plane_config->fb->base.width,
+ plane_config->fb->base.height,
+ plane_config->fb->base.bits_per_pixel, base,
+ plane_config->fb->base.pitches[0],
+ plane_config->size);
+
+ /*
+* If the fb is shared between multiple heads, we'll just get the
+* first one.
+*/
+ obj = i915_gem_object_create_stolen_for_preallocated(dev, base, base,
+
plane_config->size);
+ if (!obj)
+ return;
+
+ mode_cmd.pixel_format = fourcc;
+ mode_cmd.width = plane_config->fb->base.width;
+ mode_cmd.height = plane_config->fb->base.height;
+ mode_cmd.pitches[0] = plane_config->fb->base.pitches[0];
+
+ mutex_lock(&dev->struct_mutex);
+
+ if (intel_framebuffer_init(dev, plane_config->fb, &mode_cmd, obj)) {
+ DRM_DEBUG_KMS("intel fb init failed\n");
+ goto out_unref_obj;
+ }
+
+ mutex_unlock(&dev->struct_mutex);
+ DRM_DEBUG_KMS("plane fb obj %p\n", plane_config->fb->obj);
+ return;
+
+out_unref_obj:
+ drm_gem_object_unreference(&obj->base);
+ mutex_unlock(&dev->struct_mutex);
+}
+
static bool ironlake_get_pipe_config(struct intel_crtc *crtc,
struct intel_crtc_config *pipe_config)
{
@@ -10833,6 +10923,7 @@ static void intel_init_display(struct drm_device *dev)
if (HAS_DDI(dev)) {
dev_priv->display.get_pipe_config = haswell_get_pipe_config;
+ dev_priv->display.get_plane_config = ironlake_get_plane_config;
dev_priv->display.crtc_mode_set = haswell_crtc_mode_set;
dev_priv->display.crtc_enable = haswell_crtc_enable;
dev_priv->display.crtc_disable = haswell_crtc_disable;
@@ -10840,6 +10931,7 @@ static void intel_init_display(struct drm_device *dev)
dev_priv->display.update_plane = ironlake_update_plane;
} else if (HAS_PCH_SPLIT(dev)) {
dev_priv->display.get_pipe_config = ironlake_get_pipe_config;
+ dev_priv->display.get_plane_config = ironlake_get_plane_config;
dev_priv
[Intel-gfx] [PATCH] tests/gem_userptr_benchmark: Benchmarking userptr surfaces and impact
From: Tvrtko Ursulin This adds a small benchmark for the new userptr functionality. Apart from basic surface creation and destruction, also tested is the impact of having userptr surfaces in the process address space. Reason for that is the impact of MMU notifiers on common address space operations like munmap() which is per process. v2: * Moved to benchmarks. * Added pointer read/write tests. * Changed output to say iterations per second instead of operations per second. * Multiply result by batch size for multi-create* tests for a more comparable number with create-destroy test. Signed-off-by: Tvrtko Ursulin Cc: Chris Wilson --- Android.mk | 3 +- benchmarks/.gitignore | 1 + benchmarks/Android.mk | 36 +++ benchmarks/Makefile.am | 7 +- benchmarks/Makefile.sources| 6 + benchmarks/gem_userptr_benchmark.c | 513 + 6 files changed, 558 insertions(+), 8 deletions(-) create mode 100644 benchmarks/Android.mk create mode 100644 benchmarks/Makefile.sources create mode 100644 benchmarks/gem_userptr_benchmark.c diff --git a/Android.mk b/Android.mk index 8aeb2d4..0c969b8 100644 --- a/Android.mk +++ b/Android.mk @@ -1,2 +1 @@ -include $(call all-named-subdir-makefiles, lib tests tools) - +include $(call all-named-subdir-makefiles, lib tests tools benchmarks) diff --git a/benchmarks/.gitignore b/benchmarks/.gitignore index ddea6f7..09e5bd8 100644 --- a/benchmarks/.gitignore +++ b/benchmarks/.gitignore @@ -1,3 +1,4 @@ +gem_userptr_benchmark intel_upload_blit_large intel_upload_blit_large_gtt intel_upload_blit_large_map diff --git a/benchmarks/Android.mk b/benchmarks/Android.mk new file mode 100644 index 000..5bb8ef5 --- /dev/null +++ b/benchmarks/Android.mk @@ -0,0 +1,36 @@ +LOCAL_PATH := $(call my-dir) + +include $(LOCAL_PATH)/Makefile.sources + +## + +define add_benchmark +include $(CLEAR_VARS) + +LOCAL_SRC_FILES := $1.c + +LOCAL_CFLAGS += -DHAVE_STRUCT_SYSINFO_TOTALRAM +LOCAL_CFLAGS += -DANDROID -UNDEBUG -include "check-ndebug.h" +LOCAL_CFLAGS += -std=c99 +# FIXME: drop once Bionic correctly annotates "noreturn" on pthread_exit +LOCAL_CFLAGS += -Wno-error=return-type +# Excessive complaining for established cases. Rely on the Linux version warnings. +LOCAL_CFLAGS += -Wno-sign-compare + +LOCAL_MODULE := $1 +LOCAL_MODULE_TAGS := optional + +LOCAL_STATIC_LIBRARIES := libintel_gpu_tools + +LOCAL_SHARED_LIBRARIES := libpciaccess \ + libdrm\ + libdrm_intel + +include $(BUILD_EXECUTABLE) +endef + +## + +benchmark_list := $(bin_PROGRAMS) + +$(foreach item,$(benchmark_list),$(eval $(call add_benchmark,$(item diff --git a/benchmarks/Makefile.am b/benchmarks/Makefile.am index e2ad784..d173bf4 100644 --- a/benchmarks/Makefile.am +++ b/benchmarks/Makefile.am @@ -1,9 +1,4 @@ - -bin_PROGRAMS = \ - intel_upload_blit_large \ - intel_upload_blit_large_gtt \ - intel_upload_blit_large_map \ - intel_upload_blit_small +include Makefile.sources AM_CPPFLAGS = -I$(top_srcdir) -I$(top_srcdir)/lib AM_CFLAGS = $(DRM_CFLAGS) $(CWARNFLAGS) $(CAIRO_CFLAGS) diff --git a/benchmarks/Makefile.sources b/benchmarks/Makefile.sources new file mode 100644 index 000..fd6c107 --- /dev/null +++ b/benchmarks/Makefile.sources @@ -0,0 +1,6 @@ +bin_PROGRAMS = \ +intel_upload_blit_large \ +intel_upload_blit_large_gtt \ +intel_upload_blit_large_map \ +intel_upload_blit_small \ +gem_userptr_benchmark diff --git a/benchmarks/gem_userptr_benchmark.c b/benchmarks/gem_userptr_benchmark.c new file mode 100644 index 000..dc36f59 --- /dev/null +++ b/benchmarks/gem_userptr_benchmark.c @@ -0,0 +1,513 @@ +/* + * Copyright © 2014 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, WHE
Re: [Intel-gfx] [PATCH] drm/i915: Record pid/comm of hanging task
On Wed, 5 Feb 2014 16:40:41 + Chris Wilson wrote: > After finding the guilty batch and request, we can use it to find the > process that submitted the batch and then add the culprit into the error > state. > > This is a slightly different approach from Ben's in that instead of > adding the extra information into the struct i915_hw_context, we use the > information already captured in struct drm_file which is then referenced > from the request. > > Link: http://lists.freedesktop.org/archives/intel-gfx/2013-August/032280.html > Signed-off-by: Chris Wilson > Cc: Ben Widawsky Would be nice to have this in dmesg, so I can tell if it's my bitcoin miner or a chrome tab that's killing things. Thanks, Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 6/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v9
Retrieve current framebuffer config info from the regs and create an fb
object for the buffer the BIOS or boot loader left us. This should
allow for smooth transitions to userspace apps once we finish the
initial configuration construction.
v2: check for non-native modes and adjust (Jesse)
fixup aperture and cmap frees (Imre)
use unlocked unref if init_bios fails (Jesse)
fix curly brace around DSPADDR check (Imre)
comment failure path for pin_and_fence (Imre)
v3: fixup fixup of aperture frees (Chris)
v4: update to current bits (locking & pin_and_fence hack) (Jesse)
v5: move fb config fetch to display code (Jesse)
re-order hw state readout on initial load to suit fb inherit (Jesse)
re-add pin_and_fence in fbdev code to make sure we refcount properly (Je
v6: rename to plane_config (Daniel)
check for valid object when initializing BIOS fb (Jesse)
split from plane_config readout and other display changes (Jesse)
drop use_bios_fb option (Chris)
update comments (Jesse)
rework fbdev_init_bios for clarity (Jesse)
drop fb obj ref under lock (Chris)
v7: use fb object from plane_config instead (Ville)
take ref on fb object (Jesse)
v8: put under i915_fastboot option (Jesse)
fix fb ptr checking (Jesse)
inform drm_fb_helper if we fail to enable a connector (Jesse)
drop unnecessary enabled[] modifications in failure cases (Chris)
split from BIOS connector config readout (Daniel)
don't memset the fb buffer if preallocated (Chris)
alloc ifbdev up front and pass to init_bios (Chris)
check for bad ifbdev in restore_mode too (Chris)
v9: fix up !fastboot bpp setting (Jesse)
fix up !fastboot helper alloc (Jesse)
make sure BIOS fb is sufficient for biggest active pipe (Jesse)
Signed-off-by: Jesse Barnes
---
drivers/gpu/drm/i915/intel_drv.h |1 +
drivers/gpu/drm/i915/intel_fbdev.c | 123 +---
2 files changed, 115 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 8f5e798..80e6ad2 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -113,6 +113,7 @@ struct intel_fbdev {
struct intel_framebuffer *fb;
struct list_head fbdev_list;
struct drm_display_mode *our_mode;
+ int preferred_bpp;
};
struct intel_encoder {
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
b/drivers/gpu/drm/i915/intel_fbdev.c
index 8ce3405..34f99ae 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -132,6 +132,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
struct drm_framebuffer *fb;
struct drm_i915_gem_object *obj;
int size, ret;
+ bool prealloc = false;
mutex_lock(&dev->struct_mutex);
@@ -143,6 +144,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
intel_fb = ifbdev->fb;
} else {
DRM_DEBUG_KMS("re-using BIOS fb\n");
+ prealloc = true;
sizes->fb_width = intel_fb->base.width;
sizes->fb_height = intel_fb->base.height;
}
@@ -204,7 +206,7 @@ static int intelfb_create(struct drm_fb_helper *helper,
* If the object is stolen however, it will be full of whatever
* garbage was left in there.
*/
- if (ifbdev->fb->obj->stolen)
+ if (ifbdev->fb->obj->stolen && !prealloc)
memset_io(info->screen_base, 0, info->screen_size);
/* Use default scratch pixmap (info->pixmap.flags = FB_PIXMAP_SYSTEM) */
@@ -364,23 +366,124 @@ static void intel_fbdev_destroy(struct drm_device *dev,
kfree(ifbdev->fb);
}
+/*
+ * Build an intel_fbdev struct using a BIOS allocated framebuffer, if possible.
+ * The core display code will have read out the current plane configuration,
+ * so we use that to figure out if there's an object for us to use as the
+ * fb, and if so, we re-use it for the fbdev configuration.
+ *
+ * Note we only support a single fb shared across pipes for boot (mostly for
+ * fbcon), so we just find the biggest and use that.
+ */
+static bool intel_fbdev_init_bios(struct drm_device *dev,
+struct intel_fbdev *ifbdev)
+{
+ struct intel_framebuffer *fb = NULL;
+ struct drm_crtc *crtc;
+ struct intel_crtc *intel_crtc;
+ struct intel_plane_config *plane_config = NULL;
+ unsigned int last_size = 0, max_size = 0, tmp;
+
+ /* Find the largest framebuffer to use, then free the others */
+ list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
+ intel_crtc = to_intel_crtc(crtc);
+
+ if (!intel_crtc->active || !intel_crtc->plane_config.fb) {
+ DRM_DEBUG_KMS("pipe %c not active or no fb, skipping\n",
+ pipe_name(intel_crtc->pipe));
+ continue;
+ }
+
+
[Intel-gfx] [PATCH 5/6] drm/i915: allow re-use BIOS connector config for initial fbdev config
The BIOS or boot loader will generally create an initial display
configuration for us that includes some set of active pipes and
displays. This routine tries to figure out which pipes and connectors
are active and stuffs them into the crtcs and modes array given to us by
the drm_fb_helper code.
Signed-off-by: Jesse Barnes
---
drivers/gpu/drm/i915/intel_fbdev.c | 91
1 file changed, 91 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_fbdev.c
b/drivers/gpu/drm/i915/intel_fbdev.c
index fb07ba6..8ce3405 100644
--- a/drivers/gpu/drm/i915/intel_fbdev.c
+++ b/drivers/gpu/drm/i915/intel_fbdev.c
@@ -246,6 +246,97 @@ static void intel_crtc_fb_gamma_get(struct drm_crtc *crtc,
u16 *red, u16 *green,
*blue = intel_crtc->lut_b[regno] << 8;
}
+static struct drm_fb_helper_crtc *
+intel_fb_helper_crtc(struct drm_fb_helper *fb_helper, struct drm_crtc *crtc)
+{
+ int i;
+
+ for (i = 0; i < fb_helper->crtc_count; i++)
+ if (fb_helper->crtc_info[i].mode_set.crtc == crtc)
+ return &fb_helper->crtc_info[i];
+
+ return NULL;
+}
+
+/*
+ * Try to read the BIOS display configuration and use it for the initial
+ * fb configuration.
+ *
+ * The BIOS or boot loader will generally create an initial display
+ * configuration for us that includes some set of active pipes and displays.
+ * This routine tries to figure out which pipes and connectors are active
+ * and stuffs them into the crtcs and modes array given to us by the
+ * drm_fb_helper code.
+ *
+ * The overall sequence is:
+ * intel_fbdev_init - from driver load
+ * intel_fbdev_init_bios - initialize the intel_fbdev using BIOS data
+ * drm_fb_helper_init - build fb helper structs
+ * drm_fb_helper_single_add_all_connectors - more fb helper structs
+ * intel_fbdev_initial_config - apply the config
+ * drm_fb_helper_initial_config - call ->probe then register_framebuffer()
+ * drm_setup_crtcs - build crtc config for fbdev
+ * intel_fb_initial_config - find active connectors etc
+ * drm_fb_helper_single_fb_probe - set up fbdev
+ * intelfb_create - re-use or alloc fb, build out fbdev structs
+ *
+ * If the BIOS or boot loader leaves the display in VGA mode, there's not
+ * much we can do; switching out of that mode involves allocating a new,
+ * high res buffer, and also recalculating bandwidth requirements for the
+ * new bpp configuration.
+ */
+static bool intel_fb_initial_config(struct drm_fb_helper *fb_helper,
+ struct drm_fb_helper_crtc **crtcs,
+ struct drm_display_mode **modes,
+ bool *enabled, int width, int height)
+{
+ int i;
+
+ for (i = 0; i < fb_helper->connector_count; i++) {
+ struct drm_connector *connector;
+ struct drm_encoder *encoder;
+
+ connector = fb_helper->connector_info[i]->connector;
+ if (!enabled[i]) {
+ DRM_DEBUG_KMS("connector %d not enabled, skipping\n",
+ connector->base.id);
+ continue;
+ }
+
+ encoder = connector->encoder;
+ if (!encoder || !encoder->crtc) {
+ DRM_DEBUG_KMS("connector %d has no encoder or crtc,
skipping\n",
+ connector->base.id);
+ enabled[i] = false;
+ continue;
+ }
+
+ if (WARN_ON(!encoder->crtc->enabled)) {
+ DRM_DEBUG_KMS("connector %s on crtc %d has inconsistent
state, aborting\n",
+ drm_get_connector_name(connector),
+ encoder->crtc->base.id);
+ return false;
+ }
+
+ if (!to_intel_crtc(encoder->crtc)->active) {
+ DRM_DEBUG_KMS("connector %s on inactive crtc %d,
borting\n",
+ drm_get_connector_name(connector),
+ encoder->crtc->base.id);
+ return false;
+ }
+
+ modes[i] = &encoder->crtc->mode;
+ crtcs[i] = intel_fb_helper_crtc(fb_helper, encoder->crtc);
+
+ DRM_DEBUG_KMS("connector %s on crtc %d: %s\n",
+ drm_get_connector_name(connector),
+ encoder->crtc->base.id,
+ modes[i]->name);
+ }
+
+ return true;
+}
+
static struct drm_fb_helper_funcs intel_fb_helper_funcs = {
.gamma_set = intel_crtc_fb_gamma_set,
.gamma_get = intel_crtc_fb_gamma_get,
--
1.7.9.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/6] drm/i915: get_plane_config for i9xx v9
Read out the current plane configuration at init time into a new
plane_config structure. This allows us to track any existing
framebuffers attached to the plane and potentially re-use them in our
fbdev code for a smooth handoff.
v2: update for new pitch_for_width function (Jesse)
comment how get_plane_config works with shared fbs (Jesse)
v3: s/ARGB/XRGB (Ville)
use pipesrc width/height (Ville)
fix fourcc comment (Bob)
use drm_format_plane_cpp (Ville)
v4: use fb for tracking fb data object (Ville)
v5: fix up gen2 pitch limits (Ville)
v6: read out stride as well (Daniel)
v7: split out init ordering changes (Daniel)
don't fetch config if !CONFIG_FB
v8: use proper height in get_plane_config (Chris)
v9: fix CONFIG_FB check for modular configs (Jani)
Signed-off-by: Jesse Barnes
---
drivers/gpu/drm/i915/i915_drv.h |3 +
drivers/gpu/drm/i915/intel_display.c | 135 ++
drivers/gpu/drm/i915/intel_drv.h |8 ++
3 files changed, 146 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 728b9c3..bde0b47 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -393,6 +393,7 @@ struct drm_i915_error_state {
struct intel_connector;
struct intel_crtc_config;
+struct intel_plane_config;
struct intel_crtc;
struct intel_limit;
struct dpll;
@@ -431,6 +432,8 @@ struct drm_i915_display_funcs {
* fills out the pipe-config with the hw state. */
bool (*get_pipe_config)(struct intel_crtc *,
struct intel_crtc_config *);
+ void (*get_plane_config)(struct intel_crtc *,
+struct intel_plane_config *);
int (*crtc_mode_set)(struct drm_crtc *crtc,
int x, int y,
struct drm_framebuffer *old_fb);
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index e2f1db6..2eb1108 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2033,6 +2033,27 @@ unsigned long intel_gen4_compute_page_offset(int *x, int
*y,
}
}
+int intel_format_to_fourcc(int format)
+{
+ switch (format) {
+ case DISPPLANE_8BPP:
+ return DRM_FORMAT_C8;
+ case DISPPLANE_BGRX555:
+ return DRM_FORMAT_XRGB1555;
+ case DISPPLANE_BGRX565:
+ return DRM_FORMAT_RGB565;
+ default:
+ case DISPPLANE_BGRX888:
+ return DRM_FORMAT_XRGB;
+ case DISPPLANE_RGBX888:
+ return DRM_FORMAT_XBGR;
+ case DISPPLANE_BGRX101010:
+ return DRM_FORMAT_XRGB2101010;
+ case DISPPLANE_RGBX101010:
+ return DRM_FORMAT_XBGR2101010;
+ }
+}
+
static int i9xx_update_plane(struct drm_crtc *crtc, struct drm_framebuffer *fb,
int x, int y)
{
@@ -5517,6 +5538,96 @@ static void vlv_crtc_clock_get(struct intel_crtc *crtc,
pipe_config->port_clock = clock.dot / 5;
}
+static void i9xx_get_plane_config(struct intel_crtc *crtc,
+ struct intel_plane_config *plane_config)
+{
+ struct drm_device *dev = crtc->base.dev;
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct drm_i915_gem_object *obj = NULL;
+ struct drm_mode_fb_cmd2 mode_cmd = { 0 };
+ u32 val, base, offset;
+ int pipe = crtc->pipe, plane = crtc->plane;
+ int fourcc, pixel_format;
+ int aligned_height;
+
+ plane_config->fb = kzalloc(sizeof(*plane_config->fb), GFP_KERNEL);
+ if (!plane_config->fb) {
+ DRM_DEBUG_KMS("failed to alloc fb\n");
+ return;
+ }
+
+ val = I915_READ(DSPCNTR(plane));
+
+ if (INTEL_INFO(dev)->gen >= 4)
+ if (val & DISPPLANE_TILED)
+ plane_config->tiled = true;
+
+ pixel_format = val & DISPPLANE_PIXFORMAT_MASK;
+ fourcc = intel_format_to_fourcc(pixel_format);
+ plane_config->fb->base.pixel_format = fourcc;
+ plane_config->fb->base.bits_per_pixel =
+ drm_format_plane_cpp(fourcc, 0) * 8;
+
+ if (INTEL_INFO(dev)->gen >= 4) {
+ if (plane_config->tiled)
+ offset = I915_READ(DSPTILEOFF(plane));
+ else
+ offset = I915_READ(DSPLINOFF(plane));
+ base = I915_READ(DSPSURF(plane)) & 0xf000;
+ } else {
+ base = I915_READ(DSPADDR(plane));
+ }
+
+ val = I915_READ(PIPESRC(pipe));
+ plane_config->fb->base.width = ((val >> 16) & 0xfff) + 1;
+ plane_config->fb->base.height = ((val >> 0) & 0xfff) + 1;
+
+ val = I915_READ(DSPSTRIDE(pipe));
+ plane_config->fb->base.pitches[0] = val & 0xff80;
+
+ aligned_height = intel_align_height(dev, plane_config->fb->base.height,
+
Re: [Intel-gfx] [PATCH 5/7] drm/i915: pass status instead of enable flags to i915_enable_pipestat
On Wed, Feb 05, 2014 at 05:12:39PM +0100, Daniel Vetter wrote: > On Wed, Feb 05, 2014 at 03:35:15PM +, Jesse Barnes wrote: > > I almost think we should just separate enable vs status entirely. As > > long as the bits are named consistently it may be easier to follow (as > > Ville found in your next patch with the subtle remapping of status > > bits). > > Yeah, I think for cases where the hw engineers just made a mess of it it's > better to be explicit. So what about keeping the current pipestat > enable/disable functions as wrappers which assume a regular mapping > betweeen status and mask bit, and then add a low-level function which > takes both mask and status explicitly? > > That way we have less churn in the code, mostly pipestat enable/disable > still looks sane but the irregular cases will really stick out. For a name > I'd just go with __i915_enable_pipestat for lack of better ideas. Or maybe > i915_enable_pipestat_irregular. That could lead to someone accidentally using the regular function when they should be using the irregular one and then we have some weird bug on our hands. I rather like keeping the mess in one central place. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Request for feedback - Sprite flip notification support
On Wed, Feb 05, 2014 at 09:25:36PM +0530, Vijay Purushothaman wrote: > On 2/5/2014 8:43 PM, Ville Syrjälä wrote: > > On Wed, Feb 05, 2014 at 08:35:11PM +0530, Vijay Purushothaman wrote: > >> Hello, > >> > >> In our current driver implementation we support flip notifications only > >> for primary plane. So, in a full screen video playback scenario where > >> only one sprite plane is active, the user space is forced to rely on > >> primary plane flip notification even though there is no real need for > >> this plane to be active. Ideally we should be able to support flip > >> notifications for any given plane. Switching off the primary plane (when > >> not used) will help in better memory self refresh & decent power savings.. > >> > >> We do have a hack in android product trees which supports flip > >> notifications for one sprite plane. unfortunately this hack in its > >> current form cannot be considered for up streaming... > >> > >> My current thinking is to have an array of unpin_work items to match the > >> number of planes. Is anyone working on this or thought about this > >> scenario in detail? Any pointers / restrictions that needs to considered > >> for a generic implementation of this feature? > > > > The plan is to implement the nuclear page flip which will take care of > > all planes in the same way. > > > Thanks Ville. If the nuclear page flip is part of your bigger atomic > mode set framework, is there a way you can split this into smaller sets > for merge? Multiple product trees will benefit from the nuclear page flip. I've split things up already somewhat. Some has landed some has not. Currently I have my minimal "atomic update of sprite+primary during setplane" series I need to get in. It shouldn't need major work anymore, just some minor tweaks. But I realized I need to limit this to just pch platforms for now. Making it work reliably on gmch platforms require some extra interrupt related work. The main thing here is that it adds the mechanism to do the update atomically for the entire pipe. After that I need to post the last bits of my watermark update saga. This too will initially be limited to pch platforms only. Obviously watermarks need to updated correctly to avoid underruns when planes get shuffled around. > Is there anything that i can help with? Like testing your patches with > android user space? There's nothing to test at this point unless you want to test my old branch from year ago or something. What needs to be done: - review the latest atomic framework patches from Rob Clark - expose primary/cursor planes as drm_planes * this could in theory be skipped, but it'll lead to cruft in the API we need to maintain until the end of time. Also I think restructing stuff internally to this direction will be a good idea anyway to make the nuclear flip code neater. This more or less involves collecting the plane state to some plane_config type of structure, and being able to pre-compute that - try to collect the necessary missing bits from my last atomic branch to implement the nuclear flip * the flip helper thing to update an arbitrary collection of planes atomically, maybe could be simplified a bit * mechanism to queue nuclear flips and make them wait for the GPU to finish writing to all the relevant buffers before issuing the actual flips/updates - finally hook up the atomic ioctl to do the nuclear flip * pre-pin all buffers, pre-compute plane configs, pre-compute watermarks, check everything, wait for the GPU, and finally do the update For the atomic modeset side some of that's the same really. There too we also need to pre-compute the plane configs and pre-pin buffers. Most of the rest we already pre-compute via the pipe config. One major thing left out of the pipe config pre-compute currently is PLLs. We compute that stuff way too late still. We also need to massage the modeset code some more to make it capable of modesetting multiple pipes at once. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Record pid/comm of hanging task
After finding the guilty batch and request, we can use it to find the
process that submitted the batch and then add the culprit into the error
state.
This is a slightly different approach from Ben's in that instead of
adding the extra information into the struct i915_hw_context, we use the
information already captured in struct drm_file which is then referenced
from the request.
Link: http://lists.freedesktop.org/archives/intel-gfx/2013-August/032280.html
Signed-off-by: Chris Wilson
Cc: Ben Widawsky
---
drivers/gpu/drm/i915/i915_drv.h | 4 ++
drivers/gpu/drm/i915/i915_gem.c | 1 +
drivers/gpu/drm/i915/i915_gpu_error.c | 69 ++-
3 files changed, 49 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1d317f8c9f05..2007062e4a35 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -374,6 +374,9 @@ struct drm_i915_error_state {
u32 pp_dir_base;
};
} vm_info;
+
+ pid_t pid;
+ char comm[TASK_COMM_LEN];
} ring[I915_NUM_RINGS];
struct drm_i915_error_buffer {
@@ -1825,6 +1828,7 @@ struct drm_i915_gem_request {
struct drm_i915_file_private {
struct drm_i915_private *dev_priv;
+ struct drm_file *file;
struct {
spinlock_t lock;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 157877bd7a0b..605e4ab636e8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4981,6 +4981,7 @@ int i915_gem_open(struct drm_device *dev, struct drm_file
*file)
file->driver_priv = file_priv;
file_priv->dev_priv = dev->dev_private;
+ file_priv->file = file;
spin_lock_init(&file_priv->mm.lock);
INIT_LIST_HEAD(&file_priv->mm.request_list);
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c
b/drivers/gpu/drm/i915/i915_gpu_error.c
index ea588c66ffc4..386bf52dd16a 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -309,6 +309,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf
*m,
drm_i915_private_t *dev_priv = dev->dev_private;
struct drm_i915_error_state *error = error_priv->error;
int i, j, page, offset, elt;
+ int max_hangcheck_score;
if (!error) {
err_printf(m, "no error state collected\n");
@@ -318,6 +319,20 @@ int i915_error_state_to_str(struct
drm_i915_error_state_buf *m,
err_printf(m, "Time: %ld s %ld us\n", error->time.tv_sec,
error->time.tv_usec);
err_printf(m, "Kernel: " UTS_RELEASE "\n");
+ max_hangcheck_score = 0;
+ for (i = 0; i < ARRAY_SIZE(error->ring); i++) {
+ if (error->ring[i].hangcheck_score > max_hangcheck_score)
+ max_hangcheck_score = error->ring[i].hangcheck_score;
+ }
+ for (i = 0; i < ARRAY_SIZE(error->ring); i++) {
+ if (error->ring[i].hangcheck_score == max_hangcheck_score &&
+ error->ring[i].pid != -1) {
+ err_printf(m, "Active process (on ring %s): %s [%d]\n",
+ ring_str(i),
+ error->ring[i].comm,
+ error->ring[i].pid);
+ }
+ }
err_printf(m, "PCI ID: 0x%04x\n", dev->pdev->device);
err_printf(m, "EIR: 0x%08x\n", error->eir);
err_printf(m, "IER: 0x%08x\n", error->ier);
@@ -363,8 +378,11 @@ int i915_error_state_to_str(struct
drm_i915_error_state_buf *m,
struct drm_i915_error_object *obj;
if ((obj = error->ring[i].batchbuffer)) {
- err_printf(m, "%s --- gtt_offset = 0x%08x\n",
- dev_priv->ring[i].name,
+ err_puts(m, dev_priv->ring[i].name);
+ if (error->ring[i].pid != -1)
+ err_printf(m, " (submitted by %s [%d])",
+ error->ring[i].comm,
error->ring[i].pid);
+ err_printf(m, " --- gtt_offset = 0x%08x\n",
obj->gtt_offset);
offset = 0;
for (page = 0; page < obj->page_count; page++) {
@@ -698,9 +716,9 @@ static void i915_gem_record_fences(struct drm_device *dev,
}
}
-static struct drm_i915_error_object *
-i915_error_first_batchbuffer(struct drm_i915_private *dev_priv,
-struct intel_ring_buffer *ring)
+static struct drm_i915_gem_request *
+i915_error_first_request(struct drm_i915_private *dev_priv,
+struct intel_ring_buffer *ring)
{
struct drm_i915_gem_request *request;
u32 seqno;
@@ -708,29 +726,12 @@ i915_error_first_batchbuf
Re: [Intel-gfx] [PATCH] [RFC] drm/i915: Generate a hang error code
On Wed, Feb 5, 2014 at 5:18 PM, Daniel Vetter wrote: > I'd still like to see the same information > dumped into the error state though. This was re: Jesse's idea on irc to dump pid/comm, too. But adding the same gpu hang cookie computation to the error state decoder might still make sense. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] [RFC] drm/i915: Generate a hang error code
On Wed, Feb 05, 2014 at 04:03:45PM +, Jesse Barnes wrote: > On Wed, 5 Feb 2014 16:15:02 +0100 > Daniel Vetter wrote: > > > On Wed, Feb 05, 2014 at 02:59:08PM +, Jesse Barnes wrote: > > > On Tue, 4 Feb 2014 12:18:55 + > > > Ben Widawsky wrote: > > > > > > > We get a large number of bugs which have a, "hey I have that too" > > > > because they see a GPU hang in dmesg. While two machines of the same > > > > model having a GPU hang is indeed a coincidence, it is far from enough > > > > evidence to suggest they are the same. > > > > > > > > In order to reduce this effect, and hopefully get people to file new bug > > > > reports, clearly the error message itself has been insufficient (see ref > > > > at the bottom for a new bug report with this characteristic). > > > > > > > > The algorithm is purposely pretty naive. I don't think we need much in > > > > order to avoid the problem I am trying to solve, and keeping it naive > > > > gives us some ability to make a decent test case. > > > > > > I like the direction of this. If we can get some basic info into the > > > dmesg part of things (the only part regular users will actually look > > > at) we can probably avoid some of the "me too" action we see on general > > > GPU hangs. Having PID, comm, and some sort of hang signature are all > > > good steps in that direction imo. > > > > tbh I don't see much value in regular users trying to triage gpu hang. If > > they're not damn sure that they have a dupe (which means same platform, > > versions of the software stack and crashing games) I much prefer if they > > just send in a duplicate bug for us to triage. > > > > With the mis-design of bugzilla it's much harder to untangle a wrong > > me-too than mark something as duplicate. And especially long-running bugs > > are a royal pain if there's too much wrong me-too noise in there. > > > > Not a comment on the patch itself, just a general comment wrt avoiding > > me-too gpu hang reports. > > So you're saying the GPU error decode tool should create a bug template > for people so we don't get the "me too" reports? > > What I see above is that it's really important to avoid the "me too" > stuff, and to do it in such a way that false positives are minimized > (e.g. the IPEHR bit Ubuntu used to use). So I guess I don't see what's > unconvincing here. Today we have no way of differentiating w/o digging > in to the error record, which users definitely won't do, and this patch > seems like it could only help with that... so count me confused. We have a full paragraph explaining to users exactly what they need to do. They still me-too and fail to attach the error state. I don't how adding even more helps, since it never really did. Anyway, patch merged since meh. I'd still like to see the same information dumped into the error state though. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/7] drm/i915: pass status instead of enable flags to i915_enable_pipestat
On Wed, Feb 05, 2014 at 03:35:15PM +, Jesse Barnes wrote: > I almost think we should just separate enable vs status entirely. As > long as the bits are named consistently it may be easier to follow (as > Ville found in your next patch with the subtle remapping of status > bits). Yeah, I think for cases where the hw engineers just made a mess of it it's better to be explicit. So what about keeping the current pipestat enable/disable functions as wrappers which assume a regular mapping betweeen status and mask bit, and then add a low-level function which takes both mask and status explicitly? That way we have less churn in the code, mostly pipestat enable/disable still looks sane but the irregular cases will really stick out. For a name I'd just go with __i915_enable_pipestat for lack of better ideas. Or maybe i915_enable_pipestat_irregular. Merged the patches thus far in this series to dinq. Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] [RFC] drm/i915: Generate a hang error code
On Wed, 5 Feb 2014 16:15:02 +0100 Daniel Vetter wrote: > On Wed, Feb 05, 2014 at 02:59:08PM +, Jesse Barnes wrote: > > On Tue, 4 Feb 2014 12:18:55 + > > Ben Widawsky wrote: > > > > > We get a large number of bugs which have a, "hey I have that too" > > > because they see a GPU hang in dmesg. While two machines of the same > > > model having a GPU hang is indeed a coincidence, it is far from enough > > > evidence to suggest they are the same. > > > > > > In order to reduce this effect, and hopefully get people to file new bug > > > reports, clearly the error message itself has been insufficient (see ref > > > at the bottom for a new bug report with this characteristic). > > > > > > The algorithm is purposely pretty naive. I don't think we need much in > > > order to avoid the problem I am trying to solve, and keeping it naive > > > gives us some ability to make a decent test case. > > > > I like the direction of this. If we can get some basic info into the > > dmesg part of things (the only part regular users will actually look > > at) we can probably avoid some of the "me too" action we see on general > > GPU hangs. Having PID, comm, and some sort of hang signature are all > > good steps in that direction imo. > > tbh I don't see much value in regular users trying to triage gpu hang. If > they're not damn sure that they have a dupe (which means same platform, > versions of the software stack and crashing games) I much prefer if they > just send in a duplicate bug for us to triage. > > With the mis-design of bugzilla it's much harder to untangle a wrong > me-too than mark something as duplicate. And especially long-running bugs > are a royal pain if there's too much wrong me-too noise in there. > > Not a comment on the patch itself, just a general comment wrt avoiding > me-too gpu hang reports. So you're saying the GPU error decode tool should create a bug template for people so we don't get the "me too" reports? What I see above is that it's really important to avoid the "me too" stuff, and to do it in such a way that false positives are minimized (e.g. the IPEHR bit Ubuntu used to use). So I guess I don't see what's unconvincing here. Today we have no way of differentiating w/o digging in to the error record, which users definitely won't do, and this patch seems like it could only help with that... so count me confused. Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Introduce mapping of user pages into video memory (userptr) ioctl
On Tue, 4 Feb 2014 11:56:47 +0100 Daniel Vetter wrote: > On Mon, Feb 03, 2014 at 03:28:37PM +, Tvrtko Ursulin wrote: > > > > On 01/29/2014 08:34 PM, Daniel Vetter wrote: > > >Actually I've found something else to complain about: > > > > > >On Tue, Jan 28, 2014 at 2:16 PM, Chris Wilson > > >wrote: > > >>+#define I915_USERPTR_READ_ONLY 0x1 > > > > > >This smells like an insta-root-exploit: > > >1. mmap /lib/ld-linux.so as read-only > > >2. userptr bind that mmap'ed area as READ_ONLY > > >3. blit exploit code over it > > >4. profit > > > > > >I also don't see a way we could fix this, at least without the > > >hardware providing read-only modes in the ptes. Which also requires us > > >to actually trust it to follow them, even when they exists ... > > > > Would disallowing mapping of shared pages help and be acceptable > > considering intended use cases? > > The above exploit is the simplest one I could come up with, but I expect > the vm in general won't be too happy if we write to pages it never expects > are written to. We could do fun stuff like corrupt pagecache or swap > cache. Which in conjunction with stable kernel pages (which some I/O paths > needed) is rather likely to result in havoc. > > Essentially I'm no vm expert, and this definitely needs a full vm audit > even before considering it at all. So I'd like to drop support for it in > the initial version ... Yeah I think we'd need to only allow this usage for root (i.e. you get to keep both pieces) or for platforms where we actually have RW[X] GTT control (e.g. BDW). A shared mapping restriction *might* be sufficient, but like Daniel said, the real fix is to properly handle the PROT_* bits... Seems like it could be kind of a cool feature though, so we should try to enable it on BDW+. Jesse Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] drm/i915: factor out valleyview_pipestat_irq_handler
On Tue, Feb 04, 2014 at 09:35:46PM +0200, Imre Deak wrote:
> This will be used by other platforms too, so factor it out.
>
> The only functional change is the reordeing of gmbus_irq_handler() wrt.
> the hotplug handling, but since it only schedules a work, it isn't an
> issue.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_irq.c | 76
> +++--
> 1 file changed, 42 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 137ac65..b5524ea 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1477,15 +1477,53 @@ static void gen6_rps_irq_handler(struct
> drm_i915_private *dev_priv, u32 pm_iir)
> }
> }
>
> +static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
> +{
> + drm_i915_private_t *dev_priv = dev->dev_private;
typedefs for structs are frowned upon. I've fixed this while applying.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Request for feedback - Sprite flip notification support
On 2/5/2014 8:43 PM, Ville Syrjälä wrote: On Wed, Feb 05, 2014 at 08:35:11PM +0530, Vijay Purushothaman wrote: Hello, In our current driver implementation we support flip notifications only for primary plane. So, in a full screen video playback scenario where only one sprite plane is active, the user space is forced to rely on primary plane flip notification even though there is no real need for this plane to be active. Ideally we should be able to support flip notifications for any given plane. Switching off the primary plane (when not used) will help in better memory self refresh & decent power savings.. We do have a hack in android product trees which supports flip notifications for one sprite plane. unfortunately this hack in its current form cannot be considered for up streaming... My current thinking is to have an array of unpin_work items to match the number of planes. Is anyone working on this or thought about this scenario in detail? Any pointers / restrictions that needs to considered for a generic implementation of this feature? The plan is to implement the nuclear page flip which will take care of all planes in the same way. Thanks Ville. If the nuclear page flip is part of your bigger atomic mode set framework, is there a way you can split this into smaller sets for merge? Multiple product trees will benefit from the nuclear page flip. Is there anything that i can help with? Like testing your patches with android user space? Thanks, Vijay ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/7] drm/i915: pass status instead of enable flags to i915_enable_pipestat
On Tue, 4 Feb 2014 21:35:49 +0200
Imre Deak wrote:
> There isn't any PSR interrupt enable bit for pipe A, so we couldn't
> enable it through the current API. Passing the corresponding status bits
> solves this and also makes the mapping between enable and status bits
> simpler on VLV (addressed in an upcoming patch).
>
> Except of checking for invalid status bit arguments, no functional
> change.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.h | 6 ++--
> drivers/gpu/drm/i915/i915_irq.c | 68
> +
> drivers/gpu/drm/i915/i915_reg.h | 3 ++
> drivers/gpu/drm/i915/intel_tv.c | 8 ++---
> 4 files changed, 53 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index fa37dfd..43f37ca 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1994,10 +1994,12 @@ extern void intel_uncore_check_errors(struct
> drm_device *dev);
> extern void intel_uncore_fini(struct drm_device *dev);
>
> void
> -i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask);
> +i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> + u32 status_mask);
>
> void
> -i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32
> mask);
> +i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> + u32 status_mask);
>
> /* i915_gem.c */
> int i915_gem_init_ioctl(struct drm_device *dev, void *data,
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 3b876ee..ec56a70 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -471,36 +471,52 @@ done:
> return ret;
> }
>
> +static u32 pipestat_to_enable_mask(struct drm_device *dev, u32 status_mask)
> +{
> + return status_mask << 16;
> +}
>
> void
> -i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask)
> +i915_enable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> + u32 status_mask)
> {
> u32 reg = PIPESTAT(pipe);
> - u32 pipestat = I915_READ(reg) & 0x7fff;
> + u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> + u32 enable_mask;
>
> assert_spin_locked(&dev_priv->irq_lock);
>
> - if ((pipestat & mask) == mask)
> + if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK))
> + return;
> +
> + enable_mask = pipestat_to_enable_mask(dev_priv->dev, status_mask);
> + if ((pipestat & enable_mask) == enable_mask)
> return;
>
> /* Enable the interrupt, clear any pending status */
> - pipestat |= mask | (mask >> 16);
> + pipestat |= enable_mask | status_mask;
> I915_WRITE(reg, pipestat);
> POSTING_READ(reg);
> }
>
> void
> -i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe, u32 mask)
> +i915_disable_pipestat(drm_i915_private_t *dev_priv, enum pipe pipe,
> + u32 status_mask)
> {
> u32 reg = PIPESTAT(pipe);
> - u32 pipestat = I915_READ(reg) & 0x7fff;
> + u32 pipestat = I915_READ(reg) & PIPESTAT_INT_ENABLE_MASK;
> + u32 enable_mask;
>
> assert_spin_locked(&dev_priv->irq_lock);
>
> - if ((pipestat & mask) == 0)
> + if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK))
> return;
>
> - pipestat &= ~mask;
> + enable_mask = pipestat_to_enable_mask(dev_priv->dev, status_mask);
> + if ((pipestat & enable_mask) == 0)
> + return;
> +
> + pipestat &= ~enable_mask;
> I915_WRITE(reg, pipestat);
> POSTING_READ(reg);
> }
> @@ -518,10 +534,10 @@ static void i915_enable_asle_pipestat(struct drm_device
> *dev)
>
> spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
>
> - i915_enable_pipestat(dev_priv, PIPE_B, PIPE_LEGACY_BLC_EVENT_ENABLE);
> + i915_enable_pipestat(dev_priv, PIPE_B, PIPE_LEGACY_BLC_EVENT_STATUS);
> if (INTEL_INFO(dev)->gen >= 4)
> i915_enable_pipestat(dev_priv, PIPE_A,
> - PIPE_LEGACY_BLC_EVENT_ENABLE);
> + PIPE_LEGACY_BLC_EVENT_STATUS);
>
> spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> }
> @@ -2270,10 +2286,10 @@ static int i915_enable_vblank(struct drm_device *dev,
> int pipe)
> spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> if (INTEL_INFO(dev)->gen >= 4)
> i915_enable_pipestat(dev_priv, pipe,
> - PIPE_START_VBLANK_INTERRUPT_ENABLE);
> + PIPE_START_VBLANK_INTERRUPT_STATUS);
> else
> i915_enable_pipestat(dev_priv, pipe,
> - PIPE_VBLANK_INTERRUPT_ENABLE);
> + PIPE_VBLANK_INTERRUPT_STATUS);
>
> /* maintain vblank delivery even in deep C-states */
>
Re: [Intel-gfx] [PATCH 11/13] drm/i915: Reject commands that would store to global HWS page
On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote:
> From: Brad Volkin
>
> PIPE_CONTROL and MI_FLUSH_DW have bits that would write to the
> hardware status page. The driver stores request tracking info
> there, so don't let userspace overwrite it.
>
> Signed-off-by: Brad Volkin
> ---
> drivers/gpu/drm/i915/i915_cmd_parser.c | 30 ++
> drivers/gpu/drm/i915/i915_reg.h| 1 +
> 2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 26072a2..b93df1c 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -141,7 +141,8 @@ static const struct drm_i915_cmd_descriptor render_cmds[]
> = {
> },
> {
> .offset = 1,
> - .mask = PIPE_CONTROL_GLOBAL_GTT_IVB,
> + .mask = (PIPE_CONTROL_GLOBAL_GTT_IVB |
> + PIPE_CONTROL_STORE_DATA_INDEX),
> .expected = 0,
> .condition_offset = 1,
> .condition_mask = PIPE_CONTROL_POST_SYNC_OP_MASK
> @@ -192,8 +193,15 @@ static const struct drm_i915_cmd_descriptor video_cmds[]
> = {
> .expected = 0,
> .condition_offset = 0,
> .condition_mask = MI_FLUSH_DW_OP_MASK
> + },
> + {
> + .offset = 0,
> + .mask = MI_FLUSH_DW_STORE_INDEX,
> + .expected = 0,
> + .condition_offset = 0,
> + .condition_mask = MI_FLUSH_DW_OP_MASK
> }},
> - .bits_count = 2 ),
> + .bits_count = 3 ),
I'm disliking this separate .bits_count more at every change...
> CMD( MI_CONDITIONAL_BATCH_BUFFER_END, SMI, !F, 0xFF, B,
> .bits = {{
> .offset = 0,
> @@ -231,8 +239,15 @@ static const struct drm_i915_cmd_descriptor vecs_cmds[]
> = {
> .expected = 0,
> .condition_offset = 0,
> .condition_mask = MI_FLUSH_DW_OP_MASK
> + },
> + {
> + .offset = 0,
> + .mask = MI_FLUSH_DW_STORE_INDEX,
> + .expected = 0,
> + .condition_offset = 0,
> + .condition_mask = MI_FLUSH_DW_OP_MASK
> }},
> - .bits_count = 2 ),
> + .bits_count = 3 ),
> CMD( MI_CONDITIONAL_BATCH_BUFFER_END, SMI, !F, 0xFF, B,
> .bits = {{
> .offset = 0,
> @@ -264,8 +279,15 @@ static const struct drm_i915_cmd_descriptor blt_cmds[] =
> {
> .expected = 0,
> .condition_offset = 0,
> .condition_mask = MI_FLUSH_DW_OP_MASK
> + },
> + {
> + .offset = 0,
> + .mask = MI_FLUSH_DW_STORE_INDEX,
> + .expected = 0,
> + .condition_offset = 0,
> + .condition_mask = MI_FLUSH_DW_OP_MASK
> }},
> - .bits_count = 2 ),
> + .bits_count = 3 ),
> CMD( COLOR_BLT,S2D, !F, 0x3F, S ),
> CMD( SRC_COPY_BLT, S2D, !F, 0x3F, S ),
> };
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index ff263f4..5f77cb6 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -324,6 +324,7 @@
> #define GFX_OP_PIPE_CONTROL(len) ((0x3<<29)|(0x3<<27)|(0x2<<24)|(len-2))
> #define PIPE_CONTROL_GLOBAL_GTT_IVB(1<<24) /*
> gen7+ */
> #define PIPE_CONTROL_MMIO_WRITE(1<<23)
> +#define PIPE_CONTROL_STORE_DATA_INDEX (1<<21)
> #define PIPE_CONTROL_CS_STALL (1<<20)
> #define PIPE_CONTROL_TLB_INVALIDATE(1<<18)
> #define PIPE_CONTROL_QW_WRITE (1<<14)
> --
> 1.8.5.2
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 00/13] Gen7 batch buffer command parser
FYI, I did an initial review "sweep" of this. Will focus more on the logic and registers etc. next. BR, Jani. On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote: > From: Brad Volkin > > Certain OpenGL features (e.g. transform feedback, performance monitoring) > require userspace code to submit batches containing commands such as > MI_LOAD_REGISTER_IMM to access various registers. Unfortunately, some > generations of the hardware will noop these commands in "unsecure" batches > (which includes all userspace batches submitted via i915) even though the > commands may be safe and represent the intended programming model of the > device. > > This series introduces a software command parser similar in operation to the > command parsing done in hardware for unsecure batches. However, the software > parser allows some operations that would be noop'd by hardware, if the parser > determines the operation is safe, and submits the batch as "secure" to prevent > hardware parsing. Currently the series implements this on IVB and HSW. > > The series has one piece of prep work, one patch for the parser logic, and a > handful of patches to fill out the tables which drive the parser. There are > follow-up patches to libdrm and to i-g-t. The i-g-t tests are basic and do not > test all of the commands used by the parser on the assumption that I'm likely > to make the same mistakes in both the parser and the test. > > WARNING!!! > I've previously run the i-g-t gem_* tests, the piglit quick tests, and > generally > used Ubuntu 13.10 IVB and HSW systems with the parser running. Aside from a > failure described below, I did not see any regressions. However, the series > currently hits a BUG_ON() if you enable the parser due to a regression in > secure > batch handling on -nightly. > > At this point there are a couple of required/potential improvements. > > 1) Chained batches. The parser currently allows MI_BATCH_BUFFER_START commands >in userspace batches without parsing them. The media driver uses chained >batches, so a solution is required. I'm still working through the >requirements but don't want to continue delaying the review process for > what >I have so far. > 2) Command buffer copy. To avoid CPU modifications to buffers after parsing, > and >to avoid GPU modifications to buffers via EUs or commands in the batch, we >should copy the userspace batch buffer to memory that userspace does not >have access to, map it into GGTT, and execute that batch buffer. I have a >sense of how to do this for 1st-level batches, but it may need changes to >tie in with the chained batch parsing, so I've again held off. > 3) Coherency. I've found a coherency issue on VLV when reading the batch > buffer >from the CPU during execbuffer2. Userspace writes the batch via pwrite fast >path before calling execbuffer2. The parser reads stale data. This works > fine >on IVB and HSW, so I believe it's an LLC vs. non-LLC issue. I'm just > unclear >on what the correct flushing or synchronization is for this scenario. This >only matters if we get PPGTT working on VLV and enable the parser there. > > v2: > - Significantly reorder series > - Scan secure batches (i.e. I915_EXEC_SECURE) > - Check that parser tables are sorted during init > - Fixed gem_cpu_reloc regression > - HAS_CMD_PARSER -> CMD_PARSER_VERSION getparam > - Additional tests > > Brad Volkin (13): > drm/i915: Refactor shmem pread setup > drm/i915: Implement command buffer parsing logic > drm/i915: Initial command parser table definitions > drm/i915: Reject privileged commands > drm/i915: Allow some privileged commands from master > drm/i915: Add register whitelists for mesa > drm/i915: Add register whitelist for DRM master > drm/i915: Enable register whitelist checks > drm/i915: Reject commands that explicitly generate interrupts > drm/i915: Enable PPGTT command parser checks > drm/i915: Reject commands that would store to global HWS page > drm/i915: Add a CMD_PARSER_VERSION getparam > drm/i915: Enable command parsing by default > > drivers/gpu/drm/i915/Makefile | 3 +- > drivers/gpu/drm/i915/i915_cmd_parser.c | 845 > + > drivers/gpu/drm/i915/i915_dma.c| 4 + > drivers/gpu/drm/i915/i915_drv.h| 103 > drivers/gpu/drm/i915/i915_gem.c| 48 +- > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 17 + > drivers/gpu/drm/i915/i915_params.c | 5 + > drivers/gpu/drm/i915/i915_reg.h| 78 +++ > drivers/gpu/drm/i915/intel_ringbuffer.c| 2 + > drivers/gpu/drm/i915/intel_ringbuffer.h| 32 ++ > include/uapi/drm/i915_drm.h| 1 + > 11 files changed, 1123 insertions(+), 15 deletions(-) > create mode 100644 drivers/gpu/drm/i915/i915_cmd_parser.c > > -- > 1.8.5.2 > > ___ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > htt
Re: [Intel-gfx] [PATCH 10/13] drm/i915: Enable PPGTT command parser checks
On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote:
> From: Brad Volkin
>
> Various commands that access memory have a bit to determine whether
> the graphics address specified in the command should use the GGTT or
> PPGTT for translation. These checks ensure that the bit indicates
> PPGTT translation.
>
> Most of these checks use the existing bit-checking infrastructure.
> The PIPE_CONTROL and MI_FLUSH_DW commands, however, are multi-function
> commands. The GGTT/PPGTT bit is only relevant for certain uses of the
> command. As such, this change also extends the bit-checking code to
> include a "condition" mask and offset. If the condition mask is non-zero
> then the parser only performs the bit check when the bits specified by
> the condition mask/offset are also non-zero.
>
> NOTE: At this point in the series PPGTT must be enabled for the parser
> to work correctly. If it's not enabled, userspace will not be setting
> the PPGTT bits the way the parser requires. VLV is the only platform
> where this is a problem, so at this point, we disable parsing for VLV.
>
> OTC-Tracker: AXIA-4631
> Change-Id: I3f4c76b6734f1956ec47e698230f97d0998ff92b
> Signed-off-by: Brad Volkin
> ---
> drivers/gpu/drm/i915/i915_cmd_parser.c | 147
> +
> drivers/gpu/drm/i915/i915_drv.h| 6 ++
> drivers/gpu/drm/i915/i915_reg.h| 6 ++
> 3 files changed, 144 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 7de7c6a..26072a2 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -65,10 +65,22 @@ static const struct drm_i915_cmd_descriptor common_cmds[]
> = {
> CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, R ),
> CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
> .reg = { .offset = 1, .mask = 0x007C } ),
> - CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, W,
> - .reg = { .offset = 1, .mask = 0x007C } ),
> - CMD( MI_LOAD_REGISTER_MEM, SMI, !F, 0xFF, W,
> - .reg = { .offset = 1, .mask = 0x007C } ),
> + CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, W | B,
> + .reg = { .offset = 1, .mask = 0x007C },
> + .bits = {{
> + .offset = 0,
> + .mask = MI_GLOBAL_GTT,
> + .expected = 0
Not specific to this patch or this field, but all around I think you
should add the comma to the last line too. It's a pretty universal way
of doing things in the kernel, both for array and struct initialization.
> + }},
> + .bits_count = 1 ),
> + CMD( MI_LOAD_REGISTER_MEM, SMI, !F, 0xFF, W | B,
> + .reg = { .offset = 1, .mask = 0x007C },
> + .bits = {{
> + .offset = 0,
> + .mask = MI_GLOBAL_GTT,
> + .expected = 0
> + }},
> + .bits_count = 1 ),
> CMD( MI_BATCH_BUFFER_START,SMI, !F, 0xFF, S ),
> };
>
> @@ -80,9 +92,35 @@ static const struct drm_i915_cmd_descriptor render_cmds[]
> = {
> CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, R ),
> CMD( MI_SET_CONTEXT, SMI, !F, 0xFF, R ),
> CMD( MI_URB_CLEAR, SMI, !F, 0xFF, S ),
> + CMD( MI_STORE_DWORD_IMM, SMI, !F, 0x3F, B,
> + .bits = {{
> + .offset = 0,
> + .mask = MI_GLOBAL_GTT,
> + .expected = 0
> + }},
> + .bits_count = 1 ),
> CMD( MI_UPDATE_GTT,SMI, !F, 0xFF, R ),
> - CMD( MI_CLFLUSH, SMI, !F, 0x3FF, S ),
> - CMD( MI_CONDITIONAL_BATCH_BUFFER_END, SMI, !F, 0xFF, S ),
> + CMD( MI_CLFLUSH, SMI, !F, 0x3FF, B,
> + .bits = {{
> + .offset = 0,
> + .mask = MI_GLOBAL_GTT,
> + .expected = 0
> + }},
> + .bits_count = 1 ),
> + CMD( MI_REPORT_PERF_COUNT, SMI, !F, 0x3F, B,
> + .bits = {{
> + .offset = 1,
> + .mask = MI_REPORT_PERF_COUNT_GGTT,
> + .expected = 0
> + }},
> + .bits_count = 1 ),
> + CMD( MI_CONDITIONAL_BATCH_BUFFER_END, SMI, !F, 0xFF, B,
> + .bits = {{
> + .offset = 0,
> + .mask = MI_GLOBAL_GTT,
> + .expected = 0
> +
Re: [Intel-gfx] [PATCH 4/7] drm/i915: unify FLIP_DONE macro names
On Tue, 4 Feb 2014 21:35:48 +0200
Imre Deak wrote:
> s/FLIPDONE/FLIP_DONE/ to make all FLIP_DONE macro names consistent.
>
> No functional change.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_irq.c | 2 +-
> drivers/gpu/drm/i915/i915_reg.h | 18 +-
> 2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index e0e5190..3b876ee 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1500,7 +1500,7 @@ static void valleyview_pipestat_irq_handler(struct
> drm_device *dev, u32 iir)
> if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> drm_handle_vblank(dev, pipe);
>
> - if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> + if (pipe_stats[pipe] & PLANE_FLIP_DONE_INT_STATUS_VLV) {
> intel_prepare_page_flip(dev, pipe);
> intel_finish_page_flip(dev, pipe);
> }
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index abd18cd..8eefb26 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3227,7 +3227,7 @@
> #define PIPECONF_DITHER_TYPE_TEMP (3<<2)
> #define _PIPEASTAT (dev_priv->info->display_mmio_offset + 0x70024)
> #define PIPE_FIFO_UNDERRUN_STATUS (1UL<<31)
> -#define SPRITE1_FLIPDONE_INT_EN_VLV(1UL<<30)
> +#define SPRITE1_FLIP_DONE_INT_EN_VLV (1UL<<30)
> #define PIPE_CRC_ERROR_ENABLE (1UL<<29)
> #define PIPE_CRC_DONE_ENABLE (1UL<<28)
> #define PIPE_GMBUS_EVENT_ENABLE(1UL<<27)
> @@ -3245,12 +3245,12 @@
> #define PIPE_VBLANK_INTERRUPT_ENABLE (1UL<<17)
> #define PIPEA_HBLANK_INT_EN_VLV(1UL<<16)
> #define PIPE_OVERLAY_UPDATED_ENABLE(1UL<<16)
> -#define SPRITE1_FLIPDONE_INT_STATUS_VLV(1UL<<15)
> -#define SPRITE0_FLIPDONE_INT_STATUS_VLV(1UL<<14)
> +#define SPRITE1_FLIP_DONE_INT_STATUS_VLV (1UL<<15)
> +#define SPRITE0_FLIP_DONE_INT_STATUS_VLV (1UL<<14)
> #define PIPE_CRC_ERROR_INTERRUPT_STATUS(1UL<<13)
> #define PIPE_CRC_DONE_INTERRUPT_STATUS (1UL<<12)
> #define PIPE_GMBUS_INTERRUPT_STATUS(1UL<<11)
> -#define PLANE_FLIPDONE_INT_STATUS_VLV (1UL<<10)
> +#define PLANE_FLIP_DONE_INT_STATUS_VLV (1UL<<10)
> #define PIPE_HOTPLUG_INTERRUPT_STATUS (1UL<<10)
> #define PIPE_VSYNC_INTERRUPT_STATUS(1UL<<9)
> #define PIPE_DISPLAY_LINE_COMPARE_STATUS (1UL<<8)
> @@ -3286,14 +3286,14 @@
> #define PIPEB_LINE_COMPARE_INT_EN (1<<29)
> #define PIPEB_HLINE_INT_EN (1<<28)
> #define PIPEB_VBLANK_INT_EN(1<<27)
> -#define SPRITED_FLIPDONE_INT_EN(1<<26)
> -#define SPRITEC_FLIPDONE_INT_EN(1<<25)
> -#define PLANEB_FLIPDONE_INT_EN (1<<24)
> +#define SPRITED_FLIP_DONE_INT_EN (1<<26)
> +#define SPRITEC_FLIP_DONE_INT_EN (1<<25)
> +#define PLANEB_FLIP_DONE_INT_EN(1<<24)
> #define PIPEA_LINE_COMPARE_INT_EN (1<<21)
> #define PIPEA_HLINE_INT_EN (1<<20)
> #define PIPEA_VBLANK_INT_EN(1<<19)
> -#define SPRITEB_FLIPDONE_INT_EN(1<<18)
> -#define SPRITEA_FLIPDONE_INT_EN(1<<17)
> +#define SPRITEB_FLIP_DONE_INT_EN (1<<18)
> +#define SPRITEA_FLIP_DONE_INT_EN (1<<17)
> #define PLANEA_FLIPDONE_INT_EN (1<<16)
>
> #define DPINVGTT (VLV_DISPLAY_BASE + 0x7002c) /*
> VLV only */
Reviewed-by: Jesse Barnes
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 08/13] drm/i915: Enable register whitelist checks
On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote:
> From: Brad Volkin
>
> MI_STORE_REGISTER_MEM, MI_LOAD_REGISTER_MEM, and MI_LOAD_REGISTER_IMM
> commands allow userspace access to registers. Only certain registers
> should be allowed for such access, so enable checking for those commands.
> Each ring gets its own register whitelist.
>
> MI_LOAD_REGISTER_REG on HSW also allows register access but is currently
> unused by userspace components. Leave it rejected.
>
> PIPE_CONTROL and MEDIA_VFE_STATE allow register access based on certain
> bits being set. Reject those as well.
>
> OTC-Tracker: AXIA-4631
> Change-Id: Ie614a2f0eb2e5917de809e5a17957175d24cc44f
> Signed-off-by: Brad Volkin
> ---
> drivers/gpu/drm/i915/i915_cmd_parser.c | 23 ---
> drivers/gpu/drm/i915/i915_reg.h| 3 +++
> 2 files changed, 23 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 296e322..5d3e303 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -63,9 +63,12 @@ static const struct drm_i915_cmd_descriptor common_cmds[]
> = {
> CMD( MI_SUSPEND_FLUSH, SMI,F, 1, S ),
> CMD( MI_SEMAPHORE_MBOX,SMI, !F, 0xFF, R ),
> CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, R ),
> - CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, R ),
> - CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, R ),
> - CMD( MI_LOAD_REGISTER_MEM, SMI, !F, 0xFF, R ),
> + CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, W,
> + .reg = { .offset = 1, .mask = 0x007C } ),
> + CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, W,
> + .reg = { .offset = 1, .mask = 0x007C } ),
> + CMD( MI_LOAD_REGISTER_MEM, SMI, !F, 0xFF, W,
> + .reg = { .offset = 1, .mask = 0x007C } ),
> CMD( MI_BATCH_BUFFER_START,SMI, !F, 0xFF, S ),
> };
>
> @@ -82,9 +85,23 @@ static const struct drm_i915_cmd_descriptor render_cmds[]
> = {
> CMD( MI_CONDITIONAL_BATCH_BUFFER_END, SMI, !F, 0xFF, S ),
> CMD( GFX_OP_3DSTATE_VF_STATISTICS, S3D,F, 1, S ),
> CMD( PIPELINE_SELECT, S3D,F, 1, S ),
> + CMD( MEDIA_VFE_STATE, S3D, !F, 0x, B,
> + .bits = {{
> + .offset = 2,
> + .mask = MEDIA_VFE_STATE_MMIO_ACCESS_MASK,
> + .expected = 0
> + }},
> + .bits_count = 1 ),
>From my bikeshedding dept.: here too I think it would be beneficial to
have the count decided by an empty element, or a .valid = 1 field or
something.
> CMD( GPGPU_OBJECT, S3D, !F, 0xFF, S ),
> CMD( GPGPU_WALKER, S3D, !F, 0xFF, S ),
> CMD( GFX_OP_3DSTATE_SO_DECL_LIST, S3D, !F, 0x1FF, S ),
> + CMD( GFX_OP_PIPE_CONTROL(5), S3D, !F, 0xFF, B,
> + .bits = {{
> + .offset = 1,
> + .mask = PIPE_CONTROL_MMIO_WRITE,
> + .expected = 0
> + }},
> + .bits_count = 1 ),
> };
>
> static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index b99bacf..6592d0d 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -319,6 +319,7 @@
> #define DISPLAY_PLANE_B (1<<20)
> #define GFX_OP_PIPE_CONTROL(len) ((0x3<<29)|(0x3<<27)|(0x2<<24)|(len-2))
> #define PIPE_CONTROL_GLOBAL_GTT_IVB(1<<24) /*
> gen7+ */
> +#define PIPE_CONTROL_MMIO_WRITE(1<<23)
> #define PIPE_CONTROL_CS_STALL (1<<20)
> #define PIPE_CONTROL_TLB_INVALIDATE(1<<18)
> #define PIPE_CONTROL_QW_WRITE (1<<14)
> @@ -359,6 +360,8 @@
>
> #define PIPELINE_SELECT
> ((0x3<<29)|(0x1<<27)|(0x1<<24)|(0x4<<16))
> #define GFX_OP_3DSTATE_VF_STATISTICS
> ((0x3<<29)|(0x1<<27)|(0x0<<24)|(0xB<<16))
> +#define MEDIA_VFE_STATE
> ((0x3<<29)|(0x2<<27)|(0x0<<24)|(0x0<<16))
> +#define MEDIA_VFE_STATE_MMIO_ACCESS_MASK (0x18)
> #define GPGPU_OBJECT
> ((0x3<<29)|(0x2<<27)|(0x1<<24)|(0x4<<16))
> #define GPGPU_WALKER
> ((0x3<<29)|(0x2<<27)|(0x1<<24)|(0x5<<16))
> #define GFX_OP_3DSTATE_DX9_CONSTANTF_VS \
> --
> 1.8.5.2
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailm
Re: [Intel-gfx] [PATCH 3/7] drm/i915: vlv: s/spin_lock_irqsave/spin_lock/ in irq handler
On Tue, 4 Feb 2014 21:35:47 +0200
Imre Deak wrote:
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_irq.c | 5 ++---
> 1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b5524ea..e0e5190 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1481,10 +1481,9 @@ static void valleyview_pipestat_irq_handler(struct
> drm_device *dev, u32 iir)
> {
> drm_i915_private_t *dev_priv = dev->dev_private;
> u32 pipe_stats[I915_MAX_PIPES];
> - unsigned long irqflags;
> int pipe;
>
> - spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> + spin_lock(&dev_priv->irq_lock);
> for_each_pipe(pipe) {
> int reg = PIPESTAT(pipe);
> pipe_stats[pipe] = I915_READ(reg);
> @@ -1495,7 +1494,7 @@ static void valleyview_pipestat_irq_handler(struct
> drm_device *dev, u32 iir)
> if (pipe_stats[pipe] & 0x8000)
> I915_WRITE(reg, pipe_stats[pipe]);
> }
> - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> + spin_unlock(&dev_priv->irq_lock);
>
> for_each_pipe(pipe) {
> if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
I guess we don't have to worry about new interrupts until we ack this
one, so:
Reviewed-by: Jesse Barnes
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/7] drm/i915: factor out valleyview_pipestat_irq_handler
On Tue, 4 Feb 2014 21:35:46 +0200
Imre Deak wrote:
> This will be used by other platforms too, so factor it out.
>
> The only functional change is the reordeing of gmbus_irq_handler() wrt.
> the hotplug handling, but since it only schedules a work, it isn't an
> issue.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_irq.c | 76
> +++--
> 1 file changed, 42 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 137ac65..b5524ea 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -1477,15 +1477,53 @@ static void gen6_rps_irq_handler(struct
> drm_i915_private *dev_priv, u32 pm_iir)
> }
> }
>
> +static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
> +{
> + drm_i915_private_t *dev_priv = dev->dev_private;
> + u32 pipe_stats[I915_MAX_PIPES];
> + unsigned long irqflags;
> + int pipe;
> +
> + spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> + for_each_pipe(pipe) {
> + int reg = PIPESTAT(pipe);
> + pipe_stats[pipe] = I915_READ(reg);
> +
> + /*
> + * Clear the PIPE*STAT regs before the IIR
> + */
> + if (pipe_stats[pipe] & 0x8000)
> + I915_WRITE(reg, pipe_stats[pipe]);
> + }
> + spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> +
> + for_each_pipe(pipe) {
> + if (pipe_stats[pipe] & PIPE_START_VBLANK_INTERRUPT_STATUS)
> + drm_handle_vblank(dev, pipe);
> +
> + if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> + intel_prepare_page_flip(dev, pipe);
> + intel_finish_page_flip(dev, pipe);
> + }
> +
> + if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS)
> + i9xx_pipe_crc_irq_handler(dev, pipe);
> +
> + if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS &&
> + intel_set_cpu_fifo_underrun_reporting(dev, pipe, false))
> + DRM_ERROR("pipe %c underrun\n", pipe_name(pipe));
> + }
> +
> + if (pipe_stats[0] & PIPE_GMBUS_INTERRUPT_STATUS)
> + gmbus_irq_handler(dev);
> +}
> +
> static irqreturn_t valleyview_irq_handler(int irq, void *arg)
> {
> struct drm_device *dev = (struct drm_device *) arg;
> drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> u32 iir, gt_iir, pm_iir;
> irqreturn_t ret = IRQ_NONE;
> - unsigned long irqflags;
> - int pipe;
> - u32 pipe_stats[I915_MAX_PIPES];
>
> while (true) {
> iir = I915_READ(VLV_IIR);
> @@ -1499,35 +1537,7 @@ static irqreturn_t valleyview_irq_handler(int irq,
> void *arg)
>
> snb_gt_irq_handler(dev, dev_priv, gt_iir);
>
> - spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> - for_each_pipe(pipe) {
> - int reg = PIPESTAT(pipe);
> - pipe_stats[pipe] = I915_READ(reg);
> -
> - /*
> - * Clear the PIPE*STAT regs before the IIR
> - */
> - if (pipe_stats[pipe] & 0x8000)
> - I915_WRITE(reg, pipe_stats[pipe]);
> - }
> - spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> -
> - for_each_pipe(pipe) {
> - if (pipe_stats[pipe] &
> PIPE_START_VBLANK_INTERRUPT_STATUS)
> - drm_handle_vblank(dev, pipe);
> -
> - if (pipe_stats[pipe] & PLANE_FLIPDONE_INT_STATUS_VLV) {
> - intel_prepare_page_flip(dev, pipe);
> - intel_finish_page_flip(dev, pipe);
> - }
> -
> - if (pipe_stats[pipe] & PIPE_CRC_DONE_INTERRUPT_STATUS)
> - i9xx_pipe_crc_irq_handler(dev, pipe);
> -
> - if (pipe_stats[pipe] & PIPE_FIFO_UNDERRUN_STATUS &&
> - intel_set_cpu_fifo_underrun_reporting(dev, pipe,
> false))
> - DRM_ERROR("pipe %c underrun\n",
> pipe_name(pipe));
> - }
> + valleyview_pipestat_irq_handler(dev, iir);
>
> /* Consume port. Then clear IIR or we'll miss events */
> if (iir & I915_DISPLAY_PORT_INTERRUPT) {
> @@ -1543,8 +1553,6 @@ static irqreturn_t valleyview_irq_handler(int irq, void
> *arg)
> I915_READ(PORT_HOTPLUG_STAT);
> }
>
> - if (pipe_stats[0] & PIPE_GMBUS_INTERRUPT_STATUS)
> - gmbus_irq_handler(dev);
>
> if (pm_iir)
> gen6_rps_irq_handler(dev_priv, pm_iir);
I think we're still missing the asle bits too.
Reviewed-by: Jesse Barnes
_
Re: [Intel-gfx] [PATCH 06/13] drm/i915: Add register whitelists for mesa
On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote:
> From: Brad Volkin
>
> These registers are currently used by mesa for blitting,
> transform feedback extensions, and performance monitoring
> extensions.
>
> Signed-off-by: Brad Volkin
> ---
> drivers/gpu/drm/i915/i915_cmd_parser.c | 55
> ++
> drivers/gpu/drm/i915/i915_reg.h| 20 +
> 2 files changed, 75 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 88456638..18d5b05 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -185,6 +185,55 @@ static const struct drm_i915_cmd_table
> hsw_blt_ring_cmds[] = {
> { hsw_blt_cmds, ARRAY_SIZE(hsw_blt_cmds) },
> };
>
> +/*
> + * Register whitelists, sorted by increasing register offset.
> + *
> + * Some registers that userspace accesses are 64 bits. The register
> + * access commands only allow 32-bit accesses. Hence, we have to include
> + * entries for both halves of the 64-bit registers.
> + */
Seems like it would be useful to have a helper macro here.
#define FOO64(addr) (addr), (addr + 4)
With a better name, hopefully. My imagination fails me now.
> +
> +static const u32 gen7_render_regs[] = {
> + HS_INVOCATION_COUNT,
> + HS_INVOCATION_COUNT + sizeof(u32),
> + DS_INVOCATION_COUNT,
> + DS_INVOCATION_COUNT + sizeof(u32),
> + IA_VERTICES_COUNT,
> + IA_VERTICES_COUNT + sizeof(u32),
> + IA_PRIMITIVES_COUNT,
> + IA_PRIMITIVES_COUNT + sizeof(u32),
> + VS_INVOCATION_COUNT,
> + VS_INVOCATION_COUNT + sizeof(u32),
> + GS_INVOCATION_COUNT,
> + GS_INVOCATION_COUNT + sizeof(u32),
> + GS_PRIMITIVES_COUNT,
> + GS_PRIMITIVES_COUNT + sizeof(u32),
> + CL_INVOCATION_COUNT,
> + CL_INVOCATION_COUNT + sizeof(u32),
> + CL_PRIMITIVES_COUNT,
> + CL_PRIMITIVES_COUNT + sizeof(u32),
> + PS_INVOCATION_COUNT,
> + PS_INVOCATION_COUNT + sizeof(u32),
> + PS_DEPTH_COUNT,
> + PS_DEPTH_COUNT + sizeof(u32),
> + GEN7_SO_NUM_PRIMS_WRITTEN(0),
> + GEN7_SO_NUM_PRIMS_WRITTEN(0) + sizeof(u32),
> + GEN7_SO_NUM_PRIMS_WRITTEN(1),
> + GEN7_SO_NUM_PRIMS_WRITTEN(1) + sizeof(u32),
> + GEN7_SO_NUM_PRIMS_WRITTEN(2),
> + GEN7_SO_NUM_PRIMS_WRITTEN(2) + sizeof(u32),
> + GEN7_SO_NUM_PRIMS_WRITTEN(3),
> + GEN7_SO_NUM_PRIMS_WRITTEN(3) + sizeof(u32),
> + GEN7_SO_WRITE_OFFSET(0),
> + GEN7_SO_WRITE_OFFSET(1),
> + GEN7_SO_WRITE_OFFSET(2),
> + GEN7_SO_WRITE_OFFSET(3),
> +};
> +
> +static const u32 gen7_blt_regs[] = {
> + BCS_SWCTRL,
> +};
> +
> #define CLIENT_MASK 0xE000
> #define SUBCLIENT_MASK 0x1800
> #define MI_CLIENT0x
> @@ -313,6 +362,9 @@ void i915_cmd_parser_init_ring(struct intel_ring_buffer
> *ring)
> ring->cmd_table_count = ARRAY_SIZE(gen7_render_cmds);
> }
>
> + ring->reg_table = gen7_render_regs;
> + ring->reg_count = ARRAY_SIZE(gen7_render_regs);
> +
> ring->get_cmd_length_mask = gen7_render_get_cmd_length_mask;
> break;
> case VCS:
> @@ -329,6 +381,9 @@ void i915_cmd_parser_init_ring(struct intel_ring_buffer
> *ring)
> ring->cmd_table_count = ARRAY_SIZE(gen7_blt_cmds);
> }
>
> + ring->reg_table = gen7_blt_regs;
> + ring->reg_count = ARRAY_SIZE(gen7_blt_regs);
> +
> ring->get_cmd_length_mask = gen7_blt_get_cmd_length_mask;
> break;
> case VECS:
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 2b7c26e..b99bacf 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -385,6 +385,26 @@
> #define SRC_COPY_BLT ((0x2<<29)|(0x43<<22))
>
> /*
> + * Registers used only by the command parser
> + */
> +#define BCS_SWCTRL 0x22200
> +
> +#define HS_INVOCATION_COUNT 0x2300
> +#define DS_INVOCATION_COUNT 0x2308
> +#define IA_VERTICES_COUNT 0x2310
> +#define IA_PRIMITIVES_COUNT 0x2318
> +#define VS_INVOCATION_COUNT 0x2320
> +#define GS_INVOCATION_COUNT 0x2328
> +#define GS_PRIMITIVES_COUNT 0x2330
> +#define CL_INVOCATION_COUNT 0x2338
> +#define CL_PRIMITIVES_COUNT 0x2340
> +#define PS_INVOCATION_COUNT 0x2348
> +#define PS_DEPTH_COUNT 0x2350
> +
> +/* There are the 4 64-bit counter registers, one for each stream output */
> +#define GEN7_SO_NUM_PRIMS_WRITTEN(n) (0x5200 + (n) * 8)
> +
> +/*
> * Reset registers
> */
> #define DEBUG_RESET_I830 0x6070
> --
> 1.8.5.2
>
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org
Re: [Intel-gfx] [PATCH 1/7] drm/i915: vlv: don't unmask IIR[DISPLAY_PIPE_A/B_VBLANK] interrupt
On Tue, 4 Feb 2014 21:35:45 +0200
Imre Deak wrote:
> Bspec and the code suggests that the interrupt signaled by IIR[7,5]
> (DISPLAY_PIPE_A/B_VBLANK) is a first level IRQ flag for the second
> level PIPEA/BSTAT[2] (Start of Vertical Blank) interrupt. Measuring
> the relative timings of when IIR[7] and PIPEASTAT[1,2] get set and
> checking the effect of unmasking different pipestat and IIR events
> shows that this isn't so:
>
> First, ISR/IIR[7] gets set independently of PIPEASTAT[18] (Start of
> Vertical Blank Enable) or any other pipestat enable bit, so it isn't
> a first level IRQ bit showing the state of PIPEASTAT[2], but is
> connected directly to the timing generator.
>
> Second, setting only PIPEASTAT[18] and leaving all other pipestat events
> disabled, IIR[6] (DISPLAY_PIPE_A_EVENT) gets set close to the moment when
> PIPEASTAT[2] gets set, so the former is a first level interrupt flag for
> the latter. The bspec is rather unclear about this, but I also assume
> that IIR[6] signals all pipestat A events, except PIPEASTAT[31] (FIFO
> Under-run Status).
>
> Third, IIR[7] is set close to the moment when PIPEASTAT[1] (Framestart
> Interrupt) gets set, in the mode I used about 12usec after PIPEASTAT[2]
> and IIR[6] gets set. This means the IIR[7] isn't marking the start of
> vblank, but rather signals the framestart event.
>
> Based on the above, we don't need to unmask IIR[7] when waiting for
> start of vblank events, but we can rely on IIR[6] being always unmasked,
> which will signal when PIPEASTAT[2] gets set. Doing this will also get
> rid of the overhead of getting an interrupt and servicing IIR[7], which
> is atm raised always some time after IIR[6]/PIPEASTAT[2] is raised.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_irq.c | 14 --
> 1 file changed, 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index b226ae6..137ac65 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -2297,18 +2297,11 @@ static int valleyview_enable_vblank(struct drm_device
> *dev, int pipe)
> {
> drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> unsigned long irqflags;
> - u32 imr;
>
> if (!i915_pipe_enabled(dev, pipe))
> return -EINVAL;
>
> spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> - imr = I915_READ(VLV_IMR);
> - if (pipe == PIPE_A)
> - imr &= ~I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT;
> - else
> - imr &= ~I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
> - I915_WRITE(VLV_IMR, imr);
> i915_enable_pipestat(dev_priv, pipe,
>PIPE_START_VBLANK_INTERRUPT_ENABLE);
> spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> @@ -2366,17 +2359,10 @@ static void valleyview_disable_vblank(struct
> drm_device *dev, int pipe)
> {
> drm_i915_private_t *dev_priv = (drm_i915_private_t *) dev->dev_private;
> unsigned long irqflags;
> - u32 imr;
>
> spin_lock_irqsave(&dev_priv->irq_lock, irqflags);
> i915_disable_pipestat(dev_priv, pipe,
> PIPE_START_VBLANK_INTERRUPT_ENABLE);
> - imr = I915_READ(VLV_IMR);
> - if (pipe == PIPE_A)
> - imr |= I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT;
> - else
> - imr |= I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;
> - I915_WRITE(VLV_IMR, imr);
> spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags);
> }
>
Reviewed-by: Jesse Barnes
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 7/7] drm/i915: vlv: handle only enabled pipestat interrupt events
On Wed, 2014-02-05 at 17:11 +0200, Ville Syrjälä wrote:
> On Tue, Feb 04, 2014 at 09:35:51PM +0200, Imre Deak wrote:
> > Atm we call the handlers for pending pipestat interrupt events even if
> > they aren't explicitly enabled by i915_enable_pipestat(). This isn't an
> > issue for events other than the vblank start event, since those are
> > always enabled anyways. Otoh, we enable the vblank start event
> > on-demand, so we'll end up calling the vblank handler at times when they
> > are disabled.
> >
> > I haven't checked if this causes any real problem, but for consistency
> > and to remove some overhead we should still fix this by clearing /
> > handling only the enabled interrupt events. Also this is a dependency
> > for the upcoming VLV power domain patchset where we need to disable all
> > the pipestat interrupts whenever the display power well is off.
> >
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/i915_drv.h | 1 +
> > drivers/gpu/drm/i915/i915_irq.c | 33 ++---
> > drivers/gpu/drm/i915/i915_reg.h | 4
> > 3 files changed, 35 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 43f37ca..faca5b4 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -1427,6 +1427,7 @@ typedef struct drm_i915_private {
> > };
> > u32 gt_irq_mask;
> > u32 pm_irq_mask;
> > + u32 pipestat_irq_mask[I915_MAX_PIPES];
> >
> > struct work_struct hotplug_work;
> > bool enable_hotplug_processing;
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index eea5398..2cac477 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -419,6 +419,16 @@ done:
> > return ret;
> > }
> >
> > +static bool __cpu_fifo_underrun_reporting_enabled(struct drm_device *dev,
> > + enum pipe pipe)
> > +{
> > + struct drm_i915_private *dev_priv = dev->dev_private;
> > + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> > +
> > + return !intel_crtc->cpu_fifo_underrun_disabled;
> > +}
> > +
> > /**
> > * intel_set_pch_fifo_underrun_reporting - enable/disable FIFO underrun
> > messages
> > * @dev: drm device
> > @@ -520,6 +530,8 @@ i915_enable_pipestat(drm_i915_private_t *dev_priv, enum
> > pipe pipe,
> > if ((pipestat & enable_mask) == enable_mask)
> > return;
> >
> > + dev_priv->pipestat_irq_mask[pipe] |= status_mask;
> > +
> > /* Enable the interrupt, clear any pending status */
> > pipestat |= enable_mask | status_mask;
> > I915_WRITE(reg, pipestat);
> > @@ -550,6 +562,8 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv,
> > enum pipe pipe,
> > if ((pipestat & enable_mask) == 0)
> > return;
> >
> > + dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> > +
> > pipestat &= ~enable_mask;
> > I915_WRITE(reg, pipestat);
> > POSTING_READ(reg);
> > @@ -1530,18 +1544,31 @@ static void gen6_rps_irq_handler(struct
> > drm_i915_private *dev_priv, u32 pm_iir)
> > static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32
> > iir)
> > {
> > drm_i915_private_t *dev_priv = dev->dev_private;
> > - u32 pipe_stats[I915_MAX_PIPES];
> > + u32 pipe_stats[I915_MAX_PIPES] = { };
> > int pipe;
> >
> > spin_lock(&dev_priv->irq_lock);
> > for_each_pipe(pipe) {
> > - int reg = PIPESTAT(pipe);
> > + int reg;
> > + u32 mask;
> > +
> > + if (!dev_priv->pipestat_irq_mask[pipe] &&
> > + !__cpu_fifo_underrun_reporting_enabled(dev, pipe))
> > + continue;
> > +
> > + reg = PIPESTAT(pipe);
> > pipe_stats[pipe] = I915_READ(reg);
> >
> > /*
> > * Clear the PIPE*STAT regs before the IIR
> > */
> > - if (pipe_stats[pipe] & 0x8000)
> > + mask = PIPESTAT_INT_ENABLE_MASK | PIPE_FIFO_UNDERRUN_STATUS;
>
> Maybe we should add PIPE_FIFO_UNDERRUN_STATUS to the mask only when the
> underrun reporting is enabled. If someone goes and enables underrun
> reporting just after valleyview_pipestat_irq_handler(), we'd
> potentially report a stale underrun.
Ok, will fix it.
--Imre
>
> You get to blame me for that one though, since the bug is already there
> in the current code, which I implemented.
>
> > + if (iir & I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe))
> > + mask |= dev_priv->pipestat_irq_mask[pipe];
> > + pipe_stats[pipe] &= mask;
> > +
> > + if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
> > + PIPESTAT_INT_STATUS_MASK))
> > I915_WRITE(reg, pipe_stats[pipe]);
> > }
> > spin_unlock(&dev_priv->irq_lock);
> >
Re: [Intel-gfx] [PATCH 04/13] drm/i915: Reject privileged commands
On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote:
> From: Brad Volkin
>
> The spec defines most of these commands as privileged. A few others,
> like the semaphore mbox command and some display commands, are also
> reserved for the driver's use. Subsequent patches relax some of
> these restrictions.
>
> Signed-off-by: Brad Volkin
> ---
> drivers/gpu/drm/i915/i915_cmd_parser.c | 54
> --
> drivers/gpu/drm/i915/i915_reg.h| 31 +--
> 2 files changed, 54 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 2e27bad..cc2f68c 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -57,27 +57,27 @@
> static const struct drm_i915_cmd_descriptor common_cmds[] = {
> CMD( MI_NOOP, SMI,F, 1, S ),
> CMD( MI_USER_INTERRUPT,SMI,F, 1, S ),
> - CMD( MI_WAIT_FOR_EVENT,SMI,F, 1, S ),
> + CMD( MI_WAIT_FOR_EVENT,SMI,F, 1, R ),
> CMD( MI_ARB_CHECK, SMI,F, 1, S ),
> CMD( MI_REPORT_HEAD, SMI,F, 1, S ),
> CMD( MI_SUSPEND_FLUSH, SMI,F, 1, S ),
> - CMD( MI_SEMAPHORE_MBOX,SMI, !F, 0xFF, S ),
> - CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, S ),
> - CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, S ),
> - CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, S ),
> - CMD( MI_LOAD_REGISTER_MEM, SMI, !F, 0xFF, S ),
> + CMD( MI_SEMAPHORE_MBOX,SMI, !F, 0xFF, R ),
> + CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, R ),
> + CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, R ),
> + CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, R ),
> + CMD( MI_LOAD_REGISTER_MEM, SMI, !F, 0xFF, R ),
> CMD( MI_BATCH_BUFFER_START,SMI, !F, 0xFF, S ),
> };
>
> static const struct drm_i915_cmd_descriptor render_cmds[] = {
> CMD( MI_FLUSH, SMI,F, 1, S ),
> - CMD( MI_ARB_ON_OFF,SMI,F, 1, S ),
> + CMD( MI_ARB_ON_OFF,SMI,F, 1, R ),
> CMD( MI_PREDICATE, SMI,F, 1, S ),
> CMD( MI_TOPOLOGY_FILTER, SMI,F, 1, S ),
> - CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, S ),
> - CMD( MI_SET_CONTEXT, SMI, !F, 0xFF, S ),
> + CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, R ),
> + CMD( MI_SET_CONTEXT, SMI, !F, 0xFF, R ),
> CMD( MI_URB_CLEAR, SMI, !F, 0xFF, S ),
> - CMD( MI_UPDATE_GTT,SMI, !F, 0xFF, S ),
> + CMD( MI_UPDATE_GTT,SMI, !F, 0xFF, R ),
> CMD( MI_CLFLUSH, SMI, !F, 0x3FF, S ),
> CMD( MI_CONDITIONAL_BATCH_BUFFER_END, SMI, !F, 0xFF, S ),
> CMD( GFX_OP_3DSTATE_VF_STATISTICS, S3D,F, 1, S ),
> @@ -92,7 +92,9 @@ static const struct drm_i915_cmd_descriptor
> hsw_render_cmds[] = {
> CMD( MI_RS_CONTROL,SMI,F, 1, S ),
> CMD( MI_URB_ATOMIC_ALLOC, SMI,F, 1, S ),
> CMD( MI_RS_CONTEXT,SMI,F, 1, S ),
> - CMD( MI_LOAD_REGISTER_REG, SMI, !F, 0xFF, S ),
> + CMD( MI_LOAD_SCAN_LINES_INCL, SMI, !F, 0x3F, R ),
> + CMD( MI_LOAD_SCAN_LINES_EXCL, SMI, !F, 0x3F, R ),
> + CMD( MI_LOAD_REGISTER_REG, SMI, !F, 0xFF, R ),
> CMD( MI_RS_STORE_DATA_IMM, SMI, !F, 0xFF, S ),
> CMD( MI_LOAD_URB_MEM, SMI, !F, 0xFF, S ),
> CMD( MI_STORE_URB_MEM, SMI, !F, 0xFF, S ),
> @@ -107,8 +109,9 @@ static const struct drm_i915_cmd_descriptor
> hsw_render_cmds[] = {
> };
>
> static const struct drm_i915_cmd_descriptor video_cmds[] = {
> - CMD( MI_ARB_ON_OFF,SMI,F, 1, S ),
> + CMD( MI_ARB_ON_OFF,SMI,F, 1, R ),
> CMD( MI_STORE_DWORD_IMM, SMI, !F, 0xFF, S ),
> + CMD( MI_UPDATE_GTT,SMI, !F, 0x3F, R ),
> CMD( MI_CONDITIONAL_BATCH_BUFFER_END, SMI, !F, 0xFF, S ),
> /*
>* MFX_WAIT doesn't fit the way we handle length for most commands.
> @@ -119,18 +122,25 @@ static const struct drm_i915_cmd_descriptor
> video_cmds[] = {
> };
>
> static const struct drm_i915_cmd_descriptor vecs_cmds[] = {
> -
Re: [Intel-gfx] [PATCH] [RFC] drm/i915: Generate a hang error code
On Wed, Feb 05, 2014 at 02:59:08PM +, Jesse Barnes wrote: > On Tue, 4 Feb 2014 12:18:55 + > Ben Widawsky wrote: > > > We get a large number of bugs which have a, "hey I have that too" > > because they see a GPU hang in dmesg. While two machines of the same > > model having a GPU hang is indeed a coincidence, it is far from enough > > evidence to suggest they are the same. > > > > In order to reduce this effect, and hopefully get people to file new bug > > reports, clearly the error message itself has been insufficient (see ref > > at the bottom for a new bug report with this characteristic). > > > > The algorithm is purposely pretty naive. I don't think we need much in > > order to avoid the problem I am trying to solve, and keeping it naive > > gives us some ability to make a decent test case. > > I like the direction of this. If we can get some basic info into the > dmesg part of things (the only part regular users will actually look > at) we can probably avoid some of the "me too" action we see on general > GPU hangs. Having PID, comm, and some sort of hang signature are all > good steps in that direction imo. tbh I don't see much value in regular users trying to triage gpu hang. If they're not damn sure that they have a dupe (which means same platform, versions of the software stack and crashing games) I much prefer if they just send in a duplicate bug for us to triage. With the mis-design of bugzilla it's much harder to untangle a wrong me-too than mark something as duplicate. And especially long-running bugs are a royal pain if there's too much wrong me-too noise in there. Not a comment on the patch itself, just a general comment wrt avoiding me-too gpu hang reports. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 02/13] drm/i915: Implement command buffer parsing logic
On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote: > From: Brad Volkin > > The command parser scans batch buffers submitted via execbuffer ioctls before > the driver submits them to hardware. At a high level, it looks for several > things: > > 1) Commands which are explicitly defined as privileged or which should only be >used by the kernel driver. The parser generally rejects such commands, with >the provision that it may allow some from the drm master process. > 2) Commands which access registers. To support correct/enhanced userspace >functionality, particularly certain OpenGL extensions, the parser provides > a >whitelist of registers which userspace may safely access (for both normal > and >drm master processes). > 3) Commands which access privileged memory (i.e. GGTT, HWS page, etc). The >parser always rejects such commands. > > Each ring maintains tables of commands and registers which the parser uses in > scanning batch buffers submitted to that ring. > > The set of commands that the parser must check for is significantly smaller > than the number of commands supported, especially on the render ring. As such, > the parser tables (built up in subsequent patches) contain only those commands > required by the parser. This generally works because command opcode ranges > have > standard command length encodings. So for commands that the parser does not > need > to check, it can easily skip them. This is implementated via a per-ring length > decoding vfunc. > > Unfortunately, there are a number of commands that do not follow the standard > length encoding for their opcode range, primarily amongst the MI_* commands. > To > handle this, the parser provides a way to define explicit "skip" entries in > the > per-ring command tables. > > Other command table entries will map fairly directly to high level categories > mentioned above: rejected, master-only, register whitelist. A number of > checks, > including the privileged memory checks, are implemented via a general > bitmasking > mechanism. > > OTC-Tracker: AXIA-4631 > Change-Id: I50b98c71c6655893291c78a2d1b8954577b37a30 > Signed-off-by: Brad Volkin > --- > drivers/gpu/drm/i915/Makefile | 3 +- > drivers/gpu/drm/i915/i915_cmd_parser.c | 404 > + > drivers/gpu/drm/i915/i915_drv.h| 94 +++ > drivers/gpu/drm/i915/i915_gem_execbuffer.c | 17 ++ > drivers/gpu/drm/i915/i915_params.c | 5 + > drivers/gpu/drm/i915/intel_ringbuffer.c| 2 + > drivers/gpu/drm/i915/intel_ringbuffer.h| 32 +++ > 7 files changed, 556 insertions(+), 1 deletion(-) > create mode 100644 drivers/gpu/drm/i915/i915_cmd_parser.c > > diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile > index 4850494..2da81bf 100644 > --- a/drivers/gpu/drm/i915/Makefile > +++ b/drivers/gpu/drm/i915/Makefile > @@ -47,7 +47,8 @@ i915-y := i915_drv.o i915_dma.o i915_irq.o \ > dvo_tfp410.o \ > dvo_sil164.o \ > dvo_ns2501.o \ > - i915_gem_dmabuf.o > + i915_gem_dmabuf.o \ > + i915_cmd_parser.o If you add this anywhere but last, you only need to touch one line instead of two. It's nitpicky, but helps with things like git blame (which would now point at you for i915_gem_dmabuf.o too ;). > > i915-$(CONFIG_COMPAT) += i915_ioc32.o > > diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c > b/drivers/gpu/drm/i915/i915_cmd_parser.c > new file mode 100644 > index 000..7639dbc > --- /dev/null > +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c > @@ -0,0 +1,404 @@ > +/* > + * Copyright © 2013 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: > + *Brad Volkin > + * > + */ > + > +#include "i915_drv.h" > + > +#define CLIENT_MASK 0xE000 > +#define SUBCLIENT_MASK 0x1800 > +#define M
Re: [Intel-gfx] [PATCH 7/7] drm/i915: vlv: handle only enabled pipestat interrupt events
On Tue, Feb 04, 2014 at 09:35:51PM +0200, Imre Deak wrote:
> Atm we call the handlers for pending pipestat interrupt events even if
> they aren't explicitly enabled by i915_enable_pipestat(). This isn't an
> issue for events other than the vblank start event, since those are
> always enabled anyways. Otoh, we enable the vblank start event
> on-demand, so we'll end up calling the vblank handler at times when they
> are disabled.
>
> I haven't checked if this causes any real problem, but for consistency
> and to remove some overhead we should still fix this by clearing /
> handling only the enabled interrupt events. Also this is a dependency
> for the upcoming VLV power domain patchset where we need to disable all
> the pipestat interrupts whenever the display power well is off.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_drv.h | 1 +
> drivers/gpu/drm/i915/i915_irq.c | 33 ++---
> drivers/gpu/drm/i915/i915_reg.h | 4
> 3 files changed, 35 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 43f37ca..faca5b4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1427,6 +1427,7 @@ typedef struct drm_i915_private {
> };
> u32 gt_irq_mask;
> u32 pm_irq_mask;
> + u32 pipestat_irq_mask[I915_MAX_PIPES];
>
> struct work_struct hotplug_work;
> bool enable_hotplug_processing;
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index eea5398..2cac477 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -419,6 +419,16 @@ done:
> return ret;
> }
>
> +static bool __cpu_fifo_underrun_reporting_enabled(struct drm_device *dev,
> + enum pipe pipe)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + struct drm_crtc *crtc = dev_priv->pipe_to_crtc_mapping[pipe];
> + struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +
> + return !intel_crtc->cpu_fifo_underrun_disabled;
> +}
> +
> /**
> * intel_set_pch_fifo_underrun_reporting - enable/disable FIFO underrun
> messages
> * @dev: drm device
> @@ -520,6 +530,8 @@ i915_enable_pipestat(drm_i915_private_t *dev_priv, enum
> pipe pipe,
> if ((pipestat & enable_mask) == enable_mask)
> return;
>
> + dev_priv->pipestat_irq_mask[pipe] |= status_mask;
> +
> /* Enable the interrupt, clear any pending status */
> pipestat |= enable_mask | status_mask;
> I915_WRITE(reg, pipestat);
> @@ -550,6 +562,8 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, enum
> pipe pipe,
> if ((pipestat & enable_mask) == 0)
> return;
>
> + dev_priv->pipestat_irq_mask[pipe] &= ~status_mask;
> +
> pipestat &= ~enable_mask;
> I915_WRITE(reg, pipestat);
> POSTING_READ(reg);
> @@ -1530,18 +1544,31 @@ static void gen6_rps_irq_handler(struct
> drm_i915_private *dev_priv, u32 pm_iir)
> static void valleyview_pipestat_irq_handler(struct drm_device *dev, u32 iir)
> {
> drm_i915_private_t *dev_priv = dev->dev_private;
> - u32 pipe_stats[I915_MAX_PIPES];
> + u32 pipe_stats[I915_MAX_PIPES] = { };
> int pipe;
>
> spin_lock(&dev_priv->irq_lock);
> for_each_pipe(pipe) {
> - int reg = PIPESTAT(pipe);
> + int reg;
> + u32 mask;
> +
> + if (!dev_priv->pipestat_irq_mask[pipe] &&
> + !__cpu_fifo_underrun_reporting_enabled(dev, pipe))
> + continue;
> +
> + reg = PIPESTAT(pipe);
> pipe_stats[pipe] = I915_READ(reg);
>
> /*
>* Clear the PIPE*STAT regs before the IIR
>*/
> - if (pipe_stats[pipe] & 0x8000)
> + mask = PIPESTAT_INT_ENABLE_MASK | PIPE_FIFO_UNDERRUN_STATUS;
Maybe we should add PIPE_FIFO_UNDERRUN_STATUS to the mask only when the
underrun reporting is enabled. If someone goes and enables underrun
reporting just after valleyview_pipestat_irq_handler(), we'd
potentially report a stale underrun.
You get to blame me for that one though, since the bug is already there
in the current code, which I implemented.
> + if (iir & I915_DISPLAY_PIPE_EVENT_INTERRUPT(pipe))
> + mask |= dev_priv->pipestat_irq_mask[pipe];
> + pipe_stats[pipe] &= mask;
> +
> + if (pipe_stats[pipe] & (PIPE_FIFO_UNDERRUN_STATUS |
> + PIPESTAT_INT_STATUS_MASK))
> I915_WRITE(reg, pipe_stats[pipe]);
> }
> spin_unlock(&dev_priv->irq_lock);
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c998c4f..47e0635 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -997,6 +997,10 @@
> #define I915_DISPLAY_
Re: [Intel-gfx] Request for feedback - Sprite flip notification support
On Wed, Feb 05, 2014 at 08:35:11PM +0530, Vijay Purushothaman wrote: > Hello, > > In our current driver implementation we support flip notifications only > for primary plane. So, in a full screen video playback scenario where > only one sprite plane is active, the user space is forced to rely on > primary plane flip notification even though there is no real need for > this plane to be active. Ideally we should be able to support flip > notifications for any given plane. Switching off the primary plane (when > not used) will help in better memory self refresh & decent power savings.. > > We do have a hack in android product trees which supports flip > notifications for one sprite plane. unfortunately this hack in its > current form cannot be considered for up streaming... > > My current thinking is to have an array of unpin_work items to match the > number of planes. Is anyone working on this or thought about this > scenario in detail? Any pointers / restrictions that needs to considered > for a generic implementation of this feature? The plan is to implement the nuclear page flip which will take care of all planes in the same way. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] [RFC] drm/i915: Generate a hang error code
On Tue, 4 Feb 2014 12:18:55 + Ben Widawsky wrote: > We get a large number of bugs which have a, "hey I have that too" > because they see a GPU hang in dmesg. While two machines of the same > model having a GPU hang is indeed a coincidence, it is far from enough > evidence to suggest they are the same. > > In order to reduce this effect, and hopefully get people to file new bug > reports, clearly the error message itself has been insufficient (see ref > at the bottom for a new bug report with this characteristic). > > The algorithm is purposely pretty naive. I don't think we need much in > order to avoid the problem I am trying to solve, and keeping it naive > gives us some ability to make a decent test case. I like the direction of this. If we can get some basic info into the dmesg part of things (the only part regular users will actually look at) we can probably avoid some of the "me too" action we see on general GPU hangs. Having PID, comm, and some sort of hang signature are all good steps in that direction imo. Acked-by: Jesse Barnes Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH IGT] dpio: Add back get_dpio_port which is needed for future platform.
Future platform require the phy input to determine which PHY to target for.
Signed-off-by: Chon Ming Lee
---
lib/intel_iosf.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/lib/intel_iosf.c b/lib/intel_iosf.c
index f57212f..0c9f4d8 100644
--- a/lib/intel_iosf.c
+++ b/lib/intel_iosf.c
@@ -74,15 +74,26 @@ int intel_nc_write(uint8_t addr, uint32_t val)
return vlv_sideband_rw(IOSF_PORT_NC, PUNIT_OPCODE_REG_WRITE, addr,
&val);
}
+static int get_dpio_port(int phy)
+{
+ struct pci_device *dev = intel_get_pci_device();
+ int dpio_port;
+
+ if (IS_VALLEYVIEW(dev->device_id))
+ dpio_port = IOSF_PORT_DPIO;
+
+ return dpio_port;
+}
+
uint32_t intel_dpio_reg_read(uint32_t reg, int phy)
{
uint32_t val;
- vlv_sideband_rw(IOSF_PORT_DPIO, DPIO_OPCODE_REG_READ, reg, &val);
+ vlv_sideband_rw(get_dpio_port(phy), DPIO_OPCODE_REG_READ, reg, &val);
return val;
}
void intel_dpio_reg_write(uint32_t reg, uint32_t val, int phy)
{
- vlv_sideband_rw(IOSF_PORT_DPIO, DPIO_OPCODE_REG_WRITE, reg, &val);
+ vlv_sideband_rw(get_dpio_port(phy), DPIO_OPCODE_REG_WRITE, reg, &val);
}
--
1.8.3.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/6] drm/i915: Wrap the preallocated BIOS framebuffer and preserve for KMS fbcon v7
On Sat, 14 Dec 2013 12:36:30 +0100 Daniel Vetter wrote: > On Sat, Dec 14, 2013 at 12:13:45PM +0100, Daniel Vetter wrote: > > On Thu, Dec 12, 2013 at 12:41:54PM -0800, Jesse Barnes wrote: > > > + ifbdev->helper.funcs->initial_config = intel_fb_initial_config; > > > > This here is a bit surprising - my model of operation here presumed that > > if we correctly assign the crtc->fb and the ifbdev->fb pointers we could > > fully rely on the fastboot setcrtc logic to eschew the modeset. > > > > Being the ever-vary of special-purpose logic I'd much prefer this implicit > > approach - otherwise we have one more special case to care about in the > > fastboot=y/n and CONFIG_FB=y/n matrix. > > > > So have you tried to ditch this special initial_config functions > > (obviously only looks good with fastboot=1) or what precise corner-case > > does this fix? > > Ok, I've dug out your old patch from almost a year ago which added the > ->initial_config hook. I see the point now of copying exactly the bios > config in the hope that we end up with something that has a higher chance > of working. > > But imo this is an issue separate from the "take over bios fb" feature > here, so this should be > - split into a separate patch > - used even when we fail to take over the bios fb > The later point will require some mode-from-pipe_config reconstruction to > work outside of the fastboot=1 hack mode. > > I really like the idea though. Ok I split this up, made fb a ptr, fixed up the CONFIG_FB bits, and I think we figured out the crtc timing stuff. I think that's all the feedback from the last round, so I'll re-post for some (hopefully) final review. There are some additional improvements that would be nice: - compute_mode_changes needs to get smarter in general and look at pfit state. Eventually we'll probably need a platform specific callback for this that tells us whether a pipe shutdown is needed for a given global configuration change. - pfit disable should be split out into a separate callback from our mode_set function (which also needs to get smarter after the compute_mode_changes improvements) - need to detect audio and infoframe configs and cross-check, though I don't think these affect fastboot at all since we ought to be able to leave them alone All of the above aren't strictly necessary though, they're just improvements on current code, some of which will overlap with the atomic mode set work. So we may be able to flip the i915.fastboot=1 default switch once these bits land after some additional HSW and BDW testing... Thanks, Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 2/2] drm/i915: add a display info file to debugfs
On Thu, 30 Jan 2014 19:58:43 -0200
Rodrigo Vivi wrote:
> > + seq_printf(m, ", mode:\n");
> > + seq_printf(m, "\t\t%d:\"%s\" %d %d %d %d %d %d %d
> > %d %d %d 0x%x 0x%x\n",
> > + mode->base.id, mode->name,
> > + mode->vrefresh, mode->clock,
> > + mode->hdisplay, mode->hsync_start,
> > + mode->hsync_end, mode->htotal,
> > + mode->vdisplay, mode->vsync_start,
> > + mode->vsync_end, mode->vtotal,
> > + mode->type, mode->flags);
> > + } else {
> > + seq_printf(m, "\n");
>
> for cases like this shouldn't we use seq_put instead of seq_printf?
Yeah I guess that's a bit simpler, I'll switch it over.
> > + seq_printf(m, "\tfixed mode:\n");
> > + seq_printf(m, "\t\t%d:\"%s\" %d %d %d %d %d %d %d %d %d %d 0x%x
> > 0x%x\n",
> > + mode->base.id, mode->name,
> > + mode->vrefresh, mode->clock,
> > + mode->hdisplay, mode->hsync_start,
> > + mode->hsync_end, mode->htotal,
> > + mode->vdisplay, mode->vsync_start,
> > + mode->vsync_end, mode->vtotal,
> > + mode->type, mode->flags);
> > +}
>
> I would prefer a more bloated info, with variable_name =
> vriable_value... I know this is bloated, but I'm also think on our
> lazyness when reading bug reports ;)
Yeah I always have to look up the field names too, I'll annotate this
just to make things extra easy for us. :)
> > +#define for_each_connector_on_encoder(dev, __encoder, intel_connector) \
> > + list_for_each_entry((intel_connector),
> > &(dev)->mode_config.connector_list, base.head) \
> > + if ((intel_connector)->base.encoder == (__encoder))
>
> are all of this parenthesis needed?
I think that's the minimal amount, yeah. Anything passed in to the
macro needs to have parens around it just in case it's an expression
that would cause trouble when expanded into the code.
Thanks for the review, I'll re-post shortly.
Jesse
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/7] drm/i915: vlv: fix mapping of pipestat enable to status bits
On Wed, 2014-02-05 at 16:54 +0200, Ville Syrjälä wrote:
> On Tue, Feb 04, 2014 at 09:35:50PM +0200, Imre Deak wrote:
> > At least on VLV we can't get at the pipestat status bits by simply right
> > shifting the corresponding enable bits. The mapping between enable and
> > status bits for the sprite0,1 flip done and the PSR events don't follow
> > this rule, so we need to map them separately.
> >
> > The PSR enable for pipe A is DPFLIPSTAT[22], but I haven't added support
> > for this, since there is no user of it atm. Until support is added WARN
> > if someone tries to enable PSR interrupts, or tries to enable the same
> > (1 << 6) bit on pipe B, which MBZ.
> >
> > Signed-off-by: Imre Deak
> > ---
> > drivers/gpu/drm/i915/i915_irq.c | 36 +++-
> > drivers/gpu/drm/i915/i915_reg.h | 3 +++
> > 2 files changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c
> > b/drivers/gpu/drm/i915/i915_irq.c
> > index ec56a70..eea5398 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -471,9 +471,29 @@ done:
> > return ret;
> > }
> >
> > +static void set_pipestat_enable_bit(u32 status_mask, u32 status_bit,
> > + u32 enable_bit, u32 *enable_mask)
> > +{
> > + if (status_mask & status_bit)
> > + *enable_mask |= enable_bit;
> > + else
> > + *enable_mask &= ~enable_bit;
> > +}
> > +
> > static u32 pipestat_to_enable_mask(struct drm_device *dev, u32 status_mask)
> > {
> > - return status_mask << 16;
> > + u32 enable_mask = status_mask << 16;
> > +
> > + if (!IS_VALLEYVIEW(dev))
> > + return enable_mask;
> > +
> > + enable_mask &= ~PIPE_FIFO_UNDERRUN_STATUS;
> > + set_pipestat_enable_bit(status_mask, SPRITE1_FLIP_DONE_INT_STATUS_VLV,
> > + SPRITE1_FLIP_DONE_INT_EN_VLV, &enable_mask);
> > + set_pipestat_enable_bit(enable_mask, SPRITE0_FLIP_DONE_INT_STATUS_VLV,
> > + SPRITE0_FLIP_DONE_INT_EN_VLV, &enable_mask);
>
> This feels a bit subtle to me. Maybe do it in a bit more straightforward
> way? Eg.:
>
> enable_mask &= ~(PIPE_FIFO_UNDERRUN_STATUS |
>SPRITE0_FLIP_DONE_INT_EN_VLV |
>SPRITE1_FLIP_DONE_INT_EN_VLV);
> if (status_mask & SPRITE0_FLIP_DONE_INT_STATUS_VLV)
> enable_mask |= SPRITE0_FLIP_DONE_INT_EN_VLV;
> if (status_mask & SPRITE1_FLIP_DONE_INT_STATUS_VLV)
> enable_mask |= SPRITE1_FLIP_DONE_INT_EN_VLV;
I added the helpers in case more remappings are needed. But since we
have only the above two (and hopefully won't have more) I agree this is
simpler, so I'll rewrite it.
> > +
> > + return enable_mask;
> > }
> >
> > void
> > @@ -489,6 +509,13 @@ i915_enable_pipestat(drm_i915_private_t *dev_priv,
> > enum pipe pipe,
> > if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK))
> > return;
> >
> > + /*
> > +* On pipe A we don't support the PSR interrupt yet, on pipe B the
> > +* same bit MBZ.
> > +*/
> > + if (WARN_ON_ONCE(status_mask & PIPE_A_PSR_STATUS_VLV))
> > + return;
>
> Won't this break the BLC interrupt on gen3/4?
Good catch and me not testing this on those gens.. Will use instead here
and in i915_disable_pipestat:
if (WARN_ON_ONCE(IS_VALLEYVIEW(dev) && (status_mask &
PIPE_A_PSR_STATUS_VLV)))
return;
--Imre
> > +
> > enable_mask = pipestat_to_enable_mask(dev_priv->dev, status_mask);
> > if ((pipestat & enable_mask) == enable_mask)
> > return;
> > @@ -512,6 +539,13 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv,
> > enum pipe pipe,
> > if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK))
> > return;
> >
> > + /*
> > +* On pipe A we don't support the PSR interrupt yet, on pipe B the
> > +* same bit MBZ.
> > +*/
> > + if (WARN_ON_ONCE(status_mask & PIPE_A_PSR_STATUS_VLV))
> > + return;
> > +
> > enable_mask = pipestat_to_enable_mask(dev_priv->dev, status_mask);
> > if ((pipestat & enable_mask) == 0)
> > return;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 599c7f6..c998c4f 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -3240,6 +3240,7 @@
> > #define PIPE_LEGACY_BLC_EVENT_ENABLE (1UL<<22)
> > #define PIPE_ODD_FIELD_INTERRUPT_ENABLE (1UL<<21)
> > #define PIPE_EVEN_FIELD_INTERRUPT_ENABLE (1UL<<20)
> > +#define PIPE_B_PSR_INTERRUPT_ENABLE_VLV (1UL<<19)
> > #define PIPE_HOTPLUG_TV_INTERRUPT_ENABLE (1UL<<18) /* pre-965 */
> > #define PIPE_START_VBLANK_INTERRUPT_ENABLE (1UL<<18) /* 965 or
> > later */
> > #define PIPE_VBLANK_INTERRUPT_ENABLE (1UL<<17)
> > @@ -3256,8 +3257,10 @@
> > #define PIPE_DISPLAY_LINE_COMPARE_STATUS (1UL<<8)
> > #define PIPE_DPST_EVENT_STATUS (1UL<<7)
> > #define PIPE_LEGAC
[Intel-gfx] Request for feedback - Sprite flip notification support
Hello, In our current driver implementation we support flip notifications only for primary plane. So, in a full screen video playback scenario where only one sprite plane is active, the user space is forced to rely on primary plane flip notification even though there is no real need for this plane to be active. Ideally we should be able to support flip notifications for any given plane. Switching off the primary plane (when not used) will help in better memory self refresh & decent power savings.. We do have a hack in android product trees which supports flip notifications for one sprite plane. unfortunately this hack in its current form cannot be considered for up streaming... My current thinking is to have an array of unpin_work items to match the number of planes. Is anyone working on this or thought about this scenario in detail? Any pointers / restrictions that needs to considered for a generic implementation of this feature? *Thanks, Vijay * ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: move intel_hrawclk() to intel_display.c
On Tue, 7 Jan 2014 18:01:33 +0200
Jani Nikula wrote:
> Make it available outside of intel_dp.c.
>
> Signed-off-by: Jani Nikula
> ---
> drivers/gpu/drm/i915/intel_display.c | 33 +
> drivers/gpu/drm/i915/intel_dp.c | 34
> --
> drivers/gpu/drm/i915/intel_drv.h |1 +
> 3 files changed, 34 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index a562eef..e784feb 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -78,6 +78,39 @@ intel_pch_rawclk(struct drm_device *dev)
> return I915_READ(PCH_RAWCLK_FREQ) & RAWCLK_FREQ_MASK;
> }
>
> +/* hrawclock is 1/4 the FSB frequency */
> +int intel_hrawclk(struct drm_device *dev)
> +{
> + struct drm_i915_private *dev_priv = dev->dev_private;
> + uint32_t clkcfg;
> +
> + /* There is no CLKCFG reg in Valleyview. VLV hrawclk is 200 MHz */
> + if (IS_VALLEYVIEW(dev))
> + return 200;
> +
> + clkcfg = I915_READ(CLKCFG);
> + switch (clkcfg & CLKCFG_FSB_MASK) {
> + case CLKCFG_FSB_400:
> + return 100;
> + case CLKCFG_FSB_533:
> + return 133;
> + case CLKCFG_FSB_667:
> + return 166;
> + case CLKCFG_FSB_800:
> + return 200;
> + case CLKCFG_FSB_1067:
> + return 266;
> + case CLKCFG_FSB_1333:
> + return 333;
> + /* these two are just a guess; one of them might be right */
> + case CLKCFG_FSB_1600:
> + case CLKCFG_FSB_1600_ALT:
> + return 400;
> + default:
> + return 133;
> + }
> +}
> +
> static inline u32 /* units of 100MHz */
> intel_fdi_link_freq(struct drm_device *dev)
> {
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 7df5085..a36a2a3 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -203,40 +203,6 @@ unpack_aux(uint32_t src, uint8_t *dst, int dst_bytes)
> dst[i] = src >> ((3-i) * 8);
> }
>
> -/* hrawclock is 1/4 the FSB frequency */
> -static int
> -intel_hrawclk(struct drm_device *dev)
> -{
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - uint32_t clkcfg;
> -
> - /* There is no CLKCFG reg in Valleyview. VLV hrawclk is 200 MHz */
> - if (IS_VALLEYVIEW(dev))
> - return 200;
> -
> - clkcfg = I915_READ(CLKCFG);
> - switch (clkcfg & CLKCFG_FSB_MASK) {
> - case CLKCFG_FSB_400:
> - return 100;
> - case CLKCFG_FSB_533:
> - return 133;
> - case CLKCFG_FSB_667:
> - return 166;
> - case CLKCFG_FSB_800:
> - return 200;
> - case CLKCFG_FSB_1067:
> - return 266;
> - case CLKCFG_FSB_1333:
> - return 333;
> - /* these two are just a guess; one of them might be right */
> - case CLKCFG_FSB_1600:
> - case CLKCFG_FSB_1600_ALT:
> - return 400;
> - default:
> - return 133;
> - }
> -}
> -
> static void
> intel_dp_init_panel_power_sequencer(struct drm_device *dev,
> struct intel_dp *intel_dp,
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 46aea6c..9c5e984 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -626,6 +626,7 @@ void intel_ddi_get_config(struct intel_encoder *encoder,
>
> /* intel_display.c */
> int intel_pch_rawclk(struct drm_device *dev);
> +int intel_hrawclk(struct drm_device *dev);
> void intel_mark_busy(struct drm_device *dev);
> void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
> struct intel_ring_buffer *ring);
Reviewed-by: Jesse Barnes
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 6/7] drm/i915: vlv: fix mapping of pipestat enable to status bits
On Tue, Feb 04, 2014 at 09:35:50PM +0200, Imre Deak wrote:
> At least on VLV we can't get at the pipestat status bits by simply right
> shifting the corresponding enable bits. The mapping between enable and
> status bits for the sprite0,1 flip done and the PSR events don't follow
> this rule, so we need to map them separately.
>
> The PSR enable for pipe A is DPFLIPSTAT[22], but I haven't added support
> for this, since there is no user of it atm. Until support is added WARN
> if someone tries to enable PSR interrupts, or tries to enable the same
> (1 << 6) bit on pipe B, which MBZ.
>
> Signed-off-by: Imre Deak
> ---
> drivers/gpu/drm/i915/i915_irq.c | 36 +++-
> drivers/gpu/drm/i915/i915_reg.h | 3 +++
> 2 files changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index ec56a70..eea5398 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -471,9 +471,29 @@ done:
> return ret;
> }
>
> +static void set_pipestat_enable_bit(u32 status_mask, u32 status_bit,
> + u32 enable_bit, u32 *enable_mask)
> +{
> + if (status_mask & status_bit)
> + *enable_mask |= enable_bit;
> + else
> + *enable_mask &= ~enable_bit;
> +}
> +
> static u32 pipestat_to_enable_mask(struct drm_device *dev, u32 status_mask)
> {
> - return status_mask << 16;
> + u32 enable_mask = status_mask << 16;
> +
> + if (!IS_VALLEYVIEW(dev))
> + return enable_mask;
> +
> + enable_mask &= ~PIPE_FIFO_UNDERRUN_STATUS;
> + set_pipestat_enable_bit(status_mask, SPRITE1_FLIP_DONE_INT_STATUS_VLV,
> + SPRITE1_FLIP_DONE_INT_EN_VLV, &enable_mask);
> + set_pipestat_enable_bit(enable_mask, SPRITE0_FLIP_DONE_INT_STATUS_VLV,
> + SPRITE0_FLIP_DONE_INT_EN_VLV, &enable_mask);
This feels a bit subtle to me. Maybe do it in a bit more straightforward
way? Eg.:
enable_mask &= ~(PIPE_FIFO_UNDERRUN_STATUS |
SPRITE0_FLIP_DONE_INT_EN_VLV |
SPRITE1_FLIP_DONE_INT_EN_VLV);
if (status_mask & SPRITE0_FLIP_DONE_INT_STATUS_VLV)
enable_mask |= SPRITE0_FLIP_DONE_INT_EN_VLV;
if (status_mask & SPRITE1_FLIP_DONE_INT_STATUS_VLV)
enable_mask |= SPRITE1_FLIP_DONE_INT_EN_VLV;
> +
> + return enable_mask;
> }
>
> void
> @@ -489,6 +509,13 @@ i915_enable_pipestat(drm_i915_private_t *dev_priv, enum
> pipe pipe,
> if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK))
> return;
>
> + /*
> + * On pipe A we don't support the PSR interrupt yet, on pipe B the
> + * same bit MBZ.
> + */
> + if (WARN_ON_ONCE(status_mask & PIPE_A_PSR_STATUS_VLV))
> + return;
Won't this break the BLC interrupt on gen3/4?
> +
> enable_mask = pipestat_to_enable_mask(dev_priv->dev, status_mask);
> if ((pipestat & enable_mask) == enable_mask)
> return;
> @@ -512,6 +539,13 @@ i915_disable_pipestat(drm_i915_private_t *dev_priv, enum
> pipe pipe,
> if (WARN_ON_ONCE(status_mask & ~PIPESTAT_INT_STATUS_MASK))
> return;
>
> + /*
> + * On pipe A we don't support the PSR interrupt yet, on pipe B the
> + * same bit MBZ.
> + */
> + if (WARN_ON_ONCE(status_mask & PIPE_A_PSR_STATUS_VLV))
> + return;
> +
> enable_mask = pipestat_to_enable_mask(dev_priv->dev, status_mask);
> if ((pipestat & enable_mask) == 0)
> return;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 599c7f6..c998c4f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3240,6 +3240,7 @@
> #define PIPE_LEGACY_BLC_EVENT_ENABLE (1UL<<22)
> #define PIPE_ODD_FIELD_INTERRUPT_ENABLE(1UL<<21)
> #define PIPE_EVEN_FIELD_INTERRUPT_ENABLE (1UL<<20)
> +#define PIPE_B_PSR_INTERRUPT_ENABLE_VLV(1UL<<19)
> #define PIPE_HOTPLUG_TV_INTERRUPT_ENABLE (1UL<<18) /* pre-965 */
> #define PIPE_START_VBLANK_INTERRUPT_ENABLE (1UL<<18) /* 965 or later */
> #define PIPE_VBLANK_INTERRUPT_ENABLE (1UL<<17)
> @@ -3256,8 +3257,10 @@
> #define PIPE_DISPLAY_LINE_COMPARE_STATUS (1UL<<8)
> #define PIPE_DPST_EVENT_STATUS (1UL<<7)
> #define PIPE_LEGACY_BLC_EVENT_STATUS (1UL<<6)
> +#define PIPE_A_PSR_STATUS_VLV (1UL<<6)
> #define PIPE_ODD_FIELD_INTERRUPT_STATUS(1UL<<5)
> #define PIPE_EVEN_FIELD_INTERRUPT_STATUS (1UL<<4)
> +#define PIPE_B_PSR_STATUS_VLV (1UL<<3)
> #define PIPE_HOTPLUG_TV_INTERRUPT_STATUS (1UL<<2) /* pre-965 */
> #define PIPE_START_VBLANK_INTERRUPT_STATUS (1UL<<2) /* 965 or later */
> #define PIPE_VBLANK_INTERRUPT_STATUS (1UL<<1)
> --
> 1.8.4
>
> ___
> Intel-gfx m
Re: [Intel-gfx] [RFC][PATCH] Userptr benchmark
On Wed, Feb 05, 2014 at 12:41:45PM +, Tvrtko Ursulin wrote: > From: Tvrtko Ursulin > > A simple userptr benchmark measuring creation and destruction of userptr > surfaces and also impact of having a different number of them in the > process address space. > > Example test output from i7-4550U running Android is below. > > Questions, comments and ideas are welcome. One silly idea I wanted to test was that speed of read/write access to ptr was not affected by wrapping it up in a userptr. > IGT-Version: 1.5-NOT-GIT (android-ia) (Linux: 3.10.20-g667dce8-dirty x86_64) unsync vs sync - unsync is much faster at creating userptr (not having to hook up and search the mmu-notifier) - unsync is reasonably faster for destroying userptr - there is no scaling issue with unsync, and minor (log(n)) scaling factor for sync That seems in line with our expectations of i915_gem_userptr.c, which is reassuring. -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/13] drm/i915: Initial command parser table definitions
N.B. I'll likely make multiple passes on the patches while reviewing,
for example I did not check any of the #define values here.
On Wed, 29 Jan 2014, bradley.d.vol...@intel.com wrote:
> From: Brad Volkin
>
> Add command tables defining irregular length commands for each ring.
> This requires a few new command opcode definitions.
>
> OTC-Tracker: AXIA-4631
> Change-Id: I064bceb457e15f46928058352afe76d918c58ef5
> Signed-off-by: Brad Volkin
> ---
> drivers/gpu/drm/i915/i915_cmd_parser.c | 157
> +
> drivers/gpu/drm/i915/i915_reg.h| 46 ++
> 2 files changed, 203 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_cmd_parser.c
> b/drivers/gpu/drm/i915/i915_cmd_parser.c
> index 7639dbc..2e27bad 100644
> --- a/drivers/gpu/drm/i915/i915_cmd_parser.c
> +++ b/drivers/gpu/drm/i915/i915_cmd_parser.c
> @@ -27,6 +27,148 @@
>
> #include "i915_drv.h"
>
> +#define STD_MI_OPCODE_MASK 0xFF80
> +#define STD_3D_OPCODE_MASK 0x
> +#define STD_2D_OPCODE_MASK 0xFFC0
> +#define STD_MFX_OPCODE_MASK 0x
> +
> +#define CMD(op, opm, f, lm, fl, ...) \
> + { \
> + .flags = (fl) | (f),\
Sparse gives me
drivers/gpu/drm/i915/i915_cmd_parser.c:64:9: warning: dubious: x | !y
for the !F cases (bitwise OR with a logical NOT). I can see it's not a
bug here, but we want to keep those warnings down. Maybe just s/!F/0/ in
the tables? Or make the f argument to CMD a boolean, and make that
.flags = (fl) | (f ? CMD_DESC_FIXED : 0),
> + .cmd = { (op), (opm) }, \
> + .length = { (lm) }, \
> + __VA_ARGS__ \
> + }
> +
> +/* Convenience macros to compress the tables */
> +#define SMI STD_MI_OPCODE_MASK
> +#define S3D STD_3D_OPCODE_MASK
> +#define S2D STD_2D_OPCODE_MASK
> +#define SMFX STD_MFX_OPCODE_MASK
> +#define F CMD_DESC_FIXED
> +#define S CMD_DESC_SKIP
> +#define R CMD_DESC_REJECT
> +#define W CMD_DESC_REGISTER
> +#define B CMD_DESC_BITMASK
> +#define M CMD_DESC_MASTER
> +
> +/*Command Mask Fixed Len Action
> + -- */
> +static const struct drm_i915_cmd_descriptor common_cmds[] = {
> + CMD( MI_NOOP, SMI,F, 1, S ),
> + CMD( MI_USER_INTERRUPT,SMI,F, 1, S ),
> + CMD( MI_WAIT_FOR_EVENT,SMI,F, 1, S ),
> + CMD( MI_ARB_CHECK, SMI,F, 1, S ),
> + CMD( MI_REPORT_HEAD, SMI,F, 1, S ),
> + CMD( MI_SUSPEND_FLUSH, SMI,F, 1, S ),
> + CMD( MI_SEMAPHORE_MBOX,SMI, !F, 0xFF, S ),
> + CMD( MI_STORE_DWORD_INDEX, SMI, !F, 0xFF, S ),
> + CMD( MI_LOAD_REGISTER_IMM(1), SMI, !F, 0xFF, S ),
> + CMD( MI_STORE_REGISTER_MEM(1), SMI, !F, 0xFF, S ),
> + CMD( MI_LOAD_REGISTER_MEM, SMI, !F, 0xFF, S ),
> + CMD( MI_BATCH_BUFFER_START,SMI, !F, 0xFF, S ),
> +};
> +
> +static const struct drm_i915_cmd_descriptor render_cmds[] = {
> + CMD( MI_FLUSH, SMI,F, 1, S ),
> + CMD( MI_ARB_ON_OFF,SMI,F, 1, S ),
> + CMD( MI_PREDICATE, SMI,F, 1, S ),
> + CMD( MI_TOPOLOGY_FILTER, SMI,F, 1, S ),
> + CMD( MI_DISPLAY_FLIP, SMI, !F, 0xFF, S ),
> + CMD( MI_SET_CONTEXT, SMI, !F, 0xFF, S ),
> + CMD( MI_URB_CLEAR, SMI, !F, 0xFF, S ),
> + CMD( MI_UPDATE_GTT,SMI, !F, 0xFF, S ),
> + CMD( MI_CLFLUSH, SMI, !F, 0x3FF, S ),
> + CMD( MI_CONDITIONAL_BATCH_BUFFER_END, SMI, !F, 0xFF, S ),
> + CMD( GFX_OP_3DSTATE_VF_STATISTICS, S3D,F, 1, S ),
> + CMD( PIPELINE_SELECT, S3D,F, 1, S ),
> + CMD( GPGPU_OBJECT, S3D, !F, 0xFF, S ),
> + CMD( GPGPU_WALKER, S3D, !F, 0xFF, S ),
> + CMD( GFX_OP_3DSTATE_SO_DECL_LIST, S3D, !F, 0x1FF, S ),
> +};
> +
> +static const struct drm_i915_cmd_descriptor hsw_render_cmds[] = {
> + CMD( MI_SET_PREDICATE, SMI,F, 1, S ),
> + CMD( MI_RS_CONTROL,SMI,F, 1, S ),
> + CMD( MI_URB_ATOMIC_ALLOC, SMI,F, 1, S ),
> + CMD( MI_RS_CONTEXT,SMI,F, 1, S ),
> + CMD( MI_LOAD_REGISTER_REG, SMI, !F, 0xFF, S ),
> + CMD( MI_RS_STORE_DATA_IMM, SMI, !F, 0xFF, S ),
> + CMD( MI_LOAD_URB_MEM, SMI, !F, 0xF
[Intel-gfx] [PATCH 0/2] drm/i915: Potential gen3 vblank interrupt fixes
From: Ville Syrjälä The AGBBUSY# bit in INSTPM caught my eye while reading the code and having the spec open at the same time. This series makes the code match the spec, and also closes a potential race condition (which may or may not be real, depending on how the hardware works). Ville Syrjälä (2): drm/i915: Frob AGPBUSY# bit before enabling the vblank interrupt drm/i915: Flip the sense of AGPBUSY_DIS bit drivers/gpu/drm/i915/i915_irq.c | 14 +- drivers/gpu/drm/i915/i915_reg.h | 2 +- 2 files changed, 10 insertions(+), 6 deletions(-) -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 2/2] drm/i915: Flip the sense of AGPBUSY_DIS bit
From: Ville Syrjälä My Gen3 Bspec lists the AGPBUSY# bit in INSTPM as an enable bit rather than a disable bit. Our code has the opposite idea. Make the code match the spec. Might fix some gen3 C3 related interrupt delivery problems. Untested due to lack of hardware. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_irq.c | 4 ++-- drivers/gpu/drm/i915/i915_reg.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 369f517..0a7a4af 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2264,7 +2264,7 @@ static int i915_enable_vblank(struct drm_device *dev, int pipe) /* maintain vblank delivery even in deep C-states */ if (dev_priv->info->gen == 3) - I915_WRITE(INSTPM, _MASKED_BIT_DISABLE(INSTPM_AGPBUSY_DIS)); + I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_AGPBUSY_EN)); if (INTEL_INFO(dev)->gen >= 4) i915_enable_pipestat(dev_priv, pipe, @@ -2349,7 +2349,7 @@ static void i915_disable_vblank(struct drm_device *dev, int pipe) PIPE_START_VBLANK_INTERRUPT_ENABLE); if (dev_priv->info->gen == 3) - I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_AGPBUSY_DIS)); + I915_WRITE(INSTPM, _MASKED_BIT_DISABLE(INSTPM_AGPBUSY_EN)); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); } diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h index 9d0f4f7..1c73a45 100644 --- a/drivers/gpu/drm/i915/i915_reg.h +++ b/drivers/gpu/drm/i915/i915_reg.h @@ -838,7 +838,7 @@ #define I915_ERROR_INSTRUCTION (1<<0) #define INSTPM 0x020c0 #define INSTPM_SELF_EN (1<<12) /* 915GM only */ -#define INSTPM_AGPBUSY_DIS (1<<11) /* gen3: when disabled, pending interrupts +#define INSTPM_AGPBUSY_EN (1<<11) /* gen3: when disabled, pending interrupts will not assert AGPBUSY# and will only be delivered when out of C3. */ #define INSTPM_FORCE_ORDERING(1<<7) /* GEN6+ */ -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/i915: Frob AGPBUSY# bit before enabling the vblank interrupt
From: Ville Syrjälä Not sure what would happen if an interrupt is generated between enabling the interrupt in PIPESTAT and frobbing the AGPBUSY# bit in INSTPM. Would the already pending interrupt cause exit from C3 or not? Let's not play such guessing games, and simply flip AGPBUSY# bit before enabling the interrupt. Do the opposite when disabling the interrupt, just for symmetry. Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/i915_irq.c | 14 +- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c index 56edff3..369f517 100644 --- a/drivers/gpu/drm/i915/i915_irq.c +++ b/drivers/gpu/drm/i915/i915_irq.c @@ -2261,6 +2261,11 @@ static int i915_enable_vblank(struct drm_device *dev, int pipe) return -EINVAL; spin_lock_irqsave(&dev_priv->irq_lock, irqflags); + + /* maintain vblank delivery even in deep C-states */ + if (dev_priv->info->gen == 3) + I915_WRITE(INSTPM, _MASKED_BIT_DISABLE(INSTPM_AGPBUSY_DIS)); + if (INTEL_INFO(dev)->gen >= 4) i915_enable_pipestat(dev_priv, pipe, PIPE_START_VBLANK_INTERRUPT_ENABLE); @@ -2268,9 +2273,6 @@ static int i915_enable_vblank(struct drm_device *dev, int pipe) i915_enable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_ENABLE); - /* maintain vblank delivery even in deep C-states */ - if (dev_priv->info->gen == 3) - I915_WRITE(INSTPM, _MASKED_BIT_DISABLE(INSTPM_AGPBUSY_DIS)); spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); return 0; @@ -2341,12 +2343,14 @@ static void i915_disable_vblank(struct drm_device *dev, int pipe) unsigned long irqflags; spin_lock_irqsave(&dev_priv->irq_lock, irqflags); - if (dev_priv->info->gen == 3) - I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_AGPBUSY_DIS)); i915_disable_pipestat(dev_priv, pipe, PIPE_VBLANK_INTERRUPT_ENABLE | PIPE_START_VBLANK_INTERRUPT_ENABLE); + + if (dev_priv->info->gen == 3) + I915_WRITE(INSTPM, _MASKED_BIT_ENABLE(INSTPM_AGPBUSY_DIS)); + spin_unlock_irqrestore(&dev_priv->irq_lock, irqflags); } -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] Haswell DisplayPort Multi Stream Transport status
On Wed, Jan 22, 2014 at 12:53:50PM +0900, Mike Hommey wrote: > Hi, > > What is the current status for DP MST support on Haswell? Are there > experimental patches that can be tested? If not, what can be done to > help progress? No patches to test yet. I would gate DP MST on the current DP aux refactoring that Thierry Reding (core) and Jani Nikula (i915) are doing at the moment. Once done, you need someone with a DP 1.2 spec to: - implement the cross-drivers features (a DP aux bus, handle device discovery, ...) - implement the i915 bits - Figure out how to expose and do a modeset on those devices in the current CRTC/encoder/connector model. > Supposing the kernel parts are figured, will userland need updates > accordingly, or would the existing multi-head support work as-is? Yes, userland will most likely need updates. Not a small task by any definition. -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: don't double-restore ARB mode bits for kms
On Wed, Feb 5, 2014 at 2:01 PM, Jani Nikula wrote:
> On Thu, 16 Jan 2014, Daniel Vetter wrote:
>> On Thu, Jan 16, 2014 at 5:54 PM, Paulo Zanoni wrote:
>>> 2014/1/16 Daniel Vetter :
Our init_clock_gating functions and related code should already take
care of this. And if they don't we'd better know.
>>>
>>> For both registers, I see functions applying specific workarounds, but
>>> they only do the read-write-modify through _MASKED_BIT_ENABLE(). I
>>> don't see anybody explicitly fully initializing the registers
>>> anywhere. So we go with whatever the BIOS gives us at boot + our
>>> changes to a few specific registers. At resume, we don't know so far
>>> what we'll get, so I fear this patch may in fact cause a regression.
>>
>> That's part of the risk of it, but fixing those is a simple matter of
>> comparing register dumps. These two bits here are one of the very few
>> holdouts we have that depend upon the register save/restore code, and
>> I want to remove them. Since thus far this was a really good way to
>> hide random bugs.
>
> I'm in favour of the patch in general, but I spotted at least
>
> /* On GEN3 we really need to make sure the ARB C3 LP bit is set */
> if (IS_GEN3(dev)) {
> I915_WRITE(MI_ARB_STATE,
>_MASKED_BIT_ENABLE(MI_ARB_C3_LP_WRITE_ENABLE));
> }
>
> in i915_gem_load() that won't be set on a suspend/resume cycle. Is that
> going to be problem?
Yes, good catch. Somehow I've missed this when grepping.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: don't double-restore ARB mode bits for kms
On Thu, 16 Jan 2014, Daniel Vetter wrote:
> On Thu, Jan 16, 2014 at 5:54 PM, Paulo Zanoni wrote:
>> 2014/1/16 Daniel Vetter :
>>> Our init_clock_gating functions and related code should already take
>>> care of this. And if they don't we'd better know.
>>
>> For both registers, I see functions applying specific workarounds, but
>> they only do the read-write-modify through _MASKED_BIT_ENABLE(). I
>> don't see anybody explicitly fully initializing the registers
>> anywhere. So we go with whatever the BIOS gives us at boot + our
>> changes to a few specific registers. At resume, we don't know so far
>> what we'll get, so I fear this patch may in fact cause a regression.
>
> That's part of the risk of it, but fixing those is a simple matter of
> comparing register dumps. These two bits here are one of the very few
> holdouts we have that depend upon the register save/restore code, and
> I want to remove them. Since thus far this was a really good way to
> hide random bugs.
I'm in favour of the patch in general, but I spotted at least
/* On GEN3 we really need to make sure the ARB C3 LP bit is set */
if (IS_GEN3(dev)) {
I915_WRITE(MI_ARB_STATE,
_MASKED_BIT_ENABLE(MI_ARB_C3_LP_WRITE_ENABLE));
}
in i915_gem_load() that won't be set on a suspend/resume cycle. Is that
going to be problem?
BR,
Jani.
--
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [RFC][PATCH] Userptr benchmark
From: Tvrtko Ursulin A simple userptr benchmark measuring creation and destruction of userptr surfaces and also impact of having a different number of them in the process address space. Example test output from i7-4550U running Android is below. Questions, comments and ideas are welcome. IGT-Version: 1.5-NOT-GIT (android-ia) (Linux: 3.10.20-g667dce8-dirty x86_64) create-destroy= 258270 op/s multi-create-destroy = 2803 op/s multi-create-destroy-random = 2777 op/s Subtest userptr-unsync: SUCCESS malloc-free,0 bos = 7463 op/s malloc-free-random 0 bos = 5911 op/s malloc-realloc-free,0 bos = 2427 op/s malloc-realloc-free-random, 0 bos = 669 op/s mmap-unmap, 0 bos = 636 op/s mmap-unmap-random, 0 bos = 561 op/s malloc-free,1 bos = 7462 op/s malloc-free-random 1 bos = 5915 op/s malloc-realloc-free,1 bos = 2444 op/s malloc-realloc-free-random, 1 bos = 669 op/s mmap-unmap, 1 bos = 594 op/s mmap-unmap-random, 1 bos = 531 op/s malloc-free, 10 bos = 7464 op/s malloc-free-random 10 bos = 5915 op/s malloc-realloc-free, 10 bos = 2444 op/s malloc-realloc-free-random,10 bos = 669 op/s mmap-unmap,10 bos = 606 op/s mmap-unmap-random, 10 bos = 542 op/s malloc-free, 100 bos = 7462 op/s malloc-free-random100 bos = 5913 op/s malloc-realloc-free, 100 bos = 2444 op/s malloc-realloc-free-random, 100 bos = 669 op/s mmap-unmap, 100 bos = 613 op/s mmap-unmap-random,100 bos = 546 op/s malloc-free, 1000 bos = 7437 op/s malloc-free-random 1000 bos = 5908 op/s malloc-realloc-free, 1000 bos = 2442 op/s malloc-realloc-free-random, 1000 bos = 669 op/s mmap-unmap, 1000 bos = 600 op/s mmap-unmap-random, 1000 bos = 535 op/s Subtest userptr-impact-unsync: SUCCESS create-destroy=67453 op/s multi-create-destroy = 1837 op/s multi-create-destroy-random = 1817 op/s Subtest userptr-sync: SUCCESS malloc-free,0 bos = 7362 op/s malloc-free-random 0 bos = 5913 op/s malloc-realloc-free,0 bos = 2444 op/s malloc-realloc-free-random, 0 bos = 670 op/s mmap-unmap, 0 bos = 512 op/s mmap-unmap-random, 0 bos = 465 op/s malloc-free,1 bos = 7463 op/s malloc-free-random 1 bos = 5913 op/s malloc-realloc-free,1 bos = 2445 op/s malloc-realloc-free-random, 1 bos = 669 op/s mmap-unmap, 1 bos = 447 op/s mmap-unmap-random, 1 bos = 403 op/s malloc-free, 10 bos = 7455 op/s malloc-free-random 10 bos = 5911 op/s malloc-realloc-free, 10 bos = 2444 op/s malloc-realloc-free-random,10 bos = 669 op/s mmap-unmap,10 bos = 452 op/s mmap-unmap-random, 10 bos = 414 op/s malloc-free, 100 bos = 7462 op/s malloc-free-random100 bos = 5915 op/s malloc-realloc-free, 100 bos = 2442 op/s malloc-realloc-free-random, 100 bos = 668 op/s mmap-unmap, 100 bos = 453 op/s mmap-unmap-random,100 bos = 416 op/s malloc-free, 1000 bos = 7462 op/s malloc-free-random 1000 bos = 5907 op/s malloc-realloc-free, 1000 bos = 2441 op/s malloc-realloc-free-random, 1000 bos = 667 op/s mmap-unmap, 1000 bos = 436 op/s mmap-unmap-random, 1000 bos = 400 op/s Subtest userptr-impact-sync: SUCCESS Tvrtko Ursulin (1): tests/gem_userptr_benchmark: Benchmarking userptr surfaces and impact tests/.gitignore | 1 + tests/Makefile.sources| 1 + tests/gem_userptr_benchmark.c | 451 ++ 3 files changed, 453 insertions(+) create mode 100644 tests/gem_userptr_benchmark.c -- 1.8.4.3 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] tests/gem_userptr_benchmark: Benchmarking userptr surfaces and impact
From: Tvrtko Ursulin
This adds a small benchmark for the new userptr functionality.
Apart from basic surface creation and destruction, also tested is the
impact of having userptr surfaces in the process address space. Reason
for that is the impact of MMU notifiers on common address space
operations like munmap() which is per process.
Signed-off-by: Tvrtko Ursulin
Cc: Chris Wilson
---
tests/.gitignore | 1 +
tests/Makefile.sources| 1 +
tests/gem_userptr_benchmark.c | 451 ++
3 files changed, 453 insertions(+)
create mode 100644 tests/gem_userptr_benchmark.c
diff --git a/tests/.gitignore b/tests/.gitignore
index 1a3b9d1..125bc88 100644
--- a/tests/.gitignore
+++ b/tests/.gitignore
@@ -93,6 +93,7 @@ gem_tiling_max_stride
gem_unfence_active_buffers
gem_unref_active_buffers
gem_userptr_blits
+gem_userptr_benchmark
gem_wait_render_timeout
gem_write_read_ring_switch
gen3_mixed_blits
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index c69e0e3..7be539d 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -117,6 +117,7 @@ TESTS_progs = \
gem_unfence_active_buffers \
gem_unref_active_buffers \
gem_userptr_blits \
+ gem_userptr_benchmark \
gem_wait_render_timeout \
gen3_mixed_blits \
gen3_render_linear_blits \
diff --git a/tests/gem_userptr_benchmark.c b/tests/gem_userptr_benchmark.c
new file mode 100644
index 000..32da02c
--- /dev/null
+++ b/tests/gem_userptr_benchmark.c
@@ -0,0 +1,451 @@
+/*
+ * Copyright © 2014 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:
+ *Tvrtko Ursulin
+ *
+ */
+
+/** @file gem_userptr_benchmark.c
+ *
+ * Benchmark the userptr code and impact of having userptr surfaces
+ * in process address space on some normal operations.
+ *
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include "drm.h"
+#include "i915_drm.h"
+#include "drmtest.h"
+#include "intel_bufmgr.h"
+#include "intel_batchbuffer.h"
+#include "intel_gpu_tools.h"
+
+#define WIDTH 128
+#define HEIGHT 128
+#define PAGE_SIZE 4096
+
+#define LOCAL_I915_GEM_USERPTR 0x34
+#define LOCAL_IOCTL_I915_GEM_USERPTR DRM_IOWR (DRM_COMMAND_BASE +
LOCAL_I915_GEM_USERPTR, struct local_i915_gem_userptr)
+struct local_i915_gem_userptr {
+ uint64_t user_ptr;
+ uint64_t user_size;
+ uint32_t flags;
+#define I915_USERPTR_READ_ONLY (1<<0)
+#define I915_USERPTR_UNSYNCHRONIZED (1<<31)
+ uint32_t handle;
+};
+
+static uint32_t userptr_flags;
+
+static uint32_t linear[WIDTH*HEIGHT];
+
+static void gem_userptr_test_unsynchronized(void)
+{
+ userptr_flags = I915_USERPTR_UNSYNCHRONIZED;
+}
+
+static void gem_userptr_test_synchronized(void)
+{
+ userptr_flags = 0;
+}
+
+static int gem_userptr(int fd, void *ptr, int size, int read_only, uint32_t
*handle)
+{
+ struct local_i915_gem_userptr userptr;
+ int ret;
+
+ userptr.user_ptr = (uintptr_t)ptr;
+ userptr.user_size = size;
+ userptr.flags = userptr_flags;
+ if (read_only)
+ userptr.flags |= I915_USERPTR_READ_ONLY;
+
+ ret = drmIoctl(fd, LOCAL_IOCTL_I915_GEM_USERPTR, &userptr);
+ if (ret)
+ ret = errno;
+ igt_skip_on_f(ret == ENODEV &&
+ (userptr_flags & I915_USERPTR_UNSYNCHRONIZED) == 0,
+ "Skipping, synchronized mappings with no kernel
CONFIG_MMU_NOTIFIER?");
+ if (ret == 0)
+ *handle = userptr.handle;
+
+ return ret;
+}
+
+static uint32_t
+create_userptr(int fd, uint32_t val, uint32_t *ptr)
+{
+ uint32_t handle;
+ int i, ret;
+
+ ret = gem_userptr(fd, ptr, sizeof(linear), 0, &handle);
+ igt_assert(ret == 0);
+ igt_assert(handle != 0);
+
+ /* Fill the BO with dword
Re: [Intel-gfx] [PATCH] drm/i915: Add a comment about WIZ hashing vs. thread counts
On Wed, Feb 05, 2014 at 12:43:47PM +0200, ville.syrj...@linux.intel.com wrote: > From: Ville Syrjälä > > Add a comment next to our WIZ hashing setup to remind people about the > link between WIZ hashing disable bit and PS/WM thread counts. > > Suggested-by: Chris Wilson > Signed-off-by: Ville Syrjälä Thanks, now hopefully I won't make the same mistake again in future. Reviewed-by: Chris Wilson -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Add a comment about WIZ hashing vs. thread counts
From: Ville Syrjälä Add a comment next to our WIZ hashing setup to remind people about the link between WIZ hashing disable bit and PS/WM thread counts. Suggested-by: Chris Wilson Signed-off-by: Ville Syrjälä --- drivers/gpu/drm/i915/intel_pm.c | 16 1 file changed, 16 insertions(+) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index c3a1362..a3b61ca 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4647,6 +4647,10 @@ static void gen6_init_clock_gating(struct drm_device *dev) /* * BSpec recoomends 8x4 when MSAA is used, * however in practice 16x4 seems fastest. +* +* Note that PS/WM thread counts depend on the WIZ hashing +* disable bit, which we don't touch here, but it's good +* to keep in mind (see 3DSTATE_PS and 3DSTATE_WM). */ I915_WRITE(GEN6_GT_MODE, GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4); @@ -4830,6 +4834,10 @@ static void gen8_init_clock_gating(struct drm_device *dev) /* * BSpec recommends 8x4 when MSAA is used, * however in practice 16x4 seems fastest. +* +* Note that PS/WM thread counts depend on the WIZ hashing +* disable bit, which we don't touch here, but it's good +* to keep in mind (see 3DSTATE_PS and 3DSTATE_WM). */ I915_WRITE(GEN7_GT_MODE, GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4); @@ -4866,6 +4874,10 @@ static void haswell_init_clock_gating(struct drm_device *dev) /* * BSpec recommends 8x4 when MSAA is used, * however in practice 16x4 seems fastest. +* +* Note that PS/WM thread counts depend on the WIZ hashing +* disable bit, which we don't touch here, but it's good +* to keep in mind (see 3DSTATE_PS and 3DSTATE_WM). */ I915_WRITE(GEN7_GT_MODE, GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4); @@ -4954,6 +4966,10 @@ static void ivybridge_init_clock_gating(struct drm_device *dev) /* * BSpec recommends 8x4 when MSAA is used, * however in practice 16x4 seems fastest. +* +* Note that PS/WM thread counts depend on the WIZ hashing +* disable bit, which we don't touch here, but it's good +* to keep in mind (see 3DSTATE_PS and 3DSTATE_WM). */ I915_WRITE(GEN7_GT_MODE, GEN6_WIZ_HASHING_MASK | GEN6_WIZ_HASHING_16x4); -- 1.8.3.2 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [RFC 00/22] Gen7 batch buffer command parser
On Tue, Nov 26, 2013 at 08:51:17AM -0800, bradley.d.vol...@intel.com wrote: > From: Brad Volkin > > Certain OpenGL features (e.g. transform feedback, performance monitoring) > require userspace code to submit batches containing commands such as > MI_LOAD_REGISTER_IMM to access various registers. Unfortunately, some > generations of the hardware will noop these commands in "unsecure" batches > (which includes all userspace batches submitted via i915) even though the > commands may be safe and represent the intended programming model of the > device. > > This series introduces a software command parser similar in operation to the > command parsing done in hardware for unsecure batches. However, the software > parser allows some operations that would be noop'd by hardware, if the parser > determines the operation is safe, and submits the batch as "secure" to prevent > hardware parsing. Currently the series implements this on IVB and HSW. Just one more question... Do you have a branch for people to test? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/7] drm/i915: Fix SNB GT_MODE register setup
On Wed, Feb 05, 2014 at 11:27:31AM +0200, Ville Syrjälä wrote: > On Tue, Feb 04, 2014 at 09:23:06PM +, Chris Wilson wrote: > > On Tue, Feb 04, 2014 at 09:59:15PM +0200, ville.syrj...@linux.intel.com > > wrote: > > > From: Ville Syrjälä > > > > > > On SNB we set up WaSetupGtModeTdRowDispatch:snb early in > > > gen6_init_clock_gating(). That sets a bit in the GEN6_GT_MODE register. > > > However later we go and disable all the bits in the same register. And > > > then we go on to set some other bit. So apparently we never actually > > > implemented this workaround since the "disable all bits" part was there > > > already before the w/a got supposedly implemented. > > > > > > These are the relevant commits: > > > > > > commit 6547fbdbfff62c99e4f7b4f985ff8b3454f33b0f > > > Author: Daniel Vetter > > > Date: Fri Dec 14 23:38:29 2012 +0100 > > > > > > drm/i915: Implement WaSetupGtModeTdRowDispatch > > > > > > commit f8f2ac9a76b0f80a6763ca316116a7bab8486997 > > > Author: Ben Widawsky > > > Date: Wed Oct 3 19:34:24 2012 -0700 > > > > > > drm/i915: Fix GT_MODE default value > > > > > > So, let's drop the "disable all bits" part, move both writes to > > > closer proxomity to each other, and name the WIZ hashing bits > > > appropriately. BSpec is still a bit confused how the bits should > > > actually be interpreted, but I took the the description for the > > > high bit since the low bit part only lists values for a single bit. > > > > > > Also add a comment about our choice of WIZ hashing mode. > > > > Changing WiZ hashing mode changes the valid number of threads and > > userspace assumes best case (WiZ disabled). Worst case we start hanging > > the chip. > > > > I have no idea how relevant that piece of lore from the spec is, but it > > something to be wary of when making these changes. > > The thread numbers seem to depend only on the WIZ hashing disable bit. > I'm not touching that one. That's ok then. Can I ask that we have a comment next to the WIZ setup noting userspace's dependence on the WIZ-disable bit? -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] MAINTAINERS: Update drm/i915 git repo
On Wed, Feb 05, 2014 at 08:54:15AM +0100, Daniel Vetter wrote: > On Tue, Feb 4, 2014 at 8:37 PM, Daniel Vetter wrote: > > On Tue, Feb 4, 2014 at 8:00 PM, Daniel Vetter > > wrote: > >> Moved to a common location so that Jani also can push to it, to avoid > >> moving it every time I go on vacation. Please update autobuilders and > >> everything else pointing at the drm-intel.git repo, the old one won't > >> be updated any more. > >> > >> Cc: Dave Airlie > >> Cc: Jani Nikula > >> Signed-off-by: Daniel Vetter > > > > Also forgotten to cc our QA people ... > > And Wu Fengguang with his 0-day builder also needs to know about the > new git repo! Yeah updated, thank you for the info! Fengguang > -Daniel > > >> --- > >> MAINTAINERS | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/MAINTAINERS b/MAINTAINERS > >> index 31a046213274..e3aaf277cf58 100644 > >> --- a/MAINTAINERS > >> +++ b/MAINTAINERS > >> @@ -2832,7 +2832,7 @@ M:Jani Nikula > >> L: intel-gfx@lists.freedesktop.org > >> L: dri-de...@lists.freedesktop.org > >> Q: http://patchwork.freedesktop.org/project/intel-gfx/ > >> -T: git git://people.freedesktop.org/~danvet/drm-intel > >> +T: git git://anongit.freedesktop.org/drm-intel > >> S: Supported > >> F: drivers/gpu/drm/i915/ > >> F: include/drm/i915* > >> -- > >> 1.8.5.2 > >> > > > > > > > > -- > > Daniel Vetter > > Software Engineer, Intel Corporation > > +41 (0) 79 365 57 48 - http://blog.ffwll.ch > > > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] lib/drmtest: don't use asprintf on signal paths
On Wed, 2014-02-05 at 00:19 +0100, Daniel Vetter wrote: > On Wed, Feb 05, 2014 at 12:04:46AM +0200, Imre Deak wrote: > > On Tue, 2014-02-04 at 21:29 +, Chris Wilson wrote: > > > On Tue, Feb 04, 2014 at 09:15:14PM +0200, Imre Deak wrote: > > > > It's not signal safe and I got kms_flip in hung state with the backtrace > > > > below, while the parent process waiting for the signal helper to exit. > > > > It was quite easy to reproduce the bug by running > > > > > > > > kms_flip --run-subtest=flip-vs-dpms-off-vs-modeset > > > > > > snprintf is not signalsafe either (man 7 signal). X goes as far as > > > implementing its own limited pnprintf() instead. > > > > Thanks. I got only as far as to realize that asprintf is not signal-safe > > b/c of malloc and didn't remember any place with an official list of > > allowed functions.. > > > > I also missed at least igt_skip() calling vprintf(), so this needs some > > more thought. > > Hm, what calls igt_skip from exit handlers? That would be a fairly big bug > in the framework ... If it happens atm I guess we should splatter a few > more asserts all over the place. At least quiescent_gpu_at_exit()->__drm_open_any()->drm_get_card(). > For the actual issue at hand I dunno what to do. Trying to keep all exit > handlers signal-save seems like a lost cause since we'll always miss some > of them because normally they run as atexit callbacks. And libc isn't > friendly and tells us when we do something stupid. Overall this feels a > bit like a general reliability nightmare :( Yea, doing anything substantial in signal handlers doesn't seem like a good idea. I haven't found much info about what functions you can call from exit handlers, but I guess we should treat that case similarly to signal handlers. I think the only clean, safe and maintainable way is to run the actual test in a forked process and do any cleanup in case of error or normal exit in the parent wrapper process. We wouldn't install any cleanup signal handlers in the child (children), just let it die and have the wrapper process cleanup afterwards. If a signal handler for cleanup would be unavoidable it would be really simple and just communicate anything necessary to the wrapper so that it can do the actual cleanup. Forking helpers would happen with the control of the wrapper, using some IPC like a pipe/socket, where the wrapper would receive the pid of any newly forked helper and it could make sure none of those are left behind at exit. I would also use a different method for the (normal) termination of forked helpers. Atm, we do kill(SIGQUIT), but using some IPC (pipe) would be more robust imo. But, for the short term I would suggest auditing our current atexit handlers, and making them signal-safe. There are about 10 of those, so it's not (yet) unsurmountable. --Imre signature.asc Description: This is a digitally signed message part ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/7] drm/i915: Fix SNB GT_MODE register setup
On Tue, Feb 04, 2014 at 09:23:06PM +, Chris Wilson wrote: > On Tue, Feb 04, 2014 at 09:59:15PM +0200, ville.syrj...@linux.intel.com wrote: > > From: Ville Syrjälä > > > > On SNB we set up WaSetupGtModeTdRowDispatch:snb early in > > gen6_init_clock_gating(). That sets a bit in the GEN6_GT_MODE register. > > However later we go and disable all the bits in the same register. And > > then we go on to set some other bit. So apparently we never actually > > implemented this workaround since the "disable all bits" part was there > > already before the w/a got supposedly implemented. > > > > These are the relevant commits: > > > > commit 6547fbdbfff62c99e4f7b4f985ff8b3454f33b0f > > Author: Daniel Vetter > > Date: Fri Dec 14 23:38:29 2012 +0100 > > > > drm/i915: Implement WaSetupGtModeTdRowDispatch > > > > commit f8f2ac9a76b0f80a6763ca316116a7bab8486997 > > Author: Ben Widawsky > > Date: Wed Oct 3 19:34:24 2012 -0700 > > > > drm/i915: Fix GT_MODE default value > > > > So, let's drop the "disable all bits" part, move both writes to > > closer proxomity to each other, and name the WIZ hashing bits > > appropriately. BSpec is still a bit confused how the bits should > > actually be interpreted, but I took the the description for the > > high bit since the low bit part only lists values for a single bit. > > > > Also add a comment about our choice of WIZ hashing mode. > > Changing WiZ hashing mode changes the valid number of threads and > userspace assumes best case (WiZ disabled). Worst case we start hanging > the chip. > > I have no idea how relevant that piece of lore from the spec is, but it > something to be wary of when making these changes. The thread numbers seem to depend only on the WIZ hashing disable bit. I'm not touching that one. -- Ville Syrjälä Intel OTC ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
