Re: [PATCH 4/4] drm/meson: convert to the new canvas module
2018-08-10 0:41 GMT+02:00 Rob Herring : > Hi, this is an automated email from Rob's (experimental) review bot. I > found a couple of common problems with your patch. Please see below. > > On Wed, 1 Aug 2018 20:51:28 +0200, Maxime Jourdan wrote: >> This removes the meson_canvas files within the meson/drm layer >> and makes use of the new canvas module that is referenced in the dts. >> >> Canvases can be used by different IPs and modules, and it is as such >> preferable to rely on a module that can safely dispatch canvases on >> demand. >> >> Signed-off-by: Maxime Jourdan > > The preferred subject prefix is "dt-bindings: : ...". > >> --- >> .../bindings/display/amlogic,meson-vpu.txt| 9 +-- >> arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 7 +- >> drivers/gpu/drm/meson/Kconfig | 1 + >> drivers/gpu/drm/meson/Makefile| 2 +- >> drivers/gpu/drm/meson/meson_canvas.c | 70 --- >> drivers/gpu/drm/meson/meson_canvas.h | 42 --- >> drivers/gpu/drm/meson/meson_crtc.c| 5 +- >> drivers/gpu/drm/meson/meson_drv.c | 35 ++ >> drivers/gpu/drm/meson/meson_drv.h | 5 +- >> drivers/gpu/drm/meson/meson_plane.c | 3 +- >> drivers/gpu/drm/meson/meson_viu.c | 1 - >> 11 files changed, 39 insertions(+), 141 deletions(-) >> delete mode 100644 drivers/gpu/drm/meson/meson_canvas.c >> delete mode 100644 drivers/gpu/drm/meson/meson_canvas.h >> > > DT bindings (including binding headers) should be a separate patch. See > Documentation/devicetree/bindings/submitting-patches.txt. > Hi, What's the standard procedure here ? The reason I kept devicetree+drm changes together is because I didn't want to have floating commits that would break the drm module. Should I split the changes anyway ? Maxime ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm/meson: convert to the new canvas module
On 10/08/2018 08:35, Maxime Jourdan wrote: > 2018-08-10 0:41 GMT+02:00 Rob Herring : >> Hi, this is an automated email from Rob's (experimental) review bot. I >> found a couple of common problems with your patch. Please see below. >> >> On Wed, 1 Aug 2018 20:51:28 +0200, Maxime Jourdan wrote: >>> This removes the meson_canvas files within the meson/drm layer >>> and makes use of the new canvas module that is referenced in the dts. >>> >>> Canvases can be used by different IPs and modules, and it is as such >>> preferable to rely on a module that can safely dispatch canvases on >>> demand. >>> >>> Signed-off-by: Maxime Jourdan >> >> The preferred subject prefix is "dt-bindings: : ...". >> >>> --- >>> .../bindings/display/amlogic,meson-vpu.txt| 9 +-- >>> arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 7 +- >>> drivers/gpu/drm/meson/Kconfig | 1 + >>> drivers/gpu/drm/meson/Makefile| 2 +- >>> drivers/gpu/drm/meson/meson_canvas.c | 70 --- >>> drivers/gpu/drm/meson/meson_canvas.h | 42 --- >>> drivers/gpu/drm/meson/meson_crtc.c| 5 +- >>> drivers/gpu/drm/meson/meson_drv.c | 35 ++ >>> drivers/gpu/drm/meson/meson_drv.h | 5 +- >>> drivers/gpu/drm/meson/meson_plane.c | 3 +- >>> drivers/gpu/drm/meson/meson_viu.c | 1 - >>> 11 files changed, 39 insertions(+), 141 deletions(-) >>> delete mode 100644 drivers/gpu/drm/meson/meson_canvas.c >>> delete mode 100644 drivers/gpu/drm/meson/meson_canvas.h >>> >> >> DT bindings (including binding headers) should be a separate patch. See >> Documentation/devicetree/bindings/submitting-patches.txt. >> > > Hi, What's the standard procedure here ? The reason I kept > devicetree+drm changes together is because I didn't want to have > floating commits that would break the drm module. > > Should I split the changes anyway ? > > Maxime > Yep, split the dt-bindings and the code change. In a non-perfect work, it could be great to keep the "old" canvas until everything is merged, except if Kevin let me merge everything (including DT) in drm-misc. Neil ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm/meson: convert to the new canvas module
Hi, this is an automated email from Rob's (experimental) review bot. I found a couple of common problems with your patch. Please see below. On Wed, 1 Aug 2018 20:51:28 +0200, Maxime Jourdan wrote: > This removes the meson_canvas files within the meson/drm layer > and makes use of the new canvas module that is referenced in the dts. > > Canvases can be used by different IPs and modules, and it is as such > preferable to rely on a module that can safely dispatch canvases on > demand. > > Signed-off-by: Maxime Jourdan The preferred subject prefix is "dt-bindings: : ...". > --- > .../bindings/display/amlogic,meson-vpu.txt| 9 +-- > arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 7 +- > drivers/gpu/drm/meson/Kconfig | 1 + > drivers/gpu/drm/meson/Makefile| 2 +- > drivers/gpu/drm/meson/meson_canvas.c | 70 --- > drivers/gpu/drm/meson/meson_canvas.h | 42 --- > drivers/gpu/drm/meson/meson_crtc.c| 5 +- > drivers/gpu/drm/meson/meson_drv.c | 35 ++ > drivers/gpu/drm/meson/meson_drv.h | 5 +- > drivers/gpu/drm/meson/meson_plane.c | 3 +- > drivers/gpu/drm/meson/meson_viu.c | 1 - > 11 files changed, 39 insertions(+), 141 deletions(-) > delete mode 100644 drivers/gpu/drm/meson/meson_canvas.c > delete mode 100644 drivers/gpu/drm/meson/meson_canvas.h > DT bindings (including binding headers) should be a separate patch. See Documentation/devicetree/bindings/submitting-patches.txt. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm/meson: convert to the new canvas module
Hi Jerome, 2018-08-02 10:39 GMT+02:00 Jerome Brunet : > I looks like the consumer of your 'canvas' devices must know how the canvas > device is organized internally. Maybe something better can be done ? > > Your canvas driver could provide a consumer API, for example: > meson_canvas_get(): to translate for struct device_node to whatever abstract > pointer you would need. > meson_canvas_alloc(), setup(), etc ... > > ... This is just adding a bit of indirection but it would help hide the > plumbing > of your canvas driver from the consumers (and repeat this code in each). This > might be usefull if you ever to make this canvas driver evolve. Overall the inner workings are hidden as there is an ops struct instead of public functions. I agree that the "fetch the node" boilerplate code could be put behind a helper, but at the same time this code helps remind the developer that there needs to be a canvas node in the dts, and that it has to be linked in your own device node. I would like to keep it that way if that is okay with you. ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm/meson: convert to the new canvas module
On Wed, 2018-08-01 at 20:51 +0200, Maxime Jourdan wrote:
> This removes the meson_canvas files within the meson/drm layer
> and makes use of the new canvas module that is referenced in the dts.
>
> Canvases can be used by different IPs and modules, and it is as such
> preferable to rely on a module that can safely dispatch canvases on
> demand.
>
> Signed-off-by: Maxime Jourdan
> ---
> .../bindings/display/amlogic,meson-vpu.txt| 9 +--
> arch/arm64/boot/dts/amlogic/meson-gx.dtsi | 7 +-
> drivers/gpu/drm/meson/Kconfig | 1 +
> drivers/gpu/drm/meson/Makefile| 2 +-
> drivers/gpu/drm/meson/meson_canvas.c | 70 ---
> drivers/gpu/drm/meson/meson_canvas.h | 42 ---
> drivers/gpu/drm/meson/meson_crtc.c| 5 +-
> drivers/gpu/drm/meson/meson_drv.c | 35 ++
> drivers/gpu/drm/meson/meson_drv.h | 5 +-
> drivers/gpu/drm/meson/meson_plane.c | 3 +-
> drivers/gpu/drm/meson/meson_viu.c | 1 -
> 11 files changed, 39 insertions(+), 141 deletions(-)
> delete mode 100644 drivers/gpu/drm/meson/meson_canvas.c
> delete mode 100644 drivers/gpu/drm/meson/meson_canvas.h
>
> diff --git a/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
> b/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
> index 057b81335775..60b6e1398636 100644
> --- a/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
> +++ b/Documentation/devicetree/bindings/display/amlogic,meson-vpu.txt
> @@ -60,9 +60,9 @@ Required properties:
> - reg: base address and size of he following memory-mapped regions :
> - vpu
> - hhi
> - - dmc
> - reg-names: should contain the names of the previous memory regions
> - interrupts: should contain the VENC Vsync interrupt number
> +- amlogic,canvas: should point to a meson canvas provider node
>
> Optional properties:
> - power-domains: Optional phandle to associated power domain as described in
> @@ -98,13 +98,14 @@ tv-connector {
> vpu: vpu@d010 {
> compatible = "amlogic,meson-gxbb-vpu";
> reg = <0x0 0xd010 0x0 0x10>,
> - <0x0 0xc883c000 0x0 0x1000>,
> - <0x0 0xc8838000 0x0 0x1000>;
> - reg-names = "vpu", "hhi", "dmc";
> + <0x0 0xc883c000 0x0 0x1000>;
> + reg-names = "vpu", "hhi";
> interrupts = ;
> #address-cells = <1>;
> #size-cells = <0>;
>
> + amlogic,canvas = <>;
> +
> /* CVBS VDAC output port */
> port@0 {
> reg = <0>;
> diff --git a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> index d104b9e111fb..7c4d971ecd80 100644
> --- a/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> +++ b/arch/arm64/boot/dts/amlogic/meson-gx.dtsi
> @@ -503,13 +503,14 @@
> vpu: vpu@d010 {
> compatible = "amlogic,meson-gx-vpu";
> reg = <0x0 0xd010 0x0 0x10>,
> - <0x0 0xc883c000 0x0 0x1000>,
> - <0x0 0xc8838000 0x0 0x1000>;
> - reg-names = "vpu", "hhi", "dmc";
> + <0x0 0xc883c000 0x0 0x1000>;
> + reg-names = "vpu", "hhi";
> interrupts = ;
> #address-cells = <1>;
> #size-cells = <0>;
>
> + amlogic,canvas = <>;
> +
> /* CVBS VDAC output port */
> cvbs_vdac_port: port@0 {
> reg = <0>;
> diff --git a/drivers/gpu/drm/meson/Kconfig b/drivers/gpu/drm/meson/Kconfig
> index 3ce51d8dfe1c..c28b69f48555 100644
> --- a/drivers/gpu/drm/meson/Kconfig
> +++ b/drivers/gpu/drm/meson/Kconfig
> @@ -7,6 +7,7 @@ config DRM_MESON
> select DRM_GEM_CMA_HELPER
> select VIDEOMODE_HELPERS
> select REGMAP_MMIO
> + select MESON_CANVAS
>
> config DRM_MESON_DW_HDMI
> tristate "HDMI Synopsys Controller support for Amlogic Meson Display"
> diff --git a/drivers/gpu/drm/meson/Makefile b/drivers/gpu/drm/meson/Makefile
> index c5c4cc362f02..bd67429185ff 100644
> --- a/drivers/gpu/drm/meson/Makefile
> +++ b/drivers/gpu/drm/meson/Makefile
> @@ -1,5 +1,5 @@
> meson-drm-y := meson_drv.o meson_plane.o meson_crtc.o meson_venc_cvbs.o
> -meson-drm-y += meson_viu.o meson_vpp.o meson_venc.o meson_vclk.o
> meson_canvas.o
> +meson-drm-y += meson_viu.o meson_vpp.o meson_venc.o meson_vclk.o
>
> obj-$(CONFIG_DRM_MESON) += meson-drm.o
> obj-$(CONFIG_DRM_MESON_DW_HDMI) += meson_dw_hdmi.o
> diff --git a/drivers/gpu/drm/meson/meson_canvas.c
> b/drivers/gpu/drm/meson/meson_canvas.c
> deleted file mode 100644
> index 08f6073d967e..
> --- a/drivers/gpu/drm/meson/meson_canvas.c
> +++ /dev/null
> @@ -1,70 +0,0 @@
> -/*
> - * Copyright (C) 2016 BayLibre, SAS
> - * Author: Neil Armstrong
> - * Copyright (C) 2015 Amlogic, Inc. All rights reserved.
> -
Re: [PATCH 4/4] drm/meson: convert to the new canvas module
2018-08-02 15:01 GMT+02:00 Jerome Brunet : > What I don't like is precisely that you need to expose this 'struct ops' to > the > consumer. I would prefer an API for this (it can be a 1:1 mapping). The canvas > should remain some abstract object you get from DT. > > IMO, this is the same as a reset, a syscon or whatever other phandle you get > from DT. The consumer should not have to 'care' how it works (how data are > organized), it should just use it. > > Whatever you need to do to deal with your canvas phandle should (IMO) reside > in > your soc/amlogic/meson-canvas.c, and not be spread in the consumers. > >> >> I agree that the "fetch the node" boilerplate code could be put behind >> a helper, but at the same time this code helps remind the developer >> that there needs to be a canvas node in the dts, and that it has to be >> linked in your own device node. > > This is why we have a DT documentation. > > And, as far as I understand amlogic's display and decoder stuff, you won't get > very far w/o a canvas, so 'the developer' will be reminded fairly quickly ;) > >> >> I would like to keep it that way if that is okay with you. > > It's just my opinion, I'm not the one merging it ... :P > > Fair enough, I'll see to API-ize the module in v2 :). ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH 4/4] drm/meson: convert to the new canvas module
On Thu, 2018-08-02 at 14:34 +0200, Maxime Jourdan wrote: > Hi Jerome, > > 2018-08-02 10:39 GMT+02:00 Jerome Brunet : > > I looks like the consumer of your 'canvas' devices must know how the canvas > > device is organized internally. Maybe something better can be done ? > > > > Your canvas driver could provide a consumer API, for example: > > meson_canvas_get(): to translate for struct device_node to whatever abstract > > pointer you would need. > > meson_canvas_alloc(), setup(), etc ... > > > > ... This is just adding a bit of indirection but it would help hide the > > plumbing > > of your canvas driver from the consumers (and repeat this code in each). > > This > > might be usefull if you ever to make this canvas driver evolve. > > Overall the inner workings are hidden as there is an ops struct > instead of public functions. What I don't like is precisely that you need to expose this 'struct ops' to the consumer. I would prefer an API for this (it can be a 1:1 mapping). The canvas should remain some abstract object you get from DT. IMO, this is the same as a reset, a syscon or whatever other phandle you get from DT. The consumer should not have to 'care' how it works (how data are organized), it should just use it. Whatever you need to do to deal with your canvas phandle should (IMO) reside in your soc/amlogic/meson-canvas.c, and not be spread in the consumers. > > I agree that the "fetch the node" boilerplate code could be put behind > a helper, but at the same time this code helps remind the developer > that there needs to be a canvas node in the dts, and that it has to be > linked in your own device node. This is why we have a DT documentation. And, as far as I understand amlogic's display and decoder stuff, you won't get very far w/o a canvas, so 'the developer' will be reminded fairly quickly ;) > > I would like to keep it that way if that is okay with you. It's just my opinion, I'm not the one merging it ... :P ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
