> > > > +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
--------------------
