Messages in this thread | | | Date | Thu, 3 Aug 2023 14:34:58 +0200 | From | Neil Armstrong <> | Subject | Re: [PATCH 0/4] drm/panel: sitronix-st7789v: add support for partial mode |
| |
On 03/08/2023 13:43, Maxime Ripard wrote: > On Thu, Aug 03, 2023 at 12:26:03PM +0200, Daniel Vetter wrote: >> On Thu, 3 Aug 2023 at 11:22, Maxime Ripard <mripard@kernel.org> 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/20230714013756.1546769-1-sre@kernel.org/ >>>>>> >>>>>> 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.
To be fair, a majority of the panel drivers would do the wrong init of the controller with a different mode because: - mainly the controller model is unknown - when it's known the datasheet is missing - when the datasheet is here, most of the registers are missing - and most of the time the timings are buried in the init sequence
It's sad but it's the real situation.
Only a few drivers can handle a different mode, and we should perhaps add a flag when not set rejecting a different mode for those controllers and mark the few ones who can handle that... And this should be a first step before adding an atomic Panel API.
Neil
> > That way, if and when we come back to it, we'll know what the original > intent and semantics were. > > Maxime
| |