| Message ID | 20211016184226.3862-14-cssk@net-c.es (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi | expand |
On Sat, Oct 16, 2021 at 08:42:26PM +0200, Claudio Suarez wrote: > Once EDID is parsed, the monitor HDMI support information is available > through drm_display_info.is_hdmi. Retriving the same information with > drm_detect_hdmi_monitor() is less efficient. Change to > drm_display_info.is_hdmi where possible. We still need proof in the commit message that display_info is actually populated by the time this gets called. > > This is a TODO task in Documentation/gpu/todo.rst > > Signed-off-by: Claudio Suarez <cssk@net-c.es> > --- > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- > drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > index b04685bb6439..008e5b0ba408 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector) > to_intel_connector(connector)->detect_edid = edid; > if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { > intel_hdmi->has_audio = drm_detect_monitor_audio(edid); > - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid); > + intel_hdmi->has_hdmi_sink = connector->display_info.is_hdmi; > > connected = true; > } > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c > index 6cb27599ea03..b4065e4df644 100644 > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c > @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector) > if (edid->input & DRM_EDID_INPUT_DIGITAL) { > status = connector_status_connected; > if (intel_sdvo_connector->is_hdmi) { > - intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid); > intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid); > + intel_sdvo->has_hdmi_monitor = > + connector->display_info.is_hdmi; > } > } else > status = connector_status_disconnected; > -- > 2.33.0 >
drm_get_edid() internally calls to drm_connector_update_edid_property()
and then drm_add_display_info(), which parses the EDID.
This happens in the function intel_hdmi_set_edid() and
intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()).
Once EDID is parsed, the monitor HDMI support information is available
through drm_display_info.is_hdmi. Retriving the same information with
drm_detect_hdmi_monitor() is less efficient. Change to
drm_display_info.is_hdmi
This is a TODO task in Documentation/gpu/todo.rst
Signed-off-by: Claudio Suarez <cssk@net-c.es>
---
drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index b04685bb6439..008e5b0ba408 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
to_intel_connector(connector)->detect_edid = edid;
if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
- intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
+ intel_hdmi->has_hdmi_sink = connector->display_info.is_hdmi;
connected = true;
}
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
index 6cb27599ea03..b4065e4df644 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector)
if (edid->input & DRM_EDID_INPUT_DIGITAL) {
status = connector_status_connected;
if (intel_sdvo_connector->is_hdmi) {
- intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid);
intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid);
+ intel_sdvo->has_hdmi_monitor =
+ connector->display_info.is_hdmi;
}
} else
status = connector_status_disconnected;
On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote: > drm_get_edid() internally calls to drm_connector_update_edid_property() > and then drm_add_display_info(), which parses the EDID. > This happens in the function intel_hdmi_set_edid() and > intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()). > > Once EDID is parsed, the monitor HDMI support information is available > through drm_display_info.is_hdmi. Retriving the same information with > drm_detect_hdmi_monitor() is less efficient. Change to > drm_display_info.is_hdmi I meant we need to examine all call chains that can lead to .detect() to make sure all of them do in fact update the display_info beforehand. > > This is a TODO task in Documentation/gpu/todo.rst > > Signed-off-by: Claudio Suarez <cssk@net-c.es> > --- > drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- > drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c > index b04685bb6439..008e5b0ba408 100644 > --- a/drivers/gpu/drm/i915/display/intel_hdmi.c > +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c > @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector) > to_intel_connector(connector)->detect_edid = edid; > if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { > intel_hdmi->has_audio = drm_detect_monitor_audio(edid); > - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid); > + intel_hdmi->has_hdmi_sink = connector->display_info.is_hdmi; > > connected = true; > } > diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c > index 6cb27599ea03..b4065e4df644 100644 > --- a/drivers/gpu/drm/i915/display/intel_sdvo.c > +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c > @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector) > if (edid->input & DRM_EDID_INPUT_DIGITAL) { > status = connector_status_connected; > if (intel_sdvo_connector->is_hdmi) { > - intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid); > intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid); > + intel_sdvo->has_hdmi_monitor = > + connector->display_info.is_hdmi; > } > } else > status = connector_status_disconnected; > -- > 2.33.0 > >
On Thu, Oct 21, 2021 at 04:49:59PM +0300, Ville Syrjälä wrote: > On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote: > > drm_get_edid() internally calls to drm_connector_update_edid_property() > > and then drm_add_display_info(), which parses the EDID. > > This happens in the function intel_hdmi_set_edid() and > > intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()). > > > > Once EDID is parsed, the monitor HDMI support information is available > > through drm_display_info.is_hdmi. Retriving the same information with > > drm_detect_hdmi_monitor() is less efficient. Change to > > drm_display_info.is_hdmi > > I meant we need to examine all call chains that can lead to > .detect() to make sure all of them do in fact update the > display_info beforehand. Well, I studied it carefully and, yes, all call chains that can lead to drm_display_info.is_hdmi / drm_detect_hdmi_monitor() update display_info beforehand. In the case that this doesn't happen, the code is unchanged. Do you want I explain the changes in the code here again ? Or do you want to me change the commit message to be more clear ? In the first case, I can write here a detailed explanation. In the second case I can make a longer commit message. Or both? Best Regards, Claudio Suarez.
On Fri, Oct 22, 2021 at 12:25:33PM +0200, Claudio Suarez wrote: > On Thu, Oct 21, 2021 at 04:49:59PM +0300, Ville Syrjälä wrote: > > On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote: > > > drm_get_edid() internally calls to drm_connector_update_edid_property() > > > and then drm_add_display_info(), which parses the EDID. > > > This happens in the function intel_hdmi_set_edid() and > > > intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()). > > > > > > Once EDID is parsed, the monitor HDMI support information is available > > > through drm_display_info.is_hdmi. Retriving the same information with > > > drm_detect_hdmi_monitor() is less efficient. Change to > > > drm_display_info.is_hdmi > > > > I meant we need to examine all call chains that can lead to > > .detect() to make sure all of them do in fact update the > > display_info beforehand. > > Well, I studied it carefully and, yes, all call chains that can lead to > drm_display_info.is_hdmi / drm_detect_hdmi_monitor() update display_info > beforehand. In the case that this doesn't happen, the code is unchanged. > > Do you want I explain the changes in the code here again ? Or do you want > to me change the commit message to be more clear ? In the first case, I can > write here a detailed explanation. In the second case I can make a longer commit > message. > > Or both? I want all those call chains explained in the commit message, otherwise I have no easy way to confirm whether the change is correct or not.
On Fri, Oct 22, 2021 at 03:01:52PM +0300, Ville Syrjälä wrote: > On Fri, Oct 22, 2021 at 12:25:33PM +0200, Claudio Suarez wrote: > > On Thu, Oct 21, 2021 at 04:49:59PM +0300, Ville Syrjälä wrote: > > > On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote: > > > > drm_get_edid() internally calls to drm_connector_update_edid_property() > > > > and then drm_add_display_info(), which parses the EDID. > > > > This happens in the function intel_hdmi_set_edid() and > > > > intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()). > > > > > > > > Once EDID is parsed, the monitor HDMI support information is available > > > > through drm_display_info.is_hdmi. Retriving the same information with > > > > drm_detect_hdmi_monitor() is less efficient. Change to > > > > drm_display_info.is_hdmi > > > > > > I meant we need to examine all call chains that can lead to > > > .detect() to make sure all of them do in fact update the > > > display_info beforehand. > > > > Well, I studied it carefully and, yes, all call chains that can lead to > > drm_display_info.is_hdmi / drm_detect_hdmi_monitor() update display_info > > beforehand. In the case that this doesn't happen, the code is unchanged. > > > > Do you want I explain the changes in the code here again ? Or do you want > > to me change the commit message to be more clear ? In the first case, I can > > write here a detailed explanation. In the second case I can make a longer commit > > message. > > > > Or both? > > I want all those call chains explained in the commit message, > otherwise I have no easy way to confirm whether the change > is correct or not. Hmm. OK, so I had a bit of a dig around and seems that what we do now .detect()->drm_get_edid()->drm_connector_update_edid_property()->drm_add_display_info() Now the question is when did that start happening? Looks like it was commit 4b4df570b41d ("drm: Update edid-derived drm_display_info fields at edid property set [v2]") that started to call drm_add_display_info() from drm_connector_update_edid_property(), and then commit 5186421cbfe2 ("drm: Introduce epoch counter to drm_connector") started to call drm_connector_update_edid_property() from drm_get_edid(). Before both of those commits were in place display_info would still contain some stale garbage during .detect(). That is the story I think we want in these commit messages since it a) explains why the old code was directly parsing the edid instead b) why it's now safe to change this PS. connector->force handling in drm_get_edid() looks a bit busted since it doesn't call drm_connector_update_edid_property() at all in some cases. I think there might be some path that leads there anywya if/when we change connector->force, but we should fix drm_get_edid() to do the right thing regarless.
On Fri, Oct 22, 2021 at 03:22:57PM +0300, Ville Syrjälä wrote: > On Fri, Oct 22, 2021 at 03:01:52PM +0300, Ville Syrjälä wrote: > > On Fri, Oct 22, 2021 at 12:25:33PM +0200, Claudio Suarez wrote: > > > On Thu, Oct 21, 2021 at 04:49:59PM +0300, Ville Syrjälä wrote: > > > > On Wed, Oct 20, 2021 at 12:51:21AM +0200, Claudio Suarez wrote: > > > > > drm_get_edid() internally calls to drm_connector_update_edid_property() > > > > > and then drm_add_display_info(), which parses the EDID. > > > > > This happens in the function intel_hdmi_set_edid() and > > > > > intel_sdvo_tmds_sink_detect() (via intel_sdvo_get_edid()). > > > > > > > > > > Once EDID is parsed, the monitor HDMI support information is available > > > > > through drm_display_info.is_hdmi. Retriving the same information with > > > > > drm_detect_hdmi_monitor() is less efficient. Change to > > > > > drm_display_info.is_hdmi > > > > > > > > I meant we need to examine all call chains that can lead to > > > > .detect() to make sure all of them do in fact update the > > > > display_info beforehand. > > > > > > Well, I studied it carefully and, yes, all call chains that can lead to > > > drm_display_info.is_hdmi / drm_detect_hdmi_monitor() update display_info > > > beforehand. In the case that this doesn't happen, the code is unchanged. > > > > > > Do you want I explain the changes in the code here again ? Or do you want > > > to me change the commit message to be more clear ? In the first case, I can > > > write here a detailed explanation. In the second case I can make a longer commit > > > message. > > > > > > Or both? > > > > I want all those call chains explained in the commit message, > > otherwise I have no easy way to confirm whether the change > > is correct or not. > > Hmm. OK, so I had a bit of a dig around and seems that what we do now > .detect()->drm_get_edid()->drm_connector_update_edid_property()->drm_add_display_info() Yes. I said before that I felt something was wrong when I read the documentation and then the code. To be more explicit now, I expected that drm_connector_update_edid_property() will be done in the fill_modes/get_modes phase instead of when reading the edid. The documentation suggests that but the code reads the edid in the detect phase. Now, since drm_connector_update_edid_property() is called in the detect phase, it is not necessary to keep the edid data in the private connector struct. It is in struct drm_connector from the beginning. But this is topic for another patch. > Now the question is when did that start happening? Looks like it was > commit 4b4df570b41d ("drm: Update edid-derived drm_display_info fields > at edid property set [v2]") that started to call drm_add_display_info() > from drm_connector_update_edid_property(), and then commit 5186421cbfe2 > ("drm: Introduce epoch counter to drm_connector") started to call > drm_connector_update_edid_property() from drm_get_edid(). Before both > of those commits were in place display_info would still contain > some stale garbage during .detect(). > > That is the story I think we want in these commit messages since it > a) explains why the old code was directly parsing the edid instead > b) why it's now safe to change this ------------------commit-message?-------------------- drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi Commit a92d083d08b0 created the new flag is_hdmi in drm_display_info which is set when sink compliant with CEA-861 (EDID) shall be treated as an HDMI sink. From that day, this value can be used in some cases instead of calling drm_detect_hdmi_monitor() and a second parse is avoided because drm_detect_hdmi_monitor() parses. A TODO task was registered in Documentation/gpu/todo.rst to perform that task in the future. The flag drm_display_info.is_hdmi is set in the function drm_add_display_info(), which is called from drm_connector_update_edid_property(). Since commit 5186421cbfe2, drm_get_edid() calls drm_connector_update_edid_property() when reading the edid data from an i2c adapter. Therefore, in these cases drm_display_info.is_hdmi is updated to its correct value when returning from drm_get_edid(). Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi in the cases when drm_detect_hdmi_monitor() is called after a read from an i2c adapter using drm_get_edid() in the i915 driver. ----------------------------------------------- > > PS. connector->force handling in drm_get_edid() looks a bit busted > since it doesn't call drm_connector_update_edid_property() at all > in some cases. I think there might be some path that leads there > anywya if/when we change connector->force, but we should fix > drm_get_edid() to do the right thing regarless. In those cases, the edid isn't read and NULL is returned by drm_get_edid(). No problem because display_info.is_hdmi is inside an if (edid != NULL). BTW, struct intel_connector is allocated with kzalloc, so the initial value of is_hdmi is zero. The connector isn't HDMI by default. BR. Claudio Suarez.
On Mon, Oct 25, 2021 at 12:17:37AM +0200, Claudio Suarez wrote: [...] No new comments about this, I suppose everything is fine. I'm going to send the patch with this changes. Thanks to all and special thanks to you, Ville. Hope this helps the kernel. Don't hesitate to ask new changes if necessary. Best regards Claudio Suarez
Commit a92d083d08b0 created the new flag is_hdmi in drm_display_info
which is set when sink compliant with CEA-861 (EDID) will be treated
as an HDMI sink.
From that day, this value can be used in some cases instead of
calling drm_detect_hdmi_monitor() and a second parse is avoided
because drm_detect_hdmi_monitor() parses. A TODO task was
registered in Documentation/gpu/todo.rst to perform that task in
the future.
The flag drm_display_info.is_hdmi is set in the function
drm_add_display_info(), which is called from
drm_connector_update_edid_property(). Since commit 5186421cbfe2,
drm_get_edid() calls drm_connector_update_edid_property() when
reading the edid data from an i2c adapter. Therefore, in these
cases drm_display_info.is_hdmi is updated to its correct
value when returning from drm_get_edid().
Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
in the cases when drm_detect_hdmi_monitor() is called after a
read from an i2c adapter using drm_get_edid() in the i915 driver.
Signed-off-by: Claudio Suarez <cssk@net-c.es>
---
drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +-
drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++-
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
index b04685bb6439..008e5b0ba408 100644
--- a/drivers/gpu/drm/i915/display/intel_hdmi.c
+++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
@@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector)
to_intel_connector(connector)->detect_edid = edid;
if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) {
intel_hdmi->has_audio = drm_detect_monitor_audio(edid);
- intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid);
+ intel_hdmi->has_hdmi_sink = connector->display_info.is_hdmi;
connected = true;
}
diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c
index 6cb27599ea03..b4065e4df644 100644
--- a/drivers/gpu/drm/i915/display/intel_sdvo.c
+++ b/drivers/gpu/drm/i915/display/intel_sdvo.c
@@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector)
if (edid->input & DRM_EDID_INPUT_DIGITAL) {
status = connector_status_connected;
if (intel_sdvo_connector->is_hdmi) {
- intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid);
intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid);
+ intel_sdvo->has_hdmi_monitor =
+ connector->display_info.is_hdmi;
}
} else
status = connector_status_disconnected;
diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c index b04685bb6439..008e5b0ba408 100644 --- a/drivers/gpu/drm/i915/display/intel_hdmi.c +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c @@ -2355,7 +2355,7 @@ intel_hdmi_set_edid(struct drm_connector *connector) to_intel_connector(connector)->detect_edid = edid; if (edid && edid->input & DRM_EDID_INPUT_DIGITAL) { intel_hdmi->has_audio = drm_detect_monitor_audio(edid); - intel_hdmi->has_hdmi_sink = drm_detect_hdmi_monitor(edid); + intel_hdmi->has_hdmi_sink = connector->display_info.is_hdmi; connected = true; } diff --git a/drivers/gpu/drm/i915/display/intel_sdvo.c b/drivers/gpu/drm/i915/display/intel_sdvo.c index 6cb27599ea03..b4065e4df644 100644 --- a/drivers/gpu/drm/i915/display/intel_sdvo.c +++ b/drivers/gpu/drm/i915/display/intel_sdvo.c @@ -2060,8 +2060,9 @@ intel_sdvo_tmds_sink_detect(struct drm_connector *connector) if (edid->input & DRM_EDID_INPUT_DIGITAL) { status = connector_status_connected; if (intel_sdvo_connector->is_hdmi) { - intel_sdvo->has_hdmi_monitor = drm_detect_hdmi_monitor(edid); intel_sdvo->has_hdmi_audio = drm_detect_monitor_audio(edid); + intel_sdvo->has_hdmi_monitor = + connector->display_info.is_hdmi; } } else status = connector_status_disconnected;
Once EDID is parsed, the monitor HDMI support information is available through drm_display_info.is_hdmi. Retriving the same information with drm_detect_hdmi_monitor() is less efficient. Change to drm_display_info.is_hdmi where possible. This is a TODO task in Documentation/gpu/todo.rst Signed-off-by: Claudio Suarez <cssk@net-c.es> --- drivers/gpu/drm/i915/display/intel_hdmi.c | 2 +- drivers/gpu/drm/i915/display/intel_sdvo.c | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-)