airockchip / librga

Apache License 2.0
242 stars 52 forks source link

10bit uncompact mode parameter is not passed to RGA3 driver #45

Open nyanmisaka opened 10 months ago

nyanmisaka commented 10 months ago

My use case requires DRM_FORMAT_P010 instead of DRM_FORMAT_NV15, so I need RGA3 to convert the output of the video decoder.

I found that there is a flaw in the RGA3 kernel driver, which causes the 10bit uncompact/compact mode parameters to not be passed to the RGA3 register.

drivers/video/rockchip/rga3/rga3_reg_info.c

In win0 and win1 inputs:

...
    /* Only on raster mode, yuv 10bit can change to compact or set endian */
    if (msg->win0.rd_mode == RGA_RASTER_MODE && yuv10 == 1) {
        reg =
            ((reg & (~m_RGA3_WIN0_RD_CTRL_SW_WIN0_YUV10B_COMPACT)) |
             (s_RGA3_WIN0_RD_CTRL_SW_WIN0_YUV10B_COMPACT
             (msg->win0.is_10b_compact)));
        reg =
            ((reg & (~m_RGA3_WIN0_RD_CTRL_SW_WIN0_ENDIAN_MODE)) |
             (s_RGA3_WIN0_RD_CTRL_SW_WIN0_ENDIAN_MODE
             (msg->win0.is_10b_endian)));
    }
...
    /* Only on roster mode, yuv 10bit can change to compact or set endian */
    if (msg->win1.rd_mode == RGA_RASTER_MODE && yuv10 == 1) {
        reg =
            ((reg & (~m_RGA3_WIN1_RD_CTRL_SW_WIN1_YUV10B_COMPACT)) |
             (s_RGA3_WIN1_RD_CTRL_SW_WIN1_YUV10B_COMPACT
             (msg->win1.is_10b_compact)));
        reg =
            ((reg & (~m_RGA3_WIN1_RD_CTRL_SW_WIN1_ENDIAN_MODE)) |
             (s_RGA3_WIN1_RD_CTRL_SW_WIN1_ENDIAN_MODE
             (msg->win1.is_10b_endian)));
    }

But in wr output:

    /* Only on roster mode, yuv 10bit can change to compact or set endian */
    if (msg->wr.rd_mode == 0 && yuv10 == 1) {
        reg =
            ((reg & (~m_RGA3_WR_CTRL_SW_WR_YUV10B_COMPACT)) |
             (s_RGA3_WR_CTRL_SW_WR_YUV10B_COMPACT
             (msg->wr.is_10b_compact)));
        reg =
            ((reg & (~m_RGA3_WR_CTRL_SW_WR_ENDIAN_MODE)) |
             (s_RGA3_WR_CTRL_SW_WR_ENDIAN_MODE
             (msg->wr.is_10b_endian)));
    }

The msg->{win0,win1,wr}.rd_mode should be compared with 0 instead of RGA_RASTER_MODE.

I have re-compiled the kernel with those lines changed and verified on my end, it works fine.

Please pass this ticket to the corresponding developer, thanks! cc @Cerf-Yu

nyanmisaka commented 10 months ago

Also this patch is required to pass this parameter from librga to the kernel driver.

normalrga-cpp-add-10b-compact-endian-mode.patch for linux-rga/core/NormalRga.cpp

hbiyik commented 9 months ago

@nyanmisaka thats great news if they can merge this. but i am confused with this uncompact thing.

in rk3588 trm part 2, they show different structures for uncompact structure. pg704 is something weird, which i can not make sense. pg708 is p010 format. if it is so i can just apply this to ffmpeg without any issue. and most players already support p010

pg:704 image

pg:708 image

nyanmisaka commented 9 months ago

@hbiyik

  1. The first diagram is wrong. Just ignore it.
  2. P010 is not supported under specific rd modes.

image

hbiyik commented 9 months ago

thats what i thought, the first picture really does not make sense at all. and thats great, i always use raster mode so this really helps, i can work on this after i experiment with the external buffer mgmt.

nyanmisaka commented 9 months ago

thats what i thought, the first picture really does not make sense at all. and thats great, i always use raster mode so this really helps, i can work on this after i experiment with the external buffer mgmt.

Correct. AFBC is still too early for us to talk about.

hbiyik commented 9 months ago

i can also confirm with the changes to kernel and librga, uncompact also works for me.

two remarks: .is_10b_compact is confusing, i think it should be is_10b_uncompact because P010 is uncompact, NV15 is compact .is_10b_endian is also ambiguous, better wording would be is_10b_little_endian.

nyanmisaka commented 9 months ago

Correct. But given that this is a header file that has already been shipped to many BSP, it is more appropriate to adopt the opposite logic in the kernel and add some comments rather than modify the variable name.