Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <66fc74ae-299c-a5de-9cfb-07ae24fb3f07@loongson.cn>
Date:   Wed, 21 Jun 2023 23:30:04 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.11.0
Subject: Re: [PATCH v10 07/11] drm/etnaviv: Add support for the dma coherent
 device
Content-Language: en-US
To:     Lucas Stach <l.stach@pengutronix.de>,
        Sui Jingfeng <18949883232@163.com>,
        Russell King <linux+etnaviv@armlinux.org.uk>,
        Christian Gmeiner <christian.gmeiner@gmail.com>,
        David Airlie <airlied@gmail.com>,
        Daniel Vetter <daniel@ffwll.ch>
Cc:     linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
        etnaviv@lists.freedesktop.org,
        Philipp Zabel <p.zabel@pengutronix.de>,
        Bjorn Helgaas <bhelgaas@google.com>
References: <20230620094716.2231414-1-18949883232@163.com>
 <20230620094716.2231414-8-18949883232@163.com>
 <8f74f0962c8bab6c832919a5340667c54e1a7ddc.camel@pengutronix.de>
From:   Sui Jingfeng <suijingfeng@loongson.cn>
Organization: Loongson
In-Reply-To: <8f74f0962c8bab6c832919a5340667c54e1a7ddc.camel@pengutronix.de>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

On 2023/6/21 18:00, Lucas Stach wrote:
>>   		dma_sync_sgtable_for_cpu(dev->dev, etnaviv_obj->sgt,
>>   					 etnaviv_op_to_dma_dir(op));
>>   		etnaviv_obj->last_cpu_prep_op = op;
>> @@ -408,8 +421,9 @@ int etnaviv_gem_cpu_fini(struct drm_gem_object *obj)
>>   {
>>   	struct drm_device *dev = obj->dev;
>>   	struct etnaviv_gem_object *etnaviv_obj = to_etnaviv_bo(obj);
>> +	struct etnaviv_drm_private *priv = dev->dev_private;
>>   
>> -	if (etnaviv_obj->flags & ETNA_BO_CACHED) {
>> +	if (!priv->dma_coherent && etnaviv_obj->flags & ETNA_BO_CACHED) {
>>   		/* fini without a prep is almost certainly a userspace error */
>>   		WARN_ON(etnaviv_obj->last_cpu_prep_op == 0);
>>   		dma_sync_sgtable_for_device(dev->dev, etnaviv_obj->sgt,
>> diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>> index 3524b5811682..754126992264 100644
>> --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>> +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_prime.c
>> @@ -112,11 +112,16 @@ static const struct etnaviv_gem_ops etnaviv_gem_prime_ops = {
>>   struct drm_gem_object *etnaviv_gem_prime_import_sg_table(struct drm_device *dev,
>>   	struct dma_buf_attachment *attach, struct sg_table *sgt)
>>   {
>> +	struct etnaviv_drm_private *priv = dev->dev_private;
>>   	struct etnaviv_gem_object *etnaviv_obj;
>>   	size_t size = PAGE_ALIGN(attach->dmabuf->size);
>> +	u32 cache_flags = ETNA_BO_WC;
>>   	int ret, npages;
>>   
>> -	ret = etnaviv_gem_new_private(dev, size, ETNA_BO_WC,
>> +	if (priv->dma_coherent)
>> +		cache_flags = ETNA_BO_CACHED;
>> +
> Drop this change. Instead etnaviv_gem_new_impl() should do the upgrade
> from WC to CACHED as necessary by adding something like this:

I understand you are a profession person in vivante GPU driver domain.

I respect you reviews and instruction.

But, I'm really reluctant to agree with this, is there any space to 
negotiate?

> /*
>   * Upgrade WC to CACHED when the device is hardware coherent and the
>   * platform doesn't allow mixing cached and writecombined mappings to
>   * the same memory area.
>   */
> if ((flags & ETNA_BO_CACHE_MASK) == ETNA_BO_WC &&
>      dev_is_dma_coherent(dev) && !drm_arch_can_wc_memory())
>          flags = (flags & ~ETNA_BO_CACHE_MASK) & ETNA_BO_CACHED;

This is policy, not a mechanism.

Using what cache property is a user-space program's choice.

While you are override the WC with CACHED mapping. This is not correct 
in the concept!

you approach forbidden any possibility to use the WC BO at anywhere.


My approach need only check once, while you approach need at least 3 
check plus

so much bit-wise logic operations,  plus a function call  (&, ==, &&,  
&, ~, &) .

and every time you create a BO. This nasty judgement happens.


Please keep our original implement, it's simple and clear, Please?


-- 
Jingfeng