[PATCH v10 2/2] drm: add kms driver for loongson display controller
Sui Jingfeng
15330273260 at 189.cn
Wed Apr 5 16:55:58 UTC 2023
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 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
>>
> 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
More information about the dri-devel
mailing list