Re: [Intel-gfx] [PATCH] drm/i915: A better maximum brightness for users.

[Jani Nikula]

On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)"  wrote:
> On Mon, Nov 9, 2015 at 6:51 PM, Jani Nikula 
> wrote:
>
>> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" 
>> wrote:
>> > On Mon, Nov 9, 2015 at 6:17 PM, Jani Nikula > >
>> > wrote:
>> >
>> >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" > >
>> >> wrote:
>> >> > The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937
>> however
>> >> > the sysfs brightness level always starts from 0 so it is better to use
>> >> > 927 as the sysfs maximum brightness level and it becomes easier to map
>> >> > from the PWM brightness level to the sysfs brightness level.
>> >>
>> >> We've been thinking we should provide a fixed range to userspace
>> >> instead. Say, 0-100.
>> >>
>> >> BR,
>> >> Jani.
>> >>
>> >>
>> >>
>> > That might not be a good idea for the backward compatibility.
>>
>> Please explain how you think your change is good and a fixed range 0-100
>> is bad in terms of backward compatibility. Both just arbitrarily change
>> the max.
>>
> The original sysfs brightness range is from 0 to 937. Let's define it as A
> = {0, 1, 2, ..., 937}.
> The PWM brightness range is from 10 to 937. Let's define it as B = {10, 11,
> 12, ..., 937}.
> |A| = 938, |B| = 928
> |A| != |B|
> It implies A and B is not an 1-1 mapping.
>
> My patch is not a arbitrarily change.
> It makes A' = {0, 1, 2, ..., 927}. |A'| = 928
> You can see |A'| = |B| so it implies A' and B is an 1-1 mapping.
> It means any value in A' can be mapped to a different value in B and visa
> versa.
>
> C = {0, 100} has the same mapping problem.

Please tell me why you think this is a problem to begin with. What (user
perceptible) problem are you trying to solve?

I understand we could simplify (or remove) the scaling code with your
change, but I'm reluctant to go that way when, as I said, we've been
talking about fixing the range reported to userspace.

Part of the reason for going to 0-100 range would be that there are
barely that many user distinguishable steps in the backlight level. It
is silly to have brightness range of, say, 0-937, because you can't
distinguish them from each other. (Perhaps counter-intuitively, the
higher the PWM modulation frequency, the fewer user distinguishable
levels you can actually get.)

>> Besides, we've changed the max for some platforms before, and the ABI
>> for backlight sysfs is you can stick a value between 0 and
>> max_brightness to the brightness attribute. Any userspace that relies on
>> anything else is broken.
>>
>> > However I saw some message as the following.
>> > [3.402233] [drm:parse_lfp_backlight] VBT backlight PWM modulation
>> > frequency 200 Hz, active high, min brightness 10, level 255
>> >
>> >
>> > Does it mean the brightness range is also defined in the BIOS?
>>
>> The minimum and the PWM modulation frequency are defined in BIOS. The
>> modulation frequency is an attribute for the hardware; I think that was
>> originally exposed as the max was just for implementation convenience.
>>
> I don't mean the minimum or the PWM modulation frequency.
> I mean "level 255".
> Does it mean the brightness range or something else?

It probably means the suggested initial level of the backlight in some
units, but AFAICT we don't use that for anything, and frankly I am not
sure why we log it.

BR,
Jani.


>
> Regards,
> $4
>
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > Regards,
>> > $4
>> >
>> >>
>> >> >
>> >> > Signed-off-by: Shih-Yuan Lee (FourDollars) 
>> >> > ---
>> >> >  drivers/gpu/drm/i915/intel_panel.c | 2 +-
>> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c
>> >> b/drivers/gpu/drm/i915/intel_panel.c
>> >> > index a24df35..697fd4d 100644
>> >> > --- a/drivers/gpu/drm/i915/intel_panel.c
>> >> > +++ b/drivers/gpu/drm/i915/intel_panel.c
>> >> > @@ -1211,7 +1211,7 @@ static int
>> intel_backlight_device_register(struct
>> >> intel_connector *connector)
>> >> >* Note: Everything should work even if the backlight device max
>> >> >* presented to the userspace is arbitrarily chosen.
>> >> >*/
>> >> > - props.max_brightness = panel->backlight.max;
>> >> > + props.max_brightness = panel->backlight.max -
>> panel->backlight.min;
>> >> >   props.brightness = scale_hw_to_user(connector,
>> >> >   panel->backlight.level,
>> >> >   props.max_brightness);
>> >>
>> >> --
>> >> Jani Nikula, Intel Open Source Technology Center
>> >>
>>
>> --
>> 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

Re: [Intel-gfx] [PATCH] drm/i915: get runtime PM reference around GEM set_caching IOCTL

[Chris Wilson]

On Mon, Nov 09, 2015 at 03:36:10PM +0200, Imre Deak wrote:
> On ma, 2015-11-09 at 13:25 +, Chris Wilson wrote:
> > On Mon, Nov 09, 2015 at 03:09:18PM +0200, Imre Deak wrote:
> > > Looked through it, it seems only i915_gem_set_tiling() could
> > > release
> > > the PTE's without waking up the hardware (if no need to unbind the
> > > object). Otherwise it's true that all callers hold (or should hold)
> > > already an RPM ref. To fix the set tiling case to work after your
> > > optimization we could wake up the HW unconditionally there, use a
> > > no_resume RPM ref+and RPM barrier or a separate new lock for the
> > > fault
> > > list.
> > 
> > I was suggesting we move to the model where writes through gsm took
> > the
> > rpm reference itself.
> 
> Yes, but even then you want to have a lock around updating the new
> fault list, no? So if we go with your way and push down the RPM ref
> where GSM is written, we wouldn't have a lock around the fault_list
> update in i915_gem_set_tiling() (via i915_gem_release_mmap()). That's
> where I meant we need an extra ref/lock above.

Ok, I remember some of the specifics I had in mind about having to do it
inside the if (vma->map_and_fencable) branch of i915_vma_unbind() as
opposed to be able to push it into ggtt_unbind_vma... Hmm, pushing that
itself down to ggtt_unbind_vma isn't too silly. Equally moving the
vma->ggtt_view.pages = NULL would also help with symmetry.

Plenty of opportunity here for cleanup that should pay off nicely wrt to
rpm handling :)
-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 06/14] drm/i915: Update watermark related members in the crtc_state, v2.

[Ander Conselvan De Oliveira]

On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> This removes another couple of hacks from intel_crtc->atomic, and
> creates proper atomic state for it.

Perhaps you could be more verbose about the hacks being removed.

> Changes since v1:
> - Rebase on top of wm changes.
> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  2 ++
>  drivers/gpu/drm/i915/intel_display.c | 41 +--
> -
>  drivers/gpu/drm/i915/intel_drv.h |  6 +++---
>  3 files changed, 24 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index c4eadbc928b7..3e390db9d22b 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -95,6 +95,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>  
>   crtc_state->update_pipe = false;
>   crtc_state->disable_lp_wm = false;
> + crtc_state->visible_changed = false;
> + crtc_state->wm_changed = false;
>  
>   return _state->base;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 54e4f04bb427..356e3a9e1741 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4753,6 +4753,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>  static void intel_post_plane_update(struct intel_crtc *crtc)
>  {
>   struct intel_crtc_atomic_commit *atomic = >atomic;
> + struct intel_crtc_state *pipe_config =
> + to_intel_crtc_state(crtc->base.state);
>   struct drm_device *dev = crtc->base.dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>  
> @@ -4761,10 +4763,9 @@ static void intel_post_plane_update(struct intel_crtc
> *crtc)
>  
>   intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
> - if (atomic->disable_cxsr)
> - crtc->wm.cxsr_allowed = true;
> + crtc->wm.cxsr_allowed = true;
>  
> - if (crtc->atomic.update_wm_post)
> + if (pipe_config->wm_changed)
>   intel_update_watermarks(>base);
>  
>   if (atomic->update_fbc)
> @@ -4781,6 +4782,8 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
>   struct drm_device *dev = crtc->base.dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>   struct intel_crtc_atomic_commit *atomic = >atomic;
> + struct intel_crtc_state *pipe_config =
> + to_intel_crtc_state(crtc->base.state);
>  
>   if (atomic->disable_fbc)
>   intel_fbc_disable_crtc(crtc);
> @@ -4791,10 +4794,13 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
>   if (atomic->pre_disable_primary)
>   intel_pre_disable_primary(>base);
>  
> - if (atomic->disable_cxsr) {
> + if (pipe_config->visible_changed) {
>   crtc->wm.cxsr_allowed = false;
>   intel_set_memory_cxsr(dev_priv, false);
>   }
> +
> + if (!needs_modeset(_config->base) && pipe_config->wm_changed)
> + intel_update_watermarks(>base);
>  }
>  
>  static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned
> plane_mask)
> @@ -11617,6 +11623,7 @@ static bool needs_scaling(struct intel_plane_state
> *state)
>  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>   struct drm_plane_state *plane_state)
>  {
> + struct intel_crtc_state *pipe_config =
> to_intel_crtc_state(crtc_state);
>   struct drm_crtc *crtc = crtc_state->crtc;
>   struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>   struct drm_plane *plane = plane_state->plane;
> @@ -11663,25 +11670,18 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>plane->base.id, was_visible, visible,
>turn_off, turn_on, mode_changed);
>  
> - if (turn_on) {
> - intel_crtc->atomic.update_wm_pre = true;
> - /* must disable cxsr around plane enable/disable */
> - if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> - intel_crtc->atomic.disable_cxsr = true;
> - /* to potentially re-enable cxsr */
> - intel_crtc->atomic.wait_vblank = true;
> - intel_crtc->atomic.update_wm_post = true;
> - }
> - } else if (turn_off) {
> - intel_crtc->atomic.update_wm_post = true;
> + if (turn_on || turn_off) {
> + pipe_config->wm_changed = true;
> +
>   /* must disable cxsr around plane enable/disable */
>   if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> + pipe_config->visible_changed = true;
>   if (is_crtc_enabled)
>   intel_crtc->atomic.wait_vblank = true;
> - intel_crtc->atomic.disable_cxsr = true;
>   }

I'm a bit confused 

Re: [Intel-gfx] [PATCH v2 07/14] drm/i915: Kill off intel_crtc->atomic.wait_vblank.

[Ander Conselvan De Oliveira]

On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> By handling this after the atomic helper waits for vblanks there will
> be one less wait for vblank in the atomic path.

I found this a bit confusing. Sounds like you are referring to calling
intel_post_enable_update() after the call to drm_atomic_helper_wait_for_vblanks(
), except that move wasn't mentioned before. Also, the patch replaces that call
with our own wait for vblank function, so it is probably good to mention why in
the commit message.

> 
> Also get rid of the double wait_for_vblank on broadwell, looks like
> it's a bug doing the vblank wait twice.

Separate patch with more details in the commit message.

Ander

> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/intel_atomic.c  |  1 +
>  drivers/gpu/drm/i915/intel_display.c | 95 +++
> -
>  drivers/gpu/drm/i915/intel_drv.h |  2 +-
>  3 files changed, 64 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> b/drivers/gpu/drm/i915/intel_atomic.c
> index 3e390db9d22b..2ba0cd698f9b 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -97,6 +97,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>   crtc_state->disable_lp_wm = false;
>   crtc_state->visible_changed = false;
>   crtc_state->wm_changed = false;
> + crtc_state->fb_changed = false;
>  
>   return _state->base;
>  }
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 356e3a9e1741..2708899d9767 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4669,14 +4669,6 @@ intel_post_enable_primary(struct drm_crtc *crtc)
>   int pipe = intel_crtc->pipe;
>  
>   /*
> -  * BDW signals flip done immediately if the plane
> -  * is disabled, even if the plane enable is already
> -  * armed to occur at the next vblank :(
> -  */
> - if (IS_BROADWELL(dev))
> - intel_wait_for_vblank(dev, pipe);
> -
> - /*
>* FIXME IPS should be fine as long as one plane is
>* enabled, but in practice it seems to have problems
>* when going from primary only to sprite only and vice
> @@ -4758,9 +4750,6 @@ static void intel_post_plane_update(struct intel_crtc
> *crtc)
>   struct drm_device *dev = crtc->base.dev;
>   struct drm_i915_private *dev_priv = dev->dev_private;
>  
> - if (atomic->wait_vblank)
> - intel_wait_for_vblank(dev, crtc->pipe);
> -
>   intel_frontbuffer_flip(dev, atomic->fb_bits);
>  
>   crtc->wm.cxsr_allowed = true;
> @@ -11660,6 +11649,9 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>   if (!was_visible && !visible)
>   return 0;
>  
> + if (fb != old_plane_state->base.fb)
> + pipe_config->fb_changed = true;
> +
>   turn_off = was_visible && (!visible || mode_changed);
>   turn_on = visible && (!was_visible || mode_changed);
>  
> @@ -11676,8 +11668,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>   /* must disable cxsr around plane enable/disable */
>   if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>   pipe_config->visible_changed = true;
> - if (is_crtc_enabled)
> - intel_crtc->atomic.wait_vblank = true;
>   }
>   } else if ((was_visible || visible) &&
>  intel_wm_need_update(plane, plane_state)) {
> @@ -11724,14 +11714,6 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>   plane_state->rotation != BIT(DRM_ROTATE_0))
>   intel_crtc->atomic.disable_fbc = true;
>  
> - /*
> -  * BDW signals flip done immediately if the plane
> -  * is disabled, even if the plane enable is already
> -  * armed to occur at the next vblank :(
> -  */
> - if (turn_on && IS_BROADWELL(dev))
> - intel_crtc->atomic.wait_vblank = true;
> -
>   intel_crtc->atomic.update_fbc |= visible || mode_changed;
>   break;
>   case DRM_PLANE_TYPE_CURSOR:
> @@ -11746,12 +11728,10 @@ 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)) {
> - to_intel_crtc_state(crtc_state)->disable_lp_wm =
> true;
> - } else if (turn_off && !mode_changed) {
> - intel_crtc->atomic.wait_vblank = true;
> + pipe_config->disable_lp_wm = true;
> + } else if (turn_off && !mode_changed)
>   intel_crtc->atomic.update_sprite_watermarks |=
>

Re: [Intel-gfx] [PATCH v7 0/3] fbdev fixes (reviewed)

[Jani Nikula]

On Sat, 07 Nov 2015, Lukas Wunner  wrote:
> Hi Jani,
>
> three patches with fbdev deadlock & failure path fixes,
> each with Reviewed-by tag by Ville or Daniel, the third one
> with amended commit message as requested by Daniel in
> <20151030182818.GR16848@phenom.ffwll.local>.
>
> Patch 1 fixes double unref of bo in failure path of intelfb_alloc().
> Patch 2 fixes another double unref of bo in failure path of intelfb_alloc().
> Patch 3 fixes deadlocks in intelfb_create(), one during driver
> initialization and one in failure path.
>
> Browsable on GitHub:
> https://github.com/l1k/linux/commits/intel_fbdev_fixes
>
> The previous iteration (v6, )
> contained a 4th patch, a new version of which I'll submit separately.

Pushed all three to drm-intel-next-queued. Thanks for the patches and
review.

For future reference, please consider posting new versions of series as
new threads. This one got pretty messy in the end, with so many
different versions.

Also, I don't know how you generate and send your patches, but even the
updated patches had dates of the original or very old versions of
them. Like [1] is June 30 although you sent it just a couple of days
ago. Please look into that.

BR,
Jani.


[1] 
http://mid.gmane.org/47d4e88c91b3bf0f7a280cabec54c8c8cf0cf6f2.1446892879.git.lu...@wunner.de

-- 
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 3/3] drm/i915: Use insert_page for pwrite_fast

[kbuild test robot]

Hi Ankitprasad,

[auto build test WARNING on drm-intel/for-linux-next]
[also build test WARNING on v4.3 next-20151109]

url:
https://github.com/0day-ci/linux/commits/ankitprasad-r-sharma-intel-com/Support-for-mapping-an-object-page-by-page/20151109-191910
base:   git://anongit.freedesktop.org/drm-intel for-linux-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/drm/drm_crtc.h:875: warning: No description found for parameter 
'mutex'
   include/drm/drm_crtc.h:875: warning: No description found for parameter 
'helper_private'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tile_idr'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'delayed_event'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'edid_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'dpms_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'path_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tile_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'plane_type_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'rotation_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_src_x'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_src_y'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_src_w'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_src_h'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_crtc_x'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_crtc_y'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_crtc_w'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_crtc_h'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_fb_id'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_crtc_id'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_active'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'prop_mode_id'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'dvi_i_subconnector_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'dvi_i_select_subconnector_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_subconnector_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_select_subconnector_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_mode_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_left_margin_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_right_margin_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_top_margin_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_bottom_margin_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_brightness_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_contrast_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_flicker_reduction_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_overscan_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_saturation_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'tv_hue_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'scaling_mode_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'aspect_ratio_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'dirty_info_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'suggested_x_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'suggested_y_property'
   include/drm/drm_crtc.h:1160: warning: No description found for parameter 
'allow_fb_modifiers'
   include/drm/drm_fb_helper.h:148: warning: No description found for parameter 
'connector_info'
   include/drm/drm_dp_helper.h:709: warning: No description found for parameter 
'i2c_nack_count'
   include/drm/drm_dp_helper.h:709: warning: No description found for parameter 
'i2c_defer_count'
   drivers/gpu/drm/drm_dp_mst_topology.c:2211: warning: No description found 
for parameter 'connector'
   include/drm/drm_dp_mst_helpe

Re: [Intel-gfx] [PATCH] drm/i915: A better maximum brightness for users.

[Shih-Yuan Lee (FourDollars)]

On Mon, Nov 9, 2015 at 8:58 PM, Jani Nikula 
wrote:

> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" 
> wrote:
> > On Mon, Nov 9, 2015 at 6:51 PM, Jani Nikula  >
> > wrote:
> >
> >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)"  >
> >> wrote:
> >> > On Mon, Nov 9, 2015 at 6:17 PM, Jani Nikula <
> jani.nik...@linux.intel.com
> >> >
> >> > wrote:
> >> >
> >> >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <
> sy...@canonical.com
> >> >
> >> >> wrote:
> >> >> > The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937
> >> however
> >> >> > the sysfs brightness level always starts from 0 so it is better to
> use
> >> >> > 927 as the sysfs maximum brightness level and it becomes easier to
> map
> >> >> > from the PWM brightness level to the sysfs brightness level.
> >> >>
> >> >> We've been thinking we should provide a fixed range to userspace
> >> >> instead. Say, 0-100.
> >> >>
> >> >> BR,
> >> >> Jani.
> >> >>
> >> >>
> >> >>
> >> > That might not be a good idea for the backward compatibility.
> >>
> >> Please explain how you think your change is good and a fixed range 0-100
> >> is bad in terms of backward compatibility. Both just arbitrarily change
> >> the max.
> >>
> > The original sysfs brightness range is from 0 to 937. Let's define it as
> A
> > = {0, 1, 2, ..., 937}.
> > The PWM brightness range is from 10 to 937. Let's define it as B = {10,
> 11,
> > 12, ..., 937}.
> > |A| = 938, |B| = 928
> > |A| != |B|
> > It implies A and B is not an 1-1 mapping.
> >
> > My patch is not a arbitrarily change.
> > It makes A' = {0, 1, 2, ..., 927}. |A'| = 928
> > You can see |A'| = |B| so it implies A' and B is an 1-1 mapping.
> > It means any value in A' can be mapped to a different value in B and visa
> > versa.
> >
> > C = {0, 100} has the same mapping problem.
>
> Please tell me why you think this is a problem to begin with. What (user
> perceptible) problem are you trying to solve?
>
I am investigating some backlight issue that the i915.ko brightness
behavior is changed on Dell XPS 13 (2015).
Originally the lowest brightness won't turn off the backlight but the Linux
kernel after 3.19 will turn off the backlight.
Dell's engineer tells me that Windows driver also uses VBT to change the
brightness but it doesn't turn off the backlight.
I am not a dedicated kernel engineer but I have some interest to look at
this issue.
This regression is from
http://lists.freedesktop.org/archives/intel-gfx/2014-August/050642.html.

This patch is found during my investigation for that problem.

>
> I understand we could simplify (or remove) the scaling code with your
> change, but I'm reluctant to go that way when, as I said, we've been
> talking about fixing the range reported to userspace.
>
I personally think i915.ko just needs to respect the settings from VBT.
No matter how many valid levels from VBT, i915.ko just provides the same
levels in brightness sysfs.

>
> Part of the reason for going to 0-100 range would be that there are
> barely that many user distinguishable steps in the backlight level. It
> is silly to have brightness range of, say, 0-937, because you can't
> distinguish them from each other. (Perhaps counter-intuitively, the
> higher the PWM modulation frequency, the fewer user distinguishable
> levels you can actually get.)
>
I think i915.ko doesn't need to care about this problem.
In fact, the very end users only use a scroll bar to change the brightness.
Or they will use brightness up/down hotkeys to change the brightness but
the desktop environments like GNOME will make it only work for 20 levels.

However some advanced users like me may still prefer to have all valid
brightness levels.
That is why I made this patch and this is my first patch for Linux kernel
project.

Regards,
$4

>
> >> Besides, we've changed the max for some platforms before, and the ABI
> >> for backlight sysfs is you can stick a value between 0 and
> >> max_brightness to the brightness attribute. Any userspace that relies on
> >> anything else is broken.
> >>
> >> > However I saw some message as the following.
> >> > [3.402233] [drm:parse_lfp_backlight] VBT backlight PWM modulation
> >> > frequency 200 Hz, active high, min brightness 10, level 255
> >> >
> >> >
> >> > Does it mean the brightness range is also defined in the BIOS?
> >>
> >> The minimum and the PWM modulation frequency are defined in BIOS. The
> >> modulation frequency is an attribute for the hardware; I think that was
> >> originally exposed as the max was just for implementation convenience.
> >>
> > I don't mean the minimum or the PWM modulation frequency.
> > I mean "level 255".
> > Does it mean the brightness range or something else?
>
> It probably means the suggested initial level of the backlight in some
> units, but AFAICT we don't use that for anything, and frankly I am not
> sure why we log it.
>
> BR,
> Jani.
>
>
> >
> > Regards,

Re: [Intel-gfx] [PATCH] drm/i915: get runtime PM reference around GEM set_caching IOCTL

[Chris Wilson]

On Mon, Nov 09, 2015 at 03:09:18PM +0200, Imre Deak wrote:
> Looked through it, it seems only i915_gem_set_tiling() could release
> the PTE's without waking up the hardware (if no need to unbind the
> object). Otherwise it's true that all callers hold (or should hold)
> already an RPM ref. To fix the set tiling case to work after your
> optimization we could wake up the HW unconditionally there, use a
> no_resume RPM ref+and RPM barrier or a separate new lock for the fault
> list.

I was suggesting we move to the model where writes through gsm took the
rpm reference itself.
 
> > From the rpm point of view, this should improve the success of 
> > runtime suspend, and reduce wakelocks.
> 
> Yes, seems like a worthy optimization, since I assume struct_mutex can
> be held for a long time without the need to wake up the hardware.

Admittedly most of the time we hold struct_mutex, the hw will be awake
for other reasons. But there are many times where we do take the
struct_mutex for 10s (if not 100s!) of milliseconds where the hw is
completely idle, and so every chance to reduce usage/contention on
struct_mutex is a little victory.
 
> Are you ok to first have the fix I posted and a similar one for
>  i915_gem_set_tiling()? And then to follow-up with your plan.

Yes, adding the extra reference to that ioctl, juggling with the
struct_mutex and then moving the rpm reference to where it is required
lgtm.
-Chris

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

Re: [Intel-gfx] [PATCH] drm/i915: get runtime PM reference around GEM set_caching IOCTL

[Imre Deak]

