Re: [Intel-gfx] skylake + drm-next - warn city

[Jani Nikula]

On Tue, 03 Nov 2015, Dave Airlie  wrote:
> Just booted drm-next on a Skylake laptop that happened to be on my
> desk for a few days.
>
> I wasn't impressed. I'm very disappointed. Doesn't anyone have any
> pride in the code they write anymore.
>
> Initially the previous sentence had a lot of curse words and was Linus
> like in it's stature, but I've been promised by twitter that being
> nice will get me better results, so let's make it so.

Much appreciated; we get the message.

> So could someone from Intel takes some responsibility for testing the
> code they send me actually you know works on the hardware it's meant
> to, or at least tell me what is going so horribly wrong here.
>
> the lockdep trace at the end doesn't look fun.

Skylake wants the DMC firmware blob from linux-firmware or
https://01.org/linuxgraphics/downloads. The conclusion from the traces
below is that either you don't have it, or we fail to load it due to the
deadlock.

In any case, our DMC firmware loading is, uh, less than perfect.

The bright side is that we are aware of this, and there's a couple of
patchsets from Mika/Damien [1] and Animesh/Daniel/Imre [2] to fix
this. (Mika, Imre, any comments on the status of those?)

The bigger question is how do we fix this for drm-next/v4.4. That's 20
patches in total, and we are way past the cutoff. On the other hand,
it's all restricted to Skylake DMC firmware loading, and it is supposed
to fix stuff, not add features.

Your call.


BR,
Jani.



[1] 
http://mid.gmane.org/1445950025-5793-1-git-send-email-mika.kuopp...@intel.com
[2] http://mid.gmane.org/1446069547-24760-1-git-send-email-imre.d...@intel.com


>
> Dave.
>
>
> [8.158254] ACPI: Video Device [GFX0] (multi-head: yes  rom: no  post: no)
> [8.159953] input: Video Bus as
> /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input12
> [8.160895] [drm] Initialized i915 1.6.0 20151010 for :00:02.0 on 
> minor 0
> [8.170784] [ cut here ]
> [8.170810] WARNING: CPU: 3 PID: 103 at
> drivers/gpu/drm/i915/intel_csr.c:481 assert_csr_loaded+0xa8/0x140
> [i915]()
> [8.170812] CSR is not loaded.
> [8.170813] Modules linked in: amdkfd amd_iommu_v2 amdgpu i915 ttm
> i2c_algo_bit drm_kms_helper serio_raw drm r8169 mii video fjes
> [8.170825] CPU: 3 PID: 103 Comm: kworker/u16:2 Not tainted 4.3.0-rc5+ #1
> [8.170826] Hardware name: HP HP ProBook 470 G3/8102, BIOS N78 Ver.
> 01.01 09/04/2015
> [8.170830] Workqueue: events_unbound async_run_entry_fn
> [8.170832]   1aac9e2e 88024bd33a68
> 81416e09
> [8.170835]  88024bd33ab0 88024bd33aa0 810a8bb2
> 88003f13
> [8.170838]  88003f130510  300f
> 88024ad23000
> [8.170841] Call Trace:
> [8.170845]  [] dump_stack+0x4b/0x72
> [8.170847]  [] warn_slowpath_common+0x82/0xc0
> [8.170849]  [] warn_slowpath_fmt+0x5c/0x80
> [8.170866]  [] assert_csr_loaded+0xa8/0x140 [i915]
> [8.170885]  [] skl_set_power_well+0x7e5/0xb00 [i915]
> [8.170902]  [] skl_power_well_enable+0x13/0x20 [i915]
> [8.170917]  [] intel_display_power_get+0xab/0x100 [i915]
> [8.170944]  [] intel_hdmi_set_edid+0x3b/0x110 [i915]
> [8.170969]  [] intel_hdmi_detect+0xc0/0x130 [i915]
> [8.170974]  []
> drm_helper_probe_single_connector_modes_merge_bits+0x235/0x4d0
> [drm_kms_helper]
> [8.170978]  []
> drm_helper_probe_single_connector_modes+0x13/0x20 [drm_kms_helper]
> [8.170983]  []
> drm_fb_helper_initial_config+0xb0/0x410 [drm_kms_helper]
> [8.171007]  [] intel_fbdev_initial_config+0x1b/0x20 
> [i915]
> [8.171009]  [] async_run_entry_fn+0x4a/0x140
> [8.171011]  [] process_one_work+0x230/0x680
> [8.171013]  [] ? process_one_work+0x199/0x680
> [8.171015]  [] worker_thread+0x4e/0x450
> [8.171017]  [] ? process_one_work+0x680/0x680
> [8.171020]  [] kthread+0x101/0x120
> [8.171023]  [] ? trace_hardirqs_on_caller+0x129/0x1b0
> [8.171026]  [] ? kthread_create_on_node+0x250/0x250
> [8.171028]  [] ret_from_fork+0x3f/0x70
> [8.171031]  [] ? kthread_create_on_node+0x250/0x250
> [8.171032] ---[ end trace 4692db411b428244 ]---
> [8.171035] [ cut here ]
> [8.171053] WARNING: CPU: 3 PID: 103 at
> drivers/gpu/drm/i915/intel_csr.c:484 assert_csr_loaded+0x103/0x140
> [i915]()
> [8.171054] CSR SSP Base Not fine
> [8.171055] Modules linked in: amdkfd amd_iommu_v2 amdgpu i915 ttm
> i2c_algo_bit drm_kms_helper serio_raw drm r8169 mii video fjes
> [8.171064] CPU: 3 PID: 103 Comm: kworker/u16:2 Tainted: GW
>   4.3.0-rc5+ #1
> [8.171065] Hardware name: HP HP ProBook 470 G3/8102, BIOS N78 Ver.
> 01.01 09/04/2015
> [8.171067] Workqueue: events_unbound async_run_entry_fn
> [8.171069]   1aac9e2e 88024bd33a68
> 81416e09
> [8.171071]  88024bd33ab0 88024bd33aa0 

Re: [Intel-gfx] [PATCH v2 01/14] drm/i915: Use passed plane state for sprite planes, v3.

[Ville Syrjälä]

On Tue, Nov 03, 2015 at 08:31:40AM +0100, Maarten Lankhorst wrote:
> Don't use plane->state directly, use the pointer from commit_plane.
> 
> Changes since v1:
> - Fix uses of plane->state->rotation and color key to use the passed state 
> too.
> - Only pass crtc_state and plane_state to update_plane.
> Changes since v2:
> - Rebased.
> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/intel_drv.h|  10 +--
>  drivers/gpu/drm/i915/intel_sprite.c | 120 
> +++-
>  2 files changed, 67 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index d1a6071afabe..16d4627364c5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -647,16 +647,12 @@ struct intel_plane {
>   /*
>* NOTE: Do not place new plane state fields here (e.g., when adding
>* new plane properties).  New runtime state should now be placed in
> -  * the intel_plane_state structure and accessed via drm_plane->state.
> +  * the intel_plane_state structure and accessed via plane_state.
>*/
>  
>   void (*update_plane)(struct drm_plane *plane,
> -  struct drm_crtc *crtc,
> -  struct drm_framebuffer *fb,
> -  int crtc_x, int crtc_y,
> -  unsigned int crtc_w, unsigned int crtc_h,
> -  uint32_t x, uint32_t y,
> -  uint32_t src_w, uint32_t src_h);
> +  struct intel_crtc_state *crtc_state,
> +  struct intel_plane_state *plane_state);

These should be const.

>   void (*disable_plane)(struct drm_plane *plane,
> struct drm_crtc *crtc);
>   int (*check_plane)(struct drm_plane *plane,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
> b/drivers/gpu/drm/i915/intel_sprite.c
> index 4276c135b9f2..6d3047f9c80f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -178,28 +178,32 @@ void intel_pipe_update_end(struct intel_crtc *crtc)
>  }
>  
>  static void
> -skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
> -  struct drm_framebuffer *fb,
> -  int crtc_x, int crtc_y,
> -  unsigned int crtc_w, unsigned int crtc_h,
> -  uint32_t x, uint32_t y,
> -  uint32_t src_w, uint32_t src_h)
> +skl_update_plane(struct drm_plane *drm_plane,
> +  struct intel_crtc_state *crtc_state,
> +  struct intel_plane_state *plane_state)
>  {
>   struct drm_device *dev = drm_plane->dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_plane *intel_plane = to_intel_plane(drm_plane);
> + struct drm_framebuffer *fb = plane_state->base.fb;
>   struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>   const int pipe = intel_plane->pipe;
>   const int plane = intel_plane->plane + 1;
>   u32 plane_ctl, stride_div, stride;
> - const struct drm_intel_sprite_colorkey *key =
> - _intel_plane_state(drm_plane->state)->ckey;
> + const struct drm_intel_sprite_colorkey *key = _state->ckey;
>   unsigned long surf_addr;
>   u32 tile_height, plane_offset, plane_size;
>   unsigned int rotation;
>   int x_offset, y_offset;
> - struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
> - int scaler_id;
> +

What's with the extra newline?

> + int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
> + uint32_t crtc_w = drm_rect_width(_state->dst);
> + uint32_t crtc_h = drm_rect_height(_state->dst);
> + uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
> + uint32_t src_w = drm_rect_width(_state->src) >> 16;
> + uint32_t src_h = drm_rect_height(_state->src) >> 16;
> + const struct intel_scaler *scaler =
> + _state->scaler_state.scalers[plane_state->scaler_id];

This looks messy. I would at least get rid of the "assign two things on
one line thing".

>  
>   plane_ctl = PLANE_CTL_ENABLE |
>   PLANE_CTL_PIPE_GAMMA_ENABLE |
> @@ -208,14 +212,12 @@ skl_update_plane(struct drm_plane *drm_plane, struct 
> drm_crtc *crtc,
>   plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
>   plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
>  
> - rotation = drm_plane->state->rotation;
> + rotation = plane_state->base.rotation;
>   plane_ctl |= skl_plane_ctl_rotation(rotation);
>  
>   stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>  fb->pixel_format);
>  
> - scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
> -
>   /* Sizes are 0 based */
>   src_w--;
>   src_h--;
> @@ -256,13 +258,13 @@ skl_update_plane(struct drm_plane 

Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Ville Syrjälä]

On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
> This fixes a warning when the crtc is turned off. In that case fb
> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
> active this is not a bug, and shouldn't trigger the WARN_ON.

Mm. We want to do scaling checks and whatnot during DPMS. So this should
check .enabled, no?

> Also remove handling a null crtc_state, with all transitional helpers
> gone this can no longer happen.

What about the !intel_crtc check, how is that supposed to happen?

> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  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 304e1028c9a4..7e2caeef9a11 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13671,7 +13671,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct 
> intel_crtc_state *crtc_state
>   struct drm_i915_private *dev_priv;
>   int crtc_clock, cdclk;
>  
> - if (!intel_crtc || !crtc_state)
> + if (!intel_crtc || !crtc_state->base.active)
>   return DRM_PLANE_HELPER_NO_SCALING;
>  
>   dev = intel_crtc->base.dev;
> -- 
> 2.1.0
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 02/14] drm/i915: Extend DSL readout fix to BDW and SKL.

[Ville Syrjälä]

On Tue, Nov 03, 2015 at 08:31:41AM +0100, Maarten Lankhorst wrote:
> Those platforms have the same bug as haswell, and the same fix applies to 
> them.
> 
> Signed-off-by: Maarten Lankhorst 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91579

Sigh. 
Acked-by: Ville Syrjälä 

> ---
>  drivers/gpu/drm/i915/i915_irq.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> index 6e0a5683bbdc..825114af9c56 100644
> --- a/drivers/gpu/drm/i915/i915_irq.c
> +++ b/drivers/gpu/drm/i915/i915_irq.c
> @@ -747,7 +747,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc 
> *crtc)
>* problem.  We may need to extend this to include other platforms,
>* but so far testing only shows the problem on HSW.
>*/
> - if (IS_HASWELL(dev) && !position) {
> + if (HAS_DDI(dev) && !position) {
>   int i, temp;
>  
>   for (i = 0; i < 100; i++) {
> -- 
> 2.1.0
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 0/4] Wrap up ILK-style atomic watermarks, try two

[Jani Nikula]

On Tue, 03 Nov 2015, Matt Roper  wrote:
> Patches #3 and #4 here are the final two patches from the atomic watermark
> series that was posted here:
>
>http://lists.freedesktop.org/archives/intel-gfx/2015-September/076634.html
>
> We had to pull those out when Jani reported a BDW boot regression (divide by
> zero during watermark calculation).  Although we never found a smoking gun for
> that divide by zero, I haven't been able to reproduce the issue on a similar
> system.  There's been a lot of code churn since that time, so I'm hoping that
> we've either already fixed the issue without realizing it, or that the extra
> paranoia added in patch #2 here will avoid the crash and highlight the 
> culprit.
>
> The first patch here solves a legitimate bug that could cause a divide-by-zero
> (just not the one Jani was seeing).  The second patch adds extra guards on
> divide operations to verify our invariants and ensure that bugs elsewhere in
> the driver can't lead to a fatal divide-by-zero (at least on the ILK
> codepaths).
>
> Please don't merge #3 or #4 here until we at least get a positive test result
> from Jani.

Still gives me warnings. I didn't try one patch at a time, but here are
the dmesgs before http://pastebin.com/GVG9UC1U and after
http://pastebin.com/1HaXTJ3Z applying the series on top of today's
nightly.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Maarten Lankhorst]

Op 03-11-15 om 10:09 schreef Ville Syrjälä:
> On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
>> This fixes a warning when the crtc is turned off. In that case fb
>> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
>> active this is not a bug, and shouldn't trigger the WARN_ON.
> Mm. We want to do scaling checks and whatnot during DPMS. So this should
> check .enabled, no?
Not sure what the right decision would be here..

 * skl max scale is lower of:
 *close to 3 but not 3, -1 is for that purpose
 *or
 *cdclk/crtc_clock

So when multiple pipes are enabled potentially 3x scaling is allowed, but if 
you dpms them all off
cdclk might get set to 0. This means a previous valid amount of scaling might 
suddenly become invalid.

Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting.

This probably needs to be addressed in patch 3/14 then, so that one needs more 
love.

I will resend patch 3, 4, 5 and 14 as a separate series. The other patches from 
this series will still work with some easily fixed rejects.
>> Also remove handling a null crtc_state, with all transitional helpers
>> gone this can no longer happen.
> What about the !intel_crtc check, how is that supposed to happen?
When fb == NULL. :)
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  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 304e1028c9a4..7e2caeef9a11 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -13671,7 +13671,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct 
>> intel_crtc_state *crtc_state
>>  struct drm_i915_private *dev_priv;
>>  int crtc_clock, cdclk;
>>  
>> -if (!intel_crtc || !crtc_state)
>> +if (!intel_crtc || !crtc_state->base.active)
>>  return DRM_PLANE_HELPER_NO_SCALING;
>>  
>>  dev = intel_crtc->base.dev;
>> -- 
>> 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] skylake + drm-next - warn city

[Ville Syrjälä]

On Tue, Nov 03, 2015 at 12:35:58PM +0200, Imre Deak wrote:
> On ti, 2015-11-03 at 11:42 +0200, Jani Nikula wrote:
> > On Tue, 03 Nov 2015, Dave Airlie  wrote:
> > > Just booted drm-next on a Skylake laptop that happened to be on my
> > > desk for a few days.
> > >
> > > I wasn't impressed. I'm very disappointed. Doesn't anyone have any
> > > pride in the code they write anymore.
> > >
> > > Initially the previous sentence had a lot of curse words and was Linus
> > > like in it's stature, but I've been promised by twitter that being
> > > nice will get me better results, so let's make it so.
> > 
> > Much appreciated; we get the message.
> > 
> > > So could someone from Intel takes some responsibility for testing the
> > > code they send me actually you know works on the hardware it's meant
> > > to, or at least tell me what is going so horribly wrong here.
> > >
> > > the lockdep trace at the end doesn't look fun.
> 
> That's from the GuC firmware loader, so won't be fixed by the DMC
> patches. Ville mentioned he has a fix for that.

No, I don't. I probably just mentioned that I saw this lockdep spew
already some weeks ago, and complained about it at the time. I think
Daniel tried to ping someone specific about it, but no idea if they
took any notice.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 02/14] drm/i915: Extend DSL readout fix to BDW and SKL.

[Jani Nikula]

