Received-SPF: pass (google.com: domain of linux-kernel+bounces-82414-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240223223958.3887423-1-hsinyi@chromium.org> <20240223223958.3887423-3-hsinyi@chromium.org>
 <CAD=FV=Xs4V7ei4NW0T0x0Bq6_dQF6sZKvFSy2WGQFQsHae=61Q@mail.gmail.com>
In-Reply-To: <CAD=FV=Xs4V7ei4NW0T0x0Bq6_dQF6sZKvFSy2WGQFQsHae=61Q@mail.gmail.com>
From: Hsin-Yi Wang <hsinyi@chromium.org>
Date: Mon, 26 Feb 2024 14:38:30 -0800
Message-ID: <CAJMQK-jKuA_2Y+rQ5q4MkbmkQFJh4+DC98GhExuVt39DTfBwsg@mail.gmail.com>
Subject: Re: [PATCH 2/2] drm/panel: panel-edp: Match with panel hash for
 overridden modes
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 Mon, Feb 26, 2024 at 2:29=E2=80=AFPM Doug Anderson <dianders@chromium.or=
g> wrote:
>
> Hi,
>
> On Fri, Feb 23, 2024 at 2:40=E2=80=AFPM Hsin-Yi Wang <hsinyi@chromium.org=
> wrote:
> >
> > It's found that some panels have variants that they share the same pane=
l id
> > although their EDID and names are different. One of the variants requir=
es
> > using overridden modes to resolve glitching issue as described in commi=
t
> > 70e0d5550f5c ("drm/panel-edp: Add auo_b116xa3_mode"). Other variants sh=
ould
> > use the modes parsed from EDID.
> >
> > For example, AUO 0x405c B116XAK01.0, it has at least 2 different varian=
ts
> > that EDID and panel name are different, but using the same panel id. On=
e of
> > the variants require using overridden mode. Same case for AUO 0x615c
> > B116XAN06.1.
> >
> > Add such entries and use the hash of the EDID to match the panel needs =
the
> > overridden modes.
>
> As pointed out in an offline discussion, it's possible that we might
> want to "ignore" some of these bytes for the purpose of the CRC.
> Specifically, we might want to ignore:
> * byte 16 - Week of manufacture
> * byte 17 - Year of manufacture
> * byte 127 - Checksum
>
> That way if a manufacturer actually is updating those numbers in
> production we can still have one hash that captures all the panels. I
> have no idea if manufacturers actually are, but IMO the hash of the
> rest of the base block should be sufficient to differentiate between
> different panels anyway. It would be easy to just zero out those 3
> bytes before computing the CRC.
>
> What do you think?

Agreed that we can zero out these fields.

>
>
> > @@ -758,13 +762,13 @@ static void panel_edp_parse_panel_timing_node(str=
uct device *dev,
> >                 dev_err(dev, "Reject override mode: No display_timing f=
ound\n");
> >  }
> >
> > -static const struct edp_panel_entry *find_edp_panel(u32 panel_id);
> > +static const struct edp_panel_entry *find_edp_panel(u32 panel_id, u32 =
panel_hash);
> >
> >  static int generic_edp_panel_probe(struct device *dev, struct panel_ed=
p *panel)
> >  {
> >         struct panel_desc *desc;
> >         void *base_block;
> > -       u32 panel_id;
> > +       u32 panel_id, panel_hash;
> >         char vend[4];
> >         u16 product_id;
> >         u32 reliable_ms =3D 0;
> > @@ -796,15 +800,17 @@ static int generic_edp_panel_probe(struct device =
*dev, struct panel_edp *panel)
> >         base_block =3D drm_edid_get_base_block(panel->ddc);
> >         if (base_block) {
> >                 panel_id =3D drm_edid_get_panel_id(base_block);
> > +               panel_hash =3D crc32_le(~0, base_block, EDID_LENGTH) ^ =
0xffffffff;
>
> Any reason you need to XOR with 0xffffffff?
>
To be consistent with the crc32[1] command. It's more convenient to be
able to verify it with userspace tools.

[1] https://www.commandlinux.com/man-page/man1/crc32.1.html

>
> > @@ -2077,13 +2098,32 @@ static const struct edp_panel_entry edp_panels[=
] =3D {
> >         { /* sentinal */ }
> >  };
> >
> > -static const struct edp_panel_entry *find_edp_panel(u32 panel_id)
> > +/*
> > + * Similar to edp_panels, this table lists panel variants that require=
 using
> > + * overridden modes but have the same panel id as one of the entries i=
n edp_panels.
> > + *
> > + * Sort first by vendor, then by product ID.
>
> Add ", then by hash" just in case we need it.
>
>
> > +static const struct edp_panel_entry *find_edp_panel(u32 panel_id, u32 =
panel_hash)
> >  {
> >         const struct edp_panel_entry *panel;
> >
> > -       if (!panel_id)
> > +       if (!panel_id || !panel_hash)
> >                 return NULL;
>
> IMO just remove the check above. Not sure why it was there in the
> first place. Maybe I had it from some older version of the code?
> Callers shouldn't be calling us with a panel ID / hash of 0 anyway,
> and if they do they'll go through the loop and return NULL anyway.
>

Sure.

>
>
> -Doug