Messages in this thread | | | Date | Thu, 6 Apr 2023 00:55:58 +0800 | Subject | Re: [PATCH v10 2/2] drm: add kms driver for loongson display controller | From | Sui Jingfeng <> |
| |
On 2023/4/6 00:40, Sui Jingfeng wrote: > Hi, > > On 2023/4/4 22:10, Emil Velikov wrote: >> Greetings Sui Jingfeng, >> >> I haven't been around drm-land for a while and this is the first >> driver I skim through in a few years. So take the following >> suggestions with a healthy pinch of salt. >> >> Hope that helps o/ > Emil, we love your reviews, >> On Mon, 3 Apr 2023 at 18:13, Sui Jingfeng <suijingfeng@loongson.cn> >> wrote: >> >>> v7 -> v8: >>> 1) Zero a compile warnnings on 32-bit platform, compile with W=1 >>> 2) Revise lsdc_bo_gpu_offset() and minor cleanup >>> 3) Pageflip tested on the virtual terminal with following commands >>> >>> modetest -M loongson -s 32:1920x1080 -v >>> modetest -M loongson -s 34:1920x1080 -v -F tiles >>> >> I could be wrong, but my understanding is that new drivers should be >> capable of running under Xorg and/or Wayland compositor. There is also >> the IGT test suite, which can help verify and validate the driver's >> behaviour: >> >> https://www.kernel.org/doc/html/latest/gpu/drm-uapi.html >> > Yet it may take more time to give full answer for all of your concerns. > > Currently, drm/loongson driver do works under Xorg(X server), > > link[1] is a short video which can prove that the driver actually > works very well. > > Note that it use the generic modesetting driver on userspace. > > We could provide more videos if necessary. > > > We are carry on the IGT test suite, we feedback the test result once > it finished on our platform. > We will feedback the results once we finishe the igt test, thanks for providing such a valuable
information.
[1] https://raw.githubusercontent.com/loongson-gfx/loongson_boards/main/videos/drm_loongson_under_xserver.mp4
> I'm not familiar with it before, previously we only focus on the basic > unit tests came with libdrm. > > > I will answer rest questions in a latter time, please wait a moment. > >> >>> +static void lsdc_crtc_reset(struct drm_crtc *crtc) >>> +{ >>> + struct lsdc_display_pipe *dispipe = crtc_to_display_pipe(crtc); >>> + struct drm_device *ddev = crtc->dev; >>> + struct lsdc_device *ldev = to_lsdc(ddev); >>> + struct lsdc_crtc_state *priv_crtc_state; >>> + unsigned int index = dispipe->index; >>> + u32 val; >>> + >>> + val = LSDC_PF_XRGB8888 | CFG_RESET_N; >>> + if (ldev->descp->chip == CHIP_LS7A2000) >>> + val |= LSDC_DMA_STEP_64_BYTES; >>> + >>> + lsdc_crtc_wreg32(ldev, LSDC_CRTC0_CFG_REG, index, val); >>> + >>> + if (ldev->descp->chip == CHIP_LS7A2000) { >>> + val = PHY_CLOCK_EN | PHY_DATA_EN; >>> + lsdc_crtc_wreg32(ldev, LSDC_CRTC0_PANEL_CONF_REG, >>> index, val); >>> + } >>> + >> AFAICT no other driver touches the HW in their reset callback. Should >> the above be moved to another callback? >> >> >> >>> +static void lsdc_crtc_atomic_enable(struct drm_crtc *crtc, >>> + struct drm_atomic_state *state) >>> +{ >>> + val = lsdc_crtc_rreg32(ldev, LSDC_CRTC0_CFG_REG, index); >>> + /* clear old dma step settings */ >>> + val &= ~CFG_DMA_STEP_MASK; >>> + >>> + if (descp->chip == CHIP_LS7A2000) { >>> + /* >>> + * Using large dma step as much as possible, >>> + * for improve hardware DMA efficiency. >>> + */ >>> + if (width_in_bytes % 256 == 0) >>> + val |= LSDC_DMA_STEP_256_BYTES; >>> + else if (width_in_bytes % 128 == 0) >>> + val |= LSDC_DMA_STEP_128_BYTES; >>> + else if (width_in_bytes % 64 == 0) >>> + val |= LSDC_DMA_STEP_64_BYTES; >>> + else /* width_in_bytes % 32 == 0 */ >>> + val |= LSDC_DMA_STEP_32_BYTES; >>> + } >>> + >>> + clk_func->update(pixpll, &priv_state->pparms); >>> + >> Without knowing the hardware, the clk_func abstraction seems quite >> arbitrary and unnecessary. It should be introduced when there is a >> use-case for it. >> >> >>> + lsdc_crtc_wreg32(ldev, LSDC_CRTC0_CFG_REG, index, val | >>> CFG_OUTPUT_EN); >>> + >>> + drm_crtc_vblank_on(crtc); >>> +} >>> + >> >>> --- /dev/null >>> +++ b/drivers/gpu/drm/loongson/lsdc_debugfs.c >>> +void lsdc_debugfs_init(struct drm_minor *minor) >>> +{ >>> +#ifdef CONFIG_DEBUG_FS >>> + drm_debugfs_create_files(lsdc_debugfs_list, >>> + ARRAY_SIZE(lsdc_debugfs_list), >>> + minor->debugfs_root, >>> + minor); >>> +#endif >>> +} >> Should probably build the file when debugfs is enabled and provide >> no-op stub in the header. See nouveau for an example. >> >> >>> --- /dev/null >>> +++ b/drivers/gpu/drm/loongson/lsdc_drv.c >>> +static const struct lsdc_desc dc_in_ls7a1000 = { >>> + .chip = CHIP_LS7A1000, >>> + .num_of_crtc = LSDC_NUM_CRTC, >>> + .max_pixel_clk = 200000, >>> + .max_width = 2048, >>> + .max_height = 2048, >>> + .num_of_hw_cursor = 1, >>> + .hw_cursor_w = 32, >>> + .hw_cursor_h = 32, >>> + .pitch_align = 256, >>> + .mc_bits = 40, >>> + .has_vblank_counter = false, >>> + .has_scan_pos = true, >>> + .has_builtin_i2c = true, >>> + .has_vram = true, >>> + .has_hpd_reg = false, >>> + .is_soc = false, >>> +}; >>> + >>> +static const struct lsdc_desc dc_in_ls7a2000 = { >>> + .chip = CHIP_LS7A2000, >>> + .num_of_crtc = LSDC_NUM_CRTC, >>> + .max_pixel_clk = 350000, >>> + .max_width = 4096, >>> + .max_height = 4096, >>> + .num_of_hw_cursor = 2, >>> + .hw_cursor_w = 64, >>> + .hw_cursor_h = 64, >>> + .pitch_align = 64, >>> + .mc_bits = 40, /* support 48, but use 40 for backward >>> compatibility */ >>> + .has_vblank_counter = true, >>> + .has_scan_pos = true, >>> + .has_builtin_i2c = true, >>> + .has_vram = true, >>> + .has_hpd_reg = true, >>> + .is_soc = false, >>> +}; >>> + >> Roughly a quarter of the above are identical. It might be better to >> drop them for now and re-introduce as needed with future code. >> >>> +const char *chip_to_str(enum loongson_chip_family chip) >>> +{ >>> + if (chip == CHIP_LS7A2000) >>> + return "LS7A2000"; >>> + >>> + if (chip == CHIP_LS7A1000) >>> + return "LS7A1000"; >>> + >> If it were me, I would add the name into the lsdc_desc. >> >> >>> +static enum drm_mode_status >>> +lsdc_mode_config_mode_valid(struct drm_device *ddev, >>> + const struct drm_display_mode *mode) >>> +{ >>> + struct lsdc_device *ldev = to_lsdc(ddev); >>> + const struct drm_format_info *info = >>> drm_format_info(DRM_FORMAT_XRGB8888); >> Short-term hard coding a format is fine, but there should be a comment >> describing why. >> >>> + u64 min_pitch = drm_format_info_min_pitch(info, 0, >>> mode->hdisplay); >>> + u64 fb_size = min_pitch * mode->vdisplay; >>> + >>> + if (fb_size * 3 > ldev->vram_size) { >> Why are we using 3 here? Please add an inline comment. >> >> >>> +static const struct dev_pm_ops lsdc_pm_ops = { >>> + .suspend = lsdc_pm_suspend, >>> + .resume = lsdc_pm_resume, >>> + .freeze = lsdc_pm_freeze, >>> + .thaw = lsdc_pm_thaw, >>> + .poweroff = lsdc_pm_freeze, >>> + .restore = lsdc_pm_resume, >>> +}; >>> + >> The above section (and functions) should probably be wrapped in a >> CONFIG_PM_SLEEP block. >> >> >> >>> +static const struct pci_device_id lsdc_pciid_list[] = { >>> + {PCI_VENDOR_ID_LOONGSON, 0x7a06, PCI_ANY_ID, PCI_ANY_ID, 0, >>> 0, CHIP_LS7A1000}, >>> + {PCI_VENDOR_ID_LOONGSON, 0x7a36, PCI_ANY_ID, PCI_ANY_ID, 0, >>> 0, CHIP_LS7A2000}, >>> + {0, 0, 0, 0, 0, 0, 0} >>> +}; >>> + >>> +static int __init loongson_module_init(void) >>> +{ >>> + while ((pdev = pci_get_class(PCI_CLASS_DISPLAY_VGA << 8, >>> pdev))) { >>> + if (pdev->vendor != PCI_VENDOR_ID_LOONGSON) { >>> + pr_info("Discrete graphic card detected, >>> abort\n"); >>> + return 0; >>> + } >>> + } >> You can set the class/class_mask in the lsdc_pciid_list and drop this >> loop. The vendor is already listed above and checked by core. >> >> >> >>> +++ b/drivers/gpu/drm/loongson/lsdc_drv.h >>> @@ -0,0 +1,324 @@ >>> +/* SPDX-License-Identifier: GPL-2.0 */ >>> +/* >>> + * Copyright (C) 2022 Loongson Corporation >>> + * >> We're in 2023, update the year across the files? >> >> >> >>> +struct lsdc_gem { >>> + /* @mutex: protect objects list */ >>> + struct mutex mutex; >>> + struct list_head objects; >>> +}; >>> + >>> +struct lsdc_device { >>> + struct drm_device base; >>> + struct ttm_device bdev; >>> + >>> + /* @descp: features description of the DC variant */ >>> + const struct lsdc_desc *descp; >>> + >>> + struct pci_dev *gpu; >>> + >>> + /* @reglock: protects concurrent access */ >>> + spinlock_t reglock; >>> + void __iomem *reg_base; >>> + resource_size_t vram_base; >>> + resource_size_t vram_size; >>> + >>> + resource_size_t gtt_size; >>> + >>> + struct lsdc_display_pipe dispipe[LSDC_NUM_CRTC]; >>> + >>> + struct lsdc_gem gem; >>> + >> Last time I looked there was no other driver with a list of gem >> objects (and a mutex) in its device struct. Are you sure we need this? >> >> Very few drivers use TTM directly and I think you want to use >> drm_gem_vram_helper or drm_gem_ttm_helper instead. >> >> >> >>> +static int ls7a1000_pixpll_param_update(struct lsdc_pll * const this, >>> + struct lsdc_pll_parms const >>> *pin) >>> +{ >>> + void __iomem *reg = this->mmio; >>> + unsigned int counter = 0; >>> + bool locked; >>> + u32 val; >>> + >>> + /* Bypass the software configured PLL, using refclk directly */ >>> + val = readl(reg + 0x4); >>> + val &= ~(1 << 8); >>> + writel(val, reg + 0x4); >>> + >> There are a lot of magic numbers in this function. Let's define them >> properly in the header. >> >> >> >>> +/* Helpers for chip detection */ >>> +bool lsdc_is_ls2k2000(void); >>> +bool lsdc_is_ls2k1000(void); >>> +unsigned int loongson_cpu_get_prid(u8 *impl, u8 *rev); >> >> Since this revision does pci_devices only, we don't need this >> detection right? >> >> >> Hope that helps, >> Emil
| |