On Tue, 03 Nov 2015, Ville Syrjälä  wrote:
> On Tue, Nov 03, 2015 at 08:31:41AM +0100, Maarten Lankhorst wrote:
>> Those platforms have the same bug as haswell, and the same fix applies to 
>> them.

How about Broxton? IS_DDI matches that.

Jani.

>> 
>> Signed-off-by: Maarten Lankhorst 
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=91579
>
> Sigh. 
> Acked-by: Ville Syrjälä 
>
>> ---
>>  drivers/gpu/drm/i915/i915_irq.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_irq.c 
>> b/drivers/gpu/drm/i915/i915_irq.c
>> index 6e0a5683bbdc..825114af9c56 100644
>> --- a/drivers/gpu/drm/i915/i915_irq.c
>> +++ b/drivers/gpu/drm/i915/i915_irq.c
>> @@ -747,7 +747,7 @@ static int __intel_get_crtc_scanline(struct intel_crtc 
>> *crtc)
>>   * problem.  We may need to extend this to include other platforms,
>>   * but so far testing only shows the problem on HSW.
>>   */
>> -if (IS_HASWELL(dev) && !position) {
>> +if (HAS_DDI(dev) && !position) {
>>  int i, temp;
>>  
>>  for (i = 0; i < 100; i++) {
>> -- 
>> 2.1.0
>> 
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 5/8] drm/i915: Do not warn on PG2 enabled in gen9_disable_dc5()

[Patrik Jakobsson]

PG2 enabled is not a requirement for disabling DC5. It's just one
of the reasons why we wouldn't enter DC5. During modeset we don't care
about PG2 from a DC perspective, only the fact that DC5/DC6 is not
allowed.

Signed-off-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index d0ed38b..c901b19 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -483,8 +483,6 @@ static void assert_can_enable_dc5(struct drm_i915_private 
*dev_priv)
 
 static void assert_can_disable_dc5(struct drm_i915_private *dev_priv)
 {
-   bool pg2_enabled = intel_display_power_well_is_enabled(dev_priv,
-   SKL_DISP_PW_2);
/*
 * During initialization, the firmware may not be loaded yet.
 * We still want to make sure that the DC enabling flag is cleared.
@@ -492,7 +490,6 @@ static void assert_can_disable_dc5(struct drm_i915_private 
*dev_priv)
if (dev_priv->power_domains.initializing)
return;
 
-   WARN_ONCE(!pg2_enabled, "PG2 not enabled to disable DC5.\n");
WARN_ONCE(dev_priv->pm.suspended,
"Disabling of DC5 while platform is runtime-suspended should 
never happen.\n");
 }
-- 
2.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/8] drm/i915: Introduce a gmbus power domain

[Patrik Jakobsson]

From: Ville Syrjälä 

Currently the gmbus code uses intel_aux_display_runtime_get/put in an
effort to make sure the hardware is powered up sufficiently for gmbus.
That function only takes the runtime PM reference which on VLV/CHV/BXT
is not enough. We need the disp2d/pipe-a well on VLV/CHV and power well
2 on BXT. So add a new power domnain for gmbus and kill off the now
unused intel_aux_display_runtime_get/put. And change
intel_hdmi_set_edid() to use the gmbus power domain too since that's all
we need there.

Also toss in a BUILD_BUG_ON() to catch problems if we run out of
bits for power domains. We're already really close to the limit...

[Patrik: Add gmbus string to debugfs output]

Signed-off-by: Ville Syrjälä 
Reviewed-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/i915_debugfs.c |  2 ++
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/intel_drv.h|  2 --
 drivers/gpu/drm/i915/intel_hdmi.c   |  8 ++--
 drivers/gpu/drm/i915/intel_i2c.c|  6 --
 drivers/gpu/drm/i915/intel_runtime_pm.c | 34 -
 6 files changed, 13 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index f727887..f7b85fe 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2744,6 +2744,8 @@ static const char *power_domain_str(enum 
intel_display_power_domain domain)
return "AUX_C";
case POWER_DOMAIN_AUX_D:
return "AUX_D";
+   case POWER_DOMAIN_GMBUS:
+   return "GMBUS";
case POWER_DOMAIN_INIT:
return "INIT";
default:
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 05aeaee..c3d3b2a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -199,6 +199,7 @@ enum intel_display_power_domain {
POWER_DOMAIN_AUX_B,
POWER_DOMAIN_AUX_C,
POWER_DOMAIN_AUX_D,
+   POWER_DOMAIN_GMBUS,
POWER_DOMAIN_INIT,
 
POWER_DOMAIN_NUM,
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 933f1e8..d110555 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1394,8 +1394,6 @@ void intel_display_power_get(struct drm_i915_private 
*dev_priv,
 enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_i915_private *dev_priv,
 enum intel_display_power_domain domain);
-void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv);
-void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_get_noresume(struct drm_i915_private *dev_priv);
 void intel_runtime_pm_put(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_hdmi.c 
b/drivers/gpu/drm/i915/intel_hdmi.c
index 013bd7d..cea05b8 100644
--- a/drivers/gpu/drm/i915/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/intel_hdmi.c
@@ -1335,21 +1335,17 @@ intel_hdmi_set_edid(struct drm_connector *connector, 
bool force)
 {
struct drm_i915_private *dev_priv = to_i915(connector->dev);
struct intel_hdmi *intel_hdmi = intel_attached_hdmi(connector);
-   struct intel_encoder *intel_encoder =
-   _to_dig_port(intel_hdmi)->base;
-   enum intel_display_power_domain power_domain;
struct edid *edid = NULL;
bool connected = false;
 
-   power_domain = intel_display_port_power_domain(intel_encoder);
-   intel_display_power_get(dev_priv, power_domain);
+   intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
 
if (force)
edid = drm_get_edid(connector,
intel_gmbus_get_adapter(dev_priv,
intel_hdmi->ddc_bus));
 
-   intel_display_power_put(dev_priv, power_domain);
+   intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
 
to_intel_connector(connector)->detect_edid = edid;
if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
diff --git a/drivers/gpu/drm/i915/intel_i2c.c b/drivers/gpu/drm/i915/intel_i2c.c
index bd58da0..fe69623 100644
--- a/drivers/gpu/drm/i915/intel_i2c.c
+++ b/drivers/gpu/drm/i915/intel_i2c.c
@@ -483,7 +483,7 @@ gmbus_xfer(struct i2c_adapter *adapter,
int i = 0, inc, try = 0;
int ret = 0;
 
-   intel_aux_display_runtime_get(dev_priv);
+   intel_display_power_get(dev_priv, POWER_DOMAIN_GMBUS);
mutex_lock(_priv->gmbus_mutex);
 
if (bus->force_bit) {
@@ -595,7 +595,9 @@ timeout:
 
 out:
mutex_unlock(_priv->gmbus_mutex);
-   intel_aux_display_runtime_put(dev_priv);
+
+   intel_display_power_put(dev_priv, POWER_DOMAIN_GMBUS);
+
return ret;
 }
 

[Intel-gfx] [PATCH 3/8] drm/i915: Remove DDI power domain exclusion SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS

[Patrik Jakobsson]

From: Ville Syrjälä 

All the DDI power domains are already excluded from
SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS on account of
excluding SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS and
SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS, no need to spell them out again.

Signed-off-by: Ville Syrjälä 
Reviewed-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 7d309d5..b6ce77f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -339,10 +339,6 @@ static void hsw_set_power_well(struct drm_i915_private 
*dev_priv,
 #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (  \
(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |  \
SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \
-   SKL_DISPLAY_DDI_A_E_POWER_DOMAINS | \
-   SKL_DISPLAY_DDI_B_POWER_DOMAINS |   \
-   SKL_DISPLAY_DDI_C_POWER_DOMAINS |   \
-   SKL_DISPLAY_DDI_D_POWER_DOMAINS |   \
SKL_DISPLAY_MISC_IO_POWER_DOMAINS)) |   \
BIT(POWER_DOMAIN_INIT))
 
-- 
2.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/8] drm/i915: Add a modeset power domain

[Patrik Jakobsson]

We need DC5/DC6 to be disabled around modesets to prevent confusing the
DMC. Also, we've run out of bits in the 32 bit power domain mask so now
it's a 64 bit mask.

Signed-off-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/i915_debugfs.c | 2 ++
 drivers/gpu/drm/i915/i915_drv.h | 3 ++-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
 3 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index f7b85fe..ae9a2ed 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2746,6 +2746,8 @@ static const char *power_domain_str(enum 
intel_display_power_domain domain)
return "AUX_D";
case POWER_DOMAIN_GMBUS:
return "GMBUS";
+   case POWER_DOMAIN_MODESET:
+   return "MODESET";
case POWER_DOMAIN_INIT:
return "INIT";
default:
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c3d3b2a..efb6a00 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -200,6 +200,7 @@ enum intel_display_power_domain {
POWER_DOMAIN_AUX_C,
POWER_DOMAIN_AUX_D,
POWER_DOMAIN_GMBUS,
+   POWER_DOMAIN_MODESET,
POWER_DOMAIN_INIT,
 
POWER_DOMAIN_NUM,
@@ -1226,7 +1227,7 @@ struct i915_power_well {
int count;
/* cached hw enabled state */
bool hw_enabled;
-   unsigned long domains;
+   unsigned long long domains;
unsigned long data;
const struct i915_power_well_ops *ops;
 };
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b6ce77f..d0ed38b 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1815,7 +1815,7 @@ int intel_power_domains_init(struct drm_i915_private 
*dev_priv)
 {
struct i915_power_domains *power_domains = _priv->power_domains;
 
-   BUILD_BUG_ON(POWER_DOMAIN_NUM > 31);
+   BUILD_BUG_ON(POWER_DOMAIN_NUM > 63);
 
mutex_init(_domains->lock);
 
-- 
2.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 1/8] drm/i915: Clean up AUX power domain handling

[Patrik Jakobsson]

From: Ville Syrjälä 

Introduce intel_display_port_aux_power_domain() which simply returns
the appropriate AUX power domain for a specific port, and then replace
the intel_display_port_power_domain() with calls to the new function
in the DP code. As long as we're not actually enabling the port we don't
need the lane power domains, and those are handled now purely from
modeset_update_crtc_power_domains().

My initial motivation for this was to see if I could keep the DPIO power
wells powered down while doing AUX on CHV, but turns out I can't so this
doesn't change anything for CHV at least. But I think it's still a
worthwile change.

Signed-off-by: Ville Syrjälä 
Reviewed-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/intel_display.c | 40 ++
 drivers/gpu/drm/i915/intel_dp.c  | 48 +++-
 drivers/gpu/drm/i915/intel_drv.h |  2 ++
 3 files changed, 56 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 3f1b545..c6d60b8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5170,6 +5170,23 @@ static enum intel_display_power_domain 
port_to_power_domain(enum port port)
}
 }
 
+static enum intel_display_power_domain port_to_aux_power_domain(enum port port)
+{
+   switch (port) {
+   case PORT_A:
+   return POWER_DOMAIN_AUX_A;
+   case PORT_B:
+   return POWER_DOMAIN_AUX_B;
+   case PORT_C:
+   return POWER_DOMAIN_AUX_C;
+   case PORT_D:
+   return POWER_DOMAIN_AUX_D;
+   default:
+   WARN_ON_ONCE(1);
+   return POWER_DOMAIN_AUX_A;
+   }
+}
+
 #define for_each_power_domain(domain, mask)\
for ((domain) = 0; (domain) < POWER_DOMAIN_NUM; (domain)++) \
if ((1 << (domain)) & (mask))
@@ -5201,6 +5218,29 @@ intel_display_port_power_domain(struct intel_encoder 
*intel_encoder)
}
 }
 
+enum intel_display_power_domain
+intel_display_port_aux_power_domain(struct intel_encoder *intel_encoder)
+{
+   struct drm_device *dev = intel_encoder->base.dev;
+   struct intel_digital_port *intel_dig_port;
+
+   switch (intel_encoder->type) {
+   case INTEL_OUTPUT_UNKNOWN:
+   /* Only DDI platforms should ever use this output type */
+   WARN_ON_ONCE(!HAS_DDI(dev));
+   case INTEL_OUTPUT_DISPLAYPORT:
+   case INTEL_OUTPUT_EDP:
+   intel_dig_port = enc_to_dig_port(_encoder->base);
+   return port_to_aux_power_domain(intel_dig_port->port);
+   case INTEL_OUTPUT_DP_MST:
+   intel_dig_port = enc_to_mst(_encoder->base)->primary;
+   return port_to_aux_power_domain(intel_dig_port->port);
+   default:
+   WARN_ON_ONCE(1);
+   return POWER_DOMAIN_AUX_A;
+   }
+}
+
 static unsigned long get_crtc_power_domains(struct drm_crtc *crtc)
 {
struct drm_device *dev = crtc->dev;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1cb1f3f..258d8b1 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -277,7 +277,7 @@ static void pps_lock(struct intel_dp *intel_dp)
 * See vlv_power_sequencer_reset() why we need
 * a power domain reference here.
 */
-   power_domain = intel_display_port_power_domain(encoder);
+   power_domain = intel_display_port_aux_power_domain(encoder);
intel_display_power_get(dev_priv, power_domain);
 
mutex_lock(_priv->pps_mutex);
@@ -293,7 +293,7 @@ static void pps_unlock(struct intel_dp *intel_dp)
 
mutex_unlock(_priv->pps_mutex);
 
-   power_domain = intel_display_port_power_domain(encoder);
+   power_domain = intel_display_port_aux_power_domain(encoder);
intel_display_power_put(dev_priv, power_domain);
 }
 
@@ -816,8 +816,6 @@ intel_dp_aux_ch(struct intel_dp *intel_dp,
 
intel_dp_check_edp(intel_dp);
 
-   intel_aux_display_runtime_get(dev_priv);
-
/* Try to wait for any previous AUX channel activity */
for (try = 0; try < 3; try++) {
status = I915_READ_NOTRACE(ch_ctl);
@@ -926,7 +924,6 @@ done:
ret = recv_bytes;
 out:
pm_qos_update_request(_priv->pm_qos, PM_QOS_DEFAULT_VALUE);
-   intel_aux_display_runtime_put(dev_priv);
 
if (vdd)
edp_panel_vdd_off(intel_dp, false);
@@ -1784,7 +1781,7 @@ static bool edp_panel_vdd_on(struct intel_dp *intel_dp)
if (edp_have_panel_vdd(intel_dp))
return need_to_disable;
 
-   power_domain = intel_display_port_power_domain(intel_encoder);
+   power_domain = intel_display_port_aux_power_domain(intel_encoder);
intel_display_power_get(dev_priv, power_domain);
 
  

Re: [Intel-gfx] [PATCH] RFC drm/i915: Slaughter the thundering i915_wait_request herd

[Chris Wilson]

On Tue, Nov 03, 2015 at 10:03:19AM +, Chris Wilson wrote:
> On Tue, Nov 03, 2015 at 03:06:36AM +, Gong, Zhipeng wrote:
> > It seems that there are some gaps in the patch and first patch. 
> > Like there is no this line in the first patch. 
> >   if (req->ring->seqno_barrier)
> 
> Ah, that was in the context I hope...
> 
> > I have tried to apply this patch. And here is the cpu utilization and perf 
> > data on BDW
> 
> Do you also have a relative perf statistics like op/s we can compare to
> make sure we aren't just stalling the whole system?

Fwiw, I measure about a 3-5% (worst 20%) perf decrease in various X demos
with UXA when disabling the busywait for 20ms after we do an interrupt
driven wait. However, on the very same machine with the same benchmarks
SNA sees major improvement (which looks more likely to be from thermal
throttling affecting the results, but consistent enough to be very
interesting).
-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 03/14] drm/i915: Do not acquire crtc state to check clock during modeset, v2.

[Maarten Lankhorst]

Op 03-11-15 om 08:31 schreef Maarten Lankhorst:
> Parallel modesets are still not allowed, but this will allow updating
> a different crtc during a modeset if the clock is not changed.
>
> Additionally when all pipes are DPMS off the cdclk will be lowered
> to the minimum allowed.
>
> Changes since v1:
> - Add dev_priv->active_crtcs for tracking which crtcs are active.
> - Rename min_cdclk to min_pixclk and move to dev_priv.
> - Add a active_crtcs mask which is updated atomically.
> - Add intel_atomic_state->modeset which is set on modesets.
> - Commit new pixclk/active_crtcs right after state swap.
>
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |   5 ++
>  drivers/gpu/drm/i915/intel_atomic.c  |   2 +-
>  drivers/gpu/drm/i915/intel_display.c | 160 
> +--
>  drivers/gpu/drm/i915/intel_drv.h |   7 +-
>  4 files changed, 125 insertions(+), 49 deletions(-)
>
As I mentioned in discussion of 14/14, patch 3 4 and 5 should be dropped from 
the series.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 0/8] Skylake DMC/DC-state fixes and redesign

