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