Message-ID: <33afce11-8ccc-5e98-749b-5e6aa80b82f1@189.cn>
Date:   Thu, 6 Apr 2023 00:55:58 +0800
From:   Sui Jingfeng <15330273260@....cn>
To:     Emil Velikov <emil.l.velikov@...il.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        Sumit Semwal <sumit.semwal@...aro.org>,
        Christian Koenig <christian.koenig@....com>,
        linaro-mm-sig@...ts.linaro.org, Li Yi <liyi@...ngson.cn>,
        linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        nathan@...nel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH v10 2/2] drm: add kms driver for loongson display
 controller


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@...ngson.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

Powered by blists - more mailing lists