Re: [PATCH 1/2] dt-bindings: display: Add xylon logicvc bindings documentation

[Rob Herring]

On Fri, Sep 13, 2019 at 4:58 PM Paul Kocialkowski
<paul.kocialkowski@xxxxxxxxxxx> wrote:
>
> Hi Rob and thanks for the review!
>
> On Fri 13 Sep 19, 15:35, Rob Herring wrote:
> > On Tue, Sep 10, 2019 at 05:34:08PM +0200, Paul Kocialkowski wrote:
> > > The Xylon LogiCVC is a display controller implemented as programmable
> > > logic in Xilinx FPGAs.
> > >
> > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@xxxxxxxxxxx>
> > > ---
> > >  .../bindings/display/xylon,logicvc.txt        | 188 ++++++++++++++++++
> > >  1 file changed, 188 insertions(+)
> > >  create mode 100644 Documentation/devicetree/bindings/display/xylon,logicvc.txt
> >
> > Consider converting this to DT schema format. See
> > Documentation/devicetree/writing-schema.rst (.md in 5.3).
>
> Oh right, that would certainly be much more future-proof!
>
> > > diff --git a/Documentation/devicetree/bindings/display/xylon,logicvc.txt b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > new file mode 100644
> > > index 000000000000..eb4b1553888a
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/display/xylon,logicvc.txt
> > > @@ -0,0 +1,188 @@
> > > +Xylon LogiCVC display controller
> > > +
> > > +The Xylon LogiCVC is a display controller that supports multiple layers.
> > > +It is usually implemented as programmable logic and was optimized for use
> > > +with Xilinx Zynq-7000 SoCs and Xilinx FPGAs.
> > > +
> > > +Because the controller is intended for use in a FPGA, most of the configuration
> > > +of the controller takes place at logic configuration bitstream synthesis time.
> > > +As a result, many of the device-tree bindings are meant to reflect the
> > > +synthesis configuration. These do not allow configuring the controller
> > > +differently than synthesis configuration.
> > > +
> > > +Layers are declared in the "layers" sub-node and have dedicated configuration.
> > > +In version 3 of the controller, each layer has fixed memory offset and address
> > > +starting from the video memory base address for its framebuffer. With version 4,
> > > +framebuffers are configured with a direct memory address instead.
> > > +
> > > +Matching synthesis parameters are provided when applicable.
> > > +
> > > +Required properties:
> > > +- compatible: Should be one of:
> > > +  "xylon,logicvc-3.02.a-display"
> > > +  "xylon,logicvc-4.01.a-display"
> > > +- reg: Physical base address and size for the controller registers.
> > > +- clocks: List of phandle and clock-specifier pairs, one for each entry
> > > +  in 'clock-names'
> > > +- clock-names: List of clock names that should at least contain:
> > > +  - "vclk": The VCLK video clock input.
> > > +- interrupts: The interrupt to use for VBLANK signaling.
> > > +- xylon,display-interface: Display interface in use, should be one of:
> > > +  - "lvds-4bits": 4-bit LVDS interface (C_DISPLAY_INTERFACE == 4).
> > > +- xylon,display-colorspace: Display output colorspace in use, should be one of:
> > > +  - "rgb": RGB colorspace (C_DISPLAY_COLOR_SPACE == 0).
> > > +- xylon,display-depth: Display output depth in use (C_PIXEL_DATA_WIDTH).
> > > +- xylon,row-stride: Fixed number of pixels in a framebuffer row (C_ROW_STRIDE).
> > > +- xylon,layers-count: The number of available layers (C_NUM_OF_LAYERS).
> >
> > Presumably some of this is determined by the display attached. Isn't it
> > safe to assume the IP was configured correctly for the intended display
> > and you can just get this from the panel?
>
> Layers are what corresponds to DRM planes, which are not actually indicated
> by the panel but are a charasteristic of the display controller. In our case,
> this is directly selected at bitstream synthesis time for the controller.
>
> So I'm afraid there is no way we can auto-detect this from the driver.

Sorry, I referring to the set of properties above. In particular,
xylon,display-interface and xylon,display-colorspace, though I don't
know if the latter is talking in memory format or on the wire format.

Actually for xylon,layers-count, You should just count the child nodes
of 'layers'.

> > > +Optional properties:
> > > +- memory-region: phandle to a node describing memory, as specified in:
> > > +  Documentation/devicetree/bindings/reserved-memory/reserved-memory.txt
> > > +- clock-names: List of clock names that can optionally contain:
> > > +  - "vclk2": The VCLK2 doubled-rate video clock input.
> > > +  - "lvdsclk": The LVDS clock.
> > > +  - "lvdsclkn": The LVDS clock inverted.
> >
> > How are these really optional?
>
> Well, the controller currently only supports LVDS, but more interfaces may be
> added later, so the lvdsclk clock will be optional when another interface
> is used instead. Maybe I'm mistaken about how to categorize them though.
>
> My understanding is that the need for vclk2 and lvdsclkn depend on the target
> FPGA family. I've developped the driver without the need for them, but the
> datasheet states that they may be needed (but doesn't provide significant
> details about their role though).

Not sure what to tell you then. You'll see it becomes a bit messy to
describe in schema. Ideally we define the exact number, order, and
values possible (or sets of those).

> > > +- xylon,syscon: Syscon phandle representing the logicvc instance.
> > > +- xylon,dithering: Dithering module is enabled (C_XCOLOR).
> > > +- xylon,background-layer: The last layer is used to display a black background
> > > +  (C_USE_BACKGROUND). It must still be registered.
> > > +- xylon,layers-configurable: Configuration of layers' size, position and offset
> > > +  is enabled (C_USE_SIZE_POSITION).
> >
> > I would think this will effectively have to be enabled to make this
> > usable with DRM. I'm not sure if a "standard" userspace would use any of
> > the layers if all this is fixed.
>
> I was going with the same assumption, but drm_atomic_helper_check_plane_state
> has a can_position parameter, which will check that the plane covers the
> whole CRTC if set to false. So I guess it is somewhat expected that this can
> be the case and some drivers (e.g. arm/hdlcd_crtc.c) also set this to false.

Certainly atomic can fail on anything not supported. My question is
more whether userspace has some minimum requirements. A cursor
couldn't deal with can_position=false for example.

> I guess this falls under a more generic discussion of what should be expected
> by userspace when it comes to DRM. Since drivers may reject commits because of
> any hardware-specific limitation, there is definitely a significant grey area
> there and apparently no common rule.
>
> > > +
> > > +Required sub-nodes:
> > > +- layers: The description of the display controller layers, containing layer
> > > +  sub-nodes that each describe a registered layer.
> > > +- ports: The LogiCVC connection to an encoder input port. The connection
> > > +  is modeled using the OF graph bindings, as specified in:
> > > +  Documentation/devicetree/bindings/graph.txt
> > > +
> > > +Required layer properties:
> > > +- reg: Layer index (from front to back, starting at 0).
> > > +- xylon,layer-depth: Layer depth in use (C_LAYER_0_DATA_WIDTH).
> > > +- xylon,layer-colorspace: Layer colorspace in use, should be one of:
> > > + - "rgb": RGB colorspace (C_LAYER_*_TYPE == 0).
> >
> > Why is this needed if there's only 1?
>
> The hardware supports more but support is no implemented yet, so the binding
> document may be enriched along with the driver in the future.
>
> Should I describe other colorspaces even if they are not currently supported?

Document what the h/w supports to the extent you can. Then we can make
better decisions.

> > > +- xylon,layer-alpha-mode: Alpha mode for the layer, should be one of:
> > > + - "layer": Alpha is configured layer-wide (C_LAYER_*_ALPHA_MODE == 0).
> > > + - "pixel": Alpha is configured per-pixel (C_LAYER_*_ALPHA_MODE == 1).
> >
> > Could just be boolean.
>
> In this instance too, there are other modes that are not yet implemented but
> supported by the hardware, so I did not describe them yet but they may be added
> later.
>
> > > +- xylon,layer-base-offset: offset in number of lines (C_LAYER_*_OFFSET) starting
> > > +  from the video RAM base (C_VMEM_BASEADDR), only for version 3.
> > > +- xylon,layer-buffer-offset: offset in number of lines (C_BUFFER_*_OFFSET)
> > > +  starting from the layer base offset for the second buffer used in
> > > +  double-buffering.
> >
> > It might be better to define all this in terms of byte offsets. I'd
> > guess that is what CPU accesses are going to need.
>
> I agree that it is more convenient from a driver's perspective, but the
> rationale is that this allows copying parameters directly from the synthesis
> configuration file, where these are expressed as a number of lines.
>
> I would like to keep both delcarations as close to eachother as possible, as to
> facilitate integration work for future users of the driver. But maybe this is a
> bit too much in that case. What do you think?

Fair enough. I'd feel differently if I thought this would be common,
but this seems to be a pretty specific usecase. I guess there could be
other fixed at synthesis h/w.

Rob