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

["Christian König" <christian.koenig@amd.com>]

Am 15.03.23 um 22:15 schrieb Sui Jingfeng:
> From: suijingfeng <suijingfeng@loongson.cn>
>
> Loongson display controller IP has been integrated in both Loongson
> North Bridge chipset(ls7a1000 and ls7a2000) and Loongson SoCs(ls2k1000
> and ls2k2000 etc), it even has been included in Loongson BMC products.
>
> This display controller is a PCI device, it has two display pipe. For
> the DC in LS7A1000 and LS2K1000 each way has a DVO output interface
> which provide RGB888 signals, vertical & horizontal synchronisations,
> and the pixel clock. Each CRTC is able to support 1920x1080@60Hz,
> the maximum resolution is 2048x2048 according to the hardware spec.
>
> For the DC in LS7A2000, each display pipe is equipped with a built-in
> HDMI encoder which is compliant with HDMI 1.4 specification, thus it
> support 3840x2160@30Hz. The first display pipe is also equipped with
> a transparent vga encoder which is parallel with the HDMI encoder.
> The DC in LS7A2000 is more complete, besides above feature, it has
> two hardware cursors, two hardware vblank counter and two scanout
> position recorders.
>
>   v1 -> v2:
>    1) Use hpd status reg when polling for ls7a2000
>    2) Fix all warnings emerged when compile with W=1
>
>   v2 -> v3:
>    1) Add COMPILE_TEST in Kconfig and make the driver off by default
>    2) Alphabetical sorting headers (Thomas)
>    3) Untangle register access functions as much as possible (Thomas)
>    4) Switch to TTM based memory manager and prefer cached mapping
>       for Loongson SoC (Thomas)
>    5) Add chip id detection method, now all models are distinguishable.
>    6) Revise builtin HDMI phy driver, nearly all main stream mode
>       below 4K@30Hz is tested, this driver supported these mode very
>       well including clone display mode and extend display mode.
>
>   v3 -> v4:
>    1) Quickly fix a small mistake.
>
>   v4 -> v5:
>    1) Drop potential support for Loongson 2K series SoC temporary,
>       this part should be resend with the DT binding patch in the future.
>    2) Add per display pipe debugfs support to the builtin HDMI encoder.
>    3) Rewrite atomic_update() for hardware cursors plane(Thomas)
>    4) Rewrite encoder and connector initialization part, untangle it
>       according to the chip(Thomas).
>
>   v5 -> v6:
>    1) Remove stray code which didn't get used, say lsdc_of_get_reserved_ram
>    2) Fix all typos I could found, make sentences and code more readable
>    3) Untange lsdc_hdmi*_connector_detect() function according to the pipe
>    4) After a serious consideration, we rename this driver as loongson.
>       Because we also have drivers toward the LoongGPU IP in LS7A2000 and
>       LS2K2000. Besides, there are also drivers about the external encoder,
>       HDMI audio driver and vbios support etc. This patch only provide DC
>       driver part, my teammate Li Yi believe that loongson will be more
>       suitable for loongson graphics than lsdc in the long run.
>
>       loongson.ko = LSDC + LoongGPU + encoders driver + vbios/DT ...
>
>    v6 -> v7:
>    1) Add prime support, self-sharing is works. sharing buffer with etnaviv
>       is also tested, and its works with limitation.
>    2) Implement buffer objects tracking with list_head.
>    3) S3(sleep to RAM) is tested on ls3a5000+ls7a2000 evb and it works.
>    4) Rewrite lsdc_bo_move, since ttm core stop allocating resources
>       during BO creation. Patch V1 ~ V6 of this series no longer works
>       on latest kernel. Thus, we send V7.
>
> Signed-off-by: Li Yi <liyi@loongson.cn>
> Signed-off-by: suijingfeng <suijingfeng@loongson.cn>
> Signed-off-by: Sui Jingfeng <15330273260@189.cn>

[SNIP]

> +u64 lsdc_bo_gpu_offset(struct ttm_buffer_object *tbo)
> +{
> +	struct drm_device *ddev = tbo->base.dev;
> +	struct ttm_resource *resource = tbo->resource;
> +
> +	if (drm_WARN_ON(ddev, !tbo->pin_count))
> +		return -ENODEV;

Returning -ENODEV when the function return value is unsigned doesn't 
make much sense. I would also use 0 here.

Apart from that I briefly skimmed over the prime and TTM handling and 
couldn't find anything obviously wrong.

I obviously can't review the hw specific stuff, but over all looks 
pretty good to me.

Regards,
Christian.