On Wed, 08 Sep 2021, Doug Anderson [off-list ref] wrote:

Hi,

On Mon, Sep 6, 2021 at 3:05 AM Jani Nikula [off-list ref] wrote:

quoted

quoted

+{
+     struct edid *edid;
+     u32 val;
+
+     edid = drm_do_get_edid_blk0(drm_do_probe_ddc_edid, adapter, NULL, NULL);
+
+     /*
+      * There are no manufacturer IDs of 0, so if there is a problem reading
+      * the EDID then we'll just return 0.
+      */
+     if (IS_ERR_OR_NULL(edid))
+             return 0;
+
+     /*
+      * In theory we could try to de-obfuscate this like edid_get_quirks()
+      * does, but it's easier to just deal with a 32-bit number.

Hmm, but is it, really? AFAICT this is just an internal representation
for a table, where it could just as well be stored in a struct that
could be just as compact now, but extensible later. You populate the
table via an encoding macro, then decode the id using a function - while
it could be in a format that's directly usable without the decode. If
suitably chosen, the struct could perhaps be reused between the quirks
code and your code.

I'm not 100% sure, but I think you're suggesting having this function
return a `struct edid_panel_id` or something like that. Is that right?
Maybe that would look something like this?

struct edid_panel_id {
  char vendor[4];
  u16 product_id;
}

...or perhaps this (untested, but I think it works):

struct edid_panel_id {
  u16 vend_c1:5;
  u16 vend_c2:5;
  u16 vend_c3:5;
  u16 product_id;
}

...and then change `struct edid_quirk` to something like this:

static const struct edid_quirk {
  struct edid_panel_id panel_id;
  u32 quirks;
} ...

Is that correct? There are a few downsides that I can see:

a) I think the biggest downside is the inability compare with "==". I
don't believe it's legal to compare structs with "==" in C. Yeah, we
can use memcmp() but that feels more awkward to me.

b) Unless you use the bitfield approach, it takes up more space. I
know it's not a huge deal, but the format in the EDID is pretty much
_forced_ to fit in 32-bits. The bitfield approach seems like it'd be
more awkward than my encoding macros.

Sorry for the delayed response. Fair enough, let's go with the u32 for
now. It's not like we can't change this later.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Graphics Center