| 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 |
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) > {
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
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 --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) {