On Wed, Sep 23, 2015 at 01:52:21PM +0530, Sharma, Shashank wrote: > Regards > Shashank > > On 9/22/2015 6:45 PM, Daniel Vetter wrote: > >On Wed, Sep 16, 2015 at 11:07:07PM +0530, Shashank Sharma wrote: > >>I915 driver registers gamma correction as palette correction > >>property with DRM layer. This patch adds set_property() and get_property() > >>handlers for pipe level gamma correction. > >> > >>The set function attaches the Gamma correction blob to CRTC state, these > >>values will be committed during atomic commit. > >> > >>Signed-off-by: Shashank Sharma <shashank.sha...@intel.com> > >>Signed-off-by: Kausal Malladi <kausalmall...@gmail.com> > >>--- > >> drivers/gpu/drm/i915/intel_atomic.c | 20 ++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_color_manager.c | 21 +++++++++++++++++++++ > >> drivers/gpu/drm/i915/intel_drv.h | 5 +++++ > >> 3 files changed, 46 insertions(+) > >> > >>diff --git a/drivers/gpu/drm/i915/intel_atomic.c > >>b/drivers/gpu/drm/i915/intel_atomic.c > >>index 500d2998..0b61fef 100644 > >>--- a/drivers/gpu/drm/i915/intel_atomic.c > >>+++ b/drivers/gpu/drm/i915/intel_atomic.c > >>@@ -315,6 +315,13 @@ int intel_crtc_atomic_set_property(struct drm_crtc > >>*crtc, > >> struct drm_property *property, > >> uint64_t val) > >> { > >>+ struct drm_device *dev = crtc->dev; > >>+ struct drm_mode_config *config = &dev->mode_config; > >>+ > >>+ if (property == config->cm_palette_after_ctm_property) > >>+ return intel_color_manager_set_pipe_gamma(dev, state, > >>+ &crtc->base, val); > >>+ > >> DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); > >> return -EINVAL; > >> } > >>@@ -324,6 +331,19 @@ int intel_crtc_atomic_get_property(struct drm_crtc > >>*crtc, > >> struct drm_property *property, > >> uint64_t *val) > >> { > >>+ struct drm_device *dev = crtc->dev; > >>+ struct drm_mode_config *config = &dev->mode_config; > >>+ > >>+ if (property == config->cm_palette_after_ctm_property) { > >>+ *val = (state->palette_after_ctm_blob) ? > >>+ state->palette_after_ctm_blob->base.id : 0; > >>+ goto found; > >>+ } > > > >Since color manager properties are meant as a new standardize KMS > >extension (we put them into the core drm_crtc_state) the get/set support > >should also be in the core. See e.g. how the rotation property is handled > >in drm_atomic_plane_get/set_property. So all this code should be added to > >drm_atomic_crtc_get/set_property. > Thanks, sounds like a good one. Will move this.
When moving this please don't forget to add the blob->length sanity checks mentioned in another thread in this discussion. > > > > > >>+ > >> DRM_DEBUG_KMS("Unknown crtc property '%s'\n", property->name); > >> return -EINVAL; > >>+ > >>+found: > >>+ DRM_DEBUG_KMS("Found property %s\n", property->name); > >>+ return 0; > >> } > >>diff --git a/drivers/gpu/drm/i915/intel_color_manager.c > >>b/drivers/gpu/drm/i915/intel_color_manager.c > >>index 77f58f2..9421bb6 100644 > >>--- a/drivers/gpu/drm/i915/intel_color_manager.c > >>+++ b/drivers/gpu/drm/i915/intel_color_manager.c > >>@@ -27,6 +27,27 @@ > >> > >> #include "intel_color_manager.h" > >> > >>+int intel_color_manager_set_pipe_gamma(struct drm_device *dev, > >>+ struct drm_crtc_state *crtc_state, > >>+ struct drm_mode_object *obj, uint32_t blob_id) > >>+{ > >>+ struct drm_property_blob *blob; > >>+ > >>+ blob = drm_property_lookup_blob(dev, blob_id); > >>+ if (!blob) { > >>+ DRM_DEBUG_KMS("Invalid Blob ID\n"); > >>+ return -EINVAL; > >>+ } > >>+ > >>+ if (crtc_state->palette_after_ctm_blob) > >>+ drm_property_unreference_blob( > >>+ crtc_state->palette_after_ctm_blob); > >>+ > >>+ /* Attach the blob to be committed in state */ > >>+ crtc_state->palette_after_ctm_blob = blob; > >>+ return 0; > >>+} > > > >What is this used for? It looks a bit like legacy property code, and we > >have a generic helper to make that happen > >(drm_atomic_helper_crtc_set_property). > > > No Daniel, its not the legacy part. Please check atomic_begin() part in the > next patches. This is like the top half of atomic set_property for CRTC and > will be applicable for any property. As you already know, to set a property: > - userspace creates a blob, and sends the blob id via set_property() > interface > - set_property() saves the blob_id in the corresponding pointer space in the > crtc_state() > - From the atomic_commit_begin() we will extract the values from blob and > start committing to the registers. Its kind of bottom_half() of atomic > set_property for CRTC. > >Generally please don't add functions/structs without also adding a user, > >it means that reviewers have to constantly jump around in your patch > >series to figure out how something is used. Instead if you want to split > >things up really fine add a stub function frist (but including relevant > >callers) and then fill out the bits separately. > I think the above explanation should make it clear. In my defence I'd like to say that I was still jetlagged ;-) Reading things now again it all looks good. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx