From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754877Ab1HaCJH (ORCPT ); Tue, 30 Aug 2011 22:09:07 -0400 Received: from rcsinet15.oracle.com ([148.87.113.117]:53698 "EHLO rcsinet15.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754775Ab1HaCJF convert rfc822-to-8bit (ORCPT ); Tue, 30 Aug 2011 22:09:05 -0400 Date: Tue, 30 Aug 2011 22:06:52 -0400 From: Konrad Rzeszutek Wilk To: Rob Clark Cc: Inki Dae , sw0312.kim@samsung.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, kyungmin.park@samsung.com, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC][PATCH v3] DRM: add DRM Driver for Samsung SoC EXYNOS4210. 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-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.21 (2010-09-15) Content-Transfer-Encoding: 8BIT X-Source-IP: acsinet21.oracle.com [141.146.126.237] X-Auth-Type: Internal IP X-CT-RefId: str=0001.0A090207.4E5D9777.0043:SCFMA922111,ss=1,re=-4.000,fgs=0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > > +       entry->vaddr = dma_alloc_writecombine(dev->dev, entry->size, > > +                       (dma_addr_t *)&entry->paddr, GFP_KERNEL); > > +       if (!entry->paddr) { > > +               DRM_ERROR("failed to allocate buffer.\n"); > > +               return -ENOMEM; > > +       } > > + > > +       DRM_DEBUG_KMS("allocated : vaddr(0x%x), paddr(0x%x), size(0x%x)\n", > > +                       (unsigned int)entry->vaddr, entry->paddr, entry->size); > > + > > +       return 0; > > +} > > + > [snip] > > > diff --git a/drivers/gpu/drm/samsung/samsung_drm_buf.h b/drivers/gpu/drm/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 { > > +       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 *connector) 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 force) > > +{ > > +       struct samsung_drm_connector *samsung_connector = > > +               to_samsung_connector(connector); > > +       struct samsung_drm_display *display = > > +               samsung_drm_get_manager(samsung_connector->encoder)->display; > > +       unsigned int ret = connector_status_unknown; Not 'enum drm_connector_status ret = connector_status_unknown' ? > > + > > +       DRM_DEBUG_KMS("%s\n", __FILE__); > > + > > +       if (display && display->is_connected) { > > +               if (display->is_connected()) > > +                       ret = connector_status_connected; > > +               else > > +                       ret = connector_status_disconnected; > > +       } > > + > > +       return ret; > > +} .. snip.. > > +static void samsung_drm_fb_destroy(struct drm_framebuffer *fb) > > +{ > > +       struct samsung_drm_fb *samsung_fb = to_samsung_fb(fb); > > +       int ret; Get rid of 'ret' > > + > > +       DRM_DEBUG_KMS("%s\n", __FILE__); > > + > > +       drm_framebuffer_cleanup(fb); > > + > > +       if (samsung_fb->is_default) { > > +               ret = drm_gem_handle_delete(samsung_fb->file_priv, > > +                               samsung_fb->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) > > > > +               if (ret < 0) > > +                       DRM_ERROR("failed to delete drm_gem_handle.\n"); And just do the check on the function return value here. You are not using the 'ret' for anything. > > +       } > > + > > +       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.. 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..