Thanks for the review Daniel. My comments inline.
Regards Shashank On 4/21/2016 8:34 PM, Daniel Vetter wrote: > On Fri, Mar 25, 2016 at 01:47:35PM +0530, Shashank Sharma wrote: >> HDMI 2.0/CEA-861-F introduces two new aspect ratios: >> - 64:27 >> - 256:135 >> >> This patch adds support for these aspect ratios in >> I915 driver, at various places. >> >> Signed-off-by: Shashank Sharma <shashank.sharma at intel.com> > > Ok, we discussed this a bit internally and I had some questions. Reading > this series patches make sense up to this one, but here I have a few > questions/comments. > >> --- >> drivers/gpu/drm/drm_modes.c | 12 ++++++++++++ >> drivers/gpu/drm/i915/intel_hdmi.c | 6 ++++++ >> drivers/gpu/drm/i915/intel_sdvo.c | 6 ++++++ >> 3 files changed, 24 insertions(+) >> >> diff --git a/drivers/gpu/drm/drm_modes.c b/drivers/gpu/drm/drm_modes.c >> index 6e66136..11f219a 100644 >> --- a/drivers/gpu/drm/drm_modes.c >> +++ b/drivers/gpu/drm/drm_modes.c >> @@ -1482,6 +1482,12 @@ void drm_mode_convert_to_umode(struct >> drm_mode_modeinfo *out, >> case HDMI_PICTURE_ASPECT_16_9: >> out->flags |= DRM_MODE_FLAG_PAR16_9; >> break; >> + case HDMI_PICTURE_ASPECT_64_27: >> + out->flags |= DRM_MODE_FLAG_PAR64_27; >> + break; >> + case DRM_MODE_PICTURE_ASPECT_256_135: >> + out->flags |= DRM_MODE_FLAG_PAR256_135; >> + break; >> case HDMI_PICTURE_ASPECT_NONE: >> case HDMI_PICTURE_ASPECT_RESERVED: >> default: >> @@ -1544,6 +1550,12 @@ int drm_mode_convert_umode(struct drm_display_mode >> *out, >> case DRM_MODE_FLAG_PAR16_9: >> out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_16_9; >> break; >> + case DRM_MODE_FLAG_PAR64_27: >> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_64_27; >> + break; >> + case DRM_MODE_FLAG_PAR256_135: >> + out->picture_aspect_ratio |= HDMI_PICTURE_ASPECT_256_135; >> + break; >> default: >> out->picture_aspect_ratio = HDMI_PICTURE_ASPECT_NONE; >> } > > Above two parts is core enabling, I guess those should be squashed into > the preceeding patch? > Sure, we can do this. The idea was to create a separate patch for new aspect ratio, so that one can segregate HDMI 2.0 patches and HDMI 1.4 patches. If you still recommend this, I can move this part in next patch. >> diff --git a/drivers/gpu/drm/i915/intel_hdmi.c >> b/drivers/gpu/drm/i915/intel_hdmi.c >> index e2dab48..bc8e2c8 100644 >> --- a/drivers/gpu/drm/i915/intel_hdmi.c >> +++ b/drivers/gpu/drm/i915/intel_hdmi.c >> @@ -1545,6 +1545,12 @@ intel_hdmi_set_property(struct drm_connector >> *connector, >> case DRM_MODE_PICTURE_ASPECT_16_9: >> intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_16_9; >> break; >> + case DRM_MODE_PICTURE_ASPECT_64_27: >> + intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_64_27; >> + break; >> + case DRM_MODE_PICTURE_ASPECT_256_135: >> + intel_hdmi->aspect_ratio = HDMI_PICTURE_ASPECT_256_135; >> + break; >> default: >> return -EINVAL; >> } >> diff --git a/drivers/gpu/drm/i915/intel_sdvo.c >> b/drivers/gpu/drm/i915/intel_sdvo.c >> index fae64bc..370e4f9 100644 >> --- a/drivers/gpu/drm/i915/intel_sdvo.c >> +++ b/drivers/gpu/drm/i915/intel_sdvo.c >> @@ -2071,6 +2071,12 @@ intel_sdvo_set_property(struct drm_connector >> *connector, >> case DRM_MODE_PICTURE_ASPECT_16_9: >> intel_sdvo->aspect_ratio = HDMI_PICTURE_ASPECT_16_9; >> break; >> + case DRM_MODE_PICTURE_ASPECT_64_27: >> + intel_sdvo->aspect_ratio = HDMI_PICTURE_ASPECT_64_27; >> + break; >> + case DRM_MODE_PICTURE_ASPECT_256_135: >> + intel_sdvo->aspect_ratio = HDMI_PICTURE_ASPECT_256_135; >> + break; >> default: >> return -EINVAL; >> } > > The trouble with the aspect_ratio prop as currently implemented is that we > currently unconditionally overwrite adjusted_mode->picture_aspect_ratio > with it. > > But your patches here move the aspect ratio handling into > drm_mode_modeinfo (uabi) and drm_display_mode (kernel-internal), where it > imo belongs. But we need to figure out how to the interaction with the > property correctly. What's probably needed is a new value in the > aspect_ratio enum called "default", which selects the aspect_ratio from > the mode. Only when userspace overrides (NONE, or one of the CEA aspect > ratios) would we obey the aspect_ratio of the property. Otherwise the > aspect ratio stored in the mode would be lost. This is exactly how we have handled this in Android branch(with NONE as default), thanks for the suggestion. I will incorporate this in next patch set. > > The other bit that I can't find in this series is making sure we compute > the VIC correctly for the new modes. Or does that magically work correctly > since we compare the full mode when selecting VICs? > Yes, we are saving the right VIC from EDID, so the VIC is now a part of the mode flags, all we have to do extract this and write during preparing AVI-IF, we have a small one line patch for that. I will add that too in the next series. > Thanks, Daniel >
