Re: [PATCH v2 1/6] staging: media: wave5: Add vpuapi layer

[Dan Carpenter]

On Tue, Nov 02, 2021 at 11:47:24AM +0100, Dafna Hirschfeld wrote:
> > > +int wave5_vpu_decode(struct vpu_instance *vpu_inst, struct dec_param *option, u32 *fail_res)
> > > +{
> > > +	u32 mode_option = DEC_PIC_NORMAL, bs_option, reg_val;
> > > +	s32 force_latency = -1;
> > 
> > Ugh....  Write this as:
> > 
> > 	bool force_latency = false;
> > 
> > 
> > > +	struct dec_info *p_dec_info = &vpu_inst->codec_info->dec_info;
> > > +	struct dec_open_param *p_open_param = &p_dec_info->open_param;
> > > +	int ret;
> > > +
> > > +	if (p_dec_info->thumbnail_mode) {
> > > +		mode_option = DEC_PIC_W_THUMBNAIL;
> > > +	} else if (option->skipframe_mode) {
> > > +		switch (option->skipframe_mode) {
> > > +		case WAVE_SKIPMODE_NON_IRAP:
> > > +			mode_option = SKIP_NON_IRAP;
> > > +			force_latency = 0;
> > 
> > 	force_latency = true;
> > 
> > > +			break;
> > > +		case WAVE_SKIPMODE_NON_REF:
> > > +			mode_option = SKIP_NON_REF_PIC;
> > > +			break;
> > > +		default:
> > > +			// skip off
> > > +			break;
> > > +		}
> > > +	}
> > > +
> > > +	// set disable reorder
> > > +	if (!p_dec_info->reorder_enable)
> > > +		force_latency = 0;
> > 
> > 	force_latency = true;
> > 
> > > +
> > > +	/* set attributes of bitstream buffer controller */
> > > +	bs_option = 0;
> > > +	reg_val = 0;
> > 
> > This assign is never used.
> > 
> > > +	switch (p_open_param->bitstream_mode) {
> > > +	case BS_MODE_INTERRUPT:
> > > +		bs_option = 0;
> > 
> > Already set above.
> > 
> > > +		break;
> > > +	case BS_MODE_PIC_END:
> > > +		bs_option = BSOPTION_ENABLE_EXPLICIT_END;
> > > +		break;
> > > +	default:
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	vpu_write_reg(vpu_inst->dev, W5_BS_RD_PTR, p_dec_info->stream_rd_ptr);
> > > +	vpu_write_reg(vpu_inst->dev, W5_BS_WR_PTR, p_dec_info->stream_wr_ptr);
> > > +	if (p_dec_info->stream_endflag == 1)
> > > +		bs_option = 3; // (stream_end_flag<<1) | EXPLICIT_END
> > > +	if (p_open_param->bitstream_mode == BS_MODE_PIC_END)
> > > +		bs_option |= (1UL << 31);
> > > +	if (vpu_inst->std == W_AV1_DEC)
> > > +		bs_option |= ((p_open_param->av1_format) << 2);
> > > +	vpu_write_reg(vpu_inst->dev, W5_BS_OPTION, bs_option);
> > > +
> > > +	/* secondary AXI */
> > > +	reg_val = (p_dec_info->sec_axi_info.wave.use_bit_enable << 0) |
> > > +		(p_dec_info->sec_axi_info.wave.use_ip_enable << 9) |
> > > +		(p_dec_info->sec_axi_info.wave.use_lf_row_enable << 15);
> > > +	vpu_write_reg(vpu_inst->dev, W5_USE_SEC_AXI, reg_val);
> > > +
> > > +	/* set attributes of user buffer */
> > > +	vpu_write_reg(vpu_inst->dev, W5_CMD_DEC_USER_MASK, p_dec_info->user_data_enable);
> > > +
> > > +	vpu_write_reg(vpu_inst->dev, W5_COMMAND_OPTION,
> > > +		      ((option->disable_film_grain << 6) | (option->cra_as_bla_flag << 5) |
> > > +		 mode_option));
> > 
> > These are badly aligned and contribute to the Wall of Text Effect that
> > this code has.  :(
> > 
> > 	vpu_write_reg(vpu_inst->dev, W5_COMMAND_OPTION,
> > 		      (option->disable_film_grain << 6) |
> > 		      (option->cra_as_bla_flag << 5) |
> > 		      mode_option);
> > 
> > 
> > 
> > > +	vpu_write_reg(vpu_inst->dev, W5_CMD_DEC_TEMPORAL_ID_PLUS1,
> > > +		      ((p_dec_info->target_spatial_id + 1) << 9) |
> > > +		 (p_dec_info->temp_id_select_mode << 8) | (p_dec_info->target_temp_id + 1));
> > 
> > 	vpu_write_reg(vpu_inst->dev, W5_CMD_DEC_TEMPORAL_ID_PLUS1,
> > 		      ((p_dec_info->target_spatial_id + 1) << 9) |
> > 		      (p_dec_info->temp_id_select_mode << 8) |
> > 		      (p_dec_info->target_temp_id + 1));
> > 
> > Why do we have to add "+ 1" to p_dec_info->target_spatial_id?
> 
> for some reason the code defines 'DECODE_ALL_SPATIAL_LAYERS' to -1 and then adding '+ 1' set it to 0
> no idea why is it implemented like that.
> 
> > 
> > 
> > > +	vpu_write_reg(vpu_inst->dev, W5_CMD_SEQ_CHANGE_ENABLE_FLAG, p_dec_info->seq_change_mask);
> > > +	vpu_write_reg(vpu_inst->dev, W5_CMD_DEC_FORCE_FB_LATENCY_PLUS1, force_latency + 1);
> > 
> > 
> > 	vpu_write_reg(vpu_inst->dev, W5_CMD_DEC_FORCE_FB_LATENCY_PLUS1, force_latency);
> 
> is it nice to write bool to a reigeter?, isn't it better to set 'force_latency' to u32?
> 

It's fine to write a bool to register or to make it a u32.  But the
point is this code is obfuscated where -1 is zero/false and 0 represents
1/true.

regards,
dan carpenter