Received: by 2002:a25:8b91:0:0:0:0:0 with SMTP id j17csp435133ybl; Wed, 11 Dec 2019 02:09:40 -0800 (PST) X-Google-Smtp-Source: APXvYqw+9jkutY9Bvm32SOhu2ZqK/ZRCuc2soqEUf3aKd++1/TPmAP0jCFIQUaRy4LEZmP1ES95K X-Received: by 2002:aca:5083:: with SMTP id e125mr2187531oib.96.1576058980407; Wed, 11 Dec 2019 02:09:40 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1576058980; cv=none; d=google.com; s=arc-20160816; b=wxl/qPNgVx8EJxLX0DFnfShiJluxSy/t1F8k7I1q8UPV/rBGQ+h3RbGZtrfFi7G++1 Pe4PsBiPF4OWchJW38F/yW8AAe0rdlHoLA1LuKzJOMA5gCLFRdOjO+jp0ZNZ6ZkqTjFr kcC8aFNBsuKLd0IJL+2E4s2o5WskrxkhWqZnERRZGoi9CdEIgdyMJi0Z1HCEq/XGnnsh DK47n7yeX6UjL+gqdidYPqVvtTcIXyNK59C2LjL/kliBIy9GopOztS4U4pDdzGBb5moR HKC4rNzBwOzmPikpsfpjQT3fxoTCLa75fDDP0MKq95BaVvBY5kH6eU82noWbyZ0cTVnG 0xQQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:in-reply-to:mime-version:user-agent:date :message-id:autocrypt:references:cc:to:from:subject; bh=8h4DKNY7jWEBV90oLpUAJucOaIgYDnmA+RHuUKBlbkI=; b=Wp0L7eGp+6H40KayuMTmrnJJZdqHTfkogkGKxPPk8d+rw7YUS6xzrX/82jh79vKAsh MTR4pEUhRf9Pg0+cJ2f4PU+wt75RfVXWoe16KSSCxNlV18P90GfVRhgTm7bwrC/FMLDU T9BT5/4ZgTFqHEiQ2VUlwO73bU0hisHOIa2l/HFw35mK+NMtHber20AOOcZo4WyPtHEu 5r/2mL7ppAcLop/kBEzmZ5+aVchAxOMfyS+I3bkL9TQ+SZvKSw+xXuyjA3vc157ni1Um 1ej7jukKMcUqdoqb7lZ16IVe5Nc/wA1SubL4tY//mi6CytPdmDc72RvdffPeiz7SXOjR uA/Q== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id t184si816210oig.184.2019.12.11.02.09.27; Wed, 11 Dec 2019 02:09:40 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728696AbfLKKHc (ORCPT + 99 others); Wed, 11 Dec 2019 05:07:32 -0500 Received: from mx2.suse.de ([195.135.220.15]:36898 "EHLO mx1.suse.de" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1728404AbfLKKHc (ORCPT ); Wed, 11 Dec 2019 05:07:32 -0500 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id A3A02AD75; Wed, 11 Dec 2019 10:07:29 +0000 (UTC) Subject: Re: [PATCH v2 1/2] drm/shmem: add support for per object caching attributes From: Thomas Zimmermann To: Gerd Hoffmann , dri-devel@lists.freedesktop.org Cc: David Airlie , open list , gurchetansingh@chromium.org References: <20191211081810.20079-1-kraxel@redhat.com> <20191211081810.20079-2-kraxel@redhat.com> <0b64e917-48f7-487e-9335-2838b6c62808@suse.de> Autocrypt: addr=tzimmermann@suse.de; keydata= mQENBFs50uABCADEHPidWt974CaxBVbrIBwqcq/WURinJ3+2WlIrKWspiP83vfZKaXhFYsdg XH47fDVbPPj+d6tQrw5lPQCyqjwrCPYnq3WlIBnGPJ4/jreTL6V+qfKRDlGLWFjZcsrPJGE0 BeB5BbqP5erN1qylK9i3gPoQjXGhpBpQYwRrEyQyjuvk+Ev0K1Jc5tVDeJAuau3TGNgah4Yc hdHm3bkPjz9EErV85RwvImQ1dptvx6s7xzwXTgGAsaYZsL8WCwDaTuqFa1d1jjlaxg6+tZsB 9GluwvIhSezPgnEmimZDkGnZRRSFiGP8yjqTjjWuf0bSj5rUnTGiyLyRZRNGcXmu6hjlABEB AAG0J1Rob21hcyBaaW1tZXJtYW5uIDx0emltbWVybWFubkBzdXNlLmRlPokBVAQTAQgAPhYh BHIX+6yM6c9jRKFo5WgNwR1TC3ojBQJbOdLgAhsDBQkDwmcABQsJCAcCBhUKCQgLAgQWAgMB Ah4BAheAAAoJEGgNwR1TC3ojR80H/jH+vYavwQ+TvO8ksXL9JQWc3IFSiGpuSVXLCdg62AmR irxW+qCwNncNQyb9rd30gzdectSkPWL3KSqEResBe24IbA5/jSkPweJasgXtfhuyoeCJ6PXo clQQGKIoFIAEv1s8l0ggPZswvCinegl1diyJXUXmdEJRTWYAtxn/atut1o6Giv6D2qmYbXN7 mneMC5MzlLaJKUtoH7U/IjVw1sx2qtxAZGKVm4RZxPnMCp9E1MAr5t4dP5gJCIiqsdrVqI6i KupZstMxstPU//azmz7ZWWxT0JzgJqZSvPYx/SATeexTYBP47YFyri4jnsty2ErS91E6H8os Bv6pnSn7eAq5AQ0EWznS4AEIAMYmP4M/V+T5RY5at/g7rUdNsLhWv1APYrh9RQefODYHrNRH UE9eosYbT6XMryR9hT8XlGOYRwKWwiQBoWSDiTMo/Xi29jUnn4BXfI2px2DTXwc22LKtLAgT RjP+qbU63Y0xnQN29UGDbYgyyK51DW3H0If2a3JNsheAAK+Xc9baj0LGIc8T9uiEWHBnCH+R dhgATnWWGKdDegUR5BkDfDg5O/FISymJBHx2Dyoklv5g4BzkgqTqwmaYzsl8UxZKvbaxq0zb ehDda8lvhFXodNFMAgTLJlLuDYOGLK2AwbrS3Sp0AEbkpdJBb44qVlGm5bApZouHeJ/+n+7r 12+lqdsAEQEAAYkBPAQYAQgAJhYhBHIX+6yM6c9jRKFo5WgNwR1TC3ojBQJbOdLgAhsMBQkD wmcAAAoJEGgNwR1TC3ojpfcIAInwP5OlcEKokTnHCiDTz4Ony4GnHRP2fXATQZCKxmu4AJY2 h9ifw9Nf2TjCZ6AMvC3thAN0rFDj55N9l4s1CpaDo4J+0fkrHuyNacnT206CeJV1E7NYntxU n+LSiRrOdywn6erjxRi9EYTVLCHcDhBEjKmFZfg4AM4GZMWX1lg0+eHbd5oL1as28WvvI/uI aMyV8RbyXot1r/8QLlWldU3NrTF5p7TMU2y3ZH2mf5suSKHAMtbE4jKJ8ZHFOo3GhLgjVrBW HE9JXO08xKkgD+w6v83+nomsEuf6C6LYrqY/tsZvyEX6zN8CtirPdPWu/VXNRYAl/lat7lSI 3H26qrE= Message-ID: Date: Wed, 11 Dec 2019 11:07:25 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: <0b64e917-48f7-487e-9335-2838b6c62808@suse.de> Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="r8TvLUVhGQQ8aTEs6Xg8KLXS7KMPkSUSl" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --r8TvLUVhGQQ8aTEs6Xg8KLXS7KMPkSUSl Content-Type: multipart/mixed; boundary="PWptc28KfncWqAB1I31NMJorptpyGRJEF"; protected-headers="v1" From: Thomas Zimmermann To: Gerd Hoffmann , dri-devel@lists.freedesktop.org Cc: David Airlie , open list , gurchetansingh@chromium.org Message-ID: Subject: Re: [PATCH v2 1/2] drm/shmem: add support for per object caching attributes References: <20191211081810.20079-1-kraxel@redhat.com> <20191211081810.20079-2-kraxel@redhat.com> <0b64e917-48f7-487e-9335-2838b6c62808@suse.de> In-Reply-To: <0b64e917-48f7-487e-9335-2838b6c62808@suse.de> --PWptc28KfncWqAB1I31NMJorptpyGRJEF Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: quoted-printable Am 11.12.19 um 10:58 schrieb Thomas Zimmermann: > Hi Gerd >=20 > Am 11.12.19 um 09:18 schrieb Gerd Hoffmann: >> Add caching field to drm_gem_shmem_object to specify the cachine >> attributes for mappings. Add helper function to tweak pgprot >> accordingly. Switch vmap and mmap functions to the new helper. >> >> Set caching to write-combine when creating the object so behavior >> doesn't change by default. Drivers can override that later if >> needed. >> >> Signed-off-by: Gerd Hoffmann >=20 > If you want to merge this patch, you have my >=20 > Reviewed-by: Thomas Zimmermann >=20 > Please see my comment below. >=20 >> --- >> include/drm/drm_gem_shmem_helper.h | 12 ++++++++++++ >> drivers/gpu/drm/drm_gem_shmem_helper.c | 24 +++++++++++++++++++++--- >> 2 files changed, 33 insertions(+), 3 deletions(-) >> >> diff --git a/include/drm/drm_gem_shmem_helper.h b/include/drm/drm_gem_= shmem_helper.h >> index 6748379a0b44..9d6e02c6205f 100644 >> --- a/include/drm/drm_gem_shmem_helper.h >> +++ b/include/drm/drm_gem_shmem_helper.h >> @@ -17,6 +17,11 @@ struct drm_mode_create_dumb; >> struct drm_printer; >> struct sg_table; >> =20 >> +enum drm_gem_shmem_caching { >> + DRM_GEM_SHMEM_CACHED =3D 1, >> + DRM_GEM_SHMEM_WC, >> +}; >> + >> /** >> * struct drm_gem_shmem_object - GEM object backed by shmem >> */ >> @@ -83,6 +88,11 @@ struct drm_gem_shmem_object { >> * The address are un-mapped when the count reaches zero. >> */ >> unsigned int vmap_use_count; >> + >> + /** >> + * @caching: caching attributes for mappings. >> + */ >> + enum drm_gem_shmem_caching caching; >> }; >> =20 >> #define to_drm_gem_shmem_obj(obj) \ >> @@ -130,6 +140,8 @@ drm_gem_shmem_prime_import_sg_table(struct drm_dev= ice *dev, >> =20 >> struct sg_table *drm_gem_shmem_get_pages_sgt(struct drm_gem_object *o= bj); >> =20 >> +pgprot_t drm_gem_shmem_caching(struct drm_gem_shmem_object *shmem, pg= prot_t prot); >> + >> /** >> * DRM_GEM_SHMEM_DRIVER_OPS - Default shmem GEM operations >> * >> diff --git a/drivers/gpu/drm/drm_gem_shmem_helper.c b/drivers/gpu/drm/= drm_gem_shmem_helper.c >> index a421a2eed48a..5bb94e130a50 100644 >> --- a/drivers/gpu/drm/drm_gem_shmem_helper.c >> +++ b/drivers/gpu/drm/drm_gem_shmem_helper.c >> @@ -76,6 +76,7 @@ struct drm_gem_shmem_object *drm_gem_shmem_create(st= ruct drm_device *dev, size_t >> mutex_init(&shmem->pages_lock); >> mutex_init(&shmem->vmap_lock); >> INIT_LIST_HEAD(&shmem->madv_list); >> + shmem->caching =3D DRM_GEM_SHMEM_WC; >> =20 >> /* >> * Our buffers are kept pinned, so allocating them >> @@ -256,9 +257,11 @@ static void *drm_gem_shmem_vmap_locked(struct drm= _gem_shmem_object *shmem) >> =20 >> if (obj->import_attach) >> shmem->vaddr =3D dma_buf_vmap(obj->import_attach->dmabuf); >> - else >> + else { >> + pgprot_t prot =3D drm_gem_shmem_caching(shmem, PAGE_KERNEL); >> shmem->vaddr =3D vmap(shmem->pages, obj->size >> PAGE_SHIFT, >> - VM_MAP, pgprot_writecombine(PAGE_KERNEL)); >> + VM_MAP, prot); >> + } >> =20 >> if (!shmem->vaddr) { >> DRM_DEBUG_KMS("Failed to vmap pages\n"); >> @@ -540,7 +543,8 @@ int drm_gem_shmem_mmap(struct drm_gem_object *obj,= struct vm_area_struct *vma) >> } >> =20 >> vma->vm_flags |=3D VM_MIXEDMAP | VM_DONTEXPAND; >> - vma->vm_page_prot =3D pgprot_writecombine(vm_get_page_prot(vma->vm_f= lags)); >> + vma->vm_page_prot =3D vm_get_page_prot(vma->vm_flags); >> + vma->vm_page_prot =3D drm_gem_shmem_caching(shmem, vma->vm_page_prot= ); >> vma->vm_page_prot =3D pgprot_decrypted(vma->vm_page_prot); >> vma->vm_ops =3D &drm_gem_shmem_vm_ops; >> =20 >> @@ -683,3 +687,17 @@ drm_gem_shmem_prime_import_sg_table(struct drm_de= vice *dev, >> return ERR_PTR(ret); >> } >> EXPORT_SYMBOL_GPL(drm_gem_shmem_prime_import_sg_table); >> + >> +pgprot_t drm_gem_shmem_caching(struct drm_gem_shmem_object *shmem, pg= prot_t prot) >> +{ >> + switch (shmem->caching) { >> + case DRM_GEM_SHMEM_CACHED: >> + return prot; >> + case DRM_GEM_SHMEM_WC: >> + return pgprot_writecombine(prot); >> + default: >> + WARN_ON_ONCE(1); >> + return prot; >> + } >> +} >> +EXPORT_SYMBOL_GPL(drm_gem_shmem_caching); >=20 > Two reason why I'd reconsider this design. >=20 > I don't like switch statements new the bottom of the call graph. The > code ends up with default warnings, such as this one. >=20 > Udl has different caching flags for imported and 'native' buffers. This= > would require a new constant and additional code here. >=20 > What do you think about turning this function into a callback in struct= > shmem_funcs? The default implementation would be for WC, virtio would > use CACHED. The individual implementations could still be located in th= e > shmem code. Udl would later provide its own code. On a second thought, all this might be over-engineered and v1 of the patchset was the correct approach. You can add my Acked-by: Thomas Zimmermann if you prefer to merge v1. >=20 > Best regards > Thomas >=20 >> >=20 --=20 Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Maxfeldstr. 5, 90409 N=C3=BCrnberg, Germany (HRB 36809, AG N=C3=BCrnberg) Gesch=C3=A4ftsf=C3=BChrer: Felix Imend=C3=B6rffer --PWptc28KfncWqAB1I31NMJorptpyGRJEF-- --r8TvLUVhGQQ8aTEs6Xg8KLXS7KMPkSUSl Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAEBCAAdFiEEchf7rIzpz2NEoWjlaA3BHVMLeiMFAl3wv90ACgkQaA3BHVML eiO64Qf/YBbsWDQuJn4oSmZ4mtNOipUzxozbGDzsimdp95A//zGbwtYuSiAsEaom gqnUWkfh4aZ6eDwixlNmyUwZa7duLQ6Nw5QmA36BCyzhHdjoUoNt22gggKMHP6Jv ODbtVVpOkdMpNo9MrrxKI0zwiaZFFPEbNLH1Dg6B2JcwLupSAJs4C9JITQ3vqgct fQsNDJqQtQk5ylIh7J4qiC9/zvt0wsfQhHFTc0YkeHv24ejvi3o37dXe2vmGKMjM xzILi2sG6pRPzJPx/LxkU4hcYUSAd9C9gKlkLSt2ncae+3Vh9fQajphP/1sZv+0z eXjXG/FPxleeSVzZenwW4dE0rgiJhQ== =ZM7a -----END PGP SIGNATURE----- --r8TvLUVhGQQ8aTEs6Xg8KLXS7KMPkSUSl--