Re: [Intel-gfx] [PATCH] drm/i915/GLK: Properly handle plane CSC for BT2020 framebuffers

[Lankhorst, Maarten]

tor 2019-05-02 klockan 14:51 +0300 skrev Ville Syrjälä:
> On Thu, May 02, 2019 at 10:40:39AM +, Sharma, Shashank wrote:
> > 
> > 
> > > -Original Message-
> > > From: Ville Syrjälä [mailto:ville.syrj...@linux.intel.com]
> > > Sent: Thursday, May 2, 2019 3:45 PM
> > > To: Sharma, Shashank 
> > > Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville  > > a...@intel.com>; Lankhorst,
> > > Maarten 
> > > Subject: Re: [Intel-gfx] [PATCH] drm/i915/GLK: Properly handle
> > > plane CSC for
> > > BT2020 framebuffers
> > > 
> > > On Thu, May 02, 2019 at 03:19:42PM +0530, Shashank Sharma wrote:
> > > > Framebuffer formats P01x are supported by GLK, but the function
> > > > which
> > > > handles CSC on plane color control register, still expectes the
> > > > input
> > > > buffer to be REC709. This can cause inaccurate output for
> > > > direct P01x
> > > > flips.
> > > > 
> > > > This patch checks if the color_encoding property is set to
> > > > YCBCR_2020,
> > > > and enables the corresponding color conversion mode on plane
> > > > CSC.
> > > > 
> > > > PS: renamed variable plane_color_ctl to color_ctl for 80 char
> > > > stuff.
> > > > 
> > > > Cc: Ville Syrjala 
> > > > Cc: Maarten Lankhorst 
> > > > Cc: Uma Shankar 
> > > > Signed-off-by: Shashank Sharma 
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c | 26 --
> > > > 
> > > >  1 file changed, 16 insertions(+), 10 deletions(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index dd65d7c521c1..2d4d3128bf1f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -3868,24 +3868,30 @@ u32 glk_plane_color_ctl(const struct
> > > > intel_crtc_state
> > > 
> > > *crtc_state,
> > > > to_i915(plane_state->base.plane->dev);
> > > > const struct drm_framebuffer *fb = plane_state-
> > > > >base.fb;
> > > > struct intel_plane *plane =
> > > > to_intel_plane(plane_state->base.plane);
> > > > -   u32 plane_color_ctl = 0;
> > > > +   u32 color_ctl = 0;
> > > > 
> > > > -   plane_color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;
> > > > -   plane_color_ctl |=
> > > > glk_plane_color_ctl_alpha(plane_state);
> > > > +   color_ctl |= PLANE_COLOR_PLANE_GAMMA_DISABLE;
> > > > +   color_ctl |= glk_plane_color_ctl_alpha(plane_state);
> > > > 
> > > > if (fb->format->is_yuv && !icl_is_hdr_plane(dev_priv,
> > > > plane->id)) {
> > > > -   if (plane_state->base.color_encoding ==
> > > > DRM_COLOR_YCBCR_BT709)
> > > > -   plane_color_ctl |=
> > > 
> > > PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> > > > -   else
> > > > -   plane_color_ctl |=
> > > 
> > > PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> > > > +   switch (plane_state->base.color_encoding) {
> > > > +   case DRM_COLOR_YCBCR_BT709:
> > > > +   color_ctl |=
> > > 
> > > PLANE_COLOR_CSC_MODE_YUV709_TO_RGB709;
> > > > +   break;
> > > > +   case DRM_COLOR_YCBCR_BT2020:
> > > > +   color_ctl |=
> > > 
> > > PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020;
> > > > +   break;
> > > > +   default:
> > > > +   color_ctl |=
> > > 
> > > PLANE_COLOR_CSC_MODE_YUV601_TO_RGB709;
> > > > +   }
> > > 
> > > This isn't going to do anything without adjusting the property
> > > supported encodings as
> > > well.
> > > 
> > 
> > I might have not understood this comment properly, but, AFAIK, if
> > userspace sets this property well, and we set this color_ctl bit
> > properly, driver is setting PLANE_COLOR_CSC_MODE_YUV2020_TO_RGB2020
> > bits in GLK color control register. As GLK has a fix function plane
> > CSC, HW will apply a different matrix internally to convert input
> > buffer to RGB_2020 from YCBCR_2020 (earlier this would have been
> > YCBCR_709).  So I think we should see visible changes in output. Do
> > you think otherwise ? 
> 
> The property won't accept the BT2020 value. Or if it does we have a
> bug
> somewhere.
> 
> I guess tests would be nice. Maybe we should extend the kms_plane
> pixel
> format tests to check different YCbCr encodings as well? Or maybe
> Maarten's kms_yuv already tests this?
Not yet, unfortunately we have no way to set CSC in igt yet. :(

Best way to do so would be to add a igt_create_fb_yuv() which would a
igt_create_fb that accepts igt color encoding and range as arguments.

~Maarten

smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [v3 6/7] drm: Add Client Cap for advance gamma mode

[Lankhorst, Maarten]

mån 2019-04-15 klockan 15:43 +0300 skrev Ville Syrjälä:
> On Mon, Apr 15, 2019 at 10:57:52AM +0000, Lankhorst, Maarten wrote:
> > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar:
> > > Introduced a client cap for advance cap mode
> > > capability. Userspace should set this to get
> > > to be able to use the new gamma_mode property.
> > > 
> > > If this is not set, driver will work in legacy
> > > mode.
> > > 
> > > Suggested-by: Ville Syrjälä 
> > > Signed-off-by: Uma Shankar 
> > 
> > Nack, this doesn't seem like a sensible idea. We already guard it
> > behind the gamma mode property. Userspace shouldn't set the gamma
> > mode
> > to a value it doesn't understand.
> 
> Old userspace wouldn't know what a gamma_mode prop is. While a client
> cap isn't an entirely satisfactory solution I can't think of a better
> way at the moment.
> 
> Well, maybe the old "hey kernel, please reset all my props to some
> sane default" idea could be another way to deal with this sort of
> stuff.
Yes, but this approach wouldn't work, lot of other properties that can
cause problems when not reset, like plane alpha and blend mode. I don't
see why gamma is special in that case.

If userspace did set it to some special value, then presumably it knows
how to reset it too. It would be different if the split gamma mode was
the default. Then I would understand this, but right now this would
even break s/r.

~Maarten

smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [v3 6/7] drm: Add Client Cap for advance gamma mode

[Lankhorst, Maarten]

mån 2019-04-15 klockan 19:26 +0530 skrev Sharma, Shashank:
> > -Original Message-
> > From: Lankhorst, Maarten
> > Sent: Monday, April 15, 2019 4:28 PM
> > To: Shankar, Uma ; intel-gfx@lists.freedeskt
> > op.org; dri-
> > de...@lists.freedesktop.org
> > Cc: Syrjala, Ville ; emil.l.velikov@gmail.
> > com;
> > s...@ravnborg.org; Roper, Matthew D ;
> > seanp...@chromium.org; brian.star...@arm.com; dcasta...@chromium.or
> > g;
> > Sharma, Shashank 
> > Subject: Re: [v3 6/7] drm: Add Client Cap for advance gamma mode
> > 
> > fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar:
> > > Introduced a client cap for advance cap mode
> > > capability. Userspace should set this to get
> > > to be able to use the new gamma_mode property.
> > > 
> > > If this is not set, driver will work in legacy
> > > mode.
> > > 
> > > Suggested-by: Ville Syrjälä 
> > > Signed-off-by: Uma Shankar 
> > 
> > Nack, this doesn't seem like a sensible idea. We already guard it
> > behind the gamma mode property. Userspace shouldn't set the gamma
> > mode
> > to a value it doesn't understand.
> > 
> > ~Maarten
> 
> Hey Maarten, 
> In that case, what do you suggest should be the right way to do this
> ? 
> 
> @Ville, any comments here ? 
> 
I would say drop this patch, and just enable segmented gamma
unconditionally, it's not the first property that can cause trouble
when not understood.

~Maarten

smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [v3 6/7] drm: Add Client Cap for advance gamma mode

[Lankhorst, Maarten]

fre 2019-04-12 klockan 15:51 +0530 skrev Uma Shankar:
> Introduced a client cap for advance cap mode
> capability. Userspace should set this to get
> to be able to use the new gamma_mode property.
> 
> If this is not set, driver will work in legacy
> mode.
> 
> Suggested-by: Ville Syrjälä 
> Signed-off-by: Uma Shankar 

Nack, this doesn't seem like a sensible idea. We already guard it
behind the gamma mode property. Userspace shouldn't set the gamma mode
to a value it doesn't understand.

~Maarten

>  drivers/gpu/drm/drm_atomic_uapi.c | 3 +++
>  drivers/gpu/drm/drm_ioctl.c   | 5 +
>  include/drm/drm_atomic.h  | 1 +
>  include/drm/drm_crtc.h| 7 +++
>  include/drm/drm_file.h| 8 
>  include/uapi/drm/drm.h| 2 ++
>  6 files changed, 26 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index d85e0c9..590c87a 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -974,6 +974,8 @@ int drm_atomic_set_property(struct
> drm_atomic_state *state,
>   break;
>   }
>  
> + crtc_state->advance_gamma_mode_active =
> + state-
> >advance_gamma_mode_active;
>   ret = drm_atomic_crtc_set_property(crtc,
>   crtc_state, prop, prop_value);
>   break;
> @@ -1297,6 +1299,7 @@ int drm_mode_atomic_ioctl(struct drm_device
> *dev,
>   drm_modeset_acquire_init(,
> DRM_MODESET_ACQUIRE_INTERRUPTIBLE);
>   state->acquire_ctx = 
>   state->allow_modeset = !!(arg->flags &
> DRM_MODE_ATOMIC_ALLOW_MODESET);
> + state->advance_gamma_mode_active = file_priv-
> >advance_gamma_mode_active;
>  
>  retry:
>   copied_objs = 0;
> diff --git a/drivers/gpu/drm/drm_ioctl.c
> b/drivers/gpu/drm/drm_ioctl.c
> index d337f16..e593a4c 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -348,6 +348,11 @@ static int drm_getcap(struct drm_device *dev,
> void *data, struct drm_file *file_
>   return -EINVAL;
>   file_priv->writeback_connectors = req->value;
>   break;
> + case DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES:
> + if (req->value > 1)
> + return -EINVAL;
> + file_priv->advance_gamma_mode_active = req->value;
> + break;
>   default:
>   return -EINVAL;
>   }
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 824a5ed..02c1a68 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -338,6 +338,7 @@ struct drm_atomic_state {
>* states.
>*/
>   bool duplicated : 1;
> + bool advance_gamma_mode_active : 1;
>   struct __drm_planes_state *planes;
>   struct __drm_crtcs_state *crtcs;
>   int num_connector;
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index f789359..f11dc25 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -170,6 +170,11 @@ struct drm_crtc_state {
>   bool color_mgmt_changed : 1;
>  
>   /**
> +  * This is to indicate advance gamma mode support
> +  */
> + bool advance_gamma_mode_active : 1;
> +
> + /**
>* @no_vblank:
>*
>* Reflects the ability of a CRTC to send VBLANK events.
> This state
> @@ -952,6 +957,8 @@ struct drm_crtc {
>*/
>   bool enabled;
>  
> + bool advance_gamma_mode_active : 1;
> +
>   /**
>* @mode:
>*
> diff --git a/include/drm/drm_file.h b/include/drm/drm_file.h
> index 6710b61..b5aa24e 100644
> --- a/include/drm/drm_file.h
> +++ b/include/drm/drm_file.h
> @@ -201,6 +201,14 @@ struct drm_file {
>   bool writeback_connectors;
>  
>   /**
> +  * This is to enable advance gamma modes using
> +  * gamma_mode property
> +  *
> +  * True if client understands advance gamma
> +  */
> + bool advance_gamma_mode_active : 1;
> +
> + /**
>* @is_master:
>*
>* This client is the creator of @master. Protected by
> struct
> diff --git a/include/uapi/drm/drm.h b/include/uapi/drm/drm.h
> index 236b01a..abef966 100644
> --- a/include/uapi/drm/drm.h
> +++ b/include/uapi/drm/drm.h
> @@ -696,6 +696,8 @@ struct drm_get_cap {
>   */
>  #define DRM_CLIENT_CAP_WRITEBACK_CONNECTORS  5
>  
> +#define DRM_CLIENT_CAP_ADVANCE_GAMMA_MODES   6
> +
>  /** DRM_IOCTL_SET_CLIENT_CAP ioctl argument type */
>  struct drm_set_client_cap {
>   __u64 capability;

smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [v6 01/16] drm: Add Enhanced Gamma LUT precision structure

[Lankhorst, Maarten]

tis 2019-03-19 klockan 14:14 +0530 skrev Uma Shankar:
> Existing LUT precision structure is having only 16 bit
> precision. This is not enough for upcoming enhanced hardwares
> and advance usecases like HDR processing. Hence added a new
> structure with 32 bit precision values. Also added the code,
> for extracting the same from values passed from userspace.
> 
> v4: Rebase
> 
> v5: Relocated the helper function to drm_color_mgmt.c. Declared
> the same in a header file (Alexandru Gheorghe)
> 
> v6: Enhanced gamma lut structure to take U32.32 format as input.
> This is needed for HDR usecase which require higher precision.
> 
> Signed-off-by: Uma Shankar 
> Reviewed-by: Alexandru Gheorghe 
> ---
>  drivers/gpu/drm/drm_color_mgmt.c | 19 +++
>  include/drm/drm_color_mgmt.h |  1 +
>  include/uapi/drm/drm_mode.h  | 15 +++
>  3 files changed, 35 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c
> b/drivers/gpu/drm/drm_color_mgmt.c
> index d5d34d0..9dbfe1d 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -128,6 +128,25 @@ uint32_t drm_color_lut_extract(uint32_t
> user_input, uint32_t bit_precision)
>  }
>  EXPORT_SYMBOL(drm_color_lut_extract);
>  
> +/*
> + * Added to accommodate enhanced LUT precision.
> + * Max LUT precision is 32 bits.
> + */
> +u64 drm_color_lut_extract_ext(u64 user_input, u32 bit_precision)
> +{
> + u32 val = user_input & 0x;
> + u32 max = 0x >> (32 - bit_precision);
> +
> + /* Round only if we're not using full precision. */
> + if (bit_precision < 32) {
> + val += 1UL << (32 - bit_precision - 1);
> + val >>= 32 - bit_precision;
> + }
> +
> + return ((user_input & 0x) | clamp_val(val, 0, max));
I thought the userspace precision was U32.32, so the precision here is
max 64-bits?

Anyway the math looks wrong for a U32.32 value, it's probably:
user_input >> (64ULL - precision)

> +}
> +EXPORT_SYMBOL(drm_color_lut_extract_ext);
> +
>  /**
>   * drm_crtc_enable_color_mgmt - enable color management properties
>   * @crtc: DRM CRTC
> diff --git a/include/drm/drm_color_mgmt.h
> b/include/drm/drm_color_mgmt.h
> index d1c662d..c9d2746 100644
> --- a/include/drm/drm_color_mgmt.h
> +++ b/include/drm/drm_color_mgmt.h
> @@ -30,6 +30,7 @@
>  struct drm_plane;
>  
>  uint32_t drm_color_lut_extract(uint32_t user_input, uint32_t
> bit_precision);
> +u64 drm_color_lut_extract_ext(u64 user_input, u32 bit_precision);
>  
>  void drm_crtc_enable_color_mgmt(struct drm_crtc *crtc,
>   uint degamma_lut_size,
> diff --git a/include/uapi/drm/drm_mode.h
> b/include/uapi/drm/drm_mode.h
> index a439c2e..a0fae71 100644
> --- a/include/uapi/drm/drm_mode.h
> +++ b/include/uapi/drm/drm_mode.h
> @@ -630,6 +630,21 @@ struct drm_color_lut {
>   __u16 reserved;
>  };
>  
> +/*
> + * Creating 64 bit palette entries for better data
> + * precision. This will be required for HDR and
> + * similar color processing usecases.
> + */
> +struct drm_color_lut_ext {
> + /*
> +  * Data is U32.32 fixed point format.
> +  */
> + __u64 red;
> + __u64 green;
> + __u64 blue;
> + __u64 reserved;
> +};
> +
>  #define DRM_MODE_PAGE_FLIP_EVENT 0x01
>  #define DRM_MODE_PAGE_FLIP_ASYNC 0x02
>  #define DRM_MODE_PAGE_FLIP_TARGET_ABSOLUTE 0x4

smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC v1 3/7] drm/i915: Add Support for Multi Segment Gamma Mode

[Lankhorst, Maarten]

tis 2019-03-19 klockan 14:00 +0530 skrev Uma Shankar:
> Multi Segment Gamma Mode is added in Gen11+ platforms.
> Added a property interface to enable that.
> 
> Signed-off-by: Uma Shankar 
> ---
>  drivers/gpu/drm/i915/i915_drv.h|  1 +
>  drivers/gpu/drm/i915/intel_color.c | 23 +++
>  include/uapi/drm/i915_drm.h| 14 ++
>  3 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 02231ae..f20d418 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1736,6 +1736,7 @@ struct drm_i915_private {
>   struct drm_property *force_audio_property;
>  
>   struct drm_property *gamma_mode_property;
> + struct drm_property *multi_segment_gamma_mode_property;

Seems to me both properties should be part of drm core?

>   /* hda/i915 audio component */
>   struct i915_audio_component *audio_component;
> diff --git a/drivers/gpu/drm/i915/intel_color.c
> b/drivers/gpu/drm/i915/intel_color.c
> index 9d43d19..399d63d 100644
> --- a/drivers/gpu/drm/i915/intel_color.c
> +++ b/drivers/gpu/drm/i915/intel_color.c
> @@ -149,6 +149,26 @@ static bool crtc_state_is_legacy_gamma(const
> struct intel_crtc_state *crtc_state
>   drm_object_attach_property(>base.base, prop, 0);
>  }
>  
> +void
> +intel_attach_multi_segment_gamma_mode_property(struct intel_crtc
> *crtc)
> +{
> + struct drm_device *dev = crtc->base.dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct drm_property *prop;
> +
> + prop = dev_priv->multi_segment_gamma_mode_property;
> + if (!prop) {
> + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB,
> +"Multi-segment Gamma",
> 0);
> + if (!prop)
> + return;
> +
> + dev_priv->multi_segment_gamma_mode_property = prop;
> + }
> +
> + drm_object_attach_property(>base.base, prop, 0);
> +}
> +
>  /*
>   * When using limited range, multiply the matrix given by userspace
> by
>   * the matrix that we would use for the limited range.
> @@ -953,4 +973,7 @@ void intel_color_init(struct intel_crtc *crtc)
>  INTEL_INFO(dev_priv)-
> >color.gamma_lut_size);
>  
>   intel_attach_gamma_mode_property(crtc);
> +
> + if (INTEL_GEN(dev_priv) >= 11)
> + intel_attach_multi_segment_gamma_mode_property(crtc)
> ;
>  }
> diff --git a/include/uapi/drm/i915_drm.h
> b/include/uapi/drm/i915_drm.h
> index aa2d4c7..8f1974e 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -1842,6 +1842,20 @@ struct drm_i915_query_topology_info {
>   __u8 data[];
>  };
>  
> +/*
> + * Structure for muti segmented gamma lut
> + */
> +struct multi_segment_gamma_lut {
> + /* Number of Lut Segments */
> + __u8 segment_cnt;
> + /* Precison of LUT entries in bits */
> + __u8 precision_bits;
> + /* Pointer having number of LUT elements in each segment */
> + __u32 *segment_lut_cnt_ptr;
> + /* Pointer to store exact lut values for each segment */
> + __u32 *segment_lut_ptr;
> +};
> 
And perhaps a variation of this as description for all gamma mode
types.

smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC v5 0/8] Add Plane Color Properties

[Lankhorst, Maarten]

Hey,

sön 2018-09-16 klockan 13:45 +0530 skrev Uma Shankar:
> This is how a typical display color hardware pipeline looks like:
>  +---+
>  |RAM|
>  |  +--++-++-+   |
>  |  | FB 1 ||  FB 2   || FB N|   |
>  |  +--++-++-+   |
>  +---+
>|  Plane Color Hardware Block |
>  ++

Should be some mention of color conversion (YUV -> RGB) through
drm_color_encoding and drm_color_range enums interacting here?

>  | +---v-+   +---v---+   +---v--+ |
>  | | Plane A |   | Plane B   |   | Plane N  | |
>  | | DeGamma |   | Degamma   |   | Degamma  | |
>  | +---+-+   +---+---+   +---+--+ |
>  | | |   ||
>  | +---v-+   +---v---+   +---v--+ |
>  | |Plane A  |   | Plane B   |   | Plane N  | |
>  | |CSC/CTM  |   | CSC/CTM   |   | CSC/CTM  | |
>  | +---+-+   ++--+   ++-+ |
>  | |  |   |   |
>  | +---v-+   +v--+   +v-+ |
>  | | Plane A |   | Plane B   |   | Plane N  | |
>  | | Gamma   |   | Gamma |   | Gamma| |
>  | +---+-+   ++--+   ++-+ |
>  | |  |   |   |
>  ++
> +--v--v---v---|
> > >   ||
> > >   Pipe Blender||
> 
> +++
> >||
> >+---v--+ |
> >|  Pipe DeGamma| |
> >|  | |
> >+---+--+ |
> >|Pipe Color  |
> >+---v--+ Hardware|
> >|  Pipe CSC/CTM| |
> >|  | |
> >+---+--+ |
> >||
> >+---v--+ |
> >|  Pipe Gamma  | |
> >|  | |
> >+---+--+ |
> >||
Likewise, should probably be some mention about setting colorspace on
the connector, so the output will knows what format is used?

Perhaps pull it to this series since one can't work really well without
the other?
> 
> +-+
>  |
>  v
>Pipe Output
> 
> This patch series adds properties for plane color features. It adds
> properties for degamma used to linearize data, CSC used for gamut
> conversion, and gamma used to again non-linearize data as per panel
> supported color space. These can be utilize by user space to convert
> planes from one format to another, one color space to another etc.
> 
> Usersapce can take smart blending decisions and utilize these
> hardware
> supported plane color features to get accurate color profile. The
> same
> can help in consistent color quality from source to panel taking
> advantage of advanced color features in hardware.
> 
> These patches just add the property interfaces and enable helper
> functions.
> 
> This series adds Intel Gen9 specific plane gamma feature. We can
> build up and add other platform/hardware specific implementation
> on top of this series
> 
> Note: This is just to get a design feedback whether these interfaces
> look ok. Based on community feedback on interfaces, we will implement
> IGT tests to validate plane color features. This is un-tested
> currently.
> 
> Userspace implementation using these properties have been done in drm
> hwcomposer by "Alexandru-Cosmin Gheorghe Alexandru-Cosmin.Gheorghe@ar
> m.com"
> from ARM. A merge request has been opened by Alexandru for
> drm_hwcomposer,
> implementing the property changes for the same. Please review that as
> well:
> https://gitlab.freedesktop.org/drm-hwcomposer/drm-hwcomposer/merge_re
> quests/25
> 
> v2: Dropped legacy gamma table for plane as suggested by Maarten.
> Added
> Gen9/BDW plane gamma feature and rebase on tot.
> 
> v3: Added a new drm_color_lut_ext structure to accommodate 32 bit
> precision
> entries, pointed to by Brian, Starkey for HDR usecases. Addressed
> Sean,Paul
> comments and moved plane color properties to drm_plane instead of
> mode_config. Added property documentation as suggested by Daniel,
> Vetter.
> Fixed a rebase fumble which occurred in v2, pointed by Emil Velikov.
> 
> v4: Rebase
> 
> v5: Added "Display Color Hardware Pipeline" flow to kernel
> documentation as suggested by "Ville Syrjala" and "Brian Starkey".
> Moved the property creation to drm_color_mgmt.c file to 

Re: [Intel-gfx] [RFC v4 3/8] drm: Add Plane CTM property

[Lankhorst, Maarten]

ons 2018-08-22 klockan 12:11 +0100 skrev Brian Starkey:
> Hi,
> 
> On Wed, Aug 22, 2018 at 12:53:58PM +0300, Ville Syrjälä wrote:
> > On Wed, Aug 22, 2018 at 08:40:19AM +0000, Lankhorst, Maarten wrote:
> > > fre 2018-08-17 klockan 19:48 +0530 skrev Uma Shankar:
> > > > Add a blob property for plane CSC usage.
> > > > 
> > > > v2: Rebase
> > > > 
> > > > v3: Fixed Sean, Paul's review comments. Moved the property from
> > > > mode_config to drm_plane. Created a helper function to
> > > > instantiate
> > > > these properties and removed from
> > > > drm_mode_create_standard_properties
> > > > Added property documentation as suggested by Daniel, Vetter.
> > > > 
> > > > v4: Rebase
> > > > 
> > > > Signed-off-by: Uma Shankar 
> > > > ---
> > > >  Documentation/gpu/drm-kms.rst   |  3 +++
> > > >  drivers/gpu/drm/drm_atomic.c| 10 ++
> > > >  drivers/gpu/drm/drm_atomic_helper.c |  4 
> > > >  drivers/gpu/drm/drm_plane.c | 12 
> > > >  include/drm/drm_plane.h | 15 +++
> > > >  5 files changed, 44 insertions(+)
> > > > 
> > > > diff --git a/Documentation/gpu/drm-kms.rst
> > > > b/Documentation/gpu/drm-
> > > > kms.rst
> > > > index 8b10b12..16d6d8d 100644
> > > > --- a/Documentation/gpu/drm-kms.rst
> > > > +++ b/Documentation/gpu/drm-kms.rst
> > > > @@ -560,6 +560,9 @@ Plane Color Management Properties
> > > >  .. kernel-doc:: drivers/gpu/drm/drm_plane.c
> > > > :doc: degamma_lut_size_property
> > > > 
> > > > +.. kernel-doc:: drivers/gpu/drm/drm_plane.c
> > > > +   :doc: ctm_property
> > > > +
> > > >  Tile Group Property
> > > >  ---
> > > > 
> > > > diff --git a/drivers/gpu/drm/drm_atomic.c
> > > > b/drivers/gpu/drm/drm_atomic.c
> > > > index f8cad9b..ddda935 100644
> > > > --- a/drivers/gpu/drm/drm_atomic.c
> > > > +++ b/drivers/gpu/drm/drm_atomic.c
> > > > @@ -917,6 +917,14 @@ static int
> > > > drm_atomic_plane_set_property(struct
> > > > drm_plane *plane,
> > > > );
> > > > state->color_mgmt_changed |= replaced;
> > > > return ret;
> > > > +   } else if (property == plane->ctm_property) {
> > > > +   ret =
> > > > drm_atomic_replace_property_blob_from_id(dev,
> > > > +   >ctm,
> > > > +   val,
> > > > +   sizeof(struct
> > > > drm_color_ctm), -1,
> > > > +   );
> > > > +   state->color_mgmt_changed |= replaced;
> > > > +   return ret;
> > > > } else if (plane->funcs->atomic_set_property) {
> > > > return plane->funcs-
> > > > >atomic_set_property(plane,
> > > > state,
> > > > property, val);
> > > > @@ -988,6 +996,8 @@ static int
> > > > drm_atomic_plane_set_property(struct
> > > > drm_plane *plane,
> > > > } else if (property == plane->degamma_lut_property) {
> > > > *val = (state->degamma_lut) ?
> > > > state->degamma_lut->base.id : 0;
> > > > +   } else if (property == plane->ctm_property) {
> > > > +   *val = (state->ctm) ? state->ctm->base.id : 0;
> > > > } else if (plane->funcs->atomic_get_property) {
> > > > return plane->funcs-
> > > > >atomic_get_property(plane,
> > > > state, property, val);
> > > > } else {
> > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > > index 67c5b51..866181f 100644
> > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > @@ -3616,6 +3616,9 @@ void
> > > > __drm_atomic_helper_plane_duplicate_state(struct drm_plane
> > > > *plane,
> > > > 
> > > > if (state->degamma

Re: [Intel-gfx] [RFC v4 3/8] drm: Add Plane CTM property

[Lankhorst, Maarten]

fre 2018-08-17 klockan 19:48 +0530 skrev Uma Shankar:
> Add a blob property for plane CSC usage.
> 
> v2: Rebase
> 
> v3: Fixed Sean, Paul's review comments. Moved the property from
> mode_config to drm_plane. Created a helper function to instantiate
> these properties and removed from drm_mode_create_standard_properties
> Added property documentation as suggested by Daniel, Vetter.
> 
> v4: Rebase
> 
> Signed-off-by: Uma Shankar 
> ---
>  Documentation/gpu/drm-kms.rst   |  3 +++
>  drivers/gpu/drm/drm_atomic.c| 10 ++
>  drivers/gpu/drm/drm_atomic_helper.c |  4 
>  drivers/gpu/drm/drm_plane.c | 12 
>  include/drm/drm_plane.h | 15 +++
>  5 files changed, 44 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-kms.rst b/Documentation/gpu/drm-
> kms.rst
> index 8b10b12..16d6d8d 100644
> --- a/Documentation/gpu/drm-kms.rst
> +++ b/Documentation/gpu/drm-kms.rst
> @@ -560,6 +560,9 @@ Plane Color Management Properties
>  .. kernel-doc:: drivers/gpu/drm/drm_plane.c
> :doc: degamma_lut_size_property
>  
> +.. kernel-doc:: drivers/gpu/drm/drm_plane.c
> +   :doc: ctm_property
> +
>  Tile Group Property
>  ---
>  
> diff --git a/drivers/gpu/drm/drm_atomic.c
> b/drivers/gpu/drm/drm_atomic.c
> index f8cad9b..ddda935 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -917,6 +917,14 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
>   );
>   state->color_mgmt_changed |= replaced;
>   return ret;
> + } else if (property == plane->ctm_property) {
> + ret = drm_atomic_replace_property_blob_from_id(dev,
> + >ctm,
> + val,
> + sizeof(struct
> drm_color_ctm), -1,
> + );
> + state->color_mgmt_changed |= replaced;
> + return ret;
>   } else if (plane->funcs->atomic_set_property) {
>   return plane->funcs->atomic_set_property(plane,
> state,
>   property, val);
> @@ -988,6 +996,8 @@ static int drm_atomic_plane_set_property(struct
> drm_plane *plane,
>   } else if (property == plane->degamma_lut_property) {
>   *val = (state->degamma_lut) ?
>   state->degamma_lut->base.id : 0;
> + } else if (property == plane->ctm_property) {
> + *val = (state->ctm) ? state->ctm->base.id : 0;
>   } else if (plane->funcs->atomic_get_property) {
>   return plane->funcs->atomic_get_property(plane,
> state, property, val);
>   } else {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 67c5b51..866181f 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3616,6 +3616,9 @@ void
> __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane,
>  
>   if (state->degamma_lut)
>   drm_property_blob_get(state->degamma_lut);
> + if (state->ctm)
> + drm_property_blob_get(state->ctm);
> +
>   state->color_mgmt_changed = false;
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state);
> @@ -3663,6 +3666,7 @@ void
> __drm_atomic_helper_plane_destroy_state(struct drm_plane_state
> *state)
>   drm_crtc_commit_put(state->commit);
>  
>   drm_property_blob_put(state->degamma_lut);
> + drm_property_blob_put(state->ctm);
>  }
>  EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state);
>  
> diff --git a/drivers/gpu/drm/drm_plane.c
> b/drivers/gpu/drm/drm_plane.c
> index 03e0560..97520b1 100644
> --- a/drivers/gpu/drm/drm_plane.c
> +++ b/drivers/gpu/drm/drm_plane.c
> @@ -489,6 +489,11 @@ int drm_mode_plane_set_obj_prop(struct drm_plane
> *plane,
>   *
>   * degamma_lut_size_property:
>   *   Range Property to indicate size of the plane degamma LUT.
> + *
> + * ctm_property:
> + *   Blob property which allows a userspace to provide CTM
> coefficients
> + *   to do color space conversion or any other enhancement by
> doing a
> + *   matrix multiplication using the h/w CTM processing engine
>   */
>  int drm_plane_color_create_prop(struct drm_device *dev,
>   struct drm_plane *plane)
> @@ -509,6 +514,13 @@ int drm_plane_color_create_prop(struct
> drm_device *dev,
>   return -ENOMEM;
>   plane->degamma_lut_size_property = prop;
>  
> + prop = drm_property_create(dev,
> + DRM_MODE_PROP_BLOB,
> + "PLANE_CTM", 0);
> + if (!prop)
> + return -ENOMEM;
> + plane->ctm_property = prop;
> +
>   return 0;
>  }
>  EXPORT_SYMBOL(drm_plane_color_create_prop);
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 28357ac..5143277 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> 

Re: [Intel-gfx] [v2] drm/i915: Enable hw workaround to bypass alpha

[Lankhorst, Maarten]

tor 2018-06-21 klockan 14:09 -0700 skrev Radhakrishna Sripada:
> On Thu, Jun 21, 2018 at 08:43:56PM +0530, Vandita Kulkarni wrote:
> > Alpha blending with alpha 0 and 0xff passes through
> > alpha math and rounding logic causing differences
> > compared to fully transparent or opaque plane,resulting
> > in CRC mismatch.
> > This WA on icl and above enables hardware to bypass alpha
> > math and rounding for per pixel alpha values of 00 and 0xff
> > 
> > v2: Fix patchwork checkpatch warnings.
> > 
> > Signed-off-by: Vandita Kulkarni 
> > ---
> >  drivers/gpu/drm/i915/i915_reg.h  |  8 
> >  drivers/gpu/drm/i915/intel_display.c | 12 
> >  2 files changed, 20 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h
> > b/drivers/gpu/drm/i915/i915_reg.h
> > index 4bfd7a9..b66ec9b 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -7366,6 +7366,14 @@ enum {
> >  #define BDW_SCRATCH1   _MMIO(
> > 0xb11c)
> >  #define  GEN9_LBS_SLA_RETRY_TIMER_DECREMENT_ENABLE (1 << 2)
> >  
> > +/*GEN11 chicken */
> > +#define _PIPEA_CHICKEN 0x70038
> > +#define _PIPEB_CHICKEN 0x71038
> > +#define _PIPEC_CHICKEN 0x72038
> > +#define  PER_PIXEL_ALPHA_BYPASS_EN (1 << 7)
> > +#define PIPE_CHICKEN(pipe) _MMIO_PIPE(pipe,
> > _PIPEA_CHICKEN,\
> > +  _PIPEB_CHICKEN)
> > +
> >  /* PCH */
> >  
> >  /* south display engine interrupt: IBX */
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > index 2c8fef3..3d849ec 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -5632,6 +5632,7 @@ static void haswell_crtc_enable(struct
> > intel_crtc_state *pipe_config,
> > struct intel_atomic_state *old_intel_state =
> > to_intel_atomic_state(old_state);
> > bool psl_clkgate_wa;
> > +   u32 pipe_chicken;
> >  
> > if (WARN_ON(intel_crtc->active))
> > return;
> > @@ -5691,6 +5692,17 @@ static void haswell_crtc_enable(struct
> > intel_crtc_state *pipe_config,
> >  */
> > intel_color_load_luts(_config->base);
> >  
> > +   /*
> > +* Display WA #1153: enable hardware to bypass the alpha
> > math
> > +* and rounding for per-pixel values 00 and 0xff
> > +*/
> > +   if (INTEL_GEN(dev_priv) >= 11) {
> > +   pipe_chicken = I915_READ(PIPE_CHICKEN(pipe));
> > +   if (!(pipe_chicken & PER_PIXEL_ALPHA_BYPASS_EN))
> > +   I915_WRITE_FW(PIPE_CHICKEN(pipe),
> > + pipe_chicken |
> > PER_PIXEL_ALPHA_BYPASS_EN);
> > +   }
> 
> This would enable the wa by default for gen > 11. Would this impact
> for non 00, 0xff alpha cases?
> In other words, should we enable the wa only when alpha is 00/0xff
> and not enable for other values?
> 
This is about per pixel values, unlike the plane alpha. This looks at
the pixel contents, so if a pixel has (0xff, R, G, B), we pass through
RGB unmodified. If it's (0x00, R, G, B), we do the same for the
background pixel without introducing rounding errors.

When we program plane alpha, we handle 0x00 by disabling the plane, and
0xff by disabling plane alpha and making the plane opaque.

Thanks for the patch, pushed. :)

~Maarten

smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Enable hw workaround to bypass alpha

[Lankhorst, Maarten]

tor 2018-06-21 klockan 19:00 +0530 skrev Vandita Kulkarni:
> Alpha blending with alpha 0 and 0xff passes through
> alpha math and rounding logic causing differences
> compared to fully transparent or opaque plane,resulting
> in CRC mismatch.
> This WA on icl and above enables hardware to bypass alpha
> math and rounding for per pixel alpha values of 00 and 0xff
> 
> Signed-off-by: Vandita Kulkarni 
> ---
Thanks, pushed with my r-b. :)
>  drivers/gpu/drm/i915/i915_reg.h  |  8 
>  drivers/gpu/drm/i915/intel_display.c | 12 
>  2 files changed, 20 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 4bfd7a9..6e59bfe 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -7366,6 +7366,14 @@ enum {
>  #define BDW_SCRATCH1 _MMIO(0x
> b11c)
>  #define  GEN9_LBS_SLA_RETRY_TIMER_DECREMENT_ENABLE   (1 << 2)
>  
> +/*GEN11 chicken */
> +#define _PIPEA_CHICKEN   0x70038
> +#define _PIPEB_CHICKEN   0x71038
> +#define _PIPEC_CHICKEN   0x72038
> +#define  PER_PIXEL_ALPHA_BYPASS_EN   (1<<7)
> +#define PIPE_CHICKEN(pipe)   _MMIO_PIPE(pipe,
> _PIPEA_CHICKEN,\
> +_PIPEB_CHICKEN)
> +
>  /* PCH */
>  
>  /* south display engine interrupt: IBX */
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 2c8fef3..ca5882c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5632,6 +5632,7 @@ static void haswell_crtc_enable(struct
> intel_crtc_state *pipe_config,
>   struct intel_atomic_state *old_intel_state =
>   to_intel_atomic_state(old_state);
>   bool psl_clkgate_wa;
> + u32 pipe_chicken;
>  
>   if (WARN_ON(intel_crtc->active))
>   return;
> @@ -5691,6 +5692,17 @@ static void haswell_crtc_enable(struct
> intel_crtc_state *pipe_config,
>*/
>   intel_color_load_luts(_config->base);
>  
> + /*
> +  * Display WA #1153: enable hardware to bypass the alpha
> math
> +  * and rounding for per-pixel values 00 and 0xff
> +  */
> + if (INTEL_GEN(dev_priv) >= 11) {
> + pipe_chicken = I915_READ(PIPE_CHICKEN(pipe));
> + if (!(pipe_chicken & PER_PIXEL_ALPHA_BYPASS_EN))
> + I915_WRITE_FW(PIPE_CHICKEN(pipe),
> +   pipe_chicken |
> PER_PIXEL_ALPHA_BYPASS_EN);
> + }
> +
>   intel_ddi_set_pipe_settings(pipe_config);
>   if (!transcoder_is_dsi(cpu_transcoder))
>   intel_ddi_enable_transcoder_func(pipe_config);

smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC v1 5/6] drm: Define helper to set legacy gamma table size

[Lankhorst, Maarten]

Shankar, Uma schreef op di 26-09-2017 om 15:41 [+0530]:
> > -Original Message-
> > From: dri-devel [mailto:dri-devel-boun...@lists.freedesktop.org] On
> > Behalf Of
> > Lankhorst, Maarten
> > Sent: Tuesday, September 26, 2017 3:36 PM
> > To: Shankar, Uma <uma.shan...@intel.com>; intel-gfx@lists.freedeskt
> > op.org;
> > dri-de...@lists.freedesktop.org
> > Cc: Syrjala, Ville <ville.syrj...@intel.com>
> > Subject: Re: [RFC v1 5/6] drm: Define helper to set legacy gamma
> > table size
> > 
> > Hey,
> > 
> > Uma Shankar schreef op di 26-09-2017 om 13:32 [+0530]:
> > > Define a helper function to set legacy gamma table size for
> > > planes.
> > > 
> > > Signed-off-by: Uma Shankar <uma.shan...@intel.com>
> > > ---
> > >  drivers/gpu/drm/drm_color_mgmt.c |   41
> > > ++
> > >  include/drm/drm_color_mgmt.h |3 +++
> > >  include/drm/drm_plane.h  |4 
> > >  3 files changed, 48 insertions(+)
> > 
> > Is this needed? I'm not aware of legacy tables for planes.
> 
> I was not getting very concrete info on this. So kept it as per pipe
> gamma implementation.
> I will try to get some more info and drop this in case it's not
> required.
> 

It's not, legacy gamma would only be used in drm_mode_gamma_get_ioctl,
which if you look at it only works for a crtc. :)

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

Re: [Intel-gfx] [RFC v1 5/6] drm: Define helper to set legacy gamma table size

[Lankhorst, Maarten]

Hey,

Uma Shankar schreef op di 26-09-2017 om 13:32 [+0530]:
> Define a helper function to set legacy gamma table
> size for planes.
> 
> Signed-off-by: Uma Shankar 
> ---
>  drivers/gpu/drm/drm_color_mgmt.c |   41
> ++
>  include/drm/drm_color_mgmt.h |3 +++
>  include/drm/drm_plane.h  |4 
>  3 files changed, 48 insertions(+)

Is this needed? I'm not aware of legacy tables for planes.

Kind regards,
Maarten
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 0/2] Handle unsupported configuration with IF-ID

[Lankhorst, Maarten]

Mahesh Kumar schreef op di 18-07-2017 om 18:12 [+0530]:
> Hi Daniel,
> 
> On Monday 17 July 2017 12:56 PM, Daniel Vetter wrote:
> > On Fri, Jun 30, 2017 at 05:40:58PM +0530, Mahesh Kumar wrote:
> > > Gen9+ Interlace fetch mode doesn't support few plane
> > > configurations & pipe scaling.
> > >  - Y-tile
> > >  - 90/270 rotation
> > >  - pipe/plane scaling
> > >  - 420 planar formats
> > 
> > Do we have igt testcases that try to exercise all this? When fixing
> > bugs,
> > pls make sure you don't fix the bugs, but also make sure we have
> > solid
> > coverage. Given that this escaped for years, it's very likely our
> > coverage
> > is _really_ bad and needs to be improve a lot for testing
> > interlaced modes
> > ...
>  We have testdisplay with -y option to test Y-tiling (90/270 rotation
> need Y-tiling so those are also covered).
> But AFAIK we don't have any testcase to verify scaling with Interlace
> mode & other combinations.
> Should we extend existing IGT test for scaling/rotation/tiling/pixel-
> format to include Interlace mode as well, or should we have a
> separate Interlace mode specific IGT
> which will exercise all combinations with all Interlace modes?

We need separate tests. testdisplay is nice for testing, but it's not
automated. Ideally something that runs on all supported platforms,
regardless of having an actual interlaced display connected. We do
allow mode override with igt_output_override_mode, which could be used
for this.

> As IF-ID mode doesn't support all the above combination but PF-ID
> mode does support them from GEN9(scaling Y-tile 90/270 rotation etc).
> So I submitted a patch to always enable PF-ID mode for Interlace
> https://patchwork.freedesktop.org/patch/160275/
> But Ville suggested not to always enable PF-ID mode instead control
> fetching mode based on property.
> Currently there is no open source user for this property. What will
> you suggest here?
> 
> Thanks,
> -Mahesh
> > Thanks, Daniel
> > 
> > > Changes since V2:
> > >  - Address review comments from ville
> > > 
> > > Mahesh Kumar (2):
> > >   drm/i915/skl+: Check for supported plane configuration in
> > > Interlace
> > > mode
> > >   drm/i915/skl+: Scaling not supported in IF-ID Interlace mode
> > > 
> > >  drivers/gpu/drm/i915/intel_atomic_plane.c | 15 +++
> > >  drivers/gpu/drm/i915/intel_display.c  | 15 +++
> > >  2 files changed, 30 insertions(+)
> > > 
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 11/11] drm/i915/bxt: Enable IPC support

[Lankhorst, Maarten]

Mahesh Kumar schreef op wo 05-07-2017 om 20:01 [+0530]:
> From: "Kumar, Mahesh" 
> 
> This patch adds IPC support for platforms. This patch enables IPC
> only for BXT/KBL platform as for SKL recommendation is to keep it
> disabled.
CFL/CNL missing. ;-)
> IPC (Isochronous Priority Control) is the hardware feature, which
> dynamically controls the memory read priority of Display.
> 
> When IPC is enabled, plane read requests are sent at high priority
> until
> filling above the transition watermark, then the requests are sent at
> lower priority until dropping below the level 0 watermark.
> The lower priority requests allow other memory clients to have better
> memory access. When IPC is disabled, all plane read requests are sent
> at
> high priority.
> 
> Changes since V1:
>  - Remove commandline parameter to disable ipc
>  - Address Paulo's comments
> Changes since V2:
>  - Address review comments
>  - Set ipc_enabled flag
> Changes since V3:
>  - move ipc_enabled flag assignment inside intel_ipc_enable function
> Changes since V4:
>  - Re-enable IPC after suspend/resume
> Changes since V5:
>  - Enable IPC for all gen >=9 except SKL
> 
> Signed-off-by: Mahesh Kumar 
> ---
>  drivers/gpu/drm/i915/i915_drv.c  |  4 +++-
>  drivers/gpu/drm/i915/i915_reg.h  |  1 +
>  drivers/gpu/drm/i915/intel_display.c |  1 +
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 16 
>  5 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 9167a73f3c69..224e00610581 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1334,7 +1334,7 @@ int i915_driver_load(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  
>   intel_runtime_pm_enable(dev_priv);
>  
> - dev_priv->ipc_enabled = false;
> + intel_enable_ipc(dev_priv);
>  
>   if (IS_ENABLED(CONFIG_DRM_I915_DEBUG))
>   DRM_INFO("DRM_I915_DEBUG enabled\n");
> @@ -2598,6 +2598,8 @@ static int intel_runtime_resume(struct device
> *kdev)
>   if (!IS_VALLEYVIEW(dev_priv) && !IS_CHERRYVIEW(dev_priv))
>   intel_hpd_init(dev_priv);
>  
> + intel_enable_ipc(dev_priv);
> +
>   enable_rpm_wakeref_asserts(dev_priv);
>  
>   if (ret)
> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> b/drivers/gpu/drm/i915/i915_reg.h
> index 64cc674b652a..09af90f8cd0a 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -6728,6 +6728,7 @@ enum {
>  #define  DISP_FBC_WM_DIS (1<<15)
>  #define DISP_ARB_CTL2_MMIO(0x45004)
>  #define  DISP_DATA_PARTITION_5_6 (1<<6)
> +#define  DISP_IPC_ENABLE (1<<3)
>  #define DBUF_CTL _MMIO(0x45008)
>  #define  DBUF_POWER_REQUEST  (1<<31)
>  #define  DBUF_POWER_STATE(1<<30)
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 0648fd74be87..e610b4395dcc 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15723,6 +15723,7 @@ void intel_display_resume(struct drm_device
> *dev)
>   if (!ret)
>   ret = __intel_display_resume(dev, state, );
>  
> + intel_enable_ipc(dev_priv);
>   drm_modeset_drop_locks();
>   drm_modeset_acquire_fini();
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index d17a32437f07..d90b239bd85d 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1883,6 +1883,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
> enable_rc6);
>  int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>     struct intel_crtc_state *cstate);
> +void intel_enable_ipc(struct drm_i915_private *dev_priv);
>  static inline int intel_enable_rc6(void)
>  {
>   return i915.enable_rc6;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index ad3b3d758d5c..c18695931d33 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5772,6 +5772,22 @@ void intel_update_watermarks(struct intel_crtc
> *crtc)
>   dev_priv->display.update_wm(crtc);
>  }
>  
> +void intel_enable_ipc(struct drm_i915_private *dev_priv)
> +{
> + u32 val;
> +
> + dev_priv->ipc_enabled = false;
> + if (INTEL_GEN(dev_priv) < 9 || IS_SKYLAKE(dev_priv))
> + return;
> +
> + val = I915_READ(DISP_ARB_CTL2);
> +
> + val |= DISP_IPC_ENABLE;
> +
> + I915_WRITE(DISP_ARB_CTL2, val);
> + dev_priv->ipc_enabled = true;
> +}
> +
>  /*
>   * Lock protecting IPS related data structures
>   */
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org

Re: [Intel-gfx] [PATCH 4/8] drm/i915: Addition wrapper for fixed16.16 operation

[Lankhorst, Maarten]

Mahesh Kumar schreef op wo 21-06-2017 om 11:44 [+0530]:
> This patch introduce addition wrapper for fixed point 16.16
> operations.
> Which will be used by later patches to avoid direct member variables
> access of fixed_16_16_t structure.
> 
> add_fixed16 : takes 2 fixed_16_16_t variable & returns fixed_16_16_t
> add_fixed16_u32 : takes fixed_16_16_t & u32 variable & returns
> fixed_16_16_t

Much easier to review. :)

