This series is a follow up for 1a5e81de180e ("Revert "drm/panel-edp: Add
auo_b116xa3_mode""). It's found that 2 different AUO panels use the same
product id. One of them requires an overridden mode, while the other should
use the mode directly from edid.
Match the panel for identity (id and name). If not found, fallback to match
id.
v1: https://lore.kernel.org/lkml/[email protected]
v2: https://lore.kernel.org/lkml/[email protected]
v3: https://lore.kernel.org/lkml/[email protected]
v4: https://lore.kernel.org/lkml/[email protected]/
Hsin-Yi Wang (6):
drm_edid: Add a function to get EDID base block
drm/edid: Clean up drm_edid_get_panel_id()
drm/edid: Add a function to match EDID with identity
drm/edid: Match edid quirks with identity
drm/panel-edp: Match edp_panels with panel identity
drm/panel-edp: Fix AUO 0x405c panel naming and add a variant
drivers/gpu/drm/drm_edid.c | 144 +++++++++++++++++++++++-------
drivers/gpu/drm/panel/panel-edp.c | 68 +++++++++-----
include/drm/drm_edid.h | 11 ++-
3 files changed, 166 insertions(+), 57 deletions(-)
--
2.44.0.278.ge034bb2e1d-goog
It's found that some panels have variants that they share the same panel id
although their EDID and names are different. Besides panel id, now we need
more information from the EDID base block to distinguish these panel
variants.
Add drm_edid_read_base_block() to return the EDID base block, which is
wrapped in struct drm_edid.
Caller can further use it to get panel id or check if the block contains
certain strings, such as panel name.
Signed-off-by: Hsin-Yi Wang <[email protected]>
---
v4->v5: use _drm_edid_alloc
---
drivers/gpu/drm/drm_edid.c | 63 +++++++++++++++++++------------
drivers/gpu/drm/panel/panel-edp.c | 8 +++-
include/drm/drm_edid.h | 3 +-
3 files changed, 46 insertions(+), 28 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1ad94473e400..8c7e871eff58 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2764,58 +2764,71 @@ static u32 edid_extract_panel_id(const struct edid *edid)
}
/**
- * drm_edid_get_panel_id - Get a panel's ID through DDC
- * @adapter: I2C adapter to use for DDC
+ * drm_edid_get_panel_id - Get a panel's ID from EDID
+ * @drm_edid: EDID that contains panel ID.
*
- * This function reads the first block of the EDID of a panel and (assuming
+ * This function uses the first block of the EDID of a panel and (assuming
* that the EDID is valid) extracts the ID out of it. The ID is a 32-bit value
* (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that's
* supposed to be different for each different modem of panel.
*
+ * Return: A 32-bit ID that should be different for each make/model of panel.
+ * See the functions drm_edid_encode_panel_id() and
+ * drm_edid_decode_panel_id() for some details on the structure of this
+ * ID.
+ */
+u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid)
+{
+ return edid_extract_panel_id(drm_edid->edid);
+}
+EXPORT_SYMBOL(drm_edid_get_panel_id);
+
+/**
+ * drm_edid_read_base_block - Get a panel's EDID base block
+ * @adapter: I2C adapter to use for DDC
+ *
+ * This function returns the drm_edid containing the first block of the EDID of
+ * a panel.
+ *
* This function is intended to be used during early probing on devices where
* more than one panel might be present. Because of its intended use it must
- * assume that the EDID of the panel is correct, at least as far as the ID
- * is concerned (in other words, we don't process any overrides here).
+ * assume that the EDID of the panel is correct, at least as far as the base
+ * block is concerned (in other words, we don't process any overrides here).
+ *
+ * Caller should call drm_edid_free() after use.
*
* NOTE: it's expected that this function and drm_do_get_edid() will both
* be read the EDID, but there is no caching between them. Since we're only
* reading the first block, hopefully this extra overhead won't be too big.
*
- * Return: A 32-bit ID that should be different for each make/model of panel.
- * See the functions drm_edid_encode_panel_id() and
- * drm_edid_decode_panel_id() for some details on the structure of this
- * ID.
+ * WARNING: Only use this function when the connector is unknown. For example,
+ * during the early probe of panel. The EDID read from the function is temporary
+ * and should be replaced by the full EDID returned from other drm_edid_read.
+ *
+ * Return: Pointer to allocated EDID base block, or NULL on any failure.
*/
-
-u32 drm_edid_get_panel_id(struct i2c_adapter *adapter)
+const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter)
{
enum edid_block_status status;
void *base_block;
- u32 panel_id = 0;
-
- /*
- * There are no manufacturer IDs of 0, so if there is a problem reading
- * the EDID then we'll just return 0.
- */
base_block = kzalloc(EDID_LENGTH, GFP_KERNEL);
if (!base_block)
- return 0;
+ return NULL;
status = edid_block_read(base_block, 0, drm_do_probe_ddc_edid, adapter);
edid_block_status_print(status, base_block, 0);
- if (edid_block_status_valid(status, edid_block_tag(base_block)))
- panel_id = edid_extract_panel_id(base_block);
- else
+ if (!edid_block_status_valid(status, edid_block_tag(base_block))) {
edid_block_dump(KERN_NOTICE, base_block, 0);
+ kfree(base_block);
+ return NULL;
+ }
- kfree(base_block);
-
- return panel_id;
+ return _drm_edid_alloc(base_block, EDID_LENGTH);
}
-EXPORT_SYMBOL(drm_edid_get_panel_id);
+EXPORT_SYMBOL(drm_edid_read_base_block);
/**
* drm_get_edid_switcheroo - get EDID data for a vga_switcheroo output
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index 3fb5fcd326a4..fe51680feb61 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -766,6 +766,7 @@ static const struct edp_panel_entry *find_edp_panel(u32 panel_id);
static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
{
struct panel_desc *desc;
+ const struct drm_edid *base_block;
u32 panel_id;
char vend[4];
u16 product_id;
@@ -795,8 +796,11 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
goto exit;
}
- panel_id = drm_edid_get_panel_id(panel->ddc);
- if (!panel_id) {
+ base_block = drm_edid_read_base_block(panel->ddc);
+ if (base_block) {
+ panel_id = drm_edid_get_panel_id(base_block);
+ drm_edid_free(base_block);
+ } else {
dev_err(dev, "Couldn't identify panel via EDID\n");
ret = -EIO;
goto exit;
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 70ae6c290bdc..8b233865b085 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -565,7 +565,8 @@ struct edid *drm_do_get_edid(struct drm_connector *connector,
void *data);
struct edid *drm_get_edid(struct drm_connector *connector,
struct i2c_adapter *adapter);
-u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
+const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
+u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
struct i2c_adapter *adapter);
struct edid *drm_edid_duplicate(const struct edid *edid);
--
2.44.0.278.ge034bb2e1d-goog
Create a type drm_edid_ident as the identity of an EDID. Currently it
contains panel id and monitor name.
Create a function that can match a given EDID and an identity:
1. Reject if the panel id doesn't match.
2. If name is not null in identity, try to match it in the detailed timing
blocks. Note that some panel vendors put the monitor name after
EDID_DETAIL_MONITOR_STRING.
Signed-off-by: Hsin-Yi Wang <[email protected]>
---
v4->v5: use strncmp, change function/variable names.
---
drivers/gpu/drm/drm_edid.c | 65 ++++++++++++++++++++++++++++++++++++++
include/drm/drm_edid.h | 8 +++++
2 files changed, 73 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index febe374fa969..29ef35ebee32 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -100,6 +100,11 @@ struct detailed_mode_closure {
int modes;
};
+struct drm_edid_match_closure {
+ const struct drm_edid_ident *ident;
+ bool matched;
+};
+
#define LEVEL_DMT 0
#define LEVEL_GTF 1
#define LEVEL_GTF2 2
@@ -5405,6 +5410,66 @@ drm_parse_hdmi_vsdb_audio(struct drm_connector *connector, const u8 *db)
connector->audio_latency[0], connector->audio_latency[1]);
}
+static void
+match_identity(const struct detailed_timing *timing, void *data)
+{
+ struct drm_edid_match_closure *closure = data;
+ unsigned int i;
+ const char *name = closure->ident->name;
+ unsigned int name_len = strlen(name);
+ const char *desc = timing->data.other_data.data.str.str;
+ unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str);
+
+ if (name_len > desc_len ||
+ !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) ||
+ is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING)))
+ return;
+
+ if (strncmp(name, desc, name_len))
+ return;
+
+ /* Allow trailing white spaces and \0. */
+ for (i = name_len; i < desc_len; i++) {
+ if (desc[i] == '\n')
+ break;
+ if (!isspace(desc[i]) && !desc[i])
+ return;
+ }
+
+ closure->matched = true;
+}
+
+/**
+ * drm_edid_match - match drm_edid with given identity
+ * @drm_edid: EDID
+ * @ident: the EDID identity to match with
+ *
+ * Check if the EDID matches with the given identity.
+ *
+ * Return: True if the given identity matched with EDID, false otherwise.
+ */
+bool drm_edid_match(const struct drm_edid *drm_edid,
+ const struct drm_edid_ident *ident)
+{
+ if (!drm_edid || drm_edid_get_panel_id(drm_edid) != ident->panel_id)
+ return false;
+
+ /* Match with name only if it's not NULL. */
+ if (ident->name) {
+ struct drm_edid_match_closure closure = {
+ .ident = ident,
+ .matched = false,
+ };
+
+ drm_for_each_detailed_block(drm_edid, match_identity, &closure);
+
+ return closure.matched;
+ }
+
+ return true;
+}
+EXPORT_SYMBOL(drm_edid_match);
+
static void
monitor_name(const struct detailed_timing *timing, void *data)
{
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 8b233865b085..28f05a7b2f7b 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -367,6 +367,12 @@ struct edid {
u8 checksum;
} __attribute__((packed));
+/* EDID matching */
+struct drm_edid_ident {
+ u32 panel_id;
+ const char *name;
+};
+
#define EDID_PRODUCT_ID(e) ((e)->prod_code[0] | ((e)->prod_code[1] << 8))
/* Short Audio Descriptor */
@@ -567,6 +573,8 @@ struct edid *drm_get_edid(struct drm_connector *connector,
struct i2c_adapter *adapter);
const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
+bool drm_edid_match(const struct drm_edid *drm_edid,
+ const struct drm_edid_ident *ident);
struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
struct i2c_adapter *adapter);
struct edid *drm_edid_duplicate(const struct edid *edid);
--
2.44.0.278.ge034bb2e1d-goog
Currently edid quirks are matched by panel id only.
Modify it to match with identity so it's easier to be extended
for more complex matching if required.
Suggested-by: Jani Nikula <[email protected]>
Signed-off-by: Hsin-Yi Wang <[email protected]>
Reviewed-by: Jani Nikula <[email protected]>
---
v5: no change
---
drivers/gpu/drm/drm_edid.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 29ef35ebee32..f9e4dacd7255 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -112,13 +112,15 @@ struct drm_edid_match_closure {
#define EDID_QUIRK(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _quirks) \
{ \
- .panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, \
- product_id), \
+ .ident = { \
+ .panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, \
+ vend_chr_2, product_id), \
+ }, \
.quirks = _quirks \
}
static const struct edid_quirk {
- u32 panel_id;
+ const struct drm_edid_ident ident;
u32 quirks;
} edid_quirk_list[] = {
/* Acer AL1706 */
@@ -2880,16 +2882,17 @@ EXPORT_SYMBOL(drm_edid_duplicate);
* @drm_edid: EDID to process
*
* This tells subsequent routines what fixes they need to apply.
+ *
+ * Return: A u32 represents the quirks to apply.
*/
static u32 edid_get_quirks(const struct drm_edid *drm_edid)
{
- u32 panel_id = drm_edid_get_panel_id(drm_edid);
const struct edid_quirk *quirk;
int i;
for (i = 0; i < ARRAY_SIZE(edid_quirk_list); i++) {
quirk = &edid_quirk_list[i];
- if (quirk->panel_id == panel_id)
+ if (drm_edid_match(drm_edid, &quirk->ident))
return quirk->quirks;
}
--
2.44.0.278.ge034bb2e1d-goog
It's found that some panels have variants that they share the same panel id
although their EDID and names are different. When matching generic edp
panels, we should first match with both panel identity, which contains both
panel id and panel name. If not found, match with panel id only.
Signed-off-by: Hsin-Yi Wang <[email protected]>
---
v5: no change
---
drivers/gpu/drm/panel/panel-edp.c | 45 ++++++++++++++++---------------
1 file changed, 24 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index fe51680feb61..772bf6011d79 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -210,15 +210,12 @@ struct panel_desc {
* struct edp_panel_entry - Maps panel ID to delay / panel name.
*/
struct edp_panel_entry {
- /** @panel_id: 32-bit ID for panel, encoded with drm_edid_encode_panel_id(). */
- u32 panel_id;
+ /** @ident: edid identity used for panel matching. */
+ const struct drm_edid_ident ident;
/** @delay: The power sequencing delays needed for this panel. */
const struct panel_delay *delay;
- /** @name: Name of this panel (for printing to logs). */
- const char *name;
-
/** @override_edid_mode: Override the mode obtained by edid. */
const struct drm_display_mode *override_edid_mode;
};
@@ -691,7 +688,7 @@ static int detected_panel_show(struct seq_file *s, void *data)
else if (!p->detected_panel)
seq_puts(s, "HARDCODED\n");
else
- seq_printf(s, "%s\n", p->detected_panel->name);
+ seq_printf(s, "%s\n", p->detected_panel->ident.name);
return 0;
}
@@ -761,7 +758,7 @@ static void panel_edp_parse_panel_timing_node(struct device *dev,
dev_err(dev, "Reject override mode: No display_timing found\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, const struct drm_edid *edid);
static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
{
@@ -799,7 +796,6 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
base_block = drm_edid_read_base_block(panel->ddc);
if (base_block) {
panel_id = drm_edid_get_panel_id(base_block);
- drm_edid_free(base_block);
} else {
dev_err(dev, "Couldn't identify panel via EDID\n");
ret = -EIO;
@@ -807,7 +803,9 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
}
drm_edid_decode_panel_id(panel_id, vend, &product_id);
- panel->detected_panel = find_edp_panel(panel_id);
+ panel->detected_panel = find_edp_panel(panel_id, base_block);
+
+ drm_edid_free(base_block);
/*
* We're using non-optimized timings and want it really obvious that
@@ -840,7 +838,7 @@ static int generic_edp_panel_probe(struct device *dev, struct panel_edp *panel)
panel->detected_panel = ERR_PTR(-EINVAL);
} else {
dev_info(dev, "Detected %s %s (%#06x)\n",
- vend, panel->detected_panel->name, product_id);
+ vend, panel->detected_panel->ident.name, product_id);
/* Update the delay; everything else comes from EDID */
desc->delay = *panel->detected_panel->delay;
@@ -1954,17 +1952,21 @@ static const struct panel_delay delay_200_500_e50_po2e200 = {
#define EDP_PANEL_ENTRY(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _delay, _name) \
{ \
- .name = _name, \
- .panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, \
- product_id), \
+ .ident = { \
+ .name = _name, \
+ .panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, \
+ product_id), \
+ }, \
.delay = _delay \
}
#define EDP_PANEL_ENTRY2(vend_chr_0, vend_chr_1, vend_chr_2, product_id, _delay, _name, _mode) \
{ \
- .name = _name, \
- .panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, \
- product_id), \
+ .ident = { \
+ .name = _name, \
+ .panel_id = drm_edid_encode_panel_id(vend_chr_0, vend_chr_1, vend_chr_2, \
+ product_id), \
+ }, \
.delay = _delay, \
.override_edid_mode = _mode \
}
@@ -2111,15 +2113,16 @@ static const struct edp_panel_entry edp_panels[] = {
{ /* sentinal */ }
};
-static const struct edp_panel_entry *find_edp_panel(u32 panel_id)
+static const struct edp_panel_entry *find_edp_panel(u32 panel_id, const struct drm_edid *edid)
{
const struct edp_panel_entry *panel;
- if (!panel_id)
- return NULL;
+ for (panel = edp_panels; panel->ident.panel_id; panel++)
+ if (drm_edid_match(edid, &panel->ident))
+ return panel;
- for (panel = edp_panels; panel->panel_id; panel++)
- if (panel->panel_id == panel_id)
+ for (panel = edp_panels; panel->ident.panel_id; panel++)
+ if (panel->ident.panel_id == panel_id)
return panel;
return NULL;
--
2.44.0.278.ge034bb2e1d-goog
drm_edid_get_panel_id() now just directly call edid_extract_panel_id().
Merge them into one function.
Signed-off-by: Hsin-Yi Wang <[email protected]>
---
v4->v5: new
---
drivers/gpu/drm/drm_edid.c | 39 ++++++++++++++++++--------------------
1 file changed, 18 insertions(+), 21 deletions(-)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 8c7e871eff58..febe374fa969 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2743,8 +2743,24 @@ const struct drm_edid *drm_edid_read(struct drm_connector *connector)
}
EXPORT_SYMBOL(drm_edid_read);
-static u32 edid_extract_panel_id(const struct edid *edid)
+/**
+ * drm_edid_get_panel_id - Get a panel's ID from EDID
+ * @drm_edid: EDID that contains panel ID.
+ *
+ * This function uses the first block of the EDID of a panel and (assuming
+ * that the EDID is valid) extracts the ID out of it. The ID is a 32-bit value
+ * (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that's
+ * supposed to be different for each different modem of panel.
+ *
+ * Return: A 32-bit ID that should be different for each make/model of panel.
+ * See the functions drm_edid_encode_panel_id() and
+ * drm_edid_decode_panel_id() for some details on the structure of this
+ * ID.
+ */
+u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid)
{
+ const struct edid *edid = drm_edid->edid;
+
/*
* We represent the ID as a 32-bit number so it can easily be compared
* with "==".
@@ -2762,25 +2778,6 @@ static u32 edid_extract_panel_id(const struct edid *edid)
(u32)edid->mfg_id[1] << 16 |
(u32)EDID_PRODUCT_ID(edid);
}
-
-/**
- * drm_edid_get_panel_id - Get a panel's ID from EDID
- * @drm_edid: EDID that contains panel ID.
- *
- * This function uses the first block of the EDID of a panel and (assuming
- * that the EDID is valid) extracts the ID out of it. The ID is a 32-bit value
- * (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that's
- * supposed to be different for each different modem of panel.
- *
- * Return: A 32-bit ID that should be different for each make/model of panel.
- * See the functions drm_edid_encode_panel_id() and
- * drm_edid_decode_panel_id() for some details on the structure of this
- * ID.
- */
-u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid)
-{
- return edid_extract_panel_id(drm_edid->edid);
-}
EXPORT_SYMBOL(drm_edid_get_panel_id);
/**
@@ -2881,7 +2878,7 @@ EXPORT_SYMBOL(drm_edid_duplicate);
*/
static u32 edid_get_quirks(const struct drm_edid *drm_edid)
{
- u32 panel_id = edid_extract_panel_id(drm_edid->edid);
+ u32 panel_id = drm_edid_get_panel_id(drm_edid);
const struct edid_quirk *quirk;
int i;
--
2.44.0.278.ge034bb2e1d-goog
There are 2 different AUO panels using the same panel id. One of the
variants requires using overridden modes to resolve glitching issue as
described in commit 70e0d5550f5c ("drm/panel-edp: Add auo_b116xa3_mode").
Other variants should use the modes parsed from EDID.
Signed-off-by: Hsin-Yi Wang <[email protected]>
---
v5: no change
---
drivers/gpu/drm/panel/panel-edp.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c
index 772bf6011d79..e3de55314bda 100644
--- a/drivers/gpu/drm/panel/panel-edp.c
+++ b/drivers/gpu/drm/panel/panel-edp.c
@@ -1009,6 +1009,19 @@ static const struct panel_desc auo_b101ean01 = {
},
};
+static const struct drm_display_mode auo_b116xa3_mode = {
+ .clock = 70589,
+ .hdisplay = 1366,
+ .hsync_start = 1366 + 40,
+ .hsync_end = 1366 + 40 + 40,
+ .htotal = 1366 + 40 + 40 + 32,
+ .vdisplay = 768,
+ .vsync_start = 768 + 10,
+ .vsync_end = 768 + 10 + 12,
+ .vtotal = 768 + 10 + 12 + 6,
+ .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
+};
+
static const struct drm_display_mode auo_b116xak01_mode = {
.clock = 69300,
.hdisplay = 1366,
@@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
- EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
+ EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"),
+ EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
+ &auo_b116xa3_mode),
EDP_PANEL_ENTRY('A', 'U', 'O', 0x435c, &delay_200_500_e50, "Unknown"),
EDP_PANEL_ENTRY('A', 'U', 'O', 0x582d, &delay_200_500_e50, "B133UAN01.0"),
EDP_PANEL_ENTRY('A', 'U', 'O', 0x615c, &delay_200_500_e50, "B116XAN06.1"),
--
2.44.0.278.ge034bb2e1d-goog
Hi,
On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>
> @@ -2764,58 +2764,71 @@ static u32 edid_extract_panel_id(const struct edid *edid)
> }
>
> /**
> - * drm_edid_get_panel_id - Get a panel's ID through DDC
> - * @adapter: I2C adapter to use for DDC
> + * drm_edid_get_panel_id - Get a panel's ID from EDID
> + * @drm_edid: EDID that contains panel ID.
> *
> - * This function reads the first block of the EDID of a panel and (assuming
> + * This function uses the first block of the EDID of a panel and (assuming
> * that the EDID is valid) extracts the ID out of it. The ID is a 32-bit value
> * (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that's
> * supposed to be different for each different modem of panel.
> *
> + * Return: A 32-bit ID that should be different for each make/model of panel.
> + * See the functions drm_edid_encode_panel_id() and
> + * drm_edid_decode_panel_id() for some details on the structure of this
> + * ID.
> + */
> +u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid)
> +{
I'd leave it up to Jani, but I'd wonder whether we need to confirm
drm_edid->size here is at least as big as the base block. In other
words: is there ever any chance that someone would have allocated a
struct drm_edid but not actually read a full base block into it?
In any case:
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>
> Currently edid quirks are matched by panel id only.
>
> Modify it to match with identity so it's easier to be extended
> for more complex matching if required.
>
> Suggested-by: Jani Nikula <[email protected]>
> Signed-off-by: Hsin-Yi Wang <[email protected]>
> Reviewed-by: Jani Nikula <[email protected]>
> ---
> v5: no change
> ---
> drivers/gpu/drm/drm_edid.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>
> @@ -1009,6 +1009,19 @@ static const struct panel_desc auo_b101ean01 = {
> },
> };
>
> +static const struct drm_display_mode auo_b116xa3_mode = {
> + .clock = 70589,
> + .hdisplay = 1366,
> + .hsync_start = 1366 + 40,
> + .hsync_end = 1366 + 40 + 40,
> + .htotal = 1366 + 40 + 40 + 32,
> + .vdisplay = 768,
> + .vsync_start = 768 + 10,
> + .vsync_end = 768 + 10 + 12,
> + .vtotal = 768 + 10 + 12 + 6,
> + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> +};
> +
> static const struct drm_display_mode auo_b116xak01_mode = {
> .clock = 69300,
> .hdisplay = 1366,
> @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
> EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
> EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
> EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
> - EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
> + EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"),
> + EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
Remove the trailing space from the string above now?
Aside from that:
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>
> drm_edid_get_panel_id() now just directly call edid_extract_panel_id().
>
> Merge them into one function.
>
> Signed-off-by: Hsin-Yi Wang <[email protected]>
> ---
> v4->v5: new
> ---
> drivers/gpu/drm/drm_edid.c | 39 ++++++++++++++++++--------------------
> 1 file changed, 18 insertions(+), 21 deletions(-)
Personally I wouldn't have objected to this being squashed into patch
#1, but I'm also OK as you have it.
Reviewed-by: Douglas Anderson <[email protected]>
Hi,
On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>
> @@ -2111,15 +2113,16 @@ static const struct edp_panel_entry edp_panels[] = {
> { /* sentinal */ }
> };
>
> -static const struct edp_panel_entry *find_edp_panel(u32 panel_id)
> +static const struct edp_panel_entry *find_edp_panel(u32 panel_id, const struct drm_edid *edid)
> {
> const struct edp_panel_entry *panel;
>
> - if (!panel_id)
> - return NULL;
> + for (panel = edp_panels; panel->ident.panel_id; panel++)
> + if (drm_edid_match(edid, &panel->ident))
> + return panel;
>
> - for (panel = edp_panels; panel->panel_id; panel++)
> - if (panel->panel_id == panel_id)
> + for (panel = edp_panels; panel->ident.panel_id; panel++)
> + if (panel->ident.panel_id == panel_id)
> return panel;
Reading through this another time, I wouldn't object to a comment
reminding the user why there are two loops here. Something like "Try
to match both the panel ID and name at first. This allows handling the
case where vendors incorrectly reused the same panel ID for multiple
panels that need different settings. If we don't get a match with the
name, that's OK. Panel ID _should_ be unique anyway". Feel free to
reword.
In any case:
Reviewed-by: Douglas Anderson <[email protected]>
-Doug
Hi,
On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>
> +static void
> +match_identity(const struct detailed_timing *timing, void *data)
> +{
> + struct drm_edid_match_closure *closure = data;
> + unsigned int i;
> + const char *name = closure->ident->name;
> + unsigned int name_len = strlen(name);
> + const char *desc = timing->data.other_data.data.str.str;
> + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.datastr.str);
> +
> + if (name_len > desc_len ||
> + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) ||
> + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING)))
> + return;
> +
> + if (strncmp(name, desc, name_len))
> + return;
> +
> + /* Allow trailing white spaces and \0. */
> + for (i = name_len; i < desc_len; i++) {
> + if (desc[i] == '\n')
> + break;
> + if (!isspace(desc[i]) && !desc[i])
> + return;
> + }
If my code analysis is correct, I think you'll reject the case where:
name = "foo"
desc[13] = "foo \0zzzzzzzz"
..but you'll accept these cases:
desc[13] = "foo \nzzzzzzzz"
desc[13] = "foo \0\0\0\0\0\0\0\0\0"
It somehow seems weird to me that a '\n' terminates the string but not a '\0'.
I would have done:
for (i = name_len; i < desc_len; i++) {
/* Consider \n or \0 to terminate the string */
if (desc[i] == '\n' || desc[i] == '\0')
break;
/* OK for spaces at the end, but non-space is a fail */
if (!isspace(desc[i]))
return;
}
> @@ -367,6 +367,12 @@ struct edid {
> u8 checksum;
> } __attribute__((packed));
>
> +/* EDID matching */
> +struct drm_edid_ident {
> + u32 panel_id;
> + const char *name;
Might not hurt to have a comment for panel_id saying that it's encoded
by drm_edid_encode_panel_id() so it's obvious what this random u32 is.
-Doug
On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
> >
> > +static void
> > +match_identity(const struct detailed_timing *timing, void *data)
> > +{
> > + struct drm_edid_match_closure *closure = data;
> > + unsigned int i;
> > + const char *name = closure->ident->name;
> > + unsigned int name_len = strlen(name);
> > + const char *desc = timing->data.other_data.data.str.str;
> > + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str);
> > +
> > + if (name_len > desc_len ||
> > + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) ||
> > + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING)))
> > + return;
> > +
> > + if (strncmp(name, desc, name_len))
> > + return;
> > +
> > + /* Allow trailing white spaces and \0. */
> > + for (i = name_len; i < desc_len; i++) {
> > + if (desc[i] == '\n')
> > + break;
> > + if (!isspace(desc[i]) && !desc[i])
> > + return;
> > + }
>
> If my code analysis is correct, I think you'll reject the case where:
>
> name = "foo"
> desc[13] = "foo \0zzzzzzzz"
>
> ...but you'll accept these cases:
>
> desc[13] = "foo \nzzzzzzzz"
> desc[13] = "foo \0\0\0\0\0\0\0\0\0"
>
> It somehow seems weird to me that a '\n' terminates the string but not a '\0'.
I'm also not sure about \0... based on
https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493,
they use \n as terminator. Maybe we should also reject \0 before\n?
Since it's not printable.
>
> I would have done:
>
> for (i = name_len; i < desc_len; i++) {
> /* Consider \n or \0 to terminate the string */
> if (desc[i] == '\n' || desc[i] == '\0')
> break;
> /* OK for spaces at the end, but non-space is a fail */
> if (!isspace(desc[i]))
> return;
> }
>
>
> > @@ -367,6 +367,12 @@ struct edid {
> > u8 checksum;
> > } __attribute__((packed));
> >
> > +/* EDID matching */
> > +struct drm_edid_ident {
> > + u32 panel_id;
> > + const char *name;
>
> Might not hurt to have a comment for panel_id saying that it's encoded
> by drm_edid_encode_panel_id() so it's obvious what this random u32 is.
>
>
> -Doug
Hi,
On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <[email protected]> wrote:
>
> On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <[email protected]> wrote:
> >
> > Hi,
> >
> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
> > >
> > > +static void
> > > +match_identity(const struct detailed_timing *timing, void *data)
> > > +{
> > > + struct drm_edid_match_closure *closure = data;
> > > + unsigned int i;
> > > + const char *name = closure->ident->name;
> > > + unsigned int name_len = strlen(name);
> > > + const char *desc = timing->data.other_data.data.str.str;
> > > + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str);
> > > +
> > > + if (name_len > desc_len ||
> > > + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) ||
> > > + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING)))
> > > + return;
> > > +
> > > + if (strncmp(name, desc, name_len))
> > > + return;
> > > +
> > > + /* Allow trailing white spaces and \0. */
> > > + for (i = name_len; i < desc_len; i++) {
> > > + if (desc[i] == '\n')
> > > + break;
> > > + if (!isspace(desc[i]) && !desc[i])
> > > + return;
> > > + }
> >
> > If my code analysis is correct, I think you'll reject the case where:
> >
> > name = "foo"
> > desc[13] = "foo \0zzzzzzzz"
> >
> > ...but you'll accept these cases:
> >
> > desc[13] = "foo \nzzzzzzzz"
> > desc[13] = "foo \0\0\0\0\0\0\0\0\0"
> >
> > It somehow seems weird to me that a '\n' terminates the string but not a '\0'.
>
> I'm also not sure about \0... based on
> https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493,
> they use \n as terminator. Maybe we should also reject \0 before\n?
> Since it's not printable.
Ah, OK. I guess the EDID spec simply doesn't allow for '\0' in there.
I guess in that case I'd prefer simply removing the code to handle
'\0' instead of treating it like space until we see some actual need
for it. So just get rid of the "!desc[i]" case?
-Doug
On Wed, 06 Mar 2024, Doug Anderson <[email protected]> wrote:
> Hi,
>
> On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>>
>> @@ -2764,58 +2764,71 @@ static u32 edid_extract_panel_id(const struct edid *edid)
>> }
>>
>> /**
>> - * drm_edid_get_panel_id - Get a panel's ID through DDC
>> - * @adapter: I2C adapter to use for DDC
>> + * drm_edid_get_panel_id - Get a panel's ID from EDID
>> + * @drm_edid: EDID that contains panel ID.
>> *
>> - * This function reads the first block of the EDID of a panel and (assuming
>> + * This function uses the first block of the EDID of a panel and (assuming
>> * that the EDID is valid) extracts the ID out of it. The ID is a 32-bit value
>> * (16 bits of manufacturer ID and 16 bits of per-manufacturer ID) that's
>> * supposed to be different for each different modem of panel.
>> *
>> + * Return: A 32-bit ID that should be different for each make/model of panel.
>> + * See the functions drm_edid_encode_panel_id() and
>> + * drm_edid_decode_panel_id() for some details on the structure of this
>> + * ID.
>> + */
>> +u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid)
>> +{
>
> I'd leave it up to Jani, but I'd wonder whether we need to confirm
> drm_edid->size here is at least as big as the base block. In other
> words: is there ever any chance that someone would have allocated a
> struct drm_edid but not actually read a full base block into it?
On the one hand, I've tried to make all the drm_edid based interfaces
handle all the cases (drm_edid == NULL, drm_edid->edid == NULL,
drm_edid->size < required) gracefully, but on the other hand, panel-edp
is the only user and this would go boom for you quickly if you passed in
a bogus drm_edid.
Adding the checks is definitely not wrong, but I'm not insisting.
Reviewed-by: Jani Nikula <[email protected]>
>
> In any case:
>
> Reviewed-by: Douglas Anderson <[email protected]>
--
Jani Nikula, Intel
On Wed, 06 Mar 2024, Doug Anderson <[email protected]> wrote:
> Hi,
>
> On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>>
>> drm_edid_get_panel_id() now just directly call edid_extract_panel_id().
>>
>> Merge them into one function.
>>
>> Signed-off-by: Hsin-Yi Wang <[email protected]>
>> ---
>> v4->v5: new
>> ---
>> drivers/gpu/drm/drm_edid.c | 39 ++++++++++++++++++--------------------
>> 1 file changed, 18 insertions(+), 21 deletions(-)
>
> Personally I wouldn't have objected to this being squashed into patch
> #1, but I'm also OK as you have it.
Ditto.
Reviewed-by: Jani Nikula <[email protected]>
>
> Reviewed-by: Douglas Anderson <[email protected]>
--
Jani Nikula, Intel
On Wed, 06 Mar 2024, Doug Anderson <[email protected]> wrote:
> Hi,
>
> On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <[email protected]> wrote:
>>
>> On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <[email protected]> wrote:
>> >
>> > Hi,
>> >
>> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>> > >
>> > > +static void
>> > > +match_identity(const struct detailed_timing *timing, void *data)
>> > > +{
>> > > + struct drm_edid_match_closure *closure = data;
>> > > + unsigned int i;
>> > > + const char *name = closure->ident->name;
>> > > + unsigned int name_len = strlen(name);
>> > > + const char *desc = timing->data.other_data.data.str.str;
>> > > + unsigned int desc_len = ARRAY_SIZE(timing->data.other_datadata.str.str);
>> > > +
>> > > + if (name_len > desc_len ||
>> > > + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) ||
>> > > + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING)))
>> > > + return;
>> > > +
>> > > + if (strncmp(name, desc, name_len))
>> > > + return;
>> > > +
>> > > + /* Allow trailing white spaces and \0. */
>> > > + for (i = name_len; i < desc_len; i++) {
>> > > + if (desc[i] == '\n')
>> > > + break;
>> > > + if (!isspace(desc[i]) && !desc[i])
>> > > + return;
>> > > + }
>> >
>> > If my code analysis is correct, I think you'll reject the case where:
>> >
>> > name = "foo"
>> > desc[13] = "foo \0zzzzzzzz"
>> >
>> > ...but you'll accept these cases:
>> >
>> > desc[13] = "foo \nzzzzzzzz"
>> > desc[13] = "foo \0\0\0\0\0\0\0\0\0"
>> >
>> > It somehow seems weird to me that a '\n' terminates the string but not a '\0'.
>>
>> I'm also not sure about \0... based on
>> https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493,
>> they use \n as terminator. Maybe we should also reject \0 before\n?
>> Since it's not printable.
>
> Ah, OK. I guess the EDID spec simply doesn't allow for '\0' in there.
> I guess in that case I'd prefer simply removing the code to handle
> '\0' instead of treating it like space until we see some actual need
> for it. So just get rid of the "!desc[i]" case?
The spec text, similar for both EDID_DETAIL_MONITOR_NAME and
EDID_DETAIL_MONITOR_STRING:
Up to 13 alphanumeric characters (using ASCII codes) may be used
to define the model name of the display product. The data shall
be sequenced such that the 1st byte (ASCII code) = the 1st
character, the 2nd byte (ASCII code) = the 2nd character,
etc. If there are less than 13 characters in the string, then
terminate the display product name string with ASCII code ‘0Ah’
(line feed) and pad the unused bytes in the field with ASCII
code ‘20h’ (space).
In theory, only checking for '\n' for termination should be enough, and
this is what drm_edid_get_monitor_name() does. If there's a space
*before* that, it should be considered part of the name, and not
ignored. (So my suggestion in reply to the previous version is wrong.)
However, since the match name uses NUL termination, maybe we should
ignore NULs *before* '\n'? Like so:
for (i = name_len; i < desc_len; i++) {
if (desc[i] == '\n')
break;
if (!desc[i])
return;
}
BR,
Jani.
>
> -Doug
--
Jani Nikula, Intel
On Wed, 06 Mar 2024, Doug Anderson <[email protected]> wrote:
> Hi,
>
> On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>>
>> @@ -1009,6 +1009,19 @@ static const struct panel_desc auo_b101ean01 = {
>> },
>> };
>>
>> +static const struct drm_display_mode auo_b116xa3_mode = {
>> + .clock = 70589,
>> + .hdisplay = 1366,
>> + .hsync_start = 1366 + 40,
>> + .hsync_end = 1366 + 40 + 40,
>> + .htotal = 1366 + 40 + 40 + 32,
>> + .vdisplay = 768,
>> + .vsync_start = 768 + 10,
>> + .vsync_end = 768 + 10 + 12,
>> + .vtotal = 768 + 10 + 12 + 6,
>> + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
>> +};
>> +
>> static const struct drm_display_mode auo_b116xak01_mode = {
>> .clock = 69300,
>> .hdisplay = 1366,
>> @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
>> EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
>> EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
>> EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
>> - EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
>> + EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"),
>> + EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
>
> Remove the trailing space from the string above now?
Maybe it actually needs to be considered part of the name; see my other
reply in the earlier patch.
>
> Aside from that:
>
> Reviewed-by: Douglas Anderson <[email protected]>
--
Jani Nikula, Intel
On Thu, Mar 7, 2024 at 5:20 AM Jani Nikula <[email protected]> wrote:
>
> On Wed, 06 Mar 2024, Doug Anderson <[email protected]> wrote:
> > Hi,
> >
> > On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <[email protected]> wrote:
> >>
> >> On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <[email protected]> wrote:
> >> >
> >> > Hi,
> >> >
> >> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
> >> > >
> >> > > +static void
> >> > > +match_identity(const struct detailed_timing *timing, void *data)
> >> > > +{
> >> > > + struct drm_edid_match_closure *closure = data;
> >> > > + unsigned int i;
> >> > > + const char *name = closure->ident->name;
> >> > > + unsigned int name_len = strlen(name);
> >> > > + const char *desc = timing->data.other_data.data.str.str;
> >> > > + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str);
> >> > > +
> >> > > + if (name_len > desc_len ||
> >> > > + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) ||
> >> > > + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING)))
> >> > > + return;
> >> > > +
> >> > > + if (strncmp(name, desc, name_len))
> >> > > + return;
> >> > > +
> >> > > + /* Allow trailing white spaces and \0. */
> >> > > + for (i = name_len; i < desc_len; i++) {
> >> > > + if (desc[i] == '\n')
> >> > > + break;
> >> > > + if (!isspace(desc[i]) && !desc[i])
> >> > > + return;
> >> > > + }
> >> >
> >> > If my code analysis is correct, I think you'll reject the case where:
> >> >
> >> > name = "foo"
> >> > desc[13] = "foo \0zzzzzzzz"
> >> >
> >> > ...but you'll accept these cases:
> >> >
> >> > desc[13] = "foo \nzzzzzzzz"
> >> > desc[13] = "foo \0\0\0\0\0\0\0\0\0"
> >> >
> >> > It somehow seems weird to me that a '\n' terminates the string but not a '\0'.
> >>
> >> I'm also not sure about \0... based on
> >> https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493,
> >> they use \n as terminator. Maybe we should also reject \0 before\n?
> >> Since it's not printable.
> >
> > Ah, OK. I guess the EDID spec simply doesn't allow for '\0' in there.
> > I guess in that case I'd prefer simply removing the code to handle
> > '\0' instead of treating it like space until we see some actual need
> > for it. So just get rid of the "!desc[i]" case?
>
> The spec text, similar for both EDID_DETAIL_MONITOR_NAME and
> EDID_DETAIL_MONITOR_STRING:
>
> Up to 13 alphanumeric characters (using ASCII codes) may be used
> to define the model name of the display product. The data shall
> be sequenced such that the 1st byte (ASCII code) = the 1st
> character, the 2nd byte (ASCII code) = the 2nd character,
> etc. If there are less than 13 characters in the string, then
> terminate the display product name string with ASCII code ‘0Ah’
> (line feed) and pad the unused bytes in the field with ASCII
> code ‘20h’ (space).
>
> In theory, only checking for '\n' for termination should be enough, and
> this is what drm_edid_get_monitor_name() does. If there's a space
> *before* that, it should be considered part of the name, and not
> ignored. (So my suggestion in reply to the previous version is wrong.)
>
> However, since the match name uses NUL termination, maybe we should
> ignore NULs *before* '\n'? Like so:
>
> for (i = name_len; i < desc_len; i++) {
> if (desc[i] == '\n')
> break;
> if (!desc[i])
> return;
> }
>
Allow trailing white spaces so we don't need to add the trailing white
space in edp_panel_entry.
https://lore.kernel.org/lkml/CAA8EJpr7LHvqeGXhbFQ8KNn0LGDuv19cw0i04qVUz51TJeSQrA@mail.gmail.com/
>
> BR,
> Jani.
>
>
> >
> > -Doug
>
> --
> Jani Nikula, Intel
On Thu, Mar 7, 2024 at 5:28 AM Jani Nikula <[email protected]> wrote:
>
> On Wed, 06 Mar 2024, Doug Anderson <[email protected]> wrote:
> > Hi,
> >
> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
> >>
> >> @@ -1009,6 +1009,19 @@ static const struct panel_desc auo_b101ean01 = {
> >> },
> >> };
> >>
> >> +static const struct drm_display_mode auo_b116xa3_mode = {
> >> + .clock = 70589,
> >> + .hdisplay = 1366,
> >> + .hsync_start = 1366 + 40,
> >> + .hsync_end = 1366 + 40 + 40,
> >> + .htotal = 1366 + 40 + 40 + 32,
> >> + .vdisplay = 768,
> >> + .vsync_start = 768 + 10,
> >> + .vsync_end = 768 + 10 + 12,
> >> + .vtotal = 768 + 10 + 12 + 6,
> >> + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> >> +};
> >> +
> >> static const struct drm_display_mode auo_b116xak01_mode = {
> >> .clock = 69300,
> >> .hdisplay = 1366,
> >> @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
> >> EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
> >> EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
> >> EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
> >> - EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
> >> + EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"),
> >> + EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
> >
> > Remove the trailing space from the string above now?
>
> Maybe it actually needs to be considered part of the name; see my other
> reply in the earlier patch.
>
I randomly checked 3 of the AUO panels that I had a datasheet with,
and all of them have a white space padding before \n.
The descriptor of that field is marked as "Reserved for definition",
unlike other characters, representing the name, which are marked with
"Manufacture P/N".
For this example, do we still want to consider the white space part of
the name? I know they didn't follow the spec exactly.
> >
> > Aside from that:
> >
> > Reviewed-by: Douglas Anderson <[email protected]>
>
> --
> Jani Nikula, Intel
On Thu, 07 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> On Thu, Mar 7, 2024 at 5:28 AM Jani Nikula <[email protected]> wrote:
>>
>> On Wed, 06 Mar 2024, Doug Anderson <[email protected]> wrote:
>> > Hi,
>> >
>> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>> >>
>> >> @@ -1009,6 +1009,19 @@ static const struct panel_desc auo_b101ean01 = {
>> >> },
>> >> };
>> >>
>> >> +static const struct drm_display_mode auo_b116xa3_mode = {
>> >> + .clock = 70589,
>> >> + .hdisplay = 1366,
>> >> + .hsync_start = 1366 + 40,
>> >> + .hsync_end = 1366 + 40 + 40,
>> >> + .htotal = 1366 + 40 + 40 + 32,
>> >> + .vdisplay = 768,
>> >> + .vsync_start = 768 + 10,
>> >> + .vsync_end = 768 + 10 + 12,
>> >> + .vtotal = 768 + 10 + 12 + 6,
>> >> + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
>> >> +};
>> >> +
>> >> static const struct drm_display_mode auo_b116xak01_mode = {
>> >> .clock = 69300,
>> >> .hdisplay = 1366,
>> >> @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
>> >> EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
>> >> EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
>> >> EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
>> >> - EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
>> >> + EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"),
>> >> + EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
>> >
>> > Remove the trailing space from the string above now?
>>
>> Maybe it actually needs to be considered part of the name; see my other
>> reply in the earlier patch.
>>
> I randomly checked 3 of the AUO panels that I had a datasheet with,
> and all of them have a white space padding before \n.
> The descriptor of that field is marked as "Reserved for definition",
> unlike other characters, representing the name, which are marked with
> "Manufacture P/N".
>
> For this example, do we still want to consider the white space part of
> the name? I know they didn't follow the spec exactly.
If there's one thing that's for sure, EDIDs are full of stuff like this,
across the board.
Ignoring the whitespace at the end seemed reasonable, initially, to me
too. But the question is, if we start catering for this, what else
should we cater for? Do we keep adding "reasonable" interpretations, or
just go by the spec?
BR,
Jani.
>
>> >
>> > Aside from that:
>> >
>> > Reviewed-by: Douglas Anderson <[email protected]>
>>
>> --
>> Jani Nikula, Intel
--
Jani Nikula, Intel
Hi,
On Thu, Mar 7, 2024 at 12:28 PM Jani Nikula <[email protected]> wrote:
>
> On Thu, 07 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> > On Thu, Mar 7, 2024 at 5:28 AM Jani Nikula <[email protected]> wrote:
> >>
> >> On Wed, 06 Mar 2024, Doug Anderson <[email protected]> wrote:
> >> > Hi,
> >> >
> >> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
> >> >>
> >> >> @@ -1009,6 +1009,19 @@ static const struct panel_desc auo_b101ean01 = {
> >> >> },
> >> >> };
> >> >>
> >> >> +static const struct drm_display_mode auo_b116xa3_mode = {
> >> >> + .clock = 70589,
> >> >> + .hdisplay = 1366,
> >> >> + .hsync_start = 1366 + 40,
> >> >> + .hsync_end = 1366 + 40 + 40,
> >> >> + .htotal = 1366 + 40 + 40 + 32,
> >> >> + .vdisplay = 768,
> >> >> + .vsync_start = 768 + 10,
> >> >> + .vsync_end = 768 + 10 + 12,
> >> >> + .vtotal = 768 + 10 + 12 + 6,
> >> >> + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC,
> >> >> +};
> >> >> +
> >> >> static const struct drm_display_mode auo_b116xak01_mode = {
> >> >> .clock = 69300,
> >> >> .hdisplay = 1366,
> >> >> @@ -1990,7 +2003,9 @@ static const struct edp_panel_entry edp_panels[] = {
> >> >> EDP_PANEL_ENTRY('A', 'U', 'O', 0x239b, &delay_200_500_e50, "B116XAN06.1"),
> >> >> EDP_PANEL_ENTRY('A', 'U', 'O', 0x255c, &delay_200_500_e50, "B116XTN02.5"),
> >> >> EDP_PANEL_ENTRY('A', 'U', 'O', 0x403d, &delay_200_500_e50, "B140HAN04.0"),
> >> >> - EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0"),
> >> >> + EDP_PANEL_ENTRY('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAN04.0"),
> >> >> + EDP_PANEL_ENTRY2('A', 'U', 'O', 0x405c, &auo_b116xak01.delay, "B116XAK01.0 ",
> >> >
> >> > Remove the trailing space from the string above now?
> >>
> >> Maybe it actually needs to be considered part of the name; see my other
> >> reply in the earlier patch.
> >>
> > I randomly checked 3 of the AUO panels that I had a datasheet with,
> > and all of them have a white space padding before \n.
> > The descriptor of that field is marked as "Reserved for definition",
> > unlike other characters, representing the name, which are marked with
> > "Manufacture P/N".
> >
> > For this example, do we still want to consider the white space part of
> > the name? I know they didn't follow the spec exactly.
>
> If there's one thing that's for sure, EDIDs are full of stuff like this,
> across the board.
>
> Ignoring the whitespace at the end seemed reasonable, initially, to me
> too. But the question is, if we start catering for this, what else
> should we cater for? Do we keep adding "reasonable" interpretations, or
> just go by the spec?
Personally, I don't really care a whole lot either way. If I had to
make a judgement call I think it's a little cleaner the way Hsin-Yi
has it where we ignore whitespace at the end. Given that Dmitry also
suggested ignoring whitespace at the end [1] I guess I'd believe that
he also feels it's a little cleaner that way. However, If the only way
to get the patch series landed is to put the space at the end of the
name in panel-edp.c then I'm OK with that.
In terms of what else we should cater to, I guess we'd have to answer
that question when it comes up, with a bias against adding more
special case rules. _Hopefully_ it won't be common that we even need
this code and it will be the exception rather than the rule that
panels with incompatible timings have the same panel ID anyway...
In any case, hopefully the above explains my opinion on this. If you
feel strongly that we should remove the code handling whitespace at
the end then so be it. If you're on the fence then I guess I'd say
let's keep it...
[1] https://lore.kernel.org/lkml/CAA8EJpr7LHvqeGXhbFQ8KNn0LGDuv19cw0i04qVUz51TJeSQrA@mail.gmail.com/
On Thu, 07 Mar 2024, Doug Anderson <[email protected]> wrote:
> On Thu, Mar 7, 2024 at 12:28 PM Jani Nikula <[email protected]> wrote:
>> If there's one thing that's for sure, EDIDs are full of stuff like this,
>> across the board.
>>
>> Ignoring the whitespace at the end seemed reasonable, initially, to me
>> too. But the question is, if we start catering for this, what else
>> should we cater for? Do we keep adding "reasonable" interpretations, or
>> just go by the spec?
>
> Personally, I don't really care a whole lot either way. If I had to
> make a judgement call I think it's a little cleaner the way Hsin-Yi
> has it where we ignore whitespace at the end. Given that Dmitry also
> suggested ignoring whitespace at the end [1] I guess I'd believe that
> he also feels it's a little cleaner that way. However, If the only way
> to get the patch series landed is to put the space at the end of the
> name in panel-edp.c then I'm OK with that.
>
> In terms of what else we should cater to, I guess we'd have to answer
> that question when it comes up, with a bias against adding more
> special case rules. _Hopefully_ it won't be common that we even need
> this code and it will be the exception rather than the rule that
> panels with incompatible timings have the same panel ID anyway...
>
> In any case, hopefully the above explains my opinion on this. If you
> feel strongly that we should remove the code handling whitespace at
> the end then so be it. If you're on the fence then I guess I'd say
> let's keep it...
No, I don't feel strongly, let's go with this. It's not like it's cast
in stone either.
BR,
Jani.
>
>
> [1] https://lore.kernel.org/lkml/CAA8EJpr7LHvqeGXhbFQ8KNn0LGDuv19cw0i04qVUz51TJeSQrA@mail.gmail.com/
--
Jani Nikula, Intel
On Thu, 07 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> On Thu, Mar 7, 2024 at 5:20 AM Jani Nikula <[email protected]> wrote:
>>
>> On Wed, 06 Mar 2024, Doug Anderson <[email protected]> wrote:
>> > Hi,
>> >
>> > On Wed, Mar 6, 2024 at 4:20 PM Hsin-Yi Wang <[email protected]> wrote:
>> >>
>> >> On Wed, Mar 6, 2024 at 3:30 PM Doug Anderson <[email protected]> wrote:
>> >> >
>> >> > Hi,
>> >> >
>> >> > On Wed, Mar 6, 2024 at 12:04 PM Hsin-Yi Wang <[email protected]> wrote:
>> >> > >
>> >> > > +static void
>> >> > > +match_identity(const struct detailed_timing *timing, void *data)
>> >> > > +{
>> >> > > + struct drm_edid_match_closure *closure = data;
>> >> > > + unsigned int i;
>> >> > > + const char *name = closure->ident->name;
>> >> > > + unsigned int name_len = strlen(name);
>> >> > > + const char *desc = timing->data.other_data.data.str.str;
>> >> > > + unsigned int desc_len = ARRAY_SIZE(timing->data.other_data.data.str.str);
>> >> > > +
>> >> > > + if (name_len > desc_len ||
>> >> > > + !(is_display_descriptor(timing, EDID_DETAIL_MONITOR_NAME) ||
>> >> > > + is_display_descriptor(timing, EDID_DETAIL_MONITOR_STRING)))
>> >> > > + return;
>> >> > > +
>> >> > > + if (strncmp(name, desc, name_len))
>> >> > > + return;
>> >> > > +
>> >> > > + /* Allow trailing white spaces and \0. */
>> >> > > + for (i = name_len; i < desc_len; i++) {
>> >> > > + if (desc[i] == '\n')
>> >> > > + break;
>> >> > > + if (!isspace(desc[i]) && !desc[i])
>> >> > > + return;
>> >> > > + }
>> >> >
>> >> > If my code analysis is correct, I think you'll reject the case where:
>> >> >
>> >> > name = "foo"
>> >> > desc[13] = "foo \0zzzzzzzz"
>> >> >
>> >> > ...but you'll accept these cases:
>> >> >
>> >> > desc[13] = "foo \nzzzzzzzz"
>> >> > desc[13] = "foo \0\0\0\0\0\0\0\0\0"
>> >> >
>> >> > It somehow seems weird to me that a '\n' terminates the string but not a '\0'.
>> >>
>> >> I'm also not sure about \0... based on
>> >> https://git.linuxtv.org/edid-decode.git/tree/parse-base-block.cpp#n493,
>> >> they use \n as terminator. Maybe we should also reject \0 before\n?
>> >> Since it's not printable.
>> >
>> > Ah, OK. I guess the EDID spec simply doesn't allow for '\0' in there.
>> > I guess in that case I'd prefer simply removing the code to handle
>> > '\0' instead of treating it like space until we see some actual need
>> > for it. So just get rid of the "!desc[i]" case?
>>
>> The spec text, similar for both EDID_DETAIL_MONITOR_NAME and
>> EDID_DETAIL_MONITOR_STRING:
>>
>> Up to 13 alphanumeric characters (using ASCII codes) may be used
>> to define the model name of the display product. The data shall
>> be sequenced such that the 1st byte (ASCII code) = the 1st
>> character, the 2nd byte (ASCII code) = the 2nd character,
>> etc. If there are less than 13 characters in the string, then
>> terminate the display product name string with ASCII code ‘0Ah’
>> (line feed) and pad the unused bytes in the field with ASCII
>> code ‘20h’ (space).
>>
>> In theory, only checking for '\n' for termination should be enough, and
>> this is what drm_edid_get_monitor_name() does. If there's a space
>> *before* that, it should be considered part of the name, and not
>> ignored. (So my suggestion in reply to the previous version is wrong.)
>>
>> However, since the match name uses NUL termination, maybe we should
>> ignore NULs *before* '\n'? Like so:
>>
>> for (i = name_len; i < desc_len; i++) {
>> if (desc[i] == '\n')
>> break;
>> if (!desc[i])
>> return;
>> }
>>
> Allow trailing white spaces so we don't need to add the trailing white
> space in edp_panel_entry.
Just so it's clear here too: Agreed.
>
> https://lore.kernel.org/lkml/CAA8EJpr7LHvqeGXhbFQ8KNn0LGDuv19cw0i04qVUz51TJeSQrA@mail.gmail.com/
>
>>
>> BR,
>> Jani.
>>
>>
>> >
>> > -Doug
>>
>> --
>> Jani Nikula, Intel
--
Jani Nikula, Intel