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

[Sui jingfeng <suijingfeng@loongson.cn>]

On 2023/3/16 15:18, Christian König wrote:
>
>
> 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.
>
Yeah, this is very nice comments actually.

much better than NAK, complete NAK, big NAK or somethings like that.

Thanks a lot.

> Regards,
> Christian.