Reviewed-by: Maarten Lankhorst  for
first 4 patches.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 6/6] drm/i915/gen10: Calculate and enable transition WM

[Lankhorst, Maarten]

Hey,

Mahesh Kumar schreef op di 13-06-2017 om 11:34 [+0530]:
> GEN > 9 require transition WM to be programmed if IPC is enabled.
> This patch calculates & enable transition WM for supported platforms.
> If transition WM is enabled, Plane read requests are sent at high
> priority until filling above the transition watermark, then the
> requests are sent at lower priority until dropping below the level-0
> WM.
> The lower priority requests allow other memory clients to have better
> memory access.
> 
> transition minimum is the minimum amount needed for trans_wm to work
> to
> ensure  the demote does not happen before enough data has been read
> to
> meet the level 0 watermark requirements.
> 
> transition amount is configurable value. Higher values will
> tend to cause longer periods of high priority reads followed by
> longer
> periods of lower priority reads. Tuning to lower values will tend to
> cause shorter periods of high and lower priority reads.
> 
> Keeping transition amount to 0 in this patch.

> Signed-off-by: Mahesh Kumar 
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 51
> ++---
>  1 file changed, 48 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 10ec2660acd7..6b951aa14840 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4163,6 +4163,15 @@ skl_enable_plane_wm_levels(const struct
> drm_i915_private *dev_priv,
>   level_wm->plane_en = true;
>   }
>   }
> +
> + /*
> +  * Unsupported GEN will have plane_res_b = 0 & transition WM
> for
> +  * them will get disabled here.
> +  */
> + if (wm->trans_wm.plane_res_b && wm->trans_wm.plane_res_b <
> plane_ddb)
> + wm->trans_wm.plane_en = true;
> + else
> + wm->trans_wm.plane_en = false;
>  }
>  
>  static int
> @@ -4639,13 +4648,48 @@ skl_compute_linetime_wm(struct
> intel_crtc_state *cstate)
>  }
>  
>  static void skl_compute_transition_wm(struct intel_crtc_state
> *cstate,
> +   struct skl_wm_params *wp,
> +   struct skl_wm_level *wm_l0,
>     struct skl_wm_level *trans_wm
> /* out */)
>  {
> + struct drm_device *dev = cstate->base.crtc->dev;
> + const struct drm_i915_private *dev_priv = to_i915(dev);
> + uint16_t trans_min, trans_y_tile_min;
> + uint16_t trans_amount = 0; /* This is configurable amount */
> + uint16_t trans_offset_b, res_blocks;
> +
>   if (!cstate->base.active)
>   return;
> - /* Until we know more, just disable transition WMs */
> - trans_wm->plane_en = false;
> + /* Transition WM are not recommended by HW team for GEN9 */
> + if (INTEL_GEN(dev_priv) <= 9)
> + return;
> +
> + /* Transition WM don't have any impact if ipc is disabled */
> + if (!dev_priv->ipc_enabled)
> + return;
ipc_enabled is never set, so this patch on its own doesn't do much. :)

Otherwise series looks good, I didn't check the math on this patch,
but the series doesn't regress KBL.

For patch 3 and 4:
Reviewed-by: Maarten Lankhorst 

For patch 5 and 6:
Acked-by: Maarten Lankhorst 

> + if (INTEL_GEN(dev_priv) >= 10)
> + trans_min = 4;
> +
> + trans_offset_b = trans_min + trans_amount;
> + trans_y_tile_min = (uint16_t) mul_round_up_u32_fixed16(2,
> + wp-
> >y_tile_minimum);
> +
> + if (wp->y_tiled) {
> + res_blocks = max(wm_l0->plane_res_b,
> trans_y_tile_min) +
> + trans_offset_b;
> + } else {
> + res_blocks = wm_l0->plane_res_b + trans_offset_b;
> + }
> +
> + res_blocks += 1;
> +
> + /* WA BUG:1938466 add one block for non y-tile planes */
> + if (!wp->y_tiled && IS_CNL_REVID(dev_priv, CNL_REVID_A0,
> CNL_REVID_A0))
> + res_blocks += 1;
> +
> + trans_wm->plane_res_b = res_blocks;
>  }
>  
>  static int skl_build_pipe_wm(struct intel_crtc_state *cstate,
> @@ -4684,7 +4728,8 @@ static int skl_build_pipe_wm(struct
> intel_crtc_state *cstate,
>   _params, wm);
>   if (ret)
>   return ret;
> - skl_compute_transition_wm(cstate, >trans_wm);
> + skl_compute_transition_wm(cstate, _params, 
> >wm[0],
> +   >trans_wm);
>   }
>   pipe_wm->linetime = skl_compute_linetime_wm(cstate);
>  

smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/6] drm/i915/hsw: use intel_compute_linetime_wm function for linetime wm

[Lankhorst, Maarten]

Mahesh Kumar schreef op di 13-06-2017 om 11:34 [+0530]:
> linetime wm is time taken to fill a single display line with given
> clock
> rate, multiplied by 8.
> This patch reuses the common code of hsw_compute_linetime_wm &
> skl_compute_linetime_wm.
> 
> Signed-off-by: Mahesh Kumar 
> ---
>  drivers/gpu/drm/i915/intel_drv.h |  1 +
>  drivers/gpu/drm/i915/intel_pm.c  | 23 ++-
>  2 files changed, 15 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index 83dd40905821..ea2d29e68bfe 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1877,6 +1877,7 @@ bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
> enable_rc6);
>  int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
>     struct intel_crtc_state *cstate);
> +uint32_t intel_compute_linetime_wm(const struct intel_crtc_state
> *cstate);
This function is only used inside intel_pm.c, so should only be
declared there. :)

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

Re: [Intel-gfx] [PATCH 1/6] drm/i915: cleanup fixed-point wrappers naming

[Lankhorst, Maarten]

Mahesh Kumar schreef op di 13-06-2017 om 11:34 [+0530]:
> This patch make naming of fixed-point wrappers consistent
> operation__<1st operand>_<2nd operand>
> also shorten the name for fixed_16_16 to fixed16
> 
> s/u32_to_fixed_16_16/u32_to_fixed16
> s/fixed_16_16_to_u32/fixed16_to_u32
> s/fixed_16_16_to_u32_round_up/fixed16_to_u32_round_up
> s/min_fixed_16_16/min_fixed16
> s/max_fixed_16_16/max_fixed16
> s/mul_u32_fixed_16_16/mul_u32_fixed16
> 
> always do division internal operation in 64 bits:
> s/fixed_16_16_div/div_fixed16
> s/fixed_16_16_div_64/div_fixed16
> 
> Introduce Addition wrappers for fixed16
> add_fixed16 : takes 2 fixed_16_16_t variable & returns fixed16_16_t
> add_fixed16_u32 : takes fixed_16_16_t & u32 variable & returns
> fixed16_16_t
I think to make it readable, the additions should be done separately.

