Message-ID: <f795709e-e06a-daee-968b-b1cd0c1b5587@loongson.cn>
Date:   Thu, 16 Mar 2023 18:12:46 +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 16:46, Sui jingfeng wrote:
>
> 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.
>

We know that we should do this job cleanly.

But i am worry about the troubles it may cause when  send patch with my 
company email(suijingfeng <suijingfeng@...ngson.cn>)

So I just follow the my conventional way of sending patch about this 
series,

I will use my company email(suijingfeng <suijingfeng@...ngson.cn>) in 
the future.

>>> +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.
> Maybe you could do me a favor, give me a reviewed-by for the TTM and 
> PRIME part :)
>
Here missing a '?' at the end.

>
>> Regards,
>> Christian.

Powered by blists - more mailing lists