Re: [RFC 1/4] media: meson: add v4l2 m2m video decoder driver

[Maxime Jourdan <maxi.jourdan@wanadoo.fr>]

2018-08-02 12:30 GMT+02:00 Jerome Brunet <jbrunet@baylibre.com>:
>
> Ouch!
>
> Your architecture seems pretty modular. Maybe you could split this patch a
> little ? One patch of the 'backbone' then one for each codec ?

Hehe, it's a big baby. Sure I'll split it codec by codec.

>
> I suppose this will go away with :
> https://lkml.kernel.org/r/20180801185128.23440-1-maxi.jourdan@wanadoo.fr
>
> ? If yes, it would have been better to send a version using it directly.

Indeed they will, my bad.

>> +     while (readl_relaxed(core->dos_base + DCAC_DMA_CTRL) & 0x8000) { }
>> +     while (readl_relaxed(core->dos_base + LMEM_DMA_CTRL) & 0x8000) { }
>
> Is this guarantee to exit at some point or is there a risk of staying stuck here
> forever ?
>
> I think regmap has some helper function for polling with a timeout.

Totally forgot about those since they caused no problem so far, good catch.
I'll see with regmap.

>> +
>> +     writel_relaxed((1<<7) | (1<<6) | (1<<4), core->dos_base + DOS_SW_RESET0);
>> +     writel_relaxed(0, core->dos_base + DOS_SW_RESET0);
>> +     readl_relaxed(core->dos_base + DOS_SW_RESET0);
>> +
>> +     writel_relaxed((1<<7) | (1<<6) | (1<<4), core->dos_base + DOS_SW_RESET0);
>> +     writel_relaxed(0, core->dos_base + DOS_SW_RESET0);
>> +     writel_relaxed((1<<9) | (1<<8), core->dos_base + DOS_SW_RESET0);
>> +     writel_relaxed(0, core->dos_base + DOS_SW_RESET0);
>> +     readl_relaxed(core->dos_base + DOS_SW_RESET0);
>> +
>> +     writel_relaxed(readl_relaxed(core->dos_base + POWER_CTL_VLD) | (1 << 9) | (1 << 6), core->dos_base + POWER_CTL_VLD);
>> +
>> +     writel_relaxed(0, core->dos_base + PSCALE_CTRL);
>> +     writel_relaxed(0, core->dos_base + AV_SCRATCH_0);
>> +
>> +     workspace_offset = h264->workspace_paddr - DEF_BUF_START_ADDR;
>> +     writel_relaxed(workspace_offset, core->dos_base + AV_SCRATCH_1);
>> +     writel_relaxed(h264->ext_fw_paddr, core->dos_base + AV_SCRATCH_G);
>> +     writel_relaxed(h264->sei_paddr - workspace_offset, core->dos_base + AV_SCRATCH_I);
>
> Do we have any idea what all the above does ? I suppose, it mostly comes from
> reverse engineering the vendor kernel. If possible, a few comments would be
> nice.
>
> In general, instead of (1 << x), you could write BIT(x)
>

The resets and power_ctl, not really. The last 4 lines set the phy
addr for the "workspace" which is a memory region where the decoder
keeps some stuff, unfortunately I don't know what,
the extended firmware data and the SEI dumps.

I'll try to add some comments and see if I can find more info in the aml kernel.

I'll also change all the (1 << x) to BIT(x).


>
> What the about defining the filter shift and width, using GENMASK() maybe ?
>
>> +     mb_total = (parsed_info >> 8) & 0xffff;
>> +
>> +     /* Size of Motion Vector per macroblock ? */
>> +     mb_mv_byte = (parsed_info & 0x80000000) ? 24 : 96;
>
> #define FOO_MB_SIZE BIT(28)
> (parsed_info & FOO_MB_SIZE) ?

>> +     writel_relaxed((max_reference_size << 24) | (actual_dpb_size << 16) | (max_dpb_size << 8), core->dos_base + AV_SCRATCH_0);
>
> I know it is not always easy, especially with very little documentation, but
> could you #define a bit more the content of the registers:
>
> something like
> #define BAR_MAX_REF_SHIFT 24
>
> You get the idea ..

>> +     cmd = status & 0xff;
>> +
>> +     if (cmd == 1) {
>
> Same kind of comment, could define those command somewhere to this a bit more
> readable ?
>

>> +     for (i = 0; i < 16; i++) {
>> +             u16 cur_rps = params->p.CUR_RPS[i];
>> +             int delt = cur_rps & ((1 << (RPS_USED_BIT - 1)) - 1);
>
> Looks like using GENMASK would make things a bit more readable here.
>
>> +
>> +             if (cur_rps & 0x8000)
>
> BIT(15) ? any idea what this is ? could we define this ?
>
> Same comment in general for the rest of the patch. If you can replace mask and
> bit calculation with related macro and define things a bit more, it would be
> nice.

>> +     for (i = 0; i < num_ref_idx_l0_active; i++) {
>> +             int cIdx;
>
> Could we avoid cAmEl case ?
>

>> +             //writel_relaxed(buf_uv_paddr >> 5, core->dos_base + HEVCD_MPP_ANC2AXI_TBL_DATA);
>
> Clean up ?
>

Thanks for all the advice above.

>> +#define DOS_SW_RESET0                0xfc00
>
> I think this is not the first time you've defined this.
> Maybe this (and other re-used register offsets) needs to be in some header ?

Yes at present each file has their set of registers declared within
the .c but there are some redundancies.
I'll cater them in a header.


>> +#define HEVC_ASSIST_AFIFO_CTRL (0x3001 * 4)
>
> Defining register this way is something amlogic does in their.
> We they submitted the AXG clock controller we kindly asked them
> to directly put what the offset is and drop the calculation.
>
> Could please do the same ?

Sure

>> +     core->dos_parser_clk = devm_clk_get(dev, "dos_parser");
>> +     if (IS_ERR(core->dos_parser_clk)) {
>
> You should handle EPROBE_DEFER and not print an error in such case.
> It is fairly common for the device probe to be deferred because of a clock
> provider.
>

>> +MODULE_ALIAS("platform:meson-video-decoder");
>
> On the target platform, the device should always be instantiated from DT
> With the MODULE_DEVICE_TABLE above, I don't think you need this MODULE_ALIAS.
>

Thanks!