Reply: Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
Hi Daniel,
I'm honored to have your attention，and thanks for your advice.
I will finish the next version and submit it as soon as possible.
+
+ lcrtc-ldev = ldev;
+ lcrtc-reg_offset = index * REG_OFFSET;
+ lcrtc-cfg_reg = CFG_RESET;
+ lcrtc-crtc_id = index;
+
+ ret = loongson_plane_init(lcrtc);
+ if (ret)
+ goto free_lcrtc;
+
+ ret = drm_crtc_init_with_planes(ldev-dev,
lcrtc-base, lcrtc-plane,
+ NULL, loongson_crtc_funcs,
NULL);
Please use the drmm_crtc version here and don't kzalloc yourself. That
simplifies the cleanup (since atm you're just leaking this memory).
Also loongson hw looks a like like something which should use the simple
kms helpers since you're embedding the single plane into your crtc struct:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms-helpers.html#simple-kms-helper-reference
Also at that point your driver is probabyl small enough that single source
file is the right thing, and you should move it into drivers/gpu/drm/tiny.
It would be the first simple/tiny drm driver with 2 outputs.
Rule of thumb we have is that if it's below 1kloc, a single file and
putting it into drm/tiny is best.
Cheers, Daniel
I will try use the drmm_crtcdrmm_encoder version, and consider use simple
kms helpers.
But I'm not going to put it in drivers/gpu/drm/tiny, we still have some features
to work on and will support new graphics cards in the future.
+ return 0;
+}
+
+static int loongson_drm_load(struct drm_device *dev)
+{
+ struct loongson_device *ldev;
+ int ret;
+
+ ldev = devm_kzalloc(dev-dev, sizeof(*ldev), GFP_KERNEL);
Please don't use devm_kzalloc here, but instead embedde the struct
drm_device into your struct looongson_device.
-Daniel
This has been changed and will be submitted in the next version.
--
LiChenyang
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Reply: Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
Hi Sam, Thank you very much for your attention to our driver. I will consider your opinion, The next version is due in the near future. -Original Messages- From: "Sam Ravnborg" Sent Time: 2021-07-28 01:41:30 (Wednesday) To: "Daniel Vetter" Subject: Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip Hi Chenyang, I browsed the code on lore and noticed a few things and thought it better to bring it to your attention now. The general structure of the drivers seems good and coding style is fine. The feedback is mostly stuff we have decided to do different over time, so likely because you based the driver on some older driver. And it can be hard to follow all the refactoring going on - something that you get for free (almost) when is is mainlined. 1) Use drm_ for logging whereever possible (need drm_device) 2) Do not use irq mid-layer support in drm_driver, it is about to be legacy only. In other words avoid using drm_irq* stuff. 3) Look at drm_drv to see code snippet how to use the drmm* infrastructure. It will save you some cleanup and in general make the driver more stable 4) Sort includes alphabetically, and split them on in blocks. is one block is next block 5) Put entry in makefile in alphabetical order 6) You most like can use the simple_encoder stuff we have today 7) The *_load and *_unlod names where used in the past. Maybe be inspired by some newer driver here. _load functiosn is something used by legacy drivers so it confuses me a little. I look forward to see next revision of the patch-set. And sorry for not providing these high-level feedback issues before - I have not had time to look at your driver. Sam -- LiChenyang ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
Hi Chenyang, I browsed the code on lore and noticed a few things and thought it better to bring it to your attention now. The general structure of the drivers seems good and coding style is fine. The feedback is mostly stuff we have decided to do different over time, so likely because you based the driver on some older driver. And it can be hard to follow all the refactoring going on - something that you get for free (almost) when is is mainlined. 1) Use drm_ for logging whereever possible (need drm_device) 2) Do not use irq mid-layer support in drm_driver, it is about to be legacy only. In other words avoid using drm_irq* stuff. 3) Look at drm_drv to see code snippet how to use the drmm* infrastructure. It will save you some cleanup and in general make the driver more stable 4) Sort includes alphabetically, and split them on in blocks. is one block is next block 5) Put entry in makefile in alphabetical order 6) You most like can use the simple_encoder stuff we have today 7) The *_load and *_unlod names where used in the past. Maybe be inspired by some newer driver here. _load functiosn is something used by legacy drivers so it confuses me a little. I look forward to see next revision of the patch-set. And sorry for not providing these high-level feedback issues before - I have not had time to look at your driver. Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
On Fri, Jul 23, 2021 at 07:22:46PM +0200, Sam Ravnborg wrote: > On Fri, Jul 23, 2021 at 10:57:56AM +0200, Daniel Vetter wrote: > > On Fri, Jul 23, 2021 at 11:12:49AM +0800, lichenyang wrote: > > > From: Chenyang Li > > > > > > This patch adds an initial DRM driver for the Loongson LS7A1000 > > > bridge chip(LS7A). The LS7A bridge chip contains two display > > > controllers, support dual display output. The maximum support for > > > each channel display is to 1920x1080@60Hz. > > > At present, DC device detection and DRM driver registration are > > > completed, the crtc/plane/encoder/connector objects has been > > > implemented. > > > On Loongson 3A4000 CPU and 7A1000 system, we have achieved the use > > > of dual screen, and support dual screen clone mode and expansion > > > mode. > > > > > > v9: > > > - Optimize the error handling process. > > > - Remove the useless flags parameter. > > > - Fix some incorrect use of variables and constructs. > > > > ... > > > > Somehow this simple driver is at v9 and still not landed. Do you have > > someone from the drm maintainers/committers who's guiding you through all > > this? I'm not seeing you Cc: a specific person, without that there's good > > chances your contribution gets lost. I'm swamped myself, which is why I've > > ignored this and hope you'd fine someone else and stick to them. > > Hi Chenyang, > > Please cc: me on the next revision - then I will take a look. > But I count on someone more familiar with atomic modesetting can also > take a look. Thomas? Maxime? I think the main thing is deciding whether this should use simple display pipe helpers or not. Right now it looks like, and it would greatly trim down the code. The only other things I check is whether there's any hooks that can be removed and whether an attempt to send out the event is made in all cases, and that's good enough for me on the atomic front :-) If it's not using simple display pipe helpers there's a bunch more things to check, like whether the plane check function looks competent or fails to reinvent what simple pipe helpers have. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
On Fri, Jul 23, 2021 at 10:57:56AM +0200, Daniel Vetter wrote: > On Fri, Jul 23, 2021 at 11:12:49AM +0800, lichenyang wrote: > > From: Chenyang Li > > > > This patch adds an initial DRM driver for the Loongson LS7A1000 > > bridge chip(LS7A). The LS7A bridge chip contains two display > > controllers, support dual display output. The maximum support for > > each channel display is to 1920x1080@60Hz. > > At present, DC device detection and DRM driver registration are > > completed, the crtc/plane/encoder/connector objects has been > > implemented. > > On Loongson 3A4000 CPU and 7A1000 system, we have achieved the use > > of dual screen, and support dual screen clone mode and expansion > > mode. > > > > v9: > > - Optimize the error handling process. > > - Remove the useless flags parameter. > > - Fix some incorrect use of variables and constructs. > > ... > > Somehow this simple driver is at v9 and still not landed. Do you have > someone from the drm maintainers/committers who's guiding you through all > this? I'm not seeing you Cc: a specific person, without that there's good > chances your contribution gets lost. I'm swamped myself, which is why I've > ignored this and hope you'd fine someone else and stick to them. Hi Chenyang, Please cc: me on the next revision - then I will take a look. But I count on someone more familiar with atomic modesetting can also take a look. Thomas? Maxime? Sam ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
On Fri, Jul 23, 2021 at 11:12:49AM +0800, lichenyang wrote: > From: Chenyang Li > > This patch adds an initial DRM driver for the Loongson LS7A1000 > bridge chip(LS7A). The LS7A bridge chip contains two display > controllers, support dual display output. The maximum support for > each channel display is to 1920x1080@60Hz. > At present, DC device detection and DRM driver registration are > completed, the crtc/plane/encoder/connector objects has been > implemented. > On Loongson 3A4000 CPU and 7A1000 system, we have achieved the use > of dual screen, and support dual screen clone mode and expansion > mode. > > v9: > - Optimize the error handling process. > - Remove the useless flags parameter. > - Fix some incorrect use of variables and constructs. > > v8: > - Update the atomic_update function interface. > > v7: > - The pixel clock is limited to less than 173000. > > v6: > - Remove spin_lock in mmio reg read and write. > - TO_UNCAC is replac with ioremap. > - Fix error arguments in crtc_atomic_enable/disable/mode_valid. > > v5: > - Change the name of the chip to LS7A. > - Change magic value in crtc to macros. > - Correct mistakes words. > - Change the register operation function prefix to ls7a. > > v4: > - Move the mode_valid function to the crtc. > > v3: > - Move the mode_valid function to the connector and optimize it. > - Fix num_crtc calculation method. > > v2: > - Complete the case of 32-bit color in CRTC. > > Signed-off-by: Chenyang Li Somehow this simple driver is at v9 and still not landed. Do you have someone from the drm maintainers/committers who's guiding you through all this? I'm not seeing you Cc: a specific person, without that there's good chances your contribution gets lost. I'm swamped myself, which is why I've ignored this and hope you'd fine someone else and stick to them. A few comments below. > --- > drivers/gpu/drm/Kconfig | 2 + > drivers/gpu/drm/Makefile | 1 + > drivers/gpu/drm/loongson/Kconfig | 14 + > drivers/gpu/drm/loongson/Makefile | 14 + > drivers/gpu/drm/loongson/loongson_connector.c | 46 +++ > drivers/gpu/drm/loongson/loongson_crtc.c | 249 > drivers/gpu/drm/loongson/loongson_device.c| 35 +++ > drivers/gpu/drm/loongson/loongson_drv.c | 278 ++ > drivers/gpu/drm/loongson/loongson_drv.h | 140 + > drivers/gpu/drm/loongson/loongson_encoder.c | 37 +++ > drivers/gpu/drm/loongson/loongson_plane.c | 97 ++ > 11 files changed, 913 insertions(+) > create mode 100644 drivers/gpu/drm/loongson/Kconfig > create mode 100644 drivers/gpu/drm/loongson/Makefile > create mode 100644 drivers/gpu/drm/loongson/loongson_connector.c > create mode 100644 drivers/gpu/drm/loongson/loongson_crtc.c > create mode 100644 drivers/gpu/drm/loongson/loongson_device.c > create mode 100644 drivers/gpu/drm/loongson/loongson_drv.c > create mode 100644 drivers/gpu/drm/loongson/loongson_drv.h > create mode 100644 drivers/gpu/drm/loongson/loongson_encoder.c > create mode 100644 drivers/gpu/drm/loongson/loongson_plane.c > > diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig > index 7ff89690a976..08562d9be6e3 100644 > --- a/drivers/gpu/drm/Kconfig > +++ b/drivers/gpu/drm/Kconfig > @@ -365,6 +365,8 @@ source "drivers/gpu/drm/xen/Kconfig" > > source "drivers/gpu/drm/vboxvideo/Kconfig" > > +source "drivers/gpu/drm/loongson/Kconfig" > + > source "drivers/gpu/drm/lima/Kconfig" > > source "drivers/gpu/drm/panfrost/Kconfig" > diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile > index a118692a6df7..29c05b8cf2ad 100644 > --- a/drivers/gpu/drm/Makefile > +++ b/drivers/gpu/drm/Makefile > @@ -119,6 +119,7 @@ obj-$(CONFIG_DRM_PL111) += pl111/ > obj-$(CONFIG_DRM_TVE200) += tve200/ > obj-$(CONFIG_DRM_XEN) += xen/ > obj-$(CONFIG_DRM_VBOXVIDEO) += vboxvideo/ > +obj-$(CONFIG_DRM_LOONGSON) += loongson/ > obj-$(CONFIG_DRM_LIMA) += lima/ > obj-$(CONFIG_DRM_PANFROST) += panfrost/ > obj-$(CONFIG_DRM_ASPEED_GFX) += aspeed/ > diff --git a/drivers/gpu/drm/loongson/Kconfig > b/drivers/gpu/drm/loongson/Kconfig > new file mode 100644 > index ..3cf42a4cca08 > --- /dev/null > +++ b/drivers/gpu/drm/loongson/Kconfig > @@ -0,0 +1,14 @@ > +# SPDX-License-Identifier: GPL-2.0-only > + > +config DRM_LOONGSON > + tristate "DRM support for LS7A bridge chipset" > + depends on DRM && PCI > + depends on CPU_LOONGSON64 > + select DRM_KMS_HELPER > + select DRM_VRAM_HELPER > + select DRM_TTM > + select DRM_TTM_HELPER > + default n > + help > + Support the display controllers found on the Loongson LS7A > + bridge. > diff --git a/drivers/gpu/drm/loongson/Makefile > b/drivers/gpu/drm/loongson/Makefile > new file mode 100644 > index ..22d063953b78 > --- /dev/null > +++ b/drivers/gpu/drm/loongson/Makefile > @@ -0,0 +1,14 @@ > +# SPDX-License-Identifier: GPL-2.0-only > +# > +#
Re: [PATCH v3 1/3] drm/loongson: Add DRM Driver for Loongson 7A1000 bridge chip
On Fri, Jul 23, 2021 at 11:12:49AM +0800, lichenyang wrote:
> +static int loongson_drm_load(struct drm_device *dev)
> +{
> + struct loongson_device *ldev;
> + int ret;
> +
> + ldev = devm_kzalloc(dev->dev, sizeof(*ldev), GFP_KERNEL);
> + if (!ldev)
> + return -ENOMEM;
> +
> + dev->dev_private = ldev;
> + ldev->dev = dev;
> +
> + ret = loongson_device_init(dev);
> + if (ret)
> + goto err;
> +
> + ret = drmm_vram_helper_init(dev, ldev->vram_start, ldev->vram_size);
> + if (ret) {
> + drm_err(dev, "Error initializing vram %d\n", ret);
> + goto err;
> + }
> +
> + drm_mode_config_init(dev);
> + dev->mode_config.funcs = (void *)_mode_funcs;
> + dev->mode_config.min_width = 1;
> + dev->mode_config.min_height = 1;
> + dev->mode_config.max_width = 4096;
> + dev->mode_config.max_height = 4096;
> + dev->mode_config.preferred_depth = 32;
> + dev->mode_config.prefer_shadow = 1;
> + dev->mode_config.fb_base = ldev->vram_start;
> +
> + ret = loongson_modeset_init(ldev);
> + if (ret) {
> + drm_err(dev, "Fatal error during modeset init: %d\n", ret);
> + goto err;
> + }
> +
> + drm_kms_helper_poll_init(dev);
> + drm_mode_config_reset(dev);
> +
> + return 0;
> +
> +err:
> + kfree(ldev);
I'm sorry, in the earlier version I told you to add this kfree() but
this is devm_ allocated so the kfree() is wrong and will lead to a
double free. My bad. That's on me.
> + drm_err(dev, "failed to initialize drm driver: %d\n", ret);
> + return ret;
> +}
regards,
dan carpenter
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
