diff mbox

[24/37] drm/i915: Eliminate the ugly 'fb?:' constructs from the ilk/skl wm code

Message ID 1479498793-31021-25-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä Nov. 18, 2016, 7:53 p.m. UTC 
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Don't access plane_state->fb until we know the plane to be visible.
It it's visible, it will have an fb, and thus we don't have to
consider the NULL fb case. Makes the code look nicer.

Cc: intel-gfx@lists.freedesktop.org
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

Comments

Daniel Vetter Nov. 30, 2016, 3:51 p.m. UTC | #1 
On Fri, Nov 18, 2016 at 09:53:00PM +0200, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> Don't access plane_state->fb until we know the plane to be visible.
> It it's visible, it will have an fb, and thus we don't have to
> consider the NULL fb case. Makes the code look nicer.
> 
> Cc: intel-gfx@lists.freedesktop.org
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index bbb1eaf1e6db..8ba7413872dd 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1781,13 +1781,14 @@ static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate,
>  				   uint32_t mem_value,
>  				   bool is_lp)
>  {
> -	int cpp = pstate->base.fb ?
> -		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
>  	uint32_t method1, method2;
> +	int cpp;
>  
>  	if (!cstate->base.active || !pstate->base.visible)
>  		return 0;

Why do we still look for crtc_state->active here? Sounds like a bug in
proper validating our wm needs. Anway, change itself looks good.

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

>  
> +	cpp = drm_format_plane_cpp(pstate->base.fb->pixel_format, 0);
> +
>  	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value);
>  
>  	if (!is_lp)
> @@ -1809,13 +1810,14 @@ static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate,
>  				   const struct intel_plane_state *pstate,
>  				   uint32_t mem_value)
>  {
> -	int cpp = pstate->base.fb ?
> -		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
>  	uint32_t method1, method2;
> +	int cpp;
>  
>  	if (!cstate->base.active || !pstate->base.visible)
>  		return 0;
>  
> +	cpp = drm_format_plane_cpp(pstate->base.fb->pixel_format, 0);
> +
>  	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value);
>  	method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
>  				 cstate->base.adjusted_mode.crtc_htotal,
> @@ -1853,12 +1855,13 @@ static uint32_t ilk_compute_fbc_wm(const struct intel_crtc_state *cstate,
>  				   const struct intel_plane_state *pstate,
>  				   uint32_t pri_val)
>  {
> -	int cpp = pstate->base.fb ?
> -		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
> +	int cpp;
>  
>  	if (!cstate->base.active || !pstate->base.visible)
>  		return 0;
>  
> +	cpp = drm_format_plane_cpp(pstate->base.fb->pixel_format, 0);
> +
>  	return ilk_wm_fbc(pri_val, drm_rect_width(&pstate->base.dst), cpp);
>  }
>  
> @@ -3229,13 +3232,17 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
>  			     int y)
>  {
>  	struct intel_plane_state *intel_pstate = to_intel_plane_state(pstate);
> -	struct drm_framebuffer *fb = pstate->fb;
>  	uint32_t down_scale_amount, data_rate;
>  	uint32_t width = 0, height = 0;
> -	unsigned format = fb ? fb->pixel_format : DRM_FORMAT_XRGB8888;
> +	struct drm_framebuffer *fb;
> +	u32 format;
>  
>  	if (!intel_pstate->base.visible)
>  		return 0;
> +
> +	fb = pstate->fb;
> +	format = fb->pixel_format;
> +
>  	if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR)
>  		return 0;
>  	if (y && format != DRM_FORMAT_NV12)
> -- 
> 2.7.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Ville Syrjälä Nov. 30, 2016, 3:59 p.m. UTC | #2 
On Wed, Nov 30, 2016 at 04:51:33PM +0100, Daniel Vetter wrote:
> On Fri, Nov 18, 2016 at 09:53:00PM +0200, ville.syrjala@linux.intel.com wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Don't access plane_state->fb until we know the plane to be visible.
> > It it's visible, it will have an fb, and thus we don't have to
> > consider the NULL fb case. Makes the code look nicer.
> > 
> > Cc: intel-gfx@lists.freedesktop.org
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 23 +++++++++++++++--------
> >  1 file changed, 15 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index bbb1eaf1e6db..8ba7413872dd 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -1781,13 +1781,14 @@ static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate,
> >  				   uint32_t mem_value,
> >  				   bool is_lp)
> >  {
> > -	int cpp = pstate->base.fb ?
> > -		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
> >  	uint32_t method1, method2;
> > +	int cpp;
> >  
> >  	if (!cstate->base.active || !pstate->base.visible)
> >  		return 0;
> 
> Why do we still look for crtc_state->active here? Sounds like a bug in
> proper validating our wm needs.

Yeah, unfortunately that thing is still broken all over :( There are
broken assumptions about this higher up as well, so fixing this will
involve actual work I fear.

> Anway, change itself looks good.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> >  
> > +	cpp = drm_format_plane_cpp(pstate->base.fb->pixel_format, 0);
> > +
> >  	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value);
> >  
> >  	if (!is_lp)
> > @@ -1809,13 +1810,14 @@ static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate,
> >  				   const struct intel_plane_state *pstate,
> >  				   uint32_t mem_value)
> >  {
> > -	int cpp = pstate->base.fb ?
> > -		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
> >  	uint32_t method1, method2;
> > +	int cpp;
> >  
> >  	if (!cstate->base.active || !pstate->base.visible)
> >  		return 0;
> >  
> > +	cpp = drm_format_plane_cpp(pstate->base.fb->pixel_format, 0);
> > +
> >  	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value);
> >  	method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
> >  				 cstate->base.adjusted_mode.crtc_htotal,
> > @@ -1853,12 +1855,13 @@ static uint32_t ilk_compute_fbc_wm(const struct intel_crtc_state *cstate,
> >  				   const struct intel_plane_state *pstate,
> >  				   uint32_t pri_val)
> >  {
> > -	int cpp = pstate->base.fb ?
> > -		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
> > +	int cpp;
> >  
> >  	if (!cstate->base.active || !pstate->base.visible)
> >  		return 0;
> >  
> > +	cpp = drm_format_plane_cpp(pstate->base.fb->pixel_format, 0);
> > +
> >  	return ilk_wm_fbc(pri_val, drm_rect_width(&pstate->base.dst), cpp);
> >  }
> >  
> > @@ -3229,13 +3232,17 @@ skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
> >  			     int y)
> >  {
> >  	struct intel_plane_state *intel_pstate = to_intel_plane_state(pstate);
> > -	struct drm_framebuffer *fb = pstate->fb;
> >  	uint32_t down_scale_amount, data_rate;
> >  	uint32_t width = 0, height = 0;
> > -	unsigned format = fb ? fb->pixel_format : DRM_FORMAT_XRGB8888;
> > +	struct drm_framebuffer *fb;
> > +	u32 format;
> >  
> >  	if (!intel_pstate->base.visible)
> >  		return 0;
> > +
> > +	fb = pstate->fb;
> > +	format = fb->pixel_format;
> > +
> >  	if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR)
> >  		return 0;
> >  	if (y && format != DRM_FORMAT_NV12)
> > -- 
> > 2.7.4
> > 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index bbb1eaf1e6db..8ba7413872dd 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1781,13 +1781,14 @@  static uint32_t ilk_compute_pri_wm(const struct intel_crtc_state *cstate,
 				   uint32_t mem_value,
 				   bool is_lp)
 {
-	int cpp = pstate->base.fb ?
-		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
 	uint32_t method1, method2;
+	int cpp;
 
 	if (!cstate->base.active || !pstate->base.visible)
 		return 0;
 
+	cpp = drm_format_plane_cpp(pstate->base.fb->pixel_format, 0);
+
 	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value);
 
 	if (!is_lp)
@@ -1809,13 +1810,14 @@  static uint32_t ilk_compute_spr_wm(const struct intel_crtc_state *cstate,
 				   const struct intel_plane_state *pstate,
 				   uint32_t mem_value)
 {
-	int cpp = pstate->base.fb ?
-		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
 	uint32_t method1, method2;
+	int cpp;
 
 	if (!cstate->base.active || !pstate->base.visible)
 		return 0;
 
+	cpp = drm_format_plane_cpp(pstate->base.fb->pixel_format, 0);
+
 	method1 = ilk_wm_method1(ilk_pipe_pixel_rate(cstate), cpp, mem_value);
 	method2 = ilk_wm_method2(ilk_pipe_pixel_rate(cstate),
 				 cstate->base.adjusted_mode.crtc_htotal,
@@ -1853,12 +1855,13 @@  static uint32_t ilk_compute_fbc_wm(const struct intel_crtc_state *cstate,
 				   const struct intel_plane_state *pstate,
 				   uint32_t pri_val)
 {
-	int cpp = pstate->base.fb ?
-		drm_format_plane_cpp(pstate->base.fb->pixel_format, 0) : 0;
+	int cpp;
 
 	if (!cstate->base.active || !pstate->base.visible)
 		return 0;
 
+	cpp = drm_format_plane_cpp(pstate->base.fb->pixel_format, 0);
+
 	return ilk_wm_fbc(pri_val, drm_rect_width(&pstate->base.dst), cpp);
 }
 
@@ -3229,13 +3232,17 @@  skl_plane_relative_data_rate(const struct intel_crtc_state *cstate,
 			     int y)
 {
 	struct intel_plane_state *intel_pstate = to_intel_plane_state(pstate);
-	struct drm_framebuffer *fb = pstate->fb;
 	uint32_t down_scale_amount, data_rate;
 	uint32_t width = 0, height = 0;
-	unsigned format = fb ? fb->pixel_format : DRM_FORMAT_XRGB8888;
+	struct drm_framebuffer *fb;
+	u32 format;
 
 	if (!intel_pstate->base.visible)
 		return 0;
+
+	fb = pstate->fb;
+	format = fb->pixel_format;
+
 	if (pstate->plane->type == DRM_PLANE_TYPE_CURSOR)
 		return 0;
 	if (y && format != DRM_FORMAT_NV12)