From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 2/2] drm/i915: fixup fb bpp computation in pipe_config_set_bpp Date: Thu, 28 Mar 2013 17:13:03 +0200 Message-ID: <20130328151303.GO4469@intel.com> References: <20130328115959.GM4469@intel.com> <1364474967-31747-1-git-send-email-daniel.vetter@ffwll.ch> <1364474967-31747-2-git-send-email-daniel.vetter@ffwll.ch> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Return-path: Received: from mga14.intel.com (mga14.intel.com [143.182.124.37]) by gabe.freedesktop.org (Postfix) with ESMTP id 24302E6303 for ; Thu, 28 Mar 2013 08:13:28 -0700 (PDT) Content-Disposition: inline In-Reply-To: <1364474967-31747-2-git-send-email-daniel.vetter@ffwll.ch> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: Intel Graphics Development List-Id: intel-gfx@lists.freedesktop.org On Thu, Mar 28, 2013 at 01:49:27PM +0100, Daniel Vetter wrote: > Ville pointed out that my assumption that no unsupported pixel format > can get past the pipe config computation stage to the platform > update_plane callbacks is wrong. The reason is that this function > still checks the old fb->depth value instead of the new pixel_format. > = > While checking with all the other places that use this I've noticed > that intel_framebuffer_init already has all the platform checks we > need, so replace those checks with a WARN_ON. > = > Since fb->depth isn't set for YUV pixel formats and since we already > can't create an fb with an rgb layout not support on the running > platform I /think/ this patch doesn't fix any bug. Yeah I tried to go over the possible combinations of supported formats and came to the same conclusion. The bpp=3D0 thing for YUV saved us, and otherwise the sprite RGB formats always seem to be a subset of the primary plane formats. At least it's now safe against someone getting the urge to populate depth/bpp for YUV formats. > But it surely looks better! > = > Cc: Ville Syrj=E4l=E4 > Signed-off-by: Daniel Vetter > --- > drivers/gpu/drm/i915/intel_display.c | 28 ++++++++++++++++++---------- > 1 file changed, 18 insertions(+), 10 deletions(-) > = > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/= intel_display.c > index 8cbb65c..86130a2 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -7394,23 +7394,31 @@ pipe_config_set_bpp(struct drm_crtc *crtc, > struct drm_connector *connector; > int bpp; > = > - switch (fb->depth) { > - case 8: > + switch (fb->pixel_format) { > + case DRM_FORMAT_C8: > bpp =3D 8*3; /* since we go through a colormap */ > break; > - case 15: > - case 16: > + case DRM_FORMAT_XRGB1555: > + case DRM_FORMAT_ARGB1555: > + /* checked in intel_framebuffer_init already */ > + if (WARN_ON(INTEL_INFO(dev)->gen > 3)) > + return -EINVAL; > + case DRM_FORMAT_RGB565: > bpp =3D 6*3; /* min is 18bpp */ > break; > - case 24: > + case DRM_FORMAT_XRGB8888: > + case DRM_FORMAT_ARGB8888: > + case DRM_FORMAT_XBGR8888: > + case DRM_FORMAT_ABGR8888: If you want to be entirely pedantic XBGR8888/ABGR8888 should also have the 'gen < 4' check. > bpp =3D 8*3; > break; > - case 30: > - if (INTEL_INFO(dev)->gen < 4) { > - DRM_DEBUG_KMS("10 bpc not supported on gen2/3\n"); > + case DRM_FORMAT_XRGB2101010: > + case DRM_FORMAT_ARGB2101010: > + case DRM_FORMAT_XBGR2101010: > + case DRM_FORMAT_ABGR2101010: > + /* checked in intel_framebuffer_init already */ > + if (WARN_ON(INTEL_INFO(dev)->gen < 4)) > return -EINVAL; > - } > - > bpp =3D 10*3; > break; > /* TODO: gen4+ supports 16 bpc floating point, too. */ > -- = > 1.7.10.4 -- = Ville Syrj=E4l=E4 Intel OTC