mul_round_up_u32_fixed16 seems like it should use clamp_u64_to_fixed16,
perhaps others too. Maybe do it as a preparation patch so this patch
strictly does the renaming for review?
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 1/3] drm/i915/skl+: Don't trust cached ddb values

[Lankhorst, Maarten]

Mahesh Kumar schreef op di 30-05-2017 om 18:26 [+0530]:
> Hi Maarten,
> 
> Thanks for review :)
> 
> 
> On Tuesday 30 May 2017 03:30 PM, Maarten Lankhorst wrote:
> > Op 26-05-17 om 17:15 schreef Mahesh Kumar:
> > > Don't trust cached DDB values. Recalculate the ddb value if
> > > cached value
> > > is zero.
> > > 
> > > If i915 is build as a module, there may be a race condition when
> > > cursor_disable call comes even before intel_fbdev_initial_config.
> > > Which may lead to cached value being 0. And further commit will
> > > fail
> > > until a modeset.
> > > 
> > > Signed-off-by: Mahesh Kumar 
> > > ---
> > >   drivers/gpu/drm/i915/intel_pm.c | 15 ++-
> > >   1 file changed, 10 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 936eef1634c7..b67be1355e39 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -3721,6 +3721,7 @@ skl_ddb_get_pipe_allocation_limits(struct
> > > drm_device *dev,
> > >   struct drm_i915_private *dev_priv = to_i915(dev);
> > >   struct drm_crtc *for_crtc = cstate->base.crtc;
> > >   unsigned int pipe_size, ddb_size;
> > > + unsigned int active_crtcs;
> > >   int nth_active_pipe;
> > >   
> > >   if (WARN_ON(!state) || !cstate->base.active) {
> > > @@ -3731,10 +3732,11 @@ skl_ddb_get_pipe_allocation_limits(struct
> > > drm_device *dev,
> > >   }
> > >   
> > >   if (intel_state->active_pipe_changes)
> > > - *num_active = hweight32(intel_state-
> > > >active_crtcs);
> > > + active_crtcs = intel_state->active_crtcs;
> > >   else
> > > - *num_active = hweight32(dev_priv->active_crtcs);
> > > + active_crtcs = dev_priv->active_crtcs;
> > >   
> > > + *num_active = hweight32(active_crtcs);
> > >   ddb_size = INTEL_INFO(dev_priv)->ddb_size;
> > >   WARN_ON(ddb_size == 0);
> > >   
> > > @@ -3754,12 +3756,15 @@ skl_ddb_get_pipe_allocation_limits(struct
> > > drm_device *dev,
> > >    * copy from old state to be sure
> > >    */
> > >   *alloc = to_intel_crtc_state(for_crtc->state)-
> > > >wm.skl.ddb;
> > > - return;
> > > + if (!skl_ddb_entry_size(alloc))
> > > + DRM_DEBUG_KMS("Cached pipe DDB is 0
> > > recalculate\n");
> > > + else
> > > + return;
> > >   }
> > >   
> > > - nth_active_pipe = hweight32(intel_state->active_crtcs &
> > > + nth_active_pipe = hweight32(active_crtcs &
> > >   (drm_crtc_mask(for_crtc) -
> > > 1));
> > > - pipe_size = ddb_size / hweight32(intel_state-
> > > >active_crtcs);
> > > + pipe_size = ddb_size / hweight32(active_crtcs);
> > >   alloc->start = nth_active_pipe * ddb_size /
> > > *num_active;
> > >   alloc->end = alloc->start + pipe_size;
> > >   }
> > 
> > I'd love it if you also add a warning somewhere for active pipe
> > with no ddb allocation to prevent this from reoccuring in the
> > future. :)
> > But with the root cause being a invalid ddb allocation at boot,
> > won't the below one liner should fix it too, but better?
> 
> Above DEBUG message is actually warning. That will hit only if pipe
> is 
> active & DDB allocated for that pipe is "0" (we do return from caller
> if 
> !cstate->active).
> I can reword the msg to be WARN & make it something like "Pipe is
> active 
> with No DDB allocated, recompute ddb now". This will fix the issue
> as 
> well as add warning msg.
> 
> > 
> > I'l ltest it when f3-kbl becomes available again.
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > b/drivers/gpu/drm/i915/intel_pm.c
> > index a488e068c3d6..63a47909f618 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -5051,7 +5051,7 @@ skl_compute_wm(struct drm_atomic_state
> > *state)
> >      */
> >     for_each_new_crtc_in_state(state, crtc, cstate, i)
> >     changed = true;
> > -   if (!changed)
> > +   if (!changed && !to_i915(state->dev)->wm.distrust_bios_wm)
> >     return 0;
> >   
> 
> This change should also solve the issue during boot.

Yeah I did some testing and this fixes it.
Looks like a bug in the original implementation of skl_compute_wm that
the KBL just happened to hit.

So I guess if others are hitting it too, then this needs
Fixes: 98d39494d375 ("drm/i915/gen9: Compute DDB allocation at atomic
check time (v4)")
Cc:  # v4.8+

smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm/i915/skl+: consider max supported plane pixel rate while scaling

[Lankhorst, Maarten]

Mahesh Kumar schreef op do 18-05-2017 om 15:39 [+0530]:
> From: "Kumar, Mahesh" 
> 
> A display resolution is only supported if it meets all the
> restrictions
> below for Maximum Pipe Pixel Rate.
> 
> The display resolution must fit within the maximum pixel rate output
> from the pipe. Make sure that the display pipe is able to feed pixels
> at
> a rate required to support the desired resolution.
> For each enabled plane on the pipe {
> If plane scaling enabled {
>   Horizontal down scale amount = Maximum[1, plane horizontal size
> /
>   scaler horizontal window size]
>   Vertical down scale amount = Maximum[1, plane vertical size /
>   scaler vertical window size]
>   Plane down scale amount = Horizontal down scale amount *
>   Vertical down scale amount
>   Plane Ratio = 1 / Plane down scale amount
> }
> Else {
>   Plane Ratio = 1
> }
> If plane source pixel format is 64 bits per pixel {
>   Plane Ratio = Plane Ratio * 8/9
> }
> }
> 
> Pipe Ratio = Minimum Plane Ratio of all enabled planes on the pipe
> 
> If pipe scaling is enabled {
> Horizontal down scale amount = Maximum[1, pipe horizontal source
> size /
>   scaler horizontal window size]
> Vertical down scale amount = Maximum[1, pipe vertical source size
> /
>   scaler vertical window size]
> Note: The progressive fetch - interlace display mode is
> equivalent to a
>   2.0 vertical down scale
> Pipe down scale amount = Horizontal down scale amount *
>   Vertical down scale amount
> Pipe Ratio = Pipe Ratio / Pipe down scale amount
> }
> 
> Pipe maximum pixel rate = CDCLK frequency * Pipe Ratio
> 
> In this patch our calculation is based on pipe downscale amount
> (plane max downscale amount * pipe downscale amount) instead of Pipe
> Ratio. So,
> max supported crtc clock with given scaling = CDCLK / pipe downscale.
> Flip will fail if,
> current crtc clock > max supported crct clock with given scaling.
> 
> Changes since V1:
>  - separate out fixed_16_16 wrapper API definition
> Changes since V2:
>  - Fix buggy crtc !active condition (Maarten)
>  - use intel_wm_plane_visible wrapper as per Maarten's suggestion
> Changes since V3:
>  - Change failure return from ERANGE to EINVAL
> Changes since V4:
>  - Rebase based on previous patch changes
> Changes since V5:
>  - return EINVAL instead of continue (Maarten)
> Changes since V6:
>  - Improve commit message
>  - Address review comment
> 
> Signed-off-by: Mahesh Kumar 
> Reviewed-by: Matt Roper 
> Reviewed-by: Maarten Lankhorst 
> ---
>  drivers/gpu/drm/i915/intel_display.c |  3 ++
>  drivers/gpu/drm/i915/intel_drv.h |  2 +
>  drivers/gpu/drm/i915/intel_pm.c  | 87
> 
>  3 files changed, 92 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> index 8217ed0e7132..df0b3e77129b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11177,6 +11177,9 @@ static int intel_crtc_atomic_check(struct
> drm_crtc *crtc,
>   ret = skl_update_scaler_crtc(pipe_config);
>  
>   if (!ret)
> + ret =
> skl_check_pipe_max_pixel_rate(intel_crtc,
> + pipe_con
> fig);
> + if (!ret)
>   ret = intel_atomic_setup_scalers(dev_priv,
> intel_crtc,
>    pipe_config
> );
>   }
> diff --git a/drivers/gpu/drm/i915/intel_drv.h
> b/drivers/gpu/drm/i915/intel_drv.h
> index bd500977b3fc..93afac4a83fa 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1885,6 +1885,8 @@ bool skl_ddb_allocation_overlaps(const struct
> skl_ddb_entry **entries,
>    int ignore);
>  bool ilk_disable_lp_wm(struct drm_device *dev);
>  int sanitize_rc6_option(struct drm_i915_private *dev_priv, int
> enable_rc6);
> +int skl_check_pipe_max_pixel_rate(struct intel_crtc *intel_crtc,
> +   struct intel_crtc_state *cstate);
>  static inline int intel_enable_rc6(void)
>  {
>   return i915.enable_rc6;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index fb4cec3fb92c..4edc5f4ee598 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3863,6 +3863,93 @@ skl_plane_downscale_amount(const struct
> intel_crtc_state *cstate,
>   return mul_fixed16(downscale_w, downscale_h);
>  }
>  
> +static uint_fixed_16_16_t
> +skl_pipe_downscale_amount(const struct intel_crtc_state *config)
> +{
> + uint_fixed_16_16_t pipe_downscale = u32_to_fixed_16_16(1);
> +
> + if (!config->base.active)
> 

Re: [Intel-gfx] [PATCH 1/2] drm/i915/skl: New ddb allocation algorithm

[Lankhorst, Maarten]

Mahesh Kumar schreef op do 18-05-2017 om 15:39 [+0530]:
> From: "Kumar, Mahesh" 
> 
> This patch implements new DDB allocation algorithm as per HW team
> recommendation. This algo takecare of scenario where we allocate less
> DDB
> for the planes with lower relative pixel rate, but they require more
> DDB
> to work.
> It also takes care of enabling same watermark level for each
> plane in crtc, for efficient power saving.
> 
> Changes since v1:
>  - Rebase on top of Paulo's patch series
> 
> Changes since v2:
>  - Fix the for loop condition to enable WM
> 
> Changes since v3:
>  - Fix crash in cursor i-g-t reported by Maarten
>  - Rebase after addressing Paulo's comments
>  - Few other ULT fixes
> Changes since v4:
>  - Rebase on drm-tip
>  - Added separate function to enable WM levels
> Changes since v5:
>  - Fix a crash identified in skl-6770HQ system
> Changes since v6:
>  - Address review comments from Matt
> Changes since v7:
>  - Fix failure return in skl_compute_plane_wm (Matt)
>  - fix typo
> 
> Signed-off-by: Mahesh Kumar 
> Reviewed-by: Maarten Lankhorst 

Hm just to be sure it works as intended I tested this against full kms,
testsuite and it seems to cause FIFO underruns on my KBL with the
kms_atomic_transition test.

I think we shouldn't merge this until all those fifo underruns have
been fixed, especially because FIFO underruns may result in a complete
lockup of the gpu.

Sorry for the bad news,
Maarten
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/11] drm/i915/skl: Fail the flip if no FB for WM calculation

[Lankhorst, Maarten]

Mahesh Kumar schreef op ma 08-05-2017 om 17:18 [+0530]:
> Fail the flip if no FB is present but plane_state is set as visible.
> Above is not a valid combination so instead of continue fail the
> flip.

Why is this patch necessary? drm_atomic_plane_check handles this.

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

Re: [Intel-gfx] [PATCH v3 7/8] drm: Connector helper function to release resources

[Lankhorst, Maarten]

Daniel Vetter schreef op zo 26-02-2017 om 21:00 [+0100]:
> On Fri, Feb 24, 2017 at 12:52:53AM +, Pandiyan, Dhinakaran wrote:
> > 
> > On Thu, 2017-02-16 at 09:09 +0000, Lankhorst, Maarten wrote:
> > > 
> > > Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> > > > 
> > > > On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> > > > <dhinakaran.pandi...@intel.com> wrote:
> > > > > 
> > > > > On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> > > > > > 
> > > > > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55
> > > > > > [+]:
> > > > > > > 
> > > > > > > > 
> > > > > > > > Could we deal with the VCPI state separately in
> > > > > > > > intel_modeset_checks,
> > > > > > > > like we do with dpll?
> > > > > > > 
> > > > > > > We'd want to release the VCPI slots before they are
> > > > > > > acquired in
> > > > > > > ->compute_config(). intel_modeset_checks() will be too
> > > > > > > late to
> > > > > > > release
> > > > > > > them. Are you suggesting both acquiring and releasing
> > > > > > > slots
> > > > > > > should be
> > > > > > > done in intel_modeset_checks()?
> > > > > > 
> > > > > > That makes things a bit more nasty. Maybe add a
> > > > > > conn_funcs->atomic_check that always gets called, something
> > > > > > like
> > > > > > I did
> > > > > > below?
> > > > > > 
> > > > > > I'd love to use it for some atomic connector properties
> > > > > > too.
> > > > > 
> > > > > 
> > > > > Adding and unconditionally calling conn_funcs->atomic_check()
> > > > > should be
> > > > > doable. It also follows the pattern we have for encoders and
> > > > > CRTCs.
> > > > > But
> > > > > I'll have to move the connector->state->crtc state checks
> > > > > inside
> > > > > the
> > > > > function.
> > > > 
> > > > Adding ->atomic_check that's unconditionally called sounds
> > > > troubling,
> > > > because all the other ->atomic_check functions are _only_
> > > > called when
> > > > enabling stuff. ->atomic_release sounds much better to me, and
> > > > from a
> > > > helper pov DK's patch above is the right place.
> > > 
> > > Having an atomic check would be nice for implementing connector
> > > properties. Some of them may need to be validated regardless of
> > > crtc.
> > > 
> > 
> > Can we add this later when we need state validation that is
> > appropriate
> > for an ->atomic_check()? 
> 
> +1 on not solving problems we don't have yet :-) If we'd write code
> for
> every eventuality that we can come up with, we'd get nothing done.
> And
> ime, such unused code will also be full of bugs.
> 
> Discussing issues like this is still good and useful, just to make
> sure we
> have a coherent plan for the eventual future, once it happens.

Near future, I'm working on converting i915 specific connector
properties to atomic, and it would be nice if I had a connector atomic
check function like this. :)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 7/8] drm: Connector helper function to release resources

[Lankhorst, Maarten]

Pandiyan, Dhinakaran schreef op ma 13-02-2017 om 22:48 [+]:
> On Mon, 2017-02-13 at 21:26 +, Pandiyan, Dhinakaran wrote:
> > 
> > On Mon, 2017-02-13 at 09:05 +0000, Lankhorst, Maarten wrote:
> > > 
> > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> > > > 
> > > > On Thu, 2017-02-09 at 09:01 +, Lankhorst, Maarten wrote:
> > > > > 
> > > > > 
> > > > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-
> > > > > 0800]:
> > > > > > 
> > > > > > 
> > > > > > Having a ->atomic_release callback is useful to release
> > > > > > shared
> > > > > > resources
> > > > > > that get allocated in compute_config(). This function is
> > > > > > expected
> > > > > > to
> > > > > > be
> > > > > > called in the atomic_check() phase before new resources are
> > > > > > acquired.
> > > > > > 
> > > > > > v2: Moved the caller hunk to this patch (Daniel)
> > > > > > 
> > > > > > Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@int
> > > > > > el.com
> > > > > > > 
> > > > > > > 
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_atomic_helper.c  | 19
> > > > > > +++
> > > > > >  include/drm/drm_modeset_helper_vtables.h | 13
> > > > > > +
> > > > > >  2 files changed, 32 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > index 8795088..92bd741 100644
> > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > > > > drm_device *dev,
> > > > > >     }
> > > > > >     }
> > > > > >  
> > > > > > +   for_each_connector_in_state(state, connector,
> > > > > > connector_state, i) {
> > > > > > +   const struct drm_connector_helper_funcs
> > > > > > *conn_funcs;
> > > > > > +   struct drm_crtc_state *crtc_state;
> > > > > > +
> > > > > > +   conn_funcs = connector->helper_private;
> > > > > > +   if (!conn_funcs->atomic_release)
> > > > > > +   continue;
> > > > > > +
> > > > > > +   if (!connector->state->crtc)
> > > > > > +   continue;
> > > > > > +
> > > > > > +   crtc_state =
> > > > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > > > > > 
> > > > > > > crtc);
> > > > > > +
> > > > > > +   if (crtc_state->connectors_changed ||
> > > > > > +   crtc_state->mode_changed ||
> > > > > > +   (crtc_state->active_changed &&
> > > > > > !crtc_state-
> > > > > > > 
> > > > > > > 
> > > > > > > active))
> > > > > > +   conn_funcs-
> > > > > > >atomic_release(connector,
> > > > > > connector_state);
> > > > > > +   }
> > > > > 
> > > > > Could we deal with the VCPI state separately in
> > > > > intel_modeset_checks,
> > > > > like we do with dpll?
> > > > 
> > > > We'd want to release the VCPI slots before they are acquired in
> > > > ->compute_config(). intel_modeset_checks() will be too late to
> > > > release
> > > > them. Are you suggesting both acquiring and releasing slots
> > > > should be
> > > > done in intel_modeset_checks()?
> > > 
> > > That makes things a bit more nasty. Maybe add a
> > > conn_funcs->atomic_check that always gets called, something like
> > > I did
> > > below?
> > > 
> > > I'd love to use it for some atomic connector properties too.
> > 
> > 
> > Adding and unconditionally calling conn_funcs->atomic_check()
> > should be
> > doable. It also follows the pattern we have for encoders and CRTCs.
> > But
> > I'll have to move the connector->state->crtc state checks inside
> > the
> > function.
> > 
> > -DK
> 
> 
> This is what I mean -https://pastebin.ubuntu.com/23991405/
> But, I do have one concern with calling this conn_func-
> >atomic_check().
> We are not validating the new connector_state like atomic_check()
> seems
> to do generally but only cleaning up vcpi resources for
> compute_config()
> to later acquire. Let me know if I am wrong in my understanding what
> atomic_check() is expected to do.

