[PATCH 3/5] Add crtc properties for global histogram

[Murthy, Arun R]

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