From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sharma, Shashank" Date: Fri, 22 Apr 2016 05:40:34 +0000 Subject: Re: [Intel-gfx] [PATCH 5/5] drm/i915: Add support for new aspect ratios Message-Id: <5719B682.1030504@intel.com> List-Id: References: <1458893855-3930-1-git-send-email-shashank.sharma@intel.com> <1458893855-3930-6-git-send-email-shashank.sharma@intel.com> <20160421150414.GB2510@phenom.ffwll.local> In-Reply-To: <20160421150414.GB2510@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Daniel Vetter Cc: linux-fbdev@vger.kernel.org, airlied@linux.ie, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, daniel.vetter@intel.com, plagnioj@jcrosoft.com 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 > > 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 > From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Sharma, Shashank" Subject: Re: [PATCH 5/5] drm/i915: Add support for new aspect ratios Date: Fri, 22 Apr 2016 10:58:34 +0530 Message-ID: <5719B682.1030504@intel.com> References: <1458893855-3930-1-git-send-email-shashank.sharma@intel.com> <1458893855-3930-6-git-send-email-shashank.sharma@intel.com> <20160421150414.GB2510@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160421150414.GB2510@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: linux-fbdev@vger.kernel.org, airlied@linux.ie, intel-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org, daniel.vetter@intel.com, plagnioj@jcrosoft.com List-Id: dri-devel@lists.freedesktop.org VGhhbmtzIGZvciB0aGUgcmV2aWV3IERhbmllbC4KTXkgY29tbWVudHMgaW5saW5lLgoKUmVnYXJk cwpTaGFzaGFuawpPbiA0LzIxLzIwMTYgODozNCBQTSwgRGFuaWVsIFZldHRlciB3cm90ZToKPiBP biBGcmksIE1hciAyNSwgMjAxNiBhdCAwMTo0NzozNVBNICswNTMwLCBTaGFzaGFuayBTaGFybWEg d3JvdGU6Cj4+IEhETUkgMi4wL0NFQS04NjEtRiBpbnRyb2R1Y2VzIHR3byBuZXcgYXNwZWN0IHJh dGlvczoKPj4gLSA2NDoyNwo+PiAtIDI1NjoxMzUKPj4KPj4gVGhpcyBwYXRjaCBhZGRzIHN1cHBv cnQgZm9yIHRoZXNlIGFzcGVjdCByYXRpb3MgaW4KPj4gSTkxNSBkcml2ZXIsIGF0IHZhcmlvdXMg cGxhY2VzLgo+Pgo+PiBTaWduZWQtb2ZmLWJ5OiBTaGFzaGFuayBTaGFybWEgPHNoYXNoYW5rLnNo YXJtYUBpbnRlbC5jb20+Cj4KPiBPaywgd2UgZGlzY3Vzc2VkIHRoaXMgYSBiaXQgaW50ZXJuYWxs eSBhbmQgSSBoYWQgc29tZSBxdWVzdGlvbnMuIFJlYWRpbmcKPiB0aGlzIHNlcmllcyBwYXRjaGVz IG1ha2Ugc2Vuc2UgdXAgdG8gdGhpcyBvbmUsIGJ1dCBoZXJlIEkgaGF2ZSBhIGZldwo+IHF1ZXN0 aW9ucy9jb21tZW50cy4KPgo+PiAtLS0KPj4gICBkcml2ZXJzL2dwdS9kcm0vZHJtX21vZGVzLmMg ICAgICAgfCAxMiArKysrKysrKysrKysKPj4gICBkcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9o ZG1pLmMgfCAgNiArKysrKysKPj4gICBkcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9zZHZvLmMg fCAgNiArKysrKysKPj4gICAzIGZpbGVzIGNoYW5nZWQsIDI0IGluc2VydGlvbnMoKykKPj4KPj4g ZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fbW9kZXMuYyBiL2RyaXZlcnMvZ3B1L2Ry bS9kcm1fbW9kZXMuYwo+PiBpbmRleCA2ZTY2MTM2Li4xMWYyMTlhIDEwMDY0NAo+PiAtLS0gYS9k cml2ZXJzL2dwdS9kcm0vZHJtX21vZGVzLmMKPj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2RybV9t b2Rlcy5jCj4+IEBAIC0xNDgyLDYgKzE0ODIsMTIgQEAgdm9pZCBkcm1fbW9kZV9jb252ZXJ0X3Rv X3Vtb2RlKHN0cnVjdCBkcm1fbW9kZV9tb2RlaW5mbyAqb3V0LAo+PiAgIAljYXNlIEhETUlfUElD VFVSRV9BU1BFQ1RfMTZfOToKPj4gICAJCW91dC0+ZmxhZ3MgfD0gRFJNX01PREVfRkxBR19QQVIx Nl85Owo+PiAgIAkJYnJlYWs7Cj4+ICsJY2FzZSBIRE1JX1BJQ1RVUkVfQVNQRUNUXzY0XzI3Ogo+ PiArCQlvdXQtPmZsYWdzIHw9IERSTV9NT0RFX0ZMQUdfUEFSNjRfMjc7Cj4+ICsJCWJyZWFrOwo+ PiArCWNhc2UgRFJNX01PREVfUElDVFVSRV9BU1BFQ1RfMjU2XzEzNToKPj4gKwkJb3V0LT5mbGFn cyB8PSBEUk1fTU9ERV9GTEFHX1BBUjI1Nl8xMzU7Cj4+ICsJCWJyZWFrOwo+PiAgIAljYXNlIEhE TUlfUElDVFVSRV9BU1BFQ1RfTk9ORToKPj4gICAJY2FzZSBIRE1JX1BJQ1RVUkVfQVNQRUNUX1JF U0VSVkVEOgo+PiAgIAlkZWZhdWx0Ogo+PiBAQCAtMTU0NCw2ICsxNTUwLDEyIEBAIGludCBkcm1f bW9kZV9jb252ZXJ0X3Vtb2RlKHN0cnVjdCBkcm1fZGlzcGxheV9tb2RlICpvdXQsCj4+ICAgCWNh c2UgRFJNX01PREVfRkxBR19QQVIxNl85Ogo+PiAgIAkJb3V0LT5waWN0dXJlX2FzcGVjdF9yYXRp byB8PSBIRE1JX1BJQ1RVUkVfQVNQRUNUXzE2Xzk7Cj4+ICAgCQlicmVhazsKPj4gKwljYXNlIERS TV9NT0RFX0ZMQUdfUEFSNjRfMjc6Cj4+ICsJCW91dC0+cGljdHVyZV9hc3BlY3RfcmF0aW8gfD0g SERNSV9QSUNUVVJFX0FTUEVDVF82NF8yNzsKPj4gKwkJYnJlYWs7Cj4+ICsJY2FzZSBEUk1fTU9E RV9GTEFHX1BBUjI1Nl8xMzU6Cj4+ICsJCW91dC0+cGljdHVyZV9hc3BlY3RfcmF0aW8gfD0gSERN SV9QSUNUVVJFX0FTUEVDVF8yNTZfMTM1Owo+PiArCQlicmVhazsKPj4gICAJZGVmYXVsdDoKPj4g ICAJCW91dC0+cGljdHVyZV9hc3BlY3RfcmF0aW8gPSBIRE1JX1BJQ1RVUkVfQVNQRUNUX05PTkU7 Cj4+ICAgCX0KPgo+IEFib3ZlIHR3byBwYXJ0cyBpcyBjb3JlIGVuYWJsaW5nLCBJIGd1ZXNzIHRo b3NlIHNob3VsZCBiZSBzcXVhc2hlZCBpbnRvCj4gdGhlIHByZWNlZWRpbmcgcGF0Y2g/Cj4KU3Vy ZSwgd2UgY2FuIGRvIHRoaXMuClRoZSBpZGVhIHdhcyB0byBjcmVhdGUgYSBzZXBhcmF0ZSBwYXRj aCBmb3IgbmV3IGFzcGVjdCByYXRpbywgc28gdGhhdCAKb25lIGNhbiBzZWdyZWdhdGUgSERNSSAy LjAgcGF0Y2hlcyBhbmQgSERNSSAxLjQgcGF0Y2hlcy4gSWYgeW91IHN0aWxsIApyZWNvbW1lbmQg dGhpcywgSSBjYW4gbW92ZSB0aGlzIHBhcnQgaW4gbmV4dCBwYXRjaC4KPj4gZGlmZiAtLWdpdCBh L2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2hkbWkuYyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1 L2ludGVsX2hkbWkuYwo+PiBpbmRleCBlMmRhYjQ4Li5iYzhlMmM4IDEwMDY0NAo+PiAtLS0gYS9k cml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9oZG1pLmMKPj4gKysrIGIvZHJpdmVycy9ncHUvZHJt L2k5MTUvaW50ZWxfaGRtaS5jCj4+IEBAIC0xNTQ1LDYgKzE1NDUsMTIgQEAgaW50ZWxfaGRtaV9z ZXRfcHJvcGVydHkoc3RydWN0IGRybV9jb25uZWN0b3IgKmNvbm5lY3RvciwKPj4gICAJCWNhc2Ug RFJNX01PREVfUElDVFVSRV9BU1BFQ1RfMTZfOToKPj4gICAJCQlpbnRlbF9oZG1pLT5hc3BlY3Rf cmF0aW8gPSBIRE1JX1BJQ1RVUkVfQVNQRUNUXzE2Xzk7Cj4+ICAgCQkJYnJlYWs7Cj4+ICsJCWNh c2UgRFJNX01PREVfUElDVFVSRV9BU1BFQ1RfNjRfMjc6Cj4+ICsJCQlpbnRlbF9oZG1pLT5hc3Bl Y3RfcmF0aW8gPSBIRE1JX1BJQ1RVUkVfQVNQRUNUXzY0XzI3Owo+PiArCQkJYnJlYWs7Cj4+ICsJ CWNhc2UgRFJNX01PREVfUElDVFVSRV9BU1BFQ1RfMjU2XzEzNToKPj4gKwkJCWludGVsX2hkbWkt PmFzcGVjdF9yYXRpbyA9IEhETUlfUElDVFVSRV9BU1BFQ1RfMjU2XzEzNTsKPj4gKwkJCWJyZWFr Owo+PiAgIAkJZGVmYXVsdDoKPj4gICAJCQlyZXR1cm4gLUVJTlZBTDsKPj4gICAJCX0KPj4gZGlm ZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX3Nkdm8uYyBiL2RyaXZlcnMvZ3B1 L2RybS9pOTE1L2ludGVsX3Nkdm8uYwo+PiBpbmRleCBmYWU2NGJjLi4zNzBlNGY5IDEwMDY0NAo+ PiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9zZHZvLmMKPj4gKysrIGIvZHJpdmVy cy9ncHUvZHJtL2k5MTUvaW50ZWxfc2R2by5jCj4+IEBAIC0yMDcxLDYgKzIwNzEsMTIgQEAgaW50 ZWxfc2R2b19zZXRfcHJvcGVydHkoc3RydWN0IGRybV9jb25uZWN0b3IgKmNvbm5lY3RvciwKPj4g ICAJCWNhc2UgRFJNX01PREVfUElDVFVSRV9BU1BFQ1RfMTZfOToKPj4gICAJCQlpbnRlbF9zZHZv LT5hc3BlY3RfcmF0aW8gPSBIRE1JX1BJQ1RVUkVfQVNQRUNUXzE2Xzk7Cj4+ICAgCQkJYnJlYWs7 Cj4+ICsJCWNhc2UgRFJNX01PREVfUElDVFVSRV9BU1BFQ1RfNjRfMjc6Cj4+ICsJCQlpbnRlbF9z ZHZvLT5hc3BlY3RfcmF0aW8gPSBIRE1JX1BJQ1RVUkVfQVNQRUNUXzY0XzI3Owo+PiArCQkJYnJl YWs7Cj4+ICsJCWNhc2UgRFJNX01PREVfUElDVFVSRV9BU1BFQ1RfMjU2XzEzNToKPj4gKwkJCWlu dGVsX3Nkdm8tPmFzcGVjdF9yYXRpbyA9IEhETUlfUElDVFVSRV9BU1BFQ1RfMjU2XzEzNTsKPj4g KwkJCWJyZWFrOwo+PiAgIAkJZGVmYXVsdDoKPj4gICAJCQlyZXR1cm4gLUVJTlZBTDsKPj4gICAJ CX0KPgo+IFRoZSB0cm91YmxlIHdpdGggdGhlIGFzcGVjdF9yYXRpbyBwcm9wIGFzIGN1cnJlbnRs eSBpbXBsZW1lbnRlZCBpcyB0aGF0IHdlCj4gY3VycmVudGx5IHVuY29uZGl0aW9uYWxseSBvdmVy d3JpdGUgYWRqdXN0ZWRfbW9kZS0+cGljdHVyZV9hc3BlY3RfcmF0aW8KPiB3aXRoIGl0Lgo+Cj4g QnV0IHlvdXIgcGF0Y2hlcyBoZXJlIG1vdmUgdGhlIGFzcGVjdCByYXRpbyBoYW5kbGluZyBpbnRv Cj4gZHJtX21vZGVfbW9kZWluZm8gKHVhYmkpIGFuZCBkcm1fZGlzcGxheV9tb2RlIChrZXJuZWwt aW50ZXJuYWwpLCB3aGVyZSBpdAo+IGltbyBiZWxvbmdzLiBCdXQgd2UgbmVlZCB0byBmaWd1cmUg b3V0IGhvdyB0byB0aGUgaW50ZXJhY3Rpb24gd2l0aCB0aGUKPiBwcm9wZXJ0eSBjb3JyZWN0bHku IFdoYXQncyBwcm9iYWJseSBuZWVkZWQgaXMgYSBuZXcgdmFsdWUgaW4gdGhlCj4gYXNwZWN0X3Jh dGlvIGVudW0gY2FsbGVkICJkZWZhdWx0Iiwgd2hpY2ggc2VsZWN0cyB0aGUgYXNwZWN0X3JhdGlv IGZyb20KPiB0aGUgbW9kZS4gT25seSB3aGVuIHVzZXJzcGFjZSBvdmVycmlkZXMgKE5PTkUsIG9y IG9uZSBvZiB0aGUgQ0VBIGFzcGVjdAo+IHJhdGlvcykgd291bGQgd2Ugb2JleSB0aGUgYXNwZWN0 X3JhdGlvIG9mIHRoZSBwcm9wZXJ0eS4gT3RoZXJ3aXNlIHRoZQo+IGFzcGVjdCByYXRpbyBzdG9y ZWQgaW4gdGhlIG1vZGUgd291bGQgYmUgbG9zdC4KVGhpcyBpcyBleGFjdGx5IGhvdyB3ZSBoYXZl IGhhbmRsZWQgdGhpcyBpbiBBbmRyb2lkIGJyYW5jaCh3aXRoIE5PTkUgYXMgCmRlZmF1bHQpLCB0 aGFua3MgZm9yIHRoZSBzdWdnZXN0aW9uLiBJIHdpbGwgaW5jb3Jwb3JhdGUgdGhpcyBpbiBuZXh0 IApwYXRjaCBzZXQuCj4KPiBUaGUgb3RoZXIgYml0IHRoYXQgSSBjYW4ndCBmaW5kIGluIHRoaXMg c2VyaWVzIGlzIG1ha2luZyBzdXJlIHdlIGNvbXB1dGUKPiB0aGUgVklDIGNvcnJlY3RseSBmb3Ig dGhlIG5ldyBtb2Rlcy4gT3IgZG9lcyB0aGF0IG1hZ2ljYWxseSB3b3JrIGNvcnJlY3RseQo+IHNp bmNlIHdlIGNvbXBhcmUgdGhlIGZ1bGwgbW9kZSB3aGVuIHNlbGVjdGluZyBWSUNzPwo+Clllcywg d2UgYXJlIHNhdmluZyB0aGUgcmlnaHQgVklDIGZyb20gRURJRCwgc28gdGhlIFZJQyBpcyBub3cg YSBwYXJ0IG9mIAp0aGUgbW9kZSBmbGFncywgYWxsIHdlIGhhdmUgdG8gZG8gZXh0cmFjdCB0aGlz IGFuZCB3cml0ZSBkdXJpbmcgCnByZXBhcmluZyBBVkktSUYsIHdlIGhhdmUgYSBzbWFsbCBvbmUg bGluZSBwYXRjaCBmb3IgdGhhdC4gSSB3aWxsIGFkZCAKdGhhdCB0b28gaW4gdGhlIG5leHQgc2Vy aWVzLgo+IFRoYW5rcywgRGFuaWVsCj4KX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX18KSW50ZWwtZ2Z4IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJl ZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGlu Zm8vaW50ZWwtZ2Z4Cg==