| Message ID | 1439588061-18064-3-git-send-email-paulo.r.zanoni@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote: > Always update the currrent crtc, fb and vertical offset after calling > enable_fbc. We were forgetting to do so along the failure paths when > enabling fbc synchronously. Fix this with a new helper to enable_fbc() > and update the state simultaneously. > > v2: Improve commit message (Chris). > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++---------- > 1 file changed, 17 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > index c97aba2..fa9b004 100644 > --- a/drivers/gpu/drm/i915/intel_fbc.c > +++ b/drivers/gpu/drm/i915/intel_fbc.c > @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv) > return dev_priv->fbc.enabled; > } > > +static void intel_fbc_enable(struct intel_crtc *crtc, > + struct drm_framebuffer *fb) fb could be const > +{ > + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > + > + dev_priv->fbc.enable_fbc(crtc); > + > + dev_priv->fbc.crtc = crtc; > + dev_priv->fbc.fb_id = fb->base.id; > + dev_priv->fbc.y = crtc->base.y; > +} > + > static void intel_fbc_work_fn(struct work_struct *__work) > { > struct intel_fbc_work *work = > @@ -321,13 +333,8 @@ static void intel_fbc_work_fn(struct work_struct *__work) > /* Double check that we haven't switched fb without cancelling > * the prior work. > */ > - if (crtc_fb == work->fb) { > - dev_priv->fbc.enable_fbc(work->crtc); > - > - dev_priv->fbc.crtc = work->crtc; > - dev_priv->fbc.fb_id = crtc_fb->base.id; > - dev_priv->fbc.y = work->crtc->base.y; > - } > + if (crtc_fb == work->fb) > + intel_fbc_enable(work->crtc, work->fb); The no locking or refcounts nature of this scares me, and should be dealt with eventually. But in the meantime it makes things nicer, so Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > dev_priv->fbc.fbc_work = NULL; > } > @@ -361,7 +368,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv) > dev_priv->fbc.fbc_work = NULL; > } > > -static void intel_fbc_enable(struct intel_crtc *crtc) > +static void intel_fbc_schedule_enable(struct intel_crtc *crtc) > { > struct intel_fbc_work *work; > struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > @@ -373,7 +380,7 @@ static void intel_fbc_enable(struct intel_crtc *crtc) > work = kzalloc(sizeof(*work), GFP_KERNEL); > if (work == NULL) { > DRM_ERROR("Failed to allocate FBC work structure\n"); > - dev_priv->fbc.enable_fbc(crtc); > + intel_fbc_enable(crtc, crtc->base.primary->fb); > return; > } BTW getting rid of this allocation would be nice. Would be one less thing that can fail... > > @@ -826,7 +833,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) > __intel_fbc_disable(dev_priv); > } > > - intel_fbc_enable(intel_crtc); > + intel_fbc_schedule_enable(intel_crtc); > dev_priv->fbc.no_fbc_reason = FBC_OK; > return; > > -- > 2.4.6 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
2015-08-28 11:20 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote: >> Always update the currrent crtc, fb and vertical offset after calling >> enable_fbc. We were forgetting to do so along the failure paths when >> enabling fbc synchronously. Fix this with a new helper to enable_fbc() >> and update the state simultaneously. >> >> v2: Improve commit message (Chris). >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++---------- >> 1 file changed, 17 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c >> index c97aba2..fa9b004 100644 >> --- a/drivers/gpu/drm/i915/intel_fbc.c >> +++ b/drivers/gpu/drm/i915/intel_fbc.c >> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv) >> return dev_priv->fbc.enabled; >> } >> >> +static void intel_fbc_enable(struct intel_crtc *crtc, >> + struct drm_framebuffer *fb) > > fb could be const I guess a lot of things could be constified, if we decide to do this. > >> +{ >> + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; >> + >> + dev_priv->fbc.enable_fbc(crtc); >> + >> + dev_priv->fbc.crtc = crtc; >> + dev_priv->fbc.fb_id = fb->base.id; >> + dev_priv->fbc.y = crtc->base.y; >> +} >> + >> static void intel_fbc_work_fn(struct work_struct *__work) >> { >> struct intel_fbc_work *work = >> @@ -321,13 +333,8 @@ static void intel_fbc_work_fn(struct work_struct *__work) >> /* Double check that we haven't switched fb without cancelling >> * the prior work. >> */ >> - if (crtc_fb == work->fb) { >> - dev_priv->fbc.enable_fbc(work->crtc); >> - >> - dev_priv->fbc.crtc = work->crtc; >> - dev_priv->fbc.fb_id = crtc_fb->base.id; >> - dev_priv->fbc.y = work->crtc->base.y; >> - } >> + if (crtc_fb == work->fb) >> + intel_fbc_enable(work->crtc, work->fb); > > The no locking or refcounts nature of this scares me, and should be > dealt with eventually. > > But in the meantime it makes things nicer, so > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > >> >> dev_priv->fbc.fbc_work = NULL; >> } >> @@ -361,7 +368,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv) >> dev_priv->fbc.fbc_work = NULL; >> } >> >> -static void intel_fbc_enable(struct intel_crtc *crtc) >> +static void intel_fbc_schedule_enable(struct intel_crtc *crtc) >> { >> struct intel_fbc_work *work; >> struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; >> @@ -373,7 +380,7 @@ static void intel_fbc_enable(struct intel_crtc *crtc) >> work = kzalloc(sizeof(*work), GFP_KERNEL); >> if (work == NULL) { >> DRM_ERROR("Failed to allocate FBC work structure\n"); >> - dev_priv->fbc.enable_fbc(crtc); >> + intel_fbc_enable(crtc, crtc->base.primary->fb); >> return; >> } > > BTW getting rid of this allocation would be nice. Would be one less > thing that can fail... Chris disagrees :) > >> >> @@ -826,7 +833,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) >> __intel_fbc_disable(dev_priv); >> } >> >> - intel_fbc_enable(intel_crtc); >> + intel_fbc_schedule_enable(intel_crtc); >> dev_priv->fbc.no_fbc_reason = FBC_OK; >> return; >> >> -- >> 2.4.6 >> >> _______________________________________________ >> 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 Fri, Aug 28, 2015 at 11:50:08AM -0300, Paulo Zanoni wrote: > 2015-08-28 11:20 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > > On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote: > >> Always update the currrent crtc, fb and vertical offset after calling > >> enable_fbc. We were forgetting to do so along the failure paths when > >> enabling fbc synchronously. Fix this with a new helper to enable_fbc() > >> and update the state simultaneously. > >> > >> v2: Improve commit message (Chris). > >> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++---------- > >> 1 file changed, 17 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > >> index c97aba2..fa9b004 100644 > >> --- a/drivers/gpu/drm/i915/intel_fbc.c > >> +++ b/drivers/gpu/drm/i915/intel_fbc.c > >> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv) > >> return dev_priv->fbc.enabled; > >> } > >> > >> +static void intel_fbc_enable(struct intel_crtc *crtc, > >> + struct drm_framebuffer *fb) > > > > fb could be const > > I guess a lot of things could be constified, if we decide to do this. > > > > >> +{ > >> + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > >> + > >> + dev_priv->fbc.enable_fbc(crtc); > >> + > >> + dev_priv->fbc.crtc = crtc; > >> + dev_priv->fbc.fb_id = fb->base.id; > >> + dev_priv->fbc.y = crtc->base.y; > >> +} > >> + > >> static void intel_fbc_work_fn(struct work_struct *__work) > >> { > >> struct intel_fbc_work *work = > >> @@ -321,13 +333,8 @@ static void intel_fbc_work_fn(struct work_struct *__work) > >> /* Double check that we haven't switched fb without cancelling > >> * the prior work. > >> */ > >> - if (crtc_fb == work->fb) { > >> - dev_priv->fbc.enable_fbc(work->crtc); > >> - > >> - dev_priv->fbc.crtc = work->crtc; > >> - dev_priv->fbc.fb_id = crtc_fb->base.id; > >> - dev_priv->fbc.y = work->crtc->base.y; > >> - } > >> + if (crtc_fb == work->fb) > >> + intel_fbc_enable(work->crtc, work->fb); > > > > The no locking or refcounts nature of this scares me, and should be > > dealt with eventually. > > > > But in the meantime it makes things nicer, so > > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > >> > >> dev_priv->fbc.fbc_work = NULL; > >> } > >> @@ -361,7 +368,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv) > >> dev_priv->fbc.fbc_work = NULL; > >> } > >> > >> -static void intel_fbc_enable(struct intel_crtc *crtc) > >> +static void intel_fbc_schedule_enable(struct intel_crtc *crtc) > >> { > >> struct intel_fbc_work *work; > >> struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; > >> @@ -373,7 +380,7 @@ static void intel_fbc_enable(struct intel_crtc *crtc) > >> work = kzalloc(sizeof(*work), GFP_KERNEL); > >> if (work == NULL) { > >> DRM_ERROR("Failed to allocate FBC work structure\n"); > >> - dev_priv->fbc.enable_fbc(crtc); > >> + intel_fbc_enable(crtc, crtc->base.primary->fb); > >> return; > >> } > > > > BTW getting rid of this allocation would be nice. Would be one less > > thing that can fail... > > Chris disagrees :) He's wrong ;) > > > > > >> > >> @@ -826,7 +833,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) > >> __intel_fbc_disable(dev_priv); > >> } > >> > >> - intel_fbc_enable(intel_crtc); > >> + intel_fbc_schedule_enable(intel_crtc); > >> dev_priv->fbc.no_fbc_reason = FBC_OK; > >> return; > >> > >> -- > >> 2.4.6 > >> > >> _______________________________________________ > >> 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
On Fri, Aug 28, 2015 at 11:50:08AM -0300, Paulo Zanoni wrote: > 2015-08-28 11:20 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > > On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote: > >> Always update the currrent crtc, fb and vertical offset after calling > >> enable_fbc. We were forgetting to do so along the failure paths when > >> enabling fbc synchronously. Fix this with a new helper to enable_fbc() > >> and update the state simultaneously. > >> > >> v2: Improve commit message (Chris). > >> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> --- > >> drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++---------- > >> 1 file changed, 17 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > >> index c97aba2..fa9b004 100644 > >> --- a/drivers/gpu/drm/i915/intel_fbc.c > >> +++ b/drivers/gpu/drm/i915/intel_fbc.c > >> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv) > >> return dev_priv->fbc.enabled; > >> } > >> > >> +static void intel_fbc_enable(struct intel_crtc *crtc, > >> + struct drm_framebuffer *fb) > > > > fb could be const > > I guess a lot of things could be constified, if we decide to do this. Personally I like const on .data (especially vfunc tables since those are nice to create exploits if they're writable and you can get at them). And for core functions/vfuncs where the const has a semantic meaning. Otherwise I personally don't see to much value in sprinkling const all over. -Daniel
On Tue, Sep 01, 2015 at 12:07:01PM +0200, Daniel Vetter wrote: > On Fri, Aug 28, 2015 at 11:50:08AM -0300, Paulo Zanoni wrote: > > 2015-08-28 11:20 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > > > On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote: > > >> Always update the currrent crtc, fb and vertical offset after calling > > >> enable_fbc. We were forgetting to do so along the failure paths when > > >> enabling fbc synchronously. Fix this with a new helper to enable_fbc() > > >> and update the state simultaneously. > > >> > > >> v2: Improve commit message (Chris). > > >> > > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > >> --- > > >> drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++---------- > > >> 1 file changed, 17 insertions(+), 10 deletions(-) > > >> > > >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > > >> index c97aba2..fa9b004 100644 > > >> --- a/drivers/gpu/drm/i915/intel_fbc.c > > >> +++ b/drivers/gpu/drm/i915/intel_fbc.c > > >> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv) > > >> return dev_priv->fbc.enabled; > > >> } > > >> > > >> +static void intel_fbc_enable(struct intel_crtc *crtc, > > >> + struct drm_framebuffer *fb) > > > > > > fb could be const > > > > I guess a lot of things could be constified, if we decide to do this. > > Personally I like const on .data (especially vfunc tables since those are > nice to create exploits if they're writable and you can get at them). And > for core functions/vfuncs where the const has a semantic meaning. > Otherwise I personally don't see to much value in sprinkling const all > over. I especially like making display mode, state, etc. structs const to make it clear which functions can change them and which can't. IMO drm_framebuffer could use the same treatment.
On Tue, Sep 01, 2015 at 02:03:34PM +0300, Ville Syrjälä wrote: > On Tue, Sep 01, 2015 at 12:07:01PM +0200, Daniel Vetter wrote: > > On Fri, Aug 28, 2015 at 11:50:08AM -0300, Paulo Zanoni wrote: > > > 2015-08-28 11:20 GMT-03:00 Ville Syrjälä <ville.syrjala@linux.intel.com>: > > > > On Fri, Aug 14, 2015 at 06:34:07PM -0300, Paulo Zanoni wrote: > > > >> Always update the currrent crtc, fb and vertical offset after calling > > > >> enable_fbc. We were forgetting to do so along the failure paths when > > > >> enabling fbc synchronously. Fix this with a new helper to enable_fbc() > > > >> and update the state simultaneously. > > > >> > > > >> v2: Improve commit message (Chris). > > > >> > > > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > >> --- > > > >> drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++---------- > > > >> 1 file changed, 17 insertions(+), 10 deletions(-) > > > >> > > > >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c > > > >> index c97aba2..fa9b004 100644 > > > >> --- a/drivers/gpu/drm/i915/intel_fbc.c > > > >> +++ b/drivers/gpu/drm/i915/intel_fbc.c > > > >> @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv) > > > >> return dev_priv->fbc.enabled; > > > >> } > > > >> > > > >> +static void intel_fbc_enable(struct intel_crtc *crtc, > > > >> + struct drm_framebuffer *fb) > > > > > > > > fb could be const > > > > > > I guess a lot of things could be constified, if we decide to do this. > > > > Personally I like const on .data (especially vfunc tables since those are > > nice to create exploits if they're writable and you can get at them). And > > for core functions/vfuncs where the const has a semantic meaning. > > Otherwise I personally don't see to much value in sprinkling const all > > over. > > I especially like making display mode, state, etc. structs const to make > it clear which functions can change them and which can't. IMO > drm_framebuffer could use the same treatment. Yeah I guess making a few things in the drm core static could be useful indeed, for example plane_state->fb reall should be static, or crtc_state->mode_blob (or any other fb or blob pointer in state structures). Same for all the {plane, connector, crtc}->state pointers (although that would need some casts in swap_states()). I'm not too sure how much benefit there is if we do this just in i915, since if the top-level entry points called from drm core aren't enforcing constness trying to do it in i915 only will probably be an endless effort of fixing things up all the time. -Daniel
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c index c97aba2..fa9b004 100644 --- a/drivers/gpu/drm/i915/intel_fbc.c +++ b/drivers/gpu/drm/i915/intel_fbc.c @@ -308,6 +308,18 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv) return dev_priv->fbc.enabled; } +static void intel_fbc_enable(struct intel_crtc *crtc, + struct drm_framebuffer *fb) +{ + struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; + + dev_priv->fbc.enable_fbc(crtc); + + dev_priv->fbc.crtc = crtc; + dev_priv->fbc.fb_id = fb->base.id; + dev_priv->fbc.y = crtc->base.y; +} + static void intel_fbc_work_fn(struct work_struct *__work) { struct intel_fbc_work *work = @@ -321,13 +333,8 @@ static void intel_fbc_work_fn(struct work_struct *__work) /* Double check that we haven't switched fb without cancelling * the prior work. */ - if (crtc_fb == work->fb) { - dev_priv->fbc.enable_fbc(work->crtc); - - dev_priv->fbc.crtc = work->crtc; - dev_priv->fbc.fb_id = crtc_fb->base.id; - dev_priv->fbc.y = work->crtc->base.y; - } + if (crtc_fb == work->fb) + intel_fbc_enable(work->crtc, work->fb); dev_priv->fbc.fbc_work = NULL; } @@ -361,7 +368,7 @@ static void intel_fbc_cancel_work(struct drm_i915_private *dev_priv) dev_priv->fbc.fbc_work = NULL; } -static void intel_fbc_enable(struct intel_crtc *crtc) +static void intel_fbc_schedule_enable(struct intel_crtc *crtc) { struct intel_fbc_work *work; struct drm_i915_private *dev_priv = crtc->base.dev->dev_private; @@ -373,7 +380,7 @@ static void intel_fbc_enable(struct intel_crtc *crtc) work = kzalloc(sizeof(*work), GFP_KERNEL); if (work == NULL) { DRM_ERROR("Failed to allocate FBC work structure\n"); - dev_priv->fbc.enable_fbc(crtc); + intel_fbc_enable(crtc, crtc->base.primary->fb); return; } @@ -826,7 +833,7 @@ static void __intel_fbc_update(struct drm_i915_private *dev_priv) __intel_fbc_disable(dev_priv); } - intel_fbc_enable(intel_crtc); + intel_fbc_schedule_enable(intel_crtc); dev_priv->fbc.no_fbc_reason = FBC_OK; return;
Always update the currrent crtc, fb and vertical offset after calling enable_fbc. We were forgetting to do so along the failure paths when enabling fbc synchronously. Fix this with a new helper to enable_fbc() and update the state simultaneously. v2: Improve commit message (Chris). Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_fbc.c | 27 +++++++++++++++++---------- 1 file changed, 17 insertions(+), 10 deletions(-)