From mboxrd@z Thu Jan 1 00:00:00 1970 From: Kyungmin Park Subject: RE: Review request to DRM Driver for Samsung SoC Exynos4210. Date: Tue, 30 Aug 2011 09:01:40 +0900 Message-ID: <062401cc66a7$feebce60$fcc36b20$%park@samsung.com> References: <001501cc6636$cca3b650$65eb22f0$%dae@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mailout2.samsung.com (mailout2.samsung.com [203.254.224.25]) by gabe.freedesktop.org (Postfix) with ESMTP id 4EF879E76E for ; Mon, 29 Aug 2011 17:12:07 -0700 (PDT) Received: from epcpsbgm2.samsung.com (mailout2.samsung.com [203.254.224.25]) by mailout2.samsung.com (Oracle Communications Messaging Exchange Server 7u4-19.01 64bit (built Sep 7 2010)) with ESMTP id <0LQP005YQS2PBM30@mailout2.samsung.com> for dri-devel@lists.freedesktop.org; Tue, 30 Aug 2011 09:02:05 +0900 (KST) Received: from TNRNDGASPAPP1.tn.corp.samsungelectronics.net ([165.213.149.150]) by mmp2.samsung.com (iPlanet Messaging Server 5.2 Patch 2 (built Jul 14 2004)) with ESMTPA id <0LQP00LSAS3HTF@mmp2.samsung.com> for dri-devel@lists.freedesktop.org; Tue, 30 Aug 2011 09:02:05 +0900 (KST) In-reply-to: Content-language: ko List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: 'Dave Airlie' , 'Inki Dae' Cc: rob@ti.com, eric.y.miao@gmail.com, dri-devel@lists.freedesktop.org, 'Kyungmin Park' List-Id: dri-devel@lists.freedesktop.org CC Eric (freescale), Rob (TI) who working the DRM with other ARM SoCs. As Dave mentioned, Please review Samsung DRM codes. Thank you, Kyungmin Park -----Original Message----- From: Dave Airlie [mailto:airlied@gmail.com] Sent: Monday, August 29, 2011 11:27 PM To: Inki Dae Cc: airlied@linux.ie; kyungmin.park@samsung.com; dri-devel@lists.freedesktop.org Subject: Re: Review request to DRM Driver for Samsung SoC Exynos4210. > I had sent DRM Driver for Samsung SoC Exynos4210 over the three times. > > But I didn't received any comments from you. I was sort of hoping someone else would take a look at these ARM drivers before me, it might be worth getting some inter-company review between the vendors submitting drm components as I'm guessing its going to be a lot of the same thing. But I've done a quick review and some things that stick out. 1. include/drm/samsung_drm.h should only contain public information for use on the userspace ioctl interface, it shouldn't contain any kernel private data structures, they should be in drivers/gpu/drm/samsung/samsung_drv.h or something very similiar. 2. I'm not sure why samsung_drm_fn_encoder exists, it looks like from the crtc mode set functions you call the encoder mode set functions, is it not possible to use the helper drm_crtc_helper_set_mode so that the crtc mode set is called then the encoder ones without having the explicit calls? If not please either document it or point at the explaination I missed. 3. Not terribly urgent but is samsung the best name for this? what happens if you bring out another chip, I also think there is a lot of samsung_drm_ in function names that seems not that useful. dropping _drm_ might help. 4 ioctls: drm_samsung_gem_map_off needs explicit padding before the 64-bit, drm_samsung_gem_mmap needs explicit padding before the 64-bit,, though I'm not sure you need these ioctls all now that the dumb interface is upstream, what is usr_addr in the gem create ioctl for? this seems wrong, it also looks too short for 64-bit addresses, but passing in userspace addr to kernel is generally not a great plan. 5. you probably don't need master_create/set ops. Dave.