From mboxrd@z Thu Jan 1 00:00:00 1970 From: Konrad Rzeszutek Wilk Subject: Re: [RFC][PATCH v3] DRM: add DRM Driver for Samsung SoC EXYNOS4210. Date: Tue, 30 Aug 2011 22:06:52 -0400 Message-ID: <20110831020652.GA16547@dumpdata.com> References: <1314359274-21585-1-git-send-email-inki.dae@samsung.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from rcsinet14.oracle.com (rcsinet14.oracle.com [148.87.113.126]) by gabe.freedesktop.org (Postfix) with ESMTP id 96F649E894 for ; Tue, 30 Aug 2011 19:07:55 -0700 (PDT) Received: from rcsinet15.oracle.com (rcsinet15.oracle.com [148.87.113.117]) by rcsinet14.oracle.com (Switch-3.4.4/Switch-3.4.1) with ESMTP id p7V27tYp001888 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Wed, 31 Aug 2011 02:07:55 GMT Content-Disposition: inline In-Reply-To: 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: Rob Clark Cc: sw0312.kim@samsung.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Inki Dae , kyungmin.park@samsung.com, linux-arm-kernel@lists.infradead.org List-Id: dri-devel@lists.freedesktop.org > > + =A0 =A0 =A0 entry->vaddr =3D dma_alloc_writecombine(dev->dev, entry->= size, > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (dma_addr_t *)&entry->pad= dr, GFP_KERNEL); > > + =A0 =A0 =A0 if (!entry->paddr) { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 DRM_ERROR("failed to allocate buffer.\n"); > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 return -ENOMEM; > > + =A0 =A0 =A0 } > > + > > + =A0 =A0 =A0 DRM_DEBUG_KMS("allocated : vaddr(0x%x), paddr(0x%x), size= (0x%x)\n", > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (unsigned int)entry->vadd= r, entry->paddr, entry->size); > > + > > + =A0 =A0 =A0 return 0; > > +} > > + > [snip] > = > > diff --git a/drivers/gpu/drm/samsung/samsung_drm_buf.h b/drivers/gpu/dr= m/samsung/samsung_drm_buf.h > > new file mode 100644 > > index 0000000..d6a7e95 > > --- /dev/null > > +++ b/drivers/gpu/drm/samsung/samsung_drm_buf.h > [snip] > > +/** > > + * samsung drm buffer entry structure. > > + * > > + * @paddr: physical address of allocated memory. > > + * @vaddr: kernel virtual address of allocated memory. > > + * @size: size of allocated memory. > > + */ > > +struct samsung_drm_buf_entry { > > + =A0 =A0 =A0 unsigned int paddr; This could be made 'dma_addr_t' and then you can drop all of the casts to (dma_addr_t *). .. snip.. > > +static int samsung_drm_connector_get_modes(struct drm_connector *conne= ctor) Why not make the return be 'unsigned int'? = .. snip.. > > +/* get detection status of display device. */ > > +static enum drm_connector_status > > +samsung_drm_connector_detect(struct drm_connector *connector, bool for= ce) > > +{ > > + =A0 =A0 =A0 struct samsung_drm_connector *samsung_connector =3D > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 to_samsung_connector(connector); > > + =A0 =A0 =A0 struct samsung_drm_display *display =3D > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 samsung_drm_get_manager(samsung_connector= ->encoder)->display; > > + =A0 =A0 =A0 unsigned int ret =3D connector_status_unknown; Not 'enum drm_connector_status ret =3D connector_status_unknown' ? > > + > > + =A0 =A0 =A0 DRM_DEBUG_KMS("%s\n", __FILE__); > > + > > + =A0 =A0 =A0 if (display && display->is_connected) { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (display->is_connected()) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D connector_status_= connected; > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 else > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D connector_status_= disconnected; > > + =A0 =A0 =A0 } > > + > > + =A0 =A0 =A0 return ret; > > +} .. snip.. > > +static void samsung_drm_fb_destroy(struct drm_framebuffer *fb) > > +{ > > + =A0 =A0 =A0 struct samsung_drm_fb *samsung_fb =3D to_samsung_fb(fb); > > + =A0 =A0 =A0 int ret; Get rid of 'ret' > > + > > + =A0 =A0 =A0 DRM_DEBUG_KMS("%s\n", __FILE__); > > + > > + =A0 =A0 =A0 drm_framebuffer_cleanup(fb); > > + > > + =A0 =A0 =A0 if (samsung_fb->is_default) { > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D drm_gem_handle_delete(samsung_fb-= >file_priv, > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 samsung_f= b->gem_handle); > = > why not keep the gem buffer ptr, and do something like: > = > drm_gem_object_unreference_unlocked(samsung_fb->bo).. > = > this way, you get the right behavior if someone somewhere else took a > ref to the gem buffer object? And it avoids needing to keep the > file_priv ptr in the fb (which seems a bit strange) > = > = > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 if (ret < 0) > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 DRM_ERROR("failed to dele= te drm_gem_handle.\n"); And just do the check on the function return value here. You are not using the 'ret' for anything. > > + =A0 =A0 =A0 } > > + > > + =A0 =A0 =A0 kfree(samsung_fb); > > +} > > + > [snip] Hm, so I stopped here - just realized that I am missing some of the code and I should look at the original patch..