From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dave Airlie Subject: Re: Review request to DRM Driver for Samsung SoC Exynos4210. Date: Mon, 29 Aug 2011 15:26:32 +0100 Message-ID: References: <001501cc6636$cca3b650$65eb22f0$%dae@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mail-yi0-f49.google.com (mail-yi0-f49.google.com [209.85.218.49]) by gabe.freedesktop.org (Postfix) with ESMTP id F33A29E7B8 for ; Mon, 29 Aug 2011 07:26:32 -0700 (PDT) Received: by yic13 with SMTP id 13so3919023yic.36 for ; Mon, 29 Aug 2011 07:26:32 -0700 (PDT) In-Reply-To: <001501cc6636$cca3b650$65eb22f0$%dae@samsung.com> 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: Inki Dae Cc: kyungmin.park@samsung.com, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org > I had sent DRM Driver for Samsung SoC Exynos4210 over the three times. > > But I didn=92t 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.