Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] HDMI scrambling lifecycle helpers introduce sleeping functions into atomic display paths. - [Medium] Logic error in SCDC scrambling status monitor: the delayed work item stops scheduling itself, violating the intended periodic monitoring. --
commit eedf8ebd00133222505fd96065a3aa6dbeefff8d Author: Cristian Ciocaltea <[email protected]> drm/display: hdmi: Add HDMI 2.0 scrambling management helpers This commit introduces helpers to manage the full lifecycle of HDMI 2.0 SCDC scrambling on both source and sink sides. It includes configuration of scrambling, high TMDS clock ratios, and a delayed work item to monitor and restore the sink's scrambling status. > diff --git a/drivers/gpu/drm/display/drm_hdmi_helper.c > b/drivers/gpu/drm/display/drm_hdmi_helper.c > index 5cb0b033b1713..bf1c7963b984b 100644 > --- a/drivers/gpu/drm/display/drm_hdmi_helper.c > +++ b/drivers/gpu/drm/display/drm_hdmi_helper.c [ ... ] > +static int drm_connector_hdmi_try_scrambling_setup(struct drm_connector > *connector) > +{ > + bool done; > + > + done = drm_scdc_set_high_tmds_clock_ratio(connector, true); > + if (!done) > + return -EIO; [Severity: High] Since drm_connector_hdmi_enable_scrambling() might be called from atomic commit paths (like CRTC atomic enable), is it safe to call drm_scdc_set_high_tmds_clock_ratio() here? Looking at drm_scdc_set_high_tmds_clock_ratio() in drm_scdc_helper.c, it uses a sleeping wait: usleep_range(1000, 2000); Could this lead to sleeping in atomic context warnings or deadlocks if reached during an atomic display update? [Severity: Medium] If drm_scdc_set_high_tmds_clock_ratio() or drm_scdc_set_scrambling() fail due to a transient DDC error, we return -EIO early here and bypass the schedule_delayed_work() call below. Does this inadvertently stop the periodic monitoring from retrying the setup later? > + > + done = drm_scdc_set_scrambling(connector, true); > + if (!done) > + return -EIO; > + > + if (READ_ONCE(connector->hdmi.scrambler_enabled)) > + schedule_delayed_work(&connector->hdmi.scdc_work, > + > msecs_to_jiffies(DRM_HDMI_SCDC_POLL_DELAY_MS)); > + > + return 0; > +} > + > +static void drm_connector_hdmi_scdc_work(struct work_struct *work) > +{ [ ... ] > + if (READ_ONCE(connector->hdmi.scrambler_enabled) && > + !drm_scdc_get_scrambling_status(connector)) > + drm_connector_hdmi_try_scrambling_setup(connector); > +} [Severity: Medium] If the sink scrambling status is healthy (drm_scdc_get_scrambling_status() returns true), this function exits without rescheduling itself. Does this mean the periodic work item stops monitoring the SCDC status entirely after the first successful check? The commit message mentions a periodic work item that monitors the status and retries when lost, but it appears the loop breaks as soon as the sink is in a good state. [ ... ] > +int drm_connector_hdmi_disable_scrambling(struct drm_connector *connector) > +{ [ ... ] > + /* > + * A driver may force scrambler_enabled at init to trigger a disable > + * at boot (e.g. vc4), bypassing the enable path that sets up the work > + * item. Only cancel it once it has actually been initialized. > + */ > + if (hdmi->scdc_work_initialized) > + cancel_delayed_work_sync(&hdmi->scdc_work); [Severity: High] If drm_connector_hdmi_disable_scrambling() is invoked from an atomic context (e.g., during CRTC or encoder atomic disable callbacks), could calling cancel_delayed_work_sync() cause a sleep in atomic context? Since cancel_delayed_work_sync() can block waiting for the work to finish, this might cause system instability if called while holding spinlocks or from a non-preemptible path. > + > + if (connector->ddc && > + connector->status == connector_status_connected && > + drm_scdc_sink_supports_scrambling(connector)) { > + drm_scdc_set_scrambling(connector, false); > + drm_scdc_set_high_tmds_clock_ratio(connector, false); > + } > + > + return hdmi->funcs->scrambler_disable(connector); > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10
