diff mbox series

[v2,1/3] drm_edid: Support getting EDID through ddc without connector

Message ID 20240228011133.1238439-2-hsinyi@chromium.org (mailing list archive)
State New, archived
Headers show
Series Match panel with id and name | expand

Commit Message

Hsin-Yi Wang Feb. 28, 2024, 1:05 a.m. UTC 
Some panels are interested in the EDID during early probe when connector
is still unknown.

Add a function drm_get_edid_no_connector() to get edid without connector.
No functional change for existing usage.

Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
---
v1->v2:
add a function to return the entire edid without updating connector.
---
 drivers/gpu/drm/drm_edid.c | 45 ++++++++++++++++++++++++++++----------
 include/drm/drm_edid.h     |  1 +
 2 files changed, 34 insertions(+), 12 deletions(-)

Comments

Hsin-Yi Wang Feb. 28, 2024, 1:28 a.m. UTC | #1 
On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Some panels are interested in the EDID during early probe when connector
> is still unknown.
>
> Add a function drm_get_edid_no_connector() to get edid without connector.
> No functional change for existing usage.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> v1->v2:
> add a function to return the entire edid without updating connector.
> ---
>  drivers/gpu/drm/drm_edid.c | 45 ++++++++++++++++++++++++++++----------
>  include/drm/drm_edid.h     |  1 +
>  2 files changed, 34 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 1ad94473e400..15b97c8ed993 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2364,7 +2364,7 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>         struct edid *edid, *new;
>         size_t alloc_size = EDID_LENGTH;
>
> -       override = drm_edid_override_get(connector);
> +       override = connector ? drm_edid_override_get(connector) : false;

typo: should be NULL here. I'll update in the next version with other comments.

