[PATCH 3/5] Add crtc properties for global histogram
Kulkarni, Vandita
vandita.kulkarni at intel.com
Wed Sep 11 09:46:01 UTC 2024
> -----Original Message-----
> From: Kulkarni, Vandita <vandita.kulkarni at intel.com>
> Sent: Wednesday, September 11, 2024 2:20 PM
> To: Kulkarni, Vandita <vandita.kulkarni at intel.com>; Murthy, Arun R
> <arun.r.murthy at intel.com>; intel-gfx at lists.freedesktop.org
> Cc: Murthy, Arun R <arun.r.murthy at intel.com>
> Subject: RE: [PATCH 3/5] Add crtc properties for global histogram
>
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf Of
> > Kulkarni, Vandita
> > Sent: Wednesday, September 11, 2024 10:46 AM
> > To: Murthy, Arun R <arun.r.murthy at intel.com>; intel-
> > gfx at lists.freedesktop.org
> > Cc: Murthy, Arun R <arun.r.murthy at intel.com>
> > Subject: RE: [PATCH 3/5] Add crtc properties for global histogram
> >
> >
> >
> > > -----Original Message-----
> > > From: Intel-gfx <intel-gfx-bounces at lists.freedesktop.org> On Behalf
> > > Of Arun R Murthy
> > > Sent: Friday, July 5, 2024 3:26 PM
> > > To: intel-gfx at lists.freedesktop.org
> > > Cc: Murthy, Arun R <arun.r.murthy at 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 at 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
More information about the Intel-gfx
mailing list