[Intel-gfx] [PATCH 3/9] drm/i915: PSR HSW/BDW: Fix inverted logic at sink main_link_active bit.

[Rodrigo Vivi]

On Tue, Jan 13, 2015 at 2:26 PM, Daniel Vetter <daniel at ffwll.ch> wrote:
> On Tue, Jan 13, 2015 at 02:24:54PM +0000, R, Durgadoss wrote:
>> Hi Rodrigo,
>>
>> >-----Original Message-----
>> >From: Intel-gfx [mailto:intel-gfx-bounces at lists.freedesktop.org] On Behalf Of Rodrigo Vivi
>> >Sent: Monday, January 12, 2015 11:45 PM
>> >To: intel-gfx at lists.freedesktop.org
>> >Cc: Vivi, Rodrigo
>> >Subject: [Intel-gfx] [PATCH 3/9] drm/i915: PSR HSW/BDW: Fix inverted logic at sink main_link_active bit.
>> >
>> >We have only two possible states with so many names and combinations that
>> >might be confusing.
>> >
>> >1 - Main link active / enabled / stand by / on
>> >2 - Main link disabled / off / full off
>>
>> In this case, I think we should use 'link_active' instead of 'link_standby'
>> since 'active' is what is used by the eDP spec and most of our PSR
>> macros. But, I believe we can have this as a separate patch.
>
> I haven't read the patches in detail, but consistent naming is imo very
> important. Also since this seems to be confusing some extension to the psr
> kerneldoc overview comment is imo asked for here.

I also agree that consistent naming is good. This name caused the
confusion of inverted logic somewhere here.
But I believe active name can be counfused with active/exit state we
have on source side.

Also standby is the name used on the source. Actually we have many
names for same think

* full_link_on at VBT
* link_active on DP
* transmitter state on VLV/CHV
* link_standby on core implementation

I prefer the link_standby.

>
> Rodrigo, can you please update the patches or do a follow up? Either is
> fine with me.

I prefer a follow-up so we can continue discussion to see if I change
my mind on the better name ;)

> -Daniel

Thank you both,
Rodrigo.

>
>>
>> For patches 1-3,
>> Reviewed-by: Durgadoss R <durgadoss.r at intel.com>
>>
>> Thanks,
>> Durga
>>
>> >
>> >Let's start organizing it by fixing a inverted logic when setting the sink bit.
>> >
>> >Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> >---
>> > drivers/gpu/drm/i915/intel_psr.c | 4 ++--
>> > 1 file changed, 2 insertions(+), 2 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/intel_psr.c b/drivers/gpu/drm/i915/intel_psr.c
>> >index 3dd8886..0af89db 100644
>> >--- a/drivers/gpu/drm/i915/intel_psr.c
>> >+++ b/drivers/gpu/drm/i915/intel_psr.c
>> >@@ -163,10 +163,10 @@ static void hsw_psr_enable_sink(struct intel_dp *intel_dp)
>> > /* Enable PSR in sink */
>> > if (intel_dp->psr_dpcd[1] & DP_PSR_NO_TRAIN_ON_EXIT || only_standby)
>> > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
>> >-   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
>> >+   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
>> > else
>> > drm_dp_dpcd_writeb(&intel_dp->aux, DP_PSR_EN_CFG,
>> >-   DP_PSR_ENABLE | DP_PSR_MAIN_LINK_ACTIVE);
>> >+   DP_PSR_ENABLE & ~DP_PSR_MAIN_LINK_ACTIVE);
>> >
>> > /* Setup AUX registers */
>> > for (i = 0; i < sizeof(aux_msg); i += 4)
>> >--
>> >2.1.0
>> >
>> >_______________________________________________
>> >Intel-gfx mailing list
>> >Intel-gfx at lists.freedesktop.org
>> >http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Rodrigo Vivi
Blog: http://blog.vivi.eng.br