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/ On Mon, 3 Apr 2023 at 18:13, Sui Jingfeng <suijingfeng@xxxxxxxxxxx> 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 > +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
