Re: [PATCH 7/7] dt-bindings: display: add panel-timing property to sitronix,st7789v

[Maxime Ripard <maxime@cerno.tech>]

[-- Attachment #1: Type: text/plain, Size: 4021 bytes --]

On Fri, Mar 31, 2023 at 11:36:43AM +0200, Michael Riesch wrote:
> On 3/30/23 16:58, Maxime Ripard wrote:
> > On Wed, Mar 29, 2023 at 12:08:50PM +0200, Michael Riesch wrote:
> >> On 3/29/23 11:16, Maxime Ripard wrote:
> >>> On Thu, Mar 16, 2023 at 11:29:53PM +0100, Michael Riesch wrote:
> >>>> Hi Rob,
> >>>>
> >>>> On 3/16/23 22:57, Rob Herring wrote:
> >>>>> On Tue, Mar 14, 2023 at 12:56:44PM +0100, Gerald Loacker wrote:
> >>>>>> The sitronix-st7789v driver now considers the panel-timing property.
> >>>>>
> >>>>> I read the patch for that and still don't know 'why'. Commit messages 
> >>>>> should answer why.
> >>>>>
> >>>>>> Add the property to the documentation.
> >>>>>
> >>>>> We generally don't put timings in DT for panels. Why is this one 
> >>>>> special?
> >>>>
> >>>> For now, having the timings in the device tree allows for setting the
> >>>> hsync/vsync/de polarity.
> >>>>
> >>>> As a next step, we aim to implement the partial mode feature of this
> >>>> panel. It is possible to use only a certain region of the panel, which
> >>>> is helpful e.g., when a part of the panel is occluded and should not be
> >>>> considered by DRM. We thought that this could be specified as timing in DT.
> >>>>
> >>>> (The hactive and vactive properties serve as dimensions of this certain
> >>>> region, of course. We still need to specify somehow the position of the
> >>>> region. Maybe with additional properties hactive-start and vactive-start?)
> >>>>
> >>>> What do you think about that?
> >>>
> >>> I don't see why we would need the device tree to support that. What you
> >>> described is essentially what overscan is for HDMI/analog output, and we
> >>> already have everything to deal with overscan properly in KMS.
> >>
> >> Thanks for your response, but I am afraid I don't quite follow.
> >>
> >> How are we supposed to expose control over the hsync/vsync/data enable
> >> polarity? I only know that the display controller and the panel need to
> >> agree on a setting that works for both. What is the canonical way to do
> >> this?
> > 
> > So typically, it would come from the panel datasheet and would thus be
> > exposed by the panel driver. st7789v is not a panel itself but a (pretty
> > flexible) panel controller so it's not fixed and I don't think we have a
> > good answer to that (yet).
> 
> Then it seems to me that creating a panel driver (= st8879v panel
> controller driver with a new compatible) would make sense.

I don't see why? The entire controller is the same except (maybe) for
some initialization data. Doing a new driver for it seems like taking
the easy way out?

> By coincidence Sebastian Reichel has come up with this approach
> recently, see
> https://lore.kernel.org/dri-devel/20230317232355.1554980-1-sre@kernel.org/
> We just need a way to resolve the conflicts between the two series.
> 
> Cc: Sebastian

That's not a new driver though? That approach looks sane to me.

> >> A different question is the partial mode, for which (IIUC) you suggest
> >> the overscan feature. As I have never heard of this before, it would be
> >> very nice if you could point me to some examples. Where would the
> >> effective resolution be set in this case?
> > 
> > So, back when CRT were a thing the edges of the tube were masked by the
> > plastic case. HDMI inherited from that and that's why you still have
> > some UI on some devices (like consoles) to setup the active area of the
> > display.
> > 
> > The underlying issue is exactly what you describe: the active area is
> > larger than what the plastic case allows to see. I don't think anyone
> > ever had the usecase you have, but it would be the right solution to me
> > to solve essentially the same issue the same way we do on other output
> > types.
> 
> OK, we'll look into the overscan feature. But still the information
> about the active area should come from the driver, right?

No, the userspace is in charge there.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]