Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] drm/tidss: tidss_crtc_install_state() enables inactive CRTCs and leaks PM - [High] drm/tidss: missing drm_mode_config_create_initial_state() in tidss_probe() - [High] drm/tidss: GEM-less readout fb fails prepare_fb() and zeroes DMA address - [Medium] drm/tidss: tidss_crtc_compare_state() checks bus config on inactive CRTCs - [Medium] drm/tidss: plane readout incomplete state and -EINVAL trigger WARN_ON on commit - [Medium] drm/tidss: incomplete crtc_state->plane_mask readout on partial commits --
commit 8f81eb167182a1d35a4ac3bf95240f371e285879 Author: Maxime Ripard <[email protected]> drm/tidss: Implement readout support This commit implements hardware state readout and state comparison callbacks for CRTCs, bridges, and planes in the tidss driver using the DRM atomic state readout helpers. > diff --git a/drivers/gpu/drm/tidss/tidss_crtc.c > b/drivers/gpu/drm/tidss/tidss_crtc.c > index 3da49f77fa080..4409f8c9114bb 100644 > --- a/drivers/gpu/drm/tidss/tidss_crtc.c > +++ b/drivers/gpu/drm/tidss/tidss_crtc.c [ ... ] > @@ -370,6 +374,90 @@ static struct drm_crtc_state > *tidss_crtc_create_state(struct drm_crtc *crtc) > return &tstate->base; > } > > +static int tidss_crtc_readout_state(struct drm_crtc *crtc, > + struct drm_atomic_sro_state *state, > + struct drm_crtc_state *crtc_state) > +{ > + struct drm_device *ddev = crtc->dev; > + struct tidss_device *tidss = to_tidss(ddev); > + struct dispc_device *dispc = tidss->dispc; > + struct tidss_crtc_state *tstate = to_tidss_crtc_state(crtc_state); > + struct tidss_crtc *tcrtc = > + to_tidss_crtc(crtc); > + struct drm_display_mode mode = {}; > + int ret; > + > + tidss_runtime_get(tidss); > + > + if (!dispc_vp_is_enabled(dispc, tcrtc->hw_videoport)) > + goto out; > + > + tstate->base.active = 1; > + tstate->base.enable = 1; > + > + ret = dispc_vp_readout_mode(dispc, tcrtc->hw_videoport, &mode); > + if (ret) > + goto err_runtime_put; > + > + ret = drm_atomic_set_mode_for_crtc(&tstate->base, &mode); > + if (WARN_ON(ret)) > + goto err_runtime_put; > + > + drm_mode_copy(&tstate->base.adjusted_mode, &mode); > + > + tstate->bus_flags = dispc_vp_get_bus_flags(dispc, tcrtc->hw_videoport); > + ret = dispc_vp_get_bus_format(dispc, tcrtc->hw_videoport, > + &tstate->bus_format); > + if (ret) > + goto err_runtime_put; > + > + /* > + * The active connectors and planes will be filled by their > + * respective readout callbacks. > + */ [Severity: Medium] What happens to crtc_state->plane_mask (as well as connector_mask and encoder_mask) during post-commit verification in drm_atomic_sro_readout_and_compare() when a blocking commit includes the CRTC but only a subset of its active planes? When drm_atomic_sro_readout_and_compare(commit) calls drm_atomic_helper_sro_build_state(dev, commit) with a non-NULL commit, drm_atomic_helper_sro_readout_planes_state() only invokes atomic_sro_readout_state() for planes present in that commit. If two planes are active on a CRTC and userspace performs a blocking page flip or property update on only one of them (without changing x/y or zpos, so tidss_atomic_check() does not call drm_atomic_add_affected_planes()), tidss_plane_atomic_check() adds the CRTC to the commit via drm_atomic_get_crtc_state(), but the second active plane is not in the commit. Because tidss_crtc_readout_state() leaves crtc_state->plane_mask at 0 and relies on tidss_plane_atomic_readout_state() to OR in drm_plane_mask(plane), won't the untouched plane's bit be missing from the readout CRTC state's plane_mask, causing drm_atomic_helper_crtc_compare_state() to report a plane_mask mismatch? > + > +out: > + tidss_runtime_put(tidss); > + return 0; > + > +err_runtime_put: > + tidss_runtime_put(tidss); > + return ret; > +} > + > +static void tidss_crtc_install_state(struct drm_crtc *crtc, > + struct drm_crtc_state *crtc_state) > +{ > + struct tidss_crtc *tcrtc = to_tidss_crtc(crtc); > + struct drm_device *ddev = crtc->dev; > + struct tidss_device *tidss = to_tidss(ddev); > + int ret; > + > + tidss_runtime_get(tidss); > + > + ret = dispc_vp_enable_clk(tidss->dispc, tcrtc->hw_videoport); > + if (ret != 0) > + return; > + > + /* Turn vertical blanking interrupt reporting on. */ > + drm_crtc_vblank_on(crtc); > +} [Severity: High] Should tidss_crtc_install_state() check whether crtc_state->active is set before enabling runtime PM, the videoport clock, and vblank reporting? When a videoport is disabled in hardware (!dispc_vp_is_enabled()), tidss_crtc_readout_state() jumps to out and returns 0 with crtc_state->active == false, and drm_atomic_helper_sro_readout_crtc_state() stores that inactive state in state->crtcs[i].state. Later, drm_atomic_sro_install_state() iterates over all CRTCs and invokes atomic_sro_install_state() for every non-NULL crtc_state: drivers/gpu/drm/drm_atomic_state_readout.c:drm_atomic_sro_install_state() { ... crtc_state = state->crtcs[i].state; if (!crtc_state) continue; if (crtc_funcs->atomic_sro_install_state) crtc_funcs->atomic_sro_install_state(crtc, crtc_state); ... } For an inactive CRTC, won't tidss_crtc_install_state() unconditionally call tidss_runtime_get(), dispc_vp_enable_clk(), and drm_crtc_vblank_on(), leaking both the runtime PM reference and the videoport clock enable count because crtc->state->active is false (so tidss_crtc_atomic_disable() will not run to release them)? Also, if dispc_vp_enable_clk() returns a non-zero error in tidss_crtc_install_state(), doesn't the early return leak the runtime PM reference acquired by tidss_runtime_get(tidss)? > + > +static bool tidss_crtc_compare_state(struct drm_crtc *crtc, > + struct drm_printer *p, > + struct drm_crtc_state *expected, > + struct drm_crtc_state *actual) > +{ > + struct tidss_crtc_state *t_expected = to_tidss_crtc_state(expected); > + struct tidss_crtc_state *t_actual = to_tidss_crtc_state(actual); > + bool ret = drm_atomic_helper_crtc_compare_state(crtc, p, expected, > actual); > + > + STATE_CHECK_U32_X(ret, p, crtc->name, t_expected, t_actual, bus_format); > + STATE_CHECK_U32_X(ret, p, crtc->name, t_expected, t_actual, bus_flags); > + > + return ret; > +} [Severity: Medium] Can tidss_crtc_compare_state() report false mismatches for bus_format and bus_flags when an active CRTC is disabled? When a CRTC is disabled in a blocking commit, tidss_crtc_duplicate_state() copies the previous active state's non-zero bus_format and bus_flags into the new state: drivers/gpu/drm/tidss/tidss_crtc.c:tidss_crtc_duplicate_state() { ... state->bus_format = current_state->bus_format; state->bus_flags = current_state->bus_flags; ... } During post-commit readout in tidss_crtc_readout_state(), !dispc_vp_is_enabled() is true and jumps to out before reading bus_flags or bus_format, leaving them 0 in t_actual. While drm_atomic_helper_crtc_compare_state() returns early when both expected->active and actual->active are false: drivers/gpu/drm/drm_atomic_state_readout_helper.c:drm_atomic_helper_crtc_compare_state() { ... /* Both inactive, mode and properties are meaningless. */ if (!expected->active && !actual->active) return ret; ... } tidss_crtc_compare_state() still checks bus_format and bus_flags unconditionally. Should those checks be skipped when the CRTC is inactive? > diff --git a/drivers/gpu/drm/tidss/tidss_kms.c > b/drivers/gpu/drm/tidss/tidss_kms.c > index 3b97c08c4f714..b0b69b8531111 100644 > --- a/drivers/gpu/drm/tidss/tidss_kms.c > +++ b/drivers/gpu/drm/tidss/tidss_kms.c [ ... ] > @@ -133,6 +136,7 @@ static const struct drm_mode_config_funcs > mode_config_funcs = { > .fb_create = drm_gem_fb_create, > .atomic_check = tidss_atomic_check, > .atomic_commit = drm_atomic_helper_commit, > + .atomic_sro_readout_state = drm_atomic_helper_sro_readout_state, > .hw_reset = tidss_hw_reset, > }; [Severity: High] How are mode_config_funcs.atomic_sro_readout_state (and tidss_crtc_install_state) and the fallback mode_config_funcs.hw_reset reached during probe? In drivers/gpu/drm/drm_mode_config.c, atomic_sro_readout_state and hw_reset are only invoked by drm_mode_config_create_initial_state(): drivers/gpu/drm/drm_mode_config.c:drm_mode_config_create_initial_state() { ... if (dev->mode_config.funcs->atomic_sro_readout_state) { ret = dev->mode_config.funcs->atomic_sro_readout_state(dev); if (!ret) goto out; } /* Reset the hardware first ... */ if (dev->mode_config.funcs->hw_reset) dev->mode_config.funcs->hw_reset(dev); ... } However, tidss_probe() in drivers/gpu/drm/tidss/tidss_drv.c still calls drm_mode_config_reset(ddev): drivers/gpu/drm/tidss/tidss_drv.c:tidss_probe() { ... drm_kms_helper_poll_init(ddev); drm_mode_config_reset(ddev); ret = drm_dev_register(ddev, 0); ... } Because drm_mode_config_reset() only allocates blank software states without calling atomic_sro_readout_state or hw_reset, isn't initial state readout (along with the hardware reset moved into tidss_hw_reset()) never executed at probe time? Furthermore, if tidss_probe() is updated to call drm_mode_config_create_initial_state(ddev) after pm_runtime_enable(dev), won't dispc_reset_hw() fail when readout falls back to hw_reset because pm_runtime_set_active(dev) returns -EAGAIN when runtime PM is already enabled? > diff --git a/drivers/gpu/drm/tidss/tidss_plane.c > b/drivers/gpu/drm/tidss/tidss_plane.c > index b0d5d6959b885..ebdb63f8e8f17 100644 > --- a/drivers/gpu/drm/tidss/tidss_plane.c > +++ b/drivers/gpu/drm/tidss/tidss_plane.c [ ... ] > @@ -176,11 +179,186 @@ static const struct drm_plane_helper_funcs > tidss_primary_plane_helper_funcs = { > .get_scanout_buffer = drm_fb_dma_get_scanout_buffer, > }; > > +static const struct drm_framebuffer_funcs tidss_plane_readout_fb_funcs = { > + .destroy = drm_gem_fb_destroy, > +}; > + > +static struct drm_framebuffer *tidss_plane_readout_fb(struct drm_plane > *plane) > +{ > + struct drm_device *ddev = plane->dev; > + struct tidss_device *tidss = to_tidss(ddev); > + struct dispc_device *dispc = tidss->dispc; > + struct tidss_plane *tplane = to_tidss_plane(plane); > + const struct drm_format_info *info; > + struct drm_framebuffer *fb; > + int ret; > + > + fb = kzalloc_obj(*fb); > + if (!fb) > + return ERR_PTR(-ENOMEM); > + > + fb->dev = plane->dev; > + > + info = dispc_plane_get_current_format(dispc, tplane->hw_plane_id); > + if (IS_ERR(info)) { > + ret = PTR_ERR(info); > + goto err_free_fb; > + } > + > + // TODO: Figure out YUV and multiplanar formats, along with > + // the color encoding and range > + if (info->is_yuv) { > + ret = -EINVAL; > + goto err_free_fb; > + } > + > + fb->format = info; > + > + dispc_plane_get_picture_size(dispc, tplane->hw_plane_id, &fb->width, > + &fb->height); > + > + /* > + * A pixel increment of 1 means that the DMA engine fetches every > + * pixel of the source buffer. Any other value means that > + * horizontal decimation is enabled, and the buffer geometry > + * cannot be reconstructed from the registers. > + */ > + if (dispc_plane_get_pixel_inc(dispc, tplane->hw_plane_id) != 1) { > + ret = -EINVAL; > + goto err_free_fb; > + } > + > + /* > + * The row increment register holds the extra bytes between the > + * end of a row of the picture and the beginning of the next one, > + * which includes any padding the firmware might have programmed. > + */ > + fb->pitches[0] = fb->width * (drm_format_info_bpp(info, 0) / 8) + > + dispc_plane_get_row_inc(dispc, tplane->hw_plane_id) - > 1; > + > + /* > + * TODO: The DMA base address has the offsets baked in, and there > + * is no backing buffer to recover them from. > + */ > + fb->offsets[0] = 0; > + > + ret = drm_framebuffer_init(plane->dev, fb, > &tidss_plane_readout_fb_funcs); > + if (ret) { > + kfree(fb); > + return ERR_PTR(ret); > + } > + > + return fb; > + > +err_free_fb: > + kfree(fb); > + return ERR_PTR(ret); > +} [Severity: High] What happens on a subsequent atomic commit if a plane still references the framebuffer created by tidss_plane_readout_fb() (for example, when updating plane properties or when tidss_atomic_check() adds affected planes on the CRTC)? Because tidss_plane_readout_fb() allocates fb with fb->obj[0] == NULL and neither tidss_plane_helper_funcs nor tidss_primary_plane_helper_funcs sets a custom .prepare_fb callback, drm_atomic_helper_prepare_planes() defaults to drm_gem_plane_helper_prepare_fb() on this DRIVER_GEM driver. Won't drm_gem_fb_get_obj(state->fb, 0) trigger drm_WARN_ON_ONCE(dev, !fb->obj[plane]) and fail the commit with -EINVAL? In addition, if dispc_plane_setup() is called while state->fb is a GEM-less readout framebuffer: drivers/gpu/drm/tidss/tidss_dispc.c:dispc_plane_setup() { ... dma_addr_t dma_addr = drm_fb_dma_get_gem_addr(state->fb, state, 0); ... dispc_vid_write(dispc, hw_plane, DISPC_VID_BA_0, dma_addr & 0xffffffff); dispc_vid_write(dispc, hw_plane, DISPC_VID_BA_EXT_0, (u64)dma_addr >> 32); ... } drm_fb_dma_get_gem_addr() returns 0 when fb->obj[0] is NULL, which would overwrite DISPC_VID_BA_0 and DISPC_VID_BA_EXT_0 with 0x0. > + > +static struct drm_crtc *tidss_plane_readout_crtc(struct drm_plane *plane) > +{ > + struct drm_device *dev = plane->dev; > + > + /* TODO: Add support for multiple CRTCs */ > + if (dev->num_crtcs != 1) > + return ERR_PTR(-EINVAL); > + > + return list_first_entry(&dev->mode_config.crtc_list, struct drm_crtc, > head); > +} > + > +static int tidss_plane_atomic_readout_state(struct drm_plane *plane, > + struct drm_atomic_sro_state *state, > + struct drm_plane_state *plane_state) > +{ [ ... ] > + dispc_plane_get_size(dispc, tplane->hw_plane_id, &plane_state->crtc_w, > + &plane_state->crtc_h); > + > + // TODO: Handle crtc_x/crtc_x/src_x/src_y > + // crtc_x/crtc_y are handled by DISPC_OVR_ATTRIBUTES / > OVR1_DSS_ATTRIBUTES > + > + // TODO: Handle zpos, see DISPC_OVR_ATTRIBUTES / OVR1_DSS_ATTRIBUTES > + > + plane_state->src.x1 = 0; > + plane_state->src.x2 = plane_state->src_w; > + plane_state->src.y1 = 0; > + plane_state->src.y2 = plane_state->src_h; > + plane_state->dst.x1 = 0; > + plane_state->dst.x2 = plane_state->crtc_w; > + plane_state->dst.y1 = 0; > + plane_state->dst.y2 = plane_state->crtc_h; [Severity: Medium] Won't the unimplemented cases in tidss_plane_readout_fb(), tidss_plane_readout_crtc(), and tidss_plane_atomic_readout_state() trigger kernel warnings and state mismatch errors during normal runtime commits? When all bridges and connectors in the pipeline implement readout callbacks, drm_atomic_sro_device_can_readout() returns true and every blocking atomic commit runs drm_atomic_sro_readout_and_compare() in commit_tail(). If userspace commits a YUV plane (info->is_yuv), enables horizontal decimation (dispc_plane_get_pixel_inc() != 1), or runs on a multi-CRTC SoC (dev->num_crtcs != 1), tidss_plane_atomic_readout_state() returns -EINVAL, which triggers drm_WARN_ON(dev, ret) in drm_atomic_helper_sro_readout_plane_state() and WARN_ON(IS_ERR(readout_state)) in drm_atomic_sro_readout_and_compare(). Similarly, if a blocking commit positions a plane at non-zero crtc_x/crtc_y or src_x/src_y, or uses a non-zero zpos/normalized_zpos, leaving those fields at 0 in tidss_plane_atomic_readout_state() will cause drm_atomic_helper_plane_compare_state() to log state mismatch errors on every such commit. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=32
