Re: [PATCH xf86-video-amdgpu] Add checking color management properties

Hi Aaron,

thanks for the patch. Moving to the amd-gfx mailing list, where xf86-video-amdgpu patches are reviewed. Comments inline below.

On 2018-09-10 1:14 p.m., Aaron Liu wrote:

Add gamma_lut/degamma_lut/ctm checking before pushing staged color management properties on the CRTC. If above object is NULL, return directly.

Signed-off-by: Aaron Liu aaron.liu@amd.com

src/drmmode_display.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)

diff --git a/src/drmmode_display.c b/src/drmmode_display.c index 68fca1b..006ae7c 100644 --- a/src/drmmode_display.c +++ b/src/drmmode_display.c @@ -1116,6 +1116,11 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc,

switch (cm_prop_index) { case CM_GAMMA_LUT:

• if (!drmmode_crtc->gamma_lut) {
  

• 	xf86DrvMsg(crtc->scrn->scrnIndex, X_WARNING,
  

• 		   "gamma_lut is NULL!\n");
  

• 	return BadRequest;
  

• }
  

  /* Calculate the expected size of value in bytes */ expected_bytes = sizeof(struct drm_color_lut) * drmmode->gamma_lut_size;

@@ -1154,11 +1159,21 @@ static int drmmode_crtc_push_cm_prop(xf86CrtcPtr crtc, } break; case CM_DEGAMMA_LUT:

• if (!drmmode_crtc->degamma_lut) {
  

• 	xf86DrvMsg(crtc->scrn->scrnIndex, X_WARNING,
  

• 		   "degamma_lut is NULL!\n");
  

• 	return BadRequest;
  

• }
  

  expected_bytes = sizeof(struct drm_color_lut) * drmmode->degamma_lut_size; blob_data = drmmode_crtc->degamma_lut; break; case CM_CTM:

• if (!drmmode_crtc->ctm) {
  

• 	xf86DrvMsg(crtc->scrn->scrnIndex, X_WARNING,
  

• 		   "ctm is NULL!\n");
  

• 	return BadRequest;
  

• }
  

  expected_bytes = sizeof(struct drm_color_ctm); blob_data = drmmode_crtc->ctm; break;

(How) were you able to actually trigger this condition in practice? The only way I could imagine is via drmmode_output_set_property, in which case I think it would be better to guard all the *cm_* related code there with

if (drmmode_cm_enabled(drmmode_output->drmmode)) { ... }