Re: [PATCH v10 2/2] drm: add kms driver for loongson display controller

[Sui Jingfeng]

Hi,

On 2023/4/4 22:10, Emil Velikov wrote:
> > +++ 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?
OK, it just that we started to upstream this driver since 2022.
> > +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?

Sure, this is absolutely necessary.

Without this I can't see how much buffer object has been created.

where they are(SYETEM, GTT or VRAM) and how much size it the buffer is.

When sharing buffer with other driver,   cat /sys/kernel/debug/dri/0/bos

can be used to see that the shared buffer is pinned in the GTT.

> Very few drivers use TTM directly and I think you want to use
> drm_gem_vram_helper or drm_gem_ttm_helper instead.

We love you reviews,  yet...

yet using the TTM is pretty good.

drm_gem_vram_helper is also good for beginners.

We can explicitly specify where to put the bo with TTM,

We also need this to implement the S3 properly.

Thomas also recommend us switch to TTM.

> > +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.
Ok, I  will improve this function at the next version.
> > +/* 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?

No, we need it.

In order to get a fine control, we need to know what the host is.