Received-SPF: pass (google.com: domain of linux-kernel+bounces-85941-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240228011133.1238439-1-hsinyi@chromium.org> <20240228011133.1238439-2-hsinyi@chromium.org>
 <CAD=FV=XCGCpLmCQhfTXfrOON99KLbR5pR59jaiD_V14szn6qsA@mail.gmail.com>
In-Reply-To: <CAD=FV=XCGCpLmCQhfTXfrOON99KLbR5pR59jaiD_V14szn6qsA@mail.gmail.com>
From: Hsin-Yi Wang <hsinyi@chromium.org>
Date: Wed, 28 Feb 2024 16:54:37 -0800
Message-ID: <CAJMQK-j5oDS1H=x2=tAcna1XK9JN8SdyHV-rHJvMgJX7S-3MHg@mail.gmail.com>
Subject: Re: [PATCH v2 1/3] drm_edid: Support getting EDID through ddc without connector
To: Doug Anderson <dianders@chromium.org>
Cc: Neil Armstrong <neil.armstrong@linaro.org>, Jessica Zhang <quic_jesszhan@quicinc.com>, 
	Sam Ravnborg <sam@ravnborg.org>, Maarten Lankhorst <maarten.lankhorst@linux.intel.com>, 
	Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>, 
	David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>, dri-devel@lists.freedesktop.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Wed, Feb 28, 2024 at 4:21=E2=80=AFPM Doug Anderson <dianders@chromium.or=
g> wrote:
>
> Hi,
>
> On Tue, Feb 27, 2024 at 5:11=E2=80=AFPM Hsin-Yi Wang <hsinyi@chromium.org=
> wrote:
> >
> > Some panels are interested in the EDID during early probe when connecto=
r
> > is still unknown.
> >
> > Add a function drm_get_edid_no_connector() to get edid without connecto=
r.
> > No functional change for existing usage.
> >
> > Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> > ---
> > v1->v2:
> > add a function to return the entire edid without updating connector.
> > ---
> >  drivers/gpu/drm/drm_edid.c | 45 ++++++++++++++++++++++++++++----------
> >  include/drm/drm_edid.h     |  1 +
> >  2 files changed, 34 insertions(+), 12 deletions(-)
>
> I'll respond in the discussion in v1 too, but overall I'm not a fan of
> reading the whole EDID twice at bootup. Personally I'd love to see us
> to back to just reading the base block like in v1, but I guess we can
> see what Jani and others say.
>
>
> > @@ -2385,18 +2385,20 @@ static struct edid *_drm_do_get_edid(struct drm=
_connector *connector,
> >         if (status =3D=3D EDID_BLOCK_READ_FAIL)
> >                 goto fail;
> >
> > -       /* FIXME: Clarify what a corrupt EDID actually means. */
> > -       if (status =3D=3D EDID_BLOCK_OK || status =3D=3D EDID_BLOCK_VER=
SION)
> > -               connector->edid_corrupt =3D false;
> > -       else
> > -               connector->edid_corrupt =3D true;
> > +       if (connector) {
> > +               /* FIXME: Clarify what a corrupt EDID actually means. *=
/
> > +               if (status =3D=3D EDID_BLOCK_OK || status =3D=3D EDID_B=
LOCK_VERSION)
> > +                       connector->edid_corrupt =3D false;
> > +               else
> > +                       connector->edid_corrupt =3D true;
> >
> > -       if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> > -               if (status =3D=3D EDID_BLOCK_ZERO)
> > -                       connector->null_edid_counter++;
> > +               if (!edid_block_status_valid(status, edid_block_tag(edi=
d))) {
> > +                       if (status =3D=3D EDID_BLOCK_ZERO)
> > +                               connector->null_edid_counter++;
> >
> > -               connector_bad_edid(connector, edid, 1);
> > -               goto fail;
> > +                       connector_bad_edid(connector, edid, 1);
> > +                       goto fail;
>
> This "goto fail" is now only run "if (connector)" which means that
> you're not properly checking if the EDID is valid when "connector =3D=3D
> NULL", right? That seems like a bug unless I missed something...

We can't check with connector_bad_edid() since there's no connector.
But we still check with edid_block_read() status, similar to what the
original drm_edid_get_panel_id() checks.