subject:"\[Intel\-gfx\] \[PATCH 05\/20\] drm\/i915\: Introduce AUX_CH

Re: [Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

2020-10-22 Thread Lucas De Marchi


On Fri, Oct 23, 2020 at 02:56:21AM +0300, Ville Syrjälä wrote:

On Thu, Oct 08, 2020 at 01:52:30AM -0700, Lucas De Marchi wrote:

On Thu, Oct 08, 2020 at 11:40:28AM +0300, Ville Syrjälä wrote:
>On Wed, Oct 07, 2020 at 03:51:11PM -0700, Lucas De Marchi wrote:
>> On Tue, Oct 06, 2020 at 05:33:34PM +0300, Ville Syrjälä wrote:
>> >From: Ville Syrjälä 
>> >
>> >Just like with the DDIs tgl+ renamed the AUX CHs to reflect
>> >the type of the DDI. Let's add the aliasing enum values for
>> >the type-C AUX CHs.
>> >
>> >Signed-off-by: Ville Syrjälä 
>> >---
>> > drivers/gpu/drm/i915/display/intel_display.h |  8 +++
>> > drivers/gpu/drm/i915/display/intel_dp.c  | 53 ++--
>> > 2 files changed, 58 insertions(+), 3 deletions(-)
>> >
>> >diff --git a/drivers/gpu/drm/i915/display/intel_display.h 
b/drivers/gpu/drm/i915/display/intel_display.h
>> >index a39be3c9e0cf..cba876721ea0 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_display.h
>> >+++ b/drivers/gpu/drm/i915/display/intel_display.h
>> >@@ -290,6 +290,14 @@ enum aux_ch {
>> >   AUX_CH_G,
>> >   AUX_CH_H,
>> >   AUX_CH_I,
>> >+
>> >+  /* tgl+ */
>> >+  AUX_CH_USBC1 = AUX_CH_D,
>> >+  AUX_CH_USBC2,
>> >+  AUX_CH_USBC3,
>> >+  AUX_CH_USBC4,
>> >+  AUX_CH_USBC5,
>> >+  AUX_CH_USBC6,
>> > };
>> >
>> > #define aux_ch_name(a) ((a) + 'A')
>> >diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
b/drivers/gpu/drm/i915/display/intel_dp.c
>> >index 239016dcd544..a73c354c920e 100644
>> >--- a/drivers/gpu/drm/i915/display/intel_dp.c
>> >+++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> >@@ -1792,7 +1792,6 @@ static i915_reg_t skl_aux_ctl_reg(struct intel_dp 
*intel_dp)
>> >   case AUX_CH_D:
>> >   case AUX_CH_E:
>> >   case AUX_CH_F:
>> >-  case AUX_CH_G:
>> >   return DP_AUX_CH_CTL(aux_ch);
>> >   default:
>> >   MISSING_CASE(aux_ch);
>> >@@ -1813,7 +1812,52 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp 
*intel_dp, int index)
>> >   case AUX_CH_D:
>> >   case AUX_CH_E:
>> >   case AUX_CH_F:
>> >-  case AUX_CH_G:
>> >+  return DP_AUX_CH_DATA(aux_ch, index);
>> >+  default:
>> >+  MISSING_CASE(aux_ch);
>> >+  return DP_AUX_CH_DATA(AUX_CH_A, index);
>> >+  }
>> >+}
>> >+
>> >+static i915_reg_t tgl_aux_ctl_reg(struct intel_dp *intel_dp)
>> >+{
>> >+  struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> >+  struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> >+  enum aux_ch aux_ch = dig_port->aux_ch;
>> >+
>> >+  switch (aux_ch) {
>> >+  case AUX_CH_A:
>> >+  case AUX_CH_B:
>> >+  case AUX_CH_C:
>> >+  case AUX_CH_USBC1:
>> >+  case AUX_CH_USBC2:
>> >+  case AUX_CH_USBC3:
>> >+  case AUX_CH_USBC4:
>> >+  case AUX_CH_USBC5:
>> >+  case AUX_CH_USBC6:
>> >+  return DP_AUX_CH_CTL(aux_ch);
>> >+  default:
>> >+  MISSING_CASE(aux_ch);
>> >+  return DP_AUX_CH_CTL(AUX_CH_A);
>> >+  }
>> >+}
>> >+
>> >+static i915_reg_t tgl_aux_data_reg(struct intel_dp *intel_dp, int index)
>> >+{
>> >+  struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>> >+  struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> >+  enum aux_ch aux_ch = dig_port->aux_ch;
>> >+
>> >+  switch (aux_ch) {
>> >+  case AUX_CH_A:
>> >+  case AUX_CH_B:
>> >+  case AUX_CH_C:
>> >+  case AUX_CH_USBC1:
>> >+  case AUX_CH_USBC2:
>> >+  case AUX_CH_USBC3:
>> >+  case AUX_CH_USBC4:
>> >+  case AUX_CH_USBC5:
>> >+  case AUX_CH_USBC6:
>> >   return DP_AUX_CH_DATA(aux_ch, index);
>> >   default:
>> >   MISSING_CASE(aux_ch);
>> >@@ -1834,7 +1878,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
>> >   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>> >   struct intel_encoder *encoder = &dig_port->base;
>> >
>> >-  if (INTEL_GEN(dev_priv) >= 9) {
>> >+  if (INTEL_GEN(dev_priv) >= 12) {
>> >+  intel_dp->aux_ch_ctl_reg = tgl_aux_ctl_reg;
>>
>> why is this even a function pointer rather than just the reg? AFAICS it
>> only depends on dig_port->aux_ch that is initialized in intel_ddi_init()
>
>Just for consistency with .aux_ch_data_reg() I guess. Can't remember
>a more specific reason at least.

even that may be overkill since all the users just use index to
do `+ index * 4`


The code used to do that but we got rid of it when the i915_reg
stuff was introduced to discourage people from doing hand rolled
arithmetic on register offsets. I think the tradeoff has been
generally worth it because I can't remeber the last time someone
messed up the register offsets. Before type safety it was a
somewhat regular occurance.


ok. Just so patchwork gets it now:


Reviewed-by: Lucas De Marchi 

Lucas De Marchi



--
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

2020-10-22 Thread Ville Syrjälä

On Thu, Oct 08, 2020 at 01:52:30AM -0700, Lucas De Marchi wrote:
> On Thu, Oct 08, 2020 at 11:40:28AM +0300, Ville Syrjälä wrote:
> >On Wed, Oct 07, 2020 at 03:51:11PM -0700, Lucas De Marchi wrote:
> >> On Tue, Oct 06, 2020 at 05:33:34PM +0300, Ville Syrjälä wrote:
> >> >From: Ville Syrjälä 
> >> >
> >> >Just like with the DDIs tgl+ renamed the AUX CHs to reflect
> >> >the type of the DDI. Let's add the aliasing enum values for
> >> >the type-C AUX CHs.
> >> >
> >> >Signed-off-by: Ville Syrjälä 
> >> >---
> >> > drivers/gpu/drm/i915/display/intel_display.h |  8 +++
> >> > drivers/gpu/drm/i915/display/intel_dp.c  | 53 ++--
> >> > 2 files changed, 58 insertions(+), 3 deletions(-)
> >> >
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_display.h 
> >> >b/drivers/gpu/drm/i915/display/intel_display.h
> >> >index a39be3c9e0cf..cba876721ea0 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_display.h
> >> >+++ b/drivers/gpu/drm/i915/display/intel_display.h
> >> >@@ -290,6 +290,14 @@ enum aux_ch {
> >> >  AUX_CH_G,
> >> >  AUX_CH_H,
> >> >  AUX_CH_I,
> >> >+
> >> >+ /* tgl+ */
> >> >+ AUX_CH_USBC1 = AUX_CH_D,
> >> >+ AUX_CH_USBC2,
> >> >+ AUX_CH_USBC3,
> >> >+ AUX_CH_USBC4,
> >> >+ AUX_CH_USBC5,
> >> >+ AUX_CH_USBC6,
> >> > };
> >> >
> >> > #define aux_ch_name(a) ((a) + 'A')
> >> >diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> >> >b/drivers/gpu/drm/i915/display/intel_dp.c
> >> >index 239016dcd544..a73c354c920e 100644
> >> >--- a/drivers/gpu/drm/i915/display/intel_dp.c
> >> >+++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >> >@@ -1792,7 +1792,6 @@ static i915_reg_t skl_aux_ctl_reg(struct intel_dp 
> >> >*intel_dp)
> >> >  case AUX_CH_D:
> >> >  case AUX_CH_E:
> >> >  case AUX_CH_F:
> >> >- case AUX_CH_G:
> >> >  return DP_AUX_CH_CTL(aux_ch);
> >> >  default:
> >> >  MISSING_CASE(aux_ch);
> >> >@@ -1813,7 +1812,52 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp 
> >> >*intel_dp, int index)
> >> >  case AUX_CH_D:
> >> >  case AUX_CH_E:
> >> >  case AUX_CH_F:
> >> >- case AUX_CH_G:
> >> >+ return DP_AUX_CH_DATA(aux_ch, index);
> >> >+ default:
> >> >+ MISSING_CASE(aux_ch);
> >> >+ return DP_AUX_CH_DATA(AUX_CH_A, index);
> >> >+ }
> >> >+}
> >> >+
> >> >+static i915_reg_t tgl_aux_ctl_reg(struct intel_dp *intel_dp)
> >> >+{
> >> >+ struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >> >+ struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >> >+ enum aux_ch aux_ch = dig_port->aux_ch;
> >> >+
> >> >+ switch (aux_ch) {
> >> >+ case AUX_CH_A:
> >> >+ case AUX_CH_B:
> >> >+ case AUX_CH_C:
> >> >+ case AUX_CH_USBC1:
> >> >+ case AUX_CH_USBC2:
> >> >+ case AUX_CH_USBC3:
> >> >+ case AUX_CH_USBC4:
> >> >+ case AUX_CH_USBC5:
> >> >+ case AUX_CH_USBC6:
> >> >+ return DP_AUX_CH_CTL(aux_ch);
> >> >+ default:
> >> >+ MISSING_CASE(aux_ch);
> >> >+ return DP_AUX_CH_CTL(AUX_CH_A);
> >> >+ }
> >> >+}
> >> >+
> >> >+static i915_reg_t tgl_aux_data_reg(struct intel_dp *intel_dp, int index)
> >> >+{
> >> >+ struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >> >+ struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >> >+ enum aux_ch aux_ch = dig_port->aux_ch;
> >> >+
> >> >+ switch (aux_ch) {
> >> >+ case AUX_CH_A:
> >> >+ case AUX_CH_B:
> >> >+ case AUX_CH_C:
> >> >+ case AUX_CH_USBC1:
> >> >+ case AUX_CH_USBC2:
> >> >+ case AUX_CH_USBC3:
> >> >+ case AUX_CH_USBC4:
> >> >+ case AUX_CH_USBC5:
> >> >+ case AUX_CH_USBC6:
> >> >  return DP_AUX_CH_DATA(aux_ch, index);
> >> >  default:
> >> >  MISSING_CASE(aux_ch);
> >> >@@ -1834,7 +1878,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
> >> >  struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >> >  struct intel_encoder *encoder = &dig_port->base;
> >> >
> >> >- if (INTEL_GEN(dev_priv) >= 9) {
> >> >+ if (INTEL_GEN(dev_priv) >= 12) {
> >> >+ intel_dp->aux_ch_ctl_reg = tgl_aux_ctl_reg;
> >>
> >> why is this even a function pointer rather than just the reg? AFAICS it
> >> only depends on dig_port->aux_ch that is initialized in intel_ddi_init()
> >
> >Just for consistency with .aux_ch_data_reg() I guess. Can't remember
> >a more specific reason at least.
> 
> even that may be overkill since all the users just use index to
> do `+ index * 4`

The code used to do that but we got rid of it when the i915_reg
stuff was introduced to discourage people from doing hand rolled 
arithmetic on register offsets. I think the tradeoff has been
generally worth it because I can't remeber the last time someone
messed up the register offsets. Before type safety it was a
somewhat regular occurance.

-- 
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

2020-10-08 Thread Lucas De Marchi


On Thu, Oct 08, 2020 at 11:40:28AM +0300, Ville Syrjälä wrote:

On Wed, Oct 07, 2020 at 03:51:11PM -0700, Lucas De Marchi wrote:

On Tue, Oct 06, 2020 at 05:33:34PM +0300, Ville Syrjälä wrote:
>From: Ville Syrjälä 
>
>Just like with the DDIs tgl+ renamed the AUX CHs to reflect
>the type of the DDI. Let's add the aliasing enum values for
>the type-C AUX CHs.
>
>Signed-off-by: Ville Syrjälä 
>---
> drivers/gpu/drm/i915/display/intel_display.h |  8 +++
> drivers/gpu/drm/i915/display/intel_dp.c  | 53 ++--
> 2 files changed, 58 insertions(+), 3 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/display/intel_display.h 
b/drivers/gpu/drm/i915/display/intel_display.h
>index a39be3c9e0cf..cba876721ea0 100644
>--- a/drivers/gpu/drm/i915/display/intel_display.h
>+++ b/drivers/gpu/drm/i915/display/intel_display.h
>@@ -290,6 +290,14 @@ enum aux_ch {
>AUX_CH_G,
>AUX_CH_H,
>AUX_CH_I,
>+
>+   /* tgl+ */
>+   AUX_CH_USBC1 = AUX_CH_D,
>+   AUX_CH_USBC2,
>+   AUX_CH_USBC3,
>+   AUX_CH_USBC4,
>+   AUX_CH_USBC5,
>+   AUX_CH_USBC6,
> };
>
> #define aux_ch_name(a) ((a) + 'A')
>diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
b/drivers/gpu/drm/i915/display/intel_dp.c
>index 239016dcd544..a73c354c920e 100644
>--- a/drivers/gpu/drm/i915/display/intel_dp.c
>+++ b/drivers/gpu/drm/i915/display/intel_dp.c
>@@ -1792,7 +1792,6 @@ static i915_reg_t skl_aux_ctl_reg(struct intel_dp 
*intel_dp)
>case AUX_CH_D:
>case AUX_CH_E:
>case AUX_CH_F:
>-   case AUX_CH_G:
>return DP_AUX_CH_CTL(aux_ch);
>default:
>MISSING_CASE(aux_ch);
>@@ -1813,7 +1812,52 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp 
*intel_dp, int index)
>case AUX_CH_D:
>case AUX_CH_E:
>case AUX_CH_F:
>-   case AUX_CH_G:
>+   return DP_AUX_CH_DATA(aux_ch, index);
>+   default:
>+   MISSING_CASE(aux_ch);
>+   return DP_AUX_CH_DATA(AUX_CH_A, index);
>+   }
>+}
>+
>+static i915_reg_t tgl_aux_ctl_reg(struct intel_dp *intel_dp)
>+{
>+   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>+   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>+   enum aux_ch aux_ch = dig_port->aux_ch;
>+
>+   switch (aux_ch) {
>+   case AUX_CH_A:
>+   case AUX_CH_B:
>+   case AUX_CH_C:
>+   case AUX_CH_USBC1:
>+   case AUX_CH_USBC2:
>+   case AUX_CH_USBC3:
>+   case AUX_CH_USBC4:
>+   case AUX_CH_USBC5:
>+   case AUX_CH_USBC6:
>+   return DP_AUX_CH_CTL(aux_ch);
>+   default:
>+   MISSING_CASE(aux_ch);
>+   return DP_AUX_CH_CTL(AUX_CH_A);
>+   }
>+}
>+
>+static i915_reg_t tgl_aux_data_reg(struct intel_dp *intel_dp, int index)
>+{
>+   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
>+   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>+   enum aux_ch aux_ch = dig_port->aux_ch;
>+
>+   switch (aux_ch) {
>+   case AUX_CH_A:
>+   case AUX_CH_B:
>+   case AUX_CH_C:
>+   case AUX_CH_USBC1:
>+   case AUX_CH_USBC2:
>+   case AUX_CH_USBC3:
>+   case AUX_CH_USBC4:
>+   case AUX_CH_USBC5:
>+   case AUX_CH_USBC6:
>return DP_AUX_CH_DATA(aux_ch, index);
>default:
>MISSING_CASE(aux_ch);
>@@ -1834,7 +1878,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
>struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>struct intel_encoder *encoder = &dig_port->base;
>
>-   if (INTEL_GEN(dev_priv) >= 9) {
>+   if (INTEL_GEN(dev_priv) >= 12) {
>+   intel_dp->aux_ch_ctl_reg = tgl_aux_ctl_reg;

why is this even a function pointer rather than just the reg? AFAICS it
only depends on dig_port->aux_ch that is initialized in intel_ddi_init()


Just for consistency with .aux_ch_data_reg() I guess. Can't remember
a more specific reason at least.


even that may be overkill since all the users just use index to
do `+ index * 4`

Lucas De Marchi





but could be orthogonal to the change here.


Reviewed-by: Lucas De Marchi 

Lucas De Marchi

>+   intel_dp->aux_ch_data_reg = tgl_aux_data_reg;
>+   } else if (INTEL_GEN(dev_priv) >= 9) {
>intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
>intel_dp->aux_ch_data_reg = skl_aux_data_reg;
>} else if (HAS_PCH_SPLIT(dev_priv)) {
>--
>2.26.2
>
>___
>Intel-gfx mailing list
>Intel-gfx@lists.freedesktop.org
>https://lists.freedesktop.org/mailman/listinfo/intel-gfx


--
Ville Syrjälä
Intel

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

2020-10-08 Thread Ville Syrjälä

On Wed, Oct 07, 2020 at 03:51:11PM -0700, Lucas De Marchi wrote:
> On Tue, Oct 06, 2020 at 05:33:34PM +0300, Ville Syrjälä wrote:
> >From: Ville Syrjälä 
> >
> >Just like with the DDIs tgl+ renamed the AUX CHs to reflect
> >the type of the DDI. Let's add the aliasing enum values for
> >the type-C AUX CHs.
> >
> >Signed-off-by: Ville Syrjälä 
> >---
> > drivers/gpu/drm/i915/display/intel_display.h |  8 +++
> > drivers/gpu/drm/i915/display/intel_dp.c  | 53 ++--
> > 2 files changed, 58 insertions(+), 3 deletions(-)
> >
> >diff --git a/drivers/gpu/drm/i915/display/intel_display.h 
> >b/drivers/gpu/drm/i915/display/intel_display.h
> >index a39be3c9e0cf..cba876721ea0 100644
> >--- a/drivers/gpu/drm/i915/display/intel_display.h
> >+++ b/drivers/gpu/drm/i915/display/intel_display.h
> >@@ -290,6 +290,14 @@ enum aux_ch {
> > AUX_CH_G,
> > AUX_CH_H,
> > AUX_CH_I,
> >+
> >+/* tgl+ */
> >+AUX_CH_USBC1 = AUX_CH_D,
> >+AUX_CH_USBC2,
> >+AUX_CH_USBC3,
> >+AUX_CH_USBC4,
> >+AUX_CH_USBC5,
> >+AUX_CH_USBC6,
> > };
> >
> > #define aux_ch_name(a) ((a) + 'A')
> >diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
> >b/drivers/gpu/drm/i915/display/intel_dp.c
> >index 239016dcd544..a73c354c920e 100644
> >--- a/drivers/gpu/drm/i915/display/intel_dp.c
> >+++ b/drivers/gpu/drm/i915/display/intel_dp.c
> >@@ -1792,7 +1792,6 @@ static i915_reg_t skl_aux_ctl_reg(struct intel_dp 
> >*intel_dp)
> > case AUX_CH_D:
> > case AUX_CH_E:
> > case AUX_CH_F:
> >-case AUX_CH_G:
> > return DP_AUX_CH_CTL(aux_ch);
> > default:
> > MISSING_CASE(aux_ch);
> >@@ -1813,7 +1812,52 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp 
> >*intel_dp, int index)
> > case AUX_CH_D:
> > case AUX_CH_E:
> > case AUX_CH_F:
> >-case AUX_CH_G:
> >+return DP_AUX_CH_DATA(aux_ch, index);
> >+default:
> >+MISSING_CASE(aux_ch);
> >+return DP_AUX_CH_DATA(AUX_CH_A, index);
> >+}
> >+}
> >+
> >+static i915_reg_t tgl_aux_ctl_reg(struct intel_dp *intel_dp)
> >+{
> >+struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >+struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >+enum aux_ch aux_ch = dig_port->aux_ch;
> >+
> >+switch (aux_ch) {
> >+case AUX_CH_A:
> >+case AUX_CH_B:
> >+case AUX_CH_C:
> >+case AUX_CH_USBC1:
> >+case AUX_CH_USBC2:
> >+case AUX_CH_USBC3:
> >+case AUX_CH_USBC4:
> >+case AUX_CH_USBC5:
> >+case AUX_CH_USBC6:
> >+return DP_AUX_CH_CTL(aux_ch);
> >+default:
> >+MISSING_CASE(aux_ch);
> >+return DP_AUX_CH_CTL(AUX_CH_A);
> >+}
> >+}
> >+
> >+static i915_reg_t tgl_aux_data_reg(struct intel_dp *intel_dp, int index)
> >+{
> >+struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
> >+struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >+enum aux_ch aux_ch = dig_port->aux_ch;
> >+
> >+switch (aux_ch) {
> >+case AUX_CH_A:
> >+case AUX_CH_B:
> >+case AUX_CH_C:
> >+case AUX_CH_USBC1:
> >+case AUX_CH_USBC2:
> >+case AUX_CH_USBC3:
> >+case AUX_CH_USBC4:
> >+case AUX_CH_USBC5:
> >+case AUX_CH_USBC6:
> > return DP_AUX_CH_DATA(aux_ch, index);
> > default:
> > MISSING_CASE(aux_ch);
> >@@ -1834,7 +1878,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
> > struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> > struct intel_encoder *encoder = &dig_port->base;
> >
> >-if (INTEL_GEN(dev_priv) >= 9) {
> >+if (INTEL_GEN(dev_priv) >= 12) {
> >+intel_dp->aux_ch_ctl_reg = tgl_aux_ctl_reg;
> 
> why is this even a function pointer rather than just the reg? AFAICS it
> only depends on dig_port->aux_ch that is initialized in intel_ddi_init()

Just for consistency with .aux_ch_data_reg() I guess. Can't remember
a more specific reason at least.

> 
> but could be orthogonal to the change here.
> 
> 
> Reviewed-by: Lucas De Marchi 
> 
> Lucas De Marchi
> 
> >+intel_dp->aux_ch_data_reg = tgl_aux_data_reg;
> >+} else if (INTEL_GEN(dev_priv) >= 9) {
> > intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
> > intel_dp->aux_ch_data_reg = skl_aux_data_reg;
> > } else if (HAS_PCH_SPLIT(dev_priv)) {
> >-- 
> >2.26.2
> >
> >___
> >Intel-gfx mailing list
> >Intel-gfx@lists.freedesktop.org
> >https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

2020-10-07 Thread Lucas De Marchi


On Tue, Oct 06, 2020 at 05:33:34PM +0300, Ville Syrjälä wrote:

From: Ville Syrjälä 

Just like with the DDIs tgl+ renamed the AUX CHs to reflect
the type of the DDI. Let's add the aliasing enum values for
the type-C AUX CHs.

Signed-off-by: Ville Syrjälä 
---
drivers/gpu/drm/i915/display/intel_display.h |  8 +++
drivers/gpu/drm/i915/display/intel_dp.c  | 53 ++--
2 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.h 
b/drivers/gpu/drm/i915/display/intel_display.h
index a39be3c9e0cf..cba876721ea0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -290,6 +290,14 @@ enum aux_ch {
AUX_CH_G,
AUX_CH_H,
AUX_CH_I,
+
+   /* tgl+ */
+   AUX_CH_USBC1 = AUX_CH_D,
+   AUX_CH_USBC2,
+   AUX_CH_USBC3,
+   AUX_CH_USBC4,
+   AUX_CH_USBC5,
+   AUX_CH_USBC6,
};

#define aux_ch_name(a) ((a) + 'A')
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
b/drivers/gpu/drm/i915/display/intel_dp.c
index 239016dcd544..a73c354c920e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1792,7 +1792,6 @@ static i915_reg_t skl_aux_ctl_reg(struct intel_dp 
*intel_dp)
case AUX_CH_D:
case AUX_CH_E:
case AUX_CH_F:
-   case AUX_CH_G:
return DP_AUX_CH_CTL(aux_ch);
default:
MISSING_CASE(aux_ch);
@@ -1813,7 +1812,52 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp 
*intel_dp, int index)
case AUX_CH_D:
case AUX_CH_E:
case AUX_CH_F:
-   case AUX_CH_G:
+   return DP_AUX_CH_DATA(aux_ch, index);
+   default:
+   MISSING_CASE(aux_ch);
+   return DP_AUX_CH_DATA(AUX_CH_A, index);
+   }
+}
+
+static i915_reg_t tgl_aux_ctl_reg(struct intel_dp *intel_dp)
+{
+   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+   enum aux_ch aux_ch = dig_port->aux_ch;
+
+   switch (aux_ch) {
+   case AUX_CH_A:
+   case AUX_CH_B:
+   case AUX_CH_C:
+   case AUX_CH_USBC1:
+   case AUX_CH_USBC2:
+   case AUX_CH_USBC3:
+   case AUX_CH_USBC4:
+   case AUX_CH_USBC5:
+   case AUX_CH_USBC6:
+   return DP_AUX_CH_CTL(aux_ch);
+   default:
+   MISSING_CASE(aux_ch);
+   return DP_AUX_CH_CTL(AUX_CH_A);
+   }
+}
+
+static i915_reg_t tgl_aux_data_reg(struct intel_dp *intel_dp, int index)
+{
+   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+   enum aux_ch aux_ch = dig_port->aux_ch;
+
+   switch (aux_ch) {
+   case AUX_CH_A:
+   case AUX_CH_B:
+   case AUX_CH_C:
+   case AUX_CH_USBC1:
+   case AUX_CH_USBC2:
+   case AUX_CH_USBC3:
+   case AUX_CH_USBC4:
+   case AUX_CH_USBC5:
+   case AUX_CH_USBC6:
return DP_AUX_CH_DATA(aux_ch, index);
default:
MISSING_CASE(aux_ch);
@@ -1834,7 +1878,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
struct intel_encoder *encoder = &dig_port->base;

-   if (INTEL_GEN(dev_priv) >= 9) {
+   if (INTEL_GEN(dev_priv) >= 12) {
+   intel_dp->aux_ch_ctl_reg = tgl_aux_ctl_reg;


why is this even a function pointer rather than just the reg? AFAICS it
only depends on dig_port->aux_ch that is initialized in intel_ddi_init()

but could be orthogonal to the change here.


Reviewed-by: Lucas De Marchi 

Lucas De Marchi


+   intel_dp->aux_ch_data_reg = tgl_aux_data_reg;
+   } else if (INTEL_GEN(dev_priv) >= 9) {
intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
intel_dp->aux_ch_data_reg = skl_aux_data_reg;
} else if (HAS_PCH_SPLIT(dev_priv)) {
--
2.26.2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

2020-10-06 Thread Ville Syrjala

From: Ville Syrjälä 

Just like with the DDIs tgl+ renamed the AUX CHs to reflect
the type of the DDI. Let's add the aliasing enum values for
the type-C AUX CHs.

Signed-off-by: Ville Syrjälä 
---
 drivers/gpu/drm/i915/display/intel_display.h |  8 +++
 drivers/gpu/drm/i915/display/intel_dp.c  | 53 ++--
 2 files changed, 58 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_display.h 
b/drivers/gpu/drm/i915/display/intel_display.h
index a39be3c9e0cf..cba876721ea0 100644
--- a/drivers/gpu/drm/i915/display/intel_display.h
+++ b/drivers/gpu/drm/i915/display/intel_display.h
@@ -290,6 +290,14 @@ enum aux_ch {
AUX_CH_G,
AUX_CH_H,
AUX_CH_I,
+
+   /* tgl+ */
+   AUX_CH_USBC1 = AUX_CH_D,
+   AUX_CH_USBC2,
+   AUX_CH_USBC3,
+   AUX_CH_USBC4,
+   AUX_CH_USBC5,
+   AUX_CH_USBC6,
 };
 
 #define aux_ch_name(a) ((a) + 'A')
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c 
b/drivers/gpu/drm/i915/display/intel_dp.c
index 239016dcd544..a73c354c920e 100644
--- a/drivers/gpu/drm/i915/display/intel_dp.c
+++ b/drivers/gpu/drm/i915/display/intel_dp.c
@@ -1792,7 +1792,6 @@ static i915_reg_t skl_aux_ctl_reg(struct intel_dp 
*intel_dp)
case AUX_CH_D:
case AUX_CH_E:
case AUX_CH_F:
-   case AUX_CH_G:
return DP_AUX_CH_CTL(aux_ch);
default:
MISSING_CASE(aux_ch);
@@ -1813,7 +1812,52 @@ static i915_reg_t skl_aux_data_reg(struct intel_dp 
*intel_dp, int index)
case AUX_CH_D:
case AUX_CH_E:
case AUX_CH_F:
-   case AUX_CH_G:
+   return DP_AUX_CH_DATA(aux_ch, index);
+   default:
+   MISSING_CASE(aux_ch);
+   return DP_AUX_CH_DATA(AUX_CH_A, index);
+   }
+}
+
+static i915_reg_t tgl_aux_ctl_reg(struct intel_dp *intel_dp)
+{
+   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+   enum aux_ch aux_ch = dig_port->aux_ch;
+
+   switch (aux_ch) {
+   case AUX_CH_A:
+   case AUX_CH_B:
+   case AUX_CH_C:
+   case AUX_CH_USBC1:
+   case AUX_CH_USBC2:
+   case AUX_CH_USBC3:
+   case AUX_CH_USBC4:
+   case AUX_CH_USBC5:
+   case AUX_CH_USBC6:
+   return DP_AUX_CH_CTL(aux_ch);
+   default:
+   MISSING_CASE(aux_ch);
+   return DP_AUX_CH_CTL(AUX_CH_A);
+   }
+}
+
+static i915_reg_t tgl_aux_data_reg(struct intel_dp *intel_dp, int index)
+{
+   struct drm_i915_private *dev_priv = dp_to_i915(intel_dp);
+   struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+   enum aux_ch aux_ch = dig_port->aux_ch;
+
+   switch (aux_ch) {
+   case AUX_CH_A:
+   case AUX_CH_B:
+   case AUX_CH_C:
+   case AUX_CH_USBC1:
+   case AUX_CH_USBC2:
+   case AUX_CH_USBC3:
+   case AUX_CH_USBC4:
+   case AUX_CH_USBC5:
+   case AUX_CH_USBC6:
return DP_AUX_CH_DATA(aux_ch, index);
default:
MISSING_CASE(aux_ch);
@@ -1834,7 +1878,10 @@ intel_dp_aux_init(struct intel_dp *intel_dp)
struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
struct intel_encoder *encoder = &dig_port->base;
 
-   if (INTEL_GEN(dev_priv) >= 9) {
+   if (INTEL_GEN(dev_priv) >= 12) {
+   intel_dp->aux_ch_ctl_reg = tgl_aux_ctl_reg;
+   intel_dp->aux_ch_data_reg = tgl_aux_data_reg;
+   } else if (INTEL_GEN(dev_priv) >= 9) {
intel_dp->aux_ch_ctl_reg = skl_aux_ctl_reg;
intel_dp->aux_ch_data_reg = skl_aux_data_reg;
} else if (HAS_PCH_SPLIT(dev_priv)) {
-- 
2.26.2

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

Re: [Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

Re: [Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

Re: [Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

Re: [Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

[Intel-gfx] [PATCH 05/20] drm/i915: Introduce AUX_CH_USBCn

6 matches

Site Navigation

Mail list logo

Footer information