| Message ID | 20220406165514.6106-2-jose.exposito89@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: hdmi: Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi | expand |
Hi Jose, On Wed, Apr 06, 2022 at 06:55:14PM +0200, José Expósito wrote: > Once EDID is parsed, the monitor HDMI support information is cached in > drm_display_info.is_hdmi by drm_parse_hdmi_vsdb_video(). > > This driver calls drm_detect_hdmi_monitor() to receive the same > information and stores its own cached value, which is less efficient. > > Avoid calling drm_detect_hdmi_monitor() and use drm_display_info.is_hdmi > instead. > > drm_detect_hdmi_monitor() is called in vc4_hdmi_connector_detect() and > vc4_hdmi_connector_get_modes(). In both cases it is safe to rely on > drm_display_info.is_hdmi as shown by ftrace: How do you use ftrace to generate that? > vc4_hdmi_connector_detect: > > vc4_hdmi_connector_detect() { > drm_get_edid() { > drm_connector_update_edid_property() { > drm_add_display_info() { > drm_reset_display_info(); > drm_for_each_detailed_block.part.0(); > drm_parse_cea_ext() { > drm_find_cea_extension(); > cea_db_offsets.part.0(); > cea_db_is_hdmi_vsdb.part.0(); > drm_parse_hdmi_vsdb_video(); > /* drm_display_info.is_hdmi is cached here */ > } > } > } > } > /* drm_display_info.is_hdmi is used here */ > } > > vc4_hdmi_connector_get_modes: > > vc4_hdmi_connector_get_modes() { > drm_get_edid() { > drm_connector_update_edid_property() { > drm_add_display_info() { > drm_reset_display_info(); > drm_for_each_detailed_block.part.0(); > drm_parse_cea_ext() { > drm_find_cea_extension(); > cea_db_offsets.part.0(); > cea_db_is_hdmi_vsdb.part.0(); > drm_parse_hdmi_vsdb_video(); > /* drm_display_info.is_hdmi is cached here */ > } > } > } > } > /* drm_display_info.is_hdmi is used here */ > drm_connector_update_edid_property(); > } > > Signed-off-by: José Expósito <jose.exposito89@gmail.com> I think what you're hinting at in the cover letter would be best though: we should just remove that caching entirely and use is_hdmi everywhere Maxime
Hi Maxime, Thanks for your comments. On Fri, Apr 08, 2022 at 09:41:10AM +0200, Maxime Ripard wrote: > Hi Jose, > > On Wed, Apr 06, 2022 at 06:55:14PM +0200, José Expósito wrote: > > Once EDID is parsed, the monitor HDMI support information is cached in > > drm_display_info.is_hdmi by drm_parse_hdmi_vsdb_video(). > > > > This driver calls drm_detect_hdmi_monitor() to receive the same > > information and stores its own cached value, which is less efficient. > > > > Avoid calling drm_detect_hdmi_monitor() and use drm_display_info.is_hdmi > > instead. > > > > drm_detect_hdmi_monitor() is called in vc4_hdmi_connector_detect() and > > vc4_hdmi_connector_get_modes(). In both cases it is safe to rely on > > drm_display_info.is_hdmi as shown by ftrace: > > How do you use ftrace to generate that? I had to add noinline to a couple of relevant functions and run the usual: $ sudo trace-cmd record -p function_graph -l "vc4_hdmi_*" [...] I'll add the command to v2. > > vc4_hdmi_connector_detect: > > > > vc4_hdmi_connector_detect() { > > drm_get_edid() { > > drm_connector_update_edid_property() { > > drm_add_display_info() { > > drm_reset_display_info(); > > drm_for_each_detailed_block.part.0(); > > drm_parse_cea_ext() { > > drm_find_cea_extension(); > > cea_db_offsets.part.0(); > > cea_db_is_hdmi_vsdb.part.0(); > > drm_parse_hdmi_vsdb_video(); > > /* drm_display_info.is_hdmi is cached here */ > > } > > } > > } > > } > > /* drm_display_info.is_hdmi is used here */ > > } > > > > vc4_hdmi_connector_get_modes: > > > > vc4_hdmi_connector_get_modes() { > > drm_get_edid() { > > drm_connector_update_edid_property() { > > drm_add_display_info() { > > drm_reset_display_info(); > > drm_for_each_detailed_block.part.0(); > > drm_parse_cea_ext() { > > drm_find_cea_extension(); > > cea_db_offsets.part.0(); > > cea_db_is_hdmi_vsdb.part.0(); > > drm_parse_hdmi_vsdb_video(); > > /* drm_display_info.is_hdmi is cached here */ > > } > > } > > } > > } > > /* drm_display_info.is_hdmi is used here */ > > drm_connector_update_edid_property(); > > } > > > > Signed-off-by: José Expósito <jose.exposito89@gmail.com> > > I think what you're hinting at in the cover letter would be best though: > we should just remove that caching entirely and use is_hdmi everywhere Cool, I'll work on a follow up patch to remove vc4_encoder.hdmi_monitor and add it to v2. Thanks, Jose
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 6c58b0fd13fb..ecdb1ffc2023 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -216,7 +216,7 @@ vc4_hdmi_connector_detect(struct drm_connector *connector, bool force) if (edid) { cec_s_phys_addr_from_edid(vc4_hdmi->cec_adap, edid); - vc4_hdmi->encoder.hdmi_monitor = drm_detect_hdmi_monitor(edid); + vc4_hdmi->encoder.hdmi_monitor = connector->display_info.is_hdmi; kfree(edid); } } @@ -255,7 +255,7 @@ static int vc4_hdmi_connector_get_modes(struct drm_connector *connector) goto out; } - vc4_encoder->hdmi_monitor = drm_detect_hdmi_monitor(edid); + vc4_encoder->hdmi_monitor = connector->display_info.is_hdmi; drm_connector_update_edid_property(connector, edid); ret = drm_add_edid_modes(connector, edid);
Once EDID is parsed, the monitor HDMI support information is cached in drm_display_info.is_hdmi by drm_parse_hdmi_vsdb_video(). This driver calls drm_detect_hdmi_monitor() to receive the same information and stores its own cached value, which is less efficient. Avoid calling drm_detect_hdmi_monitor() and use drm_display_info.is_hdmi instead. drm_detect_hdmi_monitor() is called in vc4_hdmi_connector_detect() and vc4_hdmi_connector_get_modes(). In both cases it is safe to rely on drm_display_info.is_hdmi as shown by ftrace: vc4_hdmi_connector_detect: vc4_hdmi_connector_detect() { drm_get_edid() { drm_connector_update_edid_property() { drm_add_display_info() { drm_reset_display_info(); drm_for_each_detailed_block.part.0(); drm_parse_cea_ext() { drm_find_cea_extension(); cea_db_offsets.part.0(); cea_db_is_hdmi_vsdb.part.0(); drm_parse_hdmi_vsdb_video(); /* drm_display_info.is_hdmi is cached here */ } } } } /* drm_display_info.is_hdmi is used here */ } vc4_hdmi_connector_get_modes: vc4_hdmi_connector_get_modes() { drm_get_edid() { drm_connector_update_edid_property() { drm_add_display_info() { drm_reset_display_info(); drm_for_each_detailed_block.part.0(); drm_parse_cea_ext() { drm_find_cea_extension(); cea_db_offsets.part.0(); cea_db_is_hdmi_vsdb.part.0(); drm_parse_hdmi_vsdb_video(); /* drm_display_info.is_hdmi is cached here */ } } } } /* drm_display_info.is_hdmi is used here */ drm_connector_update_edid_property(); } Signed-off-by: José Expósito <jose.exposito89@gmail.com> --- drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)