On pe, 2015-11-06 at 08:54 +, Chris Wilson wrote:
> On Fri, Nov 06, 2015 at 12:57:35AM +0200, Imre Deak wrote:
> > On Thu, 2015-11-05 at 11:56 +, Chris Wilson wrote:
> > > On Thu, Nov 05, 2015 at 01:28:32PM +0200, Imre Deak wrote:
> > > > On ke, 2015-11-04 at 20:57 +, Chris Wilson wrote:
> > > > > On Wed, Nov 04, 2015 at 09:25:32PM +0200, Imre Deak wrote:
> > > > > > After Damien's D3 fix I started to get runtime suspend
> > > > > > residency for the
> > > > > > first time and that revealed a breakage on the set_caching
> > > > > > IOCTL path
> > > > > > that accesses the HW but doesn't take an RPM ref. Fix this
> > > > > > up.
> > > > > 
> > > > > Why here (and in so many random places) and not around the
> > > > > PTE write
> > > > > itself?
> > > > 
> > > > Imo we should take the RPM ref outside of any of our locks.
> > > > Otherwise we
> > > > need hacks like we have currently in the runtime suspend
> > > > handler to work
> > > > around lock inversions. It works now, but we couldn't do the
> > > > same trick
> > > > if we needed to take struct_mutex for example in the resume
> > > > handler too
> > > > for some reason.
> > > 
> > > On the other hand, it leads to hard to track down bugs and
> > > improper
> > > documentation. Neither solution is perfect.
> > 
> > I think I understand the documentation part, not sure how that
> > could be
> > solved. Perhaps RPM-held asserts right before the point where the
> > HW
> > needs to be on?
> > 
> > What do you mean by hard to track down bugs? Couldn't that also be
> > tackled by asserts?
> > 
> > > Note since intel_runtime_suspend has ample barriers of its own,
> > > you can
> > > avoid the struct_mutex if you have a dedicated dev_priv
> > > ->mm.fault_list.
> > > 
> > > Something along the lines of:
> > 
> > Ok, looks interesting. But as you said we would have to then make
> > callers of i915_gem_release_mmap() wake up the device, which isn't
> > strictly necessary. (and imo as such goes somewhat against the
> > documentation argument)
> 
> Outside of suspend, we only revoke the CPU PTE mapping when we change
> the hardware (as doing so is very expensive). So all callers should
> already have a reason (and be holding) the rpm wakelock, the only
> complication from my point of view is enforcing that and reviewing
> that
> what I said is true.

Looked through it, it seems only i915_gem_set_tiling() could release
the PTE's without waking up the hardware (if no need to unbind the
object). Otherwise it's true that all callers hold (or should hold)
already an RPM ref. To fix the set tiling case to work after your
optimization we could wake up the HW unconditionally there, use a
no_resume RPM ref+and RPM barrier or a separate new lock for the fault
list.

> From the rpm point of view, this should improve the success of 
> runtime suspend, and reduce wakelocks.

Yes, seems like a worthy optimization, since I assume struct_mutex can
be held for a long time without the need to wake up the hardware.

Are you ok to first have the fix I posted and a similar one for
 i915_gem_set_tiling()? And then to follow-up with your plan.

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

Re: [Intel-gfx] [PATCH i-g-t 4/8] kms_frontbuffer_tracking: Allow to skip suspend_resume sub test case.

[Paulo Zanoni]

2015-11-05 18:40 GMT-02:00 Ville Syrjälä :
> On Thu, Nov 05, 2015 at 06:34:07PM -0200, Paulo Zanoni wrote:
>> 2015-11-05 16:53 GMT-02:00 Rodrigo Vivi :
>> > There are few platforms with other suspend resume bugs that breaks
>> > the full execution. So let's provide a way to skip suspend resume case.
>>
>> Well, I carry a local patch that completely disables suspend subtests
>> for the tests that I usually run, so I really understand your pain.
>> Suspend subtests take a long time to run, and they usually don't work
>> on some of the preproduction machines I still use.
>>
>> But since this problem is not specific to kms_frontbuffer_tracking,
>> maybe we could adopt an igt-wide solution here? Thomas, any idea here?
>
> -x suspend is what I tell piglit on one hsw I have here which hangs on s3.

The problem with piglit is that it runs every single subtest as a
separate test program invocation. For KMS tests this is a huge problem
since it requires generating the reference CRCs every time and it also
requires a modeset to restore fbcon every single time. For non-eDP the
cost is not big, but for eDP it adds up: running a subset of only 70
subtests on kms_frontbuffer_tracking, we go from 3:05 to 6:21 in total
execution time.

