[PATCH 0/1] drm/vc4: hdmi: Replace drm_detect_hdmi_monitor() with drm_display_info.is_hdmi
Hello everyone,
This patch replaces the calls to drm_detect_hdmi_monitor() with the more efficient drm_display_info.is_hdmi in the VC4 driver.
After applying it, vc4_encoder->hdmi_monitor could be removed in a follow up patch. However, since it is used by some code not present in the mainline kernel but present in the Raspberry Pi tree [1] I decided to send only this first patch and see if the maintainers also want to remove vc4_encoder->hdmi_monitor.
Best wishes, José Expósito
[1] https://github.com/raspberrypi/linux/blob/rpi-5.15.y/drivers/gpu/drm/vc4/vc4...
José Expósito (1): drm/vc4: hdmi: Replace drm_detect_hdmi_monitor() with is_hdmi
drivers/gpu/drm/vc4/vc4_hdmi.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
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(-)
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);
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
dri-devel@lists.freedesktop.org
-
José Expósito -
Maxime Ripard