HDR enable #701

yuanzhel · 2020-08-31T00:47:58Z

This PR related to projectceladon/device-androidia-mixins#850

shuangwan01

LGTM

YuanjunHuang · 2020-10-09T07:53:44Z

wsi/drm/drmdisplay.cpp


  if (display_state_ & kNeedsModeset) {
+    /* KK: Put to check only if input layer is a hdr */
+    for (const DisplayPlaneState &comp_plane : composition_planes) {


Need to check if the layer is HDR layer, otherwise no need to apply HDR drm mode set

also need check if the display supports HDR here, for example check display_hdrMd

YuanjunHuang · 2020-10-09T07:53:56Z

wsi/drm/drmdisplay.cpp

      return false;
    }
+
+    if (!ApplyPendingHdr(pset.get(), &color_state)) {


same as above

struct hdr_metadata final_hdr_metadata, layer_metadata

YuanjunHuang · 2020-10-09T08:06:22Z

wsi/drm/drmdisplay.cpp

+                               target->hdr_md_blob_id) < 0 ||
+      drmModeAtomicAddProperty(property_set, connector_, colorspace_op_prop_,
+                               to_kernel_colorspace(color_state.o_cs)) < 0;
+  if (ret) {


before return, need to call drmModeDestroyPropertyBlob to avoid memory leak.

YuanjunHuang · 2020-10-09T08:15:47Z

wsi/drm/drmdisplay.cpp

+    *outMinLuminance = float(outminluminance);
+  }
+
+  return true;


this will be always true, no matter if (display_hdrMd), should be wrong.

YuanjunHuang · 2020-10-09T08:18:16Z

common/core/logicaldisplay.cpp

+                                        float *outMaxLuminance,
+                                        float *outMaxAverageLuminance,
+                                        float *outMinLuminance) {
+  return true;


default shall be false.

YuanjunHuang · 2020-10-09T08:18:24Z

common/core/mosaicdisplay.cpp

+                                       float *outMaxLuminance,
+                                       float *outMaxAverageLuminance,
+                                       float *outMinLuminance) {
+  return true;


default shall be false.

YuanjunHuang · 2020-10-09T08:18:31Z

common/display/virtualdisplay.cpp

+                                        float *outMaxLuminance,
+                                        float *outMaxAverageLuminance,
+                                        float *outMinLuminance) {
+  return true;


default shall be false.

same with Mosaic and logic

YuanjunHuang · 2020-10-09T08:18:38Z

common/display/virtualpanoramadisplay.cpp

+                                                float *outMaxLuminance,
+                                                float *outMaxAverageLuminance,
+                                                float *outMinLuminance) {
+  return true;


default shall be false.

same with mosaic and logic

YuanjunHuang · 2020-10-09T08:38:05Z

wsi/drm/drmdisplay.cpp

+    return;
+  }
+
+  display_hdrMd = (struct drm_edid_hdr_metadata_static *)malloc(


didn't see where display_hdrMd is freed?

Shao-Feng · 2020-10-10T06:19:59Z

common/core/logicaldisplay.h

  bool GetDisplayIdentificationData(uint8_t *outPort, uint32_t *outDataSize,
                                    uint8_t *outData) override;

  void GetDisplayCapabilities(uint32_t *numCapabilities,


Does HDR only work on DrmDisplay?
if yes, no need to add this method into Logic and Mosaic display.
you can add a default method on Native display and return false.

Shao-Feng · 2020-10-10T06:21:26Z

common/core/overlaylayer.h

  LayerComposition actual_composition_ = kAll;
  HWCLayerType type_ = kLayerNormal;
+
+  struct hdr_metadata hdrmetadata;


property should be named with "_" at the end to be different with variable in method

hdr_metadata_
color_space_

Shao-Feng · 2020-10-10T06:22:28Z

common/display/virtualdisplay.cpp

+                                        float *outMaxLuminance,
+                                        float *outMaxAverageLuminance,
+                                        float *outMinLuminance) {
+  return true;


same with Mosaic and logic

Shao-Feng · 2020-10-10T06:22:57Z

common/display/virtualdisplay.h

  bool GetDisplayIdentificationData(uint8_t *outPort, uint32_t *outDataSize,
                                    uint8_t *outData) override;

  void GetDisplayCapabilities(uint32_t *numCapabilities,


same with mosaic and logic

Shao-Feng · 2020-10-10T06:23:26Z

common/display/virtualpanoramadisplay.cpp

+                                                float *outMaxLuminance,
+                                                float *outMaxAverageLuminance,
+                                                float *outMinLuminance) {
+  return true;


same with mosaic and logic

Shao-Feng · 2020-10-10T06:41:39Z

public/hwclayer.h

 #include <hwcdefs.h>
-
 #include <platformdefines.h>
+#include "hdr_metadata_defs.h"


hdr_metadata_defs.h is also in public folder as same as hwcdefs.h
it is better to use <hdr_metadata_defs.h>

Shao-Feng · 2020-10-10T06:48:31Z

public/hwclayer.h

  uint32_t solid_color_ = 0xff;
  bool use_for_mosaic_ = false;

+  struct hdr_metadata hdr_mdata;


hdr_mdata_
colorspace_

and need to initialize with null and 0

Shao-Feng · 2020-10-10T06:50:35Z

tests/apps/linux_hdr_image_test.cpp

@@ -0,0 +1,657 @@
+/*


tests is not updated for a long time, did you ever try to run this test?

Shao-Feng · 2020-10-10T06:51:32Z

wsi/drm/drmdisplay.cpp

 */

 #include "drmdisplay.h"
+#include "hdr_metadata_defs.h"


it is a public header file, use <>

Shao-Feng · 2020-10-10T06:52:05Z

wsi/drm/drmdisplay.cpp

 #include "displayplanemanager.h"
 #include "displayqueue.h"
 #include "drmdisplaymanager.h"
+#include "hdr_metadata_defs.h"


why it is included twice?

Shao-Feng · 2020-10-10T07:56:58Z

wsi/drm/drmdisplay.cpp

+
+bool DrmDisplay::GetPerFrameMetadataKeys(uint32_t *outNumKeys,
+                                         int32_t *outKeys) {
+  *outNumKeys = KEY_NUM_PER_FRAME_METADATA_KEYS;


KEY_NUM_PER_FRAME_METADATA_KEYS is a enum, and not assigned with a value explicitly

Shao-Feng · 2020-10-10T08:02:24Z

wsi/drm/drmdisplay.cpp

+    return true;
+  for (int i = 0; i < KEY_NUM_PER_FRAME_METADATA_KEYS; i++) {
+    *(outKeys + i) = i;
+  }


outKey[i] = i;

Shao-Feng · 2020-10-10T08:04:06Z

In original coding style, we use "_" in parameter name, instead of Upcase

Shao-Feng · 2020-10-10T08:06:49Z

wsi/drm/drmdisplay.h

  uint32_t prefer_display_mode_ = 0;
  uint32_t perf_display_mode_ = 0;
-  std::string display_name_ = "";
+  std::string display_name_;


why not initialize display_name_

Shao-Feng · 2020-10-10T08:07:34Z

wsi/drm/drmdisplay.h

+  std::vector<int32_t> current_color_mode_ = {HAL_COLOR_MODE_NATIVE};
+
+  /* Display's static HDR metadata */
+  struct drm_edid_hdr_metadata_static *display_hdrMd;


need end with "_" for all propertys

implement vsync2.4 callback, setactiveconfigwithconstraints and getdisplayvsyncperiod. Add an empty SetDisplayBrightness to resolove boot problem Set the ALL_EDID_FLAG_PROPERTY to send SF all available modes. Tracked-On: OAM-92711 Signed-Off-By: Liu, Yuanzhe <yuanzhe.liu@intel.com>

shuangwan01 approved these changes Sep 17, 2020

View reviewed changes

YuanjunHuang reviewed Oct 9, 2020

View reviewed changes

Shao-Feng reviewed Oct 10, 2020

View reviewed changes

HDR enable #701

Are you sure you want to change the base?

HDR enable #701

Uh oh!

Conversation

yuanzhel commented Aug 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shuangwan01 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Shao-Feng commented Oct 10, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

yuanzhel commented Aug 31, 2020 •

edited

Loading