From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Roper Subject: Re: [PATCH 02/11] drm/i915: Register pipe level color properties Date: Thu, 24 Jul 2014 17:02:53 -0700 Message-ID: <20140725000253.GD1618@intel.com> References: <1406138705-17334-1-git-send-email-shashank.sharma@intel.com> <1406138705-17334-3-git-send-email-shashank.sharma@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga02.intel.com (mga02.intel.com [134.134.136.20]) by gabe.freedesktop.org (Postfix) with ESMTP id E88D6898F5 for ; Thu, 24 Jul 2014 17:01:55 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1406138705-17334-3-git-send-email-shashank.sharma@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: shashank.sharma@intel.com Cc: intel-gfx@lists.freedesktop.org, daniel.vetter@intel.com List-Id: intel-gfx@lists.freedesktop.org On Wed, Jul 23, 2014 at 11:34:56PM +0530, shashank.sharma@intel.com wrote: > From: Shashank Sharma > > In valleyview we have two pipe level color correction > properties: > 1. CSC correction (wide gamut) > 2. Gamma correction > > What this patch does: > 1. This patch adds software infrastructure to register pipe level > color correction properties per CRTC. Adding a new function, > intel_attach_pipe_color_correction to register the pipe level > color correction properties with the given CRTC. > 2. Adding a pointer in intel_crtc structure to store this property. > 3. Adding structure gen6_pipe_color_corrections, which contains different > pipe level correction values for VLV. > > Signed-off-by: Shashank Sharma ...snip... > +struct drm_property *intel_clrmgr_register(struct drm_device *dev, > + struct drm_mode_object *obj, struct clrmgr_property *cp) > +{ > + struct drm_property *property; > + > + /* Create drm property */ > + switch (cp->type) { > + case DRM_MODE_PROP_BLOB: > + property = drm_property_create(dev, DRM_MODE_PROP_BLOB, > + cp->name, cp->len); > + if (!property) { > + DRM_ERROR("Failed to create property %s\n", cp->name); > + goto error; > + } > + break; > + > + case DRM_MODE_PROP_RANGE: > + property = drm_property_create_range(dev, DRM_MODE_PROP_RANGE, > + cp->name, cp->min, cp->max); > + if (!property) { > + DRM_ERROR("Failed to create property %s\n", cp->name); > + goto error; > + } > + break; > + > + default: > + DRM_ERROR("Unsupported type for property %s\n", cp->name); > + goto error; > + } > + /* Attach property to object */ > + drm_object_attach_property(obj, property, 0); > + DRM_DEBUG_DRIVER("Registered property %s\n", property->name); > + return property; > + > +error: > + DRM_ERROR("Failed to create property %s\n", cp->name); > + return NULL; > +} If I'm reading this right, you're creating a unique property for each DRM object (crtc in this case) that you work with. I.e., if you have three pipes, then you wind up creating three CSC and three gamma properties total. This isn't really the way properties are meant to be used... A property describes a type of data that you want to store on a per-object basis, but doesn't actually store anything itself. You can then attach that single property to multiple objects and all of those objects will track their own value of the quantity described by the property. It's sort of like the difference between classes and objects in OOP --- a DRM property is sort of like a class (just describes something you plan to store) and attaching that property to a CRTC (or plane, or connector) is like instantiating a new object of the class. You should really only have two properties being created by the driver here...one for CSC and one for Gamma. Once you've created a property once, you'll attach it to all of your CRTC's and they'll all manage their own values. Matt -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795