Yeah looks good. I think it makes sense to have such a validation
function. There may not be much in it now but that could change when
i915 connector properties are made atomic. :)

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

Re: [Intel-gfx] [PATCH v3 7/8] drm: Connector helper function to release resources

[Lankhorst, Maarten]

Daniel Vetter schreef op di 14-02-2017 om 20:51 [+0100]:
> On Mon, Feb 13, 2017 at 10:26 PM, Pandiyan, Dhinakaran
> <dhinakaran.pandi...@intel.com> wrote:
> > On Mon, 2017-02-13 at 09:05 +, Lankhorst, Maarten wrote:
> > > Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> > > > On Thu, 2017-02-09 at 09:01 +, Lankhorst, Maarten wrote:
> > > > > 
> > > > > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-
> > > > > 0800]:
> > > > > > 
> > > > > > Having a ->atomic_release callback is useful to release
> > > > > > shared
> > > > > > resources
> > > > > > that get allocated in compute_config(). This function is
> > > > > > expected
> > > > > > to
> > > > > > be
> > > > > > called in the atomic_check() phase before new resources are
> > > > > > acquired.
> > > > > > 
> > > > > > v2: Moved the caller hunk to this patch (Daniel)
> > > > > > 
> > > > > > Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > > > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandiyan@int
> > > > > > el.com
> > > > > > > 
> > > > > > 
> > > > > > ---
> > > > > >  drivers/gpu/drm/drm_atomic_helper.c  | 19
> > > > > > +++
> > > > > >  include/drm/drm_modeset_helper_vtables.h | 13
> > > > > > +
> > > > > >  2 files changed, 32 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > index 8795088..92bd741 100644
> > > > > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > > > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > > > > drm_device *dev,
> > > > > > }
> > > > > > }
> > > > > > 
> > > > > > +   for_each_connector_in_state(state, connector,
> > > > > > connector_state, i) {
> > > > > > +   const struct drm_connector_helper_funcs
> > > > > > *conn_funcs;
> > > > > > +   struct drm_crtc_state *crtc_state;
> > > > > > +
> > > > > > +   conn_funcs = connector->helper_private;
> > > > > > +   if (!conn_funcs->atomic_release)
> > > > > > +   continue;
> > > > > > +
> > > > > > +   if (!connector->state->crtc)
> > > > > > +   continue;
> > > > > > +
> > > > > > +   crtc_state =
> > > > > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > > > > > crtc);
> > > > > > 
> > > > > > +
> > > > > > +   if (crtc_state->connectors_changed ||
> > > > > > +   crtc_state->mode_changed ||
> > > > > > +   (crtc_state->active_changed &&
> > > > > > !crtc_state-
> > > > > > > 
> > > > > > > active))
> > > > > > 
> > > > > > +   conn_funcs-
> > > > > > >atomic_release(connector,
> > > > > > connector_state);
> > > > > > +   }
> > > > > 
> > > > > Could we deal with the VCPI state separately in
> > > > > intel_modeset_checks,
> > > > > like we do with dpll?
> > > > 
> > > > We'd want to release the VCPI slots before they are acquired in
> > > > ->compute_config(). intel_modeset_checks() will be too late to
> > > > release
> > > > them. Are you suggesting both acquiring and releasing slots
> > > > should be
> > > > done in intel_modeset_checks()?
> > > 
> > > That makes things a bit more nasty. Maybe add a
> > > conn_funcs->atomic_check that always gets called, something like
> > > I did
> > > below?
> > > 
> > >

