[PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Minu Mathai]

From: Minu <minu.mathai@intel.com>

Display CRCs were not readable because the register defintions
for PORT_DFT_I9XX and PORT_DFT2_G4X were wrong.
MMIO offset needs to be added to these register offsets to fix them.

Issue: GMINL-6869
Signed-off-by: Minu Mathai <minu.mathai@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 7213224..c327c7c 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3193,7 +3193,7 @@ enum skl_disp_power_wells {
 #define PCH_HDMIC	0xe1150
 #define PCH_HDMID	0xe1160
 
-#define PORT_DFT_I9XX				0x61150
+#define PORT_DFT_I9XX				(dev_priv->info.display_mmio_offset + 0x61150)
 #define   DC_BALANCE_RESET			(1 << 25)
 #define PORT_DFT2_G4X		(dev_priv->info.display_mmio_offset + 0x61154)
 #define   DC_BALANCE_RESET_VLV			(1 << 31)
-- 
1.9.1

---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Ville Syrjälä]

On Fri, Jun 05, 2015 at 02:00:24PM +0100, Minu Mathai wrote:
> From: Minu <minu.mathai@intel.com>
> 
> Display CRCs were not readable because the register defintions
> for PORT_DFT_I9XX and PORT_DFT2_G4X were wrong.
> MMIO offset needs to be added to these register offsets to fix them.
> 
> Issue: GMINL-6869
> Signed-off-by: Minu Mathai <minu.mathai@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 7213224..c327c7c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3193,7 +3193,7 @@ enum skl_disp_power_wells {
>  #define PCH_HDMIC	0xe1150
>  #define PCH_HDMID	0xe1160
>  
> -#define PORT_DFT_I9XX				0x61150
> +#define PORT_DFT_I9XX				(dev_priv->info.display_mmio_offset + 0x61150)

PORT_DFT_I9XX isn't used on VLV/CHV, so this doesn't change anything.

>  #define   DC_BALANCE_RESET			(1 << 25)
>  #define PORT_DFT2_G4X		(dev_priv->info.display_mmio_offset + 0x61154)
>  #define   DC_BALANCE_RESET_VLV			(1 << 31)
> -- 
> 1.9.1
> 
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ
> VAT No: 860 2173 47
> 
> This e-mail and any attachments may contain confidential material for
> the sole use of the intended recipient(s). Any review or distribution
> by others is strictly prohibited. If you are not the intended
> recipient, please contact the sender and delete all copies.
> 
> _______________________________________________
> 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

Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Mathai, Minu]

This change is needed for some hardware composer tests in chv.

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Friday, June 5, 2015 2:08 PM
To: Mathai, Minu
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

On Fri, Jun 05, 2015 at 02:00:24PM +0100, Minu Mathai wrote:
> From: Minu <minu.mathai@intel.com>
> 
> Display CRCs were not readable because the register defintions for 
> PORT_DFT_I9XX and PORT_DFT2_G4X were wrong.
> MMIO offset needs to be added to these register offsets to fix them.
> 
> Issue: GMINL-6869
> Signed-off-by: Minu Mathai <minu.mathai@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> b/drivers/gpu/drm/i915/i915_reg.h index 7213224..c327c7c 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -3193,7 +3193,7 @@ enum skl_disp_power_wells {
>  #define PCH_HDMIC	0xe1150
>  #define PCH_HDMID	0xe1160
>  
> -#define PORT_DFT_I9XX				0x61150
> +#define PORT_DFT_I9XX				(dev_priv->info.display_mmio_offset + 0x61150)

PORT_DFT_I9XX isn't used on VLV/CHV, so this doesn't change anything.


>  #define   DC_BALANCE_RESET			(1 << 25)
>  #define PORT_DFT2_G4X		(dev_priv->info.display_mmio_offset + 0x61154)
>  #define   DC_BALANCE_RESET_VLV			(1 << 31)
> --
> 1.9.1
> 
> ---------------------------------------------------------------------
> Intel Corporation (UK) Limited
> Registered No. 1134945 (England)
> Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
> 
> This e-mail and any attachments may contain confidential material for 
> the sole use of the intended recipient(s). Any review or distribution 
> by others is strictly prohibited. If you are not the intended 
> recipient, please contact the sender and delete all copies.
> 
> _______________________________________________
> 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

Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Jani Nikula]

