Re: [Intel-gfx] [PATCH v2 7/8] drm/i915: Keep the AKSV details in intel_dp_hdcp_write_an_aksv()

[Ville Syrjälä]

On Fri, Feb 23, 2018 at 08:14:53PM +0530, Ramalingam C wrote:
> 
> 
> On Friday 23 February 2018 07:16 PM, Ville Syrjälä wrote:
> > On Fri, Feb 23, 2018 at 04:40:42PM +0530, Ramalingam C wrote:
> >> This is really making it cleaner.
> >>
> >> Reviewed-by: Ramalingam C 
> >>
> >>
> >>
> >> On Friday 23 February 2018 02:57 AM, Ville Syrjala wrote:
> >>> From: Ville Syrjälä 
> >>>
> >>> Let's try to keep the details on the AKSV stuff concentrated
> >>> in one place. So move the control bit and +5 data size handling
> >>> there.
> >>>
> >>> v2: Increase txbuf[] to include the payload which intel_dp_aux_xfer()
> >>>   will still load into the registers even though the hardware
> >>>   will ignore it
> >>>
> >>> Cc: Sean Paul 
> >>> Cc: Ramalingam C 
> >>> Signed-off-by: Ville Syrjälä 
> >>> ---
> >>>drivers/gpu/drm/i915/intel_dp.c | 42 
> >>> +
> >>>1 file changed, 13 insertions(+), 29 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> >>> b/drivers/gpu/drm/i915/intel_dp.c
> >>> index 217cc6aee477..0d699d230b77 100644
> >>> --- a/drivers/gpu/drm/i915/intel_dp.c
> >>> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >>> @@ -1059,29 +1059,11 @@ static uint32_t skl_get_aux_send_ctl(struct 
> >>> intel_dp *intel_dp,
> >>>  DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> >>>}
> >>>
> >>> -static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> >>> -   bool has_aux_irq,
> >>> -   int send_bytes,
> >>> -   uint32_t aux_clock_divider,
> >>> -   bool aksv_write)
> >>> -{
> >>> - uint32_t val = 0;
> >>> -
> >>> - if (aksv_write) {
> >>> - send_bytes += 5;
> >>> - val |= DP_AUX_CH_CTL_AUX_AKSV_SELECT;
> >>> - }
> >>> -
> >>> - return val | intel_dp->get_aux_send_ctl(intel_dp,
> >>> - has_aux_irq,
> >>> - send_bytes,
> >>> - aux_clock_divider);
> >>> -}
> >>> -
> >>>static int
> >>>intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >>> const uint8_t *send, int send_bytes,
> >>> -   uint8_t *recv, int recv_size, bool aksv_write)
> >>> +   uint8_t *recv, int recv_size,
> >>> +   u32 aux_send_ctl_flags)
> >>>{
> >>>   struct intel_digital_port *intel_dig_port = 
> >>> dp_to_dig_port(intel_dp);
> >>>   struct drm_i915_private *dev_priv =
> >>> @@ -1145,11 +1127,12 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >>>   }
> >>>
> >>>   while ((aux_clock_divider = 
> >>> intel_dp->get_aux_clock_divider(intel_dp, clock++))) {
> >>> - u32 send_ctl = intel_dp_get_aux_send_ctl(intel_dp,
> >>> -  has_aux_irq,
> >>> -  send_bytes,
> >>> -  aux_clock_divider,
> >>> -  aksv_write);
> >>> + u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> >>> +   has_aux_irq,
> >>> +   send_bytes,
> >>> +   aux_clock_divider);
> >>> +
> >>> + send_ctl |= aux_send_ctl_flags;
> >>>
> >>>   /* Must try at least 3 times according to DP spec */
> >>>   for (try = 0; try < 5; try++) {
> >>> @@ -1287,7 +1270,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, 
> >>> struct drm_dp_aux_msg *msg)
> >>>   memcpy(txbuf + HEADER_SIZE, msg->buffer, 
> >>> msg->size);
> >>>
> >>>   ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,
> >>> - rxbuf, rxsize, false);
> >>> + rxbuf, rxsize, 0);
> >>>   if (ret > 0) {
> >>>   msg->reply = rxbuf[0] >> 4;
> >>>
> >>> @@ -1310,7 +1293,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, 
> >>> struct drm_dp_aux_msg *msg)
> >>>   return -E2BIG;
> >>>
> >>>   ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,
> >>> - rxbuf, rxsize, false);
> >>> + rxbuf, rxsize, 0);
> >>>   if (ret > 0) {
> >>>   msg->reply = rxbuf[0] >> 4;
> >>>   /*
> >>> @@ -5085,7 +5068,7 @@ int intel_dp_hdcp_write_an_aksv(struct 
> >>> intel_digital_port *intel_dig_port,
> >>>   u8 *an)
> >>>{
> >>>   struct intel_dp *intel_dp = 
> >>> 

Re: [Intel-gfx] [PATCH v2 7/8] drm/i915: Keep the AKSV details in intel_dp_hdcp_write_an_aksv()

[Ramalingam C]

On Friday 23 February 2018 07:16 PM, Ville Syrjälä wrote:

On Fri, Feb 23, 2018 at 04:40:42PM +0530, Ramalingam C wrote:

This is really making it cleaner.

Reviewed-by: Ramalingam C 



On Friday 23 February 2018 02:57 AM, Ville Syrjala wrote:

From: Ville Syrjälä 

Let's try to keep the details on the AKSV stuff concentrated
in one place. So move the control bit and +5 data size handling
there.

v2: Increase txbuf[] to include the payload which intel_dp_aux_xfer()
  will still load into the registers even though the hardware
  will ignore it

Cc: Sean Paul 
Cc: Ramalingam C 
Signed-off-by: Ville Syrjälä 
---
   drivers/gpu/drm/i915/intel_dp.c | 42 
+
   1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 217cc6aee477..0d699d230b77 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1059,29 +1059,11 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp 
*intel_dp,
   DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
   }
   
-static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,

- bool has_aux_irq,
- int send_bytes,
- uint32_t aux_clock_divider,
- bool aksv_write)
-{
-   uint32_t val = 0;
-
-   if (aksv_write) {
-   send_bytes += 5;
-   val |= DP_AUX_CH_CTL_AUX_AKSV_SELECT;
-   }
-
-   return val | intel_dp->get_aux_send_ctl(intel_dp,
-   has_aux_irq,
-   send_bytes,
-   aux_clock_divider);
-}
-
   static int
   intel_dp_aux_xfer(struct intel_dp *intel_dp,
  const uint8_t *send, int send_bytes,
- uint8_t *recv, int recv_size, bool aksv_write)
+ uint8_t *recv, int recv_size,
+ u32 aux_send_ctl_flags)
   {
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
struct drm_i915_private *dev_priv =
@@ -1145,11 +1127,12 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
}
   
   	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {

-   u32 send_ctl = intel_dp_get_aux_send_ctl(intel_dp,
-has_aux_irq,
-send_bytes,
-aux_clock_divider,
-aksv_write);
+   u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
+ has_aux_irq,
+ send_bytes,
+ aux_clock_divider);
+
+   send_ctl |= aux_send_ctl_flags;
   
   		/* Must try at least 3 times according to DP spec */

for (try = 0; try < 5; try++) {
@@ -1287,7 +1270,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct 
drm_dp_aux_msg *msg)
memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
   
   		ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,

-   rxbuf, rxsize, false);
+   rxbuf, rxsize, 0);
if (ret > 0) {
msg->reply = rxbuf[0] >> 4;
   
@@ -1310,7 +1293,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)

