Message-ID: <62f955de-6352-a0b1-ecab-52b854ba6839@loongson.cn>
Date:   Thu, 16 Mar 2023 16:46:35 +0800
From:   Sui jingfeng <suijingfeng@...ngson.cn>
To:     Christian König <christian.koenig@....com>,
        Sui Jingfeng <15330273260@....cn>,
        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>
Cc:     linux-kernel@...r.kernel.org, dri-devel@...ts.freedesktop.org,
        linux-media@...r.kernel.org, linaro-mm-sig@...ts.linaro.org,
        Li Yi <liyi@...ngson.cn>
Subject: Re: [PATCH v7 2/2] drm: add kms driver for loongson display
 controller


On 2023/3/16 15:18, Christian König wrote:
>
>
> Am 15.03.23 um 22:15 schrieb Sui Jingfeng:
>> From: suijingfeng <suijingfeng@...ngson.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@...z,
>> 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@...z. 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@...z 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@...ngson.cn>
>> Signed-off-by: suijingfeng <suijingfeng@...ngson.cn>
>> Signed-off-by: Sui Jingfeng <15330273260@....cn>
>
> [SNIP]
>
Hi,

I send my patch series with  my personal email (15330273260@....cn), 
because it is more reliable.

I don't mind remove it when this driver is applied.

>> +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.
>
OK,

To make sense, the caller can cast the return to s64. Use 0 is also ok.

In our daily usage,  tbo->pin_count ==0  never happens.  A warning 
message is enough.

I will revise this at next version.


> 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.
>
Ok, we can help to provide more HW specific document and material.

Currently, our hardware is just a trivial compare with ati/amdgpu graphics.

Thomas and you and  are experts at graphics driver domain,

  Maybe you could do me a favor, give me a reviewed-by for the TTM and 
PRIME part :)


> Regards,
> Christian.

Powered by blists - more mailing lists