On Mon, 08 Jun 2015, "Mathai, Minu" <minu.mathai@intel.com> wrote:
> This change is needed for some hardware composer tests in chv.

As Ville said, this #define is not used by the upstream kernel on
byt/chv. Maybe you have some out-of-tree patches using that?

BR,
Jani.


>
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
> Sent: Friday, June 5, 2015 2:08 PM
> To: Mathai, Minu
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT
>
> On Fri, Jun 05, 2015 at 02:00:24PM +0100, Minu Mathai wrote:
>> From: Minu <minu.mathai@intel.com>
>> 
>> Display CRCs were not readable because the register defintions for 
>> PORT_DFT_I9XX and PORT_DFT2_G4X were wrong.
>> MMIO offset needs to be added to these register offsets to fix them.
>> 
>> Issue: GMINL-6869
>> Signed-off-by: Minu Mathai <minu.mathai@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h index 7213224..c327c7c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3193,7 +3193,7 @@ enum skl_disp_power_wells {
>>  #define PCH_HDMIC	0xe1150
>>  #define PCH_HDMID	0xe1160
>>  
>> -#define PORT_DFT_I9XX				0x61150
>> +#define PORT_DFT_I9XX				(dev_priv->info.display_mmio_offset + 0x61150)
>
> PORT_DFT_I9XX isn't used on VLV/CHV, so this doesn't change anything.
>
>
>>  #define   DC_BALANCE_RESET			(1 << 25)
>>  #define PORT_DFT2_G4X		(dev_priv->info.display_mmio_offset + 0x61154)
>>  #define   DC_BALANCE_RESET_VLV			(1 << 31)
>> --
>> 1.9.1
>> 
>> ---------------------------------------------------------------------
>> Intel Corporation (UK) Limited
>> Registered No. 1134945 (England)
>> Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
>> 
>> This e-mail and any attachments may contain confidential material for 
>> the sole use of the intended recipient(s). Any review or distribution 
>> by others is strictly prohibited. If you are not the intended 
>> recipient, please contact the sender and delete all copies.
>> 
>> _______________________________________________
>> 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

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Mathai, Minu]

The hardware composer test which found the problem with this register definition is used in Android testing and isn't open source. 
However fixing this register definition will stop potential problems for  future use cases and would reduce the rebase effort for our Android tree.

-----Original Message-----
From: Jani Nikula [mailto:jani.nikula@linux.intel.com] 
Sent: Monday, June 8, 2015 2:48 PM
To: Mathai, Minu; Ville Syrjälä
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

On Mon, 08 Jun 2015, "Mathai, Minu" <minu.mathai@intel.com> wrote:
> This change is needed for some hardware composer tests in chv.

As Ville said, this #define is not used by the upstream kernel on byt/chv. Maybe you have some out-of-tree patches using that?

BR,
Jani.


>
> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Friday, June 5, 2015 2:08 PM
> To: Mathai, Minu
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correcting the reg 
> definitions for PORT_DFT
>
> On Fri, Jun 05, 2015 at 02:00:24PM +0100, Minu Mathai wrote:
>> From: Minu <minu.mathai@intel.com>
>> 
>> Display CRCs were not readable because the register defintions for 
>> PORT_DFT_I9XX and PORT_DFT2_G4X were wrong.
>> MMIO offset needs to be added to these register offsets to fix them.
>> 
>> Issue: GMINL-6869
>> Signed-off-by: Minu Mathai <minu.mathai@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
>> b/drivers/gpu/drm/i915/i915_reg.h index 7213224..c327c7c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3193,7 +3193,7 @@ enum skl_disp_power_wells {
>>  #define PCH_HDMIC	0xe1150
>>  #define PCH_HDMID	0xe1160
>>  
>> -#define PORT_DFT_I9XX				0x61150
>> +#define PORT_DFT_I9XX				(dev_priv->info.display_mmio_offset + 0x61150)
>
> PORT_DFT_I9XX isn't used on VLV/CHV, so this doesn't change anything.
>
>
>>  #define   DC_BALANCE_RESET			(1 << 25)
>>  #define PORT_DFT2_G4X		(dev_priv->info.display_mmio_offset + 0x61154)
>>  #define   DC_BALANCE_RESET_VLV			(1 << 31)
>> --
>> 1.9.1
>> 
>> ---------------------------------------------------------------------
>> Intel Corporation (UK) Limited
>> Registered No. 1134945 (England)
>> Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
>> 
>> This e-mail and any attachments may contain confidential material for 
>> the sole use of the intended recipient(s). Any review or distribution 
>> by others is strictly prohibited. If you are not the intended 
>> recipient, please contact the sender and delete all copies.
>> 
>> _______________________________________________
>> 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

--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Ville Syrjälä]

