From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thierry Reding Subject: Re: [PATCH 2/2] drm/i915/dp: check eDP display control capability registers Date: Mon, 18 Nov 2013 16:26:17 +0100 Message-ID: <20131118152616.GK26046@ulmo.nvidia.com> References: <1384520511-24267-1-git-send-email-jani.nikula@intel.com> <1384520511-24267-2-git-send-email-jani.nikula@intel.com> <20131118142755.GF26046@ulmo.nvidia.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============1858817348==" Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Alex Deucher Cc: Jani Nikula , Intel Graphics Development , Maling list - DRI developers List-Id: intel-gfx@lists.freedesktop.org --===============1858817348== Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="T4Djgzn3z2HSNnx0" Content-Disposition: inline --T4Djgzn3z2HSNnx0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Mon, Nov 18, 2013 at 10:09:56AM -0500, Alex Deucher wrote: > On Mon, Nov 18, 2013 at 9:27 AM, Thierry Reding > wrote: > > On Fri, Nov 15, 2013 at 03:01:51PM +0200, Jani Nikula wrote: > >> Debug print the capabilities, and flag an error if the panel does not > >> support adjusting backlight through the BL_PWM_DIM pin, requiring > >> backlight control through DPCD. > >> > >> I haven't seen such panels yet, but it's a matter of time. Give > >> ourselves a reminder when we need to fix this for real. > >> > >> Signed-off-by: Jani Nikula > >> --- > >> drivers/gpu/drm/i915/intel_dp.c | 14 ++++++++++++++ > >> 1 file changed, 14 insertions(+) > > > > I have a few general comments below, but this patch itself look fine, > > so: > > > > Reviewed-by: Thierry Reding > > > >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/in= tel_dp.c > >> index cbf33be..ea4f3d1 100644 > >> --- a/drivers/gpu/drm/i915/intel_dp.c > >> +++ b/drivers/gpu/drm/i915/intel_dp.c > >> @@ -2816,6 +2816,20 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp) > >> dev_priv->psr.sink_support =3D true; > >> DRM_DEBUG_KMS("Detected EDP PSR Panel.\n"); > >> } > >> + > >> + if (intel_dp->dpcd[DP_EDP_CONFIGURATION_CAP] & > >> + DP_DPCD_DISPLAY_CONTROL_CAP) { > >> + u8 ctrl[4] =3D { 0 }; > >> + > >> + intel_dp_aux_native_read(intel_dp, DP_EDP_REV, > >> + ctrl, sizeof(ctrl)); > >> + DRM_DEBUG_KMS("eDP DPCD CTRL %02x %02x %02x %02x= \n", > >> + ctrl[0], ctrl[1], ctrl[2], ctrl[3]= ); > >> + > >> + /* We don't support DPCD backlight control yet. = */ > >> + if (ctrl[0] && (ctrl[1] & 1) && !(ctrl[2] & 1)) > >> + DRM_ERROR("eDP AUX backlight control onl= y\n"); > >> + } > >> } > >> > >> if (!(intel_dp->dpcd[DP_DOWNSTREAMPORT_PRESENT] & > > > > I think a lot of eDP utility code could be made reusable across drivers. > > We could probably do that by having each driver expose a drm_edp object > > of some sort. > > > > Actually, the same would be true of DP in general. Accessing the DPCD is > > something that's driver specific, but once you know how to do that a lot > > of code can be made generic. I think a struct drm_dp could look like > > this: > > > > struct drm_dp; > > > > struct drm_dpcd_ops { > > ssize_t (*read)(struct drm_dp *dp, unsigned int offset, > > void *buffer, size_t size); > > ssize_t (*write)(struct drm_dp *dp, unsigned int offset, > > const void *buffer, size_t size); > > }; > > > > struct drm_dp { > > const struct drm_dpcd_ops *dpcd; > > }; > > > > Perhaps that could even be extended with functionality to implement link > > training in a generic way. There are already quite a few helpers to help > > with that in drivers/gpu/drm/drm_dp_helper.c, but they assume that the > > DPCD will be handed to them as a large buffer and therefore cannot write > > DPCD registers. > > > > I suppose one could argue that it would be introducing a mid-layer, but > > that layer would be really thin in my opinion. And it would allow a lot > > of the algorithms to be written only once instead of multiple times. >=20 > I think it could probably be made to work. The tricky part would be > hw specific ordering in the training sequence. At the very minimum, > you need driver callbacks to set up the source side: >=20 > set_training_pattern() > set_vs_emph() >=20 > And probably some flags to indicate whether the the hw supports > specific features like training pattern 3. Yes, something along those lines was what I had in mind as well. I know that many people are unhappy about introducing this kind of mid-layer, but quite frankly, doing this in generic code must have been one of the primary reasons why VESA specified it that way. The alternative will be to repeat more or less the same code in all the drivers. I don't think that's a very nice alternative. Thierry --T4Djgzn3z2HSNnx0 Content-Type: application/pgp-signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJSijGYAAoJEN0jrNd/PrOhXSsP/jgl7zCvEnWZsnZwvL6J3RQB 1kYgISB98ZN9O+3ltT/0nCJciwb8D15v2PX+I1G7fiNuC0X/wF052Gvq/IwzJ/yy f3sHT4OteNb66/9APMM1aVqhGTZTv2UB319ZLtLe1Vi+mxNgCgc4mUqao/7iE8lf M6D6QQSVfJPh3TLqudtq4I970hkfZDrzCIfQpimHxrD2hP4pevZvK4e4CvRYlKfK Yd2qYC1sAEG7Qo0MhX+al5pR8eIQSzVSJOSk+n2aBPSIuPNZ8M6ljO3DPbxVz+AE R0JW2YCSyZ/pjH2RC0gARLFZkKCm/TYhmpzsctLJ1IcMA+SaiRITorOOobJp2ApN 6dtj2+yM2C1VziteOAei9Bv9UB+xse1xyQe+X6AFu3Ic0vu4OPxDi8WPErM85z3I tgf7kC+F4UgDL/9yrNlhZOrmMGpfM6uhzCVs/tNtvDF0OuXti3c3PWDPMRwpiPBe 39QKVjZNlCM7BUt85lEuB6BIGJOGpZ+z8eZZL/Jgdaa0I2H9VXvzK7eEToKVwoNR 6OOmhD3o8E6rCbs5tIHe7IGyEuBcDLX2vKCXxhKzGroto+qDCKuwbl+88kLfgepz WMOIpuCh4APR/i01FY91/SX94w2QRiYNIwM8BCZUWaDK4zu90w9rocIb9rvMp6q2 WgzaMr7WIFpcu+WgnLo0 =qrnU -----END PGP SIGNATURE----- --T4Djgzn3z2HSNnx0-- --===============1858817348== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx --===============1858817348==--