From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 32BF9C433FE for ; Mon, 25 Oct 2021 06:23:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 1AE6060EE3 for ; Mon, 25 Oct 2021 06:23:07 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230395AbhJYGZ2 (ORCPT ); Mon, 25 Oct 2021 02:25:28 -0400 Received: from ip-15.mailobj.net ([213.182.54.15]:50096 "EHLO msg-4.mailo.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230295AbhJYGZ1 (ORCPT ); Mon, 25 Oct 2021 02:25:27 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=simple/simple; d=net-c.es; s=mailo; t=1635113882; bh=uw6jUOdznN5Fu1cmQxsbGLSaS5P2w7SplYgPex568YQ=; h=X-EA-Auth:Date:From:To:Cc:Subject:Message-ID:References: MIME-Version:Content-Type:Content-Transfer-Encoding:In-Reply-To; b=kpn3YOQUKeK9qDpnf0StFTwdc4WJreIl9JR8oi4rUbw2RZC0IEtUV7NQk5CjR3JWj AEBLg5lA/FkJuQmNxIv/k09YqvRAHZmzYfR2B2Hv/gSnP/O9JktEiJjGBVqLmzKzGR /BmdwJXbj2xTfAUE0fjrTjyaO9MnP/V/8Ufrnq/A= Received: by b-6.in.mailobj.net [192.168.90.16] with ESMTP via ip-206.mailobj.net [213.182.55.206] Mon, 25 Oct 2021 00:17:45 +0200 (CEST) X-EA-Auth: tM23DGE6RIEgIBepFFyFZ3U+VcDqeb1NZ+9uNCAhSimMRHbJIRvea/6qSymDwthzabP0gvaNwIR0mJ56UkO3tKDYzFIv2ik3 Date: Mon, 25 Oct 2021 00:17:37 +0200 From: Claudio Suarez To: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Cc: dri-devel@lists.freedesktop.org, amd-gfx@lists.freedesktop.org, linux-tegra@vger.kernel.org, intel-gfx@lists.freedesktop.org, David Airlie , Daniel Vetter , Laurent Pinchart , Jani Nikula , Joonas Lahtinen , Rodrigo Vivi , Alex Deucher , Christian =?iso-8859-1?Q?K=F6nig?= , Pan Xinhui , Emma Anholt , Maxime Ripard , Thierry Reding , Patrik Jakobsson , Jingoo Han , Rob Clark , Sean Paul , linux-arm-msm@vger.kernel.org, freedreno@lists.freedesktop.org, Chen-Yu Tsai , Sandy Huang , heiko@sntech.de, Neil Armstrong , Robert Foss , Ben Skeggs , nouveau@lists.freedesktop.org Subject: Re: [Intel-gfx] [PATCH v3 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi Message-ID: References: <20211016184226.3862-1-cssk@net-c.es> <20211016184226.3862-14-cssk@net-c.es> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-arm-msm@vger.kernel.org On Fri, Oct 22, 2021 at 03:22:57PM +0300, Ville Syrjälä wrote: > On Fri, Oct 22, 2021 at 03:01:52PM +0300, Ville Syrjälä wrote: > > On Fri, Oct 22, 2021 at 12:25:33PM +0200, Claudio Suarez wrote: > > > On Thu, Oct 21, 2021 at 04:49:59PM +0300, Ville Syrjälä wrote: > > > > On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote: > > > > > drm_get_edid() internally calls to drm_connector_update_edid_property() > > > > > and then drm_add_display_info(), which parses the EDID. > > > > > This happens in the function intel_hdmi_set_edid() and > > > > > intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()). > > > > > > > > > > Once EDID is parsed, the monitor HDMI support information is available > > > > > through drm_display_info.is_hdmi. Retriving the same information with > > > > > drm_detect_hdmi_monitor() is less efficient. Change to > > > > > drm_display_info.is_hdmi > > > > > > > > I meant we need to examine all call chains that can lead to > > > > .detect() to make sure all of them do in fact update the > > > > display_info beforehand. > > > > > > Well, I studied it carefully and, yes, all call chains that can lead to > > > drm_display_info.is_hdmi / drm_detect_hdmi_monitor() update display_info > > > beforehand. In the case that this doesn't happen, the code is unchanged. > > > > > > Do you want I explain the changes in the code here again ? Or do you want > > > to me change the commit message to be more clear ? In the first case, I can > > > write here a detailed explanation. In the second case I can make a longer commit > > > message. > > > > > > Or both? > > > > I want all those call chains explained in the commit message, > > otherwise I have no easy way to confirm whether the change > > is correct or not. > > Hmm. OK, so I had a bit of a dig around and seems that what we do now > .detect()->drm_get_edid()->drm_connector_update_edid_property()->drm_add_display_info() Yes. I said before that I felt something was wrong when I read the documentation and then the code. To be more explicit now, I expected that drm_connector_update_edid_property() will be done in the fill_modes/get_modes phase instead of when reading the edid. The documentation suggests that but the code reads the edid in the detect phase. Now, since drm_connector_update_edid_property() is called in the detect phase, it is not necessary to keep the edid data in the private connector struct. It is in struct drm_connector from the beginning. But this is topic for another patch. > Now the question is when did that start happening? Looks like it was > commit 4b4df570b41d ("drm: Update edid-derived drm_display_info fields > at edid property set [v2]") that started to call drm_add_display_info() > from drm_connector_update_edid_property(), and then commit 5186421cbfe2 > ("drm: Introduce epoch counter to drm_connector") started to call > drm_connector_update_edid_property() from drm_get_edid(). Before both > of those commits were in place display_info would still contain > some stale garbage during .detect(). > > That is the story I think we want in these commit messages since it > a) explains why the old code was directly parsing the edid instead > b) why it's now safe to change this ------------------commit-message?-------------------- drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi Commit a92d083d08b0 created the new flag is_hdmi in drm_display_info which is set when sink compliant with CEA-861 (EDID) shall be treated as an HDMI sink. >From that day, this value can be used in some cases instead of calling drm_detect_hdmi_monitor() and a second parse is avoided because drm_detect_hdmi_monitor() parses. A TODO task was registered in Documentation/gpu/todo.rst to perform that task in the future. The flag drm_display_info.is_hdmi is set in the function drm_add_display_info(), which is called from drm_connector_update_edid_property(). Since commit 5186421cbfe2, drm_get_edid() calls drm_connector_update_edid_property() when reading the edid data from an i2c adapter. Therefore, in these cases drm_display_info.is_hdmi is updated to its correct value when returning from drm_get_edid(). Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi in the cases when drm_detect_hdmi_monitor() is called after a read from an i2c adapter using drm_get_edid() in the i915 driver. ----------------------------------------------- > > PS. connector->force handling in drm_get_edid() looks a bit busted > since it doesn't call drm_connector_update_edid_property() at all > in some cases. I think there might be some path that leads there > anywya if/when we change connector->force, but we should fix > drm_get_edid() to do the right thing regarless. In those cases, the edid isn't read and NULL is returned by drm_get_edid(). No problem because display_info.is_hdmi is inside an if (edid != NULL). BTW, struct intel_connector is allocated with kzalloc, so the initial value of is_hdmi is zero. The connector isn't HDMI by default. BR. Claudio Suarez.