>         if (override) {
>                 alloc_size = override->size;
>                 edid = kmemdup(override->edid, alloc_size, GFP_KERNEL);
> @@ -2385,18 +2385,20 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>         if (status == EDID_BLOCK_READ_FAIL)
>                 goto fail;
>
> -       /* FIXME: Clarify what a corrupt EDID actually means. */
> -       if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> -               connector->edid_corrupt = false;
> -       else
> -               connector->edid_corrupt = true;
> +       if (connector) {
> +               /* FIXME: Clarify what a corrupt EDID actually means. */
> +               if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> +                       connector->edid_corrupt = false;
> +               else
> +                       connector->edid_corrupt = true;
>
> -       if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> -               if (status == EDID_BLOCK_ZERO)
> -                       connector->null_edid_counter++;
> +               if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> +                       if (status == EDID_BLOCK_ZERO)
> +                               connector->null_edid_counter++;
>
> -               connector_bad_edid(connector, edid, 1);
> -               goto fail;
> +                       connector_bad_edid(connector, edid, 1);
> +                       goto fail;
> +               }
>         }
>
>         if (!edid_extension_block_count(edid))
> @@ -2444,7 +2446,8 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>         }
>
>         if (invalid_blocks) {
> -               connector_bad_edid(connector, edid, num_blocks);
> +               if (connector)
> +                       connector_bad_edid(connector, edid, num_blocks);
>
>                 edid = edid_filter_invalid_blocks(edid, &alloc_size);
>         }
> @@ -2637,6 +2640,24 @@ struct edid *drm_get_edid(struct drm_connector *connector,
>  }
>  EXPORT_SYMBOL(drm_get_edid);
>
> +/**
> + * drm_get_edid_no_connector - get EDID data without updating connector status
> + * @adapter: I2C adapter to use for DDC
> + *
> + * Similar to drm_edid_read_ddc(), but not checking any connector status. Use
> + * this function to get EDID when connector is still unknown.
> + *
> + * Return: Pointer to valid EDID or NULL if we couldn't find any.
> + */
> +struct edid *drm_get_edid_no_connector(struct i2c_adapter *adapter)
> +{
> +       if (!drm_probe_ddc(adapter))
> +               return NULL;
> +
> +       return _drm_do_get_edid(NULL, drm_do_probe_ddc_edid, adapter, NULL);
> +}
> +EXPORT_SYMBOL(drm_get_edid_no_connector);
> +
>  /**
>   * drm_edid_read_custom - Read EDID data using given EDID block read function
>   * @connector: Connector to use
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 70ae6c290bdc..80c9e32ff80e 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -565,6 +565,7 @@ 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);
> +struct edid *drm_get_edid_no_connector(struct i2c_adapter *adapter);
>  u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
>  struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
>                                      struct i2c_adapter *adapter);
> --
> 2.44.0.rc1.240.g4c46232300-goog
>
Doug Anderson Feb. 29, 2024, 12:21 a.m. UTC | #2 
Hi,

On Tue, Feb 27, 2024 at 5:11 PM Hsin-Yi Wang <hsinyi@chromium.org> wrote:
>
> Some panels are interested in the EDID during early probe when connector
> is still unknown.
>
> Add a function drm_get_edid_no_connector() to get edid without connector.
> No functional change for existing usage.
>
> Signed-off-by: Hsin-Yi Wang <hsinyi@chromium.org>
> ---
> v1->v2:
> add a function to return the entire edid without updating connector.
> ---
>  drivers/gpu/drm/drm_edid.c | 45 ++++++++++++++++++++++++++++----------
>  include/drm/drm_edid.h     |  1 +
>  2 files changed, 34 insertions(+), 12 deletions(-)

I'll respond in the discussion in v1 too, but overall I'm not a fan of
reading the whole EDID twice at bootup. Personally I'd love to see us
to back to just reading the base block like in v1, but I guess we can
see what Jani and others say.


> @@ -2385,18 +2385,20 @@ static struct edid *_drm_do_get_edid(struct drm_connector *connector,
>         if (status == EDID_BLOCK_READ_FAIL)
>                 goto fail;
>
> -       /* FIXME: Clarify what a corrupt EDID actually means. */
> -       if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> -               connector->edid_corrupt = false;
> -       else
> -               connector->edid_corrupt = true;
> +       if (connector) {
> +               /* FIXME: Clarify what a corrupt EDID actually means. */
> +               if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
> +                       connector->edid_corrupt = false;
> +               else
> +                       connector->edid_corrupt = true;
>
> -       if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> -               if (status == EDID_BLOCK_ZERO)
> -                       connector->null_edid_counter++;
> +               if (!edid_block_status_valid(status, edid_block_tag(edid))) {
> +                       if (status == EDID_BLOCK_ZERO)
> +                               connector->null_edid_counter++;
>
> -               connector_bad_edid(connector, edid, 1);
> -               goto fail;
> +                       connector_bad_edid(connector, edid, 1);
> +                       goto fail;

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

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

Patch

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 1ad94473e400..15b97c8ed993 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2364,7 +2364,7 @@  static struct edid *_drm_do_get_edid(struct drm_connector *connector,
 	struct edid *edid, *new;
 	size_t alloc_size = EDID_LENGTH;
 
-	override = drm_edid_override_get(connector);
+	override = connector ? drm_edid_override_get(connector) : false;
 	if (override) {
 		alloc_size = override->size;
 		edid = kmemdup(override->edid, alloc_size, GFP_KERNEL);
@@ -2385,18 +2385,20 @@  static struct edid *_drm_do_get_edid(struct drm_connector *connector,
 	if (status == EDID_BLOCK_READ_FAIL)
 		goto fail;
 
-	/* FIXME: Clarify what a corrupt EDID actually means. */
-	if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
-		connector->edid_corrupt = false;
-	else
-		connector->edid_corrupt = true;
+	if (connector) {
+		/* FIXME: Clarify what a corrupt EDID actually means. */
+		if (status == EDID_BLOCK_OK || status == EDID_BLOCK_VERSION)
+			connector->edid_corrupt = false;
+		else
+			connector->edid_corrupt = true;
 
-	if (!edid_block_status_valid(status, edid_block_tag(edid))) {
-		if (status == EDID_BLOCK_ZERO)
-			connector->null_edid_counter++;
+		if (!edid_block_status_valid(status, edid_block_tag(edid))) {
+			if (status == EDID_BLOCK_ZERO)
+				connector->null_edid_counter++;
 
-		connector_bad_edid(connector, edid, 1);
-		goto fail;
+			connector_bad_edid(connector, edid, 1);
+			goto fail;
+		}
 	}
 
 	if (!edid_extension_block_count(edid))
@@ -2444,7 +2446,8 @@  static struct edid *_drm_do_get_edid(struct drm_connector *connector,
 	}
 
 	if (invalid_blocks) {
-		connector_bad_edid(connector, edid, num_blocks);
+		if (connector)
+			connector_bad_edid(connector, edid, num_blocks);
 
 		edid = edid_filter_invalid_blocks(edid, &alloc_size);
 	}
@@ -2637,6 +2640,24 @@  struct edid *drm_get_edid(struct drm_connector *connector,
 }
 EXPORT_SYMBOL(drm_get_edid);
 
+/**
+ * drm_get_edid_no_connector - get EDID data without updating connector status
+ * @adapter: I2C adapter to use for DDC
+ *
+ * Similar to drm_edid_read_ddc(), but not checking any connector status. Use
+ * this function to get EDID when connector is still unknown.
+ *
+ * Return: Pointer to valid EDID or NULL if we couldn't find any.
+ */
+struct edid *drm_get_edid_no_connector(struct i2c_adapter *adapter)
+{
+	if (!drm_probe_ddc(adapter))
+		return NULL;
+
+	return _drm_do_get_edid(NULL, drm_do_probe_ddc_edid, adapter, NULL);
+}
+EXPORT_SYMBOL(drm_get_edid_no_connector);
+
 /**
  * drm_edid_read_custom - Read EDID data using given EDID block read function
  * @connector: Connector to use
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 70ae6c290bdc..80c9e32ff80e 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -565,6 +565,7 @@  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);
+struct edid *drm_get_edid_no_connector(struct i2c_adapter *adapter);
 u32 drm_edid_get_panel_id(struct i2c_adapter *adapter);
 struct edid *drm_get_edid_switcheroo(struct drm_connector *connector,
 				     struct i2c_adapter *adapter);