Re: [PATCH 08/24] drm/mtk: Promote impossible internal error to WARN_ON

2018-05-18 Thread CK Hu 
On Thu, 2018-05-17 at 16:55 +0200, Thierry Reding wrote:
> On Thu, May 17, 2018 at 09:58:19AM -0400, Sean Paul wrote:
> > On Fri, Mar 30, 2018 at 03:11:22PM +0100, Daniel Stone wrote:
> > > A FB with no object is something we should be shouting very loudly
> > > about, not quietly logging as debug.
> > > 
> > > Signed-off-by: Daniel Stone 
> > > Cc: CK Hu 
> > > Cc: Philipp Zabel 
> > > ---
> > >  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 5 +
> > >  1 file changed, 1 insertion(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c 
> > > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > > index 2f4b0ffee598..ac010365d88b 100644
> > > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > > @@ -95,10 +95,7 @@ static int mtk_plane_atomic_check(struct drm_plane 
> > > *plane,
> > >   if (!fb)
> > >   return 0;
> > >  
> > > - if (!mtk_fb_get_gem_obj(fb)) {
> > > - DRM_DEBUG_KMS("buffer is null\n");
> > > - return -EFAULT;
> > > - }
> > > + WARN_ON(!mtk_fb_get_gem_obj(fb));
> > 
> > We should presumably still bail out with an error, no?
> 
> I think we should just remove this WARN_ON(). Under what circumstances
> would this case even happen? If the GEM object for a framebuffer doesn't
> exist, then mtk_drm_mode_fb_create() will fail and no pointer to struct
> drm_framebuffer will ever be returned. After that, the GEM object is
> guaranteed to be there, so the above is effectively dead code.
> 
> Thierry

I agree with Thierry. The case should not happen. So just remove this
checking.

Regards,
CK

> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/24] drm/mtk: Promote impossible internal error to WARN_ON

2018-05-17 Thread Thierry Reding 
On Thu, May 17, 2018 at 09:58:19AM -0400, Sean Paul wrote:
> On Fri, Mar 30, 2018 at 03:11:22PM +0100, Daniel Stone wrote:
> > A FB with no object is something we should be shouting very loudly
> > about, not quietly logging as debug.
> > 
> > Signed-off-by: Daniel Stone 
> > Cc: CK Hu 
> > Cc: Philipp Zabel 
> > ---
> >  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 5 +
> >  1 file changed, 1 insertion(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c 
> > b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > index 2f4b0ffee598..ac010365d88b 100644
> > --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> > @@ -95,10 +95,7 @@ static int mtk_plane_atomic_check(struct drm_plane 
> > *plane,
> > if (!fb)
> > return 0;
> >  
> > -   if (!mtk_fb_get_gem_obj(fb)) {
> > -   DRM_DEBUG_KMS("buffer is null\n");
> > -   return -EFAULT;
> > -   }
> > +   WARN_ON(!mtk_fb_get_gem_obj(fb));
> 
> We should presumably still bail out with an error, no?

I think we should just remove this WARN_ON(). Under what circumstances
would this case even happen? If the GEM object for a framebuffer doesn't
exist, then mtk_drm_mode_fb_create() will fail and no pointer to struct
drm_framebuffer will ever be returned. After that, the GEM object is
guaranteed to be there, so the above is effectively dead code.

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/24] drm/mtk: Promote impossible internal error to WARN_ON

2018-05-17 Thread Sean Paul 
On Fri, Mar 30, 2018 at 03:11:22PM +0100, Daniel Stone wrote:
> A FB with no object is something we should be shouting very loudly
> about, not quietly logging as debug.
> 
> Signed-off-by: Daniel Stone 
> Cc: CK Hu 
> Cc: Philipp Zabel 
> ---
>  drivers/gpu/drm/mediatek/mtk_drm_plane.c | 5 +
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c 
> b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> index 2f4b0ffee598..ac010365d88b 100644
> --- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> +++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
> @@ -95,10 +95,7 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
>   if (!fb)
>   return 0;
>  
> - if (!mtk_fb_get_gem_obj(fb)) {
> - DRM_DEBUG_KMS("buffer is null\n");
> - return -EFAULT;
> - }
> + WARN_ON(!mtk_fb_get_gem_obj(fb));

We should presumably still bail out with an error, no?

>  
>   if (!state->crtc)
>   return 0;
> -- 
> 2.16.2
> 
> ___
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Sean Paul, Software Engineer, Google / Chromium OS
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/24] drm/mtk: Promote impossible internal error to WARN_ON

2018-03-30 Thread Daniel Stone 
A FB with no object is something we should be shouting very loudly
about, not quietly logging as debug.

Signed-off-by: Daniel Stone 
Cc: CK Hu 
Cc: Philipp Zabel 
---
 drivers/gpu/drm/mediatek/mtk_drm_plane.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/mediatek/mtk_drm_plane.c 
b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
index 2f4b0ffee598..ac010365d88b 100644
--- a/drivers/gpu/drm/mediatek/mtk_drm_plane.c
+++ b/drivers/gpu/drm/mediatek/mtk_drm_plane.c
@@ -95,10 +95,7 @@ static int mtk_plane_atomic_check(struct drm_plane *plane,
if (!fb)
return 0;
 
-   if (!mtk_fb_get_gem_obj(fb)) {
-   DRM_DEBUG_KMS("buffer is null\n");
-   return -EFAULT;
-   }
+   WARN_ON(!mtk_fb_get_gem_obj(fb));
 
if (!state->crtc)
return 0;
-- 
2.16.2

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel