by Conor Dooley

[permalink] [raw]

Subject: Re: [PATCH 1/9] dt-bindings: display: Add yamls for JH7110 display subsystem

Hey Keith,

On Fri, Jun 02, 2023 at 03:40:35PM +0800, Keith Zhao wrote:
> Add bindings for JH7110 display subsystem which
> has a display controller verisilicon dc8200
> and an HDMI interface.
>
> Signed-off-by: Keith Zhao <[email protected]>
> ---
> .../display/verisilicon/starfive-hdmi.yaml | 93 +++++++++++++++
> .../display/verisilicon/verisilicon-dc.yaml | 110 ++++++++++++++++++
> .../display/verisilicon/verisilicon-drm.yaml | 42 +++++++
> .../devicetree/bindings/vendor-prefixes.yaml | 2 +
> MAINTAINERS | 7 ++
> 5 files changed, 254 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/display/verisilicon/starfive-hdmi.yaml
> create mode 100644 Documentation/devicetree/bindings/display/verisilicon/verisilicon-dc.yaml
> create mode 100644 Documentation/devicetree/bindings/display/verisilicon/verisilicon-drm.yaml
>
> diff --git a/Documentation/devicetree/bindings/display/verisilicon/starfive-hdmi.yaml b/Documentation/devicetree/bindings/display/verisilicon/starfive-hdmi.yaml
> new file mode 100644
> index 000000000000..c30b7954a355
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/verisilicon/starfive-hdmi.yaml
> @@ -0,0 +1,93 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/verisilicon/starfive-hdmi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive HDMI transmiter
> +
> +description:
> + The StarFive SoC uses the HDMI signal transmiter based on innosilicon IP

Is innosilicon the same thing as verisilicon? Also
s/transmiter/transmitter/, both here and in the title.

> + to generate HDMI signal from its input and transmit the signal to the screen.
> +
> +maintainers:
> + - Keith Zhao <[email protected]>
> + - ShengYang Chen <[email protected]>
> +
> +properties:
> + compatible:
> + const: starfive,hdmi

Is this going to work on every SoC that StarFive has ever & will ever
make? Please use soc-based compatibles ;)

> +
> + reg:
> + minItems: 1
> +
> + interrupts:
> + items:
> + - description: The HDMI hot plug detection interrupt.
> +
> + clocks:
> + items:
> + - description: System clock of HDMI module.
> + - description: Mclk clock of HDMI audio.
> + - description: Bclk clock of HDMI audio.
> + - description: Pixel clock generated by HDMI module.
> +
> + clock-names:
> + items:
> + - const: sysclk
> + - const: mclk
> + - const: bclk
> + - const: pclk
> +
> + resets:
> + items:
> + - description: Reset for HDMI module.
> +
> + reset-names:
> + items:
> + - const: hdmi_tx

You only have one item here, you don't need the "items: - const:",
"const:" alone will do.

> diff --git a/Documentation/devicetree/bindings/display/verisilicon/verisilicon-dc.yaml b/Documentation/devicetree/bindings/display/verisilicon/verisilicon-dc.yaml
> new file mode 100644
> index 000000000000..1322502c4cde
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/verisilicon/verisilicon-dc.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/verisilicon/verisilicon-dc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: StarFive display controller
> +
> +description:
> + The StarFive SoC uses the display controller based on Verisilicon IP
> + to transfer the image data from a video memory
> + buffer to an external LCD interface.

Is it based on Verisilicon IP, or is it exactly that verisilicon IP? I
ask because...

> +maintainers:
> + - Keith Zhao <[email protected]>
> + - ShengYang Chen <[email protected]>
> +
> +properties:
> + compatible:
> + const: verisilicon,dc8200

...the compatible is the verisilicon IP. I would be a lot happier if
the compatibles were set yp for something like:
"starfive,jh7110-foo", "verisilicon,dc8200"