On Tue, Jun 09, 2015 at 12:06:36PM +0000, Mathai, Minu wrote:
> The hardware composer test which found the problem with this register definition is used in Android testing and isn't open source. 
> However fixing this register definition will stop potential problems for  future use cases and would reduce the rebase effort for our Android tree.

What are you testing with this register? The register isn't even listed
in any VLV/CHV documentation so I have no idea what you would even do
with it.

> 
> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com] 
> Sent: Monday, June 8, 2015 2:48 PM
> To: Mathai, Minu; Ville Syrjälä
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT
> 
> On Mon, 08 Jun 2015, "Mathai, Minu" <minu.mathai@intel.com> wrote:
> > This change is needed for some hardware composer tests in chv.
> 
> As Ville said, this #define is not used by the upstream kernel on byt/chv. Maybe you have some out-of-tree patches using that?
> 
> BR,
> Jani.
> 
> 
> >
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Friday, June 5, 2015 2:08 PM
> > To: Mathai, Minu
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correcting the reg 
> > definitions for PORT_DFT
> >
> > On Fri, Jun 05, 2015 at 02:00:24PM +0100, Minu Mathai wrote:
> >> From: Minu <minu.mathai@intel.com>
> >> 
> >> Display CRCs were not readable because the register defintions for 
> >> PORT_DFT_I9XX and PORT_DFT2_G4X were wrong.
> >> MMIO offset needs to be added to these register offsets to fix them.
> >> 
> >> Issue: GMINL-6869
> >> Signed-off-by: Minu Mathai <minu.mathai@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_reg.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> >> b/drivers/gpu/drm/i915/i915_reg.h index 7213224..c327c7c 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -3193,7 +3193,7 @@ enum skl_disp_power_wells {
> >>  #define PCH_HDMIC	0xe1150
> >>  #define PCH_HDMID	0xe1160
> >>  
> >> -#define PORT_DFT_I9XX				0x61150
> >> +#define PORT_DFT_I9XX				(dev_priv->info.display_mmio_offset + 0x61150)
> >
> > PORT_DFT_I9XX isn't used on VLV/CHV, so this doesn't change anything.
> >
> >
> >>  #define   DC_BALANCE_RESET			(1 << 25)
> >>  #define PORT_DFT2_G4X		(dev_priv->info.display_mmio_offset + 0x61154)
> >>  #define   DC_BALANCE_RESET_VLV			(1 << 31)
> >> --
> >> 1.9.1
> >> 
> >> ---------------------------------------------------------------------
> >> Intel Corporation (UK) Limited
> >> Registered No. 1134945 (England)
> >> Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
> >> 
> >> This e-mail and any attachments may contain confidential material for 
> >> the sole use of the intended recipient(s). Any review or distribution 
> >> by others is strictly prohibited. If you are not the intended 
> >> recipient, please contact the sender and delete all copies.
> >> 
> >> _______________________________________________
> >> 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
> 
> --
> Jani Nikula, Intel Open Source Technology Center

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Mathai, Minu]

Agreed .We don't need this

Thanks
Minu

-----Original Message-----
From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com] 
Sent: Tuesday, June 9, 2015 1:17 PM
To: Mathai, Minu
Cc: Jani Nikula; intel-gfx@lists.freedesktop.org
Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

On Tue, Jun 09, 2015 at 12:06:36PM +0000, Mathai, Minu wrote:
> The hardware composer test which found the problem with this register definition is used in Android testing and isn't open source. 
> However fixing this register definition will stop potential problems for  future use cases and would reduce the rebase effort for our Android tree.

What are you testing with this register? The register isn't even listed in any VLV/CHV documentation so I have no idea what you would even do with it.

