Message ID | 20221021130637.1.I8c2de0954a4e54e0c59a72938268e2ead91daa98@changeid |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:4242:0:0:0:0:0 with SMTP id s2csp883796wrr; Fri, 21 Oct 2022 13:14:15 -0700 (PDT) X-Google-Smtp-Source: AMsMyM50MqKYwhtBhvScaOyYHEbX/41j987NovozU3mX3jV4zbgYT70JkeCCQyVWuX2uzFXUejI1 X-Received: by 2002:a17:903:2284:b0:185:3948:be7c with SMTP id b4-20020a170903228400b001853948be7cmr21208826plh.51.1666383254678; Fri, 21 Oct 2022 13:14:14 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1666383254; cv=none; d=google.com; s=arc-20160816; b=XXebCW5qB8T91f+xqMjhB55RkTBQFlpnQH/ibtUQbkOJGxZF47W8Bn9in9ITte8828 0I/DTEnojlFqsN7jTaWfMl4+93a8kF5HP5dv2dvTcYIwYIUtQV2l6zZ0QhdBOIAm25si 35iFlYSLYyajuzj1cOYtyAJaGhr0V3LvuB1r+hysJW2pXSvS5frwdWlDf+AUyG6X8OuS ZmqcHUTcSwMzIh6It51Joz/Ije0/p2KH9mtcep484ujyIyEmPIDwXLLpRop1tME9zpQ4 sx7jPuUIza1yepqP0BtkUhVFxoJjPFDuJeklgi9MltkTY/yveK9KMrPqyMRPmqZlyvrc fCJw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=Vh4YIgtLxUw6WgoDzxlF5MErsV4sylhzFrfQBBr3GjE=; b=fGLPGlnnP3DqMVkEatku5SZyWg6QM81Z94QVimplD7k6DIqM/4jD+vhWXSwR9r0EFh qrCkwNzK+7mn7UX+8z42cpm9QWrX6SsGJVVAEsK13TC09Aw7G1uV/Y6ePzEdOLnJdrcu UErqH06lINT0LN6I+WJ4VDLJ59S8IhmBLwWt6jlRej/yMTth6i2HGOho9T6yj8BoFCso h9/fF2IScK6Z/miqFPPsfjwQp9jVtADmAkadQyGdX/JL0gdjga9NYI9e5TVpImaPnA82 faulARry4kIXl3aAg8f+hTa5bfuccLhhvuy/WHT9nlXBrgX/OHfT0m5EHKMsRw166BnY 1ZSw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=in9tvOX8; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id 129-20020a630087000000b0045ff2a2f56csi25259434pga.546.2022.10.21.13.14.00; Fri, 21 Oct 2022 13:14:14 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@chromium.org header.s=google header.b=in9tvOX8; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=chromium.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229716AbiJUUHe (ORCPT <rfc822;mntrajkot1@gmail.com> + 99 others); Fri, 21 Oct 2022 16:07:34 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:33336 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229484AbiJUUHc (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 21 Oct 2022 16:07:32 -0400 Received: from mail-pj1-x102d.google.com (mail-pj1-x102d.google.com [IPv6:2607:f8b0:4864:20::102d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 93F2F10B5F for <linux-kernel@vger.kernel.org>; Fri, 21 Oct 2022 13:07:31 -0700 (PDT) Received: by mail-pj1-x102d.google.com with SMTP id a5-20020a17090aa50500b002008eeb040eso5635393pjq.1 for <linux-kernel@vger.kernel.org>; Fri, 21 Oct 2022 13:07:31 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=Vh4YIgtLxUw6WgoDzxlF5MErsV4sylhzFrfQBBr3GjE=; b=in9tvOX8bP32FVB9PxADmtd00bHTP0G4N3hxe3LC3/czC3+AWMUbkfNVT5VKN6rpMn /FRxKs2wOQFDbegwIP+zAXvI1//2TkDnk8DmFTbVZOInjumlV9KGNaPX4xjgQ0/ocm8P h8TDMtEEctwPCFhtwjSSryElS7YDqI2e3aV+U= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Vh4YIgtLxUw6WgoDzxlF5MErsV4sylhzFrfQBBr3GjE=; b=rzSGOTWiDPB818pMQQoMUb5nILYLjbb/OIigDpTXyAD2RqAwhia/KT6UsYkQeYrEBp uh4BKi9i3Lb1YhGSGqdqWEee15xCKhWZwlETV8BbAK4fwCii5Iw64BQHI15SpbZx7Cp3 a3qx58kcVGancwNMvVi4e377pnxym3cxq2qavzViMcG3HE7CBC+1ILN3yXvdrDsZ9Md3 x3HQx1/XkfIEqRnAMzPPFAmDav8WinTs0aKa4E8s/sCnZSXAxUe78W7KcgFwgloG4Ct7 AdQC3o/2Z8p7yKj7YBN0ZcoP2wS0em7sSVwc25g8O6h2STm/wynI0jZG0JrWeR9OL2ly kfew== X-Gm-Message-State: ACrzQf2igREKi14UlZ9VktkAk9FET6/XRx0a26Il32LyNFBLtBLlVw+c r6+a7K/jMsVynLJIhiGEal9TNZJ22iYUGW6i X-Received: by 2002:a17:902:e1cd:b0:184:aa71:217d with SMTP id t13-20020a170902e1cd00b00184aa71217dmr20515106pla.77.1666382851068; Fri, 21 Oct 2022 13:07:31 -0700 (PDT) Received: from tictac2.mtv.corp.google.com ([2620:15c:9d:2:361c:b732:581f:2329]) by smtp.gmail.com with ESMTPSA id c1-20020a170902d48100b0017a032d7ae4sm10209137plg.104.2022.10.21.13.07.29 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 21 Oct 2022 13:07:30 -0700 (PDT) From: Douglas Anderson <dianders@chromium.org> To: dri-devel@lists.freedesktop.org Cc: Kuogee Hsieh <quic_khsieh@quicinc.com>, Abhinav Kumar <quic_abhinavk@quicinc.com>, Stephen Boyd <swboyd@chromium.org>, Douglas Anderson <dianders@chromium.org>, Daniel Vetter <daniel@ffwll.ch>, David Airlie <airlied@gmail.com>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, linux-kernel@vger.kernel.org Subject: [PATCH] drm/edid: Dump the EDID when drm_edid_get_panel_id() has an error Date: Fri, 21 Oct 2022 13:07:07 -0700 Message-Id: <20221021130637.1.I8c2de0954a4e54e0c59a72938268e2ead91daa98@changeid> X-Mailer: git-send-email 2.38.0.135.g90850a2211-goog MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.3 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1747329487513919253?= X-GMAIL-MSGID: =?utf-8?q?1747329487513919253?= |
Series |
drm/edid: Dump the EDID when drm_edid_get_panel_id() has an error
|
|
Commit Message
Doug Anderson
Oct. 21, 2022, 8:07 p.m. UTC
If we fail to get a valid panel ID in drm_edid_get_panel_id() we'd
like to see the EDID that was read so we have a chance of
understanding what's wrong. There's already a function for that, so
let's call it in the error case.
NOTE: edid_block_read() has a retry loop in it, so actually we'll only
print the block read back from the final attempt. This still seems
better than nothing.
Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
drivers/gpu/drm/drm_edid.c | 2 ++
1 file changed, 2 insertions(+)
Comments
Hi Doug On 10/21/2022 1:07 PM, Douglas Anderson wrote: > If we fail to get a valid panel ID in drm_edid_get_panel_id() we'd > like to see the EDID that was read so we have a chance of > understanding what's wrong. There's already a function for that, so > let's call it in the error case. > > NOTE: edid_block_read() has a retry loop in it, so actually we'll only > print the block read back from the final attempt. This still seems > better than nothing. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> Instead of checkinf for edid_block_status_valid() on the base_block, do you want to use drm_edid_block_valid() instead? That way you get the edid_block_dump() for free if it was invalid. > --- > > drivers/gpu/drm/drm_edid.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c > index 47465b9765f1..d63e26ec88b1 100644 > --- a/drivers/gpu/drm/drm_edid.c > +++ b/drivers/gpu/drm/drm_edid.c > @@ -2721,6 +2721,8 @@ u32 drm_edid_get_panel_id(struct i2c_adapter *adapter) > > if (edid_block_status_valid(status, edid_block_tag(base_block))) > panel_id = edid_extract_panel_id(base_block); > + else > + edid_block_dump(KERN_NOTICE, base_block, 0); > > kfree(base_block); >
Hi, On Fri, Oct 21, 2022 at 2:18 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > Hi Doug > > On 10/21/2022 1:07 PM, Douglas Anderson wrote: > > If we fail to get a valid panel ID in drm_edid_get_panel_id() we'd > > like to see the EDID that was read so we have a chance of > > understanding what's wrong. There's already a function for that, so > > let's call it in the error case. > > > > NOTE: edid_block_read() has a retry loop in it, so actually we'll only > > print the block read back from the final attempt. This still seems > > better than nothing. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > Instead of checkinf for edid_block_status_valid() on the base_block, do > you want to use drm_edid_block_valid() instead? > > That way you get the edid_block_dump() for free if it was invalid. I can... ...but it feels a bit awkward and maybe not quite how the functions were intended to work together? One thing I notice is that if I call drm_edid_block_valid() I'm doing a bunch of duplicate work that already happened in edid_block_read(), which already calls edid_block_check() and handles fixing headers. I guess also if I call drm_edid_block_valid() then I should ignore the "status" return value of edid_block_read() because we don't need to pass it anywhere (because the work is re-done in drm_edid_block_valid()). So I guess I'm happy to do a v2 like that if everyone likes it better, but to me it feels a little weird. -Doug
Hi Doug On 10/24/2022 1:28 PM, Doug Anderson wrote: > Hi, > > On Fri, Oct 21, 2022 at 2:18 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> Hi Doug >> >> On 10/21/2022 1:07 PM, Douglas Anderson wrote: >>> If we fail to get a valid panel ID in drm_edid_get_panel_id() we'd >>> like to see the EDID that was read so we have a chance of >>> understanding what's wrong. There's already a function for that, so >>> let's call it in the error case. >>> >>> NOTE: edid_block_read() has a retry loop in it, so actually we'll only >>> print the block read back from the final attempt. This still seems >>> better than nothing. >>> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> >> Instead of checkinf for edid_block_status_valid() on the base_block, do >> you want to use drm_edid_block_valid() instead? >> >> That way you get the edid_block_dump() for free if it was invalid. > > I can... ...but it feels a bit awkward and maybe not quite how the > functions were intended to work together? > > One thing I notice is that if I call drm_edid_block_valid() I'm doing > a bunch of duplicate work that already happened in edid_block_read(), > which already calls edid_block_check() and handles fixing headers. I > guess also if I call drm_edid_block_valid() then I should ignore the > "status" return value of edid_block_read() because we don't need to > pass it anywhere (because the work is re-done in > drm_edid_block_valid()). > > So I guess I'm happy to do a v2 like that if everyone likes it better, > but to me it feels a little weird. > > -Doug Alright, agreed. There is some duplication of code happening if we use drm_edid_block_valid(). I had suggested that because it has inherent support for dumping the bad EDID. In that case, this change LGTM, because in principle you are doing the same thing as _drm_do_get_edid() (with the only difference being here we read only the base block as opposed to the full EDID there). Hence, Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Hi, On Tue, Oct 25, 2022 at 1:39 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > > Hi Doug > > On 10/24/2022 1:28 PM, Doug Anderson wrote: > > Hi, > > > > On Fri, Oct 21, 2022 at 2:18 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> Hi Doug > >> > >> On 10/21/2022 1:07 PM, Douglas Anderson wrote: > >>> If we fail to get a valid panel ID in drm_edid_get_panel_id() we'd > >>> like to see the EDID that was read so we have a chance of > >>> understanding what's wrong. There's already a function for that, so > >>> let's call it in the error case. > >>> > >>> NOTE: edid_block_read() has a retry loop in it, so actually we'll only > >>> print the block read back from the final attempt. This still seems > >>> better than nothing. > >>> > >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> > >> > >> Instead of checkinf for edid_block_status_valid() on the base_block, do > >> you want to use drm_edid_block_valid() instead? > >> > >> That way you get the edid_block_dump() for free if it was invalid. > > > > I can... ...but it feels a bit awkward and maybe not quite how the > > functions were intended to work together? > > > > One thing I notice is that if I call drm_edid_block_valid() I'm doing > > a bunch of duplicate work that already happened in edid_block_read(), > > which already calls edid_block_check() and handles fixing headers. I > > guess also if I call drm_edid_block_valid() then I should ignore the > > "status" return value of edid_block_read() because we don't need to > > pass it anywhere (because the work is re-done in > > drm_edid_block_valid()). > > > > So I guess I'm happy to do a v2 like that if everyone likes it better, > > but to me it feels a little weird. > > > > -Doug > > Alright, agreed. There is some duplication of code happening if we use > drm_edid_block_valid(). I had suggested that because it has inherent > support for dumping the bad EDID. > > In that case, this change LGTM, because in principle you are doing the > same thing as _drm_do_get_edid() (with the only difference being here we > read only the base block as opposed to the full EDID there). > > Hence, > > Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> I've given this patch a bunch of time because it wasn't urgent, but seems like it could be about time to land. I'll plan to land it next Monday or Tuesday unless anyone has any other comments. Thanks! -Doug
On Fri, 11 Nov 2022, Doug Anderson <dianders@chromium.org> wrote: > Hi, > > On Tue, Oct 25, 2022 at 1:39 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> Hi Doug >> >> On 10/24/2022 1:28 PM, Doug Anderson wrote: >> > Hi, >> > >> > On Fri, Oct 21, 2022 at 2:18 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: >> >> >> >> Hi Doug >> >> >> >> On 10/21/2022 1:07 PM, Douglas Anderson wrote: >> >>> If we fail to get a valid panel ID in drm_edid_get_panel_id() we'd >> >>> like to see the EDID that was read so we have a chance of >> >>> understanding what's wrong. There's already a function for that, so >> >>> let's call it in the error case. >> >>> >> >>> NOTE: edid_block_read() has a retry loop in it, so actually we'll only >> >>> print the block read back from the final attempt. This still seems >> >>> better than nothing. >> >>> >> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> >> >> >> >> Instead of checkinf for edid_block_status_valid() on the base_block, do >> >> you want to use drm_edid_block_valid() instead? >> >> >> >> That way you get the edid_block_dump() for free if it was invalid. >> > >> > I can... ...but it feels a bit awkward and maybe not quite how the >> > functions were intended to work together? >> > >> > One thing I notice is that if I call drm_edid_block_valid() I'm doing >> > a bunch of duplicate work that already happened in edid_block_read(), >> > which already calls edid_block_check() and handles fixing headers. I >> > guess also if I call drm_edid_block_valid() then I should ignore the >> > "status" return value of edid_block_read() because we don't need to >> > pass it anywhere (because the work is re-done in >> > drm_edid_block_valid()). >> > >> > So I guess I'm happy to do a v2 like that if everyone likes it better, >> > but to me it feels a little weird. >> > >> > -Doug >> >> Alright, agreed. There is some duplication of code happening if we use >> drm_edid_block_valid(). I had suggested that because it has inherent >> support for dumping the bad EDID. >> >> In that case, this change LGTM, because in principle you are doing the >> same thing as _drm_do_get_edid() (with the only difference being here we >> read only the base block as opposed to the full EDID there). >> >> Hence, >> >> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > I've given this patch a bunch of time because it wasn't urgent, but > seems like it could be about time to land. I'll plan to land it next > Monday or Tuesday unless anyone has any other comments. Ack, it's benign enough. BR, Jani. > > Thanks! > > -Doug
Hi, On Mon, Nov 14, 2022 at 2:02 AM Jani Nikula <jani.nikula@linux.intel.com> wrote: > > On Fri, 11 Nov 2022, Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > > > On Tue, Oct 25, 2022 at 1:39 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> > >> Hi Doug > >> > >> On 10/24/2022 1:28 PM, Doug Anderson wrote: > >> > Hi, > >> > > >> > On Fri, Oct 21, 2022 at 2:18 PM Abhinav Kumar <quic_abhinavk@quicinc.com> wrote: > >> >> > >> >> Hi Doug > >> >> > >> >> On 10/21/2022 1:07 PM, Douglas Anderson wrote: > >> >>> If we fail to get a valid panel ID in drm_edid_get_panel_id() we'd > >> >>> like to see the EDID that was read so we have a chance of > >> >>> understanding what's wrong. There's already a function for that, so > >> >>> let's call it in the error case. > >> >>> > >> >>> NOTE: edid_block_read() has a retry loop in it, so actually we'll only > >> >>> print the block read back from the final attempt. This still seems > >> >>> better than nothing. > >> >>> > >> >>> Signed-off-by: Douglas Anderson <dianders@chromium.org> > >> >> > >> >> Instead of checkinf for edid_block_status_valid() on the base_block, do > >> >> you want to use drm_edid_block_valid() instead? > >> >> > >> >> That way you get the edid_block_dump() for free if it was invalid. > >> > > >> > I can... ...but it feels a bit awkward and maybe not quite how the > >> > functions were intended to work together? > >> > > >> > One thing I notice is that if I call drm_edid_block_valid() I'm doing > >> > a bunch of duplicate work that already happened in edid_block_read(), > >> > which already calls edid_block_check() and handles fixing headers. I > >> > guess also if I call drm_edid_block_valid() then I should ignore the > >> > "status" return value of edid_block_read() because we don't need to > >> > pass it anywhere (because the work is re-done in > >> > drm_edid_block_valid()). > >> > > >> > So I guess I'm happy to do a v2 like that if everyone likes it better, > >> > but to me it feels a little weird. > >> > > >> > -Doug > >> > >> Alright, agreed. There is some duplication of code happening if we use > >> drm_edid_block_valid(). I had suggested that because it has inherent > >> support for dumping the bad EDID. > >> > >> In that case, this change LGTM, because in principle you are doing the > >> same thing as _drm_do_get_edid() (with the only difference being here we > >> read only the base block as opposed to the full EDID there). > >> > >> Hence, > >> > >> Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com> > > > > I've given this patch a bunch of time because it wasn't urgent, but > > seems like it could be about time to land. I'll plan to land it next > > Monday or Tuesday unless anyone has any other comments. > > Ack, it's benign enough. Thanks! Since you didn't write the above as an Acked-by tag I didn't add it to the commit, but I went ahead and landed with Abhinav's tag. 69c7717c20cc drm/edid: Dump the EDID when drm_edid_get_panel_id() has an error -Doug
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c index 47465b9765f1..d63e26ec88b1 100644 --- a/drivers/gpu/drm/drm_edid.c +++ b/drivers/gpu/drm/drm_edid.c @@ -2721,6 +2721,8 @@ u32 drm_edid_get_panel_id(struct i2c_adapter *adapter) if (edid_block_status_valid(status, edid_block_tag(base_block))) panel_id = edid_extract_panel_id(base_block); + else + edid_block_dump(KERN_NOTICE, base_block, 0); kfree(base_block);