Re: [Intel-gfx] [PATCH v3 7/8] drm: Connector helper function to release resources

[Lankhorst, Maarten]

Pandiyan, Dhinakaran schreef op do 09-02-2017 om 18:55 [+]:
> On Thu, 2017-02-09 at 09:01 +0000, Lankhorst, Maarten wrote:
> > 
> > Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> > > 
> > > Having a ->atomic_release callback is useful to release shared
> > > resources
> > > that get allocated in compute_config(). This function is expected
> > > to
> > > be
> > > called in the atomic_check() phase before new resources are
> > > acquired.
> > > 
> > > v2: Moved the caller hunk to this patch (Daniel)
> > > 
> > > Suggested-by: Daniel Vetter <daniel.vet...@ffwll.ch>
> > > Signed-off-by: Dhinakaran Pandiyan <dhinakaran.pandi...@intel.com
> > > >
> > > ---
> > >  drivers/gpu/drm/drm_atomic_helper.c  | 19
> > > +++
> > >  include/drm/drm_modeset_helper_vtables.h | 13 +
> > >  2 files changed, 32 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> > > b/drivers/gpu/drm/drm_atomic_helper.c
> > > index 8795088..92bd741 100644
> > > --- a/drivers/gpu/drm/drm_atomic_helper.c
> > > +++ b/drivers/gpu/drm/drm_atomic_helper.c
> > > @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> > > drm_device *dev,
> > >   }
> > >   }
> > >  
> > > + for_each_connector_in_state(state, connector,
> > > connector_state, i) {
> > > + const struct drm_connector_helper_funcs
> > > *conn_funcs;
> > > + struct drm_crtc_state *crtc_state;
> > > +
> > > + conn_funcs = connector->helper_private;
> > > + if (!conn_funcs->atomic_release)
> > > + continue;
> > > +
> > > + if (!connector->state->crtc)
> > > + continue;
> > > +
> > > + crtc_state =
> > > drm_atomic_get_existing_crtc_state(state, connector->state-
> > > >crtc);
> > > +
> > > + if (crtc_state->connectors_changed ||
> > > + crtc_state->mode_changed ||
> > > + (crtc_state->active_changed && !crtc_state-
> > > > 
> > > > active))
> > > + conn_funcs->atomic_release(connector,
> > > connector_state);
> > > + }
> > 
> > Could we deal with the VCPI state separately in
> > intel_modeset_checks,
> > like we do with dpll?
> 
> We'd want to release the VCPI slots before they are acquired in
> ->compute_config(). intel_modeset_checks() will be too late to
> release
> them. Are you suggesting both acquiring and releasing slots should be
> done in intel_modeset_checks()?

That makes things a bit more nasty. Maybe add a
conn_funcs->atomic_check that always gets called, something like I did
below?

I'd love to use it for some atomic connector properties too.
> > 
> > 
> > Maybe implementing the relevant VCPI state could be done as an
> > atomic
> > helper function too, so other atomic drivers can just plug it in.
> > 
> The idea was to reduce boilerplate in the drivers and use the
> private_obj state for different object types.
> 
> 
> > 
> > Not sure how doable this is, but if it's not too hard, then it's
> > probably cleaner :)
> > ___
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 39cbacd8cd07..1e5f0a95c685 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -381,11 +381,24 @@ mode_fixup(struct drm_atomic_state *state)
 
 	for_each_new_connector_in_state(state, connector, conn_state, i) {
 		const struct drm_encoder_helper_funcs *funcs;
+		const struct drm_connector_helper_funcs *conn_funcs;
 		struct drm_encoder *encoder;
 
+		conn_funcs = connector->helper_private;
+
 		WARN_ON(!!conn_state->best_encoder != !!conn_state->crtc);
 
-		if (!conn_state->crtc || !conn_state->best_encoder)
+		if (!conn_state->crtc || !conn_state->best_encoder) {
+			if (conn_funcs && conn_funcs->atomic_check) {
+ret = conn_funcs->atomic_check(connector, conn_state);
+
+if (ret) {
+	DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] check failed\n",
+			 connector->base.id, connector->name);
+	return ret;
+}
+			}
+
 			continue;
 
 		crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
@@ -404,7 +417,15 @@ mode_fixup(struct drm_atomic_state *state)
 			return -EINVAL;
 		}
 
-		if (funcs && funcs->atomic_check) {
+		if (conn_funcs && conn_funcs->atomic_check) {
+			ret = conn_funcs->atomic_check(connector, conn_state);
+
+			if (ret) {
+DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] check failed\n",
+		  connector->base.id, connector->name);
+return ret;
+			}
+		} else if (funcs && funcs->atomic_check) {
 			ret = funcs->atomic_check(encoder, crtc_state,
 		  conn_state);
 			if (ret) {
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v3 7/8] drm: Connector helper function to release resources

[Lankhorst, Maarten]

Dhinakaran Pandiyan schreef op wo 08-02-2017 om 22:38 [-0800]:
> Having a ->atomic_release callback is useful to release shared
> resources
> that get allocated in compute_config(). This function is expected to
> be
> called in the atomic_check() phase before new resources are acquired.
> 
> v2: Moved the caller hunk to this patch (Daniel)
> 
> Suggested-by: Daniel Vetter 
> Signed-off-by: Dhinakaran Pandiyan 
> ---
>  drivers/gpu/drm/drm_atomic_helper.c  | 19 +++
>  include/drm/drm_modeset_helper_vtables.h | 13 +
>  2 files changed, 32 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 8795088..92bd741 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -576,6 +576,25 @@ drm_atomic_helper_check_modeset(struct
> drm_device *dev,
>   }
>   }
>  
> + for_each_connector_in_state(state, connector,
> connector_state, i) {
> + const struct drm_connector_helper_funcs *conn_funcs;
> + struct drm_crtc_state *crtc_state;
> +
> + conn_funcs = connector->helper_private;
> + if (!conn_funcs->atomic_release)
> + continue;
> +
> + if (!connector->state->crtc)
> + continue;
> +
> + crtc_state =
> drm_atomic_get_existing_crtc_state(state, connector->state->crtc);
> +
> + if (crtc_state->connectors_changed ||
> + crtc_state->mode_changed ||
> + (crtc_state->active_changed && !crtc_state-
> >active))
> + conn_funcs->atomic_release(connector,
> connector_state);
> + }

Could we deal with the VCPI state separately in intel_modeset_checks,
like we do with dpll?

Maybe implementing the relevant VCPI state could be done as an atomic
helper function too, so other atomic drivers can just plug it in.

Not sure how doable this is, but if it's not too hard, then it's
probably cleaner :)
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t] Add atomic modesetting testlist

[Lankhorst, Maarten]

Rami Ben Hassine schreef op do 26-01-2017 om 16:36 [+0100]:
> From: ramibh 
> 
> This is atomic modesetting acceptance tests added to
> feat_profile.json. Wating for your feedback.
> 
> ---
>  tests/feat_profile.json | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/feat_profile.json b/tests/feat_profile.json
> index 251dfd9..bba4b3c 100644
> --- a/tests/feat_profile.json
> +++ b/tests/feat_profile.json
> @@ -3,5 +3,11 @@
>  "include_tests" : "psr",
>  "exclude_tests" : "",
>  "target_rate" : 90
> -}
> +},
> +"atomic-modesetting":{
> +"include_tests" : " kms_atomic_transition kms_atomic
> kms_crtc_background_color kms_cursor_legacy Kms_cursor_crc kms_flip
> kms_flip_event_leak kms_flip_tiling kms_plane kms_plane_lowres
> kms_plane_multiple kms_panel_fitting kms_plane_scaling
> kms_psr_sink_crc kms_rmfb kms_sink_crc_basic testdisplay",
Is the capital K in kms_cursor_crc intentional?

I think adding kms_rotation_crc and kms_universal_plane would be good
too, but since we're already adding most of the kms tests, why don't we
just add all kms tests?
> +"exclude_tests" : "",
> +"target_rate" : 90
> +
> +
>  }

