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 both name and id. If not found, fallback to match id.
v1: https://lore.kernel.org/lkml/[email protected]
v2: https://lore.kernel.org/lkml/[email protected]
Hsin-Yi Wang (4):
drm_edid: Add a function to get EDID base block
drm/edid: Add a function to check monitor string
drm/panel: panel-edp: Match edp_panels with panel name
drm/panel: panel-edp: Fix AUO 0x405c panel naming and add a variant
drivers/gpu/drm/drm_edid.c | 107 +++++++++++++++++++++++-------
drivers/gpu/drm/panel/panel-edp.c | 42 +++++++++---
include/drm/drm_edid.h | 8 ++-
3 files changed, 123 insertions(+), 34 deletions(-)
--
2.44.0.rc1.240.g4c46232300-goog
Add a function to check if the EDID base block contains a given string.
One of the use cases is fetching panel from a list of panel names, since
some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
instead of EDID_DETAIL_MONITOR_NAME.
Signed-off-by: Hsin-Yi Wang <[email protected]>
---
v2->v3: move string matching to drm_edid
---
drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
include/drm/drm_edid.h | 1 +
2 files changed, 50 insertions(+)
diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 13454bc64ca2..fcdc2bd143dd 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
}
EXPORT_SYMBOL(drm_edid_get_panel_id);
+/**
+ * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
+ * @base_block: EDID base block to check.
+ * @str: pointer to a character array to hold the string to be checked.
+ *
+ * Check if the detailed timings section of a EDID base block has the given
+ * string.
+ *
+ * Return: True if the EDID base block contains the string, false otherwise.
+ */
+bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
+{
+ unsigned int i, j, k, buflen = strlen(str);
+
+ for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
+ struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
+ unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
+
+ if (buflen > size || timing->pixel_clock != 0 ||
+ timing->data.other_data.pad1 != 0 ||
+ (timing->data.other_data.type != EDID_DETAIL_MONITOR_NAME &&
+ timing->data.other_data.type != EDID_DETAIL_MONITOR_STRING))
+ continue;
+
+ for (j = 0; j < buflen; j++) {
+ char c = timing->data.other_data.data.str.str[j];
+
+ if (c != str[j] || c == '\n')
+ break;
+ }
+
+ if (j == buflen) {
+ /* Allow trailing white spaces. */
+ for (k = j; k < size; k++) {
+ char c = timing->data.other_data.data.str.str[k];
+
+ if (c == '\n')
+ return true;
+ else if (c != ' ')
+ break;
+ }
+ if (k == size)
+ return true;
+ }
+ }
+
+ return false;
+}
+
/**
* drm_edid_get_base_block - Get a panel's EDID base block
* @adapter: I2C adapter to use for DDC
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 2455d6ab2221..248ddb0a6b5d 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
struct i2c_adapter *adapter);
struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
+bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
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.rc1.240.g4c46232300-goog
On Mon, 04 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> Add a function to check if the EDID base block contains a given string.
>
> One of the use cases is fetching panel from a list of panel names, since
> some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
> instead of EDID_DETAIL_MONITOR_NAME.
>
> Signed-off-by: Hsin-Yi Wang <[email protected]>
> ---
> v2->v3: move string matching to drm_edid
> ---
> drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
> include/drm/drm_edid.h | 1 +
> 2 files changed, 50 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 13454bc64ca2..fcdc2bd143dd 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
> }
> EXPORT_SYMBOL(drm_edid_get_panel_id);
>
> +/**
> + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
> + * @base_block: EDID base block to check.
> + * @str: pointer to a character array to hold the string to be checked.
> + *
> + * Check if the detailed timings section of a EDID base block has the given
> + * string.
> + *
> + * Return: True if the EDID base block contains the string, false otherwise.
> + */
> +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
> +{
> + unsigned int i, j, k, buflen = strlen(str);
> +
> + for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
> + struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
> + unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> +
> + if (buflen > size || timing->pixel_clock != 0 ||
> + timing->data.other_data.pad1 != 0 ||
> + (timing->data.other_data.type != EDID_DETAIL_MONITOR_NAME &&
> + timing->data.other_data.type != EDID_DETAIL_MONITOR_STRING))
> + continue;
> +
> + for (j = 0; j < buflen; j++) {
> + char c = timing->data.other_data.data.str.str[j];
> +
> + if (c != str[j] || c == '\n')
> + break;
> + }
> +
> + if (j == buflen) {
> + /* Allow trailing white spaces. */
> + for (k = j; k < size; k++) {
> + char c = timing->data.other_data.data.str.str[k];
> +
> + if (c == '\n')
> + return true;
> + else if (c != ' ')
> + break;
> + }
> + if (k == size)
> + return true;
> + }
> + }
> +
> + return false;
> +}
> +
So we've put a lot of effort into converting from struct edid to struct
drm_edid, passing that around in drm_edid.c, with the allocation size it
provides, and generally cleaning stuff up.
I'm not at all happy to see *another* struct added just for the base
block, and detailed timing iteration as well as monitor name parsing
duplicated.
With struct drm_edid you can actually return an EDID that only has the
base block and size 128, even if the EDID indicates more
extensions. Because the whole thing is *designed* to handle that
gracefully. The allocated size matters, not what the blob originating
outside of the kernel tells you.
What I'm thinking is:
- Add some struct drm_edid_ident or similar. Add all the information
that's needed to identify a panel there. I guess initially that's
panel_id and name.
struct drm_edid_ident {
u32 panel_id;
const char *name;
};
- Add function:
bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
Check if stuff in ident matches drm_edid. You can use and extend the
existing drm_edid based iteration etc. in
drm_edid.c. Straightforward. The fields in ident can trivially be
extended later, and the stuff can be useful for other drivers and
quirks etc.
- Restructure struct edp_panel_entry to contain struct
drm_edid_ident. Change the iteration of edp_panels array to use
drm_edid_match() on the array elements and the edid.
- Add a function to read the EDID base block *but* make it return const
struct drm_edid *. Add warnings in the comments that it's only for
panel and for transition until it switches to reading full EDIDs.
const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
This is the *only* hackish part of the whole thing, and it's nicely
isolated. For the most part you can use drm_edid_get_panel_id() code
for this, just return the blob wrapped in a struct drm_edid envelope.
- Remove function:
u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
- Refactor edid_quirk_list to use the same id struct and match function
and mechanism within drm_edid.c (can be follow-up too).
- Once you change the panel code to read the whole EDID using
drm_edid_read family of functions in the future, you don't have to
change *anything* about the iteration or matching or anything, because
it's already passing struct drm_edid around.
I hope this covers everything.
BR,
Jani.
> /**
> * drm_edid_get_base_block - Get a panel's EDID base block
> * @adapter: I2C adapter to use for DDC
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 2455d6ab2221..248ddb0a6b5d 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> struct i2c_adapter *adapter);
> struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
> u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
> +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
> struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
> struct i2c_adapter *adapter);
> struct edid *drm_edid_duplicate(const struct edid *edid);
--
Jani Nikula, Intel
On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <[email protected]> wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> > Add a function to check if the EDID base block contains a given string.
> >
> > One of the use cases is fetching panel from a list of panel names, since
> > some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
> > instead of EDID_DETAIL_MONITOR_NAME.
> >
> > Signed-off-by: Hsin-Yi Wang <[email protected]>
> > ---
> > v2->v3: move string matching to drm_edid
> > ---
> > drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
> > include/drm/drm_edid.h | 1 +
> > 2 files changed, 50 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 13454bc64ca2..fcdc2bd143dd 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
> > }
> > EXPORT_SYMBOL(drm_edid_get_panel_id);
> >
> > +/**
> > + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
> > + * @base_block: EDID base block to check.
> > + * @str: pointer to a character array to hold the string to be checked.
> > + *
> > + * Check if the detailed timings section of a EDID base block has the given
> > + * string.
> > + *
> > + * Return: True if the EDID base block contains the string, false otherwise.
> > + */
> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
> > +{
> > + unsigned int i, j, k, buflen = strlen(str);
> > +
> > + for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
> > + struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
> > + unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> > +
> > + if (buflen > size || timing->pixel_clock != 0 ||
> > + timing->data.other_data.pad1 != 0 ||
> > + (timing->data.other_data.type != EDID_DETAIL_MONITOR_NAME &&
> > + timing->data.other_data.type != EDID_DETAIL_MONITOR_STRING))
> > + continue;
> > +
> > + for (j = 0; j < buflen; j++) {
> > + char c = timing->data.other_data.data.str.str[j];
> > +
> > + if (c != str[j] || c == '\n')
> > + break;
> > + }
> > +
> > + if (j == buflen) {
> > + /* Allow trailing white spaces. */
> > + for (k = j; k < size; k++) {
> > + char c = timing->data.other_data.data.str.str[k];
> > +
> > + if (c == '\n')
> > + return true;
> > + else if (c != ' ')
> > + break;
> > + }
> > + if (k == size)
> > + return true;
> > + }
> > + }
> > +
> > + return false;
> > +}
> > +
>
> So we've put a lot of effort into converting from struct edid to struct
> drm_edid, passing that around in drm_edid.c, with the allocation size it
> provides, and generally cleaning stuff up.
>
> I'm not at all happy to see *another* struct added just for the base
> block, and detailed timing iteration as well as monitor name parsing
> duplicated.
>
> With struct drm_edid you can actually return an EDID that only has the
> base block and size 128, even if the EDID indicates more
> extensions. Because the whole thing is *designed* to handle that
> gracefully. The allocated size matters, not what the blob originating
> outside of the kernel tells you.
>
> What I'm thinking is:
>
> - Add some struct drm_edid_ident or similar. Add all the information
> that's needed to identify a panel there. I guess initially that's
> panel_id and name.
>
> struct drm_edid_ident {
> u32 panel_id;
> const char *name;
> };
>
> - Add function:
>
> bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
>
> Check if stuff in ident matches drm_edid. You can use and extend the
> existing drm_edid based iteration etc. in
> drm_edid.c. Straightforward. The fields in ident can trivially be
> extended later, and the stuff can be useful for other drivers and
> quirks etc.
>
> - Restructure struct edp_panel_entry to contain struct
> drm_edid_ident. Change the iteration of edp_panels array to use
> drm_edid_match() on the array elements and the edid.
>
> - Add a function to read the EDID base block *but* make it return const
> struct drm_edid *. Add warnings in the comments that it's only for
> panel and for transition until it switches to reading full EDIDs.
>
> const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
>
> This is the *only* hackish part of the whole thing, and it's nicely
> isolated. For the most part you can use drm_edid_get_panel_id() code
> for this, just return the blob wrapped in a struct drm_edid envelope.
To clarify:
struct drm_edid currently is only internal to drm_edid.c. So with
change we will have to move it to the header drm_edid.h
>
> - Remove function:
>
> u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>
Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
*);? Given that we still need to parse id from
drm_edid_read_base_block().
> - Refactor edid_quirk_list to use the same id struct and match function
> and mechanism within drm_edid.c (can be follow-up too).
>
edid_quirk currently doesn't have panel names in it, and it might be a
bit difficult to get all the correct names of these panels without
having the datasheets.
One way is to leave the name as null and if the name is empty and skip
matching the name in drm_edid_match().
> - Once you change the panel code to read the whole EDID using
> drm_edid_read family of functions in the future, you don't have to
> change *anything* about the iteration or matching or anything, because
> it's already passing struct drm_edid around.
>
>
> I hope this covers everything.
>
> BR,
> Jani.
>
>
> > /**
> > * drm_edid_get_base_block - Get a panel's EDID base block
> > * @adapter: I2C adapter to use for DDC
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 2455d6ab2221..248ddb0a6b5d 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> > struct i2c_adapter *adapter);
> > struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
> > u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
> > struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
> > struct i2c_adapter *adapter);
> > struct edid *drm_edid_duplicate(const struct edid *edid);
>
> --
> Jani Nikula, Intel
On Mon, 4 Mar 2024 at 22:38, Jani Nikula <[email protected]> wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> > Add a function to check if the EDID base block contains a given string.
> >
> > One of the use cases is fetching panel from a list of panel names, since
> > some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
> > instead of EDID_DETAIL_MONITOR_NAME.
> >
> > Signed-off-by: Hsin-Yi Wang <[email protected]>
> > ---
> > v2->v3: move string matching to drm_edid
> > ---
> > drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
> > include/drm/drm_edid.h | 1 +
> > 2 files changed, 50 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> > index 13454bc64ca2..fcdc2bd143dd 100644
> > --- a/drivers/gpu/drm/drm_edid.c
> > +++ b/drivers/gpu/drm/drm_edid.c
> > @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
> > }
> > EXPORT_SYMBOL(drm_edid_get_panel_id);
> >
> > +/**
> > + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
> > + * @base_block: EDID base block to check.
> > + * @str: pointer to a character array to hold the string to be checked.
> > + *
> > + * Check if the detailed timings section of a EDID base block has the given
> > + * string.
> > + *
> > + * Return: True if the EDID base block contains the string, false otherwise.
> > + */
> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
> > +{
> > + unsigned int i, j, k, buflen = strlen(str);
> > +
> > + for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
> > + struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
> > + unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> > +
> > + if (buflen > size || timing->pixel_clock != 0 ||
> > + timing->data.other_data.pad1 != 0 ||
> > + (timing->data.other_data.type != EDID_DETAIL_MONITOR_NAME &&
> > + timing->data.other_data.type != EDID_DETAIL_MONITOR_STRING))
> > + continue;
> > +
> > + for (j = 0; j < buflen; j++) {
> > + char c = timing->data.other_data.data.str.str[j];
> > +
> > + if (c != str[j] || c == '\n')
> > + break;
> > + }
> > +
> > + if (j == buflen) {
> > + /* Allow trailing white spaces. */
> > + for (k = j; k < size; k++) {
> > + char c = timing->data.other_data.data.str.str[k];
> > +
> > + if (c == '\n')
> > + return true;
> > + else if (c != ' ')
> > + break;
> > + }
> > + if (k == size)
> > + return true;
> > + }
> > + }
> > +
> > + return false;
> > +}
> > +
>
> So we've put a lot of effort into converting from struct edid to struct
> drm_edid, passing that around in drm_edid.c, with the allocation size it
> provides, and generally cleaning stuff up.
>
> I'm not at all happy to see *another* struct added just for the base
> block, and detailed timing iteration as well as monitor name parsing
> duplicated.
>
> With struct drm_edid you can actually return an EDID that only has the
> base block and size 128, even if the EDID indicates more
> extensions. Because the whole thing is *designed* to handle that
> gracefully. The allocated size matters, not what the blob originating
> outside of the kernel tells you.
>
> What I'm thinking is:
>
> - Add some struct drm_edid_ident or similar. Add all the information
> that's needed to identify a panel there. I guess initially that's
> panel_id and name.
>
> struct drm_edid_ident {
> u32 panel_id;
> const char *name;
> };
>
> - Add function:
>
> bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
>
> Check if stuff in ident matches drm_edid. You can use and extend the
> existing drm_edid based iteration etc. in
> drm_edid.c. Straightforward. The fields in ident can trivially be
> extended later, and the stuff can be useful for other drivers and
> quirks etc.
That sounds perfect!
>
> - Restructure struct edp_panel_entry to contain struct
> drm_edid_ident. Change the iteration of edp_panels array to use
> drm_edid_match() on the array elements and the edid.
>
> - Add a function to read the EDID base block *but* make it return const
> struct drm_edid *. Add warnings in the comments that it's only for
> panel and for transition until it switches to reading full EDIDs.
>
> const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
>
> This is the *only* hackish part of the whole thing, and it's nicely
> isolated. For the most part you can use drm_edid_get_panel_id() code
> for this, just return the blob wrapped in a struct drm_edid envelope.
>
> - Remove function:
>
> u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>
> - Refactor edid_quirk_list to use the same id struct and match function
> and mechanism within drm_edid.c (can be follow-up too).
I wonder if we can take one step further and merge edp_panels to
edid_quirk_list (as one of the follow-ups). Maybe just some bits of
it.
> - Once you change the panel code to read the whole EDID using
> drm_edid_read family of functions in the future, you don't have to
> change *anything* about the iteration or matching or anything, because
> it's already passing struct drm_edid around.
>
>
> I hope this covers everything.
>
> BR,
> Jani.
>
>
> > /**
> > * drm_edid_get_base_block - Get a panel's EDID base block
> > * @adapter: I2C adapter to use for DDC
> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> > index 2455d6ab2221..248ddb0a6b5d 100644
> > --- a/include/drm/drm_edid.h
> > +++ b/include/drm/drm_edid.h
> > @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> > struct i2c_adapter *adapter);
> > struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
> > u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
> > struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
> > struct i2c_adapter *adapter);
> > struct edid *drm_edid_duplicate(const struct edid *edid);
>
> --
> Jani Nikula, Intel
--
With best wishes
Dmitry
On Mon, 04 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <[email protected]> wrote:
>>
>> On Mon, 04 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
>> > Add a function to check if the EDID base block contains a given string.
>> >
>> > One of the use cases is fetching panel from a list of panel names, since
>> > some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
>> > instead of EDID_DETAIL_MONITOR_NAME.
>> >
>> > Signed-off-by: Hsin-Yi Wang <[email protected]>
>> > ---
>> > v2->v3: move string matching to drm_edid
>> > ---
>> > drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
>> > include/drm/drm_edid.h | 1 +
>> > 2 files changed, 50 insertions(+)
>> >
>> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> > index 13454bc64ca2..fcdc2bd143dd 100644
>> > --- a/drivers/gpu/drm/drm_edid.c
>> > +++ b/drivers/gpu/drm/drm_edid.c
>> > @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
>> > }
>> > EXPORT_SYMBOL(drm_edid_get_panel_id);
>> >
>> > +/**
>> > + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
>> > + * @base_block: EDID base block to check.
>> > + * @str: pointer to a character array to hold the string to be checked.
>> > + *
>> > + * Check if the detailed timings section of a EDID base block has the given
>> > + * string.
>> > + *
>> > + * Return: True if the EDID base block contains the string, false otherwise.
>> > + */
>> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
>> > +{
>> > + unsigned int i, j, k, buflen = strlen(str);
>> > +
>> > + for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
>> > + struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
>> > + unsigned int size = ARRAY_SIZE(timing->data.other_datadata.str.str);
>> > +
>> > + if (buflen > size || timing->pixel_clock != 0 ||
>> > + timing->data.other_data.pad1 != 0 ||
>> > + (timing->data.other_data.type != EDID_DETAIL_MONITOR_NAME &&
>> > + timing->data.other_data.type != EDID_DETAIL_MONITOR_STRING))
>> > + continue;
>> > +
>> > + for (j = 0; j < buflen; j++) {
>> > + char c = timing->data.other_data.data.str.str[j];
>> > +
>> > + if (c != str[j] || c == '\n')
>> > + break;
>> > + }
>> > +
>> > + if (j == buflen) {
>> > + /* Allow trailing white spaces. */
>> > + for (k = j; k < size; k++) {
>> > + char c = timing->data.other_data.data.str.str[k];
>> > +
>> > + if (c == '\n')
>> > + return true;
>> > + else if (c != ' ')
>> > + break;
>> > + }
>> > + if (k == size)
>> > + return true;
>> > + }
>> > + }
>> > +
>> > + return false;
>> > +}
>> > +
>>
>> So we've put a lot of effort into converting from struct edid to struct
>> drm_edid, passing that around in drm_edid.c, with the allocation size it
>> provides, and generally cleaning stuff up.
>>
>> I'm not at all happy to see *another* struct added just for the base
>> block, and detailed timing iteration as well as monitor name parsing
>> duplicated.
>>
>> With struct drm_edid you can actually return an EDID that only has the
>> base block and size 128, even if the EDID indicates more
>> extensions. Because the whole thing is *designed* to handle that
>> gracefully. The allocated size matters, not what the blob originating
>> outside of the kernel tells you.
>>
>> What I'm thinking is:
>>
>> - Add some struct drm_edid_ident or similar. Add all the information
>> that's needed to identify a panel there. I guess initially that's
>> panel_id and name.
>>
>> struct drm_edid_ident {
>> u32 panel_id;
>> const char *name;
>> };
>>
>> - Add function:
>>
>> bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
>>
>> Check if stuff in ident matches drm_edid. You can use and extend the
>> existing drm_edid based iteration etc. in
>> drm_edid.c. Straightforward. The fields in ident can trivially be
>> extended later, and the stuff can be useful for other drivers and
>> quirks etc.
>>
>> - Restructure struct edp_panel_entry to contain struct
>> drm_edid_ident. Change the iteration of edp_panels array to use
>> drm_edid_match() on the array elements and the edid.
>>
>> - Add a function to read the EDID base block *but* make it return const
>> struct drm_edid *. Add warnings in the comments that it's only for
>> panel and for transition until it switches to reading full EDIDs.
>>
>> const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
>>
>> This is the *only* hackish part of the whole thing, and it's nicely
>> isolated. For the most part you can use drm_edid_get_panel_id() code
>> for this, just return the blob wrapped in a struct drm_edid envelope.
>
> To clarify:
> struct drm_edid currently is only internal to drm_edid.c. So with
> change we will have to move it to the header drm_edid.h
Absolutely not, struct drm_edid must remain an opaque type. The point is
that you ask drm_edid.c if there's a match or not, and the panel code
does not need to care what's inside struct drm_edid.
>
>>
>> - Remove function:
>>
>> u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>>
>
> Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> *);? Given that we still need to parse id from
> drm_edid_read_base_block().
No, we no longer need to parse the id outside of drm_edid.c. You'll have
the id's in panel code in the form of struct drm_edid_ident (or
whatever), and use the match function to see if the opaque drm_edid
matches.
>
>> - Refactor edid_quirk_list to use the same id struct and match function
>> and mechanism within drm_edid.c (can be follow-up too).
>>
>
> edid_quirk currently doesn't have panel names in it, and it might be a
> bit difficult to get all the correct names of these panels without
> having the datasheets.
> One way is to leave the name as null and if the name is empty and skip
> matching the name in drm_edid_match().
Exactly. NULL in drm_edid_ident would mean "don't care". I think most of
the ones in panel code also won't use the name for matching.
BR,
Jani.
>
>> - Once you change the panel code to read the whole EDID using
>> drm_edid_read family of functions in the future, you don't have to
>> change *anything* about the iteration or matching or anything, because
>> it's already passing struct drm_edid around.
>>
>>
>> I hope this covers everything.
>>
>> BR,
>> Jani.
>>
>>
>> > /**
>> > * drm_edid_get_base_block - Get a panel's EDID base block
>> > * @adapter: I2C adapter to use for DDC
>> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> > index 2455d6ab2221..248ddb0a6b5d 100644
>> > --- a/include/drm/drm_edid.h
>> > +++ b/include/drm/drm_edid.h
>> > @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>> > struct i2c_adapter *adapter);
>> > struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
>> > u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
>> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
>> > struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>> > struct i2c_adapter *adapter);
>> > struct edid *drm_edid_duplicate(const struct edid *edid);
>>
>> --
>> Jani Nikula, Intel
--
Jani Nikula, Intel
On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula <[email protected]> wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> > On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <[email protected]> wrote:
> >>
> >> On Mon, 04 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> >> > Add a function to check if the EDID base block contains a given string.
> >> >
> >> > One of the use cases is fetching panel from a list of panel names, since
> >> > some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
> >> > instead of EDID_DETAIL_MONITOR_NAME.
> >> >
> >> > Signed-off-by: Hsin-Yi Wang <[email protected]>
> >> > ---
> >> > v2->v3: move string matching to drm_edid
> >> > ---
> >> > drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
> >> > include/drm/drm_edid.h | 1 +
> >> > 2 files changed, 50 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> > index 13454bc64ca2..fcdc2bd143dd 100644
> >> > --- a/drivers/gpu/drm/drm_edid.c
> >> > +++ b/drivers/gpu/drm/drm_edid.c
> >> > @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
> >> > }
> >> > EXPORT_SYMBOL(drm_edid_get_panel_id);
> >> >
> >> > +/**
> >> > + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
> >> > + * @base_block: EDID base block to check.
> >> > + * @str: pointer to a character array to hold the string to be checked.
> >> > + *
> >> > + * Check if the detailed timings section of a EDID base block has the given
> >> > + * string.
> >> > + *
> >> > + * Return: True if the EDID base block contains the string, false otherwise.
> >> > + */
> >> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
> >> > +{
> >> > + unsigned int i, j, k, buflen = strlen(str);
> >> > +
> >> > + for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
> >> > + struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
> >> > + unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> >> > +
> >> > + if (buflen > size || timing->pixel_clock != 0 ||
> >> > + timing->data.other_data.pad1 != 0 ||
> >> > + (timing->data.other_data.type != EDID_DETAIL_MONITOR_NAME &&
> >> > + timing->data.other_data.type != EDID_DETAIL_MONITOR_STRING))
> >> > + continue;
> >> > +
> >> > + for (j = 0; j < buflen; j++) {
> >> > + char c = timing->data.other_data.data.str.str[j];
> >> > +
> >> > + if (c != str[j] || c == '\n')
> >> > + break;
> >> > + }
> >> > +
> >> > + if (j == buflen) {
> >> > + /* Allow trailing white spaces. */
> >> > + for (k = j; k < size; k++) {
> >> > + char c = timing->data.other_data.data.str.str[k];
> >> > +
> >> > + if (c == '\n')
> >> > + return true;
> >> > + else if (c != ' ')
> >> > + break;
> >> > + }
> >> > + if (k == size)
> >> > + return true;
> >> > + }
> >> > + }
> >> > +
> >> > + return false;
> >> > +}
> >> > +
> >>
> >> So we've put a lot of effort into converting from struct edid to struct
> >> drm_edid, passing that around in drm_edid.c, with the allocation size it
> >> provides, and generally cleaning stuff up.
> >>
> >> I'm not at all happy to see *another* struct added just for the base
> >> block, and detailed timing iteration as well as monitor name parsing
> >> duplicated.
> >>
> >> With struct drm_edid you can actually return an EDID that only has the
> >> base block and size 128, even if the EDID indicates more
> >> extensions. Because the whole thing is *designed* to handle that
> >> gracefully. The allocated size matters, not what the blob originating
> >> outside of the kernel tells you.
> >>
> >> What I'm thinking is:
> >>
> >> - Add some struct drm_edid_ident or similar. Add all the information
> >> that's needed to identify a panel there. I guess initially that's
> >> panel_id and name.
> >>
> >> struct drm_edid_ident {
> >> u32 panel_id;
> >> const char *name;
> >> };
> >>
> >> - Add function:
> >>
> >> bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
> >>
> >> Check if stuff in ident matches drm_edid. You can use and extend the
> >> existing drm_edid based iteration etc. in
> >> drm_edid.c. Straightforward. The fields in ident can trivially be
> >> extended later, and the stuff can be useful for other drivers and
> >> quirks etc.
> >>
> >> - Restructure struct edp_panel_entry to contain struct
> >> drm_edid_ident. Change the iteration of edp_panels array to use
> >> drm_edid_match() on the array elements and the edid.
> >>
> >> - Add a function to read the EDID base block *but* make it return const
> >> struct drm_edid *. Add warnings in the comments that it's only for
> >> panel and for transition until it switches to reading full EDIDs.
> >>
> >> const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
> >>
> >> This is the *only* hackish part of the whole thing, and it's nicely
> >> isolated. For the most part you can use drm_edid_get_panel_id() code
> >> for this, just return the blob wrapped in a struct drm_edid envelope.
> >
> > To clarify:
> > struct drm_edid currently is only internal to drm_edid.c. So with
> > change we will have to move it to the header drm_edid.h
>
> Absolutely not, struct drm_edid must remain an opaque type. The point is
> that you ask drm_edid.c if there's a match or not, and the panel code
> does not need to care what's inside struct drm_edid.
>
> >
> >>
> >> - Remove function:
> >>
> >> u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
> >>
> >
> > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> > *);? Given that we still need to parse id from
> > drm_edid_read_base_block().
>
> No, we no longer need to parse the id outside of drm_edid.c. You'll have
> the id's in panel code in the form of struct drm_edid_ident (or
> whatever), and use the match function to see if the opaque drm_edid
> matches.
>
drm_panel prints the panel_id info on whether the panel is detected or not.
https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792
Is it okay to remove this information?
> >
> >> - Refactor edid_quirk_list to use the same id struct and match function
> >> and mechanism within drm_edid.c (can be follow-up too).
> >>
> >
> > edid_quirk currently doesn't have panel names in it, and it might be a
> > bit difficult to get all the correct names of these panels without
> > having the datasheets.
> > One way is to leave the name as null and if the name is empty and skip
> > matching the name in drm_edid_match().
>
> Exactly. NULL in drm_edid_ident would mean "don't care". I think most of
> the ones in panel code also won't use the name for matching.
>
> BR,
> Jani.
>
> >
> >> - Once you change the panel code to read the whole EDID using
> >> drm_edid_read family of functions in the future, you don't have to
> >> change *anything* about the iteration or matching or anything, because
> >> it's already passing struct drm_edid around.
> >>
> >>
> >> I hope this covers everything.
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> > /**
> >> > * drm_edid_get_base_block - Get a panel's EDID base block
> >> > * @adapter: I2C adapter to use for DDC
> >> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> >> > index 2455d6ab2221..248ddb0a6b5d 100644
> >> > --- a/include/drm/drm_edid.h
> >> > +++ b/include/drm/drm_edid.h
> >> > @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >> > struct i2c_adapter *adapter);
> >> > struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
> >> > u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
> >> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
> >> > struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
> >> > struct i2c_adapter *adapter);
> >> > struct edid *drm_edid_duplicate(const struct edid *edid);
> >>
> >> --
> >> Jani Nikula, Intel
>
> --
> Jani Nikula, Intel
Hi,
On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <[email protected]> wrote:
>
> > > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> > > *);? Given that we still need to parse id from
> > > drm_edid_read_base_block().
> >
> > No, we no longer need to parse the id outside of drm_edid.c. You'll have
> > the id's in panel code in the form of struct drm_edid_ident (or
> > whatever), and use the match function to see if the opaque drm_edid
> > matches.
> >
> drm_panel prints the panel_id info on whether the panel is detected or not.
> https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792
>
> Is it okay to remove this information?
Hmmm, I guess it also is exported via debugfs, actually. See
detected_panel_show() in panel-edp.c. We probably don't want to remove
that...
On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula <[email protected]> wrote:
>
> On Mon, 04 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> > On Mon, Mar 4, 2024 at 12:38 PM Jani Nikula <[email protected]> wrote:
> >>
> >> On Mon, 04 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> >> > Add a function to check if the EDID base block contains a given string.
> >> >
> >> > One of the use cases is fetching panel from a list of panel names, since
> >> > some panel vendors put the monitor name after EDID_DETAIL_MONITOR_STRING
> >> > instead of EDID_DETAIL_MONITOR_NAME.
> >> >
> >> > Signed-off-by: Hsin-Yi Wang <[email protected]>
> >> > ---
> >> > v2->v3: move string matching to drm_edid
> >> > ---
> >> > drivers/gpu/drm/drm_edid.c | 49 ++++++++++++++++++++++++++++++++++++++
> >> > include/drm/drm_edid.h | 1 +
> >> > 2 files changed, 50 insertions(+)
> >> >
> >> > diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> >> > index 13454bc64ca2..fcdc2bd143dd 100644
> >> > --- a/drivers/gpu/drm/drm_edid.c
> >> > +++ b/drivers/gpu/drm/drm_edid.c
> >> > @@ -2789,6 +2789,55 @@ u32 drm_edid_get_panel_id(struct edid_base_block *base_block)
> >> > }
> >> > EXPORT_SYMBOL(drm_edid_get_panel_id);
> >> >
> >> > +/**
> >> > + * drm_edid_has_monitor_string - Check if a EDID base block has certain string.
> >> > + * @base_block: EDID base block to check.
> >> > + * @str: pointer to a character array to hold the string to be checked.
> >> > + *
> >> > + * Check if the detailed timings section of a EDID base block has the given
> >> > + * string.
> >> > + *
> >> > + * Return: True if the EDID base block contains the string, false otherwise.
> >> > + */
> >> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str)
> >> > +{
> >> > + unsigned int i, j, k, buflen = strlen(str);
> >> > +
> >> > + for (i = 0; i < EDID_DETAILED_TIMINGS; i++) {
> >> > + struct detailed_timing *timing = &base_block->edid.detailed_timings[i];
> >> > + unsigned int size = ARRAY_SIZE(timing->data.other_data.data.str.str);
> >> > +
> >> > + if (buflen > size || timing->pixel_clock != 0 ||
> >> > + timing->data.other_data.pad1 != 0 ||
> >> > + (timing->data.other_data.type != EDID_DETAIL_MONITOR_NAME &&
> >> > + timing->data.other_data.type != EDID_DETAIL_MONITOR_STRING))
> >> > + continue;
> >> > +
> >> > + for (j = 0; j < buflen; j++) {
> >> > + char c = timing->data.other_data.data.str.str[j];
> >> > +
> >> > + if (c != str[j] || c == '\n')
> >> > + break;
> >> > + }
> >> > +
> >> > + if (j == buflen) {
> >> > + /* Allow trailing white spaces. */
> >> > + for (k = j; k < size; k++) {
> >> > + char c = timing->data.other_data.data.str.str[k];
> >> > +
> >> > + if (c == '\n')
> >> > + return true;
> >> > + else if (c != ' ')
> >> > + break;
> >> > + }
> >> > + if (k == size)
> >> > + return true;
> >> > + }
> >> > + }
> >> > +
> >> > + return false;
> >> > +}
> >> > +
> >>
> >> So we've put a lot of effort into converting from struct edid to struct
> >> drm_edid, passing that around in drm_edid.c, with the allocation size it
> >> provides, and generally cleaning stuff up.
> >>
> >> I'm not at all happy to see *another* struct added just for the base
> >> block, and detailed timing iteration as well as monitor name parsing
> >> duplicated.
> >>
> >> With struct drm_edid you can actually return an EDID that only has the
> >> base block and size 128, even if the EDID indicates more
> >> extensions. Because the whole thing is *designed* to handle that
> >> gracefully. The allocated size matters, not what the blob originating
> >> outside of the kernel tells you.
> >>
> >> What I'm thinking is:
> >>
> >> - Add some struct drm_edid_ident or similar. Add all the information
> >> that's needed to identify a panel there. I guess initially that's
> >> panel_id and name.
> >>
> >> struct drm_edid_ident {
> >> u32 panel_id;
> >> const char *name;
> >> };
> >>
> >> - Add function:
> >>
> >> bool drm_edid_match(const struct drm_edid *drm_edid, const struct drm_edid_ident *ident);
> >>
> >> Check if stuff in ident matches drm_edid. You can use and extend the
> >> existing drm_edid based iteration etc. in
> >> drm_edid.c. Straightforward. The fields in ident can trivially be
> >> extended later, and the stuff can be useful for other drivers and
> >> quirks etc.
> >>
> >> - Restructure struct edp_panel_entry to contain struct
> >> drm_edid_ident. Change the iteration of edp_panels array to use
> >> drm_edid_match() on the array elements and the edid.
> >>
> >> - Add a function to read the EDID base block *but* make it return const
> >> struct drm_edid *. Add warnings in the comments that it's only for
> >> panel and for transition until it switches to reading full EDIDs.
> >>
> >> const struct drm_edid *drm_edid_read_base_block(struct i2c_adapter *adapter);
> >>
> >> This is the *only* hackish part of the whole thing, and it's nicely
> >> isolated. For the most part you can use drm_edid_get_panel_id() code
> >> for this, just return the blob wrapped in a struct drm_edid envelope.
> >
> > To clarify:
> > struct drm_edid currently is only internal to drm_edid.c. So with
> > change we will have to move it to the header drm_edid.h
>
> Absolutely not, struct drm_edid must remain an opaque type. The point is
> that you ask drm_edid.c if there's a match or not, and the panel code
> does not need to care what's inside struct drm_edid.
>
Sorry I might be misunderstanding about the requests here:
If drm_edid should remain opaque, then struct drm_edid remains opaque,
drm_edid_match() should take struct edid *edid as a parameter? just as
other exposed functions in drm_edid.
If panel edp doesn't hold drm_edid returned from
drm_edid_read_base_block(), what should it use to iterate the
edp_panels array?
for (panel = edp_panels; panel->panel_id; panel++)
if(drm_edid_match(drm_edid, panel->ident))
...
> >
> >>
> >> - Remove function:
> >>
> >> u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
> >>
> >
> > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> > *);? Given that we still need to parse id from
> > drm_edid_read_base_block().
>
> No, we no longer need to parse the id outside of drm_edid.c. You'll have
> the id's in panel code in the form of struct drm_edid_ident (or
> whatever), and use the match function to see if the opaque drm_edid
> matches.
>
> >
> >> - Refactor edid_quirk_list to use the same id struct and match function
> >> and mechanism within drm_edid.c (can be follow-up too).
> >>
> >
> > edid_quirk currently doesn't have panel names in it, and it might be a
> > bit difficult to get all the correct names of these panels without
> > having the datasheets.
> > One way is to leave the name as null and if the name is empty and skip
> > matching the name in drm_edid_match().
>
> Exactly. NULL in drm_edid_ident would mean "don't care". I think most of
> the ones in panel code also won't use the name for matching.
>
> BR,
> Jani.
>
> >
> >> - Once you change the panel code to read the whole EDID using
> >> drm_edid_read family of functions in the future, you don't have to
> >> change *anything* about the iteration or matching or anything, because
> >> it's already passing struct drm_edid around.
> >>
> >>
> >> I hope this covers everything.
> >>
> >> BR,
> >> Jani.
> >>
> >>
> >> > /**
> >> > * drm_edid_get_base_block - Get a panel's EDID base block
> >> > * @adapter: I2C adapter to use for DDC
> >> > diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> >> > index 2455d6ab2221..248ddb0a6b5d 100644
> >> > --- a/include/drm/drm_edid.h
> >> > +++ b/include/drm/drm_edid.h
> >> > @@ -416,6 +416,7 @@ struct edid *drm_get_edid(struct drm_connector *connector,
> >> > struct i2c_adapter *adapter);
> >> > struct edid_base_block *drm_edid_get_base_block(struct i2c_adapter *adapter);
> >> > u32 drm_edid_get_panel_id(struct edid_base_block *base_block);
> >> > +bool drm_edid_has_monitor_string(struct edid_base_block *base_block, const char *str);
> >> > struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
> >> > struct i2c_adapter *adapter);
> >> > struct edid *drm_edid_duplicate(const struct edid *edid);
> >>
> >> --
> >> Jani Nikula, Intel
>
> --
> Jani Nikula, Intel
On Mon, 04 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> On Mon, Mar 4, 2024 at 4:09 PM Jani Nikula <[email protected]> wrote:
>>
>> On Mon, 04 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
>> > To clarify:
>> > struct drm_edid currently is only internal to drm_edid.c. So with
>> > change we will have to move it to the header drm_edid.h
>>
>> Absolutely not, struct drm_edid must remain an opaque type. The point is
>> that you ask drm_edid.c if there's a match or not, and the panel code
>> does not need to care what's inside struct drm_edid.
>>
>
> Sorry I might be misunderstanding about the requests here:
>
> If drm_edid should remain opaque, then struct drm_edid remains opaque,
> drm_edid_match() should take struct edid *edid as a parameter? just as
> other exposed functions in drm_edid.
No, it should take struct drm_edid *.
> If panel edp doesn't hold drm_edid returned from
> drm_edid_read_base_block(), what should it use to iterate the
> edp_panels array?
Panel edp can hold a *pointer* to struct drm_edid * without knowing the
full type. This is one of the points of struct drm_edid. Focus more of
the EDID parsing within drm_edid.c instead of having everyone parse it
to varying degrees of correctness.
>
> for (panel = edp_panels; panel->panel_id; panel++)
> if(drm_edid_match(drm_edid, panel->ident))
> ...
>
BR,
Jani.
--
Jani Nikula, Intel
On Mon, 04 Mar 2024, Doug Anderson <[email protected]> wrote:
> Hi,
>
> On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <[email protected]> wrote:
>>
>> > > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
>> > > *);? Given that we still need to parse id from
>> > > drm_edid_read_base_block().
>> >
>> > No, we no longer need to parse the id outside of drm_edid.c. You'll have
>> > the id's in panel code in the form of struct drm_edid_ident (or
>> > whatever), and use the match function to see if the opaque drm_edid
>> > matches.
>> >
>> drm_panel prints the panel_id info on whether the panel is detected or not.
>> https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792
>>
>> Is it okay to remove this information?
>
> Hmmm, I guess it also is exported via debugfs, actually. See
> detected_panel_show() in panel-edp.c. We probably don't want to remove
> that...
You currently print the information via panel->detected_panel, which is
a struct edp_panel_entry *. That doesn't change. It'll be slightly
restructured to contain a struct drm_edid_ident, which will not be an
opaque type.
BR,
Jani.
--
Jani Nikula, Intel
Hi,
On Tue, Mar 5, 2024 at 12:17 AM Jani Nikula <[email protected]> wrote:
>
> On Mon, 04 Mar 2024, Doug Anderson <[email protected]> wrote:
> > Hi,
> >
> > On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <[email protected]> wrote:
> >>
> >> > > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> >> > > *);? Given that we still need to parse id from
> >> > > drm_edid_read_base_block().
> >> >
> >> > No, we no longer need to parse the id outside of drm_edid.c. You'll have
> >> > the id's in panel code in the form of struct drm_edid_ident (or
> >> > whatever), and use the match function to see if the opaque drm_edid
> >> > matches.
> >> >
> >> drm_panel prints the panel_id info on whether the panel is detected or not.
> >> https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792
> >>
> >> Is it okay to remove this information?
> >
> > Hmmm, I guess it also is exported via debugfs, actually. See
> > detected_panel_show() in panel-edp.c. We probably don't want to remove
> > that...
>
> You currently print the information via panel->detected_panel, which is
> a struct edp_panel_entry *. That doesn't change. It'll be slightly
> restructured to contain a struct drm_edid_ident, which will not be an
> opaque type.
Hmm. As Hsin-Yi pointed out to me offline. Somehow we'll need to get
the actual panel ID out. Right now in panel-edp.c we have:
dev_warn(dev,
"Unknown panel %s %#06x, using conservative timings\n",
vend, product_id);
Where "vend" and "product_id" come from the panel ID of a panel that
we didn't recognize. For instance:
Unknown panel BOE 0x0731, using conservative timings
We need to still be able to print this message for unrecognized
panels. Then when we see field reports including this message we know
that somehow we ended up shipping an unrecognized panel.
Any suggestions on what abstraction you'd like to see to enable us to
print that message if everything is opaque?
-Doug
On Tue, Mar 5, 2024 at 11:25 AM Doug Anderson <[email protected]> wrote:
>
> Hi,
>
> On Tue, Mar 5, 2024 at 12:17 AM Jani Nikula <[email protected]> wrote:
> >
> > On Mon, 04 Mar 2024, Doug Anderson <[email protected]> wrote:
> > > Hi,
> > >
> > > On Mon, Mar 4, 2024 at 4:19 PM Hsin-Yi Wang <[email protected]> wrote:
> > >>
> > >> > > Probably change to u32 drm_edid_get_panel_id(const struct drm_edid
> > >> > > *);? Given that we still need to parse id from
> > >> > > drm_edid_read_base_block().
> > >> >
> > >> > No, we no longer need to parse the id outside of drm_edid.c. You'll have
> > >> > the id's in panel code in the form of struct drm_edid_ident (or
> > >> > whatever), and use the match function to see if the opaque drm_edid
> > >> > matches.
> > >> >
> > >> drm_panel prints the panel_id info on whether the panel is detected or not.
> > >> https://elixir.bootlin.com/linux/v6.8-rc7/source/drivers/gpu/drm/panel/panel-edp.c#L792
> > >>
> > >> Is it okay to remove this information?
> > >
> > > Hmmm, I guess it also is exported via debugfs, actually. See
> > > detected_panel_show() in panel-edp.c. We probably don't want to remove
> > > that...
> >
> > You currently print the information via panel->detected_panel, which is
> > a struct edp_panel_entry *. That doesn't change. It'll be slightly
> > restructured to contain a struct drm_edid_ident, which will not be an
> > opaque type.
>
> Hmm. As Hsin-Yi pointed out to me offline. Somehow we'll need to get
> the actual panel ID out. Right now in panel-edp.c we have:
>
> dev_warn(dev,
> "Unknown panel %s %#06x, using conservative timings\n",
> vend, product_id);
>
> Where "vend" and "product_id" come from the panel ID of a panel that
> we didn't recognize. For instance:
>
> Unknown panel BOE 0x0731, using conservative timings
>
> We need to still be able to print this message for unrecognized
> panels. Then when we see field reports including this message we know
> that somehow we ended up shipping an unrecognized panel.
>
> Any suggestions on what abstraction you'd like to see to enable us to
> print that message if everything is opaque?
Sent v4 here: https://lore.kernel.org/lkml/[email protected]/
Besides that it still keeps drm_edid_get_panel_id() to be used on the
kernel warning when no panel is matched, other parts I think are
following the comments.
Thanks.
>
> -Doug
On Tue, 05 Mar 2024, Hsin-Yi Wang <[email protected]> wrote:
> On Tue, Mar 5, 2024 at 11:25 AM Doug Anderson <[email protected]> wrote:
>> Hmm. As Hsin-Yi pointed out to me offline. Somehow we'll need to get
>> the actual panel ID out. Right now in panel-edp.c we have:
>>
>> dev_warn(dev,
>> "Unknown panel %s %#06x, using conservative timings\n",
>> vend, product_id);
>>
>> Where "vend" and "product_id" come from the panel ID of a panel that
>> we didn't recognize. For instance:
>>
>> Unknown panel BOE 0x0731, using conservative timings
>>
>> We need to still be able to print this message for unrecognized
>> panels. Then when we see field reports including this message we know
>> that somehow we ended up shipping an unrecognized panel.
>>
>> Any suggestions on what abstraction you'd like to see to enable us to
>> print that message if everything is opaque?
>
> Sent v4 here: https://lore.kernel.org/lkml/[email protected]/
>
> Besides that it still keeps drm_edid_get_panel_id() to be used on the
> kernel warning when no panel is matched, other parts I think are
> following the comments.
Yeah we can keep that for now.
BR,
Jani.
--
Jani Nikula, Intel