Re: [PATCH v3 2/2] drm/tiny: add support for PIXPAPER 4.26 monochrome e-ink panel

Hi Devarsh,

Thanks for the SSD1677 work, and for adding the panel entry on top - that
was a convenient starting point. I have tested it on the PIXPAPER 4.26m.

Setup: Raspberry Pi 5, rpi-7.2.y with your 14 patches cherry-picked (clean
apply), SPI0 at 20 MHz, DC/RST/BUSY on GPIO26/5/6, CONFIG_DRM_SSD16XX=y,
drm.debug=0x02, no compositor.

Bring-up is fine: it probes, the connector comes up at 800x480, and
reset/BUSY and runtime PM all work. Three functional issues below - one of
them currently blocks the panel, and none produce any error or warning from
the driver.


1) Image is horizontally mirrored
---------------------------------

At rotation 0, content written to the framebuffer's top-left appears at the
panel's top-right and text reads as in a mirror. A pattern with four
differently sized corner blocks confirms it is a plain horizontal mirror,
not a flip or a transpose.

RAM X ascending appears to map right-to-left on this module, and I could
not find a source-direction control on SSD1677 to compensate with -
command 0x01 only affects the gate side. My out-of-tree 4.26m driver
worked around it with `src_x = width - 1 - x`; I think that just got lost
in the port, since it only shows up with asymmetric content.

There is a patch for this in the series linked below. I kept the flag
per-panel since I only have this one panel type; happy to move it into the
SSD1677 config if it turns out to be controller-wide.


2) Rotation
-----------

Setting rotation through the connector property at runtime:

  0     mirrored, as in 1) above
  90    garbage
  180   nothing drawn, the panel keeps its previous content
  270   garbage

fb_dirty(), display_update() and the BUSY wait all report success in every
case.

180 comes down to skip_data_entry_mode: the reversed RAM window fb_dirty()
programs for 90/180 has no matching decrement mode while 0x11 is never
sent. Dropping the flag fixes it; there is a patch for that too.

90 and 270 still fail at runtime with that fix, but rotation = <90> in DT
works - fb0 and the connector come up as 480x800 and the image is a clean
clockwise portrait, matching "90 (portrait CW)" in your commit message. So
the portrait packing looks sound, at least at 90; what seems to be missing
at runtime is the mode swap probe() does for portrait orientations.
Without it the framebuffer stays 800x480 while the driver switches to
column-major packing - fb_dirty logs "fb=800x480, orientation=90" - and
data_size is 48000 either way, so no size check catches it.

Since DT rotation works, one option is to refuse the runtime change rather
than let it corrupt the display; there is a patch for that as well.


3) refresh_mode does not take effect
------------------------------------

The property value reads back changed but the driver keeps the old one.
Reproduced with modetest -w and with modetest -a: refresh_mode reads 0
while fb_dirty logs refresh_mode=1 and display_update sends 0xf4.

As far as I can tell it is only latched in ssd16xx_plane_atomic_update(),
and only when the old and new connector states differ. I have not worked
out which commit ends up carrying what, so I cannot say exactly where it
gets lost, but the new value never reaches device->refresh_mode. rotation
is presumably unaffected because device->orientation is assigned
unconditionally in ssd16xx_crtc_atomic_enable(). color_mode and the
clear_on_* properties are latched in the same block so I would expect the
same, but I only measured refresh_mode.

This one is in your connector-state design so I have not patched it, but I
am happy to test a fix. For the measurements below I changed the relevant
defaults in the driver and rebuilt instead.


4) The 0xF7/0xFF question
-------------------------

All three work on this panel. 0xF4 is what the driver uses today; 0xFF
gives a correct partial refresh with slight ghosting, which should cover
the "pending hardware verification" note in the SSD1677 comment; and 0xF7
also renders correctly, taking 3888 ms against 3664 ms for 0xF4 - the extra
time presumably being the analog and clock shutdown it adds. So 0xF4 is not
strictly required here, and the choice between it and 0xF7 looks like a
power-versus-latency one.

Fast (0xC7) is consistently a little slower than full, not faster. Within
one boot - the first frame after init runs full, the rest fast - I measure
3664 ms for full and 3800-3804 ms after it, with an indistinguishable
image. FAST does still reuse SSD1683_CTRL2_FAST_REFRESH while PARTIAL and
FULL got SSD1677-specific values, which may or may not be related. Not a
blocker.


5) Smaller points
-----------------

A few places where the commit messages and comments do not match the code:
the panel is 800x480 not 960x680, the booster soft-start is described as
four bytes where the code sends five, the active area is given as
91.8 x 65.0 mm where the mode has 93 x 56, and nine pixel formats are
listed where ssd16xx_formats[] has only XRGB8888 and R1. The SM comment
also reads inverted to me against the datasheet, though the value itself
works. Happy to spell any of these out.

Not covered: the DRM_FORMAT_R1 input path, 3-wire SPI, and system suspend
(/sys/power/state is empty on the Pi 5).


Patches
-------

I have posted the three patches as a separate series, so they do not get
buried in this thread and can be picked up on their own:

  https://lore.kernel.org/all/[email protected]/

  drm/tiny: ssd16xx: send data entry mode on SSD1677
  drm/tiny: ssd16xx: support panels whose RAM X order is reversed
  drm/tiny: ssd16xx: reject rotation changes without a matching mode

The first and third touch code inside your series, so they cannot stand
alone before it lands. Please keep them as separate commits if you can -
git am preserves authorship; if you fold the first one in, a
Co-developed-by with my Signed-off-by would be appreciated. The third is
only a stopgap, so if you would rather fix the runtime path properly,
please do and I will drop it.

The panel support follows separately once your work is in drm-misc-next, so
that it carries no dependency:

  dt-bindings: display: solomon,ssd16xx: add Mayqueen PIXPAPER 4.26m panel
  drm/tiny: ssd16xx: add Mayqueen PIXPAPER 4.26m panel

with Suggested-by/Co-developed-by for you, as discussed.

The property issue in 3) and the commit message wording are left with you.

I maintain the existing PIXPAPER MAINTAINERS entry - would you mind adding
me as R: on the SSD16xx one, so I stay on CC and can retest?

Once the mirroring and rotation issues are sorted I will be glad to give
the SSD1677 patch a Tested-by. Thanks again for accommodating the panel
entry.

Regards,
LiangCheng