diff mbox

[02/12] drm/i915: set dev_priv->fbc.crtc before scheduling the enable work

Message ID 1447444424-17168-3-git-send-email-paulo.r.zanoni@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Zanoni, Paulo R Nov. 13, 2015, 7:53 p.m. UTC
This thing where we need to get the crtc either from the work
structure or the fbc structure itself is confusing and unnecessary.
Set fbc.crtc right when scheduling the enable work so we can always
use it.

The problem is not what gets passed and how to retrieve it. The
problem is that when we're in the other parts of the code we always
have to keep in mind that if FBC is already enabled we have to get the
CRTC from place A, if FBC is scheduled we have to get the CRTC from
place B, and if it's disabled there's no CRTC. Having a single place
to retrieve the CRTC from allows us to treat the "is enabled" and "is
scheduled" cases as the same case, reducing the mistake surface. I
guess I should add this to the commit message.

Besides the immediate advantages, this is also going to make one of
the next commits much simpler. And even later, when we introduce
enable/disable + activate/deactivate, this will be even simpler as
we'll set the CRTC at enable time. So all the
activate/deactivate/update code can just look at the single CRTC
variable regardless of the current state.

v2: Improve commit message (Chris).
v3: Rebase after changing the patch order.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |  1 -
 drivers/gpu/drm/i915/intel_fbc.c | 22 +++++++++-------------
 2 files changed, 9 insertions(+), 14 deletions(-)

Comments

