Re: [PATCH 4/5] drm/i915: Write AVI infoframes for Parade LSPCON

[David Weinehall <david.weinehall@linux.intel.com>]

On Wed, Dec 20, 2017 at 10:08:53AM +0530, Sharma, Shashank wrote:
> Thanks for the review, David.
> 
> My comments, inline.
> 
> 
> Regards
> 
> Shashank
> 
> 
> On 12/19/2017 3:43 PM, David Weinehall wrote:
> > On Mon, Dec 18, 2017 at 08:15:30PM +0100, Maarten Lankhorst wrote:
> > > Op 14-11-17 om 16:17 schreef Shashank Sharma:
> > > > Different LSPCON vendors specify their custom methods to pass
> > > > AVI infoframes to the LSPCON chip, so does Parade tech.
> > > > 
> > > > This patch adds functions to arrange and write AVI infoframes
> > > > into Parade LSPCON chips.
> > > > 
> > > > Signed-off-by: Shashank Sharma <shashank.sharma@intel.com>
> > > > ---
> > > >   drivers/gpu/drm/i915/intel_lspcon.c | 119 +++++++++++++++++++++++++++++++++++-
> > > >   1 file changed, 118 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/gpu/drm/i915/intel_lspcon.c b/drivers/gpu/drm/i915/intel_lspcon.c
> > > > index 1ac4c47..77f0687 100644
> > > > --- a/drivers/gpu/drm/i915/intel_lspcon.c
> > > > +++ b/drivers/gpu/drm/i915/intel_lspcon.c
> > > > @@ -37,6 +37,12 @@
> > > >   #define  LSPCON_MCA_AVI_IF_KICKOFF (1 << 0)
> > > >   #define  LSPCON_MCA_AVI_IF_HANDLED (1 << 1)
> > > > +/* AUX addresses to write Parade AVI IF */
> > > > +#define LSPCON_PARADE_AVI_IF_WRITE_OFFSET 0x516
> > > > +#define LSPCON_PARADE_AVI_IF_CTRL 0x51E
> > > > +#define  LSPCON_PARADE_AVI_IF_KICKOFF (1 << 7)
> > > > +#define LSPCON_PARADE_AVI_IF_DATA_SIZE 32
> > > > +
> > > >   static struct intel_dp *lspcon_to_intel_dp(struct intel_lspcon *lspcon)
> > > >   {
> > > >   	struct intel_digital_port *dig_port =
> > > > @@ -241,6 +247,113 @@ static void lspcon_resume_in_pcon_wa(struct intel_lspcon *lspcon)
> > > >   	DRM_DEBUG_KMS("LSPCON DP descriptor mismatch after resume\n");
> > > >   }
> > > > +static bool lspcon_parade_fw_ready(struct drm_dp_aux *aux)
> > > > +{
> > > > +	u8 avi_if_ctrl;
> > > > +	u8 retry;
> > > > +	ssize_t ret;
> > > > +
> > > > +	/* Check if LSPCON FW is ready for data */
> > > > +	for (retry = 0; retry < 5; retry++) {
> > > > +
> > Remove this newline.
> Why ? this is not accidental.

Because accidental or not you're not allowed to impose personal coding
style on the driver. Follow the example of the rest of the driver;
we don't insert leading blanklines inside for-blocks (or other blocks,
for that matter).

> > > > +		if (retry)
> > > > +			usleep_range(200, 300);
> > > > +
> > > > +		ret = drm_dp_dpcd_read(aux, LSPCON_PARADE_AVI_IF_CTRL,
> > > > +				       &avi_if_ctrl, 1);
> > > > +		if (ret < 0) {
> > > > +			DRM_ERROR("Failed to read AVI IF control\n");
> > > > +			return false;
> > > > +		}
> > > > +
> > > > +		if ((avi_if_ctrl & LSPCON_PARADE_AVI_IF_KICKOFF) == 0)
> > > > +			return true;
> > > > +	}
> > > > +
> > > > +	DRM_ERROR("Parade FW not ready to accept AVI IF\n");
> > > > +	return false;
> > > > +}
> > > > +
> > > > +static bool _lspcon_parade_write_infoframe_blocks(struct drm_dp_aux *aux,
> > > > +					       uint8_t *avi_buf)
> > > > +{
> > > > +	u8 avi_if_ctrl;
> > > > +	u8 block_count = 0;
> > > > +	u8 *data;
> > > > +	uint16_t reg;
> > > > +	ssize_t ret;
> > > > +
> > > > +	while (block_count < 4) {
> > > > +
> > And this one.
> Same as above.
> - Shashank
> > 
> > > > +		if (!lspcon_parade_fw_ready(aux)) {
> > > > +			DRM_DEBUG_KMS("LSPCON FW not ready, block %d\n",
> > > > +				       block_count);
> > > > +			return false;
> > > > +		}
> > > > +
> > > > +		reg = LSPCON_PARADE_AVI_IF_WRITE_OFFSET;
> > > > +		data = avi_buf + block_count * 8;
> > > > +		ret = drm_dp_dpcd_write(aux, reg, data, 8);
> > > > +		if (ret < 0) {
> > > > +			DRM_ERROR("Failed to write AVI IF block %d\n",
> > > > +				   block_count);
> > > > +			return false;
> > > > +		}
> > > > +
> > > > +		/*
> > > > +		 * Once a block of data is written, we have to inform the FW
> > > > +		 * about this by writing into avi infoframe control register:
> > > > +		 * - set the kickoff bit[7] to 1
> > > > +		 * - write the block no. to bits[1:0]
> > > > +		 */
> > > > +		reg = LSPCON_PARADE_AVI_IF_CTRL;
> > > > +		avi_if_ctrl = LSPCON_PARADE_AVI_IF_KICKOFF | block_count;
> > > > +		ret = drm_dp_dpcd_write(aux, reg, &avi_if_ctrl, 1);
> > > > +		if (ret < 0) {
> > > > +			DRM_ERROR("Failed to update (0x%x), block %d\n",
> > > > +					reg, block_count);
> > > > +			return false;
> > > > +		}
> > > > +
> > > > +		block_count++;
> > > > +	}
> > > > +
> > > > +	DRM_DEBUG_KMS("Wrote AVI IF blocks successfully\n");
> > > > +	return true;
> > > > +}
> > > > +
> > > > +static bool _lspcon_write_avi_infoframe_parade(struct drm_dp_aux *aux,
> > > > +					       const uint8_t *frame,
> > > > +					       ssize_t len)
> > > > +{
> > > > +	uint8_t avi_if[LSPCON_PARADE_AVI_IF_DATA_SIZE] = {1, };
> > > > +
> > > > +	/*
> > > > +	 * Parade's frames contains 32 bytes of data, divided
> > > > +	 * into 4 frames:
> > > > +	 *	Token byte (first byte of first frame, must be non-zero)
> > > > +	 *	HB0 to HB2	 from AVI IF (3 bytes header)
> > > > +	 *	PB0 to PB27 from AVI IF (28 bytes data)
> > > > +	 * So it should look like this
> > > > +	 *	first block: | <token> <HB0-HB2> <DB0-DB3> |
> > > > +	 *	next 3 blocks: |<DB4-DB11>|<DB12-DB19>|<DB20-DB28>|
> > > > +	 */
> > > > +
> > > > +	if (len > LSPCON_PARADE_AVI_IF_DATA_SIZE - 1) {
> > > > +		DRM_ERROR("Invalid length of infoframes\n");
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	memcpy(&avi_if[1], frame, len);
> > > > +
> > > > +	if (!_lspcon_parade_write_infoframe_blocks(aux, avi_if)) {
> > > > +		DRM_DEBUG_KMS("Failed to write infoframe blocks\n");
> > > > +		return false;
> > > > +	}
> > > > +
> > > > +	return true;
> > > > +}
> > > > +
> > > >   static bool _lspcon_write_avi_infoframe_mca(struct drm_dp_aux *aux,
> > > >   					     const uint8_t *buffer, ssize_t len)
> > > >   {
> > > > @@ -295,7 +408,7 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
> > > >   			     enum hdmi_infoframe_type type,
> > > >   			     const void *frame, ssize_t len)
> > > >   {
> > > > -	bool ret = true;
> > > > +	bool ret;
> > > >   	struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
> > > >   	struct intel_lspcon *lspcon = enc_to_intel_lspcon(encoder);
> > > > @@ -306,6 +419,10 @@ void lspcon_write_infoframe(struct drm_encoder *encoder,
> > > >   	if (lspcon->vendor == LSPCON_VENDOR_MCA)
> > > >   		ret = _lspcon_write_avi_infoframe_mca(&intel_dp->aux,
> > > >   						      frame, len);
> > > > +	else
> > > > +		ret = _lspcon_write_avi_infoframe_parade(&intel_dp->aux,
> > > > +							 frame, len);
> > > If you make it a switch (lspcon->vendor) and don't add a default case, you will get a compiler warning when you add a new vendor to the enumeration.
> > > 
> > > With that changed for first 4 patches:
> > > 
> > > Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > > _______________________________________________
> > > 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 mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx