[Intel-gfx] [PATCH 2/2] drm/i915: Rewrite IVB FDI bifurcation conflict checks
From: Ville Syrjälä ville.syrj...@linux.intel.com
Ignore the current state of the pipe and just check crtc_state-enable
and the number of FDI lanes required. This means we don't accidentally
mistake the FDI lanes as being available of one of the pipes just
happens to be disabled at the time of the check. Also we no longer
consider pipe C to require FDI lanes when it's driving the eDP
transcoder.
We also take the opportunity to make the code a bit nicer looking by
hiding the ugly bits in the new pipe_required_fdi_lanes() function.
Cc: Ander Conselvan de Oliveira ander.conselvan.de.olive...@intel.com
Cc: Daniel Vetter dan...@ffwll.ch
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_display.c | 29 +++--
1 file changed, 15 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 242a8a7..72e9816 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3152,12 +3152,6 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
FDI_FE_ERRC_ENABLE);
}
-static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
-{
- return crtc-base.state-enable crtc-active
- crtc-config-has_pch_encoder;
-}
-
/* The FDI link training functions for ILK/Ibexpeak. */
static void ironlake_fdi_link_train(struct drm_crtc *crtc)
{
@@ -5548,13 +5542,21 @@ bool intel_connector_get_hw_state(struct
intel_connector *connector)
return encoder-get_hw_state(encoder, pipe);
}
+static int pipe_required_fdi_lanes(struct drm_device *dev, enum pipe pipe)
+{
+ struct intel_crtc *crtc =
+ to_intel_crtc(intel_get_crtc_for_pipe(dev, pipe));
+
+ if (crtc-base.state-enable
+ crtc-config-has_pch_encoder)
+ return crtc-config-fdi_lanes;
+
+ return 0;
+}
+
static bool ironlake_check_fdi_lanes(struct drm_device *dev, enum pipe pipe,
struct intel_crtc_state *pipe_config)
{
- struct drm_i915_private *dev_priv = dev-dev_private;
- struct intel_crtc *pipe_B_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_B]);
-
DRM_DEBUG_KMS(checking fdi config on pipe %c, lanes %i\n,
pipe_name(pipe), pipe_config-fdi_lanes);
if (pipe_config-fdi_lanes 4) {
@@ -5581,8 +5583,8 @@ static bool ironlake_check_fdi_lanes(struct drm_device
*dev, enum pipe pipe,
case PIPE_A:
return true;
case PIPE_B:
- if (dev_priv-pipe_to_crtc_mapping[PIPE_C]-enabled
- pipe_config-fdi_lanes 2) {
+ if (pipe_config-fdi_lanes 2
+ pipe_required_fdi_lanes(dev, PIPE_C) 0) {
DRM_DEBUG_KMS(invalid shared fdi lane config on pipe
%c: %i lanes\n,
pipe_name(pipe), pipe_config-fdi_lanes);
return false;
@@ -5594,8 +5596,7 @@ static bool ironlake_check_fdi_lanes(struct drm_device
*dev, enum pipe pipe,
pipe_name(pipe), pipe_config-fdi_lanes);
return false;
}
- if (pipe_has_enabled_pch(pipe_B_crtc)
- pipe_B_crtc-config-fdi_lanes 2) {
+ if (pipe_required_fdi_lanes(dev, PIPE_B) 2) {
DRM_DEBUG_KMS(fdi link B uses too many lanes to enable
link C\n);
return false;
}
--
2.0.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 0/2] drm/i915: Try to sort out ironlake_check_fdi_lanes()
From: Ville Syrjälä ville.syrj...@linux.intel.com So these on top of Ander's latest FDI bifurcation patch [1] should hopefully result in something sane. Didn't test them though, but assuming Ander has that test somewhere maybe some of the problematic cases I identified [2] could be checked with these. [1] http://lists.freedesktop.org/archives/intel-gfx/2015-March/061802.html [2] http://lists.freedesktop.org/archives/intel-gfx/2015-March/061806.html Ville Syrjälä (2): drm/i915: Rewrite some some of the FDI lane checks drm/i915: Rewrite IVB FDI bifurcation conflict checks drivers/gpu/drm/i915/intel_display.c | 40 ++-- 1 file changed, 20 insertions(+), 20 deletions(-) -- 2.0.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept
On Wed, Mar 11, 2015 at 11:37:54AM +, Conselvan De Oliveira, Ander wrote:
On Wed, 2015-03-11 at 13:35 +0200, Ander Conselvan de Oliveira wrote:
Remove the global modeset resource function that would disable the
bifurcation bit, and instead enable/disable it when enabling the pch
transcoder. The mode set consistency check should prevent us from
disabling the bit if pipe C is enabled so the change should be safe.
Note that this doens't affect the logic that prevents the bit being
set while a pipe is active, since the patch retains the behavior of
only chaging the bit if necessary. Because of the checks during mode
set, the first change would necessarily happen with both pipes B and
C disabled, and any subsequent write would be skipped.
v2: Only change the bit during pch trancoder enable. (Ville)
Oops, I forgot the sob line.
Signed-off-by: Ander Conselvan de Oliveira
ander.conselvan.de.olive...@intel.com
So I was staring at this stuff for a while and I believe it should be
fine. We don't keep the bifurcation state entirely consistent when
neither of the the pipes B/C are actually driving a PCH transcoder, but
that shouldn't really matter. If we want to make it consistent then I
suggest that we go with my earlier idea of only changing the state at
transcoder B with 2 lanes enable/disable, and otherwise keep it enabled
all the time. The slight complication there is the initial state we get
from the BIOS which might not match that, so we'd need to sanitize it
or something.
Anyway, I also posted a couple of patches on top that try to sort out
ironlake_check_fdi_lanes() [1]. With those and this one I think things
should work even better than before.
So for this patch:
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
[1] http://lists.freedesktop.org/archives/intel-gfx/2015-March/061828.html
---
drivers/gpu/drm/i915/intel_display.c | 46
1 file changed, 10 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 4008bf4..bfbd829 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc
*crtc)
return crtc-base.state-enable crtc-config-has_pch_encoder;
}
-static void ivb_modeset_global_resources(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev-dev_private;
- struct intel_crtc *pipe_B_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_B]);
- struct intel_crtc *pipe_C_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_C]);
- uint32_t temp;
-
- /*
-* When everything is off disable fdi C so that we could enable fdi B
-* with all lanes. Note that we don't care about enabled pipes without
-* an enabled pch encoder.
-*/
- if (!pipe_has_enabled_pch(pipe_B_crtc)
- !pipe_has_enabled_pch(pipe_C_crtc)) {
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
-
- temp = I915_READ(SOUTH_CHICKEN1);
- temp = ~FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(disabling fdi C rx\n);
- I915_WRITE(SOUTH_CHICKEN1, temp);
- }
-}
-
/* The FDI link training functions for ILK/Ibexpeak. */
static void ironlake_fdi_link_train(struct drm_crtc *crtc)
{
@@ -3834,20 +3808,23 @@ static void
ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
I915_READ(VSYNCSHIFT(cpu_transcoder)));
}
-static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
+static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
{
struct drm_i915_private *dev_priv = dev-dev_private;
uint32_t temp;
temp = I915_READ(SOUTH_CHICKEN1);
- if (temp FDI_BC_BIFURCATION_SELECT)
+ if (!!(temp FDI_BC_BIFURCATION_SELECT) == enable)
return;
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
- temp |= FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(enabling fdi C rx\n);
+ temp = ~FDI_BC_BIFURCATION_SELECT;
+ if (enable)
+ temp |= FDI_BC_BIFURCATION_SELECT;
+
+ DRM_DEBUG_KMS(%sabling fdi C rx\n, enable ? en : dis);
I915_WRITE(SOUTH_CHICKEN1, temp);
POSTING_READ(SOUTH_CHICKEN1);
}
@@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct
drm_device *dev)
static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc
*intel_crtc)
{
struct drm_device *dev = intel_crtc-base.dev;
- struct drm_i915_private *dev_priv = dev-dev_private;
switch (intel_crtc-pipe) {
case PIPE_A:
break;
case PIPE_B:
if
Re: [Intel-gfx] [PATCH 4/9] drm/i915: Add debugfs functions for Displayport compliance testing
On 3/9/2015 10:57 AM, Jani Nikula wrote: On Thu, 19 Feb 2015, Todd Previte tprev...@gmail.com wrote: This patch is the amalgamation of 7 patches from the V2 series. These patches all involve the implementation of the debugfs mechanism for handling Displayport compliance testing. The following are the commit messages from those 7 patches (included here to try and preserve the patch set history): I think there should be per connector debugfs files for this, instead of doing it for all DP connectors. See my dpcd debugfs patch [1] for an example. I wish that got reviewed and merged... BR, Jani. When it comes to compliance testing, you generally don't want anything else plugged in anyways. The user app is going to kill all the other displays, since it needs to be DRM master, so it's really kind of pointless. It also makes the implementation less complex. During development and testing, I've run the user app locally (logged in as root directly on the DUT) and over the network (ssh as root into the machine and launched the user app from the console) and the network method seems easier and more efficient to me. This is especially important as I can see the debug output from the user app right on the console instead of staring at a blank screen waiting for the test to complete. That said, I think this is a reasonable request that can be addressed at a future time, once the base code is in place. The architectural and design changes necessary for this would drag out the integration of basic compliance testing even longer and that's not something I believe we can afford to do right now. I'll go look for your DPCD patch and get a review on there. -T [1] http://mid.gmane.org/1424867645-21608-1-git-send-email-jani.nik...@intel.com --- [PATCH 04/17] drm/i915: Add debugfs information for Displayport compliance testing Originally, this patch was part of [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing. It contained definitions/declarations for some of the constants and data structures added to support debugfs output for Displayport compliance testing. --- [PATCH 05/17] drm/i915: Add file ops for Displayport configuration in debugfs This patch was also part of [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing. This one added file operations structures for Displayport configuration and the declarations for open() and write() in the file ops structure. --- [PATCH 06/17] drm/i915: Add support functions in debugfs for handling Displayport compliance configuration This patch was previously part of [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing. It added two support functions for handling Displayport configuration parameters that are used for compliance testing. --- [PATCH 07/17] drm/i915: Add and implement the debugfs 'show' functions for Displayport compliance This patch was previously part of [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing. This patch implemented the show() function. --- [PATCH 08/17] drm/i915: Add Displayport link configuration structure This patch was previously part of [PATCH 07/10] drm/i915: Add structures for Displayport compliance testing parameters. It added a struct to maintain link configuration data. These were meant purely for Displayport compliance use. --- [PATCH 09/17] drm/i915: Add config parsing utilities in debugfs for Displayport compliance This patch was previously part of [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing. This patch adds two functions to handle parsing of Displayport configuration information as it formatted in the debugfs file. It is used to process incoming configuration changes from the userspace compliance application during testing. --- [PATCH 10/17] drm/i915: Implement the 'open' and 'write' debugfs functions for Displayport compliance This patch is a combination of sections out of the following two previous patches: [PATCH 05/10] drm/i915: Add debugfs interface for Displayport debug and compliance testing [PATCH 07/10] drm/i915: Add structures for Displayport compliance testing parameters This patch implements the debugfs functions for 'open' and 'write' as they are specified in the file ops structure. The 'open'
Re: [Intel-gfx] [PATCH] drm/i915: remove indirection in the PCI ID macros
On Tue, Feb 03, 2015 at 01:36:00PM +, Damien Lespiau wrote: On Tue, Feb 03, 2015 at 02:34:05PM +0200, Jani Nikula wrote: Spell all the PCI IDs out to be able to quickly grep for the IDs. No functional changes. Signed-off-by: Jani Nikula jani.nik...@intel.com --- I tested this by comparing the results of $ make drivers/gpu/drm/i915/i915_drv.s $ make arch/x86/kernel/early-quirks.s before and after the patch. No change. I'm sold by the test so: Reviewed-by: Damien Lespiau damien.lesp...@intel.com Jani just pinged me about this and I agree grepability is nice and bdw is indeed the odd one out right now. I've added GT1/2 comments so that we don't just drop that information on the floor and merged it to dinq. Minor conflict but I think wiggle did the right thing ... -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 1/2] drm/i915: Rewrite some some of the FDI lane checks
From: Ville Syrjälä ville.syrj...@linux.intel.com
The logic in the FDI lane checks is very hard for my poor brain to
grasp. Rewrite it in a more straightforward way.
Cc: Ander Conselvan de Oliveira ander.conselvan.de.olive...@intel.com
Cc: Daniel Vetter dan...@ffwll.ch
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_display.c | 15 +++
1 file changed, 7 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index c361af6..242a8a7 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5589,14 +5589,13 @@ static bool ironlake_check_fdi_lanes(struct drm_device
*dev, enum pipe pipe,
}
return true;
case PIPE_C:
- if (!pipe_has_enabled_pch(pipe_B_crtc) ||
- pipe_B_crtc-config-fdi_lanes = 2) {
- if (pipe_config-fdi_lanes 2) {
- DRM_DEBUG_KMS(invalid shared fdi lane config
on pipe %c: %i lanes\n,
- pipe_name(pipe),
pipe_config-fdi_lanes);
- return false;
- }
- } else {
+ if (pipe_config-fdi_lanes 2) {
+ DRM_DEBUG_KMS(only 2 lanes on pipe %c: required %i
lanes\n,
+ pipe_name(pipe), pipe_config-fdi_lanes);
+ return false;
+ }
+ if (pipe_has_enabled_pch(pipe_B_crtc)
+ pipe_B_crtc-config-fdi_lanes 2) {
DRM_DEBUG_KMS(fdi link B uses too many lanes to enable
link C\n);
return false;
}
--
2.0.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH v2] drm/i915: Optimistically spin for the request completion
This provides a nice boost to mesa in swap bound scenarios (as mesa
throttles itself to the previous frame and given the scenario that will
complete shortly). It will also provide a good boost to systems running
with semaphores disabled and so frequently waiting on the GPU as it
switches rings. In the most favourable of microbenchmarks, this can
increase performance by around 15% - though in practice improvements
will be marginal and rarely noticeable.
v2: Account for user timeouts
Signed-off-by: Chris Wilson ch...@chris-wilson.co.uk
Cc: Daniel Vetter daniel.vet...@ffwll.ch
---
drivers/gpu/drm/i915/i915_gem.c | 47 +++--
1 file changed, 41 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index de15bd319bd0..f8895ed368cb 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1191,6 +1191,32 @@ static bool missed_irq(struct drm_i915_private *dev_priv,
return test_bit(ring-id, dev_priv-gpu_error.missed_irq_rings);
}
+static int __i915_spin_request(struct drm_i915_gem_request *req,
+ unsigned long timeout)
+{
+ struct intel_engine_cs *ring = i915_gem_request_get_ring(req);
+ struct drm_i915_private *dev_priv = to_i915(ring-dev);
+ int ret = -EBUSY;
+
+ intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+ while (!need_resched()) {
+ if (i915_gem_request_completed(req, true)) {
+ ret = 0;
+ goto out;
+ }
+
+ if (timeout time_after_eq(jiffies, timeout))
+ break;
+
+ cpu_relax_lowlatency();
+ }
+ if (i915_gem_request_completed(req, false))
+ ret = 0;
+out:
+ intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+ return ret;
+}
+
/**
* __i915_wait_request - wait until execution of request has finished
* @req: duh!
@@ -1235,12 +1261,20 @@ int __i915_wait_request(struct drm_i915_gem_request
*req,
if (ring-id == RCS INTEL_INFO(dev)-gen = 6)
gen6_rps_boost(dev_priv, file_priv);
- if (!irq_test_in_progress WARN_ON(!ring-irq_get(ring)))
- return -ENODEV;
-
/* Record current time in case interrupted by signal, or wedged */
trace_i915_gem_request_wait_begin(req);
before = ktime_get_raw_ns();
+
+ /* Optimistic spin before touching IRQs */
+ ret = __i915_spin_request(req, timeout_expire);
+ if (ret == 0)
+ goto out;
+
+ if (!irq_test_in_progress WARN_ON(!ring-irq_get(ring))) {
+ ret = -ENODEV;
+ goto out;
+ }
+
for (;;) {
struct timer_list timer;
@@ -1289,14 +1323,15 @@ int __i915_wait_request(struct drm_i915_gem_request
*req,
destroy_timer_on_stack(timer);
}
}
- now = ktime_get_raw_ns();
- trace_i915_gem_request_wait_end(req);
-
if (!irq_test_in_progress)
ring-irq_put(ring);
finish_wait(ring-irq_queue, wait);
+out:
+ now = ktime_get_raw_ns();
+ trace_i915_gem_request_wait_end(req);
+
if (timeout) {
s64 tres = *timeout - (now - before);
--
2.1.4
___
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: Move drm_framebuffer_unreference out of struct_mutex for takeover
On Tue, Mar 10, 2015 at 12:32:43PM +, Tvrtko Ursulin wrote:
On 03/10/2015 12:19 PM, Chris Wilson wrote:
On Tue, Mar 10, 2015 at 12:02:28PM +, Tvrtko Ursulin wrote:
@@ -13707,13 +13709,18 @@ void intel_modeset_gem_init(struct drm_device
*dev)
NULL)) {
DRM_ERROR(failed to pin boot fb on pipe %d\n,
to_intel_crtc(c)-pipe);
- drm_framebuffer_unreference(c-primary-fb);
- c-primary-fb = NULL;
- update_state_fb(c-primary);
+ unused[n_unused++] = c-primary;
}
}
mutex_unlock(dev-struct_mutex);
+ while (n_unused--) {
+ struct drm_plane *p = unused[n_unused];
+ drm_framebuffer_unreference(p-fb);
+ p-fb = NULL;
+ update_state_fb(p);
+ }
+
For this one I am not sure. Should c-primary-fb = NULL remain
under the locked loop? If not what is the the mutex protecting then?
It's a dummy mutex that only exists to keep the WARNs quiet. This phase
of initialisation is explicitly single-threaded.
Would it be a simpler fix then to move the mutex only around
pin_and_fence_fb_obj?
That would be much nicer indeed.
--
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 4/9] drm/i915: Make derived plane state correct after crtc_enable
On Wed, Mar 11, 2015 at 02:19:38PM +0200, Ville Syrjälä wrote:
On Wed, Mar 11, 2015 at 11:24:34AM +0100, Daniel Vetter wrote:
On Wed, Mar 11, 2015 at 12:05:39PM +0200, Ville Syrjälä wrote:
On Wed, Mar 11, 2015 at 10:52:29AM +0100, Daniel Vetter wrote:
On Tue, Mar 10, 2015 at 07:57:13PM +0200, Ville Syrjälä wrote:
On Tue, Mar 10, 2015 at 10:01:51AM -0700, Matt Roper wrote:
On Tue, Mar 10, 2015 at 01:15:24PM +0200,
ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
-static void disable_plane_internal(struct drm_plane *plane)
+static void _intel_crtc_enable_planes(struct intel_crtc *crtc)
{
- struct intel_plane *intel_plane = to_intel_plane(plane);
- struct drm_plane_state *state =
- plane-funcs-atomic_duplicate_state(plane);
- struct intel_plane_state *intel_state =
to_intel_plane_state(state);
+ struct drm_device *dev = crtc-base.dev;
+ enum pipe pipe = crtc-pipe;
+ struct intel_plane *plane;
+ const struct drm_crtc_helper_funcs *crtc_funcs =
+ crtc-base.helper_private;
- intel_state-visible = false;
- intel_plane-commit_plane(plane, intel_state);
+ for_each_intel_plane(dev, plane) {
+ const struct drm_plane_helper_funcs *funcs =
+ plane-base.helper_private;
+ struct intel_plane_state *state =
+ to_intel_plane_state(plane-base.state);
- intel_plane_destroy_state(plane, state);
+ if (plane-pipe != pipe)
+ continue;
+
+ if (funcs-atomic_check(plane-base, state-base))
Maybe add a WARN_ON() here? I'm assuming that this shouldn't
really be
possible since if this fails it means we've already previously done
a
commit of invalid state on a previous atomic transaction. But if it
does somehow happen, the WARN will give us a clue why the plane
contents
simply didn't show up.
I can think of one way to make it fail. That is, first set a smaller
mode with the primary plane (and fb) configured to cover that fully,
and
then switch to a larger mode without reconfiguring the primary plane.
If
the hardware requires the primary plane to be fullscreen it'll fail.
But
that should actaully not be possible using the legacy modeset API as
it
always reconfigures the primary, so we'd only have to worry about that
with full atomic modeset, and for that we anyway need to change the
code
to do the check stuff up front.
So yeah, with the way things are this should not be able to fail. I'll
respin with the WARN.
I haven't fully dug into the details here, but a few randome comments:
- While transitioning we're calling the transitional plane helpers,
which
should call the atomic_check stuff for us on the primary plane. If we
need to call atomic_check on other planes too (why?)
Because we want the derived state to be updated to match the (potentially
changed) crtc config. We do call the .update_plane() hook from the
modeset path, but that happens at a time when the pipe is off, so our
clipping calculations end up saying the plane is invisible. I think fixing
that the right way pretty much involves the atomic conversion of the
modeset path.
Why do we conclude it's invisible?
Because crtc-active. So for this we'll be wanting crtc_state-active
or somesuch which tells us upfront whether the pipe is going to be
active or not.
But that's also beside the point a bit since we still want to make call
the .atomic_check() for all planes. Right now we'd call it for primary
(at the wrong point wrt. crtc-active) and we call it for sprites later
when crtc-active is showing the right state, but we don't call it at
all for cursors. That's why we still have that ad-hoc extra cursor
clipping code in intel_update_cursor(). If we could make the derived
plane state correct, we could throw that stuff out as well and trust the
regular plane clipping calculations to tell us when the cursor has gone
offscreen.
It'll also make the plane state behave in a consitent manner wrt.
crtc-active. As it stands you simply can't trust the plane state for
primary/cursor planes.
So to fix all of that, I just went ahead and called it for all planes at
the point where we currently call it for sprites. I could have just gone
ahead and called the higher level .update_plane() func for all of them
(as we did for sprites already) but going one level lower allowed me to
get the planes to pop in/out atomically.
I think it's the best way to move forward with getting the plane and wm
code into some kind of sane state.
If we can fix the state to not depend
upon the dpms state then things should
Re: [Intel-gfx] [PATCH] drm/i915: Add polish to VLV WM shift+mask operations
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 5926 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -3 281/281 278/281 ILK 308/308 308/308 SNB -1 284/284 283/284 IVB -1 375/375 374/375 BYT 294/294 294/294 HSW 384/384 384/384 BDW 315/315 315/315 -Detailed- Platform Testdrm-intel-nightly Series Applied *PNV igt_gen3_render_mixed_blits PASS(1) FAIL(2) PNV igt_gen3_render_tiledx_blits FAIL(1)PASS(1) FAIL(1) PNV igt_gen3_render_tiledy_blits FAIL(1)PASS(1) FAIL(1) *SNB igt_gem_flink_bad-flink PASS(1) DMESG_WARN(1)PASS(1) *IVB igt_gem_storedw_batches_loop_normal PASS(1) DMESG_WARN(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 55/56] drm/i915: Remove 'faked' request from LRC submission
On 03/11/2015 09:14 AM, Daniel Vetter wrote: On Wed, Mar 11, 2015 at 02:53:39PM +, John Harrison wrote: On 05/03/2015 14:49, Daniel Vetter wrote: On Thu, Mar 05, 2015 at 01:57:31PM +, john.c.harri...@intel.com wrote: From: John Harrison john.c.harri...@intel.com The LRC submission code requires a request for tracking purposes. It does not actually require that request to 'complete' it simply uses it for keeping hold of reference counts on contexts and such like. In the case where the ring buffer is completely full, the LRC code looks for a pending request that would free up sufficient space upon completion and waits for it. If no such request can be found it resorts to simply polling the free space count until it is big enough. This situation should only occur when the entire buffer is filled with the request currently being generated. I.e., the user is trying to submit a single piece of work that is large than the ring buffer itself (which should be impossible because very large batch buffers don't consume any more ring buffer space). Before starting to poll, a submit call is made to make sure that the currently queued up work in the buffer will actually be submtted and thus the poll will eventually succeed. The problem here is that the 'official' request cannot be used as that could lead to multiple LRC submissions being tagged to a single request structure. Instead, the code fakes up a private request structure and uses that. This patch moves the faked request allocation higher up in the call stack to the wait code itself (rather than being at the very lowest submission level). Thus it is now obvious where the faked request is coming from and why it is necessary. The patch also replaces it with a call to the official request allocation code rather than attempting to duplicate that code. This becomes especially important in the future when the request allocation changes to accommodate a conversion to struct fence. For: VIZ-5115 Signed-off-by: John Harrison john.c.harri...@intel.com This is only possible if you pile up tons of olr. Since your patch series fixes this design issue by removing olr I think we can just put a WARN_ON in here if this ever happens and bail out with -ELOSTMYMARBLES or something. And then rip out all this complexity. Or do I miss something important? -Daniel Yeah, you missed the extremely important bug in the free space calculation that meant this impossible code path was being hit on a regular basis. The LRC wait_request code differed from the legacy wait_request code in the the latter was updated with request-postfix changes and the former was not. Thus the LRC one would happily find a request that frees up enough space, wait on it, retire it and then find there was still not enough space! New patches to fix the space calculation bug and to completely remove the polling path will be forth coming... Hm, Jesse did dig out a regression where gem_ringfill blows up on execlist. That igt is specifically to exercise this cornercases. I'm not sure whith bugzilla Jesse meant, there's two: https://bugs.freedesktop.org/show_bug.cgi?id=89001 https://bugs.freedesktop.org/show_bug.cgi?id=88865 Can you please check whether your fixes help for those issues two? Also adding Jesse since he's chasing these atm. Ah cool, sounds related at least. 89001 was the one I looked at yesterday. The worrying thing was that this bug caused a hard system hang. Not even the reset button worked... I guess we should have an HSD for that too so we can root cause the system part of it. Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Optimistically spin for the request completion
On Wed, Mar 11, 2015 at 10:30:13AM +, Chris Wilson wrote: On Wed, Mar 11, 2015 at 11:13:59AM +0100, Daniel Vetter wrote: Also do you have microbenchmark numbers for something midly ridiculous like a loop of very short batches (enough ofc to cause a bit of delay) and immediately stalling for them? It's definitely an awesome idea given that every other lock and sync primitive does it too. I guess I have an exciting morning of letting synmark run on one machine in various configs. So looking at the basics on HSW:gt3e, comparing mesa + throttle improvements and this patch, we see a consistent improvement throughout the synmark suite, of anything between 4 - 16%. That is actually reassuring as one of my main worries is that this patch will acerbate fallout from thermal throttling. However, a quick look at a few games suggests that any improvement here is well into the noise, if any. -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 55/56] drm/i915: Remove 'faked' request from LRC submission
On Wed, Mar 11, 2015 at 02:53:39PM +, John Harrison wrote: On 05/03/2015 14:49, Daniel Vetter wrote: On Thu, Mar 05, 2015 at 01:57:31PM +, john.c.harri...@intel.com wrote: From: John Harrison john.c.harri...@intel.com The LRC submission code requires a request for tracking purposes. It does not actually require that request to 'complete' it simply uses it for keeping hold of reference counts on contexts and such like. In the case where the ring buffer is completely full, the LRC code looks for a pending request that would free up sufficient space upon completion and waits for it. If no such request can be found it resorts to simply polling the free space count until it is big enough. This situation should only occur when the entire buffer is filled with the request currently being generated. I.e., the user is trying to submit a single piece of work that is large than the ring buffer itself (which should be impossible because very large batch buffers don't consume any more ring buffer space). Before starting to poll, a submit call is made to make sure that the currently queued up work in the buffer will actually be submtted and thus the poll will eventually succeed. The problem here is that the 'official' request cannot be used as that could lead to multiple LRC submissions being tagged to a single request structure. Instead, the code fakes up a private request structure and uses that. This patch moves the faked request allocation higher up in the call stack to the wait code itself (rather than being at the very lowest submission level). Thus it is now obvious where the faked request is coming from and why it is necessary. The patch also replaces it with a call to the official request allocation code rather than attempting to duplicate that code. This becomes especially important in the future when the request allocation changes to accommodate a conversion to struct fence. For: VIZ-5115 Signed-off-by: John Harrison john.c.harri...@intel.com This is only possible if you pile up tons of olr. Since your patch series fixes this design issue by removing olr I think we can just put a WARN_ON in here if this ever happens and bail out with -ELOSTMYMARBLES or something. And then rip out all this complexity. Or do I miss something important? -Daniel Yeah, you missed the extremely important bug in the free space calculation that meant this impossible code path was being hit on a regular basis. The LRC wait_request code differed from the legacy wait_request code in the the latter was updated with request-postfix changes and the former was not. Thus the LRC one would happily find a request that frees up enough space, wait on it, retire it and then find there was still not enough space! New patches to fix the space calculation bug and to completely remove the polling path will be forth coming... Hm, Jesse did dig out a regression where gem_ringfill blows up on execlist. That igt is specifically to exercise this cornercases. I'm not sure whith bugzilla Jesse meant, there's two: https://bugs.freedesktop.org/show_bug.cgi?id=89001 https://bugs.freedesktop.org/show_bug.cgi?id=88865 Can you please check whether your fixes help for those issues two? Also adding Jesse since he's chasing these atm. -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 08/11] drm/i915/skl: Updated the i915_frequency_info debugfs function
On Fri, Mar 06, 2015 at 11:07:21AM +0530, akash.g...@intel.com wrote:
From: Akash Goel akash.g...@intel.com
Added support for SKL in the i915_frequency_info debugfs function
v2:
- corrected the handling of reqf (Damien)
- Reorderd the platform check for cagf (Ville)
Signed-off-by: Akash Goel akash.g...@intel.com
Had to dig up the PM docs for the GT_PERF_STATUS, but with the right
docs it all looks good to me.
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_debugfs.c | 25 +
1 file changed, 17 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index f9b5a97..e97de3cc 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1090,7 +1090,7 @@ static int i915_frequency_info(struct seq_file *m, void
*unused)
seq_printf(m, Current P-state: %d\n,
(rgvstat MEMSTAT_PSTATE_MASK)
MEMSTAT_PSTATE_SHIFT);
} else if (IS_GEN6(dev) || (IS_GEN7(dev) !IS_VALLEYVIEW(dev)) ||
-IS_BROADWELL(dev)) {
+IS_BROADWELL(dev) || IS_GEN9(dev)) {
u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
u32 rp_state_limits = I915_READ(GEN6_RP_STATE_LIMITS);
u32 rp_state_cap = I915_READ(GEN6_RP_STATE_CAP);
@@ -1109,11 +1109,15 @@ static int i915_frequency_info(struct seq_file *m,
void *unused)
intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
reqf = I915_READ(GEN6_RPNSWREQ);
- reqf = ~GEN6_TURBO_DISABLE;
- if (IS_HASWELL(dev) || IS_BROADWELL(dev))
- reqf = 24;
- else
- reqf = 25;
+ if (IS_GEN9(dev))
+ reqf = 23;
+ else {
+ reqf = ~GEN6_TURBO_DISABLE;
+ if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+ reqf = 24;
+ else
+ reqf = 25;
+ }
reqf = intel_gpu_freq(dev_priv, reqf);
rpmodectl = I915_READ(GEN6_RP_CONTROL);
@@ -1127,7 +1131,9 @@ static int i915_frequency_info(struct seq_file *m, void
*unused)
rpdownei = I915_READ(GEN6_RP_CUR_DOWN_EI);
rpcurdown = I915_READ(GEN6_RP_CUR_DOWN);
rpprevdown = I915_READ(GEN6_RP_PREV_DOWN);
- if (IS_HASWELL(dev) || IS_BROADWELL(dev))
+ if (IS_GEN9(dev))
+ cagf = (rpstat GEN9_CAGF_MASK) GEN9_CAGF_SHIFT;
+ else if (IS_HASWELL(dev) || IS_BROADWELL(dev))
cagf = (rpstat HSW_CAGF_MASK) HSW_CAGF_SHIFT;
else
cagf = (rpstat GEN6_CAGF_MASK) GEN6_CAGF_SHIFT;
@@ -1153,7 +1159,7 @@ static int i915_frequency_info(struct seq_file *m, void
*unused)
pm_ier, pm_imr, pm_isr, pm_iir, pm_mask);
seq_printf(m, GT_PERF_STATUS: 0x%08x\n, gt_perf_status);
seq_printf(m, Render p-state ratio: %d\n,
-(gt_perf_status 0xff00) 8);
+(gt_perf_status (IS_GEN9(dev) ? 0x1ff00 : 0xff00))
8);
seq_printf(m, Render p-state VID: %d\n,
gt_perf_status 0xff);
seq_printf(m, Render p-state limit: %d\n,
@@ -1178,14 +1184,17 @@ static int i915_frequency_info(struct seq_file *m,
void *unused)
GEN6_CURBSYTAVG_MASK);
max_freq = (rp_state_cap 0xff) 16;
+ max_freq *= (IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1);
seq_printf(m, Lowest (RPN) frequency: %dMHz\n,
intel_gpu_freq(dev_priv, max_freq));
max_freq = (rp_state_cap 0xff00) 8;
+ max_freq *= (IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1);
seq_printf(m, Nominal (RP1) frequency: %dMHz\n,
intel_gpu_freq(dev_priv, max_freq));
max_freq = rp_state_cap 0xff;
+ max_freq *= (IS_SKYLAKE(dev) ? GEN9_FREQ_SCALER : 1);
seq_printf(m, Max non-overclocked (RP0) frequency: %dMHz\n,
intel_gpu_freq(dev_priv, max_freq));
--
1.9.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 11/11] drm/i915/skl: Enable the RPS interrupts programming
On 03/05/2015 09:37 PM, akash.g...@intel.com wrote:
From: Akash Goel akash.g...@intel.com
Enable the RPS interrupts programming(enable/disable/reset) for GEN9,
as missing changes to enable the RPS support on GEN9 have been added.
Signed-off-by: Akash Goel akash.g...@intel.com
---
drivers/gpu/drm/i915/intel_pm.c | 17 +++--
1 file changed, 3 insertions(+), 14 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 6273c282..3692837 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5629,12 +5629,7 @@ static void gen6_suspend_rps(struct drm_device *dev)
flush_delayed_work(dev_priv-rps.delayed_resume_work);
- /*
- * TODO: disable RPS interrupts on GEN9+ too once RPS support
- * is added for it.
- */
- if (INTEL_INFO(dev)-gen 9)
- gen6_disable_rps_interrupts(dev);
+ gen6_disable_rps_interrupts(dev);
}
/**
@@ -5692,12 +5687,7 @@ static void intel_gen6_powersave_work(struct
work_struct *work)
mutex_lock(dev_priv-rps.hw_lock);
- /*
- * TODO: reset/enable RPS interrupts on GEN9+ too, once RPS support is
- * added for it.
- */
- if (INTEL_INFO(dev)-gen 9)
- gen6_reset_rps_interrupts(dev);
+ gen6_reset_rps_interrupts(dev);
if (IS_CHERRYVIEW(dev)) {
cherryview_enable_rps(dev);
@@ -5716,8 +5706,7 @@ static void intel_gen6_powersave_work(struct
work_struct *work)
}
dev_priv-rps.enabled = true;
- if (INTEL_INFO(dev)-gen 9)
- gen6_enable_rps_interrupts(dev);
+ gen6_enable_rps_interrupts(dev);
mutex_unlock(dev_priv-rps.hw_lock);
Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept
On Wed, Mar 11, 2015 at 06:58:12PM +0200, Ville Syrjälä wrote: On Wed, Mar 11, 2015 at 11:37:54AM +, Conselvan De Oliveira, Ander wrote: On Wed, 2015-03-11 at 13:35 +0200, Ander Conselvan de Oliveira wrote: Remove the global modeset resource function that would disable the bifurcation bit, and instead enable/disable it when enabling the pch transcoder. The mode set consistency check should prevent us from disabling the bit if pipe C is enabled so the change should be safe. Note that this doens't affect the logic that prevents the bit being set while a pipe is active, since the patch retains the behavior of only chaging the bit if necessary. Because of the checks during mode set, the first change would necessarily happen with both pipes B and C disabled, and any subsequent write would be skipped. v2: Only change the bit during pch trancoder enable. (Ville) Oops, I forgot the sob line. Signed-off-by: Ander Conselvan de Oliveira ander.conselvan.de.olive...@intel.com So I was staring at this stuff for a while and I believe it should be fine. We don't keep the bifurcation state entirely consistent when neither of the the pipes B/C are actually driving a PCH transcoder, but that shouldn't really matter. If we want to make it consistent then I suggest that we go with my earlier idea of only changing the state at transcoder B with 2 lanes enable/disable, and otherwise keep it enabled all the time. The slight complication there is the initial state we get from the BIOS which might not match that, so we'd need to sanitize it or something. Anyway, I also posted a couple of patches on top that try to sort out ironlake_check_fdi_lanes() [1]. With those and this one I think things should work even better than before. So for this patch: Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com Queued for -next, thanks for the patch. -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 07/11] drm/i915/skl: Updated the gen9_enable_rps function
On Wed, Mar 11, 2015 at 12:27:59PM -0700, Jesse Barnes wrote: On 03/05/2015 09:37 PM, akash.g...@intel.com wrote: From: Akash Goel akash.g...@intel.com On SKL, GT frequency is programmed in units of 16.66 MHZ units compared to 50 MHZ for older platforms. Also the time value specified for Up/Down EI Up/Down thresholds are expressed in units of 1.33 us, compared to 1.28 us for older platforms. So updated the gen9_enable_rps function as per that. v2: Updated to use new macro GT_INTERVAL_FROM_US v3: Removed the initial setup of certain registers, from gen9_enable_rps, which gets overridden later from gen6_set_rps (Damien) v4: Removed the enabling of rps interrupts, from gen9_enable_rps. To be done from intel_gen6_powersave_work only, as done for other platforms also. Signed-off-by: Akash Goel akash.g...@intel.com --- drivers/gpu/drm/i915/intel_pm.c | 28 +--- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index c49950f..6273c282 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4132,23 +4132,21 @@ static void gen9_enable_rps(struct drm_device *dev) gen6_init_rps_frequencies(dev); - I915_WRITE(GEN6_RPNSWREQ, 0xc80); - I915_WRITE(GEN6_RC_VIDEO_FREQ, 0xc80); - - I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 0xf4240); - I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, 0x1206); - I915_WRITE(GEN6_RP_UP_THRESHOLD, 0xe808); - I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 0x3bd08); - I915_WRITE(GEN6_RP_UP_EI, 0x101d0); - I915_WRITE(GEN6_RP_DOWN_EI, 0x55730); + /* Program defaults and thresholds for RPS*/ + I915_WRITE(GEN6_RC_VIDEO_FREQ, + GEN9_FREQUENCY(dev_priv-rps.rp1_freq)); + + /* 1 second timeout*/ + I915_WRITE(GEN6_RP_DOWN_TIMEOUT, + GT_INTERVAL_FROM_US(dev_priv, 100)); + I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 0xa); - I915_WRITE(GEN6_PMINTRMSK, 0x6); - I915_WRITE(GEN6_RP_CONTROL, GEN6_RP_MEDIA_TURBO | - GEN6_RP_MEDIA_HW_MODE | GEN6_RP_MEDIA_IS_GFX | - GEN6_RP_ENABLE | GEN6_RP_UP_BUSY_AVG | - GEN6_RP_DOWN_IDLE_AVG); - gen6_enable_rps_interrupts(dev); + /* Leaning on the below call to gen6_set_rps to program/setup the +* Up/Down EI threshold registers, as well as the RP_CONTROL, +* RP_INTERRUPT_LIMITS RPNSWREQ registers */ + dev_priv-rps.power = HIGH_POWER; /* force a reset */ Are you also sure that dev_priv-rps.cur_freq != min_freq_softlimit at this point? That's the condition for calling into the threshold update function (maybe gen6_set_rps should check both variables though). gen6 uses the same trick, so I hope it's safe. And indeed a bit there's a call in both functions to gen6_init_rps_frequences which clears cur_freq to 0. Not the clearest code though, maybe we should move that right to above the call to gen6_set_rps. There's the added confusion that vlv/chv works different. Anyway that's material for a different patch, if at all. + gen6_set_rps(dev_priv-dev, dev_priv-rps.min_freq_softlimit); intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); } I'm assuming these match the latest SKL PM bits, but either way can be updated later based on tuning. Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org Merged up to this patch, still waiting for some final review on the remaining ones. Thanks, 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: Simplify the way BC bifurcation state consistency is kept
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 5933 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -3 281/281 278/281 ILK 308/308 308/308 SNB -1 284/284 283/284 IVB 375/375 375/375 BYT 294/294 294/294 HSW 384/384 384/384 BDW 315/315 315/315 -Detailed- Platform Testdrm-intel-nightly Series Applied PNV igt_gem_userptr_blits_coherency-sync CRASH(3)PASS(2) CRASH(1)PASS(1) PNV igt_gen3_render_tiledy_blits FAIL(3)PASS(1) FAIL(1)PASS(1) *PNV igt_gem_fence_thrash_bo-write-verify-threaded-none PASS(4) CRASH(1)PASS(1) *SNB igt_gem_exec_params_sol-reset-not-gen7 PASS(2) DMESG_WARN(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 55/56] drm/i915: Remove 'faked' request from LRC submission
On 11/03/2015 16:44, Jesse Barnes wrote: On 03/11/2015 09:14 AM, Daniel Vetter wrote: On Wed, Mar 11, 2015 at 02:53:39PM +, John Harrison wrote: On 05/03/2015 14:49, Daniel Vetter wrote: On Thu, Mar 05, 2015 at 01:57:31PM +, john.c.harri...@intel.com wrote: From: John Harrison john.c.harri...@intel.com The LRC submission code requires a request for tracking purposes. It does not actually require that request to 'complete' it simply uses it for keeping hold of reference counts on contexts and such like. In the case where the ring buffer is completely full, the LRC code looks for a pending request that would free up sufficient space upon completion and waits for it. If no such request can be found it resorts to simply polling the free space count until it is big enough. This situation should only occur when the entire buffer is filled with the request currently being generated. I.e., the user is trying to submit a single piece of work that is large than the ring buffer itself (which should be impossible because very large batch buffers don't consume any more ring buffer space). Before starting to poll, a submit call is made to make sure that the currently queued up work in the buffer will actually be submtted and thus the poll will eventually succeed. The problem here is that the 'official' request cannot be used as that could lead to multiple LRC submissions being tagged to a single request structure. Instead, the code fakes up a private request structure and uses that. This patch moves the faked request allocation higher up in the call stack to the wait code itself (rather than being at the very lowest submission level). Thus it is now obvious where the faked request is coming from and why it is necessary. The patch also replaces it with a call to the official request allocation code rather than attempting to duplicate that code. This becomes especially important in the future when the request allocation changes to accommodate a conversion to struct fence. For: VIZ-5115 Signed-off-by: John Harrison john.c.harri...@intel.com This is only possible if you pile up tons of olr. Since your patch series fixes this design issue by removing olr I think we can just put a WARN_ON in here if this ever happens and bail out with -ELOSTMYMARBLES or something. And then rip out all this complexity. Or do I miss something important? -Daniel Yeah, you missed the extremely important bug in the free space calculation that meant this impossible code path was being hit on a regular basis. The LRC wait_request code differed from the legacy wait_request code in the the latter was updated with request-postfix changes and the former was not. Thus the LRC one would happily find a request that frees up enough space, wait on it, retire it and then find there was still not enough space! New patches to fix the space calculation bug and to completely remove the polling path will be forth coming... Hm, Jesse did dig out a regression where gem_ringfill blows up on execlist. That igt is specifically to exercise this cornercases. I'm not sure whith bugzilla Jesse meant, there's two: https://bugs.freedesktop.org/show_bug.cgi?id=89001 https://bugs.freedesktop.org/show_bug.cgi?id=88865 I don't think the space calculation bug could cause the gem_ringfill failure. As noted above, even when the calculation gets it horribly wrong and flunks the wait for request path, the sit and poll path should still do the right thing. Plus the 88865 bug report I've already tried to reproduce and verified as fixed by the initial version of this patch set (i.e. without the space calc bug fix or the clean up of the fake request that follows it). It would go wrong pretty reliably on a clean nightly but never failed on my anti-OLR branch after quite a large amount of trying. I've just realised that I somehow got completely the wrong bug number in the cover letter. So where patch zero talks about fixing a gem_ringfill bug, it was supposed to be BZ:88865 not BZ:4! The second one, BZ:89001 certainly looks like the same issue but I don't have a SKL to test with. Can you please check whether your fixes help for those issues two? Also adding Jesse since he's chasing these atm. Ah cool, sounds related at least. 89001 was the one I looked at yesterday. The worrying thing was that this bug caused a hard system hang. Not even the reset button worked... I guess we should have an HSD for that too so we can root cause the system part of it. Jesse ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH] drm/i915: Read CHV_PLL_DW8 from the correct offset
From: Ville Syrjälä ville.syrj...@linux.intel.com We accidentally pass 'pipe' instead of 'port' to CHV_PLL_DW8() and with PIPE_C we end up at register offset 0x8320 which isn't the 0x8020 we wanted. Fix it. The problem was fortunately caught by the sanity check in vlv_dpio_read(): WARNING: CPU: 1 PID: 238 at ../drivers/gpu/drm/i915/intel_sideband.c:200 vlv_dpio_read+0x77/0x80 [i915]() DPIO read pipe C reg 0x8320 == 0x The problem got introduced with this commit: commit 71af07f91f12bbab96335e202c82525d31680960 Author: Vijay Purushothaman vijay.a.purushotha...@linux.intel.com Date: Thu Mar 5 19:33:08 2015 +0530 drm/i915: Update prop, int co-eff and gain threshold for CHV Cc: Vijay Purushothaman vijay.a.purushotha...@linux.intel.com Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ecbad5a..198e5fc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6270,7 +6270,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc, } vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW6(port), loopfilter); - dpio_val = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW8(pipe)); + dpio_val = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW8(port)); dpio_val = ~DPIO_CHV_TDC_TARGET_CNT_MASK; dpio_val |= (tribuf_calcntr DPIO_CHV_TDC_TARGET_CNT_SHIFT); vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW8(port), dpio_val); -- 2.0.5 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Optimistically spin for the request completion
On Wed, Mar 11, 2015 at 03:29:19PM +, Chris Wilson wrote:
+ while (!need_resched()) {
+ if (i915_gem_request_completed(req, true)) {
+ ret = 0;
+ goto out;
+ }
+
+ if (timeout time_after_eq(jiffies, timeout))
+ break;
+
+ cpu_relax_lowlatency();
+ }
Hmm. This unfortunately doesn't quite work the same as the optimistic
mutex spinner. The difference being that the scheduler knows that two
processes are contending for the mutex, but it doesn't know about the
contention between the thread and the GPU. The result is that unless
there is other activity on the system we simply degrade into a busy-spin
until the GPU is finished, rather than what I intended as spin for the
next ~10ms and then fallback to the interrupt.
Arguably busy-spinning on an idle system isn't totally evil, but it
certainly is likely to come at a power cost. On the other hand, spinning
is relatively rare outside of benchmarks. Rare enough to be useful?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
On 03/09/2015 02:04 PM, Ville Syrjälä wrote:
On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote:
On 03/09/2015 10:29 AM, Daniel Vetter wrote:
On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote:
On 03/06/2015 08:34 AM, Daniel Vetter wrote:
On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
+} else {
+/* SST mode - handle short/long pulses here */
+drm_modeset_lock(dev-mode_config.connection_mutex,
NULL);
+/* Clear compliance testing flags/data here to prevent
+ * false detection in userspace */
+intel_dp-compliance_test_data = 0;
+intel_dp-compliance_testing_active = 0;
+/* For a long pulse in SST mode, disable the main link
*/
+if (long_hpd) {
+I915_WRITE(DP_TP_CTL(intel_dig_port-port),
+ ~DP_TP_CTL_ENABLE);
+}
Disabling the main link should be done in userspace. All long pulse
requests should be forwarded to userspace as a hotplug event. Userspace
can then react to that hotplug appropriately. This way we can again
exercise the normal operation of all our dp code.
What's your concern here? Do you want to make sure we get coverage on
dp_link_down()? It looks like that might be safe to use here instead of
flipping the disable bit directly. Or did you want to go through the
whole pipe/port shutdown sequence as well? If so, I think the dpms
tests will already cover that, separate from simple compliance.
This is likely to upset the state checker, we've already had some fun with
killing the hard dp pipe disable that the hdp code occasionally did. Well,
still have. The other reason is that dp compliance testing with
special-case code is somewhat pointless, except when the compliance test
contracts what real-world experience forces us to do. For these exceptions
I'd like that we fully understand them and also document them. Disabling
the link on a full hot-unplug is something we can (and most DE actually
do) do.
If we end up hitting the checker while testing, then yeah it would spew.
But I thought this was mainly about testing the DP code, making sure we
can up/down links, train at different parameters, etc, not about going
through full mode sets all the time...
But either way, I agree we should be documenting this behavior so we
don't get stuck trying to figure it out later.
I don't think we should be killing the port like this. It'll also kill
the pipe on some platforms and then we get all kinds of pipe stuck
warnings. So I think we'd definitely want a more graceful shutdown of
things.
Does that affect current platforms? Or just CTG and ILK? I can guess
BYT BSW might be affected, but I haven't tested. As long as we just
up/down the port w/o anything else it might be able to work...
I thought we actually discussed about going to the other direction, ie.
potentially allowing the link to brought up without the pipe and
enabling/disabling the pipe independently while the link remains up and
running?
I guess I was thinking the reverse: that bringing up the port w/o a pipe
driving it would be more likely to cause problems, but I guess we'll
need testing.
Depending on what we find, we could change the logic to accommodate the
platforms we want to test (HSW+ and BYT+ I think, though we could limit
it to even newer ones if those are too tough to handle).
Jesse
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 01/11] drm/i915/skl: Added new macros
On Fri, Mar 06, 2015 at 11:07:14AM +0530, akash.g...@intel.com wrote:
From: Akash Goel akash.g...@intel.com
For SKL, register definition for RPNSWREQ (A008), RPSTAT1(A01C)
have changed slightly. Also on SKL, frequency is specified in
units of 16.66 MHZ, compared to 50 MHZ for most of the earlier
platforms and the time values are expressed in units of 1.33 us,
compared to 1.28 us for earlier platforms.
Added new macros for the aforementioned changes.
v2: Renamed the GT_FREQ_FROM_PERIOD macro to GT_INTERVAL_FROM_US (Damien)
v3: Removed the implicit use of dev_priv in GT_INTERVAL_FROM_US macro (Chris)
Signed-off-by: Akash Goel akash.g...@intel.com
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/i915_reg.h | 9 +
2 files changed, 10 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b384b72..f676dc8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2414,6 +2414,7 @@ struct drm_i915_cmd_table {
#define NUM_L3_SLICES(dev) (IS_HSW_GT3(dev) ? 2 : HAS_L3_DPF(dev))
#define GT_FREQUENCY_MULTIPLIER 50
+#define GEN9_FREQ_SCALER 3
#include i915_trace.h
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 56b97c4..05ab344 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -2427,6 +2427,12 @@ enum skl_disp_power_wells {
#define GEN6_RP_STATE_LIMITS (MCHBAR_MIRROR_BASE_SNB + 0x5994)
#define GEN6_RP_STATE_CAP(MCHBAR_MIRROR_BASE_SNB + 0x5998)
+#define INTERVAL_1_28_US(us) (((us) * 100) 7)
+#define INTERVAL_1_33_US(us) (((us) * 3)2)
+#define GT_INTERVAL_FROM_US(dev_priv, us) (IS_GEN9(dev_priv) ? \
+ INTERVAL_1_33_US(us) : \
+ INTERVAL_1_28_US(us))
+
/*
* Logical Context regs
*/
@@ -6080,6 +6086,7 @@ enum skl_disp_power_wells {
#define GEN6_TURBO_DISABLE (131)
#define GEN6_FREQUENCY(x) ((x)25)
#define HSW_FREQUENCY(x) ((x)24)
+#define GEN9_FREQUENCY(x) ((x)23)
#define GEN6_OFFSET(x) ((x)19)
#define GEN6_AGGRESSIVE_TURBO (015)
#define GEN6_RC_VIDEO_FREQ 0xA00C
@@ -6098,8 +6105,10 @@ enum skl_disp_power_wells {
#define GEN6_RPSTAT1 0xA01C
#define GEN6_CAGF_SHIFT8
#define HSW_CAGF_SHIFT 7
+#define GEN9_CAGF_SHIFT23
#define GEN6_CAGF_MASK (0x7f GEN6_CAGF_SHIFT)
#define HSW_CAGF_MASK (0x7f HSW_CAGF_SHIFT)
+#define GEN9_CAGF_MASK (0x1ff GEN9_CAGF_SHIFT)
#define GEN6_RP_CONTROL 0xA024
#define GEN6_RP_MEDIA_TURBO(111)
#define GEN6_RP_MEDIA_MODE_MASK(39)
--
1.9.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 03/11] drm/i915/skl: Updated the gen6_init_rps_frequencies function
On Fri, Mar 06, 2015 at 11:07:16AM +0530, akash.g...@intel.com wrote:
From: Akash Goel akash.g...@intel.com
On SKL the frequency is specified in units of 16.66 MHZ, barring the
RP_STATE_CAP(0x5998) register, which still reports frequency in units
of 50 MHZ. So an extra conversion is required in gen6_init_rps_frequencies
function for SKL, to store the frequency values as per the actual hardware
unit.
v2: Corrected the conversion from 50 to 16.66 MHZ (Ville)
Signed-off-by: Akash Goel akash.g...@intel.com
Reviewed-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_pm.c | 7 +++
1 file changed, 7 insertions(+)
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 1b44eee..81eaa0c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4080,6 +4080,13 @@ static void gen6_init_rps_frequencies(struct
drm_device *dev)
dev_priv-rps.rp0_freq = (rp_state_cap 0) 0xff;
dev_priv-rps.rp1_freq = (rp_state_cap 8) 0xff;
dev_priv-rps.min_freq = (rp_state_cap 16) 0xff;
+ if (IS_SKYLAKE(dev)) {
+ /* Store the frequency values in 16.66 MHZ units, which is
+the natural hardware unit for SKL */
+ dev_priv-rps.rp0_freq *= GEN9_FREQ_SCALER;
+ dev_priv-rps.rp1_freq *= GEN9_FREQ_SCALER;
+ dev_priv-rps.min_freq *= GEN9_FREQ_SCALER;
+ }
/* hw_max = RP0 until we check for overclocking */
dev_priv-rps.max_freq = dev_priv-rps.rp0_freq;
--
1.9.2
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 10/11] drm/i915/skl: Enabling processing of Turbo interrupts
On 03/05/2015 09:37 PM, akash.g...@intel.com wrote:
From: Akash Goel akash.g...@intel.com
Earlier Turbo interrupts were not being processed for SKL,
as something was amiss in turbo programming for SKL.
Now missing changes have been added, so enabling the Turbo
interrupt processing for SKL.
Signed-off-by: Akash Goel akash.g...@intel.com
---
drivers/gpu/drm/i915/i915_irq.c | 5 -
1 file changed, 5 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 9baecb7..6b7cc10 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -1696,11 +1696,6 @@ static void i9xx_pipe_crc_irq_handler(struct
drm_device *dev, enum pipe pipe)
* the work queue. */
static void gen6_rps_irq_handler(struct drm_i915_private *dev_priv, u32
pm_iir)
{
- /* TODO: RPS on GEN9+ is not supported yet. */
- if (WARN_ONCE(INTEL_INFO(dev_priv)-gen = 9,
- GEN9+: unexpected RPS IRQ\n))
- return;
-
if (pm_iir dev_priv-pm_rps_events) {
spin_lock(dev_priv-irq_lock);
gen6_disable_pm_irq(dev_priv, pm_iir dev_priv-pm_rps_events);
Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 07/11] drm/i915/skl: Updated the gen9_enable_rps function
On 03/05/2015 09:37 PM, akash.g...@intel.com wrote: From: Akash Goel akash.g...@intel.com On SKL, GT frequency is programmed in units of 16.66 MHZ units compared to 50 MHZ for older platforms. Also the time value specified for Up/Down EI Up/Down thresholds are expressed in units of 1.33 us, compared to 1.28 us for older platforms. So updated the gen9_enable_rps function as per that. v2: Updated to use new macro GT_INTERVAL_FROM_US v3: Removed the initial setup of certain registers, from gen9_enable_rps, which gets overridden later from gen6_set_rps (Damien) v4: Removed the enabling of rps interrupts, from gen9_enable_rps. To be done from intel_gen6_powersave_work only, as done for other platforms also. Signed-off-by: Akash Goel akash.g...@intel.com --- drivers/gpu/drm/i915/intel_pm.c | 28 +--- 1 file changed, 13 insertions(+), 15 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index c49950f..6273c282 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -4132,23 +4132,21 @@ static void gen9_enable_rps(struct drm_device *dev) gen6_init_rps_frequencies(dev); - I915_WRITE(GEN6_RPNSWREQ, 0xc80); - I915_WRITE(GEN6_RC_VIDEO_FREQ, 0xc80); - - I915_WRITE(GEN6_RP_DOWN_TIMEOUT, 0xf4240); - I915_WRITE(GEN6_RP_INTERRUPT_LIMITS, 0x1206); - I915_WRITE(GEN6_RP_UP_THRESHOLD, 0xe808); - I915_WRITE(GEN6_RP_DOWN_THRESHOLD, 0x3bd08); - I915_WRITE(GEN6_RP_UP_EI, 0x101d0); - I915_WRITE(GEN6_RP_DOWN_EI, 0x55730); + /* Program defaults and thresholds for RPS*/ + I915_WRITE(GEN6_RC_VIDEO_FREQ, + GEN9_FREQUENCY(dev_priv-rps.rp1_freq)); + + /* 1 second timeout*/ + I915_WRITE(GEN6_RP_DOWN_TIMEOUT, + GT_INTERVAL_FROM_US(dev_priv, 100)); + I915_WRITE(GEN6_RP_IDLE_HYSTERSIS, 0xa); - I915_WRITE(GEN6_PMINTRMSK, 0x6); - I915_WRITE(GEN6_RP_CONTROL, GEN6_RP_MEDIA_TURBO | -GEN6_RP_MEDIA_HW_MODE | GEN6_RP_MEDIA_IS_GFX | -GEN6_RP_ENABLE | GEN6_RP_UP_BUSY_AVG | -GEN6_RP_DOWN_IDLE_AVG); - gen6_enable_rps_interrupts(dev); + /* Leaning on the below call to gen6_set_rps to program/setup the + * Up/Down EI threshold registers, as well as the RP_CONTROL, + * RP_INTERRUPT_LIMITS RPNSWREQ registers */ + dev_priv-rps.power = HIGH_POWER; /* force a reset */ Are you also sure that dev_priv-rps.cur_freq != min_freq_softlimit at this point? That's the condition for calling into the threshold update function (maybe gen6_set_rps should check both variables though). + gen6_set_rps(dev_priv-dev, dev_priv-rps.min_freq_softlimit); intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL); } I'm assuming these match the latest SKL PM bits, but either way can be updated later based on tuning. Reviewed-by: Jesse Barnes jbar...@virtuousgeek.org ___ 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 ULL postfix to VGT_MAGIC constant
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 5931 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -7 281/281 274/281 ILK 308/308 308/308 SNB -1 284/284 283/284 IVB 375/375 375/375 BYT 294/294 294/294 HSW -2 384/384 382/384 BDW 315/315 315/315 -Detailed- Platform Testdrm-intel-nightly Series Applied PNV igt_gem_userptr_blits_coherency-sync CRASH(2)PASS(2) CRASH(2) *PNV igt_gem_userptr_blits_coherency-unsync CRASH(1)PASS(2) CRASH(1)NRUN(1) PNV igt_gen3_render_linear_blits FAIL(1)PASS(1) FAIL(2) PNV igt_gen3_render_mixed_blits FAIL(1)PASS(1) FAIL(2) PNV igt_gen3_render_tiledx_blits FAIL(2)PASS(1) FAIL(2) PNV igt_gen3_render_tiledy_blits FAIL(2)PASS(1) FAIL(2) PNV igt_gem_tiled_pread_pwrite FAIL(1)PASS(1) FAIL(1)PASS(1) *SNB igt_gem_largeobject PASS(2) DMESG_WARN(1)PASS(1) *HSW igt_drv_debugfs_reader PASS(2) DMESG_WARN(1)PASS(1) *HSW igt_drv_hangman_error-state-sysfs-entry PASS(2) TIMEOUT(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
On Wed, Mar 11, 2015 at 11:37:23AM -0700, Jesse Barnes wrote:
On 03/09/2015 02:04 PM, Ville Syrjälä wrote:
On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote:
On 03/09/2015 10:29 AM, Daniel Vetter wrote:
On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote:
On 03/06/2015 08:34 AM, Daniel Vetter wrote:
On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
+ } else {
+ /* SST mode - handle short/long pulses here */
+ drm_modeset_lock(dev-mode_config.connection_mutex,
NULL);
+ /* Clear compliance testing flags/data here to prevent
+ * false detection in userspace */
+ intel_dp-compliance_test_data = 0;
+ intel_dp-compliance_testing_active = 0;
+ /* For a long pulse in SST mode, disable the main link
*/
+ if (long_hpd) {
+ I915_WRITE(DP_TP_CTL(intel_dig_port-port),
+~DP_TP_CTL_ENABLE);
+ }
Disabling the main link should be done in userspace. All long pulse
requests should be forwarded to userspace as a hotplug event. Userspace
can then react to that hotplug appropriately. This way we can again
exercise the normal operation of all our dp code.
What's your concern here? Do you want to make sure we get coverage on
dp_link_down()? It looks like that might be safe to use here instead of
flipping the disable bit directly. Or did you want to go through the
whole pipe/port shutdown sequence as well? If so, I think the dpms
tests will already cover that, separate from simple compliance.
This is likely to upset the state checker, we've already had some fun with
killing the hard dp pipe disable that the hdp code occasionally did. Well,
still have. The other reason is that dp compliance testing with
special-case code is somewhat pointless, except when the compliance test
contracts what real-world experience forces us to do. For these exceptions
I'd like that we fully understand them and also document them. Disabling
the link on a full hot-unplug is something we can (and most DE actually
do) do.
If we end up hitting the checker while testing, then yeah it would spew.
But I thought this was mainly about testing the DP code, making sure we
can up/down links, train at different parameters, etc, not about going
through full mode sets all the time...
But either way, I agree we should be documenting this behavior so we
don't get stuck trying to figure it out later.
I don't think we should be killing the port like this. It'll also kill
the pipe on some platforms and then we get all kinds of pipe stuck
warnings. So I think we'd definitely want a more graceful shutdown of
things.
Does that affect current platforms? Or just CTG and ILK? I can guess
BYT BSW might be affected, but I haven't tested. As long as we just
up/down the port w/o anything else it might be able to work...
I think you have that reversed. Old platforms were generally fine with
enabling/disabling ports while the pipe kept running, but I think that
changed at some point (with ilk I suppose), and killing the port is then
a bad idea.
That's at least the case with DP. I seem to recall we had at least stuck
pipes (and maybe even some lockups or something?) when we had the
dp_link_down() call in the hpd handler.
I thought we actually discussed about going to the other direction, ie.
potentially allowing the link to brought up without the pipe and
enabling/disabling the pipe independently while the link remains up and
running?
I guess I was thinking the reverse: that bringing up the port w/o a pipe
driving it would be more likely to cause problems, but I guess we'll
need testing.
Bringing up the port on its own is perfectly fine. We do that for link
training already, and if you consider MST you'll notice it's the only
way really.
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Update intel_dp_hpd_pulse() to check link status for non-MST operation
On Wed, Mar 11, 2015 at 09:10:29PM +0200, Ville Syrjälä wrote:
On Wed, Mar 11, 2015 at 11:37:23AM -0700, Jesse Barnes wrote:
On 03/09/2015 02:04 PM, Ville Syrjälä wrote:
On Mon, Mar 09, 2015 at 12:07:31PM -0700, Jesse Barnes wrote:
On 03/09/2015 10:29 AM, Daniel Vetter wrote:
On Mon, Mar 09, 2015 at 08:34:49AM -0700, Jesse Barnes wrote:
On 03/06/2015 08:34 AM, Daniel Vetter wrote:
On Thu, Mar 05, 2015 at 11:22:19AM -0700, Todd Previte wrote:
+} else {
+/* SST mode - handle short/long pulses here */
+drm_modeset_lock(dev-mode_config.connection_mutex,
NULL);
+/* Clear compliance testing flags/data here to prevent
+ * false detection in userspace */
+intel_dp-compliance_test_data = 0;
+intel_dp-compliance_testing_active = 0;
+/* For a long pulse in SST mode, disable the main link
*/
+if (long_hpd) {
+I915_WRITE(DP_TP_CTL(intel_dig_port-port),
+ ~DP_TP_CTL_ENABLE);
+}
Disabling the main link should be done in userspace. All long pulse
requests should be forwarded to userspace as a hotplug event.
Userspace
can then react to that hotplug appropriately. This way we can again
exercise the normal operation of all our dp code.
What's your concern here? Do you want to make sure we get coverage on
dp_link_down()? It looks like that might be safe to use here instead
of
flipping the disable bit directly. Or did you want to go through the
whole pipe/port shutdown sequence as well? If so, I think the dpms
tests will already cover that, separate from simple compliance.
This is likely to upset the state checker, we've already had some fun
with
killing the hard dp pipe disable that the hdp code occasionally did.
Well,
still have. The other reason is that dp compliance testing with
special-case code is somewhat pointless, except when the compliance test
contracts what real-world experience forces us to do. For these
exceptions
I'd like that we fully understand them and also document them. Disabling
the link on a full hot-unplug is something we can (and most DE actually
do) do.
If we end up hitting the checker while testing, then yeah it would spew.
But I thought this was mainly about testing the DP code, making sure we
can up/down links, train at different parameters, etc, not about going
through full mode sets all the time...
But either way, I agree we should be documenting this behavior so we
don't get stuck trying to figure it out later.
I don't think we should be killing the port like this. It'll also kill
the pipe on some platforms and then we get all kinds of pipe stuck
warnings. So I think we'd definitely want a more graceful shutdown of
things.
Does that affect current platforms? Or just CTG and ILK? I can guess
BYT BSW might be affected, but I haven't tested. As long as we just
up/down the port w/o anything else it might be able to work...
I think you have that reversed. Old platforms were generally fine with
enabling/disabling ports while the pipe kept running, but I think that
changed at some point (with ilk I suppose), and killing the port is then
a bad idea.
That's at least the case with DP. I seem to recall we had at least stuck
pipes (and maybe even some lockups or something?) when we had the
dp_link_down() call in the hpd handler.
cpu edp on ilk+ and hsw+ dp are the ones that definitely get cranky if you
just kill the port. Not sure whether we've even seen hard hangs in dp
enabling on hsw, it's been a very long time ago. Well we've seen hard
hangs on hsw because of modeset bugs, but I don't remember whether it was
this on here too.
I thought we actually discussed about going to the other direction, ie.
potentially allowing the link to brought up without the pipe and
enabling/disabling the pipe independently while the link remains up and
running?
I guess I was thinking the reverse: that bringing up the port w/o a pipe
driving it would be more likely to cause problems, but I guess we'll
need testing.
Bringing up the port on its own is perfectly fine. We do that for link
training already, and if you consider MST you'll notice it's the only
way really.
Yeah on hsw+ and cpu edp we bring up the prot first iirc, then fire up the
pipe later on. Dunno what we do for external dp on ilk-ivb or what exactly
we should be doing - we iirc still have the occasionally dp port stuck bug
floating about.
In any case there's a very well defined sequence to go about things, and
anything else tends to anger the machines a lot. If we can't just use the
setCrtc ioctl from userspace we should at least reuse the link shutdown
machinery we have properly. Of course I'd prefer if we don't have to since
that would be less extra
Re: [Intel-gfx] [Beignet] [PATCH i-g-t 2/2] configure: Bump required libdrm version to 2.4.60
On Tue, Mar 10, 2015 at 01:06:44PM -0700, Jeff McGee wrote: On Tue, Mar 10, 2015 at 07:47:03PM +0100, Daniel Vetter wrote: On Tue, Mar 10, 2015 at 01:58:52PM -0400, Rob Clark wrote: On Tue, Mar 10, 2015 at 12:59 PM, Jeff McGee jeff.mc...@intel.com wrote: On Tue, Mar 10, 2015 at 08:37:30AM +0100, Daniel Vetter wrote: On Mon, Mar 09, 2015 at 04:41:02PM -0700, jeff.mc...@intel.com wrote: From: Jeff McGee jeff.mc...@intel.com tests/core_getparams needs the new libdrm interfaces for querying subslice and EU counts. For: VIZ-4636 Signed-off-by: Jeff McGee jeff.mc...@intel.com --- configure.ac | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/configure.ac b/configure.ac index 16d6a2e..88a1c3d 100644 --- a/configure.ac +++ b/configure.ac @@ -82,7 +82,7 @@ if test x$GCC = xyes; then fi AC_SUBST(ASSEMBLER_WARN_CFLAGS) -PKG_CHECK_MODULES(DRM, [libdrm_intel = 2.4.52 libdrm]) +PKG_CHECK_MODULES(DRM, [libdrm_intel = 2.4.60 libdrm]) Please don't and instead copypaste the new structs/defines with a local_ prefix like we do it for all the other new igt testcases. Forcing libdrm to get updated for igt all the time can get annoying fast. -Daniel In this case I'm trying to exercise new API functions in libdrm which wrap the GETPARAM ioctl. Would you rather me bypass the wrapper to avoid requiring updated libdrm? I can do that, but it fails to test the complete path that client would use. Am I missing something, or does 2.4.60 not exist yet? That said dependency bumps for igt seem like less of an issue than dependency bumps for mesa.. I mean if you are using igt you are probably on the latest anyways.. I'm not sure why Daniel is so concerned about that.. (but dependency bumps to something that doesn't exist yet should perhaps be avoided) I'd like to avoid massive depency loops for igt tests so that I can merge the testcase right when the patches land in -nightly. Otherwise there's always a small delay involved where regression can creep in. Also if I have to update libdrm every time I update igt that's annoying since without that I don't have to install/update anything at all - I run igt in-place. And we've used the LOCAL_ prefixes for pretty much every abi addition in igt tests thus far. -Daniel I understand that and it certainly makes sense when libdrm is only providing defines or structs. But as I said, in this case there is code in libdrm (the wrapper) that we could test as part of the complete path. Are you recommending that I implement duplicate wrapper functions in igt with the local prefix? Sorry I totally didn't realize that. Generally we don't have a lot of igt testcase for libdrm really, imo it's usually simpler to just add the interface to each part. Since this is such a simple one there's no need to have a low-level test and the libdrm test on top, but that's what I'd do if there's something worth testing in libdrm. Because for complex functionality I really want to get the bare-metal tests in together with the kernel part. Stalling for libdrm release could take longer. And yes, personally I'd just have open-coded the getparam call here in igt, but that's just a bikeshed. -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 2/2] drm/i915: Add ULL postfix to VGT_MAGIC constant
Without this Dave's 32bit rhel compiler is annoyed. Don't ask me about the exact rules for this stuff though, but this should be safe. Reported-by: Dave Airlie airl...@gmail.com Signed-off-by: Daniel Vetter daniel.vet...@intel.com --- drivers/gpu/drm/i915/i915_vgpu.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/i915_vgpu.h b/drivers/gpu/drm/i915/i915_vgpu.h index 0db9ccf32605..97a88b5f6a26 100644 --- a/drivers/gpu/drm/i915/i915_vgpu.h +++ b/drivers/gpu/drm/i915/i915_vgpu.h @@ -32,7 +32,7 @@ * The following structure pages are defined in GEN MMIO space * for virtualization. (One page for now) */ -#define VGT_MAGIC 0x4776544776544776 /* 'vGTvGTvG' */ +#define VGT_MAGIC 0x4776544776544776ULL/* 'vGTvGTvG' */ #define VGT_VERSION_MAJOR 1 #define VGT_VERSION_MINOR 0 -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Intel-gfx] [PATCH 1/2] drm/fourcc: 64 #defines need ULL postfix
I have no idea about the exact rules, but this angered Dave's 32bit rhel gcc. Reported-by: Dave Airlie airl...@gmail.com Signed-off-by: Daniel Vetter daniel.vet...@intel.com --- include/uapi/drm/drm_fourcc.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/uapi/drm/drm_fourcc.h b/include/uapi/drm/drm_fourcc.h index e6efac23c7ea..07735822a28f 100644 --- a/include/uapi/drm/drm_fourcc.h +++ b/include/uapi/drm/drm_fourcc.h @@ -151,7 +151,7 @@ /* add more to the end as needed */ #define fourcc_mod_code(vendor, val) \ - u64)DRM_FORMAT_MOD_VENDOR_## vendor) 56) | (val 0x00ffL)) + u64)DRM_FORMAT_MOD_VENDOR_## vendor) 56) | (val 0x00ffULL)) /* * Format Modifier tokens: -- 2.1.4 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Add soft-pinning API for execbuffer
On 2015.03.06 09:44:07 +, Chris Wilson wrote: Userspace can pass in an offset that it presumes the object is located at. The kernel will then do its utmost to fit the object into that location. The assumption is that userspace is handling its own object locations (for example along with full-ppgtt) and that the kernel will rarely have to make space for the user's requests. Chris, would you add libdrm support for this? e.g beignet doesn't handle exec object itself but use libdrm. -- Open Source Technology Center, Intel ltd. $gpg --keyserver wwwkeys.pgp.net --recv-keys 4D781827 signature.asc Description: Digital signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] RFC: drm: add support for tiled/compressed/etc modifier in addfb2
+ +#define fourcc_mod_code(vendor, val) \ + u64)DRM_FORMAT_MOD_VENDOR_## vendor) 56) | (val 0x00ffL)) eh, yeah no, /home/airlied/kernel/drm-2.6/drivers/gpu/drm/i915/intel_pm.c: In function ‘skl_wm_method2’: /home/airlied/kernel/drm-2.6/drivers/gpu/drm/i915/intel_pm.c:2631: warning: integer constant is too large for ‘long’ type /home/airlied/kernel/drm-2.6/drivers/gpu/drm/i915/intel_pm.c:2632: warning: integer constant is too large for ‘long’ type I think you meant ULL here. Dave. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v4 1/8] drm/i915: Introduce a PV INFO page structure for Intel GVT-g.
+/* + * The following structure pages are defined in GEN MMIO space + * for virtualization. (One page for now) + */ +#define VGT_MAGIC 0x4776544776544776 /* 'vGTvGTvG' */ one of my dusty 32-bit compilers dislikes this, CC [M] drivers/gpu/drm/i915/i915_vgpu.o /home/airlied/kernel/drm-2.6/drivers/gpu/drm/i915/i915_vgpu.c: In function ‘i915_check_vgpu’: /home/airlied/kernel/drm-2.6/drivers/gpu/drm/i915/i915_vgpu.c:73: warning: integer constant is too large for ‘long’ type Dave. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 9/9] drm/i915: Don't re-enable an explicitly disabled primary plane due to sprite coverage changes
On Wed, Mar 11, 2015 at 11:00:32AM +0100, Daniel Vetter wrote:
On Tue, Mar 10, 2015 at 01:15:29PM +0200, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
When the sprite is covering the entire pipe (and color keying is not
enabled) we currently try to automagically disable the primary plane
which is fully covered by the sprite.
Now that .crtc_disable() will simply disable planes by setting the
state-visible to false, the sprite code will actually end up
re-enabling the primary after it got disabled, and then we proceed to
turn off the pipe and are greeted with some warnings from
assert_plane_disabled().
The code doing the automagic disable of covered planes needs to
rewritten to do things correctly as part of the atomic update (or simply
removed), but in the meantime add a a bit of duct tape and simply have
the sprite code check the primary plane's state-visible before
re-enabling it.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_sprite.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index a828736..7286497 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1287,7 +1287,9 @@ intel_commit_sprite_plane(struct drm_plane *plane,
intel_plane-obj = obj;
if (intel_crtc-active) {
- intel_crtc-primary_enabled = !state-hides_primary;
+ intel_crtc-primary_enabled =
+ to_intel_plane_state(crtc-primary-state)-visible
+ !state-hides_primary;
Can't we just nuke intel_crtc-primary_enabled with all your work to tread
state through functions correctly?
Not if we want to keep this magic disable primary when sprite covers
it code.
I think ideally we'd do the .atomic_check() for all planes, and then
once all planes are clipped and whatnot, we could check which planes
are fully covered by something opaque and make them invisible. Though
that would perhaps mean that we always have to .atomic_check() all the
planes even if only one of them changed.
-Daniel
if (state-visible) {
crtc_x = state-dst.x1;
--
2.0.5
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Optimistically spin for the request completion
On Tue, Mar 10, 2015 at 04:14:14PM +, Chris Wilson wrote: On Tue, Mar 10, 2015 at 04:06:19PM +, Chris Wilson wrote: @@ -1235,12 +1257,20 @@ int __i915_wait_request(struct drm_i915_gem_request *req, if (ring-id == RCS INTEL_INFO(dev)-gen = 6) gen6_rps_boost(dev_priv, file_priv); - if (!irq_test_in_progress WARN_ON(!ring-irq_get(ring))) - return -ENODEV; - /* Record current time in case interrupted by signal, or wedged */ trace_i915_gem_request_wait_begin(req); before = ktime_get_raw_ns(); + + /* Optimistic spin before touching IRQs */ Perhaps iff timeout == NULL, or pass it along and add a if (timeout timeout_after_eq(jiffies, timeout)) break; before the cpu_relax()? I guess the answer for that is asking how many apps use short opportunistic waits in the frame rendering loop, e.g. to wait for query results and fall back to the ones from the previous frame if they're not available? Also do you have microbenchmark numbers for something midly ridiculous like a loop of very short batches (enough ofc to cause a bit of delay) and immediately stalling for them? It's definitely an awesome idea given that every other lock and sync primitive does it too. -Daniel + ret = __i915_spin_request(req); + if (ret == 0) + goto out; -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- 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 4/9] drm/i915: Make derived plane state correct after crtc_enable
On Wed, Mar 11, 2015 at 12:05:39PM +0200, Ville Syrjälä wrote:
On Wed, Mar 11, 2015 at 10:52:29AM +0100, Daniel Vetter wrote:
On Tue, Mar 10, 2015 at 07:57:13PM +0200, Ville Syrjälä wrote:
On Tue, Mar 10, 2015 at 10:01:51AM -0700, Matt Roper wrote:
On Tue, Mar 10, 2015 at 01:15:24PM +0200, ville.syrj...@linux.intel.com
wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
-static void disable_plane_internal(struct drm_plane *plane)
+static void _intel_crtc_enable_planes(struct intel_crtc *crtc)
{
- struct intel_plane *intel_plane = to_intel_plane(plane);
- struct drm_plane_state *state =
- plane-funcs-atomic_duplicate_state(plane);
- struct intel_plane_state *intel_state =
to_intel_plane_state(state);
+ struct drm_device *dev = crtc-base.dev;
+ enum pipe pipe = crtc-pipe;
+ struct intel_plane *plane;
+ const struct drm_crtc_helper_funcs *crtc_funcs =
+ crtc-base.helper_private;
- intel_state-visible = false;
- intel_plane-commit_plane(plane, intel_state);
+ for_each_intel_plane(dev, plane) {
+ const struct drm_plane_helper_funcs *funcs =
+ plane-base.helper_private;
+ struct intel_plane_state *state =
+ to_intel_plane_state(plane-base.state);
- intel_plane_destroy_state(plane, state);
+ if (plane-pipe != pipe)
+ continue;
+
+ if (funcs-atomic_check(plane-base, state-base))
Maybe add a WARN_ON() here? I'm assuming that this shouldn't really be
possible since if this fails it means we've already previously done a
commit of invalid state on a previous atomic transaction. But if it
does somehow happen, the WARN will give us a clue why the plane contents
simply didn't show up.
I can think of one way to make it fail. That is, first set a smaller
mode with the primary plane (and fb) configured to cover that fully, and
then switch to a larger mode without reconfiguring the primary plane. If
the hardware requires the primary plane to be fullscreen it'll fail. But
that should actaully not be possible using the legacy modeset API as it
always reconfigures the primary, so we'd only have to worry about that
with full atomic modeset, and for that we anyway need to change the code
to do the check stuff up front.
So yeah, with the way things are this should not be able to fail. I'll
respin with the WARN.
I haven't fully dug into the details here, but a few randome comments:
- While transitioning we're calling the transitional plane helpers, which
should call the atomic_check stuff for us on the primary plane. If we
need to call atomic_check on other planes too (why?)
Because we want the derived state to be updated to match the (potentially
changed) crtc config. We do call the .update_plane() hook from the
modeset path, but that happens at a time when the pipe is off, so our
clipping calculations end up saying the plane is invisible. I think fixing
that the right way pretty much involves the atomic conversion of the
modeset path.
Why do we conclude it's invisible? If we can fix the state to not depend
upon the dpms state then things should work ...
then I think that
should be done as close as possible to where we do that for the primary
one. Since eventually we need to unbury that call again.
With my patch _all_ planes get their .atomic_check() called in the same
place (during plane enable phase of .crtc_enable()).
- I don't like frobbing state objects which are committed (i.e. updating
visible like here), they're supposed to be invariant. With proper atomic
the way to deal with that is to grab all the required plane states and
put them into the drm_atomic_state update structure.
We really want to frob it so that the derived state will reflect
reality. Most importantly state-visible should be false whenever the
pipe is off, otherwise we can't trust state-visible and would also need
go look at the crtc state whenever we're trying to decide if the plane
is actually on or not.
Imo that's the correct thing to do. Calling plane hooks when the pipe is
off just doesn't seem like a good idea to me. Together with runtime pm at
least, crtc/atomic helpers have some interesting heritage.
But even there you can fix it by just reordering the commit_planes call to
the bottom, where everything should be on. Iirc that's how Laurent fixed
up rcar runtime pm issues to avoid touching planes when the hw is off.
The other reason why -visible must take into account dpms state is that
we'll screw up the watermark computation otherwise. Which could result
into a failure on a subsequent dpms on, which is a big no-no.
As for the direct state frobbing, we could make a copy I guess
[Intel-gfx] [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept
Remove the global modeset resource function that would disable the
bifurcation bit, and instead enable/disable it when enabling the pch
transcoder. The mode set consistency check should prevent us from
disabling the bit if pipe C is enabled so the change should be safe.
Note that this doens't affect the logic that prevents the bit being
set while a pipe is active, since the patch retains the behavior of
only chaging the bit if necessary. Because of the checks during mode
set, the first change would necessarily happen with both pipes B and
C disabled, and any subsequent write would be skipped.
v2: Only change the bit during pch trancoder enable. (Ville)
---
drivers/gpu/drm/i915/intel_display.c | 46
1 file changed, 10 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 4008bf4..bfbd829 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc *crtc)
return crtc-base.state-enable crtc-config-has_pch_encoder;
}
-static void ivb_modeset_global_resources(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev-dev_private;
- struct intel_crtc *pipe_B_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_B]);
- struct intel_crtc *pipe_C_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_C]);
- uint32_t temp;
-
- /*
-* When everything is off disable fdi C so that we could enable fdi B
-* with all lanes. Note that we don't care about enabled pipes without
-* an enabled pch encoder.
-*/
- if (!pipe_has_enabled_pch(pipe_B_crtc)
- !pipe_has_enabled_pch(pipe_C_crtc)) {
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
-
- temp = I915_READ(SOUTH_CHICKEN1);
- temp = ~FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(disabling fdi C rx\n);
- I915_WRITE(SOUTH_CHICKEN1, temp);
- }
-}
-
/* The FDI link training functions for ILK/Ibexpeak. */
static void ironlake_fdi_link_train(struct drm_crtc *crtc)
{
@@ -3834,20 +3808,23 @@ static void ironlake_pch_transcoder_set_timings(struct
intel_crtc *crtc,
I915_READ(VSYNCSHIFT(cpu_transcoder)));
}
-static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
+static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
{
struct drm_i915_private *dev_priv = dev-dev_private;
uint32_t temp;
temp = I915_READ(SOUTH_CHICKEN1);
- if (temp FDI_BC_BIFURCATION_SELECT)
+ if (!!(temp FDI_BC_BIFURCATION_SELECT) == enable)
return;
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
- temp |= FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(enabling fdi C rx\n);
+ temp = ~FDI_BC_BIFURCATION_SELECT;
+ if (enable)
+ temp |= FDI_BC_BIFURCATION_SELECT;
+
+ DRM_DEBUG_KMS(%sabling fdi C rx\n, enable ? en : dis);
I915_WRITE(SOUTH_CHICKEN1, temp);
POSTING_READ(SOUTH_CHICKEN1);
}
@@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct
drm_device *dev)
static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc *intel_crtc)
{
struct drm_device *dev = intel_crtc-base.dev;
- struct drm_i915_private *dev_priv = dev-dev_private;
switch (intel_crtc-pipe) {
case PIPE_A:
break;
case PIPE_B:
if (intel_crtc-config-fdi_lanes 2)
- WARN_ON(I915_READ(SOUTH_CHICKEN1)
FDI_BC_BIFURCATION_SELECT);
+ cpt_set_fdi_bc_bifurcation(dev, false);
else
- cpt_enable_fdi_bc_bifurcation(dev);
+ cpt_set_fdi_bc_bifurcation(dev, true);
break;
case PIPE_C:
- cpt_enable_fdi_bc_bifurcation(dev);
+ cpt_set_fdi_bc_bifurcation(dev, true);
break;
default:
@@ -13056,8 +13032,6 @@ static void intel_init_display(struct drm_device *dev)
} else if (IS_IVYBRIDGE(dev)) {
/* FIXME: detect B0+ stepping and use auto training */
dev_priv-display.fdi_link_train = ivb_manual_fdi_link_train;
- dev_priv-display.modeset_global_resources =
- ivb_modeset_global_resources;
} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
dev_priv-display.fdi_link_train = hsw_fdi_link_train;
} else if (IS_VALLEYVIEW(dev)) {
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
Re: [Intel-gfx] [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept
On Wed, 2015-03-11 at 13:35 +0200, Ander Conselvan de Oliveira wrote:
Remove the global modeset resource function that would disable the
bifurcation bit, and instead enable/disable it when enabling the pch
transcoder. The mode set consistency check should prevent us from
disabling the bit if pipe C is enabled so the change should be safe.
Note that this doens't affect the logic that prevents the bit being
set while a pipe is active, since the patch retains the behavior of
only chaging the bit if necessary. Because of the checks during mode
set, the first change would necessarily happen with both pipes B and
C disabled, and any subsequent write would be skipped.
v2: Only change the bit during pch trancoder enable. (Ville)
Oops, I forgot the sob line.
Signed-off-by: Ander Conselvan de Oliveira
ander.conselvan.de.olive...@intel.com
---
drivers/gpu/drm/i915/intel_display.c | 46
1 file changed, 10 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 4008bf4..bfbd829 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc
*crtc)
return crtc-base.state-enable crtc-config-has_pch_encoder;
}
-static void ivb_modeset_global_resources(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev-dev_private;
- struct intel_crtc *pipe_B_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_B]);
- struct intel_crtc *pipe_C_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_C]);
- uint32_t temp;
-
- /*
- * When everything is off disable fdi C so that we could enable fdi B
- * with all lanes. Note that we don't care about enabled pipes without
- * an enabled pch encoder.
- */
- if (!pipe_has_enabled_pch(pipe_B_crtc)
- !pipe_has_enabled_pch(pipe_C_crtc)) {
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
-
- temp = I915_READ(SOUTH_CHICKEN1);
- temp = ~FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(disabling fdi C rx\n);
- I915_WRITE(SOUTH_CHICKEN1, temp);
- }
-}
-
/* The FDI link training functions for ILK/Ibexpeak. */
static void ironlake_fdi_link_train(struct drm_crtc *crtc)
{
@@ -3834,20 +3808,23 @@ static void
ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
I915_READ(VSYNCSHIFT(cpu_transcoder)));
}
-static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
+static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
{
struct drm_i915_private *dev_priv = dev-dev_private;
uint32_t temp;
temp = I915_READ(SOUTH_CHICKEN1);
- if (temp FDI_BC_BIFURCATION_SELECT)
+ if (!!(temp FDI_BC_BIFURCATION_SELECT) == enable)
return;
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
- temp |= FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(enabling fdi C rx\n);
+ temp = ~FDI_BC_BIFURCATION_SELECT;
+ if (enable)
+ temp |= FDI_BC_BIFURCATION_SELECT;
+
+ DRM_DEBUG_KMS(%sabling fdi C rx\n, enable ? en : dis);
I915_WRITE(SOUTH_CHICKEN1, temp);
POSTING_READ(SOUTH_CHICKEN1);
}
@@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct
drm_device *dev)
static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc
*intel_crtc)
{
struct drm_device *dev = intel_crtc-base.dev;
- struct drm_i915_private *dev_priv = dev-dev_private;
switch (intel_crtc-pipe) {
case PIPE_A:
break;
case PIPE_B:
if (intel_crtc-config-fdi_lanes 2)
- WARN_ON(I915_READ(SOUTH_CHICKEN1)
FDI_BC_BIFURCATION_SELECT);
+ cpt_set_fdi_bc_bifurcation(dev, false);
else
- cpt_enable_fdi_bc_bifurcation(dev);
+ cpt_set_fdi_bc_bifurcation(dev, true);
break;
case PIPE_C:
- cpt_enable_fdi_bc_bifurcation(dev);
+ cpt_set_fdi_bc_bifurcation(dev, true);
break;
default:
@@ -13056,8 +13032,6 @@ static void intel_init_display(struct drm_device *dev)
} else if (IS_IVYBRIDGE(dev)) {
/* FIXME: detect B0+ stepping and use auto training */
dev_priv-display.fdi_link_train = ivb_manual_fdi_link_train;
- dev_priv-display.modeset_global_resources =
- ivb_modeset_global_resources;
} else if (IS_HASWELL(dev) || IS_BROADWELL(dev)) {
dev_priv-display.fdi_link_train =
[Intel-gfx] [PATCH igt 2/2] tests: Add test for pipe B and C interactions in IVB
The tests exercise different combinations of enabling pipe B with modes
that require more than 2 lanes and then enabling pipe C.
v2: Added a couple more tests for different pipe transitions. (Ander)
Use custom modes to make the test reliable. (Daniel)
---
tests/Makefile.sources| 1 +
tests/kms_pipe_b_c_dpms.c | 286 ++
2 files changed, 287 insertions(+)
create mode 100644 tests/kms_pipe_b_c_dpms.c
diff --git a/tests/Makefile.sources b/tests/Makefile.sources
index 9ab0ff4..9e43a64 100644
--- a/tests/Makefile.sources
+++ b/tests/Makefile.sources
@@ -73,6 +73,7 @@ TESTS_progs_M = \
kms_nuclear \
kms_pipe_crc_basic \
kms_plane \
+ kms_pipe_b_c_dpms \
kms_psr_sink_crc \
kms_render \
kms_rotation_crc \
diff --git a/tests/kms_pipe_b_c_dpms.c b/tests/kms_pipe_b_c_dpms.c
new file mode 100644
index 000..69394f4
--- /dev/null
+++ b/tests/kms_pipe_b_c_dpms.c
@@ -0,0 +1,286 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the Software),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED AS IS, WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ * Authors:
+ * Ander Conselvan de Oliveira ander.conselvan.de.olive...@intel.com
+ */
+
+#include drmtest.h
+#include igt_kms.h
+#include intel_chipset.h
+
+typedef struct {
+ int drm_fd;
+ igt_display_t display;
+} data_t;
+
+drmModeModeInfo mode_3_lanes = {
+ .clock = 173000,
+ .hdisplay = 1920,
+ .hsync_start = 2048,
+ .hsync_end = 2248,
+ .htotal = 2576,
+ .vdisplay = 1080,
+ .vsync_start = 1083,
+ .vsync_end = 1088,
+ .vtotal = 1120,
+ .vrefresh = 60,
+ .flags = DRM_MODE_FLAG_NHSYNC | DRM_MODE_FLAG_PVSYNC,
+ .name = 3_lanes,
+};
+
+drmModeModeInfo mode_2_lanes = {
+ .clock = 138500,
+ .hdisplay = 1920,
+ .hsync_start = 1968,
+ .hsync_end = 2000,
+ .htotal = 2080,
+ .vdisplay = 1080,
+ .vsync_start = 1083,
+ .vsync_end = 1088,
+ .vtotal = ,
+ .vrefresh = 60,
+ .flags = DRM_MODE_FLAG_PHSYNC | DRM_MODE_FLAG_NVSYNC,
+ .name = 2_lanes,
+};
+
+static int
+disable_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+ igt_plane_t *primary;
+
+ igt_output_set_pipe(output, pipe);
+ primary = igt_output_get_plane(output, 0);
+ igt_plane_set_fb(primary, NULL);
+ return igt_display_commit(data-display);
+}
+
+static int
+set_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+ igt_plane_t *primary;
+ drmModeModeInfo *mode;
+ struct igt_fb fb;
+ int fb_id;
+
+ igt_output_set_pipe(output, pipe);
+
+ mode = igt_output_get_mode(output);
+
+ primary = igt_output_get_plane(output, 0);
+
+ fb_id = igt_create_color_fb(data-drm_fd,
+ mode-hdisplay, mode-vdisplay,
+ DRM_FORMAT_XRGB, I915_TILING_NONE,
+ 1.0, 1.0, 1.0, fb);
+ igt_assert(fb_id = 0);
+
+ igt_plane_set_fb(primary, fb);
+ return igt_display_try_commit2(data-display, COMMIT_LEGACY);
+}
+
+static int
+set_big_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+ igt_output_override_mode(output, mode_3_lanes);
+ return set_mode_on_pipe(data, pipe, output);
+}
+
+static int
+set_normal_mode_on_pipe(data_t *data, enum pipe pipe, igt_output_t *output)
+{
+ igt_output_override_mode(output, mode_2_lanes);
+ return set_mode_on_pipe(data, pipe, output);
+}
+
+static void
+find_outputs(data_t *data, igt_output_t **output1, igt_output_t **output2)
+{
+ int count = 0;
+ igt_output_t *output;
+
+ *output1 = NULL;
+ *output2 = NULL;
+
+ for_each_connected_output(data-display, output) {
+ if (!(*output1))
+ *output1 = output;
+ else if (!(*output2))
+
[Intel-gfx] [PATCH igt 1/2] lib/kms: Add a way to override an output's mode
So that it is possible to use a custom mode with the simplified mode set API.
---
lib/igt_kms.c | 9 +
lib/igt_kms.h | 4
2 files changed, 13 insertions(+)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 26e4913..0dccd2d 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -895,6 +895,9 @@ static void igt_output_refresh(igt_output_t *output)
if (!output-valid)
return;
+ if (output-use_override_mode)
+ output-config.default_mode = output-override_mode;
+
if (!output-name) {
drmModeConnector *c = output-config.connector;
@@ -1797,6 +1800,12 @@ drmModeModeInfo *igt_output_get_mode(igt_output_t
*output)
return output-config.default_mode;
}
+void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode)
+{
+ output-override_mode = *mode;
+ output-use_override_mode = true;
+}
+
void igt_output_set_pipe(igt_output_t *output, enum pipe pipe)
{
igt_display_t *display = output-display;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 2fab30e..ddf4432 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -228,6 +228,9 @@ typedef struct {
bool valid;
unsigned long pending_crtc_idx_mask;
+ bool use_override_mode;
+ drmModeModeInfo override_mode;
+
#ifdef HAVE_ATOMIC
/* Property set for nuclear pageflip */
drmModePropertySetPtr set;
@@ -255,6 +258,7 @@ int igt_display_get_n_pipes(igt_display_t *display);
const char *igt_output_name(igt_output_t *output);
drmModeModeInfo *igt_output_get_mode(igt_output_t *output);
+void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode);
void igt_output_set_pipe(igt_output_t *output, enum pipe pipe);
igt_plane_t *igt_output_get_plane(igt_output_t *output, enum igt_plane plane);
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Wed, Mar 11, 2015 at 10:39:31AM +0530, sonika wrote:
On Tuesday 10 March 2015 04:45 PM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
In preparation to movable/resizable primary planes pass the clipped
plane size to .update_primary_plane().
Cc: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 63
2 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
index b16c0a7..e99eef0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -578,7 +578,8 @@ struct drm_i915_display_funcs {
uint32_t flags);
void (*update_primary_plane)(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
-int x, int y);
+int x, int y,
+int crtc_w, int crtc_h);
void (*hpd_irq_setup)(struct drm_device *dev);
/* clock updates for mode set */
/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index fdc83f1..1a789f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2482,7 +2482,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2517,20 +2518,16 @@ static void i9xx_update_primary_plane(struct
drm_crtc *crtc,
if (INTEL_INFO(dev)-gen 4) {
if (intel_crtc-pipe == PIPE_B)
dspcntr |= DISPPLANE_SEL_PIPE_B;
-
- /* pipesrc and dspsize control the size that is scaled from,
-* which should always be the user's requested size.
-*/
- I915_WRITE(DSPSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(DSPSIZE(plane), ((crtc_h - 1) 16) | (crtc_w - 1));
I915_WRITE(DSPPOS(plane), 0);
} else if (IS_CHERRYVIEW(dev) plane == PLANE_B) {
- I915_WRITE(PRIMSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(PRIMSIZE(plane), ((crtc_h - 1) 16) | (crtc_w -
1));
I915_WRITE(PRIMPOS(plane), 0);
I915_WRITE(PRIMCNSTALPHA(plane), 0);
+ } else {
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+ primary plane size doesn't match pipe size\n);
}
switch (fb-pixel_format) {
@@ -2586,14 +2583,14 @@ static void i9xx_update_primary_plane(struct
drm_crtc *crtc,
if (crtc-primary-state-rotation == BIT(DRM_ROTATE_180)) {
dspcntr |= DISPPLANE_ROTATE_180;
- x += (intel_crtc-config-pipe_src_w - 1);
- y += (intel_crtc-config-pipe_src_h - 1);
+ x += (crtc_w - 1);
+ y += (crtc_h - 1);
/* Finding the last pixel of the last line of the display
data and adding to linear_offset*/
linear_offset +=
- (intel_crtc-config-pipe_src_h - 1) * fb-pitches[0] +
- (intel_crtc-config-pipe_src_w - 1) * pixel_size;
+ (crtc_h - 1) * fb-pitches[0] +
+ (crtc_w - 1) * pixel_size;
}
I915_WRITE(reg, dspcntr);
@@ -2611,7 +2608,8 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
static void ironlake_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2643,6 +2641,10 @@ static void ironlake_update_primary_plane(struct
drm_crtc *crtc,
if (IS_HASWELL(dev) || IS_BROADWELL(dev))
dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+
Re: [Intel-gfx] [PATCH 3/9] drm/i915: Use plane-state-fb instead of plane-fb in intel_plane_restore()
On Tue, Mar 10, 2015 at 07:48:39PM +0200, Ville Syrjälä wrote:
On Tue, Mar 10, 2015 at 10:01:47AM -0700, Matt Roper wrote:
On Tue, Mar 10, 2015 at 01:15:23PM +0200, ville.syrj...@linux.intel.com
wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
plane-fb is not as reliable as plane-state-fb so let's convert
intel_plane_restore() over the the new way of thinking as well.
Cc: Matt Roper matthew.d.ro...@intel.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_sprite.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index 7051da7..a828736 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1361,10 +1361,10 @@ out_unlock:
int intel_plane_restore(struct drm_plane *plane)
{
- if (!plane-crtc || !plane-fb)
+ if (!plane-crtc || !plane-state-fb)
return 0;
- return plane-funcs-update_plane(plane, plane-crtc, plane-fb,
+ return plane-funcs-update_plane(plane, plane-crtc, plane-state-fb,
While we're at it, should we s/plane-crtc/plane-state-crtc/ as well?
I tried to make that change everywhere and it blew up. But I think that
was simply because I changed it some .commit hook as well, and currently
we don't have the old state around there, so the 'crtc ? crtc : state-crtc'
just ended up as 'crtc' effectively and that of course didn't work as well
as I'd hoped ;)
But yeah maybe we should make that change. Would just need to pass the
old state to commit instead of the new state, I think.
Not sure we should be too agressive with mass-conversion. E.g. for the
plane restore I expect that we'll do some overall atomic helper to
snapshot/restore all the state for suspend/resume and similar cases.
-Daniel
Otherwise, 1-3 are
Reviewed-by: Matt Roper matthew.d.ro...@intel.com
plane-state-crtc_x, plane-state-crtc_y,
plane-state-crtc_w, plane-state-crtc_h,
plane-state-src_x, plane-state-src_y,
--
2.0.5
--
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling Development
Intel Corporation
(916) 356-2795
--
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
+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: Optimistically spin for the request completion
On Wed, Mar 11, 2015 at 11:13:59AM +0100, Daniel Vetter wrote: On Tue, Mar 10, 2015 at 04:14:14PM +, Chris Wilson wrote: On Tue, Mar 10, 2015 at 04:06:19PM +, Chris Wilson wrote: @@ -1235,12 +1257,20 @@ int __i915_wait_request(struct drm_i915_gem_request *req, if (ring-id == RCS INTEL_INFO(dev)-gen = 6) gen6_rps_boost(dev_priv, file_priv); - if (!irq_test_in_progress WARN_ON(!ring-irq_get(ring))) - return -ENODEV; - /* Record current time in case interrupted by signal, or wedged */ trace_i915_gem_request_wait_begin(req); before = ktime_get_raw_ns(); + + /* Optimistic spin before touching IRQs */ Perhaps iff timeout == NULL, or pass it along and add a if (timeout timeout_after_eq(jiffies, timeout)) break; before the cpu_relax()? I guess the answer for that is asking how many apps use short opportunistic waits in the frame rendering loop, e.g. to wait for query results and fall back to the ones from the previous frame if they're not available? My presumption is that this would help most with occlusion query bound applications (in terms of real world impact). Applications that have readback in their critical path aren't really that exciting... I was trying to see if cities skylines would benefit... But that seems broken with multimonitor setups :| Also do you have microbenchmark numbers for something midly ridiculous like a loop of very short batches (enough ofc to cause a bit of delay) and immediately stalling for them? It's definitely an awesome idea given that every other lock and sync primitive does it too. Urm, you are describing exactly how mesa behaves in swap benchmarks. Admittedly there isn't much room for improvement after the throttle adjustment patches land in mesa, but it is still there. It does increase CPU load greatly in memory bound swap benchmarks, and I wonder how much of the performance increase is from keeping the CPU from going to sleep (i.e. preventing cpufreq from destroying the benchmark). I guess I have an exciting morning of letting synmark run on one machine in various configs. -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 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Wednesday 11 March 2015 02:57 PM, Ville Syrjälä wrote:
On Wed, Mar 11, 2015 at 10:39:31AM +0530, sonika wrote:
On Tuesday 10 March 2015 04:45 PM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
In preparation to movable/resizable primary planes pass the clipped
plane size to .update_primary_plane().
Cc: Sonika Jindal sonika.jin...@intel.com
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.h | 3 +-
drivers/gpu/drm/i915/intel_display.c | 63
2 files changed, 38 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b16c0a7..e99eef0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -578,7 +578,8 @@ struct drm_i915_display_funcs {
uint32_t flags);
void (*update_primary_plane)(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
-int x, int y);
+int x, int y,
+int crtc_w, int crtc_h);
void (*hpd_irq_setup)(struct drm_device *dev);
/* clock updates for mode set */
/* cursor updates */
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index fdc83f1..1a789f0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2482,7 +2482,8 @@ intel_find_plane_obj(struct intel_crtc *intel_crtc,
static void i9xx_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2517,20 +2518,16 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
if (INTEL_INFO(dev)-gen 4) {
if (intel_crtc-pipe == PIPE_B)
dspcntr |= DISPPLANE_SEL_PIPE_B;
-
- /* pipesrc and dspsize control the size that is scaled from,
-* which should always be the user's requested size.
-*/
- I915_WRITE(DSPSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(DSPSIZE(plane), ((crtc_h - 1) 16) | (crtc_w - 1));
I915_WRITE(DSPPOS(plane), 0);
} else if (IS_CHERRYVIEW(dev) plane == PLANE_B) {
- I915_WRITE(PRIMSIZE(plane),
- ((intel_crtc-config-pipe_src_h - 1) 16) |
- (intel_crtc-config-pipe_src_w - 1));
+ I915_WRITE(PRIMSIZE(plane), ((crtc_h - 1) 16) | (crtc_w -
1));
I915_WRITE(PRIMPOS(plane), 0);
I915_WRITE(PRIMCNSTALPHA(plane), 0);
+ } else {
+ WARN_ONCE(crtc_w != intel_crtc-config-pipe_src_w ||
+ crtc_h != intel_crtc-config-pipe_src_h,
+ primary plane size doesn't match pipe size\n);
}
switch (fb-pixel_format) {
@@ -2586,14 +2583,14 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
if (crtc-primary-state-rotation == BIT(DRM_ROTATE_180)) {
dspcntr |= DISPPLANE_ROTATE_180;
- x += (intel_crtc-config-pipe_src_w - 1);
- y += (intel_crtc-config-pipe_src_h - 1);
+ x += (crtc_w - 1);
+ y += (crtc_h - 1);
/* Finding the last pixel of the last line of the display
data and adding to linear_offset*/
linear_offset +=
- (intel_crtc-config-pipe_src_h - 1) * fb-pitches[0] +
- (intel_crtc-config-pipe_src_w - 1) * pixel_size;
+ (crtc_h - 1) * fb-pitches[0] +
+ (crtc_w - 1) * pixel_size;
}
I915_WRITE(reg, dspcntr);
@@ -2611,7 +2608,8 @@ static void i9xx_update_primary_plane(struct drm_crtc
*crtc,
static void ironlake_update_primary_plane(struct drm_crtc *crtc,
struct drm_framebuffer *fb,
- int x, int y)
+ int x, int y,
+ int crtc_w, int crtc_h)
{
struct drm_device *dev = crtc-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
@@ -2643,6 +2641,10 @@ static void ironlake_update_primary_plane(struct
drm_crtc *crtc,
if (IS_HASWELL(dev) || IS_BROADWELL(dev))
dspcntr |= DISPPLANE_PIPE_CSC_ENABLE;
+ WARN_ONCE(crtc_w !=
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Tue, Mar 10, 2015 at 01:57:09PM -0700, Matt Roper wrote: On Tue, Mar 10, 2015 at 07:59:15PM +0200, Ville Syrjälä wrote: On Tue, Mar 10, 2015 at 10:10:40AM -0700, Matt Roper wrote: On Tue, Mar 10, 2015 at 01:15:25PM +0200, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com In preparation to movable/resizable primary planes pass the clipped plane size to .update_primary_plane(). Personally I feel like it would make more sense to just completely kill off .update_primary_plane() now rather than trying to evolve it. We already have an intel_plane-update_plane() function pointer which is never set or called for non-sprites at the moment. We could unify the handling of low-level plane programming by just using that function pointer for primary planes as well. I want to kill it off as well, but that means either killing off set_base_atomic() or making it use the plane commit hook. I suppose we could hand craft a suitable plane state for it and just commit that without any checks or anything? I'm not sure I follow your concern about set_base_atomic(). After your patch series, it'll be calling dev_priv-display.update_primary_plane(crtc, fb, x, y, 0, 0, intel_crtc-config-pipe_src_w, intel_crtc-config-pipe_src_h); which basically directly programs the hardware for the primary plane and doesn't do anything state-related. It seems to me that just making that low-level call be: intel_plane = to_intel_plane(crtc-primary); intel_state = to_intel_plane_state(crtc-primary-state); intel_plane-update_plane(crtc-primary, crtc, fb, intel_fb_obj(fb), 0, 0, intel_crtc-config-pipe_src_w, intel_crtc-config-pipe_src_h, x, y, drm_rect_width(intel_state-src), drm_rect_height(intel_state-src)); We can't really use the current plane state here. It might not match what we want. Although as I indicated with one FIXME in these patches, set_base_atomic() will do something bad anyway if the fbdev framebuffer is too small for the current pipe source size. I suppose we could try to enable the panel fitter to avoid that, but then we run into problems with managing the panel fitter which is a scarce resource (and there is only one on gmch platforms and potentially with extra restrictions on which pipes can use it). Maybe we should just sanity check that the fbdev framebuffer is suitably large for the current mode/pfit settings, and do nothing if it's not? Or we could try to use a plane that can be resized (or potentially even scaled) to preset the fbdev framebuffer. The other option would be to throw out set_base_atomic() entirely. I have no idea if it works at all currently. And, as you've mentioned .update_plane(), I'm actually thinking we'll be wanting to pass just a plane state there, and throw out most of the function arguments as all that data should be in the plane state already. And then we'd probably want to hand craft the plane state we pass into .update_plane() for set_base_atomic() (assuming we'll keep it at all) so that we don't leak fb references and whatnot. But all of that is material for another set of patches. shouldn't make a difference; the implementation of intel_plane-update_plane() would still be the same low-level register programming without any state manipulation, the only difference being that we get an extra pair of parameters containing source rect width/height. This would also allow us to point both primary and sprite planes at the same function on SKL, which has two nearly-identical functions at the moment for the lowest-level plane hardware programming. As I've noted in my reply to Sonika, the primary_enabled flag is the main complication here. We really need to sort that out before can fully unify this stuff. Matt I wouldn't mind also seeing the name of that low-level entrypoint renamed to something like 'update_hw_plane' to avoid confusion with the drm_plane entrypoint... Matt Cc: Sonika Jindal sonika.jin...@intel.com Signed-off-by: Ville
Re: [Intel-gfx] [PATCH 5/9] drm/i915: Pass primary plane size to .update_primary_plane()
On Tue, Mar 10, 2015 at 07:59:15PM +0200, Ville Syrjälä wrote: On Tue, Mar 10, 2015 at 10:10:40AM -0700, Matt Roper wrote: On Tue, Mar 10, 2015 at 01:15:25PM +0200, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com In preparation to movable/resizable primary planes pass the clipped plane size to .update_primary_plane(). Personally I feel like it would make more sense to just completely kill off .update_primary_plane() now rather than trying to evolve it. We already have an intel_plane-update_plane() function pointer which is never set or called for non-sprites at the moment. We could unify the handling of low-level plane programming by just using that function pointer for primary planes as well. I want to kill it off as well, but that means either killing off set_base_atomic() or making it use the plane commit hook. I suppose we could hand craft a suitable plane state for it and just commit that without any checks or anything? set_base_atomic is bonghits imo, I think we should just replace it with the set_base helper for the transitional helpers. set_base_atomic can't grab locks and assumes that the buffer is pinned already. Hm, so maybe a special version of the plane helper which forgoes the prepare/celanup_fb? -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 9/9] drm/i915: Don't re-enable an explicitly disabled primary plane due to sprite coverage changes
On Wed, Mar 11, 2015 at 12:09:24PM +0200, Ville Syrjälä wrote:
On Wed, Mar 11, 2015 at 11:00:32AM +0100, Daniel Vetter wrote:
On Tue, Mar 10, 2015 at 01:15:29PM +0200, ville.syrj...@linux.intel.com
wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
When the sprite is covering the entire pipe (and color keying is not
enabled) we currently try to automagically disable the primary plane
which is fully covered by the sprite.
Now that .crtc_disable() will simply disable planes by setting the
state-visible to false, the sprite code will actually end up
re-enabling the primary after it got disabled, and then we proceed to
turn off the pipe and are greeted with some warnings from
assert_plane_disabled().
The code doing the automagic disable of covered planes needs to
rewritten to do things correctly as part of the atomic update (or simply
removed), but in the meantime add a a bit of duct tape and simply have
the sprite code check the primary plane's state-visible before
re-enabling it.
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/intel_sprite.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/intel_sprite.c
b/drivers/gpu/drm/i915/intel_sprite.c
index a828736..7286497 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1287,7 +1287,9 @@ intel_commit_sprite_plane(struct drm_plane *plane,
intel_plane-obj = obj;
if (intel_crtc-active) {
- intel_crtc-primary_enabled = !state-hides_primary;
+ intel_crtc-primary_enabled =
+ to_intel_plane_state(crtc-primary-state)-visible
+ !state-hides_primary;
Can't we just nuke intel_crtc-primary_enabled with all your work to tread
state through functions correctly?
Not if we want to keep this magic disable primary when sprite covers
it code.
I think ideally we'd do the .atomic_check() for all planes, and then
once all planes are clipped and whatnot, we could check which planes
are fully covered by something opaque and make them invisible. Though
that would perhaps mean that we always have to .atomic_check() all the
planes even if only one of them changed.
Yeah that's what I've meant with removing primary_enabled - this should
flow into the compuation of -visible for the primary plane. Keeping
random state out of state objects will be serious trouble.
Also if we do this the wm code will grow clue about the situation and stop
allocating fifo space for the primary plane in that case too.
-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 4/9] drm/i915: Make derived plane state correct after crtc_enable
On Tue, Mar 10, 2015 at 07:57:13PM +0200, Ville Syrjälä wrote:
On Tue, Mar 10, 2015 at 10:01:51AM -0700, Matt Roper wrote:
On Tue, Mar 10, 2015 at 01:15:24PM +0200, ville.syrj...@linux.intel.com
wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
-static void disable_plane_internal(struct drm_plane *plane)
+static void _intel_crtc_enable_planes(struct intel_crtc *crtc)
{
- struct intel_plane *intel_plane = to_intel_plane(plane);
- struct drm_plane_state *state =
- plane-funcs-atomic_duplicate_state(plane);
- struct intel_plane_state *intel_state = to_intel_plane_state(state);
+ struct drm_device *dev = crtc-base.dev;
+ enum pipe pipe = crtc-pipe;
+ struct intel_plane *plane;
+ const struct drm_crtc_helper_funcs *crtc_funcs =
+ crtc-base.helper_private;
- intel_state-visible = false;
- intel_plane-commit_plane(plane, intel_state);
+ for_each_intel_plane(dev, plane) {
+ const struct drm_plane_helper_funcs *funcs =
+ plane-base.helper_private;
+ struct intel_plane_state *state =
+ to_intel_plane_state(plane-base.state);
- intel_plane_destroy_state(plane, state);
+ if (plane-pipe != pipe)
+ continue;
+
+ if (funcs-atomic_check(plane-base, state-base))
Maybe add a WARN_ON() here? I'm assuming that this shouldn't really be
possible since if this fails it means we've already previously done a
commit of invalid state on a previous atomic transaction. But if it
does somehow happen, the WARN will give us a clue why the plane contents
simply didn't show up.
I can think of one way to make it fail. That is, first set a smaller
mode with the primary plane (and fb) configured to cover that fully, and
then switch to a larger mode without reconfiguring the primary plane. If
the hardware requires the primary plane to be fullscreen it'll fail. But
that should actaully not be possible using the legacy modeset API as it
always reconfigures the primary, so we'd only have to worry about that
with full atomic modeset, and for that we anyway need to change the code
to do the check stuff up front.
So yeah, with the way things are this should not be able to fail. I'll
respin with the WARN.
I haven't fully dug into the details here, but a few randome comments:
- While transitioning we're calling the transitional plane helpers, which
should call the atomic_check stuff for us on the primary plane. If we
need to call atomic_check on other planes too (why?) then I think that
should be done as close as possible to where we do that for the primary
one. Since eventually we need to unbury that call again.
- I don't like frobbing state objects which are committed (i.e. updating
visible like here), they're supposed to be invariant. With proper atomic
the way to deal with that is to grab all the required plane states and
put them into the drm_atomic_state update structure.
- My idea for resolving our current nesting issues with
enable/disable_planes functions was two parts: a) open-code a hardcoded
disable-all-planes function by just calling plane disable code
unconditionally. Iirc there's been patches once somewhere to do that
split in i915 (maybe I'm dreaming), but this use-case is also why I
added the atomic_plane_disable hook. b) on restore we just do a normal
plane commit with the unchanged plane states, they should all still
work.
btw if we wire up the atomic_disable_plane hook then we can rip out
intel_plane_atomic_update. The don't disable twice check is already done
by the helpers in that case.
I'll grab some coffee and see what's all wrong with my ideas here now, but
please bring in critique too ;-)
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] drm/i915: Use FW_WM() macro for older gmch platforms too
On Tue, Mar 10, 2015 at 02:50:16PM -0700, Jesse Barnes wrote:
On 03/10/2015 08:02 AM, ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
Use the FW_WM() macro from the VLV wm code to polish up the wm
code for older gmch platforms.
Cc: Daniel Vetter dan...@ffwll.ch
Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
---
drivers/gpu/drm/i915/i915_reg.h | 4 ++--
drivers/gpu/drm/i915/intel_pm.c | 42
+
2 files changed, 24 insertions(+), 22 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_reg.h
b/drivers/gpu/drm/i915/i915_reg.h
index 8ff039d..793ed63 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4121,8 +4121,8 @@ enum skl_disp_power_wells {
#define DSPFW_SPRITEB_MASK_VLV (0xff16) /* vlv/chv */
#define DSPFW_CURSORA_SHIFT 8
#define DSPFW_CURSORA_MASK (0x3f8)
-#define DSPFW_PLANEC_SHIFT_OLD 0
-#define DSPFW_PLANEC_MASK_OLD(0x7f0) /* pre-gen4 sprite C
*/
+#define DSPFW_PLANEC_OLD_SHIFT 0
+#define DSPFW_PLANEC_OLD_MASK(0x7f0) /* pre-gen4 sprite C
*/
#define DSPFW_SPRITEA_SHIFT 0
#define DSPFW_SPRITEA_MASK (0x7f0) /* g4x */
#define DSPFW_SPRITEA_MASK_VLV (0xff0) /* vlv/chv */
diff --git a/drivers/gpu/drm/i915/intel_pm.c
b/drivers/gpu/drm/i915/intel_pm.c
index 8ac358d0..9ac9a2f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -301,6 +301,9 @@ static void chv_set_memory_pm5(struct drm_i915_private
*dev_priv, bool enable)
mutex_unlock(dev_priv-rps.hw_lock);
}
+#define FW_WM(value, plane) \
+ (((value) DSPFW_ ## plane ## _SHIFT) DSPFW_ ## plane ## _MASK)
+
void intel_set_memory_cxsr(struct drm_i915_private *dev_priv, bool enable)
{
struct drm_device *dev = dev_priv-dev;
@@ -661,7 +664,7 @@ static void pineview_update_wm(struct drm_crtc
*unused_crtc)
pixel_size, latency-display_sr);
reg = I915_READ(DSPFW1);
reg = ~DSPFW_SR_MASK;
- reg |= wm DSPFW_SR_SHIFT;
+ reg |= FW_WM(wm, SR);
I915_WRITE(DSPFW1, reg);
DRM_DEBUG_KMS(DSPFW1 register is %x\n, reg);
@@ -671,7 +674,7 @@ static void pineview_update_wm(struct drm_crtc
*unused_crtc)
pixel_size, latency-cursor_sr);
reg = I915_READ(DSPFW3);
reg = ~DSPFW_CURSOR_SR_MASK;
- reg |= (wm 0x3f) DSPFW_CURSOR_SR_SHIFT;
+ reg |= FW_WM(wm, CURSOR_SR);
I915_WRITE(DSPFW3, reg);
/* Display HPLL off SR */
@@ -680,7 +683,7 @@ static void pineview_update_wm(struct drm_crtc
*unused_crtc)
pixel_size,
latency-display_hpll_disable);
reg = I915_READ(DSPFW3);
reg = ~DSPFW_HPLL_SR_MASK;
- reg |= wm DSPFW_HPLL_SR_MASK;
+ reg |= FW_WM(wm, HPLL_SR);
I915_WRITE(DSPFW3, reg);
/* cursor HPLL off SR */
@@ -689,7 +692,7 @@ static void pineview_update_wm(struct drm_crtc
*unused_crtc)
pixel_size,
latency-cursor_hpll_disable);
reg = I915_READ(DSPFW3);
reg = ~DSPFW_HPLL_CURSOR_MASK;
- reg |= (wm 0x3f) DSPFW_HPLL_CURSOR_SHIFT;
+ reg |= FW_WM(wm, HPLL_CURSOR);
I915_WRITE(DSPFW3, reg);
DRM_DEBUG_KMS(DSPFW3 register is %x\n, reg);
@@ -835,8 +838,6 @@ static bool g4x_compute_srwm(struct drm_device *dev,
display, cursor);
}
-#define FW_WM(value, plane) \
- (((value) DSPFW_ ## plane ## _SHIFT) DSPFW_ ## plane ## _MASK)
#define FW_WM_VLV(value, plane) \
(((value) DSPFW_ ## plane ## _SHIFT) DSPFW_ ## plane ## _MASK_VLV)
@@ -904,7 +905,6 @@ static void vlv_write_wm_values(struct intel_crtc *crtc,
dev_priv-wm.vlv = *wm;
}
-#undef FW_WM
#undef FW_WM_VLV
static uint8_t vlv_compute_drain_latency(struct drm_crtc *crtc,
@@ -1163,17 +1163,17 @@ static void g4x_update_wm(struct drm_crtc *crtc)
plane_sr, cursor_sr);
I915_WRITE(DSPFW1,
- (plane_sr DSPFW_SR_SHIFT) |
- (cursorb_wm DSPFW_CURSORB_SHIFT) |
- (planeb_wm DSPFW_PLANEB_SHIFT) |
- (planea_wm DSPFW_PLANEA_SHIFT));
+ FW_WM(plane_sr, SR) |
+ FW_WM(cursorb_wm, CURSORB) |
+ FW_WM(planeb_wm, PLANEB) |
+ FW_WM(planea_wm, PLANEA));
I915_WRITE(DSPFW2,
(I915_READ(DSPFW2) ~DSPFW_CURSORA_MASK) |
- (cursora_wm DSPFW_CURSORA_SHIFT));
+ FW_WM(cursora_wm, CURSORA));
/* HPLL off in SR has
Re: [Intel-gfx] [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept
On Wed, Mar 11, 2015 at 01:35:43PM +0200, Ander Conselvan de Oliveira wrote:
Remove the global modeset resource function that would disable the
bifurcation bit, and instead enable/disable it when enabling the pch
transcoder. The mode set consistency check should prevent us from
disabling the bit if pipe C is enabled so the change should be safe.
Note that this doens't affect the logic that prevents the bit being
set while a pipe is active, since the patch retains the behavior of
only chaging the bit if necessary. Because of the checks during mode
set, the first change would necessarily happen with both pipes B and
C disabled, and any subsequent write would be skipped.
v2: Only change the bit during pch trancoder enable. (Ville)
---
drivers/gpu/drm/i915/intel_display.c | 46
1 file changed, 10 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 4008bf4..bfbd829 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc
*crtc)
return crtc-base.state-enable crtc-config-has_pch_encoder;
}
-static void ivb_modeset_global_resources(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev-dev_private;
- struct intel_crtc *pipe_B_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_B]);
- struct intel_crtc *pipe_C_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_C]);
- uint32_t temp;
-
- /*
- * When everything is off disable fdi C so that we could enable fdi B
- * with all lanes. Note that we don't care about enabled pipes without
- * an enabled pch encoder.
- */
- if (!pipe_has_enabled_pch(pipe_B_crtc)
- !pipe_has_enabled_pch(pipe_C_crtc)) {
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
-
- temp = I915_READ(SOUTH_CHICKEN1);
- temp = ~FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(disabling fdi C rx\n);
- I915_WRITE(SOUTH_CHICKEN1, temp);
- }
-}
-
/* The FDI link training functions for ILK/Ibexpeak. */
static void ironlake_fdi_link_train(struct drm_crtc *crtc)
{
@@ -3834,20 +3808,23 @@ static void
ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
I915_READ(VSYNCSHIFT(cpu_transcoder)));
}
-static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
+static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
{
struct drm_i915_private *dev_priv = dev-dev_private;
uint32_t temp;
temp = I915_READ(SOUTH_CHICKEN1);
- if (temp FDI_BC_BIFURCATION_SELECT)
+ if (!!(temp FDI_BC_BIFURCATION_SELECT) == enable)
return;
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
- temp |= FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(enabling fdi C rx\n);
+ temp = ~FDI_BC_BIFURCATION_SELECT;
+ if (enable)
+ temp |= FDI_BC_BIFURCATION_SELECT;
+
+ DRM_DEBUG_KMS(%sabling fdi C rx\n, enable ? en : dis);
I915_WRITE(SOUTH_CHICKEN1, temp);
POSTING_READ(SOUTH_CHICKEN1);
}
@@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct
drm_device *dev)
static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc
*intel_crtc)
{
struct drm_device *dev = intel_crtc-base.dev;
- struct drm_i915_private *dev_priv = dev-dev_private;
switch (intel_crtc-pipe) {
case PIPE_A:
break;
case PIPE_B:
if (intel_crtc-config-fdi_lanes 2)
- WARN_ON(I915_READ(SOUTH_CHICKEN1)
FDI_BC_BIFURCATION_SELECT);
+ cpt_set_fdi_bc_bifurcation(dev, false);
So we just replace the globla_resources enforced assumed state with an
explicit state change here. Should be perfectly fine since pipe is not
active at this point.
What really confuses me about the whole FDI bifurcation code is
ironlake_check_fdi_lanes(). First of all I would rewrite it a bit like
so:
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5615,14 +5615,13 @@ static bool ironlake_check_fdi_lanes(struct drm_device
*dev, enum pipe pipe,
}
return true;
case PIPE_C:
- if (!pipe_has_enabled_pch(pipe_B_crtc) ||
- pipe_B_crtc-config-fdi_lanes = 2) {
- if (pipe_config-fdi_lanes 2) {
- DRM_DEBUG_KMS(invalid shared fdi lane config
on pipe %c: %i lanes\n,
- pipe_name(pipe),
pipe_config-fdi_lanes);
-
Re: [Intel-gfx] [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept
On Wed, 2015-03-11 at 15:10 +0200, Ville Syrjälä wrote:
On Wed, Mar 11, 2015 at 01:35:43PM +0200, Ander Conselvan de Oliveira wrote:
Remove the global modeset resource function that would disable the
bifurcation bit, and instead enable/disable it when enabling the pch
transcoder. The mode set consistency check should prevent us from
disabling the bit if pipe C is enabled so the change should be safe.
Note that this doens't affect the logic that prevents the bit being
set while a pipe is active, since the patch retains the behavior of
only chaging the bit if necessary. Because of the checks during mode
set, the first change would necessarily happen with both pipes B and
C disabled, and any subsequent write would be skipped.
v2: Only change the bit during pch trancoder enable. (Ville)
---
drivers/gpu/drm/i915/intel_display.c | 46
1 file changed, 10 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 4008bf4..bfbd829 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc
*crtc)
return crtc-base.state-enable crtc-config-has_pch_encoder;
}
-static void ivb_modeset_global_resources(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev-dev_private;
- struct intel_crtc *pipe_B_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_B]);
- struct intel_crtc *pipe_C_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_C]);
- uint32_t temp;
-
- /*
-* When everything is off disable fdi C so that we could enable fdi B
-* with all lanes. Note that we don't care about enabled pipes without
-* an enabled pch encoder.
-*/
- if (!pipe_has_enabled_pch(pipe_B_crtc)
- !pipe_has_enabled_pch(pipe_C_crtc)) {
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
-
- temp = I915_READ(SOUTH_CHICKEN1);
- temp = ~FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(disabling fdi C rx\n);
- I915_WRITE(SOUTH_CHICKEN1, temp);
- }
-}
-
/* The FDI link training functions for ILK/Ibexpeak. */
static void ironlake_fdi_link_train(struct drm_crtc *crtc)
{
@@ -3834,20 +3808,23 @@ static void
ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
I915_READ(VSYNCSHIFT(cpu_transcoder)));
}
-static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
+static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
{
struct drm_i915_private *dev_priv = dev-dev_private;
uint32_t temp;
temp = I915_READ(SOUTH_CHICKEN1);
- if (temp FDI_BC_BIFURCATION_SELECT)
+ if (!!(temp FDI_BC_BIFURCATION_SELECT) == enable)
return;
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
- temp |= FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(enabling fdi C rx\n);
+ temp = ~FDI_BC_BIFURCATION_SELECT;
+ if (enable)
+ temp |= FDI_BC_BIFURCATION_SELECT;
+
+ DRM_DEBUG_KMS(%sabling fdi C rx\n, enable ? en : dis);
I915_WRITE(SOUTH_CHICKEN1, temp);
POSTING_READ(SOUTH_CHICKEN1);
}
@@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct
drm_device *dev)
static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc
*intel_crtc)
{
struct drm_device *dev = intel_crtc-base.dev;
- struct drm_i915_private *dev_priv = dev-dev_private;
switch (intel_crtc-pipe) {
case PIPE_A:
break;
case PIPE_B:
if (intel_crtc-config-fdi_lanes 2)
- WARN_ON(I915_READ(SOUTH_CHICKEN1)
FDI_BC_BIFURCATION_SELECT);
+ cpt_set_fdi_bc_bifurcation(dev, false);
So we just replace the globla_resources enforced assumed state with an
explicit state change here. Should be perfectly fine since pipe is not
active at this point.
What really confuses me about the whole FDI bifurcation code is
ironlake_check_fdi_lanes(). First of all I would rewrite it a bit like
so:
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5615,14 +5615,13 @@ static bool ironlake_check_fdi_lanes(struct
drm_device *dev, enum pipe pipe,
}
return true;
case PIPE_C:
- if (!pipe_has_enabled_pch(pipe_B_crtc) ||
- pipe_B_crtc-config-fdi_lanes = 2) {
- if (pipe_config-fdi_lanes 2) {
- DRM_DEBUG_KMS(invalid shared fdi lane config
on pipe %c: %i lanes\n,
-
Re: [Intel-gfx] [PATCH igt 1/2] lib/kms: Add a way to override an output's mode
On Wed, Mar 11, 2015 at 01:33:00PM +0200, Ander Conselvan de Oliveira wrote:
So that it is possible to use a custom mode with the simplified mode set API.
Maybe just igt_output_set_mode()?
--
Damien
---
lib/igt_kms.c | 9 +
lib/igt_kms.h | 4
2 files changed, 13 insertions(+)
diff --git a/lib/igt_kms.c b/lib/igt_kms.c
index 26e4913..0dccd2d 100644
--- a/lib/igt_kms.c
+++ b/lib/igt_kms.c
@@ -895,6 +895,9 @@ static void igt_output_refresh(igt_output_t *output)
if (!output-valid)
return;
+ if (output-use_override_mode)
+ output-config.default_mode = output-override_mode;
+
if (!output-name) {
drmModeConnector *c = output-config.connector;
@@ -1797,6 +1800,12 @@ drmModeModeInfo *igt_output_get_mode(igt_output_t
*output)
return output-config.default_mode;
}
+void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode)
+{
+ output-override_mode = *mode;
+ output-use_override_mode = true;
+}
+
void igt_output_set_pipe(igt_output_t *output, enum pipe pipe)
{
igt_display_t *display = output-display;
diff --git a/lib/igt_kms.h b/lib/igt_kms.h
index 2fab30e..ddf4432 100644
--- a/lib/igt_kms.h
+++ b/lib/igt_kms.h
@@ -228,6 +228,9 @@ typedef struct {
bool valid;
unsigned long pending_crtc_idx_mask;
+ bool use_override_mode;
+ drmModeModeInfo override_mode;
+
#ifdef HAVE_ATOMIC
/* Property set for nuclear pageflip */
drmModePropertySetPtr set;
@@ -255,6 +258,7 @@ int igt_display_get_n_pipes(igt_display_t *display);
const char *igt_output_name(igt_output_t *output);
drmModeModeInfo *igt_output_get_mode(igt_output_t *output);
+void igt_output_override_mode(igt_output_t *output, drmModeModeInfo *mode);
void igt_output_set_pipe(igt_output_t *output, enum pipe pipe);
igt_plane_t *igt_output_get_plane(igt_output_t *output, enum igt_plane
plane);
--
2.1.0
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 4/9] drm/i915: Make derived plane state correct after crtc_enable
On Wed, Mar 11, 2015 at 11:24:34AM +0100, Daniel Vetter wrote:
On Wed, Mar 11, 2015 at 12:05:39PM +0200, Ville Syrjälä wrote:
On Wed, Mar 11, 2015 at 10:52:29AM +0100, Daniel Vetter wrote:
On Tue, Mar 10, 2015 at 07:57:13PM +0200, Ville Syrjälä wrote:
On Tue, Mar 10, 2015 at 10:01:51AM -0700, Matt Roper wrote:
On Tue, Mar 10, 2015 at 01:15:24PM +0200,
ville.syrj...@linux.intel.com wrote:
From: Ville Syrjälä ville.syrj...@linux.intel.com
-static void disable_plane_internal(struct drm_plane *plane)
+static void _intel_crtc_enable_planes(struct intel_crtc *crtc)
{
- struct intel_plane *intel_plane = to_intel_plane(plane);
- struct drm_plane_state *state =
- plane-funcs-atomic_duplicate_state(plane);
- struct intel_plane_state *intel_state =
to_intel_plane_state(state);
+ struct drm_device *dev = crtc-base.dev;
+ enum pipe pipe = crtc-pipe;
+ struct intel_plane *plane;
+ const struct drm_crtc_helper_funcs *crtc_funcs =
+ crtc-base.helper_private;
- intel_state-visible = false;
- intel_plane-commit_plane(plane, intel_state);
+ for_each_intel_plane(dev, plane) {
+ const struct drm_plane_helper_funcs *funcs =
+ plane-base.helper_private;
+ struct intel_plane_state *state =
+ to_intel_plane_state(plane-base.state);
- intel_plane_destroy_state(plane, state);
+ if (plane-pipe != pipe)
+ continue;
+
+ if (funcs-atomic_check(plane-base, state-base))
Maybe add a WARN_ON() here? I'm assuming that this shouldn't really
be
possible since if this fails it means we've already previously done a
commit of invalid state on a previous atomic transaction. But if it
does somehow happen, the WARN will give us a clue why the plane
contents
simply didn't show up.
I can think of one way to make it fail. That is, first set a smaller
mode with the primary plane (and fb) configured to cover that fully, and
then switch to a larger mode without reconfiguring the primary plane. If
the hardware requires the primary plane to be fullscreen it'll fail. But
that should actaully not be possible using the legacy modeset API as it
always reconfigures the primary, so we'd only have to worry about that
with full atomic modeset, and for that we anyway need to change the code
to do the check stuff up front.
So yeah, with the way things are this should not be able to fail. I'll
respin with the WARN.
I haven't fully dug into the details here, but a few randome comments:
- While transitioning we're calling the transitional plane helpers, which
should call the atomic_check stuff for us on the primary plane. If we
need to call atomic_check on other planes too (why?)
Because we want the derived state to be updated to match the (potentially
changed) crtc config. We do call the .update_plane() hook from the
modeset path, but that happens at a time when the pipe is off, so our
clipping calculations end up saying the plane is invisible. I think fixing
that the right way pretty much involves the atomic conversion of the
modeset path.
Why do we conclude it's invisible?
Because crtc-active. So for this we'll be wanting crtc_state-active
or somesuch which tells us upfront whether the pipe is going to be
active or not.
But that's also beside the point a bit since we still want to make call
the .atomic_check() for all planes. Right now we'd call it for primary
(at the wrong point wrt. crtc-active) and we call it for sprites later
when crtc-active is showing the right state, but we don't call it at
all for cursors. That's why we still have that ad-hoc extra cursor
clipping code in intel_update_cursor(). If we could make the derived
plane state correct, we could throw that stuff out as well and trust the
regular plane clipping calculations to tell us when the cursor has gone
offscreen.
It'll also make the plane state behave in a consitent manner wrt.
crtc-active. As it stands you simply can't trust the plane state for
primary/cursor planes.
So to fix all of that, I just went ahead and called it for all planes at
the point where we currently call it for sprites. I could have just gone
ahead and called the higher level .update_plane() func for all of them
(as we did for sprites already) but going one level lower allowed me to
get the planes to pop in/out atomically.
I think it's the best way to move forward with getting the plane and wm
code into some kind of sane state.
If we can fix the state to not depend
upon the dpms state then things should work ...
That's a more drastic change and needs way more thought.
then I think that
should be done as close as possible to where we do that for the
Re: [Intel-gfx] [PATCH] drm/i915: Simplify the way BC bifurcation state consistency is kept
On Wed, Mar 11, 2015 at 01:35:43PM +0200, Ander Conselvan de Oliveira wrote:
Remove the global modeset resource function that would disable the
bifurcation bit, and instead enable/disable it when enabling the pch
transcoder. The mode set consistency check should prevent us from
disabling the bit if pipe C is enabled so the change should be safe.
Note that this doens't affect the logic that prevents the bit being
set while a pipe is active, since the patch retains the behavior of
only chaging the bit if necessary. Because of the checks during mode
set, the first change would necessarily happen with both pipes B and
C disabled, and any subsequent write would be skipped.
v2: Only change the bit during pch trancoder enable. (Ville)
Actually what I had inb mind was something like this:
pch_enable()
{
if (pipe == B fdi_lanes 2)
disable_bifurcation()
...
}
pch_disable()
{
...
if (pipe == B fdi_lanes 2)
enable_bifurcation()
}
So we only change it when enabling/disabling pipe B, never for pipe C.
Hence it remains enabled whenever pipe B doesn't need 2 FDI lanes.
---
drivers/gpu/drm/i915/intel_display.c | 46
1 file changed, 10 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_display.c
b/drivers/gpu/drm/i915/intel_display.c
index 4008bf4..bfbd829 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3153,32 +3153,6 @@ static bool pipe_has_enabled_pch(struct intel_crtc
*crtc)
return crtc-base.state-enable crtc-config-has_pch_encoder;
}
-static void ivb_modeset_global_resources(struct drm_device *dev)
-{
- struct drm_i915_private *dev_priv = dev-dev_private;
- struct intel_crtc *pipe_B_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_B]);
- struct intel_crtc *pipe_C_crtc =
- to_intel_crtc(dev_priv-pipe_to_crtc_mapping[PIPE_C]);
- uint32_t temp;
-
- /*
- * When everything is off disable fdi C so that we could enable fdi B
- * with all lanes. Note that we don't care about enabled pipes without
- * an enabled pch encoder.
- */
- if (!pipe_has_enabled_pch(pipe_B_crtc)
- !pipe_has_enabled_pch(pipe_C_crtc)) {
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
- WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
-
- temp = I915_READ(SOUTH_CHICKEN1);
- temp = ~FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(disabling fdi C rx\n);
- I915_WRITE(SOUTH_CHICKEN1, temp);
- }
-}
-
/* The FDI link training functions for ILK/Ibexpeak. */
static void ironlake_fdi_link_train(struct drm_crtc *crtc)
{
@@ -3834,20 +3808,23 @@ static void
ironlake_pch_transcoder_set_timings(struct intel_crtc *crtc,
I915_READ(VSYNCSHIFT(cpu_transcoder)));
}
-static void cpt_enable_fdi_bc_bifurcation(struct drm_device *dev)
+static void cpt_set_fdi_bc_bifurcation(struct drm_device *dev, bool enable)
{
struct drm_i915_private *dev_priv = dev-dev_private;
uint32_t temp;
temp = I915_READ(SOUTH_CHICKEN1);
- if (temp FDI_BC_BIFURCATION_SELECT)
+ if (!!(temp FDI_BC_BIFURCATION_SELECT) == enable)
return;
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_B)) FDI_RX_ENABLE);
WARN_ON(I915_READ(FDI_RX_CTL(PIPE_C)) FDI_RX_ENABLE);
- temp |= FDI_BC_BIFURCATION_SELECT;
- DRM_DEBUG_KMS(enabling fdi C rx\n);
+ temp = ~FDI_BC_BIFURCATION_SELECT;
+ if (enable)
+ temp |= FDI_BC_BIFURCATION_SELECT;
+
+ DRM_DEBUG_KMS(%sabling fdi C rx\n, enable ? en : dis);
I915_WRITE(SOUTH_CHICKEN1, temp);
POSTING_READ(SOUTH_CHICKEN1);
}
@@ -3855,20 +3832,19 @@ static void cpt_enable_fdi_bc_bifurcation(struct
drm_device *dev)
static void ivybridge_update_fdi_bc_bifurcation(struct intel_crtc
*intel_crtc)
{
struct drm_device *dev = intel_crtc-base.dev;
- struct drm_i915_private *dev_priv = dev-dev_private;
switch (intel_crtc-pipe) {
case PIPE_A:
break;
case PIPE_B:
if (intel_crtc-config-fdi_lanes 2)
- WARN_ON(I915_READ(SOUTH_CHICKEN1)
FDI_BC_BIFURCATION_SELECT);
+ cpt_set_fdi_bc_bifurcation(dev, false);
else
- cpt_enable_fdi_bc_bifurcation(dev);
+ cpt_set_fdi_bc_bifurcation(dev, true);
break;
case PIPE_C:
- cpt_enable_fdi_bc_bifurcation(dev);
+ cpt_set_fdi_bc_bifurcation(dev, true);
break;
default:
@@ -13056,8 +13032,6 @@ static void intel_init_display(struct drm_device *dev)
} else if (IS_IVYBRIDGE(dev)) {
/* FIXME: detect B0+ stepping and use auto training */
Re: [Intel-gfx] [PATCH v2] drm/i915: Optimistically spin for the request completion
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 5934 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -9 281/281 272/281 ILK -1 308/308 307/308 SNB -1 284/284 283/284 IVB 375/375 375/375 BYT 294/294 294/294 HSW 384/384 384/384 BDW 315/315 315/315 -Detailed- Platform Testdrm-intel-nightly Series Applied *PNV igt_gem_fence_thrash_bo-write-verify-none PASS(2) FAIL(1)NRUN(1) *PNV igt_gem_fence_thrash_bo-write-verify-x PASS(2) FAIL(1)NO_RESULT(1) *PNV igt_gem_fence_thrash_bo-write-verify-y PASS(2) FAIL(1)NO_RESULT(1) PNV igt_gem_userptr_blits_coherency-sync CRASH(4)PASS(2) CRASH(1)PASS(1) PNV igt_gen3_render_linear_blits FAIL(2)PASS(1) FAIL(1)PASS(1) *PNV igt_gen3_render_mixed_blits FAIL(1)PASS(2) FAIL(1)NO_RESULT(1) PNV igt_gen3_render_tiledx_blits FAIL(3)PASS(1) FAIL(1)PASS(1) PNV igt_gen3_render_tiledy_blits FAIL(4)PASS(1) FAIL(1)PASS(1) PNV igt_gem_fence_thrash_bo-write-verify-threaded-none CRASH(1)PASS(4) CRASH(1)PASS(1) *ILK igt_kms_flip_wf_vblank-ts-check PASS(2) DMESG_WARN(1)PASS(1) *SNB igt_gem_fenced_exec_thrash_too-many-fences PASS(4) DMESG_WARN(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Read CHV_PLL_DW8 from the correct offset
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 5936 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -15 281/281 266/281 ILK -1 308/308 307/308 SNB -1 284/284 283/284 IVB 375/375 375/375 BYT 294/294 294/294 HSW -1 384/384 383/384 BDW 315/315 315/315 -Detailed- Platform Testdrm-intel-nightly Series Applied *PNV igt_gem_fence_thrash_bo-write-verify-none PASS(3) FAIL(1) *PNV igt_gem_fence_thrash_bo-write-verify-x PASS(3) FAIL(1) *PNV igt_gem_fence_thrash_bo-write-verify-y PASS(3) FAIL(1) PNV igt_gem_userptr_blits_coherency-sync CRASH(5)PASS(2) CRASH(1) PNV igt_gen3_render_linear_blits FAIL(3)PASS(1) FAIL(1) PNV igt_gen3_render_mixed_blits FAIL(2)PASS(2) FAIL(1) PNV igt_gen3_render_tiledx_blits FAIL(4)PASS(1) FAIL(1) PNV igt_gem_fence_thrash_bo-write-verify-threaded-none CRASH(2)PASS(4) CRASH(1) *PNV igt_gem_partial_pwrite_pread_reads PASS(2) NRUN(1) *PNV igt_gem_partial_pwrite_pread_reads-display PASS(2) NRUN(1) *PNV igt_gem_pwrite_pread_display-pwrite-blt-gtt_mmap-performance PASS(2) NRUN(1) *PNV igt_gem_ringfill_blitter PASS(2) NRUN(1) *PNV igt_gem_ringfill_blitter-interruptible PASS(2) NRUN(1) *PNV igt_gem_tiled_pread_pwrite FAIL(1)PASS(2) NRUN(1) *PNV igt_gem_userptr_blits_forked-sync-mempressure-interruptible PASS(2) NRUN(1) *ILK igt_gem_unfence_active_buffers PASS(3) DMESG_WARN(1)PASS(1) *SNB igt_gem_flink_bad-open PASS(2) DMESG_WARN(1)PASS(1) *HSW igt_pm_rpm_reg-read-ioctl PASS(2) DMESG_FAIL(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Read CHV_PLL_DW8 from the correct offset
On 3/11/2015 1:52 PM, ville.syrj...@linux.intel.com wrote: From: Ville Syrjälä ville.syrj...@linux.intel.com We accidentally pass 'pipe' instead of 'port' to CHV_PLL_DW8() and with PIPE_C we end up at register offset 0x8320 which isn't the 0x8020 we wanted. Fix it. The problem was fortunately caught by the sanity check in vlv_dpio_read(): WARNING: CPU: 1 PID: 238 at ../drivers/gpu/drm/i915/intel_sideband.c:200 vlv_dpio_read+0x77/0x80 [i915]() DPIO read pipe C reg 0x8320 == 0x The problem got introduced with this commit: commit 71af07f91f12bbab96335e202c82525d31680960 Author: Vijay Purushothaman vijay.a.purushotha...@linux.intel.com Date: Thu Mar 5 19:33:08 2015 +0530 drm/i915: Update prop, int co-eff and gain threshold for CHV Cc: Vijay Purushothaman vijay.a.purushotha...@linux.intel.com Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com --- drivers/gpu/drm/i915/intel_display.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index ecbad5a..198e5fc 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6270,7 +6270,7 @@ static void chv_prepare_pll(struct intel_crtc *crtc, } vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW6(port), loopfilter); - dpio_val = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW8(pipe)); + dpio_val = vlv_dpio_read(dev_priv, pipe, CHV_PLL_DW8(port)); dpio_val = ~DPIO_CHV_TDC_TARGET_CNT_MASK; dpio_val |= (tribuf_calcntr DPIO_CHV_TDC_TARGET_CNT_SHIFT); vlv_dpio_write(dev_priv, pipe, CHV_PLL_DW8(port), dpio_val); Looks good to me. Reviewed-by: Todd Previte tprev...@gmail.com ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/i915/chv: Set min freq to efficient frequency on chv
On Wednesday 11 March 2015 07:36 PM, Chris Wilson wrote: On Wed, Mar 11, 2015 at 07:23:48PM +0530, Deepak S wrote: On Thursday 26 February 2015 09:42 PM, Chris Wilson wrote: On Thu, Feb 26, 2015 at 08:46:56PM +0530, deepa...@linux.intel.com wrote: From: Deepak S deepa...@linux.intel.com After feedback from the hardware team, now we set the GPU min freq to RPe. If we drop the freq to RPn, we found that the punit was not setting the voltage to Vnn, So recommendation is to set min freq to RPe. And does it change the voltage at all? Yes Voltage does change when we drop to RPe Is there really any advantage to the extra code on idle?Does efficient_freq really consume less power than min_freq when active (assuming a min_freq/efficient_freq busy workload i.e. does a workload that would be 100% busy at min_freq consume less power when run at efficient_freq)? The delta voltage usage between RPn and RPe is very small like close to zero. Also, if we run workload 100% busy at Rpe we get better performance without much of voltage loss right? btw, Punit expects us to operate between Rpe RP0. If you need 100% at RPe you obviously can't run at RPn (since that would lead to dropped frames). The question is if you have a workload that requires 100% at RPn do you save power if you ran e.g. 80% at RPe? We do not expect much of power saving running at RPn. If we need exact number I need to gather the data. If the punit only works reliably between RPe and RP0, then the current RPn is a bit of a misnomer, and that should be the explanation in the commit log. Definitely do not conflate the idea of executing at RPn and RPe with the idea of idling at RPn or RPe - this patch affects idle frequency. -Chris Yes I understand it affects idle freq but running at RPe gives better performance at lower voltage and also punit drops voltage to help save power I will update the commit msg to explain why we need lower freq at Rpe. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 55/56] drm/i915: Remove 'faked' request from LRC submission
On 05/03/2015 14:49, Daniel Vetter wrote:
On Thu, Mar 05, 2015 at 01:57:31PM +, john.c.harri...@intel.com wrote:
From: John Harrison john.c.harri...@intel.com
The LRC submission code requires a request for tracking purposes. It does not
actually require that request to 'complete' it simply uses it for keeping hold
of reference counts on contexts and such like.
In the case where the ring buffer is completely full, the LRC code looks for a
pending request that would free up sufficient space upon completion and waits
for it. If no such request can be found it resorts to simply polling the free
space count until it is big enough. This situation should only occur when the
entire buffer is filled with the request currently being generated. I.e., the
user is trying to submit a single piece of work that is large than the ring
buffer itself (which should be impossible because very large batch buffers don't
consume any more ring buffer space). Before starting to poll, a submit call is
made to make sure that the currently queued up work in the buffer will actually
be submtted and thus the poll will eventually succeed.
The problem here is that the 'official' request cannot be used as that could
lead to multiple LRC submissions being tagged to a single request structure.
Instead, the code fakes up a private request structure and uses that.
This patch moves the faked request allocation higher up in the call stack to the
wait code itself (rather than being at the very lowest submission level). Thus
it is now obvious where the faked request is coming from and why it is
necessary. The patch also replaces it with a call to the official request
allocation code rather than attempting to duplicate that code. This becomes
especially important in the future when the request allocation changes to
accommodate a conversion to struct fence.
For: VIZ-5115
Signed-off-by: John Harrison john.c.harri...@intel.com
This is only possible if you pile up tons of olr. Since your patch series
fixes this design issue by removing olr I think we can just put a WARN_ON
in here if this ever happens and bail out with -ELOSTMYMARBLES or
something. And then rip out all this complexity.
Or do I miss something important?
-Daniel
Yeah, you missed the extremely important bug in the free space
calculation that meant this impossible code path was being hit on a
regular basis. The LRC wait_request code differed from the legacy
wait_request code in the the latter was updated with request-postfix
changes and the former was not. Thus the LRC one would happily find a
request that frees up enough space, wait on it, retire it and then find
there was still not enough space!
New patches to fix the space calculation bug and to completely remove
the polling path will be forth coming...
---
drivers/gpu/drm/i915/intel_lrc.c | 45 ++
1 file changed, 26 insertions(+), 19 deletions(-)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 65eea51..1fa36de 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -507,23 +507,11 @@ static int execlists_context_queue(struct intel_engine_cs
*ring,
if (to != ring-default_context)
intel_lr_context_pin(ring, to);
- if (!request) {
- /*
-* If there isn't a request associated with this submission,
-* create one as a temporary holder.
-*/
- request = kzalloc(sizeof(*request), GFP_KERNEL);
- if (request == NULL)
- return -ENOMEM;
- request-ring = ring;
- request-ctx = to;
- kref_init(request-ref);
- request-uniq = dev_priv-request_uniq++;
- i915_gem_context_reference(request-ctx);
- } else {
- i915_gem_request_reference(request);
- WARN_ON(to != request-ctx);
- }
+ WARN_ON(!request);
+ WARN_ON(to != request-ctx);
+
+ i915_gem_request_reference(request);
+
request-tail = tail;
intel_runtime_pm_get(dev_priv);
@@ -677,6 +665,7 @@ static int logical_ring_wait_for_space(struct
intel_ringbuffer *ringbuf,
struct intel_engine_cs *ring = ringbuf-ring;
struct drm_device *dev = ring-dev;
struct drm_i915_private *dev_priv = dev-dev_private;
+ struct drm_i915_gem_request *local_req;
unsigned long end;
int ret;
@@ -684,8 +673,23 @@ static int logical_ring_wait_for_space(struct intel_ringbuffer *ringbuf,
if (ret != -ENOSPC)
return ret;
- /* Force the context submission in case we have been skipping it */
- intel_logical_ring_advance_and_submit(ringbuf, ctx, NULL);
+ /*
+* Force the context submission in case we have been skipping it.
+* This requires creating a place holder request so that the LRC
+* submission can be
Re: [Intel-gfx] [PATCH 1/5] drm/i915/chv: Remove Wait for a previous gfx force-off
On Thursday 26 February 2015 09:12 PM, Deepak S wrote:
On Thursday 26 February 2015 09:13 PM, Ville Syrjälä wrote:
On Thu, Feb 26, 2015 at 08:46:54PM +0530, deepa...@linux.intel.com
wrote:
From: Deepak S deepa...@linux.intel.com
On CHV, PUNIT team confirmed that 'VLV_GFX_CLK_STATUS_BIT' is not a
sticky bit and it will always be set. So ignore Check for previous
Gfx force off during suspend and allow the force clk as part S0ix
Sequence
Do we need the force clock at all since we don't do any gunit register
save/restore? I tried to peddle a patch to remove it totally in this bug
report, but never got any decent answer back:
https://bugs.freedesktop.org/show_bug.cgi?id=87086
hmm. your right we might not need. Let me confirm
Hi Ville,
Based on the spec we still need to follow the Gfx force clk sequence :(
Can you review the patch to skip wait for previous Gfx force-off?
This patch has gone through lot of S0ix testing.
Thanks
Deepak
Signed-off-by: Deepak S deepa...@linux.intel.com
---
drivers/gpu/drm/i915/i915_drv.c | 6 --
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_drv.c
b/drivers/gpu/drm/i915/i915_drv.c
index 4badb23..b88b7b1 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1193,11 +1193,13 @@ int vlv_force_gfx_clock(struct
drm_i915_private *dev_priv, bool force_on)
int err;
val = I915_READ(VLV_GTLC_SURVIVABILITY_REG);
-WARN_ON(!!(val VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
#define COND (I915_READ(VLV_GTLC_SURVIVABILITY_REG)
VLV_GFX_CLK_STATUS_BIT)
/* Wait for a previous force-off to settle */
-if (force_on) {
+if (force_on !IS_CHERRYVIEW(dev_priv-dev)) {
+/* WARN_ON only for the Valleyview */
+WARN_ON(!!(val VLV_GFX_CLK_FORCE_ON_BIT) == force_on);
+
err = wait_for(!COND, 20);
if (err) {
DRM_ERROR(timeout waiting for GFX clock force-off
(%08x)\n,
--
1.9.1
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] commit 440fd52 drm/mm: Support 4GiB and larger ranges oops on 32bit kernel
[cc'ing lists]
On Tue, Mar 10, 2015 at 07:17:22PM +0100, Krzysztof Kolasa wrote:
System ( 32bit, Intel 945GM ) hangs after some short time, oops:
[ cut here ]
kernel BUG at drivers/gpu/drm/drm_mm.c:305!
invalid opcode: [#1] SMP
Modules linked in: arc4 md4 cfg80211 8192cu(O) pci_stub vboxpci(O)
vboxnetadp(O) vboxnetflt(O) vboxdrv(O) snd_hda_codec_si3054
snd_hda_codec_realtek snd_hda_codec_generic snd_hda_intel
snd_hda_controller snd_hda_codec snd_hwdep snd_pcm microcode
snd_seq_midi snd_seq_midi_event snd_rawmidi joydev serio_raw snd_seq
snd_timer snd_seq_device rfcomm bnep bluetooth i915 snd lpc_ich
drm_kms_helper soundcore drm i2c_algo_bit parport_pc ppdev video
mac_hid coretemp lp parport nls_utf8 cifs usb_storage psmouse
8139too 8139cp mii
CPU: 0 PID: 1165 Comm: Xorg Tainted: G O
4.0.0-rc3-winsoft-x86+ #11
Hardware name: FUJITSU SIEMENS AMILO Pro Edition V3405/AMILO
Pro Edition V3405, BIOS R01-B0E03/20/2007
task: f64e0db0 ti: f4566000 task.ti: f4566000
EIP: 0060:[f88c7d62] EFLAGS: 00213206 CPU: 0
EIP is at drm_mm_insert_node_in_range_generic+0x432/0x4d0 [drm]
EAX: 0100 EBX: 00c78000 ECX: f6d4c908 EDX:
ESI: f6d4c900 EDI: f6d4c900 EBP: f4567c78 ESP: f4567bf8
DS: 007b ES: 007b FS: 00d8 GS: 0033 SS: 0068
CR0: 8005003b CR2: b4a02000 CR3: 345f2000 CR4: 07f0
Stack:
f4567c64 0080 0080 f6d4c900 f4882700
00a88000 1000 00478000
00f0 f472c33c
Call Trace:
[f8b2634e] i915_gem_object_pin_view+0x3ce/0x840 [i915]
[f8b1f601] ? i915_gem_obj_lookup_or_create_vma_view+0x41/0x160 [i915]
[f8b196d2] i915_gem_execbuffer_reserve_vma.isra.11+0x62/0x100 [i915]
[f8b19a01] i915_gem_execbuffer_reserve+0x291/0x2e0 [i915]
[f8b1a340] i915_gem_do_execbuffer.isra.16+0x4f0/0xd10 [i915]
[c1189430] ? poll_select_copy_remaining+0x100/0x100
[c116a8fd] ? __kmalloc+0x4d/0x190
[f8b1bbdb] i915_gem_execbuffer2+0x8b/0x2c0 [i915]
[f8b1bb50] ? i915_gem_execbuffer+0x4e0/0x4e0 [i915]
[f88bffa7] drm_ioctl+0x1b7/0x510 [drm]
[f8b1bb50] ? i915_gem_execbuffer+0x4e0/0x4e0 [i915]
[c10aac52] ? ktime_get_ts64+0x52/0x1a0
[f88bfdf0] ? drm_getmap+0xb0/0xb0 [drm]
[c11888ea] do_vfs_ioctl+0x30a/0x530
[c1305626] ? _copy_to_user+0x26/0x30
[c10aac52] ? ktime_get_ts64+0x52/0x1a0
[c1190ee2] ? __fget_light+0x22/0x60
[c1188b70] SyS_ioctl+0x60/0x90
[c1630ec8] sysenter_do_call+0x12/0x12
Code: ff ff 3b 55 e8 8d 74 26 00 77 10 73 04 0f 0b 66 90 3b 45 e4 90
8d 74 26 00 72 f2 8b 75 94 03 46 20 13 56 24 3b 55 f0 72 0d 76 06
0f 0b 8d 74 26 00 3b 45 ec 77 f5 39 55 c4 77 10 73 04 0f 0b 66
EIP: [f88c7d62] drm_mm_insert_node_in_range_generic+0x432/0x4d0
[drm] SS:ESP 0068:f4567bf8
---[ end trace 4648f53699b9eb32 ]---
after revert commit 440fd52 system working OK
Krzysztof
--
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Modifying RC6 Promotion timer for Media workloads.
On Wed, Mar 11, 2015 at 07:07:12PM +0530, Deepak S wrote: On Friday 06 March 2015 10:10 PM, Daniel Vetter wrote: On Thu, Mar 05, 2015 at 09:27:59PM +0530, deepa...@linux.intel.com wrote: From: Deepak S deepa...@linux.intel.com In normal cases, RC6 promotion timer is 1700us/500us. This will result in more time spent in C1 state. For more residency in C6 in case of media workloads, this is changed to 250us. Not doing this for 3D workloads as too many C6-C0 transition delays can result in performance impact. v2: Extend GPU busy idle detection framework for rc6 Promotion timer changes (Chris) Signed-off-by: Deepak S deepa...@linux.intel.com I've thougth Chris' idea was to put this into the gen6_rps_boost/idle functions? You could check from within them I think for whether the vcs is still busy ... One more comment below. -Daniel Hi Daniel, gen6_rps_boost/idle will be called only for RCS right? Also we get gen6_rps_boost during __wait_request But we want to program promotion timer when we add request to VCS to apply the value immediately. It's gen6_rps_busy/gen6_rps_idle. They are called from intel_mark_busy and intel_mark_idle. It is intel_mark_busy/intel_mark_idle that we want to extend to cover the VCS case as well. I think if you add a ring parameter to the functions, we can start specialising per ring and global state changes. You will then also be in a position to judge what is the best idle timer (and consider making i915_gem_idle_work_handler per ring). The goal is simply to evolve the current infrastucture for idle/busyness handling to cover your use case as well (and hopefully in the process improving the old/general cases). -Chris -- Chris Wilson, Intel Open Source Technology Centre ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH v2] drm/i915: Modifying RC6 Promotion timer for Media workloads.
On Wednesday 11 March 2015 07:26 PM, Chris Wilson wrote: On Wed, Mar 11, 2015 at 07:07:12PM +0530, Deepak S wrote: On Friday 06 March 2015 10:10 PM, Daniel Vetter wrote: On Thu, Mar 05, 2015 at 09:27:59PM +0530, deepa...@linux.intel.com wrote: From: Deepak S deepa...@linux.intel.com In normal cases, RC6 promotion timer is 1700us/500us. This will result in more time spent in C1 state. For more residency in C6 in case of media workloads, this is changed to 250us. Not doing this for 3D workloads as too many C6-C0 transition delays can result in performance impact. v2: Extend GPU busy idle detection framework for rc6 Promotion timer changes (Chris) Signed-off-by: Deepak S deepa...@linux.intel.com I've thougth Chris' idea was to put this into the gen6_rps_boost/idle functions? You could check from within them I think for whether the vcs is still busy ... One more comment below. -Daniel Hi Daniel, gen6_rps_boost/idle will be called only for RCS right? Also we get gen6_rps_boost during __wait_request But we want to program promotion timer when we add request to VCS to apply the value immediately. It's gen6_rps_busy/gen6_rps_idle. They are called from intel_mark_busy and intel_mark_idle. It is intel_mark_busy/intel_mark_idle that we want to extend to cover the VCS case as well. I think if you add a ring parameter to the functions, we can start specialising per ring and global state changes. You will then also be in a position to judge what is the best idle timer (and consider making i915_gem_idle_work_handler per ring). The goal is simply to evolve the current infrastucture for idle/busyness handling to cover your use case as well (and hopefully in the process improving the old/general cases). -Chris Thanks Chris. extending intel_mark_busy/intel_mark_idle makes sense. I will work on adding the change ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH igt 1/2] lib/kms: Add a way to override an output's mode
On Wed, 2015-03-11 at 13:26 +, Damien Lespiau wrote: On Wed, Mar 11, 2015 at 01:33:00PM +0200, Ander Conselvan de Oliveira wrote: So that it is possible to use a custom mode with the simplified mode set API. Maybe just igt_output_set_mode()? That works too. I used override since there's a chance the desired mode won't produce the expected results. But now that I think about it force mode would sound more like that. In any case, I don't mind either way. Ander ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/i915/chv: Set min freq to efficient frequency on chv
On Thursday 26 February 2015 09:42 PM, Chris Wilson wrote: On Thu, Feb 26, 2015 at 08:46:56PM +0530, deepa...@linux.intel.com wrote: From: Deepak S deepa...@linux.intel.com After feedback from the hardware team, now we set the GPU min freq to RPe. If we drop the freq to RPn, we found that the punit was not setting the voltage to Vnn, So recommendation is to set min freq to RPe. And does it change the voltage at all? Yes Voltage does change when we drop to RPe Is there really any advantage to the extra code on idle?Does efficient_freq really consume less power than min_freq when active (assuming a min_freq/efficient_freq busy workload i.e. does a workload that would be 100% busy at min_freq consume less power when run at efficient_freq)? The delta voltage usage between RPn and RPe is very small like close to zero. Also, if we run workload 100% busy at Rpe we get better performance without much of voltage loss right? btw, Punit expects us to operate between Rpe RP0. -Chris ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 3/5] drm/i915/chv: Set min freq to efficient frequency on chv
On Wed, Mar 11, 2015 at 07:23:48PM +0530, Deepak S wrote: On Thursday 26 February 2015 09:42 PM, Chris Wilson wrote: On Thu, Feb 26, 2015 at 08:46:56PM +0530, deepa...@linux.intel.com wrote: From: Deepak S deepa...@linux.intel.com After feedback from the hardware team, now we set the GPU min freq to RPe. If we drop the freq to RPn, we found that the punit was not setting the voltage to Vnn, So recommendation is to set min freq to RPe. And does it change the voltage at all? Yes Voltage does change when we drop to RPe Is there really any advantage to the extra code on idle?Does efficient_freq really consume less power than min_freq when active (assuming a min_freq/efficient_freq busy workload i.e. does a workload that would be 100% busy at min_freq consume less power when run at efficient_freq)? The delta voltage usage between RPn and RPe is very small like close to zero. Also, if we run workload 100% busy at Rpe we get better performance without much of voltage loss right? btw, Punit expects us to operate between Rpe RP0. If you need 100% at RPe you obviously can't run at RPn (since that would lead to dropped frames). The question is if you have a workload that requires 100% at RPn do you save power if you ran e.g. 80% at RPe? If the punit only works reliably between RPe and RP0, then the current RPn is a bit of a misnomer, and that should be the explanation in the commit log. Definitely do not conflate the idea of executing at RPn and RPe with the idea of idling at RPn or RPe - this patch affects idle frequency. -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 igt 1/2] lib/kms: Add a way to override an output's mode
On Wed, Mar 11, 2015 at 03:48:00PM +0200, Ander Conselvan De Oliveira wrote: On Wed, 2015-03-11 at 13:26 +, Damien Lespiau wrote: On Wed, Mar 11, 2015 at 01:33:00PM +0200, Ander Conselvan de Oliveira wrote: So that it is possible to use a custom mode with the simplified mode set API. Maybe just igt_output_set_mode()? That works too. I used override since there's a chance the desired mode won't produce the expected results. But now that I think about it force mode would sound more like that. In any case, I don't mind either way. No, me neither, can go as is anyway. -- Damien ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: Remove the preliminary_hw_support shackles from CHV
Tested-By: PRC QA PRTS (Patch Regression Test System Contact: shuang...@intel.com) Task id: 5937 -Summary- Platform Delta drm-intel-nightly Series Applied PNV -4 281/281 277/281 ILK -1 308/308 307/308 SNB -1 284/284 283/284 IVB 375/375 375/375 BYT 294/294 294/294 HSW 384/384 384/384 BDW 315/315 315/315 -Detailed- Platform Testdrm-intel-nightly Series Applied PNV igt_gem_userptr_blits_coherency-sync CRASH(5)PASS(3) CRASH(1)PASS(1) PNV igt_gen3_render_linear_blits FAIL(3)PASS(2) FAIL(1)PASS(1) PNV igt_gen3_render_mixed_blits FAIL(2)PASS(3) FAIL(1)PASS(1) PNV igt_gen3_render_tiledx_blits FAIL(4)PASS(2) FAIL(1)PASS(1) *ILK igt_gem_unfence_active_buffers PASS(3) DMESG_WARN(1)PASS(1) *SNB igt_gem_flink_flink-lifetime PASS(2) DMESG_WARN(1)PASS(1) Note: You need to pay more attention to line start with '*' ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915: add i915 specific connector debugfs file for DPCD
On 2/25/2015 5:34 AM, Jani Nikula wrote:
Occasionally it would be interesting to read some of the DPCD registers
for debug purposes, without having to resort to logging. Add an i915
specific i915_dpcd debugfs file for DP and eDP connectors to dump parts
of the DPCD. Currently the DPCD addresses to be dumped are statically
configured, and more can be added trivially.
The implementation also makes it relatively easy to add other i915 and
connector specific debugfs files in the future, as necessary.
This is currently i915 specific just because there's no generic way to
do AUX transactions given just a drm_connector. However it's all pretty
straightforward to port to other drivers.
Signed-off-by: Jani Nikula jani.nik...@intel.com
---
drivers/gpu/drm/i915/i915_debugfs.c | 93 +
drivers/gpu/drm/i915/i915_drv.h | 1 +
drivers/gpu/drm/i915/intel_dp.c | 2 +
3 files changed, 96 insertions(+)
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c
b/drivers/gpu/drm/i915/i915_debugfs.c
index 26e9c7b34113..451ef456c25f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4652,3 +4652,96 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
drm_debugfs_remove_files(info_list, 1, minor);
}
}
+
+struct dpcd_block {
+ /* DPCD dump start address. */
+ unsigned int offset;
+ /* DPCD dump end address, inclusive. If unset, .size will be used. */
+ unsigned int end;
+ /* DPCD dump size. Used if .end is unset. If unset, defaults to 1. */
+ size_t size;
+ /* Only valid for eDP. */
+ bool edp;
+};
+
+static const struct dpcd_block i915_dpcd_debug[] = {
+ { .offset = DP_DPCD_REV, .size = DP_RECEIVER_CAP_SIZE },
+ { .offset = DP_PSR_SUPPORT, .end = DP_PSR_CAPS },
+ { .offset = DP_DOWNSTREAM_PORT_0, .size = 16 },
+ { .offset = DP_LINK_BW_SET, .end = DP_EDP_CONFIGURATION_SET },
+ { .offset = DP_SINK_COUNT, .end = DP_ADJUST_REQUEST_LANE2_3 },
+ { .offset = DP_SET_POWER },
+ { .offset = DP_EDP_DPCD_REV },
+};
+
+static int i915_dpcd_show(struct seq_file *m, void *data)
+{
+ struct drm_connector *connector = m-private;
+ struct intel_dp *intel_dp =
+ enc_to_intel_dp(intel_attached_encoder(connector)-base);
+ uint8_t buf[16];
+ ssize_t err;
+ int i;
+
+ for (i = 0; i ARRAY_SIZE(i915_dpcd_debug); i++) {
+ const struct dpcd_block *b = i915_dpcd_debug[i];
+ size_t size = b-end ? b-end - b-offset + 1 : (b-size ?: 1);
+
+ if (b-edp
+ connector-connector_type != DRM_MODE_CONNECTOR_eDP)
+ continue;
+
+ /* low tech for now */
+ if (WARN_ON(size sizeof(buf)))
+ continue;
+
+ err = drm_dp_dpcd_read(intel_dp-aux, b-offset, buf, size);
+ if (err = 0) {
+ DRM_ERROR(dpcd read (%zu bytes at %u) failed (%zd)\n,
+ size, b-offset, err);
+ continue;
+ }
+
+ seq_printf(m, %04x: %*ph\n, b-offset, (int) size, buf);
+ };
+
+ return 0;
+}
+
+static int i915_dpcd_open(struct inode *inode, struct file *file)
+{
+ return single_open(file, i915_dpcd_show, inode-i_private);
+}
+
+static const struct file_operations i915_dpcd_fops = {
+ .owner = THIS_MODULE,
+ .open = i915_dpcd_open,
+ .read = seq_read,
+ .llseek = seq_lseek,
+ .release = single_release,
+};
+
+/**
+ * i915_debugfs_connector_add - add i915 specific connector debugfs files
+ * @connector: pointer to a registered drm_connector
+ *
+ * Cleanup will be done by drm_connector_unregister() through a call to
+ * drm_debugfs_connector_remove().
+ *
+ * Returns 0 on success, negative error codes on error.
+ */
+int i915_debugfs_connector_add(struct drm_connector *connector)
+{
+ struct dentry *root = connector-debugfs_entry;
+
+ /* The connector must have been registered beforehands. */
+ if (!root)
+ return -ENODEV;
+
+ if (connector-connector_type == DRM_MODE_CONNECTOR_DisplayPort ||
+ connector-connector_type == DRM_MODE_CONNECTOR_eDP)
+ debugfs_create_file(i915_dpcd, S_IRUGO, root, connector,
+ i915_dpcd_fops);
+
+ return 0;
+}
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d42040fbd3c4..f8d42c7afda4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3095,6 +3095,7 @@ int i915_verify_lists(struct drm_device *dev);
/* i915_debugfs.c */
int i915_debugfs_init(struct drm_minor *minor);
void i915_debugfs_cleanup(struct drm_minor *minor);
+int i915_debugfs_connector_add(struct drm_connector *connector);
#ifdef CONFIG_DEBUG_FS
void intel_display_crc_init(struct
Re: [Intel-gfx] [Beignet] [PATCH 2/2 v2] Query the driver directly for compute units and subslice
LGTM,
Reviewed-by: Zhigang Gong zhigang.g...@linux.intel.com
Thanks.
-Original Message-
From: Beignet [mailto:beignet-boun...@lists.freedesktop.org] On Behalf Of
jeff.mc...@intel.com
Sent: Tuesday, March 10, 2015 7:36 AM
To: beig...@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org; dri-de...@lists.freedesktop.org
Subject: [Beignet] [PATCH 2/2 v2] Query the driver directly for compute units
and subslice
From: Jeff McGee jeff.mc...@intel.com
Values of device max compute units and max subslice obtained directly from
the driver should be more accurate than our own ID-based lookup values. This
is particularly important when a single device ID may encompass more than
one configuration. If the driver cannot provide a valid value for the given
device,
we fallback on the ID-based lookup value.
This query requires libdrm 2.4.60. For now we will consider the use of this
query
to be optional and exclude it from compilation when building against older
libdrm. Later we may want to consider requiring the query or at least warning
more strongly when it is not supported.
v2: Make feature use conditional on libdrm version (Zhigang).
Signed-off-by: Jeff McGee jeff.mc...@intel.com
---
CMakeLists.txt | 9 +
src/CMakeLists.txt | 10 ++
src/intel/intel_driver.c | 25 +
3 files changed, 40 insertions(+), 4 deletions(-)
diff --git a/CMakeLists.txt b/CMakeLists.txt index 65f2c70..bb03566 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -131,6 +131,15 @@ IF(DRM_INTEL_FOUND)
ELSE(DRM_INTEL_VERSION VERSION_GREATER 2.4.57)
MESSAGE(STATUS Disable userptr support)
ENDIF(DRM_INTEL_VERSION VERSION_GREATER 2.4.57)
+ IF(DRM_INTEL_VERSION VERSION_GREATER 2.4.59)
+MESSAGE(STATUS Enable EU total query support)
+SET(DRM_INTEL_EU_TOTAL enable)
+MESSAGE(STATUS Enable subslice total query support)
+SET(DRM_INTEL_SUBSLICE_TOTAL enable)
ELSE(DRM_INTEL_VERSION
+ VERSION_GREATER 2.4.59)
+MESSAGE(STATUS Disable EU total query support)
+MESSAGE(STATUS Disable subslice total query support)
+ ENDIF(DRM_INTEL_VERSION VERSION_GREATER 2.4.59)
ELSE(DRM_INTEL_FOUND)
MESSAGE(FATAL_ERROR Looking for DRM Intel (= 2.4.52) - not found)
ENDIF(DRM_INTEL_FOUND)
diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index d4181d8..464765f
100644
--- a/src/CMakeLists.txt
+++ b/src/CMakeLists.txt
@@ -118,6 +118,16 @@ SET(CMAKE_CXX_FLAGS -DHAS_USERPTR
${CMAKE_CXX_FLAGS}) SET(CMAKE_C_FLAGS -DHAS_USERPTR
${CMAKE_C_FLAGS}) endif (DRM_INTEL_USERPTR)
+if (DRM_INTEL_EU_TOTAL)
+SET(CMAKE_CXX_FLAGS -DHAS_EU_TOTAL ${CMAKE_CXX_FLAGS})
+SET(CMAKE_C_FLAGS -DHAS_EU_TOTAL ${CMAKE_C_FLAGS}) endif
+(DRM_INTEL_EU_TOTAL)
+
+if (DRM_INTEL_SUBSLICE_TOTAL)
+SET(CMAKE_CXX_FLAGS -DHAS_SUBSLICE_TOTAL ${CMAKE_CXX_FLAGS})
+SET(CMAKE_C_FLAGS -DHAS_SUBSLICE_TOTAL ${CMAKE_C_FLAGS}) endif
+(DRM_INTEL_SUBSLICE_TOTAL)
+
set(GIT_SHA1 git_sha1.h)
add_custom_target(${GIT_SHA1} ALL
COMMAND chmod +x ${CMAKE_CURRENT_SOURCE_DIR}/git_sha1.sh
diff --git a/src/intel/intel_driver.c b/src/intel/intel_driver.c index
d61988c..755ab6b 100644
--- a/src/intel/intel_driver.c
+++ b/src/intel/intel_driver.c
@@ -757,10 +757,7 @@ static int intel_buffer_set_tiling(cl_buffer bo, static
void intel_update_device_info(cl_device_id device) { -#ifdef HAS_USERPTR
intel_driver_t *driver;
- const size_t sz = 4096;
- void *host_ptr;
driver = intel_driver_new();
assert(driver != NULL);
@@ -769,6 +766,10 @@ intel_update_device_info(cl_device_id device)
return;
}
+#ifdef HAS_USERPTR
+ const size_t sz = 4096;
+ void *host_ptr;
+
host_ptr = cl_aligned_malloc(sz, 4096);
if (host_ptr != NULL) {
cl_buffer bo = intel_buffer_alloc_userptr((cl_buffer_mgr)driver-bufmgr,
@@ -781,12 +782,28 @@ intel_update_device_info(cl_device_id device)
}
else
device-host_unified_memory = CL_FALSE;
+#endif
+
+#ifdef HAS_EU_TOTAL
+ unsigned int eu_total;
+
+ /* Prefer driver-queried max compute units if supported */
+ if (!drm_intel_get_eu_total(driver-fd, eu_total))
+device-max_compute_unit = eu_total; #endif
+
+#ifdef HAS_SUBSLICE_TOTAL
+ unsigned int subslice_total;
+
+ /* Prefer driver-queried subslice count if supported */
+ if (!drm_intel_get_subslice_total(driver-fd, subslice_total))
+device-sub_slice_count = subslice_total; #endif
intel_driver_context_destroy(driver);
intel_driver_close(driver);
intel_driver_terminate(driver);
intel_driver_delete(driver);
-#endif
}
LOCAL void
--
2.3.0
___
Beignet mailing list
beig...@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/beignet
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
