Add HME support for ICLLP AVC VDEnc #463

cchunbo · 2018-12-13T13:56:09Z

The change will include Downscaling and HME source code and enable the usage for AVC VDEnc on ICLLP

media_driver/agnostic/common/codec/hal/codechal_encode_avc_base.cpp

dvrogozh · 2018-12-13T18:39:40Z

media_driver/agnostic/common/codec/hal/codechal_encode_tracked_buffer.cpp

-            ReleaseSurfaceDS(i);
-#endif
+
+            if (m_encoder->m_openCommonKernel)


Too complex variable name and it misaligns with build option (FREE_KERNELS) which we are going to have. Can we, please, name it if (m_encoder->m_freeKernel). I think this will be simpler and feel more naturally.

"common" in a name looks weird. You plan to have non-common kernels?

sounds good to remove "common", but I think m_openKernel will be better than m_freeKernel. common means the kernel shared by all codecs

You know, I am still confused. We will have 3 build types:

No kernels at all (ENABLE_KERNELS=OFF)

Only kernels w/ open sources are available (FREE_KERNELS=ON)

Full feature build when we use pre-built kernels containing all source, for some of them we will not publish sources (default build)

So, in my mind your flag m_encoder->m_openCommonKernel should be set as follows:

m_openCommonKernel=false for "no kernels at all (ENABLE_KERNELS=OFF)"

m_openCommonKernel=true for "only kernels w/ open sources are available (FREE_KERNELS=ON)"

m_openCommonKernel=true for "full feature build when we use pre-built kernels containing all source, for some of them we will not publish sources (default build)"

However, I am not sure that this is what you had in mind and actually implemented. Moreover, the above usage is confusing. Really, see yourself:

In case of build 2) and 3) you should not differentiate the code which works with HME since feature presents in both open and non-open kernels

In case of 1) you natively should expect different flag like if(m_kernelsEnabled) meaning whether any kernels are enabled at all

That's the origin of my confusion. Here is what I naturally expect to see in the code:

