+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
