Re: [PATCH] drm/ssd130x: Set the address window in ssd132x_clear_screen()

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