smime.p7s
Description: S/MIME cryptographic signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t rfc 01/29] lib/igt_debugfs: Prevent buffer overflow

[Lankhorst, Maarten]

Robert Foss schreef op do 12-01-2017 om 11:30 [-0500]:
> 
> On 2017-01-12 04:14 AM, Lankhorst, Maarten wrote:
> > 
> > Robert Foss schreef op wo 11-01-2017 om 15:41 [-0500]:
> > > 
> > > buf array may overflow with when writing '\0' if
> > > MAX_LINE_LEN bytes are read during read().
> > How?
> > 
> > char buf[MAX_LINE_LEN + 1];
> 
> I actually missed the + 1, but parts of the commit are still
> relevant 
> though, as the errno at least in theory could be != EAGAIN.
> 
> So I'd like to keep the below check, to prevent compiler warnings.
> if (bytes_read < 0)
> 
> Sounds ok?
Yes. :)
> 
> Rob.
> > 
> > 
> > > 
> > > Signed-off-by: Robert Foss <robert.f...@collabora.com>
> > > ---
> > >  lib/igt_debugfs.c | 8 +---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> > > index d828687a..8b8a627a 100644
> > > --- a/lib/igt_debugfs.c
> > > +++ b/lib/igt_debugfs.c
> > > @@ -594,13 +594,15 @@ static int read_crc(igt_pipe_crc_t
> > > *pipe_crc,
> > > igt_crc_t *out)
> > >   read_len = MAX_LINE_LEN;
> > > 
> > >   igt_set_timeout(5, "CRC reading");
> > > - bytes_read = read(pipe_crc->crc_fd, , read_len);
> > > + bytes_read = read(pipe_crc->crc_fd, , read_len - 1);
> > >   igt_reset_timeout();
> > > 
> > > - if (bytes_read < 0 && errno == EAGAIN) {
> > > + if (bytes_read < 0 && errno == EAGAIN)
> > >   igt_assert(pipe_crc->flags & O_NONBLOCK);
> > > +
> > > + if (bytes_read < 0)
> > >   bytes_read = 0;
> > > - }
> > > +
> > >   buf[bytes_read] = '\0';
> > > 
> > >   if (bytes_read && !pipe_crc_init_from_string(pipe_crc,
> > > out,
> > > buf))
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH i-g-t rfc 01/29] lib/igt_debugfs: Prevent buffer overflow

[Lankhorst, Maarten]

Robert Foss schreef op wo 11-01-2017 om 15:41 [-0500]:
> buf array may overflow with when writing '\0' if
> MAX_LINE_LEN bytes are read during read().
How?

char buf[MAX_LINE_LEN + 1];

> Signed-off-by: Robert Foss 
> ---
>  lib/igt_debugfs.c | 8 +---
>  1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_debugfs.c b/lib/igt_debugfs.c
> index d828687a..8b8a627a 100644
> --- a/lib/igt_debugfs.c
> +++ b/lib/igt_debugfs.c
> @@ -594,13 +594,15 @@ static int read_crc(igt_pipe_crc_t *pipe_crc,
> igt_crc_t *out)
>   read_len = MAX_LINE_LEN;
>  
>   igt_set_timeout(5, "CRC reading");
> - bytes_read = read(pipe_crc->crc_fd, , read_len);
> + bytes_read = read(pipe_crc->crc_fd, , read_len - 1);
>   igt_reset_timeout();
>  
> - if (bytes_read < 0 && errno == EAGAIN) {
> + if (bytes_read < 0 && errno == EAGAIN)
>   igt_assert(pipe_crc->flags & O_NONBLOCK);
> +
> + if (bytes_read < 0)
>   bytes_read = 0;
> - }
> +
>   buf[bytes_read] = '\0';
>  
>   if (bytes_read && !pipe_crc_init_from_string(pipe_crc, out,
> buf))
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v6 2/8] drm/i915/bxt: IPC WA for Broxton

[Lankhorst, Maarten]

Hey,

Mahesh Kumar schreef op ma 28-11-2016 om 18:37 [+0530]:
> Hi,
> 
> Will keep WA number in commit message/WA location.
> thanks,

Sounds good, with that fixed patches 1-5 and 7 look good to me.
I think patch 6 will no longer be required since the workaround status
will also be kept inside intel_state.

~Maarten
-
Intel International B.V.
Registered in The Netherlands under number 34098535
Statutory seat: Haarlemmermeer
Registered address: Capronilaan 37, 1119NG Schiphol-Rijk

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v6 8/8] drm/i915/gen9: WM memory bandwidth related workaround

[Lankhorst, Maarten]

