From patchwork Wed Nov 23 03:50:22 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Archit Taneja X-Patchwork-Id: 9442555 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 59F35600BA for ; Wed, 23 Nov 2016 03:50:38 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4CEB01FFCA for ; Wed, 23 Nov 2016 03:50:38 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4190220700; Wed, 23 Nov 2016 03:50:38 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 9DF321FFCA for ; Wed, 23 Nov 2016 03:50:37 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id C68E86E064; Wed, 23 Nov 2016 03:50:30 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from smtp.codeaurora.org (smtp.codeaurora.org [198.145.29.96]) by gabe.freedesktop.org (Postfix) with ESMTPS id 556496E064 for ; Wed, 23 Nov 2016 03:50:29 +0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 1000) id 31CF061556; Wed, 23 Nov 2016 03:50:28 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1479873028; bh=BpUxp8DXimoebLgySBYD0pfyiueweCrNhyuNFCWqp80=; h=Subject:To:References:Cc:From:Date:In-Reply-To:From; b=BXf3UwjJYy5ulDcVWmxrQ+d5PioSnA/3Xr3hckcVzxitLpgNqq7TCw8zYm4Cw1bsO bJDt9OzlMbKj8ruTUFApW31R857isVIcOOKtjwV4uN0Nc1pOQX4BIN9V9w0X3dEgwd TLdZ+/EPQu6Gu0+pu1Lt8PxID1jQ6UznhVoWHza0= Received: from [10.79.40.55] (unknown [202.46.23.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: architt@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 3AA6F61503; Wed, 23 Nov 2016 03:50:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=codeaurora.org; s=default; t=1479873027; bh=BpUxp8DXimoebLgySBYD0pfyiueweCrNhyuNFCWqp80=; h=Subject:To:References:Cc:From:Date:In-Reply-To:From; b=Br0yszlNBmHphEUtdo13J0f7WdFKGKIXJyacJ3skoUB3AYp127nfXXd7fngilolmf rnZNtU50/EQpr3hhpt0lZfdyjmhl8HVRBfQjsQWXjzp818Eg1+5PXomgBfYadWP/zu xI5ckEMNE9r54F8wDW4Fy75Bf2j9nTv/M6GBWwbA= DMARC-Filter: OpenDMARC Filter v1.3.1 smtp.codeaurora.org 3AA6F61503 Authentication-Results: pdx-caf-mail.web.codeaurora.org; dmarc=none header.from=codeaurora.org Authentication-Results: pdx-caf-mail.web.codeaurora.org; spf=pass smtp.mailfrom=architt@codeaurora.org Subject: Re: [RFC][PATCH 1/3] drm/bridge: adv7511: Rework adv7511_power_on/off() so they can be reused internally To: John Stultz , Laurent Pinchart References: <1479775052-28194-1-git-send-email-john.stultz@linaro.org> <1961666.AZMl081OYz@avalon> <1661379.b8y5I25JKx@avalon> From: Archit Taneja Message-ID: <1235475f-891d-ea13-4c8a-dcbe21c57a47@codeaurora.org> Date: Wed, 23 Nov 2016 09:20:22 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: Cc: Wolfram Sang , lkml , "dri-devel@lists.freedesktop.org" X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP On 11/23/2016 01:16 AM, John Stultz wrote: > On Tue, Nov 22, 2016 at 9:38 AM, Laurent Pinchart > wrote: >> Hi John, >> >> On Tuesday 22 Nov 2016 09:25:22 John Stultz wrote: >>> On Tue, Nov 22, 2016 at 12:14 AM, Laurent Pinchart wrote: >>>> On Monday 21 Nov 2016 16:37:30 John Stultz wrote: >>> @@ -545,24 +554,13 @@ static int adv7511_get_modes(struct adv7511 *adv7511, >>>>> unsigned int count; >>>>> >>>>> /* Reading the EDID only works if the device is powered */ >>>>> >>>>> - if (!adv7511->powered) { >>>>> - regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER, >>>>> - ADV7511_POWER_POWER_DOWN, 0); >>>>> - if (adv7511->i2c_main->irq) { >>>>> - regmap_write(adv7511->regmap, >>>>> ADV7511_REG_INT_ENABLE(0), >>>>> - ADV7511_INT0_EDID_READY); >>>>> - regmap_write(adv7511->regmap, >>>>> ADV7511_REG_INT_ENABLE(1), >>>>> - ADV7511_INT1_DDC_ERROR); >>>>> - } >>>>> - adv7511->current_edid_segment = -1; >>>>> - } >>>>> + if (!adv7511->powered) >>>>> + __adv7511_power_on(adv7511); >>>> >>>> The __adv7511_power_on() function does more than the above, in particular >>>> it performs an expensive regcache_sync() and calls adv7533_dsi_power_on() >>>> for the ADV7533. Don't those operations have side effects that are either >>>> not wanted or not needed here ? In any case this patch modifies the >>>> behaviour of the driver, which needs to be documented in the kernel >>>> message. >>> >>> So yes, while the adv7533 bits aren't needed in the internal function, >>> I'm finding the logic to pulse the HPD and the regcache_sync call seem >>> to be needed side effects, as without that logic, I get i2c_transfer() >>> errors in adv7511_get_edid_block(). >> >> Does this patch fix the problem without requiring the 200ms delay ? >> >>> I'll try to rework this patch to split the two changes of reworking >>> the power_on/off function to be re-used (with no logic chage), and the >>> patch to reuse it in get_modes() which resolves a bug. >> >> Have you identified which register write fixes your problem here ? > > So I basically used regmap_sync_region() to bisect through the > registers to try to figure out which one calling sync on helps avoid > the issue, and I've narrowed it down to 0x43 > (ADV7511_REG_EDID_I2C_ADDR). I guess if this register loses its state, then i2c errors are expected. > > If instead of the proposed patch here, I use the following patch > (copy/paste whitespace damage, apologies) seems to make things work > reliably: > > diff --git a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > index 8dba729..87403d7 100644 > --- a/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > +++ b/drivers/gpu/drm/bridge/adv7511/adv7511_drv.c > > @@ -555,14 +557,18 @@ static int adv7511_get_modes(struct adv7511 *adv7511, > ADV7511_INT1_DDC_ERROR); > } > adv7511->current_edid_segment = -1; > + regcache_sync_region(adv7511->regmap, 0x43, 0x43); So, we're losing register state when get_modes() is called, or sometime before it. Could you try to read a register with a known programmed value at the beginning of adv7511_get_modes(), and check if it has already lost the state or not? It's also possible that, in adv7511_get_modes(), when the chip is powered on, i.e, when we call: regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER, ADV7511_POWER_POWER_DOWN, 0); the monitor wakes up, but there is some additional hpd toggling, which results in registers to lose their state. The patch below is something that was originally there in Lars's initial patch for ADV7533 support. I'd dropped it since it didn't have much to do with ADV7533 itself. It should at least discard any HPD toggling caused by powering on the chip in the next line. > + > } > > edid = drm_do_get_edid(connector, adv7511_get_edid_block, adv7511); > > - if (!adv7511->powered) > + if (!adv7511->powered) { > regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER, > ADV7511_POWER_POWER_DOWN, > ADV7511_POWER_POWER_DOWN); > + regcache_mark_dirty(adv7511->regmap); > + } > > kfree(adv7511->edid); > adv7511->edid = edid; > > > But I suspect this isn't a proper fix. I tried adding > ADV7511_REG_EDID_I2C_ADDR to the volatile register list, but that > didn't seem to effect anything (and I still see problematic behavior > if I'm not explictly syncing it as above). > > Thoughts on what the right thing is to do? > -john > diff --git a/drivers/gpu/drm/i2c/adv7511.c b/drivers/gpu/drm/i2c/adv7511.c index ed6d25d..5ee40a4 100644 --- a/drivers/gpu/drm/i2c/adv7511.c +++ b/drivers/gpu/drm/i2c/adv7511.c @@ -654,6 +654,9 @@ static int adv7511_get_modes(struct adv7511 *adv7511, /* Reading the EDID only works if the device is powered */ if (!adv7511->powered) { + regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER2, + ADV7511_REG_POWER2_HPD_SRC_MASK, + ADV7511_REG_POWER2_HPD_SRC_NONE); regmap_update_bits(adv7511->regmap, ADV7511_REG_POWER, ADV7511_POWER_POWER_DOWN, 0); if (adv7511->i2c_main->irq) {