Hi Fabio, > ssd132x_clear_screen() sends its zeroes without setting the column and > row ranges first, so they land in whatever window the last > ssd132x_update_rect() left programmed.
Good catch, I noticed the same asymmetry. But as Sashiko pointed out and you confirmed, ssd132x_clear_screen() is not reachable today, so there is no user-visible bug being fixed here. I don't think this should go to stable, and I would drop both the Cc: stable and the Fixes: tag in a v2. There is a second reason the clear cannot work in its current form. The driver does not override .atomic_commit_tail, so drm_atomic_helper_commit_tail() runs drm_atomic_helper_commit_modeset_disables() before drm_atomic_helper_commit_planes(). By the time the plane's atomic_disable runs, ssd130x_encoder_atomic_disable() has already sent DISPLAY_OFF and called ssd130x_power_off(). So even with the !plane_state->crtc guard fixed, the clear would be writing pixels into a controller that is already powered down. None of this makes the patch wrong - I do believe in making functions do what they are supposed to do. It does mean this is a cleanup rather than a fix, though, and that it cannot be verified on hardware until the disable path itself is sorted out. > + ret = ssd130x_write_cmd(ssd130x, 3, SSD132X_SET_COL_RANGE, 0, columns - > 1); > + if (ret < 0) > + return; > + > + ret = ssd130x_write_cmd(ssd130x, 3, SSD132X_SET_ROW_RANGE, 0, height - > 1); > + if (ret < 0) > + return; This duplicates the window setup that ssd132x_update_rect() already does. I would rather see those two commands factored out into a small helper (e.g. ssd132x_set_window()) and called from both places. Note that ssd132x_update_rect() cannot just be called from ssd132x_clear_screen() the way ssd133x_update_rect() could be from ssd133x_clear_screen(): it takes the R8 shadow buffer and packs pairs of pixels into data_array, and clear_screen() only has the data_array from the CRTC state. In the SSD135x series I have in flight, ssd135x_clear_screen() does call ssd135x_update_rect() with a full-screen rect, but only because no pixel conversion is involved there. -- Thanks, Amit