[Patrik Jakobsson]

These patches should sit on top of the DMC redesign patches from
Animesh/Imre [1] which in turn depends on Mika's FW debug patches [2].

First two patches are from Ville and is included since they otherwise
might be forgotten. The third from Ville helps with handling DC off when
doing Aux A communication.

[1]
http://lists.freedesktop.org/archives/intel-gfx/2015-October/079041.html

[2]
http://lists.freedesktop.org/archives/intel-gfx/2015-October/078898.html

Patrik Jakobsson (5):
  drm/i915: Add a modeset power domain
  drm/i915: Do not warn on PG2 enabled in gen9_disable_dc5()
  drm/i915/skl: Turn DC handling into a power well
  drm/i915/skl: Add boot parameter for disabling DC6
  drm/i915: Force loading of csr program at boot

Ville Syrjälä (3):
  drm/i915: Clean up AUX power domain handling
  drm/i915: Introduce a gmbus power domain
  drm/i915: Remove DDI power domain exclusion
SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS

 drivers/gpu/drm/i915/i915_debugfs.c |   4 +
 drivers/gpu/drm/i915/i915_drv.c |   8 +-
 drivers/gpu/drm/i915/i915_drv.h |   5 +-
 drivers/gpu/drm/i915/i915_params.c  |   6 ++
 drivers/gpu/drm/i915/intel_csr.c|   6 +-
 drivers/gpu/drm/i915/intel_display.c|  46 ++
 drivers/gpu/drm/i915/intel_dp.c |  48 +++
 drivers/gpu/drm/i915/intel_drv.h|   6 +-
 drivers/gpu/drm/i915/intel_hdmi.c   |   8 +-
 drivers/gpu/drm/i915/intel_i2c.c|   6 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 148 
 11 files changed, 163 insertions(+), 128 deletions(-)

-- 
2.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 6/8] drm/i915/skl: Turn DC handling into a power well

[Patrik Jakobsson]

Handle DC off as a power well where enabling the power well will prevent
the DMC to enter selected DC states (required around modesets and Aux
A). Disabling the power well will allow DC states again. For now the
highest DC state is DC6 but will be configurable in a later patch.

Signed-off-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/i915_drv.c |   6 --
 drivers/gpu/drm/i915/intel_display.c|   6 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 107 +---
 3 files changed, 78 insertions(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 37319b0..e190237 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1069,9 +1069,6 @@ static int skl_suspend_complete(struct drm_i915_private 
*dev_priv)
 {
skl_uninit_cdclk(dev_priv);
 
-   if (dev_priv->csr.dmc_payload)
-   skl_enable_dc6(dev_priv);
-
return 0;
 }
 
@@ -1116,9 +1113,6 @@ static int bxt_resume_prepare(struct drm_i915_private 
*dev_priv)
 
 static int skl_resume_prepare(struct drm_i915_private *dev_priv)
 {
-   if (dev_priv->csr.dmc_payload)
-   skl_disable_dc6(dev_priv);
-
skl_init_cdclk(dev_priv);
intel_csr_load_program(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index c6d60b8..e401871 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13296,6 +13296,9 @@ static int intel_atomic_commit(struct drm_device *dev,
to_intel_crtc_state(crtc->state)->update_pipe;
unsigned long put_domains = 0;
 
+   if (modeset)
+   intel_display_power_get(dev_priv, POWER_DOMAIN_MODESET);
+
if (modeset && crtc->state->active) {
update_scanline_offset(to_intel_crtc(crtc));
dev_priv->display.crtc_enable(crtc);
@@ -13319,6 +13322,9 @@ static int intel_atomic_commit(struct drm_device *dev,
modeset_put_power_domains(dev_priv, put_domains);
 
intel_post_plane_update(intel_crtc);
+
+   if (modeset)
+   intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET);
}
 
/* FIXME: add subpixel order */
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index c901b19..042d92f 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -49,9 +49,6 @@
  * present for a given platform.
  */
 
-#define GEN9_ENABLE_DC5(dev) 0
-#define SKL_ENABLE_DC6(dev) IS_SKYLAKE(dev)
-
 #define for_each_power_well(i, power_well, domain_mask, power_domains) \
for (i = 0; \
 i < (power_domains)->power_well_count &&   \
@@ -336,10 +333,15 @@ static void hsw_set_power_well(struct drm_i915_private 
*dev_priv,
SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS | \
BIT(POWER_DOMAIN_PLLS) |\
BIT(POWER_DOMAIN_INIT))
+#define SKL_DISPLAY_DC_OFF_POWER_DOMAINS ( \
+   BIT(POWER_DOMAIN_MODESET) | \
+   BIT(POWER_DOMAIN_AUX_A) |   \
+   BIT(POWER_DOMAIN_INIT))
 #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (  \
(POWER_DOMAIN_MASK & ~(SKL_DISPLAY_POWERWELL_1_POWER_DOMAINS |  \
SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \
-   SKL_DISPLAY_MISC_IO_POWER_DOMAINS)) |   \
+   SKL_DISPLAY_MISC_IO_POWER_DOMAINS | \
+   SKL_DISPLAY_DC_OFF_POWER_DOMAINS)) |\
BIT(POWER_DOMAIN_INIT))
 
 #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS (\
@@ -511,20 +513,6 @@ static void gen9_enable_dc5(struct drm_i915_private 
*dev_priv)
POSTING_READ(DC_STATE_EN);
 }
 
-static void gen9_disable_dc5(struct drm_i915_private *dev_priv)
-{
-   uint32_t val;
-
-   assert_can_disable_dc5(dev_priv);
-
-   DRM_DEBUG_KMS("Disabling DC5\n");
-
-   val = I915_READ(DC_STATE_EN);
-   val &= ~DC_STATE_EN_UPTO_DC5;
-   I915_WRITE(DC_STATE_EN, val);
-   POSTING_READ(DC_STATE_EN);
-}
-
 static void assert_can_enable_dc6(struct drm_i915_private *dev_priv)
 {
struct drm_device *dev = dev_priv->dev;
@@ -553,6 +541,21 @@ static void assert_can_disable_dc6(struct drm_i915_private 
*dev_priv)
  "DC6 already programmed to be disabled.\n");
 }
 