if (m_kernelsEnabled && !m_openKernels) { //program some kernel for which we don't publish sources } if (m_kernelsEnabled) { //program some kernel for which we publish sources: no difference for 2) and 3) since kernel is available } bool m_kernelsEnabled; // =true if kernels enabled in the build, =false if kernels are not enabled at all bool m_openKernels; // =true if _only_ open kernel enabled in the build, =false if both open/closed kernels are enabled

Eventually you can consider to use ENABLE_KERNELS macro instead of bool variable, but idea still holds.

generally it's what I try to implement, only difference is that for free kernel, currently we only enabled ICLLP AVC VDEnc, for other codecs and platforms, it's not enabled, that's why I am using the variable rather than the MARCO to control it. Eventually if we have the free kernels for all platforms and codecs, we definitely can replace with the MARCO FREE_KERNELS

dvrogozh · 2018-12-13T18:42:01Z

media_driver/agnostic/common/codec/hal/codechal_encoder_base.cpp

-    if (!m_cscDsState->IsEnabled() ||
-        CodecHal_PictureIsField(m_currOriginalPic) ||
-        CodecHal_PictureIsInterlacedFrame(m_currOriginalPic))
+    if (m_openCommonKernel)


Shouldn't that be a check for if (m_cscDsState)?

From programming perspective, should be, but actually we have this check earlier when create the encoder instance, if it's nullptr, it will failed at initialization phase.

dvrogozh · 2018-12-13T18:43:33Z

media_driver/agnostic/common/codec/hal/codechal_encoder_base.cpp

@@ -4567,6 +4568,12 @@ CodechalEncoderState::CodechalEncoderState(
    MOS_ZeroMemory(&m_vdencMeKernelBindingTable, sizeof(m_vdencMeKernelBindingTable));

    MOS_ZeroMemory(&m_vdencStreaminKernelBindingTable, sizeof(m_vdencStreaminKernelBindingTable));
+
+#ifndef _FULL_OPEN_SOURCE
+    m_openCommonKernel = true;


Shouldn't one of these values be a default in the CodechalEncoderState::CodechalEncoderState constructor? You could ease the code and drop one of the branches.

sounds good

media_driver/agnostic/common/codec/hal/codechal_encoder_base.cpp

dvrogozh · 2018-12-13T19:02:31Z

media_driver/agnostic/gen11/codec/hal/codechal_kernel_header_g11.h

+    case ENC_SCALING_CONVERSION:
+        currKrnHeader = &kernelHeaderTable->dsConvertGenX0;
+        break;
+#ifndef _FULL_OPEN_SOURCE
    case VDENC_STREAMIN_HEVC:
        currKrnHeader = &kernelHeaderTable->hmeHevcVdenc;


Why you don't expose HME kernel for HEVC?

Current plan only enable for AVC VDEnc, for HEVC VDEnc not committed

dvrogozh · 2018-12-13T19:08:58Z

media_driver/agnostic/gen11/codec/hal/codechal_kernel_header_g11.h

    invalidEntry = &(kernelHeaderTable->weightedPrediction) + 1;
+#else
+    invalidEntry = &(kernelHeaderTable->dsConvertGenX0) + 1;


Oh my God. That's really ugly!!! How about:

struct HmeDsScoreboardKernelHeaderG11 { int nKernelCount; union { struct { CODECHAL_KERNEL_HEADER hmeDownscaleGenX0; CODECHAL_KERNEL_HEADER hmeDownscaleGenX1; CODECHAL_KERNEL_HEADER hmeDownscaleGenX2; CODECHAL_KERNEL_HEADER hmeDownscaleGenX3; CODECHAL_KERNEL_HEADER hmeP; CODECHAL_KERNEL_HEADER hmeB; CODECHAL_KERNEL_HEADER hmeVdenc; CODECHAL_KERNEL_HEADER hmeHevcVdenc; CODECHAL_KERNEL_HEADER dsConvertGenX0; #ifndef _FULL_OPEN_SOURCE CODECHAL_KERNEL_HEADER hmeDetectionGenX0; CODECHAL_KERNEL_HEADER initSwScoreboard; CODECHAL_KERNEL_HEADER intraDistortion; CODECHAL_KERNEL_HEADER dynamicScaling; CODECHAL_KERNEL_HEADER weightedPrediction; #endif CODECHAL_KERNEL_HEADER invalidEntry; }; }; }; invalidEntry = &kernelHeaderTable->invalidEntry;

Looks good, but we cannot change the original kernel definition sequences, it will require current kernel binary/tools change, not only for Linux open source, also will impact Windows

In that case we need to refactor for Windows as well. What exist in the code now is simply wrong and error prone. Actually, I don't quite understand what's the reason behind having pointer to the invalid entry?! Can you, please, explain more? In a way, what you have in a code now gives a question: do you have access violation hidden in the code? Why you store pointer to the address after your structure if you can have sizeof(structure)?

media_driver/agnostic/gen11/codec/hal/codechal_vdenc_avc_g11.cpp

dvrogozh · 2018-12-13T19:09:59Z

media_driver/agnostic/gen11/codec/hal/codechal_vdenc_avc_g11.cpp

@@ -692,6 +688,7 @@ CodechalVdencAvcStateG11::CodechalVdencAvcStateG11(
    Mos_SetVirtualEngineSupported(m_osInterface, true);

    m_vdboxOneDefaultUsed = true;
+    m_openCommonKernel = true;


I REALLY don't understand what you are doing with this flag!

it's the same purpose as the "FREE_KERNEL" MACRO proposed, however, with this flag I can control by each platform

I posted big comment above. Shortly: how you name the flag and how you use it in the code is misleading. "common" is misleading, "kernel" instead of "kernel>>>s<<<" is misleading. And having if (m_openCommonKernel) instead of if ( >>>!<<< m_openCommonKernel) is misleading.

IMHO, why you cover code which should be executed for both cases when you have open source kernels and when you have closed source kernels?! This is illogical. Expectation is that you will strip out the code which should not be executed for one of the path, i.e. the code for closed source part.

dvrogozh · 2018-12-13T19:15:15Z

media_driver/agnostic/gen11/codec/media_srcs.cmake

@@ -19,5 +19,9 @@
 # OTHER DEALINGS IN THE SOFTWARE.

 media_include_subdirectory(hal)
-media_include_subdirectory(kernel)
+if("${Full_Open_Source_Support}" STREQUAL "yes")
+  media_include_subdirectory(kernel_open)


I don't like word "open" in this context. Can we, please, use "free" instead?

Can you explain more details why prefer "free" to "open" ?

dvrogozh · 2018-12-14T19:37:22Z

media_driver/agnostic/common/codec/hal/codechal_encode_tracked_buffer.cpp

-            ReleaseSurfaceDS(i);
-#endif
+
+            if (m_encoder->m_openCommonKernel)


You know, I am still confused. We will have 3 build types:

No kernels at all (ENABLE_KERNELS=OFF)

Only kernels w/ open sources are available (FREE_KERNELS=ON)

Full feature build when we use pre-built kernels containing all source, for some of them we will not publish sources (default build)

So, in my mind your flag m_encoder->m_openCommonKernel should be set as follows:

m_openCommonKernel=false for "no kernels at all (ENABLE_KERNELS=OFF)"

m_openCommonKernel=true for "only kernels w/ open sources are available (FREE_KERNELS=ON)"

m_openCommonKernel=true for "full feature build when we use pre-built kernels containing all source, for some of them we will not publish sources (default build)"

However, I am not sure that this is what you had in mind and actually implemented. Moreover, the above usage is confusing. Really, see yourself:

In case of build 2) and 3) you should not differentiate the code which works with HME since feature presents in both open and non-open kernels

In case of 1) you natively should expect different flag like if(m_kernelsEnabled) meaning whether any kernels are enabled at all

That's the origin of my confusion. Here is what I naturally expect to see in the code:

if (m_kernelsEnabled && !m_openKernels) { //program some kernel for which we don't publish sources } if (m_kernelsEnabled) { //program some kernel for which we publish sources: no difference for 2) and 3) since kernel is available } bool m_kernelsEnabled; // =true if kernels enabled in the build, =false if kernels are not enabled at all bool m_openKernels; // =true if _only_ open kernel enabled in the build, =false if both open/closed kernels are enabled

Eventually you can consider to use ENABLE_KERNELS macro instead of bool variable, but idea still holds.

dvrogozh · 2018-12-14T19:40:54Z

media_driver/agnostic/gen11/codec/hal/codechal_kernel_header_g11.h

    invalidEntry = &(kernelHeaderTable->weightedPrediction) + 1;
+#else
+    invalidEntry = &(kernelHeaderTable->dsConvertGenX0) + 1;


In that case we need to refactor for Windows as well. What exist in the code now is simply wrong and error prone. Actually, I don't quite understand what's the reason behind having pointer to the invalid entry?! Can you, please, explain more? In a way, what you have in a code now gives a question: do you have access violation hidden in the code? Why you store pointer to the address after your structure if you can have sizeof(structure)?

dvrogozh · 2018-12-14T19:46:12Z

media_driver/agnostic/gen11/codec/hal/codechal_vdenc_avc_g11.cpp

@@ -692,6 +688,7 @@ CodechalVdencAvcStateG11::CodechalVdencAvcStateG11(
    Mos_SetVirtualEngineSupported(m_osInterface, true);

    m_vdboxOneDefaultUsed = true;
+    m_openCommonKernel = true;


I posted big comment above. Shortly: how you name the flag and how you use it in the code is misleading. "common" is misleading, "kernel" instead of "kernel>>>s<<<" is misleading. And having if (m_openCommonKernel) instead of if ( >>>!<<< m_openCommonKernel) is misleading.

IMHO, why you cover code which should be executed for both cases when you have open source kernels and when you have closed source kernels?! This is illogical. Expectation is that you will strip out the code which should not be executed for one of the path, i.e. the code for closed source part.

dvrogozh

You have changed this thru the code:
#if defined(ENABLE_KERNELS) && !defined(_FULL_OPEN_SOURCE)
to:
#if defined(ENABLE_KERNELS) || defined(_FULL_OPEN_SOURCE)
This is WRONG and CRITICAL problem. Please, revert. ENABLE_KERNELS should remove all the kernels from the driver regardless whether they are free or not.

Secondly, m_freeKernels variable name contradicts your own description and according to description should be named m_enableKernels. Please, change.

dvrogozh · 2018-12-19T17:10:53Z

media_driver/agnostic/common/codec/hal/codechal_encoder_base.h

@@ -1682,6 +1682,8 @@ class CodechalEncoderState : public Codechal

    bool m_colorbitSupported;

+    bool m_freeKernels; //!< Used to indicate whether has free kernels, it will be always true if ENABLE_KERNELS defined


According to your description, rename the variable to m_enableKernels. Name you have selected is misleading. It does not imply whether kernels support is enabled or disabled, instead it implies that you have some free and non-free kernels. Please, change.

Please properly rename.

dvrogozh · 2018-12-19T17:12:33Z

media_driver/agnostic/common/codec/hal/codechal_utilities.cpp

@@ -210,7 +210,15 @@ MOS_STATUS CodecHalGetKernelBinaryAndSize(
    uint8_t**  kernelBinary,
    uint32_t*  size)
 {
-#ifndef _FULL_OPEN_SOURCE
+#ifdef _FULL_OPEN_SOURCE
+    if (kernelBase == nullptr)


This is not an answer. Code review is exactly to check that usage is correct and prevent issues: new as well as old ones. Please, explain in details.

dvrogozh · 2018-12-19T17:13:58Z

media_driver/agnostic/common/codec/hal/codechal_utilities.cpp

+    *size = 0;
+    *kernelBinary = nullptr;
+#else
+#if defined(_FULL_OPEN_SOURCE)


use #elif instead, please. This will be shorter and cleaner.

dvrogozh · 2018-12-19T17:15:13Z

media_driver/agnostic/common/codec/hal/codechal_vdenc_avc.cpp

@@ -1233,6 +1235,9 @@ MOS_STATUS CodechalVdencAvcState::Initialize(CodechalSetting * settings)
            &userFeatureData);
        m_staticFrameDetectionEnable = (userFeatureData.i32Data) ? true : false;

+#ifndef ENABLE_KERNELS


If you decided to use if() instead of #if, then do that consistently, please. I.e. expectation is that you will have single #if covering variable assignment.

media_driver/agnostic/gen11/codec/hal/codechal_encode_csc_ds_g11.cpp

media_driver/agnostic/gen11/codec/hal/codechal_vdenc_avc_g11.cpp

dvrogozh · 2018-12-19T17:21:46Z

media_driver/agnostic/gen11/codec/hal/codechal_vdenc_avc_g11.cpp

@@ -676,7 +676,8 @@ CodechalVdencAvcStateG11::CodechalVdencAvcStateG11(

    CODECHAL_ENCODE_ASSERT(m_osInterface);

-#if defined(ENABLE_KERNELS) && !defined(_FULL_OPEN_SOURCE)
+#if defined(ENABLE_KERNELS) || defined(_FULL_OPEN_SOURCE)
+    m_freeKernels = true;


You have this assignment elsewhere. Why repeat?

I have rename it to m_enableHmeKernel, and in the base class you can see:
#if ENABLE_KERNELS
#if _FULL_OPEN_SOURCE
m_enableHmeKernel = false;
#else
m_enableHmeKernel = true;
#endif
#endif

When we only open free kernels, by default m_enableHmeKernel is disabled in the base, because we only enable it for ICL AVC VDEnc, that's why I add the assignment here to enable it for Gen11 AVC VDEnc

dvrogozh · 2018-12-19T17:21:56Z

media_driver/agnostic/gen11/codec/hal/codechal_vdenc_avc_g11.cpp

@@ -676,7 +676,8 @@ CodechalVdencAvcStateG11::CodechalVdencAvcStateG11(

    CODECHAL_ENCODE_ASSERT(m_osInterface);

-#if defined(ENABLE_KERNELS) && !defined(_FULL_OPEN_SOURCE)
+#if defined(ENABLE_KERNELS) || defined(_FULL_OPEN_SOURCE)


WRONG. CRITICAL. REVERT.

dvrogozh · 2018-12-19T17:22:06Z

media_driver/agnostic/gen11/codec/hal/codechal_vdenc_avc_g11.cpp

@@ -1075,7 +1076,7 @@ MOS_STATUS CodechalVdencAvcStateG11::ExecuteSliceLevel()
            &flushDwParams));
    }

-#if defined(ENABLE_KERNELS) && !defined(_FULL_OPEN_SOURCE)
+#if defined(ENABLE_KERNELS) || defined(_FULL_OPEN_SOURCE)


WRONG. CRITICAL. REVERT.

Should not revert, should be #if defined(ENABLE_KERNELS)

dvrogozh · 2018-12-19T17:24:18Z

media_driver/agnostic/gen11/codec/media_srcs.cmake

@@ -21,5 +21,7 @@
 media_include_subdirectory(hal)
 if(ENABLE_KERNELS AND NOT FREE_KERNELS)
    media_include_subdirectory(kernel)
+elseif(FREE_KERNELS)


This is wrong. ENABLE_KERNELS removes all the kernels, free or non-free - does not matter. This section should be:

if(ENABLE_KERNELS) if(FREE_KERNELS) media_include_subdirectory(kernel_free) else() media_include_subdirectory(kernel) endif() endif()

media_driver/agnostic/common/codec/hal/codechal_encoder_base.cpp

chivakker · 2018-12-20T17:52:41Z

media_driver/agnostic/common/codec/hal/codechal_encoder_base.h

@@ -1682,6 +1682,8 @@ class CodechalEncoderState : public Codechal

    bool m_colorbitSupported;

+    bool m_freeKernels; //!< Used to indicate whether has free kernels, it will be always true if ENABLE_KERNELS defined


Please properly rename.

chivakker · 2018-12-20T17:54:34Z

media_driver/agnostic/gen11/codec/kernel_free/Source/downscale_convert_kernel_genx.cpp

@@ -0,0 +1,898 @@
+/*
+* Copyright (c) 2017, Intel Corporation


This is 2018, check all files

sure, thanks

media_driver/agnostic/gen11/codec/hal/codechal_encode_csc_ds_g11.cpp

dvrogozh · 2018-12-28T22:02:16Z

media_driver/agnostic/common/codec/hal/codechal_encoder_base.cpp

+
+#if ENABLE_KERNELS
+    #if _FULL_OPEN_SOURCE
+        m_enableHmeKernel = false;


I thought that the whole idea of this patch was to have HME kernel available when _FULL_OPEN_SOURCE is defined. So, usage of this variable still don't have sense to me. Did you want to write this instead:

#if ENABLE_KERNELS m_enableHmeKernel = true; #endif

(assuming that default value =false)?

Not exactly, actually FREE_KERNELS only enabled for ICLLP AVC VDEnc, for other cases, still no DS+HME kernel for _FULL_OPEN_SOURCE case

dvrogozh · 2018-12-28T22:04:10Z

media_driver/agnostic/common/codec/hal/codechal_vdenc_avc.cpp

-    m_cscDsState->EnableColor();
-    m_cscDsState->EnableSfc();
-#endif
+    if (m_enableHmeKernel)


if (m_cscDsState) would be more straightforward I think.

dvrogozh · 2018-12-28T22:10:48Z

media_driver/agnostic/gen11/codec/kernel_free/media_srcs.cmake

+# ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
+# OTHER DEALINGS IN THE SOFTWARE.
+
+if(ENABLE_KERNELS)


no need in this if: you have it already on the upper level in media_driver/agnostic/gen11/codec/media_srcs.cmake

make sense , thanks

dvrogozh · 2018-12-28T22:14:43Z

media_driver/agnostic/common/codec/hal/codechal_encoder_base.cpp

-    if (!m_cscDsState->IsEnabled() ||
-        CodecHal_PictureIsField(m_currOriginalPic) ||
-        CodecHal_PictureIsInterlacedFrame(m_currOriginalPic))
+    if (m_enableHmeKernel && m_cscDsState)


Looks like that just if (m_cscDsState) should be enough.

eventually I removed the m_enableHmeKernel flag, and use m_cscDsState instead, they have the same effect.

dvrogozh · 2018-12-28T22:16:54Z

media_driver/agnostic/common/codec/hal/codechal_encode_tracked_buffer.cpp

-#ifndef _FULL_OPEN_SOURCE
-            ReleaseSurfaceDS(i);
-#endif
+            if (m_encoder->m_enableHmeKernel)


Here and below: is that possible and would that be reasonable to use if (m_cscDsState) instead of if (m_encoder->m_enableHmeKernel)?

thanks, it sounds makes sense

dvrogozh

Please, update https://github.com/intel/media-driver#open-source-shader-media-features with the info about HME kernel. This should be done within this PR.

dvrogozh · 2019-01-02T18:24:45Z

media_driver/agnostic/common/codec/hal/codechal_vdenc_avc.cpp

@@ -1233,6 +1235,9 @@ MOS_STATUS CodechalVdencAvcState::Initialize(CodechalSetting * settings)
            &userFeatureData);
        m_staticFrameDetectionEnable = (userFeatureData.i32Data) ? true : false;

+#if (_FULL_OPEN_SOURCE)


Can we, please, avoid calling MOS_UserFeature_ReadValue_ID() if we don't need to

I guess that =false is default value for m_staticFrameDetectionEnable. If so, we don't need the explicit assignment to false at all.

I suggest the code:

#ifndef _FULL_OPEN_SOURCE MOS_ZeroMemory(&userFeatureData, sizeof(userFeatureData)); MOS_UserFeature_ReadValue_ID( nullptr, __MEDIA_USER_FEATURE_VALUE_STATIC_FRAME_DETECTION_ENABLE_ID, &userFeatureData); m_staticFrameDetectionEnable = (userFeatureData.i32Data) ? true : false; #endif

looks better

dvrogozh · 2019-01-02T18:41:54Z

media_driver/agnostic/gen11_icllp/media_interfaces/media_interfaces_g11_icllp.cpp

@@ -474,14 +474,21 @@ MOS_STATUS CodechalInterfacesG11Icllp::Initialize(
            return MOS_STATUS_INVALID_PARAMETER;
        }

+#if defined(ENABLE_KERNELS)
+
+    #if defined(_FULL_OPEN_SOURCE)


Can you, please, reorder conditions? Like this:

#if defined(_FULL_OPEN_SOURCE) if (info->Mode != CODECHAL_ENCODE_MODE_JPEG) #else if (info->Mode == CODECHAL_ENCODE_MODE_AVC) #endif

Reason: we mostly use ifndef(_FULL_OPEN_SOURCE) thru the code. I afraid that going forward we can make some typo mistake copying from this place.

no, it's not correct, when we have _FULL_OPEN_SOURCE defined, only AVC VDEnc need to create the DS kernel, in your suggestion, it's just the opposite direction

I misprinted in the code sample, sorry. I meant:

#ifndef _FULL_OPEN_SOURCE if (info->Mode != CODECHAL_ENCODE_MODE_JPEG) #else if (info->Mode == CODECHAL_ENCODE_MODE_AVC) #endif

I suggested to have #ifndef _FULL_OPEN_SOURCE thru the driver code whenever possible.

dvrogozh · 2019-01-02T18:42:06Z

media_driver/linux/common/codec/ddi/media_ddi_encode_avc.cpp

@@ -1,5 +1,5 @@
 /*
-* Copyright (c) 2017, Intel Corporation
+* Copyright (c) 2017-2018, Intel Corporation


dvrogozh · 2019-01-03T20:55:40Z

Tools/MediaDriverTools/KernelBinToSource/KernelBinToSource.cpp

+    oSs << std::endl << "};" << std::endl;
+
+    oSs << "#else" << std::endl
+        << sSizeName.c_str() << " = 216;" << std::endl


What is =216 magic number? Left comprehensive comment or use =0 if possible.

Please, respond.

dvrogozh · 2019-01-03T20:56:39Z

Tools/MediaDriverTools/KernelBinToSource/KernelBinToSource.cpp

              << std::endl
              << "    " << PARAM_I << " Path to Kernel binary input file (required)"             << std::endl
              << "    " << PARAM_O << " Path to Kernel binary output directory (optional)"       << std::endl
-              << "    " << PARAM_V << " Variable Name on the generated source file (optional)"   << std::endl;
+              << "    " << PARAM_V << " Variable Name on the generated source file (optional)"   << std::endl
+              << "    " << PARAM_INDEX << " Variab kernel Index  (optional)"                     << std::endl


s/Variab/Variable/

dvrogozh · 2019-01-03T21:03:36Z

media_driver/agnostic/gen11/codec/kernel_free/build.py

@@ -0,0 +1,199 @@
+import os,shutil


Which python version is supposed? python-2 or python-3?

dvrogozh · 2019-01-03T21:41:36Z

CMakeLists.txt

+        message(FATAL_ERROR "Failed to find CMC")
+    endif()
+
+    find_program(PYTHON python)


which python we actually require? 2 or 3? Optimally we would wish to use FindPythonN cmake module (https://cmake.org/cmake/help/v3.12/module/FindPython3.html). The problem is that we will need to bump cmake version up to 3.13 where they appear.

dvrogozh · 2019-01-03T21:43:30Z

Tools/MediaDriverTools/KernelBinToSource/KernelBinToSource.cpp

-              << "    " << PARAM_V << " Variable Name on the generated source file (optional)"   << std::endl;
+              << "    " << PARAM_V << " Variable Name on the generated source file (optional)"   << std::endl
+              << "    " << PARAM_INDEX << " Variab kernel Index  (optional)"                     << std::endl
+              << "    " << PARAM_TOTAL << " Variable kernel total count (optional)"              << std::endl;


I don't understand what these variables do. Especially I don't understand what Index is for (total is I guess just total kernels count, but Index is a mystery to me).

Please, respond.

Index is for kernel offset. this situation we use 14 as kernel index

I still don't understand what is index needed for. Why it is 14? Please, explain.

dvrogozh · 2019-01-03T21:50:11Z

media_driver/agnostic/gen11/codec/kernel_free/media_srcs.cmake

+
+    if(GEN11_ICLLP)
+
+        exec_program(${PYTHON} ${CMAKE_CURRENT_LIST_DIR}


In this way you will run this command within configuration stage. I.e. you mix configuration and compilation stages. This will not do. Please, refactor in a way which will just configure targets and build them within make.

For media driver, it is not compilation. It is still in configration. if no kernel changes, file will not be chagned.

That's exactly the issue: you are running compilation commands within configuration stage. That's wrong.

To avoid exec_program you should specify input/output dependencies for the add_custom_command:

OUTPUT: kernel .h/.c files

DEPENDENCIES: KernelBinToSource <cm .c files>

In this way make will know that to to produce ihd_drv_video.so it need to produce kernel .h/.c files first and to do that it should run your custom commands, i.e. generate KernlBinToSource application and use .cm files as input dependencies.

media_driver/agnostic/gen11/codec/kernel_free/media_srcs.cmake

dvrogozh · 2019-01-04T19:53:51Z

@cchunbo: I see that you have updated the PR, but I don't see actual changes. What did you change? or that was just a rebase? I believe that feedback I gave was not yet addressed...

dvrogozh

Please, provide https://github.com/intel/media-driver/blob/master/README.md update in this PR which lists HME kernel in appropriate section(s).

dvrogozh · 2019-01-08T18:11:32Z

media_driver/agnostic/gen11_icllp/media_interfaces/media_interfaces_g11_icllp.cpp

@@ -474,14 +474,21 @@ MOS_STATUS CodechalInterfacesG11Icllp::Initialize(
            return MOS_STATUS_INVALID_PARAMETER;
        }

+#if defined(ENABLE_KERNELS)
+
+    #if defined(_FULL_OPEN_SOURCE)


I misprinted in the code sample, sorry. I meant:

#ifndef _FULL_OPEN_SOURCE if (info->Mode != CODECHAL_ENCODE_MODE_JPEG) #else if (info->Mode == CODECHAL_ENCODE_MODE_AVC) #endif

I suggested to have #ifndef _FULL_OPEN_SOURCE thru the driver code whenever possible.

dvrogozh · 2019-01-08T18:11:58Z

Tools/MediaDriverTools/KernelBinToSource/KernelBinToSource.cpp

+    oSs << std::endl << "};" << std::endl;
+
+    oSs << "#else" << std::endl
+        << sSizeName.c_str() << " = 216;" << std::endl


Please, respond.

dvrogozh · 2019-01-08T18:12:09Z

Tools/MediaDriverTools/KernelBinToSource/KernelBinToSource.cpp

-              << "    " << PARAM_V << " Variable Name on the generated source file (optional)"   << std::endl;
+              << "    " << PARAM_V << " Variable Name on the generated source file (optional)"   << std::endl
+              << "    " << PARAM_INDEX << " Variab kernel Index  (optional)"                     << std::endl
+              << "    " << PARAM_TOTAL << " Variable kernel total count (optional)"              << std::endl;


Please, respond.

dvrogozh · 2019-01-08T18:12:48Z

media_driver/agnostic/gen11/codec/kernel_free/media_srcs.cmake

+            OUTPUT ${cm_krn_dir}/${cm_krn}.c ${cm_krn_dir}/${cm_krn}.h
+            DEPENDS KernelBinToSource
+            COMMAND ${CMAKE_COMMAND} -E rename ${CMAKE_CURRENT_LIST_DIR}/commonkernel.krn ${CMAKE_CURRENT_LIST_DIR}/${cm_krn}.krn
+            COMMAND KernelBinToSource ARGS -i ${CMAKE_CURRENT_LIST_DIR}/${cm_krn}.krn -o ${cm_krn_dir} -v ${cm_krn} -index 14 -t 18


Please, respond.

dvrogozh · 2019-01-09T03:51:01Z

Tools/MediaDriverTools/KernelBinToSource/KernelBinToSource.cpp

-              << "    " << PARAM_V << " Variable Name on the generated source file (optional)"   << std::endl;
+              << "    " << PARAM_V << " Variable Name on the generated source file (optional)"   << std::endl
+              << "    " << PARAM_INDEX << " Variab kernel Index  (optional)"                     << std::endl
+              << "    " << PARAM_TOTAL << " Variable kernel total count (optional)"              << std::endl;


I still don't understand what is index needed for. Why it is 14? Please, explain.

dvrogozh · 2019-01-09T03:51:57Z

media_driver/agnostic/gen11/codec/kernel_free/media_srcs.cmake

+
+    if(GEN11_ICLLP)
+
+        exec_program(${PYTHON} ${CMAKE_CURRENT_LIST_DIR}


That's exactly the issue: you are running compilation commands within configuration stage. That's wrong.

dvrogozh · 2019-01-09T17:33:51Z

media_driver/agnostic/common/codec/hal/codechal_encoder_base.cpp

    {
+      // check recon surface's alignment meet HW requirement


minor: something is wrong with text alignment here

dvrogozh · 2019-01-09T19:06:49Z

media_driver/agnostic/gen11/codec/kernel_free/media_srcs.cmake

+            DEPENDS KernelBinToSource
+            COMMAND cp -r ${CMAKE_CURRENT_LIST_DIR}/Source .
+            COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_LIST_DIR}/build.py .
+            COMMAND ${PYTHON} ARGS build.py


Please, avoid copying of the python script with:
COMMAND ${PYTHON} ARGS ${CMAKE_CURRENT_LIST_DIR}/build.py

dvrogozh · 2019-01-09T19:11:15Z

media_driver/agnostic/gen11/codec/kernel_free/media_srcs.cmake

+            COMMAND ${CMAKE_COMMAND} -E copy ${CMAKE_CURRENT_LIST_DIR}/build.py .
+            COMMAND ${PYTHON} ARGS build.py
+            COMMAND ${CMAKE_COMMAND} -E rename commonkernel.krn ${cm_krn}.krn
+            COMMAND KernelBinToSource ARGS -i ${cm_krn}.krn -o . -v ${cm_krn} -index 14 -t 17


Add a comment before this line:

# -index 14 -t 17 are needed to match a description of the kernel in # media_driver/agnostic/common/codec/kernel/codeckrnheader.h: # 1. We generate a kernel with index 14 (IDR_CODEC_HME_DS_SCOREBOARD_KERNEL) # 2. And the total number of kernels known to the media driver is 17 (IDR_CODEC_TOTAL_NUM_KERNELS)

dvrogozh · 2019-01-09T19:15:25Z

media_driver/agnostic/gen11/codec/kernel_free/media_srcs.cmake

+            COMMAND ${PYTHON} ARGS build.py
+            COMMAND ${CMAKE_COMMAND} -E rename commonkernel.krn ${cm_krn}.krn
+            COMMAND KernelBinToSource ARGS -i ${cm_krn}.krn -o . -v ${cm_krn} -index 14 -t 17
+            COMMAND ${CMAKE_COMMAND} -E copy ${cm_krn}.c ${cm_krn}.h ${CMAKE_CURRENT_LIST_DIR}/


You probably should not need this command. You can have just:
COMMAND KernelBinToSource ARGS -i ${cm_krn}.krn -o ${CMAKE_CURRENT_LIST_DIR}/ -v ${cm_krn} -index 14 -t 17

dvrogozh · 2019-01-09T19:15:50Z

media_driver/agnostic/gen11/codec/kernel_free/media_srcs.cmake

@@ -18,6 +18,33 @@
 # ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR
 # OTHER DEALINGS IN THE SOFTWARE.

+if (BUILD_KERNELS)
+
+    function(gen_cm_kernel_from_source name platform)


Please, move this function definition to https://github.com/intel/media-driver/blob/master/media_driver/agnostic/media_srcs.cmake

dvrogozh · 2019-01-09T19:18:08Z

media_driver/agnostic/gen11/codec/kernel_free/media_srcs.cmake

+    function(gen_cm_kernel_from_source name platform)
+
+        set(cm_krn_dir ${CMAKE_SOURCE_DIR}/media_driver/agnostic/${platform}/codec/kernel_free)
+        set(cm_krn ig${name}krn_g11)


Use instead:

platform_to_genx(${platform} genx kind) set(cm_krn ig${name}krn_g${genx})

You may need to adjust platform_to_genx to recognize gen11 (it now knows only gen11_icllp).

Signed-off-by: Yang, Ning <ning.yang@intel.com>

dvrogozh · 2019-01-09T21:35:43Z

media_driver/agnostic/gen11/codec/kernel_free/build.py

+            fp = os.path.join('./Source', filename)
+            fileext = os.path.splitext(filename)[1]
+            if os.path.isfile(fp) and 'cpp' in fileext:
+                command  = 'cmc /c /Qxcm -Qxcm_jit_target=gen11lp '


-Qxcm_jit_target=gen11lp - why?! The kernel is inside gen11 folder. So, for which platform this kernel is compiled?

dvrogozh · 2019-01-09T21:36:13Z

media_driver/agnostic/gen11/codec/kernel_free/build.py

+        if not isExists:
+            os.mkdir("Binary")
+
+        shutil.copy('downscale_kernel_genx_0.dat', 'Binary/DS4x_Frame.krn')


Does the order matters? If yes, how the driver knows about it?

dvrogozh · 2019-01-30T21:15:29Z

Closing in favor of #489.

dvrogozh suggested changes Dec 13, 2018

View reviewed changes

dvrogozh suggested changes Dec 14, 2018

View reviewed changes

cchunbo force-pushed the dev/icl_avcvdenc_tu1 branch 2 times, most recently from ee08d97 to 147ca50 Compare December 19, 2018 15:14

dvrogozh suggested changes Dec 19, 2018

View reviewed changes

chivakker reviewed Dec 20, 2018

View reviewed changes

dvrogozh suggested changes Dec 21, 2018

View reviewed changes

media_driver/agnostic/gen11/codec/hal/codechal_encode_csc_ds_g11.cpp Show resolved Hide resolved

cchunbo force-pushed the dev/icl_avcvdenc_tu1 branch 5 times, most recently from 84f2f00 to 82fec97 Compare December 28, 2018 07:26

dvrogozh suggested changes Dec 28, 2018

View reviewed changes

cchunbo force-pushed the dev/icl_avcvdenc_tu1 branch 2 times, most recently from 414f5d9 to a5eb184 Compare January 2, 2019 09:32

dvrogozh suggested changes Jan 3, 2019

View reviewed changes

cchunbo force-pushed the dev/icl_avcvdenc_tu1 branch from a5eb184 to 01e2348 Compare January 4, 2019 06:38

XinfengZhang added the P1 Highest priority label Jan 7, 2019

cchunbo force-pushed the dev/icl_avcvdenc_tu1 branch from 01e2348 to 20ed8e3 Compare January 8, 2019 05:23

[Encode] Enable DS and HME for ICLLP AVC VDEnc

1a7e003

dvrogozh suggested changes Jan 8, 2019

View reviewed changes

dvrogozh suggested changes Jan 9, 2019

View reviewed changes

cchunbo force-pushed the dev/icl_avcvdenc_tu1 branch from 20ed8e3 to bca8e2e Compare January 9, 2019 13:12

dvrogozh suggested changes Jan 9, 2019

View reviewed changes

yangning1368 and others added 2 commits January 10, 2019 05:05

Add build support for hme kernel open source

6298ade

Signed-off-by: Yang, Ning <ning.yang@intel.com>

[Encode] Add README for free kernel build

bca8e2e

dvrogozh suggested changes Jan 9, 2019

View reviewed changes

dvrogozh mentioned this pull request Jan 11, 2019

Add HME support for ICLLP AVC VDEnc (refactored) #489

Merged

dvrogozh closed this Jan 30, 2019

		@@ -1682,6 +1682,8 @@ class CodechalEncoderState : public Codechal

		bool m_colorbitSupported;

		bool m_freeKernels; //!< Used to indicate whether has free kernels, it will be always true if ENABLE_KERNELS defined


		if(GEN11_ICLLP)

		exec_program(${PYTHON} ${CMAKE_CURRENT_LIST_DIR}

Add HME support for ICLLP AVC VDEnc #463

Add HME support for ICLLP AVC VDEnc #463

Conversation

cchunbo commented Dec 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvrogozh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchunbo Dec 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvrogozh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvrogozh Jan 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvrogozh commented Jan 4, 2019

dvrogozh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cchunbo Dec 27, 2018 •

edited

Loading

dvrogozh Jan 3, 2019 •

edited

Loading