> 
> -----Original Message-----
> From: Jani Nikula [mailto:jani.nikula@linux.intel.com]
> Sent: Monday, June 8, 2015 2:48 PM
> To: Mathai, Minu; Ville Syrjälä
> Cc: intel-gfx@lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correcting the reg 
> definitions for PORT_DFT
> 
> On Mon, 08 Jun 2015, "Mathai, Minu" <minu.mathai@intel.com> wrote:
> > This change is needed for some hardware composer tests in chv.
> 
> As Ville said, this #define is not used by the upstream kernel on byt/chv. Maybe you have some out-of-tree patches using that?
> 
> BR,
> Jani.
> 
> 
> >
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Friday, June 5, 2015 2:08 PM
> > To: Mathai, Minu
> > Cc: intel-gfx@lists.freedesktop.org
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Correcting the reg 
> > definitions for PORT_DFT
> >
> > On Fri, Jun 05, 2015 at 02:00:24PM +0100, Minu Mathai wrote:
> >> From: Minu <minu.mathai@intel.com>
> >> 
> >> Display CRCs were not readable because the register defintions for 
> >> PORT_DFT_I9XX and PORT_DFT2_G4X were wrong.
> >> MMIO offset needs to be added to these register offsets to fix them.
> >> 
> >> Issue: GMINL-6869
> >> Signed-off-by: Minu Mathai <minu.mathai@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_reg.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h 
> >> b/drivers/gpu/drm/i915/i915_reg.h index 7213224..c327c7c 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -3193,7 +3193,7 @@ enum skl_disp_power_wells {
> >>  #define PCH_HDMIC	0xe1150
> >>  #define PCH_HDMID	0xe1160
> >>  
> >> -#define PORT_DFT_I9XX				0x61150
> >> +#define PORT_DFT_I9XX				(dev_priv->info.display_mmio_offset + 0x61150)
> >
> > PORT_DFT_I9XX isn't used on VLV/CHV, so this doesn't change anything.
> >
> >
> >>  #define   DC_BALANCE_RESET			(1 << 25)
> >>  #define PORT_DFT2_G4X		(dev_priv->info.display_mmio_offset + 0x61154)
> >>  #define   DC_BALANCE_RESET_VLV			(1 << 31)
> >> --
> >> 1.9.1
> >> 
> >> -------------------------------------------------------------------
> >> --
> >> Intel Corporation (UK) Limited
> >> Registered No. 1134945 (England)
> >> Registered Office: Pipers Way, Swindon SN3 1RJ VAT No: 860 2173 47
> >> 
> >> This e-mail and any attachments may contain confidential material 
> >> for the sole use of the intended recipient(s). Any review or 
> >> distribution by others is strictly prohibited. If you are not the 
> >> intended recipient, please contact the sender and delete all copies.
> >> 
> >> _______________________________________________
> >> 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
> 
> --
> Jani Nikula, Intel Open Source Technology Center

--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Dave Gordon]

On 05/06/15 14:08, Ville Syrjälä wrote:
> On Fri, Jun 05, 2015 at 02:00:24PM +0100, Minu Mathai wrote:
>> From: Minu <minu.mathai@intel.com>
>>
>> Display CRCs were not readable because the register defintions
>> for PORT_DFT_I9XX and PORT_DFT2_G4X were wrong.
>> MMIO offset needs to be added to these register offsets to fix them.
>>
>> Issue: GMINL-6869
>> Signed-off-by: Minu Mathai <minu.mathai@intel.com>
>> ---
>>  drivers/gpu/drm/i915/i915_reg.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
>> index 7213224..c327c7c 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -3193,7 +3193,7 @@ enum skl_disp_power_wells {
>>  #define PCH_HDMIC	0xe1150
>>  #define PCH_HDMID	0xe1160
>>  
>> -#define PORT_DFT_I9XX				0x61150
>> +#define PORT_DFT_I9XX				(dev_priv->info.display_mmio_offset + 0x61150)
> 
> PORT_DFT_I9XX isn't used on VLV/CHV, so this doesn't change anything.
> 
>>  #define   DC_BALANCE_RESET			(1 << 25)
>>  #define PORT_DFT2_G4X		(dev_priv->info.display_mmio_offset + 0x61154)
>>  #define   DC_BALANCE_RESET_VLV			(1 << 31)

Regardless of whether it's used, we have an inconsistency between the
definitions of PORT_DFT_I9XX and PORT_DFT2_G4X -- one includes the
mmio_offset and the other doesn't. Personally I think the #define with
an implicit dependency on an object called "dev_priv" is really ugly and
we should move away from that style rather than adding mode of them, but
that's a lot of work.

As Minu says PORT_DFT_I9XX isn't really needed after all, can we just
delete it to remove the inconsistency?

.Dave.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Ville Syrjälä]