Chris Wilson Nov. 13, 2015, 8:56 p.m. UTC | #1
On Fri, Nov 13, 2015 at 05:53:34PM -0200, Paulo Zanoni wrote:
> This thing where we need to get the crtc either from the work
> structure or the fbc structure itself is confusing and unnecessary.
> Set fbc.crtc right when scheduling the enable work so we can always
> use it.
> 
> The problem is not what gets passed and how to retrieve it. The
> problem is that when we're in the other parts of the code we always
> have to keep in mind that if FBC is already enabled we have to get the
> CRTC from place A, if FBC is scheduled we have to get the CRTC from
> place B, and if it's disabled there's no CRTC. Having a single place
> to retrieve the CRTC from allows us to treat the "is enabled" and "is
> scheduled" cases as the same case, reducing the mistake surface. I
> guess I should add this to the commit message.
> 
> Besides the immediate advantages, this is also going to make one of
> the next commits much simpler. And even later, when we introduce
> enable/disable + activate/deactivate, this will be even simpler as
> we'll set the CRTC at enable time. So all the
> activate/deactivate/update code can just look at the single CRTC
> variable regardless of the current state.
> 
> v2: Improve commit message (Chris).
> v3: Rebase after changing the patch order.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>  drivers/gpu/drm/i915/intel_fbc.c | 22 +++++++++-------------
>  2 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 71bd1dc..317414b 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -920,7 +920,6 @@ struct i915_fbc {
>  
>  	struct intel_fbc_work {
>  		struct delayed_work work;
> -		struct intel_crtc *crtc;
>  		struct drm_framebuffer *fb;
>  	} *fbc_work;
>  
> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> index 611672f..bc9cd33 100644
> --- a/drivers/gpu/drm/i915/intel_fbc.c
> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> @@ -334,14 +334,13 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
>  	return dev_priv->fbc.enabled;
>  }
>  
> -static void intel_fbc_enable(struct intel_crtc *crtc,
> -			     const struct drm_framebuffer *fb)
> +static void intel_fbc_enable(const struct drm_framebuffer *fb)
>  {
> -	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> +	struct drm_i915_private *dev_priv = fb->dev->dev_private;
> +	struct intel_crtc *crtc = dev_priv->fbc.crtc;

This is confusing me. I think of FBC in terms of the CRTC/pipe, and the
fb just describes the plane currently bound to the primary. You've
pushed everywhere else to also work on the CRTC, I think it would be
consistent here to pass crtc and then use fbc.fb_id =
crtc->primary->fb->id.

Have I missed something in the later patches that explains the choice
here?
-Chris
Paulo Zanoni Nov. 13, 2015, 9:13 p.m. UTC | #2
2015-11-13 18:56 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> On Fri, Nov 13, 2015 at 05:53:34PM -0200, Paulo Zanoni wrote:
>> This thing where we need to get the crtc either from the work
>> structure or the fbc structure itself is confusing and unnecessary.
>> Set fbc.crtc right when scheduling the enable work so we can always
>> use it.
>>
>> The problem is not what gets passed and how to retrieve it. The
>> problem is that when we're in the other parts of the code we always
>> have to keep in mind that if FBC is already enabled we have to get the
>> CRTC from place A, if FBC is scheduled we have to get the CRTC from
>> place B, and if it's disabled there's no CRTC. Having a single place
>> to retrieve the CRTC from allows us to treat the "is enabled" and "is
>> scheduled" cases as the same case, reducing the mistake surface. I
>> guess I should add this to the commit message.
>>
>> Besides the immediate advantages, this is also going to make one of
>> the next commits much simpler. And even later, when we introduce
>> enable/disable + activate/deactivate, this will be even simpler as
>> we'll set the CRTC at enable time. So all the
>> activate/deactivate/update code can just look at the single CRTC
>> variable regardless of the current state.
>>
>> v2: Improve commit message (Chris).
>> v3: Rebase after changing the patch order.
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
>>  drivers/gpu/drm/i915/intel_fbc.c | 22 +++++++++-------------
>>  2 files changed, 9 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 71bd1dc..317414b 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -920,7 +920,6 @@ struct i915_fbc {
>>
>>       struct intel_fbc_work {
>>               struct delayed_work work;
>> -             struct intel_crtc *crtc;
>>               struct drm_framebuffer *fb;
>>       } *fbc_work;
>>
>> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
>> index 611672f..bc9cd33 100644
>> --- a/drivers/gpu/drm/i915/intel_fbc.c
>> +++ b/drivers/gpu/drm/i915/intel_fbc.c
>> @@ -334,14 +334,13 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
>>       return dev_priv->fbc.enabled;
>>  }
>>
>> -static void intel_fbc_enable(struct intel_crtc *crtc,
>> -                          const struct drm_framebuffer *fb)
>> +static void intel_fbc_enable(const struct drm_framebuffer *fb)
>>  {
>> -     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
>> +     struct drm_i915_private *dev_priv = fb->dev->dev_private;
>> +     struct intel_crtc *crtc = dev_priv->fbc.crtc;
>
> This is confusing me. I think of FBC in terms of the CRTC/pipe, and the
> fb just describes the plane currently bound to the primary. You've
> pushed everywhere else to also work on the CRTC, I think it would be
> consistent here to pass crtc and then use fbc.fb_id =
> crtc->primary->fb->id.
>
> Have I missed something in the later patches that explains the choice
> here?

You are right that this is confusing. This is basically an artifact
from the previous FBC design that I overlooked. I guess during the
conversion, my thinking was "well, the arguments are the stuff we
store in the work struct, and since we don't store the CRTC anymore,
let's remove it and keep passing the FB since it's still part of the
work struct". Now that you bring this under a different perspective, I
see the problem. We don't even need to pass these things as arguments
since we do check that work->fb is equal to crtc->fb.

I just hope you let me address this with follow-up patches. Function
intel_fbc_activate() was created because there were 2 callers: one for
the case there the work_fn happens, another for the case where we
failed to allocate the work_fn. Later in the series there's only one
caller because there's no allocation anymore, so the best way to fix
is probably to just move those 3 lines to the place of the only
caller.

You also requested me to grab references of fbc.fb_id, and I have some
evil plans to completely kill it, which would even reduce those 3
lines further.

Thanks,
Paulo

> -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
Chris Wilson Nov. 13, 2015, 9:43 p.m. UTC | #3
On Fri, Nov 13, 2015 at 07:13:40PM -0200, Paulo Zanoni wrote:
> 2015-11-13 18:56 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
> > On Fri, Nov 13, 2015 at 05:53:34PM -0200, Paulo Zanoni wrote:
> >> This thing where we need to get the crtc either from the work
> >> structure or the fbc structure itself is confusing and unnecessary.
> >> Set fbc.crtc right when scheduling the enable work so we can always
> >> use it.
> >>
> >> The problem is not what gets passed and how to retrieve it. The
> >> problem is that when we're in the other parts of the code we always
> >> have to keep in mind that if FBC is already enabled we have to get the
> >> CRTC from place A, if FBC is scheduled we have to get the CRTC from
> >> place B, and if it's disabled there's no CRTC. Having a single place
> >> to retrieve the CRTC from allows us to treat the "is enabled" and "is
> >> scheduled" cases as the same case, reducing the mistake surface. I
> >> guess I should add this to the commit message.
> >>
> >> Besides the immediate advantages, this is also going to make one of
> >> the next commits much simpler. And even later, when we introduce
> >> enable/disable + activate/deactivate, this will be even simpler as
> >> we'll set the CRTC at enable time. So all the
> >> activate/deactivate/update code can just look at the single CRTC
> >> variable regardless of the current state.
> >>
> >> v2: Improve commit message (Chris).
> >> v3: Rebase after changing the patch order.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> >>  drivers/gpu/drm/i915/intel_fbc.c | 22 +++++++++-------------
> >>  2 files changed, 9 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> >> index 71bd1dc..317414b 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -920,7 +920,6 @@ struct i915_fbc {
> >>
> >>       struct intel_fbc_work {
> >>               struct delayed_work work;
> >> -             struct intel_crtc *crtc;
> >>               struct drm_framebuffer *fb;
> >>       } *fbc_work;
> >>
> >> diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
> >> index 611672f..bc9cd33 100644
> >> --- a/drivers/gpu/drm/i915/intel_fbc.c
> >> +++ b/drivers/gpu/drm/i915/intel_fbc.c
> >> @@ -334,14 +334,13 @@ bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
> >>       return dev_priv->fbc.enabled;
> >>  }
> >>
> >> -static void intel_fbc_enable(struct intel_crtc *crtc,
> >> -                          const struct drm_framebuffer *fb)
> >> +static void intel_fbc_enable(const struct drm_framebuffer *fb)
> >>  {
> >> -     struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
> >> +     struct drm_i915_private *dev_priv = fb->dev->dev_private;
> >> +     struct intel_crtc *crtc = dev_priv->fbc.crtc;
> >
> > This is confusing me. I think of FBC in terms of the CRTC/pipe, and the
> > fb just describes the plane currently bound to the primary. You've
> > pushed everywhere else to also work on the CRTC, I think it would be
> > consistent here to pass crtc and then use fbc.fb_id =
> > crtc->primary->fb->id.
> >
> > Have I missed something in the later patches that explains the choice
> > here?
> 
> You are right that this is confusing. This is basically an artifact
> from the previous FBC design that I overlooked. I guess during the
> conversion, my thinking was "well, the arguments are the stuff we
> store in the work struct, and since we don't store the CRTC anymore,
> let's remove it and keep passing the FB since it's still part of the
> work struct". Now that you bring this under a different perspective, I
> see the problem. We don't even need to pass these things as arguments
> since we do check that work->fb is equal to crtc->fb.
> 
> I just hope you let me address this with follow-up patches. Function
> intel_fbc_activate() was created because there were 2 callers: one for
> the case there the work_fn happens, another for the case where we
> failed to allocate the work_fn. Later in the series there's only one
> caller because there's no allocation anymore, so the best way to fix
> is probably to just move those 3 lines to the place of the only
> caller.
> 
> You also requested me to grab references of fbc.fb_id, and I have some
> evil plans to completely kill it, which would even reduce those 3
> lines further.

If you have it in hand, the rest seems fine, so
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
Daniel Stone Nov. 20, 2015, 5:46 p.m. UTC | #4
Hi,

On 13 November 2015 at 21:13, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2015-11-13 18:56 GMT-02:00 Chris Wilson <chris@chris-wilson.co.uk>:
>> This is confusing me. I think of FBC in terms of the CRTC/pipe, and the
>> fb just describes the plane currently bound to the primary. You've
>> pushed everywhere else to also work on the CRTC, I think it would be
>> consistent here to pass crtc and then use fbc.fb_id =
>> crtc->primary->fb->id.
>>
>> Have I missed something in the later patches that explains the choice
>> here?
>
> You are right that this is confusing. This is basically an artifact
> from the previous FBC design that I overlooked. I guess during the
> conversion, my thinking was "well, the arguments are the stuff we
> store in the work struct, and since we don't store the CRTC anymore,
> let's remove it and keep passing the FB since it's still part of the
> work struct". Now that you bring this under a different perspective, I
> see the problem. We don't even need to pass these things as arguments
> since we do check that work->fb is equal to crtc->fb.

With async modesetting on its way, this won't actually hold true for
long. Tracking the fb separately, rather than pulling it back out of
the CRTC, is the right thing to do here.

Basically, if you ever want to check crtc->primary->fb, and you're not
in atomic_check, you're doing it wrong.

Cheers,
Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 71bd1dc..317414b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -920,7 +920,6 @@  struct i915_fbc {
 
 	struct intel_fbc_work {
 		struct delayed_work work;
-		struct intel_crtc *crtc;
 		struct drm_framebuffer *fb;
 	} *fbc_work;
 
diff --git a/drivers/gpu/drm/i915/intel_fbc.c b/drivers/gpu/drm/i915/intel_fbc.c
index 611672f..bc9cd33 100644
--- a/drivers/gpu/drm/i915/intel_fbc.c
+++ b/drivers/gpu/drm/i915/intel_fbc.c
@@ -334,14 +334,13 @@  bool intel_fbc_enabled(struct drm_i915_private *dev_priv)
 	return dev_priv->fbc.enabled;
 }
 
-static void intel_fbc_enable(struct intel_crtc *crtc,
-			     const struct drm_framebuffer *fb)
+static void intel_fbc_enable(const struct drm_framebuffer *fb)
 {
-	struct drm_i915_private *dev_priv = crtc->base.dev->dev_private;
+	struct drm_i915_private *dev_priv = fb->dev->dev_private;
+	struct intel_crtc *crtc = dev_priv->fbc.crtc;
 
 	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;
 }
@@ -351,8 +350,8 @@  static void intel_fbc_work_fn(struct work_struct *__work)
 	struct intel_fbc_work *work =
 		container_of(to_delayed_work(__work),
 			     struct intel_fbc_work, work);
-	struct drm_i915_private *dev_priv = work->crtc->base.dev->dev_private;
-	struct drm_framebuffer *crtc_fb = work->crtc->base.primary->fb;
+	struct drm_i915_private *dev_priv = work->fb->dev->dev_private;
+	struct drm_framebuffer *crtc_fb = dev_priv->fbc.crtc->base.primary->fb;
 
 	mutex_lock(&dev_priv->fbc.lock);
 	if (work == dev_priv->fbc.fbc_work) {
@@ -360,7 +359,7 @@  static void intel_fbc_work_fn(struct work_struct *__work)
 		 * the prior work.
 		 */
 		if (crtc_fb == work->fb)
-			intel_fbc_enable(work->crtc, work->fb);
+			intel_fbc_enable(work->fb);
 
 		dev_priv->fbc.fbc_work = NULL;
 	}
@@ -400,15 +399,15 @@  static void intel_fbc_schedule_enable(struct intel_crtc *crtc)
 	WARN_ON(!mutex_is_locked(&dev_priv->fbc.lock));
 
 	intel_fbc_cancel_work(dev_priv);
+	dev_priv->fbc.crtc = crtc;
 
 	work = kzalloc(sizeof(*work), GFP_KERNEL);
 	if (work == NULL) {
 		DRM_ERROR("Failed to allocate FBC work structure\n");
-		intel_fbc_enable(crtc, crtc->base.primary->fb);
+		intel_fbc_enable(crtc->base.primary->fb);
 		return;
 	}
 
-	work->crtc = crtc;
 	work->fb = crtc->base.primary->fb;
 	INIT_DELAYED_WORK(&work->work, intel_fbc_work_fn);
 
@@ -984,11 +983,8 @@  void intel_fbc_invalidate(struct drm_i915_private *dev_priv,
 
 	mutex_lock(&dev_priv->fbc.lock);
 
-	if (dev_priv->fbc.enabled)
+	if (dev_priv->fbc.enabled || dev_priv->fbc.fbc_work)
 		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(dev_priv->fbc.crtc->pipe);
-	else if (dev_priv->fbc.fbc_work)
-		fbc_bits = INTEL_FRONTBUFFER_PRIMARY(
-					dev_priv->fbc.fbc_work->crtc->pipe);
 	else
 		fbc_bits = dev_priv->fbc.possible_framebuffer_bits;