Mahesh Kumar schreef op di 29-11-2016 om 19:17 [+0530]:
> 
> On Tuesday 29 November 2016 03:16 PM, Lankhorst, Maarten wrote:
> > 
> > Mahesh Kumar schreef op di 29-11-2016 om 11:12 [+0530]:
> > > 
> > > Hi,
> > > 
> > > 
> > > On Thursday 24 November 2016 06:21 PM, Lankhorst, Maarten wrote:
> > > > 
> > > > Mahesh Kumar schreef op do 24-11-2016 om 09:31 [+0530]:
> > > > > 
> > > > > This patch implemnets Workariunds related to display
> > > > > arbitrated
> > > > > memory
> > > > > bandwidth. These WA are applicabe for all gen-9 based
> > > > > platforms.
> > > > > 
> > > > > Changes since v1:
> > > > >    - Rebase on top of Paulo's patch series
> > > > > Changes since v2:
> > > > >    - Address review comments
> > > > >    - Rebase/rework as per other patch changes in series
> > > > > 
> > > > > Signed-off-by: Mahesh Kumar <mahesh1.ku...@intel.com>
> > > > > ---
> > > > >    drivers/gpu/drm/i915/i915_drv.h |   9 +++
> > > > >    drivers/gpu/drm/i915/intel_pm.c | 149
> > > > > +---
> > > > >    2 files changed, 149 insertions(+), 9 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > > > b/drivers/gpu/drm/i915/i915_drv.h
> > > > > index 4e2f17f..2b673c6 100644
> > > > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > > > @@ -1193,6 +1193,13 @@ enum intel_sbi_destination {
> > > > >       SBI_MPHY,
> > > > >    };
> > > > >    
> > > > > +/* SKL+ Watermark arbitrated display bandwidth Workarounds
> > > > > */
> > > > > +enum watermark_memory_wa {
> > > > > + WATERMARK_WA_NONE,
> > > > > + WATERMARK_WA_X_TILED,
> > > > > + WATERMARK_WA_Y_TILED,
> > > > > +};
> > > > > +
> > > > >    #define QUIRK_PIPEA_FORCE (1<<0)
> > > > >    #define QUIRK_LVDS_SSC_DISABLE (1<<1)
> > > > >    #define QUIRK_INVERT_BRIGHTNESS (1<<2)
> > > > > @@ -1764,6 +1771,8 @@ struct skl_ddb_allocation {
> > > > >    
> > > > >    struct skl_wm_values {
> > > > >       unsigned dirty_pipes;
> > > > > + /* any WaterMark memory workaround Required */
> > > > > + enum watermark_memory_wa mem_wa;
> > > > >       struct skl_ddb_allocation ddb;
> > > > >    };
> > > > >    
> > > > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > > > b/drivers/gpu/drm/i915/intel_pm.c
> > > > > index 3e2dd8f..547bbda 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > > > @@ -2889,11 +2889,7 @@ skl_wm_plane_id(const struct
> > > > > intel_plane
> > > > > *plane)
> > > > >       }
> > > > >    }
> > > > >    
> > > > > -/*
> > > > > - * FIXME: We still don't have the proper code detect if we
> > > > > need
> > > > > to
> > > > > apply the WA,
> > > > > - * so assume we'll always need it in order to avoid
> > > > > underruns.
> > > > > - */
> > > > > -static bool skl_needs_memory_bw_wa(struct intel_atomic_state
> > > > > *state)
> > > > > +static bool intel_needs_memory_bw_wa(struct
> > > > > intel_atomic_state
> > > > > *state)
> > > > >    {
> > > > >       struct drm_i915_private *dev_priv = to_i915(state-
> > > > > > 
> > > > > > base.dev);
> > > > >    
> > > > > @@ -3067,7 +3063,7 @@ bool intel_can_enable_sagv(struct
> > > > > drm_atomic_state *state)
> > > > >    
> > > > >       latency = dev_priv->wm.skl_latency[level];
> > > > >    
> > > > > - if (skl_needs_memory_bw_wa(intel_state) &&
> > > > > + if (intel_needs_memory_bw_wa(intel_state) &&
> > > > >       plane->base.stat

Re: [Intel-gfx] [PATCH v6 8/8] drm/i915/gen9: WM memory bandwidth related workaround

[Lankhorst, Maarten]

Mahesh Kumar schreef op di 29-11-2016 om 11:12 [+0530]:
> Hi,
> 
> 
> On Thursday 24 November 2016 06:21 PM, Lankhorst, Maarten wrote:
> > 
> > Mahesh Kumar schreef op do 24-11-2016 om 09:31 [+0530]:
> > > 
> > > This patch implemnets Workariunds related to display arbitrated
> > > memory
> > > bandwidth. These WA are applicabe for all gen-9 based platforms.
> > > 
> > > Changes since v1:
> > >   - Rebase on top of Paulo's patch series
> > > Changes since v2:
> > >   - Address review comments
> > >   - Rebase/rework as per other patch changes in series
> > > 
> > > Signed-off-by: Mahesh Kumar <mahesh1.ku...@intel.com>
> > > ---
> > >   drivers/gpu/drm/i915/i915_drv.h |   9 +++
> > >   drivers/gpu/drm/i915/intel_pm.c | 149
> > > +---
> > >   2 files changed, 149 insertions(+), 9 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > > b/drivers/gpu/drm/i915/i915_drv.h
> > > index 4e2f17f..2b673c6 100644
> > > --- a/drivers/gpu/drm/i915/i915_drv.h
> > > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > > @@ -1193,6 +1193,13 @@ enum intel_sbi_destination {
> > >   SBI_MPHY,
> > >   };
> > >   
> > > +/* SKL+ Watermark arbitrated display bandwidth Workarounds */
> > > +enum watermark_memory_wa {
> > > + WATERMARK_WA_NONE,
> > > + WATERMARK_WA_X_TILED,
> > > + WATERMARK_WA_Y_TILED,
> > > +};
> > > +
> > >   #define QUIRK_PIPEA_FORCE (1<<0)
> > >   #define QUIRK_LVDS_SSC_DISABLE (1<<1)
> > >   #define QUIRK_INVERT_BRIGHTNESS (1<<2)
> > > @@ -1764,6 +1771,8 @@ struct skl_ddb_allocation {
> > >   
> > >   struct skl_wm_values {
> > >   unsigned dirty_pipes;
> > > + /* any WaterMark memory workaround Required */
> > > + enum watermark_memory_wa mem_wa;
> > >   struct skl_ddb_allocation ddb;
> > >   };
> > >   
> > > diff --git a/drivers/gpu/drm/i915/intel_pm.c
> > > b/drivers/gpu/drm/i915/intel_pm.c
> > > index 3e2dd8f..547bbda 100644
> > > --- a/drivers/gpu/drm/i915/intel_pm.c
> > > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > > @@ -2889,11 +2889,7 @@ skl_wm_plane_id(const struct intel_plane
> > > *plane)
> > >   }
> > >   }
> > >   
> > > -/*
> > > - * FIXME: We still don't have the proper code detect if we need
> > > to
> > > apply the WA,
> > > - * so assume we'll always need it in order to avoid underruns.
> > > - */
> > > -static bool skl_needs_memory_bw_wa(struct intel_atomic_state
> > > *state)
> > > +static bool intel_needs_memory_bw_wa(struct intel_atomic_state
> > > *state)
> > >   {
> > >   struct drm_i915_private *dev_priv = to_i915(state-
> > > > 
> > > > base.dev);
> > >   
> > > @@ -3067,7 +3063,7 @@ bool intel_can_enable_sagv(struct
> > > drm_atomic_state *state)
> > >   
> > >   latency = dev_priv->wm.skl_latency[level];
> > >   
> > > - if (skl_needs_memory_bw_wa(intel_state) &&
> > > + if (intel_needs_memory_bw_wa(intel_state) &&
> > >   plane->base.state->fb->modifier ==
> > >   I915_FORMAT_MOD_X_TILED)
> > >   latency += 15;
> > > @@ -3597,7 +3593,7 @@ static int skl_compute_plane_wm(const
> > > struct
> > > drm_i915_private *dev_priv,
> > >   uint32_t y_min_scanlines;
> > >   struct intel_atomic_state *state =
> > >   to_intel_atomic_state(cstate->base.state);
> > > - bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
> > > + enum watermark_memory_wa mem_wa;
> > >   bool y_tiled, x_tiled;
> > >   
> > >   if (latency == 0 || !cstate->base.active ||
> > > !intel_pstate-
> > > > 
> > > > base.visible) {
> > > @@ -3613,7 +3609,8 @@ static int skl_compute_plane_wm(const
> > > struct
> > > drm_i915_private *dev_priv,
> > >   if (IS_KABYLAKE(dev_priv) && dev_priv->ipc_enabled)
> > >   latency += 4;
> > >   
> > > - if (apply_memory_bw_wa && x_tiled)
> > > + mem_wa = state->wm_results.mem_wa;
> > > + if (mem_

Re: [Intel-gfx] [PATCH v6 8/8] drm/i915/gen9: WM memory bandwidth related workaround

[Lankhorst, Maarten]

Mahesh Kumar schreef op do 24-11-2016 om 09:31 [+0530]:
> This patch implemnets Workariunds related to display arbitrated
> memory
> bandwidth. These WA are applicabe for all gen-9 based platforms.
> 
> Changes since v1:
>  - Rebase on top of Paulo's patch series
> Changes since v2:
>  - Address review comments
>  - Rebase/rework as per other patch changes in series
> 
> Signed-off-by: Mahesh Kumar 
> ---
>  drivers/gpu/drm/i915/i915_drv.h |   9 +++
>  drivers/gpu/drm/i915/intel_pm.c | 149
> +---
>  2 files changed, 149 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 4e2f17f..2b673c6 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1193,6 +1193,13 @@ enum intel_sbi_destination {
>   SBI_MPHY,
>  };
>  
> +/* SKL+ Watermark arbitrated display bandwidth Workarounds */
> +enum watermark_memory_wa {
> + WATERMARK_WA_NONE,
> + WATERMARK_WA_X_TILED,
> + WATERMARK_WA_Y_TILED,
> +};
> +
>  #define QUIRK_PIPEA_FORCE (1<<0)
>  #define QUIRK_LVDS_SSC_DISABLE (1<<1)
>  #define QUIRK_INVERT_BRIGHTNESS (1<<2)
> @@ -1764,6 +1771,8 @@ struct skl_ddb_allocation {
>  
>  struct skl_wm_values {
>   unsigned dirty_pipes;
> + /* any WaterMark memory workaround Required */
> + enum watermark_memory_wa mem_wa;
>   struct skl_ddb_allocation ddb;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 3e2dd8f..547bbda 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2889,11 +2889,7 @@ skl_wm_plane_id(const struct intel_plane
> *plane)
>   }
>  }
>  
> -/*
> - * FIXME: We still don't have the proper code detect if we need to
> apply the WA,
> - * so assume we'll always need it in order to avoid underruns.
> - */
> -static bool skl_needs_memory_bw_wa(struct intel_atomic_state *state)
> +static bool intel_needs_memory_bw_wa(struct intel_atomic_state
> *state)
>  {
>   struct drm_i915_private *dev_priv = to_i915(state-
> >base.dev);
>  
> @@ -3067,7 +3063,7 @@ bool intel_can_enable_sagv(struct
> drm_atomic_state *state)
>  
>   latency = dev_priv->wm.skl_latency[level];
>  
> - if (skl_needs_memory_bw_wa(intel_state) &&
> + if (intel_needs_memory_bw_wa(intel_state) &&
>   plane->base.state->fb->modifier ==
>   I915_FORMAT_MOD_X_TILED)
>   latency += 15;
> @@ -3597,7 +3593,7 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>   uint32_t y_min_scanlines;
>   struct intel_atomic_state *state =
>   to_intel_atomic_state(cstate->base.state);
> - bool apply_memory_bw_wa = skl_needs_memory_bw_wa(state);
> + enum watermark_memory_wa mem_wa;
>   bool y_tiled, x_tiled;
>  
>   if (latency == 0 || !cstate->base.active || !intel_pstate-
> >base.visible) {
> @@ -3613,7 +3609,8 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>   if (IS_KABYLAKE(dev_priv) && dev_priv->ipc_enabled)
>   latency += 4;
>  
> - if (apply_memory_bw_wa && x_tiled)
> + mem_wa = state->wm_results.mem_wa;
> + if (mem_wa != WATERMARK_WA_NONE && x_tiled)
>   latency += 15;
>  
>   width = drm_rect_width(_pstate->base.src) >> 16;
> @@ -3648,7 +3645,7 @@ static int skl_compute_plane_wm(const struct
> drm_i915_private *dev_priv,
>   y_min_scanlines = 4;
>   }
>  
> - if (apply_memory_bw_wa)
> + if (mem_wa == WATERMARK_WA_Y_TILED)
>   y_min_scanlines *= 2;
>  
>   plane_bytes_per_line = width * cpp;
> @@ -4077,6 +4074,15 @@ skl_compute_ddb(struct drm_atomic_state
> *state)
>   }
>  
>   /*
> +  * If Watermark workaround is changed we need to recalculate
> +  * watermark values for all active pipes
> +  */
> + if (intel_state->wm_results.mem_wa != dev_priv-
> >wm.skl_hw.mem_wa) {
> + realloc_pipes = ~0;
> + intel_state->wm_results.dirty_pipes = ~0;
> + }
> +
> + /*
>    * We're not recomputing for the pipes not included in the
> commit, so
>    * make sure we start with the current state.
>    */
> @@ -4102,6 +4108,129 @@ skl_compute_ddb(struct drm_atomic_state
> *state)
>  }
>  
>  static void
> +skl_compute_memory_bandwidth_wm_wa(struct drm_atomic_state *state)
> +{
> + struct drm_device *dev = state->dev;
> + struct drm_i915_private *dev_priv = to_i915(dev);
> + struct intel_crtc *intel_crtc;
> + struct intel_plane_state *intel_pstate;
> + struct intel_atomic_state *intel_state =
> to_intel_atomic_state(state);
> + struct memdev_info *memdev_info = _priv->memdev_info;
> + int num_active_pipes;
> + uint32_t max_pipe_bw_kbps, total_pipe_bw_kbps;
> + int display_bw_percentage;
> + bool y_tile_enabled = false;
> 

Re: [Intel-gfx] [PATCH v6 2/8] drm/i915/bxt: IPC WA for Broxton

[Lankhorst, Maarten]

Mahesh Kumar schreef op do 24-11-2016 om 09:31 [+0530]:
> If IPC is enabled in BXT, display underruns are observed.
> WA: The Line Time programmed in the WM_LINETIME register should be
> half of the actual calculated Line Time.
> 
> Programmed Line Time = 1/2*Calculated Line Time
> 
> Signed-off-by: Mahesh Kumar 
> Reviewed-by: Paulo Zanoni 
> ---
>  drivers/gpu/drm/i915/i915_drv.c |  2 ++
>  drivers/gpu/drm/i915/i915_drv.h |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c | 11 +--
>  3 files changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
> index 445fec9..1b0a589 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1243,6 +1243,8 @@ int i915_driver_load(struct pci_dev *pdev,
> const struct pci_device_id *ent)
>  
>   intel_runtime_pm_enable(dev_priv);
>  
> + dev_priv->ipc_enabled = false;
> +
>   /* Everything is in place, we can now relax! */
>   DRM_INFO("Initialized %s %d.%d.%d %s for %s on minor %d\n",
>    driver.name, driver.major, driver.minor,
> driver.patchlevel,
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
> index 63c0ea0..6691a4e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2128,6 +2128,8 @@ struct drm_i915_private {
>   /* perform PHY state sanity checks? */
>   bool chv_phy_assert[2];
>  
> + bool ipc_enabled;
> +
>   /* Used to save the pipe-to-encoder mapping for audio */
>   struct intel_encoder *av_enc_map[I915_MAX_PIPES];
>  
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index 8908736..7090a7c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3769,7 +3769,10 @@ skl_compute_wm_level(const struct
> drm_i915_private *dev_priv,
>  static uint32_t
>  skl_compute_linetime_wm(struct intel_crtc_state *cstate)
>  {
> + struct drm_atomic_state *state = cstate->base.state;
> + struct drm_i915_private *dev_priv = to_i915(state->dev);
>   uint32_t pixel_rate;
> + uint32_t linetime_wm;
>  
>   if (!cstate->base.active)
>   return 0;
> @@ -3779,8 +3782,12 @@ skl_compute_linetime_wm(struct
> intel_crtc_state *cstate)
>   if (WARN_ON(pixel_rate == 0))
>   return 0;
>  
> - return DIV_ROUND_UP(8 * cstate-
> >base.adjusted_mode.crtc_htotal * 1000,
> - pixel_rate);
> + linetime_wm = DIV_ROUND_UP(8 * cstate-
> >base.adjusted_mode.crtc_htotal *
> + 1000, pixel_rate);
> + if (IS_BROXTON(dev_priv) && dev_priv->ipc_enabled)
> + linetime_wm = DIV_ROUND_UP(linetime_wm, 2);
I've asked on irc to be sure, but this needs slightly more info in the
code as well. Same for 3/8 and 8/8.

12:47 < mlankhorst> danvet: if we add hw workarounds that don't have a
name, do we still have to write comments about them in the code?
12:48 < danvet> at least a bspec reference
12:48 < danvet> some hint to make it possible to find where it's from
12:49 < danvet> e.g. mesa has traditionally just pasted the entire
bspec paragraph
12:49 < mlankhorst> ok
12:49 < danvet> we've done that too in the past, but then the wa db
mostly made that redundant
12:49 < danvet> but if there's no wa entry, then full deal
12:49 < danvet> also, maybe a bspec bug notice to pls add one
12:49 < danvet> you should be able to do that over the web, and afaik
there should be one for every w/a
-
Intel International B.V.
Registered in The Netherlands under number 34098535
Statutory seat: Haarlemmermeer
Registered address: Capronilaan 37, 1119NG Schiphol-Rijk

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH v5 09/35] drm/i915: Force MMIO flips when scheduler enabled

[Lankhorst, Maarten]

Hey,


Jesse Barnes schreef op vr 19-02-2016 om 12:01 [-0800]:
> On 02/19/2016 11:53 AM, Ville Syrjälä wrote:
> > On Fri, Feb 19, 2016 at 11:28:05AM -0800, Jesse Barnes wrote:
> >> On 02/18/2016 06:26 AM, john.c.harri...@intel.com wrote:
> >>> From: John Harrison 
> >>>
> >>> MMIO flips are the preferred mechanism now but more importantly, pipe
> >>> based flips cause issues for the scheduler. Specifically, submitting
> >>> work to the rings around the side of the scheduler could cause that
> >>> work to be lost if the scheduler generates a pre-emption event on that
> >>> ring.
> >>>
> >>> For: VIZ-1587
> >>> Signed-off-by: John Harrison 
> >>> ---
> >>>  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 6e12ed7..731d20a 100644
> >>> --- a/drivers/gpu/drm/i915/intel_display.c
> >>> +++ b/drivers/gpu/drm/i915/intel_display.c
> >>> @@ -46,6 +46,7 @@
> >>>  #include 
> >>>  #include 
> >>>  #include 
> >>> +#include "i915_scheduler.h"
> >>>  
> >>>  /* Primary plane formats for gen <= 3 */
> >>>  static const uint32_t i8xx_primary_formats[] = {
> >>> @@ -11330,6 +11331,8 @@ static bool use_mmio_flip(struct intel_engine_cs 
> >>> *ring,
> >>>   return true;
> >>>   else if (i915.enable_execlists)
> >>>   return true;
> >>> + else if (i915_scheduler_is_enabled(ring->dev))
> >>> + return true;
> >>>   else if (obj->base.dma_buf &&
> >>>!reservation_object_test_signaled_rcu(obj->base.dma_buf->resv,
> >>>  false))
> >>>
> >>
> >> Why can't we use mmio flips unconditionally?  Maarten or Ville?
> > 
> > We do when execlists are used, which is always on gen9+. So I guess I'm
> > missing the point of this patch. For gen5+ we could also do it trivially.
> 
> didn't check if the scheduler is also enabled for gen8 (I guess it would be 
> nice, that would cover BDW and BSW).

I have a patch that would enable support for mmio flips on all
platforms, but because of Chris Wilson's objections I still didn't force
mmio flips by default.

> > 
> > For older platforms it'd require a bit of work since we'd need to
> > complete the flips from the vblank interrupt. Well, we actually do
> > that already even with CS flips on those platforms, but we do look
> > at the flip pending interrupt to figure out if CS already issued
> > the flip or not. So that part would need changing.
It was easy to fix in a way similar to that.

> > I also think we should switch to using the vblank interrupt for this
> > stuff on all platforms, mainly since the flip done interrupt is somewhat
> > broken on at least BDW (no idea if it got fixed in SKL or later), and
> > doing things in more than one way certainly won't decrease our bug count.
> 
> Yeah that's probably the way to go; I haven't checked the behavior on SKL 
> either.

-
Intel International B.V.
Registered in The Netherlands under number 34098535
Statutory seat: Haarlemmermeer
Registered address: Capronilaan 37, 1119NG Schiphol-Rijk

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915: Reset dpll_hw_state when selecting a new pll on hsw

[Lankhorst, Maarten]

Hey,


Gabriel Feceoru schreef op wo 11-11-2015 om 20:27 [+0200]:
> 
> On 11.11.2015 16:21, Jani Nikula wrote:
> > On Wed, 11 Nov 2015, Ander Conselvan De Oliveira  
> > wrote:
> >> On Tue, 2015-11-10 at 14:53 +0200, Jani Nikula wrote:
> >>> Ander, Maarten, where are we with this? Is it horribly wrong to merge
> >>> the original patch in this ever-growing and diverging thread?
> >>
> >> I think the patch as is will cause problems with DP, since we might clear 
> >> the
> >> pll selection made in hsw_dp_set_ddi_pll_sel(). I think the easy fix
> >> disregarding the discussion in this thread is to drop another memset in
> >>   intel_crt_compute_config(). Like this
> >
> >
> > Ander, please post this as a proper patch for review.
> >
> > BR,
> > Jani.
> 
> Hi,
> I tested this patch on my system and I can confirm it fixes the original 
> issue.
> However there are a few memset in *_ddi_pll_select functions which might 
> not be needed anymore. For instance I tried to remove the hsw one and 
> didn't see any regression.

Could you test
http://lists.freedesktop.org/archives/intel-gfx/2015-September/075964.html
?

Should be a less duct-tape fix..

~Maarten
-
Intel International B.V.
Registered in The Netherlands under number 34098535
Statutory seat: Haarlemmermeer
Registered address: Capronilaan 37, 1119NG Schiphol-Rijk

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [regression] [git pull] drm for 4.3

[Lankhorst, Maarten]

Hey,

Dave Jones schreef op di 22-09-2015 om 21:49 [-0400]:
> On Tue, Sep 22, 2015 at 09:15:58AM -0700, Matt Roper wrote:
>  > On Tue, Sep 22, 2015 at 05:13:55PM +0200, Daniel Vetter wrote:
>  > > On Tue, Sep 22, 2015 at 08:00:17AM -0700, Jesse Barnes wrote:
>  > > > Cc'ing Maarten and Matt; I'm guessing this may be related to one of
>  > > > their recent patches.
>  > 
>  > Sounds like this showed up before my recent work, but I think I might
>  > have seen similar problems while working on atomic watermarks; the
>  > issues I was seeing were because the initial hardware readout could
>  > leave primary->visible set to true even when the CRTC was off.  My
>  > series (which is still under development) contains this patch to fix
>  > that:
>  > 
>  > http://patchwork.freedesktop.org/patch/59564/
>  > 
>  > Does applying that help with the problems reported here?
> 
> No difference at all for me.
Looks like a (reopened) dup of 91952?

Can you apply "[PATCH] drm/i915: Add primary plane to mask if it's
visible", and get me the results?

~Maarten
-
Intel International B.V.
Registered in The Netherlands under number 34098535
Statutory seat: Haarlemmermeer
Registered address: Capronilaan 37, 1119NG Schiphol-Rijk

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx