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