+static void gen9_disable_dc5_dc6(struct drm_i915_private *dev_priv)
+{
+   uint32_t val;
+
+   assert_can_disable_dc5(dev_priv);
+   assert_can_disable_dc6(dev_priv);
+
+   DRM_DEBUG_KMS("Disabling DC5 and DC6\n");
+
+   val = I915_READ(DC_STATE_EN);
+   val &= ~DC_STATE_EN_UPTO_DC5_DC6_MASK;
+   I915_WRITE(DC_STATE_EN, val);
+   

[Intel-gfx] [PATCH 7/8] drm/i915/skl: Add boot parameter for disabling DC6

[Patrik Jakobsson]

Signed-off-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/i915_drv.h | 1 +
 drivers/gpu/drm/i915/i915_params.c  | 6 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
 3 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index efb6a00..9c8cb43 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2660,6 +2660,7 @@ struct i915_params {
int panel_use_ssc;
int vbt_sdvo_panel_type;
int enable_rc6;
+   int enable_dc6;
int enable_fbc;
int enable_ppgtt;
int enable_execlists;
diff --git a/drivers/gpu/drm/i915/i915_params.c 
b/drivers/gpu/drm/i915/i915_params.c
index 368df67..1a7dedf 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -32,6 +32,7 @@ struct i915_params i915 __read_mostly = {
.panel_use_ssc = -1,
.vbt_sdvo_panel_type = -1,
.enable_rc6 = -1,
+   .enable_dc6 = 1,
.enable_fbc = -1,
.enable_execlists = -1,
.enable_hangcheck = true,
@@ -79,6 +80,11 @@ MODULE_PARM_DESC(enable_rc6,
"For example, 3 would enable rc6 and deep rc6, and 7 would enable 
everything. "
"default: -1 (use per-chip default)");
 
+module_param_named(enable_dc6, i915.enable_dc6, int, 0400);
+MODULE_PARM_DESC(enable_dc6,
+   "Enable power-saving display C-state 6. "
+   "(0 = disable; 1 = enable [default])");
+
 module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, 0600);
 MODULE_PARM_DESC(enable_fbc,
"Enable frame buffer compression for power savings "
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 042d92f..4843c5c 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -753,7 +753,7 @@ static void skl_dc_off_power_well_enable(struct 
drm_i915_private *dev_priv,
 static void skl_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
  struct i915_power_well *power_well)
 {
-   if (IS_SKYLAKE(dev_priv))
+   if (IS_SKYLAKE(dev_priv) && i915.enable_dc6)
skl_enable_dc6(dev_priv);
else
gen9_enable_dc5(dev_priv);
-- 
2.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 8/8] drm/i915: Force loading of csr program at boot

[Patrik Jakobsson]

When booting (warm or cold) we must always program the csr. Previously
we checked if the CSR program memory matched with the firmware data but
this turned out to fail on warm boots.

Signed-off-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/i915_drv.c  | 2 +-
 drivers/gpu/drm/i915/intel_csr.c | 6 +++---
 drivers/gpu/drm/i915/intel_drv.h | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e190237..7e08f3f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1114,7 +1114,7 @@ static int bxt_resume_prepare(struct drm_i915_private 
*dev_priv)
 static int skl_resume_prepare(struct drm_i915_private *dev_priv)
 {
skl_init_cdclk(dev_priv);
-   intel_csr_load_program(dev_priv);
+   intel_csr_load_program(dev_priv, false);
 
return 0;
 }
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index ecb7c70..586ea55 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -210,7 +210,7 @@ static char intel_get_substepping(struct drm_device *dev)
  * Everytime display comes back from low power state this function is called to
  * copy the firmware from internal memory to registers.
  */
-void intel_csr_load_program(struct drm_i915_private *dev_priv)
+void intel_csr_load_program(struct drm_i915_private *dev_priv, bool force)
 {
u32 *payload = dev_priv->csr.dmc_payload;
uint32_t i, fw_size;
@@ -226,7 +226,7 @@ void intel_csr_load_program(struct drm_i915_private 
*dev_priv)
 * Unfortunately the ACPI subsystem doesn't yet give us a way to
 * differentiate this, hence figure it out with this hack.
 */
-   if ((!dev_priv->csr.dmc_payload) || I915_READ(CSR_PROGRAM(0)))
+   if (!force && (!dev_priv->csr.dmc_payload || I915_READ(CSR_PROGRAM(0
return;
 
fw_size = dev_priv->csr.dmc_fw_size;
@@ -384,7 +384,7 @@ static void csr_load_work_fn(struct work_struct *work)
goto out;
 
/* load csr program during system boot, as needed for DC states */
-   intel_csr_load_program(dev_priv);
+   intel_csr_load_program(dev_priv, true);
 
 out:
if (dev_priv->csr.dmc_payload) {
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d110555..58cd63a3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1208,7 +1208,7 @@ u32 skl_plane_ctl_rotation(unsigned int rotation);
 
 /* intel_csr.c */
 void intel_csr_ucode_init(struct drm_i915_private *);
-void intel_csr_load_program(struct drm_i915_private *);
+void intel_csr_load_program(struct drm_i915_private *, bool);
 void intel_csr_ucode_fini(struct drm_i915_private *);
 
 /* intel_dp.c */
-- 
2.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 7/8] drm/i915/skl: Add boot parameter for disabling DC6

[Jani Nikula]

On Tue, 03 Nov 2015, Patrik Jakobsson  wrote:
> Signed-off-by: Patrik Jakobsson 
> ---
>  drivers/gpu/drm/i915/i915_drv.h | 1 +
>  drivers/gpu/drm/i915/i915_params.c  | 6 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
>  3 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index efb6a00..9c8cb43 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2660,6 +2660,7 @@ struct i915_params {
>   int panel_use_ssc;
>   int vbt_sdvo_panel_type;
>   int enable_rc6;
> + int enable_dc6;
>   int enable_fbc;
>   int enable_ppgtt;
>   int enable_execlists;
> diff --git a/drivers/gpu/drm/i915/i915_params.c 
> b/drivers/gpu/drm/i915/i915_params.c
> index 368df67..1a7dedf 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -32,6 +32,7 @@ struct i915_params i915 __read_mostly = {
>   .panel_use_ssc = -1,
>   .vbt_sdvo_panel_type = -1,
>   .enable_rc6 = -1,
> + .enable_dc6 = 1,
>   .enable_fbc = -1,
>   .enable_execlists = -1,
>   .enable_hangcheck = true,
> @@ -79,6 +80,11 @@ MODULE_PARM_DESC(enable_rc6,
>   "For example, 3 would enable rc6 and deep rc6, and 7 would enable 
> everything. "
>   "default: -1 (use per-chip default)");
>  
> +module_param_named(enable_dc6, i915.enable_dc6, int, 0400);

_unsafe please.

BR,
Jani.

> +MODULE_PARM_DESC(enable_dc6,
> + "Enable power-saving display C-state 6. "
> + "(0 = disable; 1 = enable [default])");
> +
>  module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, 0600);
>  MODULE_PARM_DESC(enable_fbc,
>   "Enable frame buffer compression for power savings "
> diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index 042d92f..4843c5c 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -753,7 +753,7 @@ static void skl_dc_off_power_well_enable(struct 
> drm_i915_private *dev_priv,
>  static void skl_dc_off_power_well_disable(struct drm_i915_private *dev_priv,
> struct i915_power_well *power_well)
>  {
> - if (IS_SKYLAKE(dev_priv))
> + if (IS_SKYLAKE(dev_priv) && i915.enable_dc6)
>   skl_enable_dc6(dev_priv);
>   else
>   gen9_enable_dc5(dev_priv);

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] RFC drm/i915: Slaughter the thundering i915_wait_request herd

[Gong, Zhipeng]

> From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> 
> Do you also have a relative perf statistics like op/s we can compare to make
> sure we aren't just stalling the whole system?
> 
Could you please provide the commands about how to check it?
> 
> How much cpu time is left in the i915_wait_request branch? i.e. how close to
> the limit are we with chasing this path?
Could you please provide the commands here either? :)

-Zhipeng

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Ville Syrjälä]

On Tue, Nov 03, 2015 at 01:00:32PM +0100, Maarten Lankhorst wrote:
> Op 03-11-15 om 11:40 schreef Ville Syrjälä:
> > On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote:
> >> Op 03-11-15 om 10:09 schreef Ville Syrjälä:
> >>> On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
>  This fixes a warning when the crtc is turned off. In that case fb
>  will be NULL, and crtc_clock will be 0. Because the crtc is no longer
>  active this is not a bug, and shouldn't trigger the WARN_ON.
> >>> Mm. We want to do scaling checks and whatnot during DPMS. So this should
> >>> check .enabled, no?
> >> Not sure what the right decision would be here..
> >>
> >>  * skl max scale is lower of:
> >>  *close to 3 but not 3, -1 is for that purpose
> >>  *or
> >>  *cdclk/crtc_clock
> >>
> >> So when multiple pipes are enabled potentially 3x scaling is allowed, but 
> >> if you dpms them all off
> >> cdclk might get set to 0. This means a previous valid amount of scaling 
> >> might suddenly become invalid.
> >>
> >> Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting.
> > I think we should keep around the min required cdclk in the crtc state.
> > And we recalculate that one every time anything changes. Then we can
> > just compare it against the current cdclk after plane/crtc states have
> > otherwise been computed to see if we need to change the current cdclk.
> >
> What would the use be here? In that case the above formula reduces to 1<<16.
> If you want to treat dpms off the same as dpms on then you need the separate 
> cdclk's..

I don't understand what you're saying. We need to calculate the required
cdclk based on dpms on. And that is what we must check against the
hardware limit. If we then want to optimize the dpms off case we can
just do 'cdclk = state->active ? state->cdclk : 0'. We probably want to
do that when all pipes are in dpms off. But if we have some pipes in
dpms off and some in dpms on, we may want to skip the 'active' check
for all pipes (ie. behave as all pipes are dpms on), just to avoid flicker
when some of the remaining pipes transition from dpms off to dpms on.

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] RFC drm/i915: Slaughter the thundering i915_wait_request herd

[Chris Wilson]

On Tue, Nov 03, 2015 at 03:06:36AM +, Gong, Zhipeng wrote:
> It seems that there are some gaps in the patch and first patch. 
> Like there is no this line in the first patch. 
>   if (req->ring->seqno_barrier)

Ah, that was in the context I hope...

> I have tried to apply this patch. And here is the cpu utilization and perf 
> data on BDW

Do you also have a relative perf statistics like op/s we can compare to
make sure we aren't just stalling the whole system?
 
> CPU util  |   w/o patch  |   w/ patch
> --
> BDW async 1   |   116% | 95%
> BDW async 5   |   111% | 91%

How much cpu time is left in the i915_wait_request branch? i.e. how
close to the limit are we with chasing this path?
-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/kms_rotation_crc: Add a new subtest to exhaustively test for fence leaks

[Tvrtko Ursulin]

Hi,

On 03/11/15 01:25, Vivek Kasireddy wrote:

In this subtest, as a first step, MAX_FENCES+1 number of framebuffers are
created backed up by objects that have multiple GGTT views (normal and
rotated). Next, we have the i915 driver instantiate a normal view followed
by a rotated view. We continue doing the above MAX_FENCES + 1 times.

Cc: Tvrtko Ursulin 
Signed-off-by: Vivek Kasireddy 
---
  tests/kms_rotation_crc.c | 79 
  1 file changed, 79 insertions(+)

diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
index ed6eeef..44691d1 100644
--- a/tests/kms_rotation_crc.c
+++ b/tests/kms_rotation_crc.c
@@ -25,6 +25,7 @@
  #include "igt.h"
  #include 

+#define MAX_FENCES 32

  typedef struct {
int gfx_fd;
@@ -376,6 +377,78 @@ static void test_plane_rotation_ytiled_obj(data_t *data, 
enum igt_plane plane_ty
igt_assert(ret == 0);
  }

+static void test_plane_rotation_exhaust_fences(data_t *data, data_t *data2,
+  enum igt_plane plane_type)
+{
+   igt_display_t *display = >display;
+   uint64_t tiling = LOCAL_I915_FORMAT_MOD_Y_TILED;
+   uint32_t format = DRM_FORMAT_XRGB;
+   int bpp = igt_drm_format_to_bpp(format);
+   enum igt_commit_style commit = COMMIT_LEGACY;
+   int fd = data->gfx_fd;
+   igt_output_t *output = >outputs[0];
+   igt_plane_t *plane;
+   drmModeModeInfo *mode;
+   unsigned int stride, size, w, h;
+   uint32_t gem_handle;
+   int i, ret;
+
+   igt_require(output != NULL && output->valid == true);
+
+   plane = igt_output_get_plane(output, plane_type);
+   igt_require(igt_plane_supports_rotation(plane));
+
+   if (plane_type == IGT_PLANE_PRIMARY || plane_type == IGT_PLANE_CURSOR) {
+   igt_require(data->display.has_universal_planes);
+   commit = COMMIT_UNIVERSAL;
+   }
+
+   mode = igt_output_get_mode(output);
+   w = mode->hdisplay;
+   h = mode->vdisplay;
+
+   for (stride = 512; stride < (w * bpp / 8); stride *= 2)
+   ;
+   for (size = 1024*1024; size < stride * h; size *= 2)
+   ;


Looking good, just two minor comments:

I think a good addition would be to verify that we have enough GTT space 
for size * MAX_FENCES objects. Because if we don't then the test can't 
test what it is trying to test I think. So something like:


igt_require(size * MAX_FENCES < gem_total_aperture()) or whatever it is 
called. Maybe fudge down by a percentage to allow for other stuff, 10% 
or so.



+   igt_plane_set_fb(plane, NULL);
+   igt_display_commit(display);
+
+   for (i = 0; i < MAX_FENCES + 1; i++) {
+   gem_handle = gem_create(fd, size);
+   ret = __gem_set_tiling(fd, gem_handle, I915_TILING_Y, stride);
+   igt_assert(ret == 0);
+
+   do_or_die(__kms_addfb(fd, gem_handle, w, h, stride,
+ format, tiling, LOCAL_DRM_MODE_FB_MODIFIERS,
+ [i].fb.fb_id));
+   data2[i].fb.width = w;
+   data2[i].fb.height = h;
+   data2[i].fb.gem_handle = gem_handle;
+
+   igt_plane_set_fb(plane, [i].fb);
+   igt_plane_set_rotation(plane, IGT_ROTATION_0);
+
+   ret = igt_display_try_commit2(display, commit);
+   igt_assert(ret == 0);
+
+   igt_plane_set_rotation(plane, IGT_ROTATION_90);
+
+   drmModeObjectSetProperty(fd, plane->drm_plane->plane_id,
+DRM_MODE_OBJECT_PLANE,
+plane->rotation_property,
+plane->rotation);
+   ret = igt_display_try_commit2(display, commit);
+   igt_assert(ret == 0);
+   }
+
+   kmstest_restore_vt_mode();
+
+   for (i = 0; i < MAX_FENCES + 1; i++)
+   igt_remove_fb(fd, [i].fb);
+}
+
  igt_main
  {
data_t data = {};
@@ -471,6 +544,12 @@ igt_main
test_plane_rotation_ytiled_obj(, IGT_PLANE_PRIMARY);
}

+   igt_subtest_f("exhaust-fences") {
+   data_t data2[MAX_FENCES+1] = {};
+   igt_require(gen >= 9);
+   test_plane_rotation_exhaust_fences(, data2, 
IGT_PLANE_PRIMARY);


Second minor comment is that data2 could be local to 
test_plane_rotation_exhaust_fences just as well and would probably be 
cleaner like that.


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Maarten Lankhorst]

Op 03-11-15 om 11:40 schreef Ville Syrjälä:
> On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote:
>> Op 03-11-15 om 10:09 schreef Ville Syrjälä:
>>> On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
 This fixes a warning when the crtc is turned off. In that case fb
 will be NULL, and crtc_clock will be 0. Because the crtc is no longer
 active this is not a bug, and shouldn't trigger the WARN_ON.
>>> Mm. We want to do scaling checks and whatnot during DPMS. So this should
>>> check .enabled, no?
>> Not sure what the right decision would be here..
>>
>>  * skl max scale is lower of:
>>  *close to 3 but not 3, -1 is for that purpose
>>  *or
>>  *cdclk/crtc_clock
>>
>> So when multiple pipes are enabled potentially 3x scaling is allowed, but if 
>> you dpms them all off
>> cdclk might get set to 0. This means a previous valid amount of scaling 
>> might suddenly become invalid.
>>
>> Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting.
> I think we should keep around the min required cdclk in the crtc state.
> And we recalculate that one every time anything changes. Then we can
> just compare it against the current cdclk after plane/crtc states have
> otherwise been computed to see if we need to change the current cdclk.
>
What would the use be here? In that case the above formula reduces to 1<<16.
If you want to treat dpms off the same as dpms on then you need the separate 
cdclk's..
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 01/14] drm/i915: Use passed plane state for sprite planes, v3.

[Maarten Lankhorst]

Op 03-11-15 om 10:22 schreef Ville Syrjälä:
> On Tue, Nov 03, 2015 at 08:31:40AM +0100, Maarten Lankhorst wrote:
>> Don't use plane->state directly, use the pointer from commit_plane.
>>
>> Changes since v1:
>> - Fix uses of plane->state->rotation and color key to use the passed state 
>> too.
>> - Only pass crtc_state and plane_state to update_plane.
>> Changes since v2:
>> - Rebased.
>>
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/i915/intel_drv.h|  10 +--
>>  drivers/gpu/drm/i915/intel_sprite.c | 120 
>> +++-
>>  2 files changed, 67 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
>> b/drivers/gpu/drm/i915/intel_drv.h
>> index d1a6071afabe..16d4627364c5 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -647,16 +647,12 @@ struct intel_plane {
>>  /*
>>   * NOTE: Do not place new plane state fields here (e.g., when adding
>>   * new plane properties).  New runtime state should now be placed in
>> - * the intel_plane_state structure and accessed via drm_plane->state.
>> + * the intel_plane_state structure and accessed via plane_state.
>>   */
>>  
>>  void (*update_plane)(struct drm_plane *plane,
>> - struct drm_crtc *crtc,
>> - struct drm_framebuffer *fb,
>> - int crtc_x, int crtc_y,
>> - unsigned int crtc_w, unsigned int crtc_h,
>> - uint32_t x, uint32_t y,
>> - uint32_t src_w, uint32_t src_h);
>> + struct intel_crtc_state *crtc_state,
>> + struct intel_plane_state *plane_state);
> These should be const.
>
>>  void (*disable_plane)(struct drm_plane *plane,
>>struct drm_crtc *crtc);
>>  int (*check_plane)(struct drm_plane *plane,
>> diff --git a/drivers/gpu/drm/i915/intel_sprite.c 
>> b/drivers/gpu/drm/i915/intel_sprite.c
>> index 4276c135b9f2..6d3047f9c80f 100644
>> --- a/drivers/gpu/drm/i915/intel_sprite.c
>> +++ b/drivers/gpu/drm/i915/intel_sprite.c
>> @@ -178,28 +178,32 @@ void intel_pipe_update_end(struct intel_crtc *crtc)
>>  }
>>  
>>  static void
>> -skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>> - struct drm_framebuffer *fb,
>> - int crtc_x, int crtc_y,
>> - unsigned int crtc_w, unsigned int crtc_h,
>> - uint32_t x, uint32_t y,
>> - uint32_t src_w, uint32_t src_h)
>> +skl_update_plane(struct drm_plane *drm_plane,
>> + struct intel_crtc_state *crtc_state,
>> + struct intel_plane_state *plane_state)
>>  {
>>  struct drm_device *dev = drm_plane->dev;
>>  struct drm_i915_private *dev_priv = dev->dev_private;
>>  struct intel_plane *intel_plane = to_intel_plane(drm_plane);
>> +struct drm_framebuffer *fb = plane_state->base.fb;
>>  struct drm_i915_gem_object *obj = intel_fb_obj(fb);
>>  const int pipe = intel_plane->pipe;
>>  const int plane = intel_plane->plane + 1;
>>  u32 plane_ctl, stride_div, stride;
>> -const struct drm_intel_sprite_colorkey *key =
>> -_intel_plane_state(drm_plane->state)->ckey;
>> +const struct drm_intel_sprite_colorkey *key = _state->ckey;
>>  unsigned long surf_addr;
>>  u32 tile_height, plane_offset, plane_size;
>>  unsigned int rotation;
>>  int x_offset, y_offset;
>> -struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
>> -int scaler_id;
>> +
> What's with the extra newline?
>
>> +int crtc_x = plane_state->dst.x1, crtc_y = plane_state->dst.y1;
>> +uint32_t crtc_w = drm_rect_width(_state->dst);
>> +uint32_t crtc_h = drm_rect_height(_state->dst);
>> +uint32_t x = plane_state->src.x1 >> 16, y = plane_state->src.y1 >> 16;
>> +uint32_t src_w = drm_rect_width(_state->src) >> 16;
>> +uint32_t src_h = drm_rect_height(_state->src) >> 16;
>> +const struct intel_scaler *scaler =
>> +_state->scaler_state.scalers[plane_state->scaler_id];
> This looks messy. I would at least get rid of the "assign two things on
> one line thing".
>
>>  
>>  plane_ctl = PLANE_CTL_ENABLE |
>>  PLANE_CTL_PIPE_GAMMA_ENABLE |
>> @@ -208,14 +212,12 @@ skl_update_plane(struct drm_plane *drm_plane, struct 
>> drm_crtc *crtc,
>>  plane_ctl |= skl_plane_ctl_format(fb->pixel_format);
>>  plane_ctl |= skl_plane_ctl_tiling(fb->modifier[0]);
>>  
>> -rotation = drm_plane->state->rotation;
>> +rotation = plane_state->base.rotation;
>>  plane_ctl |= skl_plane_ctl_rotation(rotation);
>>  
>>  stride_div = intel_fb_stride_alignment(dev, fb->modifier[0],
>> fb->pixel_format);
>>  
>> -scaler_id = to_intel_plane_state(drm_plane->state)->scaler_id;
>> -
>>  /* Sizes are 0 based 

Re: [Intel-gfx] [PATCH] drm/915: Pad GTT views of exec objects up to user specified size

[Tvrtko Ursulin]

Hi,

On 26/10/15 17:05, Chris Wilson wrote:

On Mon, Oct 26, 2015 at 04:00:20PM +, Tvrtko Ursulin wrote:


Hi,

On 23/10/15 10:50, Tvrtko Ursulin wrote:

On 22/10/15 17:07, Chris Wilson wrote:

On Thu, Oct 22, 2015 at 03:15:55PM +0100, Tvrtko Ursulin wrote:


Hi,

On 21/10/15 16:24, Chris Wilson wrote:

Our GPUs impose certain requirements upon buffers that depend upon how
exactly they are used. Typically this is expressed as that they require
a larger surface than would be naively computed by pitch * height.
Normally such requirements are hidden away in the userspace driver, but
when we accept pointers from strangers and later impose extra
conditions
on them, the original client allocator has no idea about the
monstrosities in the GPU and we require the userspace driver to inform
the kernel how many padding pages are required beyond the client
allocation.

v2: Long time, no see


[snip]


diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 08e047cba76a..678f7d5320ae 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -691,10 +691,11 @@ struct drm_i915_gem_exec_object2 {
  #define EXEC_OBJECT_NEEDS_GTT(1<<1)
  #define EXEC_OBJECT_WRITE(1<<2)
  #define EXEC_OBJECT_SUPPORTS_48B_ADDRESS (1<<3)
-#define __EXEC_OBJECT_UNKNOWN_FLAGS
-(EXEC_OBJECT_SUPPORTS_48B_ADDRESS<<1)
+#define EXEC_OBJECT_PAD_TO_SIZE(1<<4)
+#define __EXEC_OBJECT_UNKNOWN_FLAGS -(EXEC_OBJECT_PAD_TO_SIZE<<1)
  __u64 flags;

-__u64 rsvd1;
+__u64 rsvd1; /* pad_to_size */
  __u64 rsvd2;
  };


What do you think about:

union {
__u64 pad_to_size;
__u64 rsvd1;
} ?

Kind of like a migration path for userspace?


Hmm, I think that just might work. Clang? Does clang support anonymous
unions yet? Do we care if it doesn't?


I've found some existing examples in uapi headers so think we are covered.


Any further thoughts on this? Would you consider re-spinning the
patch with this addition?


I have respun it, but haven't checked it against clang nor do I know
what others think of potential portability issues with other compilers.


As I said there are other anonymous unions in uapi headers so I think it 
should be fine.


I even think just renaming the field in the first place should be fine, 
otherwise what is the point of having reserved fields.


Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] skylake + drm-next - warn city

[Imre Deak]

On ti, 2015-11-03 at 11:42 +0200, Jani Nikula wrote:
> On Tue, 03 Nov 2015, Dave Airlie  wrote:
> > Just booted drm-next on a Skylake laptop that happened to be on my
> > desk for a few days.
> >
> > I wasn't impressed. I'm very disappointed. Doesn't anyone have any
> > pride in the code they write anymore.
> >
> > Initially the previous sentence had a lot of curse words and was Linus
> > like in it's stature, but I've been promised by twitter that being
> > nice will get me better results, so let's make it so.
> 
> Much appreciated; we get the message.
> 
> > So could someone from Intel takes some responsibility for testing the
> > code they send me actually you know works on the hardware it's meant
> > to, or at least tell me what is going so horribly wrong here.
> >
> > the lockdep trace at the end doesn't look fun.

That's from the GuC firmware loader, so won't be fixed by the DMC
patches. Ville mentioned he has a fix for that.

> Skylake wants the DMC firmware blob from linux-firmware or
> https://01.org/linuxgraphics/downloads. The conclusion from the traces
> below is that either you don't have it, or we fail to load it due to the
> deadlock.
> 
> In any case, our DMC firmware loading is, uh, less than perfect.
> 
> The bright side is that we are aware of this, and there's a couple of
> patchsets from Mika/Damien [1] and Animesh/Daniel/Imre [2] to fix
> this. (Mika, Imre, any comments on the status of those?)

All of Mika's patches [1] have an R-b, except patch 7/7.
[2] has R-b's as well except for patch 1/13. Sunil any update on that?

> The bigger question is how do we fix this for drm-next/v4.4. That's 20
> patches in total, and we are way past the cutoff. On the other hand,
> it's all restricted to Skylake DMC firmware loading, and it is supposed
> to fix stuff, not add features.

Yes, no new features, only fixes.

--Imre

> 
> Your call.
> 
> 
> BR,
> Jani.
> 
> 
> 
> [1] 
> http://mid.gmane.org/1445950025-5793-1-git-send-email-mika.kuopp...@intel.com
> [2] http://mid.gmane.org/1446069547-24760-1-git-send-email-imre.d...@intel.com
> 
> 
> >
> > Dave.
> >
> >
> > [8.158254] ACPI: Video Device [GFX0] (multi-head: yes  rom: no  post: 
> > no)
> > [8.159953] input: Video Bus as
> > /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input12
> > [8.160895] [drm] Initialized i915 1.6.0 20151010 for :00:02.0 on 
> > minor 0
> > [8.170784] [ cut here ]
> > [8.170810] WARNING: CPU: 3 PID: 103 at
> > drivers/gpu/drm/i915/intel_csr.c:481 assert_csr_loaded+0xa8/0x140
> > [i915]()
> > [8.170812] CSR is not loaded.
> > [8.170813] Modules linked in: amdkfd amd_iommu_v2 amdgpu i915 ttm
> > i2c_algo_bit drm_kms_helper serio_raw drm r8169 mii video fjes
> > [8.170825] CPU: 3 PID: 103 Comm: kworker/u16:2 Not tainted 4.3.0-rc5+ #1
> > [8.170826] Hardware name: HP HP ProBook 470 G3/8102, BIOS N78 Ver.
> > 01.01 09/04/2015
> > [8.170830] Workqueue: events_unbound async_run_entry_fn
> > [8.170832]   1aac9e2e 88024bd33a68
> > 81416e09
> > [8.170835]  88024bd33ab0 88024bd33aa0 810a8bb2
> > 88003f13
> > [8.170838]  88003f130510  300f
> > 88024ad23000
> > [8.170841] Call Trace:
> > [8.170845]  [] dump_stack+0x4b/0x72
> > [8.170847]  [] warn_slowpath_common+0x82/0xc0
> > [8.170849]  [] warn_slowpath_fmt+0x5c/0x80
> > [8.170866]  [] assert_csr_loaded+0xa8/0x140 [i915]
> > [8.170885]  [] skl_set_power_well+0x7e5/0xb00 [i915]
> > [8.170902]  [] skl_power_well_enable+0x13/0x20 [i915]
> > [8.170917]  [] intel_display_power_get+0xab/0x100 
> > [i915]
> > [8.170944]  [] intel_hdmi_set_edid+0x3b/0x110 [i915]
> > [8.170969]  [] intel_hdmi_detect+0xc0/0x130 [i915]
> > [8.170974]  []
> > drm_helper_probe_single_connector_modes_merge_bits+0x235/0x4d0
> > [drm_kms_helper]
> > [8.170978]  []
> > drm_helper_probe_single_connector_modes+0x13/0x20 [drm_kms_helper]
> > [8.170983]  []
> > drm_fb_helper_initial_config+0xb0/0x410 [drm_kms_helper]
> > [8.171007]  [] intel_fbdev_initial_config+0x1b/0x20 
> > [i915]
> > [8.171009]  [] async_run_entry_fn+0x4a/0x140
> > [8.171011]  [] process_one_work+0x230/0x680
> > [8.171013]  [] ? process_one_work+0x199/0x680
> > [8.171015]  [] worker_thread+0x4e/0x450
> > [8.171017]  [] ? process_one_work+0x680/0x680
> > [8.171020]  [] kthread+0x101/0x120
> > [8.171023]  [] ? trace_hardirqs_on_caller+0x129/0x1b0
> > [8.171026]  [] ? kthread_create_on_node+0x250/0x250
> > [8.171028]  [] ret_from_fork+0x3f/0x70
> > [8.171031]  [] ? kthread_create_on_node+0x250/0x250
> > [8.171032] ---[ end trace 4692db411b428244 ]---
> > [8.171035] [ cut here ]
> > [8.171053] WARNING: CPU: 3 PID: 103 at
> > drivers/gpu/drm/i915/intel_csr.c:484 assert_csr_loaded+0x103/0x140
> > [i915]()
> > [

Re: [Intel-gfx] [PATCH] drm/i915: Ensure associated VMAs are inactive when contexts are destroyed

[Tvrtko Ursulin]

On 26/10/15 13:10, Tvrtko Ursulin wrote:


On 26/10/15 12:10, Chris Wilson wrote:

On Mon, Oct 26, 2015 at 12:00:06PM +, Tvrtko Ursulin wrote:


On 26/10/15 11:23, Chris Wilson wrote:

On Mon, Oct 26, 2015 at 11:05:03AM +, Tvrtko Ursulin wrote:

From: Tvrtko Ursulin 

In the following commit:

  commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae
  Author: Tvrtko Ursulin 
  Date:   Mon Oct 5 13:26:36 2015 +0100

  drm/i915: Clean up associated VMAs on context destruction

I added a WARN_ON assertion that VM's active list must be empty
at the time of owning context is getting freed, but that turned
out to be a wrong assumption.

Due ordering of operations in i915_gem_object_retire__read, where
contexts are unreferenced before VMAs are moved to the inactive
list, the described situation can in fact happen.


The context is being unreferenced indirectly. Adding a direct reference
here is even more bizarre.


Perhaps is not the prettiest, but it sounds logical to me to ensure
that order of destruction of involved object hierarchy goes from the
bottom-up and is not interleaved.

If you consider the active/inactive list position as part of the
retire process, doing it at the very place in code, and the very
object that looked to be destroyed out of sequence, to me sounded
logical.

How would you do it, can you think of a better way?


The reference is via the request. We are handling requests, it makes
more sense that you take the reference on the request.


Hm, so you would be happy with:

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 9b2048c7077d..c238481a8090 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2373,19 +2373,26 @@ static void
  i915_gem_object_retire__read(struct drm_i915_gem_object *obj, int ring)
  {
 struct i915_vma *vma;
+   struct drm_i915_gem_request *req;

 RQ_BUG_ON(obj->last_read_req[ring] == NULL);
 RQ_BUG_ON(!(obj->active & (1 << ring)));

 list_del_init(>ring_list[ring]);
+
+   /* Ensure context cannot be destroyed with VMAs on the active list. */
+   req = i915_gem_request_reference(obj->last_read_req[ring]);
+
 i915_gem_request_assign(>last_read_req[ring], NULL);

 if (obj->last_write_req && obj->last_write_req->ring->id == ring)
 i915_gem_object_retire__write(obj);

 obj->active &= ~(1 << ring);
-   if (obj->active)
+   if (obj->active) {
+   i915_gem_request_unreference(req);
 return;
+   }

 /* Bump our place on the bound list to keep it roughly in LRU order
  * so that we don't steal from recently used but inactive objects
@@ -2399,6 +2406,8 @@ i915_gem_object_retire__read(struct drm_i915_gem_object 
*obj, int ring)
 list_move_tail(>mm_list, >vm->inactive_list);
 }

+   i915_gem_request_unreference(req);
+
 i915_gem_request_assign(>last_fenced_req, NULL);
 drm_gem_object_unreference(>base);
  }




Ping on this?

Regards,

Tvrtko
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 14/14] drm/i915/skl: Do not allow scaling when crtc is disabled.

[Ville Syrjälä]

On Tue, Nov 03, 2015 at 11:06:53AM +0100, Maarten Lankhorst wrote:
> Op 03-11-15 om 10:09 schreef Ville Syrjälä:
> > On Tue, Nov 03, 2015 at 08:31:53AM +0100, Maarten Lankhorst wrote:
> >> This fixes a warning when the crtc is turned off. In that case fb
> >> will be NULL, and crtc_clock will be 0. Because the crtc is no longer
> >> active this is not a bug, and shouldn't trigger the WARN_ON.
> > Mm. We want to do scaling checks and whatnot during DPMS. So this should
> > check .enabled, no?
> Not sure what the right decision would be here..
> 
>  * skl max scale is lower of:
>  *close to 3 but not 3, -1 is for that purpose
>  *or
>  *cdclk/crtc_clock
> 
> So when multiple pipes are enabled potentially 3x scaling is allowed, but if 
> you dpms them all off
> cdclk might get set to 0. This means a previous valid amount of scaling might 
> suddenly become invalid.
> 
> Maybe the fix is keeping 2 cdclk's, one for scaling and one for setting.

I think we should keep around the min required cdclk in the crtc state.
And we recalculate that one every time anything changes. Then we can
just compare it against the current cdclk after plane/crtc states have
otherwise been computed to see if we need to change the current cdclk.

> 
> This probably needs to be addressed in patch 3/14 then, so that one needs 
> more love.
> 
> I will resend patch 3, 4, 5 and 14 as a separate series. The other patches 
> from this series will still work with some easily fixed rejects.
> >> Also remove handling a null crtc_state, with all transitional helpers
> >> gone this can no longer happen.
> > What about the !intel_crtc check, how is that supposed to happen?
> When fb == NULL. :)
> >> Signed-off-by: Maarten Lankhorst 
> >> ---
> >>  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 304e1028c9a4..7e2caeef9a11 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -13671,7 +13671,7 @@ skl_max_scale(struct intel_crtc *intel_crtc, 
> >> struct intel_crtc_state *crtc_state
> >>struct drm_i915_private *dev_priv;
> >>int crtc_clock, cdclk;
> >>  
> >> -  if (!intel_crtc || !crtc_state)
> >> +  if (!intel_crtc || !crtc_state->base.active)
> >>return DRM_PLANE_HELPER_NO_SCALING;
> >>  
> >>dev = intel_crtc->base.dev;
> >> -- 
> >> 2.1.0
> >>
> >> ___
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Ensure associated VMAs are inactive when contexts are destroyed

[Chris Wilson]

On Mon, Oct 26, 2015 at 01:10:19PM +, Tvrtko Ursulin wrote:
> 
> On 26/10/15 12:10, Chris Wilson wrote:
> > On Mon, Oct 26, 2015 at 12:00:06PM +, Tvrtko Ursulin wrote:
> >>
> >> On 26/10/15 11:23, Chris Wilson wrote:
> >>> On Mon, Oct 26, 2015 at 11:05:03AM +, Tvrtko Ursulin wrote:
>  From: Tvrtko Ursulin 
> 
>  In the following commit:
> 
>   commit e9f24d5fb7cf3628b195b18ff3ac4e37937ceeae
>   Author: Tvrtko Ursulin 
>   Date:   Mon Oct 5 13:26:36 2015 +0100
> 
>   drm/i915: Clean up associated VMAs on context destruction
> 
>  I added a WARN_ON assertion that VM's active list must be empty
>  at the time of owning context is getting freed, but that turned
>  out to be a wrong assumption.
> 
>  Due ordering of operations in i915_gem_object_retire__read, where
>  contexts are unreferenced before VMAs are moved to the inactive
>  list, the described situation can in fact happen.
> >>>
> >>> The context is being unreferenced indirectly. Adding a direct reference
> >>> here is even more bizarre.
> >>
> >> Perhaps is not the prettiest, but it sounds logical to me to ensure
> >> that order of destruction of involved object hierarchy goes from the
> >> bottom-up and is not interleaved.
> >>
> >> If you consider the active/inactive list position as part of the
> >> retire process, doing it at the very place in code, and the very
> >> object that looked to be destroyed out of sequence, to me sounded
> >> logical.
> >>
> >> How would you do it, can you think of a better way?
> > 
> > The reference is via the request. We are handling requests, it makes
> > more sense that you take the reference on the request.
> 
> Hm, so you would be happy with:

Go up another level. There is just one callsite where the reference
needs to be added across the call.

And no, I would not be happy as I see this as just futher increasing the
technical debt.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 4/4] drm/i915: Add two-stage ILK-style watermark programming (v7)

[Matt Roper]

In addition to calculating final watermarks, let's also pre-calculate a
set of intermediate watermark values at atomic check time.  These
intermediate watermarks are a combination of the watermarks for the old
state and the new state; they should satisfy the requirements of both
states which means they can be programmed immediately when we commit the
atomic state (without waiting for a vblank).  Once the vblank does
happen, we can then re-program watermarks to the more optimal final
value.

v2: Significant rebasing/rewriting.

v3:
 - Move 'need_postvbl_update' flag to CRTC state (Daniel)
 - Don't forget to check intermediate watermark values for validity
   (Maarten)
 - Don't due async watermark optimization; just do it at the end of the
   atomic transaction, after waiting for vblanks.  We do want it to be
   async eventually, but adding that now will cause more trouble for
   Maarten's in-progress work.  (Maarten)
 - Don't allocate space in crtc_state for intermediate watermarks on
   platforms that don't need it (gen9+).
 - Move WaCxSRDisabledForSpriteScaling:ivb into intel_begin_crtc_commit
   now that ilk_update_wm is gone.

v4:
 - Add a wm_mutex to cover updates to intel_crtc->active and the
   need_postvbl_update flag.  Since we don't have async yet it isn't
   terribly important yet, but might as well add it now.
 - Change interface to program watermarks.  Platforms will now expose
   .initial_watermarks() and .optimize_watermarks() functions to do
   watermark programming.  These should lock wm_mutex, copy the
   appropriate state values into intel_crtc->active, and then call
   the internal program watermarks function.

v5:
 - Skip intermediate watermark calculation/check during initial hardware
   readout since we don't trust the existing HW values (and don't have
   valid values of our own yet).
 - Don't try to call .optimize_watermarks() on platforms that don't have
   atomic watermarks yet.  (Maarten)

v6:
 - Rebase

v7:
 - Further rebase

Signed-off-by: Matt Roper 
Reviewed-by(v5): Maarten Lankhorst 
---
 drivers/gpu/drm/i915/i915_drv.h  |   6 +-
 drivers/gpu/drm/i915/intel_atomic.c  |   1 +
 drivers/gpu/drm/i915/intel_display.c |  90 +++-
 drivers/gpu/drm/i915/intel_drv.h |  31 ++-
 drivers/gpu/drm/i915/intel_pm.c  | 160 ---
 5 files changed, 232 insertions(+), 56 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 09807c8..e9a07d8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -629,7 +629,11 @@ struct drm_i915_display_funcs {
  struct dpll *best_clock);
int (*compute_pipe_wm)(struct intel_crtc *crtc,
   struct drm_atomic_state *state);
-   void (*program_watermarks)(struct intel_crtc_state *cstate);
+   int (*compute_intermediate_wm)(struct drm_device *dev,
+  struct intel_crtc *intel_crtc,
+  struct intel_crtc_state *newstate);
+   void (*initial_watermarks)(struct intel_crtc_state *cstate);
+   void (*optimize_watermarks)(struct intel_crtc_state *cstate);
void (*update_wm)(struct drm_crtc *crtc);
int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/i915/intel_atomic.c 
b/drivers/gpu/drm/i915/intel_atomic.c
index 643f342..b91e166 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -95,6 +95,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
 
crtc_state->update_pipe = false;
crtc_state->disable_lp_wm = false;
+   crtc_state->wm.need_postvbl_update = false;
 
return _state->base;
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index e289311..29ad378 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11659,6 +11659,12 @@ int intel_plane_atomic_calc_changes(struct 
drm_crtc_state *crtc_state,
intel_crtc->atomic.update_wm_pre = true;
}
 
+   /* Pre-gen9 platforms need two-step watermark updates */
+   if ((intel_crtc->atomic.update_wm_pre || 
intel_crtc->atomic.update_wm_post) &&
+   INTEL_INFO(dev)->gen < 9 &&
+   dev_priv->display.optimize_watermarks)
+   to_intel_crtc_state(crtc_state)->wm.need_postvbl_update = true;
+
if (visible || was_visible)
intel_crtc->atomic.fb_bits |=
to_intel_plane(plane)->frontbuffer_bit;
@@ -11815,8 +11821,29 @@ static int intel_crtc_atomic_check(struct drm_crtc 
*crtc,
ret = 0;
if (dev_priv->display.compute_pipe_wm) {
ret = dev_priv->display.compute_pipe_wm(intel_crtc, state);
-

[Intel-gfx] [PATCH 3/4] drm/i915: Sanitize watermarks after hardware state readout (v2)

[Matt Roper]

Although we can do a good job of reading out hardware state, the
graphics firmware may have programmed the watermarks in a creative way
that doesn't match how i915 would have chosen to program them.  We
shouldn't trust the firmware's watermark programming, but should rather
re-calculate how we think WM's should be programmed and then shove those
values into the hardware.

We can do this pretty easily by creating a dummy top-level state,
running it through the check process to calculate all the values, and
then just programming the watermarks for each CRTC.

v2:  Move watermark sanitization after our BIOS fb reconstruction; the
 watermark calculations that we do here need to look at pstate->fb,
 which isn't setup yet in intel_modeset_setup_hw_state(), even
 though we have an enabled & visible plane.

Cc: Jani Nikula 
Cc: Maarten Lankhorst 
Signed-off-by: Matt Roper 
---
Jani, based on your logs it looks like the culprit is that we're calculating
watermarks at startup time after we've read out preliminary hardware state (so
we know the primary plane is enabled & visible), but before we reconstruct the
BIOS fb (so pstate->fb is NULL).  I think moving the watermark sanitization
later in the process so that we'll have a proper fb setup should hopefully
solve the issue.  Can you test this version when you get a chance?  I'll also
send a rebased patch #4 since the code movement here means that the previous
version won't apply cleanly.

 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/intel_display.c | 55 
 drivers/gpu/drm/i915/intel_pm.c  | 14 +
 3 files changed, 64 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20cd6d8..09807c8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -629,6 +629,7 @@ struct drm_i915_display_funcs {
  struct dpll *best_clock);
int (*compute_pipe_wm)(struct intel_crtc *crtc,
   struct drm_atomic_state *state);
+   void (*program_watermarks)(struct intel_crtc_state *cstate);
void (*update_wm)(struct drm_crtc *crtc);
int (*modeset_calc_cdclk)(struct drm_atomic_state *state);
void (*modeset_commit_cdclk)(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 7b3cfb6..e289311 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14936,6 +14936,54 @@ void intel_modeset_init_hw(struct drm_device *dev)
intel_enable_gt_powersave(dev);
 }
 
+/*
+ * Calculate what we think the watermarks should be for the state we've read
+ * out of the hardware and then immediately program those watermarks so that
+ * we ensure the hardware settings match our internal state.
+ */
+static void sanitize_watermarks(struct drm_device *dev)
+{
+   struct drm_i915_private *dev_priv = to_i915(dev);
+   struct drm_atomic_state *state;
+   struct drm_crtc *crtc;
+   struct drm_crtc_state *cstate;
+   int ret;
+   int i;
+
+   /* Only supported on platforms that use atomic watermark design */
+   if (!dev_priv->display.program_watermarks)
+   return;
+
+   /*
+* Calculate what we think WM's should be by creating a dummy state and
+* running it through the atomic check code.
+*/
+   state = drm_atomic_helper_duplicate_state(dev,
+ dev->mode_config.acquire_ctx);
+   if (WARN_ON(IS_ERR(state)))
+   return;
+
+   ret = intel_atomic_check(dev, state);
+   if (ret) {
+   /*
+* Just give up and leave watermarks untouched if we get an
+* error back from 'check'
+*/
+   DRM_DEBUG_KMS("Could not determine valid watermarks for 
inherited state\n");
+   return;
+   }
+
+   /* Write calculated watermark values back */
+   to_i915(dev)->wm.config = to_intel_atomic_state(state)->wm_config;
+   for_each_crtc_in_state(state, crtc, cstate, i) {
+   struct intel_crtc_state *cs = to_intel_crtc_state(cstate);
+
+   dev_priv->display.program_watermarks(cs);
+   }
+
+   drm_atomic_state_free(state);
+}
+
 void intel_modeset_init(struct drm_device *dev)
 {
struct drm_i915_private *dev_priv = dev->dev_private;
@@ -15059,6 +15107,13 @@ void intel_modeset_init(struct drm_device *dev)
 */
intel_find_initial_plane_obj(crtc, _config);
}
+
+   /*
+* Make sure hardware watermarks really match the state we read out.
+* Note that we need to do this after reconstructing the BIOS fb's
+* since the watermark calculation done here will 

Re: [Intel-gfx] [PATCH v2 10/14] drm/i915: Remove update_sprite_watermarks.

[Matt Roper]

On Tue, Nov 03, 2015 at 08:31:49AM +0100, Maarten Lankhorst wrote:
> Commit 791a32be6eb2 ("drm/i915: Drop intel_update_sprite_watermarks")
> removes the use of this variable, but forgot to remove it.
> 
> Cc: Matt Roper 
> Signed-off-by: Maarten Lankhorst 

Reviewed-by: Matt Roper 

> ---
>  drivers/gpu/drm/i915/intel_display.c | 6 +-
>  drivers/gpu/drm/i915/intel_drv.h | 1 -
>  2 files changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c 
> b/drivers/gpu/drm/i915/intel_display.c
> index b1d6ce80daaf..77fe1d75404b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11632,7 +11632,6 @@ int intel_plane_atomic_calc_changes(struct 
> drm_crtc_state *crtc_state,
>   struct intel_plane_state *old_plane_state =
>   to_intel_plane_state(plane->state);
>   int idx = intel_crtc->base.base.id, ret;
> - int i = drm_plane_index(plane);
>   bool mode_changed = needs_modeset(crtc_state);
>   bool was_crtc_enabled = crtc->state->active;
>   bool is_crtc_enabled = crtc_state->active;
> @@ -11726,11 +11725,8 @@ int intel_plane_atomic_calc_changes(struct 
> drm_crtc_state *crtc_state,
>*/
>   if (IS_IVYBRIDGE(dev) &&
>   needs_scaling(to_intel_plane_state(plane_state)) &&
> - !needs_scaling(old_plane_state)) {
> + !needs_scaling(old_plane_state))
>   pipe_config->disable_lp_wm = true;
> - } else if (turn_off && !mode_changed)
> - intel_crtc->atomic.update_sprite_watermarks |=
> - 1 << i;
>  
>   break;
>   }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index 8d86627069e5..dc024c27ca1e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -542,7 +542,6 @@ struct intel_crtc_atomic_commit {
>   unsigned fb_bits;
>   bool update_fbc;
>   bool post_enable_primary;
> - unsigned update_sprite_watermarks;
>  };
>  
>  struct intel_crtc {
> -- 
> 2.1.0
> 

-- 
Matt Roper
Graphics Software Engineer
IoTG Platform Enabling & Development
Intel Corporation
(916) 356-2795
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/7] drm/i915/skl: Refuse to load outdated dmc firmware

[Daniel Stone]

Hi Mika,

On 30 October 2015 at 15:52, Mika Kuoppala
 wrote:
> There is known issue on GT interrupt delivery with DC6 and
> firmwares <1.21. There is a suspicion that this causes
> spurious gpu hangs on driver init and with some workloads,
> as upgrading the firmware to 1.21 makes these problems
> disappear.
>
> As of now the current version included in distribution
> firmware packages is very like to be 1.19. Play it safe and
> refuse to load a firmware version that may affect gpu
> side stability.
>
> With < 1.23 there is a palette and dmc ram corruption issue
> so blacklist anything below that.

Unfortunately 1.23 is only available from 01.org, and doesn't appear
to have been submitted to linux-firmware. Rodrigo, is this going to be
submitted soon, or?

Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] skylake + drm-next - warn city

[Daniel Stone]

Hi Patrik,

On 3 November 2015 at 21:21, Patrik Jakobsson
 wrote:
> On Tue, Nov 3, 2015 at 11:35 AM, Imre Deak  wrote:
>> All of Mika's patches [1] have an R-b, except patch 7/7.
>> [2] has R-b's as well except for patch 1/13. Sunil any update on that?
>
> It could be that Dave is hitting the warm boot DMC fw loading issue so
> you probably need my series [1] as well.

It's necessary but not sufficient; see
http://lists.freedesktop.org/archives/intel-gfx/2015-November/079368.html
for fixing GuC itself.

I tested this on top of the three series (Mika's, Imre's, and yours),
but hit some power domain warnings and I never come back from DPMS.
That being said, this is also on top of Maarten's async atomic
pageflip series, so there could be some damage there. I'll try to
track that down separately and give you a more sensible Tested-by.

My other reservation with the DMC firmware series is that it requires
firmware v1.23, which is available from 01.org, but hasn't been
submitted to linux-firmware AFAICT. So that'll break DMC for everyone
who doesn't explicitly grab the firmware ...

Cheers,
Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] skylake + drm-next - warn city

[Patrik Jakobsson]

On Tue, Nov 3, 2015 at 10:47 PM, Daniel Stone  wrote:
> Hi Patrik,
>
> On 3 November 2015 at 21:21, Patrik Jakobsson
>  wrote:
>> On Tue, Nov 3, 2015 at 11:35 AM, Imre Deak  wrote:
>>> All of Mika's patches [1] have an R-b, except patch 7/7.
>>> [2] has R-b's as well except for patch 1/13. Sunil any update on that?
>>
>> It could be that Dave is hitting the warm boot DMC fw loading issue so
>> you probably need my series [1] as well.
>
> It's necessary but not sufficient; see
> http://lists.freedesktop.org/archives/intel-gfx/2015-November/079368.html
> for fixing GuC itself.

Hi Daniel

Yes, the GuC is a separate issue from the DMC ones. I'll have a look
at your patch.

>
> I tested this on top of the three series (Mika's, Imre's, and yours),
> but hit some power domain warnings and I never come back from DPMS.
> That being said, this is also on top of Maarten's async atomic
> pageflip series, so there could be some damage there. I'll try to
> track that down separately and give you a more sensible Tested-by.

It's possible that our changes collide. I'm not up-to-date with the
latest atomic work but can take a look as well tomorrow.

Thanks
Patrik

>
> My other reservation with the DMC firmware series is that it requires
> firmware v1.23, which is available from 01.org, but hasn't been
> submitted to linux-firmware AFAICT. So that'll break DMC for everyone
> who doesn't explicitly grab the firmware ...
>
> Cheers,
> Daniel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] skylake + drm-next - warn city

[Dave Airlie]

On 3 November 2015 at 20:47, Ville Syrjälä
 wrote:
> On Tue, Nov 03, 2015 at 12:35:58PM +0200, Imre Deak wrote:
>> On ti, 2015-11-03 at 11:42 +0200, Jani Nikula wrote:
>> > On Tue, 03 Nov 2015, Dave Airlie  wrote:
>> > > Just booted drm-next on a Skylake laptop that happened to be on my
>> > > desk for a few days.
>> > >
>> > > I wasn't impressed. I'm very disappointed. Doesn't anyone have any
>> > > pride in the code they write anymore.
>> > >
>> > > Initially the previous sentence had a lot of curse words and was Linus
>> > > like in it's stature, but I've been promised by twitter that being
>> > > nice will get me better results, so let's make it so.
>> >
>> > Much appreciated; we get the message.
>> >
>> > > So could someone from Intel takes some responsibility for testing the
>> > > code they send me actually you know works on the hardware it's meant
>> > > to, or at least tell me what is going so horribly wrong here.
>> > >
>> > > the lockdep trace at the end doesn't look fun.
>>
>> That's from the GuC firmware loader, so won't be fixed by the DMC
>> patches. Ville mentioned he has a fix for that.
>
> No, I don't. I probably just mentioned that I saw this lockdep spew
> already some weeks ago, and complained about it at the time. I think
> Daniel tried to ping someone specific about it, but no idea if they
> took any notice.

From my perspective outside Intel, since the QA took a dive, I think
the pipeline is far too long now.

We've added delays to the pipeline previously to facilitate QA, but
when QA is removed we haven't shortened the pipeline again.

I'm kind of tempted to ask you guys to throw away everything queued up
in drm-intel-next on the basis that it has probably had 0 actual
testing and start again.

We have a major process failure in place here, and shoving more code
in the backend and hoping it somehow magically fixes itself between
drm-intel-next and merging to Linus's tree is clearly not working for
the past 6 months at least. I'm really unhappy about how shoddy 4.2
is, and 4.3 is clearly not shaping up to have been a winner, 4.4 is
looking even less fun.

So maybe you guys can brainstrom a bit, also when Daniel gets back.
But at the moment I think until QA is fully reestablished, I think not
merging anything to drm-intel-next for a few weeks and taking a break
on new features until some of the features that were merged broken
actually get fixed.

I'm also going to start looking at reverting skylake firmware loading,
it's clearly never been tested with lockdep enabled by anyone who
cared, which to my mind says it should never have been merged in the
first place.

Dave.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] skylake + drm-next - warn city

[Patrik Jakobsson]

On Tue, Nov 3, 2015 at 11:35 AM, Imre Deak  wrote:
> On ti, 2015-11-03 at 11:42 +0200, Jani Nikula wrote:
>> On Tue, 03 Nov 2015, Dave Airlie  wrote:
>> > Just booted drm-next on a Skylake laptop that happened to be on my
>> > desk for a few days.
>> >
>> > I wasn't impressed. I'm very disappointed. Doesn't anyone have any
>> > pride in the code they write anymore.
>> >
>> > Initially the previous sentence had a lot of curse words and was Linus
>> > like in it's stature, but I've been promised by twitter that being
>> > nice will get me better results, so let's make it so.
>>
>> Much appreciated; we get the message.
>>
>> > So could someone from Intel takes some responsibility for testing the
>> > code they send me actually you know works on the hardware it's meant
>> > to, or at least tell me what is going so horribly wrong here.
>> >
>> > the lockdep trace at the end doesn't look fun.
>
> That's from the GuC firmware loader, so won't be fixed by the DMC
> patches. Ville mentioned he has a fix for that.
>
>> Skylake wants the DMC firmware blob from linux-firmware or
>> https://01.org/linuxgraphics/downloads. The conclusion from the traces
>> below is that either you don't have it, or we fail to load it due to the
>> deadlock.
>>
>> In any case, our DMC firmware loading is, uh, less than perfect.
>>
>> The bright side is that we are aware of this, and there's a couple of
>> patchsets from Mika/Damien [1] and Animesh/Daniel/Imre [2] to fix
>> this. (Mika, Imre, any comments on the status of those?)
>
> All of Mika's patches [1] have an R-b, except patch 7/7.
> [2] has R-b's as well except for patch 1/13. Sunil any update on that?

It could be that Dave is hitting the warm boot DMC fw loading issue so
you probably need my series [1] as well.

[1] http://lists.freedesktop.org/archives/intel-gfx/2015-November/079343.html

-Patrik

>> The bigger question is how do we fix this for drm-next/v4.4. That's 20
>> patches in total, and we are way past the cutoff. On the other hand,
>> it's all restricted to Skylake DMC firmware loading, and it is supposed
>> to fix stuff, not add features.
>
> Yes, no new features, only fixes.
>
> --Imre
>
>>
>> Your call.
>>
>>
>> BR,
>> Jani.
>>
>>
>>
>> [1] 
>> http://mid.gmane.org/1445950025-5793-1-git-send-email-mika.kuopp...@intel.com
>> [2] 
>> http://mid.gmane.org/1446069547-24760-1-git-send-email-imre.d...@intel.com
>>
>>
>> >
>> > Dave.
>> >
>> >
>> > [8.158254] ACPI: Video Device [GFX0] (multi-head: yes  rom: no  post: 
>> > no)
>> > [8.159953] input: Video Bus as
>> > /devices/LNXSYSTM:00/LNXSYBUS:00/PNP0A08:00/LNXVIDEO:00/input/input12
>> > [8.160895] [drm] Initialized i915 1.6.0 20151010 for :00:02.0 on 
>> > minor 0
>> > [8.170784] [ cut here ]
>> > [8.170810] WARNING: CPU: 3 PID: 103 at
>> > drivers/gpu/drm/i915/intel_csr.c:481 assert_csr_loaded+0xa8/0x140
>> > [i915]()
>> > [8.170812] CSR is not loaded.
>> > [8.170813] Modules linked in: amdkfd amd_iommu_v2 amdgpu i915 ttm
>> > i2c_algo_bit drm_kms_helper serio_raw drm r8169 mii video fjes
>> > [8.170825] CPU: 3 PID: 103 Comm: kworker/u16:2 Not tainted 4.3.0-rc5+ 
>> > #1
>> > [8.170826] Hardware name: HP HP ProBook 470 G3/8102, BIOS N78 Ver.
>> > 01.01 09/04/2015
>> > [8.170830] Workqueue: events_unbound async_run_entry_fn
>> > [8.170832]   1aac9e2e 88024bd33a68
>> > 81416e09
>> > [8.170835]  88024bd33ab0 88024bd33aa0 810a8bb2
>> > 88003f13
>> > [8.170838]  88003f130510  300f
>> > 88024ad23000
>> > [8.170841] Call Trace:
>> > [8.170845]  [] dump_stack+0x4b/0x72
>> > [8.170847]  [] warn_slowpath_common+0x82/0xc0
>> > [8.170849]  [] warn_slowpath_fmt+0x5c/0x80
>> > [8.170866]  [] assert_csr_loaded+0xa8/0x140 [i915]
>> > [8.170885]  [] skl_set_power_well+0x7e5/0xb00 [i915]
>> > [8.170902]  [] skl_power_well_enable+0x13/0x20 [i915]
>> > [8.170917]  [] intel_display_power_get+0xab/0x100 
>> > [i915]
>> > [8.170944]  [] intel_hdmi_set_edid+0x3b/0x110 [i915]
>> > [8.170969]  [] intel_hdmi_detect+0xc0/0x130 [i915]
>> > [8.170974]  []
>> > drm_helper_probe_single_connector_modes_merge_bits+0x235/0x4d0
>> > [drm_kms_helper]
>> > [8.170978]  []
>> > drm_helper_probe_single_connector_modes+0x13/0x20 [drm_kms_helper]
>> > [8.170983]  []
>> > drm_fb_helper_initial_config+0xb0/0x410 [drm_kms_helper]
>> > [8.171007]  [] intel_fbdev_initial_config+0x1b/0x20 
>> > [i915]
>> > [8.171009]  [] async_run_entry_fn+0x4a/0x140
>> > [8.171011]  [] process_one_work+0x230/0x680
>> > [8.171013]  [] ? process_one_work+0x199/0x680
>> > [8.171015]  [] worker_thread+0x4e/0x450
>> > [8.171017]  [] ? process_one_work+0x680/0x680
>> > [8.171020]  [] kthread+0x101/0x120
>> > [8.171023]  [] ? trace_hardirqs_on_caller+0x129/0x1b0
>> > [8.171026]  [] ? 

Re: [Intel-gfx] [PATCH 2/7] drm/i915/skl: Refuse to load outdated dmc firmware

[Vivi, Rodrigo]

On Tue, 2015-11-03 at 21:49 +, Daniel Stone wrote:
> Hi Mika,
> 
> On 30 October 2015 at 15:52, Mika Kuoppala
>  wrote:
> > There is known issue on GT interrupt delivery with DC6 and
> > firmwares <1.21. There is a suspicion that this causes
> > spurious gpu hangs on driver init and with some workloads,
> > as upgrading the firmware to 1.21 makes these problems
> > disappear.
> > 
> > As of now the current version included in distribution
> > firmware packages is very like to be 1.19. Play it safe and
> > refuse to load a firmware version that may affect gpu
> > side stability.
> > 
> > With < 1.23 there is a palette and dmc ram corruption issue
> > so blacklist anything below that.
> 
> Unfortunately 1.23 is only available from 01.org, and doesn't appear
> to have been submitted to linux-firmware. Rodrigo, is this going to 
> be
> submitted soon, or?

I submit along with the release at 01.org.
It just got pulled and merged there.


> Cheers,
> Daniel

Thanks,
Rodrigo.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Fix locking around GuC firmware load

[Dave Airlie]

On 4 November 2015 at 07:42, Daniel Stone  wrote:
> The GuC firmware load requires struct_mutex to create a GEM object,
> but this collides badly with request_firmware. Move struct_mutex
> locking down into the loader itself, so we don't hold it across the
> entire load process, including request_firmware.
>
> Signed-off-by: Daniel Stone 

This avoids the lockdep spew on my machine.

Reviewed-by: Dave AIrlie 

I'm going to put this into -next, granted I've still got the other WARN spew
but lockdep working is a good start.

Dave.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/skl: While sanitizing cdclock check the SWF18 as well

[Kumar, Shobhit]

Hi Ville,

On 11/02/2015 05:25 PM, Shobhit Kumar wrote:

SWF18 is set if the display has been intialized by the pre-os. It
also gives what configuration is enabled on which pipe. The DPLL and
CDCLK verification checks can fail as the pre-os does initialize the
DPLL for Audio codec initialization. So fisrt check if SWF18 is set and
then follow through with other DPLL and CDCLK verification.



Can you have a quick look at this one.



Cc: Ville Syrjälä 
Signed-off-by: Shobhit Kumar 
---
  drivers/gpu/drm/i915/i915_reg.h  | 3 +++
  drivers/gpu/drm/i915/intel_display.c | 8 
  2 files changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 9ee9481..bd476ff 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5006,6 +5006,9 @@ enum skl_disp_power_wells {
  #define SWF1(i)   (dev_priv->info.display_mmio_offset + 0x71410 + (i) * 4)
  #define SWF3(i)   (dev_priv->info.display_mmio_offset + 0x72414 + (i) * 4)

+/* VBIOS flag for display initialized status */
+#define GEN6_SWF18  (dev_priv->info.display_mmio_offset + 0x4F060)
+
  /* Pipe B */
  #define _PIPEBDSL (dev_priv->info.display_mmio_offset + 0x71000)
  #define _PIPEBCONF(dev_priv->info.display_mmio_offset + 0x71008)
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 103cacb..0ecb35c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5761,6 +5761,14 @@ int skl_sanitize_cdclk(struct drm_i915_private *dev_priv)
uint32_t cdctl = I915_READ(CDCLK_CTL);
int freq = dev_priv->skl_boot_cdclk;

+   /*
+* check if the pre-os intialized the display
+* There is SWF18 scratchpad register defined which is set by the
+* pre-os which can be used by the OS drivers to check the status
+*/
+   if ((I915_READ(GEN6_SWF18) & 0x00) == 0)
+   goto sanitize;
+
/* Is PLL enabled and locked ? */
if (!((lcpll1 & LCPLL_PLL_ENABLE) && (lcpll1 & LCPLL_PLL_LOCK)))
goto sanitize;



Regards
Shobhit
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] igt/kms_rotation_crc: Add a new subtest to exhaustively test for fence leaks (v2)

[Vivek Kasireddy]

In this subtest, as a first step, MAX_FENCES+1 number of framebuffers are
created backed up by objects that have multiple GGTT views (normal and
rotated). Next, we have the i915 driver instantiate a normal view followed
by a rotated view. We continue doing the above MAX_FENCES + 1 times.

v2:
- Add a igt_require() to check if there is enough GTT space left for
  MAX_FENCES+1 framebuffers. (Tvrtko)
- Make data2 local to test_plane_rotation_exhaust_fences(). (Tvrtko)
- If there is a failure, deallocate all the previously allocated
  framebuffers before asserting.

Cc: Tvrtko Ursulin 
Signed-off-by: Vivek Kasireddy 
---
 tests/kms_rotation_crc.c | 106 +++
 1 file changed, 106 insertions(+)

diff --git a/tests/kms_rotation_crc.c b/tests/kms_rotation_crc.c
index ed6eeef..154c6a1 100644
--- a/tests/kms_rotation_crc.c
+++ b/tests/kms_rotation_crc.c
@@ -25,6 +25,7 @@
 #include "igt.h"
 #include 
 
+#define MAX_FENCES 32
 
 typedef struct {
int gfx_fd;
@@ -376,6 +377,106 @@ static void test_plane_rotation_ytiled_obj(data_t *data, 
enum igt_plane plane_ty
igt_assert(ret == 0);
 }
 
+static void test_plane_rotation_exhaust_fences(data_t *data, enum igt_plane 
plane_type)
+{
+   igt_display_t *display = >display;
+   uint64_t tiling = LOCAL_I915_FORMAT_MOD_Y_TILED;
+   uint32_t format = DRM_FORMAT_XRGB;
+   int bpp = igt_drm_format_to_bpp(format);
+   enum igt_commit_style commit = COMMIT_LEGACY;
+   int fd = data->gfx_fd;
+   igt_output_t *output = >outputs[0];
+   igt_plane_t *plane;
+   drmModeModeInfo *mode;
+   data_t data2[MAX_FENCES+1] = {};
+   unsigned int stride, size, w, h;
+   uint32_t gem_handle;
+   uint64_t total_aperture_size, total_fbs_size;
+   int i, ret;
+
+   igt_require(output != NULL && output->valid == true);
+
+   plane = igt_output_get_plane(output, plane_type);
+   igt_require(igt_plane_supports_rotation(plane));
+
+   if (plane_type == IGT_PLANE_PRIMARY || plane_type == IGT_PLANE_CURSOR) {
+   igt_require(data->display.has_universal_planes);
+   commit = COMMIT_UNIVERSAL;
+   }
+
+   mode = igt_output_get_mode(output);
+   w = mode->hdisplay;
+   h = mode->vdisplay;
+
+   for (stride = 512; stride < (w * bpp / 8); stride *= 2)
+   ;
+   for (size = 1024*1024; size < stride * h; size *= 2)
+   ;
+
+   /*
+* Make sure there is atleast 90% of the available GTT space left
+* for creating (MAX_FENCES+1) framebuffers.
+*/
+   total_fbs_size = size * (MAX_FENCES + 1);
+   total_aperture_size = gem_available_aperture_size(fd);
+   igt_require(total_fbs_size < total_aperture_size * 0.9);
+
+   igt_plane_set_fb(plane, NULL);
+   igt_display_commit(display);
+
+   for (i = 0; i < MAX_FENCES + 1; i++) {
+   gem_handle = gem_create(fd, size);
+   ret = __gem_set_tiling(fd, gem_handle, I915_TILING_Y, stride);
+   if (ret) {
+   igt_warn("failed to set tiling\n");
+   goto err_alloc;
+   }
+
+   ret = (__kms_addfb(fd, gem_handle, w, h, stride,
+  format, tiling, LOCAL_DRM_MODE_FB_MODIFIERS,
+  [i].fb.fb_id));
+   if (ret) {
+   igt_warn("failed to create framebuffer\n");
+   goto err_alloc;
+   }
+
+   data2[i].fb.width = w;
+   data2[i].fb.height = h;
+   data2[i].fb.gem_handle = gem_handle;
+
+   igt_plane_set_fb(plane, [i].fb);
+   igt_plane_set_rotation(plane, IGT_ROTATION_0);
+
+   ret = igt_display_try_commit2(display, commit);
+   if (ret) {
+   igt_warn("failed to commit unrotated fb\n");
+   goto err_commit;
+   }
+
+   igt_plane_set_rotation(plane, IGT_ROTATION_90);
+
+   drmModeObjectSetProperty(fd, plane->drm_plane->plane_id,
+DRM_MODE_OBJECT_PLANE,
+plane->rotation_property,
+plane->rotation);
+   ret = igt_display_try_commit2(display, commit);
+   if (ret) {
+   igt_warn("failed to commit hardware rotated fb\n");
+   goto err_commit;
+   }
+   }
+
+err_alloc:
+   i--;
+err_commit:
+   kmstest_restore_vt_mode();
+
+   for (; i >= 0; i--)
+   igt_remove_fb(fd, [i].fb);
+
+   igt_assert(ret == 0);
+}
+
 igt_main
 {
data_t data = {};
@@ -471,6 +572,11 @@ igt_main
test_plane_rotation_ytiled_obj(, IGT_PLANE_PRIMARY);
}
 
+   igt_subtest_f("exhaust-fences") {
+   

[Intel-gfx] [PATCH] drm/i915/bxt: Fix eDP panel fitting

[Matt Roper]

BXT CRTC scaling uses the same gen9 codepaths as SKL; these codepaths
store panel fitter information in pipe_config->pch_pfit.  However since
HAS_PCH_SPLIT() is false for BXT we never actually wind up filling in
this structure (we wind up filling in pipe_config->gmch_pfit instead,
which is ignored when we go to program the hardware).  Make sure we
always take the PCH code path on gen9+ platforms.

Cc: Imre Deak 
Cc: Chandra Konduru 
Signed-off-by: Matt Roper 
---
Not a regression as far as I know; I believe this has been broken since BXT
support was added to the driver.

 drivers/gpu/drm/i915/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1cb1f3f..a8bbc2d 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1426,7 +1426,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
return ret;
}
 
-   if (!HAS_PCH_SPLIT(dev))
+   if (!HAS_PCH_SPLIT(dev) && INTEL_INFO(dev)->gen < 9)
intel_gmch_panel_fitting(intel_crtc, pipe_config,
 
intel_connector->panel.fitting_mode);
else
-- 
2.1.4

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] RFC drm/i915: Slaughter the thundering i915_wait_request herd

[Gong, Zhipeng]

> From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> On Tue, Nov 03, 2015 at 01:31:22PM +, Gong, Zhipeng wrote:
> >
> > > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> > >
> > > Do you also have a relative perf statistics like op/s we can compare
> > > to make sure we aren't just stalling the whole system?
> > >
> > Could you please provide the commands about how to check it?
> 
> I was presuming your workload has some measure of efficiency/throughput?
> It is one thing to say we are using 10% less CPU (per second), but the task is
> running 2x as long!
We use execute time as a measurement, the patch affects the execution time 
for our cases slightly.

Exec time(s)|   w/o patch   |   w/patch
---
BDW async 1 |65.00  |65.25
BDW async 5 |68.50  |66.42

> 
> > > How much cpu time is left in the i915_wait_request branch? i.e. how
> > > close to the limit are we with chasing this path?
> > Could you please provide the commands here either? :)
> 
> Check the perf callgraph.

Now the most of time is in io_schedule_timeout
__i915_wait_request  
|--64.04%-- io_schedule_timeout
|--22.04%-- intel_engine_add_wakeup
|--3.13%-- prepare_to_wait
|--2.99%-- gen6_rps_boost
|-...

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] skylake + drm-next - warn city

[Jesse Barnes]

On 11/03/2015 12:07 PM, Dave Airlie wrote:
> We have a major process failure in place here, and shoving more code
> in the backend and hoping it somehow magically fixes itself between
> drm-intel-next and merging to Linus's tree is clearly not working for
> the past 6 months at least. I'm really unhappy about how shoddy 4.2
> is, and 4.3 is clearly not shaping up to have been a winner, 4.4 is
> looking even less fun.
> 
> So maybe you guys can brainstrom a bit, also when Daniel gets back.
> But at the moment I think until QA is fully reestablished, I think not
> merging anything to drm-intel-next for a few weeks and taking a break
> on new features until some of the features that were merged broken
> actually get fixed.
> 
> I'm also going to start looking at reverting skylake firmware loading,
> it's clearly never been tested with lockdep enabled by anyone who
> cared, which to my mind says it should never have been merged in the
> first place.

I think this is the right way to go.  We have several known failures in
even our basic tests, and when we created that list the intention was to
"drop everything" if one of them failed on any of the past few platforms
(BYT+ on Atom and HSW+ on Core).  So far we haven't done that but imo we
should.

Jesse

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH] drm/i915: Fix locking around GuC firmware load

[Daniel Stone]

The GuC firmware load requires struct_mutex to create a GEM object,
but this collides badly with request_firmware. Move struct_mutex
locking down into the loader itself, so we don't hold it across the
entire load process, including request_firmware.

Signed-off-by: Daniel Stone 
---
 drivers/gpu/drm/i915/i915_dma.c | 7 +--
 drivers/gpu/drm/i915/intel_guc_loader.c | 6 --
 2 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index eddcbfd..b4c340d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -408,10 +408,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 * working irqs for e.g. gmbus and dp aux transfers. */
intel_modeset_init(dev);
 
-   /* intel_guc_ucode_init() needs the mutex to allocate GEM objects */
-   mutex_lock(>struct_mutex);
intel_guc_ucode_init(dev);
-   mutex_unlock(>struct_mutex);
 
ret = i915_gem_init(dev);
if (ret)
@@ -454,9 +451,7 @@ cleanup_gem:
i915_gem_context_fini(dev);
mutex_unlock(>struct_mutex);
 cleanup_irq:
-   mutex_lock(>struct_mutex);
intel_guc_ucode_fini(dev);
-   mutex_unlock(>struct_mutex);
drm_irq_uninstall(dev);
 cleanup_gem_stolen:
i915_gem_cleanup_stolen(dev);
@@ -1191,8 +1186,8 @@ int i915_driver_unload(struct drm_device *dev)
/* Flush any outstanding unpin_work. */
flush_workqueue(dev_priv->wq);
 
-   mutex_lock(>struct_mutex);
intel_guc_ucode_fini(dev);
+   mutex_lock(>struct_mutex);
i915_gem_cleanup_ringbuffer(dev);
i915_gem_context_fini(dev);
mutex_unlock(>struct_mutex);
diff --git a/drivers/gpu/drm/i915/intel_guc_loader.c 
b/drivers/gpu/drm/i915/intel_guc_loader.c
index ac31696..e56d19d 100644
--- a/drivers/gpu/drm/i915/intel_guc_loader.c
+++ b/drivers/gpu/drm/i915/intel_guc_loader.c
@@ -504,7 +504,9 @@ static void guc_fw_fetch(struct drm_device *dev, struct 
intel_guc_fw *guc_fw)
guc_fw->guc_fw_major_found, guc_fw->guc_fw_minor_found,
guc_fw->guc_fw_major_wanted, 
guc_fw->guc_fw_minor_wanted);
 
+   mutex_lock(>struct_mutex);
obj = i915_gem_object_create_from_data(dev, fw->data, fw->size);
+   mutex_unlock(>struct_mutex);
if (IS_ERR_OR_NULL(obj)) {
err = obj ? PTR_ERR(obj) : -ENOMEM;
goto fail;
@@ -540,8 +542,6 @@ fail:
  * @dev:   drm device
  *
  * Called early during driver load, but after GEM is initialised.
- * The device struct_mutex must be held by the caller, as we're
- * going to allocate a GEM object to hold the firmware image.
  *
  * The firmware will be transferred to the GuC's memory later,
  * when intel_guc_ucode_load() is called.
@@ -598,9 +598,11 @@ void intel_guc_ucode_fini(struct drm_device *dev)
direct_interrupts_to_host(dev_priv);
i915_guc_submission_fini(dev);
 
+   mutex_lock(>struct_mutex);
if (guc_fw->guc_fw_obj)
drm_gem_object_unreference(_fw->guc_fw_obj->base);
guc_fw->guc_fw_obj = NULL;
+   mutex_unlock(>struct_mutex);
 
guc_fw->guc_fw_fetch_status = GUC_FIRMWARE_NONE;
 }
-- 
2.5.0

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v2 02/14] drm/i915: Extend DSL readout fix to BDW and SKL.

