> -----Original Message-----
> From: Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> Sent: Tuesday, January 28, 2025 9:21 PM
> To: intel-xe@xxxxxxxxxxxxxxxxxxxxx; intel-gfx@xxxxxxxxxxxxxxxxxxxxx; dri-
> devel@xxxxxxxxxxxxxxxxxxxxx
> Cc: Kandpal, Suraj <suraj.kandpal@xxxxxxxxx>; dmitry.baryshkov@xxxxxxxxxx;
> Murthy, Arun R <arun.r.murthy@xxxxxxxxx>
> Subject: [PATCH v8 03/14] drm/crtc: Expose API to create drm crtc property for
> histogram
>
> Add drm-crtc property for histogram and for the properties added add the
> corresponding get/set_property.
>
> v8: Rebased
>
> Signed-off-by: Arun R Murthy <arun.r.murthy@xxxxxxxxx>
> ---
> drivers/gpu/drm/drm_atomic_state_helper.c | 14 ++++++
> drivers/gpu/drm/drm_atomic_uapi.c | 15 +++++++
> drivers/gpu/drm/drm_crtc.c | 73
> +++++++++++++++++++++++++++++++
> include/drm/drm_crtc.h | 44 +++++++++++++++++++
> 4 files changed, 146 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c
> b/drivers/gpu/drm/drm_atomic_state_helper.c
> index
> 519228eb109533d2596e899a57b571fa0995824f..dfe6293f7a42d034da3de5930
> 94019ca15014a02 100644
> --- a/drivers/gpu/drm/drm_atomic_state_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_state_helper.c
> @@ -143,6 +143,12 @@ void
> __drm_atomic_helper_crtc_duplicate_state(struct drm_crtc *crtc,
> drm_property_blob_get(state->ctm);
> if (state->gamma_lut)
> drm_property_blob_get(state->gamma_lut);
> + if (state->histogram_caps)
> + drm_property_blob_get(state->histogram_caps);
> + if (state->histogram_enable)
> + drm_property_blob_get(state->histogram_enable);
> + if (state->histogram_data)
> + drm_property_blob_get(state->histogram_data);
> state->mode_changed = false;
> state->active_changed = false;
> state->planes_changed = false;
> @@ -156,6 +162,8 @@ void __drm_atomic_helper_crtc_duplicate_state(struct
> drm_crtc *crtc,
> /* Self refresh should be canceled when a new update is available */
> state->active = drm_atomic_crtc_effectively_active(state);
> state->self_refresh_active = false;
> +
Nit : Extra line not needed. Also move right under planes_changed.
> + state->histogram_updated = false;
> }
> EXPORT_SYMBOL(__drm_atomic_helper_crtc_duplicate_state);
>
> @@ -215,6 +223,12 @@ void __drm_atomic_helper_crtc_destroy_state(struct
> drm_crtc_state *state)
> drm_property_blob_put(state->degamma_lut);
> drm_property_blob_put(state->ctm);
> drm_property_blob_put(state->gamma_lut);
> + if (state->histogram_caps)
> + drm_property_blob_put(state->histogram_caps);
> + if (state->histogram_enable)
> + drm_property_blob_put(state->histogram_enable);
> + if (state->histogram_data)
> + drm_property_blob_put(state->histogram_data);
> }
> EXPORT_SYMBOL(__drm_atomic_helper_crtc_destroy_state);
>
> diff --git a/drivers/gpu/drm/drm_atomic_uapi.c
> b/drivers/gpu/drm/drm_atomic_uapi.c
> index
> 370dc676e3aa543c9827b50df20df78f02b738c9..459d30898196c94392a7f916b
> 1fa9ca3a334eea8 100644
> --- a/drivers/gpu/drm/drm_atomic_uapi.c
> +++ b/drivers/gpu/drm/drm_atomic_uapi.c
> @@ -415,6 +415,15 @@ static int drm_atomic_crtc_set_property(struct
> drm_crtc *crtc,
> return -EFAULT;
>
> set_out_fence_for_crtc(state->state, crtc, fence_ptr);
> + } else if (property == crtc->histogram_enable_property) {
> + ret = drm_property_replace_blob_from_id(dev,
> + &state-
> >histogram_enable,
> + val,
> + -1,
> + sizeof(struct
> drm_histogram_config),
> + &replaced);
> + state->histogram_updated |= replaced;
> + return ret;
> } else if (property == crtc->scaling_filter_property) {
> state->scaling_filter = val;
> } else if (crtc->funcs->atomic_set_property) { @@ -452,6 +461,12 @@
> drm_atomic_crtc_get_property(struct drm_crtc *crtc,
> *val = (state->gamma_lut) ? state->gamma_lut->base.id : 0;
> else if (property == config->prop_out_fence_ptr)
> *val = 0;
> + else if (property == crtc->histogram_caps_property)
> + *val = (state->histogram_caps) ? state->histogram_caps-
> >base.id : 0;
> + else if (property == crtc->histogram_enable_property)
> + *val = (state->histogram_enable) ? state->histogram_enable-
> >base.id : 0;
> + else if (property == crtc->histogram_data_property)
> + *val = (state->histogram_data) ? state->histogram_data-
> >base.id : 0;
> else if (property == crtc->scaling_filter_property)
> *val = state->scaling_filter;
> else if (crtc->funcs->atomic_get_property)
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index
> 46655339003db2a1b43441434839e26f61d79b4e..d10b29aff725e40bdb93e6b
> d0828347db40fa3e8 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -959,3 +959,76 @@ bool drm_crtc_in_clone_mode(struct drm_crtc_state
> *crtc_state)
> return hweight32(crtc_state->encoder_mask) > 1; }
> EXPORT_SYMBOL(drm_crtc_in_clone_mode);
> +
> +/**
> + * drm_crtc_create_histogram_property: create histogram properties
> + *
> + * @crtc: pointer to the struct drm_crtc.
> + * @caps: pointer to the struct drm_histogram_caps, holds the
> + * histogram hardware capabilities.
> + *
> + * The property HISTOGRAM_CAPS exposes the hardware capability for
> + * histogram which includes the histogram mode, number of bins etc
> + * The property HISTOGRAM_ENABLE allows user to enable/disable the
> + * histogram feature and also configure the hardware.
> + * Upon KMD enabling by writing to the hardware registers, histogram
> + * is generated. Histogram is composed of 'n' bins with each bin
> + * being an integer(pixel count).
> + * An event HISTOGRAM will be sent to the user. User upon receiving
> +this
> + * event can read the hardware generated histogram using crtc property
> + * HISTOGRAM_DATA.
> + * User can use this histogram data to enhance the image or in shaders.
> + *
> + * Property HISTOGRAM_CAPS is a blob pointing to the struct
> +drm_histogram_caps
> + * Description of the structure is in include/uapi/drm/drm_mode.h
I feel the line " Description of the structure is in include/uapi/drm/drm_mode.h"
isn't needed since once can use ctags to directly jump to the struct
> + * Property HISTOGRAM_ENABLE is a blob pointing to the struct
> + * drm_histogram_config
> + * Description of the structure is in include/uapi/drm/drm_mode.h
> + * Property HISTOGRAM_DATA is a blob pointing to the struct
> +drm_histogram
> + * Description of the structure is in include/uapi/drm/drm_mode.h
> + *
> + * RETURNS:
> + * Zero for success or -errno
> + */
> +int drm_crtc_create_histogram_property(struct drm_crtc *crtc,
> + struct drm_histogram_caps *caps) {
> + struct drm_property *prop;
> + struct drm_property_blob *blob;
> + struct drm_histogram_caps *blob_data;
> +
> + blob = drm_property_create_blob(crtc->dev,
> + sizeof(struct drm_histogram_caps),
> + NULL);
Align with parenthesis
> + if (IS_ERR(blob))
> + return -1;
Shouldn’t be -1 maybe -EINVAL. Also new line after this if
> + blob_data = blob->data;
> + blob_data->histogram_mode = caps->histogram_mode;
> + blob_data->bins_count = caps->bins_count;
> +
> + prop = drm_property_create(crtc->dev, DRM_MODE_PROP_ATOMIC |
> + DRM_MODE_PROP_IMMUTABLE |
> DRM_MODE_PROP_BLOB,
> + "HISTOGRAM_CAPS", blob->base.id);
> + if (!prop)
> + return -ENOMEM;
New line here
> + drm_object_attach_property(&crtc->base, prop, 0);
> + crtc->histogram_caps_property = prop;
> +
> + prop = drm_property_create(crtc->dev, DRM_MODE_PROP_ATOMIC |
> + DRM_MODE_PROP_BLOB,
> "HISTOGRAM_ENABLE", 0);
> + if (!prop)
> + return -ENOMEM;
> + drm_object_attach_property(&crtc->base, prop, 0);
> + crtc->histogram_enable_property = prop;
> +
> + prop = drm_property_create(crtc->dev, DRM_MODE_PROP_ATOMIC |
> + DRM_MODE_PROP_IMMUTABLE |
> DRM_MODE_PROP_BLOB,
> + "HISTOGRAM_DATA", 0);
> + if (!prop)
> + return -ENOMEM;
*Ditto
Regards,
Suraj Kandpal
> + drm_object_attach_property(&crtc->base, prop, 0);
> + crtc->histogram_data_property = prop;
> +
> + return 0;
> +}
> +EXPORT_SYMBOL(drm_crtc_create_histogram_property);
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index
> caa56e039da2a748cf40ebf45b37158acda439d9..2da803749bdf03c07268be4e0
> 75793ef4e4eb99a 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -274,6 +274,32 @@ struct drm_crtc_state {
> */
> struct drm_property_blob *gamma_lut;
>
> + /**
> + * @histogram_caps:
> + *
> + * The blob points to the structure drm_histogram_caps.
> + * For more info on the elements of the struct drm_histogram_caps
> + * see include/uapi/drm/drm_mode.h
> + */
> + struct drm_property_blob *histogram_caps;
> + /**
> + * @histogram_enable:
> + *
> + * The blob points to the structure drm_histogram_config.
> + * For more information on the elements of struct
> drm_histogram_config
> + * see include/uapi/drm/drm_mode.h
> + */
> + struct drm_property_blob *histogram_enable;
> + /**
> + * @histogram_data:
> + *
> + * The blob points to the structure drm_histogram.
> + * For more information on the elements of struct drm_histogram
> + * see include/uapi/drm/drm_mode.h
> + */
> + struct drm_property_blob *histogram_data;
> + bool histogram_updated;
> +
> /**
> * @target_vblank:
> *
> @@ -1088,6 +1114,22 @@ struct drm_crtc {
> */
> struct drm_property *scaling_filter_property;
>
> + /**
> + * @histogram_caps_property: Optional CRTC property for getting the
> + * histogram hardware capability.
> + */
> + struct drm_property *histogram_caps_property;
> + /**
> + * @histogram_enable_property: Optional CRTC property for enabling
> or
> + * disabling global histogram.
> + */
> + struct drm_property *histogram_enable_property;
> + /**
> + * @histogram_data_proeprty: Optional CRTC property for getting the
> + * histogram blob data.
> + */
> + struct drm_property *histogram_data_property;
> +
> /**
> * @state:
> *
> @@ -1324,4 +1366,6 @@ static inline struct drm_crtc *drm_crtc_find(struct
> drm_device *dev, int drm_crtc_create_scaling_filter_property(struct drm_crtc
> *crtc,
> unsigned int supported_filters);
> bool drm_crtc_in_clone_mode(struct drm_crtc_state *crtc_state);
> +int drm_crtc_create_histogram_property(struct drm_crtc *crtc,
> + struct drm_histogram_caps *caps);
> #endif /* __DRM_CRTC_H__ */
>
> --
> 2.25.1