return -E2BIG;
   
   		ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,

-   rxbuf, rxsize, false);
+   rxbuf, rxsize, 0);
if (ret > 0) {
msg->reply = rxbuf[0] >> 4;
/*
@@ -5085,7 +5068,7 @@ int intel_dp_hdcp_write_an_aksv(struct intel_digital_port 
*intel_dig_port,
u8 *an)
   {
struct intel_dp *intel_dp = enc_to_intel_dp(_dig_port->base.base);
-   uint8_t txbuf[4], rxbuf[2], reply = 0;
+   uint8_t txbuf[4+5] = {}, rxbuf[2], reply = 0;

You might want to use the macros for size of txbuf as  HEADER_SIZE +
DRM_HDCP_KSV_LEN, as it is done in the next patch.

As the original code was using a bare 5 I figured I'll keep using it here
as well to make it easier to see what's moving where.
Suggested the above, to make the 8th patch just for pulling out the aux 
header population.

I am happy with the current shape too :)

--Ram



--Ram

ssize_t dpcd_ret;
int ret;
   
@@ -5110,7 +5093,8 @@ int 

Re: [Intel-gfx] [PATCH v2 7/8] drm/i915: Keep the AKSV details in intel_dp_hdcp_write_an_aksv()

[Ville Syrjälä]

On Fri, Feb 23, 2018 at 04:40:42PM +0530, Ramalingam C wrote:
> This is really making it cleaner.
> 
> Reviewed-by: Ramalingam C 
> 
> 
> 
> On Friday 23 February 2018 02:57 AM, Ville Syrjala wrote:
> > From: Ville Syrjälä 
> >
> > Let's try to keep the details on the AKSV stuff concentrated
> > in one place. So move the control bit and +5 data size handling
> > there.
> >
> > v2: Increase txbuf[] to include the payload which intel_dp_aux_xfer()
> >  will still load into the registers even though the hardware
> >  will ignore it
> >
> > Cc: Sean Paul 
> > Cc: Ramalingam C 
> > Signed-off-by: Ville Syrjälä 
> > ---
> >   drivers/gpu/drm/i915/intel_dp.c | 42 
> > +
> >   1 file changed, 13 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c 
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index 217cc6aee477..0d699d230b77 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1059,29 +1059,11 @@ static uint32_t skl_get_aux_send_ctl(struct 
> > intel_dp *intel_dp,
> >DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
> >   }
> >   
> > -static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,
> > - bool has_aux_irq,
> > - int send_bytes,
> > - uint32_t aux_clock_divider,
> > - bool aksv_write)
> > -{
> > -   uint32_t val = 0;
> > -
> > -   if (aksv_write) {
> > -   send_bytes += 5;
> > -   val |= DP_AUX_CH_CTL_AUX_AKSV_SELECT;
> > -   }
> > -
> > -   return val | intel_dp->get_aux_send_ctl(intel_dp,
> > -   has_aux_irq,
> > -   send_bytes,
> > -   aux_clock_divider);
> > -}
> > -
> >   static int
> >   intel_dp_aux_xfer(struct intel_dp *intel_dp,
> >   const uint8_t *send, int send_bytes,
> > - uint8_t *recv, int recv_size, bool aksv_write)
> > + uint8_t *recv, int recv_size,
> > + u32 aux_send_ctl_flags)
> >   {
> > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> > struct drm_i915_private *dev_priv =
> > @@ -1145,11 +1127,12 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
> > }
> >   
> > while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, 
> > clock++))) {
> > -   u32 send_ctl = intel_dp_get_aux_send_ctl(intel_dp,
> > -has_aux_irq,
> > -send_bytes,
> > -aux_clock_divider,
> > -aksv_write);
> > +   u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
> > + has_aux_irq,
> > + send_bytes,
> > + aux_clock_divider);
> > +
> > +   send_ctl |= aux_send_ctl_flags;
> >   
> > /* Must try at least 3 times according to DP spec */
> > for (try = 0; try < 5; try++) {
> > @@ -1287,7 +1270,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct 
> > drm_dp_aux_msg *msg)
> > memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
> >   
> > ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,
> > -   rxbuf, rxsize, false);
> > +   rxbuf, rxsize, 0);
> > if (ret > 0) {
> > msg->reply = rxbuf[0] >> 4;
> >   
> > @@ -1310,7 +1293,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct 
> > drm_dp_aux_msg *msg)
> > return -E2BIG;
> >   
> > ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,
> > -   rxbuf, rxsize, false);
> > +   rxbuf, rxsize, 0);
> > if (ret > 0) {
> > msg->reply = rxbuf[0] >> 4;
> > /*
> > @@ -5085,7 +5068,7 @@ int intel_dp_hdcp_write_an_aksv(struct 
> > intel_digital_port *intel_dig_port,
> > u8 *an)
> >   {
> > struct intel_dp *intel_dp = enc_to_intel_dp(_dig_port->base.base);
> > -   uint8_t txbuf[4], rxbuf[2], reply = 0;
> > +   uint8_t txbuf[4+5] = {}, rxbuf[2], reply = 0;
> You might want to use the macros for size of txbuf as  HEADER_SIZE + 
> DRM_HDCP_KSV_LEN, as it is done in the next patch.

As the original code was using a bare 5 I figured I'll keep using it here
as well to make it easier to see what's moving where.

> --Ram
> > ssize_t dpcd_ret;
> > int 

Re: [Intel-gfx] [PATCH v2 7/8] drm/i915: Keep the AKSV details in intel_dp_hdcp_write_an_aksv()

[Ramalingam C]

This is really making it cleaner.

Reviewed-by: Ramalingam C 



On Friday 23 February 2018 02:57 AM, Ville Syrjala wrote:

From: Ville Syrjälä 

Let's try to keep the details on the AKSV stuff concentrated
in one place. So move the control bit and +5 data size handling
there.

v2: Increase txbuf[] to include the payload which intel_dp_aux_xfer()
 will still load into the registers even though the hardware
 will ignore it

Cc: Sean Paul 
Cc: Ramalingam C 
Signed-off-by: Ville Syrjälä 
---
  drivers/gpu/drm/i915/intel_dp.c | 42 +
  1 file changed, 13 insertions(+), 29 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 217cc6aee477..0d699d230b77 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1059,29 +1059,11 @@ static uint32_t skl_get_aux_send_ctl(struct intel_dp 
*intel_dp,
   DP_AUX_CH_CTL_SYNC_PULSE_SKL(32);
  }
  
-static uint32_t intel_dp_get_aux_send_ctl(struct intel_dp *intel_dp,

- bool has_aux_irq,
- int send_bytes,
- uint32_t aux_clock_divider,
- bool aksv_write)
-{
-   uint32_t val = 0;
-
-   if (aksv_write) {
-   send_bytes += 5;
-   val |= DP_AUX_CH_CTL_AUX_AKSV_SELECT;
-   }
-
-   return val | intel_dp->get_aux_send_ctl(intel_dp,
-   has_aux_irq,
-   send_bytes,
-   aux_clock_divider);
-}
-
  static int
  intel_dp_aux_xfer(struct intel_dp *intel_dp,
  const uint8_t *send, int send_bytes,
- uint8_t *recv, int recv_size, bool aksv_write)
+ uint8_t *recv, int recv_size,
+ u32 aux_send_ctl_flags)
  {
struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
struct drm_i915_private *dev_priv =
@@ -1145,11 +1127,12 @@ intel_dp_aux_xfer(struct intel_dp *intel_dp,
}
  
  	while ((aux_clock_divider = intel_dp->get_aux_clock_divider(intel_dp, clock++))) {

-   u32 send_ctl = intel_dp_get_aux_send_ctl(intel_dp,
-has_aux_irq,
-send_bytes,
-aux_clock_divider,
-aksv_write);
+   u32 send_ctl = intel_dp->get_aux_send_ctl(intel_dp,
+ has_aux_irq,
+ send_bytes,
+ aux_clock_divider);
+
+   send_ctl |= aux_send_ctl_flags;
  
  		/* Must try at least 3 times according to DP spec */

for (try = 0; try < 5; try++) {
@@ -1287,7 +1270,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct 
drm_dp_aux_msg *msg)
memcpy(txbuf + HEADER_SIZE, msg->buffer, msg->size);
  
  		ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,

-   rxbuf, rxsize, false);
+   rxbuf, rxsize, 0);
if (ret > 0) {
msg->reply = rxbuf[0] >> 4;
  
@@ -1310,7 +1293,7 @@ intel_dp_aux_transfer(struct drm_dp_aux *aux, struct drm_dp_aux_msg *msg)

return -E2BIG;
  
  		ret = intel_dp_aux_xfer(intel_dp, txbuf, txsize,

-   rxbuf, rxsize, false);
+   rxbuf, rxsize, 0);
if (ret > 0) {
msg->reply = rxbuf[0] >> 4;
/*
@@ -5085,7 +5068,7 @@ int intel_dp_hdcp_write_an_aksv(struct intel_digital_port 
*intel_dig_port,
u8 *an)
  {
struct intel_dp *intel_dp = enc_to_intel_dp(_dig_port->base.base);
-   uint8_t txbuf[4], rxbuf[2], reply = 0;
+   uint8_t txbuf[4+5] = {}, rxbuf[2], reply = 0;
You might want to use the macros for size of txbuf as  HEADER_SIZE + 
DRM_HDCP_KSV_LEN, as it is done in the next patch.

--Ram

ssize_t dpcd_ret;
int ret;
  
@@ -5110,7 +5093,8 @@ int intel_dp_hdcp_write_an_aksv(struct intel_digital_port *intel_dig_port,

txbuf[3] = DRM_HDCP_KSV_LEN - 1;
  
  	ret = intel_dp_aux_xfer(intel_dp, txbuf, sizeof(txbuf),

-   rxbuf, sizeof(rxbuf), true);
+   rxbuf, sizeof(rxbuf),
+   DP_AUX_CH_CTL_AUX_AKSV_SELECT);
if (ret < 0) {
DRM_ERROR("Write Aksv