[Maarten Lankhorst]

Hey,

Op 03-11-15 om 12:32 schreef Jani Nikula:
> On Tue, 03 Nov 2015, Ville Syrjälä  wrote:
>> On Tue, Nov 03, 2015 at 08:31:41AM +0100, Maarten Lankhorst wrote:
>>> Those platforms have the same bug as haswell, and the same fix applies to 
>>> them.
> How about Broxton? IS_DDI matches that.
>
> Jani.
>
Judging from irc it's very likely it suffers from the same problem, but it 
would be nice if we had someone who could confirm. :)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 7/8] drm/i915/skl: Add boot parameter for disabling DC6

[Patrik Jakobsson]

On Tue, Nov 3, 2015 at 3:19 PM, Jani Nikula  wrote:
> On Tue, 03 Nov 2015, Patrik Jakobsson  
> wrote:
>> On Tue, Nov 03, 2015 at 03:08:41PM +0200, Jani Nikula wrote:
>>> On Tue, 03 Nov 2015, Patrik Jakobsson  
>>> wrote:
>>> > Signed-off-by: Patrik Jakobsson 
>>> > ---
>>> >  drivers/gpu/drm/i915/i915_drv.h | 1 +
>>> >  drivers/gpu/drm/i915/i915_params.c  | 6 ++
>>> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
>>> >  3 files changed, 8 insertions(+), 1 deletion(-)
>>> >
>>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>>> > b/drivers/gpu/drm/i915/i915_drv.h
>>> > index efb6a00..9c8cb43 100644
>>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>>> > @@ -2660,6 +2660,7 @@ struct i915_params {
>>> >int panel_use_ssc;
>>> >int vbt_sdvo_panel_type;
>>> >int enable_rc6;
>>> > +  int enable_dc6;
>>> >int enable_fbc;
>>> >int enable_ppgtt;
>>> >int enable_execlists;
>>> > diff --git a/drivers/gpu/drm/i915/i915_params.c 
>>> > b/drivers/gpu/drm/i915/i915_params.c
>>> > index 368df67..1a7dedf 100644
>>> > --- a/drivers/gpu/drm/i915/i915_params.c
>>> > +++ b/drivers/gpu/drm/i915/i915_params.c
>>> > @@ -32,6 +32,7 @@ struct i915_params i915 __read_mostly = {
>>> >.panel_use_ssc = -1,
>>> >.vbt_sdvo_panel_type = -1,
>>> >.enable_rc6 = -1,
>>> > +  .enable_dc6 = 1,
>>> >.enable_fbc = -1,
>>> >.enable_execlists = -1,
>>> >.enable_hangcheck = true,
>>> > @@ -79,6 +80,11 @@ MODULE_PARM_DESC(enable_rc6,
>>> >"For example, 3 would enable rc6 and deep rc6, and 7 would enable 
>>> > everything. "
>>> >"default: -1 (use per-chip default)");
>>> >
>>> > +module_param_named(enable_dc6, i915.enable_dc6, int, 0400);
>>>
>>> _unsafe please.
>>
>> Is the convention to taint for all non-default options in i915?
>
> The intention is to taint all i915 options that people have no clue
> about and blindly set just because they read on a forum they should do
> it, and then report bugs because it fails.

This one certainly fits that description. Thanks for the explanation.

-Patrik

>
> BR,
> Jani.
>
>
>
>>
>> -Patrik
>>
>>>
>>> BR,
>>> Jani.
>>>
>>> > +MODULE_PARM_DESC(enable_dc6,
>>> > +  "Enable power-saving display C-state 6. "
>>> > +  "(0 = disable; 1 = enable [default])");
>>> > +
>>> >  module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, 0600);
>>> >  MODULE_PARM_DESC(enable_fbc,
>>> >"Enable frame buffer compression for power savings "
>>> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
>>> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> > index 042d92f..4843c5c 100644
>>> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>>> > @@ -753,7 +753,7 @@ static void skl_dc_off_power_well_enable(struct 
>>> > drm_i915_private *dev_priv,
>>> >  static void skl_dc_off_power_well_disable(struct drm_i915_private 
>>> > *dev_priv,
>>> >  struct i915_power_well *power_well)
>>> >  {
>>> > -  if (IS_SKYLAKE(dev_priv))
>>> > +  if (IS_SKYLAKE(dev_priv) && i915.enable_dc6)
>>> >skl_enable_dc6(dev_priv);
>>> >else
>>> >gen9_enable_dc5(dev_priv);
>>>
>>> --
>>> Jani Nikula, Intel Open Source Technology Center
>
> --
> Jani Nikula, Intel Open Source Technology Center
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 7/8] drm/i915/skl: Add boot parameter for disabling DC6

[Jani Nikula]

On Tue, 03 Nov 2015, Patrik Jakobsson  wrote:
> On Tue, Nov 03, 2015 at 03:08:41PM +0200, Jani Nikula wrote:
>> On Tue, 03 Nov 2015, Patrik Jakobsson  
>> wrote:
>> > Signed-off-by: Patrik Jakobsson 
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.h | 1 +
>> >  drivers/gpu/drm/i915/i915_params.c  | 6 ++
>> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
>> >  3 files changed, 8 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
>> > b/drivers/gpu/drm/i915/i915_drv.h
>> > index efb6a00..9c8cb43 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -2660,6 +2660,7 @@ struct i915_params {
>> >int panel_use_ssc;
>> >int vbt_sdvo_panel_type;
>> >int enable_rc6;
>> > +  int enable_dc6;
>> >int enable_fbc;
>> >int enable_ppgtt;
>> >int enable_execlists;
>> > diff --git a/drivers/gpu/drm/i915/i915_params.c 
>> > b/drivers/gpu/drm/i915/i915_params.c
>> > index 368df67..1a7dedf 100644
>> > --- a/drivers/gpu/drm/i915/i915_params.c
>> > +++ b/drivers/gpu/drm/i915/i915_params.c
>> > @@ -32,6 +32,7 @@ struct i915_params i915 __read_mostly = {
>> >.panel_use_ssc = -1,
>> >.vbt_sdvo_panel_type = -1,
>> >.enable_rc6 = -1,
>> > +  .enable_dc6 = 1,
>> >.enable_fbc = -1,
>> >.enable_execlists = -1,
>> >.enable_hangcheck = true,
>> > @@ -79,6 +80,11 @@ MODULE_PARM_DESC(enable_rc6,
>> >"For example, 3 would enable rc6 and deep rc6, and 7 would enable 
>> > everything. "
>> >"default: -1 (use per-chip default)");
>> >  
>> > +module_param_named(enable_dc6, i915.enable_dc6, int, 0400);
>> 
>> _unsafe please.
>
> Is the convention to taint for all non-default options in i915?

The intention is to taint all i915 options that people have no clue
about and blindly set just because they read on a forum they should do
it, and then report bugs because it fails.

BR,
Jani.



>
> -Patrik
>
>> 
>> BR,
>> Jani.
>> 
>> > +MODULE_PARM_DESC(enable_dc6,
>> > +  "Enable power-saving display C-state 6. "
>> > +  "(0 = disable; 1 = enable [default])");
>> > +
>> >  module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, 0600);
>> >  MODULE_PARM_DESC(enable_fbc,
>> >"Enable frame buffer compression for power savings "
>> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
>> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> > index 042d92f..4843c5c 100644
>> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
>> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
>> > @@ -753,7 +753,7 @@ static void skl_dc_off_power_well_enable(struct 
>> > drm_i915_private *dev_priv,
>> >  static void skl_dc_off_power_well_disable(struct drm_i915_private 
>> > *dev_priv,
>> >  struct i915_power_well *power_well)
>> >  {
>> > -  if (IS_SKYLAKE(dev_priv))
>> > +  if (IS_SKYLAKE(dev_priv) && i915.enable_dc6)
>> >skl_enable_dc6(dev_priv);
>> >else
>> >gen9_enable_dc5(dev_priv);
>> 
>> -- 
>> Jani Nikula, Intel Open Source Technology Center

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH i-g-t] tests/gem_exec_nop: Improved test run time

[Derek Morton]

Reduced the Sleep period to 200mS and reduced the repetition count to 7
to decrease the test run time significantly.

Also fixed a non ascii character that messed up the results table formatting.

Signed-off-by: Derek Morton 
---
 tests/gem_exec_nop.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
index a287d08..5028145 100644
--- a/tests/gem_exec_nop.c
+++ b/tests/gem_exec_nop.c
@@ -133,7 +133,7 @@ static void loop(int fd, uint32_t handle, unsigned ring_id, 
const char *ring_nam
gem_sync(fd, handle);
 
for (count = 1; count <= SLOW_QUICK(1<<17, 1<<4); count <<= 1) {
-   const int reps = 13;
+   const int reps = 7;
igt_stats_t stats;
int n;
 
@@ -142,7 +142,7 @@ static void loop(int fd, uint32_t handle, unsigned ring_id, 
const char *ring_nam
for (n = 0; n < reps; n++) {
struct timespec start, end;
int loops = count;
-   sleep(1); /* wait for the hw to go back to sleep */
+   usleep(20); /* wait 200mS for the hw to go back to 
sleep */
clock_gettime(CLOCK_MONOTONIC, );
while (loops--)
do_ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, 
);
@@ -151,7 +151,7 @@ static void loop(int fd, uint32_t handle, unsigned ring_id, 
const char *ring_nam
igt_stats_push(, elapsed(, , count));
}
 
-   igt_info("Time to exec x %d:%7.3fµs (ring=%s)\n",
+   igt_info("Time to exec x %d:%7.3fuS (ring=%s)\n",
 count, igt_stats_get_trimean()/1000, ring_name);
fflush(stdout);
 
-- 
1.9.1

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 7/8] drm/i915/skl: Add boot parameter for disabling DC6

[Patrik Jakobsson]

On Tue, Nov 03, 2015 at 03:08:41PM +0200, Jani Nikula wrote:
> On Tue, 03 Nov 2015, Patrik Jakobsson  
> wrote:
> > Signed-off-by: Patrik Jakobsson 
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h | 1 +
> >  drivers/gpu/drm/i915/i915_params.c  | 6 ++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 2 +-
> >  3 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h 
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index efb6a00..9c8cb43 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2660,6 +2660,7 @@ struct i915_params {
> > int panel_use_ssc;
> > int vbt_sdvo_panel_type;
> > int enable_rc6;
> > +   int enable_dc6;
> > int enable_fbc;
> > int enable_ppgtt;
> > int enable_execlists;
> > diff --git a/drivers/gpu/drm/i915/i915_params.c 
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 368df67..1a7dedf 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -32,6 +32,7 @@ struct i915_params i915 __read_mostly = {
> > .panel_use_ssc = -1,
> > .vbt_sdvo_panel_type = -1,
> > .enable_rc6 = -1,
> > +   .enable_dc6 = 1,
> > .enable_fbc = -1,
> > .enable_execlists = -1,
> > .enable_hangcheck = true,
> > @@ -79,6 +80,11 @@ MODULE_PARM_DESC(enable_rc6,
> > "For example, 3 would enable rc6 and deep rc6, and 7 would enable 
> > everything. "
> > "default: -1 (use per-chip default)");
> >  
> > +module_param_named(enable_dc6, i915.enable_dc6, int, 0400);
> 
> _unsafe please.

Is the convention to taint for all non-default options in i915?

-Patrik

> 
> BR,
> Jani.
> 
> > +MODULE_PARM_DESC(enable_dc6,
> > +   "Enable power-saving display C-state 6. "
> > +   "(0 = disable; 1 = enable [default])");
> > +
> >  module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, 0600);
> >  MODULE_PARM_DESC(enable_fbc,
> > "Enable frame buffer compression for power savings "
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index 042d92f..4843c5c 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -753,7 +753,7 @@ static void skl_dc_off_power_well_enable(struct 
> > drm_i915_private *dev_priv,
> >  static void skl_dc_off_power_well_disable(struct drm_i915_private 
> > *dev_priv,
> >   struct i915_power_well *power_well)
> >  {
> > -   if (IS_SKYLAKE(dev_priv))
> > +   if (IS_SKYLAKE(dev_priv) && i915.enable_dc6)
> > skl_enable_dc6(dev_priv);
> > else
> > gen9_enable_dc5(dev_priv);
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] RFC drm/i915: Slaughter the thundering i915_wait_request herd

[Chris Wilson]

On Tue, Nov 03, 2015 at 01:31:22PM +, Gong, Zhipeng wrote:
> 
> > From: Chris Wilson [mailto:ch...@chris-wilson.co.uk]
> > 
> > Do you also have a relative perf statistics like op/s we can compare to make
> > sure we aren't just stalling the whole system?
> > 
> Could you please provide the commands about how to check it?

I was presuming your workload has some measure of efficiency/throughput?
It is one thing to say we are using 10% less CPU (per second), but the
task is running 2x as long!

> > How much cpu time is left in the i915_wait_request branch? i.e. how close to
> > the limit are we with chasing this path?
> Could you please provide the commands here either? :)

Check the perf callgraph.
-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 i-g-t] tests/gem_exec_nop: Improved test run time

[Chris Wilson]

On Tue, Nov 03, 2015 at 04:29:45PM +, Derek Morton wrote:
> Reduced the Sleep period to 200mS and reduced the repetition count to 7
> to decrease the test run time significantly.
> 
> Also fixed a non ascii character that messed up the results table formatting.

Pardon? Are you working around someone else's bug?
 
> Signed-off-by: Derek Morton 
> ---
>  tests/gem_exec_nop.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tests/gem_exec_nop.c b/tests/gem_exec_nop.c
> index a287d08..5028145 100644
> --- a/tests/gem_exec_nop.c
> +++ b/tests/gem_exec_nop.c
> @@ -133,7 +133,7 @@ static void loop(int fd, uint32_t handle, unsigned 
> ring_id, const char *ring_nam
>   gem_sync(fd, handle);
>  
>   for (count = 1; count <= SLOW_QUICK(1<<17, 1<<4); count <<= 1) {
> - const int reps = 13;
> + const int reps = 7;
>   igt_stats_t stats;
>   int n;
>  
> @@ -142,7 +142,7 @@ static void loop(int fd, uint32_t handle, unsigned 
> ring_id, const char *ring_nam
>   for (n = 0; n < reps; n++) {
>   struct timespec start, end;
>   int loops = count;
> - sleep(1); /* wait for the hw to go back to sleep */
> + usleep(20); /* wait 200mS for the hw to go back to 
> sleep */
>   clock_gettime(CLOCK_MONOTONIC, );
>   while (loops--)
>   do_ioctl(fd, DRM_IOCTL_I915_GEM_EXECBUFFER2, 
> );
> @@ -151,7 +151,7 @@ static void loop(int fd, uint32_t handle, unsigned 
> ring_id, const char *ring_nam
>   igt_stats_push(, elapsed(, , count));
>   }
>  
> - igt_info("Time to exec x %d:%7.3fµs (ring=%s)\n",
> + igt_info("Time to exec x %d:%7.3fuS (ring=%s)\n",

Did SI change the standard unit of measurement for time?
-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