From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 2/2] drm/panel: Add driver for Sony ACX424AKP panel Date: Mon, 2 Sep 2019 16:32:57 +0200 Message-ID: <20190902143257.GA1445@ulmo> References: <20190902090633.24239-1-linus.walleij@linaro.org> <20190902090633.24239-2-linus.walleij@linaro.org> <20190902103219.GB12946@ulmo> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1650343941==" Return-path: Received: from mail-wr1-x441.google.com (mail-wr1-x441.google.com [IPv6:2a00:1450:4864:20::441]) by gabe.freedesktop.org (Postfix) with ESMTPS id DDB7789AAD for ; Mon, 2 Sep 2019 14:33:01 +0000 (UTC) Received: by mail-wr1-x441.google.com with SMTP id g7so14288279wrx.2 for ; Mon, 02 Sep 2019 07:33:01 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Linus Walleij Cc: Sam Ravnborg , "open list:DRM PANEL DRIVERS" List-Id: dri-devel@lists.freedesktop.org --===============1650343941== Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="vtzGhvizbBRQ85DL" Content-Disposition: inline --vtzGhvizbBRQ85DL Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Sep 02, 2019 at 02:57:25PM +0200, Linus Walleij wrote: > On Mon, Sep 2, 2019 at 12:32 PM Thierry Reding = wrote: > > On Mon, Sep 02, 2019 at 11:06:33AM +0200, Linus Walleij wrote: >=20 > > > + /* > > > + * This depends on the clocking HS vs LP rate, this value > > > + * is calculated as: > > > + * vrefresh =3D (clock * 1000) / (htotal*vtotal) > > > + */ > > > + .vrefresh =3D 816, > > > > That's a bit odd. My understanding is that command mode can be done in > > continuous mode or in non-continuous mode. In continuous mode you would > > typically achieve a similar refresh rate as in video mode. For non- > > continuous mode you basically have a variable refresh rate. > > > > For continuous mode you probably want 60 Hz here and for VRR there's > > probably no sensible value to set this to. In the latter case, I don't > > think it makes sense to set anything arbitrary like you have above. > > Perhaps better to just set it to 0 as a way of signalling that this is > > actually dependent on when the display hardware sends a new frame? >=20 > I guess I should set it to 60. Not sure, perhaps someone on the list has a good idea of what vrefresh is set for VRR monitors. I think that'd be a good example to follow. I'm also not sure your formula to compute the refresh rate is good for command mode. You're assuming one pixel per clock cycle. I don't think that's accurate. Shouldn't that at least depend also on the number of lanes and the pixel format? 816 frames per second also seems a bit much for any type of panel. > > Have you actually run this is command mode? >=20 > Yes that is what I am primarily using. >=20 > > If so, what's the actual > > refresh rate? Do you do on-demand updates or do you run in continuous > > mode? >=20 > I run continuous mode, so the refresh rate is essentially dependent on > the HS frequency that the host uses. For command mode we use as > fast as it can go which is 420MHz, backwards calculated to ~816Hz > updates. >=20 > I took some data from the system when running: > 175608 "vblank" interrupts in 25 minutes, yielding > 175608/(25*60) ~=3D 117 Hz so I guess that is the pace the > hardware actually recieves it and triggers new updates. That's a factor of almost 8, so I think there's something really wrong in your refresh rate calculation, or you have an issue somewhere in the code that computes the pixel clock and byte clock from the mode's clock rate. >=20 > > > + ret =3D mipi_dsi_dcs_read(dsi, 0xda, &id1, len); > > > + if (ret < 0) { > > > + DRM_DEV_ERROR(acx->dev, "could not read device ID byte = 1\n"); > > > + return ret; > > > + } > > > + ret =3D mipi_dsi_dcs_read(dsi, 0xdb, &id2, len); > > > + if (ret < 0) { > > > + DRM_DEV_ERROR(acx->dev, "could not read device ID byte = 2\n"); > > > + return ret; > > > + } > > > + ret =3D mipi_dsi_dcs_read(dsi, 0xdc, &id3, len); > > > + if (ret < 0) { > > > + DRM_DEV_ERROR(acx->dev, "could not read device ID byte = 3\n"); > > > + return ret; > > > + } > > > + > > > + val =3D (id3 << 8) | id2; > > > > Don't you want to OR in id1 here as well? Seems a bit odd to read it but > > then not use it. >=20 > The way I have understood these "MTP IDs" is that the first byte > should just be checked for not being 0 so I will add a check like that. >=20 > The only other DSI panel doing this is panel-samsung-s6e8aa0.c function > s6e8aa0_read_mtp_id() which also reads three bytes and ignores the > first byte, also the second byte being version and the third ID matches > the data this display returns. >=20 > I'll try to make it a bit clearer and inspect the individual bytes since = they > seem to have individual meanings. I vaguely recall MTP IDs. It's a little funny that both S6E8AA0 and this new driver seem to be reading the same type of ID, but using different DCS register offsets. I had hoped that these would be somewhat standardized. Or perhaps these are standardized as part of a larger type of descriptor to which an offset is read from somewhere else? > > > + struct device *dev =3D &dsi->dev; > > > + struct acx424akp *acx; > > > + int ret; > > > + int i; > > > > unsigned int? >=20 > Just following the common idiom for integer enumerator i to be a > plain int but sure, if you prefer :) The common idiom is wrong. =3D) Jokes aside, why worry about the sign if you know up front that your values are never going to be negative? If you consistently use unsigned types for values that can never be negative, the compiler will even be able to point out inconsistent usage, etc. Thierry > > > + /* Read device ID */ > > > + i =3D 0; > > > + do { > > > + ret =3D acx424akp_read_id(acx); > > > + if (ret) > > > + continue; > > > + } while (ret && i++ < 5); > > > > Seems rather redundant to have both an "if (ret) continue;" and the ret > > check in the while's condition. A more idiomatic way to write this would > > be: > > > > for (i =3D 0; i < 5; i++) { > > ret =3D acx424akp_read_id(acx); > > if (!ret) > > break; > > } >=20 > OK! I fix. >=20 > Yours, > Linus Walleij --vtzGhvizbBRQ85DL Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQIzBAABCAAdFiEEiOrDCAFJzPfAjcif3SOs138+s6EFAl1tKBYACgkQ3SOs138+ s6GblA/9G5A/H4E0Ffu/OzVu+KObo4Rm7Pby4aneyNHtqu6HrUUZz59YdY1MWKlu Vaqc8YHOsHD4FSz36cqVTJR+N0YNz9smjdurPS1z7tfBve9Ws3jajHqDpvFd1j2d Wcmjq189NsevlDcOsIt7W1M2lxcTjp55inNifY1KT8XhtS/iVg0zeP7tb6WXIBwA F2an8S8gsc4KrWavTTo2WC572nnvzQZAPJTbVGiXYUeBKKyyaSl2HJ1HfOl95laq avpLqaKZIywqEnvt5p1RHALTTwaHT3FQ4hzPjID+R+WMhWVg4g6Bf6xwQCf8mRKF DurEiaiugSHsF5G9BM3DcvArLb+NiO8SdNzQ+abr+i4727T+lj9KwZhv6sYoZf9f v0iRxHqwONs5vH+6HPtuDelrqNJHa63Vs0ElEFqefyFS8BD12jDe/UHVxAFrQx4O LVOSBh6Pg6GakrzWDkTYS+ai0TTT/Qci01FMWcMVP/OwbffXh+ZaMP16TlHS2rmw ecBpPZ6sR41IbL8LF01IKJNk7Qy4LblC9J6ssUbcWwuZjsJn+E88AHJ7pHwEDd7G lOdWLeXh07ybO8dY9rt0ZyDVM5v5i2Ov48Yf+oSoxmSx+3JPpIgBU8WeJi3MCRPx F8v/WgBdaOywA5GF2H4cfIveVeoaRKrQAoFj2KF/+aFdokhvU1c= =do+2 -----END PGP SIGNATURE----- --vtzGhvizbBRQ85DL-- --===============1650343941== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: base64 Content-Disposition: inline X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVs IG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVs --===============1650343941==--