| Message ID | 20230901093500.3463046-2-jouni.hogander@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Handle dma fences in dirtyfb ioctl | expand |
Hi Jouni, On Fri, 2023-09-01 at 12:34 +0300, Jouni Högander wrote: > We are planning to move flush performed from work queue. This > means it is possible to have invalidate -> flip -> flush sequence. > Handle this by clearing possible busy bits on flip. > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > --- > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c > index 1c6d467cec26..817e5784660b 100644 > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > @@ -1307,11 +1307,9 @@ static void __intel_fbc_post_update(struct intel_fbc *fbc) > lockdep_assert_held(&fbc->lock); > > fbc->flip_pending = false; > + fbc->busy_bits = 0; > > - if (!fbc->busy_bits) > - intel_fbc_activate(fbc); > - else > - intel_fbc_deactivate(fbc, "frontbuffer write"); > + intel_fbc_activate(fbc); Can you explain why the call to intel_fbc_deactivate() is not needed here anymore? I think it would be a good idea to explain that in the commit message. Or, at least, an explanation about it here, so it's documented. ;) -- Cheers, Luca.
On Mon, 2023-09-04 at 07:25 +0000, Coelho, Luciano wrote: > Hi Jouni, > > On Fri, 2023-09-01 at 12:34 +0300, Jouni Högander wrote: > > We are planning to move flush performed from work queue. This > > means it is possible to have invalidate -> flip -> flush sequence. > > Handle this by clearing possible busy bits on flip. > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++---- > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > index 1c6d467cec26..817e5784660b 100644 > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > @@ -1307,11 +1307,9 @@ static void __intel_fbc_post_update(struct > > intel_fbc *fbc) > > lockdep_assert_held(&fbc->lock); > > > > fbc->flip_pending = false; > > + fbc->busy_bits = 0; > > > > - if (!fbc->busy_bits) > > - intel_fbc_activate(fbc); > > - else > > - intel_fbc_deactivate(fbc, "frontbuffer write"); > > + intel_fbc_activate(fbc); > > Can you explain why the call to intel_fbc_deactivate() is not needed > here anymore? I think it would be a good idea to explain that in the > commit message. Or, at least, an explanation about it here, so it's > documented. ;) We are clearing fbc->busy_bits -> I.e. if(!fbc->busy_bits) is always taken : Post plane update is called at the end of the flip. If you consider case where busy_bits != 0 at this point: it means someone have initiated frontbuffer write (invalidate) which is not yet completed (flush from workqueue). That flush pending in workqueue is not valid anymore as there was a flip and the buffer which was frontbuffer is not a frontbuffer anymore. Even if the same buffer would be used when doing a flip the atomic commit would take care of flushing the buffer towards fbc. Also waiting for dma fences is take caren by the atomic commit code. BR, Jouni Högander > > -- > Cheers, > Luca.
On Mon, 2023-09-04 at 08:40 +0000, Hogander, Jouni wrote: > On Mon, 2023-09-04 at 07:25 +0000, Coelho, Luciano wrote: > > Hi Jouni, > > > > On Fri, 2023-09-01 at 12:34 +0300, Jouni Högander wrote: > > > We are planning to move flush performed from work queue. This > > > means it is possible to have invalidate -> flip -> flush sequence. > > > Handle this by clearing possible busy bits on flip. > > > > > > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > > > Signed-off-by: Jouni Högander <jouni.hogander@intel.com> > > > --- > > > drivers/gpu/drm/i915/display/intel_fbc.c | 6 ++---- > > > 1 file changed, 2 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c > > > b/drivers/gpu/drm/i915/display/intel_fbc.c > > > index 1c6d467cec26..817e5784660b 100644 > > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c > > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c > > > @@ -1307,11 +1307,9 @@ static void __intel_fbc_post_update(struct > > > intel_fbc *fbc) > > > lockdep_assert_held(&fbc->lock); > > > > > > fbc->flip_pending = false; > > > + fbc->busy_bits = 0; > > > > > > - if (!fbc->busy_bits) > > > - intel_fbc_activate(fbc); > > > - else > > > - intel_fbc_deactivate(fbc, "frontbuffer write"); > > > + intel_fbc_activate(fbc); > > > > Can you explain why the call to intel_fbc_deactivate() is not needed > > here anymore? I think it would be a good idea to explain that in the > > commit message. Or, at least, an explanation about it here, so it's > > documented. ;) > > We are clearing fbc->busy_bits -> I.e. if(!fbc->busy_bits) is always > taken : > > Post plane update is called at the end of the flip. If you consider > case where busy_bits != 0 at this point: it means someone have > initiated frontbuffer write (invalidate) which is not yet completed > (flush from workqueue). That flush pending in workqueue is not valid > anymore as there was a flip and the buffer which was frontbuffer is not > a frontbuffer anymore. Even if the same buffer would be used when doing > a flip the atomic commit would take care of flushing the buffer towards > fbc. Also waiting for dma fences is take caren by the atomic commit > code. Thanks for the explanation! It makes sense. So you have my: Reviewed-by: Luca Coelho <luciano.coelho@intel.com> -- Cheers, Luca.
diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c index 1c6d467cec26..817e5784660b 100644 --- a/drivers/gpu/drm/i915/display/intel_fbc.c +++ b/drivers/gpu/drm/i915/display/intel_fbc.c @@ -1307,11 +1307,9 @@ static void __intel_fbc_post_update(struct intel_fbc *fbc) lockdep_assert_held(&fbc->lock); fbc->flip_pending = false; + fbc->busy_bits = 0; - if (!fbc->busy_bits) - intel_fbc_activate(fbc); - else - intel_fbc_deactivate(fbc, "frontbuffer write"); + intel_fbc_activate(fbc); } void intel_fbc_post_update(struct intel_atomic_state *state,