[PATCH v10 2/2] drm: add kms driver for loongson display controller
Emil Velikov
emil.l.velikov at gmail.com
Tue Apr 4 14:10:06 UTC 2023
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 at 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
> +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
More information about the dri-devel
mailing list