From mboxrd@z Thu Jan 1 00:00:00 1970 From: Inki Dae Subject: RE: [RFC][PATCH v3] DRM: add DRM Driver for Samsung SoC EXYNOS4210. Date: Wed, 31 Aug 2011 16:33:30 +0900 Message-ID: <001101cc67b0$47b63410$d7229c30$%dae@samsung.com> References: <1314359274-21585-1-git-send-email-inki.dae@samsung.com> <20110831020652.GA16547@dumpdata.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: In-reply-to: <20110831020652.GA16547@dumpdata.com> Content-language: ko List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: 'Konrad Rzeszutek Wilk' , 'Rob Clark' Cc: linux-arm-kernel@lists.infradead.org, kyungmin.park@samsung.com, sw0312.kim@samsung.com, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org Hello, Konrad Rzeszutek Wilk. > -----Original Message----- > From: Konrad Rzeszutek Wilk [mailto:konrad.wilk@oracle.com] > Sent: Wednesday, August 31, 2011 11:07 AM > 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. > = > > > + =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->p= addr, 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->va= ddr, entry->paddr, entry- > >size); > > > + > > > + =A0 =A0 =A0 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 { > > > + =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 *). > = Ok, I will correct it right now. thank you. > .. snip.. > > > +static int samsung_drm_connector_get_modes(struct drm_connector > *connector) > = > Why not make the return be 'unsigned int'? > = Yes, I think so, but please, see drm_connector_helper_funcs structure of drm_crtc_helper.h. get_modes callback has int type as return. > .. snip.. > > > +/* get detection status of display device. */ > > > +static enum drm_connector_status > > > +samsung_drm_connector_detect(struct drm_connector *connector, bool > force) > > > +{ > > > + =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_connect= or->encoder)- > >display; > > > + =A0 =A0 =A0 unsigned int ret =3D connector_status_unknown; > = > Not 'enum drm_connector_status ret =3D connector_status_unknown' ? > = Oh, you are right, I will fix up it. thank you. > > > + > > > + =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_statu= s_connected; > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 else > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ret =3D connector_statu= s_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' It seems that it doesn't need 'ret' but it needs to check 'ret' because of drm_gem_handle_delete(). > > > + > > > + =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_f= b->file_priv, > > > + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 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) > > > > > > > + =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 de= lete drm_gem_handle.\n"); > = > And just do the check on the function return value here. You are not using > the 'ret' for anything. Yes, right. it just prints out error message because drm_gem_handle_delete function which is mainline function doesn't leave any error message. anyway using 'ret' is not clear. so I will remove 'ret' and check drm_gem_handle_delete function directly instead to print out error message. > > > + =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.. Thank you for your comments. it's been very useful. please give me your comments and advices anytime then I will be pleased.