Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932467AbdCIPrf (ORCPT ); Thu, 9 Mar 2017 10:47:35 -0500 Received: from bhuna.collabora.co.uk ([46.235.227.227]:36649 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754641AbdCIPre (ORCPT ); Thu, 9 Mar 2017 10:47:34 -0500 Subject: Re: [PATCH RESEND] drm: dw_hdmi: Don't rely on the status of the bridge for updating HPD To: Jose Abreu , Archit Taneja , David Airlie References: <20170308081524.7672-1-romain.perier@collabora.com> Cc: Heiko Stuebner , dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Sjoerd Simons , Daniel Stone From: Romain Perier Message-ID: <8699114e-6185-c6a2-8110-9002317b8cc4@collabora.com> Date: Thu, 9 Mar 2017 16:37:30 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3355 Lines: 86 Hello, Le 09/03/2017 ? 15:28, Jose Abreu a ?crit : > Hi Romain, > > > On 08-03-2017 08:15, Romain Perier wrote: >> Currently, the irq handler that monitores changes for HPD anx RX_SENSE >> relies on the status of the bridge for updating the status of the HPD. >> The update is done only when the bridge is enabled. >> >> However, on Rockchip platforms we have found use cases where it could be >> a problem. When HDMI is being used, turning off/on the screen or >> unplugging/re-plugging the cable, the following simplified code path >> will happen: >> >> - dw_hdmi_irq() will be triggered by an HPD event, as the bridge is on >> hdmi->disabled is false, then the handler will update the rxsense flag >> accordingly. >> - dw_hdmi_update_power() will be invoked with the mode >> DRM_FORCE_UNSPECIFIED and rxsense == 1, so dw_hdmi_poweroff() will be >> called and the PHY will be desactivated (its pixel clocks and TMDS) >> >> [...] >> >> - dw_hdmi_bridge_disable() will be invoked, the bridge will be marked as >> disabled. >> >> - dw_hdmi_irq() will be triggered by an HPD event, as the bridge is >> currently disabled the HPD status won't be updated, so hdmi->rxsense >> won't be changed. Even if the data part of the PHY is disabled, this >> information coming from the HDMI Transmitter is correct and should be >> saved. >> >> [...] >> >> - dw_hdmi_bridge_enable() will be invoked, the bridge will be marked as >> enabled. >> - dw_hdmi_update_power() will be called. As hdmi->force will be equal to >> DRM_FORCE_UNSPECIFIED the function will rely on hdmi->rxsense. This >> field has not been updated by the irq handler, so it will be false and >> DRM_FORCE_ON won't be put to hdmi->force. >> >> Consequently, most of the time dw_hdmi_poweron() won't be called in this >> use case, TMDS won't be re-enabled the PHY won't be re-initialized, >> resulting in a "Signal not found". >> >> This commit fixes the issue by removing the check for "!hdmi->disabled". >> As already explained, even if the PHY is partially disabled, information >> coming from HDMI Transmitter about HPD should be saved for a later use. >> >> Signed-off-by: Romain Perier >> --- >> >> Note: Due to an email configuration issue, some of my patches were not >> received on infradead.org or vger.kernel.org. It is now fixed, so I >> resend this patch for this reason. >> >> drivers/gpu/drm/bridge/dw-hdmi.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c >> index 235ce7d..b621fc7 100644 >> --- a/drivers/gpu/drm/bridge/dw-hdmi.c >> +++ b/drivers/gpu/drm/bridge/dw-hdmi.c >> @@ -1790,7 +1790,7 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) >> if (intr_stat & >> (HDMI_IH_PHY_STAT0_RX_SENSE | HDMI_IH_PHY_STAT0_HPD)) { >> mutex_lock(&hdmi->mutex); >> - if (!hdmi->disabled && !hdmi->force) { >> + if (!hdmi->force) { >> /* >> * If the RX sense status indicates we're disconnected, >> * clear the software rxsense status. > Makes sense but I think you need to make sure that phy will not > be enabled when bridge is disabled i.e. you should update rxsense > only. > > Best regards, > Jose Miguel Abreu I agree with Russel, dw_hdmi_update_phy_mask already checks that. So it should not be enabled, imho. Regards, Romain