On Tue, Jun 09, 2015 at 04:01:29PM +0100, Dave Gordon wrote:
> On 05/06/15 14:08, Ville Syrjälä wrote:
> > On Fri, Jun 05, 2015 at 02:00:24PM +0100, Minu Mathai wrote:
> >> From: Minu <minu.mathai@intel.com>
> >>
> >> Display CRCs were not readable because the register defintions
> >> for PORT_DFT_I9XX and PORT_DFT2_G4X were wrong.
> >> MMIO offset needs to be added to these register offsets to fix them.
> >>
> >> Issue: GMINL-6869
> >> Signed-off-by: Minu Mathai <minu.mathai@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_reg.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> >> index 7213224..c327c7c 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -3193,7 +3193,7 @@ enum skl_disp_power_wells {
> >>  #define PCH_HDMIC	0xe1150
> >>  #define PCH_HDMID	0xe1160
> >>  
> >> -#define PORT_DFT_I9XX				0x61150
> >> +#define PORT_DFT_I9XX				(dev_priv->info.display_mmio_offset + 0x61150)
> > 
> > PORT_DFT_I9XX isn't used on VLV/CHV, so this doesn't change anything.
> > 
> >>  #define   DC_BALANCE_RESET			(1 << 25)
> >>  #define PORT_DFT2_G4X		(dev_priv->info.display_mmio_offset + 0x61154)
> >>  #define   DC_BALANCE_RESET_VLV			(1 << 31)
> 
> Regardless of whether it's used, we have an inconsistency between the
> definitions of PORT_DFT_I9XX and PORT_DFT2_G4X -- one includes the
> mmio_offset and the other doesn't. Personally I think the #define with
> an implicit dependency on an object called "dev_priv" is really ugly and
> we should move away from that style rather than adding mode of them, but
> that's a lot of work.
> 
> As Minu says PORT_DFT_I9XX isn't really needed after all, can we just
> delete it to remove the inconsistency?

No, it's used on g4x.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Jani Nikula]

On Tue, 09 Jun 2015, Dave Gordon <david.s.gordon@intel.com> wrote:
> Regardless of whether it's used, we have an inconsistency between the
> definitions of PORT_DFT_I9XX and PORT_DFT2_G4X -- one includes the
> mmio_offset and the other doesn't.

It's not inconsistent, it's consistent on another level:

We've settled on including the mmio_offset only for macros that need
it. If a macro is relevant only on platforms that all have the same mmio
offset, the offset is included statically (currently 0 or
VLV_DISPLAY_BASE). dev_priv->info.display_mmio_offset is only used when
the macro is relevant on platforms with different mmio offsets. This we
try to follow consistently.

> Personally I think the #define with an implicit dependency on an
> object called "dev_priv" is really ugly and we should move away from
> that style rather than adding mode of them, but that's a lot of work.

Agreed in principle, but I think we've lost the battle. It's way too
much churn, and makes a lot of code really ugly. Compare these:

	I915_WRITE(FOOBAR, I915_READ(FOOBAR) | FOOBAR_ENABLE);

	I915_WRITE(dev_priv, FOOBAR(dev_priv),
		   I915_READ(dev_priv, FOOBAR(dev_priv)) | FOOBAR_ENABLE);

We'll try to focus on only requiring "dev_priv" implicitly and nothing
else, and where a parameter does get passed, we'll try to make it
"dev_priv" as that pretty much has to be around anyway.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Dave Gordon]

