| Message ID | 1534515531-20599-4-git-send-email-uma.shankar@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Plane Color Properties | expand |
Hi Uma, On Fri, Aug 17, 2018 at 07:48:46PM +0530, Uma Shankar wrote: > 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 <uma.shankar@intel.com> > --- > 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, > &replaced); > state->color_mgmt_changed |= replaced; > return ret; > + } else if (property == plane->ctm_property) { > + ret = drm_atomic_replace_property_blob_from_id(dev, > + &state->ctm, > + val, > + sizeof(struct drm_color_ctm), -1, > + &replaced); > + 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 > @@ -183,6 +183,14 @@ struct drm_plane_state { > struct drm_property_blob *degamma_lut; > > /** > + * @ctm: > + * > + * Color transformation matrix. See drm_plane_enable_color_mgmt(). The > + * blob (if not NULL) is a &struct drm_color_ctm. > + */ > + struct drm_property_blob *ctm; > + > + /** > * @commit: Tracks the pending commit to prevent use-after-free conditions, > * and for async plane updates. > * > @@ -698,6 +706,13 @@ struct drm_plane { > * size of the degamma LUT as supported by the driver (read-only). > */ > struct drm_property *degamma_lut_size_property; > + > + /** > + * @plane_ctm_property: Optional Plane property to set the > + * matrix used to convert colors after the lookup in the > + * degamma LUT. > + */ > + struct drm_property *ctm_property; > }; > > #define obj_to_plane(x) container_of(x, struct drm_plane, base) > -- > 1.9.1 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel Reviewed-by: Alexandru Gheorghe <alexandru-cosmin.gheorghe@arm.com>
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 <uma.shankar@intel.com> > --- > 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, > &replaced); > state->color_mgmt_changed |= replaced; > return ret; > + } else if (property == plane->ctm_property) { > + ret = drm_atomic_replace_property_blob_from_id(dev, > + &state->ctm, > + val, > + sizeof(struct > drm_color_ctm), -1, > + &replaced); > + 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 > @@ -183,6 +183,14 @@ struct drm_plane_state { > struct drm_property_blob *degamma_lut; > > /** > + * @ctm: > + * > + * Color transformation matrix. See > drm_plane_enable_color_mgmt(). The > + * blob (if not NULL) is a &struct drm_color_ctm. > + */ > + struct drm_property_blob *ctm; > + > + /** > * @commit: Tracks the pending commit to prevent use-after- > free conditions, > * and for async plane updates. > * > @@ -698,6 +706,13 @@ struct drm_plane { > * size of the degamma LUT as supported by the driver (read- > only). > */ > struct drm_property *degamma_lut_size_property; > + > + /** > + * @plane_ctm_property: Optional Plane property to set the > + * matrix used to convert colors after the lookup in the > + * degamma LUT. > + */ > + struct drm_property *ctm_property; > I'm assuming degamma is done before converting with CTM. But how does this interact with YUV formats? I'm not sure this is explicitly defined. I'm assuming R->Cr G -> Y, B = Cb for degamma tables. There's also a enum drm_color_encoding and a enum drm_color_range. Are these ignored if CTM is set? Would be good to mention at least. ~Maarten
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 <uma.shankar@intel.com> > > --- > > 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, > > &replaced); > > state->color_mgmt_changed |= replaced; > > return ret; > > + } else if (property == plane->ctm_property) { > > + ret = drm_atomic_replace_property_blob_from_id(dev, > > + &state->ctm, > > + val, > > + sizeof(struct > > drm_color_ctm), -1, > > + &replaced); > > + 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 > > @@ -183,6 +183,14 @@ struct drm_plane_state { > > struct drm_property_blob *degamma_lut; > > > > /** > > + * @ctm: > > + * > > + * Color transformation matrix. See > > drm_plane_enable_color_mgmt(). The > > + * blob (if not NULL) is a &struct drm_color_ctm. > > + */ > > + struct drm_property_blob *ctm; > > + > > + /** > > * @commit: Tracks the pending commit to prevent use-after- > > free conditions, > > * and for async plane updates. > > * > > @@ -698,6 +706,13 @@ struct drm_plane { > > * size of the degamma LUT as supported by the driver (read- > > only). > > */ > > struct drm_property *degamma_lut_size_property; > > + > > + /** > > + * @plane_ctm_property: Optional Plane property to set the > > + * matrix used to convert colors after the lookup in the > > + * degamma LUT. > > + */ > > + struct drm_property *ctm_property; > > > I'm assuming degamma is done before converting with CTM. > > But how does this interact with YUV formats? I'm not sure this is > explicitly defined. I'm assuming R->Cr G -> Y, B = Cb for degamma > tables. The pipeline is (or at least should be IMO): ycbcr->rgb -> degamma -> ctm -> gamma We should document the full pipeline (including the crtc portions) in some docs.
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 <uma.shankar@intel.com> >> > --- >> > 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, >> > &replaced); >> > state->color_mgmt_changed |= replaced; >> > return ret; >> > + } else if (property == plane->ctm_property) { >> > + ret = drm_atomic_replace_property_blob_from_id(dev, >> > + &state->ctm, >> > + val, >> > + sizeof(struct >> > drm_color_ctm), -1, >> > + &replaced); >> > + 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 >> > @@ -183,6 +183,14 @@ struct drm_plane_state { >> > struct drm_property_blob *degamma_lut; >> > >> > /** >> > + * @ctm: >> > + * >> > + * Color transformation matrix. See >> > drm_plane_enable_color_mgmt(). The >> > + * blob (if not NULL) is a &struct drm_color_ctm. >> > + */ >> > + struct drm_property_blob *ctm; >> > + >> > + /** >> > * @commit: Tracks the pending commit to prevent use-after- >> > free conditions, >> > * and for async plane updates. >> > * >> > @@ -698,6 +706,13 @@ struct drm_plane { >> > * size of the degamma LUT as supported by the driver (read- >> > only). >> > */ >> > struct drm_property *degamma_lut_size_property; >> > + >> > + /** >> > + * @plane_ctm_property: Optional Plane property to set the >> > + * matrix used to convert colors after the lookup in the >> > + * degamma LUT. >> > + */ >> > + struct drm_property *ctm_property; >> > >> I'm assuming degamma is done before converting with CTM. >> >> But how does this interact with YUV formats? I'm not sure this is >> explicitly defined. I'm assuming R->Cr G -> Y, B = Cb for degamma >> tables. > >The pipeline is (or at least should be IMO): >ycbcr->rgb -> degamma -> ctm -> gamma > That's what I'm expecting too. >We should document the full pipeline (including the crtc portions) in >some docs. I fully agree with Ville here, we've been around this loop a few times, so the full pipeline and expectations need to be captured in docs (IMO before the properties can be decided). It's slightly unfortunate that the merged ycbcr->rgb properties only describe the format of the incoming buffer, without any specification of what the driver is meant to do with that information (what conversion is meant to be performed?). Without that, this new CTM property is somewhat useless, because generic userspace can't tell what the input data to the matrix is in YUV cases - so we need to decide and document that too (or change/enhance the API). Thanks, -Brian > >-- >Ville Syrjälä >Intel >_______________________________________________ >dri-devel mailing list >dri-devel@lists.freedesktop.org >https://lists.freedesktop.org/mailman/listinfo/dri-devel
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 <uma.shankar@intel.com> > > > > --- > > > > 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, > > > > &replaced); > > > > state->color_mgmt_changed |= replaced; > > > > return ret; > > > > + } else if (property == plane->ctm_property) { > > > > + ret = > > > > drm_atomic_replace_property_blob_from_id(dev, > > > > + &state->ctm, > > > > + val, > > > > + sizeof(struct > > > > drm_color_ctm), -1, > > > > + &replaced); > > > > + 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 > > > > @@ -183,6 +183,14 @@ struct drm_plane_state { > > > > struct drm_property_blob *degamma_lut; > > > > > > > > /** > > > > + * @ctm: > > > > + * > > > > + * Color transformation matrix. See > > > > drm_plane_enable_color_mgmt(). The > > > > + * blob (if not NULL) is a &struct drm_color_ctm. > > > > + */ > > > > + struct drm_property_blob *ctm; > > > > + > > > > + /** > > > > * @commit: Tracks the pending commit to prevent use- > > > > after- > > > > free conditions, > > > > * and for async plane updates. > > > > * > > > > @@ -698,6 +706,13 @@ struct drm_plane { > > > > * size of the degamma LUT as supported by the driver > > > > (read- > > > > only). > > > > */ > > > > struct drm_property *degamma_lut_size_property; > > > > + > > > > + /** > > > > + * @plane_ctm_property: Optional Plane property to set > > > > the > > > > + * matrix used to convert colors after the lookup in > > > > the > > > > + * degamma LUT. > > > > + */ > > > > + struct drm_property *ctm_property; > > > > > > > > > > I'm assuming degamma is done before converting with CTM. > > > > > > But how does this interact with YUV formats? I'm not sure this is > > > explicitly defined. I'm assuming R->Cr G -> Y, B = Cb for degamma > > > tables. > > > > The pipeline is (or at least should be IMO): > > ycbcr->rgb -> degamma -> ctm -> gamma > > > > That's what I'm expecting too. > > > We should document the full pipeline (including the crtc portions) > > in > > some docs. > > I fully agree with Ville here, we've been around this loop a few > times, so the full pipeline and expectations need to be captured > in docs (IMO before the properties can be decided). > > It's slightly unfortunate that the merged ycbcr->rgb properties only > describe the format of the incoming buffer, without any specification > of what the driver is meant to do with that information (what > conversion is meant to be performed?). Without that, this new CTM > property is somewhat useless, because generic userspace can't tell > what the input data to the matrix is in YUV cases - so we need to > decide and document that too (or change/enhance the API). As long as it's documented I'm fine with it. :-) Also curious on how it interacts with CRTC pipeline. ~Maarten
On Wed, Aug 22, 2018 at 12:11:42PM +0100, Brian Starkey wrote: > 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 <uma.shankar@intel.com> > >> > --- > >> > 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, > >> > &replaced); > >> > state->color_mgmt_changed |= replaced; > >> > return ret; > >> > + } else if (property == plane->ctm_property) { > >> > + ret = drm_atomic_replace_property_blob_from_id(dev, > >> > + &state->ctm, > >> > + val, > >> > + sizeof(struct > >> > drm_color_ctm), -1, > >> > + &replaced); > >> > + 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 > >> > @@ -183,6 +183,14 @@ struct drm_plane_state { > >> > struct drm_property_blob *degamma_lut; > >> > > >> > /** > >> > + * @ctm: > >> > + * > >> > + * Color transformation matrix. See > >> > drm_plane_enable_color_mgmt(). The > >> > + * blob (if not NULL) is a &struct drm_color_ctm. > >> > + */ > >> > + struct drm_property_blob *ctm; > >> > + > >> > + /** > >> > * @commit: Tracks the pending commit to prevent use-after- > >> > free conditions, > >> > * and for async plane updates. > >> > * > >> > @@ -698,6 +706,13 @@ struct drm_plane { > >> > * size of the degamma LUT as supported by the driver (read- > >> > only). > >> > */ > >> > struct drm_property *degamma_lut_size_property; > >> > + > >> > + /** > >> > + * @plane_ctm_property: Optional Plane property to set the > >> > + * matrix used to convert colors after the lookup in the > >> > + * degamma LUT. > >> > + */ > >> > + struct drm_property *ctm_property; > >> > > >> I'm assuming degamma is done before converting with CTM. > >> > >> But how does this interact with YUV formats? I'm not sure this is > >> explicitly defined. I'm assuming R->Cr G -> Y, B = Cb for degamma > >> tables. > > > >The pipeline is (or at least should be IMO): > >ycbcr->rgb -> degamma -> ctm -> gamma > > > > That's what I'm expecting too. > > >We should document the full pipeline (including the crtc portions) in > >some docs. > > I fully agree with Ville here, we've been around this loop a few > times, so the full pipeline and expectations need to be captured > in docs (IMO before the properties can be decided). > > It's slightly unfortunate that the merged ycbcr->rgb properties only > describe the format of the incoming buffer, without any specification > of what the driver is meant to do with that information (what > conversion is meant to be performed?). So far the idea has been that the internal pipeline is assumed to be full range RGB. If/when someone needs something else they can try to design a new API for it. If we had tried to deal with ycbcr passthrough and whatnot when adding the encoding/range props we probably still wouldn't have landed anything. But yeah, the docs should of course explicitly state this fact.
>-----Original Message----- >From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] >Sent: Wednesday, August 22, 2018 6:36 PM >To: Brian Starkey <brian.starkey@arm.com> >Cc: Lankhorst, Maarten <maarten.lankhorst@intel.com>; >dcastagna@chromium.org; intel-gfx@lists.freedesktop.org; dri- >devel@lists.freedesktop.org; Shankar, Uma <uma.shankar@intel.com>; >seanpaul@chromium.org; Syrjala, Ville <ville.syrjala@intel.com> >Subject: Re: [Intel-gfx] [RFC v4 3/8] drm: Add Plane CTM property > >On Wed, Aug 22, 2018 at 12:11:42PM +0100, Brian Starkey wrote: >> 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 <uma.shankar@intel.com> >> >> > --- >> >> > 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, >> >> > &replaced); >> >> > state->color_mgmt_changed |= replaced; >> >> > return ret; >> >> > + } else if (property == plane->ctm_property) { >> >> > + ret = drm_atomic_replace_property_blob_from_id(dev, >> >> > + &state->ctm, >> >> > + val, >> >> > + sizeof(struct >> >> > drm_color_ctm), -1, >> >> > + &replaced); >> >> > + 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 >> >> > @@ -183,6 +183,14 @@ struct drm_plane_state { >> >> > struct drm_property_blob *degamma_lut; >> >> > >> >> > /** >> >> > + * @ctm: >> >> > + * >> >> > + * Color transformation matrix. See >> >> > drm_plane_enable_color_mgmt(). The >> >> > + * blob (if not NULL) is a &struct drm_color_ctm. >> >> > + */ >> >> > + struct drm_property_blob *ctm; >> >> > + >> >> > + /** >> >> > * @commit: Tracks the pending commit to prevent use-after- >> >> > free conditions, >> >> > * and for async plane updates. >> >> > * >> >> > @@ -698,6 +706,13 @@ struct drm_plane { >> >> > * size of the degamma LUT as supported by the driver (read- >> >> > only). >> >> > */ >> >> > struct drm_property *degamma_lut_size_property; >> >> > + >> >> > + /** >> >> > + * @plane_ctm_property: Optional Plane property to set the >> >> > + * matrix used to convert colors after the lookup in the >> >> > + * degamma LUT. >> >> > + */ >> >> > + struct drm_property *ctm_property; >> >> > >> >> I'm assuming degamma is done before converting with CTM. >> >> >> >> But how does this interact with YUV formats? I'm not sure this is >> >> explicitly defined. I'm assuming R->Cr G -> Y, B = Cb for degamma >> >> tables. >> > >> >The pipeline is (or at least should be IMO): >> >ycbcr->rgb -> degamma -> ctm -> gamma >> > >> >> That's what I'm expecting too. >> >> >We should document the full pipeline (including the crtc portions) in >> >some docs. >> >> I fully agree with Ville here, we've been around this loop a few >> times, so the full pipeline and expectations need to be captured in >> docs (IMO before the properties can be decided). >> >> It's slightly unfortunate that the merged ycbcr->rgb properties only >> describe the format of the incoming buffer, without any specification >> of what the driver is meant to do with that information (what >> conversion is meant to be performed?). > >So far the idea has been that the internal pipeline is assumed to be full range RGB. >If/when someone needs something else they can try to design a new API for it. If >we had tried to deal with ycbcr passthrough and whatnot when adding the >encoding/range props we probably still wouldn't have landed anything. > I agree, blending limited range layers with Full range is slightly tricky as of now. I believe we need to convert the Limited range to full range explicitly and then flip to display (atleast when we want all the color hardware blocks to come in play) >But yeah, the docs should of course explicitly state this fact. > From the pipeline perspective, Ville is right (ycbcr->rgb -> degamma -> ctm -> gamma ) and that's how it works. I will try to document these as part of kernel docs under color management to give more clarity and avoid any kind of ambiguity. Regards, Uma Shankar >-- >Ville Syrjälä >Intel
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, &replaced); state->color_mgmt_changed |= replaced; return ret; + } else if (property == plane->ctm_property) { + ret = drm_atomic_replace_property_blob_from_id(dev, + &state->ctm, + val, + sizeof(struct drm_color_ctm), -1, + &replaced); + 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 @@ -183,6 +183,14 @@ struct drm_plane_state { struct drm_property_blob *degamma_lut; /** + * @ctm: + * + * Color transformation matrix. See drm_plane_enable_color_mgmt(). The + * blob (if not NULL) is a &struct drm_color_ctm. + */ + struct drm_property_blob *ctm; + + /** * @commit: Tracks the pending commit to prevent use-after-free conditions, * and for async plane updates. * @@ -698,6 +706,13 @@ struct drm_plane { * size of the degamma LUT as supported by the driver (read-only). */ struct drm_property *degamma_lut_size_property; + + /** + * @plane_ctm_property: Optional Plane property to set the + * matrix used to convert colors after the lookup in the + * degamma LUT. + */ + struct drm_property *ctm_property; }; #define obj_to_plane(x) container_of(x, struct drm_plane, base)
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 <uma.shankar@intel.com> --- 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(+)