| Message ID | 20240705095551.1244154-4-arun.r.murthy@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Display Global Histogram | expand |
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun R > Murthy > Sent: Friday, July 5, 2024 3:26 PM > To: intel-gfx@lists.freedesktop.org > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > Subject: [PATCH 3/5] Add crtc properties for global histogram > > CRTC properties have been added for enable/disable histogram, reading the > histogram data and writing the IET data. > "HISTOGRAM_EN" is the crtc property to enable/disable the global histogram and > takes a value 0/1 accordingly. > "Histogram" is a crtc property to read the binary histogram data. > "Global IET" is a crtc property to write the IET binary LUT data. + CC'ing Abhinav from QC Hi Abhinav, As discussed in Display Hackfest, can you please share your thoughts and inputs on this series. Thanks & Regards, Uma Shankar > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_atomic.c | 5 + > drivers/gpu/drm/i915/display/intel_crtc.c | 202 +++++++++++++++++- > drivers/gpu/drm/i915/display/intel_crtc.h | 5 + > drivers/gpu/drm/i915/display/intel_display.c | 13 ++ > .../drm/i915/display/intel_display_types.h | 17 ++ > .../gpu/drm/i915/display/intel_histogram.c | 1 + > 6 files changed, 242 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c > b/drivers/gpu/drm/i915/display/intel_atomic.c > index 76aa10b6f647..693a22089937 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > @@ -246,6 +246,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > > __drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->uapi); > > + if (crtc_state->global_iet) > + drm_property_blob_get(crtc_state->global_iet); > /* copy color blobs */ > if (crtc_state->hw.degamma_lut) > drm_property_blob_get(crtc_state->hw.degamma_lut); > @@ -277,6 +279,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > crtc_state->fb_bits = 0; > crtc_state->update_planes = 0; > crtc_state->dsb = NULL; > + crtc_state->histogram_en_changed = false; > > return &crtc_state->uapi; > } > @@ -312,6 +315,8 @@ intel_crtc_destroy_state(struct drm_crtc *crtc, > > drm_WARN_ON(crtc->dev, crtc_state->dsb); > > + if (crtc_state->global_iet) > + drm_property_blob_put(crtc_state->global_iet); > __drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi); > intel_crtc_free_hw_state(crtc_state); > if (crtc_state->dp_tunnel_ref.tunnel) > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c > b/drivers/gpu/drm/i915/display/intel_crtc.c > index 1b578cad2813..24f160359422 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > @@ -10,6 +10,7 @@ > #include <drm/drm_fourcc.h> > #include <drm/drm_plane.h> > #include <drm/drm_vblank_work.h> > +#include <drm/drm_atomic_uapi.h> > > #include "i915_vgpu.h" > #include "i9xx_plane.h" > @@ -26,6 +27,7 @@ > #include "intel_drrs.h" > #include "intel_dsi.h" > #include "intel_fifo_underrun.h" > +#include "intel_histogram.h" > #include "intel_pipe_crc.h" > #include "intel_psr.h" > #include "intel_sprite.h" > @@ -201,6 +203,7 @@ static struct intel_crtc *intel_crtc_alloc(void) static void > intel_crtc_free(struct intel_crtc *crtc) { > intel_crtc_destroy_state(&crtc->base, crtc->base.state); > + intel_histogram_deinit(crtc); > kfree(crtc); > } > > @@ -220,6 +223,100 @@ static int intel_crtc_late_register(struct drm_crtc > *crtc) > return 0; > } > > +static int intel_crtc_get_property(struct drm_crtc *crtc, > + const struct drm_crtc_state *state, > + struct drm_property *property, > + uint64_t *val) > +{ > + struct drm_i915_private *i915 = to_i915(crtc->dev); > + const struct intel_crtc_state *intel_crtc_state = > + to_intel_crtc_state(state); > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + > + if (property == intel_crtc->histogram_en_property) { > + *val = intel_crtc_state->histogram_en; > + } else if (property == intel_crtc->global_iet_property) { > + *val = (intel_crtc_state->global_iet) ? > + intel_crtc_state->global_iet->base.id : 0; > + } else if (property == intel_crtc->histogram_property) { > + *val = (intel_crtc_state->histogram) ? > + intel_crtc_state->histogram->base.id : 0; > + } else { > + drm_err(&i915->drm, > + "Unknown property [PROP:%d:%s]\n", > + property->base.id, property->name); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int > +intel_atomic_replace_property_blob_from_id(struct drm_device *dev, > + struct drm_property_blob **blob, > + u64 blob_id, > + ssize_t expected_size, > + ssize_t expected_elem_size, > + bool *replaced) > +{ > + struct drm_property_blob *new_blob = NULL; > + > + if (blob_id != 0) { > + new_blob = drm_property_lookup_blob(dev, blob_id); > + if (!new_blob) > + return -EINVAL; > + > + if (expected_size > 0 && > + new_blob->length != expected_size) { > + drm_property_blob_put(new_blob); > + return -EINVAL; > + } > + if (expected_elem_size > 0 && > + new_blob->length % expected_elem_size != 0) { > + drm_property_blob_put(new_blob); > + return -EINVAL; > + } > + } > + > + *replaced |= drm_property_replace_blob(blob, new_blob); > + drm_property_blob_put(new_blob); > + > + return 0; > +} > + > +static int intel_crtc_set_property(struct drm_crtc *crtc, > + struct drm_crtc_state *state, > + struct drm_property *property, > + u64 val) > +{ > + struct drm_i915_private *i915 = to_i915(crtc->dev); > + struct intel_crtc_state *intel_crtc_state = > + to_intel_crtc_state(state); > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + bool replaced = false; > + > + if (property == intel_crtc->histogram_en_property) { > + intel_crtc_state->histogram_en = val; > + intel_crtc_state->histogram_en_changed = true; > + return 0; > + } > + > + if (property == intel_crtc->global_iet_property) { > + intel_atomic_replace_property_blob_from_id(crtc->dev, > + &intel_crtc_state- > >global_iet, > + val, > + sizeof(uint32_t) * > HISTOGRAM_IET_LENGTH, > + -1, &replaced); > + if (replaced) > + intel_crtc_state->global_iet_changed = true; > + return 0; > + } > + > + drm_dbg_atomic(&i915->drm, "Unknown property [PROP:%d:%s]\n", > + property->base.id, property->name); > + return -EINVAL; > +} > + > #define INTEL_CRTC_FUNCS \ > .set_config = drm_atomic_helper_set_config, \ > .destroy = intel_crtc_destroy, \ > @@ -229,7 +326,9 @@ static int intel_crtc_late_register(struct drm_crtc *crtc) > .set_crc_source = intel_crtc_set_crc_source, \ > .verify_crc_source = intel_crtc_verify_crc_source, \ > .get_crc_sources = intel_crtc_get_crc_sources, \ > - .late_register = intel_crtc_late_register > + .late_register = intel_crtc_late_register, \ > + .atomic_set_property = intel_crtc_set_property, \ > + .atomic_get_property = intel_crtc_get_property > > static const struct drm_crtc_funcs bdw_crtc_funcs = { > INTEL_CRTC_FUNCS, > @@ -374,6 +473,10 @@ int intel_crtc_init(struct drm_i915_private *dev_priv, > enum pipe pipe) > intel_color_crtc_init(crtc); > intel_drrs_crtc_init(crtc); > intel_crtc_crc_init(crtc); > + intel_histogram_init(crtc); > + > + /* Initialize crtc properties */ > + intel_crtc_add_property(crtc); > > cpu_latency_qos_add_request(&crtc->vblank_pm_qos, > PM_QOS_DEFAULT_VALUE); > > @@ -690,3 +793,100 @@ void intel_pipe_update_end(struct intel_atomic_state > *state, > out: > intel_psr_unlock(new_crtc_state); > } > + > +static const struct drm_prop_enum_list histogram_en_names[] = { > + { INTEL_HISTOGRAM_DISABLE, "Disable" }, > + { INTEL_HISTOGRAM_ENABLE, "Enable" }, > +}; > + > +/** > + * intel_attach_histogram_en_property() - add property to > +enable/disable histogram > + * @intel_crtc: pointer to the struct intel_crtc on which the global histogram is > to > + * be enabled/disabled > + * > + * "HISTOGRAM_EN" is the crtc propety to enable/disable global > +histogram */ void intel_attach_histogram_en_property(struct intel_crtc > +*intel_crtc) { > + struct drm_crtc *crtc = &intel_crtc->base; > + struct drm_device *dev = crtc->dev; > + struct drm_property *prop; > + > + prop = intel_crtc->histogram_en_property; > + if (!prop) { > + prop = drm_property_create_enum(dev, 0, > + "HISTOGRAM_EN", > + histogram_en_names, > + > ARRAY_SIZE(histogram_en_names)); > + if (!prop) > + return; > + > + intel_crtc->histogram_en_property = prop; > + } > + > + drm_object_attach_property(&crtc->base, prop, 0); } > + > +/** > + * intel_attach_global_iet_property() - add property to write Image > +Enhancement data > + * @intel_crtc: pointer to the struct intel_crtc on which global > +histogram is enabled > + * > + * "Global IET" is the crtc property to write the Image Enhancement LUT > +binary data */ void intel_attach_global_iet_property(struct intel_crtc > +*intel_crtc) { > + struct drm_crtc *crtc = &intel_crtc->base; > + struct drm_device *dev = crtc->dev; > + struct drm_property *prop; > + > + prop = intel_crtc->global_iet_property; > + if (!prop) { > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | > DRM_MODE_PROP_ATOMIC, > + "Global IET", 0); > + if (!prop) > + return; > + > + intel_crtc->global_iet_property = prop; > + } > + > + drm_object_attach_property(&crtc->base, prop, 0); } > + > +/** > + * intel_attach_histogram_property() - crtc property to read the histogram. > + * @intel_crtc: pointer to the struct intel_crtc on which the global histogram > + * was enabled. > + * "Global Histogram" is the crtc property to read the binary histogram data. > + */ > +void intel_attach_histogram_property(struct intel_crtc *intel_crtc) { > + struct drm_crtc *crtc = &intel_crtc->base; > + struct drm_device *dev = crtc->dev; > + struct drm_property *prop; > + struct drm_property_blob *blob; > + > + prop = intel_crtc->histogram_property; > + if (!prop) { > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | > + DRM_MODE_PROP_ATOMIC | > + DRM_MODE_PROP_IMMUTABLE, > + "Global Histogram", 0); > + if (!prop) > + return; > + > + intel_crtc->histogram_property = prop; > + } > + blob = drm_property_create_blob(dev, sizeof(uint32_t) * > HISTOGRAM_BIN_COUNT, NULL); > + intel_crtc->config->histogram = blob; > + > + drm_object_attach_property(&crtc->base, prop, blob->base.id); } > + > +int intel_crtc_add_property(struct intel_crtc *intel_crtc) { > + intel_attach_histogram_en_property(intel_crtc); > + intel_attach_histogram_property(intel_crtc); > + intel_attach_global_iet_property(intel_crtc); > + > + return 0; > +} > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h > b/drivers/gpu/drm/i915/display/intel_crtc.h > index b615b7ab5ccd..56c6b7c6037e 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc.h > +++ b/drivers/gpu/drm/i915/display/intel_crtc.h > @@ -7,6 +7,7 @@ > #define _INTEL_CRTC_H_ > > #include <linux/types.h> > +#include <drm/drm_crtc.h> > > enum i9xx_plane_id; > enum pipe; > @@ -49,4 +50,8 @@ void intel_wait_for_vblank_if_active(struct > drm_i915_private *i915, > enum pipe pipe); > void intel_crtc_wait_for_next_vblank(struct intel_crtc *crtc); > > +int intel_crtc_add_property(struct intel_crtc *intel_crtc); void > +intel_attach_histogram_en_property(struct intel_crtc *intel_crtc); void > +intel_attach_global_iet_property(struct intel_crtc *intel_crtc); void > +intel_attach_histogram_property(struct intel_crtc *intel_crtc); > #endif > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index c2c388212e2e..94e9f7a71a90 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -93,6 +93,7 @@ > #include "intel_fifo_underrun.h" > #include "intel_frontbuffer.h" > #include "intel_hdmi.h" > +#include "intel_histogram.h" > #include "intel_hotplug.h" > #include "intel_link_bw.h" > #include "intel_lvds.h" > @@ -4324,6 +4325,10 @@ static int intel_crtc_atomic_check(struct > intel_atomic_state *state, > if (ret) > return ret; > > + /* HISTOGRAM changed */ > + if (crtc_state->histogram_en_changed) > + return intel_histogram_can_enable(crtc); > + > return 0; > } > > @@ -7503,6 +7508,14 @@ static void intel_atomic_commit_tail(struct > intel_atomic_state *state) > * FIXME get rid of this funny new->old swapping > */ > old_crtc_state->dsb = fetch_and_zero(&new_crtc_state->dsb); > + > + /* Re-Visit: HISTOGRAM related stuff */ > + if (new_crtc_state->histogram_en_changed) > + intel_histogram_update(crtc, > + new_crtc_state->histogram_en); > + if (new_crtc_state->global_iet_changed) > + intel_histogram_set_iet_lut(crtc, > + (u32 *)new_crtc_state- > >global_iet->data); > } > > /* Underruns don't always raise interrupts, so check manually */ diff --git > a/drivers/gpu/drm/i915/display/intel_display_types.h > b/drivers/gpu/drm/i915/display/intel_display_types.h > index e0a9c6d8c9b2..e7c33eb76a7e 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -99,6 +99,12 @@ enum intel_broadcast_rgb { > INTEL_BROADCAST_RGB_LIMITED, > }; > > +/* HISTOGRAM property */ > +enum intel_histogram_en_prop { > + INTEL_HISTOGRAM_PROP_DISABLE, > + INTEL_HISTOGRAM_PROP_ENABLE, > +}; > + > struct intel_fb_view { > /* > * The remap information used in the remapped and rotated views to > @@ -1431,6 +1437,13 @@ struct intel_crtc_state { > > /* LOBF flag */ > bool has_lobf; > + > + /* HISTOGRAM data */ > + int histogram_en; > + struct drm_property_blob *global_iet; > + struct drm_property_blob *histogram; > + bool global_iet_changed; > + bool histogram_en_changed; > }; > > enum intel_pipe_crc_source { > @@ -1539,6 +1552,10 @@ struct intel_crtc { > > /* histogram data */ > struct intel_histogram *histogram; > + /* HISTOGRAM properties */ > + struct drm_property *histogram_en_property; > + struct drm_property *global_iet_property; > + struct drm_property *histogram_property; > > #ifdef CONFIG_DEBUG_FS > struct intel_pipe_crc pipe_crc; > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c > b/drivers/gpu/drm/i915/display/intel_histogram.c > index 8fa3bc74e52b..740019fdf0df 100644 > --- a/drivers/gpu/drm/i915/display/intel_histogram.c > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c > @@ -183,6 +183,7 @@ static void intel_histogram_disable(struct intel_crtc > *intel_crtc) > > cancel_delayed_work(&histogram->handle_histogram_int_work); > histogram->enable = false; > + intel_crtc->config->histogram_en = false; > } > > int intel_histogram_update(struct intel_crtc *intel_crtc, bool enable) > -- > 2.25.1
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Arun > R Murthy > Sent: Friday, July 5, 2024 3:26 PM > To: intel-gfx@lists.freedesktop.org > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > Subject: [PATCH 3/5] Add crtc properties for global histogram > > CRTC properties have been added for enable/disable histogram, reading the > histogram data and writing the IET data. > "HISTOGRAM_EN" is the crtc property to enable/disable the global histogram > and takes a value 0/1 accordingly. > "Histogram" is a crtc property to read the binary histogram data. > "Global IET" is a crtc property to write the IET binary LUT data. > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > --- > drivers/gpu/drm/i915/display/intel_atomic.c | 5 + > drivers/gpu/drm/i915/display/intel_crtc.c | 202 +++++++++++++++++- > drivers/gpu/drm/i915/display/intel_crtc.h | 5 + > drivers/gpu/drm/i915/display/intel_display.c | 13 ++ > .../drm/i915/display/intel_display_types.h | 17 ++ > .../gpu/drm/i915/display/intel_histogram.c | 1 + > 6 files changed, 242 insertions(+), 1 deletion(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c > b/drivers/gpu/drm/i915/display/intel_atomic.c > index 76aa10b6f647..693a22089937 100644 > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > @@ -246,6 +246,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > > __drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->uapi); > > + if (crtc_state->global_iet) > + drm_property_blob_get(crtc_state->global_iet); > /* copy color blobs */ > if (crtc_state->hw.degamma_lut) > drm_property_blob_get(crtc_state->hw.degamma_lut); > @@ -277,6 +279,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > crtc_state->fb_bits = 0; > crtc_state->update_planes = 0; > crtc_state->dsb = NULL; > + crtc_state->histogram_en_changed = false; > > return &crtc_state->uapi; > } > @@ -312,6 +315,8 @@ intel_crtc_destroy_state(struct drm_crtc *crtc, > > drm_WARN_ON(crtc->dev, crtc_state->dsb); > > + if (crtc_state->global_iet) > + drm_property_blob_put(crtc_state->global_iet); > __drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi); > intel_crtc_free_hw_state(crtc_state); > if (crtc_state->dp_tunnel_ref.tunnel) > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c > b/drivers/gpu/drm/i915/display/intel_crtc.c > index 1b578cad2813..24f160359422 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > @@ -10,6 +10,7 @@ > #include <drm/drm_fourcc.h> > #include <drm/drm_plane.h> > #include <drm/drm_vblank_work.h> > +#include <drm/drm_atomic_uapi.h> > > #include "i915_vgpu.h" > #include "i9xx_plane.h" > @@ -26,6 +27,7 @@ > #include "intel_drrs.h" > #include "intel_dsi.h" > #include "intel_fifo_underrun.h" > +#include "intel_histogram.h" > #include "intel_pipe_crc.h" > #include "intel_psr.h" > #include "intel_sprite.h" > @@ -201,6 +203,7 @@ static struct intel_crtc *intel_crtc_alloc(void) static > void intel_crtc_free(struct intel_crtc *crtc) { > intel_crtc_destroy_state(&crtc->base, crtc->base.state); > + intel_histogram_deinit(crtc); > kfree(crtc); > } > > @@ -220,6 +223,100 @@ static int intel_crtc_late_register(struct drm_crtc > *crtc) > return 0; > } > > +static int intel_crtc_get_property(struct drm_crtc *crtc, > + const struct drm_crtc_state *state, > + struct drm_property *property, > + uint64_t *val) > +{ > + struct drm_i915_private *i915 = to_i915(crtc->dev); > + const struct intel_crtc_state *intel_crtc_state = > + to_intel_crtc_state(state); > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + > + if (property == intel_crtc->histogram_en_property) { > + *val = intel_crtc_state->histogram_en; > + } else if (property == intel_crtc->global_iet_property) { > + *val = (intel_crtc_state->global_iet) ? > + intel_crtc_state->global_iet->base.id : 0; > + } else if (property == intel_crtc->histogram_property) { > + *val = (intel_crtc_state->histogram) ? > + intel_crtc_state->histogram->base.id : 0; > + } else { > + drm_err(&i915->drm, > + "Unknown property [PROP:%d:%s]\n", > + property->base.id, property->name); > + return -EINVAL; > + } > + > + return 0; > +} > + > +static int > +intel_atomic_replace_property_blob_from_id(struct drm_device *dev, > + struct drm_property_blob **blob, > + u64 blob_id, > + ssize_t expected_size, > + ssize_t expected_elem_size, > + bool *replaced) > +{ > + struct drm_property_blob *new_blob = NULL; > + > + if (blob_id != 0) { > + new_blob = drm_property_lookup_blob(dev, blob_id); > + if (!new_blob) > + return -EINVAL; > + > + if (expected_size > 0 && > + new_blob->length != expected_size) { > + drm_property_blob_put(new_blob); > + return -EINVAL; > + } > + if (expected_elem_size > 0 && > + new_blob->length % expected_elem_size != 0) { > + drm_property_blob_put(new_blob); > + return -EINVAL; > + } > + } > + > + *replaced |= drm_property_replace_blob(blob, new_blob); > + drm_property_blob_put(new_blob); > + > + return 0; > +} > + > +static int intel_crtc_set_property(struct drm_crtc *crtc, > + struct drm_crtc_state *state, > + struct drm_property *property, > + u64 val) > +{ > + struct drm_i915_private *i915 = to_i915(crtc->dev); > + struct intel_crtc_state *intel_crtc_state = > + to_intel_crtc_state(state); > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + bool replaced = false; > + > + if (property == intel_crtc->histogram_en_property) { > + intel_crtc_state->histogram_en = val; > + intel_crtc_state->histogram_en_changed = true; > + return 0; > + } > + > + if (property == intel_crtc->global_iet_property) { > + intel_atomic_replace_property_blob_from_id(crtc->dev, > + &intel_crtc_state- > >global_iet, > + val, > + sizeof(uint32_t) * > HISTOGRAM_IET_LENGTH, > + -1, &replaced); > + if (replaced) > + intel_crtc_state->global_iet_changed = true; > + return 0; > + } > + > + drm_dbg_atomic(&i915->drm, "Unknown property > [PROP:%d:%s]\n", > + property->base.id, property->name); > + return -EINVAL; > +} > + > #define INTEL_CRTC_FUNCS \ > .set_config = drm_atomic_helper_set_config, \ > .destroy = intel_crtc_destroy, \ > @@ -229,7 +326,9 @@ static int intel_crtc_late_register(struct drm_crtc > *crtc) > .set_crc_source = intel_crtc_set_crc_source, \ > .verify_crc_source = intel_crtc_verify_crc_source, \ > .get_crc_sources = intel_crtc_get_crc_sources, \ > - .late_register = intel_crtc_late_register > + .late_register = intel_crtc_late_register, \ > + .atomic_set_property = intel_crtc_set_property, \ > + .atomic_get_property = intel_crtc_get_property > > static const struct drm_crtc_funcs bdw_crtc_funcs = { > INTEL_CRTC_FUNCS, > @@ -374,6 +473,10 @@ int intel_crtc_init(struct drm_i915_private > *dev_priv, enum pipe pipe) > intel_color_crtc_init(crtc); > intel_drrs_crtc_init(crtc); > intel_crtc_crc_init(crtc); > + intel_histogram_init(crtc); > + > + /* Initialize crtc properties */ > + intel_crtc_add_property(crtc); > > cpu_latency_qos_add_request(&crtc->vblank_pm_qos, > PM_QOS_DEFAULT_VALUE); > > @@ -690,3 +793,100 @@ void intel_pipe_update_end(struct > intel_atomic_state *state, > out: > intel_psr_unlock(new_crtc_state); > } > + > +static const struct drm_prop_enum_list histogram_en_names[] = { > + { INTEL_HISTOGRAM_DISABLE, "Disable" }, > + { INTEL_HISTOGRAM_ENABLE, "Enable" }, > +}; > + > +/** > + * intel_attach_histogram_en_property() - add property to > +enable/disable histogram > + * @intel_crtc: pointer to the struct intel_crtc on which the global histogram > is to > + * be enabled/disabled > + * > + * "HISTOGRAM_EN" is the crtc propety to enable/disable global > +histogram */ void intel_attach_histogram_en_property(struct intel_crtc > +*intel_crtc) { > + struct drm_crtc *crtc = &intel_crtc->base; > + struct drm_device *dev = crtc->dev; > + struct drm_property *prop; > + > + prop = intel_crtc->histogram_en_property; > + if (!prop) { > + prop = drm_property_create_enum(dev, 0, > + "HISTOGRAM_EN", > + histogram_en_names, > + > ARRAY_SIZE(histogram_en_names)); > + if (!prop) > + return; > + > + intel_crtc->histogram_en_property = prop; > + } > + > + drm_object_attach_property(&crtc->base, prop, 0); } > + > +/** > + * intel_attach_global_iet_property() - add property to write Image > +Enhancement data > + * @intel_crtc: pointer to the struct intel_crtc on which global > +histogram is enabled > + * > + * "Global IET" is the crtc property to write the Image Enhancement LUT > +binary data */ void intel_attach_global_iet_property(struct intel_crtc > +*intel_crtc) { > + struct drm_crtc *crtc = &intel_crtc->base; > + struct drm_device *dev = crtc->dev; > + struct drm_property *prop; > + > + prop = intel_crtc->global_iet_property; > + if (!prop) { > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB > | DRM_MODE_PROP_ATOMIC, > + "Global IET", 0); > + if (!prop) > + return; > + > + intel_crtc->global_iet_property = prop; > + } > + > + drm_object_attach_property(&crtc->base, prop, 0); } > + > +/** > + * intel_attach_histogram_property() - crtc property to read the histogram. > + * @intel_crtc: pointer to the struct intel_crtc on which the global histogram > + * was enabled. > + * "Global Histogram" is the crtc property to read the binary histogram data. > + */ > +void intel_attach_histogram_property(struct intel_crtc *intel_crtc) { > + struct drm_crtc *crtc = &intel_crtc->base; > + struct drm_device *dev = crtc->dev; > + struct drm_property *prop; > + struct drm_property_blob *blob; > + > + prop = intel_crtc->histogram_property; > + if (!prop) { > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB > | > + DRM_MODE_PROP_ATOMIC | > + DRM_MODE_PROP_IMMUTABLE, > + "Global Histogram", 0); > + if (!prop) > + return; > + > + intel_crtc->histogram_property = prop; > + } > + blob = drm_property_create_blob(dev, sizeof(uint32_t) * > HISTOGRAM_BIN_COUNT, NULL); > + intel_crtc->config->histogram = blob; > + > + drm_object_attach_property(&crtc->base, prop, blob->base.id); } > + > +int intel_crtc_add_property(struct intel_crtc *intel_crtc) { > + intel_attach_histogram_en_property(intel_crtc); > + intel_attach_histogram_property(intel_crtc); > + intel_attach_global_iet_property(intel_crtc); > + > + return 0; > +} > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h > b/drivers/gpu/drm/i915/display/intel_crtc.h > index b615b7ab5ccd..56c6b7c6037e 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc.h > +++ b/drivers/gpu/drm/i915/display/intel_crtc.h > @@ -7,6 +7,7 @@ > #define _INTEL_CRTC_H_ > > #include <linux/types.h> > +#include <drm/drm_crtc.h> > > enum i9xx_plane_id; > enum pipe; > @@ -49,4 +50,8 @@ void intel_wait_for_vblank_if_active(struct > drm_i915_private *i915, > enum pipe pipe); > void intel_crtc_wait_for_next_vblank(struct intel_crtc *crtc); > > +int intel_crtc_add_property(struct intel_crtc *intel_crtc); void > +intel_attach_histogram_en_property(struct intel_crtc *intel_crtc); void > +intel_attach_global_iet_property(struct intel_crtc *intel_crtc); void > +intel_attach_histogram_property(struct intel_crtc *intel_crtc); > #endif > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > b/drivers/gpu/drm/i915/display/intel_display.c > index c2c388212e2e..94e9f7a71a90 100644 > --- a/drivers/gpu/drm/i915/display/intel_display.c > +++ b/drivers/gpu/drm/i915/display/intel_display.c > @@ -93,6 +93,7 @@ > #include "intel_fifo_underrun.h" > #include "intel_frontbuffer.h" > #include "intel_hdmi.h" > +#include "intel_histogram.h" > #include "intel_hotplug.h" > #include "intel_link_bw.h" > #include "intel_lvds.h" > @@ -4324,6 +4325,10 @@ static int intel_crtc_atomic_check(struct > intel_atomic_state *state, > if (ret) > return ret; > > + /* HISTOGRAM changed */ > + if (crtc_state->histogram_en_changed) > + return intel_histogram_can_enable(crtc); > + > return 0; > } > > @@ -7503,6 +7508,14 @@ static void intel_atomic_commit_tail(struct > intel_atomic_state *state) > * FIXME get rid of this funny new->old swapping > */ > old_crtc_state->dsb = fetch_and_zero(&new_crtc_state- > >dsb); > + Since there is a wait_for_vblank in this for older platforms only, what is the expected user space behaviour here for enabling histogram and updating the iets. > + /* Re-Visit: HISTOGRAM related stuff */ > + if (new_crtc_state->histogram_en_changed) > + intel_histogram_update(crtc, > + new_crtc_state->histogram_en); Is there any particular reason that this code is not part of pre plane update? > + if (new_crtc_state->global_iet_changed) > + intel_histogram_set_iet_lut(crtc, > + (u32 *)new_crtc_state- > >global_iet->data); > } > > /* Underruns don't always raise interrupts, so check manually */ diff > --git a/drivers/gpu/drm/i915/display/intel_display_types.h > b/drivers/gpu/drm/i915/display/intel_display_types.h > index e0a9c6d8c9b2..e7c33eb76a7e 100644 > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > @@ -99,6 +99,12 @@ enum intel_broadcast_rgb { > INTEL_BROADCAST_RGB_LIMITED, > }; > > +/* HISTOGRAM property */ > +enum intel_histogram_en_prop { > + INTEL_HISTOGRAM_PROP_DISABLE, > + INTEL_HISTOGRAM_PROP_ENABLE, > +}; > + > struct intel_fb_view { > /* > * The remap information used in the remapped and rotated views to > @@ -1431,6 +1437,13 @@ struct intel_crtc_state { > > /* LOBF flag */ > bool has_lobf; > + > + /* HISTOGRAM data */ > + int histogram_en; > + struct drm_property_blob *global_iet; > + struct drm_property_blob *histogram; > + bool global_iet_changed; > + bool histogram_en_changed; > }; > > enum intel_pipe_crc_source { > @@ -1539,6 +1552,10 @@ struct intel_crtc { > > /* histogram data */ > struct intel_histogram *histogram; > + /* HISTOGRAM properties */ > + struct drm_property *histogram_en_property; > + struct drm_property *global_iet_property; > + struct drm_property *histogram_property; > > #ifdef CONFIG_DEBUG_FS > struct intel_pipe_crc pipe_crc; > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c > b/drivers/gpu/drm/i915/display/intel_histogram.c > index 8fa3bc74e52b..740019fdf0df 100644 > --- a/drivers/gpu/drm/i915/display/intel_histogram.c > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c > @@ -183,6 +183,7 @@ static void intel_histogram_disable(struct intel_crtc > *intel_crtc) > > cancel_delayed_work(&histogram->handle_histogram_int_work); > histogram->enable = false; > + intel_crtc->config->histogram_en = false; > } > > int intel_histogram_update(struct intel_crtc *intel_crtc, bool enable) > -- > 2.25.1
> -----Original Message----- > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of > Kulkarni, Vandita > Sent: Wednesday, September 11, 2024 10:46 AM > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel- > gfx@lists.freedesktop.org > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > Subject: RE: [PATCH 3/5] Add crtc properties for global histogram > > > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of > > Arun R Murthy > > Sent: Friday, July 5, 2024 3:26 PM > > To: intel-gfx@lists.freedesktop.org > > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > > Subject: [PATCH 3/5] Add crtc properties for global histogram > > > > CRTC properties have been added for enable/disable histogram, reading > > the histogram data and writing the IET data. > > "HISTOGRAM_EN" is the crtc property to enable/disable the global > > histogram and takes a value 0/1 accordingly. > > "Histogram" is a crtc property to read the binary histogram data. > > "Global IET" is a crtc property to write the IET binary LUT data. > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_atomic.c | 5 + > > drivers/gpu/drm/i915/display/intel_crtc.c | 202 +++++++++++++++++- > > drivers/gpu/drm/i915/display/intel_crtc.h | 5 + > > drivers/gpu/drm/i915/display/intel_display.c | 13 ++ > > .../drm/i915/display/intel_display_types.h | 17 ++ > > .../gpu/drm/i915/display/intel_histogram.c | 1 + > > 6 files changed, 242 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c > > b/drivers/gpu/drm/i915/display/intel_atomic.c > > index 76aa10b6f647..693a22089937 100644 > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > > @@ -246,6 +246,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > > > > __drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state- > >uapi); > > > > + if (crtc_state->global_iet) > > + drm_property_blob_get(crtc_state->global_iet); > > /* copy color blobs */ > > if (crtc_state->hw.degamma_lut) > > drm_property_blob_get(crtc_state->hw.degamma_lut); > > @@ -277,6 +279,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > > crtc_state->fb_bits = 0; > > crtc_state->update_planes = 0; > > crtc_state->dsb = NULL; > > + crtc_state->histogram_en_changed = false; > > > > return &crtc_state->uapi; > > } > > @@ -312,6 +315,8 @@ intel_crtc_destroy_state(struct drm_crtc *crtc, > > > > drm_WARN_ON(crtc->dev, crtc_state->dsb); > > > > + if (crtc_state->global_iet) > > + drm_property_blob_put(crtc_state->global_iet); > > __drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi); > > intel_crtc_free_hw_state(crtc_state); > > if (crtc_state->dp_tunnel_ref.tunnel) > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c > > b/drivers/gpu/drm/i915/display/intel_crtc.c > > index 1b578cad2813..24f160359422 100644 > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > > @@ -10,6 +10,7 @@ > > #include <drm/drm_fourcc.h> > > #include <drm/drm_plane.h> > > #include <drm/drm_vblank_work.h> > > +#include <drm/drm_atomic_uapi.h> > > > > #include "i915_vgpu.h" > > #include "i9xx_plane.h" > > @@ -26,6 +27,7 @@ > > #include "intel_drrs.h" > > #include "intel_dsi.h" > > #include "intel_fifo_underrun.h" > > +#include "intel_histogram.h" > > #include "intel_pipe_crc.h" > > #include "intel_psr.h" > > #include "intel_sprite.h" > > @@ -201,6 +203,7 @@ static struct intel_crtc *intel_crtc_alloc(void) > > static void intel_crtc_free(struct intel_crtc *crtc) { > > intel_crtc_destroy_state(&crtc->base, crtc->base.state); > > + intel_histogram_deinit(crtc); > > kfree(crtc); > > } > > > > @@ -220,6 +223,100 @@ static int intel_crtc_late_register(struct > > drm_crtc > > *crtc) > > return 0; > > } > > > > +static int intel_crtc_get_property(struct drm_crtc *crtc, > > + const struct drm_crtc_state *state, > > + struct drm_property *property, > > + uint64_t *val) > > +{ > > + struct drm_i915_private *i915 = to_i915(crtc->dev); > > + const struct intel_crtc_state *intel_crtc_state = > > + to_intel_crtc_state(state); > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > + > > + if (property == intel_crtc->histogram_en_property) { > > + *val = intel_crtc_state->histogram_en; > > + } else if (property == intel_crtc->global_iet_property) { > > + *val = (intel_crtc_state->global_iet) ? > > + intel_crtc_state->global_iet->base.id : 0; > > + } else if (property == intel_crtc->histogram_property) { > > + *val = (intel_crtc_state->histogram) ? > > + intel_crtc_state->histogram->base.id : 0; > > + } else { > > + drm_err(&i915->drm, > > + "Unknown property [PROP:%d:%s]\n", > > + property->base.id, property->name); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + Also to me the below function looks like drm_property_replace_blob_from_id, why are we not using that, am I missing something here.. Thanks, Vandita > > +static int > > +intel_atomic_replace_property_blob_from_id(struct drm_device *dev, > > + struct drm_property_blob **blob, > > + u64 blob_id, > > + ssize_t expected_size, > > + ssize_t expected_elem_size, > > + bool *replaced) > > +{ > > + struct drm_property_blob *new_blob = NULL; > > + > > + if (blob_id != 0) { > > + new_blob = drm_property_lookup_blob(dev, blob_id); > > + if (!new_blob) > > + return -EINVAL; > > + > > + if (expected_size > 0 && > > + new_blob->length != expected_size) { > > + drm_property_blob_put(new_blob); > > + return -EINVAL; > > + } > > + if (expected_elem_size > 0 && > > + new_blob->length % expected_elem_size != 0) { > > + drm_property_blob_put(new_blob); > > + return -EINVAL; > > + } > > + } > > + > > + *replaced |= drm_property_replace_blob(blob, new_blob); > > + drm_property_blob_put(new_blob); > > + > > + return 0; > > +} > > + > > +static int intel_crtc_set_property(struct drm_crtc *crtc, > > + struct drm_crtc_state *state, > > + struct drm_property *property, > > + u64 val) > > +{ > > + struct drm_i915_private *i915 = to_i915(crtc->dev); > > + struct intel_crtc_state *intel_crtc_state = > > + to_intel_crtc_state(state); > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > + bool replaced = false; > > + > > + if (property == intel_crtc->histogram_en_property) { > > + intel_crtc_state->histogram_en = val; > > + intel_crtc_state->histogram_en_changed = true; > > + return 0; > > + } > > + > > + if (property == intel_crtc->global_iet_property) { > > + intel_atomic_replace_property_blob_from_id(crtc->dev, > > + &intel_crtc_state- > > >global_iet, > > + val, > > + sizeof(uint32_t) * > > HISTOGRAM_IET_LENGTH, > > + -1, &replaced); > > + if (replaced) > > + intel_crtc_state->global_iet_changed = true; > > + return 0; > > + } > > + > > + drm_dbg_atomic(&i915->drm, "Unknown property > > [PROP:%d:%s]\n", > > + property->base.id, property->name); > > + return -EINVAL; > > +} > > + > > #define INTEL_CRTC_FUNCS \ > > .set_config = drm_atomic_helper_set_config, \ > > .destroy = intel_crtc_destroy, \ > > @@ -229,7 +326,9 @@ static int intel_crtc_late_register(struct > > drm_crtc > > *crtc) > > .set_crc_source = intel_crtc_set_crc_source, \ > > .verify_crc_source = intel_crtc_verify_crc_source, \ > > .get_crc_sources = intel_crtc_get_crc_sources, \ > > - .late_register = intel_crtc_late_register > > + .late_register = intel_crtc_late_register, \ > > + .atomic_set_property = intel_crtc_set_property, \ > > + .atomic_get_property = intel_crtc_get_property > > > > static const struct drm_crtc_funcs bdw_crtc_funcs = { > > INTEL_CRTC_FUNCS, > > @@ -374,6 +473,10 @@ int intel_crtc_init(struct drm_i915_private > > *dev_priv, enum pipe pipe) > > intel_color_crtc_init(crtc); > > intel_drrs_crtc_init(crtc); > > intel_crtc_crc_init(crtc); > > + intel_histogram_init(crtc); > > + > > + /* Initialize crtc properties */ > > + intel_crtc_add_property(crtc); > > > > cpu_latency_qos_add_request(&crtc->vblank_pm_qos, > > PM_QOS_DEFAULT_VALUE); > > > > @@ -690,3 +793,100 @@ void intel_pipe_update_end(struct > > intel_atomic_state *state, > > out: > > intel_psr_unlock(new_crtc_state); > > } > > + > > +static const struct drm_prop_enum_list histogram_en_names[] = { > > + { INTEL_HISTOGRAM_DISABLE, "Disable" }, > > + { INTEL_HISTOGRAM_ENABLE, "Enable" }, }; > > + > > +/** > > + * intel_attach_histogram_en_property() - add property to > > +enable/disable histogram > > + * @intel_crtc: pointer to the struct intel_crtc on which the global > > +histogram > > is to > > + * be enabled/disabled > > + * > > + * "HISTOGRAM_EN" is the crtc propety to enable/disable global > > +histogram */ void intel_attach_histogram_en_property(struct > > +intel_crtc > > +*intel_crtc) { > > + struct drm_crtc *crtc = &intel_crtc->base; > > + struct drm_device *dev = crtc->dev; > > + struct drm_property *prop; > > + > > + prop = intel_crtc->histogram_en_property; > > + if (!prop) { > > + prop = drm_property_create_enum(dev, 0, > > + "HISTOGRAM_EN", > > + histogram_en_names, > > + > > ARRAY_SIZE(histogram_en_names)); > > + if (!prop) > > + return; > > + > > + intel_crtc->histogram_en_property = prop; > > + } > > + > > + drm_object_attach_property(&crtc->base, prop, 0); } > > + > > +/** > > + * intel_attach_global_iet_property() - add property to write Image > > +Enhancement data > > + * @intel_crtc: pointer to the struct intel_crtc on which global > > +histogram is enabled > > + * > > + * "Global IET" is the crtc property to write the Image Enhancement > > +LUT binary data */ void intel_attach_global_iet_property(struct > > +intel_crtc > > +*intel_crtc) { > > + struct drm_crtc *crtc = &intel_crtc->base; > > + struct drm_device *dev = crtc->dev; > > + struct drm_property *prop; > > + > > + prop = intel_crtc->global_iet_property; > > + if (!prop) { > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB > > | DRM_MODE_PROP_ATOMIC, > > + "Global IET", 0); > > + if (!prop) > > + return; > > + > > + intel_crtc->global_iet_property = prop; > > + } > > + > > + drm_object_attach_property(&crtc->base, prop, 0); } > > + > > +/** > > + * intel_attach_histogram_property() - crtc property to read the > histogram. > > + * @intel_crtc: pointer to the struct intel_crtc on which the global > histogram > > + * was enabled. > > + * "Global Histogram" is the crtc property to read the binary histogram > data. > > + */ > > +void intel_attach_histogram_property(struct intel_crtc *intel_crtc) { > > + struct drm_crtc *crtc = &intel_crtc->base; > > + struct drm_device *dev = crtc->dev; > > + struct drm_property *prop; > > + struct drm_property_blob *blob; > > + > > + prop = intel_crtc->histogram_property; > > + if (!prop) { > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB > > | > > + DRM_MODE_PROP_ATOMIC | > > + DRM_MODE_PROP_IMMUTABLE, > > + "Global Histogram", 0); > > + if (!prop) > > + return; > > + > > + intel_crtc->histogram_property = prop; > > + } > > + blob = drm_property_create_blob(dev, sizeof(uint32_t) * > > HISTOGRAM_BIN_COUNT, NULL); > > + intel_crtc->config->histogram = blob; > > + > > + drm_object_attach_property(&crtc->base, prop, blob->base.id); } > > + > > +int intel_crtc_add_property(struct intel_crtc *intel_crtc) { > > + intel_attach_histogram_en_property(intel_crtc); > > + intel_attach_histogram_property(intel_crtc); > > + intel_attach_global_iet_property(intel_crtc); > > + > > + return 0; > > +} > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h > > b/drivers/gpu/drm/i915/display/intel_crtc.h > > index b615b7ab5ccd..56c6b7c6037e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_crtc.h > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.h > > @@ -7,6 +7,7 @@ > > #define _INTEL_CRTC_H_ > > > > #include <linux/types.h> > > +#include <drm/drm_crtc.h> > > > > enum i9xx_plane_id; > > enum pipe; > > @@ -49,4 +50,8 @@ void intel_wait_for_vblank_if_active(struct > > drm_i915_private *i915, > > enum pipe pipe); > > void intel_crtc_wait_for_next_vblank(struct intel_crtc *crtc); > > > > +int intel_crtc_add_property(struct intel_crtc *intel_crtc); void > > +intel_attach_histogram_en_property(struct intel_crtc *intel_crtc); > > +void intel_attach_global_iet_property(struct intel_crtc *intel_crtc); > > +void intel_attach_histogram_property(struct intel_crtc *intel_crtc); > > #endif > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > b/drivers/gpu/drm/i915/display/intel_display.c > > index c2c388212e2e..94e9f7a71a90 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > @@ -93,6 +93,7 @@ > > #include "intel_fifo_underrun.h" > > #include "intel_frontbuffer.h" > > #include "intel_hdmi.h" > > +#include "intel_histogram.h" > > #include "intel_hotplug.h" > > #include "intel_link_bw.h" > > #include "intel_lvds.h" > > @@ -4324,6 +4325,10 @@ static int intel_crtc_atomic_check(struct > > intel_atomic_state *state, > > if (ret) > > return ret; > > > > + /* HISTOGRAM changed */ > > + if (crtc_state->histogram_en_changed) > > + return intel_histogram_can_enable(crtc); > > + > > return 0; > > } > > > > @@ -7503,6 +7508,14 @@ static void intel_atomic_commit_tail(struct > > intel_atomic_state *state) > > * FIXME get rid of this funny new->old swapping > > */ > > old_crtc_state->dsb = fetch_and_zero(&new_crtc_state- > > >dsb); > > + > > Since there is a wait_for_vblank in this for older platforms only, what is the > expected user space behaviour here for enabling histogram and updating the > iets. > > + /* Re-Visit: HISTOGRAM related stuff */ > > + if (new_crtc_state->histogram_en_changed) > > + intel_histogram_update(crtc, > > + new_crtc_state->histogram_en); > Is there any particular reason that this code is not part of pre plane update? > > + if (new_crtc_state->global_iet_changed) > > + intel_histogram_set_iet_lut(crtc, > > + (u32 *)new_crtc_state- > > >global_iet->data); > > } > > > > /* Underruns don't always raise interrupts, so check manually */ > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > index e0a9c6d8c9b2..e7c33eb76a7e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > @@ -99,6 +99,12 @@ enum intel_broadcast_rgb { > > INTEL_BROADCAST_RGB_LIMITED, > > }; > > > > +/* HISTOGRAM property */ > > +enum intel_histogram_en_prop { > > + INTEL_HISTOGRAM_PROP_DISABLE, > > + INTEL_HISTOGRAM_PROP_ENABLE, > > +}; > > + > > struct intel_fb_view { > > /* > > * The remap information used in the remapped and rotated views to > > @@ -1431,6 +1437,13 @@ struct intel_crtc_state { > > > > /* LOBF flag */ > > bool has_lobf; > > + > > + /* HISTOGRAM data */ > > + int histogram_en; > > + struct drm_property_blob *global_iet; > > + struct drm_property_blob *histogram; > > + bool global_iet_changed; > > + bool histogram_en_changed; > > }; > > > > enum intel_pipe_crc_source { > > @@ -1539,6 +1552,10 @@ struct intel_crtc { > > > > /* histogram data */ > > struct intel_histogram *histogram; > > + /* HISTOGRAM properties */ > > + struct drm_property *histogram_en_property; > > + struct drm_property *global_iet_property; > > + struct drm_property *histogram_property; > > > > #ifdef CONFIG_DEBUG_FS > > struct intel_pipe_crc pipe_crc; > > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c > > b/drivers/gpu/drm/i915/display/intel_histogram.c > > index 8fa3bc74e52b..740019fdf0df 100644 > > --- a/drivers/gpu/drm/i915/display/intel_histogram.c > > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c > > @@ -183,6 +183,7 @@ static void intel_histogram_disable(struct > > intel_crtc > > *intel_crtc) > > > > cancel_delayed_work(&histogram->handle_histogram_int_work); > > histogram->enable = false; > > + intel_crtc->config->histogram_en = false; > > } > > > > int intel_histogram_update(struct intel_crtc *intel_crtc, bool > > enable) > > -- > > 2.25.1
> -----Original Message----- > From: Kulkarni, Vandita <vandita.kulkarni@intel.com> > Sent: Wednesday, September 11, 2024 2:20 PM > To: Kulkarni, Vandita <vandita.kulkarni@intel.com>; Murthy, Arun R > <arun.r.murthy@intel.com>; intel-gfx@lists.freedesktop.org > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > Subject: RE: [PATCH 3/5] Add crtc properties for global histogram > > > -----Original Message----- > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of > > Kulkarni, Vandita > > Sent: Wednesday, September 11, 2024 10:46 AM > > To: Murthy, Arun R <arun.r.murthy@intel.com>; intel- > > gfx@lists.freedesktop.org > > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > > Subject: RE: [PATCH 3/5] Add crtc properties for global histogram > > > > > > > > > -----Original Message----- > > > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf > > > Of Arun R Murthy > > > Sent: Friday, July 5, 2024 3:26 PM > > > To: intel-gfx@lists.freedesktop.org > > > Cc: Murthy, Arun R <arun.r.murthy@intel.com> > > > Subject: [PATCH 3/5] Add crtc properties for global histogram > > > > > > CRTC properties have been added for enable/disable histogram, > > > reading the histogram data and writing the IET data. > > > "HISTOGRAM_EN" is the crtc property to enable/disable the global > > > histogram and takes a value 0/1 accordingly. > > > "Histogram" is a crtc property to read the binary histogram data. > > > "Global IET" is a crtc property to write the IET binary LUT data. > > > > > > Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_atomic.c | 5 + > > > drivers/gpu/drm/i915/display/intel_crtc.c | 202 +++++++++++++++++- > > > drivers/gpu/drm/i915/display/intel_crtc.h | 5 + > > > drivers/gpu/drm/i915/display/intel_display.c | 13 ++ > > > .../drm/i915/display/intel_display_types.h | 17 ++ > > > .../gpu/drm/i915/display/intel_histogram.c | 1 + > > > 6 files changed, 242 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c > > > b/drivers/gpu/drm/i915/display/intel_atomic.c > > > index 76aa10b6f647..693a22089937 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_atomic.c > > > +++ b/drivers/gpu/drm/i915/display/intel_atomic.c > > > @@ -246,6 +246,8 @@ intel_crtc_duplicate_state(struct drm_crtc > > > *crtc) > > > > > > __drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state- uapi); > > > > > > + if (crtc_state->global_iet) > > > + drm_property_blob_get(crtc_state->global_iet); > > > /* copy color blobs */ > > > if (crtc_state->hw.degamma_lut) > > > drm_property_blob_get(crtc_state->hw.degamma_lut); > > > @@ -277,6 +279,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) > > > crtc_state->fb_bits = 0; > > > crtc_state->update_planes = 0; > > > crtc_state->dsb = NULL; > > > + crtc_state->histogram_en_changed = false; > > > > > > return &crtc_state->uapi; > > > } > > > @@ -312,6 +315,8 @@ intel_crtc_destroy_state(struct drm_crtc *crtc, > > > > > > drm_WARN_ON(crtc->dev, crtc_state->dsb); > > > > > > + if (crtc_state->global_iet) > > > + drm_property_blob_put(crtc_state->global_iet); > > > __drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi); > > > intel_crtc_free_hw_state(crtc_state); > > > if (crtc_state->dp_tunnel_ref.tunnel) > > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c > > > b/drivers/gpu/drm/i915/display/intel_crtc.c > > > index 1b578cad2813..24f160359422 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > > > @@ -10,6 +10,7 @@ > > > #include <drm/drm_fourcc.h> > > > #include <drm/drm_plane.h> > > > #include <drm/drm_vblank_work.h> > > > +#include <drm/drm_atomic_uapi.h> > > > > > > #include "i915_vgpu.h" > > > #include "i9xx_plane.h" > > > @@ -26,6 +27,7 @@ > > > #include "intel_drrs.h" > > > #include "intel_dsi.h" > > > #include "intel_fifo_underrun.h" > > > +#include "intel_histogram.h" > > > #include "intel_pipe_crc.h" > > > #include "intel_psr.h" > > > #include "intel_sprite.h" > > > @@ -201,6 +203,7 @@ static struct intel_crtc *intel_crtc_alloc(void) > > > static void intel_crtc_free(struct intel_crtc *crtc) { > > > intel_crtc_destroy_state(&crtc->base, crtc->base.state); > > > + intel_histogram_deinit(crtc); > > > kfree(crtc); > > > } > > > > > > @@ -220,6 +223,100 @@ static int intel_crtc_late_register(struct > > > drm_crtc > > > *crtc) > > > return 0; > > > } > > > > > > +static int intel_crtc_get_property(struct drm_crtc *crtc, > > > + const struct drm_crtc_state *state, > > > + struct drm_property *property, > > > + uint64_t *val) > > > +{ > > > + struct drm_i915_private *i915 = to_i915(crtc->dev); > > > + const struct intel_crtc_state *intel_crtc_state = > > > + to_intel_crtc_state(state); > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > + > > > + if (property == intel_crtc->histogram_en_property) { > > > + *val = intel_crtc_state->histogram_en; > > > + } else if (property == intel_crtc->global_iet_property) { > > > + *val = (intel_crtc_state->global_iet) ? > > > + intel_crtc_state->global_iet->base.id : 0; > > > + } else if (property == intel_crtc->histogram_property) { > > > + *val = (intel_crtc_state->histogram) ? > > > + intel_crtc_state->histogram->base.id : 0; > > > + } else { > > > + drm_err(&i915->drm, > > > + "Unknown property [PROP:%d:%s]\n", > > > + property->base.id, property->name); > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} > > > + > Also to me the below function looks like > drm_property_replace_blob_from_id, why are we not using that, am I > missing something here.. > > Thanks, > Vandita > > > +static int > > > +intel_atomic_replace_property_blob_from_id(struct drm_device *dev, > > > + struct drm_property_blob **blob, > > > + u64 blob_id, > > > + ssize_t expected_size, > > > + ssize_t expected_elem_size, > > > + bool *replaced) > > > +{ > > > + struct drm_property_blob *new_blob = NULL; > > > + > > > + if (blob_id != 0) { > > > + new_blob = drm_property_lookup_blob(dev, blob_id); > > > + if (!new_blob) > > > + return -EINVAL; > > > + > > > + if (expected_size > 0 && > > > + new_blob->length != expected_size) { > > > + drm_property_blob_put(new_blob); > > > + return -EINVAL; > > > + } > > > + if (expected_elem_size > 0 && > > > + new_blob->length % expected_elem_size != 0) { > > > + drm_property_blob_put(new_blob); > > > + return -EINVAL; > > > + } > > > + } > > > + > > > + *replaced |= drm_property_replace_blob(blob, new_blob); > > > + drm_property_blob_put(new_blob); > > > + > > > + return 0; > > > +} > > > + > > > +static int intel_crtc_set_property(struct drm_crtc *crtc, > > > + struct drm_crtc_state *state, > > > + struct drm_property *property, > > > + u64 val) > > > +{ > > > + struct drm_i915_private *i915 = to_i915(crtc->dev); > > > + struct intel_crtc_state *intel_crtc_state = > > > + to_intel_crtc_state(state); > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > + bool replaced = false; > > > + > > > + if (property == intel_crtc->histogram_en_property) { > > > + intel_crtc_state->histogram_en = val; Should this not be set only if the value changes, rather than setting it to true always? > > > + intel_crtc_state->histogram_en_changed = true; > > > + return 0; > > > + } > > > + > > > + if (property == intel_crtc->global_iet_property) { > > > + intel_atomic_replace_property_blob_from_id(crtc->dev, > > > + &intel_crtc_state- > > > >global_iet, > > > + val, > > > + sizeof(uint32_t) * > > > HISTOGRAM_IET_LENGTH, > > > + -1, &replaced); > > > + if (replaced) > > > + intel_crtc_state->global_iet_changed = true; > > > + return 0; > > > + } > > > + > > > + drm_dbg_atomic(&i915->drm, "Unknown property > > > [PROP:%d:%s]\n", > > > + property->base.id, property->name); > > > + return -EINVAL; > > > +} > > > + > > > #define INTEL_CRTC_FUNCS \ > > > .set_config = drm_atomic_helper_set_config, \ > > > .destroy = intel_crtc_destroy, \ > > > @@ -229,7 +326,9 @@ static int intel_crtc_late_register(struct > > > drm_crtc > > > *crtc) > > > .set_crc_source = intel_crtc_set_crc_source, \ > > > .verify_crc_source = intel_crtc_verify_crc_source, \ > > > .get_crc_sources = intel_crtc_get_crc_sources, \ > > > - .late_register = intel_crtc_late_register > > > + .late_register = intel_crtc_late_register, \ > > > + .atomic_set_property = intel_crtc_set_property, \ > > > + .atomic_get_property = intel_crtc_get_property > > > > > > static const struct drm_crtc_funcs bdw_crtc_funcs = { > > > INTEL_CRTC_FUNCS, > > > @@ -374,6 +473,10 @@ int intel_crtc_init(struct drm_i915_private > > > *dev_priv, enum pipe pipe) > > > intel_color_crtc_init(crtc); > > > intel_drrs_crtc_init(crtc); > > > intel_crtc_crc_init(crtc); > > > + intel_histogram_init(crtc); > > > + > > > + /* Initialize crtc properties */ > > > + intel_crtc_add_property(crtc); > > > > > > cpu_latency_qos_add_request(&crtc->vblank_pm_qos, > > > PM_QOS_DEFAULT_VALUE); > > > > > > @@ -690,3 +793,100 @@ void intel_pipe_update_end(struct > > > intel_atomic_state *state, > > > out: > > > intel_psr_unlock(new_crtc_state); > > > } > > > + > > > +static const struct drm_prop_enum_list histogram_en_names[] = { > > > + { INTEL_HISTOGRAM_DISABLE, "Disable" }, > > > + { INTEL_HISTOGRAM_ENABLE, "Enable" }, }; > > > + > > > +/** > > > + * intel_attach_histogram_en_property() - add property to > > > +enable/disable histogram > > > + * @intel_crtc: pointer to the struct intel_crtc on which the > > > +global histogram > > > is to > > > + * be enabled/disabled > > > + * > > > + * "HISTOGRAM_EN" is the crtc propety to enable/disable global > > > +histogram */ void intel_attach_histogram_en_property(struct > > > +intel_crtc > > > +*intel_crtc) { > > > + struct drm_crtc *crtc = &intel_crtc->base; > > > + struct drm_device *dev = crtc->dev; > > > + struct drm_property *prop; > > > + > > > + prop = intel_crtc->histogram_en_property; > > > + if (!prop) { > > > + prop = drm_property_create_enum(dev, 0, > > > + "HISTOGRAM_EN", > > > + histogram_en_names, > > > + > > > ARRAY_SIZE(histogram_en_names)); > > > + if (!prop) > > > + return; > > > + > > > + intel_crtc->histogram_en_property = prop; > > > + } > > > + > > > + drm_object_attach_property(&crtc->base, prop, 0); } > > > + > > > +/** > > > + * intel_attach_global_iet_property() - add property to write Image > > > +Enhancement data > > > + * @intel_crtc: pointer to the struct intel_crtc on which global > > > +histogram is enabled > > > + * > > > + * "Global IET" is the crtc property to write the Image Enhancement > > > +LUT binary data */ void intel_attach_global_iet_property(struct > > > +intel_crtc > > > +*intel_crtc) { > > > + struct drm_crtc *crtc = &intel_crtc->base; > > > + struct drm_device *dev = crtc->dev; > > > + struct drm_property *prop; > > > + > > > + prop = intel_crtc->global_iet_property; > > > + if (!prop) { > > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB > > > | DRM_MODE_PROP_ATOMIC, > > > + "Global IET", 0); > > > + if (!prop) > > > + return; > > > + > > > + intel_crtc->global_iet_property = prop; > > > + } > > > + > > > + drm_object_attach_property(&crtc->base, prop, 0); } > > > + > > > +/** > > > + * intel_attach_histogram_property() - crtc property to read the > > histogram. > > > + * @intel_crtc: pointer to the struct intel_crtc on which the > > > + global > > histogram > > > + * was enabled. > > > + * "Global Histogram" is the crtc property to read the binary > > > + histogram > > data. > > > + */ > > > +void intel_attach_histogram_property(struct intel_crtc *intel_crtc) { > > > + struct drm_crtc *crtc = &intel_crtc->base; > > > + struct drm_device *dev = crtc->dev; > > > + struct drm_property *prop; > > > + struct drm_property_blob *blob; > > > + > > > + prop = intel_crtc->histogram_property; > > > + if (!prop) { > > > + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB > > > | > > > + DRM_MODE_PROP_ATOMIC | > > > + DRM_MODE_PROP_IMMUTABLE, > > > + "Global Histogram", 0); > > > + if (!prop) > > > + return; > > > + > > > + intel_crtc->histogram_property = prop; > > > + } > > > + blob = drm_property_create_blob(dev, sizeof(uint32_t) * > > > HISTOGRAM_BIN_COUNT, NULL); > > > + intel_crtc->config->histogram = blob; > > > + > > > + drm_object_attach_property(&crtc->base, prop, blob->base.id); } > > > + > > > +int intel_crtc_add_property(struct intel_crtc *intel_crtc) { > > > + intel_attach_histogram_en_property(intel_crtc); > > > + intel_attach_histogram_property(intel_crtc); > > > + intel_attach_global_iet_property(intel_crtc); > > > + > > > + return 0; > > > +} > > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h > > > b/drivers/gpu/drm/i915/display/intel_crtc.h > > > index b615b7ab5ccd..56c6b7c6037e 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_crtc.h > > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.h > > > @@ -7,6 +7,7 @@ > > > #define _INTEL_CRTC_H_ > > > > > > #include <linux/types.h> > > > +#include <drm/drm_crtc.h> > > > > > > enum i9xx_plane_id; > > > enum pipe; > > > @@ -49,4 +50,8 @@ void intel_wait_for_vblank_if_active(struct > > > drm_i915_private *i915, > > > enum pipe pipe); > > > void intel_crtc_wait_for_next_vblank(struct intel_crtc *crtc); > > > > > > +int intel_crtc_add_property(struct intel_crtc *intel_crtc); void > > > +intel_attach_histogram_en_property(struct intel_crtc *intel_crtc); > > > +void intel_attach_global_iet_property(struct intel_crtc > > > +*intel_crtc); void intel_attach_histogram_property(struct > > > +intel_crtc *intel_crtc); > > > #endif > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > index c2c388212e2e..94e9f7a71a90 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > @@ -93,6 +93,7 @@ > > > #include "intel_fifo_underrun.h" > > > #include "intel_frontbuffer.h" > > > #include "intel_hdmi.h" > > > +#include "intel_histogram.h" > > > #include "intel_hotplug.h" > > > #include "intel_link_bw.h" > > > #include "intel_lvds.h" > > > @@ -4324,6 +4325,10 @@ static int intel_crtc_atomic_check(struct > > > intel_atomic_state *state, > > > if (ret) > > > return ret; > > > I see that you may have kept it for future use, but I cannot see any practical use for this in this series. And what could be the future use is not mentioned either. > > > + /* HISTOGRAM changed */ > > > + if (crtc_state->histogram_en_changed) > > > + return intel_histogram_can_enable(crtc); > > > + > > > return 0; > > > } > > > > > > @@ -7503,6 +7508,14 @@ static void intel_atomic_commit_tail(struct > > > intel_atomic_state *state) > > > * FIXME get rid of this funny new->old swapping > > > */ > > > old_crtc_state->dsb = fetch_and_zero(&new_crtc_state- > > > >dsb); > > > + > > > > Since there is a wait_for_vblank in this for older platforms only, > > what is the expected user space behaviour here for enabling histogram > > and updating the iets. I have couple of more comments here, since you setting histogram_en_changed to true always, the disable code inside histogram_update will get executed always until someone resets this flag. > > > + /* Re-Visit: HISTOGRAM related stuff */ > > > + if (new_crtc_state->histogram_en_changed) > > > + intel_histogram_update(crtc, > > > + new_crtc_state->histogram_en); > > Is there any particular reason that this code is not part of pre plane update? Had missed couple of comments in the previous reply. Have added it here. Sorry for multiple emails. Thanks Vandita > > > + if (new_crtc_state->global_iet_changed) > > > + intel_histogram_set_iet_lut(crtc, > > > + (u32 *)new_crtc_state- > > > >global_iet->data); > > > } > > > > > > /* Underruns don't always raise interrupts, so check manually */ > > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h > > > b/drivers/gpu/drm/i915/display/intel_display_types.h > > > index e0a9c6d8c9b2..e7c33eb76a7e 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h > > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h > > > @@ -99,6 +99,12 @@ enum intel_broadcast_rgb { > > > INTEL_BROADCAST_RGB_LIMITED, > > > }; > > > > > > +/* HISTOGRAM property */ > > > +enum intel_histogram_en_prop { > > > + INTEL_HISTOGRAM_PROP_DISABLE, > > > + INTEL_HISTOGRAM_PROP_ENABLE, > > > +}; > > > + > > > struct intel_fb_view { > > > /* > > > * The remap information used in the remapped and rotated views to > > > @@ -1431,6 +1437,13 @@ struct intel_crtc_state { > > > > > > /* LOBF flag */ > > > bool has_lobf; > > > + > > > + /* HISTOGRAM data */ > > > + int histogram_en; > > > + struct drm_property_blob *global_iet; > > > + struct drm_property_blob *histogram; > > > + bool global_iet_changed; > > > + bool histogram_en_changed; > > > }; > > > > > > enum intel_pipe_crc_source { > > > @@ -1539,6 +1552,10 @@ struct intel_crtc { > > > > > > /* histogram data */ > > > struct intel_histogram *histogram; > > > + /* HISTOGRAM properties */ > > > + struct drm_property *histogram_en_property; > > > + struct drm_property *global_iet_property; > > > + struct drm_property *histogram_property; > > > > > > #ifdef CONFIG_DEBUG_FS > > > struct intel_pipe_crc pipe_crc; > > > diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c > > > b/drivers/gpu/drm/i915/display/intel_histogram.c > > > index 8fa3bc74e52b..740019fdf0df 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_histogram.c > > > +++ b/drivers/gpu/drm/i915/display/intel_histogram.c > > > @@ -183,6 +183,7 @@ static void intel_histogram_disable(struct > > > intel_crtc > > > *intel_crtc) > > > > > > cancel_delayed_work(&histogram->handle_histogram_int_work); > > > histogram->enable = false; > > > + intel_crtc->config->histogram_en = false; > > > } > > > > > > int intel_histogram_update(struct intel_crtc *intel_crtc, bool > > > enable) > > > -- > > > 2.25.1
> > @@ -7503,6 +7508,14 @@ static void intel_atomic_commit_tail(struct > > intel_atomic_state *state) > > * FIXME get rid of this funny new->old swapping > > */ > > old_crtc_state->dsb = fetch_and_zero(&new_crtc_state- > > >dsb); > > + > > Since there is a wait_for_vblank in this for older platforms only, what is the > expected user space behaviour here for enabling histogram and updating the > iets. This wait is for all the platforms and is required after enabling histogram and before programming the guardband registers. User space will have no impact as this histogram event is not generated immediately. It's a statistics of 'n' number of frames. Thanks and Regards, Arun R Murthy --------------------
> > @@ -7503,6 +7508,14 @@ static void intel_atomic_commit_tail(struct > > intel_atomic_state *state) > > * FIXME get rid of this funny new->old swapping > > */ > > old_crtc_state->dsb = fetch_and_zero(&new_crtc_state- > > >dsb); > > + > > Since there is a wait_for_vblank in this for older platforms only, what is the > expected user space behaviour here for enabling histogram and updating the > iets. This wait is for all the platforms and is required after enabling histogram and before programming the guardband registers. User space will have no impact as this histogram event is not generated immediately. It's a statistics of 'n' number of frames. Thanks and Regards, Arun R Murthy --------------------
> > > +static int intel_crtc_get_property(struct drm_crtc *crtc, > > > + const struct drm_crtc_state *state, > > > + struct drm_property *property, > > > + uint64_t *val) > > > +{ > > > + struct drm_i915_private *i915 = to_i915(crtc->dev); > > > + const struct intel_crtc_state *intel_crtc_state = > > > + to_intel_crtc_state(state); > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > + > > > + if (property == intel_crtc->histogram_en_property) { > > > + *val = intel_crtc_state->histogram_en; > > > + } else if (property == intel_crtc->global_iet_property) { > > > + *val = (intel_crtc_state->global_iet) ? > > > + intel_crtc_state->global_iet->base.id : 0; > > > + } else if (property == intel_crtc->histogram_property) { > > > + *val = (intel_crtc_state->histogram) ? > > > + intel_crtc_state->histogram->base.id : 0; > > > + } else { > > > + drm_err(&i915->drm, > > > + "Unknown property [PROP:%d:%s]\n", > > > + property->base.id, property->name); > > > + return -EINVAL; > > > + } > > > + > > > + return 0; > > > +} > > > + > Also to me the below function looks like drm_property_replace_blob_from_id, > why are we not using that, am I missing something here.. > This function was not exposed from drm when this patch was initially developed. Thanks will make us of this function. Thanks and Regards, Arun R Murthy --------------------
> > > > +static int intel_crtc_set_property(struct drm_crtc *crtc, > > > > + struct drm_crtc_state *state, > > > > + struct drm_property *property, > > > > + u64 val) > > > > +{ > > > > + struct drm_i915_private *i915 = to_i915(crtc->dev); > > > > + struct intel_crtc_state *intel_crtc_state = > > > > + to_intel_crtc_state(state); > > > > + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > > > + bool replaced = false; > > > > + > > > > + if (property == intel_crtc->histogram_en_property) { > > > > + intel_crtc_state->histogram_en = val; > > Should this not be set only if the value changes, rather than setting it to true > always? > > > > + intel_crtc_state->histogram_en_changed = true; This is to flag that user has requested for a change and the value 'true' is used for that. > > > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c > > > > b/drivers/gpu/drm/i915/display/intel_display.c > > > > index c2c388212e2e..94e9f7a71a90 100644 > > > > --- a/drivers/gpu/drm/i915/display/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/display/intel_display.c > > > > @@ -93,6 +93,7 @@ > > > > #include "intel_fifo_underrun.h" > > > > #include "intel_frontbuffer.h" > > > > #include "intel_hdmi.h" > > > > +#include "intel_histogram.h" > > > > #include "intel_hotplug.h" > > > > #include "intel_link_bw.h" > > > > #include "intel_lvds.h" > > > > @@ -4324,6 +4325,10 @@ static int intel_crtc_atomic_check(struct > > > > intel_atomic_state *state, > > > > if (ret) > > > > return ret; > > > > > > I see that you may have kept it for future use, but I cannot see any practical use > for this in this series. > And what could be the future use is not mentioned either. > The selective fetch series of patch is expected to follow up this series of patches. Here we will have to validate the co-ordinates in atomic check. > > > > + /* HISTOGRAM changed */ > > > > + if (crtc_state->histogram_en_changed) > > > > + return intel_histogram_can_enable(crtc); > > > > + > > > > return 0; > > > > } > > > > > > > > @@ -7503,6 +7508,14 @@ static void intel_atomic_commit_tail(struct > > > > intel_atomic_state *state) > > > > * FIXME get rid of this funny new->old swapping > > > > */ > > > > old_crtc_state->dsb = fetch_and_zero(&new_crtc_state- > > > > >dsb); > > > > + > > > > > > > Since there is a wait_for_vblank in this for older platforms only, > > > what is the expected user space behaviour here for enabling > > > histogram and updating the iets. > I have couple of more comments here, since you setting histogram_en_changed > to true always, the disable code inside histogram_update will get executed > always until someone resets this flag. > In crtc_duplicate_state the flag is reset to false. > > > > + /* Re-Visit: HISTOGRAM related stuff */ > > > > + if (new_crtc_state->histogram_en_changed) > > > > + intel_histogram_update(crtc, > > > > + new_crtc_state->histogram_en); > > > > > Is there any particular reason that this code is not part of pre plane update? > Had missed couple of comments in the previous reply. Have added it here. Sorry > for multiple emails. The reason for not having this is pre_plane update is this is a crtc feature and to be enabled at the end. Thanks and Regards, Arun R Murthy --------------------
diff --git a/drivers/gpu/drm/i915/display/intel_atomic.c b/drivers/gpu/drm/i915/display/intel_atomic.c index 76aa10b6f647..693a22089937 100644 --- a/drivers/gpu/drm/i915/display/intel_atomic.c +++ b/drivers/gpu/drm/i915/display/intel_atomic.c @@ -246,6 +246,8 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) __drm_atomic_helper_crtc_duplicate_state(crtc, &crtc_state->uapi); + if (crtc_state->global_iet) + drm_property_blob_get(crtc_state->global_iet); /* copy color blobs */ if (crtc_state->hw.degamma_lut) drm_property_blob_get(crtc_state->hw.degamma_lut); @@ -277,6 +279,7 @@ intel_crtc_duplicate_state(struct drm_crtc *crtc) crtc_state->fb_bits = 0; crtc_state->update_planes = 0; crtc_state->dsb = NULL; + crtc_state->histogram_en_changed = false; return &crtc_state->uapi; } @@ -312,6 +315,8 @@ intel_crtc_destroy_state(struct drm_crtc *crtc, drm_WARN_ON(crtc->dev, crtc_state->dsb); + if (crtc_state->global_iet) + drm_property_blob_put(crtc_state->global_iet); __drm_atomic_helper_crtc_destroy_state(&crtc_state->uapi); intel_crtc_free_hw_state(crtc_state); if (crtc_state->dp_tunnel_ref.tunnel) diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c index 1b578cad2813..24f160359422 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc.c +++ b/drivers/gpu/drm/i915/display/intel_crtc.c @@ -10,6 +10,7 @@ #include <drm/drm_fourcc.h> #include <drm/drm_plane.h> #include <drm/drm_vblank_work.h> +#include <drm/drm_atomic_uapi.h> #include "i915_vgpu.h" #include "i9xx_plane.h" @@ -26,6 +27,7 @@ #include "intel_drrs.h" #include "intel_dsi.h" #include "intel_fifo_underrun.h" +#include "intel_histogram.h" #include "intel_pipe_crc.h" #include "intel_psr.h" #include "intel_sprite.h" @@ -201,6 +203,7 @@ static struct intel_crtc *intel_crtc_alloc(void) static void intel_crtc_free(struct intel_crtc *crtc) { intel_crtc_destroy_state(&crtc->base, crtc->base.state); + intel_histogram_deinit(crtc); kfree(crtc); } @@ -220,6 +223,100 @@ static int intel_crtc_late_register(struct drm_crtc *crtc) return 0; } +static int intel_crtc_get_property(struct drm_crtc *crtc, + const struct drm_crtc_state *state, + struct drm_property *property, + uint64_t *val) +{ + struct drm_i915_private *i915 = to_i915(crtc->dev); + const struct intel_crtc_state *intel_crtc_state = + to_intel_crtc_state(state); + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + + if (property == intel_crtc->histogram_en_property) { + *val = intel_crtc_state->histogram_en; + } else if (property == intel_crtc->global_iet_property) { + *val = (intel_crtc_state->global_iet) ? + intel_crtc_state->global_iet->base.id : 0; + } else if (property == intel_crtc->histogram_property) { + *val = (intel_crtc_state->histogram) ? + intel_crtc_state->histogram->base.id : 0; + } else { + drm_err(&i915->drm, + "Unknown property [PROP:%d:%s]\n", + property->base.id, property->name); + return -EINVAL; + } + + return 0; +} + +static int +intel_atomic_replace_property_blob_from_id(struct drm_device *dev, + struct drm_property_blob **blob, + u64 blob_id, + ssize_t expected_size, + ssize_t expected_elem_size, + bool *replaced) +{ + struct drm_property_blob *new_blob = NULL; + + if (blob_id != 0) { + new_blob = drm_property_lookup_blob(dev, blob_id); + if (!new_blob) + return -EINVAL; + + if (expected_size > 0 && + new_blob->length != expected_size) { + drm_property_blob_put(new_blob); + return -EINVAL; + } + if (expected_elem_size > 0 && + new_blob->length % expected_elem_size != 0) { + drm_property_blob_put(new_blob); + return -EINVAL; + } + } + + *replaced |= drm_property_replace_blob(blob, new_blob); + drm_property_blob_put(new_blob); + + return 0; +} + +static int intel_crtc_set_property(struct drm_crtc *crtc, + struct drm_crtc_state *state, + struct drm_property *property, + u64 val) +{ + struct drm_i915_private *i915 = to_i915(crtc->dev); + struct intel_crtc_state *intel_crtc_state = + to_intel_crtc_state(state); + struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + bool replaced = false; + + if (property == intel_crtc->histogram_en_property) { + intel_crtc_state->histogram_en = val; + intel_crtc_state->histogram_en_changed = true; + return 0; + } + + if (property == intel_crtc->global_iet_property) { + intel_atomic_replace_property_blob_from_id(crtc->dev, + &intel_crtc_state->global_iet, + val, + sizeof(uint32_t) * HISTOGRAM_IET_LENGTH, + -1, &replaced); + if (replaced) + intel_crtc_state->global_iet_changed = true; + return 0; + } + + drm_dbg_atomic(&i915->drm, "Unknown property [PROP:%d:%s]\n", + property->base.id, property->name); + return -EINVAL; +} + #define INTEL_CRTC_FUNCS \ .set_config = drm_atomic_helper_set_config, \ .destroy = intel_crtc_destroy, \ @@ -229,7 +326,9 @@ static int intel_crtc_late_register(struct drm_crtc *crtc) .set_crc_source = intel_crtc_set_crc_source, \ .verify_crc_source = intel_crtc_verify_crc_source, \ .get_crc_sources = intel_crtc_get_crc_sources, \ - .late_register = intel_crtc_late_register + .late_register = intel_crtc_late_register, \ + .atomic_set_property = intel_crtc_set_property, \ + .atomic_get_property = intel_crtc_get_property static const struct drm_crtc_funcs bdw_crtc_funcs = { INTEL_CRTC_FUNCS, @@ -374,6 +473,10 @@ int intel_crtc_init(struct drm_i915_private *dev_priv, enum pipe pipe) intel_color_crtc_init(crtc); intel_drrs_crtc_init(crtc); intel_crtc_crc_init(crtc); + intel_histogram_init(crtc); + + /* Initialize crtc properties */ + intel_crtc_add_property(crtc); cpu_latency_qos_add_request(&crtc->vblank_pm_qos, PM_QOS_DEFAULT_VALUE); @@ -690,3 +793,100 @@ void intel_pipe_update_end(struct intel_atomic_state *state, out: intel_psr_unlock(new_crtc_state); } + +static const struct drm_prop_enum_list histogram_en_names[] = { + { INTEL_HISTOGRAM_DISABLE, "Disable" }, + { INTEL_HISTOGRAM_ENABLE, "Enable" }, +}; + +/** + * intel_attach_histogram_en_property() - add property to enable/disable histogram + * @intel_crtc: pointer to the struct intel_crtc on which the global histogram is to + * be enabled/disabled + * + * "HISTOGRAM_EN" is the crtc propety to enable/disable global histogram + */ +void intel_attach_histogram_en_property(struct intel_crtc *intel_crtc) +{ + struct drm_crtc *crtc = &intel_crtc->base; + struct drm_device *dev = crtc->dev; + struct drm_property *prop; + + prop = intel_crtc->histogram_en_property; + if (!prop) { + prop = drm_property_create_enum(dev, 0, + "HISTOGRAM_EN", + histogram_en_names, + ARRAY_SIZE(histogram_en_names)); + if (!prop) + return; + + intel_crtc->histogram_en_property = prop; + } + + drm_object_attach_property(&crtc->base, prop, 0); +} + +/** + * intel_attach_global_iet_property() - add property to write Image Enhancement data + * @intel_crtc: pointer to the struct intel_crtc on which global histogram is enabled + * + * "Global IET" is the crtc property to write the Image Enhancement LUT binary data + */ +void intel_attach_global_iet_property(struct intel_crtc *intel_crtc) +{ + struct drm_crtc *crtc = &intel_crtc->base; + struct drm_device *dev = crtc->dev; + struct drm_property *prop; + + prop = intel_crtc->global_iet_property; + if (!prop) { + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | DRM_MODE_PROP_ATOMIC, + "Global IET", 0); + if (!prop) + return; + + intel_crtc->global_iet_property = prop; + } + + drm_object_attach_property(&crtc->base, prop, 0); +} + +/** + * intel_attach_histogram_property() - crtc property to read the histogram. + * @intel_crtc: pointer to the struct intel_crtc on which the global histogram + * was enabled. + * "Global Histogram" is the crtc property to read the binary histogram data. + */ +void intel_attach_histogram_property(struct intel_crtc *intel_crtc) +{ + struct drm_crtc *crtc = &intel_crtc->base; + struct drm_device *dev = crtc->dev; + struct drm_property *prop; + struct drm_property_blob *blob; + + prop = intel_crtc->histogram_property; + if (!prop) { + prop = drm_property_create(dev, DRM_MODE_PROP_BLOB | + DRM_MODE_PROP_ATOMIC | + DRM_MODE_PROP_IMMUTABLE, + "Global Histogram", 0); + if (!prop) + return; + + intel_crtc->histogram_property = prop; + } + blob = drm_property_create_blob(dev, sizeof(uint32_t) * HISTOGRAM_BIN_COUNT, NULL); + intel_crtc->config->histogram = blob; + + drm_object_attach_property(&crtc->base, prop, blob->base.id); +} + +int intel_crtc_add_property(struct intel_crtc *intel_crtc) +{ + intel_attach_histogram_en_property(intel_crtc); + intel_attach_histogram_property(intel_crtc); + intel_attach_global_iet_property(intel_crtc); + + return 0; +} diff --git a/drivers/gpu/drm/i915/display/intel_crtc.h b/drivers/gpu/drm/i915/display/intel_crtc.h index b615b7ab5ccd..56c6b7c6037e 100644 --- a/drivers/gpu/drm/i915/display/intel_crtc.h +++ b/drivers/gpu/drm/i915/display/intel_crtc.h @@ -7,6 +7,7 @@ #define _INTEL_CRTC_H_ #include <linux/types.h> +#include <drm/drm_crtc.h> enum i9xx_plane_id; enum pipe; @@ -49,4 +50,8 @@ void intel_wait_for_vblank_if_active(struct drm_i915_private *i915, enum pipe pipe); void intel_crtc_wait_for_next_vblank(struct intel_crtc *crtc); +int intel_crtc_add_property(struct intel_crtc *intel_crtc); +void intel_attach_histogram_en_property(struct intel_crtc *intel_crtc); +void intel_attach_global_iet_property(struct intel_crtc *intel_crtc); +void intel_attach_histogram_property(struct intel_crtc *intel_crtc); #endif diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c index c2c388212e2e..94e9f7a71a90 100644 --- a/drivers/gpu/drm/i915/display/intel_display.c +++ b/drivers/gpu/drm/i915/display/intel_display.c @@ -93,6 +93,7 @@ #include "intel_fifo_underrun.h" #include "intel_frontbuffer.h" #include "intel_hdmi.h" +#include "intel_histogram.h" #include "intel_hotplug.h" #include "intel_link_bw.h" #include "intel_lvds.h" @@ -4324,6 +4325,10 @@ static int intel_crtc_atomic_check(struct intel_atomic_state *state, if (ret) return ret; + /* HISTOGRAM changed */ + if (crtc_state->histogram_en_changed) + return intel_histogram_can_enable(crtc); + return 0; } @@ -7503,6 +7508,14 @@ static void intel_atomic_commit_tail(struct intel_atomic_state *state) * FIXME get rid of this funny new->old swapping */ old_crtc_state->dsb = fetch_and_zero(&new_crtc_state->dsb); + + /* Re-Visit: HISTOGRAM related stuff */ + if (new_crtc_state->histogram_en_changed) + intel_histogram_update(crtc, + new_crtc_state->histogram_en); + if (new_crtc_state->global_iet_changed) + intel_histogram_set_iet_lut(crtc, + (u32 *)new_crtc_state->global_iet->data); } /* Underruns don't always raise interrupts, so check manually */ diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h index e0a9c6d8c9b2..e7c33eb76a7e 100644 --- a/drivers/gpu/drm/i915/display/intel_display_types.h +++ b/drivers/gpu/drm/i915/display/intel_display_types.h @@ -99,6 +99,12 @@ enum intel_broadcast_rgb { INTEL_BROADCAST_RGB_LIMITED, }; +/* HISTOGRAM property */ +enum intel_histogram_en_prop { + INTEL_HISTOGRAM_PROP_DISABLE, + INTEL_HISTOGRAM_PROP_ENABLE, +}; + struct intel_fb_view { /* * The remap information used in the remapped and rotated views to @@ -1431,6 +1437,13 @@ struct intel_crtc_state { /* LOBF flag */ bool has_lobf; + + /* HISTOGRAM data */ + int histogram_en; + struct drm_property_blob *global_iet; + struct drm_property_blob *histogram; + bool global_iet_changed; + bool histogram_en_changed; }; enum intel_pipe_crc_source { @@ -1539,6 +1552,10 @@ struct intel_crtc { /* histogram data */ struct intel_histogram *histogram; + /* HISTOGRAM properties */ + struct drm_property *histogram_en_property; + struct drm_property *global_iet_property; + struct drm_property *histogram_property; #ifdef CONFIG_DEBUG_FS struct intel_pipe_crc pipe_crc; diff --git a/drivers/gpu/drm/i915/display/intel_histogram.c b/drivers/gpu/drm/i915/display/intel_histogram.c index 8fa3bc74e52b..740019fdf0df 100644 --- a/drivers/gpu/drm/i915/display/intel_histogram.c +++ b/drivers/gpu/drm/i915/display/intel_histogram.c @@ -183,6 +183,7 @@ static void intel_histogram_disable(struct intel_crtc *intel_crtc) cancel_delayed_work(&histogram->handle_histogram_int_work); histogram->enable = false; + intel_crtc->config->histogram_en = false; } int intel_histogram_update(struct intel_crtc *intel_crtc, bool enable)
CRTC properties have been added for enable/disable histogram, reading the histogram data and writing the IET data. "HISTOGRAM_EN" is the crtc property to enable/disable the global histogram and takes a value 0/1 accordingly. "Histogram" is a crtc property to read the binary histogram data. "Global IET" is a crtc property to write the IET binary LUT data. Signed-off-by: Arun R Murthy <arun.r.murthy@intel.com> --- drivers/gpu/drm/i915/display/intel_atomic.c | 5 + drivers/gpu/drm/i915/display/intel_crtc.c | 202 +++++++++++++++++- drivers/gpu/drm/i915/display/intel_crtc.h | 5 + drivers/gpu/drm/i915/display/intel_display.c | 13 ++ .../drm/i915/display/intel_display_types.h | 17 ++ .../gpu/drm/i915/display/intel_histogram.c | 1 + 6 files changed, 242 insertions(+), 1 deletion(-)