| Message ID | 1510056391-9684-2-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 7 November 2017 at 12:06, Uma Shankar <uma.shankar@intel.com> wrote: > Add Plane Degamma as a blob property and plane > degamma size as a range property. > > v2: Rebase > Hi Uma, seems like something has gone wrong during the rebase. > Signed-off-by: Uma Shankar <uma.shankar@intel.com> > --- > drivers/gpu/drm/drm_atomic.c | 12 ++++++++++++ > drivers/gpu/drm/drm_atomic_helper.c | 6 ++++++ > drivers/gpu/drm/drm_mode_config.c | 14 ++++++++++++++ > include/drm/drm_mode_config.h | 11 +++++++++++ > include/drm/drm_plane.h | 10 ++++++++++ > 5 files changed, 53 insertions(+) > > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 7e0e7be..30853c7 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c > @@ -713,6 +713,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > { > struct drm_device *dev = plane->dev; > struct drm_mode_config *config = &dev->mode_config; > + bool replaced = false; > + int ret; > > if (property == config->prop_fb_id) { > struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val); > @@ -758,6 +760,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > } else if (plane->funcs->atomic_set_property) { > return plane->funcs->atomic_set_property(plane, state, > property, val); > + } else if (property == config->plane_degamma_lut_property) { > + ret = drm_atomic_replace_property_blob_from_id(dev, > + &state->degamma_lut, > + val, -1, &replaced); > + state->color_mgmt_changed |= replaced; > + return ret; Namely: the driver specific atomic_set_property will be called and the newly added code will not be reached. I think we should keep the atomic_set_property call last in the if/else chain. Converting the lot to a switch statement might make things a bit more obvious. > } else { > return -EINVAL; > } > @@ -816,6 +824,9 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > *val = state->zpos; > } else if (plane->funcs->atomic_get_property) { > return plane->funcs->atomic_get_property(plane, state, property, val); > + } else if (property == config->plane_degamma_lut_property) { > + *val = (state->degamma_lut) ? > + state->degamma_lut->base.id : 0; Analogous thing happens here. Did you test the updated series through IGT - it should have caught the above (considering we have tests, and I'm not loosing my marbles). Same comments apply for CTM and gamma, patches 2 and 3 respectively. HTH Emil
Hi, On Tue, Nov 07, 2017 at 05:36:25PM +0530, Uma Shankar wrote: >Add Plane Degamma as a blob property and plane >degamma size as a range property. > >v2: Rebase > >Signed-off-by: Uma Shankar <uma.shankar@intel.com> >--- > drivers/gpu/drm/drm_atomic.c | 12 ++++++++++++ > drivers/gpu/drm/drm_atomic_helper.c | 6 ++++++ > drivers/gpu/drm/drm_mode_config.c | 14 ++++++++++++++ > include/drm/drm_mode_config.h | 11 +++++++++++ > include/drm/drm_plane.h | 10 ++++++++++ > 5 files changed, 53 insertions(+) > >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c >index 7e0e7be..30853c7 100644 >--- a/drivers/gpu/drm/drm_atomic.c >+++ b/drivers/gpu/drm/drm_atomic.c >@@ -713,6 +713,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > { > struct drm_device *dev = plane->dev; > struct drm_mode_config *config = &dev->mode_config; >+ bool replaced = false; >+ int ret; > > if (property == config->prop_fb_id) { > struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val); >@@ -758,6 +760,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > } else if (plane->funcs->atomic_set_property) { > return plane->funcs->atomic_set_property(plane, state, > property, val); >+ } else if (property == config->plane_degamma_lut_property) { >+ ret = drm_atomic_replace_property_blob_from_id(dev, >+ &state->degamma_lut, >+ val, -1, &replaced); >+ state->color_mgmt_changed |= replaced; >+ return ret; > } else { > return -EINVAL; > } >@@ -816,6 +824,9 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, > *val = state->zpos; > } else if (plane->funcs->atomic_get_property) { > return plane->funcs->atomic_get_property(plane, state, property, val); >+ } else if (property == config->plane_degamma_lut_property) { >+ *val = (state->degamma_lut) ? >+ state->degamma_lut->base.id : 0; > } else { > return -EINVAL; > } >@@ -953,6 +964,7 @@ static void drm_atomic_plane_print_state(struct drm_printer *p, > drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest)); > drm_printf(p, "\tsrc-pos=" DRM_RECT_FP_FMT "\n", DRM_RECT_FP_ARG(&src)); > drm_printf(p, "\trotation=%x\n", state->rotation); >+ drm_printf(p, "\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed); > > if (plane->funcs->atomic_print_state) > plane->funcs->atomic_print_state(p, state); >diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c >index 71d712f..ba924cf 100644 >--- a/drivers/gpu/drm/drm_atomic_helper.c >+++ b/drivers/gpu/drm/drm_atomic_helper.c >@@ -3395,6 +3395,10 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, > > state->fence = NULL; > state->commit = NULL; >+ >+ if (state->degamma_lut) >+ drm_property_blob_get(state->degamma_lut); >+ state->color_mgmt_changed = false; > } > EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); > >@@ -3439,6 +3443,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) > > if (state->commit) > drm_crtc_commit_put(state->commit); >+ >+ drm_property_blob_put(state->degamma_lut); > } > EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); > >diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c >index cda8bfa..118f6ac 100644 >--- a/drivers/gpu/drm/drm_mode_config.c >+++ b/drivers/gpu/drm/drm_mode_config.c >@@ -348,6 +348,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) > return -ENOMEM; > dev->mode_config.modifiers_property = prop; > >+ prop = drm_property_create(dev, >+ DRM_MODE_PROP_BLOB, >+ "PLANE_DEGAMMA_LUT", 0); >+ if (!prop) >+ return -ENOMEM; >+ dev->mode_config.plane_degamma_lut_property = prop; >+ >+ prop = drm_property_create_range(dev, >+ DRM_MODE_PROP_IMMUTABLE, >+ "PLANE_DEGAMMA_LUT_SIZE", 0, UINT_MAX); >+ if (!prop) >+ return -ENOMEM; >+ dev->mode_config.plane_degamma_lut_size_property = prop; >+ > return 0; > } > >diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h >index 0b4ac2e..6ee2df6 100644 >--- a/include/drm/drm_mode_config.h >+++ b/include/drm/drm_mode_config.h >@@ -718,6 +718,17 @@ struct drm_mode_config { > struct drm_property *gamma_lut_size_property; > > /** >+ * @plane_degamma_lut_property: Optional Plane property to set the LUT >+ * used to convert the framebuffer's colors to linear gamma. >+ */ >+ struct drm_property *plane_degamma_lut_property; >+ /** >+ * @plane_degamma_lut_size_property: Optional Plane property for the >+ * size of the degamma LUT as supported by the driver (read-only). >+ */ >+ struct drm_property *plane_degamma_lut_size_property; >+ >+ /** > * @suggested_x_property: Optional connector property with a hint for > * the position of the output on the host's screen. > */ >diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h >index 069c4c8..d112c50 100644 >--- a/include/drm/drm_plane.h >+++ b/include/drm/drm_plane.h >@@ -123,6 +123,14 @@ struct drm_plane_state { > */ > bool visible; > >+ /* @degamma_lut: >+ * >+ * Lookup table for converting framebuffer pixel data before apply the >+ * color conversion matrix @ctm. See drm_plane_enable_color_mgmt(). The >+ * blob (if not NULL) is an array of &struct drm_color_lut. >+ */ >+ struct drm_property_blob *degamma_lut; In one of the previous discussions[1] related to per-plane color management, Lionel suggested that the 16-bit color lut entries weren't enough when considering HDR. It might be worth creating a new gamma lut format with 32-bit entries for these new properties, as HDR is very much a real rather than hypothetical concern these days. Thanks, -Brian >+ > /** > * @commit: Tracks the pending commit to prevent use-after-free conditions, > * and for async plane updates. >@@ -132,6 +140,8 @@ struct drm_plane_state { > struct drm_crtc_commit *commit; > > struct drm_atomic_state *state; >+ >+ bool color_mgmt_changed : 1; > }; > > static inline struct drm_rect >-- >1.7.9.5 > >_______________________________________________ >Intel-gfx mailing list >Intel-gfx@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Tue, Nov 07, 2017 at 05:49:56PM +0000, Brian Starkey wrote: > >In one of the previous discussions[1] related to per-plane color >management, Lionel suggested that the 16-bit color lut entries weren't >enough when considering HDR. > >It might be worth creating a new gamma lut format with 32-bit entries >for these new properties, as HDR is very much a real rather than >hypothetical concern these days. > >Thanks, >-Brian Sorry, failed to paste the link: [1] https://patchwork.kernel.org/patch/9546905/
>-----Original Message----- >From: Brian Starkey [mailto:brian.starkey@arm.com] >Sent: Tuesday, November 7, 2017 11:40 PM >To: Shankar, Uma <uma.shankar@intel.com> >Cc: intel-gfx@lists.freedesktop.org; Syrjala, Ville <ville.syrjala@intel.com>; >Lankhorst, Maarten <maarten.lankhorst@intel.com>; dri- >devel@lists.freedesktop.org >Subject: Re: [Intel-gfx] [RFC 1/7] drm: Add Plane Degamma properties > >On Tue, Nov 07, 2017 at 05:49:56PM +0000, Brian Starkey wrote: >> >>In one of the previous discussions[1] related to per-plane color >>management, Lionel suggested that the 16-bit color lut entries weren't >>enough when considering HDR. >> >>It might be worth creating a new gamma lut format with 32-bit entries >>for these new properties, as HDR is very much a real rather than >>hypothetical concern these days. >> >>Thanks, >>-Brian > >Sorry, failed to paste the link: > >[1] https://patchwork.kernel.org/patch/9546905/ Thanks for the input Brian and sharing the link with earlier discussions around this. I will try to create a separate LUT structure with 32 bit entries which gets used for plane color features. Not sure what to do for pipe level color since we already use u16 fields. May be the same color_lut structure (we can call it color_lut2) can be used for high precision use cases like HDR. Regards, Uma Shankar
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7e0e7be..30853c7 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -713,6 +713,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, { struct drm_device *dev = plane->dev; struct drm_mode_config *config = &dev->mode_config; + bool replaced = false; + int ret; if (property == config->prop_fb_id) { struct drm_framebuffer *fb = drm_framebuffer_lookup(dev, NULL, val); @@ -758,6 +760,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, } else if (plane->funcs->atomic_set_property) { return plane->funcs->atomic_set_property(plane, state, property, val); + } else if (property == config->plane_degamma_lut_property) { + ret = drm_atomic_replace_property_blob_from_id(dev, + &state->degamma_lut, + val, -1, &replaced); + state->color_mgmt_changed |= replaced; + return ret; } else { return -EINVAL; } @@ -816,6 +824,9 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, *val = state->zpos; } else if (plane->funcs->atomic_get_property) { return plane->funcs->atomic_get_property(plane, state, property, val); + } else if (property == config->plane_degamma_lut_property) { + *val = (state->degamma_lut) ? + state->degamma_lut->base.id : 0; } else { return -EINVAL; } @@ -953,6 +964,7 @@ static void drm_atomic_plane_print_state(struct drm_printer *p, drm_printf(p, "\tcrtc-pos=" DRM_RECT_FMT "\n", DRM_RECT_ARG(&dest)); drm_printf(p, "\tsrc-pos=" DRM_RECT_FP_FMT "\n", DRM_RECT_FP_ARG(&src)); drm_printf(p, "\trotation=%x\n", state->rotation); + drm_printf(p, "\tcolor_mgmt_changed=%d\n", state->color_mgmt_changed); if (plane->funcs->atomic_print_state) plane->funcs->atomic_print_state(p, state); diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index 71d712f..ba924cf 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3395,6 +3395,10 @@ void __drm_atomic_helper_plane_duplicate_state(struct drm_plane *plane, state->fence = NULL; state->commit = NULL; + + if (state->degamma_lut) + drm_property_blob_get(state->degamma_lut); + state->color_mgmt_changed = false; } EXPORT_SYMBOL(__drm_atomic_helper_plane_duplicate_state); @@ -3439,6 +3443,8 @@ void __drm_atomic_helper_plane_destroy_state(struct drm_plane_state *state) if (state->commit) drm_crtc_commit_put(state->commit); + + drm_property_blob_put(state->degamma_lut); } EXPORT_SYMBOL(__drm_atomic_helper_plane_destroy_state); diff --git a/drivers/gpu/drm/drm_mode_config.c b/drivers/gpu/drm/drm_mode_config.c index cda8bfa..118f6ac 100644 --- a/drivers/gpu/drm/drm_mode_config.c +++ b/drivers/gpu/drm/drm_mode_config.c @@ -348,6 +348,20 @@ static int drm_mode_create_standard_properties(struct drm_device *dev) return -ENOMEM; dev->mode_config.modifiers_property = prop; + prop = drm_property_create(dev, + DRM_MODE_PROP_BLOB, + "PLANE_DEGAMMA_LUT", 0); + if (!prop) + return -ENOMEM; + dev->mode_config.plane_degamma_lut_property = prop; + + prop = drm_property_create_range(dev, + DRM_MODE_PROP_IMMUTABLE, + "PLANE_DEGAMMA_LUT_SIZE", 0, UINT_MAX); + if (!prop) + return -ENOMEM; + dev->mode_config.plane_degamma_lut_size_property = prop; + return 0; } diff --git a/include/drm/drm_mode_config.h b/include/drm/drm_mode_config.h index 0b4ac2e..6ee2df6 100644 --- a/include/drm/drm_mode_config.h +++ b/include/drm/drm_mode_config.h @@ -718,6 +718,17 @@ struct drm_mode_config { struct drm_property *gamma_lut_size_property; /** + * @plane_degamma_lut_property: Optional Plane property to set the LUT + * used to convert the framebuffer's colors to linear gamma. + */ + struct drm_property *plane_degamma_lut_property; + /** + * @plane_degamma_lut_size_property: Optional Plane property for the + * size of the degamma LUT as supported by the driver (read-only). + */ + struct drm_property *plane_degamma_lut_size_property; + + /** * @suggested_x_property: Optional connector property with a hint for * the position of the output on the host's screen. */ diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 069c4c8..d112c50 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -123,6 +123,14 @@ struct drm_plane_state { */ bool visible; + /* @degamma_lut: + * + * Lookup table for converting framebuffer pixel data before apply the + * color conversion matrix @ctm. See drm_plane_enable_color_mgmt(). The + * blob (if not NULL) is an array of &struct drm_color_lut. + */ + struct drm_property_blob *degamma_lut; + /** * @commit: Tracks the pending commit to prevent use-after-free conditions, * and for async plane updates. @@ -132,6 +140,8 @@ struct drm_plane_state { struct drm_crtc_commit *commit; struct drm_atomic_state *state; + + bool color_mgmt_changed : 1; }; static inline struct drm_rect
Add Plane Degamma as a blob property and plane degamma size as a range property. v2: Rebase Signed-off-by: Uma Shankar <uma.shankar@intel.com> --- drivers/gpu/drm/drm_atomic.c | 12 ++++++++++++ drivers/gpu/drm/drm_atomic_helper.c | 6 ++++++ drivers/gpu/drm/drm_mode_config.c | 14 ++++++++++++++ include/drm/drm_mode_config.h | 11 +++++++++++ include/drm/drm_plane.h | 10 ++++++++++ 5 files changed, 53 insertions(+)