> diff --git a/Documentation/devicetree/bindings/display/verisilicon/verisilicon-drm.yaml b/Documentation/devicetree/bindings/display/verisilicon/verisilicon-drm.yaml
> new file mode 100644
> index 000000000000..aed8d4af2c55
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/verisilicon/verisilicon-drm.yaml
> @@ -0,0 +1,42 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/verisilicon/verisilicon-drm.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Verisilicon DRM master device
> +
> +maintainers:
> + - Keith Zhao <[email protected]>
> + - ShengYang Chen <[email protected]>
> +
> +description: |
> + The Verisilicon DRM master device is a virtual device needed to list all
> + display controller or other display interface nodes that comprise the
> + graphics subsystem.
> +
> +properties:
> + compatible:
> + const: verisilicon,display-subsystem

Same here.

> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 82d39ab0231b..52c04fd098be 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1436,6 +1436,8 @@ patternProperties:
> description: Variscite Ltd.
> "^vdl,.*":
> description: Van der Laan b.v.
> + "^verisilicon,.*":
> + description: Verisilicon Technologies, Inc.

This should be in it's own patch.

Cheers,
Conor.

> "^vertexcom,.*":
> description: Vertexcom Technologies, Inc.
> "^via,.*":
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 2a0496448b7f..293aa13d484c 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -7049,6 +7049,13 @@ F: Documentation/devicetree/bindings/display/brcm,bcm2835-*.yaml
> F: drivers/gpu/drm/vc4/
> F: include/uapi/drm/vc4_drm.h
>
> +DRM DRIVERS FOR VERISILICON
> +M: Keith Zhao <[email protected]>
> +L: [email protected]
> +S: Maintained
> +T: git git://anongit.freedesktop.org/drm/drm-misc
> +F: Documentation/devicetree/bindings/display/verisilicon/
> +
> DRM DRIVERS FOR VIVANTE GPU IP
> M: Lucas Stach <[email protected]>
> R: Russell King <[email protected]>
> --
> 2.34.1
>

Attachments:

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

2023-06-06 19:06:25

On Wed, Jun 07, 2023 at 12:22:33AM +0200, Heiko St?bner wrote:
> Am Dienstag, 6. Juni 2023, 20:41:17 CEST schrieb Shengyu Qu:
> > > On Fri, Jun 02, 2023 at 03:40:35PM +0800, Keith Zhao wrote:
> > >> Add bindings for JH7110 display subsystem which
> > >> has a display controller verisilicon dc8200
> > >> and an HDMI interface.

> > >> +description:
> > >> + The StarFive SoC uses the HDMI signal transmiter based on innosilicon IP
> > > Is innosilicon the same thing as verisilicon? Also
> > > s/transmiter/transmitter/, both here and in the title.
> >
> > I think that is not the same, I remember Rockchip has used a HDMI
> > transmitter from
> >
> > Innosilicon, and there is a existing driver for that in mainline.
>
> Yep, I think Innosilicon is the company you turn to when you want to save
> a bit of money ;-) . In the bigger SoCs Rockchip most of the time uses
> Designware hdmi blocks and looking at the history only the rk3036 ever
> used an Innosilicon block.
>
> Looking at the history, 2016 really was a long time ago :-D.
>
> > So Keith, if that's true, I think it is better to seperate the HDMI
> > stuff and reuse existing driver.
>
> I'm not so sure about that - at least from a cursory glance :-) .
>
> The registers do look slightly different and I don't know how much
> the IP changed between the rk3036-version and the jh7110 version.
>
> At the very least, I know my rk3036 board isn't booting right now, so
> I can't really provide help for generalizing the rockchip-driver.
>
> At the very least both the binding and driver could drop the "starfive-hdmi"
> and actually use the Innosilicon in the naming somewhere, so that it's
> clear for future developers :-)

Seeing "based on" always makes me a little bit nervous to be honest when
it comes to using a compatible from the IP. Is it the IP? What version
is it? etc. Perhaps "starfive,jh7110-hdmi" & falling back to some sort
of "innosilicon,hdmi" would be more future/IP-silliness proof.
Driver can always be generic & bind against "innosilicon,hdmi" until
that becomes impossible.

