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.