Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required
On Fri, Jan 27, 2012 at 10:30 AM, Daniel Vetter dan...@ffwll.ch wrote: On Wed, Jan 25, 2012 at 08:16:25AM -0800, Keith Packard wrote: It is never correct to use intel_crtc-bpp in intel_dp_link_required, so instead pass an explicit bpp in to this function. This patch only supports 18bpp and 24bpp modes, which means that 10bpc modes will be computed incorrectly. Fixing that will require more extensive changes, and so must be addressed separately from this bugfix. intel_dp_link_required is called from intel_dp_mode_valid and intel_dp_mode_fixup. * intel_dp_mode_valid is called to list supported modes; in this case, the current crtc values cannot be relevant as the modes in question may never be selected. Thus, using intel_crtc-bpp is never right. * intel_dp_mode_fixup is called during mode setting, but it is run well before ironlake_crtc_mode_set is called to set intel_crtc-bpp, so using intel_crtc-bpp in this path can only ever get a stale value. Cc: Lubos Kolouch lubos.kolo...@gmail.com Cc: Adam Jackson a...@redhat.com Signed-off-by: Keith Packard kei...@keithp.com Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=42263 Tested-by: cama...@picnicpark.org (Dell Latitude 6510) Not the original reporter and might not exactly be this bug, but likely. -Daniel Tested-by: Dave Airlie airl...@redhat.com Without this patch the eDP panel on my HP 2540p won't resume. Please get this into -fixes and too me asap. Dave. ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required
On Mon, 6 Feb 2012 12:12:16 +, Dave Airlie airl...@gmail.com wrote: Please get this into -fixes and too me asap. It's in -fixes; will be on its way to you soon. This patch is sufficient to make machines work again; the second patch serves to improve things a bit, and (as such) should probably wait in drm-intel-next for another release. -- keith.pack...@intel.com pgpoNFHsIhzUz.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required
On Wed, 25 Jan 2012 08:16:25 -0800 Keith Packard kei...@keithp.com wrote: It is never correct to use intel_crtc-bpp in intel_dp_link_required, so instead pass an explicit bpp in to this function. This patch only supports 18bpp and 24bpp modes, which means that 10bpc modes will be computed incorrectly. Fixing that will require more extensive changes, and so must be addressed separately from this bugfix. intel_dp_link_required is called from intel_dp_mode_valid and intel_dp_mode_fixup. * intel_dp_mode_valid is called to list supported modes; in this case, the current crtc values cannot be relevant as the modes in question may never be selected. Thus, using intel_crtc-bpp is never right. * intel_dp_mode_fixup is called during mode setting, but it is run well before ironlake_crtc_mode_set is called to set intel_crtc-bpp, so using intel_crtc-bpp in this path can only ever get a stale value. Yeah, looks like I got this wrong when I added the pipe bpp field. Wonder if there's a good way to catch this sort of bug with an assert so we don't get the order mixed up again... The big downside here is that we'll be very pessimistic about the link bw requirements for say 16 or 8bpp modes. I see you have another patch to address some of this, but I wonder if we have enough info to calculate the bpp at prepare time so it's set early on for use by both the crtc and encoder code? -- Jesse Barnes, Intel Open Source Technology Center signature.asc Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required
On Wed, 25 Jan 2012 12:51:16 -0800, Jesse Barnes jbar...@virtuousgeek.org wrote: Yeah, looks like I got this wrong when I added the pipe bpp field. Wonder if there's a good way to catch this sort of bug with an assert so we don't get the order mixed up again... Ideally, we'd figure out a way to compute this only once. I think the only relevant piece of data here is what bpc the pipe should run at, and that can be computed at any time during the mode set. We'd then be able to compare the pipe bpc with the frame buffer bpp to decide when to dither in the crtc mode set code. The big downside here is that we'll be very pessimistic about the link bw requirements for say 16 or 8bpp modes. Right, with this patch, we'll choose link parameters capable of supporting 8bpc, even for 16bpp video modes. Note that 8bpp video modes need 8bpc links as they have colormaps with 8bpc data in them. I see you have another patch to address some of this, but I wonder if we have enough info to calculate the bpp at prepare time so it's set early on for use by both the crtc and encoder code? fixup gets exactly the same info as mode set, so it should end up with exactly the same resulting bpc, which will set the link bandwidth to support 6bpc mode if that's all that is needed. One problem with the patch is that the pre-PCH mode set path doesn't use intel_choose_pipe_bpp_dither, so the condition for 6bpc mode is duplicated in i9xx_crtc_mode_set and intel_dp_mode_fixup. If crtc-fixup was called before encoder-fixup, we could have computed bpp in the crtc-fixup function and then used it in encoder-fixup. Alternatively, crtc-fixup could call into the DP code to set the link_bw, lane_count and adjusted_mode-clock values after it sets the bpp, but that seems to break the abstraction even more. -- keith.pack...@intel.com pgpk4RZfQQ9fB.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required
On Wed, Jan 25, 2012 at 01:17:51PM -0800, Keith Packard wrote: On Wed, 25 Jan 2012 12:51:16 -0800, Jesse Barnes jbar...@virtuousgeek.org wrote: Yeah, looks like I got this wrong when I added the pipe bpp field. Wonder if there's a good way to catch this sort of bug with an assert so we don't get the order mixed up again... Ideally, we'd figure out a way to compute this only once. I think the only relevant piece of data here is what bpc the pipe should run at, and that can be computed at any time during the mode set. We'd then be able to compare the pipe bpc with the frame buffer bpp to decide when to dither in the crtc mode set code. I think we could compute this in crtc-mode_fixup (crtc-prepare doesn't have the mode and adjusted_mode arguments). We could then store the computed bpc and dithering in one of the private fields. We'd still have to loop over all encoders, but alas ... The big downside here is that we'll be very pessimistic about the link bw requirements for say 16 or 8bpp modes. Right, with this patch, we'll choose link parameters capable of supporting 8bpc, even for 16bpp video modes. Note that 8bpp video modes need 8bpc links as they have colormaps with 8bpc data in them. Afaics we'll still correctly fall back to 6bpc (undithered for 16bpp obviously) and hence things should keep on working. I see you have another patch to address some of this, but I wonder if we have enough info to calculate the bpp at prepare time so it's set early on for use by both the crtc and encoder code? fixup gets exactly the same info as mode set, so it should end up with exactly the same resulting bpc, which will set the link bandwidth to support 6bpc mode if that's all that is needed. One problem with the patch is that the pre-PCH mode set path doesn't use intel_choose_pipe_bpp_dither, so the condition for 6bpc mode is duplicated in i9xx_crtc_mode_set and intel_dp_mode_fixup. If crtc-fixup was called before encoder-fixup, we could have computed bpp in the crtc-fixup function and then used it in encoder-fixup. Alternatively, crtc-fixup could call into the DP code to set the link_bw, lane_count and adjusted_mode-clock values after it sets the bpp, but that seems to break the abstraction even more. Yeah, there are a few rough corners with the bpc computation in patch 2. I'll try to throw around a few ideas that crossed my mind while reading through it in a reply there. -Daniel -- Daniel Vetter Mail: dan...@ffwll.ch Mobile: +41 (0)79 365 57 48 ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/2] drm/i915: Force explicit bpp selection for intel_dp_link_required
On Wed, 25 Jan 2012 22:50:55 +0100, Daniel Vetter dan...@ffwll.ch wrote: I think we could compute this in crtc-mode_fixup (crtc-prepare doesn't have the mode and adjusted_mode arguments). We could then store the computed bpc and dithering in one of the private fields. We'd still have to loop over all encoders, but alas ... Alas, intel_crtc_mode_fixup is called *after* the intel_dp_mode_fixup. So, we'd either need to change drm_crtc_helper, or have intel_crtc_mode_fixup call down into intel_dp.c to set the link parameters. In either case, ick. Afaics we'll still correctly fall back to 6bpc (undithered for 16bpp obviously) and hence things should keep on working. Right, the problem is that the DP link parameters will be set to support 24bpp color, so we'll use a higher clock/lane-count than strictly necessary as intel_dp_mode_fixup doesn't take the frame buffer format into consideration when computing the link values. Yeah, there are a few rough corners with the bpc computation in patch 2. I'll try to throw around a few ideas that crossed my mind while reading through it in a reply there. Thanks. I'm not happy with it either. In short, I think we can (and should) apply the simple first patch to drm-intel-fixes so that at least displays work consistently, and then come up with a nicer patch that computes correct link parameters, and also supports 10bpc formats. -- keith.pack...@intel.com pgpXJSBxJAAzP.pgp Description: PGP signature ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
