diff mbox series

[1/2] drm/i915: Extract intel_adjusted_rate()

Message ID 20210330184254.6290-1-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series [1/2] drm/i915: Extract intel_adjusted_rate() | expand

Commit Message

Ville Syrjälä March 30, 2021, 6:42 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

Extract a small helper to calculate the downscaling
adjusted pixel rate/data rate/etc.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 .../gpu/drm/i915/display/intel_atomic_plane.c | 27 +++++++++++++------
 1 file changed, 19 insertions(+), 8 deletions(-)

Comments

Jani Nikula April 1, 2021, 12:43 p.m. UTC | #1
On Tue, 30 Mar 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> Extract a small helper to calculate the downscaling
> adjusted pixel rate/data rate/etc.
>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
>  .../gpu/drm/i915/display/intel_atomic_plane.c | 27 +++++++++++++------
>  1 file changed, 19 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> index c3f2962aa1eb..3f830b70b0c1 100644
> --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> @@ -133,25 +133,36 @@ intel_plane_destroy_state(struct drm_plane *plane,
>  	kfree(plane_state);
>  }
>  
> -unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
> -				    const struct intel_plane_state *plane_state)
> +static unsigned int intel_adjusted_rate(const struct drm_rect *src,
> +					const struct drm_rect *dst,
> +					unsigned int rate)
>  {
>  	unsigned int src_w, src_h, dst_w, dst_h;
> -	unsigned int pixel_rate = crtc_state->pixel_rate;
>  
> -	src_w = drm_rect_width(&plane_state->uapi.src) >> 16;
> -	src_h = drm_rect_height(&plane_state->uapi.src) >> 16;
> -	dst_w = drm_rect_width(&plane_state->uapi.dst);
> -	dst_h = drm_rect_height(&plane_state->uapi.dst);
> +	src_w = drm_rect_width(src) >> 16;
> +	src_h = drm_rect_height(src) >> 16;
> +	dst_w = drm_rect_width(dst);
> +	dst_h = drm_rect_height(dst);
>  
>  	/* Downscaling limits the maximum pixel rate */
>  	dst_w = min(src_w, dst_w);
>  	dst_h = min(src_h, dst_h);
>  
> -	return DIV_ROUND_UP_ULL(mul_u32_u32(pixel_rate, src_w * src_h),
> +	return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h),
>  				dst_w * dst_h);
>  }
>  
> +unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
> +				    const struct intel_plane_state *plane_state)
> +{
> +	if (!plane_state->uapi.visible)

Potential functional change not covered in the commit message? Makes
sense, but the rabbit hole is too deep to find out if this could
actually make a difference.

If mentioned in the commit message,

Reviewed-by: Jani Nikula <jani.nikula@intel.com>


> +		return 0;
> +
> +	return intel_adjusted_rate(&plane_state->uapi.src,
> +				   &plane_state->uapi.dst,
> +				   crtc_state->pixel_rate);
> +}
> +
>  unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state,
>  				   const struct intel_plane_state *plane_state)
>  {
Ville Syrjälä April 1, 2021, 2:32 p.m. UTC | #2
On Thu, Apr 01, 2021 at 03:43:37PM +0300, Jani Nikula wrote:
> On Tue, 30 Mar 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > Extract a small helper to calculate the downscaling
> > adjusted pixel rate/data rate/etc.
> >
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  .../gpu/drm/i915/display/intel_atomic_plane.c | 27 +++++++++++++------
> >  1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > index c3f2962aa1eb..3f830b70b0c1 100644
> > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > @@ -133,25 +133,36 @@ intel_plane_destroy_state(struct drm_plane *plane,
> >  	kfree(plane_state);
> >  }
> >  
> > -unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
> > -				    const struct intel_plane_state *plane_state)
> > +static unsigned int intel_adjusted_rate(const struct drm_rect *src,
> > +					const struct drm_rect *dst,
> > +					unsigned int rate)
> >  {
> >  	unsigned int src_w, src_h, dst_w, dst_h;
> > -	unsigned int pixel_rate = crtc_state->pixel_rate;
> >  
> > -	src_w = drm_rect_width(&plane_state->uapi.src) >> 16;
> > -	src_h = drm_rect_height(&plane_state->uapi.src) >> 16;
> > -	dst_w = drm_rect_width(&plane_state->uapi.dst);
> > -	dst_h = drm_rect_height(&plane_state->uapi.dst);
> > +	src_w = drm_rect_width(src) >> 16;
> > +	src_h = drm_rect_height(src) >> 16;
> > +	dst_w = drm_rect_width(dst);
> > +	dst_h = drm_rect_height(dst);
> >  
> >  	/* Downscaling limits the maximum pixel rate */
> >  	dst_w = min(src_w, dst_w);
> >  	dst_h = min(src_h, dst_h);
> >  
> > -	return DIV_ROUND_UP_ULL(mul_u32_u32(pixel_rate, src_w * src_h),
> > +	return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h),
> >  				dst_w * dst_h);
> >  }
> >  
> > +unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
> > +				    const struct intel_plane_state *plane_state)
> > +{
> > +	if (!plane_state->uapi.visible)
> 
> Potential functional change not covered in the commit message? Makes
> sense, but the rabbit hole is too deep to find out if this could
> actually make a difference.

This is fine. If the plane isn't visible then it's not
generating any pixels anyway. I think I either had some other
patches originally that wanted this, or I just wanted to make
this safe to call at any point without checking for plane
visibility in the caller. But IIRC I dropped those other
patches and so this might not be necessary anymore. I'll double
check and either drop this or amend the commit msg a bit.

> 
> If mentioned in the commit message,
> 
> Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> 
> 
> > +		return 0;
> > +
> > +	return intel_adjusted_rate(&plane_state->uapi.src,
> > +				   &plane_state->uapi.dst,
> > +				   crtc_state->pixel_rate);
> > +}
> > +
> >  unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state,
> >  				   const struct intel_plane_state *plane_state)
> >  {
> 
> -- 
> Jani Nikula, Intel Open Source Graphics Center
Ville Syrjälä April 1, 2021, 3:21 p.m. UTC | #3
On Thu, Apr 01, 2021 at 05:32:20PM +0300, Ville Syrjälä wrote:
> On Thu, Apr 01, 2021 at 03:43:37PM +0300, Jani Nikula wrote:
> > On Tue, 30 Mar 2021, Ville Syrjala <ville.syrjala@linux.intel.com> wrote:
> > > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > >
> > > Extract a small helper to calculate the downscaling
> > > adjusted pixel rate/data rate/etc.
> > >
> > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > > ---
> > >  .../gpu/drm/i915/display/intel_atomic_plane.c | 27 +++++++++++++------
> > >  1 file changed, 19 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > index c3f2962aa1eb..3f830b70b0c1 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
> > > @@ -133,25 +133,36 @@ intel_plane_destroy_state(struct drm_plane *plane,
> > >  	kfree(plane_state);
> > >  }
> > >  
> > > -unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
> > > -				    const struct intel_plane_state *plane_state)
> > > +static unsigned int intel_adjusted_rate(const struct drm_rect *src,
> > > +					const struct drm_rect *dst,
> > > +					unsigned int rate)
> > >  {
> > >  	unsigned int src_w, src_h, dst_w, dst_h;
> > > -	unsigned int pixel_rate = crtc_state->pixel_rate;
> > >  
> > > -	src_w = drm_rect_width(&plane_state->uapi.src) >> 16;
> > > -	src_h = drm_rect_height(&plane_state->uapi.src) >> 16;
> > > -	dst_w = drm_rect_width(&plane_state->uapi.dst);
> > > -	dst_h = drm_rect_height(&plane_state->uapi.dst);
> > > +	src_w = drm_rect_width(src) >> 16;
> > > +	src_h = drm_rect_height(src) >> 16;
> > > +	dst_w = drm_rect_width(dst);
> > > +	dst_h = drm_rect_height(dst);
> > >  
> > >  	/* Downscaling limits the maximum pixel rate */
> > >  	dst_w = min(src_w, dst_w);
> > >  	dst_h = min(src_h, dst_h);
> > >  
> > > -	return DIV_ROUND_UP_ULL(mul_u32_u32(pixel_rate, src_w * src_h),
> > > +	return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h),
> > >  				dst_w * dst_h);
> > >  }
> > >  
> > > +unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
> > > +				    const struct intel_plane_state *plane_state)
> > > +{
> > > +	if (!plane_state->uapi.visible)
> > 
> > Potential functional change not covered in the commit message? Makes
> > sense, but the rabbit hole is too deep to find out if this could
> > actually make a difference.
> 
> This is fine. If the plane isn't visible then it's not
> generating any pixels anyway. I think I either had some other
> patches originally that wanted this, or I just wanted to make
> this safe to call at any point without checking for plane
> visibility in the caller. But IIRC I dropped those other
> patches and so this might not be necessary anymore. I'll double
> check and either drop this or amend the commit msg a bit.

Actually the one case where this might matter a bit is a
fully offscreen cursor plane. The watermark claculations
consider the cursor visible in that case (to avoid redundant
watermark updates when the cursor pops in and out of visibility
due to going offscreen). So in fact we want it to have a
non-zero pixel rate or else the watermarks will just come out
as zero.
 
Fortunately intel_check_cursor() does populate the plane 
src/dst rectangles correctly even in that case. Normally
!visible implies that the src/dst rectangles don't
contain data we can use.

So I'll drop the check and toss in a comment somewhere to
remind us of the facts for the next time...

> 
> > 
> > If mentioned in the commit message,
> > 
> > Reviewed-by: Jani Nikula <jani.nikula@intel.com>
> > 
> > 
> > > +		return 0;
> > > +
> > > +	return intel_adjusted_rate(&plane_state->uapi.src,
> > > +				   &plane_state->uapi.dst,
> > > +				   crtc_state->pixel_rate);
> > > +}
> > > +
> > >  unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state,
> > >  				   const struct intel_plane_state *plane_state)
> > >  {
> > 
> > -- 
> > Jani Nikula, Intel Open Source Graphics Center
> 
> -- 
> Ville Syrjälä
> Intel
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff mbox series

Patch

diff --git a/drivers/gpu/drm/i915/display/intel_atomic_plane.c b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
index c3f2962aa1eb..3f830b70b0c1 100644
--- a/drivers/gpu/drm/i915/display/intel_atomic_plane.c
+++ b/drivers/gpu/drm/i915/display/intel_atomic_plane.c
@@ -133,25 +133,36 @@  intel_plane_destroy_state(struct drm_plane *plane,
 	kfree(plane_state);
 }
 
-unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
-				    const struct intel_plane_state *plane_state)
+static unsigned int intel_adjusted_rate(const struct drm_rect *src,
+					const struct drm_rect *dst,
+					unsigned int rate)
 {
 	unsigned int src_w, src_h, dst_w, dst_h;
-	unsigned int pixel_rate = crtc_state->pixel_rate;
 
-	src_w = drm_rect_width(&plane_state->uapi.src) >> 16;
-	src_h = drm_rect_height(&plane_state->uapi.src) >> 16;
-	dst_w = drm_rect_width(&plane_state->uapi.dst);
-	dst_h = drm_rect_height(&plane_state->uapi.dst);
+	src_w = drm_rect_width(src) >> 16;
+	src_h = drm_rect_height(src) >> 16;
+	dst_w = drm_rect_width(dst);
+	dst_h = drm_rect_height(dst);
 
 	/* Downscaling limits the maximum pixel rate */
 	dst_w = min(src_w, dst_w);
 	dst_h = min(src_h, dst_h);
 
-	return DIV_ROUND_UP_ULL(mul_u32_u32(pixel_rate, src_w * src_h),
+	return DIV_ROUND_UP_ULL(mul_u32_u32(rate, src_w * src_h),
 				dst_w * dst_h);
 }
 
+unsigned int intel_plane_pixel_rate(const struct intel_crtc_state *crtc_state,
+				    const struct intel_plane_state *plane_state)
+{
+	if (!plane_state->uapi.visible)
+		return 0;
+
+	return intel_adjusted_rate(&plane_state->uapi.src,
+				   &plane_state->uapi.dst,
+				   crtc_state->pixel_rate);
+}
+
 unsigned int intel_plane_data_rate(const struct intel_crtc_state *crtc_state,
 				   const struct intel_plane_state *plane_state)
 {