From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid() Date: Tue, 29 Sep 2020 21:09:12 +0300 Message-ID: <20200929180912.GG6112@intel.com> References: <20200922210510.156220-1-lyude@redhat.com> <20200928130141.GV6112@intel.com> <9e12d6c091d18be6253717f33f4c09013361e532.camel@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <9e12d6c091d18be6253717f33f4c09013361e532.camel@redhat.com> To: Lyude Paul Cc: nouveau@lists.freedesktop.org, Ben Skeggs , David Airlie , Daniel Vetter , "open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS" , open list List-Id: nouveau.vger.kernel.org On Tue, Sep 29, 2020 at 01:54:13PM -0400, Lyude Paul wrote: > On Mon, 2020-09-28 at 16:01 +0300, Ville Syrjälä wrote: > > On Tue, Sep 22, 2020 at 05:05:10PM -0400, Lyude Paul wrote: > > > While I thought I had this correct (since it actually did reject modes > > > like I expected during testing), Ville Syrjala from Intel pointed out > > > that the logic here isn't correct. max_clock refers to the max symbol > > > rate supported by the encoder, so limiting clock to ds_clock using max() > > > doesn't make sense. Additionally, we want to check against 6bpc for the > > > time being since that's the minimum possible bpc here, not the reported > > > bpc from the connector. See: > > > > > > https://lists.freedesktop.org/archives/dri-devel/2020-September/280276.html > > > > > > For more info. > > > > > > So, let's rewrite this using Ville's advice. > > > > > > Signed-off-by: Lyude Paul > > > Fixes: 409d38139b42 ("drm/nouveau/kms/nv50-: Use downstream DP clock > > > limits for mode validation") > > > Cc: Ville Syrjälä > > > Cc: Lyude Paul > > > Cc: Ben Skeggs > > > --- > > > drivers/gpu/drm/nouveau/nouveau_dp.c | 23 +++++++++++++---------- > > > 1 file changed, 13 insertions(+), 10 deletions(-) > > > > > > diff --git a/drivers/gpu/drm/nouveau/nouveau_dp.c > > > b/drivers/gpu/drm/nouveau/nouveau_dp.c > > > index 7b640e05bd4cd..24c81e423d349 100644 > > > --- a/drivers/gpu/drm/nouveau/nouveau_dp.c > > > +++ b/drivers/gpu/drm/nouveau/nouveau_dp.c > > > @@ -231,23 +231,26 @@ nv50_dp_mode_valid(struct drm_connector *connector, > > > const struct drm_display_mode *mode, > > > unsigned *out_clock) > > > { > > > - const unsigned min_clock = 25000; > > > - unsigned max_clock, ds_clock, clock; > > > + const unsigned int min_clock = 25000; > > > + unsigned int max_clock, ds_clock, clock; > > > + const u8 bpp = 18; /* 6 bpc */ > > > > AFAICS nv50_outp_atomic_check() and nv50_msto_atomic_check() > > just blindly use connector->display_info.bpc without any fallback > > logic to lower the bpc. So Ilia's concerns seem well founded. > > Without that logic I guess you should just use > > connector->display_info.bpc here as well. > > > > > enum drm_mode_status ret; > > > > > > if (mode->flags & DRM_MODE_FLAG_INTERLACE && !outp->caps.dp_interlace) > > > return MODE_NO_INTERLACE; > > > > > > max_clock = outp->dp.link_nr * outp->dp.link_bw; > > > - ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd, > > > - outp->dp.downstream_ports); > > > - if (ds_clock) > > > - max_clock = min(max_clock, ds_clock); > > > - > > > - clock = mode->clock * (connector->display_info.bpc * 3) / 10; > > > - ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock, > > > - &clock); > > > + clock = mode->clock * bpp / 8; > > > + if (clock > max_clock) > > > + return MODE_CLOCK_HIGH; > > > > This stuff vs. nouveau_conn_mode_clock_valid() still seems a bit messy. > > The max_clock you pass to nouveau_conn_mode_clock_valid() is the max > > symbol clock, but nouveau_conn_mode_clock_valid() checks it against the > > dotclock. Also only nouveau_conn_mode_clock_valid() has any kind of > > stereo 3D handling, but AFAICS stereo_allowed is also set for DP? > > ...not sure I'm following you here, it's set to true for DP so don't we want > to check it and adjust the pixel clock we output accordingly? Yes, but then you need to also double your your pixel clock derived values in this function. Ie. all the mode->clock needs to become mode->clock*2 when dealing with a 3D frame packing mode. > > > > > > + > > > + ds_clock = drm_dp_downstream_max_dotclock(outp->dp.dpcd, outp- > > > >dp.downstream_ports); > > > + if (ds_clock && mode->clock > ds_clock) > > > + return MODE_CLOCK_HIGH; > > > + > > > + ret = nouveau_conn_mode_clock_valid(mode, min_clock, max_clock, > > > &clock); > > > if (out_clock) > > > *out_clock = clock; > > > + > > > return ret; > > > } > > > -- > > > 2.26.2 > -- > Cheers, > Lyude Paul (she/her) > Software Engineer at Red Hat -- Ville Syrjälä Intel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-11.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,SIGNED_OFF_BY, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 620F3C4741F for ; Tue, 29 Sep 2020 18:09:22 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DDE3B21734 for ; Tue, 29 Sep 2020 18:09:21 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DDE3B21734 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 0B605898CC; Tue, 29 Sep 2020 18:09:21 +0000 (UTC) Received: from mga03.intel.com (mga03.intel.com [134.134.136.65]) by gabe.freedesktop.org (Postfix) with ESMTPS id 087FC898CC; Tue, 29 Sep 2020 18:09:20 +0000 (UTC) IronPort-SDR: /dZ5fI0qgyyU/cxg+qTpsa6o7+9AXwXGO3KJ/dpQMe6/40dXCuiGwq0JU6Frtb9CwM1bdwp0Wx yorRRPVaaKng== X-IronPort-AV: E=McAfee;i="6000,8403,9759"; a="162317555" X-IronPort-AV: E=Sophos;i="5.77,319,1596524400"; d="scan'208";a="162317555" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga006.jf.intel.com ([10.7.209.51]) by orsmga103.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 29 Sep 2020 11:09:17 -0700 IronPort-SDR: oSrUocykxI+vYQHmlxEf0u8Z1n658awzYDGwpTU9TobnVjIMvOoEtyeFNAlMTdDv5VB3Q0oi3R Q7gk/VA+gG6A== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.77,319,1596524400"; d="scan'208";a="312304493" Received: from stinkbox.fi.intel.com (HELO stinkbox) ([10.237.72.174]) by orsmga006.jf.intel.com with SMTP; 29 Sep 2020 11:09:13 -0700 Received: by stinkbox (sSMTP sendmail emulation); Tue, 29 Sep 2020 21:09:12 +0300 Date: Tue, 29 Sep 2020 21:09:12 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Lyude Paul Subject: Re: [PATCH] drm/nouveau/kms/nv50-: Fix clock checking algorithm in nv50_dp_mode_valid() Message-ID: <20200929180912.GG6112@intel.com> References: <20200922210510.156220-1-lyude@redhat.com> <20200928130141.GV6112@intel.com> <9e12d6c091d18be6253717f33f4c09013361e532.camel@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <9e12d6c091d18be6253717f33f4c09013361e532.camel@redhat.com> X-Patchwork-Hint: comment User-Agent: Mutt/1.10.1 (2018-07-13) X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: David Airlie , nouveau@lists.freedesktop.org, open list , "open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS" , Ben Skeggs Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" T24gVHVlLCBTZXAgMjksIDIwMjAgYXQgMDE6NTQ6MTNQTSAtMDQwMCwgTHl1ZGUgUGF1bCB3cm90 ZToKPiBPbiBNb24sIDIwMjAtMDktMjggYXQgMTY6MDEgKzAzMDAsIFZpbGxlIFN5cmrDpGzDpCB3 cm90ZToKPiA+IE9uIFR1ZSwgU2VwIDIyLCAyMDIwIGF0IDA1OjA1OjEwUE0gLTA0MDAsIEx5dWRl IFBhdWwgd3JvdGU6Cj4gPiA+IFdoaWxlIEkgdGhvdWdodCBJIGhhZCB0aGlzIGNvcnJlY3QgKHNp bmNlIGl0IGFjdHVhbGx5IGRpZCByZWplY3QgbW9kZXMKPiA+ID4gbGlrZSBJIGV4cGVjdGVkIGR1 cmluZyB0ZXN0aW5nKSwgVmlsbGUgU3lyamFsYSBmcm9tIEludGVsIHBvaW50ZWQgb3V0Cj4gPiA+ IHRoYXQgdGhlIGxvZ2ljIGhlcmUgaXNuJ3QgY29ycmVjdC4gbWF4X2Nsb2NrIHJlZmVycyB0byB0 aGUgbWF4IHN5bWJvbAo+ID4gPiByYXRlIHN1cHBvcnRlZCBieSB0aGUgZW5jb2Rlciwgc28gbGlt aXRpbmcgY2xvY2sgdG8gZHNfY2xvY2sgdXNpbmcgbWF4KCkKPiA+ID4gZG9lc24ndCBtYWtlIHNl bnNlLiBBZGRpdGlvbmFsbHksIHdlIHdhbnQgdG8gY2hlY2sgYWdhaW5zdCA2YnBjIGZvciB0aGUK PiA+ID4gdGltZSBiZWluZyBzaW5jZSB0aGF0J3MgdGhlIG1pbmltdW0gcG9zc2libGUgYnBjIGhl cmUsIG5vdCB0aGUgcmVwb3J0ZWQKPiA+ID4gYnBjIGZyb20gdGhlIGNvbm5lY3Rvci4gU2VlOgo+ ID4gPiAKPiA+ID4gaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvYXJjaGl2ZXMvZHJpLWRl dmVsLzIwMjAtU2VwdGVtYmVyLzI4MDI3Ni5odG1sCj4gPiA+IAo+ID4gPiBGb3IgbW9yZSBpbmZv Lgo+ID4gPiAKPiA+ID4gU28sIGxldCdzIHJld3JpdGUgdGhpcyB1c2luZyBWaWxsZSdzIGFkdmlj ZS4KPiA+ID4gCj4gPiA+IFNpZ25lZC1vZmYtYnk6IEx5dWRlIFBhdWwgPGx5dWRlQHJlZGhhdC5j b20+Cj4gPiA+IEZpeGVzOiA0MDlkMzgxMzliNDIgKCJkcm0vbm91dmVhdS9rbXMvbnY1MC06IFVz ZSBkb3duc3RyZWFtIERQIGNsb2NrCj4gPiA+IGxpbWl0cyBmb3IgbW9kZSB2YWxpZGF0aW9uIikK PiA+ID4gQ2M6IFZpbGxlIFN5cmrDg8aSw4LCpGzDg8aSw4LCpCA8dmlsbGUuc3lyamFsYUBsaW51 eC5pbnRlbC5jb20+Cj4gPiA+IENjOiBMeXVkZSBQYXVsIDxseXVkZUByZWRoYXQuY29tPgo+ID4g PiBDYzogQmVuIFNrZWdncyA8YnNrZWdnc0ByZWRoYXQuY29tPgo+ID4gPiAtLS0KPiA+ID4gIGRy aXZlcnMvZ3B1L2RybS9ub3V2ZWF1L25vdXZlYXVfZHAuYyB8IDIzICsrKysrKysrKysrKystLS0t LS0tLS0tCj4gPiA+ICAxIGZpbGUgY2hhbmdlZCwgMTMgaW5zZXJ0aW9ucygrKSwgMTAgZGVsZXRp b25zKC0pCj4gPiA+IAo+ID4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL25vdXZlYXUv bm91dmVhdV9kcC5jCj4gPiA+IGIvZHJpdmVycy9ncHUvZHJtL25vdXZlYXUvbm91dmVhdV9kcC5j Cj4gPiA+IGluZGV4IDdiNjQwZTA1YmQ0Y2QuLjI0YzgxZTQyM2QzNDkgMTAwNjQ0Cj4gPiA+IC0t LSBhL2RyaXZlcnMvZ3B1L2RybS9ub3V2ZWF1L25vdXZlYXVfZHAuYwo+ID4gPiArKysgYi9kcml2 ZXJzL2dwdS9kcm0vbm91dmVhdS9ub3V2ZWF1X2RwLmMKPiA+ID4gQEAgLTIzMSwyMyArMjMxLDI2 IEBAIG52NTBfZHBfbW9kZV92YWxpZChzdHJ1Y3QgZHJtX2Nvbm5lY3RvciAqY29ubmVjdG9yLAo+ ID4gPiAgCQkgICBjb25zdCBzdHJ1Y3QgZHJtX2Rpc3BsYXlfbW9kZSAqbW9kZSwKPiA+ID4gIAkJ ICAgdW5zaWduZWQgKm91dF9jbG9jaykKPiA+ID4gIHsKPiA+ID4gLQljb25zdCB1bnNpZ25lZCBt aW5fY2xvY2sgPSAyNTAwMDsKPiA+ID4gLQl1bnNpZ25lZCBtYXhfY2xvY2ssIGRzX2Nsb2NrLCBj bG9jazsKPiA+ID4gKwljb25zdCB1bnNpZ25lZCBpbnQgbWluX2Nsb2NrID0gMjUwMDA7Cj4gPiA+ ICsJdW5zaWduZWQgaW50IG1heF9jbG9jaywgZHNfY2xvY2ssIGNsb2NrOwo+ID4gPiArCWNvbnN0 IHU4IGJwcCA9IDE4OyAvKiA2IGJwYyAqLwo+ID4gCj4gPiBBRkFJQ1MgbnY1MF9vdXRwX2F0b21p Y19jaGVjaygpIGFuZCBudjUwX21zdG9fYXRvbWljX2NoZWNrKCkKPiA+IGp1c3QgYmxpbmRseSB1 c2UgY29ubmVjdG9yLT5kaXNwbGF5X2luZm8uYnBjIHdpdGhvdXQgYW55IGZhbGxiYWNrCj4gPiBs b2dpYyB0byBsb3dlciB0aGUgYnBjLiBTbyBJbGlhJ3MgY29uY2VybnMgc2VlbSB3ZWxsIGZvdW5k ZWQuCj4gPiBXaXRob3V0IHRoYXQgbG9naWMgSSBndWVzcyB5b3Ugc2hvdWxkIGp1c3QgdXNlCj4g PiBjb25uZWN0b3ItPmRpc3BsYXlfaW5mby5icGMgaGVyZSBhcyB3ZWxsLgo+ID4gCj4gPiA+ICAJ ZW51bSBkcm1fbW9kZV9zdGF0dXMgcmV0Owo+ID4gPiAgCj4gPiA+ICAJaWYgKG1vZGUtPmZsYWdz ICYgRFJNX01PREVfRkxBR19JTlRFUkxBQ0UgJiYgIW91dHAtPmNhcHMuZHBfaW50ZXJsYWNlKQo+ ID4gPiAgCQlyZXR1cm4gTU9ERV9OT19JTlRFUkxBQ0U7Cj4gPiA+ICAKPiA+ID4gIAltYXhfY2xv Y2sgPSBvdXRwLT5kcC5saW5rX25yICogb3V0cC0+ZHAubGlua19idzsKPiA+ID4gLQlkc19jbG9j ayA9IGRybV9kcF9kb3duc3RyZWFtX21heF9kb3RjbG9jayhvdXRwLT5kcC5kcGNkLAo+ID4gPiAt CQkJCQkJICBvdXRwLT5kcC5kb3duc3RyZWFtX3BvcnRzKTsKPiA+ID4gLQlpZiAoZHNfY2xvY2sp Cj4gPiA+IC0JCW1heF9jbG9jayA9IG1pbihtYXhfY2xvY2ssIGRzX2Nsb2NrKTsKPiA+ID4gLQo+ ID4gPiAtCWNsb2NrID0gbW9kZS0+Y2xvY2sgKiAoY29ubmVjdG9yLT5kaXNwbGF5X2luZm8uYnBj ICogMykgLyAxMDsKPiA+ID4gLQlyZXQgPSBub3V2ZWF1X2Nvbm5fbW9kZV9jbG9ja192YWxpZCht b2RlLCBtaW5fY2xvY2ssIG1heF9jbG9jaywKPiA+ID4gLQkJCQkJICAgICZjbG9jayk7Cj4gPiA+ ICsJY2xvY2sgPSBtb2RlLT5jbG9jayAqIGJwcCAvIDg7Cj4gPiA+ICsJaWYgKGNsb2NrID4gbWF4 X2Nsb2NrKQo+ID4gPiArCQlyZXR1cm4gTU9ERV9DTE9DS19ISUdIOwo+ID4gCj4gPiBUaGlzIHN0 dWZmIHZzLiBub3V2ZWF1X2Nvbm5fbW9kZV9jbG9ja192YWxpZCgpIHN0aWxsIHNlZW1zIGEgYml0 IG1lc3N5Lgo+ID4gVGhlIG1heF9jbG9jayB5b3UgcGFzcyB0byBub3V2ZWF1X2Nvbm5fbW9kZV9j bG9ja192YWxpZCgpIGlzIHRoZSBtYXgKPiA+IHN5bWJvbCBjbG9jaywgYnV0IG5vdXZlYXVfY29u bl9tb2RlX2Nsb2NrX3ZhbGlkKCkgY2hlY2tzIGl0IGFnYWluc3QgdGhlCj4gPiBkb3RjbG9jay4g QWxzbyBvbmx5IG5vdXZlYXVfY29ubl9tb2RlX2Nsb2NrX3ZhbGlkKCkgaGFzIGFueSBraW5kIG9m Cj4gPiBzdGVyZW8gM0QgaGFuZGxpbmcsIGJ1dCBBRkFJQ1Mgc3RlcmVvX2FsbG93ZWQgaXMgYWxz byBzZXQgZm9yIERQPwo+IAo+IC4uLm5vdCBzdXJlIEknbSBmb2xsb3dpbmcgeW91IGhlcmUsIGl0 J3Mgc2V0IHRvIHRydWUgZm9yIERQIHNvIGRvbid0IHdlIHdhbnQKPiB0byBjaGVjayBpdCBhbmQg YWRqdXN0IHRoZSBwaXhlbCBjbG9jayB3ZSBvdXRwdXQgYWNjb3JkaW5nbHk/CgpZZXMsIGJ1dCB0 aGVuIHlvdSBuZWVkIHRvIGFsc28gZG91YmxlIHlvdXIgeW91ciBwaXhlbCBjbG9jawpkZXJpdmVk IHZhbHVlcyBpbiB0aGlzIGZ1bmN0aW9uLiBJZS4gYWxsIHRoZSBtb2RlLT5jbG9jawpuZWVkcyB0 byBiZWNvbWUgbW9kZS0+Y2xvY2sqMiB3aGVuIGRlYWxpbmcgd2l0aCBhIDNEIGZyYW1lCnBhY2tp bmcgbW9kZS4KCj4gCj4gPiAKPiA+ID4gKwo+ID4gPiArCWRzX2Nsb2NrID0gZHJtX2RwX2Rvd25z dHJlYW1fbWF4X2RvdGNsb2NrKG91dHAtPmRwLmRwY2QsIG91dHAtCj4gPiA+ID5kcC5kb3duc3Ry ZWFtX3BvcnRzKTsKPiA+ID4gKwlpZiAoZHNfY2xvY2sgJiYgbW9kZS0+Y2xvY2sgPiBkc19jbG9j aykKPiA+ID4gKwkJcmV0dXJuIE1PREVfQ0xPQ0tfSElHSDsKPiA+ID4gKwo+ID4gPiArCXJldCA9 IG5vdXZlYXVfY29ubl9tb2RlX2Nsb2NrX3ZhbGlkKG1vZGUsIG1pbl9jbG9jaywgbWF4X2Nsb2Nr LAo+ID4gPiAmY2xvY2spOwo+ID4gPiAgCWlmIChvdXRfY2xvY2spCj4gPiA+ICAJCSpvdXRfY2xv Y2sgPSBjbG9jazsKPiA+ID4gKwo+ID4gPiAgCXJldHVybiByZXQ7Cj4gPiA+ICB9Cj4gPiA+IC0t IAo+ID4gPiAyLjI2LjIKPiAtLSAKPiBDaGVlcnMsCj4gCUx5dWRlIFBhdWwgKHNoZS9oZXIpCj4g CVNvZnR3YXJlIEVuZ2luZWVyIGF0IFJlZCBIYXQKCi0tIApWaWxsZSBTeXJqw6Rsw6QKSW50ZWwK X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg==