LinuxLists.cc - [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode

2023-07-18 16:12:50

Subject: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode

by Neil Armstrong

[permalink] [raw]

Subject: Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode

Hi,

On 18/07/2023 17:31, Michael Riesch wrote:
> Hi all,
>
> This series adds support for the partial display mode to the Sitronix
> ST7789V panel driver. This is useful for panels that are partially
> occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
> for this particular panel is added as well.
>
> Note: This series is already based on
> https://lore.kernel.org/lkml/[email protected]/

I understand Maxime's arguments, but by looking closely at the code,
this doesn't look like an hack at all and uses capabilities of the
panel controller to expose a smaller area without depending on any
changes or hacks on the display controller side which is coherent.

Following's Daniel's summary we cannot compare it to TV overscan
because overscan is only on *some* displays, we can still get 100%
of the picture from the signal.
While here, we cannot, there's physically less pixels on the panel.

If there's no more still a strong nack or pending comments,
I plan to apply those tomorrow.

Thanks,
Neil

>
> Looking forward to your comments!
>
> ---
> Michael Riesch (4):
> dt-bindings: vendor-prefixes: add jasonic
> dt-bindings: display: st7789v: add jasonic jt240mhqs-hwt-ek-e3 display
> drm/panel: sitronix-st7789v: add support for partial mode
> drm/panel: sitronix-st7789v: add jasonic jt240mhqs-hwt-ek-e3 support
>
> .../bindings/display/panel/sitronix,st7789v.yaml | 1 +
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 67 +++++++++++++++++++++-
> 3 files changed, 68 insertions(+), 2 deletions(-)
> ---
> base-commit: b43dae411767f34288aa347f26b5ed2dade39469
> change-id: 20230718-feature-lcd-panel-26d9f29a7830
>
> Best regards,

2023-08-03 11:13:19

On Thu, Aug 03, 2023 at 12:26:03PM +0200, Daniel Vetter wrote:
> On Thu, 3 Aug 2023 at 11:22, Maxime Ripard <[email protected]> wrote:
> >
> > On Thu, Aug 03, 2023 at 10:51:57AM +0200, Daniel Vetter wrote:
> > > On Thu, Aug 03, 2023 at 10:48:57AM +0200, Maxime Ripard wrote:
> > > > On Thu, Aug 03, 2023 at 10:11:22AM +0200, Neil Armstrong wrote:
> > > > > Hi,
> > > > >
> > > > > On 18/07/2023 17:31, Michael Riesch wrote:
> > > > > > Hi all,
> > > > > >
> > > > > > This series adds support for the partial display mode to the Sitronix
> > > > > > ST7789V panel driver. This is useful for panels that are partially
> > > > > > occluded by design, such as the Jasonic JT240MHQS-HWT-EK-E3. Support
> > > > > > for this particular panel is added as well.
> > > > > >
> > > > > > Note: This series is already based on
> > > > > > https://lore.kernel.org/lkml/[email protected]/
> > > > >
> > > > > I understand Maxime's arguments, but by looking closely at the code,
> > > > > this doesn't look like an hack at all and uses capabilities of the
> > > > > panel controller to expose a smaller area without depending on any
> > > > > changes or hacks on the display controller side which is coherent.
> > > > >
> > > > > Following's Daniel's summary we cannot compare it to TV overscan
> > > > > because overscan is only on *some* displays, we can still get 100%
> > > > > of the picture from the signal.
> > > >
> > > > Still disagree on the fact that it only affects some display. But it's
> > > > not really relevant for that series.
> > >
> > > See my 2nd point, from a quick grep aside from i915 hdmi support, no one
> > > else sets all the required hdmi infoframes correctly. Which means on a
> > > compliant hdmi tv, you _should_ get overscan. That's how that stuff is
> > > speced.
> > >
> > > Iirc you need to at least set both the VIC and the content type, maybe
> > > even more stuff.
> > >
> > > Unless all that stuff is set I'd say it's a kms driver bug if you get
> > > overscan on a hdmi TV.
> >
> > I have no doubt that i915 works there. The source of my disagreement is
> > that if all drivers but one don't do that, then userspace will have to
> > care. You kind of said it yourself, i915 is kind of the exception there.
> >
> > The exception can be (and I'm sure it is) right, but still, it deviates
> > from the norm.
>
> The right fix for these is sending the right infoframes, _not_ trying
> to fiddle with overscan margins. Only the kernel can make sure the
> right infoframes are sent out. If you try to paper over this in
> userspace, you'll make the situation worse, not better (because
> fiddling with overscan means you get scaling, and so rescaling
> artifacts, and for hard contrasts along pixel lines that'll look like
> crap).
>
> So yeah this is a case of "most upstream hdmi drivers are broken".
> Please don't try to fix kernel bugs in userspace.

ACK.

> > > > I think I'll still like to have something clarified before we merge it:
> > > > if userspace forces a mode, does it contain the margins or not? I don't
> > > > have an opinion there, I just think it should be documented.
> > >
> > > The mode comes with the margins, so if userspace does something really
> > > funny then either it gets garbage (as in, part of it's crtc area isn't
> > > visible, or maybe black bars on the screen), or the driver rejects it
> > > (which I think is the case for panels, they only take their mode and
> > > nothing else).
> >
> > Panels can usually be quite flexible when it comes to the timings they
> > accept, and we could actually use that to our advantage, but even if we
> > assume that they have a single mode, I don't think we have anything that
> > enforces that, either at the framework or documentation levels?
>
> Maybe more bugs? We've been slowly filling out all kinds of atomic kms
> validation bugs in core/helper code because as a rule of thumb,
> drivers get it wrong. Developers test until things work, then call it
> good enough, and very few driver teams make a serious effort in trying
> to really validate all invalid input. Because doing that is an
> enormous amount of work.
>
> I think for clear-cut cases like drm_panel the fix is to just put more
> stricter validation into shared code (and then if we break something,
> figure out how we can be sufficiently lenient again).

Panels are kind of weird, since they essentially don't exist at all in
the framework so it's difficult to make it handle them or their state.

It's typically handled by encoders directly, so each and every driver
would need to make that check, and from a quick grep, none of them are
(for the reasons you said).

Just like for HDMI, even though we can commit to changing those facts,
it won't happen overnight, so to circle back to that series, I'd like a
comment in the driver when the partial mode is enabled that if userspace
ever pushes a mode different from the expected one, we'll add the margins.

That way, if and when we come back to it, we'll know what the original
intent and semantics were.

Maxime

Attachments:

(No filename) (5.05 kB)
signature.asc (235.00 B)
Download all attachments

2023-08-03 13:25:36

Hi Neil,

On 8/4/23 10:41, Neil Armstrong wrote:
> Hi Michael,
>
> On 18/07/2023 17:31, Michael Riesch wrote:
>> The ST7789V controller features support for the partial mode. Here,
>> the area to be displayed can be restricted in one direction (by default,
>> in vertical direction). This is useful for panels that are partial >
>> occluded by design. Add support for the partial mode.
>
> Could you send a v2 with a comment in the code as Maxime suggests ?

Sure thing! I must admit that I do not understand his concerns exactly,
though.

@Maxime: I can prepare a suggestion but feel free to tell me the exact
wording at the preferred position.

Best regards,
Michael

>
> Thanks,
> Neil
>
>>
>> Signed-off-by: Michael Riesch <[email protected]>
>> ---
>> drivers/gpu/drm/panel/panel-sitronix-st7789v.c | 38
>> ++++++++++++++++++++++++--
>> 1 file changed, 36 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> index d16d17f21d92..729d8d7dbf7f 100644
>> --- a/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> +++ b/drivers/gpu/drm/panel/panel-sitronix-st7789v.c
>> @@ -118,6 +118,9 @@ struct st7789_panel_info {
>>       u32 bus_format;
>>       u32 bus_flags;
>>       bool invert_mode;
>> +    bool partial_mode;
>> +    u16 partial_start;
>> +    u16 partial_end;
>> };
>> struct st7789v {
>> @@ -330,9 +333,14 @@ static int st7789v_get_modes(struct drm_panel
>> *panel,
>> static int st7789v_prepare(struct drm_panel *panel)
>> {
>>       struct st7789v *ctx = panel_to_st7789v(panel);
>> -    u8 pixel_fmt, polarity;
>> +    u8 mode, pixel_fmt, polarity;
>>       int ret;
>> +    if (!ctx->info->partial_mode)
>> +        mode = ST7789V_RGBCTRL_WO;
>> +    else
>> +        mode = 0;
>> +
>>       switch (ctx->info->bus_format) {
>>       case MEDIA_BUS_FMT_RGB666_1X18:
>>           pixel_fmt = MIPI_DCS_PIXEL_FMT_18BIT;
>> @@ -472,6 +480,32 @@ static int st7789v_prepare(struct drm_panel *panel)
>>                           MIPI_DCS_EXIT_INVERT_MODE));
>>       }
>> +    if (ctx->info->partial_mode) {
>> +        u8 area_data[4] = {
>> +            (ctx->info->partial_start >> 8) & 0xff,
>> +            (ctx->info->partial_start >> 0) & 0xff,
>> +            ((ctx->info->partial_end - 1) >> 8) & 0xff,
>> +            ((ctx->info->partial_end - 1) >> 0) & 0xff,
>> +        };
>> +
>> +        ST7789V_TEST(ret, st7789v_write_command(
>> +                      ctx, MIPI_DCS_ENTER_PARTIAL_MODE));
>> +
>> +        ST7789V_TEST(ret, st7789v_write_command(
>> +                      ctx, MIPI_DCS_SET_PAGE_ADDRESS));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
>> +
>> +        ST7789V_TEST(ret, st7789v_write_command(
>> +                      ctx, MIPI_DCS_SET_PARTIAL_ROWS));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[0]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[1]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[2]));
>> +        ST7789V_TEST(ret, st7789v_write_data(ctx, area_data[3]));
>> +    }
>> +
>>       ST7789V_TEST(ret, st7789v_write_command(ctx, ST7789V_RAMCTRL_CMD));
>>       ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RAMCTRL_DM_RGB |
>>                            ST7789V_RAMCTRL_RM_RGB));
>> @@ -479,7 +513,7 @@ static int st7789v_prepare(struct drm_panel *panel)
>>                            ST7789V_RAMCTRL_MAGIC));
>>       ST7789V_TEST(ret, st7789v_write_command(ctx,
>> ST7789V_RGBCTRL_CMD));
>> -    ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_WO |
>> +    ST7789V_TEST(ret, st7789v_write_data(ctx, mode |
>>                            ST7789V_RGBCTRL_RCM(2) |
>>                            polarity));
>>       ST7789V_TEST(ret, st7789v_write_data(ctx, ST7789V_RGBCTRL_VBP(8)));
>>
>