| Message ID | 1443023547-19896-5-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
s/Export/Extract/ Export made me think you wanted to use it from another file. On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote: > Make the giant function a little less giant ...and make it a little more self-documenting by refactoring the valid size predicate.. > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++------------- > 1 file changed, 22 insertions(+), 13 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index 0a6b454..0c7b59b 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb) > } > } > > +static bool size_is_valid(struct intel_crtc *crtc) Which size? pipe_size_fits_in_fbc() pipe_size_valid() pipe_size_fits() I think I prefer pipe_size_valid(). Naming quibles aside (but admittedly that is the purpose of the patch!), Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
On Wed, Sep 23, 2015 at 06:09:04PM +0100, Chris Wilson wrote: > s/Export/Extract/ > > Export made me think you wanted to use it from another file. > > On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote: > > Make the giant function a little less giant > > ...and make it a little more self-documenting by refactoring the > valid size predicate.. > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > --- > > drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++------------- > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > > index 0a6b454..0c7b59b 100644 > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb) > > } > > } > > > > +static bool size_is_valid(struct intel_crtc *crtc) > > Which size? > > pipe_size_fits_in_fbc() > pipe_size_valid() > pipe_size_fits() > > I think I prefer pipe_size_valid(). Painted while applying, thanks. -Daniel > > Naming quibles aside (but admittedly that is the purpose of the patch!), > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> Yea > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Sep 28, 2015 at 10:59:13AM +0200, Daniel Vetter wrote: > On Wed, Sep 23, 2015 at 06:09:04PM +0100, Chris Wilson wrote: > > s/Export/Extract/ > > > > Export made me think you wanted to use it from another file. > > > > On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote: > > > Make the giant function a little less giant > > > > ...and make it a little more self-documenting by refactoring the > > valid size predicate.. > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > --- > > > drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++------------- > > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > > > index 0a6b454..0c7b59b 100644 > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb) > > > } > > > } > > > > > > +static bool size_is_valid(struct intel_crtc *crtc) > > > > Which size? > > > > pipe_size_fits_in_fbc() > > pipe_size_valid() > > pipe_size_fits() > > > > I think I prefer pipe_size_valid(). > > Painted while applying, thanks. And it's not what we really want. We should be looking at the plane src size. > -Daniel > > > > > Naming quibles aside (but admittedly that is the purpose of the patch!), > > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > > Yea > > -Chris > > > > -- > > Chris Wilson, Intel Open Source Technology Centre > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Daniel Vetter > Software Engineer, Intel Corporation > http://blog.ffwll.ch > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-09-28 9:47 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > On Mon, Sep 28, 2015 at 10:59:13AM +0200, Daniel Vetter wrote: >> On Wed, Sep 23, 2015 at 06:09:04PM +0100, Chris Wilson wrote: >> > s/Export/Extract/ >> > >> > Export made me think you wanted to use it from another file. >> > >> > On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote: >> > > Make the giant function a little less giant >> > >> > ...and make it a little more self-documenting by refactoring the >> > valid size predicate.. >> > >> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> > > --- >> > > drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++------------- >> > > 1 file changed, 22 insertions(+), 13 deletions(-) >> > > >> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >> > > index 0a6b454..0c7b59b 100644 >> > > --- a/drivers/gpu/drm/i915/intel_fbc.c >> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c >> > > @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb) >> > > } >> > > } >> > > >> > > +static bool size_is_valid(struct intel_crtc *crtc) >> > >> > Which size? >> > >> > pipe_size_fits_in_fbc() >> > pipe_size_valid() >> > pipe_size_fits() >> > >> > I think I prefer pipe_size_valid(). >> >> Painted while applying, thanks. > > And it's not what we really want. We should be looking at the plane src size. I can fix the name on patch 5/7 if we need. > >> -Daniel >> >> > >> > Naming quibles aside (but admittedly that is the purpose of the patch!), >> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> >> >> Yea >> > -Chris >> > >> > -- >> > Chris Wilson, Intel Open Source Technology Centre >> > _______________________________________________ >> > Intel-gfx mailing list >> > Intel-gfx@lists.freedesktop.org >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx >> >> -- >> Daniel Vetter >> Software Engineer, Intel Corporation >> http://blog.ffwll.ch >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > -- > Ville Syrjälä > Intel OTC > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Sep 28, 2015 at 03:47:55PM +0300, Ville Syrjälä wrote: > On Mon, Sep 28, 2015 at 10:59:13AM +0200, Daniel Vetter wrote: > > On Wed, Sep 23, 2015 at 06:09:04PM +0100, Chris Wilson wrote: > > > s/Export/Extract/ > > > > > > Export made me think you wanted to use it from another file. > > > > > > On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote: > > > > Make the giant function a little less giant > > > > > > ...and make it a little more self-documenting by refactoring the > > > valid size predicate.. > > > > > > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > --- > > > > drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++------------- > > > > 1 file changed, 22 insertions(+), 13 deletions(-) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > > > > index 0a6b454..0c7b59b 100644 > > > > --- a/drivers/gpu/drm/i915/intel_fbc.c > > > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > > @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb) > > > > } > > > > } > > > > > > > > +static bool size_is_valid(struct intel_crtc *crtc) > > > > > > Which size? > > > > > > pipe_size_fits_in_fbc() > > > pipe_size_valid() > > > pipe_size_fits() > > > > > > I think I prefer pipe_size_valid(). > > > > Painted while applying, thanks. > > And it's not what we really want. We should be looking at the plane src size. Well it also lookst at intel_crtc->config and not crtc->state so not perfect from that pov either. And since it's really just a 1:1 refactor I figured I can pull this before we figure out the exact details - we should be able to get at everything interesting through the intel_crtc. -Daniel
On Mon, Sep 28, 2015 at 10:13:05AM -0300, Paulo Zanoni wrote: > 2015-09-28 9:47 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > > On Mon, Sep 28, 2015 at 10:59:13AM +0200, Daniel Vetter wrote: > >> On Wed, Sep 23, 2015 at 06:09:04PM +0100, Chris Wilson wrote: > >> > s/Export/Extract/ > >> > > >> > Export made me think you wanted to use it from another file. > >> > > >> > On Wed, Sep 23, 2015 at 12:52:24PM -0300, Paulo Zanoni wrote: > >> > > Make the giant function a little less giant > >> > > >> > ...and make it a little more self-documenting by refactoring the > >> > valid size predicate.. > >> > > >> > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > > --- > >> > > drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++------------- > >> > > 1 file changed, 22 insertions(+), 13 deletions(-) > >> > > > >> > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > >> > > index 0a6b454..0c7b59b 100644 > >> > > --- a/drivers/gpu/drm/i915/intel_fbc.c > >> > > +++ b/drivers/gpu/drm/i915/intel_fbc.c > >> > > @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb) > >> > > } > >> > > } > >> > > > >> > > +static bool size_is_valid(struct intel_crtc *crtc) > >> > > >> > Which size? > >> > > >> > pipe_size_fits_in_fbc() > >> > pipe_size_valid() > >> > pipe_size_fits() > >> > > >> > I think I prefer pipe_size_valid(). > >> > >> Painted while applying, thanks. > > > > And it's not what we really want. We should be looking at the plane src size. > > I can fix the name on patch 5/7 if we need. Yeah, renaming when we actually start to check the plane size would seem more appropriate. > > > > >> -Daniel > >> > >> > > >> > Naming quibles aside (but admittedly that is the purpose of the patch!), > >> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > >> > >> Yea > >> > -Chris > >> > > >> > -- > >> > Chris Wilson, Intel Open Source Technology Centre > >> > _______________________________________________ > >> > Intel-gfx mailing list > >> > Intel-gfx@lists.freedesktop.org > >> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > >> > >> -- > >> Daniel Vetter > >> Software Engineer, Intel Corporation > >> http://blog.ffwll.ch > >> _______________________________________________ > >> Intel-gfx mailing list > >> Intel-gfx@lists.freedesktop.org > >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > > Ville Syrjälä > > Intel OTC > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index 0a6b454..0c7b59b 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -755,6 +755,26 @@ static bool pixel_format_is_valid(struct drm_framebuffer *fb) } } +static bool size_is_valid(struct intel_crtc *crtc) +{ + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; + unsigned int max_w, max_h; + + if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) { + max_w = 4096; + max_h = 4096; + } else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) { + max_w = 4096; + max_h = 2048; + } else { + max_w = 2048; + max_h = 1536; + } + + return crtc->config->pipe_src_w <= max_w && + crtc->config->pipe_src_h <= max_h; +} + /** * __intel_fbc_update - enable/disable FBC as needed, unlocked * @dev_priv: i915 device instance @@ -781,7 +801,6 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) struct drm_framebuffer *fb; struct drm_i915_gem_object *obj; const struct drm_display_mode *adjusted_mode; - unsigned int max_width, max_height; WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock)); @@ -830,21 +849,11 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) goto out_disable; } - if (INTEL_INFO(dev_priv)->gen >= 8 || IS_HASWELL(dev_priv)) { - max_width = 4096; - max_height = 4096; - } else if (IS_G4X(dev_priv) || INTEL_INFO(dev_priv)->gen >= 5) { - max_width = 4096; - max_height = 2048; - } else { - max_width = 2048; - max_height = 1536; - } - if (intel_crtc->config->pipe_src_w > max_width || - intel_crtc->config->pipe_src_h > max_height) { + if (!size_is_valid(intel_crtc)) { set_no_fbc_reason(dev_priv, FBC_MODE_TOO_LARGE); goto out_disable; } + if ((INTEL_INFO(dev_priv)->gen < 4 || HAS_DDI(dev_priv)) && intel_crtc->plane != PLANE_A) { set_no_fbc_reason(dev_priv, FBC_BAD_PLANE);
Make the giant function a little less giant. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_fbc.c | 35 ++++++++++++++++++++++------------- 1 file changed, 22 insertions(+), 13 deletions(-)