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

Ramalingam C
2018-02-23T14:50:48Z

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 <ramalingam.c@intel.com>
>>
>>
>>
>> On Friday 23 February 2018 02:57 AM, Ville Syrjala wrote:
>>> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>>
>>> 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 <seanpaul@chromium.org>
>>> Cc: Ramalingam C <ramalingam.c@intel.com>
>>> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>>> ---
>>> 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(&intel_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 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 over DP/AUX failed (%d)\n", ret);
>>> return ret;