Cheers,
Conor.

Attachments:

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

2023-06-07 07:25:50

Hi Keith,
On Fri, Jul 21, 2023 at 07:57:24PM +0800, Keith Zhao wrote:
> >> +
> >> +struct vs_crtc_funcs {
> >> +??? void (*enable)(struct device *dev, struct drm_crtc *crtc);
> >> +??? void (*disable)(struct device *dev, struct drm_crtc *crtc);
> >> +??? bool (*mode_fixup)(struct device *dev,
> >> +?????????????? const struct drm_display_mode *mode,
> >> +?????????????? struct drm_display_mode *adjusted_mode);
> >> +??? void (*set_gamma)(struct device *dev, struct drm_crtc *crtc,
> >> +????????????? struct drm_color_lut *lut, unsigned int size);
> >> +??? void (*enable_gamma)(struct device *dev, struct drm_crtc *crtc,
> >> +???????????????? bool enable);
> >> +??? void (*enable_vblank)(struct device *dev, bool enable);
> >> +??? void (*commit)(struct device *dev);
> >> +};
> >
> > Why is this here? You are reproducing our interface with an internal interface. I know where this leads to: you have multiple chipset revisions and each has its own implemenation of these internal interfaces.
> >
> > That will absolutely come back to haunt you in the long rung: the more chip revisions you support, the more obscure these internal interfaces and implentations become. And you won't be able to change these callbacks, as that affects all revisions. We've seen this with a few drivers. It will become unmaintainable.
> >
> > A better approach is to treat DRM's atomic callback funcs and atomic helper funcs as your interface for each chip revision. So for each model, you implement a separate modesetting pipeline. When you add a new chip revision, you copy the previous chip's code into a new file and adopt it. If you find comon code among individual revisions, you can put it into a shared helper.? With this design, each chip revision stands on its own.
> >
> > I suggest to study the mgag200 driver. It detects the chip revision very early and builds a chip-specific modesetting pipline. Although each chip is handled separately, a lot of shared code is in helpers. So the size of the driver remains small.
> >
> hi Thomas:
> I'm trying to understand what you're thinking

I am not Thomas, but let me try to put a few words on this.

> 1. Different chip ids should have their own independent drm_dev, and should not be supported based on a same drm_dev.
Yes, this part is correct understood.

> 2. diff chip id , for example dc8200 , dc9000,
>
> struct vs_crtc_funcs {
> void (*enable)(struct device *dev, struct drm_crtc *crtc);
> void (*disable)(struct device *dev, struct drm_crtc *crtc);
> bool (*mode_fixup)(struct device *dev,
> const struct drm_display_mode *mode,
> struct drm_display_mode *adjusted_mode);
> void (*set_gamma)(struct device *dev, struct drm_crtc *crtc,
> struct drm_color_lut *lut, unsigned int size);
> void (*enable_gamma)(struct device *dev, struct drm_crtc *crtc,
> bool enable);
> void (*enable_vblank)(struct device *dev, bool enable);
> void (*commit)(struct device *dev);
> };
No - the idea is that you populate crtc_funcs direct.
Drop struct vs_crtc_funcs - just fill out your own crtc_funcs structure.

If it turns out that most of the crtc operations are the same then share
them. Avoid the extra layer of indirection that you have with struct vs_crtc_funcs
as this is not needed when you use the pattern described by Thomas.

> static const struct vs_crtc_funcs vs_dc8200_crtc_funcs = {...}
> static const struct vs_crtc_funcs vs_dc9200_crtc_funcs = {...}
>
> struct vs_drm_private {
> struct drm_device base;
> struct device *dma_dev;
> struct iommu_domain *domain;
> unsigned int pitch_alignment;

This parts looks fine.

>
> const struct vs_crtc_funcs *funcs;
No, here you need a pointer to struct crtc_funcs or a struct that embeds
crtc_funcs.
> };

If you, after reading this, thinks you need struct vs_crtc_funcs, then
try to take an extra look at mgag200. It is not needed.

I hope this helps.

Sam