Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Adding old pipeline colorops to the atomic state on deactivation causes the AMD driver to incorrectly detect a deactivated pipeline as active. Pre-existing issues: - [High] Modifying a COLOROP property via the ATOMIC IOCTL fails to pull the associated plane into the atomic state, causing the update to be silently ignored by the driver. - [High] Changing the COLOR_PIPELINE plane property does not pull the affected colorops into the atomic state unless a CRTC modeset is concurrently triggered. -- commit c677eb54425763294f2391cd91e9253b70b8e880 Author: Melissa Wen <[email protected]> drm/atomic: only add states of active or transient active colorops Only consider affected colorop states those that are part of an active color pipeline or a pipeline that is about to be activated or deactivated in the same atomic commit, i.e., colorop is in the chain of old/new plane color pipeline property. To cover color_pipeline deactivation, remove the condition for plane_state->color_pipeline. > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c > index 170de30c28ae4..0d36a955c4d43 100644 > --- a/drivers/gpu/drm/drm_atomic.c > +++ b/drivers/gpu/drm/drm_atomic.c [ ... ] > @@ -812,6 +812,57 @@ static int drm_atomic_plane_check(const struct > drm_plane_state *old_plane_state, > return 0; > } > > +/* > + * This function walks old and new plane state color pipelines and adds all > + * colorops in use by @plane to the atomic configuration @state. This is > useful > + * when an atomic commit needs to check all currently enabled or about to be > + * enabled colorop on @plane, e.g. when changing the mode. This also avoids > + * including colorop states that are not part of the atomic state. > + * > + * Returns: > + * 0 on success or can fail with -EDEADLK or -ENOMEM. When the error is > EDEADLK > + * then the w/w mutex code has detected a deadlock and the entire atomic > + * sequence must be restarted. All other errors are fatal. > + */ > +static int > +drm_atomic_add_pipeline_colorops(struct drm_atomic_commit *state, > + struct drm_plane *plane) > +{ > + struct drm_colorop *colorop; > + struct drm_colorop_state *colorop_state; > + struct drm_plane_state *new_plane_state, *old_plane_state; > + > + new_plane_state = drm_atomic_get_new_plane_state(state, plane); > + old_plane_state = drm_atomic_get_old_plane_state(state, plane); > + > + if (WARN_ON(!new_plane_state || !old_plane_state)) > + return -EINVAL; > + > + drm_dbg_atomic(plane->dev, > + "Adding old+new pipeline colorops for [PLANE:%d:%s]\n", > + plane->base.id, plane->name); > + > + drm_for_each_colorop_in_pipeline(colorop, > + new_plane_state->color_pipeline) { > + colorop_state = drm_atomic_get_colorop_state(state, colorop); [Severity: High] This is a pre-existing issue, but does modifying a colorop property via the atomic ioctl fail to pull the associated plane into the atomic state? The call to drm_atomic_get_colorop_state() successfully fetches and adds the colorop to the state: drivers/gpu/drm/drm_atomic.c:drm_atomic_get_colorop_state() { ... ret = drm_modeset_lock(&colorop->plane->mutex, state->acquire_ctx); colorop_state = drm_atomic_helper_colorop_duplicate_state(colorop); ... } However, it does not call drm_atomic_get_plane_state() to add the parent plane. When the standard driver commit path executes, it iterates only over planes present in the state: drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_commit_planes() { ... for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { ... funcs->atomic_update(plane, state); ... } Does this mean the driver's atomic_update hook is skipped, and the hardware is not programmed with the new colorop parameters unless userspace also modifies a plane property in the same commit? > + if (IS_ERR(colorop_state)) > + return PTR_ERR(colorop_state); > + } > + > + /* Same color pipeline as new; no point walking old. */ > + if (new_plane_state->color_pipeline == old_plane_state->color_pipeline) > + return 0; > + > + drm_for_each_colorop_in_pipeline(colorop, > + old_plane_state->color_pipeline) { > + colorop_state = drm_atomic_get_colorop_state(state, colorop); [Severity: High] Does adding the old pipeline's colorops to the atomic state during deactivation cause issues with active pipeline detection? When the pipeline is being deactivated, the old colorops' bypass flags are untouched and remain false. In the AMD driver, dm_plane_color_pipeline_active() checks all colorops added to the state: drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:dm_plane_color_pipeline_active() { ... for_each_oldnew_colorop_in_state(state, colorop, old_colorop_state, new_colorop_state, i) { ... if (!cstate->bypass) return true; } } Will this cause the driver to see the old colorops, incorrectly evaluate the deactivated pipeline as active, and attempt to program disabled hardware? > + if (IS_ERR(colorop_state)) > + return PTR_ERR(colorop_state); > + } > + > + return 0; > +} [ ... ] > @@ -1591,11 +1642,9 @@ drm_atomic_add_affected_planes(struct > drm_atomic_commit *state, > if (IS_ERR(plane_state)) > return PTR_ERR(plane_state); > > - if (plane_state->color_pipeline) { > - ret = drm_atomic_add_affected_colorops(state, plane); > - if (ret) > - return ret; > - } > + ret = drm_atomic_add_pipeline_colorops(state, plane); [Severity: High] This is a pre-existing issue, but does changing the color pipeline plane property fail to pull the affected colorops into the atomic state if a CRTC modeset isn't triggered? The drm_atomic_add_pipeline_colorops() function is invoked here from drm_atomic_add_affected_planes(). However, this is only called when a modeset is occurring: drivers/gpu/drm/drm_atomic_helper.c:drm_atomic_helper_check_modeset() { ... if (!drm_atomic_crtc_needs_modeset(new_crtc_state)) continue; ... ret = drm_atomic_add_affected_planes(state, crtc); } If userspace dynamically changes the color pipeline property on a plane as a standalone update without triggering a full CRTC modeset, will this bypass drm_atomic_add_affected_planes() entirely? Could this result in the colorops for the newly activated or deactivated pipeline never being added to the atomic commit state? > + if (ret) > + return ret; > } > return 0; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
