Nouveau - Nov 2021 - [PATCH v4 13/13] drm/i915: replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi

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.

-- 
Ville Syrj?l?
Intel

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 at 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