diff mbox

[1/2] drm/i915: move psr_setup_done to psr struct

Message ID 1387310265-4160-1-git-send-email-rodrigo.vivi@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Rodrigo Vivi Dec. 17, 2013, 7:57 p.m. UTC
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
---
 drivers/gpu/drm/i915/i915_drv.h  | 1 +
 drivers/gpu/drm/i915/intel_dp.c  | 6 +++---
 drivers/gpu/drm/i915/intel_drv.h | 1 -
 3 files changed, 4 insertions(+), 4 deletions(-)

Comments

Paulo Zanoni Dec. 17, 2013, 8:06 p.m. UTC | #1
2013/12/17 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h  | 1 +
>  drivers/gpu/drm/i915/intel_dp.c  | 6 +++---
>  drivers/gpu/drm/i915/intel_drv.h | 1 -
>  3 files changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 53288f6..cae3225 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -715,6 +715,7 @@ struct i915_fbc {
>  struct i915_psr {
>         bool sink_support;
>         bool source_ok;
> +       bool setup_done;
>  };
>
>  enum intel_pch {
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 9b40113..f062a59 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1567,7 +1567,7 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct edp_vsc_psr psr_vsc;
>
> -       if (intel_dp->psr_setup_done)
> +       if (dev_priv->psr.setup_done)
>                 return;
>
>         /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
> @@ -1582,7 +1582,7 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
>         I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
>                    EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
>
> -       intel_dp->psr_setup_done = true;
> +       dev_priv->psr.setup_done = true;
>  }
>
>  static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
> @@ -3702,7 +3702,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>         WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
>              error, port_name(port));
>
> -       intel_dp->psr_setup_done = false;
> +       dev_priv->psr.setup_done = false;

This line is now weird because most systems will call
intel_dp_init_connector more than once, so we'll zero setup_done many
times. I don't see any bugs related to this, but we should probably
move this line to somewhere else. Or just remove this line and rely on
the fact that the struct is initialized as zero when allocated. It
seems sink_support and source_ok are not explicitly initialized, so we
should probably follow this model.


>
>         if (!intel_edp_init_connector(intel_dp, intel_connector)) {
>                 i2c_del_adapter(&intel_dp->adapter);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 46aea6c..f7b5b2f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -485,7 +485,6 @@ struct intel_dp {
>         int backlight_off_delay;
>         struct delayed_work panel_vdd_work;
>         bool want_panel_vdd;
> -       bool psr_setup_done;
>         struct intel_connector *attached_connector;
>  };
>
> --
> 1.8.3.1
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Rodrigo Vivi Dec. 17, 2013, 8:23 p.m. UTC | #2
On Tue, Dec 17, 2013 at 6:06 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2013/12/17 Rodrigo Vivi <rodrigo.vivi@gmail.com>:
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi@gmail.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.h  | 1 +
>>  drivers/gpu/drm/i915/intel_dp.c  | 6 +++---
>>  drivers/gpu/drm/i915/intel_drv.h | 1 -
>>  3 files changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
>> index 53288f6..cae3225 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -715,6 +715,7 @@ struct i915_fbc {
>>  struct i915_psr {
>>         bool sink_support;
>>         bool source_ok;
>> +       bool setup_done;
>>  };
>>
>>  enum intel_pch {
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 9b40113..f062a59 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1567,7 +1567,7 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
>>         struct drm_i915_private *dev_priv = dev->dev_private;
>>         struct edp_vsc_psr psr_vsc;
>>
>> -       if (intel_dp->psr_setup_done)
>> +       if (dev_priv->psr.setup_done)
>>                 return;
>>
>>         /* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
>> @@ -1582,7 +1582,7 @@ static void intel_edp_psr_setup(struct intel_dp *intel_dp)
>>         I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
>>                    EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
>>
>> -       intel_dp->psr_setup_done = true;
>> +       dev_priv->psr.setup_done = true;
>>  }
>>
>>  static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
>> @@ -3702,7 +3702,7 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>>         WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
>>              error, port_name(port));
>>
>> -       intel_dp->psr_setup_done = false;
>> +       dev_priv->psr.setup_done = false;
>
> This line is now weird because most systems will call
> intel_dp_init_connector more than once, so we'll zero setup_done many
> times. I don't see any bugs related to this, but we should probably
> move this line to somewhere else.

yeah, it isn't a problem... some of my tests here I was making it
false on all disable call.
But it is better if it was only once for sure...
and I was using on dp_init what I guess doesn't help either, right?
so, do you suggest any place?

> Or just remove this line and rely on
> the fact that the struct is initialized as zero when allocated. It
> seems sink_support and source_ok are not explicitly initialized, so we
> should probably follow this model.

Can we trust that? If this is a sure thing I think this is might be
the best approach.

Thanks!
>
>
>>
>>         if (!intel_edp_init_connector(intel_dp, intel_connector)) {
>>                 i2c_del_adapter(&intel_dp->adapter);
>> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
>> index 46aea6c..f7b5b2f 100644
>> --- a/drivers/gpu/drm/i915/intel_drv.h
>> +++ b/drivers/gpu/drm/i915/intel_drv.h
>> @@ -485,7 +485,6 @@ struct intel_dp {
>>         int backlight_off_delay;
>>         struct delayed_work panel_vdd_work;
>>         bool want_panel_vdd;
>> -       bool psr_setup_done;
>>         struct intel_connector *attached_connector;
>>  };
>>
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
> --
> Paulo Zanoni
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 53288f6..cae3225 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -715,6 +715,7 @@  struct i915_fbc {
 struct i915_psr {
 	bool sink_support;
 	bool source_ok;
+	bool setup_done;
 };
 
 enum intel_pch {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 9b40113..f062a59 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1567,7 +1567,7 @@  static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct edp_vsc_psr psr_vsc;
 
-	if (intel_dp->psr_setup_done)
+	if (dev_priv->psr.setup_done)
 		return;
 
 	/* Prepare VSC packet as per EDP 1.3 spec, Table 3.10 */
@@ -1582,7 +1582,7 @@  static void intel_edp_psr_setup(struct intel_dp *intel_dp)
 	I915_WRITE(EDP_PSR_DEBUG_CTL(dev), EDP_PSR_DEBUG_MASK_MEMUP |
 		   EDP_PSR_DEBUG_MASK_HPD | EDP_PSR_DEBUG_MASK_LPSP);
 
-	intel_dp->psr_setup_done = true;
+	dev_priv->psr.setup_done = true;
 }
 
 static void intel_edp_psr_enable_sink(struct intel_dp *intel_dp)
@@ -3702,7 +3702,7 @@  intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	WARN(error, "intel_dp_i2c_init failed with error %d for port %c\n",
 	     error, port_name(port));
 
-	intel_dp->psr_setup_done = false;
+	dev_priv->psr.setup_done = false;
 
 	if (!intel_edp_init_connector(intel_dp, intel_connector)) {
 		i2c_del_adapter(&intel_dp->adapter);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 46aea6c..f7b5b2f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -485,7 +485,6 @@  struct intel_dp {
 	int backlight_off_delay;
 	struct delayed_work panel_vdd_work;
 	bool want_panel_vdd;
-	bool psr_setup_done;
 	struct intel_connector *attached_connector;
 };