On 10/06/15 09:09, Jani Nikula wrote:
> On Tue, 09 Jun 2015, Dave Gordon <david.s.gordon@intel.com> wrote:
>> Regardless of whether it's used, we have an inconsistency between the
>> definitions of PORT_DFT_I9XX and PORT_DFT2_G4X -- one includes the
>> mmio_offset and the other doesn't.
> 
> It's not inconsistent, it's consistent on another level:
> 
> We've settled on including the mmio_offset only for macros that need
> it. If a macro is relevant only on platforms that all have the same mmio
> offset, the offset is included statically (currently 0 or
> VLV_DISPLAY_BASE). dev_priv->info.display_mmio_offset is only used when
> the macro is relevant on platforms with different mmio offsets. This we
> try to follow consistently.

So you only have a problem when a previously-statically located block
turns into something that can be (and is) moved around on different
platforms ... I suppose that shouldn't happen too often.

>> Personally I think the #define with an implicit dependency on an
>> object called "dev_priv" is really ugly and we should move away from
>> that style rather than adding more of them, but that's a lot of work.
> 
> Agreed in principle, but I think we've lost the battle. It's way too
> much churn, and makes a lot of code really ugly. Compare these:
> 
> 	I915_WRITE(FOOBAR, I915_READ(FOOBAR) | FOOBAR_ENABLE);
> 
> 	I915_WRITE(dev_priv, FOOBAR(dev_priv),
> 		   I915_READ(dev_priv, FOOBAR(dev_priv)) | FOOBAR_ENABLE);

I'd rather write

	uint32_t foobar = I915_DISP_REG_READ(dev_priv, FOOBAR);
	I915_DISP_REG_WRITE(dev_priv, foobar | FOOBAR_ENABLE);

And if there were very many places where that read-modify-write was
used, we could define that as

#define	I915_DISP_REG_BITSET(dev_priv, reg, bits) ...

Of course it would require people to use the right macro according to
whether the register was part of the display block, the GT block, or
however many other areas there are that might be independently relocated
... but it might be more future proof :)

> We'll try to focus on only requiring "dev_priv" implicitly and nothing
> else, and where a parameter does get passed, we'll try to make it
> "dev_priv" as that pretty much has to be around anyway.
> 
> BR,
> Jani.

Well I guess we're stuck with 'dev_priv' for the foreseeable future :(

Hmmm ... we have quite a bit of code that passes 'dev' around rather
than dev_priv, where the (usually static) called function immediately
uses it to derive dev_priv, and where the only other uses in the called
function are in calls to INTEL_INFO(dev) and its derivatives. For example:

static int get_context_size(struct drm_device *dev)
{
        struct drm_i915_private *dev_priv = dev->dev_private;
        int ret;
        u32 reg;

        switch (INTEL_INFO(dev)->gen) {
        case 6:
                reg = I915_READ(CXT_SIZE);
                ret = GEN6_CXT_TOTAL_SIZE(reg) * 64;
                break;
        case 7:
                reg = I915_READ(GEN7_CXT_SIZE);
                if (IS_HASWELL(dev))
                        ret = HSW_CXT_TOTAL_SIZE;
                else
                        ret = GEN7_CXT_TOTAL_SIZE(reg) * 64;
                break;
        case 8:
                ret = GEN8_CXT_TOTAL_SIZE;
                break;
        default:
                BUG();
        }

        return ret;
}

But since INTEL_INFO() can take 'dev_priv' as a parameter (as an
alternative to 'dev'), all the uses of 'dev' here could be replaced by
'dev_priv' and then the parameter changed to pass that directly, thus
potentially eliminating quite a few extra memory references. Applied
driver-wide, that micro-optimisation might add up to something useful :)

.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Correcting the reg definitions for PORT_DFT

[Jani Nikula]

On Wed, 10 Jun 2015, Dave Gordon <david.s.gordon@intel.com> wrote:
> But since INTEL_INFO() can take 'dev_priv' as a parameter (as an
> alternative to 'dev'), all the uses of 'dev' here could be replaced by
> 'dev_priv' and then the parameter changed to pass that directly, thus
> potentially eliminating quite a few extra memory references. Applied
> driver-wide, that micro-optimisation might add up to something useful :)

I think we haven't done that driver-wide because it would be more
disruptive than beneficial, but we're slowly moving at that direction
whenever we change stuff. And this is exactly the reason why Chris
hacked INTEL_INFO et al. to accept either dev or dev_priv.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx