From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [PATCH 2/4] drm/i915: protect backlight registers and data with a spinlock Date: Mon, 15 Apr 2013 14:03:04 +0100 Message-ID: <20130415130304.GA4106@cantiga.alporthouse.com> References: Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id E0403E600C for ; Mon, 15 Apr 2013 06:03:14 -0700 (PDT) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Jani Nikula Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Fri, Apr 12, 2013 at 03:18:37PM +0300, Jani Nikula wrote: > Backlight data and registers are fiddled through LVDS/eDP modeset > enable/disable hooks, backlight sysfs files, asle interrupts, and register > save/restore. Protect the backlight related registers and driver private > fields using a spinlock. > > The locking in register save/restore covers a little more than is strictly > necessary, including non-modeset case, for simplicity. > > v2: Cover register access, save/restore, i915_read_blc_pwm_ctl() and code > paths leading there. > > Signed-off-by: Jani Nikula Looks reasonable. intel_panel_actually_set_backlight() should have a WARN_ON(!spinlocked); The irqness of the register writes scares me slightly - since the IRQ in question is from ACPI and we have a few bug reports along the lines of "backlight makes the entire system sluggish" i.e. commonly associated with bad interrupt handling. Whilst you are looking at updating the backlight programming, can you look at pushing the writes from out of the interrupt handler? -Chris -- Chris Wilson, Intel Open Source Technology Centre