>
>>
>> >
>> > Signed-off-by: Rodrigo Vivi 
>> > ---
>> >  tests/kms_frontbuffer_tracking.c | 9 +
>> >  1 file changed, 9 insertions(+)
>> >
>> > diff --git a/tests/kms_frontbuffer_tracking.c 
>> > b/tests/kms_frontbuffer_tracking.c
>> > index d879493..1cc1c9e 100644
>> > --- a/tests/kms_frontbuffer_tracking.c
>> > +++ b/tests/kms_frontbuffer_tracking.c
>> > @@ -237,6 +237,7 @@ struct {
>> > bool fbc_check_compression;
>> > bool fbc_check_last_action;
>> > bool no_edp;
>> > +   bool no_suspend;
>> > bool small_modes;
>> > bool show_hidden;
>> > int step;
>> > @@ -250,6 +251,7 @@ struct {
>> > .fbc_check_compression = true,
>> > .fbc_check_last_action = true,
>> > .no_edp = false,
>> > +   .no_suspend = false,
>> > .small_modes = false,
>> > .show_hidden= false,
>> > .step = 0,
>> > @@ -2735,6 +2737,8 @@ static void suspend_subtest(const struct test_mode 
>> > *t)
>> >  {
>> > struct modeset_params *params = pick_params(t);
>> >
>> > +   igt_skip_on(opt.no_suspend);
>> > +
>> > prepare_subtest(t, NULL);
>> > sleep(5);
>> > igt_system_suspend_autoresume();
>> > @@ -2950,6 +2954,9 @@ static int opt_handler(int option, int option_index, 
>> > void *data)
>> > case 'e':
>> > opt.no_edp = true;
>> > break;
>> > +   case 'r':
>> > +   opt.no_suspend = true;
>> > +   break;
>> > case 'm':
>> > opt.small_modes = true;
>> > break;
>> > @@ -2992,6 +2999,7 @@ const char *help_str =
>> >  "  --no-fbc-compression-check  Don't check for the FBC compression 
>> > status\n"
>> >  "  --no-fbc-action-check   Don't check for the FBC last action\n"
>> >  "  --no-edpDon't use eDP monitors\n"
>> > +"  --no-suspendDon't run Suspend/Resume test cases\n"
>> >  "  --use-small-modes   Use smaller resolutions for the modes\n"
>> >  "  --show-hidden   Show hidden subtests\n"
>> >  "  --step  Stop on each step so you can check the 
>> > screen\n"
>> > @@ -3117,6 +3125,7 @@ int main(int argc, char *argv[])
>> > { "no-fbc-compression-check", 0, 0, 'o'},
>> > { "no-fbc-action-check",  0, 0, 'a'},
>> > { "no-edp",   0, 0, 'e'},
>> > +   { "no-suspend",   0, 0, 'r'},
>> > { "use-small-modes",  0, 0, 'm'},
>> > { "show-hidden",  0, 0, 'i'},
>> > { "step", 0, 0, 't'},
>> > --
>> > 2.4.3
>> >
>> > ___
>> > Intel-gfx mailing list
>> > Intel-gfx@lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>>
>>
>>
>> --
>> Paulo Zanoni
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Ville Syrjälä
> Intel OTC



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

Re: [Intel-gfx] [PATCH i-g-t 8/8] tests/kms_fbc_crc: ensure context is initialized correctly

[Thomas Wood]

On 4 November 2015 at 19:36, Zanoni, Paulo R  wrote:
> Em Seg, 2015-11-02 às 11:48 +, Thomas Wood escreveu:
>> Initialization was included in commit a976d7e (tests/kms_fbc_crc:
>> refactor context
>> handling code), but won't be executed since it is declared before the
>> first
>> label within a switch statement.
>>
>> kms_fbc_crc.c:178:2: warning: ‘context’ may be used uninitialized in
>> this function [-Wmaybe-uninitialized]
>>   rendercopy(batch, context,
>>   ^
>> kms_fbc_crc.c:271:22: note: ‘context’ was declared here
>>drm_intel_context *context = NULL;
>
> I don't see these warnings here. Which compiler do you use?

The warning requires -Wmaybe-uninitialized and an appropriate
optimisation level to be enabled. -Wmaybe-uninitialized is currently
disabled by the debug cflags in intel-gpu-tools due to previous false
positives, but it looks like the situation with regards to this has
improved and so it may be time to re-enable it.


>
> pzanoni@panetone:~/nfs/intel-gpu-tools/tests$ gcc --version
> gcc (Debian 5.2.1-22) 5.2.1 20151010
>
> The weird part is that it was initialized during the declaration, so
> the declaration is valid yet the initialization is not? Anyway, your
> change removes the ambiguity of the situation and avoid citations of
> the C standard, so:
>
> Reviewed-by: Paulo Zanoni 
>
>>
>>   ^
>> Cc: Paulo Zanoni 
>> Signed-off-by: Thomas Wood 
>> ---
>>  tests/kms_fbc_crc.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tests/kms_fbc_crc.c b/tests/kms_fbc_crc.c
>> index d580a94..02e95e5 100644
>> --- a/tests/kms_fbc_crc.c
>> +++ b/tests/kms_fbc_crc.c
>> @@ -255,6 +255,7 @@ static void test_crc(data_t *data, enum test_mode
>> mode)
>>  {
>>   uint32_t crtc_id = data->output->config.crtc->crtc_id;
>>   uint32_t handle = data->fb[0].gem_handle;
>> + drm_intel_context *context = NULL;
>>
>>   igt_assert(fbc_enabled(data));
>>
>> @@ -268,7 +269,6 @@ static void test_crc(data_t *data, enum test_mode
>> mode)
>>   }
>>
>>   switch (mode) {
>> - drm_intel_context *context = NULL;
>>   case TEST_PAGE_FLIP:
>>   break;
>>   case TEST_MMAP_CPU:
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: get runtime PM reference around GEM set_caching IOCTL

[Imre Deak]

On ma, 2015-11-09 at 13:25 +, Chris Wilson wrote:
> On Mon, Nov 09, 2015 at 03:09:18PM +0200, Imre Deak wrote:
> > Looked through it, it seems only i915_gem_set_tiling() could
> > release
> > the PTE's without waking up the hardware (if no need to unbind the
> > object). Otherwise it's true that all callers hold (or should hold)
> > already an RPM ref. To fix the set tiling case to work after your
> > optimization we could wake up the HW unconditionally there, use a
> > no_resume RPM ref+and RPM barrier or a separate new lock for the
> > fault
> > list.
> 
> I was suggesting we move to the model where writes through gsm took
> the
> rpm reference itself.

Yes, but even then you want to have a lock around updating the new
fault list, no? So if we go with your way and push down the RPM ref
where GSM is written, we wouldn't have a lock around the fault_list
update in i915_gem_set_tiling() (via i915_gem_release_mmap()). That's
where I meant we need an extra ref/lock above.
 
> > > From the rpm point of view, this should improve the success of 
> > > runtime suspend, and reduce wakelocks.
> > 
> > Yes, seems like a worthy optimization, since I assume struct_mutex
> > can
> > be held for a long time without the need to wake up the hardware.
> 
> Admittedly most of the time we hold struct_mutex, the hw will be
> awake
> for other reasons. But there are many times where we do take the
> struct_mutex for 10s (if not 100s!) of milliseconds where the hw is
> completely idle, and so every chance to reduce usage/contention on
> struct_mutex is a little victory.
>  
> > Are you ok to first have the fix I posted and a similar one for
> >  i915_gem_set_tiling()? And then to follow-up with your plan.
> 
> Yes, adding the extra reference to that ioctl, juggling with the
> struct_mutex and then moving the rpm reference to where it is
> required
> lgtm.

Ok, will post a new one for set_tiling.

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

Re: [Intel-gfx] [PATCH v2 4/4] drm/i915: add assert_device_not_suspended to GGTT PTE updaters

[Chris Wilson]

On Mon, Nov 09, 2015 at 09:14:19PM +0200, Imre Deak wrote:
> The device should be on when updating the GGTT PTEs, so add an assert to
> all relevant places.
> 
> v2:
> - use the existing dev_priv directly everywhere (Ville)
> 
> Signed-off-by: Imre Deak 

For completeness, add one to i915_ggtt_insert_entries(). Yes, I know
that we never will support rpm on any of those devices, but we may as
well be consistent in documenting that these functions will write
through the magic PCI region.

An issue I see here is dev_priv->pm.suspended doesn't actually say that
rpm can't suspend during the GGTT PTEs writes. Would we not want
something more like assert_rpm_wakelock() and !dev_priv->pm.can_suspend?
-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 4/4] drm/i915: add assert_device_not_suspended to GGTT PTE updaters

[Imre Deak]

On Mon, 2015-11-09 at 23:24 +0200, Imre Deak wrote:
> On Mon, 2015-11-09 at 21:11 +, Chris Wilson wrote:
> > On Mon, Nov 09, 2015 at 09:14:19PM +0200, Imre Deak wrote:
> > > The device should be on when updating the GGTT PTEs, so add an assert to
> > > all relevant places.
> > > 
> > > v2:
> > > - use the existing dev_priv directly everywhere (Ville)
> > > 
> > > Signed-off-by: Imre Deak 
> > 
> > For completeness, add one to i915_ggtt_insert_entries(). Yes, I know
> > that we never will support rpm on any of those devices, but we may as
> > well be consistent in documenting that these functions will write
> > through the magic PCI region.
> 
> Ok, forgot about that one.
> 
> > An issue I see here is dev_priv->pm.suspended doesn't actually say that
> > rpm can't suspend during the GGTT PTEs writes. Would we not want
> > something more like assert_rpm_wakelock() and !dev_priv->pm.can_suspend?
> 
> Well, after this patchset we assert that the RPM refcount!=0 too, which
> should guarantee that provided that pm.suspended=false as well. It's
> possible to have a non-zero refcount with a suspended state (in case of
> rpm_get_no_resume).

But you are right that assert_device_not_suspended doesn't reflect that
completely, so I could rename it.

> 
> > -Chris
> > 
> 


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

Re: [Intel-gfx] [PATCH v2 4/4] drm/i915: add assert_device_not_suspended to GGTT PTE updaters

[Imre Deak]

On Mon, 2015-11-09 at 21:11 +, Chris Wilson wrote:
> On Mon, Nov 09, 2015 at 09:14:19PM +0200, Imre Deak wrote:
> > The device should be on when updating the GGTT PTEs, so add an assert to
> > all relevant places.
> > 
> > v2:
> > - use the existing dev_priv directly everywhere (Ville)
> > 
> > Signed-off-by: Imre Deak 
> 
> For completeness, add one to i915_ggtt_insert_entries(). Yes, I know
> that we never will support rpm on any of those devices, but we may as
> well be consistent in documenting that these functions will write
> through the magic PCI region.

Ok, forgot about that one.

> An issue I see here is dev_priv->pm.suspended doesn't actually say that
> rpm can't suspend during the GGTT PTEs writes. Would we not want
> something more like assert_rpm_wakelock() and !dev_priv->pm.can_suspend?

Well, after this patchset we assert that the RPM refcount!=0 too, which
should guarantee that provided that pm.suspended=false as well. It's
possible to have a non-zero refcount with a suspended state (in case of
rpm_get_no_resume).

> -Chris
> 


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

Re: [Intel-gfx] [PATCH 2/4] drm/i915: use assert_device_not_suspended instead of opencoding it

[Chris Wilson]

On Mon, Nov 09, 2015 at 08:20:09PM +0200, Imre Deak wrote:
> Signed-off-by: Imre Deak 
Reviewed-by: Chris Wilson 
-Chris

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

Re: [Intel-gfx] [PATCH] drm/i915: A better maximum brightness for users.

[Shih-Yuan Lee (FourDollars)]

Should I improve this patch or you have rejected this patch?

Regards,
$4
On Mon, Nov 09, 2015 at 06:57:22PM +0200, Jani Nikula wrote:
> On Mon, 09 Nov 2015, Paulo Zanoni  wrote:
> > 2015-11-09 8:17 GMT-02:00 Jani Nikula :
> >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)"  
> >> wrote:
> >>> The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937 however
> >>> the sysfs brightness level always starts from 0 so it is better to use
> >>> 927 as the sysfs maximum brightness level and it becomes easier to map
> >>> from the PWM brightness level to the sysfs brightness level.
> >>
> >> We've been thinking we should provide a fixed range to userspace
> >> instead. Say, 0-100.
> >
> > While not clearly stated, this reply and the further ones sound like a
> > rejection to his patch.
> 
> I wanted to understand the motivation rather than bluntly rejecting.
> 
> > I'm not really an expert on backlight, but his idea sounds simple and
> > an overall improvement to the codebase. I don't think it's fair to
> > block a simple one-line improvement based on the fact that we have an
> > idea (that may or may not be implemented) for a possibly better
> > implementation. Why not apply his patch (in case we conclude it
> > doesn't have any bugs), and then go for the 0-100 idea once someone
> > actually decides to implement it?
> 
> The implementation is
> 
> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> b/drivers/gpu/drm/i915/intel_panel.c
> index a24df35e11e7..d5d86601d411 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1211,7 +1211,7 @@ static int intel_backlight_device_register(struct 
> intel_connector *connector)
>* Note: Everything should work even if the backlight device max
>* presented to the userspace is arbitrarily chosen.
>*/
> - props.max_brightness = panel->backlight.max;
> + props.max_brightness = 100;
>   props.brightness = scale_hw_to_user(connector,
>   panel->backlight.level,
>   props.max_brightness);
> 
> and "just" testing is required.
> 
> > Besides, isn't there the possibility of having a panel that has a
> > 0- range where the change from 0 to 9990 is negligible, and only
> > 9991- matters, so when we scale to 0-100 it will become basically
> > a on/off switch? We all learned to never underestimate panel hardware.
> 
> That's a somewhat more valid argument. The answer to that should be that
> we map the userspace range to the hardware range according to a curve
> rather than a linear mapping. See Ville's reply.
> 
> BR,
> Jani.
> 
> 
> 
> >
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >>
> >>
> >>>
> >>> Signed-off-by: Shih-Yuan Lee (FourDollars) 
> >>> ---
> >>>  drivers/gpu/drm/i915/intel_panel.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> >>> b/drivers/gpu/drm/i915/intel_panel.c
> >>> index a24df35..697fd4d 100644
> >>> --- a/drivers/gpu/drm/i915/intel_panel.c
> >>> +++ b/drivers/gpu/drm/i915/intel_panel.c
> >>> @@ -1211,7 +1211,7 @@ static int intel_backlight_device_register(struct 
> >>> intel_connector *connector)
> >>>* Note: Everything should work even if the backlight device max
> >>>* presented to the userspace is arbitrarily chosen.
> >>>*/
> >>> - props.max_brightness = panel->backlight.max;
> >>> + props.max_brightness = panel->backlight.max - panel->backlight.min;
> >>>   props.brightness = scale_hw_to_user(connector,
> >>>   panel->backlight.level,
> >>>   props.max_brightness);
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> >> ___
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> > -- 
> > Paulo Zanoni
> 
> -- 
> 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

[Intel-gfx] [PATCH i-g-t] lib: add a environment variable to control output

[Thomas Wood]

Disable output of terminal control characters and progress meters when
IGT_PLAIN_OUTPUT is set in the environment.

Cc: Derek Morton 
Signed-off-by: Thomas Wood 
---
 lib/igt_aux.c  |  2 +-
 lib/igt_core.c | 23 ++-
 lib/igt_core.h |  2 ++
 3 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/lib/igt_aux.c b/lib/igt_aux.c
index f3c76ae..4d08d68 100644
--- a/lib/igt_aux.c
+++ b/lib/igt_aux.c
@@ -231,7 +231,7 @@ static void igt_interactive_info(const char *format, ...)
 {
va_list args;
 
-   if (!isatty(STDERR_FILENO))
+   if (!isatty(STDERR_FILENO) || __igt_plain_output)
return;
 
if (igt_log_level > IGT_LOG_INFO)
diff --git a/lib/igt_core.c b/lib/igt_core.c
index 7123455..ea9a68b 100644
--- a/lib/igt_core.c
+++ b/lib/igt_core.c
@@ -227,6 +227,8 @@ static enum {
CONT = 0, SKIP, FAIL
 } skip_subtests_henceforth = CONT;
 
+bool __igt_plain_output = false;
+
 /* fork support state */
 pid_t *test_children;
 int num_test_children;
@@ -523,11 +525,15 @@ static int common_init(int *argc, char **argv,
int extra_opt_count;
int all_opt_count;
int ret = 0;
-   char *env = getenv("IGT_LOG_LEVEL");
+   const char *env;
+
+   if (!isatty(STDOUT_FILENO) || getenv("IGT_PLAIN_OUTPUT"))
+   __igt_plain_output = true;
 
-   if (isatty(STDOUT_FILENO))
+   if (!__igt_plain_output)
setlocale(LC_ALL, "");
 
+   env = getenv("IGT_LOG_LEVEL");
if (env) {
if (strcmp(env, "debug") == 0)
igt_log_level = IGT_LOG_DEBUG;
@@ -779,12 +785,10 @@ bool __igt_run_subtest(const char *subtest_name)
}
 
if (skip_subtests_henceforth) {
-   bool istty = isatty(STDOUT_FILENO);
-
printf("%sSubtest %s: %s%s\n",
-  (istty) ? "\x1b[1m" : "", subtest_name,
+  (!__igt_plain_output) ? "\x1b[1m" : "", subtest_name,
   skip_subtests_henceforth == SKIP ?
-  "SKIP" : "FAIL", (istty) ? "\x1b[0m" : "");
+  "SKIP" : "FAIL", (!__igt_plain_output) ? "\x1b[0m" : "");
return false;
}
 
@@ -828,14 +832,15 @@ static void exit_subtest(const char *result)
 {
struct timespec now;
double elapsed;
-   bool istty = isatty(STDOUT_FILENO);
 
gettime();
elapsed = now.tv_sec - subtest_time.tv_sec;
elapsed += (now.tv_nsec - subtest_time.tv_nsec) * 1e-9;
 
-   printf("%sSubtest %s: %s (%.3fs)%s\n", (istty) ? "\x1b[1m" : "",
-  in_subtest, result, elapsed, (istty) ? "\x1b[0m" : "");
+   printf("%sSubtest %s: %s (%.3fs)%s\n",
+  (!__igt_plain_output) ? "\x1b[1m" : "",
+  in_subtest, result, elapsed,
+  (!__igt_plain_output) ? "\x1b[0m" : "");
fflush(stdout);
 
in_subtest = NULL;
diff --git a/lib/igt_core.h b/lib/igt_core.h
index 5ae0965..a244fc3 100644
--- a/lib/igt_core.h
+++ b/lib/igt_core.h
@@ -45,6 +45,8 @@
 
 
 extern const char* __igt_test_description __attribute__((weak));
+extern bool __igt_plain_output;
+
 
 /**
  * IGT_TEST_DESCRIPTION:
-- 
1.9.1

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

[Intel-gfx] [PATCH] drm/i915: Add Backlight Control using DPCD for eDP connectors (v2)

[Yetunde Adebisi]

This patch adds support for eDP backlight control using DPCD registers to
backlight hooks in intel_panel.

It checks for backlight control over AUX channel capability and sets up
function pointers to get and set the backlight brightness level if
supported.

v2: Moved backlight functions from intel_dp.c into a new file
intel_dp_aux_backlight.c. Also moved reading of eDP display control
registers to intel_dp_get_dpcd

This patch depends on http://patchwork.freedesktop.org/patch/64253/

Cc: Bob Paauwe 
Cc: Jani Nikula 
Acked-by: Jesse Barnes 
Signed-off-by: Yetunde Adebisi 
---
 drivers/gpu/drm/i915/Makefile |   1 +
 drivers/gpu/drm/i915/intel_dp.c   |  16 ++-
 drivers/gpu/drm/i915/intel_dp_aux_backlight.c | 178 ++
 drivers/gpu/drm/i915/intel_drv.h  |   6 +
 drivers/gpu/drm/i915/intel_panel.c|   4 +
 5 files changed, 199 insertions(+), 6 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_dp_aux_backlight.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 0851de07..7a1db3d 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -77,6 +77,7 @@ i915-y += dvo_ch7017.o \
  dvo_tfp410.o \
  intel_crt.o \
  intel_ddi.o \
+ intel_dp_aux_backlight.o\
  intel_dp_link_training.o \
  intel_dp_mst.o \
  intel_dp.o \
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5264887..251e869 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3021,7 +3021,7 @@ static void chv_dp_post_pll_disable(struct intel_encoder 
*encoder)
  * Sinks are *supposed* to come up within 1ms from an off state, but we're also
  * supposed to retry 3 times per the spec.
  */
-static ssize_t
+ssize_t
 intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
void *buffer, size_t size)
 {
@@ -3675,7 +3675,6 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
struct drm_device *dev = dig_port->base.base.dev;
struct drm_i915_private *dev_priv = dev->dev_private;
-   uint8_t rev;
 
if (intel_dp_dpcd_read_wake(_dp->aux, 0x000, intel_dp->dpcd,
sizeof(intel_dp->dpcd)) < 0)
@@ -3711,6 +3710,14 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
DRM_DEBUG_KMS("PSR2 %s on sink",
dev_priv->psr.psr2_support ? "supported" : "not 
supported");
}
+
+   /* Read the eDP Display control capabilities registers */
+   if ((intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & 
DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
+   (intel_dp_dpcd_read_wake(_dp->aux, 
DP_EDP_DPCD_REV,
+   intel_dp->dpcd_edp, 
sizeof(intel_dp->dpcd_edp)) ==
+   
sizeof(intel_dp->dpcd_edp)))
+   DRM_DEBUG_KMS("EDP DPCD : %*ph\n", (int) 
sizeof(intel_dp->dpcd_edp),
+   intel_dp->dpcd_edp);
}
 
DRM_DEBUG_KMS("Display Port TPS3 support: source %s, sink %s\n",
@@ -3718,10 +3725,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
  yesno(drm_dp_tps3_supported(intel_dp->dpcd)));
 
/* Intermediate frequency support */
-   if (is_edp(intel_dp) &&
-   (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & 
DP_DPCD_DISPLAY_CONTROL_CAPABLE) &&
-   (intel_dp_dpcd_read_wake(_dp->aux, DP_EDP_DPCD_REV, , 1) 
== 1) &&
-   (rev >= 0x03)) { /* eDp v1.4 or higher */
+   if (is_edp(intel_dp) && (intel_dp->dpcd_edp[0] >= 0x03)) { /* eDp v1.4 
or higher */
__le16 sink_rates[DP_MAX_SUPPORTED_RATES];
int i;
 
diff --git a/drivers/gpu/drm/i915/intel_dp_aux_backlight.c 
b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
new file mode 100644
index 000..ec5fd09
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_dp_aux_backlight.c
@@ -0,0 +1,178 @@
+/*
+ * Copyright © 2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", 

Re: [Intel-gfx] [PATCH] drm/i915: Print a debug message when exceeding dotclock limit on pre-gen4

[Ville Syrjälä]

On Wed, Nov 04, 2015 at 05:41:16PM +0200, Ander Conselvan De Oliveira wrote:
> Reviewed-by: Ander Conselvan de Oliveira 

Pushed to dinq. Thanks for the review.

> On Fri, 2015-10-30 at 23:39 +0200, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > Currently there's no trace in dmesg when the gen2/3 dotclock checks
> > reject the modeset. Add some to avoid further head scratching.
> > 
> > While at it refactor the code a bit to look nicer.
> > 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 26 ++
> >  1 file changed, 18 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 2b70151..1509a99 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -6599,6 +6599,15 @@ static void hsw_compute_ips_config(struct intel_crtc
> > *crtc,
> > pipe_config_supports_ips(dev_priv, pipe_config);
> >  }
> >  
> > +static bool intel_crtc_supports_double_wide(const struct intel_crtc *crtc)
> > +{
> > +   const struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> > +
> > +   /* GDG double wide on either pipe, otherwise pipe A only */
> > +   return INTEL_INFO(dev_priv)->gen < 4 &&
> > +   (crtc->pipe == PIPE_A || IS_I915G(dev_priv));
> > +}
> > +
> >  static int intel_crtc_compute_config(struct intel_crtc *crtc,
> >  struct intel_crtc_state *pipe_config)
> >  {
> > @@ -6608,23 +6617,24 @@ static int intel_crtc_compute_config(struct 
> > intel_crtc
> > *crtc,
> >  
> > /* FIXME should check pixel clock limits on all platforms */
> > if (INTEL_INFO(dev)->gen < 4) {
> > -   int clock_limit = dev_priv->max_cdclk_freq;
> > +   int clock_limit = dev_priv->max_cdclk_freq * 9 / 10;
> >  
> > /*
> > -* Enable pixel doubling when the dot clock
> > +* Enable double wide mode when the dot clock
> >  * is > 90% of the (display) core speed.
> > -*
> > -* GDG double wide on either pipe,
> > -* otherwise pipe A only.
> >  */
> > -   if ((crtc->pipe == PIPE_A || IS_I915G(dev)) &&
> > -   adjusted_mode->crtc_clock > clock_limit * 9 / 10) {
> > +   if (intel_crtc_supports_double_wide(crtc) &&
> > +   adjusted_mode->crtc_clock > clock_limit) {
> > clock_limit *= 2;
> > pipe_config->double_wide = true;
> > }
> >  
> > -   if (adjusted_mode->crtc_clock > clock_limit * 9 / 10)
> > +   if (adjusted_mode->crtc_clock > clock_limit) {
> > +   DRM_DEBUG_KMS("requested pixel clock (%d kHz) too
> > high (max: %d kHz, double wide: %s)\n",
> > + adjusted_mode->crtc_clock, clock_limit,
> > + yesno(pipe_config->double_wide));
> > return -EINVAL;
> > +   }
> > }
> >  
> > /*

-- 
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 06/14] drm/i915: Update watermark related members in the crtc_state, v2.

[Ville Syrjälä]

On Mon, Nov 09, 2015 at 05:10:40PM +0100, Maarten Lankhorst wrote:
> Op 09-11-15 om 15:48 schreef Ander Conselvan De Oliveira:
> > On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> >> This removes another couple of hacks from intel_crtc->atomic, and
> >> creates proper atomic state for it.
> > Perhaps you could be more verbose about the hacks being removed.
> Right!
> >> Changes since v1:
> >> - Rebase on top of wm changes.
> >>
> >> Signed-off-by: Maarten Lankhorst 
> >> ---
> >>  drivers/gpu/drm/i915/intel_atomic.c  |  2 ++
> >>  drivers/gpu/drm/i915/intel_display.c | 41 
> >> +--
> >> -
> >>  drivers/gpu/drm/i915/intel_drv.h |  6 +++---
> >>  3 files changed, 24 insertions(+), 25 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
> >> b/drivers/gpu/drm/i915/intel_atomic.c
> >> index c4eadbc928b7..3e390db9d22b 100644
> >> --- a/drivers/gpu/drm/i915/intel_atomic.c
> >> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> >> @@ -95,6 +95,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
> >>  
> >>crtc_state->update_pipe = false;
> >>crtc_state->disable_lp_wm = false;
> >> +  crtc_state->visible_changed = false;
> >> +  crtc_state->wm_changed = false;
> >>  
> >>return _state->base;
> >>  }
> >> diff --git a/drivers/gpu/drm/i915/intel_display.c
> >> b/drivers/gpu/drm/i915/intel_display.c
> >> index 54e4f04bb427..356e3a9e1741 100644
> >> --- a/drivers/gpu/drm/i915/intel_display.c
> >> +++ b/drivers/gpu/drm/i915/intel_display.c
> >> @@ -4753,6 +4753,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
> >>  static void intel_post_plane_update(struct intel_crtc *crtc)
> >>  {
> >>struct intel_crtc_atomic_commit *atomic = >atomic;
> >> +  struct intel_crtc_state *pipe_config =
> >> +  to_intel_crtc_state(crtc->base.state);
> >>struct drm_device *dev = crtc->base.dev;
> >>struct drm_i915_private *dev_priv = dev->dev_private;
> >>  
> >> @@ -4761,10 +4763,9 @@ static void intel_post_plane_update(struct 
> >> intel_crtc
> >> *crtc)
> >>  
> >>intel_frontbuffer_flip(dev, atomic->fb_bits);
> >>  
> >> -  if (atomic->disable_cxsr)
> >> -  crtc->wm.cxsr_allowed = true;
> >> +  crtc->wm.cxsr_allowed = true;
> >>  
> >> -  if (crtc->atomic.update_wm_post)
> >> +  if (pipe_config->wm_changed)
> >>intel_update_watermarks(>base);
> >>  
> >>if (atomic->update_fbc)
> >> @@ -4781,6 +4782,8 @@ static void intel_pre_plane_update(struct intel_crtc
> >> *crtc)
> >>struct drm_device *dev = crtc->base.dev;
> >>struct drm_i915_private *dev_priv = dev->dev_private;
> >>struct intel_crtc_atomic_commit *atomic = >atomic;
> >> +  struct intel_crtc_state *pipe_config =
> >> +  to_intel_crtc_state(crtc->base.state);
> >>  
> >>if (atomic->disable_fbc)
> >>intel_fbc_disable_crtc(crtc);
> >> @@ -4791,10 +4794,13 @@ static void intel_pre_plane_update(struct 
> >> intel_crtc
> >> *crtc)
> >>if (atomic->pre_disable_primary)
> >>intel_pre_disable_primary(>base);
> >>  
> >> -  if (atomic->disable_cxsr) {
> >> +  if (pipe_config->visible_changed) {
> >>crtc->wm.cxsr_allowed = false;
> >>intel_set_memory_cxsr(dev_priv, false);
> >>}
> >> +
> >> +  if (!needs_modeset(_config->base) && pipe_config->wm_changed)
> >> +  intel_update_watermarks(>base);
> >>  }
> >>  
> >>  static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned
> >> plane_mask)
> >> @@ -11617,6 +11623,7 @@ static bool needs_scaling(struct intel_plane_state
> >> *state)
> >>  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
> >>struct drm_plane_state *plane_state)
> >>  {
> >> +  struct intel_crtc_state *pipe_config =
> >> to_intel_crtc_state(crtc_state);
> >>struct drm_crtc *crtc = crtc_state->crtc;
> >>struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> >>struct drm_plane *plane = plane_state->plane;
> >> @@ -11663,25 +11670,18 @@ int intel_plane_atomic_calc_changes(struct
> >> drm_crtc_state *crtc_state,
> >> plane->base.id, was_visible, visible,
> >> turn_off, turn_on, mode_changed);
> >>  
> >> -  if (turn_on) {
> >> -  intel_crtc->atomic.update_wm_pre = true;
> >> -  /* must disable cxsr around plane enable/disable */
> >> -  if (plane->type != DRM_PLANE_TYPE_CURSOR) {
> >> -  intel_crtc->atomic.disable_cxsr = true;
> >> -  /* to potentially re-enable cxsr */
> >> -  intel_crtc->atomic.wait_vblank = true;
> >> -  intel_crtc->atomic.update_wm_post = true;
> >> -  }
> >> -  } else if (turn_off) {
> >> -  intel_crtc->atomic.update_wm_post = true;
> >> +  if (turn_on || turn_off) {
> >> +  pipe_config->wm_changed = true;
> >> +
> >>/* must disable cxsr around plane enable/disable */
> >>

Re: [Intel-gfx] [PATCH 4/4] drm/i915: add assert_device_not_suspended to GGTT PTE updaters

[Imre Deak]

On ma, 2015-11-09 at 20:37 +0200, Ville Syrjälä wrote:
> On Mon, Nov 09, 2015 at 08:20:11PM +0200, Imre Deak wrote:
> > The device should be on when updating the GGTT PTEs, so add an
> > assert to
> > all relevant places.
> > 
> > Signed-off-by: Imre Deak 
> > ---
> >  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > index 016739e..d4a84b8 100644
> > --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> > +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> > @@ -2358,6 +2358,8 @@ static void gen8_ggtt_insert_entries(struct
> > i915_address_space *vm,
> > struct sg_page_iter sg_iter;
> > dma_addr_t addr = 0; /* shut up gcc */
> >  
> > +   assert_device_not_suspended(vm->dev->dev_private);
> > +
> 
> We do have the GFX_FLSH_CNTL_GEN6 write in the insert_entries hooks,
> which should already do the check. But I see no harm in doing it
> explicitly as well. Serves as documentation if nothing more. The
> clear_range hooks don't do any register access, so there we should
> definitely have this.

Yes, I think it's better for symmetry with the other updaters. And it's
a WARN_ONCE, so shouldn't add noise.

> 
> > for_each_sg_page(st->sgl, _iter, st->nents, 0) {
> > addr = sg_dma_address(sg_iter.sg) +
> > (sg_iter.sg_pgoffset << PAGE_SHIFT);
> > @@ -2404,6 +2406,8 @@ static void gen6_ggtt_insert_entries(struct
> > i915_address_space *vm,
> > struct sg_page_iter sg_iter;
> > dma_addr_t addr = 0;
> >  
> > +   assert_device_not_suspended(vm->dev->dev_private);
> 
> There's already a dev_priv around in all of these AFAICS.

Yep, just copy pasted these.. Will fix them.

> 
> > +
> > for_each_sg_page(st->sgl, _iter, st->nents, 0) {
> > addr = sg_page_iter_dma_address(_iter);
> > iowrite32(vm->pte_encode(addr, level, true,
> > flags), _entries[i]);
> > @@ -2442,6 +2446,8 @@ static void gen8_ggtt_clear_range(struct
> > i915_address_space *vm,
> > const int max_entries = gtt_total_entries(dev_priv->gtt) -
> > first_entry;
> > int i;
> >  
> > +   assert_device_not_suspended(vm->dev->dev_private);
> > +
> > if (WARN(num_entries > max_entries,
> >  "First entry = %d; Num entries = %d (max=%d)\n",
> >  first_entry, num_entries, max_entries))
> > @@ -2468,6 +2474,8 @@ static void gen6_ggtt_clear_range(struct
> > i915_address_space *vm,
> > const int max_entries = gtt_total_entries(dev_priv->gtt) -
> > first_entry;
> > int i;
> >  
> > +   assert_device_not_suspended(vm->dev->dev_private);
> > +
> > if (WARN(num_entries > max_entries,
> >  "First entry = %d; Num entries = %d (max=%d)\n",
> >  first_entry, num_entries, max_entries))
> > -- 
> > 2.5.0
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/7] drm/i915/skl: Store and print the DMC firmware version we load

[Ville Syrjälä]

On Wed, Nov 04, 2015 at 05:18:28PM +, Daniel Stone wrote:
> Hi,
> 
> On 27 October 2015 at 12:46, Mika Kuoppala
>  wrote:
> > From: Damien Lespiau 
> >
> > That can be handy later on to tell which DMC firmware version the user
> > has, by just looking at the dmesg.
> >
> > v2: use DRM_DEBUG_DRIVER (Chris)
> > v3: use DRM_INFO (Marc Herbert)
> 
> For the series (all 7):
> Tested-by: Daniel Stone  # SKL

Entire series merged. Thanks for patches, reviews and testing.

There was no trace on the mailing list where Imre's r-b on patch 4/7
came from, but I confirmed with Imre that it was given on irc.

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

[Intel-gfx] [PATCH] drm/i915: get runtime PM reference around GEM set_tiling IOCTL

[Imre Deak]

After fixing the same issue in the set_caching IOCTL and Chris' request
to check out the possibilities for an improved RPM ref handling I
noticed that we have the same issue in the set_tiling IOCTL. Fix this
up.I didn't see any bug reports about this one, but the GTT unbind
operation on this path accesses the HW, which needs the ref.

Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/i915/i915_gem_tiling.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_tiling.c 
b/drivers/gpu/drm/i915/i915_gem_tiling.c
index 8a6717c..7410f6c 100644
--- a/drivers/gpu/drm/i915/i915_gem_tiling.c
+++ b/drivers/gpu/drm/i915/i915_gem_tiling.c
@@ -176,6 +176,8 @@ i915_gem_set_tiling(struct drm_device *dev, void *data,
return -EINVAL;
}
 
+   intel_runtime_pm_get(dev_priv);
+
mutex_lock(>struct_mutex);
if (obj->pin_display || obj->framebuffer_references) {
ret = -EBUSY;
@@ -269,6 +271,8 @@ err:
drm_gem_object_unreference(>base);
mutex_unlock(>struct_mutex);
 
+   intel_runtime_pm_put(dev_priv);
+
return ret;
 }
 
-- 
2.5.0

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

Re: [Intel-gfx] [PATCH] drm/i915: Avoid pointer arithmetic in calculating plane surface offset

[Ville Syrjälä]

On Fri, Oct 30, 2015 at 11:40:46AM +, Tvrtko Ursulin wrote:
> 
> On 30/10/15 11:26, Mika Kuoppala wrote:
> > VMA offsets are 64 bits. Plane surface offsets are in ggtt and
> > the hardware register to set this is thus 32 bits. Be explicit
> > about these and convert carefully to from vma to final size.
> >
> > This will make sparse happy by not creating 32bit pointers out
> > of 64bit vma offsets.
> >
> > Cc: Tvrtko Ursulin 
> > Signed-off-by: Mika Kuoppala 
> > ---
> >

> 
> Reviewed-by: Tvrtko Ursulin 

Pushed to dinq. Thanks for the patch and review.

-- 
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 1/4] drm/i915: export assert_device_not_suspended

[Ville Syrjälä]

On Mon, Nov 09, 2015 at 08:20:08PM +0200, Imre Deak wrote:
> We should use the same assert in more places, so export it. Move it to
> intel_runtime_pm.c at the same time, since that's a more logical place
> for it.
> 
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/intel_drv.h| 2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 6 ++
>  drivers/gpu/drm/i915/intel_uncore.c | 7 ---
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index f9089df..373cc07 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1415,6 +1415,8 @@ 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 assert_device_not_suspended(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_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b42506b..0d4a03b 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2120,6 +2120,12 @@ void intel_power_domains_init_hw(struct 
> drm_i915_private *dev_priv, bool resume)
>   power_domains->initializing = false;
>  }
>  
> +void assert_device_not_suspended(struct drm_i915_private *dev_priv)
> +{
> + WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,

Is there any point in the HAS_RUNTIME_PM() check?

> +   "Device suspended\n");
> +}
> +
>  /**
>   * intel_power_domains_suspend - suspend power domain state
>   * @dev_priv: i915 device instance
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
> b/drivers/gpu/drm/i915/intel_uncore.c
> index 5bb269c..2b93a68 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -50,13 +50,6 @@ intel_uncore_forcewake_domain_to_str(const enum 
> forcewake_domain_id id)
>   return "unknown";
>  }
>  
> -static void
> -assert_device_not_suspended(struct drm_i915_private *dev_priv)
> -{
> - WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
> -   "Device suspended\n");
> -}
> -
>  static inline void
>  fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
>  {
> -- 
> 2.5.0
> 
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Intel-gfx] [PATCH 1/4] drm/i915: export assert_device_not_suspended

[Imre Deak]

We should use the same assert in more places, so export it. Move it to
intel_runtime_pm.c at the same time, since that's a more logical place
for it.

Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/i915/intel_drv.h| 2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 6 ++
 drivers/gpu/drm/i915/intel_uncore.c | 7 ---
 3 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f9089df..373cc07 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1415,6 +1415,8 @@ 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 assert_device_not_suspended(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_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index b42506b..0d4a03b 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2120,6 +2120,12 @@ void intel_power_domains_init_hw(struct drm_i915_private 
*dev_priv, bool resume)
power_domains->initializing = false;
 }
 
+void assert_device_not_suspended(struct drm_i915_private *dev_priv)
+{
+   WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
+ "Device suspended\n");
+}
+
 /**
  * intel_power_domains_suspend - suspend power domain state
  * @dev_priv: i915 device instance
diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
b/drivers/gpu/drm/i915/intel_uncore.c
index 5bb269c..2b93a68 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -50,13 +50,6 @@ intel_uncore_forcewake_domain_to_str(const enum 
forcewake_domain_id id)
return "unknown";
 }
 
-static void
-assert_device_not_suspended(struct drm_i915_private *dev_priv)
-{
-   WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
- "Device suspended\n");
-}
-
 static inline void
 fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
 {
-- 
2.5.0

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

[Intel-gfx] [PATCH 3/4] drm/i915: make assert_device_not_suspended more precise

[Imre Deak]

Atm, we assert that the device is not suspended after the point when the
HW is truly put to a suspended state. This is fine, but we can catch
more problems if we check the RPM refcount. After that one drops to zero
we shouldn't access the HW any more, although the actual suspend may be
delayed. The only complication is that we want to avoid asserts while
the suspend handler itself is running, so add a flag to handle this
case.

This caught additional WARNs from the atomic path, those will be fixed
as a follow-up.

Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/i915/i915_drv.c |  5 +
 drivers/gpu/drm/i915/i915_drv.h |  5 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 14 --
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 77d183d..caeb218 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1494,6 +1494,9 @@ static int intel_runtime_suspend(struct device *device)
 
return -EAGAIN;
}
+
+   dev_priv->pm.disable_suspended_assert = true;
+
/*
 * We are safe here against re-faults, since the fault handler takes
 * an RPM reference.
@@ -1518,6 +1521,8 @@ static int intel_runtime_suspend(struct device *device)
intel_uncore_forcewake_reset(dev, false);
dev_priv->pm.suspended = true;
 
+   dev_priv->pm.disable_suspended_assert = false;
+
/*
 * FIXME: We really should find a document that references the arguments
 * used below!
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5628c5a..43fd341 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1599,6 +1599,11 @@ struct skl_wm_level {
  * For more, read the Documentation/power/runtime_pm.txt.
  */
 struct i915_runtime_pm {
+   /*
+* Used for the duration of runtime suspend to avoid false device
+* suspended asserts.
+*/
+   bool disable_suspended_assert;
bool suspended;
bool irqs_enabled;
 };
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 4d39b3c..2e2083c 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2120,8 +2120,18 @@ void intel_power_domains_init_hw(struct drm_i915_private 
*dev_priv, bool resume)
 
 void assert_device_not_suspended(struct drm_i915_private *dev_priv)
 {
-   WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
- "Device suspended\n");
+   int rpm_usage;
+
+   if (!HAS_RUNTIME_PM(dev_priv) || dev_priv->pm.disable_suspended_assert)
+   return;
+
+#ifdef CONFIG_PM
+   rpm_usage = atomic_read(_priv->dev->dev->power.usage_count);
+#else
+   rpm_usage = 1;
+#endif
+
+   WARN_ONCE(dev_priv->pm.suspended || !rpm_usage, "Device suspended\n");
 }
 
 /**
-- 
2.5.0

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

[Intel-gfx] [PATCH 2/4] drm/i915: use assert_device_not_suspended instead of opencoding it

[Imre Deak]

Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 10 --
 drivers/gpu/drm/i915/intel_uncore.c |  2 +-
 2 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 0d4a03b..4d39b3c 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -464,8 +464,7 @@ static void assert_can_enable_dc5(struct drm_i915_private 
*dev_priv)
 
WARN_ONCE((I915_READ(DC_STATE_EN) & DC_STATE_EN_UPTO_DC5),
  "DC5 already programmed to be enabled.\n");
-   WARN_ONCE(dev_priv->pm.suspended,
- "DC5 cannot be enabled, if platform is runtime-suspended.\n");
+   assert_device_not_suspended(dev_priv);
 
assert_csr_loaded(dev_priv);
 }
@@ -479,8 +478,7 @@ static void assert_can_disable_dc5(struct drm_i915_private 
*dev_priv)
if (dev_priv->power_domains.initializing)
return;
 
-   WARN_ONCE(dev_priv->pm.suspended,
-   "Disabling of DC5 while platform is runtime-suspended should 
never happen.\n");
+   assert_device_not_suspended(dev_priv);
 }
 
 static void gen9_enable_dc5(struct drm_i915_private *dev_priv)
@@ -2158,7 +2156,7 @@ void intel_runtime_pm_get(struct drm_i915_private 
*dev_priv)
return;
 
pm_runtime_get_sync(device);
-   WARN(dev_priv->pm.suspended, "Device still suspended.\n");
+   assert_device_not_suspended(dev_priv);
 }
 
 /**
@@ -2186,7 +2184,7 @@ void intel_runtime_pm_get_noresume(struct 
drm_i915_private *dev_priv)
if (!HAS_RUNTIME_PM(dev))
return;
 
-   WARN(dev_priv->pm.suspended, "Getting nosync-ref while suspended.\n");
+   assert_device_not_suspended(dev_priv);
pm_runtime_get_noresume(device);
 }
 
diff --git a/drivers/gpu/drm/i915/intel_uncore.c 
b/drivers/gpu/drm/i915/intel_uncore.c
index 2b93a68..b4aea91 100644
--- a/drivers/gpu/drm/i915/intel_uncore.c
+++ b/drivers/gpu/drm/i915/intel_uncore.c
@@ -404,7 +404,7 @@ void intel_uncore_forcewake_get(struct drm_i915_private 
*dev_priv,
if (!dev_priv->uncore.funcs.force_wake_get)
return;
 
-   WARN_ON(dev_priv->pm.suspended);
+   assert_device_not_suspended(dev_priv);
 
spin_lock_irqsave(_priv->uncore.lock, irqflags);
__intel_uncore_forcewake_get(dev_priv, fw_domains);
-- 
2.5.0

___
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 assert_device_not_suspended to GGTT PTE updaters

[Imre Deak]

The device should be on when updating the GGTT PTEs, so add an assert to
all relevant places.

Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 016739e..d4a84b8 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2358,6 +2358,8 @@ static void gen8_ggtt_insert_entries(struct 
i915_address_space *vm,
struct sg_page_iter sg_iter;
dma_addr_t addr = 0; /* shut up gcc */
 
+   assert_device_not_suspended(vm->dev->dev_private);
+
for_each_sg_page(st->sgl, _iter, st->nents, 0) {
addr = sg_dma_address(sg_iter.sg) +
(sg_iter.sg_pgoffset << PAGE_SHIFT);
@@ -2404,6 +2406,8 @@ static void gen6_ggtt_insert_entries(struct 
i915_address_space *vm,
struct sg_page_iter sg_iter;
dma_addr_t addr = 0;
 
+   assert_device_not_suspended(vm->dev->dev_private);
+
for_each_sg_page(st->sgl, _iter, st->nents, 0) {
addr = sg_page_iter_dma_address(_iter);
iowrite32(vm->pte_encode(addr, level, true, flags), 
_entries[i]);
@@ -2442,6 +2446,8 @@ static void gen8_ggtt_clear_range(struct 
i915_address_space *vm,
const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
int i;
 
+   assert_device_not_suspended(vm->dev->dev_private);
+
if (WARN(num_entries > max_entries,
 "First entry = %d; Num entries = %d (max=%d)\n",
 first_entry, num_entries, max_entries))
@@ -2468,6 +2474,8 @@ static void gen6_ggtt_clear_range(struct 
i915_address_space *vm,
const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
int i;
 
+   assert_device_not_suspended(vm->dev->dev_private);
+
if (WARN(num_entries > max_entries,
 "First entry = %d; Num entries = %d (max=%d)\n",
 first_entry, num_entries, max_entries))
-- 
2.5.0

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

[Intel-gfx] [PATCH 0/4] drm/i915: improve the RPM device suspended assert

[Imre Deak]

Motivated by the discussions with Chris about imrpoving our RPM ref
get/put logic and seeing that we are still missing RPM refs from
places I improved a bit the assert that checks if the device is powered
on while we are accessing the HW. This produced at least one additional
assert on the atomic commit path, so I think it's useful.

This is based on Patrik's DC rework patches [1], but just to avoid merge
conflicts, functionally it doesn't depend on that.

Tested on SKL.

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

Imre Deak (4):
  drm/i915: export assert_device_not_suspended
  drm/i915: use assert_device_not_suspended instead of opencoding it
  drm/i915: make assert_device_not_suspended more precise
  drm/i915: add assert_device_not_suspended to GGTT PTE updaters

 drivers/gpu/drm/i915/i915_drv.c |  5 +
 drivers/gpu/drm/i915/i915_drv.h |  5 +
 drivers/gpu/drm/i915/i915_gem_gtt.c |  8 
 drivers/gpu/drm/i915/intel_drv.h|  2 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 26 --
 drivers/gpu/drm/i915/intel_uncore.c |  9 +
 6 files changed, 41 insertions(+), 14 deletions(-)

-- 
2.5.0

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

Re: [Intel-gfx] [PATCH 1/4] drm/i915: export assert_device_not_suspended

[Imre Deak]

On ma, 2015-11-09 at 20:30 +0200, Ville Syrjälä wrote:
> On Mon, Nov 09, 2015 at 08:20:08PM +0200, Imre Deak wrote:
> > We should use the same assert in more places, so export it. Move it
> > to
> > intel_runtime_pm.c at the same time, since that's a more logical
> > place
> > for it.
> > 
> > Signed-off-by: Imre Deak 
> > ---
> >  drivers/gpu/drm/i915/intel_drv.h| 2 ++
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 6 ++
> >  drivers/gpu/drm/i915/intel_uncore.c | 7 ---
> >  3 files changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index f9089df..373cc07 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1415,6 +1415,8 @@ 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 assert_device_not_suspended(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_runtime_pm.c
> > b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index b42506b..0d4a03b 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -2120,6 +2120,12 @@ void intel_power_domains_init_hw(struct
> > drm_i915_private *dev_priv, bool resume)
> > power_domains->initializing = false;
> >  }
> >  
> > +void assert_device_not_suspended(struct drm_i915_private
> > *dev_priv)
> > +{
> > +   WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv
> > ->pm.suspended,
> 
> Is there any point in the HAS_RUNTIME_PM() check?

All other platforms should have pm.suspended = false and a non-zero
refcount all the time, so I guess not. I can remove it in patch 3/4.

> 
> > + "Device suspended\n");
> > +}
> > +
> >  /**
> >   * intel_power_domains_suspend - suspend power domain state
> >   * @dev_priv: i915 device instance
> > diff --git a/drivers/gpu/drm/i915/intel_uncore.c
> > b/drivers/gpu/drm/i915/intel_uncore.c
> > index 5bb269c..2b93a68 100644
> > --- a/drivers/gpu/drm/i915/intel_uncore.c
> > +++ b/drivers/gpu/drm/i915/intel_uncore.c
> > @@ -50,13 +50,6 @@ intel_uncore_forcewake_domain_to_str(const enum
> > forcewake_domain_id id)
> > return "unknown";
> >  }
> >  
> > -static void
> > -assert_device_not_suspended(struct drm_i915_private *dev_priv)
> > -{
> > -   WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv
> > ->pm.suspended,
> > - "Device suspended\n");
> > -}
> > -
> >  static inline void
> >  fw_domain_reset(const struct intel_uncore_forcewake_domain *d)
> >  {
> > -- 
> > 2.5.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

[Intel-gfx] [PATCH 1/2] drm/dp: Add definition for Display Control DPCD Registers capability size

[Yetunde Adebisi]

cc: Jani Nikula 
Signed-off-by: Yetunde Adebisi 
---
 include/drm/drm_dp_helper.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h
index 1252108..92d9a52 100644
--- a/include/drm/drm_dp_helper.h
+++ b/include/drm/drm_dp_helper.h
@@ -621,6 +621,7 @@ u8 drm_dp_get_adjust_request_pre_emphasis(const u8 
link_status[DP_LINK_STATUS_SI
 #define DP_BRANCH_OUI_HEADER_SIZE  0xc
 #define DP_RECEIVER_CAP_SIZE   0xf
 #define EDP_PSR_RECEIVER_CAP_SIZE  2
+#define EDP_DISPLAY_CTL_CAP_SIZE   3
 
 void drm_dp_link_train_clock_recovery_delay(const u8 
dpcd[DP_RECEIVER_CAP_SIZE]);
 void drm_dp_link_train_channel_eq_delay(const u8 dpcd[DP_RECEIVER_CAP_SIZE]);
-- 
1.9.3

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

Re: [Intel-gfx] [PATCH 4/4] drm/i915: add assert_device_not_suspended to GGTT PTE updaters

[Ville Syrjälä]

On Mon, Nov 09, 2015 at 08:20:11PM +0200, Imre Deak wrote:
> The device should be on when updating the GGTT PTEs, so add an assert to
> all relevant places.
> 
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 8 
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
> b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 016739e..d4a84b8 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -2358,6 +2358,8 @@ static void gen8_ggtt_insert_entries(struct 
> i915_address_space *vm,
>   struct sg_page_iter sg_iter;
>   dma_addr_t addr = 0; /* shut up gcc */
>  
> + assert_device_not_suspended(vm->dev->dev_private);
> +

We do have the GFX_FLSH_CNTL_GEN6 write in the insert_entries hooks,
which should already do the check. But I see no harm in doing it
explicitly as well. Serves as documentation if nothing more. The
clear_range hooks don't do any register access, so there we should
definitely have this.

>   for_each_sg_page(st->sgl, _iter, st->nents, 0) {
>   addr = sg_dma_address(sg_iter.sg) +
>   (sg_iter.sg_pgoffset << PAGE_SHIFT);
> @@ -2404,6 +2406,8 @@ static void gen6_ggtt_insert_entries(struct 
> i915_address_space *vm,
>   struct sg_page_iter sg_iter;
>   dma_addr_t addr = 0;
>  
> + assert_device_not_suspended(vm->dev->dev_private);

There's already a dev_priv around in all of these AFAICS.

> +
>   for_each_sg_page(st->sgl, _iter, st->nents, 0) {
>   addr = sg_page_iter_dma_address(_iter);
>   iowrite32(vm->pte_encode(addr, level, true, flags), 
> _entries[i]);
> @@ -2442,6 +2446,8 @@ static void gen8_ggtt_clear_range(struct 
> i915_address_space *vm,
>   const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
>   int i;
>  
> + assert_device_not_suspended(vm->dev->dev_private);
> +
>   if (WARN(num_entries > max_entries,
>"First entry = %d; Num entries = %d (max=%d)\n",
>first_entry, num_entries, max_entries))
> @@ -2468,6 +2474,8 @@ static void gen6_ggtt_clear_range(struct 
> i915_address_space *vm,
>   const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
>   int i;
>  
> + assert_device_not_suspended(vm->dev->dev_private);
> +
>   if (WARN(num_entries > max_entries,
>"First entry = %d; Num entries = %d (max=%d)\n",
>first_entry, num_entries, max_entries))
> -- 
> 2.5.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 06/14] drm/i915: Update watermark related members in the crtc_state, v2.

[Maarten Lankhorst]

Op 09-11-15 om 15:48 schreef Ander Conselvan De Oliveira:
> On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
>> This removes another couple of hacks from intel_crtc->atomic, and
>> creates proper atomic state for it.
> Perhaps you could be more verbose about the hacks being removed.
Right!
>> Changes since v1:
>> - Rebase on top of wm changes.
>>
>> Signed-off-by: Maarten Lankhorst 
>> ---
>>  drivers/gpu/drm/i915/intel_atomic.c  |  2 ++
>>  drivers/gpu/drm/i915/intel_display.c | 41 
>> +--
>> -
>>  drivers/gpu/drm/i915/intel_drv.h |  6 +++---
>>  3 files changed, 24 insertions(+), 25 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_atomic.c
>> b/drivers/gpu/drm/i915/intel_atomic.c
>> index c4eadbc928b7..3e390db9d22b 100644
>> --- a/drivers/gpu/drm/i915/intel_atomic.c
>> +++ b/drivers/gpu/drm/i915/intel_atomic.c
>> @@ -95,6 +95,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc)
>>  
>>  crtc_state->update_pipe = false;
>>  crtc_state->disable_lp_wm = false;
>> +crtc_state->visible_changed = false;
>> +crtc_state->wm_changed = false;
>>  
>>  return _state->base;
>>  }
>> diff --git a/drivers/gpu/drm/i915/intel_display.c
>> b/drivers/gpu/drm/i915/intel_display.c
>> index 54e4f04bb427..356e3a9e1741 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4753,6 +4753,8 @@ intel_pre_disable_primary(struct drm_crtc *crtc)
>>  static void intel_post_plane_update(struct intel_crtc *crtc)
>>  {
>>  struct intel_crtc_atomic_commit *atomic = >atomic;
>> +struct intel_crtc_state *pipe_config =
>> +to_intel_crtc_state(crtc->base.state);
>>  struct drm_device *dev = crtc->base.dev;
>>  struct drm_i915_private *dev_priv = dev->dev_private;
>>  
>> @@ -4761,10 +4763,9 @@ static void intel_post_plane_update(struct intel_crtc
>> *crtc)
>>  
>>  intel_frontbuffer_flip(dev, atomic->fb_bits);
>>  
>> -if (atomic->disable_cxsr)
>> -crtc->wm.cxsr_allowed = true;
>> +crtc->wm.cxsr_allowed = true;
>>  
>> -if (crtc->atomic.update_wm_post)
>> +if (pipe_config->wm_changed)
>>  intel_update_watermarks(>base);
>>  
>>  if (atomic->update_fbc)
>> @@ -4781,6 +4782,8 @@ static void intel_pre_plane_update(struct intel_crtc
>> *crtc)
>>  struct drm_device *dev = crtc->base.dev;
>>  struct drm_i915_private *dev_priv = dev->dev_private;
>>  struct intel_crtc_atomic_commit *atomic = >atomic;
>> +struct intel_crtc_state *pipe_config =
>> +to_intel_crtc_state(crtc->base.state);
>>  
>>  if (atomic->disable_fbc)
>>  intel_fbc_disable_crtc(crtc);
>> @@ -4791,10 +4794,13 @@ static void intel_pre_plane_update(struct intel_crtc
>> *crtc)
>>  if (atomic->pre_disable_primary)
>>  intel_pre_disable_primary(>base);
>>  
>> -if (atomic->disable_cxsr) {
>> +if (pipe_config->visible_changed) {
>>  crtc->wm.cxsr_allowed = false;
>>  intel_set_memory_cxsr(dev_priv, false);
>>  }
>> +
>> +if (!needs_modeset(_config->base) && pipe_config->wm_changed)
>> +intel_update_watermarks(>base);
>>  }
>>  
>>  static void intel_crtc_disable_planes(struct drm_crtc *crtc, unsigned
>> plane_mask)
>> @@ -11617,6 +11623,7 @@ static bool needs_scaling(struct intel_plane_state
>> *state)
>>  int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>>  struct drm_plane_state *plane_state)
>>  {
>> +struct intel_crtc_state *pipe_config =
>> to_intel_crtc_state(crtc_state);
>>  struct drm_crtc *crtc = crtc_state->crtc;
>>  struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
>>  struct drm_plane *plane = plane_state->plane;
>> @@ -11663,25 +11670,18 @@ int intel_plane_atomic_calc_changes(struct
>> drm_crtc_state *crtc_state,
>>   plane->base.id, was_visible, visible,
>>   turn_off, turn_on, mode_changed);
>>  
>> -if (turn_on) {
>> -intel_crtc->atomic.update_wm_pre = true;
>> -/* must disable cxsr around plane enable/disable */
>> -if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>> -intel_crtc->atomic.disable_cxsr = true;
>> -/* to potentially re-enable cxsr */
>> -intel_crtc->atomic.wait_vblank = true;
>> -intel_crtc->atomic.update_wm_post = true;
>> -}
>> -} else if (turn_off) {
>> -intel_crtc->atomic.update_wm_post = true;
>> +if (turn_on || turn_off) {
>> +pipe_config->wm_changed = true;
>> +
>>  /* must disable cxsr around plane enable/disable */
>>  if (plane->type != DRM_PLANE_TYPE_CURSOR) {
>> +pipe_config->visible_changed = true;
>>  if (is_crtc_enabled)
>>  

[Intel-gfx] [PATCH 01/12] drm/i915: Don't trust CSR program memory contents

[Patrik Jakobsson]

Replaces "drm/i915: Force loading of csr program at boot" in the old
series.

Previously we called blindly into intel_csr_load_program() and depended
on a check of whether the CSR program memory was cleared or not.
This check is not reliable and no longer needed since we fixed the
call-sites of intel_csr_load_program().

Signed-off-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/intel_csr.c | 10 +++---
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index ecb7c70..ad8bc7a 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -220,14 +220,10 @@ void intel_csr_load_program(struct drm_i915_private 
*dev_priv)
return;
}
 
-   /*
-* FIXME: Firmware gets lost on S3/S4, but not when entering system
-* standby or suspend-to-idle (which is just like forced runtime pm).
-* 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 (!dev_priv->csr.dmc_payload) {
+   DRM_ERROR("Tried to program CSR with empty payload\n");
return;
+   }
 
fw_size = dev_priv->csr.dmc_fw_size;
for (i = 0; i < fw_size; i++)
-- 
2.5.0

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

[Intel-gfx] [PATCH] drm/i915: Change context lifecycle

[Nick Hoath]

Use the first retired request on a new context to unpin
the old context. This ensures that the hw context remains
bound until it has been saved.
Now that the context is pinned until later in the request/context
lifecycle, it no longer needs to be pinned from context_queue to
retire_requests.
The refcount on the context also has to be extended to cover this
new longer period.

Signed-off-by: Nick Hoath 
Issue: VIZ-4277
Cc: Daniel Vetter 
Cc: David Gordon 
Cc: Chris Wilson 
Cc: Alex Dai 
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 +
 drivers/gpu/drm/i915/i915_gem.c  |  7 +
 drivers/gpu/drm/i915/intel_lrc.c | 58 +---
 drivers/gpu/drm/i915/intel_lrc.h |  1 +
 4 files changed, 57 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 20cd6d8..778b14a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -884,6 +884,7 @@ struct intel_context {
struct {
struct drm_i915_gem_object *state;
struct intel_ringbuffer *ringbuf;
+   bool unsaved;
int pin_count;
} engine[I915_NUM_RINGS];
 
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f1e3fde..273946d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1385,6 +1385,13 @@ __i915_gem_request_retire__upto(struct 
drm_i915_gem_request *req)
tmp = list_first_entry(>request_list,
   typeof(*tmp), list);
 
+   if (i915.enable_execlists) {
+   unsigned long flags;
+
+   spin_lock_irqsave(>execlist_lock, flags);
+   intel_lr_context_complete_check(tmp);
+   spin_unlock_irqrestore(>execlist_lock, flags);
+   }
i915_gem_request_retire(tmp);
} while (tmp != req);
 
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 06180dc..d82e903 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -566,13 +566,17 @@ static int execlists_context_queue(struct 
drm_i915_gem_request *request)
struct drm_i915_gem_request *cursor;
int num_elements = 0;
 
-   if (request->ctx != ring->default_context)
-   intel_lr_context_pin(request);
-
i915_gem_request_reference(request);
 
spin_lock_irq(>execlist_lock);
 
+   if (request->ctx != ring->default_context) {
+   if (!request->ctx->engine[ring->id].unsaved) {
+   intel_lr_context_pin(request);
+   request->ctx->engine[ring->id].unsaved = true;
+   }
+   }
+
list_for_each_entry(cursor, >execlist_queue, execlist_link)
if (++num_elements > 2)
break;
@@ -958,12 +962,6 @@ void intel_execlists_retire_requests(struct 
intel_engine_cs *ring)
spin_unlock_irq(>execlist_lock);
 
list_for_each_entry_safe(req, tmp, _list, execlist_link) {
-   struct intel_context *ctx = req->ctx;
-   struct drm_i915_gem_object *ctx_obj =
-   ctx->engine[ring->id].state;
-
-   if (ctx_obj && (ctx != ring->default_context))
-   intel_lr_context_unpin(req);
list_del(>execlist_link);
i915_gem_request_unreference(req);
}
@@ -1073,6 +1071,31 @@ void intel_lr_context_unpin(struct drm_i915_gem_request 
*rq)
}
 }
 
+void intel_lr_context_complete_check(struct drm_i915_gem_request *req)
+{
+   struct intel_engine_cs *ring = req->ring;
+
+   assert_spin_locked(>execlist_lock);
+
+   if (ring->last_context && ring->last_context != req->ctx) {
+   if (req->ctx != ring->default_context
+   && ring->last_context->engine[ring->id].unsaved) {
+   /* Create fake request for unpinning the old context */
+   struct drm_i915_gem_request tmp;
+
+   tmp.ring = ring;
+   tmp.ctx = ring->last_context;
+   tmp.ringbuf =
+   ring->last_context->engine[ring->id].ringbuf;
+
+   intel_lr_context_unpin();
+   ring->last_context->engine[ring->id].unsaved = false;
+   ring->last_context = NULL;
+   }
+   }
+   ring->last_context = req->ctx;
+}
+
 static int intel_logical_ring_workarounds_emit(struct drm_i915_gem_request 
*req)
 {
int ret, i;
@@ -2390,7 +2413,22 @@ void intel_lr_context_free(struct intel_context *ctx)
intel_unpin_ringbuffer_obj(ringbuf);

Re: [Intel-gfx] [PATCH] drm/i915: A better maximum brightness for users.

[Paulo Zanoni]

2015-11-09 8:17 GMT-02:00 Jani Nikula :
> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)"  
> wrote:
>> The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937 however
>> the sysfs brightness level always starts from 0 so it is better to use
>> 927 as the sysfs maximum brightness level and it becomes easier to map
>> from the PWM brightness level to the sysfs brightness level.
>
> We've been thinking we should provide a fixed range to userspace
> instead. Say, 0-100.

While not clearly stated, this reply and the further ones sound like a
rejection to his patch.

I'm not really an expert on backlight, but his idea sounds simple and
an overall improvement to the codebase. I don't think it's fair to
block a simple one-line improvement based on the fact that we have an
idea (that may or may not be implemented) for a possibly better
implementation. Why not apply his patch (in case we conclude it
doesn't have any bugs), and then go for the 0-100 idea once someone
actually decides to implement it?

Besides, isn't there the possibility of having a panel that has a
0- range where the change from 0 to 9990 is negligible, and only
9991- matters, so when we scale to 0-100 it will become basically
a on/off switch? We all learned to never underestimate panel hardware.

>
> BR,
> Jani.
>
>
>
>
>>
>> Signed-off-by: Shih-Yuan Lee (FourDollars) 
>> ---
>>  drivers/gpu/drm/i915/intel_panel.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
>> b/drivers/gpu/drm/i915/intel_panel.c
>> index a24df35..697fd4d 100644
>> --- a/drivers/gpu/drm/i915/intel_panel.c
>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>> @@ -1211,7 +1211,7 @@ static int intel_backlight_device_register(struct 
>> intel_connector *connector)
>>* Note: Everything should work even if the backlight device max
>>* presented to the userspace is arbitrarily chosen.
>>*/
>> - props.max_brightness = panel->backlight.max;
>> + props.max_brightness = panel->backlight.max - panel->backlight.min;
>>   props.brightness = scale_hw_to_user(connector,
>>   panel->backlight.level,
>>   props.max_brightness);
>
> --
> Jani Nikula, Intel Open Source Technology Center
> ___
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



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

Re: [Intel-gfx] [PATCH v2 08/14] drm/i915: Remove intel_crtc->atomic.disable_ips.

[Ander Conselvan De Oliveira]

On Tue, 2015-11-03 at 08:31 +0100, Maarten Lankhorst wrote:
> This is already handled in pre_disable_primary, disabling it twice is useless.

The atomic.disable_ips field was added in

commit 066cf55b9ce35f1f90dde9fcec01431a9243a949
Author: Rodrigo Vivi 
Date:   Fri Jun 26 13:55:54 2015 -0700

drm/i915: Fix IPS related flicker

at which point intel_pre_disable_primary() already called hsw_disable_ips().
This patch is an exact revert of that patch, so it would be good to mention what
changed in between that makes it unnecessary. I suppose it is the fact that
everything goes through the atomic path now.

Ander

> 
> Signed-off-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 16 +---
>  drivers/gpu/drm/i915/intel_drv.h |  1 -
>  2 files changed, 1 insertion(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 2708899d9767..58074f4adca2 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4777,9 +4777,6 @@ static void intel_pre_plane_update(struct intel_crtc
> *crtc)
>   if (atomic->disable_fbc)
>   intel_fbc_disable_crtc(crtc);
>  
> - if (crtc->atomic.disable_ips)
> - hsw_disable_ips(crtc);
> -
>   if (atomic->pre_disable_primary)
>   intel_pre_disable_primary(>base);
>  
> @@ -11683,19 +11680,8 @@ int intel_plane_atomic_calc_changes(struct
> drm_crtc_state *crtc_state,
>   intel_crtc->atomic.pre_disable_primary = turn_off;
>   intel_crtc->atomic.post_enable_primary = turn_on;
>  
> - if (turn_off) {
> - /*
> -  * FIXME: Actually if we will still have any other
> -  * plane enabled on the pipe we could let IPS enabled
> -  * still, but for now lets consider that when we make
> -  * primary invisible by setting DSPCNTR to 0 on
> -  * update_primary_plane function IPS needs to be
> -  * disable.
> -  */
> - intel_crtc->atomic.disable_ips = true;
> -
> + if (turn_off)
>   intel_crtc->atomic.disable_fbc = true;
> - }
>  
>   /*
>* FBC does not work on some platforms for rotated
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 6f99c7398af3..ce5f10fc92aa 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -537,7 +537,6 @@ struct intel_mmio_flip {
>  struct intel_crtc_atomic_commit {
>   /* Sleepable operations to perform before commit */
>   bool disable_fbc;
> - bool disable_ips;
>   bool pre_disable_primary;
>  
>   /* Sleepable operations to perform after commit */
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 04/12] 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 a0307fc..40ec895 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 71bd1dc..cef0588 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 7d11aa0..483f591 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1395,8 +1395,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 v2 10/12] drm/i915/gen9: 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 for Skylake and DC5 for Broxton but will be
configurable for Skylake in a later patch.

v2: Check both DC5 and DC6 bits in power well enabled function (Ville)

Signed-off-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/i915_drv.c |   6 --
 drivers/gpu/drm/i915/i915_reg.h |   1 +
 drivers/gpu/drm/i915/intel_display.c|   6 ++
 drivers/gpu/drm/i915/intel_runtime_pm.c | 110 +++-
 4 files changed, 88 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 5a63f9a..0c7f435 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1072,9 +1072,6 @@ static int i915_pm_resume(struct device *dev)
 
 static int skl_suspend_complete(struct drm_i915_private *dev_priv)
 {
-   if (dev_priv->csr.dmc_payload)
-   skl_enable_dc6(dev_priv);
-
return 0;
 }
 
@@ -1119,9 +1116,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);
-
return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 31b3a84..df445ba 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -636,6 +636,7 @@ enum skl_disp_power_wells {
 
/* Not actual bit groups. Used as IDs for lookup_power_well() */
SKL_DISP_PW_ALWAYS_ON,
+   SKL_DISP_PW_DC_OFF,
 };
 
 #define SKL_POWER_WELL_STATE(pw) (1 << ((pw) * 2))
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index 649ac34..856d801 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13320,6 +13320,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);
@@ -13343,6 +13346,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 edf753e..95c3fcc 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 &&   \
@@ -309,9 +306,14 @@ static void hsw_set_power_well(struct drm_i915_private 
*dev_priv,
 #define SKL_DISPLAY_DDI_D_POWER_DOMAINS (  \
BIT(POWER_DOMAIN_PORT_DDI_D_LANES) |\
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_2_POWER_DOMAINS)) |   \
+   SKL_DISPLAY_POWERWELL_2_POWER_DOMAINS | \
+   SKL_DISPLAY_DC_OFF_POWER_DOMAINS)) |\
BIT(POWER_DOMAIN_INIT))
 
 #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS (\
@@ -339,6 +341,10 @@ static void hsw_set_power_well(struct drm_i915_private 
*dev_priv,
BIT(POWER_DOMAIN_AUX_A) |   \
BIT(POWER_DOMAIN_PLLS) |\
BIT(POWER_DOMAIN_INIT))
+#define BXT_DISPLAY_DC_OFF_POWER_DOMAINS ( \
+   BIT(POWER_DOMAIN_MODESET) | \
+   BIT(POWER_DOMAIN_AUX_A) |   \
+   BIT(POWER_DOMAIN_INIT))
 #define BXT_DISPLAY_ALWAYS_ON_POWER_DOMAINS (  \
(POWER_DOMAIN_MASK & ~(BXT_DISPLAY_POWERWELL_1_POWER_DOMAINS |  \
BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS)) |   \
@@ -486,15 

[Intel-gfx] [PATCH 05/12] 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 | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index c11522f..c9cb94d 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -318,11 +318,7 @@ static void hsw_set_power_well(struct drm_i915_private 
*dev_priv,
BIT(POWER_DOMAIN_INIT))
 #define SKL_DISPLAY_ALWAYS_ON_POWER_DOMAINS (  \
(POWER_DOMAIN_MASK & ~( \
-   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_POWERWELL_2_POWER_DOMAINS)) |   \
BIT(POWER_DOMAIN_INIT))
 
 #define BXT_DISPLAY_POWERWELL_2_POWER_DOMAINS (\
-- 
2.5.0

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

[Intel-gfx] [PATCH 09/12] drm/i915: Explain usage of power well IDs vs bit groups

[Patrik Jakobsson]

Signed-off-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/i915_reg.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index e6d88f5..31b3a84 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -621,6 +621,7 @@ enum punit_power_well {
PUNIT_POWER_WELL_DPIO_RX1   = 11,
PUNIT_POWER_WELL_DPIO_CMN_D = 12,
 
+   /* Not actual bit groups. Used as IDs for lookup_power_well() */
PUNIT_POWER_WELL_ALWAYS_ON,
 };
 
@@ -633,6 +634,7 @@ enum skl_disp_power_wells {
SKL_DISP_PW_1 = 14,
SKL_DISP_PW_2,
 
+   /* Not actual bit groups. Used as IDs for lookup_power_well() */
SKL_DISP_PW_ALWAYS_ON,
 };
 
-- 
2.5.0

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

[Intel-gfx] [PATCH 07/12] drm/i915: Add a modeset power domain

[Patrik Jakobsson]

We need a power domain for disabling DC5/DC6 around modesets to prevent
confusing the DMC.

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

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 2fad0a9..0b06192 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2738,6 +2738,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 fff7f63..c0252ef 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -196,6 +196,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,
-- 
2.5.0

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

[Intel-gfx] [PATCH 06/12] drm/i915: Remove distinction between DDI 2 vs 4 lanes

[Patrik Jakobsson]

We never make use of the distinction between 2 vs 4 lanes so combine
them into a per port domain instead. This saves us a few bits in the
power domain mask. Change suggested by Ville.

Signed-off-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/i915_debugfs.c | 28 +
 drivers/gpu/drm/i915/i915_drv.h | 14 +++
 drivers/gpu/drm/i915/intel_display.c| 10 ++---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 71 -
 4 files changed, 45 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c 
b/drivers/gpu/drm/i915/i915_debugfs.c
index 40ec895..2fad0a9 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -2706,24 +2706,16 @@ static const char *power_domain_str(enum 
intel_display_power_domain domain)
return "TRANSCODER_C";
case POWER_DOMAIN_TRANSCODER_EDP:
return "TRANSCODER_EDP";
-   case POWER_DOMAIN_PORT_DDI_A_2_LANES:
-   return "PORT_DDI_A_2_LANES";
-   case POWER_DOMAIN_PORT_DDI_A_4_LANES:
-   return "PORT_DDI_A_4_LANES";
-   case POWER_DOMAIN_PORT_DDI_B_2_LANES:
-   return "PORT_DDI_B_2_LANES";
-   case POWER_DOMAIN_PORT_DDI_B_4_LANES:
-   return "PORT_DDI_B_4_LANES";
-   case POWER_DOMAIN_PORT_DDI_C_2_LANES:
-   return "PORT_DDI_C_2_LANES";
-   case POWER_DOMAIN_PORT_DDI_C_4_LANES:
-   return "PORT_DDI_C_4_LANES";
-   case POWER_DOMAIN_PORT_DDI_D_2_LANES:
-   return "PORT_DDI_D_2_LANES";
-   case POWER_DOMAIN_PORT_DDI_D_4_LANES:
-   return "PORT_DDI_D_4_LANES";
-   case POWER_DOMAIN_PORT_DDI_E_2_LANES:
-   return "PORT_DDI_E_2_LANES";
+   case POWER_DOMAIN_PORT_DDI_A_LANES:
+   return "PORT_DDI_A_LANES";
+   case POWER_DOMAIN_PORT_DDI_B_LANES:
+   return "PORT_DDI_B_LANES";
+   case POWER_DOMAIN_PORT_DDI_C_LANES:
+   return "PORT_DDI_C_LANES";
+   case POWER_DOMAIN_PORT_DDI_D_LANES:
+   return "PORT_DDI_D_LANES";
+   case POWER_DOMAIN_PORT_DDI_E_LANES:
+   return "PORT_DDI_E_LANES";
case POWER_DOMAIN_PORT_DSI:
return "PORT_DSI";
case POWER_DOMAIN_PORT_CRT:
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index cef0588..fff7f63 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -180,15 +180,11 @@ enum intel_display_power_domain {
POWER_DOMAIN_TRANSCODER_B,
POWER_DOMAIN_TRANSCODER_C,
POWER_DOMAIN_TRANSCODER_EDP,
-   POWER_DOMAIN_PORT_DDI_A_2_LANES,
-   POWER_DOMAIN_PORT_DDI_A_4_LANES,
-   POWER_DOMAIN_PORT_DDI_B_2_LANES,
-   POWER_DOMAIN_PORT_DDI_B_4_LANES,
-   POWER_DOMAIN_PORT_DDI_C_2_LANES,
-   POWER_DOMAIN_PORT_DDI_C_4_LANES,
-   POWER_DOMAIN_PORT_DDI_D_2_LANES,
-   POWER_DOMAIN_PORT_DDI_D_4_LANES,
-   POWER_DOMAIN_PORT_DDI_E_2_LANES,
+   POWER_DOMAIN_PORT_DDI_A_LANES,
+   POWER_DOMAIN_PORT_DDI_B_LANES,
+   POWER_DOMAIN_PORT_DDI_C_LANES,
+   POWER_DOMAIN_PORT_DDI_D_LANES,
+   POWER_DOMAIN_PORT_DDI_E_LANES,
POWER_DOMAIN_PORT_DSI,
POWER_DOMAIN_PORT_CRT,
POWER_DOMAIN_PORT_OTHER,
diff --git a/drivers/gpu/drm/i915/intel_display.c 
b/drivers/gpu/drm/i915/intel_display.c
index c2578d9..649ac34 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5128,15 +5128,15 @@ static enum intel_display_power_domain 
port_to_power_domain(enum port port)
 {
switch (port) {
case PORT_A:
-   return POWER_DOMAIN_PORT_DDI_A_4_LANES;
+   return POWER_DOMAIN_PORT_DDI_A_LANES;
case PORT_B:
-   return POWER_DOMAIN_PORT_DDI_B_4_LANES;
+   return POWER_DOMAIN_PORT_DDI_B_LANES;
case PORT_C:
-   return POWER_DOMAIN_PORT_DDI_C_4_LANES;
+   return POWER_DOMAIN_PORT_DDI_C_LANES;
case PORT_D:
-   return POWER_DOMAIN_PORT_DDI_D_4_LANES;
+   return POWER_DOMAIN_PORT_DDI_D_LANES;
case PORT_E:
-   return POWER_DOMAIN_PORT_DDI_E_2_LANES;
+   return POWER_DOMAIN_PORT_DDI_E_LANES;
default:
WARN_ON_ONCE(1);
return POWER_DOMAIN_PORT_OTHER;
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index c9cb94d..fc206bb 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -286,13 +286,10 @@ static void hsw_set_power_well(struct drm_i915_private 
*dev_priv,
BIT(POWER_DOMAIN_TRANSCODER_C) |\
BIT(POWER_DOMAIN_PIPE_B_PANEL_FITTER) | \
BIT(POWER_DOMAIN_PIPE_C_PANEL_FITTER) | \
-   BIT(POWER_DOMAIN_PORT_DDI_B_2_LANES) |  \
-   

[Intel-gfx] [PATCH v2 00/12] Skylake DMC/DC-state fixes and redesign

[Patrik Jakobsson]

This v2 of the series is rebased on top of a new series from Imre [1]
and contains a few new patches and reordering.

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

First couple of 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-November/079412.html

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

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

Patrik Jakobsson (9):
  drm/i915: Don't trust CSR program memory contents
  drm/i915/gen9: Always set mask memory up when enabling DC5 or DC6
  drm/i915: Remove distinction between DDI 2 vs 4 lanes
  drm/i915: Add a modeset power domain
  drm/i915: Do not warn on PG2 enabled in gen9_disable_dc5()
  drm/i915: Explain usage of power well IDs vs bit groups
  drm/i915/gen9: Turn DC handling into a power well
  drm/i915/gen9: Add boot parameter for disabling DC6
  drm/i915/skl: Remove unused suspend and resume callbacks

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 |  32 ++--
 drivers/gpu/drm/i915/i915_drv.c |  23 ---
 drivers/gpu/drm/i915/i915_drv.h |  17 +--
 drivers/gpu/drm/i915/i915_params.c  |   6 +
 drivers/gpu/drm/i915/i915_reg.h |   3 +
 drivers/gpu/drm/i915/intel_csr.c|  10 +-
 drivers/gpu/drm/i915/intel_display.c|  56 ++-
 drivers/gpu/drm/i915/intel_dp.c |  48 ++
 drivers/gpu/drm/i915/intel_drv.h|   4 +-
 drivers/gpu/drm/i915/intel_hdmi.c   |   8 +-
 drivers/gpu/drm/i915/intel_i2c.c|   6 +-
 drivers/gpu/drm/i915/intel_runtime_pm.c | 255 
 12 files changed, 233 insertions(+), 235 deletions(-)

-- 
2.5.0

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

[Intel-gfx] [PATCH 08/12] 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 the DMC 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 
Reviewed-by: Ville Syrjälä 
---
 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 fc206bb..edf753e 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -466,8 +466,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.
@@ -475,7 +473,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.5.0

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

[Intel-gfx] [PATCH 02/12] drm/i915/gen9: Always set mask memory up when enabling DC5 or DC6

[Patrik Jakobsson]

Move call to gen9_set_dc_state_debugmask_memory_up() into
gen9_set_dc_state() to prevent us missing it somewhere.

Signed-off-by: Patrik Jakobsson 
---
 drivers/gpu/drm/i915/intel_runtime_pm.c | 35 -
 1 file changed, 17 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 5a36dd5..4b9ee60 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -395,6 +395,20 @@ static void assert_can_disable_dc9(struct drm_i915_private 
*dev_priv)
  */
 }
 
+static void gen9_set_dc_state_debugmask_memory_up(
+   struct drm_i915_private *dev_priv)
+{
+   uint32_t val;
+
+   /* The below bit doesn't need to be cleared ever afterwards */
+   val = I915_READ(DC_STATE_DEBUG);
+   if (!(val & DC_STATE_DEBUG_MASK_MEMORY_UP)) {
+   val |= DC_STATE_DEBUG_MASK_MEMORY_UP;
+   I915_WRITE(DC_STATE_DEBUG, val);
+   POSTING_READ(DC_STATE_DEBUG);
+   }
+}
+
 static void gen9_set_dc_state(struct drm_i915_private *dev_priv, uint32_t 
state)
 {
uint32_t val;
@@ -408,6 +422,9 @@ static void gen9_set_dc_state(struct drm_i915_private 
*dev_priv, uint32_t state)
 
WARN_ON_ONCE(state & ~mask);
 
+   if (state & DC_STATE_EN_UPTO_DC5_DC6_MASK)
+   gen9_set_dc_state_debugmask_memory_up(dev_priv);
+
val = I915_READ(DC_STATE_EN);
DRM_DEBUG_KMS("Setting DC state from %02x to %02x\n", val & mask, 
state);
val &= ~mask;
@@ -434,20 +451,6 @@ void bxt_disable_dc9(struct drm_i915_private *dev_priv)
gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
 }
 
-static void gen9_set_dc_state_debugmask_memory_up(
-   struct drm_i915_private *dev_priv)
-{
-   uint32_t val;
-
-   /* The below bit doesn't need to be cleared ever afterwards */
-   val = I915_READ(DC_STATE_DEBUG);
-   if (!(val & DC_STATE_DEBUG_MASK_MEMORY_UP)) {
-   val |= DC_STATE_DEBUG_MASK_MEMORY_UP;
-   I915_WRITE(DC_STATE_DEBUG, val);
-   POSTING_READ(DC_STATE_DEBUG);
-   }
-}
-
 void assert_csr_loaded(struct drm_i915_private *dev_priv)
 {
WARN_ONCE(!I915_READ(CSR_PROGRAM(0)),
@@ -496,8 +499,6 @@ static void gen9_enable_dc5(struct drm_i915_private 
*dev_priv)
 
DRM_DEBUG_KMS("Enabling DC5\n");
 
-   gen9_set_dc_state_debugmask_memory_up(dev_priv);
-
gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
 }
 
@@ -543,8 +544,6 @@ void skl_enable_dc6(struct drm_i915_private *dev_priv)
 
DRM_DEBUG_KMS("Enabling DC6\n");
 
-   gen9_set_dc_state_debugmask_memory_up(dev_priv);
-
gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
 
 }
-- 
2.5.0

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

[Intel-gfx] [PATCH 03/12] 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 d0fec07..c2578d9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5143,6 +5143,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))
@@ -5174,6 +5191,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 4655af0..3978540 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);
 
  

[Intel-gfx] [PATCH 12/12] drm/i915/skl: Remove unused suspend and resume callbacks

[Patrik Jakobsson]

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

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 0c7f435..77d183d 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -622,7 +622,6 @@ static void intel_suspend_encoders(struct drm_i915_private 
*dev_priv)
 static int intel_suspend_complete(struct drm_i915_private *dev_priv);
 static int vlv_resume_prepare(struct drm_i915_private *dev_priv,
  bool rpm_resume);
-static int skl_resume_prepare(struct drm_i915_private *dev_priv);
 static int bxt_resume_prepare(struct drm_i915_private *dev_priv);
 
 
@@ -857,8 +856,6 @@ static int i915_drm_resume_early(struct drm_device *dev)
 
if (IS_BROXTON(dev))
ret = bxt_resume_prepare(dev_priv);
-   else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
-   ret = skl_resume_prepare(dev_priv);
else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
hsw_disable_pc8(dev_priv);
 
@@ -1070,11 +1067,6 @@ static int i915_pm_resume(struct device *dev)
return i915_drm_resume(drm_dev);
 }
 
-static int skl_suspend_complete(struct drm_i915_private *dev_priv)
-{
-   return 0;
-}
-
 static int hsw_suspend_complete(struct drm_i915_private *dev_priv)
 {
hsw_enable_pc8(dev_priv);
@@ -1114,11 +1106,6 @@ static int bxt_resume_prepare(struct drm_i915_private 
*dev_priv)
return 0;
 }
 
-static int skl_resume_prepare(struct drm_i915_private *dev_priv)
-{
-   return 0;
-}
-
 /*
  * Save all Gunit registers that may be lost after a D3 and a subsequent
  * S0i[R123] transition. The list of registers needing a save/restore is
@@ -1582,8 +1569,6 @@ static int intel_runtime_resume(struct device *device)
 
if (IS_BROXTON(dev))
ret = bxt_resume_prepare(dev_priv);
-   else if (IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
-   ret = skl_resume_prepare(dev_priv);
else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
hsw_disable_pc8(dev_priv);
else if (IS_VALLEYVIEW(dev_priv))
@@ -1626,8 +1611,6 @@ static int intel_suspend_complete(struct drm_i915_private 
*dev_priv)
 
if (IS_BROXTON(dev_priv))
ret = bxt_suspend_complete(dev_priv);
-   else if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
-   ret = skl_suspend_complete(dev_priv);
else if (IS_HASWELL(dev_priv) || IS_BROADWELL(dev_priv))
ret = hsw_suspend_complete(dev_priv);
else if (IS_VALLEYVIEW(dev_priv))
-- 
2.5.0

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

[Intel-gfx] [PATCH v2 11/12] drm/i915/gen9: Add boot parameter for disabling DC6

[Patrik Jakobsson]

v2: Use _unsafe (Jani)

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 | 4 ++--
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index c0252ef..5628c5a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2639,6 +2639,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..6457f3a 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_unsafe(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 95c3fcc..62c1273 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -708,7 +708,7 @@ static void gen9_dc_off_power_well_enable(struct 
drm_i915_private *dev_priv,
 static void gen9_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);
@@ -720,7 +720,7 @@ static void gen9_dc_off_power_well_sync_hw(struct 
drm_i915_private *dev_priv,
if (power_well->count > 0) {
gen9_set_dc_state(dev_priv, DC_STATE_DISABLE);
} else {
-   if (IS_SKYLAKE(dev_priv))
+   if (IS_SKYLAKE(dev_priv) && i915.enable_dc6)
gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC6);
else
gen9_set_dc_state(dev_priv, DC_STATE_EN_UPTO_DC5);
-- 
2.5.0

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

Re: [Intel-gfx] [PATCH] drm/i915: A better maximum brightness for users.

[Jani Nikula]

On Mon, 09 Nov 2015, Paulo Zanoni  wrote:
> 2015-11-09 8:17 GMT-02:00 Jani Nikula :
>> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)"  
>> wrote:
>>> The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937 however
>>> the sysfs brightness level always starts from 0 so it is better to use
>>> 927 as the sysfs maximum brightness level and it becomes easier to map
>>> from the PWM brightness level to the sysfs brightness level.
>>
>> We've been thinking we should provide a fixed range to userspace
>> instead. Say, 0-100.
>
> While not clearly stated, this reply and the further ones sound like a
> rejection to his patch.

I wanted to understand the motivation rather than bluntly rejecting.

> I'm not really an expert on backlight, but his idea sounds simple and
> an overall improvement to the codebase. I don't think it's fair to
> block a simple one-line improvement based on the fact that we have an
> idea (that may or may not be implemented) for a possibly better
> implementation. Why not apply his patch (in case we conclude it
> doesn't have any bugs), and then go for the 0-100 idea once someone
> actually decides to implement it?

The implementation is

diff --git a/drivers/gpu/drm/i915/intel_panel.c 
b/drivers/gpu/drm/i915/intel_panel.c
index a24df35e11e7..d5d86601d411 100644
--- a/drivers/gpu/drm/i915/intel_panel.c
+++ b/drivers/gpu/drm/i915/intel_panel.c
@@ -1211,7 +1211,7 @@ static int intel_backlight_device_register(struct 
intel_connector *connector)
 * Note: Everything should work even if the backlight device max
 * presented to the userspace is arbitrarily chosen.
 */
-   props.max_brightness = panel->backlight.max;
+   props.max_brightness = 100;
props.brightness = scale_hw_to_user(connector,
panel->backlight.level,
props.max_brightness);

and "just" testing is required.

> Besides, isn't there the possibility of having a panel that has a
> 0- range where the change from 0 to 9990 is negligible, and only
> 9991- matters, so when we scale to 0-100 it will become basically
> a on/off switch? We all learned to never underestimate panel hardware.

That's a somewhat more valid argument. The answer to that should be that
we map the userspace range to the hardware range according to a curve
rather than a linear mapping. See Ville's reply.

BR,
Jani.



>
>>
>> BR,
>> Jani.
>>
>>
>>
>>
>>>
>>> Signed-off-by: Shih-Yuan Lee (FourDollars) 
>>> ---
>>>  drivers/gpu/drm/i915/intel_panel.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
>>> b/drivers/gpu/drm/i915/intel_panel.c
>>> index a24df35..697fd4d 100644
>>> --- a/drivers/gpu/drm/i915/intel_panel.c
>>> +++ b/drivers/gpu/drm/i915/intel_panel.c
>>> @@ -1211,7 +1211,7 @@ static int intel_backlight_device_register(struct 
>>> intel_connector *connector)
>>>* Note: Everything should work even if the backlight device max
>>>* presented to the userspace is arbitrarily chosen.
>>>*/
>>> - props.max_brightness = panel->backlight.max;
>>> + props.max_brightness = panel->backlight.max - panel->backlight.min;
>>>   props.brightness = scale_hw_to_user(connector,
>>>   panel->backlight.level,
>>>   props.max_brightness);
>>
>> --
>> Jani Nikula, Intel Open Source Technology Center
>> ___
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> -- 
> Paulo Zanoni

-- 
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] drm/i915: A better maximum brightness for users.

[Ville Syrjälä]

On Mon, Nov 09, 2015 at 10:02:24PM +0800, Shih-Yuan Lee (FourDollars) wrote:
> On Mon, Nov 9, 2015 at 8:58 PM, Jani Nikula 
> wrote:
> 
> > On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" 
> > wrote:
> > > On Mon, Nov 9, 2015 at 6:51 PM, Jani Nikula  > >
> > > wrote:
> > >
> > >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)"  > >
> > >> wrote:
> > >> > On Mon, Nov 9, 2015 at 6:17 PM, Jani Nikula <
> > jani.nik...@linux.intel.com
> > >> >
> > >> > wrote:
> > >> >
> > >> >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" <
> > sy...@canonical.com
> > >> >
> > >> >> wrote:
> > >> >> > The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937
> > >> however
> > >> >> > the sysfs brightness level always starts from 0 so it is better to
> > use
> > >> >> > 927 as the sysfs maximum brightness level and it becomes easier to
> > map
> > >> >> > from the PWM brightness level to the sysfs brightness level.
> > >> >>
> > >> >> We've been thinking we should provide a fixed range to userspace
> > >> >> instead. Say, 0-100.
> > >> >>
> > >> >> BR,
> > >> >> Jani.
> > >> >>
> > >> >>
> > >> >>
> > >> > That might not be a good idea for the backward compatibility.
> > >>
> > >> Please explain how you think your change is good and a fixed range 0-100
> > >> is bad in terms of backward compatibility. Both just arbitrarily change
> > >> the max.
> > >>
> > > The original sysfs brightness range is from 0 to 937. Let's define it as
> > A
> > > = {0, 1, 2, ..., 937}.
> > > The PWM brightness range is from 10 to 937. Let's define it as B = {10,
> > 11,
> > > 12, ..., 937}.
> > > |A| = 938, |B| = 928
> > > |A| != |B|
> > > It implies A and B is not an 1-1 mapping.
> > >
> > > My patch is not a arbitrarily change.
> > > It makes A' = {0, 1, 2, ..., 927}. |A'| = 928
> > > You can see |A'| = |B| so it implies A' and B is an 1-1 mapping.
> > > It means any value in A' can be mapped to a different value in B and visa
> > > versa.
> > >
> > > C = {0, 100} has the same mapping problem.
> >
> > Please tell me why you think this is a problem to begin with. What (user
> > perceptible) problem are you trying to solve?
> >
> I am investigating some backlight issue that the i915.ko brightness
> behavior is changed on Dell XPS 13 (2015).
> Originally the lowest brightness won't turn off the backlight but the Linux
> kernel after 3.19 will turn off the backlight.
> Dell's engineer tells me that Windows driver also uses VBT to change the
> brightness but it doesn't turn off the backlight.
> I am not a dedicated kernel engineer but I have some interest to look at
> this issue.
> This regression is from
> http://lists.freedesktop.org/archives/intel-gfx/2014-August/050642.html.
> 
> This patch is found during my investigation for that problem.
> 
> >
> > I understand we could simplify (or remove) the scaling code with your
> > change, but I'm reluctant to go that way when, as I said, we've been
> > talking about fixing the range reported to userspace.
> >
> I personally think i915.ko just needs to respect the settings from VBT.
> No matter how many valid levels from VBT, i915.ko just provides the same
> levels in brightness sysfs.
> 
> >
> > Part of the reason for going to 0-100 range would be that there are
> > barely that many user distinguishable steps in the backlight level. It
> > is silly to have brightness range of, say, 0-937, because you can't
> > distinguish them from each other. (Perhaps counter-intuitively, the
> > higher the PWM modulation frequency, the fewer user distinguishable
> > levels you can actually get.)
> >
> I think i915.ko doesn't need to care about this problem.
> In fact, the very end users only use a scroll bar to change the brightness.
> Or they will use brightness up/down hotkeys to change the brightness but
> the desktop environments like GNOME will make it only work for 20 levels.
> 
> However some advanced users like me may still prefer to have all valid
> brightness levels.
> That is why I made this patch and this is my first patch for Linux kernel
> project.

BTW there's that BLCM thing in opregion that provides some kind of
non-linear remapping for backlight levels. A while back I got fed up
with the uneven brightness steps on my IVB X1 Carbon, and so I cooked
up a patch to use BLCM. It did improve the situation quite a bit.

Pushed it here in case anyone else is interested:
git://github.com/vsyrjala/linux.git blcm_backlight

> 
> Regards,
> $4
> 
> >
> > >> Besides, we've changed the max for some platforms before, and the ABI
> > >> for backlight sysfs is you can stick a value between 0 and
> > >> max_brightness to the brightness attribute. Any userspace that relies on
> > >> anything else is broken.
> > >>
> > >> > However I saw some message as the following.
> > >> > [3.402233] [drm:parse_lfp_backlight] VBT backlight PWM modulation
> > >> > frequency 200 Hz, active high, min brightness 

Re: [Intel-gfx] [PATCH i-g-t] lib: add a environment variable to control output

[Chris Wilson]

On Mon, Nov 09, 2015 at 05:17:13PM +, Thomas Wood wrote:
> Disable output of terminal control characters and progress meters when
> IGT_PLAIN_OUTPUT is set in the environment.
> 
> Cc: Derek Morton 
> Signed-off-by: Thomas Wood 
> ---
>  lib/igt_aux.c  |  2 +-
>  lib/igt_core.c | 23 ++-
>  lib/igt_core.h |  2 ++
>  3 files changed, 17 insertions(+), 10 deletions(-)
> 
> diff --git a/lib/igt_aux.c b/lib/igt_aux.c
> index f3c76ae..4d08d68 100644
> --- a/lib/igt_aux.c
> +++ b/lib/igt_aux.c
> @@ -231,7 +231,7 @@ static void igt_interactive_info(const char *format, ...)
>  {
>   va_list args;
>  
> - if (!isatty(STDERR_FILENO))
> + if (!isatty(STDERR_FILENO) || __igt_plain_output)

Hmm, would it not be a bug if we reach this point and we haven't
initialised __igt_plain_output from common_init()? 
-Chris

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

Re: [Intel-gfx] [PATCH 1/4] drm/i915: export assert_device_not_suspended

[Chris Wilson]

On Mon, Nov 09, 2015 at 08:20:08PM +0200, Imre Deak wrote:
> We should use the same assert in more places, so export it. Move it to
> intel_runtime_pm.c at the same time, since that's a more logical place
> for it.
> 
> Signed-off-by: Imre Deak 
> ---
>  drivers/gpu/drm/i915/intel_drv.h| 2 ++
>  drivers/gpu/drm/i915/intel_runtime_pm.c | 6 ++
>  drivers/gpu/drm/i915/intel_uncore.c | 7 ---
>  3 files changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> b/drivers/gpu/drm/i915/intel_drv.h
> index f9089df..373cc07 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1415,6 +1415,8 @@ 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 assert_device_not_suspended(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_runtime_pm.c 
> b/drivers/gpu/drm/i915/intel_runtime_pm.c
> index b42506b..0d4a03b 100644
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2120,6 +2120,12 @@ void intel_power_domains_init_hw(struct 
> drm_i915_private *dev_priv, bool resume)
>   power_domains->initializing = false;
>  }
>  
> +void assert_device_not_suspended(struct drm_i915_private *dev_priv)
> +{
> + WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,

Can just be HAS_RUNTIME_PM(dev_priv).

Can we fix HAS_RUNTIME_PM not to be an ever-growing list but a nice
little feature flag?

{
  if (!HAS_RUNTIME_PM(dev_priv))
return;

  WARN_ONCE(dev_priv->pm.suspended, "Device suspended during HW access");
}

The little extra information would be nice in the error message, but not
convinced the extra whitespace is that helpful.
-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 v2 4/4] drm/i915: add assert_device_not_suspended to GGTT PTE updaters

[Imre Deak]

The device should be on when updating the GGTT PTEs, so add an assert to
all relevant places.

v2:
- use the existing dev_priv directly everywhere (Ville)

Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 016739e..4d42ca5 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2358,6 +2358,8 @@ static void gen8_ggtt_insert_entries(struct 
i915_address_space *vm,
struct sg_page_iter sg_iter;
dma_addr_t addr = 0; /* shut up gcc */
 
+   assert_device_not_suspended(dev_priv);
+
for_each_sg_page(st->sgl, _iter, st->nents, 0) {
addr = sg_dma_address(sg_iter.sg) +
(sg_iter.sg_pgoffset << PAGE_SHIFT);
@@ -2404,6 +2406,8 @@ static void gen6_ggtt_insert_entries(struct 
i915_address_space *vm,
struct sg_page_iter sg_iter;
dma_addr_t addr = 0;
 
+   assert_device_not_suspended(dev_priv);
+
for_each_sg_page(st->sgl, _iter, st->nents, 0) {
addr = sg_page_iter_dma_address(_iter);
iowrite32(vm->pte_encode(addr, level, true, flags), 
_entries[i]);
@@ -2442,6 +2446,8 @@ static void gen8_ggtt_clear_range(struct 
i915_address_space *vm,
const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
int i;
 
+   assert_device_not_suspended(dev_priv);
+
if (WARN(num_entries > max_entries,
 "First entry = %d; Num entries = %d (max=%d)\n",
 first_entry, num_entries, max_entries))
@@ -2468,6 +2474,8 @@ static void gen6_ggtt_clear_range(struct 
i915_address_space *vm,
const int max_entries = gtt_total_entries(dev_priv->gtt) - first_entry;
int i;
 
+   assert_device_not_suspended(dev_priv);
+
if (WARN(num_entries > max_entries,
 "First entry = %d; Num entries = %d (max=%d)\n",
 first_entry, num_entries, max_entries))
-- 
2.5.0

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

[Intel-gfx] [PATCH v2 3/4] drm/i915: make assert_device_not_suspended more precise

[Imre Deak]

Atm, we assert that the device is not suspended after the point when the
HW is truly put to a suspended state. This is fine, but we can catch
more problems if we check the RPM refcount. After that one drops to zero
we shouldn't access the HW any more, although the actual suspend may be
delayed. The only complication is that we want to avoid asserts while
the suspend handler itself is running, so add a flag to handle this
case.

While at it remove the HAS_RUNTIME_PM check, the pm.suspended flag is
false and the RPM refcount is non-zero on all platforms that don't
support RPM.

This caught additional WARNs from the atomic path, those will be fixed
as a follow-up.

v2:
- remove the redundant HAS_RUNTIME_PM check (Ville)

Signed-off-by: Imre Deak 
---
 drivers/gpu/drm/i915/i915_drv.c |  5 +
 drivers/gpu/drm/i915/i915_drv.h |  5 +
 drivers/gpu/drm/i915/intel_runtime_pm.c | 14 --
 3 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 77d183d..caeb218 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1494,6 +1494,9 @@ static int intel_runtime_suspend(struct device *device)
 
return -EAGAIN;
}
+
+   dev_priv->pm.disable_suspended_assert = true;
+
/*
 * We are safe here against re-faults, since the fault handler takes
 * an RPM reference.
@@ -1518,6 +1521,8 @@ static int intel_runtime_suspend(struct device *device)
intel_uncore_forcewake_reset(dev, false);
dev_priv->pm.suspended = true;
 
+   dev_priv->pm.disable_suspended_assert = false;
+
/*
 * FIXME: We really should find a document that references the arguments
 * used below!
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5628c5a..43fd341 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1599,6 +1599,11 @@ struct skl_wm_level {
  * For more, read the Documentation/power/runtime_pm.txt.
  */
 struct i915_runtime_pm {
+   /*
+* Used for the duration of runtime suspend to avoid false device
+* suspended asserts.
+*/
+   bool disable_suspended_assert;
bool suspended;
bool irqs_enabled;
 };
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c 
b/drivers/gpu/drm/i915/intel_runtime_pm.c
index 4d39b3c..2bdbcd4 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -2120,8 +2120,18 @@ void intel_power_domains_init_hw(struct drm_i915_private 
*dev_priv, bool resume)
 
 void assert_device_not_suspended(struct drm_i915_private *dev_priv)
 {
-   WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
- "Device suspended\n");
+   int rpm_usage;
+
+   if (dev_priv->pm.disable_suspended_assert)
+   return;
+
+#ifdef CONFIG_PM
+   rpm_usage = atomic_read(_priv->dev->dev->power.usage_count);
+#else
+   rpm_usage = 1;
+#endif
+
+   WARN_ONCE(dev_priv->pm.suspended || !rpm_usage, "Device suspended\n");
 }
 
 /**
-- 
2.5.0

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

Re: [Intel-gfx] [PATCH] drm/i915: get runtime PM reference around GEM set_tiling IOCTL

[Chris Wilson]

On Mon, Nov 09, 2015 at 08:16:26PM +0200, Imre Deak wrote:
> After fixing the same issue in the set_caching IOCTL and Chris' request
> to check out the possibilities for an improved RPM ref handling I
> noticed that we have the same issue in the set_tiling IOCTL. Fix this
> up.I didn't see any bug reports about this one, but the GTT unbind
> operation on this path accesses the HW, which needs the ref.
> 
> Signed-off-by: Imre Deak 

The reason why we haven't seen a bug report is that we don't get the GTT
mismatch on gen4+ devices (i.e. those with runtime pm). However, for the
sake of completeness, it is better to wrap all the GGTT unbind access in
the rpm reference (and future patches to reduce the wakelock will be
more consistent).

Reviewed-by: Chris Wilson 
-Chris

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

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Use insert_page for pwrite_fast

[Mika Kuoppala]

ankitprasad.r.sha...@intel.com writes:

> From: Ankitprasad Sharma 
>
> In pwrite_fast, map an object page by page if obj_ggtt_pin fails. First,
> we try a nonblocking pin for the whole object (since that is fastest if
> reused), then failing that we try to grab one page in the mappable
> aperture. It also allows us to handle objects larger than the mappable
> aperture (e.g. if we need to pwrite with vGPU restricting the aperture
> to a measely 8MiB or something like that).
>
> v2: Pin pages before starting pwrite, Combined duplicate loops (Chris)
>
> v3: Combined loops based on local patch by Chris (Chris)
>
> Signed-off-by: Ankitprasad Sharma 
> Signed-off-by: Chris Wilson 
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 75 
> +
>  1 file changed, 53 insertions(+), 22 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f1e3fde..9d2e6e3 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -760,20 +760,33 @@ fast_user_write(struct io_mapping *mapping,
>   * user into the GTT, uncached.
>   */
>  static int
> -i915_gem_gtt_pwrite_fast(struct drm_device *dev,
> +i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
>struct drm_i915_gem_object *obj,
>struct drm_i915_gem_pwrite *args,
>struct drm_file *file)
>  {
> - struct drm_i915_private *dev_priv = dev->dev_private;
> - ssize_t remain;
> - loff_t offset, page_base;
> + struct drm_mm_node node;
> + uint64_t remain, offset;
>   char __user *user_data;
> - int page_offset, page_length, ret;
> + int ret;
>  
>   ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE | PIN_NONBLOCK);
> - if (ret)
> - goto out;
> + if (ret) {
> + memset(, 0, sizeof(node));
> + ret = drm_mm_insert_node_in_range_generic(>gtt.base.mm,
> +   , 4096, 0,
> +   I915_CACHE_NONE, 0,
> +   
> i915->gtt.mappable_end,
> +   DRM_MM_SEARCH_DEFAULT,
> +   
> DRM_MM_CREATE_DEFAULT);
> + if (ret)
> + goto out;
> +
> + i915_gem_object_pin_pages(obj);
> + } else {
> + node.start = i915_gem_obj_ggtt_offset(obj);
> + node.allocated = false;
> + }
>  
>   ret = i915_gem_object_set_to_gtt_domain(obj, true);
>   if (ret)
> @@ -783,31 +796,39 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
>   if (ret)
>   goto out_unpin;
>  
> - user_data = to_user_ptr(args->data_ptr);
> - remain = args->size;
> -
> - offset = i915_gem_obj_ggtt_offset(obj) + args->offset;
> -
>   intel_fb_obj_invalidate(obj, ORIGIN_GTT);
> + obj->dirty = true;
>  
> - while (remain > 0) {
> + user_data = to_user_ptr(args->data_ptr);
> + offset = args->offset;
> + remain = args->size;
> + while (remain) {
>   /* Operation in this page
>*
>* page_base = page offset within aperture
>* page_offset = offset within page
>* page_length = bytes to copy for this page
>*/
> - page_base = offset & PAGE_MASK;
> - page_offset = offset_in_page(offset);
> - page_length = remain;
> - if ((page_offset + remain) > PAGE_SIZE)
> - page_length = PAGE_SIZE - page_offset;
> -
> + u32 page_base = node.start;

You truncate here as node.start is 64bit offset into the vm area.

-Mika


> + unsigned page_offset = offset_in_page(offset);
> + unsigned page_length = PAGE_SIZE - page_offset;
> + page_length = remain < page_length ? remain : page_length;
> + if (node.allocated) {
> + wmb();
> + i915->gtt.base.insert_page(>gtt.base,
> +
> i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT),
> +node.start,
> +I915_CACHE_NONE,
> +0);
> + wmb();
> + } else {
> + page_base += offset & PAGE_MASK;
> + }
>   /* If we get a fault while copying data, then (presumably) our
>* source page isn't available.  Return the error and we'll
>* retry in the slow path.
>*/
> - if (fast_user_write(dev_priv->gtt.mappable, page_base,
> + if 

Re: [Intel-gfx] [PATCH 3/3] drm/i915: Move the fbdev async_schedule() into intel_fbdev.c

[Ville Syrjälä]

On Sun, Nov 08, 2015 at 05:44:37PM +0100, Lukas Wunner wrote:
> Hi Ville,
> 
> On Fri, Nov 06, 2015 at 03:08:33PM +0200, ville.syrj...@linux.intel.com wrote:
> > From: Ville Syrjälä 
> > 
> > Reading the driver load/unload code leaves one confused as there's
> > an async_schedule() in the load, but not async_synchronize_full()
> > in sight. In fact it's hidden inside intel_fbdev.c. So let's move the
> > async_schedule() into intel_fbdev.c as well so that it's next to the
> > async_synchronize_full(), which should make the relationship easier
> > to see.
> 
> Hm, what do you think about solving it the other way round, i.e. moving
> the async_synchronize_full() to i915_driver_unload()? Incidentally I was
> working on this same part of the code and that's how I solved it. This way
> it's possible to call intel_fbdev_fini() from intel_fbdev_initial_config().
> With your solution this would deadlock.
> 
> Link: https://github.com/l1k/linux/commit/aa12badac846
> Message-Id: 
> 
> 

I think I'd still like to hide it all in intel_fbdev.c. You could just
split the fbdev_fini() into two parts; one doing the real work, and the
second one just doing async_synchronize + call the first one.

> Thanks,
> 
> Lukas
> 
> 
> > Plus this way we won't schedule a nop function call when fbdev is
> > disabled. And we were passing a pointer to a static inline
> > function to async_schedule(), which seems rather dubious to me.
> > 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >  drivers/gpu/drm/i915/i915_dma.c| 3 +--
> >  drivers/gpu/drm/i915/intel_drv.h   | 4 ++--
> >  drivers/gpu/drm/i915/intel_fbdev.c | 7 ++-
> >  3 files changed, 9 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_dma.c 
> > b/drivers/gpu/drm/i915/i915_dma.c
> > index c58048f..cae3d78 100644
> > --- a/drivers/gpu/drm/i915/i915_dma.c
> > +++ b/drivers/gpu/drm/i915/i915_dma.c
> > @@ -28,7 +28,6 @@
> >  
> >  #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> >  
> > -#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -437,7 +436,7 @@ static int i915_load_modeset_init(struct drm_device 
> > *dev)
> >  * scanning against hotplug events. Hence do this first and ignore the
> >  * tiny window where we will loose hotplug notifactions.
> >  */
> > -   async_schedule(intel_fbdev_initial_config, dev_priv);
> > +   intel_fbdev_initial_config_async(dev);
> >  
> > drm_kms_helper_poll_init(dev);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_drv.h 
> > b/drivers/gpu/drm/i915/intel_drv.h
> > index 00d9882..50c78b6 100644
> > --- a/drivers/gpu/drm/i915/intel_drv.h
> > +++ b/drivers/gpu/drm/i915/intel_drv.h
> > @@ -1298,7 +1298,7 @@ void intel_dvo_init(struct drm_device *dev);
> >  /* legacy fbdev emulation in intel_fbdev.c */
> >  #ifdef CONFIG_DRM_FBDEV_EMULATION
> >  extern int intel_fbdev_init(struct drm_device *dev);
> > -extern void intel_fbdev_initial_config(void *data, async_cookie_t cookie);
> > +extern void intel_fbdev_initial_config_async(struct drm_device *dev);
> >  extern void intel_fbdev_fini(struct drm_device *dev);
> >  extern void intel_fbdev_set_suspend(struct drm_device *dev, int state, 
> > bool synchronous);
> >  extern void intel_fbdev_output_poll_changed(struct drm_device *dev);
> > @@ -1309,7 +1309,7 @@ static inline int intel_fbdev_init(struct drm_device 
> > *dev)
> > return 0;
> >  }
> >  
> > -static inline void intel_fbdev_initial_config(void *data, async_cookie_t 
> > cookie)
> > +static inline void intel_fbdev_initial_config_async(struct drm_device *dev)
> >  {
> >  }
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_fbdev.c 
> > b/drivers/gpu/drm/i915/intel_fbdev.c
> > index 840d6bf..fe1fdb6 100644
> > --- a/drivers/gpu/drm/i915/intel_fbdev.c
> > +++ b/drivers/gpu/drm/i915/intel_fbdev.c
> > @@ -702,7 +702,7 @@ int intel_fbdev_init(struct drm_device *dev)
> > return 0;
> >  }
> >  
> > -void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
> > +static void intel_fbdev_initial_config(void *data, async_cookie_t cookie)
> >  {
> > struct drm_i915_private *dev_priv = data;
> > struct intel_fbdev *ifbdev = dev_priv->fbdev;
> > @@ -711,6 +711,11 @@ void intel_fbdev_initial_config(void *data, 
> > async_cookie_t cookie)
> > drm_fb_helper_initial_config(>helper, ifbdev->preferred_bpp);
> >  }
> >  
> > +void intel_fbdev_initial_config_async(struct drm_device *dev)
> > +{
> > +   async_schedule(intel_fbdev_initial_config, to_i915(dev));
> > +}
> > +
> >  void intel_fbdev_fini(struct drm_device *dev)
> >  {
> > struct drm_i915_private *dev_priv = dev->dev_private;
> > -- 
> > 2.4.10
> > 
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] i-g-t/libdrm email tagging & patchwork

[Damien Lespiau]

On Mon, Nov 09, 2015 at 10:45:14AM +0200, Jani Nikula wrote:
> On Sun, 08 Nov 2015, Damien Lespiau  wrote:
> > There are two new patchwork projects then:
> >
> >   http://patchwork.freedesktop.org/project/intel-gpu-tools/
> >   http://patchwork.freedesktop.org/project/libdrm-intel/
> >
> > I've also run the sorting on all the existing patches so the entries
> > that were historically in the intel-gfx project are now in those new
> > projects.
> 
> Is it possible to manually move patches between projects?

Damn, you noticed! No, it's not (yet?) possible.

> > There is still some work left to limit the noise of those lists of
> > patches, eg. some patches are still marked as New but, in reality, they
> > have been merged. Solving that is quite important and high-ish the TODO
> > list.
> 
> This may be due to git am -3 subtly changing the patch, or the committer
> not-so-subtly changing the patch [*], while applying, and the git hook
> on fdo doesn't recognize the patch. Since we've started to add the Link:
> tag to patches, we could use that extra bit of info in the hook to link
> commits to patchwork.

That would work for us, but not in the general case (for other
projects). I was thinking of using some kind of other heuristic, eg.
(subject, commit message, files touched) with a levenshtein distance on
text to allow typo correction.

Just for us, we could take a shortcut and make dim do something always
correct based on the message-id, I'll have a think.

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

[Intel-gfx] [PATCH 3/3] drm/i915: Use insert_page for pwrite_fast

[ankitprasad . r . sharma]

From: Ankitprasad Sharma 

In pwrite_fast, map an object page by page if obj_ggtt_pin fails. First,
we try a nonblocking pin for the whole object (since that is fastest if
reused), then failing that we try to grab one page in the mappable
aperture. It also allows us to handle objects larger than the mappable
aperture (e.g. if we need to pwrite with vGPU restricting the aperture
to a measely 8MiB or something like that).

v2: Pin pages before starting pwrite, Combined duplicate loops (Chris)

v3: Combined loops based on local patch by Chris (Chris)

Signed-off-by: Ankitprasad Sharma 
Signed-off-by: Chris Wilson 
---
 drivers/gpu/drm/i915/i915_gem.c | 75 +
 1 file changed, 53 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f1e3fde..9d2e6e3 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -760,20 +760,33 @@ fast_user_write(struct io_mapping *mapping,
  * user into the GTT, uncached.
  */
 static int
-i915_gem_gtt_pwrite_fast(struct drm_device *dev,
+i915_gem_gtt_pwrite_fast(struct drm_i915_private *i915,
 struct drm_i915_gem_object *obj,
 struct drm_i915_gem_pwrite *args,
 struct drm_file *file)
 {
-   struct drm_i915_private *dev_priv = dev->dev_private;
-   ssize_t remain;
-   loff_t offset, page_base;
+   struct drm_mm_node node;
+   uint64_t remain, offset;
char __user *user_data;
-   int page_offset, page_length, ret;
+   int ret;
 
ret = i915_gem_obj_ggtt_pin(obj, 0, PIN_MAPPABLE | PIN_NONBLOCK);
-   if (ret)
-   goto out;
+   if (ret) {
+   memset(, 0, sizeof(node));
+   ret = drm_mm_insert_node_in_range_generic(>gtt.base.mm,
+ , 4096, 0,
+ I915_CACHE_NONE, 0,
+ 
i915->gtt.mappable_end,
+ DRM_MM_SEARCH_DEFAULT,
+ 
DRM_MM_CREATE_DEFAULT);
+   if (ret)
+   goto out;
+
+   i915_gem_object_pin_pages(obj);
+   } else {
+   node.start = i915_gem_obj_ggtt_offset(obj);
+   node.allocated = false;
+   }
 
ret = i915_gem_object_set_to_gtt_domain(obj, true);
if (ret)
@@ -783,31 +796,39 @@ i915_gem_gtt_pwrite_fast(struct drm_device *dev,
if (ret)
goto out_unpin;
 
-   user_data = to_user_ptr(args->data_ptr);
-   remain = args->size;
-
-   offset = i915_gem_obj_ggtt_offset(obj) + args->offset;
-
intel_fb_obj_invalidate(obj, ORIGIN_GTT);
+   obj->dirty = true;
 
-   while (remain > 0) {
+   user_data = to_user_ptr(args->data_ptr);
+   offset = args->offset;
+   remain = args->size;
+   while (remain) {
/* Operation in this page
 *
 * page_base = page offset within aperture
 * page_offset = offset within page
 * page_length = bytes to copy for this page
 */
-   page_base = offset & PAGE_MASK;
-   page_offset = offset_in_page(offset);
-   page_length = remain;
-   if ((page_offset + remain) > PAGE_SIZE)
-   page_length = PAGE_SIZE - page_offset;
-
+   u32 page_base = node.start;
+   unsigned page_offset = offset_in_page(offset);
+   unsigned page_length = PAGE_SIZE - page_offset;
+   page_length = remain < page_length ? remain : page_length;
+   if (node.allocated) {
+   wmb();
+   i915->gtt.base.insert_page(>gtt.base,
+  
i915_gem_object_get_dma_address(obj, offset >> PAGE_SHIFT),
+  node.start,
+  I915_CACHE_NONE,
+  0);
+   wmb();
+   } else {
+   page_base += offset & PAGE_MASK;
+   }
/* If we get a fault while copying data, then (presumably) our
 * source page isn't available.  Return the error and we'll
 * retry in the slow path.
 */
-   if (fast_user_write(dev_priv->gtt.mappable, page_base,
+   if (fast_user_write(i915->gtt.mappable, page_base,
page_offset, user_data, page_length)) {
ret = -EFAULT;
goto 

[Intel-gfx] [PATCH v3 0/3] Support for mapping an object page by page

[ankitprasad . r . sharma]

From: Ankitprasad Sharma 

It is possible that when we want to map an object to the aperture, either
we run out of aperture space or the size of the object is larger than
the mappable aperture. In such cases we might not be able to map the whole
object to the aperture. For cases as such, here we introduce insert_page()
which allows us to map a single page in to the mappable aperture space
(which has a higher probabilty of succeeding). This can be iterated
over to access the whole object by using space as meagre as page size.

Here we try to use insert_page() for pwrite_fast in case a nonblocking
pin for the whole object fails, which helps us to iterate over the whole
object and perform the pwrite without mapping the whole object to the
mappable aperture.

We also introduce i915_gem_object_get_dma_address() to perform fast
sequential lookup of the dma address associated with any page within the
object.

Ankitprasad Sharma (1):
  drm/i915: Use insert_page for pwrite_fast

Chris Wilson (2):
  drm/i915: Add support for mapping an object page by page
  drm/i915: Introduce i915_gem_object_get_dma_address()

 drivers/char/agp/intel-gtt.c|  9 +
 drivers/gpu/drm/i915/i915_drv.h | 17 +
 drivers/gpu/drm/i915/i915_gem.c | 75 ++---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 49 
 drivers/gpu/drm/i915/i915_gem_gtt.h |  5 +++
 include/drm/intel-gtt.h |  3 ++
 6 files changed, 136 insertions(+), 22 deletions(-)

-- 
1.9.1

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

[Intel-gfx] [PATCH 1/3] drm/i915: Add support for mapping an object page by page

[ankitprasad . r . sharma]

From: Chris Wilson 

Introduced a new vm specfic callback insert_page() to program a single pte in
ggtt or ppgtt. This allows us to map a single page in to the mappable aperture
space. This can be iterated over to access the whole object by using space as
meagre as page size.

Signed-off-by: Chris Wilson 
Signed-off-by: Ankitprasad Sharma 
---
 drivers/char/agp/intel-gtt.c|  9 +++
 drivers/gpu/drm/i915/i915_gem_gtt.c | 49 +
 drivers/gpu/drm/i915/i915_gem_gtt.h |  5 
 include/drm/intel-gtt.h |  3 +++
 4 files changed, 66 insertions(+)

diff --git a/drivers/char/agp/intel-gtt.c b/drivers/char/agp/intel-gtt.c
index 1341a94..7c68576 100644
--- a/drivers/char/agp/intel-gtt.c
+++ b/drivers/char/agp/intel-gtt.c
@@ -838,6 +838,15 @@ static bool i830_check_flags(unsigned int flags)
return false;
 }
 
+void intel_gtt_insert_page(dma_addr_t addr,
+  unsigned int pg,
+  unsigned int flags)
+{
+   intel_private.driver->write_entry(addr, pg, flags);
+   wmb();
+}
+EXPORT_SYMBOL(intel_gtt_insert_page);
+
 void intel_gtt_insert_sg_entries(struct sg_table *st,
 unsigned int pg_start,
 unsigned int flags)
diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c 
b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 016739e..9fd1f857 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -2345,6 +2345,23 @@ static void gen8_set_pte(void __iomem *addr, gen8_pte_t 
pte)
 #endif
 }
 
+static void gen8_ggtt_insert_page(struct i915_address_space *vm,
+ dma_addr_t addr,
+ uint64_t offset,
+ enum i915_cache_level level,
+ u32 unused)
+{
+   struct drm_i915_private *dev_priv = to_i915(vm->dev);
+   gen8_pte_t __iomem *pte =
+   (gen8_pte_t __iomem *)dev_priv->gtt.gsm +
+   (offset >> PAGE_SHIFT);
+
+   gen8_set_pte(pte, gen8_pte_encode(addr, level, true));
+   wmb();
+
+   I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
+}
+
 static void gen8_ggtt_insert_entries(struct i915_address_space *vm,
 struct sg_table *st,
 uint64_t start,
@@ -2385,6 +2402,23 @@ static void gen8_ggtt_insert_entries(struct 
i915_address_space *vm,
POSTING_READ(GFX_FLSH_CNTL_GEN6);
 }
 
+static void gen6_ggtt_insert_page(struct i915_address_space *vm,
+ dma_addr_t addr,
+ uint64_t offset,
+ enum i915_cache_level level,
+ u32 flags)
+{
+   struct drm_i915_private *dev_priv = to_i915(vm->dev);
+   gen6_pte_t __iomem *pte =
+   (gen6_pte_t __iomem *)dev_priv->gtt.gsm +
+   (offset >> PAGE_SHIFT);
+
+   iowrite32(vm->pte_encode(addr, level, true, flags), pte);
+   wmb();
+
+   I915_WRITE(GFX_FLSH_CNTL_GEN6, GFX_FLSH_CNTL_EN);
+}
+
 /*
  * Binds an object into the global gtt with the specified cache level. The 
object
  * will be accessible to the GPU via commands whose operands reference offsets
@@ -2481,6 +2515,18 @@ static void gen6_ggtt_clear_range(struct 
i915_address_space *vm,
readl(gtt_base);
 }
 
+static void i915_ggtt_insert_page(struct i915_address_space *vm,
+ dma_addr_t addr,
+ uint64_t offset,
+ enum i915_cache_level cache_level,
+ u32 unused)
+{
+   unsigned int flags = (cache_level == I915_CACHE_NONE) ?
+   AGP_USER_MEMORY : AGP_USER_CACHED_MEMORY;
+
+   intel_gtt_insert_page(addr, offset >> PAGE_SHIFT, flags);
+}
+
 static void i915_ggtt_insert_entries(struct i915_address_space *vm,
 struct sg_table *pages,
 uint64_t start,
@@ -2996,6 +3042,7 @@ static int gen8_gmch_probe(struct drm_device *dev,
ret = ggtt_probe_common(dev, gtt_size);
 
dev_priv->gtt.base.clear_range = gen8_ggtt_clear_range;
+   dev_priv->gtt.base.insert_page = gen8_ggtt_insert_page;
dev_priv->gtt.base.insert_entries = gen8_ggtt_insert_entries;
dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
dev_priv->gtt.base.unbind_vma = ggtt_unbind_vma;
@@ -3038,6 +3085,7 @@ static int gen6_gmch_probe(struct drm_device *dev,
ret = ggtt_probe_common(dev, gtt_size);
 
dev_priv->gtt.base.clear_range = gen6_ggtt_clear_range;
+   dev_priv->gtt.base.insert_page = gen6_ggtt_insert_page;
dev_priv->gtt.base.insert_entries = gen6_ggtt_insert_entries;
dev_priv->gtt.base.bind_vma = ggtt_bind_vma;
  

[Intel-gfx] [PATCH 2/3] drm/i915: Introduce i915_gem_object_get_dma_address()

[ankitprasad . r . sharma]

From: Chris Wilson 

This utility function is a companion to i915_gem_object_get_page() that
uses the same cached iterator for the scatterlist to perform fast
sequential lookup of the dma address associated with any page within the
object.

Signed-off-by: Chris Wilson 
Signed-off-by: Ankitprasad Sharma 
---
 drivers/gpu/drm/i915/i915_drv.h | 17 +
 1 file changed, 17 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d2a546a..548a0eb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2862,6 +2862,23 @@ static inline int __sg_page_count(struct scatterlist *sg)
return sg->length >> PAGE_SHIFT;
 }
 
+static inline dma_addr_t
+i915_gem_object_get_dma_address(struct drm_i915_gem_object *obj, int n)
+{
+   if (n < obj->get_page.last) {
+   obj->get_page.sg = obj->pages->sgl;
+   obj->get_page.last = 0;
+   }
+
+   while (obj->get_page.last + __sg_page_count(obj->get_page.sg) <= n) {
+   obj->get_page.last += __sg_page_count(obj->get_page.sg++);
+   if (unlikely(sg_is_chain(obj->get_page.sg)))
+   obj->get_page.sg = sg_chain_ptr(obj->get_page.sg);
+   }
+
+   return sg_dma_address(obj->get_page.sg) + ((n - obj->get_page.last) << 
PAGE_SHIFT);
+}
+
 static inline struct page *
 i915_gem_object_get_page(struct drm_i915_gem_object *obj, int n)
 {
-- 
1.9.1

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

Re: [Intel-gfx] i-g-t/libdrm email tagging & patchwork

[Jani Nikula]

On Mon, 09 Nov 2015, Damien Lespiau  wrote:
> That would work for us, but not in the general case (for other
> projects). I was thinking of using some kind of other heuristic, eg.
> (subject, commit message, files touched) with a levenshtein distance on
> text to allow typo correction.
>
> Just for us, we could take a shortcut and make dim do something always
> correct based on the message-id, I'll have a think.

Do you mean we'd move the patchwork update to client side rather than
server side?

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] i-g-t/libdrm email tagging & patchwork

[Damien Lespiau]

On Mon, Nov 09, 2015 at 01:21:33PM +0200, Jani Nikula wrote:
> On Mon, 09 Nov 2015, Damien Lespiau  wrote:
> > That would work for us, but not in the general case (for other
> > projects). I was thinking of using some kind of other heuristic, eg.
> > (subject, commit message, files touched) with a levenshtein distance on
> > text to allow typo correction.
> >
> > Just for us, we could take a shortcut and make dim do something always
> > correct based on the message-id, I'll have a think.
> 
> Do you mean we'd move the patchwork update to client side rather than
> server side?

We could do it either way, have dim use git-pw to mark the patch as
accepted or have the post commit hook look at the brand new tag and
infer the id of the patch to close. I'm guessing you'd rather have the
second option.

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

[Intel-gfx] [PATCH] drm/i915/bxt: Broxton doesn't use gen9 scaling for rps frequencies.

[Bob Paauwe]

Signed-off-by: Bob Paauwe 
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=92768
---
 drivers/gpu/drm/i915/intel_pm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 647c0ff..fc5097f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7138,7 +7138,7 @@ static int chv_freq_opcode(struct drm_i915_private 
*dev_priv, int val)
 
 int intel_gpu_freq(struct drm_i915_private *dev_priv, int val)
 {
-   if (IS_GEN9(dev_priv->dev))
+   if (IS_GEN9(dev_priv->dev) && !IS_BROXTON(dev_priv->dev))
return (val * GT_FREQUENCY_MULTIPLIER) / GEN9_FREQ_SCALER;
else if (IS_CHERRYVIEW(dev_priv->dev))
return chv_gpu_freq(dev_priv, val);
@@ -7150,7 +7150,7 @@ int intel_gpu_freq(struct drm_i915_private *dev_priv, int 
val)
 
 int intel_freq_opcode(struct drm_i915_private *dev_priv, int val)
 {
-   if (IS_GEN9(dev_priv->dev))
+   if (IS_GEN9(dev_priv->dev) && !IS_BROXTON(dev_priv->dev))
return (val * GEN9_FREQ_SCALER) / GT_FREQUENCY_MULTIPLIER;
else if (IS_CHERRYVIEW(dev_priv->dev))
return chv_freq_opcode(dev_priv, val);
-- 
2.4.3

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

[Intel-gfx] [PATCH] drm/i915/skl: implement DP Aux Mutex framework

[Wayne Boyer]

Beginning with SKL the DP Aux channel communication can be protected
using a built in H/W mutex.

This patch provides an initial implementation for using that mutex.
The use is currently limited to protecting the sink crc request based
on feedback from the H/W designers indicating that using the mutex
for all aux channel communication is not recommended.

Cc: Rodrigo Vivi 
Signed-off-by: Wayne Boyer 
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_reg.h |  5 
 drivers/gpu/drm/i915/intel_dp.c | 52 -
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b12594b..98e991d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2604,6 +2604,7 @@ struct drm_i915_cmd_table {
 
 #define HAS_DDI(dev)   (INTEL_INFO(dev)->has_ddi)
 #define HAS_FPGA_DBG_UNCLAIMED(dev)(INTEL_INFO(dev)->has_fpga_dbg)
+#define HAS_AUX_MUTEX(dev) (INTEL_INFO(dev)->gen >= 9)
 #define HAS_PSR(dev)   (IS_HASWELL(dev) || IS_BROADWELL(dev) || \
 IS_VALLEYVIEW(dev) || IS_CHERRYVIEW(dev) || \
 IS_SKYLAKE(dev) || IS_KABYLAKE(dev))
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 8942532..a033e70 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4288,6 +4288,11 @@ enum skl_disp_power_wells {
 #define   DP_AUX_CH_CTL_FW_SYNC_PULSE_SKL(c) (((c) - 1) << 5)
 #define   DP_AUX_CH_CTL_SYNC_PULSE_SKL(c)   ((c) - 1)
 
+#define DP_AUX_MUTEX_A 0x6402C
+#define DP_AUX_MUTEX_B 0x6412C
+#define   DP_AUX_MUTEX_ENABLE  (1 << 31)
+#define   DP_AUX_MUTEX_STATUS  (1 << 30)
+
 /*
  * Computing GMCH M and N values for the Display Port link
  *
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 2fad873..e9e1239 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -781,6 +781,47 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp 
*intel_dp,
   DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
 }
 
+static bool skl_aux_mutex(struct intel_dp *intel_dp, bool get)
+{
+   struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
+   struct drm_device *dev = intel_dig_port->base.base.dev;
+   struct drm_i915_private *dev_priv = dev->dev_private;
+   uint32_t aux_ch_mutex, status;
+   int count = 0;
+
+   if (!HAS_AUX_MUTEX(dev))
+   return false;
+
+   /*
+* FIXME: determine actual aux channel
+* Hard coded to channel A for now to protect sink crc requests on eDP.
+*/
+   aux_ch_mutex = DP_AUX_MUTEX_A;
+
+   if (!get) {
+   I915_WRITE(aux_ch_mutex, DP_AUX_MUTEX_ENABLE | 
DP_AUX_MUTEX_STATUS);
+   return false;
+   }
+
+   /*
+* The Bspec specifies waiting 500us between attempts to acquire the
+* mutex.  Ten retries should be adequate to balance successfully
+* acquirng the mutex and spending too much time trying.
+*/
+   while (count++ < 10) {
+   I915_WRITE(aux_ch_mutex, DP_AUX_MUTEX_ENABLE);
+   status = I915_READ(aux_ch_mutex);
+   if (!(status & DP_AUX_MUTEX_STATUS))
+   return true;
+   udelay(500);
+   }
+
+   return false;
+}
+
+#define skl_aux_mutex_get(dev) skl_aux_mutex(dev, true)
+#define skl_aux_mutex_put(dev) skl_aux_mutex(dev, false)
+
 static int
 intel_dp_aux_ch(struct intel_dp *intel_dp,
const uint8_t *send, int send_bytes,
@@ -4188,10 +4229,14 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 
*crc)
u8 buf;
int count, ret;
int attempts = 6;
+   bool aux_mutex_acquired = false;
+
+   aux_mutex_acquired = skl_aux_mutex_get(intel_dp);
 
ret = intel_dp_sink_crc_start(intel_dp);
+
if (ret)
-   return ret;
+   goto release;
 
do {
intel_wait_for_vblank(dev, intel_crtc->pipe);
@@ -4218,6 +4263,11 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 
 stop:
intel_dp_sink_crc_stop(intel_dp);
+
+release:
+   if (aux_mutex_acquired)
+   aux_mutex_acquired = skl_aux_mutex_put(intel_dp);
+
return ret;
 }
 
-- 
1.9.1

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

Re: [Intel-gfx] [PATCH v2 3/4] drm/i915: make assert_device_not_suspended more precise

[Chris Wilson]

On Mon, Nov 09, 2015 at 09:13:45PM +0200, Imre Deak wrote:
> Atm, we assert that the device is not suspended after the point when the
> HW is truly put to a suspended state. This is fine, but we can catch
> more problems if we check the RPM refcount. After that one drops to zero
> we shouldn't access the HW any more, although the actual suspend may be
> delayed. The only complication is that we want to avoid asserts while
> the suspend handler itself is running, so add a flag to handle this
> case.
> 
> While at it remove the HAS_RUNTIME_PM check, the pm.suspended flag is
> false and the RPM refcount is non-zero on all platforms that don't
> support RPM.
> 
> This caught additional WARNs from the atomic path, those will be fixed
> as a follow-up.
> 
> v2:
> - remove the redundant HAS_RUNTIME_PM check (Ville)
> 
> Signed-off-by: Imre Deak 
> ---
> --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> @@ -2120,8 +2120,18 @@ void intel_power_domains_init_hw(struct 
> drm_i915_private *dev_priv, bool resume)
>  
>  void assert_device_not_suspended(struct drm_i915_private *dev_priv)
>  {
> - WARN_ONCE(HAS_RUNTIME_PM(dev_priv->dev) && dev_priv->pm.suspended,
> -   "Device suspended\n");
> + int rpm_usage;
> +
> + if (dev_priv->pm.disable_suspended_assert)
> + return;
> +
> +#ifdef CONFIG_PM
> + rpm_usage = atomic_read(_priv->dev->dev->power.usage_count);
> +#else
> + rpm_usage = 1;
> +#endif

Whilst this should fix the issue I was worried about, I think for e.g.
the GGTT PTE access, we should be checking that we have a rpm ref (i.e.
we have called intel_runtime_pm_get()). Bonus points if we can narrow
that down to being inside an rpm critical section (made tricky because
the wakelocks can nest :(. The simplest way does impose an extra atomic
inc/dec simply for debug purposes, on the other hand it shouldn't then
need the pm.disable_suspended_assert and you can have an extra assert
that we cannot set pm.suspended whilst intel_runtime_pm_get() is held.

Otherwise, yeah just rename this to imply we aren't just checking that
the device isn't suspended right now, but cannot be.
-Chris

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

Re: [Intel-gfx] [PATCH] drm/i915: A better maximum brightness for users.

[Shih-Yuan Lee (FourDollars)]

On Mon, Nov 9, 2015 at 6:17 PM, Jani Nikula 
wrote:

> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" 
> wrote:
> > The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937 however
> > the sysfs brightness level always starts from 0 so it is better to use
> > 927 as the sysfs maximum brightness level and it becomes easier to map
> > from the PWM brightness level to the sysfs brightness level.
>
> We've been thinking we should provide a fixed range to userspace
> instead. Say, 0-100.
>
> BR,
> Jani.
>
>
>
> That might not be a good idea for the backward compatibility.
However I saw some message as the following.
[3.402233] [drm:parse_lfp_backlight] VBT backlight PWM modulation
frequency 200 Hz, active high, min brightness 10, level 255


Does it mean the brightness range is also defined in the BIOS?

Regards,
$4

>
> >
> > Signed-off-by: Shih-Yuan Lee (FourDollars) 
> > ---
> >  drivers/gpu/drm/i915/intel_panel.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_panel.c
> b/drivers/gpu/drm/i915/intel_panel.c
> > index a24df35..697fd4d 100644
> > --- a/drivers/gpu/drm/i915/intel_panel.c
> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> > @@ -1211,7 +1211,7 @@ static int intel_backlight_device_register(struct
> intel_connector *connector)
> >* Note: Everything should work even if the backlight device max
> >* presented to the userspace is arbitrarily chosen.
> >*/
> > - props.max_brightness = panel->backlight.max;
> > + props.max_brightness = panel->backlight.max - panel->backlight.min;
> >   props.brightness = scale_hw_to_user(connector,
> >   panel->backlight.level,
> >   props.max_brightness);
>
> --
> Jani Nikula, Intel Open Source Technology Center
>



-- 
Shih-Yuan Lee (FourDollars) | Software Engineer | Commercial Engineering -
PC & Core Taipei | Ubuntu Engineering and Services | Canonical
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 24/31] drm/i915: PSR: Don't Skip aux handshake on DP_PSR_NO_TRAIN_ON_EXIT.

[Jani Nikula]

On Thu, 05 Nov 2015, Rodrigo Vivi  wrote:
> Since the beginning there is a confusion on the meaning of this bit.
>
> A previous patch had identified this already and fixed it partially:
> 'commit 3301d409 ("drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT logic")
>
> DP_PSR_NO_TRAIN_ON_EXIT means the source doesn't need to do the
> training, but it doesn't tell to avoid TP patterns or to skip
> aux handshake.
>
> This patch fixes the hard freeze reported.
>
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91436
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91437
>
> Cc: Ivan Mitev 
> Signed-off-by: Rodrigo Vivi 

I'm unhappy about mixing fixes like this in 30+ patch series.

Jani.


> ---
>  drivers/gpu/drm/i915/intel_psr.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index c090f38..4e88e2e 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -263,7 +263,6 @@ static void hsw_psr_enable_source(struct intel_dp 
> *intel_dp)
>  send the minimal TP1 possible and skip TP2. */
>   val |= EDP_PSR_TP1_TIME_100us;
>   val |= EDP_PSR_TP2_TP3_TIME_0us;
> - val |= EDP_PSR_SKIP_AUX_EXIT;
>   /* Sink should be able to train with the 5 or 6 idle patterns */
>   idle_frames += 4;
>   }

-- 
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 25/31] drm/i915: Send TP1 TP2/3 even when panel claims no NO_TRAIN_ON_EXIT.

[Jani Nikula]

On Thu, 05 Nov 2015, Rodrigo Vivi  wrote:
> On the commit 3301d4092106 ("drm/i915: PSR: Fix DP_PSR_NO_TRAIN_ON_EXIT 
> logic")'
> we already had identified that DP_PSR_NO_TRAIN_ON_EXIT
> doesn't mean we shouldn't send TPS patterns, however we start sending the
> minimal TP1 as possible and no TP2.
>
> For most of the panels this is ok, but we found a reported case where
> this is not true and panel keeps frozen without updating the screen for a 
> while.
>
> We could just get this case after patch "PSR: Don't Skip aux handshake on
> DP_PSR_NO_TRAIN_ON_EXIT." is applied since that one fix the
> hard freeze on this kind of panels.
>
> Reference: https://bugs.freedesktop.org/show_bug.cgi?id=91436#c19
>
> Cc: Ivan Mitev 
> Signed-off-by: Rodrigo Vivi 

Ditto. Do these really depend on all the other patches in the series?

Jani.

> ---
>  drivers/gpu/drm/i915/intel_psr.c | 4 
>  1 file changed, 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_psr.c 
> b/drivers/gpu/drm/i915/intel_psr.c
> index 4e88e2e..ee426ea 100644
> --- a/drivers/gpu/drm/i915/intel_psr.c
> +++ b/drivers/gpu/drm/i915/intel_psr.c
> @@ -259,10 +259,6 @@ static void hsw_psr_enable_source(struct intel_dp 
> *intel_dp)
>   const uint32_t link_entry_time = EDP_PSR_MIN_LINK_ENTRY_TIME_8_LINES;
>  
>   if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT) {
> - /* It doesn't mean we shouldn't send TPS patters, so let's
> -send the minimal TP1 possible and skip TP2. */
> - val |= EDP_PSR_TP1_TIME_100us;
> - val |= EDP_PSR_TP2_TP3_TIME_0us;
>   /* Sink should be able to train with the 5 or 6 idle patterns */
>   idle_frames += 4;
>   }

-- 
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] drm/i915: A better maximum brightness for users.

[Jani Nikula]

On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)"  wrote:
> On Mon, Nov 9, 2015 at 6:17 PM, Jani Nikula 
> wrote:
>
>> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" 
>> wrote:
>> > The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937 however
>> > the sysfs brightness level always starts from 0 so it is better to use
>> > 927 as the sysfs maximum brightness level and it becomes easier to map
>> > from the PWM brightness level to the sysfs brightness level.
>>
>> We've been thinking we should provide a fixed range to userspace
>> instead. Say, 0-100.
>>
>> BR,
>> Jani.
>>
>>
>>
> That might not be a good idea for the backward compatibility.

Please explain how you think your change is good and a fixed range 0-100
is bad in terms of backward compatibility. Both just arbitrarily change
the max.

Besides, we've changed the max for some platforms before, and the ABI
for backlight sysfs is you can stick a value between 0 and
max_brightness to the brightness attribute. Any userspace that relies on
anything else is broken.

> However I saw some message as the following.
> [3.402233] [drm:parse_lfp_backlight] VBT backlight PWM modulation
> frequency 200 Hz, active high, min brightness 10, level 255
>
>
> Does it mean the brightness range is also defined in the BIOS?

The minimum and the PWM modulation frequency are defined in BIOS. The
modulation frequency is an attribute for the hardware; I think that was
originally exposed as the max was just for implementation convenience.

BR,
Jani.


>
> Regards,
> $4
>
>>
>> >
>> > Signed-off-by: Shih-Yuan Lee (FourDollars) 
>> > ---
>> >  drivers/gpu/drm/i915/intel_panel.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/intel_panel.c
>> b/drivers/gpu/drm/i915/intel_panel.c
>> > index a24df35..697fd4d 100644
>> > --- a/drivers/gpu/drm/i915/intel_panel.c
>> > +++ b/drivers/gpu/drm/i915/intel_panel.c
>> > @@ -1211,7 +1211,7 @@ static int intel_backlight_device_register(struct
>> intel_connector *connector)
>> >* Note: Everything should work even if the backlight device max
>> >* presented to the userspace is arbitrarily chosen.
>> >*/
>> > - props.max_brightness = panel->backlight.max;
>> > + props.max_brightness = panel->backlight.max - panel->backlight.min;
>> >   props.brightness = scale_hw_to_user(connector,
>> >   panel->backlight.level,
>> >   props.max_brightness);
>>
>> --
>> 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

Re: [Intel-gfx] [PATCH] drm/i915: A better maximum brightness for users.

[Jani Nikula]

On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)"  wrote:
> The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937 however
> the sysfs brightness level always starts from 0 so it is better to use
> 927 as the sysfs maximum brightness level and it becomes easier to map
> from the PWM brightness level to the sysfs brightness level.

We've been thinking we should provide a fixed range to userspace
instead. Say, 0-100.

BR,
Jani.




>
> Signed-off-by: Shih-Yuan Lee (FourDollars) 
> ---
>  drivers/gpu/drm/i915/intel_panel.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_panel.c 
> b/drivers/gpu/drm/i915/intel_panel.c
> index a24df35..697fd4d 100644
> --- a/drivers/gpu/drm/i915/intel_panel.c
> +++ b/drivers/gpu/drm/i915/intel_panel.c
> @@ -1211,7 +1211,7 @@ static int intel_backlight_device_register(struct 
> intel_connector *connector)
>* Note: Everything should work even if the backlight device max
>* presented to the userspace is arbitrarily chosen.
>*/
> - props.max_brightness = panel->backlight.max;
> + props.max_brightness = panel->backlight.max - panel->backlight.min;
>   props.brightness = scale_hw_to_user(connector,
>   panel->backlight.level,
>   props.max_brightness);

-- 
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] drm/i915/guc: Fix a fw content lost issue after it is evicted

[Chris Wilson]

On Fri, Nov 06, 2015 at 03:18:37PM -0800, Yu Dai wrote:
> 
> 
> On 11/06/2015 02:07 PM, Chris Wilson wrote:
> >On Fri, Nov 06, 2015 at 01:55:27PM -0800, yu@intel.com wrote:
> >> From: Alex Dai 
> >>
> >> We keep a copy of GuC fw in a GEM obj. However its content is lost
> >> if the GEM obj is evicted (igt/gem_evict_*). Therefore, the later
> >> fw loading during GPU reset will fail.
> >
> >No, it's not. The bug is in sg_copy_buffer called by
> >i915_gem_object_create_from_data introduced by yourselves.
> >
> 
> My understanding is that sg_copy_from_buffer is used to copy data.
> Can you clarify why using this will cause such issue?

"However its content is lost if the GEM obj is evicted (igt/gem_evict_*)."

is not strictly true. The content is lost if the object is swapped
because the page is never marked as dirty. sg_copy_buffer() is copying
into the page but never marks said page as dirty.
-Chris

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

Re: [Intel-gfx] [PATCH] drm: Add aux plane verification in addFB2

[Kannan, Vandana]

On 11/5/2015 10:50 PM, Vandana Kannan wrote:

From: Daniel Vetter 

For render compression, userspace passes aux stride and offset values as an
additional entry in the fb structure. This should not be treated as garbage
and discarded as data belonging to no plane.
This patch introduces a check related to AUX plane to support the
scenario of render compression.

v2: Based on a discussion with Siva
Moving num_planes check below the increment.

Changing the author to Daniel instead of suggested-by, since the core logic is
his.

Signed-off-by: Daniel Vetter 
Signed-off-by: Vandana Kannan 
Cc: Sivakumar Thulasimani 
---
  drivers/gpu/drm/drm_crtc.c  | 19 ++-
  drivers/gpu/drm/drm_ioctl.c |  3 +++
  include/drm/drm_crtc.h  |  3 +++
  include/uapi/drm/drm.h  |  1 +
  include/uapi/drm/drm_mode.h |  1 +
  5 files changed, 26 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 24c5434..0d1030b 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -3204,6 +3204,16 @@ static int framebuffer_check(const struct 
drm_mode_fb_cmd2 *r)
}
}

+   if (r->flags & DRM_MODE_FB_AUX_PLANE) {
+   num_planes++;
+
+   if (num_planes == 4) {
+   DRM_DEBUG_KMS("Number of planes cannot exceed 3"
+   "(including aux plane)\n");
+   return -EINVAL;
+   }
+   }
+
for (i = num_planes; i < 4; i++) {
if (r->modifier[i]) {
DRM_DEBUG_KMS("non-zero modifier for unused plane 
%d\n", i);
@@ -3242,7 +3252,8 @@ internal_framebuffer_create(struct drm_device *dev,
struct drm_framebuffer *fb;
int ret;

-   if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS)) {
+   if (r->flags & ~(DRM_MODE_FB_INTERLACED | DRM_MODE_FB_MODIFIERS |
+   DRM_MODE_FB_AUX_PLANE)) {
DRM_DEBUG_KMS("bad framebuffer flags 0x%08x\n", r->flags);
return ERR_PTR(-EINVAL);
}
@@ -3264,6 +3275,12 @@ internal_framebuffer_create(struct drm_device *dev,
return ERR_PTR(-EINVAL);
}

+   if (r->flags & DRM_MODE_FB_AUX_PLANE &&
+   !dev->mode_config.allow_aux_plane) {
+   DRM_DEBUG_KMS("driver does not support render compression\n");
+   return ERR_PTR(-EINVAL);
+   }
+
ret = framebuffer_check(r);
if (ret)
return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
index 8ce2a0c..ee00782 100644
--- a/drivers/gpu/drm/drm_ioctl.c
+++ b/drivers/gpu/drm/drm_ioctl.c
@@ -312,6 +312,9 @@ static int drm_getcap(struct drm_device *dev, void *data, 
struct drm_file *file_
case DRM_CAP_ADDFB2_MODIFIERS:
req->value = dev->mode_config.allow_fb_modifiers;
break;
+   case DRM_CAP_RENDER_COMPRESSION:
+   req->value = dev->mode_config.allow_aux_plane;
+   break;
default:
return -EINVAL;
}
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 3f0c690..a5a9da2 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -1152,6 +1152,9 @@ struct drm_mode_config {
/* whether the driver supports fb modifiers */
bool allow_fb_modifiers;

+   /* whether the driver supports render compression */
+   bool allow_aux_plane;
+
/* cursor size */
uint32_t cursor_width, cursor_height;
  };
diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
index 3801584..0834bf7 100644
--- a/include/uapi/drm/drm.h
+++ b/include/uapi/drm/drm.h
@@ -631,6 +631,7 @@ struct drm_gem_open {
  #define DRM_CAP_CURSOR_WIDTH  0x8
  #define DRM_CAP_CURSOR_HEIGHT 0x9
  #define DRM_CAP_ADDFB2_MODIFIERS  0x10
+#define DRM_CAP_RENDER_COMPRESSION 0x11

  /** DRM_IOCTL_GET_CAP ioctl argument type */
  struct drm_get_cap {
diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h
index 6c11ca4..de59ace 100644
--- a/include/uapi/drm/drm_mode.h
+++ b/include/uapi/drm/drm_mode.h
@@ -354,6 +354,7 @@ struct drm_mode_fb_cmd {

  #define DRM_MODE_FB_INTERLACED(1<<0) /* for interlaced framebuffers */
  #define DRM_MODE_FB_MODIFIERS (1<<1) /* enables ->modifer[] */
+#define DRM_MODE_FB_AUX_PLANE   (1<<2) /* for compressed buffer */

  struct drm_mode_fb_cmd2 {
__u32 fb_id;


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

Re: [Intel-gfx] i-g-t/libdrm email tagging & patchwork

[Jani Nikula]

On Sun, 08 Nov 2015, Damien Lespiau  wrote:
> There are two new patchwork projects then:
>
>   http://patchwork.freedesktop.org/project/intel-gpu-tools/
>   http://patchwork.freedesktop.org/project/libdrm-intel/
>
> I've also run the sorting on all the existing patches so the entries
> that were historically in the intel-gfx project are now in those new
> projects.

Is it possible to manually move patches between projects?

> There is still some work left to limit the noise of those lists of
> patches, eg. some patches are still marked as New but, in reality, they
> have been merged. Solving that is quite important and high-ish the TODO
> list.

This may be due to git am -3 subtly changing the patch, or the committer
not-so-subtly changing the patch [*], while applying, and the git hook
on fdo doesn't recognize the patch. Since we've started to add the Link:
tag to patches, we could use that extra bit of info in the hook to link
commits to patchwork.

BR,
Jani.


[*] I'd go for requiring resubmission even for the tiniest typo/comment
changes, but Daniel thinks the committers can do the fixups.


-- 
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] drm/i915: quirk backlight present on Macbook 4, 1

[Ander Conselvan De Oliveira]

Reviewed-by: Ander Conselvan de Oliveira 

On Thu, 2015-11-05 at 11:49 +0200, Jani Nikula wrote:
> Unsurprisingly macbooks have backlights, just the VBT doesn't seem to
> know it in this case.
> 
> Reported-and-tested-by: Daniel Nicoletti 
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88325
> Fixes: c675949ec58c ("drm/i915: do not setup backlight if not available
> according to VBT")
> Cc: sta...@vger.kernel.org # v3.15+
> Signed-off-by: Jani Nikula 
> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 7b3cfb64e7f6..b067147183e9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14875,6 +14875,9 @@ static struct intel_quirk intel_quirks[] = {
>   /* Apple Macbook 2,1 (Core 2 T7400) */
>   { 0x27a2, 0x8086, 0x7270, quirk_backlight_present },
>  
> + /* Apple Macbook 4,1 */
> + { 0x2a02, 0x106b, 0x00a1, quirk_backlight_present },
> +
>   /* Toshiba CB35 Chromebook (Celeron 2955U) */
>   { 0x0a06, 0x1179, 0x0a88, quirk_backlight_present },
>  
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] Hardlock problems when switching monitor channels and questions (Skylake)

[Ander Conselvan De Oliveira]

On Sun, 2015-11-01 at 12:48 +0100, Maarten Maathuis wrote:
> Hi everyone.
> 
> I've been some hardlock problems when switching from the HDMI connection of my
> monitor (connected to another PC) and the displayport (connected to the
> problematic PC), several times a week at least.
> 
> In an effort to narrow down the problem I've been tried looking at drm modeset
> debug information, as well try catch something with netconsole. Neither have
> pointed me the root cause yet, but i do have some observations that I cannot
> put into place yet and would like some feedback on prior to opening a bug
> report.
> 
> My mainbord is: Asrock Fatal1ty Z170 Gaming-ITX/ac
> My CPU is: Intel i7 6700
> My monitor is: Samsung U24E850, equiped with both displayport 1.2 and HDMI 2.0
> 
> Kernel version: 4.3-RC7
> 
> The display connections on the mainbord are:
> - DP 1.2
> - HDMI 1.4
> - HDMI 2.0 via DP 1.2 (recognized as DP by driver)
> 
> I notice some funny things I would like some feedback on.
> 
> 1. The EDID retrieval doesn't always succeed, i've noticed this sometimes by
> running "xrandr" after starting up. I've also noticed it after rebooting from
> a hard lock that it goes into 1024x768 resolution instead of 3840x2160, it
> takes another reboot to get it right. As if it relies on the resolution the
> boot used? How to troubleshoot this?
> 
> 2. I notice that the driver thinks there are 3 HDMI connections, 2 of which
> seem to share the same hotplug pin as the 2 DP connections, what is going
> here? Could this cause trouble? The fake HDMI connections seem always to fail
> edid retrieval, but I'm not a 100% sure if this also happens in the hardlock
> situation (no evidence yet).
> 
> 3. Switching between HDMI and DP channel on my monitor sometimes results in a
> hotplug event and sometimes it does not? Any idea why this happens? I'm
> curious about this because the hardlocks occur after spending a few hours on
> the HDMI channel typically.
> 
> Anything I should be looking for before going down the route of a bug report?

It would be better to file a bug report and attach the output of dmesg from boot
while running with drm.debug=0xe in the kernel command line. Further instruction
s in the link below.

https://01.org/linuxgraphics/documentation/how-report-bugs

Ander

> 
> Sincerely,
> 
> Maarten.
> 
> P.S. I'm not subscribed to this mailinglist, so please CC me. 
> 
> -- 
> Far away from the primal instinct, the song seems to fade away, the river get
> wider between your thoughts and the things we do and say.
> ___
> 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 07/31] drm/i915: IPS Sysfs interface.

[Daniel Stone]

Hi Rodrigo,

On 5 November 2015 at 18:49, Rodrigo Vivi  wrote:
> diff --git a/drivers/gpu/drm/i915/intel_ips.c 
> b/drivers/gpu/drm/i915/intel_ips.c
> index b867aba..6bc5c55 100644
> --- a/drivers/gpu/drm/i915/intel_ips.c
> +++ b/drivers/gpu/drm/i915/intel_ips.c
> @@ -105,18 +105,21 @@ bool intel_ips_ready(struct intel_crtc *crtc,
>   * This function is called to enable IPS on certain pipe.
>   * All needed conditions should've checked already by intel_ips_ready.
>   */
> -void intel_ips_enable(struct intel_crtc *crtc)
> +int intel_ips_enable(struct intel_crtc *crtc)
>  {
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> +   int ret = 0;
>
> if (!crtc->config->ips_ready)
> -   return;
> +   return -EINVAL;
>
> mutex_lock(_priv->display_ips.lock);
>
> -   if (dev_priv->display_ips.enabled)
> +   if (dev_priv->display_ips.enabled) {
> +   ret = -EALREADY;
> goto out;
> +   }
>
> /*
>  * We can only enable IPS after we enable a plane
> @@ -147,6 +150,7 @@ void intel_ips_enable(struct intel_crtc *crtc)
>  */
> if (wait_for(I915_READ_NOTRACE(IPS_CTL) & IPS_ENABLE, 50)) {
> DRM_ERROR("Timed out waiting for IPS enable\n");
> +   ret = -ETIMEDOUT;
> goto out;
> }
> }
> @@ -154,6 +158,7 @@ void intel_ips_enable(struct intel_crtc *crtc)
> dev_priv->display_ips.enabled = true;
>  out:
> mutex_unlock(_priv->display_ips.lock);
> +   return ret;
>  }
>
>  /**
> @@ -162,16 +167,22 @@ out:
>   *
>   * This function is called to disable IPS on certain pipe whenever it is 
> needed
>   * to disable IPS on the pipe.
> + *
> + * Returns:
> + * 0 on success and -errno otherwise.
>   */
> -void intel_ips_disable(struct intel_crtc *crtc)
> +int intel_ips_disable(struct intel_crtc *crtc)
>  {
> struct drm_device *dev = crtc->base.dev;
> struct drm_i915_private *dev_priv = dev->dev_private;
> +   int ret = 0;
>
> mutex_lock(_priv->display_ips.lock);
>
> -   if (!dev_priv->display_ips.enabled)
> +   if (!dev_priv->display_ips.enabled) {
> +   ret = -EALREADY;
> goto out;
> +   }
>
> assert_plane_enabled(dev_priv, crtc->plane);
> if (IS_BROADWELL(dev)) {
> @@ -196,6 +207,7 @@ void intel_ips_disable(struct intel_crtc *crtc)
> dev_priv->display_ips.enabled = false;
>  out:
> mutex_unlock(_priv->display_ips.lock);
> +   return ret;
>  }

It would be nice to have these from the beginning, rather than
modifying them part-way through.

> @@ -206,6 +218,9 @@ out:
>   * It checks if there is any other plane enabled on the pipe when primary is
>   * going to be disabled. In this case IPS can continue enabled, but it needs
>   * to be disabled otherwise.
> + *
> + * Returns:
> + * 0 on success and -errno otherwise.
>   */
>  void intel_ips_disable_if_alone(struct intel_crtc *crtc)
>  {

... this one is still left void.

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

Re: [Intel-gfx] [PATCH 00/31] IPS/DRRS/PSR rework with PSR enabled by default

[Daniel Stone]

Hi Rodrigo,

On 5 November 2015 at 18:49, Rodrigo Vivi  wrote:
> So I'm confident we can enable PSR back by default now.
>
> All comments, ideas, suggestions and even bikesheddings are pretty welcome.

You did ask for it ...

I've been looking at pulling this on top of Maarten's tree, and
currently my overriding wish is that, rather than the checks sprinkled
all over various state-change functions, we instead had:
static bool intel_ips_should_enable(struct intel_crtc_state *crtc_state)

In the pre-atomic commit path, this could look like:
bool ips = intel_ips_should_enable(crtc_state);
if (ips && !intel_crtc->ips_enabled)
intel_ips_enable(intel_crtc);
else if (!ips && intel_crtc->ips_enabled)
intel_ips_disable(intel_crtc);

Post-atomic, this would be:
intel_flip_work->enable_ips = intel_ips_should_enable(crtc_state);
and actually doing the enable/disable in the work handler.

Having one place to inspect the state overall seems better, e.g. in
the case where we disable the primary plane but retain an overlay
plane on a CRTC, we keep IPS enabled. However, it doesn't seem like
there's anything to handle the case where we then disable that overlay
plane, where with no planes enabled at all, IPS should be disabled.

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

Re: [Intel-gfx] [PATCH] drm/i915: A better maximum brightness for users.

[Shih-Yuan Lee (FourDollars)]

On Mon, Nov 9, 2015 at 6:51 PM, Jani Nikula 
wrote:

> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)" 
> wrote:
> > On Mon, Nov 9, 2015 at 6:17 PM, Jani Nikula  >
> > wrote:
> >
> >> On Mon, 09 Nov 2015, "Shih-Yuan Lee (FourDollars)"  >
> >> wrote:
> >> > The PWM brightness level of Dell XPS 13 (2015) is from 10 to 937
> however
> >> > the sysfs brightness level always starts from 0 so it is better to use
> >> > 927 as the sysfs maximum brightness level and it becomes easier to map
> >> > from the PWM brightness level to the sysfs brightness level.
> >>
> >> We've been thinking we should provide a fixed range to userspace
> >> instead. Say, 0-100.
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >>
> > That might not be a good idea for the backward compatibility.
>
> Please explain how you think your change is good and a fixed range 0-100
> is bad in terms of backward compatibility. Both just arbitrarily change
> the max.
>
The original sysfs brightness range is from 0 to 937. Let's define it as A
= {0, 1, 2, ..., 937}.
The PWM brightness range is from 10 to 937. Let's define it as B = {10, 11,
12, ..., 937}.
|A| = 938, |B| = 928
|A| != |B|
It implies A and B is not an 1-1 mapping.

My patch is not a arbitrarily change.
It makes A' = {0, 1, 2, ..., 927}. |A'| = 928
You can see |A'| = |B| so it implies A' and B is an 1-1 mapping.
It means any value in A' can be mapped to a different value in B and visa
versa.

C = {0, 100} has the same mapping problem.

>
> Besides, we've changed the max for some platforms before, and the ABI
> for backlight sysfs is you can stick a value between 0 and
> max_brightness to the brightness attribute. Any userspace that relies on
> anything else is broken.
>
> > However I saw some message as the following.
> > [3.402233] [drm:parse_lfp_backlight] VBT backlight PWM modulation
> > frequency 200 Hz, active high, min brightness 10, level 255
> >
> >
> > Does it mean the brightness range is also defined in the BIOS?
>
> The minimum and the PWM modulation frequency are defined in BIOS. The
> modulation frequency is an attribute for the hardware; I think that was
> originally exposed as the max was just for implementation convenience.
>
I don't mean the minimum or the PWM modulation frequency.
I mean "level 255".
Does it mean the brightness range or something else?

Regards,
$4

>
> BR,
> Jani.
>
>
> >
> > Regards,
> > $4
> >
> >>
> >> >
> >> > Signed-off-by: Shih-Yuan Lee (FourDollars) 
> >> > ---
> >> >  drivers/gpu/drm/i915/intel_panel.c | 2 +-
> >> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/i915/intel_panel.c
> >> b/drivers/gpu/drm/i915/intel_panel.c
> >> > index a24df35..697fd4d 100644
> >> > --- a/drivers/gpu/drm/i915/intel_panel.c
> >> > +++ b/drivers/gpu/drm/i915/intel_panel.c
> >> > @@ -1211,7 +1211,7 @@ static int
> intel_backlight_device_register(struct
> >> intel_connector *connector)
> >> >* Note: Everything should work even if the backlight device max
> >> >* presented to the userspace is arbitrarily chosen.
> >> >*/
> >> > - props.max_brightness = panel->backlight.max;
> >> > + props.max_brightness = panel->backlight.max -
> panel->backlight.min;
> >> >   props.brightness = scale_hw_to_user(connector,
> >> >   panel->backlight.level,
> >> >   props.max_brightness);
> >>
> >> --
> >> Jani Nikula, Intel Open Source Technology Center
> >>
>
> --
> Jani Nikula, Intel Open Source Technology Center
>



-- 
Shih-Yuan Lee (FourDollars) | Software Engineer | Commercial Engineering -
PC & Core Taipei | Ubuntu Engineering and Services | Canonical
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] HDMI hotplug fails when using VT or boot on text mode

[Sanchez, AdolfoX]

When using i915
O.S. Ubuntu 15.10
Pltaform: Bay Trail I

Issue: When the O.S. is on Virtual Terminal or Text Mode it doesn't detect when 
connecting HDMI display, the issue is not present on graphic mode.

Is this expected behavior?

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