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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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: [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: [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-devel@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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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. :)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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-...@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) {
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [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 :)
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[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.