Re: [PATCH 17/20] drm: remove checks for same value in set_prop

[Chris Wilson <chris@chris-wilson.co.uk>]

On Wed, 1 Feb 2012 13:13:44 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Wed, Feb 01, 2012 at 11:55:44AM +0000, Chris Wilson wrote:
> > On Wed,  1 Feb 2012 11:38:35 +0100, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > The drivers currently check in set_property whether the property is
> > > unchanged. move this check into the core and do not bother the drivers
> > > with checking for unchanged properties.
> > 
> > This patch seems to have functional side-effects beyond the description.
> > 
> > For example,
> > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > > index db3b461..0024b59 100644
> > > --- a/drivers/gpu/drm/i915/intel_dp.c
> > > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > > @@ -2223,9 +2223,6 @@ intel_dp_set_property(struct drm_connector *connector,
> > >  		int i = val;
> > >  		bool has_audio;
> > >  
> > > -		if (i == intel_dp->force_audio)
> > > -			return 0;
> > > -
> > 
> > Here we are checking against the current value of the intel_dp, which
> > may in theory be modified elsewhere as well, and avoiding the modeswitch
> > if it is unnecessary.
> 
> In case of force_audio I just checked that the current value is not
> changed elsewhere, but I must admit that I haven't checked this for
> all other properties. Would the patch be ok if I review for side
> effects?
> 
> BTW if someone changes the underlying variable of a property outside
> of the .set_property callback there are likely problems elsewhere
> because without calling drm_connector_property_set_value the values
> of the variable and the property backing store will be inconsistent.
> The calls to drm_connector_property_set_value in turn are quite easy
> reviewable, those are very few.

Sure, it just jumped out as being not in the same pattern as the rest.
I'd break out the ones that are different, like intel_dp, into their own
patch so that the above analysis can be recorded along with the change
and that we have something easy to bisect/change-our-minds about later.
;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre