From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 17/20] drm: remove checks for same value in set_prop Date: Wed, 01 Feb 2012 12:23:50 +0000 Message-ID: References: <1328092718-28951-1-git-send-email-s.hauer@pengutronix.de> <1328092718-28951-18-git-send-email-s.hauer@pengutronix.de> <20120201121344.GR1990@pengutronix.de> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by gabe.freedesktop.org (Postfix) with ESMTP id F0E1D9E78D for ; Wed, 1 Feb 2012 04:23:55 -0800 (PST) In-Reply-To: <20120201121344.GR1990@pengutronix.de> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Sascha Hauer Cc: kernel@pengutronix.de, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On Wed, 1 Feb 2012 13:13:44 +0100, Sascha Hauer 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 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